Lightning mode refactor and disable/enable qp mode toggle #4928
Conversation
WalkthroughThe pull request introduces significant changes across multiple components, primarily renaming and restructuring elements associated with performance modes. The Changes
Possibly related PRs
Suggested reviewers
Poem
📜 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
|
minhtuev
changed the title
![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 (7)
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Tune.tsx (2)
57-57: LGTM: Constant usage updated correctlyThe
QP_MODEconstant is correctly used in thehrefprop of theDisabledReasoncomponent, maintaining consistency with the import change. This update aligns with the PR objective and doesn't introduce any issues.Consider updating the text prop from "add an index" to "enable query performance mode" for better clarity and consistency with the new terminology. This would make the tooltip message more explicit about the action being suggested.
- text={<DisabledReason href={QP_MODE} text={"add an index"} />} + text={<DisabledReason href={QP_MODE} text={"enable query performance mode"} />}
Line range hint
1-67: Overall impact: Minimal and consistent with PR objectiveThe changes in this file successfully implement the renaming from "lightning mode" to "query performance mode" without affecting the component's structure or functionality. The updates are minimal, localized, and don't introduce any breaking changes.
To improve code organization and make it easier to update in the future, consider extracting the hardcoded string "add an index" into a constant at the top of the file. This would centralize all text content and make it easier to maintain:
const ADD_INDEX_TEXT = "add an index"; // or with the suggested update: // const ENABLE_QP_MODE_TEXT = "enable query performance mode"; // Then use it in the component: text={<DisabledReason href={QP_MODE} text={ADD_INDEX_TEXT} />}This change would make the component more maintainable and easier to update in the future.
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Arrow.tsx (1)
63-63: LGTM: Component logic updated correctlyThe component logic has been updated to use
QP_MODEinstead ofLIGHTNING_MODEin the Tooltip's text prop. This change is consistent with the refactoring from "lightning mode" to "query performance mode" and doesn't affect the overall functionality of the component.For consistency, consider updating the text in the DisabledReason component from "add an index" to something more specific to "query performance mode", such as "enable query performance mode" or "add an index for query performance". This would make the tooltip message more aligned with the new terminology.
app/packages/state/src/recoil/lightning.ts (1)
203-211: Good integration of new selectors, minor suggestion for organizationThe new
enableQueryPerformanceConfiganddefaultQueryPerformanceConfigselectors are well-integrated into the existing codebase. They follow the same pattern as thelightningThresholdConfigselector and are placed logically within the file structure.The naming convention aligns with the "query performance" terminology mentioned in the PR objectives, which is good for consistency.
Consider grouping all configuration-related selectors together for better code organization. You could move the
lightningThresholdConfigselector (defined at line 199) next to the new selectors for improved readability and maintainability.app/packages/core/src/components/Actions/Options.tsx (1)
Line range hint
219-285: LGTM: QueryPerformance component updated correctly.The renaming from "Lightning" to "QueryPerformance" has been consistently applied throughout the component. The new
enableQpModecondition, updated attributes, and refactored active state logic improve the component's functionality and readability.Consider extracting the
enableQpModecondition to a custom hook or memoized value for better reusability and performance:const enableQpMode = useMemo(() => useRecoilValue(fos.enableQueryPerformanceConfig) && useRecoilValue(fos.defaultQueryPerformanceConfig), [fos.enableQueryPerformanceConfig, fos.defaultQueryPerformanceConfig] );This change would prevent unnecessary re-renders and improve code organization.
app/packages/core/src/components/FieldLabelAndInfo/index.tsx (2)
23-23: Approved: Import statement updated correctlyThe import statement has been successfully updated from
LIGHTNING_MODEtoQP_MODE, which aligns with the PR objective of renaming "lightning mode" to "query performance mode".Consider updating the variable name
lightnigPathtoqueryPerformancePathorqpPathin theLightningcomponent for consistency with the new naming convention.
348-348: Approved: External link updated, but consider further renamingThe
hrefattribute has been correctly updated to useQP_MODE, which is consistent with the import statement change and the PR objective.For better consistency with the new "query performance mode" terminology, consider the following refactoring suggestions:
- Rename the
Lightningcomponent toQueryPerformanceorQPMode.- Update the following variable names:
lightningtoqueryPerformanceorqpModelightnigPathtoqueryPerformancePathorqpPath- Update the link text from "Lightning indexed" to "Query Performance indexed" or "QP indexed".
These changes would ensure that the component and its internal variables align with the new naming convention throughout the codebase.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- app/packages/core/src/components/Actions/Options.tsx (4 hunks)
- app/packages/core/src/components/FieldLabelAndInfo/index.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Arrow.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Tune.tsx (2 hunks)
- app/packages/core/src/utils/links.ts (1 hunks)
- app/packages/state/src/recoil/lightning.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
app/packages/core/src/components/Actions/Options.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/FieldLabelAndInfo/index.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/Arrow.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/Tune.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/utils/links.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/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 (10)
app/packages/core/src/utils/links.ts (1)
16-17: Approve renaming, but suggest URL update and usage verification.The renaming from
LIGHTNING_MODEtoQP_MODEaligns with the PR objectives. However, consider the following points:
- The URL still contains "lightning-mode" in its path. It might be beneficial to update the documentation URL to reflect the new "query performance mode" terminology.
- Ensure that all references to this constant throughout the codebase have been updated to use
QP_MODE.To verify the usage of
QP_MODEacross the codebase, run the following script:This script will help identify any missed replacements and confirm the correct usage of the new constant name.
✅ Verification successful
Approve renaming and recommend URL update.
The renaming from
LIGHTNING_MODEtoQP_MODEhas been successfully applied across the codebase. However, the URL associated withQP_MODEstill references "lightning-mode". Please update the documentation URL to reflect the new "query performance mode" terminology.
- Update the URL in
app/packages/core/src/utils/links.tsto use "query-performance-mode" instead of "lightning-mode".🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to LIGHTNING_MODE and verify QP_MODE usage # Search for any remaining LIGHTNING_MODE references echo "Searching for LIGHTNING_MODE references:" rg --type-not md "LIGHTNING_MODE" # Search for QP_MODE usage echo "Searching for QP_MODE usage:" rg --type-not md "QP_MODE"Length of output: 1182
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Tune.tsx (1)
6-6: LGTM: Import statement updated correctlyThe import statement has been updated to use
QP_MODEinstead ofLIGHTNING_MODE, which aligns with the PR objective of renaming "lightning mode" to "query performance mode". This change is consistent and doesn't introduce any issues.app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Arrow.tsx (2)
10-10: LGTM: Import statement updated correctlyThe import statement has been updated to use
QP_MODEinstead ofLIGHTNING_MODE, which is consistent with the reported refactoring from "lightning mode" to "query performance mode". This change maintains the overall structure of the imports while reflecting the new terminology.
Line range hint
1-93: Summary: Successful refactoring of lightning mode to query performance modeThe changes in this file successfully implement the renaming of "lightning mode" to "query performance mode". The modifications are minimal and focused, affecting only the import statement and one instance in the component logic. These changes maintain the overall structure and functionality of the
Arrowcomponent while updating the terminology.The component continues to adhere to React and TypeScript best practices, and the changes are consistent with the PR objectives. No new issues have been introduced, and the refactoring has been implemented cleanly.
app/packages/state/src/recoil/lightning.ts (3)
203-206: LGTM:enableQueryPerformanceConfigselector implementationThe implementation of the
enableQueryPerformanceConfigselector follows Recoil best practices. It uses a unique key and correctly retrieves theenableQueryPerformanceproperty from theconfigobject. The naming is clear and consistent with the existing code style.
208-211: LGTM:defaultQueryPerformanceConfigselector implementationThe implementation of the
defaultQueryPerformanceConfigselector adheres to Recoil best practices. It uses a unique key and correctly retrieves thedefaultQueryPerformanceproperty from theconfigobject. The naming is clear and consistent with the existing code style.
203-211: Summary: Changes align well with PR objectivesThe additions to this file align well with the PR objectives of refactoring the "lightning mode" to "query performance mode". The new selectors (
enableQueryPerformanceConfiganddefaultQueryPerformanceConfig) provide a clear interface for accessing query performance-related configuration.These changes contribute to the overall goal of improving clarity in the codebase and unifying the handling of performance modes. The implementation is consistent with existing patterns and follows best practices for Recoil and TypeScript.
app/packages/core/src/components/Actions/Options.tsx (3)
20-20: LGTM: Import statement updated correctly.The addition of
QP_MODEimport is consistent with the renaming from "Lightning mode" to "Query Performance mode".
376-376: LGTM: Conditional rendering of QueryPerformance component updated correctly.The change ensures that the QueryPerformance component is only rendered when there's no active view, which is consistent with the component's purpose and improves the UI logic.
Line range hint
1-385: LGTM: Successful renaming of "Lightning mode" to "Query Performance mode".The changes in this file consistently rename "Lightning mode" to "Query Performance mode" throughout the components and their usage. The modifications align well with the PR objectives and maintain the overall structure and functionality of the file. The renaming has been applied correctly to imports, component names, attributes, and rendering logic.
Great job on maintaining consistency and improving the clarity of the codebase!
|
Lightning mode e2e tests are failing. It might be an actual regression or the specs might have to be updated. |
| const [threshold, setThreshold] = useRecoilState(fos.lightningThreshold); | ||
| const config = useRecoilValue(fos.lightningThresholdConfig); | ||
| const reset = useResetRecoilState(fos.lightningThreshold); | ||
| const count = useRecoilValue(fos.datasetSampleCount); | ||
| const theme = useTheme(); | ||
| const enableQpMode = useRecoilValue(fos.enableQueryPerformanceConfig) && useRecoilValue(fos.defaultQueryPerformanceConfig); |
There was a problem hiding this comment.
In React, hooks cannot be run conditionally. This should be changed to two separate lines
What changes are proposed in this pull request?
Refactored lightning mode to rename as query performance mode and allowed enabling/disabling qp mode toggle through deployment vars
How is this patch tested? If it is not, please explain why.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Improvements
Documentation