fix(desktop): filter terminal control sequence leakage

[Kitenite]

Summary

• Fixes garbage text like 1R1R1R and [I/[O appearing when switching between terminal instances
• Adds parser hooks to suppress focus in/out sequences (ESC[I, ESC[O)
• Creates filterOrphanedSequences filter for sequences split across data chunks that arrive without their ESC prefix
• All changes are display-layer only - no terminal behavior modification

Test plan

• Open multiple terminals in different worktrees
• Run programs that enable focus reporting (vim/neovim, starship prompt)
• Switch rapidly between terminals
• Verify no 1R, [I, [O garbage appears
• Exit programs, verify next shell works normally

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed potential data leakage between terminal instances and improved cleanup handling during component unmounting.
  • Enhanced ANSI escape sequence handling to prevent sequences from being split across data batches.
  • Added suppression for terminal Focus In/Out control sequences.

• Improvements

  • Improved terminal stability through lifecycle guards and subscription state management.

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

[coderabbitai]

📝 Walkthrough

Walkthrough

These changes enhance terminal lifecycle management and escape sequence handling to prevent stale closures, cross-terminal data leakage, and broken ANSI sequences across batch boundaries. Three files are modified: the Terminal component gains lifecycle guards and cleanup logic, CSI suppression expands to cover focus events, and the data batcher detects and preserves complete escape sequences during flush operations.

Changes

Cohort / File(s)	Summary
Terminal lifecycle & cleanup apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx	Adds isUnmountedRef and subscriptionEnabledRef to prevent stale closure capture; synchronizes ref with state; guards handleStreamData with unmount check; resets refs on mount/remount; clears pendingEventsRef and sets unmounted flag during termination to prevent cross-terminal data leakage.
CSI suppression expansion apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts	Registers new CSI handlers for Focus In (I) and Focus Out (O) sequences to suppress display output; handlers return true to mark responses as hidden.
Escape sequence batching apps/desktop/src/main/lib/data-batcher.ts	Introduces findSafeFlushPoint() and isCompleteSequence() helpers, plus ESC, BEL, and MAX_ESCAPE_SCAN constants; adds pending member to hold incomplete escape sequences; enhances write() to prepend pending data and flush() to detect safe flush boundaries; disposes remaining pending sequences on cleanup.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• implement more font and xterm rendering improvements #324: Modifies data-batcher.ts escape and ANSI sequence handling with changes to how terminal sequences are preserved across batches.
• fix(desktop): suppress terminal query responses using xterm.js parser… #196: Implements CSI suppression via xterm.js parser hooks, directly overlapping with the suppressQueryResponses enhancements for query-response filtering.
• persist process history #120: Modifies Terminal.tsx subscription and pending-event lifecycle management, including queuing and flushing behavior tied to component teardown.

Poem

🐰 A terminal's tale, once told awry,
When unmounted refs would break and sigh—
Now lifecycle guards keep escapes whole,
Pending events cleared, data made whole!
Focus whispers suppressed with care, 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix(desktop): filter terminal control sequence leakage' clearly and specifically describes the main change: filtering control sequence leakage in the desktop terminal component.
Description check	✅ Passed	The description provides a clear summary of changes, test plan, and context, though it omits some template sections like 'Related Issues', 'Type of Change', and 'Additional Notes' which are typically useful for comprehensive documentation.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch control-sequence-again

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a26f7a and 181bee4.

📒 Files selected for processing (2)

• apps/desktop/src/main/lib/data-batcher.ts
• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx

🧰 Additional context used

📓 Path-based instructions (5)

apps/desktop/**/*.{ts,tsx}

📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)

apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined in src/lib/trpc
Use alias as defined in tsconfig.json when possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from @trpc/server/observable instead of async generators, as the library explicitly checks isObservable(result) and throws an error otherwise

Files:

• apps/desktop/src/main/lib/data-batcher.ts
• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use type safety and avoid any types unless absolutely necessary in TypeScript files

Files:

• apps/desktop/src/main/lib/data-batcher.ts
• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx

apps/desktop/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Move Node.js functionality needed in Electron renderer to src/main/lib/ and communicate via IPC

Files:

• apps/desktop/src/main/lib/data-batcher.ts
• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx

apps/**/src/**/**/[A-Z]*.tsx

📄 CodeRabbit inference engine (AGENTS.md)

Structure component folders with one component per file using format ComponentName/ComponentName.tsx with index.ts barrel export

Files:

• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx

apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Define all Electron IPC channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers

Files:

• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx

🧠 Learnings (1)

📚 Learning: 2025-12-28T01:56:39.021Z

Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.021Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC

Applied to files:

• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build

🔇 Additional comments (13)

apps/desktop/src/main/lib/data-batcher.ts (7)

16-20: LGTM! Constants are appropriate for escape sequence handling.

The 128-byte scan window is a reasonable performance trade-off. Note that escape sequences longer than 128 bytes might not be detected as incomplete and could be split, but such sequences are rare in practice (typical sequences are <50 bytes).

---

22-45: LGTM! The split-point detection logic is sound.

The function correctly handles three cases:

1. No incomplete sequences (-1: flush everything)
2. Incomplete sequence at end (>0: flush up to split point)
3. Entire buffer is incomplete (0: hold everything)

The backwards scanning approach is efficient and the logic is clear.

---

47-92: Escape sequence detection is well-implemented.

The function correctly handles the major escape sequence types:

• CSI sequences (ESC [) with proper final byte detection
• OSC sequences (ESC ]) with BEL/ST terminator checks
• DCS sequences (ESC P) with ST terminator
• Simple two-byte Fe sequences

The safety valve at line 91 (returning true for unknown sequence types) prevents indefinite buffering, which is a reasonable trade-off. This means unknown or malformed sequences will be flushed, which is safer than risking unbounded memory growth.

---

103-103: LGTM! The pending buffer enables sequences to span flush cycles.

This member correctly holds incomplete escape sequences between flushes, ensuring they're not lost or corrupted.

---

120-123: LGTM! Pending sequence handling in write() is correct.

The logic properly prepends any held-back incomplete sequences to the new data, ensuring escape sequences split across write calls remain intact.

---

151-169: LGTM! The flush() logic correctly handles incomplete sequences.

The three-way branching based on findSafeFlushPoint properly manages:

1. Complete buffer (flush all, clear pending)
2. Partial incomplete sequence (flush safe portion, hold remainder)
3. Entirely incomplete buffer (hold all for next write)

This prevents escape sequences from being split across IPC boundaries.

---

179-183: LGTM! Disposal correctly flushes pending sequences.

The dispose logic ensures that even incomplete escape sequences are flushed when the batcher is torn down, preventing data loss while maintaining the guarantee that sequences aren't split during normal operation.

apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (6)

41-43: LGTM! Refs correctly prevent stale closure issues.

The addition of isUnmountedRef and subscriptionEnabledRef is a solid pattern to avoid race conditions where async callbacks capture stale component state. This prevents writing to disposed terminal instances and ensures subscription status checks are always current.

---

46-47: Ref synchronization pattern is correct.

Keeping subscriptionEnabledRef.current in sync with the state during render is a well-established pattern for avoiding stale closures in callbacks. While technically a side effect during render, this is safe because it's idempotent and doesn't trigger re-renders.

Note: React 19's strict mode and compiler should handle this correctly, as ref mutations during render are explicitly supported for this use case.

---

142-143: LGTM! Unmount guard prevents writing to disposed instances.

This early return correctly prevents the subscription callback from attempting to write to a disposed xterm instance, which would cause errors. The guard placement before any xterm operations is appropriate.

---

145-147: LGTM! Using ref prevents stale closure of subscription state.

Checking subscriptionEnabledRef.current instead of the subscriptionEnabled state variable ensures the callback always sees the current subscription status, avoiding bugs where the callback captures an outdated value from when it was created.

---

203-204: LGTM! Unmount flag reset ensures correct behavior on remount.

Resetting isUnmountedRef.current = false at the beginning of the effect ensures that if the component remounts (e.g., during hot reload or tab switching), the flag correctly reflects the mounted state.

---

398-402: LGTM! Cleanup logic prevents cross-terminal data leakage.

The cleanup sequence is correct:

1. Mark as unmounted (both local flag and ref) to stop in-flight operations
2. Clear pending events to prevent queued data from one terminal being applied to another

This effectively prevents the "cross-terminal contamination" mentioned in the PR objectives, where switching between terminals could cause stray data to appear.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/filterOrphanedSequences.ts (1)

39-49: LGTM! Solid iterative approach with safety guard.

The function correctly handles multiple consecutive orphaned sequences with an early exit when no more are found. The 100-iteration limit is a good safety measure to prevent infinite loops from potential regex bugs.

Consider adding JSDoc comments for better API documentation:

Optional: Add JSDoc for exported function

+/**
+ * Filters orphaned terminal control sequence fragments from a data string.
+ * Iteratively removes sequence fragments that appear at the start of the string
+ * without their ESC prefix (which arrived in a previous data chunk).
+ *
+ * @param data - The terminal data string to filter
+ * @returns The filtered string with orphaned sequences removed
+ * @example
+ * filterOrphanedSequences("1R[IHello") // returns "Hello"
+ * filterOrphanedSequences("Hello World") // returns "Hello World"
+ */
 export function filterOrphanedSequences(data: string): string {

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d40ef4f and 1a26f7a.

📒 Files selected for processing (2)

• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/filterOrphanedSequences.test.ts
• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/filterOrphanedSequences.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/filterOrphanedSequences.test.ts

🧰 Additional context used

📓 Path-based instructions (3)

apps/desktop/**/*.{ts,tsx}

📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)

apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined in src/lib/trpc
Use alias as defined in tsconfig.json when possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from @trpc/server/observable instead of async generators, as the library explicitly checks isObservable(result) and throws an error otherwise

Files:

• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/filterOrphanedSequences.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use type safety and avoid any types unless absolutely necessary in TypeScript files

Files:

• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/filterOrphanedSequences.ts

apps/desktop/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Move Node.js functionality needed in Electron renderer to src/main/lib/ and communicate via IPC

Files:

• apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/filterOrphanedSequences.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build

🔇 Additional comments (3)

apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/filterOrphanedSequences.ts (3)

1-23: LGTM! Clear documentation and smart optimization.

The file documentation clearly explains the problem of orphaned sequence fragments, and the ORPHAN_START_CHARS Set provides an efficient fast-path check before running the more expensive regex.

---

30-37: LGTM! Efficient implementation with proper edge case handling.

The helper function correctly implements the fast-path optimization and returns the appropriate length for matched orphaned sequences or zero when no match is found.

---

25-28: Remove case-insensitive flag from regex pattern — terminal sequences are case-sensitive.

The /i flag on line 63 is incorrect. Terminal control sequences have case-sensitive final bytes: ESC[I (FocusIn) is distinct from ESC[i, and lowercase variants are not valid orphaned sequences. The flag can cause false positives by matching lowercase text that isn't a terminal sequence.

Remove the "i" flag argument to the RegExp constructor on line 63.

The patterns for ;\d{1,4}R (orphaned CPR when stream splits) and 4;...rgb:... (OSC 4 color palette) are correct based on actual terminal output.