Conversation
WalkthroughThis pull request introduces a notes management feature across the Wave application. It adds a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 8
🤖 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 `@frontend/app/modals/alertmodal.tsx`:
- Around line 64-67: The modal wrapper in the AlertModal component is missing
dialog semantics and the close control is not keyboard-accessible; update the
modal container element (the div currently rendered as the modal wrapper) to
include role="dialog", aria-modal="true" and an aria-labelledby (pointing to the
modal title element id) and/or aria-describedby as appropriate, remove
tabIndex={-1} from the close button so it is focusable, ensure the close button
is a native <button> (or has keyboard handlers for Enter/Space) and add an
Escape key handler on the modal (e.g., onKeyDown on the wrapper) that invokes
the existing close function to allow keyboard dismissal.
- Around line 63-64: The createPortal calls in AlertModal use
document.getElementById("main") without the project's non-null assertion
pattern; update both occurrences (the one in the return
ReactDOM.createPortal(...) and the later createPortal at lines around 103-104)
to append a non-null assertion (!) to document.getElementById("main") so it
matches how modal.tsx handles the root element and avoids adding a runtime
guard.
In `@frontend/app/view/notes/notes-model.ts`:
- Around line 222-238: The code clears this.pendingContent at the start of
saveContent(), which causes dispose() to have nothing to flush if the RPC write
fails; change saveContent() so it does NOT set this.pendingContent = null before
awaiting env.rpc.WriteNoteCommand — only clear pendingContent after the write
succeeds (i.e., move the nulling into the try block after await and after
setting saved state and timers), and apply the same fix to the other occurrence
referenced around lines 333-337; keep dispose() behavior unchanged so it can
still flush pendingContent on errors.
- Around line 254-299: The handler currently clears pendingContent and
unconditionally replaces the editor buffer (see this.debouncedSave.cancel,
this.pendingContent and editorModel.applyEdits), which discards local unsaved
edits; change it to first check if this.pendingContent is non-null and, if so,
avoid wiping the buffer and avoid overwriting globalStore content/saveStatus
(skip editorModel.applyEdits and the globalStore.set(this.contentAtom, ...) /
saveStatus update), but still update loadError, filePath and readOnly atoms;
keep this.pendingContent intact until the local save completes.
In `@frontend/app/view/notes/notes.tsx`:
- Around line 14-17: The editor is currently always writable; readOnlyAtom is
tracked but never used, so fetch the note's read-only state with
useAtomValue(model.readOnlyAtom) and pass that value into the editor instance
props where the editor is rendered (the editor component/creation call around
lines ~67-71 and the initial editor usage near lines 14-17). Use the atom name
model.readOnlyAtom and set the editor's readOnly (or editable=false depending on
the editor API) prop so the UI respects the backend read-only flag and prevents
typing/saving when readOnly is true.
In `@frontend/preview/previews/notes.preview.tsx`:
- Around line 43-51: The preview is overriding the wrong RPC names—change the
useRpcOverride calls so they target the RPCs used by NotesViewModel: replace
"FileReadCommand" with "GetNoteCommand" and "FileWriteCommand" with
"WriteNoteCommand", keep the same failReadRef/failWriteRef checks and returned
payload shapes (e.g., return the base64-encoded MockContent for reads and null
for writes) so the preview toggles actually intercept the note read/write paths
invoked by NotesViewModel in notes-model.ts.
In `@pkg/notesmanager/notesmanager.go`:
- Around line 152-176: watchLoop currently reads mutable fields nm.watcher and
nm.notesPath directly and can race with resetInit which closes and nils those
fields; fix by capturing the watcher and notesPath into local variables under
the NotesManager lock (or by adding a new mutex if none exists) before entering
the select, and use those locals (e.g. localWatcher := nm.watcher.Events /
localNotesPath := nm.notesPath) inside the loop so the goroutine never
dereferences nm.watcher or nm.notesPath concurrently; ensure you check for nil
local watcher and exit or recreate the loop if resetInit replaced/closed the
watcher, and keep calling nm.handleExternalChange() as before.
- Around line 307-312: The current use of os.Rename to move nm.tmpPath to
nm.notesPath is not atomic on Windows; replace it with an atomic replace
implementation: create the temp file in the same directory (nm.tmpPath), fsync
the file (file.Sync()) and close it, then perform a platform-aware replace -- on
Unix use os.Rename, but on Windows call the native ReplaceFile/MoveFileEx
equivalent (or use a well-tested library such as github.com/google/renameio) to
ensure atomic swap; update the code path that currently renames nm.tmpPath to
nm.notesPath (references to nm.tmpPath and nm.notesPath) to perform the
fsync+atomic-replace sequence.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1eced695-d4db-4060-9313-0154b603f2c8
📒 Files selected for processing (25)
docs/docs/config.mdxfrontend/app/block/blockregistry.tsfrontend/app/modals/alertmodal.tsxfrontend/app/modals/modalregistry.tsxfrontend/app/store/wshclientapi.tsfrontend/app/view/notes/notes-model.tsfrontend/app/view/notes/notes.tsxfrontend/app/view/term/term-model.tsfrontend/preview/mock/mockwaveenv.tsfrontend/preview/previews/alertmodal.preview.tsxfrontend/preview/previews/notes.preview.tsxfrontend/tailwindsetup.cssfrontend/types/gotypes.d.tsfrontend/types/waveevent.d.tspkg/notesmanager/notesmanager.gopkg/tsgen/tsgenevent.gopkg/waveobj/objrtinfo.gopkg/wconfig/defaultconfig/settings.jsonpkg/wconfig/metaconsts.gopkg/wconfig/settingsconfig.gopkg/wps/wpstypes.gopkg/wshrpc/wshclient/wshclient.gopkg/wshrpc/wshrpctypes.gopkg/wshrpc/wshserver/wshserver.goschema/settings.json
| return ReactDOM.createPortal( | ||
| <div |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Portal targets using `#main` =="
rg -n --type=ts --type=tsx 'createPortal\(|getElementById\("main"\)' frontend
echo "== Definitions of id=\"main\" in frontend markup =="
rg -n 'id=["'"'"']main["'"'"']' frontend
echo "== Preview render containers (sanity check) =="
rg -n --iglob '*preview*' 'id=["'"'"']main["'"'"']|createRoot\(|render\(' frontend/previewRepository: wavetermdev/waveterm
Length of output: 143
🏁 Script executed:
# Check if the file exists and read its content
head -n 120 frontend/app/modals/alertmodal.tsx | tail -n +55Repository: wavetermdev/waveterm
Length of output: 2464
🏁 Script executed:
# Search for `#main` element definitions without file type filter
rg 'id=["\047\042]main' frontend --type htmlRepository: wavetermdev/waveterm
Length of output: 255
🏁 Script executed:
# Look for other portal usages
rg 'createPortal' frontend -A 2 -B 1Repository: wavetermdev/waveterm
Length of output: 1262
🏁 Script executed:
# Check preview/test environment setup for main element
fd -e html frontend/preview/ frontend/test/ 2>/dev/null | head -20Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Search for "main" element definition - try different approaches
rg 'id.*main' frontend --type html -B 1 -A 1Repository: wavetermdev/waveterm
Length of output: 322
🏁 Script executed:
# Check for main element in tsx/ts files where it might be created
rg 'id=.*main' frontend -B 1 -A 1 --type ts --type tsx | head -30Repository: wavetermdev/waveterm
Length of output: 92
🏁 Script executed:
# Search for app root or main container setup
rg 'createRoot|render.*main|getElementById.*main' frontend -B 2 -A 2Repository: wavetermdev/waveterm
Length of output: 3272
🏁 Script executed:
# Look at the main entry point
find frontend -name "index.html" -o -name "main.tsx" -o -name "main.ts" -o -name "app.tsx" | head -10Repository: wavetermdev/waveterm
Length of output: 113
🏁 Script executed:
# Check if there's a main index.html at root or in build config
find frontend -maxdepth 2 -name "index.html" -o -name "tsconfig.json" | grep -E "(root|tsconfig)"Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Check what happens in wave.ts if getElementById("main") returns null
cat -n frontend/wave.ts | grep -A 3 -B 3 'getElementById.*main'Repository: wavetermdev/waveterm
Length of output: 742
🏁 Script executed:
# Verify the exact line numbers in alertmodal.tsx where the createPortal call is
wc -l frontend/app/modals/alertmodal.tsxRepository: wavetermdev/waveterm
Length of output: 103
🏁 Script executed:
# Get exact content around alertmodal line 103-104
sed -n '100,110p' frontend/app/modals/alertmodal.tsxRepository: wavetermdev/waveterm
Length of output: 293
Use non-null assertion for consistency with modal.tsx pattern.
While document.getElementById("main") is guaranteed to exist at this point (the app initialization in wave.ts already depends on it), use the non-null assertion ! for consistency with the codebase pattern seen in modal.tsx rather than adding a runtime guard.
- document.getElementById("main")
+ document.getElementById("main")!The same applies at lines 103-104.
🤖 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 `@frontend/app/modals/alertmodal.tsx` around lines 63 - 64, The createPortal
calls in AlertModal use document.getElementById("main") without the project's
non-null assertion pattern; update both occurrences (the one in the return
ReactDOM.createPortal(...) and the later createPortal at lines around 103-104)
to append a non-null assertion (!) to document.getElementById("main") so it
matches how modal.tsx handles the root element and avoids adding a runtime
guard.
| <div | ||
| className="fixed inset-0 flex items-center justify-center" | ||
| style={{ zIndex: "var(--zindex-modal-wrapper)" }} | ||
| > |
There was a problem hiding this comment.
Add dialog semantics and keyboard-accessible close control.
The modal container is missing role="dialog" / aria-modal, and the close button is removed from tab order (tabIndex={-1}), which hurts accessibility.
💡 Proposed fix
- <div
+ <div
className="fixed inset-0 flex items-center justify-center"
style={{ zIndex: "var(--zindex-modal-wrapper)" }}
+ role="dialog"
+ aria-modal="true"
+ aria-label={title}
>
@@
- <button
+ <button
className="text-muted hover:text-primary transition-colors cursor-pointer p-0.5"
onClick={close}
- tabIndex={-1}
+ aria-label="Close alert"
>Also applies to: 80-90
🤖 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 `@frontend/app/modals/alertmodal.tsx` around lines 64 - 67, The modal wrapper
in the AlertModal component is missing dialog semantics and the close control is
not keyboard-accessible; update the modal container element (the div currently
rendered as the modal wrapper) to include role="dialog", aria-modal="true" and
an aria-labelledby (pointing to the modal title element id) and/or
aria-describedby as appropriate, remove tabIndex={-1} from the close button so
it is focusable, ensure the close button is a native <button> (or has keyboard
handlers for Enter/Space) and add an Escape key handler on the modal (e.g.,
onKeyDown on the wrapper) that invokes the existing close function to allow
keyboard dismissal.
| async saveContent(text: string) { | ||
| this.pendingContent = null; | ||
| console.log("[notes] saveContent start, text.len=", text.length); | ||
| try { | ||
| await this.env.rpc.WriteNoteCommand(TabRpcClient, { | ||
| content: text, | ||
| sourceoref: this.myOref, | ||
| }); | ||
| console.log("[notes] saveContent success, setting saved"); | ||
| globalStore.set(this.saveStatusAtom, "saved"); | ||
| globalStore.set(this.saveErrorAtom, ""); | ||
| this.setSavedTimer(); | ||
| } catch (err) { | ||
| console.log("[notes] saveContent error:", err); | ||
| globalStore.set(this.saveStatusAtom, "error"); | ||
| globalStore.set(this.saveErrorAtom, err?.message ?? String(err)); | ||
| } |
There was a problem hiding this comment.
Keep pendingContent until the write actually succeeds.
saveContent() nulls pendingContent before WriteNoteCommand resolves. If that write fails, dispose() no longer has anything to flush, so closing the widget after an error can lose the latest unsaved text.
Proposed fix
async saveContent(text: string) {
- this.pendingContent = null;
console.log("[notes] saveContent start, text.len=", text.length);
try {
await this.env.rpc.WriteNoteCommand(TabRpcClient, {
content: text,
sourceoref: this.myOref,
});
+ if (this.pendingContent === text) {
+ this.pendingContent = null;
+ }
console.log("[notes] saveContent success, setting saved");
globalStore.set(this.saveStatusAtom, "saved");
globalStore.set(this.saveErrorAtom, "");
this.setSavedTimer();Also applies to: 333-337
🤖 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 `@frontend/app/view/notes/notes-model.ts` around lines 222 - 238, The code
clears this.pendingContent at the start of saveContent(), which causes dispose()
to have nothing to flush if the RPC write fails; change saveContent() so it does
NOT set this.pendingContent = null before awaiting env.rpc.WriteNoteCommand —
only clear pendingContent after the write succeeds (i.e., move the nulling into
the try block after await and after setting saved state and timers), and apply
the same fix to the other occurrence referenced around lines 333-337; keep
dispose() behavior unchanged so it can still flush pendingContent on errors.
| this.debouncedSave.cancel({ upcomingOnly: true }); | ||
| this.pendingContent = null; | ||
| if (data?.error) { | ||
| const editor = this.editorRef.current; | ||
| if (editor != null) { | ||
| const editorModel = editor.getModel(); | ||
| if (editorModel != null) { | ||
| this.isApplyingRemoteEdit = true; | ||
| try { | ||
| editorModel.applyEdits([{ range: editorModel.getFullModelRange(), text: "" }]); | ||
| } finally { | ||
| this.isApplyingRemoteEdit = false; | ||
| } | ||
| } | ||
| } | ||
| globalStore.set(this.contentAtom, ""); | ||
| globalStore.set(this.filePathAtom, ""); | ||
| globalStore.set(this.readOnlyAtom, false); | ||
| globalStore.set(this.loadErrorAtom, data.error); | ||
| globalStore.set(this.saveStatusAtom, "idle"); | ||
| return; | ||
| } | ||
| const editor = this.editorRef.current; | ||
| const content = data?.content ?? ""; | ||
| if (editor != null) { | ||
| const editorModel = editor.getModel(); | ||
| if (editorModel != null) { | ||
| this.isApplyingRemoteEdit = true; | ||
| try { | ||
| editorModel.applyEdits([{ range: editorModel.getFullModelRange(), text: content }]); | ||
| } finally { | ||
| this.isApplyingRemoteEdit = false; | ||
| } | ||
| } | ||
| } | ||
| globalStore.set(this.contentAtom, content); | ||
| globalStore.set(this.loadErrorAtom, ""); | ||
| if (data?.filepath) { | ||
| globalStore.set(this.filePathAtom, data.filepath); | ||
| } | ||
| if (data?.readonly != null) { | ||
| globalStore.set(this.readOnlyAtom, data.readonly); | ||
| } | ||
| globalStore.set(this.saveStatusAtom, "synced"); | ||
| this.setSavedTimer(); | ||
| } |
There was a problem hiding this comment.
Don't overwrite local dirty edits with a remote update.
This handler cancels the pending local save and replaces the buffer immediately, even when this instance has unsaved text in pendingContent. If another widget or an external file edit lands during the debounce window, the local keystrokes are discarded.
🤖 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 `@frontend/app/view/notes/notes-model.ts` around lines 254 - 299, The handler
currently clears pendingContent and unconditionally replaces the editor buffer
(see this.debouncedSave.cancel, this.pendingContent and editorModel.applyEdits),
which discards local unsaved edits; change it to first check if
this.pendingContent is non-null and, if so, avoid wiping the buffer and avoid
overwriting globalStore content/saveStatus (skip editorModel.applyEdits and the
globalStore.set(this.contentAtom, ...) / saveStatus update), but still update
loadError, filePath and readOnly atoms; keep this.pendingContent intact until
the local save completes.
| const content = useAtomValue(model.contentAtom); | ||
| const loadError = useAtomValue(model.loadErrorAtom); | ||
| const loaded = useAtomValue(model.loadedAtom); | ||
| const wordWrap = useAtomValue(model.wordWrapAtom); |
There was a problem hiding this comment.
Respect the notes file's read-only state in the editor.
The model tracks readOnlyAtom, but the editor is hardcoded writable. That lets users type into a note that the backend has already marked read-only, which turns into avoidable save errors and misleading UX.
Proposed fix
export function NotesView({ model }: ViewComponentProps<NotesViewModel>) {
const content = useAtomValue(model.contentAtom);
const loadError = useAtomValue(model.loadErrorAtom);
const loaded = useAtomValue(model.loadedAtom);
+ const readOnly = useAtomValue(model.readOnlyAtom);
const wordWrap = useAtomValue(model.wordWrapAtom);
@@
<MonacoCodeEditor
text={content}
- readonly={false}
+ readonly={readOnly}
language="markdown"
onChange={(text) => model.onContentChange(text)}Also applies to: 67-71
🤖 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 `@frontend/app/view/notes/notes.tsx` around lines 14 - 17, The editor is
currently always writable; readOnlyAtom is tracked but never used, so fetch the
note's read-only state with useAtomValue(model.readOnlyAtom) and pass that value
into the editor instance props where the editor is rendered (the editor
component/creation call around lines ~67-71 and the initial editor usage near
lines 14-17). Use the atom name model.readOnlyAtom and set the editor's readOnly
(or editable=false depending on the editor API) prop so the UI respects the
backend read-only flag and prevents typing/saving when readOnly is true.
| useRpcOverride("FileReadCommand", async () => { | ||
| if (failReadRef.current) throw new Error("Permission denied: cannot read ~/notes.md"); | ||
| return { data64: stringToBase64(MockContent) }; | ||
| }); | ||
|
|
||
| useRpcOverride("FileWriteCommand", async () => { | ||
| if (failWriteRef.current) throw new Error("Disk full: cannot write ~/notes.md"); | ||
| return null; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Looking for note RPC usage in notes model:"
rg -n -C3 'GetNoteCommand|WriteNoteCommand|FileReadCommand|FileWriteCommand' frontend/app/view/notes/notes-model.ts frontend/preview/previews/notes.preview.tsxRepository: wavetermdev/waveterm
Length of output: 3636
Preview mocks target the wrong RPC commands.
The preview overrides FileReadCommand/FileWriteCommand (lines 43–51), but NotesViewModel in notes-model.ts calls GetNoteCommand (line 180) and WriteNoteCommand (line 226). The preview toggles won't intercept the actual note read/write paths.
🔧 Proposed fix
- useRpcOverride("FileReadCommand", async () => {
+ useRpcOverride("GetNoteCommand", async () => {
if (failReadRef.current) throw new Error("Permission denied: cannot read ~/notes.md");
- return { data64: stringToBase64(MockContent) };
+ return { text: MockContent, path: "~/notes.md" };
});
- useRpcOverride("FileWriteCommand", async () => {
+ useRpcOverride("WriteNoteCommand", async () => {
if (failWriteRef.current) throw new Error("Disk full: cannot write ~/notes.md");
return null;
});📝 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.
| useRpcOverride("FileReadCommand", async () => { | |
| if (failReadRef.current) throw new Error("Permission denied: cannot read ~/notes.md"); | |
| return { data64: stringToBase64(MockContent) }; | |
| }); | |
| useRpcOverride("FileWriteCommand", async () => { | |
| if (failWriteRef.current) throw new Error("Disk full: cannot write ~/notes.md"); | |
| return null; | |
| }); | |
| useRpcOverride("GetNoteCommand", async () => { | |
| if (failReadRef.current) throw new Error("Permission denied: cannot read ~/notes.md"); | |
| return { text: MockContent, path: "~/notes.md" }; | |
| }); | |
| useRpcOverride("WriteNoteCommand", async () => { | |
| if (failWriteRef.current) throw new Error("Disk full: cannot write ~/notes.md"); | |
| return null; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/preview/previews/notes.preview.tsx` around lines 43 - 51, The
preview is overriding the wrong RPC names—change the useRpcOverride calls so
they target the RPCs used by NotesViewModel: replace "FileReadCommand" with
"GetNoteCommand" and "FileWriteCommand" with "WriteNoteCommand", keep the same
failReadRef/failWriteRef checks and returned payload shapes (e.g., return the
base64-encoded MockContent for reads and null for writes) so the preview toggles
actually intercept the note read/write paths invoked by NotesViewModel in
notes-model.ts.
| go nm.watchLoop() | ||
| return nil | ||
| } | ||
|
|
||
| func (nm *NotesManager) watchLoop() { | ||
| for { | ||
| select { | ||
| case event, ok := <-nm.watcher.Events: | ||
| if !ok { | ||
| return | ||
| } | ||
| if filepath.Clean(event.Name) != filepath.Clean(nm.notesPath) { | ||
| continue | ||
| } | ||
| if event.Op&(fsnotify.Write|fsnotify.Create|fsnotify.Rename|fsnotify.Remove) == 0 { | ||
| continue | ||
| } | ||
| nm.handleExternalChange() | ||
| case _, ok := <-nm.watcher.Errors: | ||
| if !ok { | ||
| return | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Stop the watcher goroutine from reading mutable manager fields directly.
watchLoop() dereferences nm.watcher and nm.notesPath without synchronization while resetInit() closes the watcher and nils/resets those fields from another goroutine. A config change can race here and produce a data race or nil dereference.
Proposed fix
- nm.watcher = watcher
-
- go nm.watchLoop()
+ nm.watcher = watcher
+
+ go nm.watchLoop(watcher, notesPath)
return nil
}
-func (nm *NotesManager) watchLoop() {
+func (nm *NotesManager) watchLoop(watcher *fsnotify.Watcher, notesPath string) {
for {
select {
- case event, ok := <-nm.watcher.Events:
+ case event, ok := <-watcher.Events:
if !ok {
return
}
- if filepath.Clean(event.Name) != filepath.Clean(nm.notesPath) {
+ if filepath.Clean(event.Name) != filepath.Clean(notesPath) {
continue
}
@@
- case _, ok := <-nm.watcher.Errors:
+ case _, ok := <-watcher.Errors:
if !ok {
return
}
}
}Also applies to: 216-229
🤖 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 `@pkg/notesmanager/notesmanager.go` around lines 152 - 176, watchLoop currently
reads mutable fields nm.watcher and nm.notesPath directly and can race with
resetInit which closes and nils those fields; fix by capturing the watcher and
notesPath into local variables under the NotesManager lock (or by adding a new
mutex if none exists) before entering the select, and use those locals (e.g.
localWatcher := nm.watcher.Events / localNotesPath := nm.notesPath) inside the
loop so the goroutine never dereferences nm.watcher or nm.notesPath
concurrently; ensure you check for nil local watcher and exit or recreate the
loop if resetInit replaced/closed the watcher, and keep calling
nm.handleExternalChange() as before.
| if err := os.WriteFile(nm.tmpPath, []byte(content), 0644); err != nil { | ||
| return fmt.Errorf("writing notes temp file: %w", err) | ||
| } | ||
| if err := os.Rename(nm.tmpPath, nm.notesPath); err != nil { | ||
| return fmt.Errorf("renaming notes temp file: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's standard library, what are the platform-specific semantics of os.Rename when the destination file already exists, especially on Windows?
💡 Result:
In Go’s standard library, os.Rename’s behavior when the destination already exists is: 1) Destination exists and is not a directory - Rename replaces the destination (move/overwrite semantics). This is explicitly documented: “If newpath already exists and is not a directory, Rename replaces it.” [1][2] 2) Destination exists and is a directory - Rename returns an error (it does not replace an existing directory). Docs: “If newpath already exists and is a directory, Rename returns an error.” [2] 3) Atomicity caveat (important on Windows) - Even when replacing a non-directory destination, os.Rename is not guaranteed to be atomic on non-Unix platforms: “Even within the same directory, on non-Unix platforms Rename is not an atomic operation.” [2] Windows-specific semantics (what this means in practice) - For “file to existing file” on Windows, Go uses the Windows MoveFileEx API with MOVEFILE_REPLACE_EXISTING (plus flags for cross-volume moves, depending on Go version/implementation). Discussion and tests in Go’s issue tracker note that os.Rename on Windows is implemented using MoveFileEx and uses MOVEFILE_REPLACE_EXISTING. [3][4] - That flag cannot be used when either the new file name or existing file name is a directory; attempting to rename a directory onto an already-existing directory results in an error (commonly ERROR_ACCESS_DENIED) on Windows. This is captured in the Windows rename-to-existing-directory issue discussion: “MOVEFILE_REPLACE_EXISTING … This value cannot be used if … names a directory,” and subsequent observation of ERROR_ACCESS_DENIED. [4] Related platform documentation gap that motivated clarification efforts - The Go issue tracker notes that the Rename documentation historically didn’t clearly specify “rename a file to an existing directory” behavior, which reflects the underlying Windows constraint above (directories don’t get replaced). [5] Net summary for your question (“destination exists”)—including Windows: - If destination exists and is a file (non-directory): os.Rename replaces it, but is not atomic on Windows/non-Unix platforms. [1][2] - If destination exists and is a directory: os.Rename fails on Windows (cannot replace directory), returning an error. [2][4] If you want, I can also summarize the recommended Windows-safe alternatives (e.g., remove destination then rename, or use a different strategy) based on Go patterns—but the core semantics above are what the standard library specifies.
Citations:
- 1: https://pkg.go.dev/os@go1.19
- 2: https://pkg.go.dev/os
- 3: os: document that os.Rename will overwrite any existing file golang/go#13673
- 4: os: Rename("oldDir", "newExistingDir") doesn't work on Windows golang/go#14527
- 5: os: documentation of
Renamedoesn't specify the behavior of trying to rename a file to an existing directory golang/go#68690
os.Rename is not atomic on Windows when replacing existing files.
After init, nm.notesPath usually already exists. While os.Rename will succeed in replacing the destination on Windows (unlike older OS behavior), it is not guaranteed to be atomic on non-Unix platforms. This means there is a window during the rename operation where the destination file could be inconsistent or temporarily inaccessible, risking data loss or read errors if concurrent access occurs.
🤖 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 `@pkg/notesmanager/notesmanager.go` around lines 307 - 312, The current use of
os.Rename to move nm.tmpPath to nm.notesPath is not atomic on Windows; replace
it with an atomic replace implementation: create the temp file in the same
directory (nm.tmpPath), fsync the file (file.Sync()) and close it, then perform
a platform-aware replace -- on Unix use os.Rename, but on Windows call the
native ReplaceFile/MoveFileEx equivalent (or use a well-tested library such as
github.com/google/renameio) to ensure atomic swap; update the code path that
currently renames nm.tmpPath to nm.notesPath (references to nm.tmpPath and
nm.notesPath) to perform the fsync+atomic-replace sequence.
1 participant
New "notes" widget to use as a scratch pad. Syncs across widget instances and with any changes on disk