Bugfix Vault Assistant Persistence, Display, and AI Re-indexing#332
Conversation
- Bug 3: Persist approved/rejected status by calling saveConversation after every approve, reject, and approveAll in the vault assistant. - Bug 1: Make standalone lorebook editor reactive to external vault store changes (remove isEmbedded guard from sync effect). - Bug 2: Merge externally-added entries into local state before save instead of silently overwriting the vault store. - Bug 4: Initialize editor entries from the canonical vault store, not previewLorebook (which has pending deletes applied, truncating the array and misaligning combinedEntries index mapping). - Close-reopen freeze: Replace inline onOpenChange guard with handleOpenChange() that aborts generation and always calls onClose(), preventing the component from entering an invisible mounted-but-stuck state.
- Entry version tracking in lorebookVault store (_entryVersions map, bumped on entries change) - activeLorebookId in LorebookEntryToolContext; live-data preference in all tool executables - injectLorebookChangeNote system-note injection in InteractiveVaultService for stale-entry detection - version map persistence in saveConversation/loadConversation; new entryVersions field on VaultConversation - DB migration 035_entry_versions.sql (ALTER TABLE vault_assistant_conversations ADD entry_versions) - Sync effect now uses untrack() to avoid re-firing on keystrokes while still reconciling vault updates - handleSaveClick merge reverted to only add new entries (not overwrite existing ones)
|
Warning Review limit reached
More reviews will be available in 8 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 Walkthrough<review_stack_artifact_start /> End-to-end implementation: DB migration & types, DB wiring, lorebook version accessor, InteractiveVaultService tracking/persistence and tool-context wiring, vault merge utilities, vault-editor composed previews and editorDirty, lorebook editor vault-driven sync, assistant UI save/injection/modal changes, UI composed previews and editor wiring, and small infra/tooling edits.Adds VaultConversation.entryVersions type, migration 035 adding entry_versions column, registers the migration, and extends DatabaseService INSERT/UPDATE/SELECT to persist/restore entryVersions.range_d7460d4bceac range_7d183c17a33d range_ff98b11666d1 range_4a8d0bdf106b range_60f4eed639c8 range_0b8cf2f970fd range_7bcfac8490c7Adds `getEntryVersion(id)` to LorebookVaultStore to expose a lorebook's entry-change version derived from updatedAt, defaulting to 0 when absent.range_13b10a2b3ea9InteractiveVaultService gains `_knownEntryVersions`, updates it on approvals/rejections, injects system notes when external lorebook-entry changes are detected, persists/restores entryVersions in save/load, includes activeLorebookId in lorebook-entry tool context, and clears the map on reset.range_0ac362a0d8f0 range_7c176d5cd073 range_3e87427d0db9 range_a2a4f9b0ff18 range_8f6be37df14d range_09cbe868e8f1 range_11b79f38d503 range_53836e73a382 range_c181c06abc8d range_791a2bf17ef7 range_4255ce06dec9Adds `activeLorebookId` to LorebookEntryToolContext and `resolveTargetEntries` so lorebook-entry tools fall back to the active lorebook, perform live lookup when available, adjust bounds/errors, and record the resolved lorebookId on pending changes.range_ed8b2dfa26fa range_8b0aaf953d34 range_1f627e44d2e2 range_d4d9802f52ec range_049c96f48689 range_71ddff9bda85 range_fa160446c0ce range_af5b205876d4 range_4a8518b76ef3Adds vaultMerge utilities used to compute field-level deltas and merge array/list intents: `mergeArrayLists`, `mergeIntent`, `computeDelta`, `composePendingOnto`, and record converters for scenarios/characters.range_31a6efde61cc range_30d2afa2ed6a range_7630bdb955be range_e373feb0f219 range_897c4e67be75 range_23440692e201 range_788af0f08db6Adds reactive `editorDirty`, implements `composedData` and `composePendingChanges()` to build composed previews by applying pending intents over live vault state, adds `getEffectiveChange()`, and updates open/close transitions and reset behavior.range_c64d2da797d0 range_cf401f44c8f2 range_52fbb8e08f02 range_613039be6222 range_7024bdb504ab range_c66a854e1eb3 range_3afcd3cb52e5VaultLorebookEditorContent initializes entries from the vault in embedded mode, adds a vault-driven effect to sync entries pointwise while skipping locally edited indices, overlays pending edit data into displayed entries, merges externally added entries on save, provides Revert Changes, propagates entriesDirty to vaultEditor.editorDirty, and adds unsaved-change close/save keyboard handling and UI wiring.range_40ca89050d3a range_af5bbef8b15d range_f707f9b4275d range_81519808e63c range_172354345a07 range_20d1c373026d range_32e3a205514a range_6aa862e292c6 range_83081ceb5bbd range_1ca33101b47c range_8920e0c235c8 range_e0d49d3d48cf range_25e88d770a26 range_39d782d288de range_a434f366c273InteractiveVaultAssistant injects lorebook-change check before streaming, saves conversation after approve/reject/approveAll (and blocks when `vaultEditor.editorDirty`), and centralizes modal open/close handling to abort and clear generation state.range_76e040621287 range_035eff0b108a range_e5b161a6f86e range_6ac617174373 range_88342177eccb range_61292d4473ea range_b3e634230dd6Diff view and edit panels use composed previews: VaultDiffView computes post-update `afterData`; VaultEntityEditPanel and other editors initialize and present composed/view-mode data, scope updates to the current pending change, reflect changed fields, and editors adopt unsaved-change flows, id-based derived editors, and keyboard shortcuts.range_702ed20b4cd2 range_2dbed1ec6630 range_0e74402db2a0 range_2c0e3e58c15b range_9c7543ce50b6 range_99763b20a99c range_5e743d22aa92 range_96b00a9fb736 range_9cc5982ecf93 range_e482a849fde8 range_345c7e919d62 range_8da38960a21e range_beeb87e20b8f range_ac9a618f84db range_eaa5769cf0fb range_8a5f13c2f9b0 range_ed89b7710f41 range_d66b747ae3a3 range_2857e8f7ed66 range_09437941493d range_612ef5e7fc16 range_2369e08b540e range_c42232834ac6 range_c6e64f3621ed range_319c0c014bfc range_18c91687e885Character and scenario update tools filter out `undefined`, `null`, and empty-string scalar values when building pending updates to avoid accidental overwrites.range_1cbeaeff2d38 range_5cd33e195e18Small config and dependency edits: package.json dependency bumps and Vite plugin ordering swap.range_bdc69da127eb range_38b2581a6b15 range_1e205374a008 range_59b3d333d929Small UI updates: tag list keying change, lorebook list loop variable fix, modal open/close and keyboard binding updates across editors, added imports/icons and toast usage.range_23e72488fead range_eaa5769cf0fb range_c926b760024e range_f89ac13ae73a range_6c58415eaa52 range_40ca89050d3a range_a434f366c273🚥 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. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces entry version tracking for lorebooks to detect external edits between chat turns and across sessions, updating the database schema, Svelte stores, and AI services accordingly. Feedback on these changes highlights a potential silent fallback bug in the lorebook tools when global access is unavailable, which could lead to operations executing on the wrong lorebook. Additionally, it is recommended to avoid using a Svelte $effect to sync local state back from the store, as the component should remain the source of truth for edits.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/services/database.ts (1)
3353-3363:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize
entry_versionswith the same shape the rest of the feature expects.
conversation.entryVersions ?? '{}'writes an object even though this field is defined as[lorebookId, version][]. New conversations created through this path will persist an incompatible payload before any later save touches them.Suggested fix
[ conversation.id, conversation.title, conversation.createdAt, conversation.updatedAt, conversation.messages, conversation.chatMessages, conversation.pendingChanges, - conversation.entryVersions ?? '{}', + conversation.entryVersions ?? '[]', ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/services/database.ts` around lines 3353 - 3363, The INSERT is persisting conversation.entryVersions as an object string '{}' which doesn't match the expected [lorebookId, version][] shape; update the value passed for entry_versions in the INSERT (the parameter currently using conversation.entryVersions ?? '{}') to serialize an array shape instead, e.g. JSON.stringify(conversation.entryVersions ?? []), so new rows store an empty array or the proper [id,version] pairs; locate the INSERT into vault_assistant_conversations and change the entryVersions binding accordingly (reference: conversation.entryVersions).src/lib/services/ai/sdk/tools/lorebook.ts (2)
137-140:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReport the live bounds in
read_entryerrors.This branch validates against
targetEntries, but the error still prints the originalentrieslength, so cross-lorebook reads can return the wrong range.Suggested fix
if (index < 0 || index >= targetEntries.length) { return { found: false, - error: `Entry index ${index} out of range (0-${entries.length - 1})`, + error: `Entry index ${index} out of range (0-${targetEntries.length - 1})`, } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/services/ai/sdk/tools/lorebook.ts` around lines 137 - 140, In read_entry, the out-of-range error message validates index against targetEntries but prints entries.length; change the error to report the live bounds using targetEntries.length (e.g., "Entry index ${index} out of range (0-${targetEntries.length - 1})") so cross-lorebook reads show the correct range; update the error string where the function checks "if (index < 0 || index >= targetEntries.length)" and ensure any other messages in read_entry that reference the entries length also use targetEntries.
201-229:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the resolved
targetIdin the emitted pending changes.These executors resolve
targetId = lorebookId ?? activeLorebookIdfor lookup, but the pending change still stores the rawlorebookId. That leaveslorebookIdundefined whenever the tool is used in bound-context mode, which breaks consumers ofcreateLorebookEntryTools()outsideInteractiveVaultService.Suggested fix
- const pendingChange: LorebookEntryPendingChangeSchema = { + const pendingChange: LorebookEntryPendingChangeSchema = { id: changeId, type: 'create', toolCallId: changeId, - lorebookId, + lorebookId: targetId, entry: newEntry, status: 'pending', }Apply the same
lorebookId: targetIdchange inupdate_entry,delete_entry, andmerge_entries.Also applies to: 307-316, 375-383, 455-463
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/services/ai/sdk/tools/lorebook.ts` around lines 201 - 229, The pending change objects are using the raw lorebookId instead of the resolved targetId, so when lorebookId is omitted (bound-context mode) the pending change stores undefined; update the pendingChange construction in the create (near generateId and LorebookEntryPendingChangeSchema), and similarly in update_entry, delete_entry, and merge_entries to set lorebookId: targetId (use the same resolved targetId variable used for the lookup) so all emitted pending changes reference the actual resolved lorebook ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/migrations/035_entry_versions.sql`:
- Around line 5-6: The migration adds column entry_versions to
vault_assistant_conversations but sets a JSON-object default ('{}') while the
column is documented to store an array of tuples ([lorebookId, version][]);
update the SQL ALTER TABLE to use a JSON array default ('[]') instead of '{}'
and also update any JS insert fallback that seeds or inserts entry_versions to
use an empty array JSON ('[]') so new rows and runtime inserts share the correct
tuple-array shape for parsing/iteration.
In `@src/lib/components/vault/VaultLorebookEditorContent.svelte`:
- Around line 114-138: The merge uses entry.name as identity which causes
duplicates on rename and resurrects deletions; change reconciliation to match on
a stable unique id (e.g., entry.id or add one if missing) instead of name: build
a map keyed by id from vaultLorebook.entries, and a local id set from entries,
then when iterating use id to find vaultEntry and replace/update the localEntry
(copying fields like name) rather than appending a new record; ensure
locallyDeleted checks use the id (locallyDeleted should track ids or tombstones)
so missing vault entries that were deleted externally are not re-added. Apply
the same id-based logic to the other merge at the 448-457 location.
- Around line 486-488: After a successful save (after await
saveHandler(updatedLorebook) in VaultLorebookEditorContent.svelte) clear the
locallyDeleted tombstone set so names removed and persisted during this session
do not remain suppressed; specifically reset the locallyDeleted collection
(e.g., reassign an empty Set/array) immediately after the await and before
setting saving = false / entriesDirty = false so subsequent syncs or new entries
with the same name are not erroneously ignored.
In `@src/lib/services/ai/vault/InteractiveVaultService.ts`:
- Around line 134-135: The _knownEntryVersions map in InteractiveVaultService
holds per-lorebook versions but isn't cleared by reset(), causing stale state to
persist; update the reset() method of InteractiveVaultService to clear
_knownEntryVersions (e.g., call this._knownEntryVersions.clear()) along with any
other state resets so the map does not carry over between conversations and
trigger false "modified externally" notes.
In `@src/lib/stores/lorebookVault.svelte.ts`:
- Around line 17-18: _entryVersions is an in-memory Map so versions reset on
reload; persist or derive these per-lorebook versions and restore them in load()
so cross-session comparisons work. Modify the code paths that mutate
_entryVersions (the increment logic where entries are added/removed/updated) to
also write the Map (or a per-lorebook monotonic value like lastModified or a
persisted counter) into the same persistent store you use for lorebook data,
serializing the Map to JSON, and update load() to deserialize and repopulate the
private _entryVersions: Map<string, number> (or reconstruct versions
deterministically from persisted metadata such as timestamps or an incrementing
counter) so versions survive reloads and increments are persisted atomically
with entry changes.
- Around line 57-62: The update method currently increments this._entryVersions
before calling database.updateVaultLorebook, so a failed write still bumps the
version; change update(id: string, updates: Partial<VaultLorebook>) to await
database.updateVaultLorebook(id, updates) first and only then, if
updates.entries is present, calculate and set the new version on
this._entryVersions (i.e., move the version bump to after the successful await
and make it conditional on updates.entries) so failures don't advance the stored
version.
---
Outside diff comments:
In `@src/lib/services/ai/sdk/tools/lorebook.ts`:
- Around line 137-140: In read_entry, the out-of-range error message validates
index against targetEntries but prints entries.length; change the error to
report the live bounds using targetEntries.length (e.g., "Entry index ${index}
out of range (0-${targetEntries.length - 1})") so cross-lorebook reads show the
correct range; update the error string where the function checks "if (index < 0
|| index >= targetEntries.length)" and ensure any other messages in read_entry
that reference the entries length also use targetEntries.
- Around line 201-229: The pending change objects are using the raw lorebookId
instead of the resolved targetId, so when lorebookId is omitted (bound-context
mode) the pending change stores undefined; update the pendingChange construction
in the create (near generateId and LorebookEntryPendingChangeSchema), and
similarly in update_entry, delete_entry, and merge_entries to set lorebookId:
targetId (use the same resolved targetId variable used for the lookup) so all
emitted pending changes reference the actual resolved lorebook ID.
In `@src/lib/services/database.ts`:
- Around line 3353-3363: The INSERT is persisting conversation.entryVersions as
an object string '{}' which doesn't match the expected [lorebookId, version][]
shape; update the value passed for entry_versions in the INSERT (the parameter
currently using conversation.entryVersions ?? '{}') to serialize an array shape
instead, e.g. JSON.stringify(conversation.entryVersions ?? []), so new rows
store an empty array or the proper [id,version] pairs; locate the INSERT into
vault_assistant_conversations and change the entryVersions binding accordingly
(reference: conversation.entryVersions).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b21f011f-337c-48bf-9128-0bc8bf5a4d1c
📒 Files selected for processing (9)
src-tauri/migrations/035_entry_versions.sqlsrc-tauri/src/lib.rssrc/lib/components/vault/InteractiveVaultAssistant.sveltesrc/lib/components/vault/VaultLorebookEditorContent.sveltesrc/lib/services/ai/sdk/tools/lorebook.tssrc/lib/services/ai/vault/InteractiveVaultService.tssrc/lib/services/database.tssrc/lib/stores/lorebookVault.svelte.tssrc/lib/types/index.ts
Extract the repeated lorebookId + getLorebookEntries + activeLorebookId resolution pattern into a resolveTargetEntries helper shared by all 5 entry tools (list/read/update/delete/merge). Fixes: when a specific lorebookId is requested but global lorebook access (getLorebookEntries) is not available, the tool now returns a clear error instead of silently operating on the wrong lorebook.
The entry_versions column is parsed as [string, number][] (array of tuples)
for Map construction. Using '{}' as default produces an object at runtime
which doesn't match the expected iterable shape. Changed to '[]' in both
the migration SQL and the JS insert fallback.
Name-based matching in the sync effect couldn't detect renames: the vault had the new name but local had the old one, so both appeared as unrelated entries. Added index-based fallback with duplicate tracking: when a local entry's name isn't found in the vault, the vault entry at the same index is applied if it wasn't already matched by name from another position.
- Replace in-memory _entryVersions map with updatedAt-derived version (persists across reloads, no false negatives, no migration needed) - Replace name-based sync effect with position-based pointwise comparison (fixes duplicate entries after delete approval, handles duplicate names) - Overlay change.data onto entry in combinedEntries for edit actions (fixes rename showing old name in editor sidebar)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/components/vault/VaultLorebookEditorContent.svelte (1)
494-496:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
locallyDeletedafter a successful save.Once a delete is persisted, those names remain tombstoned for the rest of the session. If the vault later receives a new entry with the same name (from a different source), the save-time merge at lines 459-461 will incorrectly suppress it.
🐛 Proposed fix
await saveHandler(updatedLorebook) saving = false entriesDirty = false + locallyDeleted = new Set()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/vault/VaultLorebookEditorContent.svelte` around lines 494 - 496, The session-level tombstones in locallyDeleted are never cleared after a successful persist, so subsequent incoming entries with the same name get suppressed; after awaiting saveHandler(updatedLorebook) in the save flow (the block that currently sets saving = false and entriesDirty = false) clear/reset the locallyDeleted collection (e.g. reinitialize locallyDeleted) so those names are no longer treated as deleted for the remainder of the session.
🧹 Nitpick comments (1)
src/lib/components/vault/VaultLorebookEditorContent.svelte (1)
109-145: 💤 Low valueIndex-based sync may race with unsaved local deletions.
If the user deletes an entry locally and the vault is updated externally (e.g., assistant adds a new entry) before the local save completes, the sync effect will overwrite local state with vault state—potentially resurrecting the deleted entry. In embedded mode with immediate saves this window is narrow, but not zero.
Consider guarding the sync by checking
entriesDirtyor skipping reconciliation entirely while a save is in flight.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/vault/VaultLorebookEditorContent.svelte` around lines 109 - 145, The sync effect ($effect block using lorebookVault.getById and updating entries) can overwrite local user deletions; modify the effect to early-return when a local save is in progress or the local buffer is dirty (check entriesDirty or an in-flight save flag like entriesSaving), so reconciliation is skipped while entriesDirty === true or a save is pending; ensure you reference the same symbols (entries, entriesDirty, lorebookVault.getById, and the $effect) and only perform the pointwise/append/trim merge when no local save/dirtiness is present.
🤖 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.
Duplicate comments:
In `@src/lib/components/vault/VaultLorebookEditorContent.svelte`:
- Around line 494-496: The session-level tombstones in locallyDeleted are never
cleared after a successful persist, so subsequent incoming entries with the same
name get suppressed; after awaiting saveHandler(updatedLorebook) in the save
flow (the block that currently sets saving = false and entriesDirty = false)
clear/reset the locallyDeleted collection (e.g. reinitialize locallyDeleted) so
those names are no longer treated as deleted for the remainder of the session.
---
Nitpick comments:
In `@src/lib/components/vault/VaultLorebookEditorContent.svelte`:
- Around line 109-145: The sync effect ($effect block using
lorebookVault.getById and updating entries) can overwrite local user deletions;
modify the effect to early-return when a local save is in progress or the local
buffer is dirty (check entriesDirty or an in-flight save flag like
entriesSaving), so reconciliation is skipped while entriesDirty === true or a
save is pending; ensure you reference the same symbols (entries, entriesDirty,
lorebookVault.getById, and the $effect) and only perform the
pointwise/append/trim merge when no local save/dirtiness is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 146d86e1-49b3-46c6-9481-ca1f3878fff2
📒 Files selected for processing (2)
src/lib/components/vault/VaultLorebookEditorContent.sveltesrc/lib/stores/lorebookVault.svelte.ts
Persisted deletions should not remain tombstoned — otherwise reusing a deleted entry's name in a new entry would be suppressed by the save path merge. Reset the set after saveHandler resolves.
Defensive cleanup — prevents stale version data from persisting between conversations if any code path reads the map between reset() and the next conversation load.
- read_entry used closure entries.length instead of resolved targetEntries.length in out-of-range error (cross-lorebook reads would show wrong bounds) - All lorebook entry tools (create, update, delete, merge) stored the raw optional lorebookId parameter in pending changes instead of the resolved targetId, causing undefined IDs in bound-context mode - resolveTargetEntries now returns targetId alongside targetEntries
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
So, I'm actually about to get some sleep for a few hours and I'll be picking up work on this branch tonight. Specifically I discovered another issue regarding scenario edits I'd like to address since it seems to follow the pattern I used for vault lorebook entries. Will be addressing comments and then working on the changes to fix the issues I'm experiencing. |
VaultScenarioEditor initialized formData once from the scenario prop and never re-synced when scenarioVault changed after AI approval. VaultPanel held a stale editingScenario snapshot instead of deriving from the store. - Add $effect in VaultScenarioEditor to sync formData from scenarioVault when the store version diverges (matches VaultLorebookEditorContent pattern) - Replace editingScenario with editingScenarioId + $derived lookup in VaultPanel so the editor always receives the latest store data
… preview - Compute delta (changed fields only) when approving scenario/character updates so sequential approvals compose instead of overwriting - Filter null and empty-string from AI tool cleanUpdates to prevent accidental data loss (matching lorebook entry pattern) - Add composedData to vaultEditorStore: merges all pending changes for the same entity so editor shows what Approve All would produce - VaultEntityEditPanel uses composed preview for form state and changed-field highlighting - VaultDiffView shows composed After with all-changes badge when multiple updates target the same entity
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/lib/components/vault/VaultDiffView.svelte`:
- Around line 258-289: The derived composedData currently builds the preview
from raw vaultEditor.pendingChanges so edits made via updateChangeData() (which
only updates the edited-change overlay) aren’t reflected; change this to resolve
pending changes through the store helper that materializes edited overlays (use
the existing function that returns resolved/merged pending changes or add one in
the vaultEditor store), then iterate that resolvedPendingChanges instead of
vaultEditor.pendingChanges inside composedData (and apply the same replacement
where the other similar derived block is used around the other instance
mentioned).
In `@src/lib/components/vault/VaultEntityEditPanel.svelte`:
- Around line 39-50: The current $effect uses vaultEditor.composedData to seed
charData/scenarioData which causes emitUpdate()/updateChangeData() to persist
the merged multi-change snapshot; instead initialize local editable state from
this change's own data (i.e. use 'data' in change ? change.data : null) for
charData, scenarioData and entryData and only collapse or compute a composed
preview for display (not for persistence); ensure
emitUpdate()/updateChangeData() is passed the delta scoped to the active change
(compute the patch against change.previous or use the original change.data as
base) so saving a change does not include sibling pending updates (review
symbols: composedData, vaultEditor.composedData, change, charData, scenarioData,
entryData, emitUpdate, updateChangeData, $effect).
In `@src/lib/components/vault/VaultPanel.svelte`:
- Around line 95-98: The template currently gates the modal by editingScenarioId
but editingScenario = $derived(...) can be null if
scenarioVault.getById(editingScenarioId) returns null, causing
VaultScenarioEditor to receive a null scenario and crash; update the logic so
the modal (and rendering of <VaultScenarioEditor ...>) is conditional on
editingScenario being truthy, or when scenarioVault.getById(editingScenarioId)
returns null immediately clear editingScenarioId; refer to editingScenarioId,
editingScenario, scenarioVault.getById and VaultScenarioEditor to locate and fix
the checks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fc25259-472f-41af-a0dc-1fc01a59b45a
📒 Files selected for processing (8)
src/lib/components/vault/VaultDiffView.sveltesrc/lib/components/vault/VaultEntityEditPanel.sveltesrc/lib/components/vault/VaultPanel.sveltesrc/lib/components/vault/VaultScenarioEditor.sveltesrc/lib/services/ai/sdk/tools/character.tssrc/lib/services/ai/sdk/tools/scenario.tssrc/lib/services/ai/vault/InteractiveVaultService.tssrc/lib/stores/vaultEditorStore.svelte.ts
- composedData now reads from vault state instead of change.previous, so already-approved changes (e.g. NPCs) remain visible while other pending changes are still awaiting approval - _autoCloseAfterAction transitions to view mode for same-entity approvals instead of closing the editor - VaultEntityEditPanel uses display values in view mode, eliminating → timing gaps that caused stale data - VaultDiffView shows per-change preview (afterData) instead of composed all-changes preview - Extract shared merge/entity-to-record utilities into vaultMerge.ts, removing triplicated _mergeArrayLists, _scenarioToRecord, and _characterToRecord across vaultEditorStore, InteractiveVaultService, and VaultEntityEditPanel - Remove dead _computeDelta method from InteractiveVaultService - Gate scenario editor modal on editingScenario truthiness
… replacements - Use multiset counting for removals so duplicate values are handled correctly - Detect genuine replacements vs. positional shifts caused by insertions/deletions - Insert unused replacements at original indices in descending order - Fix mergeIntent: use full data copy (not computeDelta) when no live base - Delegate composePendingOnto to mergeIntent for consistent logic - Remove dedup guard on append (caller is responsible)
The method already receives the lorebook from the switch statement above, making the second vault store fetch redundant.
Deduplicates the per-change afterData computation by using the shared computeDelta from vaultMerge.
…de edit protection - scopeToOwnChanges scalar fields: use form value only when user explicitly edited it (form !== originalData), otherwise keep originalData to prevent sibling scalar values from leaking via composedData - Add formDirty guard to skip syncing view mode data when user is editing, preventing AI responses from overwriting in-progress edits - handleViewModeSave stays open with toast feedback instead of closing
… dirty guard - Save button stays open after save with toast feedback (creation still closes) - Add dedicated Close button with X icon beside Save - Disable save button when no unsaved changes (hasChanges) - Toast cooldown close protection: first close with dirty state shows warning, second attempt within 3s closes - Ctrl+S keyboard shortcut for saving - Sync form from vault store after external updates
…dirty guard - Save button stays open after save with toast feedback (creation still closes) - Add dedicated Close button with X icon beside Cancel - Disable save button when no unsaved changes (hasChanges) - Toast cooldown close protection for button, Escape, and outside click - Ctrl+S keyboard shortcut for saving
…dirty guard - Remove onSaveAndClose prop; Save button preserves editor open with toast - Add dedicated Close button with X icon - Disable save button when no unsaved changes - Toast cooldown close protection for button, Escape, and outside click - Ctrl+S keyboard shortcut for saving - Track dirtyEntryIndices so sync effect doesn't overwrite user edits - Reset vaultEditor.editorDirty on component destroy to unblock approve buttons
Use index as key instead of tag value so duplicate tags don't throw a fatal Svelte runtime error.
Wrap the one-time snapshot capture in untrack() to tell Svelte the reads of isEmbedded and lorebook are intentionally non-reactive.
Update: Improved Merge Logic, Editor UX, and Bug FixesOverviewAdds ~850 lines across 8 commits on top of the existing PR: rewrites the merge array logic to handle duplicates and out-of-order approvals, reworks all three standalone editors (character, scenario, lorebook) to save without closing, adds toast-based close protection, and fixes several data-leakage bugs in scopeToOwnChanges and approve-button blocking. ChangesBug Fixes1. Sibling scalar leakage in scopeToOwnChangesWhen two pending updates modified the same entity (e.g. Change A sets Files: 2. Svelte
|
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/components/vault/VaultScenarioEditor.svelte (1)
64-67: 💤 Low valueTimer cleanup missing on component unmount.
Same as VaultCharacterForm — the
closeCooldownTimershould be cleared when the component unmounts to avoid stale callbacks.🧹 Suggested cleanup
+ import { onDestroy } from 'svelte' + + onDestroy(() => { + clearTimeout(closeCooldownTimer) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/vault/VaultScenarioEditor.svelte` around lines 64 - 67, The component declares a closeCooldownTimer (let closeCooldownTimer) but never clears it on unmount; add lifecycle cleanup in VaultScenarioEditor.svelte by importing Svelte's onDestroy and in the onDestroy callback call clearTimeout(closeCooldownTimer) and set closeCooldownTimer to undefined so any pending timeout is cancelled when the component is destroyed. Ensure you reference the existing CLOSE_COOLDOWN_MS, closeCooldownActive and closeCooldownTimer variables so the cleanup mirrors how the timer is set.src/lib/components/vault/VaultCharacterForm.svelte (1)
38-41: 💤 Low valueTimer cleanup missing on component unmount.
The
closeCooldownTimeris not cleared when the component unmounts. If the user triggers the cooldown and then the component is unmounted (e.g., parent navigates away), the timer callback will fire on a stale component.🧹 Suggested cleanup
+ import { onDestroy } from 'svelte' + + onDestroy(() => { + clearTimeout(closeCooldownTimer) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/vault/VaultCharacterForm.svelte` around lines 38 - 41, Add a Svelte onDestroy cleanup that clears the pending cooldown timer and resets related state: import onDestroy (if not already), and in onDestroy call clearTimeout(closeCooldownTimer), set closeCooldownTimer = undefined, and set closeCooldownActive = false so the timer callback cannot run against an unmounted component; refer to the CLOSE_COOLDOWN_MS, closeCooldownTimer and closeCooldownActive variables and add this cleanup inside the VaultCharacterForm component.
🤖 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.
Nitpick comments:
In `@src/lib/components/vault/VaultCharacterForm.svelte`:
- Around line 38-41: Add a Svelte onDestroy cleanup that clears the pending
cooldown timer and resets related state: import onDestroy (if not already), and
in onDestroy call clearTimeout(closeCooldownTimer), set closeCooldownTimer =
undefined, and set closeCooldownActive = false so the timer callback cannot run
against an unmounted component; refer to the CLOSE_COOLDOWN_MS,
closeCooldownTimer and closeCooldownActive variables and add this cleanup inside
the VaultCharacterForm component.
In `@src/lib/components/vault/VaultScenarioEditor.svelte`:
- Around line 64-67: The component declares a closeCooldownTimer (let
closeCooldownTimer) but never clears it on unmount; add lifecycle cleanup in
VaultScenarioEditor.svelte by importing Svelte's onDestroy and in the onDestroy
callback call clearTimeout(closeCooldownTimer) and set closeCooldownTimer to
undefined so any pending timeout is cancelled when the component is destroyed.
Ensure you reference the existing CLOSE_COOLDOWN_MS, closeCooldownActive and
closeCooldownTimer variables so the cleanup mirrors how the timer is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1169b1f6-585d-44bd-a7da-e7f8bd18f79f
📒 Files selected for processing (11)
src/lib/components/tags/TagInput.sveltesrc/lib/components/vault/VaultCharacterForm.sveltesrc/lib/components/vault/VaultDiffView.sveltesrc/lib/components/vault/VaultEntityEditPanel.sveltesrc/lib/components/vault/VaultLorebookEditor.sveltesrc/lib/components/vault/VaultLorebookEditorContent.sveltesrc/lib/components/vault/VaultPanel.sveltesrc/lib/components/vault/VaultScenarioEditor.sveltesrc/lib/services/ai/vault/InteractiveVaultService.tssrc/lib/stores/vaultEditorStore.svelte.tssrc/lib/utils/vaultMerge.ts
|
@coderabbitai resume and @coderabbitai review I've addressed the nitpicks and a few other issues found by a local model. Please review the latest changes. |
|
✅ Actions performedReviews resumed. Review triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@vite.config.js`:
- Line 9: The plugins array in vite.config.js currently calls sveltekit() before
tailwindcss(), which can change directive/CSS processing; swap the order to
[tailwindcss(), sveltekit()] in the exported plugins array ( locate the plugins
definition where sveltekit() and tailwindcss() are invoked ), revert the change
unless there is a documented reason for the swap, and then run both dev and
build to confirm no regressions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1046097a-241f-4e44-87ee-5bfae8bdf746
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/lib/components/lorebook/LorebookList.sveltesrc/lib/components/vault/VaultCharacterForm.sveltesrc/lib/components/vault/VaultLorebookEditor.sveltesrc/lib/components/vault/VaultLorebookEditorContent.sveltesrc/lib/components/vault/VaultScenarioEditor.sveltesrc/lib/stores/vaultEditorStore.svelte.tsvite.config.js
💤 Files with no reviewable changes (1)
- src/lib/stores/vaultEditorStore.svelte.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/components/lorebook/LorebookList.svelte
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/components/vault/VaultLorebookEditor.svelte
- src/lib/components/vault/VaultCharacterForm.svelte
- src/lib/components/vault/VaultLorebookEditorContent.svelte
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vite.config.js (1)
9-9: ⚡ Quick winKeep Tailwind ahead of
sveltekit()unless you've confirmed the new order is required.Line 9 now flips the plugin order, but Tailwind’s current SvelteKit setup still documents
plugins: [tailwindcss(), sveltekit()]. I’d re-verify this change before merging, or switch back to the documented sequence. (tailwindcss.com)Suggested diff
- plugins: [sveltekit(), tailwindcss()], + plugins: [tailwindcss(), sveltekit()],🤖 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 `@vite.config.js` at line 9, The plugins array order was changed to plugins: [sveltekit(), tailwindcss()] which may break the documented SvelteKit+Tailwind setup; revert or confirm order by placing tailwindcss() before sveltekit() (i.e., plugins: [tailwindcss(), sveltekit()]) unless you've explicitly tested and validated the new sequence—update the plugins array where it is defined to restore tailwindcss() ahead of sveltekit() or add a comment noting the intentional change after verification.
🤖 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/lib/components/vault/VaultScenarioEditor.svelte`:
- Line 228: handleSave() must guard against being invoked when saving is true or
hasChanges is false (the same conditions that disable the Save button), because
the global keydown handler (svelte:window onkeydown invoking handleKeydown which
calls handleSave) can trigger repeated saves. Update the handleSave function to
immediately return if saving === true or hasChanges === false, and ensure it
does not proceed to call scenarioVault.update(...) or show duplicate toasts when
those flags prevent a save; reference the existing saving and hasChanges state
variables and the scenarioVault.update(...) call inside handleSave to apply the
early-return guard.
---
Nitpick comments:
In `@vite.config.js`:
- Line 9: The plugins array order was changed to plugins: [sveltekit(),
tailwindcss()] which may break the documented SvelteKit+Tailwind setup; revert
or confirm order by placing tailwindcss() before sveltekit() (i.e., plugins:
[tailwindcss(), sveltekit()]) unless you've explicitly tested and validated the
new sequence—update the plugins array where it is defined to restore
tailwindcss() ahead of sveltekit() or add a comment noting the intentional
change after verification.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1046097a-241f-4e44-87ee-5bfae8bdf746
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/lib/components/lorebook/LorebookList.sveltesrc/lib/components/vault/VaultCharacterForm.sveltesrc/lib/components/vault/VaultLorebookEditor.sveltesrc/lib/components/vault/VaultLorebookEditorContent.sveltesrc/lib/components/vault/VaultScenarioEditor.sveltesrc/lib/stores/vaultEditorStore.svelte.tsvite.config.js
💤 Files with no reviewable changes (1)
- src/lib/stores/vaultEditorStore.svelte.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/components/lorebook/LorebookList.svelte
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/components/vault/VaultLorebookEditor.svelte
- src/lib/components/vault/VaultCharacterForm.svelte
- src/lib/components/vault/VaultLorebookEditorContent.svelte
3 participants
Overview
Fixes 9 bugs in the vault assistant and lorebook editor, adds AI-driven re-indexing to detect stale entry data after user edits, and adds composed preview for multi-update entities.
Changes
Bug Fixes
1. Approved changes replay on chat reopen (Bug 3)
vaultEditorStore.approve()/reject()/approveAll()mutated change status in memory but never calledservice.saveConversation()to persist the updated pending-changes array. On reopen, all changes loaded withstatus: 'pending'. AddedsaveConversationafter every approve/reject/approveAll operation.Files:
InteractiveVaultAssistant.svelte2. Standalone editor shows stale data after vault assistant edits (Bug 1)
The sync
$effecthad anif (!isEmbedded) returnguard, so standalone mode never synced from the vault store. Removed the guard so both modes read from the canonical source.Files:
VaultLorebookEditorContent.svelte3. Changes revert on lorebook close/reopen (Bug 2)
handleSaveClickconstructedupdatedLorebookfrom the mount-time deep copy, silently destroying any entries added by the vault assistant after mount. Now merges vault-store entries that aren't in the local array before saving.Files:
VaultLorebookEditorContent.svelte4. Pending operations sidebar shows wrong entry names (Bug 4)
entrieswas initialized frompreviewLorebook.entries, which has pending deletes already applied (truncated). Pending changes still referenced original indices. Changedentriesinitialization to read from the canonical vault store (lorebookVault.getById()?.entries) in embedded mode.Files:
VaultLorebookEditorContent.svelte5. Close-reopen freeze on vault assistant
Inline
onOpenChangeguard silently blockedonClose()whenisGeneratingwas true, leaving the component in an invisible mounted-but-stuck state. Replaced withhandleOpenChange()that aborts generation and always callsonClose().Files:
InteractiveVaultAssistant.svelte6. Stale editor data after AI updates in same chat
The sync effect only pulled in entries with new names — updates to existing entries (same name, changed content) were silently dropped when
entriesDirtywas true. Rewrote the effect to always reconcile both new and updated entries from the vault store, usinguntrack()to avoid re-firing on keystrokes. FixedhandleSaveClickto not overwrite local changes with vault data (save should persist local state, only merge new entries to prevent accidental deletion).Files:
VaultLorebookEditorContent.svelte7. Scenario editor doesn't reflect AI-approved updates
VaultScenarioEditor initialized
formDataonce from thescenarioprop and never re-synced when the underlying scenario inscenarioVaultchanged. VaultPanel held a stale snapshot ofeditingScenario. Fixed by adding a sync$effect(matching the lorebook editor pattern) and derivingeditingScenariofrom the vault store viaeditingScenarioId.Files:
VaultScenarioEditor.svelte,VaultPanel.svelte8. Multiple pending updates to the same entity overwrite each other
When two pending updates target the same scenario/character,
applyScenarioChange/applyCharacterChangepassed the fullchange.data(a complete snapshot) toscenarioVault.update()/characterVault.update(), so the second update's unchanged fields overwrote the first update's changes. Fixed by computing a delta (_computeDelta()) that only includes fields that differ betweendataandprevious, so sequential approvals compose correctly.Files:
InteractiveVaultService.ts9. AI sends null/empty values that overwrite existing data
The AI tool calls include null values for fields it omits from an update. The lorebook entry update path already filtered
v !== '', but scenario and character update paths only filteredv !== undefined. Extended all three paths to also filterv !== null, preventing accidental data loss.Files:
scenario.ts,character.tsNew Features
10. AI lorebook re-indexing
Detects external lorebook edits between AI turns and injects a system note prompting the AI to refresh entry state.
Version tracking:
lorebookVault._entryVersionsmap — bumped on everyupdate()call that changesentries. Exposed viagetEntryVersion().Tool context:
activeLorebookIdinLorebookEntryToolContext. Entry tools (list_entries,read_entry,create_entry,update_entry,delete_entry,merge_entries) prefer livegetLorebookEntries()lookup over the snapshotentriesarray when available.Change detection:
InteractiveVaultService.injectLorebookChangeNote()— before eachsendMessageStreaming, compares the current entry version against_knownEntryVersions. If stale, pushes a system note. Version map is synced after approvals to avoid redundant notifications.Persistence:
entryVersionsfield onVaultConversationtype, saved/loaded viadatabase.saveVaultConversation/loadVaultConversation. New035_entry_versions.sqlmigration.Files:
lorebookVault.svelte.ts— version trackinglorebook.ts—activeLorebookId, live-data preferenceInteractiveVaultService.ts—injectLorebookChangeNote, approval version sync, save/restoreInteractiveVaultAssistant.svelte— wire version check into send flowdatabase.ts—entryVersionssave/loadtypes/index.ts—entryVersionsonVaultConversation035_entry_versions.sql— migrationsrc-tauri/src/lib.rs— register migration11. Revert Changes button in lorebook editor
Adds a "Revert Changes" button next to "Save All Entries" that appears when entries are dirty. On click, it restores all entries to the original deep-copied snapshot, clears the
locallyDeletedset, and resetsentriesDirty. The snapshot is captured at mount time from the canonical vault store (embedded mode) or the lorebook prop (standalone mode).Files:
VaultLorebookEditorContent.svelte,vaultEditorStore.svelte.ts12. Block approve when editor has unsaved changes
handleApproveandhandleApproveAllnow checkvaultEditor.editorDirtybefore proceeding. If the lorebook editor has unsaved local edits, the approval is blocked with a "Save your local edits before approving changes." error message instead of silently proceeding (which could lose data).Files:
InteractiveVaultAssistant.svelte,vaultEditorStore.svelte.ts13. Composed preview for multi-update entities
When multiple pending updates target the same scenario or character, the editor and diff cards now show what "Approve All" would produce — all changes merged — instead of only the individual change's data.
Diff view: "After" column shows the composed result with a blue "all changes" badge when multiple pending updates target the same entity.
Entity editor: Form initializes from the composed preview so you see the final state. Changed-field highlighting diffs composed data against original
previous, so all pending changes are highlighted, not just the active change's fields.Store:
vaultEditorStore.composedDataderived getter merges all pending changes for the same entity starting from the originalpreviousstate, overlaying each change's delta.Files:
vaultEditorStore.svelte.ts,VaultEntityEditPanel.svelte,VaultDiffView.svelteVerification
npm run check— 0 errors, 0 warningsnpm run lint— 0 errors, 181 pre-existing boundary-import warnings (unchanged)cargo check— 0 errors, 1 pre-existingunused_mutwarningSummary by CodeRabbit
New Features
Bug Fixes / Improvements