Admins can choose to disable toasts by default #4966
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/packages/app/src/components/QueryPerformanceToast.tsx (2 hunks)
- app/packages/core/src/components/Actions/Options.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/core/src/components/Actions/Options.tsx
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/app/src/components/QueryPerformanceToast.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.
🔇 Additional comments (2)
app/packages/app/src/components/QueryPerformanceToast.tsx (2)
9-9:⚠️ Potential issueRemove duplicate import of useRecoilValue.
The
useRecoilValueis already included in the combined import statement.Apply this diff to fix the duplicate import:
-import { atom, useRecoilState, useRecoilValue } from "recoil"; +import { atom, useRecoilState } from "recoil";⛔ Skipped due to learnings
Learnt from: minhtuev PR: voxel51/fiftyone#4957 File: app/packages/components/src/components/Toast/Toast.tsx:29-36 Timestamp: 2024-10-22T16:25:08.017Z Learning: When reviewing React components in `app/packages/components/src/components/*`, verify component imports carefully before suggesting missing imports, especially when components are imported via destructuring from packages like `@mui/material`.
41-41: Verify consistent usage of defaultQueryPerformanceConfig.Let's ensure this configuration is consistently applied across other components that might need similar admin control.
✅ Verification successful
Let me try different commands to gather the necessary information.
Let me try one more search with simpler commands to find any related configurations or usages.
The usage of defaultQueryPerformanceConfig is consistent and properly integrated
The verification shows that the query performance configuration is well-integrated across the codebase:
- The config is properly defined in
state/recoil/queryPerformance.tswith bothdefaultQueryPerformanceConfigandenableQueryPerformanceConfigselectors- It's consistently used in the toast component and actions options
- The implementation follows a proper hierarchy where
defaultQueryPerformanceConfigdepends onenableQueryPerformanceConfigThe current implementation in
QueryPerformanceToast.tsxcorrectly follows the established pattern of admin control for query performance features.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of defaultQueryPerformanceConfig echo "Searching for other components using defaultQueryPerformanceConfig..." rg "defaultQueryPerformanceConfig" --type ts --type tsx # Search for similar toast components that might need admin control echo "Searching for other toast components..." rg "Toast" --type ts --type tsx -g "!test/*" -g "!*.test.*"Length of output: 409
Script:
#!/bin/bash # Search for defaultQueryPerformanceConfig usage echo "Searching for defaultQueryPerformanceConfig..." rg -t typescript -t javascriptreact -t typescriptreact "defaultQueryPerformanceConfig" # Search for toast components echo "Searching for toast components..." rg -t typescript -t javascriptreact -t typescriptreact "Toast" -g "!test/*" -g "!*.test.*" # Search for files with .tsx extension that might contain the config echo "Searching for tsx files..." fd -e tsx -x grep -l "defaultQueryPerformanceConfig" {}Length of output: 608
Script:
#!/bin/bash # Look at the content of QueryPerformanceToast.tsx echo "Content of QueryPerformanceToast.tsx:" cat ./app/packages/app/src/components/QueryPerformanceToast.tsx # Search for any files containing "Performance" in their name echo -e "\nSearching for Performance-related files:" fd "Performance" # Basic grep for config-related terms echo -e "\nSearching for related config terms:" grep -r "PerformanceConfig" .Length of output: 4716
| // Admins can choose to disable toasts for each dataset | ||
| if (!shown || disabled || !useRecoilValue(fos.defaultQueryPerformanceConfig)) { | ||
| return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider restructuring conditions for better readability.
While the implementation is correct, the conditions could be reorganized for better clarity and the comment could be more specific.
Consider applying this diff:
- // Admins can choose to disable toasts for each dataset
- if (!shown || disabled || !useRecoilValue(fos.defaultQueryPerformanceConfig)) {
+ // Check if query performance toasts are enabled in dataset config
+ const queryPerformanceEnabled = useRecoilValue(fos.defaultQueryPerformanceConfig);
+ if (!queryPerformanceEnabled || !shown || disabled) {
return null;
}This restructuring:
- Separates the config check for better visibility
- Orders conditions from global (config) to local (component state)
- Makes the comment more specific about the configuration's purpose
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Admins can choose to disable toasts for each dataset | |
| if (!shown || disabled || !useRecoilValue(fos.defaultQueryPerformanceConfig)) { | |
| return null; | |
| // Check if query performance toasts are enabled in dataset config | |
| const queryPerformanceEnabled = useRecoilValue(fos.defaultQueryPerformanceConfig); | |
| if (!queryPerformanceEnabled || !shown || disabled) { | |
| return null; |
What changes are proposed in this pull request?
Admins can choose to configure vars to disable toasts by default
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
Documentation