Skip to content

fix(swift-ios): retain review content during refresh failures - #10750

Closed
saphid wants to merge 2 commits into
pingdotgg:t3code/rebuild-mobile-app-swiftfrom
saphid:pr/swiftui-review-refresh-20260908
Closed

saphid wants to merge 2 commits into
pingdotgg:t3code/rebuild-mobile-app-swiftfrom
saphid:pr/swiftui-review-refresh-20260908

Conversation

@saphid

@saphid saphid commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

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 head f2f427606, 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:

  • Review list: after one successful load, a failed refresh is dropped silently, because the error is only rendered while no review has loaded yet.
  • File diff: the full-file load uses 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 ContentUnavailableView is 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). FeatureDiffHydration only holds the diff's existing load state, so those rules have focused tests. Ignoring CancellationError keeps 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 in FeatureReviewView.swift plus FeatureDiffHydration and 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 285d2ab80c vs candidate 1993cb03b6, which is that base plus this PR's four commits. t3code/rebuild-mobile-app-swift was later rewritten. This head, badb34dfe0, replays the same four commits onto cf7902896b with an identical diff. FeatureReviewView.swift and FeatureToolModels.swift are identical between the two bases. FeatureClient.swift only gains an unrelated restartAgentSession requirement, which has a default implementation.

How it was driven: the real FeatureReviewView is hosted in a NavigationStack, and file rows are tapped with real Simulator taps to reach FeatureDiffView. The client is a fixture, not a live server. Its first full-contents load of each file fails with URLError(.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):

Empty diff: before shows “Diff unavailable” with no error; after shows “Couldn’t load this diff” with the error and Try again

Before (base): the failure is swallowed and the file looks like it has no preview.

Before: opening Strings.swift, the load fails and “Diff unavailable” appears

After (candidate): the failure is shown, and Try again loads the full diff.

After: loading, “Couldn’t load this diff” with Try again, tap, loading, full 5-line 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):

Retained diff: before shows only patch lines with no error; after keeps the patch lines and shows the error with Retry

Detail: the Retry row above the retained patch

Before (base): only the patch is shown, with no sign that the full file failed to load.

Before: opening Palette.swift, the load fails and only the two patch lines remain

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.

After: patch lines with “The network connection was lost.” and Retry, tap, full 8-line file diff

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 f2f427606 against the earlier base 93ca26695, using a similar fixture (success, then a failed refresh, then a different title on Retry). It was not re-captured on 1993cb03b6. This PR's review-list changes are the same as at f2f427606 apart from ignoring CancellationError, which this flow never triggers. The new base also changed how review comments are sent, which this flow does not use.

Review refresh: hidden failure before; retained review and Retry after — still-state comparison

Review list recordings (f2f4276 / 93ca266)

Before: Review refresh: hidden failure before; retained review and Retry after

After: Review refresh: hidden failure before; retained review and Retry after

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):

xcodebuild test -project apps/swift-ios/T3Code.xcodeproj -scheme T3Code -configuration Debug \
  -destination 'platform=iOS Simulator,id=<udid>' -parallel-testing-enabled NO CODE_SIGNING_ALLOWED=NO \
  -only-testing:T3CodeTests/FeatureDiffHydrationTests -only-testing:T3CodeTests/FeatureAsyncGenerationTests

5 of 5 passed:

  • FeatureDiffHydrationTests (4 tests):
    • a failed load keeps the patch lines until Retry succeeds;
    • a failure with no patch lines reports the error;
    • a superseded attempt cannot overwrite a newer result;
    • missing full contents keep the patch, with no error.
  • FeatureAsyncGenerationTests (1 test): stale completions are rejected.

The same run also included the uncommitted capture host.

Not checked:

  • There is no committed UI automation for the view states. The target has no UI test target, so the Simulator captures above are the view-level evidence.
  • Live server and WebSocket failures were not reproduced.
  • Physical devices were not used.
  • The cancellation path was not exercised visually.
  • No native build was run locally on this head's new base. On this head, CI's "Contract fixtures and native tests" job builds it and passes all 5 tests added here. The job still fails on 3 issues in FeaturePastedTextTests and UsagePresentationTests. 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

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 8, 2026
@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Sep 8, 2026
@saphid
saphid marked this pull request as ready for review September 8, 2026 22:27
@macroscopeapp

macroscopeapp Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at f2f4276

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.

@saphid

saphid commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@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.

Copy link
Copy Markdown
Member

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.

@saphid

saphid commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I've added the missing evidence for FeatureDiffView and updated the description:

  • Before/after screenshots for both diff failure states, base 285d2ab80c vs candidate 1993cb03b6:
    • File with no inline patch: base shows “Diff unavailable”; the candidate shows “Couldn’t load this diff” with Try again.
    • File with patch lines: base silently keeps the partial patch; the candidate keeps it and shows a Retry row.
  • Real-time recordings on base and candidate of the failed load, then Try again or Retry loading the full diff. These were captured in an iPhone 17 Pro Simulator through the real Review screen, with a fixture client that fails the first full-contents load. They don't exercise live server failures.
  • Focused tests for the diff-loading rules: patch lines are kept on failure, an empty diff reports the error, a successful retry replaces the lines, and stale results are ignored. These replace the generation-counter-only coverage (5/5 passed on 1993cb03b6).

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 f2f427606 and is labeled that way. GitHub rejects my reopen of this PR (HTTP 422), and force-pushing its closed head would block a later reopen. So the corrected change is in #14684, linked here as CONTRIBUTING's reconsideration section allows. #14684's head badb34dfe0 has the same four commits replayed onto the rewritten t3code/rebuild-mobile-app-swift (cf7902896b), with a diff identical to the captured candidate. Cancelled review refreshes no longer show as errors. Could you reconsider it there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants