Workspaces: final transfer fixes and unified kill confirmation - #630
Conversation
Deploying mouseterm with
|
| Latest commit: |
89601dd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://20aef467.mouseterm.pages.dev |
| Branch Preview URL: | https://workspaces-transfer-fixes.mouseterm.pages.dev |
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress — not a merge verdict. Mark the PR ready when you want the full review.
The grid and mouse-encoding fixes trace cleanly: the source grid rides the payload, resumeLivePtys hands it to setupTerminalEntry before any replay is written, and the unconditional flushTerminal drain now runs whether or not a pin travelled. Three things worth deciding before this lands.
The private-internal read strands the transfer rather than degrading it. serializeTransferTerminal dereferences _core.mouseStateService.activeEncoding unguarded. If that path is ever absent — a beta rename in @xterm/xterm, a test double, a Terminal-like stand-in — it throws a TypeError, and the throw escapes captureTransferContent into handOff at the await captureTransferContent(terminalIds, marks) line, which sits outside the only try there (the one wrapping transfer_workspace_content). So transfer_workspace_content never fires, the arrival stays undrainable, and the Workspace is stuck in inFlight until Rust's arrival watchdog hands it back. The workspace-move.test.ts mock having to grow _core = { mouseStateService: { activeEncoding: "DEFAULT" } } is the same edge surfacing already. An optional chain turns a stranded move back into a lost mouse mode — suggestion inline.
The pin-transfer machinery is now dead but still reads as live. With captureTransferContent returning pins: [] and planArrival no longer calling restoreTerminalPins, nothing in production reaches snapshotTerminalPins, restoreTerminalPins, transferredPinOf, or registerTerminalSourceAtLines — only their own tests do (describe('pins travelling with a Workspace') in notepad-store.test.ts). Their doc comments still describe the shipped path and cite docs/specs/transport.md → "Transferring a Workspace", which this PR rewrites to say the opposite; flushTerminal's comment still gives "a pin being re-registered over a rebuilt transcript" as the reason it exists, and routing.rs's Arrival::content doc still says the content carries "the notepad pins". A reader landing on any of those learns the old behavior.
The compatibility argument for keeping the field looks thin too. Arrival lives in Rust process memory (queued_at: Instant, drained by take_arrivals) and is never persisted — your own checklist has a mid-drag kill relaunching with fresh shells — so both ends of a transfer are always the same running build. If there is no reachable "old payload", then the pins field, the arrival's ignore path, and the new keeps notes without restoring legacy transferred pins test are all guarding a case that can't occur, and deleting the field plus the four functions behind it is the smaller end state. Happy to push that as a commit once this is out of draft.
Pinned by names files where the convention names tests. AGENTS.md: "Name the test that pins a rule; never reproduce its case inventory." The Source of truth: paragraph three lines below in the same section already does it the other way — Pinned by `a mark is ordered in the stream and a since-mark replay is exactly the remainder` in `standalone/sidecar/pty-core.test.js` . workspace-move.test.ts has forty-odd tests, so a bare filename doesn't locate the rule. The inline suggestion cites the two terminal-transfer.test.ts cases only, because transport.md is at 5,825 of its 5,850-word budget and naming the workspace-move.test.ts case too lands at 5,854; add it with a node scripts/spec-lint.mjs --ratchet docs/specs/transport.md in the same commit if you'd rather keep both.
dormouse-bot
left a comment
There was a problem hiding this comment.
A transfer in flight swallows Cmd+Q and the window close button, with nothing shown to the user. request_quit now returns after only append_log("[quit] transfer in progress; retry quit after it settles") whenever windows.arrivals is non-empty, and request_window_close does the same when the label is an arrival endpoint. Both are reached from on_window_event / RunEvent::ExitRequested after api.prevent_close() and api.prevent_exit() have fired, so the refusal is invisible: no dialog, no shake, no menu feedback, only a line in the app log.
Nothing re-drives the request either. The only request_quit call sites are the quit menu item, ExitRequested, and the last-window close path, and none of them is retriggered when the arrivals map drains — so "Retry a refused quit/close after the transfer settles" in docs/specs/standalone.md → Quit flow is something the user has to notice and do, not something the code does. That matters because an arrival only clears on adopt_done, adopt_failed, a target Destroyed, or the ARRIVAL_MAX watchdog: a target that accepts the arrival and then wedges holds it for 20 s, during which Cmd+Q is a silent no-op and the titlebar close button does nothing. transfer_workspace_content failing has the same effect — the arrival stays undrainable until the watchdog fires.
Either re-drive the pending quit/close once arrivals empties, or make the refusal visible. As written the app reads as hung on exactly the interaction users retry hardest.
Three smaller points are inline.
lib/src/components/WorkspaceStrip.tsx and WorkspaceStrip.test.tsx each grew a second (and in the test, third) import … from '…/workspace-ui-store' above the block that was already importing from it — in the test above the @vitest-environment docblock. Folding them into the existing blocks keeps one import per module.
Commenting rather than approving: the PR body says the replacement app-quit confirmation's "native manual retest is still pending, as is the last-workspace redraw retest", and this PR is what swaps both the quit and the window-close dialogs onto the typed-letter gate.
|
Addressed the remaining review in 96e39e2. Quit and native window-close requests made during a transfer are now remembered and automatically retried through the existing confirmation flow when the relevant transfer settles. Repeated requests coalesce; quit supersedes queued closes, and cancellation/destruction retires pending work. New transfers cannot starve a queued teardown. Pure state-machine and admission-wiring tests pass; the full Rust suite passes 105 tests. The three inline findings and duplicate imports are also fixed. The approved legacy transcript cleanup is included in 70f995c. The native manual coverage limitation remains stated accurately in the PR body. The user has explicitly instructed us to ignore Chromatic for this merge decision; fresh non-Chromatic CI is running. |
dormouse-bot
left a comment
There was a problem hiding this comment.
One leftover from removing windowName, outside this diff. In
apply_quit_actions, the QuitAction::RequestAll arm still emits
dormouse://quit-requested with
serde_json::json!({ "windows": app.webview_windows().len() }), under the
comment "The count is what tells each window whether to name itself in its
confirmation dialog." quit.ts's listener was the only reader of that field and
now takes no payload at all, so the count is dead and the comment describes
behavior this PR deleted — the Rust half of the describeWindow thread. Happy to
push the emit and its comment if you want it in this PR.
Two inline.
- Queue deferred quit/close requests inside the arrivals lock (`ArrivalQueue`), fold transfer admission into one predicate, and share `request_close_or_quit` between the close button and the settlement redrive. - Read the sessions directory and arrival journal once at boot (`saved_windows`); drop the unused window count from `quit-requested`. - Move the transfer guard into the window-session aggregator and give moves one entry (`startMove`); the quit dialog clears strip prompts with one reset. - Route the Workspace move prompt through `WorkspaceKillConfirm`, so a modifier or chord never answers it, and keep that component's listeners stable. - Remove dead teardown-modal branches, the redundant dialog keyboard owner, the unused `nameByPid` parser argument, and duplicated test setup. - Fix `dor workspace move` help, which still said pins travel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D2G3ZNiBMqAfGzXNNgfNdV
dormouse-bot
left a comment
There was a problem hiding this comment.
Two findings in the simplification pass.
The WorkspaceKillConfirm rule in docs/specs/layout.md no longer describes
the component. Routing the iframe move gate through it — titled Move and lose page state? via the new title prop — leaves the spec stating Must use
WorkspaceKillConfirm, titled "Confirm kill workspace", for Workspace close and
host termination confirmations, which omits the move gate and names a title one
of its callers now overrides. The component's own doc comment was updated
("Workspace close and move, window close, and app quit"); the rule that owns it
was not. AGENTS.md: "When code covered by a spec changes, change the spec." The
inline suggestion lands at 8,747 words against the 8,750 budget, so it needs no
--ratchet.
handOff's invoke-rejection path releases the pending guard
unconditionally, which is the case startMove's new doc comment says must
never happen — "a hand-back can settle while it still awaits the invoke's reply
or the marks, and a later release would drop the guard of the next move". The
line above already treats that race as reachable, deleting from inFlight only
when the entry is still this move's; the setWorkspaceTransferPending(…, false)
beside it is not guarded. If handleArrivalFailed settles first and a second
move of the same Workspace starts before the rejection lands, this clears the new
move's guard, and requestWorkspaceClose stops refusing a close mid-transfer.
The suggestion puts both under the one condition.
A hand-back can settle a move while its invoke still waits on Rust; the invoke's later refusal now releases the transfer guard only if its move is still the one in flight. Spec the move gate as a `WorkspaceKillConfirm` caller with its own title, and state the key rule once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D2G3ZNiBMqAfGzXNNgfNdV
Workspace transfers could lose mouse encoding and running-command state, and closing a transferred workspace could skip confirmation. Preserve terminal encoding, source grid, and semantic state across the transfer cut; rebuild and drain the terminal before fitting its destination. A pending workspace confirmation is dismissed on successful departure, and close cannot race an in-flight transfer. Transferred notes survive without source pins, as documented.
Window close and app quit now use the same Confirm kill workspace typed-letter interaction as workspace close. Host teardown names all affected workspaces, blocks the entire window through voting, keeps its letter stable, and retains the separate note-loss decision if archiving fails. Rust excludes transfers from quit/window-close admission so membership cannot change after approval.
This is the final PR in the Workspaces train (#614–#623). It also incorporates the outstanding review corrections from those PRs: timeout derivation checks, journal/cleanup guards, non-null stable refs, removal of duplicate replay handling and dead pin-transfer code, and corrected spec/test pointers. Current main is merged forward through every branch; land in order with merge commits.
Validation: full local
pnpm test, shared-library and standalone typechecks, and 105 Rust tests. Native user retests passed mouse input after tear-out/tear-in, transferred running-command confirmation, pending-confirmation tear-out, output continuity, and scrollback retention. The new unified app/window confirmation has automated coverage; its native manual retest is still pending, as is the last-workspace redraw retest.Fresh review fixes also cover rename/confirmation keyboard precedence, stale move-gate cleanup, harness restored-ID seeding, failed registry-report retries, shared ref grammar, retained journal ID/window-label reservation, and cleanup after missing transfer markers. The latest full local suite passed 3,162 shared-library tests and 220 standalone tests.
The approved legacy-root cleanup removes obsolete saved pane transcripts atomically and sweeps orphan temporary files, preserving layouts, geometry, archived notes, unrelated fields, and malformed snapshots. All 105 Rust tests and spec checks pass.
Final review follow-up: quit/close requested during transfer automatically resumes after settlement; failed arrival collection cleans browser-member alerts too. Removed unused window-name plumbing and corrected the exact test citation. User requested that Chromatic be ignored for the merge decision.