refactor(auth): enhance auth configuration #411
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update introduces a new React component, LoginClient, which handles user authentication via multiple providers (e.g., Google and GitHub). The login page has been refactored (renamed to LoginPage) to construct a providers array that conveys each provider's environment variable status. A new module for authentication providers centralizes configuration using dedicated interfaces and utility functions, replacing previously hardcoded values. Additionally, the build configuration now includes a new environment variable for GitHub redirection. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant LP as LoginPage
participant LC as LoginClient
participant AP as AuthProvider
participant Router as Navigation
U ->> LP: Visit Login Page
LP ->> LC: Render login UI with provider statuses
U ->> LC: Click provider button
LC ->> AP: Initiate sign-in process
AP -->> LC: Return authentication result
LC ->> Router: Redirect user upon success
LC ->> U: Display feedback via toast notifications
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
apps/mail/app/(auth)/login/login-client.tsx (1)
45-85: Consider using optional chaining for session checkThe code has good session management and routing logic, but there's an opportunity to use optional chaining for cleaner code.
- if (!isPending && session?.connectionId) { + if (!isPending && session?.connectionId) {On line 85, consider using optional chaining:
- if (isPending || (session && session.connectionId)) return null; + if (isPending || session?.connectionId) return null;🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/mail/lib/auth-providers.ts (7)
12-12: Consider using a more specific type instead ofanyUsing
anyfor theconfigproperty circumvents TypeScript's type checking benefits. Consider using a more specific type structure that reflects the expected configuration properties for authentication providers, or at minimum useRecord<string, unknown>for better type safety.- config: any; + config: Record<string, unknown>;For even better type safety, you could create specific provider configuration interfaces:
interface ProviderConfigDetails { clientId: string; clientSecret: string; [key: string]: unknown; } // Then update the interface export interface ProviderConfig { // other properties config: ProviderConfigDetails; // remaining properties }
35-36: Convert production-related comment to a tracked TODOThis comment indicates temporary code that should be removed before production. Consider converting it to a standardized TODO format that can be tracked by linters or task management tools.
- // Remove this before going to prod, it's to force to get `refresh_token` from google, some users don't have it yet. + // TODO(auth): Remove 'prompt: "consent"' before production release - it's currently used to force obtaining refresh_token from Google
31-32: Consider environment-aware redirect URIsThe default redirect URIs are hardcoded to localhost, which is appropriate for development but not for production. Consider making these values environment-aware.
defaultValue: "http://localhost:3000/api/v1/mail/auth/google/callback"You might want to implement a helper function that determines the appropriate base URL:
function getBaseUrl() { return process.env.NODE_ENV === 'production' ? process.env.PRODUCTION_URL || 'https://your-production-domain.com' : 'http://localhost:3000'; } // Then use it in defaultValue defaultValue: `${getBaseUrl()}/api/v1/mail/auth/google/callback`Also applies to: 58-59
72-74: Enhance error logs with setup guidanceThe error messages indicate what's missing but don't provide guidance on how to fix the issues. Consider enhancing these error messages with more actionable information.
- console.error(`Required provider "${provider.id}" is not configured properly.`); - console.error(`Missing environment variables: ${provider.requiredEnvVars.filter(envVar => !process.env[envVar]).join(', ')}`); + console.error(`Required provider "${provider.id}" is not configured properly.`); + const missingVars = provider.requiredEnvVars.filter(envVar => !process.env[envVar]); + console.error(`Missing environment variables: ${missingVars.join(', ')}`); + + // Add guidance for each missing variable + missingVars.forEach(envVar => { + const info = provider.envVarInfo?.find(info => info.name === envVar); + if (info) { + console.error(` - ${envVar}: Can be obtained from ${info.source}${info.defaultValue ? `. Default: "${info.defaultValue}"` : ''}`); + } + });
68-77: Add documentation for the isProviderEnabled functionAdd JSDoc comments to explain the purpose, parameters, and return value of this important function. This will improve code readability and maintainability.
+/** + * Checks if a provider is properly configured by verifying the presence of required environment variables. + * Logs errors for missing environment variables if the provider is marked as required. + * + * @param provider - The authentication provider configuration to check + * @returns boolean indicating whether the provider is properly configured + */ export function isProviderEnabled(provider: ProviderConfig): boolean {
79-91: Add documentation for the getSocialProviders functionAdd JSDoc comments to explain the purpose, behavior, and return value of this function, including information about the error it might throw.
+/** + * Constructs an object containing configurations for all enabled social providers. + * Validates that all required providers are properly configured. + * + * @returns An object mapping provider IDs to their respective configurations + * @throws Error if a required provider is not properly configured + */ export function getSocialProviders() {
7-14: Improve ProviderConfig interface documentationAdd JSDoc comments to explain the purpose and properties of this interface to improve code readability and maintainability.
+/** + * Configuration for an authentication provider + */ export interface ProviderConfig { /** Unique identifier for the provider */ id: string; /** Display name of the provider */ name: string; /** List of environment variable names required for this provider to function */ requiredEnvVars: string[]; /** Detailed information about each environment variable */ envVarInfo?: EnvVarInfo[]; /** Provider-specific configuration object */ config: any; /** Indicates if this provider is required for the application to function properly */ required?: boolean; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/mail/app/(auth)/login/login-client.tsx(1 hunks)apps/mail/app/(auth)/login/page.tsx(1 hunks)apps/mail/lib/auth-providers.ts(1 hunks)apps/mail/lib/auth.ts(2 hunks)turbo.json(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/mail/app/(auth)/login/login-client.tsx
[error] 85-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
turbo.json (1)
16-16: Good addition of GitHub redirect URI to environment variablesThis addition properly aligns with other authentication-related environment variables and enables GitHub authentication configuration.
apps/mail/lib/auth.ts (2)
9-9: Good refactoring: importing auth provider configuration functionThe import of
getSocialProviderssupports the centralization of authentication provider configuration.
30-30: Great improvement: replacing hardcoded config with centralized functionReplacing hardcoded social provider configurations with a function call improves maintainability and makes the code more flexible.
apps/mail/app/(auth)/login/page.tsx (1)
1-24: Well-structured refactoring of the login pageThe refactoring properly separates server and client components, with the server component (
LoginPage) preparing provider data and the client component handling UI and interactions. The code effectively maps provider configuration to determine availability status.apps/mail/app/(auth)/login/login-client.tsx (4)
12-30: Well-defined interfaces for authentication providersThe interfaces are clear, type-safe, and properly document the structure of provider and environment variable status data.
32-43: Good helper function for provider iconsThe
getProviderIconfunction cleanly centralizes icon rendering logic with good fallback handling.
107-220: Excellent developer experience for configuration issuesThe configuration UI for missing providers is very well implemented with:
- Clear error messaging
- Expandable sections for detailed information
- Visual indicators for set/missing environment variables
- Specific instructions for fixing issues
- Link to documentation
This greatly improves developer experience when setting up authentication.
221-248: Well-implemented login buttons with proper feedbackThe login buttons are properly implemented with:
- Provider-specific icons
- Good state handling
- Toast notifications for feedback during the login process
- Clean routing to the mail interface after successful login
apps/mail/lib/auth-providers.ts (1)
16-66: LGTM - Good work on centralizing provider configurationThe approach of centralizing provider configurations is excellent. It makes the auth system more maintainable and easier to extend with new providers in the future. The structure provides a good balance of type safety and flexibility.
| clientId: process.env.GOOGLE_CLIENT_ID!, | ||
| clientSecret: process.env.GOOGLE_CLIENT_SECRET!, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add runtime safety checks for environment variables
The non-null assertions (!) assume the environment variables will always be present, which could lead to runtime errors. Consider adding proper runtime validation to handle potential missing values gracefully.
- clientId: process.env.GOOGLE_CLIENT_ID!,
- clientSecret: process.env.GOOGLE_CLIENT_SECRET!,
+ clientId: process.env.GOOGLE_CLIENT_ID || '',
+ clientSecret: process.env.GOOGLE_CLIENT_SECRET || '',A better approach might be to validate these during application initialization:
if (!process.env.GOOGLE_CLIENT_ID || !process.env.GOOGLE_CLIENT_SECRET) {
throw new Error('Google authentication is not properly configured');
}| clientId: process.env.GITHUB_CLIENT_ID, | ||
| clientSecret: process.env.GITHUB_CLIENT_SECRET, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent null handling across providers
The Google provider uses non-null assertions (!) while GitHub doesn't. This inconsistency might lead to different runtime behaviors. Either apply the same pattern to both providers or use a common approach for handling potentially undefined environment variables.
config: {
- clientId: process.env.GITHUB_CLIENT_ID,
- clientSecret: process.env.GITHUB_CLIENT_SECRET,
+ clientId: process.env.GITHUB_CLIENT_ID || '',
+ clientSecret: process.env.GITHUB_CLIENT_SECRET || '',
},📝 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.
| clientId: process.env.GITHUB_CLIENT_ID, | |
| clientSecret: process.env.GITHUB_CLIENT_SECRET, | |
| }, | |
| config: { | |
| clientId: process.env.GITHUB_CLIENT_ID || '', | |
| clientSecret: process.env.GITHUB_CLIENT_SECRET || '', | |
| }, |
Summary by CodeRabbit
New Features
Refactor
Chores