Fix terminal reattach scroll jumps#955
Conversation
📝 WalkthroughWalkthroughThe PR introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/utils.ts`:
- Around line 23-26: stripClearScrollback currently does a stateless replaceAll
which fails when the ESC sequence "\x1b[3J" is split across chunks; change it to
a stateful stripper that preserves trailing partial sequences and removes
completed "\x1b[3J" only when fully seen. Implement this by turning
stripClearScrollback into a factory that returns a function/closure which keeps
a small carry-over buffer (tracking "", "\x1b", "\x1b[", "\x1b[3") between
calls, or update the stream hook to buffer joins before calling the existing
function; update all call sites to use the returned function (or the buffered
input) and ensure any leftover partial sequence is kept for the next invocation
to avoid passing incomplete ESC sequences to xterm.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalRestore.ts (1)
162-164: Consider passingfalsedirectly for clarity.Since this code is in the
elsebranch ofif (result.isNew),result.isNewis alwaysfalsehere. Passing it explicitly asfalsewould make this more self-documenting, though the current approach maintains callback signature consistency.♻️ Optional: More explicit parameter
} else { - restoreViewportRef.current?.(xterm, result.isNew); + restoreViewportRef.current?.(xterm, false /* isNew */); }apps/desktop/src/main/lib/terminal/daemon/history-manager.ts (1)
84-111: Remove the unused_getSessionparameter fromwriteToHistory.The
_getSessionparameter is never referenced in the method body. The single call site indaemon-manager.tsunnecessarily passes a callback, creating maintenance burden. Since this is internal code with no API compatibility constraints, removing the parameter and updating the call site is straightforward.
| export function stripClearScrollback(data: string): string { | ||
| const ESC = "\x1b"; | ||
| return data.replaceAll(`${ESC}[3J`, ""); | ||
| } |
There was a problem hiding this comment.
Stateless ED3 stripping can miss split escape sequences.
If \x1b and [3J arrive in different chunks, replaceAll won’t match and the scrollback clear still reaches xterm. Consider a stateful stripper (carry-over of trailing ESC, ESC[, ESC[3) or implement the buffering in the stream hook.
💡 Example stateful stripper (requires updating the call site to keep the returned function)
-export function stripClearScrollback(data: string): string {
- const ESC = "\x1b";
- return data.replaceAll(`${ESC}[3J`, "");
-}
+export function createClearScrollbackStripper(): (data: string) => string {
+ const ESC = "\x1b";
+ let carry = "";
+ return (data: string) => {
+ const input = carry + data;
+ const cleaned = input.replaceAll(`${ESC}[3J`, "");
+ const carryMatch = cleaned.match(/(\x1b|\x1b\[|\x1b\[3)$/);
+ carry = carryMatch?.[0] ?? "";
+ return carry ? cleaned.slice(0, -carry.length) : cleaned;
+ };
+}🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/utils.ts`
around lines 23 - 26, stripClearScrollback currently does a stateless replaceAll
which fails when the ESC sequence "\x1b[3J" is split across chunks; change it to
a stateful stripper that preserves trailing partial sequences and removes
completed "\x1b[3J" only when fully seen. Implement this by turning
stripClearScrollback into a factory that returns a function/closure which keeps
a small carry-over buffer (tracking "", "\x1b", "\x1b[", "\x1b[3") between
calls, or update the stream hook to buffer joins before calling the existing
function; update all call sites to use the returned function (or the buffered
input) and ensure any leftover partial sequence is kept for the next invocation
to avoid passing incomplete ESC sequences to xterm.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary:
Testing: not run
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.