Repository navigation
Fix notification-list layout thrash on launch (#5794) - #6886
Conversation
The notifications page and the titlebar notification popover both render
`ScrollView { LazyVStack { ForEach(notifications) } }` of heavily-modified
rows, but — unlike every other lazy list in the app (sidebar TabItemView,
Task Manager rows) — the row views were not Equatable and the call sites did
not apply `.equatable()`. So every TerminalNotificationStore publish (a new
notification, a read/unread toggle, a clear) re-evaluated the body of every
row and re-laid out the whole lazy stack. With many notifications accumulated
and agents publishing continuously, that is the AttributeGraph relayout thrash
documented for the sidebar/sessions lists (#2586 / #5752) and matches the
"heavily modified nested view lists inside a ForEach" hang reported in #5794.
- Make NotificationRow and NotificationPopoverRow Equatable and apply
`.equatable()`. Equality compares only the rendered value snapshot
(notification, tabTitle, and — for the page row — an explicit `isFocused`
so the default-action shortcut still follows focus), never the closures or
focus binding the parent rebuilds on every render.
- Resolve each row's tab title from a single-pass tabId->title index
(AppDelegate.tabTitlesByTabId) built once per render, instead of an O(tabs)
scan per notification row (was O(notifications x tabs)).
- Add NotificationRowSnapshotBoundaryTests locking the == contract and the
index, wired into the cmuxTests target.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 22 minutes and 13 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ 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 |
Greptile SummaryThis PR applies the established snapshot-boundary pattern (
Confidence Score: 5/5Safe to merge — applies a well-established pattern already used on every other lazy list in the app, and the new equality logic is directly locked by the accompanying tests. The change is narrowly scoped to two SwiftUI surfaces and one new AppDelegate helper. The Equatable conformances are correct: closures and the focus binding are excluded from ==, and isFocused is included so DefaultActionModifier tracks the right row. tabTitlesByTabId() is a pure, single-pass @mainactor function that mirrors the existing tabTitle(for:) resolution order, so no tab titles are dropped. The previous reviewer comments about the caseless-enum namespace and intermediate pairs array have both been addressed. The test suite directly exercises the equality invariant that .equatable() depends on. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Store as TerminalNotificationStore
participant Page as NotificationsPage.body
participant Index as AppDelegate.tabTitlesByTabId()
participant EQ as .equatable() wrapper
participant Row as NotificationRow.body
Store->>Page: publish (one notification changed)
Page->>Index: tabTitlesByTabId() one pass O(windows x tabs)
Index-->>Page: [UUID: String]
loop ForEach over notifications
Page->>EQ: new NotificationRow(snapshot, isFocused)
EQ->>EQ: "lhs == rhs?"
alt snapshot unchanged
EQ-->>Page: skip body re-eval (layout cache preserved)
else snapshot changed
EQ->>Row: evaluate body
Row-->>Page: updated view tree
end
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Store as TerminalNotificationStore
participant Page as NotificationsPage.body
participant Index as AppDelegate.tabTitlesByTabId()
participant EQ as .equatable() wrapper
participant Row as NotificationRow.body
Store->>Page: publish (one notification changed)
Page->>Index: tabTitlesByTabId() one pass O(windows x tabs)
Index-->>Page: [UUID: String]
loop ForEach over notifications
Page->>EQ: new NotificationRow(snapshot, isFocused)
EQ->>EQ: "lhs == rhs?"
alt snapshot unchanged
EQ-->>Page: skip body re-eval (layout cache preserved)
else snapshot changed
EQ->>Row: evaluate body
Row-->>Page: updated view tree
end
end
Reviews (6): Last reviewed commit: "Address review: preserve active-tabManag..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| enum NotificationTabTitleIndex { | ||
| /// Builds the lookup in one pass. The first pair wins on duplicate ids, | ||
| /// matching the "first matching tab" resolution the per-row scans used. | ||
| static func make<S: Sequence>(_ pairs: S) -> [UUID: String] | ||
| where S.Element == (id: UUID, title: String) { | ||
| var index: [UUID: String] = [:] | ||
| for pair in pairs where index[pair.id] == nil { | ||
| index[pair.id] = pair.title | ||
| } | ||
| return index | ||
| } | ||
| } |
There was a problem hiding this comment.
NotificationTabTitleIndex is a caseless enum whose entire API is one static func make(...). The cmux-no-ambient-global-state rule flags this pattern — a caseless enum or empty struct used purely as a static func namespace should instead be expressed as a method on the type that owns the data, or (if it must be file-scoped) as a private/fileprivate free function. Because the only production caller is AppDelegate.tabTitlesByTabId(), the clearest fix is to inline the four-line dictionary loop there and remove the enum. The test coverage for make could then be adapted to test tabTitlesByTabId() via a test double or removed, since the logic is trivial. If the index builder genuinely needs its own identity for testing, prefer a free function or a Dictionary extension over a caseless enum.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Done in 9f700bf — removed the caseless-enum namespace and build the [UUID: String] index directly (one pass) inside AppDelegate.tabTitlesByTabId(). The Equatable snapshot-boundary tests remain; dropped the now-trivial index test.
— Claude Code
| func tabTitlesByTabId() -> [UUID: String] { | ||
| var pairs: [(id: UUID, title: String)] = [] | ||
| for context in mainWindowContexts.values { | ||
| for tab in context.tabManager.tabs { | ||
| pairs.append((id: tab.id, title: tab.title)) | ||
| } | ||
| } | ||
| return NotificationTabTitleIndex.make(pairs) | ||
| } |
There was a problem hiding this comment.
pairs array allocates before the single-pass dictionary build
tabTitlesByTabId() appends every tab into a [(id: UUID, title: String)] array, then immediately iterates that array again inside NotificationTabTitleIndex.make(_:). Because make is generic over Sequence, the intermediate allocation is unnecessary — the nested for loops could write directly into the [UUID: String] dictionary in one pass, saving one full-collection allocation on every notification-list render. This is minor today but tabTitlesByTabId() is called on the main actor in SwiftUI body context, so keeping it allocation-lean is worthwhile.
There was a problem hiding this comment.
Done in 9f700bf — no more intermediate pairs array; tabTitlesByTabId() now writes straight into the [UUID: String] dictionary in a single pass.
— Claude Code
…nresponsive-on-macos-26
…space Greptile P2s: NotificationTabTitleIndex was a caseless-enum static-only namespace (cmux no-ambient-global-state / static-as-namespace policy), and tabTitlesByTabId() allocated an intermediate pairs array before the dict. Build the [UUID: String] index directly in one pass inside tabTitlesByTabId() (first matching tab wins, same as the prior per-row scan) and remove the enum. Drops the now-trivial index unit test; the Equatable snapshot-boundary tests (the real regression guards) stay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cmux policy requires new non-UI tests to use Swift Testing, not XCTest. Convert NotificationRowSnapshotBoundaryTests to import Testing / @suite / @test / #expect (mirrors HiddenRightSidebarContentMountingTests). Behavior and coverage unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cmux Aziz concurrency policy: main-thread hops should use @mainactor, not DispatchQueue.main.async. Convert the relocated onOpen hop to a main-actor Task (same deferred semantics, now expressed with @mainactor). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex P3: tabTitlesByTabId() only scanned mainWindowContexts, but the old AppDelegate.tabTitle(for:) also fell back to the active tabManager. The titlebar popover (no local fallback) would have lost titles for tabs only reachable that way. Fold the active tabManager into the index so resolution matches tabTitle(for:): contexts win, then the active manager. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Fixes #5794
Investigation
#5794 reports a ~22s main-thread hang on launch, sampled inside a SwiftUI layout pass over a nested
ScrollView/LazyStack/ForEachof "heavily modified nested view lists" (AG::Subgraph::update → ScrollViewUtilities.contentFrame → LazyStack.measureEstimates → ForEachState.forEachItem → ModifiedViewList.applyNodes). It is part of the umbrella SwiftUI-layout-hang family tracked in #5752 (the top app hang, ~11k users) alongside #2586 / #5570 / #5764 / #5845.The reporter is on the old stable build
0.64.14 (94)and could not test nightly. The sidebar path of that family (the most likely one for users with many workspaces) was already substantially fixed after that build by #6033 and #6188 — and an empirical probe confirmsLazyVStackcontent sizing is inherently lazy (only viewport-visible rows realize, even at 2000 rows), so the remaining cost in this class is per-row work multiplied across rows under store churn (ModifiedViewList.applyNodesbuilding every item), not whole-list realization.This PR fixes the one launch-restorable lazy list that still lacked the protection every other lazy list in the app already has: the notification lists.
The bug
NotificationsPageand the titlebarNotificationPopoverRowlist both renderScrollView { LazyVStack { ForEach(notifications) } }of heavily-modified rows (nested stacks, buttons, focusable, context menus). Unlike the sidebarTabItemViewand the Task Manager rows, these row views were notEquatableand the call sites did not apply.equatable(). So everyTerminalNotificationStorepublish — a new notification arriving, a read/unread toggle, a clear — re-evaluated the body of every row and re-laid out the wholeLazyVStack. With many notifications accumulated and agents publishing continuously at launch, that is exactly the AttributeGraph relayout thrash this family is about.Additionally, each row resolved its tab title with an O(tabs) scan, making the
ForEachbuild O(notifications × tabs).The fix
NotificationRowandNotificationPopoverRowEquatable+ apply.equatable()(the repo's established snapshot-boundary pattern, seeCLAUDE.mdandTaskManagerViewSnapshotBoundaryTests). Equality compares only the rendered value snapshot —notification,tabTitle, and, for the page row, an explicitisFocusedso the default-action keyboard shortcut still follows focus — never the closures or focus binding the parent rebuilds each render. A store publish that touches one notification now skips body re-evaluation for the unchanged rows.tabId -> titleindex (AppDelegate.tabTitlesByTabId()) built once per render, replacing the per-row scan — O(notifications + tabs) instead of O(notifications × tabs).Tests
cmuxTests/NotificationRowSnapshotBoundaryTests.swift(wired into thecmuxTeststarget) locks the invariant: rows with identical snapshots but freshly-rebuilt closures compare equal (so.equatable()can suppress re-eval), while a changed read-state / tab-title / focus changes equality; plus coverage for the title index. The test is intrinsically coupled to the newEquatableAPI (it cannot compile against the pre-fix code), so it ships in the same commit rather than as a separate red commit.Notes
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes #5794 by stopping layout thrash in notification lists on launch. Rows are now Equatable and tab titles are looked up in O(1), so unchanged rows don’t rebuild on store updates.
NotificationRowandNotificationPopoverRowEquatableand applied.equatable(); addedisFocusedto the page row snapshot so the default action follows focus.tabId -> titleindex viaAppDelegate.tabTitlesByTabId()and used it in both the page and titlebar popover to remove O(rows × tabs) scans; resolution matches the old order (window contexts first, then the activetabManager) so popover titles are preserved.Task { @MainActor in ... }hop instead ofDispatchQueue.main.asyncto ensure main-thread UI work.NotificationRowSnapshotBoundaryTests(using Swift Testing) to lock the snapshot boundary and equality behavior.Written for commit bbf2687. Summary will update on new commits.