Merge release/v1.1.0 to develop
#5126
Conversation
* modal checks * still allow modal frame filters when frame filtering is disabled * fix logic
* improve float tests * lightning _first fix * None handling
WalkthroughThe changes in this pull request involve modifications to several components within the application, particularly focusing on the handling of the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx (1)
Line range hint
43-63: Consider enhancing type safety with discriminated unionsThe component's behavior significantly differs based on the combination of multiple boolean flags (
queryPerformance,indexed,modal,frameField). Consider using a discriminated union type to make these relationships more explicit and type-safe.type IconState = | { type: 'lightning'; path: string; frameFilteringDisabled: boolean } | { type: 'arrow'; path: string; disabled: boolean; frameFilteringDisabled: boolean }; function determineIconState(props: { queryPerformance: boolean; indexed: boolean; modal: boolean; frameField: boolean; path: string; disabled: boolean; frameFilteringDisabled: boolean; }): IconState { const { queryPerformance, indexed, modal, frameField, path, disabled, frameFilteringDisabled } = props; if (queryPerformance && indexed && !modal && !frameField) { return { type: 'lightning', path, frameFilteringDisabled }; } return { type: 'arrow', path, disabled, frameFilteringDisabled }; }app/packages/core/src/components/Sidebar/Entries/EntryCounts.tsx (2)
44-44: Consider extracting the modal-dependent query performance logicThe condition
!modalis now coupled with query performance check. Consider extracting this logic into a separate selector for better reusability and maintainability.-const queryPerformance = useRecoilValue(fos.queryPerformance) && !modal; +const modalAwareQueryPerformance = selectorFamily<boolean, { modal: boolean }>({ + key: "modalAwareQueryPerformance", + get: (params) => ({get}) => { + return get(fos.queryPerformance) && !params.modal; + } +}); +const queryPerformance = useRecoilValue(modalAwareQueryPerformance({modal}));
Line range hint
9-13: Enhance type safety with literal type for modalThe
modalprop could benefit from a more specific type definition since it's used in boolean operations.interface PathEntryCountsProps { path: string; - modal: boolean; + modal: true | false; }app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/useTitleTemplate.tsx (2)
100-103: Consider memoizing Recoil values to prevent unnecessary re-renders.The component reads multiple Recoil values which could trigger re-renders. Consider combining these values using a selector or memoizing them.
+ const titleState = useRecoilValue( + selector({ + key: 'titleState', + get: ({get}) => ({ + enabled: !get(fos.isDisabledCheckboxPath(path)), + isFilterMode: get(fos.isSidebarFilterMode), + expandedPath: get(fos.expandPath(path)) + }) + }) + ); - const enabled = !useRecoilValue(fos.isDisabledCheckboxPath(path)); - const isFilterMode = useRecoilValue(fos.isSidebarFilterMode); - const expandedPath = useRecoilValue(fos.expandPath(path));
Line range hint
1-107: Add TypeScript type definitions for better type safety.The hook parameters and return types should be explicitly typed for better maintainability and type safety.
+ interface HoverHandlers { + onMouseEnter: () => void; + onMouseLeave: () => void; + } + + interface TitleTemplateProps { + hoverHandlers: HoverHandlers; + hoverTarget: React.RefObject<HTMLSpanElement>; + container: React.RefObject<HTMLDivElement>; + } + - const useTitleTemplate = ({ + const useTitleTemplate = ({ modal, path, }: { modal: boolean; path: string; - }) => { + }): ((props: TitleTemplateProps) => JSX.Element) => { - return function useTitleTemplate({ hoverHandlers, hoverTarget, container }) { + return function useTitleTemplate({ hoverHandlers, hoverTarget, container }: TitleTemplateProps) {fiftyone/server/lightning.py (1)
482-488: Consider adding error handling for invalid field pathsThe prefix handling for frame fields is correct, but consider adding validation for the field path to prevent potential errors when accessing invalid paths.
Add error handling like this:
prefix = "" if is_frame_field: prefix = "frames." for key in keys: path = ".".join([path, key]) if path else key + full_path = f"{prefix}{path}" + if not dataset.has_field(full_path): + raise ValueError(f"Invalid field path: {full_path}") - field = dataset.get_field(f"{prefix}{path}") + field = dataset.get_field(full_path) while isinstance(field, fof.ListField):
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/packages/core/src/components/Sidebar/Entries/EntryCounts.tsx(1 hunks)app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx(1 hunks)app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/useTitleTemplate.tsx(2 hunks)fiftyone/server/lightning.py(3 hunks)tests/unittests/lightning_tests.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/core/src/components/Sidebar/Entries/EntryCounts.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.
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/useTitleTemplate.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.
🔇 Additional comments (6)
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Icon.tsx (1)
43-46: LGTM! The modal state handling looks correct.
The changes properly integrate the modal prop into the disabled state calculations, ensuring that filtering remains enabled during modal interactions. This aligns with the PR's objective of improving modal state handling across components.
app/packages/core/src/components/Sidebar/Entries/EntryCounts.tsx (1)
Line range hint 31-42: LGTM: Proper memoization of atom getter
Good use of useCallback for memoizing the atom getter function with correct dependencies.
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/useTitleTemplate.tsx (1)
81-81: LGTM! Improved readability with positive condition.
The change from disabled to enabled using negation makes the code more intuitive and easier to understand.
fiftyone/server/lightning.py (2)
468-472: LGTM: Enhanced result parsing logic
The updated parsing logic correctly handles both the new group stage structure (using "value" field) and maintains backward compatibility (fallback to "_id").
397-405: LGTM: Improved aggregation pipeline structure
The changes improve the robustness of the aggregation pipeline by:
- Ensuring correct ordering after unwinding arrays
- Using a more standard MongoDB aggregation pattern for grouping results
Let's verify the aggregation pipeline behavior with different field types:
✅ Verification successful
Let me search for the actual usage of _first function to understand how it's used with different field types.
Let me check how the _first function is used in the context of aggregation pipelines.
Based on the code analysis, I can now provide a final assessment of the changes to the _first function.
The aggregation pipeline changes are correct and improve robustness
The modifications to the _first function's aggregation pipeline are well-structured and handle edge cases appropriately:
- The sorting step is correctly placed at the start of the pipeline with
{"$sort": {path: sort}} - The
$groupstage with"_id": Noneand dynamic$min/$maxoperator is a standard MongoDB pattern for finding extreme values - The pipeline properly handles array fields through
_match_arrayshelper - Special handling for float values is preserved through
_handle_nonfinites
The changes maintain the function's core purpose while using more idiomatic MongoDB aggregation patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the _first function to ensure compatibility
# with different field types and array scenarios
# Search for field definitions that might use this aggregation
rg -B 2 -A 2 "ListField|ArrayField"
# Search for direct calls to _first to understand usage patterns
ast-grep --pattern '_first($path, $dataset, $sort, $is_frame_field)'
Length of output: 147373
Script:
#!/bin/bash
# Search for direct calls to _first function to understand its usage patterns
rg -B 5 -A 5 "_first\("
Length of output: 3034
Script:
#!/bin/bash
# Get the full implementation and usage context of _first function
rg -B 20 -A 20 "def _first\(" fiftyone/server/lightning.py
Length of output: 1031
tests/unittests/lightning_tests.py (1)
491-491: LGTM! Enhanced test coverage with boundary value.
The addition of 0.0 to the float lists improves test coverage by including an important boundary value. This change helps ensure that the lightning queries handle zero values correctly in combination with positive and negative numbers.
Let's verify that the test assertions properly handle these new test cases:
Also applies to: 493-493, 495-495, 497-497, 511-511, 515-515, 517-517
✅ Verification successful
Test coverage is properly verified
The test data structure and assertions are well-designed to verify the handling of zero values:
- The test includes comprehensive combinations of
0.0with positive and negative numbers in lists - The test data covers edge cases by mixing
0.0with special values (inf, -inf, nan) - The test uses
FloatLightningResulttype to verify numeric properties - The assertions validate the results through the GraphQL query response
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the test assertions cover the new test cases with zero values
# Expected: The assertions should check min/max values which would be affected by zero
rg -A 5 '"lightning",' tests/unittests/lightning_tests.py | rg -A 5 'FloatLightningResult'
Length of output: 90
Script:
#!/bin/bash
# Let's try a different approach to find the test assertions in the FloatLightningQueries class
rg -B 2 -A 10 "class TestFloatLightningQueries" tests/unittests/lightning_tests.py
Length of output: 457
Script:
#!/bin/bash
# Let's also check the specific test methods that might be using these float lists
rg -B 2 -A 15 "float_list=\[0\.0, -1\.0\]" tests/unittests/lightning_tests.py
Length of output: 639
Script:
#!/bin/bash
# Search for any assertions or result validations in the test file
rg "assert|\.result" tests/unittests/lightning_tests.py
Length of output: 237
Script:
#!/bin/bash
# Let's get more context around the assertions to see what they're actually checking
rg -B 5 -A 5 "assertListEqual" tests/unittests/lightning_tests.py
Length of output: 2014
Script:
#!/bin/bash
# Let's also check the complete test method to understand the full test flow
rg -B 10 -A 30 "async def test_floats" tests/unittests/lightning_tests.py
Length of output: 1351
Script:
#!/bin/bash
# Let's get the complete test method including the query and assertions for float fields
rg -B 5 -A 40 'async def test_floats' tests/unittests/lightning_tests.py
Length of output: 1413
* Allowing TableView to pass max_inline_actions as an arg Co-authored-by: minhtuevo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (1)
Line range hint
114-114: Consider making onClick optional in ActionPropsType.The type definition shows
onClickas required, but the code uses the optional chaining operator?.. Consider updating the type to match the implementation:type ActionPropsType = { name: string; label: string; - onClick: (action: ActionPropsType, e: React.MouseEvent) => void; + onClick?: (action: ActionPropsType, e: React.MouseEvent) => void; icon: string; variant: string; mode: "inline" | "menu"; color?: string; size?: SizeType; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/TableView.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.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/TableView.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.
🔇 Additional comments (3)
app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx (1)
21-21: LGTM! The condition change improves action visibility.
The updated condition actions.length <= maxInline is more intuitive than the previous exact match, allowing better utilization of inline actions when space permits.
Please verify the UI layout renders correctly with multiple inline actions:
✅ Verification successful
The condition change is safe and properly implemented
The code analysis reveals:
- The component handles both inline and overflow menu cases correctly
maxInlineprop is properly typed and has a default value ofDEFAULT_MAX_INLINE = 1- TableView uses the component with a configurable
max_inline_actionsprop - The UI implementation uses Material-UI's Stack with proper spacing for inline actions
- The component gracefully handles size variations with appropriate styling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files and snapshots that might need updating
rg -g "*.{test,spec}.{ts,tsx}" -l "ActionsMenu"
rg -g "__snapshots__/*.snap" -l "ActionsMenu"
Length of output: 242
Script:
#!/bin/bash
# Let's try a broader search for test files and related components
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts . app/packages/core/src/plugins/SchemaIO/
fd -e tsx -e ts . app/packages/core/src/plugins/SchemaIO/ --exec rg -l "ActionsMenu"
Length of output: 307
Script:
#!/bin/bash
# Let's examine how ActionsMenu is used in TableView and check the maxInline prop usage
rg -A 5 -B 5 "ActionsMenu" app/packages/core/src/plugins/SchemaIO/components/TableView.tsx
# Also check the ActionsMenu component implementation for default value and prop types
cat app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx
Length of output: 4018
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (2)
36-36: Implementation follows React best practices
The changes maintain consistent patterns with the existing codebase:
- Proper use of default values
- Consistent prop passing
- Maintains existing component structure
Also applies to: 202-202
202-202: Verify ActionsMenu implementation for maxInline prop
The prop name changes from snake_case (max_inline_actions) to camelCase (maxInline). Let's verify the ActionsMenu component implementation.
✅ Verification successful
Props naming convention is consistent in ActionsMenu implementation
The verification shows that the maxInline prop is properly defined and used within the ActionsMenu component:
- It's correctly typed as an optional number prop in the interface
- It's properly destructured from props with a default value
- The camelCase naming convention is consistently used within the component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ActionsMenu component definition and usage of maxInline prop
ast-grep --pattern 'interface $interface {
$$$
maxInline$_: $_
$$$
}'
# Find all usages of maxInline in ActionsMenu
rg -A 5 'maxInline' app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx
Length of output: 564
| @@ -33,6 +33,7 @@ export default function TableView(props: ViewPropsType) { | |||
| selected_color, | |||
| size = "small", | |||
| variant = "filled", | |||
| max_inline_actions = 1, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript type definition for max_inline_actions
The new prop should be properly typed in the view schema type definition to maintain type safety.
Add the type definition to the view schema interface:
interface ViewSchema {
// ... existing props
max_inline_actions?: number;
}
Merge
release/v1.1.0todevelopSummary by CodeRabbit
New Features
max_inline_actionsproperty in theTableViewcomponent for customizable inline actions.Bug Fixes
IconWrappercomponent.Tests