feat: client/server ID split — Phase 1 (DB shim + API)#2447
Open
andrew-bierman wants to merge 8 commits into
Open
feat: client/server ID split — Phase 1 (DB shim + API)#2447andrew-bierman wants to merge 8 commits into
andrew-bierman wants to merge 8 commits into
Conversation
Phase 1 of the client/server ID split per docs/design/client-uuid-split.md §3 Option C. Each affected table gains a `client_uuid text UNIQUE NOT NULL` column, backfilled from the existing `id`, with a format CHECK constraint enforcing the URL-safe nanoid charset (≤64 chars). Tables touched: packs, pack_items, weight_history, pack_templates, pack_template_items, trips, trail_condition_reports. Also restores packages/api/src/db/schema.ts as a 1-line re-export shim (load-bearing for drizzle-kit per the #2414 plan §"Migration Infra"). The shim was deleted in 0154b87 along with all other re-export shims, which silently broke drizzle-kit generate since 2026-05-14. Refs: docs/plans/2026-05-17-001-feat-client-uuid-split-phase1-plan.md U1 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Re-adds packages/api/src/utils/ids.ts (deleted in 67e6afe T9 revert). Now serves two purposes during Phase 1: 1. Fills the text `id` PK (until Phase 2 swaps to bigserial). 2. Fills `client_uuid` for lean callers (MCP/CLI/web) — they don't have offline-first concerns so the server is the right owner of the idempotency token. The format (`<prefix>_<12hex>`) is URL-safe and within the 64-char limit the migration 0047 CHECK constraint enforces. Refs: docs/plans/2026-05-17-001-feat-client-uuid-split-phase1-plan.md U2 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 1 PR 2 of the client/server ID split (docs/design/client-uuid-split.md §5).
Each affected POST route now:
- Accepts optional `clientUuid` alongside legacy `id` (compat shim).
- Computes `id` and `clientUuid` per the three caller shapes:
* Legacy mobile sends `id` only → id round-trips, clientUuid = id.
* New mobile/web sends `clientUuid` → server mints fresh id.
* Lean callers (MCP/CLI) send neither → server mints both.
- Returns `clientUuid` on the response (now present on every row in DB).
- Idempotent retry via onConflictDoNothing on (client_uuid) + user-scoped
re-fetch (guards against cross-user namespace probing).
Routes updated: POST /packs, POST /packs/:packId/items,
POST /packs/:packId/weight-history, POST /trips, POST /pack-templates,
POST /pack-templates/:templateId/items, POST /trail-conditions/reports.
Also extends Zod request + response schemas in @packrat/schemas for
packs, trips, packTemplates, trailConditions. New shared `ClientUuidSchema`
in packs.ts enforces `^[A-Za-z0-9_-]{1,64}$` (matches the DB CHECK).
Seed data, test fixtures, packService.generatePacks all now populate
clientUuid (= id for synthetic rows, since they don't have a separate
client identity to preserve).
Refs: docs/plans/2026-05-17-001-feat-client-uuid-split-phase1-plan.md U3 U4 U5
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the client/server ID split design (docs/design/client-uuid-split.md §8 Q4), lean callers (this CLI, MCP, web) have no offline-first concerns. They should omit `clientUuid` and let the server mint both `id` and `clientUuid`. Drops `id: shortId(prefix)` from `packs create`, `trips create`, and `templates create` request bodies. The `shortId` helper stays in api/ids.ts — reserved for a future `--client-uuid` flag that enables explicit idempotent retries from the shell. Refs: docs/plans/2026-05-17-001-feat-client-uuid-split-phase1-plan.md U7 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the Phase 1 plan and updates the design doc: - PR 1 + PR 2: shipping in plan 2026-05-17-001 - PR 3, 4, 5: deferred to follow-up plans - PR 6 (Phase 2): separate design doc when ready Also extends the mobile compute-pack test fixture with clientUuid so the PackWithItems / PackItem types continue to typecheck after the schema change. (Mobile sync work proper happens in the deferred PR 3.) Refs: docs/plans/2026-05-17-001-feat-client-uuid-split-phase1-plan.md U8 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- packService.buildPackItems return: add `clientUuid` to the omitted fields so the synthetic items in generatePacks satisfy NewPackItem. - trips/store/trips.ts: cast TripSchema-parsed return through `unknown as TripInStore` until the local Trip interface aligns with @packrat/schemas (PR 3 of the client-uuid-split). The interface drift (description: string vs string|null) pre-dates this PR but my schema surfaced it via syncedCrud's stricter generic resolution. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (27)
WalkthroughThis PR implements Phase 1 of a client/server ID split feature, adding a ChangesClient/Server ID Split Phase 1
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged Math.random() in test fixtures as insecure (4 high alerts). Replace with the existing mintId helper from packages/api/src/utils/ids.ts — same crypto.randomUUID format as production rows. Dedupes the regex and the charset logic instead of duplicating it inline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves three CLI conflicts from dev's `runApi` refactor (positional args → options object). My "no clientUuid" changes preserved at the new call shape. Note: dev itself has 12 pre-existing typecheck errors (getRelativeTime test signature, embeddingHelper test tuple length, og-meta test imports). Confirmed by checking out origin/development directly — same 12 errors. Not introduced here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Deploying packrat-guides with
|
| Latest commit: |
1852074
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://42ce2158.packrat-guides-6gq.pages.dev |
| Branch Preview URL: | https://feat-client-uuid-split.packrat-guides-6gq.pages.dev |
Contributor
Deploying packrat-landing with
|
| Latest commit: |
1852074
|
| Status: | 🚫 Build failed. |
Contributor
There was a problem hiding this comment.
Pull request overview
Phase 1 of the client/server ID split: introduces a client_uuid idempotency token alongside the legacy id text PK for offline-first tables, updates API + schemas to accept/return clientUuid, and adjusts lean callers (CLI) to let the server mint identifiers.
Changes:
- Add
client_uuid(UNIQUE, NOT NULL, format CHECK) to 7 offline-first tables via migration + Drizzle schema updates. - Update API POST handlers to support
{id}legacy shape,{clientUuid}new shape, and “send neither” lean shape usingonConflictDoNothing(target: client_uuid)+ user-scoped re-fetch. - Update shared Zod schemas, CLI create commands, and test/fixture data to include/handle
clientUuid.
Reviewed changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/schemas/src/trips.ts | Add clientUuid to Trip schemas; allow optional id/clientUuid on create. |
| packages/schemas/src/trailConditions.ts | Add clientUuid to trail condition report schemas; allow optional id/clientUuid on create. |
| packages/schemas/src/packTemplates.ts | Add clientUuid to template + template item schemas; allow optional id/clientUuid on create. |
| packages/schemas/src/packs.ts | Introduce ClientUuidSchema; add clientUuid to pack-related request/response schemas. |
| packages/db/src/schema.ts | Add clientUuid columns to 7 tables in Drizzle schema. |
| packages/cli/src/commands/trips/index.ts | Stop minting client IDs for trip creation (server mints). |
| packages/cli/src/commands/templates/index.ts | Stop minting client IDs for template creation (server mints). |
| packages/cli/src/commands/packs/create.ts | Stop minting client IDs for pack creation (server mints). |
| packages/cli/src/api/ids.ts | Update docs/comments to reflect Phase 1 behavior; keep client-side helper for future use. |
| packages/api/test/packs.test.ts | Update mocked pack generation to include clientUuid. |
| packages/api/test/fixtures/pack-template-fixtures.ts | Update fixtures to populate clientUuid (and use server mint helper). |
| packages/api/test/fixtures/pack-fixtures.ts | Update fixtures to populate clientUuid (and use server mint helper). |
| packages/api/src/utils/ids.ts | Add server-side mintId(prefix) helper. |
| packages/api/src/utils/tests/compute-pack.test.ts | Update compute-pack tests to include clientUuid fields. |
| packages/api/src/services/packService.ts | Populate clientUuid when generating packs/items; update types accordingly. |
| packages/api/src/routes/trips/index.ts | Implement Phase 1 create semantics + idempotent insert for trips. |
| packages/api/src/routes/trailConditions/reports.ts | Implement Phase 1 create semantics + idempotent insert for trail condition reports. |
| packages/api/src/routes/packTemplates/index.ts | Implement Phase 1 create semantics + idempotent insert for templates and template items. |
| packages/api/src/routes/packs/index.ts | Implement Phase 1 create semantics + idempotent insert for packs, items, weight-history entries. |
| packages/api/src/db/seed.ts | Ensure seed inserts include clientUuid for templates/items. |
| packages/api/src/db/schema.ts | Restore Drizzle-kit re-export shim for schema discovery. |
| packages/api/drizzle/meta/0047_snapshot.json | Add schema snapshot for migration 0047. |
| packages/api/drizzle/meta/_journal.json | Record new migration entry in Drizzle journal. |
| packages/api/drizzle/0047_add_client_uuid.sql | Migration adding/backfilling client_uuid + UNIQUE + CHECK constraints. |
| docs/plans/2026-05-17-001-feat-client-uuid-split-phase1-plan.md | Add detailed implementation plan and requirements traceability. |
| docs/design/client-uuid-split.md | Update design status and PR shipping notes. |
| apps/expo/lib/utils/tests/compute-pack.test.ts | Update Expo compute-pack tests to include clientUuid. |
| apps/expo/features/trips/store/trips.ts | Add temporary casts/notes to bridge Trip schema/type drift pending PR 3. |
Comments suppressed due to low confidence (2)
packages/api/src/routes/packs/index.ts:712
- Same
onConflictDoNothingpattern here: if aclientUuidcollision happens with another user’s row, the re-fetch returns null and the handler responds with a generic 400. It would be clearer to return 409 Conflict (or prevent cross-user collisions with a composite UNIQUE on (user_id, client_uuid)).
(await db.query.packItems.findFirst({
where: and(eq(packItems.clientUuid, clientUuid), eq(packItems.userId, user.userId)),
}));
if (!newItem) return status(400, { error: 'Failed to create item' });
packages/api/src/routes/packTemplates/index.ts:711
- If
onConflictDoNothingis hit due to aclientUuidcollision owned by another user, the scoped re-fetch returns null and this currently returns 500. Consider returning 409 Conflict in that case for clearer semantics.
if (!newItem) return status(500, { error: 'Failed to create template item' });
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (!entry) return status(500, { error: 'Failed to create weight history entry' }); | ||
|
|
||
| return { ...entry, updatedAt: entry.createdAt }; |
Comment on lines
+128
to
132
| (await db.query.packs.findFirst({ | ||
| where: and(eq(packs.clientUuid, clientUuid), eq(packs.userId, user.userId)), | ||
| })); | ||
|
|
||
| if (!newPack) return status(500, { error: 'Failed to create pack' }); |
Comment on lines
+77
to
81
| (await db.query.trips.findFirst({ | ||
| where: and(eq(trips.clientUuid, clientUuid), eq(trips.userId, user.userId)), | ||
| })); | ||
|
|
||
| if (!newTrip) throw new Error('Failed to create trip'); |
Comment on lines
122
to
+126
| eq(trailConditionReports.userId, user.userId), | ||
| ), | ||
| }); | ||
| if (existing) return toReportResponse(existing); | ||
| return status(409, { error: 'Report ID already in use by another user' }); | ||
| } | ||
| })); | ||
|
|
||
| if (!newReport) return status(500, { error: 'Failed to submit report' }); |
| ), | ||
| })); | ||
|
|
||
| assertDefined(newTemplate, 'Failed to create pack template'); |
Comment on lines
+83
to
+90
| - `packages/api/drizzle/0048_add_client_uuid.sql` (new migration file) | ||
| - `packages/db/src/schema.ts` (modify seven `pgTable` definitions) | ||
| - `packages/schemas/src/{packs,packTemplates,trips,trailConditions}.ts` (modify request + response schemas) | ||
| - `packages/api/src/routes/{packs,packTemplates,trips,trailConditions}/*.ts` (modify route handlers) | ||
| - `packages/api/src/utils/ids.ts` (re-introduce — was removed in T9 revert) | ||
| - `packages/mcp/src/tools/{packs,trips}.ts` (drop client-side id minting from create tools) | ||
| - `packages/cli/src/commands/{packs,trips,templates}/*.ts` (drop `shortId` calls from create commands) | ||
| - `packages/api/test/{packs,trips}.test.ts` (new + extended idempotency tests) |
Comment on lines
+44
to
+50
| | R4 | Handlers compute the value as `data.clientUuid ?? data.id ?? mintId(prefix)` (compat shim — Phase 1 only). | §5.4 | | ||
| | R5 | Handlers use `onConflictDoNothing(target: clientUuid)` + re-fetch to guarantee idempotent retries (same `clientUuid` → same row). | §5.3 | | ||
| | R6 | Every entity response schema (Pack, PackItem, PackWeightHistory, PackTemplate, PackTemplateItem, Trip, TrailConditionReport) returns `clientUuid` alongside `id`. | §5.2 | | ||
| | R7 | Phase 1 keeps `id` text-typed; `mintId` continues filling `id` for inserts when the caller doesn't supply one. | §3 Phase 1 sketch | | ||
| | R8 | The API logs a deprecation warning (`deprecated_id_field`) when a request supplies `id` but not `clientUuid`, with userId + route, so we can measure cutover. | §5.4 | | ||
| | R9 | The migration is reversible at the DB layer (`DROP COLUMN client_uuid` per table) without any data loss. | §7 Phase 1 | | ||
| | R10 | MCP and CLI commands stop minting their own ids — they omit `clientUuid` from the request, letting the server fill it via `mintId`. | §8 Q4 (locked: server mints) | |
Comment on lines
+19
to
21
| // safe-cast: mobile TripInStore drifts from TripSchema (description nullability); aligned in PR 3. | ||
| return TripSchema.array().parse(data) as unknown as TripInStore[]; | ||
| }; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/api/src/routes/packTemplates/index.ts (1)
322-375:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
clientUuidin generate-from-online-content inserts.This endpoint doesn't set
clientUuidfor templates (line 327-343) or items (lines 345-367). Sinceclient_uuidis a non-null column per the migration, these inserts will fail at runtime.Proposed fix
For the template insert around line 328:
const [createdTemplate] = await tx .insert(packTemplates) .values({ id: templateId, + clientUuid: templateId, userId: user.userId, name: analysis.templateName,For the item records around line 348:
const itemId = `pti_${crypto.randomUUID().replace(STRIP_HYPHENS, '').slice(0, 21)}`; return { id: itemId, + clientUuid: itemId, packTemplateId: templateId,🤖 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 `@packages/api/src/routes/packTemplates/index.ts` around lines 322 - 375, The insert calls for packTemplates (creating newTemplate with id templateId) and packTemplateItems (created from itemRecords) are missing the required clientUuid column, causing runtime failures; update the packTemplates.values(...) used in the db.transaction and each object pushed into itemRecords so they include clientUuid: user.clientUuid (or the appropriate client UUID source), ensuring both the template insert and the subsequent packTemplateItems.insert(...) supply clientUuid for every row.
🤖 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/expo/features/trips/store/trips.ts`:
- Around line 17-21: The API response parsing currently assumes data/result is
present after calls like apiClient.trips.get and then calls
TripSchema.array().parse(...) (and similar parse calls at the other flows);
update each flow to guard for both error and missing payload by checking if
(error || !data) (or (error || !result)) before parsing, and throw a descriptive
Error (e.g., `Failed to list trips: ${error?.value ?? 'no data'}`) so parsing is
only invoked when data/result is defined; adjust the branches around
apiClient.trips.get, any other trips.* calls, and the TripSchema.parse usages
accordingly.
- Around line 20-21: The current double-cast "as unknown as TripInStore" after
TripSchema.parse hides mismatches (extra clientUuid and nullable fields) — add a
small mapper normalizeTripFromSchema(trip: z.infer<typeof TripSchema>):
TripInStore that strips clientUuid and enforces non-nullable fields (e.g.,
map/validate description) and return the shaped object as TripInStore; then
replace the three uses of the double-cast (the parse results where
TripSchema.array().parse(...) and the other two single-item parses) with
results.map(normalizeTripFromSchema) or normalizeTripFromSchema(parsed) to make
the transformation explicit and type-safe.
In `@docs/plans/2026-05-17-001-feat-client-uuid-split-phase1-plan.md`:
- Line 477: The migration risk statement contains a column name typo: replace
the incorrect reference `clients_uuid` with the correct column name
`client_uuid` in the Backfill description ("Backfill `UPDATE clients_uuid = id`
...") so the statement reads "Backfill `UPDATE client_uuid = id` ..." and ensure
any other occurrences of `clients_uuid` in this document are updated to
`client_uuid` for consistency with the migration and schema.
- Line 305: Resolve the contradiction by choosing a single uniqueness contract
for client identifiers: either keep the global UNIQUE on client_uuid or switch
to a per-user UNIQUE index on (user_id, client_uuid). Update the design doc text
(referencing "client_uuid UNIQUE" and the alternative "(user_id, client_uuid)"),
the migration and schema definitions that will create the index, and all
tests/expectations (U3/R1) to match the chosen contract so migrations,
telemetry, and tests are consistent; ensure the plan language and §2.2 reflect
the final decision.
In `@packages/api/drizzle/meta/0047_snapshot.json`:
- Around line 860-868: The snapshot is missing the new client_uuid CHECK
constraints; update the JSON's "checkConstraints" for each affected table to
include entries named like "<table>_client_uuid_format" (e.g.,
"pack_items_client_uuid_format") that reference the client_uuid column and the
same validation expression used in the migration (the UUID/check expression used
in your migration SQL); ensure each constraint object includes the constraint
"name", the "expression" (the UUID regex or check clause), and the target
"columns" (["client_uuid"]) so the snapshot matches the migration and prevents
schema drift.
In `@packages/api/src/utils/__tests__/compute-pack.test.ts`:
- Line 11: Tests hardcode clientUuid ('pack-1' / 'item-1') which diverges from
overridden id values; change the fixture definitions in compute-pack.test.ts so
clientUuid is derived from the test object's id (e.g., set clientUuid to the id
value or to a template based on id) instead of a fixed string—update both
occurrences where clientUuid is 'pack-1' and 'item-1' so they use the
corresponding id variable (reference symbols: clientUuid and id in the test
fixtures).
In `@packages/api/test/packs.test.ts`:
- Around line 41-44: The test's mock pack IDs use Date.now() making them
non-deterministic; update the ID generation in the loop that builds mockPacks so
it does not use Date.now() (e.g., change const id =
`generated-pack-${i}-${Date.now()}` to use the loop index or another stable
value like const id = `generated-pack-${i}`), ensuring mockPacks and clientUuid
remain deterministic across test runs.
In `@packages/db/src/schema.ts`:
- Line 112: The schema entry clientUuid (and the other similarly defined fields
like the ones at the other noted locations) is missing the migration's regex
CHECK invariant; update the schema to mirror the DB-level format constraint by
adding the same regex validation to the column definition (e.g., extend
clientUuid: text('client_uuid').unique().notNull() to include a regex guard or
validation method your schema system supports) or alternatively add an automated
parity test that asserts the schema validators match the migration CHECK for
client_uuid and the other listed fields; locate and change the clientUuid field
definition in packages/db/src/schema.ts (and the corresponding symbol names for
the other fields) so the code-level invariant matches the migration.
In `@packages/schemas/src/packs.ts`:
- Line 10: Replace the raw z.string() type used for the clientUuid field with
the existing ClientUuidSchema in all schemas (response and output) that
currently declare clientUuid (the three occurrences shown), and ensure
ClientUuidSchema is imported at the top of the file; update the clientUuid
property in each relevant schema definition to use ClientUuidSchema instead of
z.string().
In `@packages/schemas/src/packTemplates.ts`:
- Line 13: Replace the plain z.string() clientUuid fields in the entity schemas
with the shared ClientUuidSchema to keep validation consistent with request
schemas: locate the clientUuid entries in packTemplates entity schemas (the
occurrences around the shown clientUuid: z.string(), including the second
occurrence near line 32) and change them to use ClientUuidSchema; ensure
ClientUuidSchema is imported/available in this module and update any related
typings if necessary so both entity schemas use the same URL-safe nanoid (1-64
chars) validation.
In `@packages/schemas/src/trailConditions.ts`:
- Line 15: Replace the loose string type for the response field with the shared
ClientUuidSchema: change the trailConditions response schema's clientUuid
property (currently declared as clientUuid: z.string()) to use ClientUuidSchema
so the response enforces the same UUID format as the request schema; ensure you
import ClientUuidSchema at the top of packages/schemas/src/trailConditions.ts if
not already imported.
In `@packages/schemas/src/trips.ts`:
- Line 18: Replace the weak z.string() type for the clientUuid field in
TripSchema with the shared ClientUuidSchema to ensure consistent validation;
locate the TripSchema declaration and update the clientUuid property to use
ClientUuidSchema (the same validator used for create input) so request/response
payloads are validated the same way.
---
Outside diff comments:
In `@packages/api/src/routes/packTemplates/index.ts`:
- Around line 322-375: The insert calls for packTemplates (creating newTemplate
with id templateId) and packTemplateItems (created from itemRecords) are missing
the required clientUuid column, causing runtime failures; update the
packTemplates.values(...) used in the db.transaction and each object pushed into
itemRecords so they include clientUuid: user.clientUuid (or the appropriate
client UUID source), ensuring both the template insert and the subsequent
packTemplateItems.insert(...) supply clientUuid for every row.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7666cfb0-bd53-4987-aef3-707e013b30d3
📒 Files selected for processing (28)
apps/expo/features/trips/store/trips.tsapps/expo/lib/utils/__tests__/compute-pack.test.tsdocs/design/client-uuid-split.mddocs/plans/2026-05-17-001-feat-client-uuid-split-phase1-plan.mdpackages/api/drizzle/0047_add_client_uuid.sqlpackages/api/drizzle/meta/0047_snapshot.jsonpackages/api/drizzle/meta/_journal.jsonpackages/api/src/db/schema.tspackages/api/src/db/seed.tspackages/api/src/routes/packTemplates/index.tspackages/api/src/routes/packs/index.tspackages/api/src/routes/trailConditions/reports.tspackages/api/src/routes/trips/index.tspackages/api/src/services/packService.tspackages/api/src/utils/__tests__/compute-pack.test.tspackages/api/src/utils/ids.tspackages/api/test/fixtures/pack-fixtures.tspackages/api/test/fixtures/pack-template-fixtures.tspackages/api/test/packs.test.tspackages/cli/src/api/ids.tspackages/cli/src/commands/packs/create.tspackages/cli/src/commands/templates/index.tspackages/cli/src/commands/trips/index.tspackages/db/src/schema.tspackages/schemas/src/packTemplates.tspackages/schemas/src/packs.tspackages/schemas/src/trailConditions.tspackages/schemas/src/trips.ts
Comment on lines
17
to
21
| const { data, error } = await apiClient.trips.get({ query: { includePublic: 0 } }); | ||
| if (error) throw new Error(`Failed to list trips: ${error.value}`); | ||
| return TripSchema.array().parse(data); | ||
| // safe-cast: mobile TripInStore drifts from TripSchema (description nullability); aligned in PR 3. | ||
| return TripSchema.array().parse(data) as unknown as TripInStore[]; | ||
| }; |
Contributor
There was a problem hiding this comment.
Guard for !data before parsing API responses.
In Line 17/27/45 flows, the code checks error but still uses data/result without null checks. Add error || !data (and !result) guards before TripSchema.parse(...) to avoid undefined payload crashes.
Suggested diff
const listTrips = async () => {
const { data, error } = await apiClient.trips.get({ query: { includePublic: 0 } });
- if (error) throw new Error(`Failed to list trips: ${error.value}`);
+ if (error || !data) throw new Error(`Failed to list trips: ${error?.value ?? 'empty response'}`);
return TripSchema.array().parse(data) as unknown as TripInStore[];
};
@@
const { data, error } = await apiClient.trips.post({
@@
});
- if (error) throw new Error(`Failed to create trip: ${error.value}`);
+ if (error || !data) throw new Error(`Failed to create trip: ${error?.value ?? 'empty response'}`);
return TripSchema.parse(data) as unknown as TripInStore;
};
@@
const { data: result, error } = await apiClient.trips({ tripId: String(id) }).put({
@@
});
- if (error) throw new Error(`Failed to update trip: ${error.value}`);
+ if (error || !result) throw new Error(`Failed to update trip: ${error?.value ?? 'empty response'}`);
return TripSchema.parse(result) as unknown as TripInStore;
};As per coding guidelines, "Check for error || !data before using response data from the API client — responses are { data, error, status }".
Also applies to: 39-42, 55-58
🤖 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/expo/features/trips/store/trips.ts` around lines 17 - 21, The API
response parsing currently assumes data/result is present after calls like
apiClient.trips.get and then calls TripSchema.array().parse(...) (and similar
parse calls at the other flows); update each flow to guard for both error and
missing payload by checking if (error || !data) (or (error || !result)) before
parsing, and throw a descriptive Error (e.g., `Failed to list trips:
${error?.value ?? 'no data'}`) so parsing is only invoked when data/result is
defined; adjust the branches around apiClient.trips.get, any other trips.*
calls, and the TripSchema.parse usages accordingly.
Comment on lines
+20
to
21
| return TripSchema.array().parse(data) as unknown as TripInStore[]; | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify schema/store drift and all unsafe double-casts for trips.
set -euo pipefail
echo "== TripInStore type definition =="
fd -i "types.ts" apps/expo/features/trips --exec sed -n '1,220p' {}
echo
echo "== TripSchema definition =="
fd -i "trips.ts" packages/schemas/src --exec sed -n '1,220p' {}
echo
echo "== Unsafe casts in trips store =="
rg -n --type=ts "as unknown as TripInStore|as unknown as TripInStore\\[\\]" apps/expo/features/trips/store/trips.tsRepository: PackRat-AI/PackRat
Length of output: 3372
🏁 Script executed:
cat -n apps/expo/features/trips/store/trips.tsRepository: PackRat-AI/PackRat
Length of output: 4750
Replace as unknown as TripInStore with an explicit mapper function.
The schema parse returns objects with clientUuid and nullable fields that don't exist in TripInStore. The double-cast silently drops clientUuid and masks nullability mismatches (e.g., description can be null in schema but not in the store type). Create a small normalizer to make this transformation explicit:
const normalizeTripFromSchema = (trip: z.infer<typeof TripSchema>): TripInStore => {
const { clientUuid, ...rest } = trip;
return rest as TripInStore;
};Then use it at lines 20, 41, and 57 instead of the double-cast. This improves type safety and makes the implicit drift auditable.
🤖 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/expo/features/trips/store/trips.ts` around lines 20 - 21, The current
double-cast "as unknown as TripInStore" after TripSchema.parse hides mismatches
(extra clientUuid and nullable fields) — add a small mapper
normalizeTripFromSchema(trip: z.infer<typeof TripSchema>): TripInStore that
strips clientUuid and enforces non-nullable fields (e.g., map/validate
description) and return the shaped object as TripInStore; then replace the three
uses of the double-cast (the parse results where TripSchema.array().parse(...)
and the other two single-item parses) with results.map(normalizeTripFromSchema)
or normalizeTripFromSchema(parsed) to make the transformation explicit and
type-safe.
| - **Happy path — new pack without `clientUuid` (lean caller):** `POST /packs` with `{ name: 'Test' }`. Expect 200 with response containing a server-minted `clientUuid` matching `/^p_[a-f0-9]{12}$/`. | ||
| - **Compat path — old client sending `id`:** `POST /packs` with `{ id: 'p_legacy123', name: 'Test' }`. Expect 200 with response `{ id, clientUuid: 'p_legacy123', ... }`. Verify a deprecation warning fired (spy on `console.warn`). | ||
| - **Idempotency — same `clientUuid` twice:** Send `POST /packs` with `{ clientUuid: 'p_abc', name: 'Original' }`, then resend with `{ clientUuid: 'p_abc', name: 'Different' }`. Second call returns the *original* row (name still 'Original'). DB has exactly one row with `client_uuid = 'p_abc'`. | ||
| - **Cross-user isolation:** User A creates pack with `clientUuid: 'shared_uuid'`. User B's `POST /packs` with the same `clientUuid` succeeds (different `userId`) — verify two distinct rows exist with the same `client_uuid` only if the UNIQUE constraint is per-user-scoped. **Decision required during implementation:** the current design has `client_uuid UNIQUE` globally; if cross-user collision is a real concern, the UNIQUE becomes `(user_id, client_uuid)`. The design doc §2.2 says global UNIQUE; flag in PR review if telemetry suggests otherwise. |
Contributor
There was a problem hiding this comment.
Resolve client_uuid uniqueness scope contradiction before implementation.
Line 305 says cross-user duplicate clientUuid may be acceptable only if uniqueness is per-user, but earlier requirements/design lock client_uuid as globally unique. This conflict can lead to wrong migration/index choices and invalid test expectations. Pick one contract and make U3/R1/tests consistent with it.
🤖 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 `@docs/plans/2026-05-17-001-feat-client-uuid-split-phase1-plan.md` at line 305,
Resolve the contradiction by choosing a single uniqueness contract for client
identifiers: either keep the global UNIQUE on client_uuid or switch to a
per-user UNIQUE index on (user_id, client_uuid). Update the design doc text
(referencing "client_uuid UNIQUE" and the alternative "(user_id, client_uuid)"),
the migration and schema definitions that will create the index, and all
tests/expectations (U3/R1) to match the chosen contract so migrations,
telemetry, and tests are consistent; ensure the plan language and §2.2 reflect
the final decision.
|
|
||
| | Risk | Likelihood | Severity | Mitigation | | ||
| | --- | --- | --- | --- | | ||
| | Backfill `UPDATE clients_uuid = id` fails because some existing `id` values exceed 64 chars or contain disallowed chars. | Low | Medium — blocks migration | Spot-check production data length distribution + charset before merging PR A. Existing IDs are either `<prefix>_<12hex>` (16 chars) or 21-char nanoid (both URL-safe). Should be clean, but verify. | |
Contributor
There was a problem hiding this comment.
Fix column name typo in migration risk statement.
Line 477 says clients_uuid; it should be client_uuid. Keeping the exact column name avoids confusion during migration reviews.
🤖 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 `@docs/plans/2026-05-17-001-feat-client-uuid-split-phase1-plan.md` at line 477,
The migration risk statement contains a column name typo: replace the incorrect
reference `clients_uuid` with the correct column name `client_uuid` in the
Backfill description ("Backfill `UPDATE clients_uuid = id` ...") so the
statement reads "Backfill `UPDATE client_uuid = id` ..." and ensure any other
occurrences of `clients_uuid` in this document are updated to `client_uuid` for
consistency with the migration and schema.
Comment on lines
+860
to
+868
| "pack_items_client_uuid_unique": { | ||
| "name": "pack_items_client_uuid_unique", | ||
| "nullsNotDistinct": false, | ||
| "columns": ["client_uuid"] | ||
| } | ||
| }, | ||
| "policies": {}, | ||
| "checkConstraints": {}, | ||
| "isRLSEnabled": false |
Contributor
There was a problem hiding this comment.
Snapshot omits newly added client_uuid CHECK constraints.
The migration adds *_client_uuid_format checks, but these tables still show empty checkConstraints.
This creates schema/snapshot drift and can break future generated diffs.
Also applies to: 1021-1029, 1150-1158, 1231-1239, 1370-1378, 2001-2009, 2138-2146
🤖 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 `@packages/api/drizzle/meta/0047_snapshot.json` around lines 860 - 868, The
snapshot is missing the new client_uuid CHECK constraints; update the JSON's
"checkConstraints" for each affected table to include entries named like
"<table>_client_uuid_format" (e.g., "pack_items_client_uuid_format") that
reference the client_uuid column and the same validation expression used in the
migration (the UUID/check expression used in your migration SQL); ensure each
constraint object includes the constraint "name", the "expression" (the UUID
regex or check clause), and the target "columns" (["client_uuid"]) so the
snapshot matches the migration and prevents schema drift.
| // Packs table | ||
| export const packs = pgTable('packs', { | ||
| id: text('id').primaryKey(), | ||
| clientUuid: text('client_uuid').unique().notNull(), |
Contributor
There was a problem hiding this comment.
clientUuid format invariant is not represented in the canonical schema.
These fields are unique().notNull() here, but the DB migration also enforces a regex CHECK.
Please mirror that invariant in the schema definition (or add an explicit guardrail/test for migration↔schema parity) to prevent drift over time.
Also applies to: 230-230, 265-265, 279-279, 302-302, 331-331, 366-366
🤖 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 `@packages/db/src/schema.ts` at line 112, The schema entry clientUuid (and the
other similarly defined fields like the ones at the other noted locations) is
missing the migration's regex CHECK invariant; update the schema to mirror the
DB-level format constraint by adding the same regex validation to the column
definition (e.g., extend clientUuid: text('client_uuid').unique().notNull() to
include a regex guard or validation method your schema system supports) or
alternatively add an automated parity test that asserts the schema validators
match the migration CHECK for client_uuid and the other listed fields; locate
and change the clientUuid field definition in packages/db/src/schema.ts (and the
corresponding symbol names for the other fields) so the code-level invariant
matches the migration.
|
|
||
| export const PackItemSchema = z.object({ | ||
| id: z.string(), | ||
| clientUuid: z.string(), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "packs.ts" -path "*/schemas/src/*"Repository: PackRat-AI/PackRat
Length of output: 94
🏁 Script executed:
cat -n packages/schemas/src/packs.ts | head -250Repository: PackRat-AI/PackRat
Length of output: 8190
Use ClientUuidSchema in response and output schemas too.
clientUuid is currently z.string() here (and at lines 33, 200), bypassing the format constraint already defined for requests. Use ClientUuidSchema to keep contract enforcement consistent with what the database already enforces.
🤖 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 `@packages/schemas/src/packs.ts` at line 10, Replace the raw z.string() type
used for the clientUuid field with the existing ClientUuidSchema in all schemas
(response and output) that currently declare clientUuid (the three occurrences
shown), and ensure ClientUuidSchema is imported at the top of the file; update
the clientUuid property in each relevant schema definition to use
ClientUuidSchema instead of z.string().
|
|
||
| export const PackTemplateSchema = z.object({ | ||
| id: z.string(), | ||
| clientUuid: z.string(), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp/repo && cat -n packages/schemas/src/packTemplates.ts | head -50Repository: PackRat-AI/PackRat
Length of output: 122
🏁 Script executed:
cd /tmp/repo && grep -r "ClientUuidSchema" packages/schemas/src/ -A 2 -B 2Repository: PackRat-AI/PackRat
Length of output: 122
🏁 Script executed:
cd /tmp/repo && fd "packTemplates" packages/schemas/src/Repository: PackRat-AI/PackRat
Length of output: 122
🏁 Script executed:
cat -n packages/schemas/src/packTemplates.ts | head -50Repository: PackRat-AI/PackRat
Length of output: 1759
🏁 Script executed:
grep -r "ClientUuidSchema" packages/schemas/src/ -A 2 -B 2Repository: PackRat-AI/PackRat
Length of output: 3831
🏁 Script executed:
fd "ClientUuidSchema" packages/schemas/Repository: PackRat-AI/PackRat
Length of output: 44
Use ClientUuidSchema for clientUuid validation in entity schemas.
Entity schemas at lines 13 and 32 should reuse ClientUuidSchema instead of plain z.string(). Request schemas already enforce this validation (regex: URL-safe nanoid, 1-64 chars), so entity schemas should match to prevent inconsistencies between contract validation and stored data.
🤖 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 `@packages/schemas/src/packTemplates.ts` at line 13, Replace the plain
z.string() clientUuid fields in the entity schemas with the shared
ClientUuidSchema to keep validation consistent with request schemas: locate the
clientUuid entries in packTemplates entity schemas (the occurrences around the
shown clientUuid: z.string(), including the second occurrence near line 32) and
change them to use ClientUuidSchema; ensure ClientUuidSchema is
imported/available in this module and update any related typings if necessary so
both entity schemas use the same URL-safe nanoid (1-64 chars) validation.
|
|
||
| export const TrailConditionReportSchema = z.object({ | ||
| id: z.string(), | ||
| clientUuid: z.string(), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/schemas && find . -name "*.ts" -type f | head -20Repository: PackRat-AI/PackRat
Length of output: 405
🏁 Script executed:
cat -n packages/schemas/src/trailConditions.tsRepository: PackRat-AI/PackRat
Length of output: 2845
🏁 Script executed:
rg "ClientUuidSchema" packages/schemas/Repository: PackRat-AI/PackRat
Length of output: 944
🏁 Script executed:
rg "clientUuid" packages/schemas/ -A 2 -B 2Repository: PackRat-AI/PackRat
Length of output: 6355
Use ClientUuidSchema instead of z.string() to enforce the format constraint in the response.
Line 15: clientUuid: z.string() should be clientUuid: ClientUuidSchema to match the format validation applied in the request schema (line 40) and ensure consistency with the database constraints.
🤖 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 `@packages/schemas/src/trailConditions.ts` at line 15, Replace the loose string
type for the response field with the shared ClientUuidSchema: change the
trailConditions response schema's clientUuid property (currently declared as
clientUuid: z.string()) to use ClientUuidSchema so the response enforces the
same UUID format as the request schema; ensure you import ClientUuidSchema at
the top of packages/schemas/src/trailConditions.ts if not already imported.
|
|
||
| export const TripSchema = z.object({ | ||
| id: z.string(), | ||
| clientUuid: z.string(), |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Harden TripSchema.clientUuid with the shared validator.
Line 18 currently uses z.string(), which is weaker than the ClientUuidSchema used for create input. Using the same schema here keeps request/response validation aligned and catches malformed server payloads earlier.
Suggested diff
export const TripSchema = z.object({
id: z.string(),
- clientUuid: z.string(),
+ clientUuid: ClientUuidSchema,
name: z.string(),📝 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
| clientUuid: z.string(), | |
| export const TripSchema = z.object({ | |
| id: z.string(), | |
| clientUuid: ClientUuidSchema, | |
| name: z.string(), |
🤖 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 `@packages/schemas/src/trips.ts` at line 18, Replace the weak z.string() type
for the clientUuid field in TripSchema with the shared ClientUuidSchema to
ensure consistent validation; locate the TripSchema declaration and update the
clientUuid property to use ClientUuidSchema (the same validator used for create
input) so request/response payloads are validated the same way.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 of the client/server ID split per docs/design/client-uuid-split.md §3 Option C — adds a
client_uuididempotency token alongside the existingidPK on the seven offline-first tables. No PK type change yet (Phase 2 is a separate design doc).Plan: docs/plans/2026-05-17-001-feat-client-uuid-split-phase1-plan.md
What changed
client_uuid text UNIQUE NOT NULL, backfilled from existingid, with a format CHECK enforcing the URL-safe nanoid charset (≤64 chars).clientUuidand returns it on responses. Three caller shapes coexist:idonly → id round-trips,clientUuid = id(existing builds keep working).clientUuid→ server mints fresh id.onConflictDoNothing({ target: client_uuid })+ user-scoped re-fetch. Retrying the samePOST /packsreturns the same row.packs create,trips create,templates create— lean callers let the server mint per design doc §8 Q4.idin create tool inputs).packages/api/src/db/schema.tsre-export shim was deleted in0154b8711during the refactor: unify type system — drizzle → zod schemas → inferred TS types #2414 shim-purge — load-bearing for drizzle-kit, fixed here.Out of scope (separate plans)
clientUuid, types align with the schema (currently drift, papered over withsafe-castannotations intrips/store/trips.ts).clientUuidinstead ofid.id-on-create shim: once telemetry confirms mobile cutover.idtobigserial: its own design doc.Test plan
bun check-typespasses (verified locally).bun test:api:unitpasses.POST /packswith the sameclientUuidtwice → same row both times.POST /packswith onlyid(legacy mobile shape) →idround-trips.POST /packsfrompackrat packs create→ server-minted id + clientUuid in response.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation