Phase1:draft-based notes + safe save/load#2
Open
tani-dubey wants to merge 4 commits intoAOSSIE-Org:mainfrom
Open
Phase1:draft-based notes + safe save/load#2tani-dubey wants to merge 4 commits intoAOSSIE-Org:mainfrom
tani-dubey wants to merge 4 commits intoAOSSIE-Org:mainfrom
Conversation
github-actions
bot
added
no-issue-linked
configuration
dependencies
documentation
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
63-233:⚠️ Potential issue | 🟡 MinorSeveral
markdownlintviolations detected by static analysis.The following issues were flagged by
markdownlint-cli2:
- MD009 – Trailing spaces on lines 67, 70, 73, 76, and 233. Strip the trailing space from each of those lines.
- MD022 – Headings not surrounded by blank lines at lines 86, 91, 95, 104, 112, 137, 143, 148, and 155. Add a blank line before and after each
###/-###heading.- MD040 – Fenced code block without language at line 122 (the architecture directory tree). Add a language tag (e.g.,
```text).- MD031 – Fenced code block not surrounded by blank lines at line 136.
- MD047 – File should end with a single newline character (line 233).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 63 - 233, Remove trailing spaces on the lines in the Core Editor list and at EOF; ensure the file ends with a single newline. Surround each third-level heading (e.g., "### Desktop Application", "### Storage", "### AI/ML (Planned)", "### Planned Extension", and other "###" headings flagged) with a blank line before and after. Add a language tag (e.g., ```text) to the fenced architecture tree block and ensure the fenced code block is separated by blank lines above and below. Apply these fixes to the README's architecture/code-block section and all listed headings so markdownlint MD009, MD022, MD040, MD031, and MD047 are resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/index.html`:
- Around line 2-7: Add accessibility and security metadata to the HTML head: set
the <html> tag's lang attribute (e.g., lang="en"), add a responsive viewport
meta (<meta name="viewport" content="width=device-width, initial-scale=1">), and
insert a restrictive Content-Security-Policy meta (e.g., a policy that limits
default-src, allows scripts/styles only from 'self' and/or nonces, restricts
object/frame sources, and permits images from 'self' and data:) to mitigate
injection risks given nodeIntegration; place these tags inside the existing
<head> block so Lighthouse accessibility and security audits pass.
- Around line 10-21: The layout breaks because `#editor-toolbar` is a sibling of
`#sidebar` and `#editor` under the row-flex `#container`; move `#editor-toolbar` inside
a new wrapper that contains the editor so toolbar stacks above the textarea.
Wrap the toolbar and textarea together (e.g., a div with id like
"editor-wrapper" or "editor-column") and make that wrapper the third child of
`#container`, then ensure the wrapper uses column flex-direction so
`#editor-toolbar` sits above `#editor`; update any CSS selectors if needed to target
the new wrapper instead of the top-level `#container`.
In `@app/main.js`:
- Around line 17-19: Replace the existsSync + mkdirSync pattern with a single
recursive mkdir call to avoid TOCTOU: remove the if-block that uses
fs.existsSync and instead call fs.mkdirSync(notesDir, { recursive: true }) where
notesDir is defined; this will create the directory and any parents if missing
and silently succeed if it already exists (handle/propagate any thrown errors
from fs.mkdirSync as appropriate).
- Around line 133-165: The draft rename path in saveBtn.onclick can silently
overwrite an existing note because finalName is set to `${safeName}.md` (using
titleToFilename/getTitleFromEditor) and fs.renameSync(filePath, finalPath) is
called without collision checks; also titleToFilename may return "untitled"
which must instead use getNextUntitled. Fix by checking for an existing file at
finalPath (e.g., fs.existsSync or fs.statSync) before renaming; if it exists,
call getNextUntitled("untitled") or derive a unique name (e.g., append a numeric
suffix) to produce a non-colliding finalName, then recompute finalPath and call
fs.renameSync only once with that unique path; ensure the logic in the branch
that handles isEditorEmpty(), titleToFilename(), and assignment to
currentNote/isDraft is updated accordingly.
- Around line 63-77: The draft file is created empty on first keystroke so typed
text can be lost; instead of writing an empty string in the
editor.addEventListener("input") handler, write the current editor content to
the draft file (filePath) using a debounced autosave routine so we don't flood
disk on every keystroke. Modify the input handler that creates
currentNote/isDraft to: create the draft filename as now, then start or call a
debounce function (e.g., debounceAutosave or autosaveTimer tied to the editor
input handler) that calls fs.writeFileSync or fs.promises.writeFile(filePath,
getEditorContent()) after a short delay, clear/reset the timer on further input,
and ensure you also flush the latest content on explicit save and on window
beforeunload to avoid losing recent edits.
- Line 14: Remove the unused module‑scope variable `count` that shadows the
inner `count` in `getNextUntitled`: delete the top-level declaration `let count
= 0;` in app/main.js so only the local `let count = 1` inside `getNextUntitled`
remains, ensuring there is no dead code or shadowing of the `count` identifier.
- Line 65: Remove the dead commented-out line declaring safeName: delete the
commented line "// const safeName = titleToFilename( getTitleFromEditor());"
from app/main.js so there is no leftover development artifact; ensure no other
code references the identifier safeName/titleToFilename/getTitleFromEditor
remain broken after removal.
- Around line 96-107: The loadNote function currently clears editor.value and
sets isDirty = false without checking for unsaved edits; update loadNote to
first check isDirty (and currentNote) and prompt or auto-save before
switching—use the same guard logic used elsewhere but ensure it handles both
draft and non-draft notes (i.e., if currentNote && isDirty then call the
existing save routine or show a confirmation). Also modify the
newNoteBtn.onclick handler to remove the condition that requires isDraft when
saving current edits (replace the guard if (currentNote && isDraft && isDirty)
with if (currentNote && isDirty) so non-draft unsaved edits are preserved or
confirmed). Ensure you reference and call the same save function used elsewhere
(e.g., saveNote or existing save logic) and then proceed to update currentNote,
editor.value, isDraft, isDirty, and updateSaveButton().
- Around line 82-93: loadNotes presently uses the blocking fs.readdirSync and
exposes every file in notesDir; change loadNotes to be async (e.g., async
function loadNotes()) and use a non-blocking API (fs.promises.readdir or
fs.readdir callback) to avoid freezing the renderer, then filter the returned
filenames to only include Markdown files (e.g., files ending with ".md") and
ignore hidden/system files (e.g., names starting with "."); after awaiting the
async read, clear notesList.innerHTML and append list items that call
loadNote(file) as before, and add basic error handling to log or surface read
errors.
In `@app/style.css`:
- Around line 49-52: The global button { width: 100% } rule is making `#save-note`
expand full-width; either scope the generic rule to the sidebar (e.g., use
.sidebar button or `#sidebar` button so `#new-note` keeps full width only in that
area) or add an explicit width override for the toolbar save button (add a rule
for `#save-note` to set width: auto or width: initial) and remove/limit the global
button width rule; update the selector that currently defines button { width:
100%; } and add the `#save-note` width override if you prefer keeping the global
rule.
- Around line 17-22: The `#editor` textarea is still showing the browser default
border, outline and resize handle; update the `#editor` CSS rule to reset these by
removing the border and outline, disabling resizing, and clearing any default
focus/appearance (e.g., set border: none; outline: none; resize: none; and
optional -webkit-appearance: none; box-shadow: none) so the textarea renders as
a seamless editor element.
In `@main.js`:
- Around line 1-17: Remove or use the unused "path" import and add macOS
lifecycle handlers: either delete the const path = require("path") import or
change win.loadFile("app/index.html") to use path.join(__dirname, "app",
"index.html") so the import is used; add an app.on('window-all-closed') handler
that calls app.quit() only when process.platform !== 'darwin'; and add an
app.on('activate') handler that re-creates the window by calling createWindow()
when BrowserWindow.getAllWindows().length === 0. Ensure these changes reference
the existing createWindow and win.loadFile usage.
- Around line 8-11: The renderer is created with webPreferences.nodeIntegration:
true and contextIsolation: false which allows renderer JS full Node access;
change webPreferences to disable nodeIntegration and enable contextIsolation,
add a preload script and use contextBridge to expose only the minimal API needed
(e.g., implement a preload that calls contextBridge.exposeInMainWorld with a
notesAPI exposing whitelist functions like ensureDir, readDir, readFile,
writeFile, renameFile, exists) and update renderer code to call
window.notesAPI.* instead of using fs/Node primitives directly; locate
webPreferences in the BrowserWindow creation, add a preload option pointing at
the new preload module, and create the preload module that exports the safe
functions via contextBridge.
In `@package.json`:
- Around line 1-11: The repo currently ignores package-lock.json which prevents
reproducible installs; update .gitignore to remove any package-lock.json entry,
run npm install (or npm ci) in the project root to generate a fresh
package-lock.json, add and commit the package-lock.json to the repo, and ensure
CI/build steps use the lockfile to install exact dependency versions; reference
package.json (scripts/start, devDependencies) to locate the Node/Electron
project context when applying this change.
- Line 9: Update the Electron dependency in package.json to a current supported
stable release: replace the "electron" entry currently set to "^28.0.0" with the
latest stable version (e.g. "^40.6.0"); after changing the version, run your
package manager (npm/yarn/pnpm) to reinstall and then smoke-test the app for any
breaking API changes, addressing required API updates discovered during testing
(look for uses of Electron APIs in main.js, preload scripts, and any native
modules that may need rebuilding).
---
Outside diff comments:
In `@README.md`:
- Around line 63-233: Remove trailing spaces on the lines in the Core Editor
list and at EOF; ensure the file ends with a single newline. Surround each
third-level heading (e.g., "### Desktop Application", "### Storage", "### AI/ML
(Planned)", "### Planned Extension", and other "###" headings flagged) with a
blank line before and after. Add a language tag (e.g., ```text) to the fenced
architecture tree block and ensure the fenced code block is separated by blank
lines above and below. Apply these fixes to the README's architecture/code-block
section and all listed headings so markdownlint MD009, MD022, MD040, MD031, and
MD047 are resolved.
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/index.html`:
- Around line 12-26: The HTML is missing the closing tag for the `#container`
wrapper: add a matching </div> to close the <div id="container"> (so both
`#sidebar` and `#main-area` are contained and the layout remains valid) before the
<script src="main.js"></script> line; ensure the closing tag is placed after the
</div> that ends `#main-area` so the <script> is not nested inside `#container`.
In `@app/main.js`:
- Around line 22-30: The updateSaveButton function currently uses hardcoded
user-visible strings ("💾 Save" and "✅ Saved") and index.html contains "+ New
Note" and "Write markdown here..." which violates i18n rules; extract these
strings into your localization/resource file (e.g., add keys like
save_button.save, save_button.saved, new_note.label, editor.placeholder), load
the localized bundle where the UI initializes, and replace the inline literals
in updateSaveButton (referencing saveBtn and isDirty) and app/index.html with
lookups into the resource object so all user-facing text is driven by the i18n
layer.
- Around line 65-73: autoSaveIfDirty, loadNote, newNoteBtn.onclick and
saveBtn.onclick currently perform synchronous fs operations (fs.writeFileSync,
fs.renameSync, etc.) without error handling which can crash the renderer; wrap
each critical fs.*Sync call inside try-catch blocks to catch and handle errors,
surface failures to the user (e.g., alert/showError), and ensure invariants on
failure: specifically in saveBtn.onclick where filePath is reassigned to
finalPath before calling fs.renameSync, avoid mutating filePath until rename
succeeds or rollback the variable if rename fails so subsequent fs.writeFileSync
doesn't target a non-existent path (alternatively perform write to a temp path
then rename inside the same try block); apply the same try-catch +
user-notification pattern to autoSaveIfDirty (function autoSaveIfDirty),
loadNote (function loadNote), newNoteBtn.onclick and saveBtn.onclick (event
handlers) to prevent unhandled exceptions.
- Around line 77-81: The editor textarea is editable on first render causing
typed content to be lost because the input handler returns early when
currentNote is null; make the textarea initially disabled/readOnly in the HTML
so users cannot edit until a note exists, and rely on the existing enabling
logic in newNoteBtn.onclick and loadNote (which call editor.readOnly = false and
editor.disabled = false) to activate the editor when a note is created or
loaded.
In `@app/style.css`:
- Around line 11-15: The `#sidebar` rule currently sets width: 200px and padding:
10px which makes its rendered width 220px; update the `#sidebar` CSS to use
box-sizing: border-box so the padding is included in the 200px width; add
box-sizing: border-box to the `#sidebar` selector (or to a broader layout rule if
appropriate) to preserve the intended widths without changing other layout
calculations.
---
Duplicate comments:
In `@app/main.js`:
- Around line 85-104: loadNotes currently uses the blocking fs.readdirSync which
will block the renderer; change loadNotes to an async function and use the
non-blocking API (e.g., await fs.promises.readdir(notesDir) or fs.readdir with a
callback), then apply the existing .filter(f => f.endsWith(".md")) and .sort()
to the returned array, build the <li> elements and append to notesList as
before, and add try/catch to handle and log any filesystem errors; ensure calls
to autoSaveIfDirty() and loadNote(file) remain unchanged inside the click
handler so behavior is preserved.
- Around line 16-18: The current TOCTOU pattern uses fs.existsSync(notesDir)
followed by fs.mkdirSync(notesDir); remove the existence check and create the
directory with recursion enabled to avoid race conditions—replace the pair with
a single mkdir call that uses the { recursive: true } option (or the equivalent
fs.promises.mkdir(notesDir, { recursive: true })) so the code that ensures the
notes directory exists is atomic; update the area around the notesDir creation
in app/main.js accordingly.
- Around line 65-73: Add a window "beforeunload" handler that checks isDirty and
either flushes changes by calling autoSaveIfDirty() synchronously or prompts the
user to confirm closing: in main.js (during app init) add
window.addEventListener('beforeunload', ...) that if isDirty calls
autoSaveIfDirty() and, if you prefer an explicit prompt, sets
event.preventDefault() and event.returnValue to a message so the browser shows a
confirmation; reference autoSaveIfDirty, isDirty, currentNote and
updateSaveButton to ensure the same save/dirty logic is reused.
In `@app/style.css`:
- Around line 58-61: The global button selector is forcing `#save-note` to 100%
width; narrow the rule's scope so toolbar buttons aren't affected by replacing
the global "button { width: 100%; }" with a more specific selector that targets
the sidebar controls (for example ".sidebar button" or "#sidebar button") while
keeping margin-bottom: 10px; ensure the existing `#save-note` and `#editor-toolbar`
selectors remain unchanged so the Save button no longer stretches full-width.
In `@package.json`:
- Around line 8-10: The package.json devDependency for "electron" was bumped to
"^40.0.0" and the reviewer approved the change; ensure you finalize this by
running a fresh install to update the lockfile (npm/yarn/pnpm) so
package-lock.json or yarn.lock reflects "^40.0.0", run the app's CI/test suite
to validate compatibility, and, if present, update the package.json "engines" or
any Electron-specific compatibility metadata (and any Docker/CI images
referencing older Electron majors) to match the new ^40.0.0 release.
tani-dubey
commented
Feb 21, 2026
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/index.html`:
- Line 26: The script tag that loads main.js should include the defer attribute
to ensure non-blocking, forward-compatible execution; update the <script
src="main.js"></script> element to include defer (i.e., <script src="main.js"
defer></script>) so the script is downloaded asynchronously and executed after
parsing without changing placement at the end of the body.
- Around line 13-23: Replace the non-semantic containers with semantic elements
and make buttons explicit: change the <div id="sidebar"> to <aside id="sidebar">
and <div id="main-area"> to <main id="main-area"> to improve accessibility, and
add type="button" attributes to the buttons with ids "new-note" and "save-note"
so they do not default to submit behavior; keep the existing ids ("sidebar",
"main-area", "new-note", "save-note", "notes-list", "editor-toolbar", "editor")
and attributes (e.g., textarea disabled/placeholder) unchanged otherwise.
In `@app/main.js`:
- Around line 235-238: Add a beforeunload guard in the renderer (near
loadNotes()/updateSaveButton()) that checks the global isDirty flag and, if
true, cancels unloading (use event.preventDefault() and set returnValue or
return a string as appropriate) so users get a prompt when closing with unsaved
changes; additionally, in the Electron main process attach a will-prevent-unload
listener on mainWindow.webContents that shows a dialog (via
dialog.showMessageBoxSync) offering Leave/Stay and only call
event.preventDefault() based on the user's choice so the renderer's beforeunload
is respected.
- Around line 29-33: Replace uses of saveBtn.innerText with saveBtn.textContent
in the save state toggling block: update both occurrences where the code sets
"💾 Save" and "✅ Saved" so they use textContent instead of innerText; keep the
existing classList.add/remove calls (saveBtn) unchanged.
- Around line 74-87: The autoSaveIfDirty function must not swallow failures —
change it to return a boolean (true on success/no-op, false on failure) or
rethrow the error so callers can abort; specifically update autoSaveIfDirty() to
return true when save succeeds or nothing needs saving, and return false (or
throw) after calling showError(...) when fs.writeFileSync fails. Then update the
two callers (the loadNotes click handler and newNoteBtn.onclick) to check the
return value and stop the navigation/new-note flow if autoSaveIfDirty indicates
failure (i.e., if it returns false or throws), ensuring unsaved edits are not
discarded when auto-save fails. Ensure you reference autoSaveIfDirty(),
loadNotes click handler, and newNoteBtn.onclick in your changes.
---
Duplicate comments:
In `@app/main.js`:
- Around line 90-94: The app only marks drafts dirty on editor input but never
persists them until note-switch/new-note, so mid-edit crashes lose content;
modify the editor input handler (the listener on editor) to call autoSaveIfDirty
(preferably debounced) whenever isDirty is set true so drafts are written to
disk during editing; update the handler that currently sets isDirty and calls
updateSaveButton to also invoke autoSaveIfDirty (or schedule it via a short
timeout) and ensure newNoteBtn.onclick still initializes an empty file but is
overridden by subsequent saves from autoSaveIfDirty.
- Around line 16-19: Remove the TOCTOU-prone existsSync check and instead create
the directory with recursion enabled: replace the if (!fs.existsSync(notesDir))
{ fs.mkdirSync(notesDir) } sequence with a single fs.mkdirSync(notesDir, {
recursive: true }) call (inside the existing try/catch), referencing notesDir
and the fs.mkdirSync call so the directory creation is atomic and safe against
race conditions.
- Around line 103-106: The current synchronous directory read using
fs.readdirSync (the assignment to files where readdirSync(notesDir) is used)
blocks the renderer; change this to the async API (fs.promises.readdir or
fs.readdir with a callback/await) inside an async function or promise chain:
call fs.promises.readdir(notesDir) and await it (or .then), then apply the
existing .filter(f => f.endsWith(".md")) and .sort() on the returned array, and
add error handling to surface failures instead of blocking the event loop.
- Around line 46-56: getNextUntitled currently calls fs.existsSync and can throw
on I/O/permission errors; wrap the body of getNextUntitled in a try-catch that
catches errors from fs.existsSync (and returns a sensible fallback or rethrows a
controlled error) and then wrap every call site (newNoteBtn.onclick and
saveBtn.onclick) in an outer try-catch that calls showError and returns early on
failure; additionally protect the while loop inside saveBtn.onclick that
directly uses fs.existsSync with a try-catch guard so any filesystem exception
is caught, routed to showError, and prevents the renderer from crashing (refer
to getNextUntitled, newNoteBtn.onclick, saveBtn.onclick, showError,
fs.existsSync, notesDir).
Comment on lines
+13
to
+23
| <div id="sidebar"> | ||
| <button id="new-note">+ New Note</button> | ||
| <ul id="notes-list"></ul> | ||
| </div> | ||
|
|
||
| <div id="main-area"> | ||
| <div id="editor-toolbar"> | ||
| <button id="save-note">💾 Save</button> | ||
| </div> | ||
| <textarea id="editor" placeholder="Write markdown here..." disabled></textarea> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use semantic HTML elements and add explicit type="button" to buttons.
<div> is used for both the sidebar and main area. <aside> and <main> would be semantically correct and improve accessibility. Additionally, <button> elements outside a form technically default to type="submit" per spec — being explicit prevents any unintended behavior.
🛠️ Proposed fix
- <div id="sidebar">
+ <aside id="sidebar">
- <button id="new-note">+ New Note</button>
+ <button id="new-note" type="button">+ New Note</button>
<ul id="notes-list"></ul>
- </div>
+ </aside>
- <div id="main-area">
+ <main id="main-area">
<div id="editor-toolbar">
- <button id="save-note">💾 Save</button>
+ <button id="save-note" type="button">💾 Save</button>
</div>
<textarea id="editor" placeholder="Write markdown here..." disabled></textarea>
- </div>
+ </main>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div id="sidebar"> | |
| <button id="new-note">+ New Note</button> | |
| <ul id="notes-list"></ul> | |
| </div> | |
| <div id="main-area"> | |
| <div id="editor-toolbar"> | |
| <button id="save-note">💾 Save</button> | |
| </div> | |
| <textarea id="editor" placeholder="Write markdown here..." disabled></textarea> | |
| </div> | |
| <aside id="sidebar"> | |
| <button id="new-note" type="button">+ New Note</button> | |
| <ul id="notes-list"></ul> | |
| </aside> | |
| <main id="main-area"> | |
| <div id="editor-toolbar"> | |
| <button id="save-note" type="button">💾 Save</button> | |
| </div> | |
| <textarea id="editor" placeholder="Write markdown here..." disabled></textarea> | |
| </main> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/index.html` around lines 13 - 23, Replace the non-semantic containers
with semantic elements and make buttons explicit: change the <div id="sidebar">
to <aside id="sidebar"> and <div id="main-area"> to <main id="main-area"> to
improve accessibility, and add type="button" attributes to the buttons with ids
"new-note" and "save-note" so they do not default to submit behavior; keep the
existing ids ("sidebar", "main-area", "new-note", "save-note", "notes-list",
"editor-toolbar", "editor") and attributes (e.g., textarea disabled/placeholder)
unchanged otherwise.
| </div> | ||
| </div> | ||
|
|
||
| <script src="main.js"></script> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add defer to the <script> tag.
Placing the script at the end of <body> avoids render-blocking, but Lighthouse and the Google HTML style guide still recommend defer for explicit, forward-compatible non-blocking loading.
🛠️ Proposed fix
- <script src="main.js"></script>
+ <script src="main.js" defer></script>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <script src="main.js"></script> | |
| <script src="main.js" defer></script> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/index.html` at line 26, The script tag that loads main.js should include
the defer attribute to ensure non-blocking, forward-compatible execution; update
the <script src="main.js"></script> element to include defer (i.e., <script
src="main.js" defer></script>) so the script is downloaded asynchronously and
executed after parsing without changing placement at the end of the body.
Comment on lines
+29
to
+33
| saveBtn.innerText = "💾 Save"; | ||
| saveBtn.classList.add("unsaved"); | ||
| } else { | ||
| saveBtn.innerText = "✅ Saved"; | ||
| saveBtn.classList.remove("unsaved"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer textContent over innerText for plain-text updates.
innerText triggers a layout reflow to account for CSS visibility. Since these strings contain no HTML, textContent is correct and avoids the unnecessary reflow.
♻️ Proposed fix
- saveBtn.innerText = "💾 Save";
+ saveBtn.textContent = "💾 Save";
...
- saveBtn.innerText = "✅ Saved";
+ saveBtn.textContent = "✅ Saved";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/main.js` around lines 29 - 33, Replace uses of saveBtn.innerText with
saveBtn.textContent in the save state toggling block: update both occurrences
where the code sets "💾 Save" and "✅ Saved" so they use textContent instead of
innerText; keep the existing classList.add/remove calls (saveBtn) unchanged.
Comment on lines
+74
to
+87
| function autoSaveIfDirty() { | ||
| if (!currentNote || !isDirty) return; | ||
|
|
||
| try { | ||
| fs.writeFileSync( | ||
| path.join(notesDir, currentNote), | ||
| editor.value | ||
| ); | ||
| isDirty = false; | ||
| updateSaveButton(); | ||
| } catch (err) { | ||
| showError("Failed to auto-save note.", err); | ||
| } | ||
| } |
There was a problem hiding this comment.
autoSaveIfDirty silently swallows failure — callers proceed with navigation and discard unsaved content.
When fs.writeFileSync throws, showError is called but the function returns undefined. Both call sites (loadNotes click handler at Line 117 and newNoteBtn.onclick at Line 152) call autoSaveIfDirty() and immediately continue regardless. If the auto-save fails (e.g., disk full), the current note's unsaved edits are permanently lost as the editor is overwritten by the subsequent loadNote or cleared by the new-note flow.
🐛 Proposed fix
function autoSaveIfDirty() {
- if (!currentNote || !isDirty) return;
+ if (!currentNote || !isDirty) return true;
try {
fs.writeFileSync(path.join(notesDir, currentNote), editor.value);
isDirty = false;
updateSaveButton();
+ return true;
} catch (err) {
showError("Failed to auto-save note.", err);
+ return false;
}
}Then guard callers:
// In loadNotes click handler (line 116-119):
li.addEventListener("click", () => {
- autoSaveIfDirty();
- loadNote(file);
+ if (autoSaveIfDirty()) loadNote(file);
});
// In newNoteBtn.onclick (line 152):
- autoSaveIfDirty();
+ if (!autoSaveIfDirty()) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/main.js` around lines 74 - 87, The autoSaveIfDirty function must not
swallow failures — change it to return a boolean (true on success/no-op, false
on failure) or rethrow the error so callers can abort; specifically update
autoSaveIfDirty() to return true when save succeeds or nothing needs saving, and
return false (or throw) after calling showError(...) when fs.writeFileSync
fails. Then update the two callers (the loadNotes click handler and
newNoteBtn.onclick) to check the return value and stop the navigation/new-note
flow if autoSaveIfDirty indicates failure (i.e., if it returns false or throws),
ensuring unsaved edits are not discarded when auto-save fails. Ensure you
reference autoSaveIfDirty(), loadNotes click handler, and newNoteBtn.onclick in
your changes.
Comment on lines
+235
to
+238
| // -------------------- INIT -------------------- | ||
|
|
||
| loadNotes(); | ||
| updateSaveButton(); No newline at end of file |
There was a problem hiding this comment.
Missing window.beforeunload handler — closing the app with unsaved changes silently discards them.
There is no guard against accidental window close while isDirty === true. The renderer needs a beforeunload listener, and the Electron main process should handle the resulting will-prevent-unload event on webContents — calling event.preventDefault() on it ignores the beforeunload handler and allows the page to unload; otherwise, dialog.showMessageBoxSync can offer a Stay/Leave choice.
🐛 Proposed fix (renderer — app/main.js)
loadNotes();
updateSaveButton();
+
+// Prevent accidental close with unsaved changes
+window.addEventListener("beforeunload", (e) => {
+ if (isDirty) {
+ e.preventDefault();
+ e.returnValue = ""; // triggers will-prevent-unload in Electron main process
+ }
+});Then in main.js (Electron main process), handle the event:
mainWindow.webContents.on("will-prevent-unload", (event) => {
const { dialog } = require("electron");
const choice = dialog.showMessageBoxSync(mainWindow, {
type: "question",
buttons: ["Leave", "Stay"],
title: "Unsaved Changes",
message: "You have unsaved changes. Leave anyway?",
defaultId: 1,
cancelId: 1,
});
if (choice === 0) event.preventDefault(); // allow close
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/main.js` around lines 235 - 238, Add a beforeunload guard in the renderer
(near loadNotes()/updateSaveButton()) that checks the global isDirty flag and,
if true, cancels unloading (use event.preventDefault() and set returnValue or
return a string as appropriate) so users get a prompt when closing with unsaved
changes; additionally, in the Electron main process attach a will-prevent-unload
listener on mainWindow.webContents that shows a dialog (via
dialog.showMessageBoxSync) offering Leave/Stay and only call
event.preventDefault() based on the user's choice so the renderer's beforeunload
is respected.
tani-dubey
commented
Feb 21, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/style.css`:
- Around line 11-15: The `#sidebar` rule currently uses box-sizing but lacks
vertical scroll behavior; update the `#sidebar` CSS selector to add overflow-y:
auto so that when the notes list exceeds the sidebar height it becomes
scrollable and content doesn't clip; ensure the property is added alongside
width/padding in the existing `#sidebar` block.
In `@main.js`:
- Around line 4-17: The BrowserWindow instance referenced by the module-level
variable mainWindow isn't nulled when the window is closed; update createWindow
to attach a 'closed' listener to mainWindow (e.g., mainWindow.on('closed', ...)
) that sets the module variable mainWindow = null so the GC can reclaim the
BrowserWindow; ensure you add the listener immediately after creating mainWindow
in the createWindow function and before returning/ending the function.
---
Duplicate comments:
In `@app/style.css`:
- Around line 58-61: The global CSS rule button { width: 100% } is unscoped and
causes the toolbar button `#save-note` to stretch; restrict the rule to the
sidebar by changing the selector to target `#sidebar` button (or `#sidebar` .toolbar
button) so only buttons inside `#sidebar` get width: 100% and the global
`#save-note` toolbar button is unaffected; update the selector that currently
defines button { width: 100%; margin-bottom: 10px; } to the scoped selector
referencing `#sidebar` and leave margin rule as needed.
In `@main.js`:
- Around line 10-13: In main.js change the BrowserWindow webPreferences to set
nodeIntegration: false and contextIsolation: true, add a preload script
(referenced via the webPreferences.preload property) and move any renderer-side
use of require/fs/child_process into that preload; implement the preload to use
Electron's contextBridge to expose a minimal, whitelisted API (only the
functions the renderer needs) and update renderer code to call that exposed API
instead of accessing Node globals directly (search for webPreferences,
nodeIntegration, contextIsolation, preload, contextBridge to locate the affected
code).
Comment on lines
+11
to
+15
| #sidebar { | ||
| width: 200px; | ||
| padding: 10px; | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
box-sizing: border-box addressed — but #sidebar needs overflow-y: auto.
The box-sizing fix is correct. However, as the notes list grows beyond the sidebar height, content will silently clip with no scroll affordance. Add overflow-y: auto to keep the sidebar scrollable.
♻️ Proposed fix
`#sidebar` {
width: 200px;
padding: 10px;
box-sizing: border-box;
+ overflow-y: auto;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #sidebar { | |
| width: 200px; | |
| padding: 10px; | |
| box-sizing: border-box; | |
| } | |
| `#sidebar` { | |
| width: 200px; | |
| padding: 10px; | |
| box-sizing: border-box; | |
| overflow-y: auto; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/style.css` around lines 11 - 15, The `#sidebar` rule currently uses
box-sizing but lacks vertical scroll behavior; update the `#sidebar` CSS selector
to add overflow-y: auto so that when the notes list exceeds the sidebar height
it becomes scrollable and content doesn't clip; ensure the property is added
alongside width/padding in the existing `#sidebar` block.
Comment on lines
+4
to
+17
| let mainWindow; | ||
|
|
||
| function createWindow() { | ||
| mainWindow = new BrowserWindow({ | ||
| width: 900, | ||
| height: 600, | ||
| webPreferences: { | ||
| nodeIntegration: true, | ||
| contextIsolation: false | ||
| } | ||
| }); | ||
|
|
||
| mainWindow.loadFile(path.join(__dirname, "app/index.html")); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Null mainWindow on window close to release the GC reference.
mainWindow holds a reference to the destroyed BrowserWindow instance until the next createWindow() call (macOS only). Adding a 'closed' listener is the standard Electron pattern.
♻️ Proposed refactor
function createWindow() {
mainWindow = new BrowserWindow({
width: 900,
height: 600,
webPreferences: {
nodeIntegration: true,
contextIsolation: false
}
});
mainWindow.loadFile(path.join(__dirname, "app/index.html"));
+
+ mainWindow.on("closed", () => {
+ mainWindow = null;
+ });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main.js` around lines 4 - 17, The BrowserWindow instance referenced by the
module-level variable mainWindow isn't nulled when the window is closed; update
createWindow to attach a 'closed' listener to mainWindow (e.g.,
mainWindow.on('closed', ...) ) that sets the module variable mainWindow = null
so the GC can reclaim the BrowserWindow; ensure you add the listener immediately
after creating mainWindow in the createWindow function and before
returning/ending the function.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
This PR implements and stabilizes the core local-first markdown editor foundation for Smart Notes, including draft handling, dynamic file naming, and safe file persistence.
Screenshots/Recordings:
📝 Draft Creation
Typing in a new note automatically creates a draft file in the sidebar.
part1.mp4
🏷️ Dynamic Title Generation
The filename updates based on the first line of content.
part2.mp4
💾 Explicit Save & Dirty Tracking
Unsaved changes are tracked and finalized on Save.
part3.mp4
🔁 Safe Note Switching
Switching between notes preserves state and prevents overwrites.
part4.mp4
Additional Notes:
This PR establishes the core architecture of Smart Notes as a local-first, privacy-focused desktop application.
Key features implemented:
AI features (semantic search, embeddings, RAG) are intentionally excluded in this milestone and planned for future development.
This PR delivers a stable, maintainable editor foundation aligned with the project's local-first philosophy.
Checklist
Summary by CodeRabbit
New Features
Documentation
Chores