feat(fastify): authentication #43
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds Better Auth integration (magic-link + Web3 scaffold), DB schemas and token encryption, Fastify auth plugin/routes, Resend-backed email templates/service and notif package, per-worker PGLite test/migration flow, env/CI updates, and multiple dependency/tooling changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Fastify
participant AuthPlugin
participant BetterAuth
participant Database
participant ResendAPI
Client->>Fastify: POST /api/auth/sign-in/magic-link { email }
Fastify->>AuthPlugin: proxy request under /api/auth/*
AuthPlugin->>BetterAuth: handler.process(request)
BetterAuth->>Database: create verification token / session
BetterAuth->>ResendAPI: send magic link email
ResendAPI-->>Client: email delivered
BetterAuth-->>AuthPlugin: handler response
AuthPlugin-->>Fastify: forward response
Fastify-->>Client: HTTP 200
sequenceDiagram
participant TestRunner
participant VitestGlobalSetup
participant Worker
participant PGLiteInstance
participant MigrationFiles
TestRunner->>VitestGlobalSetup: start suites
VitestGlobalSetup->>Worker: set test env vars
Worker->>PGLiteInstance: create per-worker DB dir & instance
Worker->>MigrationFiles: read .sql migration files
Worker->>PGLiteInstance: execute SQL migrations
Worker-->>TestRunner: run tests against migrated DB
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/fastify/package.json (1)
38-67: Review and address known security advisories in better-auth v1.4.17 before deployment.The new dependencies introduce known security concerns. Better Auth v1.4.17 has multiple recent security advisories (including open-redirects, auth bypasses, and critical CVEs such as unauthenticated API key creation). Before deploying, audit the active CVE list, verify that the chosen version is patched for vulnerabilities affecting your use case, and test thoroughly in staging. The maintainers state they support only the latest release, so plan to keep this dependency updated.
SIWE v3.0.0 is stable and has no reported vulnerabilities, but it contains breaking API changes (validate() removed in favor of verify(), removed uri-js and valid-url dependencies). Verify the codebase is compatible with these changes.
apps/fastify/vitest.setup.ts (1)
23-49: Static imports execute before env overrides—but vitest.config.ts already mitigates this.The technical concern is valid: static imports at lines 45-49 are hoisted and evaluated before the env assignments at lines 23-40. However, vitest.config.ts already sets NODE_ENV, DATABASE_URL, and ENCRYPTION_KEY (lines 13-24) before setupFiles runs, which satisfies those dependencies.
The remaining required vars (OPENAI_API_KEY, BETTER_AUTH_SECRET, RESEND_API_KEY, EMAIL_FROM) are provided by GitHub Actions secrets in CI. Locally, the code would fail without those secrets because env.ts validation throws synchronously when the required vars are missing—making the fallbacks in vitest.setup.ts unreachable.
The fragility remains real: if a future env var becomes required without being added to CI secrets or vitest.config.ts defaults, tests would break locally. Moving the env-sensitive imports into beforeAll/afterAll (as proposed) would eliminate this risk and make the setup more robust. Alternatively, ensure all required test env vars are set in vitest.config.ts itself before setupFiles loads.
🤖 Fix all issues with AI agents
In `@apps/fastify/scripts/migrate.ts`:
- Around line 74-76: Replace the local console-based logger wrapper with the
shared logger by removing the local logger definition and adding an import for
logger from '@repo/utils/logger'; then update all current uses (the local
logger.info/logger.error calls in migrate.ts, including occurrences around the
checks and migration steps) to use the new logger interface (e.g.,
logger.info(msg) or logger.info({ context: 'migrate' }, msg) and similarly for
logger.error/debug). Ensure you reference and replace every call sites
originally using the local wrapper (all instances previously named logger) so
they call the imported logger with optional context where appropriate.
In `@apps/fastify/src/db/account.ts`:
- Around line 21-27: The loop over ENCRYPTED_FIELDS currently leaves the
original plaintext when encrypt() returns null; update the logic in the block
that iterates ENCRYPTED_FIELDS (the encrypted variable/encryptedValue handling)
to not fall back to plaintext: if encrypt(value) returns null, either throw a
clear error (e.g., "encryption failed for field <field>") so callers can fail
fast, or explicitly set encrypted[field] = null and surface that to callers—do
not leave the original value in place. Ensure checks reference encrypt(),
encrypted, encryptedValue and ENCRYPTED_FIELDS so the change is applied to the
correct loop.
In `@apps/fastify/src/lib/auth-helpers.ts`:
- Around line 3-8: The requireAuth helper throws a generic Error which won't
produce a proper HTTP 401 response; update requireAuth (the exported function
requireAuth that accepts a FastifyRequest and checks request.session) to throw a
structured HTTP Unauthorized error instead of new Error('Unauthorized')—either
use the http error class from `@repo/error/node` or, if `@fastify/sensible` is
registered, return/request.server.httpErrors.unauthorized() so Fastify responds
with status 401 and correct payload.
In `@apps/fastify/src/lib/auth-plugins/web3.ts`:
- Around line 28-31: The generated nonce (nonce from crypto.randomUUID() in
web3.ts) is currently returned without persistence — store each nonce
server-side with a TTL (e.g., 5 minutes) to prevent replay attacks; use a Redis
SET with an expiration (key namespace like "web3:nonce:{nonce}", value could be
the expected wallet address or a boolean) when creating the nonce, and on
signature verification (the verify/consume function that checks the signature)
atomically check that the nonce exists and then delete it (or use GETDEL) to
enforce one-time use; ensure error handling if the nonce is missing/expired and
add tests to cover create→verify→consume flows.
In `@apps/next/package.json`:
- Around line 35-43: Update the `@types/node` dependency in package.json from the
Node 25 line ("@types/node": "^25.0.10") to a Node 22-compatible range (e.g.
"@types/node": "^22.x"); locate the dependency entry in apps/next/package.json
and change the version string so the TypeScript types match the runtime Node.js
22 specified in the root engines.node.
In `@packages/notif/src/services/email-service.ts`:
- Around line 122-125: The sendBulk function currently runs
Promise.all(emails.map(email => buildEmailPayload({ email }))) outside any
try/catch so a single buildEmailPayload failure can reject the whole call and
violate the SendBulkResult contract; change the build step to produce a
consistent payload result for each email (e.g., use Promise.allSettled or wrap
each buildEmailPayload call in a try/catch) so failures are captured, logged,
and translated into skipped/failed entries rather than throwing, then continue
to the sending logic using the normalized results (reference sendBulk and
buildEmailPayload to locate the change and ensure returned objects conform to
SendBulkResult).
🟡 Minor comments (17)
packages/notif/README.md-14-18 (1)
14-18: Add a server-only warning for the API key example.
Consider explicitly noting thatNotificationServiceshould be initialized on the server to avoid leakingRESEND_API_KEYinto client bundles.✍️ Doc tweak
const service = new NotificationService({ email: { apiKey: process.env.RESEND_API_KEY, }, }) +// Note: initialize on the server only; never expose RESEND_API_KEY to client bundles.apps/fastify/src/routes/wallet.ts-9-20 (1)
9-20: Add schema definition for GET /wallets route.The GET endpoint is missing a schema definition, which means it won't be properly documented in the OpenAPI spec generated by
@fastify/swagger. Per coding guidelines, ensure OpenAPI schema generation is correct.Proposed fix
// List user's wallets - fastify.get('/wallets', async request => { + fastify.get( + '/wallets', + { + schema: { + operationId: 'listWallets', + description: 'List all wallets linked to the current user', + summary: 'List wallet identities', + tags: ['wallet'], + response: { + 200: Type.Object({ + wallets: Type.Array( + Type.Object({ + id: Type.String(), + userId: Type.String(), + chain: Type.Union([Type.Literal('eip155'), Type.Literal('solana')]), + address: Type.String(), + walletProvider: Type.Union([Type.String(), Type.Null()]), + createdAt: Type.String({ format: 'date-time' }), + lastUsedAt: Type.String({ format: 'date-time' }), + }), + ), + }), + }, + }, + }, + async request => { const { user } = requireAuth(request) const db = await getDb() const wallets = await db .select() .from(walletIdentities) .where(eq(walletIdentities.userId, user.id)) return { wallets } - }) + }, + )packages/email/package.json-6-18 (1)
6-18: Remove misleading"require"export fields; source exports are appropriate for workspace packages.The
"require"fields in the exports map are incorrect. The package declares"type": "module"(ESM-only) and exports TypeScript sources, which cannot be consumed via CommonJSrequire(). Since this is a private workspace package, exporting source files is acceptable (matching the pattern inpackages/ui), but the"require"entries should be removed to avoid false signaling of CJS compatibility.Corrected export shape
"exports": { "./emails/*": { "types": "./emails/*.tsx", - "import": "./emails/*.tsx", - "require": "./emails/*.tsx" + "import": "./emails/*.tsx" }, "./render": { "types": "./render.ts", "node": { - "import": "./render.ts", - "require": "./render.ts" + "import": "./render.ts" } } }apps/fastify/vitest.setup.ts-76-82 (1)
76-82: GuaranteeresetDbInstance()runs even if close fails.If
closeTestDatabase()throws, the in‑memory instance isn’t cleared and can leak across suites. Wrap cleanup intry/finally.🛠️ Proposed fix
afterAll(async () => { // Delete database instance after all tests in suite complete // This ensures clean state for next test suite and handles test failures // Even if tests fail, the instance is cleaned up here - await closeTestDatabase() - resetDbInstance() + try { + await closeTestDatabase() + } finally { + resetDbInstance() + } })apps/fastify/test/routes/web3-auth.spec.ts-10-12 (1)
10-12: Skipped suite leaves Web3 auth endpoints untested.
describe.skipdisables all assertions; please enable when endpoints are ready or gate via a feature flag with a tracking issue.🛠️ Proposed fix
-describe.skip('Web3 Authentication', () => { +describe('Web3 Authentication', () => {apps/fastify/test/routes/auth.spec.ts-21-28 (1)
21-28: Assertion allows 404, so the test doesn’t validate route mounting.The expectation currently passes on 404. If you want to confirm the route is mounted, explicitly assert it’s not 404.
🐛 Suggested fix
- expect(response.statusCode).toBeLessThan(500) + expect(response.statusCode).not.toBe(404) + expect(response.statusCode).toBeLessThan(500)apps/fastify/src/routes/auth/README.md-13-17 (1)
13-17: Clarify that password endpoints are disabled by default (if that’s the current config).Given the tests expect
/api/auth/sign-up/emailand/api/auth/sign-in/emailto be rejected, this section should note the endpoints are disabled unless password auth is enabled.📝 Suggested wording
-### Email Authentication - -- `POST /api/auth/sign-up/email` - Register new user with email/password -- `POST /api/auth/sign-in/email` - Sign in with email/password +### Email Authentication + +- `POST /api/auth/sign-up/email` - Register new user with email/password (disabled unless password auth is enabled) +- `POST /api/auth/sign-in/email` - Sign in with email/password (disabled unless password auth is enabled) + - When disabled, these endpoints return 4xx.apps/fastify/test/routes/auth.spec.ts-48-55 (1)
48-55: Test name doesn’t match the assertion.The test only checks
200and doesn’t verify session middleware behavior. Either assert middleware effects or rename the test.✏️ Suggested rename
- it('should attach session middleware to requests', async () => { + it('should keep the health endpoint reachable with auth enabled', async () => {packages/email/components/logo.tsx-3-3 (1)
3-3: Guard against missingEMAIL_ASSETS_URLto avoid broken logo URLs.When
baseUrlis empty, the image source becomes relative, which most email clients won’t resolve. Consider a fallback (e.g.,APP_URL) or skip rendering when unset.💡 Proposed fix
-const baseUrl = process.env.EMAIL_ASSETS_URL || '' +const baseUrl = process.env.EMAIL_ASSETS_URL ?? process.env.APP_URL @@ - <Img - src={`${baseUrl}/email/logo.png`} - width="40" - height="40" - alt="Logo" - className="my-0 mx-auto block logo-blend" - /> + {baseUrl ? ( + <Img + src={`${baseUrl}/email/logo.png`} + width="40" + height="40" + alt="Logo" + className="my-0 mx-auto block logo-blend" + /> + ) : null}Also applies to: 35-40
apps/fastify/src/lib/auth.ts-49-56 (1)
49-56: Error metadata marks magic link as sent even on failure.
The catch block logsmagicLinkSent: true, which is misleading for monitoring.🐛 Proposed fix
- magicLinkSent: true, + magicLinkSent: false,packages/email/emails/login-notification.tsx-33-41 (1)
33-41: Guard invalid timestamps to avoid rendering “Invalid Date”.
Date#toLocaleStringreturns “Invalid Date” without throwing; a validity check gives a cleaner fallback.🐛 Proposed fix
- const formatDate = (dateString: string) => { - try { - return new Date(dateString).toLocaleString(undefined, { - dateStyle: 'long', - timeStyle: 'short', - }) - } catch { - return dateString - } - } + const formatDate = (dateString: string) => { + const date = new Date(dateString) + if (Number.isNaN(date.getTime())) { + return dateString + } + return date.toLocaleString(undefined, { + dateStyle: 'long', + timeStyle: 'short', + }) + }packages/notif/src/types/transactions-created.ts-9-23 (1)
9-23: Date range assumes transactions are pre-sorted.
If input order isn’t guaranteed,from/tocan invert; consider sorting or min/max selection.🐛 Possible fix (sort by date)
- const firstTransaction = data.transactions[0] - const lastTransaction = data.transactions[data.transactions.length - 1] + const sortedByDate = [...data.transactions].sort( + (a, b) => new Date(a.date).getTime() - new Date(b.date).getTime(), + ) + const firstTransaction = sortedByDate[0] + const lastTransaction = sortedByDate[sortedByDate.length - 1]apps/docu/content/docs/architecture/authentication.mdx-12-16 (1)
12-16: "No SaaS dependencies" contradicts Resend usage for magic link emails.The authentication system uses Resend (a SaaS provider) to deliver magic link emails, making the claim of "no SaaS dependencies" misleading. Clarify that core authentication is self-hosted while email delivery is handled by Resend.
Suggested fix
-- **No vendor lock-in**: Self-hosted, no SaaS dependencies +- **No auth vendor lock-in**: Core auth is self-hosted; email delivery uses Resend by defaultpackages/notif/src/schemas.ts-10-10 (1)
10-10: Incorrectz.record()usage.
z.record()expects the first argument to be the key schema and the second to be the value schema. Usingz.any()for both works butz.record(z.string(), z.any())is more correct since JSON object keys are always strings.🔧 Suggested fix
- metadata: z.record(z.any(), z.any()), // Flexible - any JSON object + metadata: z.record(z.string(), z.unknown()), // Flexible - any JSON objectUsing
z.unknown()instead ofz.any()is also slightly safer as it requires type narrowing when accessed.apps/fastify/src/plugins/auth.ts-47-51 (1)
47-51: Security concern: Trust boundary for protocol and host headers.The code trusts
x-forwarded-protoandhostheaders directly from the request. In production behind a reverse proxy, these headers should be validated or the proxy should be configured to set them correctly. Malicious actors could potentially manipulate these headers if the server is directly exposed.Consider adding validation or documenting that this service must run behind a trusted proxy:
🔒 Suggested improvement
+ // Note: This assumes the service runs behind a trusted proxy that sets these headers correctly + // In production, ensure your proxy strips/overwrites x-forwarded-proto and host headers const host = request.headers.host || `localhost:${env.PORT}` - const protocol = request.headers['x-forwarded-proto'] || 'http' + const protocol = (request.headers['x-forwarded-proto'] as string)?.split(',')[0]?.trim() || 'http'packages/notif/src/base.ts-11-20 (1)
11-20: AligncreateActivityoptionality with usage.
NotificationHandler.createActivityis required in the interface definition, butindex.ts(line 92) guards it withif (handler.createActivity), treating it as optional. All current handler implementations provide it, making the check unnecessary. Make the field optional in the interface to match the usage pattern.Suggested fix
export interface NotificationHandler<T = unknown> { schema: z.ZodSchema<T> email?: { template: string subject: string from?: string replyTo?: string } - createActivity: (data: T, user: UserData) => CreateActivityInput + createActivity?: (data: T, user: UserData) => CreateActivityInput createEmail?: (packages/notif/src/index.ts-149-176 (1)
149-176: Customer branch only emails the first user—align with multi-user pattern.The
emailType === 'customer'branch sends only tofirstUser, while'team'sends to all users and'owners'filters and maps over multiple users. If multiple customers can be passed viavalidatedData.users, the customer branch should follow the same pattern. Either restrict the schema to a single customer user or map over all users like the other branches.💡 Suggested fix (send to all users)
- const emailInputs = [ - createEmailInput({ type, handler, validatedData, user: firstUser, teamContext, options }), - ] + const emailInputs = validatedData.users.map((user: UserData) => + createEmailInput({ type, handler, validatedData, user, teamContext, options }), + )
🧹 Nitpick comments (28)
packages/utils/src/web3/index.ts (1)
146-169: Redundant Solana cluster lookup.Since Solana clusters are already added to
CHAIN_REGISTRYat lines 131-133 with keys like'mainnet-beta','devnet', and'testnet', the directSOLANA_CLUSTERS[chainId]lookup on lines 153-154 will never find anything that wasn't already found byCHAIN_REGISTRY.get(chainId)on line 149.♻️ Suggested simplification
export function getChainMetadata(chainId: number | string): ChainMetadata | undefined { // Try as string chain ID if (isString(chainId)) { const byChainId = CHAIN_REGISTRY.get(chainId) if (byChainId) return byChainId - - // Try as Solana cluster name - const bySolanaCluster = SOLANA_CLUSTERS[chainId] - if (bySolanaCluster) return bySolanaCluster } // Try as numeric chain ID (EVM)apps/fastify/src/db/index.ts (1)
46-47: Add an explicit return type for the exported function.♻️ Suggested change
-export function resetDbInstance() { +export function resetDbInstance(): void { db = null // Note: pgLiteInstance is managed by test utils, don't reset it here }As per coding guidelines, add explicit return types for exported functions.
apps/fastify/src/lib/auth-helpers.ts (1)
10-10: Add explicit return type for exported function.Per coding guidelines, exported functions should have explicit return types for better type safety and documentation.
-export const getOptionalAuth = (request: FastifyRequest) => request.session +export const getOptionalAuth = (request: FastifyRequest): typeof request.session => request.sessionapps/fastify/src/db/schema/tables/wallet-identities.ts (1)
11-12: Consider address normalization for EVM wallets.EVM addresses are case-insensitive but can be stored in different formats (checksummed vs lowercase). If addresses are stored inconsistently, the unique constraint might not catch duplicates (e.g.,
0xABC...vs0xabc...).Consider normalizing addresses to lowercase before storage, or document the expected format in the application layer.
apps/fastify/src/routes/wallet.ts (1)
46-49: Remove unnecessary type assertion - use TypeBox inference.With
@fastify/type-provider-typebox, route params are automatically typed from the schema. The manual type assertion bypasses this safety and duplicates the type definition.Proposed fix
async (request, reply) => { const { user } = requireAuth(request) - const params = request.params as { chain: 'eip155' | 'solana'; address: string } - const { chain, address } = params + const { chain, address } = request.params const db = await getDb()For this to work, ensure the Fastify instance uses the TypeBox type provider. If type inference isn't working, the plugin registration may need adjustment.
packages/email/render.ts (1)
8-9: Inline the single-line async arrow for style consistency.As per coding guidelines, avoid unnecessary braces for single-line arrow functions.
♻️ Proposed refactor
-export const render = async (component: ReactNode): Promise<string> => { - return reactEmailRender(component) -} +export const render = async (component: ReactNode): Promise<string> => + reactEmailRender(component)apps/fastify/src/db/schema/tables/users.ts (1)
14-14: Index on email is redundant with the unique constraint.PostgreSQL automatically creates an index for unique constraints. The explicit
index('users_email_idx').on(table.email)is redundant and will result in a duplicate index.♻️ Remove redundant index
export const users = pgTable( 'users', { id: text('id').primaryKey(), email: varchar('email', { length: 255 }).unique(), emailVerified: boolean('email_verified').default(false).notNull(), name: text('name'), image: text('image'), createdAt: timestamp('created_at').defaultNow().notNull(), updatedAt: timestamp('updated_at').defaultNow().notNull(), }, - table => [index('users_email_idx').on(table.email)], )packages/notif/src/services/email-service.ts (1)
185-194: Add a RORO‑friendly overload to avoid positional params.The exported class method uses two positional args, which conflicts with the RORO guideline. Consider an overload that accepts
SendBulkInputwhile keeping the old signature for compatibility.🛠️ Proposed refactor
export class EmailService { `#service`: ReturnType<typeof createEmailService> constructor() { this.#service = createEmailService() } - async sendBulk(emails: EmailInput[], notificationType: string): Promise<SendBulkResult> { - return this.#service.sendBulk({ emails, notificationType }) - } + async sendBulk(input: SendBulkInput): Promise<SendBulkResult> + /** `@deprecated` Prefer sendBulk({ emails, notificationType }) */ + async sendBulk(emails: EmailInput[], notificationType: string): Promise<SendBulkResult> + async sendBulk( + emailsOrInput: EmailInput[] | SendBulkInput, + notificationType?: string, + ): Promise<SendBulkResult> { + if (Array.isArray(emailsOrInput)) { + if (!notificationType) throw new Error('notificationType is required') + return this.#service.sendBulk({ emails: emailsOrInput, notificationType }) + } + return this.#service.sendBulk(emailsOrInput) + } }packages/email/components/footer.tsx (1)
4-35: Add an explicit return type to the exportedFooter.Exported functions should declare return types to keep the public surface stable.
♻️ Suggested change
-export function Footer() { +export function Footer(): JSX.Element {As per coding guidelines, exported functions should have explicit return types.
apps/fastify/src/db/schema/tables/verification.ts (1)
6-15: Index the verification token value for lookup performance.Verification lookups commonly query by
value; without an index this can degrade as the table grows. Consider adding an index (or unique index if values must be unique).♻️ Proposed fix
table => [ + index('verification_value_idx').on(table.value), index('verification_identifier_idx').on(table.identifier), index('verification_expires_at_idx').on(table.expiresAt), ],packages/email/components/button.tsx (1)
12-12: Add an explicit return type forButton.Exported functions should declare return types;
React.ReactElementis a clear, React‑19‑friendly choice.♻️ Proposed fix
-export function Button({ href, children, variant = 'primary', className = '' }: ButtonProps) { +export function Button({ + href, + children, + variant = 'primary', + className = '', +}: ButtonProps): React.ReactElement {packages/email/emails/welcome.tsx (1)
10-12: Declare an explicit return type forWelcomeEmail.This keeps exported component signatures explicit and aligns with the TS/React guidelines.
♻️ Proposed fix
import { Body, Container, Heading, Preview, Text } from '@react-email/components' +import type React from 'react' import { Footer } from '../components/footer' import { Logo } from '../components/logo' import { EmailThemeProvider, getEmailInlineStyles, getEmailThemeClasses } from '../components/theme' @@ -export const WelcomeEmail = ({ fullName = '' }: Props) => { +export const WelcomeEmail = ({ fullName = '' }: Props): React.ReactElement => {packages/email/components/logo.tsx (1)
5-5: Add an explicit return type forLogo.♻️ Proposed fix
import { Img, Section } from '@react-email/components' +import type React from 'react' @@ -export function Logo() { +export function Logo(): React.ReactElement {packages/email/emails/magic-link-login.tsx (1)
17-21: Add an explicit return type forMagicLinkLoginEmail.♻️ Proposed fix
import { Body, Container, Heading, Preview, Section, Text } from '@react-email/components' +import type React from 'react' @@ export const MagicLinkLoginEmail = ({ magicLink, expirationMinutes = 15, fullName = '', -}: Props) => { +}: Props): React.ReactElement => {packages/notif/src/types/login-notification.ts (1)
8-37: Consider RORO-style params for handler hooks.
If feasible, refactor the NotificationHandler signatures to accept a single params object to avoid ordering mistakes. As per coding guidelines, prefer RORO for multi-parameter functions.packages/email/emails/login-notification.tsx (1)
1-27: Add an explicit return type for the exported component.
This keeps the public API precise. As per coding guidelines, exported functions should declare return types.♻️ Proposed change
+import type { ReactElement } from 'react' import { Body, Container, Heading, Preview, Section, Text } from '@react-email/components' import { Button } from '../components/button' import { Footer } from '../components/footer' import { Logo } from '../components/logo' import { EmailThemeProvider, getEmailInlineStyles, getEmailThemeClasses } from '../components/theme' @@ export const LoginNotificationEmail = ({ timestamp, ipAddress, location, device, userAgent: _userAgent, fullName = '', secureAccountUrl, thisWasMeUrl, -}: Props) => { +}: Props): ReactElement => {packages/notif/src/types/transactions-created.ts (1)
8-8: Consider RORO-style params for handler hooks.
If feasible, refactor the NotificationHandler signatures to accept a single params object to avoid ordering mistakes. As per coding guidelines, prefer RORO for multi-parameter functions.Also applies to: 39-39
apps/fastify/src/lib/auth.ts (1)
17-19: Add an explicit return type forgetAuth().
Keeps the exported API precise. As per coding guidelines, exported functions should declare return types.♻️ Proposed change
-export async function getAuth() { +export async function getAuth(): Promise<ReturnType<typeof betterAuth>> {packages/email/emails/transactions.tsx (2)
40-147: Consider extracting default sample data to a separate file.The
defaultTransactionsconstant spans over 100 lines and is primarily used for preview/development purposes. Extracting it to a separate fixture file would improve maintainability and help keep this file under the 300-line limit specified in the coding guidelines.♻️ Suggested refactor
Create a new file
packages/email/emails/fixtures/transactions.ts:export const defaultTransactions = [ // ... move the array here ]Then import it:
+import { defaultTransactions } from './fixtures/transactions' -const defaultTransactions = [ - // ... 100+ lines -]
317-336: URL may contain "undefined" if transaction dates are missing.The condition checks if
transactions[transactions.length - 1]?.dateandtransactions.at(0)?.dateexist, but falsy empty strings would pass the truthiness check while potentially being invalid. More importantly, if the dates are empty strings (which would be truthy if they weren't), the URL would be malformed.Consider validating that dates are actually valid ISO strings before constructing the URL:
♻️ Suggested improvement
- {transactions.length > 0 && - transactions[transactions.length - 1]?.date && - transactions.at(0)?.date ? ( + {transactions.length > 0 && + isValid(parseISO(transactions[transactions.length - 1]?.date ?? '')) && + isValid(parseISO(transactions.at(0)?.date ?? '')) ? (apps/fastify/src/lib/crypto.ts (1)
18-20: Consider caching the encryption key buffer.
getEncryptionKey()is called on everyencrypt()anddecrypt()operation, which creates a newBuffer.from()each time. While the env access is likely cached, the buffer allocation is not. For high-throughput scenarios, consider caching the derived key:♻️ Suggested optimization
+let cachedKey: Buffer | null = null + function getEncryptionKey(): Buffer { + if (cachedKey) return cachedKey + cachedKey = Buffer.from(env.ENCRYPTION_KEY, 'hex') - return Buffer.from(env.ENCRYPTION_KEY, 'hex') + return cachedKey }apps/fastify/src/plugins/auth.ts (1)
37-42: Performance concern: Session validation runs on every request.The
onRequesthook callsauth.api.getSession()for every incoming request, including static assets, health checks, and routes that don't require authentication. This could add unnecessary latency.Consider either:
- Making session population lazy (computed on first access)
- Using a route-level hook only for protected routes
♻️ Suggested lazy session approach
fastify.addHook('onRequest', async request => { - const session = await auth.api.getSession({ - headers: request.headers, - }) - request.session = session + // Lazy session loading - only fetch when accessed + let _session: typeof request.session + Object.defineProperty(request, 'session', { + get: async function() { + if (_session === undefined) { + _session = await auth.api.getSession({ headers: request.headers }) + } + return _session + }, + configurable: true, + }) })Note: This requires updating the session type to handle the async getter pattern.
apps/fastify/test/crypto.spec.ts (2)
8-13: Consider adding a negative test for invalid encryption key.The test only verifies that
validateEncryptionKey()doesn't throw when the key is valid. Adding tests for invalid key scenarios would improve coverage, though this may require mocking the environment.💡 Suggested additional test
// This would require environment mocking capability it('should throw when ENCRYPTION_KEY is invalid', () => { // Mock env.ENCRYPTION_KEY to be invalid (e.g., wrong length) // expect(() => validateEncryptionKey()).toThrow('must be a 64-character hex string') })
246-301: Database integration test is well-structured with proper cleanup.Good practices observed:
- Creates required foreign key (user) before account
- Verifies encryption in database
- Verifies decryption after retrieval
- Cleans up test data in correct order (account before user due to FK)
However, consider wrapping in a transaction for atomic cleanup in case of test failure:
♻️ Suggested improvement for cleanup reliability
+ try { // ... test assertions + } finally { // Cleanup await db.delete(account).where(eq(account.id, 'test-account-crypto')) await db.delete(users).where(eq(users.id, 'test-user-crypto')) + }packages/notif/src/schemas.ts (2)
57-57: DuplicateUserDatatype definition.
UserDatais also defined as an interface inpackages/notif/src/base.ts(lines 30-37). Having two definitions with the same name could lead to inconsistencies if one is updated without the other.Consider either:
- Removing the interface from
base.tsand using only the Zod-inferred type- Deriving the Zod schema from the interface using
z.object()pattern♻️ Option 1: Single source of truth from schema
In
base.ts, import and re-export from schemas:export type { UserData } from './schemas'
43-50: Consider using stricter date validation.The
timestampfield usesz.string()but represents an ISO date string. Usingz.string().datetime()would provide automatic validation of the ISO 8601 format.♻️ Suggested improvement
export const loginNotificationSchema = z.object({ users: z.array(userSchema), - timestamp: z.string(), // ISO date string + timestamp: z.string().datetime(), // ISO 8601 date string with validation ipAddress: z.string(), location: z.string().optional(), // e.g., "San Francisco, CA" device: z.string().optional(), // e.g., "Chrome on Windows" userAgent: z.string().optional(), })packages/email/components/theme.tsx (1)
186-206: Font CDN dependency with proper fallback.The fonts load from
cdn.jsdelivr.net(both URLs confirmed accessible with HTTP 200 responses). The fallback to Helvetica ensures graceful degradation if the CDN is unavailable. If reliability is a concern, consider self-hosting the fonts or documenting the fallback behavior in comments.packages/notif/src/index.ts (1)
221-234: Consider deprecating theNotificationsclass or removing it if not used externally—the preferredcreateNotifications()function already follows RORO pattern.The class wrapper has zero usages in the codebase and is marked as backward compatibility. Since the preferred functional API already aligns with RORO guidelines, refactoring an unused class adds unnecessary complexity. If this is truly for external backward compatibility on a private package, consider deprecating or removing it instead of adding overloads.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/fastify/src/db/migrate.ts (1)
14-20: Don't silently treat all read errors as "no migrations".Catching every error in
readMigrationFilescan mask permission/I/O failures and skip migrations silently. Only ignore ENOENT and rethrow the rest.🐛 Proposed fix
- } catch { - // Migrations directory doesn't exist yet - return [] - } + } catch (err) { + const error = err as NodeJS.ErrnoException + if (error.code === 'ENOENT') { + // Migrations directory doesn't exist yet + return [] + } + throw err + }
🤖 Fix all issues with AI agents
In `@apps/fastify/README.md`:
- Around line 51-65: Update the README section that currently claims a single
shared in-memory PGLite DB: change the lifecycle to state that tests create
per-worker file-based databases and that migrations are run in vitest.setup.ts
(not in vitest.global-setup.ts), explain that each worker gets its own DB file
(so state is isolated per-worker rather than shared across all test files), and
add guidance about test isolation or explicit cleanup if you need cross-worker
persistence; reference vitest.setup.ts and vitest.global-setup.ts so readers
know where the behavior is implemented.
In `@apps/fastify/src/lib/auth.ts`:
- Around line 56-59: In the telemetry/error handler where the failed magic-link
email send is logged, change the telemetry payload so the magicLinkSent field
accurately reflects failure (set magicLinkSent: false instead of true); locate
the block that constructs the data object using email.split('@')[1] and update
magicLinkSent accordingly so the logged data (including emailDomain and
magicLinkSent) correctly represents the send outcome.
In `@turbo.json`:
- Around line 76-77: Remove the Vitest-specific environment variable from lint
tasks: delete "VITEST_WORKER_ID" from the environment list used by the
lint:eslint and lint:eslint:fix task definitions so ESLint cache isn't
invalidated unnecessarily; update the turbo.json tasks that reference
VITEST_WORKER_ID (including the occurrences matching lint:eslint and
lint:eslint:fix) to only include environment vars relevant to linting.
🧹 Nitpick comments (7)
apps/fastify/src/lib/auth.ts (3)
15-19: Potential race condition on concurrent initialization.If
getAuth()is called concurrently beforeauthInstanceis assigned, multiplebetterAuthinstances may be created. Consider using a promise-based guard to ensure single initialization.♻️ Suggested fix
-let authInstance: ReturnType<typeof betterAuth> | null = null +let authInstance: ReturnType<typeof betterAuth> | null = null +let authPromise: Promise<ReturnType<typeof betterAuth>> | null = null export async function getAuth() { - if (!authInstance) { + if (authInstance) return authInstance + if (authPromise) return authPromise + + authPromise = (async () => { const db = await getDb()Then at the end of initialization:
+ authInstance = betterAuth({...}) + return authInstance + })() + + return authPromise - } - return authInstance }
17-17: Add explicit return type for exported function.Per coding guidelines, exported functions should have explicit return types.
♻️ Suggested fix
-export async function getAuth() { +export async function getAuth(): Promise<ReturnType<typeof betterAuth>> {
45-50: Consider HTML-escaping the magic link URL.While Better Auth likely generates safe URLs, defensive coding would escape HTML entities in
urlto prevent potential injection if the URL were ever tampered with or contained unexpected characters.♻️ Suggested approach
+const escapeHtml = (str: string) => + str.replace(/[&<>"']/g, c => ({ '&': '&', '<': '<', '>': '>', '"': '"', "'": ''' })[c] || c) + // In sendMagicLink: - html: ` - <p>Hello,</p> - <p>Click the link below to sign in:</p> - <a href="${url}">Sign In</a> - `, + html: ` + <p>Hello,</p> + <p>Click the link below to sign in:</p> + <a href="${escapeHtml(url)}">Sign In</a> + `,Alternatively, consider using the email template service mentioned in the PR summary for consistent, safe templating.
apps/fastify/test/utils/db.ts (1)
62-77: Add explicit return types (and an interface) for exported helpers.The exported helpers rely on type inference. Please add explicit return types and an interface for the returned object shape to align with TS guidelines. As per coding guidelines, exported functions should have explicit return types and prefer interfaces for object shapes.
♻️ Proposed refactor
+interface TestDatabase { + instance: PGlite + url: string + dir: string +} + -export async function getTestDatabase() { +export async function getTestDatabase(): Promise<TestDatabase> { if (!pgLiteInstance) { const dbDir = getWorkerDbDir() // Use file-based PGLite with worker-specific directory // Each worker gets its own isolated database pgLiteInstance = new PGlite(dbDir) await pgLiteInstance.waitReady // Generate connection string compatible with PostgreSQL clients dbUrl = TEST_DATABASE_URL } return { instance: pgLiteInstance, url: dbUrl ?? TEST_DATABASE_URL, dir: getWorkerDbDir(), } } -export async function closeTestDatabase() { +export async function closeTestDatabase(): Promise<void> { if (pgLiteInstance) { const dbDir = getWorkerDbDir() await pgLiteInstance.close() pgLiteInstance = null dbUrl = null // Clean up worker database directory try { await rm(dbDir, { recursive: true, force: true }) } catch { // Ignore cleanup errors (directory might not exist or already deleted) } } } -export async function resetTestDatabase() { +export async function resetTestDatabase(): Promise<TestDatabase> { // Always close and recreate for fresh database await closeTestDatabase() return await getTestDatabase() } -export async function setupTestDatabase() { +export async function setupTestDatabase(): Promise<PGlite> { const { instance } = await getTestDatabase() return instance }Also applies to: 84-114, 122-124
apps/fastify/vitest.setup.ts (1)
45-56: Consider sharing migration runner logic withsrc/db/migrate.ts.The SQL file discovery + breakpoint stripping + exec loop duplicates
runMigrationslogic. A shared helper (e.g.,readAndExecMigrations) would reduce drift and keep behavior consistent across runtime and tests.apps/fastify/vitest.global-setup.ts (1)
30-38: Add explicit return types for exportedsetup/teardown.Please annotate the exported functions with explicit return types to match TS style requirements. As per coding guidelines, exported functions should have explicit return types.
♻️ Proposed refactor
-export async function setup(_context: GlobalSetupContext) { +export async function setup(_context: GlobalSetupContext): Promise<void> { // No global database setup needed // Each worker creates its own database in vitest.setup.ts return } -export async function teardown() { +export async function teardown(): Promise<void> { // Workers clean up their own databases in vitest.setup.ts afterAll hook return }apps/fastify/src/db/migrate.ts (1)
65-68: Export the existing PGlite instance fromsrc/db/index.tsinstead of accessing Drizzle internals.The
pgLiteInstancevariable already exists and is properly maintained inindex.ts. Create a helper to export it and use that inmigrate.tsinstead of accessingdb._.session.client, which relies on internal Drizzle structures that may change across versions.Example implementation
In
src/db/index.ts, add:+export function getPGliteInstance(): PGlite | null { + return pgLiteInstance +}In
src/db/migrate.ts, replace the internal access:- pgliteInstance = (db as unknown as { _: { session: { client: PGlite } } })._.session.client + const instance = getPGliteInstance() + if (!instance) throw new Error('PGlite instance not initialized') + pgliteInstance = instance
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/fastify/src/plugins/auth.ts`:
- Around line 46-52: Don't build the auth URL from request headers
(host/x-forwarded-proto); instead use the trusted env.BETTER_AUTH_URL as the
base. Replace the current construction that uses request.headers.host,
'x-forwarded-proto', and fullUrl with logic that constructs the URL via new
URL(request.url, env.BETTER_AUTH_URL) (or equivalent) and assign that to the
variable currently named fullUrl / url; ensure env.BETTER_AUTH_URL is used as
the authoritative origin and that request.url is treated only as the path
component.
- Around line 36-42: Wrap the session lookup in the onRequest hook
(fastify.addHook) with a try/catch around the call to auth.api.getSession; on
success set request.session to the returned session, on error call
captureError(err) and set request.session = null so requests to non-auth routes
continue to work; follow the same error-handling pattern used in the existing
auth handler where captureError is used to log the exception and default the
session.
In `@packages/utils/package.json`:
- Around line 81-85: Remove the duplicate lodash-es dependency in
packages/utils/package.json by deleting the wildcard entry ("lodash-es": "*")
and keeping the pinned version ("lodash-es": "^4.17.23"); ensure the
dependencies object contains only the "^4.17.23" entry so Dependency resolution
and pnpm-lock.yaml remain consistent.
🧹 Nitpick comments (5)
packages/utils/package.json (1)
50-51: Constrainviem/zodpeer ranges to compatible majors.Using
"*"allows breaking majors, which can cause runtime/type incompatibilities for consumers. Consider aligning peer ranges to the supported majors (e.g., viem 2.x, zod 4.x) to prevent accidental upgrades.♻️ Proposed change
- "viem": "*", - "zod": "*", + "viem": "^2.44.4", + "zod": "^4.3.5",What peer dependency ranges are recommended for packages supporting viem 2.x and zod 4.x?apps/fastify/scripts/migrate.ts (1)
16-24: Silent error handling may hide filesystem issues.The catch block returns an empty array for all errors, which is appropriate when the migrations directory doesn't exist, but could mask permission errors or other filesystem issues during development/debugging.
Consider logging at debug level when an unexpected error occurs:
♻️ Suggested improvement
async function readMigrationFiles(): Promise<string[]> { const migrationsDir = join(projectRoot, 'src', 'db', 'migrations') try { const files = await readdir(migrationsDir) return files.filter(file => file.endsWith('.sql')).sort() - } catch { + } catch (err) { + // ENOENT is expected when directory doesn't exist + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') { + logger.debug({ context: 'migrate', err }, 'Could not read migrations directory') + } return [] } }README.md (1)
7-16: Consider addingnvm use 22for the current shell.
nvm alias default 22won’t change the active session; addingnvm use 22avoids confusion for first-time setup.💡 Doc tweak
-nvm install 22 && nvm alias default 22 +nvm install 22 && nvm alias default 22 +nvm use 22apps/fastify/src/plugins/auth.ts (2)
44-45: Add a TypeBox schema for/api/auth/*validation.Backend routes should declare TypeBox schemas; even a minimal params schema keeps validation/OpenAPI consistent. As per coding guidelines, backend validation should use TypeBox.
♻️ Minimal TypeBox schema
import { captureError } from '@repo/error/node' +import { Type } from '@sinclair/typebox'- fastify.all('/api/auth/*', async (request, reply) => { + fastify.all( + '/api/auth/*', + { + schema: { + params: Type.Object({ '*': Type.String() }), + }, + }, + async (request, reply) => {- }) + }, + )
89-98: GuardHeaders.getSetCookiefor runtime compatibility.
getSetCookieis undici/Node‑specific; if the Response uses another fetch implementation (or older Node), this will throw. Add a feature check and fallback.🧩 Compatibility guard
- const cookies = authResponse.headers.getSetCookie() + const cookies = + typeof authResponse.headers.getSetCookie === 'function' + ? authResponse.headers.getSetCookie() + : authResponse.headers.get('set-cookie') + ? [authResponse.headers.get('set-cookie')!] + : []
|
Deployment failed with the following error: Learn More: https://vercel.com/gaboesquivel?upgradeToPro=build-rate-limit |
|
Deployment failed with the following error: Learn More: https://vercel.com/gaboesquivel?upgradeToPro=build-rate-limit |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/fastify/src/plugins/auth.ts`:
- Around line 82-94: The auth proxy currently always JSON.stringify()s
request.body before creating the Fetch Request in the auth plugin, which mangles
form-urlencoded and binary payloads; update the serialization logic (around the
body variable and Request construction) to: - if request.body is a
Buffer/Uint8Array/ArrayBuffer or a string, pass it through unchanged; - if
Content-Type is application/x-www-form-urlencoded or the body is a plain object
that originated from form parsing, convert it back to a query-string (e.g.
URLSearchParams or equivalent) rather than JSON; - only JSON.stringify() when
the content-type is application/json or the body is a plain object meant as
JSON; and when you re-encode the body (i.e. change format), remove the
content-length header from headers before building the Request to avoid
mismatches.
🧹 Nitpick comments (3)
apps/fastify/src/plugins/auth.ts (3)
64-67: Add a permissive TypeBox schema and explicit return type for/api/auth/*.
The proxy route lacks a TypeBox schema and the handler return type is implicit; even for passthrough endpoints, a permissive schema keeps validation/tooling consistent. As per coding guidelines, ...♻️ Proposed update (schema + explicit return type)
- fastify.all('/api/auth/*', async (request, reply) => { + fastify.all( + '/api/auth/*', + { + schema: { + params: Type.Object({ '*': Type.String() }), + querystring: Type.Any(), + body: Type.Any(), + }, + }, + async (request, reply): Promise<string | null> => { @@ - }) + }, + )// add at top-level imports import { Type } from '@sinclair/typebox'
109-113: Confirm multipleSet-Cookieheaders are preserved.
If Fastify overwrites repeatedreply.header('Set-Cookie', ...), only the last cookie survives. Prefer passing the array in one call.♻️ Safer Set-Cookie forwarding
- for (const cookie of cookies) { - reply.header('Set-Cookie', cookie) - } + if (cookies.length) { + reply.header('set-cookie', cookies) + }
115-118: Avoid forcing auth responses to text.
text()coerces bytes into UTF‑8 and can corrupt non-text bodies. UsearrayBuffer()and send aBufferto preserve payloads.♻️ Preserve raw response bytes
- if (authResponse.body) { - const text = await authResponse.text() - return text - } - return null + if (authResponse.body) { + const buffer = Buffer.from(await authResponse.arrayBuffer()) + return buffer + } + return null
|
Deployment failed with the following error: Learn More: https://vercel.com/gaboesquivel?upgradeToPro=build-rate-limit |
1 participant
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.