diff --git a/config/scripts/mobile-web-app-external-link-seam.mjs b/config/scripts/mobile-web-app-external-link-seam.mjs index 6ee58482538..a8577a69b5b 100644 --- a/config/scripts/mobile-web-app-external-link-seam.mjs +++ b/config/scripts/mobile-web-app-external-link-seam.mjs @@ -4,23 +4,146 @@ * Shared by every page route's census rather than restated in each: two spellings of one rule * drift, and the half that stops being enforced is the half nobody reads again. */ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import ts from 'typescript-api' /** The seam, as the web build resolves it: `.web.ts` wins under the builder's resolveExtensions, * and it is the one module a page closure may reach react-native's `Linking` from. */ export const EXTERNAL_LINK_SEAM = 'src/platform/external-link.web.ts' /** - * Whether a module reaches react-native's own `Linking`, by name or through a namespace import. + * Every line on which a module reaches react-native's own `Linking`, by name or through a + * namespace import. * - * Both quote styles: the tree is single-quoted by the formatter today, so a double-quoted - * specifier would have walked past this unseen — and a census that cannot see a call site is one - * that passes for the wrong reason. + * Parsed rather than matched: a regex over the text names `Linking` inside a comment that talks + * about it and inside a string that quotes it, and a census that reports a line nobody can act on + * is one the next reader learns to ignore. The parser also settles the quote styles for free. + * + * A named import reports the import statement, once however many times the module calls through + * it, because the import is the thing the rule is about and the thing that has to go. The imported + * name is what counts, not the local one: `import { Linking as NativeLinking }` is the same import + * spelled differently, and reading only the binding let it through. + * + * A namespace import reports its uses instead, there being no single line to name — `import * as RN + * from 'react-native'` is not itself an offence — and every alias is read, because a module may + * import the namespace twice and call on either. A default import is read the same way: this + * project's interop settings accept `import RN from 'react-native'` (checked with tsc), so it is a + * binding the whole namespace hangs off exactly as `* as RN` is. + * + * A re-export is reported at the export statement. `export { Linking } from 'react-native'` puts + * the name back in reach of anything that imports this module, and so does `export *`, which + * carries it along with everything else; the statement is the line to delete, exactly as an import + * is. + * + * Lines rather than a boolean because a red census that names `path:line` is read once, and one + * that names a file is grepped for. The boolean below is derived from this, so there is one rule. + * + * `fileName` decides the script kind, and the default is only for callers holding a source with no + * path. In a `.ts` file `const id = (value: T) => value` is a generic arrow; parsed as TSX it is + * an unclosed JSX element, and everything after it — a later `RN.Linking.openURL` included — is + * swallowed into the error node and never walked. */ -export function reachesReactNativeLinking(source) { - const named = /import\s*\{[^}]*\bLinking\b[^}]*\}\s*from\s*['"]react-native['"]/s - const namespace = /import\s*\*\s*as\s*(\w+)\s*from\s*['"]react-native['"]/ - const asNamespace = namespace.exec(source) +export function reactNativeLinkingSites(source, fileName = 'module.tsx') { + // No explicit script kind: TypeScript reads it off the extension, which is the whole point. + const parsed = ts.createSourceFile(fileName, source, ts.ScriptTarget.Latest, true) + const lineOf = (node) => parsed.getLineAndCharacterOfPosition(node.getStart(parsed)).line + 1 + const sites = [] + const aliases = new Set() + const fromReactNative = (statement) => + statement.moduleSpecifier !== undefined && + ts.isStringLiteral(statement.moduleSpecifier) && + statement.moduleSpecifier.text === 'react-native' + /** `propertyName` is the exported/imported name when the clause renames it, `name` when it does not. */ + const namesLinking = (elements) => + elements.some((element) => (element.propertyName ?? element.name).text === 'Linking') + for (const statement of parsed.statements) { + if (ts.isExportDeclaration(statement) && fromReactNative(statement)) { + const clause = statement.exportClause + // No clause is `export *`, which carries `Linking` with everything else; a namespace export + // hands the whole module over under one name. Both put it back in reach. + if (clause === undefined || ts.isNamespaceExport(clause) || namesLinking(clause.elements)) { + sites.push(lineOf(statement)) + } + continue + } + if (!ts.isImportDeclaration(statement) || !fromReactNative(statement)) { + continue + } + const clause = statement.importClause + if (clause === undefined) { + continue + } + if (clause.name !== undefined) { + aliases.add(clause.name.text) + } + const bindings = clause.namedBindings + if (bindings === undefined) { + continue + } + if (ts.isNamespaceImport(bindings)) { + aliases.add(bindings.name.text) + continue + } + if (namesLinking(bindings.elements)) { + sites.push(lineOf(statement)) + } + } + if (aliases.size > 0) { + const visit = (node) => { + if ( + ts.isPropertyAccessExpression(node) && + ts.isIdentifier(node.expression) && + aliases.has(node.expression.text) && + node.name.text === 'Linking' + ) { + // The line, not the expression: two aliases meeting on one line are one site. + sites.push(lineOf(node)) + } + ts.forEachChild(node, visit) + } + ts.forEachChild(parsed, visit) + } + return [...new Set(sites)].sort((left, right) => left - right) +} + +/** Whether a module reaches react-native's own `Linking`. */ +export function reachesReactNativeLinking(source, fileName = 'module.tsx') { + return reactNativeLinkingSites(source, fileName).length > 0 +} + +/** + * Every module in a route's closure that can reach a URL without the seam, as `path:line`. + * + * The line is where the name enters the module, not where it is used: a named import is reported + * once however many times the module calls `Linking.openURL`, because the import is what the rule + * is about and what has to go. Only a namespace import reports its uses, there being no single + * line to name — `import * as RN from 'react-native'` is not itself an offence. + * + * Here rather than beside each census: three copies of this walk existed before the source-control + * routes wanted a fourth, and the seam's own module is where the rule they share belongs. A file + * the closure names but this checkout cannot read is not an offender — the closure reports paths + * relative to `mobile/`, and one outside it is read by its caller, not guessed at here. + */ +export function externalLinkOffenders(mobileDir, closure) { return ( - named.test(source) || (asNamespace !== null && source.includes(`${asNamespace[1]}.Linking`)) + closure.local + .filter((file) => file !== EXTERNAL_LINK_SEAM) + .flatMap((file) => { + let source + try { + source = readFileSync(join(mobileDir, file), 'utf8') + } catch { + return [] + } + // The path, so the parser takes the script kind from the extension rather than assuming TSX. + return reactNativeLinkingSites(source, file).map((line) => [file, line]) + }) + // By path, then by line as a number: sorting the rendered strings puts `:10` before `:2`, and + // a red list is read top to bottom against the file it names. + .sort(([leftFile, leftLine], [rightFile, rightLine]) => + leftFile === rightFile ? leftLine - rightLine : leftFile < rightFile ? -1 : 1 + ) + .map(([file, line]) => `${file}:${line}`) ) } diff --git a/config/scripts/mobile-web-app-external-link-seam.test.mjs b/config/scripts/mobile-web-app-external-link-seam.test.mjs index 4150d421aac..9e43078ba4f 100644 --- a/config/scripts/mobile-web-app-external-link-seam.test.mjs +++ b/config/scripts/mobile-web-app-external-link-seam.test.mjs @@ -1,5 +1,16 @@ +import { mkdtempSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' -import { reachesReactNativeLinking } from './mobile-web-app-external-link-seam.mjs' +import { + externalLinkOffenders, + reachesReactNativeLinking, + reactNativeLinkingSites +} from './mobile-web-app-external-link-seam.mjs' + +const mobileDir = fileURLToPath(new URL('../../mobile/', import.meta.url)) + describe('the seam predicate', () => { it.each([ ["import { Linking } from 'react-native'", true], @@ -12,3 +23,176 @@ describe('the seam predicate', () => { expect(reachesReactNativeLinking(source)).toBe(expected) }) }) + +describe('where the seam predicate says a module reaches Linking', () => { + it('reports the import line, which is what a red census is read for', () => { + expect( + reactNativeLinkingSites( + "import { View } from 'react-native'\n\nimport {\n Linking\n} from 'react-native'\n" + ) + ).toEqual([3]) + }) + + it('reports every line a namespace import is used on, not just the import', () => { + expect( + reactNativeLinkingSites( + "import * as RN from 'react-native'\nRN.Linking.openURL(a)\nconst b = 1\nRN.Linking.openURL(c)\n" + ) + ).toEqual([2, 4]) + }) + + it('inspects every alias, not just the first namespace import', () => { + // Two namespace imports of react-native, the first unused. Reading only the first alias makes + // a module that calls `Linking.openURL` on the second report no site at all. + expect( + reactNativeLinkingSites( + "import * as Unused from 'react-native'\nimport * as RN from 'react-native'\nRN.Linking.openURL(u)\n" + ) + ).toEqual([3]) + }) + + it('counts a line once when two aliases meet on it', () => { + expect( + reactNativeLinkingSites( + "import * as A from 'react-native'\nimport * as B from 'react-native'\nA.Linking.openURL(B.Linking)\n" + ) + ).toEqual([3]) + }) + + it('names an import that renames Linking, which reading the binding alone missed', () => { + // `import { Linking as NativeLinking }` is the same import spelled differently; the imported + // name lives in `propertyName` when a specifier renames it, and only in `name` when it does + // not. Reading `name` alone let `NativeLinking.openURL` through the census entirely. + expect( + reactNativeLinkingSites( + "import { Linking as NativeLinking } from 'react-native'\nNativeLinking.openURL(u)\n" + ) + ).toEqual([1]) + }) + + it('leaves alone a local binding that is only spelled Linking', () => { + // The other half of reading `propertyName`: this module imports `View`, so naming it would be + // a red line with nothing to fix at the end of it. + expect( + reactNativeLinkingSites("import { View as Linking } from 'react-native'\nLinking.foo()\n") + ).toEqual([]) + }) + + it('reads a default import as the namespace it is, which the interop here allows', () => { + // `import RN from 'react-native'` typechecks here, so it is a binding the whole namespace + // hangs off and a call through it is as invisible to a named-import rule as an alias was. + expect( + reactNativeLinkingSites("import RN from 'react-native'\nRN.Linking.openURL(u)\n") + ).toEqual([2]) + }) + + it('leaves alone an alias that is imported and never reaches Linking', () => { + // An import of react-native is not the offence; reaching `Linking` through it is. + expect(reactNativeLinkingSites("import * as RN from 'react-native'\nRN.Platform.OS\n")).toEqual( + [] + ) + }) + + it('parses a .ts module as TypeScript, where a generic arrow is not an unclosed tag', () => { + // `const id = (value: T) => value` is a generic arrow in a `.ts` file and an unclosed JSX + // element in a `.tsx` one. Parsed as TSX, everything after it falls into the error node, so + // the call below was never walked and the module reported nothing at all. + expect( + reactNativeLinkingSites( + "import * as RN from 'react-native'\nconst id = (value: T) => value\nRN.Linking.openURL(u)\n", + 'module.ts' + ) + ).toEqual([3]) + }) + + it.each([ + ["export { Linking } from 'react-native'\n", 'a named re-export'], + ["export { Linking as L } from 'react-native'\n", 'a renamed re-export'], + ["export * from 'react-native'\n", 'a wildcard re-export, which carries it with the rest'], + ["export * as RN from 'react-native'\n", 'a namespace re-export'] + ])('names %# : %s', (source) => { + // A re-export puts `Linking` back in reach of whatever imports this module, so the route's + // closure reaches it through a file that never imported it. The export statement is the line + // to delete, exactly as an import is. + expect(reactNativeLinkingSites(source, 'module.ts')).toEqual([1]) + }) + + it.each([ + ["export { View } from 'react-native'\n", 're-exports something else'], + [ + "export { Linking } from './local'\n", + 're-exports the name from somewhere that is not react-native' + ] + ])('leaves alone a module that %# : %s', (source) => { + expect(reactNativeLinkingSites(source, 'module.ts')).toEqual([]) + }) + + it('ignores the name inside a comment, which text matching cannot', () => { + // A module that talks about the rule is not breaking it, and a census that names a comment is + // one whose red list the next reader learns to skip. + expect( + reactNativeLinkingSites( + "import * as RN from 'react-native'\n// never call RN.Linking.openURL here\n/* nor RN.Linking */\n" + ) + ).toEqual([]) + }) + + it('ignores the name inside a string, and still sees the call beside it', () => { + expect( + reactNativeLinkingSites( + "import * as RN from 'react-native'\nconst hint = 'use RN.Linking.openURL'\nRN.Linking.openURL(u)\n" + ) + ).toEqual([3]) + }) + + it('ignores a commented-out named import, which was the same class of miss', () => { + expect(reactNativeLinkingSites("// import { Linking } from 'react-native'\n")).toEqual([]) + }) + + it('finds nothing in a module that only names the seam', () => { + expect( + reactNativeLinkingSites("import { openExternalLink } from '../platform/external-link'") + ).toEqual([]) + }) +}) + +describe('the offenders in a closure', () => { + // The seam itself imports `Linking` and is the one module allowed to, so a walk that did not + // exempt it would report every closure as an offender and never be able to go green. + const closure = { local: ['src/platform/external-link.web.ts', 'src/platform/external-link.ts'] } + + it('exempts the seam and names the module that went around it', () => { + expect(externalLinkOffenders(mobileDir, closure)).toEqual(['src/platform/external-link.ts:1']) + }) + + it('ignores a path this checkout cannot read rather than calling it an offender', () => { + expect(externalLinkOffenders(mobileDir, { local: ['src/not/a/file.ts'] })).toEqual([]) + }) +}) + +describe('the order a red list is read in', () => { + // Against a written fixture rather than the tree: the ordering this pins needs one module with + // sites on lines 2 and 10, the pair that sorts one way as numbers and the other as text, and no + // module in the closure has to keep having one. + const root = mkdtempSync(join(tmpdir(), 'orca-seam-census-')) + const lines = ["import * as RN from 'react-native'", 'RN.Linking.openURL(a)'] + while (lines.length < 9) { + lines.push('') + } + lines.push('RN.Linking.openURL(b)') + writeFileSync(join(root, 'wide.ts'), `${lines.join('\n')}\n`) + writeFileSync(join(root, 'above.ts'), "import { Linking } from 'react-native'\n") + + it('puts line 2 before line 10, which sorting the rendered strings does not', () => { + // `:10` sorts before `:2` as text. The namespace import on line 1 is not itself an offence. + expect(externalLinkOffenders(root, { local: ['wide.ts'] })).toEqual(['wide.ts:2', 'wide.ts:10']) + }) + + it('orders by path first, so two modules never interleave', () => { + expect(externalLinkOffenders(root, { local: ['wide.ts', 'above.ts'] })).toEqual([ + 'above.ts:1', + 'wide.ts:2', + 'wide.ts:10' + ]) + }) +}) diff --git a/config/scripts/mobile-web-app-files-external-links.test.mjs b/config/scripts/mobile-web-app-files-external-links.test.mjs index 65847719724..29bcf1d7b3b 100644 --- a/config/scripts/mobile-web-app-files-external-links.test.mjs +++ b/config/scripts/mobile-web-app-files-external-links.test.mjs @@ -12,8 +12,6 @@ * page route reaches and which the worktree list declares nothing for — and declares the grant * because its rows push to the preview in-page, under the session the explorer opened. */ -import { readFileSync } from 'node:fs' -import { join } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' import { mobileWebAppRouteClosure } from './build-mobile-web-app-bundle.mjs' @@ -21,7 +19,7 @@ import { MOBILE_WEB_PAGE_ROUTES } from './mobile-web-page-routes.mjs' import { mobileWebAppDependenciesPresent } from './mobile-web-app-bundle-dependencies.mjs' import { EXTERNAL_LINK_SEAM as SEAM, - reachesReactNativeLinking + externalLinkOffenders } from './mobile-web-app-external-link-seam.mjs' const mobileDir = fileURLToPath(new URL('../../mobile/', import.meta.url)) @@ -33,25 +31,12 @@ const PREVIEW = 'app/h/[hostId]/files/preview/[worktreeId].tsx' /** The seam's only in-domain consumer, and the reason the preview declares the grant itself. */ const MARKDOWN = 'src/components/MobileMarkdown.tsx' -function offenders(closure) { - return closure.local - .filter((file) => file !== SEAM) - .filter((file) => { - try { - return reachesReactNativeLinking(readFileSync(join(mobileDir, file), 'utf8')) - } catch { - return false - } - }) - .sort() -} - describeClosure( 'the files page closures', () => { it.each([EXPLORER, PREVIEW])('opens every external URL through the seam: %s', async (route) => { const closure = await mobileWebAppRouteClosure(route) - expect(offenders(closure)).toEqual([]) + expect(externalLinkOffenders(mobileDir, closure)).toEqual([]) }) it.each([EXPLORER, PREVIEW])( diff --git a/config/scripts/mobile-web-app-source-control-external-links.test.mjs b/config/scripts/mobile-web-app-source-control-external-links.test.mjs new file mode 100644 index 00000000000..da3e85f6789 --- /dev/null +++ b/config/scripts/mobile-web-app-source-control-external-links.test.mjs @@ -0,0 +1,101 @@ +/** + * What the source-control hub and the diff review page may reach for a URL. + * + * Inside the shell's WebView react-native-web's `Linking.openURL` calls + * `window.open(url, '_blank', 'noopener')`, which both shells refuse — iOS returns nil from + * `createWebViewWith`, Android false from `onCreateWindow` — and resolves whether or not anything + * opened. A call site left on that path reports success into a tap that did nothing, which is the + * one failure the `externalLink` grant exists to remove. + * + * Both routes reach the PR sidebar, and the sidebar is where this domain's openers are: a check's + * "open on the web", a comment's permalink, and a link inside comment Markdown. So both are held + * to the same rule and neither inherits it from the other. + * + * The rule, not the three call sites it happens to have today: a module entering either closure + * later is held to it without anyone remembering to add it here. + */ +import { fileURLToPath } from 'node:url' +import { describe, expect, it } from 'vitest' +import { mobileWebAppRouteClosure } from './build-mobile-web-app-bundle.mjs' +import { mobileWebAppDependenciesPresent } from './mobile-web-app-bundle-dependencies.mjs' +import { + EXTERNAL_LINK_SEAM as SEAM, + externalLinkOffenders +} from './mobile-web-app-external-link-seam.mjs' + +const mobileDir = fileURLToPath(new URL('../../mobile/', import.meta.url)) +const describeClosure = mobileWebAppDependenciesPresent() ? describe : describe.skip + +const HUB = 'app/h/[hostId]/source-control/[worktreeId].tsx' +const REVIEW = 'app/h/[hostId]/review/[worktreeId].tsx' + +/** The sidebar both routes render, and the reason each declares the grant on its own account. */ +const PR_COMMENT_CARD = 'src/components/pr-sidebar/PRCommentCard.tsx' + +/** The clipboard seam, as the web build resolves it. */ +const CLIPBOARD_SEAM = 'src/platform/clipboard.web.ts' + +describeClosure( + 'the source-control and review page closures', + () => { + it.each([HUB, REVIEW])('opens every external URL through the seam: %s', async (route) => { + const closure = await mobileWebAppRouteClosure(route) + expect(externalLinkOffenders(mobileDir, closure)).toEqual([]) + }) + + it.each([HUB, REVIEW])('contains the seam, so the rule is not vacuous: %s', async (route) => { + // Without this an empty offender list would also be what a closure reaching no link code at + // all produces, and the census would pass against a page that opens nothing. + const closure = await mobileWebAppRouteClosure(route) + expect(closure.local).toContain(SEAM) + expect(closure.local.length).toBeGreaterThan(400) + }) + + it('reaches the openers from the PR sidebar, which both routes render', async () => { + // The reason the grant is each route's own rather than one inherited through a hop: without + // this, `externalLink` on both would be a line in a manifest nothing holds to a caller. + const [hub, review] = await Promise.all([ + mobileWebAppRouteClosure(HUB), + mobileWebAppRouteClosure(REVIEW) + ]) + expect(hub.local).toContain(PR_COMMENT_CARD) + expect(review.local).toContain(PR_COMMENT_CARD) + }) + }, + 240_000 +) + +/** + * Neither route writes the clipboard through the browser's own. + * + * `expo-clipboard` resolves to `ExpoClipboard.web.js`, which is `navigator.clipboard`: it needs a + * secure context, and the iOS shell serves the page from a custom scheme while Android serves + * `https`, so that path works on one platform and silently not on the other. Both routes copy — + * the conflict section's refresh commands, and the review sheet's notes — so both are granted + * `native.clipboard.write` and both must reach it through the seam. + * + * Asserted as the module's absence from the closure rather than as a count of importers: a new + * import anywhere in the tree puts the file back, whoever writes it and whatever they name it. + */ +describeClosure( + 'the clipboard the source-control and review pages reach', + () => { + it.each([HUB, REVIEW])( + "does not carry expo-clipboard's web module at all: %s", + async (route) => { + const closure = await mobileWebAppRouteClosure(route) + expect(closure.modules.filter((file) => file.endsWith('ExpoClipboard.web.js'))).toEqual([]) + } + ) + + it.each([HUB, REVIEW])( + 'carries the seam that replaced it, so the absence above is not vacuous: %s', + async (route) => { + // An empty list is also what a closure reaching no clipboard code at all would produce. + const closure = await mobileWebAppRouteClosure(route) + expect(closure.local).toContain(CLIPBOARD_SEAM) + } + ) + }, + 240_000 +) diff --git a/config/scripts/mobile-web-app-tasks-external-links.test.mjs b/config/scripts/mobile-web-app-tasks-external-links.test.mjs index d52b9af39da..7c1af6f2105 100644 --- a/config/scripts/mobile-web-app-tasks-external-links.test.mjs +++ b/config/scripts/mobile-web-app-tasks-external-links.test.mjs @@ -11,15 +11,13 @@ * The rule, not the twelve call sites it happens to have today: a module entering this closure * later is held to it without anyone remembering to add it here. */ -import { readFileSync } from 'node:fs' -import { join } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' import { mobileWebAppRouteClosure } from './build-mobile-web-app-bundle.mjs' import { mobileWebAppDependenciesPresent } from './mobile-web-app-bundle-dependencies.mjs' import { EXTERNAL_LINK_SEAM as SEAM, - reachesReactNativeLinking + externalLinkOffenders } from './mobile-web-app-external-link-seam.mjs' const mobileDir = fileURLToPath(new URL('../../mobile/', import.meta.url)) @@ -30,18 +28,9 @@ describeClosure( () => { it('opens every external URL through the platform seam', async () => { const closure = await mobileWebAppRouteClosure('app/h/[hostId]/tasks.tsx') - const offenders = closure.local - .filter((file) => file !== SEAM) - .filter((file) => { - try { - // Which module the name comes from, not which text a call site writes: the tasks tree - // still calls `Linking.openURL`, and that `Linking` is the barrel's seam-backed export. - return reachesReactNativeLinking(readFileSync(join(mobileDir, file), 'utf8')) - } catch { - return false - } - }) - expect(offenders.sort()).toEqual([]) + // Which module the name comes from, not which text a call site writes: the tasks tree + // still calls `Linking.openURL`, and that `Linking` is the barrel's seam-backed export. + expect(externalLinkOffenders(mobileDir, closure)).toEqual([]) }) it('contains the seam, so the rule above is not vacuous', async () => { diff --git a/mobile/app/h/[hostId]/agent-history/[worktreeId].tsx b/mobile/app/h/[hostId]/agent-history/[worktreeId].tsx index f39273da7ab..18aa373b2d2 100644 --- a/mobile/app/h/[hostId]/agent-history/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/agent-history/[worktreeId].tsx @@ -1,7 +1,7 @@ import { useLocalSearchParams } from 'expo-router' -import { BridgeInitRouteSchema } from '../../../../src/mobile-web-shell/bridge/bridge-envelope' import { MobileAgentSessionHistoryPanel } from '../../../../src/agent-history/MobileAgentSessionHistoryPanel' import { MobileWebShellScreen } from '../../../../src/mobile-web-shell/MobileWebShellScreen' +import { shellScreenRoute } from '../../../../src/mobile-web-shell/shell-screen-route' import { useMobileWebShellEnabled } from '../../../../src/mobile-web-shell/use-mobile-web-shell-enabled' import { firstParam } from '../../../../src/source-control/mobile-source-control-screen-state' @@ -26,8 +26,8 @@ import { firstParam } from '../../../../src/source-control/mobile-source-control * * The schema is the predicate rather than a copy of its bounds: two spellings of one rule drift, * and the half that matters is the half the page reads. C3.1 made the same call for the files - * routes in `mobile-file-shell-route.ts`; once both are on main the two belong in one module - * beside the schema, which is a contract file the C2 lane owns today. + * routes first, and every switch now asks the one module beside the schema + * rather than carrying its own copy of the call. */ export default function MobileAgentSessionHistoryScreen() { const params = useLocalSearchParams<{ @@ -46,13 +46,13 @@ export default function MobileAgentSessionHistoryScreen() { if (enabled !== true || !hostId || !worktreeId) { return panel } - const route = { + const route = shellScreenRoute({ pathname: `/h/${encodeURIComponent(hostId)}/agent-history/${encodeURIComponent(worktreeId)}`, // Omitted rather than empty: the page reads the label off the search half, and a `name=` // with nothing after it is a label, where an absent one lets the panel derive its own. ...(name === '' ? {} : { params: { name } }) - } - if (!BridgeInitRouteSchema.safeParse(route).success) { + }) + if (route === null) { return panel } // Keyed on the route: a host captures the grants its session was opened with, so a screen diff --git a/mobile/app/h/[hostId]/files/[worktreeId].tsx b/mobile/app/h/[hostId]/files/[worktreeId].tsx index bfdcafbfdb6..7e26d95ea4a 100644 --- a/mobile/app/h/[hostId]/files/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/files/[worktreeId].tsx @@ -2,9 +2,9 @@ import { useLocalSearchParams } from 'expo-router' import { MobileFileExplorerPanel } from '../../../../src/files/MobileFileExplorerPanel' import { firstParam } from '../../../../src/source-control/mobile-source-control-screen-state' import { - mobileFileShellRoute, - mobileFileShellRouteKey -} from '../../../../src/files/mobile-file-shell-route' + shellScreenRoute, + shellScreenRouteKey +} from '../../../../src/mobile-web-shell/shell-screen-route' import { MobileWebShellScreen } from '../../../../src/mobile-web-shell/MobileWebShellScreen' import { useMobileWebShellEnabled } from '../../../../src/mobile-web-shell/use-mobile-web-shell-enabled' @@ -39,7 +39,7 @@ export default function MobileFileExplorerScreen() { const route = hostId && worktreeId - ? mobileFileShellRoute({ + ? shellScreenRoute({ pathname: `/h/${encodeURIComponent(hostId)}/files/${encodeURIComponent(worktreeId)}`, // Omitted rather than empty: the panel derives its own label from the worktree id when // the caller named none, where `name=` with nothing after it is a label. @@ -55,7 +55,7 @@ export default function MobileFileExplorerScreen() { // left. The key is what makes the change a remount, which disposes that bridge in the commit. return ( const shellRoute = route.ok - ? mobileFileShellRoute({ + ? shellScreenRoute({ pathname: `/h/${encodeURIComponent(route.params.hostId)}/files/preview/${encodeURIComponent( route.params.worktreeId )}`, @@ -63,7 +63,7 @@ export default function MobileFilePreviewRoute() { // was opened on, with nothing to tell it otherwise. return ( () + // Through `firstParam`, as the other four switches do: expo-router answers a repeated key with + // an array, and a bare read puts it straight into the template, where `String(['a','b'])` is + // `a,b` and `encodeURIComponent` makes it the single segment `a%2Cb` — which the bridge's + // segment rule accepts, so the shell would open a page for a host nobody has. An empty array is + // truthy, so a bare read also builds `/h/` and hands that over; this answers `''` and stays. + const params = useLocalSearchParams<{ hostId?: string | string[] }>() + const hostId = firstParam(params.hostId) const enabled = useMobileWebShellEnabled() - if (enabled !== true || !hostId) { + // Asked here as every switch asks it: encoding does not save a `.` or `..` host id, which fails + // the bridge's segment rule, and handing that over paints the page's failure screen over the + // native list this route already has. + const route = shellScreenRoute({ pathname: `/h/${encodeURIComponent(hostId)}` }) + + if (enabled !== true || !hostId || route === null) { return } return ( @@ -33,7 +46,7 @@ function HostListScreen() { // so a host id change must be a remount rather than a prop update. key={hostId} hostId={hostId} - route={{ pathname: `/h/${encodeURIComponent(hostId)}` }} + route={route} fallback={} /> ) diff --git a/mobile/app/h/[hostId]/tasks.tsx b/mobile/app/h/[hostId]/tasks.tsx index 165ab4226db..54af8c3d422 100644 --- a/mobile/app/h/[hostId]/tasks.tsx +++ b/mobile/app/h/[hostId]/tasks.tsx @@ -1,6 +1,6 @@ import { useLocalSearchParams } from 'expo-router' -import { BridgeInitRouteSchema } from '../../../src/mobile-web-shell/bridge/bridge-envelope' import { MobileWebShellScreen } from '../../../src/mobile-web-shell/MobileWebShellScreen' +import { shellScreenRoute } from '../../../src/mobile-web-shell/shell-screen-route' import { useMobileWebShellEnabled } from '../../../src/mobile-web-shell/use-mobile-web-shell-enabled' import { firstParam } from '../../../src/source-control/mobile-source-control-screen-state' import { MobileTasksScreen } from '../../../src/tasks/MobileTasksScreen' @@ -27,13 +27,13 @@ export default function MobileTasksRoute() { if (enabled !== true || !hostId) { return native } - const route = { + const route = shellScreenRoute({ pathname: `/h/${encodeURIComponent(hostId)}/tasks`, // Omitted rather than empty: an absent provider lets the page pick its own default, where // `taskSource=` is a provider named nothing. ...(taskSource === '' ? {} : { params: { taskSource } }) - } - if (!BridgeInitRouteSchema.safeParse(route).success) { + }) + if (route === null) { return native } return ( diff --git a/mobile/src/components/pr-sidebar/CommentMarkdown.tsx b/mobile/src/components/pr-sidebar/CommentMarkdown.tsx index 0fcc49008c2..531ff001814 100644 --- a/mobile/src/components/pr-sidebar/CommentMarkdown.tsx +++ b/mobile/src/components/pr-sidebar/CommentMarkdown.tsx @@ -1,5 +1,6 @@ import { useMemo, useState } from 'react' -import { Linking, Pressable, ScrollView, StyleSheet, Text, View } from 'react-native' +import { Pressable, ScrollView, StyleSheet, Text, View } from 'react-native' +import { openExternalLink } from '../../platform/external-link' import { ChevronDown, ChevronRight } from 'lucide-react-native' import { colors, radii, spacing, typography } from '../../theme/mobile-theme' import { MermaidDiagram } from './MermaidDiagram' @@ -145,7 +146,7 @@ function openMarkdownLink(url: string): void { if (!isAllowedMarkdownLinkUrl(url)) { return } - void Linking.openURL(url).catch(() => {}) + openExternalLink(url) } function alignToFlex(align: CellAlign | undefined): 'flex-start' | 'center' | 'flex-end' { diff --git a/mobile/src/components/pr-sidebar/PRChecksSection.tsx b/mobile/src/components/pr-sidebar/PRChecksSection.tsx index 95dfab4326e..881c3c60bde 100644 --- a/mobile/src/components/pr-sidebar/PRChecksSection.tsx +++ b/mobile/src/components/pr-sidebar/PRChecksSection.tsx @@ -1,5 +1,6 @@ import { useCallback, useEffect, useRef, useState } from 'react' -import { ActivityIndicator, Linking, Pressable, Text, View } from 'react-native' +import { ActivityIndicator, Pressable, Text, View } from 'react-native' +import { openExternalLink } from '../../platform/external-link' import { ChevronDown, ChevronRight, ExternalLink, RotateCw, Sparkles } from 'lucide-react-native' import { colors } from '../../theme/mobile-theme' import type { PRCheckDetail } from '../../../../src/shared/github/check-types' @@ -233,7 +234,7 @@ export function PRChecksSection({ {url ? ( void Linking.openURL(url).catch(() => {})} + onPress={() => openExternalLink(url)} hitSlop={6} accessibilityRole="button" accessibilityLabel={`Open ${check.name} on the web`} diff --git a/mobile/src/components/pr-sidebar/PRCommentCard.tsx b/mobile/src/components/pr-sidebar/PRCommentCard.tsx index 051f95877f1..224f8c08f12 100644 --- a/mobile/src/components/pr-sidebar/PRCommentCard.tsx +++ b/mobile/src/components/pr-sidebar/PRCommentCard.tsx @@ -1,5 +1,6 @@ import { memo, useState } from 'react' -import { Image, Linking, Pressable, Text, View } from 'react-native' +import { Image, Pressable, Text, View } from 'react-native' +import { openExternalLink } from '../../platform/external-link' import { Check, CornerDownRight, ExternalLink, Pencil, Trash2, Undo2 } from 'lucide-react-native' import type { GitHubReaction, @@ -140,7 +141,7 @@ export const PRCommentCard = memo(function PRCommentCard({ {comment.url ? ( void Linking.openURL(comment.url).catch(() => {})} + onPress={() => openExternalLink(comment.url)} hitSlop={8} accessibilityRole="button" accessibilityLabel="Open comment on GitHub" diff --git a/mobile/src/components/pr-sidebar/PRConflictingFilesSection.test.ts b/mobile/src/components/pr-sidebar/PRConflictingFilesSection.test.ts new file mode 100644 index 00000000000..d8ebfb67fcd --- /dev/null +++ b/mobile/src/components/pr-sidebar/PRConflictingFilesSection.test.ts @@ -0,0 +1,106 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { PRInfo } from '../../../../src/shared/github/pull-request-types' +import { PRConflictingFilesSection } from './PRConflictingFilesSection' +import { buildMergeabilityRefreshCommands } from './pr-conflict-presentation' + +const clipboard = vi.hoisted(() => ({ writeText: vi.fn() })) + +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + Pressable: 'Pressable', + ScrollView: 'ScrollView', + StyleSheet: { create: (styles: unknown) => styles }, + Text: 'Text', + View: 'View' +})) + +vi.mock('lucide-react-native', () => ({ + Check: 'Check', + Copy: 'Copy', + FileWarning: 'FileWarning', + Sparkles: 'Sparkles' +})) + +vi.mock('../../platform/clipboard', () => ({ useClipboardWriter: () => clipboard })) + +vi.mock('./PRSection', () => ({ + PRSection: ({ children }: { children: unknown }) => children +})) + +/** + * A PR the resolver gives the refresh commands to, which is the only path that copies: the host + * reports CONFLICTING, has no file list to show, and the local merge came back clean — the + * disagreement the commands exist to resolve. + */ +const PR: Pick = { + mergeable: 'CONFLICTING', + conflictSummary: { + files: [], + localMergeState: 'clean', + commitsBehind: 2, + baseCommit: 'abc1234', + baseRef: 'main' + } +} + +const COMMANDS = buildMergeabilityRefreshCommands() + +function press(tree: ReactTestRenderer): Promise { + const control = tree.root + .findAll((node) => node.props.accessibilityLabel === 'Copy mergeability refresh commands') + .at(0) + if (!control) { + throw new Error('the copy control is not rendered') + } + return act(async () => control.props.onPress()) +} + +function labels(tree: ReactTestRenderer): string[] { + return tree.root + .findAll((node) => typeof node.props.children === 'string') + .flatMap((node) => (typeof node.props.children === 'string' ? [node.props.children] : [])) +} + +describe('copying the mergeability refresh commands', () => { + let tree: ReactTestRenderer | null = null + + beforeEach(() => { + clipboard.writeText.mockReset().mockResolvedValue(undefined) + }) + + afterEach(() => { + act(() => tree?.unmount()) + tree = null + }) + + async function render(): Promise { + let rendered: ReactTestRenderer | null = null + await act(async () => { + rendered = create(createElement(PRConflictingFilesSection, { pr: PR })) + }) + if (rendered === null) { + throw new Error('the section did not render') + } + tree = rendered + return rendered + } + + it('says it copied when the pasteboard took the commands', async () => { + const rendered = await render() + await press(rendered) + expect(clipboard.writeText).toHaveBeenCalledWith(COMMANDS) + expect(labels(rendered)).toContain('Copied') + }) + + it('says it failed instead of saying nothing at all', async () => { + // The seam rejects when the pasteboard refused, which inside the page is a route that was not + // granted the verb. Dropped, the tap is indistinguishable from one that copied nothing. + clipboard.writeText.mockRejectedValue(new Error('the clipboard did not accept this text')) + const rendered = await render() + await press(rendered) + expect(labels(rendered)).toContain('Failed to copy text') + expect(labels(rendered)).not.toContain('Copied') + }) +}) diff --git a/mobile/src/components/pr-sidebar/PRConflictingFilesSection.tsx b/mobile/src/components/pr-sidebar/PRConflictingFilesSection.tsx index 0a95854d4fd..badb33a6fd0 100644 --- a/mobile/src/components/pr-sidebar/PRConflictingFilesSection.tsx +++ b/mobile/src/components/pr-sidebar/PRConflictingFilesSection.tsx @@ -1,7 +1,7 @@ import { useEffect, useRef, useState } from 'react' import { ActivityIndicator, Pressable, ScrollView, Text, View } from 'react-native' -import * as Clipboard from 'expo-clipboard' import { Check, Copy, FileWarning, Sparkles } from 'lucide-react-native' +import { useClipboardWriter } from '../../platform/clipboard' import { colors } from '../../theme/mobile-theme' import type { PRInfo } from '../../../../src/shared/github/pull-request-types' import { PRSection } from './PRSection' @@ -17,7 +17,9 @@ export type PrConflictsTriage = { } type Props = { - pr: PRInfo + // What it reads, not the whole PR: the conflict view-model is the only thing derived here, and + // a caller holding a full `PRInfo` satisfies this. + pr: Pick // True while a refresh is in flight, so the fallback notice can explain that // missing conflict file details may still be loading (desktop parity). isRefreshing?: boolean @@ -29,7 +31,12 @@ type Props = { // list is not yet available. Ports the desktop ConflictingFilesSection + // MergeConflictNotice into the mobile card shell. export function PRConflictingFilesSection({ pr, isRefreshing = false, triage }: Props) { - const [commandsCopied, setCommandsCopied] = useState(false) + // The seam, not `expo-clipboard`: inside the shell the page's own clipboard needs a secure + // context, which the iOS custom scheme is not and Android's https is. + const clipboard = useClipboardWriter() + // Three states, not a boolean: a refused write used to be caught and dropped, so the tap was + // indistinguishable from one that copied. The tasks page reports its refusals the same way. + const [copyState, setCopyState] = useState<'idle' | 'copied' | 'failed'>('idle') const copiedResetTimerRef = useRef | null>(null) const conflict = resolveConflictDisplay(pr) @@ -56,21 +63,29 @@ export function PRConflictingFilesSection({ pr, isRefreshing = false, triage }: if (!conflict.mergeabilityRefreshCommands) { return } + let next: 'copied' | 'failed' = 'copied' try { - await Clipboard.setStringAsync(conflict.mergeabilityRefreshCommands) + await clipboard.writeText(conflict.mergeabilityRefreshCommands) } catch { - return + next = 'failed' } if (copiedResetTimerRef.current) { clearTimeout(copiedResetTimerRef.current) } - setCommandsCopied(true) + setCopyState(next) copiedResetTimerRef.current = setTimeout(() => { copiedResetTimerRef.current = null - setCommandsCopied(false) + setCopyState('idle') }, 1500) } + const copyLabel = + copyState === 'copied' + ? 'Copied' + : copyState === 'failed' + ? 'Failed to copy text' + : 'Copy commands' + return ( {conflict.commitsBehind !== null && conflict.baseCommit !== null ? ( @@ -97,14 +112,12 @@ export function PRConflictingFilesSection({ pr, isRefreshing = false, triage }: accessibilityRole="button" accessibilityLabel="Copy mergeability refresh commands" > - {commandsCopied ? ( + {copyState === 'copied' ? ( ) : ( )} - - {commandsCopied ? 'Copied' : 'Copy commands'} - + {copyLabel} diff --git a/mobile/src/files/files-router-seam-census.test.ts b/mobile/src/files/files-router-seam-census.test.ts index e1b4546ec67..360b040b2df 100644 --- a/mobile/src/files/files-router-seam-census.test.ts +++ b/mobile/src/files/files-router-seam-census.test.ts @@ -1,77 +1,34 @@ -import { readdirSync, readFileSync } from 'node:fs' -import { join } from 'node:path' -import ts from 'typescript-api' import { describe, expect, it } from 'vitest' +import { + callsRouteHandoff, + importsExpoRouterValue, + parse, + productFiles +} from '../navigation/router-seam-census.test-support' const FILES_ROOT = import.meta.dirname /** * Which modules here hold a router, so the census cannot pass by seeing nothing. * - * Inside the shell's page a screen is one document standing in for one screen, and `useRouteHandoff` - * is the only thing that knows which targets the page keeps and which it hands back to the app. A - * screen holding expo-router's own `useRouter` posts no `navigate`, so a target outside the page - * paints Unmatched over it and a target inside it still works — which is why this is a census and - * not a behaviour test: the failure is invisible from either screen's own tests. + * The walk and the two rules are the seam's, shared with every other domain that runs them; what + * stays here is this domain's own evidence: which of its modules are meant to hold a router. */ const ROUTER_HOLDERS = ['MobileFilePreviewScreen.tsx', 'MobileFileExplorerPanel.tsx'] -function productFiles(): string[] { - return readdirSync(FILES_ROOT, { recursive: true, encoding: 'utf8' }) - .map((entry) => entry.replaceAll('\\', '/')) - .filter((entry) => /\.tsx?$/.test(entry) && !/\.test\.tsx?$/.test(entry)) -} - -function parse(name: string): ts.SourceFile { - return ts.createSourceFile( - name, - readFileSync(join(FILES_ROOT, name), 'utf8'), - ts.ScriptTarget.Latest, - true, - name.endsWith('.tsx') ? ts.ScriptKind.TSX : ts.ScriptKind.TS - ) -} - -/** Value imports only: a `import type { Href } from 'expo-router'` names no runtime router. */ -function importsExpoRouterValue(source: ts.SourceFile): boolean { - return source.statements.some((statement) => { - if (!ts.isImportDeclaration(statement) || statement.importClause?.isTypeOnly === true) { - return false - } - const specifier = statement.moduleSpecifier - return ts.isStringLiteral(specifier) && specifier.text === 'expo-router' - }) -} - -function callsRouteHandoff(source: ts.SourceFile): boolean { - let found = false - const visit = (node: ts.Node): void => { - if ( - ts.isCallExpression(node) && - ts.isIdentifier(node.expression) && - node.expression.text === 'useRouteHandoff' - ) { - found = true - } - ts.forEachChild(node, visit) - } - ts.forEachChild(source, visit) - return found -} - describe('the files domain reaches the router through the handoff seam', () => { - const files = productFiles() + const files = productFiles(FILES_ROOT) it('walks the modules it is written against', () => { expect(files).toEqual(expect.arrayContaining(ROUTER_HOLDERS)) }) it('imports no router from expo-router, which the page cannot hand a route back through', () => { - expect(files.filter((name) => importsExpoRouterValue(parse(name)))).toEqual([]) + expect(files.filter((name) => importsExpoRouterValue(parse(FILES_ROOT, name)))).toEqual([]) }) it('takes the router from useRouteHandoff at every screen that holds one', () => { - expect(files.filter((name) => callsRouteHandoff(parse(name))).sort()).toEqual( + expect(files.filter((name) => callsRouteHandoff(parse(FILES_ROOT, name))).sort()).toEqual( [...ROUTER_HOLDERS].sort() ) }) diff --git a/mobile/src/files/mobile-file-path-route-encoding.test.ts b/mobile/src/files/mobile-file-path-route-encoding.test.ts index 31a101c15f4..f72a7e74b05 100644 --- a/mobile/src/files/mobile-file-path-route-encoding.test.ts +++ b/mobile/src/files/mobile-file-path-route-encoding.test.ts @@ -6,7 +6,7 @@ import { import { shellRouteHref } from '../mobile-web-shell/bridge/page-bootstrap' import { stringifyRouteHref } from '../navigation/route-href' import { createMobileFilePreviewHref } from './mobile-file-preview-route' -import { mobileFileShellRoute } from './mobile-file-shell-route' +import { shellScreenRoute } from '../mobile-web-shell/shell-screen-route' /** * Every shape of a real file path that the bridge's route vocabulary would refuse as a segment. @@ -59,7 +59,7 @@ function relativePathFromHref(href: string): string | null { describe.each(HAZARD_PATHS)('a file path the route carries: %s', (relativePath) => { it('is a route the page can be given, and a pathname with no path in it', () => { - const route = mobileFileShellRoute({ + const route = shellScreenRoute({ pathname: '/h/host-1/files/preview/wt-1', params: { relativePath, source: 'worktree' } }) diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx index c93a835d9b7..3d96627cc74 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx +++ b/mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx @@ -5,13 +5,15 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' type RouteDependencies = { storage: Map pathnames: string[] - hostId: string + hostId: string | string[] + nativeRenders: number } const dependencies = vi.hoisted((): RouteDependencies => ({ storage: new Map(), pathnames: [], - hostId: 'host-1' + hostId: 'host-1', + nativeRenders: 0 })) vi.mock('@react-native-async-storage/async-storage', () => ({ @@ -31,7 +33,27 @@ vi.mock('../components/WorkspaceDetailPlaceholder', () => ({ WorkspaceDetailPlaceholder: () => null })) -vi.mock('../host-screen/HostScreen', () => ({ HostScreen: () => null })) +vi.mock('../host-screen/HostScreen', () => ({ + HostScreen: () => { + dependencies.nativeRenders += 1 + return null + } +})) + +// `firstParam` lives beside the source-control screen state, which imports the lucide barrel, and +// that barrel's `LucideProvider` re-export is the gap the web build patches with a plugin. Nine +// icons, named as the other suites name theirs; none of them renders here. +vi.mock('lucide-react-native', () => ({ + ArrowDown: vi.fn(), + ArrowDownUp: vi.fn(), + ArrowUp: vi.fn(), + Check: vi.fn(), + CloudUpload: vi.fn(), + GitBranch: vi.fn(), + GitPullRequestArrow: vi.fn(), + History: vi.fn(), + RefreshCw: vi.fn() +})) vi.mock('../layout/responsive-layout', () => ({ useResponsiveLayout: () => ({ isWideLayout: false }) @@ -57,6 +79,7 @@ describe('the native worktree-list route that hands off to the shell', () => { beforeEach(() => { dependencies.storage.clear() dependencies.pathnames.length = 0 + dependencies.nativeRenders = 0 dependencies.hostId = 'host-1' Object.assign(globalThis, { __DEV__: true }) dependencies.storage.set('orca:mobileWebShellEnabled', 'true') @@ -73,4 +96,36 @@ describe('the native worktree-list route that hands off to the shell', () => { expect(decodeURIComponent((pathname ?? '').slice('/h/'.length)), hostId).toBe(hostId) } }) + + it('keeps a dot-segment host id native instead of handing over a route the page refuses', async () => { + // `encodeURIComponent` leaves a dot alone and `%2e%2e` is a dot segment to the URL parser too, + // so this one cannot be encoded into a pathname the bridge accepts. Handed over it reaches the + // phone as an `init` naming no screen and the page paints "Update Orca to open this + // workspace" over the native list that is sitting right behind this switch. + for (const hostId of ['..', '.']) { + dependencies.hostId = hostId + dependencies.pathnames.length = 0 + dependencies.nativeRenders = 0 + await renderRoute() + expect(BRIDGE_ROUTE_PATHNAME_PATTERN.test(`/h/${hostId}`), hostId).toBe(false) + expect(dependencies.pathnames, hostId).toEqual([]) + expect(dependencies.nativeRenders, hostId).toBeGreaterThan(0) + } + }) + + it('opens the first of a repeated host id, never the pair joined into one', async () => { + // Expo Router answers a repeated key with an array. Interpolated, `String(['a','b'])` is + // `a,b` and `encodeURIComponent` makes that the single segment `a%2Cb`, which the bridge's + // segment rule accepts — so the shell would open a page for a host nobody has. + dependencies.hostId = ['host-1', 'host-2'] + await renderRoute() + expect(dependencies.pathnames).toEqual(['/h/host-1']) + }) + + it('stays native for an empty repeated host id, which names no host at all', async () => { + dependencies.hostId = [] + await renderRoute() + expect(dependencies.pathnames).toEqual([]) + expect(dependencies.nativeRenders).toBeGreaterThan(0) + }) }) diff --git a/mobile/src/mobile-web-shell/shell-screen-route-census.test.ts b/mobile/src/mobile-web-shell/shell-screen-route-census.test.ts new file mode 100644 index 00000000000..dad3840549f --- /dev/null +++ b/mobile/src/mobile-web-shell/shell-screen-route-census.test.ts @@ -0,0 +1,169 @@ +import { readdirSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import ts from 'typescript-api' +import { describe, expect, it } from 'vitest' + +const HOST_ROUTES = join(import.meta.dirname, '..', '..', 'app', 'h', '[hostId]') +const PAGE_ROUTE_REGISTRY = join( + import.meta.dirname, + '..', + '..', + '..', + 'config', + 'scripts', + 'mobile-web-page-routes.mjs' +) + +/** + * Every switch that hands a route to the shell asks whether the route is one the page can be + * given, and asks it in one place. + * + * A route the schema refuses is dropped to `null` by `bridge-host.ts` and reaches the phone as an + * `init` naming no screen, which the page answers with "Update Orca to open this workspace" — a + * failure screen in place of the native screen sitting right behind the switch. Three routes had + * each grown their own copy of the call and two had none at all, which is the state this census + * ends: the predicate is `shellScreenRoute`, and a switch that spells it itself has a second + * spelling of a rule that can only drift from the one the page reads. + * + * The walk is over the route tree rather than a list, so a route added later is held to this + * without anyone remembering to add it here. + */ +function hostRouteFiles(directory: string = HOST_ROUTES, prefix = ''): string[] { + return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { + const name = prefix === '' ? entry.name : `${prefix}/${entry.name}` + if (entry.isDirectory()) { + return hostRouteFiles(join(directory, entry.name), name) + } + return entry.name.endsWith('.tsx') && !entry.name.includes('.test.') ? [name] : [] + }) +} + +const read = (name: string): string => readFileSync(join(HOST_ROUTES, name), 'utf8') + +/** + * The one switch that hands over a route the rule refuses, on purpose. + * + * `web.tsx` is `__DEV__`-only and its fallback is `Redirect href="/h/"`, not a native + * screen. Adopting the guard there sends a `..` deep link through that redirect to the host route, + * which this PR keeps native, so the developer lands on the host list with nothing said about why + * the page did not open. Handed over instead, the same id reaches the bridge and comes back as the + * host's own failure screen, which is the better verdict for a route whose whole purpose is to + * open the page deliberately; `mobile-web-shell-route.test.tsx` pins that by name. + * + * Exempted here rather than silently unwalked, so the exception is read when it changes. + */ +const HANDS_OVER_UNJUDGED = ['web.tsx'] + +const parse = (source: string): ts.SourceFile => + ts.createSourceFile('route.tsx', source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX) + +function importsNames(parsed: ts.SourceFile, module: string): string[] { + return parsed.statements.flatMap((statement) => { + if (!ts.isImportDeclaration(statement) || !ts.isStringLiteral(statement.moduleSpecifier)) { + return [] + } + if (!statement.moduleSpecifier.text.endsWith(module)) { + return [] + } + const bindings = statement.importClause?.namedBindings + return bindings !== undefined && ts.isNamedImports(bindings) + ? bindings.elements.map((element) => element.name.text) + : [] + }) +} + +/** + * A switch is a route file that imports the shell screen. + * + * The import rather than ` { + if ( + ts.isCallExpression(node) && + ts.isIdentifier(node.expression) && + node.expression.text === name + ) { + called = true + } + ts.forEachChild(node, visit) + } + ts.forEachChild(parsed, visit) + return called +} + +describe('the switches that hand a route to the shell', () => { + const switches = hostRouteFiles().filter((name) => mountsShell(parse(read(name)))) + + it('walks the directory every registered route lives under', () => { + // The root above is written out rather than derived, so this is what ties it to the manifest: + // a page route outside `/h/[hostId]` would be a switch this census never reads. + const pathnames = [ + ...readFileSync(PAGE_ROUTE_REGISTRY, 'utf8').matchAll(/pathname: '([^']+)'/g) + ].map((match) => match[1]) + expect(pathnames.length).toBeGreaterThan(0) + expect(pathnames.filter((pathname) => !pathname.startsWith('/h/[hostId]'))).toEqual([]) + }) + + it('walks the route tree and finds them, so the rules below cannot pass vacuously', () => { + expect(switches.sort()).toEqual([ + 'agent-history/[worktreeId].tsx', + 'files/[worktreeId].tsx', + 'files/preview/[worktreeId].tsx', + 'index.tsx', + 'tasks.tsx', + 'web.tsx' + ]) + }) + + it('asks shellScreenRoute whether the route is one the page can be given', () => { + // Imports it *and* calls it: an import the call no longer reaches is a switch that stopped + // asking while still looking like one. + expect( + switches + .filter((name) => !HANDS_OVER_UNJUDGED.includes(name)) + .filter((name) => { + const parsed = parse(read(name)) + return ( + !importsNames(parsed, 'shell-screen-route').includes('shellScreenRoute') || + !callsName(parsed, 'shellScreenRoute') + ) + }) + ).toEqual([]) + }) + + it('reads the call rather than the import, on a fixture that has only the import', () => { + // The rule the case above cannot show against the tree, every switch there calling what it + // imports: an import with no call is named. + const importOnly = parse( + "import { shellScreenRoute } from '../../../src/mobile-web-shell/shell-screen-route'\n" + + "import { MobileWebShellScreen } from '../../../src/mobile-web-shell/MobileWebShellScreen'\n" + + 'export default function Route() {\n return \n}\n' + ) + expect(mountsShell(importOnly)).toBe(true) + expect(importsNames(importOnly, 'shell-screen-route')).toContain('shellScreenRoute') + expect(callsName(importOnly, 'shellScreenRoute')).toBe(false) + }) + + it('spells the rule nowhere else, so the page and the app cannot disagree about it', () => { + // The copies this census ends. `shellScreenRoute` is the one caller of the schema outside the + // bridge, and a switch that reaches for it again is writing the second spelling back. + expect(switches.filter((name) => read(name).includes('BridgeInitRouteSchema'))).toEqual([]) + // And the exemption names a switch that exists, so it cannot outlive the file it excuses. + expect(switches).toEqual(expect.arrayContaining(HANDS_OVER_UNJUDGED)) + }) +}) diff --git a/mobile/src/files/mobile-file-shell-route.test.ts b/mobile/src/mobile-web-shell/shell-screen-route.test.ts similarity index 79% rename from mobile/src/files/mobile-file-shell-route.test.ts rename to mobile/src/mobile-web-shell/shell-screen-route.test.ts index 2beeaede98e..9f3644891ed 100644 --- a/mobile/src/files/mobile-file-shell-route.test.ts +++ b/mobile/src/mobile-web-shell/shell-screen-route.test.ts @@ -1,15 +1,12 @@ import { describe, expect, it } from 'vitest' -import { BRIDGE_MAX_ROUTE_PARAM_CHARS } from '../mobile-web-shell/bridge/bridge-caps' -import { - BridgeInitRouteSchema, - type BridgeInitRoute -} from '../mobile-web-shell/bridge/bridge-envelope' -import { shellRouteHref } from '../mobile-web-shell/bridge/page-bootstrap' -import { mobileFileShellRoute, mobileFileShellRouteKey } from './mobile-file-shell-route' +import { BRIDGE_MAX_ROUTE_PARAM_CHARS } from './bridge/bridge-caps' +import { BridgeInitRouteSchema, type BridgeInitRoute } from './bridge/bridge-envelope' +import { shellRouteHref } from './bridge/page-bootstrap' +import { shellScreenRoute, shellScreenRouteKey } from './shell-screen-route' import { mobileFilePreviewShellParams, normalizeMobileFilePreviewRouteParams -} from './mobile-file-preview-route' +} from '../files/mobile-file-preview-route' const PREVIEW_PATH = '/h/host-1/files/preview/wt-1' @@ -27,11 +24,11 @@ function previewRoute(absolutePath: string) { return { pathname: PREVIEW_PATH, params: mobileFilePreviewShellParams(route.params) } } -describe('the route the files screens hand the shell', () => { +describe('the route a switch hands the shell', () => { it('is one the page could actually be given', () => { const route = previewRoute('/logs/run.txt') expect(BridgeInitRouteSchema.safeParse(route).success).toBe(true) - expect(mobileFileShellRoute(route)).toEqual(route) + expect(shellScreenRoute(route)).toEqual(route) }) it('is nothing when a file path is longer than a param may be', () => { @@ -41,7 +38,7 @@ describe('the route the files screens hand the shell', () => { // workspace" over a native screen that works. const route = previewRoute(`/logs/${'a'.repeat(BRIDGE_MAX_ROUTE_PARAM_CHARS)}.txt`) expect(BridgeInitRouteSchema.safeParse(route).success).toBe(false) - expect(mobileFileShellRoute(route)).toBeNull() + expect(shellScreenRoute(route)).toBeNull() }) it('is nothing when a worktree id is not a segment the page will route', () => { @@ -51,7 +48,7 @@ describe('the route the files screens hand the shell', () => { // The schema first, as the length case does: without it a `null` here would also be what a // guard that refused everything produces. expect(BridgeInitRouteSchema.safeParse(route).success).toBe(false) - expect(mobileFileShellRoute(route)).toBeNull() + expect(shellScreenRoute(route)).toBeNull() }) it('keeps a path with a slash, a space and a dot segment, which are params and not segments', () => { @@ -59,7 +56,7 @@ describe('the route the files screens hand the shell', () => { pathname: '/h/host-1/files/preview/wt-1', params: { relativePath: 'docs/../my notes/readme.md', source: 'worktree' } } - expect(mobileFileShellRoute(route)).toEqual(route) + expect(shellScreenRoute(route)).toEqual(route) }) }) @@ -77,6 +74,6 @@ describe('the key a shell screen remounts on', () => { params: { relativePath: 'docs/my notes/readme.md', source: 'worktree', line: '12' } } ])('is the href the page would write into its history: %o', (route) => { - expect(mobileFileShellRouteKey(route)).toBe(shellRouteHref(route)) + expect(shellScreenRouteKey(route)).toBe(shellRouteHref(route)) }) }) diff --git a/mobile/src/files/mobile-file-shell-route.ts b/mobile/src/mobile-web-shell/shell-screen-route.ts similarity index 71% rename from mobile/src/files/mobile-file-shell-route.ts rename to mobile/src/mobile-web-shell/shell-screen-route.ts index 0c00505d35f..b4929b894e3 100644 --- a/mobile/src/files/mobile-file-shell-route.ts +++ b/mobile/src/mobile-web-shell/shell-screen-route.ts @@ -1,7 +1,4 @@ -import { - BridgeInitRouteSchema, - type BridgeInitRoute -} from '../mobile-web-shell/bridge/bridge-envelope' +import { BridgeInitRouteSchema, type BridgeInitRoute } from './bridge/bridge-envelope' /** * The route to hand the shell, or nothing if the page could not be given it. @@ -12,16 +9,17 @@ import { * native screen sitting right behind the switch. Deciding here instead means the route stays * native, which is where every route starts. * - * A file path is the reason this domain needs it. Paths are params, not segments, so `/`, spaces - * and `..` are all fine; length is not bounded by anything the user cannot exceed, and - * `BRIDGE_MAX_ROUTE_PARAM_CHARS` is 1024 while a Windows long path is not. The same call also - * catches a `worktreeId` the segment rule refuses, which is the C1.8 class. + * A file path is the reason the files routes needed it first. Paths are params, not segments, so + * `/`, spaces and `..` are all fine; length is not bounded by anything the user cannot exceed, + * and `BRIDGE_MAX_ROUTE_PARAM_CHARS` is 1024 while a Windows long path is not. The same call + * also catches a `worktreeId` the segment rule refuses, which is the C1.8 class, and a host id + * that encoding does not save — a `.` or `..` — which is why every switch asks it now. * * The schema itself is the predicate rather than a copy of its bounds: two spellings of one rule - * drift, and the half that matters is the half the page reads. This belongs in the shell beside - * that schema; it lives here while the contract files are the C2 lane's. + * drift, and the half that matters is the half the page reads. Here rather than in one domain + * because three routes had grown their own copy of the call. */ -export function mobileFileShellRoute(route: BridgeInitRoute): BridgeInitRoute | null { +export function shellScreenRoute(route: BridgeInitRoute): BridgeInitRoute | null { return BridgeInitRouteSchema.safeParse(route).success ? route : null } @@ -41,7 +39,7 @@ export function mobileFileShellRoute(route: BridgeInitRoute): BridgeInitRoute | * document channel, and a native route file must not pull those into the app. The test pins the * two equal instead, which is the dependency this comment actually has. */ -export function mobileFileShellRouteKey(route: BridgeInitRoute): string { +export function shellScreenRouteKey(route: BridgeInitRoute): string { const search = new URLSearchParams(route.params ?? {}).toString() return search === '' ? route.pathname : `${route.pathname}?${search}` } diff --git a/mobile/src/navigation/router-seam-census.test-support.ts b/mobile/src/navigation/router-seam-census.test-support.ts new file mode 100644 index 00000000000..41f974620eb --- /dev/null +++ b/mobile/src/navigation/router-seam-census.test-support.ts @@ -0,0 +1,61 @@ +import { readdirSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import ts from 'typescript-api' + +/** + * How a domain census reads which of its modules hold a router, and where each one got it. + * + * Inside the shell's page a screen is one document standing in for one screen, and + * `useRouteHandoff` is the only thing that knows which targets the page keeps and which it hands + * back to the app. A screen holding expo-router's own `useRouter` posts no `navigate`, so a target + * outside the page paints Unmatched over it and a target inside it still works — which is why this + * is a census and not a behaviour test: the failure is invisible from either screen's own tests. + * + * Shared by every domain that runs it rather than copied per domain: C3.1 wrote this walk for the + * files tree and the source-control tree wanted the same four rules, and two spellings of one rule + * drift apart in exactly the half nobody reads again. + */ + +/** Every product module under a domain root, as paths relative to it. */ +export function productFiles(root: string): string[] { + return readdirSync(root, { recursive: true, encoding: 'utf8' }) + .map((entry) => entry.replaceAll('\\', '/')) + .filter((entry) => /\.tsx?$/.test(entry) && !/\.test\.tsx?$/.test(entry)) +} + +export function parse(root: string, name: string): ts.SourceFile { + return ts.createSourceFile( + name, + readFileSync(join(root, name), 'utf8'), + ts.ScriptTarget.Latest, + true, + name.endsWith('.tsx') ? ts.ScriptKind.TSX : ts.ScriptKind.TS + ) +} + +/** Value imports only: an `import type { Href } from 'expo-router'` names no runtime router. */ +export function importsExpoRouterValue(source: ts.SourceFile): boolean { + return source.statements.some((statement) => { + if (!ts.isImportDeclaration(statement) || statement.importClause?.isTypeOnly === true) { + return false + } + const specifier = statement.moduleSpecifier + return ts.isStringLiteral(specifier) && specifier.text === 'expo-router' + }) +} + +export function callsRouteHandoff(source: ts.SourceFile): boolean { + let found = false + const visit = (node: ts.Node): void => { + if ( + ts.isCallExpression(node) && + ts.isIdentifier(node.expression) && + node.expression.text === 'useRouteHandoff' + ) { + found = true + } + ts.forEachChild(node, visit) + } + ts.forEachChild(source, visit) + return found +} diff --git a/mobile/src/session/use-mobile-diff-review-send-actions.test.ts b/mobile/src/session/use-mobile-diff-review-send-actions.test.ts index d84fa008be4..f763a8f3078 100644 --- a/mobile/src/session/use-mobile-diff-review-send-actions.test.ts +++ b/mobile/src/session/use-mobile-diff-review-send-actions.test.ts @@ -14,7 +14,10 @@ import { useMobileDiffReviewSendActions } from './use-mobile-diff-review-send-ac type SendActions = ReturnType vi.mock('../platform/haptics', () => ({ triggerSuccess: vi.fn() })) -vi.mock('expo-clipboard', () => ({ setStringAsync: vi.fn().mockResolvedValue(undefined) })) +// Resolving `true`, which is what the pasteboard answers when it took the text: the seam reads +// that boolean, and a mock resolving `undefined` put every copy down the refusal arm unseen. +const clipboardMock = vi.hoisted(() => ({ setStringAsync: vi.fn() })) +vi.mock('expo-clipboard', () => clipboardMock) function sendResponse(accepted: boolean) { return { @@ -52,6 +55,7 @@ describe('useMobileDiffReviewSendActions', () => { let saveCommentsAndReviewState: ReturnType beforeEach(() => { + clipboardMock.setStringAsync.mockReset().mockResolvedValue(true) resetMobileNativeChatStaleInputForTests() setActionError = vi.fn() setSendSheet = vi.fn() @@ -85,6 +89,34 @@ describe('useMobileDiffReviewSendActions', () => { }) } + /** Copying reaches no client, so the cases below mount without one rather than stubbing it. */ + async function mountWithoutClient(): Promise { + mountedClient = null + await act(async () => { + renderer = create(createElement(Harness)) + }) + } + + it('copies the notes through the platform seam and says so', async () => { + await mountWithoutClient() + await act(async () => { + await actions?.copyNotes() + }) + expect(clipboardMock.setStringAsync).toHaveBeenCalledOnce() + expect(setActionError).toHaveBeenLastCalledWith('Review notes copied') + }) + + it('reports a refused copy instead of claiming it copied', async () => { + // The pasteboard answering `false` is the case the seam exists to surface: on the web the verb + // is refused when the route was not granted it, and the only caller is a floating promise. + clipboardMock.setStringAsync.mockResolvedValue(false) + await mountWithoutClient() + await act(async () => { + await actions?.copyNotes() + }) + expect(setActionError).toHaveBeenLastCalledWith('the clipboard did not accept this text') + }) + it('heals a marked terminal BEFORE submitting the notes', async () => { const sendRequest = vi.fn().mockResolvedValue(sendResponse(true)) await mount({ sendRequest } as unknown as RpcClient) diff --git a/mobile/src/session/use-mobile-diff-review-send-actions.ts b/mobile/src/session/use-mobile-diff-review-send-actions.ts index 1fec27ed93a..8413cf61c36 100644 --- a/mobile/src/session/use-mobile-diff-review-send-actions.ts +++ b/mobile/src/session/use-mobile-diff-review-send-actions.ts @@ -1,8 +1,8 @@ import { useCallback, type Dispatch, type SetStateAction } from 'react' -import * as Clipboard from 'expo-clipboard' import type { DiffComment, MobileDiffReviewState } from '../../../src/shared/diff-comment-types' import type { ConnectionState } from '../transport/types' import type { RpcClient } from '../transport/rpc-client' +import { useClipboardWriter } from '../platform/clipboard' import { triggerSuccess } from '../platform/haptics' import { formatDiffComments, formatMobileDiffReviewPrompt } from './mobile-diff-comments' import { clearSentMobileDiffComments, markMobileDiffCommentsSent } from './mobile-diff-comment-edit' @@ -29,6 +29,9 @@ type SendActionsInput = { } export function useMobileDiffReviewSendActions(input: SendActionsInput) { + // The seam, not `expo-clipboard`: inside the shell the page's own clipboard needs a secure + // context, which the iOS custom scheme is not and Android's https is. + const clipboard = useClipboardWriter() const { client, connState, @@ -43,10 +46,17 @@ export function useMobileDiffReviewSendActions(input: SendActionsInput) { if (screenState.kind !== 'ready' || screenState.comments.length === 0) { return } - await Clipboard.setStringAsync(formatDiffComments(screenState.comments)) + // Caught here because the only caller is `void controller.copyNotes()`: the seam rejects when + // the pasteboard refused, and an uncaught rejection would leave "copied" as the last word. + try { + await clipboard.writeText(formatDiffComments(screenState.comments)) + } catch (err) { + setActionError(err instanceof Error ? err.message : 'Unable to copy the review notes') + return + } triggerSuccess() setActionError('Review notes copied') - }, [screenState, setActionError]) + }, [clipboard, screenState, setActionError]) const clearSentNotes = useCallback(async () => { if (screenState.kind !== 'ready') { diff --git a/mobile/src/source-control/source-control-router-seam-census.test.ts b/mobile/src/source-control/source-control-router-seam-census.test.ts new file mode 100644 index 00000000000..e34d4ad93d8 --- /dev/null +++ b/mobile/src/source-control/source-control-router-seam-census.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, it } from 'vitest' +import { + callsRouteHandoff, + importsExpoRouterValue, + parse, + productFiles +} from '../navigation/router-seam-census.test-support' + +const SOURCE_CONTROL_ROOT = import.meta.dirname + +/** + * Which modules here hold a router, so the census cannot pass by seeing nothing. + * + * One holder, not one per screen: the hub's router is taken once in the openers hook and passed + * down through the state hook to the runners and the panel. So this domain's whole reach into the + * router is that single call, and the rules below say so rather than counting screens. + * + * `use-mobile-source-control-runners.ts` is the case a value/type rule is written for: it named + * expo-router only to write `ReturnType`, which is a value import in a type + * position and keeps the module in the graph. `RouteHandoff` is the seam's own name for that type. + */ +const ROUTER_HOLDERS = ['use-mobile-source-control-openers.ts'] + +describe('the source-control domain reaches the router through the handoff seam', () => { + const files = productFiles(SOURCE_CONTROL_ROOT) + + it('walks the modules it is written against', () => { + expect(files).toEqual(expect.arrayContaining(ROUTER_HOLDERS)) + expect(files).toContain('use-mobile-source-control-runners.ts') + expect(files).toContain('MobileSourceControlPanel.tsx') + }) + + it('imports no router from expo-router, which the page cannot hand a route back through', () => { + expect( + files.filter((name) => importsExpoRouterValue(parse(SOURCE_CONTROL_ROOT, name))) + ).toEqual([]) + }) + + it('takes the router from useRouteHandoff at every screen that holds one', () => { + expect( + files.filter((name) => callsRouteHandoff(parse(SOURCE_CONTROL_ROOT, name))).sort() + ).toEqual([...ROUTER_HOLDERS].sort()) + }) +}) diff --git a/mobile/src/source-control/use-mobile-source-control-openers.ts b/mobile/src/source-control/use-mobile-source-control-openers.ts index 8b168d14230..3866b7b772b 100644 --- a/mobile/src/source-control/use-mobile-source-control-openers.ts +++ b/mobile/src/source-control/use-mobile-source-control-openers.ts @@ -1,5 +1,5 @@ import { useCallback, useRef, useState, type MutableRefObject } from 'react' -import { useRouter } from 'expo-router' +import { useRouteHandoff } from '../navigation/route-handoff' import type { RpcClient } from '../transport/rpc-client' import { refusedRpcMessageOrFallback } from '../transport/rpc-refusal-message' import type { ConnectionState } from '../transport/types' @@ -67,7 +67,9 @@ export function useMobileSourceControlOpeners(params: Params) { busyActionRef, setActionError } = params - const router = useRouter() + // The seam, not expo-router's own: inside the shell's page a push to a route the page does not + // render has to be handed back to the app, and only this knows which targets those are. + const router = useRouteHandoff() const [branchDiffPreview, setBranchDiffPreview] = useState( null ) diff --git a/mobile/src/source-control/use-mobile-source-control-runners.ts b/mobile/src/source-control/use-mobile-source-control-runners.ts index 026a706a5f9..4f4d31433af 100644 --- a/mobile/src/source-control/use-mobile-source-control-runners.ts +++ b/mobile/src/source-control/use-mobile-source-control-runners.ts @@ -1,5 +1,5 @@ import { useCallback, type MutableRefObject } from 'react' -import { useRouter } from 'expo-router' +import type { RouteHandoff } from '../navigation/route-handoff' import type { RpcClient } from '../transport/rpc-client' import { triggerError, triggerSuccess } from '../platform/haptics' import { useMobileCommitMessageGeneration } from './use-mobile-commit-message-generation' @@ -28,7 +28,7 @@ type Params = { generatingMessage: boolean stageablePaths: string[] unstageablePaths: string[] - router: ReturnType + router: RouteHandoff sendGitRequest: SendGitRequest sendCommitRequest: (message: string) => Promise runGitSyncSteps: () => Promise