Uh oh!
There was an error while loading. Please reload this page.
feat(cli): confirmed second Ctrl+O/Ctrl+T collapses blocks stranded above the viewport (#4011) - #4025
Conversation
jackwener
left a comment
There was a problem hiding this comment.
I reviewed exact head fd9cc61091a826235eb7ee59df031fb4a889cc2c. The off-screen expansion state is derived from the real renderer geometry, and the second-press path correctly applies the collapsed default to all entries instead of toggling it again. I found one P2 usability issue on the normal user path and one P3 consistency gap.
[P2] Start the confirmation window when the proposal can be acted on, or do not reverse the action after it expires
The first collapse records expansionCollapseConfirm.at = Date.now() before the notice is rendered (packages/cli/src/pi-tui-runner.ts:3415-3433). The notice asks the user to read a fairly long explanation and press again within two seconds (packages/cli/src/pi-transcript.ts:643-650). If the user reads it and presses after those two seconds, the input silently falls through to the ordinary toggle path. That path changes the default back to expanded and expands the visible cards—the opposite of the collapse the user just accepted.
This is recoverable, so it is P2 rather than P1, but it is the exact interaction this PR introduces. Please either allow a realistic reading window, start the deadline when the notice is rendered, or make the first press after expiry present the proposal again instead of performing the opposite toggle.
[P3] Document the intentional exception to the inert-geometry guard
applyExpansionDefaultToAll() is the only expansion mutation without the togglesInert() guard used by the other paths (pi-transcript.ts:512-513, 546-552, 566-577, 630-632). A confirmed full redraw may make that safe by design, but a short comment should state that this path intentionally operates even after a wholesale transcript replacement leaves positions unknown.
The key-repeat guard, cross-key disarming, renderer geometry, and full redraw behavior otherwise look sound. Exact-head test and label checks succeeded, and the current-main merge is clean. I did not approve because the P2 remains live.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
…primitives (apache#4011) A Ctrl+O/Ctrl+T collapse can strand expanded blocks above the live viewport: their heads sit in terminal scrollback, which the apache#1097 contract forbids rewriting, so they stay expanded with no way back (apache#1134). apache#1140 made the all-stranded case explained but left the far more common partial case silent — some cards collapse, some stay stuck, and the keypress reads as broken. The toggle now appends a notice whenever a collapse strands blocks, naming the count and offering the escape hatch: a second press within EXPANSION_COLLAPSE_CONFIRM_WINDOW_MS collapses them too via one knowingly-accepted, scrollback-clearing full redraw (apache#1134 option 2, deferred by apache#1140 as an orthogonal follow-up). The confirm offer exists for collapses only — collapsed blocks above the viewport are compact and harmless, and arming on expand would make a quick expand-then- collapse pair read the second press as "expand everything". New exports for the runner wiring: hasExpandedEntriesAboveViewport (arming predicate) and applyExpansionDefaultToAll (applies the current default to every entry including above-viewport ones, without flipping the default the way a plain toggle would). The window constant lives beside the notice copy so the offer text and the runner's confirm window share one authority. Generated-by: OpenCode
…e stranded blocks (apache#4011) Wire the confirm gesture into the key handler: a collapse toggle that leaves expanded blocks above the viewport arms a 2s window; pressing the same key again inside it applies the collapsed default to every entry and forces one scrollback-clearing full redraw through pi-tui's existing public requestRender(true), which re-anchors the viewport at the tail. Transcript content is fully re-rendered into fresh scrollback, so nothing from the session is lost; only pre-session shell scrollback is cleared, and only after the explicit second press the notice announced. The integration test drives the real renderer on a 24-row terminal: an 80-line thinking block is expanded past the viewport, the first collapse press renders the offer with no ESC[3J in the stream, and the confirmed second press emits exactly the deliberate clear while collapsing the block back to its compact row. Generated-by: OpenCode
fd9cc61 to
3dd60a7CompareUh oh!
There was an error while loading. Please reload this page.
…bove the viewport (apache#4011) (apache#4025) * feat(cli): name stranded expansion blocks and add confirmed-collapse primitives (apache#4011) A Ctrl+O/Ctrl+T collapse can strand expanded blocks above the live viewport: their heads sit in terminal scrollback, which the apache#1097 contract forbids rewriting, so they stay expanded with no way back (apache#1134). apache#1140 made the all-stranded case explained but left the far more common partial case silent — some cards collapse, some stay stuck, and the keypress reads as broken. The toggle now appends a notice whenever a collapse strands blocks, naming the count and offering the escape hatch: a second press within EXPANSION_COLLAPSE_CONFIRM_WINDOW_MS collapses them too via one knowingly-accepted, scrollback-clearing full redraw (apache#1134 option 2, deferred by apache#1140 as an orthogonal follow-up). The confirm offer exists for collapses only — collapsed blocks above the viewport are compact and harmless, and arming on expand would make a quick expand-then- collapse pair read the second press as "expand everything". New exports for the runner wiring: hasExpandedEntriesAboveViewport (arming predicate) and applyExpansionDefaultToAll (applies the current default to every entry including above-viewport ones, without flipping the default the way a plain toggle would). The window constant lives beside the notice copy so the offer text and the runner's confirm window share one authority. Generated-by: OpenCode * feat(cli): second Ctrl+O/Ctrl+T press pays one full redraw to collapse stranded blocks (apache#4011) Wire the confirm gesture into the key handler: a collapse toggle that leaves expanded blocks above the viewport arms a 2s window; pressing the same key again inside it applies the collapsed default to every entry and forces one scrollback-clearing full redraw through pi-tui's existing public requestRender(true), which re-anchors the viewport at the tail. Transcript content is fully re-rendered into fresh scrollback, so nothing from the session is lost; only pre-session shell scrollback is cleared, and only after the explicit second press the notice announced. The integration test drives the real renderer on a 24-row terminal: an 80-line thinking block is expanded past the viewport, the first collapse press renders the offer with no ESC[3J in the stream, and the confirmed second press emits exactly the deliberate clear while collapsing the block back to its compact row. Generated-by: OpenCode * fix(cli): re-offer expired expansion confirmation
Summary
Fixes#4011 — implements option 2 of #1134 (the explicit redraw command deferred by #1140 as an orthogonal follow-up).
Today a
Ctrl+O/Ctrl+Tcollapse can strand expanded blocks above the live viewport: their heads sit in terminal scrollback, which the #1097 contract forbids rewriting, so they can never be collapsed again (#1134). #1140 made the fully-stranded case explained, but the common partial case — some cards collapse, the rest stay stuck — is silent, so the toggle reads as broken.This PR turns the #1097 tradeoff into an informed per-press choice:
EXPANSION_COLLAPSE_CONFIRM_WINDOW_MSapplies the collapsed default to every entry — without flipping the default the way a plain toggle would — and pays one deliberate scrollback-clearing full redraw through pi-tui's existing publicrequestRender(true), which re-anchors the viewport at the tail. Transcript content is fully re-rendered into fresh scrollback, so nothing from the session is lost; only pre-session shell scrollback is cleared, and only after the explicit second press.No pi-tui patch or upstream change is needed:
requestRender(force)already exposes the full-render path (resetRenderState→ width-change branch →ESC[2J ESC[H ESC[3J+ full rewrite), and the layout's viewport shadow predicts the same re-anchor (firstChanged < current → tailTop).Verification
packages/cli:npm test546/546 — 4 new unit tests (mixed-position collapse notice, confirmed collapse does not flip the default and is idempotent, expand direction stays silent, inert-window predicate) and 1 new integration test.runMakaPiTuion a 24-row terminal: an 80-line thinking block is expanded past the viewport, the first collapse press renders the offer with noESC[3Jin the output stream, and the confirmed second press emits the deliberate clear exactly once while the block returns to its compactThinking…row. (Tool cards window their expanded rendering —⋯ N lines hidden ⋯— so a thinking block is what can actually push its head past the viewport; this matches how fix(cli): flip the expansion default and explain when Ctrl+O/Ctrl+T finds no block in view #1140 verified fix(cli): a block expanded past the viewport cannot be collapsed again #1134.)npm run lint,npm run format:check,packages/clitypecheck, andcheck:asf-headersall clean.packages/cliis touched).AI use
Tool(s) and scope: OpenCode (kimi k3-256k) drafted the implementation and tests under human direction; the approach (second-press confirm, collapse-only arming) was reviewed and decided interactively. Commits carry
Generated-by: OpenCodetrailers.Checklist
Does this PR entail a change in behavior?