Skip to content

Commit d4e945f

Browse files
authored
fix(mobile): keep a Security Agent screen's Back inside its stack (#9421)
* fix(mobile): keep a Security Agent screen's Back inside its stack A link that builds a fresh destination can make a `[scope]` screen the Security Agent group's only route: the group's anchor is honoured by `withAnchor` navigations and the relaunch repair, not by every delivery of a link. That screen has nothing to pop, so its Back — the header control, the Android gesture, the hardware key — resolves on the root stack and pops the whole group. The reader lands outside the feature, and a group whose segment is rebuilt without its params paints the terminal "page not found" state whose Zurück replaces to the scope picker. The group's scene layout now puts the dashboard directly beneath a screen whose Stack lacks it. The existing routes keep their keys, so the screen the reader is on keeps its identity and its state, and every leave pops inside the group again; leaving the dashboard itself still leaves the feature. The insertion is driven by the Stack's state, so it covers each way a screen can arrive without its parent, and it acts once per state object because every screen of the group subscribes the same layout. The dev-only fixture opens a Security Agent screen as its group's only route, which is the state that makes the defect reachable by hand. The decision and the hook are covered by a unit test and a mounted test. * test(mobile): cast the harness scene props through unknown * fix(mobile): settle the fixture route and the detail Back note Anchor the fixture path pattern at the end like every other fixture parser, send a release build that reaches the fixture route to the app instead of an empty screen, state where a linked finding detail now returns, and cover the matcher and the listener cleanup. * fix(mobile): hold a finding card's frame while the list loads The findings list's loading placeholder drew three plain bars, 84 points tall at the list's 21 point inset and just under the header. A finding card is the row's own frame: 143.6 points at the list's 24 point inset, starting one header band lower. The cards therefore landed as a jump. The placeholder now repeats the card's frame and its line boxes, so a card replaces it in place, and it takes the list's own content insets and header band instead of a second copy of them. The load-more placeholder uses the same row. * docs(mobile): state the placeholder's height variance A finding card is content-sized: its quick action is a button on the rows that offer one, a one-line label on some, and absent on others. The placeholder reserves the button shape, which is what the list shows for a finding that offers an action, and now names the three variants and the height measured for the actionless one.
1 parent cea7877 commit d4e945f

18 files changed

Lines changed: 703 additions & 18 deletions

‎apps/mobile/src/app/(app)/security-agent/[scope]/_layout.tsx‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,35 @@
11
import { type Href, Stack, useLocalSearchParams } from 'expo-router';
2+
import { useCallback } from 'react';
23
import { appScreenLayout } from '@/components/app-screen-layout';
34

5+
import { type ScreenLayoutProps } from '@/components/centered-state-surface';
46
import { InvalidRouteState } from '@/components/invalid-route-state';
57
import { SecurityAgentCommandObserver } from '@/components/security-agent/security-agent-command-observer';
68
import { useFormSheetScreenOptions } from '@/lib/form-sheet';
9+
import { useRepairSecurityAgentScopeStack } from '@/lib/hooks/use-repair-security-agent-scope-stack';
710
import { parseSecurityScope } from '@/lib/security-agent';
811

912
export const unstable_settings = {
1013
anchor: 'index',
1114
};
1215

16+
/**
17+
* One scene of the group's Stack: repair the Stack if the screen arrived
18+
* without the dashboard beneath it, then render the app's shared scene layout.
19+
*
20+
* A scene layout renders outside the route's NavigationProvider, so the repair
21+
* is handed the screen's navigation instead of reading one, and the segment
22+
* comes from the layout that validated it rather than from a second read of
23+
* the route's own params.
24+
*/
25+
function SecurityAgentSceneLayout({
26+
scope,
27+
...props
28+
}: ScreenLayoutProps & Readonly<{ scope: string | null }>) {
29+
useRepairSecurityAgentScopeStack(props.navigation, scope);
30+
return appScreenLayout(props);
31+
}
32+
1333
// Mounts exactly one command observer per scope alongside a headerless Stack,
1434
// so it stays mounted across Dashboard/Findings/Settings navigation without
1535
// ever running twice for the same scope. Also the single validation point
@@ -20,6 +40,14 @@ export default function SecurityAgentScopeLayout() {
2040
const { scope: rawScope } = useLocalSearchParams<{ scope: string }>();
2141
const scope = parseSecurityScope(rawScope);
2242
const sheetOptions = useFormSheetScreenOptions();
43+
// The group's dashboard must sit beneath every screen of this Stack: a
44+
// link that opens a screen as a fresh destination makes it the Stack's only
45+
// route, and a Back press then leaves the whole Security Agent group instead
46+
// of returning to the dashboard.
47+
const screenLayout = useCallback(
48+
(props: ScreenLayoutProps) => <SecurityAgentSceneLayout {...props} scope={scope} />,
49+
[scope]
50+
);
2351

2452
if (!scope) {
2553
return <InvalidRouteState backTo={'/(app)/security-agent' as Href} />;
@@ -28,7 +56,13 @@ export default function SecurityAgentScopeLayout() {
2856
return (
2957
<>
3058
<SecurityAgentCommandObserver scope={scope} />
31-
<Stack screenLayout={appScreenLayout} screenOptions={{ headerShown: false }}>
59+
<Stack
60+
screenLayout={screenLayout}
61+
screenOptions={{ headerShown: false }}
62+
// Registered so the dashboard is declared in this Stack's route names
63+
// even when nothing has pushed it yet: the repair inserts a route by
64+
// that name.
65+
>
3266
<Stack.Screen name="index" />
3367
<Stack.Screen name="dismiss/[id]" options={sheetOptions} />
3468
<Stack.Screen name="filter" options={sheetOptions} />

‎apps/mobile/src/app/+native-intent.tsx‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { redirectSystemPath as mapWebPath } from '@/lib/deep-link-handler';
66
import { handleGlanceableFixturePath } from '@/lib/glanceable/fixture-harness';
77
import { handleHomeFixturePath } from '@/lib/home-fixture';
88
import { modelPickerFixtureRoute } from '@/lib/model-picker-fixture';
9+
import { securityAgentSingleRouteFixtureRoute } from '@/lib/security-agent-single-route-fixture';
910

1011
// Composes the native-intent concerns in order: the share-extension check
1112
// returns early before web-path mapping (a share URL is never a web route), then
@@ -38,6 +39,12 @@ export function redirectSystemPath({ path, initial }: { path: string; initial: b
3839
return null;
3940
}
4041
if (__DEV__) {
42+
// Dev-only: open a Security Agent screen as its group's only route
43+
// (`kiloapp:///dev/security-agent-single-route-fixture/<scope>/<route…>`).
44+
const singleRouteFixture = securityAgentSingleRouteFixtureRoute(path);
45+
if (singleRouteFixture) {
46+
return singleRouteFixture;
47+
}
4148
const reviewEntryFixture = /(?:^|\/)dev\/pr-review-entry-fixture\/?(\?.*)?$/.exec(path);
4249
if (reviewEntryFixture) {
4350
return `/(app)/pr-review-entry-fixture${reviewEntryFixture[1] ?? ''}`;
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
import { type Href, useLocalSearchParams, useRouter } from 'expo-router';
2+
import { useEffect } from 'react';
3+
4+
import { parseParam } from '@/lib/route-params';
5+
import { getSecurityAgentPath } from '@/lib/security-agent';
6+
7+
/**
8+
* Dev-only fixture: open a Security Agent screen the way a link that builds a
9+
* fresh destination does, so that screen is the group's only route.
10+
*
11+
* Reached only through `+native-intent.tsx`, behind `__DEV__`:
12+
* `kiloapp:///dev/security-agent-single-route-fixture/personal/settings/notifications`
13+
*/
14+
export default function SecurityAgentSingleRouteFixtureRoute() {
15+
const router = useRouter();
16+
const params = useLocalSearchParams<{ scope?: string; suffix?: string }>();
17+
const scope = parseParam(params.scope);
18+
const suffix = parseParam(params.suffix) ?? '';
19+
20+
useEffect(() => {
21+
if (!__DEV__) {
22+
// A release build registers the route but must never run the fixture, and
23+
// a screen that renders nothing would strand whoever opened it: send the
24+
// reader to the app instead.
25+
router.replace('/(app)' as Href);
26+
return;
27+
}
28+
if (scope === null) {
29+
return;
30+
}
31+
// `replace`, not `push`: a link that builds its destination swaps the
32+
// screen it lands on for the target, and the target then has nothing
33+
// beneath it in this Stack.
34+
router.replace(getSecurityAgentPath(scope, suffix) as Href);
35+
}, [router, scope, suffix]);
36+
37+
return null;
38+
}

‎apps/mobile/src/components/app-screen-layout-nested-stacks.mounted.test.tsx‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@ const scenes = vi.hoisted(() => {
6060
navigation: {
6161
isFocused: () => boolean;
6262
addListener: (event: string, listener: () => void) => () => void;
63+
getState: () => { index: number; routes: { key: string; name: string }[] };
64+
reset: () => void;
6365
};
6466
setFocused: (next: boolean) => void;
6567
listenerCount: () => number;
@@ -76,6 +78,10 @@ const scenes = vi.hoisted(() => {
7678
listeners.set(event, registered);
7779
return () => registered.delete(listener);
7880
},
81+
// The gate is this suite's subject; the Security Agent layout's own
82+
// repair reads a Stack that already has its dashboard, so it no-ops.
83+
getState: () => ({ index: 0, routes: [{ key: 'dashboard', name: 'index' }] }),
84+
reset: () => undefined,
7985
},
8086
setFocused(next: boolean) {
8187
focused = next;

‎apps/mobile/src/components/app-unlock-screen.test-helpers.tsx‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -462,9 +462,23 @@ export function expectFeedback(root: ReactTestInstance, message: string, copies:
462462
);
463463
}
464464

465+
/**
466+
* The scene navigation the layouts' `screenLayout` receives. The unlock
467+
* surfaces under test read focus and state events; a layout that repairs the
468+
* Stack it belongs to (the Security Agent group's dashboard) reads a Stack
469+
* that already has its parent, so its repair no-ops here.
470+
*/
471+
const SCENE_NAVIGATION = {
472+
isFocused: () => true,
473+
addListener: () => () => undefined,
474+
getState: () => ({ index: 0, routes: [{ key: 'index', name: 'index' }] }),
475+
reset: () => undefined,
476+
};
477+
465478
export function unlockScene(children: ReactElement, layout = appUnlockScreenLayout) {
466-
const props = { children };
467-
return layout(props as Parameters<typeof appUnlockScreenLayout>[0]);
479+
const props = { children, navigation: SCENE_NAVIGATION };
480+
// The harness supplies the props these layouts read, not the whole scene.
481+
return layout(props as unknown as Parameters<typeof appUnlockScreenLayout>[0]);
468482
}
469483

470484
export function nestedUnlockScenes(children: ReactElement) {

‎apps/mobile/src/components/centered-state-surface.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,10 @@ type ScreenLayoutProps = Omit<
4343
> & {
4444
navigation: Pick<
4545
NativeStackNavigationProp<ParamListBase>,
46-
'setOptions' | 'isFocused' | 'addListener'
46+
// `getState`/`reset` let a scene layout repair the Stack it belongs to —
47+
// a screen that a fresh destination opened on its own has no parent to
48+
// pop to (see use-repair-security-agent-scope-stack).
49+
'setOptions' | 'isFocused' | 'addListener' | 'getState' | 'reset'
4750
>;
4851
};
4952

‎apps/mobile/src/components/security-agent/finding-list-screen.mounted.test.tsx‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,13 @@ import { FlashList } from '@shopify/flash-list';
77
import { act, type ComponentProps, createElement, type ReactElement, type ReactNode } from 'react';
88
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
99

10-
import { Skeleton } from '@/components/ui/skeleton';
1110
import { Text } from '@/components/ui/text';
1211
import { EmptyState } from '@/components/empty-state';
1312
import { QueryError } from '@/components/query-error';
1413
import { ScreenHeader } from '@/components/screen-header';
1514
import { renderWithProviders } from '@/test/render-with-providers';
1615
import { FindingListScreen } from './finding-list-screen';
16+
import { FindingRowSkeleton } from './finding-row-skeleton';
1717

1818
type FindingFixture = {
1919
id: string;
@@ -100,7 +100,10 @@ vi.mock('@/components/picker-sheet', () => ({ PickerSheet: 'PickerSheet' }));
100100
vi.mock('@/components/security-agent/finding-filter-modal', () => ({
101101
FindingFilterModal: 'FindingFilterModal',
102102
}));
103-
vi.mock('@/components/security-agent/finding-row', () => ({ FindingRow: 'FindingRow' }));
103+
vi.mock('@/components/security-agent/finding-row', () => ({
104+
FindingRow: 'FindingRow',
105+
FINDING_ROW_FRAME_CLASS: 'frame',
106+
}));
104107
vi.mock('@/components/tab-screen', () => ({ useTabBarBottomPadding: () => 0 }));
105108
vi.mock('@/components/ui/button', () => ({ Button: 'Button' }));
106109
vi.mock('@/components/ui/skeleton', () => ({ Skeleton: 'Skeleton' }));
@@ -257,7 +260,7 @@ describe('Security Agent list surfaces', () => {
257260
findings.isFetching = true;
258261
findings.fetchStatus = 'fetching';
259262
const root = await mount();
260-
expect(root.findAllByType(Skeleton)).toHaveLength(3);
263+
expect(root.findAllByType(FindingRowSkeleton)).toHaveLength(3);
261264
expect(root.findAllByType(EmptyState)).toHaveLength(0);
262265
expect(root.findAllByType(QueryError)).toHaveLength(0);
263266
});
@@ -272,7 +275,7 @@ describe('Security Agent list surfaces', () => {
272275
const root = await mount();
273276

274277
expect(root.findAllByType(EmptyState)).toHaveLength(0);
275-
expect(root.findAllByType(Skeleton)).toHaveLength(0);
278+
expect(root.findAllByType(FindingRowSkeleton)).toHaveLength(0);
276279
const error = root.findByType(QueryError);
277280
expect(error.props.variant).toBe('offline');
278281
expect(error.props.message).toBe('securityAgent.findingList.couldNotLoad');
@@ -287,7 +290,7 @@ describe('Security Agent list surfaces', () => {
287290
findings.fetchStatus = 'paused';
288291
const root = await mount();
289292

290-
expect(root.findAllByType(Skeleton)).toHaveLength(3);
293+
expect(root.findAllByType(FindingRowSkeleton)).toHaveLength(3);
291294
expect(root.findAllByType(EmptyState)).toHaveLength(0);
292295
expect(root.findAllByType(QueryError)).toHaveLength(0);
293296
});

‎apps/mobile/src/components/security-agent/finding-list-screen.tsx‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,13 @@ import { QueryError } from '@/components/query-error';
2222
import { ScreenHeader } from '@/components/screen-header';
2323
import { AnalysisCapacityNotice } from '@/components/security-agent/analysis-capacity-notice';
2424
import { FindingRow } from '@/components/security-agent/finding-row';
25+
import {
26+
FindingListSkeletonRows,
27+
FindingRowSkeleton,
28+
} from '@/components/security-agent/finding-row-skeleton';
2529
import { useTabBarBottomPadding } from '@/components/tab-screen';
2630
import { Button } from '@/components/ui/button';
2731
import { IconButton } from '@/components/ui/icon-button';
28-
import { Skeleton } from '@/components/ui/skeleton';
2932
import { Text } from '@/components/ui/text';
3033
import { STATE_ACTION_CLASS } from '@/lib/a11y/tap-target';
3134
import {
@@ -53,7 +56,7 @@ function FindingsListFooter({
5356
}: Readonly<{ loading: boolean; error: boolean; onRetry: () => void }>) {
5457
const { t } = useTranslation();
5558
if (loading) {
56-
return <Skeleton className="h-24 w-full rounded-lg" />;
59+
return <FindingRowSkeleton index={0} />;
5760
}
5861
if (error) {
5962
return (
@@ -213,10 +216,13 @@ export function FindingListScreen({ scope, routeParams }: Readonly<FindingListSc
213216
) : null}
214217

215218
{body.status === 'loading' && (
216-
<View className="flex-1 gap-3 px-6 pt-4">
217-
<Skeleton className="h-24 w-full rounded-lg" />
218-
<Skeleton className="h-24 w-full rounded-lg" />
219-
<Skeleton className="h-24 w-full rounded-lg" />
219+
<View style={listStyle}>
220+
<View style={listContentContainerStyle}>
221+
{/* The loaded list's `ListHeaderComponent` band, so the first card
222+
lands where the first placeholder row stood. */}
223+
<View className="pb-3" />
224+
<FindingListSkeletonRows />
225+
</View>
220226
</View>
221227
)}
222228

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
import { View } from 'react-native';
2+
3+
import { FINDING_ROW_FRAME_CLASS } from '@/components/security-agent/finding-row';
4+
import { Skeleton } from '@/components/ui/skeleton';
5+
import { cn } from '@/lib/utils';
6+
7+
const SKELETON_ROW_COUNT = 3;
8+
9+
/** Title bar widths, varied per row so the list does not read as a grid. */
10+
const TITLE_WIDTHS = ['w-3/4', 'w-2/3', 'w-1/2'] as const;
11+
const REPO_WIDTHS = ['w-1/3', 'w-2/5', 'w-1/4'] as const;
12+
13+
/**
14+
* The line boxes a finding card gives its text at the app's 14pt rem: the 11px
15+
* severity label, a one-line `text-sm` title, the `text-xs` repository, the meta
16+
* row beside it behind `pt-0.5`, and the quick action behind `mt-1`, which is the
17+
* card's button. Each box holds a bar of its own, so the placeholder stands in
18+
* the card's frame and the cards replace it in place.
19+
*
20+
* A card is content-sized, so the frame covers the shape the list shows for a
21+
* finding that offers an action: 143.6 points measured against the card's own
22+
* 144.0. Three variants differ, and neither is reserved because a placeholder
23+
* cannot stand for all of them: a card whose quick action is the one-line
24+
* `text-xs` label is shorter by the button's height, a card with no action
25+
* measured 99.0 points, and a title that wraps to its second line adds one
26+
* `text-sm` box while a meta row that wraps adds one `text-xs` box.
27+
*/
28+
const SEVERITY_LINE_CLASS = 'h-[15px] justify-center';
29+
const TITLE_LINE_CLASS = 'h-[17.5px] justify-center';
30+
const REPO_LINE_CLASS = 'h-[14px] justify-center';
31+
const META_LINE_CLASS = 'h-[14px] flex-row items-center gap-3';
32+
const ACTION_CLASS = 'mt-1';
33+
34+
/**
35+
* One finding card's placeholder. `index` only varies the bar widths; every
36+
* row reserves the same card frame, so a three-row placeholder stands exactly
37+
* where three cards will.
38+
*/
39+
export function FindingRowSkeleton({ index }: Readonly<{ index: number }>) {
40+
const variant = index % TITLE_WIDTHS.length;
41+
return (
42+
<View testID="finding-row-skeleton" className={FINDING_ROW_FRAME_CLASS}>
43+
<View className="gap-1.5">
44+
<View className={SEVERITY_LINE_CLASS}>
45+
<Skeleton className="h-2.5 w-16 rounded-full" />
46+
</View>
47+
<View className={TITLE_LINE_CLASS}>
48+
<Skeleton className={cn('h-3.5 rounded-full', TITLE_WIDTHS[variant])} />
49+
</View>
50+
<View className={REPO_LINE_CLASS}>
51+
<Skeleton className={cn('h-2.5 rounded-full', REPO_WIDTHS[variant])} />
52+
</View>
53+
<View className="pt-0.5">
54+
<View className={META_LINE_CLASS}>
55+
<Skeleton className="h-2.5 w-20 rounded-full" />
56+
<Skeleton className="h-2.5 w-16 rounded-full" />
57+
</View>
58+
</View>
59+
</View>
60+
<View className={ACTION_CLASS}>
61+
<Skeleton className="h-[36px] w-24 rounded-md" />
62+
</View>
63+
</View>
64+
);
65+
}
66+
67+
/**
68+
* The placeholder rows of a findings list, separated by the same `h-3` band the
69+
* loaded list's item separator paints, so a row keeps its place from the first
70+
* paint to the loaded card.
71+
*/
72+
export function FindingListSkeletonRows() {
73+
return (
74+
<>
75+
{Array.from({ length: SKELETON_ROW_COUNT }, (_, index) => (
76+
<View key={index} className={index === 0 ? undefined : 'pt-3'}>
77+
<FindingRowSkeleton index={index} />
78+
</View>
79+
))}
80+
</>
81+
);
82+
}

‎apps/mobile/src/components/security-agent/finding-row.tsx‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,13 @@ import { openFindingLink } from '@/lib/security-agent-link';
3434
import { type SecurityAnalysisCapacityState } from '@/lib/security-analysis-capacity';
3535
import { cn } from '@/lib/utils';
3636

37+
/**
38+
* A finding card's own frame: the padding and the line rhythm the loading
39+
* placeholder in `finding-row-skeleton.tsx` repeats, so the cards replace the
40+
* placeholders in place.
41+
*/
42+
export const FINDING_ROW_FRAME_CLASS = 'gap-1.5 rounded-lg bg-secondary p-3';
43+
3744
const SEVERITY_KEYS = {
3845
critical: 'common.critical',
3946
high: 'securityAgent.sla.high',
@@ -224,7 +231,7 @@ export function FindingRow({
224231
});
225232

226233
return (
227-
<View className="gap-1.5 rounded-lg bg-secondary p-3">
234+
<View className={FINDING_ROW_FRAME_CLASS}>
228235
<Pressable
229236
className="gap-1.5 active:opacity-70"
230237
accessibilityRole="button"

0 commit comments

Comments
 (0)