Initial query performance sidebar e2e #5162
Conversation
WalkthroughThe pull request introduces several modifications across multiple components. Notably, the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx (1)
16-19: Consider moving inline styles to styled-componentsWhile the addition of the data-cy attribute is good for testing, the inline styles could be moved to the styled component for better maintainability.
const LightningIcon = styled(Bolt)` color: ${({ theme }) => theme.text.secondary}; + height: 16px; + margin-right: 2px; + width: 16px; `; export const LightningBolt: React.FC = (_) => { return ( <Tooltip placement="top-center" text={"Indexed"}> - <LightningIcon - data-cy={"query-performance"} - style={{ height: 16, marginRight: 2, width: 16 }} - /> + <LightningIcon data-cy={"query-performance"} /> </Tooltip> ); };app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx (3)
53-56: Consider enhancing the data-cy attribute patternWhile adding the data-cy attribute is good for testing, consider making it more specific and robust:
- The current pattern
numeric-filter-${path}might be problematic if the path contains characters that make invalid CSS selectors- Consider adding additional data attributes for more granular testing scenarios
<Container - data-cy={`numeric-filter-${path}`} + data-cy={`numeric-filter-${path.replace(/[^a-zA-Z0-9-]/g, '-')}`} + data-testid="numeric-filter" + data-filter-path={path} onClick={(e) => e.stopPropagation()} >
Line range hint
1-93: Add ARIA attributes for better accessibilityThe component could benefit from improved accessibility, especially for the filter controls and performance indicators.
- Add aria-label to the Container:
<Container data-cy={`numeric-filter-${path}`} + aria-label={`Numeric filter for ${name}`} onClick={(e) => e.stopPropagation()} >
- Add aria-live for the LightningBolt performance indicator:
{showQueryPerformanceIcon && ( + <span aria-live="polite" aria-label="Query is performant"> <LightningBolt /> + </span> )}
Line range hint
32-52: Consider memoizing expensive computationsThe field value retrieval and path splitting operations could be memoized to optimize performance, especially for frequently updating components.
const NumericFieldFilter = ({ color, modal, named = true, path }: Props) => { - const name = path.split(".").slice(-1)[0]; + const name = React.useMemo(() => path.split(".").slice(-1)[0], [path]); const [showRange, setShowRange] = React.useState(!named); - const field = fos.useAssertedRecoilValue(fos.field(path)); + const field = fos.useAssertedRecoilValue(React.useMemo(() => fos.field(path), [path]));e2e-pw/src/oss/poms/sidebar.ts (1)
189-209: LGTM! Comprehensive test coverage for query performance indicators.The assertion methods are well-designed:
- Cover both positive (visible) and negative (hidden) cases
- Handle both fields and subfields consistently
- Follow DRY principle by reusing the
queryPerformancemethodHowever, consider adding JSDoc comments to document the purpose and parameters of these public methods.
Apply this diff to improve documentation:
+ /** + * Asserts that a field has a visible query performance indicator + * @param fieldName The name of the field to check + */ async assertFieldHasQueryPerformance(fieldName: string) { await expect(this.sb.queryPerformance(fieldName)).toBeVisible(); } + /** + * Asserts that a field's query performance indicator is hidden + * @param fieldName The name of the field to check + */ async assertFieldMissingQueryPerformance(fieldName: string) { await expect(this.sb.queryPerformance(fieldName)).toBeHidden(); } + /** + * Asserts that a subfield has a visible query performance indicator + * @param fieldName The name of the field to check + * @param filterType Optional filter type (categorical or numeric) + */ async assertSubfieldHasQueryPerformance( fieldName: string, filterType?: "categorical" | "numeric" ) { await expect(this.sb.queryPerformance(fieldName, filterType)).toBeVisible(); } + /** + * Asserts that a subfield's query performance indicator is hidden + * @param fieldName The name of the field to check + * @param filterType Optional filter type (categorical or numeric) + */ async assertSubfieldMissingQueryPerformance( fieldName: string, filterType?: "categorical" | "numeric" ) { await expect(this.sb.queryPerformance(fieldName, filterType)).toBeHidden(); }e2e-pw/src/oss/specs/sidebar/query-performance.spec.ts (3)
28-149: Refactor dataset setup to eliminate code duplicationThe dataset setup code contains repeated blocks for each sample (
first,second,third,fourth). This repetition can be reduced by refactoring the code to use loops or helper functions, improving maintainability and readability.Consider refactoring like this:
samples = [ { "sample": dataset.first(), "bool": False, "bool_list": [False], "inf": float("inf"), "inf_list": [float("inf")], "inf_label_list": fo.Classifications( classifications=[fo.Classification(label="label", confidence=float("inf"))] ), "nan": float("nan"), "nan_list": [float("nan")], "nan_label_list": fo.Classifications( classifications=[fo.Classification(label="label", confidence=float("nan"))] ), "ninf": float("-inf"), "ninf_list": [float("-inf")], "ninf_label_list": fo.Classifications( classifications=[fo.Classification(label="label", confidence=float("-inf"))] ), "str": "0", "str_list": ["0"], }, { "sample": dataset.skip(1).first(), "bool": False, "bool_list": [False], "inf": 1.0, "inf_list": [1.0], "inf_label_list": fo.Classifications( classifications=[fo.Classification(label="label", confidence=1.0)] ), "nan": 1.0, "nan_list": [1.0], "nan_label_list": fo.Classifications( classifications=[fo.Classification(label="label", confidence=1.0)] ), "ninf": 1.0, "ninf_list": [1.0], "ninf_label_list": fo.Classifications( classifications=[fo.Classification(label="label", confidence=1.0)] ), "str": "1", "str_list": ["1"], }, # Add dictionaries for 'third' and 'fourth' samples similarly ] for data in samples: sample = data.pop("sample") for key, value in data.items(): sample[key] = value sample.save()This approach reduces repetition and makes it easier to adjust the test data in the future.
197-217: Refactor repeated animation waiting logic into a helperThe code for waiting on animations and clicking field dropdowns is repeated for each field. Refactoring this logic into a helper function will reduce duplication and enhance code clarity.
Consider creating a helper function:
async function clickFieldAndWaitForAnimation(fieldName: string) { const animation = eventUtils.getEventReceivedPromiseForPredicate( "animation-onRest", () => true ); await sidebar.clickFieldDropdown(fieldName); await animation; } // Usage: await clickFieldAndWaitForAnimation("inf_label_list"); await clickFieldAndWaitForAnimation("nan_label_list"); await clickFieldAndWaitForAnimation("ninf_label_list");
225-229: Enhance clarity by combining similar loopsThe nested loops for asserting subfields with query performance can be combined or restructured for better readability and maintainability.
Consider restructuring the loops:
const fields = [ "inf_label_list", "nan_label_list", "ninf_label_list", ]; const subfields = { indexed: ["id", "label", "tags"], unindexed: ["confidence"], }; for (const field of fields) { for (const subfield of subfields.indexed) { await sidebar.asserter.assertSubfieldHasQueryPerformance( `${field}.classifications.${subfield}`, "categorical" ); } for (const subfield of subfields.unindexed) { await sidebar.asserter.assertSubfieldMissingQueryPerformance( `${field}.classifications.${subfield}`, "categorical" ); } }This refactor reduces code duplication and makes the intent clearer.
Also applies to: 239-243
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx(1 hunks)app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx(2 hunks)app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx(1 hunks)e2e-pw/src/oss/poms/action-row/display-options.ts(2 hunks)e2e-pw/src/oss/poms/sidebar.ts(2 hunks)e2e-pw/src/oss/specs/sidebar/query-performance.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.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.
e2e-pw/src/oss/poms/action-row/display-options.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.
e2e-pw/src/oss/poms/sidebar.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.
e2e-pw/src/oss/specs/sidebar/query-performance.spec.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 (8)
e2e-pw/src/oss/poms/action-row/display-options.ts (2)
6-6: Well-defined type with semantic values
The QueryPerformanceMode type follows TypeScript best practices and uses semantically correct values ("enabled"/"disabled") that align well with the boolean state they represent.
15-18: LGTM! Consistent implementation pattern
The setQueryPerformance method:
- Follows the established pattern of other setter methods in the class
- Uses consistent test ID format (
qp-mode-${mode}) - Maintains the same promise-based click interaction pattern
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx (1)
9-11: LGTM! Good encapsulation of the LightningIcon component
The conversion to a local constant improves component encapsulation while maintaining proper theme integration.
app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx (1)
Line range hint 1-93: Verify query performance indicator behavior
Let's verify the query performance indicator implementation across the codebase to ensure consistent behavior.
✅ Verification successful
Query performance indicator implementation is consistent
The implementation of the query performance indicator (LightningBolt) is consistent across the codebase. The indicator is shown when:
- Query performance mode is enabled (
queryPerformanceis true) - The field is indexed (
pathHasIndexesis true) - Not in modal context (
modalis false) - Not a frame field (
isFrameFieldis false)
This behavior is consistently implemented in:
- NumericFieldFilter (via useShow hook)
- StringFilter component
- FilterablePathEntry component
- FieldLabelAndInfo component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using LightningBolt and verify consistent implementation
echo "Checking LightningBolt usage patterns:"
rg -A 3 "LightningBolt" --type ts --type tsx
# Check for other query performance related implementations
echo "\nChecking query performance related patterns:"
rg -A 3 "showQueryPerformanceIcon|useQueryPerformanceTimeout" --type ts --type tsx
Length of output: 375
Script:
#!/bin/bash
# Search for LightningBolt usage patterns with correct file extension
echo "Checking LightningBolt usage patterns:"
rg -A 3 "LightningBolt" -t typescript -t javascriptreact -t typescriptreact
echo -e "\nChecking query performance related patterns:"
rg -A 3 "showQueryPerformanceIcon|useQueryPerformanceTimeout" -t typescript -t javascriptreact -t typescriptreact
# Search for the implementation of useShow hook
echo -e "\nChecking useShow hook implementation:"
fd -e ts -e tsx "use-show" --exec cat {}
Length of output: 1397
Script:
#!/bin/bash
# Search with simpler file extensions
echo "Checking LightningBolt usage patterns:"
rg -A 3 "LightningBolt" .
echo -e "\nChecking query performance state usage:"
rg -A 3 "queryPerformance" .
echo -e "\nChecking pathHasIndexes usage:"
rg -A 3 "pathHasIndexes" .
echo -e "\nChecking isFrameField usage:"
rg -A 3 "isFrameField" .
Length of output: 44102
app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx (2)
8-8: LGTM! Import follows React/TypeScript best practices.
The import statement follows proper conventions and maintains consistency with the project structure.
106-106: Verify LightningBolt component props requirements.
The icon replacement aligns with the PR objectives. However, let's verify if the LightningBolt component requires any props for proper rendering or accessibility.
✅ Verification successful
Let me check the LightningBolt component definition to verify if it requires any props.
Let me try a different approach to find the LightningBolt component definition.
Let me check the imports and usage context in these files to locate the LightningBolt component definition.
Let me check the LightningBolt component definition in the Icon.tsx file where it's exported from.
LightningBolt component is properly implemented without requiring props
The LightningBolt component is correctly used without props as it's a self-contained component that:
- Wraps a styled MUI Bolt icon with predefined dimensions and styling
- Includes a Tooltip for accessibility with the text "Indexed"
- Has proper data-cy attribute for testing
The current usage in StringFilter.tsx aligns with the component's implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if LightningBolt component requires any props
# Search for LightningBolt component definition and usage
ast-grep --pattern 'export const LightningBolt = $_'
# Check other usages of LightningBolt for prop patterns
rg -A 2 '<LightningBolt'
Length of output: 1108
Script:
#!/bin/bash
# Search for LightningBolt component definition
ast-grep --pattern 'const LightningBolt = $_' --lang typescript
# Search for LightningBolt type or interface definitions
rg 'type.*LightningBolt|interface.*LightningBolt'
Length of output: 117
Script:
#!/bin/bash
# Search for files containing LightningBolt
fd LightningBolt
# Search for any file containing LightningBolt definition or import
rg -l "LightningBolt"
# Try to find the component in icons directory if it exists
fd -t f . "icons" -x rg -l "LightningBolt" {}
Length of output: 411
Script:
#!/bin/bash
# Check imports and surrounding context in the files that use LightningBolt
rg -B 5 "import.*LightningBolt|LightningBolt.*from" app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx
Length of output: 1675
Script:
#!/bin/bash
# Get the content of the Icon.tsx file to check LightningBolt definition
cat app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx
Length of output: 2031
e2e-pw/src/oss/poms/sidebar.ts (1)
45-50: LGTM! Clean implementation of the Page Object Model pattern.
The queryPerformance method is well-structured and follows best practices:
- Reuses existing locator methods
- Provides flexibility with optional filterType
- Returns a specific test ID for consistent testing
e2e-pw/src/oss/specs/sidebar/query-performance.spec.ts (1)
147-147: Verify that indexing all fields is necessary
Using dataset.create_index("$**") creates an index on all fields, which may impact performance. Confirm that indexing all fields is required for the test or consider indexing only the necessary fields.
Run the following script to list all indexed fields:
Ensure that only the required fields are indexed to optimize performance.
✅ Verification successful
Indexing all fields is necessary for this test case
This is a test specifically designed to verify query performance indicators in the UI, where it checks both indexed and non-indexed fields. The test asserts the presence and absence of query performance icons for various fields and their subfields. The wildcard index $** is intentionally used to support these test assertions.
- The test verifies query performance indicators for multiple field types including primitive fields, lists, and nested classification objects
- It explicitly checks both indexed fields (like 'tags', 'bool', 'str') and non-indexed fields (like 'metadata.size_bytes', 'inf', 'nan')
- The test also verifies subfield indexing behavior for classification objects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List all indexed fields in the dataset.
# Navigate to the dataset directory and list indexed fields
python -c "
import fiftyone as fo
dataset = fo.load_dataset('${datasetName}')
print('Indexed fields:', dataset.get_index_information())
"
Length of output: 204
Script:
#!/bin/bash
# Let's analyze the test file to understand the context and usage of indexing
rg -B 5 -A 5 "create_index" e2e-pw/src/oss/specs/sidebar/query-performance.spec.ts
# Also search for any query operations that might explain why all fields are indexed
rg -B 3 -A 3 "find|aggregate" e2e-pw/src/oss/specs/sidebar/query-performance.spec.ts
Length of output: 359
Script:
#!/bin/bash
# Let's look at the broader context of the test file to understand the purpose
cat e2e-pw/src/oss/specs/sidebar/query-performance.spec.ts
Length of output: 6681
What changes are proposed in this pull request?
Adds basic coverage asserting filters are correctly indicated as performant or not (lightning bolt icon)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Tests