feat: add pin notes#276
Draft
katsyuta wants to merge 15 commits intoDeepinkApp:masterfrom
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds note pinning: schema migration and DB field, controller and types to surface pin state and ordering, a global toggle command and handler with telemetry, UI updates to show a pin control in previews and editor, and new English translations for pin/unpin actions. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as NotePreview / NoteEditor
participant Cmd as Command System
participant Controller as NotesController
participant DB as SQLite Database
participant Telemetry as Telemetry & Events
User->>UI: Click pin icon
UI->>Cmd: Dispatch TOGGLE_NOTE_PIN(noteId)
Cmd->>Controller: Invoke toggle handler
Controller->>Controller: Toggle isPinned state
Controller->>DB: UPDATE notes SET pinned = ? WHERE id = ?
DB-->>Controller: Acknowledge
Controller->>Telemetry: Emit NOTE_UPDATED & NOTE_PIN_TOGGLE
Controller-->>Cmd: Respond / emit event
Cmd-->>UI: UI updates (re-render)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 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)
Comment |
Collaborator
Author
|
@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
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
2 participants
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.
Closes #90
Pinned notes are always at the top of the notes list, except in search results, because search is ordered by relevance and raising pinned notes could break this behavior.
pin.mp4
Summary by CodeRabbit