fix(mobile): Harden iOS reconnect, crash capture, and chat layout on main#3910
fix(mobile): Harden iOS reconnect, crash capture, and chat layout on main#3910mwolson wants to merge 16 commits into
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size:XXL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Medium
t3code/patches/@legendapp__list@3.2.0.patch
Line 951 in 366f0e0
The onScrollBeginDrag wrapper is created inside a useMemo(..., []) that captures the initial onScrollBeginDrag prop. If a parent passes a new callback after mount, drag events continue invoking the stale closure — the prop update is silently ignored. Store the latest callback in a ref or include onScrollBeginDrag in the memo dependencies.
🤖 Copy this AI Prompt to have your agent fix this:
In file @patches/@legendapp__list@3.2.0.patch around line 951:
The `onScrollBeginDrag` wrapper is created inside a `useMemo(..., [])` that captures the initial `onScrollBeginDrag` prop. If a parent passes a new callback after mount, drag events continue invoking the stale closure — the prop update is silently ignored. Store the latest callback in a ref or include `onScrollBeginDrag` in the memo dependencies.
Co-authored-by: codex <codex@users.noreply.github.com>
juliusmarminge
force-pushed
the
ios-fixes-main
branch
from
366f0e0 to
bfd9ca3
Compare
|
|
||
| let cancelled = false; | ||
| let timer: ReturnType<typeof setTimeout> | null = null; | ||
| const scheduleRetry = () => { |
There was a problem hiding this comment.
🟡 Medium threads/ThreadRouteScreen.tsx:199
The stall error appears after ~40s, not the ~20s implied by THREAD_DETAIL_STALL_ERROR_DELAY_MS. After two failed retries (8s + 12s), scheduleRetry() schedules the error after an additional 20s, so a permanently empty detail doesn't show detailLoadError until roughly 40 seconds. When the retry limit is reached, set the stalled state immediately instead of scheduling another timeout.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadRouteScreen.tsx around line 199:
The stall error appears after ~40s, not the ~20s implied by `THREAD_DETAIL_STALL_ERROR_DELAY_MS`. After two failed retries (8s + 12s), `scheduleRetry()` schedules the error after an additional 20s, so a permanently empty detail doesn't show `detailLoadError` until roughly 40 seconds. When the retry limit is reached, set the stalled state immediately instead of scheduling another timeout.
| void props.listRef.current?.scrollToOffset({ | ||
| animated: false, | ||
| offset: -anchorTopInset, | ||
| }); |
There was a problem hiding this comment.
🟡 Medium threads/ThreadFeed.tsx:1439
When usesNativeAutomaticInsets is false, the underflow correction scrolls to -anchorTopInset, but that same inset is already rendered as the ListHeaderComponent spacer. This double-counts the header inset and positions a short conversation at a negative overscroll offset instead of resting at offset 0. The negative offset should only be used for native automatic-inset layouts where the header inset lives in UIKit's adjustedContentInset and is absent from contentHeight.
| void props.listRef.current?.scrollToOffset({ | |
| animated: false, | |
| offset: -anchorTopInset, | |
| }); | |
| void props.listRef.current?.scrollToOffset({ | |
| animated: false, | |
| offset: usesNativeAutomaticInsets ? -anchorTopInset : 0, | |
| }); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadFeed.tsx around lines 1439-1442:
When `usesNativeAutomaticInsets` is false, the underflow correction scrolls to `-anchorTopInset`, but that same inset is already rendered as the `ListHeaderComponent` spacer. This double-counts the header inset and positions a short conversation at a negative overscroll offset instead of resting at offset `0`. The negative offset should only be used for native automatic-inset layouts where the header inset lives in UIKit's `adjustedContentInset` and is absent from `contentHeight`.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 6 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
Capture RCTFatal message and stack to Documents via a native handler so Release Hermes aborts leave last-crash.json without a device console. Harden the JS ErrorUtils path with a minimal write-first record, durable fsync writes, breadcrumb max-wait flush, and an Expo AppDelegate plugin. Skip NativeStackScreenOptions setOptions when content is unchanged so Thread catch-up re-renders do not loop through PreventRemoveProvider and hit maximum update depth.
| return ""; | ||
| } | ||
| const record = options as Record<string, unknown>; | ||
| return stableJsonStringify({ |
There was a problem hiding this comment.
🟡 Medium native/nativeStackOptionsSignature.ts:12
buildNativeStackOptionsSignature omits fields like headerShown, headerTransparent, presentation, and gestureEnabled. When a caller changes one of these omitted options while the included fields stay the same, the signature is identical, so the skip logic suppresses setOptions and the screen keeps stale options. Consider including all NativeStackNavigationOptions fields in the signature (or documenting why only this subset is tracked).
Also found in 1 other location(s)
apps/mobile/src/native/StackHeader.tsx:114
The signature cache is not scoped to
navigation: when the navigation object/context changes whileprops.optionshas the same signature, this effect reruns but returns at the signature check before calling the new navigator'ssetOptions. The replacement navigator therefore never receives this component's screen options. Reset/cache the signature per navigation object or perform the comparison only after accounting fornavigation.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/native/nativeStackOptionsSignature.ts around line 12:
`buildNativeStackOptionsSignature` omits fields like `headerShown`, `headerTransparent`, `presentation`, and `gestureEnabled`. When a caller changes one of these omitted options while the included fields stay the same, the signature is identical, so the skip logic suppresses `setOptions` and the screen keeps stale options. Consider including all `NativeStackNavigationOptions` fields in the signature (or documenting why only this subset is tracked).
Also found in 1 other location(s):
- apps/mobile/src/native/StackHeader.tsx:114 -- The signature cache is not scoped to `navigation`: when the navigation object/context changes while `props.options` has the same signature, this effect reruns but returns at the signature check before calling the new navigator's `setOptions`. The replacement navigator therefore never receives this component's screen options. Reset/cache the signature per navigation object or perform the comparison only after accounting for `navigation`.
| </View> | ||
| ) : null} | ||
| {props.contentPresentation.kind === "loading" ? ( | ||
| <View pointerEvents="none" className="bg-screen" style={StyleSheet.absoluteFill}> |
There was a problem hiding this comment.
🟡 Medium threads/ThreadFeed.tsx:1890
The loading overlay sets pointerEvents="none", so taps pass through to the still-mounted feed underneath while only the "Loading messages" placeholder is visible. Users can unknowingly activate hidden links, image viewers, or disclosure controls at the tapped coordinates. Consider removing pointerEvents="none" so the overlay intercepts input and blocks interaction with the hidden feed.
| <View pointerEvents="none" className="bg-screen" style={StyleSheet.absoluteFill}> | |
| <View className="bg-screen" style={StyleSheet.absoluteFill}> |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadFeed.tsx around line 1890:
The loading overlay sets `pointerEvents="none"`, so taps pass through to the still-mounted feed underneath while only the "Loading messages" placeholder is visible. Users can unknowingly activate hidden links, image viewers, or disclosure controls at the tapped coordinates. Consider removing `pointerEvents="none"` so the overlay intercepts input and blocks interaction with the hidden feed.
CI Mobile Native Static Analysis failed on a multi-line if where the opening brace was on the following line.
Simulator layout evidence (Release Hermes)1. Transparent header: first message not cut offScrolled to the top of a screenshot thread. The first user bubble (prompt + attached screenshot) is fully below the nav chrome; nothing is clipped under the transparent header.
2. Floating composer: content stays above the inputShort screenshot thread after send. Message body and reply sit above the floating composer with clear gap; content is not trapped under the input bar.
All captures: iPhone Simulator, Release + Hermes, |
…x-turn-mapping Squash-merge the full Julian orchestrator v2 stack onto main PRs (ios-fixes-main pingdotgg#3910 and secret-store pingdotgg#2916). Refresh this commit when upstream/t3code/codex-turn-mapping moves; do not cherry-pick its commits individually.
Warm-cache afterSequence resume can skip a dropped thread.archived delta after later events advance snapshotSequence, leaving archived threads on the home list. Always HTTP-heal (or force a full socket snapshot) before resuming live shell deltas, and reject stale full snapshots from the stream.
When HTTP shell heal fails, the full socket snapshot is the membership source of truth. Accept that first snapshot even if its sequence is behind the warm disk cache so ghost active threads cannot stick.
mwolson
changed the title
A prior session can arm acceptNextSocketSnapshotAuthoritatively when HTTP heal fails and then disconnect before the socket snapshot arrives. Clear the flag on a successful HTTP heal so a later reconnect does not treat stale enrichment snapshots as authoritative.
waitForJsonLogMatch only yieldNow'd between attempts, so under busy CI it could return before the ACP mock agent wrote session/cancel. Use a real wall-clock delay (not TestClock sleep) and fail explicitly on timeout.
CI Check treats setTimeout as an Effect lint error. Keep wall-clock delays via a diagnostics-scoped helper, fail with a tagged timeout, and avoid TestClock-bound Effect.sleep so ACP mock log polls still work under load.
Move the header options memo before the missing environmentId, threadId, or selectedThread guards so hook order stays stable if those become null on a re-render of a mounted ThreadRouteContent instance.
|
I hit the same persistent “Syncing messages…” state on a physical iPhone with a warm cached thread and no new events after reconnect. Your reconnect work is directly relevant. In our trace, the server had attached the catch-up + live subscription successfully, but emitted zero frames because the cache was current, so the client had no positive completion signal. We validated a small protocol-level completion marker emitted once the live subscription is attached; it lets a cached thread transition to live without retransmitting its full body. Happy to contribute that as a focused follow-up if it remains useful after this PR lands. |
Hi, if you have time I'd be interested to see a PR with this one as a base branch, which includes the steps to repro it consistently. I haven't come across it yet organically myself. If this PR gets merged first, GitHub should automatically change base to master. |
Will do Mike. Let me get back to you shortly. |
## Summary - Add a synchronized stream item after a resumed thread subscription has attached its live buffer. - Keep a warm cached thread synchronizing through reconnect until that item arrives. ## Notes Squash-merge of twistedgrim's PR onto ios-fixes-main. Follow-up gate commit will land separately with capability negotiation and marker-after-catch-up ordering.
Emit the completion marker only on subscribeThread when clients opt in, after catch-up replay, and keep warm threads synchronizing until it arrives. Advertise support via ServerConfig so old servers still use the ready fallback and old clients never see an unknown stream kind.
Do not paint a warm disk shell snapshot before the HTTP or socket heal. Stale active membership was flashing a larger Home list, then shrinking when the authoritative heal arrived. Disk remains offline/heal fallback.
|
@twistedgrim Thanks for filing this and for the PR. I squash-merged your branch onto Gate commit (your thread-sync idea, hardened)fix(orchestration): Gate thread resume sync marker after catch-up Your diagnosis still holds: a warm resume with empty catch-up can attach the live subscription and emit zero frames, so the client has no positive "caught up" signal. On top of your marker, this commit:
I also hit a real bug in the original tip: the marker was only wired on the shell resume path while the contracts/client/test targeted thread resume (the server test timed out with empty replay). That is fixed here. Shell-hold commit (separate Home UX; not part of your PR)fix(client-runtime): Hold shell list until server heal on reconnect This is unrelated to the thread-detail marker. While retesting on device I saw Home briefly balloon with many more threads, then shrink after the HTTP shell heal. That came from painting a stale warm disk shell snapshot (often inflated active membership after a dropped archive delta) before the authoritative heal. This commit:
Kept as its own commit so your protocol work stays reviewable on its own. Happy to take further notes if you still see sticky Syncing on a build that includes both the squash and the gate commit. |
Keep Home empty while session is still None during online connect setup; only apply disk fallback from true disconnect. Persist markerMode so thread resume re-arms awaitingCompletion after ordinary reconnects.
…Android Anchor the chat scroll position to the end of the scroll range when the keyboard opens with the list resting at the bottom, reading the live composer padding per frame. This survives the event-ordering race where composer expansion lands before the keyboard's onStart (quick tap after opening a thread), which zeroed out the previous delta-based compensation and left the last message hidden behind the input. Also re-assert the settled scroll target after the keyboard animation ends and guard that correction with shouldShiftContent.
| } else { | ||
| // Android: keyboard closing — re-capture scroll position | ||
| + extraPaddingOnStart.value = extraContentPadding.value; | ||
| + anchorToEnd.value = |
There was a problem hiding this comment.
🟡 Medium patches/react-native-keyboard-controller@1.21.13.patch:1125
On Android keyboard-close, anchorToEnd is recomputed as scroll.value + layout.value.height - size.value.height >= blankSpace.value - 1, but while the keyboard is open size.value.height still includes the keyboard padding, so this end-of-range test compares against the open scroll range, not the closed one. A user who scrolled away from the newest message while the keyboard was open can still satisfy it. The close path then forces scrollTo to the absolute end on every frame, snapping the conversation to the bottom and discarding the user's scroll position. The close-path check should account for the keyboard height (or be captured before close begins) so it reflects the post-close scroll range.
🤖 Copy this AI Prompt to have your agent fix this:
In file @patches/react-native-keyboard-controller@1.21.13.patch around line 1125:
On Android keyboard-close, `anchorToEnd` is recomputed as `scroll.value + layout.value.height - size.value.height >= blankSpace.value - 1`, but while the keyboard is open `size.value.height` still includes the keyboard padding, so this end-of-range test compares against the *open* scroll range, not the closed one. A user who scrolled away from the newest message while the keyboard was open can still satisfy it. The close path then forces `scrollTo` to the absolute end on every frame, snapping the conversation to the bottom and discarding the user's scroll position. The close-path check should account for the keyboard height (or be captured before close begins) so it reflects the post-close scroll range.
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 924ad10. Configure here.
| + } | ||
| + | ||
| + return; | ||
| + } |
There was a problem hiding this comment.
anchorToEnd ignores whenAtEnd sentinel
Medium Severity
In the non‑iOS useChatKeyboard path, onStart still sets offsetBeforeScroll to -1 when keyboardLiftBehavior is whenAtEnd and the list is not at the end, but the new onMove branch scrolls to the end whenever anchorToEnd is true and never checks that sentinel. Short or underflowing threads can satisfy the blankSpace‑based anchorToEnd test while the user is at the top, so focusing the composer can yank the feed to the bottom on Android.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 924ad10. Configure here.
2 participants
Summary
main.PreventRemoveProvidervia unstable native-stack options updates.snapshotSequence.Problem and Fix
startTurnpayloads.ErrorUtilslogger often lost the race withRCTFatalabort and left empty Documents crash logs.T3NativeControls.writeSyncText), unhandled-rejection capture, nav/outbox/stop breadcrumbs with max-wait flush, and a nativeRCTSetFatalHandlerpath (T3CrashLog+ AppDelegate plugin) that writeslast-crash.json/crash-native-*.jsonwith the real JS message and stack.navigation.setOptionsre-enteredPreventRemoveProvideruntil React hit maximum update depth.setOptions, keep stable header-item factories via refs, and memoize Thread screen options.afterSequenceresume could skip a dropped archive upsert after later shell events advancedsnapshotSequence, so archived threads stayed on Home until a full resync.{ kind: "synchronized" }on thread resume after catch-up: server advertisesServerConfig.threadResumeCompletionMarker, client requestsrequestCompletionMarkeronly when advertised, and status stays synchronizing until the marker (legacy servers keep warmsetReadypromotion). Seeded from contributor work in mwolson#2; emission corrected tosubscribeThread.onStart, so delta-based scroll compensation zeroed out and the last messages stayed behind the input.react-native-keyboard-controllerchat keyboard path: when the list is anchored at the end, target the live end of the scroll range (including current composer padding) each frame; re-assert the settled target after the animation ends; keep iOS platform files unchanged for this race.Defensive Fixes
last-breadcrumbs.jsonnever flushed before a kill.{ kind: "synchronized" }without opt-in would hard-fail decode on older clients; new clients against old servers would wait forever if the warm ready path were removed.ServerConfig.threadResumeCompletionMarker; request the marker only when advertised.awaitingCompletionon disconnect, thensetReadycould promote to live before the next marker arrived (review follow-up).markerModefor the life of the thread state; re-armawaitingCompletionon everysetSynchronizingand on stream errors while marker mode is active.session: Nonewhile the lease opens. Treating that as offline still painted disk into Home before heal (review follow-up on the shell hold path).setDisconnected) or when a live session fails HTTP heal and needs a provisional list before the socket snapshot.UI Changes
This affects thread loading feedback, screenshot messages and their viewer, transparent-header scrolling, the floating composer (including Android keyboard open), and Home membership during reconnect (no stale disk flash before heal).
Focused Simulator layout evidence (two after-state shots only): #3910 (comment)
Validation
vp check: passvp run typecheck: passlast-crash.jsonwithsource: "rct-fatal"and the JS exception message (verified for maximum update depth on Thread)PreventRemoveProviderabortopening_bracefix forT3CrashLog(CI Mobile Native Static Analysis)shell-sync): passthreads-sync+ server empty resume / no-request omit): passthreads-sync): passshell-sync): passcom.mwolson.t3code.main.dev)Remaining Work
Keep exercising heavy-thread catch-up and streaming on a physical iPhone for residual performance issues (memory / large projections are separate from the options-loop fatal).Done: large active CTM thread finished catching up on device after the stack-options fix without thePreventRemoveProviderabort (see Validation).Add focused Simulator layout screenshots.Done in fix(mobile): Harden iOS reconnect, crash capture, and chat layout on main #3910 (comment)afterSequencefromlastSequenceon each resubscribe attempt (optional follow-up)Checklist
Note
High Risk
Touches reconnect shell/thread sync, WS protocol markers, and native fatal hooks—areas where regressions affect Home membership, thread catch-up, or crash capture on Release iOS.
Overview
Hardens iOS/mobile reconnect and sync, Release crash diagnosis, and thread chat layout so cached state recovers cleanly and Release Hermes fatals leave readable on-disk records.
Client-runtime & server: Shell sync no longer paints stale disk membership on reconnect; it HTTP-heals (or forces a full socket snapshot) before resuming
afterSequence, rejects regressive full snapshots, and only falls back to disk when truly offline or heal fails. Thread detail can request an opt-in{ kind: "synchronized" }marker after warm resume (ServerConfig.threadResumeCompletionMarker+requestCompletionMarker) so empty catch-up does not stick on syncing. SharedcauseFailureMessageimproves error text; thread queries expose refresh and preserve data on refresh failure. Server WS emits the completion marker only when requested.Mobile app: Early crash logger (JS
ErrorUtils, breadcrumbs with debounced/max-wait flush, nav/outbox breadcrumbs) plus nativeT3CrashLog/ AppDelegateRCTSetFatalHandlerand fsyncwriteSyncText. Native stack options skip redundantsetOptionsvia structural signatures and stable header factories (avoidsPreventRemoveProviderupdate-depth loops). Thread feed fixes composer insets, short-content scroll, loading overlay, screenshot press targets, andfillWidthmarkdown; native shadow node rebuilds content on layout instead of stale measure cache. Thread route adds stalled-detail retries; composer images strip client fields viatoUploadChatImageAttachments.Tests: Expanded shell/thread-sync, server subscribeThread, Cursor adapter wall-clock waits, and mobile unit tests for breadcrumbs, crash records, and stack signatures.
Reviewed by Cursor Bugbot for commit 924ad10. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Harden iOS reconnect handling, crash capture, and chat feed layout
installCrashLog) that intercepts global JS errors and unhandled rejections, persisting minimal and full crash records (including breadcrumbs) via a native sync write or Expo FS fallback; an Expo config plugin modifiesAppDelegate.swiftto also intercept native RN fatal handlerscrash-logs/last-breadcrumbs.json, with navigation state changes and outbox dispatches recording breadcrumbssynchronizedstream marker protocol: the server emits{ kind: 'synchronized' }after catch-up whenrequestCompletionMarkeris set, and the client holds thread state assynchronizinguntil the marker arrives, preventing false-live status after reconnectsynchronizingwithout painting disk state, attempt HTTP heal first, and only use cached disk state as a provisional fallback when offline or when HTTP heal fails; rejects stale full socket snapshots unless marked authoritativeThreadFeedchat layout by measuring content height and correcting underflow via auto-scroll, adding aTHREAD_FEED_COMPOSER_GAPabove the floating composer, and showing a loading overlay during hydrationStackHeader.NativeStackScreenOptionsto only callnavigation.setOptionswhen the structural signature of options changes, using stable-identity wrappers for header item factoriesT3MarkdownTextShadowNodeto build content fresh from children on each layout rather than relying on cached mutable members, preventing stale or dropped textsynchronizingstate briefly before the HTTP heal completes where they previously saw stale cached content immediatelyMacroscope summarized 924ad10.