Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between da39a95844deeb0f6a5a1203393f6e1300b16cfe and 281deb1. 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change adds write-once, per-worktree markers for applied default terminal tabs. Host-admitted terminal persistence records the marker. Session writes union prior markers with new payloads. Renderer persistence writes marker patches, and hydration merges persisted markers with local state. Tests cover session preservation, host-admitted terminal handling, persistence subscriber behavior, hydration, schema parsing, and suppression of repeated default-terminal application. Priority: ⬆️ High Severity of issue fixed: High 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, scope, solution, regression behavior, testing command, and linked issue. However, it does not follow the required template and omits required sections such as Visual Proof with an N/A explanation, Testing checklist details, Notes, Review, Agent skill boundary, and the final Checklist. Resolution Use the repository template headings and complete the missing sections. Add Visual Proof with
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 1dd11312-743f-4a73-a158-31b82be4b6e9
📥 Commits
Reviewing files that changed from the base of the PR and between 8dc3c1d and da39a95844deeb0f6a5a1203393f6e1300b16cfe.
📒 Files selected for processing (8)
src/main/persistence-host-admitted-terminal-membership.test.tssrc/main/persistence/loading-store/pty-binding-persistence.tssrc/main/persistence/runtime-authored-workspace-session-fields.test.tssrc/main/persistence/runtime-authored-workspace-session-fields.tssrc/renderer/src/lib/default-terminal-tabs-applied-persist.test.tssrc/renderer/src/lib/session-write-subscriber.test.tssrc/renderer/src/store/terminals/workspace-terminal-hydration-patch.tssrc/renderer/src/store/terminals/workspace-terminal-hydration.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Stamp marker on host PTY-binding persist —
persistPtyBindingnow writesdefaultTerminalTabsAppliedByWorktreeId[bindingWorktreeId] = trueon the host session before flush (rolled back byrestoreSession()on flush failure). - Union the write-once marker across full session replaces — a new
unionWriteOnceDefaultTerminalTabsAppliedmerges prior markers into a renderer full write that omits (or only partially lists) the map;preserveRuntimeAuthoredWorkspaceSessionFieldsrefactored to chain the client-hosted-page guard and the marker union. - Union on hydration —
hydrateWorkspaceSessionnow produces{ ...session.map, ...liveStore.map }instead ofsession.map ?? {}, so a snapshot that omits the marker can't no-op a live mark. - Local marks win in scoped re-hydration —
targetScopedWorkspaceHydrationPatchspreads live state over the hydrated map so an SSH pull replace can't drop a local marker. - Tests — round-trip, union, host-stamp, and subscriber coverage added across four files; I also ran adjacent
terminals-hydration,workspace-session-host-split, andworktrees-identity-migrationsuites (all green).
The write-once semantics are consistent with the existing mergeDirectSshRemoteWorkspaceSession rule and documented clearly. Removal still flows through the worktree-teardown path (removeWorkspaceSessionStateForWorktree → deleteOwnerKeyedSessionFields), which bypasses the union, so teardown is not undermined. The patch write path (patchWorkspaceSession) also bypasses preserveRuntimeAuthoredWorkspaceSessionFields, so a normal write that drops a worktree key still removes it correctly.
ℹ️ Nitpicks
src/renderer/src/lib/default-terminal-tabs-applied-persist.test.tsimportspreserveRuntimeAuthoredWorkspaceSessionFieldsdirectly fromsrc/main/persistence/, crossing the main↔renderer layer boundary. It works because the module is pure, but it couples a renderer test to a main-process module.- The trailing
...state.defaultTerminalTabsAppliedByWorktreeIdinworkspace-terminal-hydration-patch.tsmakes the non-replace-key remit of thereplaceHydratedRecordKeysargument dead weight — correct, but the expression could be just{ ...hydrated-markers-for-replaced-keys, ...state.map }. - The
session-write-subscriber.test.tscase asserts behavior that predates the fix (defaultTerminalTabsAppliedByWorktreeIdwas already inSESSION_RELEVANT_FIELDS), so it passes with the bug present — a guard, not evidence the fix works.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Host persist snapshots wrote tabs but omitted this write-once map, and full session replaces / hydration treated omission as "never applied". Union the marker across persist and hydrate so default terminals are not re-spawned on every launch or re-attach. Fixes stablyai#18117
da39a95 to
281deb1
Compare
Sync update (
|
|
Closing as superseded by #21630, merged on 2026-09-19. That change persists |
Description
workspaceSession.defaultTerminalTabsAppliedByWorktreeIdnever gained entries, so every launch and re-attach treated every worktree as never having default terminals applied and spawned a freshpendingActivationSpawntab.Host persist snapshots already wrote tabs, but they omitted this write-once map. Full session replaces and hydration then treated omission as "never applied".
Focused fix
Preserves
Evidence
pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/lib/default-terminal-tabs-applied-persist.test.ts src/main/persistence/runtime-authored-workspace-session-fields.test.ts src/main/persistence-host-admitted-terminal-membership.test.ts src/renderer/src/lib/session-write-subscriber.test.ts{}; a later full write or hydrate from an omitted snapshot wiped any in-memory marks;applyDefaultTerminalTabsran again.markDefaultTerminalTabsApplied→ persist/patch → omitted snapshot → hydration still has the worktreeId, so default tabs are not applied again.User-regression-tradeoffs
Fixes #18117