Skip to content

Feat/vault assistant improvements#282

Open
munimunigamer wants to merge 16 commits into
masterfrom
feat/vault-assistant-improvements
Open

Feat/vault assistant improvements#282
munimunigamer wants to merge 16 commits into
masterfrom
feat/vault-assistant-improvements

Conversation

@munimunigamer

@munimunigamer munimunigamer commented Mar 22, 2026

Copy link
Copy Markdown
Member

Just general improvements to the vault assistant from bugs/stuff I noticed using it

Summary by CodeRabbit

  • New Features

    • Streaming assistant replies now update live with text, reasoning, tool activity, and a dedicated preview of pending changes.
    • Added a stop-generation flow with consistent Escape/stop behavior, plus improved generation UI (including a blinking cursor).
    • Tools can load on demand by category for more focused interactions.
    • Conversation titles can be renamed directly from the selector.
  • Bug Fixes

    • Improved persistence so concurrent/rapid generation actions don’t overwrite each other.
    • Preserved in-progress edits when new pending changes arrive.
    • Prevented duplicate vault values when composing multiple changes.
    • More reliable aborted vs error handling during streaming.

munimunigamer and others added 5 commits March 22, 2026 01:40
…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>
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, 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

  • Real-time Streaming Responses: Implemented real-time streaming for assistant text and reasoning, providing a more interactive and responsive user experience. This includes a streaming cursor and dynamic updates to the message content as it's generated.
  • Dynamic Tool Loading for AI Assistant: Introduced a dynamic tool loading mechanism, allowing the AI assistant to activate specific tool categories (e.g., characters, lorebooks, images) on demand. This optimizes tool usage and provides more focused capabilities based on the conversation context.
  • Conversation Management Enhancements: Added the ability to rename conversations directly within the UI, implemented eager saving of conversations to immediately reflect new chats in history, and improved error handling and state resetting during conversation changes or generation aborts.
  • Improved User Experience and Controls: Refined the UI for assistant messages, separating reasoning and content for clarity. The send button now transforms into a 'stop generation' button when the AI is active, giving users more control over ongoing processes.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/lib/components/vault/InteractiveVaultAssistant.svelte Outdated
Comment thread src/lib/components/vault/InteractiveVaultAssistant.svelte Outdated
Comment thread src/lib/components/vault/InteractiveVaultAssistant.svelte Outdated
munimunigamer and others added 10 commits April 3, 2026 19:26
… 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.
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ff6ab605-4eed-466f-b8e1-2b75439e83ac

📥 Commits

Reviewing files that changed from the base of the PR and between 5b21c3e and 33ce24f.

📒 Files selected for processing (5)
  • src/lib/components/vault/InteractiveVaultAssistant.svelte
  • src/lib/services/ai/vault/InteractiveVaultService.test.ts
  • src/lib/services/ai/vault/InteractiveVaultService.ts
  • src/lib/utils/vaultMerge.test.ts
  • src/lib/utils/vaultMerge.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/lib/utils/vaultMerge.ts
  • src/lib/utils/vaultMerge.test.ts
  • src/lib/services/ai/vault/InteractiveVaultService.test.ts
  • src/lib/services/ai/vault/InteractiveVaultService.ts
  • src/lib/components/vault/InteractiveVaultAssistant.svelte

📝 Walkthrough

Walkthrough

The 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.

Changes

Assistant streaming and tool loading

Layer / File(s) Summary
Streaming contracts and dynamic tool execution
src/lib/services/ai/vault/InteractiveVaultService.ts, src/lib/services/ai/sdk/agents/factory.ts, src/lib/services/prompts/templates/memory.ts
Adds category-based tool loading, per-step tool configuration, incremental stream events, abort events, and updated title-generation and prompt behavior.
Service persistence and streaming validation
src/lib/services/ai/vault/InteractiveVaultService.test.ts
Tests tool activation, initialization, conversation persistence, streaming events, tool calls, and abort/error handling.
Conversation lifecycle and streaming state
src/lib/components/vault/InteractiveVaultAssistant.svelte
Adds queued saves, streaming placeholders, shared abort handling, awaited conversation transitions, and queued approval/rejection persistence.
Streaming presentation and input controls
src/lib/components/vault/InteractiveVaultAssistant.svelte, src/lib/components/vault/VaultAssistantInput.svelte
Updates streaming markdown, reasoning, tool chips, diffs, cursor styling, inline renaming, and stop-generation controls.

Vault pending-change consistency

Layer / File(s) Summary
Pending merge and edit synchronization safeguards
src/lib/utils/vaultMerge.ts, src/lib/utils/vaultMerge.test.ts, src/lib/components/vault/VaultEntityEditPanel.svelte
Prevents duplicate composed array additions, adds merge regression coverage, and blocks state synchronization while an edit form is dirty.

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
Loading

Possibly related PRs

Suggested reviewers: a-frazier, failerko

Poem

I’m a rabbit, thumping code in the night,
Streaming replies now blink just right.
Tools hop in when categories call,
Saves queue neatly, preventing a fall.
Stop buttons pause the AI’s flight—
Merge bugs vanish by moonlight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is related to the vault assistant changes but is too generic to clearly convey the main update. Use a more specific title that names the primary change, such as dynamic tool loading, streaming/abort handling, or vault assistant persistence fixes.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Pento95

