feat(desktop): decouple terminalId from paneId#3137
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR refactors the terminal subsystem to use Changes
Sequence Diagram(s)sequenceDiagram
participant Client as TerminalPane<br/>(Frontend)
participant Registry as TerminalRuntime<br/>Registry
participant WS as WebSocket<br/>Server
participant Service as Terminal<br/>Service
participant DB as Database
Client->>Registry: attach(terminalId, container, wsUrl)
Registry->>Registry: getOrCreate(terminalId)
Registry->>Service: createRuntime(terminalId)
Service->>DB: Check terminalSessions table
alt Session exists
Service->>DB: Update lastAttachedAt
else Session not exists
Service->>DB: Insert new terminal_sessions<br/>status='active'
end
Service-->>Registry: TerminalRuntime
Client->>WS: Connect /terminal/{terminalId}
WS->>Service: Attach socket to session
WS->>DB: Update lastAttachedAt
Client->>Registry: detach(terminalId)
Registry->>Service: Detach & buffer output
Client->>Registry: dispose(terminalId)
Registry->>Service: disposeRuntime(terminalId)
Service->>DB: Update status='disposed'<br/>endedAt=now()
Service->>Registry: Cleanup in-memory session
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR decouples terminal session identity ( Key areas of the change:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer (TerminalPane)
participant HS as Host-Service
participant DB as SQLite (terminal_sessions)
participant PTY as node-pty
R->>HS: POST /terminal/sessions {terminalId, workspaceId}
HS->>DB: INSERT terminal_sessions (id=terminalId, status="active")
HS->>PTY: spawn(shell, cwd)
PTY-->>HS: pty instance
HS-->>R: 200 {terminalId, status: "active"}
R->>HS: WS /terminal/:terminalId (upgrade)
HS->>DB: UPDATE last_attached_at
HS-->>R: replay buffered output
loop PTY data
PTY-->>HS: onData(chunk)
HS-->>R: {type:"data", data:chunk}
end
R->>HS: {type:"resize", cols, rows}
HS->>PTY: pty.resize(cols, rows)
alt WebSocket closes (host-service restart)
R->>R: connectionState="closed" → startCreateSession()
R->>HS: POST /terminal/sessions (upsert: status="active")
HS->>PTY: spawn new PTY
R->>HS: WS /terminal/:terminalId (re-attach)
end
alt Pane closed / terminal disposed
R->>HS: {type:"dispose"}
HS->>PTY: pty.kill()
HS->>DB: UPDATE status="disposed", ended_at=now()
HS->>R: ws.close()
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+91
to
+98
| if (layout !== EMPTY_STATE && migrateTerminalPaneData(layout)) { | ||
| // Persist the migrated layout back so the migration only runs once | ||
| if (collections.v2WorkspaceLocalState.get(workspaceId)) { | ||
| collections.v2WorkspaceLocalState.update(workspaceId, (draft) => { | ||
| draft.paneLayout = layout; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
useMemo is a React anti-pattern
collections.v2WorkspaceLocalState.update(...) is a mutation/side effect being performed inside useMemo. React explicitly treats useMemo as a pure computation — it may call the function multiple times (notably, React 18 Strict Mode double-invokes it in development), skip the call, or re-run it at will. Running a persistence write inside useMemo means it can fire more than once unexpectedly.
The migration persist should be moved into a useEffect:
// useMemo stays pure — just compute and return the migrated layout
const persistedPaneLayout = useMemo(() => {
const layout = (localWorkspaceState?.paneLayout as WorkspaceState<PaneViewerData> | undefined) ?? EMPTY_STATE;
if (layout !== EMPTY_STATE) {
migrateTerminalPaneData(layout); // mutates in place, returns bool
}
return layout;
}, [localWorkspaceState]);
// Side effect: persist migration result exactly once
const migrationPersistedRef = useRef(false);
useEffect(() => {
if (migrationPersistedRef.current) return;
if (persistedPaneLayout === EMPTY_STATE) return;
// Only persist if migration actually ran (check for absence of terminalId would need re-checking here)
if (collections.v2WorkspaceLocalState.get(workspaceId)) {
migrationPersistedRef.current = true;
collections.v2WorkspaceLocalState.update(workspaceId, (draft) => {
draft.paneLayout = persistedPaneLayout;
});
}
}, [persistedPaneLayout, collections, workspaceId]);
Collaborator
Author
There was a problem hiding this comment.
Resolved — code removed/restructured.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![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: 2
🧹 Nitpick comments (4)
packages/host-service/src/terminal/terminal.ts (2)
297-345: Defensive null coalescing is unnecessary.On lines 298, 315, 334, and 341,
terminalId ?? ""is used, butterminalIdis always defined since it comes fromc.req.param("terminalId")which returnsstringfor declared route params. The fallback to empty string is unreachable and could mask issues if the param extraction ever changes.✨ Remove unnecessary null coalescing
onMessage: (event, ws) => { - const session = sessions.get(terminalId ?? ""); + const session = sessions.get(terminalId); // ... if (message.type === "dispose") { - disposeSession(terminalId ?? "", db); + disposeSession(terminalId, db); return; } // ... }, onClose: (_event, ws) => { - const session = sessions.get(terminalId ?? ""); + const session = sessions.get(terminalId); // ... }, onError: (_event, ws) => { - const session = sessions.get(terminalId ?? ""); + const session = sessions.get(terminalId); // ... },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/terminal/terminal.ts` around lines 297 - 345, The code uses unnecessary null coalescing for terminalId (e.g., sessions.get(terminalId ?? ""), disposeSession(terminalId ?? "", db)) even though terminalId is always a string; remove the "?? ''" fallbacks and use terminalId directly in calls inside onMessage, onClose, and onError (and in disposeSession) so sessions.get(terminalId), disposeSession(terminalId, db), and socket comparisons reference the actual terminalId value without masking potential bugs.
158-172: Clarify server endpoint contract for session reconnection.The POST
/terminal/sessionsendpoint accepts optionallaunchModeandcommandparameters (line 223-228), but the client'spostCreateSession()never sends them—it only sendsterminalIdandworkspaceId. TheonConflictDoUpdatepreserves the originalcwd,shell,launchMode, andcommandbecause reconnection should re-attach to the existing session, not alter its configuration. However, this contract is implicit. Either:
- Explicitly document that launchMode/command parameters are ignored on reconnection, or
- Remove those optional parameters from the endpoint signature to prevent confusing future callers who might expect them to update the session.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/terminal/terminal.ts` around lines 158 - 172, The server currently accepts optional launchMode and command in the POST /terminal/sessions handler but the client method postCreateSession sends only terminalId and workspaceId and reconnections must not change existing session config; make the contract explicit by either removing launchMode and command from the endpoint signature or documenting they are ignored on conflict. Locate the session upsert logic (db.insert(...).onConflictDoUpdate(...) on terminalSessions) and the client call postCreateSession and choose one approach: 1) remove the optional launchMode/command parameters from the server request DTO/handler so callers cannot pass them, or 2) add a clear comment and API docs in the handler stating that launchMode/command are only used on create and are ignored on reconnection and ensure the onConflictDoUpdate only updates status/endedAt (keep cwd/shell/launchMode/command untouched). Ensure the change is applied both in the server endpoint signature and any related API docs/types so future callers see the intended behavior.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx (1)
24-31: Inline subscribe function causes re-subscription on every render.The
subscribeToState(terminalId)call insideuseSyncExternalStorereturns a new function reference on each render. This causesuseSyncExternalStoreto unsubscribe and resubscribe on every render, which is inefficient and could cause subtle timing issues.Wrap the subscribe function in
useCallbackto stabilize its identity:♻️ Stabilize subscribe function reference
-function subscribeToState(terminalId: string) { - return (callback: () => void) => - terminalRuntimeRegistry.onStateChange(terminalId, callback); -} - -function getConnectionState(terminalId: string): ConnectionState { - return terminalRuntimeRegistry.getConnectionState(terminalId); -} export function TerminalPane({ terminalId, workspaceId }: TerminalPaneProps) { // ... existing code ... + const subscribe = useCallback( + (callback: () => void) => + terminalRuntimeRegistry.onStateChange(terminalId, callback), + [terminalId], + ); + + const getSnapshot = useCallback( + () => terminalRuntimeRegistry.getConnectionState(terminalId), + [terminalId], + ); + const connectionState = useSyncExternalStore( - subscribeToState(terminalId), - () => getConnectionState(terminalId), + subscribe, + getSnapshot, );Also applies to: 73-76
🤖 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/TerminalPane.tsx around lines 24 - 31, The subscribe function returned by subscribeToState(terminalId) is recreated each render which forces useSyncExternalStore to unsubscribe/resubscribe; fix this by memoizing the subscribe callback with React.useCallback so subscribeToState returns a stable function identity for a given terminalId (e.g., wrap the function passed into useSyncExternalStore in useCallback or make subscribeToState itself return a memoized callback keyed by terminalId); apply the same change to the other subscription site that mirrors this pattern (the second subscribe helper around lines 73-76) and keep getConnectionState(terminalId) as the stable snapshot selector for useSyncExternalStore.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts (1)
84-101: Side effect inuseMemomay cause unexpected behavior.The migration logic inside
useMemowrites tocollections.v2WorkspaceLocalState, which is a side effect. In React StrictMode (development),useMemocallbacks can run twice, potentially causing duplicate update calls. While the migration itself is idempotent, triggering collection updates from within a memoization callback is unconventional.Consider moving the persistence side effect to a
useEffectthat depends on a flag indicating migration occurred:♻️ Suggested refactor to separate migration detection from persistence
const persistedPaneLayout = useMemo(() => { const layout = (localWorkspaceState?.paneLayout as | WorkspaceState<PaneViewerData> - | undefined) ?? EMPTY_STATE; - - // Migrate legacy terminal panes ({sessionKey, cwd, …} → {terminalId}) - if (layout !== EMPTY_STATE && migrateTerminalPaneData(layout)) { - // Persist the migrated layout back so the migration only runs once - if (collections.v2WorkspaceLocalState.get(workspaceId)) { - collections.v2WorkspaceLocalState.update(workspaceId, (draft) => { - draft.paneLayout = layout; - }); - } - } - - return layout; -}, [localWorkspaceState, collections, workspaceId]); + | undefined) ?? EMPTY_STATE; + const migrated = layout !== EMPTY_STATE && migrateTerminalPaneData(layout); + return { layout, migrated }; +}, [localWorkspaceState]); + +// Persist migrated layout in a separate effect +useEffect(() => { + if (persistedPaneLayout.migrated && collections.v2WorkspaceLocalState.get(workspaceId)) { + collections.v2WorkspaceLocalState.update(workspaceId, (draft) => { + draft.paneLayout = persistedPaneLayout.layout; + }); + } +}, [persistedPaneLayout, collections, workspaceId]);Note: You'd need to adjust downstream usage to access
persistedPaneLayout.layout.🤖 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/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts around lines 84 - 101, The useMemo in useV2WorkspacePaneLayout currently runs migrateTerminalPaneData and persists the migrated layout to collections.v2WorkspaceLocalState inside the memo (persistedPaneLayout), which is a side effect; move the persistence out of the useMemo into a useEffect: have the useMemo only compute and return the layout (calling migrateTerminalPaneData purely to detect whether migrationNeeded and returning the migrated layout), expose a migration flag (e.g., migrationNeeded or migratedLayout) from that memo, then add a useEffect that watches migrationNeeded/migratedLayout, workspaceId and collections and performs the collections.v2WorkspaceLocalState.update(workspaceId, ...) call when migrationNeeded is true (ensuring idempotence). Ensure the code references remain useV2WorkspacePaneLayout, persistedPaneLayout, migrateTerminalPaneData, and collections.v2WorkspaceLocalState so callers still get the migrated layout but all writes happen in useEffect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:
- Around line 78-108: startCreateSession can enter a rapid reconnect loop when
sessions are created but sockets immediately close; add a reconnect-rate
limiter: introduce a reconnectAttemptsRef (or similar) and a lastReconnectTs to
track cross-call attempts, increment it each time startCreateSession is invoked
due to a closed socket, and if it exceeds a configurable threshold or cooldown
window prevent new attempts (or apply an exponential backoff/cooldown) until the
cooldown expires; reset reconnectAttemptsRef when sessionState becomes "ready"
or on a successful connection; update references in startCreateSession (and
related effects that call it) to check the limiter before proceeding and keep
existing controllerRef, MAX_RETRIES, and error handling logic intact.
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 110-156: The workspace DB lookup
db.query.workspaces.findFirst(...).sync() in createTerminalSessionInternal can
throw and is unhandled; wrap that call (or the minimal block that includes
workspace resolution and existsSync check) in a try/catch and return a
structured { error: string } when an exception occurs (include error.message
when available) so the function never throws synchronously; additionally, avoid
leaking sensitive env vars by replacing the spread of process.env in the spawn
options with a curated env object (merge only required keys like PATH, HOME,
TERM, COLORTERM and any explicit app-specific vars) instead of ...process.env
used in the spawn(...) call.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:
- Around line 24-31: The subscribe function returned by
subscribeToState(terminalId) is recreated each render which forces
useSyncExternalStore to unsubscribe/resubscribe; fix this by memoizing the
subscribe callback with React.useCallback so subscribeToState returns a stable
function identity for a given terminalId (e.g., wrap the function passed into
useSyncExternalStore in useCallback or make subscribeToState itself return a
memoized callback keyed by terminalId); apply the same change to the other
subscription site that mirrors this pattern (the second subscribe helper around
lines 73-76) and keep getConnectionState(terminalId) as the stable snapshot
selector for useSyncExternalStore.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts:
- Around line 84-101: The useMemo in useV2WorkspacePaneLayout currently runs
migrateTerminalPaneData and persists the migrated layout to
collections.v2WorkspaceLocalState inside the memo (persistedPaneLayout), which
is a side effect; move the persistence out of the useMemo into a useEffect: have
the useMemo only compute and return the layout (calling migrateTerminalPaneData
purely to detect whether migrationNeeded and returning the migrated layout),
expose a migration flag (e.g., migrationNeeded or migratedLayout) from that
memo, then add a useEffect that watches migrationNeeded/migratedLayout,
workspaceId and collections and performs the
collections.v2WorkspaceLocalState.update(workspaceId, ...) call when
migrationNeeded is true (ensuring idempotence). Ensure the code references
remain useV2WorkspacePaneLayout, persistedPaneLayout, migrateTerminalPaneData,
and collections.v2WorkspaceLocalState so callers still get the migrated layout
but all writes happen in useEffect.
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 297-345: The code uses unnecessary null coalescing for terminalId
(e.g., sessions.get(terminalId ?? ""), disposeSession(terminalId ?? "", db))
even though terminalId is always a string; remove the "?? ''" fallbacks and use
terminalId directly in calls inside onMessage, onClose, and onError (and in
disposeSession) so sessions.get(terminalId), disposeSession(terminalId, db), and
socket comparisons reference the actual terminalId value without masking
potential bugs.
- Around line 158-172: The server currently accepts optional launchMode and
command in the POST /terminal/sessions handler but the client method
postCreateSession sends only terminalId and workspaceId and reconnections must
not change existing session config; make the contract explicit by either
removing launchMode and command from the endpoint signature or documenting they
are ignored on conflict. Locate the session upsert logic
(db.insert(...).onConflictDoUpdate(...) on terminalSessions) and the client call
postCreateSession and choose one approach: 1) remove the optional
launchMode/command parameters from the server request DTO/handler so callers
cannot pass them, or 2) add a clear comment and API docs in the handler stating
that launchMode/command are only used on create and are ignored on reconnection
and ensure the onConflictDoUpdate only updates status/endedAt (keep
cwd/shell/launchMode/command untouched). Ensure the change is applied both in
the server endpoint signature and any related API docs/types so future callers
see the intended behavior.
🪄 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: 6f2a398e-ffd3-4e48-a2e3-1a3ce7ad2a1c
📒 Files selected for processing (14)
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/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.tsapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.tsapps/desktop/src/renderer/stores/tabs/store.tspackages/host-service/drizzle/0002_add_terminal_sessions.sqlpackages/host-service/drizzle/meta/0002_snapshot.jsonpackages/host-service/drizzle/meta/_journal.jsonpackages/host-service/src/db/schema.tspackages/host-service/src/terminal/terminal.ts
![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 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts:94">
P2: `collections.v2WorkspaceLocalState.update(...)` is a side effect inside `useMemo`. React treats `useMemo` as a pure computation and may invoke it multiple times (React 18 Strict Mode does this in dev) or skip it entirely. This can cause duplicate migration writes or a skipped persist. Move the persistence write into a `useEffect` that reacts to the migrated layout value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
saddlepaddle
reviewed
Apr 3, 2026
saddlepaddle
reviewed
Apr 3, 2026
saddlepaddle
reviewed
Apr 3, 2026
saddlepaddle
reviewed
Apr 3, 2026
Restore auto-create-on-websocket-connect in host-service instead of requiring a separate POST before attach. This removes all the extra renderer state (sessionState, retry, reconnect) that was only needed for the two-step create/attach flow. TerminalPane is back to its original shape — just terminalId instead of paneId. The POST /terminal/sessions endpoint stays as a dormant API for future use.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/host-service/src/terminal/terminal.ts (2)
222-247: Add request body validation with Zod schema.The current implementation uses type assertion without runtime validation. If the request body is malformed JSON,
c.req.json()will throw an unhandled exception. Since Zod 4 is available, consider adding schema validation for robustness.🛠️ Suggested validation with Zod
+import { z } from "zod"; + +const createSessionSchema = z.object({ + terminalId: z.string().min(1), + workspaceId: z.string().min(1), + launchMode: z.string().optional(), + command: z.string().optional(), +}); app.post("/terminal/sessions", async (c) => { - const body = await c.req.json<{ - terminalId: string; - workspaceId: string; - launchMode?: string; - command?: string; - }>(); - - if (!body.terminalId || !body.workspaceId) { - return c.json({ error: "Missing terminalId or workspaceId" }, 400); - } + let body; + try { + const raw = await c.req.json(); + body = createSessionSchema.parse(raw); + } catch (error) { + return c.json({ error: "Invalid request body" }, 400); + } const result = createTerminalSessionInternal({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/terminal/terminal.ts` around lines 222 - 247, Wrap the c.req.json() parsing in runtime validation using a Zod schema for the expected body (fields terminalId: string, workspaceId: string, optional launchMode and command), and handle parsing/validation errors by returning a 400 JSON response; update the POST handler (app.post("/terminal/sessions")) so it catches JSON parse exceptions and Zod validation failures before calling createTerminalSessionInternal, and only call createTerminalSessionInternal when the validated payload passes (then pass the validated values to createTerminalSessionInternal and keep the existing error/ success responses).
158-172: Consider updating more fields on conflict to handle session re-creation scenarios.The upsert currently only updates
statusandendedAton conflict. If a session is re-created with different parameters (e.g., differentcwd,workspaceId, orcommand), those won't be updated. This may be intentional to preserve original launch metadata, but worth confirming.💡 Optional: Update launch metadata on conflict
db.insert(terminalSessions) .values({ id: terminalId, workspaceId, cwd, shell, launchMode, command: command ?? null, status: "active", }) .onConflictDoUpdate({ target: terminalSessions.id, - set: { status: "active", endedAt: null }, + set: { + status: "active", + endedAt: null, + workspaceId, + cwd, + shell, + launchMode, + command: command ?? null, + }, }) .run();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/terminal/terminal.ts` around lines 158 - 172, The upsert only updates status and endedAt on conflict; update the onConflictDoUpdate set to also refresh launch metadata so re-created sessions reflect new parameters — include fields terminalSessions.{cwd, workspaceId, shell, launchMode, command} (alongside status and endedAt) in the set clause so when conflict on terminalSessions.id occurs the record is updated with the incoming cwd, workspaceId, shell, launchMode and command values from the insert for terminalId, workspaceId, cwd, shell, launchMode, command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 222-247: Wrap the c.req.json() parsing in runtime validation using
a Zod schema for the expected body (fields terminalId: string, workspaceId:
string, optional launchMode and command), and handle parsing/validation errors
by returning a 400 JSON response; update the POST handler
(app.post("/terminal/sessions")) so it catches JSON parse exceptions and Zod
validation failures before calling createTerminalSessionInternal, and only call
createTerminalSessionInternal when the validated payload passes (then pass the
validated values to createTerminalSessionInternal and keep the existing error/
success responses).
- Around line 158-172: The upsert only updates status and endedAt on conflict;
update the onConflictDoUpdate set to also refresh launch metadata so re-created
sessions reflect new parameters — include fields terminalSessions.{cwd,
workspaceId, shell, launchMode, command} (alongside status and endedAt) in the
set clause so when conflict on terminalSessions.id occurs the record is updated
with the incoming cwd, workspaceId, shell, launchMode and command values from
the insert for terminalId, workspaceId, cwd, shell, launchMode, command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a06c60ce-817c-4623-a7b8-e9ce4dfeab9c
📒 Files selected for processing (3)
apps/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/usePaneRegistry.tsxpackages/host-service/src/terminal/terminal.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx
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:45">
P1: The pane does not attempt to reconnect after websocket close, so terminals can remain permanently disconnected after transient failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Keep terminal_sessions minimal — only session lifecycle state: id, origin_workspace_id, status, created_at, last_attached_at, ended_at. Removed cwd, shell, launchMode, command — those are create request inputs, not session state. They can be persisted separately later if needed for revive/replay.
Contributor
There was a problem hiding this comment.
1 issue found across 6 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/drizzle/0002_add_terminal_sessions.sql">
<violation number="1" location="packages/host-service/drizzle/0002_add_terminal_sessions.sql:3">
P1: Do not rewrite an already-versioned migration file; add a new migration for the column rename/schema change to keep upgrade paths consistent.</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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/terminal/terminal.ts (1)
83-99:⚠️ Potential issue | 🟠 MajorGuard
onExitfrom overwriting a deliberate dispose.
disposeSession()writes"disposed"at Lines 96-99, but the PTY'sonExithandler still unconditionally writes"exited"at Lines 187-190. That can silently flip a deliberate dispose back to exited, and it can also clobber a freshly recreated session with the sameterminalId.🛠️ Proposed fix
pty.onExit(({ exitCode, signal }) => { session.exited = true; session.exitCode = exitCode ?? 0; session.exitSignal = signal ?? 0; + + if (sessions.get(terminalId) !== session) { + return; + } db.update(terminalSessions) .set({ status: "exited", endedAt: Date.now() }) .where(eq(terminalSessions.id, terminalId)) .run();Also applies to: 182-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/terminal/terminal.ts` around lines 83 - 99, The PTY onExit handler must not overwrite a deliberate dispose or a newly-created session with the same terminalId; capture the session object when you attach the pty.onExit handler and, inside that handler, first re-read sessions.get(terminalId) and verify it is the same session object (or that session.exited is false and not already disposed) before writing the "exited" status to the DB and setting session.exited; this ensures disposeSession (which sets status "disposed" and deletes the session) cannot be clobbered and a replaced session with the same terminalId won't be affected by the old handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 83-99: The PTY onExit handler must not overwrite a deliberate
dispose or a newly-created session with the same terminalId; capture the session
object when you attach the pty.onExit handler and, inside that handler, first
re-read sessions.get(terminalId) and verify it is the same session object (or
that session.exited is false and not already disposed) before writing the
"exited" status to the DB and setting session.exited; this ensures
disposeSession (which sets status "disposed" and deletes the session) cannot be
clobbered and a replaced session with the same terminalId won't be affected by
the old handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3144e80e-936d-4bb1-8107-a2ed5629b939
📒 Files selected for processing (5)
packages/host-service/drizzle/0002_add_terminal_sessions.sqlpackages/host-service/drizzle/meta/0002_snapshot.jsonpackages/host-service/drizzle/meta/_journal.jsonpackages/host-service/src/db/schema.tspackages/host-service/src/terminal/terminal.ts
✅ Files skipped from review due to trivial changes (3)
- packages/host-service/drizzle/meta/_journal.json
- packages/host-service/src/db/schema.ts
- packages/host-service/drizzle/meta/0002_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/host-service/drizzle/0002_add_terminal_sessions.sql
Take main's CommandPalette refactor (simplified props). Our branch had no changes to CommandPalette — conflict was from nearby context.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 5, 2026
upstream 8922b94 — N8 - paneIdとterminalIdを分離 - terminal_sessionsテーブル追加 (host-service DB) - TerminalPane/usePaneRegistryをterminalId対応に更新
9 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 5, 2026
cherry-pick方式で内容を取り込み済みの14コミットをgit履歴上もマージ済みにする。 取り込み済み (cherry-pick / 手動移植): - be22b46 superset-sh#3125 — スキップ (下記参照) - 88bc7fb superset-sh#3127 — Revert DA1 ✓ - 92d0ff9 superset-sh#3054 — DA1 fix ✓ - c48450e superset-sh#3093 — file viewer pane fix ✓ - fffa8db superset-sh#3128 — version 1.4.7 ✓ - 589a7c7 superset-sh#3136 — fuzzy scorer (ハイブリッド方式) ✓ - ceb8c81 superset-sh#3150 — Electron 40.8.5 ✓ - 8922b94 superset-sh#3137 — terminalId分離 ✓ - c7508e5 superset-sh#3152 — GitHub無料化 ✓ - 2b91f11 superset-sh#3155 — v2 terminal theme ✓ - b8b11af superset-sh#3154 — TUI dimension fix ✓ - 7599ace superset-sh#3149 — v2 sidebar file tree (手動統合) ✓ - 4d7c612 superset-sh#3174 — DnD重複削除 ✓ - 864977d superset-sh#3157 — Host Service分離 ✓ 意図的にスキップ: - be22b46 superset-sh#3125 (GitHub polling簡素化) フォーク独自のGitHubSyncService (バックエンド集中ポーリング) と 設計が異なるため不採用。upstreamはフロントエンドhover駆動、フォークは バックエンドキャッシュウォーマー方式。詳細は githubQueryPolicy.ts と github-sync-service.ts のFORK NOTEを参照。 ゴースト・マージ復元 (revert 134cfd5 で消失した内容): - 538f306 superset-sh#3120 — Patch vuln ✓ - 1588d20 superset-sh#3108 — terminal lifecycle分離 ✓ - 59426f6 superset-sh#3122 — file tree + FilePane + Alert refactor (手動統合) ✓ - 10d9a5d superset-sh#3097 — tiptap line-height ✓ - 337a9ae superset-sh#3121 — Codex hooks削除 ✓
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
2 participants
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
terminalId) from pane/layout identity (paneId). Terminal runtimes, websocket transport, and host-service sessions are now keyed byterminalId, whilepaneIdremains purely for UI layout concerns (split, focus, close, tab ordering).terminal_sessionstable to host-service DB as durable session records with status tracking (active→exited→disposed), workspace association (nullable FK withSET NULL), and launch metadata (cwd,shell,launchMode,command).POST /terminal/sessionsto create the PTY + DB row, then opens a websocket to/terminal/:terminalIdfor attach only. Host-service rejects websocket connections for unknown terminalIds.Key changes
Data model (
types.ts):TerminalPaneDatasimplified from{sessionKey, cwd, launchMode, command?}to{terminalId: string}.Renderer runtime (
terminal-runtime.ts,terminal-runtime-registry.ts): All keying switched frompaneIdtoterminalId— localStorage persistence, xterm instances, transport entries.Pane rendering (
TerminalPane.tsx): Creates session via authenticated POST before attaching. Includes retry with exponential backoff (3 attempts), auto-reconnect on websocket close (handles host-service restarts), manual retry button on failure, and properAbortControllerscoping.Lifecycle (
useGlobalTerminalLifecycle.ts): ExtractsterminalIdfrom pane data (notpaneId). Disposes on unreferenced (default policy for this cut).Migration (
useV2WorkspacePaneLayout.ts): Detects legacy{sessionKey, cwd, ...}panes and backfillsterminalIdon load; persists immediately so migration is one-shot.Host-service (
terminal.ts): Route changed to/terminal/:terminalId. Session creation writesterminal_sessionsrow with upsert for restart safety. PTY spawn honorslaunchMode/commandmetadata. Dispose updates DB status.Schema (
schema.ts+ migration):terminal_sessionstable with nullableworkspace_id(ON DELETE SET NULL), status/timestamp tracking, indexed by workspace and status.Test plan
terminalIdterminal_sessionstable has correct rows after create/close cyclesSummary by cubic
Decouples terminal session identity from panes and adds durable
host-serviceterminal sessions. Restores auto-create on WebSocket attach so terminals survive pane moves and reconnect cleanly.New Features
terminalId;paneIdis layout-only./terminal/:terminalId?workspaceId=...auto-creates or reattaches; POST/terminal/sessionsexists but is optional.host-servicepersists sessions interminal_sessions(id, origin_workspace_id, status, created_at, last_attached_at, ended_at) and updates on attach/exit/dispose.terminalIdonly; removed client-side create/retry state. Global lifecycle disposes runtimes and server sessions when unreferenced, with a short delay for cross-workspace moves.Migration
terminal_sessionswith indexes; no manual steps needed.Written for commit 7227b06. Summary will update on new commits.
Summary by CodeRabbit
Release Notes