Repository navigation
Exit split zoom when jumping to unread - #1401
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughNotification-driven focus now returns success/failure: Tab resolution is eager, split-zoom is cleared before focus, and callers (AppDelegate, TerminalController) check the boolean result to handle failures (debug logging, test hooks, error reporting). Changes
Sequence Diagram(s)sequenceDiagram
participant NotificationCenter
participant AppDelegate
participant TabManager
participant Tab
participant Panel
Note over NotificationCenter,AppDelegate: Notification opened / user action
NotificationCenter->>AppDelegate: deliverNotification(tabId, surfaceId?)
AppDelegate->>TabManager: focusTabFromNotification(tabId, surfaceId)
TabManager->>TabManager: resolve tab by id
alt tab not found or panel missing
TabManager-->>AppDelegate: return false
AppDelegate->>AppDelegate: record failure (DEBUG) / write test data (if env)
else tab found
TabManager->>Tab: clearSplitZoom()
TabManager->>Panel: focus(panelId)
TabManager-->>AppDelegate: return true
AppDelegate->>NotificationCenter: mark notification read / follow-up actions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/TabManager.swift`:
- Line 2208: The guard in focusTabFromNotification is silently returning when it
can't find the tab, which lets callers like openNotificationInContext proceed to
mark notifications read incorrectly; change focusTabFromNotification to return a
Bool (true on success, false on failure), replace the silent guard (guard let
tab = ...) with a logged failure and return false, update the rest of the
function to return true on successful focus, and modify the caller
openNotificationInContext to check the Bool result and only mark notifications
as read when focusTabFromNotification returns true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a90d6ce-7fad-46b1-afd4-46914226c24f
📒 Files selected for processing (2)
Sources/TabManager.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
…-exits-cmd-shift-enter-mode Exit split zoom when jumping to unread
Summary
Testing
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-cmd-shift-u-exit-zoom-unit build./scripts/reload.sh --tag cmd-shift-u-exit-zoomTask
Summary by cubic
Cmd-Shift-U (jump to unread) now exits split zoom (Cmd-Shift-Enter) before focusing the target pane so the destination is visible. It also guards focus failures to avoid ending in a bad state.
TabManager.focusTabFromNotification, and make it returnBool;AppDelegateandTerminalControllernow guard and log focus failures.false.Written for commit 09a98c9. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests