Repository navigation
perf: make TabItem a reference so one title write wakes one tab - #222
Conversation
Two of these fail on main. A title write goes through PaneState.tabs, which is an @observable property, so it invalidates every reader of the array rather than the one tab that changed. And because TabItem is a value, the tab handed to a view is a copy, so nothing observes its title at all. Red on this commit: testChangingOneTabTitleDoesNotInvalidateTheTabsArray, testChangingOneTabTitleInvalidatesThatTab. Claude-Session: https://claude.ai/code/session_01Bj791kgph6Xk9CJf9c41Db
PaneState.tabs is an @observable property, so pane.tabs[i].title = x wrote the array and invalidated everything that had read it: every TabItemView, the scroll content, and the split button row. A host whose tab titles animate drove that about 20 times a second. TabItem is now an @observable final class. updateTab takes the reference it already had for its change check and writes through that, so the array is only read. Observation then delivers the title to the one TabItemView that reads it. Insert, remove, and move still write tabs, which is what TabBarView needs to see. Equality and hashing were already by id alone and Codable was already hand-rolled with explicit CodingKeys, so neither changes. No local copy of a TabItem is mutated anywhere in the package, so nothing depended on value semantics. Turns the two red tests from the previous commit green. 227 XCTest and 27 swift-testing tests pass, with no changes to existing tests. Claude-Session: https://claude.ai/code/session_01Bj791kgph6Xk9CJf9c41Db
|
Warning Review limit reachedNext included review available in 52 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
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 |
|
Measured this end to end. Built cmux against a bonsplit with this branch plus #221, ran the same 20-second Instruments capture on the real app. The tab bar's share of SwiftUI graph updates goes from 73-83% to 14%, and each update gets about three times cheaper.
Both patches were in the build, so the two are not separated here. The zero on SF Symbol creation is #221; the collapse in tab bar episodes is this one. The workload is not identical run to run, since it depends on how many agents happen to be spinning. Title events during the captures, as a control:
So the after run carried about 80% of the title traffic. That does not account for a 5.7x drop in tab bar episodes. One thing this does not fix, worth saying out loud: the graph still updates about 19 times a second. The rate did not move, only the cost and the blast radius. After the change 35% of episodes are |
|
@austinywang @teamleaderleo Current Bonsplit main still defines — Inkstone pending |
|
Subagent review at 89b4c44: merge. Checked every place tabs get copied or compared and nothing relies on value semantics. One follow-up we'll do: skip same-value writes in updateTab so older toolchains don't wake the whole bar. Thank you :) |
# Conflicts: # CHANGELOG.md
A tab title change currently re-renders the whole tab bar. This makes it re-render the one tab that changed.
PaneState.tabsis an@Observableproperty andTabItemis a value, so this line inupdateTab:writes the array rather than the element. Observation invalidates every reader of
tabs, which is all ofTabBarView.body:visibleTabEntries, everyTabItemView,tabScrollContent, andsplitButtonRow.Nothing observes the individual tab either.
TabItemViewgetslet tab: TabItemby value, so the title it renders is a copy and no view is subscribed to it. Redraws happen only because the parent rebuilds.What it costs
The host is cmux, where agent tools write a spinner frame into the terminal title. Each frame is a title change. Instruments Time Profiler, 20 seconds, 8 animating tabs:
The sidebar in the same profile is 0.6%, so the tab bar is where the time actually goes.
The change
TabItembecomes an@Observable final class, andupdateTabwrites through the reference it already had:pane.tabsis now only read there, so readers of the array are not invalidated. Observation delivers the title to theTabItemViewthat reads it, which is the view that has to redraw anyway.tabsstill changes on insert, remove, and move. Those change layout, andTabBarViewdoes need to see them.Why the diff is small
TabItemalready declared==andhash(into:)byidalone, so identity comparisons behave exactly as before.Codablewas already hand-rolled with explicitCodingKeys,init(from:), andencode(to:). Onlyrequiredwas added.updateTabis the only place in the package that writes a tab field, and no local copy of aTabItemis mutated anywhere. Nothing depended on value semantics.PaneStateandBonsplitControllerare already@Observable, so this bringsTabItemin line with the models around it.The whole diff is 35 added lines and 15 removed, most of it the doc comment.
Tests
Two commits: the first adds the tests and is red, the second makes them green. Check out
24bdd8fto see them fail.Red on the first commit:
testChangingOneTabTitleDoesNotInvalidateTheTabsArray- awithObservationTrackingblock readingpane.tabs, standing in forTabBarView.body, fires on a title change.testChangingOneTabTitleInvalidatesThatTab- a block readingtab.title, standing in forTabItemView.body, does not fire, and the tab it was handed still reads the old title.Green on both commits, so they guard the parts that were already right:
Full suite: 227 XCTest and 27 swift-testing tests, no failures, no changes to existing tests.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Tab title changes now re-render only the affected tab.
PaneState.tabsis an@Observableproperty, so writingpane.tabs[i].titleused to write the whole array and invalidate every view that had read it—all ofTabBarView, not just the changed tab. On a host whose agent tools animate the terminal title, that meant roughly 20 full tab bar redraws per second.TabItemis now an@Observable final class, andupdateTabwrites through the reference it already holds instead of thetabsarray; observation delivers the change to the oneTabItemViewthat reads it.tabs, whichTabBarViewneeds for layout.id, andCodableremains hand-rolled (onlyrequiredadded); no public API changes.Written for commit fc64a08. Summary will update on new commits.