295 backend typescript migration#296
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request comprehensively migrates the backend workers codebase from JavaScript to TypeScript, introducing strong type safety across authentication, database access, middleware, routes, and utility functions. The effort includes removing planning documentation, adding type definitions for environment and application context, establishing type-first configuration constants, and refactoring all middleware and route handlers with explicit type annotations and error handling patterns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | 87a5875 | Commit Preview URL | Jan 14 2026, 06:09 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| 🔵 In progress View logs |
corates-workers-prod | fb5789b | Jan 14 2026, 05:43 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
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/durable-objects/EmailQueue.ts (1)
105-109: setTimeout may not execute reliably in Durable Objects.Durable Objects can be evicted from memory between requests, which means
setTimeoutcallbacks may never run. For reliable cleanup, use the alarm API instead.🔧 Use alarm for reliable cleanup
// Keep record for a short time for debugging, then delete await this.state.storage.put(`email:${emailRecord.id}`, emailRecord); - // Schedule cleanup - setTimeout(() => this.cleanupSentEmail(emailRecord.id), 60000); + // Schedule cleanup via alarm (more reliable than setTimeout in DOs) + await this.state.storage.put(`cleanup:${emailRecord.id}`, Date.now() + 60000); return true;Then handle cleanup in
processRetryQueueoralarmby checking forcleanup:prefixed entries.packages/workers/src/config/validation.ts (1)
207-228: Use Zod v4's discriminated union pattern to safely access issue properties without type assertions.The function should switch on
issue.codedirectly (not a fallback tokind) and rely on TypeScript's type narrowing to access code-specific properties liketypeandminimumsafely. Replace the type assertions with proper type narrowing:Suggested refactor
function mapZodErrorToValidationCode(issue: z.ZodIssue): ValidationCode { switch (issue.code) { case 'too_small': if (issue.type === 'string' && issue.minimum === 1) { return 'VALIDATION_FIELD_REQUIRED'; } return 'VALIDATION_FIELD_TOO_SHORT'; case 'too_big': return 'VALIDATION_FIELD_TOO_LONG'; case 'invalid_string': return 'VALIDATION_FIELD_INVALID_FORMAT'; case 'invalid_type': return 'VALIDATION_FIELD_INVALID_FORMAT'; default: return 'VALIDATION_FAILED'; } }
🤖 Fix all issues with AI agents
In `@packages/workers/src/auth/config.ts`:
- Around line 196-199: Update the Stripe client instantiation to use the same
API version as the rest of the codebase: change the Stripe constructor call that
creates stripeClient (the new Stripe(env.STRIPE_SECRET_KEY, { apiVersion:
'2025-12-15.clover' }) instance guarded by env.STRIPE_SECRET_KEY &&
env.STRIPE_WEBHOOK_SECRET_AUTH) to use '2025-11-17.clover' so the apiVersion
matches other Stripe clients across the project.
In `@packages/workers/src/auth/email.ts`:
- Around line 165-169: The subject currently uses HTML escaping via the imported
escapeHtml (safeProjectName → subject), which turns characters like & into
&; instead, stop using escapeHtml for the email subject and replace it with
a subject-specific sanitizer: strip control/newline characters, collapse
whitespace, and enforce a max length (e.g., 78 characters) before interpolating
into the template. Update the code around the escapeHtml import and the
safeProjectName/subject construction (remove escapeHtml usage for subject and
call a new or existing sanitizeSubject function that performs control-character
removal, trimming, and length truncation).
In `@packages/workers/src/auth/routes.ts`:
- Around line 440-444: The catch block in the auth route currently returns
err.message to clients (see the catch using const err = error as Error and
c.json), which can leak internals; change the response to always return a
generic error like { error: 'Authentication error' } (no err.message) and keep
the full error details logged server-side (e.g., enhance the existing
console.error('Auth route error:', error) or use the app logger to log
error.stack); optionally, if you must return details for non-production, gate
including err.message behind an environment check (e.g., NODE_ENV !==
'production') so only safe environments receive it.
In `@packages/workers/src/config/constants.ts`:
- Around line 26-29: ACTIVE_STATUSES is using fragile index-based access
(SUBSCRIPTION_STATUSES[0] and [3]); replace those index lookups with explicit
status identifiers so the intent is clear and resilient to reordering—use the
concrete SubscriptionStatus enum members or the exact status string literals
that correspond to the intended active statuses (e.g., SubscriptionStatus.<NAME>
or "status-name") when assigning ACTIVE_STATUSES and update any related
usages/tests accordingly.
In `@packages/workers/src/db/stripeEventLedger.ts`:
- Around line 266-281: The function
getProcessedCheckoutSessionsWithoutSubscription currently fetches all
'checkout.session.completed' rows then filters for LedgerStatus.PROCESSED in
memory and does not check for missing subscriptions; change the DB query in
getProcessedCheckoutSessionsWithoutSubscription to include both conditions in
the WHERE clause (eq(stripeEventLedger.type, 'checkout.session.completed') AND
eq(stripeEventLedger.status, LedgerStatus.PROCESSED) AND
isNull/eq(stripeEventLedger.stripeSubscriptionId, null) or the equivalent for
your query builder) so filtering happens in the database and only rows with
stripeSubscriptionId null are returned; also either rename the function to
reflect its behavior if you intend a different filter, or keep the name but
ensure the query enforces the "without subscription" condition.
In `@packages/workers/src/durable-objects/UserSession.ts`:
- Around line 86-87: The destructuring using Object.values(webSocketPair) is
unsafe because it doesn't guarantee the client/server order; instead access the
pair by index to ensure [0] is the client and [1] is the server. Replace the
Object.values(...) destructuring in the WebSocketPair creation so you explicitly
assign client = webSocketPair[0] and server = webSocketPair[1] (cast server to
WebSocketWithUser if needed), keeping the surrounding logic in the UserSession
class intact.
In `@packages/workers/src/lib/billingResolver.ts`:
- Around line 26-55: There are two divergent isSubscriptionActive
implementations (one in billingResolver and one in entitlements) causing
inconsistent semantics around 'trialing'; consolidate by creating a single
shared function (e.g., isSubscriptionActive(subscription: SubscriptionRecord |
Subscription | null, now?: Date|number, includeTrial = false)) in a common
utility module, implement unified logic handling status ('active', 'past_due',
and conditional 'trialing' when includeTrial=true), periodEnd conversion, and
cancelAtPeriodEnd checks, then replace the current isSubscriptionActive usages
so billingResolver calls it with includeTrial=true and entitlements calls it
with includeTrial=false (or alternatively rename the two existing functions to
clearly indicate their different semantics if you prefer not to refactor).
In `@packages/workers/src/lib/entitlements.ts`:
- Around line 56-67: The quotas double-cast is unsafe and doesn't handle missing
keys: in hasQuota, get the limit via a typed lookup (e.g., const limit =
getEffectiveQuotas(subscription)[quotaKey] or cast to Record<string,
number|undefined> instead of using "as unknown as Record<string,number>"), then
explicitly handle undefined (e.g., if (limit === undefined) return false or
another sensible default) before calling isUnlimitedQuota(limit) and comparing
used+requested <= limit; update the function to remove the "as unknown as"
pattern and add the undefined check.
- Around line 10-22: The isSubscriptionActive implementation in entitlements.ts
only treats status 'active' as active, causing inconsistency with
billingResolver.ts which also treats 'trialing' and 'past_due' (as long as
currentPeriodEnd > now) as active; update entitlements.ts isSubscriptionActive
to match that logic (or import/reuse the isSubscriptionActive from
billingResolver.ts) so it considers 'trialing' and 'past_due' as active when
within the currentPeriodEnd, preserving the existing period-end date handling
and timestamp conversion logic.
In `@packages/workers/src/lib/observability/logger.ts`:
- Around line 248-257: The withTiming function currently mutates the caught
error by attaching durationMs, which can cause surprising side effects; change
the catch block to compute durationMs and throw a new wrapper error (e.g., a
custom TimingError or a new Error with name/message) that includes the original
error as the cause and exposes durationMs (preserve original error via the cause
property or a wrappedError field and copy the stack if needed), so callers
receive duration metadata without mutating the original error object returned
from fn.
In `@packages/workers/src/lib/project-sync.ts`:
- Around line 51-60: The call to projectDoc.fetch in syncProjectToDO does not
validate the response, so HTTP failures are silently ignored; update
syncProjectToDO to capture the Response returned by projectDoc.fetch (the POST
to 'https://internal/sync'), check response.ok or response.status, and throw or
return an error when the status is not in the 2xx range so sync failures are
properly surfaced (mirror the response status check used elsewhere in the repo).
- Around line 31-40: The call to projectDoc.fetch(...) does not validate the
Response, so HTTP errors from the Durable Object are ignored; update the code
that calls projectDoc.fetch (the POST to '/sync-member') to capture the returned
Response, check response.ok (or status), and handle non-2xx results by throwing
or logging an error (include status and text) and/or returning a failure so sync
failures are not silent; ensure this logic lives immediately after the
projectDoc.fetch call in the same function so errors propagate properly.
In `@packages/workers/src/lib/quotaTransaction.ts`:
- Around line 16-51: The function checkQuotaForInsert uses an unsafe double-cast
on orgBilling.quotas and lacks DB error handling; update the Quotas type to
include an index signature (e.g., Record<string, number>) or add a runtime type
guard to safely read quotaKey from orgBilling.quotas instead of
(orgBilling.quotas as unknown as Record<string, number>), and wrap the database
calls (the db.select(...).from(table).where(...) and any await resolveOrgAccess
calls) in a try-catch that converts DB errors into domain errors using
createDomainError(SYSTEM_ERRORS.DB_ERROR, { reason: 'db_query_failed', quotaKey,
orgId }, 'DB query failed') so the function returns a proper domain error when
DB operations fail.
In `@packages/workers/src/middleware/errorHandler.ts`:
- Around line 83-89: The current error handler uses process.env which isn't
available in Cloudflare Workers; update the logic around
createDomainError(SYSTEM_ERRORS.INTERNAL_ERROR) to use the Hono context
environment instead (c.env.ENVIRONMENT) or refactor into a factory like
createErrorHandler(isProduction: boolean) that returns the ErrorHandler
signature used by the middleware; then conditionally include originalError and
stack when not in production based on c.env.ENVIRONMENT or the injected
isProduction flag and return c.json(error, error.statusCode as 500) as before.
In `@packages/workers/src/middleware/requireOrgWriteAccess.ts`:
- Around line 23-24: The call to resolveOrgAccess (using createDb((c as
AppContext).env.DB)) can throw on DB failures; wrap the database operation in a
try-catch inside the requireOrgWriteAccess middleware, catching errors from
resolveOrgAccess, and return the domain error using SYSTEM_ERRORS.DB_ERROR
(include the caught error details in the returned/logged payload) instead of
letting the exception propagate; keep references to createDb, AppContext,
resolveOrgAccess, orgBilling and SYSTEM_ERRORS.DB_ERROR to locate where to make
the change.
In `@packages/workers/src/routes/avatars.ts`:
- Around line 175-178: Remove the redundant null/undefined checks that follow
the requireAuth middleware: delete the `if (!user) { const error =
createDomainError(AUTH_ERRORS.REQUIRED, { reason: 'no_user' }); return
c.json(error, error.statusCode as ContentfulStatusCode); }` blocks (the ones
after retrieving `user` at both locations) because `requireAuth` guarantees
`session.user` and sets `c.set('user', session.user)` so handlers always have a
defined user; after removing them, update any subsequent usage to treat `user`
as non-null (use the existing `getAuth`/`c.get('user')` return as defined or add
a non-null assertion) and run tests to ensure no other code relies on those
early returns.
In `@packages/workers/src/routes/members.ts`:
- Around line 501-503: The code unsafely asserts c.env to access BASEPATH; add
BASEPATH?: string to the Env interface (packages/workers/src/types/env.ts) so
BASEPATH can be accessed type-safely, then update the access in members.ts to
use c.env.BASEPATH ?? '' (or use optional chaining like c.env?.BASEPATH ?? '')
instead of the (c.env as unknown as Record<...>).BASEPATH cast; ensure
imports/types align so TypeScript recognizes the new Env field.
🧹 Nitpick comments (35)
packages/workers/src/lib/access.ts (1)
1-4: Consider exporting the interface and using a stricter status type.The
Subscriptioninterface is not exported, which limits reusability for consumers of these utilities. Additionally,statusas a genericstringreduces type safety—subscription statuses from providers like Stripe have well-known values.Suggested improvement
-interface Subscription { - status: string; +export interface Subscription { + status: 'active' | 'canceled' | 'incomplete' | 'incomplete_expired' | 'past_due' | 'trialing' | 'unpaid' | 'paused'; currentPeriodEnd?: number | null; }packages/workers/src/middleware/requireOrgWriteAccess.ts (1)
24-24: Redundant type assertion.
resolveOrgAccessalready returnsPromise<OrgBilling>, so theas OrgBillingcast is unnecessary.packages/workers/src/lib/mock-templates.ts (1)
247-254: Consider usingcrypto.randomUUID()for consistency.The
generateIdfunction usesMath.random()to generate a 12-character ID, while the test factory inpackages/workers/src/__tests__/factories/utils.jsusescrypto.randomUUID(). For mock data, the current implementation works fine, but usingcrypto.randomUUID()would provide better uniqueness guarantees and consistency across the codebase.♻️ Suggested refactor
-function generateId(prefix: string = ''): string { - const chars = 'abcdefghijklmnopqrstuvwxyz0123456789'; - let id = ''; - for (let i = 0; i < 12; i++) { - id += chars[Math.floor(Math.random() * chars.length)]; - } - return prefix ? `${prefix}_${id}` : id; +function generateId(prefix: string = ''): string { + const uuid = crypto.randomUUID(); + return prefix ? `${prefix}_${uuid.slice(0, 8)}` : uuid; }packages/workers/src/durable-objects/dev-handlers.ts (1)
506-511: Consider adding a type guard before casting toY.Map.The code assumes that all intermediate path values are
Y.Map<unknown>. If a path targets a primitive value (e.g.,studies.id.name.something), the cast on line 511 could lead to unexpected behavior when trying to callget()on a non-map value.For dev-only code this is acceptable, but a defensive check could improve robustness:
♻️ Suggested improvement
const next = yMap.get(part); if (!next) { throw new Error(`Path not found: ${pathParts.slice(0, i + 1).join('.')}`); } + if (!(next instanceof Y.Map)) { + throw new Error(`Path element is not a map: ${pathParts.slice(0, i + 1).join('.')}`); + } target = next as Y.Map<unknown>;packages/workers/src/middleware/securityHeaders.ts (1)
43-57: Consider tightening CSP for production HTML responses.The production CSP includes
'unsafe-inline'forstyle-src, which weakens XSS protections. If inline styles can be avoided or nonce-based, this would improve security posture.packages/workers/src/durable-objects/UserSession.ts (1)
94-100: Add error handling for Durable Object storage operations.Storage operations (
storage.get,storage.put) can fail. Without try-catch, failures will cause unhandled rejections and potentially leave the WebSocket in an inconsistent state.🔧 Proposed fix
// Send any pending notifications - const pending = (await this.state.storage.get<Notification[]>('pendingNotifications')) || []; - if (pending.length > 0) { - for (const notification of pending) { - server.send(JSON.stringify(notification)); + try { + const pending = (await this.state.storage.get<Notification[]>('pendingNotifications')) || []; + if (pending.length > 0) { + for (const notification of pending) { + server.send(JSON.stringify(notification)); + } + await this.state.storage.put('pendingNotifications', []); } - await this.state.storage.put('pendingNotifications', []); + } catch (err) { + console.error('Failed to process pending notifications:', err); }packages/workers/src/middleware/requireOrg.ts (2)
29-39: Add try-catch around database query for consistency.The
requireProjectAccessmiddleware wraps its DB operations in try-catch blocks (lines 98-117, 134-151), butrequireOrgMembershipdoes not. For consistency and resilience, wrap this query as well.🔧 Proposed fix
const db = createDb((c as AppContext).env.DB); + let membership; + try { - const membership = await db + membership = await db .select({ id: member.id, role: member.role, orgName: organization.name, orgSlug: organization.slug, }) .from(member) .innerJoin(organization, eq(member.organizationId, organization.id)) .where(and(eq(member.organizationId, orgId), eq(member.userId, user.id))) .get(); + } catch (err) { + const dbError = createDomainError(SYSTEM_ERRORS.DB_ERROR, { + operation: 'check_org_membership', + orgId, + userId: user.id, + originalError: err instanceof Error ? err.message : String(err), + }); + return c.json(dbError, dbError.statusCode as 500); + }
70-93: Consider reordering user check before orgId check.The user authentication check (lines 90-93) happens after the
orgIdcheck. For consistency withrequireOrgMembershipand to fail fast on unauthenticated requests, consider moving the user check earlier.🔧 Proposed reordering
export function requireProjectAccess(minRole?: string): MiddlewareHandler { return async (c, next) => { const { user } = getAuth(c); + + if (!user) { + const error = createDomainError(AUTH_ERRORS.REQUIRED); + return c.json(error, error.statusCode as 401); + } + const orgId = c.get('orgId') as string | undefined; const projectId = c.req.param('projectId'); if (!orgId) { const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { reason: 'org_context_required', }); return c.json(error, error.statusCode as 403); } if (!projectId) { const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { reason: 'project_id_required', }); return c.json(error, error.statusCode as 403); } - if (!user) { - const error = createDomainError(AUTH_ERRORS.REQUIRED); - return c.json(error, error.statusCode as 401); - }packages/workers/src/config/origins.ts (1)
13-16: Consider exportingEnvWithOriginsfor reuse.This interface may be useful in other modules that need to type environment bindings with origin-related fields. If this interface is only used internally, it's fine as-is.
♻️ Suggested change
-interface EnvWithOrigins { +export interface EnvWithOrigins { ALLOWED_ORIGINS?: string; AUTH_BASE_URL?: string; }packages/workers/src/lib/entitlements.ts (1)
46-49: Unsafe type assertion bypasses type checking.Casting
entitlements as unknown as Record<string, boolean>defeats the purpose of theEntitlementstype. IfEntitlementsis a well-defined type from@corates/shared/plans, consider using proper type-safe access or adding type guards.♻️ Suggested approach
If
Entitlementshas a known shape, define a type-safe accessor:export function hasEntitlement( subscription: Subscription | null, entitlement: keyof Entitlements, ): boolean { const entitlements = getEffectiveEntitlements(subscription); return entitlements[entitlement] === true; }Alternatively, if entitlement keys are dynamic, consider validating
entitlementagainst known keys at runtime.packages/workers/src/lib/notify.ts (1)
89-98: Sequential notification may be slow for organizations with many members.Notifications are sent one-by-one in a loop. For organizations with many members, this could cause significant latency. Consider using
Promise.allorPromise.allSettledfor parallel execution.♻️ Suggested parallel notification approach
- for (const m of members) { - if (excludeUserIds.includes(m.userId)) continue; - - const result = await notifyUser(env, m.userId, event); - if (result.success) { - notified++; - } else { - failed++; - } - } + const eligibleMembers = members.filter(m => !excludeUserIds.includes(m.userId)); + const results = await Promise.allSettled( + eligibleMembers.map(m => notifyUser(env, m.userId, event)) + ); + + for (const result of results) { + if (result.status === 'fulfilled' && result.value.success) { + notified++; + } else { + failed++; + } + }packages/workers/src/db/orgAccessGrants.ts (1)
29-51: Post-query filtering is inefficient; consider SQL-based filtering.The function fetches all non-revoked grants for an org, then filters by
startsAtandexpiresAtin JavaScript. For organizations with many grants, this is inefficient. Since the schema stores these as integers (Unix timestamps), you can filter directly in the query.♻️ Suggested SQL-based filtering
export async function getActiveGrantsByOrgId( db: Database, orgId: string, now: Date | number, ): Promise<OrgAccessGrant[]> { const nowTimestamp = now instanceof Date ? Math.floor(now.getTime() / 1000) : now; + const { lte, gt } = await import('drizzle-orm'); + const results = await db .select() .from(orgAccessGrants) - .where(and(eq(orgAccessGrants.orgId, orgId), isNull(orgAccessGrants.revokedAt))) + .where( + and( + eq(orgAccessGrants.orgId, orgId), + isNull(orgAccessGrants.revokedAt), + lte(orgAccessGrants.startsAt, nowTimestamp), + gt(orgAccessGrants.expiresAt, nowTimestamp) + ) + ) .orderBy(desc(orgAccessGrants.expiresAt)) .all(); - return results.filter(grant => { - const startsAt = - grant.startsAt instanceof Date ? Math.floor(grant.startsAt.getTime() / 1000) : grant.startsAt; - const expiresAt = - grant.expiresAt instanceof Date ? - Math.floor(grant.expiresAt.getTime() / 1000) - : grant.expiresAt; - return startsAt <= nowTimestamp && nowTimestamp < expiresAt; - }); + return results; }packages/workers/src/durable-objects/ProjectDoc.ts (1)
214-225: UnusedreqWithUservariable.The
reqWithUseris cast anduseris assigned to it at line 224, but it's never used afterward. The authenticated user is verified but the enriched request object isn't passed to any handler. Consider removing this dead code if the user context isn't needed downstream.♻️ Suggested cleanup
// For HTTP requests verify auth (unless it's an upgrade to websocket) - const reqWithUser = request as RequestWithUser; if (upgradeHeader !== 'websocket') { const { user } = await verifyAuth(request, this.env); if (!user) { return new Response(JSON.stringify({ error: 'Authentication required' }), { status: 401, headers: { 'Content-Type': 'application/json' }, }); } - reqWithUser.user = user as { id: string; [key: string]: unknown }; }packages/workers/src/db/stripeEventLedger.ts (1)
51-54:sinceoption inQueryOptionsis never utilized.The
since?: Datefield is defined in the interface but none of the query functions use it to filter results. Either implement the filtering or remove the unused option to avoid confusion.packages/workers/src/middleware/csrf.ts (2)
31-32: Consider using the actual statusCode instead of casting.The cast
as 403bypasses type safety. IfAUTH_ERRORS.FORBIDDEN.statusCodewere ever changed, this would silently become a type lie. Since the error object already contains the status code, you could useerror.statusCodedirectly if the response type allows, or ensure the error constant is typed with a literal403.
43-48: Same statusCode casting concern applies here.Same observation as above regarding the
as 403cast.packages/workers/src/middleware/requireQuota.ts (3)
11-16: Remove redundant local Logger interface.The
Loggerinterface duplicates the type exported bycreateLogger. Import the type instead to ensure consistency:Suggested fix
-interface Logger { - error: (_message: string, _data?: Record<string, unknown>) => void; - warn: (_message: string, _data?: Record<string, unknown>) => void; - info: (_message: string, _data?: Record<string, unknown>) => void; - debug: (_message: string, _data?: Record<string, unknown>) => void; -} +import type { Logger } from '../lib/observability/logger';
62-62: Double type cast indicates type mismatch.The
as unknown as Record<string, number>double cast is a code smell. TheQuotastype fromOrgBillingshould already be indexable by string ifquotaKeyis dynamic.Consider typing
quotaKeymore strictly or ensuringQuotastype supports index signatures. The double cast bypasses type safety and could mask runtime errors if quota keys don't exist.- const limit = (orgBilling.quotas as unknown as Record<string, number>)[quotaKey]; + const limit = orgBilling.quotas[quotaKey as keyof typeof orgBilling.quotas];If
quotaKeymust be dynamic, add a runtime check:const limit = quotaKey in orgBilling.quotas ? orgBilling.quotas[quotaKey as keyof typeof orgBilling.quotas] : 0;
63-70: Redundant ternary in error message.On line 67,
isUnlimitedQuota(limit) ? 'unlimited' : limitis alwayslimitbecause the condition on line 63 already ensures!isUnlimitedQuota(limit).Simplify the error message
const error = createDomainError( AUTH_ERRORS.FORBIDDEN, { reason: 'quota_exceeded', quotaKey, used, limit, requested }, - `Quota exceeded: ${quotaKey}. Current usage: ${used}, Limit: ${isUnlimitedQuota(limit) ? 'unlimited' : limit}, Requested: ${requested}`, + `Quota exceeded: ${quotaKey}. Current usage: ${used}, Limit: ${limit}, Requested: ${requested}`, );packages/workers/src/middleware/auth.ts (1)
47-52: Consider a type-safe alternative for context access.The type casts on
c.get()are necessary but could be improved by leveraging Hono's typed context ifAppContextis used consistently throughout the codebase.If
AppContextfrom../types/contextis used as the context type in routes, this function could be typed more strictly:♻️ Optional improvement
-export function getAuth(c: Context): { user: AuthUser | null; session: AuthSession | null } { +import type { AppContext } from '../types/context'; + +export function getAuth(c: AppContext): { user: AuthUser | null; session: AuthSession | null } { return { - user: c.get('user') as AuthUser | null, - session: c.get('session') as AuthSession | null, + user: c.get('user'), + session: c.get('session'), }; }packages/workers/src/types/context.ts (1)
69-86: Weak typing onsubscriptionfield.The
subscription?: unknownon line 82 loses type safety. Consider using a more specific type or the same structure asOrgBilling.subscription.♻️ Suggested improvement
export interface AppVariables { user: AuthUser | null; session: AuthSession | null; orgId?: string; orgRole?: string; org?: OrgContext; projectId?: string; projectRole?: string; project?: ProjectContext; orgBilling?: OrgBilling; entitlements?: Entitlements; quotas?: Quotas; isAdmin?: boolean; - subscription?: unknown; + subscription?: OrgBilling['subscription']; tier?: string; }packages/workers/src/routes/email.ts (1)
124-151: Document the@ts-expect-errorreason more explicitly.The
@ts-expect-errorcomment would benefit from a more specific explanation of what the type mismatch is, helping future maintainers understand the workaround.♻️ Suggested improvement
-// `@ts-expect-error` OpenAPIHono strict return types don't account for error responses +// `@ts-expect-error` OpenAPIHono expects handler to return only schema-defined response types, +// but we also return error responses (400, 429, 500) which are valid but not inferred emailRoutes.openapi(queueEmailRoute, async c => {The error handling correctly uses
createDomainErrorwithSYSTEM_ERRORS.EMAIL_SEND_FAILEDper guidelines.packages/workers/src/middleware/rateLimit.ts (1)
155-166: Test environment detection could be more robust.The type assertion on
import.metais necessary but fragile. Consider extracting this to a shared utility for consistency across the codebase.♻️ Optional improvement
+function isTestEnvironment(): boolean { + if (typeof process !== 'undefined' && process.env.NODE_ENV === 'test') { + return true; + } + if (typeof import.meta !== 'undefined') { + const meta = import.meta as { env?: { MODE?: string } }; + return meta.env?.MODE === 'test'; + } + return false; +} + export function clearRateLimitStore(): void { - const isTest = - (typeof process !== 'undefined' && process.env.NODE_ENV === 'test') || - (typeof import.meta !== 'undefined' && - (import.meta as { env?: { MODE?: string } }).env?.MODE === 'test'); - - if (isTest) { + if (isTestEnvironment()) { rateLimitStore.clear(); } else { console.warn('Attempted to clear rate limit store outside of test environment'); } }packages/workers/src/routes/contact.ts (1)
185-186: Unsafe type cast for environment variable access.The cast
(env as unknown as Record<string, string | undefined>).CONTACT_EMAILbypasses TypeScript's type safety. IfCONTACT_EMAILis a valid environment variable, it should be added to theEnvtype definition. If it's optional or dynamically set, consider a more explicit approach.♻️ Suggested approaches
Option 1: Add CONTACT_EMAIL to Env type (preferred)
// In packages/workers/src/types/env.ts export interface Env { // ... existing properties CONTACT_EMAIL?: string; }Then use it directly:
- const contactEmail = - (env as unknown as Record<string, string | undefined>).CONTACT_EMAIL ?? 'contact@corates.org'; + const contactEmail = env.CONTACT_EMAIL ?? 'contact@corates.org';Option 2: If truly dynamic, document why:
// CONTACT_EMAIL may be set dynamically in certain environments const contactEmail = (env as Record<string, unknown>).CONTACT_EMAIL as string | undefined ?? 'contact@corates.org';packages/workers/src/lib/billingResolver.ts (1)
73-89: Consider improving type inference from Drizzle queries.The repeated
as SubscriptionRecordcasts suggest the Drizzle query results don't match the expected interface. While functional, this could mask type mismatches.♻️ Alternative approach using Drizzle's type inference
You could leverage Drizzle's
$inferSelector define the query result type more precisely:// Option: Use Drizzle's inferred type and map explicitly const subscriptions = await db .select({ id: subscription.id, referenceId: subscription.referenceId, status: subscription.status, plan: subscription.plan, periodEnd: subscription.periodEnd, cancelAtPeriodEnd: subscription.cancelAtPeriodEnd, }) .from(subscription) .where(eq(subscription.referenceId, orgId)) .orderBy(desc(subscription.periodEnd)) .all(); // Now subscriptions should be properly typed without castingpackages/workers/src/middleware/errorHandler.ts (2)
108-112:asyncHandleris a pass-through with no additional behavior.This wrapper doesn't add error handling - it just returns the function unchanged. If it's only for TypeScript type inference, consider adding a comment explaining its purpose. If error catching was intended, the implementation is incomplete.
💡 If this is intentional for typing
+/** + * Type-preserving wrapper for async route handlers. + * Error handling is delegated to Hono's onError middleware. + */ export function asyncHandler<T>( fn: (_c: Context, _next: () => Promise<void>) => Promise<T>, ): (_c: Context, _next: () => Promise<void>) => Promise<T> { return (c, next) => fn(c, next); }
14-20: Consider using Zod's actual type guard for more robust detection.The current
isZodErrorrelies on checking.name === 'ZodError'or the presence of an.errorsarray, which could match non-Zod objects. Ifzodis already a dependency, you can useinstanceofor Zod's exported type.♻️ More robust approach
import { ZodError as ZodErrorType } from 'zod'; function isZodError(error: unknown): error is ZodErrorType { return error instanceof ZodErrorType; }Or if you want to avoid the import for bundle size, the current approach is acceptable with a comment explaining the heuristic.
packages/workers/src/routes/health.ts (1)
40-50: Consider usingz.inferto avoid type duplication.The
HealthChecksinterface manually duplicates the shape defined inHealthResponseSchema. Using Zod's type inference would keep them in sync automatically.♻️ Use z.infer for consistency
type ServiceStatus = z.infer<typeof ServiceStatusSchema>; +type HealthChecks = z.infer<typeof HealthResponseSchema>; -interface HealthChecks { - status: 'healthy' | 'degraded'; - timestamp: string; - services: { - database?: ServiceStatus; - storage?: ServiceStatus; - durableObjects?: ServiceStatus; - }; -}packages/workers/src/routes/avatars.ts (1)
230-237: Old avatar cleanup is not atomic.The deletion of old avatars iterates and deletes objects sequentially. If a failure occurs mid-loop, some old avatars may remain. While this is wrapped in try-catch and logged as a warning (acceptable for cleanup), consider noting that partial cleanup is a known possibility.
💡 Optional: Parallel deletion for better performance
try { const oldAvatars = await c.env.PDF_BUCKET.list({ prefix: `avatars/${user.id}/` }); - for (const obj of oldAvatars.objects) { - await c.env.PDF_BUCKET.delete(obj.key); - } + await Promise.all(oldAvatars.objects.map(obj => c.env.PDF_BUCKET.delete(obj.key))); } catch (e) { console.warn('Failed to delete old avatar:', e); }packages/workers/src/durable-objects/EmailQueue.ts (1)
46-51: Consider using Zod for payload validation.The current validation is manual. Per coding guidelines, Zod should be used for schema validation on the backend. This would provide better type safety and more informative error messages.
♻️ Suggested Zod validation
+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 is required', +}); // In fetch method: - const payload = (await request.json()) as EmailPayload | null; - // Minimally validate - if (!payload?.to || !payload?.subject || (!payload?.html && !payload?.text)) { - return new Response(JSON.stringify({ error: 'Invalid payload' }), { status: 400 }); - } + const parsed = emailPayloadSchema.safeParse(await request.json()); + if (!parsed.success) { + return new Response(JSON.stringify({ error: parsed.error.message }), { status: 400 }); + } + const payload = parsed.data;packages/workers/src/config/validation.ts (1)
303-311: Consider logging JSON parse errors for debugging.The empty catch clause discards potentially useful debugging information about malformed request bodies.
♻️ Add debug logging
- } catch { + } catch (err) { + console.debug('Invalid JSON in request body:', (err as Error).message); const invalidJsonError = createValidationError( 'body', 'VALIDATION_INVALID_INPUT', null, 'invalid_json', ); return c.json(invalidJsonError, invalidJsonError.statusCode as 400); }packages/workers/src/auth/config.ts (1)
518-573: Personal org bootstrap hook is well-implemented.The after hook correctly:
- Checks for existing memberships before creating
- Creates org and membership atomically
- Sets activeOrganizationId on session
- Handles errors gracefully without failing auth
However, per coding guidelines, consider using
db.batch()for the two related inserts (organization and member) to ensure atomicity.♻️ Consider using db.batch() for atomic inserts
- await db.insert(schema.organization).values({ - id: orgId, - name: `${userName}'s Workspace`, - slug, - metadata: JSON.stringify({ type: 'personal' }), - createdAt: now, - }); - - await db.insert(schema.member).values({ - id: memberId, - userId, - organizationId: orgId, - role: 'owner', - createdAt: now, - }); + await db.batch([ + db.insert(schema.organization).values({ + id: orgId, + name: `${userName}'s Workspace`, + slug, + metadata: JSON.stringify({ type: 'personal' }), + createdAt: now, + }), + db.insert(schema.member).values({ + id: memberId, + userId, + organizationId: orgId, + role: 'owner', + createdAt: now, + }), + ]);Based on learnings, use
db.batch()for multiple related database operations in Drizzle to ensure atomicity.packages/workers/src/lib/observability/logger.ts (1)
46-55: Logger interface is well-designed.The interface provides a clean API with proper return types. The underscore prefixes in parameter names (
_message,_data, etc.) are unconventional for interface definitions - these are typically used for unused parameters, but here they serve as parameter names.💡 Consider removing underscore prefixes from interface parameters
export interface Logger { requestId: string; cfRay: string | null; - debug: (_message: string, _data?: Record<string, unknown>) => LogEntry; - info: (_message: string, _data?: Record<string, unknown>) => LogEntry; - warn: (_message: string, _data?: Record<string, unknown>) => LogEntry; - error: (_message: string, _data?: Record<string, unknown>) => LogEntry; - stripe: (_action: string, _data?: StripeLogData) => LogEntry; - child: (_context: Record<string, unknown>) => Logger; + debug: (message: string, data?: Record<string, unknown>) => LogEntry; + info: (message: string, data?: Record<string, unknown>) => LogEntry; + warn: (message: string, data?: Record<string, unknown>) => LogEntry; + error: (message: string, data?: Record<string, unknown>) => LogEntry; + stripe: (action: string, data?: StripeLogData) => LogEntry; + child: (context: Record<string, unknown>) => Logger; }packages/workers/src/routes/members.ts (2)
328-329: Consider addressing the OpenAPIHono type issue systematically.Multiple
@ts-expect-errorcomments indicate a recurring type mismatch. This could be addressed by extending the route response types or creating a wrapper utility.
733-733: Non-null assertions on memberId are risky.The
memberId!assertions assume the path parameter is always present, but the route param definition doesn't guarantee this at the type level. Consider adding a guard or using the validated params.💡 Add early guard for memberId
memberRoutes.openapi(updateRoleRoute, async c => { const projectId = c.get('projectId'); const memberId = c.req.param('userId'); const db = createDb(c.env.DB); const { role } = c.req.valid('json'); const { user: authUser } = getAuth(c); + if (!memberId) { + const error = createDomainError(VALIDATION_ERRORS.FIELD_REQUIRED, { field: 'userId' }); + return c.json(error, error.statusCode as ContentfulStatusCode); + } + if (!authUser) { const error = createDomainError(AUTH_ERRORS.REQUIRED, { reason: 'no_user' }); return c.json(error, error.statusCode as ContentfulStatusCode); } try { await requireMemberManagement(db, authUser.id, projectId); - await requireSafeRoleChange(db, projectId, memberId!, role); + await requireSafeRoleChange(db, projectId, memberId, role);This applies similarly to
removeMemberRoutewherememberIdis also used with non-null assertions.Also applies to: 745-745, 749-749
| if (env.STRIPE_SECRET_KEY && env.STRIPE_WEBHOOK_SECRET_AUTH) { | ||
| const stripeClient = new Stripe(env.STRIPE_SECRET_KEY, { | ||
| apiVersion: '2025-11-17.clover', | ||
| apiVersion: '2025-12-15.clover', | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat packages/workers/src/auth/config.ts | head -220 | tail -30Repository: InfinityBowman/corates
Length of output: 1262
🏁 Script executed:
# Search for Stripe version patterns in the codebase
rg "apiVersion" packages/workers/src/auth/ -A 2 -B 2Repository: InfinityBowman/corates
Length of output: 1962
🏁 Script executed:
# Check if there are other Stripe client instantiations for comparison
rg "new Stripe" packages/ -A 3 -B 1Repository: InfinityBowman/corates
Length of output: 7466
🌐 Web query:
Stripe API version 2025-12-15 clover valid format
💡 Result:
Yes — "2025-12-15.clover" is a valid Stripe API version string. Format: YYYY-MM-DD.clover (date + ".clover" release tag); the API major name is "clover". [1][2]
Sources:
[1] Stripe changelog (Clover releases listing 2025-12-15.clover).
[2] Stripe SDK type file showing ApiVersion like "2025-10-29.clover" and ApiMajorVersion = "clover".
Align Stripe API version with rest of codebase.
The version '2025-12-15.clover' is valid, but it's inconsistent with other Stripe client instantiations in the codebase, which use '2025-11-17.clover'. Found in:
- packages/workers/src/routes/admin/stats.js
- packages/workers/src/routes/billing/webhooks.js
- packages/workers/src/routes/billing/checkout.js
- packages/workers/src/routes/billing/invoices.js
- packages/workers/src/routes/admin/stripe-tools.js
- packages/workers/scripts/setup-stripe-test.mjs
- packages/workers/src/routes/admin/billing-observability.js
Update to '2025-11-17.clover' to maintain consistency across the application.
🤖 Prompt for AI Agents
In `@packages/workers/src/auth/config.ts` around lines 196 - 199, Update the
Stripe client instantiation to use the same API version as the rest of the
codebase: change the Stripe constructor call that creates stripeClient (the new
Stripe(env.STRIPE_SECRET_KEY, { apiVersion: '2025-12-15.clover' }) instance
guarded by env.STRIPE_SECRET_KEY && env.STRIPE_WEBHOOK_SECRET_AUTH) to use
'2025-11-17.clover' so the apiVersion matches other Stripe clients across the
project.
| // Note: Email subjects are plain text, not HTML, so we don't need HTML escaping | ||
| // However, we should still sanitize to prevent issues with email clients | ||
| const { escapeHtml } = await import('../lib/escapeHtml.js'); | ||
| const { escapeHtml } = await import('../lib/escapeHtml'); | ||
| const safeProjectName = escapeHtml(projectName); | ||
| const subject = `You're Invited to "${safeProjectName}" - CoRATES`; |
There was a problem hiding this comment.
HTML escaping in email subjects may produce unwanted output.
The comment correctly notes that email subjects are plain text, but the code still applies HTML escaping. This could result in subjects like You're Invited to "R&D Project" instead of You're Invited to "R&D Project".
Consider sanitizing differently for subjects (removing control characters, limiting length) versus HTML body content.
🔧 Suggested approach
// Note: Email subjects are plain text, not HTML, so we don't need HTML escaping
- // However, we should still sanitize to prevent issues with email clients
- const { escapeHtml } = await import('../lib/escapeHtml');
- const safeProjectName = escapeHtml(projectName);
+ // Sanitize for email subject: remove control chars, trim excessive length
+ const safeProjectName = projectName.replace(/[\x00-\x1f]/g, '').slice(0, 100);
const subject = `You're Invited to "${safeProjectName}" - CoRATES`;🤖 Prompt for AI Agents
In `@packages/workers/src/auth/email.ts` around lines 165 - 169, The subject
currently uses HTML escaping via the imported escapeHtml (safeProjectName →
subject), which turns characters like & into &; instead, stop using
escapeHtml for the email subject and replace it with a subject-specific
sanitizer: strip control/newline characters, collapse whitespace, and enforce a
max length (e.g., 78 characters) before interpolating into the template. Update
the code around the escapeHtml import and the safeProjectName/subject
construction (remove escapeHtml usage for subject and call a new or existing
sanitizeSubject function that performs control-character removal, trimming, and
length truncation).
| } catch (error) { | ||
| const err = error as Error; | ||
| console.error('Auth route error:', error); | ||
| return c.json({ error: 'Authentication error', details: error.message }, 500); | ||
| return c.json({ error: 'Authentication error', details: err.message }, 500); | ||
| } |
There was a problem hiding this comment.
Avoid exposing internal error details to clients.
Line 443 returns err.message directly in the response, which could leak internal implementation details or stack traces to clients. Consider using a generic error message in production.
🔒 Suggested fix
} catch (error) {
const err = error as Error;
console.error('Auth route error:', error);
- return c.json({ error: 'Authentication error', details: err.message }, 500);
+ return c.json({ error: 'Authentication error' }, 500);
}📝 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.
| } catch (error) { | |
| const err = error as Error; | |
| console.error('Auth route error:', error); | |
| return c.json({ error: 'Authentication error', details: error.message }, 500); | |
| return c.json({ error: 'Authentication error', details: err.message }, 500); | |
| } | |
| } catch (error) { | |
| const err = error as Error; | |
| console.error('Auth route error:', error); | |
| return c.json({ error: 'Authentication error' }, 500); | |
| } |
🤖 Prompt for AI Agents
In `@packages/workers/src/auth/routes.ts` around lines 440 - 444, The catch block
in the auth route currently returns err.message to clients (see the catch using
const err = error as Error and c.json), which can leak internals; change the
response to always return a generic error like { error: 'Authentication error' }
(no err.message) and keep the full error details logged server-side (e.g.,
enhance the existing console.error('Auth route error:', error) or use the app
logger to log error.stack); optionally, if you must return details for
non-production, gate including err.message behind an environment check (e.g.,
NODE_ENV !== 'production') so only safe environments receive it.
| export const ACTIVE_STATUSES: readonly SubscriptionStatus[] = [ | ||
| SUBSCRIPTION_STATUSES[0], | ||
| SUBSCRIPTION_STATUSES[3], | ||
| ]; |
There was a problem hiding this comment.
Fragile index-based access for ACTIVE_STATUSES.
Using SUBSCRIPTION_STATUSES[0] and SUBSCRIPTION_STATUSES[3] is brittle—if the array order changes, this will silently break. Use explicit values for clarity and safety.
🐛 Suggested fix
export const ACTIVE_STATUSES: readonly SubscriptionStatus[] = [
- SUBSCRIPTION_STATUSES[0],
- SUBSCRIPTION_STATUSES[3],
+ 'active',
+ 'trialing',
];📝 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.
| export const ACTIVE_STATUSES: readonly SubscriptionStatus[] = [ | |
| SUBSCRIPTION_STATUSES[0], | |
| SUBSCRIPTION_STATUSES[3], | |
| ]; | |
| export const ACTIVE_STATUSES: readonly SubscriptionStatus[] = [ | |
| 'active', | |
| 'trialing', | |
| ]; |
🤖 Prompt for AI Agents
In `@packages/workers/src/config/constants.ts` around lines 26 - 29,
ACTIVE_STATUSES is using fragile index-based access (SUBSCRIPTION_STATUSES[0]
and [3]); replace those index lookups with explicit status identifiers so the
intent is clear and resilient to reordering—use the concrete SubscriptionStatus
enum members or the exact status string literals that correspond to the intended
active statuses (e.g., SubscriptionStatus.<NAME> or "status-name") when
assigning ACTIVE_STATUSES and update any related usages/tests accordingly.
| export async function getProcessedCheckoutSessionsWithoutSubscription( | ||
| db: Database, | ||
| options: QueryOptions = {}, | ||
| ): Promise<StripeEventLedgerEntry[]> { | ||
| const { limit = 50 } = options; | ||
|
|
||
| const results = await db | ||
| .select() | ||
| .from(stripeEventLedger) | ||
| .where(eq(stripeEventLedger.type, 'checkout.session.completed')) | ||
| .orderBy(stripeEventLedger.receivedAt) | ||
| .limit(limit) | ||
| .all(); | ||
|
|
||
| return results.filter(entry => entry.status === LedgerStatus.PROCESSED); | ||
| } |
There was a problem hiding this comment.
Inefficient query and potentially misleading function name.
Two issues:
-
In-memory filtering is inefficient: The function queries all
checkout.session.completedevents then filters forPROCESSEDstatus in memory. Use a compound WHERE clause instead. -
Function name doesn't match behavior: The name suggests filtering entries "without subscription", but there's no check for
stripeSubscriptionId IS NULL.
🔧 Suggested fix
+import { eq, desc, and, isNull } from 'drizzle-orm';
...
export async function getProcessedCheckoutSessionsWithoutSubscription(
db: Database,
options: QueryOptions = {},
): Promise<StripeEventLedgerEntry[]> {
const { limit = 50 } = options;
const results = await db
.select()
.from(stripeEventLedger)
- .where(eq(stripeEventLedger.type, 'checkout.session.completed'))
+ .where(
+ and(
+ eq(stripeEventLedger.type, 'checkout.session.completed'),
+ eq(stripeEventLedger.status, LedgerStatus.PROCESSED),
+ isNull(stripeEventLedger.stripeSubscriptionId)
+ )
+ )
.orderBy(stripeEventLedger.receivedAt)
.limit(limit)
.all();
- return results.filter(entry => entry.status === LedgerStatus.PROCESSED);
+ return results;
}📝 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.
| export async function getProcessedCheckoutSessionsWithoutSubscription( | |
| db: Database, | |
| options: QueryOptions = {}, | |
| ): Promise<StripeEventLedgerEntry[]> { | |
| const { limit = 50 } = options; | |
| const results = await db | |
| .select() | |
| .from(stripeEventLedger) | |
| .where(eq(stripeEventLedger.type, 'checkout.session.completed')) | |
| .orderBy(stripeEventLedger.receivedAt) | |
| .limit(limit) | |
| .all(); | |
| return results.filter(entry => entry.status === LedgerStatus.PROCESSED); | |
| } | |
| export async function getProcessedCheckoutSessionsWithoutSubscription( | |
| db: Database, | |
| options: QueryOptions = {}, | |
| ): Promise<StripeEventLedgerEntry[]> { | |
| const { limit = 50 } = options; | |
| const results = await db | |
| .select() | |
| .from(stripeEventLedger) | |
| .where( | |
| and( | |
| eq(stripeEventLedger.type, 'checkout.session.completed'), | |
| eq(stripeEventLedger.status, LedgerStatus.PROCESSED), | |
| isNull(stripeEventLedger.stripeSubscriptionId) | |
| ) | |
| ) | |
| .orderBy(stripeEventLedger.receivedAt) | |
| .limit(limit) | |
| .all(); | |
| return results; | |
| } |
🤖 Prompt for AI Agents
In `@packages/workers/src/db/stripeEventLedger.ts` around lines 266 - 281, The
function getProcessedCheckoutSessionsWithoutSubscription currently fetches all
'checkout.session.completed' rows then filters for LedgerStatus.PROCESSED in
memory and does not check for missing subscriptions; change the DB query in
getProcessedCheckoutSessionsWithoutSubscription to include both conditions in
the WHERE clause (eq(stripeEventLedger.type, 'checkout.session.completed') AND
eq(stripeEventLedger.status, LedgerStatus.PROCESSED) AND
isNull/eq(stripeEventLedger.stripeSubscriptionId, null) or the equivalent for
your query builder) so filtering happens in the database and only rows with
stripeSubscriptionId null are returned; also either rename the function to
reflect its behavior if you intend a different filter, or keep the name but
ensure the query enforces the "without subscription" condition.
| const error = createDomainError(SYSTEM_ERRORS.INTERNAL_ERROR, { | ||
| ...(process.env.ENVIRONMENT !== 'production' && { | ||
| originalError: err.message, | ||
| stack: err.stack, | ||
| }), | ||
| }); | ||
| return c.json(error, error.statusCode as 500); |
There was a problem hiding this comment.
process.env is not available in Cloudflare Workers runtime.
Cloudflare Workers don't have access to process.env. Environment variables should be accessed from the Hono context (c.env). This condition will always evaluate to false in production.
🐛 Proposed fix
Since the error handler doesn't have direct access to c.env.ENVIRONMENT, you could either:
Option 1: Check a global or use a different approach
- const error = createDomainError(SYSTEM_ERRORS.INTERNAL_ERROR, {
- ...(process.env.ENVIRONMENT !== 'production' && {
- originalError: err.message,
- stack: err.stack,
- }),
- });
+ // In Workers, we can't easily check environment in error handlers
+ // Consider always excluding sensitive info, or use a build-time flag
+ const error = createDomainError(SYSTEM_ERRORS.INTERNAL_ERROR, {});Option 2: Pass environment through a factory pattern
export function createErrorHandler(isProduction: boolean): ErrorHandler {
return (err, c) => {
// ... error handling logic
const error = createDomainError(SYSTEM_ERRORS.INTERNAL_ERROR, {
...(!isProduction && {
originalError: err.message,
stack: err.stack,
}),
});
return c.json(error, error.statusCode as 500);
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const error = createDomainError(SYSTEM_ERRORS.INTERNAL_ERROR, { | |
| ...(process.env.ENVIRONMENT !== 'production' && { | |
| originalError: err.message, | |
| stack: err.stack, | |
| }), | |
| }); | |
| return c.json(error, error.statusCode as 500); | |
| // In Workers, we can't easily check environment in error handlers | |
| // Consider always excluding sensitive info, or use a build-time flag | |
| const error = createDomainError(SYSTEM_ERRORS.INTERNAL_ERROR, {}); | |
| return c.json(error, error.statusCode as 500); |
🤖 Prompt for AI Agents
In `@packages/workers/src/middleware/errorHandler.ts` around lines 83 - 89, The
current error handler uses process.env which isn't available in Cloudflare
Workers; update the logic around createDomainError(SYSTEM_ERRORS.INTERNAL_ERROR)
to use the Hono context environment instead (c.env.ENVIRONMENT) or refactor into
a factory like createErrorHandler(isProduction: boolean) that returns the
ErrorHandler signature used by the middleware; then conditionally include
originalError and stack when not in production based on c.env.ENVIRONMENT or the
injected isProduction flag and return c.json(error, error.statusCode as 500) as
before.
| const db = createDb((c as AppContext).env.DB); | ||
| const orgBilling = (await resolveOrgAccess(db, orgId)) as OrgBilling; |
There was a problem hiding this comment.
Wrap resolveOrgAccess in try-catch for resilience.
Per coding guidelines, database operations should be wrapped in try-catch blocks and return domain errors using createDomainError with SYSTEM_ERRORS.DB_ERROR. If resolveOrgAccess throws, this middleware will cause an unhandled exception.
🔧 Proposed fix
+import { SYSTEM_ERRORS } from '@corates/shared';
+
const db = createDb((c as AppContext).env.DB);
- const orgBilling = (await resolveOrgAccess(db, orgId)) as OrgBilling;
+ let orgBilling: OrgBilling;
+ try {
+ orgBilling = (await resolveOrgAccess(db, orgId)) as OrgBilling;
+ } catch (err) {
+ const error = createDomainError(SYSTEM_ERRORS.DB_ERROR, {
+ operation: 'resolve_org_access',
+ orgId,
+ originalError: err instanceof Error ? err.message : String(err),
+ });
+ return c.json(error, error.statusCode as 500);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const db = createDb((c as AppContext).env.DB); | |
| const orgBilling = (await resolveOrgAccess(db, orgId)) as OrgBilling; | |
| const db = createDb((c as AppContext).env.DB); | |
| let orgBilling: OrgBilling; | |
| try { | |
| orgBilling = (await resolveOrgAccess(db, orgId)) as OrgBilling; | |
| } catch (err) { | |
| const error = createDomainError(SYSTEM_ERRORS.DB_ERROR, { | |
| operation: 'resolve_org_access', | |
| orgId, | |
| originalError: err instanceof Error ? err.message : String(err), | |
| }); | |
| return c.json(error, error.statusCode as 500); | |
| } |
| const db = createDb((c as AppContext).env.DB); | ||
| const orgBilling = (await resolveOrgAccess(db, orgId)) as OrgBilling; |
There was a problem hiding this comment.
Wrap database operation in try-catch.
The call to resolveOrgAccess can throw on database errors. As per coding guidelines, database operations should be wrapped in try-catch and return domain errors using SYSTEM_ERRORS.DB_ERROR.
Proposed fix
+import { createDomainError, AUTH_ERRORS, SYSTEM_ERRORS } from '@corates/shared';
-import { createDomainError, AUTH_ERRORS } from '@corates/shared'; const db = createDb((c as AppContext).env.DB);
- const orgBilling = (await resolveOrgAccess(db, orgId)) as OrgBilling;
+ let orgBilling: OrgBilling;
+ try {
+ orgBilling = await resolveOrgAccess(db, orgId);
+ } catch (err) {
+ const error = createDomainError(SYSTEM_ERRORS.DB_ERROR, {
+ reason: 'billing_resolution_failed',
+ });
+ return c.json(error, error.statusCode as 500);
+ }🤖 Prompt for AI Agents
In `@packages/workers/src/middleware/requireOrgWriteAccess.ts` around lines 23 -
24, The call to resolveOrgAccess (using createDb((c as AppContext).env.DB)) can
throw on DB failures; wrap the database operation in a try-catch inside the
requireOrgWriteAccess middleware, catching errors from resolveOrgAccess, and
return the domain error using SYSTEM_ERRORS.DB_ERROR (include the caught error
details in the returned/logged payload) instead of letting the exception
propagate; keep references to createDb, AppContext, resolveOrgAccess, orgBilling
and SYSTEM_ERRORS.DB_ERROR to locate where to make the change.
| if (!user) { | ||
| const error = createDomainError(AUTH_ERRORS.REQUIRED, { reason: 'no_user' }); | ||
| return c.json(error, error.statusCode as ContentfulStatusCode); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the requireAuth middleware
cat packages/workers/src/middleware/auth.tsRepository: InfinityBowman/corates
Length of output: 1597
🏁 Script executed:
# Also check the avatars.ts file around the lines mentioned
sed -n '40,50p' packages/workers/src/routes/avatars.ts
sed -n '170,185p' packages/workers/src/routes/avatars.ts
sed -n '400,415p' packages/workers/src/routes/avatars.tsRepository: InfinityBowman/corates
Length of output: 1500
🏁 Script executed:
# Look for type definitions of getAuth and requireAuth
rg -n "getAuth|requireAuth" packages/workers/src/middleware/auth.ts -B 2 -A 5 --type=tsRepository: InfinityBowman/corates
Length of output: 534
Remove redundant auth checks after requireAuth middleware.
The requireAuth middleware guarantees that route handlers are only reached when a valid user exists—it returns a 401 error if session.user is missing and never calls next(). Since the middleware sets c.set('user', session.user) before proceeding, the if (!user) checks at lines 175-178 and 404-407 are dead code and can be removed. The subsequent getAuth calls should always return a defined user.
Also applies to: 404-407
🤖 Prompt for AI Agents
In `@packages/workers/src/routes/avatars.ts` around lines 175 - 178, Remove the
redundant null/undefined checks that follow the requireAuth middleware: delete
the `if (!user) { const error = createDomainError(AUTH_ERRORS.REQUIRED, {
reason: 'no_user' }); return c.json(error, error.statusCode as
ContentfulStatusCode); }` blocks (the ones after retrieving `user` at both
locations) because `requireAuth` guarantees `session.user` and sets
`c.set('user', session.user)` so handlers always have a defined user; after
removing them, update any subsequent usage to treat `user` as non-null (use the
existing `getAuth`/`c.get('user')` return as defined or add a non-null
assertion) and run tests to ensure no other code relies on those early returns.
| try { | ||
| const appUrl = c.env.APP_URL || 'https://corates.org'; | ||
| const basepath = c.env.BASEPATH || ''; | ||
| const basepath = (c.env as unknown as Record<string, string | undefined>).BASEPATH ?? ''; |
There was a problem hiding this comment.
BASEPATH access uses unsafe type assertion.
The cast (c.env as unknown as Record<string, string | undefined>).BASEPATH bypasses type safety. If BASEPATH is a valid environment variable, it should be added to the Env type definition.
💡 Add BASEPATH to Env type or use optional chaining
If BASEPATH is a valid env var, add it to the Env interface in packages/workers/src/types/env.ts:
interface Env {
// ... existing properties
BASEPATH?: string;
}Then simplify the access:
-const basepath = (c.env as unknown as Record<string, string | undefined>).BASEPATH ?? '';
+const basepath = c.env.BASEPATH ?? '';🤖 Prompt for AI Agents
In `@packages/workers/src/routes/members.ts` around lines 501 - 503, The code
unsafely asserts c.env to access BASEPATH; add BASEPATH?: string to the Env
interface (packages/workers/src/types/env.ts) so BASEPATH can be accessed
type-safely, then update the access in members.ts to use c.env.BASEPATH ?? ''
(or use optional chaining like c.env?.BASEPATH ?? '') instead of the (c.env as
unknown as Record<...>).BASEPATH cast; ensure imports/types align so TypeScript
recognizes the new Env field.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.