✨ add Hono RPC foundation#2268
Merged
Merged
Conversation
- apps/expo/lib/api/rpcTransport.ts: refactor processQueue to take options
object so Biome's useMaxParams rule passes.
- biome-driven import sorting across rpc foundation files
(api-client src/index.ts, client.ts, types.ts, tests).
- Adopt development's fixes for files that broke tsc on our older base:
* packages/web-ui/src/components/calendar.tsx: react-day-picker v9
Chevron API (replaces IconLeft/IconRight).
* packages/web-ui/src/components/resizable.tsx: react-resizable-panels
v4 rename (PanelGroup→Group, PanelResizeHandle→Separator).
* apps/guides/components/ui/chart.tsx: remove unused recharts-v3-broken
shadcn chart (matches dev, which moved it to packages/web-ui).
- Refactor trips routes to `openapiRoutes` pattern so TypeScript can infer deep schema types without hitting instantiation depth limits - Extract shared schemas (TripSchema, CreateTripRequestSchema) to packages/api/src/routes/trips/schemas.ts - Split monolithic trip.ts/list.ts into individual route files matching the catalog pattern (getTripsRoute, createTripRoute, getTripByIdRoute, updateTripRoute, deleteTripRoute) each exporting typed routeDefinition and RouteHandler-typed handler - Wire trips slice into AppType via rpcRoutes in packages/api/src/index.ts - Add type-proof tests covering trips RPC requests and responses in both packages/api-client/test/rpc-types.test.ts and apps/expo/test/rpc-client-proof.test.ts - Add vitest configs for api-client and expo RPC type test suites - Add test:api-client:types and test:expo:rpc-types root scripts https://claude.ai/code/session_01JtEqZjLSh3kkycRSrrF3f5
… AppType coverage Converts 22 files across 11 route domains (packs, auth, user, feed, ai, weather, upload, wildlife, trailConditions, seasonSuggestions, packTemplates) from the .openapi(route, handler) pattern to defineOpenAPIRoute + openapiRoutes([...] as const) so every domain is included in AppType and available to hc<AppType>() for end-to-end RPC type inference. Also wires all new domains into rpcRoutes in packages/api/src/index.ts. Routes with middleware side-effects (generatePacksRoute, generateFromOnlineContent) are kept as separate .route() mounts. Streaming/admin routes (chat, admin) are excluded from the typed slice. https://claude.ai/code/session_01JtEqZjLSh3kkycRSrrF3f5
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a parallel Hono RPC client infrastructure for Expo with token refresh and retry logic, while refactoring the API's route registration from per-endpoint Changes
Sequence DiagramsequenceDiagram
actor Client
participant ExpoStorage as Expo SQLite KV
participant RpcFetch as createRpcFetch()
participant Server as API Server
participant AtomState as Atom State<br/>(tokenAtom, etc.)
Client->>RpcFetch: fetch(request)
RpcFetch->>ExpoStorage: get accessToken
ExpoStorage-->>RpcFetch: token value
RpcFetch->>RpcFetch: Add Authorization header<br/>+ Accept: application/json
RpcFetch->>Server: POST with headers
alt Success (200-399)
Server-->>RpcFetch: Response OK
RpcFetch-->>Client: return response
else 401 Unauthorized
RpcFetch->>RpcFetch: check x-packrat-rpc-retry header
alt Already retried (header=true)
RpcFetch-->>Client: return 401 (prevent loop)
else First 401
alt Refresh already in progress
RpcFetch->>RpcFetch: enqueue request as Promise
else Start refresh
RpcFetch->>Server: POST /api/auth/refresh<br/>(refresh_token)
Server-->>RpcFetch: { success, accessToken, refreshToken }
RpcFetch->>ExpoStorage: store new tokens
ExpoStorage-->>RpcFetch: ack
RpcFetch->>AtomState: update tokenAtom,<br/>refreshTokenAtom
RpcFetch->>RpcFetch: retry original request<br/>with new token +<br/>x-packrat-rpc-retry: true
RpcFetch->>Server: retry request
Server-->>RpcFetch: Response (from retry)
RpcFetch->>RpcFetch: resolve/reject<br/>all queued requests
end
RpcFetch-->>Client: return response (fresh)
end
else Refresh failure
RpcFetch->>AtomState: set needsReauthAtom = true
RpcFetch->>RpcFetch: reject all queued requests
RpcFetch-->>Client: throw error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff spans 60+ files with heterogeneous changes: a new RPC client layer with token refresh & retry logic (requires careful validation of queue management and error state), widespread route refactoring across ~25 route files (repetitive pattern but needs spot-checks for consistency), new type utilities and schema definitions, and multiple configuration updates. Logic density is moderate-to-high in Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Lays the groundwork for Hono RPC-based typed clients by restructuring API route registration to support hc<AppType> inference, and introduces a new @packrat/api-client package plus Expo transport/client wiring to start consuming typed RPC.
Changes:
- Refactors many
packages/apiroute modules to export{route, handler}pairs and compose them viadefineOpenAPIRoute(...)+openapiRoutes(...)for RPC type inference. - Adds new
@packrat/api-clientpackage with Vitest typecheck coverage and integrates it into Expo (plus a fetch transport with token refresh queueing). - Updates
@packrat/web-uicomponents forreact-day-picker@9andreact-resizable-panels@4.
Reviewed changes
Copilot reviewed 60 out of 62 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds TS path aliases for @packrat/api and new @packrat/api-client. |
| packages/web-ui/src/components/resizable.tsx | Updates component bindings for react-resizable-panels v4 naming. |
| packages/web-ui/src/components/calendar.tsx | Updates DayPicker icon component usage for react-day-picker v9. |
| packages/api/src/services/weatherService.ts | Adds a typed response shape + safer weather condition extraction. |
| packages/api/src/schemas/guides.ts | Introduces reusable positive integer query parsing (string → number). |
| packages/api/src/schemas/catalog.ts | Introduces reusable query param parsers for pagination + bounds. |
| packages/api/src/routes/wildlife/index.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/weather.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/user/index.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/upload.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/trips/createTripRoute.ts | Adds new trips “create” route module (exported route/handler). |
| packages/api/src/routes/trips/updateTripRoute.ts | Adds new trips “update” route module (exported route/handler). |
| packages/api/src/routes/trips/getTripByIdRoute.ts | Adds new trips “get by id” route module (exported route/handler). |
| packages/api/src/routes/trips/getTripsRoute.ts | Adds new trips “list” route module (exported route/handler). |
| packages/api/src/routes/trips/deleteTripRoute.ts | Adds new trips “delete” route module (exported route/handler). |
| packages/api/src/routes/trips/schemas.ts | Adds shared Trips zod schemas for OpenAPI + validation. |
| packages/api/src/routes/trips/index.ts | Switches trips routing to openapiRoutes([...]) composition. |
| packages/api/src/routes/trailConditions/reports.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/trailConditions/index.ts | Mounts trail-conditions via openapiRoutes entries. |
| packages/api/src/routes/seasonSuggestions.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/packs/analyzeImage.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/packs/items.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/packs/list.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/packs/pack.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/packs/index.ts | Composes pack OpenAPI entries and keeps legacy route mounted for generate. |
| packages/api/src/routes/packTemplates/packTemplates.ts | Converts pack-template routes to exported route/handler functions. |
| packages/api/src/routes/packTemplates/packTemplateItems.ts | Converts pack-template-item routes to exported route/handler functions. |
| packages/api/src/routes/packTemplates/index.ts | Composes pack-templates via openapiRoutes, keeps generator route as-is. |
| packages/api/src/routes/knowledgeBase/reader.ts | Converts reader route to exported route/handler + openapiRoutes. |
| packages/api/src/routes/knowledgeBase/index.ts | Simplifies mounting of reader routes. |
| packages/api/src/routes/index.ts | Refactors public/protected route mounting using $() wrappers. |
| packages/api/src/routes/guides/index.ts | Composes guides endpoints via defineOpenAPIRoute list. |
| packages/api/src/routes/feed/posts.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/feed/comments.ts | Converts to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/feed/index.ts | Composes feed endpoints via defineOpenAPIRoute list. |
| packages/api/src/routes/catalog/getCatalogItemsCategoriesRoute.ts | Converts categories endpoint to exported route/handler, updates query parsing. |
| packages/api/src/routes/catalog/index.ts | Composes catalog endpoints via openapiRoutes + keeps non-OpenAPI routes mounted. |
| packages/api/src/routes/auth/index.ts | Converts auth endpoints to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/ai/index.ts | Converts AI endpoints to exported route/handler + openapiRoutes composition. |
| packages/api/src/routes/admin/index.ts | Tightens JWT verification algorithm usage. |
| packages/api/src/index.ts | Adds rpcRoutes slice for AppType inference and exports it for clients. |
| packages/api/package.json | Adds exports metadata and switches key deps to Bun catalog: versions. |
| packages/api-client/package.json | Adds new internal api-client package. |
| packages/api-client/src/client.ts | Adds createApiClient() using hc<AppType>. |
| packages/api-client/src/types.ts | Adds shared ApiRequestOf/ApiResponseOf inference helpers. |
| packages/api-client/src/index.ts | Re-exports client and shared types. |
| packages/api-client/src/responses.ts | Adds shared ApiErrorResponse type. |
| packages/api-client/test/rpc-types.test.ts | Adds type-level tests to ensure inference remains stable. |
| packages/api-client/test/rpc-probe.ts | Adds probe file for RPC param-shape inference. |
| packages/api-client/tsconfig.json | Adds TS config for the new api-client package. |
| packages/api-client/tsconfig.probe.json | Adds probe tsconfig for focused typechecking. |
| packages/api-client/vitest.config.ts | Adds typecheck-only vitest config with aliases. |
| package.json | Adds scripts for api-client type tests and expo rpc type tests; adds catalog deps. |
| bun.lock | Adds workspace entry for api-client and updates catalog versions (Hono + Hono OpenAPI). |
| apps/guides/components/ui/chart.tsx | Removes the chart UI module. |
| apps/expo/package.json | Adds @packrat/api-client workspace dependency. |
| apps/expo/tsconfig.json | Aligns Expo tsconfig with repo tsconfig + adds RPC path aliases. |
| apps/expo/vitest.types.config.ts | Adds Expo typecheck vitest config for RPC typing. |
| apps/expo/test/rpc-client-proof.test.ts | Adds type-level proof that Expo can consume inferred RPC types. |
| apps/expo/lib/api/rpcTransport.ts | Adds fetch transport with auth headers + refresh queueing for RPC. |
| apps/expo/lib/api/rpcClient.ts | Adds shared Expo RPC client instance using the transport. |
| apps/expo/features/auth/README.md | Documents RPC transport coexistence with axios client during migration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+43
to
+45
| const where = includePublic | ||
| ? and(eq(trips.deleted, false)) | ||
| : and(eq(trips.userId, auth.userId), eq(trips.deleted, false)); |
There was a problem hiding this comment.
includePublic currently removes the userId filter entirely (where becomes only deleted = false). Since the trips table has no isPublic/visibility flag, this will return all trips from all users when includePublic=1, which is a data exposure issue. Either remove includePublic for trips, or introduce/verify a visibility column and filter to only those records (while still including the authenticated user's trips).
Comment on lines
+18
to
+30
| responses: { | ||
| 200: { | ||
| description: 'Trip deleted successfully', | ||
| content: { 'application/json': { schema: z.object({ success: z.boolean() }) } }, | ||
| }, | ||
| 403: { | ||
| description: 'Forbidden - user not authorized to delete this trip', | ||
| content: { 'application/json': { schema: ErrorResponseSchema } }, | ||
| }, | ||
| 404: { | ||
| description: 'Trip not found', | ||
| content: { 'application/json': { schema: ErrorResponseSchema } }, | ||
| }, |
There was a problem hiding this comment.
The OpenAPI responses declare both 403 and 404, but the handler never returns 404. When the trip doesn't exist (or isn't owned by the user), it returns 403 with "Trip not found or unauthorized". Please align the implementation and the documented responses (e.g., return 404 for missing trips, 403 for unauthorized, or remove the 404 response if you intentionally don't distinguish).
Comment on lines
+56
to
+72
| const processQueue = async ( | ||
| { error, token }: { error: Error | null; token: string | null }, | ||
| fetchImpl: FetchLike, | ||
| ) => { | ||
| const pending = failedQueue; | ||
| failedQueue = []; | ||
|
|
||
| for (const request of pending) { | ||
| if (error) { | ||
| request.reject(error); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| const retryInit = await withAuthHeaders(request.init, token); | ||
| request.resolve(fetchImpl(request.input, retryInit)); | ||
| } catch (retryError) { |
There was a problem hiding this comment.
Queued 401 retries executed via processQueue() don't set the x-packrat-rpc-retry marker header. If a queued request is retried after a refresh and still gets a 401 (e.g., refresh succeeded but the endpoint still rejects), it can trigger another refresh cycle and potentially loop. Consider setting the retry marker on queued retries as well (and/or limiting refresh attempts per request).
Comment on lines
54
to
56
| .replace(/\t/g, ' ') // Replace tabs with space | ||
| .replace(/\u00a0/g, ' ') // Replace non-breaking spaces | ||
| .replace(/ /g, ' ') // Replace non-breaking spaces | ||
| .replace(/^\s+|\s+$/g, '') // Trim |
There was a problem hiding this comment.
This regex uses a literal non‑breaking space character (/ /g), which is hard to see/review and easy to accidentally alter. Prefer using the explicit escape (/\u00a0/g) (or a named constant) to keep the intent clear and avoid invisible character issues in diffs/editors.
Comment on lines
+33
to
+40
| export const CreateTripRequestSchema = z.object({ | ||
| id: z.string().openapi({ example: 't_123456', description: 'Client-generated trip ID' }), | ||
| name: z.string().min(1), | ||
| description: z.string().nullable().optional(), | ||
| location: LocationSchema, | ||
| startDate: z.string().optional().nullable(), | ||
| endDate: z.string().optional().nullable(), | ||
| notes: z.string().optional().nullable(), |
There was a problem hiding this comment.
TripSchema requires startDate/endDate to be ISO datetimes, but CreateTripRequestSchema accepts any string for these fields. Since the handlers do new Date(data.startDate)/new Date(data.endDate), invalid strings can become Invalid Date and cause runtime/DB errors. Consider validating these as z.string().datetime() (and still .optional().nullable() as needed) to match the response schema and prevent bad input.
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
packages/web-ui/src/components/resizable.tsx (1)
12-39:⚠️ Potential issue | 🔴 CriticalSeparator uses stale v3 attribute selector; Group's orientation styling has no v4 CSS equivalent.
The
data-[panel-group-direction=vertical]selectors on lines 13 and 29 are v3-only and will not apply in v4. However, the fix differs between components:Separator (line 29): The Separator component exposes
aria-orientationin v4, so replacedata-[panel-group-direction=vertical]:*witharia-[orientation=vertical]:*:Separator fix
- 'relative flex w-px items-center justify-center bg-border after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring focus-visible:ring-offset-1 data-[panel-group-direction=vertical]:h-px data-[panel-group-direction=vertical]:w-full data-[panel-group-direction=vertical]:after:left-0 data-[panel-group-direction=vertical]:after:h-1 data-[panel-group-direction=vertical]:after:w-full data-[panel-group-direction=vertical]:after:-translate-y-1/2 data-[panel-group-direction=vertical]:after:translate-x-0 [&[data-panel-group-direction=vertical]>div]:rotate-90', + 'relative flex w-px items-center justify-center bg-border after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring focus-visible:ring-offset-1 aria-[orientation=vertical]:h-px aria-[orientation=vertical]:w-full aria-[orientation=vertical]:after:left-0 aria-[orientation=vertical]:after:h-1 aria-[orientation=vertical]:after:w-full aria-[orientation=vertical]:after:-translate-y-1/2 aria-[orientation=vertical]:after:translate-x-0 [&[aria-orientation=vertical]>div]:rotate-90',Group (line 13): The Group component in v4 does not expose orientation via data or ARIA attributes — it only provides
data-group,data-testid, andid. Thedata-[panel-group-direction=vertical]:flex-colselector has no direct v4 equivalent. Consider using inline styles driven by theorientationprop, or wrapping the Group with a context/hook to apply flex-col conditionally. Do not attempt to usearia-[orientation=vertical]on the Group itself, as it will not match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-ui/src/components/resizable.tsx` around lines 12 - 39, The Group is using the v3 data attribute selector data-[panel-group-direction=vertical] which no longer exists in v4, and the Separator is using v3 selectors that should be replaced with the v4 aria orientation selector; update ResizablePrimitive.Separator usage inside the ResizableHandle component to replace all data-[panel-group-direction=vertical]:* selectors with aria-[orientation=vertical]:* so the separator styling responds to aria-orientation, and change the ResizablePrimitive.Group rendering (the component that applies "data-[panel-group-direction=vertical]:flex-col" in its className) to conditionally add the "flex-col" class based on the orientation prop (e.g., use the Group's orientation prop or wrap it so if orientation === 'vertical' include 'flex-col' in className) instead of relying on a data attribute; target the ResizablePrimitive.Group and ResizableHandle (the function using ResizablePrimitive.Separator) identifiers when making these edits.packages/api/src/routes/knowledgeBase/reader.ts (1)
88-100:⚠️ Potential issue | 🟠 MajorBug: handler bypasses Zod validation — use
c.req.valid('json').
extractContentRoutedeclares a JSON body schema withurl: z.string().url(), but the handler reads the body viaawait c.req.json(), which skips validation and typesurlasany. A client posting{ url: 42 }or{}will reachfetch(url)directly, producing a confusing runtime failure instead of the expected 400 response — and this also undermines the typed RPC client the PR introduces.🐛 Proposed fix
- console.log('[extract] Request received'); - const { url } = await c.req.json(); + console.log('[extract] Request received'); + const { url } = c.req.valid('json'); console.log(`[extract] URL to fetch: ${url}`);Additionally, consider validating that
urlis not an internal/loopback address before callingfetch— this endpoint is an SSRF surface by nature, since it fetches an arbitrary user-provided URL from the server.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/knowledgeBase/reader.ts` around lines 88 - 100, The handler extractContentHandler is bypassing Zod validation by calling await c.req.json() — replace that with const { url } = c.req.valid('json') (or equivalent validated payload retrieval) so the extractContentRoute's z.string().url() schema is enforced and types are correct; after obtaining the validated url, perform an explicit SSRF check (reject or sanitize loopback/internal IPs/hostnames) before calling fetch(url), and ensure you return a 400 on validation failures rather than letting fetch throw.packages/api/src/routes/packTemplates/packTemplates.ts (1)
132-138:⚠️ Potential issue | 🟡 Minor
templateWithItemsmay be undefined — response can violate schema.
findFirstreturnsT | undefined. If the template insert succeeds but the immediate read-back somehow misses (replication lag, race with deletion),c.json(undefined, 201)shipsnull/empty body, and the response won't satisfyPackTemplateWithItemsSchema.assertDefinedthe fetched row, or construct the response fromnewTemplate+ emptyitems: [].🔧 Proposed change
- // Query the created template with its items to match the response schema - const templateWithItems = await db.query.packTemplates.findFirst({ - where: eq(packTemplates.id, newTemplate.id), - with: { items: true }, - }); - - return c.json(templateWithItems, 201); + return c.json({ ...newTemplate, items: [] }, 201);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/packTemplates/packTemplates.ts` around lines 132 - 138, The result of db.query.packTemplates.findFirst (templateWithItems) can be undefined which would violate PackTemplateWithItemsSchema when returned; update the handler that calls db.query.packTemplates.findFirst to either assertDefined(templateWithItems) before returning or, if undefined, construct a response object using newTemplate plus items: [] (and any other expected fields) and return that with status 201; reference templateWithItems, db.query.packTemplates.findFirst, newTemplate and PackTemplateWithItemsSchema to locate the code and ensure the returned payload always matches the schema.packages/api/src/routes/packTemplates/packTemplateItems.ts (2)
87-90:⚠️ Potential issue | 🟡 MinorAdd soft-delete filter to the list endpoint for template items.
The query returns all items without filtering on the
deletedcolumn. Since the schema includes adeletedcolumn and the update handler supports soft deletes (line 305), this endpoint should exclude soft-deleted rows to comply with the soft-delete implementation requirement.Add
.and(eq(packTemplateItems.deleted, false))to the WHERE clause:Suggested fix
const items = await db .select() .from(packTemplateItems) .where(and( eq(packTemplateItems.packTemplateId, templateId), eq(packTemplateItems.deleted, false) ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/packTemplates/packTemplateItems.ts` around lines 87 - 90, The list query for pack template items currently returns all rows including soft-deleted ones; update the WHERE clause on the db.select(...) from packTemplateItems (the query that uses templateId) to filter out soft-deleted rows by adding a condition for packTemplateItems.deleted === false (e.g. combine eq(packTemplateItems.packTemplateId, templateId) with eq(packTemplateItems.deleted, false) using and(...) inside the .where call) so only non-deleted items are returned.
163-205:⚠️ Potential issue | 🟠 MajorHandler bypasses Zod validation by reading raw JSON instead of using valid().
addItemHandlerdeclaresCreatePackTemplateItemRequestSchemain the route body, but callsawait c.req.json()(line 168) instead ofc.req.valid('json'). This skips schema validation at runtime and loses type-safe input inference. The same issue exists inupdateItemHandler(line 281), which additionally usesc.req.param('itemId')instead ofc.req.valid('param')(line 280).Use
c.req.valid('json')andc.req.valid('param')to enforce validation and preserve type safety:♻️ Proposed changes
In
addItemHandler:const { templateId } = c.req.valid('param'); - const data = await c.req.json(); + const data = c.req.valid('json');In
updateItemHandler:- const itemId = c.req.param('itemId'); - const data = await c.req.json(); + const { itemId } = c.req.valid('param'); + const data = c.req.valid('json');Additionally,
getItemsHandlershould filter out deleted items to comply with the soft-delete requirement. Add.where(and(eq(packTemplateItems.packTemplateId, templateId), eq(packTemplateItems.deleted, false)))or similar soft-delete condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/packTemplates/packTemplateItems.ts` around lines 163 - 205, addItemHandler and updateItemHandler are bypassing runtime Zod validation by calling c.req.json() and c.req.param('itemId') directly; replace c.req.json() with c.req.valid('json') to get the validated CreatePackTemplateItemRequestSchema payload and replace c.req.param('itemId') with c.req.valid('param') to retrieve validated route params (use the same pattern used for templateId). Also update getItemsHandler to respect soft-deletes by adding a where clause that filters packTemplateItems.deleted === false (e.g., and(eq(packTemplateItems.packTemplateId, templateId), eq(packTemplateItems.deleted, false))) so deleted items are excluded. Ensure you reference packTemplateItems, packTemplates, addItemHandler, updateItemHandler and getItemsHandler when making the changes.packages/api/src/routes/packs/items.ts (4)
95-112:⚠️ Potential issue | 🟠 MajorStrip embeddings from read responses too.
These paths spread full
packItems/catalogItemrows into JSON, so embedding vectors can leak even though create/update/similar explicitly avoid returning them.🛡️ Proposed direction
- const mappedItems = items.map((item) => ({ - ...item, + const mappedItems = items.map(({ embedding: _embedding, catalogItem, ...item }) => { + const { embedding: _catalogEmbedding, ...safeCatalogItem } = catalogItem ?? {}; + + return { + ...item, + catalogItem: catalogItem ? safeCatalogItem : null, consumable: item.consumable ?? false, worn: item.worn ?? false, deleted: item.deleted ?? false, createdAt: item.createdAt.toISOString(), updatedAt: item.updatedAt.toISOString(), - })); + }; + });Apply the same stripping in
getItemRouteHandlerbeforereturn c.json(...).Also applies to: 184-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/packs/items.ts` around lines 95 - 112, The response currently spreads full packItems and nested catalogItem rows (mappedItems) into JSON, leaking embedding vectors; before returning from getItemRouteHandler (and the similar block at lines 184-203), strip out embedding fields from each pack item and its catalogItem (e.g., delete or set catalogItem.embedding and item.embedding to undefined) while preserving other transformations (consumable/worn/deleted defaults and createdAt/updatedAt ISO strings), then return the sanitized objects via c.json.
495-551:⚠️ Potential issue | 🟠 MajorSoft-delete pack items instead of hard-deleting them.
Line 547 permanently removes user content, while the list path already preserves deleted owned items for sync.
♻️ Proposed fix
- description: 'Permanently remove an item from a pack', + description: 'Soft delete an item from a pack', ... - await db.delete(packItems).where(eq(packItems.id, itemId)); + await db + .update(packItems) + .set({ deleted: true, updatedAt: new Date() }) + .where(and(eq(packItems.id, itemId), eq(packItems.userId, auth.userId)));As per coding guidelines,
packages/api/src/**/*.ts: Implement soft deletes for all user content in the API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/packs/items.ts` around lines 495 - 551, deleteItemRouteHandler currently hard-deletes a pack item via db.delete(packItems).where(...); change this to perform a soft-delete by updating the packItems row instead (e.g., set deletedAt = new Date() or isDeleted = true) and keep the rest of the flow (capture packId, update packs.updatedAt, return same response). Locate deleteItemRouteHandler and replace the db.delete(packItems).where(eq(packItems.id, itemId)) call with a db.update(packItems).set({...}) where you set the chosen soft-delete field, ensuring the initial findFirst query still filters by owner (and later codebase will need to respect the soft-delete field).
259-312:⚠️ Potential issue | 🔴 CriticalAuthorize the pack before generating embeddings or inserting.
Line 291 inserts an item into whatever
packIdthe caller supplies. A user can add items to another user’s pack, and the costly embedding call runs before any pack ownership check.🔒 Proposed fix
if (!data.id) { return c.json({ error: 'Item ID is required' }, 400); } + const pack = await db.query.packs.findFirst({ + where: and(eq(packs.id, packId), eq(packs.userId, auth.userId)), + columns: { + id: true, + }, + }); + + if (!pack) { + return c.json({ error: 'Pack not found' }, 404); + } + // Generate embedding const embeddingText = getEmbeddingText(data);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/packs/items.ts` around lines 259 - 312, The handler addItemRouteHandler currently generates embeddings and inserts into packItems for any packId before checking authorization; move the ownership check to occur immediately after resolving packId and auth (before calling getEmbeddingText or generateEmbedding) by querying the packs table via createDb to verify the pack with id packId is owned/accessible by auth.userId (or otherwise authorized), return a 403/appropriate error if not authorized, and only then proceed to call getEmbeddingText/generateEmbedding and perform the db.insert into packItems and the packs update; use the existing symbols packs, packItems, createDb, generateEmbedding, getEmbeddingText and auth.userId to locate and implement the check.
447-481:⚠️ Potential issue | 🟠 MajorDelete the old R2 image only after a successful image change.
The current code deletes the old object before the DB update succeeds, and also deletes it when the submitted image equals the old image. That can leave the item pointing at a missing object.
🛠️ Proposed fix shape
- // Delete old image from R2 if we are changing image - if ('image' in data) { - let oldImage: string | null = null; - try { - const item = await db.query.packItems.findFirst({ - where: and(eq(packItems.id, itemId), eq(packItems.userId, auth.userId)), - }); - - oldImage = item?.image ?? null; - - // Nothing to delete if old image is null - if (oldImage) { - // Use R2 bucket binding for deletion - const { PACKRAT_BUCKET } = getEnv(c); - await PACKRAT_BUCKET.delete(oldImage); - } - } catch (error) { - // Silently fail because this op shouldn't prevent the update - console.error('Error deleting old image from R2:', error); - const sentry = c.get('sentry'); - sentry.setTag('location', 'updateItemRoute/deleteOldImage'); - sentry.setContext('params', { - itemId, - oldImage, - userId: auth.userId, - }); - sentry.captureException(error); - } - } + const oldImage = existingItem.image ?? null; const [updatedItem] = await db .update(packItems) .set(updateData) .where(and(eq(packItems.id, itemId), eq(packItems.userId, auth.userId))) .returning(); + + if ('image' in data && oldImage && oldImage !== data.image) { + try { + const { PACKRAT_BUCKET } = getEnv(c); + await PACKRAT_BUCKET.delete(oldImage); + } catch (error) { + console.error('Error deleting old image from R2:', error); + const sentry = c.get('sentry'); + sentry.setTag('location', 'updateItemRoute/deleteOldImage'); + sentry.setContext('params', { itemId, oldImage, userId: auth.userId }); + sentry.captureException(error); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/packs/items.ts` around lines 447 - 481, The code currently deletes the old R2 object before the DB update and even when the new image equals the old one; change the flow so you first perform the DB update via db.update(packItems).set(updateData).where(...).returning() (use the existing update call to obtain the updated item), then only if ('image' in data) and the returned updatedItem.image differs from the previously fetched oldImage perform the R2 deletion; move the PACKRAT_BUCKET.delete call to after the update, wrap that deletion in a try/catch, and preserve the existing error logging and Sentry tags/contexts (location 'updateItemRoute/deleteOldImage' and params itemId, oldImage, userId) so deletion failures do not block the update.packages/api/src/routes/auth/index.ts (6)
775-818:⚠️ Potential issue | 🟠 MajorMake refresh-token rotation atomic.
Two concurrent refresh requests can both read the same unrevoked token before Line 805 revokes it, then each mint a valid replacement.
🔒 Proposed fix shape
- // Find the refresh token in the database - const tokenRecord = await db - .select({ - id: refreshTokens.id, - userId: refreshTokens.userId, - expiresAt: refreshTokens.expiresAt, - }) - .from(refreshTokens) - .where(and(eq(refreshTokens.token, refreshToken), isNull(refreshTokens.revokedAt))) - .limit(1); - - if (tokenRecord.length === 0) { - return c.json({ error: 'Invalid refresh token' }, 401); - } - - const token = tokenRecord[0]; - if (!token) { - return c.json({ error: 'Invalid refresh token' }, 401); - } - assertDefined(token); - - // Check if token is expired - if (new Date() > token.expiresAt) { - return c.json({ error: 'Refresh token expired' }, 401); - } - // Generate new refresh token const newRefreshToken = generateRefreshToken(); + const now = new Date(); - // Revoke old refresh token and create new one - await db + const [token] = await db .update(refreshTokens) .set({ - revokedAt: new Date(), + revokedAt: now, replacedByToken: newRefreshToken, }) - .where(eq(refreshTokens.id, token.id)); + .where( + and( + eq(refreshTokens.token, refreshToken), + isNull(refreshTokens.revokedAt), + gt(refreshTokens.expiresAt, now), + ), + ) + .returning({ + id: refreshTokens.id, + userId: refreshTokens.userId, + }); + + if (!token) { + return c.json({ error: 'Invalid or expired refresh token' }, 401); + } // Store new refresh token
1037-1058:⚠️ Potential issue | 🟠 MajorDon’t hard-delete user content during account deletion.
This permanently deletes packs and pack templates instead of marking user content deleted, and the multi-step sequence is not protected from partial failure.
As per coding guidelines,
packages/api/src/**/*.ts: Implement soft deletes for all user content in the API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/auth/index.ts` around lines 1037 - 1058, The code is hard-deleting user content; replace those db.delete calls with soft-delete updates and run them inside a single transaction to avoid partial failure: in the account deletion flow switch the delete calls for packTemplateItems, packTemplates, packs and users to update statements that set a soft-delete marker (e.g. deletedAt or isDeleted) on packTemplateItems, packTemplates, packs and the users row (use the existing column name if present or add one), keep/handle refreshTokens/authProviders/oneTimePasswords according to policy, and wrap the sequence in a db.transaction (or the project’s transactional helper) so all updates are atomic. Use the same table identifiers from the diff (refreshTokens, oneTimePasswords, authProviders, packTemplateItems, packTemplates, packs, users) so the changes are localized to the same code paths and ensure downstream queries respect the soft-delete flag.
129-131:⚠️ Potential issue | 🟠 MajorHandle null password hashes for social auth users.
Social authentication paths create users without passwords, setting
passwordHashtonull. The non-null assertion (!) suppresses TypeScript but allowsverifyPassword(password, null)to execute at runtime, causing an unhandled error instead of returning "Invalid email or password."Proposed fix
+ if (!userRecord.passwordHash) { + return c.json({ error: 'Invalid email or password' }, 401); + } + // Verify password - // biome-ignore lint/style/noNonNullAssertion: at this point, password hash would definitely not be null - const isPasswordValid = await verifyPassword(password, userRecord.passwordHash!); + const isPasswordValid = await verifyPassword(password, userRecord.passwordHash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/auth/index.ts` around lines 129 - 131, The call to verifyPassword uses a non-null assertion on userRecord.passwordHash which can be null for social-auth users; change the flow in the auth handler so you first check if userRecord.passwordHash is falsy (e.g., null) and treat that as an "Invalid email or password" response instead of calling verifyPassword, and only call verifyPassword(password, userRecord.passwordHash) when passwordHash is present; update the branch that currently uses verifyPassword and userRecord.passwordHash! to perform this guard and return the same error path as a bad password.
1028-1036:⚠️ Potential issue | 🟠 MajorUse runtime conversion for
auth.userIdbefore database operations.
as numberis a type assertion that does not convert strings at runtime. IfuserIdfrom the JWT is a string, all subsequent delete operations will compare numeric columns against a string value, causing silent failures. See line 968 in the same file for the correct pattern.🛠️ Proposed fix
- const userId = auth.userId as number; + const userId = Number(auth.userId); + if (!Number.isInteger(userId)) { + return c.json({ error: 'Unauthorized' }, 401); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/auth/index.ts` around lines 1028 - 1036, The code uses a TypeScript assertion "auth.userId as number" which doesn't convert runtime strings to numbers; change to a runtime conversion and validation before DB calls: convert auth.userId using Number(...) or parseInt(...) into a local userId variable, check for NaN (or invalid/missing value) and return an Unauthorized/Bad Request if conversion fails, then pass that numeric userId into createDb/delete operations; locate this around the verifyJWT call and the userId usage in the auth route to implement the conversion and guard.
1099-1114:⚠️ Potential issue | 🔴 CriticalVerify and validate Apple identity token JWS signature before using token claims.
The
appleHandleronly base64-decodes the identity token payload without verifying the JWS signature or validating claims. This allows attackers to forge tokens by modifying the payload and encoding it without a valid signature. Apple's JWS tokens are ES256-signed and require server-side verification.Validate the token by:
- Fetching Apple's public keys from
https://appleid.apple.com/auth/keys- Verifying the JWS ES256 signature using the matching key
- Validating claims:
iss(must behttps://appleid.apple.com),aud(your Services ID),exp(not expired), andnonce(if used during authorization)This follows the same pattern as the
googleHandlerin this file, which correctly usesgoogleClient.verifyIdToken()to validate Google tokens before trusting their claims.Reference: https://developer.apple.com/documentation/signinwithapple/verifying-a-user
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/auth/index.ts` around lines 1099 - 1114, The appleHandler currently base64-decodes the identityToken payload without verifying the JWS signature or validating claims; replace that logic with full JWS verification: fetch Apple's public keys from https://appleid.apple.com/auth/keys, select the key that matches the token's kid/header, verify the ES256 signature of identityToken, then validate required claims (iss === "https://appleid.apple.com", aud === your Services ID, exp not expired, and nonce if you used one) before trusting sub/email/email_verified; follow the same pattern used by googleHandler/googleClient.verifyIdToken for verification flow and keep using createDb after successful verification.
1229-1239:⚠️ Potential issue | 🟡 MinorWrap
verifyIdTokenin try/catch to return the documented 400 response.The google-auth-library throws on invalid tokens. Without a local try/catch, malformed Google tokens become unhandled errors that return 500 instead of the documented 400 response for "Invalid Google token".
🛠️ Proposed fix
- // Verify Google ID token - const ticket = await googleClient.verifyIdToken({ - idToken, - audience: GOOGLE_CLIENT_ID, - }); + let ticket; + try { + ticket = await googleClient.verifyIdToken({ + idToken, + audience: GOOGLE_CLIENT_ID, + }); + } catch { + return c.json({ error: 'Invalid Google token' }, 400); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/auth/index.ts` around lines 1229 - 1239, Wrap the call to googleClient.verifyIdToken(...) in a try/catch so validation failures are caught and handled as a 400 response; specifically, surround the verifyIdToken call that produces ticket (and subsequent ticket.getPayload() -> payload) with a try block and on error call c.json({ error: 'Invalid Google token' }, 400) (same response path used when payload or payload.email/sub are missing) to prevent thrown errors from bubbling up as 500s.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/tsconfig.json`:
- Around line 7-13: The tsconfig "compilerOptions.paths" in apps/expo currently
overrides the parent and omits several shared aliases, causing TypeScript/IDE
resolution failures; update the "paths" object in apps/expo/tsconfig.json (the
"paths" entry shown in the diff) to include the full set of project aliases from
the root (not just `@packrat/api` and `@packrat/api-client`) — specifically add
mappings for `@packrat/guards`, `@packrat/ui/`*, `@packrat/analytics`, nativewindui/*
(and any other aliases defined in the parent) with paths relative to apps/expo
so the Expo project can resolve those imports during tsc and in editors.
In `@packages/api-client/package.json`:
- Around line 11-13: The package.json for the api-client package is missing a
workspace dependency for the API types used by src/client.ts (it imports AppType
from `@packrat/api`); add "@packrat/api" as a dependency in package.json (use a
workspace spec like "workspace:*" or the appropriate workspace version) so
isolated installs and typechecking can resolve AppType, then reinstall
dependencies / run your typecheck to verify resolution.
In `@packages/api-client/vitest.config.ts`:
- Around line 5-9: The Vite alias for '@packrat/api' in the resolve.alias block
maps to a file (../api/src/index.ts) which breaks subpath imports like
'@packrat/api/containers'; update the alias so '@packrat/api' points to the
package directory (the api/src directory) instead of the index file, and add a
corresponding wildcard alias (e.g., '@packrat/api/*' -> api/src/*) matching the
pattern used in packages/api/vitest.config.ts and tsconfig so subpath imports
resolve correctly.
In `@packages/api/src/index.ts`:
- Around line 6-19: The AppType (RPC type surface) does not mirror the actual
runtime route tree because adminRoutes and chatRoutes are mounted in
routes/index.ts but omitted from rpcRoutes; update the RPC/type assembly to
include adminRoutes and chatRoutes (or ensure AppType is built from the same
route tree used by routes/index.ts) by importing and adding adminRoutes and
chatRoutes to the rpcRoutes composition where rpcRoutes and AppType are defined
so the generated types match the served API; apply the same change to the other
rpcRoutes/AppType composition found around the other block referenced (lines
~68-82).
- Around line 107-110: Remove rpcRoutes from the runtime export list so it's
only used for type inference: keep the type export export type AppType = typeof
rpcRoutes as-is, but change the named export to export { AppContainer, app }
(remove rpcRoutes). Ensure rpcRoutes remains declared in this module for the
typeof expression and verify no other modules import the runtime rpcRoutes
symbol; adjust any external usages to import types only if needed.
In `@packages/api/src/routes/index.ts`:
- Around line 20-41: The three intermediate OpenAPIHono instances (publicRoutes,
protectedRoutes, and routes) are missing generics which breaks type inference
for authMiddleware (and its c.set('user', ...) calls) and the composed routers;
update each `new OpenAPIHono()` to include the same generics used by the
sub-routes — `<{ Bindings: Env; Variables: Variables }>` — so `publicRoutes`,
`protectedRoutes`, and the top-level `routes` preserve the correct Env/Variables
typing throughout composition.
In `@packages/api/src/routes/trips/createTripRoute.ts`:
- Around line 22-36: The OpenAPI responses for createTripRoute currently use 200
for successful creation; change the success response key from 200 to 201 and
update its description to "Trip created" (keeping the content schema
TripWithPackSchema), and also update the route handler (createTripRoute's
response/return) to send a 201 Created status when returning the created
TripWithPack object so it matches other create endpoints (e.g., createPostRoute,
addItemRoute) and the OpenAPI spec.
- Around line 66-72: Extend TripWithPackSchema to include the optional nullable
pack relation (use PackSchema.nullable().optional()) so the returned shape
matches when you query with { pack: true }, or alternatively always normalize
the response by adding pack: null when data.packId is falsy; update the logic
around trip creation/query failures (the branches returning on insert/query
failure that use newTrip and tripWithPack) to return HTTP 500 instead of 400 for
server-side errors; locate references to TripWithPackSchema, PackSchema,
newTrip, and tripWithPack in createTripRoute.ts to apply these changes and
ensure the response conforms to the schema.
In `@packages/api/src/routes/trips/deleteTripRoute.ts`:
- Around line 44-52: The code currently performs a hard delete using
db.delete(trips); change this to a soft-delete update: instead of deleting, call
db.update(trips) (or the ORM's update method) to set a deleted flag (e.g.,
deleted = true) and deletedAt timestamp, and constrain both the initial lookup
(trips.findFirst where ...) and the update where clause to only affect
non-deleted rows (e.g., and(eq(trips.id, tripId), eq(trips.userId, auth.userId),
isNullOrFalse(trips.deleted))). Update the logic around trips.findFirst and the
db operation that currently references db.delete(trips) so the route returns the
same success response but preserves the row with deleted metadata.
In `@packages/api/src/routes/trips/getTripByIdRoute.ts`:
- Around line 35-38: The query using db.query.trips.findFirst currently filters
only by tripId and auth.userId and can return soft-deleted rows; add the deleted
filter by including eq(trips.deleted, false) in the where clause (i.e., extend
the and(...) passed to db.query.trips.findFirst to also require
eq(trips.deleted, false)) so soft-deleted trips are excluded when fetching a
trip by id.
In `@packages/api/src/routes/trips/getTripsRoute.ts`:
- Around line 43-45: The current `where` construction removes the
authenticated-user filter when `includePublic` is true and thus returns all
users' non-deleted trips; restore the auth filter or add an explicit public
predicate. Update the `where` logic that builds the query (the `where` variable
using `includePublic`, `trips.deleted`, `trips.userId`, and `auth.userId`) so
that either (A) both branches always include `eq(trips.userId, auth.userId)`
(preserving privacy) or (B) the includePublic branch uses a real public
visibility predicate such as `eq(trips.isPublic, true)` combined with
`eq(trips.deleted, false)` instead of dropping `userId`. Ensure the final
`where` passed to the query enforces deletion and the intended visibility/user
constraints.
In `@packages/api/src/routes/trips/schemas.ts`:
- Line 31: TripWithPackSchema currently aliases TripSchema so it omits the
returned pack relation; update TripWithPackSchema to extend/merge TripSchema by
adding the pack property (e.g., TripWithPackSchema =
TripSchema.openapi('TripWithPack').extend({ pack: PackSchema }) or by merging
with a PackRelationSchema), or import/define the PackSchema used elsewhere and
add pack: PackSchema (or pack: PackSchema.nullable() if it can be null) so the
generated RPC client sees the pack field that getTripsRoute returns.
- Around line 38-39: The startDate and endDate Zod entries currently accept any
string, so invalid date strings reach the handlers; change their definitions to
validate date format by refining that the value is null/undefined or a parsable
date (e.g., use startDate: z.string().optional().nullable().refine(v => v ==
null || !isNaN(Date.parse(v)), { message: "Invalid date" }) and the same for
endDate) so that bad inputs produce 400 validation errors; optionally add a
transform (or separate transform schema) to convert the validated string to a
Date before it is passed to the create/update handlers.
In `@packages/api/src/routes/trips/updateTripRoute.ts`:
- Around line 61-68: The update and refetch currently only filter by trips.id
and trips.userId so soft-deleted rows can be modified; add a condition that
trips.deletedAt is null to both the db.update(...).set(updateData).where(...)
call and the db.query.trips.findFirst(...) where clause (use the same and(...)
predicate with eq(trips.id, tripId), eq(trips.userId, auth.userId) and
isNull/eq(trips.deletedAt, null) depending on your query builder) so
soft-deleted trips are neither updated nor returned.
---
Outside diff comments:
In `@packages/api/src/routes/auth/index.ts`:
- Around line 1037-1058: The code is hard-deleting user content; replace those
db.delete calls with soft-delete updates and run them inside a single
transaction to avoid partial failure: in the account deletion flow switch the
delete calls for packTemplateItems, packTemplates, packs and users to update
statements that set a soft-delete marker (e.g. deletedAt or isDeleted) on
packTemplateItems, packTemplates, packs and the users row (use the existing
column name if present or add one), keep/handle
refreshTokens/authProviders/oneTimePasswords according to policy, and wrap the
sequence in a db.transaction (or the project’s transactional helper) so all
updates are atomic. Use the same table identifiers from the diff (refreshTokens,
oneTimePasswords, authProviders, packTemplateItems, packTemplates, packs, users)
so the changes are localized to the same code paths and ensure downstream
queries respect the soft-delete flag.
- Around line 129-131: The call to verifyPassword uses a non-null assertion on
userRecord.passwordHash which can be null for social-auth users; change the flow
in the auth handler so you first check if userRecord.passwordHash is falsy
(e.g., null) and treat that as an "Invalid email or password" response instead
of calling verifyPassword, and only call verifyPassword(password,
userRecord.passwordHash) when passwordHash is present; update the branch that
currently uses verifyPassword and userRecord.passwordHash! to perform this guard
and return the same error path as a bad password.
- Around line 1028-1036: The code uses a TypeScript assertion "auth.userId as
number" which doesn't convert runtime strings to numbers; change to a runtime
conversion and validation before DB calls: convert auth.userId using Number(...)
or parseInt(...) into a local userId variable, check for NaN (or invalid/missing
value) and return an Unauthorized/Bad Request if conversion fails, then pass
that numeric userId into createDb/delete operations; locate this around the
verifyJWT call and the userId usage in the auth route to implement the
conversion and guard.
- Around line 1099-1114: The appleHandler currently base64-decodes the
identityToken payload without verifying the JWS signature or validating claims;
replace that logic with full JWS verification: fetch Apple's public keys from
https://appleid.apple.com/auth/keys, select the key that matches the token's
kid/header, verify the ES256 signature of identityToken, then validate required
claims (iss === "https://appleid.apple.com", aud === your Services ID, exp not
expired, and nonce if you used one) before trusting sub/email/email_verified;
follow the same pattern used by googleHandler/googleClient.verifyIdToken for
verification flow and keep using createDb after successful verification.
- Around line 1229-1239: Wrap the call to googleClient.verifyIdToken(...) in a
try/catch so validation failures are caught and handled as a 400 response;
specifically, surround the verifyIdToken call that produces ticket (and
subsequent ticket.getPayload() -> payload) with a try block and on error call
c.json({ error: 'Invalid Google token' }, 400) (same response path used when
payload or payload.email/sub are missing) to prevent thrown errors from bubbling
up as 500s.
In `@packages/api/src/routes/knowledgeBase/reader.ts`:
- Around line 88-100: The handler extractContentHandler is bypassing Zod
validation by calling await c.req.json() — replace that with const { url } =
c.req.valid('json') (or equivalent validated payload retrieval) so the
extractContentRoute's z.string().url() schema is enforced and types are correct;
after obtaining the validated url, perform an explicit SSRF check (reject or
sanitize loopback/internal IPs/hostnames) before calling fetch(url), and ensure
you return a 400 on validation failures rather than letting fetch throw.
In `@packages/api/src/routes/packs/items.ts`:
- Around line 95-112: The response currently spreads full packItems and nested
catalogItem rows (mappedItems) into JSON, leaking embedding vectors; before
returning from getItemRouteHandler (and the similar block at lines 184-203),
strip out embedding fields from each pack item and its catalogItem (e.g., delete
or set catalogItem.embedding and item.embedding to undefined) while preserving
other transformations (consumable/worn/deleted defaults and createdAt/updatedAt
ISO strings), then return the sanitized objects via c.json.
- Around line 495-551: deleteItemRouteHandler currently hard-deletes a pack item
via db.delete(packItems).where(...); change this to perform a soft-delete by
updating the packItems row instead (e.g., set deletedAt = new Date() or
isDeleted = true) and keep the rest of the flow (capture packId, update
packs.updatedAt, return same response). Locate deleteItemRouteHandler and
replace the db.delete(packItems).where(eq(packItems.id, itemId)) call with a
db.update(packItems).set({...}) where you set the chosen soft-delete field,
ensuring the initial findFirst query still filters by owner (and later codebase
will need to respect the soft-delete field).
- Around line 259-312: The handler addItemRouteHandler currently generates
embeddings and inserts into packItems for any packId before checking
authorization; move the ownership check to occur immediately after resolving
packId and auth (before calling getEmbeddingText or generateEmbedding) by
querying the packs table via createDb to verify the pack with id packId is
owned/accessible by auth.userId (or otherwise authorized), return a
403/appropriate error if not authorized, and only then proceed to call
getEmbeddingText/generateEmbedding and perform the db.insert into packItems and
the packs update; use the existing symbols packs, packItems, createDb,
generateEmbedding, getEmbeddingText and auth.userId to locate and implement the
check.
- Around line 447-481: The code currently deletes the old R2 object before the
DB update and even when the new image equals the old one; change the flow so you
first perform the DB update via
db.update(packItems).set(updateData).where(...).returning() (use the existing
update call to obtain the updated item), then only if ('image' in data) and the
returned updatedItem.image differs from the previously fetched oldImage perform
the R2 deletion; move the PACKRAT_BUCKET.delete call to after the update, wrap
that deletion in a try/catch, and preserve the existing error logging and Sentry
tags/contexts (location 'updateItemRoute/deleteOldImage' and params itemId,
oldImage, userId) so deletion failures do not block the update.
In `@packages/api/src/routes/packTemplates/packTemplateItems.ts`:
- Around line 87-90: The list query for pack template items currently returns
all rows including soft-deleted ones; update the WHERE clause on the
db.select(...) from packTemplateItems (the query that uses templateId) to filter
out soft-deleted rows by adding a condition for packTemplateItems.deleted ===
false (e.g. combine eq(packTemplateItems.packTemplateId, templateId) with
eq(packTemplateItems.deleted, false) using and(...) inside the .where call) so
only non-deleted items are returned.
- Around line 163-205: addItemHandler and updateItemHandler are bypassing
runtime Zod validation by calling c.req.json() and c.req.param('itemId')
directly; replace c.req.json() with c.req.valid('json') to get the validated
CreatePackTemplateItemRequestSchema payload and replace c.req.param('itemId')
with c.req.valid('param') to retrieve validated route params (use the same
pattern used for templateId). Also update getItemsHandler to respect
soft-deletes by adding a where clause that filters packTemplateItems.deleted ===
false (e.g., and(eq(packTemplateItems.packTemplateId, templateId),
eq(packTemplateItems.deleted, false))) so deleted items are excluded. Ensure you
reference packTemplateItems, packTemplates, addItemHandler, updateItemHandler
and getItemsHandler when making the changes.
In `@packages/api/src/routes/packTemplates/packTemplates.ts`:
- Around line 132-138: The result of db.query.packTemplates.findFirst
(templateWithItems) can be undefined which would violate
PackTemplateWithItemsSchema when returned; update the handler that calls
db.query.packTemplates.findFirst to either assertDefined(templateWithItems)
before returning or, if undefined, construct a response object using newTemplate
plus items: [] (and any other expected fields) and return that with status 201;
reference templateWithItems, db.query.packTemplates.findFirst, newTemplate and
PackTemplateWithItemsSchema to locate the code and ensure the returned payload
always matches the schema.
In `@packages/web-ui/src/components/resizable.tsx`:
- Around line 12-39: The Group is using the v3 data attribute selector
data-[panel-group-direction=vertical] which no longer exists in v4, and the
Separator is using v3 selectors that should be replaced with the v4 aria
orientation selector; update ResizablePrimitive.Separator usage inside the
ResizableHandle component to replace all data-[panel-group-direction=vertical]:*
selectors with aria-[orientation=vertical]:* so the separator styling responds
to aria-orientation, and change the ResizablePrimitive.Group rendering (the
component that applies "data-[panel-group-direction=vertical]:flex-col" in its
className) to conditionally add the "flex-col" class based on the orientation
prop (e.g., use the Group's orientation prop or wrap it so if orientation ===
'vertical' include 'flex-col' in className) instead of relying on a data
attribute; target the ResizablePrimitive.Group and ResizableHandle (the function
using ResizablePrimitive.Separator) identifiers when making these edits.
---
Nitpick comments:
In `@apps/expo/lib/api/rpcTransport.ts`:
- Around line 19-22: The module-level refresh state (isRefreshing and
failedQueue) must be moved into the createRpcFetch closure so each transport
instance gets its own lock/queue; update createRpcFetch to declare let
isRefreshing = false and let failedQueue: QueuedRequest[] = [] inside its body,
move or redefine processQueue inside that closure (or change processQueue to
accept the queue and the instance's fetchImpl) and ensure when draining
failedQueue/replaying requests you use the closure's fetchImpl and auth refresh
logic so queued 401 retries target the correct transport instance.
- Around line 90-98: The code unconditionally calls await response.json()
(variable response and const data) which will throw on non-JSON error bodies;
change the logic to first check response.ok (or wrap the JSON parse in
try/catch) before assuming JSON, only parse JSON when response.ok, and on error
attempt response.text() as a fallback and throw a new Error that includes
response.status and the textual body (or parsing error) instead of the generic
'Token refresh failed' so callers retain status/context while still validating
accessToken/refreshToken presence.
- Around line 110-163: The wrapper function rpcFetch currently uses
Object.assign(async (...) => { ... }, fetchImpl) which is unnecessary and can
leak fetchImpl properties and cause type confusion; replace the Object.assign
pattern by declaring rpcFetch as the async function itself (keeping the same
async signature and body) and apply the type assertion/satisfies FetchLike
directly to that function (remove copying properties from fetchImpl), ensuring
you still reference fetchImpl inside the closure for actual network calls and
preserve the existing error/retry logic.
- Around line 45-47: The hardcoded storage keys ('access_token' and
'refresh_token') in rpcTransport.ts should be replaced with shared constants
from the auth atoms to avoid drift; import the constants (or re-export them from
apps/expo/features/auth/atoms/authAtoms.ts) and use those instead of literal
strings where tokenOverride, Storage.getItem, and any refresh logic (the
token/refresh_token usage around headers.set and the refresh path) read/write
storage so the transport always uses the same keys as the auth atoms.
In `@packages/api-client/src/types.ts`:
- Around line 9-15: ApiEndpointFn is declared but not exported, making it harder
for consumers to reference when using ApiRequestOf<T> / ApiResponseOf<T>; export
the helper type by changing its declaration to export type ApiEndpointFn =
(...args: never[]) => Promise<unknown> (so callers can import ApiEndpointFn to
assert/mock endpoint signatures when using ApiRequestOf and ApiResponseOf).
In `@packages/api-client/test/rpc-probe.ts`:
- Around line 5-10: These bare member-access statements (client.api.catalog,
client.api.guides, client.api.catalog[':id'], client.api.guides[':id']) are
being flagged as unused expressions; replace the runtime probe statements with
type-level assertions instead (e.g., introduce type aliases or conditional/type
test helpers that reference the same symbols) so the compiler still validates
the inferred parameter key shapes without emitting side-effect-free expressions
at runtime; update probes that reference client.api.catalog, client.api.guides
and the index signatures (':id') to be purely type-only checks rather than
expression statements.
In `@packages/api/src/routes/admin/index.ts`:
- Line 60: Replace the inline call to verify(authHeader.slice(7), e.JWT_SECRET,
'HS256') with the shared verifyJWT utility to centralize algorithm handling;
import and call verifyJWT with the token (e.g., token = authHeader.slice(7)) and
the same secret (e.JWT_SECRET) instead of directly invoking verify, removing the
hard-coded 'HS256' and relying on verifyJWT to enforce the algorithm
consistently across the codebase.
In `@packages/api/src/routes/ai/index.ts`:
- Around line 44-45: The route is force-casting the service result to
RagSearchResponseSchema which masks type mismatches; update
AIService.searchPackratOutdoorGuidesRAG's signature to return
Promise<z.infer<typeof RagSearchResponseSchema>> (and adjust any internal return
points) so the route can return the value without the cast, then remove the `as
z.infer<typeof RagSearchResponseSchema>` in the handler where c.json is called;
reference the aiService.searchPackratOutdoorGuidesRAG method and
RagSearchResponseSchema when making these changes.
In `@packages/api/src/routes/catalog/getCatalogItemsCategoriesRoute.ts`:
- Around line 6-12: The categoryLimitQueryParam schema is a reusable strict
digit-string→number pipeline; extract this logic into a shared exported helper
(e.g., create and export a helper named strictPositiveIntQuery or
parseDigitStringInt) in a common schema/util module, then replace the local
categoryLimitQueryParam with an import of that helper and reuse it in other
routes like packs/list.ts to avoid duplication; ensure the exported helper
preserves the
.string().regex(/^\d+$/).transform(Number).pipe(z.number().int().positive())
behavior and update imports where categoryLimitQueryParam (or similar logic) is
currently defined.
In `@packages/api/src/routes/feed/comments.ts`:
- Around line 306-316: Export the array of OpenAPI route entries so other
modules can reuse them: replace the current export of commentsRoutes with an
export of commentsOpenApiRoutes (the const array defined with defineOpenAPIRoute
entries and typed as const). Update usages that currently reconstruct routes
(e.g., feed/index.ts) to import and spread commentsOpenApiRoutes (similar to
packRouteEntries in packs/pack.ts) instead of creating new
defineOpenAPIRoute(...) entries; remove the unused commentsRoutes instance if it
remains.
In `@packages/api/src/routes/knowledgeBase/reader.ts`:
- Line 55: The regex currently uses a literal non‑breaking space inside the
.replace call in reader.ts (the line with .replace(/ /g, ' ')) which can be lost
by editors/formatters; change that regex to use the Unicode escape form for
U+00A0 (e.g. \u00A0) so the pattern reliably matches NBSP characters across
editor/formatter round-trips and remains self-documenting.
In `@packages/api/src/routes/packs/list.ts`:
- Around line 104-142: Replace the untyped c.req.json() usage in
listPostRouteHandler with the validated request body via c.req.valid('json') so
the handler uses the CreatePackRequestSchema declared on listPostRoute and gains
proper typing; remove the redundant manual id existence check (the schema
requires id: z.string()), and then use the typed data variable when constructing
the insert payload (keep field names like localCreatedAt/localUpdatedAt
conversion as needed) so runtime validation and client-side InferRequestType
reflect the schema.
- Around line 40-59: The includePublicBool calculation is using a redundant
.valueOf() on Boolean(includePublic); update listGetRouteHandler to compute
includePublicBool more clearly by replacing Boolean(includePublic).valueOf()
with a direct comparison (e.g., includePublic === 1) or simply
Boolean(includePublic), so includePublicBool is a plain boolean; update any
references (where, items.with) to use the corrected includePublicBool.
In `@packages/api/src/routes/packs/pack.ts`:
- Around line 670-674: Remove the dead packRoutes construction: delete the const
packRoutes = new OpenAPIHono<{ Bindings: Env; Variables: Variables
}>().openapiRoutes(packRouteEntries); and export only packRouteEntries; also
remove the now-unused imports OpenAPIHono, Env, and Variables from the top of
the file so there are no unused symbols remaining (leave packRouteEntries export
intact for external consumers).
In `@packages/api/src/routes/packTemplates/packTemplateItems.ts`:
- Around line 282-283: The query uses an unnecessary and() wrapper around a
single condition; update the db.query.packTemplateItems.findFirst call to use
the condition directly by replacing and(eq(packTemplateItems.id, itemId)) with
eq(packTemplateItems.id, itemId) (referencing packTemplateItems and itemId) to
improve readability.
In `@packages/api/src/routes/packTemplates/packTemplates.ts`:
- Around line 299-318: Before calling
db.update(packTemplates)...set(updateData), first validate the request body with
c.req.valid('json') and then fetch the existing template by id using
db.query.packTemplates.findFirst({ where: eq(packTemplates.id, templateId) }) to
perform an explicit existence and ownership check; if no row is found return
404, if a row exists but auth.role !== 'ADMIN' and auth.userId !==
existing.userId return 403, otherwise proceed to update (use updateData and
packTemplates/templateId as before) and then return the updated record. Include
references to packTemplates, templateId, updateData, db.update,
db.query.packTemplates.findFirst, and c.req.valid('json') when making these
changes.
In `@packages/api/src/routes/seasonSuggestions.ts`:
- Around line 77-80: The handler currently reads raw JSON with await
c.req.json() which bypasses the OpenAPI-validated payload; replace that with the
validated request body provided by the route (e.g. use the validated payload on
the request context such as c.req.validated or the equivalent property your
createRoute/OpenAPIHono setup exposes) so that seasonSuggestionsHandler consumes
the type-safe payload defined by seasonSuggestionsRoute instead of raw JSON.
In `@packages/api/src/routes/trailConditions/reports.ts`:
- Around line 310-348: The handler uses untyped c.req.param('reportId'); replace
this with the typed, schema-validated params from the route by calling
c.req.valid('param') and extracting reportId (e.g., const { reportId } =
c.req.valid('param')) inside updateReportHandler (and mirror the same change in
deleteReportHandler) so you use the params declared on
updateReportRoute/deleteReportRoute and get proper typing/validation; ensure all
uses of reportId in the handler reference the extracted variable.
- Around line 38-52: The function toReportResponse currently accepts
Record<string, unknown> and force-casts to TrailConditionReportResponse, losing
compile-time checking; change it to accept a typed DB row by introducing a
ReportRow type like `type ReportRow = typeof trailConditionReports.$inferSelect`
and update the signature to `function toReportResponse(row: ReportRow):
TrailConditionReportResponse`, then return `{ ...row, createdAt:
row.createdAt.toISOString(), updatedAt: row.updatedAt.toISOString(),
localCreatedAt: row.localCreatedAt.toISOString(), localUpdatedAt:
row.localUpdatedAt.toISOString() }` without the `as` cast so TypeScript will
validate the spread and overrides against the actual DB row shape (referencing
toReportResponse, TrailConditionReportResponse, and
trailConditionReports.$inferSelect).
In `@packages/api/src/routes/trips/createTripRoute.ts`:
- Line 43: The runtime guard `if (!data.id)` in createTripRoute should be
removed and the non-empty constraint enforced in the request schema: update
CreateTripRequestSchema so the id is a non-empty string (e.g. z.string().min(1)
or z.string().nonempty()) so validation will reject empty ids, then delete the
manual `if (!data.id) return c.json({ error: 'Trip ID is required' }, 400);`
check in the route handler and rely on schema validation errors to produce the
proper 400 response.
In `@packages/api/src/routes/weather.ts`:
- Around line 59-99: The handler is reading raw query params with
c.req.query('q') instead of using the validated schema; update searchHandler
(and likewise searchByCoordHandler and forecastHandler) to use the validated
request object via c.req.valid('query') (i.e., extract the q/lat/lon params from
the validated WeatherSearchQuerySchema result) so inputs are Zod-validated and
properly typed, and adjust any subsequent references to query/lat/lon to use
those validated values and types.
In `@packages/api/src/schemas/catalog.ts`:
- Around line 3-19: The helper boundedIntegerQueryParam is misleading because it
enforces positive values via .min(1). Rename boundedIntegerQueryParam to
positiveBoundedIntegerQueryParam (or refactor both helpers by extracting a
shared base like parseIntegerQueryParam that accepts min/max) and update all
usages; also hoist these helpers from packages/api/src/schemas/catalog.ts (and
the duplicate in packages/api/src/schemas/guides.ts) into a shared schema module
so both files import the single implementation to avoid drift.
In `@packages/api/src/schemas/guides.ts`:
- Around line 144-151: Replace the inline z.coerce.number() definitions for page
and limit in GuideSearchQuerySchema with the shared helper
positiveIntegerQueryParam to ensure digit-only string coercion and consistent
OpenAPI examples; update imports to include positiveIntegerQueryParam if missing
and call positiveIntegerQueryParam({ default: 1, example: '1', description:
'Page number for pagination' }) for page and positiveIntegerQueryParam({
default: 20, example: '20', description: 'Number of results per page' }) for
limit.
In `@packages/web-ui/src/components/calendar.tsx`:
- Around line 11-17: The Chevron component currently only accepts an orientation
prop and ignores DayPicker v9 props (className, disabled, size) and several
class name keys use deprecated v8 names; update the Chevron signature to accept
and forward { orientation, className, disabled, size } (or a props rest) to the
rendered icon component so styling/disabled/size are applied, and replace all
deprecated class key names (caption, nav_button, table, head_row, head_cell,
row, cell, day_selected, day_today, day_outside, day_disabled, day_range_middle,
day_hidden) with their v9 equivalents per the DayPicker v9 docs; ensure the
component used in calendar.tsx (Chevron) and any theme/class mapping that
references those keys are updated to the v9 key names so no styling is lost.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
7
to
13
| "paths": { | ||
| "expo-app/*": ["./*"] | ||
| "expo-app/*": ["./*"], | ||
| "@packrat/api": ["../../packages/api/src/index.ts"], | ||
| "@packrat/api/*": ["../../packages/api/src/*"], | ||
| "@packrat/api-client": ["../../packages/api-client/src/index.ts"], | ||
| "@packrat/api-client/*": ["../../packages/api-client/src/*"] | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP "from ['\"](`@packrat/`(guards|ui|analytics|web-ui)|nativewindui)" apps/expo -g '!node_modules' | head -50Repository: PackRat-AI/PackRat
Length of output: 5993
🏁 Script executed:
cat -n /root/tsconfig.json | head -100Repository: PackRat-AI/PackRat
Length of output: 113
🏁 Script executed:
cat -n apps/expo/tsconfig.jsonRepository: PackRat-AI/PackRat
Length of output: 746
🏁 Script executed:
# Check if there are any other tsconfig files in apps that might be relevant
fd tsconfig.json apps/ --type fRepository: PackRat-AI/PackRat
Length of output: 163
🏁 Script executed:
git ls-files | grep "^tsconfig.json$"Repository: PackRat-AI/PackRat
Length of output: 75
🏁 Script executed:
cat -n tsconfig.jsonRepository: PackRat-AI/PackRat
Length of output: 1818
🏁 Script executed:
cd apps/expo && cat -n package.json | grep -A 5 -B 5 check-typesRepository: PackRat-AI/PackRat
Length of output: 873
🏁 Script executed:
# Also check the root package.json check-types script
cat -n package.json | grep -A 5 -B 5 check-typesRepository: PackRat-AI/PackRat
Length of output: 755
🏁 Script executed:
# Verify the learning context - let's see if these imports should use the path aliases per the learnings
# Check one file to see what's currently imported
cat apps/expo/features/wildlife/screens/SpeciesDetailScreen.tsxRepository: PackRat-AI/PackRat
Length of output: 6974
Child paths completely overrides parent — critical aliases are inaccessible.
When apps/expo/tsconfig.json defines compilerOptions.paths, TypeScript uses those paths exclusively and ignores the parent's aliases. After this change, the Expo app cannot resolve @packrat/guards, @packrat/ui/*, @packrat/analytics, or nativewindui/* — all of which are actively imported throughout the codebase (50+ confirmed uses).
This breaks tsc --noEmit when run from apps/expo/ and causes IDE type resolution failures despite the imports working at runtime.
Re-declare the full alias set here (relative to apps/expo):
♻️ Required additions
"paths": {
"expo-app/*": ["./*"],
"@packrat/api": ["../../packages/api/src/index.ts"],
"@packrat/api/*": ["../../packages/api/src/*"],
"@packrat/api-client": ["../../packages/api-client/src/index.ts"],
- "@packrat/api-client/*": ["../../packages/api-client/src/*"]
+ "@packrat/api-client/*": ["../../packages/api-client/src/*"],
+ "@packrat/guards": ["../../packages/guards/src"],
+ "@packrat/guards/*": ["../../packages/guards/src/*"],
+ "@packrat/ui/*": ["../../packages/ui/*"],
+ "@packrat/analytics": ["../../packages/analytics/src"],
+ "@packrat/analytics/*": ["../../packages/analytics/src/*"],
+ "nativewindui/*": ["./components/ui/*"]
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/tsconfig.json` around lines 7 - 13, The tsconfig
"compilerOptions.paths" in apps/expo currently overrides the parent and omits
several shared aliases, causing TypeScript/IDE resolution failures; update the
"paths" object in apps/expo/tsconfig.json (the "paths" entry shown in the diff)
to include the full set of project aliases from the root (not just `@packrat/api`
and `@packrat/api-client`) — specifically add mappings for `@packrat/guards`,
`@packrat/ui/`*, `@packrat/analytics`, nativewindui/* (and any other aliases defined
in the parent) with paths relative to apps/expo so the Expo project can resolve
those imports during tsc and in editors.
Comment on lines
+11
to
+13
| "dependencies": { | ||
| "hono": "catalog:" | ||
| } |
Contributor
There was a problem hiding this comment.
Declare the API package dependency.
src/client.ts imports AppType from @packrat/api, but this package only declares hono. Add the workspace dependency so isolated installs and typechecks can resolve the API type source.
📦 Proposed manifest fix
"dependencies": {
+ "@packrat/api": "workspace:*",
"hono": "catalog:"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api-client/package.json` around lines 11 - 13, The package.json for
the api-client package is missing a workspace dependency for the API types used
by src/client.ts (it imports AppType from `@packrat/api`); add "@packrat/api" as a
dependency in package.json (use a workspace spec like "workspace:*" or the
appropriate workspace version) so isolated installs and typechecking can resolve
AppType, then reinstall dependencies / run your typecheck to verify resolution.
Comment on lines
+5
to
+9
| resolve: { | ||
| alias: { | ||
| '@packrat/api': resolve(__dirname, '../api/src/index.ts'), | ||
| '@packrat/api-client': resolve(__dirname, 'src/index.ts'), | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the vitest.config.ts file
find . -name "vitest.config.ts" -o -name "vitest.config.js" | head -20Repository: PackRat-AI/PackRat
Length of output: 199
🏁 Script executed:
# Check the structure of packages/api-client directory
ls -la packages/api-client/ 2>/dev/null || echo "Directory not found"Repository: PackRat-AI/PackRat
Length of output: 532
🏁 Script executed:
# Examine the actual content of the vitest.config.ts file
cat -n packages/api-client/vitest.config.ts 2>/dev/null || echo "File not found"Repository: PackRat-AI/PackRat
Length of output: 651
🏁 Script executed:
# Check the `@packrat/api` package structure
ls -la packages/api/src/ 2>/dev/null | head -20 || echo "Directory not found"Repository: PackRat-AI/PackRat
Length of output: 757
🏁 Script executed:
# Search for imports using `@packrat/api/subpath` patterns
rg "@packrat/api/" --type ts --type tsx -A 2 -B 2 | head -50Repository: PackRat-AI/PackRat
Length of output: 89
🏁 Script executed:
# Check tsconfig.json for path alias definitions
cat -n tsconfig.json | head -50Repository: PackRat-AI/PackRat
Length of output: 1818
🏁 Script executed:
# Search for imports using `@packrat/api` with subpaths
rg "@packrat/api/" --type ts -B 2 -A 2 | head -100Repository: PackRat-AI/PackRat
Length of output: 7724
🏁 Script executed:
# Check the root tsconfig to understand alias intent better
cat -n packages/api/tsconfig.json 2>/dev/nullRepository: PackRat-AI/PackRat
Length of output: 359
🏁 Script executed:
# Verify what's exported from `@packrat/api/index.ts`
head -30 packages/api/src/index.tsRepository: PackRat-AI/PackRat
Length of output: 1759
🏁 Script executed:
# Check if there are other vitest configs to see if pattern is used elsewhere
for f in ./apps/expo/vitest.config.ts ./packages/analytics/vitest.config.ts ./packages/api/vitest.config.ts; do
echo "=== $f ==="
cat "$f" | grep -A 10 "resolve:" || echo "No resolve config"
doneRepository: PackRat-AI/PackRat
Length of output: 886
🏁 Script executed:
# Verify if api-client tests or code actually import `@packrat/api` subpaths
rg "@packrat/api/" packages/api-client/ --type tsRepository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
# Double-check the exact Vite string alias behavior by looking at packages/api config as reference
echo "=== packages/api vitest.config.ts (correct pattern) ==="
cat packages/api/vitest.config.ts | grep -A 5 "resolve:"Repository: PackRat-AI/PackRat
Length of output: 242
Fix Vite alias to map @packrat/api to a directory, not a file.
The alias maps @packrat/api to ../api/src/index.ts (a file), which causes Vite to fail when resolving subpath imports like @packrat/api/containers. With Rollup-style string aliasing, this resolves as an invalid path ../api/src/index.ts/containers. Match the pattern in packages/api/vitest.config.ts and the root tsconfig.json (which defines both @packrat/api and @packrat/api/*) by mapping to the directory instead.
Suggested fix
resolve: {
alias: {
- '@packrat/api': resolve(__dirname, '../api/src/index.ts'),
+ '@packrat/api': resolve(__dirname, '../api/src'),
'@packrat/api-client': resolve(__dirname, 'src/index.ts'),
},📝 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
| resolve: { | |
| alias: { | |
| '@packrat/api': resolve(__dirname, '../api/src/index.ts'), | |
| '@packrat/api-client': resolve(__dirname, 'src/index.ts'), | |
| }, | |
| resolve: { | |
| alias: { | |
| '@packrat/api': resolve(__dirname, '../api/src'), | |
| '@packrat/api-client': resolve(__dirname, 'src/index.ts'), | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api-client/vitest.config.ts` around lines 5 - 9, The Vite alias for
'@packrat/api' in the resolve.alias block maps to a file (../api/src/index.ts)
which breaks subpath imports like '@packrat/api/containers'; update the alias so
'@packrat/api' points to the package directory (the api/src directory) instead
of the index file, and add a corresponding wildcard alias (e.g.,
'@packrat/api/*' -> api/src/*) matching the pattern used in
packages/api/vitest.config.ts and tsconfig so subpath imports resolve correctly.
Comment on lines
+6
to
+19
| import { aiRoutes } from '@packrat/api/routes/ai'; | ||
| import { authRoutes } from '@packrat/api/routes/auth'; | ||
| import { catalogRoutes } from '@packrat/api/routes/catalog'; | ||
| import { feedRoutes } from '@packrat/api/routes/feed'; | ||
| import { guidesRoutes } from '@packrat/api/routes/guides'; | ||
| import { packsRoutes } from '@packrat/api/routes/packs'; | ||
| import { packTemplatesRoutes } from '@packrat/api/routes/packTemplates'; | ||
| import { seasonSuggestionsRoutes } from '@packrat/api/routes/seasonSuggestions'; | ||
| import { trailConditionsRoutes } from '@packrat/api/routes/trailConditions'; | ||
| import { tripsRoutes } from '@packrat/api/routes/trips'; | ||
| import { uploadRoutes } from '@packrat/api/routes/upload'; | ||
| import { userRoutes } from '@packrat/api/routes/user'; | ||
| import { weatherRoutes } from '@packrat/api/routes/weather'; | ||
| import { wildlifeRoutes } from '@packrat/api/routes/wildlife'; |
Contributor
There was a problem hiding this comment.
Mirror the runtime route tree in AppType.
routes/index.ts mounts adminRoutes and chatRoutes, but rpcRoutes omits them, so the generated RPC type surface won’t match the served API.
Suggested fix
+import { adminRoutes } from '@packrat/api/routes/admin';
import { aiRoutes } from '@packrat/api/routes/ai';
import { authRoutes } from '@packrat/api/routes/auth';
import { catalogRoutes } from '@packrat/api/routes/catalog';
+import { chatRoutes } from '@packrat/api/routes/chat';
import { feedRoutes } from '@packrat/api/routes/feed'; const rpcRoutes = new OpenAPIHono<{ Bindings: Env; Variables: Variables }>()
.route('/api/auth', authRoutes)
+ .route('/api/admin', adminRoutes)
.route('/api/catalog', catalogRoutes)
.route('/api/guides', guidesRoutes)
.route('/api/trips', tripsRoutes)
.route('/api/packs', packsRoutes)
.route('/api/feed', feedRoutes)
.route('/api/ai', aiRoutes)
+ .route('/api/chat', chatRoutes)
.route('/api/weather', weatherRoutes)Also applies to: 68-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/index.ts` around lines 6 - 19, The AppType (RPC type
surface) does not mirror the actual runtime route tree because adminRoutes and
chatRoutes are mounted in routes/index.ts but omitted from rpcRoutes; update the
RPC/type assembly to include adminRoutes and chatRoutes (or ensure AppType is
built from the same route tree used by routes/index.ts) by importing and adding
adminRoutes and chatRoutes to the rpcRoutes composition where rpcRoutes and
AppType are defined so the generated types match the served API; apply the same
change to the other rpcRoutes/AppType composition found around the other block
referenced (lines ~68-82).
Comment on lines
+107
to
+110
| export type AppType = typeof rpcRoutes; | ||
|
|
||
| // Export the AppContainer class for Cloudflare Container binding | ||
| export { AppContainer }; | ||
| export { AppContainer, app, rpcRoutes }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find runtime imports of `rpcRoutes`.
# Expected result: No value imports outside this file if it is only needed for AppType.
rg -nP -C3 "import\s*\{[^}]*\brpcRoutes\b|import\s+.*\brpcRoutes\b|\brpcRoutes\b" \
--type=ts \
-g '!packages/api/src/index.ts'Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
cat -n packages/api/src/index.ts | head -120 | tail -40Repository: PackRat-AI/PackRat
Length of output: 1415
🏁 Script executed:
head -85 packages/api/src/index.ts | cat -nRepository: PackRat-AI/PackRat
Length of output: 4201
Remove the runtime export of rpcRoutes — it's only needed for type inference.
The rpcRoutes router is unauthenticated and only used for typeof AppType. Exporting it at runtime creates an unnecessary attack surface and has no external imports in the codebase.
Suggested fix
export type AppType = typeof rpcRoutes;
// Export the AppContainer class for Cloudflare Container binding
-export { AppContainer, app, rpcRoutes };
+export { AppContainer, app };📝 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
| export type AppType = typeof rpcRoutes; | |
| // Export the AppContainer class for Cloudflare Container binding | |
| export { AppContainer }; | |
| export { AppContainer, app, rpcRoutes }; | |
| export type AppType = typeof rpcRoutes; | |
| // Export the AppContainer class for Cloudflare Container binding | |
| export { AppContainer, app }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/index.ts` around lines 107 - 110, Remove rpcRoutes from the
runtime export list so it's only used for type inference: keep the type export
export type AppType = typeof rpcRoutes as-is, but change the named export to
export { AppContainer, app } (remove rpcRoutes). Ensure rpcRoutes remains
declared in this module for the typeof expression and verify no other modules
import the runtime rpcRoutes symbol; adjust any external usages to import types
only if needed.
Comment on lines
+35
to
+38
| const trip = await db.query.trips.findFirst({ | ||
| where: and(eq(trips.id, tripId), eq(trips.userId, auth.userId)), | ||
| with: { pack: true }, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the trips schema and nearby trip routes for the soft-delete field name.
# Expected result: Identify whether trips uses deletedAt, isDeleted, archivedAt, or another marker.
rg -n -C4 "export const trips|deletedAt|isDeleted|archivedAt|soft" --type=tsRepository: PackRat-AI/PackRat
Length of output: 41306
🏁 Script executed:
sed -n '324,378p' packages/api/src/db/schema.tsRepository: PackRat-AI/PackRat
Length of output: 1758
🏁 Script executed:
rg -n "deletedAt|isDeleted" packages/api/src/routes/trips/ --type=ts -B2 -A2Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
rg -n "trips\.deleted|eq(trips.deleted)" packages/api/src/routes/trips/ --type=ts -B3 -A3Repository: PackRat-AI/PackRat
Length of output: 1290
🏁 Script executed:
cat -n packages/api/src/routes/trips/getTripByIdRoute.tsRepository: PackRat-AI/PackRat
Length of output: 1680
Filter out soft-deleted trips.
This lookup only checks id and userId, so soft-deleted trips can be returned. Other trip routes use eq(trips.deleted, false) to exclude deleted content; apply the same pattern here.
Suggested fix
const trip = await db.query.trips.findFirst({
- where: and(eq(trips.id, tripId), eq(trips.userId, auth.userId)),
+ where: and(
+ eq(trips.id, tripId),
+ eq(trips.userId, auth.userId),
+ eq(trips.deleted, false),
+ ),
with: { pack: true },
});📝 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
| const trip = await db.query.trips.findFirst({ | |
| where: and(eq(trips.id, tripId), eq(trips.userId, auth.userId)), | |
| with: { pack: true }, | |
| }); | |
| const trip = await db.query.trips.findFirst({ | |
| where: and( | |
| eq(trips.id, tripId), | |
| eq(trips.userId, auth.userId), | |
| eq(trips.deleted, false), | |
| ), | |
| with: { pack: true }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/routes/trips/getTripByIdRoute.ts` around lines 35 - 38, The
query using db.query.trips.findFirst currently filters only by tripId and
auth.userId and can return soft-deleted rows; add the deleted filter by
including eq(trips.deleted, false) in the where clause (i.e., extend the
and(...) passed to db.query.trips.findFirst to also require eq(trips.deleted,
false)) so soft-deleted trips are excluded when fetching a trip by id.
Comment on lines
+43
to
+45
| const where = includePublic | ||
| ? and(eq(trips.deleted, false)) | ||
| : and(eq(trips.userId, auth.userId), eq(trips.deleted, false)); |
Contributor
There was a problem hiding this comment.
Do not expose every user’s trips for includePublic=1.
The truthy branch drops the userId filter and returns all non-deleted trips. Unless there is an explicit public/visibility predicate, this leaks private trip data across users.
Keep the authenticated-user filter, or add a real public-trip condition before honoring includePublic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/routes/trips/getTripsRoute.ts` around lines 43 - 45, The
current `where` construction removes the authenticated-user filter when
`includePublic` is true and thus returns all users' non-deleted trips; restore
the auth filter or add an explicit public predicate. Update the `where` logic
that builds the query (the `where` variable using `includePublic`,
`trips.deleted`, `trips.userId`, and `auth.userId`) so that either (A) both
branches always include `eq(trips.userId, auth.userId)` (preserving privacy) or
(B) the includePublic branch uses a real public visibility predicate such as
`eq(trips.isPublic, true)` combined with `eq(trips.deleted, false)` instead of
dropping `userId`. Ensure the final `where` passed to the query enforces
deletion and the intended visibility/user constraints.
| }) | ||
| .openapi('Trip'); | ||
|
|
||
| export const TripWithPackSchema = TripSchema.openapi('TripWithPack'); |
Contributor
There was a problem hiding this comment.
Define the pack relation in TripWithPackSchema.
getTripsRoute.ts returns trips with with: { pack: true }, but this schema is identical to TripSchema, so generated RPC clients won’t see the pack field that the endpoint actually returns.
Extend this schema with the pack relation shape, or stop including pack in the list-trip query.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/routes/trips/schemas.ts` at line 31, TripWithPackSchema
currently aliases TripSchema so it omits the returned pack relation; update
TripWithPackSchema to extend/merge TripSchema by adding the pack property (e.g.,
TripWithPackSchema = TripSchema.openapi('TripWithPack').extend({ pack:
PackSchema }) or by merging with a PackRelationSchema), or import/define the
PackSchema used elsewhere and add pack: PackSchema (or pack:
PackSchema.nullable() if it can be null) so the generated RPC client sees the
pack field that getTripsRoute returns.
Comment on lines
+38
to
+39
| startDate: z.string().optional().nullable(), | ||
| endDate: z.string().optional().nullable(), |
Contributor
There was a problem hiding this comment.
Validate trip date strings before converting them to Date.
startDate and endDate accept any string, but create/update handlers convert them with new Date(...). Invalid strings should fail as 400 validation errors instead of reaching the DB path.
🛡️ Proposed fix
- startDate: z.string().optional().nullable(),
- endDate: z.string().optional().nullable(),
+ startDate: z.string().datetime().optional().nullable(),
+ endDate: z.string().datetime().optional().nullable(),📝 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
| startDate: z.string().optional().nullable(), | |
| endDate: z.string().optional().nullable(), | |
| startDate: z.string().datetime().optional().nullable(), | |
| endDate: z.string().datetime().optional().nullable(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/routes/trips/schemas.ts` around lines 38 - 39, The startDate
and endDate Zod entries currently accept any string, so invalid date strings
reach the handlers; change their definitions to validate date format by refining
that the value is null/undefined or a parsable date (e.g., use startDate:
z.string().optional().nullable().refine(v => v == null || !isNaN(Date.parse(v)),
{ message: "Invalid date" }) and the same for endDate) so that bad inputs
produce 400 validation errors; optionally add a transform (or separate transform
schema) to convert the validated string to a Date before it is passed to the
create/update handlers.
Comment on lines
+61
to
+68
| await db | ||
| .update(trips) | ||
| .set(updateData) | ||
| .where(and(eq(trips.id, tripId), eq(trips.userId, auth.userId))); | ||
|
|
||
| const updatedTrip: Trip | undefined = await db.query.trips.findFirst({ | ||
| where: and(eq(trips.id, tripId), eq(trips.userId, auth.userId)), | ||
| }); |
Contributor
There was a problem hiding this comment.
Prevent updates to soft-deleted trips.
The update and refetch only check id and userId, so a soft-deleted trip can still be mutated and returned.
🛡️ Proposed fix
await db
.update(trips)
.set(updateData)
- .where(and(eq(trips.id, tripId), eq(trips.userId, auth.userId)));
+ .where(and(eq(trips.id, tripId), eq(trips.userId, auth.userId), eq(trips.deleted, false)));
const updatedTrip: Trip | undefined = await db.query.trips.findFirst({
- where: and(eq(trips.id, tripId), eq(trips.userId, auth.userId)),
+ where: and(eq(trips.id, tripId), eq(trips.userId, auth.userId), eq(trips.deleted, false)),
});As per coding guidelines, “Implement soft deletes for all user content in the API”.
📝 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
| await db | |
| .update(trips) | |
| .set(updateData) | |
| .where(and(eq(trips.id, tripId), eq(trips.userId, auth.userId))); | |
| const updatedTrip: Trip | undefined = await db.query.trips.findFirst({ | |
| where: and(eq(trips.id, tripId), eq(trips.userId, auth.userId)), | |
| }); | |
| await db | |
| .update(trips) | |
| .set(updateData) | |
| .where(and(eq(trips.id, tripId), eq(trips.userId, auth.userId), eq(trips.deleted, false))); | |
| const updatedTrip: Trip | undefined = await db.query.trips.findFirst({ | |
| where: and(eq(trips.id, tripId), eq(trips.userId, auth.userId), eq(trips.deleted, false)), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/routes/trips/updateTripRoute.ts` around lines 61 - 68, The
update and refetch currently only filter by trips.id and trips.userId so
soft-deleted rows can be modified; add a condition that trips.deletedAt is null
to both the db.update(...).set(updateData).where(...) call and the
db.query.trips.findFirst(...) where clause (use the same and(...) predicate with
eq(trips.id, tripId), eq(trips.userId, auth.userId) and
isNull/eq(trips.deletedAt, null) depending on your query builder) so
soft-deleted trips are neither updated nor returned.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 62 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3
to
+11
| const positiveIntegerQueryParam = (defaultValue: string) => | ||
| z | ||
| .string() | ||
| .regex(/^\d+$/) | ||
| .optional() | ||
| .default(defaultValue) | ||
| .transform((value) => Number(value)) | ||
| .pipe(z.number().int().positive()); | ||
|
|
There was a problem hiding this comment.
positiveIntegerQueryParam duplicates the same helper that was also introduced in schemas/guides.ts. To avoid subtle drift (e.g., different regex/default/pipe semantics) consider extracting this into a shared schema utility (e.g., schemas/queryParams.ts) and importing it from both places.
Comment on lines
+3
to
+10
| const positiveIntegerQueryParam = (defaultValue: string) => | ||
| z | ||
| .string() | ||
| .regex(/^\d+$/) | ||
| .optional() | ||
| .default(defaultValue) | ||
| .transform((value) => Number(value)) | ||
| .pipe(z.number().int().positive()); |
There was a problem hiding this comment.
positiveIntegerQueryParam is also defined in schemas/catalog.ts with identical logic. Consider extracting it to a shared helper module so pagination param behavior stays consistent across endpoints (and changes only need to be made once).
Comment on lines
+43
to
+46
| const where = includePublic | ||
| ? and(eq(trips.deleted, false)) | ||
| : and(eq(trips.userId, auth.userId), eq(trips.deleted, false)); | ||
|
|
There was a problem hiding this comment.
includePublic currently drops the userId filter and returns all non-deleted trips (where: and(eq(trips.deleted, false))). The trips table doesn’t have an isPublic flag, so this will expose other users’ trips when includePublic=1. Either remove includePublic and always scope to auth.userId, or implement an explicit “public trips” concept (e.g., join against a visibility column / related entity) and filter accordingly.
Comment on lines
+29
to
+32
| .openapi('Trip'); | ||
|
|
||
| export const TripWithPackSchema = TripSchema.openapi('TripWithPack'); | ||
|
|
There was a problem hiding this comment.
TripWithPackSchema is currently just TripSchema with a different OpenAPI name, but the trips handlers are returning records with with: { pack: true } (i.e., an extra pack property). This makes the OpenAPI/InferResponseType contract inconsistent. Either extend the schema to include the pack shape, or stop eager-loading pack (and/or rename the schema to reflect what is actually returned).
Comment on lines
+35
to
+41
| const trip = await db.query.trips.findFirst({ | ||
| where: and(eq(trips.id, tripId), eq(trips.userId, auth.userId)), | ||
| with: { pack: true }, | ||
| }); | ||
|
|
||
| if (!trip) return c.json({ error: 'Trip not found' }, 404); | ||
| return c.json(trip, 200); |
There was a problem hiding this comment.
The route declares a 200 response schema of TripSchema, but the handler queries with: { pack: true } and returns the result directly, which will include an extra pack field. To keep the RPC/OpenAPI typing accurate, either return a schema that includes pack (e.g., TripWithPackSchema) or remove the eager pack relation from this endpoint.
Comment on lines
+43
to
+49
| try { | ||
| const trip = await db.query.trips.findFirst({ | ||
| where: and(eq(trips.id, tripId), eq(trips.userId, auth.userId)), | ||
| }); | ||
|
|
||
| if (!trip) return c.json({ error: 'Trip not found or unauthorized' }, 403); | ||
|
|
There was a problem hiding this comment.
The delete handler never returns a 404 even though the route definition documents a 404 response. Right now missing trips return 403 with message “Trip not found or unauthorized”, which also doesn’t match the documented 403 semantics. Consider returning 404 when the trip doesn’t exist, and 403 only when the user is authenticated but not allowed to delete an existing trip (or remove 404 from the OpenAPI if you intentionally want to avoid disclosing existence).
- Convert chat.ts to openapiRoutes pattern; add /api/chat to AppType - Convert admin JSON API endpoints (8 routes) to defineOpenAPIRoute, export adminRpcRoutes and AdminAppType for a separate typed admin client - Add Variables to adminRoutes generic to satisfy RouteHandler types - Delete orphaned trips/list.ts and trips/trip.ts (superseded by individual route files, not imported anywhere) AppType now covers every JSON/SSE domain. Only middleware-gated sub-routers (generatePacksRoute, generateFromOnlineContent, backfillEmbeddings, queueCatalogEtl) remain outside the typed slice. https://claude.ai/code/session_01JtEqZjLSh3kkycRSrrF3f5
Resolves conflicts: - ai/index.ts: use RagSearchResponseSchema.parse() (runtime validation) - admin/index.ts: preserve AdminAppType additions + analytics routes from dev - tsconfig.json: merge path aliases from both branches - package.json: include both test:expo:rpc-types and test:mcp scripts - apps/expo/package.json: include api-client, config, and env workspace deps - packages/api/package.json: add version field from dev - packages/api-client: merge hono RPC client (ours) + PackRatApiClient/MCP helpers (dev) into unified index.ts; take dev version number (2.0.21) - apps/expo/lib/api/rpcClient|rpcTransport: fix clientEnvs import path to @packrat/env/expo-client (env was moved from expo-app/env to @packrat/env) https://claude.ai/code/session_01JtEqZjLSh3kkycRSrrF3f5
Contributor
Deploying packrat-guides with
|
| Latest commit: |
fae9b16
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://88232038.packrat-guides-6gq.pages.dev |
| Branch Preview URL: | https://claude-complete-type-safety.packrat-guides-6gq.pages.dev |
andrew-bierman
had a problem deploying
to
copilot
GitHub Actions
Failure
Contributor
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
packrat-admin | fae9b16 | Apr 22 2026, 11:56 AM |
github-actions
Bot
added
documentation
Contributor
Deploying packrat-landing with
|
| Latest commit: |
fae9b16
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7f7c3362.packrat-landing.pages.dev |
| Branch Preview URL: | https://claude-complete-type-safety.packrat-landing.pages.dev |
Isthisanmol
approved these changes
Apr 22, 2026
Collaborator
Isthisanmol
left a comment
Isthisanmol
left a comment
There was a problem hiding this comment.
Everything worked fine for me on this branch.
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
bun.lock was stale after merging dev branch; api-client now lists hono as a catalog dependency which wasn't reflected in the lockfile. https://claude.ai/code/session_01JtEqZjLSh3kkycRSrrF3f5
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/api/src/routes/chat.ts (2)
356-372:⚠️ Potential issue | 🟠 MajorReturn 404 when no report row is updated.
The route documents
404, but the handler returns success even wheniddoes not match any report. Check the update result before returning200.🐛 Proposed fix
- await db + const updated = await db .update(reportedContent) .set({ status, reviewed: true, reviewedBy: auth.userId, reviewedAt: new Date(), }) - .where(eq(reportedContent.id, id)); + .where(eq(reportedContent.id, id)) + .returning({ id: reportedContent.id }); + + if (!updated.length) { + return c.json({ error: 'Report not found' }, 404); + } return c.json({ success: true }, 200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/chat.ts` around lines 356 - 372, The handler currently returns 200 even when no report row is updated; after calling db.update(reportedContent).set(...).where(eq(reportedContent.id, id)) capture the update result and check the affected-row count (e.g., result.rowCount or result.count depending on the DB client) and if zero return c.json({ error: 'Report not found' }, 404); otherwise continue returning success; locate this logic around parseIntegerId and the db.update call to implement the conditional response.
58-71:⚠️ Potential issue | 🟡 MinorPreserve the default date when the request omits
date.Line 70 overwrites the initialized default body, so
datebecomesundefinedfor clients that omit it and the prompt says the current date is undefined.🐛 Proposed fix
- body = await c.req.json(); + body = { ...body, ...(await c.req.json()) };Also applies to: 93-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/chat.ts` around lines 58 - 71, The request-parsed object is overwriting the initialized default body (so date becomes undefined); fix by merging the parsed payload into the existing default instead of replacing it: call const parsed = await c.req.json() and then do body = { ...body, ...parsed } (so any omitted fields like date stay as-initialized). Apply the same merge pattern where the code currently does body = await c.req.json() (also referenced around the destructuring of messages, contextType, itemId, packId, location, date) to preserve defaults.packages/api/src/routes/admin/index.ts (1)
1126-1285:⚠️ Potential issue | 🟠 MajorUse OpenAPI
{id}path parameter syntax increateRoutedefinitions.The
@hono/zod-openapilibrary requires OpenAPI-compliant curly brace syntax for path parameters. Using:idgenerates invalid OpenAPI schemas (e.g.,/users/:idinstead of/users/{id}). The established codebase pattern in trips, packs, catalogs, guides, and chat routes all correctly use{id}syntax. Update all four admin route paths:Locations to fix
- Line 1128 (
deleteUserRoute):/users/:id→/users/{id}- Line 1179 (
deletePackRoute):/packs/:id→/packs/{id}- Line 1223 (
deleteCatalogItemRoute):/catalog/:id→/catalog/{id}- Line 1280 (
updateCatalogItemRoute):/catalog/:id→/catalog/{id}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/admin/index.ts` around lines 1126 - 1285, The route path parameters use Express-style ":id" instead of OpenAPI curly-brace syntax, causing invalid OpenAPI output; update the path strings in deleteUserRoute, deletePackRoute, deleteCatalogItemRoute, and updateCatalogItemRoute to use "/users/{id}", "/packs/{id}", "/catalog/{id}" and "/catalog/{id}" respectively so createRoute produces OpenAPI-compliant paths (adjust the path values on the route definitions referenced by those exported symbols).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/lib/api/rpcTransport.ts`:
- Around line 124-159: The refresh lock is held through the retry, so new 401s
that enqueue while draining the queue can deadlock; after successfully obtaining
nextToken in the refresh block (after await refreshAccessToken(...)), clear the
lock (set isRefreshing = false) before calling processQueue and before retrying
the original request (the subsequent fetchImpl/withAuthHeaders call) so queued
requests can trigger a new refresh if needed; update the refresh branch around
refreshAccessToken, processQueue, and the retry logic (symbols: isRefreshing,
refreshAccessToken, processQueue, failedQueue, fetchImpl, withAuthHeaders) to
release the lock immediately after obtaining nextToken and before processing the
queue/retrying.
In `@packages/api/src/routes/admin/index.ts`:
- Around line 1340-1345: The file currently exports the runtime value
adminRpcRoutes (which lacks auth middleware) and a type AdminAppType; change the
export so only the type is exported and the runtime router is not re-exported:
stop exporting the adminRpcRoutes value and keep/export only AdminAppType and
adminRoutes as appropriate. Specifically, remove adminRpcRoutes from the value
export list and ensure AdminAppType (the type alias for typeof adminRpcRoutes)
remains exported; then update the package re-export in packages/api/src/index.ts
to re-export the AdminAppType type (not the runtime adminRpcRoutes) so
downstream consumers can use the type without obtaining the unprotected runtime
router.
---
Outside diff comments:
In `@packages/api/src/routes/admin/index.ts`:
- Around line 1126-1285: The route path parameters use Express-style ":id"
instead of OpenAPI curly-brace syntax, causing invalid OpenAPI output; update
the path strings in deleteUserRoute, deletePackRoute, deleteCatalogItemRoute,
and updateCatalogItemRoute to use "/users/{id}", "/packs/{id}", "/catalog/{id}"
and "/catalog/{id}" respectively so createRoute produces OpenAPI-compliant paths
(adjust the path values on the route definitions referenced by those exported
symbols).
In `@packages/api/src/routes/chat.ts`:
- Around line 356-372: The handler currently returns 200 even when no report row
is updated; after calling
db.update(reportedContent).set(...).where(eq(reportedContent.id, id)) capture
the update result and check the affected-row count (e.g., result.rowCount or
result.count depending on the DB client) and if zero return c.json({ error:
'Report not found' }, 404); otherwise continue returning success; locate this
logic around parseIntegerId and the db.update call to implement the conditional
response.
- Around line 58-71: The request-parsed object is overwriting the initialized
default body (so date becomes undefined); fix by merging the parsed payload into
the existing default instead of replacing it: call const parsed = await
c.req.json() and then do body = { ...body, ...parsed } (so any omitted fields
like date stay as-initialized). Apply the same merge pattern where the code
currently does body = await c.req.json() (also referenced around the
destructuring of messages, contextType, itemId, packId, location, date) to
preserve defaults.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16591f2a-80f8-4db8-9b92-7aee7b78b954
📒 Files selected for processing (15)
apps/expo/lib/api/rpcClient.tsapps/expo/lib/api/rpcTransport.tsapps/expo/package.jsonpackage.jsonpackages/api-client/package.jsonpackages/api-client/src/index.tspackages/api/package.jsonpackages/api/src/index.tspackages/api/src/routes/admin/index.tspackages/api/src/routes/ai/index.tspackages/api/src/routes/chat.tspackages/api/src/routes/index.tspackages/api/src/routes/trips/list.tspackages/api/src/routes/trips/trip.tstsconfig.json
💤 Files with no reviewable changes (2)
- packages/api/src/routes/trips/trip.ts
- packages/api/src/routes/trips/list.ts
✅ Files skipped from review due to trivial changes (3)
- apps/expo/package.json
- apps/expo/lib/api/rpcClient.ts
- packages/api/src/routes/index.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/api-client/package.json
- tsconfig.json
- package.json
- packages/api/package.json
- packages/api-client/src/index.ts
- packages/api/src/routes/ai/index.ts
Comment on lines
+124
to
+159
| if (isRefreshing) { | ||
| return await new Promise<Response>((resolve, reject) => { | ||
| failedQueue.push({ | ||
| input, | ||
| init: cloneInit(init), | ||
| resolve, | ||
| reject, | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| isRefreshing = true; | ||
|
|
||
| try { | ||
| const nextToken = await refreshAccessToken(fetchImpl, baseUrl); | ||
| await processQueue({ error: null, token: nextToken }, fetchImpl); | ||
|
|
||
| const retryHeaders = new Headers(init?.headers); | ||
| retryHeaders.set('x-packrat-rpc-retry', 'true'); | ||
|
|
||
| return await fetchImpl( | ||
| input, | ||
| await withAuthHeaders( | ||
| { | ||
| ...cloneInit(init), | ||
| headers: retryHeaders, | ||
| }, | ||
| nextToken, | ||
| ), | ||
| ); | ||
| } catch (error) { | ||
| await store.set(needsReauthAtom, true); | ||
| await processQueue({ error: error as Error, token: null }, fetchImpl); | ||
| throw error; | ||
| } finally { | ||
| isRefreshing = false; |
Contributor
There was a problem hiding this comment.
Release the refresh lock before retrying the original request.
Line 139 drains the queue while isRefreshing stays true until line 159. Any 401 that queues during the original retry can hang forever because no later processQueue() runs.
🐛 Proposed fix
- try {
- const nextToken = await refreshAccessToken(fetchImpl, baseUrl);
- await processQueue({ error: null, token: nextToken }, fetchImpl);
-
- const retryHeaders = new Headers(init?.headers);
- retryHeaders.set('x-packrat-rpc-retry', 'true');
-
- return await fetchImpl(
- input,
- await withAuthHeaders(
- {
- ...cloneInit(init),
- headers: retryHeaders,
- },
- nextToken,
- ),
- );
- } catch (error) {
+ let nextToken: string;
+ try {
+ nextToken = await refreshAccessToken(fetchImpl, baseUrl);
+ } catch (error) {
+ isRefreshing = false;
await store.set(needsReauthAtom, true);
await processQueue({ error: error as Error, token: null }, fetchImpl);
throw error;
- } finally {
- isRefreshing = false;
}
+
+ isRefreshing = false;
+ await processQueue({ error: null, token: nextToken }, fetchImpl);
+
+ const retryHeaders = new Headers(init?.headers);
+ retryHeaders.set('x-packrat-rpc-retry', 'true');
+
+ return await fetchImpl(
+ input,
+ await withAuthHeaders(
+ {
+ ...cloneInit(init),
+ headers: retryHeaders,
+ },
+ nextToken,
+ ),
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/lib/api/rpcTransport.ts` around lines 124 - 159, The refresh lock
is held through the retry, so new 401s that enqueue while draining the queue can
deadlock; after successfully obtaining nextToken in the refresh block (after
await refreshAccessToken(...)), clear the lock (set isRefreshing = false) before
calling processQueue and before retrying the original request (the subsequent
fetchImpl/withAuthHeaders call) so queued requests can trigger a new refresh if
needed; update the refresh branch around refreshAccessToken, processQueue, and
the retry logic (symbols: isRefreshing, refreshAccessToken, processQueue,
failedQueue, fetchImpl, withAuthHeaders) to release the lock immediately after
obtaining nextToken and before processing the queue/retrying.
Comment on lines
+1340
to
+1345
| const adminRpcRoutes = new OpenAPIHono<{ Bindings: Env; Variables: Variables }>().openapiRoutes( | ||
| adminApiOpenApiRoutes, | ||
| ); | ||
|
|
||
| export type AdminAppType = typeof adminRpcRoutes; | ||
| export { adminRoutes, adminRpcRoutes }; |
Contributor
There was a problem hiding this comment.
Do not export the middleware-free admin RPC router as a runtime value.
adminRpcRoutes contains admin handlers but not the auth middleware from adminRoutes.use('*', ...). If it is imported and mounted later, admin JSON mutations become unprotected. Export only the type.
🛡️ Proposed fix
const adminRpcRoutes = new OpenAPIHono<{ Bindings: Env; Variables: Variables }>().openapiRoutes(
adminApiOpenApiRoutes,
);
export type AdminAppType = typeof adminRpcRoutes;
-export { adminRoutes, adminRpcRoutes };
+export { adminRoutes };Then update the re-export in packages/api/src/index.ts to use the exported type:
-import type { adminRpcRoutes } from '@packrat/api/routes/admin';
+import type { AdminAppType as AdminRpcAppType } from '@packrat/api/routes/admin';-export type AdminAppType = typeof adminRpcRoutes;
+export type AdminAppType = AdminRpcAppType;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/routes/admin/index.ts` around lines 1340 - 1345, The file
currently exports the runtime value adminRpcRoutes (which lacks auth middleware)
and a type AdminAppType; change the export so only the type is exported and the
runtime router is not re-exported: stop exporting the adminRpcRoutes value and
keep/export only AdminAppType and adminRoutes as appropriate. Specifically,
remove adminRpcRoutes from the value export list and ensure AdminAppType (the
type alias for typeof adminRpcRoutes) remains exported; then update the package
re-export in packages/api/src/index.ts to re-export the AdminAppType type (not
the runtime adminRpcRoutes) so downstream consumers can use the type without
obtaining the unprotected runtime router.
- trips/getTripsRoute: remove includePublic param that exposed all users' trips - trips/deleteTripRoute: return 404 for missing trip, 403 for wrong owner - trips/getTripByIdRoute: use TripWithPackSchema (pack relation included in query) - trips/schemas: extend TripWithPackSchema with pack field; tighten dates to datetime() - schemas: extract positiveIntegerQueryParam to shared queryParams.ts - reader.ts: replace literal NBSP in regex with escape - rpcTransport: set x-packrat-rpc-retry header on queued 401 retries https://claude.ai/code/session_01JtEqZjLSh3kkycRSrrF3f5
Switching from expo/tsconfig.base to ../../tsconfig.json lost the customConditions: ["react-native"] setting, which TypeScript needs to resolve the react-native conditional export when a package ships different types for RN vs web. https://claude.ai/code/session_01JtEqZjLSh3kkycRSrrF3f5
Conflicts resolved: - packages/api/package.json: took our structure (private, type, exports, main, types fields) with main's version bump to 2.0.22 - bun.lock: started from our HEAD lockfile (preserves hono dep in api-client) and bumped all 15 workspace package entries from 2.0.21 → 2.0.22 to match the version bump https://claude.ai/code/session_01JtEqZjLSh3kkycRSrrF3f5
Contributor
Coverage Report for Expo Unit Tests Coverage (./apps/expo)
File CoverageNo changed files found. |
Contributor
Coverage Report for API Unit Tests Coverage (./packages/api)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
andrew-bierman
commented
Apr 22, 2026
| "paths": { | ||
| "expo-app/*": ["./*"] | ||
| "expo-app/*": ["./*"], | ||
| "@packrat/api": ["../../packages/api/src/index.ts"], |
Collaborator
Author
There was a problem hiding this comment.
Should this be in base
| @@ -1,11 +1,16 @@ | |||
| { | |||
| "extends": "expo/tsconfig.base", | |||
| "extends": "../../tsconfig.json", | |||
Collaborator
Author
There was a problem hiding this comment.
Does base work better ?
Collaborator
Author
There was a problem hiding this comment.
Extending ../../tsconfig.json lets us inherit all @packrat/* workspace path aliases without duplicating them. The one thing it was missing from expo/tsconfig.base was customConditions: ["react-native"] — added that in the latest commit.
Generated by Claude Code
| // Mount routes | ||
| // Mount routes — explicit openapiRoutes slices for hc<> type inference | ||
| // (full routes mount exceeds TS depth limit; each domain must use openapiRoutes for RPC typing) | ||
| const rpcRoutes = new OpenAPIHono<{ Bindings: Env; Variables: Variables }>() |
Collaborator
Author
There was a problem hiding this comment.
Can we use normal router ?
Collaborator
Author
There was a problem hiding this comment.
No — typeof routes hits TypeScript's instantiation depth limit because of the nested $() wrappers + middleware chain. rpcRoutes is a separate flat instance used only for AppType; it never handles requests. Removed it from the value export so it's type-only now.
Generated by Claude Code
- api-client/package.json: add @packrat/api workspace dep (client.ts imports AppType from it) - api/src/index.ts: remove rpcRoutes from value export — type-only, never handles requests - routes/index.ts: add <Bindings/Variables> generics to all three OpenAPIHono instances - trips/getTripByIdRoute: exclude soft-deleted trips from query - trips/updateTripRoute: exclude soft-deleted trips from update and re-fetch queries - bun.lock: add @packrat/api workspace entry to api-client lockfile record https://claude.ai/code/session_01JtEqZjLSh3kkycRSrrF3f5
1 task
This was referenced Apr 24, 2026
This was referenced May 7, 2026
andrew-bierman
added a commit
that referenced
this pull request
May 14, 2026
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 by CodeRabbit
New Features
Tests