Skip to content

fix(swift-ios): retain thread subscriptions across navigation - #10764

Open
saphid wants to merge 19 commits into
pingdotgg:t3code/rebuild-mobile-app-swiftfrom
saphid:pr/swiftui-navigation-lifetime-20260908
Open

saphid wants to merge 19 commits into
pingdotgg:t3code/rebuild-mobile-app-swiftfrom
saphid:pr/swiftui-navigation-lifetime-20260908

Conversation

@saphid

@saphid saphid commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Review this layer

GitHub shows this PR against t3code/rebuild-mobile-app-swift, so its diff includes every earlier PR in the chain. Review only this layer's change:

This layer's diff: +711 / −56 lines across 9 files; app code +241 / −55, the rest is tests and docs.

Child-view churn can release the selected thread transport while the workspace still owns its selection. Move that lifetime to workspace selection, preserve it across navigation transitions, and reconcile foreground shell/detail authority when delivery is silently stale. Equal cursors do not suppress a changed authoritative HTTP body.

Verification: current-head CI passes, including contract fixtures and native tests. The final bootstrap/environment run passed 38 tests, covering environment switching, error visibility and archive hydration; separate focused checks cover passive-worker lifetime. Skipped checks are not claimed as executed proof.

Delivery: stacked. This branch contains #10758, #10759, #10761, #10762, #10763; merge the chain in order (#10758 → #10759 → #10761 → #10762 → #10763 → #10764 → #10765 → #10766 → #14021 → #10767). Each PR's own change is its top commit(s).

Base: Theo’s t3code/rebuild-mobile-app-swift, 157476f1fb.

Navigation verification: Baseline e163f9cee and candidate a361b2212 use actual WorkspaceView with a synthetic selected thread and a fixture full-screen cover. Both capture tests passed: no client release during cover/return, one release after the production Back action. This is ordinary navigation regression proof; it does not reproduce the race. Focused native tests cover presentation replacement, held-load cancellation, refresh ownership and compact/regular selection.

Before video · Clean candidate video · Annotated candidate video · Evidence receipt

Static context only: the unchanged selected-thread presentation is shown below. The lifetime race is established by the focused native ownership tests, not by a visible animation difference.

Selected thread context; not proof of the lifetime race

Capture currency: checked against 5b93f19c8. WorkspaceView and ThreadDetailView are unchanged since a361b2212; later edits affect environment hydration/error handling and tests, which this synthetic navigation fixture does not exercise.

Independent cross-provider review was unavailable: claude auth status returned exit 1 with loggedIn: false. No Claude review or maintainer approval is claimed.

Refresh onto 157476f1fb (2026-09-27)

Rebased with the thread-sync chain onto the current t3code/rebuild-mobile-app-swift (157476f1fb). Head a082dccec6.

  • Focused tests at this layer, measured before the review fixes below (NativeMultiEnvironmentTests, NativeThreadCatchUpTests, NativeRetryIdentityTests, FeatureRootModelTests, iOS Simulator, per-test time limits): 111 passed.
  • Full T3CodeTests at the chain top (68d92c2752): 343 XCTest cases passed (1 skipped). Swift Testing ran 892 tests; the only 2 failures are UsageModelsTests currency formatting, which fail identically on unmodified 157476f1fb.
  • Independent review: devin -p --sandbox --permission-mode smart --model swe-2-max over the whole chain diff, exit 0. No blocker, high or medium findings. A dead-state finding was fixed. A report that Stop state outlives a vanished thread was not changed, because stopSurvivesCloseReopenAndMissingReconnectSeed requires that behaviour.

Review fixes (rounds 2–5)

  • Reopened thread showed a busy … send button. This layer's navigation change lets the compact split view report a reopened thread view as disappeared right after it appears, while it stays on screen. That cancelled the draft restore, so the composer stayed busy until relaunch. The restore now runs outside the view task's lifetime. Found from an on-device activity log and bisected to this layer (upstream 157476f1fb and fix(swift-ios): reconcile silent selected-thread streams #10763 do not reproduce it). In the iOS 27 Simulator, before the fix opens 2–5 of a thread showed …; after it, every reopen restored the draft and showed the send arrow. Also confirmed on a physical iPhone. No unit test: the spurious disappear comes from SwiftUI's split-view presentation, which the unit harness does not reproduce.
  • Pairing rejection now also stops the quiet shell reconciliation loop, which kept issuing rejected shell reads every 30 s.
  • A passive peer whose socket stayed open kept its 30 s quiet shell reads after a 401 and showed as connected. The rejected credential is now handled before the live-stream shortcut and switches to the long back-off. The rejection is shared with the peer's stream: stream updates no longer mark it connected, a closed stream no longer turns Needs pairing into Reconnecting (which hid Pair again), and a new stream snapshot no longer wakes the HTTP loop before the back-off. Test: livePeerRejectedCredentialStopsReconciling, extended to close and reseed the stream after rejection. Removing either guard of the final fix makes it fail.

Independent review

GPT-6 Astra (codex exec -m gpt-6-astra -c model_reasoning_effort="xhigh" --sandbox read-only) reviewed this layer in rounds 1–4, then checked the round-5 fix on its own. Earlier rounds found the defects fixed above. The final check returned ready, with one minor, non-blocking note.

Follow-ups (minor, not blocking)

  • The wake half of livePeerRejectedCredentialStopsReconciling checks the shell read count after a later stream update is published. That ordering caught the regression in the negative check, but it is not a strict synchronisation point; an explicit worker drain would make it deterministic.
  • A rewind transcript that lands between the transcript acknowledging a send and the outbox completion lets the completed message be retained again until the next transcript (Macroscope; narrow ordering).

Model and harness: GPT-6 / Codex. Rebase onto 157476f1fb: Claude Opus 5.5 / Claude Code.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XXL 1,000+ changed lines (additions + deletions). labels Sep 8, 2026
@saphid
saphid marked this pull request as ready for review September 8, 2026 23:05
Comment thread apps/swift-ios/App/NativeFeatureClient.swift
Comment thread apps/swift-ios/App/NativeFeatureClient.swift
@macroscopeapp

macroscopeapp Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR is a large production-path concurrency and navigation refactor spanning shell streams, environment refresh workers, selected-thread detail reconciliation, message retention, and SwiftUI presentation ownership. Its broad runtime impact and unresolved High/Medium state-consistency findings require human review.

Not approved because:

  • 2 blocking correctness issues found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@saphid

saphid commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@t3dotgg Could you review this SwiftUI reliability/performance fix? It will retain selected-thread subscriptions across navigation and child-view churn. It does not change the visible interface. This is a dependent PR; prerequisites: #10763. Its diff includes those prerequisites and needs rebasing after they land.

A detached accepted-command refresh could finish after deleteThread and
publish the deleted thread again. Cancel it once the server accepts the
delete.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@saphid
saphid force-pushed the pr/swiftui-navigation-lifetime-20260908 branch from fad04ba to 03a4322 Compare September 27, 2026 00:56
let selectedClient = try await runtime.activeClient()
guard bootstrapID == foregroundBootstrapID, generation == environmentGeneration,
selectedClient === newClient, selectedClient?.environment == environment else { return false }
cancelAggregateRefresh()
if activeEnvironment?.id == environment.id, client === newClient {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High App/NativeFeatureClient.swift:1198

The same-client fast path returns with the previous shell epoch intact, so a reconnect with a restarted sequence causes acceptActiveShell to reject the new snapshot and leaves stale projects and threads visible indefinitely. Reset activeShellEpochHasSnapshot (and update activeShellConnectionID) before returning from this path.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/swift-ios/App/NativeFeatureClient.swift around line 1198:

The same-client fast path returns with the previous shell epoch intact, so a reconnect with a restarted sequence causes `acceptActiveShell` to reject the new snapshot and leaves stale projects and threads visible indefinitely. Reset `activeShellEpochHasSnapshot` (and update `activeShellConnectionID`) before returning from this path.

github-actions Bot and others added 3 commits September 27, 2026 18:36
…ng older state

A detail read that started before a newer send was accepted could publish
after it and hide the newer message until the next read. Skip publishing a
superseded read (its refresh loop reads again), and stop a cancelled shell
refresh from writing a shell read before the change.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ives

A Stop accepted during a send's detail read discarded that read without
requesting another, and an older detail could replace the transcript before
the accepted message reached it. Re-read detail after a discarded read, and
keep delivered messages until a server transcript includes them.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Do not retain a delivered message that a server transcript already
confirmed, keep only its display copy, and drop retained messages when
their thread details are removed or cleared.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@saphid
saphid force-pushed the pr/swiftui-navigation-lifetime-20260908 branch from 03a4322 to 48355be Compare September 27, 2026 20:29
Evicting a cached thread detail forgot its accepted messages, so a stale
reopen hid them. A restored outbox or a new task's local detail was also
recorded as the server transcript, so its accepted prompt was treated as
confirmed and disappeared on the next older read. Only server transcripts
now confirm delivery, and eviction keeps retained messages.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@saphid
saphid force-pushed the pr/swiftui-navigation-lifetime-20260908 branch from 48355be to d72efc4 Compare September 27, 2026 21:14
@@ -1823,6 +1921,12 @@ public final class FeatureRootModel {
}
pendingCompletionSubmissionIDs.remove(submission.id)
pendingSubmissionsByID.removeValue(forKey: submission.id)
// A transcript that already includes it confirmed delivery; do not retain it.
if serverMessageIDs[submission.threadID]?.contains(submission.identity.messageID) != true {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Root/FeatureRootModel.swift:1925

completeQueuedSubmission re-adds a delivered message to deliveredAwaitingDetail after an authoritative transcript has removed it, so later detail loads continue displaying a message that the server no longer contains. The check at line 1925 reads mutable serverMessageIDs only after await outboxStore.remove(id:); capture the transcript confirmation when completion is scheduled and carry that snapshot through the async completion/retry instead of consulting the later state.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/swift-ios/Features/Root/FeatureRootModel.swift around line 1925:

`completeQueuedSubmission` re-adds a delivered message to `deliveredAwaitingDetail` after an authoritative transcript has removed it, so later detail loads continue displaying a message that the server no longer contains. The check at line 1925 reads mutable `serverMessageIDs` only after `await outboxStore.remove(id:)`; capture the transcript confirmation when completion is scheduled and carry that snapshot through the async completion/retry instead of consulting the later state.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged as a narrow edge case: it needs a rewind transcript to land between the acknowledging transcript and the outbox completion. Listed as a follow-up in the PR body rather than changed here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

github-actions Bot and others added 4 commits September 28, 2026 07:26
A newer send can be delivered while an older one waits to retry. Delivered
copies were placed before every queued copy, which reversed them. Merge
both by send time.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A transient environment catalog read failure ended the active client's
configuration subscription, so later provider and settings updates were
ignored. Skip that publish instead.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e probe

A passive peer's catalogue probe started at bootstrap could finish after
the user saved a preference there and replace it with the older value.
Discard the probe when a config already arrived.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@saphid
saphid force-pushed the pr/swiftui-navigation-lifetime-20260908 branch from d72efc4 to 3fdd1fa Compare September 27, 2026 22:06
github-actions Bot and others added 8 commits September 28, 2026 08:23
A peer whose credential was rejected before its catalogue loaded kept
opening a probe client every 20 seconds. The probe's own error cannot show
the rejection (the socket ticket 401 surfaces as a timeout), so stop once
the peer's shell read has marked it as needing pairing; re-pairing starts
a new worker.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ismissed early

Reopening a thread in the compact split view can report the new thread view
as disappeared right after it appears while it stays on screen. That
cancelled the draft restore, so the composer showed a busy send button
until relaunch. Restore outside the view task's lifetime.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…cted

Pairing rejection cancelled the polling tasks but left the reconciliation
loop issuing rejected shell reads every 30 seconds.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A passive peer whose socket stayed open kept its 30-second quiet shell
reads after a 401, and showed as connected. Handle the rejected credential
before the live-stream shortcut and switch to the long back-off.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The open socket's updates marked a rejected peer connected again, and stream
repairs woke its HTTP loop before the back-off. Keep the rejection in the
peer's shared state.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@saphid
saphid force-pushed the pr/swiftui-navigation-lifetime-20260908 branch from 3fdd1fa to a082dcc Compare September 27, 2026 22:27
@saphid

saphid commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

@t3dotgg this is ready for your review and merge.

Merge order: stacked chain, merge in order: #10758 → #10759 → #10761 → #10762 → #10763 → #10764 → #10765 → #10766 → #10767. #12655 (composer focus freeze) is independent and can merge at any time.

Evidence at head a082dccec6:

  • CI: all checks pass on this head.
  • Full T3CodeTests at the chain top (70258b6a6b, iOS Simulator): 343 XCTest cases passed (1 skipped). Swift Testing ran 892 tests; the only 2 failures are UsageModelsTests currency formatting, which fail identically on unmodified 157476f1fb.
  • Each review fix has a focused regression test that fails without the fix, except where the description says why not.
  • Also fixes a reopen bug this layer introduced (busy … send button after reopening a thread). It was found from an on-device log, bisected to this layer, and proven fixed in the Simulator.
  • An earlier build of this chain (before the review rounds) fixed the stuck-send behaviour on a physical iPhone, as confirmed by the user. The current heads have not been re-verified on a device.

Independent review: GPT-6 Astra xhigh (read-only) reviewed this layer in rounds 1–4 and then checked the round-5 fix on its own: ready, with one minor test-determinism note listed as a follow-up.

Macroscope findings on the latest heads have been verified against the source: real ones are fixed, and the rest have a written reason and are listed under Follow-ups in the description.

This branch has not been deployed

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

Labels

size:XXL 1,000+ 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.

1 participant