feat(uploads): web+mobile upload helpers + shared upload types (#216)

[thomasluizon]

Frontend half of #216 — the client foundation for object uploads.

What this adds

• Shared contract (packages/shared): SignUploadRequest/SignUploadResponse Zod schemas, allowed content types, 8 MB cap (types/upload.ts), the API.uploads.sign endpoint constant, and uploadKeys.
• Web: app/actions/uploads.ts (Server Action via serverAuthFetch) + lib/upload-file.ts.
• Mobile (parity): lib/upload-file.ts via apiClient.
• Both helpers: sign → PUT the file straight to the signed URL → return only { key, publicUrl }; responses are Zod-parsed at the boundary.

No user-facing strings were added (these are low-level helpers), so no i18n keys.

Validation

• type-check + lint (0 errors) + test all green across shared / web / mobile (web alone: 1612 tests; the new upload tests pass on all three workspaces).

Closes #216. Backend PR (must also merge): thomasluizon/orbit-api#218

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
orbit-ui-mobile-web	Ignored		Jun 23, 2026 5:45pm

[claude]

PR Review - feat(uploads): web+mobile upload helpers + shared upload types. Decision: APPROVE - No Critical or High findings. Medium finding: signedUrl/publicUrl validated as URL but not enforced as HTTPS in packages/shared/src/types/upload.ts lines 17-18. SignUploadResponseSchema uses z.string().url() which accepts http:// - low risk since Supabase always generates HTTPS URLs, but no defense-in-depth. Fix: add .refine(u => u.startsWith('https://')) on both fields before the next PR exposes uploadFile to a UI callsite. All other dimensions clean: schema enforces UPLOAD_ALLOWED_CONTENT_TYPES enum and UPLOAD_MAX_SIZE_BYTES, parse() called at trust boundary on both platforms, StoredFile return type is precise, test coverage is behavior-focused across all schema paths, direct fetch() in mobile upload-file.ts is correct (not a workaround) since apiClient injects API_BASE and Bearer auth which are wrong for external Supabase URLs. Parity paired, i18n N/A, contract-aligner N/A in CI. Reviewed by automated PR reviewer.

[thomasluizon]

Code Review: PR #254 (orbit-ui-mobile)

Scope: PR #254 — web+mobile upload helpers + shared upload types (#216); paired with orbit-api#218
Recommendation: APPROVE

Summary

Frontend half of the object-storage slice: the shared Zod upload contract
(SignUploadRequest/Response, content-type allow-list, 8MB cap), endpoints + query
keys, a web Server Action + uploadFile helper, the mobile uploadFile parity helper,
and tests. contract-aligner confirms a full field-by-field MATCH against the
orbit-api#218 DTOs; parity is PAIRED. No Critical/High. The two Medium items the
cross-repo security pass surfaced are backend-side and are tracked on api#218 (one fixed,
one infra).

Findings

Critical / High

None.

Medium

