Repository navigation
Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a contained SwiftUI bug fix that keeps existing review and diff content visible when refreshes fail, adds retry feedback, and prevents stale async results from replacing newer state. It introduces no schema, security-sensitive, deployment, product-default, or static-analysis changes. You can add or adjust custom eligibility rules. Learn more. |
|
@t3dotgg Alex has approved this PR for your review. Review content stays visible with Retry after refresh failure. The current description contains the verification evidence. |
|
Note This comment is posted by Julius' dot The screenshots and recordings demonstrate review-list refresh and recovery, but this PR also changes FeatureDiffView when loading file contents fails. The new empty-diff error and retained-diff Retry states have no before/after evidence or demonstrated recovery; the added native test checks only the generation counter. Closing under the verification rule. Add before/after screenshots for those diff states and a short recording of failed hydration followed by Retry, then request reconsideration. |
|
Thanks for the review. I've added the missing evidence for
The description now explains why this fits the small obvious-bug exception and how the list and diff paths are the same defect. The review-list evidence is still the earlier capture from |
Continued in #14684. GitHub rejects the author's reopen of this closed PR (HTTP 422), so the corrected change is there. The description below describes that PR's head
badb34dfe0, not this PR's original headf2f427606, which is unchanged.The SwiftUI Review screen hides load failures. When a review refresh or a file's full-diff load fails, the user never sees the error and has no way to retry:
try?, so the error is swallowed. A file with no inline patch shows “Diff unavailable — No line-level preview is available.”, which is wrong. A file with inline patch lines silently keeps the partial patch.This change keeps whatever content is already on screen and reports the failure with Retry. In the review list, an inline row appears above the loaded review. In the diff, “Couldn’t load this diff” with Try again appears when there are no patch lines; otherwise the patch stays and a Retry row appears above it. Superseded and cancelled loads are ignored, so a stale or cancelled request cannot replace newer content or show a false error.
Why this is one small, obvious-bug fix. No issue or discussion is linked; this relies on the small obvious-bug exception. The defect is objective: thrown errors are discarded and, for diffs, replaced with a false message. The fix reports the error the screen already receives and retries the same request. It reuses existing patterns: the review screen already shows its error before the first load, and
ContentUnavailableViewis already used for empty diffs. It changes no defaults, contracts, server behavior or other clients. Both paths are the same failure on the same screen and share the new pieces (FeatureRefreshFailureRow,FeatureAsyncGeneration).FeatureDiffHydrationonly holds the diff's existing load state, so those rules have focused tests. IgnoringCancellationErrorkeeps the new error row from reporting a cancelled request as a failure. If maintainers count the list and diff paths as separate problems, the diff part splits cleanly: the diff hunks inFeatureReviewView.swiftplusFeatureDiffHydrationand its tests.SwiftUI mobile only. React Native mobile, web and desktop are separate clients and are not changed.
Diff failure and Retry (FeatureDiffView)
Reproduce: open Review, then open a changed file while its full-contents request fails.
Environment: iPhone 17 Pro Simulator, iOS 26.5, dark appearance. Captured on base
285d2ab80cvs candidate1993cb03b6, which is that base plus this PR's four commits.t3code/rebuild-mobile-app-swiftwas later rewritten. This head,badb34dfe0, replays the same four commits ontocf7902896bwith an identical diff.FeatureReviewView.swiftandFeatureToolModels.swiftare identical between the two bases.FeatureClient.swiftonly gains an unrelatedrestartAgentSessionrequirement, which has a default implementation.How it was driven: the real
FeatureReviewViewis hosted in aNavigationStack, and file rows are tapped with real Simulator taps to reachFeatureDiffView. The client is a fixture, not a live server. Its first full-contents load of each file fails withURLError(.networkConnectionLost)(“The network connection was lost.”) after 1.2 s; the next load succeeds. The host and fixture are a capture-only test file kept outside this PR and added uncommitted to both builds.These captures show the view's states and controls. They do not exercise live server or transport failures.
Recordings are real time, sampled to 12 fps.
File without inline patch lines
Still-state comparison of the failed load (3.5 s each):
Before (base): the failure is swallowed and the file looks like it has no preview.
After (candidate): the failure is shown, and Try again loads the full diff.
Before screenshot · After screenshot · After Try again · Before recording · After recording
File with inline patch lines
Still-state comparison of the failed load (3.5 s each):
Before (base): only the patch is shown, with no sign that the full file failed to load.
After (candidate): the patch stays, and Retry replaces it with the full-file diff. While the retry runs, the row hides and the patch stays visible; there is no separate progress indicator.
Before screenshot · After screenshot · After Retry · Before recording · After recording
Capture provenance. The fixture client logged one failed full-contents load per file on base. On the candidate it logged one failed and one successful load per file.
Review list refresh (FeatureReviewView)
This evidence is older. It was captured on the earlier head
f2f427606against the earlier base93ca26695, using a similar fixture (success, then a failed refresh, then a different title on Retry). It was not re-captured on1993cb03b6. This PR's review-list changes are the same as atf2f427606apart from ignoringCancellationError, which this flow never triggers. The new base also changed how review comments are sent, which this flow does not use.Review list recordings (f2f4276 / 93ca266)
Before screenshot · After screenshot · Baseline recording · Candidate recording
Verification
Focused tests on
1993cb03b6(same diff as this head), run on the iPhone 17 Pro Simulator (iOS 26.5):5 of 5 passed:
FeatureDiffHydrationTests(4 tests):FeatureAsyncGenerationTests(1 test): stale completions are rejected.The same run also included the uncommitted capture host.
Not checked:
FeaturePastedTextTestsandUsagePresentationTests. The base branch fails them identically (cf7902896b,9790cbbc38), and this PR does not touch them.Model and harness: GPT-6 / Codex wrote the original change. Claude Opus 5.5 / Claude Code via T3 Code did the re-port, diff hydration tests, cancellation handling and captures. Independent review: Codex GPT-6-Sol (high reasoning) via T3, in two rounds. The first round's cancellation finding was fixed. The second round, on the final diff, had no findings.
🤖 Generated with Claude Code