Skip to content

fix(split-pane): let a Shell Pane B's scroll-up reach tmux history - #506

Open
timkjr wants to merge 2 commits into
Ark0N:masterfrom
timkjr:fix/split-pane-scroll-history
Open

timkjr wants to merge 2 commits into
Ark0N:masterfrom
timkjr:fix/split-pane-scroll-history

Conversation

@timkjr

@timkjr timkjr commented Sep 28, 2026

Copy link
Copy Markdown

Follow-up to #494, closing the gap it left open: a split view's second pane (terminal-split.js) had no scroll-to-top pull, so after a burst of output in a Shell Pane B its earlier lines were unreachable.

Summary

  • Pane B is a separate xterm that loads history once at connect. tmux repaints a burst instead of scrolling it, so after a cat the pane keeps about one screen of scrollback while tmux holds every line.
  • A Shell Pane B now does what fix(terminal): let a Shell pane's scroll-up reach tmux history #494 made the primary pane do: wheel-up at the top of the normal screen pulls ?full=1&tail=${TERMINAL_TAIL_SIZE} and replays it under the reader's current place. It cannot reuse _maybeRefetchFullHistory, which reads app.terminal/activeSessionId (Pane A), so SplitTerminalPane gets its own _maybeLoadMoreHistory/_pullHistory. The wheel listener is capture-phase, because xterm stopPropagation()s the events it consumes. The alternate screen (nano, vim, less) is skipped.
  • It follows the primary pane's rules, including the 1.33.2 merge-time fixes. A window with no more rows than the pane holds is skipped without a rewrite; that check also covers a downgrade. So is a pane already at its scrollback + rows cap. A skipped window that came back truncated, or a full pane, backs off to 60 s instead of 4 s, since each ask costs the server a whole-history capture-pane. An untruncated skip keeps 4 s. Pane B has no truncation banner, so the 'tail' relabel does not apply.
  • Live frames arriving mid-replay, a {t:'c'} clear included, are held with their arrival time and applied in order only if they arrived after the capture (the _finishBufferLoad since rule). The fetch has a 10 s deadline, since it holds the pane's live output while it runs. The tail of _loadBuffer() becomes _endBufferLoad(), so the pull shares its single-flight bookkeeping with the initial load and the {t:'r'} refresh.
  • Non-shell Pane B is unchanged: it already loads full=1, and a repaint-mode CLI keeps no tmux history to recover. A shell history longer than the 1 MiB window stays out of reach in Pane B, which has no Load full history button.

Test plan

  • npm run typecheck, npm run lint, npm run format:check, npm run check:frontend-syntax, npm run check:browser-excludes
  • npm test (full suite, 8454 passed): test/split-pane-terminal-unit.test.ts loads the real SplitTerminalPane via vm and covers the pull, cooldown and single-flight, downgrade and skip, the truncated and full-pane 60 s back-off (both fail without it), live-frame queuing across the replay including clear frames, abort/deadline and destroy() mid-pull, and the capture-phase wheel listener.
  • Live in the UI: a Shell session in Pane B, cat of a file much longer than the pane, then wheel-up at the top loads the earlier lines and keeps the reader's place.

🤖 Generated with Claude Code

tmux repaints a burst of output instead of scrolling it, so a shell
pane's xterm keeps about one screen of scrollback while tmux holds every
line. The primary pane goes back for it when the wheel reaches the top;
Pane B is a separate xterm that loaded history once at connect and never
again, so after a `cat` its earlier output was unreachable.

Pane B now does the same for a shell session: wheel-up at the top of the
normal screen pulls ?full=1&tail=TERMINAL_TAIL_SIZE and holds the
reader's place across the replay. The wheel listener is capture-phase
because xterm stopPropagation()s the events it consumes.

It follows the primary pane's rules from Ark0N#494 and its 1.33.2 merge-time
fixes: a window holding no more rows than the pane (which covers a
downgrade), or a pane already at its `scrollback + rows` cap, is skipped
without a rewrite. That skip backs off to 60 s when the window was
truncated or the pane is full, since each ask costs the server a
whole-history capture-pane; an untruncated window keeps the 4 s cooldown.
There is no truncation banner in Pane B, so the 'tail' relabel does not
apply.

Live frames, a {t:'c'} clear included, are held with their arrival time
while the replay runs and applied in order only if they arrived after the
capture. The fetch has a 10 s deadline since it holds live output while
it runs. The tail of _loadBuffer() becomes _endBufferLoad() so the pull
shares its single-flight bookkeeping.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Ark0N

Ark0N commented Sep 29, 2026

Copy link
Copy Markdown
Owner

Thanks for this, and welcome to Codeman! This PR gives a Shell split-view Pane B its own scroll-to-top history pull, so the lines tmux repainted over after a burst are reachable again, following the same rules as the primary pane's #494 pull. It is carefully done: the skip and back-off rules match the primary pane, the live-frame queue handles the capture cutoff, and the tests are real (removing the back-off, the shell gate or the cutoff each makes them fail). I also ran it against the real xterm in Chromium, and the pull fires once at the top and keeps the reader's place.

One thing needs fixing before merge, plus a small parity guard:

  1. The pull erases the "Pane B disconnected" marker (src/web/public/terminal-split.js:454). When the socket drops, onclose (line 295) writes the marker into the buffer, but nothing else records the closure. A later scroll-to-top pull starts its replay with \x1bc, which wipes the marker and paints a fresh, current-looking history, while onData keeps silently dropping every keystroke. That breaks the split-pane rule in docs/architecture-invariants.md that a dropped socket leaves Pane B visibly dead. I reproduced it in both orders: socket closed before the pull, and socket closed while the fetch is in flight. It is easy to hit, because a Codeman restart drops the socket while the tmux session survives, so the HTTP pull succeeds. Suggested fix: set this._wsClosed = true in onclose, move the marker text into a small helper, and call it again in _pullHistory()'s finally (after the queued frames are flushed) when replayed && this._wsClosed. Routing the marker through _onLiveOutput() does not work, because a close that lands before the response is stamped before the cutoff and dropped. Please add a unit test for each order.

  2. Stand aside for a detached session (src/web/public/terminal-split.js:392). _maybeRefetchFullHistory() in app.js returns early when the session is in detachedSessions, and Pane B's own _sendResize() already does the same. Adding if (this.detachedSessions?.has(this.sessionId)) return; to _maybeLoadMoreHistory() keeps the two copies in step, as your CLAUDE.md line asks.

Optional, and fine as a follow-up: _liveQueue opens before the fetch (line 413), so Pane B stops painting for the whole round trip, and that is why the deadline is a fixed 10 s. Opening the queue right after await fetch(...) resolves (beside capturedAt) gives the same final buffer, because every earlier frame is either replaced by the capture or written unchanged. It would also let the pull use the shared CodemanFetchDeadline.terminalFetchDeadlineMs({ full: true }) like the primary pane.

Two wording nits you can take or leave: the comment at line 389 and the new invariants paragraph say a non-shell CLI keeps no tmux history, but codex and Claude's inline renderer do grow it (so it is out of scope rather than empty). The alternate-screen skip only matters for a direct-PTY shell, because under tmux the browser xterm never enters the alternate buffer. In CLAUDE.md line 273, the new sentence could also point at architecture-invariants#split-pane-sessions, where the detail lives.

Once 1 and 2 are in, this is ready to merge.

…sessions

Address Ark0N's review on Ark0N#506:

- The history pull's own `\x1bc` reset erased the "Pane B disconnected"
  marker onclose wrote, painting a fresh, current-looking history while
  onData kept silently dropping every keystroke on the dead socket — a
  Codeman restart drops the socket while the tmux session (and so the HTTP
  pull) survives, making this easy to hit. onclose now tracks the closure
  via `_wsClosed` in addition to writing the marker (extracted into
  `_writeDisconnectedMarker()`), and a replay re-stamps it in the pull's
  `finally` block, after the live-frame flush, whichever order the close
  and the pull land in.
- `_maybeLoadMoreHistory()` now stands aside for a detached session,
  mirroring `_sendResize()`'s existing check and app.js's
  `_maybeRefetchFullHistory()` — its own window already owns its PTY size
  and scrollback.
- Wording: a non-shell CLI's history is out of scope for this pull, not
  absent (codex and Claude's inline renderer do grow tmux history); the
  alternate-screen skip only matters for a direct-PTY shell, since tmux
  never surfaces the alt buffer to the browser xterm. CLAUDE.md points at
  the invariants heading directly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@timkjr

timkjr commented Sep 29, 2026

Copy link
Copy Markdown
Author

Both are fixed in 140ca35.

  1. You're right that it needed to work in both orders. onclose now sets _wsClosed alongside writing the marker, and the pull's finally block re-stamps it after the live-frame flush whenever a replay ran on a closed socket — covers the close landing before the pull starts and mid-fetch. Added a test for each.

  2. Done — _maybeLoadMoreHistory() now stands aside for a detached session, the same check _sendResize() already has.

Took both wording nits too: "out of scope" instead of "none" for non-shell history, the alternate-screen note now says it's a direct-PTY-shell-only concern since tmux never surfaces the alt buffer to the browser xterm, and CLAUDE.md points straight at the invariants heading. Left the live-frame-queue-timing optimization for a follow-up, as you suggested.

Full suite's green (438 files, 8459 tests), plus typecheck/lint/format/frontend-syntax/browser-excludes.

@timkjr

timkjr commented Sep 29, 2026

Copy link
Copy Markdown
Author

Heads up: the failing "Unit & integration tests" run on 140ca35 is npm ci itself — node-pty's postinstall couldn't find a prebuilt binary for the runner and fell back to compiling from source, which then hit EAI_AGAIN trying to reach nodejs.org for headers. Unrelated to this change; the prior run on this branch installed fine with the same deps, and the Typecheck & Lint job on the same commit (which also runs its own npm ci) passed. I don't have rights to re-run it from here — could you kick it again when you get a chance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants