feat(tui): add optional message timestamps - #1206
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Maintainer gate note: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f39ee77dae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // First delta only. updateContent runs once per streaming chunk, so stamping | ||
| // unconditionally would march the clock forward while the message is still | ||
| // being written and land on the completion time rather than the start. | ||
| this.arrivedAt ??= new Date(); |
There was a problem hiding this comment.
When a saved session is resumed, reloaded, or otherwise rebuilt with timestamps enabled, constructing each component immediately calls updateContent(message), so this line stamps every historical message with the current reconstruction time and ignores the persisted message.timestamp. As a result, old messages display the resume/reload time—and can change again after settings-triggered transcript rebuilds—instead of a stable historical time; reconstructed components need to derive or restore a persisted arrival value while empty streaming components can continue stamping their first update.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intentional for the contract in #1205: arrivedAt is the time a component first receives content in the current TUI, not the provider/model timestamp persisted on the message. Reusing message.timestamp would change that meaning, and the feature deliberately avoids adding persisted transcript schema. A restored or reloaded transcript is reconstructed into the current UI and receives its current UI-arrival time; live updates remain fixed by arrivedAt ??=. The regression suite locks the two-update stability contract.
| setShowTimestamps(show: boolean): void { | ||
| if (this.showTimestamps === show) return; | ||
| this.showTimestamps = show; | ||
| this.invalidate(); |
There was a problem hiding this comment.
When this setting is toggled in a long transcript, InteractiveMode calls this method for every assistant component, and invalidate() clears renderDescriptors before refreshContent(), causing reconciliation to dispose and recreate every Markdown/Text child even though only the prefix changed. This makes a simple settings toggle synchronously rebuild the complete assistant tree and can stall large sessions; clear only the outer render cache here rather than invoking the full content invalidation path.
AGENTS.md reference: packages/coding-agent/src/modes/interactive/components/AGENTS.md:L41-L41
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head. setShowTimestamps() and setTimestampEligible() now invalidate only the outer render cache; they no longer clear or reconcile content descriptors. The regression test captures the memoized content children, toggles both states, and asserts object identity is unchanged.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 320cd5b482
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const prefix = this.timestampPrefix(); | ||
| const prefixWidth = visibleWidth(prefix); | ||
| const showTimestamp = prefixWidth > 0 && width > prefixWidth; | ||
| const lines = super.render(showTimestamp ? width - prefixWidth : width); |
There was a problem hiding this comment.
When timestamps are enabled, this subtracts the nine-column prefix from the width used to render the entire assistant component, but the prefix is added only to the first non-empty line. Every continuation line therefore wraps nine columns early while still starting at column zero, unnecessarily increasing transcript height and narrowing tables or code blocks; only the prefixed line should surrender that width.
AGENTS.md reference: packages/coding-agent/src/modes/interactive/components/AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The narrower Markdown render is intentional and follows #1205's stated contract to reserve the visible prefix width before Markdown rendering. Using one width for the component keeps Markdown/table/code wrapping internally consistent and provides a simple hard width bound; the prefix is still attached only to the first non-empty line. Boundary tests cover widths 9–12 and real TUI QA covers 120x34 to 80x24 resize.
| this.hiddenThinkingLabel, | ||
| this.outputPad, | ||
| this.getMarkdownTransformers(), | ||
| this.showMessageTimestamps, |
There was a problem hiding this comment.
When a streamed assistant response contains text after a tool call, the head component already has a timestamp, but every synthetic trailing-text segment created here also receives timestamping and records a separate arrival time. The live transcript consequently shows multiple timestamps for one assistant message, while reconstructing the same persisted message produces one component and one timestamp; pass timestamp ownership only to the head component rather than each trailing segment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head. Trailing segments share the streaming head arrivedAt, start ineligible, and syncStreamingMessageTimestampEligibility() assigns exactly one visible owner across the head and sorted segments. The tool-separated regression asserts exactly one timestamp and the tool-first regression asserts the original arrival time is retained.
|
@codex review Please review exact head |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@code-yeongyu Final contributor gate note for exact head
Please merge #1203 first if that is the intended main repair, then apply the changelog action here. I will update this branch to the repaired main, rerun exact-SHA gates, and keep this PR on merge-commit delivery. |
|
@codex review Please review exact head |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review Please review exact head |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review Please review exact head |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@code-yeongyu Exact-head gate update for
Once those two maintainer-owned gates are green, please merge with a merge commit; this branch already contains a two-parent merge from current main. |
|
@codex review\n\nExact candidate: |
|
Maintainer action required (@code-yeongyu) for exact head
AI authorship is disclosed in the PR body, and this repository has no |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1e3c37b75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let assigned = false; | ||
| for (const component of [this.streamingComponent, ...segments]) { | ||
| const eligible = !assigned && component.hasVisibleContent(); | ||
| component.setTimestampEligible(eligible); |
There was a problem hiding this comment.
When timestamps and the default smooth streaming are enabled, the first target is initially rendered with zero revealed units, so hasVisibleContent() is false and this loop disables the head component's timestamp eligibility. Later reveal ticks call only updateContent() and request a render, never this synchronization method, so the response remains visibly unprefixed throughout streaming and gains its timestamp only at message_end. Preserve eligibility for the streaming head or resynchronize when the first visible unit is revealed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head. When all components are still empty, syncStreamingMessageTimestampEligibility() now keeps the streaming head as the fallback owner. A real StreamingRevealController regression starts with an empty paced frame, reveals visible content without another eligibility sync, and asserts the timestamp appears on that first reveal; built-dist smooth-stream QA also observes the timestamped partial marker before completion.
Preserve timestamp ownership through the initial empty smooth-stream frame.
Refresh the feature branch against the latest compaction and model-selection changes.
Refresh the feature branch with the latest PTY cancellation fixture.
1 participant
Summary
Message timestampsrow to/settingsHH:MM:SSupdateContent()only, so streaming updates and resize rerenders keep the original arrival timeCloses #1205.
Why this is in the built-in TUI
The extension API cannot add a native
/settingsrow or change construction/rendering of every built-in assistant message component. The nearestchanges.mdtrackers document all four upstream-owned production paths and the expected merge-conflict zones.Verification
npm install --ignore-scriptsnpm run check??=with=makes the two-update arrival-time regression failscripts/check-pr-changelog.mjswithno-changelog: 4/4 upstream-owned production paths coveredscripts/audit-changes-md.mjs: 253/253 divergent paths covered, 0 uncovered@code-yeongyu/senpi@2026.8.28-2QA: syntax, first-arrival behavior, idempotence, rollback, partial-state refusal, and real packaged TUI rendering passed on the candidate and two macOS targetsAI authorship disclosure
I used OpenAI Codex to help implement and test this change and to draft the issue/PR text. I reviewed the code, understand the settings/streaming/rendering/reload interactions, and verified the behavior through unit, mutation, and real CLI QA. I read
CONTRIBUTING.md; this repository currently has noCLA.mdor CLA workflow.Summary by cubic
Adds a default-off
Message timestampsrow to/settingsthat prefixes the first non-empty assistant content line with localHH:MM:SSarrival time. Previously assistant messages never showed when they arrived; now operators can opt in, the setting persists across sessions, and timestamps survive streaming updates, resize rerenders, transcript reloads, and upstream merges. Closes #1205.New Features
Written for commit d6606bf. Summary will update on new commits.