fix(chat): Internal search enablement matches source enablement#7338
Merged
Danelegend merged 14 commits intomainfrom Jan 14, 2026
Merged
fix(chat): Internal search enablement matches source enablement#7338Danelegend merged 14 commits intomainfrom
Danelegend merged 14 commits intomainfrom
Conversation
Contributor
Greptile SummaryThis PR synchronizes the Internal Search tool state with source filter selection to fix a bug where no sources selected would still attempt to use all sources. Key Changes:
Issues Found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ActionsPopover
participant SourcePreferences
participant LocalStorage
Note over User,LocalStorage: Initial Load
ActionsPopover->>SourcePreferences: Load saved source preferences
SourcePreferences->>LocalStorage: Read selectedInternalSearchSources
LocalStorage-->>SourcePreferences: Return saved preferences
SourcePreferences->>ActionsPopover: Initialize with saved sources
ActionsPopover->>ActionsPopover: Sync search tool enabled state with sources
Note over User,LocalStorage: User Toggles Individual Source
User->>ActionsPopover: Toggle source (e.g., Slack)
ActionsPopover->>ActionsPopover: Calculate newEnabledCount
ActionsPopover->>SourcePreferences: toggleSource(sourceKey)
SourcePreferences->>LocalStorage: Persist updated sources
ActionsPopover->>ActionsPopover: setSearchToolEnabled(newEnabledCount > 0)
Note over User,LocalStorage: User Disables All Sources
User->>ActionsPopover: Click "Disable All Sources"
ActionsPopover->>SourcePreferences: disableAllSources()
SourcePreferences->>LocalStorage: Persist empty array
ActionsPopover->>ActionsPopover: setSearchToolEnabled(false)
Note over User,LocalStorage: User Toggles Search Tool Off
User->>ActionsPopover: Disable Internal Search tool
ActionsPopover->>ActionsPopover: Store selectedSources in previouslyEnabledSourcesRef
ActionsPopover->>SourcePreferences: disableAllSources()
SourcePreferences->>LocalStorage: Persist empty array
Note over User,LocalStorage: User Toggles Search Tool On
User->>ActionsPopover: Enable Internal Search tool
ActionsPopover->>ActionsPopover: Check previouslyEnabledSourcesRef
alt Has previous sources
ActionsPopover->>SourcePreferences: enableSources(previous)
SourcePreferences->>LocalStorage: Persist previous sources
else No previous sources
ActionsPopover->>SourcePreferences: enableAllSources()
SourcePreferences->>LocalStorage: Persist all sources
end
ActionsPopover->>ActionsPopover: Clear previouslyEnabledSourcesRef
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+517
to
+523
| const handleSourceToggle = (sourceUniqueKey: string) => { | ||
| const willEnable = !isSourceEnabled(sourceUniqueKey); | ||
| const newEnabledCount = numSourcesEnabled + (willEnable ? 1 : -1); | ||
|
|
||
| toggleSource(sourceUniqueKey); | ||
| setSearchToolEnabled(newEnabledCount > 0); | ||
| }; |
Contributor
There was a problem hiding this comment.
The toggle source logic calculates newEnabledCount before calling toggleSource, but this assumes toggleSource will succeed. If toggleSource early-returns (e.g., when configuredSource is not found at line 1022 in hooks.ts), the source won't actually toggle, but setSearchToolEnabled will still be called with the predicted count.
This could desync the search tool state from the actual source state. Consider calling setSearchToolEnabled after confirming the toggle succeeded, or moving this logic into a callback. Does toggleSource guarantee the toggle will succeed, or can it fail silently in edge cases?
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/refresh-components/popovers/ActionsPopover/index.tsx
Line: 517:523
Comment:
The toggle source logic calculates `newEnabledCount` before calling `toggleSource`, but this assumes `toggleSource` will succeed. If `toggleSource` early-returns (e.g., when `configuredSource` is not found at line 1022 in hooks.ts), the source won't actually toggle, but `setSearchToolEnabled` will still be called with the predicted count.
This could desync the search tool state from the actual source state. Consider calling `setSearchToolEnabled` after confirming the toggle succeeded, or moving this logic into a callback. Does toggleSource guarantee the toggle will succeed, or can it fail silently in edge cases?
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
There was a problem hiding this comment.
handleSourceToggle should be a user triggered function, so I don't think we'd expect this.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="web/src/refresh-components/popovers/ActionsPopover/index.tsx">
<violation number="1" location="web/src/refresh-components/popovers/ActionsPopover/index.tsx:223">
P2: Using `0` as a fallback when the search tool is not found could cause unintended behavior. If the search tool doesn't exist in `displayTools`, this sets `searchToolId = 0`, which might be a valid tool ID. When `handleToggleTool` or `setSearchToolEnabled` is called with this ID, it would affect tool with ID 0 rather than being a no-op. Consider using `null` as the fallback and adding guards in functions that use `searchToolId`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="web/src/refresh-components/popovers/ActionsPopover/index.tsx">
<violation number="1" location="web/src/refresh-components/popovers/ActionsPopover/index.tsx:549">
P2: Re-enabling the search tool restores sources in component state but skips `useSourcePreferences` persistence, so restored selections are lost on page reload. Use the helper methods that write to localStorage when re-applying sources.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@greptile |
Comment on lines
+555
to
+571
| const handleToggleTool = (toolId: number) => { | ||
| const wasDisabled = disabledToolIds.includes(toolId); | ||
| toggleToolForCurrentAssistant(toolId); | ||
|
|
||
| if (toolId === searchToolId) { | ||
| if (wasDisabled) { | ||
| // Enabling - restore previous sources or enable all | ||
| const previous = previouslyEnabledSourcesRef.current; | ||
| setSelectedSources(previous.length > 0 ? previous : configuredSources); | ||
| previouslyEnabledSourcesRef.current = []; | ||
| } else { | ||
| // Disabling - store current sources then disable all | ||
| previouslyEnabledSourcesRef.current = [...selectedSources]; | ||
| disableAllSources(); | ||
| } | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
The setSelectedSources call on line 563 bypasses the localStorage persistence mechanism. When restoring previously enabled sources, the state is updated in React but not saved to localStorage, creating inconsistent state.
Compare with how toggleSource, enableAllSources, and disableAllSources all call persistSourcePreferencesState after updating sources. This restoration should do the same.
Additionally, the restoration logic has an edge case issue: if previouslyEnabledSourcesRef.current is empty and configuredSources is also empty (no sources available), it will still call setSelectedSources([]), which is correct but could be made more explicit.
Suggested change
| const handleToggleTool = (toolId: number) => { | |
| const wasDisabled = disabledToolIds.includes(toolId); | |
| toggleToolForCurrentAssistant(toolId); | |
| if (toolId === searchToolId) { | |
| if (wasDisabled) { | |
| // Enabling - restore previous sources or enable all | |
| const previous = previouslyEnabledSourcesRef.current; | |
| setSelectedSources(previous.length > 0 ? previous : configuredSources); | |
| previouslyEnabledSourcesRef.current = []; | |
| } else { | |
| // Disabling - store current sources then disable all | |
| previouslyEnabledSourcesRef.current = [...selectedSources]; | |
| disableAllSources(); | |
| } | |
| } | |
| }; | |
| const handleToggleTool = (toolId: number) => { | |
| const wasDisabled = disabledToolIds.includes(toolId); | |
| toggleToolForCurrentAssistant(toolId); | |
| if (toolId === searchToolId) { | |
| if (wasDisabled) { | |
| // Enabling - restore previous sources or enable all | |
| const previous = previouslyEnabledSourcesRef.current; | |
| const sourcesToRestore = previous.length > 0 ? previous : configuredSources; | |
| setSelectedSources(sourcesToRestore); | |
| // Persist the restored sources to localStorage | |
| if (typeof window !== "undefined") { | |
| localStorage.setItem( | |
| "selectedInternalSearchSources", | |
| JSON.stringify(sourcesToRestore) | |
| ); | |
| } | |
| previouslyEnabledSourcesRef.current = []; | |
| } else { | |
| // Disabling - store current sources then disable all | |
| previouslyEnabledSourcesRef.current = [...selectedSources]; | |
| disableAllSources(); | |
| } | |
| } | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/refresh-components/popovers/ActionsPopover/index.tsx
Line: 555:571
Comment:
The `setSelectedSources` call on line 563 bypasses the localStorage persistence mechanism. When restoring previously enabled sources, the state is updated in React but not saved to localStorage, creating inconsistent state.
Compare with how `toggleSource`, `enableAllSources`, and `disableAllSources` all call `persistSourcePreferencesState` after updating sources. This restoration should do the same.
Additionally, the restoration logic has an edge case issue: if `previouslyEnabledSourcesRef.current` is empty and `configuredSources` is also empty (no sources available), it will still call `setSelectedSources([])`, which is correct but could be made more explicit.
```suggestion
const handleToggleTool = (toolId: number) => {
const wasDisabled = disabledToolIds.includes(toolId);
toggleToolForCurrentAssistant(toolId);
if (toolId === searchToolId) {
if (wasDisabled) {
// Enabling - restore previous sources or enable all
const previous = previouslyEnabledSourcesRef.current;
const sourcesToRestore = previous.length > 0 ? previous : configuredSources;
setSelectedSources(sourcesToRestore);
// Persist the restored sources to localStorage
if (typeof window !== "undefined") {
localStorage.setItem(
"selectedInternalSearchSources",
JSON.stringify(sourcesToRestore)
);
}
previouslyEnabledSourcesRef.current = [];
} else {
// Disabling - store current sources then disable all
previouslyEnabledSourcesRef.current = [...selectedSources];
disableAllSources();
}
}
};
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
There was a problem hiding this comment.
I don't think we really care about persisting previous state. When we refresh, we should go back to the default state
Danelegend
force-pushed
the
ui_conn_filter_match
branch
from
b3a07d0 to
b0c2ece
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="web/src/refresh-components/popovers/ActionsPopover/index.tsx">
<violation number="1" location="web/src/refresh-components/popovers/ActionsPopover/index.tsx:522">
P2: Remove the debugging `console.log` to avoid introducing a missing dependency in this `useEffect` and leaking internal state to the browser console.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| useEffect(() => { | ||
| if (searchToolId === null || !sourcesInitialized) return; | ||
|
|
||
| console.log("Selected sources", selectedSources); |
Contributor
There was a problem hiding this comment.
P2: Remove the debugging console.log to avoid introducing a missing dependency in this useEffect and leaking internal state to the browser console.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/src/refresh-components/popovers/ActionsPopover/index.tsx, line 522:
<comment>Remove the debugging `console.log` to avoid introducing a missing dependency in this `useEffect` and leaking internal state to the browser console.</comment>
<file context>
@@ -518,6 +519,8 @@ export default function ActionsPopover({
useEffect(() => {
if (searchToolId === null || !sourcesInitialized) return;
+ console.log("Selected sources", selectedSources);
+
const hasEnabledSources = numSourcesEnabled > 0;
</file context>
Danelegend
commented
Jan 11, 2026
| if (isSwitchingBetweenSessions) { | ||
| setSelectedDocuments([]); | ||
| filterManager.setSelectedDocumentSets([]); | ||
| filterManager.setSelectedSources([]); |
Contributor
Author
There was a problem hiding this comment.
We want to keep sources the same when we create a new chat.
Active sources should persist across refreshes + new chats
Contributor
There was a problem hiding this comment.
2 issues found across 26 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="web/src/lib/hooks.ts">
<violation number="1">
P2: Loading source preferences no longer handles the legacy array stored in `selectedInternalSearchSources`, so an upgrade erases every user’s saved source selections by resetting to “all sources enabled.”</violation>
</file>
<file name="web/src/app/admin/configuration/llm/forms/formUtils.ts">
<violation number="1">
P2: Including invisible models in auto-mode results allows `default_model_name` to remain invisible because the downstream fallback now sees hidden names as “visible,” so auto mode can return a non-recommended default model.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@greptile |
rohoswagger
pushed a commit
that referenced
this pull request
Jan 19, 2026
jessicasingh7
pushed a commit
that referenced
this pull request
Jan 21, 2026
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.
Description
Currently, we might have no sources selected. Despite this, a chat message request will still attempt to use internal_search with all sources.
This implements frontend logic to align no sources with the tool being unavaliable for use, and as a byproduct, will then send the correct request parameters.
How Has This Been Tested?
Manually tested.
Additional Options
closes https://linear.app/onyx-app/issue/ENG-3370/filters-in-ui-does-not-match-the-behavior-in-the-chat
Summary by cubic
Sync the Internal Search tool with source filters in chat so behavior matches the UI. When no sources are selected, the tool is disabled and requests no longer include all sources, closing ENG-3370.
Written for commit edd1128. Summary will update on new commits.