fix: persisted state for toggle in notes #624
Conversation
📝 WalkthroughWalkthroughThe Changes
Possibly related PRs
Suggested labels
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: 0
🧹 Outside diff range and nitpick comments (3)
apps/frontend/src/components/Notes/NotesPage.tsx (3)
Line range hint
50-57: Consider enhancing error handling with user feedback.While the error handling is functional, consider adding user feedback for a better UX:
const fetchTheNotes = useCallback(async (): Promise<void> => { try { await fetchNotes(session) setIsFetched(true) } catch (error) { setIsFetched(false) + console.error('Failed to fetch notes:', error) + // Consider adding a toast notification or error state + // to inform users when notes fail to load } }, [session, fetchNotes, setIsFetched])
Line range hint
171-191: Critical: Improve error handling in note deletion.The current implementation has potential issues:
- Race condition: UI updates might proceed even if deletion fails
- No user feedback on error
- Possible inconsistent state if error occurs during navigation
const handleDeleteNote = async (n: Note): Promise<void> => { if (!session || !n) return try { await deleteNote(session, n) const remainingNotes = notes.filter((n_) => n_._id !== n._id) if (n._id === note?._id) { if (remainingNotes.length <= 0) { const newNote = await addNewNote() // No need to redirect here since addNewNote already does the routing } else { router.push(`/space/notes/${remainingNotes[0]._id}`) } } } catch (error) { - console.error("Error deleting note:", error) + console.error("Failed to delete note:", error) + // Prevent UI updates on error + return + // Consider adding error feedback: + // showErrorToast('Failed to delete note. Please try again.') } }
Line range hint
193-205: Consider improving save handling and cleanup.The current implementation has several areas for improvement:
- Race conditions between auto and manual saves
- No error handling for failed saves
- Potential memory leaks
useEffect(() => { if (note && isSaved) { - saveNoteToServer({ ...note, title, description: content }) + const save = async () => { + try { + await saveNoteToServer({ ...note, title, description: content }) + } catch (error) { + console.error('Failed to save note:', error) + setIsSaved(false) + // Consider adding error feedback + } + } + save() } }, [note, saveNoteToServer, isSaved]) useEffect(() => { const handleBeforeUnload = (e: BeforeUnloadEvent) => { if (!isSaved) { e.preventDefault() + // Consider showing a custom message + e.returnValue = 'You have unsaved changes. Are you sure you want to leave?' } } window.addEventListener("beforeunload", handleBeforeUnload) return () => { window.removeEventListener("beforeunload", handleBeforeUnload) + // Clear any pending timeouts + if (timeoutId.current) { + clearTimeout(timeoutId.current) + } } -}, [isSaved]) +}, [isSaved, timeoutId])Also applies to: 124-139
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/frontend/src/components/Notes/NotesPage.tsx(2 hunks)
🔇 Additional comments (2)
apps/frontend/src/components/Notes/NotesPage.tsx (2)
13-13: LGTM: Good use of persisted state for toggle.
The introduction of usePersistedState hook and its implementation for closeToggle effectively addresses the PR objective of persisting the toggle state across sessions.
Also applies to: 46-46
46-46: Verify the persistence of toggle state.
The implementation successfully addresses the PR objective by using usePersistedState. To ensure it works as expected:
Also applies to: 46-46
✅ Verification successful
Implementation of toggle state persistence is correctly implemented and consistent
The verification shows that:
- The
usePersistedStatehook is properly implemented and imported from the correct location - It's being used consistently across components (TodayPage and NotesPage)
- The hook implementation follows React's useState pattern with proper persistence logic
- The toggle state implementation in NotesPage follows the same pattern as other persisted states in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances of toggle state persistence
# and verify the implementation pattern
# Look for other usePersistedState usage
rg "usePersistedState" --type typescript
# Check for similar toggle state implementations
ast-grep --pattern 'const [$_1, $_2] = use$_3State("$_4Toggle", $_5)'
Length of output: 147
Script:
#!/bin/bash
# Description: Check for other instances of toggle state persistence
# and verify the implementation pattern with correct file type
# Look for usePersistedState usage
rg "usePersistedState" -t ts -t tsx
# Check for the hook implementation
fd -e ts -e tsx --exec cat {} \; | grep -A 5 "usePersistedState"
# Look for similar toggle state patterns
rg "Toggle.*useState|useState.*Toggle" -t ts -t tsx
Length of output: 1456
What did you ship?
Fixes:
Checklist:
OR: