Skip to content

fix(host-service): restore terminal modes on reattach via headless xterm#4231

Merged
Kitenite merged 2 commits into
mainfrom
codex-tui-newline-debug
May 8, 2026
Merged

fix(host-service): restore terminal modes on reattach via headless xterm#4231
Kitenite merged 2 commits into
mainfrom
codex-tui-newline-debug

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented May 8, 2026

Summary

  • Refreshing the renderer mid-session was dropping the codex TUI's kitty keyboard mode, so Shift+Enter started submitting instead of inserting a newline. Same class of bug latent for bracketed paste, focus reporting, mouse, and other DEC modes set once at program start.
  • Root cause: mode-setting escapes (e.g. \x1b[>7u) are emitted once at startup, broadcast straight to the live socket, and never enter the 64 KiB FIFO replay — a freshly attaching xterm has no way to learn them.
  • Fix: feed every PTY chunk through an @xterm/headless instance that tracks current mode state, and prepend a \x1b[…] preamble (built from the tracker) on every reattach. Mirrors VSCode's XtermSerializer pattern (src/vs/platform/terminal/node/ptyService.ts); other xterm.js consumers I checked (Tabby, Wave, cmux) all have the same latent gap.
  • Plan: plans/20260507-terminal-mode-replay.md.

Two headless-specific gotchas, documented inline:

  • Terminal.write is async-buffered. We synchronously drain via the internal _core._writeBuffer.writeSync(...) (xterm's own SerializeAddon does the same) so term.modes reflects the feed immediately when replayBuffer builds the preamble.
  • vtExtensions.kittyKeyboard is in the public typings, but headless's option sanitizer drops it (its DEFAULT_OPTIONS table omits the key). Without injecting it onto rawOptions post-construction, kitty handlers early-return and \x1b[>7u is silently ignored.

Test plan

  • New terminal-mode-tracker.test.ts — 10 cases (kitty push survives 200 KB of unrelated output, kitty pop, kitty set-to-zero, bracketed paste, focus + mouse SGR, multi-mode preamble ordering, hide-cursor, resize idempotence, escape sequences split across feeds, default state).
  • bun test src/terminal/ — 47/47 pass.
  • bun run typecheck — clean across all 29 workspace tasks.
  • bun run lint — clean.
  • Manual: open codex CLI in terminal v2, refresh renderer mid-conversation, verify Shift+Enter still inserts a newline (previously broke).
  • Manual: open another kitty-aware TUI (claude-code), same refresh, same check.
  • Manual: refresh while a plain shell is foreground — confirm Tab/Enter still behave normally (preamble emits no kitty when flags=0).

Summary by cubic

Restore terminal modes on renderer reattach by tracking PTY output with @xterm/headless and sending a mode preamble. Fixes lost kitty keyboard (Shift+Enter), bracketed paste, focus, mouse tracking, and other DEC modes after refresh.

  • Bug Fixes

    • Feed all PTY bytes (including held marker bytes) through a headless xterm tracker and prepend a preamble on every attach, even when the FIFO is empty.
    • Preamble restores kitty flags, bracketed paste, focus reporting, mouse tracking, app cursor keys/keypad, wrap, and cursor visibility; mouse encoding stays default due to xterm API limits.
    • Keep tracker in sync on resize; use internal writeSync for immediate parse and enable vtExtensions.kittyKeyboard.
    • Added tests for kitty flags across large output, DEC modes, split sequences, preamble ordering, default state, and resize.
  • Dependencies

    • Added @xterm/headless@6.1.0-beta.197.

Written for commit 759881c. Summary will update on new commits.

Summary by CodeRabbit

  • New Features

    • Terminal modes and settings are preserved on reattach so keyboard features, mouse modes, bracketed paste, focus, and cursor state persist across reconnects.
  • Tests

    • Added tests validating mode tracking, preamble generation, split-sequence reassembly, and resize/reattach behavior.
  • Documentation

    • Added a technical plan describing the mode-replay approach and repro steps.
  • Chores

    • Added headless terminal support dependency to enable state-driven preamble generation.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 8, 2026

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f37678fe-c635-415f-9a55-bab1dcc705b6

📥 Commits

Reviewing files that changed from the base of the PR and between cd1f109 and 759881c.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • packages/host-service/package.json

📝 Walkthrough

Walkthrough

The PR adds a per-session headless xterm-based ModeTracker that ingests PTY output, computes a mode-setting preamble (kitty keyboard, bracketed paste, focus, mouse SGR, cursor state), and prepends that preamble before replaying buffered PTY bytes on reattach.

Changes

Terminal Mode Replay on Reattach

Layer / File(s) Summary
Plan & Context
plans/20260507-terminal-mode-replay.md
Documents the FIFO-eviction repro and outlines using @xterm/headless to synthesize a preamble for replay.
Dependency
packages/host-service/package.json
Adds @xterm/headless@6.1.0-beta.197 to dependencies.
Mode Tracker Implementation
packages/host-service/src/terminal/terminal-mode-tracker.ts
Adds ModeTracker interface and createModeTracker() which feeds PTY bytes into a headless terminal and synthesizes a preamble from headless mode state (application cursor keys, bracketed paste, focus, mouse SGR, kitty keyboard flags, cursor visibility).
Terminal Session Wiring
packages/host-service/src/terminal/terminal.ts
Per-session modeTracker is created, fed PTY output and marker bytes, resized on socket resize, used to prepend a mode preamble in replayBuffer(), and disposed on session teardown.
Test Coverage
packages/host-service/src/terminal/terminal-mode-tracker.test.ts
12 Bun tests covering default state, kitty push/pop/zero handling, bracketed paste, focus/mouse SGR, combined ordering, cursor visibility, resize idempotency, and split-sequence reassembly.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Session
  participant ModeTracker
  participant FIFO
  Client->>Session: attach/reattach
  Session->>ModeTracker: buildPreamble()
  ModeTracker-->>Session: preamble bytes or null
  Session->>FIFO: read buffered PTY bytes
  Session-->>Client: send (preamble + FIFO bytes)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

  • superset-sh/superset#4231: Same feature-area changes — headless xterm dependency, ModeTracker implementation, tests, and integration into terminal replay.

Poem

🐰 A rabbit hops through terminal modes bright,
Headless eyes watch escapes in the night,
Kitty keys remembered, preambles sing,
Reattached shells resume their old swing,
Hop, tap, resume — the session takes flight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: restoring terminal modes on reattach via headless xterm, which directly addresses the core bug fixed in this PR.
Description check ✅ Passed The PR description covers all required template sections: clear summary of changes, related technical details, and testing information. However, it lacks explicit links to related GitHub issues and does not clearly categorize the change type in checkbox format as the template specifies.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex-tui-newline-debug

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Greptile Summary

This PR fixes terminal mode loss (kitty keyboard protocol, bracketed paste, focus reporting, mouse tracking) when a renderer reattaches mid-session by feeding every PTY output chunk through a headless @xterm/headless instance and prepending a synthesized mode-restore preamble to each reattach replay. The approach mirrors VSCode's XtermSerializer pattern but generates the preamble explicitly rather than relying on the live process to re-emit.

  • terminal-mode-tracker.ts: new createModeTracker / buildPreambleFromHeadless — feeds bytes synchronously via internal _writeBuffer.writeSync, patches rawOptions.vtExtensions to enable kitty keyboard tracking, and reads kitty flags via _core.coreService.kittyKeyboard.
  • terminal.ts: wires ModeTracker into TerminalSession — feeds the tracker before broadcast/buffer in onOutput, prepends the preamble in replayBuffer, resizes on client resize messages, and disposes cleanly in all teardown paths.
  • terminal-mode-tracker.test.ts: 10 tests cover kitty push/pop/zero-set, bracketed paste, focus+mouse tracking, multi-mode ordering, cursor hide, resize idempotence, and split-feed parsing.

Confidence Score: 3/5

The kitty keyboard restore is safe to merge; the SGR mouse encoding gap and unguarded _writeBuffer access are the main concerns before shipping.

The kitty keyboard fix is sound and well-tested. The integration in terminal.ts is clean — tracker is fed before broadcast, disposed in every teardown path, and resized on client resize. Two gaps stand out: SGR mouse encoding (?1006h) is explicitly called out in the plan and PR description but is not emitted in the preamble (xterm's public modes API doesn't expose it, and no internal workaround was added as was done for kitty flags); the "focus reporting and mouse SGR" test name implies SGR encoding coverage it doesn't provide, and for any session running a mouse-enabled TUI in a wide terminal, mouse coordinates will be garbled after renderer reattach. Additionally, _core._writeBuffer is accessed without the same null guard applied to optionsService.

packages/host-service/src/terminal/terminal-mode-tracker.ts — SGR mouse encoding gap and unguarded _writeBuffer access; packages/host-service/src/terminal/terminal-mode-tracker.test.ts — misleading "mouse SGR" test name.

Important Files Changed

Filename Overview
packages/host-service/src/terminal/terminal-mode-tracker.ts New module that feeds PTY output through a headless xterm to track terminal mode state; relies on internal xterm APIs (_writeBuffer, _core.coreService.kittyKeyboard) that are unguarded for _writeBuffer, and omits SGR mouse encoding (?1006h) from the preamble despite the plan listing it.
packages/host-service/src/terminal/terminal-mode-tracker.test.ts 10 test cases covering kitty push/pop/set, bracketed paste, focus, multi-mode ordering, cursor hide, resize idempotence, and split escapes; "focus reporting and mouse SGR" test name is misleading as it doesn't exercise actual SGR encoding (?1006h).
packages/host-service/src/terminal/terminal.ts Integrates ModeTracker into TerminalSession: feeds PTY bytes to the tracker before broadcast/buffer, prepends preamble to the replay buffer on reattach, resizes tracker on client resize, and disposes cleanly in all teardown paths.
packages/host-service/package.json Adds @xterm/headless 6.1.0-beta.197 as a runtime dependency.
plans/20260507-terminal-mode-replay.md Design document for the mode-replay feature; implementation matches the plan except that mouse SGR (?1006h) listed in the plan is not included in the final preamble.

Sequence Diagram

sequenceDiagram
    participant PTY as pty-daemon
    participant Host as host-service (terminal.ts)
    participant Tracker as ModeTracker (headless xterm)
    participant Buffer as FIFO replay buffer
    participant WS as renderer WebSocket

    PTY->>Host: onOutput(chunk)
    Host->>Tracker: feed(bytes) [writeSync — synchronous parse]
    alt socket connected
        Host->>WS: broadcastBytes(bytes)
    else no socket
        Host->>Buffer: bufferOutput(bytes)
    end

    note over Host,WS: Renderer refresh / new attach

    WS->>Host: WebSocket onOpen
    Host->>Tracker: buildPreamble()
    Tracker-->>Host: Uint8Array (mode-restore escapes) or null
    Host->>WS: sendBytes(preamble + FIFO contents)
    note over WS: xterm in renderer now has correct kitty flags,<br/>bracketed paste, focus, mouse tracking mode
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
packages/host-service/src/terminal/terminal-mode-tracker.ts:150-154
**Unguarded `_writeBuffer` access**

`internals._core.optionsService` is guarded with `?.` before the injection, but `internals._core._writeBuffer` is stored unconditionally. If the headless xterm version ever lacks this internal property, `writeBuffer` will be `undefined` and every call to `feed()` will throw a `TypeError`, crashing the session's `onOutput` handler silently. A null-guard similar to what is done for `optionsService` would make the failure mode explicit.

### Issue 2 of 2
packages/host-service/src/terminal/terminal-mode-tracker.test.ts:67-74
**Test name "mouse SGR" doesn't cover SGR mouse encoding**

The test feeds `\x1b[?1002h` (button-tracking mode) and `\x1b[?1004h` (focus reporting) — neither is the SGR mouse-encoding sequence (`\x1b[?1006h`). The name "mouse SGR" implies SGR encoding (`?1006h`) is exercised here, but it isn't. This is notable because the implementation plan explicitly lists `\x1b[?1006h` as a mode to be re-emitted on reattach. Without `?1006h` in the preamble, terminal applications that rely on SGR mouse encoding (all modern TUIs with mouse support) will fall back to legacy X10 encoding after reattach, clipping mouse coordinates to ≤223 columns/rows in wide terminals. The test name should be updated to reflect what is actually covered, and a follow-up test or implementation for SGR encoding (`?1006h`) should be considered.

Reviews (1): Last reviewed commit: "fix(host-service): restore terminal mode..." | Re-trigger Greptile

Comment on lines +150 to +154
const writeBuffer = internals._core._writeBuffer;

return {
feed(bytes) {
writeBuffer.writeSync(bytes);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unguarded _writeBuffer access

internals._core.optionsService is guarded with ?. before the injection, but internals._core._writeBuffer is stored unconditionally. If the headless xterm version ever lacks this internal property, writeBuffer will be undefined and every call to feed() will throw a TypeError, crashing the session's onOutput handler silently. A null-guard similar to what is done for optionsService would make the failure mode explicit.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/terminal/terminal-mode-tracker.ts
Line: 150-154

Comment:
**Unguarded `_writeBuffer` access**

`internals._core.optionsService` is guarded with `?.` before the injection, but `internals._core._writeBuffer` is stored unconditionally. If the headless xterm version ever lacks this internal property, `writeBuffer` will be `undefined` and every call to `feed()` will throw a `TypeError`, crashing the session's `onOutput` handler silently. A null-guard similar to what is done for `optionsService` would make the failure mode explicit.

How can I resolve this? If you propose a fix, please make it concise.

Comment thread packages/host-service/src/terminal/terminal-mode-tracker.test.ts Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/host-service/src/terminal/terminal-mode-tracker.ts`:
- Around line 137-151: The code dereferences private xterm internals without a
safety check; before using internals._core._writeBuffer, add an explicit guard
that verifies internals, internals._core, and
internals._core._writeBuffer?.writeSync exist (e.g. check
internals?._core?._writeBuffer?.writeSync) and if not present throw or log a
clear error like "Terminal writeBuffer.writeSync not available — incompatible
xterm internals" so session creation fails fast instead of crashing later;
update the block around internals, _core, optionsService/rawOptions.vtExtensions
and the subsequent writeBuffer usage to perform this guarded check.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 86faf266-3682-47f5-ac9b-2606d72a3e7f

📥 Commits

Reviewing files that changed from the base of the PR and between 60af03d and 696f8cf.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • packages/host-service/package.json
  • packages/host-service/src/terminal/terminal-mode-tracker.test.ts
  • packages/host-service/src/terminal/terminal-mode-tracker.ts
  • packages/host-service/src/terminal/terminal.ts
  • plans/20260507-terminal-mode-replay.md

Comment thread packages/host-service/src/terminal/terminal-mode-tracker.ts Outdated
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 6 files

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

🚀 Preview Deployment

🔗 Preview Links

Service Status Link
Neon Database (Neon) View Branch
Vercel API (Vercel) Open Preview
Vercel Web (Vercel) Open Preview
Vercel Marketing (Vercel) Open Preview
Vercel Admin (Vercel) Open Preview
Vercel Docs (Vercel) Open Preview

Preview updates automatically with new commits

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
packages/host-service/src/terminal/terminal-mode-tracker.ts (1)

47-60: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard private xterm internals before dereference.

Line 53/Line 60 and Line 96 assume private @xterm/headless internals always exist; if they drift, session attach can crash with a TypeError instead of failing fast with a clear error.

Proposed hardening diff
 type HeadlessInternals = {
-	_core: {
+	_core?: {
 		_writeBuffer: { writeSync(data: string | Uint8Array): void };
 		coreService: { kittyKeyboard?: { flags: number } };
 		optionsService: {
 			rawOptions: { vtExtensions?: { kittyKeyboard?: boolean } };
 		};
 	};
 };
@@
 	const internals = term as unknown as HeadlessInternals;
+	const core = internals._core;
+	if (!core?._writeBuffer?.writeSync || !core.optionsService?.rawOptions) {
+		term.dispose();
+		throw new Error(
+			"[terminal] Incompatible `@xterm/headless` internals: missing _core._writeBuffer.writeSync/optionsService.rawOptions",
+		);
+	}
@@
-	internals._core.optionsService.rawOptions.vtExtensions = {
+	core.optionsService.rawOptions.vtExtensions = {
 		kittyKeyboard: true,
 	};
@@
-	const writeBuffer = internals._core._writeBuffer;
+	const writeBuffer = core._writeBuffer;
@@
-		const kittyFlags = internals._core.coreService.kittyKeyboard?.flags ?? 0;
+		const kittyFlags = core.coreService.kittyKeyboard?.flags ?? 0;

Run this to verify the pinned package still exposes the internal members being used:

#!/bin/bash
set -euo pipefail

VER="6.1.0-beta.197"
META_JSON="$(curl -fsSL "https://unpkg.com/@xterm/headless@${VER}/?meta")"
TERMINAL_PATH="$(jq -r '.files[].path' <<<"$META_JSON" | rg '/Terminal\.(js|mjs)$' | head -n1)"

echo "Inspecting: ${TERMINAL_PATH}"
curl -fsSL "https://unpkg.com/@xterm/headless@${VER}${TERMINAL_PATH}" \
  | rg -n "_core|_writeBuffer|writeSync|optionsService|rawOptions|kittyKeyboard"

Also applies to: 96-97

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/host-service/src/terminal/terminal-mode-tracker.ts` around lines 47
- 60, The code dereferences private xterm/headless internals (term as
HeadlessInternals -> internals._core.optionsService.rawOptions and
internals._core._writeBuffer) without guards; add defensive checks that
internals and the nested properties exist before using them (validate internals,
internals._core, internals._core.optionsService,
internals._core.optionsService.rawOptions, and internals._core._writeBuffer),
and if any is missing throw or return a clear, descriptive error (e.g.,
"unsupported xterm/headless version: missing internal <symbol>") so session
attach fails fast instead of crashing with a TypeError; update the spots where
you set vtExtensions and assign writeBuffer to use these guards (refer to
internals, internals._core.optionsService.rawOptions.vtExtensions, and
internals._core._writeBuffer).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@packages/host-service/src/terminal/terminal-mode-tracker.ts`:
- Around line 47-60: The code dereferences private xterm/headless internals
(term as HeadlessInternals -> internals._core.optionsService.rawOptions and
internals._core._writeBuffer) without guards; add defensive checks that
internals and the nested properties exist before using them (validate internals,
internals._core, internals._core.optionsService,
internals._core.optionsService.rawOptions, and internals._core._writeBuffer),
and if any is missing throw or return a clear, descriptive error (e.g.,
"unsupported xterm/headless version: missing internal <symbol>") so session
attach fails fast instead of crashing with a TypeError; update the spots where
you set vtExtensions and assign writeBuffer to use these guards (refer to
internals, internals._core.optionsService.rawOptions.vtExtensions, and
internals._core._writeBuffer).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8378e7db-b921-4ac1-88c3-dd64650e3437

📥 Commits

Reviewing files that changed from the base of the PR and between 696f8cf and e76025b.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • packages/host-service/package.json
  • packages/host-service/src/terminal/terminal-mode-tracker.test.ts
  • packages/host-service/src/terminal/terminal-mode-tracker.ts
  • packages/host-service/src/terminal/terminal.ts
  • plans/20260507-terminal-mode-replay.md
✅ Files skipped from review due to trivial changes (1)
  • packages/host-service/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
  • plans/20260507-terminal-mode-replay.md
  • packages/host-service/src/terminal/terminal.ts

Renderer refreshes mid-session were dropping kitty keyboard, bracketed
paste, focus reporting, mouse, and other DEC private modes — codex CLI's
Shift+Enter started submitting instead of inserting a newline because the
mode-setting escape (`\x1b[>7u`) was emitted once at startup, broadcast
straight to the live socket, and never made it into the FIFO replay.

Mirror VSCode's `XtermSerializer` approach: feed every PTY chunk through
an `@xterm/headless` instance that tracks current mode state. On
`replayBuffer`, prepend a preamble built from the tracker so a freshly
created xterm rejoins in the modes the running program already believes
are active. The preamble is now sent on every attach (even when the FIFO
is empty), so the renderer is never wedged at default modes.

Two headless-specific quirks worth flagging:
- `Terminal.write` is async-buffered; we use the internal
  `_core._writeBuffer.writeSync(...)` so `term.modes` is up-to-date when
  we build the preamble. Same engine, same path xterm's own
  SerializeAddon takes.
- `vtExtensions.kittyKeyboard` is in the public typings but headless's
  option sanitizer drops it, so kitty handlers early-return. Inject it
  on `rawOptions` post-construction.

Plan: plans/20260507-terminal-mode-replay.md
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
packages/host-service/src/terminal/terminal.ts (2)

825-869: ⚖️ Poor tradeoff

Adoption path note: tracker is rebuilt from whatever the daemon replays.

onOutput correctly feeds every chunk through session.modeTracker before broadcast, so live mode escapes are captured. Worth flagging for awareness (not a blocker for this PR): on host-service restart with adoptOnly/replayOnAdoption, the tracker is freshly constructed and only learns modes that are still inside the daemon's ring buffer. If startup mode escapes were already evicted on the daemon side, the preamble after a subsequent reattach will be incomplete. This is strictly better than the pre-PR behaviour (no preamble at all), but if you want full coverage across host-service restarts you'd eventually need the daemon to track modes too.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/host-service/src/terminal/terminal.ts` around lines 825 - 869,
Adoption replay can miss startup mode escapes because session.modeTracker is
reconstructed only from the daemon's ring-buffer replay (daemon.subscribe with
replayOnAdoption); to fix, persist or transmit mode state on adoption so the
newly constructed session.modeTracker is initialized with prior modes: either
(a) extend daemon.subscribe/daemon replay to include a compact mode-history
payload alongside output chunks so the handler in onOutput can seed
session.modeTracker before feeding chunks, or (b) store the last-known mode
state in session metadata and restore it when creating session.modeTracker
(before calling session.modeTracker.feed in onOutput); reference
session.modeTracker, daemon.subscribe (replayOnAdoption), resolveShellReady,
broadcastBytes, and bufferOutput to find the replay/feeding site to implement
the seeding.

450-474: 💤 Low value

Replay path is correct; preamble + FIFO concatenation is sound.

Allocating a single combined Uint8Array and prepending the preamble before the FIFO is the right shape for the renderer attach hot path. The early-return when both are empty avoids spurious zero-byte sends.

Minor (skippable): bufferTotal is recomputed by walking session.buffer, but session.bufferBytes is already maintained as the exact same total in bufferOutput. Swapping the loop for session.bufferBytes would be a one-line tidy-up.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/host-service/src/terminal/terminal.ts` around lines 450 - 474, The
replayBuffer function recomputes bufferTotal by iterating session.buffer even
though session.bufferBytes already tracks the total; replace the manual loop and
use session.bufferBytes when sizing the combined Uint8Array and computing
preambleLen+bufferTotal (keep all other logic: preamble from
session.modeTracker.buildPreamble(), concatenation into combined, clearing
session.buffer and session.bufferBytes, and calling sendBytes(socket, combined))
so the change is a one-line tidy-up relying on session.bufferBytes instead of
the loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 825-869: Adoption replay can miss startup mode escapes because
session.modeTracker is reconstructed only from the daemon's ring-buffer replay
(daemon.subscribe with replayOnAdoption); to fix, persist or transmit mode state
on adoption so the newly constructed session.modeTracker is initialized with
prior modes: either (a) extend daemon.subscribe/daemon replay to include a
compact mode-history payload alongside output chunks so the handler in onOutput
can seed session.modeTracker before feeding chunks, or (b) store the last-known
mode state in session metadata and restore it when creating session.modeTracker
(before calling session.modeTracker.feed in onOutput); reference
session.modeTracker, daemon.subscribe (replayOnAdoption), resolveShellReady,
broadcastBytes, and bufferOutput to find the replay/feeding site to implement
the seeding.
- Around line 450-474: The replayBuffer function recomputes bufferTotal by
iterating session.buffer even though session.bufferBytes already tracks the
total; replace the manual loop and use session.bufferBytes when sizing the
combined Uint8Array and computing preambleLen+bufferTotal (keep all other logic:
preamble from session.modeTracker.buildPreamble(), concatenation into combined,
clearing session.buffer and session.bufferBytes, and calling sendBytes(socket,
combined)) so the change is a one-line tidy-up relying on session.bufferBytes
instead of the loop.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 88d53b50-0e5d-4f56-a9df-4b32d936c8a2

📥 Commits

Reviewing files that changed from the base of the PR and between e76025b and cd1f109.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • packages/host-service/package.json
  • packages/host-service/src/terminal/terminal-mode-tracker.test.ts
  • packages/host-service/src/terminal/terminal-mode-tracker.ts
  • packages/host-service/src/terminal/terminal.ts
  • plans/20260507-terminal-mode-replay.md
✅ Files skipped from review due to trivial changes (2)
  • packages/host-service/package.json
  • plans/20260507-terminal-mode-replay.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/host-service/src/terminal/terminal-mode-tracker.test.ts

@Kitenite Kitenite merged commit 118f5d1 into main May 8, 2026
9 of 11 checks passed
@Kitenite Kitenite deleted the codex-tui-newline-debug branch May 8, 2026 04:51
MocA-Love pushed a commit to MocA-Love/superset that referenced this pull request May 21, 2026
…erm (superset-sh#4231)

Renderer refreshes mid-session were dropping kitty keyboard, bracketed
paste, focus reporting, mouse, and other DEC private modes — codex CLI's
Shift+Enter started submitting instead of inserting a newline because the
mode-setting escape (`\x1b[>7u`) was emitted once at startup, broadcast
straight to the live socket, and never made it into the FIFO replay.

Mirror VSCode's `XtermSerializer` approach: feed every PTY chunk through
an `@xterm/headless` instance that tracks current mode state. On
`replayBuffer`, prepend a preamble built from the tracker so a freshly
created xterm rejoins in the modes the running program already believes
are active. The preamble is now sent on every attach (even when the FIFO
is empty), so the renderer is never wedged at default modes.

Two headless-specific quirks worth flagging:
- `Terminal.write` is async-buffered; we use the internal
  `_core._writeBuffer.writeSync(...)` so `term.modes` is up-to-date when
  we build the preamble. Same engine, same path xterm's own
  SerializeAddon takes.
- `vtExtensions.kittyKeyboard` is in the public typings but headless's
  option sanitizer drops it, so kitty handlers early-return. Inject it
  on `rawOptions` post-construction.

Plan: plans/20260507-terminal-mode-replay.md
MocA-Love added a commit to MocA-Love/superset that referenced this pull request May 21, 2026
…low-up

superset-sh#4074 exhaustive worktree search in v2 branch picker:
- adopt.ts / search-branches.ts / branch-search.ts: whole-file upstream
  overwrite. Fork's <repoPath>/.worktrees/<branch>-scoped variant
  (FORK NOTE in branch-search.ts) is dropped in favour of upstream's
  exhaustive 'all git worktrees including foreign ones' strategy. This
  matches superset-sh#4226's adopt-foreign-worktree UX direction.

superset-sh#4160 listBranches single git spawn perf (skipped, HEAD-preferred):
- packages/host-service/src/trpc/router/git/git.ts: keep fork's rich
  listBranches (sortOrder/pinDefault/buildBranch per-branch
  upstream/ahead-behind/last-commit). Upstream's 4xN -> 1 spawn
  simplification drops those fields the fork's BaseBranchSelector
  consumes; perf win not worth the rich UI regression. Revisit if
  BaseBranchSelector is unified with upstream's BaseBranchSelector
  in a future cycle.

superset-sh#4218 respawn host-service on Electron auto-update:
- packages/host-service/package.json: merge fork's ./settings export
  and upstream's ./package.json export (both needed).
- packages/host-service/src/trpc/router/host/host.ts: whole-file upstream
  overwrite. HOST_SERVICE_VERSION switches from fork's hard-coded
  '0.7.0' to dynamic hostServicePackageJson.version (currently 0.8.1).
  Bumps via apps/desktop/electron-builder.ts respawn flow are now
  automatic.

superset-sh#4231 restore terminal modes on reattach via headless xterm:
- packages/host-service/src/terminal/terminal.ts: createModeTracker
  call sites referenced un-declared 'cols, rows' inside session-create
  closure; fix to hard-code (120, 32) which matches the daemon.open()
  call above. Real cols/rows arrive via subsequent resize() calls;
  modeTracker.resize handles the update path.

Files clean cherry-picked: superset-sh#4046 host-service bump 0.7.0, plus
adopt-foreign-worktree wiring.
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.

1 participant