Pento95 commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Rebased onto current master (this branch was 4 commits behind, including the #310 mobile tab-system restructure that touched InteractiveVaultAssistant.svelte — merged, then re-ported the UI-dependent pieces by hand rather than a mechanical conflict resolve, since a naive merge would have silently dropped one side or the other in a few spots).

Changes added on top of the original PR:

  • Save race fix: handleApprove/handleReject/handleApproveAll/handleNewConversation/handleSwitchConversation were calling saveConversation() directly, bypassing the pendingSave queue used by handleSend. A save in flight from one of these could race a queued save from another and, worse, run against a service instance that had just been replaced (new conversation/switch), risking duplicate conversation rows. All save call sites now go through the same queue.
  • Clean abort: stopping generation (Stop button or Escape) surfaced as a generic "Sorry, I encountered an error: ..." bubble instead of stopping silently. The service's catch block didn't special-case AbortError, and in Tauri's WebKit-based webview the thrown value on abort doesn't reliably satisfy instanceof Error either, so the check missed it either way. Added a dedicated aborted stream event, keyed off signal.aborted rather than the shape of the caught error.
  • Stop now clears the in-progress message instead of leaving a frozen, cursor-less partial response in the chat.
  • Tool-only step feedback: a step where the model calls a tool with no preceding text/reasoning (e.g. list_characters) showed no activity indicator at all until the step finished. tool_start now also creates the streaming placeholder so the tool chip has somewhere to render immediately.
  • Fixed a pre-existing, unrelated bug found during manual testing on this branch: proposing more than one update_character/update_scenario edit for the same entity before either was approved caused tags/traits to duplicate on every additional proposal (visibly, e.g. 4 tags → 8 → 12), and made the tag editor effectively uneditable (local edits got silently discarded whenever the next AI response arrived). Root cause was in the pending-change composition logic (vaultMerge.ts's array-diff step didn't dedupe against the already-composed base) plus a missing dirty-state guard in VaultEntityEditPanel's edit-mode sync effect. Not part of this PR's original scope, but shipping it alongside since it was found while testing this branch.
  • Added test coverage for the service layer — dynamic tool loading, save/load round-trip, and the abort-vs-error distinction — none of which had tests before. 87/87 passing, svelte-check clean.
  • Minor: stopWhenDone now imports directly from stopConditions instead of the agents barrel, which was pulling in the whole factory/settings dependency chain for one pure function.

@Pento95
Pento95 marked this pull request as ready for review July 18, 2026 21:55

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lift

Stale data overwrite hazard due to blocking reactive updates.

By returning early when formDirty is true, this $effect completely halts the synchronization of incoming updates into charData and scenarioData. 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:

  1. The agent streams an initial change (e.g., { name: 'Alice', description: 'A' }), which syncs to the UI.
  2. The user edits the name, setting formDirty = true.
  3. The agent continues streaming and updates the description to 'A brave warrior'.
  4. Because formDirty is true, the form's description remains stale ('A').
  5. Upon approval, scopeToOwnChanges compares 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 formDirty is 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 win

Add 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 win

Add coverage for an emitted abort stream part.

Both tests make the iterator throw, so they remain green when the service ignores the SDK’s distinct abort event. Add { type: 'abort' } to nextStreamEvents and assert that the sole terminal event is aborted. (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 win

Exercise load_toolset through the agent configuration.

These tests do not verify that executing load_toolset changes the next prepareStep().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

📥 Commits

Reviewing files that changed from the base of the PR and between 0ab1a36 and 5b21c3e.

📒 Files selected for processing (9)
  • src/lib/components/vault/InteractiveVaultAssistant.svelte
  • src/lib/components/vault/VaultAssistantInput.svelte
  • src/lib/components/vault/VaultEntityEditPanel.svelte
  • src/lib/services/ai/sdk/agents/factory.ts
  • src/lib/services/ai/vault/InteractiveVaultService.test.ts
  • src/lib/services/ai/vault/InteractiveVaultService.ts
  • src/lib/services/prompts/templates/memory.ts
  • src/lib/utils/vaultMerge.test.ts
  • src/lib/utils/vaultMerge.ts

Comment thread src/lib/components/vault/InteractiveVaultAssistant.svelte
Comment thread src/lib/components/vault/InteractiveVaultAssistant.svelte
Comment thread src/lib/components/vault/InteractiveVaultAssistant.svelte
Comment thread src/lib/components/vault/VaultAssistantInput.svelte
Comment thread src/lib/services/ai/vault/InteractiveVaultService.ts
Comment thread src/lib/utils/vaultMerge.ts Outdated
…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.
@Pento95

Pento95 commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Addressed the CodeRabbit findings:

  • queueSave() now reports success/failure so a failed save blocks the destructive transition (new conversation / switch) instead of being silently discarded.
  • Rename/delete buttons in the conversation list are no longer hover-only (matched the existing sm:opacity-0/focus-visible pattern from BranchPanel.svelte).
  • Generation ownership is now identity-guarded, so a stale handleSend resolving after abort/a new generation can't clobber newer state.
  • Added the missing abort case for the SDK's native fullStream part (previously fell through as a done).
  • Fixed the appended-item dedup in vaultMerge.ts to not eat a genuinely-intended duplicate.
  • Added test coverage for all of the above.

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: handleDeleteConversation wasn't aborting in-flight generation before resetting the service when deleting the active conversation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants