refactor(mobile): seam moves and the shared shell route guard for the source-control domain (OTA phase C, C4.1) by Jinwoo-H · Pull Request #21732 · stablyai/orca

Jinwoo-H · 2026-09-19T22:11:47Z

	Files	Added	Deleted	Net
Test	12	$\color{#1a7f37}{\Huge{\mathbf{+}}}$726	$\color{#cf222e}{\Huge{\mathbf{−}}}$107	$\color{#1a7f37}{\Huge{\mathbf{+}}}$619
Prod	15	$\color{#1a7f37}{\Huge{\mathbf{+}}}$291	$\color{#cf222e}{\Huge{\mathbf{−}}}$68	$\color{#1a7f37}{\Huge{\mathbf{+}}}$223

Summary

Phase C, C4.1: the inert preparation for the source-control and review page routes (design-ota-c4.md §4 and §7, ruling 4 in rulings-ota-c4.md). Eleven commits (five product, the round-1 fold, the CodeRabbit fold, the round-2 fold, a second CodeRabbit fold, a merge of main, a third CodeRabbit fold), no route registered, nothing a released phone or the desktop can observe.

commit	what
`acb40930f3`	the three raw `Linking.openURL` sites in the PR sidebar (`CommentMarkdown`, `PRChecksSection`, `PRCommentCard`) open through `src/platform/external-link.ts`, the seam the `externalLink` grant is about
`ce42ea2fb7`	the two raw `expo-clipboard` sites (`PRConflictingFilesSection`, `use-mobile-diff-review-send-actions`) copy through `src/platform/clipboard.ts`; on web the raw module would have reached `navigator.clipboard` and worked on Android only
`6669c0f9d4`	`use-mobile-source-control-openers.ts` and `use-mobile-source-control-runners.ts` take their router from `useRouteHandoff`, as C3.1 did at its two sites; natively the plain router, inside a shell the C2.9 rule once a route is registered
`d5f1eb4098`	the shared route guard leaves the files domain: `src/files/mobile-file-shell-route.ts` → `src/mobile-web-shell/shell-screen-route.ts` (`shellScreenRoute`, `shellScreenRouteKey`), a pure move (renames applied and comments stripped, the bodies diff to nothing), imported by every switch
`2639852dd8`	the two new copy cases mount without a client instead of casting one

Censuses, red first

Under config/scripts, over the closures of the two future routes derived through mobileWebAppRouteClosure: mobile-web-app-source-control-external-links.test.mjs (every external URL through the seam; the seam present) named the three importing modules by path:line (the import line, once per module, whatever the call count) before the fix; the clipboard census found ExpoClipboard.web.js in the closure and the platform seam absent; the router census found both modules on the raw expo-router import and no handoff caller. The offender walk is one module now, mobile-web-app-external-link-seam.mjs, serving the files and tasks censuses too (they report path:line where they reported paths; an empty list is the same assertion), and C3.1's router-seam walk lives in src/navigation/router-seam-census.test-support.ts.

The guard census in the mobile suite asserts every switch imports the shared guard: agent-history, index.tsx, tasks.tsx were not asking it, and agent-history and tasks.tsx spelled the schema themselves.

Three things that are more than a move

A refused copy is on screen now. The conflict section's label carries a third state with the tasks page's own wording (its first component test; the prop narrowed to the two PRInfo fields it reads). At the base a rejecting setStringAsync in the review send sheet escaped as a floating rejection and a false return still said "Review notes copied"; the conflict section ticked "Copied" the same way. Through the seam a refusal throws, the send sheet reports it on the screen's error line and the conflict section does not tick. That is the seam's point, and it is a visible change.
index.tsx gains a guard it never had. A . or .. host id was handed to the shell, which painted "Update Orca to open this workspace" over the native list; it now stays native like every other switch. Pinned red first. The switch also reads the param through firstParam like its four siblings (CodeRabbit): an array id no longer builds /h/a%2Cb for the shell, it opens the first host as the siblings do, and an empty array (truthy, so the bare read built /h/) stays native.
web.tsx is exempted by name. Adopting the guard there reds mobile-web-shell-route.test.tsx, which pins on purpose that a .. host id reaches the bridge and comes back as the host's verdict, because that route's fallback is a redirect to where the user came from. The census asserts the exempted file exists.

Noted, not changed: the switches still key remounts differently (tasks on hostId, agent-history on route.pathname, files on pathname plus params); only the last makes a same-path param change a remount, which is what shellScreenRouteKey's docstring argues for. Making them agree is a behaviour change for its own PR.

The C2.9 hop census cannot have moved: no route is registered, and the derivation at adfcbaf685 gives the same ten targets and the same unresolved sites at the base and at the head.

Verified

Lane gates on 2639852dd8: mobile tsc, ratchet, whole-tree oxlint, oxfmt (2955 files), root oxfmt and oxlint on the five changed root files, full mobile suite (722 files, 8204 passed, no Errors line), config/scripts with mobile deps required (221 files, 2013 passed), changed-code quality and react-doctor against 6a0200f416, merge-tree max-lines clean, machine-path grep clean. The lane used git stash once (tagged, applied by sha, dropped) to observe the clipboard census red, against the lane rules; reported, not repeated.
CodeRabbit on 172891d1bc: the parsed walk read the local binding name, so import { Linking as NativeLinking } was missed; 82aba28dfa reads the imported name and tracks the local alias (and a default binding, import RN from 'react-native', which typechecks here and carries the whole namespace), which also removed a false positive (import { View as Linking } was named for a binding that reaches nothing); four fixtures, three red on the previous walk.
Round 2 (on db077f5faa): approve, every claim reproduced; folded in 172891d1bc: the external-link walk parses with the TypeScript API instead of matching text (a Linking.openURL in a comment, in a string, and a commented-out import were all named before; three fixtures), the guard census reads the call as well as the import (dropping shellScreenRoute( from tasks.tsx while keeping the import names it) and recognises a switch by its MobileWebShellScreen import, and a case asserts every registered pathname sits under app/h/[hostId] so a route outside it fails rather than going unwalked. Gates re-measured at db077f5faa: config/scripts 221 files / 2015 passed, mobile 722 files / 8204 passed, no Errors.
Round 1 (on 2639852dd8): approve; folded in db077f5faa: the walk's docstring says what it reports (the line a named import enters the module; a namespace import reports its uses) and its red list sorts by path then line as a number (the first fixture, lines 11 and 12, could not tell numeric from text order and let the mutation pass; it is lines 2 and 10 now, in a temp dir); the moved test's describe; the .test-support.ts exclusion the files walk never had; the web.tsx exemption's wording carries what was measured (the guard there would send a .. deep link through Redirect href="/h/.." to a host list with nothing said about why; the route is __DEV__-only and the host's failure screen is the better verdict).
CodeRabbit on 7ff4b1595f, two Majors on the walk, both reproduced and folded in 69da504f90: every file was parsed as TSX, so in a .ts module a generic arrow parsed as an unclosed tag swallowed everything after it (a namespace import above it was walked, the call below it was not, in either order); the file path now goes into the parse and TypeScript takes the kind from the extension. And ExportDeclarations were ignored, so export { Linking } from 'react-native' (named, renamed, export *, export * as) let a route import the re-export; all four shapes are named at the export statement, with two negative fixtures (another name; Linking from elsewhere) holding the other half. No real closure newly reds.
Merge of main b766f512ec (7ff4b1595f, no conflicts) to pick up the DiffViewer.tsx max-lines fix that had every PR's static analysis red.
Lead reproduction: verify-gates.sh on 2639852dd8, db077f5faa and 172891d1bc against 6a0200f416, and on 7ff4b1595f and 69da504f90 against b766f512ec; the dropped-call mutation reds the guard census on 172891d1bc; the links census reds naming the three importing modules with them reverted; the guard's old and new bodies differ only in the import path and the two names.

Part of the mobile OTA plan (Phase C). Reference PR #13386, never merged.

… (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

…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

…m (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

…(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

…g 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

coderabbitai · 2026-09-19T22:18:50Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 2c363daf-6af9-4a36-b6ea-0dad7f48b0db

📥 Commits

Reviewing files that changed from the base of the PR and between 7ff4b15 and 69da504.

📒 Files selected for processing (2)

config/scripts/mobile-web-app-external-link-seam.mjs
config/scripts/mobile-web-app-external-link-seam.test.mjs

🚧 Files skipped from review as they are similar to previous changes (2)

config/scripts/mobile-web-app-external-link-seam.mjs
config/scripts/mobile-web-app-external-link-seam.test.mjs

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

📝 Walkthrough

Walkthrough

The change centralizes external-link detection and route-closure checks. Mobile shell switches now use shellScreenRoute. PR sidebar links use openExternalLink, and clipboard writes use useClipboardWriter. Router census helpers and tests enforce Expo Router and route-handoff boundaries. Additional tests cover invalid routes, clipboard refusal, and external-link offenders.

Priority: ➖ Normal

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 61.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 27 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The description gives detailed scope, rationale, implementation changes, visible behavior changes, and testing results. It does not follow the required template and omits the mandatory linked issue, v…	Use the repository template headings. Add a linked issue in the required `Fixes #...` form, provide before/after visual proof for the behavior changes or an explicit justified `N/A`, complete the Testing and Checklist sections, and address …

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the mobile seam refactor and shared shell route guard. It is specific to the main changes, although the phase metadata adds some length.

Full details: Description check

Explanation

The description gives detailed scope, rationale, implementation changes, visible behavior changes, and testing results. It does not follow the required template and omits the mandatory linked issue, visual proof or an explicit N/A statement, checklist items, and several required sections.

Resolution

Use the repository template headings. Add a linked issue in the required Fixes #... form, provide before/after visual proof for the behavior changes or an explicit justified N/A, complete the Testing and Checklist sections, and address AI Disclosure, Review, Agent skill upstream boundary, and Notes as applicable.

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 3

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 702e32a1-0d0a-4fc5-a84e-f028ac00e9ff

📥 Commits

Reviewing files that changed from the base of the PR and between 9309350 and 2639852.

📒 Files selected for processing (26)

config/scripts/mobile-web-app-external-link-seam.mjs
config/scripts/mobile-web-app-external-link-seam.test.mjs
config/scripts/mobile-web-app-files-external-links.test.mjs
config/scripts/mobile-web-app-source-control-external-links.test.mjs
config/scripts/mobile-web-app-tasks-external-links.test.mjs
mobile/app/h/[hostId]/agent-history/[worktreeId].tsx
mobile/app/h/[hostId]/files/[worktreeId].tsx
mobile/app/h/[hostId]/files/preview/[worktreeId].tsx
mobile/app/h/[hostId]/index.tsx
mobile/app/h/[hostId]/tasks.tsx
mobile/src/components/pr-sidebar/CommentMarkdown.tsx
mobile/src/components/pr-sidebar/PRChecksSection.tsx
mobile/src/components/pr-sidebar/PRCommentCard.tsx
mobile/src/components/pr-sidebar/PRConflictingFilesSection.tsx
mobile/src/files/files-router-seam-census.test.ts
mobile/src/files/mobile-file-path-route-encoding.test.ts
mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx
mobile/src/mobile-web-shell/shell-screen-route-census.test.ts
mobile/src/mobile-web-shell/shell-screen-route.test.ts
mobile/src/mobile-web-shell/shell-screen-route.ts
mobile/src/navigation/router-seam-census.test-support.ts
mobile/src/session/use-mobile-diff-review-send-actions.test.ts
mobile/src/session/use-mobile-diff-review-send-actions.ts
mobile/src/source-control/source-control-router-seam-census.test.ts
mobile/src/source-control/use-mobile-source-control-openers.ts
mobile/src/source-control/use-mobile-source-control-runners.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

…ine (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

pullfrog

✅ No new issues found.

Reviewed changes

External-link seam: the three raw Linking.openURL sites in the PR sidebar (CommentMarkdown, PRChecksSection, PRCommentCard) now open through src/platform/external-link, which is the module the externalLink grant is about.
Clipboard seam: PRConflictingFilesSection and useMobileDiffReviewSendActions copy through useClipboardWriter. The review-notes copy now surfaces a refused write instead of claiming "Review notes copied"; the conflicts copy keeps its silent no-op.
Router seam: use-mobile-source-control-openers.ts takes its router from useRouteHandoff, and use-mobile-source-control-runners.ts imports the RouteHandoff type, so neither reaches expo-router's runtime router from inside the shell page.
Guard move: mobile-file-shell-route.ts becomes mobile-web-shell/shell-screen-route.ts (shellScreenRoute / shellScreenRouteKey); the function bodies are identical apart from the import path and names, and every switch imports the shared guard.
index.tsx gains the guard: a . or .. host id now stays native rather than handing the page a route the bridge refuses. web.tsx is exempted by name, with the exemption asserted against the file tree so it cannot outlive it.
Censuses: a shared externalLinkOffenders walk replaces the three per-domain copies, a shared router-seam test-support module serves the files and source-control walks, and new shell-screen-route / source-control censuses pin the guard and router rules.

I verified the changed mobile tests (7 files, 77 passed), the config script censuses with mobile deps present (4 files, 33 passed), pnpm --dir mobile run typecheck, and oxlint on the changed files — all clean. The move, the guard rejection of dot segments, and the clipboard refusal path all check out.

^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

…face 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

…hase 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

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 02d646ce-3c5c-4eea-9649-aed34c0519bc

📥 Commits

Reviewing files that changed from the base of the PR and between db077f5 and 172891d.

📒 Files selected for processing (7)

config/scripts/mobile-web-app-external-link-seam.mjs
config/scripts/mobile-web-app-external-link-seam.test.mjs
mobile/app/h/[hostId]/index.tsx
mobile/src/components/pr-sidebar/PRConflictingFilesSection.test.ts
mobile/src/components/pr-sidebar/PRConflictingFilesSection.tsx
mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx
mobile/src/mobile-web-shell/shell-screen-route-census.test.ts

🚧 Files skipped from review as they are similar to previous changes (4)

mobile/src/mobile-web-shell/shell-screen-route-census.test.ts
mobile/app/h/[hostId]/index.tsx
mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx
config/scripts/mobile-web-app-external-link-seam.test.mjs

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.

coderabbitai · 2026-09-19T23:17:11Z

+      aliases.add(bindings.name.text)
+      continue
+    }
+    if (bindings.elements.some((element) => element.name.text === 'Linking')) {


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Check the imported name instead of the local binding name.

For import { Linking as NativeLinking } from 'react-native', element.name.text is NativeLinking. The census therefore misses the import and permits direct calls through NativeLinking.

Use propertyName when the import has an alias.

Proposed fix

- if (bindings.elements.some((element) => element.name.text === 'Linking')) { + if ( + bindings.elements.some( + (element) => (element.propertyName ?? element.name).text === 'Linking' + ) + ) { sites.push(lineOf(statement)) }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

if (bindings.elements.some((element) => element.name.text === 'Linking')) {

if (

bindings.elements.some(

(element) => (element.propertyName ?? element.name).text === 'Linking'

)

) {

pullfrog

✅ No new issues found.

Reviewed changes

The commits since the prior pullfrog review harden the two new censuses to read code rather than text, and close the host-id and copy edge cases the CodeRabbit round raised.

Rewrote the external-link census to parse each module's AST: reactNativeLinkingSites now reads a Linking named import and every react-native namespace alias from the TypeScript tree, so a comment, a string, or a commented-out import no longer produces a line, and a second namespace import is no longer invisible.
Made the guard census read the call, not just the import: a switch is recognised by importing MobileWebShellScreen and must both import and call shellScreenRoute; a new case walks the page-route manifest and fails if any registered pathname sits outside /h/[hostId].
Normalised the host-list switch's param: index.tsx reads hostId through firstParam, so a repeated key opens the first host instead of the joined a%2Cb segment, and an empty array stays native.
Surfaced a refused clipboard write in the conflicts section: the copy control carries idle/copied/failed, so a rejected write shows "Failed to copy text" instead of a silent no-op; its first component test pins both arms.

Verified the changed mobile tests (8 files, 83 passed), the external-link censuses with mobile deps present (4 files, 40 passed), and oxlint on the changed files — all clean.

^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

… 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

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: af5894d2-e960-4d0c-9c59-8d9f4c3739f0

📥 Commits

Reviewing files that changed from the base of the PR and between 172891d and 7ff4b15.

📒 Files selected for processing (2)

config/scripts/mobile-web-app-external-link-seam.mjs
config/scripts/mobile-web-app-external-link-seam.test.mjs

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

pullfrog

✅ No new issues found.

Reviewed changes

The delta since the prior pullfrog review (172891d1bc) is one CodeRabbit fold plus a main merge, and both hold up.

Read the imported name, not the local binding. reactNativeLinkingSites now compares element.propertyName ?? element.name against Linking, so import { Linking as NativeLinking } is named (and import { View as Linking } is correctly left alone); reading name alone let the renamed import through the census entirely.
Read a default import as the namespace it is. clause.name is added to the alias set, so import RN from 'react-native' followed by RN.Linking… reports, matching the namespace-import arm.
Four new fixtures pin the renamed, local-name, default-import and unused-alias cases with exact toEqual([...]) assertions; the renamed and default cases both fail against the prior walk.
Merge of main (b766f512ec) picks up the DiffViewer.tsx max-lines fix with no conflicts and no product changes.

Verified the seam census directly (22 passed). The three closure censuses skip in this environment because mobile/node_modules/react-native-web is absent and run for real in CI's mobile_web_app job; I confirmed no module under mobile/ default-imports bare react-native, so the new default-import arm is inert against the current tree and is forward coverage only.

^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

…(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

pullfrog

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review (7ff4b1595f), a single CodeRabbit fold.

Parsed each module by its own extension. reactNativeLinkingSites now hands the file path to ts.createSourceFile instead of forcing ScriptKind.TSX, so a .ts module with a generic arrow (const id = <T>(v: T) => v) is no longer mis-parsed as an unclosed JSX element that swallows the sites below it; externalLinkOffenders passes each closure file through.
Reported re-exports. export { Linking } from 'react-native' and its renamed, export *, and export * as RN forms now name the export line, with negative fixtures for export { View } and a specifier from another module.
Routed the predicate through the walker. reachesReactNativeLinking delegates to reactNativeLinkingSites, so the boolean and the line list cannot drift into two rules.

The extension-based parse only changes behavior for .ts files, where the TS grammar is a superset of what TSX parsed, so the walk can find more sites and never fewer. The re-export arm is inert against the current tree (no mobile module re-exports react-native) and the four new positive fixtures plus two negatives exercise it directly; export { L as Linking } correctly stays clean since propertyName ?? name reads the imported name.

Verified the seam census directly: config/scripts/mobile-web-app-external-link-seam.test.mjs — 29 passed. The three closure censuses skip here because mobile/node_modules is absent and run in CI's mobile_web_app job.

^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

Jinwoo-H added 5 commits September 19, 2026 17:47

coderabbitai Bot reviewed Sep 19, 2026

View reviewed changes

Comment thread config/scripts/mobile-web-app-external-link-seam.mjs Resolved

Comment thread mobile/app/h/[hostId]/index.tsx Resolved

Comment thread mobile/src/components/pr-sidebar/PRConflictingFilesSection.tsx Resolved

pullfrog Bot reviewed Sep 19, 2026

View reviewed changes

Jinwoo-H added 2 commits September 19, 2026 18:51

coderabbitai Bot reviewed Sep 19, 2026

View reviewed changes

pullfrog Bot reviewed Sep 19, 2026

View reviewed changes

Jinwoo-H and others added 2 commits September 19, 2026 20:03

Merge remote-tracking branch 'origin/main' into ota-c4-1-seams

7ff4b15

coderabbitai Bot reviewed Sep 20, 2026

View reviewed changes

Comment thread config/scripts/mobile-web-app-external-link-seam.mjs Resolved

Comment thread config/scripts/mobile-web-app-external-link-seam.mjs Resolved

pullfrog Bot reviewed Sep 20, 2026

View reviewed changes

Jinwoo-H merged commit f3bda1b into main Sep 20, 2026
32 checks passed

Jinwoo-H deleted the ota-c4-1-seams branch September 20, 2026 01:06

Conversation

Jinwoo-H commented Sep 19, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Censuses, red first

Three things that are more than a move

Verified

Uh oh!

coderabbitai Bot commented Sep 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Sep 19, 2026

Choose a reason for hiding this comment

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Jinwoo-H commented Sep 19, 2026 •

edited by github-actions Bot

Loading

coderabbitai Bot commented Sep 19, 2026 •

edited

Loading