Repository navigation
feat(desktop): v1→v2 terminal migration — lazy recreation at the v1 cwd on workspace open - #5818
Merged
Merged
Conversation
…errides into host.db
…created at v1 cwd on workspace open
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughChangesTerminal Migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant appState
participant MigrationRunner
participant WorkspaceState
participant WorkspacePage
participant TerminalLauncher
appState->>MigrationRunner: read mapped v1 terminal panes
MigrationRunner->>WorkspaceState: append pending migrated terminals
WorkspacePage->>TerminalLauncher: create queued sessions after layout readiness
TerminalLauncher-->>WorkspacePage: return session creation results
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Contributor
Greptile SummaryThis PR adds lazy recreation of v1 terminals when a migrated v2 workspace opens. The main changes are:
Confidence Score: 4/5The terminal recovery and queue-consumption paths need fixes before merging.
apps/desktop/src/renderer/lib/v1-migration/runV1Migration.ts; apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useCreatePendingMigratedTerminals/useCreatePendingMigratedTerminals.ts
|
| Filename | Overview |
|---|---|
| apps/desktop/src/lib/trpc/routers/migration/index.ts | Adds a main-process query that maps v1 terminal panes to workspaces and selects the best available cwd. |
| apps/desktop/src/renderer/lib/v1-migration/runV1Migration.ts | Adds terminal migration and ledger reporting, but queue persistence can get ahead of the ledger and duplicate terminals after recovery. |
| apps/desktop/src/renderer/lib/v1-migration/terminals.ts | Adds planning for fresh terminal IDs, workspace grouping, and deferral of unmapped panes. |
| apps/desktop/src/renderer/stores/workspace-creates/appendPendingMigratedTerminals.ts | Creates missing local-state rows and appends pending terminals, with deduplication limited to terminal IDs. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useCreatePendingMigratedTerminals/useCreatePendingMigratedTerminals.ts | Creates queued sessions and triggers adoption, but does not react to queue updates and can leave its runner locked after invalidation failure. |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts | Adds the pending-terminal queue to workspace-local state with healing for existing rows. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant M as v1 Migration
participant Q as Local Terminal Queue
participant L as Migration Ledger
participant W as v2 Workspace Page
participant T as Terminal Service
M->>M: Read panes and assign fresh IDs
M->>Q: Append pending terminals
M->>L: Flush successful mappings
Note over Q,L: Exit between writes leaves queue ahead of ledger
W->>Q: Read queue when layout is ready
loop Pending terminals
W->>T: Create session at saved cwd
end
W->>Q: Remove created entries
W->>T: Invalidate session list for adoption
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant M as v1 Migration
participant Q as Local Terminal Queue
participant L as Migration Ledger
participant W as v2 Workspace Page
participant T as Terminal Service
M->>M: Read panes and assign fresh IDs
M->>Q: Append pending terminals
M->>L: Flush successful mappings
Note over Q,L: Exit between writes leaves queue ahead of ledger
W->>Q: Read queue when layout is ready
loop Pending terminals
W->>T: Create session at saved cwd
end
W->>Q: Remove created entries
W->>T: Invalidate session list for adoption
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/renderer/lib/v1-migration/runV1Migration.ts:550
**Queue And Ledger Diverge**
If the renderer exits after `appendPending` persists the queue but before the success outcomes are flushed, the next run assigns new UUIDs to the same v1 panes. The queue deduplicates only by `terminalId`, so both sets remain and opening the workspace creates duplicate shells and panes.
### Issue 2 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useCreatePendingMigratedTerminals/useCreatePendingMigratedTerminals.ts:28-30
**Late Queue Entries Stay Pending**
The effect reads the queue once, but collection changes are not dependencies. If migration appends terminals after the layout-ready effect has read an empty queue, or while an existing batch is running, those entries do not trigger another pass and remain pending until the workspace is reopened.
### Issue 3 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useCreatePendingMigratedTerminals/useCreatePendingMigratedTerminals.ts:61-63
**Invalidation Failure Locks Runner**
If session-list invalidation rejects after sessions are created, control never reaches the `runningRef.current = false` assignment. The hook then rejects every later effect run for this mounted workspace, so any terminals queued afterward cannot be created until the page remounts.
Reviews (1): Last reviewed commit: "feat(desktop): v1 terminal migration — l..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!projectId) { | ||
| throw new Error("workspace missing from host list"); | ||
| } | ||
| target.appendPending({ id: v2WorkspaceId, projectId }, terminals); |
Contributor
There was a problem hiding this comment.
If the renderer exits after appendPending persists the queue but before the success outcomes are flushed, the next run assigns new UUIDs to the same v1 panes. The queue deduplicates only by terminalId, so both sets remain and opening the workspace creates duplicate shells and panes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/v1-migration/runV1Migration.ts
Line: 550
Comment:
**Queue And Ledger Diverge**
If the renderer exits after `appendPending` persists the queue but before the success outcomes are flushed, the next run assigns new UUIDs to the same v1 panes. The queue deduplicates only by `terminalId`, so both sets remain and opening the workspace creates duplicate shells and panes.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+28
to
+30
| const pending = | ||
| collections.v2WorkspaceLocalState.get(workspaceId) | ||
| ?.pendingMigratedTerminals ?? []; |
Contributor
There was a problem hiding this comment.
The effect reads the queue once, but collection changes are not dependencies. If migration appends terminals after the layout-ready effect has read an empty queue, or while an existing batch is running, those entries do not trigger another pass and remain pending until the workspace is reopened.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useCreatePendingMigratedTerminals/useCreatePendingMigratedTerminals.ts
Line: 28-30
Comment:
**Late Queue Entries Stay Pending**
The effect reads the queue once, but collection changes are not dependencies. If migration appends terminals after the layout-ready effect has read an empty queue, or while an existing batch is running, those entries do not trigger another pass and remain pending until the workspace is reopened.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+61
to
+63
| await utils.terminal.listSessions.invalidate({ workspaceId }); | ||
| } | ||
| runningRef.current = false; |
Contributor
There was a problem hiding this comment.
If session-list invalidation rejects after sessions are created, control never reaches the runningRef.current = false assignment. The hook then rejects every later effect run for this mounted workspace, so any terminals queued afterward cannot be created until the page remounts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useCreatePendingMigratedTerminals/useCreatePendingMigratedTerminals.ts
Line: 61-63
Comment:
**Invalidation Failure Locks Runner**
If session-list invalidation rejects after sessions are created, control never reaches the `runningRef.current = false` assignment. The hook then rejects every later effect run for this mounted workspace, so any terminals queued afterward cannot be created until the page remounts.
How can I resolve this? If you propose a fix, please make it concise.
Member
Author
|
CDP E2E verified: seeded a v1 terminal pane (cwd = the demo worktree) in app-state.json → |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Merged
1 of 2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
PR 3 of the v1→v2 auto-migration (SUPER-1534 / SUPER-1577, decision D2: lazy terminal recreation, no boot-time PTY storm). Live v1 PTYs can't hand off (separate daemon owners), so the contract is a fresh shell at the persisted v1 cwd, in the right workspace.
migration.readV1TerminalPanes: first read ofapp-state.jsonby the migration router — resolves each v1 terminal pane to its workspace (via tab) and best-known cwd (cwd ?? initialCwd).migrateTerminalsstage inrunV1Migration()(after settings, before presets): assigns each pane a fresh v2 terminal id (v1 pane ids double as v1 daemon session ids — reusing them risks colliding with the live v1 daemon; idempotence comes from the ledger, kindterminal, v1Id = paneId) and queues{terminalId, cwd}per mapped workspace via an injectedterminalTarget. Panes on unmigrated workspaces defer to the next run. Never gates the flip.pendingMigratedTerminalsfield onv2WorkspaceLocalState(healed like the other optional fields) +appendPendingMigratedTerminalshelper that upserts the row (adopted-but-never-opened workspaces have none yet).useCreatePendingMigratedTerminalshook on the v2 workspace page: on first open (layout-ready), creates the queued sessions via the existing idempotent launcher, removes created entries (failures stay queued), and invalidatesterminal.listSessionsso feat(desktop): auto-create panes for background terminal sessions on workspace open #5740'suseAutoAdoptBackgroundSessionsbuilds the panes immediately.Dormant until the PR 4 boot trigger wires
terminalTarget— the page hook no-ops on an empty queue for all existing users.Test Plan
bun test src/renderer/lib/v1-migration/— 21 tests incl.planTerminalMigration(fresh-id assignment, per-workspace grouping, unmapped deferral)Summary by cubic
Adds v1→v2 terminal migration with lazy recreation: on first open of a migrated workspace, we spawn fresh v2 terminals at the v1 cwd instead of at boot to avoid a PTY storm. Aligns with Linear SUPER-1534 / SUPER-1577 (D2).
readV1TerminalPanesto read v1 panes fromapp-state.json, mapped to workspace and cwd.migrateTerminalsstage: assigns fresh v2 terminal IDs, records aterminalledger entry per v1 pane, and queues{terminalId, cwd}per migrated workspace via an injectedterminalTarget. Unmapped panes defer; never gates the flip.pendingMigratedTerminalsto v2 workspace local state with an append helper; healed by schema defaults.useCreatePendingMigratedTerminalshook: on first workspace open, creates queued sessions via the idempotent launcher, prunes created entries, and invalidatesterminal.listSessionsso auto-adopt builds panes.terminalTarget.Written for commit d0def35. Summary will update on new commits.
Summary by CodeRabbit