Fixed Streaks, Unautenticated Users and Blocked Friends#10
Conversation
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughPrefetch/onboarding guards tightened; music preview added with onMusicPress propagation from vault to popover; FriendService and blocked_by support added with DB migration and types; new Legal screen and settings tweaks; streak timezone removed; deep-linking logging/fallbacks adjusted. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Item as MusicCanvasItem
participant Canvas as VaultCanvas
participant Card as VaultEntryCard
participant Vault as Vault Screen
participant Popover as AudioPreviewPopover
User->>Item: taps music item
Item->>Canvas: onPress -> propagate music tag
Canvas->>Card: invoke onMusicPress with MusicTag
Card->>Vault: call onMusicPress handler
Vault->>Vault: set selectedMusic, isMusicPlayerVisible = true
Vault->>Popover: render isVisible=true with selectedMusic
Popover->>User: display preview
User->>Popover: tap backdrop / close
Popover->>Vault: onClose
Vault->>Vault: run exit animation, clear selectedMusic after delay
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/index.tsx (1)
10-11: Remove debug console logs before production.These debug logs should be removed or replaced with a proper logging solution for production code.
🔎 Proposed fix
- console.log("User: %s", user); - console.log("Session: %s", session); -
🧹 Nitpick comments (7)
frontend/services/streak-service.ts (2)
52-60: Inconsistent logging: useloggerinstead ofconsole.log.Line 56 uses
console.log, but line 169 useslogger.info. Consider usinglogger.debughere for consistency and better log management in production.🔎 Suggested fix
static async saveStreakData(userId: string, data: StreakData): Promise<void> { try { await deviceStorage.setItem(`streak_${userId}`, data); - console.log('Saved streak data:', data); + logger.debug('Saved streak data:', data); } catch (error) { console.error('Failed to save streak data:', error); } }
78-122: Misleading variable name and excessive debug logging.
todayStr(line 80) is derived fromentryDate, not necessarily "today". Consider renaming toentryDateStrfor clarity.Multiple
console.logcalls should uselogger.debugfor consistency and to allow proper log level control in production.🔎 Suggested improvements
static async updateStreak(userId: string, entryDate: Date, currentStreakData: StreakData): Promise<StreakData> { const now = new Date(); - const todayStr = format(startOfDay(entryDate), 'yyyy-MM-dd'); + const entryDateStr = format(startOfDay(entryDate), 'yyyy-MM-dd'); - console.log('Updating streak - Today:', todayStr, 'Last entry:', currentStreakData.lastEntryDate); + logger.debug('Updating streak - Entry date:', entryDateStr, 'Last entry:', currentStreakData.lastEntryDate); let newStreakData = { ...currentStreakData }; // First entry ever - start the streak! if (!newStreakData.lastEntryDate) { newStreakData.currentStreak = 1; newStreakData.maxStreak = 1; - newStreakData.lastEntryDate = todayStr; + newStreakData.lastEntryDate = entryDateStr; newStreakData.lastAccessTime = now.toISOString(); - console.log('First entry ever - streak set to 1'); + logger.debug('First entry ever - streak set to 1'); } else { // ... rest of the method - similar console.log → logger.debug changes }frontend/app/settings/blocked-users.tsx (1)
85-88: Consider adding error handling for avatar image loading.The
Imagecomponent lacks anonErrorprop. If the avatar URL fails to load (e.g., network issues, API downtime), users will see a broken image. Consider adding graceful error handling.🔎 Suggested enhancement
+ const [imageError, setImageError] = React.useState(false); + return ( <View key={friend.id} style={styles.userItem}> <View style={styles.avatarContainer}> - {avatarUrl ? ( + {avatarUrl && !imageError ? ( <Image source={{ uri: avatarUrl }} style={styles.avatar} + onError={() => setImageError(true)} /> ) : ( <View style={[styles.avatar, styles.avatarPlaceholder]}> <Text style={styles.avatarText}> {(friend.friend_profile?.full_name?.[0] || friend.friend_profile?.username?.[0] || '?').toUpperCase()} </Text> </View> )}Note: This suggestion assumes the unreachable code issue (previous comment) is addressed first.
frontend/app/settings/legal.tsx (2)
34-198: Consider externalizing legal document content.The legal content is currently hardcoded within the component with inline text spanning 160+ lines. This approach has several drawbacks:
- Maintenance burden: Updates require code changes and redeployment
- Stale dates: The "Last Updated: January 15, 2025" is hardcoded and won't update automatically
- No versioning: Cannot track content changes or show version history
- Testability: Difficult to test different content scenarios
💡 Recommended approaches
Option 1: Separate content files
Move content to JSON or TypeScript constant files:// constants/legal-content.ts export const legalContent = { terms: { lastUpdated: '2025-01-15', sections: [ { title: '1. Acceptance of Terms', text: '...' }, // ... ] }, // ... };Option 2: Remote content management
Fetch content from a backend API or CMS, enabling:
- Real-time updates without deployment
- Version tracking and audit trails
- A/B testing different legal language
- Automatic "last updated" timestamps
This would also allow non-technical team members to update legal documents.
267-267: Inconsistent scaling applied to title.The title uses
scale(16)but this results in a font size that may be smaller than intended. The other screens use fixed font sizes (e.g., line 92 in about.tsx usesfontSize: 20). Consider whether responsive scaling is necessary here or if a fixed size would be more consistent.🔎 Suggested fix for consistency
title: { - fontSize: scale(16), + fontSize: 20, fontWeight: '600', color: '#1E293B', },frontend/components/capture/canvas/music-canvas-item.tsx (1)
12-15: Consider using Pressable instead of TouchableOpacity.While TouchableOpacity works well, the React Native community now recommends
Pressablefor new code as it offers better performance, more flexibility (custom feedback, hitSlop), and is the modern standard.🔎 Proposed refactor to Pressable
-import { StyleSheet, Text, TouchableOpacity } from "react-native"; +import { StyleSheet, Text, Pressable } from "react-native";- <TouchableOpacity style={styles.musicContainer} onPress={onPress}> + <Pressable style={styles.musicContainer} onPress={onPress}> <Image source={{ uri: music.cover }} style={styles.musicImage} /> <Text style={styles.textStyle}>{music.title} - {music.artist}</Text> - </TouchableOpacity> + </Pressable>frontend/hooks/use-friends.ts (1)
70-80: Avoidanytype andas nevercast for type safety.Using
anyand casting toneverbypasses TypeScript's type checking. Consider using the proper Database types for the update payload.🔎 Proposed fix using proper types
- const updateData: any = { status }; - if (blocked_by !== undefined) { - updateData.blocked_by = blocked_by; - } + const updateData: Database['public']['Tables']['friendships']['Update'] = { + status, + ...(blocked_by !== undefined && { blocked_by }) + }; const { error } = await supabase .from(TABLES.FRIENDSHIPS) - .update(updateData as never) + .update(updateData) .eq('id', id);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
frontend/app/index.tsxfrontend/app/settings/about.tsxfrontend/app/settings/blocked-users.tsxfrontend/app/settings/index.tsxfrontend/app/settings/legal.tsxfrontend/app/vault.tsxfrontend/components/capture/canvas/music-canvas-item.tsxfrontend/components/capture/canvas/vault-canvas.tsxfrontend/components/capture/music/audio-preview-popover.tsxfrontend/components/entries/vault-entry-card.tsxfrontend/hooks/use-deep-linking.tsfrontend/hooks/use-friends.tsfrontend/hooks/use-streak-tracking.tsfrontend/services/friend-service.tsfrontend/services/streak-service.tsfrontend/supabase/migrations/20251220000000_add_blocked_by.sqlfrontend/types/database.tsfrontend/types/friends.ts
💤 Files with no reviewable changes (1)
- frontend/hooks/use-streak-tracking.ts
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/app/settings/blocked-users.tsx (1)
frontend/lib/utils.ts (1)
getDefaultAvatarUrl(6-8)
frontend/hooks/use-friends.ts (3)
frontend/constants/supabase.ts (2)
FRIENDSHIP_STATUS(31-36)TABLES(4-15)frontend/lib/supabase.ts (1)
supabase(14-30)frontend/lib/logger.ts (1)
error(32-35)
frontend/app/vault.tsx (1)
frontend/components/capture/music/audio-preview-popover.tsx (1)
AudioPreviewPopover(25-77)
frontend/types/friends.ts (1)
frontend/types/database.ts (1)
Database(11-334)
frontend/components/capture/canvas/vault-canvas.tsx (1)
frontend/components/capture/canvas/music-canvas-item.tsx (1)
MusicCanvasItem(10-17)
🔇 Additional comments (22)
frontend/services/streak-service.ts (4)
1-29: LGTM! Well-documented service with clear business logic.The documentation clearly explains the streak logic, and the simplified
StreakDatainterface (after removing timezone handling) is clean and appropriate for local device storage.
31-50: LGTM!The method correctly returns cached data when available or sensible defaults otherwise.
144-180: LGTM! Logic correctly handles streak validation on app access.The streak check logic is sound:
daysSinceLastEntry = 1maintains the streak (user has until end of day to create an entry)daysSinceLastEntry > 1resets the streak to 0Good addition of updating
lastAccessTimein lines 172-179 even when streak is still valid.
182-193: LGTM!Correctly resets the current streak while preserving
maxStreakfor historical tracking.frontend/app/settings/blocked-users.tsx (2)
10-10: LGTM!The import is appropriate for generating fallback avatar URLs using the DiceBear API.
206-215: LGTM! Styles are well-defined.The avatar placeholder styles are appropriate. However, note that these styles are currently unused due to the unreachable code issue identified in lines 79-95. Once the logic is corrected, these styles will function as intended.
frontend/app/settings/index.tsx (1)
48-55: LGTM - Clean removal with preserved reference.The Storage & Data settings item has been cleanly removed from the navigation flow while preserving the original code as a commented reference. This aligns with the PR's broader settings restructuring.
frontend/app/index.tsx (1)
33-33: LGTM - Improved authentication gating.The updated condition (
!user && !session) correctly ensures that users are redirected to onboarding only when both authentication indicators are absent, preventing premature redirects during initialization.frontend/app/settings/about.tsx (1)
2-6: LGTM - Clean refactor to internal navigation.The changes successfully transition from external URL linking to internal routing for legal documents, and introduce responsive scaling for better cross-device support. The consolidated imports and updated UI elements align well with the new legal documents screen.
Also applies to: 43-49, 85-86
frontend/app/settings/legal.tsx (1)
10-32: LGTM - Well-structured legal documents screen.The two-panel navigation pattern is intuitive, with clean separation between the document list and detail views. The conditional back button behavior appropriately handles both navigation contexts. The component structure is clear and maintainable despite the inline content.
Also applies to: 200-249
frontend/components/capture/music/audio-preview-popover.tsx (2)
32-33: LGTM: Exit animation logic is well-implemented.The conditional return allows the exit animation to complete before unmounting. The logic correctly keeps the component mounted when
musicexists butisVisibleis false, enabling the SlideOutDown animation to play. This coordinates well with thecloseMusicPlayersetTimeout in vault.tsx.
38-44: LGTM: Pointer events and backdrop interaction are correctly configured.The
pointerEventstoggle prevents interaction with the hidden overlay, while the backdrop remains accessible to triggeronClose. TheactiveOpacity={1}prevents visual feedback on the backdrop, which is appropriate for a dismissal area.frontend/components/entries/vault-entry-card.tsx (1)
16-16: LGTM: Clean callback propagation with proper typing.The
onMusicPresscallback is correctly typed withMusicTag, properly propagated through the component tree, and only passed toVaultCanvasfor photo-type entries. The implementation is type-safe and follows good practices.Also applies to: 37-37, 49-49, 209-211
frontend/app/vault.tsx (1)
58-61: LGTM: Clean music playback integration at screen level.The music player state management and callback flow are well-structured. Disabling scroll while the music player is visible (line 117) improves the user experience, and the
onMusicPresscallback is properly threaded through toVaultEntryCard. TheAudioPreviewPopoverconditional rendering is clean.Also applies to: 117-117, 134-134, 181-187
frontend/components/capture/canvas/vault-canvas.tsx (2)
5-5: LGTM: Type-safe callback propagation through the canvas component tree.The
onMusicPresscallback is correctly:
- Typed with
MusicTagfor type safety- Propagated from
VaultCanvas→VaultCanvasItem→MusicCanvasItem- Made optional at each level with proper optional chaining
- Threaded through the component hierarchy cleanly
Also applies to: 15-16, 19-19, 54-55, 65-65, 68-68
94-97: LGTM: Proper null checking in music press handler.The
onPresshandler correctly guards against null/undefined values by checking bothitem.music_tagexistence and using optional chaining ononMusicPress. This prevents runtime errors if either the music tag or callback is missing.frontend/types/friends.ts (1)
36-41: LGTM! Clean type definitions.The type aliases and
FriendWithProfileinterface properly leverage the Database types, ensuring type safety and alignment with the schema changes (including the newblocked_byfield).frontend/types/database.ts (1)
103-130: LGTM! Type definitions align with migration.The
blocked_byfield is correctly added to Row, Insert, and Update types with appropriate nullability, matching the database migration.frontend/hooks/use-friends.ts (2)
149-163: LGTM! Block action correctly setsblocked_by.The
blockFriendfunction properly setsblocked_by: userIdto track who initiated the block, and the dependency array correctly includesuserId.
165-179: LGTM! Unblock correctly clearsblocked_by.Setting
blocked_by: nullwhen unblocking and reverting status toACCEPTEDis the correct behavior.frontend/services/friend-service.ts (2)
158-184: LGTM! Clean friend request implementation.The validation, logging, and error handling are well-structured. The use of
FRIENDSHIP_STATUS.PENDINGconstant ensures consistency.
198-215: LGTM! Clean friend removal implementation.Good use of logger and proper error propagation with
throw error.
| const avatarUrl = friend.friend_profile?.avatar_url ?? getDefaultAvatarUrl(friend.friend_profile?.full_name ?? ""); | ||
|
|
||
| return ( | ||
| <View key={friend.id} style={styles.userItem}> | ||
| <View style={styles.avatarContainer}> | ||
| <Image | ||
| source={{ uri: friend.friend_profile?.avatar_url || undefined }} | ||
| style={styles.avatar} | ||
| /> | ||
| {avatarUrl ? ( | ||
| <Image | ||
| source={{ uri: avatarUrl }} | ||
| style={styles.avatar} | ||
| /> | ||
| ) : ( | ||
| <View style={[styles.avatar, styles.avatarPlaceholder]}> | ||
| <Text style={styles.avatarText}> | ||
| {(friend.friend_profile?.full_name?.[0] || friend.friend_profile?.username?.[0] || '?').toUpperCase()} | ||
| </Text> | ||
| </View> | ||
| )} |
There was a problem hiding this comment.
Unreachable fallback placeholder code.
The placeholder rendering logic (lines 90-94) is unreachable because avatarUrl is always assigned a value on line 79. The nullish coalescing operator ensures avatarUrl is either friend.friend_profile?.avatar_url or a generated DiceBear URL, so the conditional check on line 84 will always evaluate to true.
🔎 Recommended fix: Choose one fallback strategy
Option 1: Use only the DiceBear fallback (simplest)
Remove the conditional logic since avatarUrl always has a value:
- const avatarUrl = friend.friend_profile?.avatar_url ?? getDefaultAvatarUrl(friend.friend_profile?.full_name ?? "");
+ const avatarUrl = friend.friend_profile?.avatar_url ?? getDefaultAvatarUrl(friend.friend_profile?.full_name ?? friend.friend_profile?.username ?? "");
return (
<View key={friend.id} style={styles.userItem}>
<View style={styles.avatarContainer}>
- {avatarUrl ? (
- <Image
- source={{ uri: avatarUrl }}
- style={styles.avatar}
- />
- ) : (
- <View style={[styles.avatar, styles.avatarPlaceholder]}>
- <Text style={styles.avatarText}>
- {(friend.friend_profile?.full_name?.[0] || friend.friend_profile?.username?.[0] || '?').toUpperCase()}
- </Text>
- </View>
- )}
+ <Image
+ source={{ uri: avatarUrl }}
+ style={styles.avatar}
+ />
</View>Option 2: Use placeholder only when no avatar_url exists
Reserve the colored placeholder for when there's no actual avatar:
- const avatarUrl = friend.friend_profile?.avatar_url ?? getDefaultAvatarUrl(friend.friend_profile?.full_name ?? "");
+ const avatarUrl = friend.friend_profile?.avatar_url;
return (
<View key={friend.id} style={styles.userItem}>
<View style={styles.avatarContainer}>
{avatarUrl ? (
<Image
source={{ uri: avatarUrl }}
style={styles.avatar}
/>
) : (
<View style={[styles.avatar, styles.avatarPlaceholder]}>
<Text style={styles.avatarText}>
{(friend.friend_profile?.full_name?.[0] || friend.friend_profile?.username?.[0] || '?').toUpperCase()}
</Text>
</View>
)}
</View>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const avatarUrl = friend.friend_profile?.avatar_url ?? getDefaultAvatarUrl(friend.friend_profile?.full_name ?? ""); | |
| return ( | |
| <View key={friend.id} style={styles.userItem}> | |
| <View style={styles.avatarContainer}> | |
| <Image | |
| source={{ uri: friend.friend_profile?.avatar_url || undefined }} | |
| style={styles.avatar} | |
| /> | |
| {avatarUrl ? ( | |
| <Image | |
| source={{ uri: avatarUrl }} | |
| style={styles.avatar} | |
| /> | |
| ) : ( | |
| <View style={[styles.avatar, styles.avatarPlaceholder]}> | |
| <Text style={styles.avatarText}> | |
| {(friend.friend_profile?.full_name?.[0] || friend.friend_profile?.username?.[0] || '?').toUpperCase()} | |
| </Text> | |
| </View> | |
| )} | |
| const avatarUrl = friend.friend_profile?.avatar_url; | |
| return ( | |
| <View key={friend.id} style={styles.userItem}> | |
| <View style={styles.avatarContainer}> | |
| {avatarUrl ? ( | |
| <Image | |
| source={{ uri: avatarUrl }} | |
| style={styles.avatar} | |
| /> | |
| ) : ( | |
| <View style={[styles.avatar, styles.avatarPlaceholder]}> | |
| <Text style={styles.avatarText}> | |
| {(friend.friend_profile?.full_name?.[0] || friend.friend_profile?.username?.[0] || '?').toUpperCase()} | |
| </Text> | |
| </View> | |
| )} | |
| </View> |
🤖 Prompt for AI Agents
In frontend/app/settings/blocked-users.tsx around lines 79 to 95, the
placeholder branch is unreachable because avatarUrl is always set using the
nullish coalescing to a generated URL; either (A) always render the <Image> by
removing the conditional and keep the DiceBear fallback (delete the placeholder
branch), or (B) change the avatarUrl assignment to only use
friend.friend_profile?.avatar_url (remove the DiceBear default) so the existing
conditional can render the colored placeholder when avatarUrl is null; update
the avatarUrl computation and remove or preserve branches accordingly to match
the chosen strategy.
| static async getFriends(userId: string): Promise<FriendWithProfile[]> { | ||
| if (!userId) { | ||
| logger.error('User ID is required to retrive friends'); | ||
| throw new Error('User ID is required'); | ||
| } |
There was a problem hiding this comment.
Typo: "retrive" should be "retrieve".
🔎 Proposed fix
- logger.error('User ID is required to retrive friends');
+ logger.error('User ID is required to retrieve friends');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static async getFriends(userId: string): Promise<FriendWithProfile[]> { | |
| if (!userId) { | |
| logger.error('User ID is required to retrive friends'); | |
| throw new Error('User ID is required'); | |
| } | |
| static async getFriends(userId: string): Promise<FriendWithProfile[]> { | |
| if (!userId) { | |
| logger.error('User ID is required to retrieve friends'); | |
| throw new Error('User ID is required'); | |
| } |
🤖 Prompt for AI Agents
In frontend/services/friend-service.ts around lines 91 to 95, the logger message
contains a typo "retrive" — update the string passed to logger.error to "User ID
is required to retrieve friends" (correcting "retrive" to "retrieve"); leave the
thrown Error text as-is unless you also want to make it consistent.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
frontend/services/friend-service.ts (2)
123-127: Typo: "retrive" should be "retrieve".🔎 Proposed fix
- logger.error('User ID is required to retrive friends'); + logger.error('User ID is required to retrieve friends');
156-177: Critical: Error check occurs after data processing — potential null dereference.
data?.map()on line 156 processes before checkingerror(line 175) or handling!data(line 173). If the query fails,datacould be null/undefined. While optional chaining prevents immediate crash on.map(), thefriendsvariable would beundefined, and the logic flow is confusing.🔎 Proposed fix: Check error before processing data
.or(`user_id.eq.${userId},friend_id.eq.${userId}`) .order('created_at', { ascending: false }) as { data: any[], error: any, }; + if (error) { + throw new Error(error.message); + } + + if (!data) { + return []; + } + const friends: FriendWithProfile[] = data.map(friend => { const { user_profile: user, friend_profile: friend_, ...friend_record } = friend; const profile = friend.friend_id === userId ? user : friend_; return { ...friend_record, friend_profile: profile } }) - if (!data) return []; - - if (error) { - throw new Error(error.message); - } - // Cache the friends data - if (data) { - await deviceStorage.setFriends(userId, friends); - } + await deviceStorage.setFriends(userId, friends); return friends as FriendWithProfile[];
🧹 Nitpick comments (3)
frontend/hooks/use-friends.ts (2)
69-81: Avoid usinganytype andas nevertype assertions.Using
anyforupdateDataandas neverfor the update call bypasses TypeScript's type safety. Consider defining a proper type for the update payload.🔎 Proposed fix
- mutationFn: async ({ id, status, blocked_by }: { id: string; status: typeof FRIENDSHIP_STATUS.ACCEPTED | typeof FRIENDSHIP_STATUS.DECLINED | typeof FRIENDSHIP_STATUS.BLOCKED; blocked_by?: string | null }) => { + mutationFn: async ({ id, status, blocked_by }: { + id: string; + status: typeof FRIENDSHIP_STATUS.ACCEPTED | typeof FRIENDSHIP_STATUS.DECLINED | typeof FRIENDSHIP_STATUS.BLOCKED; + blocked_by?: string | null + }) => { if (__DEV__) console.log('Updating friendship:', { id, status, blocked_by }); - const updateData: any = { status }; + const updateData: { status: string; blocked_by?: string | null } = { status }; if (blocked_by !== undefined) { updateData.blocked_by = blocked_by; } const { error } = await supabase .from(TABLES.FRIENDSHIPS) - .update(updateData as never) + .update(updateData) .eq('id', id);
197-201: Remove debugconsole.logfrom production code.The
console.log({ contacts })on line 199 will output to production logs. Consider wrapping it in__DEV__or removing it.🔎 Proposed fix
const contacts = await FriendService.getSuggestedFriendsFromContacts(); - console.log({ contacts }); + if (__DEV__) console.log({ contacts }); return contacts.filter(contact => contact.id !== profile?.id);frontend/services/friend-service.ts (1)
200-207: Avoidas nevertype assertion.The
as nevercast on line 206 bypasses type checking. Consider using proper typing for the insert payload.🔎 Proposed fix
const { error } = await supabase .from(TABLES.FRIENDSHIPS) .insert({ user_id: userId, friend_id: friendId, status: FRIENDSHIP_STATUS.PENDING, - } as never) + }) .select() .single();If TypeScript complains, ensure the
Databasetypes in@/types/database.tsare properly generated from your Supabase schema.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/app/index.tsxfrontend/app/settings/blocked-users.tsxfrontend/hooks/use-friends.tsfrontend/services/friend-service.tsfrontend/supabase/migrations/20251220000000_add_blocked_by.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/settings/blocked-users.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/hooks/use-friends.ts (1)
frontend/constants/supabase.ts (2)
FRIENDSHIP_STATUS(31-36)TABLES(4-15)
frontend/services/friend-service.ts (3)
frontend/types/friends.ts (2)
Profile(37-37)FriendWithProfile(39-41)frontend/lib/logger.ts (2)
logger(18-36)error(32-35)frontend/constants/supabase.ts (2)
TABLES(4-15)FRIENDSHIP_STATUS(31-36)
🔇 Additional comments (8)
frontend/app/index.tsx (1)
16-20: No action needed —prefetchSuggestedFriendsis properly memoized.The function is wrapped in
useCallbackwithin theuseFriendshook (with dependencies[queryClient, profile?.id]), making it stable across renders. The dependency array now correctly includes it, resolving the earlier exhaustive-deps warning.Likely an incorrect or invalid review comment.
frontend/supabase/migrations/20251220000000_add_blocked_by.sql (2)
13-21: Good implementation of the WITH CHECK clause.The policy now correctly restricts
blocked_bymanipulation: users can only set it toNULLor their ownauth.uid(), preventing unauthorized attribution of blocks. This addresses the previous review feedback.
4-8: LGTM: Schema changes are well-structured.The
blocked_bycolumn withON DELETE SET NULLmaintains referential integrity gracefully, and the index will optimize queries filtering by blocker.frontend/hooks/use-friends.ts (2)
40-46: Good refactor: Delegating to FriendService.The query now cleanly delegates to
FriendService.getFriends(), improving separation of concerns. Theenabled: !!userIdguard ensures the non-null assertion on line 43 is safe.
52-54: Good implementation of blocked friends filtering.The filter correctly identifies friends blocked by the current user using
blocked_by === userId, aligning with the new schema.frontend/services/friend-service.ts (3)
17-47: Good addition of validation helpers.The
isValidEmail,isValidPhoneNumber, andisValidUUIDvalidators address the previous review concerns about untrusted input being interpolated into queries. The implementations are practical and cover common edge cases.
268-283: Good input validation before query construction.The contact data is now properly validated and filtered using the new helper methods before being used in the query, addressing the previous security concerns about untrusted device contact data.
294-310: Good defensive query building with validated data.The query construction now only adds
.or()and.not()clauses when valid data exists, preventing malformed queries and potential injection issues.
| const blockFriend = useCallback(async (friendshipId: string) => { | ||
| try { | ||
| await updateFriendshipMutation.mutateAsync({ id: friendshipId, status: FRIENDSHIP_STATUS.BLOCKED }); | ||
| await updateFriendshipMutation.mutateAsync({ | ||
| id: friendshipId, | ||
| status: FRIENDSHIP_STATUS.BLOCKED, | ||
| blocked_by: userId! | ||
| }); | ||
| return { success: true }; | ||
| } catch (error) { | ||
| return { | ||
| success: false, | ||
| error: error instanceof Error ? error.message : 'Failed to block friend', | ||
| }; | ||
| } | ||
| }, [updateFriendshipMutation]); | ||
| }, [updateFriendshipMutation, userId]); |
There was a problem hiding this comment.
Non-null assertion on userId could cause runtime error.
blockFriend uses userId! but this callback can be invoked by consumers even when userId is undefined. Consider adding a guard or returning an error early.
🔎 Proposed fix
const blockFriend = useCallback(async (friendshipId: string) => {
+ if (!userId) {
+ return { success: false, error: 'User ID is required to block a friend' };
+ }
try {
await updateFriendshipMutation.mutateAsync({
id: friendshipId,
status: FRIENDSHIP_STATUS.BLOCKED,
- blocked_by: userId!
+ blocked_by: userId
});
return { success: true };🤖 Prompt for AI Agents
In frontend/hooks/use-friends.ts around lines 149 to 163, the callback uses a
non-null assertion userId! which can crash if userId is undefined; add an
explicit guard at the start of blockFriend to check for a valid userId and
immediately return a failure result (or throw a clear error) when missing,
remove the non-null assertion from the mutate payload, and keep userId in the
dependency array so the callback updates when auth state changes.
| logger.info(`Getting retriving saved friends for ${session.user.id}`); | ||
| const friends = await deviceStorage.getFriends(session.user.id); |
There was a problem hiding this comment.
Typo: "retriving" should be "retrieving".
🔎 Proposed fix
- logger.info(`Getting retriving saved friends for ${session.user.id}`);
+ logger.info(`Retrieving saved friends for ${session.user.id}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.info(`Getting retriving saved friends for ${session.user.id}`); | |
| const friends = await deviceStorage.getFriends(session.user.id); | |
| logger.info(`Retrieving saved friends for ${session.user.id}`); | |
| const friends = await deviceStorage.getFriends(session.user.id); |
🤖 Prompt for AI Agents
In frontend/services/friend-service.ts around lines 285 to 286, there's a typo
in the log message: "Getting retriving saved friends for ..." should read
"Getting retrieving saved friends for ...". Update the logger.info string to
correct "retriving" to "retrieving" (keep the rest of the message as-is) so logs
are spelled correctly.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.