Skip to content

Commit 08a6a46

Browse files
authored
fix(mobile): keep code reviewer custom instructions recoverable after an empty save (#9301)
* fix(mobile): confirm clearing custom instructions and keep the row reachable Saving an emptied custom-instructions field replaced the stored text with nothing, with no question, and the overview row that leads back into the editor only existed while text was stored. One accidental save removed both the text and the only way to type it again. The screen now asks before an empty draft replaces stored instructions, on the Save button and on the back guard's Save, and the overview row stays on every config with a not-set value. * fix(mobile): keep the back guard doc on the hook The new outcome type sat between useSettingsBackGuard's doc comment and the function, so the doc described the type. Move it above the comment. * refactor(mobile): drop the unused save mutation contract The editor no longer takes the save mutation as a prop; the screen owns the save, so the named contract had no consumer. * fix(mobile): pad row text blocks on the side facing their control The text block beside a switch kept a physical right padding, so in a right-to-left layout the block ran into the padding box and the design gate measured it as clipped on the code reviewer overview. Pad the end side instead, as the shared choice row already does, so the gap follows the writing direction. * fix(mobile): color the shared switch from the palette and unify the row copy The shared switch passed no colors, so Android resolved its track and thumb from the device theme: the on track came out as the framework accent (a teal that is in no palette) and the off thumb was near-white in dark mode, far brighter than the muted one in light mode. It now names palette colors for both states, so the on track is the primary fill and the off thumb is the ink that sits on it, in either theme. The overview row that opens the instructions editor used its own label, which had drifted from the screen it opens: the row and the deprecation note read "Custom Instructions" while the header reads "Custom instructions". The row now names the screen with the screen's own title key, the note follows it in sentence case, and the duplicate overview key is gone from every catalog. * fix(mobile): lift the dark off track and mark the deprecation note as a notice The shared switch takes one thumb colour for both states: white in light mode, the palette's near-black primary ink in dark mode. In dark mode the off track was the muted ink, so the whole control read as one dim shape against the card. The off track now takes the lighter secondary ink there, which keeps the thumb and the track edge visible; light mode keeps its track. The Custom instructions screen's deprecation line was muted body text and read as a field description. It now renders as a tinted warning notice with its own warning mark. * refactor(mobile): resolve the switch's dark track from the palette it renders with The shared switch read the platform appearance to pick its off track, which made every mounted suite that stubs the native module fail on the missing export. The palette it already renders with identifies the theme on its own, so the wrapper reads that instead and needs no second platform call. * test(mobile): drop the palette mocks the branch's own switch wrapper needed The wrapper this branch carried read the palette through useThemeColors, so its mounted suites mocked the hook. The merge of main's shared switch took that wrapper away; the shared switch resolves the palette itself and the suites pass unchanged without the mock. One suite also stayed under the file-length limit only without the extra block. * chore: trigger review * fix(web): review a merge-commit head whose content no review covers * fix(web): match merge-head coverage across the first-parent chain * fix(web): carry a finished review verdict onto a merge head
1 parent 5908fec commit 08a6a46

108 files changed

Lines changed: 1459 additions & 474 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎apps/mobile/src/app/(app)/code-reviewer/[scope]/[platform]/(edit)/instructions.mounted.test.tsx‎

Lines changed: 175 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,8 @@ const textInputs = vi.hoisted(() => ({
6363
const fieldHandle = vi.hoisted(() => ({ setSelection: vi.fn() }));
6464
const confirmDialog = vi.hoisted(() => ({
6565
current: null as {
66+
title?: string;
67+
message?: string;
6668
cancelLabel?: string;
6769
confirmLabel?: string;
6870
extraAction?: { label: string; onPress: () => void };
@@ -125,6 +127,8 @@ vi.mock('@/components/ui/button', () => ({ Button: 'Button' }));
125127
vi.mock('@/components/ui/sheet', () => ({ Sheet: 'Sheet' }));
126128
vi.mock('@/components/destructive-confirm-dialog', () => ({
127129
DestructiveConfirmDialog: (props: {
130+
title?: string;
131+
message?: string;
128132
cancelLabel?: string;
129133
confirmLabel?: string;
130134
extraAction?: { label: string; onPress: () => void };
@@ -139,6 +143,10 @@ vi.mock('@/components/destructive-confirm-dialog', () => ({
139143
}));
140144
vi.mock('@/components/ui/skeleton', () => ({ Skeleton: 'Skeleton' }));
141145
vi.mock('@/components/ui/text', () => ({ Text: 'Text' }));
146+
vi.mock('@/components/ui/icons', () => ({ AlertTriangle: 'AlertTriangle' }));
147+
vi.mock('@/lib/hooks/use-theme-colors', () => ({
148+
useThemeColors: () => ({ warn: '#f2b05f' }),
149+
}));
142150
vi.mock('@/components/invalid-route-state', () => ({ InvalidRouteState: 'InvalidRouteState' }));
143151
vi.mock('@/components/screen-header', () => ({
144152
ScreenHeader: (props: Record<string, unknown>): ReactNode => createElement('ScreenHeader', props),
@@ -212,6 +220,32 @@ function latestInput() {
212220
return input;
213221
}
214222

223+
// The editor's Save is the screen's: the button only asks for the save, so the
224+
// test presses the same last `Button` the editor renders.
225+
function pressSaveButton(renderer: TestRenderer.ReactTestRenderer) {
226+
const saveButton = renderer.root.findAll(node => String(node.type) === 'Button').at(-1);
227+
if (!saveButton) {
228+
throw new Error('save button not found');
229+
}
230+
const onPress = (saveButton.props as Record<string, unknown>).onPress;
231+
if (typeof onPress !== 'function') {
232+
throw new TypeError('save button onPress is not a function');
233+
}
234+
// typeof narrowing proves callable, not zero-arg; the screen's own Button
235+
// calls it the same way.
236+
(onPress as () => void)();
237+
}
238+
239+
// Runs `trigger` inside act and lets the screen's async save (mutation, then
240+
// navigation, or a pending confirmation) settle before the assertions.
241+
async function flush(trigger: () => void) {
242+
await act(async () => {
243+
trigger();
244+
await Promise.resolve();
245+
await Promise.resolve();
246+
});
247+
}
248+
215249
beforeEach(() => {
216250
config.data = null;
217251
config.isLoading = false;
@@ -414,52 +448,146 @@ describe('InstructionsRoute editor', () => {
414448
expect(navigation.dispatch).toHaveBeenCalledTimes(1);
415449
});
416450

417-
it('persists the trimmed draft from the save button and bypasses the guard on its own back', () => {
451+
it('persists the trimmed draft from the save button and bypasses the guard on its own back', async () => {
418452
config.data = { customInstructions: STORED };
419453
const renderer = mount();
420454

421-
const input = latestInput();
422455
act(() => {
423-
input.onChangeText(' rewritten draft\n');
456+
latestInput().onChangeText(' rewritten draft\n');
424457
});
425458

426-
const buttons = renderer.root.findAll(node => String(node.type) === 'Button');
427-
const saveButton = buttons.at(-1);
428-
if (!saveButton) {
429-
throw new Error('save button not found');
430-
}
431-
const saveProps = saveButton.props as Record<string, unknown>;
432-
const saveOnPress = saveProps.onPress;
433-
if (typeof saveOnPress !== 'function') {
434-
throw new TypeError('save button onPress is not a function');
435-
}
436-
// typeof narrowing proves callable, not zero-arg; the screen's own
437-
// Button calls it the same way.
438-
const pressSave = saveOnPress as () => void;
459+
await flush(() => {
460+
pressSaveButton(renderer);
461+
});
462+
expect(save.mutateAsync).toHaveBeenCalledWith({ customInstructions: 'rewritten draft' });
463+
expect(router.back).toHaveBeenCalledTimes(1);
464+
// The next navigation is the screen's own back: the guard must let it
465+
// through without a confirm, then re-arm for later edits.
439466
act(() => {
440-
pressSave();
467+
triggerPreventRemove();
441468
});
442-
expect(save.mutate).toHaveBeenCalledWith(
443-
{ customInstructions: 'rewritten draft' },
444-
expect.objectContaining({ onSuccess: expect.any(Function) })
445-
);
469+
expect(navigation.dispatch).toHaveBeenCalledTimes(1);
470+
expect(visibleConfirmationCount(renderer)).toBe(0);
471+
});
472+
473+
it('asks before an empty field removes the stored instructions, and keeps the editor on cancel', async () => {
474+
config.data = { customInstructions: STORED };
475+
const renderer = mount();
446476

447-
const options = save.mutate.mock.calls.at(-1)?.[1] as { onSuccess?: () => void } | undefined;
448-
const onSuccess = options?.onSuccess;
449-
if (!onSuccess) {
450-
throw new Error('save onSuccess not captured');
451-
}
452477
act(() => {
453-
onSuccess();
478+
latestInput().onChangeText('');
479+
});
480+
481+
await flush(() => {
482+
pressSaveButton(renderer);
454483
});
484+
expect(visibleConfirmationCount(renderer)).toBe(1);
485+
const removal = confirmDialog.current;
486+
expect(removal?.title).toBe(i18n.t('codeReviewer.instructions.clearTitle'));
487+
expect(removal?.message).toBe(i18n.t('codeReviewer.instructions.clearMessage'));
488+
expect(removal?.confirmLabel).toBe(i18n.t('common.remove'));
489+
expect(removal?.cancelLabel).toBe(i18n.t('common.keepEditing'));
490+
// The question is a yes/no about the removal, with no third choice.
491+
expect(removal?.extraAction).toBeUndefined();
492+
expect(save.mutateAsync).not.toHaveBeenCalled();
493+
expect(router.back).not.toHaveBeenCalled();
494+
495+
await flush(() => {
496+
removal?.onCancel();
497+
});
498+
expect(visibleConfirmationCount(renderer)).toBe(0);
499+
expect(save.mutateAsync).not.toHaveBeenCalled();
500+
expect(router.back).not.toHaveBeenCalled();
501+
// The edited draft is still unsaved, so the guard still holds the exit.
502+
expect(preventRemove.enabled).toBe(true);
503+
});
504+
505+
it('removes the stored instructions once the reader confirms it', async () => {
506+
config.data = { customInstructions: STORED };
507+
const renderer = mount();
508+
509+
act(() => {
510+
latestInput().onChangeText(' ');
511+
});
512+
513+
await flush(() => {
514+
pressSaveButton(renderer);
515+
});
516+
expect(save.mutateAsync).not.toHaveBeenCalled();
517+
518+
await flush(() => {
519+
confirmDialog.current?.onConfirm();
520+
});
521+
expect(save.mutateAsync).toHaveBeenCalledWith({ customInstructions: '' });
455522
expect(router.back).toHaveBeenCalledTimes(1);
456-
// The next navigation is the screen's own back: the guard must let it
457-
// through without a confirm, then re-arm for later edits.
523+
});
524+
525+
it('saves an empty field with no confirmation when nothing is stored', async () => {
526+
config.data = { customInstructions: '' };
527+
const renderer = mount();
528+
529+
await flush(() => {
530+
pressSaveButton(renderer);
531+
});
532+
expect(visibleConfirmationCount(renderer)).toBe(0);
533+
expect(save.mutateAsync).toHaveBeenCalledWith({ customInstructions: '' });
534+
expect(router.back).toHaveBeenCalledTimes(1);
535+
});
536+
537+
it('asks the same question from the back guard save and stays when it is declined', async () => {
538+
config.data = { customInstructions: STORED };
539+
const renderer = mount();
540+
541+
act(() => {
542+
latestInput().onChangeText('');
543+
});
458544
act(() => {
459545
triggerPreventRemove();
460546
});
547+
const saveChanges = confirmDialog.current?.extraAction;
548+
expect(saveChanges?.label).toBe(i18n.t('securityAgent.settingsSave.saveChanges'));
549+
550+
await flush(() => {
551+
saveChanges?.onPress();
552+
});
553+
// The guard's own sheet stays mounted while the removal question is
554+
// presented over it (the emulator shows one native sheet, the removal
555+
// one), so the save does not run straight through.
556+
expect(visibleConfirmationCount(renderer)).toBe(2);
557+
expect(confirmDialog.current?.title).toBe(i18n.t('codeReviewer.instructions.clearTitle'));
558+
expect(confirmDialog.current?.message).toBe(i18n.t('codeReviewer.instructions.clearMessage'));
559+
expect(save.mutateAsync).not.toHaveBeenCalled();
560+
561+
await flush(() => {
562+
confirmDialog.current?.onCancel();
563+
});
564+
// Declining puts the unsaved-changes sheet back in front: the leave is
565+
// dropped and the draft is still there to discard or save.
566+
expect(confirmDialog.current?.title).toBe(i18n.t('securityAgent.settingsSave.unsavedTitle'));
567+
expect(save.mutateAsync).not.toHaveBeenCalled();
568+
expect(navigation.dispatch).not.toHaveBeenCalled();
569+
expect(preventRemove.enabled).toBe(true);
570+
});
571+
572+
it('removes the stored instructions from the back guard save once it is confirmed', async () => {
573+
config.data = { customInstructions: STORED };
574+
mount();
575+
576+
act(() => {
577+
latestInput().onChangeText('');
578+
});
579+
act(() => {
580+
triggerPreventRemove();
581+
});
582+
await flush(() => {
583+
confirmDialog.current?.extraAction?.onPress();
584+
});
585+
await flush(() => {
586+
confirmDialog.current?.onConfirm();
587+
});
588+
589+
expect(save.mutateAsync).toHaveBeenCalledWith({ customInstructions: '' });
461590
expect(navigation.dispatch).toHaveBeenCalledTimes(1);
462-
expect(visibleConfirmationCount(renderer)).toBe(0);
463591
});
464592

465593
it('does not clobber an in-progress edit when the query refetches', async () => {
@@ -506,6 +634,23 @@ describe('InstructionsRoute editor', () => {
506634
expect(latestInput().accessibilityLabel).toBe(i18n.t('codeReviewer.instructions.title'));
507635
});
508636

637+
// As body-coloured copy the deprecation note read as a description of the
638+
// field under it. It is now a notice of its own: a tinted block with its
639+
// warning mark, in the warning ink rather than the muted one.
640+
it('renders the deprecation note as a warning notice of its own', () => {
641+
config.data = { customInstructions: STORED };
642+
const renderer = mount();
643+
644+
const noticeCopy = findByHostType(renderer, 'Text').filter(node => {
645+
const props = node.props as { className?: string };
646+
return props.className?.includes('text-warn') === true;
647+
});
648+
expect(noticeCopy).toHaveLength(1);
649+
const copy = noticeCopy[0]?.props as { children?: string } | undefined;
650+
expect(copy?.children).toBe(i18n.t('codeReviewer.instructions.deprecation'));
651+
expect(findByHostType(renderer, 'AlertTriangle')).toHaveLength(1);
652+
});
653+
509654
it('reads the editor name from the active catalog', async () => {
510655
await act(async () => {
511656
await i18n.changeLanguage('ar');

0 commit comments

Comments
 (0)