None in this PR. Two backend Mediums live on orbit-api#218: (1) the sign endpoint
had no rate limit — fixed in api commit f330605; (2) the signed-URL PUT bypasses
the server-side size/MIME cap — requires Supabase bucket config (see api#218 review).

Low / Info

[INFO] sizeBytes is number (TS) ↔ long (C#) — safe; the 8MB cap is far below 2^53, no precision loss.

Subagents

Agent	Verdict
parity-checker	PAIRED (apps/web/lib/upload-file.ts ↔ apps/mobile/lib/upload-file.ts; adapters differ correctly: Server Action vs apiClient, File source)
i18n-syncer	N/A (no user-facing strings)
contract-aligner	MATCH (path + 3 request fields + 3 response fields + allow-list + 8MB cap all align with api#218)
security-reviewer	Covered the paired api#218 — see that PR's review

Validation

Check	Result
Lint	PASS
Type check	PASS
Tests	PASS (shared upload.test.ts, web + mobile upload-file.test.ts)
Build (api)	N/A (frontend PR)

What's good

• Clean adapter split: the contract is defined once in packages/shared; web uses a Server Action, mobile uses apiClient — no duplicated logic.
• x-upsert: 'true' on the PUT is safe: the object key is the server-generated {userId}/{guid}.{ext}, so a client can only overwrite the brand-new object it just signed for (confirmed in the api#218 security pass).
• The content-type allow-list and size cap live in shared and match the backend exactly.

Recommendation

APPROVE. Deploy the API (api#218) before the client relies on uploads. #216 auto-closes
via this PR; ensure api#218 also merges. Before enabling uploads in prod, complete
the two api#218 items: the rate-limit (already fixed) and the Supabase bucket
file_size_limit + allowed_mime_types (operator infra).

[claude]

Code Review: PR #254

Scope: PR #254 — feat(uploads): web+mobile upload helpers + shared upload types (#216)
Recommendation: NEEDS WORK

Summary

PR #254 adds the client foundation for object uploads: shared Zod schemas, the endpoint constant, and symmetric uploadFile helpers for web and mobile. The implementation is clean, well-tested, and follows established patterns. One High finding blocks merge: uploadKeys is exported from packages/shared/src/query/keys.ts but is missing from the packages/shared/src/query/index.ts barrel, making it unreachable to any consumer via @orbit/shared/query.

Findings

Critical

None.

High

[HIGH] uploadKeys missing from the shared query barrel

• dimension: Dead / stale code (rubric #2); CLAUDE.md rule 2 + rule 10
• location: packages/shared/src/query/index.ts (export absent)
• issue: uploadKeys is exported from packages/shared/src/query/keys.ts (lines 104-107) but is NOT listed in packages/shared/src/query/index.ts. Every other key factory (habitKeys, goalKeys, profileKeys, …, versionCheckKeys, checklistTemplateKeys) is explicitly named in that barrel. Consumers importing from @orbit/shared/query cannot reach uploadKeys. The only reference outside the definition is the test, which imports directly from ../query/keys — bypassing the barrel and masking the omission.
• risk: When a hook wraps signUpload and needs mutationKey or cache invalidation via uploadKeys.all, the import resolves to undefined at runtime. The test gives false confidence because it sidesteps the public surface.
• fix:

  // packages/shared/src/query/index.ts — add uploadKeys to the named export:
  export {
    …,
    versionCheckKeys,
    checklistTemplateKeys,
    uploadKeys,   // ← add this line
  } from './keys'

  Optionally update packages/shared/src/__tests__/upload.test.ts line 3 to import from '../query/index' rather than '../query/keys' so the barrel is tested as the public surface.
• reference: CLAUDE.md rule 2 (no dead/stale exports that ship); packages/shared/CLAUDE.md ("All public surfaces typed — this is the contract")

Medium

None.

Low / Info

None (signal gate applied).

Subagents

Agent	Verdict
parity-checker	PAIRED
i18n-syncer	IN SYNC
contract-aligner	MATCH (client-side; backend verified in orbit-api PR #218)
security-reviewer	N/A (no orbit-api changes in this PR)

Parity: Web uses a runtime assertAllowedContentType guard before signing; mobile constrains LocalUpload.contentType to the allowed union at the type level. Both are correct — TypeScript prevents an invalid LocalUpload from being constructed, and backend Zod is the source of truth for both paths. PAIRED.

Contract-aligner note: Confirm the C# DTO in orbit-api PR #218 has Filename absent (dropped in c45d8b7) and that key/signedUrl/publicUrl serialize camelCase.

Validation

Check	Result
Lint	PASS (per PR description: 0 errors)
Type check	PASS (per PR description: 0 errors)
Tests	PASS (per PR description: 1612 web + shared + mobile all green)
Build (api)	N/A

What's good

• Clean data flow: sign then PUT then return only {key, publicUrl}; signedUrl is never leaked to callers.
• Zod parse at the trust boundary: both helpers call SignUploadResponseSchema.parse() on the raw API response before using the signed URL — correct application of CLAUDE.md rule 8.
• Adapter split is correct: web goes through serverAuthFetch via a Server Action; mobile uses apiClient directly. Both are the right pattern per their respective CLAUDE.md files.
• Shared constants: UPLOAD_ALLOWED_CONTENT_TYPES and UPLOAD_MAX_SIZE_BYTES in packages/shared — single source of truth for both clients and the backend.
• Test coverage: three suites covering disallowed type, over-size, zero-size, non-URL signed URL, PUT failure, and happy path. Thorough.
• No any casts, no console.log, no narration comments anywhere in the diff.
• i18n judgment is correct: the two internal error strings are thrown from low-level helpers, never rendered to the user directly.

Recommendation

One fix before merging: add uploadKeys to the named export list in packages/shared/src/query/index.ts.

[claude]

Code Review: PR #254

Scope: PR #254 — feat(uploads): web+mobile upload helpers + shared upload types
Recommendation: NEEDS WORK

Summary

PR #254 adds the client-side foundation for object uploads: shared Zod schemas (SignUploadRequest/SignUploadResponse), the API.uploads.sign endpoint constant, and uploadFile helpers for both web and mobile. The overall design is sound — sign via API, PUT directly to object storage, return only {key, publicUrl}. Two issues block merge: a parity gap where mobile skips the runtime content-type guard that web enforces, and a StoredFile type that is duplicated across both apps instead of living in packages/shared.

---

Findings

Critical

None.

High

[HIGH] Mobile lacks runtime content-type validation before signing
· dimension: Parity (#9) / Correctness (#1)
· location: apps/mobile/lib/upload-file.ts:22
· issue: Web's uploadFile calls assertAllowedContentType(file.type) before the sign request and throws Unsupported content type locally. Mobile's LocalUpload.contentType is typed as (typeof UPLOAD_ALLOWED_CONTENT_TYPES)[number] — a compile-time constraint only. At runtime any caller that uses JSON-parsed data, a type cast, or receives the value from an unsafe source can pass a disallowed content type straight to apiClient, which then fires a sign request that the backend will reject with a server round-trip error instead of a fast local failure.
· risk: Behavioral divergence between platforms. Users on mobile get a slower, less informative failure path. Any consumer that constructs LocalUpload from external data (camera roll, share extension) and doesn't independently validate will silently rely on backend rejection.
· fix: Add the same guard to apps/mobile/lib/upload-file.ts before the apiClient call:

if (!(UPLOAD_ALLOWED_CONTENT_TYPES as readonly string[]).includes(upload.contentType)) {
  throw new Error(`Unsupported content type: ${upload.contentType}`)
}

Then add a matching test case to apps/mobile/__tests__/lib/upload-file.test.ts — the web test at line 47–50 (rejects a disallowed content type before signing) has no mobile mirror.
· reference: CLAUDE.md "Cross-platform parity (MANDATORY)" — "Logic, features, behavior, data flow, error handling: identical."

[HIGH] StoredFile type duplicated across both apps instead of shared
· dimension: SOLID / clean architecture (#3) / DRY at the right level (rule 10)
· location: apps/web/lib/upload-file.ts:8, apps/mobile/lib/upload-file.ts:10
· issue: Both files declare export type StoredFile = Pick<SignUploadResponse, 'key' | 'publicUrl'>. This type represents the canonical return value of an upload operation and will be referenced by callers in hooks, forms, and components in both platforms. Duplicating it here means any future change (e.g., adding bucket) requires touching two files.
· risk: The two definitions drift the moment one platform adds a field. packages/shared already owns SignUploadResponse; StoredFile belongs there.
· fix: Add export type StoredFile = Pick<SignUploadResponse, 'key' | 'publicUrl'> to packages/shared/src/types/upload.ts, export it from packages/shared/src/types/index.ts, then replace both app-local declarations with import { type StoredFile } from '@orbit/shared'.
· reference: CLAUDE.md rule 10 — "Cross-app duplication belongs in packages/shared."

Medium

[MEDIUM] uploadKeys registered as a query key but sign is a mutation
· dimension: SOLID / clean architecture (#3)
· location: packages/shared/src/query/keys.ts:104-107
· issue: uploadKeys.sign() returns ['uploads', 'sign'] and is exported from the query-keys barrel. TanStack Query mutation keys are separate from query keys — using a query key factory for a mutation is unconventional. No current callsite uses uploadKeys outside the test that verifies its shape, so the blast radius is zero today.
· risk: Future developers following the pattern will register a useMutation({ mutationKey: uploadKeys.sign() }) using this factory, which works but is misleading. If a query is ever accidentally keyed here it will create phantom cache entries.
· fix: Either rename uploadKeys to uploadMutationKeys to signal intent, or remove the sign key and set the mutationKey inline at the call site (the common pattern for simple one-off mutations). Can be addressed in a follow-up.
· reference: CLAUDE.md rule 1 — root cause over workarounds; correctness at the type/shape boundary.

---

Subagents

Agent	Verdict
parity-checker	PARTIAL — mobile missing runtime content-type guard and matching test
i18n-syncer	IN SYNC — no user-facing strings added; error throws are internal
contract-aligner	NOT VERIFIABLE IN CI — orbit-api repo not on runner; Filename removal risk flagged (see note)
security-reviewer	N/A — diff does not touch orbit-api

Contract note: The companion orbit-api PR #218 drops Filename from the sign request. The packages/shared side already removed it (commit c45d8b7). Verify deploy order: orbit-api#218 must be live before or simultaneously with this PR to avoid a window where the server still expects Filename but the client no longer sends it.

---

Validation

Check	Result
Lint	N/A — runner requires approval for npm; PR body claims 0 errors
Type check	N/A — same
Tests	N/A — same; PR body claims all green (web: 1612 tests)
Build (api)	N/A — orbit-api not on runner

---

What's good

• The sign → PUT → return {key, publicUrl} contract is clean. The helper correctly strips the signed URL from the return value so callers never persist a short-lived token.
• Zod parse at the trust boundary (SignUploadResponseSchema.parse(...)) on both platforms is exactly right.
• Web's assertAllowedContentType fast-fail guard is the correct pattern — the mobile fix is simply mirroring it.
• Shared schema constants (UPLOAD_ALLOWED_CONTENT_TYPES, UPLOAD_MAX_SIZE_BYTES) are the right single source of truth.
• Test coverage is strong on web and in the shared suite. The shared upload.test.ts schema matrix is especially valuable as a contract regression guard.
• No any, no console.log, no banned comments, no workarounds.

---

Recommendation

Two fixes required before merge:

1. apps/mobile/lib/upload-file.ts — add runtime content-type guard before apiClient call, mirroring web's assertAllowedContentType. Add the corresponding test to apps/mobile/__tests__/lib/upload-file.test.ts.
2. packages/shared/src/types/upload.ts — add and export StoredFile; remove the duplicated declarations from both app upload-file.ts files.

The uploadKeys naming issue (Medium) can be a tracked follow-up.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
93.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #254 — APPROVE

Summary

PR #254 adds the client-side upload foundation for object storage: shared Zod schemas and constants in packages/shared, a Server Action + helper in apps/web, and a mirrored helper in apps/mobile. The implementation is correct, well-tested, and architecturally sound. Parity is complete, i18n is N/A (no user-facing strings), and the shared contract is internally consistent. Two medium findings are raised; no Critical or High findings.

---

Findings

Critical

None

High

None

Medium

[MEDIUM] Mobile uploadFile calls fetch() directly without a WHY comment

• dimension: 5. No-workaround / root-cause
• location: apps/mobile/lib/upload-file.ts:36
• issue: apps/mobile/CLAUDE.md states "All mutations through apiClient. Never call fetch directly." The PUT to signed.signedUrl calls global fetch directly. The call is architecturally correct — apiClient prepends API_BASE and injects Bearer auth, neither of which is wanted for a Supabase pre-signed URL — but the rule exception is not documented. CLAUDE.md rule 1 allows an unavoidable workaround ONLY with a one-line WHY note linking an upstream issue/doc.
• risk: The next reviewer sees a naked fetch and may replace it with apiClient, breaking uploads (auth header sent to storage provider; URL mangled by API_BASE prepending).
• fix: Add a WHY comment with a Supabase URL above the fetch call, e.g.:

  // https://supabase.com/docs/guides/storage/uploads/signed-uploads — PUT targets the storage
  // provider directly via its pre-signed URL; apiClient cannot be used here because it prepends
  // API_BASE and injects a Bearer token that the storage provider would reject.
  const response = await fetch(signed.signedUrl, { ... })

• reference: CLAUDE.md rule 1 (root-cause over workarounds); CLAUDE.md rule 5 (WHY note requires upstream URL)

---

[MEDIUM] Web uploadFile test does not assert on PUT headers

• dimension: 1. Correctness (missing behavioral test coverage)
• location: apps/web/__tests__/lib/upload-file.test.ts:40-43
• issue: The web test asserts expect.objectContaining({ method: 'PUT', body: file }) but omits the headers assertion that the mobile test includes ({ 'Content-Type': file.type, 'x-upsert': 'true' }). The x-upsert: true header is required for Supabase to upsert rather than plain-insert; if it goes missing in a refactor the web test still passes while uploads silently fail or create duplicate objects in storage.
• risk: Silent regression: a future edit dropping x-upsert from the web path is undetected.
• fix:

  expect(fetchMock).toHaveBeenCalledWith(signed.signedUrl, expect.objectContaining({
    method: 'PUT',
    headers: { 'Content-Type': 'image/png', 'x-upsert': 'true' },
    body: file,
  }))

• reference: CLAUDE.md "Testing" (tests assert on behavior, not implementation details)

Low / Info

None

---

Subagents

Agent	Verdict
parity-checker	PAIRED — both platforms implement identical sign → PUT → StoredFile flow; adapter differences (Server Action vs apiClient, browser File vs LocalUpload) are intentional
i18n-syncer	N/A — no user-facing strings added; error messages are developer-facing exceptions
contract-aligner	MATCH (frontend-internal) / N/A (orbit-api inaccessible from CI) — schemas convention-consistent with expected C# DTOs; orbit-api#218 must be reviewed independently
security-reviewer	N/A — diff does not touch orbit-api

Validation

Check	Result
Lint	N/A (CI — PR body reports 0 errors)
Type check	N/A (CI — PR body reports 0 errors)
Tests	N/A (CI — PR body reports all green: 1612 web tests pass)
Build (api)	N/A — no orbit-api changes

---

What's good

• Shared contract is crisp: all upload constants and schemas in one file, exported cleanly, consumed symmetrically by both apps.
• Content-type guard fires before any network call in both implementations — fail-fast, no wasted round-trip.
• StoredFile = Pick<SignUploadResponse, 'key' | 'publicUrl'> elegantly strips the ephemeral signedUrl before it can be persisted.
• Zod parse at both trust boundaries (SignUploadResponseSchema.parse(...)) is exactly right — the signed URL is an external value that must be validated before use.
• Test coverage is thorough across all three workspaces: happy path, disallowed content-type guard, and PUT failure all exercised.
• uploadMutationKeys is consistent with the rest of keys.ts and properly exported from the query barrel.

Recommendation

Zero Critical/High findings — this PR is ready to merge. Two medium findings are quick follow-ups:

1. Add a WHY comment with a Supabase docs URL above the fetch call in apps/mobile/lib/upload-file.ts to protect the next reviewer.
2. Add the headers assertion to the web test to match mobile's rigor and guard against silent x-upsert regressions.

🤖 Generated with Claude Code

[claude]

PR 254: APPROVE. No Critical or High findings. The upload implementation is solid. Two non-blocking Medium items: (1) apps/mobile/lib/upload-file.ts:36 bare fetch() needs a WHY comment explaining apiClient cannot be used here (it injects Bearer which Supabase storage rejects); (2) apps/web/tests/lib/upload-file.test.ts:40-43 web PUT mock should assert x-upsert header like the mobile test does.