Improve standalone Markdown windows and Settings access - #200
Conversation
📝 WalkthroughWalkthroughChangesThe pull request adds revision-aware atomic persistence, durable draft checkpoints, conflict recovery, safe window closing, a dedicated Preferences window, editor display settings, sidebar sorting and folder collapsing, shared shortcuts, and Vitest test infrastructure. Workspace and persistence
Settings and windows
Sidebar and editor layout
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/editor/Editor.tsx (1)
855-880: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the auto-save debounce with the documented interval.
The WYSIWYG auto-save timer uses 500 ms. The source-mode handler at Line 2362 uses 300 ms. The coding guidelines require a 300 ms auto-save debounce. Use one constant for both paths.
As per coding guidelines: "Debounce user-triggered operations: auto-save 300ms, search 150ms, file watcher 500ms, git status 1000ms".
🤖 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 855 - 880, Update the auto-save debounce in the saveTimeoutRef timer within the editor save flow to use the shared 300 ms debounce constant, and reuse that same constant in the source-mode handler near the existing 300 ms timer. Ensure both auto-save paths remain aligned without introducing separate interval literals.Source: Coding guidelines
🧹 Nitpick comments (15)
src-tauri/src/lib.rs (3)
1894-1902: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDrop the unsynchronized pre-read before
save_if_revision.This block reads the file outside the per-path lock only to obtain a
ContentRevisionvalue.persistence::save_if_revisionrepeats the read under the lock and performs the authoritative comparison, so the pre-check adds one full file read per save and cannot change the outcome. It also means the conflict snapshot returned from this early branch can already be stale.Consider exposing a constructor such as
ContentRevision::from_hex(&str)inpersistence, then callingsave_if_revisiononce and mapping its typed result.🤖 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 1894 - 1902, Remove the unsynchronized persistence::read_snapshot pre-check and expected revision match. Add or reuse a persistence::ContentRevision constructor such as from_hex for expected_revision, call persistence::save_if_revision exactly once so comparison occurs under the per-path lock, and map its typed result into the existing FileSaveResult outcomes, including conflict snapshots.
2108-2131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlan retention for draft checkpoints.
Checkpoints are keyed by
(window_label, note_id)and removed only by an explicitclear_draft_checkpointfor that exact key. Preview window labels derive from a hash of the file path, so every distinct file that a user ever opens in a standalone window can leave one checkpoint file behind. A crash, a forced quit, or a window that never clears its key leaves the record permanently. Each record stores the full note text.
list_draft_checkpointsalso filters to the current window label, so checkpoints from labels that no longer exist are never surfaced and never cleaned. Consider adding an age-based sweep at startup, or aprune_draft_checkpointscommand that drops records older than a fixed window.Also applies to: 2169-2187
🤖 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 2108 - 2131, Implement age-based retention for draft checkpoints by adding a startup sweep or a prune_draft_checkpoints command that removes records older than a fixed retention window, including checkpoints whose window labels are no longer active. Integrate the cleanup with the existing draft_checkpoint storage flow near write_draft_checkpoint and list_draft_checkpoints, while preserving explicit key-based clearing behavior.
3939-3943: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid indexing
app.config().app.windows[0].
windows[0]panics if the window list intauri.conf.jsonis empty. A panic inside theopen_preferences_windowcommand aborts the process instead of returning a user-facing error. The same indexing is now used at Line 3998 for the preview window. Use.first()and map the absence to an error.🛡️ Proposed fix
- let runtime_config = runtime_window_config_from_template( - &app.config().app.windows[0], - "preferences", - WebviewUrl::App("index.html?mode=preferences".into()), - ); + let template = app + .config() + .app + .windows + .first() + .cloned() + .ok_or_else(|| "No window template is configured".to_string())?; + let runtime_config = runtime_window_config_from_template( + &template, + "preferences", + WebviewUrl::App("index.html?mode=preferences".into()), + );🤖 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 3939 - 3943, Update the window configuration lookups in open_preferences_window, including the preferences and preview window paths near runtime_config, to use first() instead of indexing windows[0]. Convert a missing window configuration into the command’s existing user-facing error result rather than allowing a panic.src-tauri/src/persistence.rs (1)
270-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the duplicated SHA-256 and atomic file helpers into a shared module.
src-tauri/src/persistence.rsandsrc-tauri/src/draft_checkpoint.rsboth definesha256,sha256_compress,hex_sha256,TemporaryPath,create_temporary_file,sync_parent_directory, and atomic-write helpers, but onlypersistence.rsincludes tests for the SHA-256 behavior.Prefer
sha2::Sha256over the hand-written hash if dependencies are allowed. Otherwise, centralize the existing helpers in one module and delete the duplicated code fromdraft_checkpoint.rs, keeping Windows replace-file behavior if needed.🤖 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/persistence.rs` around lines 270 - 362, In src-tauri/src/persistence.rs lines 270-362 and src-tauri/src/draft_checkpoint.rs lines 314-410, move the shared sha256, sha256_compress, hex_sha256, TemporaryPath, create_temporary_file, sync_parent_directory, and atomic-write helpers into one shared module, then update both callers to use it and remove duplicate definitions. Prefer sha2::Sha256 if the dependency is available; otherwise retain the existing implementation and its SHA-256 tests, preserving Windows replace-file behavior.src/context/ThemeContext.tsx (1)
547-598: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface a user-facing message when the new setters fail to persist.
setEditorWidthResizeEnabled,setEditorToolbarVisible, andupdateTitleBarNoteInfoonly callconsole.errorwhenupdateSettingsPatchfails. The user does not see any indication that their preference change was not saved.As per coding guidelines,
src/**/*.{ts,tsx}must "Implement error handling with user-friendly messages." Surface a toast or inline error (the app already renders<Toaster />at the root) instead of only logging to the console.🤖 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 547 - 598, Update setEditorWidthResizeEnabled, setEditorToolbarVisible, and updateTitleBarNoteInfo so failures from updateSettingsPatch also trigger the app’s existing user-facing toast mechanism, using a clear, friendly message for the relevant preference; retain console.error logging if consistent with surrounding error handling and preserve the current state updates.Source: Coding guidelines
src/services/draftCheckpoint.ts (1)
23-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the parameter to
noteIdbecausewindowLabelis ignored.
clearDraftCheckpointaccepts a fullDraftCheckpointKeybut forwards onlynoteId. The backend derives the window identity. The current signature invites callers to pass a meaninglesswindowLabel;src/components/preview/PreviewApp.tsxat line 269 already passeswindowLabel: "". AcceptnoteId: stringto make the contract explicit, and align it withgetDraftCheckpoint.♻️ Proposed change
-export async function clearDraftCheckpoint( - key: DraftCheckpointKey, -): Promise<void> { - return invoke("clear_draft_checkpoint", { noteId: key.noteId }); +export async function clearDraftCheckpoint(noteId: string): Promise<void> { + return invoke("clear_draft_checkpoint", { noteId }); }Update the callers, for example in
src/components/preview/PreviewApp.tsx:await draftCheckpointService.clearDraftCheckpoint(filePath);🤖 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/draftCheckpoint.ts` around lines 23 - 27, Change clearDraftCheckpoint to accept a noteId: string instead of DraftCheckpointKey, while continuing to invoke clear_draft_checkpoint with that noteId. Update all callers, including PreviewApp, to pass the note ID directly and remove unused windowLabel object arguments, matching the getDraftCheckpoint contract.Source: Coding guidelines
src/lib/standaloneRecreation.ts (1)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider preserving the concurrent file state in the thrown error.
The conflict branch discards
result.current, which holds the content and revision written by the other writer. The caller insrc/components/preview/PreviewApp.tsxat line 250 can therefore only show a generic message and cannot offer a follow-up merge or accept action. Attach the current state to the error so the conflict UI can use it.♻️ Proposed change
+export class StandaloneRecreationConflictError extends Error { + constructor( + readonly current: { content: string; revision: string } | null, + ) { + super("The source path was recreated elsewhere; conflict preserved"); + this.name = "StandaloneRecreationConflictError"; + } +} + export async function recreateDeletedStandaloneDraft( path: string, content: string, recreateFile: RecreateFile, ): Promise<FileContent> { const result = await recreateFile(path, content); if (result.status === "conflict") { - throw new Error( - "The source path was recreated elsewhere; conflict preserved", - ); + throw new StandaloneRecreationConflictError(result.current); } return result.file; }🤖 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/standaloneRecreation.ts` around lines 14 - 18, Update the conflict branch in standalone recreation to attach result.current, including the concurrent content and revision, to the thrown error. Preserve the existing conflict message while exposing the current state so PreviewApp can render merge or accept actions.src/components/editor/Editor.tsx (4)
944-960: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssign
queueCheckpointCaptureRef.currentinside an effect.Line 944 writes a ref during render. React can discard a render, so the ref can hold a closure from work that never committed. Move the assignment into an effect that depends on
persistCurrentCrashCheckpoint. The lazy scheduler initialization at Line 645 is a supported pattern and needs no change.♻️ Proposed change
- queueCheckpointCaptureRef.current = () => { - const now = Date.now(); - checkpointCaptureStartedAtRef.current ??= now; - if (checkpointCaptureTimerRef.current) { - clearTimeout(checkpointCaptureTimerRef.current); - } - const delay = nextCheckpointCaptureDelay( - now - checkpointCaptureStartedAtRef.current, - 250, - 750, - ); - checkpointCaptureTimerRef.current = window.setTimeout(() => { - checkpointCaptureTimerRef.current = null; - checkpointCaptureStartedAtRef.current = null; - void persistCurrentCrashCheckpoint(); - }, delay); - }; + useEffect(() => { + queueCheckpointCaptureRef.current = () => { + const now = Date.now(); + checkpointCaptureStartedAtRef.current ??= now; + if (checkpointCaptureTimerRef.current) { + clearTimeout(checkpointCaptureTimerRef.current); + } + const delay = nextCheckpointCaptureDelay( + now - checkpointCaptureStartedAtRef.current, + 250, + 750, + ); + checkpointCaptureTimerRef.current = window.setTimeout(() => { + checkpointCaptureTimerRef.current = null; + checkpointCaptureStartedAtRef.current = null; + void persistCurrentCrashCheckpoint(); + }, delay); + }; + }, [persistCurrentCrashCheckpoint]);🤖 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 944 - 960, Move the assignment to queueCheckpointCaptureRef.current into an effect dependent on persistCurrentCrashCheckpoint, keeping the existing debounce and timer behavior unchanged. Do not modify the lazy scheduler initialization or other checkpoint logic.Source: Linters/SAST tools
1794-1805: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDispose the checkpoint scheduler on unmount.
The cleanup clears the capture timer but never calls
checkpointScheduler.dispose().dispose()insrc/lib/draftCheckpoint.tsflushes the pending checkpoint. Without it, a checkpoint marked within the last debounce window is discarded when the editor unmounts. React cleanup cannot await the promise, so calldispose()and report failures throughonError.♻️ Proposed change
if (checkpointCaptureTimerRef.current) { clearTimeout(checkpointCaptureTimerRef.current); checkpointCaptureStartedAtRef.current = null; } + void checkpointScheduler.dispose().catch((error) => { + console.error("Failed to flush crash checkpoint on unmount:", 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/components/editor/Editor.tsx` around lines 1794 - 1805, Update the useEffect cleanup in Editor.tsx to call checkpointScheduler.dispose() during unmount, ensuring pending checkpoints are flushed; handle its asynchronous failure without awaiting React cleanup by routing the rejection through onError.
962-975: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoute the visibility flush through the scheduler API.
This effect calls
persistCurrentCrashCheckpointdirectly.checkpointScheduler.handleVisibilityChangeinsrc/lib/draftCheckpoint.tsexists for this case and has test coverage, but no caller. Either call it here aftermarkDirty, or remove it from the scheduler 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/components/editor/Editor.tsx` around lines 962 - 975, Update the visibilitychange effect in Editor.tsx to route the flush through checkpointScheduler.handleVisibilityChange instead of calling persistCurrentCrashCheckpoint directly; ensure the checkpoint is marked dirty before invoking the scheduler API, and preserve the existing timer cleanup and listener lifecycle.
595-604: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMemoize
titleBarNoteInfo.
Editorre-renders on every selection change because ofsetSelectionKey.getTitleBarNoteInfoTextthen callsformatDateTimeagain on each render. Wrap the computation inuseMemokeyed on the visibility flags and the note fields.As per coding guidelines: "Use
useCallbackanduseMemofor 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/components/editor/Editor.tsx` around lines 595 - 604, Memoize the titleBarNoteInfo computation in Editor using useMemo, including currentNote, titleBarModifiedDateVisible, titleBarFilenameVisible, and formatDateTime-related note fields in its dependencies so selection-only re-renders reuse the cached value while changes to visibility or note data recompute it.Source: Coding guidelines
src/lib/draftCheckpoint.ts (1)
176-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the type import to the top of the file.
The
Notetype import sits after every declaration. TypeScript hoists it, so the code compiles, but the placement hides the dependency and breaks the file convention.♻️ Proposed move
At the top of the file:
+import type { Note } from "../types/note"; + export interface DraftCheckpointKey {At the end 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 Note type import to the top import section of draftCheckpoint.ts, before all declarations, while preserving its type-only import form and removing the trailing import.src/lib/conflictResolution.test.ts (1)
61-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a clean draft.
runConflictResolutionskipspersistRecoverywhendraft.dirtyis false. No test covers that branch. Add one case that assertspersistRecoveryis not called and the strategy action still runs.💚 Proposed additional test
+ it("skips recovery for a clean draft", async () => { + const persistRecovery = vi.fn(async () => "/recovery/Plan.md"); + const acceptRemote = vi.fn(async () => undefined); + + await runConflictResolution( + "useRemote", + { draft: { content: remote.content, dirty: false }, remote }, + { + persistRecovery, + overwriteRemote: async () => undefined, + recreateDeleted: async () => undefined, + acceptRemote, + }, + ); + + expect(persistRecovery).not.toHaveBeenCalled(); + expect(acceptRemote).toHaveBeenCalledWith(remote); + });🤖 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/conflictResolution.test.ts` around lines 61 - 74, Add a clean-draft test alongside the existing dirty-draft case for runConflictResolution, setting draft.dirty to false and mocking persistRecovery. Assert persistRecovery is not called, and verify the selected strategy action, such as overwriteRemote, still executes successfully.src/lib/windowCloseCallsites.test.ts (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBroaden the forbidden close patterns.
The assertions match only the literal identifier
appWindow.src/App.tsxcloses the window withgetCurrentWindow().close()at the Cmd/Ctrl+W handler, so a future switch togetCurrentWindow().destroy()would pass this test. Match.destroy(instead of theappWindow-prefixed form to make the guard resistant to renamed locals.♻️ Proposed change
expect(source).toContain("closeWindowAfterSave"); - expect(source).not.toContain("appWindow.close()"); - expect(source).not.toContain("appWindow.destroy()"); + expect(source).not.toMatch(/\.destroy\s*\(/);🤖 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/windowCloseCallsites.test.ts` around lines 16 - 18, Update the forbidden-pattern assertions in the window-close source test to reject any `.destroy(` invocation, not only `appWindow.destroy()`, while preserving the existing `appWindow.close()` guard and `closeWindowAfterSave` requirement.src/lib/useWindowShortcuts.ts (1)
14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the ref writes out of the render body.
Lines 16-17 write
ref.currentduring render. React can discard or replay a render, so the write can leak from UI that never commits. The same pattern exists elsewhere in this repository, so this is not a new defect. An effect keeps render pure and satisfies theno-ref-current-in-renderrule.♻️ Proposed change
const interfaceZoomRef = useRef(interfaceZoom); const openPreferencesRef = useRef(onOpenPreferences); - interfaceZoomRef.current = interfaceZoom; - openPreferencesRef.current = onOpenPreferences; + useEffect(() => { + interfaceZoomRef.current = interfaceZoom; + openPreferencesRef.current = onOpenPreferences; + }, [interfaceZoom, onOpenPreferences]);🤖 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 14 - 17, Move the assignments to interfaceZoomRef.current and openPreferencesRef.current out of the render body in useWindowShortcuts and into an appropriate effect, preserving the refs’ latest values while keeping render pure and satisfying no-ref-current-in-render.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/draft_checkpoint.rs`:
- Around line 120-128: Update list_checkpoints so individual checkpoint entries
that fail path access, file reading, JSON deserialization, or
ensure_identity_matches are skipped rather than propagated as errors. Continue
collecting valid checkpoints and preserve the existing extension filtering and
directory-iteration behavior.
In `@src-tauri/src/lib.rs`:
- Around line 3867-3881: Update the menu-building logic around the
application_menu_name lookup so the Preferences item is inserted into the
default Windows/Linux submenu rather than only a product-name submenu. Use
platform-aware selection of the appropriate File or Help menu, while preserving
the existing macOS product-name behavior and the CmdOrCtrl+, menu path.
In `@src-tauri/src/persistence.rs`:
- Around line 184-203: Update atomic_create_new to retry destination creation
with OpenOptions write/create_new after fs::hard_link returns Unsupported or
PermissionDenied, preserving the existing create-only behavior; propagate other
hard-link errors unchanged and retain temporary-file cleanup and
parent-directory syncing.
In `@src/App.tsx`:
- Around line 110-132: Update the close handling around runSafeWindowClose in
App.tsx so its resolved { recoveredTo, saveError } result is processed on every
close path. When saveError is present, persist a recovery notice using the
existing startup-message mechanism and include recoveredTo; retain rejection
handling for failures without a recovery result. Apply the equivalent result
handling in PreviewApp.tsx, and do not rely on a transient Sonner toast that
disappears when the window closes.
In `@src/components/editor/Editor.tsx`:
- Around line 2495-2501: Remove the role="status" attribute from the conflict
trigger button near the “Resolve Conflict” label, preserving the button’s native
semantics. Leave the separate non-interactive status indicator unchanged.
In `@src/components/layout/Sidebar.tsx`:
- Around line 185-209: The handleNoteSortOrderChange callback currently performs
a stale read-modify-write through getSettings and updateSettings. Replace this
flow with the supported atomic sidebarSortOrder update mechanism, passing only
the changed field if updateSettings preserves other settings, while retaining
the existing optimistic state update and rollback/error behavior.
In `@src/components/layout/SidebarFolderSection.tsx`:
- Around line 51-71: Update the disclosure content rendering in the component
containing the Folders button so the wrapper with id={contentId} is always
present in the DOM, while toggling its visibility based on expanded instead of
conditionally unmounting it. Preserve the existing children and aria-expanded
behavior so aria-controls consistently references an existing element.
In `@src/components/preview/PreviewApp.tsx`:
- Around line 103-109: Update the file-loading useEffect around readFileDirect
and getDraftCheckpoint to track disposal with a cancellation flag and return
cleanup that sets it when filePath changes or the component unmounts. Before
writing loaded content, revision state, or clearing the checkpoint, check that
the effect is still active so stale asynchronous results cannot update state.
- Around line 110-127: Update the revision assignment in the recovery flow
around revisionRef.current and setRevision so recovered checkpoint content uses
checkpoint.metadata.baseRevision as its save base, while unchanged or absent
checkpoints continue using result.revision. Ensure both revision state values
remain consistent so the next save detects external changes through the backend
instead of overwriting them.
- Around line 91-94: Handle rejection of the promise returned by
appWindow.onCloseRequested in the disposed/unlisten registration flow by adding
a catch handler that logs the registration failure and informs the user,
preventing an unhandled rejection and preserving draft-flush feedback.
In `@src/context/ThemeContext.tsx`:
- Around line 235-238: Serialize the read-merge-write operations in
updateSettingsPatch by routing each invocation through a shared promise queue,
so the next call waits for the previous getSettings and updateSettings sequence
to finish. Ensure the queue continues processing after a failed update, and
preserve the existing patch merge behavior for all callers including
setEditorWidthResizeEnabled, setEditorToolbarVisible, and
updateTitleBarNoteInfo.
In `@src/lib/windowShortcutCallsites.test.ts`:
- Around line 24-31: Validate that preferencesStart and preferencesEnd are found
before calling source.slice in the test, asserting each marker index is
non-negative. Keep the existing PreferencesApp and App marker lookup and
subsequent assertions unchanged so renames fail explicitly rather than producing
an empty slice.
---
Outside diff comments:
In `@src/components/editor/Editor.tsx`:
- Around line 855-880: Update the auto-save debounce in the saveTimeoutRef timer
within the editor save flow to use the shared 300 ms debounce constant, and
reuse that same constant in the source-mode handler near the existing 300 ms
timer. Ensure both auto-save paths remain aligned without introducing separate
interval literals.
---
Nitpick comments:
In `@src-tauri/src/lib.rs`:
- Around line 1894-1902: Remove the unsynchronized persistence::read_snapshot
pre-check and expected revision match. Add or reuse a
persistence::ContentRevision constructor such as from_hex for expected_revision,
call persistence::save_if_revision exactly once so comparison occurs under the
per-path lock, and map its typed result into the existing FileSaveResult
outcomes, including conflict snapshots.
- Around line 2108-2131: Implement age-based retention for draft checkpoints by
adding a startup sweep or a prune_draft_checkpoints command that removes records
older than a fixed retention window, including checkpoints whose window labels
are no longer active. Integrate the cleanup with the existing draft_checkpoint
storage flow near write_draft_checkpoint and list_draft_checkpoints, while
preserving explicit key-based clearing behavior.
- Around line 3939-3943: Update the window configuration lookups in
open_preferences_window, including the preferences and preview window paths near
runtime_config, to use first() instead of indexing windows[0]. Convert a missing
window configuration into the command’s existing user-facing error result rather
than allowing a panic.
In `@src-tauri/src/persistence.rs`:
- Around line 270-362: In src-tauri/src/persistence.rs lines 270-362 and
src-tauri/src/draft_checkpoint.rs lines 314-410, move the shared sha256,
sha256_compress, hex_sha256, TemporaryPath, create_temporary_file,
sync_parent_directory, and atomic-write helpers into one shared module, then
update both callers to use it and remove duplicate definitions. Prefer
sha2::Sha256 if the dependency is available; otherwise retain the existing
implementation and its SHA-256 tests, preserving Windows replace-file behavior.
In `@src/components/editor/Editor.tsx`:
- Around line 944-960: Move the assignment to queueCheckpointCaptureRef.current
into an effect dependent on persistCurrentCrashCheckpoint, keeping the existing
debounce and timer behavior unchanged. Do not modify the lazy scheduler
initialization or other checkpoint logic.
- Around line 1794-1805: Update the useEffect cleanup in Editor.tsx to call
checkpointScheduler.dispose() during unmount, ensuring pending checkpoints are
flushed; handle its asynchronous failure without awaiting React cleanup by
routing the rejection through onError.
- Around line 962-975: Update the visibilitychange effect in Editor.tsx to route
the flush through checkpointScheduler.handleVisibilityChange instead of calling
persistCurrentCrashCheckpoint directly; ensure the checkpoint is marked dirty
before invoking the scheduler API, and preserve the existing timer cleanup and
listener lifecycle.
- Around line 595-604: Memoize the titleBarNoteInfo computation in Editor using
useMemo, including currentNote, titleBarModifiedDateVisible,
titleBarFilenameVisible, and formatDateTime-related note fields in its
dependencies so selection-only re-renders reuse the cached value while changes
to visibility or note data recompute it.
In `@src/context/ThemeContext.tsx`:
- Around line 547-598: Update setEditorWidthResizeEnabled,
setEditorToolbarVisible, and updateTitleBarNoteInfo so failures from
updateSettingsPatch also trigger the app’s existing user-facing toast mechanism,
using a clear, friendly message for the relevant preference; retain
console.error logging if consistent with surrounding error handling and preserve
the current state updates.
In `@src/lib/conflictResolution.test.ts`:
- Around line 61-74: Add a clean-draft test alongside the existing dirty-draft
case for runConflictResolution, setting draft.dirty to false and mocking
persistRecovery. Assert persistRecovery is not called, and verify the selected
strategy action, such as overwriteRemote, still executes successfully.
In `@src/lib/draftCheckpoint.ts`:
- Line 176: Move the Note type import to the top import section of
draftCheckpoint.ts, before all declarations, while preserving its type-only
import form and removing the trailing import.
In `@src/lib/standaloneRecreation.ts`:
- Around line 14-18: Update the conflict branch in standalone recreation to
attach result.current, including the concurrent content and revision, to the
thrown error. Preserve the existing conflict message while exposing the current
state so PreviewApp can render merge or accept actions.
In `@src/lib/useWindowShortcuts.ts`:
- Around line 14-17: Move the assignments to interfaceZoomRef.current and
openPreferencesRef.current out of the render body in useWindowShortcuts and into
an appropriate effect, preserving the refs’ latest values while keeping render
pure and satisfying no-ref-current-in-render.
In `@src/lib/windowCloseCallsites.test.ts`:
- Around line 16-18: Update the forbidden-pattern assertions in the window-close
source test to reject any `.destroy(` invocation, not only
`appWindow.destroy()`, while preserving the existing `appWindow.close()` guard
and `closeWindowAfterSave` requirement.
In `@src/services/draftCheckpoint.ts`:
- Around line 23-27: Change clearDraftCheckpoint to accept a noteId: string
instead of DraftCheckpointKey, while continuing to invoke clear_draft_checkpoint
with that noteId. Update all callers, including PreviewApp, to pass the note ID
directly and remove unused windowLabel object arguments, matching the
getDraftCheckpoint contract.
🪄 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: d5169c9b-67d3-49f6-9aa8-df86ce4a1f28
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (57)
package.jsonsrc-tauri/capabilities/default.jsonsrc-tauri/src/draft_checkpoint.rssrc-tauri/src/lib.rssrc-tauri/src/persistence.rssrc/App.tsxsrc/components/editor/Editor.tsxsrc/components/editor/EditorWidthHandle.test.tsxsrc/components/editor/EditorWidthHandle.tsxsrc/components/layout/Sidebar.tsxsrc/components/layout/SidebarControls.test.tsxsrc/components/layout/SidebarControls.tsxsrc/components/layout/SidebarFolderSection.test.tsxsrc/components/layout/SidebarFolderSection.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/ThemeContext.tsxsrc/lib/conflictResolution.test.tssrc/lib/conflictResolution.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/serializedWriter.test.tssrc/lib/serializedWriter.tssrc/lib/standaloneRecreation.test.tssrc/lib/standaloneRecreation.tssrc/lib/standaloneReload.test.tssrc/lib/standaloneReload.tssrc/lib/titleBarNoteInfo.test.tssrc/lib/titleBarNoteInfo.tssrc/lib/useWindowShortcuts.tssrc/lib/windowClose.test.tssrc/lib/windowClose.tssrc/lib/windowCloseCallsites.test.tssrc/lib/windowShortcutCallsites.test.tssrc/lib/windowShortcuts.test.tssrc/lib/windowShortcuts.tssrc/services/draftCheckpoint.test.tssrc/services/draftCheckpoint.tssrc/services/files.test.tssrc/services/files.tssrc/services/notes.tssrc/services/windowLifecycle.test.tssrc/services/windowLifecycle.tssrc/types/note.tsvitest.config.ts
| const updateSettingsPatch = useCallback(async (patch: Partial<Settings>) => { | ||
| const settings = await getSettings(); | ||
| await updateSettings({ ...settings, ...patch }); | ||
| }, []); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize updateSettingsPatch writes to prevent lost settings updates.
updateSettingsPatch reads settings with getSettings(), then writes the merged result with updateSettings(). If two calls overlap (for example, a user quickly toggles two preferences that each call this helper, such as setEditorWidthResizeEnabled and setEditorToolbarVisible), both calls can read the same stale settings object. The second write then persists over the first call's patch, and the first change is silently lost.
The reset flow already recognizes this hazard: resetEditorFontSettings documents a "single atomic save to avoid race conditions." Extend that same safeguard to updateSettingsPatch, since it now backs three separate setters (setEditorWidthResizeEnabled, setEditorToolbarVisible, updateTitleBarNoteInfo) that can each fire independently.
🔒 Proposed fix: serialize writes through a queue
+ const settingsWriteQueueRef = useRef(Promise.resolve());
const updateSettingsPatch = useCallback(async (patch: Partial<Settings>) => {
- const settings = await getSettings();
- await updateSettings({ ...settings, ...patch });
+ const next = settingsWriteQueueRef.current.then(async () => {
+ const settings = await getSettings();
+ await updateSettings({ ...settings, ...patch });
+ });
+ settingsWriteQueueRef.current = next.catch(() => {});
+ return next;
}, []);📝 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.
| const updateSettingsPatch = useCallback(async (patch: Partial<Settings>) => { | |
| const settings = await getSettings(); | |
| await updateSettings({ ...settings, ...patch }); | |
| }, []); | |
| const settingsWriteQueueRef = useRef(Promise.resolve()); | |
| const updateSettingsPatch = useCallback(async (patch: Partial<Settings>) => { | |
| const next = settingsWriteQueueRef.current.then(async () => { | |
| const settings = await getSettings(); | |
| await updateSettings({ ...settings, ...patch }); | |
| }); | |
| settingsWriteQueueRef.current = next.catch(() => {}); | |
| return next; | |
| }, []); |
🤖 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 235 - 238, Serialize the
read-merge-write operations in updateSettingsPatch by routing each invocation
through a shared promise queue, so the next call waits for the previous
getSettings and updateSettings sequence to finish. Ensure the queue continues
processing after a failed update, and preserve the existing patch merge behavior
for all callers including setEditorWidthResizeEnabled, setEditorToolbarVisible,
and updateTitleBarNoteInfo.
1cb30a5 to
5e0494e
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/context/ThemeContext.tsx (2)
468-499: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd the missing
try {inresetEditorFontSettings.The
catchblock at Line 494 has no matchingtry. The function body goes straight from the state setters toawait updateGlobalSettings({...})at Line 480, then to} catch (error) {. The file does not parse. Biome reports parse errors at Lines 494-499 for this reason.🐛 Proposed fix: open the `try` block before the persist call
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, In resetEditorFontSettings, add the missing try block before the await updateGlobalSettings persistence call so the existing catch handler correctly matches it. Keep the current state updates, error logging, toast, and loadSettingsFromBackend recovery behavior unchanged.Source: Linters/SAST tools
719-723: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMove the
useMemocall above the early return.Line 719 returns
nullwhenisInitializedis false. Line 723 then callsuseMemo. React skips that hook on the first render and calls it on the second render, afterisInitializedbecomes true. The hook count changes between renders, and React throws "Rendered more hooks than during the previous render". The provider crashed path is reached on every mount, becauseisInitializedalways starts asfalse.The previous inline context object was not a hook, so the early return was safe. Introducing
useMemorequires moving the guard after all hook calls.🐛 Proposed fix: compute the memo first, then guard
- // Don't render until initialized to prevent flash - if (!isInitialized) { - return null; - } - const contextValue = useMemo<ThemeContextType>(Then place the guard directly before the
return:+ // Don't render until initialized to prevent flash + if (!isInitialized) { + return null; + } + return ( <ThemeContext.Provider value={contextValue}>🤖 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 719 - 723, Move the useMemo call that creates contextValue above the isInitialized early-return guard in the theme provider, then keep the guard immediately before the final return. Ensure all hooks execute on every render while preserving the existing null result until initialization completes.src/components/notes/NoteList.tsx (1)
326-329: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not re-sort search results by modified date.
displayItemsholds backend search results when a query is active (Lines 314-324). Those results arrive ranked by relevance score.sortedDisplayItemsthen re-sorts every item by modified date, so the ranking is discarded and the flat list at Line 428 renders search hits in date order. Before this change the flat list rendereddisplayItemsdirectly and preserved relevance order.Apply the sidebar sort order only when no search is active.
🐛 Proposed fix
const sortedDisplayItems = useMemo( - () => sortNotesByModified(displayItems, sortOrder), - [displayItems, sortOrder], + () => + searchQuery.trim() + ? displayItems + : sortNotesByModified(displayItems, sortOrder), + [displayItems, searchQuery, sortOrder], );🤖 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/notes/NoteList.tsx` around lines 326 - 329, Update the sortedDisplayItems useMemo in NoteList so sortNotesByModified is applied only when no search query is active; while searching, return displayItems unchanged to preserve backend relevance ranking, and retain the existing sortOrder behavior for non-search results.
🧹 Nitpick comments (11)
src/services/notes.test.ts (2)
147-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
duplicateNotetest out of thescoped settings writesblock.The block name describes settings writes. The test at lines 169-186 verifies the
duplicate_notecommand, which is not a settings write. Place it in its owndescribeblock so the suite name matches the behavior under test.Also applies to: 169-186
🤖 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` at line 147, Move the duplicateNote test covering the duplicate_note command out of the “scoped settings writes” describe block, and place it in a separate describe block named for duplicate-note behavior. Keep the test logic unchanged and ensure the surrounding suite names accurately match the behaviors they contain.
115-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the negative assertions to lock the single-command contract.
Each
not.toHaveBeenCalledWithassertion pairs a command name with one exact payload. The assertion passes if the service calls the forbidden command with any other payload. It also passes if the service calls a third, unexpected command.Assert the call count instead. Each of these services must issue exactly one command.
♻️ Proposed assertion change (shown for `openWorkspaceWindow`)
expect(invokeMock).toHaveBeenCalledWith("open_workspace_window", { path: "/notes/client", }); - expect(invokeMock).not.toHaveBeenCalledWith("set_notes_folder", { - path: "/notes/client", - }); + expect(invokeMock).toHaveBeenCalledTimes(1);Also applies to: 128-130, 141-143
🤖 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 115 - 117, Strengthen the negative assertions in the affected service tests, including the cases around invokeMock at the shown locations, to verify exactly one command call rather than only excluding one command/payload pair. Assert invokeMock’s total call count is one while preserving the existing expected command assertion, covering openWorkspaceWindow and the other noted cases.src/components/layout/WorkspaceMenu.tsx (1)
40-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove focus into the menu when it opens, and close it when focus leaves.
The component restores focus on Escape, which is correct. Two gaps remain for keyboard users:
- When
openbecomestrue, focus stays on the trigger. A keyboard user must Tab through the trigger before reaching the first workspace item.- The effect only listens for
pointerdown. If the user Tabs past the last menu item, the menu stays open while focus sits outside it.Add a focus-in effect and a
focusoutdismissal.♻️ Proposed focus handling
useEffect(() => { if (!open) return; + rootRef.current + ?.querySelector<HTMLElement>('[role="menuitemradio"], [role="menuitem"]') + ?.focus(); + }, [open]); + + useEffect(() => { + if (!open) return; + const handlePointerDown = (event: PointerEvent) => {+ const handleFocusOut = (event: FocusEvent) => { + const next = event.relatedTarget as Node | null; + if (next && !rootRef.current?.contains(next)) { + setActionMenuPath(null); + setOpen(false); + } + }; + document.addEventListener("pointerdown", handlePointerDown); document.addEventListener("keydown", handleKeyDown); + document.addEventListener("focusout", handleFocusOut); return () => { document.removeEventListener("pointerdown", handlePointerDown); document.removeEventListener("keydown", handleKeyDown); + document.removeEventListener("focusout", handleFocusOut); }; }, [actionMenuPath, open]);🤖 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/WorkspaceMenu.tsx` around lines 40 - 78, Update the focus handling around the existing open/actionMenuPath effect: when open becomes true, move focus to the first workspace menu item instead of leaving it on the trigger, and add a focusout listener that closes the menu when focus moves outside the menu root. Preserve the existing Escape behavior and action-submenu focus restoration, and clean up the new listener on effect teardown.src/lib/useWindowShortcuts.ts (1)
35-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider the updater form of
setInterfaceZoomto avoid listener churn.
interfaceZoomis in the dependency array, so thekeydownlistener is removed and re-added after every zoom step.setInterfaceZoominsrc/context/ThemeContext.tsx(Lines 522-533) already accepts an updater and applies the same clamp and rounding. Using the updater removes the duplicated math and theinterfaceZoomdependency.The toast label needs the resolved value, so compute it inside the updater.
♻️ Proposed refactor
const delta = action === "zoom-in" ? 0.05 : -0.05; - const next = Math.round( - Math.min(Math.max(interfaceZoom + delta, 0.7), 1.5) * 20, - ) / 20; - setInterfaceZoom(next); - toast(`Zoom ${Math.round(next * 100)}%`, { - id: "zoom", - duration: 1500, - }); + setInterfaceZoom((prev) => { + const next = Math.round(Math.min(Math.max(prev + delta, 0.7), 1.5) * 20) / 20; + toast(`Zoom ${Math.round(next * 100)}%`, { id: "zoom", duration: 1500 }); + return next; + }); }; window.addEventListener("keydown", handleKeyDown); return () => window.removeEventListener("keydown", handleKeyDown); - }, [interfaceZoom, onOpenPreferences, setInterfaceZoom]); + }, [onOpenPreferences, setInterfaceZoom]);Note: calling
toastinside the updater runs during render scheduling. If that is a concern, keep the toast outside and read the value from a ref instead.🤖 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 35 - 48, Update the zoom handling in the keydown handler to call setInterfaceZoom with an updater, relying on its existing clamp and rounding logic instead of duplicating the calculation and removing interfaceZoom from the effect dependency array. Compute the resolved zoom value inside the updater and use it for the toast label, while preserving the current zoom-step behavior and bounds.src/components/settings/SettingsPage.test.tsx (1)
30-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnmount the roots created by
mountSettingsPage.
mountSettingsPagereturnsroot, but the tests at Lines 45-65 never callroot.unmount(). TheafterEachhook at Lines 26-28 detaches the container nodes without unmounting, so the React roots stay alive across tests. Return an unmount helper, or unmount inafterEachby tracking the created roots.🤖 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 - 42, Ensure every React root created by mountSettingsPage is unmounted during test cleanup. Update the helper and/or the existing afterEach hook to track returned roots and call root.unmount() before removing their containers, covering all tests that mount SettingsPage.src/lib/useWindowSessionPersistence.ts (1)
171-179: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancel the writer after the final flush.
The unmount effect flushes pending patches but never calls
writer.cancel(). If a write fails during or after unmount,createWindowSessionPatchWriterschedules a retry timer that keeps firing for a window that no longer exists. Cancel the writer after the flush settles so no retry timer survives unmount.♻️ Proposed cleanup
useEffect(() => { return () => { void writer .flush() .catch((error) => { console.warn("Final window session update failed", error); + }) + .finally(() => { + writer.cancel(); }); }; }, [writer]);🤖 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 171 - 179, Update the unmount cleanup effect around writer.flush to call writer.cancel after the final flush settles, including when flush rejects. Preserve the existing warning for flush failures and ensure cancellation always runs so retry timers from createWindowSessionPatchWriter cannot survive unmount.src/lib/windowSession.test.ts (1)
118-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
flush,cancel, and the write-failure retry.The suite covers only the debounce and coalesce path.
createWindowSessionPatchWriteralso re-queues the failed patch and schedules a retry, drops pending work aftercancel(), and serializes against an in-flight write. These branches carry the session-loss risk and are untested. Add cases that reject the firstwriteand assert the retry sends the merged patch, and thatcancel()prevents any further write.🤖 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/windowSession.test.ts` around lines 118 - 150, Extend the createWindowSessionPatchWriter test suite with cases covering flush, cancel, and write-failure retry: reject the first write and verify the merged patch is retried, verify flush writes pending work immediately while respecting any in-flight write, and verify cancel drops queued work and prevents subsequent writes. Use the existing fake-timer setup and writer API, keeping assertions focused on write order and patch contents.src/lib/workspaceSwitch.test.ts (1)
5-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
loadWorkspacereceives the backend-resolved path.
runWorkspaceSwitchpasses the value returned byswitchBackendWorkspacetoloadWorkspace, notrequestedPath. This test returns the same string from the mock, so a regression that passesrequestedPaththrough would still pass. Return a different path fromswitchBackendWorkspaceand assert the argument ofloadWorkspace.💚 Proposed test addition
it("flushes the current draft before rebinding and loading the next workspace", async () => {it("loads the workspace path resolved by the backend", async () => { const loadWorkspace = vi.fn(async () => {}); const path = await runWorkspaceSwitch("/notes/requested", { flushCurrentDraft: vi.fn(async () => {}), switchBackendWorkspace: vi.fn(async () => "/notes/resolved"), loadWorkspace, }); expect(path).toBe("/notes/resolved"); expect(loadWorkspace).toHaveBeenCalledWith("/notes/resolved"); });🤖 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/workspaceSwitch.test.ts` around lines 5 - 23, Add coverage for backend-resolved paths in the runWorkspaceSwitch test: make switchBackendWorkspace return a path different from the requested path, capture the loadWorkspace mock, and assert both the returned value and that loadWorkspace receives the resolved path.src-tauri/src/hashing.rs (2)
203-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test vectors that exercise the multi-block and long-padding branches.
The current vectors are
b""(0 bytes) andb"abc"(3 bytes). Both take theremainder.len() < 56branch at Line 105 and never enter thechunks_exact(64)loop at Line 98. The two-block padding path and the message-schedule carry across blocks stay untested. A regression in either path would produce silently wrong revisions and checkpoint file names.Add published vectors for inputs of 56–63 bytes and for inputs longer than 64 bytes.
💚 Proposed additional test vectors
#[test] fn sha256_hex_deterministic() { assert_eq!(sha256_hex(b"same"), sha256_hex(b"same")); assert_ne!(sha256_hex(b"same"), sha256_hex(b"changed")); } + + #[test] + fn sha256_hex_two_block_padding() { + // 56 bytes: forces the 128-byte padding branch. + assert_eq!( + sha256_hex(&[b'a'; 56]), + "b35439a4ac6f0948b6d6f9e3c6af0f5f590ce20f1bde7090ef7970686ec6738a" + ); + } + + #[test] + fn sha256_hex_multi_block_input() { + // 448 bits + full blocks: exercises chunks_exact(64). + assert_eq!( + sha256_hex(b"abcdbcdecdefdefgefghfghighijhijkijkljklmklmnlmnomnopnopq"), + "248d6a61d20638b8e5c026930c3e6039a33ce45964ff2167f6ecedd419db06c1" + ); + }🤖 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 203 - 231, Add published SHA-256 test vectors in the hashing tests for inputs of 56–63 bytes, including the two-block padding boundary, and for an input longer than 64 bytes to exercise multi-block processing and message-schedule carry. Extend the existing sha256_hex tests with fixed expected digests, while preserving the current empty, abc, and determinism coverage.
166-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
From<String>andFrom<&str>let any string become aContentRevision.
ContentRevisiondocuments itself as a "Stable SHA-256 identifier". The twoFromimpls accept arbitrary text with no length or alphabet check. A caller can build a revision that no content can ever produce, andsave_if_revisioninsrc-tauri/src/persistence.rsthen compares it against real digests. The comparison always fails and reports a conflict. Consider a checked constructor that validates 64 lowercase hexadecimal characters, and keep the unchecked conversions private to the crate.🤖 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 166 - 197, Restrict ContentRevision creation to valid SHA-256 identifiers: add a checked constructor that accepts exactly 64 lowercase hexadecimal characters and rejects all other strings, then make the existing From<String> and From<&str> conversions crate-private or otherwise prevent external callers from using them unchecked. Preserve content_revision’s generated digest behavior and ensure save_if_revision comparisons only receive validated revisions.src-tauri/src/persistence.rs (1)
113-134: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
lock_keycan produce two keys for one file.
lock_keycanonicalizes the nearest existing ancestor and then appends the untouched suffix. If two callers reach the same file through different symlinked parents, or through paths that differ only in.or..components inside the suffix, they get different keys and therefore different mutexes. The in-process serialization insave_if_revisionis then lost for that file. Canonicalizing the full parent chain, or canonicalizing the destination itself when it exists, removes the gap.🤖 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/persistence.rs` around lines 113 - 134, Update lock_key to return one normalized key for equivalent paths by canonicalizing the full destination when it exists, or otherwise canonicalizing the nearest existing ancestor and normalizing the remaining suffix so "."/".." and symlinked parents cannot produce distinct keys. Preserve the existing fallback behavior when canonicalization fails, and ensure save_if_revision continues using this unified key.
🤖 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/draft_checkpoint.rs`:
- Around line 409-425: Update the mismatch_content JSON in
list_checkpoints_skips_identity_mismatch to use the camelCase fields expected by
DraftCheckpointKey and DraftCheckpointMetadata, including windowLabel, noteId,
sourcePath, baseRevision, and updatedAt. Keep the filename identity mismatch
intact so deserialization succeeds and ensure_identity_matches performs the
rejection.
In `@src-tauri/src/lib.rs`:
- Around line 1917-1933: Complete the workspace-scoped migration in
src-tauri/src/lib.rs: at lines 1917-1933 restore configure_main_workspace’s
parameters and return type, then initialize normalized_path and session via
prepare_notes_folder and WorkspaceSession::initialize_prepared before the
existing config block; at lines 1910-1914 remove the stale load_settings
assignment; at lines 3809-3826 close update_workspace_settings after Ok(()) and
remove its nested duplicate get_settings; at lines 3888-3911 remove the stale
second half of update_git_enabled that updates state.settings; and at lines
6259-6276 remove the obsolete startup settings/search_index computation and
unterminated binding.
- Around line 3126-3138: Update both collision-retry branches in duplicate_note
to build subsequent candidate IDs from the sanitized identifier rather than raw
copy_title. Preserve the existing folder_prefix formatting and counter behavior,
including the branch around abs_path_from_id and the later conflict branch, so
retries remain valid sanitized IDs.
In `@src-tauri/src/persistence.rs`:
- Around line 204-216: Update the fallback write flow around write_result,
flush_result, sync_result, and sync_parent_directory so write, flush, or
sync_all failures remove the destination file before returning the error. Defer
the parent-directory sync error until after reporting the write sequence errors,
ensuring the most specific write failure takes precedence while preserving
cleanup of the temporary file.
In `@src/App.tsx`:
- Around line 198-209: Update openSettings in AppContent to reuse the existing
flushCurrentDraft logic used by toggleSettings instead of referencing the
undeclared persistenceControllerRef. Preserve the current error toast, early
return on flush failure, and settings view transition.
In `@src/components/settings/SettingsPage.test.tsx`:
- Around line 69-101: Update the “omits drag regions when isWindows is true”
test to wrap rendering, assertions, and unmount cleanup in a try block, then
restore navigator.userAgent in a finally block so the original value is restored
even when an assertion fails.
In `@src/context/GitContext.tsx`:
- Around line 343-352: Update the GitProvider effect containing
handleSettingsChanged to also subscribe through the cross-window
listen<SettingsChangedEvent>("settings-changed", ...) bridge, incrementing
settingsRevision only for Git-relevant scope changes; retain the existing DOM
listener for in-WebView events and clean up both subscriptions on unmount.
In `@src/lib/windowClose.ts`:
- Around line 17-25: Update the error-handling flow around flushDraft and
persistRecovery so a recovery rejection is caught and the original saveError is
rethrown with the recovery failure attached as its cause. Replace the ambiguous
undefined recovery result with a distinct outcome for “nothing to recover,”
preserving the existing no-target behavior while allowing clean drafts to close
after a flush failure; update the callers consuming this result, including
App.tsx and PreviewApp.tsx, to honor the distinction.
---
Outside diff comments:
In `@src/components/notes/NoteList.tsx`:
- Around line 326-329: Update the sortedDisplayItems useMemo in NoteList so
sortNotesByModified is applied only when no search query is active; while
searching, return displayItems unchanged to preserve backend relevance ranking,
and retain the existing sortOrder behavior for non-search results.
In `@src/context/ThemeContext.tsx`:
- Around line 468-499: In resetEditorFontSettings, add the missing try block
before the await updateGlobalSettings persistence call so the existing catch
handler correctly matches it. Keep the current state updates, error logging,
toast, and loadSettingsFromBackend recovery behavior unchanged.
- Around line 719-723: Move the useMemo call that creates contextValue above the
isInitialized early-return guard in the theme provider, then keep the guard
immediately before the final return. Ensure all hooks execute on every render
while preserving the existing null result until initialization completes.
---
Nitpick comments:
In `@src-tauri/src/hashing.rs`:
- Around line 203-231: Add published SHA-256 test vectors in the hashing tests
for inputs of 56–63 bytes, including the two-block padding boundary, and for an
input longer than 64 bytes to exercise multi-block processing and
message-schedule carry. Extend the existing sha256_hex tests with fixed expected
digests, while preserving the current empty, abc, and determinism coverage.
- Around line 166-197: Restrict ContentRevision creation to valid SHA-256
identifiers: add a checked constructor that accepts exactly 64 lowercase
hexadecimal characters and rejects all other strings, then make the existing
From<String> and From<&str> conversions crate-private or otherwise prevent
external callers from using them unchecked. Preserve content_revision’s
generated digest behavior and ensure save_if_revision comparisons only receive
validated revisions.
In `@src-tauri/src/persistence.rs`:
- Around line 113-134: Update lock_key to return one normalized key for
equivalent paths by canonicalizing the full destination when it exists, or
otherwise canonicalizing the nearest existing ancestor and normalizing the
remaining suffix so "."/".." and symlinked parents cannot produce distinct keys.
Preserve the existing fallback behavior when canonicalization fails, and ensure
save_if_revision continues using this unified key.
In `@src/components/layout/WorkspaceMenu.tsx`:
- Around line 40-78: Update the focus handling around the existing
open/actionMenuPath effect: when open becomes true, move focus to the first
workspace menu item instead of leaving it on the trigger, and add a focusout
listener that closes the menu when focus moves outside the menu root. Preserve
the existing Escape behavior and action-submenu focus restoration, and clean up
the new listener on effect teardown.
In `@src/components/settings/SettingsPage.test.tsx`:
- Around line 30-42: Ensure every React root created by mountSettingsPage is
unmounted during test cleanup. Update the helper and/or the existing afterEach
hook to track returned roots and call root.unmount() before removing their
containers, covering all tests that mount SettingsPage.
In `@src/lib/useWindowSessionPersistence.ts`:
- Around line 171-179: Update the unmount cleanup effect around writer.flush to
call writer.cancel after the final flush settles, including when flush rejects.
Preserve the existing warning for flush failures and ensure cancellation always
runs so retry timers from createWindowSessionPatchWriter cannot survive unmount.
In `@src/lib/useWindowShortcuts.ts`:
- Around line 35-48: Update the zoom handling in the keydown handler to call
setInterfaceZoom with an updater, relying on its existing clamp and rounding
logic instead of duplicating the calculation and removing interfaceZoom from the
effect dependency array. Compute the resolved zoom value inside the updater and
use it for the toast label, while preserving the current zoom-step behavior and
bounds.
In `@src/lib/windowSession.test.ts`:
- Around line 118-150: Extend the createWindowSessionPatchWriter test suite with
cases covering flush, cancel, and write-failure retry: reject the first write
and verify the merged patch is retried, verify flush writes pending work
immediately while respecting any in-flight write, and verify cancel drops queued
work and prevents subsequent writes. Use the existing fake-timer setup and
writer API, keeping assertions focused on write order and patch contents.
In `@src/lib/workspaceSwitch.test.ts`:
- Around line 5-23: Add coverage for backend-resolved paths in the
runWorkspaceSwitch test: make switchBackendWorkspace return a path different
from the requested path, capture the loadWorkspace mock, and assert both the
returned value and that loadWorkspace receives the resolved path.
In `@src/services/notes.test.ts`:
- Line 147: Move the duplicateNote test covering the duplicate_note command out
of the “scoped settings writes” describe block, and place it in a separate
describe block named for duplicate-note behavior. Keep the test logic unchanged
and ensure the surrounding suite names accurately match the behaviors they
contain.
- Around line 115-117: Strengthen the negative assertions in the affected
service tests, including the cases around invokeMock at the shown locations, to
verify exactly one command call rather than only excluding one command/payload
pair. Assert invokeMock’s total call count is one while preserving the existing
expected command assertion, covering openWorkspaceWindow and the other noted
cases.
🪄 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: f6efd3b8-8dcb-4358-91b0-8106fd7fd728
📒 Files selected for processing (72)
src-tauri/capabilities/default.jsonsrc-tauri/src/draft_checkpoint.rssrc-tauri/src/hashing.rssrc-tauri/src/lib.rssrc-tauri/src/note_persistence_tests.rssrc-tauri/src/persistence.rssrc-tauri/src/watcher_debounce.rssrc/App.tsxsrc/components/editor/Editor.tsxsrc/components/editor/EditorWidthHandle.test.tsxsrc/components/editor/EditorWidthHandle.tsxsrc/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 (33)
- src/lib/standaloneRecreation.ts
- src/services/draftCheckpoint.test.ts
- src/lib/editorToolbar.ts
- src/lib/standaloneReload.test.ts
- src/components/editor/EditorWidthHandle.test.tsx
- src/lib/standaloneReload.ts
- src/lib/windowShortcuts.test.ts
- src/lib/titleBarNoteInfo.ts
- src/lib/titleBarNoteInfo.test.ts
- src/lib/conflictResolution.ts
- src/lib/windowCloseCallsites.test.ts
- src/lib/standaloneRecreation.test.ts
- src/services/files.test.ts
- src/lib/draftCheckpoint.ts
- src/lib/serializedWriter.ts
- src/services/windowLifecycle.test.ts
- src/lib/windowShortcuts.ts
- src/lib/serializedWriter.test.ts
- src/components/settings/SettingsPage.tsx
- src/components/editor/EditorWidthHandle.tsx
- src/lib/windowShortcutCallsites.test.ts
- src/components/settings/EditorSettingsSection.test.tsx
- src/lib/windowClose.test.ts
- src/lib/editorToolbar.test.ts
- src/services/files.ts
- src/lib/editorWidthResize.ts
- src/lib/editorWidthResize.test.ts
- src/lib/draftCheckpoint.test.ts
- src/services/draftCheckpoint.ts
- src/lib/conflictResolution.test.ts
- src/components/settings/EditorSettingsSection.tsx
- src/components/preview/PreviewApp.tsx
- src/components/editor/Editor.tsx
| let mut final_id = sanitized.clone(); | ||
| let mut counter = 1; | ||
| while abs_path_from_id(&folder_path, &final_id) | ||
| .map(|p| p.exists()) | ||
| .unwrap_or(false) | ||
| { | ||
| if has_counter { | ||
| final_id = sanitized.replace("{counter}", &counter.to_string()); | ||
| final_id = if let Some(ref prefix) = folder_prefix { | ||
| format!("{}/{}-{}", prefix, copy_title, counter) | ||
| } else { | ||
| final_id = format!("{}-{}", base_id, counter); | ||
| } | ||
| format!("{}-{}", copy_title, counter) | ||
| }; | ||
| counter += 1; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
duplicate_note builds collision names from the unsanitized title.
The first candidate uses sanitized (line 3126), but both collision paths on lines 3132 to 3136 and 3178 to 3182 build the next candidate from copy_title. copy_title is the raw "<title> (Copy)" string and has not passed sanitize_filename, so a note title that contains / or a reserved character produces an unintended nested ID or makes abs_path_from_id fail. create_note correctly retries from the sanitized base_id on line 3036.
Retry from the sanitized identifier.
🐛 Proposed fix
+ let sanitized_leaf = sanitize_filename(©_title);
let mut final_id = sanitized.clone();
let mut counter = 1;
while abs_path_from_id(&folder_path, &final_id)
.map(|p| p.exists())
.unwrap_or(false)
{
final_id = if let Some(ref prefix) = folder_prefix {
- format!("{}/{}-{}", prefix, copy_title, counter)
+ format!("{}/{}-{}", prefix, sanitized_leaf, counter)
} else {
- format!("{}-{}", copy_title, counter)
+ format!("{}-{}", sanitized_leaf, counter)
};
counter += 1;
}Apply the same substitution in the conflict branch on lines 3178 to 3182.
Also applies to: 3174-3184
🤖 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 3126 - 3138, Update both collision-retry
branches in duplicate_note to build subsequent candidate IDs from the sanitized
identifier rather than raw copy_title. Preserve the existing folder_prefix
formatting and counter behavior, including the branch around abs_path_from_id
and the later conflict branch, so retries remain valid sanitized IDs.
| const openSettings = useCallback(async () => { | ||
| if (view === "settings") return; | ||
| try { | ||
| await persistenceControllerRef.current?.flush(); | ||
| } catch (error) { | ||
| toast.error(`Settings not opened: ${error}`); | ||
| return; | ||
| } | ||
| setView("settings"); | ||
| }, [view]); | ||
|
|
||
| useWindowShortcuts({ onOpenPreferences: openSettings }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
openSettings calls an undefined ref.
persistenceControllerRef is not declared in AppContent. The component declares only currentNoteRef, editorRef, and closeInProgressRef. This reference fails to compile under TypeScript, and at runtime it would throw before the view changes. openSettings is the handler passed to useWindowShortcuts on line 209, so Cmd/Ctrl+, is the affected path.
toggleSettings already performs the same flush through flushCurrentDraft. Reuse that flush and drop the duplicate logic.
🐛 Proposed fix
const openSettings = useCallback(async () => {
if (view === "settings") return;
try {
- await persistenceControllerRef.current?.flush();
+ await flushCurrentDraft();
} catch (error) {
toast.error(`Settings not opened: ${error}`);
return;
}
setView("settings");
- }, [view]);
+ }, [flushCurrentDraft, view]);📝 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.
| const openSettings = useCallback(async () => { | |
| if (view === "settings") return; | |
| try { | |
| await persistenceControllerRef.current?.flush(); | |
| } catch (error) { | |
| toast.error(`Settings not opened: ${error}`); | |
| return; | |
| } | |
| setView("settings"); | |
| }, [view]); | |
| useWindowShortcuts({ onOpenPreferences: openSettings }); | |
| const openSettings = useCallback(async () => { | |
| if (view === "settings") return; | |
| try { | |
| await flushCurrentDraft(); | |
| } catch (error) { | |
| toast.error(`Settings not opened: ${error}`); | |
| return; | |
| } | |
| setView("settings"); | |
| }, [flushCurrentDraft, view]); | |
| useWindowShortcuts({ onOpenPreferences: openSettings }); |
🤖 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 198 - 209, Update openSettings in AppContent to
reuse the existing flushCurrentDraft logic used by toggleSettings instead of
referencing the undeclared persistenceControllerRef. Preserve the current error
toast, early return on flush failure, and settings view transition.
| it("omits drag regions when isWindows is true", async () => { | ||
| const originalUA = globalThis.navigator?.userAgent; | ||
| Object.defineProperty(globalThis.navigator || {}, "userAgent", { | ||
| value: "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36", | ||
| configurable: true, | ||
| }); | ||
|
|
||
| vi.resetModules(); | ||
| const { SettingsPage: WindowsSettingsPage } = await import("./SettingsPage"); | ||
|
|
||
| const container = document.createElement("div"); | ||
| document.body.append(container); | ||
| const root = createRoot(container); | ||
| act(() => | ||
| root.render( | ||
| <TooltipProvider> | ||
| <WindowsSettingsPage onBack={undefined} /> | ||
| </TooltipProvider>, | ||
| ), | ||
| ); | ||
|
|
||
| expect(container.querySelectorAll("[data-tauri-drag-region]")).toHaveLength( | ||
| 0, | ||
| ); | ||
| act(() => root.unmount()); | ||
|
|
||
| if (originalUA !== undefined) { | ||
| Object.defineProperty(globalThis.navigator, "userAgent", { | ||
| value: originalUA, | ||
| configurable: true, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore navigator.userAgent in a finally block.
The restore at Lines 95-100 runs only when every assertion passes. If the assertion at Line 90 fails, the fake Windows user agent stays installed for the rest of the worker, and later tests that read platform flags get the wrong value. Wrap the render and assertions in try and move the restore into finally.
💚 Proposed fix
vi.resetModules();
const { SettingsPage: WindowsSettingsPage } = await import("./SettingsPage");
- const container = document.createElement("div");
- document.body.append(container);
- const root = createRoot(container);
- act(() =>
- root.render(
- <TooltipProvider>
- <WindowsSettingsPage onBack={undefined} />
- </TooltipProvider>,
- ),
- );
-
- expect(container.querySelectorAll("[data-tauri-drag-region]")).toHaveLength(
- 0,
- );
- act(() => root.unmount());
-
- if (originalUA !== undefined) {
- Object.defineProperty(globalThis.navigator, "userAgent", {
- value: originalUA,
- configurable: true,
- });
- }
+ const container = document.createElement("div");
+ document.body.append(container);
+ const root = createRoot(container);
+ try {
+ act(() =>
+ root.render(
+ <TooltipProvider>
+ <WindowsSettingsPage onBack={undefined} />
+ </TooltipProvider>,
+ ),
+ );
+ expect(
+ container.querySelectorAll("[data-tauri-drag-region]"),
+ ).toHaveLength(0);
+ } finally {
+ act(() => root.unmount());
+ if (originalUA !== undefined) {
+ Object.defineProperty(globalThis.navigator, "userAgent", {
+ value: originalUA,
+ configurable: true,
+ });
+ }
+ }📝 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.
| it("omits drag regions when isWindows is true", async () => { | |
| const originalUA = globalThis.navigator?.userAgent; | |
| Object.defineProperty(globalThis.navigator || {}, "userAgent", { | |
| value: "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36", | |
| configurable: true, | |
| }); | |
| vi.resetModules(); | |
| const { SettingsPage: WindowsSettingsPage } = await import("./SettingsPage"); | |
| const container = document.createElement("div"); | |
| document.body.append(container); | |
| const root = createRoot(container); | |
| act(() => | |
| root.render( | |
| <TooltipProvider> | |
| <WindowsSettingsPage onBack={undefined} /> | |
| </TooltipProvider>, | |
| ), | |
| ); | |
| expect(container.querySelectorAll("[data-tauri-drag-region]")).toHaveLength( | |
| 0, | |
| ); | |
| act(() => root.unmount()); | |
| if (originalUA !== undefined) { | |
| Object.defineProperty(globalThis.navigator, "userAgent", { | |
| value: originalUA, | |
| configurable: true, | |
| }); | |
| } | |
| }); | |
| it("omits drag regions when isWindows is true", async () => { | |
| const originalUA = globalThis.navigator?.userAgent; | |
| Object.defineProperty(globalThis.navigator || {}, "userAgent", { | |
| value: "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36", | |
| configurable: true, | |
| }); | |
| vi.resetModules(); | |
| const { SettingsPage: WindowsSettingsPage } = await import("./SettingsPage"); | |
| const container = document.createElement("div"); | |
| document.body.append(container); | |
| const root = createRoot(container); | |
| try { | |
| act(() => | |
| root.render( | |
| <TooltipProvider> | |
| <WindowsSettingsPage onBack={undefined} /> | |
| </TooltipProvider>, | |
| ), | |
| ); | |
| expect( | |
| container.querySelectorAll("[data-tauri-drag-region]"), | |
| ).toHaveLength(0); | |
| } finally { | |
| act(() => root.unmount()); | |
| if (originalUA !== undefined) { | |
| Object.defineProperty(globalThis.navigator, "userAgent", { | |
| value: originalUA, | |
| configurable: true, | |
| }); | |
| } | |
| } | |
| }); |
🤖 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 69 - 101, Update
the “omits drag regions when isWindows is true” test to wrap rendering,
assertions, and unmount cleanup in a try block, then restore navigator.userAgent
in a finally block so the original value is restored even when an assertion
fails.
32e7ec5 to
29c0459
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
src/context/ThemeContext.tsx (1)
235-238: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake settings patches atomic in the Tauri backend.
Both sites read a complete settings snapshot and write a merged complete object. Concurrent changes from the editor and Preferences windows can start from the same snapshot. The final write can discard the other change.
Add a Tauri command that applies a partial settings patch under the backend settings lock. Use that command at both sites. A frontend queue alone does not protect independent WebViews.
src/context/ThemeContext.tsx#L235-L238: replace full-object read-modify-write with an atomic patch command.src/components/layout/Sidebar.tsx#L192-L199: persistsidebarSortOrderthrough the same atomic patch command.🤖 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 235 - 238, The settings updates in src/context/ThemeContext.tsx lines 235-238 and src/components/layout/Sidebar.tsx lines 192-199 must use a shared Tauri atomic patch command instead of reading and rewriting complete settings objects. Add the command to apply Partial<Settings> while holding the backend settings lock, then update updateSettingsPatch in ThemeContext and the sidebarSortOrder persistence in Sidebar to invoke it.src/components/editor/Editor.tsx (1)
2561-2567: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove
role="status"from the conflict trigger button.This element is a
DropdownMenu.Triggerbutton.role="status"replaces the implicitbuttonrole, so assistive technology announces a live region instead of a menu control. Keyboard and screen-reader users then cannot identify the control that opens the conflict menu. The indicator at Line 2623 usesrole="status"correctly because that element is not interactive.♿ Proposed fix
<button - role="status" + aria-label="Resolve save conflict" className="h-7 px-2 flex items-center gap-1 text-xs text-text-muted hover:bg-bg-emphasis rounded font-medium" >🤖 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 2561 - 2567, Remove the role="status" attribute from the conflict menu trigger button containing the Resolve Conflict label and RefreshCwIcon. Preserve the button’s implicit button role and all existing styling and behavior; leave the separate non-interactive status indicator unchanged.
🧹 Nitpick comments (13)
src/components/preview/PreviewApp.tsx (2)
416-429: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
previewData.This object literal is rebuilt on every render of
PreviewApp.Editorreceives it aspreviewMode, so the new identity defeats any memoization inside the editor subtree. Wrap it inuseMemokeyed on its members. The coding guidelines requireuseMemoon performance-critical paths, and the editor is the hot path in this window.♻️ Proposed change
- const previewData: PreviewModeData = { - content, - title, - filePath, - modified, - revision, - hasExternalChanges, - hasSaveConflict, - reloadVersion, - save, - reload, - resolveConflict, - registerPersistenceController, - }; + const previewData: PreviewModeData = useMemo( + () => ({ + content, + title, + filePath, + modified, + revision, + hasExternalChanges, + hasSaveConflict, + reloadVersion, + save, + reload, + resolveConflict, + registerPersistenceController, + }), + [ + content, + title, + filePath, + modified, + revision, + hasExternalChanges, + hasSaveConflict, + reloadVersion, + save, + reload, + resolveConflict, + registerPersistenceController, + ], + );As per coding guidelines: "Use
useCallbackanduseMemofor 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/components/preview/PreviewApp.tsx` around lines 416 - 429, Memoize the `previewData` object in `PreviewApp` with `useMemo`, including all listed members in its dependency array, so its identity remains stable unless one of those values changes before being passed to `Editor` as `previewMode`.Source: Coding guidelines
140-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
isStale()checks between synchronous setters.
isStale()can only change value after anawaitor after cleanup runs. Lines 140 through 165 contain noawaitbetween the setters, so the repeated checks cannot flip. Keep the checks that followawait readFileDirectandawait getDraftCheckpoint, and drop the rest. This reduces the noise in the effect without changing behavior.♻️ Proposed simplification
const recovered = checkpoint && checkpoint.markdown !== result.content ? checkpoint.markdown : result.content; - if (isStale()) return; setContent(recovered); - if (isStale()) return; setTitle(result.title); - if (isStale()) return; setModified(result.modified); const recoveryRevision = standaloneRecoveryBaseRevision( result.revision, result.content, checkpoint, ); revisionRef.current = recoveryRevision; - if (isStale()) return; setRevision(recoveryRevision); if (checkpoint && checkpoint.markdown === result.content) { await draftCheckpointService .clearDraftCheckpoint(checkpoint.key) .catch(() => undefined); } else if (checkpoint) { - if (isStale()) return; setHasExternalChanges(true); - if (isStale()) return; setHasSaveConflict(true); - if (isStale()) return; toast.warning("Recovered an unsaved draft from an interrupted session"); }🤖 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 140 - 165, Remove the redundant isStale() checks between the synchronous setters in the recovery flow, including those around setContent, setTitle, setModified, setRevision, setHasExternalChanges, setHasSaveConflict, and the warning toast. Preserve the checks that follow await readFileDirect and await getDraftCheckpoint, and keep the existing recovery behavior unchanged.src/lib/standaloneReload.test.ts (2)
70-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the no-checkpoint case.
standaloneRecoveryBaseRevisionacceptscheckpoint: DraftCheckpoint | null. Thenullcase is the normal path: a standalone window opens a file with no recovered draft. It must return the disk revision. Add one assertion for it.💚 Proposed test
it("uses the disk revision when no divergent recovery is applied", () => { expect( standaloneRecoveryBaseRevision( "current-disk-revision", "same content", checkpoint("same content", "older-revision"), ), ).toBe("current-disk-revision"); }); + + it("uses the disk revision when no checkpoint exists", () => { + expect( + standaloneRecoveryBaseRevision("current-disk-revision", "disk", null), + ).toBe("current-disk-revision"); + });🤖 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/standaloneReload.test.ts` around lines 70 - 100, Extend the standaloneRecoveryBaseRevision test suite with a no-checkpoint case that passes null for checkpoint and verifies the current disk revision is returned. Keep the existing divergent-recovery and matching-content assertions unchanged.
35-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the null controller.
flushDirtyDraftBeforeReloadacceptsnull. That path is reachable:PreviewApppassespersistenceControllerRef.current, which isnulluntil theEditorregisters its controller. A reload that arrives in that window must resolve without a flush and without a throw. No test covers it.💚 Proposed test
it("does not manufacture a write for a clean standalone note", async () => { const flush = vi.fn(async () => undefined); await flushDirtyDraftBeforeReload({ flush, getDraft: () => ({ noteId: "/note.md", content: "disk", dirty: false }), }); expect(flush).not.toHaveBeenCalled(); }); + + it("resolves when no controller is registered yet", async () => { + await expect(flushDirtyDraftBeforeReload(null)).resolves.toBeUndefined(); + });🤖 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/standaloneReload.test.ts` around lines 35 - 68, Add a test in the existing flushDirtyDraftBeforeReload suite covering a null controller, verifying the function resolves without throwing and does not invoke the flush callback when the controller is unavailable. Use the same invocation shape as the existing tests and preserve the current dirty/clean behavior coverage.src/lib/recoveryNotice.test.ts (1)
33-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the record path with malformed stored data.
This test proves
consumePendingRecoveryNoticestolerates malformed data.recordPendingRecoveryNoticeshares the samereadNoticeshelper but has no such guard, and that gap blocks window close. Add the symmetric test so the fix stays in place.💚 Proposed test
it("drops malformed stored notices without throwing", () => { const storage = createStorage(); storage.setItem("scratch:pendingRecoveryNotices", "not-json"); expect(consumePendingRecoveryNotices(storage)).toEqual([]); }); + + it("records a new notice over malformed stored data", () => { + const storage = createStorage(); + storage.setItem("scratch:pendingRecoveryNotices", "not-json"); + + expect(() => + recordPendingRecoveryNotice("/recovery/Plan.md", "disk full", storage), + ).not.toThrow(); + expect(consumePendingRecoveryNotices(storage)).toHaveLength(1); + });🤖 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/recoveryNotice.test.ts` around lines 33 - 38, Add a symmetric test in recoveryNotice.test.ts for recordPendingRecoveryNotice using malformed stored data such as “not-json”; verify the call does not throw and preserves the expected storage behavior, covering the readNotices path used during recording.src/lib/draftRepresentation.ts (1)
23-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the discard-before-flush ordering.
The order at Lines 34-40 is load-bearing.
discardFormattedanddiscardSourceadvance the save generation counters inEditor.tsx, so they must run before the flush to stop a stale in-flight save from clearing newer edits. Add a short comment that states this requirement, so a later reader does not reorder the calls.🤖 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/draftRepresentation.ts` around lines 23 - 42, Add a short explanatory comment in flushPendingDraftRepresentation immediately before the discard/flush branches, documenting that discardFormatted or discardSource must execute before the corresponding flush because they advance save-generation counters and prevent stale in-flight saves from clearing newer edits. Keep the existing ordering and behavior unchanged.src/lib/recoveryNotice.ts (1)
37-50: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider making notice recording non-fatal.
recordPendingRecoveryNoticethrows when storage is unavailable (Line 42), andstorage.setItemat Line 49 can throw when the quota is exceeded. Both throws propagate into thebeforeClosestep ofrunSafeWindowCloseand keep the window open. A failed notice is bookkeeping; it should not block a close whose recovery snapshot already succeeded.Wrap the
recordPendingRecoveryNoticecalls inApp.tsxandPreviewApp.tsxin try/catch, or make this function report failure through a return value instead of a throw.🤖 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/recoveryNotice.ts` around lines 37 - 50, Make pending recovery notice recording non-fatal by updating the callers of recordPendingRecoveryNotice in App.tsx and PreviewApp.tsx to catch and suppress recording failures, including unavailable storage and setItem errors, so runSafeWindowClose can complete after a successful recovery snapshot.src/lib/draftRepresentation.test.ts (1)
7-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the clean-draft case and move the flush tests to their own
describe.Two gaps:
- No test covers the
nullrepresentation. That branch prevents a write when neither representation is dirty. It protects a clean note from a spurious save.- The tests at Lines 27-57 exercise
flushPendingDraftRepresentation, but they sit insidedescribe("choosePendingDraftRepresentation"). The report then attributes the failures to the wrong function.💚 Proposed change
it("falls back to the only dirty representation", () => { expect(choosePendingDraftRepresentation(false, true, false)).toBe( "source", ); expect(choosePendingDraftRepresentation(true, false, true)).toBe( "formatted", ); }); + + it("returns null when no representation is dirty", () => { + expect(choosePendingDraftRepresentation(false, false, false)).toBeNull(); + expect(choosePendingDraftRepresentation(true, false, false)).toBeNull(); + }); +}); + +describe("flushPendingDraftRepresentation", () => { + it("performs no write when no representation is dirty", async () => { + const actions = { + discardSource: vi.fn(), + discardFormatted: vi.fn(), + flushSource: vi.fn(async () => undefined), + flushFormatted: vi.fn(async () => undefined), + }; + + await expect( + flushPendingDraftRepresentation(false, false, false, actions), + ).resolves.toBeNull(); + expect(actions.flushSource).not.toHaveBeenCalled(); + expect(actions.flushFormatted).not.toHaveBeenCalled(); + });🤖 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/draftRepresentation.test.ts` around lines 7 - 58, Add a test for choosePendingDraftRepresentation that passes clean source and formatted states and asserts the null representation, covering the no-write case. Move the two flushPendingDraftRepresentation tests into a separate describe block named for that function, leaving the existing choosePendingDraftRepresentation tests together.src/components/editor/Editor.tsx (3)
982-1002: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the two unmount cleanups.
Two cleanup paths now clear the same timers. The layout-effect cleanup at Lines 994-998 clears
checkpointCaptureTimerRefand nulls it. The effect cleanup at Lines 1868-1871 clears it again, and Lines 1862-1867 clearsaveTimeoutRefandsourceTimeoutRefthatflushAllPendingSavesalready cleared. The duplication is harmless today, but it splits one lifecycle concern across two effects and makes the cleanup order load-bearing.Move the timer and popup teardown into a single cleanup, or document why the split is required.
Also applies to: 1858-1870
🤖 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 982 - 1002, Consolidate unmount cleanup so timer and popup teardown occurs in one lifecycle cleanup instead of both the layout-effect cleanup and the effect cleanup. Update the cleanup logic around flushAllPendingSavesRef, checkpointCaptureTimerRef, checkpointSchedulerRef, saveTimeoutRef, and sourceTimeoutRef to remove duplicate clearing while preserving pending-save flushing and crash-checkpoint persistence.
2395-2431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
flushSourceSavein the debounced source-save timer.The timer body at Lines 2406-2428 repeats the save, generation check, dirty reset, and checkpoint clear that
flushSourceSavealready implements at Lines 893-911. The two copies already differ: this body saves the capturedvalueand setslastSaveRefdirectly, whileflushSourceSavesavessourceContentRef.currentthroughsaveImmediately. The copies can drift further.Call
flushSourceSavefrom the timer and keep the error handling here.🤖 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 2395 - 2431, Update the debounced timer inside handleSourceChange to call the existing flushSourceSave helper instead of duplicating saveNote, generation checks, dirty-state updates, and checkpoint handling. Preserve the timer’s current error handling and ensure flushSourceSave uses the latest source content through its established saveImmediately path.
964-965: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the render-phase ref writes into an effect.
Lines 965, 980, and 1004 assign ref values during render. React can discard a render, so the refs can hold values from work that never commits. The unmount cleanup at Lines 987 and 991 reads these refs, so a discarded render can flush the wrong callback closure.
Assign these three refs inside a
useEffectthat runs after every commit.♻️ Proposed change
- const flushAllPendingSavesRef = useRef(flushAllPendingSaves); - flushAllPendingSavesRef.current = flushAllPendingSaves; + const flushAllPendingSavesRef = useRef(flushAllPendingSaves);- const persistCurrentCrashCheckpointRef = useRef(persistCurrentCrashCheckpoint); - persistCurrentCrashCheckpointRef.current = persistCurrentCrashCheckpoint; + const persistCurrentCrashCheckpointRef = useRef(persistCurrentCrashCheckpoint); + + useEffect(() => { + flushAllPendingSavesRef.current = flushAllPendingSaves; + persistCurrentCrashCheckpointRef.current = persistCurrentCrashCheckpoint; + queueCheckpointCaptureRef.current = queueCheckpointCapture; + }, [flushAllPendingSaves, persistCurrentCrashCheckpoint, queueCheckpointCapture]);Wrap the
queueCheckpointCaptureRef.current = () => { ... }body in auseCallbacknamedqueueCheckpointCaptureand assign it in the same effect.Also applies to: 979-980, 1004-1020
🤖 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 964 - 965, Move the render-phase assignments to flushAllPendingSavesRef, the checkpoint-related ref at the referenced lines, and queueCheckpointCaptureRef into a useEffect that runs after every commit. Create the queueCheckpointCapture callback with useCallback as requested, then update all three refs within that effect so unmount cleanup reads only committed callback closures.Source: Linters/SAST tools
src/lib/draftCheckpoint.ts (1)
177-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute draft checkpoint load fallback through
reconcileDraftCheckpoint.
src/components/preview/PreviewApp.tsxre-implements the checkpoint recovery decision inline. That logic matchessrc/lib/draftCheckpoint.ts, so the load path should callreconcileDraftCheckpointto keep recovery behavior consistent and avoid losing unsaved checkpoint text.🤖 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 177 - 201, The PreviewApp checkpoint-load path should stop duplicating recovery decisions and call reconcileDraftCheckpoint from draftCheckpoint.ts instead. Use its returned note, remote, recovered, and shouldClear values to preserve checkpoint text and consistent cleanup behavior, removing the equivalent inline logic.src/lib/standaloneReload.ts (1)
26-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the empty-string revision sentinel.
standaloneRecoveryBaseRevision()returns""when a divergent checkpoint has nometadata.baseRevision. Document this intentionally as the “unknown base revision” branch: it preventssaveFileDirect()from using the disk revision and instead makes the save use the empty-content hash, so edits are reported as a conflict until a clean known base is available.🤖 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/standaloneReload.ts` around lines 26 - 35, Document the empty-string sentinel in standaloneRecoveryBaseRevision(): when checkpoint.markdown differs from diskContent and checkpoint.metadata.baseRevision is missing, keep returning "" intentionally as the unknown base revision branch. Clarify in the standaloneRecoveryBaseRevision flow that this value is meant to avoid saveFileDirect() falling back to diskRevision so the save uses the empty-content hash and surfaces the conflict until a known base is available.
🤖 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/Editor.tsx`:
- Around line 994-999: Update the cleanup block in Editor.tsx to handle the
promise returned by checkpointSchedulerRef.current?.dispose(), attaching a catch
that reports failures through the same console.error path used by the
scheduler’s onError handler. Preserve the existing timer cleanup and optional
scheduler disposal behavior.
In `@src/components/settings/SettingsPage.test.tsx`:
- Around line 47-62: Ensure every test root is unmounted even when assertions
fail by moving each root.unmount call into a finally block: update both cases in
src/components/settings/SettingsPage.test.tsx (lines 47-62 and 70-84) and the
case in src/components/settings/SettingsPage.windows.test.tsx (lines 46-58).
Preserve the existing test actions and assertions.
In `@src/context/GitContext.tsx`:
- Around line 77-101: Apply the dual-context pattern used by NotesContext: in
src/context/GitContext.tsx lines 77-101, split Git state data from Git action
callbacks into separate contexts and providers so state-only consumers do not
rerender on action changes; update corresponding consumers while preserving
existing behavior. In src/context/ThemeContext.tsx lines 741-749, likewise
expose theme preference state and setters through separate data and actions
contexts, updating consumers to use the appropriate context.
In `@src/context/NotesContext.test.tsx`:
- Around line 41-74: Update the test probe declarations for actions and error to
use explicit nullable unions with typed null assertions, preserving
ReturnType<typeof useNotesActions> | null and string | null rather than inferred
null-only types. Keep the assignments inside Probe and the existing saveNote
assertion flow unchanged.
In `@src/context/ThemeContext.tsx`:
- Around line 547-584: The preference setters in setEditorWidthResizeEnabled,
setEditorToolbarVisible, and updateTitleBarNoteInfo must roll back their local
state or reload persisted settings when updateSettingsPatch fails, instead of
leaving unsaved values displayed. Add a user-facing error toast in each failure
path using the existing notification mechanism and a clear friendly message,
while retaining the existing console error logging where appropriate.
In `@src/lib/draftCheckpoint.ts`:
- Around line 107-129: Reschedule the checkpoint timer when storage.write fails
in flush, after restoring the checkpoint to pending, so transient failures are
retried without requiring another caller action. Reuse the existing schedule
function and preserve the rejected promise propagation and onError handling.
In `@src/lib/recoveryNotice.ts`:
- Around line 22-35: Update readNotices to catch JSON.parse failures and return
an empty notice list for malformed stored data, keeping the existing array and
notice validation behavior unchanged. Centralize this tolerance in readNotices
so both recordPendingRecoveryNotice and consumePendingRecoveryNotices handle
invalid storage consistently.
In `@src/lib/useWindowShortcuts.ts`:
- Around line 14-15: Update the zoom flow in useWindowShortcuts so the clamped,
rounded value is the one actually committed by setInterfaceZoom instead of
current + delta, and keep the toast in sync with the committed interfaceZoom.
Remove the render-phase interfaceZoomRef write if it is only there to read the
latest value for the shortcut handler, and compute the next zoom inside the
state updater or from committed state so the bounds and 0.05 քայլ rounding are
preserved.
---
Duplicate comments:
In `@src/components/editor/Editor.tsx`:
- Around line 2561-2567: Remove the role="status" attribute from the conflict
menu trigger button containing the Resolve Conflict label and RefreshCwIcon.
Preserve the button’s implicit button role and all existing styling and
behavior; leave the separate non-interactive status indicator unchanged.
In `@src/context/ThemeContext.tsx`:
- Around line 235-238: The settings updates in src/context/ThemeContext.tsx
lines 235-238 and src/components/layout/Sidebar.tsx lines 192-199 must use a
shared Tauri atomic patch command instead of reading and rewriting complete
settings objects. Add the command to apply Partial<Settings> while holding the
backend settings lock, then update updateSettingsPatch in ThemeContext and the
sidebarSortOrder persistence in Sidebar to invoke it.
---
Nitpick comments:
In `@src/components/editor/Editor.tsx`:
- Around line 982-1002: Consolidate unmount cleanup so timer and popup teardown
occurs in one lifecycle cleanup instead of both the layout-effect cleanup and
the effect cleanup. Update the cleanup logic around flushAllPendingSavesRef,
checkpointCaptureTimerRef, checkpointSchedulerRef, saveTimeoutRef, and
sourceTimeoutRef to remove duplicate clearing while preserving pending-save
flushing and crash-checkpoint persistence.
- Around line 2395-2431: Update the debounced timer inside handleSourceChange to
call the existing flushSourceSave helper instead of duplicating saveNote,
generation checks, dirty-state updates, and checkpoint handling. Preserve the
timer’s current error handling and ensure flushSourceSave uses the latest source
content through its established saveImmediately path.
- Around line 964-965: Move the render-phase assignments to
flushAllPendingSavesRef, the checkpoint-related ref at the referenced lines, and
queueCheckpointCaptureRef into a useEffect that runs after every commit. Create
the queueCheckpointCapture callback with useCallback as requested, then update
all three refs within that effect so unmount cleanup reads only committed
callback closures.
In `@src/components/preview/PreviewApp.tsx`:
- Around line 416-429: Memoize the `previewData` object in `PreviewApp` with
`useMemo`, including all listed members in its dependency array, so its identity
remains stable unless one of those values changes before being passed to
`Editor` as `previewMode`.
- Around line 140-165: Remove the redundant isStale() checks between the
synchronous setters in the recovery flow, including those around setContent,
setTitle, setModified, setRevision, setHasExternalChanges, setHasSaveConflict,
and the warning toast. Preserve the checks that follow await readFileDirect and
await getDraftCheckpoint, and keep the existing recovery behavior unchanged.
In `@src/lib/draftCheckpoint.ts`:
- Around line 177-201: The PreviewApp checkpoint-load path should stop
duplicating recovery decisions and call reconcileDraftCheckpoint from
draftCheckpoint.ts instead. Use its returned note, remote, recovered, and
shouldClear values to preserve checkpoint text and consistent cleanup behavior,
removing the equivalent inline logic.
In `@src/lib/draftRepresentation.test.ts`:
- Around line 7-58: Add a test for choosePendingDraftRepresentation that passes
clean source and formatted states and asserts the null representation, covering
the no-write case. Move the two flushPendingDraftRepresentation tests into a
separate describe block named for that function, leaving the existing
choosePendingDraftRepresentation tests together.
In `@src/lib/draftRepresentation.ts`:
- Around line 23-42: Add a short explanatory comment in
flushPendingDraftRepresentation immediately before the discard/flush branches,
documenting that discardFormatted or discardSource must execute before the
corresponding flush because they advance save-generation counters and prevent
stale in-flight saves from clearing newer edits. Keep the existing ordering and
behavior unchanged.
In `@src/lib/recoveryNotice.test.ts`:
- Around line 33-38: Add a symmetric test in recoveryNotice.test.ts for
recordPendingRecoveryNotice using malformed stored data such as “not-json”;
verify the call does not throw and preserves the expected storage behavior,
covering the readNotices path used during recording.
In `@src/lib/recoveryNotice.ts`:
- Around line 37-50: Make pending recovery notice recording non-fatal by
updating the callers of recordPendingRecoveryNotice in App.tsx and
PreviewApp.tsx to catch and suppress recording failures, including unavailable
storage and setItem errors, so runSafeWindowClose can complete after a
successful recovery snapshot.
In `@src/lib/standaloneReload.test.ts`:
- Around line 70-100: Extend the standaloneRecoveryBaseRevision test suite with
a no-checkpoint case that passes null for checkpoint and verifies the current
disk revision is returned. Keep the existing divergent-recovery and
matching-content assertions unchanged.
- Around line 35-68: Add a test in the existing flushDirtyDraftBeforeReload
suite covering a null controller, verifying the function resolves without
throwing and does not invoke the flush callback when the controller is
unavailable. Use the same invocation shape as the existing tests and preserve
the current dirty/clean behavior coverage.
In `@src/lib/standaloneReload.ts`:
- Around line 26-35: Document the empty-string sentinel in
standaloneRecoveryBaseRevision(): when checkpoint.markdown differs from
diskContent and checkpoint.metadata.baseRevision is missing, keep returning ""
intentionally as the unknown base revision branch. Clarify in the
standaloneRecoveryBaseRevision flow that this value is meant to avoid
saveFileDirect() falling back to diskRevision so the save uses the empty-content
hash and surfaces the conflict until a known base is available.
🪄 Autofix
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: 6e557ab9-d67f-4074-bb9b-2de808c4eff4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (71)
package.jsonsrc-tauri/capabilities/default.jsonsrc-tauri/src/draft_checkpoint.rssrc-tauri/src/lib.rssrc-tauri/src/persistence.rssrc-tauri/src/sha256.rssrc/App.tsxsrc/components/editor/Editor.tsxsrc/components/editor/EditorWidthHandle.test.tsxsrc/components/editor/EditorWidthHandle.tsxsrc/components/layout/Sidebar.tsxsrc/components/layout/SidebarControls.test.tsxsrc/components/layout/SidebarControls.tsxsrc/components/layout/SidebarFolderSection.test.tsxsrc/components/layout/SidebarFolderSection.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/components/settings/SettingsPage.windows.test.tsxsrc/context/GitContext.tsxsrc/context/NotesContext.test.tsxsrc/context/NotesContext.tsxsrc/context/ThemeContext.tsxsrc/lib/conflictResolution.test.tssrc/lib/conflictResolution.tssrc/lib/draftCheckpoint.test.tssrc/lib/draftCheckpoint.tssrc/lib/draftRepresentation.test.tssrc/lib/draftRepresentation.tssrc/lib/editorToolbar.test.tssrc/lib/editorToolbar.tssrc/lib/editorWidthResize.test.tssrc/lib/editorWidthResize.tssrc/lib/folderTree.test.tssrc/lib/folderTree.tssrc/lib/recoveryNotice.test.tssrc/lib/recoveryNotice.tssrc/lib/serializedWriter.test.tssrc/lib/serializedWriter.tssrc/lib/settingsEvents.test.tssrc/lib/settingsEvents.tssrc/lib/standaloneRecreation.test.tssrc/lib/standaloneRecreation.tssrc/lib/standaloneReload.test.tssrc/lib/standaloneReload.tssrc/lib/titleBarNoteInfo.test.tssrc/lib/titleBarNoteInfo.tssrc/lib/useWindowShortcuts.test.tsxsrc/lib/useWindowShortcuts.tssrc/lib/windowClose.test.tssrc/lib/windowClose.tssrc/lib/windowCloseCallsites.test.tssrc/lib/windowMode.test.tssrc/lib/windowMode.tssrc/lib/windowShortcutCallsites.test.tssrc/lib/windowShortcuts.test.tssrc/lib/windowShortcuts.tssrc/services/draftCheckpoint.test.tssrc/services/draftCheckpoint.tssrc/services/files.test.tssrc/services/files.tssrc/services/notes.tssrc/services/windowLifecycle.test.tssrc/services/windowLifecycle.tssrc/types/note.tsvitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (41)
- src/lib/editorToolbar.test.ts
- src/lib/windowCloseCallsites.test.ts
- package.json
- src/services/draftCheckpoint.test.ts
- src/lib/windowShortcuts.ts
- src/services/files.test.ts
- src/lib/windowShortcutCallsites.test.ts
- src/lib/standaloneRecreation.test.ts
- src/lib/folderTree.test.ts
- src/services/files.ts
- vitest.config.ts
- src/services/windowLifecycle.test.ts
- src/lib/titleBarNoteInfo.ts
- src/lib/editorWidthResize.ts
- src/lib/titleBarNoteInfo.test.ts
- src/lib/standaloneRecreation.ts
- src/lib/editorWidthResize.test.ts
- src/lib/conflictResolution.test.ts
- src/components/notes/NoteList.tsx
- src/lib/editorToolbar.ts
- src/components/layout/SidebarControls.tsx
- src/lib/windowShortcuts.test.ts
- src/components/layout/SidebarFolderSection.test.tsx
- src/components/settings/EditorSettingsSection.test.tsx
- src/lib/conflictResolution.ts
- src/lib/serializedWriter.ts
- src/components/layout/SidebarControls.test.tsx
- src/components/editor/EditorWidthHandle.test.tsx
- src/lib/serializedWriter.test.ts
- src/components/layout/SidebarFolderSection.tsx
- src/components/notes/FolderTreeView.tsx
- src/services/windowLifecycle.ts
- src/lib/folderTree.ts
- src/components/settings/SettingsPage.tsx
- src/components/notes/FolderTreeView.test.tsx
- src/services/draftCheckpoint.ts
- src/lib/draftCheckpoint.test.ts
- src/components/editor/EditorWidthHandle.tsx
- src/App.tsx
- src/components/settings/EditorSettingsSection.tsx
- src-tauri/src/lib.rs
| if (checkpointCaptureTimerRef.current) { | ||
| clearTimeout(checkpointCaptureTimerRef.current); | ||
| checkpointCaptureTimerRef.current = null; | ||
| checkpointCaptureStartedAtRef.current = null; | ||
| } | ||
| }, 500); | ||
| }, [saveImmediately, getMarkdown, currentNote?.id]); | ||
| checkpointSchedulerRef.current?.dispose(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the promise returned by dispose().
Line 999 calls checkpointSchedulerRef.current?.dispose() and discards the result. dispose returns flush() (src/lib/draftCheckpoint.ts Lines 154-157), and flush rejects when the final storage.write fails. The rejection is unhandled, so unmount produces an unhandled promise rejection instead of a reported error. The scheduler's onError handler at Lines 661-664 does not run, because it applies only to timer-driven flushes.
src/components/editor/Editor.tsx#L994-L999: attach acatchto thedispose()call and route the error to the sameconsole.errorused by the scheduler'sonError.
🐛 Proposed fix
if (checkpointCaptureTimerRef.current) {
clearTimeout(checkpointCaptureTimerRef.current);
checkpointCaptureTimerRef.current = null;
checkpointCaptureStartedAtRef.current = null;
}
- checkpointSchedulerRef.current?.dispose();
+ void checkpointSchedulerRef.current?.dispose().catch((error) => {
+ console.error("Failed to persist final crash checkpoint:", error);
+ });📝 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 (checkpointCaptureTimerRef.current) { | |
| clearTimeout(checkpointCaptureTimerRef.current); | |
| checkpointCaptureTimerRef.current = null; | |
| checkpointCaptureStartedAtRef.current = null; | |
| } | |
| }, 500); | |
| }, [saveImmediately, getMarkdown, currentNote?.id]); | |
| checkpointSchedulerRef.current?.dispose(); | |
| if (checkpointCaptureTimerRef.current) { | |
| clearTimeout(checkpointCaptureTimerRef.current); | |
| checkpointCaptureTimerRef.current = null; | |
| checkpointCaptureStartedAtRef.current = null; | |
| } | |
| void checkpointSchedulerRef.current?.dispose().catch((error) => { | |
| console.error("Failed to persist final crash checkpoint:", 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/components/editor/Editor.tsx` around lines 994 - 999, Update the cleanup
block in Editor.tsx to handle the promise returned by
checkpointSchedulerRef.current?.dispose(), attaching a catch that reports
failures through the same console.error path used by the scheduler’s onError
handler. Preserve the existing timer cleanup and optional scheduler disposal
behavior.
| act(() => | ||
| root.render( | ||
| <TooltipProvider> | ||
| <SettingsPage onBack={onBack} /> | ||
| </TooltipProvider>, | ||
| ), | ||
| ); | ||
|
|
||
| const backButton = container.querySelector<HTMLButtonElement>( | ||
| 'button[aria-label^="Back"]', | ||
| ); | ||
| expect(backButton).not.toBeNull(); | ||
| act(() => backButton?.click()); | ||
| expect(onBack).toHaveBeenCalledOnce(); | ||
|
|
||
| act(() => root.unmount()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Always unmount test roots during cleanup.
A failed assertion skips root.unmount(). Removing the container does not remove SettingsPage window listeners. Use try/finally, or register each root for afterEach cleanup.
src/components/settings/SettingsPage.test.tsx#L47-L62: unmount the root infinally.src/components/settings/SettingsPage.test.tsx#L70-L84: unmount the root infinally.src/components/settings/SettingsPage.windows.test.tsx#L46-L58: unmount the root infinally.
📍 Affects 2 files
src/components/settings/SettingsPage.test.tsx#L47-L62(this comment)src/components/settings/SettingsPage.test.tsx#L70-L84src/components/settings/SettingsPage.windows.test.tsx#L46-L58
🤖 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 47 - 62, Ensure
every test root is unmounted even when assertions fail by moving each
root.unmount call into a finally block: update both cases in
src/components/settings/SettingsPage.test.tsx (lines 47-62 and 70-84) and the
case in src/components/settings/SettingsPage.windows.test.tsx (lines 46-58).
Preserve the existing test actions and assertions.
| useEffect(() => { | ||
| let disposed = false; | ||
| let unlisten: (() => void) | undefined; | ||
|
|
||
| void listen<GitSettingsChangedEvent>("settings-changed", (event) => { | ||
| if ( | ||
| disposed || | ||
| !isGitSettingsEventForFolder(event.payload, notesFolderRef.current) | ||
| ) { | ||
| return; | ||
| } | ||
| settingsReadRequestIdRef.current += 1; | ||
| if (typeof event.payload.gitEnabled === "boolean") { | ||
| setGitEnabledState(event.payload.gitEnabled); | ||
| } | ||
| }).then((removeListener) => { | ||
| if (disposed) removeListener(); | ||
| else unlisten = removeListener; | ||
| }); | ||
|
|
||
| return () => { | ||
| disposed = true; | ||
| unlisten?.(); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Split context data from context actions.
These providers expose changing state and action callbacks through one context value. A state update rerenders consumers that only need actions. Separate data and actions contexts, following the NotesContext pattern.
src/context/GitContext.tsx#L77-L101: expose Git state and Git actions through separate contexts.src/context/ThemeContext.tsx#L741-L749: expose theme preference state and setters through separate contexts.
As per coding guidelines, “Use NotesContext with dual context pattern (data/actions separated) for performance optimization.”
📍 Affects 2 files
src/context/GitContext.tsx#L77-L101(this comment)src/context/ThemeContext.tsx#L741-L749
🤖 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/GitContext.tsx` around lines 77 - 101, Apply the dual-context
pattern used by NotesContext: in src/context/GitContext.tsx lines 77-101, split
Git state data from Git action callbacks into separate contexts and providers so
state-only consumers do not rerender on action changes; update corresponding
consumers while preserving existing behavior. In src/context/ThemeContext.tsx
lines 741-749, likewise expose theme preference state and setters through
separate data and actions contexts, updating consumers to use the appropriate
context.
Source: Coding guidelines
| let actions: ReturnType<typeof useNotesActions> | null = null; | ||
| let error: string | null = null; | ||
|
|
||
| function Probe() { | ||
| actions = useNotesActions(); | ||
| error = useNotesData().error; | ||
| return null; | ||
| } | ||
|
|
||
| const container = document.createElement("div"); | ||
| document.body.append(container); | ||
| const root = createRoot(container); | ||
|
|
||
| await act(async () => { | ||
| root.render( | ||
| <NotesProvider> | ||
| <Probe /> | ||
| </NotesProvider>, | ||
| ); | ||
| }); | ||
|
|
||
| expect(actions).not.toBeNull(); | ||
|
|
||
| await act(async () => { | ||
| await expect( | ||
| actions!.saveNote("# Draft\n\nLatest content", "draft.md"), | ||
| ).rejects.toBe(failure); | ||
| }); | ||
|
|
||
| expect(saveNoteMock).toHaveBeenCalledWith( | ||
| "draft.md", | ||
| "# Draft\n\nLatest content", | ||
| ); | ||
| expect(error).toBe("disk full"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Run the repository typecheck to confirm whether the test file compiles.
set -u
echo "--- package.json scripts ---"
fd -t f -d 2 '^package.json$' --exec sh -c 'echo "== {}"; jq ".scripts, .devDependencies.typescript" {}'
echo "--- tsconfig include/exclude ---"
fd -t f 'tsconfig.*json' --exec sh -c 'echo "== {}"; cat {}'
echo "--- typecheck ---"
npx tsc --noEmit 2>&1 | head -40Repository: erictli/scratch
Length of output: 6066
🏁 Script executed:
#!/bin/bash
set -u
echo "--- files ---"
git ls-files 'src/**/*.{ts,tsx}' | sed -n '1,120p'
echo "--- NotesContext.test.tsx lines 1-90 ---"
cat -n src/context/NotesContext.test.tsx | sed -n '1,90p'
echo "--- NotesContext.tsx relevant declarations ---"
rg -n "export .*NotesProvider|useNotesActions|saveNote|error" src/context/NotesContext.tsx || true
sed -n '1,220p' src/context/NotesContext.tsx
echo "--- TypeScript version/tool availability ---"
node - <<'JS'
try {
const tsv = require('./node_modules/typescript/package.json').version;
console.log('node_modules/typescript:', tsv);
} catch {
console.log('node_modules/typescript: not present');
}
try {
const ts = require('./node_modules/typescript');
const source = `let actions: string | null = null; (() => { actions = "x"; })(); actions!.length;`;
const result = ts.transpileModule(source, { compilerOptions: { strict: true, noImplicitAny: true, preserveConstEnums: false } });
console.log('typescript available for probe');
console.log(result.diagnostics.map(d => `${d.file?.fileName}:${d.start}:${d.code || 'ts'} ${ts.flattenDiagnosticMessageText(d.messageText, '\n')}`).join('\n'));
} catch (e) {
console.log('typescript probe unavailable:', e && e.message);
}
JSRepository: erictli/scratch
Length of output: 12331
🌐 Web query:
TypeScript control flow narrowing let variable assigned inside function initializer null unresolved assignment outside function
💡 Result:
TypeScript's control flow analysis (CFA) is intentionally limited regarding side effects occurring inside closures (functions) [1][2][3]. The compiler does not perform inter-procedural analysis to track how captured variables might be mutated by functions, as doing so would be computationally prohibitive and impact compile-time performance [1][3]. When you assign a value to a let variable inside a function initializer or a callback, the TypeScript compiler generally cannot guarantee that the assignment happens synchronously or at all before the variable is used elsewhere [4][5][2]. Consequently, TypeScript follows these key behaviors: 1. Narrowing Persistence Limitations: In most cases, narrowing applied to a let variable is lost when the variable is captured by a closure [5][2]. The compiler assumes that the closure might be invoked at any time or multiple times, potentially reassigning the variable and invalidating previous narrowing assumptions [5][3]. 2. Exception for Last Assignments (TypeScript 5.4+): As of TypeScript 5.4, the compiler preserves type refinements for let variables in closures, but only if the closure is created after the last assignment to that variable in the outer scope [4][2]. If the variable is reassigned after the closure is created, narrowing is reset because the compiler must account for the possibility of the variable's value changing [4][2]. 3. Unresolved Assignments: Because the compiler does not track mutations inside functions, it may fail to recognize that a variable has been assigned a value, leading to situations where the compiler continues to treat the variable as having its initial (often broader) type, or conversely, failing to narrow it correctly after a function call [6][3]. Recommended Workarounds: - Use type assertions or definite assignment assertions (!) if you are certain a variable will be assigned before it is used [7]. - Explicitly copy the value into a const-bound variable within the scope where you need the narrowed type [1][5]. - If you need a more specific type after a function call, manually assert the type (e.g., as T) rather than relying on the compiler to track cross-function mutations [6]. For authoritative details, refer to the TypeScript GitHub discussions on control flow analysis trade-offs [1] and the implementation notes for preserved narrowing in closures [4].
Citations:
- 1: Trade-offs in Control Flow Analysis microsoft/TypeScript#9998
- 2: https://stackoverflow.com/questions/78903564/narrowing-related-behaviours-on-callbacks-in-typescript-when-using-let
- 3: https://stackoverflow.com/questions/70390528/the-modified-closure-variable-ts-cannot-be-detected
- 4: Preserve type refinements in closures created past last assignment microsoft/TypeScript#56908
- 5: https://stackoverflow.com/questions/73052676/type-narrowed-let-variable-reverts-to-its-original-type-when-passed-to-a-closu
- 6: Control flow analysis fails to account for possible assignments to captured variables in closures. microsoft/TypeScript#30097
- 7: Definite assignment assertions microsoft/TypeScript#20166
Declare test probes explicitly instead of collapsing with null initializers.
actions and error are only assigned inside Probe while Probe is invoked inside act, but TypeScript only sees declarations initialized to null. This keeps both variables narrowed to null, making actions!.saveNote(...) uncallable. Use ReturnType<typeof useNotesActions> | null and string | null with null as ... so the declared union is preserved.
🤖 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.test.tsx` around lines 41 - 74, Update the test
probe declarations for actions and error to use explicit nullable unions with
typed null assertions, preserving ReturnType<typeof useNotesActions> | null and
string | null rather than inferred null-only types. Keep the assignments inside
Probe and the existing saveNote assertion flow unchanged.
| const setEditorWidthResizeEnabled = useCallback( | ||
| async (enabled: boolean) => { | ||
| setEditorWidthResizeEnabledState(enabled); | ||
| try { | ||
| await updateSettingsPatch({ editorWidthResizeEnabled: enabled }); | ||
| } catch (error) { | ||
| console.error("Failed to save editor width resize setting:", error); | ||
| } | ||
| }, | ||
| [updateSettingsPatch], | ||
| ); | ||
|
|
||
| const setEditorToolbarVisible = useCallback( | ||
| async (visible: boolean) => { | ||
| setEditorToolbarVisibleState(visible); | ||
| try { | ||
| await updateSettingsPatch({ editorToolbarVisible: visible }); | ||
| } catch (error) { | ||
| console.error("Failed to save editor toolbar setting:", error); | ||
| } | ||
| }, | ||
| [updateSettingsPatch], | ||
| ); | ||
|
|
||
| const updateTitleBarNoteInfo = useCallback( | ||
| (kind: TitleBarNoteInfoKind, visible: boolean) => { | ||
| const next = updateTitleBarNoteInfoVisibility( | ||
| titleBarNoteInfoVisibilityRef.current, | ||
| kind, | ||
| visible, | ||
| ); | ||
| applyTitleBarNoteInfoVisibility(next); | ||
| void updateSettingsPatch({ | ||
| titleBarModifiedDateVisible: next.modifiedDateVisible, | ||
| titleBarFilenameVisible: next.filenameVisible, | ||
| }).catch((error) => { | ||
| console.error("Failed to save title bar information setting:", error); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore state and show a user-facing error after a failed save.
These setters apply the preference before persistence. If updateSettingsPatch fails, the UI keeps the new value although it will be lost after reload. Restore the previous value or reload settings, and show an error toast.
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 547 - 584, The preference setters
in setEditorWidthResizeEnabled, setEditorToolbarVisible, and
updateTitleBarNoteInfo must roll back their local state or reload persisted
settings when updateSettingsPatch fails, instead of leaving unsaved values
displayed. Add a user-facing error toast in each failure path using the existing
notification mechanism and a clear friendly message, while retaining the
existing console error logging where appropriate.
Source: Coding guidelines
| 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; | ||
| throw error; | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| const schedule = () => { | ||
| cancelTimer(); | ||
| timer = setTimeout(() => { | ||
| timer = undefined; | ||
| void flush().catch(onError); | ||
| }, delayMs); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A failed checkpoint write is never retried.
flush restores pending when storage.write rejects (Lines 116-119), but it does not reschedule the timer. Nothing then retries the write. The next attempt happens only when the caller invokes markDirty, flush, handleVisibilityChange, or dispose. Editor.tsx schedules markDirty from keystrokes only, and its onError handler just logs (Lines 661-664).
The failure mode: the user types, the checkpoint write fails once (for example a transient filesystem error), and the user stops typing. The crash checkpoint for that draft is then never written, so a crash loses the draft. That defeats the purpose of the checkpoint.
Reschedule the timer after a failed write.
🛡️ 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;
+ if (!disposed) schedule();
throw error;
}
});
};📝 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.
| 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; | |
| throw error; | |
| } | |
| }); | |
| }; | |
| const schedule = () => { | |
| cancelTimer(); | |
| timer = setTimeout(() => { | |
| timer = undefined; | |
| void flush().catch(onError); | |
| }, delayMs); | |
| }; | |
| 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; | |
| if (!disposed) schedule(); | |
| throw error; | |
| } | |
| }); | |
| }; | |
| const schedule = () => { | |
| cancelTimer(); | |
| timer = setTimeout(() => { | |
| timer = undefined; | |
| void flush().catch(onError); | |
| }, delayMs); | |
| }; |
🤖 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 107 - 129, Reschedule the checkpoint
timer when storage.write fails in flush, after restoring the checkpoint to
pending, so transient failures are retried without requiring another caller
action. Reuse the existing schedule function and preserve the rejected promise
propagation and onError handling.
| function readNotices(storage: RecoveryNoticeStorage): RecoveryNotice[] { | ||
| const stored = storage.getItem(STORAGE_KEY); | ||
| if (!stored) return []; | ||
| const parsed: unknown = JSON.parse(stored); | ||
| if (!Array.isArray(parsed)) return []; | ||
| return parsed.filter( | ||
| (notice): notice is RecoveryNotice => | ||
| typeof notice === "object" && | ||
| notice !== null && | ||
| typeof notice.recoveredTo === "string" && | ||
| typeof notice.saveError === "string" && | ||
| typeof notice.createdAt === "string", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Malformed stored data makes recordPendingRecoveryNotice throw and blocks window close.
readNotices calls JSON.parse at Line 25 without error handling. consumePendingRecoveryNotices guards that call with try/catch (Lines 56-63), but recordPendingRecoveryNotice does not (Line 43). A malformed value under scratch:pendingRecoveryNotices therefore makes recordPendingRecoveryNotice throw.
The reachable consequence: App.tsx (Lines 124-128) and PreviewApp.tsx (Lines 92-96) call recordPendingRecoveryNotice from the beforeClose step of runSafeWindowClose. The throw rejects that flow. The catch handler resets closeInProgressRef and shows "Window kept open because the draft could not be saved". The recovery snapshot was already written at that point, so the message is wrong, and the user cannot close the window until the malformed key is cleared.
Move the tolerance into readNotices so both callers behave the same.
🐛 Proposed fix
function readNotices(storage: RecoveryNoticeStorage): RecoveryNotice[] {
const stored = storage.getItem(STORAGE_KEY);
if (!stored) return [];
- const parsed: unknown = JSON.parse(stored);
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(stored);
+ } catch {
+ return [];
+ }
if (!Array.isArray(parsed)) return [];Also applies to: 37-50
🤖 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/recoveryNotice.ts` around lines 22 - 35, Update readNotices to catch
JSON.parse failures and return an empty notice list for malformed stored data,
keeping the existing array and notice validation behavior unchanged. Centralize
this tolerance in readNotices so both recordPendingRecoveryNotice and
consumePendingRecoveryNotices handle invalid storage consistently.
| const interfaceZoomRef = useRef(interfaceZoom); | ||
| interfaceZoomRef.current = interfaceZoom; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The zoom updater discards the clamped value, so the zoom state escapes its bounds.
Lines 40-42 compute next with a clamp to [0.7, 1.5] and a round to a 0.05 step. Line 44 then ignores next and commits current + delta. Three defects follow:
- The clamp does not reach the state. Repeated
zoom-inpushesinterfaceZoomabove 1.5, and repeatedzoom-outpushes it below 0.7. - The rounding does not reach the state, so floating-point drift accumulates. Three
zoom-inpresses from 1 store1.1500000000000001. - The toast on line 45 reports
next, not the committed value. At a bound the toast repeats "Zoom 150%" while the state keeps growing. The first press in the opposite direction then produces no visible change.
interfaceZoomRef exists only to read the current zoom without adding interfaceZoom to the effect dependencies. That forces the render-phase ref write on line 15, which React Doctor flags. Compute the clamp and the round inside the state updater instead. The ref is then unnecessary, and the render stays pure.
The toast text must report the committed value. Move it into an effect on interfaceZoom, or derive the target value before the call as shown.
🐛 Proposed fix
-import { useEffect, useRef } from "react";
+import { useEffect } from "react";
import { toast } from "sonner";
import { useTheme } from "../context/ThemeContext";
import { resolveWindowShortcut } from "./windowShortcuts";
@@
export function useWindowShortcuts({
onOpenPreferences,
}: UseWindowShortcutsOptions): void {
- const { interfaceZoom, setInterfaceZoom } = useTheme();
- const interfaceZoomRef = useRef(interfaceZoom);
- interfaceZoomRef.current = interfaceZoom;
+ const { setInterfaceZoom } = useTheme();
+
+ const clampZoom = (value: number) =>
+ Math.round(Math.min(Math.max(value, 0.7), 1.5) * 20) / 20;
@@
if (action === "zoom-reset") {
- interfaceZoomRef.current = 1;
setInterfaceZoom(1);
toast("Zoom 100%", { id: "zoom", duration: 1500 });
return;
}
const delta = action === "zoom-in" ? 0.05 : -0.05;
- const next = Math.round(
- Math.min(Math.max(interfaceZoomRef.current + delta, 0.7), 1.5) * 20,
- ) / 20;
- interfaceZoomRef.current = next;
- setInterfaceZoom((current) => current + delta);
- toast(`Zoom ${Math.round(next * 100)}%`, {
- id: "zoom",
- duration: 1500,
- });
+ setInterfaceZoom((current) => {
+ const next = clampZoom(current + delta);
+ toast(`Zoom ${Math.round(next * 100)}%`, {
+ id: "zoom",
+ duration: 1500,
+ });
+ return next;
+ });Note: if setInterfaceZoom runs its updater more than once, as React Strict Mode does in development, the toast call inside the updater duplicates. In that case keep the updater pure and read the committed interfaceZoom in a separate effect to raise the toast.
Also applies to: 39-48
🧰 Tools
🪛 React Doctor (0.9.3)
[error] 15-15: This ref is mutated during render. React can replay or discard render work, so the mutation can leak from UI that never commits.
Move ref writes into an event handler or effect. Render must stay pure because React can replay or discard it. The predictable null-guarded lazy initialization pattern remains supported.
(no-ref-current-in-render)
🤖 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 14 - 15, Update the zoom flow in
useWindowShortcuts so the clamped, rounded value is the one actually committed
by setInterfaceZoom instead of current + delta, and keep the toast in sync with
the committed interfaceZoom. Remove the render-phase interfaceZoomRef write if
it is only there to read the latest value for the shortcut handler, and compute
the next zoom inside the state updater or from committed state so the bounds and
0.05 քայլ rounding are preserved.
Source: Linters/SAST tools
1 participant
Summary
Cmd/Ctrl+,and the native application menu.Stack
This is PR 3 of the Scratch 1.0.1 backport stack. It depends on #198 and #199.
The branch currently includes PR 1 and PR 2 commits. Merge the stack in order; this PR diff will shrink to the standalone-window commit after its dependencies land.
Verification
npm test -- --run: 22 files, 77 tests passed.npm run build: passed.cargo test --manifest-path src-tauri/Cargo.toml --quiet: 15 tests passed.cargo clippy --manifest-path src-tauri/Cargo.toml --all-targets --all-features -- -D warnings: passed.git diff --check HEAD^..HEAD: passed.Tests cover serialized writes, close interception, dirty-draft checkpoints, recovery snapshots, stale-revision conflicts, explicit resolution, external reload decisions, standalone recreation, window shortcuts, and required call sites.
macOS runtime acceptance
An isolated built app was launched with an external Markdown file and verified during development:
Cmd+,opened the Settings window;Cmd+Wflushed the dirty content before closing.Scope
Multiple workspaces/windows, selection formatting, tables, image drag/drop, and block drag/drop remain outside this change.
No zero-bug guarantee is possible. Risk is reduced through donor-identical persistence modules, focused regression tests, a full build, Rust tests, Clippy, scope auditing, and real macOS window/save validation.
Summary by CodeRabbit
New Features
Bug Fixes
Tests