Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions .claude/rules/cross-app-mirrors-need-provenance.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
description: Hand-copied webapp logic in the companion (or any second app) must be marked as a mirror, point at its source, and prefer extraction to packages/*
description: Hand-copied webapp logic in the companion must be marked as a mirror, point at its source, and prefer extraction to packages/*. Companion is the only secondary app today — extend the glob when another appears.
globs: apps/companion/**
---

Expand Down Expand Up @@ -35,5 +35,7 @@ const ROLE_PERMISSIONS = { OWNER: { qr: ["read", "update"] } };
*/
```

Existing mirrors: `apps/companion/lib/permissions.ts`. When you touch one,
diff it against its canonical source before shipping.
Existing mirrors: none — the permissions mirror was extracted to
`@shelf/permissions` (packages/permissions). If you create a new mirror,
add it to this list; when you touch one, diff it against its canonical
source before shipping.
68 changes: 34 additions & 34 deletions apps/companion/app/(tabs)/assets/link-qr.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,7 @@ import * as Haptics from "expo-haptics";
import { Image } from "expo-image";
import { useRouter, useLocalSearchParams } from "expo-router";
import { Ionicons } from "@expo/vector-icons";
import {
api,
type AssetListItem,
type QrResolveFailureReason,
} from "@/lib/api";
import { api, type AssetListItem } from "@/lib/api";
import { useOrg } from "@/lib/org-context";
import {
fontSize,
Expand Down Expand Up @@ -103,6 +99,26 @@ function LinkQrContent() {
return () => clearTimeout(timer);
}, [searchInput]);

/**
* Workspace-switch guard (mirrors the scanner's originOrgId pin): the
* scanned `qrId` belongs to the workspace that was active when the picker
* opened. If the user switches workspaces mid-flow, the list would show
* the NEW org's assets while the link targets the OLD org's code — the
* server would 403 on confirm, a confusing dead end. Leave instead.
*/
const originOrgIdRef = useRef(currentOrg?.id);
useEffect(() => {
if (!originOrgIdRef.current || !currentOrg?.id) return;
if (currentOrg.id !== originOrgIdRef.current) {
Alert.alert(
"Workspace Changed",
"The scanned QR code belongs to the workspace you started in. Scan it again from this workspace to link it here.",
[{ text: "OK", onPress: () => router.back() }],
{ cancelable: false }
);
}
}, [currentOrg?.id, router]);

Comment on lines +102 to +121

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate link-qr.tsx and scanner.tsx =="
fd -a 'link-qr.tsx|scanner.tsx$' . | sed 's#^\./##'

echo "== outline link-qr.tsx =="
if [ -f "apps/companion/app/(tabs)/assets/link-qr.tsx" ]; then
  ast-grep outline "apps/companion/app/(tabs)/assets/link-qr.tsx" || true
  echo "== relevant lines =="
  sed -n '1,240p' "apps/companion/app/(tabs)/assets/link-qr.tsx" | nl -ba | sed -n '1,260p'
fi

echo "== locate scanner file =="
scanner_file="$(fd -a 'scanner.tsx$' . | grep -F 'apps/companion/app/(tabs)' | head -n1 || true)"
echo "$scanner_file"
if [ -n "$scanner_file" ] && [ -f "$scanner_file" ]; then
  wc -l "$scanner_file"
  ast-grep outline "$scanner_file" || true
  echo "== scanner refs/effects/org guard =="
  rg -n "activeOrgIdRef|originOrgId|useEffect|currentOrg" "$scanner_file" -C 4
fi

echo "== programmatic probe of one-shot useRef behavior from original effect =="
node - <<'JS'
const useRef = (initial) => ({ current: initial });
function run(original, currentOrgs = [undefined, { id: 'org-a' }, { id: 'org-b' }]) {
  const originOrgIdRef = useRef(currentOrgs[0]?.id);
  const alerts = [];
  function go(n) {
    if (!originOrgIdRef.current || !currentOrgs[n]?.id) return;
    if (currentOrgs[n].id !== originOrgIdRef.current) alerts.push({ at: n, current: currentOrgs[n].id, origin: originOrgIdRef.current });
  }
  currentOrgs.forEach(go);
  return { currentValue: originOrgIdRef.current, alerts };
}
console.log("original-first-undefined:", JSON.stringify(run(true)));
console.log("hardened-version:", (() => {
  const originOrgIdRef = useRef(undefined);
  const currentOrgs = [undefined, { id: 'org-a' }, { id: 'org-b' }];
  const alerts = [];
  currentOrgs.forEach((currentOrg, n) => {
    if (!currentOrg?.id) return;
    if (!originOrgIdRef.current) {
      originOrgIdRef.current = currentOrg.id;
      return;
    }
    if (currentOrg.id !== originOrgIdRef.current) alerts.push({ at: n, current: currentOrg.id, origin: originOrgIdRef.current });
  });
  return JSON.stringify({ currentValue: originOrgIdRef.current, alerts });
})());
JS

Repository: Shelf-nu/shelf.nu

Length of output: 789


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== find useOrg definition/usages =="
rg -n "function useOrg|const useOrg|useOrg =|export .*useOrg|useOrg\\(" apps packages lib . --glob '!node_modules' --glob '!dist' --glob '!build' -C 3

echo "== companion assets route files =="
fd -a '.*' "apps/companion/app/(tabs)/assets" || true

Repository: Shelf-nu/shelf.nu

Length of output: 25422


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== apps/companion/lib/org-context.tsx relevant sections =="
sed -n '1,190p' apps/companion/lib/org-context.tsx | awk '{printf "%5d  %s\n", NR, $0}'

echo "== apps/companion/app/(tabs)/assets/link-qr.tsx relevant sections =="
sed -n '55,140p' "apps/companion/app/(tabs)/assets/link-qr.tsx" | awk '{printf "%5d  %s\n", NR+54, $0}'
sed -n '140,220p' "apps/companion/app/(tabs)/assets/link-qr.tsx" | awk '{printf "%5d  %s\n", NR+139, $0}'

echo "== apps/companion/app/(tabs)/scanner.tsx origin/org guard sections =="
sed -n '160,225p' "apps/companion/app/(tabs)/scanner.tsx" | awk '{printf "%5d  %s\n", NR+159, $0}'
rg -n "originOrgId|activeOrgIdRef|useEffect" "apps/companion/app/(tabs)/scanner.tsx" -C 5

echo "== programmatic probe of original guard with first render undefined =="
node - <<'JS'
function originalGuard(initialCurrentOrg, renders) {
  const originOrgIdRef = { current: initialCurrentOrg?.id ?? undefined };
  const alerts = [];
  for (const [n, currentOrg] of renders.entries()) {
    if (!originOrgIdRef.current || !currentOrg?.id) continue;
    if (currentOrg.id !== originOrgIdRef.current) alerts.push({ at: n, current: currentOrg.id, origin: originOrgIdRef.current });
  }
  return { currentValue: originOrgIdRef.current, alerts };
}
function hardenedGuard(initialCurrentOrg, renders) {
  const originOrgIdRef = { current: initialCurrentOrg?.id ?? undefined };
  const alerts = [];
  for (const [n, currentOrg] of renders.entries()) {
    if (!currentOrg?.id) continue;
    if (!originOrgIdRef.current) {
      originOrgIdRef.current = currentOrg.id;
      continue;
    }
    if (currentOrg.id !== originOrgIdRef.current) alerts.push({ at: n, current: currentOrg.id, origin: originOrgIdRef.current });
  }
  return { currentValue: originOrgIdRef.current, alerts };
}
const renders = [undefined, { id: 'org-a' }, { id: 'org-b' }];
console.log(JSON.stringify({
  original: originalGuard(undefined, renders),
  hardened: hardenedGuard(undefined, renders)
}, null, 2));
JS

Repository: Shelf-nu/shelf.nu

Length of output: 20010


Initialize the workspace-switch guard when currentOrg becomes available.

originOrgIdRef is set once at mount to currentOrg?.id, and OrgProvider initializes currentOrg as null during loading. If this screen renders before the org is loaded, originOrgIdRef.current stays undefined, so the guard never fires for the first valid org. Store the first available org once, then alert on subsequent changes.

🛡️ Suggested hardening
   const originOrgIdRef = useRef(currentOrg?.id);
   useEffect(() => {
-    if (!originOrgIdRef.current || !currentOrg?.id) return;
+    if (!currentOrg?.id) return;
+    if (!originOrgIdRef.current) {
+      originOrgIdRef.current = currentOrg.id;
+      return;
+    }
     if (currentOrg.id !== originOrgIdRef.current) {
📝 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
/**
* Workspace-switch guard (mirrors the scanner's originOrgId pin): the
* scanned `qrId` belongs to the workspace that was active when the picker
* opened. If the user switches workspaces mid-flow, the list would show
* the NEW org's assets while the link targets the OLD org's code the
* server would 403 on confirm, a confusing dead end. Leave instead.
*/
const originOrgIdRef = useRef(currentOrg?.id);
useEffect(() => {
if (!originOrgIdRef.current || !currentOrg?.id) return;
if (currentOrg.id !== originOrgIdRef.current) {
Alert.alert(
"Workspace Changed",
"The scanned QR code belongs to the workspace you started in. Scan it again from this workspace to link it here.",
[{ text: "OK", onPress: () => router.back() }],
{ cancelable: false }
);
}
}, [currentOrg?.id, router]);
/**
* Workspace-switch guard (mirrors the scanner's originOrgId pin): the
* scanned `qrId` belongs to the workspace that was active when the picker
* opened. If the user switches workspaces mid-flow, the list would show
* the NEW org's assets while the link targets the OLD org's code the
* server would 403 on confirm, a confusing dead end. Leave instead.
*/
const originOrgIdRef = useRef(currentOrg?.id);
useEffect(() => {
if (!currentOrg?.id) return;
if (!originOrgIdRef.current) {
originOrgIdRef.current = currentOrg.id;
return;
}
if (currentOrg.id !== originOrgIdRef.current) {
Alert.alert(
"Workspace Changed",
"The scanned QR code belongs to the workspace you started in. Scan it again from this workspace to link it here.",
[{ text: "OK", onPress: () => router.back() }],
{ cancelable: false }
);
}
}, [currentOrg?.id, router]);
🤖 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/app/`(tabs)/assets/link-qr.tsx around lines 102 - 121, Update
the workspace guard around originOrgIdRef and its useEffect so the first
available currentOrg.id is stored when it becomes available, while preserving it
thereafter. Continue alerting and navigating back only when a later
currentOrg.id differs from the captured origin workspace.

/**
* Monotonic token identifying the newest fetch. Search/workspace changes
* rebuild `fetchAssets` (new deps) and the list effect starts a fresh
Expand Down Expand Up @@ -161,37 +177,21 @@ function LinkQrContent() {
};

/**
* POST the link, recovering once from a claim that didn't stick: a 400 with
* `reason: "unclaimed"` means the QR lost (or never got) its org — re-run
* the claim and retry the link a single time before surfacing the error.
* POST the link. No claim-recovery path: the link-asset endpoint delegates
* to `relinkAssetQrCode`, which claims an unclaimed code inline as part of
* the link, so a `reason: "unclaimed"` failure can no longer occur here.
*
* @returns The final `{ error }` string, or `null` on success.
* @returns The `{ error }` string, or `null` on success.
*/
const linkWithClaimRecovery = useCallback(
const linkQr = useCallback(
async (linkQrId: string, assetId: string): Promise<string | null> => {
if (!currentOrg) return "No workspace selected.";
const first = await api.linkQrToAsset(currentOrg.id, linkQrId, assetId);
if (!first.error) return null;
// `satisfies` ties the literal to the wire contract type, so a typo
// (or a server-side rename) fails to compile.
if (
first.errorDetails?.reason !==
("unclaimed" satisfies QrResolveFailureReason)
) {
return first.error;
}

// Claim didn't stick — claim into the current org and retry once.
// The claim's error is deliberately NOT short-circuited: its 403 is
// generic ("Failed to claim qr code") and also covers a
// timed-out-but-landed claim or a same-org teammate winning the race —
// cases where the code IS now claimed by this org and the retry
// succeeds (mirrors the scanner's re-resolve recovery). The link
// endpoint's own guards (unclaimed / wrong-org / already-linked)
// produce the definitive, accurate error either way.
await api.claimQr(currentOrg.id, linkQrId);
const second = await api.linkQrToAsset(currentOrg.id, linkQrId, assetId);
return second.error;
const { error } = await api.linkQrToAsset(
currentOrg.id,
linkQrId,
assetId
);
return error ?? null;
},
[currentOrg]
);
Expand All @@ -218,7 +218,7 @@ function LinkQrContent() {
text: "Link",
onPress: async () => {
setLinkingAssetId(asset.id);
const linkError = await linkWithClaimRecovery(qrId, asset.id);
const linkError = await linkQr(qrId, asset.id);
setLinkingAssetId(null);

if (linkError) {
Expand Down Expand Up @@ -266,7 +266,7 @@ function LinkQrContent() {
{ cancelable: false }
);
},
[qrId, currentOrg, linkingAssetId, linkWithClaimRecovery, router]
[qrId, currentOrg, linkingAssetId, linkQr, router]
);

const renderAsset = useCallback(
Expand Down
18 changes: 12 additions & 6 deletions apps/companion/app/(tabs)/scanner.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -540,10 +540,9 @@ function ScannerContent() {
* org won the race, permission revoked) surfaces as an error card.
*
* @param claimQrId - The unclaimed QR id (from the resolve error payload).
* @param next - Which follow-up to open once the code is claimed.
*/
const claimQrAndProceed = useCallback(
async (claimQrId: string, next: "create" | "link") => {
async (claimQrId: string) => {
// Same lock discipline as handleBarCodeScanned: the result card's two
// buttons are plain touchables, so a rapid double-tap (or one tap on
// each) would otherwise start two concurrent claim flows and stack two
Expand Down Expand Up @@ -611,8 +610,7 @@ function ScannerContent() {
// a clean slate (and re-scanning the same label resolves fresh).
dismissResult();
pushIntoTab("/(tabs)/assets", {
pathname:
next === "create" ? "/(tabs)/assets/new" : "/(tabs)/assets/link-qr",
pathname: "/(tabs)/assets/new",
params: { qrId: claimQrId },
});
},
Expand Down Expand Up @@ -733,14 +731,22 @@ function ScannerContent() {
label: "Create New Asset",
icon: "add-circle-outline",
onPress: () => {
void claimQrAndProceed(unclaimedQrId, "create");
void claimQrAndProceed(unclaimedQrId);
},
},
secondaryAction: {
label: "Link Existing Asset",
icon: "link-outline",
// No claim step: the link-asset endpoint delegates to
// relinkAssetQrCode, which claims an unclaimed code inline
// as part of the link. Navigating directly also means an
// abandoned picker leaves the label unclaimed for anyone.
onPress: () => {
void claimQrAndProceed(unclaimedQrId, "link");
dismissResult();
pushIntoTab("/(tabs)/assets", {
pathname: "/(tabs)/assets/link-qr",
params: { qrId: unclaimedQrId },
});
},
},
});
Expand Down
91 changes: 25 additions & 66 deletions apps/companion/lib/permissions.ts
Original file line number Diff line number Diff line change
@@ -1,64 +1,19 @@
/**
* Client-side permission helpers for the mobile companion app.
*
* Mirrors the webapp's Role2PermissionMap to determine which UI actions
* a user can see based on their organization role. These checks are
* purely cosmetic (hide/show UI) — the server enforces permissions
* via requireMobilePermission on every API call.
*/

type PermissionEntity = "asset" | "booking" | "audit" | "kit" | "qr";
type PermissionAction =
| "read"
| "create"
| "update"
| "delete"
| "custody"
| "checkout"
| "checkin";

/**
* Simplified permission map matching the webapp's Role2PermissionMap.
* Only includes entities/actions relevant to mobile scanner actions.
* No longer a hand-copied mirror: both helpers delegate to the shared
* `@shelf/permissions` package — the SAME matrix + resolution logic
* (including the ADMIN/OWNER allow-all short-circuit) that the webapp's
* server validator uses, so web and mobile can never drift.
*
* These checks remain purely cosmetic (hide/show UI) — the server
* independently enforces permissions via requireMobilePermission on every
* API call.
*
* @see {@link file://../../../packages/permissions/src/index.ts}
*/
const ROLE_PERMISSIONS: Record<
string,
Record<PermissionEntity, PermissionAction[]>
> = {
OWNER: {
asset: ["read", "create", "update", "delete", "custody"],
booking: ["read", "create", "update", "delete", "checkout", "checkin"],
audit: ["read", "create", "update", "delete"],
kit: ["read", "create", "update", "delete", "custody"],
// qr:update gates the native claim / link-existing flows. The server
// short-circuits ADMIN/OWNER to allow-all, so listing it here mirrors
// the effective server behaviour rather than the literal map.
qr: ["read", "update"],
},
ADMIN: {
asset: ["read", "create", "update", "delete", "custody"],
booking: ["read", "create", "update", "delete", "checkout", "checkin"],
audit: ["read", "create", "update", "delete"],
kit: ["read", "create", "update", "delete", "custody"],
qr: ["read", "update"],
},
SELF_SERVICE: {
asset: ["read", "custody"],
booking: ["read", "create", "update", "checkout", "checkin"],
audit: ["read", "update"],
kit: ["read", "custody"],
// Web's Role2PermissionMap grants BASE / SELF_SERVICE qr:read only —
// they never see the native claim / link actions.
qr: ["read"],
},
BASE: {
asset: ["read"],
booking: ["read"],
audit: ["read"],
kit: ["read"],
qr: ["read"],
},
};
import type { PermissionAction, PermissionEntity } from "@shelf/permissions";
import { roleHasPermission } from "@shelf/permissions";

/**
* Returns true when the user holds an org role that grants visibility
Expand Down Expand Up @@ -87,21 +42,25 @@ export function userCanSeeOrgWideAudits(roles: string[] | undefined): boolean {
/**
* Checks if a user with the given roles has permission for an entity/action.
* Returns true if any of the user's roles grant the permission.
*
* Thin adapter over the shared resolver: call sites keep passing plain
* string literals (`"asset"`, `"create"`), which the shared package accepts
* as template-literal types of its enums.
*
* @param roles - The user's org-role strings as returned by `/me`.
* @param entity - The permission entity being checked.
* @param action - The action being checked on that entity.
* @returns `true` when any held role grants the action on the entity.
* @throws Never — unknown roles/entities safely resolve to `false`.
*/
export function userHasPermission({
roles,
entity,
action,
}: {
roles: string[] | undefined;
entity: PermissionEntity;
action: PermissionAction;
entity: `${PermissionEntity}`;
action: `${PermissionAction}`;
}): boolean {
if (!roles?.length) return false;

return roles.some((role) => {
const perms = ROLE_PERMISSIONS[role];
if (!perms) return false;
return perms[entity]?.includes(action) ?? false;
});
return roleHasPermission({ roles, entity, action });
}
1 change: 1 addition & 0 deletions apps/companion/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"@react-navigation/native": "^7.0.14",
"@sentry/react-native": "~7.2.0",
"@shelf/labels": "workspace:*",
"@shelf/permissions": "workspace:*",
"@supabase/supabase-js": "^2.49.1",
"expo": "~54.0.33",
"expo-av": "^16.0.8",
Expand Down
8 changes: 6 additions & 2 deletions apps/webapp/app/modules/qr/service.server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,9 @@ describe("claimQrCode", () => {
const err = await captureClaimThrow();

expect(err.status).toBe(403);
expect(err.message).toBe("Failed to claim qr code");
expect(err.message).toBe(
"This QR code already belongs to an organization so you cannot claim it."
);
expect(db.qr.update).not.toHaveBeenCalled();
});

Expand All @@ -159,7 +161,9 @@ describe("claimQrCode", () => {
// not-found (404) — makeShelfError would collapse a propagated P2025
// to a 404 if the mapping branch were removed.
expect(err.status).toBe(403);
expect(err.message).toBe("Failed to claim qr code");
expect(err.message).toBe(
"This QR code has already been claimed or linked so you cannot claim it."
);
});

it("claims atomically: the update WHERE requires the unclaimed AND unlinked state", async () => {
Expand Down
8 changes: 8 additions & 0 deletions apps/webapp/app/modules/qr/service.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,14 @@ export async function claimQrCode({
throw updateCause;
}
} catch (cause) {
// why: our own guard errors above carry user-facing messages ("not
// available for claiming", already-claimed 403). ShelfError copies
// status/title from a cause but NOT message, so re-wrapping would ship
// the generic "Failed to claim qr code" to clients (the mobile error
// envelope only carries `message`). Pass our own errors through intact.
if (isLikeShelfError(cause)) {
throw cause;
}
throw new ShelfError({
cause,
message: "Failed to claim qr code",
Expand Down
7 changes: 5 additions & 2 deletions apps/webapp/app/routes/api+/mobile+/qr.claim.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ describe("POST /api/mobile/qr/claim", () => {
vi.mocked(claimQrCode).mockRejectedValue(
new ShelfError({
cause: null,
message: "Failed to claim qr code",
message:
"This QR code already belongs to an organization so you cannot claim it.",
label: "QR",
status: 403,
})
Expand All @@ -142,7 +143,9 @@ describe("POST /api/mobile/qr/claim", () => {
const { body, status } = await callAction({ qrId: "qr-1" });

expect(status).toBe(403);
expect(body.error?.message).toBe("Failed to claim qr code");
expect(body.error?.message).toBe(
"This QR code already belongs to an organization so you cannot claim it."
);
});

it("returns 400 for a body without a qrId", async () => {
Expand Down
5 changes: 5 additions & 0 deletions apps/webapp/app/routes/api+/mobile+/qr.claim.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
PermissionAction,
PermissionEntity,
} from "~/utils/permissions/permission.data";
import { enforceUserRateLimit } from "~/utils/rate-limit.server";

/** Zod schema for the claim JSON body. */
const ClaimQrSchema = z.object({
Expand Down Expand Up @@ -79,6 +80,10 @@ export async function action({ request }: ActionFunctionArgs) {
action: PermissionAction.update,
});

// Claiming is irreversible on a shared physical label — per-user write
// bucket (generous enough for a full label sheet, unlike "bulk").
await enforceUserRateLimit(user.id, "write");

// why: raw `.parse` surfaces a ZodError as a 500 through makeShelfError's
// unknown-error branch, and `request.json()` itself throws a SyntaxError
// (also a 500) on a non-JSON/empty body — `.catch(() => null)` funnels
Expand Down
Loading
Loading