mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
Jinwoo Hong
23207bfde2
* refactor(mobile): move the review route body onto a component and the handoff seam (OTA phase C, C4.4) The review route file called `useMobileDiffReviewController` at its top level. A switch cannot keep it there: hooks are unconditional, so the whole controller — its client subscriptions included — would run behind the shell's page whenever the shell renders. As an element passed for `fallback` it is created and not mounted, which is how the explorer switch already behaves. `useRouter` becomes `useRouteHandoff` in the same move. It was the one raw expo-router router left in the review closure (measured: the only other value import of one is the seam's own web sibling), and inside the page the session screen `openSession` replaces to is native, so that target has to be handed back to the app rather than posted into a document that does not render it. The params are read in the component rather than handed down, so this is the route body and the route file above it is free to become a switch. `session-router-seam-census.test.ts` gains the module by name. Kept with `useRouter` the census reds twice — `imports nothing from expo-router that can navigate` names `MobileDiffReviewRouteScreen.tsx (useRouter)`, and the completeness case gains `useRouter` — which is what forces the swap rather than leaving it to a reviewer. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(mobile): switch the source-control and review routes to the shell, still unregistered (OTA phase C, C4.4) Both take the files switch's shape: `firstParam`/`firstReviewParam` on every param, `shellScreenRoute` as the one predicate, `MobileWebShellScreen` keyed on `shellScreenRouteKey`, the native screen built as an element and passed for `fallback`. Both gain a `.web.tsx` sibling for `index.web.tsx`'s reason — the native file reaches OrcaMobileWebShellView, whose module throws at import in a browser, and the route manifest imports every route. Inert on its own. A switched route renders the shell only once `MOBILE_WEB_PAGE_ROUTES` lists it, which is the next commit; until then the flag is the only thing that changes and it is off. Query params are omitted rather than sent empty, and the whole record is omitted when none was named: `tab=` is a lens named nothing and lands on `changes` through a different branch than an absent one, and the same holds for `name`, `origin`, `scope`, `file` and `area`. `pr` and `history` are deliberately not switched. Both are `Redirect`s into `source-control`, and a redirect inside the page would leave the session bound to a pathname the page has left; left native they replace into this route and its switch mounts the shell. Three censuses red without their rows, measured on this tree: - `mobile-web-app-web-overrides.test.mjs` `lists exactly the .web.* files on disk` names the two new siblings; `states a reason for every override` reds on a placeholder under 20 characters. - `mobile-web-shell-flag-census.test.ts` `reaches the switched routes through that hook and no others` reds without the two `SWITCHED_ROUTES` names. - `shell-screen-route-census.test.ts` `walks the route tree and finds them` reds without the two switch names. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(mobile): register the source-control and review page routes (OTA phase C, C4.4) Two entries in `MOBILE_WEB_PAGE_ROUTES`, five grants each, with the reason for each grant read off the screen that needs it. The two lists are equal on purpose: the hub's changed-file rows push review and review replaces back, and a target declaring no more than its opener is a hop the handoff keeps inside the document. Registering either alone would have put a native frame and a second bridge session between a changed-file row and its diff. `pageRouteGrants` is derived from this list, so the two rows are a consequence of the entries and there is no second table to edit. `pr` and `history` stay native redirects and are never listed; the derived target list at this tree is [files, files/preview, source-control, [p], accounts, agent-history, review, session, tasks, web], with no `pr` or `history` row, because the census reads call sites and both redirects name `source-control`. Measured on this tree, not carried from the draft: - The hop census goes 8 -> 16. The eight new rows are exactly `{/h/[hostId], agent-history, files/[worktreeId], files/preview} -> {source-control, review}`, each handed off for `native.clipboard.write` and the first four also for `externalLink`. `source-control <-> review` is absent in both directions, which a new case now asserts as grant-list equality rather than as the absence of a row — absent is also what an unregistered route looks like. - The Back census now walks six trees and finds 8 controls, both rules printing empty. The two new ones are `MobileSourceControlHeader.tsx:46 role=button label=Back to session` and `MobileDiffReviewHeader.tsx:48 role=button label=Back`, which is what C4.3 bought. The `ARRIVING_SCREENS` describe it wrote for this moment is removed: with the rows in `PAGE_SERVED_SCREENS` its trees are covered and its cases were a second reading of the same thing. - Both closures reach the haptics seam, so `haptics` is declared by measurement: the seam census derives the reaching set and its two cases pass with the routes in its `ROUTE_MODULES` map. Without the two manifest entries these red on this tree: `pins every hop the handoff must take away from the page`, `keeps the hub and review local to each other`, `declares only routes the bundle has a module for`, `reaches the built manifest`, `covers every page route and finds a control in each`, and both haptics-seam cases. `build-mobile-web-app-bundle.test.mjs` is split rather than fenced. The two pinned entries put it at 607 non-comment lines against the 600 cap, and the declaration block is a different concern from how the bundle is built — it grows once per registered domain while that file does not. It moves whole into `mobile-web-page-routes.test.mjs`, named for the module it is written against, so the next route to register does not have to choose between a lint fence and a split it did not ask for. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): render-check the two page routes, and make the oversized stage-all readable (OTA phase C, C4.4) The render check mounts both routes in a real browser, asserts each paints its own screen rather than the Unmatched route with no console error and no page fault, and asserts each fetches its own chunk on a client-side navigation. It also reads the shipped `img-src 'self' data:` out of the Kotlin source it is served with, pins the Swift twin beside it, and asserts neither route leaves the origin or logs a policy violation while it paints. The avatar skip itself (ruling 3) is `PRCommentCard`: on web it renders its existing empty-avatar `View` rather than letting one `<Image>` per comment attempt a fetch the policy refuses. Its branch is pinned by a component test, which reds on the platform check being removed. The render check's off-origin case is honest about being the negative half only — no comment card renders there, because the PR chain behind it is not scripted, and the file says so. The `useAnimatedScrollHandler` risk is answered by the two static facts rather than by a probe, and they are recorded as assertions: the hook is deliberately outside the four `MAPPER_HOOKS` because it is an event handler, and its updater's only effect is a write to `scrollOffsetY`, which `RightDrawer.tsx` assigns in two places and reads in none. A later read reds that case the moment it is added. The `oversized` stage-all refusal (ruling 2, made testable by ruling 5) was a silent no-op, and this is the fix as well as the case. Measured on this tree before it: `git.bulkStage` with 12,000 paths posts one 1,033,012-byte frame, the shell's reader drops it with `{ kind: 'refused', refusal: 'oversized' }`, and the page's promise never settles — `busyAction` never cleared and `setActionError` was never called. Both new cases red by timing out at 15s against that path. Refused at the page's own send boundary instead, under the shell reader's own predicate rather than a second spelling of it: `isBridgeFrameWithinCap` is extracted from `parseBridgeMessage` and used by both sides. `sendFrame` answers `sent` / `oversized` / `port-failed`, so `sendRequest` rejects with a `BridgeRequestOversizedError` whose message is a sentence the panel puts on its error surface, and the members whose contract is a boolean keep it. No delivery-unknown mark: the frame never left, so nothing ran on the desktop and the smaller retry is safe to offer. The case runs the real chain — bridge port pair, `useMobileGitRequests`, `runGitWorkflow` — with only react-native and the haptics seam mocked, and asserts the message that lands is a sentence and that the busy flag is raised and then cleared. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): drop the type assertions from the two new C4.4 test files (OTA phase C, C4.4) The changed-code quality gate named five, all in the files the previous commit added, and a fence is not the answer to any of them. A separate commit because a reported head does not move by amend. - The comment fixture is a real `PRComment` rather than a cast: the type's six required fields are all this case needs, and the SAFETY disable that stood in for them was inert anyway — oxfmt had wrapped it onto three lines, and a wrapped `oxlint-disable-next-line` matches nothing. - The image lookup goes through `findAll` on the host tag rather than `findAllByType`, which takes a component. Through `String`, because `node.type` is `ElementType` and React Native declares no intrinsic elements, so the compiler reads a bare tag comparison as unreachable. - The runners hook takes its router from `useRouteHandoff` with expo-router mocked under it, which is how a `RouteHandoff` is obtained rather than asserted into existence. No target is pressed. - The rejection and the diagnostic are read through narrowings instead of casts, which also drops an `expect.any` that only type-checked because of one. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(mobile): keep a frame the page cannot serialize inside the send contract (OTA phase C, C4.4 round 1) Round 1 finding 1. The oversized refusal moved `JSON.stringify` outside `sendFrame`'s `try`, so a frame carrying a cycle, a `BigInt` or a throwing `toJSON` threw past the whole send path. Three things followed, all measured here on a cyclic `params`: - the caller was rejected with a bare `TypeError` from `JSON.stringify` instead of the `BridgeSendFailedError` every other undelivered frame raises; - no `send-failed` diagnostic was raised, so nothing recorded that a frame had been lost; - `sendRequest` opens the id before it posts and abandons it on the way out, and the throw skipped the abandon: 63 of the 64 in-flight slots were usable afterwards, against 64 on a client that sent no such frame. Sixty-four of them and every later request is refused with nothing to say why. `posted()` carried the same escape into the members whose contract is a boolean, where a throw is worse still: those callers are taps and teardowns with no catch on them. Serialization goes back inside the `try`, with the oversized refusal kept in front of the post. The docstring said the port arm's throw is never `JSON.stringify`'s, which was exactly the assumption that broke; it now says why the call sits where it does. The new file is the pin: the rejection's name, the diagnostic, nothing reaching the shell, and the slot count with a no-cyclic-frame control beside it so the count cannot pass by the cap moving. Both changed cases red on the serialization moving back out — `expected 'TypeError' to be 'BridgeSendFailedError'` and `expected 63 to be 64`. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): pin the page's frame cap to the reader's, at the boundary and by construction (OTA phase C, C4.4 round 1) Round 1 finding 2. Nothing held the sender's predicate to the reader's. Replacing `isBridgeFrameWithinCap(json)` with an inline `json.length > BRIDGE_MAX_MESSAGE_BYTES + 1` passed 85 of the 86 mobile-web-shell and source-control test files on this tree, and a frame at exactly cap+1 would then be posted and silently dropped — the hang the refusal exists to end, back for every frame in that one-unit band. Two rules, because either alone passes against the defect: - The boundary. A frame of exactly the cap is posted, arrives at `parseBridgeMessage` and is accepted; a frame one byte over is refused with `BridgeRequestOversizedError`, posts nothing, and is the same string the reader answers `oversized` to. An off-by-one reds the second. - The census. The client reaches the cap through the shared predicate and does not name `BRIDGE_MAX_MESSAGE_BYTES` at all, and the module that exports the predicate is the module that parses inbound frames. A private copy that is correct on the day it is written reds here. The overhead the boundary frames are built from is itself checked rather than trusted: a frame asked for at exactly the cap must serialize to exactly the cap, so the constant cannot rot behind an envelope that grew a field. Against the mutation both new rules red — `expected null to be 'BridgeRequestOversizedError'` and the census failing to find the predicate — while the rest of the suite stays green, which is the finding reproduced. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(mobile): count the outbound frame in the unit both shells count it in (OTA phase C, C4.4 round 1) Round 1 finding 3. Read off both shells rather than assumed, and they agree: iOS gates the inbound frame on `json.utf8.count` (`MobileWebShellView.swift`, through `MobileWebShellBridge.acceptsByteCount`) and Android on `json.toByteArray(Charsets.UTF_8).size` (`MobileWebShellView.kt`, through `acceptsMobileWebShellBridgeByteCount`), both against `640 * 1024`. UTF-8 bytes on each platform. The predicate was already right. `isBridgeFrameWithinCap` decides on `utf8ByteLength`, and the `raw.length` clause in front of it is a cheap refusal in the safe direction, not a second rule: every code unit encodes to at least one byte, so a string over the cap in units is over it in bytes too. The diagnostic was not. It reported `json.length` — UTF-16 code units — in a field named `bytes`, so a frame of CJK text read as a quarter of the cap at the moment it was refused by it. It now reports `utf8ByteLength(json)`, and the type says which unit that is. Pinned with a 250,000-character frame of three-byte characters, which is under the cap in code units and over it in bytes, plus a source case reading the measuring expression out of each shell. Three mutations, all red: dropping the byte clause from the predicate reds the refusal (`expected null to be 'BridgeRequestOversizedError'`) and the diagnostic; reporting `json.length` again reds the diagnostic alone (`expected 250094 to be greater than 655360`), which is the defect this commit fixes, in the number it would have printed. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(config): drop the render check's avatar assertion, which could not fail (OTA phase C, C4.4 round 1) Round 1 finding 4. The case asserted that no avatar host was requested while both routes painted, which reads as a proof of the web skip and is not one: no comment card renders on either page, because the PR chain the file's own closing note names is not scripted. Reproduced here — deleting the `Platform.OS !== 'web'` guard from `PRCommentCard` leaves the file at 5 passed. Deleted rather than propped up. Giving the page a presence precondition means five hand-written fixtures against five Zod schemas inside the shell double, which is exactly what the harness's docstring says that double must not become. So the only proof of that branch is `pr-comment-card-web-avatar.test.tsx`, which reds when the check is removed, and the render check now says so in its header instead of implying otherwise. What survives is a property of these two closures rather than of that component: not one request leaves the origin while either route paints, and nothing either paints violates the policy. That one can fail — planting a `fetch` to a provider host in a module both routes reach reds it twice, on the console-error case and on the off-origin case, with the `connect-src 'self'` refusal in the output. The CSP half is unchanged and was never in question: the served header is read from the Kotlin source and the Swift twin is pinned beside it. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb