Add editor shortcuts, mentions, and callouts - #132
Conversation
- Add keyboard shortcuts for note creation, search, save, preview, and pinning - Surface and atomically convert unlinked note mentions - Render typed Markdown callouts with custom titles and nested content
PR Agent ReviewNote
Prompt to fix — P2 · Unmount flush sets state on unmounted component and can lose savesFix all findings (agent prompt)151800b ⋅ general ⋅ 8m 42s ⋅ meta/muse-spark-1.2-contributor |
![zeus-review[bot]](https://avatars.githubusercontent.com/in/3697123?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Track this run on the progress stub in the PR conversation.
quality Here's what the quality found.
| } | ||
| }; | ||
|
|
||
| const togglePin = async () => { |
There was a problem hiding this comment.
P2 · Collapse duplicated local vs remote persistence branches
web/src/app/notes/[id]/page.tsx · lines 437-510
The page repeats isLocal branches in togglePin, saveAndClose, and linkMention. Each branch clears the local timer and spreads title/body/updated_at. The same updateLocalNote pattern appears at line 184. The file now exceeds 900 lines and exceeds the 500-line warning.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, and keep changes minimal.
Repository: definitelynotguru/Recall
Pull request: #132
Head SHA: 151800bd404439249b7a02acf743bfe4fafdac73
[P2] @web/src/app/notes/[id]/page.tsx lines 437-510
Extract a persistence helper that owns isLocal, timer clearing, and updateLocalNote vs apiFetch. Page then calls one method for pin, save, and link.
| onTogglePreview?: ShortcutHandler; | ||
| onTogglePin?: ShortcutHandler; | ||
| }; | ||
|
|
There was a problem hiding this comment.
P2 · Stabilize useKeyboardShortcuts listener; remove per-render churn
web/src/hooks/useKeyboardShortcuts.ts · lines 14-42
The effect depends on the shortcuts object (line 41). Callers build a new literal each render and handlers change each render. Each state update removes and adds the window keydown listener. This causes churn and risks missed keys and stale closures.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, and keep changes minimal.
Repository: definitelynotguru/Recall
Pull request: #132
Head SHA: 151800bd404439249b7a02acf743bfe4fafdac73
[P2] @web/src/hooks/useKeyboardShortcuts.ts lines 14-42
Store handlers in refs and subscribe once. Update refs when callbacks change. Memoize page handlers with useCallback.
There was a problem hiding this comment.
Note
Track this run on the progress stub in the PR conversation.
tests Here's what the tests found.
| if (pinnedAt && Number.isNaN(pinnedAt.getTime())) { | ||
| if (requestedPinnedAt && Number.isNaN(requestedPinnedAt.getTime())) { | ||
| return errorResponse("Invalid pinned_at", 400); | ||
| } |
There was a problem hiding this comment.
P0 · PATCH link_mention early-return must not bump updatedAt
web/src/app/api/v1/notes/[id]/route.ts · lines 87-115
No test covers the early-return when link_mention finds no plain occurrence. The path at lines 98-111 returns the existing row without a write. Without a pin, a regression could bump updated_at or call captureRevisionIfChanged on a no-op, which pollutes sync last-writer-wins and revision history.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, and keep changes minimal.
Repository: definitelynotguru/Recall
Pull request: #132
Head SHA: 151800bd404439249b7a02acf743bfe4fafdac73
[P0] @web/src/app/api/v1/notes/[id]/route.ts lines 87-115
Add integration test for PATCH with link_mention that has no plain occurrence: mock FOR UPDATE select to return note with body '[[Project Atlas]]', call PATCH with {link_mention:'Project Atlas'}, assert linked_mention false, updated_at unchanged, and no UPDATE or revision capture.
| @@ -22,19 +23,31 @@ export function useDebouncedNoteSave( | |||
|
|
|||
There was a problem hiding this comment.
P1 · Cover disabled, empty id, and timer clear in debounced save
web/src/hooks/useDebouncedNoteSave.ts · lines 22-45
Tests cover timer cancellation and in-flight chaining, but not the disabled or empty-id branches. flush should return false and not call apiFetch when enabled is false or noteId is empty, and timer should clear on manual flush and unmount. Error path that sets status to error is also not asserted.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, and keep changes minimal.
Repository: definitelynotguru/Recall
Pull request: #132
Head SHA: 151800bd404439249b7a02acf743bfe4fafdac73
[P1] @web/src/hooks/useDebouncedNoteSave.ts lines 22-45
Add jsdom tests with fake timers: disabled and empty id return false without fetch, flush clears timer, unmount clears timer, and rejected fetch sets error status with retry.
| if (!title) return []; | ||
| return allNotes.filter( | ||
| (note) => | ||
| note.id !== currentId && |
There was a problem hiding this comment.
P1 · Ignore mentions inside inline and fenced code
web/src/lib/wiki-links.ts · lines 40-65
protectedRanges only covers wiki links and markdown links. Inline code (code) and fenced blocks (```) are not protected. A title inside code then counts as an unlinked mention and Link mention wraps it with [[ ]], which corrupts code content.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, and keep changes minimal.
Repository: definitelynotguru/Recall
Pull request: #132
Head SHA: 151800bd404439249b7a02acf743bfe4fafdac73
[P1] @web/src/lib/wiki-links.ts lines 40-65
Add ranges for inline code and fenced code to protectedRanges before searching. Add test: findUnlinkedMentionIndex('`Project Atlas`','Project Atlas') returns -1 and fenced block returns -1.
| @@ -109,6 +109,31 @@ export async function putLocalNote(note: ApiNote): Promise<void> { | |||
| await runWrite((store) => store.put(note)); | |||
There was a problem hiding this comment.
P2 · Cover missing id and abort for updateLocalNote
web/src/lib/local-notes.ts · lines 109-135
Existing test only covers successful update. The early return when id is missing (request.result falsy leaves updated undefined) and the abort/error paths are not covered. Missing coverage risks swallowed errors in offline pin and link flows.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, and keep changes minimal.
Repository: definitelynotguru/Recall
Pull request: #132
Head SHA: 151800bd404439249b7a02acf743bfe4fafdac73
[P2] @web/src/lib/local-notes.ts lines 109-135
Add fake-indexeddb tests: updateLocalNote for missing id resolves undefined without put, and transaction abort/error rejects and closes db.
There was a problem hiding this comment.
Note
Track this run on the progress stub in the PR conversation.
correctness Here's what the correctness found.
| onTogglePreview?: ShortcutHandler; | ||
| onTogglePin?: ShortcutHandler; | ||
| }; | ||
|
|
There was a problem hiding this comment.
P1 · Global shortcuts fire inside inputs/textareas
web/src/hooks/useKeyboardShortcuts.ts · lines 14-33
The window handler fires for every Ctrl/⌘ chord regardless of focus. When an input or textarea has focus, Ctrl+N still creates a note and Ctrl+Enter still triggers saveAndClose. This surprises the user and can navigate away during an edit.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, and keep changes minimal.
Repository: definitelynotguru/Recall
Pull request: #132
Head SHA: 151800bd404439249b7a02acf743bfe4fafdac73
[P1] @web/src/hooks/useKeyboardShortcuts.ts lines 14-33
Guard onKeyDown: return early if event.target is INPUT, TEXTAREA, SELECT or contentEditable, except for shortcuts that must work in inputs. Stabilize deps with refs to avoid churn.
| const nextBody = body.body ?? existing.body; | ||
| const pinnedAt = | ||
| const requestedPinnedAt = | ||
| body.pinned_at === undefined |
There was a problem hiding this comment.
P2 · PATCH link_mention drops body param and accepts whitespace
web/src/app/api/v1/notes/[id]/route.ts · lines 80-103
PATCH sets nextBody to link_mention result when link_mention exists and ignores body. A request with both fields silently drops the body edit. Whitespace-only link_mention passes z.string().min(1) but trims to empty and returns 200 with linked_mention false instead of 400.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, and keep changes minimal.
Repository: definitelynotguru/Recall
Pull request: #132
Head SHA: 151800bd404439249b7a02acf743bfe4fafdac73
[P2] @web/src/app/api/v1/notes/[id]/route.ts lines 80-103
Reject requests that send link_mention together with body, or merge explicitly, and validate link_mention.trim().length > 0 with 400 on failure.
There was a problem hiding this comment.
Note
Track this run on the progress stub in the PR conversation.
security Here's what the security found.
| @@ -11,12 +11,14 @@ import { | |||
| } from "@/lib/api-utils"; | |||
| import { eq, and, isNull } from "drizzle-orm"; | |||
There was a problem hiding this comment.
P2 · Unbounded link_mention enables ReDoS/DoS in FOR UPDATE PATCH
web/src/app/api/v1/notes/[id]/route.ts · lines 12-19
patchSchema adds link_mention as z.string().min(1) with no max. PATCH parses with await request.json() and bypasses readJsonBody 512KB limit. Server builds new RegExp(escapeRegExp(title), "gi") and scans the note body inside SELECT ... FOR UPDATE. An authenticated caller can send a large link_mention to force expensive regex and body scan while it holds the row lock.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, and keep changes minimal.
Repository: definitelynotguru/Recall
Pull request: #132
Head SHA: 151800bd404439249b7a02acf743bfe4fafdac73
[P2] @web/src/app/api/v1/notes/[id]/route.ts lines 12-19
Add length caps for link_mention, title and body and enforce request size with readJsonBody before parse. Validate link_mention length before RegExp.
1 participant
Summary
Validation
npm test(168 passed, 8 integration tests skipped without a database)npm run lint(0 errors, 12 existing warnings)npm run format:checknpm run buildnpm run knipnpm audit(0 vulnerabilities)Closes #51
Closes #61
Closes #85
PR Agent Description
PR Type
Enhancement, Bug fix, Tests
Description
Review map
web/src/app/api/v1/notes/[id]/route.ts: Atomic link_mention PATCH with FOR UPDATE and linked_mention contractweb/src/lib/wiki-links.ts: Core unlinked mention detection with protected link rangesweb/src/hooks/useDebouncedNoteSave.ts: Serialized flush via inFlight chain to prevent overlapping savesweb/src/app/notes/[id]/page.tsx: Pin, save-and-close, and mention link integration for local and remote notesweb/src/lib/local-notes.ts: New IndexedDB atomic updateLocalNote helper used by pin and autosave