Conversation
|
@sergio-jva is attempting to deploy a commit to the Zero Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request refines the user settings management across both server-side actions and client-side pages. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as Settings UI
participant SS as saveUserSettings
participant DB as Database
participant TN as Toast Notification
U->>UI: Submit updated settings
UI->>SS: Call saveUserSettings(partialSettings)
SS->>DB: Retrieve existing settings
DB-->>SS: Return current settings
SS->>SS: Merge defaults, existing & partial settings
SS->>SS: Validate merged settings
SS->>DB: Update/insert validated settings
DB-->>SS: Confirm update
SS-->>UI: Return validated settings
UI->>TN: Display success message
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (
|
sergioxro
force-pushed
the
feat/persit-all-settings
branch
3 times, most recently
from
025edb5 to
e94de24
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/mail/lib/auth.ts (1)
189-192: 🛠️ Refactor suggestionCheck Structure of User Settings Merge with Timezone
In the user settings insertion logic,
defaultUserSettingsis merged with a standalonetimezoneproperty at the top level. However, the default JSON structure (as defined in the SQL migration) nests the timezone under the"general"key. For consistency and to avoid potential schema mismatches, consider merging the timezone into thegeneralobject. For example:- settings: { - ...defaultUserSettings, - timezone, - }, + settings: { + ...defaultUserSettings, + general: { + ...defaultUserSettings.general, + timezone, + }, + },This change will ensure that the resulting settings object conforms to the expected nested structure.
🧹 Nitpick comments (4)
packages/db/src/user_settings.ts (1)
15-17: Simple appearance settings schemaThe appearance schema is concise and focuses on inbox type customization. As the application grows, consider expanding this schema to include other appearance-related settings such as theme preferences or layout options.
apps/mail/app/(routes)/settings/notifications/page.tsx (3)
75-84: Missing translation keys in UI text.Unlike other settings pages, this component uses hardcoded English strings instead of translation keys with the
t()function.Apply this diff to ensure consistent internationalization:
- title="Notifications" - description="Choose what notifications you want to receive." + title={t('pages.settings.notifications.title')} + description={t('pages.settings.notifications.description')}And lower in the code:
- {isSaving ? 'Saving...' : 'Save Changes'} + {isSaving ? t('common.actions.saving') : t('common.actions.saveChanges')}
98-117: Missing translations in form labels and descriptions.The form labels and descriptions aren't using internationalization.
Apply this diff to ensure consistent internationalization:
- <FormLabel>New Mail Notifications</FormLabel> + <FormLabel>{t('pages.settings.notifications.newMailNotifications')}</FormLabel> <Select onValueChange={field.onChange} defaultValue={field.value}> <FormControl> <SelectTrigger className="w-[240px]"> <Bell className="mr-2 h-4 w-4" /> <SelectValue placeholder="Select notification level" /> </SelectTrigger> </FormControl> <SelectContent> - <SelectItem value="none">None</SelectItem> - <SelectItem value="important">Important Only</SelectItem> - <SelectItem value="all">All Messages</SelectItem> + <SelectItem value="none">{t('pages.settings.notifications.none')}</SelectItem> + <SelectItem value="important">{t('pages.settings.notifications.importantOnly')}</SelectItem> + <SelectItem value="all">{t('pages.settings.notifications.allMessages')}</SelectItem> </SelectContent> </Select> <FormDescription> - Choose which messages you want to receive notifications for + {t('pages.settings.notifications.newMailNotificationsDescription')} </FormDescription>
123-131: Missing translations in marketing communications settings.The form labels and descriptions for marketing communications aren't using internationalization.
Apply this diff to ensure consistent internationalization:
<FormItem className="flex items-center justify-between rounded-lg border p-4"> <div className="space-y-0.5"> - <FormLabel className="text-base">Marketing Communications</FormLabel> - <FormDescription>Receive updates about new features</FormDescription> + <FormLabel className="text-base">{t('pages.settings.notifications.marketingCommunications')}</FormLabel> + <FormDescription>{t('pages.settings.notifications.marketingCommunicationsDescription')}</FormDescription> </div> <FormControl> <Switch checked={field.value} onCheckedChange={field.onChange} /> </FormControl> </FormItem>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/mail/actions/settings.ts(2 hunks)apps/mail/app/(routes)/settings/appearance/page.tsx(2 hunks)apps/mail/app/(routes)/settings/general/page.tsx(2 hunks)apps/mail/app/(routes)/settings/notifications/page.tsx(2 hunks)apps/mail/app/(routes)/settings/security/page.tsx(2 hunks)apps/mail/hooks/use-settings.ts(2 hunks)apps/mail/lib/auth.ts(1 hunks)packages/db/migrations/0019_eager_bucky.sql(1 hunks)packages/db/migrations/meta/0019_snapshot.json(1 hunks)packages/db/migrations/meta/_journal.json(1 hunks)packages/db/package.json(1 hunks)packages/db/src/schema.ts(1 hunks)packages/db/src/user_settings.ts(1 hunks)packages/db/src/user_settings_default.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/db/src/user_settings_default.ts
🧰 Additional context used
🧬 Code Definitions (6)
apps/mail/hooks/use-settings.ts (2)
packages/db/src/user_settings.ts (1)
defaultUserSettings(33-38)apps/mail/lib/timezones.ts (1)
getBrowserTimezone(1-1)
apps/mail/actions/settings.ts (2)
packages/db/src/user_settings.ts (2)
UserSettings(31-31)defaultUserSettings(33-38)packages/db/src/schema.ts (1)
userSettings(107-116)
apps/mail/app/(routes)/settings/notifications/page.tsx (3)
packages/db/src/user_settings.ts (1)
notificationSettingsSchema(19-22)apps/mail/hooks/use-settings.ts (1)
useSettings(7-41)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
apps/mail/app/(routes)/settings/security/page.tsx (3)
packages/db/src/user_settings.ts (1)
securitySettingsSchema(10-13)apps/mail/hooks/use-settings.ts (1)
useSettings(7-41)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
apps/mail/app/(routes)/settings/appearance/page.tsx (3)
packages/db/src/user_settings.ts (1)
appearanceSettingsSchema(15-17)apps/mail/hooks/use-settings.ts (1)
useSettings(7-41)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
apps/mail/app/(routes)/settings/general/page.tsx (2)
packages/db/src/user_settings.ts (1)
generalSettingsSchema(3-8)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
🔇 Additional comments (41)
packages/db/migrations/meta/_journal.json (1)
137-144: New Journal Entry Added: Verify Migration OrderThe new journal entry with
idx19 and tag"0019_eager_bucky"is correctly appended following the sequential order. Please double‐check that its timestamp and ordering are in line with your overall migration history.packages/db/src/schema.ts (1)
2-2: Updated Import for Default User SettingsThe import for
defaultUserSettingsnow correctly references the new module (@zero/db/user_settings), ensuring consistency with the updated project structure.packages/db/migrations/0019_eager_bucky.sql (1)
1-1: SQL Migration for Default SettingsThe SQL migration appropriately alters the
settingscolumn’s default in themail0_user_settingstable using a comprehensive JSONB structure. Ensure that this default structure is fully compatible with downstream application logic.packages/db/package.json (1)
22-26: Updated Package Exports for User SettingsThe changes in the
"exports"section—removing the deprecated export and adding"./user_settings": "./src/user_settings.ts"—align with the restructuring of the user settings module. This update ensures that consumers reference the new module location.apps/mail/lib/auth.ts (1)
5-5: Updated Import for Default User SettingsThe import statement at line 5 now correctly sources
defaultUserSettingsfrom@zero/db/user_settingsinstead of the deprecated module. This update promotes consistency across the codebase.apps/mail/hooks/use-settings.ts (2)
4-4: Appropriate change to import sourceThe import source has been updated to use the consolidated user settings module, which aligns with the PR objective of having a unified approach to configuration management.
19-24: Well-structured default settings with browser timezoneThe restructuring of default settings to include a nested
generalproperty properly aligns with the new schema design. This ensures that when a user has no saved settings, they still get appropriate defaults with their browser's timezone, improving user experience.packages/db/src/user_settings.ts (5)
3-8: Well-defined general settings schemaThe schema correctly defines language, timezone, and content preferences with appropriate default values. This structure provides a solid foundation for persisting user preferences.
10-13: Security settings with sensible defaultsThe security schema uses appropriate defaults - two-factor authentication is disabled by default (opt-in) while login notifications are enabled (opt-out), which follows good security practices.
19-22: Comprehensive notification preferencesThe notification settings include both email notifications and marketing preferences, covering the essentials for user communication preferences.
24-29: Well-organized composite schemaThe user settings schema follows a logical organization by grouping related settings into separate categories, making the configuration easier to manage and extend in the future.
33-38: Effective default settings implementationUsing
parse({})to generate default values is an elegant approach that leverages Zod's schema validation while ensuring all default values are applied consistently across the application.apps/mail/actions/settings.ts (4)
3-3: Updated import to use the new settings moduleThe import has been correctly updated to use the new consolidated user settings module, ensuring consistency across the application.
53-53: Improved function signature for flexible updatesChanging the parameter type from
UserSettingstoPartial<UserSettings>significantly improves the API by allowing partial updates to user settings, which is more flexible and user-friendly.
58-74: Robust settings merging strategyThe implementation uses a solid three-layer merging strategy:
- Start with default settings as the baseline
- Apply existing user settings from the database
- Apply the newly submitted partial settings
This ensures that settings are never lost and new partial updates are properly integrated, creating a seamless user experience when updating preferences.
93-93: Enhanced return valueReturning the validated settings object rather than just a success message provides more value to callers, allowing them to use the updated settings without making additional database queries.
apps/mail/app/(routes)/settings/general/page.tsx (5)
33-35: Good reuse of schema definitionUsing the imported schema from the central settings module rather than defining a local schema ensures consistency between validation rules and reduces duplication.
55-55: Updated form reset logic for nested structureThe form reset logic has been correctly updated to use
settings.general, aligning with the new nested settings structure.
63-64: Defensive programming with settings checkAdding a validation check for settings availability before attempting to save is a good defensive programming practice that prevents potential runtime errors.
65-66: Proper structure for settings updateThe submission logic now correctly wraps the form values in a
generalproperty, matching the expected structure for partial updates to the settings schema.
74-76: Improved toast notification logicMoving the success toast inside the else condition avoids showing duplicate success messages when the language changes (which already shows its own toast).
apps/mail/app/(routes)/settings/appearance/page.tsx (5)
3-3: Updated imports to support the new settings management system.The imports now include necessary hooks, actions, and components for settings management with proper validation and user feedback.
Also applies to: 11-16
19-19: Good schema refactoring.Replacing the local schema definition with the imported
appearanceSettingsSchemaimproves maintainability and ensures consistency across the app.
23-23: Enhanced state management with useSettings hook.Using the settings hook provides access to user settings and the mutate function for updating them.
33-37: Form now properly syncs with saved settings.The useEffect hook ensures the form displays the current user settings whenever they're available, providing a better user experience.
39-54: Improved form submission with error handling.The onSubmit function now includes:
- Proper error checking for settings availability
- Integration with the saveUserSettings action
- User feedback via toast notifications
- Client-side state updates with SWR's mutate
- Graceful error handling with fallback state revalidation
This is a significant improvement over the previous implementation, providing better UX and error resilience.
apps/mail/app/(routes)/settings/security/page.tsx (6)
18-23: Updated imports to support the new settings management system.The imports now include necessary hooks, actions, and components for settings management with proper validation and user feedback.
25-25: Good schema refactoring.Replacing the local schema definition with the imported
securitySettingsSchemaimproves maintainability and ensures consistency across the app.
29-29: Enhanced state management with useSettings hook.Using the settings hook provides access to user settings and the mutate function for updating them.
40-44: Form now properly syncs with saved settings, but has a type issue.The useEffect hook ensures the form displays the current user settings, but there's an unresolved type error that needs fixing.
Please address the TODO comment about the type error. This might be caused by a mismatch between the form schema type and the security settings type from the API.
46-61: Improved form submission with error handling.The onSubmit function now includes:
- Proper error checking for settings availability
- Integration with the saveUserSettings action
- User feedback via toast notifications
- Client-side state updates with SWR's mutate
- Graceful error handling with fallback state revalidation
This is a significant improvement over the previous implementation.
64-64: Removed unnecessary empty line.apps/mail/app/(routes)/settings/notifications/page.tsx (6)
24-30: Updated imports to support the new settings management system.The imports now include necessary hooks, actions, and components for settings management with proper validation and user feedback.
32-32: Good schema refactoring.Replacing the local schema definition with the imported
notificationSettingsSchemaimproves maintainability and ensures consistency across the app.
36-37: Enhanced state management with useSettings hook and translations.Using the settings hook provides access to user settings and the mutate function for updating them, while the translations hook enables internationalization.
47-51: Form now properly syncs with saved settings.The useEffect hook ensures the form displays the current user settings whenever they're available, providing a better user experience.
53-68: Improved form submission with error handling.The onSubmit function now includes:
- Proper error checking for settings availability
- Integration with the saveUserSettings action
- User feedback via toast notifications
- Client-side state updates with SWR's mutate
- Graceful error handling with fallback state revalidation
This is a significant improvement over the previous implementation.
71-71: Removed unnecessary empty line.packages/db/migrations/meta/0019_snapshot.json (3)
1-690: Database schema now includes comprehensive user settings support.The database schema snapshot defines a structured approach to storing user settings, with:
- A dedicated
mail0_user_settingstable with auser_idforeign key to the users table- A
settingscolumn using JSONB to store nested settings objects- A well-defined default value that includes all settings categories (general, security, appearance, notification)
- Proper constraints including a unique constraint on
user_idto ensure one settings record per userThis provides a solid foundation for the user settings persistence feature.
584-584: Default settings JSON structure aligns with schema definitions.The default JSONB value correctly includes all the settings categories with their default values, matching the schema definitions imported by the settings pages:
general: language, timezone, dynamicContent, externalImagessecurity: twoFactorAuth, loginNotificationsappearance: inboxTypenotification: newMailNotifications, marketingCommunicationsThis ensures that new users will have complete and valid settings from the start.
563-628: User settings table has appropriate foreign key and unique constraints.The
mail0_user_settingstable includes:
- A foreign key relationship to the user table, ensuring referential integrity
- A unique constraint on
user_id, enforcing one settings record per user- Proper timestamps for auditing when settings are created and updated
These constraints help maintain data integrity in the database.
sergioxro
force-pushed
the
feat/persit-all-settings
branch
from
e94de24 to
839af20
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/mail/app/(routes)/settings/security/page.tsx (1)
40-44: LGTM, but consider adding settings.security dependency.The useEffect implementation correctly resets the form with current security settings when they become available. This ensures the form displays the latest user preferences.
- }, [form, settings]); + }, [form, settings, settings?.security]);apps/mail/app/(routes)/settings/notifications/page.tsx (2)
47-51: LGTM: Proper form initialization from settings.The useEffect hook correctly resets the form with current notification settings when they become available. This ensures the form displays up-to-date user preferences.
- }, [form, settings]); + }, [form, settings, settings?.notification]);
79-81: Consider updating the Reset to Defaults functionality.The Reset to Defaults button currently resets to the form's defaultValues rather than the application's actual defaults. This may not properly restore the system defaults if they differ from the form's initial values.
- <Button type="button" variant="outline" onClick={() => form.reset()}> + <Button + type="button" + variant="outline" + onClick={() => { + const defaultSettings = { + newMailNotifications: 'all', + marketingCommunications: false, + }; + form.reset(defaultSettings); + }} + > Reset to Defaults </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/mail/actions/ai-reply.ts(1 hunks)apps/mail/actions/settings.ts(2 hunks)apps/mail/app/(routes)/settings/appearance/page.tsx(2 hunks)apps/mail/app/(routes)/settings/general/page.tsx(2 hunks)apps/mail/app/(routes)/settings/notifications/page.tsx(2 hunks)apps/mail/app/(routes)/settings/security/page.tsx(2 hunks)apps/mail/hooks/use-settings.ts(2 hunks)apps/mail/lib/auth.ts(1 hunks)packages/db/migrations/0019_eager_bucky.sql(1 hunks)packages/db/migrations/meta/0019_snapshot.json(1 hunks)packages/db/migrations/meta/_journal.json(1 hunks)packages/db/package.json(1 hunks)packages/db/src/schema.ts(1 hunks)packages/db/src/user_settings.ts(1 hunks)packages/db/src/user_settings_default.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/db/src/user_settings_default.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/db/src/schema.ts
- apps/mail/hooks/use-settings.ts
- packages/db/migrations/0019_eager_bucky.sql
- packages/db/package.json
- apps/mail/lib/auth.ts
- packages/db/migrations/meta/_journal.json
- apps/mail/app/(routes)/settings/general/page.tsx
🧰 Additional context used
🧬 Code Definitions (5)
apps/mail/actions/ai-reply.ts (1)
packages/db/src/schema.ts (1)
userSettings(108-117)
apps/mail/app/(routes)/settings/appearance/page.tsx (3)
packages/db/src/user_settings.ts (1)
appearanceSettingsSchema(16-18)apps/mail/hooks/use-settings.ts (1)
useSettings(7-41)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
apps/mail/app/(routes)/settings/security/page.tsx (3)
packages/db/src/user_settings.ts (1)
securitySettingsSchema(11-14)apps/mail/hooks/use-settings.ts (1)
useSettings(7-41)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
apps/mail/actions/settings.ts (2)
packages/db/src/user_settings.ts (2)
UserSettings(32-32)defaultUserSettings(34-39)packages/db/src/schema.ts (1)
userSettings(108-117)
apps/mail/app/(routes)/settings/notifications/page.tsx (3)
packages/db/src/user_settings.ts (1)
notificationSettingsSchema(20-23)apps/mail/hooks/use-settings.ts (1)
useSettings(7-41)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
🔇 Additional comments (26)
apps/mail/actions/ai-reply.ts (1)
49-49: Settings structure update correctly implementedThe code now accesses
customPromptthroughuserSettings?.general.customPromptinstead of directly fromuserSettings, matching the new nested settings structure. Good use of optional chaining and fallback to empty string.packages/db/src/user_settings.ts (7)
3-9: Well-structured general settings schema with appropriate defaultsThe schema nicely defines general settings with proper types and default values for each field. The
customPromptfield allows for empty strings by default.
11-14: Clean security settings schemaGood definition of security-related settings with appropriate boolean types and sensible defaults.
16-18: Good use of enum for inbox typeThe schema correctly uses Zod's enum type to restrict possible values for inbox type with a sensible default.
20-23: Comprehensive notification settings schemaThe schema properly defines notification preferences using appropriate types. Good use of enum for notification levels and boolean for marketing communications.
25-30: Well-organized composite settings schemaThe main schema nicely combines all individual schemas into a comprehensive structure, making it easy to validate the entire settings object or just parts of it.
32-32: Type inference ensures type safetyGood practice using Zod's type inference to create a TypeScript type from the schema, ensuring type consistency throughout the application.
34-39: Clever default settings initializationSmart approach using schema parsing with empty objects to automatically get all default values defined in the schemas. This ensures defaults are consistent and validated.
apps/mail/actions/settings.ts (7)
3-3: Updated imports for new settings structureCorrectly importing the new types and schemas from the dedicated user settings module.
53-53: Improved function signature for partial updatesThe function now accepts
Partial<UserSettings>instead ofUserSettings, allowing for granular updates to specific parts of the settings. This is a great improvement for flexibility.
58-64: Proper retrieval of existing settingsThe code correctly queries the database for existing settings before applying updates.
65-70: Robust settings merging strategyThe implementation uses a three-level merging approach that:
- Starts with default settings as the base
- Applies existing settings (if any)
- Applies the new partial settings
This ensures all required fields exist and preserves existing values for fields not included in the partial update.
72-73: Validation before database operationsGood practice to validate the merged settings before saving to ensure data integrity.
75-91: Handles both update and insert scenariosThe function properly handles both cases where settings already exist (update) or need to be created (insert).
93-93: Returning validated settings is useful for clientsReturning the complete validated settings object allows clients to use the updated data without making another request.
apps/mail/app/(routes)/settings/appearance/page.tsx (5)
11-16: Good imports for enhanced functionalityAdded imports for hooks, actions, and utilities needed for the improved settings management functionality.
19-19: Leveraging the centralized schema definitionUsing the imported
appearanceSettingsSchemaensures consistency between client and server validation.
23-23: Using the settings hook for state managementProperly utilizing the settings hook to access current settings and the mutate function for local state updates.
33-37: Form synchronization with current settingsThe
useEffecthook ensures the form is populated with current appearance settings when they become available, providing a good user experience by displaying current values.
39-54: Comprehensive form submission with error handlingThe submit function implements a complete workflow:
- Checks that settings are available
- Updates only the appearance section using partial settings
- Optimistically updates the UI
- Provides user feedback through toast notifications
- Properly handles errors with logging and user notifications
- Ensures loading state is reset in all cases
This provides a smooth user experience with appropriate feedback.
apps/mail/app/(routes)/settings/security/page.tsx (2)
46-61: Well-implemented error handling and settings persistence.The onSubmit function now properly:
- Validates settings availability
- Persists security settings through saveUserSettings
- Updates local state via mutate
- Provides user feedback with toast notifications
- Handles error cases gracefully
This implementation aligns perfectly with the PR objective of persisting all user configurations.
25-25: Improved schema management through centralized definition.Using the imported securitySettingsSchema improves maintainability by centralizing the schema definition. This makes the code more robust against schema changes.
apps/mail/app/(routes)/settings/notifications/page.tsx (2)
53-68: Well-implemented error handling and settings persistence.The onSubmit function successfully:
- Validates settings availability before proceeding
- Persists notification settings using saveUserSettings
- Updates local state with mutate
- Provides clear user feedback with toast notifications
- Handles error cases with appropriate error messages
This implementation aligns with the PR objective of persisting all user configurations.
32-32: Improved schema management through centralized definition.Using the imported notificationSettingsSchema improves maintainability by centralizing the schema definition, making the code more resistant to schema changes.
packages/db/migrations/meta/0019_snapshot.json (2)
563-628: Well-structured database schema for user settings.The mail0_user_settings table definition is well-designed with:
- A properly structured jsonb settings column containing all setting categories
- Appropriate defaults for all settings
- A foreign key relationship to the mail0_user table
- A unique constraint on user_id to ensure one settings record per user
This database structure effectively supports the PR objective of persisting all user configurations.
580-585: Comprehensive default settings structure.The default JSON structure includes all necessary setting categories:
- general: language, timezone, content preferences
- security: two-factor authentication, login notifications
- appearance: inbox type
- notification: mail notifications, marketing communications
This matches the frontend schemas and provides a complete foundation for user settings.
sergioxro
force-pushed
the
feat/persit-all-settings
branch
from
839af20 to
a727213
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/db/src/user_settings.ts (3)
25-30: Consider adding "notifications" instead of "notification" for consistencyThe schema property name "notification" doesn't match the plural form used in the schema name "notificationSettingsSchema". Consider renaming to "notifications" for consistency.
export const userSettingsSchema = z.object({ general: generalSettingsSchema, security: securitySettingsSchema, appearance: appearanceSettingsSchema, - notification: notificationSettingsSchema, + notifications: notificationSettingsSchema, });
34-39: Update defaultUserSettings to match the userSettingsSchema property namingIf you rename the property in the userSettingsSchema, make sure to update it here as well.
export const defaultUserSettings: UserSettings = { general: generalSettingsSchema.parse({}), security: securitySettingsSchema.parse({}), appearance: appearanceSettingsSchema.parse({}), - notification: notificationSettingsSchema.parse({}), + notifications: notificationSettingsSchema.parse({}), };
3-9: Add documentation for generalSettingsSchema propertiesConsider adding JSDoc comments to describe what each setting controls, especially for properties like
dynamicContentandcustomPromptwhich might not be self-explanatory.apps/mail/actions/settings.ts (1)
68-69: Consider safer type handling for existing settingsThe type assertion
(existingResult?.settings as Partial<UserSettings> || {})assumes the database structure matches the TypeScript types. Consider adding validation to ensure this cast is safe.const mergedSettings = { ...defaultUserSettings, - ...(existingResult?.settings as Partial<UserSettings> || {}), + ...(existingResult?.settings ? validateSettings(existingResult.settings) : {}), ...partialSettings, }; // Then remove or modify the validateSettings call below as neededapps/mail/app/(routes)/settings/security/page.tsx (1)
71-76: Consider adding confirmation dialog for account deletionThe "Delete Account" button is potentially destructive. Consider adding a confirmation dialog before allowing the user to delete their account.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/mail/actions/ai-reply.ts(1 hunks)apps/mail/actions/settings.ts(2 hunks)apps/mail/app/(routes)/settings/appearance/page.tsx(2 hunks)apps/mail/app/(routes)/settings/general/page.tsx(2 hunks)apps/mail/app/(routes)/settings/notifications/page.tsx(2 hunks)apps/mail/app/(routes)/settings/security/page.tsx(2 hunks)apps/mail/hooks/use-settings.ts(2 hunks)apps/mail/lib/auth.ts(1 hunks)packages/db/migrations/0019_eager_bucky.sql(1 hunks)packages/db/migrations/meta/0019_snapshot.json(1 hunks)packages/db/migrations/meta/_journal.json(1 hunks)packages/db/package.json(1 hunks)packages/db/src/schema.ts(1 hunks)packages/db/src/user_settings.ts(1 hunks)packages/db/src/user_settings_default.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/db/src/user_settings_default.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/db/migrations/0019_eager_bucky.sql
- packages/db/src/schema.ts
- apps/mail/actions/ai-reply.ts
- apps/mail/lib/auth.ts
- packages/db/package.json
- apps/mail/hooks/use-settings.ts
- packages/db/migrations/meta/_journal.json
- apps/mail/app/(routes)/settings/general/page.tsx
🧰 Additional context used
🧬 Code Definitions (4)
apps/mail/app/(routes)/settings/appearance/page.tsx (3)
packages/db/src/user_settings.ts (1)
appearanceSettingsSchema(16-18)apps/mail/hooks/use-settings.ts (1)
useSettings(7-41)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
apps/mail/actions/settings.ts (2)
packages/db/src/user_settings.ts (2)
UserSettings(32-32)defaultUserSettings(34-39)packages/db/src/schema.ts (1)
userSettings(108-117)
apps/mail/app/(routes)/settings/security/page.tsx (3)
packages/db/src/user_settings.ts (1)
securitySettingsSchema(11-14)apps/mail/hooks/use-settings.ts (1)
useSettings(7-41)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
apps/mail/app/(routes)/settings/notifications/page.tsx (3)
packages/db/src/user_settings.ts (1)
notificationSettingsSchema(20-23)apps/mail/hooks/use-settings.ts (1)
useSettings(7-41)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
🔇 Additional comments (22)
packages/db/src/user_settings.ts (1)
1-39: Well-structured schema definitions using ZodThe implementation creates a comprehensive and type-safe settings schema system using Zod. This is a good approach for ensuring data validation and providing default values.
apps/mail/actions/settings.ts (4)
3-3: Updated import statements to use the new user_settings moduleThe import statement now correctly imports from the new centralized settings module.
53-53: Great enhancement to support partial settings updatesChanging the function signature to accept
Partial<UserSettings>is a good improvement that allows for more flexible updates.
58-70: Robust implementation of settings merging strategyThe merging strategy correctly prioritizes new settings over existing ones while maintaining default values for unspecified fields.
93-93: Return validated settings for client-side updatesReturning the validated settings is beneficial for updating the client-side state with the server's validated data.
apps/mail/app/(routes)/settings/appearance/page.tsx (4)
16-19: Updated to use the centralized appearance settings schemaUsing the imported
appearanceSettingsSchemafrom the centralized module ensures consistency across the application.
33-37: Improved form synchronization with user settingsThe added useEffect hook ensures the form reflects the current user settings when they become available, improving UX by showing the correct values.
42-52: Enhanced error handling and user feedbackThe updated submission logic now includes proper error handling, user feedback via toast notifications, and optimistic UI updates, which significantly improves the user experience.
35-35: Verify the property path matches the schema structureMake sure the property path
settings.appearancematches the structure from the schema. If you change the schema property from "notification" to "notifications", be sure to update similar references throughout the codebase.apps/mail/app/(routes)/settings/security/page.tsx (5)
23-25: Updated to use the centralized security settings schemaUsing the imported
securitySettingsSchemaensures consistency with the schema definitions across the application.
40-44: Improved form synchronization with user settingsThe added useEffect hook ensures the form reflects the current user settings when they become available, improving UX by showing the correct values.
49-59: Enhanced error handling and user feedbackThe improved submission logic now includes proper error handling, user feedback via toast notifications, and optimistic UI updates through SWR's mutate function.
64-64: Removed unnecessary empty lineGood cleanup of empty line to maintain code cleanliness.
42-42: Verify the property path matches the schema structureMake sure the property path
settings.securitymatches the structure from the schema. If you change the schema property from "notification" to "notifications", be sure to update similar references throughout the codebase.apps/mail/app/(routes)/settings/notifications/page.tsx (5)
24-30: Good structure with appropriate imports.The added imports properly support the enhanced functionality:
useEffectfor form reset logic, schema definitions for validation, settings hooks for state management, and toast for user feedback.
32-32: Well-structured schema integration.Using the imported
notificationSettingsSchemaensures consistency between frontend validation and database schema, facilitating future schema extensions.
36-37: Good use of custom hooks.Destructuring the settings and mutate function from useSettings provides clean access to user settings and a way to update them, while the translation hook supports internationalization.
47-51: Effective form reset implementation.The useEffect correctly resets the form with current notification settings when they become available, ensuring the form always reflects the latest saved settings.
53-68: Comprehensive error handling with clear user feedback.The updated onSubmit function properly validates settings existence, handles errors, and provides user feedback through toast notifications. The error case also triggers a mutate to ensure UI consistency.
packages/db/migrations/meta/0019_snapshot.json (3)
563-628: Well-structured user settings storage schema.The mail0_user_settings table is properly designed with:
- A primary key and foreign key relationship to mail0_user
- A unique constraint on user_id to ensure one settings record per user
- A jsonb column for flexible settings storage with sensible defaults
This implementation aligns perfectly with the PR objective to persist all user configurations.
580-585: Comprehensive default settings configuration.The default JSON structure includes all necessary settings categories (general, security, appearance, notification) with appropriate default values, providing a complete foundation for user settings.
600-624: Proper data integrity constraints.The foreign key relationship to mail0_user ensures referential integrity, while the unique constraint on user_id prevents duplicate settings entries for a single user.
sergioxro
force-pushed
the
feat/persit-all-settings
branch
from
a727213 to
82e254b
Compare
sergioxro
force-pushed
the
feat/persit-all-settings
branch
from
82e254b to
8a7a3e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/db/src/user_settings.ts (1)
34-39: Consider adding documentation for schema exportsWhile the code is self-documenting, adding JSDoc comments for the schemas and the
defaultUserSettingsexport would improve maintainability, especially for other developers who might need to extend these schemas in the future.+/** + * Default user settings object with all schemas initialized with their default values. + * Used as a base for new users and for merging with existing user settings. + */ export const defaultUserSettings: UserSettings = { general: generalSettingsSchema.parse({}), security: securitySettingsSchema.parse({}), appearance: appearanceSettingsSchema.parse({}), notification: notificationSettingsSchema.parse({}), };apps/mail/app/(routes)/settings/appearance/page.tsx (1)
26-31: Consider removing redundant defaultValuesSince you're resetting the form with the current settings in the useEffect hook, these defaultValues might be redundant. However, they serve as a fallback if settings aren't available immediately.
apps/mail/app/(routes)/settings/security/page.tsx (1)
64-64: Remove unnecessary empty lineThis empty line can be removed for better code cleanliness.
-packages/db/migrations/meta/0019_snapshot.json (2)
7-111: Review of Table: public.mail0_account
The definition for themail0_accounttable is well structured. The columns (such asid,account_id,provider_id,user_id, tokens, and timestamps) are clearly defined with appropriate constraints. The foreign key constraint (mail0_account_user_id_mail0_user_id_fk) properly linksuser_idto themail0_usertable, ensuring referential integrity.Consider indexing columns like
user_idif you expect high query volumes on this table.
263-348: Review of Table: public.mail0_note
Themail0_notetable is designed to capture user notes effectively with fields for content, color, and timestamps. One point to note is the use of the column name"order". SinceORDERis a reserved keyword in SQL, this can lead to potential issues in queries or ORM mappings.
Consider renaming this column (e.g., to"order_index") to avoid conflicts with SQL reserved words.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/mail/actions/ai-reply.ts(1 hunks)apps/mail/actions/settings.ts(2 hunks)apps/mail/app/(routes)/settings/appearance/page.tsx(2 hunks)apps/mail/app/(routes)/settings/general/page.tsx(2 hunks)apps/mail/app/(routes)/settings/notifications/page.tsx(2 hunks)apps/mail/app/(routes)/settings/security/page.tsx(2 hunks)apps/mail/hooks/use-settings.ts(2 hunks)apps/mail/lib/auth.ts(1 hunks)packages/db/migrations/0020_wakeful_natasha_romanoff.sql(1 hunks)packages/db/migrations/meta/0019_snapshot.json(1 hunks)packages/db/migrations/meta/0020_snapshot.json(1 hunks)packages/db/migrations/meta/_journal.json(1 hunks)packages/db/package.json(1 hunks)packages/db/src/schema.ts(2 hunks)packages/db/src/user_settings.ts(1 hunks)packages/db/src/user_settings_default.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/db/src/user_settings_default.ts
✅ Files skipped from review due to trivial changes (1)
- packages/db/migrations/0020_wakeful_natasha_romanoff.sql
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/mail/actions/ai-reply.ts
- packages/db/package.json
- packages/db/src/schema.ts
- apps/mail/hooks/use-settings.ts
- apps/mail/actions/settings.ts
- apps/mail/lib/auth.ts
- packages/db/migrations/meta/_journal.json
- apps/mail/app/(routes)/settings/general/page.tsx
- apps/mail/app/(routes)/settings/notifications/page.tsx
🧰 Additional context used
🧬 Code Definitions (2)
apps/mail/app/(routes)/settings/appearance/page.tsx (3)
packages/db/src/user_settings.ts (1)
appearanceSettingsSchema(16-18)apps/mail/hooks/use-settings.ts (1)
useSettings(7-41)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
apps/mail/app/(routes)/settings/security/page.tsx (3)
packages/db/src/user_settings.ts (1)
securitySettingsSchema(11-14)apps/mail/hooks/use-settings.ts (1)
useSettings(7-41)apps/mail/actions/settings.ts (1)
saveUserSettings(53-98)
🔇 Additional comments (19)
packages/db/src/user_settings.ts (1)
1-39: Well-structured settings schema with ZodThe implementation of settings schemas is clean and well-organized. I appreciate the separation into logical categories (general, security, appearance, notification) that makes it easy to understand and extend.
apps/mail/app/(routes)/settings/appearance/page.tsx (3)
18-19: Good use of Zod schema abstractionUsing the imported schema directly removes duplication and ensures consistency between the database and UI layers.
33-37: Good pattern for form initializationUsing useEffect to reset the form when settings are loaded ensures that the UI reflects the current user preferences.
39-55: Improved error handling and feedbackGood implementation of error handling with appropriate feedback using toast notifications. The mutation approach correctly updates the local state while also handling revalidation.
apps/mail/app/(routes)/settings/security/page.tsx (3)
25-25: Good use of schema abstractionDirectly using the imported schema ensures consistency with the database schema and reduces duplication.
40-44: Consistent error handling patternThe useEffect pattern for form initialization matches the implementation in other settings pages, which is good for consistency.
46-62: Well-implemented settings update logicThe error handling, state updates, and user feedback mechanisms are well-implemented. I like the thoughtful approach of returning to the original state if an error occurs during saving.
packages/db/migrations/meta/0020_snapshot.json (3)
579-585: Well-structured default settings in database schemaThe default JSON structure in the settings column aligns perfectly with the schema defined in the user_settings.ts file. This ensures consistency between the code and database layers.
616-624: Good constraint on user_id in settings tableThe unique constraint on user_id ensures that each user can only have one settings record, which is appropriate for this implementation.
601-613: Appropriate foreign key relationshipThe foreign key constraint to the user table ensures data integrity by preventing orphaned settings records.
packages/db/migrations/meta/0019_snapshot.json (9)
1-6: Metadata Header and Migration Information Check
The migration file header correctly specifies a uniqueid, the previous migration identifier (prevId), the schemaversion, and indicates the PostgreSQL dialect. This structured metadata provides clear context for the migration system.
112-210: Review of Table: public.mail0_connection
Themail0_connectiontable includes comprehensive columns for managing connection details, including tokens and expiry information. The foreign key (mail0_connection_user_id_mail0_user_id_fk) enforces a clear relationship with themail0_usertable, which is essential for data consistency.
211-262: Review of Table: public.mail0_early_access
This table is well defined for early access management. It enforces uniqueness on thefalse) for theis_early_accessflag. The schema is clear and maintains data integrity.
349-431: Review of Table: public.mail0_session
The session table includes all necessary fields to manage user sessions—such as the uniquetoken, expiry, and related metadata. The defined foreign key (mail0_session_user_id_mail0_user_id_fk) maintains referential consistency with themail0_usertable. The unique constraint on thetokencolumn is a good practice for security and lookup performance.
432-493: Review of Table: public.mail0_summary
The schema formail0_summaryis clear and captures summary details appropriately including message ID, content, and connectivity information. One observation is that theconnection_idcolumn lacks an explicit foreign key constraint linking it to themail0_connectiontable.Please verify whether this omission is intentional or if a relational link should be enforced.
494-562: Review of Table: public.mail0_user
Themail0_usertable is properly defined with essential user details and a unique constraint ondefault_connection_idfield appears to serve as a pointer to a connection record; however, there is no foreign key constraint enforcing its validity against themail0_connectiontable.Confirm if you intended to leave
default_connection_idunconstrained or if a foreign key relationship should be established.
563-628: Review of Table: public.mail0_user_settings
This table is central to the PR objective of persisting all user configurations. Thesettingscolumn is defined asjsonbwith a comprehensive default value covering general, security, appearance, and notification settings. Moreover, the foreign key and unique constraint onuser_idensure that each user has a single, consistent configuration record.
629-677: Review of Table: public.mail0_verification
Themail0_verificationtable includes necessary fields for handling verification, such as theidentifier,value, and expiration details. While thecreated_atandupdated_atfields are set to be nullable (which might be acceptable depending on the use case), the overall design is straightforward and meets typical verification requirements.
678-690: Global Metadata and Schema Overview
The sections for enums, schemas, sequences, roles, policies, and views are appropriately left empty, indicating no additional custom global definitions are needed at this time. The_metasection correctly encapsulates these aspects, ensuring consistency within the migration snapshot.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/db/migrations/meta/0019_snapshot.json (2)
263-348: Table Schema forpublic.mail0_note& Default Value for ColorThe
mail0_notetable is well-structured with columns for content, order, and timestamps. One potential point for review is the default for thecolorcolumn (lines 291–296), which is set as"'default'". Please verify whether the intention is to have the literal valuedefaultwithout the enclosing single quotes to avoid any unintended double quoting.
563-628: Table Schema forpublic.mail0_user_settings& Default JSON StructureThe
mail0_user_settingstable introduces asettingscolumn of typejsonbwith a detailed default value (line 584) that reflects the new schema for user configurations. This default value is comprehensive; however, consider reviewing the embedded JSON string for proper escaping and long-term maintainability. It might be beneficial to externalize this default JSON structure into a separate constant or file if updates become more frequent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/db/migrations/0019_right_stryfe.sql(1 hunks)packages/db/migrations/meta/0019_snapshot.json(1 hunks)packages/db/migrations/meta/_journal.json(1 hunks)packages/db/src/schema.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/db/migrations/0019_right_stryfe.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/src/schema.ts
- packages/db/migrations/meta/_journal.json
🔇 Additional comments (9)
packages/db/migrations/meta/0019_snapshot.json (9)
1-6: Header Section ConsistencyThe header (lines 1–6) clearly defines the migration ID, previous migration ID, version, dialect, and begins the tables object. Everything looks consistent with the expected migration metadata.
7-111: Table Schema forpublic.mail0_accountThe schema for the
mail0_accounttable is well defined. All columns have appropriate data types and constraints—including a primary key foridand a valid foreign key (lines 91–105) linkinguser_idto themail0_usertable.
112-210: Table Schema forpublic.mail0_connectionThe
mail0_connectiontable is structured correctly with properly defined columns and NOT NULL constraints where needed. The foreign key (lines 191–203) tomail0_userensures referential integrity.
211-262: Table Schema forpublic.mail0_early_accessThe schema for the
mail0_early_accesstable is clear and concise. The unique constraint on theis_early_accessis appropriately set to false.
349-431: Table Schema forpublic.mail0_sessionThe session table is defined correctly. With columns such as
token,ip_address, anduser_agent, and a unique constraint ontoken(lines 420–426), the schema supports session management effectively. Ensure that allowing optional values forip_addressanduser_agentaligns with overall application logic.
432-493: Table Schema forpublic.mail0_summaryThe
mail0_summarytable is straightforward and properly defined. Primary keys and necessary fields for storing message summaries are in place, making the table suitable for its intended purpose.
494-562: Table Schema forpublic.mail0_userThe
mail0_usertable includes all the essential fields with correct data types and constraints. The unique constraint on
629-677: Table Schema forpublic.mail0_verificationThe verification table’s definition is clear and concise, with appropriate columns for storing verification identifiers and expiry details. No issues are apparent in this section.
678-690: Remaining Schema ComponentsThe empty sections for enums, schemas, sequences, roles, policies, views, and the
_metablock are acceptable as placeholders. They offer flexibility for future enhancements and align with the snapshot’s role in migration management.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
2 participants
Description
PR to persist all user configs, currently we only save the general settings
Implemented changes so we can persist all data, also, a easier way to extend the config schema
Type of Change
Please delete options that are not relevant.
Areas Affected
Please check all that apply:
Testing Done
Describe the tests you've done:
Security Considerations
For changes involving data or authentication:
Checklist
Screenshots/Recordings
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit
New Features
Chores