[f] improve behavior and update liveblocks#80
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request introduce a new public route for displaying audio snippets, represented by the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant PublicSnippet
participant AudioPlayer
User->>App: Navigate to /p/:snippetId
App->>PublicSnippet: Render with snippetId
PublicSnippet->>PublicSnippet: Fetch snippet data
PublicSnippet-->>User: Display snippet information
User->>AudioPlayer: Play audio
AudioPlayer->>AudioPlayer: Set current time based on startTime
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (5)
src/lib/storage.ts (1)
1-17: Consider architectural improvements for better maintainability.Since these utilities are used across multiple components, consider these architectural improvements:
- Create a
StorageKeysenum/const object for commonly used keys- Add JSDoc comments describing the purpose and usage
- Consider grouping these utilities in a class/namespace
Example implementation:
/** Keys used for local storage */ export const StorageKeys = { UPVOTE_COUNT: 'upvoteCount', IS_STARRED: 'isStarred', // Add other keys as needed } as const; /** Utilities for managing browser's local storage */ export class StorageUtils { // ... existing functions with added JSDoc ... }This would provide better maintainability and help prevent typos in storage keys.
🧰 Tools
🪛 eslint
[error] 11-11: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/components/LabelButton.tsx (1)
Line range hint
45-71: Enhance upvote handling with loading state and user feedback.While the optimistic updates are good, the user experience could be improved.
Consider these enhancements:
+const [isLoading, setIsLoading] = useState(false); + const handleUpvote = async (e: React.MouseEvent) => { e.stopPropagation() - if (!user) return // Prevent upvoting if not logged in + if (!user) { + toast.error('Please login to upvote'); + return; + } + if (isLoading) return; const newIsUpvoted = !isUpvoted // Optimistic updates setIsUpvoted(newIsUpvoted) setUpvoteCount(prevCount => (newIsUpvoted ? prevCount + 1 : prevCount - 1)) + setIsLoading(true) try { const { data, error } = await supabase.rpc('toggle_upvote_label', { snippet_id: snippetId, label_text: label.text }) if (error) throw error if (!data || (Array.isArray(data) && data.length === 0) || (data.labels && data.labels.length === 0)) { onLabelDeleted(label.id) } } catch (error) { console.error('Error toggling upvote:', error) // Revert optimistic updates setIsUpvoted(!newIsUpvoted) setUpvoteCount(prevCount => (newIsUpvoted ? prevCount - 1 : prevCount + 1)) + toast.error('Failed to update upvote. Please try again.') + } finally { + setIsLoading(false) } }src/components/SnippetCard.tsx (1)
31-33: Add error handling for local storage operations.Local storage operations can fail (e.g., in private browsing mode or when storage is full).
useEffect(() => { - setLocalStorageItem(`starred_${snippet.id}`, isStarred) + try { + setLocalStorageItem(`starred_${snippet.id}`, isStarred) + } catch (error) { + console.warn('Failed to save starred state:', error) + } }, [isStarred, snippet.id])src/components/SnippetDetail.tsx (1)
Based on the gathered information, I can now provide the final response:
Accessibility concerns are partially addressed, but security improvements needed
The component uses Radix UI's accessible dropdown menu implementation (
@radix-ui/react-dropdown-menu), which provides built-in keyboard navigation and ARIA attributes. However, some improvements are still needed:
Security:
- Add authentication checks before download operations (auth hooks are present in the codebase but not used here)
- Implement download URL validation for the audio files
Accessibility:
- Add aria-label to the "Back" button to describe its action
- Add descriptive aria-label to the Star button (currently only has sr-only text)
- Include download format information in the dropdown menu items for screen readers
Example fix for the Star button:
<Button variant='ghost' size='icon' aria-label="Add to favorites">🔗 Analysis chain
Line range hint
67-82: Enhance security and accessibility for interactive elements.The download functionality and interactive elements need improvements:
Security:
- Add CSRF protection for download functionality
- Validate file paths before download
Accessibility:
- Add aria-labels for interactive buttons
- Ensure keyboard navigation works for the dropdown menu
- Add screen reader announcements for download progress
Let's verify the current accessibility state:
Let me try to find the specific component and its dependencies to analyze the accessibility implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for aria-labels and role attributes in the codebase rg -i 'aria-|role=' --type tsxLength of output: 60
Script:
#!/bin/bash # Search for aria-labels and role attributes in the codebase with correct file extension rg -i 'aria-|role=' --type ts --type tsx --type jsx --type js # Let's also check if we're using any accessibility-related components or hooks rg -i 'useAria|aria|accessible|screen.?reader' --type ts --type tsx --type jsx --type js # Check for dropdown menu implementation details ast-grep --pattern 'DropdownMenu'Length of output: 1979
Script:
#!/bin/bash # Find the SnippetDetail component and its imports fd "SnippetDetail" --type f --exec cat {} # Look for any existing CSRF protection implementation rg -i "csrf|token|security" --type ts --type tsx --type jsx --type jsLength of output: 5793
Script:
#!/bin/bash # Let's check the dropdown-menu component implementation for accessibility features cat src/components/ui/dropdown-menu.tsx # Check if we're using any authentication/security middleware or utilities fd "auth|security" --type f # Let's also check the download implementation rg -B 5 -A 5 "download" --type ts --type tsx --type jsx --type jsLength of output: 7516
src/layouts/AuthenticatedLayout.tsx (1)
48-50: Ensure navigation to login occurs only onceThe navigation to
/logininsidesupabase.auth.getSession()may cause unexpected behavior if the component re-renders. Consider ensuring that navigation happens only if the component is mounted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
src/App.tsx(2 hunks)src/components/AudioPlayer.tsx(3 hunks)src/components/LabelButton.tsx(3 hunks)src/components/LiveblocksComments.tsx(2 hunks)src/components/PublicSnippet.tsx(1 hunks)src/components/SearchInterface.tsx(1 hunks)src/components/SnippetCard.tsx(3 hunks)src/components/SnippetDetail.tsx(1 hunks)src/layouts/AuthenticatedLayout.tsx(3 hunks)src/lib/storage.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/SearchInterface.tsx
🧰 Additional context used
🪛 eslint
src/components/LiveblocksComments.tsx
[error] 45-45: Unsafe assignment of an any value.
(@typescript-eslint/no-unsafe-assignment)
src/components/PublicSnippet.tsx
[error] 123-123: Unsafe member access .context on an any value.
(@typescript-eslint/no-unsafe-member-access)
src/layouts/AuthenticatedLayout.tsx
[error] 102-102: Unsafe member access .email on an any value.
(@typescript-eslint/no-unsafe-member-access)
src/lib/storage.ts
[error] 11-11: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🔇 Additional comments (19)
src/App.tsx (2)
19-19: LGTM!
The import statement is clean and follows the existing import organization pattern.
34-34: Verify route ordering to prevent path conflicts.
The new public route for snippets looks good and aligns with the VER-36 requirement. However, let's verify that there are no path conflicts with other routes, especially with the authenticated snippet route.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the route definitions. The ripgrep search shows all routes in the application, allowing us to verify the route ordering and potential conflicts.
Route ordering is correct and no path conflicts detected
The public snippet route /p/:snippetId is correctly positioned before the authenticated routes (which are nested within AuthenticatedLayout), and there are no conflicting patterns. The route ordering ensures that:
- Public routes (
/p/:snippetId) are evaluated first - Authenticated routes (
/snippet/:snippetId) are properly nested and won't interfere - No other routes use the
/p/pattern that could cause conflicts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential route path conflicts
# Expected: No other routes should match the pattern '/p/*'
# Test 1: Search for all route definitions
echo "Checking all route definitions:"
rg -A 1 '<Route\s+path=' src/
# Test 2: Search for any other usage of '/p/' path pattern
echo -e "\nChecking for '/p/' path pattern usage:"
rg '/p/' src/ --type ts --type tsx
Length of output: 1083
src/components/LabelButton.tsx (3)
1-8: LGTM! Clean import organization and interface definition.
The storage utility imports and component interface are well-structured.
Line range hint 73-103: LGTM! Clean and accessible UI implementation.
The UI rendering logic is well-structured with proper accessibility and state handling.
Line range hint 1-103: Verify public view behavior aligns with VER-36 requirements.
Since this component is used in the new public view (VER-36), let's verify its integration.
✅ Verification successful
Based on the evidence gathered, I can now provide a final response:
LabelButton implementation correctly handles both authenticated and public contexts
The component has proper authentication checks and optimistic updates:
- Guards upvote actions with
if (!user) return - Maintains local state with localStorage for better UX
- Handles both authenticated and non-authenticated states gracefully
- Is correctly integrated in both SnippetCard and SnippetDetail components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how LabelButton is used in public views
echo "Checking LabelButton usage in public components..."
rg -l "LabelButton" src/components/Public*
# Verify proper auth checks
echo "Verifying auth checks..."
ast-grep --pattern 'if (!user) { $$$; return; }'
Length of output: 305
Script:
#!/bin/bash
# Let's check the actual usage context in public components
echo "Checking LabelButton usage in PublicSnippet.tsx..."
rg -A 5 "LabelButton" src/components/PublicSnippet.tsx
echo "\nChecking LabelButton usage in PublicHeader.tsx..."
rg -A 5 "LabelButton" src/components/PublicHeader.tsx
# Let's also verify if there are any specific public view handlers
echo "\nChecking for public view specific handlers..."
rg "isPublic|public view" src/components/LabelButton.tsx
Length of output: 496
Script:
#!/bin/bash
# Let's try a different approach to find the component usage
echo "Checking for any imports of LabelButton..."
rg "import.*LabelButton" -A 5
# Also check for any public view related code in the parent components
echo "\nChecking for public view context in parent components..."
rg -l "isPublic.*=.*true|PUBLIC_VIEW" src/
# Let's verify the actual component rendering
echo "\nChecking LabelButton rendering in public components..."
ast-grep --pattern 'const PublicSnippet = $$$' src/components/PublicSnippet.tsx
Length of output: 1950
Script:
#!/bin/bash
# Let's check the actual implementation of SnippetCard and SnippetDetail
echo "Checking LabelButton usage in SnippetCard..."
rg "LabelButton" -B 2 -A 10 src/components/SnippetCard.tsx
echo "\nChecking LabelButton usage in SnippetDetail..."
rg "LabelButton" -B 2 -A 10 src/components/SnippetDetail.tsx
# Let's also verify if there's any public route handling
echo "\nChecking for public routes..."
rg "public.*route|isPublic" src/App.tsx src/routes/*
Length of output: 3349
src/components/SnippetCard.tsx (3)
1-1: LGTM! Imports are properly organized.
Also applies to: 12-12
95-95: LGTM! Event handler is properly bound.
Line range hint 1-95: Verify integration with LiveblocksComments component.
The changes to SnippetCard align with the PR objectives. Since this PR also involves changes to the liveblocks notification system (VER-35) and separate liveblocks rooms for comments (VER-112), let's verify the integration with LiveblocksComments.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
LiveblocksComments integration is properly implemented
The verification shows that:
- LiveblocksComments is correctly imported and used in both SnippetCard and SnippetDetail components
- The component receives the required
snippetIdprop consistently across usages - Room separation is maintained by passing unique snippet IDs to each LiveblocksComments instance
- The integration supports both collapsed (SnippetCard) and expanded (SnippetDetail) comment views through the
showFullCommentsprop
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify LiveblocksComments integration and room separation
# Test 1: Check if LiveblocksComments is properly imported and used
rg -A 5 "LiveblocksComments" src/
# Test 2: Check for room configuration in LiveblocksComments
ast-grep --pattern 'LiveblocksComments snippetId={$_}'
Length of output: 3134
src/components/SnippetDetail.tsx (2)
114-117: LGTM: Audio player enhancement with start time.
The addition of the startTime prop to the AudioPlayer component is a good improvement that allows for more precise audio playback control.
Line range hint 182-182: LGTM: Proper integration with LiveblocksComments.
The implementation correctly passes the snippetId to LiveblocksComments, which aligns with the requirement to have separate liveblocks rooms for each snippet (VER-112).
src/components/AudioPlayer.tsx (3)
13-13: LGTM: Well-typed optional prop addition
The optional startTime prop is correctly typed as string to handle the HH:MM:SS format.
16-16: LGTM: Component signature properly updated
The function signature correctly destructures the new startTime prop, maintaining consistency with the existing pattern.
46-46: 🛠️ Refactor suggestion
Consider handling audio loading state
The effect runs when startTime changes, but audio.duration might not be available immediately. Consider adding a check for audio readiness.
To verify if this is a common issue in the codebase:
Consider this improvement:
useEffect(() => {
const audio = audioRef.current
if (!audio) return
+ // Handle the case where duration isn't available yet
+ const handleCanPlay = () => {
+ if (startTime) {
+ // ... startTime parsing logic ...
+ }
+ }
+
+ audio.addEventListener('canplay', handleCanPlay)
audio.addEventListener('loadeddata', setAudioData)
audio.addEventListener('timeupdate', setAudioTime)
return () => {
+ audio.removeEventListener('canplay', handleCanPlay)
audio.removeEventListener('loadeddata', setAudioData)
audio.removeEventListener('timeupdate', setAudioTime)
}
}, [startTime])src/components/LiveblocksComments.tsx (5)
1-2: Imports are correctly added
The necessary imports for RoomProvider and useThreads are appropriately included.
11-14: Conditional rendering is appropriate
The component correctly returns null when showFullComments is false, ensuring that comments are only displayed when intended.
16-20: Proper usage of RoomProvider
Wrapping LiveblocksCommentsContent with RoomProvider using snippetId ensures that the comments are scoped to the correct Liveblocks room.
23-23: Component LiveblocksCommentsContent is well-defined
The LiveblocksCommentsContent component is appropriately defined to manage the display of comments for a given snippetId.
39-47: Rendering of comments and composer is correct
Comments are being rendered appropriately by mapping over threads and displaying each Thread component. The Composer component is correctly included for adding new comments.
🧰 Tools
🪛 eslint
[error] 45-45: Unsafe assignment of an any value.
(@typescript-eslint/no-unsafe-assignment)
src/layouts/AuthenticatedLayout.tsx (1)
104-109: 🛠️ Refactor suggestion
Optimize component structure by removing unnecessary wrappers
Consider simplifying the JSX structure by removing unnecessary <div> elements to reduce DOM complexity.
Apply this diff to streamline the component:
-<div className='flex flex-col'>
<HeaderBar />
- <div className='flex-grow overflow-hidden bg-ghost-white'>
<Outlet />
- </div>
-</div>
+<HeaderBar />
+<Outlet />Likely invalid or redundant comment.
| export const getLocalStorageItem = (key: string) => { | ||
| try { | ||
| const item = localStorage.getItem(key) | ||
| return item ? JSON.parse(item) : null | ||
| } catch (error) { | ||
| console.error('Error reading from localStorage:', error) | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and error handling.
The function could benefit from improved type safety and more robust error handling:
Consider applying these improvements:
-export const getLocalStorageItem = (key: string) => {
+export const getLocalStorageItem = <T>(key: string): T | null => {
try {
+ if (typeof window === 'undefined' || !window.localStorage) {
+ console.warn('localStorage is not available');
+ return null;
+ }
const item = localStorage.getItem(key)
- return item ? JSON.parse(item) : null
+ return item ? (JSON.parse(item) as T) : null
} catch (error) {
- console.error('Error reading from localStorage:', error)
+ console.error(`Error reading '${key}' from localStorage:`, error)
return null
}
}This improves the implementation by:
- Adding generic type parameter for type-safe parsing
- Checking localStorage availability
- Including key name in error messages for better debugging
📝 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.
| export const getLocalStorageItem = (key: string) => { | |
| try { | |
| const item = localStorage.getItem(key) | |
| return item ? JSON.parse(item) : null | |
| } catch (error) { | |
| console.error('Error reading from localStorage:', error) | |
| return null | |
| } | |
| } | |
| export const getLocalStorageItem = <T>(key: string): T | null => { | |
| try { | |
| if (typeof window === 'undefined' || !window.localStorage) { | |
| console.warn('localStorage is not available'); | |
| return null; | |
| } | |
| const item = localStorage.getItem(key) | |
| return item ? (JSON.parse(item) as T) : null | |
| } catch (error) { | |
| console.error(`Error reading '${key}' from localStorage:`, error) | |
| return null | |
| } | |
| } |
| export const setLocalStorageItem = (key: string, value: any) => { | ||
| try { | ||
| localStorage.setItem(key, JSON.stringify(value)) | ||
| } catch (error) { | ||
| console.error('Error writing to localStorage:', error) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix any type and improve error handling.
The function uses any type (flagged by eslint) and could benefit from better error handling:
Consider applying these improvements:
-export const setLocalStorageItem = (key: string, value: any) => {
+export const setLocalStorageItem = <T>(key: string, value: T): boolean => {
try {
+ if (typeof window === 'undefined' || !window.localStorage) {
+ console.warn('localStorage is not available');
+ return false;
+ }
localStorage.setItem(key, JSON.stringify(value))
+ return true;
} catch (error) {
- console.error('Error writing to localStorage:', error)
+ console.error(`Error writing '${key}' to localStorage:`, error)
+ return false;
}
}This improves the implementation by:
- Replacing
anywith generic type parameter - Adding return value to indicate success/failure
- Checking localStorage availability
- Including key name in error messages
📝 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.
| export const setLocalStorageItem = (key: string, value: any) => { | |
| try { | |
| localStorage.setItem(key, JSON.stringify(value)) | |
| } catch (error) { | |
| console.error('Error writing to localStorage:', error) | |
| } | |
| } | |
| export const setLocalStorageItem = <T>(key: string, value: T): boolean => { | |
| try { | |
| if (typeof window === 'undefined' || !window.localStorage) { | |
| console.warn('localStorage is not available'); | |
| return false; | |
| } | |
| localStorage.setItem(key, JSON.stringify(value)) | |
| return true; | |
| } catch (error) { | |
| console.error(`Error writing '${key}' to localStorage:`, error) | |
| return false; | |
| } | |
| } |
🧰 Tools
🪛 eslint
[error] 11-11: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
| const [isUpvoted, setIsUpvoted] = useState(() => { | ||
| const localUpvoted = getLocalStorageItem(`upvoted_${snippetId}_${label.id}`) | ||
| return localUpvoted !== null ? localUpvoted : label.upvoted_by.some(upvoter => upvoter.email === user?.email) | ||
| }) | ||
|
|
||
| const checkUpvoteStatus = useCallback(async () => { | ||
| const [upvoteCount, setUpvoteCount] = useState(() => { | ||
| const localCount = getLocalStorageItem(`upvoteCount_${snippetId}_${label.id}`) | ||
| return localCount !== null ? localCount : label.upvoted_by.length | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing storage cleanup and race condition handling.
While the local storage implementation is good, consider these improvements:
- Add cleanup logic to prevent memory bloat from accumulated storage items
- Handle potential race conditions between local storage and initial render
Consider implementing a cleanup mechanism:
// Add this utility function to storage.ts
export const cleanupSnippetStorage = (snippetId: string) => {
const storageKeys = Object.keys(localStorage);
const snippetKeys = storageKeys.filter(key => key.startsWith(`upvoted_${snippetId}_`));
snippetKeys.forEach(key => localStorage.removeItem(key));
};Then use it when the component unmounts:
+useEffect(() => {
+ return () => {
+ // Cleanup storage when component unmounts
+ cleanupSnippetStorage(snippetId);
+ };
+}, [snippetId]);
src/components/LabelButton.tsx
Outdated
| // Check initial upvote status when component mounts or when user/upvoted_by changes | ||
| useEffect(() => { | ||
| if (user) { | ||
| setIsUpvoted(label.upvoted_by.some(upvoter => upvoter.email === user.email)) | ||
| const isCurrentlyUpvoted = label.upvoted_by.some(upvoter => upvoter.email === user.email) | ||
| setIsUpvoted(isCurrentlyUpvoted) | ||
| setLocalStorageItem(`upvoted_${snippetId}_${label.id}`, isCurrentlyUpvoted) | ||
| } | ||
| }, [label.upvoted_by]) | ||
| }, [user, label.upvoted_by, snippetId, label.id]) | ||
|
|
||
| // Update local storage when states change | ||
| useEffect(() => { | ||
| checkUpvoteStatus() | ||
| }, [checkUpvoteStatus]) | ||
| setLocalStorageItem(`upvoted_${snippetId}_${label.id}`, isUpvoted) | ||
| setLocalStorageItem(`upvoteCount_${snippetId}_${label.id}`, upvoteCount) | ||
| }, [isUpvoted, upvoteCount, snippetId, label.id]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing effect hooks for better performance.
The current implementation might cause unnecessary rerenders and storage updates.
Consider batching the storage updates:
-useEffect(() => {
- setLocalStorageItem(`upvoted_${snippetId}_${label.id}`, isUpvoted)
- setLocalStorageItem(`upvoteCount_${snippetId}_${label.id}`, upvoteCount)
-}, [isUpvoted, upvoteCount, snippetId, label.id])
+useEffect(() => {
+ const batchUpdate = () => {
+ const updates = {
+ [`upvoted_${snippetId}_${label.id}`]: isUpvoted,
+ [`upvoteCount_${snippetId}_${label.id}`]: upvoteCount
+ };
+ Object.entries(updates).forEach(([key, value]) =>
+ setLocalStorageItem(key, value)
+ );
+ };
+
+ // Debounce storage updates
+ const timeoutId = setTimeout(batchUpdate, 100);
+ return () => clearTimeout(timeoutId);
+}, [isUpvoted, upvoteCount, snippetId, label.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.
| // Check initial upvote status when component mounts or when user/upvoted_by changes | |
| useEffect(() => { | |
| if (user) { | |
| setIsUpvoted(label.upvoted_by.some(upvoter => upvoter.email === user.email)) | |
| const isCurrentlyUpvoted = label.upvoted_by.some(upvoter => upvoter.email === user.email) | |
| setIsUpvoted(isCurrentlyUpvoted) | |
| setLocalStorageItem(`upvoted_${snippetId}_${label.id}`, isCurrentlyUpvoted) | |
| } | |
| }, [label.upvoted_by]) | |
| }, [user, label.upvoted_by, snippetId, label.id]) | |
| // Update local storage when states change | |
| useEffect(() => { | |
| checkUpvoteStatus() | |
| }, [checkUpvoteStatus]) | |
| setLocalStorageItem(`upvoted_${snippetId}_${label.id}`, isUpvoted) | |
| setLocalStorageItem(`upvoteCount_${snippetId}_${label.id}`, upvoteCount) | |
| }, [isUpvoted, upvoteCount, snippetId, label.id]) | |
| // Check initial upvote status when component mounts or when user/upvoted_by changes | |
| useEffect(() => { | |
| if (user) { | |
| const isCurrentlyUpvoted = label.upvoted_by.some(upvoter => upvoter.email === user.email) | |
| setIsUpvoted(isCurrentlyUpvoted) | |
| setLocalStorageItem(`upvoted_${snippetId}_${label.id}`, isCurrentlyUpvoted) | |
| } | |
| }, [user, label.upvoted_by, snippetId, label.id]) | |
| // Update local storage when states change | |
| useEffect(() => { | |
| const batchUpdate = () => { | |
| const updates = { | |
| [`upvoted_${snippetId}_${label.id}`]: isUpvoted, | |
| [`upvoteCount_${snippetId}_${label.id}`]: upvoteCount | |
| }; | |
| Object.entries(updates).forEach(([key, value]) => | |
| setLocalStorageItem(key, value) | |
| ); | |
| }; | |
| // Debounce storage updates | |
| const timeoutId = setTimeout(batchUpdate, 100); | |
| return () => clearTimeout(timeoutId); | |
| }, [isUpvoted, upvoteCount, snippetId, label.id]); |
| const [isStarred, setIsStarred] = useState(() => { | ||
| const localStarred = getLocalStorageItem(`starred_${snippet.id}`) | ||
| return localStarred !== null ? localStarred : snippet.starred_by_user | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding type safety and memoization.
While the implementation is good, consider these improvements:
- Add type safety for the local storage value
- Memoize the initializer function to prevent unnecessary recalculations
+ const getInitialStarredState = React.useCallback(() => {
+ const localStarred = getLocalStorageItem<boolean>(`starred_${snippet.id}`)
return localStarred !== null ? localStarred : snippet.starred_by_user
+ }, [snippet.id, snippet.starred_by_user])
- const [isStarred, setIsStarred] = useState(() => {
- const localStarred = getLocalStorageItem(`starred_${snippet.id}`)
- return localStarred !== null ? localStarred : snippet.starred_by_user
- })
+ const [isStarred, setIsStarred] = useState(getInitialStarredState)📝 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 [isStarred, setIsStarred] = useState(() => { | |
| const localStarred = getLocalStorageItem(`starred_${snippet.id}`) | |
| return localStarred !== null ? localStarred : snippet.starred_by_user | |
| }) | |
| const getInitialStarredState = React.useCallback(() => { | |
| const localStarred = getLocalStorageItem<boolean>(`starred_${snippet.id}`) | |
| return localStarred !== null ? localStarred : snippet.starred_by_user | |
| }, [snippet.id, snippet.starred_by_user]) | |
| const [isStarred, setIsStarred] = useState(getInitialStarredState) |
src/layouts/AuthenticatedLayout.tsx
Outdated
| const users = userIds.map(userId => { | ||
| const user = allUsers.find(u => u.email === userId); | ||
| return { | ||
| name: user?.raw_user_meta_data.name || userId, | ||
| avatar: user?.raw_user_meta_data.avatar_url || "" | ||
| }; | ||
| }); | ||
| return users; |
There was a problem hiding this comment.
Handle undefined users in 'resolveUsers'
In resolveUsers, if a user is not found in allUsers, user will be undefined, leading to potential issues when accessing user properties. Provide default values to handle undefined users gracefully.
Apply this diff to handle undefined users:
const users = userIds.map(userId => {
const user = allUsers.find(u => u.email === userId);
return {
- name: user?.raw_user_meta_data.name || userId,
- avatar: user?.raw_user_meta_data.avatar_url || ""
+ name: user?.user_metadata?.name || user?.email || userId,
+ avatar: user?.user_metadata?.avatar_url || ""
};
});📝 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 users = userIds.map(userId => { | |
| const user = allUsers.find(u => u.email === userId); | |
| return { | |
| name: user?.raw_user_meta_data.name || userId, | |
| avatar: user?.raw_user_meta_data.avatar_url || "" | |
| }; | |
| }); | |
| return users; | |
| const users = userIds.map(userId => { | |
| const user = allUsers.find(u => u.email === userId); | |
| return { | |
| name: user?.user_metadata?.name || user?.email || userId, | |
| avatar: user?.user_metadata?.avatar_url || "" | |
| }; | |
| }); | |
| return users; |
| const { data: allUsers = [] } = useQuery({ | ||
| queryKey: ['users'], | ||
| queryFn: fetchAllUsers, | ||
| enabled: !!session, | ||
| select: (users) => users.map(user => ({ | ||
| ...user, | ||
| raw_user_meta_data: { | ||
| name: user.raw_user_meta_data?.name || user.email, | ||
| avatar_url: user.raw_user_meta_data?.avatar_url || "" | ||
| } | ||
| })) | ||
| }) |
There was a problem hiding this comment.
Add type parameter to 'useQuery' for type safety
Include the type parameter <UserData[]> in the useQuery hook to ensure that allUsers is properly typed.
Apply this diff to specify the type:
-const { data: allUsers = [] } = useQuery({
+const { data: allUsers = [] } = useQuery<UserData[]>({
queryKey: ['users'],
queryFn: fetchAllUsers,
enabled: !!session,
select: (users) => users.map(user => ({
...user,
- raw_user_meta_data: {
- name: user.raw_user_meta_data?.name || user.email,
- avatar_url: user.raw_user_meta_data?.avatar_url || ""
+ user_metadata: {
+ name: user.user_metadata?.name || user.email,
+ avatar_url: user.user_metadata?.avatar_url || ""
}
}))
})Committable suggestion was skipped due to low confidence.
| const fetchPublicSnippet = async (snippetId: string): Promise<PublicSnippet> => { | ||
| const response = await fetch(`/api/public/snippets/${snippetId}`) | ||
| if (!response.ok) throw new Error('Failed to fetch snippet') | ||
| return response.json() | ||
| } |
There was a problem hiding this comment.
Ensure type safety by casting response data to PublicSnippet
The response.json() method returns a Promise<any>, which can lead to unsafe member access due to lost type information. Cast the result to PublicSnippet to maintain type safety.
Apply this diff to cast the response data:
const fetchPublicSnippet = async (snippetId: string): Promise<PublicSnippet> => {
const response = await fetch(`/api/public/snippets/${snippetId}`)
if (!response.ok) throw new Error('Failed to fetch snippet')
- return response.json()
+ const data: PublicSnippet = await response.json()
+ return data
}📝 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 fetchPublicSnippet = async (snippetId: string): Promise<PublicSnippet> => { | |
| const response = await fetch(`/api/public/snippets/${snippetId}`) | |
| if (!response.ok) throw new Error('Failed to fetch snippet') | |
| return response.json() | |
| } | |
| const fetchPublicSnippet = async (snippetId: string): Promise<PublicSnippet> => { | |
| const response = await fetch(`/api/public/snippets/${snippetId}`) | |
| if (!response.ok) throw new Error('Failed to fetch snippet') | |
| const data: PublicSnippet = await response.json() | |
| return data | |
| } |
| const { data: snippet, isLoading } = useQuery({ | ||
| queryKey: ['publicSnippet', snippetId], | ||
| queryFn: () => fetchPublicSnippet(snippetId || ''), | ||
| enabled: !!snippetId | ||
| }) |
There was a problem hiding this comment.
Specify generic type in useQuery to ensure type safety
Currently, the useQuery call does not specify the generic type parameter, resulting in snippet being typed as unknown. This leads to unsafe member access when accessing properties of snippet.
Apply this diff to specify the generic type:
- const { data: snippet, isLoading } = useQuery({
+ const { data: snippet, isLoading } = useQuery<PublicSnippet>({
queryKey: ['publicSnippet', snippetId],
queryFn: () => fetchPublicSnippet(snippetId || ''),
enabled: !!snippetId
})📝 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 { data: snippet, isLoading } = useQuery({ | |
| queryKey: ['publicSnippet', snippetId], | |
| queryFn: () => fetchPublicSnippet(snippetId || ''), | |
| enabled: !!snippetId | |
| }) | |
| const { data: snippet, isLoading } = useQuery<PublicSnippet>({ | |
| queryKey: ['publicSnippet', snippetId], | |
| queryFn: () => fetchPublicSnippet(snippetId || ''), | |
| enabled: !!snippetId | |
| }) |
src/components/PublicSnippet.tsx
Outdated
| audioSrc={`https://audio.verdad.app/${snippet.file_path}`} | ||
| startTime={snippet.start_time} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding URLs; use configuration or environment variables instead
Currently, audioSrc is using a hardcoded base URL. To enhance maintainability and flexibility, consider extracting the base URL into a configuration file or environment variable.
Apply this diff to use a configuration value:
- audioSrc={`https://audio.verdad.app/${snippet.file_path}`}
+ audioSrc={`${process.env.REACT_APP_AUDIO_BASE_URL}/${snippet.file_path}`}Ensure that REACT_APP_AUDIO_BASE_URL is defined in your environment variables or configuration.
📝 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.
| audioSrc={`https://audio.verdad.app/${snippet.file_path}`} | |
| startTime={snippet.start_time} | |
| audioSrc={`${process.env.REACT_APP_AUDIO_BASE_URL}/${snippet.file_path}`} | |
| startTime={snippet.start_time} |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (3)
src/components/PublicHeaderBar.tsx (1)
9-14: Enhance accessibility for the header and logo.The header and logo sections should include proper ARIA labels and semantic HTML for better accessibility.
Apply these improvements:
- <header className='flex items-center justify-between border-b border-blue-600 bg-blue-50 px-8 py-2'> + <header className='flex items-center justify-between border-b border-blue-600 bg-blue-50 px-8 py-2' role="banner" aria-label="Public header"> - <Link to='/' className='no-underline'> + <Link to='/' className='no-underline' aria-label="Go to homepage"> - <div className='font-inter cursor-pointer bg-gradient-to-r from-blue-500 to-blue-600 bg-clip-text py-2 text-2xl font-bold leading-7 tracking-wide text-transparent'> + <h1 className='font-inter cursor-pointer bg-gradient-to-r from-blue-500 to-blue-600 bg-clip-text py-2 text-2xl font-bold leading-7 tracking-wide text-transparent m-0'> VERDAD - </div> + </h1>src/components/ui/tooltip.tsx (1)
12-27: Consider enhancing accessibility and documentation.While the implementation is solid, consider these improvements:
- Add
aria-labelprop for better screen reader support- Document the animation classes for better maintainability
Here's how you could enhance the component:
const TooltipContent = React.forwardRef< React.ElementRef<typeof TooltipPrimitive.Content>, React.ComponentPropsWithoutRef<typeof TooltipPrimitive.Content> ->(({ className, sideOffset = 4, ...props }, ref) => ( +>(({ className, sideOffset = 4, "aria-label": ariaLabel, ...props }, ref) => ( <TooltipPrimitive.Portal> <TooltipPrimitive.Content ref={ref} sideOffset={sideOffset} + aria-label={ariaLabel} className={cn( + // Positioning and visibility "z-50 overflow-hidden rounded-md bg-primary px-3 py-1.5 text-xs text-primary-foreground", + // Entry animations "animate-in fade-in-0 zoom-in-95", + // Exit animations "data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=closed]:zoom-out-95", + // Slide animations based on position "data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2", className )} {...props} /> </TooltipPrimitive.Portal> ))src/components/PublicSnippet.tsx (1)
42-45: Verify rate limiting on public endpoint.Since this is a public endpoint, ensure that appropriate rate limiting is in place to prevent abuse.
Consider implementing:
- API rate limiting per IP address
- Caching for frequently accessed snippets
- Request size limits for the RPC call
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.json(1 hunks)src/components/PublicHeaderBar.tsx(1 hunks)src/components/PublicSnippet.tsx(1 hunks)src/components/ShareButton.tsx(1 hunks)src/components/SnippetCard.tsx(3 hunks)src/components/SnippetDetail.tsx(3 hunks)src/components/ui/tooltip.tsx(1 hunks)src/layouts/AuthenticatedLayout.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/SnippetDetail.tsx
🧰 Additional context used
🪛 eslint
src/components/PublicHeaderBar.tsx
[error] 17-17: Unsafe call of an any typed value.
(@typescript-eslint/no-unsafe-call)
src/components/PublicSnippet.tsx
[error] 135-135: Unsafe member access .context on an any value.
(@typescript-eslint/no-unsafe-member-access)
src/components/ShareButton.tsx
[error] 60-60: Unsafe assignment of an any value.
(@typescript-eslint/no-unsafe-assignment)
src/components/ui/tooltip.tsx
[error] 28-28: Unsafe member access .Content on an any value.
(@typescript-eslint/no-unsafe-member-access)
src/layouts/AuthenticatedLayout.tsx
[error] 115-115: Unsafe member access .email on an any value.
(@typescript-eslint/no-unsafe-member-access)
🔇 Additional comments (15)
src/components/PublicHeaderBar.tsx (1)
1-26: Well-structured implementation of the public header bar.
The component is well-organized and aligns perfectly with the PR objective (VER-36) to implement a public view. The implementation is clean, focused, and maintains a good separation of concerns.
🧰 Tools
🪛 eslint
[error] 17-17: Unsafe call of an any typed value.
(@typescript-eslint/no-unsafe-call)
src/components/ui/tooltip.tsx (2)
1-10: LGTM! Clean implementation of imports and basic exports.
The code follows best practices by using aliased imports and leveraging Radix UI's accessible primitives.
30-30: LGTM! Clean export implementation.
The named exports provide a clear and maintainable API for the tooltip components.
src/components/ShareButton.tsx (4)
1-20: LGTM! Clean imports and well-defined props interface.
The imports are organized logically, and the props interface is properly typed with clear prop names and types.
42-76: LGTM! Well-structured component with good UX considerations.
The component implementation:
- Provides clear visual feedback
- Handles accessibility through proper ARIA attributes (via UI components)
- Implements responsive design with the showLabel prop
- Prevents event bubbling appropriately
🧰 Tools
🪛 eslint
[error] 60-60: Unsafe assignment of an any value.
(@typescript-eslint/no-unsafe-assignment)
59-70:
Fix type safety issue with Tooltip open prop.
There's a type safety issue with the open prop on the Tooltip component.
Apply this fix to properly type the open prop:
- <Tooltip open={open}>
+ <Tooltip open={open as boolean}>Consider creating a proper type definition for the Tooltip component if one doesn't exist.
Let's check for existing Tooltip type definitions:
#!/bin/bash
# Search for Tooltip component type definitions
rg -t typescript "interface TooltipProps" 🧰 Tools
🪛 eslint
[error] 60-60: Unsafe assignment of an any value.
(@typescript-eslint/no-unsafe-assignment)
22-25: Consider environment-aware URL construction.
The current URL construction using window.location.origin might not work correctly in all environments (e.g., SSR, testing). Consider:
- Using environment variables for the base URL
- Handling cases where window is undefined
- Ensuring URL construction is XSS-safe
Let's check if there's any environment configuration in place:
Consider implementing a centralized URL construction utility that handles these edge cases consistently across the application.
package.json (2)
40-40: LGTM: Tooltip addition aligns with existing Radix UI usage.
Adding @radix-ui/react-tooltip is consistent with the project's existing use of Radix UI components and will help maintain a cohesive design system.
39-39:
Consider consolidating toast libraries.
The codebase already includes react-toastify for notifications. Adding @radix-ui/react-toast creates redundancy and could lead to inconsistent user experience. Consider standardizing on one toast library, preferably Radix UI to maintain consistency with other UI components.
Let's verify the usage of both toast libraries:
src/layouts/AuthenticatedLayout.tsx (2)
9-22: Previous type safety recommendations are still valid.
The previously suggested improvements for type safety haven't been addressed:
- The property name should be
user_metadatainstead ofraw_user_meta_datato match Supabase'sUsertype - The
fetchAllUsersfunction should have an explicit return type annotation
49-56: Implementation aligns well with PR objectives.
The added routing logic for public snippets (redirecting from /snippet/:id to /p/:id for unauthenticated users) successfully implements the public view requirement from VER-36. The enhanced Liveblocks configuration with proper user resolution supports the improvements needed for the notification system (VER-35).
src/components/SnippetCard.tsx (2)
87-87: LGTM! ShareButton implementation looks good.
The ShareButton component is properly integrated with the required snippetId prop.
94-94: LGTM! Star button click handler is properly implemented.
The click handler is correctly bound to the onClick event.
src/components/PublicSnippet.tsx (2)
1-40: LGTM! Well-structured imports and interface definition.
The PublicSnippet interface is comprehensive and properly typed, including all necessary fields for the component.
120-123: 🛠️ Refactor suggestion
Add error handling for audio playback.
The AudioPlayer component should handle cases where the audio file might not be available.
Let's verify the audio file accessibility:
Consider adding error handling:
<AudioPlayer
audioSrc={`https://audio.verdad.app/${snippet.file_path}`}
startTime={snippet.start_time}
+ onError={(e) => {
+ console.error('Audio playback failed:', e)
+ // Show user-friendly error message
+ }}
/>| import React from 'react' | ||
| import { Link, useNavigate } from 'react-router-dom' | ||
| import { Button } from '@/components/ui/button' |
There was a problem hiding this comment.
Add proper TypeScript types for the navigation function.
The useNavigate hook requires proper typing to ensure type safety throughout the component.
Apply this diff to fix the TypeScript error:
import React from 'react'
-import { Link, useNavigate } from 'react-router-dom'
+import { Link, useNavigate, NavigateFunction } from 'react-router-dom'
import { Button } from '@/components/ui/button'
const PublicHeaderBar: React.FC = () => {
- const navigate = useNavigate()
+ const navigate: NavigateFunction = useNavigate()Also applies to: 5-7
src/components/PublicHeaderBar.tsx
Outdated
| <Button | ||
| variant='ghost' | ||
| onClick={() => navigate('/login')} | ||
| className="hover:bg-blue-100 text-blue-600 hover:text-blue-700" | ||
| > | ||
| Login | ||
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the login button with loading state and accessibility.
The login button could benefit from a loading state during navigation and proper ARIA attributes.
Consider implementing these improvements:
<Button
variant='ghost'
- onClick={() => navigate('/login')}
+ onClick={async () => {
+ setIsLoading(true);
+ try {
+ await navigate('/login');
+ } finally {
+ setIsLoading(false);
+ }
+ }}
+ disabled={isLoading}
+ aria-label="Login to your account"
className="hover:bg-blue-100 text-blue-600 hover:text-blue-700"
>
- Login
+ {isLoading ? 'Redirecting...' : 'Login'}Don't forget to add the loading state:
const [isLoading, setIsLoading] = useState(false);🧰 Tools
🪛 eslint
[error] 17-17: Unsafe call of an any typed value.
(@typescript-eslint/no-unsafe-call)
| /> | ||
| </TooltipPrimitive.Portal> | ||
| )) | ||
| TooltipContent.displayName = TooltipPrimitive.Content.displayName |
There was a problem hiding this comment.
Fix TypeScript type safety for displayName assignment.
The current implementation triggers a TypeScript error due to unsafe member access.
Here's how to fix the type safety issue:
-TooltipContent.displayName = TooltipPrimitive.Content.displayName
+TooltipContent.displayName = "TooltipContent"📝 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.
| TooltipContent.displayName = TooltipPrimitive.Content.displayName | |
| TooltipContent.displayName = "TooltipContent" |
🧰 Tools
🪛 eslint
[error] 28-28: Unsafe member access .Content on an any value.
(@typescript-eslint/no-unsafe-member-access)
| const handleCopy = async (e: React.MouseEvent) => { | ||
| e.stopPropagation() | ||
| try { | ||
| await navigator.clipboard.writeText(publicUrl) | ||
| setCopied(true) | ||
| setOpen(true) | ||
| setTimeout(() => { | ||
| setCopied(false) | ||
| setOpen(false) | ||
| }, 1000) | ||
| } catch (err) { | ||
| console.error('Failed to copy:', err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and timeout management.
While the copy functionality works, consider these improvements:
- Add user feedback for copy failures
- Extract the timeout duration to a constant
- Consider using a cleanup function to prevent state updates if the component unmounts
Here's a suggested implementation:
+const COPY_FEEDBACK_DURATION = 1000;
+
const handleCopy = async (e: React.MouseEvent) => {
e.stopPropagation()
+ let timeoutId: NodeJS.Timeout;
try {
await navigator.clipboard.writeText(publicUrl)
setCopied(true)
setOpen(true)
- setTimeout(() => {
+ timeoutId = setTimeout(() => {
setCopied(false)
setOpen(false)
- }, 1000)
+ }, COPY_FEEDBACK_DURATION)
} catch (err) {
console.error('Failed to copy:', err)
+ // Show user feedback
+ setOpen(true)
}
+ return () => timeoutId && clearTimeout(timeoutId);
}Committable suggestion skipped: line range outside the PR's diff.
| if (!session) { | ||
| const snippetMatch = location.pathname.match(/^\/snippet\/(.+)$/) | ||
| if (snippetMatch) { | ||
| navigate(`/p/${snippetMatch[1]}`) | ||
| } else { | ||
| navigate('/login') | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| const { | ||
| data: { subscription } | ||
| } = supabase.auth.onAuthStateChange((_event, session) => { | ||
| setSession(session) | ||
| setUser(session?.user || null) | ||
| if (!session) navigate('/login') | ||
| if (!session) { | ||
| const snippetMatch = location.pathname.match(/^\/snippet\/(.+)$/) | ||
| if (snippetMatch) { | ||
| navigate(`/p/${snippetMatch[1]}`) | ||
| } else { | ||
| navigate('/login') | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated routing logic.
The routing logic for handling unauthenticated users is duplicated between the initial session check and the auth state change handler. Consider extracting this into a separate function:
+ const handleUnauthenticatedRedirect = () => {
+ const snippetMatch = location.pathname.match(/^\/snippet\/(.+)$/)
+ if (snippetMatch) {
+ navigate(`/p/${snippetMatch[1]}`)
+ } else {
+ navigate('/login')
+ }
+ }
useEffect(() => {
void supabase.auth.getSession().then(({ data: { session } }) => {
setSession(session)
setUser(session?.user || null)
if (!session) {
- const snippetMatch = location.pathname.match(/^\/snippet\/(.+)$/)
- if (snippetMatch) {
- navigate(`/p/${snippetMatch[1]}`)
- } else {
- navigate('/login')
- }
+ handleUnauthenticatedRedirect()
}
})
const {
data: { subscription }
} = supabase.auth.onAuthStateChange((_event, session) => {
setSession(session)
setUser(session?.user || null)
if (!session) {
- const snippetMatch = location.pathname.match(/^\/snippet\/(.+)$/)
- if (snippetMatch) {
- navigate(`/p/${snippetMatch[1]}`)
- } else {
- navigate('/login')
- }
+ handleUnauthenticatedRedirect()
}
})📝 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.
| if (!session) { | |
| const snippetMatch = location.pathname.match(/^\/snippet\/(.+)$/) | |
| if (snippetMatch) { | |
| navigate(`/p/${snippetMatch[1]}`) | |
| } else { | |
| navigate('/login') | |
| } | |
| } | |
| }) | |
| const { | |
| data: { subscription } | |
| } = supabase.auth.onAuthStateChange((_event, session) => { | |
| setSession(session) | |
| setUser(session?.user || null) | |
| if (!session) navigate('/login') | |
| if (!session) { | |
| const snippetMatch = location.pathname.match(/^\/snippet\/(.+)$/) | |
| if (snippetMatch) { | |
| navigate(`/p/${snippetMatch[1]}`) | |
| } else { | |
| navigate('/login') | |
| } | |
| } | |
| if (!session) { | |
| handleUnauthenticatedRedirect() | |
| } | |
| }) | |
| const { | |
| data: { subscription } | |
| } = supabase.auth.onAuthStateChange((_event, session) => { | |
| setSession(session) | |
| setUser(session?.user || null) | |
| if (!session) { | |
| handleUnauthenticatedRedirect() | |
| } | |
| }) | |
| const handleUnauthenticatedRedirect = () => { | |
| const snippetMatch = location.pathname.match(/^\/snippet\/(.+)$/) | |
| if (snippetMatch) { | |
| navigate(`/p/${snippetMatch[1]}`) | |
| } else { | |
| navigate('/login') | |
| } | |
| } |
src/layouts/AuthenticatedLayout.tsx
Outdated
| resolveMentionSuggestions={async ({ text }) => { | ||
| if (!text) { | ||
| return allUsers.map(user => user.email); | ||
| } | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error('Failed to fetch user suggestions') | ||
| } | ||
| const filteredData = allUsers.filter(user => { | ||
| const name = user.raw_user_meta_data?.name?.toLowerCase() || ""; | ||
| const email = user.email.toLowerCase(); | ||
| const searchText = text.toLowerCase(); | ||
| return name.includes(searchText) || email.includes(searchText); | ||
| }); | ||
|
|
||
| // Return the list of user IDs | ||
| return await response.json() | ||
| } catch (error) { | ||
| console.error('Error in resolveMentionSuggestions:', error) | ||
| return [] | ||
| } | ||
| return filteredData.map(user => user.email); |
There was a problem hiding this comment.
Add type safety to mention suggestions implementation.
The mention suggestions implementation has type safety issues with accessing the email property. Let's improve type safety while maintaining the functionality:
resolveMentionSuggestions={async ({ text }) => {
if (!text) {
- return allUsers.map(user => user.email);
+ return allUsers.map((user: UserData) => user.email);
}
- const filteredData = allUsers.filter(user => {
+ const filteredData = allUsers.filter((user: UserData) => {
const name = user.raw_user_meta_data?.name?.toLowerCase() || "";
const email = user.email.toLowerCase();
const searchText = text.toLowerCase();
return name.includes(searchText) || email.includes(searchText);
});
- return filteredData.map(user => user.email);
+ return filteredData.map((user: UserData) => user.email);
}}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 115-115: Unsafe member access .email on an any value.
(@typescript-eslint/no-unsafe-member-access)
| const [isStarred, setIsStarred] = useState(() => { | ||
| const localStarred = getLocalStorageItem(`starred_${snippet.id}`) | ||
| return localStarred !== null ? localStarred : snippet.starred_by_user | ||
| }) | ||
|
|
||
| useEffect(() => { | ||
| setLocalStorageItem(`starred_${snippet.id}`, isStarred) | ||
| }, [isStarred, snippet.id]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a custom hook for starred state management.
The starred state management logic could be extracted into a reusable custom hook to improve maintainability and reusability.
+const useStarredState = (snippetId: string, initialStarred: boolean) => {
+ const [isStarred, setIsStarred] = useState(() => {
+ const localStarred = getLocalStorageItem<boolean>(`starred_${snippetId}`)
+ return localStarred !== null ? localStarred : initialStarred
+ })
+
+ useEffect(() => {
+ setLocalStorageItem(`starred_${snippetId}`, isStarred)
+ }, [isStarred, snippetId])
+
+ return [isStarred, setIsStarred] as const
+}
const SnippetCard: React.FC<SnippetCardProps> = ({ snippet, onSnippetClick }) => {
- const [isStarred, setIsStarred] = useState(() => {
- const localStarred = getLocalStorageItem(`starred_${snippet.id}`)
- return localStarred !== null ? localStarred : snippet.starred_by_user
- })
-
- useEffect(() => {
- setLocalStorageItem(`starred_${snippet.id}`, isStarred)
- }, [isStarred, snippet.id])
+ const [isStarred, setIsStarred] = useStarredState(snippet.id, snippet.starred_by_user)📝 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 [isStarred, setIsStarred] = useState(() => { | |
| const localStarred = getLocalStorageItem(`starred_${snippet.id}`) | |
| return localStarred !== null ? localStarred : snippet.starred_by_user | |
| }) | |
| useEffect(() => { | |
| setLocalStorageItem(`starred_${snippet.id}`, isStarred) | |
| }, [isStarred, snippet.id]) | |
| const useStarredState = (snippetId: string, initialStarred: boolean) => { | |
| const [isStarred, setIsStarred] = useState(() => { | |
| const localStarred = getLocalStorageItem<boolean>(`starred_${snippetId}`) | |
| return localStarred !== null ? localStarred : initialStarred | |
| }) | |
| useEffect(() => { | |
| setLocalStorageItem(`starred_${snippetId}`, isStarred) | |
| }, [isStarred, snippetId]) | |
| return [isStarred, setIsStarred] as const | |
| } | |
| const SnippetCard: React.FC<SnippetCardProps> = ({ snippet, onSnippetClick }) => { | |
| const [isStarred, setIsStarred] = useStarredState(snippet.id, snippet.starred_by_user) |
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button variant='ghost' className='flex items-center space-x-2'> | ||
| <Download className='h-4 w-4' /> | ||
| <span>Download</span> | ||
| <ChevronDown className='h-4 w-4' /> | ||
| </Button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent> | ||
| <DropdownMenuItem>Original transcript (Spanish)</DropdownMenuItem> | ||
| <DropdownMenuItem>Translated transcript (English)</DropdownMenuItem> | ||
| <DropdownMenuItem>Audio</DropdownMenuItem> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> |
There was a problem hiding this comment.
Implement download functionality for menu items.
The download dropdown menu items are currently non-functional. They should trigger appropriate download actions for transcripts and audio.
Consider implementing handlers for each download option:
const handleDownload = (type: 'spanish' | 'english' | 'audio') => {
switch (type) {
case 'spanish':
// Download Spanish transcript
break;
case 'english':
// Download English transcript
break;
case 'audio':
// Download audio file
break;
}
}Then update the menu items:
-<DropdownMenuItem>Original transcript (Spanish)</DropdownMenuItem>
+<DropdownMenuItem onSelect={() => handleDownload('spanish')}>
+ Original transcript (Spanish)
+</DropdownMenuItem>| const fetchPublicSnippet = async (snippetId: string): Promise<PublicSnippet> => { | ||
| const { data, error } = await supabase.rpc('get_public_snippet', { | ||
| snippet_id: snippetId | ||
| }) | ||
|
|
||
| if (error) throw error | ||
| return data | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling with specific error types.
While the basic error handling is present, it could be more informative for debugging and user feedback.
Consider this enhancement:
const fetchPublicSnippet = async (snippetId: string): Promise<PublicSnippet> => {
const { data, error } = await supabase.rpc('get_public_snippet', {
snippet_id: snippetId
})
- if (error) throw error
+ if (error) {
+ console.error('Failed to fetch public snippet:', error)
+ throw new Error(`Failed to fetch snippet: ${error.message}`)
+ }
+ if (!data) {
+ throw new Error('Snippet not found')
+ }
return data
}📝 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 fetchPublicSnippet = async (snippetId: string): Promise<PublicSnippet> => { | |
| const { data, error } = await supabase.rpc('get_public_snippet', { | |
| snippet_id: snippetId | |
| }) | |
| if (error) throw error | |
| return data | |
| } | |
| const fetchPublicSnippet = async (snippetId: string): Promise<PublicSnippet> => { | |
| const { data, error } = await supabase.rpc('get_public_snippet', { | |
| snippet_id: snippetId | |
| }) | |
| if (error) { | |
| console.error('Failed to fetch public snippet:', error) | |
| throw new Error(`Failed to fetch snippet: ${error.message}`) | |
| } | |
| if (!data) { | |
| throw new Error('Snippet not found') | |
| } | |
| return data | |
| } |
src/App.tsx
Outdated
| <Route path={LOGIN_PATH} element={<LoginPage />} /> | ||
| <Route path={FORGET_PASSWORD_PATH} element={<ForgetPassword />} /> | ||
| <Route path={RESET_PASSWORD_PATH} element={<ResetPassword />} /> | ||
| <Route path='/p/:snippetId' element={<PublicSnippet />} /> |
There was a problem hiding this comment.
Please update the route to routes.ts file a
| onLabelDeleted(label.id) | ||
| } | ||
| } catch (error) { | ||
| console.error('Error toggling upvote:', error) |
There was a problem hiding this comment.
Should we implement toast.error when the upvote function is error
src/components/ShareButton.tsx
Outdated
| const handleCopy = async (e: React.MouseEvent) => { | ||
| e.stopPropagation() | ||
| try { | ||
| await navigator.clipboard.writeText(publicUrl) |
There was a problem hiding this comment.
We should handle the clipboard copy using timeout for safari browser
src/components/SnippetDetail.tsx
Outdated
| </div> | ||
| <AudioPlayer audioSrc={`https://audio.verdad.app/${snippet.file_path}`} /> | ||
| <AudioPlayer | ||
| audioSrc={`https://audio.verdad.app/${snippet.file_path}`} |
There was a problem hiding this comment.
Please extract using environment variables or constant urls
Dat-H-Tran
deleted the
feature/update-liveblocks-comment-and-improvement
branch
2 participants
Implement https://linear.app/pdw/issue/VER-36/public-view-of-snippet-detail-page
Fix https://linear.app/pdw/issue/VER-35/liveblocks-notification-system
Fix https://linear.app/pdw/issue/VER-112/comments-should-be-in-a-separate-liveblocks-room-for-each-snippet
Summary by CodeRabbit
Release Notes
New Features
/p/:snippetId.PublicSnippetcomponent to display public audio snippets with metadata and playback options.AudioPlayerto support optional start time for playback control.LabelButtonandSnippetCardcomponents for better state management and user interactions.LiveblocksCommentsfor clearer comment rendering.ShareButtoncomponent for easy snippet sharing.PublicHeaderBarfor improved navigation with a login option.Bug Fixes
LabelButtonandSnippetCardcomponents.Documentation
Chores