Repository navigation
Flash manual unread workspace jumps - #4436
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughJump-to-latest-unread accepts an excluding workspace ID, may emit a manual-unread dismiss flash before clearing unread state, and avoids re-opening the same workspace; the tmux overlay now tracks last flash tokens per workspace. Tests validate jump exclusion, manual/restored unread flashes, overlay flash timing, and shortcut matching edge cases. ChangesJump-to-unread indicator flash + overlay token tracking
Shortcut matching refinements
Sequence DiagramsequenceDiagram
participant AppDelegate
participant NotificationStore
participant Workspace
AppDelegate->>NotificationStore: select unread candidates (filter excludingNotificationId, excludingWorkspaceId)
AppDelegate->>Workspace: determine shouldTriggerManualUnreadJumpFlash(panelId)
AppDelegate->>NotificationStore: check manual/restored unread for tabId
AppDelegate->>Workspace: triggerUnreadIndicatorDismissFlash(panelId)
AppDelegate->>Workspace: clearUnreadAfterJump(panelId)
AppDelegate->>Workspace: openLatestWorkspaceUnread(selectedPanelId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (14 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes missing unread-dismiss flashes on workspace jumps and tightens panel-level unread toggling, navigation exclusion, and shortcut matching.
Confidence Score: 5/5Safe to merge — all three bug fixes (nil-surfaceId false-positive, phantom workspace unread on panel mark, missing unread-dismiss flash) are straightforward and each is pinned by a targeted regression test. The production changes are incremental corrections to well-understood state machines, not rewrites. The flash-token-per-workspace model is the most structurally new piece, but its four unit tests cover the exact deferred-rect and switch-back scenarios. The shortcut guard is narrowly scoped and covered by four cases. No confirmed behavioural regressions were found across the diff. AppDelegate.swift carries the most complexity — the multi-condition toggleFocusedNotificationUnread panel-target routing is dense and the combined case (workspace-level notification coexisting with panel manual unread) has no dedicated test, though each individual path is tested. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant AD as AppDelegate
participant NS as NotificationStore
participant WS as Workspace
participant OV as OverlayModel
U->>AD: markFocusedNotificationAsOldestUnreadAndJumpToNextLatestUnread
AD->>AD: markFocusedNotificationAsOldestUnread
alt panel target exists
AD->>WS: markPanelUnread(panelId)
AD-->>AD: .markedWorkspaceWithoutNotification(tabId)
else no panel target
AD->>NS: markUnread(forTabId:)
AD-->>AD: .markedWorkspaceWithoutNotification(tabId)
end
AD->>AD: jumpToLatestUnread(excludingWorkspaceId: tabId)
AD->>NS: notifications filtered by excludingWorkspaceId
AD->>AD: openTerminalNotification / openLatestWorkspaceUnread
AD->>AD: clearWorkspaceUnreadAfterJump(workspace, panelId)
alt shouldTriggerManualUnreadJumpFlash
AD->>WS: triggerUnreadIndicatorDismissFlash(panelId)
WS-->>NS: flash token incremented
end
AD->>WS: clearUnreadAfterJump(panelId)
Note over OV: apply() sees new flashToken per workspaceId
OV->>OV: "didChangeFlashToken and flashRect present: flashStartedAt = now()"
Reviews (20): Last reviewed commit: "fix: scope unread duplicate checks to fo..." | Re-trigger Greptile |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
10909-10932:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
excludingWorkspaceIdduring notification candidate selection too
jumpToLatestUnreadexcludesexcludedWorkspaceIdonly in the workspace-level fallback (openLatestWorkspaceUnread). The notification-first path still allows unread notifications from that same workspace, so.markedWorkspaceWithoutNotification(tabId)can still jump back into the excluded workspace instead of advancing to the next one.🤖 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 `@Sources/AppDelegate.swift` around lines 10909 - 10932, jumpToLatestUnread currently only applies excludedWorkspaceId in the workspace fallback, so the notification-first loop can return a notification from the excluded workspace; update the notification selection to skip notifications belonging to the excluded workspace. In the for-loop inside jumpToLatestUnread (iterating notificationStore.notifications and using Self.shouldOpenFromJumpToLatestUnread and openTerminalNotification), add a check that notification.workspaceId != excludedWorkspaceId (or pass excludedWorkspaceId into the predicate if you prefer) so notifications from the excluded workspace are not considered before falling back to openLatestWorkspaceUnread.
🤖 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/WorkspaceContentView.swift`:
- Around line 122-133: The code records state.flashToken into
lastFlashTokenByWorkspaceId[state.workspaceId] even when state.flashRect is nil,
which consumes the token before a rect exists and prevents a later non-nil rect
with the same token from animating; change the logic in the block that updates
lastFlashTokenByWorkspaceId (around currentWorkspaceId,
lastFlashTokenByWorkspaceId, state.flashToken, state.flashRect, and
flashStartedAt) so you only set lastFlashTokenByWorkspaceId[state.workspaceId] =
state.flashToken after confirming state.flashRect is non-nil (i.e., preserve the
previous token when flashRect is nil), and keep the existing flashStartedAt and
didChangeWorkspace checks intact.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 10909-10932: jumpToLatestUnread currently only applies
excludedWorkspaceId in the workspace fallback, so the notification-first loop
can return a notification from the excluded workspace; update the notification
selection to skip notifications belonging to the excluded workspace. In the
for-loop inside jumpToLatestUnread (iterating notificationStore.notifications
and using Self.shouldOpenFromJumpToLatestUnread and openTerminalNotification),
add a check that notification.workspaceId != excludedWorkspaceId (or pass
excludedWorkspaceId into the predicate if you prefer) so notifications from the
excluded workspace are not considered before falling back to
openLatestWorkspaceUnread.
🪄 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: a7646cbc-d059-4e42-ae08-fc9691f36bb2
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/WorkspaceContentView.swiftcmuxTests/WindowAndDragTests.swiftcmuxTests/WorkspaceManualUnreadTests.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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/KeyboardShortcutSettings.swift`:
- Around line 1516-1518: The nil-coalescing default for
eventCharsArePrintableASCII is misleading; update the expression or clarify
intent: either change the fallback from "?? true" to "?? false" to be
conservative, or add a short comment near the declaration of
eventCharsArePrintableASCII (and referencing hasEventChars) stating the fallback
is never reached because hasEventChars guards its usage. Locate the
eventCharsArePrintableASCII variable and the hasEventChars usage to apply the
change or add the explanatory comment.
- Around line 1523-1526: The predicate
commandPrintableCharacterShouldBlockFallback needs to skip blocking when Control
is held; update its condition to include a Control-key check (e.g., require
!flags.contains(.control)) so that when both Command and Control are pressed the
logic does not short-circuit and allows the ANSI keyCode fallback path (which
already checks flags.contains(.control)). Modify the boolean expression around
commandPrintableCharacterShouldBlockFallback (which uses
flags.contains(.command), hasEventChars, eventCharsArePrintableASCII,
shortcutKeyIsLetter or eventCharacterIsLetterOrNumber) to also ensure Control is
not present.
🪄 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: 5587bc46-0bbe-4ae6-937c-f116a25552ba
📒 Files selected for processing (2)
Sources/KeyboardShortcutSettings.swiftcmuxTests/WorkspaceUnitTests.swift
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
False positive after verification. A blanket !control carve-out would fail testCommandControlPunctuationDoesNotStealPrintableLetterShortcut and reintroduce the reported printable-u fallback steal. The current matcher still permits Command-Control fallback for control-character events, covered by testCommandControlLetterCanUseLayoutFallbackForControlCharacter. Replied on the review thread.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e655991. Configure here.
This branch was successfully deployed
1 participant
Summary
Verification
Notes
Need help on this PR? Tag
@codesmithwith what you need.Note
Medium Risk
Changes span AppDelegate navigation, notification store state, workspace unread badges, overlay animation, and global shortcut matching—user-visible behavior with broad test coverage but non-trivial interaction edge cases.
Overview
This PR tightens unread navigation and visual feedback across workspaces and panels, and fixes keyboard shortcut matching for overlapping ⌘ bindings.
Unread jumps and flashes:
jumpToLatestUnreadcan exclude a workspace (and its notifications) so “mark oldest and jump next” does not immediately reopen the tab you just marked. When opening unread via jump, the app can fire an unread-dismiss flash before clearing manual/restored unread state. The tmux pane overlay tracks flash tokens per workspace, animates on first-seen tokens, replays after switching back, and waits for aflashRectbefore consuming a token.Panel vs workspace unread: Toggle/mark flows operate on the focused panel when applicable (notifications, focused-read indicators, restored unread, workspace-only restored state). The notification store clears focused-read indicators on mark-read, treats visible indicators with optional
surfaceIdcorrectly, and avoids creating workspace manual unread for missing panel notifications. Workspace/panel restored-unread ownership is refined when clearing panel read state.Shortcuts:
StoredShortcutmatching prefers printable event characters for ⌘ shortcuts over physical key fallbacks, while preserving sensible ⌘⌃ letter fallbacks and preventing ⌘⌃ punctuation from stealing letter shortcuts.Extensive unit tests cover overlay flash timing, unread jump exclusions, panel toggles, and shortcut collisions.
Reviewed by Cursor Bugbot for commit 40b4861. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes missing unread-dismiss flashes and makes unread navigation more predictable. Flashes now animate on first-seen tokens and after workspace switches, and unread toggles act on the focused panel.
unreadIndicatorDismissbefore clearing manual/restored unread; add a store-backed fallback and prevent duplicate restored-unread flashes.flashRectbefore consuming a token.⌘shortcuts over physical fallbacks; keep⌘⌃fallbacks for letters; prevent⌘⌃punctuation from stealing printable letter bindings.Written for commit 40b4861. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Tests