Skip to content

Fix Shift+Enter newline and initial PTY size race in terminal#156

Closed
spamsch wants to merge 10 commits into
jsgrrchg:mainfrom
spamsch:worktree-shift-enter-newline
Closed

Fix Shift+Enter newline and initial PTY size race in terminal#156
spamsch wants to merge 10 commits into
jsgrrchg:mainfrom
spamsch:worktree-shift-enter-newline

Conversation

@spamsch
Copy link
Copy Markdown
Contributor

@spamsch spamsch commented May 27, 2026

Summary

  • Shift+Enter inserts a newline in Claude Code's prompt instead of submitting. xterm.js has no built-in Shift+Enter binding and sent \r (carriage return) for both Enter and Shift+Enter — identical to submit. We now intercept the keydown in attachCustomKeyEventHandler, write \n to the PTY (which Claude Code maps to chat:newline, same as Ctrl+J), and return false to suppress xterm's default.

  • Initial PTY resize fires immediately instead of waiting the 80ms settle debounce. The PTY starts at the 120×24 snapshot default; Claude Code's Ink UI renders its first frame against those dimensions. With the debounce, the correct pixel-calculated size doesn't reach the PTY for at least 80ms, so Claude Code's initial render is wrong and requires a Ctrl+L to fix. Skipping the debounce on the first fit (when lastRequestedSizeRef.current === null) matches how Hyper handles componentDidMount — synchronous first fit, debounced everything after.

Upstream tracking issue for the Ink cursor-tracking desync that underlies the redraw bug: anthropics/claude-code#62740

Test plan

  • Open a Claude Code terminal tab. Type a prompt and press Shift+Enter — confirm a newline is inserted and the prompt grows rather than submitting.
  • Open Claude Code in a tall pane (40+ rows). Confirm the prompt renders correctly on first launch without needing Ctrl+L.
  • Drag a pane splitter rapidly while a terminal is open — confirm no PTY resize thrashing (debounce still applies after first fit).
  • Resize window, switch tabs, open/close the right panel — confirm terminal re-fits correctly in all cases.

- Intercept Shift+Enter in xterm's key handler and write \n to the PTY
  instead of letting xterm send \r. Claude Code maps \n to chat:newline
  (same as Ctrl+J), so the prompt grows rather than submitting.

- Fire the first PTY resize immediately on mount instead of waiting the
  80ms debounce. The PTY starts at the 120×24 snapshot default; without
  this, Claude Code renders its initial Ink frame at the wrong dimensions
  and requires Ctrl+L to redraw correctly. Pattern matches Hyper, which
  calls fitAddon.fit() synchronously in componentDidMount then debounces
  all subsequent ResizeObserver-triggered resizes.
@jsgrrchg jsgrrchg marked this pull request as draft May 27, 2026 07:58
@jsgrrchg
Copy link
Copy Markdown
Owner

Good catch I miss this,

About the checks, are failing because the tests still expect the old resize behavior.

This PR now sends the first terminal resize immediately, but TerminalViewport.test.tsx still asserts that resize has not been called yet:

  • TerminalViewport.test.tsx:55
  • TerminalViewport.test.tsx:124

Both now receive an immediate resize(80, 24), which matches the new intended behavior. The tests need to be updated accordingly. Let me know if you can do it or I can take it from here.

Best

Simon Pamies added 2 commits May 27, 2026 14:33
- Reflect that the first PTY resize now fires immediately on mount (no
  80ms debounce): update test 1 to expect resize after flushPromises
  rather than after timer advance; verify no second call after the rAF.

- Update the noisy-resize coalescing test: initial resize fires
  immediately, then the three same-size ResizeObserver callbacks are
  absorbed by the dedup check rather than the debounce timer. Final
  call count is still 1 and no extra timer is queued.

- Expose triggerKeyEvent on the xterm mock (stores the handler from
  attachCustomKeyEventHandler instead of ignoring it) and add it to
  XtermMockInstance type.

- New test: Shift+Enter keydown writes \n and returns false; plain
  Enter and Shift+Enter keyup are not intercepted.
Once rawOutput crosses MAX_RAW_OUTPUT_CHARS, appendTerminalRawOutput trims
the front on every subsequent chunk. The viewport saw the non-prefix result
as a genuine content mismatch and called terminal.reset() + full 2MB rewrite
on every single write — producing a blank-flash loop that never stopped.

Fix: detect the truncation case (rawOutput didn't shrink — only the front
was sliced) and recover the new tail via a 32-byte probe + 32-byte
verification into the previous rawOutput, then write only the new bytes.
Falls back to reset+rewrite for genuine mismatches or bursts > 512KB.

MAX_SEARCH bumped to 512KB (was 100KB) to cover coalesced PTY bursts
(paste echo, large file cat, TUI redraws). False-positive comment softened
— 64 bytes reduces but doesn't eliminate repeated-ANSI-sequence collisions;
a wrong delta produces one correctable garbled frame, not a loop.

Follow-up: surface trimmedFront from appendTerminalRawOutput so the delta
is computed arithmetically instead of via string search (no heuristics).

Tests: assert terminal.reset() is not called on truncation, and that the
final tail bytes reach xterm even when output stops at the trim boundary.
@jsgrrchg
Copy link
Copy Markdown
Owner

Thanks! Next time, if a PR isn’t finished yet, just let me know to wrap it up, I can’t really tell on my end whether you’re done or not. ;)

@jsgrrchg jsgrrchg marked this pull request as ready for review May 27, 2026 15:10
@spamsch spamsch marked this pull request as draft May 27, 2026 16:11
@jsgrrchg
Copy link
Copy Markdown
Owner

I reviewed the code by hand because this is delicate, non of them are blockers, but worth discussing, thanks for marking these as a draft, sorry for changing that 😅

Shift+Enter is applied globally
The handler at TerminalViewport.tsx:320 is attached to every terminal session (regular shells, AI auth modal, etc.), not just Claude Code. For readline-based shells (bash/zsh) it's invisible since both \n and \r trigger accept-line. But for TUIs that distinguish CR from LF (vim in insert mode, some REPLs in raw mode), Shift+Enter will now insert a literal LF where it used to send CR. Probably the desired behavior and consistent with iTerm2/kitty, but worth calling out in the description.

Missing event.preventDefault() on the Shift+Enter branch
The neighboring handlers in the same block (Cmd/Ctrl+F, Escape) call event.preventDefault() before return false. The new Shift+Enter branch only returns false. Returning false already tells xterm not to process the key, so in practice it works, but adding preventDefault() would be consistent with the existing pattern and would harden against the hidden textarea potentially inserting a stray LF.

32-char probe can collide in ANSI-heavy output
The truncation probe (PROBE_LEN = 32 + 32-char verification window) is fine for normal text, but TUI redraws with repeating ANSI sequences (cursor-home + clear-screen, spinners, etc.) can produce false-positive matches. The fallback (terminal.reset() on the next render) self-corrects, so worst case is one glitched frame followed by a clean reset — acceptable, just noting it. Bumping PROBE_LEN to 64 or requiring a non-ESC byte in the probe would reduce the collision surface if it becomes a real issue.

Chunks > 512KB fall through to full reset
With MAX_SEARCH = 512_000, a single PTY chunk larger than ~512KB causing a cap-trim won't find its probe and will trigger a full terminal.reset(). The inline comment already documents this; just confirming it's an accepted trade-off for the common case.

Let me know your thoughts,

Thanks,

@spamsch spamsch marked this pull request as ready for review May 28, 2026 07:43
@spamsch spamsch marked this pull request as draft May 28, 2026 07:44
Consistent with the adjacent Cmd+F and Escape branches; also hardens
against the xterm hidden textarea inserting a stray LF independently
of the return false path.
@spamsch
Copy link
Copy Markdown
Contributor Author

spamsch commented May 28, 2026

event.preventDefault() added — good catch.

Shift+Enter scope: intentional. Readline-based shells treat \n and \r identically so there's no regression; raw-mode TUIs that distinguish CR from LF already had no way to receive Shift+Enter before this change (xterm was sending \r). Consistent with iTerm2/kitty.

Probe collisions: acknowledged and documented in the code. A false positive produces one correctable garbled frame, not a loop — acceptable for a stopgap. The follow-up to surface trimmedFront from appendTerminalRawOutput is tracked in the commit message; that removes the heuristic entirely.

512KB cutoff: accepted trade-off, already noted in the inline comment.

@spamsch
Copy link
Copy Markdown
Contributor Author

spamsch commented May 28, 2026

Also looked at how Hyper handles this. Short answer: it sidesteps the problem entirely by never maintaining a string replay buffer. PTY data flows straight into the live xterm instance via a Redux middleware and is discarded by the app layer — the store holds session metadata only, no output. Inactive tabs are parked offscreen at left: -9999em rather than unmounted, so the xterm instance and its buffer survive tab switches with no replay needed. No buffer cap, no truncation, no reset+rewrite path, no probe.

The trade-off is Hyper makes none of the promises NeverWrite makes: no persistence across reloads, scrollback capped at 1000 lines (xterm's own limit), everything lost on window close. The rawOutput mirror exists here precisely to support persistence and extended history, so Hyper's approach isn't a drop-in. The right long-term fix remains surfacing trimmedFront from appendTerminalRawOutput to make the delta arithmetic instead of a heuristic — the probe is a pragmatic stopgap until that lands.

@spamsch spamsch marked this pull request as ready for review May 28, 2026 07:54
@jsgrrchg
Copy link
Copy Markdown
Owner

Thank you for the detailed response. It was very useful, I had to learn quite a bit about terminals to fully understand it. My initial implementation was very rough, so I really appreciate all the help.

The Hyper comparison is helpful too. I agree it is not a drop-in model for NeverWrite, since we intentionally keep rawOutput for persistence and replay.

My only remaining concern is the truncation probe. I agree the heuristic is acceptable as a short-term stopgap, but I’m not fully convinced the worst case is limited to a single garbled frame. If the probe matches the wrong repeated region, we could compute the wrong delta, write too little of the new tail, update lastRawOutputRef, and silently drop output from xterm’s live buffer. Maybe I'm over thinking this 🤔.

I’d be comfortable merging this if either:

  • we add an immediate follow-up issue or commit for the trimmedFront approach, or
  • we include a regression test with repeated output / repeated ANSI-like content to pin down the current behavior (this is my favorite approach, the terminal is already working great).

No blocker on the Shift+Enter part from my side. I just want to be rigorous about the truncation edge case, but if you think is good enough, I'll merge as is.

@spamsch
Copy link
Copy Markdown
Contributor Author

spamsch commented May 28, 2026

You're right to push on this — my "one garbled frame" description undersells the risk. If the probe matches the wrong position, the delta arithmetic is wrong, lastRawOutputRef advances past the true match, and that content is gone from xterm with no self-correction.

Given that, I'd go with the regression test. A test with a repeated or ANSI-heavy tail would pin the probe behavior concretely and catch any regression if the heuristic ever gets tuned. The trimmedFront follow-up is the right permanent fix, but a test gives this PR a real safety net in the meantime — and it's clearly something we can write now since we understand the failure mode.

I'll add a test that covers:

  • repeated plain-text output (e.g. "a".repeat(200)) as the replay buffer tail
  • a probe window that could match in multiple positions

If the test passes and it accurately describes the current behavior, that gives us a baseline to validate the trimmedFront rewrite against. I'll open a follow-up issue for that work so it doesn't get lost.

@jsgrrchg jsgrrchg marked this pull request as draft May 28, 2026 18:44
@jsgrrchg
Copy link
Copy Markdown
Owner

I deleted my previous message, I read to fast your message, I'll wait for the test before closing this PR. I want to push another update today to include Opus 4.8 support. Thank you ;)

@spamsch
Copy link
Copy Markdown
Contributor Author

spamsch commented May 29, 2026

Tests added and pushed (69b6f74). Two cases: repeated-content overlap with correct probe hit (no reset, exact delta written); repeated-content with early false-positive hit (verification rejects, falls to reset, no silent drop). Assertions use toBe on the full expected screen state.

Two new cases: probe finds correct offset when overlap is plain repeated
characters; probe hits wrong earlier position and verification rejects it,
confirming no silent output drop (falls through to reset + full replay).
@spamsch spamsch marked this pull request as ready for review May 29, 2026 06:35
@spamsch spamsch marked this pull request as draft May 29, 2026 06:39
@spamsch
Copy link
Copy Markdown
Contributor Author

spamsch commented May 29, 2026

Found another bug when removing lines. The bottom part does not redraw and leaves artifacts. Checking.

spamsch and others added 2 commits May 29, 2026 08:41
event.preventDefault() doesn't reliably suppress Electron's hidden textarea
from emitting a second \n through onData after the explicit write in the
custom key handler. Each Shift+Enter was reaching the PTY twice; Claude Code's
Ink prompt grew by N rows but tracked N/2 internally, leaving blank ghost rows
on screen when lines were deleted.

Fix: set a flag after the explicit write and drop the next \n that arrives
through onData while the flag is set.
@jsgrrchg
Copy link
Copy Markdown
Owner

Gooood catch, I missed that one. I found one more issue while reviewing the immediate initial resize path.

TerminalViewport marks the first fitted size as requested before we know whether it actually reached the backend. If the viewport mounts while snapshot.sessionId is still empty, resize() returns early in terminalRuntimeStore because there is no active session yet. But lastRequestedSizeRef has already been set to the fitted size.

When the real sessionId arrives, syncSize() can then hit the queued-size dedupe and skip sending the resize again. That leaves the PTY at the old initial size, even though the xterm viewport has already fitted to the correct dimensions.

So the issue is not the Shift+Enter ghost-row fix; it is specifically the initial resize bookkeeping. I think the fix should either scope/reset lastRequestedSizeRef when snapshot.sessionId changes, or avoid recording the size as requested until there is a real session to receive it.

@spamsch
Copy link
Copy Markdown
Contributor Author

spamsch commented May 29, 2026

Superseded by the xterm direct-pipe rewrite. The Shift+Enter and immediate-resize fixes from here are already on main; the truncation-probe approach is removed entirely because the new pipeline has no accumulator to diff. Replacement PR incoming.

@jsgrrchg
Copy link
Copy Markdown
Owner

Thank you SImon :)

@spamsch spamsch deleted the worktree-shift-enter-newline branch May 29, 2026 14:00
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