P0-B: relocate a live terminal surface between hosts without killing the PTY - #33
Open
rockyway wants to merge 19 commits into
Open
P0-B: relocate a live terminal surface between hosts without killing the PTY#33rockyway wants to merge 19 commits into
rockyway wants to merge 19 commits into
Conversation
…osables and share their wiring
…on window (design 012 §6.2)
…mode (design 012 §8.1)
…al (design 012 §5.9)
012 §13 T11 says explicitly: count gate READS, or drop the count. The plan implemented a raw substring count over the whole file, which also counts comments — and at 8 <= 8 it had zero headroom, so the first thing it did was make someone reword a doc comment to keep the suite green. A test that constrains prose is the tail wagging the dog. Strips comments first and counts `this.paneActive` in code: 3 reads today (constructor seed, setActive's write, geometryEligible's own), asserted <= 4. Red-checked by reintroducing a bare gate — it fails, as a tripwire must.
…containers (design 012 §5)
…ggest gate (design 012 §5.9-§5.12)
…orts cannot strand it (design 012 §5.4)
…oss relocation (design 012 §6)
…p and an identity-guarded cleanup (design 012 §4.2)
…canvas host (design 012 §4.2)
…und var (design 012 §4.4)
…ew 102) Both said "captured at :179", a design-spec line number that reads as a source line here and points nowhere useful. Says what it means instead, and records WHY the cleanup uses the capture rather than re-reading the ref: React nulls terminalRef.current during the deletion traversal, before passive cleanup runs, so a re-read would be null exactly when it matters.
… fit-only one (review 103 F3)
armActivationFit and setFontSize both write the single fitTimer slot, so
whichever arms second replaces the first. setFontSize installed a fit-ONLY
callback, dropping the flushDeferredResizeOnActivation() the activation
callback carries:
hidden pane parks a resize (pendingResize set, resizeTimer null)
-> relocateTo(canvas) raises eligibility, arming the ONLY callback that
will ever flush that parked value
-> a font change inside those 50ms replaces it with the fit-only timer
-> if the new fit proposes the same grid, xterm emits no onResize so
nothing reschedules, and healOnce refuses to run while pendingResize
is set — the PTY stays stale indefinitely.
This is a REGRESSION THIS BRANCH INTRODUCED, not a pre-existing bug: on
develop setFontSize early-returns on !paneActive, so a hidden pane could
never reach the timer. §7.1 row 3 widened that gate to geometryEligible(),
which is what opens the path.
Arming the richer callback costs nothing — the flush early-returns when
nothing is parked. Red-checked: restoring the fit-only timer fails the
new test.
Contributor
Author
External review round: 1 approve, 1 block — not mergingCI is green on all five checks. agy approved with zero findings (review/102). codex found three HIGH defects and blocked (review/103). I verified all three against the code; all three are real.
Neither open finding can fire today — nothing registers a canvas surface host yet, so every F3 was a regression this branch introduced, not a pre-existing bug: on F1 is a hole in the spec's own reasoning. §7.3 says the return trip's gap is filled by "the surviving F2 compounds a pre-existing defect: Full analysis, options and a recommendation for each: |
…e outbound one External review 103, finding 1. A background pane coming home from a canvas node stayed at the CANVAS node's grid until the tab was next activated. design 012 rev 6 §7.2 row 4 had setSurfaceDisplayed(false) record and return, justified by §7.3's claim that "the surviving fitTimer" fills the gap R3 opens when it lowers eligibility before R7 arms the pane's observer. That timer is the one R3 armed on the way OUT, and armActivationFit's callback nulls fitTimer as its first statement -- so it only survives a canvas visit shorter than 50ms. Every real visit is longer. Nothing survived, row 4 armed nothing, and the observer callback was gated off. Row 4a: the return leg onto a background pane arms the settle fit. - It fires 50ms after R3, therefore after R6's synchronous move, so it measures the pane. A timer armed before the outbound leg only happened to. - It does not breach the §6.2 hidden-pane SIGWINCH park: the fit resizes xterm only; the backend resize it provokes fails geometryEligible() and parks, and the PTY learns the size on the next activation via the same callback's flush. - Narrow to !paneActive: with the pane visible, R7's observer already fits. The FT rule is restated as a property of the END STATE -- no transition may leave the engine with no pending fit while its surface has moved to a container it has not measured -- rather than as a prohibition on clearTimeout. That phrasing is what made a bare record-and-return look compliant when it is the exact failure the rule exists to prevent. T10e pins the long visit and asserts the park still holds. T10d keeps the short one; its note claiming the drained-timer variant was "unreachable by design rather than by a bug" was exactly wrong and is corrected.
… its own
External review 103, finding 2. Reusing a pane node for a different terminal id
left BOTH engines' xterm elements in it, permanently.
mount() was append-only on both paths and unmount() deliberately leaves
term.element in the DOM, because the cache still owns the live Terminal and a
later mount reattaches it. Each is correct alone. Together they leak whenever
TerminalPane's unkeyed TerminalDisplay changes terminalId in place: engine A's
surface is still in the pane when engine B appends its own, so the pane hosts
two full-height surfaces with A still painting through its cache-lifetime
bridge subscription while its input wiring is gone.
It also pinned A's cache entry against eviction -- enforceCacheCap skips any
entry whose element is still isConnected (cache.ts:142) -- so the orphan held a
Terminal, its scrollback and two bridge subscriptions for the life of the app.
detachForeignSurfaces() keys on ELEMENT IDENTITY through the cache, not on a
.xterm class sweep: the only nodes it may remove are ones it can positively
identify as another engine's surface. Overlays, the WebGL scratch canvas and any
future chrome are none of its business. Removal is non-destructive -- the
element stays owned by its cache entry, and appendChild reattaches it just as
well from a detached node.
This is NOT a P0-B defect. The bare mount/unmount/mount sequence in the new test
reproduces it with no canvas involved; P0-B only made it reachable on a
supported path, by returning A to the captured pane before B is installed. Hence
the repair sits in mount() rather than in the relocation cleanup.
- the xterm mock now classes its element `terminal xterm` as the real one does,
so a test can tell a terminal surface from the pane's other children
- T23's comment recorded this defect as a fixed property of the world ("neither
is reachable, in this harness or in the real component"). It was an accurate
reading of the code and the wrong conclusion; the plan's original assertion is
restored, and the harness's mount stand-in now mirrors the real one
- the FT-rule test is renamed to what it actually asserts: setSurfaceDisplayed
always leaves a fit PENDING, which is the rev 7 phrasing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements P0-B — terminal surface relocation (
termflow-fabric/docs/design/012rev 6, viadocs/plan/015). This is the second of three Canvas Mode prerequisites; P0-A merged as #32.What this adds
TerminalEngine.relocateTo(container, opts?)— move a live terminal's rendered surface between its tab pane and another host without killing the PTY, losing scrollback, or resizing the PTY into a scrollback wipe.The mechanism is deliberately minimal: a single synchronous
container.appendChild(term.element)inside one engine operation. No portal, no broker, no cache pin, no open/closing/closed protocol — all four were designed, reviewed and rejected across five spec revisions, and012§12 records why.Why it is shaped this way
Two things drove the design, and both were measured rather than argued:
appendChildof an already-parented node is remove-then-insert inside one synchronous algorithm. Spikereference/004Q2 confirmed no observer of any kind —MutationObserver,ResizeObserver, a microtask queued immediately before, a synchronous read — ever seesisConnected === false.[host]alone is unreachable on the commit that creates the engine. Spike004Q1 measured all layout effects firing before any passive effect, so the layout effect reads a nullengineRef. Hence theengineGenerationtoken.Notable invariants
relocateTonever callsmount().mount()does eight things relocation must not (012§5.0); its reattach catch-all deletes the cache entry on any thrown error and falls through to creating a blank terminal — silent total scrollback loss (H1).fitTimer— not R2, not R3 in either direction, not the abort path. Exactly one transition in the engine cancels it. Verified structurally:relocateTo's body contains noclearTimeout.convergenceResizeAt, so a ratatui/codexESC[2J ESC[3Janswer to the SIGWINCH is repaired rather than lost.Scope
packages/terminal-core/andsrc/renderer/. Nothing undersrc-tauri/(012success criterion 12) — verified:git diff --name-only develop..HEAD -- src-tauriis empty.Nothing calls
relocateToin production yet; Canvas Mode (plan/013Task 9) is its first consumer.Tests
terminal-coretsc --noEmitclean in both roots. Test identifiers carry012§13's T-numbers so spec and implementation can be cross-checked.Manual verification in a develop build (
012§13, plan Task 17) is still outstanding and is a human gate — no spike ran inside WebView2, and no unit test can prove a real ratatui app survives the SIGWINCH.