Repository navigation
fix: data-loss guards, OS-trash deletes, chat persistence, note-banner opacity - #105
Merged
Merged
Conversation
…r opacity - Deletes now route to the OS trash (shell.trashItem) via a pluggable PathRemover; MCP binary keeps hard delete. Falls back to fs delete. - Project delete no longer wipes a same-named project's .md files: slug-collision guard skips the folder rmSync when a survivor shares it (IPC + MCP + post-merge cleanup); MCP delete reordered to DB-first. - Chat threads/messages re-hydrate from SQLite on launch + workspace switch (loadChatFromDb), and the active thread is persisted/restored, so conversations survive restarts/updates even if localStorage is wiped. - ask_questions form is scoped to its thread (tagged with threadId) so it no longer leaks into other chats. - AI-change note banner background sits on var(--background) so scrolling content no longer shows through.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds SQLite chat restoration, thread-scoped pending questions, safer note and project deletion, custom workspace slash commands, community command installation, autocomplete fixes, and note-editor and settings cleanup. ChangesSafe note and project deletion
Chat state restoration
Custom slash commands
Note editor and settings cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CommandsSettings
participant CommandsStore
participant ElectronIPC
participant SQLite
User->>CommandsSettings: create or install command
CommandsSettings->>CommandsStore: createCommand or installCommunityCommand
CommandsStore->>ElectronIPC: persist command
ElectronIPC->>SQLite: insert or update slash command
SQLite-->>ElectronIPC: return command row
ElectronIPC-->>CommandsStore: return mapped command
CommandsStore-->>CommandsSettings: update workspace commands
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 5
🧹 Nitpick comments (1)
src/store/slices/chat.ts (1)
85-94: 🚀 Performance & Scalability | 🔵 TrivialBatch the per-thread message load.
loadChatFromDbfetches the workspace threads once, then issues onechat.messages(t.id)IPC call per thread. Add achat.messagesByWorkspace(workspaceId)handler if available, so workspaces with many threads do not issue one IPC round-trip per thread during workspace switches.🤖 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/store/slices/chat.ts` around lines 85 - 94, Update loadChatFromDb to use the batched chat.messagesByWorkspace(workspaceId) IPC handler instead of mapping over dbThreads and issuing one chat.messages(t.id) call per thread. Preserve the existing response validation and flattening behavior, and fall back to the current per-thread loading only if the workspace-level handler is unavailable.
🤖 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/db-handlers.ts`:
- Around line 171-175: Scope survivor project queries to the relevant workspace
before deleting notes folders. In electron/ipc/db-handlers.ts lines 171-175,
pass project.workspaceId to q.getProjects; in lines 211-215, pass the source or
target workspaceId as appropriate; and in electron/mcp/tools/projects.ts lines
62-68, pass project.workspaceId to q.getProjects.
In `@electron/shared/notes-io.ts`:
- Around line 19-53: Make PathRemover completion-aware by allowing it to return
a promise, and update removePath to await the active remover and its hard-delete
fallback. Ensure deleteNoteFile awaits removal before pruning parent
directories, and make the db:note:moveToProject source cleanup await completion
independently of synchronous user-delete callers so stale source files cannot
remain after the move operation returns.
In `@src/hooks/useChatStream.ts`:
- Around line 88-94: Replace the single pendingQuestionsState slot with a
per-thread map keyed by threadId, updating the state declaration and all related
logic in the ask_questions handler and the code around lines 339 and 361. Store
each thread’s PendingQuestion[] independently, read only the active thread’s
entry for rendering, and remove that thread’s entry when its questions are
answered or cleared so switching threads preserves unanswered forms.
In `@src/store/slices/chat.ts`:
- Around line 117-122: Update loadChatFromDb so saved-thread restoration only
matches a chat thread whose workspaceId equals the workspace being loaded. Also
capture the load’s workspace/request identity and verify it is still current
before applying fetched or merged state, including activeChatThreadId, so a
superseded workspace load cannot overwrite the newer workspace.
- Around line 96-114: Preserve in-memory lastUsage when loadChatFromDb merges
threads returned by the database, since chat_threads does not persist that
field. Update the thread merge around dbThreadIds and mergedThreads to copy each
existing thread’s lastUsage onto the matching DB thread when available, while
retaining DB values for other fields and the current behavior for new or
unmatched threads.
---
Nitpick comments:
In `@src/store/slices/chat.ts`:
- Around line 85-94: Update loadChatFromDb to use the batched
chat.messagesByWorkspace(workspaceId) IPC handler instead of mapping over
dbThreads and issuing one chat.messages(t.id) call per thread. Preserve the
existing response validation and flattening behavior, and fall back to the
current per-thread loading only if the workspace-level handler is unavailable.
🪄 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: 3ee2f8f5-9a08-4e06-8d67-6736ef296f9e
📒 Files selected for processing (13)
changelogs/v2.5.18.mdelectron/ipc/db-handlers.tselectron/main.tselectron/mcp/tools/projects.tselectron/notes-files.tselectron/shared/notes-io.test.tselectron/shared/notes-io.tssrc/components/notes/note-editor.tsxsrc/hooks/useChatStream.tssrc/lib/constants.tssrc/store/index.tssrc/store/slices/chat.tssrc/store/slices/ui.ts
…er-thread questions, guard chat load Address PR review findings: - db-handlers/mcp: scope getProjects survivor check to the project's workspace so a same-named project in another workspace can't block a legitimate folder delete (delete + merge + MCP paths). - notes-io: PathRemover may now return a promise; removePath returns it so callers can await completion. Add hardDeleteNoteFile (sync, bypasses the OS-trash remover) and use it for the move-to-project stale-copy cleanup so the old file is gone before the handler returns (no watcher re-import race). - useChatStream: store pending questions in a per-thread map so a second thread's ask_questions no longer clobbers an unanswered form; entries are cleared per-thread on send/clear. - chat.loadChatFromDb: bail if the workspace switched while awaiting IPC (superseded load); only restore a saved active thread that belongs to the loaded workspace; preserve in-memory lastUsage (chat_threads doesn't persist it) when merging DB threads. Skipped: batched chat.messagesByWorkspace nitpick — no such IPC/query/preload exists; adding one is beyond a minimal review fix.
Feature: custom slash commands (Settings -> Commands) - New workspace-global slash_commands table (migration v31) + queries + db-mappers; command:* IPC handlers + preload API (list/create/update/delete). - New Zustand commands slice (customCommands), refetched on every hydrate so commands created in another window / after a workspace switch stay in sync. - Centralized built-in command registry in src/lib/slash-commands.ts with getCommandsForScope() merge (a custom command overrides a same-named built-in). - Chat panel + coding-agent pane now render merged commands instead of local hardcoded arrays. - New Commands settings section: create/edit/delete custom commands (name/description/insertText/scope) + built-in documentation. Fix: slash-command palette - Tab/click-completing a command whose completion starts with '/' (e.g. /compact) re-detected the trigger and reopened the palette, swallowing the follow-up Enter meant to send. Suppress trigger re-scans for 250ms after a selection, close the palette, and move the cursor to the end so Enter sends. Cleanup: removed the redundant read-only "Project context" snippet panel from Settings -> AI & Chat (no runtime effect). Groundwork: community slash commands - shared/chat/registry-schema.ts parses a new manifest commands[] array (fail-soft, optional so pre-v2 manifests still parse); RegistryCommandEntry added to types + preload. Browse/install UI lands next. Changelog + What's New entry added.
- BrowseCommandsModal: fetches the cairn-community manifest (cache-first, refreshable), lists community slash commands with search + category filters, and shows Install / Update / Installed states. Update detection diffs the installed row against the manifest entry (commands have no version column). - commands slice: installCommunityCommand(entry) writes a workspace-global slash_commands row with source='community', communityId=<connector id>; re-installs onto the same row (by communityId) so updates preserve the id. - Commands settings: replaced the 'coming soon' placeholder with a working Browse Community button that opens the modal. - Changelog + What's New updated.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/store/index.ts (1)
522-533: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftGate chat initialization until SQLite restores threads.
hydrateFromElectronpublishes the workspace snapshot before startingloadChatFromDb. IfACTIVE_CHAT_THREAD_KEYis absent in localStorage,ChatPanelcan callgetOrCreateThreadand set a new activity while the DB thread set is still pending. Restore chat threads before the hydrate promise resolves, or holdgetOrCreateThread/active-thread selection until chat restoration completes. Keep the async load isolated with explicit error handling.🤖 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/store/index.ts` around lines 522 - 533, Update hydrateFromElectron so initial chat restoration via loadChatFromDb completes before the hydrate promise resolves and publishes usable workspace state, preventing ChatPanel from creating or selecting a thread during restoration. Keep refresh hydrates unchanged, and wrap the asynchronous chat load with explicit error handling so failures are contained and reported through the existing store error mechanism.
🧹 Nitpick comments (2)
electron/db/schema.ts (1)
777-801: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnforce slash-command name uniqueness and format across schema, persistence, and the install flow.
The
slash_commandsfeature has no enforced invariant for command-name uniqueness or format within a workspace. The client-sideCreateCommandFormduplicate check and the community-manifest name regex both assume this invariant holds, but nothing enforces it end-to-end. Installing a community command whose name matches an existing custom command's name creates a duplicate row, andgetCommandsForScopethen surfaces both as separate/nameentries in the palette.
electron/db/schema.ts#L777-L801: add aUNIQUEindex on(workspace_id, name)in a new migration (never edit v31 directly) so the invariant is guaranteed at the data layer.electron/db/queries.ts#L964-L1023: validatename(matching the format enforced for community entries) andscope/sourceenum membership increateSlashCommand/updateSlashCommandbefore writing, and surface a clear conflict error when the new unique index would reject a write.src/store/slices/commands.ts#L109-L148: ininstallCommunityCommand, check for an existing row byname(in addition tocommunityId) before creating, and prompt the user to resolve the conflict (for example by renaming) instead of silently inserting a duplicate.🤖 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/db/schema.ts` around lines 777 - 801, Enforce slash-command invariants across all three affected sites: in electron/db/schema.ts, add a new migration after v31 creating a UNIQUE index on (workspace_id, name) without modifying v31; in electron/db/queries.ts, update createSlashCommand and updateSlashCommand to validate the community-compatible name format plus scope/source enum values and translate uniqueness failures into a clear conflict error; in src/store/slices/commands.ts, update installCommunityCommand to check for an existing command by name as well as communityId and prompt for conflict resolution, such as renaming, before insertion.src/components/settings/CommandsSettings.tsx (1)
221-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an accessible name to the icon-only close button.
The close button in
CreateCommandFormonly renders anXicon, with noaria-labelortitle. A screen reader announces this control as an unlabeled button. The delete and edit buttons inCommandRowalready usetitle="Edit"/title="Delete"; apply the same pattern here.♿ Proposed fix
<button type="button" onClick={() => { reset(); setOpen(false); }} + aria-label="Close" className="p-1 rounded text-[var(--text-tertiary)] hover:text-[var(--text-primary)]" > <X size={13} /> </button>🤖 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/CommandsSettings.tsx` around lines 221 - 230, Add a title of "Close" to the icon-only button in CreateCommandForm that calls reset and setOpen(false), matching the existing accessible naming pattern used by CommandRow’s edit and delete buttons.
🤖 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/settings/tools/BrowseCommandsModal.tsx`:
- Around line 30-42: Before invoking runInstall for an updatable installed
community command, add a confirmation step in the update flow that warns the
user the registry values will overwrite local command changes; proceed only when
confirmed. Keep isOutdated and existing installation behavior unchanged for new
commands and declined confirmations.
In `@src/lib/slash-commands.ts`:
- Around line 22-31: Prevent custom commands from overriding the reserved
built-ins “compact” and “archive-chat” in CreateCommandForm within
CommandsSettings.tsx. Validate the command name against these two identifiers
and reject creation or editing when it conflicts, preserving the existing
custom-command override behavior for all other built-in names.
- Around line 44-48: Update ALL_BUILTIN_COMMANDS to merge entries sharing the
same name across BUILTIN_CHAT_COMMANDS and BUILTIN_AGENT_COMMANDS into one
command with a combined “both” scope, while preserving chat-only and agent-only
entries and their existing metadata.
In `@src/store/slices/commands.ts`:
- Around line 109-148: Update installCommunityCommand to detect an existing
command in the active workspace by either communityId or the definition’s name,
then update and reuse that row instead of creating another command. Preserve the
existing update payload and state replacement behavior, ensuring both
same-community and same-name installations return the existing command id.
---
Outside diff comments:
In `@src/store/index.ts`:
- Around line 522-533: Update hydrateFromElectron so initial chat restoration
via loadChatFromDb completes before the hydrate promise resolves and publishes
usable workspace state, preventing ChatPanel from creating or selecting a thread
during restoration. Keep refresh hydrates unchanged, and wrap the asynchronous
chat load with explicit error handling so failures are contained and reported
through the existing store error mechanism.
---
Nitpick comments:
In `@electron/db/schema.ts`:
- Around line 777-801: Enforce slash-command invariants across all three
affected sites: in electron/db/schema.ts, add a new migration after v31 creating
a UNIQUE index on (workspace_id, name) without modifying v31; in
electron/db/queries.ts, update createSlashCommand and updateSlashCommand to
validate the community-compatible name format plus scope/source enum values and
translate uniqueness failures into a clear conflict error; in
src/store/slices/commands.ts, update installCommunityCommand to check for an
existing command by name as well as communityId and prompt for conflict
resolution, such as renaming, before insertion.
In `@src/components/settings/CommandsSettings.tsx`:
- Around line 221-230: Add a title of "Close" to the icon-only button in
CreateCommandForm that calls reset and setOpen(false), matching the existing
accessible naming pattern used by CommandRow’s edit and delete buttons.
🪄 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: 55dfc2da-e93d-4217-ae9f-20f4fe5002f6
📒 Files selected for processing (28)
changelogs/v2.5.18.mdelectron/db/queries.tselectron/db/schema.tselectron/ipc/db-handlers.tselectron/lib/community-registry.test.tselectron/lib/community-registry.tselectron/mcp/tools/projects.tselectron/notes-files.tselectron/preload.tselectron/shared/db-mappers.tselectron/shared/notes-io.test.tselectron/shared/notes-io.tsscripts/features.config.jsshared/chat/registry-schema.tssrc/components/agent/AgentChatPane.tsxsrc/components/chat/ChatInput.tsxsrc/components/chat/chat-panel/index.tsxsrc/components/settings/CommandsSettings.tsxsrc/components/settings/MCPSettings.tsxsrc/components/settings/settings-view.tsxsrc/components/settings/tools/BrowseCommandsModal.tsxsrc/components/settings/tools/BrowseCommunityModal.tsxsrc/hooks/useChatStream.tssrc/lib/slash-commands.tssrc/store/index.tssrc/store/slices/chat.tssrc/store/slices/commands.tssrc/types/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/store/slices/chat.ts
- electron/mcp/tools/projects.ts
- electron/shared/notes-io.test.ts
- electron/notes-files.ts
- slash-commands: block custom commands from overriding the reserved, executable built-ins /compact and /archive-chat (isReservedCommandName); ALL_BUILTIN_COMMANDS now merges same-name chat+agent entries into one 'both'-scoped entry for the docs view. - CommandsSettings: reject creating a reserved-name command with an inline error; add title="Close" to the create-form dismiss button (a11y parity). - commands slice: installCommunityCommand reuses an existing row matched by communityId OR name so a re-install never creates a duplicate /name. - BrowseCommandsModal: confirm before an Update overwrites a locally-edited command; new installs and declined confirmations are unaffected. - Changelog updated.
16 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?
Fixes four unrelated bugs surfaced during a session, plus a data-loss hardening pass:
.mdfile/folder to the system Trash / Recycle Bin (shell.trashItem) instead of a permanentfsdelete, so it can be restored. Implemented as a pluggablePathRemoverinjected by the Electron main process; the standalonecairn-mcpbinary keeps hard-delete (no desktop trash headless), and there's a hard-delete fallback if trashing fails.fs.rmSync'd the shared folder and destroyed the survivor's files. Added a slug-collision guard so the folder is only removed when no surviving project shares the slug (IPC handler, MCPdelete_project, and post-merge cleanup); MCP delete reordered to delete DB rows before touching disk.loadChatFromDb(called on launch + workspace switch) to re-hydrate from SQLite, and persist/restore the last-active thread pointer.ask_questionsleak — the inline question form is now tagged with the thread it arrived on so it no longer renders in other chats after a thread switch.var(--background)instead oftransparent, so note content no longer scrolls through it.Type of change
Screenshots / recording
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)scripts/features.config.js— n/a (bug fixes only)var(--accent),var(--text-primary), etc.)text-[Npx]pixel font classes — rem equivalents onlyhandle()and returnIpcResult<T>— n/a (no new IPC handlers)schema.ts— n/a (no schema changes)electron/db/queries.ts— n/a (uses existing chat queries)dependencies/devDependenciesadded toROLE_MAP— n/a (no new deps)--external:<pkg>allowlist /electron-builder.yml— n/a (no compile-flag changes)Notes for reviewer
npm run test:e2ebefore merging per the checklist.cairn-mcpstandalone binary intentionally still hard-deletes (agents run it headless with Cairn closed, where there's no desktop trash). Only the Electron main process injectsshell.trashItem. If you'd prefer the MCP path to move files to an in-workspace.trash/folder instead, that's a straightforward follow-up.chat.threads/chat.messagesread IPC.changelogs/v2.5.18.md(pending release ahead of the current 2.5.17).Summary by CodeRabbit
New Features
Bug Fixes
Changes