added notification settings to local storage and synced to supabase#6
added notification settings to local storage and synced to supabase#6fortune710 merged 4 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds per-user notification settings end-to-end: DB migration and types, push token table, PushNotificationService methods for cached get/save, a useNotificationSettings React hook with optimistic updates and toggle rules, and a refactored Notifications UI that consumes the hook. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Notifications UI
participant Hook as useNotificationSettings
participant Service as PushNotificationService
participant Storage as Local Storage
participant DB as Supabase
User->>UI: Toggle setting
UI->>Hook: toggleSetting(id)
rect rgb(240,248,255)
Note over Hook: Optimistic update
Hook->>Hook: cancel queries, compute new map, store previous
Hook->>UI: update cache -> UI re-renders
end
rect rgb(240,255,240)
Note over Hook,Service: Persist changes
Hook->>Service: saveNotificationSettings(userId, settings)
par
Service->>Storage: write local cache (best-effort)
Service->>DB: upsert notification_settings
and
Service-->>Hook: resolve / reject
end
end
alt Success
Hook->>Hook: invalidate/refetch query
Hook->>UI: final state confirmed
else Error
Hook->>Hook: rollback to previous state
Hook->>UI: re-render reverted state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/settings/notifications.tsx (1)
83-105: GuardpushEnabledwith a boolean fallback to avoid accidental disabling.- const pushEnabled = settingsMap[NotificationSettings.PUSH_NOTIFICATIONS]; + const pushEnabled = settingsMap[NotificationSettings.PUSH_NOTIFICATIONS] ?? true;
🧹 Nitpick comments (4)
frontend/supabase/migrations/20251212000000_notification_settings.sql (1)
21-31: Drop redundantuser_idindex (UNIQUE already indexes it).
UNIQUE(user_id)creates an index, soidx_notification_settings_user_idis typically duplicate.--- Indexes for performance -CREATE INDEX IF NOT EXISTS idx_notification_settings_user_id - ON notification_settings(user_id);Also applies to: 61-64
frontend/app/settings/notifications.tsx (1)
53-60: Consider disabling switches while settings aren’t actionable (e.g., no user / loading).Right now
toggleSettingcan no-op (when!user?.id), which feels like “tap didn’t work”. Consider also pullingisLoading(and/or acanEditflag) from the hook and wiringdisabledaccordingly.Also applies to: 98-104
frontend/services/push-notification-service.ts (1)
143-189: Merge cached settings with defaults to handle new toggles / partial data.Right now, a cached object from an older app version can miss keys and you’ll return an incomplete map.
static async getNotificationSettings(userId: string): Promise<NotificationSettingsMap | null> { try { + const defaults: NotificationSettingsMap = { + [NotificationSettings.FRIEND_REQUESTS]: true, + [NotificationSettings.PUSH_NOTIFICATIONS]: true, + [NotificationSettings.ENTRY_REMINDER]: false, + [NotificationSettings.FRIEND_ACTIVITY]: true, + }; + // 1. Try local storage first const local = await deviceStorage.getItem<NotificationSettingsMap>( NOTIFICATION_SETTINGS_STORAGE_KEY(userId), ); if (local) { - return local; + return { ...defaults, ...local }; }frontend/hooks/use-notification-settings.ts (1)
75-114: Optional: computenextfrom the latest cached value to avoid stale-closure edge cases.If users toggle multiple switches quickly, using
queryClient.getQueryData(queryKey)insidetoggleSettingcan be more robust than relying on the render-timecurrentSettings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/app/settings/notifications.tsx(3 hunks)frontend/constants/supabase.ts(2 hunks)frontend/hooks/use-notification-settings.ts(1 hunks)frontend/services/push-notification-service.ts(1 hunks)frontend/supabase/migrations/20251212000000_notification_settings.sql(1 hunks)frontend/types/database.ts(1 hunks)frontend/types/notifications.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/hooks/use-notification-settings.ts (2)
frontend/services/push-notification-service.ts (2)
NotificationSettingsMap(10-10)PushNotificationService(15-223)frontend/providers/auth-provider.tsx (1)
useAuthContext(69-75)
frontend/services/push-notification-service.ts (3)
frontend/lib/supabase.ts (1)
supabase(14-30)frontend/lib/logger.ts (1)
error(32-35)frontend/services/device-storage.ts (1)
deviceStorage(170-170)
frontend/app/settings/notifications.tsx (1)
frontend/hooks/use-notification-settings.ts (1)
useNotificationSettings(25-123)
🔇 Additional comments (3)
frontend/types/notifications.ts (1)
26-33: Enum values align with DB column naming; good typing win.frontend/constants/supabase.ts (1)
4-14: TABLES addition looks consistent with the migration name.frontend/types/database.ts (1)
230-261: DB typing matches the migration’sbigserialPK; good.If you keep timestamps nullable in SQL, consider loosening
created_at/updated_attostring | null(or make the SQLNOT NULL).
| const { mutate: saveSettings, isPending: isSaving, error: mutationError } = useMutation({ | ||
| mutationFn: async (next: NotificationSettingsMap) => { | ||
| if (!user?.id) return; | ||
| await PushNotificationService.saveNotificationSettings(user.id, next); | ||
| return next; | ||
| }, | ||
| onMutate: async (next: NotificationSettingsMap) => { | ||
| await queryClient.cancelQueries({ queryKey }); | ||
| const previous = queryClient.getQueryData<NotificationSettingsMap>(queryKey); | ||
|
|
||
| queryClient.setQueryData<NotificationSettingsMap>(queryKey, next); | ||
|
|
||
| return { previous }; | ||
| }, | ||
| onError: (_err, _next, context) => { | ||
| if (context?.previous) { | ||
| queryClient.setQueryData<NotificationSettingsMap>(queryKey, context.previous); | ||
| } | ||
| }, | ||
| onSettled: () => { | ||
| queryClient.invalidateQueries({ queryKey }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Optimistic rollback won’t happen unless saveNotificationSettings throws on failure.
onError rollback only runs if the mutation rejects; with the current service implementation, Supabase failures are logged but not thrown. Fixing the service to throw (or returning a failure result and throwing here) will make this hook behave as intended.
Also applies to: 120-121
🤖 Prompt for AI Agents
In frontend/hooks/use-notification-settings.ts around lines 49-71 (also apply
similar fix at lines ~120-121), the optimistic rollback in onError will never
run if PushNotificationService.saveNotificationSettings does not throw on
failure; update the code so failures produce a rejected mutation: either modify
PushNotificationService.saveNotificationSettings to throw when the Supabase call
fails, or (if you prefer not to change the service) change the mutationFn to
inspect the service result and throw an Error when it indicates failure so the
mutation rejects and onError can restore the previous state.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
frontend/types/database.ts (1)
230-261: Type-schema mismatch:created_atandupdated_atare modeled as required but SQL allows NULL.Lines 238-239 define
created_atandupdated_atasstring(required), but the migration in20251212000000_notification_settings.sql(lines 28-29) usesDEFAULT now()withoutNOT NULL, allowing NULL values. This mismatch could cause runtime type errors if NULL timestamps are inserted.This issue is already flagged in the past review comments for the migration file. Ensure the migration is updated to enforce
NOT NULLon these columns, or update these types tostring | null.Based on past review comments on the migration file.
frontend/supabase/migrations/20251212000000_notification_settings.sql (1)
28-29: AddNOT NULLconstraint to timestamp columns.These columns have
DEFAULT now()but allow NULL, which creates a type-schema mismatch withfrontend/types/database.ts(lines 238-239) where they're modeled as required strings.This issue is already flagged in the past review comments. Ensure you update the migration as suggested.
Based on past review comments for this file.
frontend/services/push-notification-service.ts (1)
59-63: Fix the logging order:currentTokenis null when logged.Line 59 logs
this.currentToken, which is stillnullat that point—it's assigned on line 63 after the log statement.Apply this diff:
- console.log('Push token:', this.currentToken); - // Save token to Supabase if (this.userId && !this.currentToken) { this.currentToken = tokenData.data; + console.log('Push token:', this.currentToken); //await this.savePushToken(this.currentToken, this.userId); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/constants/supabase.ts(2 hunks)frontend/services/push-notification-service.ts(1 hunks)frontend/supabase/migrations/20250825200450_misty_boat.sql(5 hunks)frontend/supabase/migrations/20251212000000_notification_settings.sql(1 hunks)frontend/types/database.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/constants/supabase.ts
🔇 Additional comments (3)
frontend/services/push-notification-service.ts (1)
195-224: Good: Supabase failures now throw, enabling proper error handling.The updated
saveNotificationSettingsmethod now throws when the Supabase upsert fails (line 222), which addresses the past review concern. Callers (e.g.,useMutation) can now detect sync failures and perform rollback or show unsynced state.frontend/types/database.ts (1)
262-290: LGTM:push_tokenstypes align with the migration schema.The type definitions correctly model the table structure, including the
'web'platform option and nullabledevice_id.frontend/supabase/migrations/20251212000000_notification_settings.sql (1)
49-54: Good:WITH CHECKclause added to UPDATE policy.The UPDATE policy now includes
WITH CHECK (auth.uid() = user_id)on line 54, which prevents users from reassigning theuser_idto another value during updates. This addresses the critical concern from the past review comments.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
frontend/services/push-notification-service.ts (1)
16-72:initialize()returnsnullwhen called withoutuserId+ logs wrong token + potential web crash
Right now the token is only assigned insideif (this.userId && !this.currentToken)and you logthis.currentTokenbefore assignment. Also,Device.isDeviceis not a reliable “web guard” for Expo; you should explicitly gate non-native platforms before calling Expo push APIs.async initialize(userId?: string): Promise<string | null> { try { this.userId = userId || null; + + // Expo push tokens are only meaningful on native platforms + if (Platform.OS !== 'ios' && Platform.OS !== 'android') { + console.warn('Push notifications are not supported on this platform:', Platform.OS); + return null; + } // Check if device supports push notifications if (!Device.isDevice) { console.warn('Push notifications only work on physical devices'); return null; } @@ const tokenData = await Notifications.getExpoPushTokenAsync({ projectId: Constants.expoConfig?.extra?.eas?.projectId || Constants.easConfig?.projectId, }); - - console.log('Push token:', this.currentToken); + this.currentToken = tokenData.data; // Save token to Supabase - if (this.userId && !this.currentToken) { - this.currentToken = tokenData.data; - //await this.savePushToken(this.currentToken, this.userId); + if (this.userId) { + await PushNotificationService.savePushToken(this.currentToken, this.userId); } return this.currentToken;
🧹 Nitpick comments (1)
frontend/services/push-notification-service.ts (1)
108-142:removePushToken()/updateUserId()are inconsistent with staticsavePushToken()(and currently don’t persist)
updateUserId()can’t call the persistence API because it’s commented and static;removePushToken()relies on the same possibly-nulldeviceIdissue assavePushToken(). Consider either (a) making token persistence instance-based consistently or (b) keeping it static but always callingPushNotificationService.savePushToken(...).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/services/push-notification-service.ts(1 hunks)frontend/supabase/migrations/20250825200450_misty_boat.sql(5 hunks)frontend/supabase/migrations/20251212000000_notification_settings.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/supabase/migrations/20251212000000_notification_settings.sql
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/services/push-notification-service.ts (3)
frontend/lib/supabase.ts (1)
supabase(14-30)frontend/lib/logger.ts (1)
error(32-35)frontend/services/device-storage.ts (1)
deviceStorage(170-170)
🔇 Additional comments (4)
frontend/services/push-notification-service.ts (2)
7-13: Good: user-scoped storage key + strongly-typed settings map
Keeps the cache per-user and makes UI/service integration harder to misuse.
201-230: LGTM: local best-effort + remote failure throws
This matches optimistic-update patterns (callers can roll back / show unsynced state).frontend/supabase/migrations/20250825200450_misty_boat.sql (2)
158-158: RLS enablement + owner-scoped policies look consistent
CRUD access is constrained toauth.uid() = user_id, which matches the frontend usage.Also applies to: 287-311
322-324: Indexes + updated_at trigger: LGTM
idx_push_tokens_user_idsupports per-user reads; trigger keepsupdated_atin sync.Also applies to: 350-354
| // Save push token to Supabase | ||
| static async savePushToken(token: string, userId: string): Promise<void> { | ||
| try { | ||
| const deviceId = Constants.installationId || Device.osName; | ||
| // Only persist tokens for native platforms we support | ||
| if (Platform.OS !== 'ios' && Platform.OS !== 'android') { | ||
| console.warn('Skipping push token save for unsupported platform:', Platform.OS); | ||
| return; | ||
| } | ||
|
|
||
| const platform: 'ios' | 'android' = Platform.OS; | ||
|
|
||
| const { error } = await supabase | ||
| .from('push_tokens') | ||
| .upsert({ | ||
| user_id: userId, | ||
| token: token, | ||
| platform: platform, | ||
| device_id: deviceId, | ||
| updated_at: new Date().toISOString(), | ||
| } as never, { | ||
| onConflict: 'user_id,device_id' | ||
| } as never); | ||
|
|
||
| if (error) { | ||
| console.error('Error saving push token:', error); | ||
| } else { | ||
| console.log('Push token saved successfully'); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error in savePushToken:', error); | ||
| } | ||
|
|
||
| // Remove push token (logout) | ||
| async removePushToken(userId: string): Promise<void> { | ||
| try { | ||
| const deviceId = Constants.installationId || Device.osName; | ||
|
|
||
| const { error } = await this.supabase | ||
| .from('push_tokens') | ||
| .delete() | ||
| .eq('user_id', userId) | ||
| .eq('device_id', deviceId); | ||
|
|
||
| if (error) { | ||
| console.error('Error removing push token:', error); | ||
| } else { | ||
| this.currentToken = null; | ||
| console.log('Push token removed successfully'); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error in removePushToken:', error); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
expo-constants 18.0.8 Constants.installationId documentation
💡 Result:
Short answer: Constants.installationId is deprecated — Expo recommends generating and storing your own installation ID (e.g., use expo-application.androidId on Android and a local storage API such as expo-secure-store on iOS / localStorage on web). The Constants API page and related Expo issues document this deprecation and the migration approach. [1][2]
Sources:
- Expo Constants docs (API/notes). [1]
- Expo GitHub issues discussing the deprecation and migration guidance. [2]
🏁 Script executed:
# Search for Device.osName usage in codebase to understand its purpose
rg -t ts 'Device\.osName' -B 2 -A 2Repository: fortune710/keepsafe
Length of output: 997
🏁 Script executed:
# Search for other push token or device ID handling patterns
rg -t ts 'push.*token|device.*id' -i --max-count 10Repository: fortune710/keepsafe
Length of output: 6913
🏁 Script executed:
# Check if there are any test files or other examples of device ID handling
fd -t ts -e spec -e test | head -20Repository: fortune710/keepsafe
Length of output: 232
🏁 Script executed:
# Get the complete removePushToken implementation
cat -n frontend/services/push-notification-service.ts | sed -n '100,150p'Repository: fortune710/keepsafe
Length of output: 1777
🏁 Script executed:
# Check the database schema definition
cat -n frontend/types/database.ts | grep -A 10 -B 5 "push_tokens"Repository: fortune710/keepsafe
Length of output: 562
🏁 Script executed:
# Search for Device import to understand what Device.osName actually is
rg -t ts "import.*Device|from.*Device" -A 1 -B 1Repository: fortune710/keepsafe
Length of output: 1876
Fix unreliable device ID logic and token logging
The current device_id approach has critical flaws that break token management:
-
Constants.installationIdis deprecated (Expo 18.0.8+). When it's unavailable,Device.osName(which returns "ios" or "android") is used as a fallback. This means all devices on the same platform will collide on the samedevice_id, causingremovePushToken()to delete all user tokens on that OS instead of just the current device's token. -
Token logging exposure:
console.log('Push token:', ...)in the hook exposes sensitive tokens in logs and crash reports. -
Silent error swallowing: Supabase errors are logged but not surfaced to callers. Persistence failures remain invisible, leaving the app in an inconsistent state.
Recommendations:
- Replace
Constants.installationId || Device.osNamewith a proper per-device identifier usingexpo-application(Android) andexpo-secure-store(iOS) as Expo's migration guidance suggests. - Remove all token and success message logging from this code path.
- Consider returning a status or throwing errors so callers can handle persistence failures.
| static async getNotificationSettings(userId: string): Promise<NotificationSettingsMap | null> { | ||
| try { | ||
| // 1. Try local storage first | ||
| const local = await deviceStorage.getItem<NotificationSettingsMap>( | ||
| NOTIFICATION_SETTINGS_STORAGE_KEY(userId), | ||
| ); | ||
| if (local) { | ||
| return local; | ||
| } | ||
|
|
||
| // 2. Fallback to Supabase (single row per user) | ||
| const { data, error } = await supabase | ||
| .from('notification_settings') | ||
| .select('friend_requests, push_notifications, entry_reminder, friend_activity') | ||
| .eq('user_id', userId) | ||
| .maybeSingle<{ | ||
| friend_requests: boolean | null; | ||
| push_notifications: boolean | null; | ||
| entry_reminder: boolean | null; | ||
| friend_activity: boolean | null; | ||
| }>(); | ||
|
|
||
| if (error) { | ||
| console.error('Error fetching notification settings from Supabase:', error); | ||
| return null; | ||
| } | ||
|
|
||
| if (!data) { | ||
| return null; | ||
| } | ||
|
|
||
| const fromRemote: NotificationSettingsMap = { | ||
| [NotificationSettings.FRIEND_REQUESTS]: data.friend_requests ?? true, | ||
| [NotificationSettings.PUSH_NOTIFICATIONS]: data.push_notifications ?? true, | ||
| [NotificationSettings.ENTRY_REMINDER]: data.entry_reminder ?? false, | ||
| [NotificationSettings.FRIEND_ACTIVITY]: data.friend_activity ?? true, | ||
| }; | ||
|
|
||
| // Cache remotely-loaded settings locally | ||
| await deviceStorage.setItem(NOTIFICATION_SETTINGS_STORAGE_KEY(userId), fromRemote); | ||
|
|
||
| return fromRemote; | ||
| } catch (error) { | ||
| console.error('Error in getNotificationSettings:', error); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t fail the whole read if local cache write fails
If deviceStorage.setItem(...) throws after a successful Supabase fetch, the catch returns null and you lose the remotely loaded settings.
const fromRemote: NotificationSettingsMap = {
[NotificationSettings.FRIEND_REQUESTS]: data.friend_requests ?? true,
[NotificationSettings.PUSH_NOTIFICATIONS]: data.push_notifications ?? true,
[NotificationSettings.ENTRY_REMINDER]: data.entry_reminder ?? false,
[NotificationSettings.FRIEND_ACTIVITY]: data.friend_activity ?? true,
};
// Cache remotely-loaded settings locally
- await deviceStorage.setItem(NOTIFICATION_SETTINGS_STORAGE_KEY(userId), fromRemote);
+ try {
+ await deviceStorage.setItem(NOTIFICATION_SETTINGS_STORAGE_KEY(userId), fromRemote);
+ } catch (e) {
+ console.error('Error caching remotely-loaded settings locally:', e);
+ }
return fromRemote;🤖 Prompt for AI Agents
In frontend/services/push-notification-service.ts around lines 149 to 195, the
function currently throws away remotely-fetched settings if
deviceStorage.setItem(...) fails (the outer try/catch returns null); change the
flow so the Supabase fetch result is returned regardless of local cache write
errors by isolating the local write in its own try/catch (or attach a .catch()
to the promise), log any setItem error but do not rethrow it, and always return
fromRemote after attempting to cache it.
| - `push_tokens` - Device push notification tokens | ||
| - `id` (uuid, primary key) | ||
| - `user_id` (uuid, references auth.users) | ||
| - `token` (text, Expo push token) | ||
| - `platform` (text: ios, android, web) | ||
| - `device_id` (text, optional) | ||
| - `created_at` (timestamp) | ||
| - `updated_at` (timestamp) | ||
|
|
There was a problem hiding this comment.
Schema matches the app’s onConflict: 'user_id,device_id', but nullable device_id is a footgun
Postgres UNIQUE allows multiple NULLs, so device_id being nullable means the “one row per device” guarantee can be silently broken (and upserts won’t conflict). Either make device_id NOT NULL (and ensure the app always provides one) or change the uniqueness/upsert key (e.g., (user_id, token) or a generated per-install id).
Also applies to: 140-150
🤖 Prompt for AI Agents
In frontend/supabase/migrations/20250825200450_misty_boat.sql around lines 54-62
(and also apply the same change to 140-150), the schema allows device_id to be
NULL which breaks the UNIQUE behavior used by the app’s onConflict:
'user_id,device_id' because Postgres permits multiple NULLs; fix by either
altering the schema to make device_id NOT NULL and update the app to always
supply a device identifier (and add a migration to backfill/validate existing
rows), or change the UNIQUE constraint/upsert key to a non-null combination such
as (user_id, token) or a generated per-install id and update the app’s upsert
logic to use that key; ensure corresponding application changes and migrations
are kept in sync.
| -- Create push_tokens table | ||
| CREATE TABLE IF NOT EXISTS push_tokens ( | ||
| id uuid PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| user_id uuid NOT NULL REFERENCES auth.users(id) ON DELETE CASCADE, | ||
| token text NOT NULL, | ||
| platform text CHECK (platform IN ('ios', 'android', 'web')), | ||
| device_id text, | ||
| created_at timestamptz DEFAULT now(), | ||
| updated_at timestamptz DEFAULT now(), | ||
| CONSTRAINT push_tokens_user_device_key UNIQUE (user_id, device_id) | ||
| ); |
There was a problem hiding this comment.
Avoid editing an older “initial schema” migration if it may already be applied
If any environment has already run 20250825200450_misty_boat.sql, changing it will drift schema history. Prefer a new forward-only migration (e.g., 20251212..._push_tokens.sql) that creates/alter tables, policies, indexes, and triggers idempotently.
Also applies to: 287-311, 322-324, 350-354
🤖 Prompt for AI Agents
In frontend/supabase/migrations/20250825200450_misty_boat.sql around lines
140-150 (also applies to blocks at 287-311, 322-324, 350-354): do not modify
this historical migration; instead create a new forward-only migration (e.g.,
20251212_add_push_tokens.sql) that performs the required changes (CREATE TABLE
IF NOT EXISTS or ALTER TABLE as appropriate), adds policies, indexes and
triggers idempotently, and documents intent; keep the original file unchanged to
avoid schema-history drift.
1 participant
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.