Repository navigation
Sidebar: make rows height-stable under agent churn (no height animation, eager markdown) - #6026
Closed
azooz2003-bit wants to merge 3 commits into
Closed
azooz2003-bit wants to merge 3 commits into
azooz2003-bit wants to merge 3 commits into
Conversation
…on, eager markdown) Two changes that stop agent activity from continuously varying sidebar row heights, which kept re-feeding the sidebar-wide layout/measurement cycle at animation frame rate (#5764, #5845): 1. Remove the three implicit .animation(value:) modifiers on agent-mutable snapshot fields (latestLog, progress, metadataBlocks.count) and reduce the four height-moving .transition(.opacity.combined(.move(edge: .top))) modifiers in TabItemView's log/progress/metadata sections to .transition(.opacity). While a row-height animation runs, every frame produces a different LazyVStack content height; with dozens of agent sessions some row is always animating. Content changes now apply in one discrete layout pass. 2. SidebarMetadataMarkdownBlockRow parsed its markdown in onAppear into @State: a guaranteed nil -> attributed swap (and height change) on every first appearance of every block scrolling in. It now renders inline via a new SidebarMetadataMarkdownRenderer with a bounded (512-entry) memo cache, so the FIRST render is already attributed and appearance performs no state write and no height change. Matches the SidebarWorkspaceDescriptionText sibling, plus memoization to keep repeat body evals cheap and growth bounded. WWDC backing: lazy rows must be height-stable after appearing; initialize row state in the initializer, not onAppear (WWDC26 "Dive into lazy stacks", 321); keep body cheap / precompute (WWDC23 10160, WWDC25 306). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a bounded in-memory memoization cache for rendered markdown (SidebarMetadataMarkdownRenderer), wires the new file into the Xcode project, and refactors ContentView to call the renderer inline while simplifying transitions and removing implicit animations. ChangesMarkdown Caching and Integration
🎯 3 (Moderate) | ⏱️ ~20 minutes
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 passed)
✨ 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 |
Contributor
Greptile SummaryThis PR fixes per-frame sidebar layout churn in agent-heavy sessions by removing implicit height animations from agent-mutable snapshot fields and replacing deferred
Confidence Score: 5/5Safe to merge — the animation removal and eager markdown rendering changes are narrowly scoped to the sidebar row layout path with no regressions identified. The two root-cause fixes (animation removal, inline rendering with a bounded cache) are each self-contained and well-reasoned. The cache correctly handles nil failed-parse entries via updateValue, the @mainactor isolation is sound, and the ContentView changes touch only transition/animation modifiers and the onAppear state pattern. No new correctness or concurrency issues are introduced by this PR. No files require special attention. SidebarMetadataMarkdownRenderer.swift is the most novel change but its logic is straightforward and the previously-noted concerns were either fixed (nil eviction) or are bounded constants (FIFO eviction cost). Important Files Changed
Reviews (3): Last reviewed commit: "Merge origin/main (pick up workflow-guar..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+37
to
+38
| cache[markdown] = parsed | ||
| insertionOrder.append(markdown) |
Contributor
There was a problem hiding this comment.
cache[markdown] = parsed when parsed is nil is equivalent to cache.removeValue(forKey: markdown) — the key is never stored. insertionOrder.append(markdown) still runs unconditionally, so a markdown string that fails to parse is appended to insertionOrder on every call but never inserted into cache. Once insertionOrder.count >= capacity, evictions fire against phantom keys, prematurely ejecting valid cache entries. For a workspace that has any malformed metadata block, every body re-evaluation retries the parse and grows insertionOrder with a duplicate — defeating the height-stability goal of this PR for those rows.
Suggested change
| cache[markdown] = parsed | |
| insertionOrder.append(markdown) | |
| cache.updateValue(parsed, forKey: markdown) | |
| insertionOrder.append(markdown) |
Comment on lines
+22
to
+36
| private static var insertionOrder: [String] = [] | ||
| private static let capacity = 512 | ||
|
|
||
| static func rendered(_ markdown: String) -> AttributedString? { | ||
| if let hit = cache[markdown] { | ||
| return hit | ||
| } | ||
| let parsed = try? AttributedString( | ||
| markdown: markdown, | ||
| options: .init(interpretedSyntax: .full) | ||
| ) | ||
| if insertionOrder.count >= capacity, let oldest = insertionOrder.first { | ||
| insertionOrder.removeFirst() | ||
| cache.removeValue(forKey: oldest) | ||
| } |
Contributor
There was a problem hiding this comment.
Array.removeFirst() is O(n) — it shifts all remaining elements left. On a full 512-entry cache every eviction copies up to 511 pointers on the main actor. In agent-heavy sessions with many distinct blocks the eviction path can fire on every body pass for uncached keys. A simple head-index or ring-buffer gives O(1) removal.
Suggested change
| private static var insertionOrder: [String] = [] | |
| private static let capacity = 512 | |
| static func rendered(_ markdown: String) -> AttributedString? { | |
| if let hit = cache[markdown] { | |
| return hit | |
| } | |
| let parsed = try? AttributedString( | |
| markdown: markdown, | |
| options: .init(interpretedSyntax: .full) | |
| ) | |
| if insertionOrder.count >= capacity, let oldest = insertionOrder.first { | |
| insertionOrder.removeFirst() | |
| cache.removeValue(forKey: oldest) | |
| } | |
| private static var insertionOrder: [String] = [] | |
| private static var insertionHead: Int = 0 | |
| private static let capacity = 512 | |
| static func rendered(_ markdown: String) -> AttributedString? { | |
| if let hit = cache[markdown] { | |
| return hit | |
| } | |
| let parsed = try? AttributedString( | |
| markdown: markdown, | |
| options: .init(interpretedSyntax: .full) | |
| ) | |
| if insertionOrder.count >= capacity { | |
| let oldest = insertionOrder[insertionHead % capacity] | |
| cache.removeValue(forKey: oldest) | |
| insertionOrder[insertionHead % capacity] = markdown | |
| insertionHead += 1 | |
| } else { | |
| insertionOrder.append(markdown) | |
| } |
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!
…values) With [String: AttributedString?], `cache[markdown] = parsed` removes the key when parsed is nil, so unparseable blocks re-parsed on every body eval and appended phantom keys to insertionOrder, mis-evicting valid entries once at capacity. Caught by Greptile on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Good catch on the optional-dictionary subscript footgun — fixed in fc42869 with |
Collaborator
Author
|
Re CodeRabbit's "Cmux Swiftui State Layout" error (render-time mutation of the static memo cache): the rule targets the feedback-loop class — writes from body to OBSERVABLE state (@published / @State / store writes) that re-dirty the view graph and spin layout. A private static dictionary is not observable: no view depends on it, so mutating it during render cannot invalidate anything; it is memoization, same category as Foundation's own caches that run under body all the time. The suggested alternative (populate the cache in a non-render lifecycle, e.g. onAppear or the row initializer with @State) is exactly the height-changing appear-time write this PR removes. The principled long-term home for the parse is the snapshot builder (attributed string computed when the metadata block is ingested, carried on the snapshot), which also removes the inline parse from |
Collaborator
Author
|
Superseded by the combined PR #6033 (this PR's change is included verbatim there), opened to run a single CI/review/dogfood pass across all three sidebar-perf fixes. Closing; track 6033. |
azooz2003-bit
added a commit
that referenced
this pull request
Jun 13, 2026
…(Fix 5: F9, F11) F9 — SidebarWorkspaceFrameAnchorModifier used if/else around `content`, so the drop-target frame anchor was structurally present only while collecting. Flipping isEnabled at drag start/end (shouldCollectWorkspaceDropTargets) gave every materialized row a new structural identity, recreating each TabItemView subtree (dropping per-row @State, forcing fresh snapshot builds + relayout) inside the drag transaction. Make it branchless: always apply anchorPreference, emit `isEnabled ? [id: anchor] : [:]`. Row identity stays stable across the flip; the emitted preference is byte-identical to before while dragging, and the virtualization-defeating *reader* stays gated on the drag (#5325), so emitting an empty dict when idle costs nothing. F11 — SidebarWorkspaceDescriptionText carried `.id(description)`, keying the view's identity to agent-updated text, so every description rewrite tore down and recreated the view (markdown re-parse, layout caches) on top of the in-place re-render the text change already triggers. Delete it; the view updates in place. Verified on tag rowapat: simulated sidebar drag drives the frame-anchor path (drop indicator tracked across all steps, socket 31ms, zero layout-loop symbols); markdown description renders bold/italic and wraps with no .id. Deferred from Fix 5 with rationale: F8 (per-row rowHeight @State feeding the drop delegate) and F13 (per-row Finder .task pre-warm) — both now low-value post-#6026 (rows are height-stable) and both carry real regression surface (reorder hit-testing via a SidebarTabDropDelegate interface change; Reveal-in-Finder is `.disabled(url == nil)` gated on the pre-warm), so they warrant a focused follow-up rather than riding with these contained changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
azooz2003-bit
added a commit
that referenced
this pull request
Jun 13, 2026
…lazy Finder resolve (Fix 5: F9/F11/F13) (#6052) * Sidebar: branchless frame-anchor modifier + drop the description .id (Fix 5: F9, F11) F9 — SidebarWorkspaceFrameAnchorModifier used if/else around `content`, so the drop-target frame anchor was structurally present only while collecting. Flipping isEnabled at drag start/end (shouldCollectWorkspaceDropTargets) gave every materialized row a new structural identity, recreating each TabItemView subtree (dropping per-row @State, forcing fresh snapshot builds + relayout) inside the drag transaction. Make it branchless: always apply anchorPreference, emit `isEnabled ? [id: anchor] : [:]`. Row identity stays stable across the flip; the emitted preference is byte-identical to before while dragging, and the virtualization-defeating *reader* stays gated on the drag (#5325), so emitting an empty dict when idle costs nothing. F11 — SidebarWorkspaceDescriptionText carried `.id(description)`, keying the view's identity to agent-updated text, so every description rewrite tore down and recreated the view (markdown re-parse, layout caches) on top of the in-place re-render the text change already triggers. Delete it; the view updates in place. Verified on tag rowapat: simulated sidebar drag drives the frame-anchor path (drop indicator tracked across all steps, socket 31ms, zero layout-loop symbols); markdown description renders bold/italic and wraps with no .id. Deferred from Fix 5 with rationale: F8 (per-row rowHeight @State feeding the drop delegate) and F13 (per-row Finder .task pre-warm) — both now low-value post-#6026 (rows are height-stable) and both carry real regression surface (reorder hit-testing via a SidebarTabDropDelegate interface change; Reveal-in-Finder is `.disabled(url == nil)` gated on the pre-warm), so they warrant a focused follow-up rather than riding with these contained changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar: resolve Finder dir at context-menu time, drop per-row pre-warm (Fix 5: F13) Each TabItemView ran `.task(id: finderDirectoryCacheKey)` that stat'd the workspace's directory (FileManager.fileExists) on every row appearance, just to populate the `.disabled(finderDirectoryURL == nil)` gate on the "Show in Finder" context-menu item. Scrolling N workspaces issued N disk stats + N deferred row invalidations. "Show in Finder" already re-validates the directory at click time (WorkspaceFinderDirectoryOpener.openInFinder), so the pre-warm only gated the item's enabled state. Resolve it synchronously while the context menu builds (one stat per right-click, user-initiated) via the new WorkspaceFinderDirectoryResolver.existingDirectoryURLNow. Removes the per-row @State, the `.task`, and the now-dead WorkspaceFinderDirectoryCache / CacheKey / cache(for:). Behavior unchanged: item disabled when the dir is missing, reveals it when present. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar: read row height from a reference cell, not @State (Fix 5: F8) Each sidebar row (TabItemView) and group header (SidebarWorkspaceGroupHeaderView) kept its measured height in @State, written from a background GeometryReader on appearance (a guaranteed 1 -> realHeight write) and on every later height change. That write forced a second full body evaluation of the row inside the lazy placement pass — and the write was load-bearing: the drop delegate captured the height at body-eval time, so without the re-eval it would have been stuck at 1. Replace @State with SidebarRowHeightStore, a tiny reference cell the probe mutates in place (no invalidation). The drop delegates (SidebarTabDropDelegate, SidebarWorkspaceGroupHeaderDropDelegate) now hold the store and read `height` LAZILY at drop time via a computed property, so they always see the current measurement without a body re-eval reconstructing them. Drop targets with no measured row (empty area, first-row gap) pass a nil store; the extension-sidebar delegates keep their fixed constants. Height value at drop time is unchanged (same measurement the probe always fed), so reorder hit-testing behavior is preserved — only the read path changes. Verified at runtime: drags over rows + group header mount the delegates, app responsive (64ms), zero layout-loop symbols, sidebar renders. Real-drag drop position needs human dogfood (no socket path drives DropInfo hit-testing). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * F13 fix: gate Show-in-Finder on configured path, no body-time stat (autoreview P1) The previous F13 commit resolved the Finder directory with a synchronous existence stat in the context-menu builder, assuming it ran only on right-click. SwiftUI evaluates the `.contextMenu { }` items as part of the row body, so that reintroduced a synchronous FileManager.fileExists on the main actor for every row body eval of a configured workspace — exactly the hot-path filesystem IO the sidebar avoids. Gate the menu item on whether a directory is configured (WorkspaceFinderDirectoryResolver.path, in-memory, IO-free) and defer the stat to the click action, where WorkspaceFinderDirectoryOpener already re-validates and beeps if the directory is gone. Removes the synchronous helper. Behavior unchanged for existing directories; a configured-but-deleted directory now shows an enabled item that beeps on click instead of a disabled item. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Revert "Sidebar: read row height from a reference cell, not @State (Fix 5: F8)" This reverts commit 3aaf643. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
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.
Problem
Agent activity continuously varies sidebar row heights, which keeps re-feeding the sidebar-wide layout/measurement cycle at animation frame rate (#5764, #5845). Two mechanisms:
.animation(.easeInOut(0.2), value:)modifiers on agent-mutable snapshot fields (latestLog,progress != nil,metadataBlocks.count) plus height-moving.move(edge: .top)transitions in TabItemView's log/progress/metadata sections. While a row-height animation runs, every frame produces a different LazyVStack content height; with dozens of agent sessions some row is always animating, so the whole-sidebar layout re-runs at display refresh rate for the full 0.2s after every log/progress/metadata event.SidebarMetadataMarkdownBlockRowparsed its markdown inonAppearinto@State: a guaranteed nil -> attributed swap (and intrinsic height change) on every first appearance of every metadata block scrolling in. Agent-heavy workspaces, the livelock precondition, are exactly the rows with metadata blocks.Fix
.animation(value:)modifiers; reduce the four TabItemView transitions to.transition(.opacity)(the fifth, in the extension sidebar, is untouched). Content changes now apply in one discrete layout pass; rows are bouncier-less but height-stable.SidebarMetadataMarkdownRenderer(own file, keeps ContentView under its length budget): inline rendering with a bounded 512-entry memo cache, so the FIRST render is already attributed and appearance performs no state write and no height change. Matches the existingSidebarWorkspaceDescriptionTextinline-parse sibling; the cache bound also prevents unbounded growth in long agent sessions.WWDC backing: lazy rows must be height-stable after appearing and row state belongs in the initializer, not onAppear (WWDC26 "Dive into lazy stacks"); keep body cheap / precompute (WWDC23 10160, WWDC25 306).
Companion to #6019 (structural loop-edge removal) and #6021 (cheap ForEach identity); all three are independent.
Preflighted on tag
hstable: markdown metadata block (report_meta_blockwith bold/italic/list) renders attributed from the first frame; sidebar drags + workspace creation responsive. Localization audit: no user-facing strings added or changed.Note on tests: the regression is per-frame layout churn, not testable as a unit assertion; the existing snapshot/UI suites cover rendering. Behavioral gate is the trace diff in dogfood (accepted
workspaceRowsMeasurementwrites drop from per-frame to per-event, per the validation plan in #5764).🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Stabilizes sidebar row heights during agent churn to stop per-frame layout re-measurement and keep scrolling responsive. Addresses issue-5764 by removing height animations and rendering metadata markdown eagerly with a bounded cache.
latestLog,progress, andmetadataBlocks.count; simplified transitions to.opacityinTabItemViewto apply changes in one layout pass.SidebarMetadataMarkdownRendererwith a 512-entry memo cache to render attributed text on first paint, replacingonAppearparsing and height changes.updateValue, preventing re-parsing and incorrect evictions at capacity.Written for commit c8e71c1. Summary will update on new commits.
Summary by CodeRabbit