mirror of
https://github.com/stablyai/orca.git
synced 2026-09-21 16:02:20 +00:00
Jinwoo HongandClaude
f3bda1bf3e
* refactor(mobile): open PR sidebar URLs through the external-link seam (OTA phase C, C4.1) The three openers in the PR sidebar called `Linking.openURL` directly: a check's "open on the web", a comment's permalink, and a link inside comment Markdown. Inside the shell's WebView react-native-web routes that to `window.open(url, '_blank', 'noopener')`, which both shells refuse and which resolves anyway, so the tap reports success and opens nothing. Both C4 routes reach the sidebar, so both would have shipped that. The census is the point rather than the three edits. It derives the two future route closures through `mobileWebAppRouteClosure` and holds every module in them to the seam, so a module entering either closure later is ruled without anyone adding it here. Red first it named all three by `path:line`: CommentMarkdown.tsx:2, PRChecksSection.tsx:2, PRCommentCard.tsx:2, on both routes. The walk it runs was the third copy of one function, so it moves into the seam's own module beside the predicate that module exists to share, and the files and tasks censuses now call it too. It reports `path:line` where the copies reported paths; `reachesReactNativeLinking` keeps its name and its meaning and is now derived from the line list, so there is one rule. An empty offender list is empty in either shape, which is why repointing the two landed censuses moves nothing they assert. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): copy through the platform clipboard seam in review and conflicts (OTA phase C, C4.1) The two copy actions both C4 routes reach called `expo-clipboard` directly: the conflict section's refresh commands and the review sheet's notes. On the web that module is `navigator.clipboard`, which needs a secure context — the iOS shell serves the page from a custom scheme and Android from https, so the path works on one platform and silently not on the other. `useClipboardWriter` is the seam C2.4 landed for exactly that. Red first, the census named both routes: `ExpoClipboard.web.js` in each closure, and `src/platform/clipboard.web.ts` in neither. Both call sites also stopped ignoring whether the pasteboard took the text. The conflict section already returned on a throw, so the seam's rejection reaches an arm it had. `copyNotes` had none and its only caller is `void controller.copyNotes()`, so a rejection would have been unhandled with "Review notes copied" left on screen; it now catches and reports through the screen's own error line. That is the one behaviour change here and the reason `clipboard` joins its dependency array. Its suite mocked `setStringAsync` as resolving `undefined`, which the seam reads as a pasteboard that refused, so every copy would have gone down the new refusal arm unseen. The mock now resolves `true` and two cases pin both arms; mutating the catch away kills the refusal one. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): take the source-control router from the handoff seam (OTA phase C, C4.1) The hub takes its router once, in the openers hook, and passes it down to the runners and the panel — so one `useRouter()` is this domain's whole reach into routing, and it was expo-router's own. Inside the shell's page that posts no `navigate`, so the hub's push to review would stay in the document whatever its grants, and its push to a native route would paint Unmatched over the page. Inert today: no C4 route is registered yet. `use-mobile-source-control-runners.ts` is the second case and the reason the rule reads value imports rather than identifiers: it named expo-router only to write `ReturnType<typeof useRouter>`, a value import in a type position that keeps the module in the graph. `RouteHandoff` is the seam's own name for that type. The census is C3.1's, and its walk moves to `src/navigation` rather than being copied a second time; each domain keeps only its own evidence, the list of modules meant to hold a router. Red first it named both modules on the expo-router rule and reported no handoff caller at all. The C2.9 hop census is unchanged and cannot move: its targets come from the call sites, and the derivation over this tree returns the same ten targets and the same 26 unresolved sites before and after this commit, byte for byte. Its `HANDED_OFF` pin is over registered routes, of which this adds none. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): move the shell route guard out of the files domain (OTA phase C, C4.1) `src/files/mobile-file-shell-route.ts` was never about files: it parses a route against `BridgeInitRouteSchema` and builds the key a shell screen remounts on. It moves to `src/mobile-web-shell/shell-screen-route.ts` as `shellScreenRoute` / `shellScreenRouteKey`, with its test. The move is pure — with the rename applied and comments stripped, the old file and the new one diff to nothing. Three routes had grown their own copy of the call and two had none. The copies go: `agent-history` and `tasks` now ask the shared predicate, which is the same schema and the same fallback they already had. `index.tsx` had no guard at all, so a `.` or `..` host id was handed over and came back as "Update Orca to open this workspace" painted over the native list behind the switch; it now stays native. That is the one behaviour change here, pinned red first and killed by mutation. `web.tsx` keeps handing that route over on purpose and is exempt by name: its fallback is a redirect to the route the user came from, so the host's own verdict is the better answer there, which `mobile-web-shell-route.test.tsx` already pins. No `key=` expression moved; the three switches still key differently (host id, pathname, pathname plus params) and making them agree is a behaviour change for another PR. The census walks the route tree rather than a list, so a switch added later is held to both rules without being added here. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): mount the copy cases without a client instead of casting one (OTA phase C, C4.1) The changed-code gate flagged four type assertions on the two cases added with the clipboard seam: they stubbed an `RpcClient` the way the file's older cases do, and the gate reads changed lines. Copying reaches no client at all, so they mount without one, which is both cast-free and a truer statement of what the path needs. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): say what the censuses report and sort the red list by line (OTA phase C, C4.1 round 1) Round-1 folds, four wordings and one ordering. `externalLinkOffenders` said "every call site" and reports the line the name enters the module: a named import once, however many times the module calls `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. The docstring now says that. Its red list sorted the rendered strings, which puts `:10` before `:2`. It now sorts by path and then by line as a number. Pinned against a written fixture rather than the tree, because the case needs a module with sites either side of line ten and no module in a closure has to keep having one — the first fixture used lines 11 and 12, where both orders agree, and the mutation walked straight through it. `shell-screen-route.test.ts` still named the files screens in its describe after the guard stopped being theirs; it names what a switch does now. `router-seam-census.test-support.ts` excluded `.test-support.ts` from the walk, which the files census it was extracted from never did. Dropped, so both censuses walk the same set. Inert today: neither `src/files` nor `src/source-control` holds such a file, so it only decides the next one. The `web.tsx` exemption claimed a redirect "that looks like nothing happened". What was measured: adopting the guard there sends a `..` deep link through `Redirect href="/h/.."` 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. The route is `__DEV__`-only and the host's own failure screen is the better verdict. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(mobile): see every react-native alias, normalise the host id, surface a refused copy (OTA phase C, C4.1 CodeRabbit) Three bots findings on #21732, all real. **Every alias, not the first.** `reactNativeLinkingSites` found namespace imports with `exec` and inspected only the first binding, so a module importing the namespace twice and calling `Linking.openURL` on the second reported no site at all. It reads every alias now and counts a line once however many meet on it. Red first with exactly that fixture. **The host id can be an array.** `app/h/[hostId]/index.tsx` read it bare, and Expo Router answers a repeated key with one: `String(['a','b'])` is `a,b`, `encodeURIComponent` makes that the single segment `a%2Cb`, and the segment rule accepts it — so the shell opened a page for a host nobody has. Through `firstParam`, as the other four switches do. Red first it handed over `/h/host-1%2Chost-2`, and the empty-array case found a second one: `[]` is truthy, so a bare read built `/h/` and handed that over too; `firstParam` answers `''` and the route stays native. That import pulls the source-control screen state, and with it the lucide barrel whose `LucideProvider` re-export is the gap the web build patches, so the suite mocks the barrel as the other suites do. It moves no page closure: the closure resolves `index.web.tsx`, which this does not touch, and the index route still measures 3426 modules, 289 local, 22 families. **A refused copy said nothing.** `PRConflictingFilesSection` caught the rejection and returned: no tick, no message, a tap indistinguishable from one that copied. The label now carries the third state, reusing the tasks page's own wording for it, and the component has its first test. Mutating the failure arm away reds it. Its prop narrows to `Pick<PRInfo, 'mergeable' | 'conflictSummary'>`, which is what it reads and what let the test drop a cast the gate flagged; every caller holds a full `PRInfo` and satisfies it. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): read the censuses' subjects as code, not as text (OTA phase C, C4.1 round 2) Round-2 additions. A separate commit because `d4b14d54e5` was already made and this lane does not amend. **The seam walk parses now.** Matching `X.Linking` in the text named it inside a comment that talks about it and inside a string that quotes it, and the named-import regex did the same for a commented-out import. Checked against the previous implementation, all three fixtures were red there: the comment case reported lines 2 and 3, the string case reported the string's line beside the real call, and `// import { Linking } from 'react-native'` reported line 1. The walk builds a `SourceFile` and reads import declarations and property accesses, so comments and strings are gone by construction and the quote styles stop being a special case. Cost measured on the three closure censuses: 3.3 s, unchanged. **The route census reads the call, not the import.** A switch that keeps the import while the call goes — deleted, or moved behind a branch that never runs — looked exactly like one that asks. It now needs both, proved by mutation: dropping `shellScreenRoute(` from `tasks.tsx` while leaving its import names `tasks.tsx`. A fixture carries the same rule in isolation, since every switch in the tree calls what it imports and the case would otherwise be unfalsifiable against it. **And recognises a switch by its import** of `MobileWebShellScreen` rather than by `<MobileWebShellScreen` in the text, so an alias or a line break the formatter chose cannot hide one and a comment cannot invent one. The `app/h/[hostId]` root stays written out: deriving it from the manifest is not a one-liner from here, the manifest being an `.mjs` this test reads as text. What ties the two together instead is a new case asserting every registered pathname starts with that prefix, so a page route outside it fails rather than going unwalked. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): read the imported name, not the local one (OTA phase C, C4.1 CodeRabbit) `import { Linking as NativeLinking } from 'react-native'` went through the census untouched: the walk compared the specifier's local binding, which is `NativeLinking`, while the imported name lives in `propertyName` when a specifier renames it and only in `name` when it does not. Reproduced before the fix — the aliased import with a call beside it reported no site at all. Reading `propertyName ?? name` closes it in both directions. A module that renames `Linking` is named at its import line like any other, and a module that imports `View as Linking` is no longer named for a local binding that reaches nothing. The second was a false positive the old comparison had by construction. One more of the same class, found while checking and verified rather than assumed: `import RN from 'react-native'` typechecks in this project (tsc accepts it), and a default binding is the whole namespace exactly as `* as RN` is, so `RN.Linking.openURL` through it was invisible too. The default binding joins the alias set, which already reports uses rather than the import. Four fixtures. Three red on the previous walk: the renamed import, the local-only `Linking`, and the default import. The fourth — an alias imported that never reaches `Linking` — passed before and is here to hold the other half of the rule, that importing react-native is not itself the offence. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): parse each module as its own kind, and read re-exports (OTA phase C, C4.1 CodeRabbit) Two ways a module reached `Linking` past the census, both reproduced before the change. Every file was parsed as TSX. In a `.ts` module `const id = <T>(value: T) => value` is a generic arrow; as TSX it is an unclosed JSX element, and the parser folds the rest of the file into the error node. A `RN.Linking.openURL` after one reported nothing, and so did the same call with its import above the arrow. The file name goes into the parse now and TypeScript reads the kind off the extension; `externalLinkOffenders` passes the real path, which it had all along. `ExportDeclaration` was never inspected, so `export { Linking } from 'react-native'` put the name back in reach of anything importing that module while the census saw an import list it was not on. All four shapes are read — named, renamed, `export *` and `export * as` — and reported at the export statement, which is the line to delete exactly as an import is. A re-export of another name, or of `Linking` from somewhere that is not react-native, stays unnamed. Seven fixtures. Five red on the previous walk: the `.ts` generic arrow and the four re-export shapes. The two that pass before and after hold the other half, that re-exporting is not itself the offence. The named-import and re-export clauses read `propertyName ?? name` through one helper rather than two spellings of it. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --------- Co-authored-by: Claude <noreply@anthropic.com>