[SSP] bugfix and UI improvements#7346
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:
WalkthroughThis PR adds explicit query-type (image/text) and search-scope (view/dataset) constants; threads sequenceDiagram
participant User
participant Frontend
participant Operator
participant Dataset
User->>Frontend: start search (uses QUERY_TYPE_*, SCOPE_*)
Frontend->>Operator: executeOperator(similarity_search, SimilaritySearchParams)
Operator->>Dataset: if search_scope == "view" use ctx.view else dataset.view()
Dataset-->>Operator: return matched ids (result_ids)
Operator->>Operator: persist result_ids (dynamic_results = false by default)
Operator-->>Frontend: return search results + run metadata
Frontend->>Frontend: update runs, set highlightedRunId if run completed
User->>Frontend: interact with RunList (highlight/apply/clone/delete)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
lanzhenw
commented
Apr 11, 2026
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/packages/similarity-search/src/__tests__/utils.test.ts (1)
1-12:⚠️ Potential issue | 🟠 MajorReintroduce direct tests for
buildExecutionParams.This PR changes operator payload assembly, but this suite no longer asserts
buildExecutionParamsbehavior (notablysearch_scope, upload payload fields, and run-name fallback). Please restore focused unit coverage to prevent silent request-shape regressions.Suggested test additions
+import { buildExecutionParams } from "../utils"; +import { QueryType } from "../types"; + +describe("buildExecutionParams", () => { + it("propagates search_scope and trims text query", () => { + const params = buildExecutionParams({ + brainKey: "clip", + queryType: QueryType.Text, + textQuery: " dogs ", + queryIds: [], + reverse: false, + patchesField: undefined, + searchScope: "view", + hasView: true, + view: [], + k: 25, + distField: "", + runName: "", + negativeQueryIds: [], + dynamicResults: false, + uploadedImage: null, + }); + expect(params.search_scope).toBe("view"); + expect(params.query).toBe("dogs"); + expect(params.run_name).toBe("dogs"); + }); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/similarity-search/src/__tests__/utils.test.ts` around lines 1 - 12, Tests removed coverage for buildExecutionParams causing regressions in operator payload shape; restore focused unit tests by adding assertions that directly call buildExecutionParams and validate the produced payload fields: verify search_scope is set correctly, uploaded file payload fields are present and populated, and that run-name falls back to the expected default when not provided. Target the buildExecutionParams export in the same module used by the suite, create inputs for different QueryType/SimilarityRun scenarios (including upload vs non-upload modes), and assert the resulting object's search_scope, upload-related keys, and run name behavior to guard against request-shape regressions.app/packages/similarity-search/src/components/NewSearch/NewSearch.tsx (1)
337-348: 🧹 Nitpick | 🔵 TrivialRemove the commented-out Dynamic Results JSX block.
Please delete this inactive UI block and control behavior via state defaults/feature gating. Leaving commented JSX in the component invites drift and future confusion.
Cleanup diff
- {/* Dynamic results — commented out; all searches use cached (static) results for now - <Toggle - checked={form.dynamicResults} - onChange={(checked) => form.setDynamicResults(checked)} - label={ - form.dynamicResults - ? "Dynamic results — results will always reflect the latest dataset state, but loading will be slower" - : "Dynamic results" - } - size={Size.Sm} - /> - */}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/similarity-search/src/components/NewSearch/NewSearch.tsx` around lines 337 - 348, Remove the commented-out Dynamic Results JSX block inside the NewSearch component (the Toggle block referencing form.dynamicResults) to avoid dead/commented UI; instead enforce the behavior via state defaults or a feature flag in the form initialization or surrounding logic (e.g., set form.dynamicResults default or check a feature gate) and delete the entire commented JSX section so the component only reflects active UI and state-driven behavior.plugins/panels/similarity_search/operators.py (1)
152-173:⚠️ Potential issue | 🟠 MajorGuard cached mode when
kis unbounded.With
dynamic_resultsnow defaulting toFalse, any request that omitskwill materialize the entire sorted view intoresult_idsbefore saving the run. On large views that turns every search into an O(N) memory/write spike and can bloat the execution store. Keep cached mode for bounded searches only, or fall back to serialized view whenkis unset.Suggested safeguard
- dynamic_results = ctx.params.get("dynamic_results", False) + dynamic_results = ctx.params.get("dynamic_results", False) + if k is None and not dynamic_results: + dynamic_results = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/panels/similarity_search/operators.py` around lines 152 - 173, The code currently materializes result_ids whenever dynamic_results is False, which will O(N) explode if the caller omits k; change the branch to detect an unbounded search (e.g., k missing or None via ctx.params.get("k")) and in that case treat the run as dynamic (serialize the view into result_view_stages and set result_count from len(result_view)) instead of building result_ids; only build result_ids when dynamic_results is False and a bounded k is present (use k to limit the IDs). Ensure you update the logic around dynamic_results, result_ids, result_view_stages, and result_count accordingly so unbounded searches are serialized rather than cached as full ID lists.
🤖 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/similarity-search/src/components/Home/RunList.tsx`:
- Around line 196-203: The row-level onClick in RunList that calls
onApply(run.run_id) is firing when nested RunActions buttons are clicked; update
the row click logic or the RunActions buttons so interactive descendants don't
trigger the row handler: either change the row onClick (the inline handler in
RunList.tsx that currently conditionally calls onApply) to accept the click
event and early-return when the event target is an interactive descendant (e.g.,
button, a, input, [role="button"], etc.), or add stopPropagation() to the action
handlers inside RunActions (Clone/Delete/Expand) so their clicks don’t bubble up
to onApply. Ensure you modify the inline onClick or the RunActions handlers
(whichever you choose) referencing onApply and RunActions.
In `@app/packages/similarity-search/src/hooks/useFilteredRuns.ts`:
- Around line 24-33: The effect is patching shared filterStateAtom after render,
causing an initial render with OWNER_ALL and risking overwriting a user's manual
choice; instead set the intended default synchronously before the first render.
Initialize or reset the atom's ownerFilter to OWNER_MINE when a currentUser is
present (e.g., in the filterStateAtom initialiser or a synchronous setup routine
used by useFilteredRuns) rather than using the mount-time useEffect, and only do
this when the atom is still at the original OWNER_ALL sentinel (so you don't
overwrite an existing user-selected value); update references to
defaultsApplied, setFilterState, OWNER_MINE and OWNER_ALL accordingly.
In `@app/packages/similarity-search/src/hooks/useRuns.ts`:
- Around line 53-56: The current refreshRuns behavior returns Promise.resolve()
when fetchingRef.current is true, which breaks await semantics for callers
(e.g., code awaiting refreshRuns in useSimilarityPanel) and causes race
conditions; change refreshRuns so that when a fetch is in-flight it records and
returns the actual in-flight Promise instead of Promise.resolve(): store the
running promise (e.g., in a ref like inFlightRefreshRef or reuse fetchingRef to
hold the Promise) and set pendingRefreshRef.current to true, then return that
stored Promise so callers awaiting refreshRuns wait for the queued refresh to
complete; update the logic around fetchingRef.current,
pendingRefreshRef.current, and the refreshRuns function to create/return the
single in-flight Promise for all queued callers.
- Around line 83-100: The pending-refresh drain calls to refreshRuns() in both
the finally block and the catch block can return Promises and currently are not
awaited/consumed, causing unhandled rejections; update both spots (the call
inside the finally that checks pendingRefreshRef.current and the one inside the
catch handler) to explicitly consume rejections from refreshRuns() (e.g., append
a .catch(...) that logs the error and clears pendingRefreshRef) so any failure
is observed and the pending flag is still cleared.
In `@app/packages/similarity-search/src/mui.ts`:
- Around line 12-13: Update the module-level comment/header in mui.ts to
accurately reflect the current exports: mention that besides layout components
this module also re-exports the FileUploadOutlined icon. Locate the top-of-file
header comment and change the text to note that icons (e.g., FileUploadOutlined)
are exported alongside layout components so docs match the actual exports.
---
Outside diff comments:
In `@app/packages/similarity-search/src/__tests__/utils.test.ts`:
- Around line 1-12: Tests removed coverage for buildExecutionParams causing
regressions in operator payload shape; restore focused unit tests by adding
assertions that directly call buildExecutionParams and validate the produced
payload fields: verify search_scope is set correctly, uploaded file payload
fields are present and populated, and that run-name falls back to the expected
default when not provided. Target the buildExecutionParams export in the same
module used by the suite, create inputs for different QueryType/SimilarityRun
scenarios (including upload vs non-upload modes), and assert the resulting
object's search_scope, upload-related keys, and run name behavior to guard
against request-shape regressions.
In `@app/packages/similarity-search/src/components/NewSearch/NewSearch.tsx`:
- Around line 337-348: Remove the commented-out Dynamic Results JSX block inside
the NewSearch component (the Toggle block referencing form.dynamicResults) to
avoid dead/commented UI; instead enforce the behavior via state defaults or a
feature flag in the form initialization or surrounding logic (e.g., set
form.dynamicResults default or check a feature gate) and delete the entire
commented JSX section so the component only reflects active UI and state-driven
behavior.
In `@plugins/panels/similarity_search/operators.py`:
- Around line 152-173: The code currently materializes result_ids whenever
dynamic_results is False, which will O(N) explode if the caller omits k; change
the branch to detect an unbounded search (e.g., k missing or None via
ctx.params.get("k")) and in that case treat the run as dynamic (serialize the
view into result_view_stages and set result_count from len(result_view)) instead
of building result_ids; only build result_ids when dynamic_results is False and
a bounded k is present (use k to limit the IDs). Ensure you update the logic
around dynamic_results, result_ids, result_view_stages, and result_count
accordingly so unbounded searches are serialized rather than cached as full ID
lists.
🪄 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: 9e3e957a-e1c1-4142-a89c-211e3d7b0bc1
📒 Files selected for processing (15)
app/packages/core/src/components/Actions/Similarity/constants.tsapp/packages/core/src/components/Actions/Similarity/useSimilarityPopover.tsapp/packages/core/src/components/Actions/Similarity/utils.tsapp/packages/similarity-search/src/__tests__/utils.test.tsapp/packages/similarity-search/src/components/Home/RunActions.tsxapp/packages/similarity-search/src/components/Home/RunList.tsxapp/packages/similarity-search/src/components/NewSearch/NewSearch.tsxapp/packages/similarity-search/src/constants.tsapp/packages/similarity-search/src/hooks/useFilteredRuns.tsapp/packages/similarity-search/src/hooks/useRuns.tsapp/packages/similarity-search/src/mui.tsapp/packages/similarity-search/src/types/index.tsapp/packages/similarity-search/src/utils.tsplugins/panels/similarity_search/__init__.pyplugins/panels/similarity_search/operators.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/similarity-search/src/components/Home/RunList.tsx`:
- Around line 206-214: The inline style object for the highlighted item is
recreated on every render; change the code in RunList to avoid recreating it by
extracting the highlighted style into a constant or CSS class and then
conditionally apply/merge it (e.g., create HIGHLIGHT_STYLE = { boxShadow: "...",
borderRadius: 6 } or a CSS class and use it when highlightedRunId ===
run.run_id), or memoize the merged style with useMemo keyed on (highlightedRunId
=== run.run_id, selectMode) so you only recreate the style when the highlight
state changes; reference highlightedRunId, run.run_id and selectMode when
implementing this.
In `@app/packages/similarity-search/src/components/SimilaritySearchCTA.tsx`:
- Around line 21-26: Extract the inline style object used in the
UNSUPPORTED_WARNING constant into a named constant or a CSS class to avoid
embedding inline styles and to match project styling conventions; update the
UNSUPPORTED_WARNING declaration in SimilaritySearchCTA.tsx to reference the new
STYLE_CONST or CSS class name (or a styled-component) instead of the inline
object so the style is centralized and reused.
- Line 42: The inline arrow passed to PanelCTA as the Actions prop creates a new
component identity every render (in SimilaritySearchCTA.tsx), causing
unnecessary re-renders; change this to a stable memoized value by creating a
stable Actions renderer using React.useCallback or React.useMemo (e.g., memoize
a function/component that returns <Actions isUnsupported={isUnsupported} />) and
pass that memoized reference to PanelCTA instead of () => <Actions ... /> so
that PanelCTA sees a stable Actions prop across renders.
In `@app/packages/similarity-search/src/hooks/useSimilarityPanel.ts`:
- Around line 342-348: The useCallback for handleApply includes the
referentially-stable state setter setHighlightedRunId in its dependency array
unnecessarily; remove setHighlightedRunId from the dependency list and only keep
actions.handleApply so the callback depends on the external action but not the
stable React state setter (update the dependency array of the handleApply
useCallback accordingly).
🪄 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: 331370e9-ded8-4f90-bfd8-a9bfe21fedd5
📥 Commits
Reviewing files that changed from the base of the PR and between f59c6d0 and 8125f7a20a09f88cfbcdaff17f308068623695e8.
📒 Files selected for processing (5)
app/packages/similarity-search/src/components/Home/RunList.tsxapp/packages/similarity-search/src/components/SimilaritySearchCTA.tsxapp/packages/similarity-search/src/components/SimilaritySearchView.tsxapp/packages/similarity-search/src/hooks/useSimilarityPanel.tsplugins/panels/similarity_search/__init__.py
lanzhenw
force-pushed
the
bugfix/ssp-current-view
branch
from
8125f7a to
7180d02
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/packages/similarity-search/src/hooks/useSimilarityPanel.ts (1)
301-307: 🧹 Nitpick | 🔵 Trivial
setHighlightedRunIdis stable and unnecessary in deps.React
useStatesetters are referentially stable across renders. Including it in the dependency array is harmless but redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/similarity-search/src/hooks/useSimilarityPanel.ts` around lines 301 - 307, The dependency array for the useCallback handleApplyWithHighlight in useSimilarityPanel includes the stable state setter setHighlightedRunId unnecessarily; remove setHighlightedRunId from the dependencies so the array becomes [actions] to avoid redundant deps while keeping correct behavior of handleApplyWithHighlight (which calls setHighlightedRunId and actions.handleApply).app/packages/similarity-search/src/hooks/useRuns.ts (1)
69-78:⚠️ Potential issue | 🟡 MinorUnhandled promise rejection in queued refresh drain.
When
drainPendingtriggers the recursiverefreshRuns()call on line 74, any rejection is unobserved. Callers awaiting the original promise have already resolved/rejected, so this new promise is orphaned. Add explicit error handling to avoid silent failures.🛡️ Proposed fix
if ( pendingRefreshRef.current && consecutiveErrorsRef.current < MAX_RETRY ) { pendingRefreshRef.current = false; - refreshRuns(); + void refreshRuns().catch((err) => { + console.error("Queued refresh failed:", err); + }); } else { pendingRefreshRef.current = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/similarity-search/src/hooks/useRuns.ts` around lines 69 - 78, The queued refresh drain (drainPending) calls refreshRuns() when pendingRefreshRef.current is true and consecutiveErrorsRef.current < MAX_RETRY but does not handle promise rejection, leaving the new refreshRuns() promise orphaned; update drainPending to handle errors from refreshRuns by attaching a .catch handler or awaiting it inside a try/catch so any rejection is observed, and in the catch update consecutiveErrorsRef (or other retry logic) and log the error via your logger; reference functions/refs: drainPending, refreshRuns, pendingRefreshRef, consecutiveErrorsRef, and constant MAX_RETRY.app/packages/similarity-search/src/components/Home/RunList.tsx (1)
209-217: 🧹 Nitpick | 🔵 TrivialInline style object recreated on every render.
The highlight/cursor style object is recreated for each list item on every render. This was flagged in a previous review but remains unaddressed. Consider extracting to constants or memoizing if performance becomes a concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/similarity-search/src/components/Home/RunList.tsx` around lines 209 - 217, The inline style object in RunList.tsx (used where style is built from selectMode, highlightedRunId and run.run_id) is recreated on every render; refactor to avoid allocations by extracting static parts into constants and memoizing the per-row style—e.g., create a constant baseStyle and highlightedStyle outside the render, and inside the row renderer use useMemo keyed on [selectMode, highlightedRunId === run.run_id] (or compute a boolean isHighlighted = highlightedRunId === run.run_id and memoize the final style) or replace with conditional className/CSS classes to avoid building a new object each render.
🤖 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/similarity-search/src/utils.ts`:
- Line 2: You are importing buildRunName from core's internal src which creates
a cross-package, internal-path dependency; remove the import from
"@fiftyone/core/src/components/Actions/Similarity/utils" in
app/packages/similarity-search/src/utils.ts and either (A) move buildRunName
into a neutral shared module (create a small shared utility package or export it
from core's public API) and import it from that stable package, or (B) paste a
local copy of buildRunName into similarity-search's utils.ts and use that;
update any references to buildRunName to use the new local/shared symbol and
ensure no imports reference core/src.
---
Duplicate comments:
In `@app/packages/similarity-search/src/components/Home/RunList.tsx`:
- Around line 209-217: The inline style object in RunList.tsx (used where style
is built from selectMode, highlightedRunId and run.run_id) is recreated on every
render; refactor to avoid allocations by extracting static parts into constants
and memoizing the per-row style—e.g., create a constant baseStyle and
highlightedStyle outside the render, and inside the row renderer use useMemo
keyed on [selectMode, highlightedRunId === run.run_id] (or compute a boolean
isHighlighted = highlightedRunId === run.run_id and memoize the final style) or
replace with conditional className/CSS classes to avoid building a new object
each render.
In `@app/packages/similarity-search/src/hooks/useRuns.ts`:
- Around line 69-78: The queued refresh drain (drainPending) calls refreshRuns()
when pendingRefreshRef.current is true and consecutiveErrorsRef.current <
MAX_RETRY but does not handle promise rejection, leaving the new refreshRuns()
promise orphaned; update drainPending to handle errors from refreshRuns by
attaching a .catch handler or awaiting it inside a try/catch so any rejection is
observed, and in the catch update consecutiveErrorsRef (or other retry logic)
and log the error via your logger; reference functions/refs: drainPending,
refreshRuns, pendingRefreshRef, consecutiveErrorsRef, and constant MAX_RETRY.
In `@app/packages/similarity-search/src/hooks/useSimilarityPanel.ts`:
- Around line 301-307: The dependency array for the useCallback
handleApplyWithHighlight in useSimilarityPanel includes the stable state setter
setHighlightedRunId unnecessarily; remove setHighlightedRunId from the
dependencies so the array becomes [actions] to avoid redundant deps while
keeping correct behavior of handleApplyWithHighlight (which calls
setHighlightedRunId and actions.handleApply).
🪄 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: 36dd3961-dc3d-47db-9f40-0ff8d8a62718
📥 Commits
Reviewing files that changed from the base of the PR and between 8125f7a20a09f88cfbcdaff17f308068623695e8 and cae63ac.
📒 Files selected for processing (6)
app/packages/core/src/components/Actions/Similarity/utils.tsapp/packages/similarity-search/src/components/Home/RunList.tsxapp/packages/similarity-search/src/hooks/useFilteredRuns.tsapp/packages/similarity-search/src/hooks/useRuns.tsapp/packages/similarity-search/src/hooks/useSimilarityPanel.tsapp/packages/similarity-search/src/utils.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/packages/similarity-search/src/components/Home/RunList.tsx (1)
206-214: 🧹 Nitpick | 🔵 TrivialExtract the highlight styles out of
listItems.The conditional style object is still recreated for every row on each recomputation. Hoist the pointer/highlight styles to constants or a class to reduce churn here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/similarity-search/src/components/Home/RunList.tsx` around lines 206 - 214, The inline style object for rows in RunList.tsx (the conditional pointer and highlight styles using selectMode, highlightedRunId and run.run_id) is being recreated on every render; hoist those styles into stable constants or a memoized value (e.g., useMemo) or apply a CSS/className so the style object isn’t rebuilt per row. Locate the JSX that builds listItems/row style in the RunList component and replace the inline conditional object with references to the extracted constants or memoized style/highlight class to eliminate churn.
🤖 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/similarity-search/src/components/Home/RunActions.tsx`:
- Around line 51-59: The Show results button in RunActions.tsx triggers
onApply(run.run_id) but lets the click bubble to the row handler in RunList.tsx
which calls onApply again; change the Button onClick to accept the click event,
call event.stopPropagation() (and optionally event.preventDefault()) before
invoking onApply(run.run_id), keeping the existing disabled check, so the row's
click handler will not receive the event and duplicate the apply work.
---
Duplicate comments:
In `@app/packages/similarity-search/src/components/Home/RunList.tsx`:
- Around line 206-214: The inline style object for rows in RunList.tsx (the
conditional pointer and highlight styles using selectMode, highlightedRunId and
run.run_id) is being recreated on every render; hoist those styles into stable
constants or a memoized value (e.g., useMemo) or apply a CSS/className so the
style object isn’t rebuilt per row. Locate the JSX that builds listItems/row
style in the RunList component and replace the inline conditional object with
references to the extracted constants or memoized style/highlight class to
eliminate churn.
🪄 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: a85057f7-f541-486c-98cc-cca10a7435bf
📒 Files selected for processing (2)
app/packages/similarity-search/src/components/Home/RunActions.tsxapp/packages/similarity-search/src/components/Home/RunList.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/packages/similarity-search/src/hooks/useRuns.ts (1)
61-67:⚠️ Potential issue | 🟠 MajorReturn the drained refresh from the queue path.
Line 120 starts a second
refreshRuns(), but queued callers still receive the promise for the first fetch. That breaks theawait refreshRuns()contract for callers likeapp/packages/similarity-search/src/hooks/useSimilarityPanel.ts:197-205, and any failure from the drained refresh is left unobserved.Suggested fix
- inFlightRef.current = promise + inFlightRef.current = promise .then(() => { errorCountRef.current = 0; }) .catch(() => { errorCountRef.current += 1; }) .finally(() => { - inFlightRef.current = null; - if (pendingRef.current && errorCountRef.current < MAX_RETRY) { - pendingRef.current = false; - refreshRuns(); - } else { - pendingRef.current = false; - } + const shouldDrain = + pendingRef.current && errorCountRef.current < MAX_RETRY; + + pendingRef.current = false; + inFlightRef.current = null; + + if (shouldDrain) { + return refreshRuns(); + } });Also applies to: 109-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/similarity-search/src/hooks/useRuns.ts` around lines 61 - 67, The current refreshRuns implementation returns the original inFlightRef.current when a refresh is queued (fetchingRef, pendingRef, inFlightRef), causing queued callers to await the first fetch instead of the drained follow-up; change the queued-path so refreshRuns creates and returns a new promise that resolves/rejects when the drained refresh completes (store that promise in a new queuedRef or attach a chained promise to inFlightRef.current) and ensure pendingRef.current triggers the follow-up fetch to settle that queued promise; apply the same fix to the other queued-return branch that uses fetchingRef/pendingRef/inFlightRef so callers awaiting refreshRuns always get the promise representing the actual work they will observe.app/packages/similarity-search/src/hooks/useFilteredRuns.ts (1)
15-16:⚠️ Potential issue | 🟡 MinorDo not repair
ownerFilterafter mount.The atom still initializes to
OWNER_ALL, so the first render filters against all owners. Then Lines 44-50 mutate shared state on mount, which also turns an intentionalAllselection back intoMineafter remounting the same dataset. Initialize this synchronously per dataset/user, or track an explicit untouched sentinel instead of treatingOWNER_ALLas both the default and a real user choice. Based on learnings, hooks inapp/**/hooks.tsnameduse<Feature>()must be idempotent.Also applies to: 41-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/similarity-search/src/hooks/useFilteredRuns.ts` around lines 15 - 16, The atom filterStateAtom currently initializes ownerFilter to OWNER_ALL but then mutates shared state on mount (in useFilteredRuns) which flips an intentional "All" back to "Mine" on remount; instead make initialization synchronous and per-dataset/user or add an explicit untouched sentinel so OWNER_ALL is not used as both default and a user choice. Concretely: stop mutating filterStateAtom inside the mount effect in useFilteredRuns, change makeDefaultFilterState (or RunFilterState) to accept datasetId/userId (or include an ownerFilterTouched boolean/sentinel) and initialize ownerFilter correctly when creating filterStateAtom so the hook is idempotent; update any logic referencing ownerFilter to check the new touched/sentinel flag rather than treating OWNER_ALL as the untouched state.
🤖 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/similarity-search/src/hooks/useFilteredRuns.ts`:
- Around line 15-16: The atom filterStateAtom currently initializes ownerFilter
to OWNER_ALL but then mutates shared state on mount (in useFilteredRuns) which
flips an intentional "All" back to "Mine" on remount; instead make
initialization synchronous and per-dataset/user or add an explicit untouched
sentinel so OWNER_ALL is not used as both default and a user choice. Concretely:
stop mutating filterStateAtom inside the mount effect in useFilteredRuns, change
makeDefaultFilterState (or RunFilterState) to accept datasetId/userId (or
include an ownerFilterTouched boolean/sentinel) and initialize ownerFilter
correctly when creating filterStateAtom so the hook is idempotent; update any
logic referencing ownerFilter to check the new touched/sentinel flag rather than
treating OWNER_ALL as the untouched state.
In `@app/packages/similarity-search/src/hooks/useRuns.ts`:
- Around line 61-67: The current refreshRuns implementation returns the original
inFlightRef.current when a refresh is queued (fetchingRef, pendingRef,
inFlightRef), causing queued callers to await the first fetch instead of the
drained follow-up; change the queued-path so refreshRuns creates and returns a
new promise that resolves/rejects when the drained refresh completes (store that
promise in a new queuedRef or attach a chained promise to inFlightRef.current)
and ensure pendingRef.current triggers the follow-up fetch to settle that queued
promise; apply the same fix to the other queued-return branch that uses
fetchingRef/pendingRef/inFlightRef so callers awaiting refreshRuns always get
the promise representing the actual work they will observe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5dc658f4-3992-424a-9d43-24485674ef2d
📒 Files selected for processing (4)
app/packages/similarity-search/src/components/Home/RunActions.tsxapp/packages/similarity-search/src/constants.tsapp/packages/similarity-search/src/hooks/useFilteredRuns.tsapp/packages/similarity-search/src/hooks/useRuns.ts
mozabes
reviewed
Apr 13, 2026
Contributor
|
Please try and keep PR's smaller. This looks like it should be at least 7 Pr's imo. |
imanjra
approved these changes
Apr 17, 2026
imanjra
left a comment
imanjra
left a comment
Contributor
There was a problem hiding this comment.
Left some comments. Nothing too concerning so approved
…uct run name to fiftyone/utilities format.ts and other misc fixes
lanzhenw
force-pushed
the
bugfix/ssp-current-view
branch
from
0db9ae9 to
32ef80a
Compare
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
fixes the following issues:
New improvement:
📋 What changes are proposed in this pull request?
New Features
Improvements
selected state:
https://github.com/user-attachments/assets/07f1f1ad-b1c7-48b5-bef1-d292b315f6c5
🧪 How is this patch tested? If it is not, please explain why.
Screen.Recording.2026-04-13.at.10.22.43.AM.mov
A user can trigger a similarity sort run from the pop over or from within the panel,
under both immediate execution and delegated operation,
in the run list page, if a run completes, automatically load the results for that run. Meanwhile, the run card will be highlighted.
Clicking on the run card will load the results for that run (if completed).
📝 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