refactor: split Editor.tsx into smaller components and hooks#81
Closed
refactor: split Editor.tsx into smaller components and hooks#81
Conversation
Break the 2300-line monolithic Editor component into focused modules: - FormatBar.tsx: Toolbar with formatting buttons and table grid picker - EditorTopBar.tsx: Top bar with save status, pin, search, export controls - SearchHighlightExtension.ts: TipTap extension for search match highlighting - useNoteSaving.ts: Save/load logic, source mode, auto-save debouncing - useEditorExtensions.ts: TipTap editor setup with all extensions - useEditorSearch.ts: Find-in-note with decorations and navigation - usePopupManager.ts: Link, block math, and image popup management - useCopyExport.ts: Copy and export handlers (markdown, plain text, HTML, PDF) - useTableContextMenu.ts: Table right-click context menu via Tauri Editor.tsx is now a ~530-line orchestrator that composes these hooks and components, down from 2300 lines (77% reduction). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughA major refactoring of the Editor component that extracts large inline logic into modular hooks (useNoteSaving, usePopupManager, useEditorExtensions, useEditorSearch, useCopyExport, useTableContextMenu) and new components (EditorTopBar, FormatBar). A SearchHighlightExtension is introduced for search functionality. The onEditorReady public API type signature is updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/editor/Editor.tsx (1)
168-177:⚠️ Potential issue | 🟠 MajorPrevent unnecessary settings fetch churn and handle failures visibly.
getSettings()currently re-runs onnotesmutations and failures are either console-only or uncaught in the reload callback.💡 Suggested patch
useEffect(() => { - if (currentNote?.id && !previewMode) { - notesService - .getSettings() - .then(setSettings) - .catch((error) => { - console.error("Failed to load settings:", error); - }); - } - }, [currentNote?.id, notes, previewMode]); + if (!currentNote?.id || previewMode) return; + let cancelled = false; + void notesService + .getSettings() + .then((next) => { + if (!cancelled) setSettings(next); + }) + .catch((error) => { + console.error("Failed to load settings:", error); + toast.error("Couldn't load editor settings."); + }); + return () => { + cancelled = true; + }; + }, [currentNote?.id, previewMode]); const handleSettingsReload = useCallback(async () => { - const updatedSettings = await notesService.getSettings(); - setSettings(updatedSettings); + try { + const updatedSettings = await notesService.getSettings(); + setSettings(updatedSettings); + } catch (error) { + console.error("Failed to reload settings:", error); + toast.error("Couldn't refresh settings."); + } }, []);As per coding guidelines, "Implement proper error handling with user-friendly messages for all non-blocking async operations".
Also applies to: 182-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/editor/Editor.tsx` around lines 168 - 177, The useEffect is re-fetching settings on any "notes" change and only logs failures to console; update the dependency array to [currentNote?.id, previewMode] so notes mutations don't trigger getSettings, and replace the console.error handler in the notesService.getSettings().catch(...) with a user-visible error surface (e.g., call an existing notify/showToast or set an error state) so failures are visible to users; likewise, locate any reload callback that re-invokes getSettings (refer to notesService.getSettings, setSettings and the reload handler in this component) and ensure it returns/awaits the promise and handles errors by surfacing them via the same notification mechanism instead of allowing uncaught rejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/editor/EditorTopBar.tsx`:
- Around line 125-132: The async handlers in EditorTopBar (notably onReload and
the pin/unpin handlers) currently swallow failures — wrap these async calls in
try/catch, show a user-facing error (e.g., via the app's toast/notification API
or a small inline error state) and ensure UI feedback for in-flight state
(disable the button or show a spinner) and re-enable on completion; specifically
update onReload (and the pin/unpin handlers) to await the async operation, catch
errors and call the notification helper (or set an error state) with a clear
message, log the full error for debugging, and ensure the button state
(disabled/loading) is toggled so users get immediate feedback.
In `@src/components/editor/useCopyExport.ts`:
- Around line 95-97: The keyboard shortcut check is case-sensitive and misses
"C" when Shift is held; update the key comparison used alongside the existing
modifier checks (the condition that currently tests (e.metaKey || e.ctrlKey) &&
e.shiftKey && e.key === "c") to compare the key case-insensitively (e.g.,
normalize e.key with toLowerCase() before comparing) so the
setCopyMenuOpen(true) branch fires for both "c" and "C".
In `@src/components/editor/useEditorExtensions.ts`:
- Around line 144-146: The frontend assumes get_notes_folder returns a non-null
string; update the invoke call to use invoke<string | null>("get_notes_folder")
and then explicitly check notesFolder for null before using join(notesFolder,
relativePath); if null, handle it (e.g., log/error/return) so join and
convertFileSrc are never called with a null path (refer to notesFolder,
get_notes_folder, join, and convertFileSrc).
- Around line 118-123: The handleKeyDown in useEditorExtensions.ts currently
always returns false, so Tab handling is a no-op; update the handleKeyDown
implementation to detect event.key === "Tab", call event.preventDefault(),
perform the intended Tab behavior (e.g., indentation logic or call existing
indent function), and then return true to mark the event handled, while keeping
non-Tab keys returning false; locate the handleKeyDown handler in
useEditorExtensions.ts and modify it to preventDefault and return true for Tab
events.
In `@src/components/editor/useEditorSearch.ts`:
- Around line 223-231: The reset block currently only runs when currentNoteId is
truthy, leaving stale search UI and highlights when currentNoteId becomes
undefined; change the condition so the reset runs when there is no active note
(i.e., when currentNoteId is falsy) or remove the guard entirely so the
following calls always execute: setSearchOpen(false), setSearchQuery(""),
setSearchMatches([]), setCurrentMatchIndex(0), and—if editor
exists—updateSearchDecorations([], 0, editor); update the check around
currentNoteId in useEditorSearch.ts to reference currentNoteId appropriately and
ensure updateSearchDecorations is still called when editor is defined.
In `@src/components/editor/useNoteSaving.ts`:
- Around line 121-132: The autosave debounce and error handling need fixing:
change the timeout delay from 500 to 300ms (update the window.setTimeout call)
and wrap the async save inside the timer callback in a try/catch around await
saveImmediately(savingNoteId, markdown); on catch set needsSaveRef.current =
true to preserve pending changes and call the app's user-facing toast/error
helper (e.g., showSaveErrorToast or notifyUser) with a concise message; keep the
early return checks using currentNoteIdRef and needsSaveRef and still clear/set
saveTimeoutRef as before.
In `@src/components/editor/usePopupManager.ts`:
- Around line 407-432: The handleAddImage flow in usePopupManager (function
handleAddImage) assumes invoke("get_notes_folder") always returns a string but
backend can return null/Option; update the code to treat notesFolder as possibly
null/undefined, validate notesFolder before calling join(notesFolder,
relativePath) and bail with a user-visible error if missing; also catch and
surface any image-insert errors (from copy_image_to_assets, join,
convertFileSrc, or editor.chain().setImage) to the user via the app's
notification/toast mechanism in addition to console.error so failures are
visible (include the actual error message in the notification for debugging).
In `@src/components/editor/useTableContextMenu.ts`:
- Around line 171-173: The catch in useTableContextMenu.ts currently only logs
errors to console and must surface a user-friendly notification; update the
catch block inside the async handler in useTableContextMenu to both log the
technical error (console.error) and call the app's notification/Toast API (e.g.,
showToast/notify/enqueueSnackbar — whichever notification utility your app uses)
with a concise, user-facing message like "Failed to open table menu" so the user
is informed; keep the original console.error for diagnostics and ensure the
notification is non-blocking and localized if your app supports i18n.
---
Outside diff comments:
In `@src/components/editor/Editor.tsx`:
- Around line 168-177: The useEffect is re-fetching settings on any "notes"
change and only logs failures to console; update the dependency array to
[currentNote?.id, previewMode] so notes mutations don't trigger getSettings, and
replace the console.error handler in the notesService.getSettings().catch(...)
with a user-visible error surface (e.g., call an existing notify/showToast or
set an error state) so failures are visible to users; likewise, locate any
reload callback that re-invokes getSettings (refer to notesService.getSettings,
setSettings and the reload handler in this component) and ensure it
returns/awaits the promise and handles errors by surfacing them via the same
notification mechanism instead of allowing uncaught rejections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18ec33e2-bca2-417d-953a-91c022d62eb5
📒 Files selected for processing (10)
src/components/editor/Editor.tsxsrc/components/editor/EditorTopBar.tsxsrc/components/editor/FormatBar.tsxsrc/components/editor/SearchHighlightExtension.tssrc/components/editor/useCopyExport.tssrc/components/editor/useEditorExtensions.tssrc/components/editor/useEditorSearch.tssrc/components/editor/useNoteSaving.tssrc/components/editor/usePopupManager.tssrc/components/editor/useTableContextMenu.ts
Comment on lines
+125
to
+132
| <button | ||
| onClick={onReload} | ||
| className="h-7 px-2 flex items-center gap-1 text-xs text-text-muted hover:bg-bg-emphasis rounded transition-colors font-medium" | ||
| > | ||
| <RefreshCwIcon className="w-4 h-4 stroke-[1.6]" /> | ||
| <span>Refresh</span> | ||
| </button> | ||
| </Tooltip> |
There was a problem hiding this comment.
Top-bar async actions fail silently for users.
onReload and pin/unpin failures are only logged (or not locally handled), so users get no actionable feedback.
Suggested fix
+import { toast } from "sonner";
...
- <button
- onClick={onReload}
+ <button
+ onClick={async () => {
+ try {
+ await onReload();
+ } catch (error) {
+ console.error("Failed to reload note:", error);
+ toast.error("Failed to refresh note");
+ }
+ }}
className="h-7 px-2 flex items-center gap-1 text-xs text-text-muted hover:bg-bg-emphasis rounded transition-colors font-medium"
>
...
onClick={async () => {
if (!currentNote) return;
try {
if (isPinned) {
await unpinNote(currentNote.id);
} else {
await pinNote(currentNote.id);
}
await onSettingsReload();
} catch (error) {
console.error("Failed to pin/unpin note:", error);
+ toast.error("Failed to update pin status");
}
}}As per coding guidelines, "Implement proper error handling with user-friendly messages for all non-blocking async operations".
Also applies to: 149-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/editor/EditorTopBar.tsx` around lines 125 - 132, The async
handlers in EditorTopBar (notably onReload and the pin/unpin handlers) currently
swallow failures — wrap these async calls in try/catch, show a user-facing error
(e.g., via the app's toast/notification API or a small inline error state) and
ensure UI feedback for in-flight state (disable the button or show a spinner)
and re-enable on completion; specifically update onReload (and the pin/unpin
handlers) to await the async operation, catch errors and call the notification
helper (or set an error state) with a clear message, log the full error for
debugging, and ensure the button state (disabled/loading) is toggled so users
get immediate feedback.
Comment on lines
+95
to
+97
| if ((e.metaKey || e.ctrlKey) && e.shiftKey && e.key === "c") { | ||
| e.preventDefault(); | ||
| setCopyMenuOpen(true); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
KeyboardEvent.key behavior with Shift modifier keyboard shortcuts
💡 Result:
KeyboardEvent.key is the key value after applying the current keyboard layout and active modifiers (including Shift)—i.e., it represents the character/meaning produced, not the physical key. [1], [3]
What this means with Shift
- For printable keys, Shift changes
event.key(e.g., on a US layout:qvsQ,1vs!). The UI Events “key values” spec explicitly calls out"q"or"Q"when Shift is held. [4] - For the Shift key itself,
event.keyis the literal string"Shift". [1] - Whether Shift is pressed is also available as a boolean:
event.shiftKey. [2]
Why this matters for keyboard shortcuts
If you write shortcuts against event.key, Shift can change the value and layouts can change it even more. Example from the spec: on a Dvorak layout, Ctrl + Q may yield event.key === "5", not "q". [4]
Recommended patterns
- Character-based shortcut (wants the typed meaning):
- Use
event.key, and normalize if you want Shift-insensitive matching:- e.g.
event.key.toLowerCase() === "s"(plusctrlKey/metaKeyas needed).
- e.g.
- Use
- Physical-key shortcut (WASD/game controls, layout-independent):
- Use
event.code(physical key position) plus modifier flags (shiftKey,ctrlKey, etc.).codeis designed to be layout-independent. [5], [6]
- Use
Sources: MDN KeyboardEvent.key and shiftKey, W3C UI Events + UI Events Key Values, and UI Events code values / explanations. [1]-[6]
🏁 Script executed:
# Find and examine the useCopyExport.ts file
fd useCopyExport.tsRepository: erictli/scratch
Length of output: 97
🏁 Script executed:
# Check file size first
wc -l src/components/editor/useCopyExport.tsRepository: erictli/scratch
Length of output: 101
🏁 Script executed:
# Read the file around the flagged lines with context
cat -n src/components/editor/useCopyExport.ts | sed -n '85,110p'Repository: erictli/scratch
Length of output: 978
Cmd/Ctrl+Shift+C shortcut is case-sensitive and will not trigger.
When Shift is held, browsers emit event.key === "C" (uppercase), so the comparison e.key === "c" at line 95 will fail and the shortcut won't work.
Suggested fix
- if ((e.metaKey || e.ctrlKey) && e.shiftKey && e.key === "c") {
+ if (
+ (e.metaKey || e.ctrlKey) &&
+ e.shiftKey &&
+ e.key.toLowerCase() === "c"
+ ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ((e.metaKey || e.ctrlKey) && e.shiftKey && e.key === "c") { | |
| e.preventDefault(); | |
| setCopyMenuOpen(true); | |
| if ( | |
| (e.metaKey || e.ctrlKey) && | |
| e.shiftKey && | |
| e.key.toLowerCase() === "c" | |
| ) { | |
| e.preventDefault(); | |
| setCopyMenuOpen(true); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/editor/useCopyExport.ts` around lines 95 - 97, The keyboard
shortcut check is case-sensitive and misses "C" when Shift is held; update the
key comparison used alongside the existing modifier checks (the condition that
currently tests (e.metaKey || e.ctrlKey) && e.shiftKey && e.key === "c") to
compare the key case-insensitively (e.g., normalize e.key with toLowerCase()
before comparing) so the setCopyMenuOpen(true) branch fires for both "c" and
"C".
Comment on lines
+118
to
+123
| handleKeyDown: (_view, event) => { | ||
| if (event.key === "Tab") { | ||
| return false; | ||
| } | ||
| return false; | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Tiptap ProseMirror editorProps handleKeyDown return value true false suppress default
💡 Result:
In Tiptap, editorProps.handleKeyDown is passed straight through to ProseMirror’s EditorProps.handleKeyDown(view, event) → boolean | undefined. (prosemirror.net)
-
Return
true: means “I handled this keydown”. ProseMirror will stop running otherhandleKeyDownprops / keymap handlers after yours. (discuss.prosemirror.net)- Important: unlike most other ProseMirror event handlers, returning
truehere does not automatically callevent.preventDefault(). If you want to suppress the browser’s default behavior, you must callevent.preventDefault()yourself (or intentionally not, if you want the browser default). (prosemirror.net)
- Important: unlike most other ProseMirror event handlers, returning
-
Return
falseorundefined: means “not handled”, so ProseMirror continues to other handlers/keymaps and the browser default may happen as usual. (prosemirror.net)
Citations:
- 1: https://prosemirror.net/docs//ref/?utm_source=openai
- 2: https://discuss.prosemirror.net/t/prevent-keybinding-in-handlekeydown-function/1540?utm_source=openai
- 3: https://prosemirror.net/docs/ref/version/0.15.0.html?utm_source=openai
- 4: https://prosemirror.net/docs//ref/?utm_source=openai
🏁 Script executed:
# First, let's examine the actual file to understand the context better
fd "useEditorExtensions.ts" --type fRepository: erictli/scratch
Length of output: 103
🏁 Script executed:
cat -n src/components/editor/useEditorExtensions.ts | head -150Repository: erictli/scratch
Length of output: 5664
Tab key handling is ineffective—the handler does nothing.
The handleKeyDown prop returns false for both Tab and all other keys, making the Tab-specific check meaningless. In ProseMirror, returning false (or undefined) signals "not handled," allowing the browser default and other handlers to run. If the intent is to suppress Tab's default behavior and handle it within the editor (common for indentation), the handler must call event.preventDefault() and return true.
Suggested fix
handleKeyDown: (_view, event) => {
if (event.key === "Tab") {
+ event.preventDefault();
+ return true;
}
return false;
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/editor/useEditorExtensions.ts` around lines 118 - 123, The
handleKeyDown in useEditorExtensions.ts currently always returns false, so Tab
handling is a no-op; update the handleKeyDown implementation to detect event.key
=== "Tab", call event.preventDefault(), perform the intended Tab behavior (e.g.,
indentation logic or call existing indent function), and then return true to
mark the event handled, while keeping non-Tab keys returning false; locate the
handleKeyDown handler in useEditorExtensions.ts and modify it to preventDefault
and return true for Tab events.
Comment on lines
+144
to
+146
| const notesFolder = await invoke<string>("get_notes_folder"); | ||
| const absolutePath = await join(notesFolder, relativePath); | ||
| const assetUrl = convertFileSrc(absolutePath); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check backend implementation
echo "=== Backend get_notes_folder function ==="
rg -n -C5 'fn get_notes_folder' src-tauri/src/lib.rs
echo -e "\n=== Frontend invoke call ==="
rg -n -C3 'invoke.*get_notes_folder' src/components/editor/useEditorExtensions.ts
echo -e "\n=== Check for other usages of get_notes_folder ==="
rg -n 'get_notes_folder' --type ts --type tsx src/Repository: erictli/scratch
Length of output: 832
get_notes_folder return type mismatch: backend returns Option<String>, frontend expects string.
The backend command returns Option<String> (serialized as string | null), but the frontend invoke generic is typed as <string>. This mismatch causes TypeScript to incorrectly assume notesFolder is always defined, leading to potential runtime errors when passed to join(). Update the type annotation to invoke<string | null>() and add an explicit null check before using the value.
Suggested fix
- const notesFolder = await invoke<string>("get_notes_folder");
+ const notesFolder = await invoke<string | null>("get_notes_folder");
+ if (!notesFolder) {
+ throw new Error("Notes folder not set");
+ }
const absolutePath = await join(notesFolder, relativePath);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const notesFolder = await invoke<string>("get_notes_folder"); | |
| const absolutePath = await join(notesFolder, relativePath); | |
| const assetUrl = convertFileSrc(absolutePath); | |
| const notesFolder = await invoke<string | null>("get_notes_folder"); | |
| if (!notesFolder) { | |
| throw new Error("Notes folder not set"); | |
| } | |
| const absolutePath = await join(notesFolder, relativePath); | |
| const assetUrl = convertFileSrc(absolutePath); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/editor/useEditorExtensions.ts` around lines 144 - 146, The
frontend assumes get_notes_folder returns a non-null string; update the invoke
call to use invoke<string | null>("get_notes_folder") and then explicitly check
notesFolder for null before using join(notesFolder, relativePath); if null,
handle it (e.g., log/error/return) so join and convertFileSrc are never called
with a null path (refer to notesFolder, get_notes_folder, join, and
convertFileSrc).
Comment on lines
+223
to
+231
| if (currentNoteId) { | ||
| setSearchOpen(false); | ||
| setSearchQuery(""); | ||
| setSearchMatches([]); | ||
| setCurrentMatchIndex(0); | ||
| if (editor) { | ||
| updateSearchDecorations([], 0, editor); | ||
| } | ||
| } |
There was a problem hiding this comment.
Search reset is skipped when currentNoteId becomes undefined.
This guard leaves stale search UI/highlights when transitioning to no active note.
Suggested fix
useEffect(() => {
- if (currentNoteId) {
- setSearchOpen(false);
- setSearchQuery("");
- setSearchMatches([]);
- setCurrentMatchIndex(0);
- if (editor) {
- updateSearchDecorations([], 0, editor);
- }
- }
+ setSearchOpen(false);
+ setSearchQuery("");
+ setSearchMatches([]);
+ setCurrentMatchIndex(0);
+ if (editor) {
+ updateSearchDecorations([], 0, editor);
+ }
}, [currentNoteId, editor, updateSearchDecorations]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/editor/useEditorSearch.ts` around lines 223 - 231, The reset
block currently only runs when currentNoteId is truthy, leaving stale search UI
and highlights when currentNoteId becomes undefined; change the condition so the
reset runs when there is no active note (i.e., when currentNoteId is falsy) or
remove the guard entirely so the following calls always execute:
setSearchOpen(false), setSearchQuery(""), setSearchMatches([]),
setCurrentMatchIndex(0), and—if editor exists—updateSearchDecorations([], 0,
editor); update the check around currentNoteId in useEditorSearch.ts to
reference currentNoteId appropriately and ensure updateSearchDecorations is
still called when editor is defined.
Comment on lines
+121
to
+132
| saveTimeoutRef.current = window.setTimeout(async () => { | ||
| if (currentNoteIdRef.current !== savingNoteId || !needsSaveRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| if (editorRef.current) { | ||
| needsSaveRef.current = false; | ||
| const markdown = getMarkdown(editorRef.current); | ||
| await saveImmediately(savingNoteId, markdown); | ||
| } | ||
| }, 500); | ||
| }, [saveImmediately, getMarkdown, currentNote?.id]); |
There was a problem hiding this comment.
Autosave timer is off-spec and timer-save failures are not handled locally.
Line 131 uses 500ms debounce (project standard is 300ms), and Line 129 awaits in a timer callback without a local catch/toast path.
Suggested fix
+import { toast } from "sonner";
...
- saveTimeoutRef.current = window.setTimeout(async () => {
+ saveTimeoutRef.current = window.setTimeout(async () => {
if (currentNoteIdRef.current !== savingNoteId || !needsSaveRef.current) {
return;
}
if (editorRef.current) {
needsSaveRef.current = false;
const markdown = getMarkdown(editorRef.current);
- await saveImmediately(savingNoteId, markdown);
+ try {
+ await saveImmediately(savingNoteId, markdown);
+ } catch (error) {
+ console.error("Failed to auto-save note:", error);
+ toast.error("Auto-save failed");
+ }
}
- }, 500);
+ }, 300);As per coding guidelines, "Implement debouncing for all backend-heavy operations: auto-save (300ms)..." and "Implement proper error handling with user-friendly messages for all non-blocking async operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/editor/useNoteSaving.ts` around lines 121 - 132, The autosave
debounce and error handling need fixing: change the timeout delay from 500 to
300ms (update the window.setTimeout call) and wrap the async save inside the
timer callback in a try/catch around await saveImmediately(savingNoteId,
markdown); on catch set needsSaveRef.current = true to preserve pending changes
and call the app's user-facing toast/error helper (e.g., showSaveErrorToast or
notifyUser) with a concise message; keep the early return checks using
currentNoteIdRef and needsSaveRef and still clear/set saveTimeoutRef as before.
Comment on lines
+145
to
+152
| if (needsSaveRef.current && editorRef.current) { | ||
| needsSaveRef.current = false; | ||
| const manager = editorRef.current.storage.markdown?.manager; | ||
| const markdown = manager | ||
| ? manager.serialize(editorRef.current.getJSON()) | ||
| : editorRef.current.getText(); | ||
| saveNote(markdown); | ||
| } |
There was a problem hiding this comment.
Unmount flush can save to the wrong note because noteId is omitted.
On cleanup, saveNote(markdown) is called without the loaded note id. During note transitions, this can write content to an unintended note target.
Suggested fix
- if (needsSaveRef.current && editorRef.current) {
+ if (needsSaveRef.current && editorRef.current && loadedNoteIdRef.current) {
needsSaveRef.current = false;
- const manager = editorRef.current.storage.markdown?.manager;
- const markdown = manager
- ? manager.serialize(editorRef.current.getJSON())
- : editorRef.current.getText();
- saveNote(markdown);
+ const markdown = getMarkdown(editorRef.current);
+ void saveNote(markdown, loadedNoteIdRef.current).catch((error) => {
+ console.error("Failed to flush pending save on unmount:", error);
+ });
}Based on learnings, "Manage note state in NotesContext with dual context pattern (data and actions separated) and race condition protection during note switches".
Comment on lines
+407
to
+432
| const handleAddImage = useCallback(async () => { | ||
| const editor = editorRef.current; | ||
| if (!editor) return; | ||
| const selected = await openDialog({ | ||
| multiple: false, | ||
| filters: [ | ||
| { | ||
| name: "Images", | ||
| extensions: ["png", "jpg", "jpeg", "gif", "webp", "svg"], | ||
| }, | ||
| ], | ||
| }); | ||
| if (selected) { | ||
| try { | ||
| const relativePath = await invoke<string>("copy_image_to_assets", { | ||
| sourcePath: selected as string, | ||
| }); | ||
| const notesFolder = await invoke<string>("get_notes_folder"); | ||
| const absolutePath = await join(notesFolder, relativePath); | ||
| const assetUrl = convertFileSrc(absolutePath); | ||
| editor.chain().focus().setImage({ src: assetUrl }).run(); | ||
| } catch (error) { | ||
| console.error("Failed to add image:", error); | ||
| } | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify backend return type contract for get_notes_folder
echo "=== Checking Rust backend implementation ==="
rg -nP 'fn\s+get_notes_folder\s*\([^)]*\)\s*->\s*Option<String>' src-tauri/src/lib.rs -C2
echo ""
echo "=== Checking frontend typing of get_notes_folder ==="
# Verify frontend currently assumes non-null string
rg -nP 'invoke<\s*string\s*>\("get_notes_folder"\)' src/components/editor/usePopupManager.ts -C2Repository: erictli/scratch
Length of output: 557
🏁 Script executed:
#!/bin/bash
# Check if toast is imported and available in usePopupManager.ts
echo "=== Checking imports in usePopupManager.ts ==="
head -50 src/components/editor/usePopupManager.ts | grep -i "import\|from"
echo ""
echo "=== Checking for existing toast usage in the file ==="
rg -n 'toast\.' src/components/editor/usePopupManager.tsRepository: erictli/scratch
Length of output: 864
🏁 Script executed:
#!/bin/bash
# Verify the actual get_notes_folder function signature in Rust
echo "=== Full get_notes_folder function signature ==="
rg -A10 'fn get_notes_folder' src-tauri/src/lib.rsRepository: erictli/scratch
Length of output: 415
Fix nullable get_notes_folder type contract and expose image-insert errors to users.
Backend get_notes_folder returns Option<String> (line 734 of src-tauri/src/lib.rs), but frontend currently types it as non-null string on line 424. If null, join(notesFolder, relativePath) will fail at runtime. Additionally, errors are only logged to console; users need feedback when image insertion fails.
💡 Suggested patch
const handleAddImage = useCallback(async () => {
const editor = editorRef.current;
if (!editor) return;
- const selected = await openDialog({
- multiple: false,
- filters: [
- {
- name: "Images",
- extensions: ["png", "jpg", "jpeg", "gif", "webp", "svg"],
- },
- ],
- });
- if (selected) {
- try {
- const relativePath = await invoke<string>("copy_image_to_assets", {
- sourcePath: selected as string,
- });
- const notesFolder = await invoke<string>("get_notes_folder");
- const absolutePath = await join(notesFolder, relativePath);
- const assetUrl = convertFileSrc(absolutePath);
- editor.chain().focus().setImage({ src: assetUrl }).run();
- } catch (error) {
- console.error("Failed to add image:", error);
- }
- }
+ try {
+ const selected = await openDialog({
+ multiple: false,
+ filters: [
+ {
+ name: "Images",
+ extensions: ["png", "jpg", "jpeg", "gif", "webp", "svg"],
+ },
+ ],
+ });
+ if (typeof selected !== "string") return;
+
+ const relativePath = await invoke<string>("copy_image_to_assets", {
+ sourcePath: selected,
+ });
+ const notesFolder = await invoke<string | null>("get_notes_folder");
+ if (!notesFolder) {
+ toast.error("Set a notes folder before inserting images.");
+ return;
+ }
+
+ const absolutePath = await join(notesFolder, relativePath);
+ const assetUrl = convertFileSrc(absolutePath);
+ editor.chain().focus().setImage({ src: assetUrl }).run();
+ } catch (error) {
+ console.error("Failed to add image:", error);
+ toast.error("Couldn't insert image.");
+ }
}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/editor/usePopupManager.ts` around lines 407 - 432, The
handleAddImage flow in usePopupManager (function handleAddImage) assumes
invoke("get_notes_folder") always returns a string but backend can return
null/Option; update the code to treat notesFolder as possibly null/undefined,
validate notesFolder before calling join(notesFolder, relativePath) and bail
with a user-visible error if missing; also catch and surface any image-insert
errors (from copy_image_to_assets, join, convertFileSrc, or
editor.chain().setImage) to the user via the app's notification/toast mechanism
in addition to console.error so failures are visible (include the actual error
message in the notification for debugging).
Comment on lines
+171
to
+173
| } catch (err) { | ||
| console.error("Table context menu error:", err); | ||
| } |
There was a problem hiding this comment.
Table-menu failures are silent to the user.
The catch block only logs. Surface a user-facing error message for this non-blocking async action.
Suggested fix
import { useCallback } from "react";
import { type Editor as TiptapEditor } from "@tiptap/react";
import { Menu, MenuItem, PredefinedMenuItem } from "@tauri-apps/api/menu";
+import { toast } from "sonner";
...
} catch (err) {
console.error("Table context menu error:", err);
+ toast.error("Failed to open table menu");
}As per coding guidelines, "Implement proper error handling with user-friendly messages for all non-blocking async operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/editor/useTableContextMenu.ts` around lines 171 - 173, The
catch in useTableContextMenu.ts currently only logs errors to console and must
surface a user-friendly notification; update the catch block inside the async
handler in useTableContextMenu to both log the technical error (console.error)
and call the app's notification/Toast API (e.g.,
showToast/notify/enqueueSnackbar — whichever notification utility your app uses)
with a concise, user-facing message like "Failed to open table menu" so the user
is informed; keep the original console.error for diagnostics and ensure the
notification is non-blocking and localized if your app supports i18n.
erictli
pushed a commit
that referenced
this pull request
Mar 5, 2026
- Fix Cmd+Shift+C shortcut case sensitivity (e.key.toLowerCase()) - Fix Tab key handler to actually prevent default and return true - Handle nullable get_notes_folder return in image paste and insert - Clear search state when no note is selected (not just on switch) - Reduce autosave debounce from 500ms to 300ms per spec, add error toast - Pass loadedNoteIdRef to unmount flush save to prevent wrong-note writes - Surface image insert errors via toast in usePopupManager - Add toast for table context menu failures - Add error toasts for reload and pin/unpin in EditorTopBar - Remove `notes` from getSettings dependency array, add error toast https://claude.ai/code/session_01WVmueJTJkkSASmYfFdA7q1
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.
Summary
Editor.tsxinto 9 focused modules (hooks + components)Editor.tsxis now a ~530-line orchestrator that composes them, a 77% reductionNew files
FormatBar.tsxEditorTopBar.tsxSearchHighlightExtension.tsuseNoteSaving.tsuseEditorExtensions.tsuseEditorSearch.tsusePopupManager.tsuseCopyExport.tsuseTableContextMenu.tsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes