Repository navigation
Fix stuttery iOS workspace refresh animation - #8397
Closed
azooz2003-bit wants to merge 9 commits into
Closed
azooz2003-bit wants to merge 9 commits into
azooz2003-bit wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe iOS workspace list now coordinates pull-to-refresh completion with authoritative diffable snapshots and collapse settling. It adds custom refresh geometry and presentation, lifecycle validation, teardown handling, deterministic tests, and a DEBUG preview refresh controlled by an environment-configured delay. ChangesWorkspace refresh coordination
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WorkspaceListTableCoordinator
participant WorkspaceListView
participant DiffableDataSource
User->>WorkspaceListTableCoordinator: pull workspace list
WorkspaceListTableCoordinator->>WorkspaceListView: invoke refresh
WorkspaceListView-->>WorkspaceListTableCoordinator: refresh completion generation
WorkspaceListTableCoordinator->>DiffableDataSource: apply authoritative snapshot
DiffableDataSource-->>WorkspaceListTableCoordinator: snapshot completion
WorkspaceListTableCoordinator->>WorkspaceListTableCoordinator: collapse refresh presentation
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 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/WorkspaceListTableCoordinator.swift`:
- Around line 331-334: Update the mismatch branch in the refresh completion
handling to ignore stale completions instead of resetting refreshLifecycle or
clearing activeRefreshControl; only the currently authoritative refresh
generation/ID may mutate active refresh state, while invalidation continues
cleaning up the replaced control. Add a regression test covering refresh A being
replaced by refresh B before A completes, asserting A’s late completion leaves B
active and its lifecycle intact.
- Around line 361-363: Update the scroll-view delegate handling near
scrollViewDidScroll(_:) by adding scrollViewDidEndDragging(_:willDecelerate:)
and call settleRefreshCollapse(in:) with the ending scroll view there,
preserving the existing scrollViewDidScroll behavior.
In
`@Packages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceListRefreshCoordinatorTests.swift`:
- Around line 34-36: Replace the fixed ten-iteration Task.yield waits in the
affected tests with explicit completion signals: await the refreshDidComplete
probe and the RecordingRefreshControl.endRefreshing() probe before asserting
results. Ensure both refresh continuation and diffable snapshot completion are
observed, using bounded predicate polling only if necessary, and remove the
fixed scheduler-yield loops.
🪄 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
Run ID: 30b643cc-30dc-424a-befe-e6981933f0d3
📒 Files selected for processing (8)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListLayoutPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListRefreshLifecycle.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView+Table.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceListRefreshCoordinatorTests.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceListRefreshLifecycleTests.swift
Comment on lines
+361
to
+363
| func scrollViewDidScroll(_ scrollView: UIScrollView) { | ||
| settleRefreshCollapse(in: scrollView) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swift'
# Map the file structure, then inspect the relevant section.
ast-grep outline "$file" --view expanded || true
echo '---'
sed -n '1,520p' "$file" | cat -n | sed -n '300,430p'Repository: manaflow-ai/cmux
Length of output: 7419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swift'
# Inspect the implementation and nearby delegate methods.
grep -n "settleRefreshCollapse\|scrollViewDid\|endRefreshing\|isTracking\|refresh" "$file" || true
echo '---'
sed -n '1,260p' "$file" | cat -n | sed -n '1,260p'Repository: manaflow-ai/cmux
Length of output: 15126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swift'
nl -ba "$file" | sed -n '320,420p'Repository: manaflow-ai/cmux
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swift'
echo "[delegate methods]"
grep -nE 'func scrollViewDid|func settleRefreshCollapse|endRefreshing|isTracking' "$file" || true
echo
echo "[context]"
nl -ba "$file" | sed -n '340,390p'Repository: manaflow-ai/cmux
Length of output: 573
Call settleRefreshCollapse(in:) on drag end too.
scrollViewDidScroll(_:) alone can miss the transition out of isTracking if endRefreshing() happens while the finger is still down and no further offset change occurs, leaving snapshot animation suppression stuck until the next scroll. Add scrollViewDidEndDragging(_:willDecelerate:) here as well.
🤖 Prompt for 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.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swift`
around lines 361 - 363, Update the scroll-view delegate handling near
scrollViewDidScroll(_:) by adding scrollViewDidEndDragging(_:willDecelerate:)
and call settleRefreshCollapse(in:) with the ending scroll view there,
preserving the existing scrollViewDidScroll behavior.
Source: Coding guidelines
Contributor
Greptile SummaryThis PR replaces a custom pull-to-refresh gesture state machine with UIKit's standard
Confidence Score: 5/5Safe to merge. The change eliminates multiple concurrent geometry writers and delegates full visual ownership of the refresh lifecycle to UIKit. The generation-counter handshake correctly sequences endRefreshing() inside the datasource apply completion, so the spinner collapses only after the refreshed snapshot is committed. Task cancellation on refresh-control removal, the UUID-based guard against stale task callbacks, and the isRefreshing animation suppression are all handled correctly. The two residual edge cases noted in prior review threads pre-date this PR and are not worsened by the change. WorkspaceListTableCoordinator.swift carries the two pre-existing lifecycle edge cases noted in earlier review threads; the rest of the changed files are straightforward wiring or test scaffolding. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant UIRefreshControl
participant Coordinator as WorkspaceListTableCoordinator
participant Task as Task[@MainActor]
participant SwiftUI as WorkspaceListView (SwiftUI)
participant DataSource as UITableViewDiffableDataSource
User->>UIRefreshControl: pull-to-refresh gesture
UIRefreshControl->>Coordinator: refreshRequested(_:) [valueChanged]
Coordinator->>Task: "Task { await refresh() }"
Note over Coordinator: refreshTask = (uuid, task)
Task->>SwiftUI: await refresh() [async action]
SwiftUI-->>Task: data model updated, returns
Task->>SwiftUI: refreshDidComplete() increment generation
Note over Task: refreshTask = nil
SwiftUI->>Coordinator: "update(configuration:in:) completesRefresh=true"
Coordinator->>DataSource: apply(snapshot, animatingDifferences: false)
DataSource-->>UIRefreshControl: endRefreshing() in completion block
UIRefreshControl-->>User: spinner collapses
%%{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 User
participant UIRefreshControl
participant Coordinator as WorkspaceListTableCoordinator
participant Task as Task[@MainActor]
participant SwiftUI as WorkspaceListView (SwiftUI)
participant DataSource as UITableViewDiffableDataSource
User->>UIRefreshControl: pull-to-refresh gesture
UIRefreshControl->>Coordinator: refreshRequested(_:) [valueChanged]
Coordinator->>Task: "Task { await refresh() }"
Note over Coordinator: refreshTask = (uuid, task)
Task->>SwiftUI: await refresh() [async action]
SwiftUI-->>Task: data model updated, returns
Task->>SwiftUI: refreshDidComplete() increment generation
Note over Task: refreshTask = nil
SwiftUI->>Coordinator: "update(configuration:in:) completesRefresh=true"
Coordinator->>DataSource: apply(snapshot, animatingDifferences: false)
DataSource-->>UIRefreshControl: endRefreshing() in completion block
UIRefreshControl-->>User: spinner collapses
Reviews (7): Last reviewed commit: "fix(ios): finish refresh after table upd..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+361
to
+363
| func scrollViewDidScroll(_ scrollView: UIScrollView) { | ||
| settleRefreshCollapse(in: scrollView) | ||
| } |
Contributor
There was a problem hiding this comment.
collapsing → idle transition has no fallback for animation end
observeCollapse is only invoked from scrollViewDidScroll, which fires on every frame of the UIKit refresh-control collapse animation. If the animation is interrupted mid-flight by a concurrent programmatic setContentOffset (e.g., search-bar appearance, safe-area inset change, or a push navigation triggering a scroll restoration), scrollViewDidEndScrollingAnimation fires for the new scroll but scrollViewDidScroll may not subsequently reach within the 0.5 pt tolerance at the original restingTopY. The lifecycle stays stuck in .collapsing, keeping suppressesSnapshotAnimations = true for all subsequent diffable snapshots until the user manually scrolls. Implementing scrollViewDidEndScrollingAnimation (and scrollViewDidEndDecelerating) to also call settleRefreshCollapse(in:) would cover these interruption paths and make the settlement definitive.
Comment on lines
+323
to
+325
| guard let refreshID = refreshLifecycle.begin( | ||
| currentGeneration: configuration.refreshCompletionGeneration | ||
| ) else { return } |
Contributor
There was a problem hiding this comment.
collapsing is silently swallowed
If refreshLifecycle.begin() returns nil (phase is anything other than .idle), the method returns early without calling refreshControl.endRefreshing(). During the .collapsing phase isRefreshing is already false so UIKit won't normally let the user initiate a new pull, but if the refresh action is extremely fast (action completes and endRefreshing fires before the user releases their pull gesture), a second UIControlEvents.valueChanged can fire on the same control. In that case the refreshControl is left spinning with no lifecycle entry, no Task, and no future endRefreshing call. Adding refreshControl.endRefreshing() in the else { return } path would prevent the control from hanging.
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/WorkspaceListTableCoordinator.swift`:
- Around line 53-59: Replace the DispatchQueue.main.async hop in
WorkspaceListTableCoordinator.init’s scheduleRefreshCollapse closure with a
direct action() call from the authoritative snapshot completion. If a separate
phase is required, use an explicit UIKit completion callback instead, without
relying on delayed dispatch for refresh rendering.
- Around line 403-410: The failure branch of the queued closure in
scheduleRefreshCollapse must cancel the matching pending collapse before
returning. Preserve token validation by invoking
refreshLifecycle.cancelCollapse(collapseID) with the captured collapse ID, so
stale closures cannot affect a newer lifecycle or leave it in
.collapseScheduled.
🪄 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
Run ID: cb2d807c-dd5b-4dc8-99b2-249dc92b6140
📒 Files selected for processing (4)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListRefreshLifecycle.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceListRefreshCoordinatorTests.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceListRefreshLifecycleTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/WorkspaceListTableCoordinator.swift`:
- Line 57: Remove the production-only refreshTaskDidFinish callback seam from
the coordinator, including its property, initializer parameter, stored
assignment, and invocation. Update
staleCancelledTaskCannotReleaseReplacementTaskOwnership to observe task
ownership through `@testable` import instead, or use an existing genuine
production task-owner abstraction without adding test-only APIs under Sources.
🪄 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
Run ID: 29133495-fd91-4b85-ac58-91aa24423c38
📒 Files selected for processing (5)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListRefreshLifecycle.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTable.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceListRefreshCoordinatorTests.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceListRefreshLifecycleTests.swift
lawrencecchen
added
the
stale-revisit
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
3 participants
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
Root cause
The custom branch had multiple writers for UIScrollView geometry. The baseline native path also called endRefreshing before SwiftUI delivered the refreshed data, so an animated diffable snapshot could land during UIKit’s collapse.
Mechanism
UIRefreshControl owns all scroll geometry. The refresh action bumps a completion generation after the data round-trip. The coordinator applies that generation’s diffable snapshot without row animation, then calls endRefreshing from the snapshot completion.
Testing
Commits