Support selecting label in with alt click and styles#7334
Merged
Conversation
…ion, operators Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-in operators Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…, keypoint, segmentation, heatmap, classification Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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:
WalkthroughAdds end-to-end label-selection styling: new style names and defaults; session/state types, atoms, selectors, and persisted session field Sequence Diagram(s)sequenceDiagram
participant User
participant Canvas as Looker Canvas
participant App as App Event Handler
participant Recoil as Recoil Session/Selectors
participant Relay as Relay Client
participant Server as Backend
User->>Canvas: Click/Select label (Alt/Shift state)
Canvas->>App: dispatch SelectEvent (isAltPressed, isShiftPressed)
App->>Recoil: update selectedLabels (include type "default"/"alt")
Recoil->>Canvas: provide options (selectedLabels, selectedLabelTypes, labelSelectionStyle)
Canvas->>Canvas: resolveLabelSelectionVisuals -> render overlays with selection style/color
App->>Relay: commitMutation(setLabelSelectionStyle, style)
Relay->>Server: GraphQL mutation set_label_selection_style
Server->>Server: dispatch SetLabelSelectionStyle -> update server state
Server->>App: broadcast updated state
App->>Recoil: update session.labelSelectionStyle
Recoil->>Canvas: re-render overlays with new visuals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/schema.graphql (1)
782-789:⚠️ Potential issue | 🟠 MajorConstrain
SelectedLabel.typeto an enum, not free-form string.
type: Stringallows arbitrary values, but this field is semantically constrained to selection variants (default/alt). Keeping it asStringpermits invalid values into API/state flow.Suggested schema hardening
+enum SelectionType { + default + alt +} + input SelectedLabel { labelId: ID! field: String! sampleId: ID! frameNumber: Int = null instanceId: ID = null - type: String = null + type: SelectionType = null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/schema.graphql` around lines 782 - 789, Change the free-form string "type" on the SelectedLabel input to a GraphQL enum to restrict allowed values: add a new enum (e.g., SelectionVariant or SelectedLabelType) with the permitted members (DEFAULT and ALT) and update the SelectedLabel.input's type field from "type: String = null" to use that enum (keeping nullable/default behavior if needed). Ensure any usages of SelectedLabel in queries/mutations are updated to accept the enum type.
🤖 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/useSetLabelSelectionStyle.ts`:
- Around line 11-14: The callback in useSetLabelSelectionStyle currently
accesses payload.style unsafely and doesn't normalize partial style objects;
update the callback in useSetLabelSelectionStyle to guard for undefined payload
(e.g., default payload = {}), derive style by merging defaults with any provided
partial (use DEFAULT_LABEL_SELECTION_STYLE merged with payload.style), and pass
the resulting normalized style to setter("labelSelectionStyle", style) so
session state always gets a complete, safe style object.
In `@app/packages/app/src/useEvents/utils.ts`:
- Around line 99-107: The resolveLabelSelectionStyle function currently returns
state.label_selection_style directly which allows partial objects from the
server to omit required keys; instead merge the hydrated object onto
DEFAULT_LABEL_SELECTION_STYLE so missing keys like default/alt are filled.
Update resolveLabelSelectionStyle to, when not env().VITE_NO_STATE, return a
merged object (DEFAULT_LABEL_SELECTION_STYLE spread first, then
state.label_selection_style) or use a shallow merge utility so
LabelSelectionStyle always contains the default properties; keep the
VITE_NO_STATE branch returning DEFAULT_LABEL_SELECTION_STYLE unchanged.
In `@app/packages/looker/src/state.ts`:
- Around line 188-190: Tighten the BaseOptions type by replacing the
overly-broad selectedLabelTypes and labelSelectionStyle declarations: change
selectedLabelTypes from Record<string, string> to a mapped type keyed by the
supported label type union (e.g., Record<SupportedLabelType,
SupportedLabelMode>) and change labelSelectionStyle from { default: string; alt:
string } to an object whose properties use a constrained SelectionStyle union
(e.g., { default: SelectionStyle; alt: SelectionStyle }), keeping
selectedLabelTags?: string[] as-is; update or add the corresponding union types
(SupportedLabelType, SupportedLabelMode, SelectionStyle) and adjust any
imports/exports that reference BaseOptions so type-checking enforces only the
allowed values for selectedLabelTypes and labelSelectionStyle.
In `@app/packages/state/src/recoil/selectors.ts`:
- Around line 294-305: In selectedLabelTypes selector, remove the unsafe
double-cast and instead import and use the SelectionType union to narrow
label.type: iterate over get(atoms.selectedLabels) and for each label check
whether (label as any).type is one of SelectionType values (e.g., if ((label as
any).type === "alt") use "alt" else use "default"); replace the casted
expression with this explicit check and return a string typed as SelectionType
(or "default") for types[label.labelId]; ensure to import SelectionType from
types.ts and avoid casting to Record<string, unknown>.
In `@app/packages/state/src/recoil/types.ts`:
- Line 231: SelectedLabelData currently allows any string for its type field
which lets invalid values slip through; change the SelectedLabelData.type
declaration from `string` to the enum/union `SelectionType` (update the import
or local reference to SelectionType if missing) so TypeScript enforces only
valid selection kinds and keeps client/server payloads consistent.
In `@fiftyone/core/session/events.py`:
- Line 98: The LabelData type annotation and default should be changed so it
cannot be None: update the LabelData declaration (symbol: LabelData.type) to use
a concrete default string (e.g., "default") and a non-optional type (str)
instead of t.Optional[str] = None so serialized payloads never produce None and
state.selected_labels remains normalized; adjust any constructor/serialization
logic in the same module (events.py) that relies on Optional to accept/emit the
concrete default.
In `@fiftyone/core/session/session.py`:
- Around line 1490-1506: The _normalize_selected_labels function currently lets
non-dict labels pass through, which is inconsistent with
normalize_selected_samples and risky; update _normalize_selected_labels to
validate inputs the same way: if a label is a dict, enforce
label.setdefault("type","default") and validate type against
VALID_SELECTION_TYPES (as already implemented), otherwise raise a TypeError (or
ValueError) rejecting non-dict entries with a clear message (e.g., "selected
label must be a dict with at least 'label_id' and optional 'type'"); update
callers/selectors such as select_labels and _get_selected_labels tests to assert
this behavior and add a unit test that passes a non-dict value to
_normalize_selected_labels to ensure it raises.
In `@fiftyone/server/mutation.py`:
- Around line 236-243: The code currently corrects only the 'default'/'alt'
values but forwards the entire client-supplied style dict; instead, create a new
sanitized dict containing only the allowed keys (e.g., "default" and "alt"),
populate it from the incoming style (or DEFAULT_LABEL_SELECTION_STYLE when
absent), validate each value against VALID_LABEL_SELECTION_STYLES and fall back
to DEFAULT_LABEL_SELECTION_STYLE where invalid, and then pass that sanitized
dict to dispatch_event (replace the current use of `style` in the call to
fose.SetLabelSelectionStyle). Reference the existing symbols
VALID_LABEL_SELECTION_STYLES, DEFAULT_LABEL_SELECTION_STYLE and
fose.SetLabelSelectionStyle to locate where to implement the change.
---
Outside diff comments:
In `@app/schema.graphql`:
- Around line 782-789: Change the free-form string "type" on the SelectedLabel
input to a GraphQL enum to restrict allowed values: add a new enum (e.g.,
SelectionVariant or SelectedLabelType) with the permitted members (DEFAULT and
ALT) and update the SelectedLabel.input's type field from "type: String = null"
to use that enum (keeping nullable/default behavior if needed). Ensure any
usages of SelectedLabel in queries/mutations are updated to accept the enum
type.
🪄 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: 41dd3108-cc83-4ad3-b10f-06e3b63602dd
⛔ Files ignored due to path filters (6)
app/packages/relay/src/mutations/__generated__/createSavedViewMutation.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/mutations/__generated__/setLabelSelectionStyleMutation.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/mutations/__generated__/setSelectedLabelsMutation.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/mutations/__generated__/setViewMutation.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/queries/__generated__/aggregationsQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (35)
app/packages/app/src/useEvents/index.tsapp/packages/app/src/useEvents/useSetLabelSelectionStyle.tsapp/packages/app/src/useEvents/utils.tsapp/packages/app/src/useWriters/index.tsapp/packages/app/src/useWriters/onSetLabelSelectionStyle.tsapp/packages/looker/src/elements/common/canvas.tsapp/packages/looker/src/overlays/classifications.tsapp/packages/looker/src/overlays/detection.tsapp/packages/looker/src/overlays/heatmap.tsapp/packages/looker/src/overlays/keypoint.tsapp/packages/looker/src/overlays/polyline.tsapp/packages/looker/src/overlays/segmentation.tsapp/packages/looker/src/overlays/util.tsapp/packages/looker/src/state.tsapp/packages/operators/src/built-in-operators.tsapp/packages/relay/src/mutations/index.tsapp/packages/relay/src/mutations/setLabelSelectionStyle.tsapp/packages/state/src/hooks/useOnSelectLabel.tsapp/packages/state/src/recoil/atoms.tsapp/packages/state/src/recoil/looker.tsapp/packages/state/src/recoil/selectors.tsapp/packages/state/src/recoil/types.tsapp/packages/state/src/session.tsapp/schema.graphqlfiftyone/core/session/constants.pyfiftyone/core/session/events.pyfiftyone/core/session/session.pyfiftyone/core/state.pyfiftyone/operators/executor.pyfiftyone/operators/operations.pyfiftyone/server/events/dispatch.pyfiftyone/server/inputs.pyfiftyone/server/mutation.pytests/unittests/operators/executor_tests.pytests/unittests/session_events_tests.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/packages/looker/src/state.ts (1)
189-193: 🛠️ Refactor suggestion | 🟠 MajorTighten
selectedLabelTypesto supported union values
Record<string, string>still permits invalid mode values and weakens compile-time guarantees when indexing style config. This concern is still present.#!/bin/bash # Verify current broad typing and producer typing rg -nP --type=ts "selectedLabelTypes\\s*:\\s*Record<string,\\s*string>" app/packages/looker/src/state.ts app/packages/state/src/recoil/selectors.ts rg -nP --type=ts "type\\s+SelectionType|\"default\"\\s*\\|\\s*\"alt\"" app/packages/state/src/recoil/types.tsAs per coding guidelines
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/looker/src/state.ts` around lines 189 - 193, The selectedLabelTypes field is too permissive as Record<string, string>; change its type to Record<string, LabelSelectionStyleName> (or the explicit union used by LabelSelectionStyleName) so indexing into labelSelectionStyle is type-safe; update any producers/assignments (selectors or setters) that currently write arbitrary strings to selectedLabelTypes to provide only the union values or cast/convert them, and import/use LabelSelectionStyleName where selectedLabelTypes is declared to ensure compile-time enforcement across get/set paths (refer to selectedLabelTypes, LabelSelectionStyleName, and labelSelectionStyle in the file and related selectors).
🤖 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/state/src/recoil/atoms.ts`:
- Around line 215-223: The sessionAtom generic parameters are incorrect — they
should be a keyof Session, not the value types; remove the explicit generic
arguments on the two atoms so TypeScript can infer the key from the key
property. Update the calls to sessionAtom used for sampleSelectionStyle and
labelSelectionStyle (which use DEFAULT_SELECTION_STYLE and
DEFAULT_LABEL_SELECTION_STYLE) by deleting the <SelectionStyle> and
<LabelSelectionStyle> generics so the compiler infers types from the provided
key strings.
---
Duplicate comments:
In `@app/packages/looker/src/state.ts`:
- Around line 189-193: The selectedLabelTypes field is too permissive as
Record<string, string>; change its type to Record<string,
LabelSelectionStyleName> (or the explicit union used by LabelSelectionStyleName)
so indexing into labelSelectionStyle is type-safe; update any
producers/assignments (selectors or setters) that currently write arbitrary
strings to selectedLabelTypes to provide only the union values or cast/convert
them, and import/use LabelSelectionStyleName where selectedLabelTypes is
declared to ensure compile-time enforcement across get/set paths (refer to
selectedLabelTypes, LabelSelectionStyleName, and labelSelectionStyle in the file
and related selectors).
🪄 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: 9a0eccb8-5909-4a72-aef8-97b7a372804f
📒 Files selected for processing (6)
app/packages/looker/src/state.tsapp/packages/operators/src/state.tsapp/packages/state/src/recoil/atoms.tsfiftyone/core/session/constants.pyfiftyone/core/session/session.pytests/unittests/operators/operations_tests.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/packages/app/src/useEvents/utils.ts (1)
89-97: 🧹 Nitpick | 🔵 TrivialConsider aligning
resolveSampleSelectionStylewith the same merge pattern.
resolveLabelSelectionStylemerges defaults with incoming state, butresolveSampleSelectionStyledoes a simple fallback. If the server ever sends a partialsample_selection_style, it could cause similar issues downstream.Proposed alignment
const resolveSampleSelectionStyle = (state: { sample_selection_style?: SelectionStyle; }): SelectionStyle => { if (env().VITE_NO_STATE) { return DEFAULT_SELECTION_STYLE; } - return state.sample_selection_style || DEFAULT_SELECTION_STYLE; + return { + ...DEFAULT_SELECTION_STYLE, + ...(state.sample_selection_style || {}), + }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/app/src/useEvents/utils.ts` around lines 89 - 97, resolveSampleSelectionStyle currently returns state.sample_selection_style or DEFAULT_SELECTION_STYLE which loses default fields when the server sends a partial style; change it to merge defaults with the incoming partial like resolveLabelSelectionStyle does. Update resolveSampleSelectionStyle to early-return DEFAULT_SELECTION_STYLE when env().VITE_NO_STATE is true, otherwise return a merged object (e.g., combine DEFAULT_SELECTION_STYLE with state.sample_selection_style via object spread) so missing fields are filled from DEFAULT_SELECTION_STYLE; reference the function name resolveLabelSelectionStyle as the pattern to follow.
🤖 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/state/src/recoil/selectors.ts`:
- Around line 295-305: The selector selectedLabelTypes currently reads
atoms.selectedLabels and collapses duplicate labelIds by last-write, which can
downgrade an existing "alt" when shift-click appends entries without a type;
change the selector to derive types from the canonical map used elsewhere
(selectors.selectedLabelMap) instead of atoms.selectedLabels so duplicate-ID
resolution matches the canonical semantics—fetch
get(selectors.selectedLabelMap), iterate its entries to produce the
Record<string, SelectionType>, and coerce missing/undefined types to "default"
while preserving "alt".
---
Outside diff comments:
In `@app/packages/app/src/useEvents/utils.ts`:
- Around line 89-97: resolveSampleSelectionStyle currently returns
state.sample_selection_style or DEFAULT_SELECTION_STYLE which loses default
fields when the server sends a partial style; change it to merge defaults with
the incoming partial like resolveLabelSelectionStyle does. Update
resolveSampleSelectionStyle to early-return DEFAULT_SELECTION_STYLE when
env().VITE_NO_STATE is true, otherwise return a merged object (e.g., combine
DEFAULT_SELECTION_STYLE with state.sample_selection_style via object spread) so
missing fields are filled from DEFAULT_SELECTION_STYLE; reference the function
name resolveLabelSelectionStyle as the pattern to follow.
🪄 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: 5f68dac3-a85c-4d9d-8457-24e1fe45c929
📒 Files selected for processing (6)
app/packages/app/src/useEvents/useSetLabelSelectionStyle.tsapp/packages/app/src/useEvents/utils.tsapp/packages/state/src/recoil/selectors.tsapp/packages/state/src/recoil/types.tsfiftyone/core/session/events.pyfiftyone/server/mutation.py
…nonical map Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
benjaminpkane
approved these changes
Apr 9, 2026
Member
benjaminpkane
left a comment
benjaminpkane
left a comment
There was a problem hiding this comment.
Looks great. Incredible incredible detail ✨
If we do encounter bugs/regressions we should update e2e POMs as necessary
7 tasks
tom-vx51
approved these changes
Apr 9, 2026
swheaton
approved these changes
Apr 9, 2026
Contributor
swheaton
left a comment
swheaton
left a comment
There was a problem hiding this comment.
looks fine to me from backend (minimal changes in this area)
🚢 🇮🇹
Member
Author
dashed-green and dashed-red |
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.
🔗 Related Issues
closes FOEPD-3357
📋 What changes are proposed in this pull request?
Summary
dashed(cyan, default),dashed-green, ordashed-redChanges
Python SDK
session.set_label_selection_style(default="dashed", alt="dashed-red")— configure visual styles per selection typesession.clear_label_selection_style()— reset to defaultssession.label_selection_style— get/set the style configsession.selected_labelsnow includes atypekey ("default"or"alt")Operators
ctx.label_selection_style— read label selection style in operator contextctx.ops.set_label_selection_style(default, alt)/ctx.ops.clear_label_selection_style()Frontend
"alt"selection typeresolveLabelSelectionVisuals()resolves per-label color based on type and configured style🧪 How is this patch tested? If it is not, please explain why.
session.set_label_selection_style(alt="dashed-red")→ alt-clicked labels turn red dashedsession.clear_label_selection_style()→ reverts all to cyan dashedTesting sample code
Python SDK testing
Validation checklist for all overlay types
📝 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
Bug Fixes