fix(mobile): polish native iOS onboarding and navigation (CODE-581) - #519
fix(mobile): polish native iOS onboarding and navigation (CODE-581)#519Zerlight wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new primary-action registration currently uses unstable function/object identities that can cause repeated re-registration and potential render-loop behavior without the suggested fixes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Polishes the mobile app’s onboarding and navigation by leaning on native iOS navigation chrome (including iOS 26 tab-bar behavior), moving Settings behind an overflow menu, and reshaping host onboarding into a dedicated “Add host” sheet flow.
Changes:
- Introduces a “primary action” registry to drive iOS 26’s separated tab-bar slot and reuses it for trailing header actions (primary button + overflow → Settings).
- Moves manual host entry out of the Connect screen into a dedicated
/add-hostform-sheet route, updating root stack options accordingly. - Updates i18n strings and E2E flows to match the new “Manage hosts” navigation and revised first-run/back behavior.
File summaries
| File | Description |
|---|---|
| packages/presentation/i18n/src/locales/zh-cn.ts | Updates Connect title and adds new labels (cancel, more) for revised nav UI. |
| packages/presentation/i18n/src/locales/en.ts | Same as zh-CN: Connect title and new labels for the overflow/menu and sheet. |
| apps/mobile/src/components/shell/use-trailing-actions.tsx | Adds iOS native header menu + Android RN fallback for trailing actions. |
| apps/mobile/src/components/shell/use-stack-screen-options.ts | Refactors shared stack header defaults and exports reusable visible/large-title presets. |
| apps/mobile/src/components/shell/root-navigator.tsx | Configures /add-host as a form sheet with a visible header. |
| apps/mobile/src/components/shell/primary-action.ts | Adds contexts + hooks for registering/reading a focused tab’s primary action. |
| apps/mobile/src/components/shell/primary-action-scope.tsx | Implements the primary-action registry provider for the tabs subtree. |
| apps/mobile/src/components/shell/ios-26-navigation.ts | Centralizes iOS 26 navigation detection gate. |
| apps/mobile/src/components/shell/brand-mark.tsx | Adjusts brand mark rendering (continuous corner curve, decorative image semantics). |
| apps/mobile/src/components/settings/settings-screen.tsx | Moves settings header behavior to shared options and removes host menu from header-left. |
| apps/mobile/src/components/connect/manual-host-section.tsx | Replaces inline manual-entry form with a navigation row to /add-host. |
| apps/mobile/src/components/connect/add-host-screen.tsx | Adds the new SwiftUI-based “Add host” sheet screen + header items. |
| apps/mobile/src/app/terminal-appearance.tsx | Applies shared visible-header options. |
| apps/mobile/src/app/sign-in.tsx | Reworks first-run sign-in UI with native-ish iOS styling and improved accessibility. |
| apps/mobile/src/app/settings.tsx | Adds a standalone Settings route pushed from overflow, separate from tabs. |
| apps/mobile/src/app/session/[sessionId].tsx | Applies shared visible-header options and adjusts list footer padding on iOS. |
| apps/mobile/src/app/connect.tsx | Updates header options and removes disclosure-based manual host entry. |
| apps/mobile/src/app/add-host.tsx | Adds the /add-host route entry. |
| apps/mobile/src/app/account.tsx | Applies shared visible-header options. |
| apps/mobile/src/app/(tabs)/threads/index.tsx | Registers a per-tab primary action and uses shared trailing actions + header options. |
| apps/mobile/src/app/(tabs)/threads/_layout.tsx | Simplifies stack options usage for the Threads tab stack. |
| apps/mobile/src/app/(tabs)/terminals/index.tsx | Registers a per-tab primary action and uses shared trailing actions + header options. |
| apps/mobile/src/app/(tabs)/terminals/_layout.tsx | Simplifies stack options usage for the Terminals tab stack. |
| apps/mobile/src/app/(tabs)/settings/index.tsx | Removes the Settings tab route. |
| apps/mobile/src/app/(tabs)/settings/_layout.tsx | Removes the Settings tab stack layout. |
| apps/mobile/src/app/(tabs)/compose.ts | Adds an unreachable route for the iOS 26 tab-bar “compose slot” trigger. |
| apps/mobile/src/app/(tabs)/_layout.tsx | Adds PrimaryActionScope and iOS 26 “compose slot” trigger behavior. |
| apps/mobile/e2e/flows/settings.yaml | Updates guard comments to reflect new navigation timing/structure. |
| apps/mobile/e2e/flows/first-run.yaml | Updates assertions and back-navigation expectations for the revised first-run flow. |
| apps/mobile/e2e/flows/add-host.yaml | Updates the flow for the new “Add host by URL” navigation + sheet screen. |
Review details
- Files reviewed: 30/30 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const primaryAction: PrimaryAction | null = | ||
| connection?.status === 'ready' | ||
| ? { | ||
| sf: 'plus', | ||
| icon: PlusIcon, | ||
| label: t('newTerminal'), | ||
| onPress: () => setSheetOpen(true), | ||
| } | ||
| : null; |
| const primaryAction: PrimaryAction | null = | ||
| connection?.status === 'ready' | ||
| ? { | ||
| sf: 'square.and.pencil', | ||
| icon: SquarePenIcon, | ||
| label: t('newThread'), | ||
| onPress: () => setSheetOpen(true), | ||
| } | ||
| : null; |
| export function usePrimaryAction(tab: string, action: PrimaryAction | null): void { | ||
| const register = use(RegisterPrimaryActionContext); | ||
| useEffect(() => { | ||
| if (action) return register(tab, action); | ||
| }, [register, tab, action]); | ||
| } |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ The navigation restructure holds up — the concerns below are one real UI mislabel, one weakened E2E guard, and some polish.
Reviewed changes
- Traced the new primary-action registry (
primary-action.ts,primary-action-scope.tsx,(tabs)/_layout.tsx) and both registration sites against a React-Compiler-off render model. - Traced
dismissTo('/threads')from the newadd-hostformSheet throughexpo-router'slinkTo→getNavigateAction→findDivergentState→StackRouter'sPOP_TO. - Traced the sign-in
push/Redirectchange against every entry path into/connect. - Re-read the three edited Maestro flows against the renamed i18n strings.
- Ran the real gates on this branch:
tsc --build --noEmit apps/mobile(exit 0) andeslintoverapps/mobile/src/{app,components/shell,components/connect}(0 errors; the 2 warnings are pre-existing).
⚠️ This branch's gates were never run on this branch
The description says pnpm check:ci and pnpm test were verified at the stack tip 33bd138a, and that the onboarding and connect E2E flows "were not rerun for this PR publication." Since master → #519 → #520 → #521 merge independently, #519 has to stand on its own.
I ran the JavaScript gates for you and they are clean on 83b8aa1 — tsc --build --noEmit apps/mobile exits 0, and eslint over the touched trees reports 0 errors (the no-console at (tabs)/_layout.tsx:30 and the set-state-in-effect at my-machines-section.tsx:26 both predate this branch).
What I could not cover is the part that matters most here. add-host.yaml, first-run.yaml, and settings.yaml are the only automated coverage for the flows this PR restructures — the manual-host DisclosureGroup becoming a formSheet, the skip-to-Connect back affordance, and the Settings tab becoming a root-stack screen. All three were edited in this PR and none were rerun. Two of them have specific problems noted inline. Please run the flows on this branch before merging.
ℹ️ Nitpicks
add-host-screen.tsx now duplicates useOpenHost's setLastActiveHostId + navigate pair rather than reusing it. The duplication is justified — the sheet needs dismissTo, not push — but the two "adopt a host" paths can now drift independently. A shared helper that takes the navigation verb would keep them honest.
The urlValid gate in the new sheet is a real behavior change worth calling out, even though I think it is the right trade. The deleted manual-host-section.tsx carried this note:
The fields are backed by native state rather than mirrored into React:
get()reads what the field itself holds, so submitting never depends on a change event reaching JS first.
Enabling the checkmark bar button off urlValid reintroduces exactly that dependency — the button stays disabled until an onTextChange lands in JS. With a native TextField that is effectively synchronous, so this is fine in practice, but the invariant the old comment protected is gone and nothing records that it was deliberate.
Everything else I chased turned out clean, listed here so nobody re-investigates: (tabs)/compose.ts does not leak as a phantom tab on pre-26 iOS (native tabs pass useOnlyUserDefinedScreens = true to withLayoutContext, so a route with no Trigger never reaches the navigator); the fresh primaryAction object literal in usePrimaryAction's dependency array does not loop despite React Compiler being off for apps/mobile (the context split keeps screens off PrimaryActionRegistryContext, and React Navigation's StaticContainer stops registry churn short of the screen bodies); dismissTo('/threads') from the formSheet resolves to payload name (tabs), not threads, so POP_TO finds it at index 0 and pops correctly; the appleAvailable === null spinner gate does not hang Android (expo-apple-authentication's stub resolves false); the new ListFooterComponent in session/[sessionId].tsx is correct because that FlatList is inverted; and en.ts / zh-cn.ts are the only locales, both updated.
Claude Opus ｜ 𝕏
| if (focused) focused.onPress(); | ||
| else router.navigate('/threads'); | ||
| }, | ||
| }} | ||
| > | ||
| <NativeTabs.Trigger.Icon sf={focused?.sf ?? 'square.and.pencil'} /> | ||
| <NativeTabs.Trigger.Label> | ||
| {focused?.label ?? tThreads('newThread')} |
There was a problem hiding this comment.
The fallback is unconditionally the Threads action, so on the Terminals tab it advertises the wrong thing.
focused is actions[segments.at(1) ?? 'threads'] ?? null, and TerminalsRoute only registers when connection?.status === 'ready'. Stand on Terminals with the host still connecting, unreachable, or not yet selected — the common first-run state — and actions['terminals'] is undefined, so focused is null. The separated slot then renders square.and.pencil with the label "New thread", and tapping it jumps the user out of Terminals over to /threads.
The comment two lines up says the slot "carries the focused tab's primary action", which is what makes this read as unintended rather than a deliberate default.
The fix is to make the placeholder follow the segment rather than hardcoding the Threads one — pick the icon and label from the current tab, and keep router.navigate pointed at the tab you are already on so a disabled action is inert instead of navigational. segments.at(1) is already in hand for that.
| # The guard has to be a row, not the title: a host screen carries a Settings *tab*, so "Settings" | ||
| # alone is satisfied by the very redirect this retry exists to outlast. | ||
| # The guard is a row, not the title, so a pass needs the SwiftUI form itself — the navigation | ||
| # bar alone renders before the redirect this retry exists to outlast. |
There was a problem hiding this comment.
The rename to Manage hosts makes this guard satisfiable by the exact screen it exists to exclude.
The guard below is assertVisible: 'Manage hosts'. Settings has a Manage hosts row, which is what this comment is describing — but as of en.ts in this PR, the Connect screen's navigation-bar title is also Manage hosts. So if the hydration redirect wins and lands the app on /connect, assertVisible: 'Manage hosts' passes against Connect's title bar, the retry loop exits satisfied, and the flow then fails at assertVisible: 'Settings' on line 21 with no retries left. That is the flaky failure the retry was written to prevent.
The new wording ("a pass needs the SwiftUI form itself — the navigation bar alone renders before the redirect") is now the opposite of what the string does: a navigation bar alone is sufficient to satisfy it.
Switch the guard to a string that only Settings has — Terminal appearance and Share usage analytics are both already asserted further down, so either is free.
| # bar alone renders before the redirect this retry exists to outlast. | |
| # The guard is a row unique to Settings, not the title: Connect's navigation bar now also reads | |
| # "Manage hosts", so a title match would be satisfied by the redirect this retry exists to outlast. |
(and change line 19's assertVisible to 'Terminal appearance')
| placeholder={t('searchPlaceholder')} | ||
| placement="stacked" | ||
| hideWhenScrolling={false} | ||
| hideWhenScrolling |
There was a problem hiding this comment.
This flip drops a documented constraint that still appears to hold, with nothing recording what invalidated it.
The line it replaces was hideWhenScrolling={false}, justified by: the screen body is a SwiftUI host, so nothing here can drive hide-on-scroll. That justification is still true on this branch — the body below is <Host useViewportSizeMeasurement> wrapping a SwiftUI List, not a UIScrollView the UINavigationController knows about.
UIKit's hidesSearchBarWhenScrolling = true starts the field collapsed and only reveals it when the tracked scroll view reaches the top. With no tracked scroll view, the plausible outcome is a search field that never appears at all, which would make thread search unreachable rather than just differently animated.
If iOS 26 plus placement="stacked" changes this and you have watched the field behave correctly on a real 26 build, that is a fine answer — but it needs to be the replacement comment, because the next person will otherwise re-derive the original constraint and flip it back. If it has only been observed on 18.5, please check 26 before merging.
| # Skip pushes host setup above sign-in so the native back affordance returns to the OOBE instead of | ||
| # revealing an older copy of the same Connect screen. | ||
| - tapOn: | ||
| point: '6%,8%' |
There was a problem hiding this comment.
A percentage tap for the back chevron will silently start hitting the wrong thing.
Every other interaction in this file selects by visible text, and the header comments elsewhere in the suite are explicit about not depending on incidental screen state. point: '6%,8%' bakes in the navigation bar's height and the chevron's inset — both of which move with the device, the status-bar height, Dynamic Type, and the iOS 26 bar metrics this PR is specifically adapting to. When it drifts it will not fail loudly; it will tap whatever is at that coordinate and fail on a confusing downstream assertion.
Maestro can target the back button by accessibility identifier. Since /connect is pushed from /sign-in, the chevron carries a back label — prefer tapOn: { id: '...' } against that over a coordinate. If the minimal back-button display mode leaves nothing addressable, adding an explicit accessibility label to the Connect screen's back affordance is a better trade than the coordinate.
| animation={{ scale: false }} | ||
| style={styles.manualButton} | ||
| variant="ghost" | ||
| onPress={() => router.push('/connect')} |
There was a problem hiding this comment.
Minor: push here plus the focus-scoped Redirect above makes back-from-Connect flash this screen after sign-in.
Keeping sign-in beneath Connect is the point of the change and it is right for the skip path. But Redirect runs its router.replace inside useFocusEffect, not on mount, so it fires every time this screen regains focus. Sign in from Connect's SignInSection, then tap back: sign-in comes into view, account.status is now signed-in, the redirect fires, and the user is pushed forward onto a fresh /connect instance — losing the scroll position and any sheet state on the one they came from.
It is not a dead end and it does not accumulate (SignInSection hides once signed in, so the round trip is not repeatable), which is why this is a nit rather than a blocker. If you want it gone, dismissing back to the existing Connect rather than replacing into a new one would do it.
Summary
Polish mobile onboarding, host entry, and navigation with native iOS controls. Preserve first-run/back-navigation behavior, move settings into the header menu, and adapt titles and primary actions to the supported iOS generations.
Part 1 of the mobile stack:
master→ #519 → #520 → #521. Base is the latest master,da9c0673. Merge in this order.Refs CODE-581.
Verification
33bd138a); this lower branch was not independently rerun.Checklist
pnpm check:ciandpnpm testpass at the complete stack tip