Preserve terminal mouse encoding and grid across workspace transfers - #630
Preserve terminal mouse encoding and grid across workspace transfers#630nedtwigg wants to merge 1 commit into
Conversation
Deploying mouseterm with
|
| Latest commit: |
f169d75
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://29868aef.mouseterm.pages.dev |
| Branch Preview URL: | https://workspaces-transfer-fixes.mouseterm.pages.dev |
dormouse-bot
left a comment
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.
| const encoding = (terminal as unknown as { | ||
| _core: { mouseStateService: { activeEncoding: string } }; | ||
| })._core.mouseStateService.activeEncoding; |
There was a problem hiding this comment.
A missing _core.mouseStateService here throws out through captureTransferContent and past handOff's only try, so transfer_workspace_content never fires and the move hangs until the arrival watchdog hands it back. Optional-chaining degrades it to what it should be: a lost mouse mode.
| const encoding = (terminal as unknown as { | |
| _core: { mouseStateService: { activeEncoding: string } }; | |
| })._core.mouseStateService.activeEncoding; | |
| const encoding = (terminal as unknown as { | |
| _core?: { mouseStateService?: { activeEncoding?: string } }; | |
| })._core?.mouseStateService?.activeEncoding; |
| serializer. Pinned by `lib/src/lib/terminal-transfer.test.ts` and | ||
| `standalone/src/workspace-move.test.ts`. |
There was a problem hiding this comment.
AGENTS.md: "Name the test that pins a rule" — and the Source of truth: paragraph just below already cites by test title. This fits the 5,850-word budget (5,840); adding the workspace-move.test.ts case needs a --ratchet in the same commit.
| serializer. Pinned by `lib/src/lib/terminal-transfer.test.ts` and | |
| `standalone/src/workspace-move.test.ts`. | |
| serializer. Pinned by `preserves mouse tracking and encoding` and `rebuilds a | |
| full-screen grid larger than xterm defaults without clipping` in | |
| `lib/src/lib/terminal-transfer.test.ts`. |
| /** The buffer as the escape stream that rebuilds it; `''` for a Session this | ||
| * Window no longer held. */ | ||
| serialized: string; | ||
| grid?: TerminalGrid; |
There was a problem hiding this comment.
The only undocumented field in this interface, and the one whose contract isn't guessable from the name — that the target constructs xterm at it and only then fits.
| grid?: TerminalGrid; | |
| /** The source's grid, which the target rebuilds the buffer at before it fits | |
| * the pane; absent for a Session this Window no longer held. */ | |
| grid?: TerminalGrid; |
2 participants
Workspace transfers left ascii-splash unable to read mouse input until it restarted, and could replay full-screen content at the wrong grid until a resize. Preserve SGR/SGR-pixel mouse encoding alongside xterm serialization, rebuild at the source grid, and drain parsing before the target Wall mounts and fits its pane.
Transferred notes now arrive without source pins, and the spec documents that limitation. Older payloads containing pins are ignored.
Validation: 3,030 shared-library tests passed; standalone suite passed (200 tests), with the final expanded transfer suite passing all 36 tests; shared-library and standalone typechecks passed; spec lint and self-tests passed. Added real-xterm encoding/reset/grid regressions and arrival parsing/pin-drop checks.
Native ascii-splash retest remains pending for both tear-out and transfer into an existing window, especially moving a window's last Workspace. The encoding accessor uses pinned xterm internals and is covered by real-xterm tests.
Stacked on workspaces-harness (#623).