Add selection formatting and improved tables - #201
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:
📝 WalkthroughWalkthroughThis change adds workspace-aware routing, revision-safe persistence, draft recovery, scoped settings, window sessions, Preferences support, and a Notion-style Tiptap editor with table editing, Markdown metadata, clipboard handling, and Vitest coverage. ChangesWorkspace, persistence, and editor platform
Estimated code review effort: 5 (Critical) | ~180 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant NotesContext
participant RustWorkspaceSession
participant Filesystem
User->>Editor: edit note or switch workspace
Editor->>NotesContext: flush dirty draft
NotesContext->>RustWorkspaceSession: save with expected revision
RustWorkspaceSession->>Filesystem: atomic compare-and-swap write
Filesystem-->>RustWorkspaceSession: saved revision or conflict
RustWorkspaceSession-->>NotesContext: typed result
NotesContext->>RustWorkspaceSession: load workspace or resolve conflict
RustWorkspaceSession-->>Editor: updated note, recovery, or conflict state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 (7)
src/components/preview/PreviewApp.tsx (1)
103-133: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGuard the load effect against out-of-order resolution.
The effect awaits
readFileDirectandgetDraftCheckpoint, then calls setters. IffilePathchanges before the promises settle, the stale result overwrites the new file state, includingrevisionRef.current. A stale revision then drives a save against the wrong base.Add a cancellation flag.
🛡️ Proposed fix
useEffect(() => { + let cancelled = false; filesService .readFileDirect(filePath) .then(async (result) => { const checkpoint = await draftCheckpointService .getDraftCheckpoint(filePath) .catch(() => null); + if (cancelled) return; const recovered =Return
() => { cancelled = true; }from the effect.🤖 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/preview/PreviewApp.tsx` around lines 103 - 133, Update the useEffect loading flow in PreviewApp to use a cancellation flag initialized per effect run and return a cleanup that marks it cancelled. After readFileDirect and getDraftCheckpoint resolve, skip all state updates, revisionRef.current assignment, conflict handling, and toast calls when cancelled, preventing stale file loads from overwriting the current file state.Source: Linters/SAST tools
src-tauri/src/lib.rs (1)
2964-2995: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
create_notestill uses a check-then-write sequence that can overwrite a note.Lines 2968 to 2978 loop until
final_idnames a path that does not exist. Line 2993 then callsfs::write, which truncates an existing file. Two windows bound to the same workspace can both settle on the samefinal_idand the secondfs::writereplaces the first note.Every other write path in this change moved to compare-and-swap. This command did not. Use the create-only persistence path so a concurrent creation returns a conflict instead of replacing content.
🐛 Proposed fix
- fs::write(&file_path, &content) - .await - .map_err(|e| e.to_string())?; + let create_path = file_path.clone(); + let create_content = content.clone(); + let created = tauri::async_runtime::spawn_blocking(move || { + persistence::save_if_revision(&create_path, &create_content, None) + .map_err(|error| error.to_string()) + }) + .await + .map_err(|error| format!("Note create task failed: {error}"))??; + if matches!(created, persistence::SaveResult::Conflict { .. }) { + return Err("A note with that name was created concurrently".to_string()); + }🤖 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-tauri/src/lib.rs` around lines 2964 - 2995, Update create_note’s final persistence step to use an atomic create-only filesystem operation instead of fs::write, so concurrent creation of the same final_id returns a conflict rather than truncating existing content. Preserve the existing uniqueness loop, parent-directory creation, and content generation while ensuring the write fails when the target already exists.src/context/NotesContext.tsx (2)
993-1096: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe
noteConflictdependency drops watcher events.Line 1096 lists
noteConflictas a dependency. Every conflict transition tears down thefile-changelistener and registers a new one. Registration is asynchronous, so any watcher event that arrives in that gap is lost. The listener reads all other mutable state through refs.
noteConflictis read only at Line 1048 to populatesyncState.conflict, andreconcileRemoteNoteinsrc/lib/noteSync.tsdoes not readstate.conflict. Hold the conflict in a ref and register the listener once.🐛 Proposed fix to stabilize the listener
+ const noteConflictRef = useRef<NoteSyncConflict | null>(null); + noteConflictRef.current = noteConflict;- conflict: noteConflict, + conflict: noteConflictRef.current,- }, [noteConflict, refreshNotes]); + }, [refreshNotes]);🤖 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/context/NotesContext.tsx` around lines 993 - 1096, Remove noteConflict from the useEffect dependency array and store the latest conflict in a ref that is updated when noteConflict changes. Use that ref when constructing syncState inside the file-change listener, while keeping refreshNotes as the only necessary dependency so the listener remains registered without gaps.
488-498: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the side effects out of the
setSelectedNoteIdupdater.The updater at Line 489 writes
selectedNoteIdRef, writescurrentNoteRef, and callssetCurrentNoteandsetNoteConflict. React can invoke an updater more than once, so these effects can repeat or observe inconsistent state.selectedNoteIdRef.currentalready holds the selected id synchronously, so the branch does not need an updater.🐛 Proposed fix to keep the updater pure
- // Only clear selection if we're deleting the currently selected note - setSelectedNoteId((prevId) => { - if (prevId === id) { - selectedNoteIdRef.current = null; - currentNoteRef.current = null; - setCurrentNote(null); - setNoteConflict(null); - return null; - } - return prevId; - }); + // Only clear selection if we're deleting the currently selected note + if (selectedNoteIdRef.current === id) { + selectedNoteIdRef.current = null; + currentNoteRef.current = null; + setSelectedNoteId(null); + setCurrentNote(null); + setNoteConflict(null); + }The same pattern exists at Lines 620-626, 658-669, 692-702, and 733-744. Those ranges are outside this change, so fix them separately if you agree.
🤖 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/context/NotesContext.tsx` around lines 488 - 498, In the deletion flow around the selected-note handling, replace the side-effectful setSelectedNoteId updater with a direct selectedNoteIdRef.current comparison, then perform the ref updates and setCurrentNote/setNoteConflict calls outside the updater before clearing the selected ID. Keep the updater pure, and apply the same pattern to the corresponding handlers using this pattern elsewhere in NotesContext.Source: Linters/SAST tools
src/services/notes.ts (1)
102-119: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDelete the placeholder note when the duplicate save conflicts.
duplicateNotecallscreateNotefirst, thensaveNote. IfsaveNotereturnsstatus: "conflict", the function throws and the empty note created on line 107 stays on disk. The user then sees an error plus a stray untitled note.🛡️ Proposed fix
if (result.status === "conflict") { + await deleteNote(newNote.id).catch(() => undefined); throw new Error("The duplicated note changed before its content was saved"); }🤖 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/services/notes.ts` around lines 102 - 119, Update duplicateNote to delete the newly created placeholder note via the existing note-deletion operation when saveNote returns a conflict, before throwing the error. Keep the normal successful return path unchanged and ensure cleanup targets newNote.id.src/context/ThemeContext.tsx (2)
466-492: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle a failed reset.
resetEditorFontSettingsawaitsupdateGlobalSettingswithout atry/catch.EditorSettingsSection.tsxline 354 binds it directly toonClick, so the returned promise is never handled. If the write fails, the UI shows reset values, the backend keeps the old values, and the user sees no message. Every other setter in this file wraps the call intry/catch.🛡️ Proposed fix
- await updateGlobalSettings({ - editorFont: defaultEditorFontSettings, - ... - }); + try { + await updateGlobalSettings({ + editorFont: defaultEditorFontSettings, + textDirection: "auto", + editorWidth: "normal", + interfaceZoom: 1.0, + customEditorWidthPx: null, + editorWidthResizeEnabled: null, + editorToolbarVisible: null, + titleBarModifiedDateVisible: null, + titleBarFilenameVisible: null, + sidebarWidthPx: null, + customColorsLight: null, + customColorsDark: null, + }); + } catch (error) { + console.error("Failed to reset appearance settings:", error); + await loadSettingsFromBackend(); + }Add a user-facing message on the failure path as well.
As per coding guidelines: "Implement error handling with user-friendly messages".
🤖 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/context/ThemeContext.tsx` around lines 466 - 492, Update resetEditorFontSettings to wrap updateGlobalSettings in try/catch, preserving the existing state reset while handling persistence failures. On catch, display a user-facing error through the file’s established notification/message mechanism, matching the pattern used by other setters in ThemeContext.Source: Coding guidelines
714-751: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftMemoize the provider value and split state from actions.
The provider passes an inline object literal as the context value. Every
ThemeProviderrender creates a new object, so everyuseThemeconsumer re-renders. This change adds eight more members to that single value, which widens the blast radius: a title-bar toggle now re-renders each consumer that only readstheme.Wrap the value in
useMemo, or split the context into a data context and an actions context as the project already does forNotesContext.As per coding guidelines: "Use
NotesContextwith dual context pattern (data/actions separated) for performance optimization" and "UseuseCallbackanduseMemofor performance-critical paths".🤖 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/context/ThemeContext.tsx` around lines 714 - 751, Memoize the context value object used by the ThemeContext.Provider with useMemo, including every referenced state value and action in its dependency list so consumers only re-render when exposed data or callbacks change. Prefer the existing dual data/actions context pattern used by NotesContext if ThemeContext already supports that structure, while preserving the current provider API and all members listed in the value.Source: Coding guidelines
🟡 Minor comments (16)
src/App.css-1470-1479 (1)
1470-1479: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the deprecated
clipproperty in the visually hidden announcer.Stylelint reports
clipas deprecated at Line 1476. Useclip-path: inset(50%), which is the current visually hidden pattern and keeps the announcement text available to screen readers.🎨 Proposed fix
.notion-table-announcement { position: fixed; width: 1px; height: 1px; padding: 0; overflow: hidden; - clip: rect(0 0 0 0); + clip-path: inset(50%); white-space: nowrap; border: 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/App.css` around lines 1470 - 1479, Update the .notion-table-announcement visually hidden style by replacing the deprecated clip declaration with clip-path: inset(50%), while preserving the existing accessibility-focused hiding behavior and all other declarations.Source: Linters/SAST tools
src/App.css-1364-1376 (1)
1364-1376: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLowercase the
currentcolorkeyword.Stylelint reports
value-keyword-caseerrors at Line 1368 and Line 1375. CSS-wide keywords must be lowercase for this rule.🎨 Proposed fix
.notion-table-edge-resize::before { content: ""; width: 12px; height: 4px; - border-block: 1px solid currentColor; + border-block: 1px solid currentcolor; } .notion-table-edge-resize.is-column::before { width: 4px; height: 12px; border-block: 0; - border-inline: 1px solid currentColor; + border-inline: 1px solid currentcolor; }🤖 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/App.css` around lines 1364 - 1376, Update the currentColor values in the ::before rules for .notion-table-edge-resize and .notion-table-edge-resize.is-column to the lowercase currentcolor form required by Stylelint.Source: Linters/SAST tools
src/components/editor/notion/tableView.ts-44-54 (1)
44-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
restore()leaves thecolelements that the preview appended.Lines 44-46 append
colelements when the colgroup holds fewer columns thanbaselineWidths.restore()restores attributes only, so the appended elements stay in the colgroup after the drag ends. The table then renders extra columns until the nextupdateScratchTableColumnsrun. Record the appended elements and remove them inrestore().🐛 Proposed fix
+ const appendedColumns: HTMLTableColElement[] = []; while (colgroup.children.length < normalizedBaseline.length) { - colgroup.appendChild(colgroup.ownerDocument.createElement("col")); + const column = colgroup.ownerDocument.createElement("col"); + colgroup.appendChild(column); + appendedColumns.push(column); } @@ restore() { if (restored) return; restored = true; restoreAttribute(table, "style", tableStyle); restoreAttribute(table, "data-fit-to-width", fitToWidth); restoreAttribute(table, "data-column-resizing", resizing); columns.forEach((column, index) => restoreAttribute(column, "style", columnStyles[index]), ); + appendedColumns.forEach((column) => column.remove()); },Also applies to: 81-91
🤖 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/editor/notion/tableView.ts` around lines 44 - 54, Update the column setup logic around normalizedBaseline and restore() to track any col elements appended to colgroup, then remove those exact elements during restore() after restoring the saved attributes. Preserve existing handling for original columns and ensure no temporary columns remain after resizing ends.src/components/editor/notion/tableRowResize.ts-29-31 (1)
29-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
toLowerCase()for tag names.
toLocaleLowerCase()follows the host locale. In Turkish and Azeri locales,"I"maps to"ı", so an ancestor such asLIproduces the selector segmentlı:nth-child(n). The generated rule then matches nothing, and the row-resize preview silently stops working. CSS selectors need ASCII lowercasing.🐛 Proposed fix
segments.unshift( - `${current.tagName.toLocaleLowerCase()}:nth-child(${siblingIndex})`, + `${current.tagName.toLowerCase()}:nth-child(${siblingIndex})`, );🤖 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/editor/notion/tableRowResize.ts` around lines 29 - 31, Update the selector segment construction in the table-row resize logic to call toLowerCase() instead of toLocaleLowerCase() on current.tagName, ensuring CSS tag names use locale-independent ASCII lowercasing while preserving the existing nth-child formatting.src/components/editor/editorHistory.ts-6-12 (1)
6-12: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winParse loaded HTML in an inert document.
containeris created from the livedocument, so resource-loading attributes such as<img src=... onerror=...>can execute during theinnerHTMLassignment, before ProseMirror strips unknown nodes. Use an inert document created bydocument.implementation.createHTMLDocument(). The parse result stays identical, and no script or resource loads run.🔒 Proposed fix to parse in an inert document
function normalizeLoadedContent(editor: Editor, content: Content): Content { if (typeof content === "string") { - const container = document.createElement("div"); + const inertDocument = document.implementation.createHTMLDocument(""); + const container = inertDocument.createElement("div"); container.innerHTML = content; const parsed = ProseMirrorDOMParser.fromSchema(editor.schema).parse(container); return normalizeNestedTablesInJson(parsed.toJSON()); }🤖 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/editor/editorHistory.ts` around lines 6 - 12, Update normalizeLoadedContent to create the HTML parsing container from document.implementation.createHTMLDocument() instead of the live document, then assign innerHTML and parse it as before so the normalized ProseMirror result remains unchanged while scripts and resource loads cannot execute.Source: Linters/SAST tools
src-tauri/src/lib.rs-3493-3513 (1)
3493-3513: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSilent fallback to default settings hides a routing failure.
Line 3503 maps a failed
workspace_for_windowtoWorkspaceSettings::default(). The window then reports Git as disabled and reports no pinned notes, without any signal that the workspace could not be resolved. A user can interpret that as lost settings.Log the resolution failure so the cause is visible in diagnostics.
🤖 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-tauri/src/lib.rs` around lines 3493 - 3513, Update get_settings to log when workspace_for_window(window.label()) fails instead of silently converting the failure to default workspace settings. Preserve the existing default fallback and merge behavior, but emit a diagnostic containing the window label and resolution failure before returning the merged settings.src-tauri/src/lib.rs-3002-3017 (1)
3002-3017: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
create_notedoes not emit afile-changeevent.
save_note,delete_note, andmove_noteall callemit_workspace_file_changeso peer windows bound to the same workspace update.create_notereturns the newNotewithout emitting anything.Two windows on one workspace therefore disagree about the note list until the file watcher fires or the user triggers a relist. Emit a
"created"semantic event here for consistency with the new event contract.🤖 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-tauri/src/lib.rs` around lines 3002 - 3017, The create_note flow must notify peer windows after successfully creating and indexing the note. Before returning the new Note, call emit_workspace_file_change with the workspace, created note path/identity, and the "created" semantic event, matching the event contract used by save_note, delete_note, and move_note.src-tauri/src/lib.rs-209-232 (1)
209-232: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCarry the selected note only when the workspace matches.
fallback_main_sessionclones the storedmainrecord at line 225 and then overrides onlyworkspaceat line 230.selected_note_idfrom the previous, unavailable workspace stays in the returned session.The main window then tries to select a note id that belongs to a different workspace. The result is either a missing-note error or an unrelated note with the same relative id.
Clear
selected_note_idwhen the workspace changes.🐛 Proposed fix
let mut fallback = config .records .get("main") .cloned() .unwrap_or_else(|| WindowSession::for_workspace(workspace.clone())); + if fallback.workspace != workspace { + fallback.selected_note_id = None; + } fallback.workspace = workspace; Some(fallback)🤖 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-tauri/src/lib.rs` around lines 209 - 232, Update fallback_main_session so selected_note_id is preserved only when the cloned main record already belongs to the selected workspace; otherwise clear it before returning the fallback session. Keep the existing workspace assignment and default WindowSession behavior unchanged.src-tauri/src/lib.rs-3643-3649 (1)
3643-3649: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHold the settings lock across the save.
Line 3644 takes a write lock, mutates
git_enabled, and drops the lock at line 3646. Line 3648 then takes a read lock and saves. Another command can change the same settings between the two blocks, and this call persists the newer in-memory state under the assumption that it wrote its own.
update_workspace_settingsat line 3576 keeps one lock across the save. Match that pattern.🛡️ Proposed fix
- { - let mut settings = workspace.settings().write().expect("settings write lock"); - settings.git_enabled = enabled; - } - - let settings = workspace.settings().read().expect("settings read lock"); - save_settings(&folder, &settings).map_err(|e| e.to_string())?; + let mut settings = workspace.settings().write().expect("settings write lock"); + settings.git_enabled = enabled; + save_settings(&folder, &settings).map_err(|e| e.to_string())?;🤖 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-tauri/src/lib.rs` around lines 3643 - 3649, Update the settings mutation and persistence flow in the affected command to retain the write lock from the `git_enabled` assignment through `save_settings`, rather than dropping it and reacquiring a read lock. Match the lock scope used by `update_workspace_settings`, ensuring this call saves the state it just modified.src/lib/windowShortcutCallsites.test.ts-22-31 (1)
22-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the slice indices so the test cannot pass vacuously.
Line 24 and line 25 use
indexOf, which returns-1when the marker text is absent. IfPreferencesAppis renamed,preferencesStartbecomes-1,source.slice(-1, preferencesEnd)returns an almost empty string, and the assertions at lines 29 and 30 no longer test the intended code. Line 30 in particular passes for any missing region.Assert both indices before slicing.
💚 Proposed fix
const preferencesStart = source.indexOf("function PreferencesApp()"); const preferencesEnd = source.indexOf("function App()", preferencesStart); + expect(preferencesStart).toBeGreaterThanOrEqual(0); + expect(preferencesEnd).toBeGreaterThan(preferencesStart); const preferencesSource = source.slice(preferencesStart, preferencesEnd);🤖 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/windowShortcutCallsites.test.ts` around lines 22 - 31, Guard the marker lookups in the “keeps Back only for in-window Settings navigation” test by asserting that both `preferencesStart` and `preferencesEnd` are found before calling `source.slice`. Keep the existing source-content assertions unchanged so a renamed or missing `PreferencesApp`/`App` boundary cannot make the test pass vacuously.src/App.tsx-182-198 (1)
182-198: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cmd+,closes Preferences instead of keeping it open.
useWindowShortcutsmaps the preferences action totoggleSettings.toggleSettingstoggles the view. If the user is already in the settings view and pressesCmd+,, the app returns to the notes view. The globalkeydownhandler at Line 286 blocks other shortcuts in the settings view, but this hook is independent, so the toggle still runs. Pass an open-only callback.Also consider a user-friendly message at Line 187 instead of interpolating the raw
errorvalue.🐛 Proposed fix
+ const openSettings = useCallback(async () => { + if (view === "settings") return; + try { + await flushCurrentDraft(); + } catch (error) { + toast.error("Settings not opened because the draft could not be saved."); + console.error("Failed to flush draft before opening settings:", error); + return; + } + setView("settings"); + }, [flushCurrentDraft, view]); + - useWindowShortcuts({ onOpenPreferences: toggleSettings }); + useWindowShortcuts({ onOpenPreferences: openSettings });As per coding guidelines: "Implement error handling with user-friendly messages".
🤖 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/App.tsx` around lines 182 - 198, Update the Preferences shortcut wiring in App.tsx so useWindowShortcuts receives an open-only callback rather than toggleSettings, preventing Cmd+, from closing an already open settings view. Keep toggleSettings for user-initiated view toggling, and replace the raw error interpolation in its catch block with a user-friendly message while preserving the early return.Source: Coding guidelines
src/lib/windowClose.ts-17-25 (1)
17-25: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClosing continues when recovery returns no path after a save failure.
If
flushDraftrejects andpersistRecoveryresolves withundefined, the function still callscloseWindow. The unsaved content is then lost without any signal to the caller other thansaveError. Callers insrc/App.tsx(Lines 123-135) only inspectrecoveredTo, so no warning appears.Consider rejecting when a save error has no recovery target, or make the caller report
saveErrorwhenrecoveredTois absent.♻️ Proposed change
} catch (saveError) { const recoveredTo = await dependencies.persistRecovery(); + if (!recoveredTo) throw saveError; result = { recoveredTo, saveError }; }🤖 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/windowClose.ts` around lines 17 - 25, Update the error path in the window-close flow around flushDraft and persistRecovery so a failed save with no recoveredTo target does not silently close the window; either reject before closeWindow or propagate a result that App.tsx reports through saveError when recoveredTo is absent, while preserving normal recovery and successful-close behavior.src/components/settings/SettingsPage.test.tsx-68-72 (1)
68-72: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe drag-region count assertion depends on the host platform.
SettingsPagerenders the twodata-tauri-drag-regiondivs only when!isWindows. IfisWindowsresolves to true in the test environment, this expectation of 2 becomes 0 and the test fails on a Windows machine. Mock or stub the platform module so the assertion is deterministic, or assert the count against the sameisWindowsvalue the component uses.🤖 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/settings/SettingsPage.test.tsx` around lines 68 - 72, Update the SettingsPage test’s data-tauri-drag-region assertion to use a deterministic platform value by mocking or stubbing the platform module consumed by SettingsPage, or derive the expected count from that same isWindows value. Preserve the existing non-Windows expectation of two regions.src/context/NotesContext.tsx-327-334 (1)
327-334: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the internal error text with a user-facing message.
Line 333 throws
Missing base revision for ${savingNoteId}. Line 391 puts that text intoerror, which the UI shows. Use a message the user can act on, and keep the identifier in aconsolelog for diagnostics.♻️ Proposed change
if (!expectedRevision) { - throw new Error(`Missing base revision for ${savingNoteId}`); + console.error("Missing base revision for note", savingNoteId); + throw new Error( + "Could not save this note because its version is unknown. Reload the note and try again.", + ); }As per coding guidelines: "Implement error handling with user-friendly messages".
🤖 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/context/NotesContext.tsx` around lines 327 - 334, Update the save flow in NotesContext around the expectedRevision validation to throw a user-facing, actionable message instead of exposing the internal “Missing base revision” text through the UI error state. Log the detailed missing revision message, including savingNoteId, via console for diagnostics before throwing the user-friendly error.Source: Coding guidelines
src/components/layout/Sidebar.tsx-98-110 (1)
98-110: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not label every switch failure as a cancellation.
The catch block reports "Workspace switch cancelled" for any rejection.
switchWorkspacealso rejects when the folder is unreadable, missing, or the backend fails. The user then reads a message that contradicts the real cause.🐛 Proposed fix
} catch (error) { console.error("Failed to switch workspace:", error); - toast.error("Workspace switch cancelled"); + toast.error("Failed to switch folder"); }If the backend signals a user cancellation with a distinct error value, branch on that value and keep the cancellation wording only for that case.
As per coding guidelines: "Implement error handling with user-friendly messages".
🤖 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/layout/Sidebar.tsx` around lines 98 - 110, Update the catch block in handleSwitchWorkspace to distinguish the backend’s specific user-cancellation error from other switchWorkspace failures. Keep “Workspace switch cancelled” only for that cancellation value, and show a user-friendly failure message for unreadable, missing, or backend-error cases while preserving error logging.Source: Coding guidelines
src/lib/useWindowShortcuts.ts-25-28 (1)
25-28: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReport a failed preferences launch to the user.
onOpenPreferencesresolves toopenPreferencesWindow, which invokes the Rust commandopen_preferences_window. If that command rejects,voiddiscards the rejection and the user sees no window and no message.🛡️ Proposed fix
if (action === "preferences") { - void openPreferencesRef.current(); + void Promise.resolve(openPreferencesRef.current()).catch((error) => { + console.error("Failed to open preferences:", error); + toast.error("Failed to open Preferences"); + }); return; }As per coding guidelines: "Implement error handling with user-friendly messages".
🤖 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/useWindowShortcuts.ts` around lines 25 - 28, Update the preferences branch in the window shortcut handler so rejected promises from openPreferencesRef.current() are caught instead of discarded. Report the failure to the user with the established user-friendly error notification mechanism, while preserving the existing successful launch and early-return behavior.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b026ec6-48b3-4335-931c-f84e9a5903c8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (117)
package.jsonsrc-tauri/capabilities/default.jsonsrc-tauri/src/draft_checkpoint.rssrc-tauri/src/lib.rssrc-tauri/src/note_persistence_tests.rssrc-tauri/src/persistence.rssrc-tauri/src/watcher_debounce.rssrc/App.csssrc/App.tsxsrc/components/editor/Editor.tsxsrc/components/editor/EditorWidthHandle.test.tsxsrc/components/editor/EditorWidthHandle.tsxsrc/components/editor/editorHistory.test.tssrc/components/editor/editorHistory.tssrc/components/editor/notion/NotionMenus.test.tsxsrc/components/editor/notion/NotionMenus.tsxsrc/components/editor/notion/TableControls.test.tsxsrc/components/editor/notion/TableControls.tsxsrc/components/editor/notion/interfaceGeometry.test.tssrc/components/editor/notion/interfaceGeometry.tssrc/components/editor/notion/markdownDocument.test.tssrc/components/editor/notion/markdownDocument.tssrc/components/editor/notion/markdownMarks.test.tssrc/components/editor/notion/markdownMarks.tssrc/components/editor/notion/selectionDecoration.test.tssrc/components/editor/notion/selectionDecoration.tssrc/components/editor/notion/tableAutoScroll.test.tssrc/components/editor/notion/tableAutoScroll.tssrc/components/editor/notion/tableClipboard.test.tssrc/components/editor/notion/tableClipboard.tssrc/components/editor/notion/tableEdgeDrag.test.tssrc/components/editor/notion/tableEdgeDrag.tssrc/components/editor/notion/tableExtensions.test.tssrc/components/editor/notion/tableExtensions.tssrc/components/editor/notion/tableIntegrity.test.tssrc/components/editor/notion/tableIntegrity.tssrc/components/editor/notion/tableMetadata.tssrc/components/editor/notion/tableNonRegression.test.tssrc/components/editor/notion/tablePerformance.test.tssrc/components/editor/notion/tablePointerDrag.test.tssrc/components/editor/notion/tablePointerDrag.tssrc/components/editor/notion/tableProximity.test.tssrc/components/editor/notion/tableProximity.tssrc/components/editor/notion/tableRowResize.test.tssrc/components/editor/notion/tableRowResize.tssrc/components/editor/notion/tableTransactions.test.tssrc/components/editor/notion/tableTransactions.tssrc/components/editor/notion/tableView.test.tssrc/components/editor/notion/tableView.tssrc/components/layout/Sidebar.tsxsrc/components/layout/SidebarControls.test.tsxsrc/components/layout/SidebarControls.tsxsrc/components/layout/SidebarFolderSection.test.tsxsrc/components/layout/SidebarFolderSection.tsxsrc/components/layout/WorkspaceMenu.test.tsxsrc/components/layout/WorkspaceMenu.tsxsrc/components/notes/FolderTreeView.test.tsxsrc/components/notes/FolderTreeView.tsxsrc/components/notes/NoteList.tsxsrc/components/preview/PreviewApp.tsxsrc/components/settings/EditorSettingsSection.test.tsxsrc/components/settings/EditorSettingsSection.tsxsrc/components/settings/SettingsPage.test.tsxsrc/components/settings/SettingsPage.tsxsrc/context/GitContext.tsxsrc/context/NotesContext.tsxsrc/context/ThemeContext.tsxsrc/lib/conflictResolution.test.tssrc/lib/conflictResolution.tssrc/lib/documentMutationSafety.test.tssrc/lib/documentMutationSafety.tssrc/lib/draftCheckpoint.test.tssrc/lib/draftCheckpoint.tssrc/lib/editorToolbar.test.tssrc/lib/editorToolbar.tssrc/lib/editorWidthResize.test.tssrc/lib/editorWidthResize.tssrc/lib/folderTree.test.tssrc/lib/folderTree.tssrc/lib/noteSync.test.tssrc/lib/noteSync.tssrc/lib/serializedWriter.test.tssrc/lib/serializedWriter.tssrc/lib/settingsScope.test.tssrc/lib/settingsScope.tssrc/lib/standaloneRecreation.test.tssrc/lib/standaloneRecreation.tssrc/lib/standaloneReload.test.tssrc/lib/standaloneReload.tssrc/lib/titleBarNoteInfo.test.tssrc/lib/titleBarNoteInfo.tssrc/lib/useWindowSessionPersistence.tssrc/lib/useWindowShortcuts.tssrc/lib/windowClose.test.tssrc/lib/windowClose.tssrc/lib/windowCloseCallsites.test.tssrc/lib/windowSession.test.tssrc/lib/windowSession.tssrc/lib/windowShortcutCallsites.test.tssrc/lib/windowShortcuts.test.tssrc/lib/windowShortcuts.tssrc/lib/workspace.test.tssrc/lib/workspace.tssrc/lib/workspaceSwitch.test.tssrc/lib/workspaceSwitch.tssrc/services/draftCheckpoint.test.tssrc/services/draftCheckpoint.tssrc/services/files.test.tssrc/services/files.tssrc/services/notes.test.tssrc/services/notes.tssrc/services/windowLifecycle.test.tssrc/services/windowLifecycle.tssrc/services/windowSession.test.tssrc/services/windowSession.tssrc/types/note.tsvitest.config.ts
7ce1747 to
516b698
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/context/ThemeContext.tsx (2)
468-499: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
resetEditorFontSettingshas acatchwith notry. The file does not parse.Line 480 starts
await updateGlobalSettings({...})at statement level, and line 494 opens} catch (error) {. Notry {block is opened, so the module fails to compile. Biome reports parse errors on lines 494 to 499.Add the missing
try {before theawait.🐛 Proposed fix
setCustomColorsLightState({}); setCustomColorsDarkState({}); - await updateGlobalSettings({ + try { + await updateGlobalSettings({ editorFont: defaultEditorFontSettings, textDirection: "auto", editorWidth: "normal", interfaceZoom: 1.0, customEditorWidthPx: null, editorWidthResizeEnabled: null, editorToolbarVisible: null, titleBarModifiedDateVisible: null, titleBarFilenameVisible: null, sidebarWidthPx: null, customColorsLight: null, customColorsDark: null, }); } catch (error) {🤖 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/context/ThemeContext.tsx` around lines 468 - 499, Add the missing try block in resetEditorFontSettings before the await updateGlobalSettings call, enclosing that operation and its closing logic so the existing catch block is syntactically paired and the module parses successfully.Source: Linters/SAST tools
716-721: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPlace
useMemobefore the initialization guard.When
isInitializedis false,ThemeProviderreturnsnullbeforeuseMemo. Once initialization completes, React sees an additional hook and throws during the render. Moveconst contextValue = useMemo<ThemeContextType>(...)aboveif (!isInitialized) return null;, then return immediately after that guard.🤖 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/context/ThemeContext.tsx` around lines 716 - 721, Move the contextValue useMemo hook in ThemeProvider before the isInitialized guard so it executes on every render. Keep the null return immediately after the guard, preserving the existing initialized rendering behavior while maintaining consistent hook order.
♻️ Duplicate comments (4)
src-tauri/src/lib.rs (4)
429-444: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
File > Open Folder…still emits the new-window event.
emit_native_open_folder_requestemitsopen-folder-in-new-window, the same event asemit_native_new_window_request. The listener cannot distinguish the two menu actions. Emit a distinct event such asopen-folder.🤖 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-tauri/src/lib.rs` around lines 429 - 444, Update emit_native_open_folder_request to emit a distinct "open-folder" event instead of "open-folder-in-new-window", while leaving emit_native_new_window_request unchanged.
446-457: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
workspace_path_keystill derives persisted identifiers fromDefaultHasher.
get_workspace_search_index_path(Line 1800) writes this key into an on-disk directory name, andworkspace_window_labelembeds it in persistedrecordskeys.DefaultHasheroutput is not stable across Rust releases, so a toolchain upgrade orphans every search index and every window-session record. Use a stable digest such ashex_sha256.🤖 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-tauri/src/lib.rs` around lines 446 - 457, Update workspace_path_key to derive its persisted identifier with the stable hex_sha256 digest instead of DefaultHasher, preserving the existing workspace- prefix and path-based input. Ensure workspace_window_label continues using workspace_path_key so both search-index directories and session record keys receive stable identifiers.
2073-2075: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftForce-destroying editor windows still discards unsaved drafts.
window.destroy()bypasses the close request, so the WebView never flushes its draft and never writes a recovery snapshot.close_window_after_saveat Line 2091 exists to sequence that flush. Request a graceful close, or write a recovery snapshot for each window before destroying it.🤖 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-tauri/src/lib.rs` around lines 2073 - 2075, Update the window cleanup loop to avoid calling window.destroy() directly, which discards unsaved drafts; request a graceful close that lets each editor flush and write its recovery snapshot, reusing the existing close_window_after_save flow where applicable. Ensure every window is processed before force-destruction is considered.
3954-3959: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winImport still notifies the hardcoded
mainwindow.Line 3871 resolves the workspace from
window.label(), but Lines 3955 to 3959 emitselect-noteto"main"and focus"main". When a workspace window invokes the import, the wrong window receives the selection. Targetwindow.label().🤖 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-tauri/src/lib.rs` around lines 3954 - 3959, Update the import notification and focus flow near the existing metadata selection logic to target the invoking window’s label instead of the hardcoded "main" label. Use window.label() for both app.emit_to and the subsequent webview-window lookup, preserving the existing show and set_focus behavior.
🧹 Nitpick comments (9)
src/components/editor/notion/selectionDecoration.test.ts (2)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve
App.cssrelative to this test file.
process.cwd()depends on where Vitest starts. If the process starts outside the repository root,readFileSyncthrows at module load and the whole suite fails before any test runs. Resolve the path fromimport.meta.urlinstead.♻️ Proposed fix
-import { readFileSync } from "node:fs"; -import { resolve } from "node:path"; +import { readFileSync } from "node:fs"; +import { dirname, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; @@ -const appStyles = readFileSync( - resolve(process.cwd(), "src/App.css"), - "utf8", -); +const appStyles = readFileSync( + resolve(dirname(fileURLToPath(import.meta.url)), "../../../App.css"), + "utf8", +);🤖 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/editor/notion/selectionDecoration.test.ts` around lines 61 - 64, Update the appStyles file lookup in selectionDecoration.test.ts to resolve App.css relative to the test module using import.meta.url, rather than process.cwd(), while preserving the existing UTF-8 read.
195-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test names promise extension behavior that the assertions do not check.
In both tests the second dispatch asserts only
defaultPrevented === falseand then repeats the previous anchor and head. The inline comment states that happy-dom applies no keyboard-selection default, so the "extend" half of each name is not verified. Rename the tests to describe the verified behavior, which is that the extension leaves the event unhandled, or drive the extension with an explicitsyncNativeDOMHeadcall as the first half does.Also applies to: 214-219, 239-250
🤖 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/editor/notion/selectionDecoration.test.ts` at line 195, Update the affected selection tests around the Shift+ArrowLeft and Shift+ArrowRight cases so their names match the behavior actually asserted: the extension event remains unhandled. Alternatively, explicitly invoke syncNativeDOMHead for the second dispatch and assert the resulting extension. Apply the same correction to both test cases, not just the first.src/lib/draftCheckpoint.ts (1)
176-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
Notetype import to the top of the file.
reconcileDraftCheckpointat Line 151 usesNote, but the import appears on the last line. TypeScript hoists the import, so the code compiles. Place the import with the other module-level declarations so the dependency is visible.♻️ Proposed change
-import type { Note } from "../types/note";Add at the top of the file:
import type { Note } from "../types/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/lib/draftCheckpoint.ts` at line 176, Move the type-only Note import to the top of the file alongside the other module-level imports, while keeping reconcileDraftCheckpoint and its Note usage unchanged.src-tauri/src/lib.rs (1)
1257-1274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the redundant second session lookup.
For a non-fallback label, Line 1261 already resolved
workspace_session(window_label)and returnedNone. Line 1271 repeats the same lookup and acquires the same two locks again. Return the error directly instead.♻️ Proposed simplification
if uses_default_workspace_fallback(window_label) { return Ok(WorkspaceRuntime::Main(self)); } - self.workspace_session(window_label) - .map(WorkspaceRuntime::Session) - .ok_or_else(|| format!("Workspace session not found for window: {}", window_label)) + Err(format!( + "Workspace session not found for window: {}", + window_label + ))🤖 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-tauri/src/lib.rs` around lines 1257 - 1274, In workspace_for_window, remove the repeated workspace_session(window_label) lookup in the final error path; for non-fallback labels, return the existing “Workspace session not found for window” error directly after the initial lookup returns None, while preserving the fallback-to-main behavior.src/lib/noteSync.ts (1)
59-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the three return states.
The function returns three distinct signals:
undefinedfor an unrelated event,nullfor a deletion, and a string for the current ID. The distinction betweennullandundefinedcontrols whether the caller clears the open note or ignores the event. Add a doc comment so a later change does not collapse the two.♻️ Proposed comment
+/** + * Resolves the note ID that a file-change event maps the open note to. + * Returns `undefined` when the event does not affect `currentId`, + * `null` when the note was deleted, and the new ID otherwise. + */ export function resolveRemoteNoteId(🤖 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/noteSync.ts` around lines 59 - 71, Add a doc comment directly above resolveRemoteNoteId documenting its three return states: undefined for unrelated events, null when the note was deleted, and a string containing the current note ID otherwise. Explicitly preserve the distinction between null and undefined because callers use it to clear or ignore the open note.src/components/editor/Editor.tsx (1)
902-927: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the editor auto-save debounce with the documented 300 ms interval.
The debounce timer is 500 ms. The coding guidelines require 300 ms for auto-save.
handleSourceChangealready uses 300 ms, so the two paths also disagree.As per coding guidelines: "Debounce user-triggered operations: auto-save 300ms".
♻️ Proposed change
- }, 500); + }, 300);🤖 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/editor/Editor.tsx` around lines 902 - 927, Update the auto-save debounce timer in the save callback around saveImmediately from 500 ms to 300 ms, matching handleSourceChange and the documented auto-save interval. Preserve the existing save validation and error-handling behavior.Source: Coding guidelines
src/context/NotesContext.tsx (1)
489-498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the ref writes and nested state updates out of the
setSelectedNoteIdupdater.React can call a state updater more than once. This updater writes
selectedNoteIdRef, writescurrentNoteRef, and callssetCurrentNoteandsetNoteConflict. Keep the updater pure and perform the side effects in the callback body, whereselectedNoteIdRef.currentalready holds the value you need.♻️ Proposed change
- setSelectedNoteId((prevId) => { - if (prevId === id) { - selectedNoteIdRef.current = null; - currentNoteRef.current = null; - setCurrentNote(null); - setNoteConflict(null); - return null; - } - return prevId; - }); + if (selectedNoteIdRef.current === id) { + selectedNoteIdRef.current = null; + currentNoteRef.current = null; + setSelectedNoteId(null); + setCurrentNote(null); + setNoteConflict(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/context/NotesContext.tsx` around lines 489 - 498, Update the selected-note clearing logic around setSelectedNoteId so its updater only computes and returns the next ID, without ref writes or nested state updates. Move updates to selectedNoteIdRef, currentNoteRef, setCurrentNote, and setNoteConflict into the surrounding callback body, using the existing selectedNoteIdRef.current value to determine when the current selection is cleared.Source: Linters/SAST tools
src/components/editor/notion/markdownDocument.ts (1)
576-586: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip the second full parse when the document has no tables.
serializeMarkdownDocumentalways callsmanager.parse(markdown)to buildvisibleDocument. This doubles the parse cost of every save, every clipboard copy, and every source-mode toggle, even for notes that contain no tables.collectTableGeometriesonly readsvisibleDocumentinside itstablebranch.♻️ Proposed change
export function serializeMarkdownDocument( manager: MarkdownManagerLike, document: JSONContent, ): string { const markdown = manager.serialize(prepareTablesForMarkdown(document)); - const visibleDocument = manager.parse(markdown); + if (collectTables(document).length === 0) return markdown; + const visibleDocument = manager.parse(markdown); return injectTableMetadata( markdown, collectTableGeometries(manager, document, visibleDocument), ); }🤖 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/editor/notion/markdownDocument.ts` around lines 576 - 586, Update serializeMarkdownDocument to detect whether document contains any tables before calling manager.parse(markdown); skip the parse and geometry collection when none exist, while preserving the existing visibleDocument and injectTableMetadata flow for documents with tables. Reuse the existing table-detection or traversal helpers if available rather than introducing duplicate detection logic.src/components/editor/editorHistory.ts (1)
6-12: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winXSS (CWE-79): Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')
Reachability: Internal
Reachability path
● Entry src-tauri/src/lib.rs:2198 restore_window_session │ ▼ ● Hop src/components/preview/PreviewApp.tsx │ ▼ ● Hop src/components/editor/Editor.tsx │ ▼ ● Sink src/components/editor/editorHistory.tsParse string content in an inert HTML document instead of
innerHTML.
replaceEditorContentWithoutHistory(editor, currentNote.content)can receive raw note/source content as a string, and string content is parsed withcontainer.innerHTML = content. UseDOMParser.parseFromStringfor that path to keep resource-loading attributes and inline scripts from exercising the live document environment.🛡️ Proposed change
function normalizeLoadedContent(editor: Editor, content: Content): Content { if (typeof content === "string" { - const container = document.createElement("div"); - container.innerHTML = content; - const parsed = ProseMirrorDOMParser.fromSchema(editor.schema).parse(container); + const inert = new DOMParser().parseFromString( + `<body>${content}</body>`, + "text/html", + ); + const parsed = ProseMirrorDOMParser.fromSchema(editor.schema).parse( + inert.body, + ); return normalizeNestedTablesInJson(parsed.toJSON()); }🤖 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/editor/editorHistory.ts` around lines 6 - 12, Update normalizeLoadedContent to parse string content with DOMParser.parseFromString using an inert HTML document instead of assigning content via container.innerHTML. Pass the resulting document/body into ProseMirrorDOMParser.fromSchema(editor.schema).parse, preserving normalizeNestedTablesInJson and the existing non-string handling.Source: Linters/SAST tools
🤖 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/App.css`:
- Around line 1364-1376: Update both border declarations in
.notion-table-edge-resize::before and
.notion-table-edge-resize.is-column::before to use the lowercase currentcolor
keyword, preserving all other styling unchanged.
- Around line 1470-1479: Update the .notion-table-announcement rule to replace
the deprecated clip declaration with an equivalent clip-path value, preserving
the existing visually hidden behavior and satisfying Stylelint.
In `@src/components/editor/Editor.tsx`:
- Around line 949-955: Update flushAllPendingSaves to await flushSourceSave when
sourceNeedsSaveRef.current is true, then continue to await flushPendingSave
instead of returning early. In toggleSourceMode, when exiting source mode, clear
sourceNeedsSaveRef.current and sourceTimeoutRef so stale source state cannot
persist.
In `@src/components/editor/notion/tableIntegrity.ts`:
- Around line 107-120: Update normalizeNestedTablesInJson to calculate table
width from column occupancy rather than direct cell counts, honoring each cell’s
colspan and active rowspan. Track occupied columns across rows so padding fills
only unoccupied positions and preserves rectangular alignment without creating
extra cells. Keep normalizeCell attrs intact, and add regression cases covering
colspan and overlapping active rowspan scenarios.
In `@src/components/settings/SettingsPage.test.tsx`:
- Around line 70-72: Update the drag-region assertion in the SettingsPage test
to account for the isWindows platform flag, expecting zero elements on Windows
and both guarded elements otherwise; alternatively, mock the platform module to
make the test environment deterministic.
In `@src/context/NotesContext.tsx`:
- Around line 781-788: Update the recovered-draft handling in the NotesContext
provider so the informational message from listDraftCheckpoints is no longer
passed to setError. Route the orphaned.length notification through the existing
toast mechanism or a dedicated informational state, preserving the
singular/plural message and only notifying when recovered drafts are present.
In `@src/lib/draftCheckpoint.ts`:
- Around line 151-175: Update reconcileDraftCheckpoint to compare
checkpoint.metadata.baseRevision with diskNote.revision when deciding whether
recovery is needed, not just checkpoint.markdown against diskNote.content.
Preserve the existing clean-checkpoint result when both the content and base
revision match, and ensure revision mismatches explicitly produce the recovered
draft with diskNote as remote.
- Around line 87-95: When the write in flush fails and pending is restored,
re-arm the existing schedule timer so the checkpoint is retried even if no
further markDirty call occurs. Update the flush catch block to invoke schedule
after assigning pending, while accounting for schedule’s const initialization
order by moving schedule above flush or deferring the call until initialization.
In `@src/lib/useWindowSessionPersistence.ts`:
- Around line 73-78: Update the useEffect persistence flow in
useWindowSessionPersistence so skipHydrationPersistenceRef is consumed
immediately after the isRestored check and before returning for a missing
notesFolder. Preserve the existing skip-and-reset behavior, ensuring later
notesFolder availability cannot discard a real sidebar or focus change.
- Around line 181-184: Update the useCallback close-persistence flow around
geometryCaptureRef.current() so geometry capture failures are isolated and do
not reject the callback or block window closure. Catch or otherwise contain that
failure, then always invoke writer.flush() to persist queued patches, preserving
the existing writer dependency.
---
Outside diff comments:
In `@src/context/ThemeContext.tsx`:
- Around line 468-499: Add the missing try block in resetEditorFontSettings
before the await updateGlobalSettings call, enclosing that operation and its
closing logic so the existing catch block is syntactically paired and the module
parses successfully.
- Around line 716-721: Move the contextValue useMemo hook in ThemeProvider
before the isInitialized guard so it executes on every render. Keep the null
return immediately after the guard, preserving the existing initialized
rendering behavior while maintaining consistent hook order.
---
Duplicate comments:
In `@src-tauri/src/lib.rs`:
- Around line 429-444: Update emit_native_open_folder_request to emit a distinct
"open-folder" event instead of "open-folder-in-new-window", while leaving
emit_native_new_window_request unchanged.
- Around line 446-457: Update workspace_path_key to derive its persisted
identifier with the stable hex_sha256 digest instead of DefaultHasher,
preserving the existing workspace- prefix and path-based input. Ensure
workspace_window_label continues using workspace_path_key so both search-index
directories and session record keys receive stable identifiers.
- Around line 2073-2075: Update the window cleanup loop to avoid calling
window.destroy() directly, which discards unsaved drafts; request a graceful
close that lets each editor flush and write its recovery snapshot, reusing the
existing close_window_after_save flow where applicable. Ensure every window is
processed before force-destruction is considered.
- Around line 3954-3959: Update the import notification and focus flow near the
existing metadata selection logic to target the invoking window’s label instead
of the hardcoded "main" label. Use window.label() for both app.emit_to and the
subsequent webview-window lookup, preserving the existing show and set_focus
behavior.
---
Nitpick comments:
In `@src-tauri/src/lib.rs`:
- Around line 1257-1274: In workspace_for_window, remove the repeated
workspace_session(window_label) lookup in the final error path; for non-fallback
labels, return the existing “Workspace session not found for window” error
directly after the initial lookup returns None, while preserving the
fallback-to-main behavior.
In `@src/components/editor/Editor.tsx`:
- Around line 902-927: Update the auto-save debounce timer in the save callback
around saveImmediately from 500 ms to 300 ms, matching handleSourceChange and
the documented auto-save interval. Preserve the existing save validation and
error-handling behavior.
In `@src/components/editor/editorHistory.ts`:
- Around line 6-12: Update normalizeLoadedContent to parse string content with
DOMParser.parseFromString using an inert HTML document instead of assigning
content via container.innerHTML. Pass the resulting document/body into
ProseMirrorDOMParser.fromSchema(editor.schema).parse, preserving
normalizeNestedTablesInJson and the existing non-string handling.
In `@src/components/editor/notion/markdownDocument.ts`:
- Around line 576-586: Update serializeMarkdownDocument to detect whether
document contains any tables before calling manager.parse(markdown); skip the
parse and geometry collection when none exist, while preserving the existing
visibleDocument and injectTableMetadata flow for documents with tables. Reuse
the existing table-detection or traversal helpers if available rather than
introducing duplicate detection logic.
In `@src/components/editor/notion/selectionDecoration.test.ts`:
- Around line 61-64: Update the appStyles file lookup in
selectionDecoration.test.ts to resolve App.css relative to the test module using
import.meta.url, rather than process.cwd(), while preserving the existing UTF-8
read.
- Line 195: Update the affected selection tests around the Shift+ArrowLeft and
Shift+ArrowRight cases so their names match the behavior actually asserted: the
extension event remains unhandled. Alternatively, explicitly invoke
syncNativeDOMHead for the second dispatch and assert the resulting extension.
Apply the same correction to both test cases, not just the first.
In `@src/context/NotesContext.tsx`:
- Around line 489-498: Update the selected-note clearing logic around
setSelectedNoteId so its updater only computes and returns the next ID, without
ref writes or nested state updates. Move updates to selectedNoteIdRef,
currentNoteRef, setCurrentNote, and setNoteConflict into the surrounding
callback body, using the existing selectedNoteIdRef.current value to determine
when the current selection is cleared.
In `@src/lib/draftCheckpoint.ts`:
- Line 176: Move the type-only Note import to the top of the file alongside the
other module-level imports, while keeping reconcileDraftCheckpoint and its Note
usage unchanged.
In `@src/lib/noteSync.ts`:
- Around line 59-71: Add a doc comment directly above resolveRemoteNoteId
documenting its three return states: undefined for unrelated events, null when
the note was deleted, and a string containing the current note ID otherwise.
Explicitly preserve the distinction between null and undefined because callers
use it to clear or ignore the open note.
🪄 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: fd0bffeb-3784-4f7b-9009-97e410a81161
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (110)
package.jsonsrc-tauri/capabilities/default.jsonsrc-tauri/src/draft_checkpoint.rssrc-tauri/src/lib.rssrc-tauri/src/note_persistence_tests.rssrc-tauri/src/persistence.rssrc-tauri/src/watcher_debounce.rssrc/App.csssrc/App.tsxsrc/components/editor/Editor.tsxsrc/components/editor/EditorWidthHandle.test.tsxsrc/components/editor/EditorWidthHandle.tsxsrc/components/editor/editorHistory.test.tssrc/components/editor/editorHistory.tssrc/components/editor/notion/NotionMenus.test.tsxsrc/components/editor/notion/NotionMenus.tsxsrc/components/editor/notion/TableControls.test.tsxsrc/components/editor/notion/TableControls.tsxsrc/components/editor/notion/interfaceGeometry.test.tssrc/components/editor/notion/interfaceGeometry.tssrc/components/editor/notion/markdownDocument.test.tssrc/components/editor/notion/markdownDocument.tssrc/components/editor/notion/markdownMarks.test.tssrc/components/editor/notion/markdownMarks.tssrc/components/editor/notion/selectionDecoration.test.tssrc/components/editor/notion/selectionDecoration.tssrc/components/editor/notion/tableAutoScroll.test.tssrc/components/editor/notion/tableAutoScroll.tssrc/components/editor/notion/tableClipboard.test.tssrc/components/editor/notion/tableClipboard.tssrc/components/editor/notion/tableEdgeDrag.test.tssrc/components/editor/notion/tableEdgeDrag.tssrc/components/editor/notion/tableExtensions.test.tssrc/components/editor/notion/tableExtensions.tssrc/components/editor/notion/tableIntegrity.test.tssrc/components/editor/notion/tableIntegrity.tssrc/components/editor/notion/tableMetadata.tssrc/components/editor/notion/tableNonRegression.test.tssrc/components/editor/notion/tablePerformance.test.tssrc/components/editor/notion/tablePointerDrag.test.tssrc/components/editor/notion/tablePointerDrag.tssrc/components/editor/notion/tableProximity.test.tssrc/components/editor/notion/tableProximity.tssrc/components/editor/notion/tableRowResize.test.tssrc/components/editor/notion/tableRowResize.tssrc/components/editor/notion/tableTransactions.test.tssrc/components/editor/notion/tableTransactions.tssrc/components/editor/notion/tableView.test.tssrc/components/editor/notion/tableView.tssrc/components/layout/Sidebar.error.test.tssrc/components/layout/Sidebar.tsxsrc/components/layout/SidebarControls.tsxsrc/components/layout/WorkspaceMenu.test.tsxsrc/components/layout/WorkspaceMenu.tsxsrc/components/notes/NoteList.tsxsrc/components/preview/PreviewApp.tsxsrc/components/settings/EditorSettingsSection.test.tsxsrc/components/settings/EditorSettingsSection.tsxsrc/components/settings/SettingsPage.test.tsxsrc/components/settings/SettingsPage.tsxsrc/context/GitContext.tsxsrc/context/NotesContext.tsxsrc/context/ThemeContext.tsxsrc/lib/conflictResolution.test.tssrc/lib/conflictResolution.tssrc/lib/documentMutationSafety.test.tssrc/lib/documentMutationSafety.tssrc/lib/draftCheckpoint.test.tssrc/lib/draftCheckpoint.tssrc/lib/editorToolbar.test.tssrc/lib/editorToolbar.tssrc/lib/editorWidthResize.test.tssrc/lib/editorWidthResize.tssrc/lib/noteSync.test.tssrc/lib/noteSync.tssrc/lib/serializedWriter.test.tssrc/lib/serializedWriter.tssrc/lib/settingsScope.test.tssrc/lib/settingsScope.tssrc/lib/standaloneRecreation.test.tssrc/lib/standaloneRecreation.tssrc/lib/standaloneReload.test.tssrc/lib/standaloneReload.tssrc/lib/titleBarNoteInfo.test.tssrc/lib/titleBarNoteInfo.tssrc/lib/useWindowSessionPersistence.tssrc/lib/useWindowShortcuts.tssrc/lib/windowClose.test.tssrc/lib/windowClose.tssrc/lib/windowCloseCallsites.test.tssrc/lib/windowSession.test.tssrc/lib/windowSession.tssrc/lib/windowShortcutCallsites.test.tssrc/lib/windowShortcuts.test.tssrc/lib/windowShortcuts.tssrc/lib/workspace.test.tssrc/lib/workspace.tssrc/lib/workspaceSwitch.test.tssrc/lib/workspaceSwitch.tssrc/services/draftCheckpoint.test.tssrc/services/draftCheckpoint.tssrc/services/files.test.tssrc/services/files.tssrc/services/notes.test.tssrc/services/notes.tssrc/services/windowLifecycle.test.tssrc/services/windowLifecycle.tssrc/services/windowSession.test.tssrc/services/windowSession.tssrc/types/note.ts
💤 Files with no reviewable changes (1)
- src/components/layout/SidebarControls.tsx
🚧 Files skipped from review as they are similar to previous changes (92)
- src/lib/windowCloseCallsites.test.ts
- src/lib/standaloneReload.test.ts
- src-tauri/capabilities/default.json
- src/lib/standaloneReload.ts
- src/lib/workspace.test.ts
- src/lib/conflictResolution.ts
- src/lib/workspaceSwitch.ts
- src/lib/conflictResolution.test.ts
- src/lib/standaloneRecreation.test.ts
- src/services/windowSession.test.ts
- src/lib/windowShortcuts.test.ts
- src/components/editor/notion/tableEdgeDrag.test.ts
- src/components/editor/notion/interfaceGeometry.ts
- src/lib/documentMutationSafety.ts
- src/services/windowSession.ts
- src/components/editor/notion/tablePointerDrag.test.ts
- src/services/draftCheckpoint.test.ts
- src/components/editor/notion/tableTransactions.test.ts
- package.json
- src/lib/documentMutationSafety.test.ts
- src/lib/windowShortcutCallsites.test.ts
- src/services/files.test.ts
- src/components/editor/notion/tableExtensions.test.ts
- src/components/settings/SettingsPage.tsx
- src/components/editor/EditorWidthHandle.test.tsx
- src/lib/windowShortcuts.ts
- src/lib/standaloneRecreation.ts
- src/lib/windowClose.ts
- src/lib/editorWidthResize.test.ts
- src/components/editor/notion/interfaceGeometry.test.ts
- src/components/settings/EditorSettingsSection.test.tsx
- src/components/editor/notion/NotionMenus.test.tsx
- src/lib/workspaceSwitch.test.ts
- src/lib/windowClose.test.ts
- src/lib/windowSession.test.ts
- src-tauri/src/watcher_debounce.rs
- src/services/windowLifecycle.ts
- src/components/editor/editorHistory.test.ts
- src/components/editor/notion/tableEdgeDrag.ts
- src/lib/editorWidthResize.ts
- src/components/editor/notion/tableView.test.ts
- src/components/editor/notion/tableAutoScroll.ts
- src/components/editor/notion/tableRowResize.ts
- src/components/editor/notion/selectionDecoration.ts
- src/components/layout/WorkspaceMenu.tsx
- src/lib/editorToolbar.ts
- src/components/editor/notion/markdownMarks.ts
- src/components/editor/notion/tablePointerDrag.ts
- src/lib/settingsScope.ts
- src/lib/titleBarNoteInfo.ts
- src/lib/noteSync.test.ts
- src/services/notes.test.ts
- src/components/layout/WorkspaceMenu.test.tsx
- src/context/GitContext.tsx
- src/components/notes/NoteList.tsx
- src/components/editor/notion/tableRowResize.test.ts
- src/services/windowLifecycle.test.ts
- src/components/editor/notion/tableView.ts
- src/components/editor/notion/tableMetadata.ts
- src/lib/serializedWriter.ts
- src/lib/settingsScope.test.ts
- src/services/files.ts
- src/lib/titleBarNoteInfo.test.ts
- src/lib/workspace.ts
- src/components/editor/notion/tablePerformance.test.ts
- src-tauri/src/note_persistence_tests.rs
- src/components/editor/notion/markdownMarks.test.ts
- src/lib/serializedWriter.test.ts
- src/components/editor/notion/tableClipboard.ts
- src/components/editor/notion/tableAutoScroll.test.ts
- src/types/note.ts
- src/components/editor/notion/markdownDocument.test.ts
- src/lib/draftCheckpoint.test.ts
- src/components/settings/EditorSettingsSection.tsx
- src/components/editor/notion/tableIntegrity.test.ts
- src/components/editor/notion/NotionMenus.tsx
- src/components/editor/notion/TableControls.tsx
- src/components/editor/notion/TableControls.test.tsx
- src/services/notes.ts
- src/lib/windowSession.ts
- src/components/editor/notion/tableProximity.test.ts
- src/components/layout/Sidebar.tsx
- src/components/editor/notion/tableClipboard.test.ts
- src-tauri/src/draft_checkpoint.rs
- src/components/editor/notion/tableProximity.ts
- src/components/editor/EditorWidthHandle.tsx
- src/components/editor/notion/tableTransactions.ts
- src/lib/editorToolbar.test.ts
- src/components/editor/notion/tableExtensions.ts
- src-tauri/src/persistence.rs
- src/App.tsx
- src/components/editor/notion/tableNonRegression.test.ts
| const flushAllPendingSaves = useCallback(async () => { | ||
| if (sourceNeedsSaveRef.current) { | ||
| await flushSourceSave(); | ||
| return; | ||
| } | ||
| await flushPendingSave(); | ||
| }, [flushPendingSave, flushSourceSave]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Flush both pending saves instead of returning after the source flush.
flushAllPendingSaves returns after flushSourceSave when sourceNeedsSaveRef.current is true. toggleSourceMode does not clear sourceNeedsSaveRef when the user leaves source mode, so the flag can remain true while sourceContentRef holds stale text. If the user then edits the rich editor and the window closes or the workspace switches, this path writes the stale source snapshot and skips flushPendingSave, so the newer editor edits are lost.
Flush both pending saves, source first, then the editor.
🛡️ Proposed fix
const flushAllPendingSaves = useCallback(async () => {
if (sourceNeedsSaveRef.current) {
await flushSourceSave();
- return;
}
await flushPendingSave();
}, [flushPendingSave, flushSourceSave]);Also clear sourceNeedsSaveRef.current and sourceTimeoutRef when toggleSourceMode exits source mode, so the stale flag cannot survive the transition.
🤖 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/editor/Editor.tsx` around lines 949 - 955, Update
flushAllPendingSaves to await flushSourceSave when sourceNeedsSaveRef.current is
true, then continue to await flushPendingSave instead of returning early. In
toggleSourceMode, when exiting source mode, clear sourceNeedsSaveRef.current and
sourceTimeoutRef so stale source state cannot persist.
| } else { | ||
| const orphaned = await listDraftCheckpoints().catch(() => []); | ||
| if (orphaned.length > 0) { | ||
| setError( | ||
| `${orphaned.length} recovered draft${orphaned.length === 1 ? " is" : "s are"} available from a previous interrupted session`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not report recovered drafts through the error channel.
setError drives the provider's error state, which the UI presents as a failure. Recovered drafts are informational, so the user sees an error for a successful recovery. Use a toast or a dedicated informational field.
🤖 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/context/NotesContext.tsx` around lines 781 - 788, Update the
recovered-draft handling in the NotesContext provider so the informational
message from listDraftCheckpoints is no longer passed to setError. Route the
orphaned.length notification through the existing toast mechanism or a dedicated
informational state, preserving the singular/plural message and only notifying
when recovered drafts are present.
| return enqueue(async () => { | ||
| try { | ||
| await storage.write(checkpoint); | ||
| } catch (error) { | ||
| pending ??= checkpoint; | ||
| throw error; | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A failed checkpoint write is restored but never retried.
flush restores pending when storage.write rejects, which shows retry intent. No new timer is armed after the restore. If the user stops editing after a transient failure, nothing calls flush again, so the checkpoint stays in memory and the draft has no recovery snapshot. schedule runs only from markDirty.
Re-arm the timer when you restore the pending checkpoint.
🐛 Proposed fix
const flush = (): Promise<void> => {
cancelTimer();
const checkpoint = pending;
if (!checkpoint) return operationTail;
pending = undefined;
return enqueue(async () => {
try {
await storage.write(checkpoint);
} catch (error) {
- pending ??= checkpoint;
+ pending ??= checkpoint;
+ if (!disposed) schedule();
throw error;
}
});
};schedule is declared after flush, so hoisting applies to the const binding. Move schedule above flush, or wrap the call so it runs only after initialization.
📝 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.
| return enqueue(async () => { | |
| try { | |
| await storage.write(checkpoint); | |
| } catch (error) { | |
| pending ??= checkpoint; | |
| throw error; | |
| } | |
| }); | |
| }; | |
| return enqueue(async () => { | |
| try { | |
| await storage.write(checkpoint); | |
| } catch (error) { | |
| pending ??= checkpoint; | |
| if (!disposed) schedule(); | |
| throw error; | |
| } | |
| }); | |
| }; |
🤖 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/draftCheckpoint.ts` around lines 87 - 95, When the write in flush
fails and pending is restored, re-arm the existing schedule timer so the
checkpoint is retried even if no further markDirty call occurs. Update the flush
catch block to invoke schedule after assigning pending, while accounting for
schedule’s const initialization order by moving schedule above flush or
deferring the call until initialization.
| export function reconcileDraftCheckpoint( | ||
| diskNote: Note, | ||
| checkpoint: DraftCheckpoint, | ||
| ): { | ||
| note: Note; | ||
| remote: Note | null; | ||
| recovered: boolean; | ||
| shouldClear: boolean; | ||
| } { | ||
| if (checkpoint.markdown === diskNote.content) { | ||
| return { | ||
| note: diskNote, | ||
| remote: null, | ||
| recovered: false, | ||
| shouldClear: true, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| note: { ...diskNote, content: checkpoint.markdown }, | ||
| remote: diskNote, | ||
| recovered: true, | ||
| shouldClear: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find callers of reconcileDraftCheckpoint and check baseRevision handling.
rg -n -C10 'reconcileDraftCheckpoint|baseRevision' src --type=ts --type=tsxRepository: erictli/scratch
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and search callers/baseRevision only in files present in the repo.
wc -l src/lib/draftCheckpoint.ts
echo '--- outline draftCheckpoint.ts ---'
ast-grep outline src/lib/draftCheckpoint.ts || true
echo '--- reconcileDraftCheckpoint and baseRevision references ---'
rg -n -C12 'reconcileDraftCheckpoint|baseRevision' $(git ls-files | grep -E '\.(ts|tsx)$' | tr '\n' ' ')Repository: erictli/scratch
Length of output: 17529
Handle baseRevision mismatches in checkpoint recovery.
reconcileDraftCheckpoint only checks markdown against diskNote.content, but checkpoints record metadata.baseRevision from the note revision at save time. If the disk file changed after recovery, the caller receives the draft with recovered: true and remote: diskNote; handle this either by checking checkpoint.metadata.baseRevision === diskNote.revision here or making the recovery decision explicit in src/context/NotesContext.tsx.
🤖 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/draftCheckpoint.ts` around lines 151 - 175, Update
reconcileDraftCheckpoint to compare checkpoint.metadata.baseRevision with
diskNote.revision when deciding whether recovery is needed, not just
checkpoint.markdown against diskNote.content. Preserve the existing
clean-checkpoint result when both the content and base revision match, and
ensure revision mismatches explicitly produce the recovered draft with diskNote
as remote.
| useEffect(() => { | ||
| if (!isRestored || !notesFolder) return; | ||
| if (skipHydrationPersistenceRef.current) { | ||
| skipHydrationPersistenceRef.current = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The hydration skip flag can be consumed after a real change.
Line 74 returns before line 75 consumes skipHydrationPersistenceRef. If notesFolder is still null when hydration completes, the flag stays true. When notesFolder becomes non-null later, the first run consumes the flag and drops that patch, so a sidebar or focus change made in between is never persisted. Consume the flag before the notesFolder guard.
🐛 Proposed fix
useEffect(() => {
- if (!isRestored || !notesFolder) return;
- if (skipHydrationPersistenceRef.current) {
- skipHydrationPersistenceRef.current = false;
- return;
- }
+ if (!isRestored) return;
+ if (skipHydrationPersistenceRef.current) {
+ skipHydrationPersistenceRef.current = false;
+ return;
+ }
+ if (!notesFolder) return;📝 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.
| useEffect(() => { | |
| if (!isRestored || !notesFolder) return; | |
| if (skipHydrationPersistenceRef.current) { | |
| skipHydrationPersistenceRef.current = false; | |
| return; | |
| } | |
| useEffect(() => { | |
| if (!isRestored) return; | |
| if (skipHydrationPersistenceRef.current) { | |
| skipHydrationPersistenceRef.current = false; | |
| return; | |
| } | |
| if (!notesFolder) return; |
🤖 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/useWindowSessionPersistence.ts` around lines 73 - 78, Update the
useEffect persistence flow in useWindowSessionPersistence so
skipHydrationPersistenceRef is consumed immediately after the isRestored check
and before returning for a missing notesFolder. Preserve the existing
skip-and-reset behavior, ensuring later notesFolder availability cannot discard
a real sidebar or focus change.
Verify request_full_window_closure_for_preview uses window.close() instead of window.destroy(). Verify is_full_editor_window only matches main and workspace-*. Add Rust tests proving only main and workspace-* are targeted, preview windows are ignored, and Preferences is not treated as a full editor window.
Move ContentRevision to a dedicated hashing module and use it from persistence. This stabilizes note identity across workspace switches.
Wire native menu actions to their own events. File > New Window opens a new workspace window. File > Open Folder… targets the focused window.
Stabilize window session persistence with best-effort geometry capture and serialized writers that don't swallow failures silently.
Import completion and preview actions target the invoking window label. Best-effort cleanup for preview draft checkpoints.
516b698 to
8fa3d41
Compare
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 (2)
src/context/ThemeContext.tsx (2)
724-795: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftSplit the provider into data and actions contexts.
contextValuecombines all settings data and all setters. Any settings-data update re-renders consumers that only need actions. Use theNotesContextdual-context pattern for separate data and actions values.As per coding guidelines, “Use
NotesContextwith dual context pattern (data/actions separated) for performance optimization.”🤖 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/context/ThemeContext.tsx` around lines 724 - 795, Split the ThemeContext provider value around contextValue into separate data and actions contexts, following the existing NotesContext dual-context pattern. Put theme and settings state values in the data context, setters and reset/reload/cycle functions in the actions context, and update consumers/types/provider exports to use the appropriate context while preserving the existing API behavior.Source: Coding guidelines
559-590: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecover state after a settings write fails.
These setters update local state before persistence. If
updateGlobalSettings()fails, they only log the error. Show a user-facing error and reload or roll back the affected setting so the UI does not display an unsaved value.As per coding guidelines, “Implement error handling with user-friendly messages.”
🤖 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/context/ThemeContext.tsx` around lines 559 - 590, Update setEditorWidthResizeEnabled, setEditorToolbarVisible, and updateTitleBarNoteInfo so failed updateGlobalSettings calls both show a user-friendly user-facing error and restore the affected setting from persisted state, either by reloading settings or rolling back the local state. Preserve the optimistic update behavior when persistence succeeds.Source: Coding guidelines
🧹 Nitpick comments (4)
src/services/windowLifecycle.ts (1)
17-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument how
requestCurrentWindowClosediffers fromcloseWindowAfterSave.This module now exports two close entry points with different semantics.
requestCurrentWindowClosestarts a close request, which theonCloseRequestedhandler insrc/App.tsxintercepts to flush the draft.closeWindowAfterSaveperforms the final destruction after that approval. A caller that picks the wrong one skips draft flushing.Add a short doc comment so the ordering is explicit at the call site.
♻️ Proposed comment
+/** + * Requests a close for the current window. This fires `onCloseRequested`, so + * draft flushing still runs. Call `closeWindowAfterSave` only from inside that + * handler, after the draft is persisted. + */ export async function requestCurrentWindowClose(): Promise<void> { await getCurrentWindow().close(); }🤖 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/services/windowLifecycle.ts` around lines 17 - 19, Add a concise doc comment above requestCurrentWindowClose explaining that it initiates the close request intercepted by onCloseRequested for draft flushing, while closeWindowAfterSave performs final destruction after approval; make the ordering and distinction clear for callers.src-tauri/src/hashing.rs (2)
233-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese two tests do not exercise the code they describe.
workspace_path_key_deterministicanddraft_checkpoint_filename_deterministicreimplement the identity encoding locally and then assertsha256_hex(x) == sha256_hex(x). The real producers areworkspace_path_keyinsrc-tauri/src/lib.rsandcheckpoint_file_nameinsrc-tauri/src/draft_checkpoint.rs. If either changes, these tests still pass.Either call the real functions, or remove the tests and keep determinism coverage in the owning modules.
🤖 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-tauri/src/hashing.rs` around lines 233 - 258, Replace the self-referential tests workspace_path_key_deterministic and draft_checkpoint_filename_deterministic with assertions that call the production functions workspace_path_key and checkpoint_file_name using identical inputs twice. Remove the locally duplicated identity encoding and verify the real outputs are deterministic, retaining the 64-character hash assertion only where applicable.
101-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a known-answer test for the two-block padding path.
The SHA-256 implementation handles 56-byte final remainders with a 128-byte padding tail. Current tests only cover empty input and short inputs that fit padding in one final block, so this branch is uncovered. Add a known digest for an input such as
&[b'a'; 56].🤖 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-tauri/src/hashing.rs` around lines 101 - 110, Add a known-answer test for the two-block padding branch in the SHA-256 implementation, using an input such as &[b'a'; 56] and asserting its expected digest. Keep the existing empty and short-input tests unchanged while covering the tail_length = 128 path.src/components/editor/notion/markdownDocument.ts (1)
168-176: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReplace the dynamic
RegExpinisFenceClosewith string checks.The static analysis ReDoS warning is a false positive.
fence.characteris one of two hard-coded literals andfence.lengthis a number, so no metacharacters reach the pattern, and the generated pattern is a single bounded repetition that cannot backtrack.The pattern is still recompiled for every line inside an open fence. A string comparison removes both the recompilation and the warning.
♻️ Proposed refactor
function isFenceClose( line: string, fence: { character: string; length: number }, ): boolean { - const pattern = new RegExp( - `^\\s*${fence.character === "`" ? "`" : "~"}{${fence.length},}\\s*$`, - ); - return pattern.test(line); + const trimmed = line.trim(); + if (trimmed.length < fence.length) return false; + for (const character of trimmed) { + if (character !== fence.character) return false; + } + return true; }Note one behavior difference: the original allows only leading whitespace of any length, and CommonMark allows at most three leading spaces on a closing fence. Neither version enforces that limit, so this refactor does not change fence matching for existing documents.
🤖 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/editor/notion/markdownDocument.ts` around lines 168 - 176, Replace the dynamic RegExp construction in isFenceClose with string-based checks: derive the expected fence character, verify optional leading whitespace and trailing whitespace, and confirm the non-whitespace fence run has at least fence.length characters. Preserve the current behavior of allowing any amount of leading whitespace and requiring only the matching fence character followed by whitespace.Source: Linters/SAST tools
🤖 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-tauri/src/persistence.rs`:
- Around line 204-215: Update the fallback write flow around direct.write_all,
direct.flush, and direct.sync_all so any failure removes the destination file at
path before returning the original write error. Defer
sync_parent_directory(parent) until after all write, flush, and sync results
have succeeded, ensuring those underlying errors are reported before any
parent-directory sync failure.
In `@src/App.tsx`:
- Around line 527-531: Update the folder-selection flow around the App
component’s open call to use a service wrapper that invokes a Tauri command
instead of calling the native dialog plugin directly. Define and register the
corresponding folder-selection command in src-tauri/src/lib.rs, have it perform
the existing directory-only, single-selection dialog behavior, and update the
frontend to consume the command result.
- Around line 538-557: The open-folder listener currently lives only in
AppContent, so Preferences and preview windows lack a handler. Move the
useEffect containing listen("open-folder") and its cleanup/error handling from
AppContent into App or a shared shell component mounted by every window mode,
preserving its openFolderInWorkspaceWindow dependency and disposal behavior.
In `@src/components/editor/editorHistory.ts`:
- Around line 36-43: Update the editor state refresh in editorHistory.ts to
clear history while preserving existing non-history plugin state, using
EditorState.reconfigure() or an equivalent merge rather than
EditorState.create() with loadedState.plugins. Ensure selection and
search-highlight plugin state survives note/source-mode reloads, and add or
update tests covering preservation of selection decorations.
In `@src/components/editor/notion/tableIntegrity.ts`:
- Around line 43-50: Update the inline-node check in normalizeCellContent to
include wikilink alongside text and hardBreak, ensuring wikilink nodes are
accumulated in pendingInline and normalized into paragraph content rather than
pushed directly as table-cell children.
In `@src/components/settings/SettingsPage.test.tsx`:
- Around line 30-41: Update the SettingsPage test teardown around
mountSettingsPage so every created root is tracked and unmounted during
afterEach, ensuring window keydown listeners are cleaned up. Save the original
navigator.userAgent before the Windows-specific test and restore it in a finally
block, so cleanup occurs even when rendering or assertions fail.
In `@src/context/NotesContext.tsx`:
- Around line 664-673: Guard relocated-note reload responses against stale
selections before calling setCurrentNote. In src/context/NotesContext.tsx lines
664-673, 696-703, and 734-743, reuse the request-token guard from selectNote or
a shared guarded reload helper so each response updates currentNote only if the
corresponding note selection is still current.
- Around line 498-504: Reset hasExternalChanges when deleteNote clears the
selected note. In deleteFolderAction, clear selectedNoteIdRef, currentNoteRef,
noteConflict, and hasExternalChanges together with the selection state; apply
these changes at src/context/NotesContext.tsx lines 498-504 and 625-632.
In `@src/services/notes.test.ts`:
- Around line 169-186: Update the duplicateNote test around the existing
invokeMock assertion to verify exactly one backend invocation. Add a call-count
assertion after checking the duplicate_note command, while preserving the
existing result assertions.
---
Outside diff comments:
In `@src/context/ThemeContext.tsx`:
- Around line 724-795: Split the ThemeContext provider value around contextValue
into separate data and actions contexts, following the existing NotesContext
dual-context pattern. Put theme and settings state values in the data context,
setters and reset/reload/cycle functions in the actions context, and update
consumers/types/provider exports to use the appropriate context while preserving
the existing API behavior.
- Around line 559-590: Update setEditorWidthResizeEnabled,
setEditorToolbarVisible, and updateTitleBarNoteInfo so failed
updateGlobalSettings calls both show a user-friendly user-facing error and
restore the affected setting from persisted state, either by reloading settings
or rolling back the local state. Preserve the optimistic update behavior when
persistence succeeds.
---
Nitpick comments:
In `@src-tauri/src/hashing.rs`:
- Around line 233-258: Replace the self-referential tests
workspace_path_key_deterministic and draft_checkpoint_filename_deterministic
with assertions that call the production functions workspace_path_key and
checkpoint_file_name using identical inputs twice. Remove the locally duplicated
identity encoding and verify the real outputs are deterministic, retaining the
64-character hash assertion only where applicable.
- Around line 101-110: Add a known-answer test for the two-block padding branch
in the SHA-256 implementation, using an input such as &[b'a'; 56] and asserting
its expected digest. Keep the existing empty and short-input tests unchanged
while covering the tail_length = 128 path.
In `@src/components/editor/notion/markdownDocument.ts`:
- Around line 168-176: Replace the dynamic RegExp construction in isFenceClose
with string-based checks: derive the expected fence character, verify optional
leading whitespace and trailing whitespace, and confirm the non-whitespace fence
run has at least fence.length characters. Preserve the current behavior of
allowing any amount of leading whitespace and requiring only the matching fence
character followed by whitespace.
In `@src/services/windowLifecycle.ts`:
- Around line 17-19: Add a concise doc comment above requestCurrentWindowClose
explaining that it initiates the close request intercepted by onCloseRequested
for draft flushing, while closeWindowAfterSave performs final destruction after
approval; make the ordering and distinction clear for callers.
🪄 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: 4572fb02-f594-4b3e-90b5-c4edd51f17ad
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (64)
package.jsonsrc-tauri/capabilities/default.jsonsrc-tauri/src/draft_checkpoint.rssrc-tauri/src/hashing.rssrc-tauri/src/lib.rssrc-tauri/src/persistence.rssrc/App.csssrc/App.tsxsrc/components/editor/Editor.tsxsrc/components/editor/editorHistory.test.tssrc/components/editor/editorHistory.tssrc/components/editor/notion/NotionMenus.test.tsxsrc/components/editor/notion/NotionMenus.tsxsrc/components/editor/notion/TableControls.test.tsxsrc/components/editor/notion/TableControls.tsxsrc/components/editor/notion/interfaceGeometry.test.tssrc/components/editor/notion/interfaceGeometry.tssrc/components/editor/notion/markdownDocument.test.tssrc/components/editor/notion/markdownDocument.tssrc/components/editor/notion/markdownMarks.test.tssrc/components/editor/notion/markdownMarks.tssrc/components/editor/notion/selectionDecoration.test.tssrc/components/editor/notion/selectionDecoration.tssrc/components/editor/notion/tableAutoScroll.test.tssrc/components/editor/notion/tableAutoScroll.tssrc/components/editor/notion/tableClipboard.test.tssrc/components/editor/notion/tableClipboard.tssrc/components/editor/notion/tableEdgeDrag.test.tssrc/components/editor/notion/tableEdgeDrag.tssrc/components/editor/notion/tableExtensions.test.tssrc/components/editor/notion/tableExtensions.tssrc/components/editor/notion/tableIntegrity.test.tssrc/components/editor/notion/tableIntegrity.tssrc/components/editor/notion/tableMetadata.tssrc/components/editor/notion/tableNonRegression.test.tssrc/components/editor/notion/tablePerformance.test.tssrc/components/editor/notion/tablePointerDrag.test.tssrc/components/editor/notion/tablePointerDrag.tssrc/components/editor/notion/tableProximity.test.tssrc/components/editor/notion/tableProximity.tssrc/components/editor/notion/tableRowResize.test.tssrc/components/editor/notion/tableRowResize.tssrc/components/editor/notion/tableTransactions.test.tssrc/components/editor/notion/tableTransactions.tssrc/components/editor/notion/tableView.test.tssrc/components/editor/notion/tableView.tssrc/components/layout/Sidebar.tsxsrc/components/notes/NoteList.tsxsrc/components/preview/PreviewApp.tsxsrc/components/settings/SettingsPage.test.tsxsrc/context/NotesContext.test.tsxsrc/context/NotesContext.tsxsrc/context/ThemeContext.tsxsrc/lib/serializedWriter.tssrc/lib/useWindowSessionPersistence.tssrc/lib/useWindowShortcuts.tssrc/lib/windowClose.tssrc/lib/windowCloseCallsites.test.tssrc/lib/windowSession.test.tssrc/lib/windowSession.tssrc/lib/windowShortcutCallsites.test.tssrc/services/notes.test.tssrc/services/notes.tssrc/services/windowLifecycle.ts
💤 Files with no reviewable changes (1)
- src-tauri/capabilities/default.json
🚧 Files skipped from review as they are similar to previous changes (45)
- src/components/editor/notion/tableEdgeDrag.ts
- src/components/editor/notion/tableRowResize.test.ts
- src/lib/windowCloseCallsites.test.ts
- src/components/editor/notion/tableMetadata.ts
- src/components/editor/notion/tableProximity.ts
- src/components/editor/notion/tableClipboard.test.ts
- src/lib/windowClose.ts
- src/components/editor/notion/tableExtensions.test.ts
- src/lib/windowShortcutCallsites.test.ts
- src/components/editor/notion/tablePointerDrag.ts
- src/components/editor/notion/tableTransactions.test.ts
- src/components/editor/notion/selectionDecoration.ts
- src/components/editor/notion/tableAutoScroll.ts
- src/components/editor/notion/tableProximity.test.ts
- package.json
- src/components/editor/notion/tableExtensions.ts
- src/lib/useWindowShortcuts.ts
- src/components/editor/notion/interfaceGeometry.test.ts
- src/components/editor/notion/tableView.ts
- src/components/editor/editorHistory.test.ts
- src/components/editor/notion/tableAutoScroll.test.ts
- src/components/editor/notion/TableControls.test.tsx
- src/components/notes/NoteList.tsx
- src/components/editor/notion/tableEdgeDrag.test.ts
- src/components/editor/notion/tablePerformance.test.ts
- src/components/editor/notion/tableNonRegression.test.ts
- src/components/editor/notion/selectionDecoration.test.ts
- src/components/layout/Sidebar.tsx
- src/components/editor/notion/markdownMarks.test.ts
- src/components/editor/notion/tableView.test.ts
- src/components/editor/notion/interfaceGeometry.ts
- src/App.css
- src/components/editor/notion/markdownDocument.test.ts
- src/lib/windowSession.ts
- src/components/editor/notion/markdownMarks.ts
- src/components/editor/notion/tableClipboard.ts
- src/components/editor/notion/TableControls.tsx
- src/components/editor/notion/tableRowResize.ts
- src/components/editor/notion/tablePointerDrag.test.ts
- src/components/editor/notion/NotionMenus.tsx
- src/components/editor/notion/NotionMenus.test.tsx
- src/components/preview/PreviewApp.tsx
- src/components/editor/Editor.tsx
- src/components/editor/notion/tableTransactions.ts
- src-tauri/src/lib.rs
| const selected = await open({ | ||
| directory: true, | ||
| multiple: false, | ||
| title: "Open Folder", | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Route folder selection through a Tauri command.
Lines 527-531 call the native dialog plugin directly from the frontend. Move folder selection into a command defined in src-tauri/src/lib.rs, then call that command with invoke() through a service wrapper.
As per coding guidelines, “All backend operations must go through Tauri commands defined in src-tauri/src/lib.rs.”
🤖 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/App.tsx` around lines 527 - 531, Update the folder-selection flow around
the App component’s open call to use a service wrapper that invokes a Tauri
command instead of calling the native dialog plugin directly. Define and
register the corresponding folder-selection command in src-tauri/src/lib.rs,
have it perform the existing directory-only, single-selection dialog behavior,
and update the frontend to consume the command result.
Source: Coding guidelines
| useEffect(() => { | ||
| let disposed = false; | ||
| let unlisten: (() => void) | undefined; | ||
| void listen("open-folder", async () => { | ||
| if (disposed) return; | ||
| try { | ||
| await openFolderInWorkspaceWindow(); | ||
| } catch (error) { | ||
| console.error("Failed to open folder:", error); | ||
| if (!disposed) toast.error("Failed to open folder"); | ||
| } | ||
| }).then((cleanup) => { | ||
| if (disposed) cleanup(); | ||
| else unlisten = cleanup; | ||
| }); | ||
| return () => { | ||
| disposed = true; | ||
| unlisten?.(); | ||
| }; | ||
| }, [openFolderInWorkspaceWindow]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Register open-folder in every window mode.
This listener exists only in AppContent. Lines 789-809 render Preferences and preview windows without AppContent, so their native open-folder event has no handler. Move this listener to App or a shared shell mounted by all window modes.
🤖 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/App.tsx` around lines 538 - 557, The open-folder listener currently lives
only in AppContent, so Preferences and preview windows lack a handler. Move the
useEffect containing listen("open-folder") and its cleanup/error handling from
AppContent into App or a shared shell component mounted by every window mode,
preserving its openFolderInWorkspaceWindow dependency and disposal behavior.
| function mountSettingsPage(onBack?: () => void) { | ||
| const container = document.createElement("div"); | ||
| document.body.append(container); | ||
| const root = createRoot(container); | ||
| act(() => | ||
| root.render( | ||
| <TooltipProvider> | ||
| <SettingsPage onBack={onBack} /> | ||
| </TooltipProvider>, | ||
| ), | ||
| ); | ||
| return { container, root }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'mountSettingsPage|root\.unmount|userAgent|addEventListener\("keydown"' \
src/components/settings/SettingsPage.test.tsx \
src/components/settings/SettingsPage.tsxRepository: erictli/scratch
Length of output: 3984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- SettingsPage.test.tsx relevant section ---\n'
sed -n '1,115p' src/components/settings/SettingsPage.test.tsx
printf '\n--- SettingsPage.tsx useEffect section ---\n'
sed -n '1,100p' src/components/settings/SettingsPage.tsx
printf '\n--- test framework cleanup hooks in settings tests ---\n'
rg -n 'afterEach|beforeEach|afterAll|beforeAll|test\.extend|expect\(' src/components/settings/SettingsPage.test.tsxRepository: erictli/scratch
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- SettingsPage.test.tsx relevant section ---'
sed -n '1,115p' src/components/settings/SettingsPage.test.tsx
printf '%s\n' ''
printf '%s\n' '--- SettingsPage.tsx useEffect section ---'
sed -n '1,100p' src/components/settings/SettingsPage.tsx
printf '%s\n' ''
printf '%s\n' '--- test framework cleanup hooks in settings tests ---'
rg -n 'afterEach|beforeEach|afterAll|beforeAll|test\.extend|expect\(' src/components/settings/SettingsPage.test.tsxRepository: erictli/scratch
Length of output: 7490
Unmount settings roots and restore the user agent in teardown.
SettingsPage registers a window keydown listener in useEffect and runs cleanup in the component unmount path. The first two tests only discard { container, root }, so their keydown listeners stay mounted. The Windows test unmounts only after assertions and restores navigator.userAgent only after assertions; render or assertion failures can leak modified DOM or the Windows userAgent into later tests. Track mounted roots in afterEach and restore the user agent from a saved value in finally.
🤖 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/settings/SettingsPage.test.tsx` around lines 30 - 41, Update
the SettingsPage test teardown around mountSettingsPage so every created root is
tracked and unmounted during afterEach, ensuring window keydown listeners are
cleaned up. Save the original navigator.userAgent before the Windows-specific
test and restore it in a finally block, so cleanup occurs even when rendering or
assertions fail.
| if (selectedNoteIdRef.current === id) { | ||
| selectedNoteIdRef.current = null; | ||
| currentNoteRef.current = null; | ||
| setSelectedNoteId(null); | ||
| setCurrentNote(null); | ||
| setNoteConflict(null); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear note synchronization state when deletion clears selection.
deleteNote leaves hasExternalChanges set. deleteFolderAction leaves both noteConflict and hasExternalChanges set. A previous conflict can remain active with no selected note.
src/context/NotesContext.tsx#L498-L504: ResethasExternalChangeswhen deleting the selected note.src/context/NotesContext.tsx#L625-L632: Clear the selected-note refs,noteConflict, andhasExternalChangeswith the selection state.
Proposed state cleanup
if (selectedNoteIdRef.current === id) {
selectedNoteIdRef.current = null;
currentNoteRef.current = null;
setSelectedNoteId(null);
setCurrentNote(null);
setNoteConflict(null);
+ setHasExternalChanges(false);
}
if (shouldClearSelection) {
+ selectedNoteIdRef.current = null;
+ currentNoteRef.current = null;
setCurrentNote(null);
setSelectedNoteId(null);
+ setNoteConflict(null);
+ setHasExternalChanges(false);
}📝 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.
| if (selectedNoteIdRef.current === id) { | |
| selectedNoteIdRef.current = null; | |
| currentNoteRef.current = null; | |
| setSelectedNoteId(null); | |
| setCurrentNote(null); | |
| setNoteConflict(null); | |
| } | |
| if (selectedNoteIdRef.current === id) { | |
| selectedNoteIdRef.current = null; | |
| currentNoteRef.current = null; | |
| setSelectedNoteId(null); | |
| setCurrentNote(null); | |
| setNoteConflict(null); | |
| setHasExternalChanges(false); | |
| } |
| if (selectedNoteIdRef.current === id) { | |
| selectedNoteIdRef.current = null; | |
| currentNoteRef.current = null; | |
| setSelectedNoteId(null); | |
| setCurrentNote(null); | |
| setNoteConflict(null); | |
| } | |
| let shouldClearSelection = false; | |
| if (selectedNoteIdRef.current && selectedNoteIdRef.current.startsWith(path + "/")) { | |
| shouldClearSelection = true; | |
| } | |
| if (shouldClearSelection) { | |
| selectedNoteIdRef.current = null; | |
| currentNoteRef.current = null; | |
| setCurrentNote(null); | |
| setSelectedNoteId(null); | |
| setNoteConflict(null); | |
| setHasExternalChanges(false); | |
| } |
📍 Affects 1 file
src/context/NotesContext.tsx#L498-L504(this comment)src/context/NotesContext.tsx#L625-L632
🤖 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/context/NotesContext.tsx` around lines 498 - 504, Reset
hasExternalChanges when deleteNote clears the selected note. In
deleteFolderAction, clear selectedNoteIdRef, currentNoteRef, noteConflict, and
hasExternalChanges together with the selection state; apply these changes at
src/context/NotesContext.tsx lines 498-504 and 625-632.
| const selectedId = selectedNoteIdRef.current; | ||
| if (selectedId && selectedId.startsWith(oldPrefix)) { | ||
| const newId = newPrefix + selectedId.substring(oldPrefix.length); | ||
| setSelectedNoteId(newId); | ||
| notesService.readNote(newId).then((note) => { | ||
| setCurrentNote(note); | ||
| }).catch((err) => { | ||
| setError(err instanceof Error ? err.message : "Failed to read renamed note"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard relocated-note reloads against a later selection.
Each path starts notesService.readNote(newId) and later calls setCurrentNote(note) without checking whether the user selected another note. A delayed relocation response can overwrite currentNote after selectedNoteId changes.
Reuse the request-token guard from selectNote, or use one shared guarded reload helper.
src/context/NotesContext.tsx#L664-L673: Guard the renamed-note reload before updatingcurrentNote.src/context/NotesContext.tsx#L696-L703: Guard the moved-note reload before updatingcurrentNote.src/context/NotesContext.tsx#L734-L743: Guard the moved-folder note reload before updatingcurrentNote.
📍 Affects 1 file
src/context/NotesContext.tsx#L664-L673(this comment)src/context/NotesContext.tsx#L696-L703src/context/NotesContext.tsx#L734-L743
🤖 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/context/NotesContext.tsx` around lines 664 - 673, Guard relocated-note
reload responses against stale selections before calling setCurrentNote. In
src/context/NotesContext.tsx lines 664-673, 696-703, and 734-743, reuse the
request-token guard from selectNote or a shared guarded reload helper so each
response updates currentNote only if the corresponding note selection is still
current.
| it("duplicates a note with a single atomic backend command", async () => { | ||
| invokeMock.mockResolvedValueOnce({ | ||
| id: "Original-Copy", | ||
| title: "Original (Copy)", | ||
| content: "# Original (Copy)\n\nContent.\n", | ||
| path: "/notes/Original-Copy.md", | ||
| modified: 1, | ||
| revision: "new-revision", | ||
| }); | ||
|
|
||
| const result = await duplicateNote("Original"); | ||
|
|
||
| expect(invokeMock).toHaveBeenCalledWith("duplicate_note", { | ||
| id: "Original", | ||
| }); | ||
| expect(result.id).toBe("Original-Copy"); | ||
| expect(result.title).toBe("Original (Copy)"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that duplication makes exactly one backend call.
The test proves that duplicate_note occurs. It does not prove that no additional read or write command occurs. Add expect(invokeMock).toHaveBeenCalledTimes(1) to enforce the stated atomic-operation contract.
🤖 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/services/notes.test.ts` around lines 169 - 186, Update the duplicateNote
test around the existing invokeMock assertion to verify exactly one backend
invocation. Add a call-count assertion after checking the duplicate_note
command, while preserving the existing result assertions.
8fa3d41 to
1c6bc10
Compare
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)
src/context/ThemeContext.tsx (1)
384-393: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd user-facing error feedback for failed settings persistence.
Most setters in this file update local state immediately, then call
updateGlobalSettings. If the call fails, the catch block only logs toconsole.error. The user sees no indication that the setting did not persist, and local state stays out of sync with the backend until the next full reload.
resetEditorFontSettings(Lines 468-500) already uses the better pattern: catch the error, showtoast.error, and callloadSettingsFromBackend()to resync. Apply the same pattern tosaveThemeSettings,saveFontSettings,setTextDirection,setEditorWidth,setCustomEditorWidthPx,setEditorWidthResizeEnabled,setEditorToolbarVisible,updateTitleBarNoteInfo,setSidebarWidthPx,setCustomColor,resetCustomColor, andresetAllCustomColors.As per path instructions,
src/**/*.{ts,tsx}: "Implement error handling with user-friendly messages."Also applies to: 440-450, 503-520, 546-575, 577-593, 613-630, 660-707
🤖 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/context/ThemeContext.tsx` around lines 384 - 393, Settings persistence failures currently only log errors, leaving users uninformed and local state unsynchronized. Update saveThemeSettings and the listed setters—saveFontSettings, setTextDirection, setEditorWidth, setCustomEditorWidthPx, setEditorWidthResizeEnabled, setEditorToolbarVisible, updateTitleBarNoteInfo, setSidebarWidthPx, setCustomColor, resetCustomColor, and resetAllCustomColors—to catch failures, display a user-friendly toast.error message, and call loadSettingsFromBackend() to resync state, following resetEditorFontSettings.Source: Path instructions
🧹 Nitpick comments (1)
src/context/ThemeContext.tsx (1)
536-543: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider debouncing interface zoom persistence.
This
useEffectpersistsinterfaceZoomto the backend on every state change.setInterfaceZoomis called during interactive zoom actions (Line 523-534), so rapid changes trigger a backend write for each step.As per path instructions,
src/**/*.{ts,tsx}: "Debounce user-triggered operations: auto-save 300ms, search 150ms, file watcher 500ms, git status 1000ms." Debounce this persistence call to avoid redundant backend writes during interactive zoom.🤖 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/context/ThemeContext.tsx` around lines 536 - 543, Debounce the interface zoom persistence in the useEffect watching interfaceZoom and isInitialized, delaying updateGlobalSettings({ interfaceZoom }) by 300ms so rapid interactive changes produce one backend write. Clear the pending timer on dependency changes or unmount, while preserving the existing initialization guard and error logging.Source: Path instructions
🤖 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/editor/notion/markdownDocument.ts`:
- Around line 10-11: Update TABLE_DELIMITER_PATTERN to match one-column
delimiter rows such as “| --- |” and “| :--- |” by allowing zero additional
delimiter cells, while still requiring a pipe delimiter through the pattern or
existing line.includes("|") validation used by isTableStart.
---
Outside diff comments:
In `@src/context/ThemeContext.tsx`:
- Around line 384-393: Settings persistence failures currently only log errors,
leaving users uninformed and local state unsynchronized. Update
saveThemeSettings and the listed setters—saveFontSettings, setTextDirection,
setEditorWidth, setCustomEditorWidthPx, setEditorWidthResizeEnabled,
setEditorToolbarVisible, updateTitleBarNoteInfo, setSidebarWidthPx,
setCustomColor, resetCustomColor, and resetAllCustomColors—to catch failures,
display a user-friendly toast.error message, and call loadSettingsFromBackend()
to resync state, following resetEditorFontSettings.
---
Nitpick comments:
In `@src/context/ThemeContext.tsx`:
- Around line 536-543: Debounce the interface zoom persistence in the useEffect
watching interfaceZoom and isInitialized, delaying updateGlobalSettings({
interfaceZoom }) by 300ms so rapid interactive changes produce one backend
write. Clear the pending timer on dependency changes or unmount, while
preserving the existing initialization guard and error logging.
🪄 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: f9ae27bf-af85-44b8-b8b0-b9173b395e90
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
package.jsonsrc-tauri/capabilities/default.jsonsrc-tauri/src/draft_checkpoint.rssrc-tauri/src/git.rssrc-tauri/src/hashing.rssrc-tauri/src/lib.rssrc-tauri/src/note_persistence_tests.rssrc-tauri/src/persistence.rssrc/App.csssrc/App.tsxsrc/components/editor/Editor.tsxsrc/components/editor/editorHistory.test.tssrc/components/editor/editorHistory.tssrc/components/editor/notion/NotionMenus.test.tsxsrc/components/editor/notion/NotionMenus.tsxsrc/components/editor/notion/TableControls.test.tsxsrc/components/editor/notion/TableControls.tsxsrc/components/editor/notion/interfaceGeometry.test.tssrc/components/editor/notion/interfaceGeometry.tssrc/components/editor/notion/markdownDocument.test.tssrc/components/editor/notion/markdownDocument.tssrc/components/editor/notion/markdownMarks.test.tssrc/components/editor/notion/markdownMarks.tssrc/components/editor/notion/selectionDecoration.test.tssrc/components/editor/notion/selectionDecoration.tssrc/components/editor/notion/tableAutoScroll.test.tssrc/components/editor/notion/tableAutoScroll.tssrc/components/editor/notion/tableClipboard.test.tssrc/components/editor/notion/tableClipboard.tssrc/components/editor/notion/tableEdgeDrag.test.tssrc/components/editor/notion/tableEdgeDrag.tssrc/components/editor/notion/tableExtensions.test.tssrc/components/editor/notion/tableExtensions.tssrc/components/editor/notion/tableIntegrity.test.tssrc/components/editor/notion/tableIntegrity.tssrc/components/editor/notion/tableMetadata.tssrc/components/editor/notion/tableNonRegression.test.tssrc/components/editor/notion/tablePerformance.test.tssrc/components/editor/notion/tablePointerDrag.test.tssrc/components/editor/notion/tablePointerDrag.tssrc/components/editor/notion/tableProximity.test.tssrc/components/editor/notion/tableProximity.tssrc/components/editor/notion/tableRowResize.test.tssrc/components/editor/notion/tableRowResize.tssrc/components/editor/notion/tableTransactions.test.tssrc/components/editor/notion/tableTransactions.tssrc/components/editor/notion/tableView.test.tssrc/components/editor/notion/tableView.tssrc/context/NotesContext.test.tsxsrc/context/NotesContext.tsxsrc/context/ThemeContext.tsx
💤 Files with no reviewable changes (1)
- src-tauri/capabilities/default.json
🚧 Files skipped from review as they are similar to previous changes (46)
- src/components/editor/notion/interfaceGeometry.ts
- src/components/editor/notion/tableRowResize.test.ts
- src/components/editor/notion/selectionDecoration.test.ts
- src/components/editor/notion/tableExtensions.test.ts
- src/components/editor/notion/NotionMenus.test.tsx
- src/components/editor/notion/tableAutoScroll.test.ts
- src/components/editor/notion/tableEdgeDrag.test.ts
- src/components/editor/notion/tableEdgeDrag.ts
- src/components/editor/notion/TableControls.test.tsx
- src/components/editor/notion/markdownDocument.test.ts
- src/components/editor/notion/interfaceGeometry.test.ts
- src/components/editor/notion/tableClipboard.test.ts
- src/components/editor/notion/tablePointerDrag.test.ts
- src/components/editor/notion/tableIntegrity.test.ts
- src/components/editor/notion/tableExtensions.ts
- src-tauri/src/hashing.rs
- src/components/editor/notion/tablePerformance.test.ts
- src/components/editor/notion/tableView.ts
- src/components/editor/notion/tableTransactions.test.ts
- src/components/editor/notion/tableProximity.ts
- src/components/editor/notion/tableIntegrity.ts
- src/components/editor/notion/markdownMarks.test.ts
- src/components/editor/notion/tableView.test.ts
- src/components/editor/notion/tableMetadata.ts
- src/components/editor/notion/selectionDecoration.ts
- src/context/NotesContext.test.tsx
- src/components/editor/notion/tablePointerDrag.ts
- src/App.css
- src/App.tsx
- src/components/editor/notion/tableRowResize.ts
- src-tauri/src/persistence.rs
- src/components/editor/notion/TableControls.tsx
- src/components/editor/notion/NotionMenus.tsx
- src/components/editor/notion/tableTransactions.ts
- src-tauri/src/note_persistence_tests.rs
- src-tauri/src/draft_checkpoint.rs
- src/components/editor/notion/tableClipboard.ts
- src/context/NotesContext.tsx
- src/components/editor/Editor.tsx
- package.json
- src/components/editor/editorHistory.test.ts
- src-tauri/src/lib.rs
- src/components/editor/notion/tableProximity.test.ts
- src/components/editor/notion/tableAutoScroll.ts
- src/components/editor/notion/markdownMarks.ts
- src/components/editor/notion/tableNonRegression.test.ts
1c6bc10 to
ac3d8a3
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src-tauri/src/lib.rs (2)
1297-1315: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake workspace binding and session removal atomic.
Both paths determine that a workspace has no bindings, release
workspace_bindings, and then remove its shared session. A concurrent window open or rebind can bind that workspace after the check but before removal. The new window then has a binding with no session, andworkspace_for_windowreturnsWorkspace session not found.
src-tauri/src/lib.rs#L1297-L1315: Coordinate old-workspace cleanup with binding creation atomically.src-tauri/src/lib.rs#L1360-L1389: Coordinate last-window cleanup with binding creation atomically.Use one lifecycle lock or one registry operation for binding changes and conditional session removal.
🤖 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-tauri/src/lib.rs` around lines 1297 - 1315, Make workspace binding changes and conditional session cleanup atomic in src-tauri/src/lib.rs:1297-1315 and src-tauri/src/lib.rs:1360-1389. Coordinate the binding creation/rebind logic with the no-bindings check and removal from workspace_sessions and workspace_initializers using a shared lifecycle lock or registry operation, so concurrent opens or rebinds cannot observe a removed session. Update both cleanup paths consistently.
4716-4729: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winMove the index rebuild out of the Tauri command flow.
rebuild_search_indexis synchronous and callsSearchIndex::rebuild_indexsynchronously, which performs filesystem traversal,std::fs::read_to_string, lock contention, andwriter.commit(). This can block command processing; make the command async and run the rebuild as blocking work while keeping thesearch_indexmutex scope as small as possible.🤖 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-tauri/src/lib.rs` around lines 4716 - 4729, The synchronous rebuild in rebuild_search_index blocks the Tauri command flow. Convert the command to async and execute SearchIndex::rebuild_index in blocking work, cloning or preparing the required folder and ignored_dirs beforehand; acquire the search_index mutex only inside that blocking operation and keep the guard scoped through the rebuild, then propagate its result and errors as before.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src-tauri/src/lib.rs`:
- Around line 1297-1315: Make workspace binding changes and conditional session
cleanup atomic in src-tauri/src/lib.rs:1297-1315 and
src-tauri/src/lib.rs:1360-1389. Coordinate the binding creation/rebind logic
with the no-bindings check and removal from workspace_sessions and
workspace_initializers using a shared lifecycle lock or registry operation, so
concurrent opens or rebinds cannot observe a removed session. Update both
cleanup paths consistently.
- Around line 4716-4729: The synchronous rebuild in rebuild_search_index blocks
the Tauri command flow. Convert the command to async and execute
SearchIndex::rebuild_index in blocking work, cloning or preparing the required
folder and ignored_dirs beforehand; acquire the search_index mutex only inside
that blocking operation and keep the guard scoped through the rebuild, then
propagate its result and errors as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f629796-11f6-40f5-82ef-89fdeb54eb94
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
package.jsonsrc-tauri/capabilities/default.jsonsrc-tauri/src/draft_checkpoint.rssrc-tauri/src/git.rssrc-tauri/src/hashing.rssrc-tauri/src/lib.rssrc-tauri/src/note_persistence_tests.rssrc-tauri/src/persistence.rssrc/App.csssrc/App.tsxsrc/components/editor/Editor.tsxsrc/components/editor/editorHistory.test.tssrc/components/editor/editorHistory.tssrc/components/editor/notion/NotionMenus.test.tsxsrc/components/editor/notion/NotionMenus.tsxsrc/components/editor/notion/TableControls.test.tsxsrc/components/editor/notion/TableControls.tsxsrc/components/editor/notion/interfaceGeometry.test.tssrc/components/editor/notion/interfaceGeometry.tssrc/components/editor/notion/markdownDocument.test.tssrc/components/editor/notion/markdownDocument.tssrc/components/editor/notion/markdownMarks.test.tssrc/components/editor/notion/markdownMarks.tssrc/components/editor/notion/selectionDecoration.test.tssrc/components/editor/notion/selectionDecoration.tssrc/components/editor/notion/tableAutoScroll.test.tssrc/components/editor/notion/tableAutoScroll.tssrc/components/editor/notion/tableClipboard.test.tssrc/components/editor/notion/tableClipboard.tssrc/components/editor/notion/tableEdgeDrag.test.tssrc/components/editor/notion/tableEdgeDrag.tssrc/components/editor/notion/tableExtensions.test.tssrc/components/editor/notion/tableExtensions.tssrc/components/editor/notion/tableIntegrity.test.tssrc/components/editor/notion/tableIntegrity.tssrc/components/editor/notion/tableMetadata.tssrc/components/editor/notion/tableNonRegression.test.tssrc/components/editor/notion/tablePerformance.test.tssrc/components/editor/notion/tablePointerDrag.test.tssrc/components/editor/notion/tablePointerDrag.tssrc/components/editor/notion/tableProximity.test.tssrc/components/editor/notion/tableProximity.tssrc/components/editor/notion/tableRowResize.test.tssrc/components/editor/notion/tableRowResize.tssrc/components/editor/notion/tableTransactions.test.tssrc/components/editor/notion/tableTransactions.tssrc/components/editor/notion/tableView.test.tssrc/components/editor/notion/tableView.tssrc/context/NotesContext.test.tsxsrc/context/NotesContext.tsxsrc/context/ThemeContext.tsx
💤 Files with no reviewable changes (1)
- src-tauri/capabilities/default.json
🚧 Files skipped from review as they are similar to previous changes (44)
- src/components/editor/notion/tableAutoScroll.test.ts
- src/components/editor/notion/tableExtensions.test.ts
- src/components/editor/notion/tableRowResize.test.ts
- src/context/NotesContext.test.tsx
- src/components/editor/editorHistory.test.ts
- package.json
- src/components/editor/notion/tableAutoScroll.ts
- src/components/editor/notion/interfaceGeometry.ts
- src/components/editor/notion/tableEdgeDrag.ts
- src/components/editor/notion/tablePerformance.test.ts
- src/components/editor/notion/interfaceGeometry.test.ts
- src/components/editor/notion/tablePointerDrag.ts
- src/components/editor/notion/tableEdgeDrag.test.ts
- src/components/editor/notion/selectionDecoration.test.ts
- src/components/editor/notion/tableProximity.test.ts
- src/components/editor/notion/tableRowResize.ts
- src/components/editor/notion/tableClipboard.test.ts
- src/components/editor/notion/tableView.ts
- src/components/editor/notion/tableMetadata.ts
- src/components/editor/notion/markdownMarks.test.ts
- src/components/editor/notion/tableIntegrity.ts
- src/components/editor/notion/NotionMenus.tsx
- src-tauri/src/git.rs
- src/components/editor/notion/tablePointerDrag.test.ts
- src/components/editor/notion/tableExtensions.ts
- src/components/editor/notion/tableProximity.ts
- src/components/editor/notion/markdownMarks.ts
- src/components/editor/notion/tableTransactions.test.ts
- src/components/editor/notion/markdownDocument.test.ts
- src/components/editor/notion/NotionMenus.test.tsx
- src/components/editor/notion/TableControls.test.tsx
- src/components/editor/notion/TableControls.tsx
- src/App.tsx
- src-tauri/src/note_persistence_tests.rs
- src-tauri/src/persistence.rs
- src/components/editor/notion/tableIntegrity.test.ts
- src/context/ThemeContext.tsx
- src/components/editor/notion/tableClipboard.ts
- src-tauri/src/draft_checkpoint.rs
- src/App.css
- src/context/NotesContext.tsx
- src/components/editor/Editor.tsx
- src/components/editor/notion/tableTransactions.ts
- src/components/editor/notion/selectionDecoration.ts
1 participant
Summary
/tablewith persisted column widths and row heights, fit-to-width metadata, row/column controls, resizing, edge auto-scroll, TSV clipboard support, integrity guards, and history-safe transactions3.29.2versionScope
This is PR 5 of the Scratch 1.0.1 compatibility series.
It intentionally excludes:
Those remain isolated for the following PRs.
Dependencies
This branch is stacked on the earlier compatibility work:
The PR targets
main; its diff will shrink as those dependencies merge.Validation
npm test -- --run: 50 test files, 257 tests passednpm run build: TypeScript and Vite production build passedcargo test --manifest-path src-tauri/Cargo.toml --quiet: 89 tests passedcargo clippy --manifest-path src-tauri/Cargo.toml --all-targets --all-features -- -D warnings: passednpm ls --all: dependency tree valid and TipTap/ProseMirror deduplicatedgit diff --check: passedPackaged macOS WKWebView validation:
a -> ma -> awithShift+Left/Shift+RightCmd+Shift+LeftandCmd+Shift+Rightselect the expected line boundariesAudit note
npm auditreports 5 existing toolchain findings: 2 low and 3 high. The affected versions of Babel, esbuild, picomatch, PostCSS, and Vite are identical to the parent PR4 lockfile; this PR adds no net-new audit finding.Summary by CodeRabbit
New Features
Bug Fixes
Tests