[SSP] Trigger new sort run from similarity popover in the grid/sample modal + bugfix + remove feature flag#7339
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe similarity UI was refactored from a Recoil-driven "sort by similarity" flow into a simplified operator-driven similarity popover (component renamed to SimilarityPopover). Recoil hooks/selectors for similarity were removed and query construction moved to submit-time; the component now uses local Sequence Diagram(s)mermaid UI->>Storage: read lastUsedBrainKeys[datasetId] Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/core/src/components/Actions/Similarity/constants.ts`:
- Around line 1-3: The three duplicate constants SEARCH_OPERATOR_URI,
INIT_RUN_OPERATOR_URI, and PANEL_NAME are defined here and also in the
similarity-search package; either consolidate them to a single source of truth
by removing these local definitions and importing the constants from
app/packages/similarity-search/src/constants.ts (update any references in
Actions/Similarity to use the imported symbols), or if intentional to avoid
cross-package imports add a concise comment above these exports explaining the
reason for duplication and referencing the original file so future maintainers
know it's deliberate.
In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx`:
- Around line 80-86: Extract the literal "open_panel" into a named constant
(e.g., OPEN_PANEL_OPERATOR_URI) and use that constant inside the useCallback in
Similar.tsx; update the executeOperator call in openPanel to pass
OPEN_PANEL_OPERATOR_URI instead of the raw string, keeping the rest of the
payload (PANEL_NAME, isActive, layout) unchanged; align the new constant
alongside existing SEARCH_OPERATOR_URI and INIT_RUN_OPERATOR_URI for
consistency.
- Around line 55-66: The code in the useMemo for resolvedBrainKey is
double-parsing JSON because lastUsedBrainKeys (from useBrowserStorage) already
returns a parsed object; remove the manual JSON.parse and treat
lastUsedBrainKeys as an object/map instead of a string. Update the resolution
logic in resolvedBrainKey to read lastUsedBrainKeys[datasetId] (guarding for
null/undefined) and keep the fallback to keys[0]; also consider switching the
useBrowserStorage invocation that provides lastUsedBrainKeys to a
typed/initial-value form so it returns a typed Record<string,string> (or null)
instead of a raw string, and adjust any related code that currently uses
JSON.stringify/JSON.parse for storage/retrieval.
- Around line 119-135: The call to executeOperator(SEARCH_OPERATOR_URI, params,
...) lacks error handling so failures are silently ignored; update the call to
pass an error callback (or use the existing handleError function pattern from
useSearchSubmission.ts) that logs or surfaces the error and prevents downstream
calls; ensure the callback for delegated results still checks result?.delegated
and only calls executeOperator(INIT_RUN_OPERATOR_URI, {..., operator_run_id:
operatorRunId}, { callback: () => openPanel() }) when no error occurred, and
call openPanel() or an error handler appropriately when errors are received.
In `@app/packages/similarity-search/src/hooks/useSearchSubmission.ts`:
- Around line 37-44: The code in handleOptionSelected double-serializes
lastUsedBrainKeys because it calls JSON.parse on lastUsedBrainKeys and
JSON.stringify when setting it even though useBrowserStorage already serializes
values; change handleOptionSelected to treat lastUsedBrainKeys as an object
(avoid JSON.parse) and call setLastUsedBrainKeys with the merged object directly
(avoid JSON.stringify). Update usage of lastUsedBrainKeys in that function so
you do const current = lastUsedBrainKeys || {} and then setLastUsedBrainKeys({
...current, [datasetId]: input.brainKey }) to let useBrowserStorage handle
serialization.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1af8a575-1320-42d5-8b12-d427eeb2c163
📥 Commits
Reviewing files that changed from the base of the PR and between 4f16c55 and a92b85f4fa2ff41a1e0ac846e528598d96ee312e.
📒 Files selected for processing (6)
app/packages/core/src/components/Actions/Similarity/Similar.tsxapp/packages/core/src/components/Actions/Similarity/constants.tsapp/packages/core/src/components/Actions/Similarity/index.tsxapp/packages/core/src/components/Actions/Similarity/utils.tsapp/packages/similarity-search/src/constants.tsapp/packages/similarity-search/src/hooks/useSearchSubmission.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/packages/core/src/components/Actions/Similarity/Similar.tsx (2)
54-68:⚠️ Potential issue | 🟠 MajorRemove the extra JSON layer around
lastUsedBrainKeys.
useBrowserStoragealready deserializes on read and serializes on write. Re-parsing inresolvedBrainKeyand pre-stringifying beforesetLastUsedBrainKeyscreates a second storage format, which makes this shared key incompatible with code that stores the map directly and can throw at Line 115 once that happens.Suggested fix
- const [lastUsedBrainKeys, setLastUsedBrainKeys] = - useBrowserStorage("lastUsedBrainKeys"); + const [lastUsedBrainKeys, setLastUsedBrainKeys] = + useBrowserStorage<Record<string, string>>("lastUsedBrainKeys", {}); @@ const resolvedBrainKey = useMemo(() => { if (keys.length === 0) return undefined; - try { - const stored = lastUsedBrainKeys - ? JSON.parse(lastUsedBrainKeys)[datasetId] - : null; - if (stored && keys.includes(stored)) return stored; - } catch { - // parse may fail - } + const stored = lastUsedBrainKeys?.[datasetId]; + if (stored && keys.includes(stored)) return stored; return keys[0]; }, [keys, lastUsedBrainKeys, datasetId]); @@ - const current = lastUsedBrainKeys ? JSON.parse(lastUsedBrainKeys) : {}; - setLastUsedBrainKeys( - JSON.stringify({ ...current, [datasetId]: resolvedBrainKey }) - ); + setLastUsedBrainKeys((current = {}) => ({ + ...current, + [datasetId]: resolvedBrainKey, + }));Also applies to: 115-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx` around lines 54 - 68, The code double-serializes lastUsedBrainKeys: stop JSON.parse/stringify and treat the value from useBrowserStorage as an already-deserialized object. In resolvedBrainKey (and the setter usage around setLastUsedBrainKeys), replace JSON.parse(lastUsedBrainKeys)[datasetId] with a safe access like lastUsedBrainKeys?.[datasetId] (guarding for null/undefined) and when updating use setLastUsedBrainKeys(updatedMap) with the updated object (not JSON.stringify(updatedMap)). Update references in resolvedBrainKey, and any code that writes to setLastUsedBrainKeys (lines around setLastUsedBrainKeys usage) to remove manual serialization and handle null initial state by creating a new object before setting.
120-138:⚠️ Potential issue | 🟠 MajorAdd failure handling for both operator executions.
The popover closes at Line 120, but neither
executeOperator(SEARCH_OPERATOR_URI, ...)nor the delegatedINIT_RUN_OPERATOR_URIcall handles errors. If either request fails, the user loses the popover and gets a silent no-op.Suggested fix
+ const handleOperatorError = (error: unknown) => { + console.error("Similarity search failed:", error); + }; + close(); executeOperator(SEARCH_OPERATOR_URI, params, { callback: (result) => { if (result?.delegated) { @@ executeOperator( INIT_RUN_OPERATOR_URI, { ...params, operator_run_id: operatorRunId }, - { callback: () => openPanel() } + { + callback: () => openPanel(), + errorCallback: handleOperatorError, + } ); } else { openPanel(); } }, + errorCallback: handleOperatorError, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx` around lines 120 - 138, The code currently calls close() then executeOperator(SEARCH_OPERATOR_URI, ...) and a nested executeOperator(INIT_RUN_OPERATOR_URI, ...) without any failure handling; add error callbacks (or promise rejection handlers depending on executeOperator API) to both executeOperator calls so that on error you reopen the popover (or call openPanel/restore UI) and surface an error message via the same logger/UI notification used elsewhere; specifically update the executeOperator call for SEARCH_OPERATOR_URI (and the nested INIT_RUN_OPERATOR_URI call) to provide an onError/err callback that calls openPanel() (or reverses close()) and reports the error with context including operator name/params and operator_run_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx`:
- Around line 32-36: The anchorRef prop in SimilarityPopoverProps is too narrow
(MutableRefObject) and should be changed to accept RefObject to match callers
and the Popout component; update the SimilarityPopoverProps definition (the
anchorRef?: ...) to use React.RefObject<HTMLElement> (or
React.RefObject<HTMLElement> | null) instead of MutableRefObject<HTMLElement>,
and update any usages/interfaces that reference SimilarityPopoverProps or pass
anchorRef to Popout to ensure types align with Popout's expected
RefObject<HTMLElement> | null.
---
Duplicate comments:
In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx`:
- Around line 54-68: The code double-serializes lastUsedBrainKeys: stop
JSON.parse/stringify and treat the value from useBrowserStorage as an
already-deserialized object. In resolvedBrainKey (and the setter usage around
setLastUsedBrainKeys), replace JSON.parse(lastUsedBrainKeys)[datasetId] with a
safe access like lastUsedBrainKeys?.[datasetId] (guarding for null/undefined)
and when updating use setLastUsedBrainKeys(updatedMap) with the updated object
(not JSON.stringify(updatedMap)). Update references in resolvedBrainKey, and any
code that writes to setLastUsedBrainKeys (lines around setLastUsedBrainKeys
usage) to remove manual serialization and handle null initial state by creating
a new object before setting.
- Around line 120-138: The code currently calls close() then
executeOperator(SEARCH_OPERATOR_URI, ...) and a nested
executeOperator(INIT_RUN_OPERATOR_URI, ...) without any failure handling; add
error callbacks (or promise rejection handlers depending on executeOperator API)
to both executeOperator calls so that on error you reopen the popover (or call
openPanel/restore UI) and surface an error message via the same logger/UI
notification used elsewhere; specifically update the executeOperator call for
SEARCH_OPERATOR_URI (and the nested INIT_RUN_OPERATOR_URI call) to provide an
onError/err callback that calls openPanel() (or reverses close()) and reports
the error with context including operator name/params and operator_run_id.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 080894b9-a979-4802-81a0-941b8b73fb0f
📥 Commits
Reviewing files that changed from the base of the PR and between a92b85f4fa2ff41a1e0ac846e528598d96ee312e and 792bf358d0b4a0600bea45014ab1c79d04cf36cb.
📒 Files selected for processing (3)
app/packages/core/src/components/Actions/Similarity/Similar.tsxapp/packages/similarity-search/src/constants.tsapp/packages/similarity-search/src/hooks/useRuns.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/packages/core/src/components/Actions/Similarity/Similar.tsx (2)
134-159:⚠️ Potential issue | 🟠 MajorHandle operator failures before discarding the popover state.
The popover is closed first, and this request path has no failure callback. If the operator call errors, the user loses the query input, gets no feedback, and the panel never opens.
#!/bin/bash set -euo pipefail echo "Current SimilarityPopover submission flow:" sed -n '134,170p' app/packages/core/src/components/Actions/Similarity/Similar.tsx echo echo "executeOperator definition and option types:" rg -n -C3 'export .*executeOperator|function executeOperator|const executeOperator' app/packages echo echo "Existing failure-handled operator submission patterns:" rg -n -C3 'errorCallback|handleError|onError' app/packages/core/src/components app/packages/similarity-search/src/hooks🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx` around lines 134 - 159, The popover is closed immediately before calling executeOperator(SEARCH_OPERATOR_URI, params, ...) which means on failures users lose input and the panel never opens; modify the flow so you only call close() and clear fos.modalSelector after a successful operator run: move close()/set(fos.modalSelector, null) into the success path (onSearchComplete) and add an error/failure handler to both executeOperator calls (the SEARCH_OPERATOR_URI call and the delegated INIT_RUN_OPERATOR_URI call) to restore the popover state (do not call openPanel) and surface an error (e.g., show a toast or set error state) when either operator fails; keep the existing onSearchComplete/openPanel behavior for success and ensure you reference executeOperator, SEARCH_OPERATOR_URI, INIT_RUN_OPERATOR_URI, params, onSearchComplete, close, set(fos.modalSelector, ...), and openPanel when making these changes.
6-9:⚠️ Potential issue | 🟠 MajorWiden
anchorRefto the ref type the callers actually pass.
MutableRefObject<HTMLElement>is narrower than the usualuseRef<HTMLDivElement>(null)result and narrower than thePopoutanchor contract, so this either forces casts or fails type-checking.Proposed fix
import React, { - type MutableRefObject, + type RefObject, useCallback, useMemo, useState, } from "react"; @@ interface SimilarityPopoverProps { modal: boolean; isImageSearch: boolean; close: () => void; - anchorRef?: MutableRefObject<HTMLElement>; + anchorRef?: RefObject<HTMLElement | null>; }#!/bin/bash set -euo pipefail echo "SimilarityPopover prop type:" rg -n -C2 'anchorRef\?:' app/packages/core/src/components/Actions/Similarity/Similar.tsx echo echo "Caller-side ref creation and prop passing:" rg -n -C2 'useRef<|anchorRef=' app/packages/core/src/components/Actions/Similarity/index.tsx echo echo "Popout anchorRef prop type:" rg -n -C2 'anchorRef\?:|anchorRef=' app/packages/core/src/components/Actions/Popout.tsxAlso applies to: 33-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx` around lines 6 - 9, The prop type for anchorRef in Similar.tsx is too narrow (MutableRefObject<HTMLElement>) and conflicts with callers that pass useRef<HTMLDivElement>(null) and the Popout anchor contract; update the anchorRef prop type on the Similar component to accept the broader ref shapes used by callers (e.g., React.RefObject<HTMLElement | null> or MutableRefObject<HTMLElement | null> and allow undefined), mirror that wider type where Similar passes anchorRef into Popout, and remove any forced casts at call sites so the ref flows without type errors (look for the anchorRef prop in Similar.tsx, the caller in index.tsx, and Popout's anchorRef type to align them).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx`:
- Around line 71-82: The submit path currently does an unguarded JSON.parse of
lastUsedBrainKeys which can throw on malformed/stale storage and abort the
search; make the write side defensive just like the read side by wrapping
JSON.parse(...) in a try/catch (or use a safe parse helper) before
merging/updating the object for datasetId, falling back to an empty object when
parse fails, then safely stringify and write back; update the code in the submit
handler (the block referencing lastUsedBrainKeys around lines ~129-132) and keep
behavior consistent with the useMemo resolvedBrainKey logic so malformed stored
values never throw.
- Around line 205-208: The warning text is hard-coded and incorrectly references
a selected label/patch flow even when the modal is doing sample-based search;
update the UI to render context-aware messages instead of the fixed string.
Replace the PopoutSectionTitle usage that currently shows "No similarity index
found for the selected label field" (rendered when showNoIndexWarning is true)
with a conditional that checks whether a label field is actually selected (e.g.,
selectedLabelField or the modal's label selection state) and render one message
for label-based searches and a different message for sample-based fallback
(e.g., mention "sample-based search using sample ID" or similar). Apply the same
conditional replacement for the other similar block around lines 226-230 so both
warnings/CTAs reflect the active search mode.
- Around line 116-127: The operator payload currently built in the block around
resolvePatchesField(...) omits the active view stages so searches fall back to
the full dataset; add a source_view property to the params object containing the
current view's stages (e.g., JSON-serialised view stages from your component's
active view — something like currentView?.stages or via getActiveViewStages()),
ensuring you set params.source_view only when those stages exist; keep the rest
of the payload (brain_key, query_type, query, reverse, k) unchanged so
plugins/panels/similarity_search/operators.py will use the intended
filtered/sliced view.
---
Duplicate comments:
In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx`:
- Around line 134-159: The popover is closed immediately before calling
executeOperator(SEARCH_OPERATOR_URI, params, ...) which means on failures users
lose input and the panel never opens; modify the flow so you only call close()
and clear fos.modalSelector after a successful operator run: move
close()/set(fos.modalSelector, null) into the success path (onSearchComplete)
and add an error/failure handler to both executeOperator calls (the
SEARCH_OPERATOR_URI call and the delegated INIT_RUN_OPERATOR_URI call) to
restore the popover state (do not call openPanel) and surface an error (e.g.,
show a toast or set error state) when either operator fails; keep the existing
onSearchComplete/openPanel behavior for success and ensure you reference
executeOperator, SEARCH_OPERATOR_URI, INIT_RUN_OPERATOR_URI, params,
onSearchComplete, close, set(fos.modalSelector, ...), and openPanel when making
these changes.
- Around line 6-9: The prop type for anchorRef in Similar.tsx is too narrow
(MutableRefObject<HTMLElement>) and conflicts with callers that pass
useRef<HTMLDivElement>(null) and the Popout anchor contract; update the
anchorRef prop type on the Similar component to accept the broader ref shapes
used by callers (e.g., React.RefObject<HTMLElement | null> or
MutableRefObject<HTMLElement | null> and allow undefined), mirror that wider
type where Similar passes anchorRef into Popout, and remove any forced casts at
call sites so the ref flows without type errors (look for the anchorRef prop in
Similar.tsx, the caller in index.tsx, and Popout's anchorRef type to align
them).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d46eb29-f914-4e88-9045-4814982add02
📥 Commits
Reviewing files that changed from the base of the PR and between 792bf358d0b4a0600bea45014ab1c79d04cf36cb and 8605bff6c6ada98816331f53773aa47171887297.
📒 Files selected for processing (5)
app/packages/core/src/components/Actions/Similarity/Similar.tsxapp/packages/core/src/components/Actions/Similarity/utils.tsapp/packages/core/src/components/Modal/Actions/index.tsxplugins/panels/similarity_search/__init__.pyplugins/panels/similarity_search/operators.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
app/packages/core/src/components/Actions/Similarity/Similar.tsx (4)
29-33:⚠️ Potential issue | 🟠 MajorBroaden
anchorReftoRefObject.
SimilaritypassesuseRef<HTMLDivElement>(null), but this prop only acceptsMutableRefObject<HTMLElement>. That is narrower than the caller and thePopoutboundary, so this can fail type-checking under the current React typings.Suggested fix
import React, { - type MutableRefObject, + type RefObject, useCallback, useMemo, useState, } from "react"; @@ interface SimilarityPopoverProps { modal: boolean; isImageSearch: boolean; close: () => void; - anchorRef?: MutableRefObject<HTMLElement>; + anchorRef?: RefObject<HTMLElement | null>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx` around lines 29 - 33, The prop type for anchorRef in SimilarityPopoverProps is too narrow (MutableRefObject<HTMLElement>) and can reject callers using RefObject like useRef<HTMLDivElement>(null); update the type of anchorRef to accept React.RefObject<HTMLElement> or the broader React.RefObject<HTMLElement> | React.MutableRefObject<HTMLElement> (or simply React.RefObject<HTMLElement> | null) so the Similarity component and the Popout boundary accept useRef<HTMLDivElement>(null) without type errors; adjust the interface SimilarityPopoverProps and any consumers (e.g., Similarity, Popout) to use the broader ref type.
125-127:⚠️ Potential issue | 🟡 MinorGuard the write-path storage parse.
resolvedBrainKeyalready treats malformed storage defensively, but this submit path does an unguardedJSON.parse. A stale browser value will throw here and abort the search before the operator runs.Suggested fix
- const current = lastUsedBrainKeys ? JSON.parse(lastUsedBrainKeys) : {}; + let current = {}; + try { + current = lastUsedBrainKeys ? JSON.parse(lastUsedBrainKeys) : {}; + } catch { + current = {}; + } setLastUsedBrainKeys( JSON.stringify({ ...current, [datasetId]: resolvedBrainKey }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx` around lines 125 - 127, Guard the JSON.parse of lastUsedBrainKeys before writing: in the submit path that calls setLastUsedBrainKeys, wrap parsing of lastUsedBrainKeys in a safe try/catch (or use a safeParse helper) and fall back to an empty object on error, then merge { [datasetId]: resolvedBrainKey } into that safe current object and stringify for setLastUsedBrainKeys; update the code around lastUsedBrainKeys / setLastUsedBrainKeys in Similar.tsx to use this guarded parse so a stale/malformed stored value doesn't throw and abort the operator.
213-216:⚠️ Potential issue | 🟡 MinorMake the no-index warning match the actual search mode.
When modal image search falls back to the sample ID because no label is selected, this still says “selected label field”. That misdescribes the path the user is on.
Suggested fix
{showNoIndexWarning && ( <PopoutSectionTitle style={{ fontSize: 12 }}> - No similarity index found for the selected label field + {hasSelectedLabels + ? "No similarity index found for the selected label field" + : "No similarity index found for sample-based similarity search"} </PopoutSectionTitle> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx` around lines 213 - 216, The warning text "No similarity index found for the selected label field" is inaccurate when modal image search falls back to using sample ID; update the message in Similar.tsx where showNoIndexWarning is rendered (the PopoutSectionTitle) to reflect the actual search mode: detect whether a label field is selected (or whether modal image search is falling back to sample ID) and render a conditional string such as "No similarity index found for the selected label field" when a label is chosen, otherwise "No similarity index found for the current search mode (searching by sample ID)" — adjust the conditional logic around showNoIndexWarning (or use the existing selectedLabel/ searchMode state) so the displayed text matches the active search path.
112-123:⚠️ Potential issue | 🟠 MajorInclude the active view in the operator payload.
This payload never passes the current grid view. Searches started from a filtered or sliced grid can therefore run against the full dataset instead of the active view.
Suggested fix
async () => { if (!resolvedBrainKey) return; + const sourceView = await snapshot.getPromise(fos.view); const queryIds = isImageSearch ? await getQueryIds(snapshot, resolvedBrainKey) : undefined; @@ const params: Record<string, unknown> = { brain_key: resolvedBrainKey, query_type: isImageSearch ? "image" : "text", query: isImageSearch ? queryIds : textQuery.trim(), reverse: false, k: DEFAULT_K, + source_view: sourceView, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx` around lines 112 - 123, The operator payload built in params (in Similar.tsx near resolvePatchesField) is missing the current active view, so searches ignore grid filters; add the active view identifier to the params object (e.g., params.active_view = activeView or params.view_id = activeGridView depending on the component prop/name you have) before sending the request so the search uses the current grid slice/filter; ensure you reference the actual component variable that holds the active view (for example activeView, currentView, or activeGridView) and include it alongside existing keys like brain_key, query_type, query, reverse, and k.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx`:
- Around line 29-33: The prop type for anchorRef in SimilarityPopoverProps is
too narrow (MutableRefObject<HTMLElement>) and can reject callers using
RefObject like useRef<HTMLDivElement>(null); update the type of anchorRef to
accept React.RefObject<HTMLElement> or the broader React.RefObject<HTMLElement>
| React.MutableRefObject<HTMLElement> (or simply React.RefObject<HTMLElement> |
null) so the Similarity component and the Popout boundary accept
useRef<HTMLDivElement>(null) without type errors; adjust the interface
SimilarityPopoverProps and any consumers (e.g., Similarity, Popout) to use the
broader ref type.
- Around line 125-127: Guard the JSON.parse of lastUsedBrainKeys before writing:
in the submit path that calls setLastUsedBrainKeys, wrap parsing of
lastUsedBrainKeys in a safe try/catch (or use a safeParse helper) and fall back
to an empty object on error, then merge { [datasetId]: resolvedBrainKey } into
that safe current object and stringify for setLastUsedBrainKeys; update the code
around lastUsedBrainKeys / setLastUsedBrainKeys in Similar.tsx to use this
guarded parse so a stale/malformed stored value doesn't throw and abort the
operator.
- Around line 213-216: The warning text "No similarity index found for the
selected label field" is inaccurate when modal image search falls back to using
sample ID; update the message in Similar.tsx where showNoIndexWarning is
rendered (the PopoutSectionTitle) to reflect the actual search mode: detect
whether a label field is selected (or whether modal image search is falling back
to sample ID) and render a conditional string such as "No similarity index found
for the selected label field" when a label is chosen, otherwise "No similarity
index found for the current search mode (searching by sample ID)" — adjust the
conditional logic around showNoIndexWarning (or use the existing selectedLabel/
searchMode state) so the displayed text matches the active search path.
- Around line 112-123: The operator payload built in params (in Similar.tsx near
resolvePatchesField) is missing the current active view, so searches ignore grid
filters; add the active view identifier to the params object (e.g.,
params.active_view = activeView or params.view_id = activeGridView depending on
the component prop/name you have) before sending the request so the search uses
the current grid slice/filter; ensure you reference the actual component
variable that holds the active view (for example activeView, currentView, or
activeGridView) and include it alongside existing keys like brain_key,
query_type, query, reverse, and k.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0b11daa-2640-4a34-9ee1-f549443c2f18
📥 Commits
Reviewing files that changed from the base of the PR and between 8605bff6c6ada98816331f53773aa47171887297 and 1f6bc62dcf88654ec99e65fa13c7d3d2030f9a34.
📒 Files selected for processing (1)
app/packages/core/src/components/Actions/Similarity/Similar.tsx
lanzhenw
force-pushed
the
feat/wires-up-grid-action
branch
from
1f6bc62 to
d65b067
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/core/src/components/Actions/Similarity/Similar.tsx`:
- Around line 144-155: The ToPatches stage kwargs currently only include the
field; update the executeOperator call for "set_view" so the kwargs array
matches the canonical pattern used in useToPatches.ts by adding the ["_state",
null] entry alongside ["field", patchesField] (i.e., include the _state
parameter set to null in the kwargs for the ToPatches stage passed to
executeOperator) so the stage is constructed correctly when openPanel() is
called.
In `@app/packages/core/src/components/Actions/Similarity/utils.ts`:
- Around line 18-26: The current logic in the block using
snapshot.getPromise(fos.similarityMethods) and computing labels_field can return
an empty array when selectedLabelIds exist but labels_field is undefined or no
labels match that field; change the branch in the function that uses
selectedLabelIds/selectedLabelMap (the block referencing labels_field) to detect
when labels_field is falsy or when the filtered result is empty and, in that
case, return a fallback containing modalSampleId instead of an empty array;
specifically update the code that computes labels_field and the subsequent
return so that if !labels_field or [...selectedLabelIds].filter(...).length ===
0 you return [modalSampleId], otherwise return the filtered ids.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: febe50d7-b8b3-43b6-a511-97ecab3cf50e
📥 Commits
Reviewing files that changed from the base of the PR and between 1f6bc62dcf88654ec99e65fa13c7d3d2030f9a34 and d65b0672c414ca5b020eb82244d80faf58fd52cf.
📒 Files selected for processing (5)
app/packages/core/src/components/Actions/Similarity/Similar.tsxapp/packages/core/src/components/Actions/Similarity/utils.tsapp/packages/core/src/components/Modal/Actions/index.tsxplugins/panels/similarity_search/__init__.pyplugins/panels/similarity_search/operators.py
lanzhenw
changed the title
tom-vx51
approved these changes
Apr 9, 2026
imanjra
reviewed
Apr 9, 2026
Contributor
imanjra
left a comment
imanjra
left a comment
There was a problem hiding this comment.
can we please add a loading state. There is decent delay before panel opens and it looks like the action did not work to the user
Contributor
|
Couple of thoughts from local testing:
|
lanzhenw
force-pushed
the
feat/wires-up-grid-action
branch
from
bbce196 to
aea065a
Compare
lanzhenw
changed the title
…tead, always run sort on dataset.view()
lanzhenw
force-pushed
the
feat/wires-up-grid-action
branch
from
c69c432 to
3dc98bf
Compare
Member
Author
|
When no index applicable, clicking the icon opens the panel directly; Screen.Recording.2026-04-09.at.5.02.08.PM.mov |
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
3 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.
🔗 Related Issues
closes #FOEPD-3370
📋 What changes are proposed in this pull request?
Summary
Replaces the old direct POST /sort similarity flow with an operator-based flow that triggers @voxel51/panels/similarity_search and opens the similarity search panel.
Popover changes:
Panel changes:
Operator changes:
Modal:
🧪 How is this patch tested? If it is not, please explain why.
Compute similarity index on dataset and on
predictionspatch view:select label -> one click patch search:
select_label_search_similar_patch.mov
select sample -> one click sample search:
select-sample-one-click-search.mov
text -> one click text search:
text-search.mov
📝 Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Improvements
Bug Fixes