fix(tui): prevent viewport jump when thinking finalizes above viewport - #1141
fix(tui): prevent viewport jump when thinking finalizes above viewport#1141starSumi wants to merge 9 commits into
Conversation
🦋 Changeset detectedLatest commit: bb81d31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![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: 3cfd3a6df6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (child instanceof ThinkingComponent) { | ||
| if ((child as ThinkingComponent).compact()) { | ||
| this.host.state.ui.requestRender(); | ||
| } | ||
| break; |
There was a problem hiding this comment.
When transcriptContainer already contains an older ThinkingComponent (for example after a previous turn or replayed message), this loop stops on that older finalized block; compact() returns false, but _pendingThinkingCompact has already been cleared and the loop still breaks. The just-finalized stable-mode block later in the transcript is then never compacted, leaving it in the live-shaped thought view instead of the finalized preview.
Useful? React with 👍 / 👎.
| if (state.appState.streamingPhase === 'idle') return; | ||
| this.host.deferUserMessages = false; | ||
| // Last-chance compact in case no assistant text triggered it | ||
| this.compactThinkingIfPending(); |
There was a problem hiding this comment.
For a turn that ends while thinking is still active and no assistant text has started, _pendingThinkingCompact is still false at this point, so this call is a no-op; finalizeLiveTextBuffers() below then calls onThinkingEnd() and sets the pending flag after the last-chance compact has already passed. That leaves no-assistant thinking blocks in stable/live-shaped rendering after turn completion and leaks the pending compact into a later event.
Useful? React with 👍 / 👎.
|
Follow-up pushed in
Validation:
Transport note: HTTPS and explicit SSH @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb139392d2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // After finalizeLiveTextBuffers, onThinkingEnd may have set | ||
| // _pendingThinkingCompact. Compact now so the thinking block | ||
| // reaches its final compact form before the turn ends. | ||
| this.compactThinkingIfPending(); |
There was a problem hiding this comment.
When a turn goes from thinking.delta directly to tool.call.started or tool.call.delta, the tool-call paths first call finalizeLiveTextBuffers('tool'), which now sets _pendingThinkingCompact, but they never consume it before adding the tool UI. The first compaction then happens here in finalizeTurn() after the tool/result has already rendered, so the thinking block can shrink above the viewport with no newly-added content below and recreate the same scroll-jump this patch is trying to avoid. Please also compact the pending thinking block before rendering/flushing the tool component.
Useful? React with 👍 / 👎.
| // identical to live mode, preventing a destructive fullRender when | ||
| // this component is above the viewport (fixes #981). | ||
| this._activeThinkingComponent.finalize(); | ||
| this._pendingThinkingCompact = true; |
There was a problem hiding this comment.
onThinkingEnd() is also used by SessionReplayRenderer.flushAssistant(). For replayed assistant messages that contain only think parts, or think plus tool calls with no text, replay never calls onStreamingTextStart() or finalizeTurn(), so this pending flag is never consumed and the transcript keeps rendering the block in stable live-format (thought header with only the tail lines) instead of the normal compact finalized preview. Resume/replay should either compact immediately in that path or expose a finalized mode that does not leave pending live state behind.
Useful? React with 👍 / 👎.
|
Follow-up pushed in
Regression coverage added:
Validation:
Transport note: I first tried a normal push while my local branch still had the pre-API SHA for the earlier test-only follow-up. I fetched the fork head, rebased this commit onto the actual remote @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a92658eff5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (this._thinkingDraft.length > 0 || this._streamingBlock !== null) { | ||
| this.finalizeLiveTextBuffers('tool'); | ||
| this.compactPendingThinking(); |
There was a problem hiding this comment.
When a thinking block has already been finalized by another boundary (for example handleStepBegin() calls finalizeLiveTextBuffers('waiting')), _pendingThinkingCompact can be true while both _thinkingDraft and _streamingBlock are empty. If the next event is tool.call.delta, this condition is skipped, so the tool preview is added under the stable thought live-shape block and compaction is delayed until a later turn boundary, reintroducing the stale preview/viewport-jump risk for that sequence. Call compactPendingThinking() before rendering the preview regardless of whether there is current draft/assistant text.
Useful? React with 👍 / 👎.
|
Follow-up pushed in
Validation:
Transport note: the first HTTPS push picked up an invalid stored credential. Retrying with a one-shot @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56662c0a53
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // identical to live mode, preventing a destructive fullRender when | ||
| // this component is above the viewport (fixes #981). | ||
| this._activeThinkingComponent.finalize(); | ||
| this._pendingThinkingCompact = true; |
There was a problem hiding this comment.
When a thinking block is finalized, this flag now defers the actual compacting, but several existing finalizers never drain it. For example, handleHookResult() calls flushThinkingToTranscript() and then appends the hook-result assistant entry without calling compactPendingThinking(), and interruption/error/compaction paths also call finalizeLiveTextBuffers() without a later assistant/tool/finalizeTurn drain. In those flows a long thinking block is left indefinitely in stable live-shape (thought, tail lines, no expand hint, and Ctrl+O has no effect), whereas before it finalized into the normal compact/expandable transcript entry. Please compact in those terminal/non-tool paths before adding the next transcript/status entry or before returning to idle.
Useful? React with 👍 / 👎.
When the ThinkingComponent transitions from 'live' to 'finalized' mode while above the visible viewport, its rendered line count changes (4→3 lines). This triggers pi-tui's `firstChanged < prevViewportTop` branch, which falls back to a destructive fullRender (clear-screen), causing the terminal to jump to the top. Introduce a stable transition mechanism: - `finalize()` now enters 'stable mode' that keeps the same line count as live mode (spinner replaced by static bullet, same content region) - `compact()` defers the actual line-count reduction to a safe window - StreamingUIController calls compactThinkingIfPending() at the start of assistant text streaming, so both changes land in the same render cycle and the fullRender is masked by the new content below Fixes MoonshotAI#981 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two bugs caught by Codex automated review: 1. compactThinkingIfPending() found the FIRST ThinkingComponent in transcript, which could be an older block from a previous turn. Now walks in reverse to find the most recent stable-mode instance. 2. Last-chance compaction in finalizeTurn() was called BEFORE finalizeLiveTextBuffers(), so _pendingThinkingCompact was still false when thinking was still active. Moved after finalizeLiveTextBuffers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add controller-level regressions for thinking-only turn compaction and latest pending thinking block selection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses current-head Codex P2 feedback on MoonshotAI#1141: - r3496662311: compact pending thinking before hook results, step interruptions, session errors, and compaction begin, so finalized thinking blocks are not left indefinitely in stable live-shape. - Add regression tests for hook result, step interrupted, session error, and compaction.started paths. Validation: - pnpm exec vitest run apps/kimi-code/test/tui/kimi-tui-message-flow.test.ts -t "compacts pending thinking" --reporter=dot - pnpm exec vitest run apps/kimi-code/test/tui/kimi-tui-message-flow.test.ts -t thinking --reporter=dot - pnpm exec vitest run apps/kimi-code/test/tui/message-replay.test.ts -t compacts --reporter=dot - pnpm --filter @moonshot-ai/kimi-code typecheck - pnpm exec oxlint apps/kimi-code/src/tui/controllers/streaming-ui.ts apps/kimi-code/src/tui/controllers/session-event-handler.ts apps/kimi-code/src/tui/controllers/session-replay.ts apps/kimi-code/test/tui/kimi-tui-message-flow.test.ts - git diff --check
56662c0 to
5265028
Compare
|
Follow-up pushed in
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5265028d5a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| onStreamingTextStart(): void { | ||
| // Compact thinking before adding assistant content so both changes | ||
| // land in the same render cycle (fixes #981 viewport jump). | ||
| this.compactPendingThinking(); |
There was a problem hiding this comment.
When a flushed thinking block is followed by assistant.delta, this compacts immediately, but appendAssistantDelta() only appends the delta to _assistantDraft; the AssistantMessageComponent stays empty until the later streaming flush calls onStreamingTextUpdate(). Because empty assistant messages render no lines, the first render after this line can still contain only the thinking line-count shrink, so the above-viewport clear-screen/jump that this change is meant to avoid can still occur on the normal thinking→assistant path. Move the compaction to the first assistant content update or synchronously populate the assistant block before requesting render.
Useful? React with 👍 / 👎.
|
Follow-up pushed in
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2f01c5dae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else if (thinking.length > 0) { | ||
| streamingUI.compactPendingThinking(); |
There was a problem hiding this comment.
When a persisted assistant record has thinking plus a whitespace-only text part, this else branch is skipped, and onStreamingTextUpdate() also refuses to compact because fullText.trim() is empty. If that replayed message is followed by tool calls, renderToolCalls() adds the tool card without compacting, so resumed transcripts keep the thinking block in stable thought mode with the live tail preview and no expand footer, unlike the live tool-boundary path that compacts before rendering tools.
Useful? React with 👍 / 👎.
|
Follow-up pushed in
Validation:
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Summary
When
ThinkingComponenttransitions from live to finalized above the viewport, its line count changes (4→3), triggering pi-tui's destructivefullRender(true)which clears the screen and jumps to the top.Fix: Stable transition mechanism —
finalize()keeps live-format line count,compact()defers the reduction to a safe render cycle when assistant content also changes.Root Cause
Solution
thinking.ts— Stable modefinalize()enters stable mode: stops spinner, keeps same line shape as livecompact()defers actual line-count reduction, returns booleanrender()usesmode === 'live' || stableModefor identical outputstreaming-ui.ts— Deferred compactiononThinkingEnd()→finalize()(stable, not compact)compactThinkingIfPending()called atonStreamingTextStart()— both changes in same render cyclefinalizeTurn(), cleanup inresetLiveText()thinking.test.ts— Updated + 3 new testsWhy it works
Testing
Related