Improve iPad mobile navigation and offline caching#3794
Improve iPad mobile navigation and offline caching#3794juliusmarminge wants to merge 71 commits into
Conversation
- Enable adaptive sidebar navigation on wide mobile windows - Keep compact single-pane behavior on phones - Extract and test shared thread grouping and layout logic Co-authored-by: codex <codex@users.noreply.github.com>
- Adapt iPad/mobile layout for sidebar, inspector, and sheets - Move review diff payloads off Fabric props and add native scrolling - Add iOS header button module for consistent toolbar actions Co-authored-by: codex <codex@users.noreply.github.com>
- Keep sidebar threads swipeable with archive/delete actions - Hide archived threads from navigation groups - Stabilize review diff visibility and optimistic file selection Co-authored-by: codex <codex@users.noreply.github.com>
- Stabilize thread selection routing in the adaptive workspace layout - Extract and memoize thread sidebar rows to reduce unnecessary rerenders Co-authored-by: codex <codex@users.noreply.github.com>
- add thread selection context for split-view routing - switch thread feeds to automatic content insets on iOS glass headers - add native scroll-edge effect helper and app domain config
- Update Clerk packages across web, desktop, relay, and mobile - Refresh lockfile entries for related Expo and native packages
- Nest settings routes for form-sheet navigation - Persist server config and VCS refs across web and mobile - Add mobile header actions for empty connection state
|
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)
Comment |
github-actions
Bot
added
vouch:trusted
juliusmarminge
changed the title
|
Superseded by #3795, which is rebased on current main and contains only the offline-cache change. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| <NativeTerminalSurfaceView | ||
| appearanceScheme={appearanceScheme} | ||
| backgroundColor={theme.background} | ||
| focusRequest={props.isRunning ? (props.keyboardFocusRequest ?? 0) : 0} |
There was a problem hiding this comment.
🟠 High terminal/NativeTerminalSurface.tsx:221
On Android native terminals, incrementing keyboardFocusRequest no longer reopens the keyboard after the user dismisses it. This PR replaced the hidden React TextInput focus workaround with a focusRequest prop on NativeTerminalSurfaceView, but the Android native module (T3TerminalModule.kt) does not export a focusRequest prop, so the prop is silently ignored. After the keyboard is dismissed, the floating "Show keyboard" button increments keyboardFocusRequest, which now has no effect — terminal input is unavailable until the user finds another focus path. Add a Prop("focusRequest") handler to the Android native module that focuses the terminal surface, or restore the hidden TextInput fallback for platforms that lack native focusRequest support.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/terminal/NativeTerminalSurface.tsx around line 221:
On Android native terminals, incrementing `keyboardFocusRequest` no longer reopens the keyboard after the user dismisses it. This PR replaced the hidden React `TextInput` focus workaround with a `focusRequest` prop on `NativeTerminalSurfaceView`, but the Android native module (`T3TerminalModule.kt`) does not export a `focusRequest` prop, so the prop is silently ignored. After the keyboard is dismissed, the floating "Show keyboard" button increments `keyboardFocusRequest`, which now has no effect — terminal input is unavailable until the user finds another focus path. Add a `Prop("focusRequest")` handler to the Android native module that focuses the terminal surface, or restore the hidden `TextInput` fallback for platforms that lack native `focusRequest` support.
| headerRightBarButtonItems && isHeaderBarButtonsAvailableForCurrentPlatform | ||
| ? prepareHeaderBarButtonItems(headerRightBarButtonItems, 'right') | ||
| : undefined; | ||
| + const preparedHeaderCenterBarButtonItems = |
There was a problem hiding this comment.
🟡 Medium patches/react-native-screens@4.25.2.patch:1357
headerCenterBarButtonItems and headerToolbarItems are prepared with side hard-coded to 'right', so prepareMenu derives buttonId/menuId values using the same side-plus-index scheme as headerRightBarButtonItems. When a screen has both real right items and center or toolbar items at the same index, the IDs collide, causing presses and menu selections to be dispatched to the wrong item callback. Use distinct side values (e.g. 'center' and 'toolbar') for the two new prepareHeaderBarButtonItems calls so the generated IDs don't overlap with right or left items.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @patches/react-native-screens@4.25.2.patch around line 1357:
`headerCenterBarButtonItems` and `headerToolbarItems` are prepared with `side` hard-coded to `'right'`, so `prepareMenu` derives `buttonId`/`menuId` values using the same `side`-plus-index scheme as `headerRightBarButtonItems`. When a screen has both real right items and center or toolbar items at the same index, the IDs collide, causing presses and menu selections to be dispatched to the wrong item callback. Use distinct `side` values (e.g. `'center'` and `'toolbar'`) for the two new `prepareHeaderBarButtonItems` calls so the generated IDs don't overlap with right or left items.
| return <OpeningThreadLoadingScreen />; | ||
| } | ||
|
|
||
| if (selectedThread !== null && selectedThreadKey === routeThreadKey) { |
There was a problem hiding this comment.
🟡 Medium threads/ThreadRouteScreen.tsx:192
ThreadRouteScreen renders ThreadUnavailableScreen for a valid thread that is still synchronizing, because the early-return that checks selectedThread !== null && selectedThreadKey === routeThreadKey is skipped when useThreadSelection() returns null during hydration. The code then falls through to the stillHydrating branch, which only checks connection state and ignores useSelectedThreadDetailState(), so a thread that loads a moment later is shown as unavailable instead of showing the loading screen. Consider falling back to the loading screen when the route references a known environment/thread but selectedThread is still null, rather than treating that state as a hydration failure.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadRouteScreen.tsx around line 192:
`ThreadRouteScreen` renders `ThreadUnavailableScreen` for a valid thread that is still synchronizing, because the early-return that checks `selectedThread !== null && selectedThreadKey === routeThreadKey` is skipped when `useThreadSelection()` returns `null` during hydration. The code then falls through to the `stillHydrating` branch, which only checks connection state and ignores `useSelectedThreadDetailState()`, so a thread that loads a moment later is shown as unavailable instead of showing the loading screen. Consider falling back to the loading screen when the route references a known environment/thread but `selectedThread` is still `null`, rather than treating that state as a hydration failure.
| guard let headerOffset = contentView.fileHeaderOffset(forFileId: fileId) else { | ||
| if !rows.isEmpty { | ||
| pendingScrollFileId = nil | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
🟡 Medium ios/T3ReviewDiffView.swift:830
applyPendingScrollIfNeeded silently drops a queued scrollToFile request when the requested fileId is not present in the current rows and rows is non-empty. During a diff switch, the JS inspector updates immediately to the new file list while native rows still reflect the previous diff until setRowsJson finishes decoding. If the user taps a file in that window, fileHeaderOffset(forFileId:) returns nil, so pendingScrollFileId is cleared — and when the new rows arrive, the queued scroll is gone and the navigation never happens. Consider keeping the pending request until the new rows match (or until an explicit reset clears it) instead of clearing it just because the current rows are non-empty.
guard let headerOffset = contentView.fileHeaderOffset(forFileId: fileId) else {
- if !rows.isEmpty {
- pendingScrollFileId = nil
- }
return
- }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift around lines 830-835:
`applyPendingScrollIfNeeded` silently drops a queued `scrollToFile` request when the requested `fileId` is not present in the current `rows` and `rows` is non-empty. During a diff switch, the JS inspector updates immediately to the new file list while native `rows` still reflect the previous diff until `setRowsJson` finishes decoding. If the user taps a file in that window, `fileHeaderOffset(forFileId:)` returns `nil`, so `pendingScrollFileId` is cleared — and when the new rows arrive, the queued scroll is gone and the navigation never happens. Consider keeping the pending request until the new rows match (or until an explicit reset clears it) instead of clearing it just because the current rows are non-empty.
| if (event.nativeEvent.actionName === "increment") { | ||
| props.onResizeBy(ACCESSIBILITY_RESIZE_STEP); | ||
| } else if (event.nativeEvent.actionName === "decrement") { | ||
| props.onResizeBy(-ACCESSIBILITY_RESIZE_STEP); | ||
| } |
There was a problem hiding this comment.
🟡 Medium layout/workspace-pane-divider.tsx:61
handleAccessibilityAction ignores the resizeDirection prop, so accessibility "increment" and "decrement" actions move the pane in the wrong direction when resizeDirection is -1. The only caller passes resizeDirection={-1}, meaning VoiceOver/keyboard users who activate "Make pane wider" actually shrink the pane and vice versa. The drag gesture multiplies by resizeDirection, but the accessibility handler does not. Consider applying props.resizeDirection in handleAccessibilityAction the same way handleResize does.
| if (event.nativeEvent.actionName === "increment") { | |
| props.onResizeBy(ACCESSIBILITY_RESIZE_STEP); | |
| } else if (event.nativeEvent.actionName === "decrement") { | |
| props.onResizeBy(-ACCESSIBILITY_RESIZE_STEP); | |
| } | |
| if (event.nativeEvent.actionName === "increment") { | |
| props.onResizeBy(ACCESSIBILITY_RESIZE_STEP * props.resizeDirection); | |
| } else if (event.nativeEvent.actionName === "decrement") { | |
| props.onResizeBy(-ACCESSIBILITY_RESIZE_STEP * props.resizeDirection); | |
| } |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/layout/workspace-pane-divider.tsx around lines 61-65:
`handleAccessibilityAction` ignores the `resizeDirection` prop, so accessibility "increment" and "decrement" actions move the pane in the wrong direction when `resizeDirection` is `-1`. The only caller passes `resizeDirection={-1}`, meaning VoiceOver/keyboard users who activate "Make pane wider" actually shrink the pane and vice versa. The drag gesture multiplies by `resizeDirection`, but the accessibility handler does not. Consider applying `props.resizeDirection` in `handleAccessibilityAction` the same way `handleResize` does.
| readonly threads: ReadonlyArray<EnvironmentThreadShell>; | ||
| } | ||
|
|
||
| const threadActivityOrder = Order.mapInput( |
There was a problem hiding this comment.
🟡 Medium threads/thread-navigation-groups.ts:16
threadActivityOrder sorts threads by new Date(thread.updatedAt ?? thread.createdAt).getTime(), but the canonical thread sorting elsewhere uses the latest user-message timestamp for recency. When a thread has an older updatedAt but a newer latest user message, this ordering places it below older conversations, so the navigation drawer displays threads in the wrong recency order. Consider deriving the sort key from the same latest-user-message timestamp used by the canonical sort, or document why updatedAt is intentionally used here.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/thread-navigation-groups.ts around line 16:
`threadActivityOrder` sorts threads by `new Date(thread.updatedAt ?? thread.createdAt).getTime()`, but the canonical thread sorting elsewhere uses the latest user-message timestamp for recency. When a thread has an older `updatedAt` but a newer latest user message, this ordering places it below older conversations, so the navigation drawer displays threads in the wrong recency order. Consider deriving the sort key from the same latest-user-message timestamp used by the canonical sort, or document why `updatedAt` is intentionally used here.
| const hasTerminalDetailState = | ||
| selectedThreadDetailState.status === "deleted" || | ||
| Option.isSome(selectedThreadDetailState.error); | ||
|
|
There was a problem hiding this comment.
🟠 High threads/ThreadRouteScreen.tsx:187
Opening a thread on iOS crashes immediately with Couldn't find the header height. Are you inside a screen in a navigator with a header? because ThreadDetailScreen calls useHeaderHeight() unconditionally, but the new usesNativeHeaderGlass branch renders inside a standalone ScreenStack with the outer native-stack headerShown: false. There is no HeaderHeightContext provider above ThreadDetailScreen on this path, so the hook throws. Consider guarding the useHeaderHeight() call (falling back to 0 or insets.top when usesAutomaticContentInsets is false) so the screen can render outside a React Navigation header context.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadRouteScreen.tsx around line 187:
Opening a thread on iOS crashes immediately with `Couldn't find the header height. Are you inside a screen in a navigator with a header?` because `ThreadDetailScreen` calls `useHeaderHeight()` unconditionally, but the new `usesNativeHeaderGlass` branch renders inside a standalone `ScreenStack` with the outer native-stack `headerShown: false`. There is no `HeaderHeightContext` provider above `ThreadDetailScreen` on this path, so the hook throws. Consider guarding the `useHeaderHeight()` call (falling back to `0` or `insets.top` when `usesAutomaticContentInsets` is `false`) so the screen can render outside a React Navigation header context.
| ); | ||
| }); | ||
|
|
||
| yield* Stream.concat( |
There was a problem hiding this comment.
🟡 Medium state/vcs.ts:85
makeCachedVcsRefsState swallows every vcsListRefs refresh failure in the Stream.runForEach handler and leaves state unchanged. For uncached requests (any searched or paginated branch list), state starts as Option.none(), and cachedVcsRefsChanges filters out None via Result.failVoid, so the stream never emits anything. A failed branch-list RPC therefore leaves the atom permanently stuck with no error surfaced to callers — the previous query-based implementation returned an AsyncResult failure that UI code could show. Consider propagating refresh failures so callers can observe and display them.
Also found in 1 other location(s)
apps/mobile/src/App.tsx:21
Appignores the error value fromuseFontsand only checksfontsLoaded. Expo's font hook returns[loaded, error]; on a font load failureloadedstays false whileerroris set. In that case line 34 keeps renderingLoadingScreenforever, so the app never reachesRootNavigatorand startup is stuck until restart.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/client-runtime/src/state/vcs.ts around line 85:
`makeCachedVcsRefsState` swallows every `vcsListRefs` refresh failure in the `Stream.runForEach` handler and leaves `state` unchanged. For uncached requests (any searched or paginated branch list), `state` starts as `Option.none()`, and `cachedVcsRefsChanges` filters out `None` via `Result.failVoid`, so the stream never emits anything. A failed branch-list RPC therefore leaves the atom permanently stuck with no error surfaced to callers — the previous query-based implementation returned an `AsyncResult` failure that UI code could show. Consider propagating refresh failures so callers can observe and display them.
Also found in 1 other location(s):
- apps/mobile/src/App.tsx:21 -- `App` ignores the error value from `useFonts` and only checks `fontsLoaded`. Expo's font hook returns `[loaded, error]`; on a font load failure `loaded` stays false while `error` is set. In that case line 34 keeps rendering `LoadingScreen` forever, so the app never reaches `RootNavigator` and startup is stuck until restart.
| }; | ||
| } | ||
|
|
||
| export function AppNavigationProvider(props: { readonly children: ReactNode }) { |
There was a problem hiding this comment.
🟡 Medium navigation/app-navigation.tsx:157
When the app opens via a deep link to /settings/auth or /settings/archive, useCurrentPathname() returns "/" on the first render because pathname is initialized to "/" and syncState only runs in onReady/onStateChange, which fire after the navigation tree has already mounted. This means one-time consumers of useCurrentPathname() read the stale root value: ClerkSettingsSheetDetentProvider captures the initial pathname in local state, so it mounts with initiallyExpanded=false and never expands for those routes. syncState needs to compute the pathname synchronously before the first render (e.g. derive it from the initial navigation state) rather than waiting for the onReady callback.
Also found in 1 other location(s)
apps/mobile/src/navigation/RootNavigator.tsx:145
ClerkSettingsSheetDetentProvideronly readsinitiallyExpandedonce in itsuseStateinitializer, butRootNavigatorcomputes that prop frompathnamebefore the navigation container has synced the real initial route.AppNavigationProviderstartspathnameas"/", so on a cold launch or deep link to/settings/auththis provider mounts withfalseand never updates. As a result the auth settings sheet opens at the collapsed detent instead of the intended expanded one.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/navigation/app-navigation.tsx around line 157:
When the app opens via a deep link to `/settings/auth` or `/settings/archive`, `useCurrentPathname()` returns `"/"` on the first render because `pathname` is initialized to `"/"` and `syncState` only runs in `onReady`/`onStateChange`, which fire after the navigation tree has already mounted. This means one-time consumers of `useCurrentPathname()` read the stale root value: `ClerkSettingsSheetDetentProvider` captures the initial pathname in local state, so it mounts with `initiallyExpanded=false` and never expands for those routes. `syncState` needs to compute the pathname synchronously before the first render (e.g. derive it from the initial navigation state) rather than waiting for the `onReady` callback.
Also found in 1 other location(s):
- apps/mobile/src/navigation/RootNavigator.tsx:145 -- `ClerkSettingsSheetDetentProvider` only reads `initiallyExpanded` once in its `useState` initializer, but `RootNavigator` computes that prop from `pathname` before the navigation container has synced the real initial route. `AppNavigationProvider` starts `pathname` as `"/"`, so on a cold launch or deep link to `/settings/auth` this provider mounts with `false` and never updates. As a result the auth settings sheet opens at the collapsed detent instead of the intended expanded one.
| ); | ||
| } | ||
|
|
||
| export function useThreadGitRightHeaderItems(props: ThreadGitControlsProps): HeaderItems { |
There was a problem hiding this comment.
🟡 Medium threads/ThreadGitControls.tsx:397
useThreadGitHeaderActionItems never surfaces props.auxiliaryPaneControl, so when ThreadGitControls's inline toolbar is replaced by native header items on compact screens, there is no toggle/close button for the inspector pane. ThreadGitControls still renders an auxiliary-pane toggle button, but the native-header path served by useThreadGitRightHeaderItems/useThreadGitCenterHeaderItems omits it, regressing the compact inspector workflow. Consider including the auxiliaryPaneControl as a header item in useThreadGitHeaderActionItems (e.g., inserting it at the start of useThreadGitRightHeaderItems) so the toggle remains reachable.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/ThreadGitControls.tsx around line 397:
`useThreadGitHeaderActionItems` never surfaces `props.auxiliaryPaneControl`, so when `ThreadGitControls`'s inline toolbar is replaced by native header items on compact screens, there is no toggle/close button for the inspector pane. `ThreadGitControls` still renders an auxiliary-pane toggle button, but the native-header path served by `useThreadGitRightHeaderItems`/`useThreadGitCenterHeaderItems` omits it, regressing the compact inspector workflow. Consider including the `auxiliaryPaneControl` as a header item in `useThreadGitHeaderActionItems` (e.g., inserting it at the start of `useThreadGitRightHeaderItems`) so the toggle remains reachable.
1 participant
Summary
Why
The mobile navigation stack needed a native, predictable structure for iPad and iPhone layouts. Separately, cached project snapshots were insufficient for offline pending tasks because models and Git branches were only available from live server requests.
Impact
The mobile app has native-stack navigation and iPad-aware workspace surfaces. Offline users retain the last known provider/model and branch state; live server data replaces cached values after reconnecting, and environment removal clears the cache.
Validation
vp checkvp run typecheckvp run lint:mobilevp test packages/client-runtime/src/state/server.test.ts packages/client-runtime/src/state/vcs.test.ts