Skip to content

Commit cbfce91

Browse files
authored
fix(mobile): show a destructive confirm when discarding a draft on Android (#6512)
* fix(mobile): show a destructive confirm when discarding a draft on Android #6512 * docs(mobile): name the cross-platform discard confirm after the main merge The merged main removed the in-app dialog's sign-out caller (#6397 reverted that surface to the themed native alert), so the dialog and guard comments now describe the one remaining caller instead of a stale sign-out reference. * test(mobile): drop the native-alert pins the discard confirm no longer has main added two source-parity guards that hold use-new-session-discard-guard.ts to one shared Alert.alert call site: that call site is gone, because the guard now mounts the in-app DestructiveConfirmDialog so Android shows the destructive red fill. Delete those two clauses (and the now-unused source reads) and keep every assertion about the plugins themselves, whose other Alert.alert callers remain. * docs(mobile): name the native alert actions, not the ported discard confirm The button case now applies to the alerts still rendered natively.
1 parent 600095b commit cbfce91

8 files changed

Lines changed: 558 additions & 141 deletions

‎apps/mobile/src/app/(app)/agent-chat/__tests__/use-new-session-discard-guard.mounted.test.tsx‎

Lines changed: 171 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { createElement, type RefObject } from 'react';
1+
import { createElement, Fragment, type RefObject } from 'react';
22
import { act, TestRenderer } from '@/test/renderer';
33
import { beforeEach, describe, expect, it, vi } from 'vitest';
44

@@ -7,6 +7,13 @@ import { useNewSessionDiscardGuard } from '@/components/agents/use-new-session-d
77
const alertMock = vi.hoisted(() => vi.fn());
88
const dispatchMock = vi.hoisted(() => vi.fn());
99
const toastErrorMock = vi.hoisted(() => vi.fn());
10+
// Mutable so a single file proves the confirm is one cross-platform
11+
// implementation: the same in-app dialog opens for both platform values, and
12+
// the native alert is never reached on either.
13+
const platform = vi.hoisted(() => ({ os: 'ios' as 'ios' | 'android' }));
14+
15+
/** Both platform values, so every behaviour below is proven for iOS and Android. */
16+
const PLATFORMS = ['ios', 'android'] as const;
1017

1118
type Action = { type: string };
1219

@@ -22,8 +29,37 @@ const usePreventRemoveMock = vi.hoisted(() =>
2229
})
2330
);
2431

32+
// The dialog the hook returns is rendered for real, so its primitives and the
33+
// UI components it composes are stubbed the same way the dialog's own mounted
34+
// test stubs them. `Alert` stays mocked only so a regression that reaches for
35+
// the native confirm again fails here.
2536
vi.mock('react-native', () => ({
2637
Alert: { alert: alertMock },
38+
I18nManager: { isRTL: false },
39+
Modal: 'Modal',
40+
Platform: {
41+
get OS() {
42+
return platform.os;
43+
},
44+
},
45+
Pressable: 'Pressable',
46+
Text: 'Text',
47+
View: 'View',
48+
}));
49+
50+
vi.mock('@rn-primitives/slot', () => ({ Text: 'Slot.Text' }));
51+
52+
vi.mock('@/components/ui/activity-indicator', () => ({
53+
ActivityIndicator: 'ActivityIndicator',
54+
}));
55+
56+
vi.mock('@/lib/hooks/use-theme-colors', () => ({
57+
useThemeColors: () => ({
58+
destructiveForeground: '#FFFFFF',
59+
foreground: '#1A1A10',
60+
primary: '#00BAA9',
61+
primaryForeground: '#FFFFFF',
62+
}),
2763
}));
2864

