Allow SelectGroupSlices for QP #5428
Conversation
WalkthroughThe pull request introduces enhancements to the query performance functionality in the Recoil state management module. The changes focus on improving the Changes
Sequence DiagramsequenceDiagram
participant User
participant QueryPerformance
participant QueryPerformanceStore
User->>QueryPerformance: Request performance check
QueryPerformance->>QueryPerformance: Check valid stages
QueryPerformance->>QueryPerformanceStore: Retrieve/Set performance setting
QueryPerformanceStore-->>QueryPerformance: Return performance configuration
QueryPerformance-->>User: Provide performance status
Possibly related PRs
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/state/src/recoil/queryPerformance.ts (1)
22-24: Consider centralizing stage constants for maintainabilityDefining
SELECT_GROUP_SLICESandVALID_QP_STAGESwithin this file may lead to duplication if these constants are used elsewhere. Consider importing them from a shared constants module to improve maintainability and ensure consistency across the codebase.app/packages/state/src/recoil/queryPerformance.test.ts (1)
31-33: Simplify type casting in test setupInstead of casting
queryPerformance.isQueryPerformantViewtounknownbefore assigning toTestSelector, consider direct casting to improve readability:-const test = <TestSelector<typeof queryPerformance.isQueryPerformantView>>( - (<unknown>queryPerformance.isQueryPerformantView) -); +const test = queryPerformance.isQueryPerformantView as TestSelector<typeof queryPerformance.isQueryPerformantView>;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/state/src/recoil/queryPerformance.test.ts(2 hunks)app/packages/state/src/recoil/queryPerformance.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/state/src/recoil/queryPerformance.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/state/src/recoil/queryPerformance.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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build / build
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
🔇 Additional comments (7)
app/packages/state/src/recoil/queryPerformance.ts (5)
194-208: LGTM: Correct implementation ofisQueryPerformantViewselectorThe
isQueryPerformantViewselector accurately assesses whether the current view stages are valid for query performance optimizations based on the definedVALID_QP_STAGES.
223-224: LGTM: Updated logic inqueryPerformanceselectorThe
queryPerformanceselector now appropriately combinesqueryPerformanceSettingandisQueryPerformantViewto determine if query performance should be enabled.
225-231: Ensure correct handling ofDefaultValueinsetmethodConfirm that the
value instanceof DefaultValuecheck correctly resets thequeryPerformanceStoreatom family to its default state when necessary.
Line range hint
233-254: LGTM: Addition ofqueryPerformanceSettingselectorThe
queryPerformanceSettingselector effectively manages the query performance setting by considering the configuration, stored values, and default settings.
255-264: LGTM: Proper use ofqueryPerformanceStoreatom familyThe
queryPerformanceStoreatom family correctly utilizes browser storage effects to persist the query performance settings per dataset.app/packages/state/src/recoil/queryPerformance.test.ts (2)
6-9: LGTM: Proper import of test typesThe import of
TestSelectorandTestSelectorFamilytypes from the mocked recoil module enhances type safety in tests.
30-62: LGTM: Comprehensive test cases forisQueryPerformantViewThe added test cases thoroughly verify the
isQueryPerformantViewselector's logic with various view configurations, ensuring robust functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/packages/state/src/recoil/queryPerformance.ts (2)
194-208: Consider extracting stage validation logic for better maintainability.The selector logic is correct, but could be more maintainable by extracting the stage validation into a separate function.
Consider this refactor:
+const isValidQPStage = (stage: { _cls: string }) => VALID_QP_STAGES.has(stage._cls); + export const isQueryPerformantView = selector({ key: "isQueryPerformantView", get: ({ get }) => { const stages = get(view); if (!stages?.length) { return true; } if (stages.length === 1) { - return VALID_QP_STAGES.has(stages[0]._cls); + return isValidQPStage(stages[0]); } return false; }, });
Line range hint
226-247: Consider adding explicit type annotations for better maintainability.The selector logic is solid, but could benefit from explicit TypeScript types.
Consider this enhancement:
+type QueryPerformanceValue = boolean | undefined; + export const queryPerformanceSetting = selector<boolean>({ key: "queryPerformanceSetting", - get: ({ get }) => { + get: ({ get }): boolean => { if (!get(enableQueryPerformanceConfig)) { return false; } - const storedValue = get(queryPerformanceStore(get(datasetId))); + const storedValue: QueryPerformanceValue = get(queryPerformanceStore(get(datasetId))); if (storedValue !== undefined) { return storedValue; } return get(defaultQueryPerformanceConfig); }, - set: ({ get, set }, value) => { + set: ({ get, set }, value: boolean | DefaultValue): void => { set( queryPerformanceStore(get(datasetId)), value instanceof DefaultValue ? undefined : value ); }, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/packages/state/src/recoil/queryPerformance.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/state/src/recoil/queryPerformance.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 (3)
app/packages/state/src/recoil/queryPerformance.ts (3)
22-23: LGTM! Well-structured constants for QP stage validation.The use of a Set for
VALID_QP_STAGESprovides efficient lookups, and the naming follows TypeScript conventions.
222-224: LGTM! Clean integration of QP stage validation.The selector correctly combines the performance setting with stage validation, following Recoil best practices.
248-257: LGTM! Well-configured storage for query performance state.The atomFamily correctly uses session storage with proper dataset isolation and value type configuration.
What changes are proposed in this pull request?
Allows QP when a
SelectGroupSlicesstage is presentHow is this patch tested? If it is not, please explain why.
Selector test
Release Notes
SelectGroupSlicesviewsWhat areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Tests
New Features
Refactor