Skip to content

Revert "fix(desktop): strip ESC[3J to prevent terminal scrollback jump on reattach"#968

Merged
Kitenite merged 1 commit intomainfrom
revert-963-reimpl-fix
Jan 26, 2026
Merged

Revert "fix(desktop): strip ESC[3J to prevent terminal scrollback jump on reattach"#968
Kitenite merged 1 commit intomainfrom
revert-963-reimpl-fix

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Jan 26, 2026

Reverts #963

Summary by CodeRabbit

  • Refactor

    • Simplified terminal escape sequence handling by removing the strip clear scrollback utility function.
    • Reorganized escape sequence detection logic for improved code clarity and maintainability.
  • Documentation

    • Enhanced inline documentation for terminal escape sequence handling and clear command behavior.
  • Tests

    • Updated test documentation and removed obsolete test suite.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 26, 2026

📝 Walkthrough

Walkthrough

This PR removes the stripClearScrollbackSequence utility function and its usages throughout the codebase. It eliminates related tests, refactors clear-scrollback detection logic in the escape-filter module with enhanced documentation, and updates terminal restore logic to handle data without stripping escape sequences.

Changes

Cohort / File(s) Summary
Clear-scrollback detection refactoring
apps/desktop/src/main/lib/terminal-escape-filter.ts, apps/desktop/src/main/lib/terminal-escape-filter.test.ts
Reordered and clarified ED3 sequence detection logic; moved ED3_SEQUENCE constant and explicitly defined CLEAR_SCROLLBACK_PATTERN via RegExp. Expanded inline documentation explaining exclusion of ESC c (RIS) and ED3 vs. RIS handling. Added test case comment for clarity.
stripClearScrollbackSequence removal
apps/desktop/src/shared/terminal-escape.ts, apps/desktop/src/shared/terminal-escape.test.ts
Deleted stripClearScrollbackSequence(data: string): string function and internal constants (ESC, ED3_SEQUENCE). Removed entire test suite for the deleted function (61 lines of tests covering ED3 stripping, ANSI codes, and edge cases).
Terminal restore hook and utility updates
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalRestore.ts, apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/utils.ts
Removed calls to stripClearScrollbackSequence() in flushPendingEvents and maybeApplyInitialState, replacing xterm.write(stripClearScrollbackSequence(event.data)) with xterm.write(event.data). Removed re-export of stripClearScrollbackSequence from utils module; updated imports.
Session logic clarification
apps/desktop/src/main/lib/terminal/session.ts
Rewrote comment in data handling logic to clarify that headless recreation on clear is due to async writes from xterm. No functional behavior change.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐰 A function once stripped what the scrollback did clear,
But now we detect it—no stripping to fear!
The sequences flow where they're meant to stay,
And patterns explicit light up the way ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description is minimal and only states 'Reverts #963' without filling required template sections like Type of Change, Testing, or Additional Notes. Expand the description to include the Type of Change (Refactor), explain the rationale for reverting, and document any testing performed to verify the revert.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: reverting a previous commit that stripped ESC[3J sequences to prevent terminal scrollback jumps.

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

✨ Finishing touches
  • 📝 Generate docstrings

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.

@Kitenite Kitenite merged commit ce7dbab into main Jan 26, 2026
5 checks passed
@Kitenite Kitenite deleted the revert-963-reimpl-fix branch January 26, 2026 20:30
@github-actions
Copy link
Copy Markdown
Contributor

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ⚠️ Neon database branch
  • ⚠️ Electric Fly.io app

Thank you for your contribution! 🎉

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