Merge release/v1.1.0 to develop
#5138
Conversation
extend compatibility of array deserialization in lookers
* fix false positives in toast * cleanup * schema bugfix * use suspense and events * add grid handling, cleanup * yolo * fetch cleanup * lint --------- Co-authored-by: Benjamin Kane <ben@voxel51.com>
[docs] SMTP config
WalkthroughThe pull request introduces several enhancements across multiple components in the application. Key changes include the addition of a new Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (46)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx (1)
Remove unused props from EvaluateProps type definition
Looking at the implementation, the component only uses
onEvaluateprop, whilevariantandpermissionsare defined in the type but never used. This matches the original review comment's observation.type EvaluateProps = { - variant: "empty" | "overview"; onEvaluate: () => void; - permissions: Record<string, boolean>; };🔗 Analysis chain
Line range hint
13-17: Clean up unused type propertiesThe type definition includes properties that aren't used in the implementation:
variantprop is mentioned in AI summary as removedpermissionsprop is defined but not usedIf these props are truly unused, apply this diff:
type EvaluateProps = { - variant: "empty" | "overview"; onEvaluate: () => void; - permissions: Record<string, boolean>; };Let's verify if these props are used elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of Evaluate component with variant or permissions props rg -A 5 '<Evaluate.*(?:variant|permissions)=' --type ts --type tsxLength of output: 96
Script:
#!/bin/bash # Search for usage of Evaluate component with variant or permissions props, fixing the file type rg -A 5 '<Evaluate.*(?:variant|permissions)=' -t typescript -t tsx # Let's also check the implementation of the Evaluate component rg -A 10 '^function Evaluate|^const Evaluate' -t typescript -t tsx # Search for any usage of the Evaluate component rg -A 5 '<Evaluate' -t typescript -t tsx # Let's also check the file content to understand the implementation cat "app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx"Length of output: 849
app/packages/embeddings/src/useComputeVisualization.ts (2)
1-3: Add TypeScript type annotations for imported hooksThe imported hooks lack TypeScript type definitions, which could lead to type-safety issues.
Consider adding type annotations:
-import { useFirstExistingUri, usePanelEvent } from "@fiftyone/operators"; -import { usePanelId } from "@fiftyone/spaces"; +import type { UseFirstExistingUriResult, PanelEventHandler } from "@fiftyone/operators"; +import { useFirstExistingUri, usePanelEvent } from "@fiftyone/operators"; +import type { PanelId } from "@fiftyone/spaces"; +import { usePanelId } from "@fiftyone/spaces";
Line range hint
7-24: Add TypeScript return type and improve type safetyThe hook lacks return type annotation and type safety for event parameters.
Consider adding types:
-export default function useComputeVisualization() { +interface ComputeVisualizationHook { + isAvailable: boolean; + prompt: () => void; +} + +interface PanelEventParams { + delegate: boolean; + operator: string; + prompt: boolean; +} + +export default function useComputeVisualization(): ComputeVisualizationHook {app/packages/core/src/components/Filters/use-query-performance-timeout.ts (2)
6-9: Add explicit return type and improve type safetyConsider adding explicit TypeScript return type and interface for the parameters.
+interface QueryPerformanceTimeoutProps { + modal: boolean; + path: string; +} -export default function useQueryPerformanceTimeout( - modal: boolean, - path: string -) { +export default function useQueryPerformanceTimeout({ + modal, + path +}: QueryPerformanceTimeoutProps): void {
16-20: Consider abstracting event dispatch logicThe direct use of
window.dispatchEventcould be abstracted into a separate utility function for better testability and maintenance.Consider creating an event service:
// services/events.ts export const dispatchQueryPerformanceEvent = ( path: string, isFrameField: boolean ): void => { window.dispatchEvent( new QueryPerformanceToastEvent(path, isFrameField) ); };app/packages/core/src/components/Filters/StringFilter/useOnSelect.ts (1)
Line range hint
7-33: Consider improving type safety and documentationThe function would benefit from explicit TypeScript return type definitions and documentation of its behavior.
Consider applying these improvements:
+/** + * Hook to handle selection in StringFilter + * @param modal - Whether the filter is in modal mode + * @param path - The field path + * @param selectedAtom - Recoil atom for selected values + * @returns A callback that handles selection and returns a Promise<string> + */ export default function ( modal: boolean, path: string, selectedAtom: RecoilState<(string | null)[]> -) { +): (value: string | null, d?: Result) => Promise<string> { return useRecoilCallback( ({ snapshot, set }) => async (value: string | null, d?: Result) => {app/packages/components/src/components/MuiButton/index.tsx (1)
12-24: Consider memoizing theme-dependent stylesThe style objects are recreated on every render. Consider using
useMemofor better performance, especially for the theme-dependent styles.const theme = useTheme(); - const containedStyles = - variant === "contained" ? { sx: { color: "white" } } : {}; - const outlinedStyles = - variant === "outlined" - ? { - sx: { - color: theme.palette.text.secondary, - borderColor: theme.palette.text.secondary, - }, - } - : {}; + const outlinedStyles = React.useMemo( + () => + variant === "outlined" + ? { + sx: { + color: theme.palette.text.secondary, + borderColor: theme.palette.text.secondary, + }, + } + : {}, + [theme.palette.text.secondary, variant] + ); + + const containedStyles = variant === "contained" ? { sx: { color: "white" } } : {};app/packages/embeddings/src/styled-components.tsx (1)
23-24: Layout changes look good, but there's a duplicate property.The added layout properties improve the spacing and width of the selectors. However, there's a duplicate
display: flexdeclaration in the component (one at the top and another a few lines down).Consider removing the duplicate property:
export const Selectors = styled.div` display: flex; gap: 1rem; position: absolute; top: 1rem; - display: flex; column-gap: 1rem; z-index: 1; padding: 0 1rem; > div { display: flex; column-gap: 1rem; } justify-content: space-between; width: 100%; `;app/packages/embeddings/src/EmbeddingsCTA.tsx (3)
5-10: Consider memoizing the Actions componentSince this component is passed as a prop to PanelCTA (line 22), memoizing it could prevent unnecessary re-renders.
-export function Actions(props: ActionsProps) { +export const Actions = React.memo(function Actions(props: ActionsProps) { const computeViz = useComputeVisualization(); const defaultHandler = () => computeViz.prompt(); const { handler = defaultHandler } = props; return <ComputeVisualizationButton onClick={handler} />; -} +});
12-28: Consider externalizing static contentThe component contains hardcoded strings and URLs that should be externalized for better maintainability and internationalization support.
Consider creating a constants file:
// constants.ts export const EMBEDDINGS_CTA = { LABEL: "Embeddings help you explore and understand your dataset", DEMO_LABEL: "Upgrade to Fiftyone Teams to Create Embeddings", DESCRIPTION: "You can compute and visualize embeddings for your dataset using a selection of pre-trained models or your own embeddings", DOC_LINK: "https://docs.voxel51.com/user_guide/app.html#embeddings-panel", DOC_CAPTION: "Not ready to upgrade yet? Learn how to create embeddings visualizations via code." };Then update the component to use these constants.
35-37: Consider adding return type for handler functionThe handler function type could be more explicit about its return type.
type ActionsProps = { - handler?: () => void; + handler?: () => Promise<void> | void; };app/packages/components/src/components/index.ts (1)
42-42: Maintain alphabetical ordering of exportsThe
PanelCTAexport should be moved to maintain alphabetical ordering, placing it betweenPendingandPillButtonexports.Apply this diff to fix the ordering:
export { default as Pending } from "./Pending"; +export { default as PanelCTA } from "./PanelCTA"; export { default as PillButton } from "./PillButton"; export { default as Popout, PopoutDiv } from "./Popout"; export { default as PopoutButton } from "./PopoutButton"; export { default as PopoutSectionTitle } from "./PopoutSectionTitle"; export { default as Resizable } from "./Resizable"; export * from "./Selection"; export { default as Selection } from "./Selection"; export { default as Selector, SelectorValidationError } from "./Selector"; export { default as StatusButton } from "./StatusButton"; export type { UseSearch } from "./Selector"; export { default as TabOption } from "./TabOption"; export { default as TextField } from "./TextField"; export { default as ThemeProvider, useFont, useTheme } from "./ThemeProvider"; export { default as Tooltip } from "./Tooltip"; export { default as Toast } from "./Toast"; -export { default as PanelCTA } from "./PanelCTA"; export * from "./types";app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx (2)
67-67: Good enhancement of loading state managementThe Suspense fallback now includes performance monitoring while maintaining the same visual feedback. This is a good architectural pattern for consistent loading state management across filters.
Consider documenting the performance monitoring strategy in a central location to ensure consistent implementation across other components.
90-93: Consider these enhancements to the Loading componentThe component has a good foundation, but could benefit from these improvements:
- Extract the props interface for reusability
- Add error boundary or error handling for the performance timeout
- Make the loading text configurable
Here's a suggested implementation:
+interface LoadingProps { + modal: boolean; + path: string; + text?: string; +} -const Loading = ({ modal, path }: { modal: boolean; path: string }) => { +const Loading = ({ modal, path, text = "Loading" }: LoadingProps) => { try { useQueryPerformanceTimeout(modal, path); - return <Box text="Loading" />; + return <Box text={text} />; } catch (error) { console.error('Performance timeout error:', error); return <Box text="Error loading content" />; } };app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationNotes.tsx (3)
8-8: Document the rationale for increasing MAX_NOTES_HEIGHTThe maximum height has been increased from 72px to 128px. While this change improves content visibility, please add a comment explaining the reasoning behind this specific value to help future maintainers understand the decision.
+// Increased to 128px to accommodate approximately 6-7 lines of text while maintaining +// a compact view that doesn't overwhelm the UI const MAX_NOTES_HEIGHT = 128;
38-39: Consider using a color mapping object for better maintainabilityWhile the current logic is clear, it could be made more maintainable using a color mapping object.
- const isOverview = variant === "overview"; - const color = hasNotes ? (isOverview ? "secondary" : "primary") : "tertiary"; + const colorMap = { + noNotes: "tertiary" as const, + overview: "secondary" as const, + details: "primary" as const, + }; + const color = hasNotes ? colorMap[variant] : colorMap.noNotes;
Line range hint
57-67: Extract gradient calculation for better readabilityThe background gradient calculation makes the styles harder to read. Consider extracting this logic into a separate function or using theme mixins.
+const getGradientBackground = (theme) => { + const baseColor = theme.palette.mode === "dark" ? "15%" : "100%"; + return `linear-gradient(to bottom, hsla(200, 0%, ${baseColor}, 0.9) 10%, hsl(200, 0%, ${baseColor}) 100%)`; +}; <Box sx={{ - background: `linear-gradient(to bottom, hsla(200, 0%, ${ - theme.palette.mode === "dark" ? "15%" : "100%" - }, 0.9) 10%, hsl(200, 0%, ${ - theme.palette.mode === "dark" ? "15%" : "100%" - }) 100%)`, + background: getGradientBackground(theme), position: "relative", top: expanded ? 0 : "-12px", p: 0.5, }} >app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx (2)
87-108: Simplify conditional rendering logicThe nested conditional rendering with multiple conditions could be simplified for better maintainability.
Consider extracting the rendering logic into separate components or using early returns:
const renderContent = () => { if (page !== 'overview') return null; if (showEmptyOverview || showCTA) { return <PanelCTA {...panelProps} />; } return <Overview {...overviewProps} />; };
89-94: Extract hardcoded strings into constantsConsider moving the hardcoded strings into a constants file or using a localization system for better maintainability and potential internationalization.
Example implementation:
const MODEL_EVALUATION_STRINGS = { CREATE_FIRST: "Create you first model evaluation", UPGRADE_LABEL: "Upgrade to Fiftyone Teams to Evaluate Models", DESCRIPTION: "Analyze and improve models collaboratively with your team", DOC_LINK: "https://docs.voxel51.com/user_guide/evaluation.html", DOC_CAPTION: "Not ready to upgrade yet? Learn how to create model evaluation via code." } as const;app/packages/operators/src/CustomPanel.tsx (2)
Line range hint
23-91: Consider performance optimizationsThe CustomPanel component could benefit from the following improvements:
- Memoize the component using React.memo to prevent unnecessary re-renders
- Extract DimensionRefresher to a separate file for better code organization
- Consider using useCallback for event handlers
Example implementation:
import { memo, useCallback } from 'react'; export const CustomPanel = memo((props: CustomPanelProps) => { // ... existing code ... const handlePanelStateChangeCallback = useCallback( (value) => handlePanelStateChange(value), [handlePanelStateChange] ); // ... rest of the component });
Line range hint
39-45: Fix useEffect dependenciesThe useEffect hook's dependency array is empty but uses
setPanelCloseEffectandtrackEventfrom outside the effect. This could lead to stale closures.Update the dependencies:
useEffect(() => { setPanelCloseEffect(() => { clearUseKeyStores(panelId); trackEvent("close_panel", { panel: panelName }); }); - }, []); + }, [setPanelCloseEffect, trackEvent, panelId, panelName]);app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx (1)
150-155: Enhance type safety and readability of the HOC.While the implementation is functional, consider these improvements:
- Add explicit return type annotation
- Use React.FC for better type safety
- Consider a more descriptive name that reflects its purpose
-const withQueryPerformanceTimeout = (modal: boolean, path: string) => { - return ({ children }: React.PropsWithChildren) => { +const withQueryPerformanceTimeout = (modal: boolean, path: string): React.FC<React.PropsWithChildren> => { + return function QueryPerformanceTimeoutWrapper({ children }: React.PropsWithChildren) { useQueryPerformanceTimeout(modal, path); return <>{children}</>; }; };app/packages/core/src/components/QueryPerformanceToast.tsx (4)
13-13: Document the significance of the QP_WAIT constant value.The magic number 5151 should be documented to explain its significance and why this specific duration was chosen.
-export const QP_WAIT = 5151; +// Duration (in ms) to wait before showing the query performance toast +// to avoid overwhelming users with immediate notifications +export const QP_WAIT = 5151;
72-74: Consider more graceful error handling.Instead of throwing an error when the element is not found, consider rendering a fallback or logging the error.
- if (!element) { - throw new Error("no query performance element"); - } + if (!element) { + console.error("Query performance element not found in DOM"); + return null; + }
Line range hint
91-100: Add ARIA labels to improve accessibility.The buttons lack proper ARIA labels for screen readers.
<Button variant="contained" size="small" + aria-label="View query performance documentation" onClick={() => { onClick(data.isFrameField); trackEvent("query_performance_toast_clicked", { path: data.path, }); setData(null); }} > <Button data-cy="btn-dismiss-query-performance-toast" variant="text" color="secondary" size="small" + aria-label="Dismiss query performance notification" onClick={() => { setDisabled(true); trackEvent("query_performance_toast_dismissed", { path: data.path, }); setData(null); }} >Also applies to: 119-126
128-129: Use sx prop for consistent styling.Replace inline style with sx prop for consistency with Material-UI's styling approach.
- // Right align the button - style={{ marginLeft: "auto", color: theme.text.secondary }} + sx={{ + marginLeft: "auto", + color: theme.text.secondary + }}app/packages/core/src/components/Grid/Grid.tsx (1)
126-133: Consider using useRef for timeout trackingWhile the current implementation works, using a useRef to track the timeout would make it easier to access it across different effect cleanup functions and prevent potential stale closure issues.
+ const timeoutRef = useRef<NodeJS.Timeout>(); useLayoutEffect(() => { const info = fos.getQueryPerformancePath(); - const timeout = setTimeout(() => { + timeoutRef.current = setTimeout(() => { if (info) { window.dispatchEvent( new QueryPerformanceToastEvent(info.path, info.isFrameField) ); } }, QP_WAIT);app/packages/components/src/components/PanelCTA/index.tsx (3)
75-81: Consider extracting nested Stack componentsThe component has multiple nested Stack components with similar styling properties. This could be extracted into a separate styled component for better maintainability.
Consider creating a styled component:
const ContentStack = styled(Stack)(({ theme }) => ({ height: '100%', alignItems: 'center', justifyContent: 'center' }));Also applies to: 65-74
152-164: Enhance type safety of TypographyOrNode helperThe function handles different types of children well, but could benefit from more specific typing.
Consider this enhancement:
-function TypographyOrNode(props: TypographyProps) { +function TypographyOrNode(props: TypographyProps & { + children: string | React.ReactElement | null; +}) { const { children, ...otherProps } = props;
166-182: Enhance type definitions with JSDoc comments and specific typesThe type definitions would benefit from documentation and more specific typing.
Consider these enhancements:
/** * Props for the PanelCTA component */ export type PanelCTAProps = { /** Optional component to render custom actions */ Actions?: FunctionComponent<PanelCTAActionProps>; /** Caption text or node to display */ caption?: string | React.ReactNode; /** Main description content */ description?: string | React.ReactNode; /** Caption for the documentation section */ docCaption?: string; /** URL for the documentation link */ docLink?: string; /** Icon to display (string for MuiIconFont or ReactNode for custom icon) */ icon?: string | React.ReactNode; /** Props to pass to the MuiIconFont component when icon is a string */ iconProps?: IconProps; /** Main label content */ label: string | React.ReactNode; /** Component display mode */ mode?: 'onboarding' | 'default'; /** Name used in the back button text */ name: string; /** Callback function for back button click */ onBack: () => void; /** Additional props for the panel */ panelProps?: Record<string, unknown>; /** Alternative label for demo mode */ demoLabel?: string; /** Alternative description for demo mode */ demoDescription?: string; /** Alternative caption for demo mode */ demoCaption?: string; };app/packages/embeddings/src/Embeddings.tsx (2)
70-70: Consider extracting the conditional rendering logicWhile the current implementation works, consider extracting the conditional rendering logic into a separate function for better readability and maintainability.
+ const renderContent = () => { + if (canSelect && !showCTA) { + return <EmbeddingsContainer>...</EmbeddingsContainer>; + } + return ( + <EmbeddingsCTA + mode={canSelect ? "default" : "onboarding"} + onBack={() => setShowCTA(false)} + /> + ); + }; + export default function Embeddings({ containerHeight, dimensions }) { // ... existing code ... - if (canSelect && !showCTA) { - return <EmbeddingsContainer>...</EmbeddingsContainer>; - } - return ( - <EmbeddingsCTA - mode={canSelect ? "default" : "onboarding"} - onBack={() => { - setShowCTA(false); - }} - /> - ); + return renderContent(); }Also applies to: 182-189
155-167: Enhance button implementation with accessibility and type safetyWhile the button implementation is functional, consider these improvements:
- Add an
aria-labelfor better accessibility- Consider using a type-safe constant for the mode check
<MuiButton startIcon={<Add />} + aria-label="Compute embeddings visualization" onClick={() => { - if (constants.IS_APP_MODE_FIFTYONE) { + if (constants.AppMode.FIFTYONE === constants.IS_APP_MODE_FIFTYONE) { setShowCTA(true); } else { computeViz.prompt(); } }} variant="contained" > Compute Embeddings </MuiButton>app/packages/components/src/components/Selector/Selector.tsx (1)
210-212: Consider improving the loading state implementationWhile the implementation works, there are a few improvements that could be made:
- Avoid using
floatfor layout. Consider using flex or grid instead.- Make the "Loading" text configurable for internationalization.
- Allow style customization through props.
Here's a suggested improvement:
-<DuringSuspense> - <LoadingDots style={{ float: "right" }} text="Loading" /> -</DuringSuspense> +<DuringSuspense> + <div style={{ display: "flex", justifyContent: "flex-end" }}> + <LoadingDots text={props.loadingText || "Loading"} /> + </div> +</DuringSuspense>And add to the interface:
loadingText?: string;app/packages/state/src/hooks/useCreateLooker.ts (1)
Line range hint
246-259: Consider implementing controller cleanupThe
ImaVidFramesControllerStoremaintains a reference to controllers by sample ID, but there's no cleanup mechanism. This could lead to memory leaks if controllers aren't removed when they're no longer needed.Consider adding cleanup logic in the effect hook:
useEffect(() => { return () => { // sending abort signal to clean up all event handlers abortControllerRef.current.abort(); + // Clean up ImaVidFramesController if it exists + const thisSampleId = sample?._id as string; + if (thisSampleId && ImaVidFramesControllerStore.has(thisSampleId)) { + ImaVidFramesControllerStore.delete(thisSampleId); + } }; }, []);docs/source/teams/pluggable_auth.md (3)
73-94: Consider adding security-related documentation for invitation linksThe documentation would benefit from additional information about:
- Invitation link expiration periods
- Security considerations when sharing invitation links
- Whether invitation links can be revoked or disabled
- Rate limiting or other security measures to prevent abuse
95-122: Enhance SMTP configuration documentation with technical detailsConsider adding:
- Required SMTP ports and protocols
- SSL/TLS configuration options
- Email template customization options (if available)
- Troubleshooting guide for common SMTP issues
- Best practices for SMTP server configuration
73-122: Add cross-references to related documentation sectionsConsider adding:
- A "See Also" section linking to related topics (e.g., user management, organization settings)
- Cross-references to relevant API documentation for programmatic invitation management
- Links to troubleshooting guides or FAQs
app/packages/core/src/plugins/SchemaIO/components/ToastView.tsx (3)
3-3: Add type annotations toToastViewcomponent's propsTo leverage TypeScript's type checking capabilities and improve code maintainability, consider adding type annotations to the
propsparameter of theToastViewcomponent.
66-66: Review defaultheightvalue insnackbarStyleThe default
heightvalue is set to5, which might correspond to5px. This may be too small for theSnackbar. Consider using a more appropriate default value or omitting theheightwhenlayout.heightis not provided.Apply this diff to adjust the default
heightvalue:- height: layout?.height || 5, + ...(layout?.height && { height: layout.height }),Alternatively, you may set a more suitable default height.
73-74: Use optional chaining for cleaner codeInstead of using the logical AND operator to check for the existence of
primaryandsecondarybefore invoking them, you can use optional chaining for more concise and readable code.Apply this diff to use optional chaining:
- {primary && primary(setOpen)} {/* Pass setOpen to primary button */} - {secondary && secondary(setOpen)} {/* Pass setOpen to secondary button */} + {primary?.(setOpen)} {/* Pass setOpen to primary button */} + {secondary?.(setOpen)} {/* Pass setOpen to secondary button */}🧰 Tools
🪛 Biome
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 74-74: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/packages/state/src/recoil/filters.ts (2)
87-98: Improve variable naming for clarityThe variable
setQueryPerformancecan be misleading as it suggests a function. Renaming it tooptimizationInfoorcanOptimizePathenhances readability and accurately reflects its purpose.Apply this diff to rename the variable:
-const setQueryPerformance = +const optimizationInfo = !modal && get(queryPerformance) && get(pathCanBeOptimized(path)); if (filter === null || filter instanceof DefaultValue) { delete newFilters[path]; - setQueryPerformance && setQueryPerformancePath(null); + optimizationInfo && setQueryPerformancePath(null); } else { newFilters[path] = filter; - setQueryPerformance && - setQueryPerformancePath(path, setQueryPerformance.isFrameField); + optimizationInfo && + setQueryPerformancePath(path, optimizationInfo.isFrameField); }
142-167: Clarify return type ofpathCanBeOptimizedselectorThe selector
pathCanBeOptimizedreturns eitherfalseor an object{ isFrameField: boolean }, which can be confusing. Consider defining a specific TypeScript type or interface for the return value to improve code clarity and maintainability.Example:
type OptimizationInfo = { isFrameField: boolean } | false; export const pathCanBeOptimized = selectorFamily<OptimizationInfo, string>({ // existing code });fiftyone/operators/builtins/panels/model_evaluation/__init__.py (3)
28-31: Consider normalizing environment variable values for robustnessCurrently, the environment variable
FIFTYONE_DISABLE_EVALUATION_CACHINGis compared directly toTRUTHY_VALUES. To make the code more robust, consider normalizing the environment variable value by stripping whitespace and converting it to lowercase before comparison. This will handle cases where the environment variable might have extra spaces or different casing.Apply this diff to normalize the environment variable value:
TRUTHY_VALUES = ["true", "True", "1", 1] ENABLE_CACHING = ( - os.environ.get("FIFTYONE_DISABLE_EVALUATION_CACHING") not in TRUTHY_VALUES + os.environ.get("FIFTYONE_DISABLE_EVALUATION_CACHING", "") + .strip() + .lower() + not in [str(v).lower() for v in TRUTHY_VALUES] ) CACHE_TTL = 30 * 24 * 60 * 60 # 30 days in seconds
246-273: Refactor repetitive code inget_confusion_matricesmethodThe method contains repetitive code for generating confusion matrices and colorscales for different class orderings. Consider refactoring this code to reduce duplication and improve maintainability.
You can refactor the code using a loop over the different sorting methods:
freq = Counter(results.ytrue) if results.missing in freq: freq.pop(results.missing) - az_classes = sorted(classes) - za_classes = sorted(classes, reverse=True) - mc_classes = sorted(freq, key=freq.get, reverse=True) - lc_classes = sorted(freq, key=freq.get) - az_matrix = results.confusion_matrix(classes=az_classes) - za_matrix = results.confusion_matrix(classes=za_classes) - mc_matrix = results.confusion_matrix(classes=mc_classes) - lc_matrix = results.confusion_matrix(classes=lc_classes) - az_colorscale = self.get_confusion_matrix_colorscale(az_matrix) - za_colorscale = self.get_confusion_matrix_colorscale(za_matrix) - mc_colorscale = self.get_confusion_matrix_colorscale(mc_matrix) - lc_colorscale = self.get_confusion_matrix_colorscale(lc_matrix) - return { - "az_classes": az_classes, - "za_classes": za_classes, - "mc_classes": mc_classes, - "lc_classes": lc_classes, - "az_matrix": az_matrix.tolist(), - "za_matrix": za_matrix.tolist(), - "mc_matrix": mc_matrix.tolist(), - "lc_matrix": lc_matrix.tolist(), - "az_colorscale": az_colorscale, - "za_colorscale": za_colorscale, - "mc_colorscale": mc_colorscale, - "lc_colorscale": lc_colorscale, - } + orderings = { + "az": sorted(classes), + "za": sorted(classes, reverse=True), + "mc": sorted(freq, key=freq.get, reverse=True), + "lc": sorted(freq, key=freq.get), + } + matrices = {} + for key, class_order in orderings.items(): + matrix = results.confusion_matrix(classes=class_order) + colorscale = self.get_confusion_matrix_colorscale(matrix) + matrices[f"{key}_classes"] = class_order + matrices[f"{key}_matrix"] = matrix.tolist() + matrices[f"{key}_colorscale"] = colorscale + return matrices
307-309: Use logging instead of printing tracebackUsing
traceback.print_exc()will print the traceback to standard error, which may not be appropriate in production environments. Consider using a logger to record exceptions.app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
44-44: UseuseRecoilValueinstead ofuseRecoilStatewhen only reading stateYou are only reading the value of
stagesfrom Recoil without modifying it. It's more appropriate to useuseRecoilValuewhen you don't need the setter function.Apply this diff to improve code clarity:
- import { useRecoilState } from "recoil"; + import { useRecoilValue } from "recoil"; ... - const [stages] = useRecoilState(view); + const stages = useRecoilValue(view);Also applies to: 1533-1533
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
docs/source/images/teams/cas/SMTP_config.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/cas/SMTP_config_test_email.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/cas/org_enable_invitations.pngis excluded by!**/*.png,!**/*.pngdocs/source/images/teams/cas/org_send_email_invitations.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (37)
app/packages/components/src/components/MuiButton/index.tsx(2 hunks)app/packages/components/src/components/PanelCTA/index.tsx(1 hunks)app/packages/components/src/components/Selector/Selector.tsx(3 hunks)app/packages/components/src/components/index.ts(1 hunks)app/packages/components/src/components/types.ts(1 hunks)app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx(3 hunks)app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx(3 hunks)app/packages/core/src/components/Filters/StringFilter/useOnSelect.ts(1 hunks)app/packages/core/src/components/Filters/StringFilter/useSelected.ts(0 hunks)app/packages/core/src/components/Filters/use-query-performance-timeout.ts(1 hunks)app/packages/core/src/components/Grid/Grid.tsx(3 hunks)app/packages/core/src/components/QueryPerformanceToast.tsx(4 hunks)app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EmptyOverview.tsx(0 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(26 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationNotes.tsx(4 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationPlot.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx(4 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/utils.ts(1 hunks)app/packages/core/src/plugins/SchemaIO/components/ToastView.tsx(1 hunks)app/packages/embeddings/src/Embeddings.tsx(5 hunks)app/packages/embeddings/src/EmbeddingsCTA.tsx(1 hunks)app/packages/embeddings/src/EmptyEmbeddings.tsx(0 hunks)app/packages/embeddings/src/styled-components.tsx(1 hunks)app/packages/embeddings/src/useComputeVisualization.ts(1 hunks)app/packages/operators/src/CustomPanel.tsx(1 hunks)app/packages/operators/src/hooks.ts(2 hunks)app/packages/operators/src/index.ts(1 hunks)app/packages/state/src/hooks/useCreateLooker.ts(1 hunks)app/packages/state/src/recoil/filters.ts(4 hunks)app/packages/state/src/recoil/selectors.ts(1 hunks)app/packages/utilities/src/constants.ts(1 hunks)app/packages/utilities/src/fetch.ts(0 hunks)app/packages/utilities/src/index.ts(1 hunks)docs/source/teams/pluggable_auth.md(1 hunks)fiftyone/operators/builtins/panels/model_evaluation/__init__.py(6 hunks)
💤 Files with no reviewable changes (4)
- app/packages/core/src/components/Filters/StringFilter/useSelected.ts
- app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EmptyOverview.tsx
- app/packages/embeddings/src/EmptyEmbeddings.tsx
- app/packages/utilities/src/fetch.ts
✅ Files skipped from review due to trivial changes (1)
- app/packages/components/src/components/types.ts
🧰 Additional context used
📓 Path-based instructions (30)
app/packages/components/src/components/MuiButton/index.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/components/src/components/PanelCTA/index.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/components/src/components/Selector/Selector.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/components/src/components/index.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Filters/StringFilter/useOnSelect.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Filters/use-query-performance-timeout.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Grid/Grid.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/QueryPerformanceToast.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationNotes.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationPlot.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/utils.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/ToastView.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/embeddings/src/Embeddings.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/embeddings/src/EmbeddingsCTA.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/embeddings/src/styled-components.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/embeddings/src/useComputeVisualization.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/operators/src/CustomPanel.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/operators/src/hooks.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/operators/src/index.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/hooks/useCreateLooker.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/recoil/filters.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/recoil/selectors.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/utilities/src/constants.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/utilities/src/index.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
📓 Learnings (1)
app/packages/core/src/plugins/SchemaIO/components/ToastView.tsx (1)
Learnt from: minhtuev
PR: voxel51/fiftyone#4957
File: app/packages/components/src/components/Toast/Toast.tsx:29-36
Timestamp: 2024-11-12T12:36:39.324Z
Learning: When reviewing React components in `app/packages/components/src/components/*`, verify component imports carefully before suggesting missing imports, especially when components are imported via destructuring from packages like `@mui/material`.
🪛 Biome
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx
[error] 1318-1318: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 1391-1391: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
app/packages/core/src/plugins/SchemaIO/components/ToastView.tsx
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 74-74: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (40)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx (2)
1-2: LGTM! Import changes align with component's purpose
The switch to MuiButton from the components library and the use of the Add icon better represent the evaluation action.
6-10:
Remove unused variant prop from implementation
The component's implementation doesn't use the variant prop that's defined in EvaluateProps. This creates a disconnect between the type definition and actual usage.
Apply this diff to align implementation with types:
-export default function Evaluate(props: EvaluateProps) {
- const { onEvaluate } = props;
+export default function Evaluate({ onEvaluate, variant, permissions }: EvaluateProps) {Likely invalid or redundant comment.
app/packages/embeddings/src/useComputeVisualization.ts (1)
Line range hint 13-19: Add error handling for operator execution
The prompt callback doesn't handle potential failures in operator execution.
Consider adding error handling:
const prompt = useCallback(() => {
- triggerEvent(panelId, {
- params: { delegate: true },
- operator: computeVisUri,
- prompt: true,
- });
+ try {
+ triggerEvent(panelId, {
+ params: { delegate: true },
+ operator: computeVisUri,
+ prompt: true,
+ });
+ } catch (error) {
+ console.error('Failed to execute compute visualization:', error);
+ // Consider adding error handling UI feedback
+ }
}, [panelId, triggerEvent, computeVisUri]);Let's verify the error handling patterns in other operators:
app/packages/core/src/components/Filters/use-query-performance-timeout.ts (2)
1-4: LGTM! Clean and well-organized imports
The imports are properly structured and include all necessary dependencies for the hook's functionality.
10-26: LGTM! Well-implemented React hook
The implementation follows React best practices:
- Proper use of dependencies array in useEffect
- Correct cleanup implementation
- Efficient early return pattern
- Proper state management with Recoil
Let's verify the usage of this hook across the codebase:
✅ Verification successful
Hook implementation and usage are consistent and correct
The verification confirms that:
- The hook is properly used in both NumericFieldFilter and StringFilter components with correct parameters
- The QueryPerformanceToastEvent is consistently implemented and used across the codebase
- The QP_WAIT constant is properly defined and shared
- All usages follow the same pattern and respect the hook's contract
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of useQueryPerformanceTimeout
# Expect: Usage in filter components with proper parameters
# Check for hook usage
rg -t typescript -t typescriptreact "useQueryPerformanceTimeout" -A 2
# Verify QP_WAIT constant definition
ast-grep --pattern 'const QP_WAIT = $_'
Length of output: 237
Script:
#!/bin/bash
# Let's try with ripgrep without type specification since the previous attempt failed
rg "useQueryPerformanceTimeout" -A 2
# Let's also check for the QueryPerformanceToastEvent usage
rg "QueryPerformanceToastEvent" -A 2
# Check the implementation of the hook itself
fd "use-query-performance-timeout.ts" --exec cat {}
Length of output: 4607
app/packages/core/src/components/Filters/StringFilter/useOnSelect.ts (1)
29-31: Verify the purpose of the empty string return value
The newly added empty string return value seems arbitrary and its purpose is unclear. This could affect consumers of this hook who might expect a meaningful return value.
Let's check how this hook's return value is used:
✅ Verification successful
Empty string return value is intentionally unused
The empty string return is acceptable in this case. The useOnSelect hook is used as a callback for the Selector component's onSelect prop, where the return value is not utilized. The hook's primary purpose is to:
- Validate ObjectId strings
- Update the Recoil state via
selectedAtom - Handle error cases by throwing
SelectorValidationError
The empty string return is just a formality to maintain the Promise chain and doesn't affect the component's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find usages of useOnSelect hook to verify how the return value is consumed
rg -l "useOnSelect" | xargs rg -A 5 "useOnSelect\(.+\)"
# Find any await/then usage of this hook since it returns a Promise
rg "await.*useOnSelect|\.then.*useOnSelect"
Length of output: 825
Script:
#!/bin/bash
# Let's examine the implementation of useOnSelect to understand its return type and usage
cat app/packages/core/src/components/Filters/StringFilter/useOnSelect.ts
# Let's also check the StringFilter component to see how onSelect is used
cat app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx
Length of output: 5969
app/packages/components/src/components/MuiButton/index.tsx (2)
2-8: LGTM: Clean import organization
The imports are well-organized and the addition of useTheme is appropriate for accessing theme values.
Line range hint 45-47: LGTM: Clean type definition
The type definition is well-structured and properly extends the Material-UI ButtonProps.
app/packages/operators/src/index.ts (1)
2-2: LGTM! The export follows TypeScript best practices.
The new hook export is well-placed among other hook exports and follows consistent export patterns.
Let's verify the hook implementation exists:
✅ Verification successful
✓ Export statement correctly references implemented hook
The hook useFirstExistingUri is properly implemented in hooks.ts as a React hook that searches for the first matching URI from a list of URIs against available operators. The export statement is valid and follows the codebase's conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the useFirstExistingUri hook implementation exists
# Expected: Find the hook implementation in the hooks.ts file
# Check if the hook is implemented in hooks.ts
ast-grep --pattern 'export function useFirstExistingUri' || ast-grep --pattern 'export const useFirstExistingUri ='
Length of output: 58
Script:
#!/bin/bash
# Let's try a broader search to locate the hook implementation
# First, check if the hooks.ts file exists in the operators package
fd "hooks.ts" app/packages/operators/src
# Then search for any mention of useFirstExistingUri in the codebase
rg "useFirstExistingUri" app/packages/operators/src -l
Length of output: 253
Script:
#!/bin/bash
# Let's examine the actual implementation in hooks.ts
rg "useFirstExistingUri" app/packages/operators/src/hooks.ts -A 5 -B 1
Length of output: 346
app/packages/embeddings/src/styled-components.tsx (1)
23-24: Verify layout impact on parent containers.
Since the component is absolutely positioned and now takes full width, please verify that:
- The parent container has
position: relativeset - The full width doesn't cause overflow issues
- The space-between justification doesn't create too much space between elements when there are few items
Let's check the component usage:
✅ Verification successful
Let me check the parent container's styling to verify the layout concerns.
Let me check the parent container's styling and layout structure to verify the concerns.
Layout changes are properly handled and safe
The layout concerns are addressed by the codebase structure:
- The parent
EmbeddingsContainerhasposition: relativeset, which properly contains the absolutely positionedSelectorscomponent - The full width is intentional and safe as:
EmbeddingsContainerhaswidth: 100%and proper margin/paddingSelectorshas padding and proper spacing withgap: 1remandcolumn-gap: 1rem
- The space-between justification is appropriate as:
- Left side contains a flex container with selectors and options
- Right side has a single "Compute Embeddings" button
- The layout maintains proper spacing with flex and gap properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all files that import and use the Selectors component
rg -l "import.*Selectors.*from.*styled-components" app/packages/embeddings/src/
rg -A 5 "<Selectors" app/packages/embeddings/src/
Length of output: 574
Script:
#!/bin/bash
# Check the parent container's styling in Embeddings.tsx
cat app/packages/embeddings/src/Embeddings.tsx
# Check for any styled-components definitions in Embeddings.tsx
ast-grep --pattern 'const $_ = styled.$_`$$$`'
Length of output: 140262
Script:
#!/bin/bash
# Check the EmbeddingsContainer styling and structure
cat app/packages/embeddings/src/styled-components.tsx
# Check the Embeddings component usage and layout
cat app/packages/embeddings/src/Embeddings.tsx
Length of output: 7632
app/packages/embeddings/src/EmbeddingsCTA.tsx (1)
1-4: LGTM: Clean and well-organized imports
The imports are properly structured with clear separation between external and local dependencies.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationPlot.tsx (2)
53-53: LGTM: Consistent interaction model
The addition of "select2d" to the removed buttons is consistent with the existing pattern of limiting plot interactions, as evidenced by the removal of other selection and zoom controls.
60-60: Verify if removing the "toImage" functionality aligns with requirements
Removing the "toImage" button prevents users from downloading plot visualizations. This might impact users who need to:
- Include plots in reports or documentation
- Share evaluation results with team members
- Archive evaluation results for future reference
Let's check if this is a consistent pattern across other visualization components:
app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx (1)
7-7: LGTM: Import statement follows conventions
The import statement is properly placed and follows React naming conventions.
app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.tsx (2)
Line range hint 1-99: Implementation follows React/TypeScript best practices
The overall implementation is well-structured with:
- Proper type definitions and prop typing
- Clean component composition and reusability
- Good separation of concerns
- Proper error handling for missing containers
- Consistent usage of Material-UI patterns
37-37: Verify layout behavior with height: "100%"
Adding height: "100%" to the Box component ensures it fills its parent container, but this might affect the layout behavior in different scenarios:
- When the parent container doesn't have a defined height
- When used within flex or grid layouts
- When containing content that should determine its own height
Let's verify the usage context:
✅ Verification successful
Let me gather more context about the layout structure and usage patterns.
Based on the search results, I can now provide a conclusive response about the height styling:
Height styling is consistent with application patterns
The height: "100%" addition to the Box component in ContainerizedComponent is verified to be correct because:
- It follows the established pattern in the codebase where parent containers like CustomPanel, PlotlyView, and NativeModelEvaluationView use
height: "100%"for proper layout containment - ContainerizedComponent is used within DynamicIO which needs to fill its container space
- The styling is consistent with other layout components in the SchemaIO plugin
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that render ContainerizedComponent
ast-grep --pattern 'const $_ = $$$; return <ContainerizedComponent $$$>$$$</ContainerizedComponent>'
# Search for parent containers that might be affected
rg -B 5 -A 5 '<ContainerizedComponent'
Length of output: 1202
Script:
#!/bin/bash
# Check the parent component DynamicIO's structure and styling
ast-grep --pattern 'export $_ DynamicIO$_: $_ = $$$'
# Look for Box component styling patterns in the codebase
rg -B 2 -A 2 'sx=\{.*height.*\}' --type tsx
# Check if ContainerizedComponent is used within any layout components
rg -B 3 -A 3 '<(Grid|Stack|Box).*>.*ContainerizedComponent' --type tsx
Length of output: 229
Script:
#!/bin/bash
# Check the parent component DynamicIO's structure and styling
ast-grep --pattern 'export $_ DynamicIO$_: $_ = $$$'
# Look for Box component styling patterns in the codebase
rg -B 2 -A 2 'sx=\{.*height'
# Check if ContainerizedComponent is used within any layout components
rg -B 3 -A 3 '<(Grid|Stack|Box).*>.*ContainerizedComponent'
# Check ContainerizedComponent's implementation
ast-grep --pattern 'export $_ ContainerizedComponent$_: $_ = $$$'
Length of output: 6976
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EvaluationNotes.tsx (1)
Line range hint 10-91: Implementation follows React/TypeScript best practices
The component is well-structured with proper:
- TypeScript type definitions
- React hooks usage
- Accessibility considerations
- Theme integration
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx (1)
49-56: LGTM! Well-implemented state management and callback
The implementation follows React best practices:
- Proper use of
useCallbackwith correct dependency array - Clear conditional logic based on app mode
- Efficient state management with
useState
app/packages/operators/src/CustomPanel.tsx (1)
82-82: LGTM: Height styling ensures proper dimension management
The addition of height: "100%" to the Box component is appropriate for maintaining proper layout hierarchy and ensuring the DimensionRefresher component fills its container.
app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx (2)
10-10: LGTM! Import statement follows conventions.
The import is correctly placed and follows the project's import organization pattern.
132-132: Verify loading state behavior with performance timeout.
The integration of withQueryPerformanceTimeout looks good, but we should verify that the loading states are handled correctly during suspense.
✅ Verification successful
Let me gather more information about the loading state handling.
Loading state implementation is consistent and properly handled
The loading states are correctly implemented across components:
- StringFilter uses
withQueryPerformanceTimeoutwith Suspense boundary - NumericFieldFilter has consistent loading pattern with
LoadingDotscomponent - Performance timeout is properly integrated with the recoil state management
- Path optimization checks are in place via
pathCanBeOptimizedselector
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using DuringSuspense prop with performance timeout
ast-grep --pattern 'DuringSuspense={withQueryPerformanceTimeout($_, $_)}'
# Check for consistent loading state handling patterns
rg -A 5 'useQueryPerformanceTimeout'
Length of output: 3227
Script:
#!/bin/bash
# Check implementation of withQueryPerformanceTimeout HOC
ast-grep --pattern 'withQueryPerformanceTimeout($_, $_)'
# Look for loading state patterns in the components
rg -A 3 'Loading' app/packages/core/src/components/Filters/
# Check implementation of pathCanBeOptimized
rg -A 5 'pathCanBeOptimized'
Length of output: 5275
app/packages/operators/src/hooks.ts (2)
10-10: LGTM: Import addition follows consistent style
The new import is correctly grouped with related operator imports and maintains the established import pattern.
154-163: Verify hook usage patterns
Let's verify the integration of this hook with the rest of the codebase.
app/packages/core/src/components/QueryPerformanceToast.tsx (2)
15-31: LGTM! Well-structured event implementation.
The QueryPerformanceToastEvent class and WindowEventMap extension are well-implemented with proper TypeScript typing.
54-57: LGTM! Improved state management.
The consolidated state management using a single data object improves code maintainability and state cohesion.
app/packages/core/src/components/Grid/Grid.tsx (4)
15-15: LGTM: Clean import addition for performance monitoring
The new imports are well-organized and properly integrated with existing imports.
134-137: LGTM: Clean integration of timeout clearing
The mount handler properly clears the timeout before dispatching the mount event, preventing any potential race conditions.
Line range hint 144-150: LGTM: Comprehensive cleanup implementation
The cleanup function properly handles all resources, including the new timeout clearing, while maintaining the existing cleanup operations.
126-133: Verify performance monitoring integration
Let's verify that the performance monitoring integration is consistent with other components in the codebase.
Also applies to: 144-150
✅ Verification successful
Let me gather more information about the implementation details to ensure consistency.
Performance monitoring integration is correctly implemented
The implementation in Grid.tsx follows the established patterns in the codebase:
- Uses the same
QP_WAITconstant (5151ms) defined in QueryPerformanceToast - Correctly dispatches
QueryPerformanceToastEventwith required parameters - Proper timeout cleanup is implemented in the component lifecycle
The implementation is consistent with the dedicated hook useQueryPerformanceTimeout and the toast component's expectations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar performance monitoring patterns
rg -l "QueryPerformanceToastEvent|QP_WAIT" --type ts
# Check for consistent timeout handling patterns
ast-grep --pattern 'useLayoutEffect(() => {
$$$
const timeout = setTimeout($$$)
$$$
return () => {
clearTimeout(timeout)
$$$
}
})'
Length of output: 385
Script:
#!/bin/bash
# Check the implementation of QueryPerformanceToast
cat app/packages/core/src/components/QueryPerformanceToast.tsx
# Check the hook implementation
cat app/packages/core/src/components/Filters/use-query-performance-timeout.ts
# Look for similar timeout patterns in Grid.tsx
rg -A 5 "setTimeout" app/packages/core/src/components/Grid/Grid.tsx
Length of output: 6463
app/packages/components/src/components/PanelCTA/index.tsx (1)
1-19: LGTM: Imports and constants are well-organized
The imports are properly structured with clear separation between external and internal dependencies. Constants are appropriately destructured from utilities.
app/packages/embeddings/src/Embeddings.tsx (2)
1-1: LGTM: Import statements are well-organized
The new imports for MuiButton, constants, and EmbeddingsCTA are properly added and logically grouped with related imports.
Also applies to: 4-4, 15-15
48-48: LGTM: State management is properly implemented
The showCTA state is well-named and follows React hooks best practices.
app/packages/components/src/components/Selector/Selector.tsx (2)
35-35: LGTM! Well-typed prop addition
The new DuringSuspense prop is properly typed as an optional function component, maintaining backward compatibility and following React conventions.
56-56: LGTM! Clean default implementation
The default value for DuringSuspense follows React best practices by using a fragment and maintaining proper type safety.
app/packages/state/src/hooks/useCreateLooker.ts (2)
Line range hint 291-301: LGTM! Well-implemented error handling and cleanup
The implementation follows React best practices:
- Uses AbortController for proper cleanup of event listeners
- Properly propagates errors to React Error Boundary
- Ensures cleanup of event listeners using AbortController signal
136-136: Verify the order of spreading fieldSchema
The current implementation spreads fieldSchema after defining the frames property. This could potentially override the frames property if fieldSchema contains its own frames definition.
✅ Verification successful
No issue with spreading fieldSchema
Based on the codebase analysis, the spread of fieldSchema after the frames property definition is safe and intentional. The frames property being spread is a special schema definition that includes frame-specific metadata, and the subsequent fieldSchema spread adds additional field definitions without overriding the frames structure. This is evidenced by:
- The
framesproperty has a specific structure withftype: LIST_FIELDandembeddedDocType: "fiftyone.core.frames.FrameSample" - The
fieldSchemais managed through Recoil state and contains sample-level fields that complement, not override, the frame structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if fieldSchema contains a frames property that could override the explicit frames definition
# Search for frames property definition in schema-related files
rg -A 5 'frames.*:.*{' app/packages/state/src/
Length of output: 136
Script:
#!/bin/bash
# Search for fieldSchema definition and usage
rg "fieldSchema" app/packages/state/src/ -A 5
# Also search for frames property definitions
rg "frames:" app/packages/state/src/ -A 5
Length of output: 18517
app/packages/state/src/recoil/selectors.ts (1)
Line range hint 587-593: LGTM! Verify the usage of the newly exported selector.
The implementation is correct and follows Recoil best practices. The selector maintains a single responsibility of mapping frame fields to their paths with the "frames." prefix.
Let's verify where this newly exported selector might be used:
✅ Verification successful
Export of frameFieldsList selector is correctly used internally
The verification shows that frameFieldsList is used internally within the same file by the isFrameField selector family (line reference shows get(frameFieldsList).some((p) => path.startsWith(p))). The export change appears to be part of a refactoring to make the selector's interface more consistent, but it maintains its original usage pattern within the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find potential usages of frameFieldsList selector
# Expected: Components that might be importing and using this selector
# Search for potential imports of frameFieldsList
rg "import.*frameFieldsList.*from.*selectors" -g "*.{ts,tsx}"
# Search for direct usage of frameFieldsList
rg "frameFieldsList\(" -g "*.{ts,tsx}"
Length of output: 104
Script:
#!/bin/bash
# Let's try a broader search strategy to find any references to frameFieldsList
# Search for any mention of frameFieldsList
rg "frameFieldsList" -g "*.{ts,tsx}" -A 2 -B 2
# Search for any selector that might use atoms.frameFields
rg "get\(atoms\.frameFields\)" -g "*.{ts,tsx}" -A 2 -B 2
# Look for the isFrameField selector family mentioned in the comment
ast-grep --pattern 'const isFrameField = selectorFamily'
Length of output: 3413
app/packages/utilities/src/index.ts (1)
17-17: LGTM! Clean namespace export addition.
The new namespace export for constants is a good practice as it:
- Organizes related constants under a single namespace
- Prevents naming collisions
- Makes imports more explicit (e.g.,
import { constants } from '@fiftyone/utilities')
app/packages/state/src/recoil/filters.ts (2)
9-9: Imports are appropriate and consistent
The imported modules pathHasIndexes, queryPerformance, hiddenLabelIds, and isFrameField are correctly imported and align with the project's conventions.
Also applies to: 11-11
87-98: 🛠️ Refactor suggestion
Update setQueryPerformancePath usage after refactoring
With queryPerformancePath managed by a Recoil atom, adjust how you update its value within this block.
Apply this diff:
-optimizationInfo && setQueryPerformancePath(null);
+optimizationInfo && set(queryPerformancePathState, null);And:
-optimizationInfo &&
- setQueryPerformancePath(path, optimizationInfo.isFrameField);
+optimizationInfo &&
+ set(queryPerformancePathState, {
+ isFrameField: optimizationInfo.isFrameField,
+ path,
+ });Likely invalid or redundant comment.
| import { useFirstExistingUri, usePanelEvent } from "@fiftyone/operators"; | ||
| import { usePanelId } from "@fiftyone/spaces"; | ||
| import { useCallback } from "react"; | ||
|
|
||
| const IS_OSS = true; // false in fiftyone-teams |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using environment variables for feature flags
The IS_OSS constant is hardcoded with a comment indicating different values for different environments. This approach is prone to errors during builds.
Consider using environment variables or build-time configuration:
-const IS_OSS = true; // false in fiftyone-teams
+const IS_OSS = process.env.NEXT_PUBLIC_IS_OSS === 'true';📝 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.
| import { useFirstExistingUri, usePanelEvent } from "@fiftyone/operators"; | |
| import { usePanelId } from "@fiftyone/spaces"; | |
| import { useCallback } from "react"; | |
| const IS_OSS = true; // false in fiftyone-teams | |
| import { useFirstExistingUri, usePanelEvent } from "@fiftyone/operators"; | |
| import { usePanelId } from "@fiftyone/spaces"; | |
| import { useCallback } from "react"; | |
| const IS_OSS = process.env.NEXT_PUBLIC_IS_OSS === 'true'; |
| <Button | ||
| {...containedStyles} | ||
| {...outlinedStyles} | ||
| variant={variant} | ||
| {...otherProps} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix props spreading order and remove duplicate variant prop
The current props spreading order might cause style overrides, and the variant prop is being spread twice.
<Button
+ variant={variant}
+ {...otherProps}
{...containedStyles}
{...outlinedStyles}
- variant={variant}
- {...otherProps}
/>📝 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.
| <Button | |
| {...containedStyles} | |
| {...outlinedStyles} | |
| variant={variant} | |
| {...otherProps} | |
| /> | |
| <Button | |
| variant={variant} | |
| {...otherProps} | |
| {...containedStyles} | |
| {...outlinedStyles} | |
| /> |
| value, | ||
| compareValue, | ||
| percentage = false, | ||
| fractionDigits?: number | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript type annotations to parameters and return type
The function parameters and return type lack TypeScript type annotations, which reduces type safety and IDE support.
export function getNumericDifference(
- value,
- compareValue,
+ value: number,
+ compareValue: number,
percentage = false,
- fractionDigits?: number
+ fractionDigits?: number
+): number | string | undefined📝 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.
| value, | |
| compareValue, | |
| percentage = false, | |
| fractionDigits?: number | |
| ) { | |
| value: number, | |
| compareValue: number, | |
| percentage = false, | |
| fractionDigits?: number | |
| ): number | string | undefined |
| if (typeof value === "number" && typeof compareValue === "number") { | ||
| const difference = value - compareValue; | ||
| if (percentage) { | ||
| return formatValue(((a - b) / a) * 100, fractionDigits); | ||
| const percentageDifference = (difference / compareValue) * 100; | ||
| return formatValue(percentageDifference, fractionDigits); | ||
| } | ||
| return formatValue(a - b, fractionDigits); | ||
| return formatValue(difference, fractionDigits); |
There was a problem hiding this comment.
Handle division by zero and add explicit error return
The function has two potential issues:
- Division by zero when
compareValueis 0 andpercentageis true - Missing explicit return when type check fails
- if (typeof value === "number" && typeof compareValue === "number") {
+ if (typeof value !== "number" || typeof compareValue !== "number") {
+ return undefined;
+ }
+
+ const difference = value - compareValue;
+
+ if (percentage) {
+ if (compareValue === 0) {
+ return Infinity;
+ }
+ const percentageDifference = (difference / compareValue) * 100;
+ return formatValue(percentageDifference, fractionDigits);
+ }
- const difference = value - compareValue;
- if (percentage) {
- const percentageDifference = (difference / compareValue) * 100;
- return formatValue(percentageDifference, fractionDigits);
- }
- return formatValue(difference, fractionDigits);
- }
+ return formatValue(difference, fractionDigits);📝 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 (typeof value === "number" && typeof compareValue === "number") { | |
| const difference = value - compareValue; | |
| if (percentage) { | |
| return formatValue(((a - b) / a) * 100, fractionDigits); | |
| const percentageDifference = (difference / compareValue) * 100; | |
| return formatValue(percentageDifference, fractionDigits); | |
| } | |
| return formatValue(a - b, fractionDigits); | |
| return formatValue(difference, fractionDigits); | |
| if (typeof value !== "number" || typeof compareValue !== "number") { | |
| return undefined; | |
| } | |
| const difference = value - compareValue; | |
| if (percentage) { | |
| if (compareValue === 0) { | |
| return Infinity; | |
| } | |
| const percentageDifference = (difference / compareValue) * 100; | |
| return formatValue(percentageDifference, fractionDigits); | |
| } | |
| return formatValue(difference, fractionDigits); |
| export const BOOK_A_DEMO_LINK = "https://voxel51.com/book-a-demo/"; | ||
| export const TRY_IN_BROWSER_LINK = "https://voxel51.com/try-fiftyone/"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving URLs to environment configuration
Hardcoding URLs directly in constants makes it difficult to manage different environments (staging, production) and future URL changes.
Consider moving these URLs to environment configuration:
-export const BOOK_A_DEMO_LINK = "https://voxel51.com/book-a-demo/";
-export const TRY_IN_BROWSER_LINK = "https://voxel51.com/try-fiftyone/";
+export const BOOK_A_DEMO_LINK = process.env.BOOK_A_DEMO_URL || "https://voxel51.com/book-a-demo/";
+export const TRY_IN_BROWSER_LINK = process.env.TRY_IN_BROWSER_URL || "https://voxel51.com/try-fiftyone/";📝 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.
| export const BOOK_A_DEMO_LINK = "https://voxel51.com/book-a-demo/"; | |
| export const TRY_IN_BROWSER_LINK = "https://voxel51.com/try-fiftyone/"; | |
| export const BOOK_A_DEMO_LINK = process.env.BOOK_A_DEMO_URL || "https://voxel51.com/book-a-demo/"; | |
| export const TRY_IN_BROWSER_LINK = process.env.TRY_IN_BROWSER_URL || "https://voxel51.com/try-fiftyone/"; |
| export const { getQueryPerformancePath, setQueryPerformancePath } = (() => { | ||
| let queryPerformancePath: { isFrameField: boolean; path: string } | null = | ||
| null; | ||
|
|
||
| return { | ||
| getQueryPerformancePath: () => queryPerformancePath, | ||
| setQueryPerformancePath: (path: string | null, isFrameField = false) => { | ||
| if (path) { | ||
| queryPerformancePath = { isFrameField, path }; | ||
| } else { | ||
| queryPerformancePath = null; | ||
| } | ||
| }, | ||
| }; | ||
| })(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using closures to manage shared state outside Recoil atoms
Storing queryPerformancePath within a closure may lead to issues with state synchronization and debugging since it's external to Recoil's state management. It's recommended to use a Recoil atom to manage this state, ensuring better integration with Recoil's reactivity and debugging tools.
Apply this diff to refactor the code:
+import { atom } from "recoil";
+
+export const queryPerformancePathState = atom<
+ { isFrameField: boolean; path: string } | null
+>({
+ key: "queryPerformancePath",
+ default: null,
+});
-
-export const { getQueryPerformancePath, setQueryPerformancePath } = (() => {
- let queryPerformancePath: { isFrameField: boolean; path: string } | null =
- null;
-
- return {
- getQueryPerformancePath: () => queryPerformancePath,
- setQueryPerformancePath: (path: string | null, isFrameField = false) => {
- if (path) {
- queryPerformancePath = { isFrameField, path };
- } else {
- queryPerformancePath = null;
- }
- },
- };
-})();After refactoring, you'd use Recoil's useRecoilState or similar hooks to get and set queryPerformancePathState, ensuring state changes are tracked properly.
Committable suggestion skipped: line range outside the PR's diff.
| def has_evaluation_results(self, dataset, eval_key): | ||
| try: | ||
| return bool(dataset._doc.evaluations[eval_key].results) | ||
| except Exception: | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid accessing protected member _doc; use public API instead
Accessing the protected member dataset._doc directly may lead to maintenance issues and violates encapsulation. Consider using public APIs to access evaluation data.
If a public method or property is available to access evaluation results, please use it instead of accessing dataset._doc.evaluations.
| updated_pending_evaluations_for_dataset_in_stored = [] | ||
| update_store = False | ||
| for pending in pending_evaluations_for_dataset_in_store: | ||
| pending_eval_key = pending.get("eval_key") | ||
| if pending_eval_key in eval_keys: | ||
| update_store = True | ||
| else: | ||
| pending_evaluations.append(pending) | ||
| updated_pending_evaluations_for_dataset_in_stored.append( | ||
| pending | ||
| ) | ||
| for key in eval_keys: | ||
| if not self.has_evaluation_results(ctx.dataset, key): | ||
| pending_evaluations.append({"eval_key": key}) | ||
| if update_store: | ||
| pending_evaluations_in_store[ | ||
| dataset_id | ||
| ] = updated_pending_evaluations_for_dataset_in_stored | ||
| store.set("pending_evaluations", pending_evaluations) |
There was a problem hiding this comment.
Fix variable name typo: 'in_stored' should be 'in_store'
The variable updated_pending_evaluations_for_dataset_in_stored appears to have a typo. It should be updated_pending_evaluations_for_dataset_in_store. This typo could lead to runtime errors or unexpected behavior.
Apply this diff to correct the variable name:
updated_pending_evaluations_for_dataset_in_store = []
for pending in pending_evaluations_for_dataset_in_store:
pending_eval_key = pending.get("eval_key")
if pending_eval_key in eval_keys:
update_store = True
else:
pending_evaluations.append(pending)
- updated_pending_evaluations_for_dataset_in_stored.append(
+ updated_pending_evaluations_for_dataset_in_store.append(
pending
)
if update_store:
pending_evaluations_in_store[
dataset_id
- ] = updated_pending_evaluations_for_dataset_in_stored
+ ] = updated_pending_evaluations_for_dataset_in_store
store.set("pending_evaluations", pending_evaluations)
ctx.panel.set_data("pending_evaluations", pending_evaluations)📝 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.
| updated_pending_evaluations_for_dataset_in_stored = [] | |
| update_store = False | |
| for pending in pending_evaluations_for_dataset_in_store: | |
| pending_eval_key = pending.get("eval_key") | |
| if pending_eval_key in eval_keys: | |
| update_store = True | |
| else: | |
| pending_evaluations.append(pending) | |
| updated_pending_evaluations_for_dataset_in_stored.append( | |
| pending | |
| ) | |
| for key in eval_keys: | |
| if not self.has_evaluation_results(ctx.dataset, key): | |
| pending_evaluations.append({"eval_key": key}) | |
| if update_store: | |
| pending_evaluations_in_store[ | |
| dataset_id | |
| ] = updated_pending_evaluations_for_dataset_in_stored | |
| store.set("pending_evaluations", pending_evaluations) | |
| updated_pending_evaluations_for_dataset_in_store = [] | |
| update_store = False | |
| for pending in pending_evaluations_for_dataset_in_store: | |
| pending_eval_key = pending.get("eval_key") | |
| if pending_eval_key in eval_keys: | |
| update_store = True | |
| else: | |
| pending_evaluations.append(pending) | |
| updated_pending_evaluations_for_dataset_in_store.append( | |
| pending | |
| ) | |
| for key in eval_keys: | |
| if not self.has_evaluation_results(ctx.dataset, key): | |
| pending_evaluations.append({"eval_key": key}) | |
| if update_store: | |
| pending_evaluations_in_store[ | |
| dataset_id | |
| ] = updated_pending_evaluations_for_dataset_in_store | |
| store.set("pending_evaluations", pending_evaluations) |
| setClassPerformanceDialogConfig((state) => { | ||
| return { | ||
| ...state, | ||
| limit: isNaN(newLimit) ? undefined : newLimit, |
There was a problem hiding this comment.
Use Number.isNaN instead of global isNaN for reliable NaN checking
The global isNaN function attempts type coercion and can lead to unexpected results. Using Number.isNaN ensures that only values of type number that are NaN are detected.
Apply this diff to fix the issue:
- limit: isNaN(newLimit) ? undefined : newLimit,
+ limit: Number.isNaN(newLimit) ? undefined : newLimit,Also applies to: 1391-1391
🧰 Tools
🪛 Biome
[error] 1318-1318: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| const classes = matrices[`${sortBy}_classes`].slice(parsedLimit); | ||
| const matrix = matrices[`${sortBy}_matrix`].slice(parsedLimit); |
There was a problem hiding this comment.
Fix incorrect slicing in getMatrix function to apply limit correctly
Currently, .slice(parsedLimit) slices from parsedLimit to the end of the array, which is likely not the intended behavior. To limit the number of items, you should slice from the start up to parsedLimit using .slice(0, parsedLimit).
Apply this diff to fix the issue:
- const classes = matrices[`${sortBy}_classes`].slice(parsedLimit);
- const matrix = matrices[`${sortBy}_matrix`].slice(parsedLimit);
+ const classes = matrices[`${sortBy}_classes`].slice(0, parsedLimit);
+ const matrix = matrices[`${sortBy}_matrix`].slice(0, parsedLimit);📝 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.
| const classes = matrices[`${sortBy}_classes`].slice(parsedLimit); | |
| const matrix = matrices[`${sortBy}_matrix`].slice(parsedLimit); | |
| const classes = matrices[`${sortBy}_classes`].slice(0, parsedLimit); | |
| const matrix = matrices[`${sortBy}_matrix`].slice(0, parsedLimit); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/packages/utilities/src/size-bytes-estimate.test.ts (1)
35-36: LGTM! Consider adding more function test cases.The test case correctly verifies the new behavior ensuring non-zero size estimates for functions.
Consider expanding the test coverage with additional function scenarios:
test("different function types return non-zero", () => { // Named function function named() { return null; } expect(sizeBytesEstimate(named)).toBe(1); // Async function expect(sizeBytesEstimate(async () => null)).toBe(1); // Generator function expect(sizeBytesEstimate(function* () { yield null; })).toBe(1); });app/packages/utilities/src/size-bytes-estimate.ts (2)
Line range hint
1-68: Consider enhancing type safety and documentationWhile the implementation is solid, there are opportunities to improve type safety and documentation:
- Add JSDoc comments to explain the minimum size guarantee
- Consider making the return type explicit
- Add unit type for byte sizes
Here's the suggested enhancement:
+ /** Size in bytes, minimum of 1 byte */ + type Bytes = number; + /** + * Estimates the size of an object in bytes + * @param object The object to measure + * @returns {Bytes} The estimated size in bytes (minimum 1) + */ - export default function sizeBytesEstimate(object: SizeTypes) + export default function sizeBytesEstimate(object: SizeTypes): Bytes
Line range hint
22-63: Consider optimizing size calculations for typed arraysThe
sizerfunction handles ArrayBuffer but doesn't optimize for TypedArrays (Uint8Array, Float32Array, etc.) which have known fixed sizes per element.Add specific handling for TypedArrays:
if (object instanceof ArrayBuffer) { return object.byteLength; } + + // Optimize for TypedArrays + if (ArrayBuffer.isView(object)) { + return object.byteLength; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/utilities/src/size-bytes-estimate.test.ts(1 hunks)app/packages/utilities/src/size-bytes-estimate.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/utilities/src/size-bytes-estimate.test.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/utilities/src/size-bytes-estimate.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (1)
app/packages/utilities/src/size-bytes-estimate.ts (1)
67-68: Verify the impact of enforcing non-zero sizes
The change ensures a minimum size of 1 byte, which is more accurate than returning 0 for functions and other non-object types. However, let's verify if this change affects any existing size calculations in the codebase.
✅ Verification successful
Based on the search results, I can now provide a final response:
Change to enforce non-zero sizes is safe and well-tested
The change to enforce a minimum size of 1 byte is safe and well-tested. The test file explicitly verifies this behavior with the test case "non-objects return non-zero", and all usages of sizeBytesEstimate in the codebase are for memory estimation purposes where a minimum value of 1 is appropriate:
- Used in overlays (classifications.ts, base.ts) for label size estimation
- Used in frame readers (frame-reader.ts, ima-vid-frame-samples.ts) for frame data size calculation
- No instances found where the function's output is compared against 0 or where exact zero values are required
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of sizeBytesEstimate to identify potential impacts
# Look for comparisons with 0 or size-dependent logic
# Search for direct usage of the function
rg -A 3 "sizeBytesEstimate\("
# Search for size comparisons that might be affected
rg -A 3 "size\s*(===?|<=?|>=?)\s*0"
Length of output: 15823
Merge
release/v1.1.0todevelopSummary by CodeRabbit
Release Notes
New Features
PanelCTAcomponent for enhanced call-to-action capabilities.DuringSuspenseprop to theSelectorcomponent for improved loading state management.Improvements
MuiButtonstyling with theme context support.NumericFieldFilterandStringFiltercomponents with better loading state handling.Evaluationcomponent for more dynamic performance metrics and confusion matrix configurations.Bug Fixes
ToastViewcomponent for better user notifications.Documentation