Feat/vault assistant improvements#282
Conversation
…utton - Add inline rename UI with pencil icon on conversation list items - Fix auto-title: generateTitle now reads from chatMessages instead of conversationHistory (which isn't populated during eager save) - Stop button uses same accent style as send, just swaps icon Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Vault Assistant's interactivity and usability. It introduces real-time streaming of AI responses, making interactions feel more fluid and immediate. The underlying AI service now supports dynamic tool loading, allowing the assistant to adapt its capabilities based on the conversation's needs. Additionally, several quality-of-life improvements have been made, including conversation renaming, more robust conversation saving, and better control over AI generation, all contributing to a more polished and efficient user experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces substantial improvements to the Vault Assistant, enhancing both user experience and architectural robustness. Key features include real-time streaming of assistant responses, a 'stop generation' button, and inline conversation renaming. The introduction of dynamic tool loading is a significant architectural upgrade that should improve efficiency and scalability. The error handling and state management have also been made more robust. I've identified a few areas for minor refactoring to improve code clarity and maintainability, detailed in the comments.
… onto restructured UI master gained a mobile tab layout (activeTab/isCompact, PR #310) and other vault assistant work (#311, #326, #332) after this branch was cut, which conflicted with this branch's own changes on InteractiveVaultAssistant.svelte. Manually re-integrated the four features onto current master's structure instead of taking either side of the conflicts: - Token-by-token streaming of assistant text/reasoning via new text_delta/ reasoning_delta StreamEvent variants, rendered into a live per-message placeholder (replacing the old static "Progress indicator" block). - Inline conversation rename in the history selector, kept scoped to the conversation-selector rows only (the raw merge falsely paired this against the unrelated approve/reject buttons in the separate Pending Changes list, which live in a different section and are untouched here). - Stop/abort button in VaultAssistantInput (new since this branch was cut), wired to the existing abort logic (now factored into handleAbort(), reused by Escape/dialog-close handlers too). - Dynamic per-category tool loading (load_toolset) in InteractiveVaultService and the interactive-lorebook prompt template — this merged automatically, tool name/category mapping verified against current master's tool files. Also fixes two pre-existing issues while in this code: - initializeService()'s call to the async service.initialize() was never awaited; now awaited at all call sites. - Conversation saves (eager save on send, final save on completion) were two unsequenced fire-and-forget writes to the same row; now serialized through a single pendingSave promise chain, and the final save always runs (success, error, or abort) instead of only on success. svelte-check: 0 errors, 0 warnings.
…edback Three issues found in critical review of the vault-assistant port: 1. handleNewConversation/handleSwitchConversation and the handleApprove/handleReject/handleApproveAll trio still called service.saveConversation() directly, bypassing the pendingSave queue introduced for handleSend. A save queued from handleSend could race one of these direct calls on the same (or, worse, an already-replaced) service instance, with both branches hitting the "create new conversation" path concurrently and producing duplicate DB rows. All call sites now go through queueSave(), which also now returns its promise so callers can await completion before reassigning `service`. 2. sendMessageStreaming's single try/catch turned every error, including the AbortError from clicking Stop or pressing Escape, into a generic `type: 'error'` event, so aborting always surfaced a "Sorry, I encountered an error" bubble. The component's own `if (e.name === 'AbortError') return` was dead code — the generator never lets that exception reach it. Added a dedicated `aborted` StreamEvent (mirroring the pattern already used in NarrativePhase.ts) so a user-initiated stop is silent. 3. The streaming placeholder message was only created on text/reasoning deltas, not on tool_start, so a step where the model calls a tool with no preceding text (common for e.g. list_characters or generate_portrait) showed no feedback at all until the step finished. tool_start now also ensures the placeholder exists. svelte-check: 0 errors, 0 warnings.
…eam events Adds InteractiveVaultService.test.ts (16 tests) targeting the logic touched by this port, none of which had any coverage before: - getActiveToolNames / TOOL_CATEGORIES / initialize()'s category seeding — the dynamic tool-loading feature. Exported the three previously-module- private pieces so they're directly testable. - saveConversation / loadConversation — create-vs-update branching, title generation/truncation, and a full round trip through the (in-memory fake) DB layer. - sendMessageStreaming's event translation, including a regression test for the abort-vs-error fix from the previous commit: an AbortError from the underlying stream must yield a single `aborted` event and never a generic `error`. The AI SDK call (createStreamingAgenticAssistant) is mocked so each test scripts its own fullStream — this exercises the service's own event handling, not the agent loop or the vault tool factories themselves. Rune-based stores (settings.svelte, characterVault.svelte, debug.svelte) are mocked because vitest.config.ts runs plain node without the Svelte preprocessor. svelte-check: 0 errors, 0 warnings. Full suite: 76/76 passing.
The '../sdk/agents' barrel re-exports from ./factory too, so importing stopWhenDone through it pulled in the whole factory.ts -> settings/providers chain for one pure function. stopConditions.ts (where stopWhenDone actually lives) has no such dependencies.
…n stop Manual testing in the real Tauri app surfaced the exact bug the abort fix was meant to prevent: clicking Stop did abort generation, but still showed "Sorry, I encountered an error: Unknown error" plus a persistent error banner. "Unknown error" is the fallback used when the caught value is not `instanceof Error` — meaning in Tauri's WebKit-based webview, an aborted fetch rejects with something that fails `instanceof Error`, so neither the `error.name === 'AbortError'` check nor the `.message` extraction fired. Fixed by checking `signal?.aborted` first — the reliable, environment- independent signal that this was a user-initiated stop — with the `errorName === 'AbortError'` shape check (now not gated on `instanceof Error`) kept as a fallback. Added a regression test that throws a plain object with `name: 'AbortError'` (not an Error instance) while the signal is aborted, confirming it still yields a single `aborted` event. svelte-check: 0 errors, 0 warnings. Full suite: 77/77 passing.
…f freezing it Requested after manual testing: stopping generation left the partial assistant message visible (frozen, no cursor). handleAbort() now removes that message from the chat entirely when one is in progress, since it never finished and there's nothing worth keeping — same behavior whether triggered from the Stop button, Escape, or closing the dialog mid-stream (all three share this function). Not covered by the vitest suite: this is .svelte component state, and the project's vitest.config.ts deliberately runs plain node without the Svelte preprocessor, so component-level behavior isn't unit-tested here — needs manual verification in the running app.
…posals Reported: asking the vault assistant to add tags to a character caused the tags to double, then triple, roughly every 5s, and the tags field became uneditable (clicking the X to remove one did nothing). Two independent bugs compound to cause this: 1. src/lib/utils/vaultMerge.ts (mergeArrayLists): when the agent proposes update_character more than once for the same character before either proposal is approved, each pending change independently diffs its own `data` against the same stale `previous` (the live, still-unapproved character). composePendingOnto() applies these sibling changes in sequence onto an accumulating result — but the "appended items" step only compared data.length against previous.length, so every sibling change re-appended the same "new" tags, since from its own (data, previous) pair they genuinely look new. Fixed by skipping appended items that are already present in the accumulating base. 2. src/lib/components/vault/VaultEntityEditPanel.svelte: the $effect that resyncs local form state from the active pending change only guarded against overwriting in-progress user edits (formDirty) in view mode, not in edit mode. Every time a new sibling pending change arrived (e.g. the agent proposing another update to the same entity), the effect silently discarded whatever the user had just changed in the form, which is why removing a tag via the X button appeared to do nothing. Neither file is touched by the PR #282 vault-assistant port (verified via git diff against that branch) — this is a pre-existing bug in the general pending-change composition system, affecting any array field (tags, traits, scenario NPCs), not tag-specific. Added vaultMerge.test.ts (10 tests) covering the duplicate-sibling regression directly, plus existing mergeArrayLists/mergeIntent behavior (replacement, removal, multiset dedup) to guard against regressing those while fixing the append step. svelte-check: 0 errors, 0 warnings. Full suite: 70/70 passing. Root cause of *why* the agent proposes the same update repeatedly is still open — worth a separate look, but both bugs above would also surface with any two legitimate, independent edits to the same array field before either is approved, not just a stuck agent loop.
Caught by the project's pre-push hook before this could reach the PR.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR adds dynamic tool loading and incremental assistant streaming, centralizes abort and queued conversation persistence, updates streaming and rename UI behavior, adds service and merge regression tests, and prevents entity form synchronization from overwriting active edits. ChangesAssistant streaming and tool loading
Vault pending-change consistency
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant VaultAssistantInput
participant InteractiveVaultAssistant
participant InteractiveVaultService
User->>VaultAssistantInput: send or stop generation
VaultAssistantInput->>InteractiveVaultAssistant: handleSend() or onAbort()
InteractiveVaultAssistant->>InteractiveVaultService: sendMessageStreaming()
InteractiveVaultService-->>InteractiveVaultAssistant: text_delta, reasoning_delta, tool events
InteractiveVaultAssistant-->>User: update placeholder and streaming diff
InteractiveVaultAssistant->>InteractiveVaultService: queued save
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
|
Rebased onto current Changes added on top of the original PR:
|
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 (1)
src/lib/components/vault/VaultEntityEditPanel.svelte (1)
97-115: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftStale data overwrite hazard due to blocking reactive updates.
By returning early when
formDirtyis true, this$effectcompletely halts the synchronization of incoming updates intocharDataandscenarioData. While this successfully prevents sibling changes from visually overwriting the user's active edits, it introduces a severe data loss bug when the assistant is streaming updates:
- The agent streams an initial change (e.g.,
{ name: 'Alice', description: 'A' }), which syncs to the UI.- The user edits the name, setting
formDirty = true.- The agent continues streaming and updates the description to
'A brave warrior'.- Because
formDirtyis true, the form's description remains stale ('A').- Upon approval,
scopeToOwnChangescompares the form's stale description ('A') against the agent's final description ('A brave warrior'). Finding a difference, it assumes the user manually edited the description and saves the stale'A', silently discarding the agent's streamed output.In View mode, this block causes a similar race condition: if an entity updates in the background while
formDirtyis true, clicking Save will blindly overwrite the entity with the stale local state.To safely prevent mid-edit overwrites, the synchronization logic must be field-aware. Instead of blocking the entire object sync, track which specific fields the user has modified (e.g., by comparing current edits against a snapshot captured when the user began editing) and only block incoming updates for those specific dirty fields.
🤖 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/VaultEntityEditPanel.svelte` around lines 97 - 115, Replace the whole-object early returns in the synchronization $effect with field-level merge logic. Track fields modified since editing began and preserve only those local values, while applying incoming updates to all untouched fields in charData and scenarioData; ensure streamed changes remain current for scopeToOwnChanges and view-mode saves. Update the relevant dirty-state tracking and sync code around formDirty, viewCharData, viewScenarioData, and change without overwriting user-edited fields.
🧹 Nitpick comments (3)
src/lib/utils/vaultMerge.test.ts (1)
26-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for explicit duplicate additions.
To prevent future regressions and validate the multiset sibling-addition logic, add a test ensuring that explicitly appended duplicates are preserved if they were already present in the original array.
🧪 Proposed test case
it('still applies a genuine positional replacement', () => { expect(mergeArrayLists(['a', 'z'], ['a', 'b'], ['a', 'b'])).toEqual(['a', 'z']) }) + + it('preserves an explicit duplicate appended to the data array if it was already in the base array', () => { + expect(mergeArrayLists(['a', 'b', 'a'], ['a', 'b'], ['a', 'b'])).toEqual(['a', 'b', 'a']) + }) })🤖 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/utils/vaultMerge.test.ts` around lines 26 - 29, Extend the tests for mergeArrayLists with a case covering explicitly appended duplicate values that already exist in the original array. Assert that the duplicate additions are preserved in the merged result, alongside the existing positional replacement test.src/lib/services/ai/vault/InteractiveVaultService.test.ts (2)
319-367: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for an emitted
abortstream part.Both tests make the iterator throw, so they remain green when the service ignores the SDK’s distinct
abortevent. Add{ type: 'abort' }tonextStreamEventsand assert that the sole terminal event isaborted. (ai-sdk.dev)🤖 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/vault/InteractiveVaultService.test.ts` around lines 319 - 367, Add coverage in both abort-related tests around sendMessageStreaming so nextStreamEvents includes an emitted { type: 'abort' } part before the stream error, then assert no generic error events and that the final terminal event is { type: 'aborted' }. Keep the existing thrown-error and non-Error abort scenarios intact while ensuring the SDK abort event alone is handled correctly.
110-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise
load_toolsetthrough the agent configuration.These tests do not verify that executing
load_toolsetchanges the nextprepareStep().activeTools. Capture the mocked factory options, execute the registered tool, and assert the subsequent active-tool 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/services/ai/vault/InteractiveVaultService.test.ts` around lines 110 - 163, Add coverage in the InteractiveVaultService tests for load_toolset: capture the mocked factory options when initializing the service, execute the registered load_toolset handler for a category, then call prepareStep() and assert its activeTools include the always-active tools plus the newly loaded category tools. Use the existing service initialization and tool-category symbols, and verify the next prepared step rather than only loadedCategories.
🤖 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/InteractiveVaultAssistant.svelte`:
- Around line 104-117: The queueSave function currently swallows persistence
errors, causing callers such as handleNewConversation and
handleSwitchConversation to proceed after failed saves. Keep pendingSave
recoverable for subsequent queued saves, but preserve rejection in the promise
returned to callers so awaited destructive transitions stop when
saveConversation fails.
- Around line 971-980: The rename button in the conversation row, identified by
its onclick handler calling startRename, is hidden by default and inaccessible
without hover. Update its class list to include the established compact/touch
visibility behavior and focus-visible:opacity-100, while preserving the existing
hover opacity transition.
- Around line 741-756: Make generation cancellation awaitable and
identity-guarded: update handleAbort to signal cancellation without clearing
shared generation ownership until the async iterator finishes; await that
completion before resetting or loading conversations in lines 299-324, and
before replacing the active service after deletion in lines 359-369. In the
generation finalization path at lines 653-663, only finalize state and save
messages when the request still owns the active generation; apply these changes
at src/lib/components/vault/InteractiveVaultAssistant.svelte lines 741-756,
299-324, 359-369, and 653-663.
In `@src/lib/components/vault/VaultAssistantInput.svelte`:
- Around line 7-17: Update the keyboard submission handler in
VaultAssistantInput so Enter does not call handleSend while isGenerating is
true; preserve the current inputValue draft during generation while retaining
normal submission behavior when generation is inactive.
In `@src/lib/services/ai/vault/InteractiveVaultService.ts`:
- Around line 688-700: Handle the SDK stream’s explicit abort event in the
stream loop by adding a type 'abort' case that yields { type: 'aborted' } and
returns before reaching the done path. Update the corresponding stream tests in
src/lib/services/ai/vault/InteractiveVaultService.test.ts:319-367 to cover this
event and verify the aborted result.
In `@src/lib/utils/vaultMerge.ts`:
- Around line 64-86: Update the multiset logic in the appended-item handling to
subtract each JSON-serialized item count from prevArr out of baseValueCounts
before iterating new dataArr entries. Then use only the remaining counts to skip
items previously added by sibling pending changes, while preserving explicit
duplicate additions that already existed in prevArr.
---
Outside diff comments:
In `@src/lib/components/vault/VaultEntityEditPanel.svelte`:
- Around line 97-115: Replace the whole-object early returns in the
synchronization $effect with field-level merge logic. Track fields modified
since editing began and preserve only those local values, while applying
incoming updates to all untouched fields in charData and scenarioData; ensure
streamed changes remain current for scopeToOwnChanges and view-mode saves.
Update the relevant dirty-state tracking and sync code around formDirty,
viewCharData, viewScenarioData, and change without overwriting user-edited
fields.
---
Nitpick comments:
In `@src/lib/services/ai/vault/InteractiveVaultService.test.ts`:
- Around line 319-367: Add coverage in both abort-related tests around
sendMessageStreaming so nextStreamEvents includes an emitted { type: 'abort' }
part before the stream error, then assert no generic error events and that the
final terminal event is { type: 'aborted' }. Keep the existing thrown-error and
non-Error abort scenarios intact while ensuring the SDK abort event alone is
handled correctly.
- Around line 110-163: Add coverage in the InteractiveVaultService tests for
load_toolset: capture the mocked factory options when initializing the service,
execute the registered load_toolset handler for a category, then call
prepareStep() and assert its activeTools include the always-active tools plus
the newly loaded category tools. Use the existing service initialization and
tool-category symbols, and verify the next prepared step rather than only
loadedCategories.
In `@src/lib/utils/vaultMerge.test.ts`:
- Around line 26-29: Extend the tests for mergeArrayLists with a case covering
explicitly appended duplicate values that already exist in the original array.
Assert that the duplicate additions are preserved in the merged result,
alongside the existing positional replacement test.
🪄 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: 9ad84cdb-c4b9-429a-8a9b-6276a9e51dfe
📒 Files selected for processing (9)
src/lib/components/vault/InteractiveVaultAssistant.sveltesrc/lib/components/vault/VaultAssistantInput.sveltesrc/lib/components/vault/VaultEntityEditPanel.sveltesrc/lib/services/ai/sdk/agents/factory.tssrc/lib/services/ai/vault/InteractiveVaultService.test.tssrc/lib/services/ai/vault/InteractiveVaultService.tssrc/lib/services/prompts/templates/memory.tssrc/lib/utils/vaultMerge.test.tssrc/lib/utils/vaultMerge.ts
…e way
Fixed (verified against current code, not the stale diff CodeRabbit saw):
- queueSave() now returns whether the save actually succeeded instead of
always resolving. pendingSave itself still always resolves (so one failed
save can't block saves queued after it), but handleNewConversation/
handleSwitchConversation now bail out with an error instead of silently
discarding the current conversation when their save fails.
- Rename/delete buttons in the conversation list were opacity-0 unconditionally,
invisible without a mouse hover — unreachable on touch and undiscoverable for
keyboard users. Matched the existing sm:opacity-0/sm:group-hover:opacity-100
pattern already used for this exact kind of button in BranchPanel.svelte, and
added focus-visible:opacity-100.
- Generation ownership is now identity-guarded: handleSend captures a
generation id, handleAbort bumps a shared counter, and handleSend's finally
only finalizes/saves if it's still the current generation. Without this, a
handleSend that's still resolving after handleAbort() (or after a newer
handleSend started) could reset a newer generation's isGenerating/
abortController, or write a stale save over newer state.
- sendMessageStreaming now has a case for the AI SDK's native `abort` fullStream
part (distinct from a thrown AbortError) — previously unhandled, so it fell
through the switch, the loop ended "normally", and the caller saw a `done`
event as if the response had actually completed.
- vaultMerge.ts's appended-item dedup (from the previous duplicate-tags fix)
could wrongly suppress a genuinely-intended duplicate addition when the value
also happened to already exist in `previous`. Now subtracts prevArr's own
counts from baseArr before checking, so only copies added by an earlier
sibling change get skipped.
- Added test coverage for all of the above: native abort stream part,
load_toolset -> prepareStep wiring, and the duplicate-vs-intentional array
merge case.
Skipped (verified not applicable):
- "Enter should not submit while isGenerating" — already impossible: the
Textarea has disabled={disabled || isGenerating}, and a disabled native
<textarea> cannot receive focus or keydown events at all.
- "Field-level merge in VaultEntityEditPanel's sync effect instead of a
whole-form dirty guard" — a real architectural improvement, but a much
larger change (per-field dirty tracking, distinguishing user edits from
incoming AI updates per field) than the reported bug needs. The existing
whole-form formDirty guard already fixes the data-loss bug; field-level
merging is a follow-up worth its own review, not folded into this fix.
Also found while re-reading this code (not from CodeRabbit): handleDeleteConversation
didn't call handleAbort() before resetting the service when deleting the
conversation currently being generated into — the in-flight request kept
running and, without the generation-id guard above being triggered, could
have finalized/saved against the just-reset service. Now aborts first, same
as handleNewConversation/handleSwitchConversation.
svelte-check: 0 errors, 0 warnings. Full suite: 90/90 passing.
|
Addressed the CodeRabbit findings:
Skipped two suggestions with reasons in the commit message: the Enter-during-generation one (already blocked by the disabled textarea) and the field-level merge refactor (bigger change than the bug needs; the existing whole-form guard already fixes it). Also found and fixed one more: |
Just general improvements to the vault assistant from bugs/stuff I noticed using it
Summary by CodeRabbit
New Features
Bug Fixes