fix: improve onboarding and error handling#167
Conversation
…ogle OAuth error handling
- Fix unsafe return type in twilio.module.ts - Remove unnecessary condition checks in auth.controller.ts - Fix type definitions in google.strategy.ts - Add eslint-disable comments for console statements - Fix type assertions in auth.controller.ts
- Use 'as unknown as Twilio.Twilio' for mock client type assertion - Add eslint-disable comment for unsafe return type - Fixes TS2740 error for missing properties in mock object
- Add return types to mock Twilio client functions - Replace || with ?? operators in auth.controller.ts - Fix unnecessary optional chain in google.strategy.ts - Add eslint-disable for console.warn in google.strategy.ts - Use non-null assertion for validated profile fields
- Fix unnecessary optional chain error - Add non-null assertion since emails[0] is validated in the check above
- Replace profile?.emails?.[0] with explicit checks - profile parameter is not optional, so optional chain is unnecessary - Fixes @typescript-eslint/no-unnecessary-condition error
…eter - profile is a required parameter, so !profile check is unnecessary - Use profile.emails.length === 0 instead of !profile.emails[0] - Destructure name and emails directly from profile after validation - Fixes @typescript-eslint/no-unnecessary-condition error
- Add apiVersion to Stripe initialization for consistency - Fix street address parsing for 3-part and 4-part address formats - Add explicit demo step ID check (step 6) for skip action - Improve address parsing error message to show missing fields
- Create unified app-config.ts utility for frontend URL and CORS origin - Support multiple environment variable names for flexibility: - APP_URL, FRONTEND_URL, NEXT_PUBLIC_APP_URL (for frontend URL) - CORS_ORIGIN, FRONTEND_URL, APP_URL (for CORS origin) - Update all files to use centralized config functions - Improve compatibility with different environment variable naming conventions Files updated: - src/utils/app-config.ts (new) - src/main.ts - src/modules/auth/auth.controller.ts - src/modules/stripe/stripe.service.ts - src/modules/google-calendar/calendar-oauth.controller.ts
- Use only APP_URL for frontend URL (remove FRONTEND_URL, NEXT_PUBLIC_APP_URL) - Use only CORS_ORIGIN for CORS (remove FRONTEND_URL, APP_URL fallbacks) - Simplify code by using single environment variable names
- Add import statement for getAppUrl from @/utils/app-config - Fixes TypeScript error: Cannot find name 'getAppUrl'
- Add forgot-password endpoint - Add reset-password endpoint - Add ResetPasswordDto - Update auth service with password reset logic
There was a problem hiding this comment.
Pull request overview
This PR improves onboarding and error handling by introducing centralized configuration utilities, enhancing address parsing with multiple regex patterns and manual fallback logic, improving OAuth error handling, and allowing graceful degradation in development when optional service credentials are missing.
Key Changes
- Centralized frontend URL and CORS origin configuration through new utility functions in
app-config.ts - Enhanced Australian address parsing with multiple regex patterns and comprehensive manual parsing fallback
- Improved error handling for Google OAuth with profile validation and redirect error states
- Added graceful degradation for Stripe, Google OAuth, and Twilio services in development mode when credentials are missing
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/app-config.ts | New utility module providing centralized getAppUrl() and getCorsOrigin() functions with development fallbacks |
| src/modules/stripe/stripe.service.ts | Refactored to use getAppUrl() utility and added development mode handling with dummy Stripe credentials |
| src/modules/onboarding/onboarding.service.ts | Enhanced address parsing with three regex patterns, manual parsing fallback, improved error messages, and demo step skip handling |
| src/modules/health/health.controller.ts | Formatting improvements for API documentation decorators (multi-line strings) |
| src/modules/google-calendar/calendar-oauth.controller.ts | Updated to use getAppUrl() utility instead of direct environment variable access |
| src/modules/company/schema/company.schema.ts | Removed trailing whitespace |
| src/modules/auth/strategies/google.strategy.ts | Added development mode handling for missing OAuth credentials and profile validation |
| src/modules/auth/dto/reset-password.dto.ts | Removed trailing whitespace |
| src/modules/auth/auth.service.ts | Reorganized imports (moved process import to proper position) |
| src/modules/auth/auth.controller.ts | Added OAuth error handling with early returns, improved type safety, and formatting improvements for forgot/reset password endpoints |
| src/main.ts | Updated to use getCorsOrigin() utility for CORS configuration |
| src/lib/twilio/twilio.module.ts | Added development mode handling with mock Twilio client when credentials are missing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Format: "Street, Suburb, State, Postcode" (4 parts) or more | ||
| // For 4 parts: parts[0] is street, parts[1] is suburb | ||
| // For more parts: join all but last 3 as street |
There was a problem hiding this comment.
The manual parsing logic for the case where state and postcode are separated (4+ parts) has an inconsistency. When there are 4 parts, parts[parts.length - 3] is parts[1], which is correct for the suburb. However, when there are more than 4 parts (e.g., 5 parts like "Unit 10, 123 Main St, Sydney, NSW, 2000"), parts[parts.length - 3] would be parts[2] (Sydney), but the street should be "Unit 10, 123 Main St" (parts 0-1). The current logic uses parts.slice(0, -3).join(', ') which would give "Unit 10, 123 Main St", but then suburb would be parts[2] which is "Sydney" - this is actually correct. However, the comment says "For more parts: join all but last 3 as street" which would be inconsistent with how the 3-part case works above (lines 68-70). The logic appears correct but the comment could be clearer about the indexing.
| const mockClient = { | ||
| calls: { | ||
| create: (): Promise<never> => | ||
| Promise.reject(new Error('Twilio not configured')), | ||
| list: (): Promise<never[]> => Promise.resolve([]), | ||
| }, | ||
| messages: { | ||
| create: (): Promise<never> => | ||
| Promise.reject(new Error('Twilio not configured')), | ||
| list: (): Promise<never[]> => Promise.resolve([]), | ||
| }, | ||
| }; | ||
|
|
||
| return mockClient as unknown as Twilio.Twilio; |
There was a problem hiding this comment.
The mock Twilio client provides incomplete type coverage. The Twilio.Twilio type includes many more properties and methods beyond just calls and messages (e.g., accounts, addresses, applications, etc.). Code that attempts to use other Twilio features will fail at runtime even though TypeScript won't catch it due to the type assertion. Consider either implementing a more complete mock object or using a proper mocking library, or at minimum document this limitation clearly.
| const parts = field.split('.'); | ||
| if (parts.length >= 2 && parts[1]) { | ||
| const key = parts[1]; | ||
| await this.userService.patch(userId, { [key]: answer.trim() }); |
There was a problem hiding this comment.
When saving user fields at line 219, answer.trim() is called without first checking if the field is empty (which is handled at lines 202-205). If the demo step logic allows an empty field to reach this code path, calling trim() on an empty answer could result in saving an empty string to user fields. Consider adding a check to ensure answer is not empty before calling userService.patch, or ensuring the demo step logic prevents this code from executing for empty fields.
| await this.userService.patch(userId, { [key]: answer.trim() }); | |
| const trimmedAnswer = answer?.trim(); | |
| if (trimmedAnswer) { | |
| await this.userService.patch(userId, { [key]: trimmedAnswer }); | |
| } |
| // Validate profile data - profile.emails and profile.name are optional | ||
| if (!profile.emails || profile.emails.length === 0 || !profile.name) { | ||
| done(new UnauthorizedException('Invalid Google profile data'), false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The validation logic incorrectly rejects profiles where name.givenName or name.familyName is missing. The check on line 65 verifies that profile.name exists, but doesn't account for the fact that name.givenName and name.familyName are optional fields (as indicated by the type definition). However, lines 75-76 correctly handle these optional fields with nullish coalescing operators. The validation should only verify that profile.name exists, not require both givenName and familyName to be present. Consider changing the validation to check only for emails, since names can legitimately be missing from some Google profiles.
| res.redirect( | ||
| `${frontendUrl}/auth/callback?user=${encodeURIComponent(JSON.stringify(safeUser))}&csrfToken=${encodeURIComponent(csrfToken)}`, | ||
| ); |
There was a problem hiding this comment.
The CSRF token is being included in both an httpOnly cookie (line 217) and as a query parameter in the redirect URL (line 228). This creates a security issue: the CSRF token should not be exposed in the URL where it could be logged in browser history, server logs, or referrer headers. The httpOnly cookie approach is secure, but including it in the URL query parameter defeats this protection. Consider removing the csrfToken from the query parameters and relying solely on the cookie-based approach.
| // Ensure all required fields are present after parsing | ||
| const streetAddress = update.$set['answers.user.address.streetAddress']; | ||
| const suburb = update.$set['answers.user.address.suburb']; | ||
| const state = update.$set['answers.user.address.state']; | ||
| const postcode = update.$set['answers.user.address.postcode']; | ||
|
|
||
| // Collect missing fields for detailed error message | ||
| const missingFields: string[] = []; | ||
| if (!streetAddress) missingFields.push('streetAddress'); | ||
| if (!suburb) missingFields.push('suburb'); | ||
| if (!state) missingFields.push('state'); | ||
| if (!postcode) missingFields.push('postcode'); | ||
|
|
||
| if (missingFields.length > 0) { | ||
| throw new BadRequestException( | ||
| `Address parsing failed: missing required fields - ${missingFields.join(', ')}`, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
The address validation at lines 232-242 checks if the parsed address fields are missing after parsing, but this validation only runs when field === 'user.address.full'. However, if the validator at line 40 throws an exception (line 108), this validation code is never reached. This means the detailed error message about missing fields will never be shown to users - they'll only see the generic parsing error from line 109. Consider either removing this redundant validation or restructuring the validator to not throw exceptions and let this validation provide the error messages.
| // Ensure all required fields are present after parsing | |
| const streetAddress = update.$set['answers.user.address.streetAddress']; | |
| const suburb = update.$set['answers.user.address.suburb']; | |
| const state = update.$set['answers.user.address.state']; | |
| const postcode = update.$set['answers.user.address.postcode']; | |
| // Collect missing fields for detailed error message | |
| const missingFields: string[] = []; | |
| if (!streetAddress) missingFields.push('streetAddress'); | |
| if (!suburb) missingFields.push('suburb'); | |
| if (!state) missingFields.push('state'); | |
| if (!postcode) missingFields.push('postcode'); | |
| if (missingFields.length > 0) { | |
| throw new BadRequestException( | |
| `Address parsing failed: missing required fields - ${missingFields.join(', ')}`, | |
| ); | |
| } | |
| const streetAddress = update.$set['answers.user.address.streetAddress']; | |
| const suburb = update.$set['answers.user.address.suburb']; | |
| const state = update.$set['answers.user.address.state']; | |
| const postcode = update.$set['answers.user.address.postcode']; |
Changes
Onboarding Improvements
Environment Variables
Other Fixes
Environment Variables (No Changes Required)