Skip to content

feat(desktop): use headless xterm for terminal scrollback storage#686

Merged
Kitenite merged 6 commits intomainfrom
feat/headless-xterm-scrollback
Jan 9, 2026
Merged

feat(desktop): use headless xterm for terminal scrollback storage#686
Kitenite merged 6 commits intomainfrom
feat/headless-xterm-scrollback

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Jan 9, 2026

Summary

  • Replace raw PTY data storage with headless xterm + serialize addon for cleaner scrollback
  • Properly processes escape sequences instead of storing them verbatim, eliminating "unpleasant" raw escape sequence artifacts
  • Uses @xterm/headless (official Node.js version of xterm.js) with @xterm/addon-serialize to produce clean ANSI output

Changes

  • Add @xterm/headless@5.5.0 dependency (matches @xterm/xterm@5.5.0)
  • Replace scrollback: string with headless: HeadlessTerminal + serializer: SerializeAddon in session type
  • Feed PTY data to headless terminal for proper escape sequence processing
  • Use serializer.serialize() to get clean scrollback output
  • Recreate headless terminal on ED3 (clear scrollback) sequences to handle xterm's async write queue
  • Keep headless terminal dimensions in sync on resize
  • Update tests for new architecture

How it works

PTY Output → Data Handler → Headless xterm → Serialize Addon → Clean Output
                              (processes escape sequences)

The serialize addon outputs minimal ANSI sequences that reconstruct the terminal state, rather than storing raw escape-sequence-laden PTY data.

Test plan

  • All terminal tests pass (153/153)
  • Typecheck passes
  • App builds successfully
  • Manual test: verify scrollback is restored correctly on terminal reattach
  • Manual test: verify clear command (Cmd+K) properly clears scrollback

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests

    • More robust terminal tests with async waits and headless-backed serialization to validate scrollback recovery and disposal.
  • Chores

    • Added a headless xterm serializer dependency to improve escape-sequence handling and serialized scrollback.
  • Refactor

    • Reworked terminal session lifecycle to use serialized headless output, handle clear-scrollback reliably, and ensure accurate recovery and cleanup.

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

Replace raw PTY data storage with headless xterm + serialize addon for
cleaner scrollback. This properly processes escape sequences instead of
storing them verbatim.

Changes:
- Add @xterm/headless dependency for Node.js terminal emulation
- Store terminal state in HeadlessTerminal instead of raw string
- Use SerializeAddon to produce clean ANSI output for scrollback
- Recreate headless terminal on ED3 (clear) sequences to handle async
- Keep headless terminal dimensions in sync on resize
- Update tests for new architecture

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 9, 2026

Warning

Rate limit exceeded

@Kitenite has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 13 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between a5a188c and 43ce171.

📒 Files selected for processing (3)
  • apps/desktop/src/main/lib/terminal/manager.ts
  • apps/desktop/src/main/lib/terminal/session.ts
  • apps/desktop/src/main/lib/terminal/types.ts
📝 Walkthrough

Walkthrough

Refactors terminal scrollback to use an xterm headless terminal with the Serialize addon. Sessions now carry headless and serializer instead of a raw scrollback string; manager and session flows write to, serialize, resize, clear, and dispose headless terminals. Tests and a runtime dependency were added.

Changes

Cohort / File(s) Summary
Dependency
apps/desktop/package.json
Added runtime dependency @xterm/headless ^5.5.0.
Types
apps/desktop/src/main/lib/terminal/types.ts
Removed scrollback: string; added headless: HeadlessTerminal and serializer: SerializeAddon to TerminalSession.
Session core
apps/desktop/src/main/lib/terminal/session.ts
Added createHeadlessTerminal; session state now holds headless + serializer; added getSerializedScrollback; changed recoverScrollback signature/behavior; routing of writes, clear-scrollback detection (recreate headless), resize and flushSession disposal updated to manage headless instance.
Manager integration
apps/desktop/src/main/lib/terminal/manager.ts
Replaced direct session.scrollback usage with getSerializedScrollback(session); use serialized scrollback when attaching/creating; ensure session.headless is resized and recreated/cleared appropriately.
Tests — session
apps/desktop/src/main/lib/terminal/session.test.ts
Added headless/Serialize test helpers; updated tests to write to headless, await async processing, assert serialized output, and verify headless disposal and recovery boolean behavior.
Tests — manager
apps/desktop/src/main/lib/terminal/manager.test.ts
Added short (50ms) async delays after writes in tests to allow headless async processing before assertions.

