fix(desktop): restore terminal scroll position when reattaching by Kitenite · Pull Request #698 · superset-sh/superset

Kitenite · 2026-01-09T23:53:26Z

Summary

Saves terminal viewport scroll position when detaching (e.g., switching tabs)
Restores scroll position when reattaching to preserve user's reading position
Uses xterm.write callback to ensure scroll restoration happens after content is rendered

Test plan

Open a terminal with enough content to scroll
Scroll up in the terminal
Switch to a different tab
Switch back to the terminal tab
Verify scroll position is preserved

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

New Features
- Terminal scroll position now persists across session switches, automatically restoring your viewport position when reattaching to a terminal.

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

coderabbitai · 2026-01-09T23:53:42Z

Warning

Rate limit exceeded

@Kitenite has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 21 minutes and 25 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 b4cfe85 and c49d943.

📒 Files selected for processing (4)

apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
apps/desktop/src/main/lib/terminal/manager.ts
apps/desktop/src/main/lib/terminal/types.ts
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx

📝 Walkthrough

Walkthrough

This PR introduces viewport scroll position preservation to terminal sessions. It adds an optional viewportY field to session data structures, captures scroll positions during detach operations, and restores them on reattach, enabling scroll state persistence across terminal session cycles.

Changes

Cohort / File(s)	Summary
Terminal TRPC Router `apps/desktop/src/lib/trpc/routers/terminal/terminal.ts`	Exposed `viewportY` in createOrAttach mutation return payload; extended detach input schema with optional `viewportY` parameter
Terminal Manager Core Logic `apps/desktop/src/main/lib/terminal/manager.ts`	Implemented viewport persistence: createOrAttach returns existing session's `viewportY`; detach accepts and stores optional `viewportY` on session alongside lastActive timestamp
Terminal Type Definitions `apps/desktop/src/main/lib/terminal/types.ts`	Added optional `viewportY?: number` property to TerminalSession and SessionResult interfaces for scroll position storage
Terminal UI Component `apps/desktop/src/renderer/.../Terminal/Terminal.tsx`	Integrated scroll restoration on reattach via xterm API using provided `viewportY`; captures current scroll position on detach and passes it to mutation

Sequence Diagram

sequenceDiagram
    participant UI as Terminal UI
    participant Router as TRPC Router
    participant Mgr as TerminalManager
    participant XTerm as XTerm Instance

    rect rgb(200, 220, 240)
    Note over UI,XTerm: Reattach Flow - Restore Scroll Position
    UI->>Router: createOrAttach({ paneId, ... })
    Router->>Mgr: createOrAttach(...)
    Mgr->>Mgr: Reuse existing session<br/>Extract viewportY
    Mgr-->>Router: Return SessionResult<br/>{ ..., viewportY }
    Router-->>UI: Return { ..., viewportY }
    UI->>UI: applyInitialState({ ..., viewportY })
    UI->>XTerm: write(scrollback)
    UI->>XTerm: scrollToLine(viewportY)
    end

    rect rgb(240, 200, 220)
    Note over UI,XTerm: Detach Flow - Capture Scroll Position
    UI->>XTerm: Get currentViewportY
    UI->>Router: detach({ paneId, viewportY })
    Router->>Mgr: detach({ paneId, viewportY })
    Mgr->>Mgr: session.viewportY = viewportY<br/>session.lastActive = now()
    Mgr-->>UI: Complete
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

xterm ui #115: Directly modifies the terminal router and UI component paths being extended by this PR's scroll restoration feature.
Fix setup script appearing 3 times when starting a worktree #222: Alters the Terminal component's attach/reattach flow and applyInitialState callback logic that this PR extends for scroll restoration.
feat(desktop): use headless xterm for terminal scrollback storage #686: Modifies terminal session API and TerminalManager behavior including session data model and create/attach/detach flows affected by viewportY additions.

Poem

🐰 A hop through the scrollback we go,
Remembering where we left our view below,
With viewportY saved like a clover so sweet,
Each reattach brings us back to our seat! 🌿

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: restoring terminal scroll position when reattaching, which aligns with the core functionality across all modified files.
Description check	✅ Passed	The description includes a summary of changes and a concrete test plan, but lacks sections for 'Related Issues', 'Type of Change', and 'Testing' as specified in the template.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ 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.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

apps/desktop/src/main/lib/terminal/manager.ts (1)
242-256: Consider clearing viewportY when scrollback is cleared.

The detach implementation correctly stores the viewport position. However, the clearScrollback method (line 258-278) recreates the headless terminal without resetting session.viewportY. This could result in an attempt to restore an invalid scroll position after the buffer is cleared.
♻️ Suggested fix to clear viewportY in clearScrollback
 clearScrollback(params: { paneId: string }): void {
   const { paneId } = params;
   const session = this.sessions.get(paneId);

   if (!session) {
     console.warn(
       `Cannot clear scrollback for terminal ${paneId}: session not found`,
     );
     return;
   }

   // Recreate headless (xterm writes are async, so clear() alone is unreliable)
   session.headless.dispose();
   const { headless, serializer } = createHeadlessTerminal({
     cols: session.cols,
     rows: session.rows,
   });
   session.headless = headless;
   session.serializer = serializer;
+  // Clear saved scroll position since buffer was reset
+  session.viewportY = undefined;
   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 2654b6a and b4cfe85.

📒 Files selected for processing (4)

apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
apps/desktop/src/main/lib/terminal/manager.ts
apps/desktop/src/main/lib/terminal/types.ts
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx

🧰 Additional context used

📓 Path-based instructions (6)

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/lib/trpc/routers/terminal/terminal.ts
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/main/lib/terminal/manager.ts
apps/desktop/src/main/lib/terminal/types.ts

**/*.{ts,tsx}