feat(terminal): integrate WebGL context manager for GPU-accelerated rendering#1831
feat(terminal): integrate WebGL context manager for GPU-accelerated rendering#1831
Conversation
github-actions
bot
added
area/frontend
Summary of ChangesHello @AndyMik90, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances terminal rendering performance by leveraging WebGL for GPU acceleration. It provides users with control over this feature through a new setting, ensuring a smoother experience, especially when managing multiple terminals. The integration is designed to be robust, handling browser limitations and falling back gracefully on unsupported environments. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional GPU acceleration (type and settings), a GPU selection in DisplaySettings with i18n, lazy WebGL loading/unloading in useXterm with tests and mocks, introduces hasExited guards and clearPendingDelete in terminal lifecycle, and replaces some direct renderer IPC with safeSendToRenderer plus defensive logging wrappers. Changes
Sequence DiagramsequenceDiagram
actor User
participant DisplaySettings
participant SettingsStore
participant useXterm
participant WebGL as webglContextManager
User->>DisplaySettings: choose GPU option (auto/on/off)
DisplaySettings->>SettingsStore: set gpuAcceleration
SettingsStore->>useXterm: notify settings changed
alt gpuAcceleration == "off"
useXterm->>WebGL: (skip import/acquire)
else
useXterm->>WebGL: import (dynamic) & register(terminal)
WebGL-->>useXterm: registered
useXterm->>WebGL: acquire(terminal)
WebGL-->>useXterm: acquired
end
Note over useXterm,WebGL: On terminal disposal
useXterm->>WebGL: unregister(terminal)
WebGL-->>useXterm: unregistered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a GPU acceleration setting for the terminal, leveraging WebGL for improved rendering performance. The changes are well-structured, including a new UI setting, integration into the useXterm hook, and comprehensive tests. The implementation correctly handles the WebGL context lifecycle and provides a fallback for unsupported environments. My review focuses on improving the maintainability of the new tests, which are currently a bit brittle due to their reliance on component rendering order.
| // The GPU acceleration select is the second Select rendered (index 1, after logOrder) | ||
| const gpuSelect = container.querySelector('[data-testid="select-root-1"]'); | ||
| expect(gpuSelect).toHaveAttribute('data-value', 'on'); |
There was a problem hiding this comment.
This test selector is brittle because it relies on the rendering order of <Select> components. If another <Select> is added to DisplaySettings.tsx before this one, this test will likely fail. This same issue exists on line 99.
You can make this more robust by finding the SelectTrigger via its unique data-testid (which is derived from its id) and then traversing up to the Select root element.
// The GPU acceleration select can be found reliably via its trigger's test-id
const gpuTrigger = screen.getByTestId('select-trigger-gpuAcceleration');
const gpuSelect = gpuTrigger.closest('[data-testid^="select-root-"]');
expect(gpuSelect).toHaveAttribute('data-value', 'on');
| // selectCallbacks[1] is the GPU acceleration Select's onValueChange | ||
| selectCallbacks[1]('on'); |
There was a problem hiding this comment.
Accessing the onValueChange callback by its index in the selectCallbacks array is brittle and depends on the rendering order. This can lead to test failures if the component's structure changes, and this pattern is repeated in multiple tests in this file.
To make these tests more robust, consider improving the mock for the <Select> component. For example, you could store callbacks in a Map keyed by a stable identifier (like the id of the SelectTrigger).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
apps/frontend/src/renderer/components/settings/__tests__/DisplaySettings.test.tsx
Fixed
Show fixed
Hide fixed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/renderer/components/terminal/useXterm.ts (1)
329-329: 🧹 Nitpick | 🔵 TrivialNote:
gpuAccelerationis intentionally read viagetState()and not in the dependency array.This means changing the GPU acceleration setting only takes effect for newly opened terminals. If this UX trade-off is intentional (as stated in the PR design decisions), consider adding a brief comment near the setting read (line 124) to make this explicit for future maintainers.
📝 Suggested comment addition
// WebGL acceleration: register terminal and acquire context based on user setting // Must happen AFTER xterm.open() — xterm.js requires the terminal to be mounted first + // Note: Read once at creation time (not reactive) — changing the setting only affects new terminals const gpuAcceleration = useSettingsStore.getState().settings.gpuAcceleration ?? 'auto';
🤖 Fix all issues with AI agents
In
`@apps/frontend/src/renderer/components/settings/__tests__/DisplaySettings.test.tsx`:
- Around line 18-42: The current mock stores Select onValueChange handlers in an
index array (selectCallbacks) which is fragile; change selectCallbacks to a map
keyed by an identifier and ensure the mock threads an id through
Select/SelectTrigger so callbacks are registered by that id (e.g.,
selectCallbacks: Record<string, (v:string)=>void> and in the mocked Select use a
prop id or read the id passed to SelectTrigger to register selectCallbacks[id] =
onValueChange); update data-testid values to include that id (use the existing
select-trigger-{id} pattern) and update tests to call
selectCallbacks['gpuAcceleration']('on') instead of selectCallbacks[1]('on').
In `@apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts`:
- Around line 865-878: The test declares block-scoped variables
originalRequestAnimationFrame and originalCancelAnimationFrame that shadow
module-level constants with the same names; rename these local variables (e.g.,
origRAF and origCAF or
originalRequestAnimationFrameLocal/originalCancelAnimationFrameLocal) in the
beforeAll/afterAll scope to avoid shadowing and confusion, update their usages
inside beforeAll and afterAll and any references in that describe block (the
mocked global.requestAnimationFrame/cancelAnimationFrame setup) to use the new
identifiers.
In `@apps/frontend/src/renderer/components/terminal/useXterm.ts`:
- Around line 565-566: In dispose() inside useXterm.ts wrap the call to
webglContextManager.unregister(terminalId) in a try/catch so an exception there
cannot short-circuit the rest of the cleanup; catch the error, log or report it
(e.g., via existing logger/console) with context including terminalId, then
continue to dispose remaining addons and the xterm instance (the code paths
around webglContextManager.unregister, addon disposal loops and
xterm.dispose()). Ensure the catch is narrow (only around unregister) and does
not suppress the later disposal steps in the function.
apps/frontend/src/renderer/components/settings/__tests__/DisplaySettings.test.tsx
Outdated
Show resolved
Hide resolved
| describe('useXterm WebGL context management', () => { | ||
| // Mock requestAnimationFrame for jsdom environment | ||
| const originalRequestAnimationFrame = global.requestAnimationFrame; | ||
| const originalCancelAnimationFrame = global.cancelAnimationFrame; | ||
|
|
||
| beforeAll(() => { | ||
| global.requestAnimationFrame = vi.fn((cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number); | ||
| global.cancelAnimationFrame = vi.fn((id: number) => clearTimeout(id)); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| global.requestAnimationFrame = originalRequestAnimationFrame; | ||
| global.cancelAnimationFrame = originalCancelAnimationFrame; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: originalRequestAnimationFrame / originalCancelAnimationFrame shadow module-level constants.
Lines 867–868 re-declare names identical to lines 102–103. Not a bug (block-scoped), but could confuse readers. Consider renaming to origRAF / origCAF or similar to differentiate from the outer scope.
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts`
around lines 865 - 878, The test declares block-scoped variables
originalRequestAnimationFrame and originalCancelAnimationFrame that shadow
module-level constants with the same names; rename these local variables (e.g.,
origRAF and origCAF or
originalRequestAnimationFrameLocal/originalCancelAnimationFrameLocal) in the
beforeAll/afterAll scope to avoid shadowing and confusion, update their usages
inside beforeAll and afterAll and any references in that describe block (the
mocked global.requestAnimationFrame/cancelAnimationFrame setup) to use the new
identifiers.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 1 structural/other issue(s) require attention.
Branch is out of date with base branch. Update branch first - if no conflicts arise, you can merge. If merge conflicts arise, resolve them and run follow-up review again. 1 non-blocking suggestion(s) to consider.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Medium | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
Findings Summary
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (1 selected of 1 total)
🔵 [e3d69cc2e31e] [LOW] Unused import: fireEvent in DisplaySettings test
📁 apps/frontend/src/renderer/components/settings/__tests__/DisplaySettings.test.tsx:5
The fireEvent import from @testing-library/react is imported but never used in the test file. The tests use the mocked selectCallbacks array to simulate value changes instead of fireEvent, making this import dead code.
Suggested fix:
Remove `fireEvent` from the import: `import { render, screen } from '@testing-library/react';`
This review was generated by Auto Claude.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
✅ Auto Claude Review - APPROVED
Status: Ready to Merge
Summary: ## 🟡 Follow-up Review: Merge With Changes
🟡 Can merge - Minor suggestions noted, no blockers.
Resolution Status
- ✅ Resolved: 0 previous findings addressed
- ❌ Unresolved: 1 previous findings remain
- 🆕 New Issues: 2 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 3 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
Verdict
All 20 CI checks are passing. The previous blocking issue (branch out of date) is resolved by the merge commit d1e84d5.
3 confirmed-valid findings remain, all LOW severity:
- [e3d69cc2e31e] Unused
fireEventimport - trivial cleanup (unresolved from previous review) - [NEW-001] GPU acceleration setting reads once at terminal init (not reactive) - this is a reasonable design choice given WebGL context complexity; a UX note ("changes apply to new terminals") would be a nice addition
- [NEW-003] Test uses positional indexing for Select callbacks - fragile but pragmatic for jsdom testing
3 findings were dismissed as false positives after validation:
- NEW-002: Type cast is safe (Select constrains values, matches existing logOrder pattern)
- NEW-004: Mock pattern is harmless (no test asserts on updateSettings)
- NEW-005: Standard settings dialog save pattern (identical to all other non-preview settings)
No security vulnerabilities, no logic bugs, no regressions. The WebGL context management integration is correct (register after open, acquire based on setting, unregister before dispose). i18n is complete (en + fr). Types are properly defined. Tests provide good coverage. All remaining items are optional polish that can be addressed post-merge.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
💡 Suggestions (3)
These are non-blocking suggestions for consideration:
🔵 [e3d69cc2e31e] [LOW] [UNRESOLVED] Unused import: fireEvent in DisplaySettings test
📁 apps/frontend/src/renderer/components/settings/__tests__/DisplaySettings.test.tsx:5
The fireEvent import from @testing-library/react is imported but never used in the test file. The tests use the mocked selectCallbacks array to simulate value changes instead of fireEvent, making this import dead code.
Resolution note: Line 5 of DisplaySettings.test.tsx still contains: import { render, screen, fireEvent } from '@testing-library/react'; — fireEvent appears only in this import and is never used anywhere in the 151-line file. Tests use selectCallbacks[] to simulate Select changes.
Suggested fix:
Remove `fireEvent` from the import: `import { render, screen } from '@testing-library/react';`
🔵 [NEW-001] [LOW] GPU acceleration setting not reactive for existing terminals
📁 apps/frontend/src/renderer/components/terminal/useXterm.ts:124
The gpuAcceleration setting is read once at terminal initialization via useSettingsStore.getState() (a snapshot, not a subscription). Changing GPU acceleration in settings won't affect already-open terminals — only newly created ones. Font settings have reactive updates via useTerminalFontSettingsStore.subscribe, but GPU acceleration does not. This is likely intentional since WebGL context hot-swapping is complex, but a UX note ('Changes apply to new terminals only') would improve clarity.
Suggested fix:
Add a helper text like 'Changes apply to new terminals only' beneath the GPU acceleration dropdown in DisplaySettings.tsx, or add a reactive subscription if hot-swapping is feasible.
🔵 [NEW-003] [LOW] Test relies on fragile positional indexing of Select components
📁 apps/frontend/src/renderer/components/settings/__tests__/DisplaySettings.test.tsx:19
Tests use selectCallbacks[1] to reference the GPU acceleration Select's onValueChange, assuming it is the 2nd Select rendered (after logOrder). If Selects are reordered or a new Select is added before it, tests would silently break by invoking the wrong callback. Comments document the assumption but don't prevent the issue.
Suggested fix:
Consider using a Map keyed by the Select's id prop instead of a positional array, making tests resilient to component reordering.
This automated review found no blocking issues. The PR can be safely merged.
Generated by Auto Claude
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/terminal-session-store.ts`:
- Around line 369-371: The debug logging currently eagerly serializes
this.pendingDelete with [...this.pendingDelete].join(', ') on every call in
updateSessionInMemory/removeSession, causing allocations on a hot path; change
the call so serialization only happens when debug logging is active — either
wrap the serialization in a conditional that checks the logger's enabled flag
before constructing the string, or switch to a lazy callback pattern if debugLog
supports a thunk (e.g., pass () => [...this.pendingDelete].join(', ')); update
the debugLog invocation sites around the debug message in TerminalSessionStore
(references: debugLog, pendingDelete, updateSessionInMemory, removeSession)
accordingly.
In `@apps/frontend/src/renderer/components/terminal/useXterm.ts`:
- Around line 122-134: The try-catch around webglContextManager.register/
acquire can leave the terminal unregistered yet dispose() still calls
webglContextManager.unregister(terminalId), producing misleading errors; modify
useXterm to track registration success (e.g., a boolean or ref like
registrationSucceeded) by setting it true immediately after
webglContextManager.register(terminalId, xterm) and only call
webglContextManager.unregister(terminalId) inside dispose() when
registrationSucceeded is true (or alternatively ensure
webglContextManager.unregister is idempotent and no-op for unknown IDs);
reference the functions/register points: webglContextManager.register,
webglContextManager.acquire, webglContextManager.unregister, the terminalId
variable, and the dispose() cleanup path.
| debugLog('[TerminalSessionStore] Skipping save for deleted session:', session.id, | ||
| 'pendingDelete size:', this.pendingDelete.size, | ||
| 'all pending IDs:', [...this.pendingDelete].join(', ')); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Debug logging spreads pendingDelete on every invocation.
[...this.pendingDelete].join(', ') allocates an array and builds a string on every updateSessionInMemory and removeSession call, even if debug logging is effectively a no-op at runtime. If debugLog is not gated behind a compile-time or lazy-evaluation check, this is wasted work on a hot path (updateSessionInMemory is called by the periodic saveSessionAsync).
Consider deferring the serialization behind a condition or using a lazy callback pattern if debugLog supports one.
Also applies to: 611-613
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal-session-store.ts` around lines 369 - 371, The
debug logging currently eagerly serializes this.pendingDelete with
[...this.pendingDelete].join(', ') on every call in
updateSessionInMemory/removeSession, causing allocations on a hot path; change
the call so serialization only happens when debug logging is active — either
wrap the serialization in a conditional that checks the logger's enabled flag
before constructing the string, or switch to a lazy callback pattern if debugLog
supports a thunk (e.g., pass () => [...this.pendingDelete].join(', ')); update
the debugLog invocation sites around the debug message in TerminalSessionStore
(references: debugLog, pendingDelete, updateSessionInMemory, removeSession)
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/terminal-session-store.ts`:
- Around line 666-668: The log calculates "Remaining" speculatively before
actually removing the session from the Set; update the cleanup in
TerminalSessionStore so you call this.pendingDelete.delete(sessionId) first and
then log the actual remaining count (this.pendingDelete.size) — reference the
pendingDelete Set and the cleanup timer callback where the current debugLog is
called; ensure the debug message still includes sessionId but uses the
post-delete size for accuracy.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/terminal/pty-manager.ts`:
- Around line 343-348: Remove the redundant terminal.hasExited check in the
synchronous (non-chunked) write branch of the write handling code: the function
already returns early when terminal.hasExited, so the later guard inside the
direct write path (the one that calls resolve() and return) is unreachable;
delete that redundant if (terminal.hasExited) { resolve(); return; } block in
the direct write branch and leave the re-check in the chunked/setImmediate path
intact (keep the guard used before yielding).
In `@apps/frontend/src/main/terminal/terminal-lifecycle.ts`:
- Around line 57-62: Move the call to SessionHandler.clearPendingDelete(id)
inside the existing try block so any exceptions it throws are caught by the
surrounding catch and the function can consistently return { success: false,
error: ... }; update the try/catch around the session persistence logic (the
block that currently handles other errors) to include
SessionHandler.clearPendingDelete(id) and ensure the catch returns the
standardized error object.
In `@apps/frontend/src/shared/constants/config.ts`:
- Around line 70-74: The default for gpuAcceleration in
apps/frontend/src/shared/constants/config.ts currently reads 'off' but the PR
description and intent specify the default should be 'auto'; update the
gpuAcceleration constant (gpuAcceleration: 'off' as const) to use 'auto'
(gpuAcceleration: 'auto' as const) and adjust the inline comment to reflect that
the default is 'auto' (i.e., WebGL enabled where supported, users can opt-out
via Settings), or alternatively change the PR description to match the current
'off' default — prefer changing the constant to 'auto' to match the PR intent.
| // Clear any pendingDelete for this terminal ID. This handles the case where | ||
| // a terminal is destroyed and immediately re-created with the same ID (e.g., | ||
| // worktree switching, terminal restart after shell exit). Without this, the | ||
| // pendingDelete guard (5-second window) blocks session persistence for the | ||
| // new terminal, causing it to be invisible to the session store. | ||
| SessionHandler.clearPendingDelete(id); |
There was a problem hiding this comment.
clearPendingDelete outside try block could propagate an unhandled error.
If SessionHandler.clearPendingDelete(id) throws, the exception bypasses the try/catch on line 64 and propagates as an unhandled rejection instead of returning { success: false, error: ... }. Consider moving it inside the try block for consistency with the function's error-handling contract.
Suggested fix
- // Clear any pendingDelete for this terminal ID. ...
- SessionHandler.clearPendingDelete(id);
-
try {
+ // Clear any pendingDelete for this terminal ID. This handles the case where
+ // a terminal is destroyed and immediately re-created with the same ID (e.g.,
+ // worktree switching, terminal restart after shell exit). Without this, the
+ // pendingDelete guard (5-second window) blocks session persistence for the
+ // new terminal, causing it to be invisible to the session store.
+ SessionHandler.clearPendingDelete(id);
+
// For auth terminals, don't inject existing OAuth token - we want a fresh login🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/terminal-lifecycle.ts` around lines 57 - 62,
Move the call to SessionHandler.clearPendingDelete(id) inside the existing try
block so any exceptions it throws are caught by the surrounding catch and the
function can consistently return { success: false, error: ... }; update the
try/catch around the session persistence logic (the block that currently handles
other errors) to include SessionHandler.clearPendingDelete(id) and ensure the
catch returns the standardized error object.
| autoNameClaudeTerminals: true, | ||
| // GPU acceleration for terminal rendering | ||
| // Default to 'off' until WebGL stability is proven across all GPU drivers. | ||
| // Users can opt-in via Settings > Display > GPU Acceleration. | ||
| gpuAcceleration: 'off' as const |
There was a problem hiding this comment.
Default value 'off' contradicts the PR description which states 'auto'.
The PR objectives state: "user-configurable GPU Acceleration setting with default 'auto'" and "Default to 'auto' to enable WebGL where supported." However, the code defaults to 'off'. The inline comment explains the reasoning, but the PR description should be updated to reflect the actual default, or this should be changed to 'auto' if that was the intended behavior.
🤖 Prompt for AI Agents
In `@apps/frontend/src/shared/constants/config.ts` around lines 70 - 74, The
default for gpuAcceleration in apps/frontend/src/shared/constants/config.ts
currently reads 'off' but the PR description and intent specify the default
should be 'auto'; update the gpuAcceleration constant (gpuAcceleration: 'off' as
const) to use 'auto' (gpuAcceleration: 'auto' as const) and adjust the inline
comment to reflect that the default is 'auto' (i.e., WebGL enabled where
supported, users can opt-out via Settings), or alternatively change the PR
description to match the current 'off' default — prefer changing the constant to
'auto' to match the PR intent.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 3 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 0 previous findings addressed
- ❌ Unresolved: 3 previous findings remain
- 🆕 New Issues: 3 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 6 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 4 findings verified as genuine issues
- 👤 Needs Human Review: 1 findings require manual verification
🚨 Blocking Issues
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
Verdict
Branch is out of date with base branch. Update branch first - if no conflicts arise, you can merge. If merge conflicts arise, resolve them and run follow-up review again.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (6 selected of 6 total)
🔵 [e3d69cc2e31e] [LOW] [UNRESOLVED] [UNRESOLVED] Unused import: fireEvent in DisplaySettings test
📁 apps/frontend/src/renderer/components/settings/__tests__/DisplaySettings.test.tsx:5
The fireEvent import from @testing-library/react is imported but never used in the test file. The tests use the mocked selectCallbacks array to simulate value changes instead of fireEvent, making this import dead code.
Resolution note: Line 5 of DisplaySettings.test.tsx still contains: import { render, screen, fireEvent } from '@testing-library/react'; — fireEvent appears only in this import and is never used anywhere in the 151-line file. Tests use selectCallbacks[] to simulate Select changes.
Resolution note: import { render, screen, fireEvent } from '@testing-library/react'; — fireEvent is still imported at line 5 but never used in the 151-line test file. None of the 3 new commits modified this file.
Suggested fix:
Remove `fireEvent` from the import: `import { render, screen } from '@testing-library/react';`
🔵 [NEW-001] [LOW] [UNRESOLVED] GPU acceleration setting not reactive for existing terminals
📁 apps/frontend/src/renderer/components/terminal/useXterm.ts:124
The gpuAcceleration setting is read once at terminal initialization via useSettingsStore.getState() (a snapshot, not a subscription). Changing GPU acceleration in settings won't affect already-open terminals — only newly created ones. Font settings have reactive updates via useTerminalFontSettingsStore.subscribe, but GPU acceleration does not. This is likely intentional since WebGL context hot-swapping is complex, but a UX note ('Changes apply to new terminals only') would improve clarity.
Resolution note: const gpuAcceleration = useSettingsStore.getState().settings.gpuAcceleration ?? 'off'; — Still reads as a one-time snapshot at terminal initialization (line 129). No subscription for gpuAcceleration changes exists. However, code comments (lines 124-128) indicate this is an intentional design choice to prevent GPU process instability.
Suggested fix:
Add a helper text like 'Changes apply to new terminals only' beneath the GPU acceleration dropdown in DisplaySettings.tsx, or add a reactive subscription if hot-swapping is feasible.
🔵 [NEW-003] [LOW] [UNRESOLVED] Test relies on fragile positional indexing of Select components
📁 apps/frontend/src/renderer/components/settings/__tests__/DisplaySettings.test.tsx:19
Tests use selectCallbacks[1] to reference the GPU acceleration Select's onValueChange, assuming it is the 2nd Select rendered (after logOrder). If Selects are reordered or a new Select is added before it, tests would silently break by invoking the wrong callback. Comments document the assumption but don't prevent the issue.
Resolution note: selectCallbacks1 at lines 109, 121, 135. Tests still use positional array indexing to reference the GPU acceleration Select's onValueChange callback. None of the 3 new commits modified this file.
Suggested fix:
Consider using a Map keyed by the Select's id prop instead of a positional array, making tests resilient to component reordering.
🔵 [NCR-001] [LOW] Debug log computes speculative 'Remaining' count before actual delete
📁 apps/frontend/src/main/terminal-session-store.ts:666
The cleanup timer debug log at line 667 computes this.pendingDelete.size - 1 before this.pendingDelete.delete(sessionId) executes on line 668. If clearPendingDelete() already removed the entry, the logged count is off by one. Purely cosmetic debug logging issue.
Suggested fix:
Move debugLog after the delete call and log this.pendingDelete.size directly.
🔵 [NCR-003] [LOW] Redundant hasExited guard in synchronous write path
📁 apps/frontend/src/main/terminal/pty-manager.ts:345
The hasExited check at line 345 is redundant because line 303 already checks hasExited at the top of performWrite(). In the synchronous (non-chunked) branch, no async operation yields the event loop between these two checks, making the second check unreachable when hasExited is true. The check in the chunked/setImmediate path at line 316 IS necessary.
Suggested fix:
Remove the redundant if (terminal.hasExited) { resolve(); return; } block at line 345-348 in the synchronous else branch.
🔵 [CMT-001] [LOW] [FROM COMMENTS] CodeRabbit: Guard unregister in dispose() - ALREADY ADDRESSED
📁 apps/frontend/src/renderer/components/terminal/useXterm.ts:584
CodeRabbit flagged that unregister() could throw and skip subsequent disposal. The code at lines 583-589 already wraps webglManagerRef.current.unregister(terminalId) in a try-catch block with debugError logging. This concern was addressed in the implementation.
This review was generated by Auto Claude.
- Remove unused fireEvent import from DisplaySettings test - Add i18n helper text below GPU acceleration dropdown (en/fr) - Replace positional selectCallbacks array with Map keyed by Select id - Move debugLog after delete in terminal-session-store cleanup timer - Remove redundant hasExited guard in pty-manager synchronous write path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/terminal-session-store.ts (1)
407-410: 🧹 Nitpick | 🔵 TrivialRedundant
.slice()allocations for debug logging.Lines 407 and 421 each call
session.outputBuffer.slice(-MAX_OUTPUT_BUFFER).lengthsolely for the log message, then lines 415 and 427 repeat the same slice for actual storage. On a 100KB buffer this duplicates a non-trivial string allocation on every periodic save.♻️ Suggested fix: compute length without allocating
- const truncatedLen = session.outputBuffer.slice(-MAX_OUTPUT_BUFFER).length; + const truncatedLen = Math.min(session.outputBuffer.length, MAX_OUTPUT_BUFFER);Apply to both occurrences (lines 407 and 421).
Also applies to: 421-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/main/terminal-session-store.ts` around lines 407 - 410, The debug logging currently calls session.outputBuffer.slice(-MAX_OUTPUT_BUFFER).length twice (and the same slice is later used for storage), causing redundant allocations; change the logic in TerminalSessionStore to compute the truncated length without allocating (use Math.min(session.outputBuffer.length, MAX_OUTPUT_BUFFER) for the log) and compute the truncated string once (e.g. truncated = session.outputBuffer.length > MAX_OUTPUT_BUFFER ? session.outputBuffer.slice(-MAX_OUTPUT_BUFFER) : session.outputBuffer) and reuse truncated for both the debugLog and for saving; update the occurrences around debugLog, truncatedLen, and where the buffer is stored so you only slice once and log the computed length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/frontend/src/renderer/components/settings/__tests__/DisplaySettings.test.tsx`:
- Around line 64-156: Add an assertion that the helperText ("Changes apply to
new terminals only") is rendered for the GPU acceleration control; in the
DisplaySettings.test.tsx tests (where DisplaySettings is rendered and
description is asserted, e.g., the 'should render the GPU acceleration
description text' or a new test), after rendering call
expect(screen.getByText('Changes apply to new terminals
only')).toBeInTheDocument() so the new i18n key is covered.
---
Outside diff comments:
In `@apps/frontend/src/main/terminal-session-store.ts`:
- Around line 407-410: The debug logging currently calls
session.outputBuffer.slice(-MAX_OUTPUT_BUFFER).length twice (and the same slice
is later used for storage), causing redundant allocations; change the logic in
TerminalSessionStore to compute the truncated length without allocating (use
Math.min(session.outputBuffer.length, MAX_OUTPUT_BUFFER) for the log) and
compute the truncated string once (e.g. truncated = session.outputBuffer.length
> MAX_OUTPUT_BUFFER ? session.outputBuffer.slice(-MAX_OUTPUT_BUFFER) :
session.outputBuffer) and reuse truncated for both the debugLog and for saving;
update the occurrences around debugLog, truncatedLen, and where the buffer is
stored so you only slice once and log the computed length.
---
Duplicate comments:
In `@apps/frontend/src/main/terminal-session-store.ts`:
- Around line 381-383: The debug log eagerly serializes pendingDelete with
`[...this.pendingDelete].join(', ')` inside updateSessionInMemory (invoked by
saveSessionAsync), causing allocations on the hot path; change the call to avoid
eager work by only serializing pendingDelete when debug logging is enabled (e.g.
guard with a debugLog.isEnabled / debugLog.enabled check before constructing the
array/string) or use a lazy callback variant of debugLog if available so the
join runs only when the message will actually be emitted; update the call site
that references debugLog and pendingDelete accordingly.
| it('should render the GPU acceleration dropdown with all 3 options', () => { | ||
| render( | ||
| <DisplaySettings settings={defaultSettings} onSettingsChange={mockOnSettingsChange} /> | ||
| ); | ||
|
|
||
| expect(screen.getByText('GPU Acceleration')).toBeInTheDocument(); | ||
| expect(screen.getByTestId('select-item-auto')).toBeInTheDocument(); | ||
| expect(screen.getByTestId('select-item-on')).toBeInTheDocument(); | ||
| expect(screen.getByTestId('select-item-off')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should display the correct translated labels for each option', () => { | ||
| render( | ||
| <DisplaySettings settings={defaultSettings} onSettingsChange={mockOnSettingsChange} /> | ||
| ); | ||
|
|
||
| expect(screen.getByText('Auto (use WebGL when supported)')).toBeInTheDocument(); | ||
| expect(screen.getByText('Always on')).toBeInTheDocument(); | ||
| expect(screen.getByText('Off (default)')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should display the current GPU acceleration value from settings', () => { | ||
| const settingsWithOn: AppSettings = { ...defaultSettings, gpuAcceleration: 'on' }; | ||
|
|
||
| render( | ||
| <DisplaySettings settings={settingsWithOn} onSettingsChange={mockOnSettingsChange} /> | ||
| ); | ||
|
|
||
| // The GPU acceleration select is identified by its trigger id | ||
| const gpuTrigger = screen.getByTestId('select-trigger-gpuAcceleration'); | ||
| const gpuSelect = gpuTrigger.closest('[data-value]'); | ||
| expect(gpuSelect).toHaveAttribute('data-value', 'on'); | ||
| }); | ||
|
|
||
| it('should default to "off" when gpuAcceleration is not set', () => { | ||
| const settingsWithoutGpu: AppSettings = { ...defaultSettings, gpuAcceleration: undefined }; | ||
|
|
||
| render( | ||
| <DisplaySettings settings={settingsWithoutGpu} onSettingsChange={mockOnSettingsChange} /> | ||
| ); | ||
|
|
||
| const gpuTrigger = screen.getByTestId('select-trigger-gpuAcceleration'); | ||
| const gpuSelect = gpuTrigger.closest('[data-value]'); | ||
| expect(gpuSelect).toHaveAttribute('data-value', 'off'); | ||
| }); | ||
|
|
||
| it('should call onSettingsChange with gpuAcceleration "on" when selected', () => { | ||
| render( | ||
| <DisplaySettings settings={defaultSettings} onSettingsChange={mockOnSettingsChange} /> | ||
| ); | ||
|
|
||
| selectCallbacks.get('gpuAcceleration')!('on'); | ||
|
|
||
| expect(mockOnSettingsChange).toHaveBeenCalledWith( | ||
| expect.objectContaining({ gpuAcceleration: 'on' }) | ||
| ); | ||
| }); | ||
|
|
||
| it('should call onSettingsChange with gpuAcceleration "off" when selected', () => { | ||
| render( | ||
| <DisplaySettings settings={defaultSettings} onSettingsChange={mockOnSettingsChange} /> | ||
| ); | ||
|
|
||
| selectCallbacks.get('gpuAcceleration')!('off'); | ||
|
|
||
| expect(mockOnSettingsChange).toHaveBeenCalledWith( | ||
| expect.objectContaining({ gpuAcceleration: 'off' }) | ||
| ); | ||
| }); | ||
|
|
||
| it('should call onSettingsChange with gpuAcceleration "auto" when selected', () => { | ||
| const settingsWithOff: AppSettings = { ...defaultSettings, gpuAcceleration: 'off' }; | ||
|
|
||
| render( | ||
| <DisplaySettings settings={settingsWithOff} onSettingsChange={mockOnSettingsChange} /> | ||
| ); | ||
|
|
||
| selectCallbacks.get('gpuAcceleration')!('auto'); | ||
|
|
||
| expect(mockOnSettingsChange).toHaveBeenCalledWith( | ||
| expect.objectContaining({ gpuAcceleration: 'auto' }) | ||
| ); | ||
| }); | ||
|
|
||
| it('should render the GPU acceleration description text', () => { | ||
| render( | ||
| <DisplaySettings settings={defaultSettings} onSettingsChange={mockOnSettingsChange} /> | ||
| ); | ||
|
|
||
| expect( | ||
| screen.getByText('Use WebGL for terminal rendering (experimental, faster with many terminals)') | ||
| ).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good coverage of GPU acceleration dropdown behavior.
Tests cover rendering, translated labels, current value binding, undefined-default fallback, all three onValueChange paths, and description text. Consider adding a quick assertion for the helperText ("Changes apply to new terminals only") to complete coverage of the new i18n keys, though this is optional.
🧪 Optional: add helperText assertion
it('should render the GPU acceleration description text', () => {
render(
<DisplaySettings settings={defaultSettings} onSettingsChange={mockOnSettingsChange} />
);
expect(
screen.getByText('Use WebGL for terminal rendering (experimental, faster with many terminals)')
).toBeInTheDocument();
+ expect(
+ screen.getByText('Changes apply to new terminals only')
+ ).toBeInTheDocument();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/frontend/src/renderer/components/settings/__tests__/DisplaySettings.test.tsx`
around lines 64 - 156, Add an assertion that the helperText ("Changes apply to
new terminals only") is rendered for the GPU acceleration control; in the
DisplaySettings.test.tsx tests (where DisplaySettings is rendered and
description is asserted, e.g., the 'should render the GPU acceleration
description text' or a new test), after rendering call
expect(screen.getByText('Changes apply to new terminals
only')).toBeInTheDocument() so the new i18n key is covered.
…endering Wire up the existing webgl-context-manager into the terminal rendering pipeline with a user-configurable GPU Acceleration setting (auto/on/off). WebGL gives 3-5x rendering performance for terminals while falling back gracefully on unsupported browsers (Safari, some Linux+NVIDIA setups). - Add GpuAcceleration type and gpuAcceleration field to AppSettings - Add GPU Acceleration dropdown in Display Settings (i18n: en + fr) - Register/acquire WebGL context after xterm.open(), unregister on dispose - Respect user setting: 'off' skips WebGL, 'auto'/'on' acquires context - Add 13 tests covering WebGL lifecycle and DisplaySettings GPU dropdown Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The WebGL register/acquire/unregister calls in useXterm were not wrapped in try-catch, meaning any failure during WebGL context acquisition (e.g., LRU eviction edge case, GPU memory pressure) would propagate as an uncaught exception and crash the renderer process. Also adds debug logging to terminal-session-store's pendingDelete mechanism to help diagnose a reported crash when spawning terminals after extended use with 5+ concurrent instances. The "Skipping save for deleted session" warning now includes the full pendingDelete state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… crashes Two bugs in terminal session persistence caused crashes when terminals were destroyed and recreated with the same ID: 1. pendingDelete blocked legitimate terminal recreation: When a terminal exits and is recreated (worktree switch, shell restart), the 5-second pendingDelete window silently blocked all session saves for the new terminal, leaving it invisible to the session store. Added clearPendingDelete() to remove the guard when createTerminal() is called with a reused ID. 2. Sync save() and async saveAsync() raced on the same temp file: Both methods wrote to terminals.json.tmp without coordination, causing ENOENT errors when one renamed a file the other had already moved. save() now checks writeInProgress and defers to the async writer when a write is in-flight. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sses - Introduced safe logging functions to prevent crashes from console write failures and unhandled errors. - Updated error logging setup to use safeLogUnhandled for uncaught exceptions and unhandled rejections. - Added guards in terminal process management to avoid operations on exited PTYs, preventing potential crashes. - Improved WebGL context management in terminal rendering to ensure stability during GPU acceleration. This commit aims to enhance the robustness of the logging and terminal handling mechanisms, addressing potential crash scenarios and improving overall application stability.
- Remove unused fireEvent import from DisplaySettings test - Add i18n helper text below GPU acceleration dropdown (en/fr) - Replace positional selectCallbacks array with Map keyed by Select id - Move debugLog after delete in terminal-session-store cleanup timer - Remove redundant hasExited guard in pty-manager synchronous write path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…imer The cleanup timer debug log was computing pendingDelete.size - 1 before the actual delete call, logging an incorrect remaining count. Moved both delete calls before the debug log so it reports the accurate size. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AndyMik90
force-pushed
the
feature/webgl-context-management
branch
from
e5d5b82 to
04618cc
Compare
1 participant
Summary
webgl-context-managerinto the terminal rendering pipeline with a user-configurable GPU Acceleration setting (auto/on/off)Changes
Type & Config (2 files)
settings.ts— AddedGpuAccelerationtype ('auto' | 'on' | 'off') andgpuAcceleration?field toAppSettingsconfig.ts— AddedgpuAcceleration: 'auto'default toDEFAULT_APP_SETTINGSi18n (2 files)
en/settings.json— English keys: label, description, auto/on/off optionsfr/settings.json— French translationsUI (1 file)
DisplaySettings.tsx— Added GPU Acceleration<Select>dropdown after Log Order setting (identical layout pattern)Core Integration (1 file)
useXterm.ts— Three additions:webglContextManageranduseSettingsStorexterm.open(): register terminal, conditionally acquire WebGL context based on settingdispose(): unregister WebGL context before addon disposalTests (2 files)
DisplaySettings.test.tsx(new) — 8 tests for GPU dropdown rendering, i18n labels, value binding, onSettingsChange callbacksuseXterm.test.ts— 5 new tests for WebGL lifecycle: auto acquires, on acquires, off skips, dispose unregisters, undefined fallbackDesign Decisions
getState()not a hookxterm.dispose()autoas defaultTest plan
[WebGLContextManager] Acquired context🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Localization
Tests
Bug Fixes / Chores