fix(cli): give rewind selections visible feedback and preserve mid-switch input - #3475
Conversation
|
在
本地验证:CLI 340/340、CLI typecheck、Biome(两个改动文件)和 |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for making rewind visibly pending and preserving newer editor input instead of overwriting it. Two independent exact-head passes found three remaining boundaries where the final-state contract is not yet complete; I have consolidated them inline so the fixes can stay focused in the existing TUI input/activity seams.
AI-assisted review disclosure: OpenAI Codex coordinated two independent exact-head reviews. I verified the retained activity-lease, resumed-history, and bracketed-paste paths, plus the live PR and CI state, and I made the final review decision.
| }); | ||
| text: '正在回退到该轮之前…', | ||
| }; | ||
| state.entries.push(pendingNotice); |
There was a problem hiding this comment.
[P2] This notice is synchronous only after runControl() has already acquired the session activity lease. If another Host operation holds that lease, selecting Rewind still waits with no TUI-body feedback — the lease-blocked path called out in #3383. Could the notice be emitted before lease acquisition, or could runControl expose a synchronous started hook? A test that pre-reserves the session activity should see the notice before the lease is released.
There was a problem hiding this comment.
Agreed — withdrawing acknowledged. The synchronous busy guard ahead of runControl plus BusyAfterPickerOpenDriver cover this shape; no change made for this point.
| level: 'info', | ||
| text: refill | ||
| ? '已回退到该轮之前(分支为新任务,原任务保留),该轮 prompt 已回填输入框,可修改后重新发送。' | ||
| : '已回退到该轮之前(分支为新任务,原任务保留)。输入框已有未发送内容,未回填该轮 prompt(可按 ↑ 从输入历史找回)。', |
There was a problem hiding this comment.
[P2] The “press ↑” recovery promise is not reliable for a session entered through startup resume or /resume: editor history is populated only by prompts submitted in this TUI process, so the rewound result.prompt may never be present. If the user typed a newer draft during rewind, the old prompt is then neither refilled nor recoverable as advertised. Could we explicitly add the rewound prompt to editor history (or keep a switchable draft stash) and cover a resumed-session rewind?
There was a problem hiding this comment.
Fixed in dee251e. rewindToTurn now records the rewound prompt via editor.addToHistory(result.prompt) unconditionally, before deciding on the refill. The live path is a no-op (addToHistory dedupes consecutive duplicates), and a session entered through startup resume or /resume — which had no history entry at all — now gets one, so the notice's ↑ promise is true in every case. The wording was also tightened to state exactly what happened: input kept untouched, prompt recorded in history.
Test coverage: keeps a draft typed while the rewind is in flight instead of overwriting it now presses ↑ after completion (twice — the first ↑ only jumps to line start while the cursor is past col 0) and asserts the editor shows the rewound prompt. That test submits nothing before the rewind, so it exercises exactly the resumed-session shape you described.
| // was in flight. The picker's neutral-editor guarantee only holds at open | ||
| // time, so a non-empty draft here is newer user work and wins; the prompt | ||
| // stays recoverable from the editor history (submitPrompt added it). | ||
| const refill = editor.getText().trim().length === 0; |
There was a problem hiding this comment.
[P2] editor.getText() can still be empty while StdinBuffer is holding an incomplete bracketed paste (start marker received, end marker not yet received). This sets refill = true; when the paste completes, its content is appended to the rewound prompt, producing unintended combined input. Could the input seam expose a pending-paste/dirty generation and let that newer input win, with a split ESC[200~ … ESC[201~ regression test across the rewind completion?
There was a problem hiding this comment.
Fixed in dee251e. editorPastePending is now tracked at the global input listener (the seam every chunk crosses before focus dispatch), mirroring the editor's own per-chunk marker matching — start marker without an end marker in the same chunk sets it, the end marker clears it, and bytes after the end marker may re-arm it. The refill condition is now editor.getText().trim().length === 0 && !editorPastePending, so a paste still being buffered wins over the refill and lands alone when its end marker arrives.
I kept this in Maka's input seam rather than exposing pasteBuffer from pi-tui: the field is private in the package, and mirroring the editor's exact matching logic keeps the flag consistent with what the editor will buffer without patching upstream.
Test coverage: new lets a bracketed paste still being buffered win over the prompt refill splits ESC[200~pasted half / rest ESC[201~ around the rewind completion and asserts the editor ends up with exactly pasted half rest and no refilled content.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks @me2seeks — following up at the same head, 9910007ef82a7776364b183b93790fd724a468d2.
Withdrawing my earlier first point. I previously asked whether the busy notice could be emitted before lease acquisition, on the basis that the notice inside rewindToTurn only fires after runControl() has taken the session activity lease. That reading was incomplete: the picker's selection callback already has a synchronous guard ahead of runControl, which pushes 无法回退:当前有正在进行的操作 and returns without ever attempting the lease. That is exactly the shape I was asking for, and BusyAfterPickerOpenDriver covers the case where a Host operation takes busy while the picker is open. My apologies for the noise — that point is resolved and needs no change.
The other two points still stand at this head, and I am not repeating their details since the inline threads are still open. One clarification on severity, and one new minor note:
On the /resume history gap — keeping this at P2 rather than lowering it. The prompt is only pushed to history along the live path, so a session entered through /resume has no entry to recall. Resuming is an ordinary entry point rather than an unusual configuration, and the UI states that the prompt can be recovered with ↑ — so the failure is a promise the product makes and does not keep. For a prompt that may represent a long piece of work, and with no other way for the user to get it back, that is a real loss rather than a cosmetic gap.
[P3] The draft guard is a single condition rather than per-target state. Preservation keys off editor.getText().trim().length === 0, i.e. "there is already a draft, so do not overwrite". That prevents clobbering across a single switch, but it means the retained content is whatever happened to be in the editor rather than something associated with the target being switched away from. With repeated switches the user has no way to get back to an earlier target's text. Not a functional block, and the current behaviour is the safe direction — noting it because the accompanying wording implies more recall than the mechanism provides.
This review was AI-assisted. It is not a substitute for independent human review by a committer.
…ed pastes Review follow-ups on apache#3475 (apache#3383): - add the rewound prompt to the editor history unconditionally: prompts submitted in this TUI process were already covered (addToHistory dedupes consecutive duplicates), but a session entered via startup resume or /resume had no entry, so the draft-preserved notice promised a ↑ recovery it could not deliver; - treat a bracketed paste still being buffered as newer user input too: between the start and end markers getText() stays empty, so the refill check saw an empty editor and the completing paste would have been appended to the refilled prompt. The global input listener now tracks paste buffering with the same per-chunk marker matching the editor uses, and the refill requires no pending paste; - reword the draft-preserved notice to state exactly what happened (input kept, prompt recorded in history). Tests: ↑ recalls the rewound prompt after a draft-preserving rewind even with no in-process submission, and a paste split across the rewind completion lands alone instead of being concatenated onto the refill.
…ed pastes Review follow-ups on apache#3475 (apache#3383): - add the rewound prompt to the editor history unconditionally: prompts submitted in this TUI process were already covered (addToHistory dedupes consecutive duplicates), but a session entered via startup resume or /resume had no entry, so the draft-preserved notice promised a ↑ recovery it could not deliver; - treat a bracketed paste still being buffered as newer user input too: between the start and end markers getText() stays empty, so the refill check saw an empty editor and the completing paste would have been appended to the refilled prompt. The global input listener now tracks paste buffering with the same per-chunk marker matching the editor uses, and the refill requires no pending paste; - reword the draft-preserved notice to state exactly what happened (input kept, prompt recorded in history). Tests: ↑ recalls the rewound prompt after a draft-preserving rewind even with no in-process submission, and a paste split across the rewind completion lands alone instead of being concatenated onto the refill.
67ba73f to
dee251e
Compare
|
Pushed
Full |
…itch input A rewind selection closed the picker and then stayed silent for the whole branch-and-switch chain (~6-8 serialized runtime-host round trips), while control-busy renders nothing in the TUI body (apache#3383): - push a 正在回退到该轮之前… notice synchronously on selection; the transcript replacement wipes it on success and the failure path splices it out so only the error remains; - keep an editor draft typed while the switch is in flight instead of letting editor.setText clobber it — the picker's neutral-editor guarantee only holds at open time; the rewound prompt stays recoverable from the editor history; - refuse a selection with an error notice when busy is already held (e.g. a Goal auto-continuation started while the picker was open) — matching /goal's explicit refusal instead of runControl's silent early return. Generated-by: Maka
- an in-progress notice appears while the branch is being created and is gone once the branch lands; - a draft typed while the switch is in flight survives completion (Enter stays swallowed by disableSubmit) instead of being overwritten by the refilled prompt; - selecting a target while another action holds busy surfaces the refusal notice instead of dropping the rewind silently. Generated-by: Maka
…ed pastes Review follow-ups on apache#3475 (apache#3383): - add the rewound prompt to the editor history unconditionally: prompts submitted in this TUI process were already covered (addToHistory dedupes consecutive duplicates), but a session entered via startup resume or /resume had no entry, so the draft-preserved notice promised a ↑ recovery it could not deliver; - treat a bracketed paste still being buffered as newer user input too: between the start and end markers getText() stays empty, so the refill check saw an empty editor and the completing paste would have been appended to the refilled prompt. The global input listener now tracks paste buffering with the same per-chunk marker matching the editor uses, and the refill requires no pending paste; - reword the draft-preserved notice to state exactly what happened (input kept, prompt recorded in history). Tests: ↑ recalls the rewound prompt after a draft-preserving rewind even with no in-process submission, and a paste split across the rewind completion lands alone instead of being concatenated onto the refill. Generated-by: Maka
dee251e to
2ef8efa
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the three points from the previous round are each answered on their own terms, and the way you answered them is worth naming. You withdrew one on the merits rather than changing code to satisfy it, you fixed one by widening a guarantee instead of narrowing it (addToHistory unconditionally, so the ↑ promise holds for /resume sessions that never had a history entry), and you fixed the third at the seam every chunk actually crosses rather than at the point where the symptom showed up.
Reviewed at exact head 2ef8efabbdab9bd1bc605b517fbc91e678c71328 against base 8b60ddffa89682c03238ddce6595f531eb2b6f29. No checks have run on this head — not green, not red, simply never executed — so this stays a COMMENT regardless of how the code reads. One P3 and one design note below; no P0, P1 or P2.
Re-verified against this head, not the head your replies were written on:
- Lease-ahead notice — you withdrew it, and the withdrawal holds. The synchronous busy guard sits ahead of
runControlin the same tick, so there is no window between the check and the control taking effect, andBusyAfterPickerOpenDrivercovers the shape. /resume↑ promise —editor.addToHistory(result.prompt)runs unconditionally before the refill decision. The live path is a no-op because consecutive duplicates dedupe; the resume path, which previously had no entry at all, now gets one. The test asserting recall with nothing submitted in-process pins exactly that case.- Bracketed paste race — the mirror's per-chunk marker matching tracks the editor's own
isInPaste/pasteBufferbehaviour, including re-arming on bytes after an end marker. The test splittingESC[200~/ESC[201~pins paste winning over refill.
[P3] paddingX: 0 → 1 is unrelated to this change. One line of visual adjustment, not mentioned in the description, in a PR that is otherwise about rewind feedback and refill safety. Not blocking — please confirm it is deliberate, or drop it so the diff stays about one thing.
Design note, not a finding. The paste mirror necessarily reimplements marker matching that lives in pi-tui's editor as private state, and you have already explained why (the field is private and patching upstream is out of scope for this PR). That reasoning is sound. The cost is that the two can drift silently: if the editor's matching ever changes, nothing here fails. If a comment naming pi-tui's version and the specific behaviour being mirrored is cheap to add, it would give the next person reading this a thread to pull.
One thing we did not do: we verified statically against the pi-tui source and this repository, and did not execute the test suite. Given that no CI has run on this head either, the change currently has no executed verification behind it at all. Getting a CI run on this head is worth more than anything else on this list.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.
Astro-Han
left a comment
There was a problem hiding this comment.
Following up now that CI has run on this head — test is green.
Still at 2ef8efabbdab9bd1bc605b517fbc91e678c71328, unchanged since my earlier review. No P0–P2; the only open item is the P3 I noted then, the unrelated paddingX: 0 → 1. That doesn't block, so I'm approving — please drop it or confirm it's deliberate whenever you next touch the branch.
Approving.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are mine to correct — please push back where I got it wrong.
Closes #3383.
Problem
Selecting a turn in the Esc-Esc Rewind picker closed the overlay and then stayed silent for the whole branch-and-switch chain (~6–8 serialized runtime-host round trips), while control-busy renders nothing in the TUI body. In that window Enter was silently swallowed (
disableSubmit), typed text / ↑ history recall accumulated in the editor — and were then clobbered byeditor.setText(result.prompt)when the rewind landed. If busy was claimed while the picker was open (e.g. a Goal auto-continuation starting a turn),runControl's early return dropped the rewind with no notice at all. Net effect: "nothing happened", or it happened a beat later with my input gone (#3383).Changes (
packages/cli/src/pi-tui-runner.ts)rewindToTurnpushes a正在回退到该轮之前…notice synchronously before its first await, so the Enter keypress is visibly accepted immediately. On success the transcript replacement wipes it; on failure the catch splices it out so only the error notice remains.submitPromptadded it). The completion notice says which variant happened.onSelectrefuses with an error notice whenbusyis already held — same contract as/goal's guard at the formerrunControlsilent early return.Tests (
packages/cli/src/__tests__/pi-tui-runner.test.ts)shows an in-progress notice while the rewind branch is being created— deferred driver gate proves the notice renders while the switch is still in flight, and disappears once the branch lands;keeps a draft typed while the rewind is in flight instead of overwriting it— draft survives completion, Enter stays swallowed (driver.promptsstays empty), no refill happens;refuses a rewind selection with a notice when another action claimed busy— a Host-started turn claims busy under the open picker (the Goal-continuation shape); selection surfaces the refusal andrewoundstays empty.Verification
packages/cli: full suite green — 340/340 pass (includes all 8 rewind tests);tsc -p packages/cliclean;biome checkclean on both touched files.Out of scope (noted in the issue)
listRewindTargetsalready loaded to avoid the re-fetch insiderewindToTurn(perf-only follow-up).AI use
Tool(s) and scope: Maka (AI coding agent) authored the implementation and tests; the diff was human-reviewed before push.
Generated-by: Makatrailers are present on the branch commits.