feat: integrate RAG environment selection in chat window#1227
Open
jeffmaury wants to merge 1 commit intokortex-hub:mainfrom
Open
feat: integrate RAG environment selection in chat window#1227jeffmaury wants to merge 1 commit intokortex-hub:mainfrom
jeffmaury wants to merge 1 commit intokortex-hub:mainfrom
Conversation
Fixes kortex-hub#515 Signed-off-by: Jeff MAURY <jmaury@redhat.com>
jeffmaury
requested review from
bmahabirbu and
fbricon
and removed request for
a team
📝 WalkthroughWalkthroughThis change introduces a RAG environment selection dropdown in the chat interface. A new selector component displays available RAG knowledge bases with MCP server integration. When users select a RAG environment, associated MCP servers are dynamically added or removed, and tool selections are updated accordingly. Changes
Sequence DiagramsequenceDiagram
actor User
participant RagSelector as RagEnvironmentSelector
participant ChatHeader as ChatHeader
participant Chat as Chat
participant MCP as MCP State
User->>RagSelector: Select RAG Environment
RagSelector->>ChatHeader: onSelect(newEnvironment)
ChatHeader->>ChatHeader: Determine old & new MCP servers
alt Has Previous MCP Server
ChatHeader->>Chat: onMCPServerRemove(oldServer)
Chat->>MCP: Delete tools from selectedMCPTools
end
alt Has New MCP Server
ChatHeader->>Chat: onMCPServerAdd(newServer)
Chat->>MCP: Initialize tools from newServer.tools
end
ChatHeader->>ChatHeader: Store selectedRagEnvironment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/renderer/src/lib/chat/components/rag-environment-selector.svelte (1)
101-101: Use a more stable key thanragEnv.namefor list rendering.At Line 101, keying only by name can collide when names are duplicated. Prefer a composite key (for example name + provider + connection) to avoid DOM reuse issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/chat/components/rag-environment-selector.svelte` at line 101, The list is currently keyed with ragEnv.name which can collide for duplicate names; update the each-block key to a stable composite identifier (e.g., combine ragEnv.name with ragEnv.provider and ragEnv.connection or an explicit unique id) so that the {`#each` ragEnvironmentsWithMCP as ragEnv (ragEnv.name)} uses a non-colliding key (for example (ragEnv.name + ':' + ragEnv.provider + ':' + ragEnv.connection) or ragEnv.id) to prevent DOM reuse issues when rendering the ragEnvironmentsWithMCP list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/renderer/src/lib/chat/components/chat.svelte`:
- Around line 86-90: The onMCPServerAdd handler currently only updates
selectedMCPTools when a matching server exists in $mcpRemoteServerInfos, so
selections get dropped if that store is stale; change onMCPServerAdd to always
call selectedMCPTools.set(mcpServer.id, new SvelteSet(...)) using
Object.keys(server.tools) when server is found and falling back to
Object.keys(mcpServer.tools) (the incoming payload) when not found, keeping the
SvelteSet usage and preserving behavior when $mcpRemoteServerInfos is empty.
In `@packages/renderer/src/lib/chat/components/rag-environment-selector.svelte`:
- Around line 34-43: The selected RagEnvironment must be cleared when the
available RAG environments change to avoid stale labels/tools; add a
watcher/subscription to the environments store (the list used to populate the
dropdown) that checks whether the current selected (selected) still exists and
still has an active MCP/tool, and if not set selected = undefined and call
onSelect(undefined) to clear active MCPs; implement this as a reactive statement
or store subscription that compares selected.id or selected.name against the new
environments list and resets selected and the selection handler accordingly so
selectedText and active tools cannot remain stale.
---
Nitpick comments:
In `@packages/renderer/src/lib/chat/components/rag-environment-selector.svelte`:
- Line 101: The list is currently keyed with ragEnv.name which can collide for
duplicate names; update the each-block key to a stable composite identifier
(e.g., combine ragEnv.name with ragEnv.provider and ragEnv.connection or an
explicit unique id) so that the {`#each` ragEnvironmentsWithMCP as ragEnv
(ragEnv.name)} uses a non-colliding key (for example (ragEnv.name + ':' +
ragEnv.provider + ':' + ragEnv.connection) or ragEnv.id) to prevent DOM reuse
issues when rendering the ragEnvironmentsWithMCP list.
🪄 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
Run ID: 990df5ad-4f92-4695-890f-e0fbb94fdc9d
📒 Files selected for processing (3)
packages/renderer/src/lib/chat/components/chat-header.sveltepackages/renderer/src/lib/chat/components/chat.sveltepackages/renderer/src/lib/chat/components/rag-environment-selector.svelte
Comment on lines
+86
to
+90
| function onMCPServerAdd(mcpServer: MCPRemoteServerInfo): void { | ||
| const server = $mcpRemoteServerInfos.find(r => r.id === mcpServer.id); | ||
| if (server) { | ||
| selectedMCPTools.set(mcpServer.id, new SvelteSet(Object.keys(server.tools))); | ||
| } |
There was a problem hiding this comment.
Use the selected RAG MCP payload as fallback when syncing tools.
At Line 87-90, selection is applied only if the server is found in $mcpRemoteServerInfos. If that store is temporarily stale/empty, selecting a RAG environment silently does nothing and tool count won’t update.
Proposed fix
function onMCPServerAdd(mcpServer: MCPRemoteServerInfo): void {
- const server = $mcpRemoteServerInfos.find(r => r.id === mcpServer.id);
- if (server) {
- selectedMCPTools.set(mcpServer.id, new SvelteSet(Object.keys(server.tools)));
- }
+ const server = $mcpRemoteServerInfos.find(r => r.id === mcpServer.id) ?? mcpServer;
+ selectedMCPTools.set(mcpServer.id, new SvelteSet(Object.keys(server.tools)));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function onMCPServerAdd(mcpServer: MCPRemoteServerInfo): void { | |
| const server = $mcpRemoteServerInfos.find(r => r.id === mcpServer.id); | |
| if (server) { | |
| selectedMCPTools.set(mcpServer.id, new SvelteSet(Object.keys(server.tools))); | |
| } | |
| function onMCPServerAdd(mcpServer: MCPRemoteServerInfo): void { | |
| const server = $mcpRemoteServerInfos.find(r => r.id === mcpServer.id) ?? mcpServer; | |
| selectedMCPTools.set(mcpServer.id, new SvelteSet(Object.keys(server.tools))); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/chat/components/chat.svelte` around lines 86 - 90,
The onMCPServerAdd handler currently only updates selectedMCPTools when a
matching server exists in $mcpRemoteServerInfos, so selections get dropped if
that store is stale; change onMCPServerAdd to always call
selectedMCPTools.set(mcpServer.id, new SvelteSet(...)) using
Object.keys(server.tools) when server is found and falling back to
Object.keys(mcpServer.tools) (the incoming payload) when not found, keeping the
SvelteSet usage and preserving behavior when $mcpRemoteServerInfos is empty.
Comment on lines
+34
to
+43
| let selected = $state<RagEnvironment | undefined>(undefined); | ||
|
|
||
| const selectedText = $derived(selected?.name ?? 'No Knowledge Base'); | ||
|
|
||
| function onSelectRagEnvironment(env: RagEnvironment | undefined, event: Event): void { | ||
| event.preventDefault(); // prevent dropdown from closing itself | ||
| onSelect(env); | ||
| selected = env; | ||
| open = false; // close the dropdown after selection | ||
| } |
There was a problem hiding this comment.
Reset invalid selection when RAG environments change.
Line 34-43 keeps selected even if the environment disappears/loses MCP after a store refresh, leaving stale label/state and potentially stale active MCP tools.
Proposed fix
let selected = $state<RagEnvironment | undefined>(undefined);
const selectedText = $derived(selected?.name ?? 'No Knowledge Base');
+
+$effect(() => {
+ if (!selected) return;
+ const stillAvailable = ragEnvironmentsWithMCP.some(
+ env =>
+ env.name === selected.name &&
+ env.ragConnection.providerId === selected.ragConnection.providerId &&
+ env.ragConnection.name === selected.ragConnection.name,
+ );
+ if (!stillAvailable) {
+ selected = undefined;
+ onSelect(undefined);
+ }
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let selected = $state<RagEnvironment | undefined>(undefined); | |
| const selectedText = $derived(selected?.name ?? 'No Knowledge Base'); | |
| function onSelectRagEnvironment(env: RagEnvironment | undefined, event: Event): void { | |
| event.preventDefault(); // prevent dropdown from closing itself | |
| onSelect(env); | |
| selected = env; | |
| open = false; // close the dropdown after selection | |
| } | |
| let selected = $state<RagEnvironment | undefined>(undefined); | |
| const selectedText = $derived(selected?.name ?? 'No Knowledge Base'); | |
| $effect(() => { | |
| if (!selected) return; | |
| const stillAvailable = ragEnvironmentsWithMCP.some( | |
| env => | |
| env.name === selected.name && | |
| env.ragConnection.providerId === selected.ragConnection.providerId && | |
| env.ragConnection.name === selected.ragConnection.name, | |
| ); | |
| if (!stillAvailable) { | |
| selected = undefined; | |
| onSelect(undefined); | |
| } | |
| }); | |
| function onSelectRagEnvironment(env: RagEnvironment | undefined, event: Event): void { | |
| event.preventDefault(); // prevent dropdown from closing itself | |
| onSelect(env); | |
| selected = env; | |
| open = false; // close the dropdown after selection | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/chat/components/rag-environment-selector.svelte`
around lines 34 - 43, The selected RagEnvironment must be cleared when the
available RAG environments change to avoid stale labels/tools; add a
watcher/subscription to the environments store (the list used to populate the
dropdown) that checks whether the current selected (selected) still exists and
still has an active MCP/tool, and if not set selected = undefined and call
onSelect(undefined) to clear active MCPs; implement this as a reactive statement
or store subscription that compares selected.id or selected.name against the new
environments list and resets selected and the selection handler accordingly so
selectedText and active tools cannot remain stale.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
|
From a quick look, the code looks fine. I still need to actually test it. I'm a bit worried about the UX though. We'll have to move it down closer to the chat box (#1139), eventually. My real concern is: how many other selectable "components", capable of contributing tools, will need to be added to the UI. If we get 1 dropdown per "component", it might not scale. If we know RAG is the only thing that will be selectable, then it's a moot point. |
Contributor
There's no indication on how to create a new KB. There should be a "Create a new Knowledge Base" option in the dropdown, equivalent to opening the RAG page + clicking on New Knowledge Base button |
Contributor
|
actually there also needs to be a Configure RAG option if none is available |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To test:
Fixes #515