Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge release/v1.1.0 to develop #5011

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions app/packages/app/src/pages/datasets/DatasetPage.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Dataset, Snackbar, Starter } from "@fiftyone/core";
import { Dataset, Snackbar, Starter, QueryPerformanceToast } from "@fiftyone/core";
import "@fiftyone/embeddings";
import "@fiftyone/map";
import { OperatorCore } from "@fiftyone/operators";
Expand All @@ -10,7 +10,6 @@ import { usePreloadedQuery } from "react-relay";
import { useRecoilValue } from "recoil";
import { graphql } from "relay-runtime";
import Nav from "../../components/Nav";
import QueryPerformanceToast from "../../components/QueryPerformanceToast";
import type { Route } from "../../routing";
import style from "../index.module.css";
import type { DatasetPageQuery } from "./__generated__/DatasetPageQuery.graphql";
Expand Down
128 changes: 128 additions & 0 deletions app/packages/core/src/components/QueryPerformanceToast.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
import { Toast } from "@fiftyone/components";
import { QP_MODE } from "@fiftyone/core";
import { getBrowserStorageEffectForKey } from "@fiftyone/state";
import { Box, Button, Typography } from "@mui/material";
import { Bolt } from "@mui/icons-material";
import React, { useEffect, useState } from "react";
import { createPortal } from "react-dom";
import { atom, useRecoilState } from "recoil";
import { useTheme } from "@fiftyone/components";

const SHOWN_FOR = 5000;

const hideQueryPerformanceToast = atom({
key: "hideQueryPerformanceToast",
default: false,
effects: [
getBrowserStorageEffectForKey("hideQueryPerformanceToast", {
valueClass: "boolean",
}),
],
});
Comment on lines +13 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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",
}),
],
});


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);
}, []);
Comment on lines +23 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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);
}, []);


if (!element) {
throw new Error("no query performance element");
}

if (!shown || disabled) {
return null;
}
Comment on lines +36 to +42
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;
}


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
);
};
Comment on lines +44 to +126
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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
);
};


export default QueryPerformanceToast;
1 change: 1 addition & 0 deletions app/packages/core/src/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ 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";
Loading