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.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/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
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.
An action runs while a cached-status stream may still be open: the screen stays interactive throughout, and the stream's accumulator still holds the local half from before the action. A late event folds that stale half into a status that overwrites the action's result, so a successful commit could show the old dirty working tree until a manual reload; a late stream error, including the bounded-wait expiry, could likewise mask the action's own failure message. Split the two roles the load token was serving. `loadGeneration` still owns the loading indicator and is bumped only by a load, so the indicator is always cleared by the load that set it. `statusGeneration` invalidates status and error writes and is bumped by an action as well, so an action supersedes an open stream without stranding the indicator.
Pull-to-refresh is not gated on a running action, so a refresh started after an action could still land last and wipe that action's failure message — the mirror image of the race the previous commit closed. Guard the action's own writes with the same token so whichever side started later wins, rather than whichever happens to finish later. Drop the first-status-only error latch from the streamed load: any writer that could have set an error since the stream started has already bumped the token and invalidated it, so the latch defends against nothing now.
An action supersedes any stream that is still open, which is what keeps a stale local half from reverting its result. But when the action itself fails it writes no status, so on a cold-cache entry the screen was left holding the pending status the stream had produced so far — reporting the remote as unavailable and withholding the pull-request actions until the user reloaded by hand. Re-run the cached-status stream after a failed action, without clearing the message that says why it failed.
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.
1 participant
Observed problem and reproduction
Open SwiftUI Source Control on a cold cache or slow network. Before this change, the screen showed a spinner until a full VCS refresh finished, including remote fetch-class work and pull-request lookup, even when cached local branch and changed-file status already existed.
Cause
The native Source Control screen used the forcing
vcs.refreshStatuspath for initial presentation and represented the remote half as already known. It had no screen-specific accumulator for the server's local-firstvcs.subscribeStatusstream or bounded pending-remote presentation.Change and ownership boundary
Initial presentation now streams cached VCS status, renders local branch and changed files immediately, and fills ahead/behind and pull-request state when remote data resolves. Explicit toolbar and pull-to-refresh actions still force
vcs.refreshStatus.Pending remote state is distinct from zero, so create-PR actions remain withheld until remote state is known. After the bounded presentation wait reports remote unavailability, the screen follows the target branch's shared monitor and accepts a later remote result. Action failures use their own alert, background load failures stay inline, actions supersede stale loading ownership, and pull-to-refresh cannot race a running action.
Conflict resolution merged target commit
22b22f1463b83963d851bd0574a36a20f224a28d. It preserves the target's sharedsourceControlStatusEventsmonitor and its thread/workspace consumers, then re-expresses this PR's Source Control screen behavior on top. Both monitor paths now use one event accumulator, including branch-change invalidation and remote-before-local ordering. Only the saphid-owned head branch was modified.Non-goals
This PR does not change the server VCS cache, polling/backoff, wire events, git commands, pull-request semantics, or other clients. It does not keep the bounded presentation stream open indefinitely. Web, desktop, and React Native mobile remain unchanged.
Affected areas
Validation
Current head:
9236039409eb3dadfe6004d3b38bc11d0e20ff11.T3CodeTests/FeatureToolStateTestson iPhone 17 Pro with isolated DerivedData: 28 passed, 0 failed, 0 skipped; realxcodebuildexit 0.swiftc -frontend -parse.git diff --checkpasses.5db6b01a8bb0b5fa174ebf3ddf60e61fea8aa29cc002f1869697da50173a8696, matching the reviewed and tested frozen diff.claude-opus-5high review found actionable race and presentation issues; those were adjudicated and fixed. The required fresh follow-up attempt produced no output by the five-minute threshold and was terminated, so no fresh-review verdict is claimed.Risks, untested paths, and known gaps
Evidence
No current-head screenshots or video are embedded. The prior head's proof is historical only and is not claimed for this conflict-resolved head. The required visual proof refresh is tracked in saphid/t3code-personal#150.
Owning issue and stack
Owning issue: saphid/t3code-personal#107. This PR targets the open SwiftUI parent branch and depends on #5178 landing. It is not stacked on #7345. Maintainer edits are enabled. Human review is not re-requested until current-head proof is complete.
Implementation and conflict resolution: GPT-5.6 Sol high in the Codex harness. Independent review attempt: Claude Opus 5 high through the direct Claude CLI.
Note
Medium Risk
iOS-only but touches async VCS streaming, timeouts, and race-prone UI updates that gate git/PR actions; server contracts are unchanged.
Overview
iOS Source Control no longer blocks on a full
refreshStatusfor the first paint. Initial load uses a newsourceControlStatusesstream overvcs.subscribeStatus, so branch and changed files appear from cache while ahead/behind and PR data fill in later. Toolbar reload and pull-to-refresh still forcesourceControlStatusso refresh bypasses the cache.FeatureSourceControlStatusgainsisRemoteKnown: pending remote is not treated as zero, and create-PR actions stay hidden until remote state is known.NativeSourceControlStatusAccumulatorcentralizes folding VCS events for both the bounded presentation stream and the existing shared monitor (branch changes, snapshots, remote-before-local).The presentation stream is capped at 30 seconds; if remote never lands, the client surfaces
remoteStatusUnavailableand the UI can keep listening viasourceControlStatusEventsfor a late remote result.FeatureSourceControlViewsplits load vs reload, uses generation guards against stale streams/actions, separates action alerts from inline load errors, and recovers status after failed actions.Focused tests cover the accumulator, stream completion rules, and action progress ordering.
Reviewed by Cursor Bugbot for commit ede5862. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Show cached source control status with bounded streaming API in Swift iOS app
FeatureClient.sourceControlStatuses(threadID:)returning anAsyncThrowingStreamthat yields cached-local-first status, then remote status as it resolvesNativeSourceControlStatusAccumulatorto fold VCS status events intoFeatureSourceControlStatusvalues, tracking local/remote halves and resetting remote on branch changessourceControlStatusStreamTimeoutSeconds) that finishes the stream withNativeFeatureClientError.remoteStatusUnavailableif remote data never arrivesFeatureSourceControlViewto stream status, show inline pending/unavailable remote states, use a dedicated action error alert, and trigger uncached refresh on pull-to-refreshFeatureSourceControlStatus.isRemoteKnownis falsesourceControlStatusdefault implementation inFeatureClientget a one-element stream; conformers not overridingsourceControlStatuses(threadID:)will not benefit from cached-local-first deliveryMacroscope summarized ede5862.