Skip to content

feature/persist user settings#513

Merged
nizzyabi merged 12 commits intoMail-0:stagingfrom
sergioxro:feature/persist-user-settings
Mar 25, 2025
Merged

feature/persist user settings#513
nizzyabi merged 12 commits intoMail-0:stagingfrom
sergioxro:feature/persist-user-settings

Conversation

@sergioxro
Copy link
Contributor

@sergioxro sergioxro commented Mar 24, 2025

Description

add persistent user settings:

current persisten configs: general

added:
update schema to use jsonb
create schema and type for jsonb field
add save on settings page
i18n strings


Type of Change

Please delete options that are not relevant.

  • ✨ New feature (non-breaking change which adds functionality)
  • [?] 💥 Breaking change (fix or feature with breaking changes)

Areas Affected

Please check all that apply:

  • Email Integration (Gmail, IMAP, etc.)
  • Data Storage/Management
  • API Endpoints
  • Development Workflow

Testing Done

Describe the tests you've done:

  • Manual testing performed
  • Cross-browser testing (if UI changes)

Security Considerations

For changes involving data or authentication:

  • No sensitive data is exposed
  • Authentication checks are in place
  • Input validation is implemented

Screenshots/Recordings

image


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
    • Enhanced the user settings experience with automatic loading and asynchronous saving, ensuring your preferences are accurately managed.
    • Introduced dynamic timezone selection based on your browser's settings and refined language selection with localized placeholders.
    • Improved the sign-up process by automatically initializing default settings for new users, streamlining the onboarding experience.
    • Upgraded notifications for settings changes to provide clear success and error feedback, ensuring a smoother interaction with your account preferences.
    • Added new localization support for settings-related messages and options within the application.
    • Introduced a new hook for managing user settings efficiently.
    • Added new tables to the database for storing user settings and notes, enhancing data management capabilities.

@vercel
Copy link

vercel bot commented Mar 24, 2025

@CodyCodes95 is attempting to deploy a commit to the Zero Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 24, 2025

Walkthrough

This PR introduces a comprehensive user settings management feature. New functions to retrieve and update settings have been added to the back-end, and the front-end now asynchronously loads and saves these settings with proper validation. A custom hook simplifies data fetching of user settings, and authentication is enhanced to initialize settings upon sign-up. Additionally, new utilities for timezone handling and updated localization messages have been implemented. Finally, the database schema is updated with new tables and migrations to persist user settings and related entities.

Changes