Sequence Diagram(s)

sequenceDiagram
  participant Manager
  participant Session
  participant Headless
  participant Serializer
  Manager->>Session: createSession(cols, rows, existingScrollback?)
  Session->>Headless: createHeadlessTerminal(cols, rows, scrollback?)
  Headless->>Serializer: attach SerializeAddon
  alt existingScrollback present
    Manager->>Session: recoverScrollback({ existingScrollback, headless })
    Session->>Headless: write(existingScrollback)
    Headless->>Serializer: serialize()
    Serializer-->>Session: serializedOutput
    Session-->>Manager: wasRecovered = true
  end
  Manager->>Session: getSerializedScrollback(session)
  Session->>Serializer: serialize()
  Serializer-->>Manager: stringScrollback
  Manager->>Session: resize(cols, rows)
  Session->>Headless: resize(cols, rows)
  Manager->>Session: clearScrollback()
  Session->>Headless: clear() / recreate headless
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Poem

🐰 I hopped into bytes where terminals play,
Headless and tidy, escape codes kept at bay.
I write, then I wait for serialized light,
Tests nod and pass — scrollback restored just right.
A tiny rabbit cheer for the code's new day. 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: replacing PTY data storage with headless xterm for terminal scrollback, matching the primary objective of the PR.
Description check ✅ Passed The description includes a comprehensive summary, detailed changes, implementation explanation with a diagram, and test plan. All required template sections are covered with substantive content.

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


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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @apps/desktop/package.json:
- Around line 67-68: The two @xterm dependencies use inconsistent version
ranges; update the package.json entries for "@xterm/headless" and "@xterm/xterm"
so they use the same version strategy (e.g., both pinned to "5.5.0" or both
caret-prefixed "^5.5.0"), then run your package install to update the lockfile
and run the app/tests to verify no compatibility breakage.

