Conversation
…dling and type safety
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughLogoFooter image now loads lazily. NotificationSettings is refactored to use DB enum-derived types, accept an array of settings plus explicit Changes
Sequence Diagram(s)(omitted — changes are focused inside a UI component and cache/mutation handling; no new multi-system sequential flow diagram is necessary) Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
alexmarqs
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
apps/web/src/components/settings/notification/NotificationSettings.tsx
Outdated
Show resolved
Hide resolved
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/settings/notification/NotificationSettings.tsx (1)
70-92: Add guard to prevent optimistic updates when creating new notification settings.At line 84, the comparison
item.id === newData.setting.idwill be problematic whenidisundefined(when creating a new setting). Although the condition won't match any items, it's clearer and safer to explicitly skip the optimistic update for new settings by addingnewData.setting.id !== undefinedto the condition at line 80.Consider this update:
// Only perform optimistic update if previousData exists -if (previousData) { +if (previousData && newData.setting.id !== undefined) { queryClient.setQueryData<Tables<"notification_settings">[]>( [NotificationsServerKeys.GET_USER_NOTIFICATION_SETTINGS], previousData.map((item) => item.id === newData.setting.id ? { ...item, ...newData.setting } : item, ), ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/components/LogoFooter.tsxapps/web/src/components/settings/notification/NotificationSettings.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/components/settings/notification/NotificationSettings.tsx (2)
apps/web/src/lib/supabase/database.types.ts (2)
Enums(203-218)Tables(124-151)apps/web/src/components/ui/switch.tsx (1)
Switch(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Web E2E Tests
- GitHub Check: Cursor Bugbot
🔇 Additional comments (7)
apps/web/src/components/LogoFooter.tsx (1)
26-26: Good performance optimization!Adding
loading="lazy"to the footer badge image is a best practice that defers loading until it's near the viewport.apps/web/src/components/settings/notification/NotificationSettings.tsx (6)
9-9: Excellent type safety improvement!Using
Enums<>from the database types ensures compile-time validation against the actual database schema for notification types and channels.Also applies to: 18-19
14-16: Type-safe label mapping looks good.The
Record<NotificationType, string>ensures all notification types have corresponding labels, making future additions type-safe.
39-43: Focused rendering approach is clearer.Rendering a single, explicitly-defined notification setting is more maintainable than generic iteration when only one type/channel combination is currently supported.
50-62: Component signature refactoring improves reusability.Accepting the full settings array with explicit type/channel props allows the component to derive its target setting internally, making it more flexible.
130-142: UI rendering is well-implemented.The stable
labelIdgeneration, type-safe label lookup, and safe default for the switchcheckedstate all handle the missing setting case appropriately for the UI layer.
64-64: No action required. Theuser_iddefault value is harmless—theupsertUserNotificationSettingmutation function retrieves the authenticated user's ID from the server session and overwrites any passed value withsession.user.id(line 52 inapps/web/src/lib/db/notifications.ts). Invalid database entries cannot be created through this code path.Likely an incorrect or invalid review comment.
… handle existing items and improve data consistency
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/components/settings/notification/NotificationSettings.tsx (2)
14-19: Consider defining types before usage for clarity.While TypeScript hoists type aliases, declaring
NotificationTypeandNotificationChannelafter their usage inNOTIFICATION_LABEL_MAP(line 14) can confuse readers scanning top-to-bottom.Suggested reordering
+type NotificationType = Enums<"notification_type">; +type NotificationChannel = Enums<"notification_channel">; + const NOTIFICATION_LABEL_MAP: Record<NotificationType, string> = { new_companies: "Receive email updates for new companies added", }; - -type NotificationType = Enums<"notification_type">; -type NotificationChannel = Enums<"notification_channel">;
118-123: Consider typing the mutation context explicitly.The type assertion for
contextworks but is brittle. React Query v5 can infer context types, but sinceuseUpsertUserNotificationSettingis a pre-defined hook, you may want to define a context type for better type safety.Alternative approach
type MutationContext = { previousData: Tables<"notification_settings">[]; }; // Then in onError: onError: (_err, _newData, context: MutationContext | undefined) => { queryClient.setQueryData( [NotificationsServerKeys.GET_USER_NOTIFICATION_SETTINGS], context?.previousData, ); },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/components/settings/notification/NotificationSettings.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Web E2E Tests
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
apps/web/src/components/settings/notification/NotificationSettings.tsx (3)
39-43: Clean, extensible pattern for notification settings.Passing explicit
typeandchannelprops with the settings array enables easy addition of new notification types while maintaining type safety through the enum constraints.
85-107: Optimistic update now correctly handles new settings.The logic addresses the previous concern about optimistic updates failing for new notification settings. By searching for
existingItemusing theuser_id + type + channelcombination (rather than justid), and creating a temporary record when none exists, the UI now responds immediately for both existing and first-time settings.
133-165: LGTM — callback and rendering logic are correct.
- The
useCallbackdependencies are exhaustive and accurate.- The
labelIdpattern (notification-setting-${type}-${channel}) ensures unique accessibility IDs.- Defaulting
checkedtofalsewhen no setting exists is the safe behavior.- The throttled callback appropriately prevents rapid-fire mutations.
apps/web/src/components/settings/notification/NotificationSettings.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/components/settings/notification/NotificationSettings.tsx
Outdated
Show resolved
Hide resolved
…ure proper fallback and avoid temporary IDs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/components/settings/notification/NotificationSettings.tsx (1)
64-67: Good fix for temp ID filtering; comment still misleading.Line 65 correctly prevents temp IDs from being sent to the server—this addresses the prior review concern.
However, the comment on line 67 claims "user id is already overridden in the onMutate function," but
onMutatedoesn't modifyuser_id—it passes the value through as-is. Consider clarifying whereuser_idis actually resolved (likely the backend/mutation hook).Suggested comment update
- const user_id = setting?.user_id ?? ""; // fallback to empty string if user id is not set, the user id is already overridden in the onMutate function. + const user_id = setting?.user_id ?? ""; // fallback to empty string; actual user_id is set by the backend from the session
🧹 Nitpick comments (1)
apps/web/src/components/settings/notification/NotificationSettings.tsx (1)
14-19: Consider moving type aliases before their usage.The type aliases
NotificationTypeandNotificationChannelare defined afterNOTIFICATION_LABEL_MAPusesNotificationType. While TypeScript hoists type declarations, placing the aliases before the map improves readability.Suggested reordering
+type NotificationType = Enums<"notification_type">; +type NotificationChannel = Enums<"notification_channel">; + const NOTIFICATION_LABEL_MAP: Record<NotificationType, string> = { new_companies: "Receive email updates for new companies added", }; - -type NotificationType = Enums<"notification_type">; -type NotificationChannel = Enums<"notification_channel">;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/components/settings/notification/NotificationSettings.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Web E2E Tests
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
apps/web/src/components/settings/notification/NotificationSettings.tsx (4)
39-43: LGTM!Clean simplification that passes the settings array with explicit type and channel props. The component now derives the matching setting internally, which is a good separation of concerns.
87-109: Well-designed optimistic update handling.The logic now correctly handles both update and insert scenarios:
- Finding existing items by
user_id + type + channel(not by ID) ensures proper upsert semantics.- New items get a temp-prefixed ID, timestamps, and safe
enabledfallback.This addresses the prior review concern about optimistic updates failing for new notification settings.
135-148: LGTM!The mutation payload correctly omits
idfor new settings (allowing server-side ID generation) and includes all necessary fields. The dependency array is complete.
155-168: LGTM!Deterministic
labelIdbased on type/channel ensures stable accessibility. Thesetting?.enabled ?? falsefallback safely handles cases where the setting doesn't exist yet.
alexmarqs
deleted the
chore/fix-notification-settings-for-old-db-entries
branch
1 participant
fix NotificationSettings component to improve notification handling and type safety
Note
Notifications
NotificationSettings.tsxto use typed enums (Enums<"notification_type">,Enums<"notification_channel">) and a typedNOTIFICATION_LABEL_MAP.NotificationSettingItemfornew_companiesviaemailinstead of mapping all settings.queryKey, snapshots previous data (default[]), updates or appends items (with temp IDs and timestamps), then invalidates on settle.created_at/updated_atin mutation), stabilizeslabelId, and safely handles missing settings (checked={setting?.enabled ?? false}).UI/Perf
loading="lazy"to the OpenStatus badge image inLogoFooter.tsx.Written by Cursor Bugbot for commit b7dd521. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Performance
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.