fix: resolve 15-second SSH delay with fish shell prompt detection#1631
Conversation
|
@jasperan is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAppends incoming PTY chunks to a capped rolling buffer, expands escape-stripping to remove OSC/other single-byte escapes and CRs, and runs prompt regexes against the cleaned buffer; adds tests covering fish-style prompts (including prompts split across chunks and noisy escape sequences). Changes
Sequence Diagram(s)sequenceDiagram
participant RemotePTY as Remote PTY
participant Buffer as Rolling Buffer
participant Stripper as stripAnsi
participant Matcher as Prompt Regex
participant Caller as waitForShellPrompt caller
RemotePTY->>Buffer: emit chunk (may be partial)
Buffer->>Buffer: append & trim (max buffer size)
Buffer->>Stripper: sanitize(buffer, options)
Stripper->>Matcher: cleaned text
alt regex matches prompt
Matcher->>Caller: finish() — prompt detected
else no match yet
Matcher-->>RemotePTY: await further chunks
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/utils/__tests__/waitForShellPrompt.test.ts`:
- Around line 350-363: Test title says "detects prompt when > arrives alone
after accumulated context" but the test emits '$ ' instead of '> '; update the
test so the description matches the emitted token: either rename the it(...)
string to reference '$' (e.g. mention "$ arrives alone") or change the emitted
prompt in the body from pty.emit('$ ') to pty.emit('> '); ensure references to
waitForShellPrompt, createMockPty, and pty.emit are updated accordingly so the
assertion remains valid.
🪄 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: 46b76c10-a1ac-4bfe-9612-8d7cd30c6d1c
📒 Files selected for processing (3)
src/main/utils/__tests__/waitForShellPrompt.test.tssrc/main/utils/waitForShellPrompt.tssrc/shared/text/stripAnsi.ts
|
@jasperan thank you for taking this on again! |
…neralaction#1597) Fish shell renders prompts in multiple small writes over SSH, causing the prompt character (>) to arrive in its own TCP segment without preceding context. The per-chunk regex matching would fail and fall back to a 15-second timeout. Two fixes: - Accumulate PTY chunks into a rolling buffer (4KB cap) so split prompts are matched against the full accumulated output - Strip Fe escape sequences (\e7/\e8 cursor save/restore) and OSC-ST terminators that fish appends after the prompt character, which prevented the trailing \s*$ from matching
Rename test from ">" to "$" to match the actual emitted token.
10d88f2 to
ad446aa
Compare
|
done @arnestrickmann |
|
Thanks! |
Summary
Fixes #1597. The previous fix (#1615) addressed a different issue (focus-stealing, #1467) and didn't touch prompt detection.
waitForShellPrompttested each PTY data chunk independently against the prompt regex. Fish shell renders its prompt in multiple small writes over SSH (colored segments, escape sequences, prompt character). When>arrived in its own TCP segment without preceding non-whitespace context, the regex failed and fell back to the 15-second timeout.\e7/\e8) and OSC-ST terminators weren't stripped, leaving non-whitespace bytes after>that broke the\s*$anchor.Fix (2 files, 3 changes):
stripOtherEscapes,stripOscSt,stripCarriageReturn) to handle fish's escape sequencesstripOtherEscapesregex to cover Fe/Fp/Fs single-byte escapes (was only matching[A-Za-z], now matches all except CSI/OSC openers)Test plan
>rejection, Fe escapes, OSC-STwaitForShellPrompttests passstripAnsitests pass (no regressions)Summary by CodeRabbit
Bug Fixes
Tests