Conversation
|
Companion: #436, which keeps the output a capture could not contain. The two are independent and merge cleanly in either order. |
cb8d5de to
42efd78
Compare
|
Both jobs are green on this head now, so if the draft flag is just "not finished yet", say what is left and I will wait. If it is habit, please mark it ready for review. The practical reason I ask: the reviewer that goes over PRs here skips drafts, so both this and #436 are currently sitting outside the review queue rather than at the back of it. I am assembling 1.29.2 and these two are the kind of thing I would want in it, but I am not going to merge a pair of terminal capture changes unreviewed. Two things I did check while triaging, so you have them: Your companion note is correct. I merged each onto the current master in sequence, both orders, and there is no conflict. They are also clean alongside the other four open PRs, so ordering is not a constraint on any of this. (My first pass compared the two heads against each other directly and reported a conflict in And master has moved since you opened this: 1.29.1 went out an hour ago and carries #376's auto-naming, which touches On the content, without pre-empting the review: the thing I appreciate most here is the paragraph saying the retry does not repair the desktop-claim case and that you want that stated rather than implied. That is the right call and the right way to write it up. Who owns the pane size while a desktop claim is live really is a policy question, and I would rather decide it deliberately than have it fall out of a retry loop. Reporting the geometry so the client can at least see the mismatch is a genuine improvement on being blind to it, and it is the part that makes the policy question answerable later. Mark it ready when you are happy with it and it goes into the queue. |
|
Note on the "draft" status: I put both in draft while I reviewed them myself, only moving them out of draft when I think they are ready for your review. |
|
Thanks for this, and for the measurements. The geometry a capture was taken at genuinely was missing from the response, and a terminal shorter than the pane really does lose the overflow rows. One thing I want changed before this goes in. The retry is not gated on I ran your browser harness with the stub reporting What I would like: const framePositionsRowsAbsolutely = data.source === 'mux-visible';
const capturedTallerThanTerminal =
framePositionsRowsAbsolutely &&
Number.isFinite(data.captureRows) &&
data.captureRows > (this.terminal?.rows || 0);with the same gate applied to Two smaller ones in the same area, worth folding into the same pass:
One observation rather than an ask: there are two structural mismatches, not one. Besides the declined resize you documented, Everything else reads well. The full gate is green here at 7014 tests across 369 files, your new suite genuinely fails on master (2 of its 3 cases), the port is unique, the glob is registered in |
|
Thanks for this, and for turning the source gate around quickly. The response now says what pane geometry a frame was built for, and the client replays once when that frame does not fit, which closes the last of the ways a replayed frame and the terminal could disagree. All three asks landed: the comparison is gated on One thing I want to change before it ships, and three smaller ones I will fold in at merge time. The retry cannot converge for two of its three triggers, and pays a full extra replay each time ( The full-history re-arm is unreachable ( A Two notes with no action needed. Next step: I will apply the clamp guard, the comment and test wording, the changeset trim, and the CLAUDE.md lines for the two new response fields and the one-attempt rule, then merge. Thanks for the measurements and for stating plainly what the retry does not fix. That paragraph is what makes the policy question answerable later. |
|
All three landed, and the clamp guard comes with a test of its own, so you should not need to apply anything at merge beyond the CLAUDE.md lines. The clamp guard. The retry now skips when the reported geometry equals The unreachable re-arm. You are right, and the comment now says so rather than implying a page load retries. The line stays for the invariant. The bigger problem was downstream, as you spotted: the stub answered Geometry on a I also trimmed the changeset paragraph you flagged. You were right that it described the difference between my two commits rather than a change from 1.29.x; it now reads as release notes. The declined-resize half is yours to open as a follow-up, and I agree it belongs with the pane-ownership question rather than here. Same for Verified: full gate green at 369 files and 7015 tests, browser suite 6 of 6, and each of the three guards has a case that fails without it. Typecheck, lint, format and the frontend syntax check are clean. |
|
Perfect! Thanks a lot for your Contributions to the Project :-) |
|
Thanks for this, and for the third round. The response now says what pane geometry a frame was built for, the client replays once when that frame does not fit, and all three of the earlier asks landed cleanly: the comparison is gated on One thing I would like fixed before this goes in, and it is one nobody has raised yet. The retry throws away typed-but-unsent local echo text ( Local echo is on by default on touch devices ( This is pre-existing for the one gesture that already takes that path, tapping the tab you are already on, so it is not something you introduced. What is new is that nothing the user did triggers it, and on a phone next to an active desktop tab it now fires on every tab switch. Given how this repo treats losing input, I would rather close it here than leave it. What I would like: flush before nulling the id. Lifting the block at const echoText = this._localEchoOverlay?.pendingText || '';
if (echoText) {
this._sendInputAsync(sessionId, echoText);
const flushed = this._localEchoOverlay?.getFlushed() || { count: 0, text: '' };
this._flushedOffsets?.set(sessionId, flushed.count + echoText.length);
this._flushedTexts?.set(sessionId, (flushed.text || '') + echoText);
}A seventh case in Two small things I will fold in at merge, no action needed from you:
The declined-resize half stays mine to open as a follow-up, as agreed, and I will write the CLAUDE.md and architecture-invariants lines for the two new response fields and the one-attempt rule. Send the echo flush with its case and I will merge. |
|
The echo flush is in with its case, and you found a real one. I reproduced it before touching anything, because a bug about losing input deserves better than agreeing with the reasoning: with the overlay on, typing The fix. The flush moved into Your account of the mechanism was right in every step, including that it predates this branch. What I would add is why it is worth closing here rather than filing: before, it took a deliberate gesture on a tab you were already looking at, and now it fires on its own while someone is typing into a terminal that has not finished loading. Those are different bugs wearing the same stack trace. The seventh case. It forces the overlay on through the setting rather than the flag alone, so it survives the recompute every select runs, and it asserts the characters really are sitting unsent before triggering the replay. Headless chromium reports no touch support, so without that the case would pass on a build where typing goes straight to the PTY and there is nothing to lose. It records what crosses into The PR body. Fixed, and thank you for reading it. It still listed I left Verified: full gate green at 369 files and 7015 tests, browser suite 7 of 7, and each of the four guards on this branch has a case that fails without it. Typecheck, lint, format and the frontend syntax check are clean. |
|
Thanks for this one. The diagnosis is exact and the part most people would get wrong, fencing the comparison to a Two things before I merge. 1. The replay has no memory that it did not help (src/web/public/app.js:6556). The repo already has the pattern: 2. Please rebase (config/test-suites.ts:30). The branch is conflicting, which means GitHub ran no CI on it at all. The conflict is one line: you and #436 both appended a glob to Two smaller things I will fold in at merge unless you would rather do them:
One nit you can take or leave: the Everything I ran is green on your head: typecheck, lint, format:check, check:frontend-syntax, check:public-assets, the full Once the retry memo and the rebase are in, this goes in. |
A visible-frame capture repaints each row at an absolute position, counting up to the pane's height. A terminal shorter than that clamps every address past its own height onto its last line. The overflow rows then overwrite one another, and the rows underneath are lost. Replaying a real 50-row capture into a 30-row terminal rendered 28 lines of a 45-line command and drew the frame twice. Nothing in the response said what height the frame was built for, so the client could not detect this. A capture now reports the geometry it was really taken at through `capturedGeometry` on `PaneCaptureOptions`, and the terminal response carries it as `captureCols` and `captureRows`. When the captured pane is taller than the terminal, or the size that produced the capture did not survive the load, `selectSession` replays once at the size that stuck. `resizeRetry` caps that at one attempt, so two competing fits cannot trade replays forever. The retry re-arms the full-history flag only when the pass that ran had consumed it. A tab switch takes the bounded tail, so its retry takes the tail too: clearing the flag unconditionally would upgrade that switch into a fresh scrollback capture the user never asked for, which the route's own comments put at tens of megabytes. What this repairs is a capture that won a race against the resize meant to precede it. It does not repair a capture whose pane was too tall because `Session.resize` declined the resize outright, which it does for a small viewport while a desktop viewport's size claim is live. The retry re-sends the same declined resize and captures the same pane, and `resizeRetry` then stops it. Repairing that means changing who owns the pane size, which is a policy question this does not touch. The reported geometry still helps there, because the client can see the mismatch at all rather than being blind to it. Follows Ark0N#395, Ark0N#396 and Ark0N#397, which fixed the other ways the replayed frame and the terminal could disagree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only a visible-frame capture positions its rows absolutely, so only that frame can be damaged by a terminal of the wrong size. A `full=1` body is linear scrollback closed by a relative cursor move, which is relative precisely so the browser's row count need not match the pane's, and a `history` body is the byte stream, which carries no row alignment to protect. The geometry comparison ran on all three, so it fired most often on the one response it cannot help: `_fullHistoryLoaded` is empty on the first select of every non-shell session per page, and a session whose pane a desktop tab holds too tall to ever fit then paid a second whole-scrollback capture, reset and replay on every page load and every first tab switch. `framePositionsRowsAbsolutely` gates both the captured-geometry comparison and `sizeMovedUnderLoad`. A size that moved under a byte-stream or scrollback replay is healed by xterm's own reflow plus the SIGWINCH the trailing `sendResize` already sends. A pane WIDER than the terminal damages the same frame a second way, so `captureCols` is now compared rather than only logged. `formatPaneSnapshot` paints each row out to the pane's own width, so a narrower browser wraps every painted row, and the wrap on the last one scrolls the whole frame up by a row. The terminal response no longer falls back to `session.ptyCols`/`ptyRows` when the capture reported no geometry. The cursor query is what produces the absolute addressing in the first place, so a capture that lost it returned a raw frame that was never positioned, and a byte-history response was never positioned either. Naming the session's own PTY size there described a frame that does not exist and invited a repair for damage that is not present. `_ptyCols` is also written only by `resize()` while the PTY is spawned at the size queried from tmux, so it can be wrong on its own terms. Both fields are now absent instead, and the `Session` getters added for that fallback go with it. Two browser cases cover the new behaviour and each fails without its fix: a `mux-full-history` response with both dimensions mismatched asserts one fetch (two without the gate), and a `mux-visible` response wider than the terminal but short enough to fit asserts two (one without the width comparison). Corrects a claim in the comment above `capturedGeometry` in tmux-manager.ts. Both replay paths do not address rows absolutely; the full-history one ends in a relative move, which is the whole reason the gate is right. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three follow-ups to the source gate, each one measured rather than reasoned. A pane already drawing at the size the client just requested is left alone. The replay runs at `dimsAfterLoad`, so it can only change what is on screen if the pane was drawing at some other size; when the reported geometry already IS that size, the second pass captures the identical frame and pays a full reload to do it, including a visible re-flash, a dropped and reopened WebSocket and a deleted xterm snapshot. That equality is the signature of a clamp rather than a race: `getTerminalDimensions()` floors at 40x10 while `fitAddon.fit()` does not, so a terminal narrower than 40 columns or shorter than 10 rows reports a pane permanently bigger than itself and replayed on every tab switch without ever converging. A race never produces the equality, since its premise is that the pane was still at the size it was asked to leave. The declined-resize case does not produce it either, so that one still costs the single capped attempt and needs the pane-ownership question this does not touch. The full-history re-arm is unreachable and now says so. A pass that consumed the flag sent `full=1`, and the route answers `full=1` with `mux-full-history` or `history`, never `mux-visible`, so the source gate already rules out every such pass. The line stays for the invariant, but its comment no longer reads as if a page load retries, and the suite pins that it does not. The response no longer reports geometry for a body that carries no capture. The full-history path writes `capturedGeometry` from the cursor query and then returns '' for a pane holding nothing visible, which drops the source to `history` with the geometry already recorded: a `full=1` request whose capture reported 100x50 and returned nothing answered `source: "history"` with both fields set. Nothing acted on it, because the client ignores geometry on any other source, but the field said a frame had been drawn at a size when none had. The browser stub now derives `source` from the request the way the route does, rather than answering `full=1` with `mux-visible`, which the route cannot produce. Each case reaches a visible-frame response the way production does, by not being the first select of the page. Three cases pin the new behaviour and each fails without its guard: the clamp case sees two fetches instead of one, the scope case and the full-history case both see a replay the gate forbids, and the width case sees one fetch instead of two. The changeset now describes the change from 1.29.x rather than the difference between the two commits on this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
On a touch device the characters the user has typed live only in the local-echo overlay until Enter; they have never reached the PTY. The replay re-enters `selectSession` with `forceReload` on the session that is still active, and that branch nulled `activeSessionId` before `_cleanupPreviousSession` ran. The flush there is guarded on a session it can still see, so it was skipped, and the unconditional `_localEchoOverlay.clear()` that follows took the characters with it. Measured in chromium against the previous head: typing into the overlay and then making the call the replay makes left `pendingText` empty with nothing crossing into the delivery layer on either transport. The flush moves into `_flushLocalEchoTo(sessionId)`, called from both `_cleanupPreviousSession` and the `forceReload` branch before it nulls the id. The session is a parameter because the two callers mean different ones: cleanup flushes to the tab being left, the branch to the tab being reloaded. This was reachable before this branch, through the one gesture that already takes the `forceReload` path on an active session. What is new is that nothing the user does triggers it. The replay fires on its own the moment a tab switch finishes, which is exactly when someone typing into a still-loading terminal has text in the overlay, and on a phone beside an active desktop tab that is every tab switch. A seventh browser case pins it: it forces the overlay on, since headless chromium reports no touch support and the case would otherwise pass vacuously, asserts the typed characters really are sitting unsent, then triggers the replay and asserts they reached the session. Without the fix it fails with nothing delivered at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`resizeRetry` caps the recursion inside one select and says nothing about the next one, so a pane this browser cannot size reported the same mismatch on every select and bought the same failed repair each time: two fetches per tab switch for the life of the page, measured as a running count of 2, 4, 6 across three selects. That is the case this branch describes as happening every time rather than occasionally, a phone whose resize `Session.resize` declines while a desktop claim is live, and it is not the only one — any pane Codeman cannot size lands there, including one a second tmux client is also holding. Each wasted pass costs another `capture-pane`, which is `execSync` and blocks the server's event loop, plus a reset and chunked rewrite, a discarded snapshot and cache entry, and a dropped and reopened WebSocket. `_geometryRetryUseless` mirrors the existing `_fullHistoryRepullUseless`: a retry pass whose frame still does not fit adds the session, geometry that fits removes it, and the replay gate consults it. The proof has to come from a retry pass rather than a first one, because the retry ran at the size that stuck and the pane ignored it. Clearing on a fitting frame is what stops a pane that becomes sizeable again, once the desktop tab closes or its claim goes idle, from staying permanently unrepaired. The race case never reaches the latch, since it converges on its first attempt. The new browser case walks all of that: three selects reading 2, 3, 4 instead of 2, 4, 6, then a fitting frame, then a mismatch diagnosed afresh. Without the gate it fails on the second switch with `expected 4 to be 3`. Rebased onto master, which has moved to 1.30.0 and taken Ark0N#436. The one conflict was `config/test-suites.ts`, where both branches appended a glob to `BROWSER_TEST_GLOBS`; both are kept. Everything else merged clean, Ark0N#436's own changes to the same buffer-load path included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fb1f939 to
95dc6fe
Compare
|
Both are in, and CI has finally run: two jobs green on The retry memo. Two decisions inside that worth naming. The proof has to come from a retry pass rather than a first one, because the retry is the one that ran at the size that stuck and watched the pane ignore it; latching on a first-pass mismatch would swallow the race repair, which is the case that does converge. And the case does not stop at the latch: it then feeds a fitting frame and asserts the memo lifts, then a mismatch again and asserts it is diagnosed afresh. Without that half, closing the desktop tab that was holding the pane would leave the session permanently unrepaired, which is a worse bug than the one being fixed. I agree the resize endpoint answering with the size actually applied is the real fix, and that it does not belong here. It would also retire One thing I mirrored rather than improved: neither Set is cleared in The rebase. Done, and the conflict was the one line you called: both branches appended a glob to Your two smaller items are yours to fold in, as offered: the changeset trim and the Taking the nit: you are right that an invariant belongs in a static test over the source rather than a branch nothing can cover. I left the line as it is for this PR rather than change the shape of something you are about to merge, but if you want the static test instead of the unreachable guard, that is a small follow-up and I am happy to write it. Browser suite is 8 of 8, and each of the five guards on this branch has a case that fails without it. |
|
This is going in. Five rounds, and it earned every one of them. The thing that made this worth the depth: the diagnosis was exact and it was the part most people would have got wrong. A visible capture repaints each row at an absolute position, so a terminal smaller than the pane damages the frame two separate ways, and neither is visible as corruption you could reason backwards from. Too short clamps the overflow addresses onto the last line and silently eats the rows underneath. Too narrow wraps every painted row and the wrap on the last one scrolls the whole frame up by one. Your 50-row-pane-into-a-30-row-terminal measurement (28 of 45 lines, survivors drawn twice) is the kind of number that makes a fix reviewable instead of plausible. The new Playwright suite failing five of eight against the merge base is what I look for and rarely get. It is the difference between a test that describes the fix and a test that would have caught the bug. What I am folding in at merge, all small: The changeset. Rewritten to one user-facing paragraph, the same shape as #436. The version you had still described two guards and never mentioned
The invariants and CLAUDE.md lines, as promised. Three rules written down: a visible capture reports its geometry and the response omits it when nothing was positioned (so consumers test The follow-ups the thread already agreed on are still follow-ups: the declined-resize policy question, a resize endpoint that answers with the size it actually applied, the static test replacing the unreachable On #451, your other open PR: it is not stalled. The trailing trim is obviously right, but the shared leading-indent strip is a default this project would be choosing on its own with no native-terminal precedent, and it rewrites content in cases that are not TUI margins (git log bodies, indented Python out of |
…d capture geometry #454: the behaviour the PR adds had no test, so a regression test drives runGrok() at tabCount 3 and asserts three quick-start POSTs with sequential w<n>-<case> names (verified to fail against master's session-ui.js). Each caller now reads the count BEFORE its opening banner and announces it there, the way runClaude() already did, so a launch no longer prints two headers and a launch with another session already active still says how many are starting. runClaude() calls the shared _readTabCount() instead of its own copy of the 1..20 clamp, and that helper optional-chains the element read, since hoisting it above each caller's try block would otherwise let a missing #tabCount throw where the launch-error path cannot report it. #435: sizeMovedUnderLoad derived from data.source alone. `mux-visible` is not sufficient: a failed display-message cursor query makes capturePaneBuffer skip the snapshot repaint and return the raw capture, which the route still labels mux-visible, so a size that moved during such a load bought a full forced reload to repair a frame that was never positioned. It now tests Number.isFinite(data.captureRows) like its two siblings. Plus the invariants and CLAUDE.md lines promised on #435: a visible capture reports its geometry and omits it when nothing was positioned, the comparison runs on mux-visible only, and the replay is capped at one attempt and latches per session when it cannot converge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
A visible-frame capture repaints each row at an absolute position, counting up to the pane's height and out to its width. The terminal response said nothing about either, so a terminal smaller than the pane silently lost rows and neither side could tell. The capture now reports the geometry it was taken at, and the client replays once when the frame does not fit.
The problem
formatPaneSnapshotpaints each row withESC[<row>;1H, truncated to the pane's width. A terminal shorter than the pane clamps every address past its own height onto its last line, so the overflow rows overwrite one another and the rows they land on are gone. A terminal narrower than the pane wraps every painted row, and the wrap on the last one scrolls the whole frame up by a row.I measured the height case against a real session on 1.28.2. A shell pane sized 100x50 ran
for i in $(seq 1 45); do echo probe-line-$i; done. The visible-frame capture came back addressing rows 1 through 50. Replaying that same capture into@xterm/headlessat two heights gives:probe-line-45probe-line-28Seventeen lines of output are unreachable, and the surviving frame is drawn twice.
The mismatch is not only a race between a resize and a capture.
Session.resizedeclines a small viewport's request outright while a desktop viewport's size claim is live. It returns before touching the PTY whenever the desktop has been active within the 90 secondDESKTOP_CLAIM_IDLE_MS:A phone opening a session that a desktop tab is holding therefore gets a frame built for the desktop's rows, every time rather than occasionally.
POST /api/sessions/:id/resizereturns an empty body, so the phone believes its resize took.The replay does not repair that second case, and I want that stated rather than implied. It re-sends the same resize, the server declines it the same way, and the capture comes back at the same height;
resizeRetrythen stops it after the one extra attempt and the frame is shown as it is. Repairing it means changing who owns the pane size while a desktop claim is live, which is a policy question this change does not touch. What the reported geometry buys there is that the client can see the mismatch at all rather than being blind to it. The replay does repair the race case, where the resize has landed by the second attempt.Changes
src/mux-interface.ts—PaneCaptureOptions.capturedGeometry, an out-parameter the implementation fills with the size the capture was really taken at.src/tmux-manager.ts—capturePaneBufferwrites it once, before either replay path returns, and only whenqueryPaneCursoractually produced geometry.src/web/routes/session-routes.ts— the visible path now passes an options object where it passedundefined, purely so the geometry can come back on it. The response carries it ascaptureColsandcaptureRows, and both are absent unless the response really carries a capture. A body that was never positioned has no geometry to describe, and naming one would invite the client to repair damage that is not there. That covers two separate cases: a failed cursor query, which is what produces the absolute addressing in the first place, and a capture that reports geometry and still returns nothing, which is what the full-history path does for a pane holding nothing visible.src/web/public/app.js—selectSessioncompares the reported geometry against its own size and replays once when the frame cannot fit. The comparison runs on amux-visibleresponse only: a full-history body is linear scrollback closed by a relative cursor move, and a byte-history body carries no row alignment at all, so a mismatch damages neither and a replay repairs neither. Two guards keep the replay to the pass that can converge.resizeRetrycaps it at one attempt, and a pane already drawing at the size the client just requested is left alone, which is the signature of a clamp rather than a race. Separately, the unsent local-echo text is flushed to the session before the reload nulls the active id, so a replay firing while the user types cannot clear characters that never reached the PTY.test/routes/session-routes.test.ts, the write ordering plus a rendering test for why the height is needed at all intest/tmux-capture-full-history.test.ts, and a new browser suite.A note on six assertions I changed
test/routes/session-routes.test.tshad six assertions of the formtoHaveBeenCalledWith(muxName, undefined). Theundefinedwas only ever a proxy for "this is not a full-history request", and the visible path now passes an object. They assertexpect.not.objectContaining({ fullHistory: true })instead, which is what they were checking for. No coverage is lost.Verification
Every CI gate passes locally:
npm run typecheck,npm run lint,npm run format:check,npm run check:frontend-syntaxnpm run check:lockfileandnpm run generate:cli-catalog -- --check/api/statusnpm test, green at 7015 tests across 369 filestest/capture-geometry-retry.browser.test.tsdrives the real client in chromium and stubs only the terminal endpoint, because staging the mismatch against live tmux needs two viewports. Its stub derivessourcefrom the request the way the route does, so no case rests on a response shape production cannot produce. Seven cases, and I removed each guard in turn to confirm none of them passes vacuously:I also checked the geometry reporting against a running server, not only in tests. A 100x50 pane returns
captureCols: 100, captureRows: 50on the visible path. After a resize to 24 rows it returns 24, and the capture portion of the payload addresses exactly 24 rows.Running the browser suite in a tree that borrows another checkout's
node_modulesneedsnode scripts/prepare-test-vendor.mjsfirst. Without the vendored xterm bundles the page never definesTerminal, and the test fails on its wait forapp.terminalrather than passing vacuously against a zero-row terminal.Related
Follows #395, #396 and #397, which fixed the other ways a replayed frame and the terminal could disagree — row alignment in the full-history replay, fitting only once the font is measurable, and a detached window owning its own pane size. This is the remaining case: the frame is built correctly, for a pane the terminal is not.