Repository navigation
Fix iOS workspace-list scroll stutter from live updates - #9139
Merged
Merged
Conversation
…live-update fixture mode The DEBUG scroll-metrics probe now records display-link frame pacing during its sweep (hitch frames, hitch ms/s, max frame ms) so workspace list scroll work is quantifiable before and after changes. The layout preview fixture gains CMUX_UITEST_WORKSPACE_LIST_PREVIEW_LIVE_UPDATES=timestamps, which restamps previewAt/lastActivityAt sub-minute without visible changes - the exact update shape the Mac emits while agents stream. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd re-rendering unchanged rows Two measured main-thread costs ran on every workspace-list emission while agents stream (the iOS workspace-list scroll stutter): 1. Any workspace field delta reconfigured the row, but the Mac restamps preview_at/last_activity_at from the latest notification on every emission while the row renders that time at minute granularity. Reconfigure now decides by render equivalence: full struct equality (fail-closed for future fields) with same-minute timestamps normalized out. 2. Payload-only updates rode NSDiffableDataSourceSnapshot.apply, which runs the diffable apply queue plus UITableView's whole batch-update pass per tick (~1.3ms on an M-series simulator, more on device) with nothing to diff. When no changed row's height key moved, the visible changed cells are now re-configured in place and offscreen rows pick up the payload on dequeue; height-changing payloads keep the snapshot path so UITableView re-queries heights. 30s fixture window, 400 rows, updates every 80ms, M-series simulator: timestamp-only churn 3.27s -> 2.59s CPU, visible churn 3.33s -> 3.00s; the diffable-apply subtree disappears from the sample profile. Sweep invariants hold: 0 contentSize corrections, 1 distinct draw height. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe DEBUG workspace preview gains configurable live-update modes, the scroll probe reports display-link frame pacing, and the table coordinator routes stable payload changes through in-place cell reconfiguration while preserving snapshot application for structural changes. ChangesWorkspace preview live updates
Scroll frame pacing metrics
Workspace table payload routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CADisplayLink
participant WorkspaceListScrollMetricsProbe
participant ScrollMetricsReport
CADisplayLink->>WorkspaceListScrollMetricsProbe: provide frame timestamps
WorkspaceListScrollMetricsProbe->>WorkspaceListScrollMetricsProbe: calculate intervals and hitch statistics
WorkspaceListScrollMetricsProbe->>ScrollMetricsReport: write framePacing to scroll-metrics.json
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (21 passed)
✨ Finishing Touches 💡 1📝 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListLayoutPreviewView.swift`:
- Around line 50-51: The .timestampsOnly update in seeded() must preserve each
row’s existing normalized minute bucket. Compute one sub-minute restamp relative
to the row’s current timestamp and assign that same value to both previewAt and
lastActivityAt, instead of replacing both with the current Date().
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListScrollMetricsProbe.swift`:
- Around line 244-269: Update the framePacing calculation to iterate over each
frame duration together with its corresponding sweepExpectedDurations value,
classifying hitches using that frame’s expected duration and accumulating
duration minus that value. Retain the median expected duration only for
FramePacing.expectedFrameMs, leaving the other aggregate metrics unchanged.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swift`:
- Around line 49-69: Move PayloadApplyRoute, lastPayloadApplyRoute, and
recordPayloadApplyRoute out of WorkspaceListTableCoordinator into a dedicated
DEBUG-only extension/file following the WorkspaceListScrollMetricsProbe pattern.
Guard every recordPayloadApplyRoute call site with `#if` DEBUG, including the
changed ID mapping, so Release builds perform no tracking or array allocation.
Preserve the existing test-observable routing behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0b3e3c01-b567-46e8-a2c8-590d6390ca22
📒 Files selected for processing (4)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListLayoutPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListScrollMetricsProbe.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceListScrollUpdateTests.swift
Collaborator
Author
|
CI evidence: the five new WorkspaceListScrollUpdateTests pass on the iPhone simulator in https://github.com/manaflow-ai/cmux/actions/runs/30437841665 (test_filter=CmuxMobileShellUITests, 277 tests). The run's only failure, "uses the Japanese localized count without English inflection markup" in TerminalArtifactChildCountFormatterTests, fails identically on main in https://github.com/manaflow-ai/cmux/actions/runs/30439529771 (272 tests, same 3 issues), so it predates this branch; the 5-test delta between the runs is exactly the new coverage. The unfiltered dispatch https://github.com/manaflow-ai/cmux/actions/runs/30434879901 hit the 35-minute job timeout on a slow runner plus main's pre-existing mobile-core-package and package-conventions-lint failures; neither is related to this change. |
…s, in-bucket fixture restamps The apply-route test hook moves out of the production coordinator into a dedicated DEBUG file (extension + registry) with #if DEBUG call sites, so Release builds also stop allocating the changed-id array. The scroll probe judges each sweep frame against the expected interval captured at the callback that started it instead of the median, so a mid-sweep refresh-rate change is not misclassified as a hitch. The timestamps fixture mode restamps relative to each row's own clock so the first tick no longer jumps seeded hours-old timestamps across a rendered minute. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListLayoutPreviewView.swift`:
- Around line 52-62: The timestamp restamp in the preview update logic must
remain within the current minute bucket. Replace the unconditional one-second
addition in the workspaces update block with logic that alternates between two
second values in the existing minute, preserving the timestamp’s date, hour, and
minute while avoiding any minute rollover.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator`+ApplyRouteProbe.swift:
- Around line 32-50: Update WorkspaceListApplyRouteProbe so route entries are
tied to coordinator lifetime rather than retained in the process-wide
ObjectIdentifier dictionary. Use coordinator-owned/associated storage or
weak-key storage with cleanup, ensuring deallocated coordinators and their
routes are released and address reuse cannot return stale routes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71ad6d09-e968-4c13-b663-d1a7af8ac444
📒 Files selected for processing (4)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListLayoutPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListScrollMetricsProbe.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator+ApplyRouteProbe.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swift
…obe storage The timestamps fixture bump now wraps to the start of the row's current minute instead of crossing into the next one, so every tick honors the mode's zero-work contract. The DEBUG route registry keys weakly through NSMapTable so entries die with their coordinator and a reused address cannot return a predecessor's route. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Final-head verification (https://github.com/manaflow-ai/cmux/actions/runs/30497899613): all five WorkspaceListScrollUpdateTests pass. Two unrelated failures: the Japanese localized-count formatter test (pre-existing, fails identically on main in https://github.com/manaflow-ai/cmux/actions/runs/30439529771) and a one-off deadline race in TerminalFolderTapPolicyTests ("disabled still accepts a fast classification before the deadline"), which shares no code with this PR and passed in both earlier runs of this suite on this branch (https://github.com/manaflow-ai/cmux/actions/runs/30437841665, https://github.com/manaflow-ai/cmux/actions/runs/30497207325). All review threads resolved. |
This was referenced Jul 30, 2026
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.
Scrolling the iOS workspace list stuttered slightly while agents were active. Static scrolling profiled clean in an isolated simulator; the hitches came from list updates landing mid-scroll. Two main-thread costs ran on every workspace-list emission.
First, the Mac restamps
preview_at/last_activity_atfrom the latest notification on every list emission, while the row renders that time at minute granularity, so sub-minute restamps re-rendered rows that look identical. Reconfigure now decides by render equivalence: full struct equality (fail-closed for any field added later) with same-minute timestamps normalized out (WorkspaceListTableCoordinator.workspaceRenderEquivalent).Second, payload-only updates rode
NSDiffableDataSourceSnapshot.applywith nothing to diff, which still runs the diffable apply queue plus UITableView's whole batch-update pass per tick (~1.3ms on an M-series simulator, more on an iPhone Debug build). When no changed row's height key moved, the visible changed cells are now re-configured in place and offscreen rows pick up the new payload on dequeue; height-changing payloads (description arrival, chip digit growth, banner text) keep the snapshot path so UITableView re-queries heights.Measured on an isolated sim (400-row fixture, an update every 80ms, 30s window): timestamp-only churn drops 3.27s to 2.59s CPU and the
__UIDiffableDataSourceapply subtree disappears from the sample profile; visible churn drops 3.33s to 3.00s. Sweep invariants from #8186 hold: 0 contentSize corrections, 1 distinct draw height, and static scrolling is unchanged at 0 hitches. The remaining per-emission cost is SwiftUI re-derivingWorkspaceListView.body(about 1ms at realistic row counts on the sim); not addressed here.Also adds measurement infra: the DEBUG scroll probe records display-link frame pacing during its sweep, and the layout fixture gains a
timestampslive-update mode reproducing the Mac's restamp-only emissions.WorkspaceListScrollUpdateTestscovers route selection (no work for sub-minute restamps, in-place reconfigure for text/unread changes, snapshot apply for height and structure changes) and the equivalence edge cases (minute crossing, nil transitions, non-timestamp fields).🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes iOS workspace-list scroll stutter during live updates by ignoring sub-minute timestamp churn and reconfiguring visible cells in place when layout and heights are unchanged. Adds per-frame hitch metrics and a timestamps-only fixture that restamps within the current minute to reproduce and measure the issue.
Bug Fixes
previewAt/lastActivityAtvia a render-equivalence check so identical rows don’t re-render.NSDiffableDataSourceSnapshot.apply; keep snapshot applies when structure or heights change.New Features
timestampslive-update mode that restamps within the current minute (guaranteed render-equivalent);1keeps visible churn,offdisables updates. The DEBUG apply-route probe is isolated and now uses weak-key storage to avoid leaks and stale routes.Written for commit 5875bf6. Summary will update on new commits.
Summary by CodeRabbit