Skip to content

Commit 4665417

Browse files
authored
fix(mobile): give the PR Review paste control a visible result on every tap (#9475)
* fix(mobile): give the PR Review paste control a visible result on every tap The paste control runs as a fire-and-forget press handler, and it read the clipboard with no error handling. A read the platform refuses therefore disappeared: no text in the field, no inline validation, no toast, nothing. Read the clipboard through one helper that never rejects. A refused text read, or a refused text probe on the empty path, now reports the paste as denied, so the tap always ends in the field being replaced or a toast that names why nothing was pasted. * fix(mobile): apply the repository formatter to the paste change Reformat the new test's import and the toast selection to the shared print width, and end the file with a newline.
1 parent be16925 commit 4665417

5 files changed

Lines changed: 200 additions & 13 deletions

File tree

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
// The paste control runs a fire-and-forget press handler, so a clipboard read
2+
// the platform refuses must still end in a visible result. These cases live
3+
// apart from the URL-field suite to keep both files within the lint line cap.
4+
5+
import { beforeEach, describe, expect, it, vi } from 'vitest';
6+
7+
import {
8+
find,
9+
flush,
10+
mocks,
11+
propsOf,
12+
renderLoaded,
13+
resetHookSlots,
14+
} from './pr-review-entry-screen-test-utils';
15+
16+
beforeEach(() => {
17+
vi.clearAllMocks();
18+
resetHookSlots();
19+
mocks.clipboard.current = '';
20+
mocks.clipboardHasString.current = false;
21+
mocks.clipboardFailure.current = null;
22+
mocks.clipboardHasStringFailure.current = null;
23+
});
24+
25+
function pasteControl(tree: unknown) {
26+
return find(
27+
tree,
28+
'Pressable',
29+
p => p.accessibilityLabel === 'Paste pull request or merge request link'
30+
);
31+
}
32+
33+
describe('paste clipboard read failures', () => {
34+
it('reports a refused read instead of leaving the tap without a result', async () => {
35+
mocks.clipboardFailure.current = new Error('clipboard unavailable');
36+
const tree = await renderLoaded();
37+
await (propsOf(pasteControl(tree)).onPress as () => Promise<void>)();
38+
await flush();
39+
expect(mocks.toastError).toHaveBeenCalledWith(
40+
'Paste access was denied. Allow paste and try again.'
41+
);
42+
expect(mocks.toastError).toHaveBeenCalledTimes(1);
43+
expect(mocks.push).not.toHaveBeenCalled();
44+
});
45+
46+
it('reports the same denial when only the text probe fails', async () => {
47+
mocks.clipboardHasStringFailure.current = new Error('clipboard unavailable');
48+
const tree = await renderLoaded();
49+
await (propsOf(pasteControl(tree)).onPress as () => Promise<void>)();
50+
await flush();
51+
expect(mocks.toastError).toHaveBeenCalledWith(
52+
'Paste access was denied. Allow paste and try again.'
53+
);
54+
expect(mocks.toastError).not.toHaveBeenCalledWith('Clipboard is empty');
55+
expect(mocks.push).not.toHaveBeenCalled();
56+
});
57+
});

‎apps/mobile/src/components/pr-review/pr-review-entry-screen-test-utils.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,10 @@ const harnessMocks = vi.hoisted(() => ({
2323
toastError: vi.fn(),
2424
clipboard: { current: '' as string },
2525
clipboardHasString: { current: false },
26+
// A clipboard read the platform refuses: the mocks below throw these so a
27+
// test can prove the tap still ends in a visible result.
28+
clipboardFailure: { current: null as Error | null },
29+
clipboardHasStringFailure: { current: null as Error | null },
2630
}));
2731

2832
// A hoisted binding cannot be exported directly; alias it for the test files.
@@ -53,10 +57,16 @@ vi.mock('expo-router', () => ({
5357
vi.mock('expo-clipboard', () => ({
5458
getStringAsync: async () => {
5559
await Promise.resolve();
60+
if (harnessMocks.clipboardFailure.current) {
61+
throw harnessMocks.clipboardFailure.current;
62+
}
5663
return harnessMocks.clipboard.current;
5764
},
5865
hasStringAsync: async () => {
5966
await Promise.resolve();
67+
if (harnessMocks.clipboardHasStringFailure.current) {
68+
throw harnessMocks.clipboardHasStringFailure.current;
69+
}
6070
return harnessMocks.clipboardHasString.current;
6171
},
6272
}));

‎apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,10 @@ import { ICON_TARGET_BOX_CLASS } from '@/lib/a11y/tap-target';
2424
import { useThemeColors } from '@/lib/hooks/use-theme-colors';
2525
import { providerPrRefLabel, providerPrRoutePath } from '@/lib/pr-review/provider-pr-ref';
2626
import {
27-
decidePrLinkPaste,
2827
prLinkToastClipboardEmptyCopy,
2928
prLinkToastPasteDeniedCopy,
29+
readPrLinkPasteClipboard,
3030
selectPrLinkClearButtonVisible,
31-
selectPrLinkPasteEmptyReason,
3231
} from '@/lib/pr-review/pr-link-paste';
3332
import {
3433
getRecentPrs,
@@ -92,22 +91,22 @@ export function PrReviewEntryScreen() {
9291
};
9392

9493
const handlePaste = async () => {
95-
const clipboard = await Clipboard.getStringAsync();
96-
const decision = decidePrLinkPaste(clipboard);
97-
if (decision.kind === 'empty') {
98-
// iOS answers a denied prompt with an empty read, so ask the prompt-free
99-
// probe whether the clipboard still holds text before calling it empty.
100-
const clipboardHasString = await Clipboard.hasStringAsync();
94+
// Every tap ends in a visible result: the field is replaced, or a toast
95+
// names why nothing could be pasted. The reader never rejects, so a
96+
// refused or broken clipboard read cannot vanish into the press handler.
97+
const outcome = await readPrLinkPasteClipboard(Clipboard);
98+
if (outcome.kind === 'empty') {
99+
// iOS answers a denied prompt with an empty read, so the reader asks the
100+
// prompt-free probe whether the clipboard still holds text before calling
101+
// it empty; a read that failed outright reports the same denial.
101102
announcingToast.error(
102-
selectPrLinkPasteEmptyReason(clipboard, clipboardHasString) === 'denied'
103-
? prLinkToastPasteDeniedCopy()
104-
: prLinkToastClipboardEmptyCopy()
103+
outcome.reason === 'denied' ? prLinkToastPasteDeniedCopy() : prLinkToastClipboardEmptyCopy()
105104
);
106105
return;
107106
}
108107
// Replace the native field, ref and derived UI together, never append at the cursor.
109-
writeField(decision.text);
110-
if (decision.kind === 'non-url-text') {
108+
writeField(outcome.text);
109+
if (outcome.kind === 'non-url-text') {
111110
return;
112111
}
113112
handleSubmit();

‎apps/mobile/src/lib/pr-review/pr-link-paste.test.ts‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
decidePrLinkPaste,
66
prLinkToastClipboardEmptyCopy,
77
prLinkToastPasteDeniedCopy,
8+
readPrLinkPasteClipboard,
89
selectPrLinkClearButtonVisible,
910
selectPrLinkPasteEmptyReason,
1011
} from './pr-link-paste';
@@ -137,6 +138,63 @@ describe('selectPrLinkPasteEmptyReason', () => {
137138
});
138139
});
139140

141+
describe('readPrLinkPasteClipboard', () => {
142+
const reader = (text: string | null, hasString: boolean) => ({
143+
getStringAsync: async () => {
144+
await Promise.resolve();
145+
return text;
146+
},
147+
hasStringAsync: async () => {
148+
await Promise.resolve();
149+
return hasString;
150+
},
151+
});
152+
153+
it('hands a resolvable read to the paste decision', async () => {
154+
const url = 'https://github.com/octocat/hello-world/pull/42';
155+
await expect(readPrLinkPasteClipboard(reader(url, true))).resolves.toEqual({
156+
kind: 'valid-pr-url',
157+
text: url,
158+
});
159+
await expect(readPrLinkPasteClipboard(reader(' not a link ', true))).resolves.toEqual({
160+
kind: 'non-url-text',
161+
text: 'not a link',
162+
});
163+
});
164+
165+
it('names an empty clipboard as empty', async () => {
166+
await expect(readPrLinkPasteClipboard(reader('', false))).resolves.toEqual({
167+
kind: 'empty',
168+
reason: 'empty',
169+
});
170+
});
171+
172+
it('reports a declined paste prompt as a denial', async () => {
173+
await expect(readPrLinkPasteClipboard(reader('', true))).resolves.toEqual({
174+
kind: 'empty',
175+
reason: 'denied',
176+
});
177+
});
178+
179+
it('turns a rejected text read into the denied result instead of rejecting', async () => {
180+
await expect(
181+
readPrLinkPasteClipboard({
182+
getStringAsync: () => Promise.reject(new Error('clipboard unavailable')),
183+
hasStringAsync: () => Promise.resolve(false),
184+
})
185+
).resolves.toEqual({ kind: 'empty', reason: 'denied' });
186+
});
187+
188+
it('turns a rejected text probe into the denied result instead of rejecting', async () => {
189+
await expect(
190+
readPrLinkPasteClipboard({
191+
getStringAsync: () => Promise.resolve(''),
192+
hasStringAsync: () => Promise.reject(new Error('clipboard unavailable')),
193+
})
194+
).resolves.toEqual({ kind: 'empty', reason: 'denied' });
195+
});
196+
});
197+
140198
describe('selectPrLinkClearButtonVisible', () => {
141199
it('is present when the field has content', () => {
142200
expect(selectPrLinkClearButtonVisible({ hasInput: true })).toBe(true);

‎apps/mobile/src/lib/pr-review/pr-link-paste.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,69 @@ export function selectPrLinkPasteEmptyReason(
6767
return (clipboard ?? '').length === 0 && clipboardHasString ? 'denied' : 'empty';
6868
}
6969

70+
type PrLinkPasteClipboardReader = {
71+
getStringAsync: () => Promise<string | null | undefined>;
72+
hasStringAsync: () => Promise<boolean>;
73+
};
74+
75+
/**
76+
* A paste tap's clipboard outcome. `empty` carries why the read produced
77+
* nothing, so the call site picks the toast that names it; every other kind
78+
* carries the text to write into the field.
79+
*/
80+
export type PrLinkPasteOutcome =
81+
| { kind: 'valid-pr-url'; text: string }
82+
| { kind: 'non-url-text'; text: string }
83+
| { kind: 'empty'; reason: PrLinkPasteEmptyReason };
84+
85+
/**
86+
* Read the clipboard for a paste tap without ever rejecting.
87+
*
88+
* The control runs this as a fire-and-forget press handler, so a rejected
89+
* read would leave the tap with no visible result at all: no text, no
90+
* validation, no toast. A read the platform refuses — the iOS paste prompt
91+
* declined, a service that is not available, a throw out of the native
92+
* module — is reported as `denied`, the state whose copy already tells the
93+
* reader that paste access was refused and asks them to try again. The
94+
* clipboard-empty answer keeps its own copy through
95+
* `selectPrLinkPasteEmptyReason`, so a resolver that throws on the second
96+
* probe cannot swallow the tap either.
97+
*
98+
* The attachment helpers (`readClipboardText`, `hasClipboardText`) also
99+
* swallow a failed read, but they collapse it into `''`/`false`, which this
100+
* control cannot use: it has to tell a refused read from a clipboard that is
101+
* genuinely empty to pick the copy that names the right cause.
102+
*/
103+
export async function readPrLinkPasteClipboard(
104+
reader: PrLinkPasteClipboardReader
105+
): Promise<PrLinkPasteOutcome> {
106+
const textRead = await settleClipboardRead(() => reader.getStringAsync());
107+
if (!textRead.ok) {
108+
return { kind: 'empty', reason: 'denied' };
109+
}
110+
const clipboard = textRead.value ?? '';
111+
const decision = decidePrLinkPaste(clipboard);
112+
if (decision.kind !== 'empty') {
113+
return decision;
114+
}
115+
const probe = await settleClipboardRead(() => reader.hasStringAsync());
116+
if (!probe.ok) {
117+
return { kind: 'empty', reason: 'denied' };
118+
}
119+
return { kind: 'empty', reason: selectPrLinkPasteEmptyReason(clipboard, probe.value) };
120+
}
121+
122+
/** One clipboard read, with a refusal reported as data instead of a rejection. */
123+
async function settleClipboardRead<T>(
124+
read: () => Promise<T>
125+
): Promise<{ ok: true; value: T } | { ok: false }> {
126+
try {
127+
return { ok: true, value: await read() };
128+
} catch {
129+
return { ok: false };
130+
}
131+
}
132+
70133
/** Toast copy when paste finds an empty clipboard. */
71134
export function prLinkToastClipboardEmptyCopy(): string {
72135
return i18n.t('prReview.linkPasteClipboardEmpty');

0 commit comments

Comments
 (0)