Terminal Improvement And GPU acceleration#79
Open
gajendraxdev wants to merge 12 commits into
Open
Conversation
Keep the xterm canvas in sync during layout transitions by always running visual fit while deferring PTY resize IPC until layout settles. Remove legacy width pinning, add refresh after fit, window-resize refit, and a layout-transition safety timeout. Ensure the terminal container fills its host via CSS. Disable single-instance in debug builds so tauri dev no longer focuses the installed production app that shares the same identifier.
Introduce a modular terminal renderer stack with WebGL2 GPU rendering, explicit canvas fallback when GPU is disabled or WebGL is disposed, and compatible GPU + ligature activation. Settings include a GPU toggle, live renderer status for the active tab, and agent tests for policy, capability probing, session lifecycle, controller sync, and diagnostics.
Move Terminal.tsx under components/terminal/. Extract terminalCache, ligatures, renderer setup, and instance API into src/lib/terminal/. Update terminalSlice and aiContext to import from lib/terminal. Refresh TERMINAL_ROADMAP and related docs with current paths and GPU status.
…on guard - Explicitly call child.kill() in PtyManager close paths (roadmap item 5.7) instead of relying on Drop - Add reconnect_lock: Arc<Mutex<()>> to ConnectionHandle and take lock in reconnect_stored_connection - Improve error handling on tunnel stop (only remove handles on full success) - Remove .clone() on flush_deadline Instant in select branches - Closes races on rapid disconnect/reconnect and ensures child processes are cleaned Refs P1 5.7 + CodeRabbit reconnect / handle / clone comments.
…dingRestore on disconnect
- Add pendingInputBytes to TerminalCache and update incrementally instead of re-encoding full pendingInput on every keystroke (5.8)
- clearTerminalPendingInput also resets the byte counter
- clearTerminals accepts { preservePendingRestore?: boolean } and connectionSlice sets pendingRestore=true on SSH disconnect instead of wiping tabs (5.9)
- Remove sensitive console.log of fullConfig on connect (CodeRabbit)
- Improves keystroke perf and keeps terminal tabs alive across reconnect for better UX
Refs P1 5.8/5.9 + CodeRabbit zync-sh#1.
…sync and wakeup modules - New focused modules under src/lib/terminal/: ptyLifecycle, terminalSpawn, terminalLazyPty, terminalConnectionWakeup, terminalLifecycleListeners, terminalResizeSync, terminalFit, inputPipeline, inputQueue, spawnContext, suspendAllTerminals, terminalSpawnErrors, connectionBackend, connectionOpQueue - Centralize exports in index.ts; update Terminal.tsx / TerminalManager to delegate to lib - Update connectionConfig, MainLayout, ConnectionItem for new wakeup / op queue behavior - Enables Phase 1/2 exit criteria (lazy PTY, serialized input, gated flush, generation handling, reconnect wakeup) - Big reduction in Terminal.tsx size and coupling Part of terminal module reorganization.
…connect, spawn and renderer runner - New dedicated tests: terminalPtyLifecycle, terminalInput*, terminalReconnect*, terminalSpawn*, terminalLifecycleIntegration, terminalConnectionWakeup, terminalResizeSync, connectionOpQueue, terminalSpawnContext/Errors - Add and wire runTerminalRendererTests.mjs (addresses long chain in test:terminal-renderer) - Update runAllAgentTests.mjs and tsconfig.agent-tests.json for new coverage - Package.json now points test:terminal-renderer at the aggregator script Covers P1 exit criteria and CodeRabbit test runner feedback.
- 8d17335: child kill (5.7) + reconnect lock + CR fixes in Rust - 1fd77d5: pendingInputBytes (5.8) + preservePendingRestore (5.9) + remove console.log - d15f536: terminal module extraction / lifecycle refactor - a684bb5: tests + dedicated renderer test runner Also marks roadmap items and includes .code-review-graph ignore (from prior graph tooling).
Member
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
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 (16)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThis PR extracts terminal lifecycle, input, renderer, sizing, and reconnect logic into ChangesTerminal Stack Modularization and GPU Renderer
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/store/terminalSlice.ts (1)
224-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
preservePendingRestorestill tears down terminal instances before preserving tab metadata.In preserve mode, the function currently executes
terminal:killanddestroyTerminalInstance(...)first, which defeats the reconnect-preserve behavior and can drop in-memory terminal state before tabs are marked pending restore.Suggested fix
clearTerminals: (connectionId, options = {}) => { set(state => { - // Kill all known terminals for this connection and destroy cached instances const tabs = state.terminals[connectionId] || []; - tabs.forEach(t => { - ipc.send('terminal:kill', { termId: t.id }); - destroyTerminalInstance(t.id); - }); if (options.preservePendingRestore) { // Preserve tab list with pendingRestore metadata for SSH reconnect/restore flow (roadmap 5.9) // Do not delete active/synced so reconnect can wake the tabs. return { terminals: { ...state.terminals, [connectionId]: tabs.map(t => ({ ...t, pendingRestore: true })) }, }; } + // Kill all known terminals for this connection and destroy cached instances + tabs.forEach(t => { + ipc.send('terminal:kill', { termId: t.id }); + destroyTerminalInstance(t.id); + }); + const newTerminals = { ...state.terminals }; delete newTerminals[connectionId];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/terminalSlice.ts` around lines 224 - 239, The terminal kill and destroy operations in the forEach loop (containing `ipc.send('terminal:kill')` and `destroyTerminalInstance(t.id)`) are executing unconditionally before checking the `preservePendingRestore` flag, which means terminals are being destroyed even when preservation is intended. Move the entire forEach loop that kills and destroys terminals inside a conditional block so that it only executes when `preservePendingRestore` is false or not set, allowing terminals to be preserved intact when the flag is true.
🧹 Nitpick comments (2)
tests/terminalResizeSync.test.mjs (1)
20-26: ⚡ Quick winRestore the previous
globalThis.windowafter this test module finishes.This test mutates a process-global and never restores it, which can leak into subsequent test modules when run in a single Node process.
Suggested fix
+const previousWindow = globalThis.window; -globalThis.window = { +globalThis.window = { ipcRenderer: { send: (_channel, payload) => { ipcResizes.push(payload); }, }, }; -runTest('syncTerminalResize skips IPC when cache entry is missing', () => { +try { +runTest('syncTerminalResize skips IPC when cache entry is missing', () => { terminalCache.clear(); ipcResizes.length = 0; syncTerminalResize(SESSION, term); assert.equal(ipcResizes.length, 0); }); @@ runTest('syncTerminalResize sends IPC when PTY is live', () => { @@ }); console.log('Terminal resize sync tests passed.'); +} finally { + terminalCache.clear(); + if (previousWindow === undefined) { + delete globalThis.window; + } else { + globalThis.window = previousWindow; + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/terminalResizeSync.test.mjs` around lines 20 - 26, The test module is modifying globalThis.window without restoring it after the tests complete, which can cause side effects for other tests run in the same process. Save the original value of globalThis.window before the test setup (where ipcRenderer is being mocked), and add a cleanup function using an appropriate test framework hook (such as afterEach or after) to restore the original globalThis.window value after the tests finish running.src/features/connections/infrastructure/connectionOpQueue.ts (1)
3-24: ⚡ Quick winClean up settled queue tails from
opChainsto avoid unbounded key retention.
opChainsnever deletes completed entries, so removed/ephemeral connection IDs accumulate over uptime.Suggested fix
const previous = opChains.get(connectionId) ?? Promise.resolve(); const next = previous .catch(() => {}) .then(() => op()); - opChains.set( - connectionId, - next.then( - () => undefined, - () => undefined, - ), - ); + const tail = next.then( + () => undefined, + () => undefined, + ); + opChains.set(connectionId, tail); + tail.finally(() => { + if (opChains.get(connectionId) === tail) { + opChains.delete(connectionId); + } + }); return next; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/connections/infrastructure/connectionOpQueue.ts` around lines 3 - 24, The opChains Map accumulates entries indefinitely without removing completed operations, causing memory bloat over time. In the runSerializedConnectionOp function, after setting the next operation in the map, add cleanup logic to delete the connectionId entry from opChains once the promise settles (regardless of success or failure). This can be done by chaining an additional handler after the existing .then() calls that resolves to undefined, which removes the entry from the map using opChains.delete(connectionId).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 7-37: The CHANGELOG.md [Unreleased] section contains duplicate
"### Added" headings (appearing twice) and duplicate "### Fixed" headings
(appearing twice), which violates markdown linting rules (MD024). Consolidate
all entries by merging the first "### Added" block (containing Terminal Phase 2
Lifecycle Extraction, Phase 1 Hardening, Lazy PTY Spawn, etc.) with the second
"### Added" block (containing Terminal GPU Acceleration, GPU + Font Ligatures,
etc.) into a single unified "### Added" section. Similarly, merge the first "###
Fixed" block (Connecting Screen Transparency, Terminal External Writes, Input
Queue Drain, etc.) with the second "### Fixed" block (Terminal Resize Hardening,
Dev Single-Instance Focus Steal, GPU Off Blank Terminal, etc.) into a single
"### Fixed" section. Delete all duplicate heading lines so only one "### Added"
and one "### Fixed" heading remain under [Unreleased].
In `@src-tauri/src/commands.rs`:
- Around line 1840-1849: The code holds the state.connections lock while
awaiting the reconnect_lock, which can deadlock if a concurrent reconnect needs
state.connections while holding reconnect_lock. Fix this by acquiring the
reconnect_lock reference and releasing the state.connections lock before
awaiting the reconnect_lock - restructure the code to extract the clone of
h.reconnect_lock inside the lock scope, then drop the connections lock before
calling lock_owned().await. Additionally, ensure the same reconnect_lock
instance is preserved when replacing the handle (also applies to the code around
line 1899-1901) so that all waiters continue using the same lock instead of
splitting across old and new locks.
In `@src-tauri/src/pty.rs`:
- Around line 779-788: The close() method now explicitly kills the child process
for TerminalHandle::Local using child.kill(), but the close_by_connection()
method (lines 813-820) only aborts tasks without killing the child process. This
can leave the local shell process alive when closing by connection. Apply the
same explicit child process cleanup pattern used in close() to the
TerminalHandle::Local variant in close_by_connection() by adding the let _ =
child.kill(); call after handling the task aborts.
In `@src/lib/terminal/inputPipeline.ts`:
- Around line 35-38: The code in the section where cached.pendingInput is being
cleared (setting it to empty string and cached.pendingInputBytes to 0) happens
before the window.ipcRenderer.send() call completes. Move the clearing of
cached.pendingInput and cached.pendingInputBytes to occur after the
window.ipcRenderer.send() call succeeds, or wrap the send call in error handling
to restore the data if the send fails. This ensures that if the IPC write throws
an exception, the buffered input data is not lost.
In `@src/lib/terminal/inputQueue.ts`:
- Around line 22-40: The clearTerminalInputQueue function deletes the queue
entry immediately after bumping the epoch, which allows new tasks to start
executing before the current pre-clear task has finished executing. This breaks
per-session serialization and can cause PTY writes to interleave. Instead of
deleting the sessionQueues entry, you should either keep it as-is (the epoch
bump will prevent stale tasks from executing anyway) or ensure the deletion only
happens after the current promise in the queue has settled by waiting for it to
complete first before removing the entry from sessionQueues.
In `@src/lib/terminal/ligatures.ts`:
- Around line 41-47: The `cached.ligaturesEnabled` flag becomes stale because it
is not explicitly set to false when `disposeTerminalLigatures` is called,
leaving the flag in an inconsistent state with the actual addon lifecycle.
Ensure that whenever `disposeTerminalLigatures(cached)` is executed, the
`cached.ligaturesEnabled` property is explicitly set to false either within the
`disposeTerminalLigatures` function itself or immediately before the function
returns, so that the cached state accurately reflects that the ligatures addon
has been disabled.
In `@src/lib/terminal/rendererController.ts`:
- Around line 104-106: The issue is that when desiredKind changes while a
loadPromise is still in-flight, the method returns the old loadPromise without
reconciling to the new desiredKind. Fix this in the loadSync method by adding a
check after confirming rendererState.loadPromise exists: verify that the current
kind matches the desiredKind, and if they differ, do not return the stale
loadPromise but instead proceed to create a new transition to the updated
desiredKind. This same pattern needs to be applied at both locations where
loadPromise is returned (around lines 104-106 and 132-134) to ensure the
renderer state is reconciled whenever a transition completes and the desired
mode has changed in the interim.
In `@src/lib/terminal/terminalLifecycleListeners.ts`:
- Around line 27-37: Add proper error handling to all three listen() calls by
attaching .catch() handlers to each promise chain to handle registration
failures. Refactor the listener registration logic into a helper function that
properly manages the async flow, ensuring that cached.listenerAttached is only
set to true after all three listen() registrations complete successfully. In the
.then() callbacks for each listen() call, ensure the returned unlistenFn is
always stored in the terminalCache entry's unlisten array (initialize the array
if it doesn't exist), and handle the edge case where the cache entry is
unexpectedly absent by logging a warning and still returning the cleanup
function to prevent listener leaks. This prevents unhandled promise rejections,
ensures the listenerAttached flag accurately reflects registration state, and
guarantees cleanup functions are stored for proper teardown.
In `@src/lib/terminal/terminalResizeSync.ts`:
- Around line 21-22: The cached.lastResize state is being updated before the
window.ipcRenderer.send call for the terminal:resize event, which means if the
send operation fails, the cache is already marked as updated and subsequent
retries for the same size will be suppressed. Move the cached.lastResize =
nextSize assignment to execute only after confirming that
window.ipcRenderer.send successfully completes, or wrap the cache update in
error handling to ensure the state is only cached when the resize dispatch
actually succeeds.
In `@src/store/connectionSlice.ts`:
- Around line 531-542: The return statement at line 541 inside the finally block
is unsafe because it can suppress exceptions thrown from earlier code paths.
Remove the return statement from this finally block in the disconnect flow.
Instead, restructure the code to handle the early exit logic in the main
conditional block before the set call completes, so that the return happens in
the normal control flow rather than in the finally block. Ensure the logic that
maps terminals with pendingRestore: true and calls markConnectionStatus still
executes correctly without needing the finally block return.
In `@tests/connectionOpQueue.test.mjs`:
- Around line 4-12: The `runTest` function needs to properly handle async test
functions. Make the `runTest` function itself async and await the call to `fn()`
so that async test bodies complete before logging success. This ensures the
try-catch block properly catches any errors thrown during async test execution
and prevents false passes when tests are still pending.
In `@tests/runTerminalRendererTests.mjs`:
- Around line 3-18: The tests array is missing two terminal regression test
files that are part of the terminal behavior coverage. Add the missing test
files 'tests/terminalSpawnErrors.test.mjs' and
'tests/terminalReconnectFlow.test.mjs' to the tests array in the appropriate
positions to ensure complete regression test coverage when running the terminal
renderer test suite.
In `@tests/terminalReconnectReset.test.mjs`:
- Around line 20-22: The mock send function in the test is only capturing the
payload parameter into ipcKills but ignoring the _channel parameter, which means
the test cannot verify that the correct IPC channel name is being sent. Modify
the send mock function to capture both the _channel and payload (perhaps as an
object with channel and payload properties) into ipcKills so that assertions can
verify the correct IPC event name is being used in addition to the payload
content. Apply this fix to both occurrences of the send function mock.
---
Outside diff comments:
In `@src/store/terminalSlice.ts`:
- Around line 224-239: The terminal kill and destroy operations in the forEach
loop (containing `ipc.send('terminal:kill')` and
`destroyTerminalInstance(t.id)`) are executing unconditionally before checking
the `preservePendingRestore` flag, which means terminals are being destroyed
even when preservation is intended. Move the entire forEach loop that kills and
destroys terminals inside a conditional block so that it only executes when
`preservePendingRestore` is false or not set, allowing terminals to be preserved
intact when the flag is true.
---
Nitpick comments:
In `@src/features/connections/infrastructure/connectionOpQueue.ts`:
- Around line 3-24: The opChains Map accumulates entries indefinitely without
removing completed operations, causing memory bloat over time. In the
runSerializedConnectionOp function, after setting the next operation in the map,
add cleanup logic to delete the connectionId entry from opChains once the
promise settles (regardless of success or failure). This can be done by chaining
an additional handler after the existing .then() calls that resolves to
undefined, which removes the entry from the map using
opChains.delete(connectionId).
In `@tests/terminalResizeSync.test.mjs`:
- Around line 20-26: The test module is modifying globalThis.window without
restoring it after the tests complete, which can cause side effects for other
tests run in the same process. Save the original value of globalThis.window
before the test setup (where ipcRenderer is being mocked), and add a cleanup
function using an appropriate test framework hook (such as afterEach or after)
to restore the original globalThis.window value after the tests finish running.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52af6cd4-cc39-437e-b74c-9eda1adaf05b
📒 Files selected for processing (70)
.gitignoreCHANGELOG.mddocs/SESSION_PERSISTENCE.mddocs/TERMINAL_GHOST_SUGGESTIONS.mddocs/TERMINAL_ROADMAP.mdpackage.jsonsrc-tauri/src/commands.rssrc-tauri/src/lib.rssrc-tauri/src/pty.rssrc/components/layout/MainLayout.tsxsrc/components/layout/sidebar/ConnectionItem.tsxsrc/components/settings/constants/defaults.tssrc/components/settings/tabs/TerminalRendererStatus.tsxsrc/components/settings/tabs/TerminalTab.tsxsrc/components/terminal/Terminal.tsxsrc/components/terminal/TerminalManager.tsxsrc/features/connections/domain/connectionConfig.tssrc/features/connections/infrastructure/connectionOpQueue.tssrc/index.csssrc/lib/aiContext.tssrc/lib/terminal/connectionBackend.tssrc/lib/terminal/index.tssrc/lib/terminal/inputPipeline.tssrc/lib/terminal/inputQueue.tssrc/lib/terminal/instanceApi.tssrc/lib/terminal/ligatures.tssrc/lib/terminal/ptyLifecycle.tssrc/lib/terminal/rendererController.tssrc/lib/terminal/rendererDiagnostics.tssrc/lib/terminal/rendererLifecycle.tssrc/lib/terminal/rendererPolicy.tssrc/lib/terminal/rendererSession.tssrc/lib/terminal/rendererSetup.tssrc/lib/terminal/spawnContext.tssrc/lib/terminal/suspendAllTerminals.tssrc/lib/terminal/terminalCache.tssrc/lib/terminal/terminalConnectionWakeup.tssrc/lib/terminal/terminalFit.tssrc/lib/terminal/terminalLazyPty.tssrc/lib/terminal/terminalLifecycleListeners.tssrc/lib/terminal/terminalResizeSync.tssrc/lib/terminal/terminalSpawn.tssrc/lib/terminal/terminalSpawnErrors.tssrc/lib/terminal/types.tssrc/lib/terminal/webglCapability.tssrc/store/connectionSlice.tssrc/store/sessionSlice.tssrc/store/settingsSlice.tssrc/store/terminalSlice.tstests/connectionDomain.test.mjstests/connectionOpQueue.test.mjstests/runAllAgentTests.mjstests/runTerminalRendererTests.mjstests/terminalConnectionWakeup.test.mjstests/terminalInputPipeline.test.mjstests/terminalInputQueue.test.mjstests/terminalLazyPty.test.mjstests/terminalLifecycleIntegration.test.mjstests/terminalPtyLifecycle.test.mjstests/terminalReconnectFlow.test.mjstests/terminalReconnectReset.test.mjstests/terminalRendererController.test.mjstests/terminalRendererDiagnostics.test.mjstests/terminalRendererPolicy.test.mjstests/terminalRendererSession.test.mjstests/terminalResizeSync.test.mjstests/terminalSpawnContext.test.mjstests/terminalSpawnErrors.test.mjstests/terminalWebglCapability.test.mjstsconfig.agent-tests.json
- CHANGELOG: deduplicate ### Added/### Fixed headings under Unreleased - commands.rs: fix reconnect_lock acquisition (no lock held across await) + preserve same lock instance on handle replace - pty.rs: add explicit child.kill() in close_by_connection for Local - inputPipeline.ts: clear pendingInput after send - inputQueue.ts: avoid deleting queue entry on clear (epoch suffices) - ligatures.ts: set ligaturesEnabled=false on dispose - rendererController.ts: reconcile desiredKind before returning stale loadPromise - terminalLifecycleListeners.ts: add .catch handlers + set listenerAttached correctly - terminalResizeSync.ts: update lastResize after successful send - connectionSlice.ts: remove return from finally block - terminalSlice.ts: only kill/destroy when not preserving pendingRestore - connectionOpQueue.ts: add cleanup for opChains entries - Tests: make runTest async, capture channel in mocks, restore globalThis.window, add missing test files to runner All items verified against current code and fixed minimally.
- Added reference to 579efb4 for the review fixes.
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
Documentation
Tests