Repository navigation
Conversation
|
@Fewmanism is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-workspace, versioned, canonicalized session snapshot persistence and atomic storage; integrates async workspace-session restoration into TabManager with opt-in flags; wires creation sites to enable/disable restore; updates AppDelegate to coordinate primary and per-workspace saves; and adds tests for persistence and restore behaviors. ChangesWorkspace Session Persistence
Sequence DiagramsequenceDiagram
participant TabManager
participant SessionPersistenceStore
participant FileSystem
participant Workspace
TabManager->>SessionPersistenceStore: saveWorkspaceSnapshot(snapshot, workingDirectory)
SessionPersistenceStore->>SessionPersistenceStore: canonicalize directory & compute SHA-256 digest
SessionPersistenceStore->>FileSystem: write envelope JSON to per-workspace session.json
TabManager->>SessionPersistenceStore: loadWorkspaceSnapshot(workingDirectory) [async]
SessionPersistenceStore->>FileSystem: read per-workspace session.json
SessionPersistenceStore->>SessionPersistenceStore: validate version & match canonical workingDirectory
SessionPersistenceStore->>TabManager: return SessionWorkspaceSnapshot?
TabManager->>Workspace: restoreSessionSnapshot(snapshot) — apply title/currentDirectory/panels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 2 inconclusive)
✅ Passed checks (11 passed)
✨ 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 |
Greptile SummaryThis PR adds per-directory workspace session persistence: snapshots are keyed by SHA-256 of the canonicalized working directory and stored in Application Support, restored asynchronously when a workspace is opened for an explicit directory, and skipped for config/layout-driven workspaces.
Confidence Score: 5/5Safe to merge; all new code paths carry adequate guards and the existing test suite exercises the main restore, opt-out, mutation-guard, and idempotent-write paths. The core persistence logic is well-isolated: canonicalization, key stability, and idempotent writes are all tested. The async restore path is correctly @MainActor-anchored with Task.detached for I/O and a triple-guard before applying state. The two open items are declaration-level and lifecycle hygiene concerns that do not affect runtime correctness today. Sources/SessionPersistence.swift (SessionSnapshotWriteResult isolation annotation) and Sources/TabManager.swift (restore Task lifecycle) have minor cleanup left but no blocking correctness issue. Important Files Changed
Sequence DiagramsequenceDiagram
participant AC as AppDelegate (MainActor)
participant TM as TabManager (MainActor)
participant WS as Workspace (MainActor)
participant SP as SessionPersistenceStore
participant FS as FileSystem (App Support)
Note over AC,FS: Workspace open (explicit directory)
AC->>TM: addWorkspace(workingDirectory:restoreWorkspaceSession:true)
TM->>WS: "create Workspace(workspaceSessionRootDirectory = dir)"
TM->>TM: scheduleWorkspaceSessionRestore(workspaceId, expectedState)
TM-->>SP: Task.detached(priority:.utility) loadWorkspaceSnapshot(dir)
SP->>FS: canonicalize dir to SHA256 key, read session.json
SP-->>TM: SessionWorkspaceSnapshot?
TM->>TM: guard workspace still matches expectedState
alt snapshot found
TM->>WS: restoreSessionSnapshot(snapshot)
TM->>TM: clearWorkspaceGitProbes / scheduleGitRefresh
else no snapshot
TM->>AC: sendWelcomeCommandWhenReady (if autoWelcome)
end
Note over AC,FS: Autosave or window close
AC-->>SP: sessionPersistenceQueue.async saveResult(appSnapshot)
SP->>FS: write app session.json (skip if unchanged)
alt write succeeded or unchanged
SP->>SP: saveWorkspaceSnapshots(from: appSnapshot)
loop each workspace
SP->>FS: canonicalize workspaceSessionRootDirectory or currentDirectory
SP->>FS: read existing envelope (preserve createdAt)
SP->>FS: write workspace SHA256(dir)/session.json (skip if identical)
end
end
Reviews (8): Last reviewed commit: "fix: address remaining workspace restore..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 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="Sources/SessionPersistence.swift">
<violation number="1" location="Sources/SessionPersistence.swift:503">
P2: The `createdAt` timestamp defeats the deduplication optimization in `saveWorkspaceSnapshot`, causing unnecessary repeated writes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Addressed the review feedback in Changes:
Verification:
Local blocker:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: Length of output: 39423
🔴 Nit –
|
| Concern from prior review | Status |
|---|---|
| File I/O off main actor | ✅ workspaceSessionRestoreQueue |
| Pristine-workspace guard | ✅ customTitle + currentDirectory + panelIds |
| Explicit title preservation | ✅ explicitTitle override after restore |
| Sync writes on background queue | ✅ sessionPersistenceQueue.sync |
createdAt reuse / dedup early-return |
✅ existingEnvelope check |
| Regression coverage | ✅ 7 new test cases across 2 files |
| Config/layout opt-out | ✅ restoreWorkspaceSession: false in CmuxConfigExecutor and TerminalController |
The nonisolated struct syntax is the only item that could produce a real compiler diagnostic — the rest are polish. Once that's resolved the implementation looks solid.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 3735-3737: The per-workspace writes must only run when the primary
session save succeeds: capture and check the result of
SessionPersistenceStore.save(snapshot) inside the if let snapshot block and only
call SessionPersistenceStore.saveWorkspaceSnapshots(from: snapshot) when that
save returned success; if the save fails, avoid invoking saveWorkspaceSnapshots
(and emit/log an error or handle the failure path) so workspace restore files
can't diverge from a failed main session write.
In `@Sources/TabManager.swift`:
- Around line 2247-2259: addWorkspace schedules git/PR probing for the bootstrap
terminal before the async restore runs, but after
workspace.restoreSessionSnapshot(...) replaces the panel graph the restored
terminal panels never get re-probed; update the restore path initiated by
scheduleWorkspaceSessionRestore to, once the async restore completes, explicitly
re-register the git/PR probing for the restored panels by invoking the same
probing helper used in the full-manager restore path (i.e. the helper that
probes git/PR state for terminal panels), ensuring the call occurs after
workspace.restoreSessionSnapshot(...) so restored terminals receive branch/PR
sidebar state.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f6ec2fb3-6494-441b-973c-109b62379594
📒 Files selected for processing (5)
Sources/AppDelegate.swiftSources/SessionPersistence.swiftSources/TabManager.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/TabManagerSessionSnapshotTests.swift
|
Follow-up fixes pushed in Additional changes:
Verification:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: Length of output: 35142
🔴
|
| Concern from prior round | Status |
|---|---|
| File I/O off main actor | ✅ Task.detached(priority: .utility) |
| Pristine-workspace guard | ✅ customTitle + currentDirectory + panelIds check |
| Sync termination flush skips workspace I/O | ✅ writePrimarySnapshotBlock returns Bool; sync path discards it |
| Workspace snapshots gated on primary save | ✅ if writePrimarySnapshotBlock(), let snapshot |
| Git probe re-registration after restore | ✅ clearWorkspaceGitProbes + scheduleInitialWorkspaceGitMetadataRefreshIfPossible |
| Explicit title preservation after restore | ✅ workspace.setCustomTitle(explicitTitle) override |
The nonisolated struct is the only remaining compiler-visible issue. Once that is removed, the implementation is ready.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 3743-3749: The synchronous branch currently runs
writePrimarySnapshotBlock on sessionPersistenceQueue but does not call
SessionPersistenceStore.saveWorkspaceSnapshots, leaving per-workspace snapshots
stale; update the synchronously == true path (where sessionPersistenceQueue.sync
executes writePrimarySnapshotBlock) to detect the returned snapshot (from
writePrimarySnapshotBlock) and call
SessionPersistenceStore.saveWorkspaceSnapshots(from:) just like the async
closure does so per-workspace snapshot files are written on both sync and async
flows.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d62c69d-561b-4e61-9b1d-ffccf80169e6
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/TabManager.swift
|
Second follow-up pushed in Changes:
Verification:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 35203
🔴
|
| Concern | Status |
|---|---|
writeAllSnapshotsBlock gates workspace I/O on primary save |
✅ if writePrimarySnapshotBlock(), let snapshot |
| Sync termination flush now includes workspace snapshots | ✅ both paths dispatch writeAllSnapshotsBlock via sessionPersistenceQueue |
scheduleWorkspaceSessionRestore main-actor isolation |
✅ @MainActor outer + Task.detached(priority: .utility) for I/O |
| Explicit title preservation after restore | ✅ workspace.setCustomTitle(explicitTitle) override |
The nonisolated struct is the sole remaining blocker. Everything else is in good shape.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 3743-3746: The current logic inside writeAllSnapshotsBlock
incorrectly skips calling SessionPersistenceStore.saveWorkspaceSnapshots(from:)
when writePrimarySnapshotBlock() returns false; change the flow so that if let
snapshot { SessionPersistenceStore.saveWorkspaceSnapshots(from: snapshot) } is
executed regardless of the boolean result of writePrimarySnapshotBlock(), and
only treat false as a hard failure when SessionPersistenceStore.save(_:)
actually indicates a failure (consider updating SessionPersistenceStore.save(_:)
to return a richer enum like .written / .unchanged / .failed and adjust
writePrimarySnapshotBlock and its callers to handle the new status if needed).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb8496cd-661f-47e0-b99e-46acaef9f0c3
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/TabManager.swift
|
Follow-up for the latest CodeRabbit comment pushed in Changes:
Verification:
Local blocker remains unchanged:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 38061
One item still needs to be fixed before merge: 🔴
|
| Item | Status |
|---|---|
SessionSnapshotWriteResult .written/.unchanged/.failed semantics |
✅ |
save(_:) -> Bool backward-compat via saveResult(...).succeeded |
✅ |
writeAllSnapshotsBlock gates workspace writes on != .failed |
✅ |
Sync path uses sessionPersistenceQueue.sync |
✅ |
scheduleWorkspaceSessionRestore @MainActor isolation |
✅ |
testSaveResultDistinguishesUnchangedFromFailedWrites regression |
✅ |
The two nonisolated removals are the only remaining change needed. Would you like to open a follow-up issue for the stale snapshot pruning strategy mentioned in earlier rounds?
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 3743-3749: The fast path in writeAllSnapshotsBlock returns when
snapshot is nil and thus skips
SessionPersistenceStore.saveWorkspaceSnapshots(from:), causing lost
per-workspace state for the last-closed window; change the branch in
writeAllSnapshotsBlock so you persist per-workspace snapshots from the
pre-removal snapshot before taking the fast exit (i.e., call
SessionPersistenceStore.saveWorkspaceSnapshots(from: snapshot) prior to or
immediately after invoking writePrimarySnapshotBlock() in the snapshot == nil
branch), or alternatively move the saveSessionSnapshot call in
unregisterMainWindow(_:) earlier so the closing workspaces are still present
when writeAllSnapshotsBlock runs; refer to writeAllSnapshotsBlock,
writePrimarySnapshotBlock(),
SessionPersistenceStore.saveWorkspaceSnapshots(from:), and
unregisterMainWindow(_:) when making the change.
In `@Sources/SessionPersistence.swift`:
- Around line 380-387: The file declares nominal types with invalid
`nonisolated` modifiers; remove the `nonisolated` keyword from the type
declarations for WorkspaceSessionSnapshotEnvelope and SessionSnapshotWriteResult
so they are declared as normal `struct WorkspaceSessionSnapshotEnvelope:
Codable, Sendable { ... }` and `enum SessionSnapshotWriteResult: Equatable,
Sendable { ... }`, preserving their existing conformances and members; do not
change any nested method/ property `nonisolated` uses (if present) — only delete
the `nonisolated` before the `struct` and `enum` keywords.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6de7020-7894-4566-8174-ba8098477640
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/SessionPersistence.swiftcmuxTests/SessionPersistenceTests.swift
|
Follow-up pushed in Changes:
Verification:
Local blocker remains unchanged:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 48023
✅
|
| Item | Status |
|---|---|
nonisolated removed from WorkspaceSessionSnapshotEnvelope |
✅ |
nonisolated removed from SessionSnapshotWriteResult |
✅ |
Stable workspaceSessionRootDirectory lookup key |
✅ Set in Workspace.init, stamped canonical in saveWorkspaceSnapshot, used by saveWorkspaceSnapshots |
Live currentDirectory preserved in snapshot |
✅ Key and payload no longer conflict |
Closing window snapshot captured before unregisterMainWindowContext |
✅ Last-window-close data loss is fixed |
persistWorkspaceSessionSnapshots helper extracted |
✅ Async path correct for non-termination closes |
sessionWindowSnapshot(for:...) helper extracted |
✅ Removes duplicate inline construction |
| New regression tests | ✅ Batch stable-root save + live-directory preservation |
🟡 Minor: double canonicalization in saveWorkspaceSnapshot
Not a bug, but worth noting: saveWorkspaceSnapshot calls canonicalWorkspaceDirectoryPath(workingDirectory:) to produce canonicalDirectory, then passes canonicalDirectory to workspaceSnapshotFileURL(workingDirectory:...) which calls canonicalWorkspaceDirectoryPath again internally. The second call is a no-op on an already-canonical path, but it is redundant work on every save. Low priority, but an easy cleanup if the function signature ever gains an "already-canonical" fast path.
🟡 Reminder: two previously-raised follow-up items (not blockers)
- Stale snapshot accumulation — per-directory
session.jsonfiles are never pruned when directories are renamed/deleted. Opening a follow-up issue is the suggested path. - Silent skip for remote/virtual paths —
canonicalWorkspaceDirectoryPathreturnsnilfor non-local paths; a debug-levelos_logon that return would help diagnose unexpected persistence skips.
Would you like me to open a GitHub issue to track the stale snapshot pruning work?
✅ Summary table (all rounds)
| Concern | Status |
|---|---|
nonisolated on struct/enum declarations |
✅ Resolved |
| File I/O off main actor | ✅ Task.detached(priority: .utility) |
| Pristine-workspace guard | ✅ title + cwd + panelIds |
| Explicit title preservation after restore | ✅ setCustomTitle(explicitTitle) |
Sync writes on sessionPersistenceQueue |
✅ writeAllSnapshotsBlock |
createdAt reuse / dedup |
✅ existingEnvelope check |
| Config/layout opt-out | ✅ restoreWorkspaceSession: false |
| Workspace snapshots gated on primary save | ✅ .failed guard |
| Git probe re-registration after restore | ✅ clearWorkspaceGitProbes + refresh |
| Last-window-close data loss | ✅ Snapshot captured before unregisterMainWindowContext |
Stable root key vs live currentDirectory |
✅ workspaceSessionRootDirectory field |
| Regression coverage | ✅ 9 new test cases across 2 files |
The implementation is ready for merge. 🎉
(∩^ᗜ^)⊃━ ✦ ✦ ✦
✏️ Learnings added
Learnt from: Fewmanism
URL: https://github.com/manaflow-ai/cmux/pull/4300
Timestamp: 2026-05-18T11:34:34.448Z
Learning: In the cmux repository (Swift/macOS), `nonisolated` is an actor-member modifier and cannot be applied to nominal type declarations (`struct`, `enum`, `class`). Structs and enums conforming to `Sendable` are already implicitly nonisolated. Applying `nonisolated` to a struct or enum declaration produces a Swift compiler warning under strict concurrency checking and should be removed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
@coderabbitai resolve All latest CodeRabbit inline comments have been addressed in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 26821
One note: there are 5 unresolved threads in the PR from
The last three of those were directly addressed in your follow-up commits ( (´｡• ᵕ •｡`) ♡ [resolve] |
|
@coderabbitai full review Please re-review the latest head |
🧩 Analysis chain🏁 Script executed: Length of output: 48023
✅ All previously raised actionable items are resolved
🟡 Minor carry-over items (not blockers)Double canonicalization in
Stale snapshot accumulation — per-directory Silent skip for remote/virtual paths — 🟢 Polish (same as prior rounds)
✅ Implementation is ready for mergeAll correctness concerns have been addressed. The minor items above are polish/follow-up material with no correctness impact. Would you like me to open a GitHub issue to track the stale snapshot pruning strategy? ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/TabManagerSessionSnapshotTests.swift`:
- Around line 193-197: The assertions that "no restore applied" run after only a
short run loop slice and can miss a late async restore; change the test to wait
the full configured restore timeout before asserting by waiting for the actual
timeout used by the restore logic (or use an XCTestExpectation and XCTWaiter
with that timeout) so the test observes that no restore arrived; update the
occurrences around the RunLoop.current.run(...) call and the checks of
workspace.customTitle, workspace.customDescription, workspace.currentDirectory
(also apply same fix to the other occurrence at lines 233-235) to use the full
timeout-based wait/expectation instead of the 0.2s run loop slice.
In `@Sources/TabManager.swift`:
- Around line 2294-2307: After calling
workspace.restoreSessionSnapshot(restoredSnapshot) (and after
workspace.setCustomTitle if present), explicitly refresh the window title so
async restore doesn't leave the bootstrap title displayed; invoke the existing
window-title update path for this workspace (i.e. re-run whatever logic that the
selectedTabId side effects trigger—call the function that recomputes/applies the
window title for the workspace, e.g. updateWindowTitle(forWorkspaceId:
workspace.id) or the equivalent method used elsewhere) immediately after the
restore and custom title handling; keep the clearWorkspaceGitProbes(...) and
scheduleInitialWorkspaceGitMetadataRefreshIfPossible(...) calls intact.
In `@Sources/Workspace.swift`:
- Around line 268-271: The restore logic ignores nil/blank values and therefore
never clears the stored workspaceSessionRootDirectory; update the handling of
snapshot.workspaceSessionRootDirectory so that when the snapshot value is nil or
trims to an empty string you explicitly clear workspaceSessionRootDirectory (set
it to nil or the empty-state the property uses), and when it contains non-empty
trimmed text assign that trimmed value to workspaceSessionRootDirectory (use
snapshot.workspaceSessionRootDirectory, normalizedSessionRootDirectory, and
workspaceSessionRootDirectory to locate and adjust the code).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b57c1e2a-2bfa-4ea1-a60f-62a8906c3bf4
📒 Files selected for processing (8)
Sources/AppDelegate.swiftSources/CmuxConfigExecutor.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/TabManagerSessionSnapshotTests.swift
|
Follow-up pushed in Changes:
Verification:
Note: @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 49219
✅ All changes in
|
| Item | Status |
|---|---|
Window title refresh after async restore (updateWindowTitle) |
✅ Correctly guarded on selectedTabId == workspace.id |
restoreSessionSnapshot clears workspaceSessionRootDirectory for nil/blank |
✅ Explicit else { workspaceSessionRootDirectory = nil } branch |
closingWorkspaceSnapshot captured before unregisterMainWindowContext |
✅ Correct ordering |
persistWorkspaceSessionSnapshots(from:synchronously:) helper |
✅ Clean dispatch to sessionPersistenceQueue |
sessionWindowSnapshot(for:...) extraction removes inline duplication |
✅ |
| Negative async-restore tests strengthened (poll full timeout) | ✅ deadline-based loop + final predicate check |
testWorkspaceRestoreClearsMissingSessionRootDirectory |
✅ Tests restoreSessionSnapshot in isolation, no async dependency needed |
nonisolated on struct/enum declarations |
✅ Already resolved in 9a0dc2bd9, not re-introduced here |
🟡 Carry-over minor items (not blockers, unchanged from prior rounds)
- Double canonicalization in
saveWorkspaceSnapshot→workspaceSnapshotFileURL(redundant call, no correctness impact). - Stale snapshot accumulation — per-directory
session.jsonfiles in Application Support are never pruned. - Silent skip for remote/virtual paths —
canonicalWorkspaceDirectoryPathreturnsnilwithout logging. - Test name
testInitialWorkspaceRestoreCanBeDisabledForExplicitLayouts— tests therestoreWorkspaceSession: falseflag, not a layout path directly. Minor naming inconsistency. - Busy-polling in
waitForWorkspaceSessionRestore—XCTestExpectationwould give cleaner failure messages; low priority.
✅ Summary (all rounds)
| Concern | Status |
|---|---|
nonisolated on nominal type declarations |
✅ Resolved |
| File I/O off main actor | ✅ Task.detached(priority: .utility) |
| Pristine-workspace guard | ✅ title + cwd + panelIds |
| Explicit title preservation after restore | ✅ setCustomTitle(explicitTitle) |
| Window title UI refresh after restore | ✅ updateWindowTitle(for:) |
Sync writes on sessionPersistenceQueue |
✅ writeAllSnapshotsBlock |
createdAt reuse / dedup |
✅ existingEnvelope check |
| Config/layout opt-out | ✅ restoreWorkspaceSession: false |
| Workspace snapshots gated on primary save | ✅ != .failed guard |
| Git probe re-registration after restore | ✅ clearWorkspaceGitProbes + refresh |
| Last-window-close data loss | ✅ closingWorkspaceSnapshot before unregisterMainWindowContext |
Stable root key vs live currentDirectory |
✅ workspaceSessionRootDirectory field |
workspaceSessionRootDirectory cleared on nil/blank restore |
✅ |
| Regression coverage | ✅ 10 new test cases across 2 files |
The implementation is ready for merge. Would you like me to open a GitHub issue to track the stale snapshot pruning work?
|
@coderabbitai resolve Latest head |
|
Regarding the formal (ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ ✅ Actions performedComments resolved and changes approved. |
teamleaderleo
added
S2: major
This branch has not been deployed
2 participants
Summary
Test Plan
git diff --checkswiftc -parse Sources/SessionPersistence.swift Sources/TabManager.swift Sources/AppDelegate.swift Sources/CmuxConfigExecutor.swift Sources/TerminalController.swift cmuxTests/SessionPersistenceTests.swift cmuxTests/TabManagerSessionSnapshotTests.swiftxcodebuild testcould not run locally because the active developer directory is CommandLineTools, not a full Xcode install.Notes
Summary by cubic
Persist per-directory workspace sessions and auto-restore them when opening that directory. Keys are stable across subdirectory changes; restore runs async with safety checks.
New Features
workspaceSessionRootDirectorystored in snapshots; batch saves prefer this root over the livecurrentDirectory.workingDirectoryand no initial command/input/env; runs off the main thread, verifies the workspace didn’t change (title/cwd/panels), preserves explicit titles, updates the window title, refreshes Git metadata, and falls back to Welcome if nothing to restore.restoreWorkspaceSession: false(used for config/layout flows).Migration
TabManager.init/addWorkspaceadd:workspaceSessionAppSupportDirectory,workspaceSessionBundleIdentifier,restoreWorkspaceSession(defaulttrue).TerminalControllerrestores only for non-layout workspaces;CmuxConfigExecutordisables restore for config-driven workspaces.Written for commit c20f276. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Behavior
Tests