244 partial zod openapi hono#288
Conversation
…inityBowman/corates into 244-partial-zod-openapi-hono
…inityBowman/corates into 244-partial-zod-openapi-hono
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR migrates the workers package from manually-generated OpenAPI documentation to inline OpenAPI schema definitions using Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Health Check Route
participant DB as D1 Database
participant Storage as R2 Storage
participant DO as Durable Objects
Client->>Server: GET /health
par Database Check
Server->>DB: SELECT 1
DB-->>Server: ok (healthy) or error
and Storage Check
Server->>Storage: List PDF bucket
Storage-->>Server: ok (healthy) or error
and Durable Objects Check
Server->>DO: Check env bindings
DO-->>Server: status (healthy) or error
end
alt All checks healthy
Server-->>Client: 200 HealthResponse {status: "healthy"}
else Any check failed
Server-->>Client: 503 HealthResponse {status: "degraded"}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | dd22d32 | Commit Preview URL | Jan 14 2026, 12:32 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| 🔵 In progress View logs |
corates | 5edfd2d | Jan 13 2026, 11:24 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
packages/workers/src/routes/google-drive.js (2)
464-471: UsePROJECT_ERRORS.NOT_FOUNDfor missing project.Using
SYSTEM_ERRORS.DB_ERRORfor a "project not found" case is semantically incorrect. This is a business logic error (resource not found), not a database failure. Based on coding guidelines, use the appropriate error constant.Proposed fix
if (!project) { - const error = createDomainError(SYSTEM_ERRORS.DB_ERROR, { - operation: 'fetch_project_for_import', + const error = createDomainError(PROJECT_ERRORS.NOT_FOUND, { projectId, - message: 'Project not found', }); return c.json(error, error.statusCode); }You'll need to add
PROJECT_ERRORSto the imports:import { createDomainError, createValidationError, VALIDATION_ERRORS, AUTH_ERRORS, FILE_ERRORS, SYSTEM_ERRORS, + PROJECT_ERRORS, isDomainError, isPdfSignature, PDF_MAGIC_BYTES, } from '@corates/shared';
496-512: Database insert failure silently ignored, creating orphaned files in R2.If the
mediaFilesinsert fails, the error is logged but the operation returns success to the user. This leaves an orphaned file in R2 with no corresponding database record, making it untrackable and unmanageable.The user will see a successful response but the file won't appear in their project, leading to confusion and wasted storage.
Proposed fix: propagate the error
try { await db.insert(mediaFiles).values({ id: mediaFileId, filename: uniqueFileName, originalName: originalFileName, fileType: 'application/pdf', fileSize: fileSize, uploadedBy: user.id, bucketKey: r2Key, orgId: project.orgId, projectId, studyId, createdAt: new Date(), }); } catch (dbError) { console.error('Failed to insert mediaFiles record after Google Drive import:', dbError); + // Attempt to clean up the orphaned R2 file + try { + await c.env.PDF_BUCKET.delete(r2Key); + } catch (cleanupError) { + console.error('Failed to clean up R2 file after DB error:', cleanupError); + } + const systemError = createDomainError(SYSTEM_ERRORS.DB_ERROR, { + operation: 'insert_media_file_record', + originalError: dbError?.message || String(dbError), + }); + return c.json(systemError, systemError.statusCode); }packages/web/src/primitives/__tests__/useProject.test.js (1)
977-982: Inconsistent use ofindexeddbProviderinstead ofdexieProvider.This test still references
indexeddbProviderwhile all other tests have been migrated to usedexieProvider. Based on the implementation (which usesdexieProvider: nullin the entry structure), these mock assignments won't have the intended effect.🔧 Suggested fix to align with dexieProvider naming
// Mock cleanup conn1.connectionManager = { destroy: vi.fn() }; - conn1.indexeddbProvider = { destroy: vi.fn() }; + conn1.dexieProvider = true; const ydoc1DestroySpy = vi.spyOn(conn1.ydoc, 'destroy'); conn2.connectionManager = { destroy: vi.fn() }; - conn2.indexeddbProvider = { destroy: vi.fn() }; + conn2.dexieProvider = true; const ydoc2DestroySpy = vi.spyOn(conn2.ydoc, 'destroy');packages/workers/src/routes/orgs/dev-routes.js (1)
278-285: Handle undefined template parameter.
templateis defined as optional in the schema (line 122), but it's used directly in URL construction without a null check. When undefined, the URL becomes?template=undefined&mode=replace.Proposed fix
const query = c.req.valid('query'); const template = query.template; const mode = query.mode || 'replace'; + if (!template) { + return c.json({ error: 'Template ID is required' }, 400); + } + try { const projectDoc = getProjectDocStub(c.env, projectId); const response = await projectDoc.fetch(Alternatively, make the parameter required in the schema:
request: { query: z.object({ - template: z.string().optional().openapi({ description: 'Template ID to apply' }), + template: z.string().openapi({ description: 'Template ID to apply' }), mode: z.enum(['replace', 'merge']).optional().openapi({ description: 'Apply mode', example: 'replace' }), }), },packages/workers/src/routes/users.js (1)
368-377: Deleting user account cascades projects to other members without proper cleanup.Line 371 deletes all projects where the user is the creator. Since users created as
projectMemberswith role 'owner' are automatically added on creation (seeprojects.js:312-318), projects can have other collaborators. When the account is deleted, those collaborators lose access without notification becausesyncMemberToDO(line 365) only notifies member removal—not project deletion.The test (lines 464-525 in
users.test.js) only covers the single-member scenario and doesn't verify this data loss impact.Consider:
- Only delete projects where the user is the sole member
- Transfer ownership to another member before deletion
- Keep the project but null the
createdByreference- Explicitly sync project deletion to Durable Objects for all members
- Add test coverage for multi-member projects
packages/workers/src/routes/admin/projects.js (1)
601-628: Admin member removal doesn't check for last-owner scenario.Unlike
orgs/members.jswhich protects against removing the last owner (lines 769-785), this admin route removes members without checking if it would leave the project without an owner. While admins may need this power, consider at least warning or requiring confirmation.🔧 Suggested: Add last-owner warning/check
projectRoutes.openapi(removeProjectMemberRoute, async c => { const { projectId, memberId } = c.req.valid('param'); const db = createDb(c.env.DB); try { - // Verify the member belongs to the project - const [existingMember] = await db - .select({ id: projectMembers.id }) + // Verify the member belongs to the project and get their role + const [existingMember] = await db + .select({ id: projectMembers.id, role: projectMembers.role }) .from(projectMembers) .where(and(eq(projectMembers.id, memberId), eq(projectMembers.projectId, projectId))) .limit(1); if (!existingMember) { const error = createDomainError(PROJECT_ERRORS.NOT_FOUND, { memberId }); return c.json(error, error.statusCode); } + // Warn if removing the last owner + if (existingMember.role === 'owner') { + const [ownerCount] = await db + .select({ count: count() }) + .from(projectMembers) + .where(and(eq(projectMembers.projectId, projectId), eq(projectMembers.role, 'owner'))); + + if (ownerCount?.count <= 1) { + // Log warning but allow admin to proceed + console.warn(`[Admin] Removing last owner from project ${projectId}`); + } + } + await db.delete(projectMembers).where(eq(projectMembers.id, memberId));packages/workers/src/routes/admin/users.js (1)
1064-1080: DO sync before DB batch creates inconsistency risk; also, project deletion may orphan members.Two concerns:
Consistency: If DO sync succeeds (line 1064-1067) but the database batch fails (line 1071), the DOs will have removed the member while the database still contains the membership records.
Project ownership: Line 1074 deletes projects where
createdBy = userId, but these projects may have other members who would lose access without warning. Consider transferring ownership instead.🐛 Suggested fix for consistency and ownership
// Fetch all projects the user is a member of before any deletions (with orgId) const userProjects = await db .select({ projectId: projectMembers.projectId, orgId: projects.orgId, }) .from(projectMembers) .innerJoin(projects, eq(projectMembers.projectId, projects.id)) .where(eq(projectMembers.userId, userId)); - // Sync all member removals to DOs atomically (fail fast if any fails) - await Promise.all( - userProjects.map(({ projectId }) => syncMemberToDO(c.env, projectId, 'remove', { userId })), - ); - - // Only proceed with database deletions if all DO syncs succeeded // Delete all user data atomically using batch transaction await db.batch([ db.update(mediaFiles).set({ uploadedBy: null }).where(eq(mediaFiles.uploadedBy, userId)), db.delete(projectMembers).where(eq(projectMembers.userId, userId)), - db.delete(projects).where(eq(projects.createdBy, userId)), + // Transfer project ownership instead of deleting - find another owner or admin + // For now, just nullify createdBy to preserve projects with other members + db.update(projects).set({ createdBy: null }).where(eq(projects.createdBy, userId)), db.delete(twoFactor).where(eq(twoFactor.userId, userId)), db.delete(session).where(eq(session.userId, userId)), db.delete(account).where(eq(account.userId, userId)), db.delete(verification).where(eq(verification.identifier, userToDelete.email)), db.delete(user).where(eq(user.id, userId)), ]); + // Sync member removals to DOs after successful DB transaction + // Failures here are logged but don't rollback the deletion + await Promise.allSettled( + userProjects.map(({ projectId }) => + syncMemberToDO(c.env, projectId, 'remove', { userId }).catch(err => { + console.error(`Failed to sync member removal for project ${projectId}:`, err); + }) + ), + );packages/workers/src/routes/billing/invoices.js (1)
140-142: Update Stripe API version to the current version.The version
'2025-11-17.clover'is valid and follows the correct format, but it's outdated. Stripe's current API version is'2025-12-15.clover'. Update this and all other instances across the codebase to the latest version to ensure access to the newest features and security updates.packages/workers/src/routes/admin/database.js (1)
1-954: Critical: Admin database routes lack authentication middleware.This file exports routes that are mounted at
/api/admin/database/*without any authentication or authorization checks. The routes expose sensitive debugging information including database schemas, table row counts, user emails, and storage analytics.Each route handler should validate that the request is authenticated. Apply
requireAuthmiddleware:Add authentication middleware
+import { requireAuth } from '@/middleware/auth.js'; + const databaseRoutes = new OpenAPIHono({ defaultHook: (result, c) => { // ... }, }); + +// Require authentication for all database routes +databaseRoutes.use('*', requireAuth);
🤖 Fix all issues with AI agents
In @.claude/skills/agent-browser/SKILL.md:
- Around line 1-143: The PR contains unrelated documentation additions (the new
agent-browser SKILL.md) that are outside the stated migration scope; please
remove or revert the changes to .claude/skills/agent-browser/SKILL.md from this
branch (either revert the file in a new commit or create a separate branch/PR
containing the agent-browser documentation), then amend the current PR so it
only includes migration work (keep references to symbols like "agent-browser",
"snapshot", and the SKILL.md filename to locate the change), and ensure commit
history is cleaned (squash or rebase) so the unrelated doc does not appear in
this migration PR.
In @package.json:
- Line 50: The package constraint for agent-browser is pinned to "^0.4.4" which
prevents automatically receiving the 0.5.0 release; update the dependency
specifier for "agent-browser" in package.json to "^0.5.0" and then run your
package manager (npm/yarn/pnpm) to install/update, followed by running the test
suite and any compatibility checks to ensure no breaking changes affect code
that uses agent-browser.
In @packages/web/src/primitives/__tests__/useProject.test.js:
- Around line 26-31: Add assertions in the cleanup unit tests that set
entry.dexieProvider = true to verify DexieYProvider.release was called with the
correct Y.Doc: after the cleanup call assert DexieYProvider.release was invoked
with entry.ydoc (e.g.,
expect(DexieYProvider.release).toHaveBeenCalledWith(entry.ydoc)); ensure the
vi.mock for 'y-dexie' exposes DexieYProvider.release as a spy and reset/clear
the mock between tests so assertions are isolated.
In @packages/workers/src/index.js:
- Around line 71-85: The OpenAPI spec route registered via
app.doc31('/openapi.json', ...) is missing an environment guard and may be
exposed in production; wrap the app.doc31(...) registration in the same
environment check used for the /docs route (inspect c.env.ENVIRONMENT and only
register when not 'production') or attach middleware to block production access
so '/openapi.json' is only available in development; reference app.doc31, the
'/openapi.json' route, and c.env.ENVIRONMENT when making the change.
In @packages/workers/src/routes/admin/billing-observability.js:
- Around line 281-283: The three parsed values incompleteThresholdMinutes,
checkoutNoSubThresholdMinutes, and processingLagThresholdMinutes can become NaN
if query contains non-numeric strings; update the parsing to validate and
fallback to the intended defaults (e.g., parseInt(..., 10) result checked with
Number.isFinite or Number.isNaN and replace with 30/15/5 respectively) or move
this logic into the request validation schema by coercing the query fields to
numbers (e.g., Zod .coerce.number() or .transform()) so invalid inputs are
rejected/coerced before these constants are used.
In @packages/workers/src/routes/admin/database.js:
- Around line 127-136: UserAnalyticsSchema and RecentUploadSchema currently
expose userEmail/uploadedByEmail; remove these email fields from the public
admin analytics schemas and responses and instead return non-identifying
identifiers (userId and userName) or a redacted/hash version if you must keep an
email-like value. Update UserAnalyticsSchema and RecentUploadSchema to drop
userEmail/uploadedByEmail, update any code that constructs these responses to
stop including raw emails (replace with userId/userName or redactedEmail), and
if emails are required for privileged debugging, surface them only via a
separate explicit admin-only endpoint or flag that returns fullEmail after
additional access checks.
In @packages/workers/src/routes/admin/storage.js:
- Around line 180-212: The deleteDocumentsRoute created via createRoute is
missing a requestBody schema so its handler manually parses/validates the
payload; add a new request schema (e.g., DeleteDocumentsRequestSchema with {
keys: string[] }) and attach it to deleteDocumentsRoute under requestBody ->
content -> 'application/json' -> schema to enable OpenAPI/defaultHook
validation; then remove or simplify the manual validation in the handler that
currently parses the body so the route relies on the built-in validation,
keeping existing response schemas (DeleteDocumentsResponseSchema and
StorageErrorSchema) unchanged.
In @packages/workers/src/routes/admin/stripe-tools.js:
- Around line 593-595: The code reads query.limit and does
Math.min(parseInt(query.limit || '10', 10), 50) which yields NaN for non-numeric
strings; change the logic in the handler around query and limit so you first
parse the value, check for NaN, and fall back to the default (10) before capping
at 50 — e.g., parse the limit from query.limit into a variable (using parseInt
or Number), if Number.isNaN(parsed) use 10, then set limit =
Math.min(parsedOrDefault, 50); alternatively enforce/coerce the limit as a
number in the route schema (z.coerce.number()) so the handler always receives a
valid numeric limit.
In @packages/workers/src/routes/email.js:
- Around line 15-40: The defaultHook in OpenAPIHono always constructs an error
with VALIDATION_ERRORS.FIELD_REQUIRED.code; update the logic inside defaultHook
(the block handling result.error.issues[0]) to choose the validation code based
on the actual issue (e.g., inspect firstIssue.type, firstIssue.code, or patterns
in firstIssue.message) so missing fields still map to
VALIDATION_ERRORS.FIELD_REQUIRED.code but format/type errors map to
VALIDATION_ERRORS.FIELD_INVALID.code (or other appropriate entries from
VALIDATION_ERRORS); keep building the user-facing message like you do now, set
error = createValidationError(String(field), chosenCode, null), preserve
error.message, and return c.json(error, 400).
- Around line 23-26: Replace fragile string checks in the isMissing computation
with Zod's structured error properties: stop using message.includes('received
undefined') and message.includes('Required') and instead detect
required/undefined fields by checking firstIssue?.code === 'invalid_type' &&
firstIssue?.received === 'undefined' (alongside any existing checks on
firstIssue?.received). Update the isMissing logic (the const isMissing
definition that references firstIssue, received, and message) to use this
code/received pattern consistent with other files like contact.js.
In @packages/workers/src/routes/orgs/dev-routes.js:
- Around line 188-220: The importRoute is missing a request body schema while
the handler (import route handler referenced at line ~323) expects JSON input;
add a request.body entry to importRoute that specifies application/json and
references an ImportRequestSchema (or create ImportRequestSchema matching the
handler's expected fields), mark it required, and ensure the schema shape
matches the handler's validation/fields so OpenAPI generation and runtime
validation cover the incoming JSON.
- Line 257: The openapi() calls incorrectly include middleware as a second
argument (devRoutes.openapi(getTemplatesRoute, requireProjectAccess(), ...))
which passes requireProjectAccess() into the handler position; remove the
middleware from the openapi() calls and apply it via Hono's per-route middleware
pattern by either calling devRoutes.use(getTemplatesRoute.getRoutingPath(),
requireProjectAccess()) before devRoutes.openapi(getTemplatesRoute, async c => {
... }) or by chaining middleware correctly inside openapi as
devRoutes.openapi(getTemplatesRoute, async c => { ... }) and registering
requireProjectAccess() with devRoutes.use for the GET /dev/templates
(getTemplatesRoute) and POST /dev/apply-template (applyTemplateRoute) routes so
requireProjectAccess() runs as middleware, not as a handler argument.
In @packages/workers/src/routes/orgs/pdfs.js:
- Around line 654-682: The handler currently logs DB insert failures but still
returns the generated mediaFileId (mediaFileId) after the R2 upload; change the
catch around db.insert(mediaFiles).values(...) so that on error you either (A)
delete the uploaded R2 object identified by bucket key (variable key / R2 client
used earlier) and then return a 5xx error response instead of proceeding to
return c.json, or (B) immediately return a 5xx error (do not return mediaFileId)
so the client does not receive an unpersisted ID; ensure the catch does not
swallow the error and that any cleanup (R2 deletion) is awaited before
responding and that the return c.json block only runs when the insert succeeded.
In @packages/workers/src/routes/orgs/projects.js:
- Around line 456-467: The JSON success response for the project creation is
missing the createdBy field required by ProjectSchema; update the returned
object (the response that currently includes id, name, description, orgId, role,
createdAt, updatedAt) to include a createdBy property that matches the schema (a
nullable string) — set it to the current user id if available (e.g., use the
existing auth/user id variable in the handler) or explicitly null if no user id
is present so the response conforms to ProjectSchema.
In @packages/workers/src/routes/projects.js:
- Around line 359-368: The response object newProject returns the original
untrimmed name and description while the DB write uses name.trim() and
description?.trim() || null; update the newProject construction to use the same
trimmed values (e.g., use trimmedName = name.trim() and trimmedDescription =
description?.trim() || null or inline name.trim() / description?.trim() || null)
so the API response matches what is stored, keeping id/projectId, role,
createdAt and updatedAt unchanged.
| --- | ||
| name: agent-browser | ||
| description: Automates browser interactions for web testing, form filling, screenshots, and data extraction. Use when the user needs to navigate websites, interact with web pages, fill forms, take screenshots, test web applications, or extract information from web pages. | ||
| --- | ||
|
|
||
| # Browser Automation with agent-browser | ||
|
|
||
| ## Quick start | ||
|
|
||
| ```bash | ||
| agent-browser open <url> # Navigate to page | ||
| agent-browser snapshot -i # Get interactive elements with refs | ||
| agent-browser click @e1 # Click element by ref | ||
| agent-browser fill @e2 "text" # Fill input by ref | ||
| agent-browser close # Close browser | ||
| ``` | ||
|
|
||
| ## Core workflow | ||
|
|
||
| 1. Navigate: `agent-browser open <url>` | ||
| 2. Snapshot: `agent-browser snapshot -i` (returns elements with refs like `@e1`, `@e2`) | ||
| 3. Interact using refs from the snapshot | ||
| 4. Re-snapshot after navigation or significant DOM changes | ||
|
|
||
| ## Commands | ||
|
|
||
| ### Navigation | ||
| ```bash | ||
| agent-browser open <url> # Navigate to URL | ||
| agent-browser back # Go back | ||
| agent-browser forward # Go forward | ||
| agent-browser reload # Reload page | ||
| agent-browser close # Close browser | ||
| ``` | ||
|
|
||
| ### Snapshot (page analysis) | ||
| ```bash | ||
| agent-browser snapshot # Full accessibility tree | ||
| agent-browser snapshot -i # Interactive elements only (recommended) | ||
| agent-browser snapshot -c # Compact output | ||
| agent-browser snapshot -d 3 # Limit depth to 3 | ||
| ``` | ||
|
|
||
| ### Interactions (use @refs from snapshot) | ||
| ```bash | ||
| agent-browser click @e1 # Click | ||
| agent-browser dblclick @e1 # Double-click | ||
| agent-browser fill @e2 "text" # Clear and type | ||
| agent-browser type @e2 "text" # Type without clearing | ||
| agent-browser press Enter # Press key | ||
| agent-browser press Control+a # Key combination | ||
| agent-browser hover @e1 # Hover | ||
| agent-browser check @e1 # Check checkbox | ||
| agent-browser uncheck @e1 # Uncheck checkbox | ||
| agent-browser select @e1 "value" # Select dropdown | ||
| agent-browser scroll down 500 # Scroll page | ||
| agent-browser scrollintoview @e1 # Scroll element into view | ||
| ``` | ||
|
|
||
| ### Get information | ||
| ```bash | ||
| agent-browser get text @e1 # Get element text | ||
| agent-browser get value @e1 # Get input value | ||
| agent-browser get title # Get page title | ||
| agent-browser get url # Get current URL | ||
| ``` | ||
|
|
||
| ### Screenshots | ||
| ```bash | ||
| agent-browser screenshot # Screenshot to stdout | ||
| agent-browser screenshot path.png # Save to file | ||
| agent-browser screenshot --full # Full page | ||
| ``` | ||
|
|
||
| ### Wait | ||
| ```bash | ||
| agent-browser wait @e1 # Wait for element | ||
| agent-browser wait 2000 # Wait milliseconds | ||
| agent-browser wait --text "Success" # Wait for text | ||
| agent-browser wait --load networkidle # Wait for network idle | ||
| ``` | ||
|
|
||
| ### Semantic locators (alternative to refs) | ||
| ```bash | ||
| agent-browser find role button click --name "Submit" | ||
| agent-browser find text "Sign In" click | ||
| agent-browser find label "Email" fill "user@test.com" | ||
| ``` | ||
|
|
||
| ## Example: Form submission | ||
|
|
||
| ```bash | ||
| agent-browser open https://example.com/form | ||
| agent-browser snapshot -i | ||
| # Output shows: textbox "Email" [ref=e1], textbox "Password" [ref=e2], button "Submit" [ref=e3] | ||
|
|
||
| agent-browser fill @e1 "user@example.com" | ||
| agent-browser fill @e2 "password123" | ||
| agent-browser click @e3 | ||
| agent-browser wait --load networkidle | ||
| agent-browser snapshot -i # Check result | ||
| ``` | ||
|
|
||
| ## Example: Authentication with saved state | ||
|
|
||
| ```bash | ||
| # Login once | ||
| agent-browser open https://app.example.com/login | ||
| agent-browser snapshot -i | ||
| agent-browser fill @e1 "username" | ||
| agent-browser fill @e2 "password" | ||
| agent-browser click @e3 | ||
| agent-browser wait --url "**/dashboard" | ||
| agent-browser state save auth.json | ||
|
|
||
| # Later sessions: load saved state | ||
| agent-browser state load auth.json | ||
| agent-browser open https://app.example.com/dashboard | ||
| ``` | ||
|
|
||
| ## Sessions (parallel browsers) | ||
|
|
||
| ```bash | ||
| agent-browser --session test1 open site-a.com | ||
| agent-browser --session test2 open site-b.com | ||
| agent-browser session list | ||
| ``` | ||
|
|
||
| ## JSON output (for parsing) | ||
|
|
||
| Add `--json` for machine-readable output: | ||
| ```bash | ||
| agent-browser snapshot -i --json | ||
| agent-browser get text @e1 --json | ||
| ``` | ||
|
|
||
| ## Debugging | ||
|
|
||
| ```bash | ||
| agent-browser open example.com --headed # Show browser window | ||
| agent-browser console # View console messages | ||
| agent-browser errors # View page errors | ||
| ``` |
There was a problem hiding this comment.
Inconsistent PR scope: Unrelated documentation changes.
This new browser automation skill documentation is unrelated to the PR's stated objective of migrating workers to Zod/OpenAPI/Hono. Including unrelated changes in a PR reduces reviewability and makes git history less meaningful. Consider moving documentation additions like this to a separate PR.
🤖 Prompt for AI Agents
In @.claude/skills/agent-browser/SKILL.md around lines 1 - 143, The PR contains
unrelated documentation additions (the new agent-browser SKILL.md) that are
outside the stated migration scope; please remove or revert the changes to
.claude/skills/agent-browser/SKILL.md from this branch (either revert the file
in a new commit or create a separate branch/PR containing the agent-browser
documentation), then amend the current PR so it only includes migration work
(keep references to symbols like "agent-browser", "snapshot", and the SKILL.md
filename to locate the change), and ensure commit history is cleaned (squash or
rebase) so the unrelated doc does not appear in this migration PR.
| "devDependencies": { | ||
| "@eslint/js": "^9.39.2", | ||
| "@typescript-eslint/parser": "^8.53.0", | ||
| "agent-browser": "^0.4.4", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch agent-browser package info from npm registry
npm_info=$(curl -s https://registry.npmjs.org/agent-browser)
echo "=== Package Details ==="
echo "$npm_info" | jq -r '.["dist-tags"] | to_entries | map("\(.key): \(.value)") | .[]'
echo ""
echo "=== Latest Version Info ==="
latest_version=$(echo "$npm_info" | jq -r '.["dist-tags"].latest')
echo "Latest version: $latest_version"
echo ""
echo "=== Version 0.4.4 Details ==="
echo "$npm_info" | jq ".versions[\"0.4.4\"] | {published: .time, deprecated: .deprecated}" 2>/dev/null || echo "Version 0.4.4 info not found"
echo ""
echo "=== Recent Versions Timeline ==="
echo "$npm_info" | jq -r '.time | to_entries | select(.key != "created" and .key != "modified") | sort_by(.value) | reverse | .[0:5] | .[] | "\(.key): \(.value)"'Repository: InfinityBowman/corates
Length of output: 345
🏁 Script executed:
#!/bin/bash
# Get all available versions and their timestamps
echo "=== All Available Versions ==="
npm_info=$(curl -s https://registry.npmjs.org/agent-browser)
# Get versions list and display recent ones
echo "$npm_info" | jq -r '.versions | keys | sort -V | reverse | .[0:10]'
# Try to get release date info
echo ""
echo "=== Version Release Information ==="
echo "$npm_info" | jq -r '.time | to_entries | select(.key as $k | ["0.5.0", "0.4.4", "0.4.3", "0.4.2"] | index($k)) | "\(.key): \(.value)"' 2>/dev/null
# Check if package has security advisory or deprecation
echo ""
echo "=== Package Metadata ==="
echo "$npm_info" | jq '{deprecated: .deprecated, name: .name, description: .description}'Repository: InfinityBowman/corates
Length of output: 440
🏁 Script executed:
#!/bin/bash
# Get full npm package data and extract versions more carefully
npm_info=$(curl -s https://registry.npmjs.org/agent-browser)
echo "=== Available Versions (last 10) ==="
echo "$npm_info" | jq '.versions | keys' | tail -20
echo ""
echo "=== Specific Version Info ==="
echo "$npm_info" | jq '.versions | {v044: ."0.4.4", v050: ."0.5.0"}' | head -30
echo ""
echo "=== Time/Release Info ==="
echo "$npm_info" | jq '.time | {created, modified, v044: ."0.4.4", v050: ."0.5.0"}'
echo ""
echo "=== Package Maintainers & Latest Info ==="
echo "$npm_info" | jq '{name, latest: ."dist-tags".latest, maintainers: .maintainers}'Repository: InfinityBowman/corates
Length of output: 1472
Consider updating agent-browser to the latest version.
Version 0.5.0 was released on 2026-01-13 (one day after 0.4.4). The current constraint ^0.4.4 caps at <0.5.0, preventing automatic updates to the latest release. The package is actively maintained by the Vercel Labs team (maintained by ctate) with no known security issues. Consider updating the version constraint to ^0.5.0 unless 0.4.4 is intentionally pinned for compatibility reasons.
🤖 Prompt for AI Agents
In @package.json at line 50, The package constraint for agent-browser is pinned
to "^0.4.4" which prevents automatically receiving the 0.5.0 release; update the
dependency specifier for "agent-browser" in package.json to "^0.5.0" and then
run your package manager (npm/yarn/pnpm) to install/update, followed by running
the test suite and any compatibility checks to ensure no breaking changes affect
code that uses agent-browser.
| vi.mock('y-dexie', () => ({ | ||
| default: () => {}, // Dexie addon (no-op in tests) | ||
| DexieYProvider: { | ||
| release: vi.fn(), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for dexieProvider usage in the useProject implementation
rg -n "dexieProvider" packages/web/src/primitives/ --type jsRepository: InfinityBowman/corates
Length of output: 1221
🏁 Script executed:
#!/bin/bash
# Get context around the dexieProvider checks in the implementation
sed -n '70,85p' packages/web/src/primitives/useProject/index.js
echo "---"
sed -n '100,115p' packages/web/src/primitives/useProject/index.jsRepository: InfinityBowman/corates
Length of output: 1030
Add assertions to verify DexieYProvider.release() is called during cleanup.
The implementation calls DexieYProvider.release(entry.ydoc) during cleanup at lines 76 and 104, but tests at lines 775, 855, 902, 1036, and 1040 only set entry.dexieProvider = true without asserting the release call. Update the cleanup tests to verify that DexieYProvider.release is invoked with the correct Y.Doc instance when dexieProvider exists:
expect(DexieYProvider.release).toHaveBeenCalledWith(entry.ydoc);🤖 Prompt for AI Agents
In @packages/web/src/primitives/__tests__/useProject.test.js around lines 26 -
31, Add assertions in the cleanup unit tests that set entry.dexieProvider = true
to verify DexieYProvider.release was called with the correct Y.Doc: after the
cleanup call assert DexieYProvider.release was invoked with entry.ydoc (e.g.,
expect(DexieYProvider.release).toHaveBeenCalledWith(entry.ydoc)); ensure the
vi.mock for 'y-dexie' exposes DexieYProvider.release as a spy and reset/clear
the mock between tests so assertions are isolated.
| // OpenAPI JSON spec (development only) | ||
| app.doc31('/openapi.json', c => ({ | ||
| openapi: '3.1.0', | ||
| info: { | ||
| title: 'Corates API', | ||
| version: '1.0.0', | ||
| description: 'API for Corates - Collaborative Research Appraisal Tool for Evidence Synthesis', | ||
| }, | ||
| servers: [ | ||
| { | ||
| url: c.env.ENVIRONMENT === 'production' ? 'https://corates.org' : 'http://localhost:8787', | ||
| description: c.env.ENVIRONMENT === 'production' ? 'Production' : 'Local development', | ||
| }, | ||
| ], | ||
| })); |
There was a problem hiding this comment.
Add environment guard for OpenAPI spec endpoint.
The comment indicates this should be development-only, but unlike /docs (line 66), there's no environment check. The spec could expose internal API details in production.
🐛 Proposed fix
// OpenAPI JSON spec (development only)
-app.doc31('/openapi.json', c => ({
+app.get('/openapi.json', c => {
+ if (c.env.ENVIRONMENT === 'production') return c.text('Not Found', 404);
+ return c.json({
+ openapi: '3.1.0',
+ info: {
+ title: 'Corates API',
+ version: '1.0.0',
+ description: 'API for Corates - Collaborative Research Appraisal Tool for Evidence Synthesis',
+ },
+ servers: [
+ {
+ url: c.env.ENVIRONMENT === 'production' ? 'https://corates.org' : 'http://localhost:8787',
+ description: c.env.ENVIRONMENT === 'production' ? 'Production' : 'Local development',
+ },
+ ],
+ });
+});
- openapi: '3.1.0',
- info: {
- title: 'Corates API',
- version: '1.0.0',
- description: 'API for Corates - Collaborative Research Appraisal Tool for Evidence Synthesis',
- },
- servers: [
- {
- url: c.env.ENVIRONMENT === 'production' ? 'https://corates.org' : 'http://localhost:8787',
- description: c.env.ENVIRONMENT === 'production' ? 'Production' : 'Local development',
- },
- ],
-}));Alternatively, if you want to keep using app.doc31() for the OpenAPI generation benefits, wrap the entire route registration conditionally or add middleware.
📝 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.
| // OpenAPI JSON spec (development only) | |
| app.doc31('/openapi.json', c => ({ | |
| openapi: '3.1.0', | |
| info: { | |
| title: 'Corates API', | |
| version: '1.0.0', | |
| description: 'API for Corates - Collaborative Research Appraisal Tool for Evidence Synthesis', | |
| }, | |
| servers: [ | |
| { | |
| url: c.env.ENVIRONMENT === 'production' ? 'https://corates.org' : 'http://localhost:8787', | |
| description: c.env.ENVIRONMENT === 'production' ? 'Production' : 'Local development', | |
| }, | |
| ], | |
| })); | |
| // OpenAPI JSON spec (development only) | |
| app.get('/openapi.json', c => { | |
| if (c.env.ENVIRONMENT === 'production') return c.text('Not Found', 404); | |
| return c.json({ | |
| openapi: '3.1.0', | |
| info: { | |
| title: 'Corates API', | |
| version: '1.0.0', | |
| description: 'API for Corates - Collaborative Research Appraisal Tool for Evidence Synthesis', | |
| }, | |
| servers: [ | |
| { | |
| url: c.env.ENVIRONMENT === 'production' ? 'https://corates.org' : 'http://localhost:8787', | |
| description: c.env.ENVIRONMENT === 'production' ? 'Production' : 'Local development', | |
| }, | |
| ], | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In @packages/workers/src/index.js around lines 71 - 85, The OpenAPI spec route
registered via app.doc31('/openapi.json', ...) is missing an environment guard
and may be exposed in production; wrap the app.doc31(...) registration in the
same environment check used for the /docs route (inspect c.env.ENVIRONMENT and
only register when not 'production') or attach middleware to block production
access so '/openapi.json' is only available in development; reference app.doc31,
the '/openapi.json' route, and c.env.ENVIRONMENT when making the change.
| const incompleteThresholdMinutes = parseInt(query.incompleteThreshold || '30', 10); | ||
| const checkoutNoSubThresholdMinutes = parseInt(query.checkoutNoSubThreshold || '15', 10); | ||
| const processingLagThresholdMinutes = parseInt(query.processingLagThreshold || '5', 10); |
There was a problem hiding this comment.
parseInt can return NaN without validation.
If a non-numeric string is passed (e.g., ?incompleteThreshold=abc), parseInt returns NaN, which could cause unexpected behavior in numeric comparisons (e.g., NaN > 30 is false).
🐛 Suggested fix
- const incompleteThresholdMinutes = parseInt(query.incompleteThreshold || '30', 10);
- const checkoutNoSubThresholdMinutes = parseInt(query.checkoutNoSubThreshold || '15', 10);
- const processingLagThresholdMinutes = parseInt(query.processingLagThreshold || '5', 10);
+ const incompleteThresholdMinutes = parseInt(query.incompleteThreshold || '30', 10) || 30;
+ const checkoutNoSubThresholdMinutes = parseInt(query.checkoutNoSubThreshold || '15', 10) || 15;
+ const processingLagThresholdMinutes = parseInt(query.processingLagThreshold || '5', 10) || 5;Alternatively, use Zod's .coerce.number() or .transform() in the query schema to handle this at validation time.
📝 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.
| const incompleteThresholdMinutes = parseInt(query.incompleteThreshold || '30', 10); | |
| const checkoutNoSubThresholdMinutes = parseInt(query.checkoutNoSubThreshold || '15', 10); | |
| const processingLagThresholdMinutes = parseInt(query.processingLagThreshold || '5', 10); | |
| const incompleteThresholdMinutes = parseInt(query.incompleteThreshold || '30', 10) || 30; | |
| const checkoutNoSubThresholdMinutes = parseInt(query.checkoutNoSubThreshold || '15', 10) || 15; | |
| const processingLagThresholdMinutes = parseInt(query.processingLagThreshold || '5', 10) || 5; |
🤖 Prompt for AI Agents
In @packages/workers/src/routes/admin/billing-observability.js around lines 281
- 283, The three parsed values incompleteThresholdMinutes,
checkoutNoSubThresholdMinutes, and processingLagThresholdMinutes can become NaN
if query contains non-numeric strings; update the parsing to validate and
fallback to the intended defaults (e.g., parseInt(..., 10) result checked with
Number.isFinite or Number.isNaN and replace with 30/15/5 respectively) or move
this logic into the request validation schema by coercing the query fields to
numbers (e.g., Zod .coerce.number() or .transform()) so invalid inputs are
rejected/coerced before these constants are used.
|
|
||
| // GET /dev/templates | ||
| devRoutes.get('/templates', requireProjectAccess(), async c => { | ||
| devRoutes.openapi(getTemplatesRoute, requireProjectAccess(), async c => { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@hono/zod-openapi openapi method signature middleware support
💡 Result:
Short answer: @hono/zod-openapi is not a pure "OpenAPI middleware" that you drop globally — it is an OpenAPI-aware Hono wrapper (OpenAPIHono) / route helper, but you can still attach Hono middleware per-route (and register the route with the OpenAPI generator). If you want a middleware-first approach, use the separate hono-openapi project (or third‑party middleware wrappers) which were built specifically to be middleware-based.
Key points and links:
- @hono/zod-openapi is an extension of Hono (OpenAPIHono / createRoute) that registers schemas and generates docs; routes are registered via the package API (app.openapi(route, handler) / createRoute). [1][2]
- You can still attach Hono middleware to the route (e.g. app.use(route.getRoutingPath(), middleware...)) — per-route middleware is supported. [3][4]
- If you need a middleware-first OpenAPI integrator (so handlers remain plain Hono handlers and docs are added purely by middleware), see hono-openapi or third‑party packages (they implement an OpenAPI middleware instead of wrapping Hono). [5][6]
Sources:
[1] npm package README for @hono/zod-openapi. [2] Hono docs / snippets showing zod-openapi usage. [3] @hono/zod-openapi README mentions app.use(route.getRoutingPath(), ...). [4] HonoHub / Hono OpenAPI docs. [5] hono-openapi docs (middleware-first alternative). [6] Third‑party hono-zod-openapi middleware projects and GH issues discussing behavior.
🏁 Script executed:
# Check the dev-routes.js file to see the actual usage pattern
cat -n packages/workers/src/routes/orgs/dev-routes.js | sed -n '250,290p'Repository: InfinityBowman/corates
Length of output: 1591
🏁 Script executed:
# Search for other uses of openapi() in the codebase to understand the pattern
rg -n "\.openapi\(" packages/workers/src/routes/ -A 2 -B 1 | head -100Repository: InfinityBowman/corates
Length of output: 5676
Remove middleware from openapi() call and apply using Hono's per-route middleware pattern.
The @hono/zod-openapi API signature is openapi(route, handler) — a 2-argument pattern. Passing requireProjectAccess() as the second argument places it in the handler position, which breaks the middleware chain. All other routes in the codebase use the correct 2-argument pattern.
Apply middleware using Hono's per-route pattern before the openapi call:
devRoutes.use(getTemplatesRoute.getRoutingPath(), requireProjectAccess());
devRoutes.openapi(getTemplatesRoute, async c => {
Or use Hono's standard middleware chaining:
devRoutes.openapi(getTemplatesRoute, requireProjectAccess(), async c => {
should be:
devRoutes.openapi(getTemplatesRoute, async c => {
// handler
});
This affects both the GET /dev/templates route (line 257) and POST /dev/apply-template route (line 276).
🤖 Prompt for AI Agents
In @packages/workers/src/routes/orgs/dev-routes.js at line 257, The openapi()
calls incorrectly include middleware as a second argument
(devRoutes.openapi(getTemplatesRoute, requireProjectAccess(), ...)) which passes
requireProjectAccess() into the handler position; remove the middleware from the
openapi() calls and apply it via Hono's per-route middleware pattern by either
calling devRoutes.use(getTemplatesRoute.getRoutingPath(),
requireProjectAccess()) before devRoutes.openapi(getTemplatesRoute, async c => {
... }) or by chaining middleware correctly inside openapi as
devRoutes.openapi(getTemplatesRoute, async c => { ... }) and registering
requireProjectAccess() with devRoutes.use for the GET /dev/templates
(getTemplatesRoute) and POST /dev/apply-template (applyTemplateRoute) routes so
requireProjectAccess() runs as middleware, not as a handler argument.
| const { betterAuth } = await import('better-auth'); | ||
| const { magicLink } = await import('better-auth/plugins'); | ||
| const { drizzleAdapter } = await import('better-auth/adapters/drizzle'); | ||
| const { drizzle } = await import('drizzle-orm/d1'); | ||
| const schema = await import('@/db/schema.js'); | ||
| const { MAGIC_LINK_EXPIRY_MINUTES } = await import('@/auth/emailTemplates.js'); | ||
|
|
||
| const authSecret = c.env.AUTH_SECRET || c.env.SECRET; | ||
| if (!authSecret) { | ||
| throw createDomainError( | ||
| SYSTEM_ERRORS.CONFIG_MISSING, | ||
| { key: 'AUTH_SECRET' }, | ||
| 'AUTH_SECRET must be configured', | ||
| ); | ||
| } | ||
|
|
||
| const tempDb = drizzle(c.env.DB, { schema }); | ||
| const tempAuth = betterAuth({ | ||
| database: drizzleAdapter(tempDb, { | ||
| provider: 'sqlite', | ||
| schema: { | ||
| user: schema.user, | ||
| session: schema.session, | ||
| account: schema.account, | ||
| verification: schema.verification, | ||
| twoFactor: schema.twoFactor, | ||
| const tempDb = drizzle(c.env.DB, { schema }); | ||
| const tempAuth = betterAuth({ | ||
| database: drizzleAdapter(tempDb, { | ||
| provider: 'sqlite', | ||
| schema: { | ||
| user: schema.user, | ||
| session: schema.session, | ||
| account: schema.account, | ||
| verification: schema.verification, | ||
| twoFactor: schema.twoFactor, | ||
| }, | ||
| }), | ||
| baseURL: authBaseUrl, | ||
| secret: authSecret, | ||
| plugins: [ | ||
| magicLink({ | ||
| sendMagicLink: async ({ url }) => { | ||
| capturedMagicLinkUrl = url; | ||
| }, | ||
| expiresIn: 60 * MAGIC_LINK_EXPIRY_MINUTES, | ||
| }), | ||
| baseURL: authBaseUrl, | ||
| secret: authSecret, | ||
| plugins: [ | ||
| magicLink({ | ||
| sendMagicLink: async ({ url }) => { | ||
| capturedMagicLinkUrl = url; | ||
| }, | ||
| expiresIn: 60 * MAGIC_LINK_EXPIRY_MINUTES, | ||
| }), | ||
| ], | ||
| }); | ||
|
|
||
| await tempAuth.api.signInMagicLink({ | ||
| body: { | ||
| email: email.toLowerCase(), | ||
| callbackURL: callbackURL, | ||
| newUserCallbackURL: callbackURL, | ||
| }, | ||
| headers: new Headers(), | ||
| }); | ||
|
|
||
| if (!capturedMagicLinkUrl) { | ||
| throw createDomainError( | ||
| SYSTEM_ERRORS.INTERNAL, | ||
| { service: 'magic-link' }, | ||
| 'Failed to generate magic link URL', | ||
| ); | ||
| } | ||
|
|
||
| if (c.env.ENVIRONMENT !== 'production') { | ||
| console.log('[Email] Project invitation magic link URL:', capturedMagicLinkUrl); | ||
| } | ||
|
|
||
| const { getProjectInvitationEmailHtml, getProjectInvitationEmailText } = | ||
| await import('@/auth/emailTemplates.js'); | ||
| const { escapeHtml } = await import('@/lib/escapeHtml.js'); | ||
| ], | ||
| }); | ||
|
|
||
| const projectName = project?.name || 'Unknown Project'; | ||
| const inviterName = inviter?.displayName || inviter?.name || inviter?.email || 'Someone'; | ||
| await tempAuth.api.signInMagicLink({ | ||
| body: { | ||
| email: email.toLowerCase(), | ||
| callbackURL: callbackURL, | ||
| newUserCallbackURL: callbackURL, | ||
| }, | ||
| headers: new Headers(), | ||
| }); | ||
|
|
||
| const emailHtml = getProjectInvitationEmailHtml({ | ||
| projectName, | ||
| inviterName, | ||
| invitationUrl: capturedMagicLinkUrl, | ||
| role, | ||
| }); | ||
| const emailText = getProjectInvitationEmailText({ | ||
| projectName, | ||
| inviterName, | ||
| invitationUrl: capturedMagicLinkUrl, | ||
| role, | ||
| }); | ||
|
|
||
| const safeProjectName = escapeHtml(projectName); | ||
|
|
||
| const queueId = c.env.EMAIL_QUEUE.idFromName('default'); | ||
| const queue = c.env.EMAIL_QUEUE.get(queueId); | ||
| await queue.fetch( | ||
| new Request('https://internal/enqueue', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ | ||
| to: email, | ||
| subject: `You're Invited to "${safeProjectName}" - CoRATES`, | ||
| html: emailHtml, | ||
| text: emailText, | ||
| }), | ||
| }), | ||
| if (!capturedMagicLinkUrl) { | ||
| throw createDomainError( | ||
| SYSTEM_ERRORS.INTERNAL, | ||
| { service: 'magic-link' }, | ||
| 'Failed to generate magic link URL', | ||
| ); | ||
| } catch (err) { | ||
| console.error('Failed to queue invitation email:', err); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "CONFIG_MISSING|INTERNAL" packages/shared/src/errors/Repository: InfinityBowman/corates
Length of output: 218
🏁 Script executed:
rg -n "SYSTEM_ERRORS" packages/shared/src/errors/ -A 5 -B 2Repository: InfinityBowman/corates
Length of output: 3189
🏁 Script executed:
wc -l packages/workers/src/routes/orgs/invitations.jsRepository: InfinityBowman/corates
Length of output: 118
🏁 Script executed:
sed -n '163,204p' packages/shared/src/errors/domains/domain.tsRepository: InfinityBowman/corates
Length of output: 1144
🏁 Script executed:
rg "CONFIG_MISSING" packages/shared/Repository: InfinityBowman/corates
Length of output: 48
🏁 Script executed:
sed -n '560,615p' packages/workers/src/routes/orgs/invitations.jsRepository: InfinityBowman/corates
Length of output: 1736
🏁 Script executed:
rg "CONFIG_MISSING|\.INTERNAL" packages/workers/src/Repository: InfinityBowman/corates
Length of output: 3184
🏁 Script executed:
rg "SYSTEM_ERRORS\." packages/workers/src/routes/ | head -20Repository: InfinityBowman/corates
Length of output: 2132
Fix incorrect error constant references—SYSTEM_ERRORS.CONFIG_MISSING and SYSTEM_ERRORS.INTERNAL do not exist.
Lines 567 and 607 reference error constants that don't exist in the SYSTEM_ERRORS definition. The correct constant is SYSTEM_ERRORS.INTERNAL_ERROR (not SYSTEM_ERRORS.INTERNAL), and SYSTEM_ERRORS.CONFIG_MISSING is not defined anywhere. Replace both with appropriate existing constants from the shared error definitions:
- For missing configuration: Use
SYSTEM_ERRORS.INTERNAL_ERRORwith a message describing the missing AUTH_SECRET - For failed magic link generation: Use
SYSTEM_ERRORS.INTERNAL_ERROR
Additionally, the dynamic imports for better-auth, better-auth/plugins, better-auth/adapters/drizzle, drizzle-orm/d1, and schema on each invitation creation may have performance implications in cold-start scenarios. Consider moving these to module level or using a lazy initialization pattern if performance testing indicates an issue.
| // Insert into mediaFiles table | ||
| const mediaFileId = crypto.randomUUID(); | ||
| try { | ||
| await db.insert(mediaFiles).values({ | ||
| id: mediaFileId, | ||
| filename: uniqueFileName, | ||
| originalName: originalFileName, | ||
| fileType: 'application/pdf', | ||
| fileSize: pdfData.byteLength, | ||
| uploadedBy: user.id, | ||
| bucketKey: key, | ||
| orgId, | ||
| projectId, | ||
| studyId, | ||
| createdAt: new Date(), | ||
| }); | ||
| } catch (error) { | ||
| console.error('PDF upload error:', error); | ||
| const uploadError = createDomainError( | ||
| FILE_ERRORS.UPLOAD_FAILED, | ||
| { operation: 'upload_pdf', originalError: error.message }, | ||
| error.message, | ||
| ); | ||
| return c.json(uploadError, uploadError.statusCode); | ||
| } catch (dbError) { | ||
| // Log error but don't fail the request (R2 object exists, can be cleaned up later) | ||
| console.error('Failed to insert mediaFiles record after R2 upload:', dbError); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| return c.json({ | ||
| success: true, | ||
| id: mediaFileId, | ||
| key, | ||
| fileName: uniqueFileName, | ||
| originalFileName: originalFileName !== uniqueFileName ? originalFileName : undefined, | ||
| size: pdfData.byteLength, | ||
| }); |
There was a problem hiding this comment.
Critical: DB insert failure returns invalid ID to client.
If the database insert fails (lines 670-673), the error is logged but the handler continues and returns the mediaFileId that was never persisted. This creates data integrity issues:
- Client receives an ID that doesn't exist in the database
- Subsequent list/download/delete operations will fail or behave incorrectly
- R2 object becomes orphaned without a corresponding DB record
This contradicts the "database is source of truth" principle applied in the delete handler.
Proposed fix: fail the upload if DB insert fails
// Insert into mediaFiles table
const mediaFileId = crypto.randomUUID();
- try {
- await db.insert(mediaFiles).values({
- id: mediaFileId,
- filename: uniqueFileName,
- originalName: originalFileName,
- fileType: 'application/pdf',
- fileSize: pdfData.byteLength,
- uploadedBy: user.id,
- bucketKey: key,
- orgId,
- projectId,
- studyId,
- createdAt: new Date(),
- });
- } catch (dbError) {
- // Log error but don't fail the request (R2 object exists, can be cleaned up later)
- console.error('Failed to insert mediaFiles record after R2 upload:', dbError);
- }
+ await db.insert(mediaFiles).values({
+ id: mediaFileId,
+ filename: uniqueFileName,
+ originalName: originalFileName,
+ fileType: 'application/pdf',
+ fileSize: pdfData.byteLength,
+ uploadedBy: user.id,
+ bucketKey: key,
+ orgId,
+ projectId,
+ studyId,
+ createdAt: new Date(),
+ });
+ // Note: If DB insert fails, the outer catch will handle it.
+ // The orphaned R2 object can be cleaned up by a background job.
return c.json({
success: true,Alternatively, if you want to handle partial success, consider deleting the R2 object on DB failure or returning a different response structure that indicates partial success without an invalid ID.
🤖 Prompt for AI Agents
In @packages/workers/src/routes/orgs/pdfs.js around lines 654 - 682, The handler
currently logs DB insert failures but still returns the generated mediaFileId
(mediaFileId) after the R2 upload; change the catch around
db.insert(mediaFiles).values(...) so that on error you either (A) delete the
uploaded R2 object identified by bucket key (variable key / R2 client used
earlier) and then return a 5xx error response instead of proceeding to return
c.json, or (B) immediately return a 5xx error (do not return mediaFileId) so the
client does not receive an unpersisted ID; ensure the catch does not swallow the
error and that any cleanup (R2 deletion) is awaited before responding and that
the return c.json block only runs when the insert succeeded.
| return c.json( | ||
| { | ||
| id: projectId, | ||
| name, | ||
| description, | ||
| orgId, | ||
| role: 'owner', | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| }, | ||
| 201, | ||
| ); |
There was a problem hiding this comment.
Response missing createdBy field declared in schema.
The ProjectSchema at line 73 declares createdBy: z.string().nullable(), but the success response at lines 456-466 doesn't include createdBy. This could cause OpenAPI validation errors in strict clients.
Proposed fix
return c.json(
{
id: projectId,
name,
description,
orgId,
role: 'owner',
createdAt: now,
updatedAt: now,
+ createdBy: authUser.id,
},
201,
);📝 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.
| return c.json( | |
| { | |
| id: projectId, | |
| name, | |
| description, | |
| orgId, | |
| role: 'owner', | |
| createdAt: now, | |
| updatedAt: now, | |
| }, | |
| 201, | |
| ); | |
| return c.json( | |
| { | |
| id: projectId, | |
| name, | |
| description, | |
| orgId, | |
| role: 'owner', | |
| createdAt: now, | |
| updatedAt: now, | |
| createdBy: authUser.id, | |
| }, | |
| 201, | |
| ); |
🤖 Prompt for AI Agents
In @packages/workers/src/routes/orgs/projects.js around lines 456 - 467, The
JSON success response for the project creation is missing the createdBy field
required by ProjectSchema; update the returned object (the response that
currently includes id, name, description, orgId, role, createdAt, updatedAt) to
include a createdBy property that matches the schema (a nullable string) — set
it to the current user id if available (e.g., use the existing auth/user id
variable in the handler) or explicitly null if no user id is present so the
response conforms to ProjectSchema.
| const newProject = { | ||
| id: projectId, | ||
| name, | ||
| description, | ||
| role: 'owner', | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| }; | ||
|
|
||
| return c.json(newProject, 201); |
There was a problem hiding this comment.
Response returns untrimmed values while DB stores trimmed versions.
Lines 306-307 insert name.trim() and description?.trim() || null into the database, but the response object at lines 361-362 returns the original untrimmed values. This creates an inconsistency between what the client receives and what's actually stored.
🐛 Proposed fix
const newProject = {
id: projectId,
- name,
- description,
+ name: name.trim(),
+ description: description?.trim() || null,
role: 'owner',
createdAt: now,
updatedAt: now,
};🤖 Prompt for AI Agents
In @packages/workers/src/routes/projects.js around lines 359 - 368, The response
object newProject returns the original untrimmed name and description while the
DB write uses name.trim() and description?.trim() || null; update the newProject
construction to use the same trimmed values (e.g., use trimmedName = name.trim()
and trimmedDescription = description?.trim() || null or inline name.trim() /
description?.trim() || null) so the API response matches what is stored, keeping
id/projectId, role, createdAt and updatedAt unchanged.
2 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.