Repository navigation
Remove compact workspace row tap gestures - #6124
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThree small navigation fixes in ChangesPush Navigation Gesture Cleanup and Stale-State Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (20 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 |
e412177 to
09cefcb
Compare
Greptile SummaryRemoves redundant
Confidence Score: 5/5Safe to merge. Removes a redundant gesture that was fighting NavigationStack's own path management, and correctly relocates the pending-create state cleanup into the path observer. All three files make small, focused removals or guard-clause splits. The only mutation path in compact mode is now the NavigationStack path observer, which was already properly guarded against loops. No new state, no new concurrency, no user-facing strings, and the sidebar code path is untouched. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant NavigationLink
participant compactNavigationPath
participant onChange_path as onChange(compactNavigationPath)
participant Store as store.selectedWorkspaceID
participant onChange_sel as onChange(selectedWorkspaceID)
Note over User,onChange_sel: Before PR — double-update via simultaneousGesture
User->>NavigationLink: tap
NavigationLink->>compactNavigationPath: append workspaceID
NavigationLink-->>Store: simultaneousGesture calls selectWorkspace()
NavigationLink-->>compactNavigationPath: selectWorkspace() also resets compactNavigationPath
Note over User,onChange_sel: After PR — single path via NavigationStack
User->>NavigationLink: tap
NavigationLink->>compactNavigationPath: append workspaceID
compactNavigationPath->>onChange_path: fires
onChange_path->>onChange_path: clear pendingCompactCreateNavigationWorkspaceIDs
onChange_path->>Store: set selectedWorkspaceID (if changed)
Store->>onChange_sel: fires
onChange_sel->>compactNavigationPath: pathForSelectionChange (returns same path — no-op)
Reviews (1): Last reviewed commit: "Remove compact workspace row tap gesture..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swift (1)
113-122:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffMove pending-intent clearing before the path-empty guard to prevent stale navigation on pop-to-root.
The current order clears
pendingCompactCreateNavigationWorkspaceIDsonly whenpath.lastis non-nil. When the user pops back to the root (empty path), the early return on line 115 skips the clearing on line 117, leaving the pending one-shot creation intent set.Concrete failure mode: User taps "Create workspace," then pops back to the root before the async creation completes. The pending set remains. When creation finishes and
selectedWorkspaceIDchanges to the new workspace, the.onChange(of: selectedWorkspaceID)observer (lines 97–106) detects a "created workspace," clears the pending set, and pushes[newWorkspaceID]onto the path — unexpectedly navigating the user away from the root they just chose.Pop-to-root is an explicit navigation action that should clear the pending intent, just as
selectWorkspace(_:)(line 183) clears it on any manual selection.🔧 Proposed fix: clear pending intent before checking path
.onChange(of: compactNavigationPath) { _, path in + pendingCompactCreateNavigationWorkspaceIDs = nil guard let selectedWorkspaceID = path.last else { return } - pendingCompactCreateNavigationWorkspaceIDs = nil guard store.selectedWorkspaceID != selectedWorkspaceID else { return } store.selectedWorkspaceID = selectedWorkspaceID }🤖 Prompt for 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. In `@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swift` around lines 113 - 122, The pending creation intent clearing in the onChange closure for compactNavigationPath must happen before checking if the path is empty, not after. Move the line pendingCompactCreateNavigationWorkspaceIDs = nil to execute before the guard let selectedWorkspaceID = path.last check, so that pop-to-root navigation (empty path) properly clears the pending intent and prevents stale navigation when creation completes asynchronously.
🤖 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.
Outside diff comments:
In
`@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swift`:
- Around line 113-122: The pending creation intent clearing in the onChange
closure for compactNavigationPath must happen before checking if the path is
empty, not after. Move the line pendingCompactCreateNavigationWorkspaceIDs = nil
to execute before the guard let selectedWorkspaceID = path.last check, so that
pop-to-root navigation (empty path) properly clears the pending intent and
prevents stale navigation when creation completes asynchronously.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 678bbb1f-befd-40b4-9f41-77028384c593
📒 Files selected for processing (3)
Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceGroupHeaderRow.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceNavigationRow.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swift
💤 Files with no reviewable changes (2)
- Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceNavigationRow.swift
- Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceGroupHeaderRow.swift
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
This branch was successfully deployed
1 participant
Summary
simultaneousGesture(TapGesture())from compact workspace rows and group headers.NavigationStack(path:)drive compact navigation and syncselectedWorkspaceIDfrom the parent path observer.Testing
./scripts/reload-cloud.sh --tag ntap(cloud unavailable, local tagged macOS build succeeded)ios/scripts/reload.sh --tag ntap(iOS Simulator build/install succeeded on iPhone 17)swift test --package-path Packages/CmuxMobileShellUI(blocked before compile: missing localGhosttyKit.xcframeworkartifact in fresh worktree)Notes
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Removed redundant tap gestures from compact workspace rows and group headers so
NavigationLinkfully controls navigation. This stops duplicate taps and keeps compact selection in sync with the parent navigation path..simultaneousGesture(TapGesture())from compact rows and headers to avoid duplicate selection.NavigationStack(path:)updates drive compact selection; updateselectedWorkspaceIDfromcompactNavigationPath.pendingCompactCreateNavigationWorkspaceIDson any compact path update to avoid stale navigation state.Written for commit 09cefcb. Summary will update on new commits.
Summary by CodeRabbit