feat: note tags with tag filtering - #18
Open
ark-commits wants to merge 1 commit into
Open
Conversation
Notes can now be tagged with short labels. Tags are added inline in the sidebar via a text input on the active note, and removed with the × button. A tag filter bar above the notes list lets you filter to notes matching all selected tags. Tags persist to localStorage with the note. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Cache: Disabled due to Reviews > Disable Cache setting 📝 WalkthroughWalkthroughAdds per-note tag metadata, tag CRUD handlers, and UI for tag filtering. Notes are normalized to include ChangesTag System Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App.jsx`:
- Around line 369-377: In handleAddTag the duplicate check uses rawTag while you
normalize to tag, so variants like different case/spacing bypass dedupe; update
the contains check to use the normalized tag (change note.tags.includes(rawTag)
to note.tags.includes(tag)) and ensure the tags array stores the normalized
value (you already push tag), so in setNotes -> previousNotes.map(...) use the
normalized tag for both the includes check and when returning { ...note, tags:
[...note.tags, tag] }.
- Around line 148-151: visibleNotes is memoized with useMemo but only lists
notes in the dependency array while it also reads activeTags, causing stale
results when filters change; update the useMemo dependency array for
visibleNotes to include activeTags (so useMemo depends on both notes and
activeTags) to ensure the filtered list recomputes when either value changes
(check the visibleNotes useMemo declaration and adjust its dependency array
accordingly).
- Around line 381-393: When removing a tag in handleRemoveTag, also reconcile
activeTags so it doesn't contain tags that no longer exist in the notes: after
you call setNotes (in handleRemoveTag) compute the new set of tags present
across the updated notes (e.g. derive newAllTags from the updated notes payload)
and call setActiveTags to filter out any active tag not in newAllTags; update
handleRemoveTag to perform both the notes update and an activeTags cleanup so
dangling active filters are removed when tags are deleted from all notes.
In `@src/components/NotesSidebar.jsx`:
- Around line 126-137: The tag filter buttons in NotesSidebar.jsx don’t expose
their selected state to assistive tech; update the button rendering within the
tag map (the element using key={tag} and onClick={() => onToggleTagFilter(tag)})
to include the ARIA pressed state by adding aria-pressed={isActive} so screen
readers announce whether each toggle chip is active; ensure you use the existing
isActive boolean that's used to choose classes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 689efc4f-7089-4d22-8ae5-dab4aa3f0d74
📒 Files selected for processing (2)
src/App.jsxsrc/components/NotesSidebar.jsx
Comment on lines
+148
to
+151
| const visibleNotes = useMemo(() => { | ||
| if (activeTags.length === 0) return notes | ||
| return notes.filter((note) => activeTags.every((tag) => note.tags.includes(tag))) | ||
| }, [notes]) |
There was a problem hiding this comment.
Fix stale visibleNotes derivation by including activeTags in dependencies.
visibleNotes reads activeTags, but the memo only depends on notes, so toggling filters won’t recompute the list reliably.
Suggested patch
const visibleNotes = useMemo(() => {
if (activeTags.length === 0) return notes
return notes.filter((note) => activeTags.every((tag) => note.tags.includes(tag)))
- }, [notes])
+ }, [notes, activeTags])📝 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
| const visibleNotes = useMemo(() => { | |
| if (activeTags.length === 0) return notes | |
| return notes.filter((note) => activeTags.every((tag) => note.tags.includes(tag))) | |
| }, [notes]) | |
| const visibleNotes = useMemo(() => { | |
| if (activeTags.length === 0) return notes | |
| return notes.filter((note) => activeTags.every((tag) => note.tags.includes(tag))) | |
| }, [notes, activeTags]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App.jsx` around lines 148 - 151, visibleNotes is memoized with useMemo
but only lists notes in the dependency array while it also reads activeTags,
causing stale results when filters change; update the useMemo dependency array
for visibleNotes to include activeTags (so useMemo depends on both notes and
activeTags) to ensure the filtered list recomputes when either value changes
(check the visibleNotes useMemo declaration and adjust its dependency array
accordingly).
Comment on lines
+369
to
+377
| const handleAddTag = (noteId, rawTag) => { | ||
| const tag = rawTag.trim().toLowerCase() | ||
| if (!tag) return | ||
| setNotes((previousNotes) => | ||
| previousNotes.map((note) => { | ||
| if (note.id !== noteId) return note | ||
| if (note.tags.includes(rawTag)) return note | ||
| return { ...note, tags: [...note.tags, tag] } | ||
| }), |
There was a problem hiding this comment.
Use normalized tag for duplicate detection in handleAddTag.
You normalize to tag, but dedupe checks rawTag, so equivalent tags (case/spacing differences) can be inserted multiple times.
Suggested patch
const handleAddTag = (noteId, rawTag) => {
const tag = rawTag.trim().toLowerCase()
if (!tag) return
setNotes((previousNotes) =>
previousNotes.map((note) => {
if (note.id !== noteId) return note
- if (note.tags.includes(rawTag)) return note
+ if (note.tags.includes(tag)) return note
return { ...note, tags: [...note.tags, tag] }
}),
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App.jsx` around lines 369 - 377, In handleAddTag the duplicate check uses
rawTag while you normalize to tag, so variants like different case/spacing
bypass dedupe; update the contains check to use the normalized tag (change
note.tags.includes(rawTag) to note.tags.includes(tag)) and ensure the tags array
stores the normalized value (you already push tag), so in setNotes ->
previousNotes.map(...) use the normalized tag for both the includes check and
when returning { ...note, tags: [...note.tags, tag] }.
Comment on lines
+381
to
+393
| const handleRemoveTag = (noteId, tag) => { | ||
| setNotes((previousNotes) => | ||
| previousNotes.map((note) => | ||
| note.id === noteId ? { ...note, tags: note.tags.filter((t) => t !== tag) } : note, | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| const handleToggleTagFilter = (tag) => { | ||
| setActiveTags((previous) => | ||
| previous.includes(tag) ? previous.filter((t) => t !== tag) : [...previous, tag], | ||
| ) | ||
| } |
There was a problem hiding this comment.
Reconcile activeTags when tags are removed from notes.
After removing tags, activeTags can contain values no longer present in allTags; this can leave the list filtered with no visible chip to deselect (especially when allTags becomes empty).
Suggested patch
const allTags = useMemo(
() => [...new Set(notes.flatMap((note) => note.tags))].sort(),
[notes],
)
+
+ useEffect(() => {
+ setActiveTags((previous) => {
+ const next = previous.filter((tag) => allTags.includes(tag))
+ return next.length === previous.length ? previous : next
+ })
+ }, [allTags])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App.jsx` around lines 381 - 393, When removing a tag in handleRemoveTag,
also reconcile activeTags so it doesn't contain tags that no longer exist in the
notes: after you call setNotes (in handleRemoveTag) compute the new set of tags
present across the updated notes (e.g. derive newAllTags from the updated notes
payload) and call setActiveTags to filter out any active tag not in newAllTags;
update handleRemoveTag to perform both the notes update and an activeTags
cleanup so dangling active filters are removed when tags are deleted from all
notes.
Comment on lines
+126
to
+137
| <button | ||
| key={tag} | ||
| type="button" | ||
| onClick={() => onToggleTagFilter(tag)} | ||
| className={`rounded-full px-2 py-0.5 text-xs font-medium transition ${ | ||
| isActive | ||
| ? 'bg-ink text-white' | ||
| : 'bg-slate-200 text-slate-600 hover:bg-slate-300' | ||
| }`} | ||
| > | ||
| #{tag} | ||
| </button> |
There was a problem hiding this comment.
Expose selected state on tag filter toggle buttons.
These buttons act as toggles, but selected state isn’t announced to screen readers. Add aria-pressed={isActive} on each filter chip.
Suggested patch
<button
key={tag}
type="button"
onClick={() => onToggleTagFilter(tag)}
+ aria-pressed={isActive}
className={`rounded-full px-2 py-0.5 text-xs font-medium transition ${
isActive
? 'bg-ink text-white'
: 'bg-slate-200 text-slate-600 hover:bg-slate-300'
}`}
>📝 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
| <button | |
| key={tag} | |
| type="button" | |
| onClick={() => onToggleTagFilter(tag)} | |
| className={`rounded-full px-2 py-0.5 text-xs font-medium transition ${ | |
| isActive | |
| ? 'bg-ink text-white' | |
| : 'bg-slate-200 text-slate-600 hover:bg-slate-300' | |
| }`} | |
| > | |
| #{tag} | |
| </button> | |
| <button | |
| key={tag} | |
| type="button" | |
| onClick={() => onToggleTagFilter(tag)} | |
| aria-pressed={isActive} | |
| className={`rounded-full px-2 py-0.5 text-xs font-medium transition ${ | |
| isActive | |
| ? 'bg-ink text-white' | |
| : 'bg-slate-200 text-slate-600 hover:bg-slate-300' | |
| }`} | |
| > | |
| #{tag} | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/NotesSidebar.jsx` around lines 126 - 137, The tag filter
buttons in NotesSidebar.jsx don’t expose their selected state to assistive tech;
update the button rendering within the tag map (the element using key={tag} and
onClick={() => onToggleTagFilter(tag)}) to include the ARIA pressed state by
adding aria-pressed={isActive} so screen readers announce whether each toggle
chip is active; ensure you use the existing isActive boolean that's used to
choose classes.
Owner
Author
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Added the ability to tag notes with short labels. You can add tags inline from the sidebar on any active note, remove them with the × button, and filter the notes list by one or more tags using the filter bar that appears once you've tagged anything. Tags are lowercased and stored alongside the note in localStorage.
@coderabbitai summary