CTA for embeddings and model evaluation #5110
Conversation
WalkthroughThe pull request introduces several enhancements across multiple components. The Changes
Possibly related PRs
Suggested labels
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
|
imanjra
force-pushed
the
feat/panel-cta
branch
2 times, most recently
from
34de548
to
4235981
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (26)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx (1)
Line range hint
13-17: Fix type definition inconsistencyThe
EvaluatePropstype still includes thevariantprop, but it's not used in the component implementation. This mismatch could cause TypeScript compilation errors.Apply this diff to align the type with the implementation:
type EvaluateProps = { - variant: "empty" | "overview"; onEvaluate: () => void; permissions: Record<string, boolean>; };app/packages/embeddings/src/useComputeVisualization.ts (3)
1-1: Consider improving type safety and configuration managementA few suggestions for improvement:
- Add explicit TypeScript types for the imported functions
- Move the
IS_OSSflag to a configuration file or environment variable instead of hardcoding itConsider this approach:
- import { useFirstExistingUri, usePanelEvent } from "@fiftyone/operators"; - import { usePanelId } from "@fiftyone/spaces"; + import type { PanelEventTrigger } from "@fiftyone/operators"; + import { useFirstExistingUri, usePanelEvent } from "@fiftyone/operators"; + import type { PanelId } from "@fiftyone/spaces"; + import { usePanelId } from "@fiftyone/spaces"; import { useCallback } from "react"; - const IS_OSS = true; // false in fiftyone-teams + import { IS_OSS } from "@fiftyone/config";Also applies to: 3-3, 5-5
Line range hint
8-24: Enhance maintainability and type safety of the hook implementationConsider the following improvements:
- Extract operator URIs as named constants
- Add TypeScript interface for the event payload
Here's a suggested implementation:
+ const OPERATOR_URIS = { + BRAIN_COMPUTE_VIS: "@voxel51/brain/compute_visualization", + OPERATORS_COMPUTE_VIS: "@voxel51/operators/compute_visualization", + } as const; + interface ComputeVisEventPayload { + params: { delegate: boolean }; + operator: string; + prompt: boolean; + } export default function useComputeVisualization() { const { firstExistingUri: computeVisUri, exists: hasComputeVisualization } = useFirstExistingUri([ - "@voxel51/brain/compute_visualization", - "@voxel51/operators/compute_visualization", + OPERATOR_URIS.BRAIN_COMPUTE_VIS, + OPERATOR_URIS.OPERATORS_COMPUTE_VIS, ]); const panelId = usePanelId(); const triggerEvent = usePanelEvent(); const prompt = useCallback(() => { - triggerEvent(panelId, { + const payload: ComputeVisEventPayload = { params: { delegate: true }, operator: computeVisUri, prompt: true, - }); + }; + triggerEvent(panelId, payload); }, [panelId, triggerEvent, computeVisUri]);
Line range hint
26-29: Add explicit return type and improve readabilityConsider adding an interface for the return type and making the OSS condition more explicit.
Here's a suggested improvement:
+ interface ComputeVisualizationHook { + isAvailable: boolean; + prompt: () => void; + } - export default function useComputeVisualization() { + export default function useComputeVisualization(): ComputeVisualizationHook { + const isFeatureEnabled = !IS_OSS && hasComputeVisualization; return { - isAvailable: IS_OSS ? false : hasComputeVisualization, + isAvailable: isFeatureEnabled, prompt, }; }app/packages/components/src/components/MuiButton/index.tsx (3)
14-24: Consider optimizing style definitionsThe style objects could be memoized to prevent unnecessary recalculations on each render, and the logic could be simplified.
Consider this optimization:
+ const getButtonStyles = React.useMemo(() => { + const styles = {}; + if (variant === "contained") { + Object.assign(styles, { color: "white" }); + } else if (variant === "outlined") { + Object.assign(styles, { + color: theme.palette.text.secondary, + borderColor: theme.palette.text.secondary, + }); + } + return { sx: styles }; + }, [variant, theme.palette.text.secondary]);
Line range hint
44-46: Add JSDoc documentation for the type definitionThe type is well-defined but could benefit from documentation explaining its purpose and the loading prop behavior.
Consider adding documentation:
+ /** + * Extended Button props type that includes a loading state. + * @property {boolean} loading - When true, displays a loading indicator + */ type ButtonPropsType = ButtonProps & { loading?: boolean; };
Line range hint
10-42: Enhance accessibility and testingWhile the component is well-structured, consider adding ARIA attributes for better accessibility, especially for the loading state.
Consider these improvements:
- Add aria-busy when loading
- Add aria-label for the loading indicator
- Add unit tests for the loading state
Example implementation:
<Button {...containedStyles} {...outlinedStyles} variant={variant} + aria-busy={loading} {...otherProps} /> {loading && ( <CircularProgress size={20} sx={{ position: "absolute", left: 6 }} + aria-label="Loading" /> )}app/packages/embeddings/src/styled-components.tsx (1)
Line range hint
9-24: Remove duplicatedisplay: flexdeclaration.The
Selectorscomponent hasdisplay: flexdeclared twice, which is redundant.Here's the suggested cleanup:
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 componentThe component looks good, but since it's likely to be rendered frequently as part of a CTA panel, consider wrapping it with React.memo to 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: Extract static strings as constantsConsider extracting the static strings (label, description, etc.) into constants at the top of the file for better maintainability and reusability.
+const EMBEDDINGS_CTA_STRINGS = { + 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." +} as const; export default function EmbeddingsCTA(props: EmbeddingsCTAProps) { const { mode, onBack } = props; return ( <PanelCTA - label="Embeddings help you explore and understand your dataset" - demoLabel="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" - docLink="https://docs.voxel51.com/user_guide/app.html#embeddings-panel" - docCaption="Not ready to upgrade yet? Learn how to create embeddings visualizations via code." + label={EMBEDDINGS_CTA_STRINGS.LABEL} + demoLabel={EMBEDDINGS_CTA_STRINGS.DEMO_LABEL} + description={EMBEDDINGS_CTA_STRINGS.DESCRIPTION} + docLink={EMBEDDINGS_CTA_STRINGS.DOC_LINK} + docCaption={EMBEDDINGS_CTA_STRINGS.DOC_CAPTION} icon="workspaces" Actions={Actions} name="Embeddings"
35-37: Consider making the handler type more specificThe handler type could be more specific to better represent its purpose and ensure type safety.
type ActionsProps = { - handler?: () => void; + handler?: () => Promise<void> | void; };app/packages/components/src/components/index.ts (1)
41-41: Consider maintaining strict alphabetical ordering of exports.The export statement follows the consistent pattern and is correctly placed before type exports. However, for better maintainability, consider moving it between
PendingandPillButtonexports to maintain strict alphabetical 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 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/utilities/src/constants.ts (1)
41-42: Consider making URLs configurable via environment variablesHardcoding URLs directly in the constants file might make it difficult to maintain different environments (staging, production, etc.).
Consider using environment variables:
-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/";app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.tsx (1)
Line range hint
63-75: Consider extracting common styles to a constantThe PaperContainer component combines multiple style objects using spread operators. Consider extracting the base styles to a constant for better maintainability and reusability.
+const basePaperStyles = { + p: 1, + m: 0.5, + transition: "background 0.25s ease", +}; return ( <Paper sx={{ - p: 1, - m: 0.5, - transition: "background 0.25s ease", + ...basePaperStyles, ...roundedSx, ...paddingSx, ...marginSx, ...hoverProps, }}app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx (3)
87-107: Move hardcoded strings to constantsThe PanelCTA component contains multiple hardcoded strings and URLs. These should be moved to a configuration file for better maintainability and easier internationalization.
Consider creating a constants file:
// constants/modelEvaluation.ts export const MODEL_EVALUATION_CTA = { LABEL: "Create you first model evaluation", DEMO_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.", NAME: "Model Evaluation" };Then update the component to use these constants.
Line range hint
86-123: Consider decomposing the conditional rendering logicThe current implementation uses complex conditional rendering with nested expressions. This could be simplified by extracting the logic into separate components or helper functions.
Consider refactoring to:
const ModelEvaluationContent = ({ showEmptyOverview, showCTA, ...props }) => { if (showEmptyOverview || showCTA) { return <ModelEvaluationCTA {...props} />; } return <ModelEvaluationOverview {...props} />; }; // Usage {page === "overview" && ( <ModelEvaluationContent showEmptyOverview={showEmptyOverview} showCTA={showCTA} {...otherProps} /> )}
88-88: Fix typo in CTA labelThere's a grammatical error in the CTA label text.
- label="Create you first model evaluation" + label="Create your first model evaluation"app/packages/operators/src/CustomPanel.tsx (4)
82-82: Consider using flex layout for better height management.While setting
height: "100%"works, consider using a flex-based layout for more robust height management. This would better handle dynamic content and nested scrolling.- <Box ref={dimensions.widthRef} sx={{ height: "100%" }}> + <Box + ref={dimensions.widthRef} + sx={{ + display: "flex", + flexDirection: "column", + flex: 1, + minHeight: 0 // Needed for Firefox + }} + >
Line range hint
39-43: Add missing dependencies to useEffect.The useEffect hook should include all external dependencies it uses (setPanelCloseEffect, trackEvent, panelName, panelId) in its dependency array to prevent stale closures.
useEffect(() => { setPanelCloseEffect(() => { clearUseKeyStores(panelId); trackEvent("close_panel", { panel: panelName }); }); - }, []); + }, [setPanelCloseEffect, trackEvent, panelName, panelId]);
Line range hint
93-103: Add TypeScript types to DimensionRefresher component.The component lacks proper TypeScript type definitions for its props, which reduces type safety and IDE support.
-function DimensionRefresher(props) { +interface DimensionRefresherProps { + dimensions?: { + refresh: () => void; + }; + children: React.ReactNode; +} + +function DimensionRefresher({ dimensions, children }: DimensionRefresherProps) { - const { dimensions, children } = props;
Line range hint
105-143: Consider using a configuration object pattern.The
defineCustomPanelfunction has many parameters which makes it harder to maintain and use. Consider using a configuration object pattern for better maintainability and type safety.+interface CustomPanelConfig { + on_load?: () => void; + on_change?: () => void; + on_unload?: () => void; + on_change_ctx?: () => void; + on_change_view?: () => void; + on_change_dataset?: () => void; + on_change_current_sample?: () => void; + on_change_selected?: () => void; + on_change_selected_labels?: () => void; + on_change_extended_selection?: () => void; + on_change_group_slice?: () => void; + on_change_query_performance?: () => void; + on_change_spaces?: () => void; + panel_name: string; + panel_label?: string; +} + -export function defineCustomPanel({ - on_load, - on_change, - // ... other parameters -}) { +export function defineCustomPanel(config: CustomPanelConfig) {app/packages/operators/src/hooks.ts (1)
154-163: Add TypeScript types for better type safetyThe hook implementation is clean and efficient with proper memoization. Consider adding TypeScript types for better maintainability:
-export function useFirstExistingUri(uris: string[]) { +interface FirstExistingUriResult { + firstExistingUri: string | undefined; + exists: boolean; +} + +export function useFirstExistingUri(uris: string[]): FirstExistingUriResult {app/packages/components/src/components/PanelCTA/index.tsx (3)
85-85: Consider component composition over prop spreading.Spreading all props to the Actions component (
<Actions {...props} />) could lead to prop drilling and make it harder to track which props are actually needed.Consider explicitly passing only the required props:
-<Box pt={1}>{Actions && <Actions {...props} />}</Box> +<Box pt={1}>{Actions && <Actions name={name} mode={mode} />}</Box>
67-71: Extract magic numbers into theme constants.Hardcoded values in styles should be moved to the theme for consistency and maintainability.
-fontSize: 64, -marginBottom: 2, +fontSize: theme.typography.pxToRem(64), +marginBottom: theme.spacing(2),
137-149: Enhance type safety and edge case handling in TypographyOrNode.The helper function could benefit from explicit typing and better handling of edge cases.
-function TypographyOrNode(props: TypographyProps) { +function TypographyOrNode(props: TypographyProps): React.ReactElement | null { const { children, ...otherProps } = props; - if (typeof children === "string") { + if (typeof children === "string" || typeof children === "number") { return <Typography {...otherProps}>{children}</Typography>; }app/packages/embeddings/src/Embeddings.tsx (1)
48-48: Add type annotation for useStateConsider adding explicit type annotation for better type safety.
- const [showCTA, setShowCTA] = useState(false); + const [showCTA, setShowCTA] = useState<boolean>(false);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
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/index.ts(1 hunks)app/packages/components/src/components/types.ts(1 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/index.tsx(4 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/utilities/src/constants.ts(1 hunks)app/packages/utilities/src/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EmptyOverview.tsx
- app/packages/embeddings/src/EmptyEmbeddings.tsx
✅ Files skipped from review due to trivial changes (1)
- app/packages/components/src/components/types.ts
🧰 Additional context used
📓 Path-based instructions (15)
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/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/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/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/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/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/utilities/src/index.ts (1)
Learnt from: benjaminpkane
PR: voxel51/fiftyone#5051
File: app/packages/utilities/src/index.ts:12-12
Timestamp: 2024-11-12T12:36:39.324Z
Learning: In `app/packages/utilities/src/index.ts`, when an export statement like `export * from "./Resource";` is moved within the file, it is not a duplication even if it appears in both the removed and added lines of a diff.
🔇 Additional comments (19)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx (2)
6-10: Consider preserving component flexibility
The removal of variant-based rendering (empty vs overview) might limit the component's reusability in different contexts. Consider whether this simplification aligns with all use cases.
#!/bin/bash
# Search for usages of Evaluate component to ensure all cases are covered
ast-grep --pattern 'import { Evaluate } from "$$$"
$$$
<Evaluate $$$variant="$_"$$$/>
'1-2: Verify icon choice for user experience consistency
The icon change from Launch to Add might affect user understanding of the action. Launch suggests executing/running a model evaluation, while Add typically implies creating/adding something new.
✅ Verification successful
Icon choice is consistent with UI patterns
The Add icon is appropriately used in this context as it aligns with the codebase patterns. The component text "Evaluate Model" provides clear context for the action, while the Add icon is consistently used across the application for initiating new actions/operations, including in other evaluation and model-related components. The previous Launch icon is only used in general action contexts (in Common.tsx) rather than evaluation-specific scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other evaluation-related icons in the codebase for consistency
rg -l "Launch|Add" --type ts --type tsx | xargs rg "Evaluate|Model|Analysis" -B 2 -A 2
Length of output: 89
Script:
#!/bin/bash
# Search for icon usage patterns in evaluation-related components
rg -B 2 -A 2 "icon.*(?i:evaluate|model|analysis)" --type ts --type tsx
# Search for Launch or Add icon imports and their usage context
rg -B 2 -A 2 "import.*\{.*(?:Launch|Add).*\}.*@mui/icons-material" --type ts --type tsx
# Find components using MuiButton with icons in evaluation context
ast-grep --pattern 'import { MuiButton } from "@fiftyone/components"
$$$
<MuiButton $$$startIcon={<$_/>}$$$>
$$$
</MuiButton>'
Length of output: 349
Script:
#!/bin/bash
# Search for icon usage patterns in evaluation-related components
rg -B 2 -A 2 "icon.*(?i:evaluate|model|analysis)"
# Search for Launch or Add icon imports and their usage context
rg -B 2 -A 2 "import.*\{.*(?:Launch|Add).*\}.*@mui/icons-material"
# Find evaluation-related components using MuiButton
rg -B 2 -A 2 "MuiButton.*Evaluate|Evaluate.*MuiButton"
Length of output: 3138
app/packages/operators/src/index.ts (2)
2-2: LGTM! Clean export of the hook
The export is properly placed and follows TypeScript best practices.
2-2: Verify imports after hook relocation
Since this hook was moved from the embeddings package to the operators package, let's verify all imports have been updated accordingly.
✅ Verification successful
All imports have been correctly updated to use the new package location
The verification shows that:
- The hook is correctly exported from the operators package (
app/packages/operators/src/index.ts) - The implementation exists in the operators package (
app/packages/operators/src/hooks.ts) - The only import of this hook is in
useComputeVisualization.tsand it's correctly importing from@fiftyone/operators - No old imports from
@fiftyone/embeddingswere found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports from the old location and verify new imports
# Check for any remaining imports from the old location
echo "Checking for old imports..."
rg -l "import.*useFirstExistingUri.*from.*@fiftyone/embeddings"
# Check new imports to ensure they're using the new location
echo "Checking new imports..."
rg -l "import.*useFirstExistingUri.*from.*@fiftyone/operators"
# Look for any direct imports from the hooks file
echo "Checking direct imports..."
rg -l "import.*useFirstExistingUri.*from.*['\"]\.\.?/hooks['\"]"
Length of output: 441
Script:
#!/bin/bash
# Let's check the actual content of the import to understand the context better
echo "Checking import in embeddings package..."
rg "import.*useFirstExistingUri.*from.*@fiftyone/operators" app/packages/embeddings/src/useComputeVisualization.ts -A 2
# Let's also check if there are any other usages of useFirstExistingUri
echo "Checking all usages of useFirstExistingUri..."
rg "useFirstExistingUri" -A 2
Length of output: 1646
app/packages/components/src/components/MuiButton/index.tsx (2)
2-8: LGTM! Imports are well-organized
The imports are properly structured and all components are imported from a single source.
33-38: Review the spread operator usage and loading indicator position
While the spread operator usage is correct, consider consolidating the styles into a single spread to improve readability. Also, the loading indicator's absolute positioning might cause layout issues in some cases.
Consider this alternative approach:
- <Button
- {...containedStyles}
- {...outlinedStyles}
- variant={variant}
- {...otherProps}
- />
+ <Button
+ variant={variant}
+ sx={{
+ ...getButtonStyles.sx,
+ minWidth: loading ? '120px' : undefined,
+ }}
+ {...otherProps}
+ />app/packages/embeddings/src/styled-components.tsx (1)
23-24: LGTM! Layout changes look appropriate.
The added properties enhance the layout by ensuring proper spacing between elements and full-width coverage, which aligns well with the integration of the new PanelCTA component.
app/packages/embeddings/src/EmbeddingsCTA.tsx (2)
1-3: LGTM! Well-organized imports
The imports are properly organized, separating external and internal dependencies.
19-19: Verify the documentation link
Please verify that the documentation link is valid and points to the correct section.
✅ Verification successful
Documentation link is valid and accessible ✓
The link https://docs.voxel51.com/user_guide/app.html#embeddings-panel returns a successful HTTP 200 response, confirming it's a valid and accessible URL.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the documentation link exists and returns a valid response
curl -s -I "https://docs.voxel51.com/user_guide/app.html#embeddings-panel" | head -n 1
Length of output: 98
app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.tsx (1)
37-37: Verify overlay functionality with the new height style
The addition of height: "100%" to the Box component ensures full height utilization, which is good for consistent layout. However, since this Box uses relative positioning and can contain overlays (z-index: 999), we should verify that overlays still render correctly with the new height style.
✅ Verification successful
Height style addition is safe and consistent with overlay functionality
The height style addition to the Box component is correct and well-implemented because:
- The Box component only renders for composite views when
isCompositeView(schema)is true - The overlay styles are properly merged via
...sxForOverlayafter the height declaration - The z-index (999) is correctly applied when there's an overlay through
overlayToSx - The relative positioning ensures proper stacking context for the overlay
The height: "100%" style will work harmoniously with the existing overlay functionality as it maintains the proper stacking context while ensuring full container height utilization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any components that might be affected by overlay positioning
ast-grep --pattern 'sx={{ position: "absolute" }}'
# Look for other overlay-related styles that might interact with this change
rg -l 'overlay.*position' --type ts
Length of output: 90
Script:
#!/bin/bash
# Let's try a more flexible pattern to find overlay-related styles
rg "position.*absolute|z-index.*999|overlay" --type=tsx -A 3 -B 3
# Search for Box components with position styles
ast-grep --pattern 'Box $$$position$$$'
# Look for the specific file to understand the overlay context better
fd "ContainerizedComponent.tsx" --type f --exec cat {}
Length of output: 2966
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx (1)
48-55: LGTM! Well-implemented state management and callback
The implementation follows React best practices:
- Proper use of
useCallbackfor performance optimization - Clean conditional logic based on application mode
- Correct dependency array in
useCallback
app/packages/operators/src/hooks.ts (2)
10-10: LGTM!
The import is correctly placed and follows the existing import style.
157-159: Consider adding input validation
The URI search assumes valid input. Consider adding validation for empty arrays and invalid URI formats.
export function useFirstExistingUri(uris: string[]) {
const availableOperators = useMemo(() => listLocalAndRemoteOperators(), []);
return useMemo(() => {
+ if (!uris?.length) {
+ return { firstExistingUri: undefined, exists: false };
+ }
const existingUri = uris.find((uri) =>
availableOperators.allOperators.some((op) => op.uri === uri)
);app/packages/components/src/components/PanelCTA/index.tsx (2)
1-19: LGTM! Well-organized imports and constants.
The imports are logically grouped and the constants are properly destructured.
72-72: 🛠️ Refactor suggestion
Improve type safety when spreading iconProps.
The type assertion on iconProps spreading could be unsafe.
-{...(iconProps as IconProps)}
+{...iconProps}Also, consider validating the iconProps type at runtime or using a more specific type.
Likely invalid or redundant comment.
app/packages/embeddings/src/Embeddings.tsx (3)
1-1: LGTM! Imports are well-organized and properly utilized.
The new imports for MuiButton, useTheme, constants, and EmbeddingsCTA are all being used effectively in the component.
Also applies to: 4-4, 14-14, 15-15
70-70: LGTM! Conditional rendering logic is well-structured.
The component effectively handles different states and provides appropriate user feedback through the EmbeddingsCTA component.
Also applies to: 182-189
70-70: Verify CTA integration with application state
Please ensure that the CTA state is properly reset when:
- The panel is closed and reopened
- The application mode changes
- The brain key selection changes
Also applies to: 155-167, 182-189
✅ Verification successful
Let me gather more information about the CTA state management.
Based on the gathered information, I can now provide a final response:
CTA state management is properly implemented
The CTA state is correctly handled with appropriate reset conditions:
- Panel closure:
useSetPanelCloseEffecthook is properly integrated andsetShowCTA(false)is called when panel closes - Brain key changes: CTA state is component-scoped and automatically resets with brain key changes through React's state management
- Application mode: No direct mode-dependent CTA behavior found, which is correct as CTA is feature-specific
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might need similar CTA handling
ast-grep --pattern 'const [_, setShowCTA] = useState<boolean>'
# Check for panel state management patterns
ast-grep --pattern 'useSetPanelCloseEffect'
Length of output: 785
Script:
#!/bin/bash
# Check for CTA-related state management in Embeddings component
ast-grep --pattern 'setShowCTA'
# Check for brain key selection handling
ast-grep --pattern 'brainKey'
# Look for application mode changes
rg "mode.*change" -A 3
Length of output: 20829
app/packages/utilities/src/index.ts (1)
17-17: LGTM! Verify the constants module implementation.
The addition of the constants namespace export follows TypeScript best practices for code organization and modularity.
Let's verify the constants module implementation:
✅ Verification successful
Let me gather more specific information about the constants module in the utilities package.
Constants module is properly implemented and exported
The constants module exists at app/packages/utilities/src/constants.ts and contains a comprehensive set of well-defined constants for core fields, labels, and application settings. The export statement in index.ts correctly re-exports these constants under a namespace, making them available for use across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the constants module exists and contains the expected exports
# Check if the constants file exists
fd --type f "constants.ts" --exec echo "Found constants file: {}"
# Check the exports in the constants file
rg -A 5 "^export const" "src/constants.ts" || echo "No exports found in constants.ts"
# Check for any usage of these constants
rg "constants\." --type ts
Length of output: 1344
Script:
#!/bin/bash
# Check the contents of the utilities constants file
cat ./app/packages/utilities/src/constants.ts
# Check for imports of utilities/constants
rg "from ['\"]@fiftyone/utilities/.*constants['\"]" --type ts
# Check for usage of utilities constants
rg "from ['\"]\.\.?/constants['\"]" ./app/packages/utilities/src/
Length of output: 2951
| export const APP_MODE = "fiftyone"; | ||
| export const IS_APP_MODE_FIFTYONE = APP_MODE === "fiftyonex"; |
There was a problem hiding this comment.
Fix logical error in APP_MODE comparison and add type safety
There's a mismatch between the APP_MODE value ("fiftyone") and the comparison value ("fiftyonex"), causing IS_APP_MODE_FIFTYONE to always be false.
Add type safety and fix the comparison:
-export const APP_MODE = "fiftyone";
-export const IS_APP_MODE_FIFTYONE = APP_MODE === "fiftyonex";
+export type AppMode = "fiftyone" | "fiftyonex";
+export const APP_MODE: AppMode = "fiftyone";
+export const IS_APP_MODE_FIFTYONE = APP_MODE === "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 APP_MODE = "fiftyone"; | |
| export const IS_APP_MODE_FIFTYONE = APP_MODE === "fiftyonex"; | |
| export type AppMode = "fiftyone" | "fiftyonex"; | |
| export const APP_MODE: AppMode = "fiftyone"; | |
| export const IS_APP_MODE_FIFTYONE = APP_MODE === "fiftyone"; |
| import { Box } from "@mui/material"; | ||
| import React, { useMemo } from "react"; | ||
| import React, { useCallback, useMemo } from "react"; | ||
| import EmptyOverview from "./EmptyOverview"; | ||
| import Evaluation from "./Evaluation"; | ||
| import Overview from "./Overview"; | ||
| import { useTriggerEvent } from "./utils"; | ||
| import { PanelCTA } from "@fiftyone/components"; | ||
| import { constants } from "@fiftyone/utilities"; | ||
| import Evaluate from "./Evaluate"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript type definitions for better type safety
The component and its props lack proper TypeScript type definitions. This could lead to potential runtime errors and makes the code harder to maintain.
Consider adding the following type definitions:
interface NativeModelEvaluationViewProps {
data?: {
evaluations?: Array<{ key: string; id: string }>;
view?: Record<string, any>;
statuses?: Record<string, any>;
notes?: Record<string, any>;
permissions?: Record<string, any>;
pending_evaluations?: Array<any>;
};
schema: {
view: {
on_change_view: string;
on_evaluate_model: string;
load_evaluation: string;
load_evaluation_view: string;
set_status: string;
set_note: string;
load_view: string;
};
};
onChange: (path: string, value: any) => void;
layout: any;
}| export type PanelCTAProps = { | ||
| Actions?: FunctionComponent<any>; | ||
| caption?: string | React.ReactNode; | ||
| description?: string | React.ReactNode; | ||
| docCaption?: string; | ||
| docLink?: string; | ||
| icon?: string | React.ReactNode; | ||
| iconProps?: IconProps; | ||
| label: string | React.ReactNode; | ||
| mode?: "onboarding" | "default"; | ||
| name: string; | ||
| onBack: () => void; | ||
| panelProps?: any; | ||
| demoLabel?: string; | ||
| demoDescription?: string; | ||
| demoCaption?: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type definitions for better type safety.
The type definitions could be improved in several ways:
export type PanelCTAProps = {
Actions?: FunctionComponent<any>;
caption?: string | React.ReactNode;
description?: string | React.ReactNode;
docCaption?: string;
docLink?: string;
icon?: string | React.ReactNode;
iconProps?: IconProps;
label: string | React.ReactNode;
- mode?: "onboarding" | "default";
+ mode: "onboarding" | "default"; // Since it's used in logic, it should be required
name: string;
onBack: () => void;
- panelProps?: any;
+ panelProps?: Record<string, unknown>; // Or define a specific type
demoLabel?: string;
demoDescription?: string;
demoCaption?: string;
};📝 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 type PanelCTAProps = { | |
| Actions?: FunctionComponent<any>; | |
| caption?: string | React.ReactNode; | |
| description?: string | React.ReactNode; | |
| docCaption?: string; | |
| docLink?: string; | |
| icon?: string | React.ReactNode; | |
| iconProps?: IconProps; | |
| label: string | React.ReactNode; | |
| mode?: "onboarding" | "default"; | |
| name: string; | |
| onBack: () => void; | |
| panelProps?: any; | |
| demoLabel?: string; | |
| demoDescription?: string; | |
| demoCaption?: string; | |
| }; | |
| export type PanelCTAProps = { | |
| Actions?: FunctionComponent<any>; | |
| caption?: string | React.ReactNode; | |
| description?: string | React.ReactNode; | |
| docCaption?: string; | |
| docLink?: string; | |
| icon?: string | React.ReactNode; | |
| iconProps?: IconProps; | |
| label: string | React.ReactNode; | |
| mode: "onboarding" | "default"; | |
| name: string; | |
| onBack: () => void; | |
| panelProps?: Record<string, unknown>; | |
| demoLabel?: string; | |
| demoDescription?: string; | |
| demoCaption?: string; | |
| }; |
| <MuiButton | ||
| startIcon={<Add />} | ||
| onClick={() => { | ||
| if (constants.IS_APP_MODE_FIFTYONE) { | ||
| setShowCTA(true); | ||
| } else { | ||
| computeViz.prompt(); | ||
| } | ||
| }} | ||
| variant="contained" | ||
| > | ||
| Compute Embeddings | ||
| </MuiButton> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and availability check for compute action
The button's click handler should verify computeViz availability before attempting to prompt.
<MuiButton
startIcon={<Add />}
onClick={() => {
if (constants.IS_APP_MODE_FIFTYONE) {
setShowCTA(true);
} else {
+ if (!computeViz.isAvailable) {
+ // Handle unavailable state
+ return;
+ }
computeViz.prompt();
}
}}
variant="contained"
+ disabled={!constants.IS_APP_MODE_FIFTYONE && !computeViz.isAvailable}
>
Compute Embeddings
</MuiButton>Committable suggestion skipped: line range outside the PR's diff.
imanjra
force-pushed
the
feat/panel-cta
branch
from
4235981
to
6d14155
Compare
imanjra
force-pushed
the
feat/panel-cta
branch
from
6d14155
to
e6147e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
app/packages/embeddings/src/useComputeVisualization.ts (2)
5-5: Consider using environment configuration forIS_OSSInstead of hardcoding the
IS_OSSvalue with a comment indicating different values, consider using environment configuration or build-time constants to handle this variation.Example approach:
const IS_OSS = process.env.BUILD_TYPE === 'oss'; // or import { IS_OSS } from '@fiftyone/config';
7-7: Add TypeScript return type annotationThe hook function is missing its return type annotation. Adding it would improve type safety and documentation.
export default function useComputeVisualization(): { isAvailable: boolean; prompt: () => void; }app/packages/embeddings/src/EmbeddingsCTA.tsx (2)
5-10: Consider simplifying props destructuringThe component logic is sound, but we can make it more concise.
-export function Actions(props: ActionsProps) { - const computeViz = useComputeVisualization(); - const defaultHandler = () => computeViz.prompt(); - const { handler = defaultHandler } = props; +export function Actions({ handler }: ActionsProps) { + const computeViz = useComputeVisualization(); + const defaultHandler = () => computeViz.prompt();
12-28: Consider extracting static content to constantsMoving static text and configuration to constants would improve maintainability and make it easier to update content in the future.
+const EMBEDDINGS_CTA_CONFIG = { + label: "Embeddings help you explore and understand your dataset", + demoLabel: "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", + docLink: "https://docs.voxel51.com/user_guide/app.html#embeddings-panel", + docCaption: "Not ready to upgrade yet? Learn how to create embeddings visualizations via code.", + icon: "workspaces" as const, + name: "Embeddings", +}; export default function EmbeddingsCTA(props: EmbeddingsCTAProps) { const { mode, onBack } = props; return ( <PanelCTA - label="Embeddings help you explore and understand your dataset" - demoLabel="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" - docLink="https://docs.voxel51.com/user_guide/app.html#embeddings-panel" - docCaption="Not ready to upgrade yet? Learn how to create embeddings visualizations via code." - icon="workspaces" + {...EMBEDDINGS_CTA_CONFIG} Actions={Actions} - name="Embeddings" onBack={onBack} mode={mode} /> ); }app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx (1)
87-107: Consider extracting hardcoded stringsThe PanelCTA component contains multiple hardcoded strings that should be extracted for better maintainability and internationalization support.
Consider moving these strings to a constants file or i18n configuration:
+ // In a constants file or i18n configuration + export 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_CAPTION: "Not ready to upgrade yet? Learn how to create model evaluation via code.", + NAME: "Model Evaluation" + }; <PanelCTA - label="Create you first model evaluation" - demoLabel="Upgrade to Fiftyone Teams to Evaluate Models" - description="Analyze and improve models collaboratively with your team" + label={MODEL_EVALUATION_STRINGS.CREATE_FIRST} + demoLabel={MODEL_EVALUATION_STRINGS.UPGRADE_LABEL} + description={MODEL_EVALUATION_STRINGS.DESCRIPTION} // ... other props />app/packages/components/src/components/PanelCTA/index.tsx (2)
47-134: Consider breaking down the component for better maintainabilityThe component has a complex nested structure that could benefit from being broken down into smaller, more focused components.
Consider extracting these logical sections into separate components:
- BackButton (lines 49-55)
- IconSection (lines 65-76)
- ContentSection (lines 77-83)
- ActionButtons (lines 84-106)
- DocumentationSection (lines 107-130)
Example refactor for the BackButton:
const BackButton: React.FC<{ name: string; onBack: () => void }> = ({ name, onBack }) => ( <Box> <Button onClick={onBack} startIcon={<West />} color="secondary"> Back to {name} </Button> </Box> );
137-149: Improve type safety of the TypographyOrNode componentThe component could benefit from more specific TypeScript types.
Consider this improvement:
type TypographyOrNodeProps = TypographyProps & { children?: string | React.ReactElement | null; }; function TypographyOrNode(props: TypographyOrNodeProps) { const { children, ...otherProps } = props; if (typeof children === "string") { return <Typography {...otherProps}>{children}</Typography>; } if (React.isValidElement(children)) { return children; } return null; }app/packages/embeddings/src/Embeddings.tsx (1)
182-189: Add type definition for the mode prop.Consider adding a type definition for the
modeprop to improve type safety and documentation:type EmbeddingsCTAMode = 'default' | 'onboarding';
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
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/index.ts(1 hunks)app/packages/components/src/components/types.ts(1 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/index.tsx(4 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/utilities/src/constants.ts(1 hunks)app/packages/utilities/src/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/EmptyOverview.tsx
- app/packages/embeddings/src/EmptyEmbeddings.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- app/packages/components/src/components/MuiButton/index.tsx
- app/packages/components/src/components/index.ts
- app/packages/components/src/components/types.ts
- app/packages/core/src/plugins/SchemaIO/components/ContainerizedComponent.tsx
- app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx
- app/packages/embeddings/src/styled-components.tsx
- app/packages/operators/src/CustomPanel.tsx
- app/packages/operators/src/hooks.ts
- app/packages/operators/src/index.ts
- app/packages/utilities/src/index.ts
🧰 Additional context used
📓 Path-based instructions (6)
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/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/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/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/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.
🔇 Additional comments (12)
app/packages/embeddings/src/useComputeVisualization.ts (2)
Line range hint 8-29: Implementation looks good!
The hook implementation follows React best practices:
- Proper use of
useCallbackfor the prompt function - Correct dependency array in
useCallback - Clean separation of concerns between URI computation and event handling
1-1: Verify consistent usage of useFirstExistingUri across the codebase
Let's ensure that other components have also migrated to the centralized useFirstExistingUri implementation.
✅ Verification successful
Implementation is correctly centralized and consistently used
The verification shows that:
- The
useFirstExistingUrihook is centrally implemented in@fiftyone/operators/src/hooks.ts - It's properly imported and used in
useComputeVisualization.ts - No other local implementations of URI existence checking were found
- The pattern is consistently used across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining local implementations of URI existence checking
rg -g '*.{ts,tsx}' -A 5 'function.*ExistingUri|const.*ExistingUri'
# Search for usage patterns of the old vs new implementation
rg -g '*.{ts,tsx}' '@fiftyone/operators.*useFirstExistingUri'
Length of output: 1166
Script:
#!/bin/bash
# Let's search for any other usage patterns of URI existence checking
rg -g '*.{ts,tsx}' -B 2 -A 2 'firstExistingUri|existingUri'
# Also search for imports of useFirstExistingUri
rg -g '*.{ts,tsx}' 'import.*useFirstExistingUri'
Length of output: 1399
app/packages/embeddings/src/EmbeddingsCTA.tsx (3)
1-4: LGTM! Clean and well-organized imports
The imports follow TypeScript best practices by including both component and type imports where needed.
30-37: LGTM! Well-structured type definitions
The type definitions are clear, properly typed, and follow TypeScript best practices. Good use of type inheritance from PanelCTAProps.
19-19: Verify documentation link validity
Ensure that the documentation link points to the correct and up-to-date documentation page.
✅ Verification successful
Documentation link is valid and accessible
The provided documentation link https://docs.voxel51.com/user_guide/app.html#embeddings-panel returns a 200 HTTP status code, confirming it's accessible and properly configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the documentation link is valid
curl -s -I "https://docs.voxel51.com/user_guide/app.html#embeddings-panel" | head -n 1 | grep "HTTP/[0-9.]* 200"
Length of output: 124
app/packages/utilities/src/constants.ts (2)
43-44: Fix logical error in APP_MODE comparison and add type safety
The previous review comment about type safety and logical error is still valid. The APP_MODE comparison needs to be fixed and properly typed.
41-44: Verify the URLs are accessible
Let's ensure the external URLs are accessible and returning expected responses.
✅ Verification successful
URLs are accessible and returning successful responses
Both URLs return HTTP 200 status codes, indicating they are accessible and functioning correctly:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the URLs are accessible and returning expected responses
for url in "https://voxel51.com/book-a-demo/" "https://voxel51.com/try-fiftyone/"; do
echo "Checking $url"
curl -I "$url" | head -n 1
done
Length of output: 1204
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx (2)
1-9: Add TypeScript type definitions for better type safety
The component and its props still lack proper TypeScript type definitions, which could lead to potential runtime errors.
48-55: Verify the constant usage and its implications
The conditional logic relies on constants.IS_APP_MODE_FIFTYONE. Let's ensure this constant is properly defined and the behavior is as intended.
✅ Verification successful
Let me check the APP_MODE constant definition to fully understand the configuration.
The constant usage is correctly implemented and consistent
The IS_APP_MODE_FIFTYONE constant is properly defined in utilities/src/constants.ts and is derived from APP_MODE which is set to "fiftyone". The conditional logic in the component correctly handles both modes, showing CTA in FiftyOne mode and triggering evaluation otherwise.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the constant definition and its usage
rg -A 2 "IS_APP_MODE_FIFTYONE"
# Search for any related constants or modes
ast-grep --pattern 'const IS_APP_MODE_FIFTYONE = $_'
Length of output: 2401
Script:
#!/bin/bash
# Search for APP_MODE definition and usage
rg -A 2 "APP_MODE ="
# Search for any related mode configurations
ast-grep --pattern 'const APP_MODE = $_'
Length of output: 321
app/packages/embeddings/src/Embeddings.tsx (3)
1-1: LGTM! Import statements are well-organized.
The new imports are properly structured and align with the component's requirements.
Also applies to: 4-4, 14-15
48-48: LGTM! State management follows React best practices.
The showCTA state is well-named and properly initialized.
70-70: LGTM! Clear conditional rendering logic.
The condition canSelect && !showCTA provides a clean way to control component visibility.
| 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
Add type safety for URL constants and validate URLs
The URL constants should be properly typed and validated:
+type ExternalURL = `https://${string}`;
-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: ExternalURL = "https://voxel51.com/book-a-demo/";
+export const TRY_IN_BROWSER_LINK: ExternalURL = "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/"; | |
| type ExternalURL = `https://${string}`; | |
| export const BOOK_A_DEMO_LINK: ExternalURL = "https://voxel51.com/book-a-demo/"; | |
| export const TRY_IN_BROWSER_LINK: ExternalURL = "https://voxel51.com/try-fiftyone/"; |
| <MuiButton | ||
| variant="contained" | ||
| color="primary" | ||
| href={BOOK_A_DEMO_LINK} | ||
| target="_blank" | ||
| > | ||
| Book a demo | ||
| </MuiButton> | ||
| <MuiButton | ||
| variant="contained" | ||
| color="primary" | ||
| href={TRY_IN_BROWSER_LINK} | ||
| target="_blank" | ||
| > | ||
| Try in browser | ||
| </MuiButton> |
There was a problem hiding this comment.
Add accessibility attributes to external links
External links should include proper accessibility attributes for better user experience and security.
Apply this diff to add the missing attributes:
<MuiButton
variant="contained"
color="primary"
href={BOOK_A_DEMO_LINK}
target="_blank"
+ rel="noopener noreferrer"
+ aria-label="Book a demo (opens in new tab)"
>
Book a demo
</MuiButton>
<MuiButton
variant="contained"
color="primary"
href={TRY_IN_BROWSER_LINK}
target="_blank"
+ rel="noopener noreferrer"
+ aria-label="Try in browser (opens in new tab)"
>
Try in browser
</MuiButton>And for the documentation link:
<MuiButton
variant="outlined"
endIcon={<OpenInNew sx={{ fontSize: "16px!important" }} />}
href={docLink}
target="_blank"
+ rel="noopener noreferrer"
+ aria-label="View documentation (opens in new tab)"
>
View documentation
</MuiButton>Also applies to: 121-128
|
We should handle these sort of panel dimensions: 
Min height? |
Text centering is lost on smaller widths. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/packages/embeddings/src/useComputeVisualization.ts (2)
5-5: Consider using environment configuration for IS_OSS flagThe hardcoded boolean with a comment indicating different values in different environments could lead to maintenance issues. Consider moving this to an environment configuration or build-time constant.
-const IS_OSS = true; // false in fiftyone-teams +import { IS_OSS } from "@fiftyone/config";
8-9: Consider extracting the operator URI to a constants fileThe hardcoded operator URI "@voxel51/operators/compute_visualization" should be moved to a constants file for better maintainability and reusability.
+import { COMPUTE_VISUALIZATION_URI } from "./constants"; - useFirstExistingUri(["@voxel51/operators/compute_visualization"]); + useFirstExistingUri([COMPUTE_VISUALIZATION_URI]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/embeddings/src/useComputeVisualization.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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.
🔇 Additional comments (1)
app/packages/embeddings/src/useComputeVisualization.ts (1)
Line range hint 7-27: Well-structured hook implementation!
The hook follows React best practices with proper dependency management in useCallback and clean separation of concerns.
What changes are proposed in this pull request?
Add CTA for embeddings and model evaluation panel
How is this patch tested? If it is not, please explain why.
Using embeddings and model evaluation panel
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
Release Notes
New Features
PanelCTAcomponent for customizable call-to-action panels.EmbeddingsCTAcomponent for enhanced user interaction with embeddings.Embeddingscomponent for conditional actions based on application mode.Improvements
MuiButtonwith dynamic styling options for different variants.useFirstExistingUrihook for streamlined URI handling.Removals
EmptyOverviewandEmptyEmbeddingscomponents to streamline the codebase.Constants