diff --git a/mobile/src/platform/clipboard.web.test.tsx b/mobile/src/platform/clipboard.web.test.tsx index 67fa20e42d9..01edf0d66f1 100644 --- a/mobile/src/platform/clipboard.web.test.tsx +++ b/mobile/src/platform/clipboard.web.test.tsx @@ -129,10 +129,11 @@ describe('reading the clipboard from inside the shell', () => { /** * The degradation, recorded rather than implied. * - * No shell serves an image yet — `native.clipboard.read` refuses `{ mime: 'image' }` by name and - * a 24 MiB base64 image cannot cross an 8 MiB reply cap — so the page answers what an empty - * clipboard answers and the terminal's paste takes the branch it already had. On the page an - * image on the clipboard pastes nothing until the media verbs land. + * No shell reads an image for the page yet — `native.clipboard.read` admits only `text`, so an + * image mime is `invalid-params` rather than a refusal of its own, and a 24 MiB base64 image + * cannot cross an 8 MiB reply cap. The pasteboard's image is `native.media.pick + * { source: 'clipboard' }`, landed in C7.4 and unwired until C7.6. So the page answers what an + * empty clipboard answers and the terminal's paste takes the branch it already had. */ it('answers no image, without asking the shell for one', async () => { const pair = createFakeBridgePortPair() diff --git a/mobile/src/platform/clipboard.web.ts b/mobile/src/platform/clipboard.web.ts index 7b2f20fb192..52839c6ff51 100644 --- a/mobile/src/platform/clipboard.web.ts +++ b/mobile/src/platform/clipboard.web.ts @@ -31,12 +31,14 @@ export function useClipboardWriter(): ClipboardWriter { /** * Web sibling: the shell reads text for the page, and no shell reads an image for it yet. * - * `native.clipboard.read` serves text and refuses `{ mime: 'image' }` by name. Widening it cannot - * work — `CLIPBOARD_IMAGE_MAX_BASE64_CHARS` is 24 MiB against a reply cap of 8 MiB — so an image - * is the media verbs' job and is not served here. `readImage` therefore answers null, which is the - * answer an empty clipboard already gives, and the terminal's paste takes the branch it has always - * taken for one. A recorded degradation, not a silent one: on the page an image on the clipboard - * pastes nothing until the media verbs land. + * `native.clipboard.read` is text and only text: `BRIDGE_CLIPBOARD_MIMES` is `['text']`, so an + * image mime is not a refusal the verb spells out but a value its schema does not admit, answered + * `invalid-params`. Widening it cannot work — `CLIPBOARD_IMAGE_MAX_BASE64_CHARS` is 24 MiB against + * a reply cap of 8 MiB — so an image on the pasteboard is `native.media.pick { source: 'clipboard' }`, + * which C7.4 landed and C7.6 will wire. `readImage` therefore answers null, which is the answer an + * empty clipboard already gives, and the terminal's paste takes the branch it has always taken for + * one. A recorded degradation, not a silent one: on the page an image on the clipboard pastes + * nothing until that wiring lands. * * `contents` cannot be a probe. The shell serves no "is there text" verb and reading to find out * would raise iOS's paste-consent prompt on every mount and every foreground, which is the whole diff --git a/mobile/src/session/clipboard-write-failure-branch-census.test.ts b/mobile/src/session/clipboard-write-failure-branch-census.test.ts index f151c8ae761..cfe62b04453 100644 --- a/mobile/src/session/clipboard-write-failure-branch-census.test.ts +++ b/mobile/src/session/clipboard-write-failure-branch-census.test.ts @@ -21,42 +21,59 @@ const SESSION_ROOT = import.meta.dirname /** The seam's own name, so a local helper called `writeText` is not mistaken for it. */ const SEAM_METHOD = 'writeText' +const CATCH_METHOD = 'catch' const SEAM_HOOK = 'useClipboardWriter' /** * Whether a call is answered for, by either shape this tree uses. * - * `await` inside a `try` with a `catch`, or a `.catch(...)` on the promise itself. Walked upward - * from the call rather than matched on text: both shapes put the handler somewhere other than the - * line the write is on, and a regex over the file would pass a `catch` that belongs to a different - * statement entirely. + * `await` inside the block of a `try` that catches, or a `.catch(...)` on the promise itself. + * Walked upward from the call rather than matched on text: both shapes put the handler somewhere + * other than the line the write is on, and a regex over the file would pass a `catch` that belongs + * to a different statement entirely. + * + * The `await` is not a formality. A promise nobody waits for settles after the block that started + * it has returned, so a `void clipboard.writeText(...)` inside a `try` is the unhandled rejection + * this rule exists to stop, with a `catch` above it that can never run. `return` is not enough + * either, for the same reason: only `return await` keeps the call inside the block. */ -function hasFailureBranch(call: ts.Node): boolean { - let node: ts.Node | undefined = call - while (node !== undefined) { - if (ts.isTryStatement(node) && node.catchClause !== undefined) { - return true +function hasFailureBranch(call: ts.CallExpression): boolean { + let node: ts.Node = call + let awaited = false + while (node.parent !== undefined) { + const parent: ts.Node = node.parent + if (ts.isAwaitExpression(parent)) { + awaited = true } - // `clipboard.writeText(x).then(...).catch(...)`: the catch is further out in the same chain. + // `clipboard.writeText(x).then(...).catch(...)`: the handler is further along this same chain, + // and `parent.expression === node` is what keeps it to this chain rather than any nearby catch. if ( - ts.isCallExpression(node) && - ts.isPropertyAccessExpression(node.expression) && - node.expression.name.text === 'catch' + ts.isPropertyAccessExpression(parent) && + parent.expression === node && + parent.name.text === CATCH_METHOD ) { return true } - // A function boundary ends the search: a `catch` outside it belongs to a different call. - if (ts.isFunctionDeclaration(node) || ts.isMethodDeclaration(node)) { + // `tryBlock === node` because a write inside the catch or finally clause is not answered for by + // the try it is written in: that handler has already run. + if (ts.isTryStatement(parent) && parent.catchClause !== undefined && parent.tryBlock === node) { + return awaited + } + // Every function-like node ends the search, arrows and function expressions included: a `catch` + // outside one answers for whoever called it, not for the call left running inside. + if (ts.isFunctionLike(parent)) { return false } - node = node.parent + node = parent } return false } -/** Every `x.writeText(...)` in a module, as `file:line`, with whether its failure is handled. */ -function clipboardWrites(root: string, name: string): { at: string; handled: boolean }[] { - const source = parse(root, name) +/** Every `x.writeText(...)` in a parsed module, as `file:line`, with whether its failure is handled. */ +function clipboardWritesIn( + source: ts.SourceFile, + name: string +): { at: string; handled: boolean }[] { const found: { at: string; handled: boolean }[] = [] const visit = (node: ts.Node): void => { if ( @@ -73,6 +90,27 @@ function clipboardWrites(root: string, name: string): { at: string; handled: boo return found } +function clipboardWrites(root: string, name: string): { at: string; handled: boolean }[] { + return clipboardWritesIn(parse(root, name), name) +} + +/** A snippet read the way the census reads a module, for shapes no product file holds today. */ +function writesInSnippet(source: string): { at: string; handled: boolean }[] { + return clipboardWritesIn( + ts.createSourceFile('snippet.ts', source, ts.ScriptTarget.Latest, true), + 'snippet.ts' + ) +} + +/** Whether the census would report the single write in a snippet. */ +function reports(source: string): boolean { + const writes = writesInSnippet(source) + if (writes.length !== 1) { + throw new Error(`the snippet holds ${writes.length} writes, not one`) + } + return !writes[0].handled +} + describe('every clipboard write in the session domain answers for its failure', () => { const files = productFiles(SESSION_ROOT) const writes = files.flatMap((name) => clipboardWrites(SESSION_ROOT, name)) @@ -91,3 +129,89 @@ describe('every clipboard write in the session domain answers for its failure', expect(writes.filter((write) => !write.handled).map((write) => write.at)).toEqual([]) }) }) + +/** + * What the rule counts as somewhere for a failure to go, in the shapes a product file does not hold. + * + * The census walks real modules, so the shapes it must refuse cannot be planted in one: a `void` + * write is exactly the mistake it exists to catch, and it would have to be committed to be tested. + * Snippets instead, read through the same reader, so a rule that stops matching is a red here + * rather than a site that quietly passes for years. + */ +describe('the failure branch the census will accept', () => { + it('refuses a write nobody waits for, however well the block around it is guarded', () => { + // The whole point of the rule. `void` detaches the promise from the block: the `try` has + // returned long before the rejection settles, and the process gets an unhandled rejection with + // a `catch` sitting three lines above it that never runs. + expect( + reports(` + async function copy(clipboard: Clipboard, text: string) { + try { + void clipboard.writeText(text) + } catch { + showToast("Couldn't copy") + } + } + `) + ).toBe(true) + }) + + it('refuses a write whose only catch is outside the function it sits in', () => { + // An arrow ends the search as surely as a declaration does. The `catch` here answers for + // `forEach`, which returns before the write it started has settled. + expect( + reports(` + async function copyAll(clipboard: Clipboard, rows: string[]) { + try { + rows.forEach((row) => { + clipboard.writeText(row) + }) + } catch { + showToast("Couldn't copy") + } + } + `) + ).toBe(true) + }) + + it('refuses a write in the catch block, which its own try does not answer for', () => { + expect( + reports(` + async function copy(clipboard: Clipboard, text: string) { + try { + await save(text) + } catch { + await clipboard.writeText(text) + } + } + `) + ).toBe(true) + }) + + it('accepts an awaited write inside a try that catches', () => { + expect( + reports(` + async function copy(clipboard: Clipboard, text: string) { + try { + await clipboard.writeText(text) + } catch { + showToast("Couldn't copy") + } + } + `) + ).toBe(false) + }) + + it('accepts a write that carries its own catch along the chain', () => { + expect( + reports(` + function copy(clipboard: Clipboard, text: string) { + clipboard + .writeText(text) + .then(() => showToast('Copied')) + .catch(() => showToast("Couldn't copy")) + } + `) + ).toBe(false) + }) +}) diff --git a/mobile/src/session/mobile-session-route-parity.test.ts b/mobile/src/session/mobile-session-route-parity.test.ts index 6afd381218a..82ceec90a1f 100644 --- a/mobile/src/session/mobile-session-route-parity.test.ts +++ b/mobile/src/session/mobile-session-route-parity.test.ts @@ -111,8 +111,10 @@ const HEAD_TIMER_CLEANUP_SHA256 = 'c73f1d1c2cc89642f3d727d6f3b6b81860a9d6f342345 // Six method literals fewer than before step 6: `terminal.send` and `terminal.clearBuffer` went // first, then `worktree.activate` twice, `session.tabs.createTerminal` and // `terminal.setDisplayMode`. Each is now fixed at its operation's definition instead of being -// spelled at the call site. One literal more in C7.2: the Markdown copy action's "Couldn't copy", -// the toast the other copy paths already showed when a write was refused. +// spelled at the call site. Two literals more across C7.2, both of them the toast a refused write +// now shows: "Couldn't copy path" when the sheets moved onto the clipboard seam, taking the count +// from 532 to 533, and "Couldn't copy" when the Markdown copy action gained the failure branch the +// other copy paths already had, taking it to 534. const HEAD_RUNTIME_STRING_SHA256 = 'ce4c68956cec3b49aaf785e99bc2d7efd3eafeb4fdac6ce116cd854546c045f4' const HEAD_HOST_JSX_SHA256 = '390405926b1695fa3a33686f0bc192b432f5468d8576499d7cafbb4922defbb5'