Feature - adding better auth for handling authentication and session management (server) #3446
Conversation
|
""" WalkthroughThis update introduces a comprehensive authentication system using the "better-auth" library, integrates new database tables for account, session, and verification management, and enhances environment configuration for security and CORS. The Fastify server is updated to handle authentication routes, and the database schema is extended with new tables and modified user fields. Environment variables for authentication secrets and CORS origins are added across deployment configurations. The setup and test scripts are updated to support these changes, and new tests are introduced to verify authentication endpoint behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastifyServer
participant AuthModule
participant Database
Client->>FastifyServer: HTTP request to /api/auth/*
FastifyServer->>AuthModule: Converts request to Fetch API format
AuthModule->>Database: Authenticates user/session via Drizzle ORM
Database-->>AuthModule: Returns user/session data
AuthModule-->>FastifyServer: Returns authentication result
FastifyServer-->>Client: Sends response (JSON or error)
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
docs/docs/docs/developer-resources/better-auth.mdis excluded by!**/docs/docs/**docs/docs/docs/getting-started/environment-variables.mdis excluded by!**/docs/docs/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
drizzle_migrations/20250411200702_natural_silk_fever.sql(1 hunks)drizzle_migrations/meta/_journal.json(1 hunks)envFiles/.env.ci(1 hunks)envFiles/.env.deploy(1 hunks)envFiles/.env.devcontainer(1 hunks)envFiles/.env.production(1 hunks)package.json(1 hunks)scripts/setup/setup.ts(3 hunks)src/createServer.ts(4 hunks)src/drizzle/schema.ts(1 hunks)src/drizzle/tables/account.ts(1 hunks)src/drizzle/tables/session.ts(1 hunks)src/drizzle/tables/users.ts(3 hunks)src/drizzle/tables/verification.ts(1 hunks)src/envConfigSchema.ts(1 hunks)src/graphql/types/Query/signIn.ts(1 hunks)src/lib/auth.ts(1 hunks)src/lib/db.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/drizzle/tables/verification.ts (1)
src/drizzle/schema.ts (1)
verificationTable(113-113)
src/drizzle/tables/account.ts (2)
src/drizzle/schema.ts (2)
accountTable(114-114)usersTable(94-94)src/drizzle/tables/users.ts (1)
usersTable(59-216)
src/drizzle/tables/session.ts (2)
src/drizzle/schema.ts (2)
sessionTable(112-112)usersTable(94-94)src/drizzle/tables/users.ts (1)
usersTable(59-216)
src/createServer.ts (1)
src/lib/auth.ts (1)
auth(15-123)
🪛 GitHub Actions: Pull request workflow
src/drizzle/tables/verification.ts
[error] 9-9: Formatter would have printed the following content: - }); + });
src/drizzle/tables/account.ts
[error] 20-20: Formatter would have printed the following content: - }); + });
src/drizzle/tables/session.ts
[error] 14-14: Formatter would have printed the following content: - }); + });
src/lib/db.ts
[error] 47-47: Formatter would have printed the following content: - process.on("SIGTERM",·shutdownHandler); + process.on("SIGTERM",·shutdownHandler);
src/lib/auth.ts
[error] 123-123: Formatter would have printed the following content: - }); + });
src/envConfigSchema.ts
[error] 302-302: Formatter would have printed the following content: - * Type of the object containing parsed configuration environment variables.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (27)
package.json (1)
19-19:✅ Verification successful
Verify compatibility with the better-auth package.
The addition of the
better-authpackage (v1.2.5) is appropriate for the stated goal of enhancing authentication and session management. However, it's important to ensure this package is compatible with your existing authentication flow.
🏁 Script executed:
#!/bin/bash # Checking for better-auth package documentation and compatibility npm view better-auth description npm view better-auth repository npm view better-auth dependencies # Check if other files reference this package rg "better-auth" --type tsLength of output: 883
Confirmed: Better-auth package compatibility verified
The package metadata and source code usage have been reviewed. The
better-authpackage (v1.2.5) is correctly integrated across the authentication-related modules (e.g., insrc/lib/auth.ts), and its dependencies are clearly defined. Based on the executed scripts, no compatibility issues with your existing authentication flow were found.envFiles/.env.ci (1)
56-56:✅ Verification successful
Verify CORS configuration for security.
The CORS origin is set to a local development URL. For CI environments, you should ensure this configuration is appropriate for your testing needs.
🏁 Script executed:
#!/bin/bash # Check for CORS origin usage in the codebase rg -A 5 "API_CORS_ORIGIN" --type ts # Check for other CORS settings rg -A 3 "cors" --type tsLength of output: 2067
Action Required: Confirm That the Default Localhost CORS Setting Is Intentional for CI Testing
After verifying the usage of
API_CORS_ORIGINacross the codebase, it's clear that the default value"http://localhost:4321"is consistently applied in several key areas:
- In src/createServer.ts, it's used as the fallback for the Fastify CORS configuration.
- In src/lib/auth.ts, it serves as the default for trusted origins.
- In src/envConfigSchema.ts, it is specified with URI formatting.
- In scripts/setup/setup.ts, it is explicitly set up during project initialization.
Please review and confirm that using
"http://localhost:4321"for the CI environment meets your testing and security requirements. If this default is intended strictly for local development or CI testing, no change is needed. Otherwise, consider updating the configuration to better align with your environment-specific security needs.src/drizzle/schema.ts (1)
112-114:❓ Verification inconclusive
Verify proper relations for new authentication tables.
You've added exports for three new tables (session, verification, account) which aligns with the authentication enhancement objectives. However, these exports don't include table relations unlike other tables in this file.
🏁 Script executed:
#!/bin/bash # Check if these tables have relation definitions cat src/drizzle/tables/session.ts cat src/drizzle/tables/verification.ts cat src/drizzle/tables/account.ts # Check if migrations were created for these tables find ./drizzle_migrations -type f -name "*.sql" | xargs grep -l "CREATE TABLE.*session\|verification\|account" | sort -r | head -1 | xargs catLength of output: 3803
Authentication Tables Relation Consistency
- The
sessionTableandaccountTablecorrectly define foreign key references tousersTable(via theuserIdfield), which is also supported by the migration SQL.- The
verificationTable, however, does not include any foreign key or relation reference. Please confirm whether this is intentional or if it should also reference an associated table (e.g.,usersTable) to maintain consistency with the other authentication tables.envFiles/.env.production (1)
53-53:✅ Verification successful
Ensure secure secret generation for production.
The BETTER_AUTH_SECRET is properly left empty in the production template, but ensure you have a process to generate and securely store a strong random secret for actual production deployments.
🏁 Script executed:
#!/bin/bash # Check if there's a setup script that handles secret generation rg -A 10 "BETTER_AUTH_SECRET" --type ts # Check your setup process cat scripts/setup/setup.ts 2>/dev/null || echo "Setup script not found in expected location"Length of output: 19492
Ensure proper secret instantiation and secure storage in production
- The production template intentionally leaves
BETTER_AUTH_SECRETempty to avoid committing secrets.- The setup process in
scripts/setup/setup.tscorrectly generates a strong secret via thegenerateBetterauthSecret()function.- The schema in
src/envConfigSchema.tsenforces a minimum secret length, confirming that generated secrets are robust.Overall, the current approach is sound. Just be sure your production deployment integrates this setup properly and that the generated secret is stored securely using best practices.
drizzle_migrations/meta/_journal.json (1)
25-31: Migration entry looks goodThe new entry in the migrations journal correctly follows the established pattern and properly increments the index to track the database schema changes for the authentication enhancement.
src/drizzle/tables/account.ts (1)
1-20:✅ Verification successful
Security concerns and structural improvements for account table
The account table has several issues that should be addressed:
- The
passwordfield appears redundant and potentially insecure, as theusersTablealready has apasswordHashfield.- Missing indexes on frequently queried fields (
accountId,providerId).- Timestamp fields need default values and update triggers.
- Code has formatting issues flagged by the pipeline.
import { pgTable, text, timestamp, uuid } from "drizzle-orm/pg-core"; import { usersTable } from "./users"; -export const accountTable = pgTable("account", { +export const accountTable = pgTable( + "account", + { id: uuid("id").primaryKey().defaultRandom(), accountId: text("account_id").notNull(), providerId: text("provider_id").notNull(), userId: uuid("user_id") .notNull() .references(() => usersTable.id, { onDelete: "cascade" }), accessToken: text("access_token"), refreshToken: text("refresh_token"), idToken: text("id_token"), accessTokenExpiresAt: timestamp("access_token_expires_at"), refreshTokenExpiresAt: timestamp("refresh_token_expires_at"), scope: text("scope"), - password: text("password"), - createdAt: timestamp("created_at").notNull(), - updatedAt: timestamp("updated_at").notNull(), + createdAt: timestamp("created_at").notNull().defaultNow(), + updatedAt: timestamp("updated_at").notNull().$onUpdate(() => new Date()), -}); + }, + (table) => ({ + accountProviderIdx: index("account_provider_idx").on(table.accountId, table.providerId), + userIdIdx: index("account_user_id_idx").on(table.userId), + }), +);Also consider whether the
passwordfield is actually needed here sinceusersTablealready has apasswordHashfield. If it has a specific purpose, rename it to clarify (e.g.,providerPasswordHash).
🏁 Script executed:
#!/bin/bash # Check if the usersTable already contains a passwordHash field grep -A 5 "passwordHash" src/drizzle/tables/users.tsLength of output: 174
Account Table: Security & Structural Enhancements Confirmed
The changes have been verified against the users table (which contains the
passwordHashfield), confirming that thepasswordfield in the account table is redundant. The diff snippet addresses:
- Redundant password field: Confirmed removal; if this field is intended for a distinct purpose, consider renaming it (e.g.,
providerPasswordHash) for clarity.- Missing indexes: Added indexes on
accountId,providerId, anduserIdto improve query performance.- Timestamp defaults & updates: The
createdAtfield now defaults to the current time, and theupdatedAtfield includes an update trigger.- Code formatting: The snippet has been refactored for better readability and consistency with our coding standards.
Please confirm these changes align with your design and security requirements.
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] 20-20: Formatter would have printed the following content: - }); + });
src/graphql/types/Query/signIn.ts (1)
74-79:❓ Verification inconclusive
Improved null handling, but consider broader security implications.
The change correctly handles cases where
passwordHashmight be null or undefined, which is a good defensive programming practice. However, allowing users without password hashes raises security concerns.Consider implementing a more explicit check for users without password hashes:
if ( - !(await verify( - existingUser.passwordHash ? existingUser.passwordHash : "", - parsedArgs.input.password, - )) + !existingUser.passwordHash || + !(await verify( + existingUser.passwordHash, + parsedArgs.input.password, + )) ) {This would explicitly handle users without a password hash, making the security failure mode clearer.
Check if there are other places in the codebase that handle password verification to ensure consistent behavior:
🏁 Script executed:
#!/bin/bash # Find all places where password verification is performed rg --type=ts "verify.*password" --no-filenameLength of output: 47
Action Required: Verify and Enhance Password Verification Consistency
The updated code correctly handles cases where a user’s password hash is missing. However, explicitly rejecting users without a password hash can clarify the failure mode and avoid potential security issues. The suggested change is:
if ( - !(await verify( - existingUser.passwordHash ? existingUser.passwordHash : "", - parsedArgs.input.password, - )) + !existingUser.passwordHash || + !(await verify( + existingUser.passwordHash, + parsedArgs.input.password, + )) ) {Next Steps:
- File:
src/graphql/types/Query/signIn.ts(Lines 74-79)
- Update the condition as above to explicitly handle users without a password hash.
- Manual Verification Needed:
The automated search for similar password verification patterns using the command
rg --type=ts "verify.*password" --no-filename
did not return any results. Please manually verify that:
- No other parts of the codebase implement password verification in a conflicting or inconsistent manner.
- The security implications of rejecting users without password hashes are acceptable given the broader authentication strategy.
src/lib/db.ts (3)
1-5: Imports and environment configuration look good
All necessary modules are imported, anddotenv.config()usage is standard.
6-28: Check validity of DATABASE_URL and environment variables
While constructingDATABASE_URLis correct, ensure that all required environment variables (API_POSTGRES_*) are present. Otherwise, the connection may fail silently at runtime.Would you like a verification script to confirm that all required
API_POSTGRES_*variables are set in your environment?
30-46: Database ORM initialization and graceful shutdown
Exporting thedbinstance and defining a comprehensive shutdown handler is a best practice. Good job handling potential errors on shutdown.src/lib/auth.ts (3)
1-13: Imports and initial setup
Imports forbetterAuth,drizzleAdapter, and environment handling are appropriate.
15-48: Comprehensive authentication configuration
Configuring advanced options withgenerateId: falseand mapping user fields is valid for more control. Ensure your database schemas align with these field names.
49-119: Hooks for sign-up and sign-in flows
Logic for returning different payloads on/sign-upand/sign-inendpoints is clear and handles user data well. The error handling also appears robust.src/createServer.ts (3)
1-1: Imported type for headers
This addition helps ensure type safety when iterating over incoming request headers.
15-15: Properly importing authentication module
Theauthimport references your new authentication configuration.
73-82: CORS options set correctly
Definingorigin,credentials, and allowed methods/headers is a robust and secure approach.src/drizzle/tables/users.ts (3)
144-144: Appropriate default value for email verification statusMaking
isEmailAddressVerifieddefault tofalseis a sensible choice for user creation, as it ensures new users start with an unverified status until they complete the verification process.
174-174: Removing NOT NULL constraint enables third-party authenticationMaking
passwordHashnullable is essential for supporting users who authenticate via OAuth or other third-party providers where passwords aren't stored in your system.
182-184: Good default role implementationSetting a default role of "regular" ensures every user has a valid role without requiring explicit assignment during creation, which improves code robustness.
scripts/setup/setup.ts (3)
60-74: Secure implementation of authentication secret generationThe function properly generates a cryptographically secure random secret with appropriate length checking and error handling.
614-640: Well-structured setup function for authentication configurationThis function properly collects all necessary configuration parameters for the Better Auth system with appropriate defaults and error handling.
716-728: User-friendly configuration with sensible defaultsThe implementation gives users the flexibility to choose between customized settings and recommended defaults, which is a good practice for configuration systems.
drizzle_migrations/20250411200702_natural_silk_fever.sql (5)
1-15: Well-structured account table for authenticationThe
accounttable design properly supports multi-provider authentication with appropriate fields for tokens and expiration timestamps.
17-27: Comprehensive session table with security considerationsThe session table includes all necessary fields for secure session management, including expiration, unique tokens, and user context information (IP address, user agent).
29-36: Purpose-built verification tableThis table provides a flexible structure for handling various verification processes (email, phone, etc.) with appropriate expiration management.
38-42: Consistent database schema changes aligning with code updatesThese alterations to the
userstable match exactly with the changes made in the Drizzle ORM definition, ensuring consistency between your code and database schema.
43-44: Proper referential integrity with appropriate cascade behaviorsThe foreign key constraints ensure data integrity while the cascade delete behavior ensures no orphaned records remain when a user is deleted.
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (6)
src/envConfigSchema.ts (3)
288-290: 🛠️ Refactor suggestionAdd documentation for BETTER_AUTH_SECRET.
The new environment variable
BETTER_AUTH_SECRETis well-constrained with a minimum length requirement, but lacks documentation about its purpose and usage within the authentication system.+/** + * Secret key used for authentication and session management. + * This should be a secure random string of at least 32 characters. + * @security This is a security-critical value and should never be reused across environments. + */ BETTER_AUTH_SECRET: Type.String({ minLength: 32, // Ensure a reasonable minimum length for security }),
291-295: 🛠️ Refactor suggestionAdd documentation for NODE_ENV.
The standard
NODE_ENVvariable is included but without documentation about its specific impact on this application.+/** + * Defines the runtime environment which affects various behaviors like error verbosity, + * logging levels, and performance optimizations. + */ NODE_ENV: Type.Enum({ development: "development", production: "production", test: "test", }),
296-299: 🛠️ Refactor suggestionAdd documentation for API_CORS_ORIGIN.
The
API_CORS_ORIGINvariable is added without documentation about its purpose or accepted formats, which is important for proper configuration.+/** + * URI of allowed origins for Cross-Origin Resource Sharing. + * Example formats: "http://localhost:4321" or "https://example.com" + * Use comma-separated values for multiple origins or "*" for allowing all origins (not recommended for production). + */ API_CORS_ORIGIN: Type.String({ format: "uri", minLength: 1, }),src/drizzle/tables/verification.ts (1)
1-9: Add indexes and default timestamps for the verification tableThe verification table looks good structurally, but could benefit from some improvements:
- Missing index on
identifierwhich will likely be queried frequently- The timestamp fields should have default values and update triggers
- The code has a formatting issue flagged by the pipeline
src/drizzle/tables/session.ts (1)
1-14: Add defaults for timestamp fields and indexes for performanceThe session table schema is well-structured but needs improvements for better maintainability and performance:
src/lib/db.ts (1)
46-47: Fix formatting issue in shutdownHandlerThere's a formatting issue with the SIGTERM line as indicated by the pipeline failure.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/drizzle/tables/account.ts(1 hunks)src/drizzle/tables/session.ts(1 hunks)src/drizzle/tables/verification.ts(1 hunks)src/envConfigSchema.ts(1 hunks)src/lib/auth.ts(1 hunks)src/lib/db.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/drizzle/tables/session.ts (2)
src/drizzle/schema.ts (2)
sessionTable(112-112)usersTable(94-94)src/drizzle/tables/users.ts (1)
usersTable(59-216)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (2)
src/lib/auth.ts (2)
1-13: Import structure is clean and comprehensive.The imports are well-organized, bringing in all necessary dependencies for authentication with better-auth, database connectivity via Drizzle, and environment configuration. The explicit import of schema tables supports the adapter configuration.
103-118: Good error handling with detailed logging.The error handling is thorough, with proper type checking and comprehensive logging that includes both the error message and stack trace. The differentiation between Error instances and unknown errors is a best practice.
src/lib/db.ts
Outdated
| try { | ||
| client = postgres(DATABASE_URL, { | ||
| prepare: false, | ||
| max: 10, // ✅ Connection pooling | ||
| idle_timeout: 30, // ✅ Auto-close idle connections | ||
| ssl: process.env.API_POSTGRES_SSL_MODE === "true" ? "allow" : undefined, | ||
| ...(process.env.NODE_ENV === "development" && { | ||
| debug: (connection, query, params) => { | ||
| console.log("Running SQL Query:", query); | ||
| console.log("📌 Query Parameters:", params); | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| console.log("✅ Database connected successfully"); | ||
| } catch (error) { | ||
| console.error("❌ Failed to connect to the database:", error); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement connection retry logic
The database connection code lacks a retry mechanism for handling transient connection failures, which is important for application resilience.
-let client: postgres.Sql;
+let client: postgres.Sql | null = null;
+const MAX_RETRIES = 5;
+const RETRY_DELAY = 3000; // 3 seconds
+
+async function connectWithRetry(retries = MAX_RETRIES): Promise<postgres.Sql> {
+ try {
+ const newClient = postgres(DATABASE_URL, {
+ prepare: false,
+ max: 10,
+ idle_timeout: 30,
+ ssl: process.env.API_POSTGRES_SSL_MODE === "true" ? "allow" : undefined,
+ ...(process.env.NODE_ENV === "development" && {
+ debug: (connection, query, params) => {
+ console.log("Running SQL Query:", query);
+ console.log("📌 Query Parameters:", params);
+ },
+ }),
+ });
+
+ // Test the connection
+ await newClient`SELECT 1`;
+ console.log("✅ Database connected successfully");
+ return newClient;
+ } catch (error) {
+ if (retries > 0) {
+ console.log(`❌ Database connection failed. Retrying in ${RETRY_DELAY/1000} seconds... (${retries} attempts left)`);
+ await new Promise(resolve => setTimeout(resolve, RETRY_DELAY));
+ return connectWithRetry(retries - 1);
+ }
+ console.error("❌ Failed to connect to the database after multiple attempts:", error);
+ process.exit(1);
+ }
+}
-try {
- client = postgres(DATABASE_URL, {
- prepare: false,
- max: 10, // ✅ Connection pooling
- idle_timeout: 30, // ✅ Auto-close idle connections
- ssl: process.env.API_POSTGRES_SSL_MODE === "true" ? "allow" : undefined,
- ...(process.env.NODE_ENV === "development" && {
- debug: (connection, query, params) => {
- console.log("Running SQL Query:", query);
- console.log("📌 Query Parameters:", params);
- },
- }),
- });
-
- console.log("✅ Database connected successfully");
-} catch (error) {
- console.error("❌ Failed to connect to the database:", error);
- process.exit(1);
-}
+// Initialize connection
+connectWithRetry().then(c => {
+ client = c;
+});📝 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.
| try { | |
| client = postgres(DATABASE_URL, { | |
| prepare: false, | |
| max: 10, // ✅ Connection pooling | |
| idle_timeout: 30, // ✅ Auto-close idle connections | |
| ssl: process.env.API_POSTGRES_SSL_MODE === "true" ? "allow" : undefined, | |
| ...(process.env.NODE_ENV === "development" && { | |
| debug: (connection, query, params) => { | |
| console.log("Running SQL Query:", query); | |
| console.log("📌 Query Parameters:", params); | |
| }, | |
| }), | |
| }); | |
| console.log("✅ Database connected successfully"); | |
| } catch (error) { | |
| console.error("❌ Failed to connect to the database:", error); | |
| process.exit(1); | |
| } | |
| let client: postgres.Sql | null = null; | |
| const MAX_RETRIES = 5; | |
| const RETRY_DELAY = 3000; // 3 seconds | |
| async function connectWithRetry(retries = MAX_RETRIES): Promise<postgres.Sql> { | |
| try { | |
| const newClient = postgres(DATABASE_URL, { | |
| prepare: false, | |
| max: 10, // ✅ Connection pooling | |
| idle_timeout: 30, // ✅ Auto-close idle connections | |
| ssl: process.env.API_POSTGRES_SSL_MODE === "true" ? "allow" : undefined, | |
| ...(process.env.NODE_ENV === "development" && { | |
| debug: (connection, query, params) => { | |
| console.log("Running SQL Query:", query); | |
| console.log("📌 Query Parameters:", params); | |
| }, | |
| }), | |
| }); | |
| // Test the connection | |
| await newClient`SELECT 1`; | |
| console.log("✅ Database connected successfully"); | |
| return newClient; | |
| } catch (error) { | |
| if (retries > 0) { | |
| console.log(`❌ Database connection failed. Retrying in ${RETRY_DELAY / 1000} seconds... (${retries} attempts left)`); | |
| await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); | |
| return connectWithRetry(retries - 1); | |
| } | |
| console.error("❌ Failed to connect to the database after multiple attempts:", error); | |
| process.exit(1); | |
| } | |
| } | |
| // Initialize connection | |
| connectWithRetry().then(c => { | |
| client = c; | |
| }); |
| statusCode: "10000", | ||
| message: "Success", | ||
| data: { | ||
| token: session.token, | ||
| id: user.id, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded status codes with named constants.
Using string literals like "10000" and "10001" as status codes reduces maintainability. Consider defining these as named constants at the top of the file or importing them from a shared constants file.
+// Define status codes as constants at the top of the file
+const STATUS_SUCCESS = "10000";
+const STATUS_ERROR = "10001";
+
export const auth = betterAuth({
// ...existing config...
hooks: {
after: createAuthMiddleware(async (ctx) => {
// ...existing code...
return ctx.json({
- statusCode: "10000",
+ statusCode: STATUS_SUCCESS,
message: "Success",
// ...rest of the response...
});
// ...in other places similarly...
}),
},
});Also applies to: 83-94, 100-101, 114-117
| hooks: { | ||
| after: createAuthMiddleware(async (ctx) => { | ||
| try { | ||
| const { path } = ctx; | ||
| const newSession = ctx.context.newSession; | ||
|
|
||
| if (newSession) { | ||
| const user = newSession.user; | ||
| const session = newSession.session; | ||
|
|
||
| if (path.startsWith("/sign-up")) { | ||
| return ctx.json({ | ||
| statusCode: "10000", | ||
| message: "Success", | ||
| data: { | ||
| token: session.token, | ||
| id: user.id, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| if (path.startsWith("/sign-in")) { | ||
| const userDetailsResult = await db | ||
| .select({ | ||
| role: usersTable.role, | ||
| countryCode: usersTable.countryCode, | ||
| avatarName: usersTable.avatarName, | ||
| }) | ||
| .from(usersTable) | ||
| .where(eq(usersTable.id, user.id)); | ||
|
|
||
| const userDetails = userDetailsResult[0]; | ||
|
|
||
| return ctx.json({ | ||
| statusCode: "10000", | ||
| message: "Success", | ||
| data: { | ||
| token: session.token, | ||
| id: user.id, | ||
| email: user.email, | ||
| name: user.name, | ||
| role: userDetails?.role, | ||
| countryCode: userDetails?.countryCode, | ||
| avatarName: userDetails?.avatarName, | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // If no session is available, return an error | ||
| return ctx.json({ | ||
| statusCode: "10001", | ||
| message: "No active session", | ||
| }); | ||
| } catch (error: unknown) { | ||
| console.error( | ||
| "authentication error:", | ||
| error instanceof Error ? error.message : "Unknown error", | ||
| { | ||
| path: ctx.path, | ||
| error: error instanceof Error ? error.stack : String(error), | ||
| }, | ||
| ); | ||
|
|
||
| return ctx.json({ | ||
| statusCode: "10001", | ||
| message: "Failure", | ||
| error: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } | ||
| }), | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider decomposing the authentication hook for better maintainability.
The authentication hook is quite lengthy and handles multiple responsibilities (sign-up, sign-in, error handling). Consider refactoring it into smaller, focused functions to improve readability and maintainability.
+// Extract handlers to separate functions for better maintainability
+const handleSignUp = (user, session) => {
+ return {
+ statusCode: "10000",
+ message: "Success",
+ data: {
+ token: session.token,
+ id: user.id,
+ },
+ };
+};
+
+const handleSignIn = async (user, session, db) => {
+ const userDetailsResult = await db
+ .select({
+ role: usersTable.role,
+ countryCode: usersTable.countryCode,
+ avatarName: usersTable.avatarName,
+ })
+ .from(usersTable)
+ .where(eq(usersTable.id, user.id));
+
+ const userDetails = userDetailsResult[0];
+
+ return {
+ statusCode: "10000",
+ message: "Success",
+ data: {
+ token: session.token,
+ id: user.id,
+ email: user.email,
+ name: user.name,
+ role: userDetails?.role,
+ countryCode: userDetails?.countryCode,
+ avatarName: userDetails?.avatarName,
+ },
+ };
+};
+
export const auth = betterAuth({
// ...existing config...
hooks: {
after: createAuthMiddleware(async (ctx) => {
try {
const { path } = ctx;
const newSession = ctx.context.newSession;
if (newSession) {
const user = newSession.user;
const session = newSession.session;
if (path.startsWith("/sign-up")) {
- return ctx.json({
- statusCode: "10000",
- message: "Success",
- data: {
- token: session.token,
- id: user.id,
- },
- });
+ return ctx.json(handleSignUp(user, session));
}
if (path.startsWith("/sign-in")) {
- const userDetailsResult = await db
- .select({
- role: usersTable.role,
- countryCode: usersTable.countryCode,
- avatarName: usersTable.avatarName,
- })
- .from(usersTable)
- .where(eq(usersTable.id, user.id));
-
- const userDetails = userDetailsResult[0];
-
- return ctx.json({
- statusCode: "10000",
- message: "Success",
- data: {
- token: session.token,
- id: user.id,
- email: user.email,
- name: user.name,
- role: userDetails?.role,
- countryCode: userDetails?.countryCode,
- avatarName: userDetails?.avatarName,
- },
- });
+ return ctx.json(await handleSignIn(user, session, db));
}
}
// ...rest of the function📝 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.
| hooks: { | |
| after: createAuthMiddleware(async (ctx) => { | |
| try { | |
| const { path } = ctx; | |
| const newSession = ctx.context.newSession; | |
| if (newSession) { | |
| const user = newSession.user; | |
| const session = newSession.session; | |
| if (path.startsWith("/sign-up")) { | |
| return ctx.json({ | |
| statusCode: "10000", | |
| message: "Success", | |
| data: { | |
| token: session.token, | |
| id: user.id, | |
| }, | |
| }); | |
| } | |
| if (path.startsWith("/sign-in")) { | |
| const userDetailsResult = await db | |
| .select({ | |
| role: usersTable.role, | |
| countryCode: usersTable.countryCode, | |
| avatarName: usersTable.avatarName, | |
| }) | |
| .from(usersTable) | |
| .where(eq(usersTable.id, user.id)); | |
| const userDetails = userDetailsResult[0]; | |
| return ctx.json({ | |
| statusCode: "10000", | |
| message: "Success", | |
| data: { | |
| token: session.token, | |
| id: user.id, | |
| email: user.email, | |
| name: user.name, | |
| role: userDetails?.role, | |
| countryCode: userDetails?.countryCode, | |
| avatarName: userDetails?.avatarName, | |
| }, | |
| }); | |
| } | |
| } | |
| // If no session is available, return an error | |
| return ctx.json({ | |
| statusCode: "10001", | |
| message: "No active session", | |
| }); | |
| } catch (error: unknown) { | |
| console.error( | |
| "authentication error:", | |
| error instanceof Error ? error.message : "Unknown error", | |
| { | |
| path: ctx.path, | |
| error: error instanceof Error ? error.stack : String(error), | |
| }, | |
| ); | |
| return ctx.json({ | |
| statusCode: "10001", | |
| message: "Failure", | |
| error: error instanceof Error ? error.message : "Unknown error", | |
| }); | |
| } | |
| }), | |
| }, | |
| // Extract handlers to separate functions for better maintainability | |
| const handleSignUp = (user, session) => { | |
| return { | |
| statusCode: "10000", | |
| message: "Success", | |
| data: { | |
| token: session.token, | |
| id: user.id, | |
| }, | |
| }; | |
| }; | |
| const handleSignIn = async (user, session, db) => { | |
| const userDetailsResult = await db | |
| .select({ | |
| role: usersTable.role, | |
| countryCode: usersTable.countryCode, | |
| avatarName: usersTable.avatarName, | |
| }) | |
| .from(usersTable) | |
| .where(eq(usersTable.id, user.id)); | |
| const userDetails = userDetailsResult[0]; | |
| return { | |
| statusCode: "10000", | |
| message: "Success", | |
| data: { | |
| token: session.token, | |
| id: user.id, | |
| email: user.email, | |
| name: user.name, | |
| role: userDetails?.role, | |
| countryCode: userDetails?.countryCode, | |
| avatarName: userDetails?.avatarName, | |
| }, | |
| }; | |
| }; | |
| export const auth = betterAuth({ | |
| // ...existing config... | |
| hooks: { | |
| after: createAuthMiddleware(async (ctx) => { | |
| try { | |
| const { path } = ctx; | |
| const newSession = ctx.context.newSession; | |
| if (newSession) { | |
| const user = newSession.user; | |
| const session = newSession.session; | |
| if (path.startsWith("/sign-up")) { | |
| return ctx.json(handleSignUp(user, session)); | |
| } | |
| if (path.startsWith("/sign-in")) { | |
| return ctx.json(await handleSignIn(user, session, db)); | |
| } | |
| } | |
| // If no session is available, return an error | |
| return ctx.json({ | |
| statusCode: "10001", | |
| message: "No active session", | |
| }); | |
| } catch (error: unknown) { | |
| console.error( | |
| "authentication error:", | |
| error instanceof Error ? error.message : "Unknown error", | |
| { | |
| path: ctx.path, | |
| error: error instanceof Error ? error.stack : String(error), | |
| }, | |
| ); | |
| return ctx.json({ | |
| statusCode: "10001", | |
| message: "Failure", | |
| error: error instanceof Error ? error.message : "Unknown error", | |
| }); | |
| } | |
| }), | |
| }, | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
compose.yaml(1 hunks)docker/compose.deploy.yaml(1 hunks)
🔇 Additional comments (2)
compose.yaml (1)
87-87: Approval: Correct Declaration for BETTER_AUTH_SECRETThe new environment variable
BETTER_AUTH_SECRETis declared correctly, enforcing the intended requirement that it must be set.docker/compose.deploy.yaml (1)
87-87: Approval: Correct Declaration for BETTER_AUTH_SECRETThe new environment variable
BETTER_AUTH_SECRETin the API service section is correctly declared.
|
can we merge it now ? |
|
nothing is showing in the error in failed test cases only it is showing process.exit is called with 1 |
palisadoes
added
the
ignore-sensitive-files-pr
|
We can't merge code with failing tests. |
|
i am trying to merge it as soon as possible . one by one many problem came during development process . main development is part is completed but it 's side part is taking time . As it is a core part of development many dependencies are found which are related to each other so let's see .......... |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/lib/db.ts (2)
1-7: Validate database environment variablesThe code constructs a database URL from environment variables without validating their presence. If any of these variables are undefined, it could lead to cryptic runtime errors.
8-28: Implement connection retry logic for resilienceThe database connection code lacks a retry mechanism for handling transient connection failures, which is important for application resilience. Adding retry logic would enhance the robustness of the application, especially during deployment or when the database might temporarily be unavailable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/db.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
src/lib/db.ts
Outdated
| // ✅ Graceful Shutdown Handler | ||
| const shutdownHandler = async () => { | ||
| console.log("🛑 Closing database connections..."); | ||
| try { | ||
| await client.end(); | ||
| console.log("✅ Database connections closed."); | ||
| } catch (error) { | ||
| console.error("❌ Error closing database connections:", error); | ||
| } | ||
| process.exit(0); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider more robust error handling in shutdown procedure
While the shutdown handler logs errors during database connection closure, it always exits with code 0 regardless of whether an error occurred. For better observability in production environments, consider using a non-zero exit code when errors occur during shutdown.
const shutdownHandler = async () => {
console.log("🛑 Closing database connections...");
try {
await client.end();
console.log("✅ Database connections closed.");
+ process.exit(0);
} catch (error) {
console.error("❌ Error closing database connections:", error);
+ process.exit(1);
}
- process.exit(0);
};📝 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.
| // ✅ Graceful Shutdown Handler | |
| const shutdownHandler = async () => { | |
| console.log("🛑 Closing database connections..."); | |
| try { | |
| await client.end(); | |
| console.log("✅ Database connections closed."); | |
| } catch (error) { | |
| console.error("❌ Error closing database connections:", error); | |
| } | |
| process.exit(0); | |
| }; | |
| // ✅ Graceful Shutdown Handler | |
| const shutdownHandler = async () => { | |
| console.log("🛑 Closing database connections..."); | |
| try { | |
| await client.end(); | |
| console.log("✅ Database connections closed."); | |
| process.exit(0); | |
| } catch (error) { | |
| console.error("❌ Error closing database connections:", error); | |
| process.exit(1); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/lib/db.ts (5)
1-6: 🛠️ Refactor suggestionValidate environment variables before use
The code constructs a database URL from environment variables without validating their presence. If any of these variables is undefined, it will lead to an invalid connection string and potentially unclear error messages.
Consider implementing validation for all required environment variables:
import dotenv from "dotenv"; import { drizzle } from "drizzle-orm/postgres-js"; import postgres from "postgres"; dotenv.config(); +// Validate required environment variables +const requiredEnvVars = [ + "API_POSTGRES_USER", + "API_POSTGRES_PASSWORD", + "API_POSTGRES_HOST", + "API_POSTGRES_PORT", + "API_POSTGRES_DATABASE" +]; + +for (const envVar of requiredEnvVars) { + if (!process.env[envVar]) { + console.error(`❌ Required environment variable ${envVar} is missing!`); + process.exit(1); + } +} const DATABASE_URL = `postgres://${process.env.API_POSTGRES_USER}:${process.env.API_POSTGRES_PASSWORD}@${process.env.API_POSTGRES_HOST}:${process.env.API_POSTGRES_PORT}/${process.env.API_POSTGRES_DATABASE}`;
8-28: 🛠️ Refactor suggestionImplement connection retry logic for database resilience
The current implementation attempts to connect to the database once and exits if it fails. For better resilience, especially in containerized environments where dependent services might start at different times, implement a retry mechanism.
Consider implementing connection retry logic:
-let client: postgres.Sql; +let client: postgres.Sql | null = null; +const MAX_RETRIES = 5; +const RETRY_DELAY = 3000; // 3 seconds + +async function connectWithRetry(retries = MAX_RETRIES): Promise<postgres.Sql> { + try { + const newClient = postgres(DATABASE_URL, { + prepare: false, + max: 10, + idle_timeout: 30, + ssl: process.env.API_POSTGRES_SSL_MODE === "true" ? "allow" : undefined, + ...(process.env.NODE_ENV === "development" && { + debug: (connection, query, params) => { + console.log("Running SQL Query:", query); + console.log("📌 Query Parameters:", params); + }, + }), + }); + + // Test the connection + await newClient`SELECT 1`; + console.log("✅ Database connected successfully"); + return newClient; + } catch (error) { + if (retries > 0) { + console.log(`❌ Database connection failed. Retrying in ${RETRY_DELAY/1000} seconds... (${retries} attempts left)`); + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); + return connectWithRetry(retries - 1); + } + console.error("❌ Failed to connect to the database after multiple attempts:", error); + process.exit(1); + } +} -try { - client = postgres(DATABASE_URL, { - prepare: false, - max: 10, //Connection pooling - idle_timeout: 30, //auto-close idle connections - ssl: process.env.API_POSTGRES_SSL_MODE === "true" ? "allow" : undefined, - ...(process.env.NODE_ENV === "development" && { - debug: (connection, query, params) => { - console.log("Running SQL Query:", query); - console.log("Query Parameters:", params); - }, - }), - }); - - console.log("✅ Database connected successfully"); -} catch (error) { - console.error("❌ Failed to connect to the database:", error); - process.exit(1); -} +// Initialize connection +connectWithRetry().then(c => { + client = c; +});
33-43: 🛠️ Refactor suggestionUse appropriate exit codes in shutdown handler
The shutdown handler always exits with code 0, even when errors occur during database connection closure. This may mask issues and affect monitoring in production environments.
Modify the shutdown handler to use appropriate exit codes:
const shutdownHandler = async () => { console.log("Closing database connections..."); try { await client.end(); console.log("Database connections closed."); + process.exit(0); } catch (error) { console.error("❌ Error closing database connections:", error); + process.exit(1); } - process.exit(0); };
47-47: 🧹 Nitpick (assertive)Remove trailing whitespace
The pipeline reported a formatting issue with trailing whitespace on this line in a previous review.
- process.on("SIGTERM", shutdownHandler);·· + process.on("SIGTERM", shutdownHandler);
35-35: 🧹 Nitpick (assertive)Use consistent logging style for shutdown messages
For consistency with other log messages in the file that use emojis, consider adding an emoji to the shutdown log message.
- console.log("Closing database connections..."); + console.log("🛑 Closing database connections...");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/db.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
|
which files are not changed also test cases are failing which is not related to the pr ? today i have taken the latest code from upstream and some new test cases are failing . code is working , documentation is present , enviromental variable added , setupscript done ,validation on variable is done, ai suggestion done , added in docker compose ,all related files are also done , only the test cases changes coming from upstream are failing . what should i do ? |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
scripts/setup/setup.ts(3 hunks)test/scripts/setup/apiSetup.test.ts(3 hunks)test/scripts/setup/cloudbeaverSetup.test.ts(1 hunks)test/scripts/setup/minioSetup.test.ts(1 hunks)test/scripts/setup/postgresSetup.test.ts(1 hunks)test/scripts/setup/setup.test.ts(3 hunks)test/scripts/setup/updateEnvVariable.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pull request workflow
test/scripts/setup/apiSetup.test.ts
[error] 71-71: TypeScript error TS2345: Argument of type '([{ name }]: UnnamedDistinctQuestion & { name: string; }) => Promise<{ [x: number]: any; }>' is not assignable to parameter of type 'NormalizedProcedure<...>'. Property 'ui' is missing in type 'Promise<{ [x: number]: any; }>' but required in type '{ ui: PromptsRunner<{ [x: string]: any; }>; }'.
[error] 71-71: TypeScript error TS2488: Type 'UnnamedDistinctQuestion & { name: string; }' must have a 'Symbol.iterator' method that returns an iterator.
test/scripts/setup/setup.test.ts
[error] 73-73: Biome formatter error: formatting issues detected at line 73 involving extra spaces or indentation.
scripts/setup/setup.ts
[error] 633-636: Biome formatter error: formatting issues detected. Formatter would have changed whitespace and indentation around lines 633-636.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (10)
test/scripts/setup/cloudbeaverSetup.test.ts (1)
37-37: Good addition for better auth testing coverage.This new mock response for
useDefaultBetterAuthcorrectly extends test coverage to include the new BetterAuth configuration option in the setup flow, aligning with the PR's objective of enhancing authentication and session management.test/scripts/setup/minioSetup.test.ts (1)
59-59: Good integration with setup test flow.The mock response for
useDefaultBetterAuthis consistently added to the test cases, ensuring proper test coverage for the new auth configuration option in different setup scenarios.test/scripts/setup/postgresSetup.test.ts (1)
56-56: Consistent implementation across setup tests.Adding the
useDefaultBetterAuthmock response maintains consistency with the other setup test files and ensures the new authentication setup flow is properly tested.test/scripts/setup/updateEnvVariable.test.ts (1)
8-8: Good practice to isolate test environment.Changing the environment file name from
.envto.env_testproperly isolates the test environment from actual environment files, preventing unintended side effects during testing of the new authentication configuration.test/scripts/setup/setup.test.ts (2)
26-26: Better Auth configuration added for CI=false test caseThe test now includes a mock response for
useDefaultBetterAuthset to "true", ensuring the setup test covers the newly added Better Auth configuration flow. This aligns with the feature additions in the main setup script.
86-86: Better Auth configuration added for CI=true test caseThe test now includes a mock response for
useDefaultBetterAuthset to "true", ensuring the setup test also covers the Better Auth configuration flow in CI mode. This addition is consistent with the PR's feature to add better authentication.test/scripts/setup/apiSetup.test.ts (1)
39-39: Better Auth configuration added to mock responsesThe test now includes a mock response for
useDefaultBetterAuthset to true, aligning with the feature additions in the main setup script.scripts/setup/setup.ts (3)
60-74: New Better Auth secret generation functionThe function to generate a 32-byte secret for Better Auth is well-implemented with proper error handling and validation.
Minor suggestion: Consider using the same naming convention throughout:
-export function generateBetterauthSecret(): string { +export function generateBetterAuthSecret(): string {This would match the camelCase pattern used in
generateJwtSecret().
614-640: Better Auth setup function follows established patternThe
betterAuthSetupfunction follows the same pattern as other service setup functions, prompting for the necessary configuration values with sensible defaults.The function handles:
- Better Auth secret generation
- CORS origin configuration
- Node environment setting
Fix the minor indentation issue in the function:
} catch (err) { handlePromptError(err); } - return answers; + return answers; }🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] 633-636: Biome formatter error: formatting issues detected. Formatter would have changed whitespace and indentation around lines 633-636.
716-724: Setup flow extended with Better Auth configurationThe main setup function now includes a prompt for using default Better Auth settings, maintaining consistency with the pattern used for other services.
This change integrates Better Auth configuration into the setup flow, aligning with the PR's feature for enhanced authentication and session management.
|
Can you fix the type errors, unused imports and |
fixed all those types now you can see. |
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/createServer.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
test/createServer.test.ts
Outdated
| test("should handle errors from auth.handler and return 500", async () => { | ||
| (auth.handler as Mock).mockRejectedValueOnce( | ||
| new Error("Auth service failed"), | ||
| ); | ||
|
|
||
| const response = await server.inject({ | ||
| method: "POST", | ||
| url: "/api/auth/sign-in/email", | ||
| headers: { "content-type": "application/json" }, | ||
| payload: { email: "[email protected]", password: "test" }, | ||
| }); | ||
|
|
||
| expect(response.statusCode).toBe(500); | ||
| expect(response.json()).toEqual({ | ||
| statusCode: "10001", | ||
| message: "Authentication service error", | ||
| error: "Internal server error", | ||
| }); | ||
| expect(auth.handler).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance error handling test
The current error test mocks a rejected promise from auth.handler but doesn't test the server's handling of malformed or invalid requests.
Consider adding tests for specific error scenarios:
// Add after line 98
test("should handle malformed request body and return 400", async () => {
const response = await server.inject({
method: "POST",
url: "/api/auth/sign-in/email",
headers: { "content-type": "application/json" },
payload: "invalid json data",
});
expect(response.statusCode).toBe(400);
expect(response.json()).toHaveProperty("error");
});📝 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.
| test("should handle errors from auth.handler and return 500", async () => { | |
| (auth.handler as Mock).mockRejectedValueOnce( | |
| new Error("Auth service failed"), | |
| ); | |
| const response = await server.inject({ | |
| method: "POST", | |
| url: "/api/auth/sign-in/email", | |
| headers: { "content-type": "application/json" }, | |
| payload: { email: "[email protected]", password: "test" }, | |
| }); | |
| expect(response.statusCode).toBe(500); | |
| expect(response.json()).toEqual({ | |
| statusCode: "10001", | |
| message: "Authentication service error", | |
| error: "Internal server error", | |
| }); | |
| expect(auth.handler).toHaveBeenCalled(); | |
| }); | |
| expect(auth.handler).toHaveBeenCalled(); | |
| }); | |
| +test("should handle malformed request body and return 400", async () => { | |
| + const response = await server.inject({ | |
| + method: "POST", | |
| + url: "/api/auth/sign-in/email", | |
| + headers: { "content-type": "application/json" }, | |
| + payload: "invalid json data", | |
| + }); | |
| + | |
| + expect(response.statusCode).toBe(400); | |
| + expect(response.json()).toHaveProperty("error"); | |
| +}); |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
src/lib/db.ts (5)
1-4: 🧹 Nitpick (assertive)Organize imports according to style guidelines.
The import statements should be reorganized according to Biome's suggestions:
-import { drizzle } from "drizzle-orm/postgres-js"; -import postgres from "postgres"; -import { EnvConfig, envConfigSchema, envSchemaAjv } from "../envConfigSchema"; -import envSchema from "env-schema"; +import envSchema from "env-schema"; +import postgres from "postgres"; +import { drizzle } from "drizzle-orm/postgres-js"; +import type { EnvConfig } from "../envConfigSchema"; +import { envConfigSchema, envSchemaAjv } from "../envConfigSchema";🧰 Tools
🪛 Biome (1.9.4)
[error] 3-3: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.(lint/style/useImportType)
🪛 GitHub Actions: Pull request workflow
[error] 3-3: Biome lint/style/useImportType: Some named imports are only used as types. Use 'import type' to import types to avoid loading unnecessary modules.
[error] 1-4: Biome organizeImports: Import statements could be sorted. Suggested order: import envSchema, then postgres, then EnvConfig and related imports.
17-35: 🛠️ Refactor suggestionImplement connection retry logic for better resilience.
The current database connection code lacks a retry mechanism for handling transient connection failures, which is important for application resilience.
-let client: postgres.Sql; +let client: postgres.Sql | null = null; +const MAX_RETRIES = 5; +const RETRY_DELAY = 3000; // 3 seconds + +async function connectWithRetry(retries = MAX_RETRIES): Promise<postgres.Sql> { + try { + const newClient = postgres(DATABASE_URL, { + prepare: false, + max: 10, // ✅ Connection pooling + idle_timeout: 30, // ✅ Auto-close idle connections + ssl: envConfig.API_POSTGRES_SSL_MODE === true ? "allow" : undefined, + ...(envConfig.NODE_ENV === "development" && { + debug: (connection, query, params) => { + console.log("Running SQL Query:", query); + console.log("📌 Query Parameters:", params); + }, + }), + }); + + // Test the connection + await newClient`SELECT 1`; + console.log("✅ Database connected successfully"); + return newClient; + } catch (error) { + if (retries > 0) { + console.log(`❌ Database connection failed. Retrying in ${RETRY_DELAY/1000} seconds... (${retries} attempts left)`); + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); + return connectWithRetry(retries - 1); + } + console.error("❌ Failed to connect to the database after multiple attempts:", error); + process.exit(1); + } +} -try { - client = postgres(DATABASE_URL, { - prepare: false, - max: 10, - idle_timeout: 30, - ssl: envConfig.API_POSTGRES_SSL_MODE === true ? "allow" : undefined, - ...(envConfig.NODE_ENV === "development" && { - debug: (connection, query, params) => { - console.log("Running SQL Query:", query); - console.log("Query Parameters:", params); - }, - }), - }); - - console.log("Database connected successfully"); -} catch (error) { - console.error("Failed to connect to the database:", error); - process.exit(1); -} +// Initialize connection +(async () => { + client = await connectWithRetry(); +})().catch(error => { + console.error("❌ Failed to initialize database connection:", error); + process.exit(1); +});
26-26: 🧹 Nitpick (assertive)Add emoji for consistent log formatting.
For consistency with other debug logs in the codebase, add an emoji to the query parameters log.
- console.log("Query Parameters:", params); + console.log("📌 Query Parameters:", params);
31-31: 🧹 Nitpick (assertive)Add emoji for consistent log formatting.
For consistency with other logs, add a checkmark emoji to the database connection success message.
- console.log("Database connected successfully"); + console.log("✅ Database connected successfully");
40-50: 🛠️ Refactor suggestionImprove error handling in shutdown procedure.
The shutdown handler logs errors during database connection closure, but always exits with code 0 regardless of whether an error occurred. For better observability in production environments, consider using a non-zero exit code when errors occur during shutdown.
-//Graceful Shutdown Handler +// Graceful Shutdown Handler const shutdownHandler = async () => { - console.log("Closing database connections..."); + console.log("🛑 Closing database connections..."); try { await client.end(); - console.log("Database connections closed."); + console.log("✅ Database connections closed."); + process.exit(0); } catch (error) { console.error("❌ Error closing database connections:", error); + process.exit(1); } - process.exit(0); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/db.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/db.ts (1)
src/envConfigSchema.ts (3)
EnvConfig(305-305)envSchemaAjv(310-317)envConfigSchema(8-300)
🪛 Biome (1.9.4)
src/lib/db.ts
[error] 3-3: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.
(lint/style/useImportType)
🪛 GitHub Actions: Pull request workflow
src/lib/db.ts
[error] 3-3: Biome lint/style/useImportType: Some named imports are only used as types. Use 'import type' to import types to avoid loading unnecessary modules.
[error] 1-4: Biome organizeImports: Import statements could be sorted. Suggested order: import envSchema, then postgres, then EnvConfig and related imports.
[error] 13-13: Biome format: Formatter would have changed console.log quotes from single to double quotes.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
| prepare: false, | ||
| max: 10, | ||
| idle_timeout: 30, | ||
| ssl: envConfig.API_POSTGRES_SSL_MODE === true ? "allow" : undefined, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify SSL configuration handling.
The SSL configuration seems simplified compared to what the schema supports. According to the schema snippet, API_POSTGRES_SSL_MODE can be an enum with values like "allow", "prefer", "require", "verify-full", or a boolean. The current implementation only handles the boolean case.
- ssl: envConfig.API_POSTGRES_SSL_MODE === true ? "allow" : undefined,
+ ssl: typeof envConfig.API_POSTGRES_SSL_MODE === "boolean"
+ ? (envConfig.API_POSTGRES_SSL_MODE ? "allow" : undefined)
+ : envConfig.API_POSTGRES_SSL_MODE,Please verify the usage of the API_POSTGRES_SSL_MODE in your codebase to ensure it handles all possible values from the schema correctly.
🏁 Script executed:
#!/bin/bash
set -e
# Locate occurrences of API_POSTGRES_SSL_MODE
echo "Searching for API_POSTGRES_SSL_MODE references..."
rg "API_POSTGRES_SSL_MODE" -n .
# Identify the schema or type definition for envConfig/API_POSTGRES_SSL_MODE
echo -e "\nSearching for envConfig definition..."
rg "envConfig" -n src
# Search for where the configuration schema is defined (e.g., Zod, Joi, or TS types)
echo -e "\nLooking for schema definitions mentioning SSL_MODE..."
rg -n "ssl_mode\|SSL_MODE\|postgres" -n srcLength of output: 59460
🏁 Script executed:
#!/bin/bash
set -e
# Display the API_POSTGRES_SSL_MODE schema definition
echo "Extracting API_POSTGRES_SSL_MODE union definition from envConfigSchema.ts..."
sed -n '210,260p' src/envConfigSchema.tsLength of output: 1494
🏁 Script executed:
#!/bin/bash
set -e
# Display the first 100 lines of src/lib/db.ts for context
echo "Extracting src/lib/db.ts content (lines 1-100)..."
sed -n '1,100p' src/lib/db.tsLength of output: 1703
Ensure all SSL mode values are supported
The API_POSTGRES_SSL_MODE schema permits both booleans and the strings "allow", "prefer", "require", or "verify-full". Right now in src/lib/db.ts a string value always falls through to undefined, disabling SSL when you actually asked for e.g. "require". We need to handle both cases:
- ssl: envConfig.API_POSTGRES_SSL_MODE === true ? "allow" : undefined,
+ ssl:
+ typeof envConfig.API_POSTGRES_SSL_MODE === "boolean"
+ ? envConfig.API_POSTGRES_SSL_MODE
+ ? "allow"
+ : undefined
+ : envConfig.API_POSTGRES_SSL_MODE,Please apply the same change to where you configure SSL in the Drizzle plugin:
• src/lib/db.ts (line 22)
• src/plugins/drizzleClient.ts (line 29)
📝 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.
| ssl: envConfig.API_POSTGRES_SSL_MODE === true ? "allow" : undefined, | |
| // Replace this: | |
| - ssl: envConfig.API_POSTGRES_SSL_MODE === true ? "allow" : undefined, | |
| // With this: | |
| ssl: | |
| typeof envConfig.API_POSTGRES_SSL_MODE === "boolean" | |
| ? envConfig.API_POSTGRES_SSL_MODE | |
| ? "allow" | |
| : undefined | |
| : envConfig.API_POSTGRES_SSL_MODE, |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/lib/db.ts (5)
36-36: Add emoji for consistent success logging
Align this success message with the team's logging style by adding a checkmark emoji:- console.log("Database connected successfully"); + console.log("✅ Database connected successfully");
42-43: Standardize inline comment formatting
Ensure a space follows//for consistency:- //Connect Drizzle ORM + // Connect Drizzle ORM - //Graceful Shutdown Handler + // Graceful Shutdown Handler - //Listen for termination signals + // Listen for termination signalsAlso applies to: 45-45, 57-59
16-19:⚠️ Potential issueRemove clear-text logging of sensitive information
Logging the fullDATABASE_URLexposes credentials (including passwords) in plaintext logs. This poses a high security risk. Remove or sanitize this output before logging.- console.log("DATABASE_URL", DATABASE_URL); + // Avoid logging sensitive credentials in clear text + console.log( + "Database connection configured:", + `postgres://${envConfig.API_POSTGRES_USER}:***@${envConfig.API_POSTGRES_HOST}:${envConfig.API_POSTGRES_PORT}/${envConfig.API_POSTGRES_DATABASE}` + );
45-55: 🛠️ Refactor suggestionImprove shutdown handler exit codes and logging
Currently, the handler always exits with code0, even on errors, obscuring shutdown issues. Also, enhance clarity with emojis:const shutdownHandler = async () => { - console.log("Closing database connections..."); + console.log("🛑 Closing database connections..."); try { await client.end(); - console.log("Database connections closed."); + console.log("✅ Database connections closed."); - } catch (error) { - console.error("❌ Error closing database connections:", error); - } - process.exit(0); + } catch (error) { + console.error("❌ Error closing database connections:", error); + process.exit(1); + } + process.exit(0); };
22-34:⚠️ Potential issueSupport all
API_POSTGRES_SSL_MODEvalues
Currently, only booleantruemaps to"allow"and all other values disable SSL. This ignores explicit string modes like"require"or"verify-full". Update thesslsetting to handle both boolean and string values from the schema:- ssl: envConfig.API_POSTGRES_SSL_MODE === true ? "allow" : undefined, + ssl: + typeof envConfig.API_POSTGRES_SSL_MODE === "boolean" + ? envConfig.API_POSTGRES_SSL_MODE + ? "allow" + : undefined + : envConfig.API_POSTGRES_SSL_MODE,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/db.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (2)
src/lib/db.ts (2)
10-14: Env schema validation implemented correctly
Usingenv-schemato load and strictly validate environment variables at startup is a robust approach that prevents missing or malformed configuration.
42-43: Drizzle ORM initialization
Exportingdbviadrizzle(client)correctly exposes the ORM instance for use across the application.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
envFiles/.env.ci (1)
55-55: Security concern: Hardcoded authentication secret in CI environment
ThisBETTER_AUTH_SECRETis checked into the repository, exposing a sensitive credential. Please replace it with a placeholder that is injected via CI secrets or environment variables, e.g.:-BETTER_AUTH_SECRET=jvc2rzepazKXR1Glj72lfiW9iOkjZnve +BETTER_AUTH_SECRET=${BETTER_AUTH_SECRET}🧰 Tools
🪛 Gitleaks (8.21.2)
55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
src/lib/db.ts (3)
22-27: Ensure all SSL mode values are supported
The current SSL setup only handles booleantrue→"allow"and treats all strings ("require","verify-full", etc.) asundefined, disabling SSL when it may be required.
Please handle both boolean and enum cases, e.g.:- ssl: envConfig.API_POSTGRES_SSL_MODE === true ? "allow" : undefined, + ssl: typeof envConfig.API_POSTGRES_SSL_MODE === "boolean" + ? (envConfig.API_POSTGRES_SSL_MODE ? "allow" : undefined) + : envConfig.API_POSTGRES_SSL_MODE,
28-33: Add emojis to debug logs for consistency
To match your existing log style, prefix debug statements with emojis. For example:- console.log("Running SQL Query:", query); - console.log("Query Parameters:", params); + console.log("🔍 Running SQL Query:", query); + console.log("📌 Query Parameters:", params);
45-55: Improve shutdown exit codes on failure
Currently you alwaysprocess.exit(0)even ifclient.end()fails. It’s better to exit non-zero on errors:const shutdownHandler = async () => { - console.log("Closing database connections..."); + console.log("🛑 Closing database connections..."); try { await client.end(); - console.log("Database connections closed."); + console.log("✅ Database connections closed."); + process.exit(0); } catch (error) { console.error("❌ Error closing database connections:", error); + process.exit(1); } - process.exit(0); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
envFiles/.env.ci(1 hunks)src/lib/db.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/db.ts (1)
src/envConfigSchema.ts (3)
EnvConfig(305-305)envSchemaAjv(310-317)envConfigSchema(8-300)
🪛 Gitleaks (8.21.2)
envFiles/.env.ci
55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (6)
envFiles/.env.ci (2)
54-54: Ensure correct CI environment mode
SettingNODE_ENV=testfor CI runs is appropriate to isolate test configuration and should prevent accidental production settings from being applied.
56-56: Approve CORS origin configuration for CI
TheAPI_CORS_ORIGIN=http://localhost:4321setting aligns with local testing needs.src/lib/db.ts (4)
1-16: The import statements and initial environment loading (includingdotenv.config()andenvSchemainvocation) follow the expected pattern; no issues detected.
36-40: Startup error handling looks solid
On connection failure you log the error and exit with code 1, which is the correct behavior for unrecoverable startup issues.
42-43: Drizzle ORM instantiation is correct
Exportingdbviadrizzle(client)properly wires up the ORM layer to yourpostgresclient.
57-59: Termination signal handlers are set correctly
You’re listening forSIGINTandSIGTERM, which covers the most common restart/stop scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
envFiles/.env.ci (1)
55-55:⚠️ Potential issueRemove hardcoded authentication secret
HardcodingBETTER_AUTH_SECRETin the CI env file exposes a sensitive key. Use a placeholder and inject the real secret via CI-managed environment variables or a secrets manager.-BETTER_AUTH_SECRET=jvc2rzepazKXR1Glj72lfiW9iOkjZnve +BETTER_AUTH_SECRET=${BETTER_AUTH_SECRET:?Please set BETTER_AUTH_SECRET in CI secrets}🧰 Tools
🪛 Gitleaks (8.21.2)
55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
src/lib/db.ts (8)
10-14: 🧹 Nitpick (assertive)Extract environment loading into a dedicated function
For better error handling and maintainability, consider extracting the environment validation into a separate function.
+/** + * Loads and validates environment configuration using env-schema + * @returns Validated environment configuration + */ +function loadEnvironmentConfig(): EnvConfig { + try { + return envSchema<EnvConfig>({ + ajv: envSchemaAjv, + dotenv: true, + schema: envConfigSchema, + }); + } catch (error) { + console.error("❌ Environment configuration error:", error); + process.exit(1); + } +} -const envConfig = envSchema<EnvConfig>({ - ajv: envSchemaAjv, - dotenv: true, - schema: envConfigSchema, -}); +const envConfig = loadEnvironmentConfig();
18-38: 🛠️ Refactor suggestionImplement connection retry logic for database resilience
The current implementation lacks a retry mechanism for handling transient connection failures. This is important for application resilience, especially in containerized environments.
-let client: postgres.Sql; +let client: postgres.Sql | null = null; +const MAX_RETRIES = 5; +const RETRY_DELAY = 3000; // 3 seconds + +async function connectWithRetry(retries = MAX_RETRIES): Promise<postgres.Sql> { + try { + const newClient = postgres(DATABASE_URL, { + prepare: false, + max: 10, + idle_timeout: 30, + ssl: typeof envConfig.API_POSTGRES_SSL_MODE === "boolean" + ? envConfig.API_POSTGRES_SSL_MODE ? "allow" : undefined + : envConfig.API_POSTGRES_SSL_MODE, + ...(envConfig.NODE_ENV === "development" && { + debug: (connection, query, params) => { + console.log("🔍 Running SQL Query:", query); + console.log("📌 Query Parameters:", params); + }, + }), + }); + + // Test the connection + await newClient`SELECT 1`; + console.log("✅ Database connected successfully"); + return newClient; + } catch (error) { + if (retries > 0) { + console.log(`❌ Database connection failed. Retrying in ${RETRY_DELAY/1000} seconds... (${retries} attempts left)`); + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); + return connectWithRetry(retries - 1); + } + console.error("❌ Failed to connect to the database after multiple attempts:", error); + process.exit(1); + } +} -try { - client = postgres(DATABASE_URL, { - prepare: false, - max: 10, - idle_timeout: 30, - ssl: envConfig.API_POSTGRES_SSL_MODE === true ? "allow" : undefined, - ...(envConfig.NODE_ENV === "development" && { - debug: (connection, query, params) => { - console.log("Running SQL Query:", query); - console.log("Query Parameters:", params); - }, - }), - }); - - console.log("Database connected successfully"); -} catch (error) { - console.error("Failed to connect to the database:", error); - process.exit(1); -} +// Initialize connection +(async () => { + client = await connectWithRetry(); +})().catch(error => { + console.error("❌ Failed to initialize database connection:", error); + process.exit(1); +});
25-25:⚠️ Potential issueFix SSL mode configuration to support all schema values
The current implementation only handles the boolean case for
API_POSTGRES_SSL_MODE, but according to the environment schema, it can also be a string enum with values like "allow", "prefer", "require", "verify-full".- ssl: envConfig.API_POSTGRES_SSL_MODE === true ? "allow" : undefined, + ssl: + typeof envConfig.API_POSTGRES_SSL_MODE === "boolean" + ? envConfig.API_POSTGRES_SSL_MODE + ? "allow" + : undefined + : envConfig.API_POSTGRES_SSL_MODE,
27-30: 🧹 Nitpick (assertive)Enhance debug log formatting with descriptive emojis
For consistency with other logs in the codebase, add descriptive emojis to SQL debug statements.
- console.log("Running SQL Query:", query); - console.log("Query Parameters:", params); + console.log("🔍 Running SQL Query:", query); + console.log("📌 Query Parameters:", params);
34-34: 🧹 Nitpick (assertive)Add emoji for consistent log formatting
For consistency with other logs in the codebase, add a checkmark emoji to the database connection success message.
- console.log("Database connected successfully"); + console.log("✅ Database connected successfully");
40-41: 🧹 Nitpick (assertive)Improve comment style
Add a space after the comment slashes for better readability and consistency.
-//Connect Drizzle ORM +// Connect Drizzle ORM export const db = drizzle(client);
43-53: 🛠️ Refactor suggestionImprove error handling in shutdown procedure
The shutdown handler always exits with code 0, even if there's an error when closing the connection. For better observability in production environments, consider using a non-zero exit code when errors occur.
-//Graceful Shutdown Handler +// Graceful Shutdown Handler const shutdownHandler = async () => { - console.log("Closing database connections..."); + console.log("🛑 Closing database connections..."); try { await client.end(); - console.log("Database connections closed."); + console.log("✅ Database connections closed."); + process.exit(0); } catch (error) { console.error("❌ Error closing database connections:", error); + process.exit(1); } - process.exit(0); };
55-57: 🧹 Nitpick (assertive)Improve comment style
Add a space after the comment slashes for better readability and consistency.
-//Listen for termination signals +// Listen for termination signals process.on("SIGINT", shutdownHandler); process.on("SIGTERM", shutdownHandler);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
envFiles/.env.ci(1 hunks)src/lib/db.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/db.ts (1)
src/envConfigSchema.ts (3)
EnvConfig(305-305)envSchemaAjv(310-317)envConfigSchema(8-300)
🪛 Gitleaks (8.21.2)
envFiles/.env.ci
55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
envFiles/.env.ci (1)
56-56: Verify CORS origin configuration for CI
Ensure thatAPI_CORS_ORIGINmatches your CI test environment requirements. If CORS isn’t needed during CI runs, consider removing or overriding this value to avoid unnecessary configuration.
…used the code fails
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
test/createServer.test.ts (3)
12-31: Simplify db mock for authentication testsThe current db mock includes detailed data structures that don't appear to be directly used in these authentication tests.
Since these tests focus on the authentication API endpoints rather than database interactions, consider simplifying the db mock if it's not directly used by the tests:
vi.mock("~/src/lib/db", async () => { const actual = await vi.importActual<typeof import("~/src/lib/db")>("~/src/lib/db"); return { ...actual, - db: { - select: vi.fn(() => ({ - from: () => ({ - where: () => [ - { - role: "admin", - countryCode: "IN", - avatarName: "avatar123", - }, - ], - }), - })), - }, + db: { + ...actual.db, + // Only mock methods that are actually used in these tests + }, }; });If the db mock is used by these tests, please add a comment explaining its purpose.
60-77: Clarify test purpose for missing headers caseThe test "should handle requests with missing headers gracefully" doesn't clearly test for the missing headers case.
This test configures auth.handler to return a 400 response, but it's not clear that the 400 is specifically due to missing headers. Consider:
- Adding a comment explaining the expected behavior when headers are missing
- Explicitly asserting that the auth.handler receives a request without the expected headers
test("should handle requests with missing headers gracefully", async () => { + // When content-type header is missing, the request should be rejected with 400 (auth.handler as Mock).mockResolvedValueOnce( new Response(JSON.stringify({ success: false }), { status: 400, headers: { "content-type": "application/json" }, }), ); const response = await Server.inject({ method: "POST", url: "/api/auth/sign-in/email", payload: { email: "[email protected]", password: "test" }, }); expect(response.statusCode).toBe(400); expect(response.json()).toEqual({ success: false }); expect(auth.handler).toHaveBeenCalled(); + + // Verify that the request passed to auth.handler doesn't have content-type + const req = (auth.handler as Mock).mock.calls[0]?.[0] as Request; + expect(req.headers.get("content-type")).toBeNull(); });
40-58: 🛠️ Refactor suggestionAdd cookie validation to authentication tests
Given that the PR introduces "cookie management similar to refresh tokens", the tests should verify that cookies are properly handled.
Enhance the successful authentication test to validate cookies in the response:
test("should handle valid requests and return the expected response", async () => { (auth.handler as Mock).mockResolvedValueOnce( new Response(JSON.stringify({ success: true }), { status: 200, headers: { "content-type": "application/json", + "set-cookie": "auth-token=xyz123; HttpOnly; Secure; SameSite=Strict" }, }), ); const response = await Server.inject({ method: "POST", url: "/api/auth/sign-in/email", headers: { "content-type": "application/json" }, payload: { email: "[email protected]", password: "test" }, }); expect(response.statusCode).toBe(200); expect(response.json()).toEqual({ success: true }); + expect(response.headers["set-cookie"]).toBeDefined(); + expect(response.headers["set-cookie"][0]).toContain("auth-token="); + expect(response.headers["set-cookie"][0]).toContain("HttpOnly"); expect(auth.handler).toHaveBeenCalled(); });This ensures that the server properly forwards authentication cookies from the auth handler to the client.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/createServer.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/createServer.test.ts (2)
src/lib/auth.ts (1)
auth(15-123)test/server.ts (1)
server(12-27)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
| }); | ||
|
|
||
| test("should forward headers and body correctly to auth.handler", async () => { | ||
| (auth.handler as Mock).mockResolvedValueOnce( | ||
| new Response(null, { status: 204 }), | ||
| ); | ||
| const response = await Server.inject({ | ||
| method: "POST", | ||
| url: "/api/auth/update", | ||
| headers: { "x-custom-header": "value" }, | ||
| payload: { data: "some data" }, | ||
| }); | ||
|
|
||
| expect(response.statusCode).toBe(204); | ||
| expect((auth.handler as Mock).mock.calls.length).toBeGreaterThan(0); | ||
|
|
||
| const req = (auth.handler as Mock).mock.calls[0]?.[0] as Request; | ||
| expect(req.method).toBe("POST"); | ||
|
|
||
| const contentType = req.headers.get("content-type"); | ||
| expect(contentType).toContain("application/json"); | ||
|
|
||
| const body = await req.text(); | ||
| expect(body).toBe(JSON.stringify({ data: "some data" })); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling test coverage
The current tests verify basic error handling but don't test the server's handling of malformed or invalid requests.
Add a test for malformed request body:
test("should handle malformed request body and return 400", async () => {
const response = await Server.inject({
method: "POST",
url: "/api/auth/sign-in/email",
headers: { "content-type": "application/json" },
payload: "invalid json data",
});
expect(response.statusCode).toBe(400);
expect(response.json()).toHaveProperty("error");
});This tests how the server handles invalid JSON in the request body, which is a common error case in authentication endpoints.
🛠️ Refactor suggestion
Add test for session management endpoints
The PR objectives mention "session management functionalities", but the tests don't cover session-specific endpoints.
Add a test for session management endpoints:
test("should handle session management endpoints", async () => {
(auth.handler as Mock).mockResolvedValueOnce(
new Response(JSON.stringify({ session: { active: true, expires: "2025-04-15T00:00:00Z" } }), {
status: 200,
headers: { "content-type": "application/json" },
}),
);
const response = await Server.inject({
method: "GET",
url: "/api/auth/session",
headers: {
"content-type": "application/json",
"cookie": "auth-token=xyz123"
},
});
expect(response.statusCode).toBe(200);
expect(response.json()).toHaveProperty("session.active", true);
expect(auth.handler).toHaveBeenCalled();
// Verify that cookies are forwarded to auth.handler
const req = (auth.handler as Mock).mock.calls[0]?.[0] as Request;
expect(req.headers.get("cookie")).toContain("auth-token=xyz123");
});This test would verify that session-related endpoints work correctly and that cookies are properly forwarded to the auth handler.
|
it is passed finally . |
|
Congratulations. Please get CodeRabbit to approve your changes. |
|
@palisadoes you can add this . it will work perfectly. all thing is completed. |
|
Is the equivalent working in Talawa Admin? |
|
It is working fine . I already checked it |
|
Yes, but the Admin PR isn't merged. I cannot merge this until then |
|
Sorry but due to typescript error eslint in admin flow it is unable to pass. You can unassign me. Any one who will work on this only have to fix the eslint workflow then it will ready to merge . |
|
What kind of change does this PR introduce?
Feature
Issue Number
Fixes #3305
Additional Context
Snapshots/Videos:
If relevant, did you update the documentation?
Yes i have updated the documentation in docs/docs/docs/developer-resources
Summary
-Now Login , sign up, sign out, session management , and enable cookie (similar to refresh token) is implemented with better auth.
Does this PR introduce a breaking change?
-Yes in this pr we have to migrate the database schema.due to addition of session , account, verification table in database.
Checklist
CodeRabbit AI Review
Test Coverage
Other information
It is related to our new authentication system.
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests