-
Notifications
You must be signed in to change notification settings - Fork 10
fix(mobile): polish native iOS onboarding and navigation (CODE-581) #519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5a51efe
a97214e
ad5ee63
281aa87
b15e707
6d6b543
d4f5b4c
83b8aa1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,8 +9,8 @@ appId: com.arcboxlabs.linkcode.mobile | |||||||
| - launchApp | ||||||||
| # A cold start redirects once the persisted host registry hydrates, which can land after the deep | ||||||||
| # link and replace the screen it opened; retrying re-issues the link past that window. | ||||||||
| # 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. | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rename to The guard below is 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 —
Suggested change
(and change line 19's |
||||||||
| - retry: | ||||||||
| maxRetries: 3 | ||||||||
| commands: | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,11 @@ | ||
| import { USES_IOS_26_NAVIGATION } from '@mobile/components/shell/ios-26-navigation'; | ||
| import { usePrimaryActions } from '@mobile/components/shell/primary-action'; | ||
| import { PrimaryActionScope } from '@mobile/components/shell/primary-action-scope'; | ||
| import { router, useSegments } from 'expo-router'; | ||
| import { NativeTabs } from 'expo-router/unstable-native-tabs'; | ||
| import { useTranslations } from 'use-intl'; | ||
|
|
||
| /** The app's three top-level surfaces. `NativeTabs` is a real `UITabBarController`, so the iOS 26 | ||
| /** The app's top-level surfaces. `NativeTabs` is a real `UITabBarController`, so the iOS 26 | ||
| * floating tab bar and its scroll-minimize behaviour come from UIKit rather than being drawn here. | ||
| * | ||
| * The tabs sit at the root and the host is a selection, not a parent route — switching hosts is a | ||
|
|
@@ -10,9 +14,21 @@ import { useTranslations } from 'use-intl'; | |
| * pushed screen, so pushing them from the root stack is the only way to keep the bar off a | ||
| * composer or a terminal canvas. */ | ||
| export default function TabsLayout(): React.ReactNode { | ||
| return ( | ||
| <PrimaryActionScope> | ||
| <TabsNavigator /> | ||
| </PrimaryActionScope> | ||
| ); | ||
| } | ||
|
|
||
| function TabsNavigator(): React.ReactNode { | ||
| const tThreads = useTranslations('mobile.sessions'); | ||
| const tTerminals = useTranslations('mobile.terminals'); | ||
| const tSettings = useTranslations('mobile.settings'); | ||
| const actions = usePrimaryActions(); | ||
| // Runtime segments under this layout are ['(tabs)', '<tab>'] — wider than the untyped-routes | ||
| // 1-tuple, hence `.at`. Before hydration fall back to home. | ||
| const segments = useSegments(); | ||
| const focused = actions[segments.at(1) ?? 'threads'] ?? null; | ||
|
|
||
| return ( | ||
| <NativeTabs> | ||
|
|
@@ -24,10 +40,26 @@ export default function TabsLayout(): React.ReactNode { | |
| <NativeTabs.Trigger.Icon sf="apple.terminal" /> | ||
| <NativeTabs.Trigger.Label>{tTerminals('title')}</NativeTabs.Trigger.Label> | ||
| </NativeTabs.Trigger> | ||
| <NativeTabs.Trigger name="settings"> | ||
| <NativeTabs.Trigger.Icon sf="gearshape" /> | ||
| <NativeTabs.Trigger.Label>{tSettings('title')}</NativeTabs.Trigger.Label> | ||
| </NativeTabs.Trigger> | ||
| {/* iOS 26's separated tab-bar slot (the `search` role) carries the focused tab's primary | ||
| * action: `disabled` keeps native selection prevented while tabPress still reaches JS. */} | ||
| {USES_IOS_26_NAVIGATION ? ( | ||
| <NativeTabs.Trigger | ||
| name="compose" | ||
| role="search" | ||
| disabled | ||
| listeners={{ | ||
| tabPress() { | ||
| if (focused) focused.onPress(); | ||
| else router.navigate('/threads'); | ||
| }, | ||
| }} | ||
| > | ||
| <NativeTabs.Trigger.Icon sf={focused?.sf ?? 'square.and.pencil'} /> | ||
| <NativeTabs.Trigger.Label> | ||
| {focused?.label ?? tThreads('newThread')} | ||
|
Comment on lines
+52
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fallback is unconditionally the Threads action, so on the Terminals tab it advertises the wrong thing.
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 |
||
| </NativeTabs.Trigger.Label> | ||
| </NativeTabs.Trigger> | ||
| ) : null} | ||
| </NativeTabs> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| /** Unreachable: the trigger is `disabled`, so native selection is prevented and its tabPress | ||
| * listener runs the focused tab's primary action instead. The file only gives the trigger a | ||
| * route. */ | ||
| export default function ComposeRoute(): React.ReactNode { | ||
| return null; | ||
| } |
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,10 @@ import { HostClientGate } from '@mobile/components/host/host-client-gate'; | |
| import { NewThreadSheet } from '@mobile/components/host/new-thread-sheet'; | ||
| import { ThreadList } from '@mobile/components/host/thread-list/thread-list'; | ||
| import { useHostMenuItems } from '@mobile/components/host/use-host-menu-items'; | ||
| import { HeaderIconButton } from '@mobile/components/shell/header-icon-button'; | ||
| import type { PrimaryAction } from '@mobile/components/shell/primary-action'; | ||
| import { usePrimaryAction } from '@mobile/components/shell/primary-action'; | ||
| import { VISIBLE_HEADER_OPTIONS } from '@mobile/components/shell/use-stack-screen-options'; | ||
| import { useTrailingActions } from '@mobile/components/shell/use-trailing-actions'; | ||
| import { useHostConnection } from '@mobile/runtime/host-connection'; | ||
| import { captureMobileProductEvent } from '@mobile/runtime/product-analytics'; | ||
| import { useWorkspaces } from '@mobile/runtime/use-workspaces'; | ||
|
|
@@ -49,24 +52,26 @@ export default function ThreadsRoute(): React.ReactNode { | |
| const connection = useHostConnection(); | ||
| const [sheetOpen, setSheetOpen] = useState(false); | ||
|
|
||
| const primaryAction: PrimaryAction | null = | ||
| connection?.status === 'ready' | ||
| ? { | ||
| sf: 'square.and.pencil', | ||
| icon: SquarePenIcon, | ||
| label: t('newThread'), | ||
| onPress: () => setSheetOpen(true), | ||
| } | ||
| : null; | ||
|
Comment on lines
+55
to
+63
|
||
| usePrimaryAction('threads', primaryAction); | ||
| const trailingActions = useTrailingActions(primaryAction); | ||
|
|
||
| return ( | ||
| <View className="flex-1 bg-background"> | ||
| <Stack.Screen | ||
| options={{ | ||
| headerShown: true, | ||
| headerLargeTitle: true, | ||
| ...VISIBLE_HEADER_OPTIONS, | ||
| title: t('title'), | ||
| unstable_headerLeftItems: () => hostMenuItems, | ||
| headerRight: | ||
| connection?.status === 'ready' | ||
| ? () => ( | ||
| <HeaderIconButton | ||
| icon={SquarePenIcon} | ||
| label={t('newThread')} | ||
| onPress={() => setSheetOpen(true)} | ||
| /> | ||
| ) | ||
| : undefined, | ||
| ...trailingActions, | ||
| }} | ||
| /> | ||
| <HostClientGate> | ||
|
|
@@ -77,7 +82,7 @@ export default function ThreadsRoute(): React.ReactNode { | |
| } | ||
|
|
||
| /** Threads inbox: sessions grouped by workspace (project) under collapsible headers, with the | ||
| * native search bar stacked under the large title. Empty workspace groups are hidden — the sheet | ||
| * native search bar stacked below the navigation bar. Empty workspace groups are hidden — the sheet | ||
| * is where they surface. */ | ||
| function ThreadsScreen({ | ||
| sheetOpen, | ||
|
|
@@ -151,12 +156,11 @@ function ThreadsScreen({ | |
|
|
||
| return ( | ||
| <> | ||
| {/* `stacked` keeps the field under the large title instead of collapsing into the iOS 26 | ||
| toolbar; the screen body is a SwiftUI host, so nothing here can drive hide-on-scroll. */} | ||
| {/* `stacked` keeps the field below the inline title instead of moving into the iOS 26 toolbar. */} | ||
| <Stack.SearchBar | ||
| placeholder={t('searchPlaceholder')} | ||
| placement="stacked" | ||
| hideWhenScrolling={false} | ||
| hideWhenScrolling | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This flip drops a documented constraint that still appears to hold, with nothing recording what invalidated it. The line it replaces was UIKit's If iOS 26 plus |
||
| hideNavigationBar={false} | ||
| autoCapitalize="none" | ||
| onChangeText={onSearchChange} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { AddHostScreen } from '@mobile/components/connect/add-host-screen'; | ||
|
|
||
| export default function AddHostRoute(): React.ReactNode { | ||
| return <AddHostScreen />; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
/connectis pushed from/sign-in, the chevron carries a back label — prefertapOn: { id: '...' }against that over a coordinate. If theminimalback-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.