fix(mobile): Stabilize iOS v2 runtime paths, image attach ids, header, and markdown text#3814
fix(mobile): Stabilize iOS v2 runtime paths, image attach ids, header, and markdown text#3814mwolson wants to merge 1 commit 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
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if (routeConnectionState !== "connected") { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Medium threads/ThreadRouteScreen.tsx:182
The retry counter in detailRefreshAttemptsRef is never reset when the route drops out of the "connected" state. After the two scheduled refreshes are used once, a later disconnect/reconnect on the same thread leaves the counter at 2, so THREAD_DETAIL_STALL_RETRY_DELAYS_MS[attempts] is undefined and the effect never schedules another refresh. A thread detail that is still empty after reconnect stays stuck on the loading/unavailable path indefinitely.
The effect bails out early when routeConnectionState !== "connected", so the attempt counter is only deleted when detail arrives or the thread changes — not on disconnect. Consider resetting the counter whenever routeConnectionState leaves "connected".
| if (routeConnectionState !== "connected") { | |
| return; | |
| } | |
| if (routeConnectionState !== "connected") { | |
| detailRefreshAttemptsRef.current.delete(routeThreadKey); | |
| return; | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadRouteScreen.tsx around lines 182-184:
The retry counter in `detailRefreshAttemptsRef` is never reset when the route drops out of the `"connected"` state. After the two scheduled refreshes are used once, a later disconnect/reconnect on the same thread leaves the counter at 2, so `THREAD_DETAIL_STALL_RETRY_DELAYS_MS[attempts]` is `undefined` and the effect never schedules another refresh. A thread detail that is still empty after reconnect stays stuck on the loading/unavailable path indefinitely.
The effect bails out early when `routeConnectionState !== "connected"`, so the attempt counter is only deleted when detail arrives or the thread changes — not on disconnect. Consider resetting the counter whenever `routeConnectionState` leaves `"connected"`.
mwolson
force-pushed
the
ios-v2
branch
2 times, most recently
from
44d02f1 to
2bd64fe
Compare
| function formatThreadStateFailure(cause: Cause.Cause<unknown>): string { | ||
| const error = Cause.squash(cause); | ||
| return error instanceof Error && error.message.trim().length > 0 | ||
| ? error.message | ||
| : "Could not load conversation."; | ||
| } |
There was a problem hiding this comment.
🟡 Medium state/threads.ts:39
formatThreadStateFailure discards non-Error failure payloads. Cause.squash returns the original error value for Cause.fail, so a string error like Cause.fail("nope") yields "nope", but the error instanceof Error check falls through to the generic "Could not load conversation." message, hiding the real failure text. Consider converting non-Error values to a string (e.g. String(error)) before checking whether it is non-empty.
| function formatThreadStateFailure(cause: Cause.Cause<unknown>): string { | |
| const error = Cause.squash(cause); | |
| return error instanceof Error && error.message.trim().length > 0 | |
| ? error.message | |
| : "Could not load conversation."; | |
| } | |
| function formatThreadStateFailure(cause: Cause.Cause<unknown>): string { | |
| const error = Cause.squash(cause); | |
| const message = error instanceof Error ? error.message : typeof error === "string" ? error : String(error); | |
| return message.trim().length > 0 | |
| ? message | |
| : "Could not load conversation."; | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/state/threads.ts around lines 39-44:
`formatThreadStateFailure` discards non-`Error` failure payloads. `Cause.squash` returns the original error value for `Cause.fail`, so a string error like `Cause.fail("nope")` yields `"nope"`, but the `error instanceof Error` check falls through to the generic `"Could not load conversation."` message, hiding the real failure text. Consider converting non-`Error` values to a string (e.g. `String(error)`) before checking whether it is non-empty.
| return; | ||
| } | ||
|
|
||
| if (selectedThreadDetail !== null || selectedThreadDetailState.status === "deleted") { |
There was a problem hiding this comment.
🟡 Medium threads/ThreadRouteScreen.tsx:177
The retry effect at lines 168–222 fires refresh() even when the failed load already produced an error. When the detail is null but selectedThreadDetailState.error is populated (a real server-side fetch failure), the effect treats it the same as a stalled empty load and schedules two extra refresh() calls against the same failing request, producing duplicate network traffic and repeated error churn. Consider adding selectedThreadDetailState.error to the early-return guard alongside the null-detail and deleted-status checks so retries only fire for the intended stalled-empty case.
- if (selectedThreadDetail !== null || selectedThreadDetailState.status === "deleted") {
+ if (
+ selectedThreadDetail !== null ||
+ selectedThreadDetailState.error !== null ||
+ selectedThreadDetailState.status === "deleted"
+ ) {🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadRouteScreen.tsx around line 177:
The retry effect at lines 168–222 fires `refresh()` even when the failed load already produced an error. When the detail is `null` but `selectedThreadDetailState.error` is populated (a real server-side fetch failure), the effect treats it the same as a stalled empty load and schedules two extra `refresh()` calls against the same failing request, producing duplicate network traffic and repeated error churn. Consider adding `selectedThreadDetailState.error` to the early-return guard alongside the `null`-detail and `deleted`-status checks so retries only fire for the intended stalled-empty case.
| return useSelectedThreadDetailQuery().state; | ||
| } | ||
|
|
||
| export function useSelectedThreadDetailQuery() { |
There was a problem hiding this comment.
🟡 Medium state/use-thread-detail.ts:37
useSelectedThreadDetailQuery builds its target from selectedThread, which stays null during a cold open before the shell list materializes. The hook therefore queries (null, null) and its refresh only touches the empty-state atom, so the retry logic in ThreadRouteScreen can never refresh the real thread detail — the route stays stuck on the loading screen in the exact stalled-load case this PR intends to recover. The target should be derived from the route-backed thread id (useThreadSelection / route params) rather than selectedThread.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/state/use-thread-detail.ts around line 37:
`useSelectedThreadDetailQuery` builds its target from `selectedThread`, which stays `null` during a cold open before the shell list materializes. The hook therefore queries `(null, null)` and its `refresh` only touches the empty-state atom, so the retry logic in `ThreadRouteScreen` can never refresh the real thread detail — the route stays stuck on the loading screen in the exact stalled-load case this PR intends to recover. The target should be derived from the route-backed thread id (`useThreadSelection` / route params) rather than `selectedThread`.
ApprovabilityVerdict: Needs human review 10 blocking correctness issues found. Multiple unresolved review comments identify potential bugs in thread retry logic, state synchronization timing, and scroll behavior. The PR also introduces new crash logging infrastructure and modifies native iOS code and third-party library patches, warranting careful human review. You can customize Macroscope's approvability policy. Learn more. |
| const setReady = SubscriptionRef.update(state, (current) => ({ | ||
| ...current, | ||
| status: Option.isSome(current.snapshot) ? ("live" as const) : ("synchronizing" as const), | ||
| error: Option.none(), | ||
| })); |
There was a problem hiding this comment.
🟡 Medium state/shell.ts:103
setReady now sets the shell status to "live" whenever a cached snapshot exists, even when the shell subscription has not yet replayed missed updates after reconnect. On reconnect with stale cached data and pending server-side changes, the UI stops showing "synchronizing" and treats the shell as current until the first stream item arrives, so consumers briefly render out-of-date environment data as if synchronization completed. The previous implementation preserved the existing "live" status but otherwise stayed in "synchronizing"; the new version should similarly avoid promoting to "live" before the stream confirms it is up to date. Consider keeping the prior guard (only stay "live" if already "live", otherwise set "synchronizing") or waiting for the first stream item before marking "live".
- const setReady = SubscriptionRef.update(state, (current) => ({
- ...current,
- status: Option.isSome(current.snapshot) ? ("live" as const) : ("synchronizing" as const),
- error: Option.none(),
- }));
+ const setReady = SubscriptionRef.update(state, (current) =>
+ current.status === "live"
+ ? current
+ : {
+ ...current,
+ status: "synchronizing" as const,
+ error: Option.none(),
+ },
+ );🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/shell.ts around lines 103-107:
`setReady` now sets the shell status to `"live"` whenever a cached snapshot exists, even when the shell subscription has not yet replayed missed updates after reconnect. On reconnect with stale cached data and pending server-side changes, the UI stops showing `"synchronizing"` and treats the shell as current until the first stream item arrives, so consumers briefly render out-of-date environment data as if synchronization completed. The previous implementation preserved the existing `"live"` status but otherwise stayed in `"synchronizing"`; the new version should similarly avoid promoting to `"live"` before the stream confirms it is up to date. Consider keeping the prior guard (only stay `"live"` if already `"live"`, otherwise set `"synchronizing"`) or waiting for the first stream item before marking `"live"`.
| ); | ||
| } | ||
|
|
||
| if (props.contentPresentation.kind === "loading") { |
There was a problem hiding this comment.
🟡 Medium threads/ThreadFeed.tsx:1648
When contentPresentation transitions from loading to ready while the feed is still empty, the KeyboardAwareLegendList mounts with a zero bottom content inset. The useLayoutEffect that calls props.listRef.current?.reportContentInset({ bottom }) only reruns when listMountKey changes, and listMountKey stays ${threadId}:empty across the loading→ready transition — so the effect already ran while the list was unmounted (ref was null) and never fires again for the newly mounted instance. The composer inset is never reported, leaving the list with a zero bottom inset until a separate inset change occurs, causing empty threads to rest one composer-height too low after loading clears. Consider including contentPresentation.kind in listMountKey, or adding props.contentPresentation.kind to the effect's dependency array so it reruns when the list mounts.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadFeed.tsx around line 1648:
When `contentPresentation` transitions from `loading` to `ready` while the feed is still empty, the `KeyboardAwareLegendList` mounts with a zero bottom content inset. The `useLayoutEffect` that calls `props.listRef.current?.reportContentInset({ bottom })` only reruns when `listMountKey` changes, and `listMountKey` stays `${threadId}:empty` across the loading→ready transition — so the effect already ran while the list was unmounted (ref was `null`) and never fires again for the newly mounted instance. The composer inset is never reported, leaving the list with a zero bottom inset until a separate inset change occurs, causing empty threads to rest one composer-height too low after loading clears. Consider including `contentPresentation.kind` in `listMountKey`, or adding `props.contentPresentation.kind` to the effect's dependency array so it reruns when the list mounts.
bebf066 to
4e4c4b5
Compare
| // mount positions during attach, where UIKit applies the inset. | ||
| key={listMountKey} | ||
| style={{ flex: 1 }} | ||
| bounces={false} |
There was a problem hiding this comment.
🟡 Medium threads/ThreadFeed.tsx:1676
Setting bounces={false} together with alwaysBounceVertical={false} on the KeyboardAwareLegendList makes the pure-inset blank region (anchoredEndSpace/contentInset) immovable. Short threads get stuck under the composer/header with no way to drag the empty space into view, even though the existing applyWorkaroundForContentInsetHitTestBug prop shows the feed intentionally relies on the user being able to scroll that inset region. Consider removing alwaysBounceVertical={false} (or both props) so the inset area remains scrollable while still controlling overshcroll where needed.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadFeed.tsx around line 1676:
Setting `bounces={false}` together with `alwaysBounceVertical={false}` on the `KeyboardAwareLegendList` makes the pure-inset blank region (`anchoredEndSpace`/`contentInset`) immovable. Short threads get stuck under the composer/header with no way to drag the empty space into view, even though the existing `applyWorkaroundForContentInsetHitTestBug` prop shows the feed intentionally relies on the user being able to scroll that inset region. Consider removing `alwaysBounceVertical={false}` (or both props) so the inset area remains scrollable while still controlling overshcroll where needed.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Medium
The new loading early return at line 1649 renders ThreadFeedPlaceholder instead of the <View onLayout={handleViewportLayout}> wrapper, so viewportHeight stays 0 (and viewportWidth stays 0 in split layout) while the thread is loading. When the state transitions to ready, the KeyboardAwareLegendList mounts with viewportHeight === 0, so estimatedListSize is omitted and the list has no seeded viewport size for its attach-time scroll math — the feed can initially position too high or too low under the header/composer until a layout pass corrects it. Consider seeding viewportHeight/viewportWidth from useWindowDimensions() or restructuring the early returns so the viewport-measuring View stays mounted across the loading→ready transition.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadFeed.tsx around line 1275:
The new `loading` early return at line 1649 renders `ThreadFeedPlaceholder` instead of the `<View onLayout={handleViewportLayout}>` wrapper, so `viewportHeight` stays `0` (and `viewportWidth` stays `0` in split layout) while the thread is loading. When the state transitions to `ready`, the `KeyboardAwareLegendList` mounts with `viewportHeight === 0`, so `estimatedListSize` is omitted and the list has no seeded viewport size for its attach-time scroll math — the feed can initially position too high or too low under the header/composer until a layout pass corrects it. Consider seeding `viewportHeight`/`viewportWidth` from `useWindowDimensions()` or restructuring the early returns so the viewport-measuring `View` stays mounted across the loading→ready transition.
91d3b42 to
b6bf722
Compare
github-actions
Bot
added
size:XL
mwolson
changed the title
mwolson
changed the title
github-actions
Bot
removed
the
size:XL
github-actions
Bot
added
the
size:XXL
There was a problem hiding this comment.
🟡 Medium
t3code/patches/@legendapp__list@3.2.0.patch
Line 585 in 41b3536
The onScrollBeginDrag wrapper is created inside a useMemo([]) with an empty dependency array, so it permanently captures the onScrollBeginDrag prop from the first render. If the parent replaces or removes that callback after mount, drag events still invoke the stale callback (or undefined when it was removed). Store the callback in a ref or include it in the memo dependencies so the wrapper always calls the current prop.
🤖 Copy this AI Prompt to have your agent fix this:
In file @patches/@legendapp__list@3.2.0.patch around line 585:
The `onScrollBeginDrag` wrapper is created inside a `useMemo([])` with an empty dependency array, so it permanently captures the `onScrollBeginDrag` prop from the first render. If the parent replaces or removes that callback after mount, drag events still invoke the stale callback (or `undefined` when it was removed). Store the callback in a ref or include it in the memo dependencies so the wrapper always calls the current prop.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 41b3536. Configure here.
…ad UX - Replace Hermes-unsupported array helpers on mobile and client-runtime paths. - Remap draft image attachments to server ids before dispatching turns. - Fix native selectable markdown dropping trailing words while streaming and clipping wrapped text; fill the available message width. - Stop Home/Threads header setOptions loops that crashed Release builds. - Add a persisted fatal JS crash logger with startup replay. - Retry stalled thread detail loads and restore live status after reconnect. - Use RNGH Pressable for compact thread rows; disable feed overscroll bounce. - Hide subagent threads from the thread list, matching the desktop sidebar. - Keep the chat feed anchored during composer and keyboard inset changes (keyboard-controller and Legend List patches). - Measure Legend List's maintain-at-end zone from the scroll-range end so completed turns no longer rubber-band scroll-up attempts. - Surface string and tagged-object failure messages in thread and environment error states instead of a generic fallback.
| const lastAppliedOptionsRef = useRef<NativeStackNavigationOptions | undefined>(undefined); | ||
| const normalizedOptions = useMemo(() => normalizeScreenOptions(props.options), [props.options]); |
There was a problem hiding this comment.
🟡 Medium native/StackHeader.tsx:71
When the navigation object changes but normalizedOptions stays referentially stable, the effect returns early because lastAppliedOptionsRef.current === normalizedOptions, so navigation.setOptions is never called for the new navigation context. The deduplication guard ignores navigation, so options are silently dropped on a navigation change. Consider resetting lastAppliedOptionsRef.current when navigation changes so the options are re-applied to the new navigator.
| const lastAppliedOptionsRef = useRef<NativeStackNavigationOptions | undefined>(undefined); | |
| const normalizedOptions = useMemo(() => normalizeScreenOptions(props.options), [props.options]); | |
| const lastAppliedOptionsRef = useRef<NativeStackNavigationOptions | undefined>(undefined); | |
| const normalizedOptions = useMemo(() => normalizeScreenOptions(props.options), [props.options]); | |
| useEffect(() => { | |
| lastAppliedOptionsRef.current = undefined; | |
| }, [navigation]); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/native/StackHeader.tsx around lines 71-72:
When the `navigation` object changes but `normalizedOptions` stays referentially stable, the effect returns early because `lastAppliedOptionsRef.current === normalizedOptions`, so `navigation.setOptions` is never called for the new navigation context. The deduplication guard ignores `navigation`, so options are silently dropped on a navigation change. Consider resetting `lastAppliedOptionsRef.current` when `navigation` changes so the options are re-applied to the new navigator.
|
Superseded by the more minimal CTM mobile PR #3923 ( #3923 keeps the proven subset (draft attachment id persistence, Hermes Keeping the |
1 participant
Summary
Pressable) without removing long-press menus that main already has.Problem and Fix
toSorted,toReversed, andfindLaston mobile/client-runtime paths with compatible copy, sort, and reverse iteration logic.idand adataUrlwere dispatched with draft-only ids, so turns could reference attachments the server never stored.assetsPersistChatAttachmentsand dispatch turns with server-assigned attachment ids; mobile send paths strip draft-only fields viatoUploadChatImageAttachments.fillWidthpath so assistant markdown stretches to the available feed width.HomeHeaderandNativeStackScreenOptionsre-applied fresh option objects every render, causingsetOptionsupdate loops (maximum update depth) that crashed Release builds.livewhen the supervisor is ready and data already exists.causeFailureMessagehelper that surfacesErrormessages, string failures, and objects with a stringmessage, using the caller-supplied fallback only when nothing usable exists.Pressable. Main already keeps both tap and long-press (menu wrap +shouldOpenOnLongPress).ControlPillMenulong-press wrap (same as main/CTM). Use RNGHPressablefor the compact row body so tap-to-open stays reliable next to long-press and swipe. Archive/delete stay on long-press menu and swipe.contentInsetvalues without preserving the visible offset.setContentInset:override on the patched list.contentSize - scroll - scrollLength) so only genuinely at-end positions maintain scroll position.UI Changes
Pressableso those gestures coexist more reliably on the v2 stack.Validation
vp checkandvp run typecheckon the squashed single commit.vp test packages/client-runtime/src/state/threads-sync.test.ts packages/client-runtime/src/state/shell-sync.test.tsvp test apps/mobile/src/lib/nativeMarkdownText.test.tsvp test packages/client-runtime/src/errors/causeMessage.test.ts(8 tests for the shared failure-message helper)vp run lint:mobilepassed, with SwiftLint, ktlint, and detekt skipped because they are not installed locally.trees/ios-v2(baset3code/codex-turn-mapping, tip only) on Simulator and onMOiPhone25(personal signing for the device install).orchestrator-v2integration tree.Checklist
Note
Fix iOS v2 runtime paths, image attachment ids, header stability, and markdown layout
T3MarkdownTextShadowNodelayout to derive content from current children on each layout pass and only callsetStateDatawhen content changes, preventing stale or dropped text on iOS.persistAttachments: dual-tagged draft attachments (with both a client id anddataUrl) are now uploaded viaassetsPersistChatAttachmentsand replaced with server-assigned ids before dispatch.Array.prototype.findLast,toSorted, andtoReversedcalls across runtime state modules with explicit reverse loops and copy-then-sort patterns for iOS v2 compatibility.HomeHeaderoptions referentially stable viauseMemo/propsRefand guardsNativeStackScreenOptionsagainst re-applying identical option objects, preventing navigation update loops.readyevent,EnvironmentThreadStateandEnvironmentShellStatenow transition tolivewhen cached data exists, rather than stayingsynchronizing; adds auto-retry (2.5s and 6s) for missing thread detail while connected.Macroscope summarized f350d96.
Note
Medium Risk
Touches turn dispatch attachment persistence, native Fabric markdown state, and heavily patched scroll/keyboard inset behavior on iOS; regressions could affect message sending, thread layout, or post-reconnect sync UX.
Overview
This PR tightens mobile iOS reliability across chat UI, sync, and turn delivery, with supporting client-runtime and vendor patch changes.
Native markdown no longer caches measured text in mutable shadow-node state; layout rebuilds content from children and updates state only when it changed, fixing dropped text when Yoga layouts without measuring. Assistant messages use
fillWidthso markdown reflows correctly in shrink-to-fit bubbles.Image attachments on mobile now strip draft-only fields via
toUploadChatImageAttachmentsbeforestartTurn.persistAttachmentstreats anything withdataUrlas an upload (even if it also has a clientid), persists throughassetsPersistChatAttachments, and dispatches with server attachment ids.Navigation / home:
HomeHeadermemoizes stack options and routes callbacks through a ref to avoidsetOptionsloops;NativeStackScreenOptionsskips re-applying identical option objects. Home thread grouping hides subagent threads like desktop. Compact list rows use gesture-handlerPressablefor better long-press / swipe behavior.Thread experience: dedicated loading feed placeholder; 2.5s / 6s detail refresh retries while connected but detail is missing; thread queries expose
refreshand sharedcauseFailureMessageerrors. Composer/list spacing adds a 24px gap above the floating composer. Fatal JS crash logging installs at entry viainstallCrashLog.Reconnect: shell and per-thread state return to
liveimmediately when cached projection/snapshot exists after reconnect, without waiting for new stream events.Compatibility: widespread replacement of
findLast/toSorted/toReversedwith manual reverse loops in client-runtime and mobile state.Patches extend
@legendapp/listandreact-native-keyboard-controller(and a small native-stack mail toolbar tweak) for translucent-header insets, composer height adjustment, and scroll offset preservation under automatic content insets.Reviewed by Cursor Bugbot for commit f350d96. Bugbot is set up for automated code reviews on this repo. Configure here.