Create compound group indexes by default #5174
Conversation
WalkthroughThe pull request introduces several modifications across various components in the application. Key changes include the addition of new methods and attributes for improved functionality and testability, such as a new Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (24)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🧰 Additional context used📓 Path-based instructions (14)app/packages/components/src/components/PillBadge/PillBadge.tsx (1)Pattern app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx (1)Pattern app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx (1)Pattern app/packages/core/src/components/Grid/Grid.tsx (1)Pattern app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx (1)Pattern app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)Pattern app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)Pattern app/packages/core/src/plugins/SchemaIO/components/TimerView.tsx (1)Pattern app/packages/core/src/plugins/SchemaIO/components/index.ts (1)Pattern app/packages/spotlight/src/row.ts (1)Pattern app/packages/spotlight/src/types.ts (1)Pattern e2e-pw/src/oss/poms/action-row/display-options.ts (1)Pattern e2e-pw/src/oss/poms/sidebar.ts (1)Pattern e2e-pw/src/oss/specs/sidebar/query-performance.spec.ts (1)Pattern 📓 Learnings (1)app/packages/spotlight/src/row.ts (1)🔇 Additional comments (18)e2e-pw/src/oss/poms/action-row/display-options.ts (2)
The new
The implementation follows POM best practices with reliable element selection. Let's verify that the UI components have the corresponding test IDs: ✅ Verification successfulUI components have the correct test IDs implemented The verification confirms that the UI components in 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for the presence of qp-mode-* test IDs in UI components
# Search for data-testid attributes matching the pattern
rg -g '*.{tsx,jsx}' 'data-testid=["'\'']qp-mode-(?:enabled|disabled)["'\'']'
Length of output: 78 Script: #!/bin/bash
# Let's try a broader search for test IDs containing 'qp-mode'
rg -g '*.{tsx,jsx,ts,js}' 'qp-mode'
# Also search for QueryPerformanceMode to understand the enum values
ast-grep --pattern 'enum QueryPerformanceMode {
$$$
}'
Length of output: 372 Script: #!/bin/bash
# Let's check the Options.tsx file to see the complete implementation
cat 'app/packages/core/src/components/Actions/Options.tsx'
# Also check for the enum definition
rg -g '*.ts' 'enum QueryPerformanceMode'
Length of output: 7906 app/packages/spotlight/src/types.ts (1)
The implementation looks correct, but let's verify its usage pattern in the Grid component to ensure consistent behavior with ✅ Verification successfulThe The verification shows that both methods are used appropriately in cleanup scenarios:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check usage patterns of detach and destroy methods in Grid component
# Expected: Similar usage patterns for both methods
# Check for destroy method usage
echo "Checking destroy method usage:"
rg -A 3 "destroy\(" "app/packages/core/src/components/Grid"
# Check for detach method usage
echo "Checking detach method usage:"
rg -A 3 "detach\(" "app/packages/core/src/components/Grid"
# Look for any cleanup patterns in Grid component
echo "Checking cleanup patterns:"
ast-grep --pattern 'useEffect(() => { $$$ return () => { $$$ } }, $$$)' "app/packages/core/src/components/Grid"
Length of output: 6138 app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx (1)
The LightningIcon is correctly defined as a local constant, following component encapsulation best practices. The styling approach using theme variables is appropriate. app/packages/core/src/plugins/SchemaIO/components/index.ts (1)
The new TimerView export follows the consistent pattern and is correctly alphabetically ordered. Let's verify the TimerView component implementation: ✅ Verification successfulTimerView component implementation verified and follows best practices The component follows React and TypeScript conventions with:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify TimerView component implementation and type definitions
# Check if TimerView component exists and follows React conventions
ast-grep --pattern 'export default function TimerView($_) {
$$$
}'
# Check for proper TypeScript type definitions
ast-grep --pattern 'interface TimerViewProps {
$$$
}'
# Check for proper React hook usage
rg -A 5 'useTimer|useEffect|useState' "./app/packages/core/src/plugins/SchemaIO/components/TimerView.tsx"
Length of output: 1568 app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx (1)
The replacement of Material UI's Bolt with a custom LightningBolt icon aligns well with the PR's focus on query performance visualization. app/packages/core/src/components/Grid/Grid.tsx (3)
The
The addition of the
Let's ensure this new ✅ Verification successfulDetach functionality is properly integrated and consistent across the codebase The
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for other Spotlight instantiations to verify compatibility
ast-grep --pattern 'new Spotlight<$_,$_>({
$$$
})'
# Search for any existing detach-like functionality
rg -g '*.{ts,tsx}' 'detach.*=.*\(.*\).*=>'
Length of output: 3950 Script: #!/bin/bash
# Let's check for the Looker interface or class definition to verify detach method
ast-grep --pattern 'interface $_Looker {
$$$
}'
# Also check for any class implementations
ast-grep --pattern 'class $_ implements $_Looker {
$$$
}'
# Search for detach method usage
rg -g '*.{ts,tsx}' '\.detach\(' -B 2 -A 2
Length of output: 2170 app/packages/spotlight/src/row.ts (1)
The fiftyone/operators/builtin.py (1)
The fiftyone/operators/types.py (2)
The method signature reformatting enhances readability while maintaining the existing functionality.
The new docs/source/user_guide/app.rst (4)Line range hint The new section effectively explains:
The notes about frame fields and frame filtering provide crucial information for users working with video datasets. The separation into two distinct notes improves readability.
The section effectively covers:
The note about dynamic assignment of fields to default groups provides important context for understanding runtime behavior. docs/source/user_guide/using_datasets.rst (1)
The added text effectively explains that queries can be optimized for better performance, which aligns well with the PR's objective of creating compound group indexes by default. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (27)
app/packages/spotlight/src/types.ts (1)
61-61: Add JSDoc documentation for the new method.The
detachmethod would benefit from JSDoc documentation explaining its purpose, parameters, and how it differs from thedestroymethod.+ /** + * Detaches an item from the spotlight configuration. + * @param id The ID of the item to detach + */ detach?: (id: ID) => void;app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx (1)
Line range hint
44-71: Consider optimizing state dependencies and component structureThe IconWrapper component has multiple Recoil state dependencies and complex conditional logic. Consider:
- Breaking down the state dependencies into a custom hook
- Memoizing individual state values to prevent unnecessary re-renders
- Splitting the component into smaller, more focused components
This would improve maintainability and potentially performance.
Example refactor:
// Custom hook to manage state const useIconState = (path: string, modal: boolean) => { const disabled = useRecoilValue(fos.isDisabledFilterPath(path)) && !modal; const expandedPath = useRecoilValue(fos.expandPath(path)); // ... other state return { disabled, expandedPath, // ... other values }; }; // Simplified component const IconWrapper = ({ modal, path }: { modal: boolean; path: string }) => { const { disabled, expandedPath, frameFilteringDisabled, indexed, queryPerformance, frameField, } = useIconState(path, modal); if (queryPerformance && indexed && !modal && !frameField) { return <Lightning path={path} frameFilteringDisabled={frameFilteringDisabled} />; } return <ArrowIcon {...{ disabled, expandedPath, frameFilteringDisabled, path, modal }} />; };app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx (1)
53-56: LGTM! Good addition of test selector.The data-cy attribute follows good practices by creating a unique identifier using the path prop, which will help with component testing.
Consider adding additional test selectors to child components (RangeSlider, Box) to enable more granular testing scenarios. For example:
<Box> <Button + data-cy={`numeric-filter-${path}-load-button`} text={`Filter by ${name}`} color={color} onClick={handleShowRange}app/packages/core/src/plugins/SchemaIO/components/TimerView.tsx (4)
14-19: Enhance Timer base class with abstract methodsThe base Timer class should define the contract for its implementations using abstract methods.
-class Timer { +abstract class Timer { protected _ref: NodeJS.Timeout | null = null; - start() {} - stop() {} + abstract start(): void; + abstract stop(): void; }
67-73: Move console warning to development environmentThe warning should only appear in development mode to avoid polluting production logs.
if (!interval && !timeout) { - console.warn( - "useTimer requires either `interval` or `timeout` to be defined." - ); + if (process.env.NODE_ENV === 'development') { + console.warn( + "useTimer requires either `interval` or `timeout` to be defined." + ); + } return; }
75-95: Extract event handling logicThe event handling logic could be extracted into a separate function for better maintainability.
+const createEventHandler = ( + triggerEvent: Function, + panelId: string, + operator: string | undefined, + params: object | undefined +) => () => { + if (operator) { + triggerEvent(panelId, { + operator, + params: params || {}, + prompt: null, + }); + } +}; + -const handleTimerEvent = interval - ? () => { - if (on_interval) { - triggerEvent(panelId, { - operator: on_interval, - params: operator_params || {}, - prompt: null, - }); - } - } - : () => { - if (on_timeout) { - triggerEvent(panelId, { - operator: on_timeout, - params: operator_params || {}, - prompt: null, - }); - } - }; +const handleTimerEvent = createEventHandler( + triggerEvent, + panelId, + interval ? on_interval : on_timeout, + operator_params +);
1-117: Add error handling and unit testsThis component handles time-based operations which are critical for proper application function. Consider:
- Adding error boundaries
- Writing comprehensive unit tests for timer behavior
Would you like me to:
- Generate an error boundary implementation for this component?
- Create a test suite covering the timer functionality?
app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx (2)
106-106: Consider documenting the query performance icon conditionsThe LightningBolt icon's visibility depends on multiple conditions (
named,queryPerformance,indexed,!modal,!frameField). Consider adding a comment explaining these conditions for better maintainability.+ // Show performance icon for indexed fields in the main view (non-modal) + // when performance tracking is enabled and the field is not a frame field {showQueryPerformanceIcon && <LightningBolt />}
Line range hint
54-143: Consider breaking down the StringFilter componentThe component handles multiple responsibilities (filtering, search, performance indicators) and has complex conditional logic. Consider splitting it into smaller, focused sub-components for better maintainability and testability:
- StringFilterHeader (for label and performance indicator)
- StringFilterSearch (for search functionality)
- StringFilterContent (for checkboxes and results)
This would improve:
- Code organization
- Testing capabilities
- Reusability
- Maintenance
app/packages/core/src/components/Grid/Grid.tsx (1)
65-68: Add type annotation for the id parameterThe
detachmethod follows the same pattern asdestroy, but lacks type safety. Consider adding proper type annotation to theidparameter.- detach: (id) => { + detach: (id: { description: string }) => {app/packages/components/src/components/PillBadge/PillBadge.tsx (3)
63-65: Enhance type safety and performance of the getColor functionConsider these improvements:
- Add explicit return type annotation
- Validate the colorValue parameter
- Memoize the function since COLORS is constant
- const getColor = (colorValue: string): string => { + const getColor = React.useCallback((colorValue: string): keyof typeof COLORS | string => { + if (!colorValue) return COLORS.default; return COLORS[colorValue] || colorValue; - }; + }, []);
68-68: LGTM! Consider making the fallback chain more explicitThe color resolution logic is correct but could be more readable.
- color: getColor(chipColor || color || "default"), + const resolvedColor = chipColor ?? color ?? "default"; + color: getColor(resolvedColor),
Line range hint
7-17: Consider architectural improvements for better maintainabilityThe component could benefit from these structural improvements:
- Extract props interface to a separate type
- Split the complex conditional rendering into smaller components
- Consider using a reducer for state management
Example for props interface extraction:
interface PillBadgeProps { text: string | string[] | [string, string][]; color?: string; variant?: "filled" | "outlined"; showIcon?: boolean; operator?: () => void; readOnly?: boolean; tooltipTitle?: string; }app/packages/spotlight/src/row.ts (1)
234-235: Enhance error reporting in cleanup operations.The implementation correctly supports both destroy and detach operations with proper error handling. Consider enhancing the error reporting to include more context about failed items.
Consider this improvement to error handling:
const errors = []; for (const item of this.#row) { try { destroy(item.item.id); } catch (e) { - errors.push(e); + errors.push({ + id: item.item.id, + error: e instanceof Error ? e.message : String(e) + }); } } if (errors.length > 0) { - console.error("Errors occurred during row destruction:", errors); + console.error( + `Errors occurred during row ${destroyItems ? 'destruction' : 'detachment'}:`, + errors.map(({id, error}) => `ID ${id.description}: ${error}`).join('\n') + ); }e2e-pw/src/oss/poms/sidebar.ts (1)
189-209: Add JSDoc documentation and consider timeout parameters.The assertion methods are well-structured but could benefit from:
- JSDoc documentation describing the purpose and parameters of each method.
- Optional timeout parameters for assertions, especially useful in slower environments.
Example improvement:
+ /** + * Asserts that a field has a visible query performance indicator + * @param fieldName - The name of the field to check + * @param timeoutMs - Optional timeout in milliseconds + */ async assertFieldHasQueryPerformance(fieldName: string) { - await expect(this.sb.queryPerformance(fieldName)).toBeVisible(); + await expect(this.sb.queryPerformance(fieldName)).toBeVisible({ timeout: 5000 }); }app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (2)
38-38: Optimize tooltip implementation for better performance and cleaner DOM.The tooltip implementation can be improved in several ways:
- Remove unnecessary span wrapper and spaces
- Memoize tooltip components to prevent unnecessary re-renders
Apply these changes:
- <span> {content} </span> + {content}Consider memoizing the tooltip component:
const TooltipCell = React.memo(({ tooltip, children }) => ( <Tooltip title={tooltip} arrow> {children} </Tooltip> ));Also applies to: 188-189, 205-205, 208-208
194-196: Optimize cell background styling for better performance.The current implementation creates a new style object on every render.
Consider memoizing the style object:
const getCellStyle = React.useCallback( (isSelected: boolean) => ({ background: isSelected ? selectedCellColor : 'inherit' }), [selectedCellColor] );Then use it in the TableCell:
sx={getCellStyle(isSelected)}fiftyone/operators/builtins/panels/model_evaluation/__init__.py (1)
357-357: LGTM! Bug fix for pending evaluations storage.The change correctly uses
pending_evaluations_in_storeinstead ofpending_evaluations, fixing a bug where completed evaluations could be incorrectly restored.Consider these improvements:
- Remove the unused
skip_updateparameter- Add docstring explaining the method's purpose and parameters
- def load_pending_evaluations(self, ctx, skip_update=False): + def load_pending_evaluations(self, ctx): + """Load and update pending model evaluations. + + Args: + ctx: The evaluation context object + """app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (3)
1353-1353: Consider consolidating dialog state management.The dialog close handlers share similar patterns and could be consolidated into a single reusable function to improve maintainability.
Consider refactoring the dialog management like this:
-const closeClassPerformanceConfigDialog = () => { - setClassPerformanceDialogConfig((state) => ({ ...state, open: false })); -}; -const closeConfusionMatrixConfigDialog = () => { - setConfusionMatrixDialogConfig((state) => ({ ...state, open: false })); -}; +const createDialogCloseHandler = (setConfig) => () => { + setConfig((state) => ({ ...state, open: false })); +}; + +const closeClassPerformanceConfigDialog = createDialogCloseHandler(setClassPerformanceDialogConfig); +const closeConfusionMatrixConfigDialog = createDialogCloseHandler(setConfusionMatrixDialogConfig);
Line range hint
40-50: Add error boundaries and memoize callback functions.The component handles complex user interactions and state updates. Consider these improvements:
- Wrap the component with an error boundary to gracefully handle runtime errors
- Memoize callback functions using
useCallbackto optimize performanceExample implementation:
// Add error boundary class EvaluationErrorBoundary extends React.Component<{children: React.ReactNode}> { componentDidCatch(error: Error) { console.error('Error in Evaluation component:', error); } render() { return this.props.children; } } // Memoize callbacks const handleExpandChange = useCallback((e: React.SyntheticEvent, expanded: boolean) => { setExpanded(expanded ? "summary" : ""); }, []);
Line range hint
1467-1479: Enhance type safety and documentation.The type definitions lack proper documentation and could benefit from stronger type safety.
Add proper JSDoc documentation and improve type safety:
/** * Configuration options for plot visualization * @property sortBy - Sorting criteria for the plot * @property limit - Maximum number of items to display * @property log - Whether to use logarithmic scale */ type PLOT_CONFIG_TYPE = { sortBy?: 'default' | 'az' | 'za' | 'best' | 'worst' | 'mc' | 'lc'; limit?: number; log?: boolean; }; /** * Extended configuration type for plot dialog * @extends PLOT_CONFIG_TYPE * @property open - Dialog open state */ type PLOT_CONFIG_DIALOG_TYPE = PLOT_CONFIG_TYPE & { open?: boolean; };docs/source/user_guide/config.rst (1)
672-678: LGTM! Consider adding example values for clarity.The documentation for the new query performance configuration options is well-structured and maintains consistency with the existing format. The descriptions are clear and the cross-references are properly formatted.
Consider adding example values in the descriptions to make it even clearer for users. For instance:
- Default if a user hasn't selected a query performance mode in their current session. + Default if a user hasn't selected a query performance mode in their current session (e.g., True enables optimized query performance by default).docs/source/user_guide/app.rst (2)
403-407: Consider adding a visual example of the lightning bolt icon.Adding a small screenshot showing the lightning bolt icon would help users quickly identify optimized filters in the UI.
587-589: Consider adding a warning about data persistence.While the note about persistence is good, consider adding a warning that changes are saved to the dataset itself and may affect other users accessing the same dataset.
docs/source/release-notes.rst (1)
2228-2230: LGTM! Consider adding performance metrics.The documentation update clearly communicates that sidebar filters can now leverage indexes for improved performance. This is a valuable addition to the release notes.
Consider adding specific performance metrics or examples to quantify the improvement, e.g. "Queries are now 10x faster when filtering indexed fields" to help users better understand the impact.
e2e-pw/src/oss/specs/sidebar/query-performance.spec.ts (2)
197-217: Refactor repeated code for waiting on animationsThe code for waiting on animations after clicking field dropdowns is duplicated for each field. Consider refactoring this into a loop to improve maintainability and reduce redundancy.
Apply this diff to refactor the code:
- let animation = eventUtils.getEventReceivedPromiseForPredicate( - "animation-onRest", - () => true - ); - await sidebar.clickFieldDropdown("inf_label_list"); - await animation; - - animation = eventUtils.getEventReceivedPromiseForPredicate( - "animation-onRest", - () => true - ); - await sidebar.clickFieldDropdown("nan_label_list"); - await animation; - - animation = eventUtils.getEventReceivedPromiseForPredicate( - "animation-onRest", - () => true - ); - await sidebar.clickFieldDropdown("ninf_label_list"); - await animation; + const fields = ["inf_label_list", "nan_label_list", "ninf_label_list"]; + for (const field of fields) { + const animation = eventUtils.getEventReceivedPromiseForPredicate( + "animation-onRest", + () => true + ); + await sidebar.clickFieldDropdown(field); + await animation; + }
147-147: Consider indexing only necessary fields to improve test setup performanceUsing
dataset.create_index("$**")indexes all fields, which may be unnecessary and could slow down the test setup. Consider indexing only the fields required for the test to improve efficiency.Apply this diff to specify the fields to index:
- dataset.create_index("$**") + dataset.create_index(["tags", "metadata.mime_type", "id", "filepath", "created_at", "last_modified_at", "bool", "bool_list", "str", "str_list"])
🛑 Comments failed to post (10)
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx (1)
16-19: 💡 Codebase verification
data-cyattribute name should be revised for consistencyThe current
query-performancedata-cy attribute doesn't align with existing performance-related selectors in the codebase. The only similar attribute found isbtn-dismiss-query-performance-toastin QueryPerformanceToast.tsx, which follows a more descriptive naming pattern. Consider updating the attribute to be more specific about its purpose, such asicon-query-performanceorsidebar-query-performance.🔗 Analysis chain
Verify consistent usage of data-cy attribute
The addition of the data-cy attribute improves testability. Let's verify the consistency of this attribute name across related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of the query-performance data-cy attribute # across components to ensure testing consistency # Search for query-performance data-cy usage rg 'data-cy=["'\'']query-performance["'\'']' --type ts --type tsx # Search for similar performance-related data-cy attributes rg 'data-cy=["'\''].*performance.*["'\'']' --type ts --type tsxLength of output: 189
Script:
#!/bin/bash # Let's try without file type specifications since tsx might not be recognized # Search for query-performance data-cy usage rg 'data-cy=["'\'']query-performance["'\'']' # Search for similar performance-related data-cy attributes rg 'data-cy=["'\''].*performance.*["'\'']' # Also search for the LightningIcon component usage to understand the context rg '<LightningIcon'Length of output: 331
app/packages/core/src/plugins/SchemaIO/components/TimerView.tsx (3)
6-12: 🛠️ Refactor suggestion
Strengthen type safety for TimerViewParams
The current type definition allows both
intervalandtimeoutto be undefined, which could lead to runtime warnings. Consider using a discriminated union type to enforce at least one timing parameter.-export type TimerViewParams = { - on_interval?: string; - on_timeout?: string; - interval?: number; - timeout?: number; - params?: object; -}; +type BaseTimerParams = { + params?: Record<string, unknown>; +}; + +type IntervalTimerParams = BaseTimerParams & { + on_interval: string; + interval: number; + on_timeout?: never; + timeout?: never; +}; + +type TimeoutTimerParams = BaseTimerParams & { + on_timeout: string; + timeout: number; + on_interval?: never; + interval?: never; +}; + +export type TimerViewParams = IntervalTimerParams | TimeoutTimerParams;📝 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.type BaseTimerParams = { params?: Record<string, unknown>; }; type IntervalTimerParams = BaseTimerParams & { on_interval: string; interval: number; on_timeout?: never; timeout?: never; }; type TimeoutTimerParams = BaseTimerParams & { on_timeout: string; timeout: number; on_interval?: never; interval?: never; }; export type TimerViewParams = IntervalTimerParams | TimeoutTimerParams;
21-53: 🛠️ Refactor suggestion
Add input validation and reduce code duplication
The timer implementations could benefit from:
- Validation of interval/timeout values
- Shared cleanup logic
- Browser-safe typing
+type TimeoutRef = ReturnType<typeof setTimeout>; + class Timer { - protected _ref: NodeJS.Timeout | null = null; + protected _ref: TimeoutRef | null = null; + + protected validateTime(ms: number): void { + if (ms <= 0) { + throw new Error('Time must be a positive number'); + } + } + + protected cleanup(): void { + if (this._ref) { + clearTimeout(this._ref); + this._ref = null; + } + } } class IntervalTimer extends Timer { constructor(private interval: number, private onInterval: () => void) { super(); + this.validateTime(interval); } start() { - this.stop(); + this.cleanup(); this._ref = setInterval(() => this.onInterval(), this.interval); } stop() { - if (this._ref) { - clearInterval(this._ref); - this._ref = null; - } + this.cleanup(); } }Committable suggestion skipped: line range outside the PR's diff.
111-117: 🛠️ Refactor suggestion
Improve type safety in TimerView component
The current implementation uses unsafe type casting. Consider adding runtime validation.
+const isTimerViewParams = (view: unknown): view is TimerViewParams => { + if (typeof view !== 'object' || !view) return false; + const v = view as Record<string, unknown>; + return ( + (typeof v.interval === 'number' && typeof v.on_interval === 'string') || + (typeof v.timeout === 'number' && typeof v.on_timeout === 'string') + ); +}; + export default function TimerView(props: ViewPropsType) { const { schema } = props; const { view = {} } = schema; - useTimer(view as TimerViewParams); + if (!isTimerViewParams(view)) { + if (process.env.NODE_ENV === 'development') { + console.warn('Invalid timer view parameters:', view); + } + return null; + } + + useTimer(view); return null; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const isTimerViewParams = (view: unknown): view is TimerViewParams => { if (typeof view !== 'object' || !view) return false; const v = view as Record<string, unknown>; return ( (typeof v.interval === 'number' && typeof v.on_interval === 'string') || (typeof v.timeout === 'number' && typeof v.on_timeout === 'string') ); }; export default function TimerView(props: ViewPropsType) { const { schema } = props; const { view = {} } = schema; if (!isTimerViewParams(view)) { if (process.env.NODE_ENV === 'development') { console.warn('Invalid timer view parameters:', view); } return null; } useTimer(view); return null; }e2e-pw/src/oss/poms/sidebar.ts (1)
45-50: 💡 Codebase verification
The data-testid "query-performance" implementation exists but uses data-cy instead
The search results show that the query performance indicator is implemented in the codebase but uses the attribute
data-cyinstead ofdata-testid:// app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx data-cy={"query-performance"}This mismatch between the test's expected attribute (
data-testid) and the actual implementation (data-cy) could cause test failures.
- Update either the test locator to use
data-cyor the component to usedata-testidfor consistency- Location:
e2e-pw/src/oss/poms/sidebar.ts🔗 Analysis chain
Consider adding parameter validation and verifying data-testid existence.
The method looks well-structured, but consider these improvements:
- Add validation for the
fieldNameparameter to handle empty strings.- Verify that the 'query-performance' data-testid exists in your application.
queryPerformance(fieldName: string, filterType?: "categorical" | "numeric") { + if (!fieldName?.trim()) { + throw new Error("fieldName cannot be empty"); + } const locator = filterType ? this.filter(fieldName, filterType) : this.fieldContainer(fieldName); return locator.getByTestId("query-performance"); }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of query-performance data-testid in the codebase # Search for query-performance data-testid usage rg -g '*.{tsx,jsx,html}' 'data-testid="query-performance"'Length of output: 60
Script:
#!/bin/bash # Let's broaden our search to include TypeScript files and check for both the data-testid attribute # and any test files that might be asserting this attribute # Search in all relevant file types rg -g '*.{tsx,jsx,ts,js,html}' 'query-performance' # Also check the test files specifically for this sidebar component fd -g '*sidebar*.test.{ts,tsx,js,jsx}' --exec cat {}Length of output: 15018
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
50-69: 🛠️ Refactor suggestion
Improve type safety in getRowActions implementation.
The current implementation uses 'any' type and has an unused parameter.
Consider these improvements:
- const computedRowActions = [] as any; + interface RowAction { + name: string; + on_click: string; + rows?: Record<number, boolean>; + } + const computedRowActions: RowAction[] = []; - onClick: (action, e) => { + onClick: () => {Committable suggestion skipped: line range outside the PR's diff.
fiftyone/operators/builtin.py (1)
1106-1110: 🛠️ Refactor suggestion
Add error handling for group field access.
While the implementation is logically correct, it should handle cases where the group field structure might not be as expected.
Consider adding validation:
if ctx.dataset.media_type == fom.GROUP and group_index: + if not hasattr(ctx.dataset, 'group_field'): + raise ValueError("Dataset is missing group_field attribute") + group_path = ctx.dataset.group_field + ".name" + + # Verify the group field exists in the schema + if group_path not in ctx.dataset.get_field_schema(flat=True): + raise ValueError(f"Group field path '{group_path}' not found in schema") + index_spec = [(group_path, 1), (field_name, 1)] ctx.dataset.create_index(index_spec, unique=unique, wait=False)📝 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 ctx.dataset.media_type == fom.GROUP and group_index: if not hasattr(ctx.dataset, 'group_field'): raise ValueError("Dataset is missing group_field attribute") group_path = ctx.dataset.group_field + ".name" # Verify the group field exists in the schema if group_path not in ctx.dataset.get_field_schema(flat=True): raise ValueError(f"Group field path '{group_path}' not found in schema") index_spec = [(group_path, 1), (field_name, 1)] ctx.dataset.create_index(index_spec, unique=unique, wait=False)docs/source/user_guide/app.rst (1)
409-412:
⚠️ Potential issueFix typo in "comprehensive".
-widgets are shown that include comphrensive counts +widgets are shown that include comprehensive counts📝 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.When a view is present and indexes are no longer applicable, granular filter widgets are shown that include comprehensive counts. Granular filters can be toggled for a dataset via the settings "Gear" or `enable_query_performance` and `disabled_query_performance` operators.e2e-pw/src/oss/specs/sidebar/query-performance.spec.ts (2)
198-199:
⚠️ Potential issueSpecify the predicate function in
getEventReceivedPromiseForPredicateto avoid potential flakinessThe predicate function currently always returns
true, which may cause the promise to resolve on any'animation-onRest'event, not necessarily the one related to the field dropdown you just clicked. This could lead to flakiness if other animations are occurring at the same time. Consider making the predicate more specific by checking that the event pertains to the field you are interacting with.Also applies to: 205-206, 212-213
147-149:
⚠️ Potential issueSave the sample before creating the index to ensure the index includes all data
Currently,
fourth.save()is called after creating the index. This may result in the fourth sample's data not being included in the index. To ensure that all data is indexed, save the sample before creating the index.Apply this diff to fix the issue:
+ fourth.save() dataset.create_index("$**") - fourth.save()📝 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.fourth.save() dataset.create_index("$**")
|
|
||
| if ctx.dataset.media_type == fom.GROUP and group_index: | ||
| group_path = ctx.dataset.group_field + ".name" | ||
| index_spec = [(group_path, 1), (field_name, 1)] |
No description provided.