Repository navigation
Fix crash diagnostic window restore - #6596
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 a new ChangesCrash-Diagnostic Window Pruning
Sequence Diagram(s)sequenceDiagram
participant AppDelegate
participant SessionPersistencePolicy
participant GhosttyCrashBreadcrumb
participant FileSystem
rect rgba(135, 206, 250, 0.5)
Note over AppDelegate,SessionPersistencePolicy: Startup / Restore
AppDelegate->>SessionPersistencePolicy: pruningCmuxCrashDiagnosticWindows(from: snapshot)
SessionPersistencePolicy->>FileSystem: standardize & compare path components
FileSystem-->>SessionPersistencePolicy: crash directory match result
SessionPersistencePolicy-->>AppDelegate: (prunedSnapshot, removedCrashDiagnosticState)
end
rect rgba(144, 238, 144, 0.5)
Note over AppDelegate,SessionPersistencePolicy: External Open / File Request
AppDelegate->>SessionPersistencePolicy: isCmuxCrashStoragePath(directory)
SessionPersistencePolicy-->>AppDelegate: true → filter out directory
AppDelegate->>SessionPersistencePolicy: isCmuxCrashStorageURL(fileURL)
SessionPersistencePolicy-->>AppDelegate: true → skip file URL
end
rect rgba(255, 182, 193, 0.5)
Note over GhosttyCrashBreadcrumb,SessionPersistencePolicy: Multi-directory crash detection
GhosttyCrashBreadcrumb->>SessionPersistencePolicy: cmuxCrashDirectoryURLs()
SessionPersistencePolicy-->>GhosttyCrashBreadcrumb: [defaultURL, xdgURL, ...]
GhosttyCrashBreadcrumb->>FileSystem: scan each directory for ghosttycrash files
FileSystem-->>GhosttyCrashBreadcrumb: latest crash per directory
GhosttyCrashBreadcrumb-->>GhosttyCrashBreadcrumb: select newest across all directories
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 22 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (22 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR fixes a bug where Ghostty crash reports stored under cmux's own crash directory (
Confidence Score: 4/5Safe to merge after addressing the window-close misclassification; the rest of the fix is well-structured and covered by tests. The startup-restore, session-save, routing-guard, and marker-management paths all look correct and are backed by behavioral tests. One concrete defect exists in the window-close path: Sources/AppDelegate.swift — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Window/File Open] -->|file URL| B{isCmuxCrashStorageURL?}
B -->|yes| C[Reject – return nil]
B -->|no| D[Normal open routing]
E[App Startup] --> F[syncManualRestoreSnapshotCachePruningCrashDiagnostics]
F -->|primary loaded| G[pruningCmuxCrashDiagnosticWindows]
G -->|has non-crash windows| H[Save pruned → manual-restore backup]
G -->|all-crash| I[Keep old backup as-is, clear crashOnly marker]
F -->|primary missing + marker set| J[Keep backup for next-launch restore]
F -->|primary unusable| K[Clear crashOnly marker]
E --> L[loadStartupSessionSnapshotPruningCrashDiagnostics]
L -->|primary loaded & pruned OK| M[Use pruned snapshot]
L -->|primary all-crash| N[Fall back to manual-restore backup]
L -->|primary missing + marker| N
O[Window Close] --> P{isCrashDiagnostic? includeScrollback:false}
P -->|yes – all crash| Q[Skip undo-close history, removeWhenEmpty=true, preserveBackup marker]
P -->|no| R[recordClosedWindowHistoryIfNeeded includeScrollback:true]
S[Session Save] --> T[buildSessionSnapshotResult includeScrollback:false]
T --> U[pruningCmuxCrashDiagnosticWindows per window]
U -->|removedCrashDiagnosticState| V[persistSessionSnapshot, markCrashOnly if empty]
U -->|kept non-crash windows| W[Save normal snapshot, clear crashOnly marker]
%%{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"}}}%%
flowchart TD
A[Window/File Open] -->|file URL| B{isCmuxCrashStorageURL?}
B -->|yes| C[Reject – return nil]
B -->|no| D[Normal open routing]
E[App Startup] --> F[syncManualRestoreSnapshotCachePruningCrashDiagnostics]
F -->|primary loaded| G[pruningCmuxCrashDiagnosticWindows]
G -->|has non-crash windows| H[Save pruned → manual-restore backup]
G -->|all-crash| I[Keep old backup as-is, clear crashOnly marker]
F -->|primary missing + marker set| J[Keep backup for next-launch restore]
F -->|primary unusable| K[Clear crashOnly marker]
E --> L[loadStartupSessionSnapshotPruningCrashDiagnostics]
L -->|primary loaded & pruned OK| M[Use pruned snapshot]
L -->|primary all-crash| N[Fall back to manual-restore backup]
L -->|primary missing + marker| N
O[Window Close] --> P{isCrashDiagnostic? includeScrollback:false}
P -->|yes – all crash| Q[Skip undo-close history, removeWhenEmpty=true, preserveBackup marker]
P -->|no| R[recordClosedWindowHistoryIfNeeded includeScrollback:true]
S[Session Save] --> T[buildSessionSnapshotResult includeScrollback:false]
T --> U[pruningCmuxCrashDiagnosticWindows per window]
U -->|removedCrashDiagnosticState| V[persistSessionSnapshot, markCrashOnly if empty]
U -->|kept non-crash windows| W[Save normal snapshot, clear crashOnly marker]
Reviews (11): Last reviewed commit: "fix: restore backup after closing crash ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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`+CmuxSSHURL.swift:
- Around line 194-195: The crash-storage exclusion check using
SessionPersistencePolicy.isCmuxCrashStorageURL() is currently applied to
standardizedURL before symlinks are resolved, which allows symlinks pointing
into crash storage to bypass the check. Move the symlink resolution using
resolvingSymlinksInPath() before the guard statement, then apply the
SessionPersistencePolicy.isCmuxCrashStorageURL() check to the symlink-resolved
URL instead of standardizedURL to ensure symlinks are properly validated.
In `@Sources/SessionPersistence.swift`:
- Around line 2163-2166: The anchorWorkspaceId assignment in the code around the
anchorMemberIndex update should be modified to reference the actual kept
anchor's workspace ID instead of the originalAnchorWorkspaceId when the original
anchor has been pruned. After determining the anchorMemberIndex using the
flatMap and fallback to 0, set anchorWorkspaceId to the workspaceId of the
member at that index in keptMembers. This ensures consistency between
anchorMemberIndex and anchorWorkspaceId when the original anchor is no longer in
the kept members list.
🪄 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: a6004eaf-b5ad-42c2-9b29-c2ecd4d9b085
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Sources/AppDelegate+CmuxSSHURL.swiftSources/AppDelegate.swiftSources/GhosttyCrashBreadcrumb.swiftSources/SessionPersistence.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/WindowAndDragTests.swift
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/SessionPersistencePolicy`+CrashStorage.swift:
- Around line 227-230: The compactMap loop over groups repeatedly filters the
same originalWorkspaces and keptWorkspaces collections, resulting in O(groups ×
workspaces) complexity. Before the compactMap operation on groups, create two
dictionaries that map group IDs to their respective workspace members (one for
originalWorkspaces grouped by groupId and one for keptWorkspaces grouped by
groupId). Then replace the filter operations inside the compactMap with direct
dictionary lookups using group.id as the key to achieve constant-time access
instead of repeated collection scans.
🪄 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: 73d9b811-de0e-46a9-a6db-63371a0f5227
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
Sources/SessionPersistencePolicy+CrashStorage.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CrashDiagnosticSessionPolicyTests.swift
# Conflicts: # cmux.xcodeproj/project.pbxproj
Contributor
Author
|
Addressed the CodeRabbit Cmux Swift @Concurrent note in b0353d5 by using @Concurrent on compiler >= 6.2 with the existing @sendable fallback for older compilers. The current CodeRabbit and Greptile checks are green; the remaining docstring item is CodeRabbit's optional generated warning and this change adds no public package API requiring DocC coverage. |
Closed
This branch was successfully deployed
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
1 participant
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.
Closes #6593
Repro / investigation
gh issue view; the report is cmux 0.64.16 (96) on macOS 26.5.1 after DMG update/relaunch opening an extra window rooted at.../state/cmux/crash, and closing that window does not stick.Root cause
~/.local/state/cmux/crash, or$XDG_STATE_HOME/cmux/crash)..../state/cmux/crashcould become ordinary restorable app state.Fix
Tests / validation
cmux.xcodeproj.git diff --check,./scripts/check-pbxproj.sh,./scripts/lint-pbxproj-test-wiring.sh,python3 scripts/check-package-resolved-policy.py,python3 scripts/swift_file_length_budget.py, andxcrun swiftc -parse -parse-as-library Sources/AppDelegate.swift Sources/AppDelegate+CrashSessionSnapshotRemoval.swift Sources/SessionPersistencePolicy+CrashStorage.swift cmuxTests/CrashDiagnosticSessionPolicyTests.swift.reload.sh, launch a dev app, or run barexcodebuild. CI is the app/test build gate.Localization