fix: restore terminal working directories on relaunch - #2171
jorgitin02 wants to merge 6 commits into
Conversation
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
@jorgitin02 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughImplements split-resize logic in TabManager with tree traversal and divider-position updates, threads focused panel ID through workspace restore to resolve terminal working directories, and adds unit tests for split resizing and session persistence behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TabManager
participant BonSplit as BonSplit Controller
participant Tree as ExternalTreeNode
Client->>TabManager: resizeSplit(tabId, surfaceId, direction, amount)
TabManager->>TabManager: Validate amount > 0
TabManager->>TabManager: Resolve target pane for surfaceId
TabManager->>Tree: treeSnapshot()
Tree-->>TabManager: tree
TabManager->>TabManager: resizeSplitCollectCandidates(node, targetPaneId)
loop traverse tree
TabManager->>Tree: inspect node bounds & membership
TabManager->>TabManager: append ResizeSplitCandidate when split found
end
TabManager->>TabManager: filter candidates by orientation & child side
TabManager->>TabManager: compute divider delta (amount / axisPixels) * sign
TabManager->>TabManager: clamp to [0.1,0.9]
TabManager->>BonSplit: setDividerPosition(newPos, forSplit, fromExternal: true)
BonSplit-->>Client: success
sequenceDiagram
participant Client
participant Workspace
participant Snapshot as Session Snapshot
participant Creator as Panel Creator
Client->>Workspace: restoreSession(snapshot)
Workspace->>Snapshot: read focusedPanelId
Snapshot-->>Workspace: focusedPanelId
loop for each pane
Workspace->>Workspace: restorePane(..., workspaceFocusedPanelId)
Workspace->>Creator: createPanel(..., workspaceFocusedPanelId)
alt panel is terminal & focused
Creator->>Creator: use snapshot.currentDirectory as workingDirectory
else
Creator->>Creator: use panel.snapshot.terminal?.workingDirectory or snapshot.directory
end
Creator-->>Workspace: panel created
end
Workspace-->>Client: restored
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 unit tests (beta)
⚔️ Resolve merge conflicts
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 addresses session CWD restore for relaunched terminals and simultaneously ships the full Session restore CWD change (
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[restoreSessionSnapshot] --> B[Set currentDirectory from snapshot.currentDirectory]
B --> C[restorePane for each layout leaf]
C --> D[createPanel from snapshot + workspaceFocusedPanelId]
D --> E{snapshot.terminal?.workingDirectory?}
E -- set --> F[use workingDirectory]
E -- nil --> G{snapshot.directory?}
G -- set --> F
G -- nil --> H{snapshot.id == workspaceFocusedPanelId?}
H -- yes --> I[use currentDirectory]
H -- no --> J[nil → ?? currentDirectory]
J --> I
I --> K[newTerminalSurface with workingDirectory]
style H fill:#ffe0b2,stroke:#f57c00
style J fill:#ffe0b2,stroke:#f57c00
Reviews (1): Last reviewed commit: "fix: restore terminal working directorie..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let workingDirectory = snapshot.terminal?.workingDirectory | ||
| ?? snapshot.directory | ||
| ?? (snapshot.id == workspaceFocusedPanelId ? currentDirectory : nil) | ||
| ?? currentDirectory |
There was a problem hiding this comment.
The ?? (snapshot.id == workspaceFocusedPanelId ? currentDirectory : nil) line evaluates to either currentDirectory or nil. But the immediately following ?? currentDirectory supplies currentDirectory for the nil branch as well, meaning every panel — focused or not — ends up with currentDirectory when neither snapshot.terminal?.workingDirectory nor snapshot.directory is set. The conditional branch buys nothing over the old two-fallback chain.
If the intent is to prevent non-focused panels from inheriting the workspace's (focused panel's) directory when their own CWD was never saved, the final catch-all must be replaced with a different fallback (e.g. the workspace's startup requestedDirectory). As written, the semantics are identical to the pre-PR code:
// current (effectively equivalent to the old code for all panels)
let workingDirectory = snapshot.terminal?.workingDirectory
?? snapshot.directory
?? (snapshot.id == workspaceFocusedPanelId ? currentDirectory : nil)
?? currentDirectoryEither simplify back to the two-fallback form, or replace the trailing ?? currentDirectory with a distinct value so the workspaceFocusedPanelId guard actually differentiates the two cases.
| let requestedDirectory = root.appendingPathComponent("requested", isDirectory: true) | ||
| let restoredDirectory = root.appendingPathComponent("restored", isDirectory: true) | ||
| try FileManager.default.createDirectory(at: requestedDirectory, withIntermediateDirectories: true) | ||
| try FileManager.default.createDirectory(at: restoredDirectory, withIntermediateDirectories: true) | ||
|
|
||
| var snapshot = makeSnapshot(version: SessionSnapshotSchema.currentVersion).windows[0].tabManager.workspaces[0] | ||
| let panelId = UUID() | ||
| snapshot.focusedPanelId = panelId | ||
| snapshot.currentDirectory = restoredDirectory.path | ||
| snapshot.layout = .pane(SessionPaneLayoutSnapshot(panelIds: [panelId], selectedPanelId: panelId)) | ||
| snapshot.panels = [ | ||
| SessionPanelSnapshot( | ||
| id: panelId, | ||
| type: .terminal, | ||
| title: "Terminal", | ||
| customTitle: nil, | ||
| directory: nil, | ||
| isPinned: false, | ||
| isManuallyUnread: false, | ||
| gitBranch: nil, | ||
| listeningPorts: [], | ||
| ttyName: nil, | ||
| terminal: SessionTerminalPanelSnapshot( | ||
| workingDirectory: nil, | ||
| scrollback: nil | ||
| ), | ||
| browser: nil, | ||
| markdown: nil | ||
| ) | ||
| ] | ||
|
|
||
| let restored = Workspace(title: "Terminal", workingDirectory: requestedDirectory.path, portOrdinal: 0) | ||
| restored.restoreSessionSnapshot(snapshot) | ||
|
|
||
| let restoredFocusedPanelId = try XCTUnwrap(restored.focusedPanelId) | ||
| let restoredTerminal = try XCTUnwrap(restored.terminalPanel(for: restoredFocusedPanelId)) | ||
| XCTAssertEqual(restored.currentDirectory, restoredDirectory.path) | ||
| XCTAssertEqual( | ||
| restoredTerminal.requestedWorkingDirectory, | ||
| restoredDirectory.path, | ||
| "Expected restored terminals to use the workspace snapshot cwd when no panel-specific cwd was persisted" | ||
| ) | ||
| } |
There was a problem hiding this comment.
testWorkspaceSessionSnapshotRestoresFocusedTerminalWorkingDirectoryWhenPanelDirectoryIsMissing builds a snapshot where terminal.workingDirectory == nil and directory == nil, then asserts that restoredTerminal.requestedWorkingDirectory == restoredDirectory.path.
In restoreSessionSnapshot, currentDirectory is updated to snapshot.currentDirectory (restoredDirectory.path) before createPanel is called. The original pre-PR fallback chain was:
let workingDirectory = snapshot.terminal?.workingDirectory ?? snapshot.directory ?? currentDirectoryWith both optional fields nil, this already resolves to currentDirectory — which is already restoredDirectory.path at that point. The assertion in the test would therefore pass against the unmodified code, meaning this test does not go red on the first commit as CLAUDE.md's regression-test policy requires.
Either the test is covering the wrong scenario (it should simulate the case where currentDirectory was not updated, or where snapshot.currentDirectory is empty), or the actual bug lies elsewhere and the fix should target that path. Consider revisiting the test setup so it reliably fails on the old code.
![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
🧹 Nitpick comments (1)
cmuxTests/SessionPersistenceTests.swift (1)
60-63: Tighten the focused-panel assertions to prevent false positives.The snapshot test resolves
snapshot.panels.first, and the restore test uses a single terminal. A regression that appliessnapshot.currentDirectoryto every terminal, or a panel-ordering change, would still pass. Resolve the persisted panel bysnapshot.focusedPanelIdand add a non-focused terminal withworkingDirectory == nil, then assert only the focused terminal picks uprestoredDirectory.path.Also applies to: 78-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/SessionPersistenceTests.swift` around lines 60 - 63, The test currently inspects snapshot.panels.first which can mask regressions; instead find the persisted panel by matching snapshot.focusedPanelId (use snapshot.panels.first { $0.id == snapshot.focusedPanelId }) and assert that its terminal's workingDirectory equals restoredDirectory.path, while adding a second (non-focused) terminal in the test setup with workingDirectory == nil and asserting that that non-focused terminal's workingDirectory remains nil; apply the same change pattern to the other assertions in the 78-114 block so only the focused panel is expected to receive snapshot.currentDirectory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 547-550: The fallback to currentDirectory is currently applied
unconditionally, nullifying the workspaceFocusedPanelId check; change the
expression for workingDirectory so the workspace cwd (currentDirectory) is only
used when snapshot.id == workspaceFocusedPanelId by removing the final
unconditional "?? currentDirectory" and making the ternary the last fallback,
e.g. keep snapshot.terminal?.workingDirectory ?? snapshot.directory ??
(snapshot.id == workspaceFocusedPanelId ? currentDirectory : nil);
alternatively, if the global fallback is intended, remove
workspaceFocusedPanelId plumbing (references to workspaceFocusedPanelId)
instead.
---
Nitpick comments:
In `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 60-63: The test currently inspects snapshot.panels.first which can
mask regressions; instead find the persisted panel by matching
snapshot.focusedPanelId (use snapshot.panels.first { $0.id ==
snapshot.focusedPanelId }) and assert that its terminal's workingDirectory
equals restoredDirectory.path, while adding a second (non-focused) terminal in
the test setup with workingDirectory == nil and asserting that that non-focused
terminal's workingDirectory remains nil; apply the same change pattern to the
other assertions in the 78-114 block so only the focused panel is expected to
receive snapshot.currentDirectory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba0956f1-3d31-4396-ae84-ca7a7011759a
📒 Files selected for processing (4)
Sources/TabManager.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/TabManagerUnitTests.swift
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e26e5f155
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let workingDirectory = snapshot.terminal?.workingDirectory | ||
| ?? snapshot.directory | ||
| ?? (snapshot.id == workspaceFocusedPanelId ? currentDirectory : nil) | ||
| ?? currentDirectory |
There was a problem hiding this comment.
This fallback chain is effectively unconditional because it ends with ?? currentDirectory, so non-focused terminals still restore to the workspace cwd whenever both terminal.workingDirectory and directory are missing. In practice, snapshots with multiple panels missing per-panel cwd will still assign the focused panel’s directory to other panels, so the new workspaceFocusedPanelId check does not change behavior as intended.
Useful? React with 👍 / 👎.
![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 4 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/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:550">
P2: The new focused-panel fallback is ineffective because the unconditional `?? currentDirectory` makes the branch a no-op. This leaves restore behavior unchanged and the new parameter unused in practice.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
Pushed follow-up in to address the restore-path issue and tighten the regression tests.\n\nChanges:\n- scoped workspace cwd fallback to the focused restored terminal only\n- strengthened session-persistence coverage to resolve panels by \n- added a second restored terminal assertion so non-focused panels keep their own persisted directory\n\nVerification:\n- |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1852c3c6cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| restored.restoreSessionSnapshot(snapshot) | ||
|
|
||
| let restoredFocusedPanel = try XCTUnwrap(restored.terminalPanel(for: try XCTUnwrap(restored.focusedPanelId))) | ||
| let restoredUnfocusedPanel = try XCTUnwrap(restored.terminalPanel(for: unfocusedPanelId)) |
There was a problem hiding this comment.
restoreSessionSnapshot creates new panel UUIDs and tracks old→new IDs internally, so unfocusedPanelId from the snapshot is stale after restore. Looking up restored.terminalPanel(for: unfocusedPanelId) will almost always return nil, causing this regression test to fail before it validates the non-focused cwd behavior; the assertion should use a restored panel ID (for example via the restored layout/selection) instead of the pre-restore UUID.
Useful? React with 👍 / 👎.
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)
Sources/Workspace.swift (1)
227-233:⚠️ Potential issue | 🟠 MajorUse an immutable snapshot cwd for restore, not the live
currentDirectory.Restore can still pick the wrong cwd here.
createTab(...)selects internally, so earlier tabs recreated in the focused pane can temporarily becomefocusedPanelId, andapplySessionPanelMetadata(...)can overwritecurrentDirectorybefore the real focused snapshot panel is created. If the focused tab is not first in its pane, this fallback can relaunch it in another tab’s directory. Capture the normalized workspace snapshot directory once inrestoreSessionSnapshot, thread that immutable value throughrestorePane/createPanel, and seedpanelDirectoriesfrom the resolved launch cwd so a subsequent snapshot preserves it.💡 Proposed fix
func restoreSessionSnapshot(_ snapshot: SessionWorkspaceSnapshot) { restoredTerminalScrollbackByPanelId.removeAll(keepingCapacity: false) let normalizedCurrentDirectory = snapshot.currentDirectory.trimmingCharacters(in: .whitespacesAndNewlines) if !normalizedCurrentDirectory.isEmpty { currentDirectory = normalizedCurrentDirectory } + let workspaceSnapshotDirectory = currentDirectory let panelSnapshotsById = Dictionary(uniqueKeysWithValues: snapshot.panels.map { ($0.id, $0) }) let leafEntries = restoreSessionLayout(snapshot.layout) var oldToNewPanelIds: [UUID: UUID] = [:] for entry in leafEntries { restorePane( entry.paneId, snapshot: entry.snapshot, panelSnapshotsById: panelSnapshotsById, workspaceFocusedPanelId: snapshot.focusedPanelId, + workspaceSnapshotDirectory: workspaceSnapshotDirectory, oldToNewPanelIds: &oldToNewPanelIds ) } @@ private func restorePane( _ paneId: PaneID, snapshot: SessionPaneLayoutSnapshot, panelSnapshotsById: [UUID: SessionPanelSnapshot], workspaceFocusedPanelId: UUID?, + workspaceSnapshotDirectory: String?, oldToNewPanelIds: inout [UUID: UUID] ) { @@ guard let createdPanelId = createPanel( from: panelSnapshot, inPane: paneId, - workspaceFocusedPanelId: workspaceFocusedPanelId + workspaceFocusedPanelId: workspaceFocusedPanelId, + workspaceSnapshotDirectory: workspaceSnapshotDirectory ) else { continue } createdPanelIds.append(createdPanelId) oldToNewPanelIds[oldPanelId] = createdPanelId } @@ private func createPanel( from snapshot: SessionPanelSnapshot, inPane paneId: PaneID, - workspaceFocusedPanelId: UUID? + workspaceFocusedPanelId: UUID?, + workspaceSnapshotDirectory: String? ) -> UUID? { switch snapshot.type { case .terminal: let workingDirectory = snapshot.terminal?.workingDirectory ?? snapshot.directory - ?? (snapshot.id == workspaceFocusedPanelId ? currentDirectory : nil) + ?? (snapshot.id == workspaceFocusedPanelId ? workspaceSnapshotDirectory : nil) let replayEnvironment = SessionScrollbackReplayStore.replayEnvironment( for: snapshot.terminal?.scrollback ) @@ } applySessionPanelMetadata(snapshot, toPanelId: terminalPanel.id) + if let workingDirectory = workingDirectory { + panelDirectories[terminalPanel.id] = workingDirectory + } return terminalPanel.idAlso applies to: 492-510, 540-549
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 227 - 233, The restore path is reading the live currentDirectory and can pick the wrong cwd during recreation; capture the normalized snapshot cwd once in restoreSessionSnapshot and thread that immutable value into restorePane and createPanel (instead of reading Workspace.currentDirectory), seed panelDirectories from that resolved launch cwd, and ensure applySessionPanelMetadata uses the passed-in immutable cwd rather than mutating/reading currentDirectory so recreated tabs keep the intended directory; update calls at the restore sites (including the call replacing restorePane(entry.paneId, ...)) to pass the captured snapshot cwd through.
🤖 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 `@Sources/Workspace.swift`:
- Around line 227-233: The restore path is reading the live currentDirectory and
can pick the wrong cwd during recreation; capture the normalized snapshot cwd
once in restoreSessionSnapshot and thread that immutable value into restorePane
and createPanel (instead of reading Workspace.currentDirectory), seed
panelDirectories from that resolved launch cwd, and ensure
applySessionPanelMetadata uses the passed-in immutable cwd rather than
mutating/reading currentDirectory so recreated tabs keep the intended directory;
update calls at the restore sites (including the call replacing
restorePane(entry.paneId, ...)) to pass the captured snapshot cwd through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eacc8002-66c8-4e0f-8455-e7bc38082dcb
📒 Files selected for processing (2)
Sources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/SessionPersistenceTests.swift
1 participant
Summary
Testing
Summary by cubic
Restores terminal working directories on relaunch by falling back to the workspace snapshot when panel-specific data is missing. Addresses Linear 2125 and adds Ghostty-compatible split resizing with targeted regression tests.
Bug Fixes
New Features
Written for commit 0e26e5f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests