feat: add pin notes#276
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements note pinning end-to-end: DB migration adds pinned_at, types and controller map pinned state and support pinnedAt sorting, adds toggle command+handler and telemetry, updates event payloads, UI components (preview/editor/context menu), list hooks, query callers/tests, and locales. ChangesNote pinning feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/core/features/notes/controller/NotesController.ts (1)
169-200:⚠️ Potential issue | 🟠 MajorPinned order is lost in the search branch.
PoC: when the text index returns
[unpinned, pinned]and the DB filter set is larger,intersectSets()keeps the index order, so the pinned note can still land below an unpinned one.
ORDER BY pinned DESConly affects the SQL-backed path. Please apply the same pin-first ordering to the final search result list before returning, otherwise the “pinned notes always appear at the top” guarantee is still bypassed for searches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/core/features/notes/controller/NotesController.ts` around lines 169 - 200, The search path currently bypasses the SQL ORDER BY pinned DESC, so after intersectSets() or wherever the search branch constructs the final results list in NotesController (look for the method using intersectSets and the orderQuery/ORDER BY logic), apply a stable sort to the final results array that places items with pinned=true before pinned=false while preserving the existing relative order (i.e., stable partition). In other words, after the search-result assembly but before returning from the NotesController method, reorder the array so pinned notes come first (use a stable algorithm or two-pass filter/concat) to guarantee the "pinned first" requirement regardless of whether results came from the DB or text index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/src/components/NotePreview/NotePreview.tsx`:
- Around line 62-76: The FaThumbtack currently rendered as a clickable Box in
NotePreview should be a real button to support keyboard focus/activation and
prevent click bubbling; replace the Box-as={FaThumbtack} with an IconButton (or
Chakra IconButton) wired to the same styling/props and change the click handler
to call event.stopPropagation() before invoking pin.onToggle (or wrap
pin.onToggle in a handler that stops propagation and then toggles). Ensure the
IconButton uses the same visual props (boxSize, color, transform, opacity,
_groupHover/_active equivalents) and sets an accessible aria-label/title so
keyboard users can activate it without triggering the note row's click handler.
---
Outside diff comments:
In `@packages/app/src/core/features/notes/controller/NotesController.ts`:
- Around line 169-200: The search path currently bypasses the SQL ORDER BY
pinned DESC, so after intersectSets() or wherever the search branch constructs
the final results list in NotesController (look for the method using
intersectSets and the orderQuery/ORDER BY logic), apply a stable sort to the
final results array that places items with pinned=true before pinned=false while
preserving the existing relative order (i.e., stable partition). In other words,
after the search-result assembly but before returning from the NotesController
method, reorder the array so pinned notes come first (use a stable algorithm or
two-pass filter/concat) to guarantee the "pinned first" requirement regardless
of whether results came from the DB or text index.
🪄 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
Run ID: 69f5d96a-ac0e-4454-8f37-5629f0629ec3
📒 Files selected for processing (11)
packages/app/src/components/NotePreview/NotePreview.tsxpackages/app/src/core/database/sqlite/migrations/index.tspackages/app/src/core/database/sqlite/migrations/sql/2_add_pinned_to_notes.sqlpackages/app/src/core/features/notes/controller/NotesController.tspackages/app/src/core/features/notes/controller/index.tspackages/app/src/core/features/telemetry/index.tspackages/app/src/features/MainScreen/NotesListPanel/NotesList.tsxpackages/app/src/features/NoteEditor/index.tsxpackages/app/src/hooks/commands/index.tspackages/app/src/hooks/notes/useNoteCommandHandlers.tspackages/app/src/locales/en/features.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/app/src/components/NotePreview/NotePreview.tsx (1)
63-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the pin toggle keyboard-reachable when inactive.
PoC (unit test):
it('lets keyboard users focus the pin toggle when note is not pinned', async () => { render( <NotePreview title="t" text="x" pin={{ title: 'Pin note', isActive: false, onToggle: vi.fn() }} />, ); await userEvent.tab(); expect(screen.getByRole('button', { name: 'Pin note' })).toHaveFocus(); });This currently fails because Line 70 sets
visibility="hidden", which removes the button from keyboard focus order until mouse hover.
Use opacity-based hiding (and reveal on hover/focus) instead ofvisibility.Suggested fix
- visibility={pin.isActive ? 'visible' : 'hidden'} - _groupHover={{ - visibility: 'visible', - }} + opacity={pin.isActive ? 1 : 0} + _groupHover={{ opacity: 1 }} + _groupFocusWithin={{ opacity: 1 }} + _focusVisible={{ opacity: 1 }}Based on learnings: in TSX files, bug findings should start with a minimal reproducing unit test (PoC).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/components/NotePreview/NotePreview.tsx` around lines 63 - 73, The pin Button in NotePreview currently uses visibility="hidden" which removes it from keyboard focus; update the Button (component named Button used with props aria-label={pin.title}, title={pin.title}, color={...}) to hide via opacity (e.g., opacity={pin.isActive ? undefined : 0}) instead of visibility and ensure it becomes visible on hover and keyboard focus by adding the reveal styles on both _groupHover and a focus state (e.g., _groupHover and _focusVisible or _groupFocusWithin) so keyboard users can tab to the pin toggle even when it’s not pinned; keep the existing aria-label/title and pin.onToggle behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/app/src/components/NotePreview/NotePreview.tsx`:
- Around line 63-73: The pin Button in NotePreview currently uses
visibility="hidden" which removes it from keyboard focus; update the Button
(component named Button used with props aria-label={pin.title},
title={pin.title}, color={...}) to hide via opacity (e.g., opacity={pin.isActive
? undefined : 0}) instead of visibility and ensure it becomes visible on hover
and keyboard focus by adding the reveal styles on both _groupHover and a focus
state (e.g., _groupHover and _focusVisible or _groupFocusWithin) so keyboard
users can tab to the pin toggle even when it’s not pinned; keep the existing
aria-label/title and pin.onToggle behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e11ec8c-5ab4-4646-a8e3-478368aaeb43
📒 Files selected for processing (1)
packages/app/src/components/NotePreview/NotePreview.tsx
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/app/src/core/features/notes/controller/NotesController.ts`:
- Around line 85-87: The isPinned flag is being transformed by the update
mapping in the case 'isPinned' inside formatNoteMeta() (pinned_at =
timestamp/null) and those transformed values are later used verbatim to build
WHERE predicates; extract the incoming isPinned filter out of meta before
calling formatNoteMeta(), do not pass it through the pinned_at update mapping,
and in the filter-building block (where you already handle isDeleted with IS
NULL/IS NOT NULL) add explicit handling for isPinned so that true maps to
"pinned_at IS NOT NULL" and false maps to "pinned_at IS NULL" instead of
generating equality against a timestamp or NULL. Ensure you still call
formatNoteMeta() for other meta fields and keep the 'case "isPinned"' update
mapping only for UPDATE operations.
In `@packages/app/src/features/MainScreen/NotesListPanel/usePinNoteEffect.tsx`:
- Around line 22-24: The flash timeout in usePinNoteEffect.tsx is racy and lacks
cleanup: before calling setTimeout that sets setFlashingNoteId(null) store the
timer id in a ref (e.g., flashTimerRef), clear any existing timer with
clearTimeout(flashTimerRef.current) so previous timeouts can't clear a new
flash, assign the new id to flashTimerRef.current, and add a cleanup return in
the effect to clearTimeout(flashTimerRef.current) on unmount; optionally check
the ref value before clearing to ensure only the intended timer is cleared.
🪄 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: 3c13a384-d075-4186-bf48-76c27a374c60
📒 Files selected for processing (24)
packages/app/src/components/NotePreview/NotePreview.theme.tspackages/app/src/components/NotePreview/NotePreview.tsxpackages/app/src/core/database/sqlite/migrations/index.tspackages/app/src/core/database/sqlite/migrations/sql/2_add_pinned_to_notes.sqlpackages/app/src/core/features/notes/bin/DeletedNotesController.tspackages/app/src/core/features/notes/controller/NotesController.filters.test.tspackages/app/src/core/features/notes/controller/NotesController.test.tspackages/app/src/core/features/notes/controller/NotesController.tspackages/app/src/core/features/notes/controller/NotesTextIndexer.tspackages/app/src/core/features/notes/controller/index.tspackages/app/src/core/features/telemetry/index.tspackages/app/src/core/storage/interop/export/index.tspackages/app/src/core/storage/interop/import/NotesImporter.test.tspackages/app/src/features/MainScreen/NotesListPanel/NotesList.tsxpackages/app/src/features/MainScreen/NotesListPanel/usePinNoteEffect.tsxpackages/app/src/features/NoteEditor/NoteMenu.tsxpackages/app/src/features/NoteEditor/index.tsxpackages/app/src/features/NotesContainer/NoteContextMenu/index.tspackages/app/src/features/NotesContainer/NoteContextMenu/useNoteContextMenu.tspackages/app/src/hooks/commands/index.tspackages/app/src/hooks/notes/useNoteCommandHandlers.tspackages/app/src/hooks/notes/useUpdateNotes.tspackages/app/src/locales/en/features.jsonpackages/app/src/locales/en/menu.json
|
@katsyuta what's status here? Is there any screenshots/description how it does work? |
I added a video to the pull request definition |
katsyuta
force-pushed
the
90-add-pin-notes
branch
from
f17cd06 to
c7ee0e2
Compare
|
@katsyuta I see a but on 0:27 The pin icon in notes list looks nice. For me it seems we have too many buttons to pin the note (3 or 4 if tabs have that option too, but they must not have, because it is different context). Choose most important ones. Also I don't like the pin button position in hamburger menu. It's too far down, but my expectation is pin feature will be relatively frequently used. And all features in that menu must be sorted by some meaning. For example the most frequent used features is on top, the most dangerous at the bottom. Now it seems the pin button is not frequently used and quite dangerous. Also the purple border looks like bug: 
I don't know why it shown here. |
|
@vitonsky , Ok, i fix you comments. Now pin and unpin actions never change the scroll position. Because when a user is browsing notes and pins the note, changing the scroll position leads to loss of context, and the user would need to scroll the notes list manually to come back. I also removed note highlighting. I agree about the number of pin buttons. I think the important places are the note context menu (only in the notes list context menu) and the note editor. This allows for quickly managing the note pin status. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app/src/hooks/notes/useNoteCommandHandlers.ts (1)
156-157: ⚡ Quick winEmit
reason: 'meta'for metadata-only update events.These handlers only change meta flags, so emitting an explicit meta reason keeps
NOTE_UPDATEDconsumers consistent with the pin flow and avoids unnecessary content-update reactions.♻️ Proposed change
- eventBus.emit(WorkspaceEvents.NOTE_UPDATED, { noteId }); + eventBus.emit(WorkspaceEvents.NOTE_UPDATED, { noteId, reason: 'meta' }); ... - eventBus.emit(WorkspaceEvents.NOTE_UPDATED, { noteId }); + eventBus.emit(WorkspaceEvents.NOTE_UPDATED, { noteId, reason: 'meta' });Also applies to: 177-178
🤖 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 `@packages/app/src/hooks/notes/useNoteCommandHandlers.ts` around lines 156 - 157, The NOTE_UPDATED emit currently sends only { noteId } for metadata-only updates; update the event payload in the metadata-only handlers to include reason: 'meta' (i.e., change eventBus.emit(WorkspaceEvents.NOTE_UPDATED, { noteId }) to eventBus.emit(WorkspaceEvents.NOTE_UPDATED, { noteId, reason: 'meta' })) so consumers can distinguish meta-only updates; apply the same change in both places that emit NOTE_UPDATED (the two eventBus.emit calls in useNoteCommandHandlers).
🤖 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.
Nitpick comments:
In `@packages/app/src/hooks/notes/useNoteCommandHandlers.ts`:
- Around line 156-157: The NOTE_UPDATED emit currently sends only { noteId } for
metadata-only updates; update the event payload in the metadata-only handlers to
include reason: 'meta' (i.e., change eventBus.emit(WorkspaceEvents.NOTE_UPDATED,
{ noteId }) to eventBus.emit(WorkspaceEvents.NOTE_UPDATED, { noteId, reason:
'meta' })) so consumers can distinguish meta-only updates; apply the same change
in both places that emit NOTE_UPDATED (the two eventBus.emit calls in
useNoteCommandHandlers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b45fd885-9dc8-4e2a-a658-b22a3c94cfdc
📒 Files selected for processing (26)
packages/app/src/api/events/workspace/index.tsxpackages/app/src/components/NotePreview/NotePreview.theme.tspackages/app/src/components/NotePreview/NotePreview.tsxpackages/app/src/core/database/sqlite/migrations/index.tspackages/app/src/core/database/sqlite/migrations/sql/2_add_pinned_to_notes.sqlpackages/app/src/core/features/notes/bin/DeletedNotesController.tspackages/app/src/core/features/notes/controller/NotesController.filters.test.tspackages/app/src/core/features/notes/controller/NotesController.test.tspackages/app/src/core/features/notes/controller/NotesController.tspackages/app/src/core/features/notes/controller/NotesTextIndexer.tspackages/app/src/core/features/notes/controller/index.tspackages/app/src/core/features/telemetry/index.tspackages/app/src/core/storage/interop/export/index.tspackages/app/src/core/storage/interop/import/NotesImporter.test.tspackages/app/src/features/MainScreen/NotesListPanel/NotesList.tsxpackages/app/src/features/MainScreen/NotesListPanel/useNotesData.tsxpackages/app/src/features/MainScreen/NotesListPanel/useScrollToActiveNote.tsxpackages/app/src/features/NoteEditor/index.tsxpackages/app/src/features/NotesContainer/NoteContextMenu/index.tspackages/app/src/features/NotesContainer/NoteContextMenu/useNoteContextMenu.tspackages/app/src/features/NotesContainer/index.tsxpackages/app/src/hooks/commands/index.tspackages/app/src/hooks/notes/useNoteCommandHandlers.tspackages/app/src/hooks/notes/useUpdateNotes.tspackages/app/src/locales/en/features.jsonpackages/app/src/locales/en/menu.json
| align: 'center', | ||
| userSelect: 'none', | ||
| paddingInlineEnd: '.3rem', | ||
| overflowAnchor: 'none', |
There was a problem hiding this comment.
Disable the browser scroll anchoring. Without it when notes are reordered (for example, when pinning from the context menu or the editor), the scroll position changes and jumps to the top.
The browser tries to keep the shifted element in view and therefore triggers automatic scrolling.
See: https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-anchor
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app/src/core/database/sqlite/migrations/sql/2_add_pinned_to_notes.sql (1)
1-2: ⚡ Quick winAdd an index for the new pin-ordering column.
Pinned-first sorting will likely query/order by
pinned_at; without an index this can degrade list performance as notes grow. Add a follow-up index migration (potentially composite with existing sort columns used by notes list queries).🤖 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 `@packages/app/src/core/database/sqlite/migrations/sql/2_add_pinned_to_notes.sql` around lines 1 - 2, The migration adds a pinned_at column to the notes table but does not create an index for queries that order/filter by pinned_at; add a follow-up SQL migration that creates an index on notes(pinned_at) (and if your notes-list queries also sort by another column, add a composite index such as notes(pinned_at, updated_at) or notes(pinned_at, created_at) to match that query pattern) so pinned-first listing remains performant as the table grows.
🤖 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.
Nitpick comments:
In
`@packages/app/src/core/database/sqlite/migrations/sql/2_add_pinned_to_notes.sql`:
- Around line 1-2: The migration adds a pinned_at column to the notes table but
does not create an index for queries that order/filter by pinned_at; add a
follow-up SQL migration that creates an index on notes(pinned_at) (and if your
notes-list queries also sort by another column, add a composite index such as
notes(pinned_at, updated_at) or notes(pinned_at, created_at) to match that query
pattern) so pinned-first listing remains performant as the table grows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77fd7eb7-b371-43ff-9642-ae583239a46c
📒 Files selected for processing (26)
packages/app/src/api/events/workspace/index.tsxpackages/app/src/components/NotePreview/NotePreview.theme.tspackages/app/src/components/NotePreview/NotePreview.tsxpackages/app/src/core/database/sqlite/migrations/index.tspackages/app/src/core/database/sqlite/migrations/sql/2_add_pinned_to_notes.sqlpackages/app/src/core/features/notes/bin/DeletedNotesController.tspackages/app/src/core/features/notes/controller/NotesController.filters.test.tspackages/app/src/core/features/notes/controller/NotesController.test.tspackages/app/src/core/features/notes/controller/NotesController.tspackages/app/src/core/features/notes/controller/NotesTextIndexer.tspackages/app/src/core/features/notes/controller/index.tspackages/app/src/core/features/telemetry/index.tspackages/app/src/core/storage/interop/export/index.tspackages/app/src/core/storage/interop/import/NotesImporter.test.tspackages/app/src/features/MainScreen/NotesListPanel/NotesList.tsxpackages/app/src/features/MainScreen/NotesListPanel/useNotesData.tsxpackages/app/src/features/MainScreen/NotesListPanel/useScrollToActiveNote.tsxpackages/app/src/features/NoteEditor/index.tsxpackages/app/src/features/NotesContainer/NoteContextMenu/index.tspackages/app/src/features/NotesContainer/NoteContextMenu/useNoteContextMenu.tspackages/app/src/features/NotesContainer/index.tsxpackages/app/src/hooks/commands/index.tspackages/app/src/hooks/notes/useNoteCommandHandlers.tspackages/app/src/hooks/notes/useUpdateNotes.tspackages/app/src/locales/en/features.jsonpackages/app/src/locales/en/menu.json
|
@vitonsky, PR is ready to review |
katsyuta
force-pushed
the
90-add-pin-notes
branch
from
5dd8b41 to
3835210
Compare
| [WorkspaceEvents.NOTE_UPDATED]: NoteId; | ||
| [WorkspaceEvents.NOTE_UPDATED]: { | ||
| noteId: NoteId; | ||
| reason?: NoteUpdateReason; |
There was a problem hiding this comment.
Why we needed in a reason here?
|
|
||
| export const notePreviewRecipe = defineSlotRecipe({ | ||
| slots: ['root', 'body', 'title', 'text', 'meta'], | ||
| slots: ['root', 'body', 'title', 'text', 'meta', 'pinIcon', 'header'], |
There was a problem hiding this comment.
- Order slots in order of appearance in DOM
- Don't use so specific slots as
pinIcon. What happens if we would have 2 or 3 icons? Would you add 3 slots for that? It would be difficult to maintain.
| ]); | ||
| }); | ||
|
|
||
| test('sorts notes by pinned time', async () => { |
There was a problem hiding this comment.
Too much code for that file. Should we split it? Maybe introduce the NotesController.sort.test.ts or something like that?
| ).resolves.toStrictEqual([note3, note1, note2]); | ||
| }); | ||
|
|
||
| test('sorts notes by pinned time and update time', async () => { |
There was a problem hiding this comment.
That test names gives me no information about your expectations. Read the internet how to write test names. It must define the expected behaviour.
I want to get known from the test name the answer on question "what about that test?" and get high level summary about the test idea. I don't want to see the generic names.
| // Meta changes (e.g. pin/unpin) reorder the notes list, but note data is updated with debounce, | ||
| // which can cause stale UI (e.g. after unpinning the note may still show the pin icon) | ||
| // To avoid inconsistency immediately refresh the updated note |
There was a problem hiding this comment.
It sounds as a workaround. Think better how to solve a problem with no that hack.
To be honest, i don't understand what the problem we have here.
If our data design are bad, improve that. We must not to handle "meta" updates different than the "data" updates, because it bloat the complexity.
Keep it simple and stupid.
2 participants
Closes #90
Pinned notes provide quick access to important notes and always appear first in the list.
They are sorted by pinning time, except in the bin and search results, where pin status is ignored.
You can pin a note using the note context menu or the button in the note editor.
Changes:
NotesControllersort API to support sorting pinned notes by pin time and other notes by update timeto-21.mp4
Summary by CodeRabbit