Feat/address console errors #6480
Conversation
fix overlay initialization on scene recreation
fix sample delta generation for ambiguous label types
* don't re-render inputs * tests * update via overlay after user input * tests * lint --------- Co-authored-by: Gavin McDonald <gavin@mcmorgans.us> Co-authored-by: Benjamin Kane <ben@voxel51.com>
* fix disacrd changes and bounds updates * linting * bugs * linting
FOEPD-2447: lighter: fix selection + drag bug
* fix ignoreSideEffects * Update app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * add test * lint * remove isBridgeLogicHandled --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
[HA] add discard action to annotation footer
* fix undo/redo and selection for classifications, clear undo/redo stack on exit * add label to classification creation
Fix typo and space
Release notes for OSS 1.10.0 / Enterprise 2.13.0
More 3D fixes
Changing wait interval for waitForSampleLoadDomAttribute
This reverts commit 2611d59.
undo latest 3d commit
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
app/packages/operators/src/components/OperatorExecutionMenu/index.tsx (1)
70-94: Add TypeScript types to Item component parameters.The
Itemfunction component's parameters are not typed, which reduces type safety and makes the code harder to maintain.Apply this diff to add proper types:
-function Item({ target, disabled, onClose, onOptionClick }) { +function Item({ + target, + disabled, + onClose, + onOptionClick +}: { + target: OperatorExecutionOption; + disabled: boolean; + onClose: () => void; + onOptionClick?: (option: OperatorExecutionOption) => void; +}) { return (app/packages/lighter/src/overlay/ClassificationOverlay.ts (1)
42-97: Restore rendering instead of returning early.
With the newreturn;the overlay never disposes previous elements, never draws the classification label, and never callsemitLoaded(), so classification overlays vanish and selection state changes have no visible effect. Please drop the guard so the original rendering path runs again.Apply this diff to remove the regression:
protected renderImpl(renderer: Renderer2D): void { - return;app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx (1)
56-78: Remove the sample dependency concern—this is actually a bug requiring a fix.The analysis confirms the original review comment identified a genuine issue. When users navigate to a different sample:
sampleRef.currentupdates to the new sample (line 51)- The overlay effect (lines 56-78) does NOT re-run because sample is not in its dependency array
- The sceneId effect (lines 80-86) runs only once (empty dependencies), so sceneId never changes when sample changes
- Result: old overlays persist instead of reloading for the new sample
The component needs to either:
- Add
sampleto the sceneId effect dependencies so it regenerates when sample changes, triggering a scene/component remount, OR- Add
sampleto the overlay effect dependencies so overlays reload when sample changes, OR- Clear existing overlays before loading new ones when sample changes
This is not a question of whether media "needs" to reload—it objectively should when the sample prop changes.
app/packages/looker-3d/src/annotation/annotation-toolbar/useAnnotationActions.tsx (1)
153-160: Remove unused dependenciescurrentSampleIdandcurrentActiveFieldfrom thehandleDeleteSelectedPointdependency array.The function body does not reference these variables. They should be removed from the dependency array (lines 154-155) to avoid unnecessary re-renders when their values change. The current dependencies should be:
selectedPoint,setPolylinePointTransforms,setSelectedPoint, andsetCurrentArchetypeSelectedForTransform.app/packages/looker-3d/src/annotation/annotation-toolbar/CoordinateInputs.tsx (1)
239-315: Normalize polylinePointTransforms usage before indexing.Both the memoized read and the task setter assume
polylinePointTransformsis an object. Right after deselecting a label it’snull, sopolylinePointTransforms[selectedPoint.labelId](and laterprev[labelId]) will throw. Normalize the container once and reuse that normalized value when you build the next state.- const segments = - polylinePointTransforms[selectedPoint.labelId]?.segments || []; + const segments = + polylinePointTransforms?.[selectedPoint.labelId]?.segments || []; @@ - setPolylinePointTransforms((prev) => { - const currentSegments = prev[labelId]?.segments || []; + setPolylinePointTransforms((prev) => { + const base = prev ?? {}; + const existing = base[labelId] ?? {}; + const currentSegments = existing.segments ?? []; @@ - const existingLabelData = prev[labelId]; - const path = existingLabelData?.path || currentActiveField || ""; - const sampleId = existingLabelData?.sampleId || currentSampleId; + const path = existing.path || currentActiveField || ""; + const sampleId = existing.sampleId || currentSampleId; @@ - ...prev, + ...base, [labelId]: { - ...(existingLabelData ?? {}), + ...existing, segments: newSegments, path, sampleId, }, };
♻️ Duplicate comments (11)
app/packages/core/src/components/Modal/Sidebar/Annotate/useFocus.ts (2)
15-15: Critical: Invalid hook usage - pass callbacks, not hook functionsYou're still passing hook functions (
useExit,useSave) directly touseConfirmExit, which expects plain callbacks. This violates the Rules of Hooks and will cause a runtime type mismatch.Call the hooks first to obtain their callbacks, then pass those to
useConfirmExit:+ const exit = useExit(); + const save = useSave(); - const { confirmExit } = useConfirmExit(useExit, useSave); + const { confirmExit } = useConfirmExit(exit, save); const selectId = useRef<string | null>(null); const onExit = useExit(false);
42-46: Intended overlay selection is lost when no unsaved changes existWhen there are no unsaved changes, the handler exits without calling
select(). If the user previously attempted to select another overlay (stored inselectId.currentby the SELECT handler), that selection is never applied.if (!current || !STORE.get(hasChanges)) { // no unsaved changes, allow the exit onExit(); + select(); return; }app/packages/state/src/jotai/modal.ts (1)
17-43: Critical: SSR and storage safety guards still missing.Direct
localStorageaccess ingetItem,setItem, andremoveItemwill throw in SSR, Safari Private Mode, or sandboxed contexts. This directly contradicts the PR objective to address console errors.Apply this diff to add the necessary guards:
getItem: (key: string, initialValue: T): T => { const datasetName = getDatasetName(); const prefixedKey = datasetName ? `${datasetName}_${key}` : key; - const storedValue = localStorage.getItem(prefixedKey); - - if (storedValue === null) { - return initialValue; - } - + if (typeof window === "undefined") return initialValue; try { + const storedValue = window.localStorage.getItem(prefixedKey); + if (storedValue === null) return initialValue; return JSON.parse(storedValue) as T; } catch { return initialValue; } }, setItem: (key: string, value: T): void => { const datasetName = getDatasetName(); const prefixedKey = datasetName ? `${datasetName}_${key}` : key; - localStorage.setItem(prefixedKey, JSON.stringify(value)); + if (typeof window === "undefined") return; + try { + window.localStorage.setItem(prefixedKey, JSON.stringify(value)); + } catch { + // ignore write errors (quota/private-mode) + } }, removeItem: (key: string): void => { const datasetName = getDatasetName(); const prefixedKey = datasetName ? `${datasetName}_${key}` : key; - localStorage.removeItem(prefixedKey); + if (typeof window === "undefined") return; + try { + window.localStorage.removeItem(prefixedKey); + } catch { + // ignore remove errors + } },app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useDelete.ts (1)
34-48: Stop dispatching when schema/scene are missing
getFieldSchemastill returnsField | null, so the non-null assertion will crash when the schema lookup fails (this was already flagged earlier). On top of that we flipisSavingtotrueeven whensceneis undefined, leaving the UI stuck because no dispatch runs to reset it. Please guard both conditions before dispatching so we either exit cleanly or dispatch with a realField.- setSaving(true); - scene?.dispatchSafely({ + if (!scene) { + return; + } + + const fieldSchema = getFieldSchema(schema, label.path); + if (!fieldSchema) { + return; + } + + setSaving(true); + scene.dispatchSafely({ type: LIGHTER_EVENTS.DO_REMOVE_OVERLAY, detail: { label, - schema: getFieldSchema(schema, label?.path)!, + schema: fieldSchema, onSuccess: () => {app/packages/looker-3d/src/annotation/annotation-toolbar/useAnnotationActions.tsx (1)
137-140: Still filtering only empty segments; polylines need ≥2 vertices.As noted in the previous review, filtering
segment.points.length > 0can leave single-vertex segments, which are invalid for polylines.Apply this diff to filter degenerate segments:
- // Remove empty segments const filteredSegments = newSegments.filter( - (segment) => segment.points.length > 0 + (segment) => segment.points.length >= 2 );app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useExit.ts (1)
59-72: Unsaved label cleanup is still bypassed.The early
if (!label || !revertLabel)guard runs before theunsaved?.isNewbranch, so new overlays never hitremoveOverlay(...)—exactly the critical bug called out earlier. Please restore the prior ordering so the unsaved-removal logic runs first.- if (!label || !revertLabel) { - setSaved(null); - setEditing(null); - return; - } - - // label has not been persisted, so remove it - if (unsaved?.isNew) { + // label has not been persisted, so remove it + if (unsaved?.isNew) { removeOverlay(unsaved?.overlay.id); scene?.exitInteractiveMode(); setEditing(null); setSaved(null); return; } + if (!label || !revertLabel) { + setSaved(null); + setEditing(null); + return; + }app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx (1)
54-87: Compare overlay ids and guard event.detail safely.The bounds events fire with the overlay id, not the label’s
_id, soevent.detail.id !== data?._idstill filters out legitimate updates. It also blows up whenevent.detailis missing. Please key offoverlay.id(as previously suggested) and null-check the detail before readingid.- const handler = (event) => { + const handler = (event: CustomEvent<{ id?: string }>) => { if ( !(overlay instanceof BoundingBoxOverlay) || - !overlay.hasValidBounds() || - event.detail.id !== data?._id + !overlay.hasValidBounds() || + event.detail?.id !== overlay?.id ) { return; } @@ - }, [data?._id, overlay, scene, setData]); + }, [overlay?.id, overlay, scene, setData]);app/packages/looker-3d/src/annotation/useSetEditingToNewPolyline.tsx (1)
92-101: Ensure new polyline path is always a string.This still uses
transformData.path ?? currentActiveField, so when both areundefined/nullwe end up with a non-string path — exactly what the earlier review flagged. Please fall back to an empty string to keep downstream lookups and typing happy.- path: transformData.path ?? currentActiveField, + path: transformData.path ?? currentActiveField ?? "",app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx (2)
118-141: Harden transform updates against null state (and keep path as string).
setPolylinePointTransforms(null)leaves this setter receivingprev === null, soprev[labelId]throws before optional chaining runs. At the same time, the path fallback still omits the empty-string guard that was previously requested. Reusing a normalizedbase/existingstructure avoids the crash and lets you reuse the same object for both the segment data and the string fallback.- setPolylinePointTransforms((prev) => { - const labelId = label._id; - const currentSegments = prev[labelId]?.segments || []; + setPolylinePointTransforms((prev) => { + const labelId = label._id; + const base = prev ?? {}; + const existing = base[labelId] ?? { segments: [] }; + const currentSegments = existing.segments ?? []; @@ - return { - ...prev, - [labelId]: { - segments: newSegments, - path: prev[labelId]?.path ?? currentActiveField, - sampleId: prev[labelId]?.sampleId ?? currentSampleId, - }, - }; + return { + ...base, + [labelId]: { + ...existing, + segments: newSegments, + path: existing.path ?? currentActiveField ?? "", + sampleId: existing.sampleId ?? currentSampleId, + }, + }; });
336-358: Fix null handling when inserting vertices (and keep path string-safe).Same crash vector exists here:
prevmay benull, andprev[labelId](plus the.pathaccess) will blow up. Please normalizeprevbefore reading and reuse that when building the return payload, while also applying the empty-string path fallback that was requested earlier.- setPolylinePointTransforms((prev) => { - const labelId = label._id; - const currentSegments = prev[labelId]?.segments || []; + setPolylinePointTransforms((prev) => { + const labelId = label._id; + const base = prev ?? {}; + const existing = base[labelId] ?? {}; + const currentSegments = existing.segments ?? []; @@ - return { - ...prev, - [labelId]: { - segments: newSegments, - path: prev[labelId].path ?? currentActiveField, - sampleId: prev[labelId].sampleId ?? currentSampleId, - }, - }; + return { + ...base, + [labelId]: { + ...existing, + segments: newSegments, + path: existing.path ?? currentActiveField ?? "", + sampleId: existing.sampleId ?? currentSampleId, + }, + }; });app/packages/looker-3d/src/labels/index.tsx (1)
303-323: Stop spreading transform misc onto Polyline propsThis reintroduces the same prop-leak we called out before: the result of
coerceStringBooleans(...)gets merged intooverlayCombined/overlayLabeland then spread straight into<Polyline />. React ends up seeing arbitrary keys likeclosedon the underlying<group>, which is exactly the console noise this PR is trying to remove. Keep the misc payload on the label metadata (e.g.label.misc) and pass the supported props (closed, etc.) explicitly when rendering the Polyline. Based on prior feedback.Apply something along these lines:
- let finalPoints3d = maybeExistingTransformData?.segments - ? maybeExistingTransformData.segments.map((seg) => seg.points) - : (overlay as PolyLineProps).points3d; - - if (finalPoints3d) { - finalPoints3d = finalPoints3d.filter(isValidPolylineSegment); - } - - const overlayCombined = { - ...overlay, - ...coerceStringBooleans(maybeExistingTransformData?.misc ?? {}), - points3d: finalPoints3d, - }; + let finalPoints3d = maybeExistingTransformData?.segments + ? maybeExistingTransformData.segments.map((seg) => seg.points) + : (overlay as PolyLineProps).points3d; + + if (finalPoints3d) { + finalPoints3d = finalPoints3d.filter(isValidPolylineSegment); + } + + const sanitizedMisc = coerceStringBooleans( + maybeExistingTransformData?.misc ?? {} + ); + const overlayLabelWithMisc = { + ...overlay, + points3d: finalPoints3d, + misc: sanitizedMisc, + }; + const isClosed = sanitizedMisc.closed === true; ... - newPolylineOverlays.push( - <Polyline - key={`polyline-${overlay._id ?? overlay.id}-${overlay.sampleId}`} - rotation={overlayRotation} - opacity={labelAlpha} - lineWidth={polylineWidth} - {...(overlayCombined as PolyLineProps)} - label={overlayCombined} - onClick={(e) => handleSelect(overlay, "polyline", e)} - tooltip={tooltip} - /> - ); + newPolylineOverlays.push( + <Polyline + key={`polyline-${overlay._id ?? overlay.id}-${overlay.sampleId}`} + rotation={overlayRotation} + opacity={labelAlpha} + lineWidth={polylineWidth} + {...(overlay as PolyLineProps)} + points3d={finalPoints3d} + closed={isClosed} + label={overlayLabelWithMisc} + onClick={(e) => handleSelect(overlay, "polyline", e)} + tooltip={tooltip} + /> + ); ... - const overlayLabel = { + const sanitizedMisc = coerceStringBooleans(transformData.misc ?? {}); + const isClosed = sanitizedMisc.closed === true; + const overlayLabel = { _id: labelId, _cls: "Polyline", type: "Polyline", path: transformData.path, label: transformData.label, selected: false, sampleId: currentSampleId, tags: [], points3d, - ...coerceStringBooleans(transformData.misc ?? {}), + misc: sanitizedMisc, }; + const { misc: _unused, ...polylineProps } = overlayLabel; ... - newPolylineOverlays.push( - <Polyline - key={`polyline-${labelId}-${transformData.sampleId}`} - rotation={overlayRotation} - opacity={labelAlpha} - lineWidth={polylineWidth} - {...(overlayLabel as unknown as PolyLineProps)} - label={overlayLabel} - onClick={(e) => handleSelect(overlayLabel, "polyline", e)} - tooltip={tooltip} - color={getLabelColor({ + newPolylineOverlays.push( + <Polyline + key={`polyline-${labelId}-${transformData.sampleId}`} + rotation={overlayRotation} + opacity={labelAlpha} + lineWidth={polylineWidth} + {...(polylineProps as unknown as PolyLineProps)} + closed={isClosed} + label={overlayLabel} + onClick={(e) => handleSelect(overlayLabel, "polyline", e)} + tooltip={tooltip} + color={getLabelColor({ coloring, path: transformData.path, isTagged: false, labelTagColors, customizeColorSetting, label: overlayLabel, embeddedDocType: overlayLabel._cls, })} /> );Also applies to: 355-386
🧹 Nitpick comments (16)
app/packages/core/src/components/Modal/Sidebar/Annotate/Icons.tsx (2)
5-5: Icon substitution addresses missing/unsuitable icon.The import change from
PolylineOutlinedtoTimelineOutlined(aliased for compatibility) combined with the 90-degree rotation at line 62 is a reasonable solution ifPolylineOutlineddoesn't exist or isn't suitable in MUI.Consider adding a brief comment explaining this substitution for maintainability:
+// MUI doesn't provide PolylineOutlined; using rotated TimelineOutlined instead TimelineOutlined as PolylineOutlined,
59-65: Console error fix looks good, but consider consistency.The change from
fillprop tosx={{ color: fill }}correctly addresses the console error, as MUI icons don't acceptfillas a prop.For consistency with the
LockingandShowncomponents (lines 73, 84) which use thestyleprop, consider:- <PolylineOutlined sx={{ transform: "rotate(90deg)", color: fill }} /> + <PolylineOutlined style={{ transform: "rotate(90deg)", color: fill }} />Both approaches work, but consistency improves maintainability.
app/packages/core/src/components/Actions/ToggleSidebar/index.tsx (1)
9-44: Consider adding a displayName for easier debugging.The forwardRef component lacks a displayName, which can make debugging in React DevTools less clear.
Apply this diff to add a displayName:
const ToggleSidebar = React.forwardRef< HTMLButtonElement, ActionProps & { modal: boolean; } >(({ modal, adaptiveMenuItemProps }, ref) => { const [visible, setVisible] = useRecoilState(sidebarVisible(modal)); return ( <PillButton onClick={() => { setVisible(!visible); }} title={`${visible ? "Hide" : "Show"} sidebar (s)`} tooltipPlacement={modal ? "bottom" : "top"} open={visible} icon={ visible ? ( modal ? ( <KeyboardArrowRight /> ) : ( <KeyboardArrowLeft /> ) ) : modal ? ( <KeyboardArrowLeft /> ) : ( <KeyboardArrowRight /> ) } highlight={!visible} ref={ref} data-cy="action-toggle-sidebar" {...(getStringAndNumberProps(adaptiveMenuItemProps) || {})} /> ); }); + +ToggleSidebar.displayName = "ToggleSidebar";app/packages/core/src/components/Modal/ModalNavigation.tsx (1)
125-152: Consider covering additional text-entry input types.The keyboard guards correctly prevent navigation when TEXTAREA or text INPUT elements are focused. However, the INPUT check only covers
type="text"and may miss other text-entry scenarios like email, tel, url, search, password, or number inputs where users might also be typing.If broader coverage is desired, consider:
// Prevent navigation when editing text in input fields if (active?.tagName === "INPUT") { - if ((active as HTMLInputElement).type === "text") { + const inputType = (active as HTMLInputElement).type; + if (["text", "email", "tel", "url", "search", "password", "number"].includes(inputType)) { return; } }Alternatively, if the current scope is intentional (e.g., to allow arrow navigation in other input types like checkboxes or range sliders), the implementation is appropriate as-is.
app/packages/core/src/components/Modal/Sample3d.tsx (1)
26-32: LGTM: Conditional logic correctly implements annotation mode support.The expanded condition properly handles both group media and annotation mode by bypassing the
SampleWrapperwhen appropriate. The boolean logic is sound and the implementation is correct.Consider adding a brief comment explaining why these modes render
Looker3ddirectly without the wrapper, to improve maintainability:+ {/* Groups and annotation mode render Looker3d directly without wrapper */} {isGroup || (canAnnotate && isInAnnotateMode) ? (app/packages/components/src/components/LabeledField/index.tsx (1)
10-18: Consider allowing ReactNode for description.The
labelprop acceptsReact.ReactNodewhiledescriptionis limited tostring. For consistency and flexibility, consider allowingReact.ReactNodefor the description as well.Apply this diff if you'd like to make the types consistent:
export const LabeledField = ({ formControl, label, description, }: { formControl: React.ReactNode; label?: React.ReactNode; - description?: string; + description?: React.ReactNode; }) => {app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx (1)
101-103: Consider validating origin prop structure before forwarding.The
menu_anchor_originandmenu_transform_originprops are forwarded directly from the schema view without validation. If the schema provides malformed values (e.g., incorrect vertical/horizontal values), they'll be passed to the underlying component, potentially causing console warnings or unexpected behavior.Consider adding validation:
+const validateOrigin = (origin) => { + if (!origin) return origin; + const validVertical = ["top", "bottom", "center"]; + const validHorizontal = ["left", "right", "center"]; + if (validVertical.includes(origin.vertical) && validHorizontal.includes(origin.horizontal)) { + return origin; + } + return undefined; +}; + return ( <Box {...getComponentProps(props, "container")}> <TooltipProvider title={title} {...getComponentProps(props, "tooltip")}> <OperatorExecutionButton operatorUri={operator} onSuccess={handleOnSuccess} onError={handleOnError} onOptionSelected={handleOnOptionSelected} executionParams={computedParams} variant={variant} disabled={disabled} insideModal={inside_modal} - menuAnchorOrigin={menu_anchor_origin} - menuTransformOrigin={menu_transform_origin} + menuAnchorOrigin={validateOrigin(menu_anchor_origin)} + menuTransformOrigin={validateOrigin(menu_transform_origin)} startIcon={icon_position === "left" ? Icon : undefined} endIcon={icon_position === "right" ? Icon : undefined} title={description} {...getComponentProps(props, "button", getButtonProps(props))} >app/packages/looker-3d/src/AnnotationTips.tsx (3)
36-104: Prefer theme colors over hardcoded values for consistency.Multiple styled components use hardcoded color values (#e0e0e0, #bdbdbd, #999, #ccc, #666, etc.) instead of leveraging the theme system. While
TipsContainercorrectly usestheme.primary.mainfor the border, other components should follow suit for maintainability and consistent theming.Consider extracting commonly used colors from the theme (e.g.,
theme.text.primary,theme.text.secondary, etc.) and applying them consistently across all styled components.
108-123: Consider platform-specific keyboard shortcuts.The tip content uses macOS-specific keyboard shortcuts (⌘) without considering Windows/Linux users who would use
Ctrlinstead. Consider detecting the user's platform and displaying the appropriate modifier key.Example approach:
const isMac = navigator.platform.toUpperCase().indexOf('MAC') >= 0; const modifierKey = isMac ? '⌘' : 'Ctrl';Then use
{modifierKey}in the tip text.
167-195: Add accessibility attributes for screen readers.The tips overlay lacks proper ARIA attributes, which could impact users relying on assistive technologies. Consider adding:
role="dialog"orrole="alert"to the containeraria-labeloraria-labelledbyto describe the dialogaria-describedbyto reference the tip content- Focus management when the overlay appears
Example enhancements:
return ( <TipsContainer $border={theme.primary.main} $text="#e0e0e0" $isMultiviewOn={isMultiviewOn} + role="dialog" + aria-label="3D Viewer Tips" + aria-describedby="tip-content" > <TipsHeader> <TipsTitle> <InfoOutlined style={{ fontSize: 16, color: theme.primary.main }} /> FiftyOne 3D Viewer Tips </TipsTitle> </TipsHeader> <TipsContent> - <TipsList> + <TipsList id="tip-content"> <TipsListItem>{currentTip.text}</TipsListItem> </TipsList> </TipsContent> <TipsActions> - <TipsButton $variant="primary" onClick={handleCancel}> + <TipsButton $variant="primary" onClick={handleCancel} aria-label="Dismiss tips"> Got it </TipsButton> - <TipsButton $variant="secondary" onClick={handleDoNotShowAgain}> + <TipsButton $variant="secondary" onClick={handleDoNotShowAgain} aria-label="Don't show tips again"> Don't show again </TipsButton> </TipsActions> </TipsContainer> );app/packages/core/src/utils/time.ts (2)
1-18: Documentation could clarify timeout behavior.The JSDoc is clear, but could be more precise: the fallback is returned when the function is called after the timeout has elapsed, not automatically when timeout expires. Additionally, once timeout occurs, the function continues returning the fallback value on subsequent calls until the predicate becomes true.
24-25: Remove unusedhasTimedOutvariable.The
hasTimedOutvariable is set on line 44 and reset on line 33, but it's never read or used in any conditional logic. This appears to be dead code.Apply this diff to remove the unused variable:
): () => boolean { let startTime: number | null = null; - let hasTimedOut = false; return () => {And remove the assignments:
// If predicate is true, reset and return true if (predicateResult) { startTime = null; - hasTimedOut = false; return true; }const elapsed = Date.now() - startTime; if (elapsed >= timeoutMs) { - hasTimedOut = true; return fallbackReturnValue; }app/packages/core/src/utils/time.test.ts (1)
136-158: Test comment is misleading.The test works correctly, but the comment on line 156 says "returns true (from fallback after timeout)". However, at call 4,
callCountbecomes 4, sopredicate()returns true (4 % 2 === 0), meaning the wrapper returns true because the predicate is true, not because of the timeout fallback.Consider updating the comment to:
- // Call 4: returns true (from fallback after timeout) + // Call 4: returns true (predicate returns true since callCount=4) expect(wrapper()).toBe(true);app/packages/core/src/components/Modal/Sidebar/Annotate/useFocus.ts (1)
73-80: Consider handling overlay selection when editing new labelsWhen editing a new label, the handler stores the target overlay id in
selectIdbut returns early without selecting it (line 75). This prevents users from switching overlays during new label creation, which may be intentional to avoid losing work. However:
- The stored
selectIdis never cleared if the user cancels the new label, leaving stale data- Users may find it confusing that clicking another overlay has no effect
Consider either:
- Clearing
selectId.currenton the early return to avoid stale data- Or prompting the user to save/discard the new label before switching overlays (similar to the existing label flow)
app/packages/core/src/plugins/SchemaIO/components/DropdownView.tsx (1)
202-203: Remove unreachable code.The
if (multiple)branch inrenderValuewill never execute because whenmultipleis true, the component returns early at line 164 with<AutocompleteView>. This is dead code.Apply this diff to remove the unreachable code:
} return placeholder; } - if (multiple) { - return value.map((item) => choiceLabels[item] || item).join(", "); - } return choiceLabels[value] || value; }}docs/source/release-notes.rst (1)
62-66: Prefer “parameter” over “kwarg” for user‑facing docsReplace colloquial “kwarg” with “parameter” and simplify phrasing.
- includes a kwarg for a batcher, which can be a + includes a parameter for a batcher, which can be a
| <Item | ||
| key={ | ||
| item.isSectionHeader | ||
| ? `section-${item.sectionKey}-${idx}` | ||
| : `{item.shortcut}-${idx}` | ||
| } | ||
| {...item} | ||
| /> |
There was a problem hiding this comment.
Fix React key interpolation for non-section help items.
The non-section branch is building the key as the literal string "{item.shortcut}-${idx}", so every row ends up with the same key. React will warn and reconciliation can break when keys collide. Switch this to a template literal so the shortcut and index are actually interpolated.
- : `{item.shortcut}-${idx}`
+ : `${item.shortcut}-${idx}`📝 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.
| <Item | |
| key={ | |
| item.isSectionHeader | |
| ? `section-${item.sectionKey}-${idx}` | |
| : `{item.shortcut}-${idx}` | |
| } | |
| {...item} | |
| /> | |
| <Item | |
| key={ | |
| item.isSectionHeader | |
| ? `section-${item.sectionKey}-${idx}` | |
| : `${item.shortcut}-${idx}` | |
| } | |
| {...item} | |
| /> |
🤖 Prompt for AI Agents
In app/packages/components/src/components/HelpPanel/HelpPanel.tsx around lines
81 to 88, the non-section key branch currently produces the literal string
"{item.shortcut}-${idx}" so keys collide; change that branch to use a JavaScript
template literal that interpolates item.shortcut and idx (e.g. backtick-based
`${item.shortcut}-${idx}`) so each key is unique and React reconciliation works
correctly.
| sx: { | ||
| // default dialog z-index is 1300 | ||
| zIndex: insideModal ? 2400 : undefined, | ||
| }, | ||
| MenuListProps: { | ||
| sx: { paddingY: 0, zIndex: 999 }, | ||
| sx: { | ||
| paddingY: 0, | ||
| zIndex: 999, | ||
| maxHeight: "400px", | ||
| width: hideActions ? "100%" : "270px", | ||
| overflowY: "auto", | ||
| background: (theme) => theme.background.level2, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the z-index value is appropriate for all modal contexts.
The z-index of 2400 for modal contexts should overlay standard MUI Dialogs (z-index 1300), but ensure this doesn't conflict with other high-z-index UI elements like tooltips, popovers, or nested modals.
🏁 Script executed:
#!/bin/bash
# Search for z-index values in the codebase to check for potential conflicts
rg -n --type=tsx --type=ts 'zIndex.*:.*\d{4}' -C 2Length of output: 27
I'll fix the script syntax and search for z-index values to verify potential conflicts:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for high z-index values (4 digits) in TypeScript and TSX files
rg -n 'zIndex\s*:\s*\d{4,}' -g '*.ts' -g '*.tsx' -C 2
Length of output: 7312
🏁 Script executed:
#!/bin/bash
# Also search for z-index references in the Selection component specifically
rg -n 'zIndex' app/packages/components/src/components/Selection/ -C 1Length of output: 1221
🏁 Script executed:
#!/bin/bash
# Search for MUI Dialog and Modal z-index related code
rg -n 'z-index.*1300|zIndex.*1300' -g '*.ts' -g '*.tsx'Length of output: 496
🏁 Script executed:
#!/bin/bash
# Check for tooltip, popover z-index values to identify hierarchy
rg -n '(tooltip|popover).*zIndex|zIndex.*(tooltip|popover)' -g '*.ts' -g '*.tsx' -iLength of output: 359
🏁 Script executed:
#!/bin/bash
# List all zIndex values found to establish a complete picture
rg -n 'zIndex\s*:\s*[^,}]+' -g '*.ts' -g '*.tsx' | head -30Length of output: 2980
🏁 Script executed:
#!/bin/bash
# Search for how Selection component is used within modal contexts
rg -n 'Selection' app/packages/core/src/components/Modal/ -C 2Length of output: 1201
🏁 Script executed:
#!/bin/bash
# Check if there are any Popper or Menu z-index related issues documented
rg -n 'MenuProps|Popper' app/packages/components/src/components/Selection/ -A 3 -B 1Length of output: 483
🏁 Script executed:
#!/bin/bash
# Check MUI theme z-index configuration in the codebase
rg -n 'zIndex\s*:\s*\{' app/packages/components/src/components/ThemeProvider/ -A 10Length of output: 1017
🏁 Script executed:
#!/bin/bash
# Search for comments or documentation about the 999 z-index value
rg -n '999|MenuListProps' app/packages/components/src/components/Selection/ -B 2 -A 2Length of output: 2758
🏁 Script executed:
#!/bin/bash
# Check if there are any git comments or TODOs related to z-index in Selection
rg -n 'TODO|FIXME|BUG|z-index' app/packages/components/src/components/Selection/Selection.tsx -B 1 -A 1Length of output: 427
🏁 Script executed:
#!/bin/bash
# Verify how MUI Select MenuProps work by checking if there are other similar patterns
rg -n 'MenuProps.*zIndex.*:.*\d+' -A 2 -B 2 -g '*.ts' -g '*.tsx'Length of output: 0
Fix z-index inconsistency in Selection component and typo in styledComponents.
The z-index value of 2400 for MenuProps correctly overlays MUI Dialogs (z-index 1300), but MenuListProps uses 999, creating an inverted hierarchy. When insideModal=true, the dropdown list content should have a z-index at least as high as its parent to ensure it remains visible above the modal.
Additionally, there's a CSS property typo in styledComponents.ts line 88: zindex should be zIndex.
Required fixes:
Selection.tsxline 112: ChangeMenuListPropsz-index from 999 to at least 2400 (or use conditional like parent)styledComponents.tsline 88: Correctzindex: 9999;tozIndex: 9999;
🤖 Prompt for AI Agents
In app/packages/components/src/components/Selection/Selection.tsx around lines
105-117, the MenuListProps currently sets zIndex: 999 which conflicts with the
parent MenuProps zIndex of 2400 when insideModal is true; change
MenuListProps.sx.zIndex to match the parent (e.g., set to 2400 or conditionally
use insideModal ? 2400 : undefined) so the dropdown list remains above MUI
Dialogs. In app/packages/components/src/components/Selection/styledComponents.ts
line 88, fix the CSS typo by replacing the non-standard `zindex: 9999;` with the
camelCase React style `zIndex: 9999;`.
| <MenuItem | ||
| key="create-view-option" | ||
| value="create-view-option" | ||
| label="" | ||
| sx={{ padding: 0 }} | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| }} | ||
| > | ||
| {lastFixedOption} | ||
| </LastOption> | ||
| <LastOption>{lastFixedOption}</LastOption> | ||
| </MenuItem> |
There was a problem hiding this comment.
Consider potential issue with fixed MenuItem value.
The lastFixedOption is wrapped in a MenuItem with a hardcoded value="create-view-option". While e.preventDefault() is called, MUI Select might still allow this value to be selected through keyboard navigation or other interactions, which could cause the Select to display an unexpected value.
Consider one of these approaches:
- Use
disableListItemprop if available, or add pointer-events styling:
<MenuItem
key="create-view-option"
value="create-view-option"
- sx={{ padding: 0 }}
+ sx={{ padding: 0, pointerEvents: 'none' }}
onClick={(e) => {
e.preventDefault();
}}
>
<LastOption>{lastFixedOption}</LastOption>
</MenuItem>- Or use a non-MenuItem wrapper if this is truly a non-selectable action:
-<MenuItem
- key="create-view-option"
- value="create-view-option"
- sx={{ padding: 0 }}
- onClick={(e) => {
- e.preventDefault();
- }}
->
- <LastOption>{lastFixedOption}</LastOption>
-</MenuItem>
+<ListSubheader sx={{ padding: 0, position: 'sticky', bottom: 0 }}>
+ <LastOption>{lastFixedOption}</LastOption>
+</ListSubheader>📝 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.
| <MenuItem | |
| key="create-view-option" | |
| value="create-view-option" | |
| label="" | |
| sx={{ padding: 0 }} | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| }} | |
| > | |
| {lastFixedOption} | |
| </LastOption> | |
| <LastOption>{lastFixedOption}</LastOption> | |
| </MenuItem> | |
| <MenuItem | |
| key="create-view-option" | |
| value="create-view-option" | |
| sx={{ padding: 0, pointerEvents: 'none' }} | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| }} | |
| > | |
| <LastOption>{lastFixedOption}</LastOption> | |
| </MenuItem> |
🤖 Prompt for AI Agents
In app/packages/components/src/components/Selection/Selection.tsx around lines
233-242, the MenuItem for lastFixedOption has a hardcoded value
("create-view-option") which can still be selected via keyboard; make this item
non-selectable by either replacing the MenuItem with a non-selectable wrapper
(e.g., Box or ListSubheader) so it is not part of the Select options, or if you
must keep MenuItem, remove the value prop and mark it disabled and
non-interactive (set disabled, sx={{ pointerEvents: 'none' }}, tabIndex={-1} and
aria-hidden="true") so it cannot be focused or chosen.
| useEffect(() => { | ||
| // sceneId should be deterministic, but unique for a given sample snapshot | ||
| const sample = sampleRef.current; | ||
| setSceneId( | ||
| `${sample?.sample?._id}-${sample?.sample?.last_modified_at?.datetime}` | ||
| ); | ||
| }, [sample]); | ||
| }, []); |
There was a problem hiding this comment.
Critical: sceneId never updates when sample changes.
The empty dependency array means sceneId is set only once on mount. If the sample prop changes to represent a different sample, the sceneId will remain stale. This creates a mismatch between the scene identifier and the actual sample data, which could lead to:
- Wrong scene being displayed
- Stale scene references
- Data inconsistency bugs
If the intent is to keep the scene stable across sample updates for the same logical sample, consider adding dependencies for the ID fields:
useEffect(() => {
// sceneId should be deterministic, but unique for a given sample snapshot
const sample = sampleRef.current;
setSceneId(
`${sample?.sample?._id}-${sample?.sample?.last_modified_at?.datetime}`
);
- }, []);
+ }, [sample?.sample?._id, sample?.sample?.last_modified_at?.datetime]);If you want the component to be stable across sample navigation, you'll need a different architecture (e.g., key prop to force remount).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx
around lines 80 to 86, the useEffect that sets sceneId runs only on mount (empty
dependency array) so sceneId stays stale when the sample changes; update the
effect to depend on the sample identity/timestamp (e.g.,
sampleRef.current.sample._id and
sampleRef.current.sample.last_modified_at?.datetime or the prop(s) that feed
sampleRef) so sceneId is recalculated whenever the sample or its
last_modified_at changes; alternatively, if you intend to force a full remount
on sample change, move the sceneId logic into a stable key on the component
consumer instead.
| if (annotationSupported && (loading || !scene)) { | ||
| return <Loading />; | ||
| } |
There was a problem hiding this comment.
Restore 3D annotate loading behavior.
annotationSupported is true for 3D, but those datasets never get a lighterSceneAtom instance, so this guard now keeps the sidebar in an endless spinner. We still need to bypass the scene requirement when mediaType === "3d" like before.
Apply this diff:
- if (annotationSupported && (loading || !scene)) {
+ if (
+ annotationSupported &&
+ (loading || (!scene && mediaType !== "3d"))
+ ) {📝 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 (annotationSupported && (loading || !scene)) { | |
| return <Loading />; | |
| } | |
| if ( | |
| annotationSupported && | |
| (loading || (!scene && mediaType !== "3d")) | |
| ) { | |
| return <Loading />; | |
| } |
🤖 Prompt for AI Agents
In app/packages/core/src/components/Modal/Sidebar/Annotate/Annotate.tsx around
lines 105 to 107, the current guard returns Loading whenever annotationSupported
is true and scene is missing, which leaves 3D datasets stuck because they have
annotationSupported true but no lighterSceneAtom; update the condition to bypass
the scene requirement for 3D media by only requiring scene when mediaType !==
"3d" (i.e., change the check to return Loading if annotationSupported &&
(loading || (!scene && mediaType !== "3d"))), ensuring you reference the
existing mediaType variable in the component.
| {mode === "annotate" && isPolylineAnnotateActive && <AnnotationToolbar />} | ||
| </Fo3dSceneContext.Provider> |
There was a problem hiding this comment.
Don’t hide the annotation toolbar by default
Requiring isPolylineAnnotateActive here means annotate mode renders with no toolbar until the polyline flag is already true—users lose the controls they need to activate polyline mode (and any other tools). Drop this extra condition so the toolbar is always available when the modal is in annotate mode.
Apply this diff:
- {mode === "annotate" && isPolylineAnnotateActive && <AnnotationToolbar />}
+ {mode === "annotate" && <AnnotationToolbar />}📝 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.
| {mode === "annotate" && isPolylineAnnotateActive && <AnnotationToolbar />} | |
| </Fo3dSceneContext.Provider> | |
| {mode === "annotate" && <AnnotationToolbar />} | |
| </Fo3dSceneContext.Provider> |
🤖 Prompt for AI Agents
In app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx around lines 940 to 941,
the AnnotationToolbar is currently gated by both mode === "annotate" and
isPolylineAnnotateActive, which hides the toolbar until a polyline flag is true;
remove the isPolylineAnnotateActive condition so the toolbar renders whenever
mode === "annotate". Update the JSX to only check mode === "annotate" (i.e.,
render <AnnotationToolbar /> when annotate mode is active) and ensure no other
logic relies on the removed condition.
| useEffect(() => { | ||
| if (isSelectedForAnnotation) { | ||
| setIsPolylineAnnotateActive(true); | ||
| } | ||
| }, [isSelectedForAnnotation]); |
There was a problem hiding this comment.
Reset annotate-active flag when deselected.
The effect only ever sets the atom to true. Once a label is selected, the flag stays latched even after deselection, so other UI that relies on isPolylineAnnotateActive never turns off. Please reflect the current selection each time and include the setter in the dependency list.
- useEffect(() => {
- if (isSelectedForAnnotation) {
- setIsPolylineAnnotateActive(true);
- }
- }, [isSelectedForAnnotation]);
+ useEffect(() => {
+ setIsPolylineAnnotateActive(isSelectedForAnnotation);
+ }, [isSelectedForAnnotation, setIsPolylineAnnotateActive]);🤖 Prompt for AI Agents
In app/packages/looker-3d/src/labels/polyline.tsx around lines 59 to 63, the
useEffect currently only sets setIsPolylineAnnotateActive(true) when selected,
causing the flag to remain latched after deselection; change the effect to set
the atom to the current isSelectedForAnnotation value (e.g.,
setIsPolylineAnnotateActive(isSelectedForAnnotation)) so it turns off when
deselected, and include setIsPolylineAnnotateActive in the dependency array.
| subscribe: ( | ||
| key: string, | ||
| callback: (value: T) => void, | ||
| initialValue: T | ||
| ): (() => void) | undefined => { | ||
| if ( | ||
| typeof window === "undefined" || | ||
| typeof window.addEventListener === "undefined" | ||
| ) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const handler = (e: StorageEvent) => { | ||
| const datasetName = getDatasetName(); | ||
| const prefixedKey = datasetName ? `${datasetName}_${key}` : key; | ||
|
|
||
| if (e.storageArea === localStorage && e.key === prefixedKey) { | ||
| let newValue: T; | ||
| try { | ||
| newValue = e.newValue ? JSON.parse(e.newValue) : initialValue; | ||
| } catch { | ||
| newValue = initialValue; | ||
| } | ||
| callback(newValue); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("storage", handler); | ||
| return () => window.removeEventListener("storage", handler); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Unsafe localStorage reference in storage event handler.
Line 61 directly references the global localStorage which can throw in restricted contexts even when window exists. Use window.localStorage consistently and guard the comparison.
Apply this diff:
const handler = (e: StorageEvent) => {
const datasetName = getDatasetName();
const prefixedKey = datasetName ? `${datasetName}_${key}` : key;
- if (e.storageArea === localStorage && e.key === prefixedKey) {
+ if (e.storageArea === window.localStorage && e.key === prefixedKey) {
let newValue: T;
try {
newValue = e.newValue ? JSON.parse(e.newValue) : initialValue;
} catch {
newValue = initialValue;
}
callback(newValue);
}
};📝 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.
| subscribe: ( | |
| key: string, | |
| callback: (value: T) => void, | |
| initialValue: T | |
| ): (() => void) | undefined => { | |
| if ( | |
| typeof window === "undefined" || | |
| typeof window.addEventListener === "undefined" | |
| ) { | |
| return undefined; | |
| } | |
| const handler = (e: StorageEvent) => { | |
| const datasetName = getDatasetName(); | |
| const prefixedKey = datasetName ? `${datasetName}_${key}` : key; | |
| if (e.storageArea === localStorage && e.key === prefixedKey) { | |
| let newValue: T; | |
| try { | |
| newValue = e.newValue ? JSON.parse(e.newValue) : initialValue; | |
| } catch { | |
| newValue = initialValue; | |
| } | |
| callback(newValue); | |
| } | |
| }; | |
| window.addEventListener("storage", handler); | |
| return () => window.removeEventListener("storage", handler); | |
| }, | |
| }; | |
| subscribe: ( | |
| key: string, | |
| callback: (value: T) => void, | |
| initialValue: T | |
| ): (() => void) | undefined => { | |
| if ( | |
| typeof window === "undefined" || | |
| typeof window.addEventListener === "undefined" | |
| ) { | |
| return undefined; | |
| } | |
| const handler = (e: StorageEvent) => { | |
| const datasetName = getDatasetName(); | |
| const prefixedKey = datasetName ? `${datasetName}_${key}` : key; | |
| if (e.storageArea === window.localStorage && e.key === prefixedKey) { | |
| let newValue: T; | |
| try { | |
| newValue = e.newValue ? JSON.parse(e.newValue) : initialValue; | |
| } catch { | |
| newValue = initialValue; | |
| } | |
| callback(newValue); | |
| } | |
| }; | |
| window.addEventListener("storage", handler); | |
| return () => window.removeEventListener("storage", handler); | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In app/packages/state/src/jotai/modal.ts around lines 45 to 75, the storage
event handler references the global localStorage directly which can throw in
restricted environments; update the handler to use window.localStorage
consistently and guard the comparison by checking that window.localStorage is
defined before comparing e.storageArea to it (e.g., get a localStorageRef =
typeof window.localStorage !== "undefined" ? window.localStorage : null and only
perform the e.storageArea === localStorageRef check when localStorageRef is
non-null).
| - Increased maximum length for the name of a dataset or view to over 1000 | ||
| characters. | ||
| - Fixed a bug where the UI would allow exporting media greater than 100 MB, even | ||
| though that is not supported, resulting in the download hanging. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Resolve inconsistency on name length limit and avoid duplication in Enterprise notes
Enterprise says “over 1000” while OS says “1551”. Either align Enterprise to “1551” or drop the Enterprise bullet to avoid duplicating an OS change under “Includes all updates from...”. Suggest:
- If unique Enterprise behavior: specify exactly what differs (e.g., UI limit vs backend limit) and keep the exact number.
- If not unique: remove the Enterprise bullet.
Also applies to: 67-69
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Extract the exact content of release-notes.rst to verify the inconsistency claim
cat docs/source/release-notes.rstLength of output: 327678
Resolve duplicated/inconsistent dataset-name length in release notes
Enterprise 2.13.0 (App) states "over 1000" while FiftyOne 1.10.0 (Core) states the limit was increased "from 100 to 1551" — either change the Enterprise bullet to "1551" if it's the same change, or remove the Enterprise bullet (it's redundant because Enterprise "Includes all updates from FiftyOne 1.10.0"). File: docs/source/release-notes.rst (Enterprise 2.13.0 App section and FiftyOne 1.10.0 Core section).
🤖 Prompt for AI Agents
In docs/source/release-notes.rst around lines 27 to 31, the Enterprise 2.13.0
(App) bullet claims "over 1000" for dataset/view name length which conflicts
with the FiftyOne 1.10.0 (Core) note saying it increased "from 100 to 1551";
update the Enterprise bullet to match the Core change by replacing "over 1000"
with "1551" (or remove the Enterprise bullet entirely if you prefer to avoid
redundancy since Enterprise includes all Core updates) so the two sections are
consistent.
| FiftyOne 1.10.0 | ||
| -------------- | ||
| *Released October 31, 2025* | ||
|
|
||
| Model Zoo | ||
|
|
||
| - Added `FiftyOneTransformerForPoseEstimation` to support transformer-based | ||
| keypoint prediction models ( | ||
| `ViTPose <https://huggingface.co/docs/transformers/model_doc/vitpose#vitpose>`_ | ||
| ) to the fiftyone model zoo. | ||
| `#6371 <https://github.com/voxel51/fiftyone/pull/6371>`_ | ||
|
|
There was a problem hiding this comment.
Capitalize “FiftyOne Model Zoo” and tighten phrasing
Use consistent branding and tidy punctuation. Proposed edit:
- Added `FiftyOneTransformerForPoseEstimation` to support transformer-based
- keypoint prediction models (
- `ViTPose <https://huggingface.co/docs/transformers/model_doc/vitpose#vitpose>`_
- ) to the fiftyone model zoo.
+ Added `FiftyOneTransformerForPoseEstimation` to support transformer-based
+ keypoint prediction models (e.g.,
+ `ViTPose <https://huggingface.co/docs/transformers/model_doc/vitpose#vitpose>`_)
+ in the FiftyOne Model Zoo.📝 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.
| FiftyOne 1.10.0 | |
| -------------- | |
| *Released October 31, 2025* | |
| Model Zoo | |
| - Added `FiftyOneTransformerForPoseEstimation` to support transformer-based | |
| keypoint prediction models ( | |
| `ViTPose <https://huggingface.co/docs/transformers/model_doc/vitpose#vitpose>`_ | |
| ) to the fiftyone model zoo. | |
| `#6371 <https://github.com/voxel51/fiftyone/pull/6371>`_ | |
| FiftyOne 1.10.0 | |
| -------------- | |
| *Released October 31, 2025* | |
| Model Zoo | |
| - Added `FiftyOneTransformerForPoseEstimation` to support transformer-based | |
| keypoint prediction models (e.g., | |
| `ViTPose <https://huggingface.co/docs/transformers/model_doc/vitpose#vitpose>`_) | |
| in the FiftyOne Model Zoo. | |
| `#6371 <https://github.com/voxel51/fiftyone/pull/6371>`_ |
🤖 Prompt for AI Agents
In docs/source/release-notes.rst around lines 48 to 59, the section heading and
phrasing need tightening: change the section title from "Model Zoo" to "FiftyOne
Model Zoo" (capitalized as requested), reword the bullet to a single concise
sentence such as "Added FiftyOneTransformerForPoseEstimation to the FiftyOne
Model Zoo to support transformer-based keypoint prediction models (ViTPose)."
ensure the ViTPose URL remains as the inline reference and the PR link (`#6371`)
stays at the end of the bullet with correct punctuation and balanced parentheses
and periods.
What changes are proposed in this pull request?
Updated code to address errors logged to the dev console.
How is this patch tested? If it is not, please explain why.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests