use centralized error codes object now#111
Conversation
|
Warning Rate limit exceeded@InfinityBowman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds a typed shared error package and centralizes error handling across frontend and backend: new Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Web UI
participant WebErr as Web Error Utils
participant API as Workers API
participant DB as Database
Browser->>API: HTTP request (e.g., POST/GET)
API->>DB: perform operation
DB-->>API: result or error
alt API returns DomainError JSON (non-ok)
API-->>Browser: 4xx/5xx DomainError {code,message,statusCode,details}
Browser->>WebErr: handleFetchError(response)
WebErr-->>Browser: DomainError object
Browser->>Browser: handleError -> set UI state / showToast / navigate
else Network/transport failure
API-->>Browser: network error (reject)
Browser->>WebErr: parseError / normalizeError(Error)
WebErr-->>Browser: TransportError (TRANSPORT_*)
Browser->>Browser: handleError -> show global error/toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | 9e6e727 | Commit Preview URL | Dec 20 2025, 03:47 AM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/web/src/components/billing/BillingPage.jsx (1)
29-40: Loading state may not reset if handleError throws.The setPortalLoading(false) on line 38 is outside the finally block, so it won't execute if handleError throws an error. Although handleError is designed to catch errors internally, defensive coding suggests using a finally block for guaranteed cleanup.
Recommended fix
const handleManageSubscription = async () => { setPortalLoading(true); try { await redirectToPortal(); } catch (error) { const { handleError } = await import('@/lib/error-utils.js'); await handleError(error, { toastTitle: 'Portal Error', }); - setPortalLoading(false); + } finally { + setPortalLoading(false); } };packages/web/src/components/billing/PricingTable.jsx (1)
17-30: Loading state may not reset if handleError throws.The setLoadingTier(null) on line 28 is outside a finally block, so it won't execute if handleError throws an error. Although handleError is designed to catch errors internally, defensive coding suggests using a finally block for guaranteed cleanup.
Recommended fix
const handleUpgrade = async tier => { if (tier === 'free' || tier === currentTier()) return; setLoadingTier(tier); try { await redirectToCheckout(tier, billingInterval()); } catch (error) { const { handleError } = await import('@/lib/error-utils.js'); await handleError(error, { toastTitle: 'Checkout Error', }); - setLoadingTier(null); + } finally { + setLoadingTier(null); } };packages/web/src/components/profile-ui/MergeAccountsDialog.jsx (1)
114-132: Inconsistent error handling migration.The
handleCompleteMergefunction still uses the old error handling pattern (setError(err.message)at line 127), whilehandleSendCodeandhandleVerifyCodehave been migrated to use the centralizedhandleError. This inconsistency should be addressed for uniformity.Suggested fix
async function handleCompleteMerge() { setStep(STEPS.MERGING); setLoading(true); setError(null); try { const result = await completeMerge(mergeToken()); setStep(STEPS.SUCCESS); const linkedInfo = result.mergedProviders.length ? `Linked: ${result.mergedProviders.join(', ')}` : ''; showToast.success('Accounts Merged', `Successfully merged accounts. ${linkedInfo}`); props.onSuccess?.(); } catch (err) { - setError(err.message); + const { handleError } = await import('@/lib/error-utils.js'); + await handleError(err, { + setError, + showToast: false, + }); setStep(STEPS.CONFIRM); } finally { setLoading(false); } }packages/web/src/components/auth-ui/SignIn.jsx (1)
64-97: Inconsistent error handling migration.While the password sign-in flow has been migrated to use centralized
handleError(lines 129-133), the OAuth sign-in error handlers for Google (lines 75-79) and ORCID (lines 92-96) still use the old pattern withconsole.errorand directsetErrorcalls. This creates inconsistency in error handling and UX.Suggested fix
async function handleGoogleSignIn() { setGoogleLoading(true); setError(''); try { // Mark as OAuth signup in case this is a new user who needs to complete profile localStorage.setItem('oauthSignup', 'true'); // Redirect to complete-profile which will check if profile is complete // and redirect to dashboard if so await signinWithGoogle('/complete-profile'); } catch (err) { - console.error('Google sign-in error:', err); - setError('Failed to sign in with Google. Please try again.'); + await handleError(err, { + setError, + showToast: false, + toastTitle: 'Failed to sign in with Google', + }); localStorage.removeItem('oauthSignup'); setGoogleLoading(false); } } async function handleOrcidSignIn() { setOrcidLoading(true); setError(''); try { // Mark as OAuth signup in case this is a new user who needs to complete profile localStorage.setItem('oauthSignup', 'true'); // Redirect to complete-profile which will check if profile is complete await signinWithOrcid('/complete-profile'); } catch (err) { - console.error('ORCID sign-in error:', err); - setError('Failed to sign in with ORCID. Please try again.'); + await handleError(err, { + setError, + showToast: false, + toastTitle: 'Failed to sign in with ORCID', + }); localStorage.removeItem('oauthSignup'); setOrcidLoading(false); } }
🧹 Nitpick comments (8)
packages/web/src/constants/errors.js (2)
217-223: Consider adding input validation.The getErrorMessage function doesn't validate whether the errorCode parameter is a valid key in ERROR_CODES before attempting to access it. While the fallback handles undefined cases, explicitly checking could improve clarity.
Optional enhancement
export function getErrorMessage(errorCode) { if (!errorCode) { return ERROR_CODES.UNKNOWN_ERROR.message; } - const errorDef = ERROR_CODES[errorCode]; - return errorDef ? errorDef.message : ERROR_CODES.UNKNOWN_ERROR.message; + const errorDef = ERROR_CODES[errorCode]; + if (!errorDef) { + console.warn(`Unknown error code: ${errorCode}`); + return ERROR_CODES.UNKNOWN_ERROR.message; + } + return errorDef.message; }
230-237: Inconsistent pattern in ACCESS_DENIED_ERRORS array.The array mixes string literals (lines 231-234) with ERROR_CODES references (lines 235-236). For consistency and maintainability, consider using ERROR_CODES references throughout or documenting why string literals are preserved.
packages/web/src/components/auth-ui/SignUp.jsx (1)
14-14: Consider using dynamic import for consistency.This file uses a static import of
handleError, while other files in this PR use dynamic imports (await import('@/lib/error-utils.js')). For consistency across the codebase and to avoid loading error handling utilities until they're needed, consider switching to dynamic imports in the catch blocks.Suggested refactor for consistency
-import { handleError } from '@/lib/error-utils.js';Then in the error handlers (lines 69-72 and 89-92):
} catch (err) { console.error('Google sign-up error:', err); + const { handleError } = await import('@/lib/error-utils.js'); await handleError(err, { setError, showToast: false, });packages/web/src/components/auth-ui/TwoFactorVerify.jsx (1)
7-7: Consider using dynamic import for consistency.Similar to SignUp.jsx, this file uses a static import while most other files in this PR use dynamic imports. Consider switching to a dynamic import in the catch block for consistency and to defer loading the error utilities until needed.
Suggested refactor for consistency
-import { handleError } from '@/lib/error-utils.js';Then in the error handler (lines 46-49):
} catch (err) { + const { handleError } = await import('@/lib/error-utils.js'); await handleError(err, { setError, showToast: false, });packages/web/src/components/project-ui/add-studies/DoiLookupSection.jsx (1)
132-135: LGTM - Error handling correctly integrated.The centralized error handler properly replaces the inline error handling for manual PDF uploads.
Optional: Consider using a more specific toast title like 'PDF Upload Failed' instead of the generic 'Error' for better user clarity.
packages/web/src/components/profile-ui/GoogleDriveSettings.jsx (1)
36-39: LGTM - Centralized error handling correctly applied.Both error handlers properly delegate to the centralized error utility, replacing the previous console-only error logging with user-facing error messages.
Optional: Consider using more specific toast titles like 'Connection Failed' (line 38) and 'Disconnect Failed' (line 58) instead of the generic 'Error' for better user clarity.
Also applies to: 56-59
packages/web/src/components/project-ui/overview-tab/AddMemberModal.jsx (1)
5-5: Consider dynamic import for consistency.This file uses a static import of
handleFetchError, while other files in this PR use dynamic imports (await import('@/lib/error-utils.js')) to avoid potential circular dependencies. If this module doesn't have circular dependency concerns, the static import is fine and more efficient.packages/web/src/lib/error-utils.js (1)
168-185: Better Auth error handling may not trigger as expected.The check for
error.error(lines 169-185) is inside theif (error instanceof Error)block. Standard JavaScriptErrorobjects don't have an.errorproperty. If Better Auth returns a plain object with anerrorproperty (not an Error instance), this code path won't be reached becauseinstanceof Errorwill be false.Consider moving this check outside the
instanceof Errorblock or adding a separate branch to handle plain objects with the Better Auth format.Proposed refactor
export function parseError(error) { + // Handle Better Auth error format: { error: { status: number, message: string } } + if (error && typeof error === 'object' && error.error && typeof error.error === 'object') { + if (error.error.status === 429) { + return { + code: 'RATE_LIMITED', + message: getErrorMessage('RATE_LIMITED'), + }; + } + if (error.error.status === 401) { + return { + code: 'AUTH_REQUIRED', + message: getErrorMessage('AUTH_REQUIRED'), + }; + } + if (error.error.message) { + return parseError(error.error.message); + } + } + // Handle Error objects if (error instanceof Error) { const msg = error.message?.toLowerCase() || ''; // ... rest of Error handling ... - - // Better Auth error format: { error: { status: number, message: string } } - if (error.error && typeof error.error === 'object') { - if (error.error.status === 429) { - return { - code: 'RATE_LIMITED', - message: getErrorMessage('RATE_LIMITED'), - }; - } - if (error.error.status === 401) { - return { - code: 'AUTH_REQUIRED', - message: getErrorMessage('AUTH_REQUIRED'), - }; - } - if (error.error.message) { - return parseError(error.error.message); - } - }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
packages/web/src/api/auth-client.js(2 hunks)packages/web/src/api/better-auth-store.js(1 hunks)packages/web/src/components/admin-ui/UserTable.jsx(3 hunks)packages/web/src/components/auth-ui/CompleteProfile.jsx(2 hunks)packages/web/src/components/auth-ui/MagicLinkForm.jsx(3 hunks)packages/web/src/components/auth-ui/ResetPassword.jsx(3 hunks)packages/web/src/components/auth-ui/SignIn.jsx(2 hunks)packages/web/src/components/auth-ui/SignUp.jsx(3 hunks)packages/web/src/components/auth-ui/TwoFactorVerify.jsx(2 hunks)packages/web/src/components/billing/BillingPage.jsx(1 hunks)packages/web/src/components/billing/PricingTable.jsx(1 hunks)packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx(1 hunks)packages/web/src/components/checklist-ui/CreateLocalChecklist.jsx(1 hunks)packages/web/src/components/checklist-ui/LocalChecklistView.jsx(1 hunks)packages/web/src/components/profile-ui/GoogleDriveSettings.jsx(2 hunks)packages/web/src/components/profile-ui/MergeAccountsDialog.jsx(2 hunks)packages/web/src/components/profile-ui/ProfilePage.jsx(2 hunks)packages/web/src/components/profile-ui/TwoFactorSetup.jsx(3 hunks)packages/web/src/components/project-ui/CreateProjectForm.jsx(2 hunks)packages/web/src/components/project-ui/ProjectDashboard.jsx(1 hunks)packages/web/src/components/project-ui/ProjectHeader.jsx(3 hunks)packages/web/src/components/project-ui/add-studies/DoiLookupSection.jsx(1 hunks)packages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsx(2 hunks)packages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsx(2 hunks)packages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsx(1 hunks)packages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsx(3 hunks)packages/web/src/components/project-ui/overview-tab/AddMemberModal.jsx(3 hunks)packages/web/src/components/project-ui/overview-tab/OverviewTab.jsx(1 hunks)packages/web/src/constants/errors.js(1 hunks)packages/web/src/lib/error-utils.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/billing/BillingPage.jsxpackages/web/src/components/profile-ui/GoogleDriveSettings.jsxpackages/web/src/components/profile-ui/MergeAccountsDialog.jsxpackages/web/src/components/profile-ui/ProfilePage.jsxpackages/web/src/components/auth-ui/MagicLinkForm.jsxpackages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsxpackages/web/src/constants/errors.jspackages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/admin-ui/UserTable.jsxpackages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsxpackages/web/src/api/better-auth-store.jspackages/web/src/components/auth-ui/SignIn.jsxpackages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsxpackages/web/src/lib/error-utils.jspackages/web/src/api/auth-client.jspackages/web/src/components/profile-ui/TwoFactorSetup.jsxpackages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsxpackages/web/src/components/project-ui/overview-tab/OverviewTab.jsxpackages/web/src/components/auth-ui/SignUp.jsxpackages/web/src/components/auth-ui/CompleteProfile.jsxpackages/web/src/components/checklist-ui/CreateLocalChecklist.jsxpackages/web/src/components/auth-ui/ResetPassword.jsxpackages/web/src/components/project-ui/add-studies/DoiLookupSection.jsxpackages/web/src/components/auth-ui/TwoFactorVerify.jsxpackages/web/src/components/project-ui/ProjectDashboard.jsxpackages/web/src/components/billing/PricingTable.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/overview-tab/AddMemberModal.jsxpackages/web/src/components/project-ui/CreateProjectForm.jsx
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx,js,ts}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Import stores directly where needed instead of passing values through multiple components
When you need to compute a value based on props or state in SolidJS, usecreateMemoto ensure it updates reactively
For complex state or state objects in SolidJS, use Solid'screateStorefor better performance and reactivity
You may create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Files:
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/billing/BillingPage.jsxpackages/web/src/components/profile-ui/GoogleDriveSettings.jsxpackages/web/src/components/profile-ui/MergeAccountsDialog.jsxpackages/web/src/components/profile-ui/ProfilePage.jsxpackages/web/src/components/auth-ui/MagicLinkForm.jsxpackages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsxpackages/web/src/constants/errors.jspackages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/admin-ui/UserTable.jsxpackages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsxpackages/web/src/api/better-auth-store.jspackages/web/src/components/auth-ui/SignIn.jsxpackages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsxpackages/web/src/lib/error-utils.jspackages/web/src/api/auth-client.jspackages/web/src/components/profile-ui/TwoFactorSetup.jsxpackages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsxpackages/web/src/components/project-ui/overview-tab/OverviewTab.jsxpackages/web/src/components/auth-ui/SignUp.jsxpackages/web/src/components/auth-ui/CompleteProfile.jsxpackages/web/src/components/checklist-ui/CreateLocalChecklist.jsxpackages/web/src/components/auth-ui/ResetPassword.jsxpackages/web/src/components/project-ui/add-studies/DoiLookupSection.jsxpackages/web/src/components/auth-ui/TwoFactorVerify.jsxpackages/web/src/components/project-ui/ProjectDashboard.jsxpackages/web/src/components/billing/PricingTable.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/overview-tab/AddMemberModal.jsxpackages/web/src/components/project-ui/CreateProjectForm.jsx
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/billing/BillingPage.jsxpackages/web/src/components/profile-ui/GoogleDriveSettings.jsxpackages/web/src/components/profile-ui/MergeAccountsDialog.jsxpackages/web/src/components/profile-ui/ProfilePage.jsxpackages/web/src/components/auth-ui/MagicLinkForm.jsxpackages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsxpackages/web/src/constants/errors.jspackages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/admin-ui/UserTable.jsxpackages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsxpackages/web/src/api/better-auth-store.jspackages/web/src/components/auth-ui/SignIn.jsxpackages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsxpackages/web/src/lib/error-utils.jspackages/web/src/api/auth-client.jspackages/web/src/components/profile-ui/TwoFactorSetup.jsxpackages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsxpackages/web/src/components/project-ui/overview-tab/OverviewTab.jsxpackages/web/src/components/auth-ui/SignUp.jsxpackages/web/src/components/auth-ui/CompleteProfile.jsxpackages/web/src/components/checklist-ui/CreateLocalChecklist.jsxpackages/web/src/components/auth-ui/ResetPassword.jsxpackages/web/src/components/project-ui/add-studies/DoiLookupSection.jsxpackages/web/src/components/auth-ui/TwoFactorVerify.jsxpackages/web/src/components/project-ui/ProjectDashboard.jsxpackages/web/src/components/billing/PricingTable.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/overview-tab/AddMemberModal.jsxpackages/web/src/components/project-ui/CreateProjectForm.jsx
packages/web/src/components/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/components/**/*.{jsx,tsx}: Use responsive design principles for UI components
Use Zag.js for UI components and design system
Zag components exist inpackages/web/src/components/zag/*and should be reused; check the README.md in that folder for a list of existing components before adding new components and when debugging
Components should receive at most 1–5 props, and only for local configuration, not shared state
If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context
Components should be lean and focused and should not implement business logic; move business logic into stores, utilities, or primitives
Never have a component act as a 'God component' coordinating multiple large concerns
Files:
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/billing/BillingPage.jsxpackages/web/src/components/profile-ui/GoogleDriveSettings.jsxpackages/web/src/components/profile-ui/MergeAccountsDialog.jsxpackages/web/src/components/profile-ui/ProfilePage.jsxpackages/web/src/components/auth-ui/MagicLinkForm.jsxpackages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsxpackages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/admin-ui/UserTable.jsxpackages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsxpackages/web/src/components/auth-ui/SignIn.jsxpackages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsxpackages/web/src/components/profile-ui/TwoFactorSetup.jsxpackages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsxpackages/web/src/components/project-ui/overview-tab/OverviewTab.jsxpackages/web/src/components/auth-ui/SignUp.jsxpackages/web/src/components/auth-ui/CompleteProfile.jsxpackages/web/src/components/checklist-ui/CreateLocalChecklist.jsxpackages/web/src/components/auth-ui/ResetPassword.jsxpackages/web/src/components/project-ui/add-studies/DoiLookupSection.jsxpackages/web/src/components/auth-ui/TwoFactorVerify.jsxpackages/web/src/components/project-ui/ProjectDashboard.jsxpackages/web/src/components/billing/PricingTable.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/overview-tab/AddMemberModal.jsxpackages/web/src/components/project-ui/CreateProjectForm.jsx
packages/web/src/**/*.{jsx,tsx,js,ts,css}
📄 CodeRabbit inference engine (.cursorrules)
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Files:
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/billing/BillingPage.jsxpackages/web/src/components/profile-ui/GoogleDriveSettings.jsxpackages/web/src/components/profile-ui/MergeAccountsDialog.jsxpackages/web/src/components/profile-ui/ProfilePage.jsxpackages/web/src/components/auth-ui/MagicLinkForm.jsxpackages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsxpackages/web/src/constants/errors.jspackages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/admin-ui/UserTable.jsxpackages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsxpackages/web/src/api/better-auth-store.jspackages/web/src/components/auth-ui/SignIn.jsxpackages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsxpackages/web/src/lib/error-utils.jspackages/web/src/api/auth-client.jspackages/web/src/components/profile-ui/TwoFactorSetup.jsxpackages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsxpackages/web/src/components/project-ui/overview-tab/OverviewTab.jsxpackages/web/src/components/auth-ui/SignUp.jsxpackages/web/src/components/auth-ui/CompleteProfile.jsxpackages/web/src/components/checklist-ui/CreateLocalChecklist.jsxpackages/web/src/components/auth-ui/ResetPassword.jsxpackages/web/src/components/project-ui/add-studies/DoiLookupSection.jsxpackages/web/src/components/auth-ui/TwoFactorVerify.jsxpackages/web/src/components/project-ui/ProjectDashboard.jsxpackages/web/src/components/billing/PricingTable.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/overview-tab/AddMemberModal.jsxpackages/web/src/components/project-ui/CreateProjectForm.jsx
packages/web/src/components/**/*.{jsx,tsx,js}
📄 CodeRabbit inference engine (.cursorrules)
Group related components in subdirectories with an
index.jsbarrel export
Files:
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/billing/BillingPage.jsxpackages/web/src/components/profile-ui/GoogleDriveSettings.jsxpackages/web/src/components/profile-ui/MergeAccountsDialog.jsxpackages/web/src/components/profile-ui/ProfilePage.jsxpackages/web/src/components/auth-ui/MagicLinkForm.jsxpackages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsxpackages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/admin-ui/UserTable.jsxpackages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsxpackages/web/src/components/auth-ui/SignIn.jsxpackages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsxpackages/web/src/components/profile-ui/TwoFactorSetup.jsxpackages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsxpackages/web/src/components/project-ui/overview-tab/OverviewTab.jsxpackages/web/src/components/auth-ui/SignUp.jsxpackages/web/src/components/auth-ui/CompleteProfile.jsxpackages/web/src/components/checklist-ui/CreateLocalChecklist.jsxpackages/web/src/components/auth-ui/ResetPassword.jsxpackages/web/src/components/project-ui/add-studies/DoiLookupSection.jsxpackages/web/src/components/auth-ui/TwoFactorVerify.jsxpackages/web/src/components/project-ui/ProjectDashboard.jsxpackages/web/src/components/billing/PricingTable.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/overview-tab/AddMemberModal.jsxpackages/web/src/components/project-ui/CreateProjectForm.jsx
packages/web/src/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx}: Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
Do not destructure props in SolidJS components as it breaks reactivity. Instead, access props directly from the props object or wrap them in a function to ensure they are always up-to-date
Files:
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/billing/BillingPage.jsxpackages/web/src/components/profile-ui/GoogleDriveSettings.jsxpackages/web/src/components/profile-ui/MergeAccountsDialog.jsxpackages/web/src/components/profile-ui/ProfilePage.jsxpackages/web/src/components/auth-ui/MagicLinkForm.jsxpackages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsxpackages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/admin-ui/UserTable.jsxpackages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsxpackages/web/src/components/auth-ui/SignIn.jsxpackages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsxpackages/web/src/components/profile-ui/TwoFactorSetup.jsxpackages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsxpackages/web/src/components/project-ui/overview-tab/OverviewTab.jsxpackages/web/src/components/auth-ui/SignUp.jsxpackages/web/src/components/auth-ui/CompleteProfile.jsxpackages/web/src/components/checklist-ui/CreateLocalChecklist.jsxpackages/web/src/components/auth-ui/ResetPassword.jsxpackages/web/src/components/project-ui/add-studies/DoiLookupSection.jsxpackages/web/src/components/auth-ui/TwoFactorVerify.jsxpackages/web/src/components/project-ui/ProjectDashboard.jsxpackages/web/src/components/billing/PricingTable.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/overview-tab/AddMemberModal.jsxpackages/web/src/components/project-ui/CreateProjectForm.jsx
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
UsecreateMemofor derived values to ensure they update reactively
Files:
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/billing/BillingPage.jsxpackages/web/src/components/profile-ui/GoogleDriveSettings.jsxpackages/web/src/components/profile-ui/MergeAccountsDialog.jsxpackages/web/src/components/profile-ui/ProfilePage.jsxpackages/web/src/components/auth-ui/MagicLinkForm.jsxpackages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsxpackages/web/src/constants/errors.jspackages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/admin-ui/UserTable.jsxpackages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsxpackages/web/src/api/better-auth-store.jspackages/web/src/components/auth-ui/SignIn.jsxpackages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsxpackages/web/src/lib/error-utils.jspackages/web/src/api/auth-client.jspackages/web/src/components/profile-ui/TwoFactorSetup.jsxpackages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsxpackages/web/src/components/project-ui/overview-tab/OverviewTab.jsxpackages/web/src/components/auth-ui/SignUp.jsxpackages/web/src/components/auth-ui/CompleteProfile.jsxpackages/web/src/components/checklist-ui/CreateLocalChecklist.jsxpackages/web/src/components/auth-ui/ResetPassword.jsxpackages/web/src/components/project-ui/add-studies/DoiLookupSection.jsxpackages/web/src/components/auth-ui/TwoFactorVerify.jsxpackages/web/src/components/project-ui/ProjectDashboard.jsxpackages/web/src/components/billing/PricingTable.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/overview-tab/AddMemberModal.jsxpackages/web/src/components/project-ui/CreateProjectForm.jsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/billing/BillingPage.jsxpackages/web/src/components/profile-ui/GoogleDriveSettings.jsxpackages/web/src/components/profile-ui/MergeAccountsDialog.jsxpackages/web/src/components/profile-ui/ProfilePage.jsxpackages/web/src/components/auth-ui/MagicLinkForm.jsxpackages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsxpackages/web/src/constants/errors.jspackages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/admin-ui/UserTable.jsxpackages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsxpackages/web/src/api/better-auth-store.jspackages/web/src/components/auth-ui/SignIn.jsxpackages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsxpackages/web/src/lib/error-utils.jspackages/web/src/api/auth-client.jspackages/web/src/components/profile-ui/TwoFactorSetup.jsxpackages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsxpackages/web/src/components/project-ui/overview-tab/OverviewTab.jsxpackages/web/src/components/auth-ui/SignUp.jsxpackages/web/src/components/auth-ui/CompleteProfile.jsxpackages/web/src/components/checklist-ui/CreateLocalChecklist.jsxpackages/web/src/components/auth-ui/ResetPassword.jsxpackages/web/src/components/project-ui/add-studies/DoiLookupSection.jsxpackages/web/src/components/auth-ui/TwoFactorVerify.jsxpackages/web/src/components/project-ui/ProjectDashboard.jsxpackages/web/src/components/billing/PricingTable.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/overview-tab/AddMemberModal.jsxpackages/web/src/components/project-ui/CreateProjectForm.jsx
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/components/**/*.{js,jsx,ts,tsx}: Use responsive design principles for UI components
Group related components in subdirectories with an index.js barrel export
Use Zag.js for UI components and design system
Zag component exist inpackages/web/src/components/zag/*and should be reused. Check the README.md in that folder for a list of existing components before adding new components and when debugging
Components should receive at most 1–5 props, and only for local configuration, not shared state. If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context
Do not destructure props in SolidJS components as it breaks reactivity. Instead, access props directly from the props object or wrap them in a function to ensure they are always up-to-date
Components should be lean and focused. They should not implement business logic; move that into stores, utilities, or primitives
Never have a component act as a God component coordinating multiple large concerns
Files:
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/billing/BillingPage.jsxpackages/web/src/components/profile-ui/GoogleDriveSettings.jsxpackages/web/src/components/profile-ui/MergeAccountsDialog.jsxpackages/web/src/components/profile-ui/ProfilePage.jsxpackages/web/src/components/auth-ui/MagicLinkForm.jsxpackages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsxpackages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/admin-ui/UserTable.jsxpackages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsxpackages/web/src/components/auth-ui/SignIn.jsxpackages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsxpackages/web/src/components/profile-ui/TwoFactorSetup.jsxpackages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsxpackages/web/src/components/project-ui/overview-tab/OverviewTab.jsxpackages/web/src/components/auth-ui/SignUp.jsxpackages/web/src/components/auth-ui/CompleteProfile.jsxpackages/web/src/components/checklist-ui/CreateLocalChecklist.jsxpackages/web/src/components/auth-ui/ResetPassword.jsxpackages/web/src/components/project-ui/add-studies/DoiLookupSection.jsxpackages/web/src/components/auth-ui/TwoFactorVerify.jsxpackages/web/src/components/project-ui/ProjectDashboard.jsxpackages/web/src/components/billing/PricingTable.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/overview-tab/AddMemberModal.jsxpackages/web/src/components/project-ui/CreateProjectForm.jsx
🧠 Learnings (9)
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts} : Import stores directly where needed instead of passing values through multiple components
Applied to files:
packages/web/src/components/billing/BillingPage.jsxpackages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/project-ui/ProjectDashboard.jsx
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts,css} : Ensure browser compatibility for all frontend code (Safari is usually problematic)
Applied to files:
packages/web/src/constants/errors.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Ensure browser compatibility for all frontend code (Safari is usually problematic)
Applied to files:
packages/web/src/constants/errors.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/components/**/*.{jsx,tsx} : Zag components exist in `packages/web/src/components/zag/*` and should be reused; check the README.md in that folder for a list of existing components before adding new components and when debugging
Applied to files:
packages/web/src/components/checklist-ui/LocalChecklistView.jsxpackages/web/src/components/checklist-ui/CreateLocalChecklist.jsx
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/web/src/api/better-auth-store.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/web/src/api/better-auth-store.jspackages/web/src/api/auth-client.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/web/src/stores/**/*.{js,ts} : Use Solid's `createStore` for complex state or state objects for better performance and reactivity
Applied to files:
packages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsx
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts} : For complex state or state objects in SolidJS, use Solid's `createStore` for better performance and reactivity
Applied to files:
packages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsxpackages/web/src/components/project-ui/CreateProjectForm.jsx
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts} : When you need to compute a value based on props or state in SolidJS, use `createMemo` to ensure it updates reactively
Applied to files:
packages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsx
🧬 Code graph analysis (27)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/billing/BillingPage.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/profile-ui/GoogleDriveSettings.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/profile-ui/MergeAccountsDialog.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/profile-ui/ProfilePage.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/auth-ui/MagicLinkForm.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/checklist-ui/LocalChecklistView.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/admin-ui/UserTable.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/api/better-auth-store.js (2)
packages/web/src/lib/error-utils.js (3)
parseApiError(15-94)response(267-267)parseError(102-206)packages/web/src/constants/errors.js (1)
getErrorMessage(217-223)
packages/web/src/components/auth-ui/SignIn.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/lib/error-utils.js (1)
packages/web/src/constants/errors.js (4)
mapBackendCodeToFrontend(208-210)getErrorMessage(217-223)ERROR_CODES(11-191)ERROR_CODES(11-191)
packages/web/src/api/auth-client.js (1)
packages/web/src/lib/error-utils.js (4)
parsedError(222-222)parsedError(270-270)parsedError(279-279)parseError(102-206)
packages/web/src/components/profile-ui/TwoFactorSetup.jsx (1)
packages/web/src/lib/error-utils.js (4)
parsedError(222-222)parsedError(270-270)parsedError(279-279)handleError(219-256)
packages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/project-ui/overview-tab/OverviewTab.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/auth-ui/SignUp.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/checklist-ui/CreateLocalChecklist.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/auth-ui/ResetPassword.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/project-ui/add-studies/DoiLookupSection.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/auth-ui/TwoFactorVerify.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/project-ui/ProjectDashboard.jsx (1)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)
packages/web/src/components/project-ui/ProjectHeader.jsx (2)
packages/web/src/lib/error-utils.js (1)
handleError(219-256)packages/web/src/components/auth-ui/CompleteProfile.jsx (1)
error(23-23)
packages/web/src/components/project-ui/overview-tab/AddMemberModal.jsx (1)
packages/web/src/lib/error-utils.js (2)
response(267-267)handleFetchError(265-287)
packages/web/src/components/project-ui/CreateProjectForm.jsx (1)
packages/web/src/lib/error-utils.js (2)
response(267-267)handleFetchError(265-287)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (33)
packages/web/src/components/project-ui/all-studies-tab/study-card/StudyPdfSection.jsx (1)
49-53: LGTM! Centralized error handling properly integrated.The dynamic import and handleError usage correctly delegates error presentation to the centralized utility while maintaining proper cleanup in the finally block.
packages/web/src/constants/errors.js (2)
11-191: LGTM! Well-structured centralized error codes.The ERROR_CODES object provides a clean mapping between frontend error codes and backend numeric codes with user-friendly messages. The structure is comprehensive and covers all major error categories.
196-210: LGTM! Efficient reverse lookup implementation.The BACKEND_CODE_MAP construction and mapBackendCodeToFrontend function provide an efficient way to translate backend numeric codes to frontend error constants.
packages/web/src/components/auth-ui/CompleteProfile.jsx (2)
137-140: LGTM! Proper error handling with local state update.The handleError call correctly passes setError to update local error state and disables toast (showToast: false), which is appropriate since the error is displayed inline via the ErrorMessage component.
9-9: No standardization needed—the codebase uses an intentional pattern wherehandleErroris statically imported at the top level in components that consistently use it, and dynamically imported within error catch blocks where it's conditionally needed. This context-aware approach optimizes load times by deferring error-handling utilities until necessary.Likely an incorrect or invalid review comment.
packages/web/src/api/auth-client.js (1)
9-9: LGTM! Clean integration with centralized error parsing.The parseError usage in the onError callback provides consistent error logging with structured error codes and messages, replacing the previous ad-hoc status code checking.
Also applies to: 19-20
packages/web/src/components/project-ui/ProjectDashboard.jsx (1)
112-115: LGTM! Centralized error handling properly integrated.The dynamic import and handleError usage correctly delegates error presentation to the centralized utility with an appropriate toast title for the delete operation.
packages/web/src/components/project-ui/all-studies-tab/EditPdfMetadataModal.jsx (1)
14-14: LGTM! Proper error handling with cleanup.The direct import of handleError and its usage in the catch block correctly delegates error presentation to the centralized utility, while the finally block ensures proper cleanup of the saving state.
Also applies to: 72-77
packages/web/src/components/checklist-ui/LocalChecklistView.jsx (1)
69-73: LGTM - Centralized error handling correctly integrated.The dynamic import and handleError invocation properly centralizes error handling while maintaining the existing UI state management (setError) and loading flow.
packages/web/src/components/admin-ui/UserTable.jsx (1)
81-85: LGTM - Consistent error handling across all operations.All three error handlers (unban/impersonate, ban, delete/revoke) correctly delegate to the centralized handleError utility while preserving the finally blocks for cleanup and maintaining the error state display in the UI.
Also applies to: 102-106, 126-130
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (1)
185-188: LGTM - Error handling aligns with upload failure UX.The centralized error handler with a custom toast title provides clear feedback for PDF upload failures while maintaining the cleanup logic for orphaned files.
packages/web/src/components/auth-ui/SignUp.jsx (1)
69-72: LGTM - Error handling correctly integrated.Both OAuth error handlers properly delegate to the centralized error utility, maintaining the error state and preventing duplicate toast notifications.
Also applies to: 89-92
packages/web/src/components/auth-ui/TwoFactorVerify.jsx (1)
46-49: LGTM - Centralized error handling correctly applied.The error handler properly delegates to the centralized utility, consolidating the previous inline error message handling into the shared error system.
packages/web/src/components/project-ui/overview-tab/OverviewTab.jsx (1)
74-77: LGTM - Well-structured error handling.The centralized error handler with a clear, specific toast title ('Remove Failed') provides good user feedback while maintaining the success toast behavior for the happy path.
packages/web/src/components/auth-ui/MagicLinkForm.jsx (1)
6-6: LGTM! Consistent migration to centralized error handling.The migration is well-executed with static import of
handleErrorand consistent usage across both error paths. TheshowToast: falseoption appropriately preserves the inline error display behavior.Also applies to: 44-47, 77-80
packages/web/src/components/project-ui/ProjectHeader.jsx (1)
4-5: LGTM! Clean migration to centralized error handling.The changes consistently replace direct toast calls with the centralized
handleError, using appropriatetoastTitleoptions for clear user feedback. The removal of theshowToastimport is correct.Also applies to: 28-30, 53-55
packages/web/src/components/checklist-ui/CreateLocalChecklist.jsx (1)
63-67: LGTM! Proper migration to centralized error handling.The error handling correctly uses dynamic import and delegates to
handleErrorwith appropriate options.packages/web/src/components/project-ui/all-studies-tab/AssignReviewersModal.jsx (1)
10-10: LGTM! Proper migration to centralized error handling.The changes correctly remove the unused
showToastimport and migrate to the centralizedhandleErrorwith an appropriatetoastTitleoption.Also applies to: 63-66
packages/web/src/components/auth-ui/ResetPassword.jsx (1)
8-8: LGTM! Consistent migration to centralized error handling.Both password reset flows have been properly migrated to use the centralized
handleErrorwith appropriate options to preserve inline error display behavior.Also applies to: 60-63, 171-174
packages/web/src/components/auth-ui/SignIn.jsx (1)
128-133: Proper migration with navigate option.The password sign-in error handling correctly uses
handleErrorwith thenavigateoption, which enables special-case navigation (e.g., for email verification). This is a good use of the centralized error handler's capabilities.packages/web/src/components/project-ui/CreateProjectForm.jsx (1)
94-129: Good use of handleFetchError wrapper.The migration correctly uses
handleFetchErrorto wrap the fetch call, which is appropriate for this use case. The catch block properly acknowledges that error handling is already performed byhandleFetchErrorbefore it re-throws.Note: This pattern differs from other files that use
handleErrordirectly in catch blocks.handleFetchErroris a wrapper that handles the response checking, error parsing, and user feedback, then throws for control flow. Both patterns are valid depending on the use case.packages/web/src/components/profile-ui/ProfilePage.jsx (2)
196-199: LGTM - Centralized error handling for avatar upload.The dynamic import of
handleErrorcorrectly delegates error presentation while preserving the optimistic update reversion. ThetoastTitleoption provides context-specific messaging.
226-229: LGTM - Centralized error handling for account deletion.Consistent pattern with the avatar upload error handling. The
setDeletingAccount(false)on line 230 correctly remains outside the error handler to reset UI state.packages/web/src/components/project-ui/google-drive/GoogleDrivePickerLauncher.jsx (3)
46-51: LGTM - Error handling for connection status check.The inline error display via
setErrorwithshowToast: falseis appropriate for this background check. The fallback tosetConnected(false)ensures graceful degradation.
73-78: LGTM - Error handling for connect flow.Correctly sets inline error state and re-throws for caller handling. The
showToast: falseprevents duplicate notifications since the UI displays the error inline.
106-111: LGTM - Error handling for picker opening.Consistent with the connect flow pattern. Re-throwing after handling allows callers to respond appropriately.
packages/web/src/components/project-ui/overview-tab/AddMemberModal.jsx (2)
58-74: LGTM - Search users with centralized error handling.The
handleFetchErrorwrapper correctly manages error state viasetErrorand suppresses toasts for inline display. The empty catch block is appropriate sincehandleFetchErrorthrows after handling.
91-114: LGTM - Add member with centralized error handling.Consistent pattern with the search flow. The wrapper handles error state and the empty catch allows the flow to continue to the
finallyblock for cleanup.packages/web/src/api/better-auth-store.js (1)
556-564: LGTM - Centralized API error parsing for auth fetch.The dynamic import avoids circular dependencies. Using
parseApiErrordirectly (instead ofhandleError) is appropriate for this lower-level auth store since it shouldn't trigger UI side effects like toasts.packages/web/src/components/profile-ui/TwoFactorSetup.jsx (1)
95-99: LGTM - Simple error handling for verification.Clean implementation that delegates to the centralized handler with an appropriate toast title.
packages/web/src/lib/error-utils.js (3)
1-8: LGTM!Clear module documentation and properly structured imports using aliases as per coding guidelines.
15-94: Well-structured API error parsing with good fallback chain.The function handles multiple error formats gracefully:
- Backend numeric codes mapped to frontend codes
- Direct frontend string codes
- Raw error messages
- HTTP status code fallbacks
The defensive
.catch(() => ({}))on line 17 prevents JSON parse failures from crashing the handler.
219-256: Clean unified error handler with good extensibility.The function provides a flexible API with sensible defaults. The destructuring of
showToastoption asshowToastOptioncorrectly avoids shadowing the importedshowToastfunction.
| // Re-throw if it's already been processed | ||
| if (err.message && !err.message.includes('Request failed with status')) { | ||
| throw err; | ||
| } | ||
| throw err; | ||
|
|
||
| // Handle unprocessed errors | ||
| const { parseError, getErrorMessage } = await import('@/lib/error-utils.js'); | ||
| const parsedError = parseError(err); | ||
|
|
||
| if (parsedError.code === 'AUTH_REQUIRED' || parsedError.code === 'AUTH_EXPIRED') { | ||
| setAuthError(getErrorMessage('AUTH_EXPIRED')); | ||
| } | ||
|
|
||
| throw new Error(parsedError.message); |
There was a problem hiding this comment.
Fragile string-based check for processed errors.
The condition !err.message.includes('Request failed with status') relies on specific error message text to detect if an error was already processed. This is brittle and could break if the message format changes elsewhere.
Consider a more robust approach using a custom error type or property:
Suggested improvement
} catch (err) {
- // Re-throw if it's already been processed
- if (err.message && !err.message.includes('Request failed with status')) {
- throw err;
- }
+ // If the error has already been parsed (has a message that's not a generic status error),
+ // we still want to check for auth errors before re-throwing
+ const { parseError, getErrorMessage } = await import('@/lib/error-utils.js');
+ const parsedError = parseError(err);
- // Handle unprocessed errors
- const { parseError, getErrorMessage } = await import('@/lib/error-utils.js');
- const parsedError = parseError(err);
-
- if (parsedError.code === 'AUTH_REQUIRED' || parsedError.code === 'AUTH_EXPIRED') {
- setAuthError(getErrorMessage('AUTH_EXPIRED'));
- }
+ if (parsedError.code === 'AUTH_REQUIRED' || parsedError.code === 'AUTH_EXPIRED') {
+ setAuthError(getErrorMessage('AUTH_EXPIRED'));
+ }
- throw new Error(parsedError.message);
+ throw new Error(parsedError.message);
}📝 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.
| // Re-throw if it's already been processed | |
| if (err.message && !err.message.includes('Request failed with status')) { | |
| throw err; | |
| } | |
| throw err; | |
| // Handle unprocessed errors | |
| const { parseError, getErrorMessage } = await import('@/lib/error-utils.js'); | |
| const parsedError = parseError(err); | |
| if (parsedError.code === 'AUTH_REQUIRED' || parsedError.code === 'AUTH_EXPIRED') { | |
| setAuthError(getErrorMessage('AUTH_EXPIRED')); | |
| } | |
| throw new Error(parsedError.message); | |
| } catch (err) { | |
| // If the error has already been parsed (has a message that's not a generic status error), | |
| // we still want to check for auth errors before re-throwing | |
| const { parseError, getErrorMessage } = await import('@/lib/error-utils.js'); | |
| const parsedError = parseError(err); | |
| if (parsedError.code === 'AUTH_REQUIRED' || parsedError.code === 'AUTH_EXPIRED') { | |
| setAuthError(getErrorMessage('AUTH_EXPIRED')); | |
| } | |
| throw new Error(parsedError.message); | |
| } |
| const { handleError } = await import('@/lib/error-utils.js'); | ||
| const parsedError = await handleError(err, { | ||
| showToast: true, | ||
| toastTitle: '2FA Setup Failed', | ||
| }); | ||
| // Check if error is about missing/invalid password - close setup and show message | ||
| if (message.toLowerCase().includes('password') || message.toLowerCase().includes('invalid')) { | ||
| const message = parsedError.message.toLowerCase(); | ||
| if (message.includes('password') || message.includes('invalid')) { | ||
| setNeedsPassword(true); | ||
| setSetupMode(false); // Close setup mode to show the message in main view | ||
| setPassword(''); | ||
| } |
There was a problem hiding this comment.
Prefer error codes over message content for logic branching.
The check message.includes('password') || message.includes('invalid') is fragile as it depends on specific error message text which could vary or change. Consider checking parsedError.code instead for more reliable error categorization.
Suggested improvement
const { handleError } = await import('@/lib/error-utils.js');
const parsedError = await handleError(err, {
showToast: true,
toastTitle: '2FA Setup Failed',
});
// Check if error is about missing/invalid password - close setup and show message
- const message = parsedError.message.toLowerCase();
- if (message.includes('password') || message.includes('invalid')) {
+ if (parsedError.code === 'INVALID_PASSWORD' || parsedError.code === 'PASSWORD_REQUIRED') {
setNeedsPassword(true);
setSetupMode(false); // Close setup mode to show the message in main view
setPassword('');
}This requires ensuring the backend returns appropriate error codes. If the codes don't exist yet, consider adding them to the error codes mapping.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/web/src/components/profile-ui/TwoFactorSetup.jsx around lines 63 to
74, the current branching uses fragile message text checks ("password" /
"invalid"); instead, base the logic on parsedError.code (e.g.,
'MISSING_PASSWORD', 'INVALID_PASSWORD') returned from the backend. Modify the
condition to check parsedError.code for those canonical values and only fall
back to the existing message.includes checks if code is undefined (to preserve
backward compatibility). If the backend does not yet supply these codes, add
them to the API/error mapping and update tests; finally, keep the existing
setNeedsPassword(true), setSetupMode(false) and setPassword('') actions when the
code matches.
| const { handleError } = await import('@/lib/error-utils.js'); | ||
| const parsedError = await handleError(err, { | ||
| showToast: true, | ||
| toastTitle: 'Disable Failed', | ||
| }); | ||
| const message = parsedError.message.toLowerCase(); | ||
| if (message.includes('password') || message.includes('invalid')) { | ||
| setNeedsPassword(true); | ||
| setDisableMode(false); // Close disable mode to show message in main view | ||
| setPassword(''); | ||
| } |
There was a problem hiding this comment.
Same fragile message-based check as in setup flow.
This has the same issue as the password submit handler - relying on message content for logic branching.
Suggested improvement
const { handleError } = await import('@/lib/error-utils.js');
const parsedError = await handleError(err, {
showToast: true,
toastTitle: 'Disable Failed',
});
- const message = parsedError.message.toLowerCase();
- if (message.includes('password') || message.includes('invalid')) {
+ if (parsedError.code === 'INVALID_PASSWORD' || parsedError.code === 'PASSWORD_REQUIRED') {
setNeedsPassword(true);
setDisableMode(false); // Close disable mode to show message in main view
setPassword('');
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/shared/src/errors/validate.ts (1)
12-46: Validation logic is correct; consider Zod for consistency.The validation logic properly checks the required shape and rejects transport/unknown codes from API responses.
Minor observations:
- The
as stringcast on line 45 is redundant since both branches of the ternary already produce a string.- Per coding guidelines, Zod is preferred for schema validation. This manual approach works but may diverge from the validation patterns used elsewhere in the codebase.
Remove redundant cast
- timestamp: (typeof error.timestamp === 'string' ? - error.timestamp - : new Date().toISOString()) as string, + timestamp: typeof error.timestamp === 'string' + ? error.timestamp + : new Date().toISOString(),packages/shared/src/errors/helpers.ts (1)
128-156: Simplify by using the existing DOMAIN_ERRORS object.The function manually constructs an array of domain errors, but the
DOMAIN_ERRORSobject (exported fromdomain.ts) already aggregates all domain errors.Proposed refactor
+import { DOMAIN_ERRORS } from './domains/domain.js'; + export function getErrorMessage(code: string): string { // Check domain errors - const allDomainErrors = [ - ...Object.values(AUTH_ERRORS), - ...Object.values(VALIDATION_ERRORS), - ...Object.values(PROJECT_ERRORS), - ...Object.values(FILE_ERRORS), - ...Object.values(USER_ERRORS), - ...Object.values(SYSTEM_ERRORS), - ]; - const domainError = allDomainErrors.find(e => e.code === code); + const domainError = Object.values(DOMAIN_ERRORS).find(e => e.code === code); if (domainError) { return domainError.defaultMessage; } // Check transport errors const transportError = Object.values(TRANSPORT_ERRORS).find(e => e.code === code); if (transportError) { return transportError.defaultMessage; } // Check unknown errors const unknownError = Object.values(UNKNOWN_ERRORS).find(e => e.code === code); if (unknownError) { return unknownError.defaultMessage; } return 'An unexpected error occurred'; }packages/shared/src/errors/types.ts (1)
101-106: Consider adding clarifying comments for forward declarations.These string type aliases are forward declarations that get overridden by more specific literal union types from
domain.tswhen imported throughindex.ts. While functionally correct, this pattern may confuse developers reading this file in isolation.Suggested clarification
-// Error code type exports (will be defined in domain files) +// Error code type exports (forward declarations) +// Note: These are re-exported from domain.ts with specific literal union types +// The domain.ts exports will shadow these when imported through index.ts export type ValidationErrorCode = string; export type ProjectErrorCode = string; export type FileErrorCode = string; export type AuthErrorCode = string; export type SystemErrorCode = string;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
packages/shared/.gitignore(1 hunks)packages/shared/README.md(1 hunks)packages/shared/package.json(1 hunks)packages/shared/src/errors/domains/domain.ts(1 hunks)packages/shared/src/errors/domains/transport.ts(1 hunks)packages/shared/src/errors/domains/unknown.ts(1 hunks)packages/shared/src/errors/helpers.ts(1 hunks)packages/shared/src/errors/index.ts(1 hunks)packages/shared/src/errors/normalize.ts(1 hunks)packages/shared/src/errors/types.ts(1 hunks)packages/shared/src/errors/validate.ts(1 hunks)packages/shared/src/index.ts(1 hunks)packages/shared/tsconfig.json(1 hunks)packages/web/src/lib/error-utils.js(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/shared/README.md
- packages/shared/src/index.ts
- packages/shared/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/lib/error-utils.js
🧰 Additional context used
📓 Path-based instructions (3)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/shared/src/errors/domains/unknown.tspackages/shared/tsconfig.jsonpackages/shared/package.jsonpackages/shared/src/errors/validate.tspackages/shared/src/errors/normalize.tspackages/shared/src/errors/domains/transport.tspackages/shared/src/errors/domains/domain.tspackages/shared/src/errors/helpers.tspackages/shared/src/errors/types.tspackages/shared/src/errors/index.ts
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/shared/src/errors/domains/unknown.tspackages/shared/src/errors/validate.tspackages/shared/src/errors/normalize.tspackages/shared/src/errors/domains/transport.tspackages/shared/src/errors/domains/domain.tspackages/shared/src/errors/helpers.tspackages/shared/src/errors/types.tspackages/shared/src/errors/index.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/shared/src/errors/domains/unknown.tspackages/shared/src/errors/validate.tspackages/shared/src/errors/normalize.tspackages/shared/src/errors/domains/transport.tspackages/shared/src/errors/domains/domain.tspackages/shared/src/errors/helpers.tspackages/shared/src/errors/types.tspackages/shared/src/errors/index.ts
🧠 Learnings (8)
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/**/*.{js,jsx,ts,tsx} : Prefer modern ES6+ syntax and features
Applied to files:
packages/shared/tsconfig.json
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/**/*.{js,jsx,ts,tsx} : Prefer using config files rather than hardcoding values
Applied to files:
packages/shared/tsconfig.json
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Prefer modern ES6+ syntax and features
Applied to files:
packages/shared/tsconfig.json
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/**/*.{js,jsx,ts,tsx} : Use aliases for imports when appropriate to improve readability
Applied to files:
packages/shared/tsconfig.json
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/**/*.{js,jsx,ts,tsx} : Use Zod for schema and input validation
Applied to files:
packages/shared/tsconfig.json
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Ensure browser compatibility for all frontend code (Safari is usually problematic)
Applied to files:
packages/shared/tsconfig.json
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Use aliases for imports when appropriate to improve readability
Applied to files:
packages/shared/tsconfig.json
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts,css} : Ensure browser compatibility for all frontend code (Safari is usually problematic)
Applied to files:
packages/shared/tsconfig.json
🧬 Code graph analysis (3)
packages/shared/src/errors/domains/unknown.ts (1)
packages/shared/src/errors/index.ts (2)
UNKNOWN_ERRORS(43-43)UnknownErrorCode(43-43)
packages/shared/src/errors/domains/transport.ts (2)
packages/shared/src/errors/index.ts (2)
TRANSPORT_ERRORS(40-40)TransportErrorCode(40-40)packages/shared/src/errors/types.ts (1)
TransportErrorCode(47-50)
packages/shared/src/errors/types.ts (3)
packages/shared/src/errors/index.ts (16)
AppError(8-8)DomainError(9-9)DomainErrorCode(13-13)ErrorDetails(11-11)TransportError(10-10)TransportErrorCode(40-40)ValidationErrorDetails(14-14)ProjectErrorDetails(15-15)FileErrorDetails(16-16)AuthErrorDetails(17-17)SystemErrorDetails(18-18)ValidationErrorCode(32-32)ProjectErrorCode(33-33)FileErrorCode(34-34)AuthErrorCode(31-31)SystemErrorCode(36-36)packages/shared/src/errors/domains/transport.ts (1)
TransportErrorCode(22-22)packages/shared/src/errors/domains/domain.ts (5)
ValidationErrorCode(71-72)ProjectErrorCode(103-103)FileErrorCode(134-134)AuthErrorCode(30-30)SystemErrorCode(191-191)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (8)
packages/shared/package.json (1)
1-28: Well-structured package configuration.The exports map correctly exposes both the root and
./errorssubpaths with proper type and import conditions. The ESM configuration is correct.One minor consideration: the
cleanscript usesrm -rfwhich is Unix-specific. If Windows compatibility is needed, consider using a cross-platform solution likerimrafor Node's built-infs.rm.packages/shared/tsconfig.json (1)
1-24: Solid TypeScript configuration.The configuration properly enables strict type checking, declaration generation for consumers, and modern ES2022 target. The
bundlermodule resolution works well with modern build tools.packages/shared/src/errors/domains/unknown.ts (1)
6-24: Clean error definition pattern.The const assertion with derived type union is idiomatic TypeScript. The error codes and messages are appropriately descriptive for fallback error handling.
packages/shared/src/errors/normalize.ts (2)
84-99: Appropriate error handling for edge cases.The Response object check with
console.erroris a good safeguard against misuse. The fallback toUNKNOWN_UNHANDLED_ERRORwith the original error stringified provides useful debugging context.
26-33: Well-structured type guard.The
isDomainErrorcheck correctly validates the shape and excludes transport/unknown prefixes, ensuring domain errors only come from API responses.packages/shared/src/errors/domains/transport.ts (1)
6-22: Code structure is correct and intentionally consistent with type definitions.The
TRANSPORT_ERRORSobject correctly omitsstatusCodebecause transport errors extendTransportError, notErrorDefinition. The type definition explicitly notes that transport errors don't have statusCode since they occur before/after API calls, not in API responses. The structure and usage patterns are correct.packages/shared/src/errors/domains/domain.ts (1)
1-201: LGTM! Well-structured error domain definitions.The centralized error code system is well-organized with consistent structure across all domains. The use of
as constassertions preserves literal types for type safety, and the derived union types enable proper TypeScript discrimination.packages/shared/src/errors/index.ts (1)
1-59: LGTM! Clean public API organization.The index file effectively consolidates all error system exports into a single, well-organized entry point. The categorization by types, domains, helpers, normalization, and validation makes the API easy to navigate.
…https://github.com/InfinityBowman/corates into 110-migrate-frontend-to-use-error-codes-like-backend
…https://github.com/InfinityBowman/corates into 110-migrate-frontend-to-use-error-codes-like-backend
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/workers/src/routes/pdfs.js (1)
35-49: Inconsistent error response format in middleware.The
verifyProjectMembershipmiddleware still uses the old inline error format ({ error: '...' }) instead of the new domain error structure. This breaks consistency with the rest of the file.🔎 Proposed fix to use domain errors
if (!projectId || !studyId) { - return c.json({ error: 'Missing project or study ID' }, 400); + const error = createDomainError( + VALIDATION_ERRORS.FIELD_REQUIRED, + { field: 'projectId or studyId' }, + 'Missing project or study ID', + ); + return c.json(error, error.statusCode); } // Verify user is a member of this project using Drizzle ORM const db = createDb(c.env.DB); const membership = await db .select({ role: projectMembers.role }) .from(projectMembers) .where(and(eq(projectMembers.projectId, projectId), eq(projectMembers.userId, user.id))) .get(); if (!membership) { - return c.json({ error: 'Not a member of this project' }, 403); + const error = createDomainError( + AUTH_ERRORS.FORBIDDEN, + { reason: 'not_project_member' }, + 'Not a member of this project', + ); + return c.json(error, error.statusCode); }packages/workers/src/routes/avatars.js (1)
194-204: Inconsistent error response format for "not found" cases.The GET endpoint still uses the old inline error format (
{ error: 'Avatar not found' }) instead of domain errors. This is inconsistent with the rest of the file.🔎 Proposed fix to use domain errors
if (listed.objects.length === 0) { - return c.json({ error: 'Avatar not found' }, 404); + const error = createDomainError(FILE_ERRORS.NOT_FOUND, { userId }); + return c.json(error, error.statusCode); } // Get the most recent avatar const avatarKey = listed.objects[0].key; const object = await c.env.PDF_BUCKET.get(avatarKey); if (!object) { - return c.json({ error: 'Avatar not found' }, 404); + const error = createDomainError(FILE_ERRORS.NOT_FOUND, { userId, key: avatarKey }); + return c.json(error, error.statusCode); }
♻️ Duplicate comments (1)
packages/shared/src/errors/normalize.ts (1)
60-74: CORS errors use generic network error code (previously flagged).The pattern matching includes
'cors'at line 65 but createsTRANSPORT_NETWORK_ERRORinstead ofTRANSPORT_CORS_ERROR. SinceTRANSPORT_CORS_ERRORexists in the transport error definitions, consider using it for more precise error categorization.
🧹 Nitpick comments (9)
packages/web/src/lib/__tests__/error-utils.test.js (2)
101-119: Consider verifying error types in rejection tests.The tests at lines 112 and 118 only verify that an error is thrown via
rejects.toThrow(), but don't assert on the error code or type. For consistency with the test pattern elsewhere, consider verifying the specific error code.Proposed enhancement
it('should parse and throw domain error for non-ok response', async () => { const response = new Response( JSON.stringify({ code: 'PROJECT_NOT_FOUND', message: 'Project not found', statusCode: 404, }), { status: 404 }, ); const fetchPromise = Promise.resolve(response); - await expect(handleFetchError(fetchPromise, { showToast: false })).rejects.toThrow(); + await expect(handleFetchError(fetchPromise, { showToast: false })).rejects.toMatchObject({ + code: 'PROJECT_NOT_FOUND', + }); }); it('should handle network errors as transport errors', async () => { const fetchPromise = Promise.reject(new Error('Failed to fetch')); - await expect(handleFetchError(fetchPromise, { showToast: false })).rejects.toThrow(); + await expect(handleFetchError(fetchPromise, { showToast: false })).rejects.toMatchObject({ + code: 'TRANSPORT_NETWORK_ERROR', + }); });
169-176: Consider asserting the error message passed to setError.For consistency with the
handleDomainErrortest at line 142 which assertstoHaveBeenCalledWith('Project not found'), this test could verify the specific message passed tosetError.Proposed enhancement
it('should update error state if setter provided', async () => { const error = createTransportError('TRANSPORT_NETWORK_ERROR'); const setError = vi.fn(); await handleTransportError(error, { setError, showToast: false }); - expect(setError).toHaveBeenCalled(); + expect(setError).toHaveBeenCalledWith(error.message); });packages/shared/src/errors/__tests__/helpers.test.ts (1)
39-45: Consider avoiding non-null assertion.Line 41 uses
error.timestamp!which bypasses TypeScript's null check. Since the assertion on line 43 already validates it's a Date instance, you could restructure slightly to avoid the assertion.Alternative approach
it('should include timestamp', () => { const error = createDomainError(PROJECT_ERRORS.NOT_FOUND); - const timestamp = new Date(error.timestamp!); - - expect(timestamp).toBeInstanceOf(Date); - expect(isNaN(timestamp.getTime())).toBe(false); + expect(error.timestamp).toBeDefined(); + expect(new Date(error.timestamp!).getTime()).not.toBeNaN(); });Or keep the original structure but add an explicit check first:
it('should include timestamp', () => { const error = createDomainError(PROJECT_ERRORS.NOT_FOUND); + expect(error.timestamp).toBeDefined(); const timestamp = new Date(error.timestamp!); expect(timestamp).toBeInstanceOf(Date); expect(isNaN(timestamp.getTime())).toBe(false); });packages/workers/src/routes/pdfs.js (1)
91-98: Consider using a more accurate error type for R2 operations.
SYSTEM_ERRORS.DB_ERRORis semantically incorrect for R2 bucket list operations. Consider usingSYSTEM_ERRORS.INTERNAL_ERRORor introducing a dedicated storage/file system error if one exists in the shared package.🔎 Proposed fix
} catch (error) { console.error('PDF list error:', error); - const dbError = createDomainError(SYSTEM_ERRORS.DB_ERROR, { + const storageError = createDomainError(SYSTEM_ERRORS.INTERNAL_ERROR, { operation: 'list_pdfs', originalError: error.message, }); - return c.json(dbError, dbError.statusCode); + return c.json(storageError, storageError.statusCode); }packages/workers/src/routes/avatars.js (1)
175-179: Consider using a more accurate error type for R2 operations.Similar to
pdfs.js, usingSYSTEM_ERRORS.DB_ERRORfor R2 bucket operations is semantically misleading. These are storage/file system errors, not database errors.🔎 Proposed fix
} catch (error) { console.error('Avatar upload error:', error); - const dbError = createDomainError(SYSTEM_ERRORS.DB_ERROR, { + const storageError = createDomainError(SYSTEM_ERRORS.INTERNAL_ERROR, { operation: 'upload_avatar', originalError: error.message, }); - return c.json(dbError, dbError.statusCode); + return c.json(storageError, storageError.statusCode); }Apply similar changes to
fetch_avataranddelete_avatarerror handlers.Also applies to: 215-219, 241-245
packages/workers/src/config/validation.js (1)
121-144: Consider simplifying redundant branches in the switch.Lines 135-138 have two branches that return the same value. This can be consolidated.
Proposed simplification
case 'invalid_string': - if (issue.validation === 'email' || issue.validation === 'uuid') { - return 'VALIDATION_FIELD_INVALID_FORMAT'; - } - return 'VALIDATION_FIELD_INVALID_FORMAT'; + return 'VALIDATION_FIELD_INVALID_FORMAT';packages/web/src/lib/error-utils.js (2)
14-16: Unused import:PROJECT_ERRORS.
PROJECT_ERRORSis imported but never used in this file.Proposed fix
import { validateErrorResponse, normalizeError, isDomainError, isTransportError, getErrorMessage, createUnknownError, AUTH_ERRORS, - PROJECT_ERRORS, } from '@corates/shared';
70-85: Redundant fallback logic after transport error handling.Lines 77-84 re-call
normalizeError(error)after it was already called at line 71. If the error wasn't a transport error at line 72, callingnormalizeErroragain will produce the same result. This block seems unreachable in practice sincenormalizeErrorshould always return either a domain or transport error.Proposed simplification
} catch (error) { // If already a domain error, re-throw if (isDomainError(error)) { throw error; } // Otherwise, it's a transport error (network, fetch failure, etc.) const transportError = normalizeError(error); if (isTransportError(transportError)) { await handleTransportError(transportError, options); throw transportError; } - // Fallback to unknown error - const unknownError = normalizeError(error); - if (isDomainError(unknownError)) { - await handleDomainError(unknownError, options); - } else { - await handleTransportError(unknownError, options); - } - throw unknownError; + // normalizeError returned a domain-like error (unknown errors are domain errors) + await handleDomainError(transportError, options); + throw transportError; }packages/shared/src/errors/helpers.ts (1)
129-157: Consider caching the merged error definitions for performance.
getErrorMessagerebuilds theallDomainErrorsarray on every call. While acceptable for error handling (not a hot path), caching could help if called frequently.Proposed optimization
+// Cache merged domain errors for getErrorMessage lookup +const allDomainErrors = [ + ...Object.values(AUTH_ERRORS), + ...Object.values(VALIDATION_ERRORS), + ...Object.values(PROJECT_ERRORS), + ...Object.values(FILE_ERRORS), + ...Object.values(USER_ERRORS), + ...Object.values(SYSTEM_ERRORS), +]; + /** * Get error message for a code */ export function getErrorMessage(code: string): string { - // Check domain errors - const allDomainErrors = [ - ...Object.values(AUTH_ERRORS), - ...Object.values(VALIDATION_ERRORS), - ...Object.values(PROJECT_ERRORS), - ...Object.values(FILE_ERRORS), - ...Object.values(USER_ERRORS), - ...Object.values(SYSTEM_ERRORS), - ]; const domainError = allDomainErrors.find(e => e.code === code);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
packages/shared/package.json(1 hunks)packages/shared/src/errors/__tests__/helpers.test.ts(1 hunks)packages/shared/src/errors/__tests__/normalize.test.ts(1 hunks)packages/shared/src/errors/__tests__/validate.test.ts(1 hunks)packages/shared/src/errors/helpers.ts(1 hunks)packages/shared/src/errors/normalize.ts(1 hunks)packages/shared/vitest.config.js(1 hunks)packages/web/package.json(1 hunks)packages/web/src/constants/errors.js(1 hunks)packages/web/src/lib/__tests__/error-utils.test.js(1 hunks)packages/web/src/lib/error-utils.js(1 hunks)packages/workers/package.json(1 hunks)packages/workers/src/config/__tests__/validation.test.js(4 hunks)packages/workers/src/config/constants.js(1 hunks)packages/workers/src/config/validation.js(5 hunks)packages/workers/src/routes/avatars.js(6 hunks)packages/workers/src/routes/members.js(15 hunks)packages/workers/src/routes/pdfs.js(12 hunks)packages/workers/src/routes/projects.js(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/shared/vitest.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/shared/package.json
- packages/web/src/constants/errors.js
🧰 Additional context used
📓 Path-based instructions (8)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/shared/src/errors/__tests__/helpers.test.tspackages/workers/src/config/__tests__/validation.test.jspackages/workers/src/config/constants.jspackages/shared/src/errors/__tests__/normalize.test.tspackages/workers/src/routes/pdfs.jspackages/web/src/lib/error-utils.jspackages/web/package.jsonpackages/shared/src/errors/normalize.tspackages/web/src/lib/__tests__/error-utils.test.jspackages/workers/src/config/validation.jspackages/shared/src/errors/__tests__/validate.test.tspackages/workers/src/routes/members.jspackages/workers/src/routes/projects.jspackages/workers/src/routes/avatars.jspackages/shared/src/errors/helpers.tspackages/workers/package.json
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/shared/src/errors/__tests__/helpers.test.tspackages/workers/src/config/__tests__/validation.test.jspackages/workers/src/config/constants.jspackages/shared/src/errors/__tests__/normalize.test.tspackages/workers/src/routes/pdfs.jspackages/web/src/lib/error-utils.jspackages/shared/src/errors/normalize.tspackages/web/src/lib/__tests__/error-utils.test.jspackages/workers/src/config/validation.jspackages/shared/src/errors/__tests__/validate.test.tspackages/workers/src/routes/members.jspackages/workers/src/routes/projects.jspackages/workers/src/routes/avatars.jspackages/shared/src/errors/helpers.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/shared/src/errors/__tests__/helpers.test.tspackages/workers/src/config/__tests__/validation.test.jspackages/workers/src/config/constants.jspackages/shared/src/errors/__tests__/normalize.test.tspackages/workers/src/routes/pdfs.jspackages/web/src/lib/error-utils.jspackages/shared/src/errors/normalize.tspackages/web/src/lib/__tests__/error-utils.test.jspackages/workers/src/config/validation.jspackages/shared/src/errors/__tests__/validate.test.tspackages/workers/src/routes/members.jspackages/workers/src/routes/projects.jspackages/workers/src/routes/avatars.jspackages/shared/src/errors/helpers.ts
packages/workers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/workers/**/*.{js,ts}: Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/config/__tests__/validation.test.jspackages/workers/src/config/constants.jspackages/workers/src/routes/pdfs.jspackages/workers/src/config/validation.jspackages/workers/src/routes/members.jspackages/workers/src/routes/projects.jspackages/workers/src/routes/avatars.js
packages/workers/src/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/workers/src/**/*.{js,ts}: Use Zod for schema and input validation on the backend
Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/config/__tests__/validation.test.jspackages/workers/src/config/constants.jspackages/workers/src/routes/pdfs.jspackages/workers/src/config/validation.jspackages/workers/src/routes/members.jspackages/workers/src/routes/projects.jspackages/workers/src/routes/avatars.js
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx,js,ts}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Import stores directly where needed instead of passing values through multiple components
When you need to compute a value based on props or state in SolidJS, usecreateMemoto ensure it updates reactively
For complex state or state objects in SolidJS, use Solid'screateStorefor better performance and reactivity
You may create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Files:
packages/web/src/lib/error-utils.jspackages/web/src/lib/__tests__/error-utils.test.js
packages/web/src/**/*.{jsx,tsx,js,ts,css}
📄 CodeRabbit inference engine (.cursorrules)
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Files:
packages/web/src/lib/error-utils.jspackages/web/src/lib/__tests__/error-utils.test.js
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
UsecreateMemofor derived values to ensure they update reactively
Files:
packages/web/src/lib/error-utils.jspackages/web/src/lib/__tests__/error-utils.test.js
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/web/src/primitives/**/*.{js,ts} : Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Ensure browser compatibility for all frontend code (Safari is usually problematic)
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Zod for schema and input validation on the backend
Applied to files:
packages/workers/src/config/__tests__/validation.test.jspackages/workers/src/config/validation.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/routes/pdfs.jspackages/workers/src/routes/members.jspackages/workers/src/routes/projects.jspackages/workers/src/routes/avatars.jspackages/workers/package.json
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/routes/pdfs.jspackages/workers/src/routes/members.jspackages/workers/src/routes/projects.jspackages/workers/src/routes/avatars.jspackages/workers/package.json
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/web/package.jsonpackages/workers/package.json
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/web/package.jsonpackages/workers/package.json
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/**/*.{js,jsx,ts,tsx} : Use Zod for schema and input validation
Applied to files:
packages/workers/src/config/validation.js
🧬 Code graph analysis (10)
packages/shared/src/errors/__tests__/helpers.test.ts (1)
packages/shared/src/errors/helpers.ts (6)
createDomainError(29-41)createValidationError(46-64)createMultiFieldValidationError(69-81)createTransportError(86-102)createUnknownError(107-124)getErrorMessage(129-157)
packages/shared/src/errors/__tests__/normalize.test.ts (2)
packages/shared/src/errors/helpers.ts (2)
createDomainError(29-41)createTransportError(86-102)packages/shared/src/errors/normalize.ts (3)
isDomainError(26-33)isTransportError(38-40)normalizeError(46-108)
packages/workers/src/routes/pdfs.js (2)
packages/shared/src/errors/helpers.ts (1)
createDomainError(29-41)packages/workers/src/config/constants.js (2)
FILE_SIZE_LIMITS(24-28)FILE_SIZE_LIMITS(24-28)
packages/web/src/lib/error-utils.js (3)
packages/shared/src/errors/validate.ts (1)
validateErrorResponse(12-47)packages/shared/src/errors/normalize.ts (3)
isDomainError(26-33)normalizeError(46-108)isTransportError(38-40)packages/shared/src/errors/domains/domain.ts (1)
AUTH_ERRORS(7-28)
packages/shared/src/errors/normalize.ts (2)
packages/shared/src/errors/types.ts (2)
DomainError(14-18)TransportError(21-25)packages/shared/src/errors/helpers.ts (3)
createTransportError(86-102)getErrorMessage(129-157)createUnknownError(107-124)
packages/web/src/lib/__tests__/error-utils.test.js (2)
packages/web/src/lib/error-utils.js (8)
response(54-54)parseApiError(25-39)handleFetchError(52-86)handleDomainError(98-128)handleTransportError(139-157)handleError(166-195)parseError(202-204)isErrorCode(212-214)packages/shared/src/errors/helpers.ts (2)
createDomainError(29-41)createTransportError(86-102)
packages/workers/src/config/validation.js (1)
packages/shared/src/errors/helpers.ts (2)
createValidationError(46-64)createMultiFieldValidationError(69-81)
packages/shared/src/errors/__tests__/validate.test.ts (2)
packages/web/src/lib/error-utils.js (1)
data(27-27)packages/workers/src/routes/avatars.js (1)
timestamp(124-124)
packages/workers/src/routes/members.js (4)
packages/workers/src/routes/pdfs.js (14)
error(113-117)error(124-128)error(144-148)error(154-158)error(171-175)error(179-183)error(188-192)error(202-206)error(214-218)error(264-268)error(273-277)error(287-287)error(320-324)dbError(93-96)packages/workers/src/routes/projects.js (9)
error(69-69)error(198-202)error(251-255)c(91-91)c(187-187)dbError(76-79)dbError(171-174)dbError(225-228)dbError(348-351)packages/shared/src/errors/helpers.ts (1)
createDomainError(29-41)packages/workers/src/routes/avatars.js (3)
dbError(175-178)dbError(215-218)dbError(241-244)
packages/workers/src/routes/projects.js (2)
packages/shared/src/errors/helpers.ts (1)
createDomainError(29-41)packages/workers/src/routes/avatars.js (3)
dbError(175-178)dbError(215-218)dbError(241-244)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates
- GitHub Check: Workers Builds: corates-workers-prod
🔇 Additional comments (42)
packages/workers/package.json (1)
26-26: LGTM! Workspace dependency added correctly.The addition of
@corates/sharedas a workspace dependency is appropriate and aligns with the PR objective to centralize error handling. Using theworkspace:*protocol is the correct approach for pnpm monorepos.packages/web/package.json (1)
18-18: LGTM! Dependency addition is correctly configured.The workspace dependency is properly formatted, alphabetically ordered, correctly placed in runtime dependencies, and the
@corates/sharedpackage has proper ESM exports configured with type definitions. The shared package contains only TypeScript utilities with no runtime dependencies, ensuring full browser compatibility.packages/web/src/lib/__tests__/error-utils.test.js (3)
1-27: LGTM - well-structured test setup.The imports are properly organized, and the
showToastmock is appropriately configured to isolate toast side effects during testing.
29-89: LGTM - comprehensive parseApiError test coverage.The test suite covers the key scenarios: valid domain errors, shape validation, invalid JSON handling, and rejection of transport error codes from API responses. The assertion at line 87 correctly verifies that transport codes are normalized to
UNKNOWN_INVALID_RESPONSE.
122-153: LGTM - good coverage for error handling and utility functions.The
handleDomainError,handleError,parseError, andisErrorCodetests provide solid coverage of the core error handling paths, including edge cases like null/undefined inputs.Also applies to: 179-235
packages/shared/src/errors/normalize.ts (3)
9-21: LGTM - clean internal guard function.The
isAppErrorfunction properly validates the base shape with type-safe property checks.
23-40: LGTM - well-designed type guards.The
isDomainErrorandisTransportErrorguards provide clear discriminated union checks using code prefixes and statusCode presence. This aligns with the type definitions intypes.ts.
84-101: Good defensive check for Response objects.The detection of Response-like objects with a programmer error log helps catch misuse of
normalizeErrorwhenparseApiErrorshould be used instead. This provides helpful debugging guidance.packages/workers/src/config/constants.js (1)
71-73: LGTM - clear migration note.The comment provides helpful guidance for developers looking for the previously defined
ERROR_CODESconstant.packages/shared/src/errors/__tests__/helpers.test.ts (2)
78-100: LGTM - good coverage for multi-field validation errors.The test appropriately uses a runtime type guard at lines 95-98 to safely access the nested
fieldsarray, which is necessary given the dynamic nature of thedetailstype.
102-159: LGTM - comprehensive tests for transport, unknown errors, and getErrorMessage.The tests correctly verify that transport errors lack
statusCode(line 108), unknown errors includestatusCode(line 128), andgetErrorMessageprovides appropriate fallback behavior (lines 156-159).packages/workers/src/routes/members.js (6)
12-19: LGTM - clean import of shared error utilities.The imports align with the centralized error handling approach, bringing in the necessary domain error creators and error type constants.
29-47: Silent error handling for DO sync may hide issues.The
syncMemberToDOfunction catches errors and only logs them (lines 44-46). While this prevents sync failures from blocking the main operation, consider whether failed syncs should be tracked or retried, especially if they could cause state inconsistency between the database and Durable Object.Is it acceptable for member state to diverge between the database and Durable Object if sync fails? If not, consider adding a retry mechanism or surfacing these failures for monitoring.
57-77: LGTM - consistent error responses in middleware.The middleware correctly uses
VALIDATION_ERRORS.FIELD_REQUIREDfor missing projectId andPROJECT_ERRORS.ACCESS_DENIEDfor unauthorized access, returning the error with the appropriate status code.
117-121: LGTM - consistent DB error handling pattern.All database error handlers consistently use
SYSTEM_ERRORS.DB_ERRORwith meaningfuloperationcontext and include theoriginalErrormessage for debugging. This aligns with the pattern seen in other route files likeavatars.js.Also applies to: 259-263, 327-331, 435-439
134-139: LGTM - appropriate use of domain-specific error codes.Good use of:
AUTH_ERRORS.FORBIDDENfor permission denials with descriptivereasoncontextUSER_ERRORS.NOT_FOUNDfor user lookup failuresPROJECT_ERRORS.MEMBER_ALREADY_EXISTSfor duplicate member additionsPROJECT_ERRORS.LAST_OWNERfor ownership constraintsThe error messages and details provide sufficient context for debugging and user feedback.
Also applies to: 177-193, 277-282, 304-309, 348-353, 384-389
367-372: The suggested error code does not exist in the codebase.
PROJECT_ERRORS.MEMBER_NOT_FOUNDis not defined. The available PROJECT_ERRORS codes are:NOT_FOUND,ACCESS_DENIED,MEMBER_ALREADY_EXISTS,LAST_OWNER, andINVALID_ROLE. UsingPROJECT_ERRORS.NOT_FOUNDwith a message override is the appropriate approach given the available error definitions.Likely an incorrect or invalid review comment.
packages/shared/src/errors/__tests__/validate.test.ts (1)
1-123: LGTM! Comprehensive test coverage for error response validation.The test suite covers all critical paths: valid domain errors, missing required fields, rejection of transport/unknown error codes from API responses, non-object inputs, and timestamp handling. Good edge case coverage.
packages/workers/src/config/__tests__/validation.test.js (4)
27-49: LGTM! Test assertions properly updated for domain error structure.The tests correctly verify the new error shape with
code,message,statusCode, anddetails.fieldproperties. The regex pattern/^VALIDATION_/provides flexibility while specific code assertions ensure correctness.
70-87: LGTM! Clear validation error code assertions.Good use of specific error codes (
VALIDATION_FIELD_TOO_SHORT,VALIDATION_FIELD_TOO_LONG) and field-level details verification.
163-171: LGTM! Refinement error handling correctly documented.The comment clarifying that
refine()errors map toVALIDATION_FAILEDis helpful for understanding the expected behavior.
272-283: LGTM! Consistent error code assertions for email schema.The test correctly expects
VALIDATION_FAILEDfor refinement-based validation (requiring eitherhtmlortext).packages/workers/src/routes/pdfs.js (3)
105-252: LGTM! Upload endpoint with comprehensive domain error handling.The upload flow properly validates content-length, content-type, file type (multipart vs raw PDF), file name, PDF magic bytes, and duplicate file existence. All error paths use appropriate domain errors with contextual details.
258-309: LGTM! Download endpoint with proper error handling.File name validation and not-found scenarios are handled with appropriate domain errors.
315-364: LGTM! Delete endpoint with consistent error handling.Permission check and validation errors use the domain error pattern consistently.
packages/shared/src/errors/__tests__/normalize.test.ts (1)
1-108: LGTM! Comprehensive test coverage for error normalization.The test suite properly covers:
- Type guards (
isDomainError,isTransportError) with valid and invalid inputs- Pass-through behavior for already-normalized errors
- Conversion of native
Errorobjects based on message patterns- Programmer error detection for Response objects
- Edge cases with null/undefined inputs
The conditional
isDomainErrorguards before checkingstatusCodeare correctly placed sincenormalizeErrorcan return eitherTransportError(nostatusCode) orDomainError.packages/workers/src/routes/avatars.js (1)
71-165: LGTM! Upload endpoint with proper domain error handling.The upload flow correctly validates content-length, file presence, file type, and file size with appropriate domain errors and contextual details.
packages/workers/src/routes/projects.js (5)
12-13: LGTM!Clean import organization separating constants from shared error utilities. The pattern aligns with other route files like
avatars.js.
68-81: LGTM!The error handling pattern is consistent and well-structured. Using
error.statusCodedirectly from the domain error ensures the correct HTTP status is returned without hardcoding.
169-176: LGTM!Appropriate use of
SYSTEM_ERRORS.DB_TRANSACTION_FAILEDfor the batch operation failure, distinguishing it from regular DB errors. The operation context (create_project) aids debugging.
197-204: LGTM!Good use of
messageOverrideparameter to provide context-specific messaging while using the standardAUTH_ERRORS.FORBIDDENerror definition. Thereasondetail is helpful for debugging.
250-257: Consistent pattern across authorization checks.The delete authorization error mirrors the update pattern appropriately, with distinct
reasonvalues for each operation.packages/workers/src/config/validation.js (3)
159-176: LGTM!Clean transformation of Zod issues to structured validation errors with proper field path resolution. The dual support for
issue.kind(v4) andissue.code(v3) ensures backward compatibility.
178-191: LGTM!Good differentiation between single-field and multi-field validation errors. Single errors get more specific error codes while multiple errors use the aggregate
VALIDATION_MULTI_FIELDcode.
215-234: LGTM!The middleware correctly leverages
DomainError.statusCodefor HTTP responses. Invalid JSON handling creates a proper validation error with descriptive constraint.packages/web/src/lib/error-utils.js (3)
52-86: LGTM on the core fetch error handling pattern.The separation of domain errors (from API responses) and transport errors (network failures) provides a clean abstraction. The past review comment about double-parsing has been addressed.
98-128: LGTM!Clean handling of domain errors with appropriate navigation for auth-related errors. The pattern supports flexible UI feedback through options.
166-195: LGTM!The unified
handleErrorfunction provides good backward compatibility by auto-detecting error types. HandlingResponseobjects directly maintains compatibility with existing call sites.packages/shared/src/errors/helpers.ts (4)
29-41: LGTM!Clean implementation of the core domain error factory. The optional
messageOverrideprovides flexibility while defaulting to the definition's message.
46-64: Previous review issue addressed.The lookup now correctly uses
Object.values(VALIDATION_ERRORS).find(e => e.code === errorCode)to match the full error code value rather than incorrectly using it as a key.
69-81: LGTM!The multi-field validation error properly aggregates field errors into a single
DomainErrorwith structured details.
86-102: LGTM!Consistent pattern with
createValidationErrorfor transport errors. The defensivethrowon invalid codes prevents silent failures.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
eslint.config.js (1)
113-115: Remove non-existent ESLint rule configuration.
no-new-erroris not a valid ESLint rule. Configuring non-existent rules adds confusion and clutter. If the intent is to document a pattern preference, consider using a code comment elsewhere or adding documentation in the error handling guide instead.Proposed fix
// Prevent throwing literals - must throw Error objects 'no-throw-literal': 'error', - // Prevent creating Error objects without proper error handling - // Encourage using error helpers from @corates/shared - 'no-new-error': 'off', // This doesn't exist, but we document the pattern // 'sonarjs/cognitive-complexity': 'error',packages/web/src/lib/form-errors.js (1)
207-213: Consider avoidingthisfor better destructuring support.Using
this.setFieldErrorandthis.setGlobalErrorrequires the method to be called with proper context. If users destructure the returned object (common in SolidJS), thethisbinding will be lost.Proposed fix - use direct references
handleError(error) { + const self = this; handleFormError( error, - (field, message) => this.setFieldError(field, message), - message => this.setGlobalError(message), + (field, message) => self.setFieldError(field, message), + message => self.setGlobalError(message), ); },Alternatively, define the setters as standalone functions and reference them directly.
packages/web/src/lib/error-utils.js (1)
74-82: Redundant fallback logic - consider simplifying.Lines 74-81 appear unreachable. After line 68 normalizes the error and lines 69-72 handle transport errors, the code path to line 74 only occurs if
isTransportError(transportError)is false. However, sincenormalizeErrorshould return either a domain or transport error, this block re-normalizes unnecessarily.Proposed simplification
// Otherwise, it's a transport error (network, fetch failure, etc.) const transportError = normalizeError(error); if (isTransportError(transportError)) { await handleTransportError(transportError, options); throw transportError; + } else { + // Normalized to domain error (shouldn't happen for network errors, but handle defensively) + await handleDomainError(transportError, options); + throw transportError; } - - // Fallback to unknown error - const unknownError = normalizeError(error); - if (isDomainError(unknownError)) { - await handleDomainError(unknownError, options); - } else { - await handleTransportError(unknownError, options); - } - throw unknownError; } }docs/error-handling-guide.md (1)
91-103: Minor inconsistency in import example.Line 93 imports
handleFetchErrorandhandleDomainErrorbut then references them as if they're used together. The example showshandleFetchErrorwrapping fetch, which handles errors automatically - the explicithandleDomainErrorimport is unused in the example.Proposed fix
```javascript // packages/web/src/lib/error-utils.js -import { handleFetchError, handleDomainError } from '@/lib/error-utils.js'; +import { handleFetchError } from '@/lib/error-utils.js'; // Wrap fetch callspackages/web/src/components/__tests__/ErrorBoundary.test.jsx (1)
1-33: Consider expanding test coverage for component behavior.The current tests validate
normalizeErrorlogic but don't test the ErrorBoundary component itself—error catching, fallback rendering, reset functionality, or navigation. While the existing tests are correct, adding component-level tests would strengthen confidence in the error boundary behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/error-handling-guide.md(1 hunks)eslint.config.js(1 hunks)packages/shared/src/errors/__tests__/normalize.test.ts(1 hunks)packages/shared/src/errors/normalize.ts(1 hunks)packages/web/src/components/ErrorBoundary.jsx(1 hunks)packages/web/src/components/__tests__/ErrorBoundary.test.jsx(1 hunks)packages/web/src/lib/__tests__/form-errors.test.js(1 hunks)packages/web/src/lib/error-utils.js(1 hunks)packages/web/src/lib/form-errors.js(1 hunks)packages/web/src/main.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/shared/src/errors/tests/normalize.test.ts
- packages/shared/src/errors/normalize.ts
🧰 Additional context used
📓 Path-based instructions (10)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/web/src/main.jsxpackages/web/src/components/__tests__/ErrorBoundary.test.jsxpackages/web/src/lib/__tests__/form-errors.test.jseslint.config.jspackages/web/src/lib/error-utils.jspackages/web/src/components/ErrorBoundary.jsxdocs/error-handling-guide.mdpackages/web/src/lib/form-errors.js
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx,js,ts}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Import stores directly where needed instead of passing values through multiple components
When you need to compute a value based on props or state in SolidJS, usecreateMemoto ensure it updates reactively
For complex state or state objects in SolidJS, use Solid'screateStorefor better performance and reactivity
You may create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Files:
packages/web/src/main.jsxpackages/web/src/components/__tests__/ErrorBoundary.test.jsxpackages/web/src/lib/__tests__/form-errors.test.jspackages/web/src/lib/error-utils.jspackages/web/src/components/ErrorBoundary.jsxpackages/web/src/lib/form-errors.js
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/web/src/main.jsxpackages/web/src/components/__tests__/ErrorBoundary.test.jsxpackages/web/src/lib/__tests__/form-errors.test.jspackages/web/src/lib/error-utils.jspackages/web/src/components/ErrorBoundary.jsxpackages/web/src/lib/form-errors.js
packages/web/src/**/*.{jsx,tsx,js,ts,css}
📄 CodeRabbit inference engine (.cursorrules)
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Files:
packages/web/src/main.jsxpackages/web/src/components/__tests__/ErrorBoundary.test.jsxpackages/web/src/lib/__tests__/form-errors.test.jspackages/web/src/lib/error-utils.jspackages/web/src/components/ErrorBoundary.jsxpackages/web/src/lib/form-errors.js
packages/web/src/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx}: Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
Do not destructure props in SolidJS components as it breaks reactivity. Instead, access props directly from the props object or wrap them in a function to ensure they are always up-to-date
Files:
packages/web/src/main.jsxpackages/web/src/components/__tests__/ErrorBoundary.test.jsxpackages/web/src/components/ErrorBoundary.jsx
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
UsecreateMemofor derived values to ensure they update reactively
Files:
packages/web/src/main.jsxpackages/web/src/components/__tests__/ErrorBoundary.test.jsxpackages/web/src/lib/__tests__/form-errors.test.jspackages/web/src/lib/error-utils.jspackages/web/src/components/ErrorBoundary.jsxpackages/web/src/lib/form-errors.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/web/src/main.jsxpackages/web/src/components/__tests__/ErrorBoundary.test.jsxpackages/web/src/lib/__tests__/form-errors.test.jseslint.config.jspackages/web/src/lib/error-utils.jspackages/web/src/components/ErrorBoundary.jsxpackages/web/src/lib/form-errors.js
packages/web/src/components/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/components/**/*.{jsx,tsx}: Use responsive design principles for UI components
Use Zag.js for UI components and design system
Zag components exist inpackages/web/src/components/zag/*and should be reused; check the README.md in that folder for a list of existing components before adding new components and when debugging
Components should receive at most 1–5 props, and only for local configuration, not shared state
If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context
Components should be lean and focused and should not implement business logic; move business logic into stores, utilities, or primitives
Never have a component act as a 'God component' coordinating multiple large concerns
Files:
packages/web/src/components/__tests__/ErrorBoundary.test.jsxpackages/web/src/components/ErrorBoundary.jsx
packages/web/src/components/**/*.{jsx,tsx,js}
📄 CodeRabbit inference engine (.cursorrules)
Group related components in subdirectories with an
index.jsbarrel export
Files:
packages/web/src/components/__tests__/ErrorBoundary.test.jsxpackages/web/src/components/ErrorBoundary.jsx
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/components/**/*.{js,jsx,ts,tsx}: Use responsive design principles for UI components
Group related components in subdirectories with an index.js barrel export
Use Zag.js for UI components and design system
Zag component exist inpackages/web/src/components/zag/*and should be reused. Check the README.md in that folder for a list of existing components before adding new components and when debugging
Components should receive at most 1–5 props, and only for local configuration, not shared state. If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context
Do not destructure props in SolidJS components as it breaks reactivity. Instead, access props directly from the props object or wrap them in a function to ensure they are always up-to-date
Components should be lean and focused. They should not implement business logic; move that into stores, utilities, or primitives
Never have a component act as a God component coordinating multiple large concerns
Files:
packages/web/src/components/__tests__/ErrorBoundary.test.jsxpackages/web/src/components/ErrorBoundary.jsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/web/src/primitives/**/*.{js,ts} : Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts} : You may create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Ensure browser compatibility for all frontend code (Safari is usually problematic)
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Zod for schema and input validation on the backend
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Better-Auth for authentication and user management
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts,css} : Ensure browser compatibility for all frontend code (Safari is usually problematic)
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Better-Auth for authentication and user management
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts} : Import stores directly where needed instead of passing values through multiple components
Applied to files:
packages/web/src/main.jsx
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Prefer modern ES6+ syntax and features
Applied to files:
eslint.config.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx} : Do not destructure props in SolidJS components as it breaks reactivity. Instead, access props directly from the props object or wrap them in a function to ensure they are always up-to-date
Applied to files:
packages/web/src/components/ErrorBoundary.jsx
🧬 Code graph analysis (5)
packages/web/src/main.jsx (1)
packages/web/src/components/ErrorBoundary.jsx (1)
AppErrorBoundary(95-121)
packages/web/src/components/__tests__/ErrorBoundary.test.jsx (3)
packages/web/src/components/ErrorBoundary.jsx (1)
error(21-21)packages/web/src/lib/referenceParser.js (1)
normalized(235-249)packages/shared/src/errors/normalize.ts (1)
normalizeError(46-114)
packages/web/src/lib/__tests__/form-errors.test.js (1)
packages/web/src/lib/form-errors.js (3)
handleFormError(16-55)createFormErrorState(62-126)createFormErrorSignals(135-215)
packages/web/src/components/ErrorBoundary.jsx (2)
packages/shared/src/errors/normalize.ts (2)
isTransportError(38-40)normalizeError(46-114)packages/web/src/lib/error-utils.js (2)
handleError(163-192)normalizedError(184-184)
packages/web/src/lib/form-errors.js (2)
packages/web/src/components/ErrorBoundary.jsx (2)
error(21-21)message(35-38)packages/web/src/lib/referenceParser.js (1)
field(104-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (15)
packages/web/src/lib/form-errors.js (2)
16-55: LGTM - Well-structured form error handling.The
handleFormErrorfunction correctly handles the hierarchy of validation errors (single-field, multi-field, global) and falls back appropriately for domain and transport errors.
62-126: LGTM - Clean non-reactive state manager.The Map-based implementation provides a straightforward API for managing form errors outside of reactive contexts.
packages/web/src/lib/__tests__/form-errors.test.js (3)
168-173: These tests will likely fail due to thesetGlobalErrorrecursion bug.The test at line 171 calls
signals.setGlobalError('Something went wrong'), which will cause infinite recursion due to the shadowing issue identified inform-errors.js. Ensure the fix is applied to make these tests pass.
1-16: LGTM - Good test setup and imports.The test file properly imports dependencies from vitest, solid-js, and the shared error package. The test structure is well-organized.
94-144: LGTM - Comprehensive tests for createFormErrorState.The tests cover initialization, setting/getting errors, clearing individual and all errors, and the
getAllErrorsmethod.packages/web/src/lib/error-utils.js (4)
22-36: LGTM - Robust API error parsing with fallback.The function correctly handles JSON parsing failures and provides a sensible fallback error structure.
136-154: LGTM - Clean transport error handling.The function correctly separates transport error handling from domain errors and provides appropriate defaults.
163-192: LGTM - Good unified error handler for backward compatibility.The function correctly routes different error types to their appropriate handlers while maintaining a consistent API.
199-211: LGTM - Simple utility functions.Both
parseErrorandisErrorCodeare clean, focused utilities that serve their documented purpose.docs/error-handling-guide.md (2)
1-57: LGTM - Excellent documentation for the error system.The overview and error type sections clearly explain the domain/transport/validation error distinction with helpful code examples.
200-218: LGTM - Clear best practices section.The DO/DON'T format effectively communicates the intended usage patterns and common pitfalls to avoid.
packages/web/src/main.jsx (1)
5-5: LGTM! Clean root-level error boundary integration.The AppErrorBoundary wrapping provides centralized error handling for the entire routing tree, aligning well with the new error handling architecture.
Also applies to: 13-17
packages/web/src/components/ErrorBoundary.jsx (3)
20-23: LGTM! Correct handling of non-reactive error boundary parameters.The eslint-disable directives are appropriate here. The ErrorBoundary fallback function receives
errorandresetas plain function parameters, not reactive SolidJS props, so accessing them directly is correct.
95-121: LGTM! Well-structured error boundary with proper normalization.The implementation correctly:
- Normalizes all errors through the centralized error system
- Logs unknown/programmer errors for monitoring (with TODO for service integration)
- Supports custom fallback and onError hooks
- Accesses props directly without destructuring (correct for SolidJS reactivity)
127-151: LGTM! Clean section-level error boundary wrapper.Provides appropriate scoped error UI for smaller sections while reusing the core AppErrorBoundary logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/web/src/lib/form-errors.js (1)
207-213:thisbinding dependency creates destructuring gotcha.The
handleErrormethod relies onthis.setFieldErrorandthis.setGlobalError, which fails if the method is destructured:const { handleError } = createFormErrorSignals(createSignal); handleError(error); // TypeError: this is undefinedConsider capturing the methods in the closure to make the API more robust:
🔎 Proposed refactor to eliminate `this` binding
export function createFormErrorSignals(createSignal) { const [fieldErrors, setFieldErrors] = createSignal({}); const [globalError, _setGlobalErrorSignal] = createSignal(''); + // Define methods that will be returned + const setFieldError = (field, message) => { + if (field && message) { + setFieldErrors(prev => ({ ...prev, [field]: message })); + } + }; + + const setGlobalError = (message) => { + _setGlobalErrorSignal(message || ''); + }; + return { /** * Reactive signal for field errors: { [fieldName]: errorMessage } */ fieldErrors, /** * Reactive signal for global form error */ globalError, /** * Set error for a specific field * @param {string} field - Field name * @param {string} message - Error message */ - setFieldError(field, message) { - if (field && message) { - setFieldErrors(prev => ({ ...prev, [field]: message })); - } - }, + setFieldError, /** * Clear error for a specific field * @param {string} field - Field name */ clearFieldError(field) { setFieldErrors(prev => { const next = { ...prev }; delete next[field]; return next; }); }, /** * Clear all field errors */ clearFieldErrors() { setFieldErrors({}); }, /** * Set global form error * @param {string} message - Error message */ - setGlobalError(message) { - _setGlobalErrorSignal(message || ''); - }, + setGlobalError, /** * Clear global error */ clearGlobalError() { _setGlobalErrorSignal(''); }, /** * Clear all errors (both field and global) */ clearAll() { setFieldErrors({}); _setGlobalErrorSignal(''); }, /** * Handle an error using handleFormError * @param {DomainError|TransportError} error - Error to handle */ handleError(error) { handleFormError( error, - (field, message) => this.setFieldError(field, message), - message => this.setGlobalError(message), + setFieldError, + setGlobalError, ); }, }; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/shared/src/errors/__tests__/normalize.test.ts(1 hunks)packages/web/src/lib/error-utils.js(1 hunks)packages/web/src/lib/form-errors.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared/src/errors/tests/normalize.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/web/src/lib/form-errors.jspackages/web/src/lib/error-utils.js
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx,js,ts}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Import stores directly where needed instead of passing values through multiple components
When you need to compute a value based on props or state in SolidJS, usecreateMemoto ensure it updates reactively
For complex state or state objects in SolidJS, use Solid'screateStorefor better performance and reactivity
You may create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Files:
packages/web/src/lib/form-errors.jspackages/web/src/lib/error-utils.js
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/web/src/lib/form-errors.jspackages/web/src/lib/error-utils.js
packages/web/src/**/*.{jsx,tsx,js,ts,css}
📄 CodeRabbit inference engine (.cursorrules)
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Files:
packages/web/src/lib/form-errors.jspackages/web/src/lib/error-utils.js
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
UsecreateMemofor derived values to ensure they update reactively
Files:
packages/web/src/lib/form-errors.jspackages/web/src/lib/error-utils.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/web/src/lib/form-errors.jspackages/web/src/lib/error-utils.js
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/web/src/primitives/**/*.{js,ts} : Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts,css} : Ensure browser compatibility for all frontend code (Safari is usually problematic)
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Ensure browser compatibility for all frontend code (Safari is usually problematic)
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts} : You may create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
🧬 Code graph analysis (1)
packages/web/src/lib/error-utils.js (4)
packages/shared/src/errors/index.ts (6)
validateErrorResponse(59-59)isDomainError(56-56)normalizeError(56-56)isTransportError(56-56)AUTH_ERRORS(24-24)USER_ERRORS(28-28)packages/shared/src/errors/validate.ts (1)
validateErrorResponse(12-47)packages/shared/src/errors/normalize.ts (3)
isDomainError(26-33)normalizeError(46-114)isTransportError(38-40)packages/shared/src/errors/domains/domain.ts (2)
AUTH_ERRORS(7-28)USER_ERRORS(137-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (8)
packages/web/src/lib/error-utils.js (6)
1-16: LGTM!Clean imports and clear documentation. The centralized error handling approach is well-structured.
23-37: LGTM!Proper error handling for malformed API responses with appropriate fallback behavior.
96-126: LGTM!Proper use of centralized error codes and clean separation of concerns for navigation, state updates, and user notifications.
137-155: LGTM!Appropriate handling for transport errors with sensible defaults and clean structure.
164-193: LGTM!Well-structured unified error handler providing backward compatibility while leveraging the new typed error system.
200-212: LGTM!Clean utility functions. The optional chaining in
isErrorCodeis well-supported in modern browsers and provides defensive null checking.packages/web/src/lib/form-errors.js (2)
1-55: LGTM!Comprehensive form error handling with proper support for single-field, multi-field, and global errors. Clean separation of validation and transport error handling.
62-126: LGTM!Clean Map-based state manager with comprehensive helper methods. Appropriate for non-reactive form error handling.
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (2)
packages/workers/src/routes/avatars.js (2)
217-221: Same semantic mismatch: DB_ERROR for R2 operations.This catch block has the same issue as lines 175-179. The try block (lines 190-214) only performs R2 storage operations, yet errors are wrapped as
SYSTEM_ERRORS.DB_ERROR.
243-247: Same semantic mismatch: DB_ERROR for R2 operations.This catch block has the same issue as lines 175-179 and 217-221. The try block (lines 232-239) only performs R2 storage operations, yet errors are wrapped as
SYSTEM_ERRORS.DB_ERROR.
🧹 Nitpick comments (30)
packages/workers/src/middleware/__tests__/cors.test.js (1)
9-106: Consider expanding test coverage for completeness.The test suite provides good baseline coverage, but could be enhanced with additional test cases:
- Other allowed methods (PUT, PATCH, DELETE) in preflight requests
- Other allowed headers (X-File-Name, X-Requested-With, Accept, Origin, User-Agent)
- Requests with missing origin header
- Multiple origins in ALLOWED_ORIGINS (comma-separated)
These additions would provide more comprehensive coverage of the middleware configuration shown in the relevant code snippet.
packages/workers/src/routes/google-drive.js (2)
245-252: Consider providing field-specific validation errors.The current validation checks all three fields together and returns a generic error for
'fileId/projectId/studyId'. For better client-side handling, consider validating each field individually to indicate exactly which field(s) are missing.Example of field-specific validation
- if (!fileId || !projectId || !studyId) { - const error = createValidationError( - 'fileId/projectId/studyId', - VALIDATION_ERRORS.FIELD_REQUIRED.code, - null, - 'required', - ); - return c.json(error, error.statusCode); - } + const missingFields = []; + if (!fileId) missingFields.push('fileId'); + if (!projectId) missingFields.push('projectId'); + if (!studyId) missingFields.push('studyId'); + + if (missingFields.length > 0) { + const error = createValidationError( + missingFields.join(', '), + VALIDATION_ERRORS.FIELD_REQUIRED.code, + null, + 'required', + ); + return c.json(error, error.statusCode); + }
150-154: Consider extracting duplicate "not connected" error creation.The same error creation pattern appears at lines 150-154 and 258-262. Extracting this to a helper would reduce duplication and ensure consistency.
Proposed helper function
/** * Creates a standardized "Google not connected" error */ function createGoogleNotConnectedError() { return createDomainError(AUTH_ERRORS.INVALID, { context: 'google_not_connected', code: 'GOOGLE_NOT_CONNECTED', }); }Then use it in both places:
if (!tokens?.accessToken) { - const error = createDomainError(AUTH_ERRORS.INVALID, { - context: 'google_not_connected', - code: 'GOOGLE_NOT_CONNECTED', - }); + const error = createGoogleNotConnectedError(); return c.json(error, error.statusCode); }Also applies to: 258-262
packages/workers/src/routes/account-merge.js (5)
119-122: Consider user enumeration risk in error response.Returning
{ email: normalizedEmail }in the NOT_FOUND error could enable user enumeration. While this endpoint is protected byrequireAuth, you may want to return a generic error message without confirming whether the email exists, or ensure the frontend doesn't expose this detail to the user.
179-183: Avoid leaking internal error details to client.Including
originalError: emailResult.error?.messagein the response payload may expose internal system details to the client. Consider logging this server-side only (which is already done on line 172) and returning a sanitized error without the original error message.Proposed fix
const error = createDomainError(SYSTEM_ERRORS.EMAIL_SEND_FAILED, { operation: 'send_merge_verification', - originalError: emailResult.error?.message, });
222-228: Consider separate validation for each field.Using
'mergeToken/code'as a composite field name is unconventional. For better error specificity, consider validatingmergeTokenandcodeseparately, which would help the frontend identify exactly which field needs attention.
268-272: Consider using a more appropriate error code.
USER_ERRORS.NOT_FOUNDis semantically intended for user entities, but here it's used for a merge request. Consider adding aRESOURCE_NOT_FOUNDor similar error code for non-user entities to improve error clarity and consistency.
570-574: Avoid exposing database error details to client.Similar to the email error, including
originalError: err.messagecould expose database schema or internal implementation details. The error is already logged on line 569 for debugging purposes.Proposed fix
const dbError = createDomainError(SYSTEM_ERRORS.DB_ERROR, { operation: 'merge_accounts', - originalError: err.message, });packages/workers/src/routes/pdfs.js (2)
97-101: Consider using a more specific error type for R2 operations.
SYSTEM_ERRORS.DB_ERRORis used here, but this is an R2 bucket list operation, not a database query. If aSYSTEM_ERRORS.STORAGE_ERRORor similar exists in your error codes, it would be more semantically accurate. Otherwise,SYSTEM_ERRORS.INTERNAL_ERROR(used in the download/delete catch blocks) might be more appropriate for consistency.
249-254: Consider using a generic user-facing message.Passing
error.messagedirectly as themessageOverridecould expose internal implementation details to clients. Consider using the default message fromFILE_ERRORS.UPLOAD_FAILEDwhile keepingoriginalErrorin details for server-side logging.Proposed fix
const uploadError = createDomainError( FILE_ERRORS.UPLOAD_FAILED, { operation: 'upload_pdf', originalError: error.message }, - error.message, );packages/workers/src/routes/__tests__/contact.test.js (1)
208-223: Consider isolating rate limit state for more deterministic tests.Allowing both 400 and 429 status codes accommodates rate limiter interference, but this means the test may pass without actually validating the JSON parsing error path. If feasible, consider resetting rate limiter state in
beforeEachor using a unique IP to ensure this test reliably hits the 400 path.packages/workers/src/routes/contact.js (1)
86-109: String-based error code detection is fragile.The logic relies on checking if
errorMessage.includes('expected'),includes('100'),includes('2000'), etc. This couples the code to specific Zod message formats and string literals in your schema, which may break silently if messages change.Consider using Zod's issue properties directly:
firstIssue.code(e.g.,'too_small','too_big','invalid_string')firstIssue.typefor discriminating error typesAdditionally, mutating
error.messageon line 108 after creation is a code smell. Consider passing the message tocreateValidationErrorif the API supports it, or restructuring to avoid mutation.Example using Zod issue codes
// Determine error code from Zod's issue.code let errorCode = VALIDATION_ERRORS.INVALID_INPUT.code; switch (firstIssue.code) { case 'invalid_type': case 'too_small': if (firstIssue.minimum === 1) { errorCode = VALIDATION_ERRORS.FIELD_REQUIRED.code; } else { errorCode = VALIDATION_ERRORS.FIELD_TOO_SHORT.code; } break; case 'too_big': errorCode = VALIDATION_ERRORS.FIELD_TOO_LONG.code; break; case 'invalid_string': errorCode = VALIDATION_ERRORS.FIELD_INVALID_FORMAT.code; break; }packages/workers/src/middleware/__tests__/rateLimit.test.js (3)
49-60: Consider validating the Retry-After header.The test checks
body.retryAfterbut doesn't validate theRetry-Afterresponse header. According to the middleware implementation, this header should be set when rate limiting occurs.Add header validation
expect(res.status).toBe(429); const body = await res.json(); expect(body.error).toBe('Too many requests'); expect(body.retryAfter).toBeDefined(); +expect(res.headers.get('Retry-After')).toBeDefined();
73-75: Strengthen header validation by checking values.The test only verifies headers are defined but doesn't validate their values. This misses potential bugs in the header calculations.
Validate header values
expect(res.headers.get('X-RateLimit-Limit')).toBe('10'); -expect(res.headers.get('X-RateLimit-Remaining')).toBeDefined(); -expect(res.headers.get('X-RateLimit-Reset')).toBeDefined(); +expect(res.headers.get('X-RateLimit-Remaining')).toBe('9'); // After 1 request +const reset = parseInt(res.headers.get('X-RateLimit-Reset')); +expect(reset).toBeGreaterThan(Math.floor(Date.now() / 1000));
9-253: Consider adding tests for remaining middleware options.The test suite provides solid coverage of the core functionality. However, two middleware options are not tested:
skipFailedRequests- Should decrement count for 4xx/5xx responseskeyPrefix- Should allow prefixing keys for namespacingThese are optional enhancements that would improve coverage if you plan to rely on these features.
Example test for skipFailedRequests
it('should skip failed requests when configured', async () => { const app = new Hono(); app.use('*', rateLimit({ limit: 2, windowMs: 60000, skipFailedRequests: true })); app.get('/test', c => { const shouldFail = c.req.query('fail'); if (shouldFail) { return c.json({ error: 'Bad request' }, 400); } return c.json({ message: 'success' }); }); const uniqueIP = `192.168.7.${testCounter}`; // Make 2 failed requests (should not count) for (let i = 0; i < 2; i++) { const res = await app.request('/test?fail=1', { headers: { 'CF-Connecting-IP': uniqueIP }, }); expect(res.status).toBe(400); } // Should still be able to make 2 successful requests for (let i = 0; i < 2; i++) { const res = await app.request('/test', { headers: { 'CF-Connecting-IP': uniqueIP }, }); expect(res.status).toBe(200); } // 3rd successful request should be blocked const res = await app.request('/test', { headers: { 'CF-Connecting-IP': uniqueIP }, }); expect(res.status).toBe(429); });packages/workers/src/middleware/__tests__/auth.test.js (1)
100-119: Consider testing additional error scenarios.The test correctly verifies that errors during auth initialization are handled gracefully. However, consider adding a test case for errors thrown by
getSessionitself (rather than justcreateAuth) to ensure complete error handling coverage.Optional: Additional test case for getSession errors
it('should handle getSession errors gracefully', async () => { const { createAuth } = await import('../../auth/config.js'); createAuth.mockImplementationOnce(() => ({ api: { getSession: vi.fn(async () => { throw new Error('Session fetch failed'); }), }, })); const app = new Hono(); app.use('*', requireAuth); app.get('/protected', c => c.json({ message: 'success' })); const res = await app.request('/protected', { headers: { 'x-test-user-id': 'user-123', }, }); expect(res.status).toBe(401); const body = await res.json(); expect(body.error).toBe('Authentication required'); });packages/workers/src/routes/email.js (1)
29-36: Consider using Zod schema validation.The validation error handling correctly uses the centralized error utilities. However, the manual field checking doesn't align with the coding guideline to "Use Zod for schema and input validation on the backend."
Recommended: Zod schema for email payload validation
As per coding guidelines, consider replacing manual validation with a Zod schema:
import { z } from 'zod'; const emailPayloadSchema = z.object({ to: z.string().email(), subject: z.string().min(1), html: z.string().optional(), text: z.string().optional(), }).refine( data => data.html || data.text, { message: 'Either html or text content is required' } );Then validate the payload:
try { const payload = await c.req.json(); - if (!payload?.to) { - const error = createValidationError( - 'to', - VALIDATION_ERRORS.FIELD_REQUIRED.code, - null, - 'required', - ); - return c.json(error, error.statusCode); - } + const result = emailPayloadSchema.safeParse(payload); + if (!result.success) { + // Transform Zod errors to domain validation errors + const firstError = result.error.errors[0]; + const error = createValidationError( + firstError.path.join('.'), + VALIDATION_ERRORS.FIELD_REQUIRED.code, + null, + firstError.message, + ); + return c.json(error, error.statusCode); + }This would provide comprehensive validation (email format, required fields, content validation) and align with the coding guidelines.
Based on coding guidelines.
packages/workers/src/middleware/__tests__/csrf.test.js (3)
175-198: Consider simplifying the env override pattern.The wrapper middleware with
.finally()to restorec.envworks but is somewhat fragile and assumes all code paths return promises. Consider using Hono's request context API more directly, or creating a test helper function to encapsulate this pattern for reusability.Alternative approach with helper function
+// Test helper for custom env +function createAppWithEnv(customEnv) { + const app = new Hono(); + app.use('*', (c, next) => { + c.env = { ...c.env, ...customEnv }; + return requireTrustedOrigin(c, next); + }); + return app; +} + it('should respect env ALLOWED_ORIGINS', async () => { - const app = new Hono(); - // Create middleware with custom env - const customEnv = { ALLOWED_ORIGINS: 'https://custom.com' }; - app.use('*', (c, next) => { - // Temporarily override env for this request - const originalEnv = c.env; - c.env = { ...c.env, ...customEnv }; - return requireTrustedOrigin(c, next).finally(() => { - c.env = originalEnv; - }); - }); + const app = createAppWithEnv({ ALLOWED_ORIGINS: 'https://custom.com' }); app.post('/test', c => c.json({ message: 'success' }));
78-92: Consider testing the Origin/Referer precedence behavior.While the current test verifies that Referer works when Origin is absent, there's no explicit test for when both headers are present. The middleware should prefer Origin over Referer in such cases, and a test would document this behavior.
Suggested additional test case
it('should prefer Origin over Referer when both are present', async () => { const app = new Hono(); app.use('*', requireTrustedOrigin); app.post('/test', c => c.json({ message: 'success' })); // Trusted origin but untrusted referer const res = await app.request('/test', { method: 'POST', headers: { 'content-type': 'application/json', origin: 'http://localhost:5173', referer: 'https://evil.com/page', }, }); expect(res.status).toBe(200); // Should allow based on trusted Origin });
112-126: Optional: Add error message assertion for consistency.For consistency with the untrusted Origin test (lines 94-110), consider asserting the error message here as well.
Suggested addition
expect(res.status).toBe(403); + const body = await res.json(); + expect(body.error).toBe('Untrusted Origin'); });packages/workers/src/__tests__/health.test.js (1)
7-10: Unused import:HonoThe
Honoimport on line 8 is not used in this file sinceappis dynamically imported from../index.jsinbeforeAll.Proposed fix
import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; -import { Hono } from 'hono'; import { env, createExecutionContext, waitOnExecutionContext } from 'cloudflare:test'; import { resetTestDatabase, json, fetchApp } from './helpers.js';packages/workers/src/routes/billing/checkout.js (1)
26-30: Consider usingcreateValidationErrorfor field validation consistency.Looking at
packages/workers/src/routes/billing/index.js(lines 125-130), similar tier/interval validation usescreateValidationError:const error = createValidationError( 'tier', VALIDATION_ERRORS.INVALID_INPUT.code, tier, 'invalid_tier', );For consistency across billing routes, consider using
createValidationErrorhere as well.Proposed fix
-import { createDomainError, VALIDATION_ERRORS } from '@corates/shared'; +import { createValidationError, VALIDATION_ERRORS } from '@corates/shared';if (!priceId) { - const error = createDomainError(VALIDATION_ERRORS.INVALID_INPUT, { - field: 'tier/interval', - value: `${tier}/${interval}`, - }); + const error = createValidationError( + 'tier/interval', + VALIDATION_ERRORS.INVALID_INPUT.code, + `${tier}/${interval}`, + 'invalid_tier_interval', + ); throw error; }packages/workers/src/routes/__tests__/database.test.js (1)
94-101: Consider renaming this test for clarity.The test is named "should require authentication" but expects status 200 because authentication is globally mocked to always succeed. This doesn't actually verify that authentication is required—it verifies that the mocked authentication flow works.
Consider renaming to something like "should work with mocked authentication" or remove this test if it's not adding meaningful coverage.
packages/workers/src/__tests__/setup.js (1)
10-25: Consider simplifying the Postmark mock.The mock defines both instance properties (lines 14-15) and class methods (lines 17-22) for
sendEmailandsendEmailBatch. This duplication is unnecessary—you can remove either the instance property assignments in the constructor or the class method definitions.Simplified mock (remove instance properties)
vi.mock('postmark', () => { return { Client: class { - constructor() { - this.sendEmail = vi.fn(() => Promise.resolve({ Message: 'mock' })); - this.sendEmailBatch = vi.fn(() => Promise.resolve({ Message: 'mock' })); - } + constructor() {} sendEmail() { return Promise.resolve({ Message: 'mock' }); } sendEmailBatch() { return Promise.resolve({ Message: 'mock' }); } }, }; });packages/workers/src/routes/billing/__tests__/webhooks.test.js (3)
50-68:createWebhookRequesthelper returns unusedeventvariable in most call sites.The helper returns
{ testEnv, event }but most tests only destructuretestEnv. This is not a defect, but theeventreturn value is only useful for the signature verification test. Consider if this is intentional or if the helper could be simplified.
71-95: Test assertion could be more precise.The signature verification test has flexible assertions that accept either domain error codes or plain error messages. While this provides resilience, it may mask issues if the error shape changes unexpectedly. Consider tightening the assertion to explicitly expect the domain error format.
🔎 Suggested improvement
} catch (error) { - // Domain error will have code property, regular error will have message - expect(error.code || error.message).toBeDefined(); - if (error.code) { - expect(error.code).toMatch(/AUTH_INVALID/); - } else if (error.message) { - expect(error.message).toMatch(/signature/i); - } + // Webhook handler throws domain error for signature verification failures + expect(error.code).toBeDefined(); + expect(error.code).toMatch(/AUTH_INVALID/); }
167-203: Test verifies event was received but doesn't verify DB state.Unlike other tests in this file,
customer.subscription.createdonly assertsresult.received === truewithout verifying that a subscription was actually created in the database.🔎 Suggested addition
const result = await handleWebhook(testEnv, 'raw-body', 'valid-signature'); expect(result.received).toBe(true); + + // Verify subscription was processed (or add appropriate DB verification) + // Note: subscription.created may require additional setup depending on handler logicpackages/workers/src/routes/__tests__/members.test.js (1)
9-16: Unused import:fetchApp
fetchAppis imported from helpers but never used in this file. The file defines its ownfetchMembershelper instead.Proposed fix
import { resetTestDatabase, seedUser, seedProject, seedProjectMember, json, - fetchApp, } from '../../__tests__/helpers.js';packages/workers/src/routes/__tests__/projects.test.js (2)
9-16: Unused import:fetchApp
fetchAppis imported but not used. The file defines its ownfetchProjectshelper.Proposed fix
import { resetTestDatabase, seedUser, seedProject, seedProjectMember, json, - fetchApp, } from '../../__tests__/helpers.js';
17-78: Consider centralizing repeated mock patternsThe postmark and auth middleware mocks are duplicated between
members.test.jsandprojects.test.js. Consider extracting these to a shared test setup file for maintainability. This is optional since co-located mocks can improve test readability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
packages/web/src/lib/error-utils.js(1 hunks)packages/workers/TESTING.md(1 hunks)packages/workers/src/__tests__/admin.test.js(1 hunks)packages/workers/src/__tests__/app.test.js(1 hunks)packages/workers/src/__tests__/health.test.js(1 hunks)packages/workers/src/__tests__/helpers.js(1 hunks)packages/workers/src/__tests__/setup.js(1 hunks)packages/workers/src/db/subscriptions.js(1 hunks)packages/workers/src/middleware/__tests__/auth.test.js(1 hunks)packages/workers/src/middleware/__tests__/cors.test.js(1 hunks)packages/workers/src/middleware/__tests__/csrf.test.js(1 hunks)packages/workers/src/middleware/__tests__/rateLimit.test.js(1 hunks)packages/workers/src/routes/__tests__/contact.test.js(1 hunks)packages/workers/src/routes/__tests__/database.test.js(1 hunks)packages/workers/src/routes/__tests__/email.test.js(1 hunks)packages/workers/src/routes/__tests__/members.test.js(1 hunks)packages/workers/src/routes/__tests__/projects.test.js(1 hunks)packages/workers/src/routes/__tests__/users.test.js(1 hunks)packages/workers/src/routes/account-merge.js(9 hunks)packages/workers/src/routes/admin.js(14 hunks)packages/workers/src/routes/avatars.js(7 hunks)packages/workers/src/routes/billing/__tests__/index.test.js(1 hunks)packages/workers/src/routes/billing/__tests__/webhooks.test.js(1 hunks)packages/workers/src/routes/billing/checkout.js(2 hunks)packages/workers/src/routes/billing/index.js(5 hunks)packages/workers/src/routes/billing/portal.js(2 hunks)packages/workers/src/routes/billing/webhooks.js(3 hunks)packages/workers/src/routes/contact.js(5 hunks)packages/workers/src/routes/database.js(4 hunks)packages/workers/src/routes/email.js(2 hunks)packages/workers/src/routes/google-drive.js(10 hunks)packages/workers/src/routes/pdfs.js(14 hunks)packages/workers/src/routes/users.js(8 hunks)packages/workers/vitest.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/lib/error-utils.js
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/workers/src/middleware/__tests__/cors.test.jspackages/workers/TESTING.mdpackages/workers/src/middleware/__tests__/rateLimit.test.jspackages/workers/src/__tests__/admin.test.jspackages/workers/vitest.config.jspackages/workers/src/routes/pdfs.jspackages/workers/src/__tests__/setup.jspackages/workers/src/routes/billing/__tests__/index.test.jspackages/workers/src/routes/billing/webhooks.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/routes/avatars.jspackages/workers/src/routes/database.jspackages/workers/src/routes/__tests__/database.test.jspackages/workers/src/routes/billing/__tests__/webhooks.test.jspackages/workers/src/routes/users.jspackages/workers/src/db/subscriptions.jspackages/workers/src/routes/email.jspackages/workers/src/routes/__tests__/users.test.jspackages/workers/src/routes/billing/portal.jspackages/workers/src/middleware/__tests__/csrf.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/__tests__/contact.test.jspackages/workers/src/routes/__tests__/members.test.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/routes/admin.jspackages/workers/src/routes/account-merge.jspackages/workers/src/routes/__tests__/email.test.jspackages/workers/src/routes/billing/checkout.jspackages/workers/src/routes/contact.jspackages/workers/src/routes/billing/index.jspackages/workers/src/__tests__/health.test.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/helpers.js
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/workers/src/middleware/__tests__/cors.test.jspackages/workers/src/middleware/__tests__/rateLimit.test.jspackages/workers/src/__tests__/admin.test.jspackages/workers/vitest.config.jspackages/workers/src/routes/pdfs.jspackages/workers/src/__tests__/setup.jspackages/workers/src/routes/billing/__tests__/index.test.jspackages/workers/src/routes/billing/webhooks.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/routes/avatars.jspackages/workers/src/routes/database.jspackages/workers/src/routes/__tests__/database.test.jspackages/workers/src/routes/billing/__tests__/webhooks.test.jspackages/workers/src/routes/users.jspackages/workers/src/db/subscriptions.jspackages/workers/src/routes/email.jspackages/workers/src/routes/__tests__/users.test.jspackages/workers/src/routes/billing/portal.jspackages/workers/src/middleware/__tests__/csrf.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/__tests__/contact.test.jspackages/workers/src/routes/__tests__/members.test.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/routes/admin.jspackages/workers/src/routes/account-merge.jspackages/workers/src/routes/__tests__/email.test.jspackages/workers/src/routes/billing/checkout.jspackages/workers/src/routes/contact.jspackages/workers/src/routes/billing/index.jspackages/workers/src/__tests__/health.test.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/helpers.js
packages/workers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/workers/**/*.{js,ts}: Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/middleware/__tests__/cors.test.jspackages/workers/src/middleware/__tests__/rateLimit.test.jspackages/workers/src/__tests__/admin.test.jspackages/workers/vitest.config.jspackages/workers/src/routes/pdfs.jspackages/workers/src/__tests__/setup.jspackages/workers/src/routes/billing/__tests__/index.test.jspackages/workers/src/routes/billing/webhooks.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/routes/avatars.jspackages/workers/src/routes/database.jspackages/workers/src/routes/__tests__/database.test.jspackages/workers/src/routes/billing/__tests__/webhooks.test.jspackages/workers/src/routes/users.jspackages/workers/src/db/subscriptions.jspackages/workers/src/routes/email.jspackages/workers/src/routes/__tests__/users.test.jspackages/workers/src/routes/billing/portal.jspackages/workers/src/middleware/__tests__/csrf.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/__tests__/contact.test.jspackages/workers/src/routes/__tests__/members.test.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/routes/admin.jspackages/workers/src/routes/account-merge.jspackages/workers/src/routes/__tests__/email.test.jspackages/workers/src/routes/billing/checkout.jspackages/workers/src/routes/contact.jspackages/workers/src/routes/billing/index.jspackages/workers/src/__tests__/health.test.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/helpers.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/middleware/__tests__/cors.test.jspackages/workers/src/middleware/__tests__/rateLimit.test.jspackages/workers/src/__tests__/admin.test.jspackages/workers/vitest.config.jspackages/workers/src/routes/pdfs.jspackages/workers/src/__tests__/setup.jspackages/workers/src/routes/billing/__tests__/index.test.jspackages/workers/src/routes/billing/webhooks.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/routes/avatars.jspackages/workers/src/routes/database.jspackages/workers/src/routes/__tests__/database.test.jspackages/workers/src/routes/billing/__tests__/webhooks.test.jspackages/workers/src/routes/users.jspackages/workers/src/db/subscriptions.jspackages/workers/src/routes/email.jspackages/workers/src/routes/__tests__/users.test.jspackages/workers/src/routes/billing/portal.jspackages/workers/src/middleware/__tests__/csrf.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/__tests__/contact.test.jspackages/workers/src/routes/__tests__/members.test.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/routes/admin.jspackages/workers/src/routes/account-merge.jspackages/workers/src/routes/__tests__/email.test.jspackages/workers/src/routes/billing/checkout.jspackages/workers/src/routes/contact.jspackages/workers/src/routes/billing/index.jspackages/workers/src/__tests__/health.test.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/helpers.js
packages/workers/src/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/workers/src/**/*.{js,ts}: Use Zod for schema and input validation on the backend
Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/middleware/__tests__/cors.test.jspackages/workers/src/middleware/__tests__/rateLimit.test.jspackages/workers/src/__tests__/admin.test.jspackages/workers/src/routes/pdfs.jspackages/workers/src/__tests__/setup.jspackages/workers/src/routes/billing/__tests__/index.test.jspackages/workers/src/routes/billing/webhooks.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/routes/avatars.jspackages/workers/src/routes/database.jspackages/workers/src/routes/__tests__/database.test.jspackages/workers/src/routes/billing/__tests__/webhooks.test.jspackages/workers/src/routes/users.jspackages/workers/src/db/subscriptions.jspackages/workers/src/routes/email.jspackages/workers/src/routes/__tests__/users.test.jspackages/workers/src/routes/billing/portal.jspackages/workers/src/middleware/__tests__/csrf.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/__tests__/contact.test.jspackages/workers/src/routes/__tests__/members.test.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/routes/admin.jspackages/workers/src/routes/account-merge.jspackages/workers/src/routes/__tests__/email.test.jspackages/workers/src/routes/billing/checkout.jspackages/workers/src/routes/contact.jspackages/workers/src/routes/billing/index.jspackages/workers/src/__tests__/health.test.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/helpers.js
🧠 Learnings (7)
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: See TESTING.md for testing guidelines and best practices; do NOT add tests unless asked
Applied to files:
packages/workers/TESTING.md
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/routes/pdfs.jspackages/workers/src/routes/avatars.jspackages/workers/src/routes/database.jspackages/workers/src/routes/__tests__/database.test.jspackages/workers/src/routes/users.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/routes/pdfs.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/routes/avatars.jspackages/workers/src/routes/database.jspackages/workers/src/routes/__tests__/database.test.jspackages/workers/src/routes/users.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/src/routes/database.jspackages/workers/src/routes/users.jspackages/workers/src/routes/__tests__/users.test.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/routes/admin.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/src/routes/database.jspackages/workers/src/routes/users.jspackages/workers/src/routes/__tests__/users.test.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/routes/admin.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Zod for schema and input validation on the backend
Applied to files:
packages/workers/src/routes/database.jspackages/workers/src/routes/users.jspackages/workers/src/routes/contact.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/**/*.{js,jsx,ts,tsx} : Use Zod for schema and input validation
Applied to files:
packages/workers/src/routes/contact.js
🧬 Code graph analysis (24)
packages/workers/src/middleware/__tests__/cors.test.js (1)
packages/workers/src/middleware/cors.js (1)
createCorsMiddleware(13-35)
packages/workers/src/middleware/__tests__/rateLimit.test.js (1)
packages/workers/src/middleware/rateLimit.js (1)
rateLimit(51-114)
packages/workers/src/routes/pdfs.js (3)
packages/shared/src/errors/helpers.ts (1)
createDomainError(29-41)packages/workers/src/routes/avatars.js (3)
dbError(175-178)dbError(217-220)file(91-91)packages/workers/src/config/constants.js (2)
FILE_SIZE_LIMITS(24-28)FILE_SIZE_LIMITS(24-28)
packages/workers/src/routes/billing/webhooks.js (3)
packages/workers/src/routes/billing/checkout.js (2)
error(26-29)db(21-21)packages/workers/src/routes/billing/portal.js (3)
error(25-28)subscription(22-22)db(19-19)packages/workers/src/db/subscriptions.js (1)
updateSubscriptionByStripeId(126-138)
packages/workers/src/routes/__tests__/projects.test.js (1)
packages/workers/src/__tests__/helpers.js (5)
res(320-320)seedUser(150-185)seedProject(190-204)seedProjectMember(209-216)json(304-311)
packages/workers/src/routes/avatars.js (2)
packages/workers/src/routes/pdfs.js (4)
error(36-38)error(51-51)error(117-121)dbError(97-100)packages/shared/src/errors/helpers.ts (1)
createDomainError(29-41)
packages/workers/src/routes/database.js (4)
packages/workers/src/routes/account-merge.js (5)
dbError(570-573)c(78-78)c(219-219)c(358-358)c(587-587)packages/workers/src/routes/admin.js (3)
dbError(66-69)dbError(184-187)c(267-267)packages/workers/src/routes/pdfs.js (1)
dbError(97-100)packages/workers/src/routes/users.js (7)
dbError(123-126)dbError(167-170)dbError(203-206)dbError(284-287)error(61-66)error(141-143)error(233-233)
packages/workers/src/routes/billing/__tests__/webhooks.test.js (3)
packages/workers/src/routes/billing/webhooks.js (3)
event(59-59)subscription(121-121)db(75-75)packages/workers/src/db/subscriptions.js (8)
result(15-19)result(31-35)result(47-51)result(80-94)result(100-114)result(136-136)result(158-167)getSubscriptionByStripeSubscriptionId(46-54)packages/workers/src/db/client.js (1)
createDb(9-11)
packages/workers/src/routes/users.js (1)
packages/workers/src/routes/account-merge.js (10)
error(81-86)error(93-98)error(120-120)error(179-182)error(222-227)error(232-237)c(78-78)c(219-219)c(358-358)c(587-587)
packages/workers/src/db/subscriptions.js (1)
packages/workers/src/db/schema.js (2)
subscriptions(109-124)subscriptions(109-124)
packages/workers/src/routes/email.js (3)
packages/workers/src/routes/database.js (1)
error(57-62)packages/workers/src/routes/pdfs.js (12)
error(36-38)error(51-51)error(117-121)error(128-132)error(148-152)error(158-162)error(175-179)error(183-187)error(192-196)error(206-210)error(218-222)error(268-272)packages/workers/src/routes/users.js (3)
error(61-66)error(141-143)error(233-233)
packages/workers/src/routes/__tests__/users.test.js (1)
packages/workers/src/__tests__/helpers.js (6)
resetTestDatabase(10-145)res(320-320)seedUser(150-185)json(304-311)seedProject(190-204)seedProjectMember(209-216)
packages/workers/src/routes/billing/portal.js (4)
packages/workers/src/routes/account-merge.js (8)
error(81-86)error(93-98)error(120-120)error(179-182)error(222-227)error(232-237)error(243-248)error(268-271)packages/workers/src/routes/billing/checkout.js (1)
error(26-29)packages/workers/src/routes/billing/index.js (2)
error(126-131)error(185-190)packages/workers/src/routes/billing/webhooks.js (1)
error(68-71)
packages/workers/src/middleware/__tests__/csrf.test.js (1)
packages/workers/src/middleware/csrf.js (1)
requireTrustedOrigin(14-54)
packages/workers/src/__tests__/app.test.js (1)
packages/workers/src/__tests__/helpers.js (7)
fetchApp(316-323)testEnv(317-317)ctx(318-318)req(319-319)res(320-320)json(304-311)text(305-305)
packages/workers/src/routes/__tests__/contact.test.js (2)
packages/workers/src/__tests__/helpers.js (2)
res(320-320)json(304-311)packages/workers/src/routes/contact.js (2)
env(56-56)body(59-59)
packages/workers/src/routes/__tests__/members.test.js (1)
packages/workers/src/__tests__/helpers.js (5)
res(320-320)seedUser(150-185)seedProject(190-204)seedProjectMember(209-216)json(304-311)
packages/workers/src/middleware/__tests__/auth.test.js (2)
packages/workers/src/middleware/auth.js (1)
requireAuth(34-53)packages/workers/src/auth/config.js (1)
createAuth(11-302)
packages/workers/src/routes/admin.js (5)
packages/workers/src/routes/account-merge.js (21)
dbError(570-573)error(81-86)error(93-98)error(120-120)error(179-182)error(222-227)error(232-237)error(243-248)error(268-271)error(280-285)error(292-297)error(303-308)error(361-366)error(379-382)error(391-396)error(402-407)error(414-419)c(78-78)c(219-219)c(358-358)c(587-587)packages/workers/src/routes/billing/index.js (1)
dbError(56-59)packages/workers/src/routes/database.js (2)
dbError(44-47)dbError(87-90)packages/workers/src/routes/pdfs.js (1)
dbError(97-100)packages/workers/src/routes/users.js (4)
dbError(123-126)dbError(167-170)dbError(203-206)dbError(284-287)
packages/workers/src/routes/billing/checkout.js (3)
packages/workers/src/routes/billing/index.js (2)
error(126-131)error(185-190)packages/workers/src/routes/billing/portal.js (1)
error(25-28)packages/workers/src/routes/billing/webhooks.js (3)
error(68-71)tier(113-113)tier(146-146)
packages/workers/src/routes/contact.js (1)
packages/workers/src/routes/account-merge.js (19)
error(81-86)error(93-98)error(120-120)error(179-182)error(222-227)error(232-237)error(243-248)error(268-271)error(280-285)error(292-297)error(303-308)error(361-366)error(379-382)error(391-396)error(402-407)c(78-78)c(219-219)c(358-358)c(587-587)
packages/workers/src/routes/billing/index.js (3)
packages/workers/src/routes/pdfs.js (1)
dbError(97-100)packages/workers/src/routes/billing/checkout.js (2)
error(26-29)createCheckoutSession(19-81)packages/workers/src/routes/billing/webhooks.js (4)
error(68-71)tier(113-113)tier(146-146)handleWebhook(55-105)
packages/workers/src/__tests__/health.test.js (2)
packages/workers/src/__tests__/admin.test.js (5)
app(46-46)res(261-261)ctx(259-259)req(260-260)text(250-250)packages/workers/src/__tests__/helpers.js (8)
resetTestDatabase(10-145)res(320-320)fetchApp(316-323)json(304-311)testEnv(317-317)ctx(318-318)req(319-319)text(305-305)
packages/workers/src/__tests__/helpers.js (2)
packages/workers/src/__tests__/admin.test.js (6)
run(51-51)res(261-261)text(250-250)app(46-46)ctx(259-259)req(260-260)packages/workers/src/routes/__tests__/users.test.js (4)
res(73-73)app(51-51)ctx(64-64)req(65-72)
🪛 markdownlint-cli2 (0.18.1)
packages/workers/TESTING.md
137-137: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates
- GitHub Check: Workers Builds: corates-workers-prod
| it('should block unauthenticated requests with 401', async () => { | ||
| const app = new Hono(); | ||
| app.use('*', requireAuth); | ||
| app.get('/protected', c => c.json({ message: 'success' })); | ||
|
|
||
| const res = await app.request('/protected'); | ||
|
|
||
| expect(res.status).toBe(401); | ||
| const body = await res.json(); | ||
| expect(body.error).toBe('Authentication required'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if centralized error codes/structures exist in the codebase
echo "=== Searching for centralized error patterns ==="
rg -n "DomainError|TransportError|ERROR_CODES" packages/workers/src/ || echo "No centralized error patterns found"
# Also check for any error handling patterns
echo ""
echo "=== Checking middleware implementation ==="
fd -t f "auth" packages/workers/src/middleware/ --exec cat -n {} \;
# Check if Better-Auth is being used
echo ""
echo "=== Checking for Better-Auth usage ==="
rg -n "better-auth|BetterAuth" packages/workers/src/ || echo "No Better-Auth found"Repository: InfinityBowman/corates
Length of output: 20974
Middleware should use centralized error system, not plain error strings.
The authentication middleware returns { error: 'Authentication required' }, but all routes in the codebase use createDomainError() with centralized error constants. The test correctly validates the current behavior, but the middleware implementation should be updated to follow the established pattern.
Update the middleware to use createDomainError() with AUTH_ERRORS constant (as used throughout routes/), then update this test accordingly.
🤖 Prompt for AI Agents
In packages/workers/src/middleware/__tests__/auth.test.js around lines 67-77,
the test and middleware currently use a plain error object with { error:
'Authentication required' }; update the middleware to return the centralized
domain error by calling createDomainError(AUTH_ERRORS.AUTHENTICATION_REQUIRED)
(import AUTH_ERRORS and createDomainError) instead of the plain string object,
and then change this test to assert against the standardized error shape/value
produced by createDomainError(AUTH_ERRORS.AUTHENTICATION_REQUIRED) rather than
the literal 'Authentication required'.
…https://github.com/InfinityBowman/corates into 110-migrate-frontend-to-use-error-codes-like-backend
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
packages/workers/src/routes/__tests__/projects.test.js (1)
460-508: Tighten assertion to match actual behavior.The comment on line 506 states "Non-members get 403 ACCESS_DENIED, not 404", but the assertion on line 507 permissively accepts either status code. The test should definitively assert the expected behavior.
🔎 Proposed fix
Based on the comment and the route's actual behavior returning 403 for non-members:
- // Non-members get 403 ACCESS_DENIED, not 404 - expect([403, 404]).toContain(res.status); + // Non-members get 403 ACCESS_DENIED + expect(res.status).toBe(403);Additionally, consider updating the test name for clarity:
- it('should return 404 for non-members', async () => { + it('should return 403 for non-members', async () => {
🧹 Nitpick comments (14)
packages/workers/src/routes/__tests__/projects.test.js (4)
124-126: Consider tightening the error assertion.The fallback pattern
body.code || body.errorsuggests uncertainty about the error response shape. Since this PR introduces centralized error handling with domain errors that have acodefield, consider asserting directly onbody.codefor consistency and to catch regressions if the error shape changes unexpectedly.🔎 Proposed simplification
- // Domain errors have 'code' field, not 'error' - expect(body.code || body.error).toBeDefined(); - expect(body.code).toMatch(/NOT_FOUND/); + expect(body.code).toBeDefined(); + expect(body.code).toMatch(/NOT_FOUND/);
286-288: Consider tightening the error assertion.Similar to the GET tests, the fallback pattern
body.code || body.errorcould be simplified to assert directly onbody.codefor consistency with the centralized error handling system.🔎 Proposed simplification
- // Domain errors have 'code' field, not 'error' - expect(body.code || body.error).toBeDefined(); - expect(body.code).toMatch(/VALIDATION/); + expect(body.code).toBeDefined(); + expect(body.code).toMatch(/VALIDATION/);
455-457: Consider tightening the error assertion.The fallback pattern
body.code || body.errorcould be simplified to assert directly onbody.codefor consistency with the centralized error handling system.🔎 Proposed simplification
- // Domain errors have 'code' field, not 'error' - expect(body.code || body.error).toBeDefined(); - expect(body.code).toMatch(/FORBIDDEN/); + expect(body.code).toBeDefined(); + expect(body.code).toMatch(/FORBIDDEN/);
613-615: Consider tightening the error assertion.The fallback pattern
body.code || body.errorcould be simplified to assert directly onbody.codefor consistency with the centralized error handling system.🔎 Proposed simplification
- // Domain errors have 'code' field, not 'error' - expect(body.code || body.error).toBeDefined(); - expect(body.code).toMatch(/FORBIDDEN/); + expect(body.code).toBeDefined(); + expect(body.code).toMatch(/FORBIDDEN/);packages/workers/src/middleware/__tests__/auth.test.js (3)
89-110: Consider clarifying the error simulation.The test mocks
createAuthto throw an error but still includes authentication headers (x-test-user-id) in the request. Since the auth service is unavailable, these headers are irrelevant. For clarity, consider removing the headers to emphasize that the test focuses on service failure rather than authentication state.Suggested clarification
const res = await app.request('/protected', { - headers: { - 'x-test-user-id': 'user-123', - }, });
137-157: Consider using a more realistic test setup.The test manually constructs a mock context object to test
getAuthin isolation. While this works, it would be more realistic to testgetAuththrough the actual middleware flow (e.g., withauthMiddlewarethat doesn't block unauthenticated requests).Alternative approach using authMiddleware
it('should return null values when not authenticated', async () => { const app = new Hono(); app.use('*', authMiddleware); // Use the non-blocking middleware app.get('/test', c => { const auth = getAuth(c); return c.json(auth); }); const res = await app.request('/test'); // No auth headers expect(res.status).toBe(200); const body = await res.json(); expect(body.user).toBeNull(); expect(body.session).toBeNull(); });This would also provide test coverage for
authMiddleware, which is currently untested.
1-158: Consider adding test coverage for authMiddleware.The test suite thoroughly covers
requireAuthandgetAuth, butauthMiddleware(the non-blocking variant) lacks dedicated tests. SinceauthMiddlewarehas different behavior (sets user/session to null on error instead of blocking), it would benefit from explicit test cases covering:
- Authenticated requests properly set context
- Unauthenticated requests set null values and continue
- Auth errors are caught and logged without blocking the request
packages/workers/src/routes/billing/index.js (1)
126-134: Consider using Zod for request validation.The manual validation works but doesn't align with the coding guideline to use Zod for schema and input validation on the backend. Consider defining a Zod schema for the request body to validate both tier and interval together.
As per coding guidelines, Zod should be used for validation on the backend.
Example Zod schema approach
import { z } from 'zod'; const checkoutSchema = z.object({ tier: z.enum(['pro', 'team', 'enterprise']), interval: z.enum(['monthly', 'yearly']).default('monthly'), }); // In the handler: const parsed = checkoutSchema.safeParse(body); if (!parsed.success) { // Convert Zod errors to validation errors }packages/workers/src/routes/google-drive.js (2)
207-211: Consider safer access to error.message.The error handling correctly creates a domain error with operation context. However, line 209 directly accesses
error.message, which could be undefined if the caught error isn't a standard Error object.Suggested improvement for consistency
const systemError = createDomainError(SYSTEM_ERRORS.DB_ERROR, { operation: 'disconnect_google_account', - originalError: error.message, + originalError: error?.message || String(error), });This ensures a meaningful error value is always provided.
237-255: Good validation error handling.The validation errors properly use
createValidationErrorwith appropriate error codes. The JSON parsing error (lines 237-243) and missing fields error (lines 249-255) both provide clear context.One minor improvement: line 250 combines all three fields as
'fileId/projectId/studyId'. If the shared error utilities support it, consider validating each field separately for more precise error reporting:if (!fileId) { const error = createValidationError('fileId', VALIDATION_ERRORS.FIELD_REQUIRED.code, null, 'required'); return c.json(error, error.statusCode); } if (!projectId) { /* ... */ } if (!studyId) { /* ... */ }packages/workers/src/durable-objects/__tests__/EmailQueue.test.js (2)
212-234: Conditional check may silently skip assertions if storage is empty.The
if (emails.size > 0)block contains the actual assertions. If storage is unexpectedly empty, the test passes without verifying anything. Consider adding an explicit assertion before the conditional.🔎 Suggested improvement
await runInDurableObject(stub, async (instance, state) => { const emails = await state.storage.list({ prefix: 'email:' }); + expect(emails.size).toBeGreaterThan(0); // Ensure we have emails to test if (emails.size > 0) { const emailEntries = Array.from(emails.entries()); - let emailRecord = emailEntries[0][1]; + const emailRecord = emailEntries[0][1];
348-366: Same conditional pattern concern as retry tests.Consider adding an explicit assertion that
emails.size > 0before the conditional block to prevent silent test passes.packages/workers/src/__tests__/app.test.js (2)
79-138: Consider more specific test assertions for better error detection.Several route mounting tests accept multiple status codes (e.g., lines 86, 96, 106, 123, 137), which can mask unexpected behaviors or regressions. While this approach confirms routes are mounted, it doesn't validate specific error conditions or success scenarios.
For example:
- Line 106: Accepts 200, 400, or 401 - consider separate tests for authenticated vs unauthenticated scenarios
- Lines 123 & 137: Accept 200, 400, or 500 - consider testing valid and invalid payloads separately
Example: More specific test structure
describe('User routes', () => { it('should require authentication', async () => { const res = await fetchApp('/api/users/search?q=test'); expect(res.status).toBe(401); }); it('should validate search query parameter', async () => { const res = await fetchApp('/api/users/search', { headers: { 'x-test-user-id': 'user-1' } }); expect(res.status).toBe(400); }); it('should return results for valid search', async () => { const res = await fetchApp('/api/users/search?q=test', { headers: { 'x-test-user-id': 'user-1' } }); expect(res.status).toBe(200); }); });
214-258: Tighten assertions for PDF proxy and Durable Object tests.Similar to the route mounting tests, these tests accept multiple status codes (lines 225, 243, 251, 257), which can mask specific failure modes:
- PDF proxy tests (lines 214-244): Accept both 400 and 401, making it unclear whether validation or authentication is being tested. Separate concerns into distinct test cases.
- Durable Object tests (lines 248-258): Accept 200, 400, or 401, which doesn't validate specific behaviors or error conditions.
The conditional assertion pattern at lines 226-229 is a workaround for loose assertions - separate test cases would be clearer.
Example: Separate authentication from validation
describe('PDF Proxy - Authentication', () => { it('should require authentication', async () => { const res = await fetchApp('/api/pdf-proxy', { method: 'POST', headers: { 'content-type': 'application/json' }, body: JSON.stringify({ url: 'https://example.com/test.pdf' }), }); expect(res.status).toBe(401); }); }); describe('PDF Proxy - Validation', () => { it('should reject requests without URL', async () => { const res = await fetchApp('/api/pdf-proxy', { method: 'POST', headers: { 'content-type': 'application/json', 'x-test-user-id': 'user-1', }, body: JSON.stringify({}), }); expect(res.status).toBe(400); const body = await json(res); expect(body.error).toMatch(/URL/i); }); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/workers/src/__tests__/app.test.js(1 hunks)packages/workers/src/__tests__/health.test.js(1 hunks)packages/workers/src/durable-objects/__tests__/EmailQueue.test.js(1 hunks)packages/workers/src/durable-objects/__tests__/UserSession.test.js(1 hunks)packages/workers/src/middleware/__tests__/auth.test.js(1 hunks)packages/workers/src/middleware/__tests__/cors.test.js(1 hunks)packages/workers/src/middleware/__tests__/rateLimit.test.js(1 hunks)packages/workers/src/middleware/auth.js(3 hunks)packages/workers/src/middleware/rateLimit.js(1 hunks)packages/workers/src/routes/__tests__/email.test.js(1 hunks)packages/workers/src/routes/__tests__/projects.test.js(1 hunks)packages/workers/src/routes/billing/__tests__/webhooks.test.js(1 hunks)packages/workers/src/routes/billing/index.js(5 hunks)packages/workers/src/routes/billing/portal.js(2 hunks)packages/workers/src/routes/google-drive.js(10 hunks)packages/workers/src/routes/pdfs.js(14 hunks)packages/workers/src/routes/users.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/workers/src/routes/billing/portal.js
- packages/workers/src/routes/billing/tests/webhooks.test.js
- packages/workers/src/routes/tests/email.test.js
- packages/workers/src/middleware/tests/cors.test.js
- packages/workers/src/tests/health.test.js
- packages/workers/src/middleware/tests/rateLimit.test.js
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/workers/src/middleware/rateLimit.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/billing/index.jspackages/workers/src/durable-objects/__tests__/UserSession.test.jspackages/workers/src/routes/users.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/middleware/auth.jspackages/workers/src/durable-objects/__tests__/EmailQueue.test.jspackages/workers/src/routes/google-drive.js
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/workers/src/middleware/rateLimit.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/billing/index.jspackages/workers/src/durable-objects/__tests__/UserSession.test.jspackages/workers/src/routes/users.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/middleware/auth.jspackages/workers/src/durable-objects/__tests__/EmailQueue.test.jspackages/workers/src/routes/google-drive.js
packages/workers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/workers/**/*.{js,ts}: Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/middleware/rateLimit.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/billing/index.jspackages/workers/src/durable-objects/__tests__/UserSession.test.jspackages/workers/src/routes/users.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/middleware/auth.jspackages/workers/src/durable-objects/__tests__/EmailQueue.test.jspackages/workers/src/routes/google-drive.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/middleware/rateLimit.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/billing/index.jspackages/workers/src/durable-objects/__tests__/UserSession.test.jspackages/workers/src/routes/users.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/middleware/auth.jspackages/workers/src/durable-objects/__tests__/EmailQueue.test.jspackages/workers/src/routes/google-drive.js
packages/workers/src/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/workers/src/**/*.{js,ts}: Use Zod for schema and input validation on the backend
Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/middleware/rateLimit.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/billing/index.jspackages/workers/src/durable-objects/__tests__/UserSession.test.jspackages/workers/src/routes/users.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/middleware/auth.jspackages/workers/src/durable-objects/__tests__/EmailQueue.test.jspackages/workers/src/routes/google-drive.js
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Zod for schema and input validation on the backend
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/routes/pdfs.jspackages/workers/src/routes/users.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/routes/google-drive.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/routes/pdfs.jspackages/workers/src/routes/users.jspackages/workers/src/routes/__tests__/projects.test.jspackages/workers/src/routes/google-drive.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/src/routes/users.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/middleware/auth.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/src/routes/users.jspackages/workers/src/middleware/__tests__/auth.test.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/middleware/auth.js
🧬 Code graph analysis (7)
packages/workers/src/routes/pdfs.js (2)
packages/shared/src/errors/helpers.ts (1)
createDomainError(29-41)packages/workers/src/config/constants.js (2)
FILE_SIZE_LIMITS(24-28)FILE_SIZE_LIMITS(24-28)
packages/workers/src/routes/billing/index.js (4)
packages/workers/src/routes/avatars.js (2)
dbError(175-178)dbError(217-220)packages/workers/src/routes/billing/portal.js (1)
error(25-29)packages/workers/src/routes/billing/checkout.js (2)
error(26-29)createCheckoutSession(19-81)packages/workers/src/routes/billing/webhooks.js (4)
error(68-71)tier(113-113)tier(146-146)handleWebhook(55-105)
packages/workers/src/durable-objects/__tests__/UserSession.test.js (4)
packages/workers/src/routes/users.js (2)
userId(137-137)userId(182-182)packages/workers/src/__tests__/app.test.js (2)
req(50-50)res(51-51)packages/workers/src/routes/__tests__/projects.test.js (2)
req(66-73)res(74-74)packages/workers/src/middleware/auth.js (2)
session(16-18)session(38-40)
packages/workers/src/routes/users.js (3)
packages/workers/src/routes/account-merge.js (11)
error(81-86)error(93-98)error(120-120)error(179-182)error(222-227)error(232-237)c(78-78)c(219-219)c(358-358)c(587-587)dbError(570-573)packages/workers/src/routes/admin.js (9)
c(267-267)dbError(66-69)dbError(184-187)dbError(253-256)dbError(300-303)dbError(330-333)dbError(395-398)dbError(421-424)dbError(476-479)packages/workers/src/routes/database.js (2)
dbError(44-47)dbError(87-90)
packages/workers/src/middleware/__tests__/auth.test.js (2)
packages/workers/src/middleware/auth.js (4)
requireAuth(35-56)auth(15-15)auth(37-37)getAuth(63-68)packages/workers/src/auth/config.js (1)
createAuth(11-302)
packages/workers/src/__tests__/app.test.js (1)
packages/workers/src/__tests__/helpers.js (8)
resetTestDatabase(10-145)fetchApp(316-323)testEnv(317-317)ctx(318-318)req(319-319)res(320-320)json(304-311)text(305-305)
packages/workers/src/middleware/auth.js (2)
packages/workers/src/routes/pdfs.js (16)
error(37-39)error(52-52)error(118-122)error(129-133)error(149-153)error(159-163)error(176-180)error(184-188)error(193-197)error(207-211)error(219-223)error(269-273)error(278-282)error(292-292)error(325-329)error(338-342)packages/workers/src/routes/google-drive.js (1)
authError(173-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates
- GitHub Check: Workers Builds: corates-workers-prod
🔇 Additional comments (47)
packages/workers/src/middleware/auth.js (3)
43-44: LGTM! Consistent with centralized error pattern.The change from a plain error object to
createDomainError(AUTH_ERRORS.REQUIRED)aligns with the PR objective to use centralized error codes. The pattern correctly returns both the error object and itsstatusCodeproperty.
51-54: LGTM! Error handling follows the same pattern.The catch block correctly uses the same centralized error pattern, ensuring consistent error responses across both authentication failure paths.
7-7: AUTH_ERRORS constant mapping verified.The import on line 7 correctly brings in
createDomainErrorandAUTH_ERRORSfrom@corates/shared.AUTH_ERRORS.REQUIREDmaps to code'AUTH_REQUIRED'with HTTP status 401, matching the test expectations at lines 63 and 107 of the test file. The centralized error handling is properly configured.packages/workers/src/middleware/__tests__/auth.test.js (2)
9-34: LGTM! Clean and effective mock implementation.The mock setup correctly simulates Better-Auth behavior by using test headers to control authentication state. This approach provides clear test isolation without requiring actual auth infrastructure.
54-66: LGTM! Validates centralized error structure.The test correctly verifies that unauthenticated requests receive the standardized domain error response with
code,message, andstatusCodefields, confirming the migration to centralized error handling.packages/workers/src/routes/billing/index.js (4)
14-20: LGTM!Clean import of centralized error utilities from the shared package. All imported functions are utilized in the error handling throughout this file.
57-61: LGTM!Proper use of centralized error handling for database operations. The error includes contextual information (operation, originalError) and returns with the appropriate status code.
186-192: LGTM!Proper validation of required Stripe signature header with appropriate error response.
199-208: LGTM!Correct use of the
isDomainErrorhelper to detect and pass through domain errors before wrapping unexpected errors. This addresses the concern raised in the previous review comment.packages/workers/src/routes/google-drive.js (6)
11-18: LGTM! Clean integration of centralized error utilities.The imports properly bring in the centralized error handling utilities from
@corates/shared, aligning well with the PR objective.
60-67: LGTM! Proper domain error creation for token refresh failure.The error handling correctly creates a domain error with appropriate context and preserves the original error text for debugging.
99-105: LGTM! Clear error handling for missing refresh token.The error handling correctly creates a domain error with helpful context and a user-friendly message explaining the required action.
149-155: LGTM! Consistent error handling for disconnected Google account.The error handling properly returns a domain error with clear context when the user hasn't connected their Google account.
261-315: LGTM! Comprehensive error handling for Google Drive operations.All error cases are properly handled with appropriate domain error types:
- Not connected:
AUTH_ERRORS.INVALID(lines 261-265)- File not found:
FILE_ERRORS.NOT_FOUNDwith source metadata (lines 283-287)- Fetch failure:
SYSTEM_ERRORS.INTERNAL_ERRORwith operation context (lines 289-293)- Invalid file type:
FILE_ERRORS.INVALID_TYPEwith type details (lines 300-304)- File too large:
FILE_ERRORS.TOO_LARGEwith size limits (lines 310-314)Each error includes relevant metadata for debugging and user feedback.
328-332: LGTM! Proper error handling for download failure.The error handling correctly wraps the download failure as a system error with clear operation context.
packages/workers/src/durable-objects/__tests__/UserSession.test.js (5)
10-20: Well-structured mock setup for auth verification.The mock pattern exposing
__mockVerifyAuthfor test access is a clean approach to control authentication behavior per test case.
23-31: Good test isolation with proper cleanup.The
beforeEachcorrectly clears mocks, storage, and alarms to ensure tests run independently.
303-327: Sequential notification loop is acceptable for testing the cap.The 55-iteration loop properly validates the 50-notification cap with correct boundary checks (first 5 removed, indexes 5-54 remain).
381-412: Comprehensive alarm lifecycle tests with good coverage of edge cases.The tests properly validate:
- Inactive session cleanup (25 hours threshold)
- Active session rescheduling
- Alarm state verification
The mix of direct
instance.alarm()andrunDurableObjectAlarmis acceptable since the former runs withinrunInDurableObjectcontext.Also applies to: 414-449
452-462: Good coverage of path extraction edge cases.The tests validate both valid paths and edge cases (empty path, non-matching prefix).
packages/workers/src/routes/users.js (6)
21-28: Clean import of centralized error utilities.The imports from
@corates/sharedare well-organized and include the necessary domain error creators and error type constants.
60-68: Validation error handling follows the centralized pattern.Using
createValidationErrorwith field name, error code, value, and constraint provides structured error responses.
121-128: Consistent DB error handling pattern.The error structure with operation name and
originalErrormatches the pattern used in other routes (database.js, admin.js) per the relevant code snippets.
140-145: Authorization denial uses appropriate domain error.Using
AUTH_ERRORS.FORBIDDENwith a descriptive reason is consistent with the centralized error approach.
232-235: Previously flagged bug has been addressed.The code now correctly uses
currentUser.idinstead of the undefinedtargetUserIdthat was flagged in a prior review.
282-289: DB error handling follows established pattern.Consistent with other error handling throughout the file.
packages/workers/src/durable-objects/__tests__/EmailQueue.test.js (5)
9-33: Mock setup for Postmark and email service is well-structured.The dual mock approach (Postmark Client class + email service) allows testing both the low-level client and the service abstraction.
36-51: Thorough test isolation with prefix-based storage cleanup.The cleanup handles both email queue and dead letter queue storage, ensuring clean state between tests.
70-166: Comprehensive validation testing for email payloads.The tests cover all required fields, partial payloads, and method validation with appropriate status code assertions.
272-333: Email storage tests properly validate record structure and cleanup.The tests verify storage persistence and cleanup functionality with appropriate assertions.
388-418: Alarm deduplication test is well-designed.The test queues two emails and verifies that the alarm timestamp remains unchanged, properly validating the no-duplicate behavior.
packages/workers/src/__tests__/app.test.js (1)
141-175: LGTM - Middleware tests are well-structured.The middleware chain tests validate CORS, security headers, and preflight handling with specific assertions. Good coverage of cross-cutting concerns.
packages/workers/src/routes/pdfs.js (15)
14-21: Previous issue resolved: PROJECT_ERRORS now imported.The missing
PROJECT_ERRORSimport flagged in the earlier review has been correctly added at line 20, resolving the ReferenceError that would have occurred at line 52.
37-40: LGTM: Correct validation error for missing parameters.The use of
VALIDATION_ERRORS.FIELD_REQUIREDwith field details is appropriate for missing projectId or studyId.
52-53: LGTM: Access control error correctly implemented.The use of
PROJECT_ERRORS.ACCESS_DENIEDis now valid with the import added at line 20.
118-123: LGTM: Authorization check correctly uses FORBIDDEN error.The viewer role enforcement properly uses
AUTH_ERRORS.FORBIDDENwith contextual details.
129-135: LGTM: File size validation correctly implemented at multiple stages.The three size checks (Content-Length header, multipart file, raw arrayBuffer) consistently use
FILE_ERRORS.TOO_LARGEwith detailed context. The early Content-Length rejection is a good optimization.Also applies to: 159-165, 176-182
149-154: LGTM: Validation errors correctly categorized.The missing file check uses
FIELD_REQUIREDand the fileName validation usesFIELD_INVALID_FORMAT, both with appropriate field context.Also applies to: 193-198
184-189: LGTM: File type validation errors correctly implemented.Both the Content-Type check and PDF magic bytes validation appropriately use
FILE_ERRORS.INVALID_TYPEwith relevant type information in details.Also applies to: 207-212
219-224: LGTM: Duplicate file handling with helpful error message.The use of
FILE_ERRORS.ALREADY_EXISTSwith actionable guidance in the message is good UX.
250-255: LGTM: Generic upload error handler preserves context.The catch-all uses
FILE_ERRORS.UPLOAD_FAILEDwhile preserving the original error message and operation context for debugging.
269-274: LGTM: Download parameter validation mirrors upload patterns.The fileName validation in the download route consistently uses the same error types as the upload route.
Also applies to: 278-283
292-293: LGTM: File not found error correctly implemented.The use of
FILE_ERRORS.NOT_FOUNDwith fileName and key context is appropriate.
307-312: LGTM: Generic download error handler preserves context.The catch-all uses
SYSTEM_ERRORS.INTERNAL_ERRORwhile maintaining operation context and original error details.
325-330: LGTM: Delete authorization check mirrors upload pattern.The viewer role enforcement is consistent with the upload route's authorization logic.
338-343: LGTM: Delete parameter validation consistent across routes.The fileName validation follows the same pattern as upload and download routes.
Also applies to: 347-352
362-367: LGTM: Generic delete error handler preserves context.The catch-all uses
SYSTEM_ERRORS.INTERNAL_ERRORconsistently with the download route's error handling.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
packages/workers/src/routes/google-drive.js (2)
169-180: Strengthen domain error detection.Line 174 checks only
error.codeto determine if the caught error is a domain error, but doesn't verify the presence ofstatusCodeormessage. Any error object with acodeproperty will be returned directly, potentially resulting in a malformed response.Validate all required domain error fields
const authError = - error.code ? error : ( + (error.code && error.statusCode && error.message) ? error : ( createDomainError(AUTH_ERRORS.INVALID, { context: 'google_token_expired', originalError: typeof error?.message === 'string' ? error.message : String(error), }) );
363-365: Strengthen domain error detection.Similar to line 174, this code relies on
isDomainError(error)to determine if an error is a domain error, but doesn't provide a fallback if the check is insufficient. EnsureisDomainErrorvalidates all required fields (code,statusCode,message).Verify that
isDomainErrorfrom@corates/sharedchecks all required domain error properties:#!/bin/bash # Search for the isDomainError implementation ast-grep --pattern 'function isDomainError($$$) { $$$ }' ast-grep --pattern 'export const isDomainError = $$$'packages/workers/src/__tests__/app.test.js (1)
7-7: Past review comment about fetchApp duplication is outdated.The past review suggested importing
fetchAppfrom helpers.js to avoid duplication. This has already been implemented—line 7 correctly importsfetchAppfrom './helpers.js', and the code uses it throughout the test suite without any local re-implementation.packages/workers/src/durable-objects/__tests__/UserSession.test.js (1)
76-87: Past review comment about mock coverage has been addressed.The previous review suggested using
mockResolvedValueinstead ofmockResolvedValueOnceto ensure both requests in this test have proper auth mock coverage. This has been correctly implemented at line 77, wheremockResolvedValueis now used to handle multiple authenticated requests within the test.
🧹 Nitpick comments (6)
packages/workers/src/middleware/rateLimit.js (1)
163-178: Security concern from previous review has been addressed.The environment guards successfully prevent production misuse by checking both Node.js (
process.env.NODE_ENV === 'test') and modern bundler (import.meta.env?.MODE === 'test') test environments. The defensivetypeofchecks prevent reference errors across different runtime contexts.One minor consideration: The previous review suggested throwing an error when called outside test environments, but you've chosen to log a warning instead. While the warning approach is more lenient and won't disrupt the application if accidentally invoked, an error would provide stricter enforcement. Both approaches are valid—the current implementation balances safety with robustness.
Optional: Stricter enforcement with error throwing
If you prefer explicit failure over silent warnings:
if (isTest) { rateLimitStore.clear(); } else { - console.warn('Attempted to clear rate limit store outside of test environment'); + throw new Error('clearRateLimitStore can only be called in test environment'); }packages/workers/src/routes/billing/index.js (2)
123-133: Use Zod for request body validation.The request body parsing and validation are done manually, but the coding guidelines specify using Zod for schema and input validation on the backend. Consider defining a Zod schema for the checkout request body to validate both presence and allowed values for
tierandinterval.As per coding guidelines, "Use Zod for schema and input validation on the backend."
Example Zod schema approach
Define a schema at the top of the file or in a separate schemas file:
import { z } from 'zod'; const checkoutSchema = z.object({ tier: z.enum(['pro', 'team', 'enterprise']).refine(val => val !== 'free', { message: 'Cannot checkout with free tier' }), interval: z.enum(['monthly', 'yearly']).default('monthly') });Then use it in the handler:
try { const body = await c.req.json(); - const { tier, interval = 'monthly' } = body; - - if (!tier || tier === 'free') { - const error = createValidationError( - 'tier', - VALIDATION_ERRORS.INVALID_INPUT.code, - tier, - 'invalid_tier', - ); - return c.json(error, error.statusCode); - } + const parseResult = checkoutSchema.safeParse(body); + if (!parseResult.success) { + const error = createValidationError( + 'tier', + VALIDATION_ERRORS.INVALID_INPUT.code, + body.tier, + 'invalid_tier', + ); + return c.json(error, error.statusCode); + } + const { tier, interval } = parseResult.data; const result = await createCheckoutSession(c.env, user, tier, interval); return c.json(result);
182-192: Consider using Zod for header validation.The signature header validation is done manually, but the coding guidelines specify using Zod for schema and input validation on the backend. While header validation is slightly different from body validation, using Zod would provide consistency across all validation logic.
As per coding guidelines, "Use Zod for schema and input validation on the backend."
Example Zod schema approach
import { z } from 'zod'; const webhookHeadersSchema = z.object({ 'stripe-signature': z.string().min(1) });Then use it in the handler:
try { const signature = c.req.header('stripe-signature'); - const rawBody = await c.req.text(); - - if (!signature) { - const error = createValidationError( - 'signature', - VALIDATION_ERRORS.FIELD_REQUIRED.code, - null, - 'required', - ); - return c.json(error, error.statusCode); - } + const rawBody = await c.req.text(); + + const parseResult = webhookHeadersSchema.safeParse({ + 'stripe-signature': signature + }); + if (!parseResult.success) { + const error = createValidationError( + 'signature', + VALIDATION_ERRORS.FIELD_REQUIRED.code, + null, + 'required', + ); + return c.json(error, error.statusCode); + } const result = await handleWebhook(c.env, rawBody, signature); return c.json(result);packages/workers/src/__tests__/app.test.js (1)
55-63: Clarify the comment about expected status codes.The comment on line 61 states "Should return 401 (auth required) or 404 (not found), not 404 from app", but then the assertion expects both 401 and 404. This is confusing—if 404 is acceptable, the comment should clarify that it's a legitimate response from the mounted route (e.g., resource not found), as opposed to a 404 from the top-level app indicating the route itself isn't mounted.
packages/workers/src/__tests__/helpers.js (1)
150-266: Consider using Drizzle's insert API for seed functions.The seed functions use parameterized SQL queries, which is secure but could benefit from Drizzle ORM's insert API for type safety, better IDE support, and consistency with production code. This would align better with the project's use of Drizzle ORM.
Example refactor for seedUser
+import { db } from '../db/connection.js'; +import { user } from '../db/schema.js'; + export async function seedUser({ id, name, email, // ... other params }) { - await env.DB.prepare( - `INSERT INTO user ( - id, name, email, displayName, username, role, - emailVerified, banned, banReason, banExpires, createdAt, updatedAt - ) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12)`, - ) - .bind( - id, - name, - // ... other bindings - ) - .run(); + await db.insert(user).values({ + id, + name, + email, + displayName, + username, + role, + emailVerified, + banned, + banReason, + banExpires, + createdAt, + updatedAt, + }); }Based on coding guidelines, which specify that Drizzle ORM should be used for database interactions.
packages/workers/src/durable-objects/__tests__/UserSession.test.js (1)
389-391: Consider simplifying date arithmetic for clarity.The date manipulation uses
setTimewith manual millisecond calculation. While correct, it could be more readable using direct arithmetic or a helper.Alternative approach
- const oldDate = new Date(); - oldDate.setTime(oldDate.getTime() - 25 * 60 * 60 * 1000); // 25 hours ago in milliseconds + const oldDate = new Date(Date.now() - 25 * 60 * 60 * 1000); // 25 hours ago
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/workers/src/__tests__/app.test.js(1 hunks)packages/workers/src/__tests__/helpers.js(1 hunks)packages/workers/src/durable-objects/__tests__/UserSession.test.js(1 hunks)packages/workers/src/middleware/rateLimit.js(1 hunks)packages/workers/src/routes/billing/index.js(5 hunks)packages/workers/src/routes/google-drive.js(10 hunks)packages/workers/src/routes/pdfs.js(14 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/workers/src/durable-objects/__tests__/UserSession.test.jspackages/workers/src/middleware/rateLimit.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/billing/index.jspackages/workers/src/__tests__/helpers.js
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/workers/src/durable-objects/__tests__/UserSession.test.jspackages/workers/src/middleware/rateLimit.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/billing/index.jspackages/workers/src/__tests__/helpers.js
packages/workers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/workers/**/*.{js,ts}: Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/durable-objects/__tests__/UserSession.test.jspackages/workers/src/middleware/rateLimit.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/billing/index.jspackages/workers/src/__tests__/helpers.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/durable-objects/__tests__/UserSession.test.jspackages/workers/src/middleware/rateLimit.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/billing/index.jspackages/workers/src/__tests__/helpers.js
packages/workers/src/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/workers/src/**/*.{js,ts}: Use Zod for schema and input validation on the backend
Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/durable-objects/__tests__/UserSession.test.jspackages/workers/src/middleware/rateLimit.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/app.test.jspackages/workers/src/routes/billing/index.jspackages/workers/src/__tests__/helpers.js
🧠 Learnings (4)
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/routes/pdfs.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/routes/pdfs.jspackages/workers/src/routes/google-drive.jspackages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/src/__tests__/app.test.jspackages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/src/__tests__/app.test.jspackages/workers/src/__tests__/helpers.js
🧬 Code graph analysis (5)
packages/workers/src/durable-objects/__tests__/UserSession.test.js (3)
packages/workers/src/routes/users.js (2)
userId(137-137)userId(182-182)packages/workers/src/__tests__/helpers.js (1)
req(320-320)packages/workers/src/middleware/auth.js (2)
session(16-18)session(38-40)
packages/workers/src/routes/pdfs.js (2)
packages/shared/src/errors/helpers.ts (1)
createDomainError(29-41)packages/workers/src/config/constants.js (2)
FILE_SIZE_LIMITS(24-28)FILE_SIZE_LIMITS(24-28)
packages/workers/src/routes/google-drive.js (3)
packages/workers/src/routes/billing/index.js (5)
error(127-132)error(186-191)systemError(144-147)systemError(168-171)systemError(204-207)packages/workers/src/routes/pdfs.js (14)
error(37-39)error(52-52)error(118-122)error(129-133)error(149-153)error(159-163)error(176-180)error(184-188)error(193-197)error(207-211)error(219-223)error(269-273)error(278-282)error(292-292)packages/workers/src/middleware/auth.js (1)
authError(53-53)
packages/workers/src/__tests__/app.test.js (1)
packages/workers/src/__tests__/helpers.js (5)
resetTestDatabase(10-145)res(321-321)fetchApp(317-324)json(305-312)text(306-306)
packages/workers/src/routes/billing/index.js (4)
packages/workers/src/routes/account-merge.js (5)
dbError(570-573)c(78-78)c(219-219)c(358-358)c(587-587)packages/workers/src/routes/avatars.js (3)
dbError(175-178)dbError(217-220)dbError(243-246)packages/workers/src/routes/billing/webhooks.js (3)
tier(113-113)tier(146-146)handleWebhook(55-105)packages/workers/src/routes/billing/checkout.js (1)
createCheckoutSession(19-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates
- GitHub Check: Workers Builds: corates-workers-prod
🔇 Additional comments (10)
packages/workers/src/routes/billing/index.js (5)
14-20: LGTM! Clean centralized error handling imports.The imports from
@corates/sharedproperly support the unified error handling pattern used throughout this file.
55-62: LGTM! Appropriate error handling for database operations.The error is correctly wrapped as a DB_ERROR since the only operation in the try block is the database query. The operation context is clear and the error is returned with the appropriate status code.
140-148: LGTM! Correct use of isDomainError helper.The error handling now properly uses the
isDomainErrorhelper to detect and pass through domain errors, avoiding unnecessary wrapping. This addresses the previous review feedback.
164-172: LGTM! Correct use of isDomainError helper.The error handling properly uses the
isDomainErrorhelper for consistent domain error detection and passthrough. This addresses the previous review feedback.
199-208: LGTM! Correct domain error passthrough with clear comment.The error handling now properly detects and passes through domain errors before wrapping unexpected errors. The comment clearly explains the pattern. This addresses the previous review feedback.
packages/workers/src/routes/pdfs.js (5)
13-21: LGTM!All necessary error utilities and constants are properly imported. The addition of
PROJECT_ERRORSaddresses the previous review concern.
36-54: LGTM!The middleware correctly uses domain errors for validation (
VALIDATION_ERRORS.FIELD_REQUIRED) and access control (PROJECT_ERRORS.ACCESS_DENIED), with appropriate metadata for debugging.
117-256: LGTM!The upload route's error handling is comprehensive and well-structured:
- Permission checks use
AUTH_ERRORS.FORBIDDEN- Size validation uses
FILE_ERRORS.TOO_LARGEwith clear metadata- Missing/invalid inputs use appropriate
VALIDATION_ERRORScodes- Type validation (content-type and magic bytes) uses
FILE_ERRORS.INVALID_TYPE- Conflict detection uses
FILE_ERRORS.ALREADY_EXISTS- Generic upload failures use
FILE_ERRORS.UPLOAD_FAILEDAll error responses include helpful metadata and consistent status codes.
268-313: LGTM!The download route properly validates inputs with
VALIDATION_ERRORScodes, handles missing files withFILE_ERRORS.NOT_FOUND, and catches unexpected failures withSYSTEM_ERRORS.INTERNAL_ERROR. All error responses include appropriate metadata.
324-368: LGTM!The delete route follows the same solid error handling patterns as the other routes, with appropriate permission checks, input validation, and error recovery using the centralized error system.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/workers/src/__tests__/helpers.js (1)
7-174: Sync the embedded SQL with the migration file and add CI verification.The embedded
MIGRATION_SQLin helpers.js is currently out of sync withpackages/workers/migrations/0001_init.sql. The test helper is missing:
- The first comment line:
-- Consolidated Corates Database Schema- The final index:
CREATE INDEX idx_subscriptions_stripeSubscriptionId ON subscriptions(stripeSubscriptionId);- Trailing whitespace consistency
Embedding SQL is a pragmatic workaround for Cloudflare Workers' filesystem restrictions, but keeping it manually in sync is error-prone. Add a CI check to verify the embedded SQL matches the migration file on every commit.
🧹 Nitpick comments (5)
packages/workers/src/routes/__tests__/google-drive.test.js (2)
66-103: Consider extracting R2 mock to shared test utility.The R2 bucket mock is comprehensive and handles multiple body types well (ArrayBuffer, ReadableStream, etc.). If this mock is needed in other test files, consider extracting it to a shared test utility in
__tests__/helpers.jsto reduce duplication and improve maintainability.
343-356: Mock fetch chaining is functional but relies on call order.The test uses
mockResolvedValueOncechaining which depends on the implicit order of API calls. This works correctly, but consider adding comments indicating which mock corresponds to which API call (file metadata vs. file content) for improved clarity.Example with clarifying comments
// Mock Google Drive API calls mockFetch + // First call: fetch file metadata .mockResolvedValueOnce({ ok: true, json: async () => ({ id: 'file-123', name: 'document.pdf', mimeType: 'application/pdf', size: '1024', }), }) + // Second call: fetch file content .mockResolvedValueOnce({ ok: true, body: new Uint8Array([0x25, 0x50, 0x44, 0x46, 0x2d]), // %PDF- });packages/workers/src/routes/__tests__/account-merge.test.js (1)
90-105: Consider movingseedAccountto the shared helpers module.The
seedAccounthelper uses the same pattern asseedUser,seedProject, and other helpers imported fromhelpers.js. For consistency and reusability across test files, consider moving this function topackages/workers/src/__tests__/helpers.js.Proposed location for the helper
Add to
packages/workers/src/__tests__/helpers.js:export async function seedAccount({ userId, providerId = 'google', accountId = null }) { const nowSec = Math.floor(Date.now() / 1000); await env.DB.prepare( `INSERT INTO account (id, userId, accountId, providerId, createdAt, updatedAt) VALUES (?1, ?2, ?3, ?4, ?5, ?6)`, ) .bind( `acc-${userId}-${providerId}`, userId, accountId || `${providerId}-${userId}`, providerId, nowSec, nowSec, ) .run(); }packages/workers/src/routes/__tests__/pdfs.test.js (1)
134-153: Confusing path parameter convention using's'for list endpoint.The
fetchPdfhelper uses's'as a magic value to represent the root list endpoint. This is non-intuitive and could confuse future maintainers.Proposed improvement for clarity
-async function fetchPdf(projectId, studyId, path = '', init = {}) { +async function fetchPdf(projectId, studyId, path = '/', init = {}) { const ctx = createExecutionContext(); // Route is mounted at /api/projects/:projectId/studies/:studyId/pdf - // For list endpoint, path should be empty or '/' - const routePath = path === 's' ? '' : path; + // Normalize path - empty string or '/' both hit the list endpoint + const routePath = path === '/' ? '' : path; const req = new Request( `http://localhost/api/projects/${projectId}/studies/${studyId}/pdf${routePath}`,Then update the test calls:
- const res = await fetchPdf('project-1', 'study-1', 's'); + const res = await fetchPdf('project-1', 'study-1', '/');packages/workers/src/__tests__/helpers.js (1)
179-223: Unused variablenextCharin SQL parser.Line 191 declares
nextCharbut it's never used. This appears to be leftover from an incomplete implementation or future-proofing that was never completed.Proposed fix
for (let i = 0; i < withoutComments.length; i++) { const char = withoutComments[i]; - const nextChar = withoutComments[i + 1]; // Track string boundaries
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/workers/src/__tests__/helpers.js(1 hunks)packages/workers/src/middleware/__tests__/requireAdmin.test.js(1 hunks)packages/workers/src/routes/__tests__/account-merge.test.js(1 hunks)packages/workers/src/routes/__tests__/avatars.test.js(1 hunks)packages/workers/src/routes/__tests__/google-drive.test.js(1 hunks)packages/workers/src/routes/__tests__/pdfs.test.js(1 hunks)packages/workers/src/routes/google-drive.js(10 hunks)packages/workers/src/routes/pdfs.js(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/workers/src/routes/google-drive.js
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/workers/src/middleware/__tests__/requireAdmin.test.jspackages/workers/src/routes/__tests__/avatars.test.jspackages/workers/src/routes/__tests__/account-merge.test.jspackages/workers/src/routes/__tests__/pdfs.test.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/__tests__/google-drive.test.jspackages/workers/src/__tests__/helpers.js
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/workers/src/middleware/__tests__/requireAdmin.test.jspackages/workers/src/routes/__tests__/avatars.test.jspackages/workers/src/routes/__tests__/account-merge.test.jspackages/workers/src/routes/__tests__/pdfs.test.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/__tests__/google-drive.test.jspackages/workers/src/__tests__/helpers.js
packages/workers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/workers/**/*.{js,ts}: Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/middleware/__tests__/requireAdmin.test.jspackages/workers/src/routes/__tests__/avatars.test.jspackages/workers/src/routes/__tests__/account-merge.test.jspackages/workers/src/routes/__tests__/pdfs.test.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/__tests__/google-drive.test.jspackages/workers/src/__tests__/helpers.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/middleware/__tests__/requireAdmin.test.jspackages/workers/src/routes/__tests__/avatars.test.jspackages/workers/src/routes/__tests__/account-merge.test.jspackages/workers/src/routes/__tests__/pdfs.test.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/__tests__/google-drive.test.jspackages/workers/src/__tests__/helpers.js
packages/workers/src/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/workers/src/**/*.{js,ts}: Use Zod for schema and input validation on the backend
Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/middleware/__tests__/requireAdmin.test.jspackages/workers/src/routes/__tests__/avatars.test.jspackages/workers/src/routes/__tests__/account-merge.test.jspackages/workers/src/routes/__tests__/pdfs.test.jspackages/workers/src/routes/pdfs.jspackages/workers/src/routes/__tests__/google-drive.test.jspackages/workers/src/__tests__/helpers.js
🧠 Learnings (7)
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/src/middleware/__tests__/requireAdmin.test.jspackages/workers/src/routes/__tests__/avatars.test.jspackages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/src/middleware/__tests__/requireAdmin.test.jspackages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/routes/pdfs.jspackages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/routes/pdfs.jspackages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/migrations/0001_init.sql : When adding new tables or schema changes, edit the existing 0001_init.sql file directly rather than creating separate migration files
Applied to files:
packages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/migrations/*.sql : All migrations should go in a single file: `packages/workers/migrations/0001_init.sql`. Do NOT create separate migration files since this project is not yet in production
Applied to files:
packages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/migrations/0001_init.sql : All migrations should go in a single file: `packages/workers/migrations/0001_init.sql`. Do NOT create separate migration files since this project is not yet in production
Applied to files:
packages/workers/src/__tests__/helpers.js
🧬 Code graph analysis (6)
packages/workers/src/middleware/__tests__/requireAdmin.test.js (3)
packages/workers/src/auth/admin.js (1)
isAdminUser(12-16)packages/workers/src/middleware/requireAdmin.js (1)
requireAdmin(15-39)packages/workers/src/auth/config.js (1)
createAuth(11-302)
packages/workers/src/routes/__tests__/account-merge.test.js (2)
packages/workers/src/routes/account-merge.js (1)
accountMergeRoutes(40-40)packages/workers/src/__tests__/helpers.js (6)
res(419-419)seedUser(248-283)json(403-410)seedProject(288-302)seedProjectMember(307-314)seedSubscription(331-364)
packages/workers/src/routes/__tests__/pdfs.test.js (2)
packages/workers/src/routes/pdfs.js (15)
key(216-216)key(286-286)key(355-355)prefix(83-83)projectId(33-33)projectId(80-80)projectId(112-112)projectId(264-264)projectId(333-333)studyId(34-34)studyId(81-81)studyId(113-113)studyId(265-265)studyId(334-334)file(146-146)packages/workers/src/__tests__/helpers.js (7)
ctx(417-417)req(418-418)res(419-419)seedUser(248-283)seedProject(288-302)seedProjectMember(307-314)json(403-410)
packages/workers/src/routes/pdfs.js (3)
packages/shared/src/errors/helpers.ts (1)
createDomainError(29-41)packages/workers/src/routes/billing/index.js (3)
systemError(144-147)systemError(168-171)systemError(204-207)packages/workers/src/config/constants.js (2)
FILE_SIZE_LIMITS(24-28)FILE_SIZE_LIMITS(24-28)
packages/workers/src/routes/__tests__/google-drive.test.js (2)
packages/workers/src/routes/google-drive.js (1)
googleDriveRoutes(21-21)packages/workers/src/__tests__/helpers.js (3)
res(419-419)seedUser(248-283)json(403-410)
packages/workers/src/__tests__/helpers.js (1)
packages/workers/src/__tests__/admin.test.js (6)
run(51-51)res(261-261)text(250-250)app(46-46)ctx(259-259)req(260-260)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates-workers-prod
- GitHub Check: Workers Builds: corates
🔇 Additional comments (26)
packages/workers/src/routes/__tests__/google-drive.test.js (7)
17-50: Well-structured mock setup.The mock configurations for Postmark and authentication middleware are clean and use test headers effectively for controlling user identity in tests. This approach provides good flexibility for testing different user scenarios.
115-150: Helper functions are well-structured.Both
fetchGoogleDriveandseedGoogleAccountfollow the existing test patterns in the codebase. The use of raw SQL inseedGoogleAccountis consistent with other seed functions likeseedUserinhelpers.js.
152-190: Good coverage of status endpoint scenarios.The tests cover both connected and disconnected states effectively, with clear assertions on the expected response structure.
192-280: Excellent test coverage for picker-token endpoint.The tests comprehensively cover the happy path, authorization errors, and token refresh scenarios. The assertion on
body.codematching'AUTH_INVALID'(line 227) properly validates the centralized error code system, which aligns with this PR's objectives.
282-311: Good verification of disconnect endpoint.The test properly verifies both the success response and the database side effect (account deletion), ensuring the disconnect operation is complete.
377-430: Good validation of file type restrictions.The test properly verifies that non-PDF files are rejected with the appropriate error code (
FILE_INVALID_TYPE), aligning with the centralized error handling system.
432-556: Comprehensive error scenario coverage for import endpoint.The tests effectively validate multiple error conditions:
- File size limits (413 with
FILE_TOO_LARGE)- Authentication requirements (401 with
AUTH_INVALID)- Input validation (400 with
VALIDATION_FIELD_REQUIRED)All tests properly assert on the centralized error codes, which aligns perfectly with this PR's objective of migrating to a unified error handling system.
packages/workers/src/routes/__tests__/account-merge.test.js (3)
6-60: LGTM! Well-structured test setup.The imports and mocking strategy are appropriate for integration testing. The auth middleware mock using test headers is a clean pattern for simulating authenticated requests.
242-641: LGTM! Comprehensive test coverage for verify and complete flows.The tests thoroughly validate:
- Successful verification with correct codes
- Rejection of invalid/expired verification codes
- Complete merge with provider consolidation and ownership transfer
- Subscription tier prioritization (team > pro)
- Database side effects (user deletion, project ownership updates)
The assertions are precise and the test data setup is realistic.
643-703: LGTM! Cancel flow properly tested.The test correctly verifies that the merge request can be canceled and the verification record is cleaned up from the database.
packages/workers/src/middleware/__tests__/requireAdmin.test.js (3)
138-153: LGTM! Comprehensive coverage of the admin check helper.The test suite thoroughly covers all scenarios: admin users, non-admin users with various roles, and null/undefined inputs. The assertions are clear and validate the expected behavior correctly.
7-7: The import is correct. The test properly importsisAdmin, which is re-exported fromrequireAdmin.jsas an alias forisAdminUserfrom the auth module. No changes needed.Likely an incorrect or invalid review comment.
70-72: No changes needed to test assertions; they correctly validate the current middleware behavior.The tests accurately assert against the actual error responses returned by the
requireAdminmiddleware, which currently uses plain error message strings ('Authentication required'and'Admin access required'). The middleware implementation has not yet been updated to use centralized error codes—if that becomes part of the implementation in a future iteration, the tests should be updated at that time.Likely an incorrect or invalid review comment.
packages/workers/src/routes/__tests__/avatars.test.js (2)
1-166: Well-structured test setup with comprehensive mocking.The test file has a solid foundation with proper mocking of external dependencies (Postmark, auth middleware, R2 bucket, Project DO). The in-memory R2 bucket mock correctly handles different body types including
ReadableStream,ArrayBuffer, and objects witharrayBuffer()method.
168-343: Good coverage of error scenarios with domain error code validation.The tests properly validate the centralized error codes (
FILE_TOO_LARGE,FILE_INVALID_TYPE,FILE_NOT_FOUND) from the new error system, ensuring the avatar routes integrate correctly with the shared error handling.packages/workers/src/routes/__tests__/pdfs.test.js (2)
234-501: Comprehensive upload test coverage with proper error code validation.The POST tests thoroughly cover success cases, file size limits, invalid types, duplicates, permission restrictions, and raw upload paths. Error codes align with the centralized error system (
FILE_TOO_LARGE,FILE_INVALID_TYPE,FILE_ALREADY_EXISTS,AUTH_FORBIDDEN).
503-670: Good coverage for download and delete operations.The GET and DELETE tests properly validate success paths, 404 handling (
FILE_NOT_FOUND), and permission enforcement (AUTH_FORBIDDEN). The tests correctly seed data and verify side effects.packages/workers/src/routes/pdfs.js (6)
14-21: Proper imports for centralized error system.All required error types are now imported from
@corates/shared, includingPROJECT_ERRORSwhich was previously missing. This addresses the earlier review comment.
31-61: Clean membership verification with domain errors.The middleware correctly uses
VALIDATION_ERRORS.FIELD_REQUIREDfor missing parameters andPROJECT_ERRORS.ACCESS_DENIEDfor unauthorized access. Uses Drizzle ORM as per coding guidelines.
96-103: Correct error type for list operation failure.Now uses
SYSTEM_ERRORS.INTERNAL_ERRORinstead of the previously suggestedFILE_ERRORS.UPLOAD_FAILED, which is semantically correct for an R2 list operation failure. This addresses the earlier review feedback.
110-256: Comprehensive error handling in upload route.All error paths consistently use
createDomainErrorwith appropriate error types:
AUTH_ERRORS.FORBIDDENfor permission denialFILE_ERRORS.TOO_LARGEwith size metadataVALIDATION_ERRORS.FIELD_REQUIREDfor missing fileFILE_ERRORS.INVALID_TYPEfor content type issuesVALIDATION_ERRORS.FIELD_INVALID_FORMATfor invalid filenamesFILE_ERRORS.ALREADY_EXISTSfor duplicatesFILE_ERRORS.UPLOAD_FAILEDfor storage errors
263-314: Consistent error handling in download route.Properly validates filename, returns
FILE_ERRORS.NOT_FOUNDfor missing files, and catches internal errors withSYSTEM_ERRORS.INTERNAL_ERROR.
320-369: Consistent error handling in delete route.Permission check, filename validation, and error handling follow the same patterns as other routes.
packages/workers/src/__tests__/helpers.js (3)
228-243: Test database reset implementation is functional.While the previous review suggested using Drizzle migrations directly, this embedded SQL approach is a reasonable workaround for the CF Workers test environment constraints. The implementation correctly enables foreign keys and executes statements sequentially.
248-364: Comprehensive seed helpers for test data.The seed functions cover all essential entities (users, projects, project members, sessions, subscriptions) with sensible defaults. Parameter binding prevents SQL injection.
369-434: Useful test utilities for mocked environment and request handling.
createTestEnvprovides a clean way to set up mocked bindings,jsonhandles response parsing gracefully,fetchAppabstracts request execution, andcreateAuthHeadersdocuments the mocked auth approach.
| let syncCalled = false; | ||
| const originalFetch = mockProjectDO.get({ toString: () => 'do-project-1' }).fetch; | ||
| mockProjectDO.get({ toString: () => 'do-project-1' }).fetch = async request => { | ||
| const url = new URL(request.url); | ||
| if (url.pathname === '/sync-member') { | ||
| syncCalled = true; | ||
| const body = await request.json(); | ||
| expect(body.action).toBe('update'); | ||
| expect(body.member.userId).toBe('user-1'); | ||
| expect(body.member.image).toBe('/api/users/avatar/user-1'); | ||
| } | ||
| return originalFetch(request); | ||
| }; | ||
|
|
||
| const imageData = new Uint8Array([0xff, 0xd8, 0xff, 0xe0]); | ||
| const file = new File([imageData], 'avatar.jpg', { type: 'image/jpeg' }); | ||
| const formData = new FormData(); | ||
| formData.append('avatar', file); | ||
|
|
||
| const res = await fetchAvatar('/api/users/avatar', { | ||
| method: 'POST', | ||
| body: formData, | ||
| }); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| // Note: sync happens asynchronously, so we can't easily verify it in this test | ||
| // but the code path is exercised | ||
| }); |
There was a problem hiding this comment.
Sync test mock override does not actually intercept calls.
The test captures originalFetch from a fresh object returned by mockProjectDO.get(), then overwrites fetch on another fresh object. Since mockProjectDO.get() returns a new object each time it's called, the override never intercepts the actual calls made by the route handler.
Consider restructuring the mock to capture all fetch calls:
Proposed fix
it('should sync avatar to project memberships', async () => {
// ... seed setup ...
let syncCalled = false;
- const originalFetch = mockProjectDO.get({ toString: () => 'do-project-1' }).fetch;
- mockProjectDO.get({ toString: () => 'do-project-1' }).fetch = async request => {
+ const originalGet = mockProjectDO.get;
+ mockProjectDO.get = _id => ({
+ fetch: async request => {
const url = new URL(request.url);
if (url.pathname === '/sync-member') {
syncCalled = true;
const body = await request.json();
expect(body.action).toBe('update');
expect(body.member.userId).toBe('user-1');
expect(body.member.image).toBe('/api/users/avatar/user-1');
}
- return originalFetch(request);
- };
+ return new Response(JSON.stringify({ ok: true }), { status: 200 });
+ },
+ });
// ... rest of test ...
+ // Restore original mock after test
+ mockProjectDO.get = originalGet;
});🤖 Prompt for AI Agents
packages/workers/src/routes/__tests__/avatars.test.js around lines 316-343: the
test calls mockProjectDO.get() multiple times so the fetch override is applied
to a different object than the one used by the code under test, meaning the mock
never intercepts the real calls; fix by calling mockProjectDO.get(...) once into
a const (e.g. const projectDO = mockProjectDO.get({...})), capture originalFetch
from that same reference, then override projectDO.fetch with the async wrapper
that sets syncCalled and asserts the request body so all fetch calls go through
the mocked function.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/workers/src/__tests__/helpers.js (1)
227-242: Use Drizzle migrations instead of raw SQL schema creation.The
resetTestDatabasefunction uses embedded raw SQL instead of Drizzle migrations, which violates the coding guideline to "Use Drizzle ORM for database interactions and migrations." This creates a maintenance risk where test schema can drift from production schema.Based on coding guidelines, which specify that Drizzle ORM should be used for database interactions and migrations in
packages/workers/src/**/*.{js,ts}.
🧹 Nitpick comments (2)
packages/workers/src/__tests__/helpers.js (2)
247-282: Refactor seed functions to use Drizzle ORM instead of raw SQL.All seed functions (
seedUser,seedProject,seedProjectMember,seedSession,seedSubscription) use raw SQL INSERT statements instead of Drizzle ORM, violating the coding guideline to "Use Drizzle ORM for database interactions and migrations."Consider refactoring these functions to use Drizzle's insert API for consistency with project standards and type safety.
Based on coding guidelines, which specify that Drizzle ORM should be used for database interactions in
packages/workers/src/**/*.{js,ts}.Example refactor using Drizzle
import { users, projects, projectMembers, sessions, subscriptions } from '../db/schema'; import { eq } from 'drizzle-orm'; import { drizzle } from 'drizzle-orm/d1'; export async function seedUser(userData) { const db = drizzle(env.DB); await db.insert(users).values({ id: userData.id, name: userData.name, email: userData.email, displayName: userData.displayName, username: userData.username, role: userData.role || 'researcher', emailVerified: userData.emailVerified || 0, banned: userData.banned || 0, banReason: userData.banReason, banExpires: userData.banExpires, createdAt: userData.createdAt, updatedAt: userData.updatedAt, }); }Also applies to: 287-301, 306-313, 318-325, 330-363
247-363: Add Zod validation to seed function parameters.The seed functions lack input validation. Adding Zod schemas would provide runtime type safety and clearer error messages when tests pass invalid data.
Based on coding guidelines, which specify "Use Zod for schema and input validation" in
packages/workers/src/**/*.{js,ts}.Example with Zod validation
import { z } from 'zod'; const seedUserSchema = z.object({ id: z.string(), name: z.string(), email: z.string().email(), createdAt: z.number().int(), updatedAt: z.number().int(), role: z.string().default('researcher'), displayName: z.string().nullable().default(null), username: z.string().nullable().default(null), banned: z.number().int().default(0), banReason: z.string().nullable().default(null), banExpires: z.number().int().nullable().default(null), emailVerified: z.number().int().default(0), }); export async function seedUser(data) { const validated = seedUserSchema.parse(data); // ... rest of function }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
eslint.config.js(2 hunks)packages/workers/src/__tests__/helpers.js(1 hunks)packages/workers/src/middleware/__tests__/requireAdmin.test.js(1 hunks)packages/workers/src/routes/__tests__/avatars.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/workers/src/routes/tests/avatars.test.js
- packages/workers/src/middleware/tests/requireAdmin.test.js
- eslint.config.js
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/workers/src/__tests__/helpers.js
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/workers/src/__tests__/helpers.js
packages/workers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/workers/**/*.{js,ts}: Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/__tests__/helpers.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/__tests__/helpers.js
packages/workers/src/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/workers/src/**/*.{js,ts}: Use Zod for schema and input validation on the backend
Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/__tests__/helpers.js
🧠 Learnings (7)
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/migrations/0001_init.sql : When adding new tables or schema changes, edit the existing 0001_init.sql file directly rather than creating separate migration files
Applied to files:
packages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/migrations/*.sql : All migrations should go in a single file: `packages/workers/migrations/0001_init.sql`. Do NOT create separate migration files since this project is not yet in production
Applied to files:
packages/workers/src/__tests__/helpers.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/migrations/0001_init.sql : All migrations should go in a single file: `packages/workers/migrations/0001_init.sql`. Do NOT create separate migration files since this project is not yet in production
Applied to files:
packages/workers/src/__tests__/helpers.js
🧬 Code graph analysis (1)
packages/workers/src/__tests__/helpers.js (6)
packages/workers/src/routes/contact.js (1)
env(56-56)packages/workers/src/routes/email.js (1)
id(38-38)packages/workers/src/routes/avatars.js (1)
userId(188-188)packages/workers/src/__tests__/app.test.js (1)
app(21-21)packages/workers/src/__tests__/health.test.js (1)
app(23-23)packages/workers/src/auth/routes.js (1)
path(114-114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates-workers-prod
- GitHub Check: Workers Builds: corates
🔇 Additional comments (1)
packages/workers/src/__tests__/helpers.js (1)
368-433: Test utility functions look good.The test utility functions (
createTestEnv,json,fetchApp,createAuthHeaders) are well-structured for their purpose. The mocked bindings cover the necessary services (R2, Durable Objects, KV), and the execution context integration is correct.
| const char = withoutComments[i]; | ||
|
|
||
| // Track string boundaries | ||
| if ((char === "'" || char === '"') && (i === 0 || withoutComments[i - 1] !== '\\')) { |
There was a problem hiding this comment.
Fix incorrect SQL quote escaping logic.
SQL uses doubled quotes for escaping (e.g., 'O''Brien'), not backslashes. The current check for withoutComments[i - 1] !== '\\' is incorrect for SQL and may cause parsing errors if the migration SQL contains properly escaped quotes.
Proposed fix
- if ((char === "'" || char === '"') && (i === 0 || withoutComments[i - 1] !== '\\')) {
+ // In SQL, quotes are escaped by doubling them (e.g., 'O''Brien')
+ // Check if this quote is escaped by looking ahead
+ const isEscaped = (char === "'" || char === '"') &&
+ i + 1 < withoutComments.length &&
+ withoutComments[i + 1] === char;
+
+ if ((char === "'" || char === '"') && !isEscaped) {Note: A more robust approach would handle both entry and exit of escaped quotes properly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/workers/src/__tests__/helpers.js around line 193, the quote-escaping
check incorrectly treats backslashes as SQL escapes; replace the backslash logic
with SQL-style doubled-quote handling so that a quote is considered escaped when
the previous character is the same quote (e.g., treat "''" or '""' as an escaped
quote). Update the condition to check that the previous character is not the
same quote character instead of !== '\\', ensuring the parser does not
prematurely toggle quote state for valid SQL escaped quotes.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/landing/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/landing/package.json
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Zod for schema and input validation on the backend
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (1)
packages/landing/package.json (1)
11-11: The build order is correct as-is.The landing package does not depend on
@corates/sharedor@corates/web. Thebuild:prodscript orchestrates a monorepo build where: (1) landing builds first to create the.output/publicdirectory, (2) shared builds to provide dependencies for web, and (3) web builds and copies its output into landing's build directory via thecopy-to-landing.jsscript. This sequence is necessary and should not be changed.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/workers/scripts/generate-test-sql.mjs (1)
17-31: Consider adding directory creation before writing output.The script reads and escapes the migration SQL correctly. However, if the output directory (
src/__tests__/) doesn't exist,writeFileSyncwill fail.Proposed fix to ensure output directory exists
Add this import at the top:
-import { readFileSync, writeFileSync } from 'fs'; +import { readFileSync, writeFileSync, mkdirSync } from 'fs';Then before writing the file:
+ // Ensure output directory exists + mkdirSync(dirname(OUTPUT_FILE), { recursive: true }); + // Write the generated file writeFileSync(OUTPUT_FILE, jsContent, 'utf-8');packages/workers/DRIZZLE_README.md (2)
9-15: Fix markdown formatting issues flagged by linter.Static analysis identified valid markdown formatting concerns:
- The ASCII diagram (lines 9-15) should specify a language identifier (e.g.,
textorplaintext)- Lines 90 and 96 use bold emphasis instead of proper heading syntax
Proposed markdown formatting fixes
Fix 1: Add language to ASCII diagram
-``` +```text Drizzle Schema (src/db/schema.js) ↓Fix 2: Use proper heading for Option A
-**Option A: Manual Update (Current Approach)** +#### Option A: Manual Update (Current Approach)Fix 3: Use proper heading for Option B
-**Option B: Use Drizzle Kit (Future)** +#### Option B: Use Drizzle Kit (Future)Also applies to: 90-90, 96-96
23-25: Consider clarifying TypeScript notation for .js file.The documentation mentions
src/db/schema.jswith "(TypeScript)" in parentheses, which might be slightly confusing since the file extension is.js. If the file uses JSDoc for type annotations, consider clarifying this for readers.Example clarification:
-- **`src/db/schema.js`** - Drizzle ORM schema definitions (TypeScript) +- **`src/db/schema.js`** - Drizzle ORM schema definitions (with JSDoc types)Or if it's actual TypeScript with a .js extension for compatibility, that could be noted as well.
Also applies to: 78-84
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.gitignore(1 hunks)packages/workers/DRIZZLE_README.md(1 hunks)packages/workers/drizzle.config.ts(1 hunks)packages/workers/package.json(3 hunks)packages/workers/scripts/generate-test-sql.mjs(1 hunks)packages/workers/src/__tests__/helpers.js(1 hunks)packages/workers/src/__tests__/seed-schemas.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/workers/src/tests/helpers.js
- packages/workers/package.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/workers/src/__tests__/seed-schemas.jspackages/workers/scripts/generate-test-sql.mjspackages/workers/drizzle.config.tspackages/workers/DRIZZLE_README.md
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/workers/src/__tests__/seed-schemas.jspackages/workers/drizzle.config.ts
packages/workers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/workers/**/*.{js,ts}: Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/__tests__/seed-schemas.jspackages/workers/drizzle.config.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/__tests__/seed-schemas.jspackages/workers/drizzle.config.ts
packages/workers/src/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/workers/src/**/*.{js,ts}: Use Zod for schema and input validation on the backend
Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/__tests__/seed-schemas.js
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Better-Auth for authentication and user management
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Better-Auth for authentication and user management
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Zod for schema and input validation on the backend
Applied to files:
packages/workers/src/__tests__/seed-schemas.js.gitignorepackages/workers/DRIZZLE_README.md
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/**/*.{js,jsx,ts,tsx} : Use Zod for schema and input validation
Applied to files:
packages/workers/src/__tests__/seed-schemas.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
.gitignorepackages/workers/scripts/generate-test-sql.mjspackages/workers/drizzle.config.tspackages/workers/DRIZZLE_README.md
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
.gitignorepackages/workers/scripts/generate-test-sql.mjspackages/workers/drizzle.config.tspackages/workers/DRIZZLE_README.md
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/migrations/*.sql : All migrations should go in a single file: `packages/workers/migrations/0001_init.sql`. Do NOT create separate migration files since this project is not yet in production
Applied to files:
.gitignorepackages/workers/scripts/generate-test-sql.mjs
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/migrations/0001_init.sql : When adding new tables or schema changes, edit the existing 0001_init.sql file directly rather than creating separate migration files
Applied to files:
.gitignorepackages/workers/scripts/generate-test-sql.mjspackages/workers/DRIZZLE_README.md
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/migrations/0001_init.sql : All migrations should go in a single file: `packages/workers/migrations/0001_init.sql`. Do NOT create separate migration files since this project is not yet in production
Applied to files:
.gitignorepackages/workers/scripts/generate-test-sql.mjs
🪛 markdownlint-cli2 (0.18.1)
packages/workers/DRIZZLE_README.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
90-90: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
96-96: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates-workers-prod
- GitHub Check: Workers Builds: corates
🔇 Additional comments (9)
.gitignore (2)
51-51: LGTM!Adding the auto-generated test SQL file to
.gitignoreis appropriate and follows best practices for excluding generated artifacts from version control.
54-54: LGTM! Typo fix for docs ignore pattern.The change from
-/docs/plans*to/docs/plans*corrects what appears to be a typo in the path prefix.packages/workers/scripts/generate-test-sql.mjs (3)
1-12: LGTM!The shebang, imports, and ESM
__dirnamesetup are correct and follow standard Node.js practices.
14-15: LGTM!The path constants correctly reference the single migration file (
0001_init.sql) and the output location matches the.gitignoreentry.
37-40: LGTM!Error handling is appropriate with proper logging and non-zero exit code on failure.
packages/workers/DRIZZLE_README.md (1)
1-237: Excellent comprehensive documentation!The README provides thorough documentation for the Drizzle ORM workflow, including:
- Clear architecture explanation with single source of truth pattern
- Well-defined workflows for schema changes
- Helpful examples for test helpers and seed functions
- Practical troubleshooting guidance
This documentation aligns well with the coding guidelines for using Drizzle ORM for database interactions and migrations.
Based on learnings, this follows the established pattern of using Drizzle ORM for database interactions and migrations in the workers package.
packages/workers/drizzle.config.ts (1)
1-7: LGTM! Configuration is properly structured.The Drizzle Kit configuration correctly specifies the SQLite dialect, schema path (
./src/db/schema.js), and migrations output directory (./migrations). All paths are valid and present in the codebase.packages/workers/src/__tests__/seed-schemas.js (2)
32-37: LGTM!The
dateOrTimestampToNumberhelper is correctly implemented and properly converts both Date objects and Unix timestamps to normalized Unix timestamps in seconds.
42-152: Excellent schema implementation!All five schemas are well-structured with:
- Comprehensive field validation and appropriate error messages
- Consistent date/timestamp transformations to Unix seconds
- Proper boolean-to-integer conversions (0/1) for database compatibility
- Correct handling of nullable/optional fields with sensible defaults
- Enum validation for constrained fields
The schemas follow Zod best practices and align with the coding guidelines for input validation.
As per coding guidelines, Zod is correctly used for schema and input validation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/workers/src/__tests__/seed-schemas.js (2)
7-7: Enums properly centralized.The constants are now correctly imported from the centralized location. The previous review comment about centralizing
SUBSCRIPTION_TIERSandSUBSCRIPTION_STATUSEShas been addressed.
12-17: Helper function is correctly implemented and used throughout.The
dateOrTimestampToNumberhelper is actively used for timestamp normalization in multiple schemas. The previous review comment about removing an unuseddateOrTimestamphelper does not apply here, as this is a different helper with active usage.packages/workers/src/db/subscriptions.js (1)
108-109: Array indexing for defaults inherits fragility from constants definition.These lines use
SUBSCRIPTION_TIERS[0]andSUBSCRIPTION_STATUSES[0]to default to 'free' and 'active'. While the inline comments document the expected values, the code still relies on array ordering. This issue stems from the design inpackages/workers/src/config/constants.js.See the review comment on
packages/workers/src/config/constants.jslines 21-29 for the recommended fix using named constants.Also applies to: 164-165
packages/workers/src/middleware/subscription.js (1)
28-28: Array indexing for defaults inherits fragility from constants definition.These lines use
SUBSCRIPTION_TIERS[0]to default to 'free', relying on array ordering.See the review comment on
packages/workers/src/config/constants.jslines 21-29 for the recommended fix using named constants.Also applies to: 80-80, 123-123
packages/workers/src/routes/billing/index.js (1)
39-40: Array indexing for defaults inherits fragility from constants definition.These lines use
SUBSCRIPTION_TIERS[0]andSUBSCRIPTION_STATUSES[0]to default to 'free' and 'active', relying on array ordering.See the review comment on
packages/workers/src/config/constants.jslines 21-29 for the recommended fix using named constants.
🧹 Nitpick comments (2)
packages/workers/src/__tests__/seed-schemas.js (1)
36-44: Consider extracting nullable date transform logic.The nullable date/timestamp transform logic is duplicated across three fields (
banExpires,currentPeriodStart,currentPeriodEnd). While this is test code with limited duplication, extracting to a reusable helper would improve maintainability.Proposed refactor to reduce duplication
+/** + * Helper for nullable date or timestamp fields + */ +const nullableDateOrTimestamp = z + .union([z.date(), z.number().int(), z.null()]) + .optional() + .transform(val => { + if (val === null || val === undefined) return null; + if (typeof val === 'number') return val; + return Math.floor(val.getTime() / 1000); + }) + .default(null); + export const seedUserSchema = z.object({ // ... other fields - banExpires: z - .union([z.date(), z.number().int(), z.null()]) - .optional() - .transform(val => { - if (val === null || val === undefined) return null; - if (typeof val === 'number') return val; - return Math.floor(val.getTime() / 1000); - }) - .default(null), + banExpires: nullableDateOrTimestamp, }); export const seedSubscriptionSchema = z.object({ // ... other fields - currentPeriodStart: z - .union([z.date(), z.number().int(), z.null()]) - .optional() - .transform(val => { - if (val === null || val === undefined) return null; - if (typeof val === 'number') return val; - return Math.floor(val.getTime() / 1000); - }) - .default(null), - currentPeriodEnd: z - .union([z.date(), z.number().int(), z.null()]) - .optional() - .transform(val => { - if (val === null || val === undefined) return null; - if (typeof val === 'number') return val; - return Math.floor(val.getTime() / 1000); - }) - .default(null), + currentPeriodStart: nullableDateOrTimestamp, + currentPeriodEnd: nullableDateOrTimestamp, });Also applies to: 108-116, 117-125
packages/workers/src/routes/billing/index.js (1)
127-134: Consider enhancing validation error message for better developer experience.The validation correctly rejects the free tier for checkout, but the error reason
'invalid_tier'doesn't explain what constitutes a valid tier. Consider adding additional context to help API consumers understand the constraint.🔎 Suggested enhancement
if (!tier || tier === SUBSCRIPTION_TIERS[0]) { const error = createValidationError( 'tier', VALIDATION_ERRORS.INVALID_INPUT.code, tier, - 'invalid_tier', + `invalid_tier: must be one of ${SUBSCRIPTION_TIERS.slice(1).join(', ')}`, ); return c.json(error, error.statusCode); }This provides actionable feedback to API consumers about valid tier values.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/landing/package.json(1 hunks)packages/workers/scripts/generate-test-sql.mjs(1 hunks)packages/workers/src/__tests__/seed-schemas.js(1 hunks)packages/workers/src/config/constants.js(3 hunks)packages/workers/src/db/subscriptions.js(3 hunks)packages/workers/src/middleware/subscription.js(4 hunks)packages/workers/src/routes/billing/index.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/workers/scripts/generate-test-sql.mjs
- packages/landing/package.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/workers/src/middleware/subscription.jspackages/workers/src/config/constants.jspackages/workers/src/routes/billing/index.jspackages/workers/src/db/subscriptions.jspackages/workers/src/__tests__/seed-schemas.js
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/workers/src/middleware/subscription.jspackages/workers/src/config/constants.jspackages/workers/src/routes/billing/index.jspackages/workers/src/db/subscriptions.jspackages/workers/src/__tests__/seed-schemas.js
packages/workers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/workers/**/*.{js,ts}: Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/middleware/subscription.jspackages/workers/src/config/constants.jspackages/workers/src/routes/billing/index.jspackages/workers/src/db/subscriptions.jspackages/workers/src/__tests__/seed-schemas.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/middleware/subscription.jspackages/workers/src/config/constants.jspackages/workers/src/routes/billing/index.jspackages/workers/src/db/subscriptions.jspackages/workers/src/__tests__/seed-schemas.js
packages/workers/src/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/workers/src/**/*.{js,ts}: Use Zod for schema and input validation on the backend
Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/middleware/subscription.jspackages/workers/src/config/constants.jspackages/workers/src/routes/billing/index.jspackages/workers/src/db/subscriptions.jspackages/workers/src/__tests__/seed-schemas.js
🧠 Learnings (3)
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Zod for schema and input validation on the backend
Applied to files:
packages/workers/src/__tests__/seed-schemas.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/**/*.{js,jsx,ts,tsx} : Use Zod for schema and input validation
Applied to files:
packages/workers/src/__tests__/seed-schemas.js
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/src/__tests__/seed-schemas.js
🧬 Code graph analysis (3)
packages/workers/src/middleware/subscription.js (2)
packages/web/src/primitives/useSubscription.js (1)
subscription(37-46)packages/workers/src/config/constants.js (4)
SUBSCRIPTION_TIERS(24-24)SUBSCRIPTION_TIERS(24-24)SUBSCRIPTION_STATUSES(29-29)SUBSCRIPTION_STATUSES(29-29)
packages/workers/src/db/subscriptions.js (1)
packages/workers/src/config/constants.js (4)
SUBSCRIPTION_TIERS(24-24)SUBSCRIPTION_TIERS(24-24)SUBSCRIPTION_STATUSES(29-29)SUBSCRIPTION_STATUSES(29-29)
packages/workers/src/__tests__/seed-schemas.js (1)
packages/workers/src/config/constants.js (6)
PROJECT_ROLES(9-9)PROJECT_ROLES(9-9)SUBSCRIPTION_TIERS(24-24)SUBSCRIPTION_TIERS(24-24)SUBSCRIPTION_STATUSES(29-29)SUBSCRIPTION_STATUSES(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates-workers-prod
- GitHub Check: Workers Builds: corates
🔇 Additional comments (5)
packages/workers/src/__tests__/seed-schemas.js (1)
70-74: The current enum error syntax is valid in Zod 4. The error parameter accepts both string literals and functions—string literals are the simpler and documented approach. No changes needed.packages/workers/src/config/constants.js (1)
112-128: LGTM! Validators follow established patterns.The subscription tier and status validators are well-structured and consistent with the existing role validators in this file.
packages/workers/src/routes/billing/index.js (3)
56-63: LGTM! Domain error handling is consistent.The error handling properly wraps database errors in domain errors with appropriate context (operation and original error message) and returns the correct status code.
141-149: LGTM! Error handling properly distinguishes domain errors from unexpected failures.The error handlers correctly use
isDomainErrorto pass through domain errors while wrapping unexpected errors. This addresses the past review comments and maintains consistency across both handlers.Also applies to: 165-173
187-209: LGTM! Webhook validation and error handling are well-structured.The signature validation appropriately uses
createValidationErrorfor missing signatures, and the error handler correctly distinguishes domain errors from unexpected failures. This addresses the past review comments.
| /** | ||
| * Valid subscription tiers | ||
| */ | ||
| export const SUBSCRIPTION_TIERS = ['free', 'basic', 'pro', 'team', 'enterprise']; | ||
|
|
||
| /** | ||
| * Valid subscription statuses | ||
| */ | ||
| export const SUBSCRIPTION_STATUSES = ['active', 'canceled', 'past_due', 'trialing', 'incomplete']; |
There was a problem hiding this comment.
Prefer named constants for default tier and status over array indexing.
The codebase uses SUBSCRIPTION_TIERS[0] and SUBSCRIPTION_STATUSES[0] throughout to reference the default 'free' tier and 'active' status. This creates a fragile dependency on array ordering—if these arrays are reordered, the code will break silently with no compile-time or runtime safeguards.
🔎 Recommended fix: Export named constants for defaults
/**
* Valid subscription tiers
*/
export const SUBSCRIPTION_TIERS = ['free', 'basic', 'pro', 'team', 'enterprise'];
+
+/**
+ * Default subscription tier for new users
+ */
+export const DEFAULT_SUBSCRIPTION_TIER = 'free';
/**
* Valid subscription statuses
*/
export const SUBSCRIPTION_STATUSES = ['active', 'canceled', 'past_due', 'trialing', 'incomplete'];
+
+/**
+ * Default subscription status
+ */
+export const DEFAULT_SUBSCRIPTION_STATUS = 'active';Then update all references to SUBSCRIPTION_TIERS[0] and SUBSCRIPTION_STATUSES[0] throughout the codebase to use DEFAULT_SUBSCRIPTION_TIER and DEFAULT_SUBSCRIPTION_STATUS instead.
📝 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.
| /** | |
| * Valid subscription tiers | |
| */ | |
| export const SUBSCRIPTION_TIERS = ['free', 'basic', 'pro', 'team', 'enterprise']; | |
| /** | |
| * Valid subscription statuses | |
| */ | |
| export const SUBSCRIPTION_STATUSES = ['active', 'canceled', 'past_due', 'trialing', 'incomplete']; | |
| /** | |
| * Valid subscription tiers | |
| */ | |
| export const SUBSCRIPTION_TIERS = ['free', 'basic', 'pro', 'team', 'enterprise']; | |
| /** | |
| * Default subscription tier for new users | |
| */ | |
| export const DEFAULT_SUBSCRIPTION_TIER = 'free'; | |
| /** | |
| * Valid subscription statuses | |
| */ | |
| export const SUBSCRIPTION_STATUSES = ['active', 'canceled', 'past_due', 'trialing', 'incomplete']; | |
| /** | |
| * Default subscription status | |
| */ | |
| export const DEFAULT_SUBSCRIPTION_STATUS = 'active'; |
🤖 Prompt for AI Agents
In packages/workers/src/config/constants.js around lines 21 to 29, the default
subscription values are inferred by indexing arrays (SUBSCRIPTION_TIERS[0],
SUBSCRIPTION_STATUSES[0]) which is fragile; add and export explicit named
constants (e.g., DEFAULT_SUBSCRIPTION_TIER = 'free' and
DEFAULT_SUBSCRIPTION_STATUS = 'active') in this file and then replace all usages
of SUBSCRIPTION_TIERS[0] and SUBSCRIPTION_STATUSES[0] across the codebase with
these new named constants to avoid order-dependent bugs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/web/.env.example (1)
9-10: QuoteCharacter warnings are minor style preferences; maintain consistency with file.The static analysis tool flags quoted values as a style issue, but the existing entries (lines 2–6) consistently use quoted values. Keeping the quotes maintains internal consistency.
If you prefer to follow the tool's preferred style, you could remove quotes (since these values contain no spaces or special characters), but ensure all lines are updated consistently:
-VITE_GOOGLE_PICKER_API_KEY="your-google-picker-api-key-here" -VITE_GOOGLE_PICKER_APP_ID="your-google-project-number" +VITE_GOOGLE_PICKER_API_KEY=your-google-picker-api-key-here +VITE_GOOGLE_PICKER_APP_ID=your-google-project-numberOtherwise, the current format is perfectly valid and matches the existing style in the file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/web/.env.example(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
packages/web/.env.example
[warning] 9-9: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 10-10: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
…https://github.com/InfinityBowman/corates into 110-migrate-frontend-to-use-error-codes-like-backend
InfinityBowman
deleted the
110-migrate-frontend-to-use-error-codes-like-backend
branch
github-actions
Bot
restored the
110-migrate-frontend-to-use-error-codes-like-backend
branch
InfinityBowman
deleted the
110-migrate-frontend-to-use-error-codes-like-backend
branch
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.