Fix workspace notification ring invalidation blanking active terminal - #2892
Closed
austinywang wants to merge 13 commits into
Closed
austinywang wants to merge 13 commits into
austinywang wants to merge 13 commits into
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 Combine-driven per-tab notification presentation layer (workspace snapshots, per-tab presentation stores, and a cache) and updates UI to consume cached per-tab presentations instead of querying the global notification store per render. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Layer (ContentView / WorkspaceContentView)
participant Cache as WorkspaceNotificationPresentationStoreCache
participant Store as TerminalNotificationStore
participant Pub as Publisher (CombineLatest)
participant Present as WorkspaceNotificationPresentationStore
participant Tab as TabItemView
Store->>Pub: emit notifications / focusedReadIndicator changes
Pub->>Present: publish workspaceSnapshot(forTabId)
Present->>Present: update `@Published` presentation
UI->>Cache: request store(for: selectedTabId)
Cache->>Present: create/return store instance
Present->>Tab: presentation change triggers TabItemView update
Tab->>Tab: read presentation.visibleSurfaceIds for badges/overlays
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR fixes a bug where a notification arriving in a background workspace would trigger SwiftUI re-renders in unrelated sidebar rows and Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant NS as TerminalNotificationStore
participant Pub as workspaceSnapshotPublisher(tabId)
participant Cache as WorkspaceNotificationPresentationStoreCache
participant Store as WorkspaceNotificationPresentationStore
participant Row as TabItemView (@ObservedObject)
participant WCV as WorkspaceContentView (onReceive)
Note over NS: Background workspace B gets notification
NS->>Pub: $notifications publishes (all tabId subscribers fire)
Pub->>Pub: removeDuplicates() — workspace A snapshot unchanged, filtered out
Pub->>Store: workspace B snapshot changed, passes through
Store->>Store: "guard presentation != newSnapshot (extra dedup)"
Store->>Row: "@Published presentation update → body re-evaluates (workspace B row only)"
Store-->>Row: workspace A row: no publish → .equatable() guard blocks re-eval
Note over NS: Sidebar appears / tabs change
Cache->>Cache: removeStaleStores(keepingTabIds:)
Cache->>Store: store(for: tab.id) — returns cached or creates new store
Store->>NS: subscribes via presentationPublisher(forTabId:)
Note over WCV: WorkspaceContentView uses onReceive directly
NS->>WCV: workspaceSnapshotPublisher(forTabId: workspace.id) + removeDuplicates()
WCV->>WCV: "guard notificationSnapshot != snapshot → @State update only if changed"
Reviews (1): Last reviewed commit: "Scope workspace notification updates" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // action handlers use the plain references without triggering re-evaluation. | ||
| let tabManager: TabManager | ||
| let notificationStore: TerminalNotificationStore | ||
| @ObservedObject var notificationPresentationStore: WorkspaceNotificationPresentationStore |
Contributor
There was a problem hiding this comment.
@ObservedObject added without updating ==
The CLAUDE.md pitfall for TabItemView explicitly says: "Do not add @EnvironmentObject, @ObservedObject (besides tab), or @Binding properties without updating the == function." The notificationPresentationStore is excluded from ==, which is correct in intent (the cache makes the reference stable and the @ObservedObject subscription is the desired update path), but still violates the documented invariant.
If the cache ever produces a new store for a pre-existing tabId (e.g., after a future refactor of removeStaleStores), SwiftUI would skip body updates for the new store — the .equatable() guard returns true because == never compares stores, so the stale subscription stays active.
Adding an identity check would satisfy the policy without changing current behavior (the reference is always stable today):
Suggested change
| @ObservedObject var notificationPresentationStore: WorkspaceNotificationPresentationStore | |
| @ObservedObject var notificationPresentationStore: WorkspaceNotificationPresentationStore |
// In ==:
lhs.notificationPresentationStore === rhs.notificationPresentationStore &&![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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
🧹 Nitpick comments (2)
Sources/WorkspaceContentView.swift (1)
241-355: Project only badge-visible notification state into@State.
WorkspaceContentViewonly readsvisibleSurfaceIds, butnotificationSnapshotalso carriesunreadCount,hasRead,latestNotification, andfocusedReadIndicatorSurfaceId. Changes in those unused fields will still mutate local state and re-evaluate the whole workspace tree, which leaves an avoidable invalidation path in the terminal host.♻️ Suggested narrowing of the stored state
- `@State` private var notificationSnapshot = TerminalNotificationWorkspaceSnapshot.empty + `@State` private var visibleNotificationSurfaceIds: Set<UUID> = [] @@ let showsNotificationRing = Workspace.shouldShowUnreadIndicator( - hasUnreadNotification: notificationSnapshot.visibleSurfaceIds.contains(panel.id), + hasUnreadNotification: visibleNotificationSurfaceIds.contains(panel.id), isManuallyUnread: workspace.manualUnreadPanelIds.contains(panel.id) ) @@ .onAppear { - refreshNotificationSnapshot() + refreshVisibleNotificationSurfaceIds() syncBonsplitNotificationBadges() refreshGhosttyAppearanceConfig(reason: "onAppear") } .onReceive( notificationStore.workspaceSnapshotPublisher(forTabId: workspace.id) + .map(\.visibleSurfaceIds) + .removeDuplicates() .receive(on: RunLoop.main) - ) { snapshot in - guard notificationSnapshot != snapshot else { return } - notificationSnapshot = snapshot + ) { surfaceIds in + visibleNotificationSurfaceIds = surfaceIds } - .onChange(of: notificationSnapshot) { _, _ in + .onChange(of: visibleNotificationSurfaceIds) { _, _ in syncBonsplitNotificationBadges() } @@ let shouldShow = panelId.map { - notificationSnapshot.visibleSurfaceIds.contains($0) || manualUnread.contains($0) + visibleNotificationSurfaceIds.contains($0) || manualUnread.contains($0) } ?? false @@ - private func refreshNotificationSnapshot() { - notificationSnapshot = notificationStore.workspaceSnapshot(forTabId: workspace.id) + private func refreshVisibleNotificationSurfaceIds() { + visibleNotificationSurfaceIds = notificationStore.workspaceSnapshot(forTabId: workspace.id).visibleSurfaceIds }Also applies to: 405-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/WorkspaceContentView.swift` around lines 241 - 355, The view currently stores the entire TerminalNotificationWorkspaceSnapshot in `@State` (notificationSnapshot) which causes unnecessary invalidations when fields other than visibleSurfaceIds change; change the `@State` to store only the minimal data needed (e.g. a Set of visibleSurfaceIds or a tiny struct wrapping that set), update the .onReceive publisher handler to project snapshot.visibleSurfaceIds into that state, and update all local references (uses in Workspace.shouldShowUnreadIndicator, syncBonsplitNotificationBadges, refreshNotificationSnapshot, and any comparisons) to read from the new notification-visible-state variable instead of notificationSnapshot so only visibleSurfaceIds drives re-renders (also apply the same narrowing for the other occurrence mentioned in the comment).Sources/ContentView.swift (1)
10203-10205: Finish the row cutover to presentation-derived values.
TabItemViewnow has a per-tabWorkspaceNotificationPresentationStore, but it also takes the globalnotificationStore. That keeps some notification-derived row state outside the new row-local presentation path; please precompute those menu flags above the row or expose them through the presentation layer instead.As per coding guidelines, "Do not read
tabManagerornotificationStorein the body; use precomputed let parameters instead."Also applies to: 12643-12645
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 10203 - 10205, TabItemView still reads global notification state (notificationStore / tabManager) inside the row; instead precompute any notification-derived booleans/flags (e.g. contextual menu visibility, badge/showing indicators) above the row and pass them into TabItemView as let parameters, or add accessors on WorkspaceNotificationPresentationStore/notificationPresentationStore to expose the required presentation values and use those, and remove any direct reads of TerminalNotificationStore.shared or tabManager from the TabItemView body (also apply same change for the other occurrence around the block that previously used TerminalNotificationStore.shared at lines noted in the review).
🤖 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/ContentView.swift`:
- Around line 2968-2974: The overlay can briefly see the previous tab's snapshot
because workspaceSnapshotPublisher(forTabId:) takes a moment to emit after
selectedTabId changes; update refreshSelectedWorkspaceNotificationSnapshot to
synchronously seed selectedWorkspaceNotificationSnapshot with
notificationStore.workspaceSnapshot(forTabId: selectedTabId) whenever
tabManager.selectedTabId changes (in the same place you currently set it) so the
new tab's current snapshot is available immediately, and also apply the same
synchronous seeding pattern where you create the Combine stream (e.g., in
workspaceSnapshotPublisher(forTabId:) usage around lines 3202-3215) to prepend
or set the current notificationStore.workspaceSnapshot(forTabId:) before relying
on publisher emissions.
In `@Sources/TerminalNotificationStore.swift`:
- Around line 674-698: The snapshot currently cannot represent workspace-scoped
indicators because visibleSurfaceIds is a Set<UUID> (can't hold nil) and
hasVisibleNotificationIndicator(surfaceId:) returns false for nil; add a
dedicated Bool property (e.g., workspaceVisibleIndicator) to
TerminalNotificationWorkspaceSnapshot, include it in the static empty
initializer, set or clear it wherever visible surface IDs are recorded (the code
paths around the previous lines ~987-997 that only record non-nil IDs), and
change hasVisibleNotificationIndicator(surfaceId:) to return
workspaceVisibleIndicator when surfaceId is nil, otherwise check
visibleSurfaceIds.contains(surfaceId).
- Around line 982-1007: The closure subscribed to workspaceSnapshotPublisher
currently ignores the emitted $notifications and $focusedReadIndicatorByTabId
and calls workspaceSnapshot(forTabId:) which reads stale self.notifications;
change workspaceSnapshot(forTabId:) to accept the freshly emitted notifications
and focusedReadIndicatorByTabId (the types matching the `@Published` outputs) and
update its implementation to compute hasRead, visibleSurfaceIds,
latestNotification, and unreadCount from those parameters instead of
self.notifications/self.focusedReadIndicatorByTabId; then update the
workspaceSnapshotPublisher closure to pass the emitted values into
workspaceSnapshot(...) so snapshots are built from the new, not-yet-assigned
published values.
---
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 10203-10205: TabItemView still reads global notification state
(notificationStore / tabManager) inside the row; instead precompute any
notification-derived booleans/flags (e.g. contextual menu visibility,
badge/showing indicators) above the row and pass them into TabItemView as let
parameters, or add accessors on
WorkspaceNotificationPresentationStore/notificationPresentationStore to expose
the required presentation values and use those, and remove any direct reads of
TerminalNotificationStore.shared or tabManager from the TabItemView body (also
apply same change for the other occurrence around the block that previously used
TerminalNotificationStore.shared at lines noted in the review).
In `@Sources/WorkspaceContentView.swift`:
- Around line 241-355: The view currently stores the entire
TerminalNotificationWorkspaceSnapshot in `@State` (notificationSnapshot) which
causes unnecessary invalidations when fields other than visibleSurfaceIds
change; change the `@State` to store only the minimal data needed (e.g. a Set of
visibleSurfaceIds or a tiny struct wrapping that set), update the .onReceive
publisher handler to project snapshot.visibleSurfaceIds into that state, and
update all local references (uses in Workspace.shouldShowUnreadIndicator,
syncBonsplitNotificationBadges, refreshNotificationSnapshot, and any
comparisons) to read from the new notification-visible-state variable instead of
notificationSnapshot so only visibleSurfaceIds drives re-renders (also apply the
same narrowing for the other occurrence mentioned in the comment).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03b30737-494f-4a52-b2c8-f86987c6bbfd
📒 Files selected for processing (4)
Sources/ContentView.swiftSources/TerminalNotificationStore.swiftSources/WorkspaceContentView.swiftcmuxTests/NotificationAndMenuBarTests.swift
Comment on lines
+674
to
+698
| let visibleSurfaceIds: Set<UUID> | ||
| let latestNotification: TerminalNotification? | ||
| let focusedReadIndicatorSurfaceId: UUID? | ||
|
|
||
| static let empty = TerminalNotificationWorkspaceSnapshot( | ||
| tabId: nil, | ||
| unreadCount: 0, | ||
| hasRead: false, | ||
| visibleSurfaceIds: [], | ||
| latestNotification: nil, | ||
| focusedReadIndicatorSurfaceId: nil | ||
| ) | ||
|
|
||
| var hasUnreadNotifications: Bool { | ||
| unreadCount > 0 | ||
| } | ||
|
|
||
| var hasReadNotifications: Bool { | ||
| hasRead | ||
| } | ||
|
|
||
| func hasVisibleNotificationIndicator(surfaceId: UUID?) -> Bool { | ||
| guard let surfaceId else { return false } | ||
| return visibleSurfaceIds.contains(surfaceId) | ||
| } |
There was a problem hiding this comment.
Preserve workspace-level indicators in the snapshot.
Line 695 now hard-codes surfaceId == nil to false, and Lines 990-991 only record non-nil surface IDs. The existing store API treats (tabId, nil) as a valid unread key, so this presentation model can no longer represent workspace-scoped notifications at all. Any caller migrated from hasVisibleNotificationIndicator(forTabId:surfaceId:) to this snapshot will silently lose that indicator.
🧩 Proposed fix
struct TerminalNotificationWorkspaceSnapshot: Equatable, Sendable {
let tabId: UUID?
let unreadCount: Int
let hasRead: Bool
+ let hasVisibleWorkspaceIndicator: Bool
let visibleSurfaceIds: Set<UUID>
let latestNotification: TerminalNotification?
let focusedReadIndicatorSurfaceId: UUID?
static let empty = TerminalNotificationWorkspaceSnapshot(
tabId: nil,
unreadCount: 0,
hasRead: false,
+ hasVisibleWorkspaceIndicator: false,
visibleSurfaceIds: [],
latestNotification: nil,
focusedReadIndicatorSurfaceId: nil
)
@@
func hasVisibleNotificationIndicator(surfaceId: UUID?) -> Bool {
- guard let surfaceId else { return false }
+ guard let surfaceId else { return hasVisibleWorkspaceIndicator }
return visibleSurfaceIds.contains(surfaceId)
}
}
@@
func workspaceSnapshot(forTabId tabId: UUID) -> TerminalNotificationWorkspaceSnapshot {
var hasRead = false
+ var hasVisibleWorkspaceIndicator = false
var visibleSurfaceIds = Set<UUID>()
let focusedReadIndicatorSurfaceId = focusedReadIndicatorByTabId[tabId]
for notification in notifications where notification.tabId == tabId {
if notification.isRead {
hasRead = true
} else if let surfaceId = notification.surfaceId {
visibleSurfaceIds.insert(surfaceId)
+ } else {
+ hasVisibleWorkspaceIndicator = true
}
}
@@
return TerminalNotificationWorkspaceSnapshot(
tabId: tabId,
unreadCount: unreadCount(forTabId: tabId),
hasRead: hasRead,
+ hasVisibleWorkspaceIndicator: hasVisibleWorkspaceIndicator,
visibleSurfaceIds: visibleSurfaceIds,
latestNotification: latestNotification(forTabId: tabId),
focusedReadIndicatorSurfaceId: focusedReadIndicatorSurfaceId
)
}Also applies to: 987-997
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalNotificationStore.swift` around lines 674 - 698, The snapshot
currently cannot represent workspace-scoped indicators because visibleSurfaceIds
is a Set<UUID> (can't hold nil) and hasVisibleNotificationIndicator(surfaceId:)
returns false for nil; add a dedicated Bool property (e.g.,
workspaceVisibleIndicator) to TerminalNotificationWorkspaceSnapshot, include it
in the static empty initializer, set or clear it wherever visible surface IDs
are recorded (the code paths around the previous lines ~987-997 that only record
non-nil IDs), and change hasVisibleNotificationIndicator(surfaceId:) to return
workspaceVisibleIndicator when surfaceId is nil, otherwise check
visibleSurfaceIds.contains(surfaceId).
![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 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/TerminalNotificationStore.swift">
<violation number="1" location="Sources/TerminalNotificationStore.swift:1015">
P1: Build the workspace snapshot from the emitted `notifications` and `focusedReadIndicatorByTabId` values instead of reading through `self` in this closure; otherwise the pipeline can observe stale state and drop real notification changes after `removeDuplicates()`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is kicking off a free cloud agent to fix this issue. This run is complimentary, but you can enable autofix for all future PRs in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6204b10. Configure here.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/TerminalNotificationStore.swift (1)
670-705:⚠️ Potential issue | 🟠 MajorPreserve workspace-scoped notifications in the snapshot.
The new snapshot still drops
surfaceId == nilnotifications.workspaceSnapshot(...)counts them inunreadCount, but neitherunreadSurfaceIdsnorvisibleSurfaceIdscan represent them, and both helper methods hard-returnfalsefornil. Any caller migrated from the old per-tab/per-surface helpers will silently lose workspace-level unread/visible indicators.🧩 Suggested fix
struct TerminalNotificationWorkspaceSnapshot: Equatable, Sendable { let tabId: UUID? let unreadCount: Int let hasRead: Bool + let hasUnreadWorkspaceNotification: Bool let unreadSurfaceIds: Set<UUID> let visibleSurfaceIds: Set<UUID> let latestNotification: TerminalNotification? let focusedReadIndicatorSurfaceId: UUID? static let empty = TerminalNotificationWorkspaceSnapshot( tabId: nil, unreadCount: 0, hasRead: false, + hasUnreadWorkspaceNotification: false, unreadSurfaceIds: [], visibleSurfaceIds: [], latestNotification: nil, focusedReadIndicatorSurfaceId: nil ) @@ func hasUnreadNotification(surfaceId: UUID?) -> Bool { - guard let surfaceId else { return false } + guard let surfaceId else { return hasUnreadWorkspaceNotification } return unreadSurfaceIds.contains(surfaceId) } func hasVisibleNotificationIndicator(surfaceId: UUID?) -> Bool { - guard let surfaceId else { return false } + guard let surfaceId else { return hasUnreadWorkspaceNotification } return visibleSurfaceIds.contains(surfaceId) } } @@ ) -> TerminalNotificationWorkspaceSnapshot { var unreadCount = 0 var hasRead = false + var hasUnreadWorkspaceNotification = false var unreadSurfaceIds = Set<UUID>() var visibleSurfaceIds = Set<UUID>() @@ } else if let surfaceId = notification.surfaceId { unreadCount += 1 unreadSurfaceIds.insert(surfaceId) visibleSurfaceIds.insert(surfaceId) } else { unreadCount += 1 + hasUnreadWorkspaceNotification = true } } @@ tabId: tabId, unreadCount: unreadCount, hasRead: hasRead, + hasUnreadWorkspaceNotification: hasUnreadWorkspaceNotification, unreadSurfaceIds: unreadSurfaceIds, visibleSurfaceIds: visibleSurfaceIds, latestNotification: latestNotification, focusedReadIndicatorSurfaceId: focusedReadIndicatorSurfaceId ) }Also applies to: 997-1036
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalNotificationStore.swift` around lines 670 - 705, The snapshot currently cannot represent workspace-scoped (surfaceId == nil) notifications because unreadSurfaceIds/visibleSurfaceIds are UUID sets and hasUnreadNotification/hasVisibleNotificationIndicator return false for nil; add explicit Bool properties (e.g. workspaceHasUnread and workspaceHasVisible) to TerminalNotificationWorkspaceSnapshot, initialize them in static let empty and wherever workspaceSnapshot(...) is built, and update hasUnreadNotification(surfaceId:) and hasVisibleNotificationIndicator(surfaceId:) to return workspaceHasUnread/workspaceHasVisible when surfaceId is nil so callers retain workspace-scoped unread/visible state; ensure Equatable conformance includes the new properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/TerminalNotificationStore.swift`:
- Around line 670-705: The snapshot currently cannot represent workspace-scoped
(surfaceId == nil) notifications because unreadSurfaceIds/visibleSurfaceIds are
UUID sets and hasUnreadNotification/hasVisibleNotificationIndicator return false
for nil; add explicit Bool properties (e.g. workspaceHasUnread and
workspaceHasVisible) to TerminalNotificationWorkspaceSnapshot, initialize them
in static let empty and wherever workspaceSnapshot(...) is built, and update
hasUnreadNotification(surfaceId:) and
hasVisibleNotificationIndicator(surfaceId:) to return
workspaceHasUnread/workspaceHasVisible when surfaceId is nil so callers retain
workspace-scoped unread/visible state; ensure Equatable conformance includes the
new properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e12d931f-dab7-4660-8398-2889d2dd4980
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/TerminalNotificationStore.swiftcmuxTests/NotificationAndMenuBarTests.swift
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/NotificationAndMenuBarTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/ContentView.swift
…lack-notification-ring # Conflicts: # Sources/ContentView.swift
…lack-notification-ring Resolve Sources/ContentView.swift conflicts in TabItemView: keep the per-tab notification presentation store from this branch (effective unread count / notification text) and adopt main's workspaceSnapshot caching (workspaceSnapshotStorage) from the sessions CPU-loop fix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…-2887-terminal-black-notification-ring
lawrencecchen
added
the
stale-revisit
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
3 participants
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.
Summary
Closes #2887
Note
Medium Risk
Touches notification indexing and several UI update paths (workspace content, sidebar rows, command palette) via new snapshot/publisher logic; regressions could cause stale/missing badges or rings but the scope is localized and covered by new tests.
Overview
Fixes cross-workspace notification updates from invalidating unrelated UI by introducing a per-workspace notification snapshot model and publishers.
TerminalNotificationStorenow builds and publishesTerminalNotificationWorkspaceSnapshot(unread/read presence, per-surface visibility including focused read indicator, latest notification), plus per-tabWorkspaceNotificationPresentationStoreinstances cached viaWorkspaceNotificationPresentationStoreCache.ContentView,WorkspaceContentView, and sidebarTabItemVieware refactored to consume these workspace-scoped snapshots/stores (including tmux overlay/unread rect calculations and command palette context flags), with cache pruning on tab changes. Adds regression tests for snapshot correctness, per-workspace isolation, stale-snapshot fallback, and cache pruning, plus a small File Explorer test stabilization around pending expansion completion.Reviewed by Cursor Bugbot for commit fc48431. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes a bug where notifications from other workspaces invalidated the active workspace and blanked its terminal. Scopes notification state per workspace so rings/overlays, sidebar badges, and the command palette stay accurate and reactive. Closes #2887.
TerminalNotificationWorkspaceSnapshot(unread count, read presence, per-surface unread and visible sets incl. focused read indicator, latest notification).WorkspaceNotificationPresentationStorecached viaWorkspaceNotificationPresentationStoreCache; sidebar rows observe their own store for unread count and latest message to avoid cross-row invalidation and flicker.ContentViewcaches the selected workspace snapshot, updates it viaworkspaceSnapshotPublisher(forTabId:), prunes stale presentation stores on tab changes, usesvisibleSurfaceIdsfor unread rects, and drives command palette read/unread from snapshots.WorkspaceContentViewsubscribes toworkspaceSnapshotPublisher(forTabId:)and usesvisibleSurfaceIdsfor unread rings and tmux overlay rects.TerminalNotificationStore.sharedto reduce environment-driven invalidations; added publishers (presentationPublisher/workspaceSnapshotPublisher) for reactive updates.Written for commit fc48431. Summary will update on new commits.
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests