Support alternative selecting samples#7109
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces per-sample selection type tracking and configurable selection styles throughout the application. The Sequence DiagramsequenceDiagram
participant UI as User/UI
participant State as State<br/>(Recoil)
participant Mutation as Relay<br/>Mutation
participant Backend as Backend<br/>Server
participant Render as Rendering<br/>Engine
UI->>State: Select sample<br/>(with modifier)
State->>State: Update selectedSamples<br/>Map[id→type]
State->>Render: Notify selection<br/>changed
Render->>State: Read selectedSamples<br/>& sampleSelectionStyle
Render->>Render: Resolve selection icon<br/>based on type + style
Render->>UI: Apply icon class<br/>to checkbox
UI->>UI: Trigger mutation<br/>(on style change)
UI->>Mutation: setSelectedSamples<br/>or setSampleSelectionStyle
Mutation->>Backend: Send samples array<br/>or style JSON
Backend->>Backend: Update session state<br/>& dispatch event
Backend->>State: Event dispatch<br/>(SelectSamples/<br/>SetSampleSelectionStyle)
State->>State: Sync selectedSamples<br/>& sampleSelectionStyle
State->>Render: Trigger re-render
Render->>UI: Update display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 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 |
lanzhenw
changed the title
Member
Author
|
@coderabbitai summary |
Contributor
✅ Actions performedSummary regeneration triggered. |
lanzhenw
requested review from
benjaminpkane,
brimoor,
imanjra,
ritch and
swheaton
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/packages/operators/src/operators.ts (1)
754-780:⚠️ Potential issue | 🟠 MajorMissing
selected_metaandselection_stylein non-generator execution path.The non-generator remote execution path (lines 754-780) does not include
selected_metaandselection_style, while the generator path (line 600-601) and all resolution paths do. This inconsistency means operators executed withoutexecuteAsGeneratorwon't receive selection metadata.🐛 Proposed fix
const serverResult = await getFetchFunction()( "POST", "/operators/execute", { current_sample: currentContext.currentSample, dataset_name: currentContext.datasetName, delegation_target: currentContext.delegationTarget, extended: currentContext.extended, extended_selection: currentContext.extendedSelection, filters: currentContext.filters, operator_uri: operatorURI, params: ctx.params, request_delegation: ctx.requestDelegation, selected: currentContext.selectedSamples ? Array.from(currentContext.selectedSamples) : [], + selected_meta: currentContext.selectedMeta || {}, + selection_style: currentContext.selectionStyle || {}, selected_labels: formatSelectedLabels(currentContext.selectedLabels), view: currentContext.view, view_name: currentContext.viewName, group_slice: currentContext.groupSlice, query_performance: currentContext.queryPerformance, spaces: currentContext.spaces, workspace_name: currentContext.workspaceName, prompt_id: ctx.promptId, active_fields: ctx.activeFields, } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/operators/src/operators.ts` around lines 754 - 780, The non-generator remote execution payload is missing selection metadata fields; update the POST body created in the getFetchFunction() call (the serverResult assignment) to include selected_meta and selection_style so it matches the generator/resolution paths — e.g. add selected_meta: formatSelectedMeta ? formatSelectedMeta(currentContext.selectedMeta) : currentContext.selectedMeta and selection_style: ctx.selectionStyle (or the exact symbols used elsewhere) alongside the existing selected/selected_labels fields so operators receive selection metadata in the non-generator path.
🤖 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/app/src/useWriters/onSelectSamples.ts`:
- Around line 16-20: selectedMeta is set to undefined when rawMeta is falsy but
SESSION_DEFAULT.selectedMeta is an empty object; change the fallback so
selectedMeta becomes {} instead of undefined to keep behavior consistent with
SESSION_DEFAULT. Locate the assignment to selectedMeta (the ternary using
rawMeta) in onSelectSamples.ts and replace the falsy branch with an empty object
so selectedMeta is either the filtered Object.fromEntries(...) or {}.
In `@app/packages/core/src/components/Grid/useSelectSample.ts`:
- Around line 127-130: The shift-click path can call addRange(index, current,
records) when there is no existing record-backed anchor, causing a crash; modify
the branch handling shiftKey in useSelectSample.ts to first check whether there
is a valid anchor (e.g., current has any IDs that exist in records or
current.size > 0) and only call addRange when an anchor exists, otherwise fall
back to the single-item selection/toggle behavior for sampleId (use the same
logic you use for non-shift clicks), referencing variables sampleId, current,
records and function addRange/newSelected to locate the code to change.
In `@app/packages/operators/src/built-in-operators.ts`:
- Around line 498-505: SetSelectionStyle.execute accesses params without
checking for undefined which can crash; update the method to guard params (e.g.,
treat params = params || {} or use optional chaining/default destructuring)
before reading params.default and params.alt, then compute style.default =
params.default ?? "checkmark" and style.alt = params.alt ?? null (or use
params?.alt), and set fos.altSelectionMode using Boolean(params?.alt); adjust
the state.set calls in execute to use these guarded values.
- Around line 472-485: The code sets hooks.setSelected and then writes meta
directly, which can persist IDs not in samples; before calling hooks.setMeta
normalize the meta object to only include keys present in the samples array (and
for any missing sample IDs add default entries { type: "default" }), i.e.
compute a newMeta from params.meta filtered by samples (or build defaultMeta
when params.meta is falsy) and pass newMeta to hooks.setMeta; update references
near params/samples/meta and the hooks.setMeta call.
In `@app/packages/state/src/recoil/atoms.ts`:
- Around line 216-219: Replace the independent atom altSelectionMode with a
derived value from the existing selectionStyle so there is a single source of
truth: either convert altSelectionMode into a selector that reads selectionStyle
(e.g., returns selectionStyle.alt) or add a central synchronization effect where
setSelectionStyle updates altSelectionMode; locate altSelectionMode and
selectionStyle and implement the selector-or-sync approach so restored/hydrated
selectionStyle.alt always determines altSelectionMode and prevents state drift.
In `@app/packages/state/src/session.ts`:
- Around line 92-94: Change the loose string typing for selection icons to a
strict union that matches the known set in ICON_CLASS_MAP: define a
SelectionIconStyle union type (e.g., "checkmark" | "green-checkmark" |
"red-checkmark" | "thumbsup" | "thumbsdown" | "pin" | "star" | "x" | "bookmark")
and update the Session interface's selectionStyle property to use { default:
SelectionIconStyle; alt?: SelectionIconStyle | null } so selectionStyle.default
and selectionStyle.alt are compile-time checked against the valid icon names
referenced by ICON_CLASS_MAP in thumbnail.ts.
In `@fiftyone/core/session/session.py`:
- Around line 1249-1254: Replace the lambda assigned to on_set_selection_style
with a top-level or enclosing named function to avoid Ruff E731: define a
function (e.g., def on_set_selection_style(event: SetSelectionStyle) -> None:)
that calls setattr(session._state, "selection_style", event.style), then pass
that function to session._client.add_event_listener("set_selection_style",
on_set_selection_style); keep the same type handling/annotation for
SetSelectionStyle and same behavior.
- Around line 835-839: The selected_meta setter is bypassing _resolve_meta which
allows invalid sample IDs/types to be stored and emitted; update the setter
(selected_meta) to call self._resolve_meta(meta or {}) to validate/normalize the
meta, assign the resolved result to self._state.selected_meta, and pass that
resolved meta into self._client.send_event(SelectSamples(self._state.selected,
meta=resolved_meta)) so selection invariants match select_samples and selection
events use the normalized data.
In `@fiftyone/core/state.py`:
- Around line 91-92: The StateDescription deserialization in from_dict is
dropping the new fields selected_meta and selection_style; update from_dict to
extract these keys from the input dict and pass them into the
StateDescription(...) constructor (alongside the existing fields) so
selected_meta and selection_style are restored on load; ensure you use the same
key names expected in the dict and preserve any existing default behavior if the
keys are missing.
In `@fiftyone/operators/executor.py`:
- Around line 844-850: selection_style currently returns {} when missing which
breaks callers expecting a "default" key; change the getter (method
selection_style) to read the value from self.request_params and ensure it always
returns a dict with at least a "default" key (e.g. merge/normalize the value:
style = self.request_params.get("selection_style") or {}; if "default" not in
style: style = {"default": {}} or similarly merge defaults) so callers can
safely use ctx.selection_style["default"].
In `@fiftyone/operators/operations.py`:
- Around line 767-777: The _validate_meta function assumes meta has .keys() and
.items(); first validate that meta is a mapping (e.g., isinstance(meta,
collections.abc.Mapping)) and if not raise a clear TypeError (or ValueError)
like "meta must be a mapping of sample_id -> meta_entry"; only after that
perform the existing extra_keys check and iterate meta.items() and validate
entry["type"] against _VALID_SELECTION_TYPES. This ensures callers receive a
clear validation error instead of an AttributeError.
In `@tests/unittests/session_events_tests.py`:
- Around line 59-125: These tests directly mutate StateDescription fields
instead of exercising the selection code paths; update them to call the session
APIs (e.g., use Session.select_samples(...) or dispatch the SelectSamples event
/ call its resolve helper) and apply SetSelectionStyle via the real handler (or
its resolve) rather than assigning fields directly so validation/formatting
logic runs; specifically, replace direct assignments to state.selected,
state.selected_meta, and state.selection_style in the
test_select_samples_with_meta, test_select_samples_without_meta_clears_stale,
test_set_selection_style, and test_clear_selection_style cases with calls to
Session.select_samples / SelectSamples.resolve and SetSelectionStyle.resolve (or
the session wrapper) to verify real behavior and backwards-compat handling.
---
Outside diff comments:
In `@app/packages/operators/src/operators.ts`:
- Around line 754-780: The non-generator remote execution payload is missing
selection metadata fields; update the POST body created in the
getFetchFunction() call (the serverResult assignment) to include selected_meta
and selection_style so it matches the generator/resolution paths — e.g. add
selected_meta: formatSelectedMeta ?
formatSelectedMeta(currentContext.selectedMeta) : currentContext.selectedMeta
and selection_style: ctx.selectionStyle (or the exact symbols used elsewhere)
alongside the existing selected/selected_labels fields so operators receive
selection metadata in the non-generator path.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9c5079ee-dbd4-42c7-8dcb-154a84754bc4
⛔ Files ignored due to path filters (2)
app/packages/relay/src/mutations/__generated__/setSelectedMutation.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/mutations/__generated__/setSelectionStyleMutation.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**
📒 Files selected for processing (32)
app/packages/app/src/useEvents/index.tsapp/packages/app/src/useEvents/useSelectSamples.tsapp/packages/app/src/useEvents/useSetSelectionStyle.tsapp/packages/app/src/useWriters/index.tsapp/packages/app/src/useWriters/onSelectMeta.tsapp/packages/app/src/useWriters/onSelectSamples.tsapp/packages/app/src/useWriters/onSetSelectionStyle.tsapp/packages/core/src/components/Grid/useSelectSample.tsapp/packages/core/src/components/Grid/useUpdates.tsapp/packages/looker/src/elements/common/thumbnail.tsapp/packages/looker/src/elements/common/util.module.cssapp/packages/looker/src/lookers/abstract.tsapp/packages/looker/src/state.tsapp/packages/operators/src/built-in-operators.tsapp/packages/operators/src/hooks.tsapp/packages/operators/src/operators.tsapp/packages/operators/src/state.tsapp/packages/relay/src/mutations/index.tsapp/packages/relay/src/mutations/setSelected.tsapp/packages/relay/src/mutations/setSelectionStyle.tsapp/packages/state/src/hooks/useCreateLooker.tsapp/packages/state/src/recoil/atoms.tsapp/packages/state/src/recoil/types.tsapp/packages/state/src/session.tsapp/schema.graphqlfiftyone/core/session/events.pyfiftyone/core/session/session.pyfiftyone/core/state.pyfiftyone/operators/executor.pyfiftyone/operators/operations.pyfiftyone/server/mutation.pytests/unittests/session_events_tests.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
fiftyone/operators/executor.py (1)
835-852:⚠️ Potential issue | 🟠 MajorNormalize nullable request values to stable dict contracts.
These getters return
Nonewhen request params contain explicit nulls, despite docstrings promising dicts. Useor {}and enforce a"default"key forselection_style.Proposed fix
def selected_meta(self): @@ - return self.request_params.get("selected_meta", {}) + return self.request_params.get("selected_meta") or {} `@property` def selection_style(self): @@ - return self.request_params.get( - "selection_style", {"default": "checkmark"} - ) + style = self.request_params.get("selection_style") or {} + if "default" not in style: + style = {"default": "checkmark", **style} + return style#!/bin/bash set -euo pipefail # Check callers that assume dict-shaped selection style/meta rg -n -C2 'ctx\.selection_style|selection_style\["default"\]|selected_meta\.items\(' fiftyone --type=py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiftyone/operators/executor.py` around lines 835 - 852, The getters selected_meta and selection_style can return None if request_params explicitly contains null; change them to normalize to stable dict contracts by using self.request_params.get("selected_meta") or {} for selected_meta, and for selection_style use (self.request_params.get("selection_style") or {}) and ensure a "default" key exists (e.g., set to "checkmark" when missing) so callers can safely index ["default"] and iterate .items(); update these two properties accordingly to always return dicts with selection_style containing at least a "default" entry.tests/unittests/session_events_tests.py (1)
78-109: 🧹 Nitpick | 🔵 TrivialUse Session APIs in style tests instead of direct state assignment.
These cases bypass the real session methods and listeners, so regressions in event emission/synchronization won’t be caught.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unittests/session_events_tests.py` around lines 78 - 109, Tests are directly mutating StateDescription (state.selection_style and state.selected_meta) instead of using the session event APIs, so they bypass event emission and listeners; update both test_set_selection_style and test_clear_selection_style to create/obtain a Session instance and apply the SetSelectionStyle event via the session API (e.g., call the session method that handles incoming events or dispatches SetSelectionStyle, rather than assigning state.selection_style = event.style), and when updating selected_meta in test_clear_selection_style ensure you trigger the same session-level update path (dispatch the appropriate selection-meta update event or use the session's selection update method) so listeners and synchronization logic run; keep references to StateDescription, SetSelectionStyle, state.selection_style and state.selected_meta to locate the code to change.app/packages/operators/src/built-in-operators.ts (1)
477-483:⚠️ Potential issue | 🟠 MajorSanitize
metavalues to allowed selection types before storing.
meta[id]is copied directly, so invalid entries (for example{ type: "invalid" }) bypass the selection-type contract and can desynchronize app/backend behavior.Proposed fix
- const normalizedMeta: Record<string, { type: string }> = {}; + const normalizedMeta: Record<string, { type: "default" | "alt" }> = {}; for (const id of samples) { - normalizedMeta[id] = meta[id] || { type: "default" }; + normalizedMeta[id] = { + type: meta?.[id]?.type === "alt" ? "alt" : "default", + }; } hooks.setMeta(normalizedMeta);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/operators/src/built-in-operators.ts` around lines 477 - 483, The code copies meta[id] blindly into normalizedMeta before calling hooks.setMeta, allowing invalid selection types to propagate; update the block that builds normalizedMeta (used with samples and passed to hooks.setMeta) to validate meta[id].type against a whitelist of allowed selection types (e.g., "default", "text", "number", "choice" — whatever the app supports) and only copy the entry if the type is allowed, otherwise replace it with a safe fallback like { type: "default" }; ensure you preserve other allowed fields if present but always enforce the type normalization before calling hooks.setMeta.app/packages/core/src/components/Grid/useSelectSample.ts (1)
127-141:⚠️ Potential issue | 🔴 CriticalUse a record-backed anchor check before calling
addRange().
Line 129only checkscurrent.size === 0. If selected IDs exist but none are present inrecords,addRange()receives an empty anchor set and crashes inargMinreduce. Gate range-add on an actualrecordsanchor and fall back to single selection otherwise.Proposed fix
- if (shiftKey && !current.has(sampleId)) { + const hasRangeAnchor = [...current].some((id) => records.has(id)); + if (shiftKey && !current.has(sampleId) && hasRangeAnchor) { // Shift-click (or shift+alt-click) range add - if (current.size === 0) { - // No anchor — treat as normal click - current.add(sampleId); - currentObjects.set(sampleId, sample); - currentMeta[sampleId] = { type: selectionType }; - set(selectedSamples, current); - set(selectedSampleObjects, currentObjects); - set(selectedMeta, currentMeta); - setSelected(new Set(current)); - return; - } const newSelected = addRange(index, current, records); for (const id of newSelected) { if (!current.has(id)) { currentMeta[id] = { type: selectionType }; } } for (const id of newSelected) { current.add(id); } currentObjects.set(sampleId, sample); + } else if (shiftKey && !current.has(sampleId)) { + // No record-backed anchor — treat as normal click + current.add(sampleId); + currentObjects.set(sampleId, sample); + currentMeta[sampleId] = { type: selectionType }; } else if (shiftKey) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/core/src/components/Grid/useSelectSample.ts` around lines 127 - 141, The shift-range logic in useSelectSample checks only current.size === 0 before calling addRange(index, current, records), which can pass an anchor set with IDs not present in records and cause addRange (argMin) to crash; update the check to verify there is at least one anchor ID from current that exists in records (e.g., find any id in current where records contains it) and only call addRange when such a record-backed anchor exists; if no valid anchor is found, fall back to the single-selection branch (add sampleId to current, set currentObjects/currentMeta, call set(selectedSamples)/set(selectedSampleObjects)/set(selectedMeta)/setSelected) instead of calling addRange.
🤖 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/app/src/useEvents/useSetSelectionStyle.ts`:
- Around line 10-12: The handler in useSetSelectionStyle.ts must add a proper
type for the incoming payload and guard the parsed value before accessing
.style: update the anonymous parameter to a typed payload (matching the shape
used in useSetSample), wrap JSON.parse in a try/catch and validate that the
result is an object, then use optional chaining (e.g. parsedPayload?.style) and
fall back to the default ({ default: "checkmark" }) when undefined; keep
setStyle(style) as the final call and mirror the validation/typing pattern used
in useSetSample to locate the correct shape.
In `@app/packages/operators/src/built-in-operators.ts`:
- Around line 505-509: The selectionStyle object currently always includes alt
set to null which breaks the expected shape; update the construction that uses
params / defaultIcon / alt (the style or selectionStyle variable) so it always
sets default: defaultIcon || "checkmark" but only adds the alt property when alt
is provided (e.g., check alt !== undefined or truthiness) so unset alt is
omitted rather than set to null.
In `@fiftyone/core/session/session.py`:
- Around line 868-878: clear_selection_style currently updates
self._state.selected_meta to normalize alt meta but only emits
SetSelectionStyle, which can leave app state with stale meta; after setting
self._state.selected_meta = {k: {"type":"default"} ...} also emit a
SelectSamples event carrying the current selected ids and the normalized meta
(use self._state.selected for sample ids and the updated
self._state.selected_meta for meta) in addition to sending
SetSelectionStyle(style=style), so both SetSelectionStyle and SelectSamples are
sent from clear_selection_style to fully propagate the normalization to the
client.
In `@fiftyone/core/state.py`:
- Around line 91-92: Initialize persisted state fields with non-null defaults:
replace any assignments that set self.selected_meta or self.selection_style to
None with concrete defaults (e.g., self.selected_meta = {} and
self.selection_style = "" or another sensible default value for selection
style). Update both the constructor/initialization location where selected_meta
and selection_style are first set and the other occurrence where they are
reassigned (the second block assigning self.selected_meta/self.selection_style)
so persisted state never stores None across layers.
---
Duplicate comments:
In `@app/packages/core/src/components/Grid/useSelectSample.ts`:
- Around line 127-141: The shift-range logic in useSelectSample checks only
current.size === 0 before calling addRange(index, current, records), which can
pass an anchor set with IDs not present in records and cause addRange (argMin)
to crash; update the check to verify there is at least one anchor ID from
current that exists in records (e.g., find any id in current where records
contains it) and only call addRange when such a record-backed anchor exists; if
no valid anchor is found, fall back to the single-selection branch (add sampleId
to current, set currentObjects/currentMeta, call
set(selectedSamples)/set(selectedSampleObjects)/set(selectedMeta)/setSelected)
instead of calling addRange.
In `@app/packages/operators/src/built-in-operators.ts`:
- Around line 477-483: The code copies meta[id] blindly into normalizedMeta
before calling hooks.setMeta, allowing invalid selection types to propagate;
update the block that builds normalizedMeta (used with samples and passed to
hooks.setMeta) to validate meta[id].type against a whitelist of allowed
selection types (e.g., "default", "text", "number", "choice" — whatever the app
supports) and only copy the entry if the type is allowed, otherwise replace it
with a safe fallback like { type: "default" }; ensure you preserve other allowed
fields if present but always enforce the type normalization before calling
hooks.setMeta.
In `@fiftyone/operators/executor.py`:
- Around line 835-852: The getters selected_meta and selection_style can return
None if request_params explicitly contains null; change them to normalize to
stable dict contracts by using self.request_params.get("selected_meta") or {}
for selected_meta, and for selection_style use
(self.request_params.get("selection_style") or {}) and ensure a "default" key
exists (e.g., set to "checkmark" when missing) so callers can safely index
["default"] and iterate .items(); update these two properties accordingly to
always return dicts with selection_style containing at least a "default" entry.
In `@tests/unittests/session_events_tests.py`:
- Around line 78-109: Tests are directly mutating StateDescription
(state.selection_style and state.selected_meta) instead of using the session
event APIs, so they bypass event emission and listeners; update both
test_set_selection_style and test_clear_selection_style to create/obtain a
Session instance and apply the SetSelectionStyle event via the session API
(e.g., call the session method that handles incoming events or dispatches
SetSelectionStyle, rather than assigning state.selection_style = event.style),
and when updating selected_meta in test_clear_selection_style ensure you trigger
the same session-level update path (dispatch the appropriate selection-meta
update event or use the session's selection update method) so listeners and
synchronization logic run; keep references to StateDescription,
SetSelectionStyle, state.selection_style and state.selected_meta to locate the
code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae744022-1704-4bf3-91a3-d8a4bb4e2fdf
📒 Files selected for processing (11)
app/packages/app/src/useEvents/useSetSelectionStyle.tsapp/packages/app/src/useWriters/onSelectSamples.tsapp/packages/core/src/components/Grid/useSelectSample.tsapp/packages/operators/src/built-in-operators.tsapp/packages/state/src/recoil/atoms.tsapp/packages/state/src/session.tsfiftyone/core/session/session.pyfiftyone/core/state.pyfiftyone/operators/executor.pyfiftyone/operators/operations.pytests/unittests/session_events_tests.py
Member
Author
|
@coderabbitai pause |
Contributor
✅ Actions performedReviews paused. |
7 tasks
ritch
reviewed
Mar 5, 2026
| confirm("Are you sure you want to clear your current selection?") | ||
| ) { | ||
| reset(fos.selectedSamples); | ||
| reset(fos.selectedMeta); |
Contributor
There was a problem hiding this comment.
This is highlighting a code smell we have. It leaks the coupling and implementation details of selection. Instead we should use the domain store pattern that is used elsewhere.
The rest of the app should only know about this domain store and internally it just happens to use recoil.
export type SelectionStore = {
// state
selectedSamples: string[];
selectedMeta: SelectionMeta;
hasSelection: boolean;
// actions
_setSelectedSamples: (ids: string[]) => void;
_setSelectedMeta: (meta: SelectionMeta) => void;
clear: () => void; // clears both
// convenience actions
addSample: (id: string, meta: SelectionMetaItem) => void;
removeSample: (id: string) => void;
};The concept is covered pretty well in this article.
Member
Author
There was a problem hiding this comment.
In the new proposed syntax (see below), we would remove the concept of samples and labels meta and thus condense the source of truth to one session atom.
# Selected sample IDs, regardless of selection type
ctx.selected = [list, of, all, ids]
# NEW PROPERTY shows selected samples with 'type' key
ctx.selected_samples = [
{
"sample_id": "XXX",
"type": "default" or "alt",
},
...
]
ctx.selected_samples = [
{
"sample_id": "XXX",
"type": "default" or "alt",
},
...
]
# NEW ATTRIBUTE: 'type' key shows selection type for selected labels
ctx.selected_labels = [
{
"sample_id": "XXX",
"label_id": "YYY",
"instance_id": "ZZZ" or None,
"field": "ground_truth",
"frame_number": 1 or None,
"type": "default" or "alt", # NEW KEY
},
...
]
# NEW SYNTAX: select samples with custom selection types
ctx.ops.set_selected_samples(
[
{
"sample_id": "XXX",
"type": "default" or "alt",
},
...
]
)
# NEW SYNTAX: select labels with custom selection types
ctx.ops.set_selected_labels(
[
{
"sample_id": "XXX",
"label_id": "YYY",
"instance_id": "ZZZ" or None, # optional
"field": "ground_truth",
"frame_number": 1, # frame labels only
"type": "default" or "alt", # NEW OPTIONAL KEY
},
...
],
)
721fa5b to
d2f2da9
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
benjaminpkane
left a comment
benjaminpkane
left a comment
There was a problem hiding this comment.
Very comprehensive work. The comments are minor. Just two questions. Otherwise LGTM
Should we add a version of the summary when two different selections are in use? The below was printed by my session's repr when 1 alt and 1 default selection were chosen
Dataset: cifar10
Media type: image
Num samples: 60000
Selected samples: 2
Selected labels: 0
Session type: remote
Should we remove the use of selected from StateDescription or are we worried about backward compatibility?
swheaton
approved these changes
Mar 12, 2026
Contributor
swheaton
left a comment
swheaton
left a comment
There was a problem hiding this comment.
Thanks for adding the tests!
I think Ben may have some open comments but it's good on the backend (minimal changes)
Co-authored-by: Benjamin Kane <ben@tapes.co>
Co-authored-by: Benjamin Kane <ben@tapes.co>
Co-authored-by: Benjamin Kane <ben@tapes.co>
Co-authored-by: Benjamin Kane <ben@tapes.co>
…different from default
…e_selection_style setter
…pdate sample_id to id
# Conflicts: # app/packages/core/src/components/Grid/useSelectSample.ts # app/packages/looker/src/elements/common/thumbnail.ts # app/packages/looker/src/lookers/abstract.ts # fiftyone/server/events/initialize.py
benjaminpkane
approved these changes
Apr 3, 2026
Member
benjaminpkane
left a comment
benjaminpkane
left a comment
There was a problem hiding this comment.
Very thorough tests! Feature looks great! Adding groups carousel support too 🚀
My notes for potential future work:
- The checkbox icon the modal remains fixed. I had thumbs up/down configured (see screenshot)
./e2e-pwPOM updates to support new selection modes. Maybe a smoke test too?
Member
Author
fixed the sample selection. |
This was referenced Apr 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
What changes are proposed in this pull request?
This PR adds a second way of selecting samples on the samples grid and provided a list of UI icons for style customization. Why? Python panels sometimes need more than one way to select samples on the grid. Yet, currently it is not possible in Fiftyone. This PR adds a configurable alternative way of selection on samples grid, so that users can have better UX with grid selection in few shots, similarity search panels and the future VAL panel.
Summary by CodeRabbit
Release Notes
New Features
There will be a follow-up PR that add alternative selection to labels on samples grid and sample modal in the same pattern.
How is this patch tested? If it is not, please explain why.
How to test it?
in python SDK
To test alt-click in App
load a dataset with samples, then alt click, you should see normal checkbox
Turn on alt-click by setting the sample selections tyle:
click a sample -> thumbsup icon
alt+click a sample -> thumbsdown icon
shift+click -> range select
shift+alt+click -> range alt select
go to a group dataset, set sample selection style, check carousel thumbnail show the correct icons
To test the changes in execution context
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
adds a configurable alternative way of selection on samples grid, so that users can have better UX with grid selection in few shots, similarity search panels and the future VAL panel.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests