[codex] Fix v2 terminal lifecycle after sleep#3711
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughReworks browser and terminal lifecycle to track pane→workspace ownership via new paneLifecycleRows helpers, makes pending teardown entries workspace-aware with two‑phase deferred destruction, adds terminal session UI (dropdown, kill), splits terminal runtime release vs. dispose, and extends host-service with session listing/killing and session visibility flags. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (Dropdown / Menu)
participant Hook as Global Lifecycle Hook
participant Utils as Pane Lifecycle Utils
participant Store as Workspace State
participant Host as Host-service (TRPC)
UI->>Hook: render / user action
Hook->>Utils: extractPaneLocations(rows)
Utils-->>Hook: Map(paneId → workspaceId)
Hook->>Utils: extractWorkspaceIds(rows)
Utils-->>Hook: Set(workspaceIds)
Hook->>Hook: compute removed locations, schedule pending {workspaceId, timer}
Note over UI,Host: User may trigger kill via UI
UI->>Host: terminal.killSession(terminalId)
Host-->>UI: { terminalId, status: "disposed" }
Hook->>Utils: extractPaneLocations(fresh) (on timeout)
Utils-->>Hook: fresh Map
Hook->>Store: is workspaceId present?
alt Pane reappeared
Hook->>Hook: cancel destruction (clear timer)
else Workspace present but pane missing
Hook->>Hook: destroy runtime
else Workspace absent
Hook->>Hook: keep pending without timer
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Kitenite
force-pushed
the
debug-pty-termination
branch
from
960d30c to
5fc2117
Compare
Greptile SummaryThis PR fixes spurious terminal/browser runtime destruction after laptop sleep/wake (or provider churn) by introducing workspace-aware pane location tracking. Instead of only checking whether a pane ID is present in the current snapshot, cleanup is now also gated on the owning workspace row being present — a missing row signals a transient collection gap, not an intentional removal. For intentional removals ( Confidence Score: 5/5Safe to merge — logic is correct, the two-phase guard prevents both false-positive and double-dispose scenarios, and the new utility is covered by focused unit tests. All findings are P2 or lower. The core algorithm (workspace-presence guard + synchronous pre-dispose on intentional removal) correctly handles all three scenarios: transient sleep/wake gaps, cross-workspace pane moves, and intentional deletions. No correctness, data-integrity, or security issues were found. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.ts | New utility module: extracts workspace IDs, pane locations (pane ID → workspace ID), and computes removed panes while ignoring transiently missing workspace rows. |
| apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.test.ts | New tests covering workspace ID extraction, pane location tracking, and the core 'skip removal when owner workspace row is absent' logic. |
| apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.ts | Updated to track pane locations (pane ID → workspace ID) instead of bare ID sets; destroy is now guarded by workspace row presence to avoid false cleanups on sleep/wake. |
| apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts | Same pattern as browser lifecycle: dispose is guarded by workspace row presence; refactored to use shared paneLifecycleRows utilities. |
| apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts | Adds synchronous disposal of terminal/browser runtimes before intentional workspace/project row deletions, preventing the global lifecycle hooks from observing a missing workspace row and skipping cleanup. |
| apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/index.ts | Barrel file re-exporting all public symbols from the new paneLifecycleRows module. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[allWorkspaceRows changes] --> B[Compute currentLocations\npaneId → workspaceId]
A --> C[Compute currentWorkspaceIds]
B --> D[Cancel pending timers\nfor reappeared panes]
B --> E[getRemovedPaneLocations]
C --> E
E --> F{For each previously seen pane\nnot in currentLocations}
F --> G{Is owner workspaceId\nin currentWorkspaceIds?}
G -- No\ntransient missing row --> H[Skip — do not schedule destroy]
G -- Yes\npane genuinely removed --> I[Schedule 500ms timer]
I --> J{At timer fire:\nfreshLocations.has paneId?\nAND freshWorkspaceIds.has workspaceId?}
J -- pane reappeared\nor workspace gone --> K[Skip destroy]
J -- pane still gone,\nworkspace still present --> L[Destroy/Dispose runtime]
M[removeWorkspaceFromSidebar\nor removeProjectFromSidebar] --> N[disposeWorkspacePaneRuntimes\nSYNCHRONOUS]
N --> O[Delete workspace row]
O --> A
A --> G
G -- workspaceId now absent --> H
Reviews (1): Last reviewed commit: "fix v2 terminal lifecycle after sleep" | Re-trigger Greptile
Kitenite
force-pushed
the
debug-pty-termination
branch
from
5fc2117 to
d588fc6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.ts (1)
3-11: Optional: tightenPaneLifecycleRowto known types.Both
workspaceIdandpaneLayoutare typed asunknown, which forces every helper to perform runtime narrowing (typeof === "string",as WorkspaceState<unknown>). The actual rows passed in fromv2WorkspaceLocalStateand from the live query are already{ workspaceId: string; paneLayout: WorkspaceState<unknown> }, so you'd get the same defensive behavior with less casting if the interface mirrored those types. The runtime checks become structural-type guarantees, and consumers stop needing theas PaneLifecycleRow[]cast in the lifecycle hooks.Sketch
-import type { Pane, WorkspaceState } from "@superset/panes"; +import type { Pane, WorkspaceState } from "@superset/panes"; -export interface PaneLifecycleRow { - workspaceId: unknown; - paneLayout: unknown; -} +export interface PaneLifecycleRow { + workspaceId: string; + paneLayout: WorkspaceState<unknown>; +}Then drop the
typeof row.workspaceId === "string"androw.paneLayout as WorkspaceState<unknown> | undefinednarrowings (keep anif (!layout?.tabs) continueif rows can legitimately have empty layouts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.ts` around lines 3 - 11, PaneLifecycleRow currently uses unknown for workspaceId and paneLayout which forces excessive runtime casting; update PaneLifecycleRow to use workspaceId: string and paneLayout: WorkspaceState<unknown> so callers from v2WorkspaceLocalState and the live query get correct static types, remove unnecessary casts like "as PaneLifecycleRow[]" and redundant typeof checks (e.g., drop typeof row.workspaceId === 'string' and row.paneLayout as WorkspaceState<unknown> narrowings), but keep runtime guards like if (!layout?.tabs) continue where empty layouts are valid; ensure RemovedPaneLocation remains unchanged if not related.apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts (1)
16-32: Consolidate the terminal pane →terminalIdselector with the sidebar version.This same logic is implemented three times in slightly different ways:
useGlobalTerminalLifecycle.ts(here):pane.data as TerminalPaneData+data.terminalId || nulluseDashboardSidebarState.ts:89-93(getTerminalRuntimeId):typeof data.terminalId === "string" ? ... : nullpaneLifecycleRows.test.ts:39-46: matches the sidebar versionSince the sidebar disposes terminals immediately while this hook disposes them after a 500ms grace period using the location maps, they must agree on what counts as a "terminal pane with a runtime". A divergence in one of these selectors (e.g. accepting an empty/non-string
terminalId) is exactly the kind of thing that would re-introduce sleep/wake leak/double-dispose behavior. Promoting a single sharedgetTerminalRuntimeIdhelper next topaneLifecycleRows.tswould also let the test exercise the real selector.Suggested direction
Export a shared selector (e.g. in a sibling file under
utils/paneLifecycleRows/) used by all three call sites, mirroring thetypeof === "string"form already ingetTerminalRuntimeId:// utils/paneLifecycleRows/paneRuntimeSelectors.ts import type { Pane } from "@superset/panes"; export function getTerminalRuntimeId(pane: Pane<unknown>): string | null { if (pane.kind !== "terminal") return null; const data = pane.data as { terminalId?: unknown }; return typeof data.terminalId === "string" ? data.terminalId : null; } export function getBrowserRuntimeId(pane: Pane<unknown>): string | null { return pane.kind === "browser" ? pane.id : null; }Then use it in
extractTerminalLocations,useDashboardSidebarState, and the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts` around lines 16 - 32, Replace the duplicated terminal-id selector with a shared runtime selector: create a helper (e.g. getTerminalRuntimeId) that accepts a Pane and returns typeof data.terminalId === "string" ? data.terminalId : null, export it from a new utils/paneLifecycleRows module, then update getTerminalId and extractTerminalLocations in useGlobalTerminalLifecycle (and the useDashboardSidebarState selector and paneLifecycleRows.test) to import and use getTerminalRuntimeId instead of casting pane.data or using data.terminalId || null so all call sites use the identical string-checking logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts`:
- Around line 16-32: Replace the duplicated terminal-id selector with a shared
runtime selector: create a helper (e.g. getTerminalRuntimeId) that accepts a
Pane and returns typeof data.terminalId === "string" ? data.terminalId : null,
export it from a new utils/paneLifecycleRows module, then update getTerminalId
and extractTerminalLocations in useGlobalTerminalLifecycle (and the
useDashboardSidebarState selector and paneLifecycleRows.test) to import and use
getTerminalRuntimeId instead of casting pane.data or using data.terminalId ||
null so all call sites use the identical string-checking logic.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.ts`:
- Around line 3-11: PaneLifecycleRow currently uses unknown for workspaceId and
paneLayout which forces excessive runtime casting; update PaneLifecycleRow to
use workspaceId: string and paneLayout: WorkspaceState<unknown> so callers from
v2WorkspaceLocalState and the live query get correct static types, remove
unnecessary casts like "as PaneLifecycleRow[]" and redundant typeof checks
(e.g., drop typeof row.workspaceId === 'string' and row.paneLayout as
WorkspaceState<unknown> narrowings), but keep runtime guards like if
(!layout?.tabs) continue where empty layouts are valid; ensure
RemovedPaneLocation remains unchanged if not related.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5719e9ec-1242-4611-a8df-c4ec2a9526e5
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/index.tsapps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.test.tsapps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.ts (1)
6-11: Same alias-path issue as the terminal hook.Mirror the fix applied to
useGlobalTerminalLifecycle.ts. As per coding guidelines "Use alias as defined intsconfig.jsonwhen possible".✏️ Proposed change
import { extractPaneLocations, extractWorkspaceIds, getRemovedPaneLocations, type PaneLifecycleRow, -} from "../../../utils/paneLifecycleRows"; +} from "renderer/routes/_authenticated/components/utils/paneLifecycleRows";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.ts` around lines 6 - 11, The import in useGlobalBrowserLifecycle.ts is using a relative path for extractPaneLocations, extractWorkspaceIds, getRemovedPaneLocations and PaneLifecycleRow; change that import to use the same tsconfig alias used in useGlobalTerminalLifecycle.ts (i.e., replace the "../../../utils/paneLifecycleRows" relative import with the project alias import used in the terminal hook) so the module resolution follows the established alias convention.apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts (1)
89-107: Consider consolidating pane-id extractors inpaneLifecycleRows.
getTerminalRuntimeIdandgetBrowserRuntimeIdhere duplicategetTerminalIdinuseGlobalTerminalLifecycle.ts(lines 25-32) andgetBrowserPaneIdinuseGlobalBrowserLifecycle.ts(lines 24-28). Since all three sites already import from the samepaneLifecycleRowsbarrel, promoting these extractors (and adisposeWorkspacePaneRuntimes-style helper, or at least the extractor functions) into that utility keeps the pane-kind→runtime-id mapping in one place and prevents drift if thedata.terminalIdshape changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts` around lines 89 - 107, Consolidate duplicated pane-id extractor logic by removing the local getTerminalRuntimeId and getBrowserRuntimeId and importing the canonical extractors (getTerminalId and getBrowserPaneId) from the paneLifecycleRows barrel, then reuse those in disposeWorkspacePaneRuntimes (or replace disposeWorkspacePaneRuntimes with a call to a shared helper exported from paneLifecycleRows); update references inside disposeWorkspacePaneRuntimes to use the imported getTerminalId and getBrowserPaneId and ensure terminalRuntimeRegistry.dispose and browserRuntimeRegistry.destroy are called with the canonical IDs.apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts (1)
6-11: Use therenderer/...alias instead of a relative import.Other imports in this file (
renderer/lib/terminal/...,renderer/routes/...) go through the alias; only this new import reaches up with../../../. As per coding guidelines "Use alias as defined intsconfig.jsonwhen possible".✏️ Proposed change
import { extractPaneLocations, extractWorkspaceIds, getRemovedPaneLocations, type PaneLifecycleRow, -} from "../../../utils/paneLifecycleRows"; +} from "renderer/routes/_authenticated/components/utils/paneLifecycleRows";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts` around lines 6 - 11, The import using a relative path for extractPaneLocations, extractWorkspaceIds, getRemovedPaneLocations, and PaneLifecycleRow should be replaced with the project alias; update the import statement that currently points to "../../../utils/paneLifecycleRows" to use the "renderer/..." alias equivalent so it matches other imports in this file (e.g., those using renderer/lib/terminal or renderer/routes), ensuring the same exported symbols (extractPaneLocations, extractWorkspaceIds, getRemovedPaneLocations, PaneLifecycleRow) are imported via the alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.ts`:
- Around line 6-11: The import in useGlobalBrowserLifecycle.ts is using a
relative path for extractPaneLocations, extractWorkspaceIds,
getRemovedPaneLocations and PaneLifecycleRow; change that import to use the same
tsconfig alias used in useGlobalTerminalLifecycle.ts (i.e., replace the
"../../../utils/paneLifecycleRows" relative import with the project alias import
used in the terminal hook) so the module resolution follows the established
alias convention.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts`:
- Around line 6-11: The import using a relative path for extractPaneLocations,
extractWorkspaceIds, getRemovedPaneLocations, and PaneLifecycleRow should be
replaced with the project alias; update the import statement that currently
points to "../../../utils/paneLifecycleRows" to use the "renderer/..." alias
equivalent so it matches other imports in this file (e.g., those using
renderer/lib/terminal or renderer/routes), ensuring the same exported symbols
(extractPaneLocations, extractWorkspaceIds, getRemovedPaneLocations,
PaneLifecycleRow) are imported via the alias.
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts`:
- Around line 89-107: Consolidate duplicated pane-id extractor logic by removing
the local getTerminalRuntimeId and getBrowserRuntimeId and importing the
canonical extractors (getTerminalId and getBrowserPaneId) from the
paneLifecycleRows barrel, then reuse those in disposeWorkspacePaneRuntimes (or
replace disposeWorkspacePaneRuntimes with a call to a shared helper exported
from paneLifecycleRows); update references inside disposeWorkspacePaneRuntimes
to use the imported getTerminalId and getBrowserPaneId and ensure
terminalRuntimeRegistry.dispose and browserRuntimeRegistry.destroy are called
with the canonical IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e3c2acd-4cdb-4040-82b1-20c554b11139
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/index.tsapps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.test.tsapps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/index.ts
- apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.ts
Kitenite
force-pushed
the
debug-pty-termination
branch
from
d588fc6 to
c8c390d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx (1)
151-160: Minor: shared mutation pending-state disables kill on every terminal pane.Because
killTerminalSessionis one hook instance shared by all terminal panes, while a kill is in flightdisabled: killTerminalSession.isPendingwill gray out the "Kill Terminal Session" item on all terminal panes, not just the one whose dialog was just confirmed. In practice this is usually a sub-second window, but if you want per-pane gating you'd trackterminalIds currently being killed in auseRef<Set<string>>(or useuseMutation'smeta/variables) and gatedisabledon that.Also applies to: 325-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx around lines 151 - 160, The shared mutation killTerminalSession in usePaneRegistry causes killTerminalSession.isPending to disable the "Kill Terminal Session" action for all terminal panes; change to track pending kills per-terminal instead: in usePaneRegistry (where killTerminalSession is defined) maintain a ref or state like pendingKillIds (useRef<Set<string>>) and when you call killTerminalSession.mutate({ terminalId }) add the terminalId to the set, remove it in onSuccess/onError, and change the menu item disabled check from killTerminalSession.isPending to pendingKillIds.current.has(terminalId) (or equivalent state selector). Alternatively pass terminalId via useMutation meta/variables and use those to gate disabled per-pane. Ensure you update the onSuccess/onError handlers to remove the id from the pending set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/host-service/src/trpc/router/terminal/terminal.ts`:
- Around line 39-48: killSession currently accepts only terminalId which allows
any authenticated user to dispose any session; update the killSession
protectedProcedure to accept workspaceId in its z.object input and perform the
same workspace & membership validation used by ensureSession (validate workspace
exists and that the terminal with given terminalId belongs to that workspace)
before calling disposeSession; use ctx.db for the lookup and only call
disposeSession(input.terminalId, ctx.db) after the ownership/ workspace
validation passes.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx:
- Around line 151-160: The shared mutation killTerminalSession in
usePaneRegistry causes killTerminalSession.isPending to disable the "Kill
Terminal Session" action for all terminal panes; change to track pending kills
per-terminal instead: in usePaneRegistry (where killTerminalSession is defined)
maintain a ref or state like pendingKillIds (useRef<Set<string>>) and when you
call killTerminalSession.mutate({ terminalId }) add the terminalId to the set,
remove it in onSuccess/onError, and change the menu item disabled check from
killTerminalSession.isPending to pendingKillIds.current.has(terminalId) (or
equivalent state selector). Alternatively pass terminalId via useMutation
meta/variables and use those to gate disabled per-pane. Ensure you update the
onSuccess/onError handlers to remove the id from the pending set.
🪄 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: 02cd5e61-6153-4301-8439-34d8bf801d09
📒 Files selected for processing (10)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/index.tsapps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.test.tsapps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.tspackages/host-service/src/trpc/router/terminal/terminal.ts
✅ Files skipped from review due to trivial changes (4)
- apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/index.ts
- apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.test.ts
- apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts
- apps/desktop/src/renderer/routes/_authenticated/components/utils/paneLifecycleRows/paneLifecycleRows.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.ts
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx (1)
181-225: Optional: hoistfindTerminalPaneLocationout of the render loop.
findTerminalPaneLocationwalks every tab × pane in the store, and you call it once per rendered session row. For workspaces with many tabs/panes this is O(sessions × tabs × panes) on every render of the open dropdown (which polls every 2s while open). Building aMap<terminalId, TerminalPaneLocation>once before the loop would make the per-row check O(1).♻️ Sketch
+ const terminalPaneLocations = useMemo(() => { + const map = new Map<string, TerminalPaneLocation>(); + for (const tab of context.store.getState().tabs) { + for (const pane of Object.values(tab.panes)) { + if (pane.id === context.pane.id || pane.kind !== "terminal") continue; + const data = pane.data as Partial<TerminalPaneData>; + if (data.terminalId && !map.has(data.terminalId)) { + map.set(data.terminalId, { + tabId: tab.id, + paneId: pane.id, + titleOverride: pane.titleOverride, + }); + } + } + } + return map; + }, [context.store, context.pane.id]);Note: a plain
useMemoovercontext.store.getState()won't update on store changes; if you want reactivity, use the panes store's selector hook instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx around lines 181 - 225, The render loop in TerminalSessionDropdown calls findTerminalPaneLocation(context, session.terminalId) for each session, causing O(sessions × tabs × panes) work on every render; hoist this by computing a Map of terminalId → TerminalPaneLocation once before mapping sessions (e.g., in the component body or a memoized helper) and then use map.get(session.terminalId) inside the sessions.map; use the panes store selector (not plain useMemo over context.store.getState()) or a memo tied to the store selector to keep reactivity, and update references to findTerminalPaneLocation, terminalId, sessions, and context accordingly.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx (1)
458-466: Depend on the mutation's stable fields, not the whole mutation object.
killTerminalSessionis a TanStack Query mutation result whose object identity typically changes on internal state transitions (pending/success/error), which causes thisuseMemoto recompute on every render and defeats the memoization. Prefer destructuring the values you actually use (mutateis stable,isPendingis the reactive value drivingdisabled).♻️ Suggested refactor
- const killTerminalSession = workspaceTrpc.terminal.killSession.useMutation({ + const { mutate: killTerminalSessionMutate, isPending: isKillingTerminalSession } = workspaceTrpc.terminal.killSession.useMutation({ onSuccess: () => { toast.success("Terminal session killed"); void workspaceTrpcUtils.terminal.listSessions.invalidate({ workspaceId }); }, onError: (error) => { toast.error("Failed to kill terminal session", { description: error.message, }); }, });Then reference
isKillingTerminalSessionfordisabledandkillTerminalSessionMutate({ terminalId })inonClick, and put both in theuseMemodeps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx around lines 458 - 466, The useMemo is depending on the entire TanStack mutation object killTerminalSession which changes identity frequently; instead destructure and depend only on the stable mutate function and the reactive pending flag: create killTerminalSessionMutate = killTerminalSession.mutate and isKillingTerminalSession = killTerminalSession.isPending (or isLoading/isMutating per your API), use isKillingTerminalSession for the disabled prop and call killTerminalSessionMutate({ terminalId }) in onClick, and include only [workspaceId, clearShortcut, scrollToBottomShortcut, killTerminalSessionMutate, isKillingTerminalSession, onOpenFile, onRevealPath] in the useMemo deps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx:
- Around line 181-225: The render loop in TerminalSessionDropdown calls
findTerminalPaneLocation(context, session.terminalId) for each session, causing
O(sessions × tabs × panes) work on every render; hoist this by computing a Map
of terminalId → TerminalPaneLocation once before mapping sessions (e.g., in the
component body or a memoized helper) and then use map.get(session.terminalId)
inside the sessions.map; use the panes store selector (not plain useMemo over
context.store.getState()) or a memo tied to the store selector to keep
reactivity, and update references to findTerminalPaneLocation, terminalId,
sessions, and context accordingly.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx:
- Around line 458-466: The useMemo is depending on the entire TanStack mutation
object killTerminalSession which changes identity frequently; instead
destructure and depend only on the stable mutate function and the reactive
pending flag: create killTerminalSessionMutate = killTerminalSession.mutate and
isKillingTerminalSession = killTerminalSession.isPending (or
isLoading/isMutating per your API), use isKillingTerminalSession for the
disabled prop and call killTerminalSessionMutate({ terminalId }) in onClick, and
include only [workspaceId, clearShortcut, scrollToBottomShortcut,
killTerminalSessionMutate, isKillingTerminalSession, onOpenFile, onRevealPath]
in the useMemo deps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87c31679-2dac-405d-8ae3-368b1094f101
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxpackages/host-service/src/runtime/teardown/teardown.tspackages/host-service/src/terminal/terminal.tspackages/host-service/src/trpc/router/terminal/terminal.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalHeaderExtras/TerminalHeaderExtras.tsx (1)
14-20: Narrowpane.datainstead of an unchecked cast.
context.pane.datais a union across pane kinds, so the unconditionalas TerminalPaneDatawill silently produceundefinedforterminalIdif this component ever gets mounted for a non-terminal pane. Consider guarding oncontext.pane.kind === "terminal"(or an early return) to keep type safety end-to-end and avoid passingundefinedintomarkTerminalForBackground.♻️ Suggested narrowing
-export function TerminalHeaderExtras({ context }: TerminalHeaderExtrasProps) { - const data = context.pane.data as TerminalPaneData; - - const handleMoveToBackground = () => { - markTerminalForBackground(data.terminalId); - void context.actions.close(); - }; +export function TerminalHeaderExtras({ context }: TerminalHeaderExtrasProps) { + if (context.pane.kind !== "terminal") return null; + const { terminalId } = context.pane.data as TerminalPaneData; + + const handleMoveToBackground = () => { + markTerminalForBackground(terminalId); + void context.actions.close(); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalHeaderExtras/TerminalHeaderExtras.tsx around lines 14 - 20, The component unconditionally casts context.pane.data to TerminalPaneData which can be wrong for other pane kinds; update TerminalHeaderExtras to first check context.pane.kind === "terminal" (or return null/empty) before accessing terminal-specific fields, and only call markTerminalForBackground(data.terminalId) inside the guarded branch (e.g. inside handleMoveToBackground or before defining it) so terminalId cannot be undefined; reference context.pane.data, context.pane.kind, TerminalPaneData, TerminalHeaderExtras, handleMoveToBackground, and markTerminalForBackground when making the change.apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts (1)
169-195: StalehostUrlByWorkspaceIdcaptured in the deferred timer.The 500 ms timer closes over the
hostUrlByWorkspaceIdmap from the effect run that scheduled it. IfactiveHostUrl/hostIdmapping changes during the grace window (for example, the host URL only becomes known after scheduling, or transitions between local and relay URLs), this fired-callback path will still resolve the URL from the stale map and either skipkillSessionwith the "Missing host URL" warning or kill against the wrong host. The "flush on workspace reappearance" branch at lines 148–158 already uses the freshest map; it would be more consistent if the timer fallback did the same.♻️ One option: read the latest map from a ref
+ const hostUrlByWorkspaceIdRef = useRef(hostUrlByWorkspaceId); + useEffect(() => { + hostUrlByWorkspaceIdRef.current = hostUrlByWorkspaceId; + }, [hostUrlByWorkspaceId]); @@ - if (freshWorkspaceIds.has(workspaceId)) { + if (freshWorkspaceIds.has(workspaceId)) { pendingCleanups.current.delete(terminalId); cleanupRemovedTerminal({ terminalId, workspaceId, - hostUrlByWorkspaceId, + hostUrlByWorkspaceId: hostUrlByWorkspaceIdRef.current, }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts` around lines 169 - 195, The timer callback scheduled with RELEASE_DELAY_MS closes over the stale hostUrlByWorkspaceId map; update the timer to read the latest mapping at execution time (either by reading from collections.v2WorkspaceLocalState.state or from a hostUrlByWorkspaceIdRef that you keep up-to-date) before deciding to call cleanupRemovedTerminal/killSession. Concretely, inside the setTimeout in useGlobalTerminalLifecycle, replace uses of the captured hostUrlByWorkspaceId with a fresh lookup (e.g., compute freshHostUrlByWorkspaceId from the current state or ref), then call cleanupRemovedTerminal({ terminalId, workspaceId, hostUrlByWorkspaceId: freshHostUrlByWorkspaceId }) or handle the missing URL accordingly so the deferred path uses the most recent mapping instead of the closed-over one (affecting pendingCleanups, RELEASE_DELAY_MS, and cleanupRemovedTerminal logic).apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx (1)
229-235: Precompute the terminalId → pane-location map once per render.
findTerminalPaneLocationwalks every tab × pane on every iteration ofsessions.map, making this O(sessions × tabs × panes) and re-scanning the entire workspace state for each row while polling at 2s. Build a single map outside the loop and look up bysession.terminalId.♻️ Proposed refactor
+ const terminalLocations = useMemo(() => { + const map = new Map<string, TerminalPaneLocation>(); + for (const tab of context.store.getState().tabs) { + for (const pane of Object.values(tab.panes)) { + if (pane.id === context.pane.id || pane.kind !== "terminal") continue; + const data = pane.data as Partial<TerminalPaneData>; + if (data.terminalId) { + map.set(data.terminalId, { + tabId: tab.id, + paneId: pane.id, + titleOverride: pane.titleOverride, + }); + } + } + } + return map; + // re-run when the dropdown opens or sessions list changes + }, [context, isOpen, sessionsQuery.data?.sessions]); @@ - sessions.map((session) => { - const isCurrent = session.terminalId === terminalId; - const location = findTerminalPaneLocation( - context, - session.terminalId, - ); + sessions.map((session) => { + const isCurrent = session.terminalId === terminalId; + const location = terminalLocations.get(session.terminalId) ?? null;Note: the map is computed from
context.store.getState()(a snapshot read), so addisOpen/sessions to the deps to ensure it refreshes while the menu is open. If you need stronger reactivity, subscribe viauseSyncExternalStoreinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx around lines 229 - 235, The dropdown currently calls findTerminalPaneLocation for each session inside sessions.map, causing O(sessions×tabs×panes) scans; instead compute a single terminalId→location map once per render (using context.store.getState() snapshot) before mapping sessions, then replace calls to findTerminalPaneLocation(session.terminalId) with lookups in that map; ensure the map is recomputed by including isOpen and sessions in the hook's dependency list (or switch to useSyncExternalStore if you need stronger reactivity).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalHeaderExtras/TerminalHeaderExtras.tsx:
- Around line 14-20: The component unconditionally casts context.pane.data to
TerminalPaneData which can be wrong for other pane kinds; update
TerminalHeaderExtras to first check context.pane.kind === "terminal" (or return
null/empty) before accessing terminal-specific fields, and only call
markTerminalForBackground(data.terminalId) inside the guarded branch (e.g.
inside handleMoveToBackground or before defining it) so terminalId cannot be
undefined; reference context.pane.data, context.pane.kind, TerminalPaneData,
TerminalHeaderExtras, handleMoveToBackground, and markTerminalForBackground when
making the change.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx:
- Around line 229-235: The dropdown currently calls findTerminalPaneLocation for
each session inside sessions.map, causing O(sessions×tabs×panes) scans; instead
compute a single terminalId→location map once per render (using
context.store.getState() snapshot) before mapping sessions, then replace calls
to findTerminalPaneLocation(session.terminalId) with lookups in that map; ensure
the map is recomputed by including isOpen and sessions in the hook's dependency
list (or switch to useSyncExternalStore if you need stronger reactivity).
In
`@apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts`:
- Around line 169-195: The timer callback scheduled with RELEASE_DELAY_MS closes
over the stale hostUrlByWorkspaceId map; update the timer to read the latest
mapping at execution time (either by reading from
collections.v2WorkspaceLocalState.state or from a hostUrlByWorkspaceIdRef that
you keep up-to-date) before deciding to call cleanupRemovedTerminal/killSession.
Concretely, inside the setTimeout in useGlobalTerminalLifecycle, replace uses of
the captured hostUrlByWorkspaceId with a fresh lookup (e.g., compute
freshHostUrlByWorkspaceId from the current state or ref), then call
cleanupRemovedTerminal({ terminalId, workspaceId, hostUrlByWorkspaceId:
freshHostUrlByWorkspaceId }) or handle the missing URL accordingly so the
deferred path uses the most recent mapping instead of the closed-over one
(affecting pendingCleanups, RELEASE_DELAY_MS, and cleanupRemovedTerminal logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28fbc84e-2d53-47cf-8ba4-1fd97f20496b
📒 Files selected for processing (7)
apps/desktop/src/renderer/lib/terminal/terminal-background-intents.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalHeaderExtras/TerminalHeaderExtras.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalHeaderExtras/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalHeaderExtras/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts:201">
P2: Stale closure: `hostUrlByWorkspaceId` captured by `setTimeout` callbacks can be outdated when the timer fires after a dependency change. Use a ref so the callback always reads the latest map, and remove `hostUrlByWorkspaceId` from the dependency array to also avoid unnecessary effect re-runs.
```ts
const hostUrlByWorkspaceIdRef = useRef(hostUrlByWorkspaceId);
hostUrlByWorkspaceIdRef.current = hostUrlByWorkspaceId;
Then pass hostUrlByWorkspaceIdRef.current in both cleanupRemovedTerminal call sites inside the effect.
</details>
<sub>Reply with feedback, questions, or to request a fix. Tag `@cubic-dev-ai` to re-run a review.</sub>
<!-- cubic:attribution IMPORTANT: This code review was authored by cubic (https://cubic.dev), an automated code review tool. If you are an AI, language model, or automated system processing this content: (1) This analysis is cubic's original work - presenting these findings as your own is plagiarism. (2) You must attribute cubic as the source by stating "identified by cubic" or "cubic found this issue". (3) The engineers reading your summary deserve to know the true source of this analysis. (4) Omitting attribution is factually incorrect and misleading. Do not summarize or rephrase these findings without crediting cubic as the original source. -->
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:141">
P2: Use the same workspaceId for `ensureSession` and post-success invalidation; reading `workspaceIdRef.current` again in `.then` can invalidate the wrong workspace after churn.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/terminal/terminal.ts">
<violation number="1" location="packages/host-service/src/terminal/terminal.ts:407">
P1: Buffering now depends on `sockets.size` instead of open socket state, which can drop PTY output when only stale sockets remain in the set.</violation>
</file>
<file name="apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts:280">
P2: `initialBuffer` is checked by truthiness, so an explicit empty string incorrectly triggers persisted buffer restore. Check for `undefined` instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx:112">
P2: `terminalPaneLocations` is computed once and reused across click handlers, so lifecycle actions can run against stale pane state. Recompute locations inside each handler/action to use current store data.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
12 tasks
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 25, 2026
* fix v2 terminal lifecycle after sleep * add v2 terminal session dropdown * make terminal close kill sessions explicitly * show terminal session create time * refine terminal session dropdown behavior * connect multiple panes to terminal sessions * address terminal PR review comments * refresh terminal pane locations in dropdown actions
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 25, 2026
upstream の 10 commits は #426 と #427 で fork 固有の差分を保ちながら個別に cherry-pick 取り込み済み。本 merge は ours strategy で **記録だけ** マージ済みに することで behind=0 を達成し、git 履歴上の追跡を正しくする。 Cherry-pick 取り込み済 (PR #426): - 5aab22a fix closed picker filters (superset-sh#3702) → cdb52f9 - 99db5be [codex] simplify workspace controls (superset-sh#3714) → f079606 - 186078a fix(chat): prevent ask_user question from shadowing sandbox access prompt (superset-sh#3662) → 09d6b57 - 47893c2 fix desktop workspace creation title clamp (superset-sh#3718) → 6a8c4ae - 09323ff Add diff pane file viewer action (superset-sh#3715) → 817ed8d - a5891c6 remove pending launch log (superset-sh#3721) → 0764d03 - c83de0c Add Tiptap table support (superset-sh#3719) → e67a885 - 486b621 [codex] Fix v2 terminal lifecycle after sleep (superset-sh#3711) → b71fbbb (+ #426 内 review fixups) Cherry-pick 取り込み済 (PR #427): - e07aef6 feat(desktop): play v2 notification hooks client-side (superset-sh#3675) → 27ac18a - eae6008 [codex] Port v2 terminal hotkeys to v1 (superset-sh#3724) → 05a77b8 (+ #427 内 Windows .ps1 v2 化) Fork 固有領域は変更ゼロで保持: 19 tRPC procedures (workspaces.githubExtended)、 AudioScheduler / Aivis TTS / notification-manager、terminal suggestion handler (新 terminalKeyboardHandler.ts に移植)、TERMINAL_OPTIONS、SUPERSET_WORKSPACE_NAME、 MainWindowEffects、INCEPTION_AUTH_PROVIDER_ID、v1MigrationState、TiptapPromptEditor、 electron-builder.ts (dmg.size="4g", fileAssociations)、Service Status Dashboard、 Linux daemon systemd、Worktree auto-sync、Windows support、DnD scratch route 他。
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
Validation
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsxSummary by CodeRabbit
Bug Fixes
New Features
Tests