Repository navigation
Update ensure_note to preserve content when content is omitted during updates - #66
Merged
Merged
Conversation
Updated the `ensure_note` tool logic to make the `content` parameter optional during updates. If the `content` field is omitted when updating an existing note, the current content will now be preserved instead of being overwritten with an empty string. Documentation and test coverage have been updated to reflect this behavior.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesNote update behavior
Note management and search
Task link and unlink flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
electron/ipc/chat-executor.test.ts (1)
439-449: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the explicit empty-string case too.
This change hinges on distinguishing omitted
contentfrom an intentional clear, but the new coverage only exercises omission. Please add a companion assertion forcontent: ""so both sides of the contract stay protected.🤖 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 `@electron/ipc/chat-executor.test.ts` around lines 439 - 449, The new test in chat-executor.test.ts only covers the omitted-content path for ensure_note; add a companion assertion in the same test or a nearby test that calls exec with content: "" and verifies the note is cleared, while the omitted-content case still preserves the original content. Use the ensure_note and get_note flows already present so both behaviors remain covered by the test suite.
🤖 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 `@electron/mcp/tools/notes.ts`:
- Line 57: The note update path is incorrectly reusing `existing?.content` from
the snapshot when `content` is omitted, which can overwrite newer persisted note
text. In `ensure_note` and the `updateNote` call in
`electron/mcp/tools/notes.ts`, keep `content` and `contentText` undefined when
the argument is missing so the DB layer’s omission-safe `COALESCE` behavior in
`queries.ts` preserves stored content. Then use the row returned by
`q.updateNote(...)` as the source for `writeNoteFile` instead of reconstructing
content from `snap.notes`.
---
Nitpick comments:
In `@electron/ipc/chat-executor.test.ts`:
- Around line 439-449: The new test in chat-executor.test.ts only covers the
omitted-content path for ensure_note; add a companion assertion in the same test
or a nearby test that calls exec with content: "" and verifies the note is
cleared, while the omitted-content case still preserves the original content.
Use the ensure_note and get_note flows already present so both behaviors remain
covered by the test suite.
🪄 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: 0c605f3a-5a52-4f82-9f27-856f9c761a54
📒 Files selected for processing (4)
electron/ipc/chat-executor.test.tselectron/lib/tool-schemas.tselectron/lib/tools.tselectron/mcp/tools/notes.ts
Updates the `ensure_note` logic to distinguish between omitting the content argument (which retains existing content) and explicitly passing an empty string (which clears the content). Previously, the implementation treated both cases identically by falling back to the existing content. Test coverage was added to verify that empty string inputs now correctly update the note content to be empty.
- Added `unlink_note_from_task` tool to remove connections between task cards and notes. - Updated `get_task` to include `blockingCardIds` in its output, allowing for easier dependency discovery. - Updated `link_note_to_task` to synchronize metadata with the physical file system via `writeNoteFile`. - Added unit tests for new linking and unlinking functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
electron/mcp/tools/tasks.ts (1)
124-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional: extract shared link/unlink logic.
unlink_note_from_taskis almost a line-for-line copy oflink_note_to_task(snapshot lookup, transaction,writeNoteFile, notification), differing only in set union vs. filter and the notification text/return key. A small shared helper would reduce drift risk between the two 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 `@electron/mcp/tools/tasks.ts` around lines 124 - 150, `unlink_note_from_task` duplicates most of `link_note_to_task`, so factor the shared snapshot lookup, transaction/update, `writeNoteFile`, and notification flow into a common helper to reduce drift. Keep the link/unlink-specific parts in the wrappers: the collection transform on `linkedCardIds`/`linkedNoteIds`, the notification text, and the returned status key. Use the existing `link_note_to_task` and `unlink_note_from_task` symbols to consolidate the repeated logic without changing behavior.
🤖 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 `@electron/mcp/tools/tasks.ts`:
- Around line 96-150: `link_note_to_task` and `unlink_note_from_task` write note
files via `writeNoteFile` without the `aiWriteLock` protection, so concurrent
note edits can clobber frontmatter updates. Update the note-write flow in
`tasks.ts` to use the same lock/unlock pattern as `ensure_note`,
`append_to_note`, and `patch_note`, or refactor to a shared locked write helper
so `noteId` is always guarded before calling `writeNoteFile`.
---
Nitpick comments:
In `@electron/mcp/tools/tasks.ts`:
- Around line 124-150: `unlink_note_from_task` duplicates most of
`link_note_to_task`, so factor the shared snapshot lookup, transaction/update,
`writeNoteFile`, and notification flow into a common helper to reduce drift.
Keep the link/unlink-specific parts in the wrappers: the collection transform on
`linkedCardIds`/`linkedNoteIds`, the notification text, and the returned status
key. Use the existing `link_note_to_task` and `unlink_note_from_task` symbols to
consolidate the repeated logic without changing behavior.
🪄 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: f899add7-2c4a-4f96-a9d8-818a3643291f
📒 Files selected for processing (7)
changelogs/v2.2.7.mdelectron/ipc/chat-executor.test.tselectron/ipc/chat-executor.tselectron/lib/tool-schemas.tselectron/lib/tools.tselectron/mcp/tools/index.tselectron/mcp/tools/tasks.ts
✅ Files skipped from review due to trivial changes (1)
- changelogs/v2.2.7.md
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/lib/tools.ts
- Added `rename_note` tool to safely rename notes and automatically update internal wikilinks. - Added `bulk_move_notes` tool for efficient batch reorganization of notes. - Added `list_folders` tool to retrieve a project's active directory tree. - Enhanced `search_notes` with `offset` for pagination and `updatedAfter` for temporal filtering. - Implemented corresponding IPC handlers and unit tests to ensure stability and proper mutex locking for file operations.
Consolidated logic for linking and unlinking notes from tasks into a shared `applyNoteLinkChange` helper function. Introduced note locking during database transactions to prevent race conditions or partial updates when syncing note files.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@electron/ipc/chat-executor.test.ts`:
- Around line 596-604: The offset pagination test in search_notes only verifies
result length, so it can pass even if offset is ignored. Update the test setup
around makeDb, seed, and createNote to use deterministic updated_at values, then
assert the exact IDs and ordering returned by exec(..., "search_notes", ...) for
the second page so the test proves offset is actually applied.
In `@electron/lib/tool-schemas.ts`:
- Around line 63-67: The search_notes schema in tool-schemas.ts is too
permissive for pagination and date filtering. Update the schema definition for
offset to require a non-negative integer using the existing schema builder in
that block, and change updatedAfter to use the ISO datetime validator instead of
a plain string so the contract is enforced at the schema level. Keep the change
localized to the search_notes schema entries near query, projectId, limit,
offset, and updatedAfter.
In `@electron/mcp/tools/notes.ts`:
- Around line 222-250: Move the external file writes in the note rename flow out
of the db.transaction block in notes.ts: the current updatedNote logic mixes
q.updateNote and writeNoteFile, so a failure later in the loop can roll back
SQLite while already-written files remain changed. Keep the transaction focused
on updating the database and collecting the affected note rows, then after
commit iterate the persisted rows and call writeNoteFile for each rewritten
note.
- Around line 216-225: The rename flow in notes.ts should reject title
collisions within the same project before calling q.updateNote. In the rename
handler that loads note/project and then updates the note, check whether any
other non-archived note in that project already has the normalized newTitle, and
return an error if so. Keep the check scoped to the existing note/project lookup
logic so ensure_note, q.updateNote, and the project’s note list stay consistent.
In `@electron/shared/read-tools-pure.ts`:
- Around line 194-205: The updatedAfter filter in read-tools-pure.ts is
comparing date strings lexicographically inside the note filtering logic, which
can reject valid ISO timestamps with offsets. Update the filtering in the note
query path to parse updatedAfter and each note’s updatedAt as timestamps once,
then compare epoch values in the snap.notes.filter callback so the behavior is
correct regardless of timezone format.
🪄 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: f2a96c39-5c31-47d1-8fc5-f6e7466b7437
📒 Files selected for processing (9)
changelogs/v2.2.7.mdelectron/ipc/chat-executor.test.tselectron/ipc/chat-executor.tselectron/lib/tool-schemas.tselectron/lib/tools.tselectron/mcp/tools/index.tselectron/mcp/tools/notes.tselectron/mcp/tools/tasks.tselectron/shared/read-tools-pure.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- electron/mcp/tools/tasks.ts
- electron/lib/tools.ts
- Enhance `search_notes` validation and logic:
- Enforce non-negative integers for pagination.
- Improve ISO datetime validation for `updatedAfter`.
- Fix date comparison logic by converting to timestamps.
- Add deterministic test cases for search pagination.
- Strengthen `rename_note` atomicity:
- Add duplicate title collision check before performing updates.
- Decouple file system writes from the database transaction to prevent inconsistencies during rollbacks.
This was referenced Jul 20, 2026
Merged
17 of 18 tasks
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.
What does this PR do?
This PR updates the
ensure_notetool so that omitting thecontentargument during an update operation preserves the existing note content instead of defaulting to an empty string. Documentation and Zod schemas have been updated to reflect this behavior, and a new test case ensures this functionality remains intact.Type of change
Checklist
npm run type-check:allpassesnpm run lintpassesnpm testpasses (runsnpm run compilefirst soelectron/bundle-guard.test.tsactually executes —npm run test:bundleto run just that)npm run test:e2epasses (run before merging UI changes or cutting a release)var(--accent),var(--text-primary), etc.)text-[Npx]pixel font classes — rem equivalents only (text-[0.714rem],text-xs, etc.)handle()and returnIpcResult<T>schema.tselectron/db/queries.ts— single source of truth (imported by both Electron main process and MCP server); never construct aDatabaseinstance outsidedb/client.ts(Electron) ormcp-server.ts(MCP runtime)dependenciesordevDependenciesadded toROLE_MAPinscripts/generate-licenses.jsandlicenses.jsonregeneratedelectron/mcp/tools/index.tsdispatch +electron/lib/tool-schemas.tsZod schema--external:<pkg>flag in thecompilescript: updated the appropriate allowlist group (RUNTIME_PROVIDED/OPTIONAL_TRANSITIVE/SUBPROCESS_ONLY/SHIPPED_NATIVE) inelectron/bundle-guard.test.tsand (ifSHIPPED_NATIVE) shipped the package viaelectron-builder.ymlNotes for reviewer
The fix involves changing how the
markdownvariable is derived inelectron/mcp/tools/notes.ts. It now performs an explicit checkcontent !== undefinedto distinguish between an intended update to empty content and an omitted parameter.Summary by CodeRabbit
get_tasknow returns blockingCardIds.ensure_notenow preserves existing note content whencontentis omitted (clears whencontentis"").ensure_note.contentbehavior.