Repository navigation
perf: incremental notification badge sync (#4636) - #4643
austinywang wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 2 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds targeted badge invalidations in TerminalNotificationStore, a per-panel Bonsplit presentation reconciler in Workspace, and incremental badge-sync wiring in WorkspaceContentView that subscribes to per-workspace invalidations and updates only affected panels. ChangesNotification badge invalidation and incremental sync
Sequence Diagram(s)sequenceDiagram
participant WorkspaceContentView
participant TerminalNotificationStore
participant Workspace
participant BonsplitController
TerminalNotificationStore->>TerminalNotificationStore: detect notifications/manualUnread/focus diffs
TerminalNotificationStore->>WorkspaceContentView: emit badgeInvalidations(forTabId:) (.panelIds/.workspaceManualUnread)
WorkspaceContentView->>Workspace: syncBonsplitTabPresentationState(forPanelIds:)
Workspace->>BonsplitController: updateTab(tabId, showsNotificationBadge/isPinned/kind)
WorkspaceContentView->>WorkspaceContentView: bump unreadBadgeRevision / adjust local manual-unread tracking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 c8a6276. Configure here.
Greptile SummaryNarrows notification badge sync from an O(workspaces × panes × tabs) sweep per notification to an O(changed-panels) targeted update for the affected workspace.
Confidence Score: 5/5Safe to merge — the algorithmic change is well-scoped, the full-repair path on onAppear is preserved, and the new tests cover both publisher scoping and targeted-sync isolation. The three emission sites in TerminalNotificationStore each correctly compute symmetric differences before sending, the WorkspaceContentView caches and updates workspace-manual-unread state consistently across all change paths, and unreadBadgeRevision is read in body to ensure ring indicators refresh after every targeted sync. No missed invalidation path or stale-state condition was found. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "merge: resolve conflicts with main" | 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: 3
🤖 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/TerminalNotificationStore.swift`:
- Around line 2209-2221: Loop currently emits a .panelIds invalidation for every
tabId even when the focused panel didn’t change; modify the loop in
TerminalNotificationStore so you skip a tab when oldValue[tabId] ==
newValue[tabId] (or both nil) before building changedPanelIds and sending
badgeInvalidationSubject. Specifically, inside the for tabId in tabIds loop,
check equality of oldValue[tabId] and newValue[tabId] and continue if unchanged,
then proceed to collect changedPanelIds and send
TerminalNotificationBadgeInvalidation(scope: .panelIds(changedPanelIds)).
In `@Sources/Workspace.swift`:
- Around line 10176-10181: The reconciliation currently treats a nil
expectedKind as "no-op" because kindUpdate is created with expectedKind.map {
.some($0) } and the guard only checks kind mismatch when expectedKind != nil, so
stale tab.kind never gets cleared; change kindUpdate to explicitly wrap the
optional (e.g., let kindUpdate: String?? = .some(expectedKind)) so a nil
expectedKind becomes an explicit clear, and update the guard condition(s) to
compare tab.kind != expectedKind (remove the expectedKind != nil gating) so
differences including nil vs non-nil are detected and applied; apply the same
change in the other similar block around the code referenced at 10185-10189.
In `@Sources/WorkspaceContentView.swift`:
- Line 176: The State property unreadBadgeRevision is dead code because it's
only written to by bumpUnreadBadgeRevision() and never read in body; either
reference unreadBadgeRevision inside WorkspaceContentView.body (for example by
adding let _ = unreadBadgeRevision or using .id(unreadBadgeRevision) on the
view) to force SwiftUI refresh when bumpUnreadBadgeRevision() is called, or
remove the unreadBadgeRevision declaration and all uses of
bumpUnreadBadgeRevision() entirely; update symbols: unreadBadgeRevision and
bumpUnreadBadgeRevision() accordingly.
🪄 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: 375eec17-5ad6-45eb-a600-9b2c4fc01544
📒 Files selected for processing (4)
Sources/TerminalNotificationStore.swiftSources/Workspace.swiftSources/WorkspaceContentView.swiftcmuxTests/WorkspaceManualUnreadTests.swift
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
Fixes #4636.
This takes the combination approach from the issue:
@Published, workspace-scopedTerminalNotificationStore.badgeInvalidations(forTabId:)publisher, so unrelated workspace notifications no longer wake every mountedWorkspaceContentViewbadge sync pathWorkspaceand updates by dirty panel IDs with directpanelId -> tabId -> updateTablookup instead of walkingallPaneIds × tabs(inPane:)Verification
Local tests/builds were not run per repo/task instructions; CI is the verification path for this PR.
I did not capture before/after
samplenumbers because I did not synthesize the high-fanout workload locally. The algorithmic change is the verification argument:notificationStore.notificationschange was observed by every mounted workspace and ranO(workspaces × panes × tabs)tab evaluation, e.g. 20 workspaces × 5 panes × 4 tabs = about 400 tab evaluations per notification before anyupdateTabdiff guardtabId, and only the changed panel IDs are synced, so the hot path isO(changed-panels)for the affected workspace; unrelated mounted workspaces receive no badge invalidationBehaviors Covered
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches unread/badge presentation across notifications, workspace manual unread, and Bonsplit tab state; behavior should match prior logic but regressions could show wrong badges on edge cases (representative panel, focused-read indicator).
Overview
Replaces global
notificationStore.notificationsobservation in mounted workspaces with a workspace-scopedbadgeInvalidations(forTabId:)Combine publisher that emits panel- or workspace-manual-unread scoped invalidations when notifications, focused-read indicators, or workspace manual unread change.Workspacenow owns targeted Bonsplit tab presentation sync (showsNotificationBadge, kind, pin) viasyncBonsplitTabPresentationState(forPanelIds:)using directpanelId → tabIdupdates instead of scanning all panes and tabs; full sync remains for on-appear repair.WorkspaceContentViewsubscribes to invalidations and symmetric-difference panel sets for manual/restored unread, with explicit handling when the workspace manual-unread representative panel moves. Tests cover publisher scoping and that targeted sync does not rewrite unrelated tabs.Reviewed by Cursor Bugbot for commit 084c049. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Makes notification badge sync incremental and tab-scoped so only affected panels update, fixing #4636. Bonsplit tab presentation state (badge/kind/pin) now updates per panel to cut work and improve responsiveness.
badgeInvalidations(forTabId:)that emitsTerminalNotificationBadgeInvalidationscoped to.panelIdsor.workspaceManualUnread, including focused‑read changes.Workspacenow providessyncBonsplitTabPresentationState(forPanelIds:)andsyncBonsplitTabPresentationStateForAllPanels()to update exact panels; full sync remains for on‑appear/repair.WorkspaceContentViewlistens via.onReceiveand applies panel‑scoped invalidations; it uses symmetric‑diffs for manual/restored unread and updates only representative panels on workspace manual‑unread flips.Written for commit 084c049. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Improvements
Tests