Human Annotation Sidebar #6410
Human Annotation Sidebar #6410
Conversation
WalkthroughThis change refactors lighter integration across modal rendering and the Annotate sidebar, replaces legacy overlay flows, adds polyline/3D-aware schema loading, introduces UpdateLabelCommand with extended command events, simplifies InteractiveDetectionHandler, updates overlay APIs, adjusts color mapping via context, and revises label state/creation/editing pipelines and related UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant UI as Annotate UI
participant LS as Lighter Scene
participant OV as Overlay
participant EB as EventBus
U->>UI: Edit label property
UI->>LS: new UpdateLabelCommand(currentLabel,nextLabel)
LS->>OV: overlay.updateLabel(nextLabel)
note right of OV: Overlay label updated
LS-->>EB: COMMAND_EXECUTED {id, undoable, command}
EB-->>UI: Notify command executed
sequenceDiagram
autonumber
actor U as User
participant UI as Actions (Detection)
participant LS as Lighter Scene
participant OD as InteractiveDetectionHandler
participant OV as BoundingBoxOverlay
U->>UI: Click "Detection"
UI->>LS: Create overlay via factory
LS->>OD: Initialize with overlay
U->>OD: Drag on canvas
OD->>OV: update absoluteBounds
U->>OD: Release mouse
OD->>OV: finalize bounds (min-size check)
note right of OV: Overlay remains on scene
sequenceDiagram
autonumber
participant App as App
participant SB as Annotate Sidebar
participant ST as State (atoms)
participant LS as Lighter Scene
App->>SB: Render
SB->>ST: read editing, scene, labels
alt editing
SB-->>App: Render Edit (Id/Position/Schema)
else showImport
SB-->>App: Render ImportSchema
else
SB-->>App: Render AnnotateSidebar
end
SB->>LS: subscribe overlay deselect -> clear editing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
fiftyone/core/annotation.py (1)
130-132: Inconsistent default in exception handler.When
is_listis True and an exception occurs (too many distinct values), the exception handler returnsdefault: Noneinstead ofdefault: []. This is inconsistent with the list handling above.Consider applying this diff for consistency:
except: # too many distinct values - return {"default": None, "type": "input"} + return {"default": [] if is_list else None, "type": "input"}app/packages/core/src/components/Modal/Lighter/useOverlayPersistence.ts (1)
43-49: Critical: Null pointer error when overlay is falsy.The else branch attempts to access
overlay.constructor.namewhenoverlayis falsy (line 25's condition failed). This will throw a TypeError at runtime.Apply this diff to fix the error:
} else { console.error( - "Overlay", - overlay.constructor.name, - "not supported for persistence" + "Invalid overlay: overlay data is missing or falsy" ); }app/packages/core/src/components/Modal/Lighter/SharedCanvas.ts (1)
56-60: Use explicit null checks for proper TypeScript type narrowing.While the optional chaining
this.canvas?.parentNodeguards correctly at runtime, TypeScript's control flow analysis does not narrowthis.canvasto non-null within the if block. This means line 57's access tothis.canvas.parentNodeis flagged as potentially unsafe by TypeScript's type system with strict null checks enabled.Apply this diff to restore explicit null checking for proper type narrowing:
- if (this.canvas?.parentNode) { - this.canvas.parentNode.removeChild(this.canvas); + if (this.canvas && this.canvas.parentNode) { + this.canvas.parentNode.removeChild(this.canvas); this.isAttached = false; this.container = null; }This follows TypeScript best practices by ensuring the compiler can properly narrow types and verify safety.
As per coding guidelines.
app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx (1)
44-45: Don’t destroy the shared scene in component cleanupscene.destroy() in this renderer likely tears down the shared scene used by LighterSetupWithPixi and other consumers, and will fire on every sample change due to dependency on sample. Prefer removing the sample-specific overlay and clearing canonical media.
- const { scene, isReady, addOverlay } = useLighter(); + const { scene, isReady, addOverlay, removeOverlay } = useLighter(); … - return () => { - scene.destroy(); - }; + return () => { + if (mediaOverlay) { + try { + // clear canonical media if pointing to this overlay + if (scene.getCanonicalMedia?.() === mediaOverlay) { + scene.setCanonicalMedia(null); + } + } catch {} + removeOverlay(mediaOverlay, false); + } + };Also consider excluding sample from the effect if you intend to swap media without reinitializing; otherwise you will repeatedly add/remove overlays anyway.
Also applies to: 71-74
app/packages/looker/src/overlays/base.ts (1)
125-146: Guard optionallabel.tagsbefore calling.includes
BaseLabel.tagsis now optional, butthis.label.tags.includes(tag)and the call toshouldShowLabelTagassume it’s always an array. When a label comes through without tags, this will throw at runtime. Normalize the value first.- isTagFiltered(state: Readonly<State>): boolean { - return state.options.selectedLabelTags?.some((tag) => - this.label.tags.includes(tag) - ); - } + isTagFiltered(state: Readonly<State>): boolean { + const labelTags = this.label.tags ?? []; + return state.options.selectedLabelTags?.some((tag) => + labelTags.includes(tag) + ); + } … - isTagged: shouldShowLabelTag(selectedLabelTags, this.label.tags), + isTagged: shouldShowLabelTag( + selectedLabelTags, + this.label.tags ?? [] + ),app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx (1)
80-88: Guard attributes presence when building schema.Looping over
attributeswhen undefined will throw.- return useMemo(() => { + return useMemo(() => { const properties = {}; - - const attributes = config?.attributes; + const attributes = config?.attributes ?? {}; properties.label = createSelect("label", config?.classes ?? []); - - for (const attr in attributes) { + for (const attr in attributes) { if (attr === "id") { continue; } if (attributes[attr].type === "input") { properties[attr] = createInput(attr); } if (attributes[attr].type === "radio") { properties[attr] = createRadio(attr, attributes[attr].values); } if (attributes[attr].type === "tags") { properties[attr] = createTags(attr, attributes[attr].values); } - if (attributes[attr].type === "text") { - throw "text"; - } + if (attributes[attr].type === "text") { + // Fallback: treat as plain input + properties[attr] = createInput(attr); + } }Also applies to: 89-110
app/packages/core/src/components/Modal/Sidebar/Annotate/useLabels.ts (1)
135-168: RestorehandleSampleDatato an invokable callbackAssigning
useEffecttohandleSampleDatameans you now exportundefined(or a cleanup fn) to callers that still expect an executable callback—anyhandleSampleData()invocation will throw. Please keephandleSampleDataas a memoized function and invoke it from an effect instead of returning the effect result.- const handleSampleData = useEffect(() => { - if (modalSampleData.state !== "loading" && schemaMap) { - handleSample({ - addLabel, - paths, - filter, - sample: modalSampleData.contents, - getFieldType, - schemas: schemaMap, - }).then((result) => { - setLoading(false); - setLabels(result); - }); - } else { - setLoading(true); - } - }, [ - addLabel, - filter, - getFieldType, - modalSampleData, - paths, - schemaMap, - setLabels, - setLoading, - ]); + const handleSampleData = useCallback(() => { + if (modalSampleData.state !== "loading" && schemaMap) { + handleSample({ + addLabel, + paths, + filter, + sample: modalSampleData.contents, + getFieldType, + schemas: schemaMap, + }).then((result) => { + setLoading(false); + setLabels(result); + }); + } else { + setLoading(true); + } + }, [ + addLabel, + filter, + getFieldType, + modalSampleData, + paths, + schemaMap, + setLabels, + setLoading, + ]); + + useEffect(() => { + handleSampleData(); + }, [handleSampleData]); return { handleSampleData, };
🧹 Nitpick comments (17)
app/packages/core/src/components/Modal/Lighter/useOverlayPersistence.ts (1)
62-62: Remove debug console.log statement.Production code should not contain console.log statements used for debugging.
Apply this diff:
const { id, sampleId, path } = event.detail; try { - console.log("removing bounding box", id, sampleId, path); removeBoundingBox.execute({app/packages/core/src/components/Modal/Sidebar/Annotate/LabelEntry.tsx (1)
86-87: Avoid direct store access; prefer hooks.Directly calling
getDefaultStore().get(atom).overlay.setSelected(true)bypasses React's rendering cycle and Jotai's typical hook patterns. This can lead to:
- Stale closures or race conditions
- Reduced testability
- Breaking React's unidirectional data flow
Consider using Jotai hooks instead:
onClick={() => { setEditing(atom); - const store = getDefaultStore(); - store.get(atom).overlay.setSelected(true); }}Then handle the selection state change via a Jotai atom or callback. If
overlay.setSelectedneeds to be called imperatively, consider wrapping it in a proper state update mechanism or custom hook that maintains React's data flow.Alternatively, if imperative access is genuinely required (e.g., for performance reasons or external library integration), document why this pattern is necessary to avoid confusion in future maintenance.
app/packages/lighter/src/renderer/SharedPixiApplication.ts (1)
6-11: Global PIXI scaleMode default: verify API and import orderSetting TextureStyle.defaultOptions.scaleMode = "nearest" relies on a specific PIXI API and on this module loading before any textures. Two asks:
- Verify your PIXI version supports TextureStyle.defaultOptions.scaleMode with string literals ('nearest' | 'linear'); otherwise prefer the constants API (e.g., SCALE_MODES.NEAREST).
- Ensure this module is imported before any texture creation; otherwise some textures will still use the previous default.
If you need a defensive fallback across PIXI versions, gate it:
-import { TextureStyle } from "pixi.js"; +import * as PIXI from "pixi.js"; +// @ts-expect-error: optional for cross-version compatibility +const TextureStyle = (PIXI as any).TextureStyle; -TextureStyle.defaultOptions.scaleMode = "nearest"; +if (TextureStyle?.defaultOptions) { + TextureStyle.defaultOptions.scaleMode = "nearest"; +} else if ((PIXI as any).settings?.SCALE_MODE) { + // PIXI v6/early v7 + (PIXI as any).settings.SCALE_MODE = + (PIXI as any).SCALE_MODES?.NEAREST ?? (PIXI as any).NEAREST ?? 0; +}app/packages/core/src/components/Modal/Sidebar/Annotate/Actions.tsx (2)
134-149: Removed disabled gating: verify UX when no default fieldClassification/Detection are now always clickable. If defaultField(type) is unset, useCreate returns undefined and editing is set to the raw type. Confirm this is intentional and doesn’t regress flows. If not, restore disabled state:
-const Classification = () => { - const create = useCreate(CLASSIFICATION); - return (<Square onClick={create}>…</Square>); -}; +const Classification = () => { + const create = useCreate(CLASSIFICATION); + const hasDefault = useRecoilValue(defaultFieldHasValueSelector(CLASSIFICATION)); // example + return ( + <Square className={hasDefault ? "" : "disabled"} onClick={hasDefault ? create : undefined}> + … + </Square> + ); +}Same for Detection. If you want, I can wire this to your actual selector/atom.
Also applies to: 155-170
84-86: Leftover debug alertsalert("NAVIGATION") and alert("Move") look like placeholders. Replace with real tool activation or remove before release.
Also applies to: 110-112
app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx (2)
52-69: Media overlay lifecycle: capture ref for cleanupStore the created media overlay in a ref so cleanup can remove it reliably on re-renders. Example:
+ let overlayRef: ImageOverlay | null = null; if (mediaUrl) { const mediaOverlay = overlayFactory.create<ImageOptions, ImageOverlay>("image", { src: mediaUrl, maintainAspectRatio: true, }); addOverlay(mediaOverlay, false); + overlayRef = mediaOverlay; scene.setCanonicalMedia(mediaOverlay); }
103-104: Canvas acquisition: guard nullsIf containerRef.current is ever null during layout thrash, singletonCanvas.getCanvas may throw. Optional chaining or early-return can harden this:
-const canvas = singletonCanvas.getCanvas(containerRef.current); +const canvas = containerRef.current + ? singletonCanvas.getCanvas(containerRef.current) + : null; +if (!canvas) return null;app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/state.ts (3)
71-83: No-op early return when path is unchangedAvoid unnecessary overlay.updateField and atom writes if the path didn't change.
(get, set, path: string) => { const label = get(current); if (!label) { return; } + if (label.path === path) { + return; + } label.overlay.updateField(path); set(current, { ...label, path }); }
173-194: datasetId/sampleId are unused; confirm persistence paths and update behaviorBoth saveValue and deleteValue accept { datasetId, sampleId } but never use them. Also, saveValue only handles isNew; updates to existing labels aren’t persisted here.
- If addLabel/remove flows require dataset/sample IDs, thread them through; otherwise drop these params to avoid confusion.
- Confirm that non-new edits are persisted via overlay/command flows (e.g., UpdateLabelCommand). If not, add an update path here.
Also applies to: 196-217
20-21: Typo: IS_CLASSIFICIATIONMinor rename for clarity/consistency.
-const IS_CLASSIFICIATION = new Set([CLASSIFICATION, CLASSIFICATIONS]); +const IS_CLASSIFICATION = new Set([CLASSIFICATION, CLASSIFICATIONS]); ... - [CLASSIFICATION]: IS_CLASSIFICIATION, + [CLASSIFICATION]: IS_CLASSIFICATION,Also applies to: 25-28
app/packages/core/src/components/Modal/Lighter/useLighterTooltipEventHandler.ts (1)
5-7: Avoid deep import path for typesImporting Hoverable from '@fiftyone/lighter/src/types' can be brittle. Prefer an entrypoint export (e.g., '@fiftyone/lighter') and re-export Hoverable there.
app/packages/lighter/src/event/EventBus.ts (1)
5-6: Use type-only imports to reduce runtime couplingCommand and BaseOverlay are used only in types; switch to type-only imports.
-import { Command } from "../commands/Command"; -import { BaseOverlay } from "../overlay/BaseOverlay"; +import type { Command } from "../commands/Command"; +import type { BaseOverlay } from "../overlay/BaseOverlay";Also applies to: 135-141
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Id.tsx (1)
28-40: Remove redundant wrappers.The component has both a fragment (
<></>) and a<div>wrapper that appear redundant.Apply this diff to simplify:
const Id = () => { const overlay = useAtomValue(currentOverlay); return ( - <> - <div> - <SchemaIOComponent schema={createSchema()} data={{ id: overlay?.id }} /> - </div> - </> + <SchemaIOComponent schema={createSchema()} data={{ id: overlay?.id }} /> ); };app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useColor.ts (1)
9-18: Consider adding a comment to clarify therefreshpattern.Line 15 references
refreshwithout using its value, which is a valid pattern to force re-computation whenrefreshchanges. However, this could be confusing to other developers.Apply this diff to add a clarifying comment:
export default function useColor(overlay?: BaseOverlay) { const coloring = useColorMappingContext(); const refresh = useAtomValue(current); const brand = useTheme().primary.plainColor; return useMemo(() => { - refresh; + refresh; // Force re-computation when current atom changes return overlay ? getOverlayColor(overlay, coloring) : brand; }, [brand, coloring, refresh, overlay]); }app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx (1)
61-79: Filter overlay move/resize events to the active overlay to avoid unnecessary renders.Current handler runs on every overlay event. Filter by the event’s overlay to reduce updates.
+import type { BaseOverlay } from "@fiftyone/lighter"; @@ - useEffect(() => { - const handler = () => { + useEffect(() => { + const handler = (o?: BaseOverlay) => { if (!(overlay instanceof BoundingBoxOverlay)) { return; } + if (o && o !== overlay) return; const rect = overlay.getAbsoluteBounds(); setState({ position: { x: rect.x, y: rect.y }, dimensions: { width: rect.width, height: rect.height }, }); }; scene?.on(LIGHTER_EVENTS.OVERLAY_DRAG_MOVE, handler); scene?.on(LIGHTER_EVENTS.OVERLAY_RESIZE_MOVE, handler); @@ - }, [overlay, scene]); + }, [overlay, scene]);If the event payload doesn’t include an overlay, consider namespaced handlers or throttling. Based on learnings
app/packages/core/src/components/Modal/Sidebar/Annotate/useLoadSchemas.ts (1)
63-64: Consider including operator executor in deps.Safer to include
get(orget.execute) in the callback deps if it’s not stable.app/packages/core/src/components/Modal/Lighter/useBridge.ts (1)
21-35: Avoid unnecessary re-marking of overlays when context is unchanged.If
useColorMappingContext()returns a new object each render with same values, this effect will churn. Memoize the context (inside the hook) or compare a stable key before marking overlays dirty.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx(4 hunks)app/packages/core/src/components/Modal/Lighter/SharedCanvas.ts(1 hunks)app/packages/core/src/components/Modal/Lighter/looker-lighter-bridge.ts(0 hunks)app/packages/core/src/components/Modal/Lighter/useBridge.ts(3 hunks)app/packages/core/src/components/Modal/Lighter/useColorMappingContext.ts(1 hunks)app/packages/core/src/components/Modal/Lighter/useLighterTooltipEventHandler.ts(1 hunks)app/packages/core/src/components/Modal/Lighter/useOverlayPersistence.ts(4 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Actions.tsx(5 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Annotate.tsx(3 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AddSchema.tsx(3 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx(3 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Edit.tsx(2 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Field.tsx(3 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Footer.tsx(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Header.tsx(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Id.tsx(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/state.ts(2 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useColor.ts(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useCreate.ts(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useMove.ts(0 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useSpatialAttribute.ts(0 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Icons.tsx(3 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/LabelEntry.tsx(3 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/state.ts(0 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/useAddAnnotationLabel.ts(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/useColor.ts(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/useEntries.ts(0 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/useLabels.ts(5 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/useLoadSchemas.ts(3 hunks)app/packages/core/src/plugins/SchemaIO/components/AutocompleteView.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/index.tsx(0 hunks)app/packages/lighter/src/commands/UpdateLabelCommand.ts(1 hunks)app/packages/lighter/src/core/Scene2D.ts(2 hunks)app/packages/lighter/src/event/EventBus.ts(2 hunks)app/packages/lighter/src/index.ts(2 hunks)app/packages/lighter/src/interaction/InteractiveDetectionHandler.ts(2 hunks)app/packages/lighter/src/overlay/BaseOverlay.ts(4 hunks)app/packages/lighter/src/overlay/BoundingBoxOverlay.ts(8 hunks)app/packages/lighter/src/overlay/ClassificationOverlay.ts(2 hunks)app/packages/lighter/src/react/useLighterSetup.ts(1 hunks)app/packages/lighter/src/renderer/Renderer2D.ts(1 hunks)app/packages/lighter/src/renderer/SharedPixiApplication.ts(1 hunks)app/packages/looker/src/overlays/base.ts(2 hunks)app/packages/looker/src/overlays/polyline.ts(1 hunks)app/packages/state/src/recoil/sidebar.ts(2 hunks)fiftyone/core/annotation.py(1 hunks)
💤 Files with no reviewable changes (6)
- app/packages/core/src/plugins/SchemaIO/index.tsx
- app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useSpatialAttribute.ts
- app/packages/core/src/components/Modal/Sidebar/Annotate/useEntries.ts
- app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useMove.ts
- app/packages/core/src/components/Modal/Sidebar/Annotate/state.ts
- app/packages/core/src/components/Modal/Lighter/looker-lighter-bridge.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/core/src/plugins/SchemaIO/components/AutocompleteView.tsxapp/packages/core/src/components/Modal/Lighter/SharedCanvas.tsapp/packages/core/src/components/Modal/Lighter/useColorMappingContext.tsapp/packages/core/src/components/Modal/Lighter/useOverlayPersistence.tsapp/packages/state/src/recoil/sidebar.tsapp/packages/core/src/components/Modal/Lighter/useLighterTooltipEventHandler.tsapp/packages/looker/src/overlays/base.tsapp/packages/lighter/src/overlay/ClassificationOverlay.tsapp/packages/lighter/src/event/EventBus.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useCreate.tsapp/packages/core/src/components/Modal/Lighter/useBridge.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useColor.tsapp/packages/lighter/src/interaction/InteractiveDetectionHandler.tsapp/packages/lighter/src/core/Scene2D.tsapp/packages/lighter/src/overlay/BoundingBoxOverlay.tsapp/packages/lighter/src/renderer/SharedPixiApplication.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/LabelEntry.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsxapp/packages/lighter/src/commands/UpdateLabelCommand.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/useColor.tsapp/packages/lighter/src/renderer/Renderer2D.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Footer.tsxapp/packages/looker/src/overlays/polyline.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/useLabels.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Header.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/state.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Id.tsxapp/packages/lighter/src/react/useLighterSetup.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/useAddAnnotationLabel.tsapp/packages/lighter/src/index.tsapp/packages/lighter/src/overlay/BaseOverlay.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Annotate.tsxapp/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/useLoadSchemas.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Edit.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Icons.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AddSchema.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Actions.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Field.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx
🔇 Additional comments (22)
fiftyone/core/annotation.py (1)
124-125: Semantic improvement: "tags" with empty list default for list fields.The change from "checkbox" to "tags" and from None to
[]as the default is more semantically appropriate for list-type string fields. This aligns well with the annotation types documented in the function docstring.However, ensure that all consumers of this annotation schema (particularly in the App/frontend mentioned in the PR objectives) handle the new default value correctly. The change from None to
[]could affect existing code that checks for None.app/packages/core/src/components/Modal/Lighter/useOverlayPersistence.ts (2)
5-8: Refactor approach looks sound.The shift from label-centric to overlay-centric persistence using bounding box operators is clean. The event handling pattern with proper cleanup and dependency management follows React best practices.
Also applies to: 13-88
14-15: Verify operator executor stability.Ensure that
useOperatorExecutorreturns stable references across renders. If the executors are recreated on each render, the callbacks on lines 17-52 and 54-73 will also be unnecessarily recreated, though correctness is maintained by the effect's dependency array (line 87).Run the following script to examine the
useOperatorExecutorimplementation:app/packages/core/src/components/Modal/Sidebar/Annotate/LabelEntry.tsx (2)
63-81: Verifylabel.overlayis always defined.The code now consistently uses
label.overlay.idfor hover detection (line 63), event dispatching (lines 71, 76), effect dependencies (line 79), and color mapping (line 81). This refactoring appears intentional and consistent.However, ensure that
label.overlayis always defined and has the expected properties (id, color-related fields, andsetSelectedmethod) to prevent runtime errors.Run the following script to verify the
AnnotationLabeltype definition guaranteesoverlayis non-nullable:
97-99: Verify data source consistency:overlayvsdata.The component now uses
label.overlayfor color and identification, but still useslabel.data.labelfor displaying the label text. This mixed approach could indicate incomplete refactoring.Ensure this split is intentional:
- If
label.overlayandlabel.datashould stay synchronized, consider deriving both from the same source- If they serve different purposes, consider adding a comment explaining why
data.labelis used for text whileoverlayis used for other propertiesRun the following script to check how
label.data.labelandlabel.overlayrelate throughout the codebase:app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useCreate.ts (1)
75-83: Editing fallback shape: confirm consumer expectationsWhen creation fails, setEditing(type) stores the raw LabelType. Confirm editing atom/type unions support this across consumers. If not, prefer set to null/undefined and keep UI disabled. I can help audit downstream usage.
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/state.ts (1)
122-139: currentType throws when not editing; ensure guarded usageThis atom throws "no type" if editing is null. Verify all consumers guard with isEditing before dereferencing currentType/currentFields/currentDisabledFields.
If not guaranteed, consider returning null and widening the type accordingly to prevent runtime errors.
app/packages/lighter/src/core/Scene2D.ts (1)
1134-1137: Richer COMMAND_EXECUTED payload looks good; verify consumersIncluding the command object is useful. Ensure downstream listeners are updated for the new detail shape and won’t accidentally retain heavy references long-term.
app/packages/lighter/src/renderer/Renderer2D.ts (1)
5-5: Type-only import is correct hereReduces runtime coupling and avoids unnecessary bundling.
app/packages/core/src/components/Modal/Lighter/useColorMappingContext.ts (1)
5-15: LGTM!The hook correctly reads Recoil state and memoizes the result with appropriate dependencies. The implementation follows React and Recoil best practices.
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AddSchema.tsx (1)
59-63: LGTM!The addition of
setEditing(null)correctly resets the editing state before opening the modal, ensuring a clean state for schema creation.app/packages/state/src/recoil/sidebar.ts (3)
119-129: LGTM!The addition of strongly-typed overlay properties (
ClassificationOverlayandBoundingBoxOverlay) improves type safety and aligns with the lighter integration.
114-117: Verify that all Label usages provide the requiredpathproperty.The
pathproperty is now required on theLabelinterface. If this was previously optional, this is a breaking change that could cause TypeScript errors or runtime issues in code that creates Label objects without providing a path.Run the following script to verify all Label usages:
131-135: Verify thatClassificationOverlayis appropriate forPolylineAnnotationLabel.The
PolylineAnnotationLabelusesClassificationOverlayfor its overlay type, which seems inconsistent with the pattern whereDetectionAnnotationLabelusesBoundingBoxOverlay. Typically, polylines would have their own dedicated overlay type (e.g.,PolylineOverlay).Please confirm whether:
ClassificationOverlayis intentionally reused for polylines due to shared properties- A dedicated
PolylineOverlaytype exists but wasn't used here- This is a temporary implementation pending a proper polyline overlay type
Run the following script to check for polyline overlay types:
app/packages/lighter/src/index.ts (2)
56-56: LGTM!The
UpdateLabelCommandexport is correctly placed in the Undo/Redo section and follows the established pattern of other command exports.
81-81: LGTM!The
getOverlayColorexport appropriately exposes the color mapping utility for external use, supporting the new color context architecture.app/packages/core/src/components/Modal/Sidebar/Annotate/Icons.tsx (2)
59-65: LGTM!The
Polylinecomponent follows the same pattern as other icon components (Classification,Detection) and correctly applies the fill color.
89-96: LGTM!The ICONS mapping correctly includes both
PolylineandPolylinesentries, following the established pattern for other label types.app/packages/lighter/src/react/useLighterSetup.ts (1)
8-15: LGTM!The import refactoring to use
globalPixiResourceLoaderandlighterSceneAtomfrom the public index improves module encapsulation without changing functionality.app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Id.tsx (1)
18-26: LGTM!The
createSchemahelper correctly creates an object schema for the ID field.app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Footer.tsx (1)
21-27: Handle missing sampleId and confirm Cancel behavior.
- Protect against
sampleIdbeing null before saving/deleting.- Confirm
deleteValuetreatsisNewas cancel (no backend delete).- <Button - onClick={() => { - saveCurrent({ datasetId, sampleId }); - }} - > + <Button + disabled={!sampleId} + onClick={() => { + if (!sampleId) return; + saveCurrent({ datasetId, sampleId }); + }} + > Save </Button> @@ - onClick={() => { - deleteCurrent({ datasetId, sampleId }); - }} + onClick={() => { + if (!sampleId) return; + deleteCurrent({ datasetId, sampleId }); + }}Also applies to: 29-41
app/packages/lighter/src/overlay/ClassificationOverlay.ts (1)
16-39: Don’t strip classification overlay rendering
ClassificationOptionsno longer carries the label, position, or confidence, andrenderImplnow returns immediately. As a result, classification overlays never draw anything—existing annotations disappear from the canvas. Please keep the rendering logic (or provide an equivalent replacement) and continue passing the label data the overlay needs.Could you confirm that classification overlays still appear after this change? If not, please restore the previous rendering logic before merging.
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AddSchema.tsx
Show resolved
Hide resolved
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx
Show resolved
Hide resolved
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx
Show resolved
Hide resolved
| if (!field) { | ||
| throw new Error("no overlay"); | ||
| } | ||
|
|
||
| if (!overlay) { | ||
| throw new Error("no overlay"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Don’t throw on missing field/overlay; render nothing instead.
Throwing breaks the UI. Gracefully no-op until state is ready.
- if (!field) {
- throw new Error("no overlay");
- }
-
- if (!overlay) {
- throw new Error("no overlay");
- }
+ if (!field || !overlay) {
+ return null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!field) { | |
| throw new Error("no overlay"); | |
| } | |
| if (!overlay) { | |
| throw new Error("no overlay"); | |
| } | |
| if (!field || !overlay) { | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx
around lines 161 to 168, the component currently throws Errors when field or
overlay are missing which breaks the UI; instead return null (render nothing)
until state is ready. Replace the throw statements with safe early returns
(e.g., return null) and ensure the component's return type allows null
(JSX.Element | null) or that the parent can handle undefined; keep any necessary
TypeScript narrowing so subsequent code assumes field/overlay exist after the
guards.
What changes are proposed in this pull request?
Sidebar integration for Human Annotation
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores