Optimize modal sample tagging (#5417) for 1.3.1 #5513
Conversation
* tag counts with respect to modal sample/group * group issues * simplify, some cleanup * revert slice view change * fix e2e
WalkthroughThe changes reorganize the data fetching and payload construction within the tagging functionality. In the core components, the logic now distinguishes between a new "mode" value obtained via a group statistics function and "currentSlices" fetched separately. Similar adjustments have been made in utility functions to better structure the data passed to downstream operations. Additionally, updates in the state management modules adjust slice handling, aggregation paths, and type safety. The backend logic for mixed view handling in aggregations was also simplified. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Tagger
participant FosModule
participant TagAPI
User->>Tagger: Initiate tagging action
Tagger->>FosModule: Call groupStatistics(modal) → obtain mode
Tagger->>FosModule: Call currentSlices(modal) → obtain currentSlices
Tagger->>FosModule: Call groupSlices(modal) → obtain slices
Tagger->>TagAPI: Submit payload { mode, currentSlices, slices }
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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: 0
🧹 Nitpick comments (2)
app/packages/core/src/components/Actions/utils.tsx (2)
87-91: LGTM! Consider adding type assertions for better type safety.The restructuring of the group data object improves clarity by separating current and group slices. The changes align well with the PR's objective of optimizing modal sample tagging.
Consider adding type assertions to ensure type safety:
- currentSlices: get(fos.currentSlices(modal)), - mode: get(groupStatistics(modal)), - slice: get(fos.currentSlice(modal)), - slices: get(fos.groupSlices), + currentSlices: get(fos.currentSlices(modal)) as string[] | null, + mode: get(groupStatistics(modal)) as "group" | "slice", + slice: get(fos.currentSlice(modal)) as string | null, + slices: get(fos.groupSlices) as string[] | null,
159-159: LGTM! Consider using early return pattern for better readability.The changes improve type safety and simplify the logic for handling non-group mode. The condition update for
shouldShowCurrentSampleis more precise.Consider using early return pattern for better readability:
- if (groupData && !groups) { - groupData.slices = groupData.currentSlices; - } + if (!groupData || groups) return; + groupData.slices = groupData.currentSlices;Also applies to: 168-168, 170-172
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/packages/core/src/components/Actions/Tagger.tsx(2 hunks)app/packages/core/src/components/Actions/utils.tsx(5 hunks)app/packages/state/src/recoil/aggregations.ts(3 hunks)app/packages/state/src/recoil/modal.ts(2 hunks)app/packages/state/src/recoil/pathData/tags.ts(3 hunks)fiftyone/server/aggregations.py(0 hunks)
💤 Files with no reviewable changes (1)
- fiftyone/server/aggregations.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{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/Actions/Tagger.tsxapp/packages/state/src/recoil/modal.tsapp/packages/core/src/components/Actions/utils.tsxapp/packages/state/src/recoil/pathData/tags.tsapp/packages/state/src/recoil/aggregations.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (11)
app/packages/state/src/recoil/pathData/tags.ts (3)
3-3: LGTM: Added import for groupStatistics.The new import is correctly added and will be used to determine the mixed parameter value.
14-23: LGTM: Improved aggregation logic.The aggregation parameters are now correctly determined using
groupStatistics(modal), making the mixed parameter dynamic based on group mode.
56-62: LGTM: Consistent use of groupStatistics.The same pattern is correctly applied to the sample tag counts selector.
app/packages/core/src/components/Actions/Tagger.tsx (2)
351-355: LGTM: Improved data fetching organization.The code now correctly separates:
- Mode from group statistics
- Current slices
- Group slices
This restructuring improves code clarity and maintainability.
370-374: LGTM: Enhanced group data structure.The group data object now properly includes:
- Mode from group statistics
- Current slices for immediate context
- Group slices for broader context
app/packages/state/src/recoil/modal.ts (2)
32-32: LGTM: Enhanced type safety with explicit return type.The selector now correctly specifies its return type as
null | string.
44-44: LGTM: Improved type safety for sample ID.The code now correctly type casts
sample?.sample?._idto string, addressing the type concern noted in the comment above.app/packages/state/src/recoil/aggregations.ts (3)
8-14: LGTM: Well-organized imports.The imports are now properly grouped and include all necessary group-related functions.
85-85: LGTM: Improved slice handling logic.The code now correctly uses
groupSlicesfor mixed state andcurrentSlicesotherwise.
159-163: LGTM: Enhanced path generation logic.The code now:
- Uses spread operator for better array handling
- Correctly prepends 'tags' for non-numeric, non-frames paths
- Maintains better separation of concerns
Also applies to: 170-173
app/packages/core/src/components/Actions/utils.tsx (1)
135-135: LGTM! Simplified return statement improves code clarity.The direct return of tags from
tagStatisticsremoves unnecessary complexity while maintaining the same functionality.
Cherry pick #5417 for 1.3.1
Release Notes
modal tagging <app-tagging>What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Refactor
Chores