Merged
Conversation
Keep live PTY-backed terminals and project workspace layouts stable when switching projects, while guarding restore flows against stale async updates and focus drift. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 terminal-aware editor persistence and restoration: per-restore cancellation/owner tracking, reconciliation between persisted terminal tabs and live PTYs, workspace-store APIs ( Changes
Sequence DiagramsequenceDiagram
participant Editor as Editor Persistence
participant TerminalRestore as Terminal Restore
participant TerminalStore as Terminal Store
participant WorkspaceStore as Workspace Store
participant AutoSave as Terminal Auto-Save
Editor->>TerminalRestore: restore(projectId)
activate TerminalRestore
TerminalRestore->>AutoSave: setTerminalRestoreInProgress(projectId, true, ownerId)
TerminalRestore->>TerminalStore: loadPersistedTerminals(projectId)
TerminalRestore->>TerminalStore: fetch live terminals for project
TerminalRestore->>TerminalRestore: isCancelled? (abort if true)
TerminalRestore->>TerminalRestore: reconcile persisted layout with live terminals -> mapping
TerminalRestore->>WorkspaceStore: ensureTerminalTab(mapped/live terminalIds...)
TerminalRestore->>TerminalRestore: isCancelled? (abort if true)
alt Not cancelled
TerminalRestore->>WorkspaceStore: loadProjectWorkspace(reconciledRoot, activePaneId)
end
TerminalRestore->>AutoSave: setTerminalRestoreInProgress(projectId, false, ownerId)
deactivate TerminalRestore
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 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 |
![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
🧹 Nitpick comments (1)
src/renderer/hooks/use-terminal-restore.ts (1)
369-409: Consider removing unused_projectIdparameter.The
selectTerminalForProjectfunction no longer uses theprojectIdparameter (renamed to_projectIdto suppress the lint warning). Since the function now just returns a terminal ID based onexistingTerminalsandlayout, this parameter could be removed entirely.♻️ Optional cleanup
function selectTerminalForProject( - _projectId: string, existingTerminals: Array<{ id: string; name: string; projectId: string }>, layout: PersistedTerminalLayout | null ): string | null {And update the call site at line 272-276:
const terminalIdToSelect = selectTerminalForProject( - projectIdToRestore, liveProjectTerminals, layout )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/use-terminal-restore.ts` around lines 369 - 409, Remove the unused _projectId parameter from the selectTerminalForProject function signature and its internal references so the function only accepts (existingTerminals, layout) and returns the selected terminal ID; then update all call sites that pass a projectId (notably the invocation in the same file that currently supplies a projectId argument) to call selectTerminalForProject(existingTerminals, layout) instead, and run typechecks to update any TypeScript signatures/usages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/use-editor-persistence.test.ts`:
- Around line 494-556: The test declares resolveProjectA as "((value: ...) =>
void) | null" so TypeScript disallows calling it via optional chaining; change
its declared union to use undefined instead of null (i.e., "... | undefined") OR
replace the optional-chained call site "resolveProjectA?.(...)" with an explicit
runtime check and call (if (resolveProjectA) resolveProjectA(...)); update the
declaration for resolveProjectA and the call in the 'ignores stale restore
results after switching projects' test that uses useEditorPersistence to match
one of these fixes.
In `@src/renderer/hooks/use-terminal-restore.test.ts`:
- Around line 145-184: Change the resolver variable declaration so TypeScript
knows it may be undefined (not null) — replace "let resolveProjectALayout:
((value: null) => void) | null = null" with "let resolveProjectALayout: ((value:
null) => void) | undefined = undefined" (so resolveProjectALayout?.(null) is
seen as callable), leaving the rest of the test (the Promise executor assigning
it and the resolveProjectALayout?.(null) call) unchanged.
- Around line 186-212: The test's call to rejectProjectALayout is causing a
TypeScript "not callable" error; change the invocation to an explicit null check
before calling (e.g. if (rejectProjectALayout) rejectProjectALayout(new
Error('restore failed'))), updating the line that currently does
rejectProjectALayout?.(new Error('restore failed')) so the variable declared as
let rejectProjectALayout: ((reason?: unknown) => void) | null = null is narrowed
before use.
---
Nitpick comments:
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Around line 369-409: Remove the unused _projectId parameter from the
selectTerminalForProject function signature and its internal references so the
function only accepts (existingTerminals, layout) and returns the selected
terminal ID; then update all call sites that pass a projectId (notably the
invocation in the same file that currently supplies a projectId argument) to
call selectTerminalForProject(existingTerminals, layout) instead, and run
typechecks to update any TypeScript signatures/usages accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0a9bf90-d6d1-4076-96bc-8442d306d0d5
📒 Files selected for processing (8)
src/renderer/hooks/use-editor-persistence.test.tssrc/renderer/hooks/use-editor-persistence.tssrc/renderer/hooks/use-terminal-restore.test.tssrc/renderer/hooks/use-terminal-restore.tssrc/renderer/hooks/useTerminalAutoSave.tssrc/renderer/layouts/WorkspaceLayout.tsxsrc/renderer/stores/workspace-store.test.tssrc/renderer/stores/workspace-store.ts
Tighten the follow-up test typings and remove the unused project parameter from terminal restore selection to keep the PR green under typecheck. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/use-terminal-restore.ts (1)
298-307:⚠️ Potential issue | 🔴 CriticalCancellation stops before the expensive restore work actually finishes.
After the Line 299 guard, the code still awaits
restoreFromLayout()/createDefaultTerminal(). Both helpers do more async work and then mutate the stores without rechecking cancellation, so a fast project switch here can still spawn PTYs or re-add terminals for a stale project.🐛 Proposed fix
- if (layout && layout.terminals.length > 0) { - await restoreFromLayout(projectIdToRestore, layout) - } else { - await createDefaultTerminal(projectIdToRestore) + if (layout && layout.terminals.length > 0) { + await restoreFromLayout(projectIdToRestore, layout, isCancelled) + } else { + await createDefaultTerminal(projectIdToRestore, isCancelled) }Then bail out inside those helpers before each store mutation and after awaited spawn steps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/use-terminal-restore.ts` around lines 298 - 307, The restore flow in useTerminalRestore still awaits restoreFromLayout(projectIdToRestore, layout) and createDefaultTerminal(projectIdToRestore) even after the isCancelled() guard, so those helpers can continue async work and mutate stores for a stale project; update restoreFromLayout and createDefaultTerminal to check cancellation (use the same isCancelled or callId guard) before every store mutation and immediately after any awaited spawn/long async step, returning early if cancelled to prevent spawning PTYs or re-adding terminals for the wrong project in useTerminalRestore.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/use-terminal-restore.test.ts`:
- Around line 186-211: The test can spuriously pass because it only waits for
mockSetTerminalRestoreInProgress('project-b', true) and not for the
cancelled-restore path to finish, so ensure the rejected
mockLoadPersistedTerminals resolution is processed and any fallback creation
(createDefaultTerminal -> mockTerminalSpawn /
mockTerminalStoreState.addTerminal) would have had a chance to run; after
calling rejectProjectALayout(new Error(...)) await the promise microtask flush
(e.g., await act(() => Promise.resolve()) or simply await waitFor(() => {/*
no-op to let promise settle */})) and then replace the final synchronous expects
with waitFor-based assertions like await waitFor(() =>
expect(mockTerminalSpawn).not.toHaveBeenCalled()) and await waitFor(() =>
expect(mockTerminalStoreState.addTerminal).not.toHaveBeenCalled()), referencing
useTerminalRestore, mockLoadPersistedTerminals, createDefaultTerminal,
mockTerminalSpawn, and mockSetTerminalRestoreInProgress so the test reliably
fails if a fallback terminal is spawned.
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Around line 271-293: The workspace tab is activated but
terminalStore.activeTerminalId is never updated, leaving actions targeting the
old terminal; after determining terminalIdToSelect and calling
workspaceStore.setActiveTab (using selectTerminalForProject,
workspaceStore.getActivePaneLeaf, terminalTabId), also retrieve the terminal
store (e.g. useTerminalStore.getState() or terminalStore) and set its active
terminal to terminalIdToSelect (via the setter or by assigning activeTerminalId)
so the terminal store selection is kept in sync with the restored live tab;
perform this update only when terminalIdToSelect and activePane are present and
after the isCancelled() check.
---
Outside diff comments:
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Around line 298-307: The restore flow in useTerminalRestore still awaits
restoreFromLayout(projectIdToRestore, layout) and
createDefaultTerminal(projectIdToRestore) even after the isCancelled() guard, so
those helpers can continue async work and mutate stores for a stale project;
update restoreFromLayout and createDefaultTerminal to check cancellation (use
the same isCancelled or callId guard) before every store mutation and
immediately after any awaited spawn/long async step, returning early if
cancelled to prevent spawning PTYs or re-adding terminals for the wrong project
in useTerminalRestore.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 129f3f20-26bb-46f8-911e-9d995879e77d
📒 Files selected for processing (3)
src/renderer/hooks/use-editor-persistence.test.tssrc/renderer/hooks/use-terminal-restore.test.tssrc/renderer/hooks/use-terminal-restore.ts
Sync live terminal selection with workspace activation and guard restore helpers against stale async mutations. Tighten restore regression tests so cancelled errors cannot spawn fallback terminals. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update editor persistence expectations to match the current project-switch restore sequence and latest-call behavior. Keep the full Vitest suite green after the terminal restore follow-up changes. 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)
src/renderer/hooks/use-terminal-restore.ts (1)
665-673:⚠️ Potential issue | 🟠 MajorSame PTY leak pattern applies here.
The cancellation check at lines 670-673 has the same issue: if cancelled after spawn succeeds, the PTY is orphaned. Apply the same fix pattern as suggested for
restoreFromLayout.🐛 Proposed fix
const spawnResult = await terminalApi.spawn({ shell, cwd: project?.path }) if (isCancelled()) { debugLog('createDefaultTerminal', `CANCELLED [${defaultId}] after spawn`) + // Clean up the spawned PTY to prevent resource leak + if (spawnResult.success) { + terminalApi.kill(spawnResult.data.id).catch((err) => { + debugLog('createDefaultTerminal', `Failed to kill orphaned PTY on cancel`, { err }) + }) + } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/use-terminal-restore.ts` around lines 665 - 673, After spawning the PTY in createDefaultTerminal (spawnResult from terminalApi.spawn) you must clean up the spawned PTY if isCancelled() returns true to avoid orphaning the PTY; mirror the fix used in restoreFromLayout by calling the terminal cleanup API (e.g. terminalApi.kill or the appropriate dispose/close method) with the identifier returned on spawn (spawnResult.id or spawnResult.pid) before logging the cancellation (debugLog('createDefaultTerminal'...)) and returning.
🧹 Nitpick comments (1)
src/renderer/hooks/use-editor-persistence.test.ts (1)
516-540: Third mock is unused — consider removing for clarity.The test expects exactly 2 calls to
mockPersistenceRead(line 571), but three mocks are chained. The third mock (lines 524-540) is never invoked. While this doesn't affect test correctness, removing it would reduce confusion.♻️ Suggested simplification
mockPersistenceRead .mockImplementationOnce( () => new Promise((resolve) => { projectARead.resolve = resolve }) ) .mockResolvedValueOnce({ success: true, data: null }) - .mockResolvedValueOnce({ - success: true, - data: { - openFiles: [], - activeFilePath: null, - expandedDirs: ['/projects/b'], - fileExplorerVisible: true, - activeTabId: null, - activePaneId: 'pane-b', - paneLayout: { - type: 'leaf', - id: 'pane-b', - tabs: [{ type: 'editor', filePath: '/projects/b/src/index.ts' }], - activeTabId: 'edit-/projects/b/src/index.ts' - } - } - })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/use-editor-persistence.test.ts` around lines 516 - 540, The test chains three responses on mockPersistenceRead but only asserts two calls; remove the unused third stub to avoid confusion by deleting the mockResolvedValueOnce call that returns the full editor state (the block passed to mockPersistenceRead.mockResolvedValueOnce with data containing openFiles, activeFilePath, expandedDirs, paneLayout, etc.), or alternatively update the test's expectation if that third response should be consumed; locate the stub on mockPersistenceRead in use-editor-persistence.test.ts and remove the third mockResolvedValueOnce entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Around line 493-538: The code can orphan a spawned PTY if cancellation occurs
after terminalApi.spawn returns but before the terminal is tracked; after
calling terminalApi.spawn (and before mutating idMap/newTerminals or
incrementing SPAWN_CALL_COUNT), check isCancelled() and if true, synchronously
terminate/cleanup the spawned PTY using the terminalApi method that closes/kills
a PTY (pass spawnResult.data.id), ensure TERMINALS_PENDING_PTY_ASSIGNMENT
cleanup still runs, and then continue the loop without incrementing
SPAWN_CALL_COUNT or adding the terminal; reference resolveShellToPath,
normalizeShellForStartup, terminalApi.spawn, spawnResult.data.id,
SPAWN_CALL_COUNT, idMap, newTerminals, and TERMINALS_PENDING_PTY_ASSIGNMENT when
making the change.
---
Outside diff comments:
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Around line 665-673: After spawning the PTY in createDefaultTerminal
(spawnResult from terminalApi.spawn) you must clean up the spawned PTY if
isCancelled() returns true to avoid orphaning the PTY; mirror the fix used in
restoreFromLayout by calling the terminal cleanup API (e.g. terminalApi.kill or
the appropriate dispose/close method) with the identifier returned on spawn
(spawnResult.id or spawnResult.pid) before logging the cancellation
(debugLog('createDefaultTerminal'...)) and returning.
---
Nitpick comments:
In `@src/renderer/hooks/use-editor-persistence.test.ts`:
- Around line 516-540: The test chains three responses on mockPersistenceRead
but only asserts two calls; remove the unused third stub to avoid confusion by
deleting the mockResolvedValueOnce call that returns the full editor state (the
block passed to mockPersistenceRead.mockResolvedValueOnce with data containing
openFiles, activeFilePath, expandedDirs, paneLayout, etc.), or alternatively
update the test's expectation if that third response should be consumed; locate
the stub on mockPersistenceRead in use-editor-persistence.test.ts and remove the
third mockResolvedValueOnce entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95d479f9-51c4-4271-80a0-a292ba9257c3
📒 Files selected for processing (3)
src/renderer/hooks/use-editor-persistence.test.tssrc/renderer/hooks/use-terminal-restore.test.tssrc/renderer/hooks/use-terminal-restore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/hooks/use-terminal-restore.test.ts
Kill spawned PTYs when project switches cancel terminal restoration so background processes are not orphaned. Add regression coverage for both layout restore and default terminal creation cancellation paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/renderer/hooks/use-terminal-restore.ts (1)
475-478:⚠️ Potential issue | 🟠 MajorClean up already-spawned PTYs on the later cancellation exits too.
The post-spawn kill only covers the PTY from the current iteration. If cancellation happens after one terminal has already been pushed into
newTerminalsand before the loop finishes or beforesetTerminals, these early returns still orphan the earlier PTYs.Also applies to: 548-550
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/use-terminal-restore.ts` around lines 475 - 478, In restoreFromLayout, when isCancelled() returns true you must clean up any PTYs already spawned in this run instead of returning immediately; track spawned PTY objects (e.g., collect them in newTerminals or a temporary spawnedPtys array inside restoreFromLayout) and call their kill/close method before the early return, and apply the same cleanup at the other cancellation exit (the later isCancelled() check around the 548-550 area) so previously pushed terminals are not orphaned; ensure you still log the CANCELLED [restoreId] message after performing the cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Around line 290-292: The code currently always uses
workspaceStore.getActivePaneLeaf() when calling workspaceStore.setActiveTab(...
terminalTabId(...)), which can set an activeTabId on the wrong pane if the
restored terminal lives elsewhere; update the logic to locate the pane/leaf that
actually contains the restored tab (e.g. search workspaceStore leaves for a leaf
whose tabs include terminalTabId(terminalIdToSelect) or call a helper like
workspaceStore.findLeafByTabId if available), then call
workspaceStore.setActiveTab(foundLeaf.id, terminalTabId(terminalIdToSelect));
fall back to getActivePaneLeaf() only if no containing leaf is found. Ensure
this change is tied to the restore flow that calls ensureTerminalTab and uses
terminalTabId and setActiveTab.
- Around line 529-530: SPAWN_CALL_COUNT is only incremented and never
decremented, causing the 50-spawn guard to permanently block restores; change
the approach so SPAWN_CALL_COUNT represents active/spawned-but-not-yet-closed
PTYs (or is reset per restore session): increment SPAWN_CALL_COUNT where
currently done (the SPAWN_CALL_COUNT++ at lines like the shown 529 and the other
occurrence near 704), and decrement it when the corresponding PTY/terminal is
closed or exits (hook into the PTY dispose/exit handler or terminal close
callback), or alternatively reset SPAWN_CALL_COUNT = 0 at the start of the
restore loop or on project switch; ensure both increment sites and the chosen
decrement/reset point are updated so the guard does not permanently block
restores.
- Line 223: The current setTerminalRestoreInProgress usage creates a race where
a cancelled restore effect can clear another effect's in-progress flag; modify
setTerminalRestoreInProgress to accept an owner/token (e.g., a symbol or unique
id) when setting the flag and require that same owner/token when clearing it so
only the effect that acquired the flag can release it; update all callers
(including where setTerminalRestoreInProgress(projectIdToRestore, true) is
invoked and the finally blocks that write false) to pass a generated owner token
from the effect (e.g., const owner = Symbol() or uuid) and verify ownership
inside setTerminalRestoreInProgress before removing the projectId from the Set.
Ensure the token is returned/kept by the effect so its finally uses the same
token to clear the flag.
---
Duplicate comments:
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Around line 475-478: In restoreFromLayout, when isCancelled() returns true you
must clean up any PTYs already spawned in this run instead of returning
immediately; track spawned PTY objects (e.g., collect them in newTerminals or a
temporary spawnedPtys array inside restoreFromLayout) and call their kill/close
method before the early return, and apply the same cleanup at the other
cancellation exit (the later isCancelled() check around the 548-550 area) so
previously pushed terminals are not orphaned; ensure you still log the CANCELLED
[restoreId] message after performing the cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed588df6-9973-4449-8878-50224af54e2e
📒 Files selected for processing (2)
src/renderer/hooks/use-terminal-restore.test.tssrc/renderer/hooks/use-terminal-restore.ts
Comment on lines
+529
to
+530
| // FIX #6: Increment SPAWN_CALL_COUNT for each successful spawn in the loop | ||
| SPAWN_CALL_COUNT++ |
There was a problem hiding this comment.
Don't let the spawn guard turn into a session-lifetime fuse.
SPAWN_CALL_COUNT only grows, but the guard blocks restores once it reaches 50. After enough successful restores/default creations in one session, later project switches stop restoring terminals even if every previous PTY was already closed.
Also applies to: 704-704
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/use-terminal-restore.ts` around lines 529 - 530,
SPAWN_CALL_COUNT is only incremented and never decremented, causing the 50-spawn
guard to permanently block restores; change the approach so SPAWN_CALL_COUNT
represents active/spawned-but-not-yet-closed PTYs (or is reset per restore
session): increment SPAWN_CALL_COUNT where currently done (the
SPAWN_CALL_COUNT++ at lines like the shown 529 and the other occurrence near
704), and decrement it when the corresponding PTY/terminal is closed or exits
(hook into the PTY dispose/exit handler or terminal close callback), or
alternatively reset SPAWN_CALL_COUNT = 0 at the start of the restore loop or on
project switch; ensure both increment sites and the chosen decrement/reset point
are updated so the guard does not permanently block restores.
Track restore ownership so stale effects cannot clear active restore state, reuse the correct pane when reselecting live terminals, and clean up spawned PTYs during cancellation while resetting the restore spawn guard per session. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/renderer/hooks/use-terminal-restore.ts (1)
65-93: Consider parallelizing PTY cleanup for faster cancellation.The sequential
for...ofloop withawaitkills PTYs one at a time. For faster cleanup during rapid project switching, these kills could be parallelized since they're independent operations.♻️ Proposed parallel cleanup
async function cleanupSpawnedPtys( terminals: Array<{ ptyId?: string }>, restoreId: string, phase: string ): Promise<void> { const ptyIds = Array.from( new Set( terminals .map((terminal) => terminal.ptyId) .filter((ptyId): ptyId is string => !!ptyId) ) ) if (ptyIds.length === 0) { return } debugLog('restoreFromLayout', `CLEANING UP SPAWNED PTYS [${restoreId}]`, { phase, ptyIds }) - for (const ptyId of ptyIds) { - const killResult = await terminalApi.kill(ptyId) - if (!killResult.success) { - console.error('Failed to kill cancelled restore PTY:', killResult.error) - } - } + await Promise.all( + ptyIds.map(async (ptyId) => { + const killResult = await terminalApi.kill(ptyId) + if (!killResult.success) { + console.error('Failed to kill cancelled restore PTY:', killResult.error) + } + }) + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/use-terminal-restore.ts` around lines 65 - 93, The cleanupSpawnedPtys function currently kills PTYs sequentially which is slow; change it to run terminalApi.kill calls in parallel by mapping ptyIds to kill promises and awaiting Promise.all (or Promise.allSettled) so all kills execute concurrently, then handle/log failures per pty (using killResult.success or rejected promise reason) to preserve error reporting; reference the function cleanupSpawnedPtys and the terminalApi.kill calls when making this change.src/renderer/hooks/useTerminalAutoSave.test.ts (1)
40-51: Add state cleanup to prevent test pollution.The
setTerminalRestoreInProgresstests mutate module-level state (terminalRestoreProjectsInProgressmap) but don't clean up after themselves. If future tests in this file depend onisTerminalRestoreInProgress()returningfalse, they could fail intermittently depending on test execution order.♻️ Proposed fix to add cleanup
describe('setTerminalRestoreInProgress', () => { + afterEach(() => { + // Clean up any lingering restore state + setTerminalRestoreInProgress('proj-1', false, 'owner-a') + setTerminalRestoreInProgress('proj-1', false, 'owner-b') + }) + it('only clears restore state for the matching owner', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/useTerminalAutoSave.test.ts` around lines 40 - 51, The test mutates module-level terminalRestoreProjectsInProgress via setTerminalRestoreInProgress and doesn't reset it; add cleanup to avoid pollution by clearing/resetting state after each test (or at the end of this `describe`): call the appropriate reset/clear sequence so isTerminalRestoreInProgress() returns false for subsequent tests (e.g., clear terminalRestoreProjectsInProgress or call setTerminalRestoreInProgress for affected owners to false) and ensure this runs in an afterEach hook adjacent to the describe containing setTerminalRestoreInProgress and isTerminalRestoreInProgress.src/renderer/hooks/useTerminalAutoSave.ts (1)
20-22: Consider adding debug logging for unexpected falsy parameters.The silent early return when
projectIdorownerIdis falsy could mask programming errors. Adding a debug log would help diagnose issues during development without affecting production.♻️ Proposed enhancement
export function setTerminalRestoreInProgress( projectId: string, isRestoring: boolean, ownerId: string ): void { if (!projectId || !ownerId) { + if (import.meta.env.DEV) { + console.warn('setTerminalRestoreInProgress called with falsy params:', { projectId, ownerId }) + } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/useTerminalAutoSave.ts` around lines 20 - 22, In useTerminalAutoSave (the useTerminalAutoSave hook) the early return when projectId or ownerId is falsy is silent; update the guard to log a debug message (e.g., using the project's logger or console.debug) that includes which parameter(s) are missing and their values so developers can diagnose unexpected falsy inputs; ensure the log is at a debug/verbose level and keep the existing early return behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Around line 65-93: The cleanupSpawnedPtys function currently kills PTYs
sequentially which is slow; change it to run terminalApi.kill calls in parallel
by mapping ptyIds to kill promises and awaiting Promise.all (or
Promise.allSettled) so all kills execute concurrently, then handle/log failures
per pty (using killResult.success or rejected promise reason) to preserve error
reporting; reference the function cleanupSpawnedPtys and the terminalApi.kill
calls when making this change.
In `@src/renderer/hooks/useTerminalAutoSave.test.ts`:
- Around line 40-51: The test mutates module-level
terminalRestoreProjectsInProgress via setTerminalRestoreInProgress and doesn't
reset it; add cleanup to avoid pollution by clearing/resetting state after each
test (or at the end of this `describe`): call the appropriate reset/clear
sequence so isTerminalRestoreInProgress() returns false for subsequent tests
(e.g., clear terminalRestoreProjectsInProgress or call
setTerminalRestoreInProgress for affected owners to false) and ensure this runs
in an afterEach hook adjacent to the describe containing
setTerminalRestoreInProgress and isTerminalRestoreInProgress.
In `@src/renderer/hooks/useTerminalAutoSave.ts`:
- Around line 20-22: In useTerminalAutoSave (the useTerminalAutoSave hook) the
early return when projectId or ownerId is falsy is silent; update the guard to
log a debug message (e.g., using the project's logger or console.debug) that
includes which parameter(s) are missing and their values so developers can
diagnose unexpected falsy inputs; ensure the log is at a debug/verbose level and
keep the existing early return behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50d291b0-742d-41b3-b812-b89269b79ef1
📒 Files selected for processing (4)
src/renderer/hooks/use-terminal-restore.test.tssrc/renderer/hooks/use-terminal-restore.tssrc/renderer/hooks/useTerminalAutoSave.test.tssrc/renderer/hooks/useTerminalAutoSave.ts
3 tasks
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
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests