Lightning filter fixes #4757
Lightning filter fixes #4757
Conversation
WalkthroughThe changes involve modifications to several components within the codebase, including adjustments to conditional logic for loading indicators, minor formatting updates, alterations to object key definitions, the introduction of unit tests for a state management module, and updates to filtering logic in a selector function. These changes collectively enhance the functionality and maintainability of the code. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Checkboxes
participant LoadingIndicator
User->>Checkboxes: Trigger loading
Checkboxes->>LoadingIndicator: Check loading state
alt Loading with results
LoadingIndicator-->>User: Show loading indicator
else No results
LoadingIndicator-->>User: Show loading indicator
end
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
app/packages/state/src/recoil/lightning.ts (1)
172-172: Approve the modification in filtering logic.The change to use
schemaAtoms.dbPathfor path transformation before checking membership in theindexesset is a significant improvement. This ensures that paths are correctly resolved based on the schema's structure before being used in filtering operations.The code changes are approved.
Consider adding comments explaining the use of
schemaAtoms.dbPathin this context to aid future maintainability.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/packages/core/src/components/Filters/StringFilter/Checkboxes.tsx (1 hunks)
- app/packages/core/src/components/Filters/StringFilter/useSelected.ts (1 hunks)
- app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/FilterItem.tsx (1 hunks)
- app/packages/state/src/recoil/lightning.test.ts (1 hunks)
- app/packages/state/src/recoil/lightning.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/packages/core/src/components/Filters/StringFilter/useSelected.ts
Additional context used
Path-based instructions (4)
app/packages/state/src/recoil/lightning.test.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/FilterItem.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/Checkboxes.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/state/src/recoil/lightning.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 not posted (3)
app/packages/state/src/recoil/lightning.test.ts (1)
1-5: Proper setup of test environment.The use of
vitestand mocking ofrecoilandrecoil-relayensures that the tests are isolated and reproducible.app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/FilterItem.tsx (1)
26-26: Key change inFILTERSobject approved, verify consistency.The change from
"_LABEL_TAGS"to_LABEL_TAGSsimplifies the object key. Ensure that all references to this key across the codebase are updated to reflect this change.app/packages/core/src/components/Filters/StringFilter/Checkboxes.tsx (1)
183-183: Enhanced conditional logic for loading indicator.The updated condition for displaying the loading indicator is more comprehensive, improving user feedback during data loading scenarios.
| describe("tests lightning selectors", () => { | ||
| it("resolves wildcard indexed fields with database path", () => { | ||
| const test = <TestSelectorFamily<typeof lightning.lightningPaths>>( | ||
| (<unknown>lightning.lightningPaths("ground_truth")) | ||
| ); | ||
| setMockAtoms({ | ||
| dbPath: (p) => | ||
| p === "ground_truth.id" ? "ground_truth._id" : "ground_truth.label", | ||
| expandPath: () => "ground_truth", | ||
| fieldPaths: () => ["id", "label"], | ||
| indexesByPath: new Set(["ground_truth._id", "ground_truth.label"]), | ||
| isLabelPath: () => true, | ||
| }); | ||
|
|
||
| expect(test()).toEqual(new Set(["ground_truth.id", "ground_truth.label"])); | ||
| }); |
There was a problem hiding this comment.
Well-implemented test case for lightningPaths selector.
The test case effectively checks the selector's ability to resolve database paths. Consider adding more test cases to cover edge cases and error handling scenarios.
Would you like me to help in writing additional test cases?
benjaminpkane
force-pushed
the
bugfix/lightning-filters
branch
from
79d519a to
eba6938
Compare
| @@ -180,7 +180,7 @@ const Checkboxes = ({ | |||
| const show = useRecoilValue(showSearchSelector({ modal, path })); | |||
| const getCount = useGetCount(modal, path); | |||
|
|
|||
| if (loading) { | |||
| if (loading || (!show && results === null)) { | |||
There was a problem hiding this comment.
probably we should add a comment here to explain this
|
If this is fixes, should this target release/v0.25.1? |
|
Could use more testing. Converting to draft |
What changes are proposed in this pull request?
Indexed id fields are incorrectly included as (unindexed) subfilters in lightning mode
Also fixes loading state with boolean lightning filters.
Note that indexed ObjectId fields offer indexed matches/querying but do not offer ixscan substring prefix search, so search results are not enabled. Only exact 24 hex character searches are allowed
Before
Screen.Recording.2024-08-30.at.9.14.46.AM.mov
After
Screen.Recording.2024-08-30.at.9.15.12.AM.mov
How is this patch tested? If it is not, please explain why.
Selector test
Release Notes
idfields to lightning filtersWhat areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Tests