Merge release/v1.1.0 to develop
#5011
Conversation
WalkthroughThe changes in this pull request involve the addition of a new component, Changes
Possibly related PRs
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (3)
app/packages/core/src/components/index.ts (1)
12-12: Consider maintaining alphabetical order of exports.The new export follows the correct naming and path conventions, but for better maintainability, consider moving it before
ResourceCountto maintain alphabetical ordering with other exports.export { default as Loading } from "./Loading"; +export { default as QueryPerformanceToast } from "./QueryPerformanceToast"; export { default as ResourceCount } from "./ResourceCount"; export * from "./Sidebar"; export { default as Snackbar } from "./Snackbar"; export { default as ViewBar, rollbackViewBar } from "./ViewBar/ViewBar"; export * from "./Starter"; -export { default as QueryPerformanceToast } from "./QueryPerformanceToast";app/packages/app/src/pages/datasets/DatasetPage.tsx (1)
Line range hint
1-15: Consider organizing imports by category.While the current import organization is functional, consider grouping imports into categories for better maintainability:
- External dependencies (React, Relay)
- Internal packages (@fiftyone/*)
- Local components and types
- Styles
+ // External dependencies import React from "react"; import { usePreloadedQuery } from "react-relay"; import { useRecoilValue } from "recoil"; import { graphql } from "relay-runtime"; + // Internal packages import { Dataset, Snackbar, Starter, QueryPerformanceToast } from "@fiftyone/core"; import "@fiftyone/embeddings"; import "@fiftyone/map"; import { OperatorCore } from "@fiftyone/operators"; import "@fiftyone/relay"; import * as fos from "@fiftyone/state"; import { datasetQueryContext } from "@fiftyone/state"; + // Local imports import Nav from "../../components/Nav"; import type { Route } from "../../routing"; import type { DatasetPageQuery } from "./__generated__/DatasetPageQuery.graphql"; + // Styles import style from "../index.module.css";app/packages/core/src/components/QueryPerformanceToast.tsx (1)
1-11: Add TypeScript types and improve constant naming.Consider these improvements for better type safety and clarity:
- Add explicit type imports from React (e.g.,
FC,MouseEvent)- Rename
SHOWN_FORto something more descriptive likeTOAST_DISPLAY_DURATION_MS- Add type annotation to the constant
-import React, { useEffect, useState } from "react"; +import React, { FC, useEffect, useState } from "react"; -const SHOWN_FOR = 5000; +const TOAST_DISPLAY_DURATION_MS: number = 5000;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/packages/app/src/pages/datasets/DatasetPage.tsx(1 hunks)app/packages/core/src/components/QueryPerformanceToast.tsx(1 hunks)app/packages/core/src/components/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/app/src/pages/datasets/DatasetPage.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/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/index.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/app/src/pages/datasets/DatasetPage.tsx (2)
1-1: LGTM! Import statement follows package organization.
The QueryPerformanceToast import is correctly grouped with other core components from @fiftyone/core package.
Line range hint 92-94: Verify toast notification hierarchy and z-index.
The QueryPerformanceToast is placed alongside Snackbar, which is correct for global notifications. However, ensure there are no z-index conflicts or overlay issues between these components.
Consider implementing a toast/notification manager to handle multiple notifications if not already present.
app/packages/core/src/components/QueryPerformanceToast.tsx (1)
23-126: 🛠️ Refactor suggestion
Consider performance optimizations.
The component could benefit from these performance improvements:
- Memoize callback functions using useCallback
- Memoize complex JSX using useMemo
- Consider using React.memo for the component itself if it's used in a list or frequently re-rendered context
+const QueryPerformanceToast: FC = React.memo(() => {
// ... existing implementation
-});
+}, (prevProps, nextProps) => true);| const hideQueryPerformanceToast = atom({ | ||
| key: "hideQueryPerformanceToast", | ||
| default: false, | ||
| effects: [ | ||
| getBrowserStorageEffectForKey("hideQueryPerformanceToast", { | ||
| valueClass: "boolean", | ||
| }), | ||
| ], | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and naming for Recoil atom.
Consider these improvements:
- Add an interface for the atom state
- Namespace the atom key to avoid potential conflicts
- Consider using a constant for the storage key
+interface QueryPerformanceToastState {
+ hidden: boolean;
+}
+const STORAGE_KEY = "fiftyone.queryPerformance.hideToast";
const hideQueryPerformanceToast = atom({
- key: "hideQueryPerformanceToast",
+ key: "fiftyone.atoms.hideQueryPerformanceToast",
default: false,
+ effects: [
+ getBrowserStorageEffectForKey(STORAGE_KEY, {
valueClass: "boolean",
}),
],
});📝 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 hideQueryPerformanceToast = atom({ | |
| key: "hideQueryPerformanceToast", | |
| default: false, | |
| effects: [ | |
| getBrowserStorageEffectForKey("hideQueryPerformanceToast", { | |
| valueClass: "boolean", | |
| }), | |
| ], | |
| }); | |
| interface QueryPerformanceToastState { | |
| hidden: boolean; | |
| } | |
| const STORAGE_KEY = "fiftyone.queryPerformance.hideToast"; | |
| const hideQueryPerformanceToast = atom({ | |
| key: "fiftyone.atoms.hideQueryPerformanceToast", | |
| default: false, | |
| effects: [ | |
| getBrowserStorageEffectForKey(STORAGE_KEY, { | |
| valueClass: "boolean", | |
| }), | |
| ], | |
| }); |
| if (!element) { | ||
| throw new Error("no query performance element"); | ||
| } | ||
|
|
||
| if (!shown || disabled) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Improve error handling approach.
Instead of throwing an error, consider:
- Logging the error and gracefully handling the missing element
- Providing a more descriptive error message
- Adding error boundaries for better error handling
- if (!element) {
- throw new Error("no query performance element");
- }
+ if (!element) {
+ console.error("QueryPerformanceToast: Target DOM element #queryPerformance not found");
+ return null;
+ }📝 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.
| if (!element) { | |
| throw new Error("no query performance element"); | |
| } | |
| if (!shown || disabled) { | |
| return null; | |
| } | |
| if (!element) { | |
| console.error("QueryPerformanceToast: Target DOM element #queryPerformance not found"); | |
| return null; | |
| } | |
| if (!shown || disabled) { | |
| return null; | |
| } |
| const QueryPerformanceToast = () => { | ||
| const [shown, setShown] = useState(false); | ||
| const [disabled, setDisabled] = useRecoilState(hideQueryPerformanceToast); | ||
| const element = document.getElementById("queryPerformance"); | ||
| const theme = useTheme(); | ||
| useEffect(() => { | ||
| const listen = () => { | ||
| setShown(true); | ||
| }; | ||
| window.addEventListener("queryperformance", listen); | ||
| return () => window.removeEventListener("queryperformance", listen); | ||
| }, []); |
There was a problem hiding this comment.
Improve component type safety and event handling.
Several TypeScript and React best practices could be applied here:
- Add FC type to the component
- Define custom event type for 'queryperformance'
- Move DOM element access into useEffect to avoid potential null checks during SSR
+interface QueryPerformanceEvent extends Event {
+ // add any custom event properties here
+}
-const QueryPerformanceToast = () => {
+const QueryPerformanceToast: FC = () => {
const [shown, setShown] = useState(false);
const [disabled, setDisabled] = useRecoilState(hideQueryPerformanceToast);
- const element = document.getElementById("queryPerformance");
const theme = useTheme();
+
useEffect(() => {
+ const element = document.getElementById("queryPerformance");
+ if (!element) {
+ console.error("Query performance element not found");
+ return;
+ }
+
- const listen = () => {
+ const listen = (event: QueryPerformanceEvent) => {
setShown(true);
};
- window.addEventListener("queryperformance", listen);
- return () => window.removeEventListener("queryperformance", listen);
- }, []);
+ window.addEventListener("queryperformance", listen as EventListener);
+ return () => window.removeEventListener("queryperformance", listen as EventListener);
+ }, []);📝 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 QueryPerformanceToast = () => { | |
| const [shown, setShown] = useState(false); | |
| const [disabled, setDisabled] = useRecoilState(hideQueryPerformanceToast); | |
| const element = document.getElementById("queryPerformance"); | |
| const theme = useTheme(); | |
| useEffect(() => { | |
| const listen = () => { | |
| setShown(true); | |
| }; | |
| window.addEventListener("queryperformance", listen); | |
| return () => window.removeEventListener("queryperformance", listen); | |
| }, []); | |
| interface QueryPerformanceEvent extends Event { | |
| // add any custom event properties here | |
| } | |
| const QueryPerformanceToast: FC = () => { | |
| const [shown, setShown] = useState(false); | |
| const [disabled, setDisabled] = useRecoilState(hideQueryPerformanceToast); | |
| const theme = useTheme(); | |
| useEffect(() => { | |
| const element = document.getElementById("queryPerformance"); | |
| if (!element) { | |
| console.error("Query performance element not found"); | |
| return; | |
| } | |
| const listen = (event: QueryPerformanceEvent) => { | |
| setShown(true); | |
| }; | |
| window.addEventListener("queryperformance", listen as EventListener); | |
| return () => window.removeEventListener("queryperformance", listen as EventListener); | |
| }, []); |
| return createPortal( | ||
| <Toast | ||
| duration={SHOWN_FOR} | ||
| layout={{ | ||
| bottom: "100px", | ||
| vertical: "bottom", | ||
| horizontal: "center", | ||
| backgroundColor: theme.custom.toastBackgroundColor, | ||
| }} | ||
| primary={(setOpen) => { | ||
| return ( | ||
| <Button | ||
| variant="contained" | ||
| size="small" | ||
| onClick={() => { | ||
| open(QP_MODE, "_blank")?.focus(); | ||
| setOpen(false); | ||
| }} | ||
| sx={{ | ||
| marginLeft: "auto", | ||
| backgroundColor: theme.primary.main, | ||
| color: theme.text.primary, | ||
| boxShadow: 0, | ||
| }} // Right align the button | ||
| > | ||
| View Documentation | ||
| </Button> | ||
| ); | ||
| }} | ||
| secondary={(setOpen) => { | ||
| return ( | ||
| <div> | ||
| <Button | ||
| variant="text" | ||
| color="secondary" | ||
| size="small" | ||
| onClick={() => { | ||
| setDisabled(true); | ||
| setOpen(false); | ||
| }} | ||
| style={{ marginLeft: "auto", color: theme.text.secondary }} // Right align the button | ||
| > | ||
| Dismiss | ||
| </Button> | ||
| </div> | ||
| ); | ||
| }} | ||
| message={ | ||
| <> | ||
| <Box sx={{ display: "flex", alignItems: "center" }}> | ||
| <Bolt sx={{ color: theme.custom.lightning, marginRight: "8px" }} /> | ||
| <Typography | ||
| variant="subtitle1" | ||
| sx={{ | ||
| fontWeight: 500, | ||
| marginRight: "8px", | ||
| color: theme.text.primary, | ||
| }} | ||
| > | ||
| Query Performance is Available! | ||
| </Typography> | ||
| <Typography | ||
| variant="caption" | ||
| sx={{ | ||
| color: theme.custom.lightning, | ||
| borderRadius: "2px", | ||
| padding: "2px 4px", | ||
| fontSize: "1rem", | ||
| }} | ||
| > | ||
| NEW | ||
| </Typography> | ||
| </Box> | ||
| <Typography variant="body2" sx={{ color: theme.text.secondary }}> | ||
| Index the most critical fields for faster data loading and query | ||
| performance. | ||
| </Typography> | ||
| </> | ||
| } | ||
| />, | ||
| element | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and style organization.
Consider these improvements:
- Add aria labels for better accessibility
- Move inline styles to sx prop
- Extract magic numbers to constants
- Add proper button types
<Button
variant="contained"
size="small"
+ type="button"
+ aria-label="View query performance documentation"
onClick={() => {
open(QP_MODE, "_blank")?.focus();
setOpen(false);
}}
sx={{
marginLeft: "auto",
backgroundColor: theme.primary.main,
color: theme.text.primary,
boxShadow: 0,
- }} // Right align the button
+ }}
>
<Button
variant="text"
color="secondary"
size="small"
+ type="button"
+ aria-label="Dismiss notification"
onClick={() => {
setDisabled(true);
setOpen(false);
}}
- style={{ marginLeft: "auto", color: theme.text.secondary }}
+ sx={{
+ marginLeft: "auto",
+ color: theme.text.secondary
+ }}
>📝 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.
| return createPortal( | |
| <Toast | |
| duration={SHOWN_FOR} | |
| layout={{ | |
| bottom: "100px", | |
| vertical: "bottom", | |
| horizontal: "center", | |
| backgroundColor: theme.custom.toastBackgroundColor, | |
| }} | |
| primary={(setOpen) => { | |
| return ( | |
| <Button | |
| variant="contained" | |
| size="small" | |
| onClick={() => { | |
| open(QP_MODE, "_blank")?.focus(); | |
| setOpen(false); | |
| }} | |
| sx={{ | |
| marginLeft: "auto", | |
| backgroundColor: theme.primary.main, | |
| color: theme.text.primary, | |
| boxShadow: 0, | |
| }} // Right align the button | |
| > | |
| View Documentation | |
| </Button> | |
| ); | |
| }} | |
| secondary={(setOpen) => { | |
| return ( | |
| <div> | |
| <Button | |
| variant="text" | |
| color="secondary" | |
| size="small" | |
| onClick={() => { | |
| setDisabled(true); | |
| setOpen(false); | |
| }} | |
| style={{ marginLeft: "auto", color: theme.text.secondary }} // Right align the button | |
| > | |
| Dismiss | |
| </Button> | |
| </div> | |
| ); | |
| }} | |
| message={ | |
| <> | |
| <Box sx={{ display: "flex", alignItems: "center" }}> | |
| <Bolt sx={{ color: theme.custom.lightning, marginRight: "8px" }} /> | |
| <Typography | |
| variant="subtitle1" | |
| sx={{ | |
| fontWeight: 500, | |
| marginRight: "8px", | |
| color: theme.text.primary, | |
| }} | |
| > | |
| Query Performance is Available! | |
| </Typography> | |
| <Typography | |
| variant="caption" | |
| sx={{ | |
| color: theme.custom.lightning, | |
| borderRadius: "2px", | |
| padding: "2px 4px", | |
| fontSize: "1rem", | |
| }} | |
| > | |
| NEW | |
| </Typography> | |
| </Box> | |
| <Typography variant="body2" sx={{ color: theme.text.secondary }}> | |
| Index the most critical fields for faster data loading and query | |
| performance. | |
| </Typography> | |
| </> | |
| } | |
| />, | |
| element | |
| ); | |
| }; | |
| return createPortal( | |
| <Toast | |
| duration={SHOWN_FOR} | |
| layout={{ | |
| bottom: "100px", | |
| vertical: "bottom", | |
| horizontal: "center", | |
| backgroundColor: theme.custom.toastBackgroundColor, | |
| }} | |
| primary={(setOpen) => { | |
| return ( | |
| <Button | |
| variant="contained" | |
| size="small" | |
| type="button" | |
| aria-label="View query performance documentation" | |
| onClick={() => { | |
| open(QP_MODE, "_blank")?.focus(); | |
| setOpen(false); | |
| }} | |
| sx={{ | |
| marginLeft: "auto", | |
| backgroundColor: theme.primary.main, | |
| color: theme.text.primary, | |
| boxShadow: 0, | |
| }} | |
| > | |
| View Documentation | |
| </Button> | |
| ); | |
| }} | |
| secondary={(setOpen) => { | |
| return ( | |
| <div> | |
| <Button | |
| variant="text" | |
| color="secondary" | |
| size="small" | |
| type="button" | |
| aria-label="Dismiss notification" | |
| onClick={() => { | |
| setDisabled(true); | |
| setOpen(false); | |
| }} | |
| sx={{ | |
| marginLeft: "auto", | |
| color: theme.text.secondary | |
| }} | |
| > | |
| Dismiss | |
| </Button> | |
| </div> | |
| ); | |
| }} | |
| message={ | |
| <> | |
| <Box sx={{ display: "flex", alignItems: "center" }}> | |
| <Bolt sx={{ color: theme.custom.lightning, marginRight: "8px" }} /> | |
| <Typography | |
| variant="subtitle1" | |
| sx={{ | |
| fontWeight: 500, | |
| marginRight: "8px", | |
| color: theme.text.primary, | |
| }} | |
| > | |
| Query Performance is Available! | |
| </Typography> | |
| <Typography | |
| variant="caption" | |
| sx={{ | |
| color: theme.custom.lightning, | |
| borderRadius: "2px", | |
| padding: "2px 4px", | |
| fontSize: "1rem", | |
| }} | |
| > | |
| NEW | |
| </Typography> | |
| </Box> | |
| <Typography variant="body2" sx={{ color: theme.text.secondary }}> | |
| Index the most critical fields for faster data loading and query | |
| performance. | |
| </Typography> | |
| </> | |
| } | |
| />, | |
| element | |
| ); | |
| }; |
Merge
release/v1.1.0todevelopSummary by CodeRabbit
New Features
QueryPerformanceToastcomponent to provide users with notifications about query performance.Enhancements
QueryPerformanceToastinto theDatasetPagefor improved user experience.