Improve sidebar performance and refresh reliability#323
Merged
Conversation
- Apply the first round of sidebar / tab-bar perf patches addressing re-renders during coding-agent tool storms. - Extract `AgentPresenceManager` into its own `AgentPresenceFeature` TCA reducer. - Replace the `worktreeInfoByID` dictionary with an `IdentifiedArrayOf<WorktreeInfoEntry>` to prepare for per-row scoping.
- Add a per-row `SidebarItemFeature` reducer with reconcile path. - Invoke the reconcile step from `applyRepositories` whenever the roster changes. - Mirror every per-row aggregate (lifecycle, diff stats, PR, running scripts, terminal projection) into the new `sidebarItems` collection after each write. - Migrate per-row reads to `state.sidebarItems[id:]` so views can source from the row state directly.
Swift Observation tracks `@ObservableState` at the property level, so any mutation to an `IdentifiedArrayOf` element invalidated all N row observers and made the sidebar lag during coding-agent tool storms. Scoping each row through `store.scope(state: \.sidebarItems[id:], action: \.sidebarItems[id:])` gives every row its own observation registrar, so a per-row write only re-renders the affected row.
- Delete `SidebarItemModel` and `WorktreeInfoEntry`; per-row data now lives entirely on `SidebarItemFeature.State`. - Mutate row state directly and remove the parallel aggregate sets (`runningScriptsByWorktreeID`, `archivingWorktreeIDs`, `deleteScriptWorktreeIDs`, `deletingWorktreeIDs`, `pendingSetupScriptWorktreeIDs`). - Migrate every test off the aggregate state to drive `sidebarItems`.
- Add cross-feature delegate routes so `AgentPresenceFeature` and `WorktreeTerminalManager` projection events fan out into per-row `agentSnapshotChanged` / `terminalProjectionChanged` actions. - Route diff, pull-request, lifecycle, and `runningScripts` mutations through per-row actions instead of mutating from parent reducers. - Move terminal-focus token and drag-highlight flags from aggregate sets onto per-row state. - Delete the back-channel closures from `WorktreeTerminalManager` (`sendPresenceAction`, `hasAgentActivity`, `agentsForSurfaces`). - Cover the archived-row carry-forward case in `XCTAssertSidebarConsistent` and remove the related false-positive.
- Move free-floating sidebar helpers onto `RepositoriesFeature.State` and drop the sync-bridge. - Wire `pullRequestQueryStarted` from production so the watermark is set at dispatch time rather than in tests. - Trim caller-enumeration paragraphs from the migrated helpers so the surface stays explanatory without rotting on consumer renames.
- Collapse parallel lifecycle booleans into a single `Lifecycle` enum and group all lifecycle predicates next to the enum. - Make `surfaceToItemID` a computed property over `sidebarItems` so the reverse index can no longer drift out of sync. - Delete the dead `rosterChanged` / `RosterDelta` action and rewrite the stale-PR guard test to drive initial state directly. - Call `syncSidebar` after pin / unpin / pinned-move / unpinned-move so the cached `sidebarGrouping` projection matches `state.$sidebar` immediately. - Centralize the orphan-row drop in `reconcileSidebarItems` and move archived-row `runningScripts` cleanup into the same path. - Extend `XCTAssertSidebarConsistent` to assert per-bucket order, not just set membership. - Unify `sidebarDisplayName` on `SidebarItemFeature.State`, extract a `resetRowLifecycleSyncBeforeReconcile` helper, and tidy up the remaining em dashes. - Hoist the computed `surfaceToItemID` once before the agent presence fan-out loop.
Move the periodic `kill(2)` liveness check off the main actor through a new `livenessSweepResult` action that carries both the snapshot and the alive delta. The apply step subtracts only the pids the sweep proved dead from the current record, so any `.sessionStart` or `.sessionEnd` that lands during the off-main hop stays authoritative. Renames the helpers to match their shapes (`liveness(forSnapshot:)` returns a delta; `applyLiveness(delta:snapshot:into:)` merges it back) and adds regression tests for both the mid-hop sessionStart and sessionEnd races.
Smaller chunks (5 branches × 5 PRs) keep the `statusCheckRollup` payload under the GraphQL gateway's 504 threshold on busy CI repos. When a chunk still trips the timeout, retry once after a 1s backoff driven by the injected continuous clock so cancellation propagates and tests can drive fake time. Classify the timeout at the `runGh` catch site by pattern-matching the `ShellClientError` stderr (`HTTP[/0-9.]* 504`) rather than substring matching the full command + stdout + stderr string. Adds a typed `GithubCLIError.gatewayTimeout` case for retry-eligible callers, plus a test that locks the "retry once, then propagate" contract.
When a worktree was included in the batch request snapshot but the response omits it (branch deleted upstream, PR closed and pruned), the row never received `pullRequestChanged` and its `pullRequestBranchAtQueryTime` watermark stayed pinned. The row's equality guard then suppressed every subsequent refresh for that branch. Union the queried IDs with the response keys so every queried row clears its watermark exactly once and stays eligible for the next periodic refresh.
The dict was private with no external observers; the only read was the write itself in `updateRunningState`. Tab dirty state is computed on-demand by `isTabBusy` from the surface tree, so the dict was dead storage and the equality-guard write it gained protected nothing.
Pin the trailing accessory HStack to its natural width so the +N/-N diff counter, the PR number badge, and the running-agent avatars stop collapsing to ellipses when the sidebar is narrow. The title takes the squeeze instead with the existing tail-truncation behaviour.
…eature # Conflicts: # supacode/Commands/WorktreeCommands.swift # supacode/Features/Repositories/Views/SidebarItemsView.swift
`SidebarListView.body` was synthesizing `[SidebarItemFeature.State]` via `orderedSidebarItems(includingRepositoryIDs:)`, which reads `sidebarItems[id:]` for every row and observation-tracks every property of every row's state. Any per-row mutation then invalidated the parent list body, defeating the per-element `store.scope` we set up for row isolation. Add an ID-only flavor that walks `sidebarGrouping` (stored, roster-only) and pass `hotkeyIDs: [Worktree.ID]` down through `SidebarListView`, `SidebarSectionView`, `SidebarRootView`, `SidebarItemsView`, and `SidebarFolderRow`. The shortcut-index dictionary and the folder shortcut-hint lookup now work off plain IDs. Also fix a latent Cmd+N misroute: `rebuildSidebarGrouping` was appending pending worktree IDs to `bucket[.unpinned]`, but `sidebarItemGroups` renders pending rows before non-pending unpinned. The bucket now prepends pending, so the hotkey order matches the visual order while a worktree is being created. Harden the shortcut-index dictionary against duplicate IDs: a forged bucket roster used to trap inside the SwiftUI render loop. It now keeps the first slot and `assertionFailure`s in DEBUG. Document the bucket/items consistency invariant on `SidebarGrouping` and add doc warnings on both `orderedSidebarItems` flavors so future render-path callers reach for the ID variant. `SidebarView`'s focused-scene-value `visibleHotkeyWorktreeRows` still uses the full `[SidebarItemFeature.State]` flavor; that path feeds the menu bar which needs the row details, and it does not invalidate the list render.
sbertix
force-pushed
the
sbertix/sidebar-row-feature
branch
from
095b733 to
6139195
Compare
`SidebarView` was publishing `[SidebarItemFeature.State]` through
`focusedSceneValue(\.visibleHotkeyWorktreeRows)`. Every per-row
mutation (PR query started/finished, lifecycle change, running-script
tick) re-pushed a new array, which re-evaluated `WorktreeCommands`'
body, rebuilt `CommandMenu("Worktrees")` and the sibling Window /
View / Help groups, and made AppKit drop the user's hover mid-open.
Ship a lightweight `HotkeyWorktreeSlot { id, name, repositoryID }`
projection (Equatable / Hashable). The slot carries only fields the
menu actually consumes and that mutate exclusively during sidebar
reconcile, so the focused-scene-value dedupes across the noisy
per-row ticks. Same story as the original #289 fix, different
trigger this time.
`worktreeID(byOffset:)` was iterating each expanded repository's raw `repository.worktrees` list, which ignored the pinned / unpinned bucket order, didn't include pending rows, and let archived rows sneak in. Cmd+Down landed on whatever git's enumeration order happened to give us, so arrow navigation, Cmd+1..9 slot selection, and the visible sidebar all walked three different sequences. Route navigation through `orderedSidebarItemIDs(...)` so all three agree: [main, pinnedTail, pending, unpinnedTail] across expanded repositories.
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
1 participant
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
Per-row sidebar architecture so the sidebar stops re-rendering every row during coding-agent tool storms, plus a tail of perf / reliability hardening on top.
Per-row Observation scoping
Swift Observation tracks
@ObservableStateat the property level, so any mutation to anIdentifiedArrayOfelement invalidated every observer of the aggregate. A single Claude / Codex tool-call hook (high-frequency under load) was re-rendering N rows in the sidebar plus the tab bar.AgentPresenceManagerinto aAgentPresenceFeatureTCA reducer; replaceworktreeInfoByIDwithIdentifiedArrayOf<WorktreeInfoEntry>to prepare for per-row scoping.SidebarItemFeature— a per-row reducer with its own reconcile path; mirror every per-row aggregate (lifecycle, diff stats, PR, running scripts, terminal projection) into asidebarItemscollection.store.scope— each row gets its own Observation registrar so a per-row write only re-renders the affected row, not the whole list.SidebarItemModel/WorktreeInfoEntryand the parallel sets (runningScriptsByWorktreeID,archivingWorktreeIDs,deleteScriptWorktreeIDs,deletingWorktreeIDs,pendingSetupScriptWorktreeIDs). Per-row data now lives entirely onSidebarItemFeature.State.AgentPresenceFeatureandWorktreeTerminalManagerprojection events fan out into per-rowagentSnapshotChanged/terminalProjectionChangedactions; diff, PR, lifecycle, and running-scripts mutations all flow through per-row actions instead of being written from parent reducers. Back-channel closures onWorktreeTerminalManager(sendPresenceAction,hasAgentActivity,agentsForSurfaces) are gone.Statemethods — move free-floating sidebar helpers ontoRepositoriesFeature.State; wirepullRequestQueryStartedfrom production so the watermark is set at dispatch rather than in tests.Lifecycleenum, makesurfaceToItemIDa computed property so the reverse index can't drift, delete deadrosterChanged/RosterDeltaaction, callsyncSidebarafter pin / unpin moves, centralize the orphan-row drop inreconcileSidebarItems, extendXCTAssertSidebarConsistentto assert per-bucket order, unifysidebarDisplayNameonSidebarItemFeature.State.Reliability and perf hardening
kill(2)check off the main actor through a newlivenessSweepResultaction that carries both the snapshot and the alive delta. The apply step subtracts only the pids the sweep proved dead from the current record, so any.sessionStartor.sessionEndthat lands during the off-main hop stays authoritative. Regression tests cover both mid-hop sessionStart and sessionEnd races.statusCheckRollupunder the GraphQL gateway's 504 threshold on busy CI repos. On a timeout, retry once after a 1s backoff driven by@Dependency(\.continuousClock)so cancellation propagates and tests can drive fake time. Classify at therunGhcatch site by pattern-matchingShellClientError.stderr(HTTP[/0-9.]* 504) rather than substring matching the full description; newGithubCLIError.gatewayTimeoutcase. Test locks the "retry once, then propagate" contract.pullRequestChangedand itspullRequestBranchAtQueryTimewatermark stayed pinned, blocking every future refresh. Union the queried IDs with the response keys so every queried row clears its watermark exactly once.tabIsRunningByIdbookkeeping — the dict was private with no external observers; its only read was the equality-guard write itself.isTabBusyalready computes from the surface tree on demand.+N/-Ndiff counts, the PR number, and agent avatars stop collapsing to…. Title takes the squeeze instead.Test plan
make build-apppasses.AgentPresenceFeatureTests(incl. new mid-hop sessionStart / sessionEnd race tests),GithubCLIClientTests(incl. new gateway-timeout retry + propagation tests usingImmediateClock),RepositoriesFeatureSidebarTests(incl. new queried-but-missing watermark test),SidebarItemFeatureTests.Close #256