refactor(zustand): manage notes separately from occurrences #330
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Notes are now managed separately from occurrences, removing note embedding from occurrence objects and introducing a notesByOccurrenceId store. Occurrence-related components and services have been updated to fetch and display notes via this new mapping, improving separation of concerns and consistency. User objects now include a fetchedAt timestamp for better tracking.
Contributor
📝 WalkthroughWalkthroughThis PR decouples notes from occurrences: occurrences no longer embed a Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Occurrence UI (Chip/Form/ListItem)
participant Hook as useNotesByOccurrenceId
participant NotesStore as notes.store
participant NotesService as notes.service
participant DB as Database
UI->>Hook: request notes map for occurrenceIds
Hook->>NotesStore: return notesByOccurrenceId[occurrenceId]
NotesStore-->>Hook: notes map (in-memory)
Hook-->>UI: occurrenceNote (if present)
alt User creates/edits note via Form
UI->>NotesStore: addNote/updateNote/deleteNote action
NotesStore->>NotesService: call add/update/delete API
NotesService->>DB: persist change
DB-->>NotesService: result (note)
NotesService-->>NotesStore: update notes & notesByOccurrenceId
NotesStore-->>UI: updated occurrenceNote via selector
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
#173 Bundle Size — 1.82MiB (+0.05%).bad8491(current) vs 8e3fefa main#171(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #173 |
Baseline #171 |
|
|---|---|---|
 Initial JS |
1.08MiB(+0.08%) |
1.08MiB |
 Initial CSS |
0B |
0B |
 Cache Invalidation |
87.09% |
100% |
| Chunks |
8 |
8 |
| Assets |
9 |
9 |
| Modules |
6673 |
6673 |
| Duplicate Modules |
0 |
0 |
| Duplicate Code |
0% |
0% |
| Packages |
223 |
223 |
| Duplicate Packages |
2 |
2 |
Bundle size by type 1 change 1 regression
| Current #173 |
Baseline #171 |
|
|---|---|---|
| JS |
1.59MiB (+0.05%) |
1.59MiB |
| CSS |
241.08KiB |
241.08KiB |
Bundle analysis report Branch refactor/notes-by-occurrence-id Project dashboard
Generated by RelativeCI Documentation Report issue
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/notes.store.ts (1)
46-49:clearNotesshould also resetnotesByOccurrenceId.
Otherwise stale occurrence notes remain visible after a clear/reset.🛠️ Proposed fix
clearNotes: () => { set((state) => { state.notes = {}; + state.notesByOccurrenceId = {}; }); },
🤖 Fix all issues with AI agents
In `@src/stores/notes.store.ts`:
- Around line 55-63: The code deletes state.notes[id] and then reads
state.notes[id], causing a runtime error; capture the note before mutation. In
the setter used by deleteNote, first assign const note = state.notes[id]; then,
if note exists and has an occurrenceId (check 'occurrenceId' in note &&
note.occurrenceId), delete state.notesByOccurrenceId[note.occurrenceId]; finally
delete state.notes[id]. Refer to state.notes, state.notesByOccurrenceId, and the
delete logic inside the set((state) => { ... }) block to locate and update the
code.
In `@src/stores/occurrences.store.ts`:
- Around line 149-155: The code updates state.occurrences correctly but still
uses the old occurredAt parameter when writing to occurrencesByDate, which
misplaces items if the API returned a new date and can throw if the date bucket
doesn't exist; update the write to use the returned updatedOccurrence.occurredAt
(via toClientOccurrence(updatedOccurrence).occurredAt or directly from
updatedOccurrence), ensure you check/create the occurrencesByDate bucket key
computed with toCalendarDate(updatedOccurredAt).toString() before assigning, and
also consider removing the entry from the old date bucket if the date changed;
update references around the map update, toClientOccurrence, occurrencesByDate
and toCalendarDate calls accordingly.
- Around line 64-74: The code in the state setter computes dateKey from
occurrence.occurredAt.split('T')[0], causing UTC-vs-local bucketing
inconsistency; change it to derive the key from the client-side object the rest
of the store uses by computing dateKey =
clientOccurrence.toCalendarDate().toString() (so addOccurrence, which updates
state.occurrences and state.occurrencesByDate using clientOccurrence and
nextOccurrence.id, will bucket identically to
fetchOccurrences/removeOccurrence/updateOccurrence).
🧹 Nitpick comments (2)
src/stores/user.store.ts (1)
75-86: Potential type inconsistency between service and store.The
updateUserservice (from@services) returnscamelcaseKeys(data.user, { deep: true })without addingfetchedAt, while the store'sUsertype expectsfetchedAt: string. The code here manually addsfetchedAtto createnewUser, which is the correct approach.However, for consistency with
getSession(which addsfetchedAtin the service layer), consider updating theupdateUserservice to also includefetchedAtin its return value. This would centralize the responsibility for addingfetchedAtin the service layer.♻️ Optional: Add fetchedAt in service for consistency
In
src/services/user.service.ts:export const updateUser = async (attributes: UserAttributes) => { const { data, error } = await supabaseClient.auth.updateUser(attributes); if (error) { throw new Error(error.message); } - return camelcaseKeys(data.user, { deep: true }); + return camelcaseKeys( + { ...data.user, fetchedAt: new Date().toISOString() }, + { deep: true } + ); };Then simplify the store:
- const updatedSupabaseUser = await updateUser(userAttributes); - - const newUser = { - ...updatedSupabaseUser, - fetchedAt: new Date().toISOString(), - }; + const newUser = await updateUser(userAttributes); set((state) => { state.user = newUser; }); return newUser;src/components/occurrence/OccurrenceListItem.tsx (1)
34-36: Minor: Memoization dependency could be more precise.The
useMemodependency array includes the entireoccurrenceobject, but onlyoccurrence.idis used in the computation. This may cause unnecessary recalculations if other properties ofoccurrencechange.♻️ Suggested improvement
const occurrenceNote = React.useMemo(() => { return notes[occurrence.id]; - }, [notes, occurrence]); + }, [notes, occurrence.id]);
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/notes.store.ts (1)
46-49: Clear notes must also clear notesByOccurrenceId.
clearNotesleaves stale occurrence notes, so UI can still show notes after a clear.🛠️ Proposed fix
clearNotes: () => { set((state) => { state.notes = {}; + state.notesByOccurrenceId = {}; }); },
🤖 Fix all issues with AI agents
In `@src/stores/notes.store.ts`:
- Around line 92-97: When updating state in set, you must remove any stale
mapping from notesByOccurrenceId when a note's occurrenceId changes or is
removed: read the previous note (state.notes[id]) before overwriting, if it has
an occurrenceId and it differs from updatedNote.occurrenceId delete
state.notesByOccurrenceId[oldNote.occurrenceId]; then assign
state.notes[id]=updatedNote and, if updatedNote has a truthy occurrenceId, set
state.notesByOccurrenceId[updatedNote.occurrenceId]=updatedNote; use the
existing set, notes, notesByOccurrenceId, updatedNote and id identifiers to
locate and implement this logic.
- Around line 55-61: Guard against a missing note before using the "in" operator
on noteToDelete: in the set callback where noteToDelete is assigned from
state.notes[id], first check that noteToDelete is not null/undefined, then check
for the occurrenceId property and its truthiness before deleting from
state.notesByOccurrenceId (i.e., ensure noteToDelete exists, then if
'occurrenceId' in noteToDelete and noteToDelete.occurrenceId is truthy, delete
state.notesByOccurrenceId[noteToDelete.occurrenceId]). This touches the same
block using noteToDelete and state.notesByOccurrenceId in the set(...) function
in notes.store.ts.
Comment on lines
55
to
+61
| set((state) => { | ||
| const noteToDelete = state.notes[id]; | ||
| delete state.notes[id]; | ||
|
|
||
| if ('occurrenceId' in noteToDelete && noteToDelete?.occurrenceId) { | ||
| delete state.notesByOccurrenceId[noteToDelete.occurrenceId]; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against missing note before using in.
If state.notes[id] is undefined, 'occurrenceId' in noteToDelete throws.
🛠️ Proposed fix
set((state) => {
const noteToDelete = state.notes[id];
delete state.notes[id];
- if ('occurrenceId' in noteToDelete && noteToDelete?.occurrenceId) {
+ if (noteToDelete && isNoteOfOccurrence(noteToDelete)) {
delete state.notesByOccurrenceId[noteToDelete.occurrenceId];
}
});📝 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.
Suggested change
| set((state) => { | |
| const noteToDelete = state.notes[id]; | |
| delete state.notes[id]; | |
| if ('occurrenceId' in noteToDelete && noteToDelete?.occurrenceId) { | |
| delete state.notesByOccurrenceId[noteToDelete.occurrenceId]; | |
| } | |
| set((state) => { | |
| const noteToDelete = state.notes[id]; | |
| delete state.notes[id]; | |
| if (noteToDelete?.occurrenceId) { | |
| delete state.notesByOccurrenceId[noteToDelete.occurrenceId]; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@src/stores/notes.store.ts` around lines 55 - 61, Guard against a missing note
before using the "in" operator on noteToDelete: in the set callback where
noteToDelete is assigned from state.notes[id], first check that noteToDelete is
not null/undefined, then check for the occurrenceId property and its truthiness
before deleting from state.notesByOccurrenceId (i.e., ensure noteToDelete
exists, then if 'occurrenceId' in noteToDelete and noteToDelete.occurrenceId is
truthy, delete state.notesByOccurrenceId[noteToDelete.occurrenceId]). This
touches the same block using noteToDelete and state.notesByOccurrenceId in the
set(...) function in notes.store.ts.
Comment on lines
92
to
+97
| set((state) => { | ||
| state.notes[id] = updatedNote; | ||
|
|
||
| if ('occurrenceId' in updatedNote && updatedNote.occurrenceId) { | ||
| state.notesByOccurrenceId[updatedNote.occurrenceId] = updatedNote; | ||
| } |
Contributor
There was a problem hiding this comment.
Update should remove stale occurrence mapping.
If a note changes occurrenceId (or becomes a period note), the old entry remains in notesByOccurrenceId.
🛠️ Proposed fix
set((state) => {
- state.notes[id] = updatedNote;
-
- if ('occurrenceId' in updatedNote && updatedNote.occurrenceId) {
- state.notesByOccurrenceId[updatedNote.occurrenceId] = updatedNote;
- }
+ const previousNote = state.notes[id];
+ const nextOccurrenceId = isNoteOfOccurrence(updatedNote)
+ ? updatedNote.occurrenceId
+ : null;
+
+ state.notes[id] = updatedNote;
+
+ if (previousNote && isNoteOfOccurrence(previousNote)) {
+ if (previousNote.occurrenceId !== nextOccurrenceId) {
+ delete state.notesByOccurrenceId[previousNote.occurrenceId];
+ }
+ }
+
+ if (nextOccurrenceId) {
+ state.notesByOccurrenceId[nextOccurrenceId] = updatedNote;
+ }
});🤖 Prompt for AI Agents
In `@src/stores/notes.store.ts` around lines 92 - 97, When updating state in set,
you must remove any stale mapping from notesByOccurrenceId when a note's
occurrenceId changes or is removed: read the previous note (state.notes[id])
before overwriting, if it has an occurrenceId and it differs from
updatedNote.occurrenceId delete state.notesByOccurrenceId[oldNote.occurrenceId];
then assign state.notes[id]=updatedNote and, if updatedNote has a truthy
occurrenceId, set
state.notesByOccurrenceId[updatedNote.occurrenceId]=updatedNote; use the
existing set, notes, notesByOccurrenceId, updatedNote and id identifiers to
locate and implement this logic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Notes are now managed separately from occurrences, removing note embedding from occurrence objects and introducing a notesByOccurrenceId store. Occurrence-related components and services have been updated to fetch and display notes via this new mapping, improving separation of concerns and consistency. User objects now include a fetchedAt timestamp for better tracking.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.