File(s) Change Summary
apps/mail/actions/settings.ts Added asynchronous functions getUserSettings, saveUserSettings, getAuthenticatedUserId, and validateSettings for user settings management.
apps/mail/app/(routes)/settings/general/page.tsx Enhanced the GeneralPage component to load settings on mount, reset form values, handle asynchronous saves, and update default timezone dynamically.
apps/mail/hooks/use-settings.ts Introduced a custom hook useSettings that utilizes SWR and session data to fetch user settings, providing default settings when none exist.
apps/mail/lib/auth.ts Added an authentication hook that initializes new user settings during sign-up by checking for existing settings and inserting defaults if absent.
apps/mail/lib/timezones.ts Introduced functions getBrowserTimezone and isValidTimezone for timezone utilities.
apps/mail/locales/en.json Updated localization keys for settings notifications and UI text, adding a new "settings" section.
packages/db/migrations/*; packages/db/src/* Introduced new database structures including tables (mail0_note, mail0_user_settings, and userSettings), migration snapshots, journal entries, and default settings definitions for persisting user settings.
packages/db/package.json Modified the "exports" section to include a new export for ./user_settings_default.
packages/db/src/user_settings_default.ts Added default user settings and a schema for validating user settings using the Zod library.

Sequence Diagram(s)

sequenceDiagram
    participant U as User
    participant GP as GeneralPage
    participant AS as Settings Actions
    participant DB as Database

    U->>GP: Open Settings Page
    GP->>AS: Call getUserSettings()
    AS->>DB: Query userSettings table
    DB-->>AS: Return settings / null
    AS->>GP: Return validated settings (or error)
    Note over GP: Form resets with fetched settings
    U->>GP: Submit updated settings
    GP->>AS: Call saveUserSettings(updatedSettings)
    AS->>DB: Update/Insert settings
    DB-->>AS: Confirmation
    AS->>GP: Send success response
Loading
sequenceDiagram
    participant NU as New User
    participant Auth as Auth Middleware
    participant DB as Database
    participant TZ as Timezone Utils

    NU->>Auth: Sign-up request (newSession)
    Auth->>DB: Check for existing settings
    alt No settings found
        Auth->>TZ: Get browser timezone
        TZ-->>Auth: Return timezone
        Auth->>DB: Insert default user settings with timezone
        DB-->>Auth: Confirmation
    else Settings exist
        Auth->>Auth: Skip initialization
    end
Loading

Possibly related PRs

  • Feature/persist user settings #403: Implements getUserSettings and saveUserSettings in settings.ts to manage user settings.
  • i18n #437: Modifies the GeneralPage component to handle user settings, directly related to the updated user settings functionality.
  • New Crowdin updates #440: Involves changes to the GeneralPage component that invoke the same settings functions for asynchronous operations.

Suggested reviewers

  • MrgSub
  • nizzyabi
  • ahmetskilinc

Poem

Hop, hop—I'm a rabbit on the run,
Bounding through code under the moon and sun.
Settings now load and save with ease,
Form resets and timezones set to please.
From DB to UI, changes are sublime,
Celebrating our code, one hop at a time!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 039dd52 and adda0c8.

📒 Files selected for processing (1)
  • apps/mail/app/(routes)/settings/general/page.tsx (4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/mail/app/(routes)/settings/general/page.tsx (3)
apps/mail/hooks/use-settings.ts (1)
  • useSettings (7-38)
apps/mail/lib/timezones.ts (1)
  • getBrowserTimezone (1-1)
apps/mail/actions/settings.ts (1)
  • saveUserSettings (53-88)
🔇 Additional comments (7)
apps/mail/app/(routes)/settings/general/page.tsx (7)

27-32: Good implementation of required imports

The new imports correctly support the added functionality for user settings persistence, including hooks for state management and API interaction.


45-45: Well-implemented SWR pattern with useSettings hook

Using the custom useSettings hook with SWR for data fetching addresses the previous review feedback and follows the project's patterns for data loading.


50-51: Improved default values for better user experience

Using locale as string and getBrowserTimezone() instead of hardcoded values provides a more personalized initial experience for users.


57-61: Good form reset implementation

The useEffect hook correctly resets the form with user settings when they become available, ensuring the form displays the user's saved preferences.


63-87: Robust async implementation with proper error handling

The onSubmit function now properly persists settings, handles errors, and provides user feedback through toast notifications. The optimistic updates with mutate and fallback handling are well implemented.


113-113: Improved internationalization for placeholders

Using translation keys for form placeholders ensures consistent localization throughout the application.

Also applies to: 137-137


141-145: Better timezone implementation with dynamic options

Using Intl.supportedValuesOf('timeZone') to dynamically generate timezone options is more maintainable and comprehensive than hardcoded values.

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sergioxro sergioxro changed the base branch from feature/persist-user-settings to staging March 24, 2025 20:33
@sergioxro sergioxro force-pushed the feature/persist-user-settings branch from 724509d to 58b2577 Compare March 24, 2025 23:10
@sergioxro sergioxro force-pushed the feature/persist-user-settings branch from 58b2577 to 3f239f6 Compare March 24, 2025 23:11
@sergioxro sergioxro marked this pull request as ready for review March 25, 2025 04:57
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (9)
apps/mail/lib/timezones.ts (1)

1-1: Consider adding error handling to getBrowserTimezone

While Intl.DateTimeFormat().resolvedOptions().timeZone is well-supported in modern browsers, it might be safer to add error handling with a fallback to a default timezone (like "UTC").

-export const getBrowserTimezone = () => Intl.DateTimeFormat().resolvedOptions().timeZone;
+export const getBrowserTimezone = () => {
+  try {
+    return Intl.DateTimeFormat().resolvedOptions().timeZone;
+  } catch (error) {
+    console.error("Failed to get browser timezone:", error);
+    return "UTC";
+  }
+};
apps/mail/hooks/use-settings.ts (1)

1-5: Add TypeScript interface for settings

Consider adding TypeScript types for the settings object to ensure type safety throughout the application.

import { getUserSettings } from "@/actions/settings";
import { useSession } from "@/lib/auth-client";
import useSWR from "swr";
-import { defaultUserSettings } from "@zero/db/user_settings_default";
+import { defaultUserSettings, UserSettings } from "@zero/db/user_settings_default";

Then update the return type:

export function useSettings() {
  // Rest of the code...
  
  return {
-    settings: data,
+    settings: data as UserSettings | undefined,
    isLoading,
    error,
    mutate,
  };
}
apps/mail/lib/auth.ts (2)

7-7: Potential server/client environment mismatch.
Using getBrowserTimezone() on the server may not reflect the actual client timezone if no header is provided. Consider ensuring that the fallback truly represents the user's browser or handle this scenario differently.


155-191: Hook configuration for sign-up logic looks correct.

  1. The conditional block for checking existing settings helps avoid duplicates.
  2. Vercel header usage is a neat optimization. However, be mindful that if the header is absent or malformed, the fallback might not accurately represent the user’s timezone when running on the server.
  3. Consider adding extensive logging if the insert fails, to easily trace issues.
apps/mail/actions/settings.ts (2)

11-49: Get user settings implementation is robust.

  1. Good error handling for unauthorized sessions.
  2. Throwing an error for invalid schema is appropriately strict, though you may consider partial merges for certain user scenarios.
  3. Returning null if no stored settings is a valid approach to fallback to defaults.

51-109: Save user settings approach is solid.

  1. Properly validates input with a schema.
  2. Auto-creates settings if none exist.
  3. If concurrency is expected (e.g., multiple simultaneous updates), consider an optimistic or pessimistic locking strategy to prevent overwriting changes.
apps/mail/app/(routes)/settings/general/page.tsx (1)

55-68: Consider adding loading state for initial settings retrieval.

The component shows a saving state when submitting the form, but no loading indicator when initially fetching settings. Consider adding a loading state during the initial fetch to improve user experience.

 export default function GeneralPage() {
   const [isSaving, setIsSaving] = useState(false);
+  const [isLoading, setIsLoading] = useState(true);
   const locale = useLocale();
   const t = useTranslations();

   // ...
   
   useEffect(() => {
     async function loadSettings() {
+      setIsLoading(true);
       try {
         const settings = await getUserSettings();
         if (settings) {
           form.reset(settings);
         }
       } catch (error) {
         console.error('Failed to load settings:', error);
         toast.error(t('common.settings.notFound'));
+      } finally {
+        setIsLoading(false);
       }
     }
     loadSettings();
   }, [form, t]);
   
   // ...
   
   return (
     <div className="grid gap-6">
       <SettingsCard
         // ...
         footer={
           <Button type="submit" form="general-form" disabled={isSaving}>
             {isSaving ? t('common.actions.saving') : t('common.actions.saveChanges')}
           </Button>
         }
       >
+        {isLoading ? (
+          <div className="flex justify-center py-8">
+            <LoadingSpinner /> {/* Assuming you have a loading component */}
+          </div>
+        ) : (
         <Form {...form}>
           {/* Form content */}
         </Form>
+        )}
       </SettingsCard>
     </div>
   );
 }

Also applies to: 70-88

packages/db/src/user_settings_default.ts (1)

1-17: Consider adding more specific timezone validation.

The current schema accepts any string for timezone. Consider restricting it to valid timezone identifiers to prevent invalid values.

-    timezone: z.string(),
+    timezone: z.string().refine(
+      (tz) => {
+        try {
+          Intl.DateTimeFormat(undefined, { timeZone: tz });
+          return true;
+        } catch (e) {
+          return false;
+        }
+      },
+      {
+        message: "Invalid timezone identifier",
+      }
+    ),
packages/db/migrations/meta/0017_snapshot.json (1)

585-585: Consider using a more maintainable approach for defaults.

The default JSON value is hardcoded in the migration file, which could lead to inconsistencies if the defaults change in the TypeScript code but aren't updated here. Consider using a migration helper function or a separate config that can be shared.

For future migrations, consider creating a SQL function that reads the defaults from a centralized location or generating the migration file programmatically to ensure consistency.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between db00358 and 5b6c234.

📒 Files selected for processing (12)
  • apps/mail/actions/settings.ts (1 hunks)
  • apps/mail/app/(routes)/settings/general/page.tsx (4 hunks)
  • apps/mail/hooks/use-settings.ts (1 hunks)
  • apps/mail/lib/auth.ts (2 hunks)
  • apps/mail/lib/timezones.ts (1 hunks)
  • apps/mail/locales/en.json (2 hunks)
  • packages/db/migrations/0017_bouncy_shotgun.sql (1 hunks)
  • packages/db/migrations/meta/0017_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_default.ts (1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
apps/mail/hooks/use-settings.ts (2)
apps/mail/actions/settings.ts (1)
  • getUserSettings (11-49)
packages/db/src/user_settings_default.ts (1)
  • defaultUserSettings (3-8)
apps/mail/lib/auth.ts (3)
packages/db/src/schema.ts (1)
  • userSettings (106-115)
apps/mail/lib/timezones.ts (2)
  • isValidTimezone (3-9)
  • getBrowserTimezone (1-1)
packages/db/src/user_settings_default.ts (1)
  • defaultUserSettings (3-8)
packages/db/src/schema.ts (1)
packages/db/src/user_settings_default.ts (1)
  • defaultUserSettings (3-8)
apps/mail/app/(routes)/settings/general/page.tsx (4)
apps/mail/lib/timezones.ts (1)
  • getBrowserTimezone (1-1)
apps/mail/i18n/config.ts (1)
  • Locale (18-18)
apps/mail/components/ui/select.tsx (4)
  • SelectValue (145-145)
  • SelectTrigger (146-146)
  • SelectContent (147-147)
  • SelectItem (149-149)
apps/mail/components/ui/form.tsx (1)
  • FormControl (166-166)
🔇 Additional comments (26)
packages/db/migrations/meta/_journal.json (1)

123-129: Migration journal updated correctly

The new entry for migration "0017_bouncy_shotgun" follows the existing pattern and is properly formatted.

packages/db/package.json (1)

24-25: New export path added for user settings defaults

The addition of the user_settings_default export path will make the default settings available to other parts of the application.

apps/mail/lib/timezones.ts (1)

3-9: Implementation uses recommended Intl API

Good implementation following the suggestion to use Intl.supportedValuesOf('timeZone') with proper error handling.

apps/mail/hooks/use-settings.ts (1)

10-26: Well-structured data fetching with SWR

Good use of SWR for data fetching with proper conditional fetching based on user ID and error handling.

packages/db/src/schema.ts (2)

1-2: Proper imports for new functionality

The imports have been correctly updated to include jsonb from drizzle-orm and the defaultUserSettings from the newly created module.


106-115: Well-defined userSettings table schema

The userSettings table is properly defined with appropriate fields, constraints, and defaults. The foreign key relationship to the user table ensures data integrity, and the unique constraint on userId ensures that each user has only one settings record. The use of jsonb for the settings field provides flexibility for storing structured settings data.

apps/mail/lib/auth.ts (1)

2-3: No issues with the new imports.
These imports appear consistent with the rest of the file and help integrate user settings logic.

apps/mail/actions/settings.ts (1)

1-9: Imports and server directive look fine.
No immediate concerns. The typed imports from your database schema and validation modules are consistent.

packages/db/migrations/0017_bouncy_shotgun.sql (3)

1-11: Creation of mail0_note table appears fine.

  1. Fields such as color, is_pinned, and order have sensible default values.
  2. The cascade deletion for user references ensures child notes are removed when a user is removed.

13-20: mail0_user_settings table aligns well with app logic.

  1. The default JSON for settings is consistent with defaultUserSettings.
  2. The UNIQUE constraint on user_id enforces a one-to-one relationship for settings.

22-23: Foreign key constraints are clearly defined.
No concerns here, although note that user settings are not cascaded on delete. Ensure that is the intended design.

apps/mail/app/(routes)/settings/general/page.tsx (8)

27-31: Import additions look good and align with the new functionality.

The new imports for React hooks, timezone utilities, and user settings actions are necessary for implementing persistent settings.


48-49: Type change and browser timezone detection enhance user experience.

Good changes:

  1. Converting locale to string aligns with the form's expected input type
  2. Using getBrowserTimezone() provides a sensible default value for new users

55-68: Well-implemented settings loading with proper error handling.

The useEffect hook correctly loads user settings on component mount and handles potential errors. The dependency array includes form and t, ensuring it runs when these dependencies change.


70-88: Robust implementation of settings persistence.

The async function handles several scenarios well:

  • Saving settings to the backend
  • Changing language with proper user feedback
  • Comprehensive error handling with user-friendly notifications

114-114: Good use of internationalization for placeholder text.

Using translation keys instead of hardcoded strings improves the application's localization support.


138-138: Good use of internationalization for placeholder text.

Using translation keys instead of hardcoded strings improves the application's localization support.


142-146: Dynamic timezone options using native browser API.

Using Intl.supportedValuesOf('timeZone') is a clean approach to get all supported timezones without maintaining a static list.


70-75:

Details

✅ Verification successful

Verify the authentication checks for saveUserSettings.

The PR description mentions authentication checks, but there's no explicit auth verification in this component. This is likely handled in the imported saveUserSettings function.


🏁 Script executed:

#!/bin/bash
# Check if saveUserSettings has authentication verification
rg -A 10 "export (async )?function saveUserSettings" apps/mail/actions/settings.ts

Length of output: 377


Authentication check in saveUserSettings verified

The authentication logic is correctly handled in the imported saveUserSettings function in apps/mail/actions/settings.ts. The shell script output confirms that the function retrieves the session and throws an "Unauthorized" error if no session is found. No additional auth checks are needed in this component.

packages/db/src/user_settings_default.ts (3)

3-8: Good default settings with appropriate values.

The default settings are well-structured, with reasonable defaults for language (English), timezone (UTC), disabled dynamic content (security-conscious default), and enabled external images.


10-15: Well-structured schema validation with Zod.

The Zod schema correctly defines the types for all settings properties, ensuring type safety throughout the application. This will prevent invalid data from being stored in the database.


17-17: Good TypeScript type derivation.

Using typeof defaultUserSettings to derive the type is an excellent approach. It ensures the type definition automatically stays in sync with the default settings object.

apps/mail/locales/en.json (2)

187-192: Well-structured localization for settings messages.

The new settings localization keys provide appropriate feedback messages for user interactions with settings. The inclusion of a parameterized message for language changes (languageChanged) is particularly useful.


258-258: Clear selection prompts for language and timezone.

The added prompts for language and timezone selection provide clear guidance to users, enhancing the UX of the settings interface.

Also applies to: 260-260

packages/db/migrations/meta/0017_snapshot.json (2)

564-629: Well-designed user settings table with appropriate constraints.

The mail0_user_settings table is well-structured with:

  • Primary key for the record
  • Foreign key relationship to the user table
  • Unique constraint on user_id ensuring one settings record per user
  • JSONB field for storing the settings with the correct default value
  • Appropriate timestamp fields for record tracking

This approach allows for flexible storage of user settings while maintaining database integrity.


580-586:

Details

✅ Verification successful

Verify that the default JSON matches the TypeScript defaults.

The default JSON value in the database migration matches the defaultUserSettings defined in the TypeScript file. This consistency is important to maintain, but could be prone to drift if the defaults are updated in one place but not the other.


🏁 Script executed:

#!/bin/bash
# Check if default settings in TypeScript match the database migration default

# Get the default settings from the TypeScript file
TS_DEFAULTS=$(cat packages/db/src/user_settings_default.ts | grep -A 5 "defaultUserSettings" | grep -v "export const")

# Get the default settings from the database migration
DB_DEFAULTS=$(cat packages/db/migrations/meta/0017_snapshot.json | grep -A 1 "default" | grep "language")

echo "TypeScript defaults:"
echo "$TS_DEFAULTS"
echo -e "\nDatabase defaults:"
echo "$DB_DEFAULTS"

# Check if all TypeScript defaults appear in the database default
TS_LANGUAGE=$(echo "$TS_DEFAULTS" | grep "language" | grep -o '"[^"]*"' | tail -1)
TS_TIMEZONE=$(echo "$TS_DEFAULTS" | grep "timezone" | grep -o '"[^"]*"' | tail -1)
TS_DYNAMIC=$(echo "$TS_DEFAULTS" | grep "dynamicContent" | grep -o 'false\|true')
TS_EXTERNAL=$(echo "$TS_DEFAULTS" | grep "externalImages" | grep -o 'false\|true')

echo -e "\nComparing individual values:"
echo "Language: TypeScript=$TS_LANGUAGE, In DB=$(echo "$DB_DEFAULTS" | grep -o '"language":"[^"]*"' | grep -o ':"[^"]*"' | grep -o '"[^"]*"')"
echo "Timezone: TypeScript=$TS_TIMEZONE, In DB=$(echo "$DB_DEFAULTS" | grep -o '"timezone":"[^"]*"' | grep -o ':"[^"]*"' | grep -o '"[^"]*"')"
echo "Dynamic Content: TypeScript=$TS_DYNAMIC, In DB=$(echo "$DB_DEFAULTS" | grep -o '"dynamicContent":[^,}]*' | grep -o ':[^,}]*')"
echo "External Images: TypeScript=$TS_EXTERNAL, In DB=$(echo "$DB_DEFAULTS" | grep -o '"externalImages":[^,}]*' | grep -o ':[^,}]*')"

Length of output: 3375


Default JSON Consistency Verified

The default JSON value in the migration file (packages/db/migrations/meta/0017_snapshot.json, lines 580–586) is consistent with the defaultUserSettings defined in the TypeScript file (packages/db/src/user_settings_default.ts). In both cases, the settings specify:

  • language: "en"
  • timezone: "UTC"
  • dynamicContent: false
  • externalImages: true

Please ensure that future updates to these default values are made in both places to avoid drift.

@MrgSub
Copy link
Collaborator

MrgSub commented Mar 25, 2025

LGTM, but before we merge, can we experiment with preloading the user's settings in Server side props? It's probably faster

@sergioxro
Copy link
Contributor Author

@MrgSub I think we can't (? since we are using app dir, but ill try this

@sergioxro
Copy link
Contributor Author

@MrgSub mmh I think we have a complication since the page is client side, we cant make async the component

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
apps/mail/actions/settings.ts (3)

21-31: Avoid logging potentially sensitive details in validation.
While logging the entire settings object is helpful for debugging, it may leak sensitive or personal information. Consider redacting or minimizing what gets logged to reduce potential exposure.

 console.error("Settings validation error: Schema mismatch", {
-  error,
-  settings
+  error
 });

33-51: Ensure index on userId column for efficient lookups.
The .where(eq(userSettings.userId, userId)) query should be backed by an index on the userId column to prevent potential performance bottlenecks on large tables.


53-88: Return updated settings for better client-side handling.
After saving new settings, consider returning them to the caller instead of just { success: true }, allowing immediate UI updates if needed.

return { 
  success: true,
+ updatedSettings: settings 
};
apps/mail/app/(routes)/settings/general/page.tsx (2)

27-31: Lazy-load imports if usage is infrequent.
If these functions are used rarely, you could dynamically import them to optimize initial load. If they're critical for the page, then this is fine as is.


145-149: Provide fallback for unsupported timezone listings.
Not all runtimes and older browsers support Intl.supportedValuesOf('timeZone'). Consider a fallback array of common timezones or a message if unavailable.

+const supportedTimezones = typeof Intl.supportedValuesOf === "function"
+  ? Intl.supportedValuesOf("timeZone")
+  : ["UTC", "America/New_York", "Europe/London"]; // fallback
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fdb11ac and c808d0c.

📒 Files selected for processing (2)
  • apps/mail/actions/settings.ts (1 hunks)
  • apps/mail/app/(routes)/settings/general/page.tsx (4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/mail/app/(routes)/settings/general/page.tsx (2)
apps/mail/lib/timezones.ts (1)
  • getBrowserTimezone (1-1)
apps/mail/actions/settings.ts (2)
  • getUserSettings (33-51)
  • saveUserSettings (53-88)
🔇 Additional comments (3)
apps/mail/actions/settings.ts (1)

10-19: Function initialization looks good.
This helper method correctly retrieves and validates the user's ID from the session. It properly throws an error when authentication is missing, ensuring unauthorized requests are blocked.

apps/mail/app/(routes)/settings/general/page.tsx (2)

55-68: Good approach for initial settings fetch.
The asynchronous useEffect properly loads settings and gracefully handles errors, improving user experience by showing a notification when loading fails.


70-90: Error handling on submission is clear and user-friendly.
Wrapping saveUserSettings in try-catch ensures the UI provides feedback for both success and failure. The setIsSaving flag also helps track submission state.

@sergioxro
Copy link
Contributor Author

Comment:
To make it more scaleable the action settings.ts its going to be modified on a next pr when we persist all user config, but for this pr would be too much info

@sergioxro sergioxro requested a review from MrgSub March 25, 2025 16:14
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
apps/mail/app/(routes)/settings/general/page.tsx (2)

62-82: Consider separating language change logic for better maintainability.

The onSubmit function handles multiple responsibilities. For better maintainability, consider extracting the language change logic to a separate function.

+ async function handleLanguageChange(language: string) {
+   if (language !== locale) {
+     await changeLocale(language as Locale);
+     const localeName = new Intl.DisplayNames([language], { type: 'language' }).of(language);
+     toast.success(t('common.settings.languageChanged', { locale: localeName }));
+   }
+ }

  async function onSubmit(values: z.infer<typeof formSchema>) {
    setIsSaving(true);

    try {
      await mutate(values, { revalidate: false });

-     if (values.language !== locale) {
-       await changeLocale(values.language as Locale);
-       const localeName = new Intl.DisplayNames([values.language], { type: 'language' }).of(
-         values.language,
-       );
-       toast.success(t('common.settings.languageChanged', { locale: localeName }));
-     }
+     await handleLanguageChange(values.language);

      toast.success(t('common.settings.saved'));
    } catch (error) {
      // ...
    }
  }

77-82: Consider adding more specific error handling.

While the current error handling is good, you could enhance it by handling specific error types differently for a better user experience.

  } catch (error) {
    console.error('Failed to save settings:', error);
-   toast.error(t('common.settings.failedToSave'));
+   if (error instanceof NetworkError) {
+     toast.error(t('common.errors.network'));
+   } else if (error instanceof ValidationError) {
+     toast.error(t('common.errors.validation'));
+   } else {
+     toast.error(t('common.settings.failedToSave'));
+   }
    // Revert the optimistic update
    await mutate();
  }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c808d0c and 039dd52.

📒 Files selected for processing (1)
  • apps/mail/app/(routes)/settings/general/page.tsx (4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/mail/app/(routes)/settings/general/page.tsx (2)
apps/mail/hooks/use-settings.ts (1)
  • useSettings (7-38)
apps/mail/lib/timezones.ts (1)
  • getBrowserTimezone (1-1)
🔇 Additional comments (7)
apps/mail/app/(routes)/settings/general/page.tsx (7)

27-31: Good addition of necessary imports for handling user settings.

The inclusion of React hooks and utility imports enables proper state management and user settings synchronization.


44-44: Well done implementing the useSettings hook as suggested.

You've successfully incorporated the useSettings hook to access user settings and the mutate function, following previous review feedback.


49-50: Improved defaults with dynamic values.

Good improvement replacing hardcoded values with dynamic ones - using the current locale and browser timezone provides a better initial user experience.


56-60: Nice implementation of form synchronization with settings.

The useEffect hook ensures the form reflects user settings once they're loaded, providing a seamless experience.


62-85: Well-structured async onSubmit with proper error handling.

Your implementation includes:

  • Optimistic updates with the mutate function
  • Special handling for language changes with appropriate feedback
  • Robust error handling with fallback and user feedback
  • Proper state management for loading indicators

This addresses the previous feedback about handling specific errors.


111-111: Good internationalization of placeholders.

Using translation keys for placeholders improves the multilingual user experience.

Also applies to: 135-135


139-143: Excellent use of native browser API for timezones.

Using Intl.supportedValuesOf('timeZone') to dynamically generate timezone options improves maintenance and ensures the list stays current with browser standards.

@sergioxro sergioxro marked this pull request as draft March 25, 2025 18:29
@sergioxro sergioxro marked this pull request as ready for review March 25, 2025 19:07
@vercel
Copy link

vercel bot commented Mar 25, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
0 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 25, 2025 7:58pm

@nizzyabi nizzyabi merged commit 2463442 into Mail-0:staging Mar 25, 2025
4 checks passed
nizzyabi added a commit that referenced this pull request Mar 26, 2025
* adjustable height

* h1 h2 h3 working in reply composer

* select dropdown for categories

* feat(navbar): update item label based on auth status

* feature/persist user settings (#513)

* feat: persist setting (codycodes95)

* feat: update settings to jsonb

* feat: run migration

* feat: save changes to db

* fix: naming

* feat: validate settings schema

* feat: add i18n

* fix: set i18n variables

* fix: coderabbit comment

* feat: improve function readability

* feat: use hook

* fix:update settings

---------

Co-authored-by: Cody Partington <codythatsme@gmail.com>

* remove unique status from email in schema

* early access check added to schema

* updated readme

* add contributors

* remove text-decoration

* text-decoration

* remove auto focus on search

* ahuh

* gg

* i18n

* check email for early access (#519)

* check email for early access

* one check

* saving...

* disable buttons

* disable

* fix

* saving...

---------

Co-authored-by: Nizzy <nizabizaher@gmail.com>
Co-authored-by: pietrodev07 <pietro.dev.07@gmail.com>
Co-authored-by: Sergio JVA <60497216+sergio-jva@users.noreply.github.com>
Co-authored-by: Cody Partington <codythatsme@gmail.com>
Co-authored-by: Ahmet Kilinc <akx9@icloud.com>
Co-authored-by: user12224 <122770437+user12224@users.noreply.github.com>
Co-authored-by: nizzy <140507264+nizzyabi@users.noreply.github.com>
@coderabbitai coderabbitai bot mentioned this pull request Mar 30, 2025
18 tasks
nizzyabi added a commit that referenced this pull request Apr 1, 2025
* adjustable height

* h1 h2 h3 working in reply composer

* select dropdown for categories

* feat(navbar): update item label based on auth status

* feature/persist user settings (#513)

* feat: persist setting (codycodes95)

* feat: update settings to jsonb

* feat: run migration

* feat: save changes to db

* fix: naming

* feat: validate settings schema

* feat: add i18n

* fix: set i18n variables

* fix: coderabbit comment

* feat: improve function readability

* feat: use hook

* fix:update settings

---------

Co-authored-by: Cody Partington <codythatsme@gmail.com>

* remove unique status from email in schema

* early access check added to schema

* updated readme

* add contributors

* remove text-decoration

* text-decoration

* remove auto focus on search

* ahuh

* gg

* i18n

* check email for early access (#519)

* check email for early access

* one check

* saving...

* disable buttons

* disable

* fix

* saving...

* saving...

* minor

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (Arabic)

* New translations en.json (Catalan)

* New translations en.json (Czech)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Korean)

* New translations en.json (Polish)

* New translations en.json (Portuguese)

* New translations en.json (Russian)

* New translations en.json (Turkish)

* New translations en.json (Latvian)

* New translations en.json (Hindi)

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (Arabic)

* New translations en.json (Catalan)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Korean)

* New translations en.json (Polish)

* New translations en.json (Portuguese)

* New translations en.json (Hindi)

* reply and searchbar display

* reply ai (#526)

* reply ai

* ai functionality

* line height

* adam fixes

---------

Co-authored-by: user12224 <122770437+user12224@users.noreply.github.com>
Co-authored-by: Nizzy <nizabizaher@gmail.com>

* Autocompletions for reply and create

* email avatars (#528)

* added email avatars

* fix small issue

* small ui fix

* color fix

* reply ui

* New translations en.json (Japanese)

* New translations en.json (Korean)

* no drop down

* ui fix

* wip performance

* saving...

* saving...

* saving...

* saving...

* - updated phrases
- added delay of 2 matching characters

* Improved ai with custom prompt (#534)

* ai

* improved ai

* improved-ai-with-custom-prompt

* empty commit

* removed new lines

* empty commit

* search

* forwarding

* search filter removed. all in ai now

* saving...

* fix double submit on command enter create email

* saving...

* saving...

* turn search ai into a server action

* fuix

* show most recent email in thread

* saving...

* saving...

* forward and reply in one compose button

* saving...

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (Arabic)

* New translations en.json (Catalan)

* New translations en.json (Czech)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Korean)

* New translations en.json (Polish)

* New translations en.json (Portuguese)

* New translations en.json (Russian)

* New translations en.json (Turkish)

* New translations en.json (Latvian)

* New translations en.json (Hindi)

* fix to height reply composer

* posthog

* remove github login for now

* refresh

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (Arabic)

* New translations en.json (Catalan)

* New translations en.json (Czech)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Korean)

* New translations en.json (Polish)

* New translations en.json (Portuguese)

* New translations en.json (Russian)

* New translations en.json (Turkish)

* New translations en.json (Latvian)

* New translations en.json (Hindi)

* revert

* a

* fix load more

* fix load more

* remove use memo from thread to not load when opening an email

* fix switching accounts

---------

Co-authored-by: Nizzy <nizabizaher@gmail.com>
Co-authored-by: pietrodev07 <pietro.dev.07@gmail.com>
Co-authored-by: Sergio JVA <60497216+sergio-jva@users.noreply.github.com>
Co-authored-by: Cody Partington <codythatsme@gmail.com>
Co-authored-by: Ahmet Kilinc <akx9@icloud.com>
Co-authored-by: user12224 <122770437+user12224@users.noreply.github.com>
Co-authored-by: nizzy <140507264+nizzyabi@users.noreply.github.com>
Co-authored-by: [bot] <zero@ibra.rip>
Co-authored-by: needle <122770437+needleXO@users.noreply.github.com>
nizzyabi added a commit that referenced this pull request Apr 1, 2025
* adjustable height

* h1 h2 h3 working in reply composer

* select dropdown for categories

* feat(navbar): update item label based on auth status

* feature/persist user settings (#513)

* feat: persist setting (codycodes95)

* feat: update settings to jsonb

* feat: run migration

* feat: save changes to db

* fix: naming

* feat: validate settings schema

* feat: add i18n

* fix: set i18n variables

* fix: coderabbit comment

* feat: improve function readability

* feat: use hook

* fix:update settings

---------

Co-authored-by: Cody Partington <codythatsme@gmail.com>

* remove unique status from email in schema

* early access check added to schema

* updated readme

* add contributors

* remove text-decoration

* text-decoration

* remove auto focus on search

* ahuh

* gg

* i18n

* check email for early access (#519)

* check email for early access

* one check

* saving...

* disable buttons

* disable

* fix

* saving...

* saving...

* minor

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (Arabic)

* New translations en.json (Catalan)

* New translations en.json (Czech)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Korean)

* New translations en.json (Polish)

* New translations en.json (Portuguese)

* New translations en.json (Russian)

* New translations en.json (Turkish)

* New translations en.json (Latvian)

* New translations en.json (Hindi)

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (Arabic)

* New translations en.json (Catalan)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Korean)

* New translations en.json (Polish)

* New translations en.json (Portuguese)

* New translations en.json (Hindi)

* reply and searchbar display

* reply ai (#526)

* reply ai

* ai functionality

* line height

* adam fixes

---------

Co-authored-by: user12224 <122770437+user12224@users.noreply.github.com>
Co-authored-by: Nizzy <nizabizaher@gmail.com>

* Autocompletions for reply and create

* email avatars (#528)

* added email avatars

* fix small issue

* small ui fix

* color fix

* reply ui

* New translations en.json (Japanese)

* New translations en.json (Korean)

* no drop down

* ui fix

* wip performance

* saving...

* saving...

* saving...

* saving...

* - updated phrases
- added delay of 2 matching characters

* Improved ai with custom prompt (#534)

* ai

* improved ai

* improved-ai-with-custom-prompt

* empty commit

* removed new lines

* empty commit

* search

* forwarding

* search filter removed. all in ai now

* saving...

* fix double submit on command enter create email

* saving...

* saving...

* turn search ai into a server action

* fuix

* show most recent email in thread

* saving...

* saving...

* forward and reply in one compose button

* saving...

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (Arabic)

* New translations en.json (Catalan)

* New translations en.json (Czech)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Korean)

* New translations en.json (Polish)

* New translations en.json (Portuguese)

* New translations en.json (Russian)

* New translations en.json (Turkish)

* New translations en.json (Latvian)

* New translations en.json (Hindi)

* fix to height reply composer

* posthog

* remove github login for now

* refresh

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (Arabic)

* New translations en.json (Catalan)

* New translations en.json (Czech)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Korean)

* New translations en.json (Polish)

* New translations en.json (Portuguese)

* New translations en.json (Russian)

* New translations en.json (Turkish)

* New translations en.json (Latvian)

* New translations en.json (Hindi)

* revert

* a

* fix load more

* fix load more

* remove use memo from thread to not load when opening an email

* fix switching accounts

* navbar changed to login

---------

Co-authored-by: Nizzy <nizabizaher@gmail.com>
Co-authored-by: pietrodev07 <pietro.dev.07@gmail.com>
Co-authored-by: Sergio JVA <60497216+sergio-jva@users.noreply.github.com>
Co-authored-by: Cody Partington <codythatsme@gmail.com>
Co-authored-by: Adam <x_1337@outlook.com>
Co-authored-by: Ahmet Kilinc <akx9@icloud.com>
Co-authored-by: user12224 <122770437+user12224@users.noreply.github.com>
Co-authored-by: nizzy <140507264+nizzyabi@users.noreply.github.com>
Co-authored-by: [bot] <zero@ibra.rip>
@coderabbitai coderabbitai bot mentioned this pull request Apr 5, 2025
14 tasks
This was referenced Jun 18, 2025
@coderabbitai coderabbitai bot mentioned this pull request Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants