feat(companion): native claim + link/create for unclaimed QR codes - #2753
Conversation
Scanning an unclaimed QR (label-sheet sticker, no organization) in the companion app dead-ended with a web bridge. The scanner now takes over the full flow natively, mirroring web semantics by reusing the same services. Webapp (mobile API): - resolveMobileScannedCode: unclaimed 404 carries additive reason "unclaimed" + qrId; message/status unchanged for all consumers - new POST /api/mobile/qr/claim wrapping claimQrCode (ADMIN/OWNER only) - new POST /api/mobile/qr/link-asset wrapping updateAssetQrCode with assertAssetsBelongToOrg on the user-supplied assetId - claimQrCode hardened with an atomic conditional WHERE (claim race, benefits web too); 25 co-located unit tests Companion: - unclaimed scan result offers Create New Asset / Link Existing Asset (claim-first, admin/owner gate mirroring the server) - claimed-but-unlinked result additionally offers Link Existing Asset - new /(tabs)/assets/link-qr searchable picker with web-parity list and replace-QR confirm warning; auto re-claim retry on claim race - structured reason branching replaces error-message string matching - manual-entry pill hidden while a result card is shown (device-test finding: it occluded the card's second action) - Xcode Debug DEVELOPMENT_TEAM corrected to the company team Tested: 3644 webapp tests green, companion tsc/lint green, live API matrix 11/11 (IDOR, role denials, races, audit-scanner regression), on-device create + link flows verified against the dev database.
🩺 React Doctor — webapp✅ No new findings on the files changed by this PR. Run locally with |
🩺 React Doctor — companionFindings on the files changed by this PR:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 458ecf2def
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
WalkthroughAdds mobile QR claim and asset-linking APIs, structured QR error propagation, atomic claim handling, scanner actions, and a searchable companion asset picker for linking QR codes. ChangesQR linking flow
iOS signing configuration
Mirror maintenance guidance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Scanner
participant MobileAPI
participant QRService
participant AssetPicker
Scanner->>MobileAPI: resolve scanned QR
MobileAPI-->>Scanner: return QR or structured unclaimed error
Scanner->>MobileAPI: claim QR
MobileAPI->>QRService: atomically claim QR
Scanner->>AssetPicker: open asset linking flow
AssetPicker->>MobileAPI: link QR to selected asset
MobileAPI-->>AssetPicker: return linked QR
AssetPicker-->>Scanner: navigate to asset detail
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/webapp/app/modules/qr/service.server.ts (1)
485-497: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider suppressing Sentry capture for the pre-check "already claimed" throw too.
The new lost-race throw sets
shouldBeCaptured: falsesince it's an expected, benign outcome. The pre-existing pre-check throw (Line 445-454, same file) for the identical "already claimed" outcome doesn't set this, so it defaults totrueand gets captured — even though it can also be hit by two concurrentgetQrreads racing (the comment above the atomic write explicitly says the pre-check "is not atomic with this write"). Aligning the two would avoid noisy captures for what is effectively the same race outcome.♻️ Optional alignment
if (qr.organizationId) { throw new ShelfError({ message: "This QR code already belongs to an organization so you cannot claim it.", title: "QR code already claimed", status: 403, additionalData: { id, organizationId, userId }, label, cause: null, + shouldBeCaptured: false, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/modules/qr/service.server.ts` around lines 485 - 497, Update the pre-check “already claimed” ShelfError in the QR claim flow to set shouldBeCaptured: false, matching the lost-race throw in the shown block. Keep the existing message, status, and error metadata unchanged.apps/webapp/app/routes/api+/mobile+/qr.$qrId.ts (1)
34-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated mobile error-envelope construction across the two resolve routes. Both loaders build the identical
{ message, ...(reason ? {reason, qrId} : {}) }shape from aResolveMobileCodeResultfailure; the shared root cause is the missing helper for turning a not-okResolveMobileCodeResultinto the wire error envelope.
apps/webapp/app/routes/api+/mobile+/qr.$qrId.ts#L34-L46: extract a shared helper (e.g.toMobileErrorEnvelope(result)exported frommobile-code-resolve.server.ts) and use it here.apps/webapp/app/routes/api+/mobile+/get-scanned-item.$qrId.ts#L29-L42: use the same shared helper here instead of repeating the spread logic.As per coding guidelines: "When you encounter duplicated code patterns across files or functions, extract them into focused reusable helpers instead of repeating the logic."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/routes/api`+/mobile+/qr.$qrId.ts around lines 34 - 46, Extract the duplicated mobile error-envelope construction into an exported toMobileErrorEnvelope helper in mobile-code-resolve.server.ts, preserving the message, conditional reason, and qrId fields from a failed ResolveMobileCodeResult. Update apps/webapp/app/routes/api+/mobile+/qr.$qrId.ts lines 34-46 and apps/webapp/app/routes/api+/mobile+/get-scanned-item.$qrId.ts lines 29-42 to use the shared helper instead of building the spread inline.Source: Coding guidelines
apps/webapp/app/modules/api/mobile-code-resolve.server.ts (1)
156-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the QR claimability predicate.
The resolver, web claim loader, web public scan redirect, and mobile claim action each rely on
!qr.assetId && !qr.kitId, whileclaimQrCodeonly checksorganizationId. Put a sharedisQrClaimable(qr)helper in the QR module/commonplace server utilities and consume it from the shared server-side paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/modules/api/mobile-code-resolve.server.ts` around lines 156 - 170, Extract the repeated `!qr.assetId && !qr.kitId` predicate into a shared `isQrClaimable(qr)` helper in the QR module or common server utilities. Replace the inline checks in the resolver’s `claimable` calculation and the web claim loader, public scan redirect, and mobile claim action with this helper, and update `claimQrCode` to validate it alongside `organizationId`.Source: Coding guidelines
apps/companion/lib/api/client.ts (1)
200-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the extracted
messageinstead of re-readingjson?.error?.message.
errorDetails.messageis already the validated string; the duplicatedanytraversal can drift fromextractErrorDetails's validation.♻️ Proposed tidy-up
const errorDetails = extractErrorDetails(json); // 403 = forbidden → user lacks permission, but session is valid if (response.status === 403) { return { data: null, error: - json?.error?.message || + errorDetails?.message || "You don't have permission to perform this action.", errorDetails, }; } return { data: null, - error: json?.error?.message || `Request failed (${response.status})`, + error: errorDetails?.message || `Request failed (${response.status})`, errorDetails, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/companion/lib/api/client.ts` around lines 200 - 215, Update the error responses in the request handling flow to use the validated message from errorDetails.message instead of re-reading json?.error?.message. Apply this to both the 403 branch and the generic failure return, while preserving their existing fallback messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/companion/app/`(tabs)/assets/link-qr.tsx:
- Around line 106-131: Update fetchAssets so load-more failures (reset=false) do
not trigger the destructive error state used by the FlatList; preserve the
existing assets and scroll position while surfacing the pagination error
non-destructively. Only set the fatal setError state for reset/first-page
failures, and keep successful reset and append behavior unchanged.
---
Nitpick comments:
In `@apps/companion/lib/api/client.ts`:
- Around line 200-215: Update the error responses in the request handling flow
to use the validated message from errorDetails.message instead of re-reading
json?.error?.message. Apply this to both the 403 branch and the generic failure
return, while preserving their existing fallback messages.
In `@apps/webapp/app/modules/api/mobile-code-resolve.server.ts`:
- Around line 156-170: Extract the repeated `!qr.assetId && !qr.kitId` predicate
into a shared `isQrClaimable(qr)` helper in the QR module or common server
utilities. Replace the inline checks in the resolver’s `claimable` calculation
and the web claim loader, public scan redirect, and mobile claim action with
this helper, and update `claimQrCode` to validate it alongside `organizationId`.
In `@apps/webapp/app/modules/qr/service.server.ts`:
- Around line 485-497: Update the pre-check “already claimed” ShelfError in the
QR claim flow to set shouldBeCaptured: false, matching the lost-race throw in
the shown block. Keep the existing message, status, and error metadata
unchanged.
In `@apps/webapp/app/routes/api`+/mobile+/qr.$qrId.ts:
- Around line 34-46: Extract the duplicated mobile error-envelope construction
into an exported toMobileErrorEnvelope helper in mobile-code-resolve.server.ts,
preserving the message, conditional reason, and qrId fields from a failed
ResolveMobileCodeResult. Update apps/webapp/app/routes/api+/mobile+/qr.$qrId.ts
lines 34-46 and apps/webapp/app/routes/api+/mobile+/get-scanned-item.$qrId.ts
lines 29-42 to use the shared helper instead of building the spread inline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b80fe264-6e08-4196-9e91-76e3a1cc6654
📒 Files selected for processing (21)
apps/companion/app/(tabs)/assets/_layout.tsxapps/companion/app/(tabs)/assets/link-qr.tsxapps/companion/app/(tabs)/scanner.tsxapps/companion/components/scanner/scan-result-card.tsxapps/companion/ios/Shelf.xcodeproj/project.pbxprojapps/companion/lib/api/assets.tsapps/companion/lib/api/client.tsapps/companion/lib/api/types.tsapps/companion/lib/permissions.tsapps/webapp/app/modules/api/mobile-code-resolve.server.test.tsapps/webapp/app/modules/api/mobile-code-resolve.server.tsapps/webapp/app/modules/qr/service.server.test.tsapps/webapp/app/modules/qr/service.server.tsapps/webapp/app/routes/api+/mobile+/get-scanned-item.$qrId.test.tsapps/webapp/app/routes/api+/mobile+/get-scanned-item.$qrId.tsapps/webapp/app/routes/api+/mobile+/qr.$qrId.test.tsapps/webapp/app/routes/api+/mobile+/qr.$qrId.tsapps/webapp/app/routes/api+/mobile+/qr.claim.test.tsapps/webapp/app/routes/api+/mobile+/qr.claim.tsapps/webapp/app/routes/api+/mobile+/qr.link-asset.test.tsapps/webapp/app/routes/api+/mobile+/qr.link-asset.ts
Review findings from PR #2753 bots: - scanner: pin the originating org for an in-flight claim and drop the continuation if the active workspace changes before it settles; the follow-up create/link must never open under a different org than the claim targeted (Codex P1) - link-qr picker: version each asset-list fetch and discard stale responses that finish after a newer search/workspace request started (Codex P2) - link-qr picker: a failed load-more no longer wipes the loaded list; only first-page failures show the fatal error state (CodeRabbit)
|
Good catch to question — the split is deliberate, but it deserves its rationale on the record (this PR only added the Why the companion has its own permissions file:
That said, the drift risk is real — this is the same class as our companion↔webapp wire-contract drift. Proposed follow-up (kept out of this PR to avoid scope creep): extract the matrix and the resolution logic (matrix + short-circuit) into a shared workspace package (e.g. |
Codifies the rationale from the PR #2753 permissions discussion: any hand-copied webapp truth in the companion must declare its canonical source, mirror effective behavior (not raw data), stay UI-cosmetic, and name its extraction target in packages/*.
|
To be precise about two layers my earlier comment blurred by calling both "permissions" — that's on me: 1. Authorization — authoritative, and shared with web. Every mobile mutation is enforced server-side by 2. UI gating — the file your question was about. Agreed on the fix, as a separate PR: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/rules/cross-app-mirrors-need-provenance.md:
- Around line 2-3: Align the rule’s description and scope by either broadening
the `globs` setting to cover every intended secondary app or revising the
description to apply only to `apps/companion/**`; ensure the configured scope
matches the behavior being documented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c62a74e7-b993-4930-bb5a-5cde77178f05
📒 Files selected for processing (1)
.claude/rules/cross-app-mirrors-need-provenance.md
|
Follow-up delivered: #2757 extracts the matrix + resolution logic (incl. the ADMIN/OWNER short-circuit) into a shared |
DonKoko
left a comment
DonKoko
left a comment
There was a problem hiding this comment.
Solid work. claimQrCode's atomic WHERE is a real security win that benefits web too, the reason discriminator is genuinely additive (verified: audit scanner unaffected), and claim-before-create correctly dodges createAsset's loose branch. Ran the new tests locally: 32/32 green.
My notes are almost entirely about where the guards live, not whether they're correct.
Would change before merge (1 thing, if you have appetite)
qr.link-asset.ts should call relinkAssetQrCode, not updateAssetQrCode.
relinkAssetQrCode (apps/webapp/app/modules/asset/service.server.ts:6716, used by the web asset-detail relink action) already does every guard this route hand-rolls — QR-not-found, wrong-org 403, kit-linked, asset-linked-elsewhere, org-scoped asset update — and writes the system note, and claims an unclaimed code inline. The route currently reuses the weaker service plus ~60 lines of re-implemented guards, and silently drops the audit note on what is a destructive operation (the asset's existing QR is unlinked with no trace).
Caveat worth deciding consciously: it auto-claims, which makes the picker's reason: "unclaimed" recovery path (apps/companion/app/(tabs)/assets/link-qr.tsx:170) dead code. That's a simplification, but it's a contract change — your call, not a silent one.
Follow-up PRs (not blocking)
-
Move the guards into the shared service — web is the unguarded caller today. The comment at
qr.link-asset.ts:109-122has the right instinct ("fix it inupdateAssetQrCodefor web AND mobile together") but the sibling is already exposed:qr+/_private+/$qrId_.link.asset.tsx:170-204's action has no QR guards at all — they live in the loader, which a direct POST never runs (qr+/route.tsxhas no loader,_private+is pathless). So an admin in org A can POST org B's claimed-but-unlinked QR id with their ownassetIdand re-parent it onto their asset (connectdoesn't constrainQr.organizationId). Pre-existing, needs a known cuid2 id, so Medium-low — but this PR is the natural moment. -
Fix
createAsset's loose QR connect (service.server.ts:1394-1411). It connects an unclaimed code without settingorganizationId/userId, producing rows that resolve as 404 "not linked to any organization" forever. You defend against those rows in three places (mobile-code-resolve.server.ts:160,qr.claim.ts:122, plus docs); fixing the producer deletes all three special cases.
Smaller notes
- The claim error message never reaches users.
claimQrCode's outer catch re-wraps every innerShelfErrorwith"Failed to claim qr code";ShelfErrorinheritsstatus/titlefrom the cause but notmessage(utils/error.ts:180-185), and the mobile envelope ships onlymessage. Your tests lock this in. One-liner fix:if (isLikeShelfError(cause)) throw cause;at the top of the outer catch. - Body-parsing fix applied twice, not shared. The
safeParse(await request.json().catch(() => null))pattern fixes a genuine 500-on-malformed-body; ~36 sibling mobile routes still have it (asset.create.ts:78et al). Worth extractingparseMobileJsonBody(request, schema)while it's fresh. link-qr.tsxis a third copy of the paginated asset list (assets/index.tsx:121,custody.tsx:66) — and it's the best copy: it adds afetchVersionRefstale-response guard the other two lack. ExtractuseAssetList()and the main assets list gets the race fix for free.- The picker has no workspace-switch guard the way the scanner now does. Switch org mid-flow and it lists the new org's assets while
qrIdbelongs to the old one; the server 403s on confirm. No data damage, just a confusing dead end — mirror the scanner'sactiveOrgIdRefpattern. - Rate limiting: your reasoning is sound and the IP gate covers
/api/mobile/*, but 19 sibling write routes useenforceUserRateLimit(user.id, "bulk"), and claim is irreversible. Awritebucket (~60/min) would fit without throttling a label sheet. DEVELOPMENT_TEAM: Debug-only, Release untouched — right call to flag it; just get a 👍 from whoever owned3V6BGGX7JS.
Verified good, no action needed
Org-scoped assetId per org-scope-user-supplied-ids; claim target always from requireOrganizationAccess (never body-supplied); lib/permissions.ts mirror accurate (confirmed ADMIN/OWNER short-circuit in permission.validator.server.ts:42-48 and BASE/SELF_SERVICE qr: [read] only); deep-link allowlist untouched with openShelfWebUrl for the bridge; retry: false + re-resolve recovery on non-idempotent writes; scanner's originOrgId pin against mid-flight workspace switches; Ionicons as any removal.
|
All addressed in 2 commits — adopted the appetite item plus the small ones in-PR; the four bigger notes became tracked issues. Adopted: Also in-PR:
Follow-up issues filed: #2760 (guards into the shared service + the unguarded web link action), #2761 (createAsset orgless-connect producer), #2762 (parseMobileJsonBody sweep), #2763 (useAssetList extraction — gifts the stale-response guard to the other two lists). The |
2 participants
Need
Scanning an unclaimed QR (fresh label-sheet sticker,
Qr.organizationId = NULL) in the companion app is a dead end: the mobile resolver 404s and the scanner can only offer "Link in Browser". The single most valuable mobile workflow — scan a new sticker, have an asset seconds later, phone never leaves your hand — bounces the user to the web app mid-task. This was the top item on the post-launch backlog (unlinked-QR dead-end), and label-sheet customers hit it on literally every sticker of a new sheet.Hypothesis
If the companion can claim an unclaimed code into the active workspace and then either create a new asset or link an existing one — reusing the exact web services (
claimQrCode,updateAssetQrCode,createAsset) so semantics stay identical — then the label-sheet onboarding loop completes fully on-device, with no behavior change for web, the audit scanner, or non-admin roles.What changed
Webapp (mobile API):
resolveMobileScannedCode: the unclaimed 404 now carries additivereason: "unclaimed"+qrId(message/status byte-identical; plain not-found and wrong-org carry no reason — the app never offers claim for those). Kills the companion's error-message string matching.POST /api/mobile/qr/claim{ qrId }— wrapsclaimQrCode(); claims into the caller's current org only (no org selection from mobile). Gate:qr:update→ effectively ADMIN/OWNER, same as web.POST /api/mobile/qr/link-asset{ qrId, assetId }— wrapsupdateAssetQrCode()(QR-swap semantics identical to web);assetIdorg-scoped viaassertAssetsBelongToOrgperorg-scope-user-supplied-ids.claimQrCode()hardened with an atomic conditionalupdateWHERE (organizationId: null, assetId: null, kitId: null) — closes the two-orgs-claim-one-code race for web AND mobile.Companion:
openShelfWebUrl(deep-link-allowlist safe). Kit-linked QRs unchanged (web bridge)./(tabs)/assets/link-qrpicker: searchable paginated org-asset list (web-parity: no exclusions; confirm dialog carries the same "current QR will be replaced" warning), one-shot auto re-claim retry if the claim raced out, full empty/error/offline states, a11y labels.reasonbranching viaapiFetcherrorDetails;qr:updateadded tolib/permissions.tsmirroring the server's effective gate.Deliberately out of scope (no drift): kits stay web-bridged; no claim UI for BASE/SELF_SERVICE; no deep-link allowlist changes (no new claimed paths); Android untouched beyond shared JS.
Test plan
claimQrCoderace coverage (pre-check 403, P2025 lost-race → 403 not 404, atomic WHERE assertion). Behavior-driven: status codes, reasons, permission denials, org-mismatch — no mock-shape DB assertions.reason:"unclaimed"(exact contract)organizationId+userIdset (trueclaimQrCodeparity, not the loosecreateAssetbranch)asset/createwith qrId → no orgless-linked corruptionget-scanned-item) unchanged; wrong-org 403 carries no reason (no info leak); corrupted orgless-but-linked QR → no claim offered, claim route guards itorganizationId+userId+assetIdset (claim-first semantics).scanner.tsx, with why-comment).Verification / benchmark
pnpm webapp:validate: 3,644 tests green; lint 0 errors; typecheck clean.tsc --noEmit+expo lintclean;webapp:doctor+companion:doctor: zero newly-introduced errors.asset.create; the bulk bucket's 10/min would throttle claiming a physical label sheet.Known follow-up (deliberate, documented in-code)
qr.link-asset.tsuses read-then-write state guards (comment block explains the accepted race). Fully atomic guards belong inside sharedupdateAssetQrCodeso web and mobile harden together — separate PR to avoid forking the shared service here.Note for reviewers: Xcode Debug team change
project.pbxprojDebugDEVELOPMENT_TEAMchanged3V6BGGX7JS→27Q4MHFB8K(Shelf AssetManagement, Inc.). The old ID doesn't match the company team and blocked local device builds
(personal teams can't sign the Associated Domains entitlement). Release config untouched; EAS
cloud builds manage credentials independently. Flagging in case
3V6BGGX7JSwas intentionalfor someone else's local setup.
Reviewer starting points
apps/webapp/app/modules/api/mobile-code-resolve.server.ts(reason field) →apps/companion/lib/api/types.tsrequireMobilePermission(qr, update); server is source of truth,lib/permissions.tsonly mirrors for UIscanner.tsx(nevercreateAsset's loose unclaimed branch)🤖 Generated with Claude Code
Summary by CodeRabbit