Bug fixes for Query Performance Toast #5048
Conversation
WalkthroughThe changes involve modifications to several components related to query performance in a React application. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
|
| ({ get }) => { | ||
| return Boolean(get(boundsAtom(params))?.every((b) => b !== null)); | ||
| const shouldCalculate = params.shouldCalculate ?? true; |
There was a problem hiding this comment.
I added this because I was seeing that whenever a sidebar section was opened we were calling to lightningModeQuery to get the bounds which in QP mode we should never see that until a user actually clicks a button to view a slider or opens a dropdown. This was firing off because if we aren't in QP mode we want to check if something has bounds to determine if we want to show it at all instead of just saying "No Results". This way in QP we don't check bounds until the user selects the field and in non QP mode we always check it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/FilterablePathEntries.tsx (1)
22-23: Document the color prop handlingConsider adding a brief comment explaining why the color prop from the data is being discarded in favor of the color from the pathColor selector.
+ {/* Override individual entry colors with the parent path color */} {data.map(({ color: _, ...props }) => ( <FilterItem key={props.path} color={color} {...events} {...props} /> ))}app/packages/core/src/components/Filters/NumericFieldFilter/state.ts (1)
12-15: Simplify the boolean expressionsThe logic is correct and aligns well with the performance optimization goals. However, the boolean expressions can be simplified.
Consider this cleaner implementation:
- const shouldCalculate = params.shouldCalculate ?? true; - return shouldCalculate - ? Boolean(get(boundsAtom(params))?.every((b) => b !== null)) - : Boolean(false); + const shouldCalculate = params.shouldCalculate ?? true; + if (!shouldCalculate) return false; + return get(boundsAtom(params))?.every((b) => b !== null) ?? false;This version:
- Early returns when calculation is not needed
- Removes redundant Boolean conversions
- Handles the null case explicitly with the nullish coalescing operator
app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx (2)
92-92: Consider using theme-based spacing for button heightInstead of hardcoding the height to "30px", consider using theme-based spacing for better maintainability and consistency across the application.
- height: "30px", + height: theme.spacing(3.75), // or appropriate theme variable
Line range hint
1-103: Consider performance optimizationsThe component could benefit from several optimizations:
- Memoize expensive computations and callbacks using useMemo/useCallback
- Consider using React.memo to prevent unnecessary re-renders
- Move styled components outside the component to prevent recreation
Example optimization:
const handleShowRange = useCallback(() => { setShowRange(true); }, []); const showButton = useMemo(() => isGroup && queryPerformance && !showRange, [isGroup, queryPerformance, showRange] );app/packages/core/src/components/QueryPerformanceToast.tsx (4)
14-23: Consider adding TypeScript interface for the atom state.The atom implementation looks good and correctly uses session storage for persistence. For better type safety, consider defining an interface for the atom state.
interface QueryPerformanceToastState { hideQueryPerformanceToast: boolean; }
25-41: Add proper TypeScript interface for component props.While the implementation is correct, adding proper TypeScript interfaces would improve maintainability and type safety.
interface QueryPerformanceToastProps { onClick?: (isFrameFilter: boolean) => void; onDispatch?: (event: CustomEvent) => void; text?: string; }
Line range hint
42-51: Add type safety and error handling for custom events.The event handling could be improved with proper TypeScript types and error handling for malformed events.
interface QueryPerformanceEvent extends CustomEvent { path: string; } useEffect(() => { const listen = (event: Event) => { try { const customEvent = event as QueryPerformanceEvent; if (!customEvent.path) { console.error('Invalid queryperformance event received'); return; } onDispatch(customEvent); setPath(customEvent.path); setShown(true); } catch (error) { console.error('Error processing queryperformance event:', error); } }; window.addEventListener("queryperformance", listen); return () => window.removeEventListener("queryperformance", listen); }, []);
69-80: Extract frame field check logic for better readability.The frame field checking logic in the onClick handler could be extracted into a separate function for better maintainability.
const isFrameFilterPath = (path: string, frameFields: Array<{path: string}>) => frameFields.some((frame) => path.includes(`frames.${frame.path}`)); // Then in onClick: onClick(isFrameFilterPath(path, frameFields));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx(2 hunks)app/packages/core/src/components/Filters/NumericFieldFilter/state.ts(1 hunks)app/packages/core/src/components/QueryPerformanceToast.tsx(4 hunks)app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/FilterablePathEntries.tsx(2 hunks)app/packages/core/src/utils/links.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.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/Filters/NumericFieldFilter/state.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/core/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.
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/FilterablePathEntries.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.
🔇 Additional comments (7)
app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/FilterablePathEntries.tsx (2)
Line range hint 1-26: LGTM! Component successfully simplified
The refactoring has effectively simplified the component by removing the query performance tracking logic, making it more focused and maintainable while preserving its core filtering functionality.
Line range hint 1-26: Verify removal of query performance tracking
The QueryPerformance component and related functionality have been removed. Let's verify that this aligns with the new implementation mentioned in the PR objectives.
✅ Verification successful
Query performance tracking has been moved to a more efficient implementation
The codebase shows that query performance tracking functionality is still present and well-integrated, but has evolved into a more sophisticated system:
- Query performance is now controlled through configuration (
enableQueryPerformance,defaultQueryPerformance) - It's integrated with lightning mode queries for better performance
- The UI provides clear indicators (lightning bolt icons) and user controls for query performance
- A dedicated toast component (
QueryPerformanceToast) provides user feedback - The functionality is properly tied to database indexes through
pathHasIndexes
The changes align with the PR objectives by providing a more efficient and user-friendly implementation rather than removing the functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that query performance tracking has been moved elsewhere
# and that we're not breaking any existing functionality
# Check for any remaining query performance related code
echo "Checking for remaining query performance references..."
rg -i "queryperformance" --type ts --type tsx
# Look for the new implementation mentioned in PR objectives
echo "Checking for new implementation of performance tracking..."
rg -i "lightningModeQuery|paginateSamplesQuery" --type ts --type tsx -A 5
Length of output: 423
Script:
#!/bin/bash
# Let's try without file type restrictions since tsx isn't recognized
echo "Checking for query performance references..."
rg -i "queryperformance"
echo "Checking for performance tracking implementations..."
rg -i "lightningModeQuery|paginateSamplesQuery"
# Also check for any performance monitoring related imports or components
echo "Checking for performance related components..."
rg -i "performance|monitor" -g "*.{ts,tsx,js,jsx}" -A 3
Length of output: 103412
app/packages/core/src/utils/links.ts (1)
19-21: LGTM! The new constant follows the established patterns.
The addition of QP_MODE_SUMMARY is well-placed and aligns with the PR objective of directing users to the summary page for additional information.
Let's verify the URL is accessible and points to the correct documentation section:
✅ Verification successful
URL verification successful - link is accessible
The documentation URL for summary fields (https://docs.voxel51.com/user_guide/using_datasets.html#summary-fields) returns a 200 status code, confirming it's accessible and valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the documentation URL exists and contains relevant content
# Expected: URL should return 200 status and contain "summary fields" content
curl -s -I "https://docs.voxel51.com/user_guide/using_datasets.html#summary-fields" | grep "HTTP/"
Length of output: 108
app/packages/core/src/components/Filters/NumericFieldFilter/state.ts (1)
10-10: LGTM! Type definition is well-defined
The addition of the shouldCalculate boolean parameter maintains type safety and clearly indicates its purpose.
app/packages/core/src/components/Filters/NumericFieldFilter/NumericFieldFilter.tsx (1)
51-53: Verify the logic of shouldCalculate parameter
The negation of queryPerformance (!queryPerformance) as the shouldCalculate parameter might lead to unexpected behavior. Please verify that bounds should indeed not be calculated when query performance monitoring is active.
✅ Verification successful
Based on the search results, I can now provide a definitive response about the shouldCalculate: !queryPerformance logic:
The negation logic is correct and intentional
The code's behavior is correct because:
- When
queryPerformanceis true, bounds calculation is skipped (shouldCalculate: false) as the system uses lightning queries and indexed fields - When
queryPerformanceis false, bounds are calculated (shouldCalculate: true) to show the full range of values - This is consistent with other components' behavior, where performance mode (
queryPerformance: true) skips expensive calculations and relies on indexes
This pattern is consistently used across the codebase, particularly in:
NumericFieldFilter.tsxfor conditional rendering based on bounds and performance modestate.tsin the numeric filters implementation- Other filter components that adapt their behavior based on the query performance mode
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of hasBounds to verify the logic
ast-grep --pattern 'state.hasBounds({ $$ })'
Length of output: 46
Script:
#!/bin/bash
# Let's try a broader search for hasBounds usage
rg "hasBounds" -A 3 -B 3
# Also search for the state import to understand the context
rg "import.*state" --type typescript
# Look for queryPerformance usage
rg "queryPerformance" -A 3 -B 3
Length of output: 49116
app/packages/core/src/components/QueryPerformanceToast.tsx (2)
1-12: LGTM! Import organization and constant updates look good.
The increased toast duration of 10 seconds provides users with more time to read and interact with the notification.
Line range hint 93-104: LGTM! Dismiss button implementation is solid.
The dismiss button correctly persists the user's preference in session storage and updates the local state.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
app/packages/core/src/components/QueryPerformanceToast.tsx (5)
1-13: Consider adding type definitions for imported componentsWhile the constant change to 10 seconds aligns with requirements, consider adding type definitions for imported components to improve type safety.
Add type imports for components:
+import type { ToastProps } from "@fiftyone/components";
26-43: Add TypeScript interface for component propsThe component would benefit from explicit prop types and documentation.
Consider adding:
+interface QueryPerformanceToastProps { + /** Callback when documentation link is clicked */ + onClick?: (isFrameFilter: boolean) => void; + /** Callback when toast event is dispatched */ + onDispatch?: (event: CustomEvent) => void; + /** Text for the primary action button */ + text?: string; +} -const QueryPerformanceToast = ({ +const QueryPerformanceToast: React.FC<QueryPerformanceToastProps> = ({
Line range hint
44-65: Consider adding error boundaries and type safety for eventsWhile the event handling works, it could benefit from better error handling and type safety.
Consider these improvements:
+interface QueryPerformanceEvent extends CustomEvent { + path: string; +} useEffect(() => { - const listen = (event) => { + const listen = (event: QueryPerformanceEvent) => { onDispatch(event); + if (!event.path) { + console.warn('Query performance event missing path'); + return; + } setPath(event.path); setShown(true); }; - window.addEventListener("queryperformance", listen); + window.addEventListener("queryperformance", listen as EventListener); - return () => window.removeEventListener("queryperformance", listen); + return () => window.removeEventListener("queryperformance", listen as EventListener); }, []);
76-87: Optimize button handler and improve accessibilityThe onClick handler could be memoized, and the button needs accessibility improvements.
Consider these changes:
+const handlePrimaryClick = useCallback(() => { + onClick(frameFields.some((frame) => path.includes(`frames.${frame.path}`))); + setShown(false); +}, [frameFields, path, onClick]); <Button variant="contained" size="small" - onClick={() => { - onClick(frameFields.some((frame) => path.includes(`frames.${frame.path}`))); - setShown(false); - }} + onClick={handlePrimaryClick} + aria-label="View query performance documentation"
Line range hint
100-116: Add aria-label to dismiss buttonThe dismiss button needs an accessibility label.
<Button data-cy="btn-dismiss-query-performance-toast" variant="text" color="secondary" size="small" + aria-label="Dismiss query performance notification" onClick={() => {app/packages/utilities/src/fetch.ts (2)
25-31: LGTM! Consider adding JSDoc documentation.The
QueryPerformanceToastclass is well-implemented. Consider adding JSDoc documentation to describe the purpose of this event class and its path parameter.+/** + * Custom event for query performance notifications. + * @param path Optional field path that triggered the performance event + */ class QueryPerformanceToast extends Event { path?: string; constructor(path?: string) {
33-33: Enhance constant declaration with type and descriptive name.Consider making the constant more descriptive and adding a type annotation.
-const TIMEOUT = 5000; +/** Timeout in milliseconds before triggering query performance notification */ +const QUERY_PERFORMANCE_TIMEOUT_MS: number = 5000;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/core/src/components/QueryPerformanceToast.tsx(5 hunks)app/packages/utilities/src/fetch.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/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.
app/packages/utilities/src/fetch.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.
🪛 Biome
app/packages/utilities/src/fetch.ts
[error] 435-435: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
app/packages/core/src/components/QueryPerformanceToast.tsx (1)
15-24: LGTM! Well-implemented state persistence
The Recoil atom implementation with session storage effect is clean and follows best practices.
| let timeoutId = undefined; | ||
| const filterPath = getFirstFilterPath(body); | ||
| if (filterPath) { | ||
| timeoutId = setTimeout(() => { | ||
| window.dispatchEvent(new QueryPerformanceToast(filterPath)); | ||
| }, TIMEOUT); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety and improve race condition handling.
The timeout implementation could benefit from:
- Stronger typing for the timeout ID
- Protection against potential race conditions if multiple requests are made simultaneously
- let timeoutId = undefined;
+ let timeoutId: number | undefined = undefined;
const filterPath = getFirstFilterPath(body);
if (filterPath) {
+ // Ensure any existing timeout is cleared before setting a new one
+ if (timeoutId) clearTimeout(timeoutId);
timeoutId = setTimeout(() => {
+ // Verify the request hasn't completed before showing the toast
+ if (timeoutId) {
window.dispatchEvent(new QueryPerformanceToast(filterPath));
+ }
}, TIMEOUT);
}Also applies to: 153-155
| const getFirstFilterPath = (requestBody: any) => { | ||
| if (requestBody && requestBody.query) { | ||
| if (requestBody.query.includes("paginateSamplesQuery")) { | ||
| const pathsArray = requestBody?.variables?.filters; | ||
| const paths = pathsArray ? Object.keys(pathsArray) : []; | ||
| return paths.length > 0 ? paths[0] : undefined; | ||
| } | ||
| if (requestBody.query.includes("lightningQuery")) { | ||
| const paths = requestBody?.variables?.input?.paths; | ||
| return paths && paths.length > 0 ? paths[0].path : undefined; | ||
| } | ||
| } | ||
| return undefined; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling.
The function could benefit from:
- TypeScript interface for the request body
- Optional chaining for safer property access
- More defensive error handling
+interface QueryRequestBody {
+ query?: string;
+ variables?: {
+ filters?: Record<string, unknown>;
+ input?: {
+ paths?: Array<{ path: string }>;
+ };
+ };
+}
-const getFirstFilterPath = (requestBody: any) => {
+const getFirstFilterPath = (requestBody: QueryRequestBody): string | undefined => {
try {
- if (requestBody && requestBody.query) {
- if (requestBody.query.includes("paginateSamplesQuery")) {
- const pathsArray = requestBody?.variables?.filters;
+ if (requestBody?.query) {
+ if (requestBody.query.includes("paginateSamplesQuery")) {
+ const pathsArray = requestBody.variables?.filters;
const paths = pathsArray ? Object.keys(pathsArray) : [];
return paths.length > 0 ? paths[0] : undefined;
}
- if (requestBody.query.includes("lightningQuery")) {
- const paths = requestBody?.variables?.input?.paths;
+ if (requestBody.query.includes("lightningQuery")) {
+ const paths = requestBody.variables?.input?.paths;
return paths && paths.length > 0 ? paths[0].path : undefined;
}
}
return undefined;
+ } catch (error) {
+ console.warn('Error extracting filter path:', error);
+ return undefined;
+ }
};📝 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.
| const getFirstFilterPath = (requestBody: any) => { | |
| if (requestBody && requestBody.query) { | |
| if (requestBody.query.includes("paginateSamplesQuery")) { | |
| const pathsArray = requestBody?.variables?.filters; | |
| const paths = pathsArray ? Object.keys(pathsArray) : []; | |
| return paths.length > 0 ? paths[0] : undefined; | |
| } | |
| if (requestBody.query.includes("lightningQuery")) { | |
| const paths = requestBody?.variables?.input?.paths; | |
| return paths && paths.length > 0 ? paths[0].path : undefined; | |
| } | |
| } | |
| return undefined; | |
| }; | |
| interface QueryRequestBody { | |
| query?: string; | |
| variables?: { | |
| filters?: Record<string, unknown>; | |
| input?: { | |
| paths?: Array<{ path: string }>; | |
| }; | |
| }; | |
| } | |
| const getFirstFilterPath = (requestBody: QueryRequestBody): string | undefined => { | |
| try { | |
| if (requestBody?.query) { | |
| if (requestBody.query.includes("paginateSamplesQuery")) { | |
| const pathsArray = requestBody.variables?.filters; | |
| const paths = pathsArray ? Object.keys(pathsArray) : []; | |
| return paths.length > 0 ? paths[0] : undefined; | |
| } | |
| if (requestBody.query.includes("lightningQuery")) { | |
| const paths = requestBody.variables?.input?.paths; | |
| return paths && paths.length > 0 ? paths[0].path : undefined; | |
| } | |
| } | |
| return undefined; | |
| } catch (error) { | |
| console.warn('Error extracting filter path:', error); | |
| return undefined; | |
| } | |
| }; |
🧰 Tools
🪛 Biome
[error] 435-435: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
What changes are proposed in this pull request?
Done:
How is this patch tested? If it is not, please explain why.
Changed TIMEOUT to 0 so the toast is always triggered and then added a console log statement to QPToast to show that the path is set accordingly.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Summary by CodeRabbit
New Features
NumericFieldFiltercomponent to conditionally calculate bounds based on query performance.QueryPerformanceToastfrom 5 to 10 seconds.Bug Fixes
QueryPerformanceToastfor better user experience.Refactor
QueryPerformancecomponents and related event handling from theFilterablePathEntriescomponent.