In @apps/desktop/src/main/lib/terminal/session.ts:
- Around line 186-197: This block duplicates headless terminal setup; replace it
by calling the existing createHeadlessTerminal helper and reuse its returned
terminal/serializer instead of manually new-ing HeadlessTerminal and
SerializeAddon. Specifically, call createHeadlessTerminal with the session's
cols/rows (or the helper's expected args), assign its returned headless instance
to session.headless and the returned serializer to session.serializer (or if the
helper only returns the terminal, ensure it loads and exposes the SerializeAddon
so you still set session.serializer). Remove the manual new HeadlessTerminal /
new SerializeAddon / loadAddon code and use createHeadlessTerminal to keep
configuration consistent.
🧹 Nitpick comments (3)
apps/desktop/src/main/lib/terminal/manager.test.ts (2)

580-582: Consider a more deterministic synchronization approach.

The 50ms delay is used to wait for the headless terminal's async write queue. While functional, fixed delays can be flaky on slow systems and wasteful on fast ones.

Explore whether xterm's headless terminal provides a write completion callback or drain event for more reliable synchronization:

// Current approach:
await new Promise((resolve) => setTimeout(resolve, 50));

// Potential alternative (if available):
await new Promise<void>((resolve) => {
  session.headless.write("", resolve);
});

If such an API exists, it would make tests more reliable and potentially faster.


610-612: Duplicated async delay pattern.

This is the same 50ms delay pattern from line 580-582. Consider extracting to a test helper to reduce duplication and make future adjustments easier.

🔄 Extract delay helper
// At the top of the test file
async function waitForHeadlessWrite() {
  await new Promise((resolve) => setTimeout(resolve, 50));
}

// Then use it:
await waitForHeadlessWrite();
apps/desktop/src/main/lib/terminal/session.ts (1)

90-93: Consider including onData callback in the params object.

Per coding guidelines, functions with 2+ parameters should accept a single params object. The callback could be included as a named property for consistency.

♻️ Suggested signature refactor
-export async function createSession(
-	params: InternalCreateSessionParams,
-	onData: (paneId: string, data: string) => void,
-): Promise<TerminalSession> {
+export async function createSession(
+	params: InternalCreateSessionParams & {
+		onData: (paneId: string, data: string) => void;
+	},
+): Promise<TerminalSession> {
+	const { onData, ...sessionParams } = params;
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 78a5d8a and e7dccf5.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • apps/desktop/package.json
  • apps/desktop/src/main/lib/terminal/manager.test.ts
  • apps/desktop/src/main/lib/terminal/manager.ts
  • apps/desktop/src/main/lib/terminal/session.test.ts
  • apps/desktop/src/main/lib/terminal/session.ts
  • apps/desktop/src/main/lib/terminal/types.ts
🧰 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/terminal/manager.test.ts
  • apps/desktop/src/main/lib/terminal/session.test.ts
  • apps/desktop/src/main/lib/terminal/types.ts
  • apps/desktop/src/main/lib/terminal/manager.ts
  • apps/desktop/src/main/lib/terminal/session.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid using any type - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness

Files:

  • apps/desktop/src/main/lib/terminal/manager.test.ts
  • apps/desktop/src/main/lib/terminal/session.test.ts
  • apps/desktop/src/main/lib/terminal/types.ts
  • apps/desktop/src/main/lib/terminal/manager.ts
  • apps/desktop/src/main/lib/terminal/session.ts
**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Co-locate tests with implementation files using .test.ts or .test.tsx suffix

Files:

  • apps/desktop/src/main/lib/terminal/manager.test.ts
  • apps/desktop/src/main/lib/terminal/session.test.ts
apps/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Drizzle ORM for all database operations - never use raw SQL

Files:

  • apps/desktop/src/main/lib/terminal/manager.test.ts
  • apps/desktop/src/main/lib/terminal/session.test.ts
  • apps/desktop/src/main/lib/terminal/types.ts
  • apps/desktop/src/main/lib/terminal/manager.ts
  • apps/desktop/src/main/lib/terminal/session.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Biome for formatting and linting - run at root level with bun run lint:fix or biome check --write

Files:

  • apps/desktop/src/main/lib/terminal/manager.test.ts
  • apps/desktop/src/main/lib/terminal/session.test.ts
  • apps/desktop/src/main/lib/terminal/types.ts
  • apps/desktop/src/main/lib/terminal/manager.ts
  • apps/desktop/src/main/lib/terminal/session.ts
🧠 Learnings (1)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility

Applied to files:

  • apps/desktop/src/main/lib/terminal/session.ts
🧬 Code graph analysis (2)
apps/desktop/src/main/lib/terminal/session.test.ts (2)
apps/desktop/src/main/lib/terminal/session.ts (3)
  • recoverScrollback (59-69)
  • getSerializedScrollback (52-54)
  • flushSession (235-238)
apps/desktop/src/main/lib/terminal/types.ts (1)
  • TerminalSession (6-24)
apps/desktop/src/main/lib/terminal/manager.ts (1)
apps/desktop/src/main/lib/terminal/session.ts (1)
  • getSerializedScrollback (52-54)
⏰ 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). (6)
  • GitHub Check: Deploy API
  • GitHub Check: Deploy Docs
  • GitHub Check: Deploy Marketing
  • GitHub Check: Deploy Admin
  • GitHub Check: Deploy Web
  • GitHub Check: Build
🔇 Additional comments (17)
apps/desktop/src/main/lib/terminal/types.ts (2)

1-2: LGTM - Type imports align with new architecture.

The imports for SerializeAddon and HeadlessTerminal are correctly sourced from the respective packages.


14-17: Good architectural separation between internal state and external API.

Replacing the raw scrollback: string with headless: HeadlessTerminal and serializer: SerializeAddon enables proper escape sequence processing. The external API (SessionResult.scrollback: string on line 41) remains stable as a computed value, which is excellent design.

apps/desktop/src/main/lib/terminal/manager.ts (4)

6-11: LGTM - Proper import of serialization helper.

The import of getSerializedScrollback centralizes the scrollback serialization logic, promoting consistency across the codebase.


41-41: Excellent consistency in scrollback serialization.

All usages of session.scrollback have been systematically replaced with getSerializedScrollback(session). This ensures:

  • Consistent scrollback representation across all code paths
  • Proper escape sequence processing via the headless terminal
  • Single source of truth for scrollback generation

The changes cover:

  • Existing session reattachment (line 41)
  • New session creation with existing scrollback (line 49)
  • New session return (line 100)
  • Fallback shell recovery (line 130)

Also applies to: 49-49, 100-100, 130-130


194-195: Critical: Headless terminal dimensions synchronized on resize.

Keeping session.headless.resize(cols, rows) in sync with the PTY resize ensures the headless terminal accurately reflects the terminal state for proper scrollback serialization. This is essential for correctness.


261-262: LGTM - Clear scrollback now properly handled.

The change from string manipulation to session.headless.clear() aligns with the new architecture and ensures the headless terminal's buffer is properly cleared.

apps/desktop/src/main/lib/terminal/session.test.ts (5)

11-26: Good test helper for consistent test setup.

The createTestHeadless() helper promotes DRY principles and ensures consistent configuration across tests. The headless terminal is properly initialized with reasonable defaults (80x24, 1000 scrollback).


30-50: Excellent async handling in tests.

The test properly waits for xterm's async write to complete using the callback pattern before asserting on serialized content. This is more reliable than fixed delays.


52-63: Good negative test case coverage.

Testing the null/empty scrollback path ensures recoverScrollback handles both scenarios correctly.


88-113: Comprehensive resource cleanup verification.

The test verifies both dataBatcher.dispose() and headless.dispose() are called, ensuring no resource leaks. This is critical given that headless terminals may hold significant memory.


23-23: This type assertion is a documented workaround for xterm's type definitions. The @xterm/addon-serialize@0.13.0 is the correct compatible version for @xterm/headless@5.5.0, and the double cast is the standard approach used across xterm projects to load addons. No changes needed.

Likely an incorrect or invalid review comment.

apps/desktop/src/main/lib/terminal/session.ts (6)

1-19: LGTM!

Imports are correctly organized. Constants are well-named with appropriate documentation for DEFAULT_SCROLLBACK.


21-47: LGTM!

Well-documented helper with clear justification for the type cast workaround. The object params pattern follows coding guidelines.


49-69: LGTM!

Clean helper functions. The recoverScrollback correctly uses object parameters per coding guidelines and returns a meaningful boolean for the caller to track recovery state.


71-88: LGTM!

Standard PTY spawn configuration with proper object parameter usage.


198-206: LGTM!

The content-after-clear handling is correct. Writing only the post-clear content ensures the fresh headless terminal starts with the right state.


235-238: LGTM!

Proper resource cleanup disposing both the data batcher and headless terminal.

Comment thread apps/desktop/package.json Outdated
Comment thread apps/desktop/src/main/lib/terminal/session.ts Outdated
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 9, 2026

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ✅ Neon database branch
  • ✅ Electric Fly.io app

Thank you for your contribution! 🎉

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
apps/desktop/src/main/lib/terminal/manager.ts (2)

196-208: Handle partial resize failure to prevent state inconsistency.

If session.headless.resize() throws but session.pty.resize() succeeds, the catch block will log the error but leave the terminals out of sync. Additionally, session.cols and session.rows won't be updated, creating a mismatch between the tracked dimensions and the actual PTY size.

Consider restructuring to handle this scenario:

🔧 Proposed fix to handle partial failure
 	try {
 		session.pty.resize(cols, rows);
+		session.cols = cols;
+		session.rows = rows;
+		
 		// Keep headless terminal in sync for accurate scrollback serialization
-		session.headless.resize(cols, rows);
-		session.cols = cols;
-		session.rows = rows;
+		try {
+			session.headless.resize(cols, rows);
+		} catch (headlessError) {
+			console.error(
+				`[TerminalManager] Failed to resize headless terminal ${paneId} (cols=${cols}, rows=${rows}):`,
+				headlessError,
+			);
+		}
 		session.lastActive = Date.now();
 	} catch (error) {
 		console.error(
 			`[TerminalManager] Failed to resize terminal ${paneId} (cols=${cols}, rows=${rows}):`,
 			error,
 		);
 	}

This ensures:

  • PTY dimensions are tracked even if headless resize fails
  • Headless resize errors are logged separately for better diagnostics
  • Main PTY resize errors still propagate to the outer catch block

253-267: Add error handling for headless terminal clear operation.

The session.headless.clear() call lacks error handling, unlike the similar session.headless.resize() operation in the resize() method. If the clear operation throws, it could break the user-facing "clear scrollback" action (Cmd+K) without any diagnostic logging.

🛡️ Proposed fix to add error handling
 	// Clear the headless terminal's scrollback buffer
-	session.headless.clear();
+	try {
+		session.headless.clear();
+	} catch (error) {
+		console.error(
+			`[TerminalManager] Failed to clear scrollback for terminal ${paneId}:`,
+			error,
+		);
+	}
 	session.lastActive = Date.now();
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e7dccf5 and 14ab7eb.

📒 Files selected for processing (1)
  • apps/desktop/src/main/lib/terminal/manager.ts
🧰 Additional context used
📓 Path-based instructions (4)
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/terminal/manager.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid using any type - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness

Files:

  • apps/desktop/src/main/lib/terminal/manager.ts
apps/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Drizzle ORM for all database operations - never use raw SQL

Files:

  • apps/desktop/src/main/lib/terminal/manager.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Biome for formatting and linting - run at root level with bun run lint:fix or biome check --write

Files:

  • apps/desktop/src/main/lib/terminal/manager.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/terminal/manager.ts (1)
apps/desktop/src/main/lib/terminal/session.ts (1)
  • getSerializedScrollback (52-54)
⏰ 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). (6)
  • GitHub Check: Deploy Marketing
  • GitHub Check: Deploy Web
  • GitHub Check: Deploy Admin
  • GitHub Check: Deploy Docs
  • GitHub Check: Deploy API
  • GitHub Check: Build

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
apps/desktop/src/main/lib/terminal/session.ts (1)

56-69: Potential async timing issue with scrollback recovery.

headless.write() queues data asynchronously. If getSerializedScrollback() is called immediately after recoverScrollback(), the write may not have completed, resulting in incomplete scrollback serialization.

Consider whether a small delay or synchronization is needed when recovering scrollback, similar to what the tests do. The PR summary mentions tests use "small asynchronous delays to accommodate headless processing."

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e9ab70 and a5a188c.

📒 Files selected for processing (2)
  • apps/desktop/src/main/lib/terminal/manager.ts
  • apps/desktop/src/main/lib/terminal/session.ts
🧰 Additional context used
📓 Path-based instructions (4)
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/terminal/manager.ts
  • apps/desktop/src/main/lib/terminal/session.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid using any type - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness

Files:

  • apps/desktop/src/main/lib/terminal/manager.ts
  • apps/desktop/src/main/lib/terminal/session.ts
apps/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Drizzle ORM for all database operations - never use raw SQL

Files:

  • apps/desktop/src/main/lib/terminal/manager.ts
  • apps/desktop/src/main/lib/terminal/session.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Biome for formatting and linting - run at root level with bun run lint:fix or biome check --write

Files:

  • apps/desktop/src/main/lib/terminal/manager.ts
  • apps/desktop/src/main/lib/terminal/session.ts
🧠 Learnings (1)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility

Applied to files:

  • apps/desktop/src/main/lib/terminal/session.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/terminal/session.ts (2)
apps/desktop/src/main/lib/terminal/types.ts (1)
  • TerminalSession (6-24)
apps/desktop/src/main/lib/terminal-escape-filter.ts (2)
  • containsClearScrollbackSequence (29-31)
  • extractContentAfterClear (38-46)
⏰ 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). (7)
  • GitHub Check: Deploy Marketing
  • GitHub Check: Deploy Docs
  • GitHub Check: Deploy Admin
  • GitHub Check: Deploy Web
  • GitHub Check: Deploy API
  • GitHub Check: Build
  • GitHub Check: Typecheck
🔇 Additional comments (10)
apps/desktop/src/main/lib/terminal/manager.ts (4)

6-12: LGTM!

The imports are well-organized and correctly bring in the new headless terminal utilities from the session module.


114-117: Good defensive ordering.

Capturing scrollback before flushSession is correct since flushSession disposes the headless terminal. The comment clearly explains the intent.


196-209: LGTM!

Keeping the headless terminal dimensions synchronized ensures accurate scrollback serialization. The error handling properly wraps both resize operations.


254-275: LGTM!

Recreating the headless terminal is the correct approach given xterm's asynchronous write queue. This ensures a clean slate for scrollback.

apps/desktop/src/main/lib/terminal/session.ts (6)

49-54: LGTM!

Clean abstraction that encapsulates the serialization logic.


123-163: LGTM!

Session creation correctly initializes the headless terminal before spawning the PTY, and properly stores both headless and serializer in the session object.


178-200: LGTM!

The ED3 clear sequence handling is well-implemented:

  • Properly disposes the old terminal before creating a new one
  • Extracts and preserves any content after the clear sequence
  • The conditional write on line 194 is correct (empty string would be a no-op)

229-232: LGTM!

Proper cleanup of both the data batcher and headless terminal resources.


14-19: Good constant extraction.

The DEFAULT_SCROLLBACK constant follows the coding guideline to extract magic numbers. 10,000 lines is a reasonable default for terminal scrollback.


21-47: LGTM — type cast is justified, and allowProposedApi: true is required.

The type assertion on lines 40-44 is correct. SerializeAddon is an experimental/proposed API in xterm.js that requires allowProposedApi: true to function; it cannot be instantiated otherwise. The comment correctly explains that SerializeAddon only accesses the buffer API, which both browser and headless terminals share at runtime. The code also properly disposes the headless terminal in flushSession() and handles terminal recreation when clear scrollback sequences are detected.

@Kitenite Kitenite merged commit c0e24f8 into main Jan 9, 2026
13 checks passed
@Kitenite Kitenite deleted the feat/headless-xterm-scrollback branch January 9, 2026 02:46
saddlepaddle pushed a commit that referenced this pull request Jan 10, 2026
* feat(desktop): use headless xterm for terminal scrollback storage

Replace raw PTY data storage with headless xterm + serialize addon for
cleaner scrollback. This properly processes escape sequences instead of
storing them verbatim.

Changes:
- Add @xterm/headless dependency for Node.js terminal emulation
- Store terminal state in HeadlessTerminal instead of raw string
- Use SerializeAddon to produce clean ANSI output for scrollback
- Recreate headless terminal on ED3 (clear) sequences to handle async
- Keep headless terminal dimensions in sync on resize
- Update tests for new architecture
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