2965
vi.mock('sonner-native', () => ({
@@ -38,8 +74,6 @@ vi.mock('@/lib/navigation/prevent-remove', () => ({
3874
usePreventRemove: usePreventRemoveMock,
3975
}));
4076

41-
type AlertButton = { text: string; style?: string; onPress?: () => void };
42-
4377
function GuardHarness({
4478
dirty,
4579
hasUnclaimedAttachments = false,
@@ -51,13 +85,15 @@ function GuardHarness({
5185
onDiscard: () => Promise<void>;
5286
skipRef: RefObject<boolean>;
5387
}) {
54-
useNewSessionDiscardGuard({
88+
const { discardConfirm } = useNewSessionDiscardGuard({
5589
dirty,
5690
hasUnclaimedAttachments,
5791
onDiscard,
5892
skipNextGuardRef: skipRef,
5993
});
60-
return null;
94+
// The screen body renders `discardConfirm` once in its tree; the harness does
95+
// the same so the returned node is exercised, not just its props.
96+
return createElement(Fragment, null, discardConfirm);
6197
}
6298

6399
async function noOpDiscard(): Promise<void> {
@@ -101,8 +137,42 @@ function triggerPreventRemove(): Action {
101137
return action;
102138
}
103139

104-
function lastAlertButtons(): AlertButton[] | undefined {
105-
return alertMock.mock.calls.at(-1)?.[2] as AlertButton[] | undefined;
140+
/** Triggers a leave inside `act` so the confirm's state update flushes. */
141+
function triggerPreventRemoveInAct(): Action {
142+
const holder: { action: Action } = { action: { type: 'GO_BACK' } };
143+
act(() => {
144+
holder.action = triggerPreventRemove();
145+
});
146+
return holder.action;
147+
}
148+
149+
function isType(node: TestRenderer.ReactTestInstance, type: string): boolean {
150+
return typeof node.type === 'string' && node.type === type;
151+
}
152+
153+
function classNameOf(node: TestRenderer.ReactTestInstance): string {
154+
return typeof node.props.className === 'string' ? node.props.className : '';
155+
}
156+
157+
/** Every string rendered through a `Text` node, in tree order. */
158+
function renderedLabels(renderer: TestRenderer.ReactTestRenderer): string[] {
159+
return renderer.root
160+
.findAll(node => isType(node, 'Text'))
161+
.flatMap(node => node.children.filter((child): child is string => typeof child === 'string'));
162+
}
163+
164+
/**
165+
* Presses the dialog control whose button variant carries `token` (the
166+
* destructive fill or the neutral outline), by the same class-based lookup the
167+
* dialog's own mounted test uses.
168+
*/
169+
function pressDialogButton(renderer: TestRenderer.ReactTestRenderer, token: string) {
170+
const node = renderer.root.find(
171+
candidate => isType(candidate, 'Pressable') && classNameOf(candidate).includes(token)
172+
);
173+
act(() => {
174+
(node.props as { onPress?: () => void }).onPress?.();
175+
});
106176
}
107177

108178
async function flushMicrotasks() {
@@ -115,6 +185,7 @@ async function flushMicrotasks() {
115185

116186
describe('useNewSessionDiscardGuard', () => {
117187
beforeEach(() => {
188+
platform.os = 'ios';
118189
alertMock.mockReset();
119190
dispatchMock.mockReset();
120191
toastErrorMock.mockReset();
@@ -123,35 +194,57 @@ describe('useNewSessionDiscardGuard', () => {
123194
usePreventRemoveHolder.callback = undefined;
124195
});
125196

126-
it('shows the confirm when the prompt is non-empty', () => {
197+
it.each(PLATFORMS)('guards the leave while the prompt is non-empty (%s)', os => {
198+
platform.os = os;
127199
const { renderer } = mountGuard(true, noOpDiscard);
128200

129201
expect(usePreventRemoveMock).toHaveBeenCalledTimes(1);
130202
expect(usePreventRemoveMock.mock.calls[0]?.[0]).toBe(true);
131203

132-
triggerPreventRemove();
133-
expect(alertMock).toHaveBeenCalledTimes(1);
134-
expect(alertMock.mock.calls[0]?.[0]).toBe('Discard draft?');
135-
expect(lastAlertButtons()?.map(button => button.text)).toEqual(['Keep editing', 'Discard']);
204+
act(() => {
205+
renderer.unmount();
206+
});
207+
});
208+
209+
// The requested behaviour: one implementation for both platforms. Neither
210+
// platform falls back to the native `Alert.alert`; both open the in-app
211+
// destructive confirm, which paints the discard choice with the red fill.
212+
it.each(PLATFORMS)('opens the in-app destructive confirm on %s', os => {
213+
platform.os = os;
214+
const { renderer } = mountGuard(true, noOpDiscard);
215+
216+
triggerPreventRemoveInAct();
217+
218+
expect(alertMock).not.toHaveBeenCalled();
219+
expect(renderedLabels(renderer)).toEqual(
220+
expect.arrayContaining(['Discard draft?', 'Discard', 'Keep editing'])
221+
);
222+
// The destructive choice carries the red fill, the safe one a neutral outline.
223+
expect(
224+
renderer.root.findAll(
225+
node => isType(node, 'Pressable') && classNameOf(node).includes('bg-destructive')
226+
)
227+
).toHaveLength(1);
136228

137229
act(() => {
138230
renderer.unmount();
139231
});
140232
});
141233

142-
it('shows the upload-specific confirm for an attachment-only dirty state', () => {
234+
it.each(PLATFORMS)('shows the upload-specific confirm for an attachment-only state (%s)', os => {
235+
platform.os = os;
143236
const { renderer } = mountGuardWithAttachments();
144237

145238
// An unsent upload makes the screen dirty even with an empty prompt, so
146239
// the leave is still intercepted.
147240
expect(usePreventRemoveMock).toHaveBeenCalledTimes(1);
148241
expect(usePreventRemoveMock.mock.calls[0]?.[0]).toBe(true);
149242

150-
triggerPreventRemove();
151-
expect(alertMock).toHaveBeenCalledTimes(1);
152-
expect(alertMock.mock.calls[0]?.[0]).toBe('Discard draft?');
243+
triggerPreventRemoveInAct();
244+
245+
expect(alertMock).not.toHaveBeenCalled();
153246
// The copy names the unclaimed uploads instead of the prompt-only message.
154-
expect(alertMock.mock.calls[0]?.[1]).toBe(
247+
expect(renderedLabels(renderer)).toContain(
155248
'Your prompt and any unclaimed uploads will be deleted.'
156249
);
157250

@@ -160,7 +253,7 @@ describe('useNewSessionDiscardGuard', () => {
160253
});
161254
});
162255

163-
it('leaves with no alert when the prompt is empty', () => {
256+
it('leaves with no confirm when the prompt is empty', () => {
164257
const { renderer } = mountGuard(false, noOpDiscard);
165258

166259
expect(usePreventRemoveMock).toHaveBeenCalledTimes(1);
@@ -170,6 +263,7 @@ describe('useNewSessionDiscardGuard', () => {
170263
// guard delegates the decision to the hook and nothing else fires.
171264
expect(alertMock).not.toHaveBeenCalled();
172265
expect(dispatchMock).not.toHaveBeenCalled();
266+
expect(renderedLabels(renderer)).not.toContain('Discard draft?');
173267

174268
act(() => {
175269
renderer.unmount();
@@ -188,6 +282,7 @@ describe('useNewSessionDiscardGuard', () => {
188282
expect(dispatchMock).toHaveBeenCalledTimes(1);
189283
expect(dispatchMock).toHaveBeenCalledWith(action);
190284
expect(alertMock).not.toHaveBeenCalled();
285+
expect(renderedLabels(renderer)).not.toContain('Discard draft?');
191286
// The bypass is one-shot: consumed on the removal it armed.
192287
expect(skipRef.current).toBe(false);
193288

@@ -209,6 +304,7 @@ describe('useNewSessionDiscardGuard', () => {
209304
usePreventRemoveHolder.callback?.({ data: { action } });
210305

211306
expect(alertMock).not.toHaveBeenCalled();
307+
expect(renderedLabels(renderer)).not.toContain('Discard draft?');
212308
expect(dispatchMock).toHaveBeenCalledTimes(1);
213309
expect(dispatchMock).toHaveBeenCalledWith(action);
214310

@@ -218,73 +314,80 @@ describe('useNewSessionDiscardGuard', () => {
218314
}
219315
);
220316

221-
it('Discard runs onDiscard before dispatching the captured action', async () => {
222-
const order: string[] = [];
223-
dispatchMock.mockImplementation(() => {
224-
order.push('dispatch');
225-
});
226-
const onDiscard = vi.fn(async () => {
227-
await Promise.resolve();
228-
order.push('discard');
229-
});
230-
const { renderer } = mountGuard(true, onDiscard);
231-
triggerPreventRemove();
317+
it.each(PLATFORMS)(
318+
'runs onDiscard before dispatching when Discard is accepted (%s)',
319+
async os => {
320+
platform.os = os;
321+
const order: string[] = [];
322+
dispatchMock.mockImplementation(() => {
323+
order.push('dispatch');
324+
});
325+
const onDiscard = vi.fn(async () => {
326+
await Promise.resolve();
327+
order.push('discard');
328+
});
329+
const { renderer } = mountGuard(true, onDiscard);
232330

233-
const discard = lastAlertButtons()?.find(button => button.text === 'Discard');
234-
expect(discard?.onPress).toBeDefined();
331+
const action = triggerPreventRemoveInAct();
332+
pressDialogButton(renderer, 'bg-destructive');
235333

236-
act(() => {
237-
discard?.onPress?.();
238-
});
239-
await flushMicrotasks();
334+
// Accepting closes the confirm right away; the clear runs behind it.
335+
expect(renderedLabels(renderer)).not.toContain('Discard draft?');
240336

241-
expect(onDiscard).toHaveBeenCalledTimes(1);
242-
expect(dispatchMock).toHaveBeenCalledTimes(1);
243-
// onDiscard (clear the draft) must run before navigation (dispatch).
244-
expect(order).toEqual(['discard', 'dispatch']);
337+
await flushMicrotasks();
245338

246-
act(() => {
247-
renderer.unmount();
248-
});
249-
});
339+
expect(onDiscard).toHaveBeenCalledTimes(1);
340+
expect(dispatchMock).toHaveBeenCalledTimes(1);
341+
expect(dispatchMock).toHaveBeenCalledWith(action);
342+
// The draft clear must finish before the captured leave is replayed.
343+
expect(order).toEqual(['discard', 'dispatch']);
250344

251-
it('stays and toasts when the draft clear fails', async () => {
252-
const onDiscard = vi.fn(async () => {
253-
await Promise.reject(new Error('storage failure'));
254-
});
255-
const { renderer } = mountGuard(true, onDiscard);
256-
triggerPreventRemove();
345+
act(() => {
346+
renderer.unmount();
347+
});
348+
}
349+
);
257350

258-
const discard = lastAlertButtons()?.find(button => button.text === 'Discard');
259-
expect(discard?.onPress).toBeDefined();
351+
it.each(PLATFORMS)(
352+
'closes the confirm, stays, and toasts when the clear fails (%s)',
353+
async os => {
354+
platform.os = os;
355+
const onDiscard = vi.fn(async () => {
356+
await Promise.reject(new Error('storage failure'));
357+
});
358+
const { renderer } = mountGuard(true, onDiscard);
260359

261-
act(() => {
262-
discard?.onPress?.();
263-
});
264-
await flushMicrotasks();
360+
triggerPreventRemoveInAct();
361+
expect(renderedLabels(renderer)).toContain('Discard draft?');
265362

266-
expect(onDiscard).toHaveBeenCalledTimes(1);
267-
// The failed clear must not navigate: the screen stays and the draft is kept.
268-
expect(dispatchMock).not.toHaveBeenCalled();
269-
expect(toastErrorMock).toHaveBeenCalledWith('Could not discard the draft. Please try again.');
363+
pressDialogButton(renderer, 'bg-destructive');
364+
await flushMicrotasks();
270365

271-
act(() => {
272-
renderer.unmount();
273-
});
274-
});
366+
expect(onDiscard).toHaveBeenCalledTimes(1);
367+
expect(dispatchMock).not.toHaveBeenCalled();
368+
expect(toastErrorMock).toHaveBeenCalledWith('Could not discard the draft. Please try again.');
369+
// Closed, so the user can retry by leaving again with the draft intact.
370+
expect(renderedLabels(renderer)).not.toContain('Discard draft?');
371+
372+
act(() => {
373+
renderer.unmount();
374+
});
375+
}
376+
);
275377

276-
it('Keep editing leaves the draft intact', () => {
378+
it.each(PLATFORMS)('keeps the draft and stays when the confirm is dismissed (%s)', os => {
379+
platform.os = os;
277380
const onDiscard = vi.fn(async () => {
278381
await Promise.resolve();
279382
});
280383
const { renderer } = mountGuard(true, onDiscard);
281-
triggerPreventRemove();
282384

283-
const keep = lastAlertButtons()?.find(button => button.text === 'Keep editing');
284-
// The cancel button carries no handler: dismissing it must not clear or leave.
285-
expect(keep?.onPress).toBeUndefined();
385+
triggerPreventRemoveInAct();
386+
pressDialogButton(renderer, 'border-border');
387+
286388
expect(onDiscard).not.toHaveBeenCalled();
287389
expect(dispatchMock).not.toHaveBeenCalled();
390+
expect(renderedLabels(renderer)).not.toContain('Discard draft?');
288391

289392
act(() => {
290393
renderer.unmount();

‎apps/mobile/src/components/agents/new-session-screen-body.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -582,7 +582,7 @@ export function NewSessionScreenBody() {
582582
attachments.releaseUnclaimedUploads();
583583
}, [userId, promptRef, attachments]);
584584

585-
useNewSessionDiscardGuard({
585+
const { discardConfirm } = useNewSessionDiscardGuard({
586586
dirty: (isCloneEntry ? false : hasPrompt) || attachments.hasUnclaimedAttachments,
587587
hasUnclaimedAttachments: attachments.hasUnclaimedAttachments,
588588
onDiscard: handleDiscardDraft,
@@ -841,6 +841,9 @@ export function NewSessionScreenBody() {
841841
cloudCreateError={cloudCreateError}
842842
onRetryCloudCreate={handleStartSession}
843843
/>
844+
{/* The discard confirm: a Modal overlay, so the composer behind it keeps
845+
its layout while the destructive choice keeps its red fill. */}
846+
{discardConfirm}
844847
</View>
845848
);
846849
}

0 commit comments

Comments
 (0)