fix: use persisted source functions when toggling search tool#9548
Open
Subash-Mohan wants to merge 2 commits intomainfrom
Open
fix: use persisted source functions when toggling search tool#9548Subash-Mohan wants to merge 2 commits intomainfrom
Subash-Mohan wants to merge 2 commits intomainfrom
Conversation
Contributor
Greptile SummaryThis PR fixes a localStorage persistence bug (ENG-3753) in Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ActionsPopover
participant useSourcePreferences
participant localStorage
Note over User,localStorage: Toggle Search Tool OFF
User->>ActionsPopover: click slash button (handleToggleTool)
ActionsPopover->>ActionsPopover: store selectedSources in previouslyEnabledSourcesRef
ActionsPopover->>useSourcePreferences: baseDisableAllSources()
useSourcePreferences->>ActionsPopover: setSelectedSources([])
useSourcePreferences->>localStorage: persistSourcePreferencesState([], configuredSources)
Note over User,localStorage: Toggle Search Tool ON
User->>ActionsPopover: click slash button (handleToggleTool)
alt previouslyEnabledSourcesRef.current.length > 0
ActionsPopover->>useSourcePreferences: enableSources(previous)
useSourcePreferences->>ActionsPopover: setSelectedSources(previous)
useSourcePreferences->>localStorage: persistSourcePreferencesState(previous, configuredSources)
else no previous sources
ActionsPopover->>useSourcePreferences: baseEnableAllSources()
useSourcePreferences->>ActionsPopover: setSelectedSources(ALL configuredSources)
useSourcePreferences->>localStorage: persistSourcePreferencesState(configuredSources, configuredSources)
Note over ActionsPopover: ⚠️ agentAccessibleSources filter skipped
end
ActionsPopover->>ActionsPopover: previouslyEnabledSourcesRef.current = []
Prompt To Fix All With AIThis is a comment left during a code review.
Path: web/src/refresh-components/popovers/ActionsPopover/index.tsx
Line: 856
Comment:
**`baseEnableAllSources` bypasses `agentAccessibleSources` filter**
When re-enabling the search tool with no previously saved sources, `baseEnableAllSources()` is called. This calls `enableSources(configuredSources)` inside `useSourcePreferences`, where `configuredSources` is all configured sources derived from `availableSources` — without filtering by `agentAccessibleSources`.
The local `enableAllSources` wrapper (defined around line 381) explicitly filters sources by `agentAccessibleSources` before calling `setSelectedSources`. By bypassing the wrapper, sources that the agent doesn't have access to may be re-enabled when the search tool is toggled back on from a "no previous sources" state.
Compare:
- **Wrapper `enableAllSources` (line 381–393):** Filters by `agentAccessibleSources`, then calls `setSelectedSources`.
- **`baseEnableAllSources` (from hook):** Calls `enableSources(configuredSources)` with ALL configured sources, no accessibility filter.
The fix should call the local wrapper `enableAllSources` — but since it uses `setSelectedSources` directly (which was the original bug), that wrapper also needs to be updated to persist via `enableSources`. A cleaner approach would be to update the `enableAllSources` wrapper to delegate to `baseEnableAllSources` while retaining the accessibility filter:
```typescript
const enableAllSources = useCallback(() => {
const allConfiguredSources = getConfiguredSources(availableSources);
const accessibleSources = allConfiguredSources.filter(
(s) =>
agentAccessibleSources === null ||
agentAccessibleSources.has(s.uniqueKey)
);
enableSources(accessibleSources); // ← persist + set state
if (internalSearchTool) {
setForcedToolIds([internalSearchTool.id]);
}
}, [...]);
```
Then `handleToggleTool` can call `enableAllSources()` (the wrapper) safely.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/tests/e2e/chat/actions_popover.spec.ts
Line: 72-76
Comment:
**Avoid `any` type annotation**
Using `any` for the tool response type suppresses TypeScript's type-checking, which is inconsistent with the team's strict typing standards.
Consider defining an inline or shared type, or at minimum using `unknown` with a type assertion:
```suggestion
allTools.forEach((t: { in_code_tool_id?: string; id: number }) => {
```
**Context Used:** Use explicit type annotations for variables to enh... ([source](https://app.greptile.com/review/custom-context?memory=instruction-4))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/tests/e2e/utils/tools.ts
Line: 64-66
Comment:
**`openSourceManagement` hardcodes `"Configure Connectors"` label — will break when no connectors exist**
The utility always targets `button[aria-label="Configure Connectors"]`, but `ActionLineItem` now conditionally sets the aria-label to either `"Add Connectors"` (when `isSearchToolWithNoConnectors`) or `"Configure Connectors"` (when connectors exist). If called in a test environment where the file connector setup fails (or in a future test that runs before connector creation), the locator will match nothing and the test will fail with an opaque timeout error.
Consider using a combined selector to handle both states, similar to how `toggleToolDisabled` handles the dual-label button:
```suggestion
await searchOption
.locator(
'button[aria-label="Configure Connectors"], button[aria-label="Add Connectors"]'
)
.click();
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/tests/e2e/chat/actions_popover.spec.ts
Line: 259-261
Comment:
**Cleanup step in test body is fragile when preceding assertions fail**
The "re-enable the tool for cleanup" step at the end of the test body (commented `// Re-enable the tool for cleanup (serial tests follow)`) runs only if all previous assertions pass. If any assertion earlier in this test fails, the search tool remains disabled for the remainder of the suite.
Since the suite is in `serial` mode and all tests share the same browser state, a more reliable pattern is to add an `afterEach` that always re-enables the tool:
```typescript
test.afterEach(async ({ page }) => {
// Ensure search tool is re-enabled between tests
await loginAs(page, "admin");
await page.goto("/app");
await page.waitForLoadState("networkidle");
// ... re-enable logic
});
```
Or, scope the disable/re-enable test to its own isolated browser context rather than relying on in-body cleanup.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix: enhance accessibility for tool togg..." | Re-trigger Greptile |
Contributor
🖼️ Visual Regression Report
|
Danelegend
requested changes
Mar 23, 2026
Contributor
Danelegend
left a comment
Danelegend
left a comment
There was a problem hiding this comment.
Could we get some playwright tests. I feel like I saw a previous PR that removed persistence for a reason
Contributor
|
Preview Deployment
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| enableSources(previous); | ||
| } else { | ||
| setSelectedSources(configuredSources); | ||
| baseEnableAllSources(); |
Contributor
There was a problem hiding this comment.
baseEnableAllSources bypasses agentAccessibleSources filter
When re-enabling the search tool with no previously saved sources, baseEnableAllSources() is called. This calls enableSources(configuredSources) inside useSourcePreferences, where configuredSources is all configured sources derived from availableSources — without filtering by agentAccessibleSources.
The local enableAllSources wrapper (defined around line 381) explicitly filters sources by agentAccessibleSources before calling setSelectedSources. By bypassing the wrapper, sources that the agent doesn't have access to may be re-enabled when the search tool is toggled back on from a "no previous sources" state.
Compare:
- Wrapper
enableAllSources(line 381–393): Filters byagentAccessibleSources, then callssetSelectedSources. baseEnableAllSources(from hook): CallsenableSources(configuredSources)with ALL configured sources, no accessibility filter.
The fix should call the local wrapper enableAllSources — but since it uses setSelectedSources directly (which was the original bug), that wrapper also needs to be updated to persist via enableSources. A cleaner approach would be to update the enableAllSources wrapper to delegate to baseEnableAllSources while retaining the accessibility filter:
const enableAllSources = useCallback(() => {
const allConfiguredSources = getConfiguredSources(availableSources);
const accessibleSources = allConfiguredSources.filter(
(s) =>
agentAccessibleSources === null ||
agentAccessibleSources.has(s.uniqueKey)
);
enableSources(accessibleSources); // ← persist + set state
if (internalSearchTool) {
setForcedToolIds([internalSearchTool.id]);
}
}, [...]);Then handleToggleTool can call enableAllSources() (the wrapper) safely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/refresh-components/popovers/ActionsPopover/index.tsx
Line: 856
Comment:
**`baseEnableAllSources` bypasses `agentAccessibleSources` filter**
When re-enabling the search tool with no previously saved sources, `baseEnableAllSources()` is called. This calls `enableSources(configuredSources)` inside `useSourcePreferences`, where `configuredSources` is all configured sources derived from `availableSources` — without filtering by `agentAccessibleSources`.
The local `enableAllSources` wrapper (defined around line 381) explicitly filters sources by `agentAccessibleSources` before calling `setSelectedSources`. By bypassing the wrapper, sources that the agent doesn't have access to may be re-enabled when the search tool is toggled back on from a "no previous sources" state.
Compare:
- **Wrapper `enableAllSources` (line 381–393):** Filters by `agentAccessibleSources`, then calls `setSelectedSources`.
- **`baseEnableAllSources` (from hook):** Calls `enableSources(configuredSources)` with ALL configured sources, no accessibility filter.
The fix should call the local wrapper `enableAllSources` — but since it uses `setSelectedSources` directly (which was the original bug), that wrapper also needs to be updated to persist via `enableSources`. A cleaner approach would be to update the `enableAllSources` wrapper to delegate to `baseEnableAllSources` while retaining the accessibility filter:
```typescript
const enableAllSources = useCallback(() => {
const allConfiguredSources = getConfiguredSources(availableSources);
const accessibleSources = allConfiguredSources.filter(
(s) =>
agentAccessibleSources === null ||
agentAccessibleSources.has(s.uniqueKey)
);
enableSources(accessibleSources); // ← persist + set state
if (internalSearchTool) {
setForcedToolIds([internalSearchTool.id]);
}
}, [...]);
```
Then `handleToggleTool` can call `enableAllSources()` (the wrapper) safely.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+72
to
+76
| new Set([...(currentConfig.tool_ids || []), ...desiredToolIds]) | ||
| ); | ||
|
|
||
| await page.request.patch("/api/admin/default-assistant", { | ||
| data: { tool_ids: uniqueToolIds }, |
Contributor
There was a problem hiding this comment.
any type annotation
Using any for the tool response type suppresses TypeScript's type-checking, which is inconsistent with the team's strict typing standards.
Consider defining an inline or shared type, or at minimum using unknown with a type assertion:
Suggested change
| new Set([...(currentConfig.tool_ids || []), ...desiredToolIds]) | |
| ); | |
| await page.request.patch("/api/admin/default-assistant", { | |
| data: { tool_ids: uniqueToolIds }, | |
| allTools.forEach((t: { in_code_tool_id?: string; id: number }) => { |
Context Used: Use explicit type annotations for variables to enh... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/tests/e2e/chat/actions_popover.spec.ts
Line: 72-76
Comment:
**Avoid `any` type annotation**
Using `any` for the tool response type suppresses TypeScript's type-checking, which is inconsistent with the team's strict typing standards.
Consider defining an inline or shared type, or at minimum using `unknown` with a type assertion:
```suggestion
allTools.forEach((t: { in_code_tool_id?: string; id: number }) => {
```
**Context Used:** Use explicit type annotations for variables to enh... ([source](https://app.greptile.com/review/custom-context?memory=instruction-4))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+64
to
+66
| await searchOption | ||
| .locator('button[aria-label="Configure Connectors"]') | ||
| .click(); |
Contributor
There was a problem hiding this comment.
openSourceManagement hardcodes "Configure Connectors" label — will break when no connectors exist
The utility always targets button[aria-label="Configure Connectors"], but ActionLineItem now conditionally sets the aria-label to either "Add Connectors" (when isSearchToolWithNoConnectors) or "Configure Connectors" (when connectors exist). If called in a test environment where the file connector setup fails (or in a future test that runs before connector creation), the locator will match nothing and the test will fail with an opaque timeout error.
Consider using a combined selector to handle both states, similar to how toggleToolDisabled handles the dual-label button:
Suggested change
| await searchOption | |
| .locator('button[aria-label="Configure Connectors"]') | |
| .click(); | |
| await searchOption | |
| .locator( | |
| 'button[aria-label="Configure Connectors"], button[aria-label="Add Connectors"]' | |
| ) | |
| .click(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/tests/e2e/utils/tools.ts
Line: 64-66
Comment:
**`openSourceManagement` hardcodes `"Configure Connectors"` label — will break when no connectors exist**
The utility always targets `button[aria-label="Configure Connectors"]`, but `ActionLineItem` now conditionally sets the aria-label to either `"Add Connectors"` (when `isSearchToolWithNoConnectors`) or `"Configure Connectors"` (when connectors exist). If called in a test environment where the file connector setup fails (or in a future test that runs before connector creation), the locator will match nothing and the test will fail with an opaque timeout error.
Consider using a combined selector to handle both states, similar to how `toggleToolDisabled` handles the dual-label button:
```suggestion
await searchOption
.locator(
'button[aria-label="Configure Connectors"], button[aria-label="Add Connectors"]'
)
.click();
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+259
to
+261
| const slashButton = searchOption.locator( | ||
| 'button[aria-label="Disable"], button[aria-label="Enable"]' | ||
| ); |
Contributor
There was a problem hiding this comment.
The "re-enable the tool for cleanup" step at the end of the test body (commented // Re-enable the tool for cleanup (serial tests follow)) runs only if all previous assertions pass. If any assertion earlier in this test fails, the search tool remains disabled for the remainder of the suite.
Since the suite is in serial mode and all tests share the same browser state, a more reliable pattern is to add an afterEach that always re-enables the tool:
test.afterEach(async ({ page }) => {
// Ensure search tool is re-enabled between tests
await loginAs(page, "admin");
await page.goto("/app");
await page.waitForLoadState("networkidle");
// ... re-enable logic
});Or, scope the disable/re-enable test to its own isolated browser context rather than relying on in-body cleanup.
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/tests/e2e/chat/actions_popover.spec.ts
Line: 259-261
Comment:
**Cleanup step in test body is fragile when preceding assertions fail**
The "re-enable the tool for cleanup" step at the end of the test body (commented `// Re-enable the tool for cleanup (serial tests follow)`) runs only if all previous assertions pass. If any assertion earlier in this test fails, the search tool remains disabled for the remainder of the suite.
Since the suite is in `serial` mode and all tests share the same browser state, a more reliable pattern is to add an `afterEach` that always re-enables the tool:
```typescript
test.afterEach(async ({ page }) => {
// Ensure search tool is re-enabled between tests
await loginAs(page, "admin");
await page.goto("/app");
await page.waitForLoadState("networkidle");
// ... re-enable logic
});
```
Or, scope the disable/re-enable test to its own isolated browser context rather than relying on in-body cleanup.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="web/tests/e2e/chat/actions_popover.spec.ts">
<violation number="1" location="web/tests/e2e/chat/actions_popover.spec.ts:146">
P1: Custom agent: **No debugging code**
Remove the direct debug `console.log`/`console.warn` statements from this test file before merge; they are temporary debugging output and violate the no-debugging-code rule.</violation>
<violation number="2" location="web/tests/e2e/chat/actions_popover.spec.ts:223">
P1: Wait for the tool-preference PATCH response before reloading or asserting persisted toggle state; otherwise this test can race the async save and become flaky.
(Based on your team's feedback about pre-creating waitForResponse before trigger clicks in Playwright tests.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| await expect(page.locator(TOOL_IDS.searchOption)).toBeVisible(); | ||
|
|
||
| // Disable the search tool | ||
| await toggleToolDisabled(page, TOOL_IDS.searchOption); |
Contributor
There was a problem hiding this comment.
P1: Wait for the tool-preference PATCH response before reloading or asserting persisted toggle state; otherwise this test can race the async save and become flaky.
(Based on your team's feedback about pre-creating waitForResponse before trigger clicks in Playwright tests.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/tests/e2e/chat/actions_popover.spec.ts, line 223:
<comment>Wait for the tool-preference PATCH response before reloading or asserting persisted toggle state; otherwise this test can race the async save and become flaky.
(Based on your team's feedback about pre-creating waitForResponse before trigger clicks in Playwright tests.) </comment>
<file context>
@@ -0,0 +1,303 @@
+ await expect(page.locator(TOOL_IDS.searchOption)).toBeVisible();
+
+ // Disable the search tool
+ await toggleToolDisabled(page, TOOL_IDS.searchOption);
+
+ // Verify localStorage was written (the fix being tested)
</file context>
| .locator(TOOL_IDS.imageGenerationOption) | ||
| .isVisible() | ||
| .catch(() => false); | ||
| console.log(`[tools] web_search=${webVisible}, image_gen=${imgVisible}`); |
Contributor
There was a problem hiding this comment.
P1: Custom agent: No debugging code
Remove the direct debug console.log/console.warn statements from this test file before merge; they are temporary debugging output and violate the no-debugging-code rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/tests/e2e/chat/actions_popover.spec.ts, line 146:
<comment>Remove the direct debug `console.log`/`console.warn` statements from this test file before merge; they are temporary debugging output and violate the no-debugging-code rule.</comment>
<file context>
@@ -0,0 +1,303 @@
+ .locator(TOOL_IDS.imageGenerationOption)
+ .isVisible()
+ .catch(() => false);
+ console.log(`[tools] web_search=${webVisible}, image_gen=${imgVisible}`);
+ });
+
</file context>
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
ActionsPopover
setSelectedSources(configuredSources) directly, which bypassed the localStorage persistence
layer
persist state to localStorage
ticket - https://linear.app/onyx-app/issue/ENG-3753/tool-status-saving-is-wrong
How Has This Been Tested?
Tested by enabling the tool, sending the message, and confirming that the internal search tool remains enabled even after refreshing the page.
Additional Options
Summary by cubic
Fixes wrong tool status saving (ENG-3753) by persisting the search tool’s enable/disable state and selected sources when toggled in the
ActionsPopover, and adds ARIA labels for the toggle and connector buttons to improve accessibility. Replaces directsetSelectedSourceswithenableSources,baseEnableAllSources, andbaseDisableAllSourcesso state is written to localStorage and survives refresh.Written for commit d9be78c. Summary will update on new commits.