PY-64: Notes#64
Merged
Merged
Conversation
✅ Deploy Preview for pno-project-y ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
More reviews will be available in 35 minutes and 28 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (30)
📝 WalkthroughWalkthroughThis PR introduces a full notes feature: three DB migrations add notes, user-scoped note labels, entity links, and archive support; server mutations and React Query hooks handle CRUD and live sync; a TipTap editor, split-layout detail page, sidebar (attachments/links/activity), list with filtering/grouping, and label management UI are all wired to new routes. The existing Labels and Statuses taxonomy pages are refactored to use a new reusable ChangesNotes System
Sequence Diagram(s)sequenceDiagram
participant User
participant NotesIndexRoute
participant NoteListRow
participant NotePageContent
participant NoteEditor
participant NoteSidebar
participant ServerFn as Server Functions
participant DB
User->>NotesIndexRoute: navigates to /notes
NotesIndexRoute->>ServerFn: fetchNotes, fetchNoteLabels
ServerFn->>DB: query notes, note_labels
ServerFn-->>NotesIndexRoute: NoteWithRelations[], NoteLabel[]
NotesIndexRoute->>NoteListRow: renders each note row
User->>NoteListRow: clicks note title
NoteListRow->>NotePageContent: navigates to /notes/$noteId
NotePageContent->>ServerFn: fetchNote, fetchNoteLinks
ServerFn->>DB: query note + relations + entity_links
ServerFn-->>NotePageContent: NoteWithRelations, NoteLinkedResource[]
User->>NoteEditor: types body content
NoteEditor->>NotePageContent: onSave after 500ms debounce
NotePageContent->>ServerFn: updateNote(id, body, bodyPlain)
ServerFn->>DB: update notes row
ServerFn-->>NotePageContent: ok
User->>NoteSidebar: drops file
NoteSidebar->>ServerFn: createAttachment(noteId, file)
ServerFn->>DB: insert attachment row
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/db/schema.ts (1)
348-370: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the attachment parent XOR at the database level.
The Zod refine below only protects the current server mutation path; the table itself still allows orphaned rows (
task_idandnote_idboth null) and doubly-linked rows (both set). Since downstream code assumes an attachment belongs to exactly one parent, this needs a DBCHECKconstraint, with the generated migration updated to match.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/schema.ts` around lines 348 - 370, The attachments table currently allows invalid parent combinations because only the Zod refine enforces the taskId/noteId XOR in application code. Update the attachments definition in schema.ts to add a database CHECK constraint on the attachment table so exactly one of taskId or noteId is non-null, and make sure the generated migration mirrors that constraint. Use the attachments/createTable definition and its existing taskId/noteId columns to locate the change.
🟡 Minor comments (13)
src/components/notes/NoteLabelSelect.tsx-79-80 (1)
79-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't map every create failure to a duplicate-name error.
This
catchalso handles network/auth/server failures, so users can get a false duplicate warning for unrelated problems. Surface the actual error when available and fall back to a generic "Failed to create note label" message instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/notes/NoteLabelSelect.tsx` around lines 79 - 80, The catch block in NoteLabelSelect’s label-creation flow is mapping every failure to a duplicate-name toast, which can misreport network/auth/server errors as name conflicts. Update the create-label handler in NoteLabelSelect to capture the thrown error from the label creation call, inspect it if available, and show the real message when possible; otherwise fall back to a generic “Failed to create note label” toast instead of always using the duplicate-name text.src/lib/format-relative-time.ts-9-9 (1)
9-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
constfordiffSecto satisfy lint and express immutability.Line 9 declares
diffSecwithlet, but it is never reassigned (prefer-const).Suggested fix
- let diffSec = Math.round((d.getTime() - now.getTime()) / 1000); + const diffSec = Math.round((d.getTime() - now.getTime()) / 1000);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/format-relative-time.ts` at line 9, `formatRelativeTime` computes `diffSec` once and never reassigns it, so change the declaration in `format-relative-time.ts` from a mutable binding to an immutable one. Update the `diffSec` variable in `formatRelativeTime` to use `const` so it aligns with the `prefer-const` lint rule and matches the intended one-time calculation.Source: Linters/SAST tools
src/db/queries/linkable-resources.ts-61-107 (1)
61-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't double-limit the per-type search results.
Tasks and projects are each queried with
limit: SEARCH_LIMIT, then concatenated with tasks first and truncated again. Once 20 task matches exist, every project match disappears, which makes some valid link targets unreachable from the picker. Apply the cap after a single merged ranking, or reserve slots per type explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/queries/linkable-resources.ts` around lines 61 - 107, The search in linkable-resources.ts is applying SEARCH_LIMIT separately to tasks and projects and then truncating the combined list in filterUnlinkedResources, which can starve project results when task matches are abundant. Update the logic around the matchedTasks/matchedProjects Promise.all flow and the resources aggregation so the cap is enforced after a single merged result set or by reserving explicit slots per type, ensuring both task and project matches can surface in the picker.src/lib/notes/note-activity.ts-36-43 (1)
36-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't hide real edits made within the first minute.
This collapses any save within 60 seconds of creation into the
"created"event, so legitimate early edits never appear in the activity feed. The safe dedupe case is identical timestamps, not an arbitrary one-minute window.Proposed fix
- if (Math.abs(updatedAt.getTime() - createdAt.getTime()) > 60_000) { + if (updatedAt.getTime() !== createdAt.getTime()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/notes/note-activity.ts` around lines 36 - 43, The activity dedupe logic in note-activity should not use a 60-second window to suppress updates, because that hides real early edits from the feed. Update the condition in the code that builds the items list so it only skips the “updated” event when updatedAt and createdAt are identical, and keep the “Note updated” entry for any real timestamp difference. Refer to the note activity builder around the updatedAt/createdAt comparison to make the change.src/db/mutations/note-labels.ts-143-154 (1)
143-154: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPatch or invalidate
note-labels-with-countshere too.The note-label management screen reads from
useNoteLabelsWithCountsQuery()insrc/routes/_signed-in/note-labels.tsx:33-114, but this mutation only updates["note-labels"]and note caches. After a rename/color change, that page can keep showing stale label data until an external refetch happens.Also applies to: 183-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/mutations/note-labels.ts` around lines 143 - 154, The label mutation update path only refreshes note-label caches, so the note-label management screen using useNoteLabelsWithCountsQuery can stay stale after a rename or color change. In the mutation handlers in note-labels.ts, update the same query cache key used by note-labels-with-counts as well as the existing ["note-labels"] and ["notes"] entries, or invalidate that query after the mutation succeeds so the UI refetches fresh data. Apply this in both relevant update flows in the note-label mutation module.src/db/mutations/attachments.ts-50-56 (1)
50-56: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse the same sync payload shape for task updates.
Line 52 sends
{ taskId }, while note updates here and task updates from attachment deletion use{ id }. Align this to avoid consumers missing the changed entity ID.Proposed fix
await sync("attachment-create", { data }); if (data.taskId) { - await sync(`task-update-${data.taskId}`, { data: { taskId: data.taskId } }); + await sync(`task-update-${data.taskId}`, { data: { id: data.taskId } }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/mutations/attachments.ts` around lines 50 - 56, The task update sync payload in the attachment mutation is inconsistent with the rest of the code and should use the same ID shape as note updates and attachment deletion. Update the payload passed to sync in the attachment create path inside the attachment mutation logic so task updates send the changed entity identifier as id, matching the existing sync contract used by the task-update and note-update flows.src/db/mutations/notes.ts-124-135 (1)
124-135: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMirror server-derived titles in the optimistic cache.
Line 126 stores the raw
noteData.title, but the server derivestitlefromdata.title/bodyPlainon Lines 84-89. Clearing a title with existing body text can leave the client cache withnullwhile the persisted note has a derived title.Proposed fix
const patchNote = (entry: NoteWithRelations): NoteWithRelations => ({ ...entry, - ...(noteData.title !== undefined ? { title: noteData.title } : {}), + ...(noteData.title !== undefined + ? { + title: deriveNoteTitle( + noteData.title, + noteData.bodyPlain ?? entry.bodyPlain + ), + } + : {}), ...(noteData.body !== undefined ? { body: noteData.body } : {}),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/mutations/notes.ts` around lines 124 - 135, Update the optimistic cache in patchNote so it mirrors the server-side title derivation used in the note mutation flow. Instead of blindly applying noteData.title, compute the cached title the same way the create/update logic does by deriving it from data.title/bodyPlain when title is absent or cleared. Keep patchNote in sync with the server’s title handling so NoteWithRelations stays consistent after optimistic updates.src/db/mutations/entity-links.ts-61-101 (1)
61-101: 🗄️ Data Integrity & Integration | 🟡 MinorHandle the concurrent duplicate-link race
entityLinksalready has a unique index on(owner, sourceType, sourceId, targetType, targetId, linkKind), so duplicate rows are blocked. The read-before-insert path still races under concurrent requests, though, and the loser can fail with a unique-violation instead of returning the existinglinkId. UseON CONFLICT DO NOTHING/RETURNINGor catch the conflict and fetch the row.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/mutations/entity-links.ts` around lines 61 - 101, The existing read-before-insert flow in entity-links.ts can race under concurrent requests, causing a unique-violation instead of returning the already-created linkId. Update the logic around the existing lookup and the insert in the entity-link mutation to use an atomic insert with conflict handling (for the unique key on entityLinks) or, if the insert conflicts, immediately query and return the existing row’s id. Keep the returned shape from the mutation unchanged.src/components/AttachmentArea.tsx-45-53 (1)
45-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis effect closes the panel dropzone after upload errors.
In the compact/sidebar flow,
onUploadErrorsetsshowUpload(true), but this effect runs as soon asisUploadingreturns tofalseand immediately resets it tofalseagain. That collapses the retry UI right after a failed upload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AttachmentArea.tsx` around lines 45 - 53, The AttachmentArea useEffect is overriding the compact/sidebar retry state after an upload failure by forcing setShowUpload(false) whenever isUploading becomes false. Update this effect so the panel dropzone stays open after onUploadError sets showUpload(true), preserving the retry UI in the panel flow while still switching view based on attachments.length and isPanelVariant. Use the existing AttachmentArea useEffect, setShowUpload, setView, and onUploadError flow to gate the reset logic instead of unconditionally closing the panel.src/components/notes/NoteListFilters.tsx-54-71 (1)
54-71: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Allnever becomes active while a search is present.
isNoteListFilterActive()countssearchQuery, butclearFilters()deliberately preserves it. That leaves the “All” chip visually inactive even after clicking it, which is confusing for the only clear/reset affordance in this group. Either clearsearchQueryhere or derive the chip state from chip-only filters.Also applies to: 103-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/notes/NoteListFilters.tsx` around lines 54 - 71, The “All” chip state is still being driven by `isNoteListFilterActive()` even though `clearFilters()` keeps `filters.searchQuery`, so it can remain visually inactive after reset. Update `NoteListFilters` so the All chip’s active state is based only on chip-controlled filters (or alternatively clear `searchQuery` in `clearFilters()`), and ensure the logic in `toggleLabel`/`clearFilters` matches that behavior consistently.src/components/site-header.tsx-39-49 (1)
39-49: 🎯 Functional Correctness | 🟡 MinorNormalize the shortcut key check.
event.key === "n"is brittle here because the shortcut includesShift; on common layouts this yields"N", so the handler can miss the intended chord. Useevent.code === "KeyN"or normalize withevent.key.toLowerCase().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/site-header.tsx` around lines 39 - 49, The keyboard shortcut check in useEffect is too brittle because it compares event.key directly while Shift is pressed, which can change the reported value and miss the chord. Update the handler in site-header.tsx to normalize the key comparison, either by using event.code for the N key or by lowercasing event.key before checking it, while keeping the existing meta/ctrl and shift modifiers intact in createNoteShortcut.src/components/PageLayout.tsx-29-45 (1)
29-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winOnly render the actions container when
actionsexists.Line 43 still mounts an empty wrapper on title-only pages, so the
mt-4/md:ml-4spacing applies even when there is nothing to show on the right.Suggested fix
- <div className="mt-4 flex shrink-0 items-center gap-2 md:mt-0 md:ml-4"> - {actions ? actions : null} - </div> + {actions ? ( + <div className="mt-4 flex shrink-0 items-center gap-2 md:mt-0 md:ml-4"> + {actions} + </div> + ) : null}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PageLayout.tsx` around lines 29 - 45, The actions wrapper in PageLayout should only be rendered when there are actual actions to show, because the always-mounted container still applies spacing on title-only pages. Update the conditional around the actions area in PageLayout so the right-side div is created only when actions is present, while keeping the existing header/title rendering unchanged.src/routes/_signed-in/notes.index.tsx-93-108 (1)
93-108: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winArchived-only workspaces hit the wrong empty state.
Line 93 treats “no active notes” as “no notes at all”, so a user with only archived notes sees “No notes yet” even though notes already exist.
Possible fix
- {!hasNotes && !filters.showArchived ? ( + {notesQuery.data.length === 0 ? ( <div className="text-muted-foreground flex flex-col items-center gap-3 py-16 text-center"> <StickyNote className="size-10 opacity-40" /> <p className="text-sm"> No notes yet. Create one to capture a quick thought. </p> </div> + ) : !hasNotes && hasArchivedNotes && !filters.showArchived && !isFilterActive ? ( + <div className="text-muted-foreground flex flex-col items-center gap-3 py-16 text-center"> + <StickyNote className="size-10 opacity-40" /> + <p className="text-sm">All notes are archived. Turn on Archived to view them.</p> + </div> ) : filteredNotes.length === 0 ? (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/_signed-in/notes.index.tsx` around lines 93 - 108, Update the empty-state branching in notes.index.tsx so “No notes yet” only shows when there are truly no notes, not merely no active notes. In the main conditional around the empty state, use the existing symbols hasNotes, filteredNotes.length, and filters.showArchived to distinguish archived-only workspaces from genuinely empty ones, and ensure archived-only users fall through to the archived-related empty state instead of the “No notes yet” message.
🧹 Nitpick comments (2)
src/components/taxonomy/TaxonomyManagementSection.tsx (1)
66-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winForward
createLink.paramsthrough the shared create button.
CreateLinkincludesparams, but this wrapper drops them. That makes the abstraction unusable for any taxonomy route whose create destination needs path params.Suggested fix
<PageCreateButton label={createButtonLabel} to={createLink.to} search={createLink.search} + params={createLink.params} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/taxonomy/TaxonomyManagementSection.tsx` around lines 66 - 70, Forward the missing create-route path params from TaxonomyManagementSection through PageCreateButton, since the shared create button currently passes only to and search and drops createLink.params. Update the TaxonomyManagementSection wrapper to include params when rendering PageCreateButton, and make sure PageCreateButton accepts and forwards params so create destinations that require route parameters still work.src/routes/_signed-in/notes.$noteId.tsx (1)
18-20: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid re-fetching the note in the loader.
You already load this query in
ensureQueryData; use that result forpageTitleinstead of callingfetchQueryagain.Proposed change
loader: async ({ context, params }) => { - await Promise.all([ - context.queryClient.ensureQueryData(noteQueryOptions(params.noteId)), + const [note] = await Promise.all([ + context.queryClient.ensureQueryData(noteQueryOptions(params.noteId)), context.queryClient.ensureQueryData(noteLinksQueryOptions(params.noteId)), context.queryClient.ensureQueryData(noteLabelsQueryOptions()), ]); - - const note = await context.queryClient.fetchQuery( - noteQueryOptions(params.noteId) - ); return { pageTitle: note?.title || "Untitled note",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/_signed-in/notes`.$noteId.tsx around lines 18 - 20, The loader is fetching the same note twice, once via ensureQueryData and again via fetchQuery for pageTitle. Update the notes.$noteId loader to reuse the result already returned by ensureQueryData, and reference that value when building pageTitle instead of calling fetchQuery again in the same loader.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/AttachmentListItem.tsx`:
- Around line 114-117: The AttachmentListItem deletion flow leaves isDeleting
stuck true if deleteAttachment(attachment.id) throws, so move the state reset
into a finally block around the async onConfirm handler. Update the onConfirm
logic in AttachmentListItem to setIsDeleting(true) before calling
deleteAttachment and always call setIsDeleting(false) afterward, regardless of
success or failure.
In `@src/components/notes/CreateNoteButton.tsx`:
- Around line 101-107: The useCreateNoteShortcut callback is currently
re-entrant, so repeated keydown events can trigger createEmptyNote multiple
times before navigation completes. Add a single-flight guard inside
useCreateNoteShortcut, using a ref or in-flight promise to ignore subsequent
invocations until the current createEmptyNote(createNote, navigate) call
finishes, while keeping the existing createNote and navigate dependencies
intact.
In `@src/components/notes/NoteEditor.tsx`:
- Around line 176-217: The icon-only toolbar controls in NoteEditor are missing
accessible names, so add an aria-label and ideally a title to each MenuBarButton
in the formatting toolbar. Update the button entries for bold, italic,
strikethrough, bullet list, ordered list, code block, and blockquote so
assistive tech can identify each action, using the existing button handlers in
NoteEditor and the MenuBarButton component.
- Around line 84-99: The debounced save in NoteEditor’s onUpdate is skipping the
first user edit after mount or note changes because the hasInitialized guard
returns early, leaving the initial body change unsaved if the user stops typing.
Update the NoteEditor onUpdate/hasInitialized flow so the first
post-initialization edit still schedules the save debounce, while only
suppressing the programmatic update from loading a note; use the existing
onUpdate, hasInitialized, debounceRef, and onSaveRef logic to distinguish those
cases.
In `@src/components/notes/NotePageContent.tsx`:
- Around line 35-58: The local title state in NotePageContent is only
initialized from the incoming note once, so when the note prop changes it can
keep a stale value and be reused by handleSaveBody when calling updateNote.
Update NotePageContent to synchronize the title state with note.title whenever
the note changes, and keep the existing save flow in
handleSaveBody/useUpdateNoteMutation using the current note.id and trimmed
title.
- Around line 74-100: The handleUpload callback is passing nullable upload
metadata straight into a required database field, so remove the unsafe cast on
attachment.serverData.uploadedBy and ensure createAttachment always receives a
non-null uploadedBy value. Use the existing handleUpload flow in NotePageContent
and either skip attachments with a null uploadedBy or map them to a valid
fallback before calling createAttachment, so the persisted uploadedBy column
never receives null.
In `@src/components/notes/use-note-label-color.ts`:
- Around line 4-22: The color-only label update helper is sending a full
snapshot instead of just the changed field, which can overwrite newer name/order
edits. Update toNoteLabelUpdate and the useUpdateNoteLabelColor flow so the
mutation payload only includes the label id and the new color, and keep the
early return logic in useUpdateNoteLabelColor intact.
In `@src/components/taxonomy/TaxonomyColorPicker.tsx`:
- Around line 16-45: The current TaxonomyColorPicker interaction uses
listbox/option roles on standalone buttons, which does not match the implemented
keyboard behavior. Update the component to use radiogroup/radio semantics around
the COLOR_VALUES map and the button items, or alternatively add full listbox
keyboard handling with roving focus if you keep listbox roles. Make sure the
selection state and disabled handling stay wired through the existing onChange,
value, and disabled props in TaxonomyColorPicker.
In `@src/components/taxonomy/TaxonomyListItem.tsx`:
- Around line 113-124: The icon-only Button controls in TaxonomyListItem lack
accessible names, so add a clear accessible label to each affected Button
(including the grip handle and the other icon-only action around the second
occurrence) using an aria-label or equivalent accessible text. Update the Button
usages in TaxonomyListItem so assistive technologies can identify the action,
while keeping the existing icon rendering and drag/listener behavior intact.
In `@src/components/taxonomy/TaxonomySortableList.tsx`:
- Around line 39-45: handleDragEnd in TaxonomySortableList currently treats a
missing drop target as a valid reorder, so cancelled or outside drops can still
persist changes. Update the DragEndEvent handling to return early when over is
null, and also guard against missing item positions by checking the oldIndex and
newIndex before calling onReorder(arrayMove(...)). Keep the fix localized to
handleDragEnd and use the existing sortedItems lookup logic.
In `@src/db/mutations/attachments.ts`:
- Around line 22-42: The attachment create flow in the mutations handler should
strictly validate parent ownership before inserting. Update the attachment
creation logic in the mutation that checks data.taskId and data.noteId so it
rejects requests with neither ID, rejects requests with both IDs, and does not
silently return when the parent task/note lookup fails. Use the existing
db.query.tasks.findFirst, db.query.notes.findFirst, and db.insert(attachments)
path to enforce a single valid parent and surface an explicit error instead of
leaving uploaded files orphaned.
In `@src/db/mutations/note-labels.ts`:
- Around line 196-224: The bulk update handler in updateMultipleNoteLabels
currently bypasses the same case-insensitive name-uniqueness validation enforced
by createNoteLabel and updateNoteLabel, so add that check before applying any
updates. In the updateMultipleNoteLabels createServerFn flow, validate the
incoming data against existing noteLabels for the session.user.id and reject any
duplicate name (excluding the label being updated) before the Promise.all
update/sync steps run. Reuse the same normalization/duplicate-detection logic as
the single-label mutation paths so the bulk path preserves the same server-side
invariant.
In `@src/db/mutations/notes.ts`:
- Around line 202-214: The note deletion flow in the delete mutation currently
removes the row before calling deleteAttachmentFilesFromStorage, so a storage
failure leaves orphaned files and makes retries fail with “Note not found.”
Update the note deletion path in the mutation around the db.delete(notes) and
deleteAttachmentFilesFromStorage calls to make attachment cleanup best-effort,
such as catching and logging cleanup errors after the row is deleted, or
queueing cleanup into a durable/background process instead of failing the whole
operation.
- Around line 271-282: The note label replacement logic in the mutation should
be atomic because the current delete-then-insert flow can leave partial state
and allow concurrent interleaving. Update the note label update path in the
relevant notes mutation function to run the delete and optional insert together
inside a single transaction on the same db instance, so the replacement either
fully succeeds or fully रोलs back. Use the existing note label replacement block
that deletes from noteLabelsToNotes and inserts via data.labelIds as the place
to wrap with a transaction.
In `@src/db/schema.ts`:
- Around line 278-289: The join table schema in noteLabelsToNotes only has the
composite primary key, so deletes and lookups by noteId are not indexed. Add a
dedicated note_id-leading index to the note_labels_to_notes table definition in
schema.ts, and make sure the corresponding migration creates that index as well
so the delete path in notes mutations can use it efficiently.
- Around line 272-275: The note-label unique constraint in the schema does not
match the case-insensitive behavior enforced by createNoteLabel, so duplicates
like "Bug" and "bug" can still slip through under concurrency. Update the
NoteLabel index definition in schema.ts to enforce uniqueness on a
normalized/lowercased name per user, and make the companion migration/backfill
apply the same case-insensitive rule. Use the existing createNoteLabel logic and
the note_label_user_name_unique index as the anchor points when updating both
schema and migration.
---
Outside diff comments:
In `@src/db/schema.ts`:
- Around line 348-370: The attachments table currently allows invalid parent
combinations because only the Zod refine enforces the taskId/noteId XOR in
application code. Update the attachments definition in schema.ts to add a
database CHECK constraint on the attachment table so exactly one of taskId or
noteId is non-null, and make sure the generated migration mirrors that
constraint. Use the attachments/createTable definition and its existing
taskId/noteId columns to locate the change.
---
Minor comments:
In `@src/components/AttachmentArea.tsx`:
- Around line 45-53: The AttachmentArea useEffect is overriding the
compact/sidebar retry state after an upload failure by forcing
setShowUpload(false) whenever isUploading becomes false. Update this effect so
the panel dropzone stays open after onUploadError sets showUpload(true),
preserving the retry UI in the panel flow while still switching view based on
attachments.length and isPanelVariant. Use the existing AttachmentArea
useEffect, setShowUpload, setView, and onUploadError flow to gate the reset
logic instead of unconditionally closing the panel.
In `@src/components/notes/NoteLabelSelect.tsx`:
- Around line 79-80: The catch block in NoteLabelSelect’s label-creation flow is
mapping every failure to a duplicate-name toast, which can misreport
network/auth/server errors as name conflicts. Update the create-label handler in
NoteLabelSelect to capture the thrown error from the label creation call,
inspect it if available, and show the real message when possible; otherwise fall
back to a generic “Failed to create note label” toast instead of always using
the duplicate-name text.
In `@src/components/notes/NoteListFilters.tsx`:
- Around line 54-71: The “All” chip state is still being driven by
`isNoteListFilterActive()` even though `clearFilters()` keeps
`filters.searchQuery`, so it can remain visually inactive after reset. Update
`NoteListFilters` so the All chip’s active state is based only on
chip-controlled filters (or alternatively clear `searchQuery` in
`clearFilters()`), and ensure the logic in `toggleLabel`/`clearFilters` matches
that behavior consistently.
In `@src/components/PageLayout.tsx`:
- Around line 29-45: The actions wrapper in PageLayout should only be rendered
when there are actual actions to show, because the always-mounted container
still applies spacing on title-only pages. Update the conditional around the
actions area in PageLayout so the right-side div is created only when actions is
present, while keeping the existing header/title rendering unchanged.
In `@src/components/site-header.tsx`:
- Around line 39-49: The keyboard shortcut check in useEffect is too brittle
because it compares event.key directly while Shift is pressed, which can change
the reported value and miss the chord. Update the handler in site-header.tsx to
normalize the key comparison, either by using event.code for the N key or by
lowercasing event.key before checking it, while keeping the existing meta/ctrl
and shift modifiers intact in createNoteShortcut.
In `@src/db/mutations/attachments.ts`:
- Around line 50-56: The task update sync payload in the attachment mutation is
inconsistent with the rest of the code and should use the same ID shape as note
updates and attachment deletion. Update the payload passed to sync in the
attachment create path inside the attachment mutation logic so task updates send
the changed entity identifier as id, matching the existing sync contract used by
the task-update and note-update flows.
In `@src/db/mutations/entity-links.ts`:
- Around line 61-101: The existing read-before-insert flow in entity-links.ts
can race under concurrent requests, causing a unique-violation instead of
returning the already-created linkId. Update the logic around the existing
lookup and the insert in the entity-link mutation to use an atomic insert with
conflict handling (for the unique key on entityLinks) or, if the insert
conflicts, immediately query and return the existing row’s id. Keep the returned
shape from the mutation unchanged.
In `@src/db/mutations/note-labels.ts`:
- Around line 143-154: The label mutation update path only refreshes note-label
caches, so the note-label management screen using useNoteLabelsWithCountsQuery
can stay stale after a rename or color change. In the mutation handlers in
note-labels.ts, update the same query cache key used by note-labels-with-counts
as well as the existing ["note-labels"] and ["notes"] entries, or invalidate
that query after the mutation succeeds so the UI refetches fresh data. Apply
this in both relevant update flows in the note-label mutation module.
In `@src/db/mutations/notes.ts`:
- Around line 124-135: Update the optimistic cache in patchNote so it mirrors
the server-side title derivation used in the note mutation flow. Instead of
blindly applying noteData.title, compute the cached title the same way the
create/update logic does by deriving it from data.title/bodyPlain when title is
absent or cleared. Keep patchNote in sync with the server’s title handling so
NoteWithRelations stays consistent after optimistic updates.
In `@src/db/queries/linkable-resources.ts`:
- Around line 61-107: The search in linkable-resources.ts is applying
SEARCH_LIMIT separately to tasks and projects and then truncating the combined
list in filterUnlinkedResources, which can starve project results when task
matches are abundant. Update the logic around the matchedTasks/matchedProjects
Promise.all flow and the resources aggregation so the cap is enforced after a
single merged result set or by reserving explicit slots per type, ensuring both
task and project matches can surface in the picker.
In `@src/lib/format-relative-time.ts`:
- Line 9: `formatRelativeTime` computes `diffSec` once and never reassigns it,
so change the declaration in `format-relative-time.ts` from a mutable binding to
an immutable one. Update the `diffSec` variable in `formatRelativeTime` to use
`const` so it aligns with the `prefer-const` lint rule and matches the intended
one-time calculation.
In `@src/lib/notes/note-activity.ts`:
- Around line 36-43: The activity dedupe logic in note-activity should not use a
60-second window to suppress updates, because that hides real early edits from
the feed. Update the condition in the code that builds the items list so it only
skips the “updated” event when updatedAt and createdAt are identical, and keep
the “Note updated” entry for any real timestamp difference. Refer to the note
activity builder around the updatedAt/createdAt comparison to make the change.
In `@src/routes/_signed-in/notes.index.tsx`:
- Around line 93-108: Update the empty-state branching in notes.index.tsx so “No
notes yet” only shows when there are truly no notes, not merely no active notes.
In the main conditional around the empty state, use the existing symbols
hasNotes, filteredNotes.length, and filters.showArchived to distinguish
archived-only workspaces from genuinely empty ones, and ensure archived-only
users fall through to the archived-related empty state instead of the “No notes
yet” message.
---
Nitpick comments:
In `@src/components/taxonomy/TaxonomyManagementSection.tsx`:
- Around line 66-70: Forward the missing create-route path params from
TaxonomyManagementSection through PageCreateButton, since the shared create
button currently passes only to and search and drops createLink.params. Update
the TaxonomyManagementSection wrapper to include params when rendering
PageCreateButton, and make sure PageCreateButton accepts and forwards params so
create destinations that require route parameters still work.
In `@src/routes/_signed-in/notes`.$noteId.tsx:
- Around line 18-20: The loader is fetching the same note twice, once via
ensureQueryData and again via fetchQuery for pageTitle. Update the notes.$noteId
loader to reuse the result already returned by ensureQueryData, and reference
that value when building pageTitle instead of calling fetchQuery again in the
same loader.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9425b13b-6d44-4400-b354-c2e073d4bb95
📒 Files selected for processing (82)
drizzle/0007_chemical_speedball.sqldrizzle/0008_note_labels.sqldrizzle/0009_note_archive.sqldrizzle/meta/0007_snapshot.jsondrizzle/meta/0008_snapshot.jsondrizzle/meta/0009_snapshot.jsondrizzle/meta/_journal.jsonsrc/components/AttachmentArea.tsxsrc/components/AttachmentListItem.tsxsrc/components/DetailSplitLayout.tsxsrc/components/EntityFormSheets.tsxsrc/components/EntityList.tsxsrc/components/LabelSelect.tsxsrc/components/PageBackLink.tsxsrc/components/PageLayout.tsxsrc/components/PageSection.tsxsrc/components/app-sidebar.tsxsrc/components/forms/EntityConfigCreateForm.tsxsrc/components/nav-main.tsxsrc/components/notes/CreateNoteButton.tsxsrc/components/notes/NoteArchivedBadge.tsxsrc/components/notes/NoteEditableLabelBadge.tsxsrc/components/notes/NoteEditor.tsxsrc/components/notes/NoteLabelColorPicker.tsxsrc/components/notes/NoteLabelSelect.tsxsrc/components/notes/NoteLabels.tsxsrc/components/notes/NoteLinkPicker.tsxsrc/components/notes/NoteLinkedResourceItem.tsxsrc/components/notes/NoteListFilters.tsxsrc/components/notes/NoteListRow.tsxsrc/components/notes/NotePageContent.tsxsrc/components/notes/NotePageMenu.tsxsrc/components/notes/NoteSidebar.tsxsrc/components/notes/styles.csssrc/components/notes/use-note-label-color.tssrc/components/site-header.tsxsrc/components/taxonomy/TaxonomyColorPicker.tsxsrc/components/taxonomy/TaxonomyListItem.tsxsrc/components/taxonomy/TaxonomyManagementSection.tsxsrc/components/taxonomy/TaxonomySortableList.tsxsrc/components/taxonomy/index.tssrc/components/taxonomy/types.tssrc/db/mutations/attachments.server.tssrc/db/mutations/attachments.tssrc/db/mutations/entity-links.tssrc/db/mutations/note-labels.tssrc/db/mutations/notes.tssrc/db/mutations/sync.tssrc/db/queries/attachments.tssrc/db/queries/linkable-resources.tssrc/db/queries/note-labels.tssrc/db/queries/note-links.tssrc/db/queries/notes.tssrc/db/schema.tssrc/hooks/live-sync-provider.tsxsrc/lib/form-sheet-search.tssrc/lib/format-relative-time.test.tssrc/lib/format-relative-time.tssrc/lib/notes/derive-title.test.tssrc/lib/notes/derive-title.tssrc/lib/notes/entity-link-utils.tssrc/lib/notes/filter-notes.test.tssrc/lib/notes/filter-notes.tssrc/lib/notes/linkable-resources.test.tssrc/lib/notes/linkable-resources.tssrc/lib/notes/list-preview.test.tssrc/lib/notes/list-preview.tssrc/lib/notes/note-activity.test.tssrc/lib/notes/note-activity.tssrc/lib/page-layout-width.tssrc/routeTree.gen.tssrc/routes/_signed-in/labels.tsxsrc/routes/_signed-in/note-labels.tsxsrc/routes/_signed-in/notes.$noteId.tsxsrc/routes/_signed-in/notes.index.tsxsrc/routes/_signed-in/notes.tsxsrc/routes/_signed-in/projects.$projectId.tasks.tsxsrc/routes/_signed-in/settings.integrations.tsxsrc/routes/_signed-in/sprints.$sprintId.tasks.tsxsrc/routes/_signed-in/statuses.tsxsrc/routes/_signed-in/tasks.tsxsrc/server/uploadthing.ts
💤 Files with no reviewable changes (1)
- src/components/EntityList.tsx
Comment on lines
+272
to
+275
| (example) => [ | ||
| index("note_label_user_idx").on(example.userId), | ||
| uniqueIndex("note_label_user_name_unique").on(example.userId, example.name), | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make note-label uniqueness match the app's case-insensitive contract.
createNoteLabel treats "Bug" and "bug" as duplicates, but this unique index does not. Two concurrent requests can both pass the lower(name) pre-check and commit case-variant duplicates that the rest of the app considers the same label. Please enforce the uniqueness rule case-insensitively here and in the companion migration/backfill.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db/schema.ts` around lines 272 - 275, The note-label unique constraint in
the schema does not match the case-insensitive behavior enforced by
createNoteLabel, so duplicates like "Bug" and "bug" can still slip through under
concurrency. Update the NoteLabel index definition in schema.ts to enforce
uniqueness on a normalized/lowercased name per user, and make the companion
migration/backfill apply the same case-insensitive rule. Use the existing
createNoteLabel logic and the note_label_user_name_unique index as the anchor
points when updating both schema and migration.
|
🎉 This PR is included in version 0.14.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
Task
PY-64: Notes
Summary
Summary by CodeRabbit
New Features
Bug Fixes