mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-09 00:49:41 +02:00
fix(terminal): Ark0N's read of the #464 geometry work
Five items, two of which he could only see by running it, plus six smaller ones. Taking the two blockers first, because both were wrong in ways the existing tests could not catch. **Adopting the PTY's rows put the CLI's input line off-screen.** A phone that took a desktop's 43 rows into a viewport with room for 18 painted an `.xterm-screen` far taller than its container; xterm's own viewport then had nothing to scroll, so the bottom of the frame sat below the container with no gesture able to reach it. Output visible, typing invisible, for as long as the desktop kept the claim hot. `reconcilePtyGeometry` adopts COLUMNS ONLY now: width is the axis Ink's wrap and `eraseLines` arithmetic depend on, and keeping the local row count keeps the composer at the bottom of a viewport that scrolls. Measured at his geometry — a 360x300 container against a 198x43 pane now keeps 13 rows, takes 198 columns, paints 202px into a 210px container, and the input line is inside the box. **`capture-geometry-retry.browser.test.ts` failed, and CI could not see it** because the file is in `BROWSER_TEST_GLOBS`. Its premise WAS the clamp — `getTerminalDimensions()` floored while `fitAddon.fit()` did not — which this work removes at the source, so it can never hold again at any viewport. The case survives on its own terms: a pane already drawing at the requested size must not be replayed. Its premise is now the #464 invariant itself, that the floored report and the terminal agree, which is a stronger guard because the clamp coming back fails it here rather than silently restoring the replay loop. The helper docblock that repeated the old premise is corrected too. **A session with no pane reported 120x40 and the client adopted it.** `resize()` writes `_ptyCols`/`_ptyRows` only when `ptyProcess` is set and nothing seeds them from the spawn geometry, so a dead-pane session still held the constructor defaults — clicking that tab resized the browser terminal to 120x40 and, on anything narrower, claimed another device owned the pane when none existed. `Session.ptyGeometry` returns null without a pane, the HTTP route answers `{}` and the socket sends no frame at all. The raw `ptyCols`/`ptyRows` getters are deleted rather than left available to be misused again. **The 40-column floor clipped the pane with nothing able to reach it.** The affordance keyed on a PTY mismatch, and the floor produces no mismatch — xterm and the PTY agree throughout, the terminal is simply wider than the box. It keys on what does not FIT now, MEASURED (`.xterm-screen` against the container, on the next frame, because the screen takes its width with the render) rather than derived from cell arithmetic. Measured at 360px: font 24 applies 40 columns and paints 560px, and all 200px of the overhang is reachable. `.pty-oversized` is renamed `.term-overflows-x`, because after this the old name describes only one of the two causes. **"Scroll sideways" did not work on touch for the sessions it targets.** `touch-action: pan-x` is cancelled before it starts by the `preventDefault()` `touchstart` calls on every 'content' tap. The terminal's own touchmove handler pans the container now, with the axis locked once per gesture so a diagonal cannot pan and scroll at once, and the CSS grants no `touch-action` at all — handing the browser a pan AS WELL would move the pane twice for one finger on the taps where that preventDefault does not run. Measured under real touch dispatch: a 140px swipe reaches `scrollLeft` 140 where it reached 0 before, the buffer does not move with it, and a vertical swipe still scrolls the scrollback. Three defects in the above, found while checking it rather than by being told: - `canPanHorizontally` first tested `scrollWidth > clientWidth` alone, which is true of a container that is not a scroller — a sideways swipe would have locked the axis, done nothing, AND suppressed the vertical scroll it should have been. Gated on the class as well. - The notice advised scrolling sideways whenever the PTY was wider, including when it still fitted and nothing scrolled. It is gated on measured overflow, and on a comparison against the width this container WOULD request rather than the one it currently holds — once adopted those are equal, so the second question answers itself false while the condition is still true. - `_syncTerminalOverflowAffordance` could throw out of `document.getElementById` before reaching its try block. It runs off every geometry change, so a cosmetic affordance could have taken the resize down with it. The smaller items: - `docs/architecture-invariants.md` no longer explains the equality guard as a clamp signature; it records what the clamp used to do and why it cannot any more. Edited by hand — that file is outside the Prettier glob, and letting Prettier near it rewrote eleven unrelated emphasis markers. - `throttledResize`'s HTTP fallback reads the reply. It is the path where a declined resize is least likely to be noticed, because no socket means no `{"t":"zc"}` frame either. - The changeset covers the whole release: the geometry work, the queued replay clear, the renderer watchdog, the body-covering fetch deadline, the WebSocket output-gap reconcile, the build-generated service-worker precache and per-build cache key, and the crash-trail hygiene. - `@xterm/headless` is declared in the root devDependencies instead of being reached through workspace hoisting. - The output-gap marker is cleared after any response arrives, not only when the capture was non-empty: a server that answers with an empty capture HAS reconciled us, and leaving the marker set refetched on every reconnect. - `e587d845`'s message claimed a test asserted the failed-load copy against the built asset. It did not — that assertion lived in a probe deleted with the other scratch scripts, so the claim was false when it was written. There is a real test now, and it reads the source rather than `dist/`, because `dist/` is not committed and a test that skips when it is absent would pass for the wrong reason in CI. `Session.ptyGeometry` gets behavioural coverage against the real class in `session-resize-arbitration.test.ts` rather than a source guard, including the contrast — a pane that does exist still reports, and still follows a resize — so "always null" would fail it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Rounak Datta
co-authored by
Claude Opus 5
Claude Opus 5
parent
e587d84590
commit
e1e7dc5bd8
+160
-30
@@ -764,6 +764,32 @@ Object.assign(CodemanApp.prototype, {
|
||||
let longPressStartX = 0;
|
||||
let longPressStartY = 0;
|
||||
let touchStartY = 0;
|
||||
let touchStartX = 0;
|
||||
// 'x' | 'y' | null — locked on the first travel past the tap slop, so a
|
||||
// diagonal drag cannot pan and scroll at the same time.
|
||||
let panAxis = null;
|
||||
/**
|
||||
* Can this gesture pan sideways? Only while the terminal is wider than
|
||||
* the box showing it (`.term-overflows-x`, set by
|
||||
* `_syncTerminalOverflowAffordance`).
|
||||
*
|
||||
* ⚠️ This has to be done in JS. `touch-action: pan-x` alone does nothing
|
||||
* for the sessions the affordance targets: `touchstart` calls
|
||||
* preventDefault() for every 'content' tap — the normal case for a
|
||||
* mouse-tracking TUI sitting at the bottom of its buffer — which cancels
|
||||
* the browser's pan before it starts. Measured under touch emulation, a
|
||||
* 140px horizontal swipe reached scrollLeft 141 without that
|
||||
* preventDefault and 0 with it. It only ever worked for shell sessions,
|
||||
* while scrolled up, or with a mouse.
|
||||
*/
|
||||
const canPanHorizontally = () =>
|
||||
// Both halves. The class is what makes the container a scroller at all
|
||||
// (`overflow-x: auto`); without it `scrollLeft` silently stays 0, and a
|
||||
// gesture locked to 'x' on that basis would do nothing AND suppress the
|
||||
// vertical scroll it should have been. The measurement is the second
|
||||
// half because sub-pixel cell widths can leave a stray pixel of
|
||||
// scrollWidth on a terminal that fits perfectly well.
|
||||
container.classList.contains('term-overflows-x') && container.scrollWidth - container.clientWidth > 1;
|
||||
let tapStartedWithTerminalFocus = false;
|
||||
let tapStartIntentCache = null;
|
||||
// px — ignore micro-drift to distinguish tap from scroll. Shared with the
|
||||
@@ -783,6 +809,8 @@ Object.assign(CodemanApp.prototype, {
|
||||
touchLastX = ev.touches[0].clientX;
|
||||
touchLastY = ev.touches[0].clientY;
|
||||
touchStartY = touchLastY;
|
||||
touchStartX = touchLastX;
|
||||
panAxis = null;
|
||||
velocity = 0;
|
||||
pixelAccum = 0;
|
||||
isTouching = true;
|
||||
@@ -851,8 +879,15 @@ Object.assign(CodemanApp.prototype, {
|
||||
}
|
||||
if (ev.touches.length === 1 && isTouching) {
|
||||
const touchY = ev.touches[0].clientY;
|
||||
if (!didScroll && Math.abs(touchY - touchStartY) >= TAP_THRESHOLD) {
|
||||
didScroll = true;
|
||||
const touchX = ev.touches[0].clientX;
|
||||
if (!didScroll) {
|
||||
const travelY = Math.abs(touchY - touchStartY);
|
||||
const travelX = Math.abs(touchX - touchStartX);
|
||||
const sideways = canPanHorizontally() && travelX >= TAP_THRESHOLD;
|
||||
if (travelY >= TAP_THRESHOLD || sideways) {
|
||||
didScroll = true;
|
||||
panAxis = sideways && travelX > travelY ? 'x' : 'y';
|
||||
}
|
||||
}
|
||||
// Below the tap threshold, treat the gesture as a potential tap:
|
||||
// don't preventDefault (iOS needs click synthesis to show the
|
||||
@@ -862,6 +897,15 @@ Object.assign(CodemanApp.prototype, {
|
||||
// fling, so a jittery tap would both position the cursor AND scroll.
|
||||
if (!didScroll) return;
|
||||
ev.preventDefault();
|
||||
if (panAxis === 'x') {
|
||||
// Pan the container, and touch nothing the vertical path owns —
|
||||
// no pixelAccum, no velocity, so touchend cannot turn a sideways
|
||||
// swipe into a momentum fling down the scrollback.
|
||||
container.scrollLeft -= touchX - touchLastX;
|
||||
touchLastX = touchX;
|
||||
touchLastY = touchY;
|
||||
return;
|
||||
}
|
||||
const delta = touchLastY - touchY; // positive = scroll down
|
||||
pixelAccum += delta;
|
||||
velocity = delta * 1.2;
|
||||
@@ -1087,11 +1131,24 @@ Object.assign(CodemanApp.prototype, {
|
||||
}
|
||||
}
|
||||
if (!sentViaWs) {
|
||||
// ⚠️ The reply carries the geometry that actually took, and this
|
||||
// is the path where a declined resize is LEAST likely to be
|
||||
// noticed: no socket means no `{"t":"zc"}` frame either, so
|
||||
// discarding it here left the one transport that cannot hear the
|
||||
// answer also not asking for it.
|
||||
const resizedSessionId = this.activeSessionId;
|
||||
fetch(`/api/sessions/${this.activeSessionId}/resize`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({ cols, rows, viewportType }),
|
||||
}).catch(() => {});
|
||||
})
|
||||
.then(async (res) => {
|
||||
const applied = (await res.json())?.data ?? {};
|
||||
this._onPtyGeometryReport(resizedSessionId, applied.cols, applied.rows);
|
||||
})
|
||||
.catch(() => {
|
||||
/* a resize that never landed tells us nothing about the PTY */
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -5454,7 +5511,11 @@ Object.assign(CodemanApp.prototype, {
|
||||
}
|
||||
const dims = this.getTerminalDimensions();
|
||||
if (!dims) return null;
|
||||
return this._resizeTerminalTo(dims) ? dims : null;
|
||||
if (!this._resizeTerminalTo(dims)) return null;
|
||||
// The floor can leave this terminal wider than the box that shows it, and
|
||||
// that clips columns with no gesture to reach them (issue #464, item 4).
|
||||
this._scheduleOverflowAffordanceSync();
|
||||
return dims;
|
||||
},
|
||||
|
||||
/**
|
||||
@@ -5583,7 +5644,7 @@ Object.assign(CodemanApp.prototype, {
|
||||
* 120 columns against a 62-column terminal draws every wrapped line twice.
|
||||
*
|
||||
* Adopting can leave the pane wider than the viewport, and the container is
|
||||
* `overflow: hidden`, so `.pty-oversized` grants horizontal reach for exactly
|
||||
* `overflow: hidden`, so `.term-overflows-x` grants horizontal reach for exactly
|
||||
* as long as the mismatch lasts. Correct-and-reachable beats correct-and-
|
||||
* clipped beats garbled; nothing here is worth trapping content behind.
|
||||
*
|
||||
@@ -5594,38 +5655,107 @@ Object.assign(CodemanApp.prototype, {
|
||||
_onPtyGeometryReport(sessionId, cols, rows) {
|
||||
if (!this.terminal || sessionId !== this.activeSessionId) return;
|
||||
const local = { cols: this.terminal.cols, rows: this.terminal.rows };
|
||||
const { adopt, oversized } = window.CodemanTerminalGeometry.reconcilePtyGeometry(local, { cols, rows });
|
||||
if (!adopt) {
|
||||
this._setPtyOversized(false);
|
||||
return;
|
||||
const { adopt } = window.CodemanTerminalGeometry.reconcilePtyGeometry(local, { cols, rows });
|
||||
// Columns only, and the local row count is kept — see reconcilePtyGeometry
|
||||
// for why adopting rows put the CLI's input line below the container with
|
||||
// nothing able to scroll to it.
|
||||
if (adopt && this._resizeTerminalTo({ cols, rows: local.rows })) {
|
||||
// The numbers we would report next are now the PTY's, not the container's:
|
||||
// without this the dedupe in throttledResize/sendResize compares against a
|
||||
// request that was refused and suppresses the retry that recovers the pane.
|
||||
this._lastResizeDims = { cols, rows: local.rows };
|
||||
}
|
||||
if (!this._resizeTerminalTo({ cols, rows })) return;
|
||||
// The numbers we would report next are now the PTY's, not the container's:
|
||||
// without this the dedupe in throttledResize/sendResize compares against a
|
||||
// request that was refused and suppresses the retry that recovers the pane.
|
||||
this._lastResizeDims = { cols, rows };
|
||||
this._setPtyOversized(oversized);
|
||||
// Is the PTY at a width this container did not ask for? Compared against
|
||||
// what we WOULD request, not against what the terminal currently holds:
|
||||
// once adopted those two are equal, so the second question answers itself
|
||||
// false and the condition would look resolved while it is still true.
|
||||
// The floor widens this terminal too, and that is the reader's own font
|
||||
// setting rather than another device — hence the comparison, not `>`.
|
||||
const wanted = this.getTerminalDimensions();
|
||||
this._paneWidthRefused = !!wanted && Number.isFinite(cols) && cols !== wanted.cols;
|
||||
this._scheduleOverflowAffordanceSync();
|
||||
},
|
||||
|
||||
/**
|
||||
* Let the reader reach a pane wider than their screen, and say why once.
|
||||
* Measure on the NEXT frame, coalesced.
|
||||
*
|
||||
* Chrome for a condition that is not happening is clutter, so both the scroll
|
||||
* affordance and the notice exist only while the mismatch does. The notice is
|
||||
* once per transition, not per report: reports arrive on every resize, and a
|
||||
* toast that repeats is noise about a situation the reader can already see.
|
||||
* `terminal.resize()` updates the buffer synchronously but the screen element
|
||||
* takes its new width with the render, so measuring in the same tick reads
|
||||
* the size the terminal just left. Coalesced because a settling container
|
||||
* fires several resizes and only the last one's measurement is the truth.
|
||||
*/
|
||||
_setPtyOversized(oversized) {
|
||||
const container = document.getElementById('terminalContainer');
|
||||
if (container) container.classList.toggle('pty-oversized', !!oversized);
|
||||
if (oversized === this._ptyOversized) return;
|
||||
this._ptyOversized = oversized;
|
||||
if (oversized) {
|
||||
// 53 characters: measured at one line on a 430px phone. The longer
|
||||
// wording wrapped to two, which is a lot of the terminal to cover for a
|
||||
// notice about a condition that resolves itself.
|
||||
this.showToast('Another device is setting the width — scroll sideways', 'info');
|
||||
_scheduleOverflowAffordanceSync() {
|
||||
if (typeof requestAnimationFrame !== 'function') {
|
||||
this._syncTerminalOverflowAffordance();
|
||||
return;
|
||||
}
|
||||
if (this._overflowAffordanceFrame) return;
|
||||
this._overflowAffordanceFrame = requestAnimationFrame(() => {
|
||||
this._overflowAffordanceFrame = null;
|
||||
this._syncTerminalOverflowAffordance();
|
||||
});
|
||||
},
|
||||
|
||||
/**
|
||||
* Let the reader reach a pane wider than the box that shows it.
|
||||
*
|
||||
* ⚠️ Keyed on what actually does not FIT, not on a PTY mismatch. Two
|
||||
* different causes put the terminal wider than its container and both leave
|
||||
* columns unreachable behind `.terminal-container`'s clip:
|
||||
*
|
||||
* - another device holds the sizing claim, so this terminal adopts a width
|
||||
* it did not ask for; and
|
||||
* - the 40-column floor. On a 360px phone, font 18 applies 40 columns and
|
||||
* paints 433px, and font 24 paints 578px — 218px, 38% of the pane, with no
|
||||
* gesture that could reach it. `increaseFontSize` goes to 24 and applies
|
||||
* immediately, so that is two taps away, and the PTY agrees with the
|
||||
* terminal throughout: a mismatch test would never fire.
|
||||
*
|
||||
* Measured rather than derived from cell arithmetic, because the cell width
|
||||
* is fractional and the container's padding is not ours to assume. One pixel
|
||||
* of slack keeps sub-pixel rounding from flapping the class.
|
||||
*/
|
||||
_syncTerminalOverflowAffordance() {
|
||||
// ⚠️ Nothing in here may throw. It runs off every geometry change, which is
|
||||
// the resize path, and the affordance is cosmetic: a terminal that cannot
|
||||
// be measured — disposed mid-resize, or a harness with no real DOM — must
|
||||
// lose the scroll affordance, never the resize.
|
||||
let container = null;
|
||||
let overflows = false;
|
||||
try {
|
||||
container = document.getElementById('terminalContainer');
|
||||
const screen = container?.querySelector('.xterm-screen');
|
||||
if (container && screen) {
|
||||
overflows = screen.getBoundingClientRect().width - container.clientWidth > 1;
|
||||
}
|
||||
} catch {
|
||||
/* unmeasurable; fall through with the affordance off */
|
||||
}
|
||||
container?.classList.toggle('term-overflows-x', overflows);
|
||||
// The notice tells the reader to scroll sideways, so it is only true advice
|
||||
// once there is something to scroll. A wide PTY on a screen wide enough to
|
||||
// show it needs no explanation and gets none.
|
||||
if (!overflows || !this._paneWidthRefused) {
|
||||
this._paneOwnedElsewhere = false;
|
||||
return;
|
||||
}
|
||||
this._notePaneOwnedElsewhere();
|
||||
},
|
||||
|
||||
/**
|
||||
* Say, once, that this pane's width belongs to another device.
|
||||
*
|
||||
* Once per transition, not per report: reports arrive on every resize, and a
|
||||
* toast that repeats is noise about a situation already on screen. Silent
|
||||
* when it resolves — the pane simply reflows back to this screen.
|
||||
*/
|
||||
_notePaneOwnedElsewhere() {
|
||||
if (this._paneOwnedElsewhere) return;
|
||||
this._paneOwnedElsewhere = true;
|
||||
// 53 characters: measured at one line on a 430px phone. The longer
|
||||
// wording wrapped to two, which is a lot of the terminal to cover for a
|
||||
// notice about a condition that resolves itself.
|
||||
this.showToast('Another device is setting the width — scroll sideways', 'info');
|
||||
},
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user