refactor(admin): web-search#9761
Open
raunakab wants to merge 5 commits intorefactor/select-card-migrationfrom
Open
refactor(admin): web-search#9761raunakab wants to merge 5 commits intorefactor/select-card-migrationfrom
raunakab wants to merge 5 commits intorefactor/select-card-migrationfrom
Conversation
Contributor
Greptile SummaryThis PR refactors the
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WebSearchPage] --> B[combinedSearchProviders]
A --> C[combinedContentProviders]
B --> D[ProviderCard\nsearch]
C --> E[ProviderCard\ncontent]
D --> F{status}
E --> F
F -->|disconnected| G[SelectCard: empty\nConnect button visible]
F -->|connected| H[SelectCard: filled\nSet as Default button visible]
F -->|selected| I[SelectCard: selected\nCurrent Default/Crawler badge]
G -->|onClick / Connect click| J[openSearchModal / openContentModal]
H -->|Set as Default click| K[handleActivate*Provider]
I -->|onClick on card| L[handleDeactivate*Provider]
D -->|Disconnect click\non hover| M[setDisconnectTarget]
E -->|Disconnect click\non hover| M
M --> N[WebSearchDisconnectModal]
N -->|confirm| O[disconnectProvider in svc.ts\nactivate replacement → DELETE]
J --> P[WebProviderSetupModal]
P -->|Connect| Q[connectProviderFlow\ntest → upsert]
Q --> R[mutate SWR cache]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: web/tests/e2e/admin/web-search/web_search_providers.spec.ts
Line: 18
Comment:
**Debug `console.log` statements should be removed**
These `console.log` calls (`"[web-search-test] Page loaded successfully"`, `"[web-search-test] Clicked Connect, waiting for validation..."`, etc.) are debugging statements added during development and should be removed before merging. The same pattern appears in `web/tests/e2e/admin/web-search/web_content_providers.spec.ts` (lines 16, 56, 62, and 68 in that file).
**Rule Used:** Remove temporary debugging code before merging to ... ([source](https://app.greptile.com/review/custom-context?memory=b39fa59b-2568-4dd3-9576-83b46251a7b8))
**Learnt From**
[onyx-dot-app/onyx#4792](https://github.com/onyx-dot-app/onyx/pull/4792)
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/admin/web-search/svc.ts
Line: 69-71
Comment:
**Regex pattern with trailing space may silently fail**
The regex `/^Edit /` requires the button's accessible name to start with `"Edit "` (with a trailing space). However, the Edit button in `ProviderCard` is an icon-only button rendered with `icon={SvgSettings}` and `tooltip="Edit"` — its accessible name will be `"Edit"` (no trailing space). The regex therefore never matches, causing this fallback path to silently do nothing instead of clicking the Edit button.
```suggestion
const editButton = card.getByRole("button", { name: /^Edit/i });
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (7): Last reviewed commit: "style: convert all relative imports to a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
Preview Deployment
|
![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
Confidence score: 3/5
- There is a concrete regression risk in
web/src/app/admin/configuration/web-search/page.tsx:onDeselectis passed toProviderCardbut never used, so users may be unable to deselect a previously selected provider. - Because this is a user-facing behavior break (severity 6/10, high confidence 8/10), the change carries some merge risk until deselection is wired back in (or an explicit deselect control is added).
- Pay close attention to
web/src/app/admin/configuration/web-search/page.tsx- restore deselection behavior by connecting selected-state interactions toonDeselect.
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/src/app/admin/configuration/web-search/page.tsx">
<violation number="1" location="web/src/app/admin/configuration/web-search/page.tsx:280">
P2: `onDeselect` is passed into `ProviderCard` but never used, so selected providers can no longer be deselected. Wire the selected state to call `onDeselect` (or add an explicit control) to preserve the existing behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
raunakab
force-pushed
the
feat/web-search-select-card
branch
2 times, most recently
from
2bdaf56 to
5736e33
Compare
raunakab
force-pushed
the
feat/web-search-select-card
branch
from
5736e33 to
40e37cf
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (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/src/refresh-pages/admin/WebSearchPage/index.tsx">
<violation number="1" location="web/src/refresh-pages/admin/WebSearchPage/index.tsx:298">
P2: `Set as Default` is no longer wrapped in `Hoverable.Item`, so it is always visible instead of reveal-on-hover like the other card actions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
raunakab
changed the title
Contributor
🖼️ Visual Regression Report
|
…yout Replace the legacy Select component from refresh-components/cards with the new Opal SelectCard and CardHeaderLayout primitives. Adds a local ProviderCard component that maps provider status to Interactive.Stateful states (disconnected→empty, connected→filled, selected→selected) and uses Hoverable for reveal-on-hover actions.
…pers Move page content from app/admin/configuration/web-search/ into refresh-pages/admin/WebSearchPage/ following the UsersPage pattern. Extract API calls into svc.ts and shared types into interfaces.ts. Deduplicate e2e test helpers (findProviderCard, openProviderModal, mockWebSearchApis, fake data) into tests/e2e/admin/web-search/svc.ts.
raunakab
force-pushed
the
feat/web-search-select-card
branch
from
566f61b to
f410ab9
Compare
Comment on lines
+69
to
+71
| const editButton = card.getByRole("button", { name: /^Edit / }); | ||
| await editButton.waitFor({ state: "visible", timeout: 5000 }); | ||
| await editButton.click(); |
Contributor
There was a problem hiding this comment.
The regex /^Edit / requires the button's accessible name to start with "Edit " (with a trailing space). However, the Edit button in ProviderCard is an icon-only button rendered with icon={SvgSettings} and tooltip="Edit" — its accessible name will be "Edit" (no trailing space). The regex therefore never matches, causing this fallback path to silently do nothing instead of clicking the Edit button.
Suggested change
| const editButton = card.getByRole("button", { name: /^Edit / }); | |
| await editButton.waitFor({ state: "visible", timeout: 5000 }); | |
| await editButton.click(); | |
| const editButton = card.getByRole("button", { name: /^Edit/i }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/tests/e2e/admin/web-search/svc.ts
Line: 69-71
Comment:
**Regex pattern with trailing space may silently fail**
The regex `/^Edit /` requires the button's accessible name to start with `"Edit "` (with a trailing space). However, the Edit button in `ProviderCard` is an icon-only button rendered with `icon={SvgSettings}` and `tooltip="Edit"` — its accessible name will be `"Edit"` (no trailing space). The regex therefore never matches, causing this fallback path to silently do nothing instead of clicking the Edit button.
```suggestion
const editButton = card.getByRole("button", { name: /^Edit/i });
```
How can I resolve this? If you propose a fix, please make it concise.
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
1 participant
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
Refactor the
/admin/configuration/web-searchpage to use the new OpalSelectCardandCardHeaderLayoutprimitives (from #9760) instead of the legacySelectcomponent fromrefresh-components/cards.ProviderCardcomponent that maps provider status toInteractive.Statefulstates (disconnected→empty,connected→filled,selected→selected)Hoverablefor reveal-on-hover actions (Set as Default, Disconnect)CardHeaderLayoutfor the card content structure withrightChildrenandbottomRightChildrenslotsScreenshots + Videos
Screen.Recording.2026-03-30.at.12.07.32.PM.mov
Additional Options
Summary by cubic
Refactored the
/admin/configuration/web-searchpage to use@opal/componentsSelectCardand@opal/layoutsCardHeaderLayoutfor a consistent, stateful card UI. Moved the page intorefresh-pageswith shared service/types, e2e helpers, and absolute imports.Refactors
refresh-components/cardsSelectwith@opal/componentsSelectCardand@opal/layoutsCardHeaderLayout.ProviderCardthat maps provider status toSelectCardstates and wires connect/select/deselect/edit/disconnect.@opal/coreHoverableto reveal "Disconnect" on hover; "Set as Default" is always visible; show "Current Default" when selected.web/src/refresh-pages/admin/WebSearchPage/(index.tsx), moved API calls tosvc.tsand shared types tointerfaces.ts; deduped e2e helpers/fake data inweb/tests/e2e/admin/web-search/svc.ts; converted relative imports to absolute.Bug Fixes
Interactiveimport and correctly wiredonDeselectinProviderCard.Hoverablewrapper from "Set as Default" button.Written for commit f410ab9. Summary will update on new commits.