Repository navigation
feat(desktop): wire v2 terminal to user theme/font settings + addon parity - #3155
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a shared terminal appearance module and hook, introduces an addon loader, refactors runtime creation to accept and update appearance, updates the runtime registry and runtime implementation to support dynamic appearance changes and addon lifecycle, and wires these into the TerminalPane UI. Changes
Sequence DiagramsequenceDiagram
participant TerminalPane
participant useTerminalAppearance
participant TerminalRegistry
participant TerminalRuntime
participant XTerm
participant Addons
TerminalPane->>useTerminalAppearance: request appearance
useTerminalAppearance-->>TerminalPane: TerminalAppearance
TerminalPane->>TerminalRegistry: attach(terminalId, container, wsUrl, appearance)
TerminalRegistry->>TerminalRuntime: createRuntime(terminalId, appearance)
TerminalRuntime->>XTerm: createTerminal(appearance)
TerminalRuntime->>Addons: loadAddons(terminal)
Addons-->>TerminalRuntime: cleanupDisposer
TerminalRuntime-->>TerminalRegistry: runtime ready
TerminalRegistry-->>TerminalPane: attached
rect rgba(100,150,200,0.5)
TerminalPane->>TerminalRegistry: updateAppearance(terminalId, appearance)
TerminalRegistry->>TerminalRuntime: updateRuntimeAppearance(runtime, appearance)
TerminalRuntime->>XTerm: update theme & font options
end
TerminalPane->>TerminalRegistry: detach/dispose
TerminalRegistry->>TerminalRuntime: disposeRuntime(runtime)
TerminalRuntime->>Addons: cleanupDisposer()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/lib/terminal/terminal-addons.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-addons.ts:39">
P2: Avoid empty catch blocks here; log a warning with the error so unexpected addon load failures aren’t silently hidden.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/lib/terminal/appearance/index.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/appearance/index.ts:63">
P2: Avoid empty catch blocks here; log a warning with context so cached theme failures don’t disappear silently.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Greptile SummaryThis PR wires v2 workspace terminals to the existing user-configured theme and font settings, achieving parity with v1 terminals without importing v1 component logic. It introduces a shared Key changes and observations:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant TP as TerminalPane
participant UA as useTerminalAppearance
participant RR as TerminalRuntimeRegistry
participant RT as terminal-runtime
participant AD as terminal-addons
TP->>UA: useTerminalAppearance()
UA->>UA: useTerminalTheme() + getFontSettings (staleTime 30s)
UA-->>TP: memoized TerminalAppearance
Note over TP: Effect 1 — attach (deps: terminalId, wsUrl)
TP->>RR: attach(terminalId, container, wsUrl, appearanceRef.current)
RR->>RT: createRuntime(terminalId, appearance)
RT->>AD: loadAddons(terminal)
AD->>AD: ClipboardAddon, Unicode11Addon, ImageAddon, LigaturesAddon (sync)
AD-->>AD: WebglAddon via rAF (deferred)
RT-->>RR: TerminalRuntime
RR->>RT: attachToContainer(runtime, container)
RT-->>TP: terminal visible + focused
Note over TP: Effect 2 — appearance (deps: terminalId, appearance)
TP->>RR: updateAppearance(terminalId, appearance)
RR->>RT: updateRuntimeAppearance(runtime, appearance)
RT->>RT: terminal.options.theme = appearance.theme
RT->>RT: font changed? → fitAddon.fit() if visible
Note over TP: On theme/font settings change
UA-->>TP: new TerminalAppearance (memo busted)
TP->>RR: updateAppearance(terminalId, newAppearance)
RR->>RT: updateRuntimeAppearance(runtime, newAppearance)
Reviews (1): Last reviewed commit: "Deslop" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts (1)
40-53:⚠️ Potential issue | 🟠 MajorRemove invalid
scrollbaroption—not supported in xterm.js 5.x.The option
scrollbar: { showScrollbar: false }does not exist in xterm.js 5.x ITerminalOptions and will not hide the scrollbar. Usescrollback: 0to disable scrolling (which removes the scrollbar), or hide it via CSS:.xterm-viewport::-webkit-scrollbar { display: none; }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts` around lines 40 - 53, The XTerm constructor in terminal-runtime.ts is passing an invalid option `scrollbar: { showScrollbar: false }` (xterm.js 5.x ITerminalOptions doesn't support `scrollbar`); remove that option from the XTerm(...) call (look for the `terminal = new XTerm({...})` instantiation) and instead disable scrolling via `scrollback: 0` or hide the scrollbar with CSS (e.g., target `.xterm-viewport::-webkit-scrollbar { display: none; }`) while preserving existing options like `cols`, `rows`, `cursorBlink`, `fontFamily`, `fontSize`, `theme`, `allowProposedApi`, `cursorStyle`, and `cursorInactiveStyle`.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/lib/terminal/appearance/index.ts (1)
39-39: Hardcoded fallback background duplicated.The fallback background
"#151110"appears both here (line 39) and inuseTerminalAppearance.ts(line 31). Consider exporting a constant from this module to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/terminal/appearance/index.ts` at line 39, The hardcoded fallback color "#151110" is duplicated; export a single constant (e.g. DEFAULT_TERMINAL_BACKGROUND) from this appearance module and use it wherever the fallback is needed. Change the object here to use background: theme.background ?? DEFAULT_TERMINAL_BACKGROUND, export DEFAULT_TERMINAL_BACKGROUND from this file, and update useTerminalAppearance.ts to import and use that constant instead of repeating the literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts`:
- Around line 40-53: The XTerm constructor in terminal-runtime.ts is passing an
invalid option `scrollbar: { showScrollbar: false }` (xterm.js 5.x
ITerminalOptions doesn't support `scrollbar`); remove that option from the
XTerm(...) call (look for the `terminal = new XTerm({...})` instantiation) and
instead disable scrolling via `scrollback: 0` or hide the scrollbar with CSS
(e.g., target `.xterm-viewport::-webkit-scrollbar { display: none; }`) while
preserving existing options like `cols`, `rows`, `cursorBlink`, `fontFamily`,
`fontSize`, `theme`, `allowProposedApi`, `cursorStyle`, and
`cursorInactiveStyle`.
---
Nitpick comments:
In `@apps/desktop/src/renderer/lib/terminal/appearance/index.ts`:
- Line 39: The hardcoded fallback color "#151110" is duplicated; export a single
constant (e.g. DEFAULT_TERMINAL_BACKGROUND) from this appearance module and use
it wherever the fallback is needed. Change the object here to use background:
theme.background ?? DEFAULT_TERMINAL_BACKGROUND, export
DEFAULT_TERMINAL_BACKGROUND from this file, and update useTerminalAppearance.ts
to import and use that constant instead of repeating the literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfdea033-f05c-4213-b12b-345250f84297
📒 Files selected for processing (7)
apps/desktop/src/renderer/lib/terminal/appearance/index.tsapps/desktop/src/renderer/lib/terminal/terminal-addons.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalAppearance/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalAppearance/useTerminalAppearance.ts
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
saddlepaddle
reviewed
Apr 3, 2026
| /** Fallback grid size used when the host is not visible. */ | ||
| lastCols: number; | ||
| lastRows: number; | ||
| _disposeAddons: (() => void) | null; |
saddlepaddle
reviewed
Apr 3, 2026
| }, [terminalId, websocketUrl]); | ||
|
|
||
| useEffect(() => { | ||
| terminalRuntimeRegistry.updateAppearance(terminalId, appearance); |
Collaborator
There was a problem hiding this comment.
this should probably just be managed by whatever is doing the lifecycle of terminals - most of the other logic for the lifecycle of terminals is colocated so it might be a better fit
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 5, 2026
…arity (ParadevAI#3155) * add appearance setup * Terminal addons * Lint * Remove done plan * Deslop * Deslop * p-2
9 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 5, 2026
cherry-pick方式で内容を取り込み済みの14コミットをgit履歴上もマージ済みにする。 取り込み済み (cherry-pick / 手動移植): - be22b46 ParadevAI#3125 — スキップ (下記参照) - 88bc7fb ParadevAI#3127 — Revert DA1 ✓ - 92d0ff9 ParadevAI#3054 — DA1 fix ✓ - c48450e ParadevAI#3093 — file viewer pane fix ✓ - fffa8db ParadevAI#3128 — version 1.4.7 ✓ - 589a7c7 ParadevAI#3136 — fuzzy scorer (ハイブリッド方式) ✓ - ceb8c81 ParadevAI#3150 — Electron 40.8.5 ✓ - 8922b94 ParadevAI#3137 — terminalId分離 ✓ - c7508e5 ParadevAI#3152 — GitHub無料化 ✓ - 2b91f11 ParadevAI#3155 — v2 terminal theme ✓ - b8b11af ParadevAI#3154 — TUI dimension fix ✓ - 7599ace ParadevAI#3149 — v2 sidebar file tree (手動統合) ✓ - 4d7c612 ParadevAI#3174 — DnD重複削除 ✓ - 864977d ParadevAI#3157 — Host Service分離 ✓ 意図的にスキップ: - be22b46 ParadevAI#3125 (GitHub polling簡素化) フォーク独自のGitHubSyncService (バックエンド集中ポーリング) と 設計が異なるため不採用。upstreamはフロントエンドhover駆動、フォークは バックエンドキャッシュウォーマー方式。詳細は githubQueryPolicy.ts と github-sync-service.ts のFORK NOTEを参照。 ゴースト・マージ復元 (revert 134cfd5 で消失した内容): - 538f306 ParadevAI#3120 — Patch vuln ✓ - 1588d20 ParadevAI#3108 — terminal lifecycle分離 ✓ - 59426f6 ParadevAI#3122 — file tree + FilePane + Alert refactor (手動統合) ✓ - 10d9a5d ParadevAI#3097 — tiptap line-height ✓ - 337a9ae ParadevAI#3121 — Codex hooks削除 ✓
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TerminalAppearancecontract (appearance/index.ts) with type, defaults (JetBrains Mono primary), and localStorage-cached theme for flash-free first paintuseTerminalAppearancehook that reads the theme store + font settings from local DB, returning a memoized appearance objectterminal-runtime-registryto defer runtime creation until firstattach()(no eager xterm instance before appearance is known) and addsupdateAppearance()for live updatesterminal-runtimenow acceptsTerminalAppearanceat creation and exposesupdateRuntimeAppearancefor in-place theme/font changes without remountingterminal-addons.ts— loads Clipboard, Unicode11, Image, Ligatures sync afteropen(), defers WebGL to rAF with global failure tracking (VS Code pattern)scrollback,allowProposedApi,cursorStyle, hidden scrollbar, etc.TerminalPanewires appearance into attach (via ref to avoid re-mount) and pushes live updates via a separate effect; container background driven byappearance.backgroundTest plan
▐▛███▜▌) render without line gaps (WebGL renderer)Summary by cubic
Wired the v2 terminal to user theme and font settings with live updates, matching v1. Improves first paint and stability by caching theme, deferring runtime/addons, and using WebGL with a safe DOM fallback.
TerminalAppearancewith defaults and a localStorage-cached terminal theme for flash-free first paint;useTerminalAppearancereads theme and font from settings.attach()and addsupdateAppearance()for live theme/font updates; applies appearance on reattach and forwards PTY resize when cols/rows change.TerminalAppearance, exposesupdateRuntimeAppearance, and reaches@xterm/xtermoption parity (DEFAULT_TERMINAL_SCROLLBACK,allowProposedApi, cursor styles, hidden scrollbar).open(); defers@xterm/addon-webglvia rAF with global failure tracking and context-loss fallback.TerminalPanepasses appearance intoattach()and pushes updates without remounts; container background usesappearance.background.Written for commit 1d048e3. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Documentation