fiftyone
fiftyone copied to clipboard
Group modal sidebar label field aggregations
What changes are proposed in this pull request?
When latency is a concern for the App server, grouping the modal sidebar requests (aggregations for label fields) into two distinct network calls, samples and frames (if applicable), improves the loading experience.
This behavior is only targeted for the modal, where aggregation requests are only with respect to a sample or group.
How is this patch tested? If it is not, please explain why.
Recoil unit tests
Release Notes
- Optimized the expanded sample view sidebar
What areas of FiftyOne does this PR affect?
- [x] App: FiftyOne application changes
- [ ] Build: Build and test infrastructure changes
- [ ] Core: Core
fiftyonePython library changes - [ ] Documentation: FiftyOne documentation changes
- [ ] Other
Summary by CodeRabbit
Summary by CodeRabbit
- New Features
- Enhanced aggregation handling in the app to support different paths based on modal conditions.
- Introduced functionality to access label paths based on their expansion status.
- Refactor
- Improved the logic for filtering and finding within the aggregation selector.
- Renamed and modified selectors for better clarity and functionality.
- Tests
- Added test cases for aggregation path accumulation functionality.
Codecov Report
Attention: Patch coverage is 47.41379% with 61 lines in your changes are missing coverage. Please review.
Project coverage is 28.02%. Comparing base (
02c4d49) to head (08a6d1f). Report is 15 commits behind head on develop.
Additional details and impacted files
@@ Coverage Diff @@
## develop #4182 +/- ##
===========================================
+ Coverage 27.99% 28.02% +0.03%
===========================================
Files 814 814
Lines 101631 101701 +70
Branches 1178 1187 +9
===========================================
+ Hits 28453 28505 +52
- Misses 73178 73196 +18
| Flag | Coverage Δ | |
|---|---|---|
| app | 15.60% <47.41%> (+0.04%) |
:arrow_up: |
| python | 99.23% <ø> (ø) |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
experimental ai reviewer ...
@coderabbitai review
@swheaton, I've reviewed the changes in the pull request. Here are some observations and recommendations:
TypeScript Source Files
- aggregations.test.ts: The unit tests are well-structured and make good use of mocking for
recoilandrecoil-relay. It's great to see comprehensive tests for themodalAggregationPathsselector family. Ensure that all possible edge cases are covered, especially with complex data structures and asynchronous operations. - aggregations.ts: This file introduces several selector families for managing aggregations. The use of
graphQLSelectorFamilyfromrecoil-relayand the detailed JSDoc comments enhance readability and maintainability. Consider verifying the performance implications of these selectors, especially in scenarios with large datasets. - schema.ts: This file contains a comprehensive set of selectors and utilities for schema management. The code is well-organized, and the use of TypeScript types enhances code safety. Pay attention to potential performance bottlenecks when dealing with deeply nested structures or large schemas.
TypeScript Configuration File (tsconfig.json)
- The configuration promotes modern JavaScript features and ensures strict type-checking, which is beneficial for project maintainability and code quality. The inclusion of
"noUnusedLocals","noUnusedParameters", and"noImplicitReturns"options is commendable as it encourages cleaner code. However,"noImplicitAny": falsemight lead to less strict type checking in some cases. Consider enabling it if the project's type coverage is high enough to avoid potential runtime errors.
General Recommendations
- Ensure consistent coding styles and patterns across the project. Consider using tools like ESLint or Prettier if not already in use.
- Review the project's documentation to ensure that it's up-to-date with these changes, especially for public APIs and interfaces.
- Consider the impact of these changes on the overall architecture and any potential integration points with other parts of the system.
This review focuses on code quality, maintainability, and adherence to best practices. Depending on the project's specific requirements and context, additional considerations may apply.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the list of files changed in the pull request.
gh pr view https://github.com/voxel51/fiftyone/pull/4182 --json files --jq '.files[].path'
Length of output: 262
Script:
#!/bin/bash
# Fetch the contents of the TypeScript source files and the configuration file for review.
cat app/packages/state/src/recoil/aggregations.test.ts
cat app/packages/state/src/recoil/aggregations.ts
cat app/packages/state/src/recoil/schema.ts
cat app/packages/state/tsconfig.json
Length of output: 27679
Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Walkthrough
The recent updates introduce enhancements and new features across several modules of the state management library. Key changes include improved handling of aggregation paths, the introduction of a selector for managing label paths based on their expansion status, and refined filtering capabilities within the schema. Additionally, a new test suite ensures the reliability of aggregation functionalities. These modifications collectively aim to offer more nuanced control and flexibility in managing state, particularly in scenarios involving modal-specific logic and expanded label paths.
Changes
| Files | Change Summary |
|---|---|
.../recoil/aggregations.ts |
- Added State import, refactored aggregation logic, changed filtering and finding logic, added modalAggregationPaths selector. |
.../recoil/labels.ts |
- Introduced labelPathsSetExpanded selector for accessing label paths with expanded status. |
.../recoil/schema.ts |
- Added labelPathsSetExpanded import, renamed filterFields to filterFieldsCommon, added modalFilterFields. |
.../components/Sidebar/Entries/FilterablePathEntry/Arrow.tsx |
- Added disabled prop for conditional rendering of the arrow component. |
.../components/Sidebar/Entries/FilterablePathEntry/Icon.tsx |
- Updated IconWrapper component with a new disabled prop, adjusted order of useRecoilValue calls. |
🐰✨
In a world of code and state,
Changes come, both small and great.
Paths expand, selectors align,
Through tests, our bugs we confine.
So hop along, let's celebrate,
For improvements, we create!
🌟🐾
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?
Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai generate interesting stats about this repository and render them as a table.@coderabbitai show all the console.log statements in this repository.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
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 as PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger a review. This is useful when automatic reviews are disabled for the repository.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai helpto get help.
Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
CodeRabbit Configration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - The JSON schema for the configuration file is available here.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json
CodeRabbit Discord Community
Join our Discord Community to get help, request features, and share feedback.
hit rate limit last time @coderabbitai review
[!TIP] For best results, initiate chat on the files or code changes.
@swheaton, after checking the latest commit in the pull request, it appears there have been no new changes since the last review. If you've made updates that haven't been pushed yet, please do so. Otherwise, let me know if there's anything specific you'd like me to look into or if there are any concerns from the previous review that need further attention.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the latest commit hash of the PR to check for new changes since the last review.
git log -n 1 --pretty=format:"%H" origin/HEAD
Length of output: 86
Tested locally and mostly things are looking good. Nice work.
Issue I found while testing locally:
- Expanding frames.id seems to crash the app
Thank you! I will debug and address