pawarprasad123 commented on code in PR #688:
URL: https://github.com/apache/atlas/pull/688#discussion_r3900822803
##########
dashboard/src/redux/slice/sessionSlice.ts:
##########
@@ -77,6 +93,27 @@ const sessionSlice = createSlice({
data: null,
error: (action.payload as string) || action.error?.message || 'An
error occurred'
};
+ }),
+ builder.addCase(fetchVersionData.pending, (state) => {
+ state.versionData.loading = true;
+ state.versionData.error = null;
+ }),
Review Comment:
Version text can flash empty during refetch despite the spinner UX.
Suggested comment: sessionSlice.ts lines 100–105 — Preserve existing
state.versionData.data on pending (stale-while-revalidate), and add a test for
it.
##########
dashboard/src/components/EntityDisplayImage.tsx:
##########
@@ -15,89 +15,60 @@
* limitations under the License.
*/
-import { useEffect, useState } from "react";
-import { Avatar, Skeleton } from "@mui/material";
+import type { SyntheticEvent } from "react";
+import { Avatar } from "@mui/material";
import { getEntityIconPath } from "../utils/Utils";
-import axios from "axios";
+
+interface DisplayImageProps {
+ entity: Record<string, unknown>;
+ width?: string | number;
+ height?: string | number;
+ avatarDisplay?: boolean;
+ isProcess?: boolean;
+}
const DisplayImage = ({
entity,
width,
height,
avatarDisplay,
isProcess
-}: any) => {
- const [imageUrl, setImageUrl] = useState<any>(null);
- const [checkEntityImage, setCheckEntityImage] = useState<any>({
- [entity.guid]: false
- });
-
- useEffect(() => {
- const fetchImagePath = async () => {
- let entityData = { ...entity, ...{ isProcess: isProcess } };
- let imagePath: any = getEntityIconPath({ entityData: entityData });
- try {
- const response = await axios.get(imagePath, {
- responseType: "blob"
- });
- const contentType: any = response.headers["content-type"];
-
- if (contentType && contentType.startsWith("image/")) {
- let cache = { [entityData.guid]: imagePath };
- setCheckEntityImage(cache);
- setImageUrl(getEntityIconPath({ entityData: entityData }));
- } else {
- setImageUrl(
- getEntityIconPath({ entityData: entityData, errorUrl: imagePath })
- );
- }
- } catch (_error) {
- setImageUrl(
- getEntityIconPath({ entityData: entityData, errorUrl: imagePath })
- );
- }
- };
+}: DisplayImageProps) => {
+ const entityData = { ...entity, isProcess };
+
+ const primaryUrl = getEntityIconPath({ entityData }) || "";
+ const fallbackUrl = getEntityIconPath({ entityData, errorUrl: primaryUrl })
|| "";
- fetchImagePath();
- }, []);
+ const handleError = (e: SyntheticEvent<HTMLImageElement, Event>) => {
+ const target = e.currentTarget;
+ if (target.dataset.fallbackApplied !== "true") {
+ target.dataset.fallbackApplied = "true";
+ target.onerror = null;
+ target.src = fallbackUrl;
+ }
+ };
- return imageUrl != undefined ? (
+ return (
<div className="search-result-table-name-col" data-cy="entityIcon">
- {checkEntityImage[entity.guid] !== false ? (
- avatarDisplay == undefined ? (
- <img
- className="search-result-table-img"
- id={entity.guid}
- data-cy={entity.guid}
- src={checkEntityImage[entity.guid]}
- alt="Entity Icon"
- />
- ) : (
- <Avatar
- alt="entityImg"
- src={checkEntityImage[entity.guid]}
- sx={{ width: width, height: height }}
- variant="square"
- ></Avatar>
- )
- ) : avatarDisplay == undefined ? (
+ {avatarDisplay === undefined ? (
<img
className="search-result-table-img"
- id={entity.guid}
- data-cy={entity.guid}
- src={imageUrl}
+ id={entity.guid ? String(entity.guid) : undefined}
Review Comment:
When guid is undefined, this produces "undefined" string attributes.
Test gap: No test for missing guid in EntityDisplayImage.test.tsx.
— Guard with entity.guid ? String(entity.guid) : undefined and add a
negative test.
##########
dashboard/src/views/SideBar/SideBarBody.tsx:
##########
@@ -308,198 +389,124 @@ const SideBarBody = (props: {
data-cy="atlas-logo"
/>
</span>
- <Paper
- sx={{
- width: "100%",
- }}
- className="sidebar-searchbar"
- >
- <InputBase
- fullWidth
- sx={{ color: "rgba(0, 0, 0, 0.7)" }}
- placeholder="Entities, Classifications, Glossaries"
- inputProps={{ "aria-label": "search" }}
- value={searchTerm}
- onChange={(e: ChangeEvent<HTMLInputElement>) => {
- setSearchTerm(e.target.value);
- }}
- data-cy="searchNode"
- />
-
- <IconButton type="submit" size="small" aria-label="search">
- <SearchIcon fontSize="inherit" />
- </IconButton>
- </Paper>
+ <SidebarSearchInput
+ searchTerm={searchTerm}
+ onChange={setSearchTerm}
+ dataCy="searchNode"
+ />
</Stack>
</DrawerHeader>
)}
<Paper
- className="sidebar-wrapper"
- sx={{
- flex: 1,
- overflow: "hidden auto",
- paddingBottom: "0px", // Account for bottom toggle button
- ...(open == false && {
- overflow: "hidden",
- }),
- }}
+ className={`sidebar-wrapper ${!open ? "sidebar-wrapper--hidden" :
""}`}
>
- <div
- className="sidebar-treeview-container"
- data-cy="r_entityTreeRender"
- >
- <Suspense
- fallback={
- <SkeletonLoader
- animation="pulse"
- variant="text"
- width={330}
- count={5}
- />
- // <Stack className="tree-item-loader-box">
- // </Stack>
- }
- >
- <EntitiesTree
- sideBarOpen={open}
- loading={loading}
- searchTerm={searchTerm}
- />
- </Suspense>
- </div>
+ <div
+ className="sidebar-treeview-container"
+ data-cy="r_entityTreeRender"
+ >
+ <Suspense
+ fallback={<TreeSkeletonLoader count={2} />}
+ >
+ <EntitiesTree
+ sideBarOpen={open}
+ searchTerm={searchTerm}
+ />
+ </Suspense>
+ </div>
- <div
- className="sidebar-treeview-container"
- data-cy="r_classificationTreeRender"
- >
- <Suspense
- fallback={
- <SkeletonLoader
- animation="pulse"
- variant="text"
- width={330}
- count={5}
- />
- // <Stack className="tree-item-loader-box">
- // </Stack>
- }
- >
- <ClassificationTree
- sideBarOpen={open}
- loading={loader}
- searchTerm={searchTerm}
- />
- </Suspense>
- </div>
+ <div
+ className="sidebar-treeview-container"
+ data-cy="r_classificationTreeRender"
+ >
+ <Suspense
+ fallback={<TreeSkeletonLoader count={2} />}
+ >
+ <ClassificationTree
+ sideBarOpen={open}
+ searchTerm={searchTerm}
+ />
+ </Suspense>
+ </div>
- <div
- className="sidebar-treeview-container"
- data-cy="r_businessMetadataTreeRender"
- >
- <Suspense
- fallback={
- <SkeletonLoader
- animation="pulse"
- variant="text"
- width={330}
- count={5}
- />
- // <Stack className="tree-item-loader-box">
- // </Stack>
- }
- >
- <BusinessMetadataTree
- sideBarOpen={open}
- searchTerm={searchTerm}
- />
- </Suspense>
- </div>
+ <div
+ className="sidebar-treeview-container"
+ data-cy="r_glossaryTreeRender"
+ >
+ <Suspense
+ fallback={<TreeSkeletonLoader count={2} />}
+ >
+ <GlossaryTree sideBarOpen={open} searchTerm={searchTerm} />
+ </Suspense>
+ </div>
- <div
- className="sidebar-treeview-container"
- data-cy="r_glossaryTreeRender"
- >
- <Suspense
- fallback={
- <SkeletonLoader
- animation="pulse"
- variant="text"
- width={330}
- count={5}
- />
- // <Stack className="tree-item-loader-box">
- // </Stack>
- }
- >
- <GlossaryTree sideBarOpen={open} searchTerm={searchTerm} />
- </Suspense>
- </div>
- {relationshipSearch && (
- <div
- className="sidebar-treeview-container"
- data-cy="r_relationshipTreeRender"
- >
- <Suspense
- fallback={
- <SkeletonLoader
- animation="pulse"
- variant="text"
- width={330}
- count={5}
+ <div
+ className="sidebar-treeview-container"
+ data-cy="r_businessMetadataTreeRender"
+ >
+ <Suspense
+ fallback={<TreeSkeletonLoader count={2} />}
+ >
+ <BusinessMetadataTree
+ sideBarOpen={open}
+ searchTerm={searchTerm}
/>
- // <Stack className="tree-item-loader-box">
- // </Stack>
- }
+ </Suspense>
+ </div>
+ {relationshipSearch && (
+ <div
+ className="sidebar-treeview-container"
+ data-cy="r_relationshipTreeRender"
+ >
+ <Suspense
+ fallback={<TreeSkeletonLoader count={2} />}
+ >
+ <RelationshipsTree
+ sideBarOpen={open}
+ searchTerm={searchTerm}
+ />
+ </Suspense>
+ </div>
+ )}
+
+ <div
+ className="sidebar-treeview-container"
+ data-cy="r_customFilterTreeRender"
>
- <RelationshipsTree
- sideBarOpen={open}
- searchTerm={searchTerm}
- />
- </Suspense>
+ <Suspense
+ fallback={<TreeSkeletonLoader count={2} />}
+ >
+ <CustomFiltersTree sideBarOpen={open}
searchTerm={searchTerm} />
+ </Suspense>
+ </div>
+ </Paper>
+ <div
+ className={`sidebar-toggle-container ${open ?
"sidebar-toggle-open" : "sidebar-toggle-closed"}`}
+ >
+ {open && (
+ <div className="sidebar-version-container">
+ <Typography variant="body2" className="sidebar-version-text">
+ {isVersionLoading ? (
+ <CircularProgress size={12}
className="sidebar-version-loader" />
+ ) : versionError ? (
Review Comment:
line 490-578
There is no .sidebar-wrapper--hidden class anywhere in the codebase. Popover
trees are separate instances mounted only when a popover opens.
Impact: Toggling expanded ↔ collapsed remounts trees and can re-trigger data
loading/state loss — the opposite of what the PR description claims.
Either keep trees mounted with CSS visibility, or update the PR description
and add a test that documents the current (remount) behavior.
##########
dashboard/src/views/SideBar/__tests__/SideBarBody.test.tsx:
##########
@@ -454,6 +465,12 @@ describe('SideBarBody', () => {
expect(metricsSlice.fetchMetricEntity).toHaveBeenCalled();
});
+ it('should dispatch fetchVersionData on mount', () => {
+ renderWithProviders();
+
+ expect(sessionSlice.fetchVersionData).toHaveBeenCalled();
+ });
+
it('should pass loading state to tree components', () => {
Review Comment:
SideBarBody no longer accepts or passes a loading prop (removed in
Layout.tsx). This test only checks tree presence, not loading behavior.
— Remove or rewrite to assert actual behavior.
##########
dashboard/src/components/__tests__/EntityDisplayImage.test.tsx:
##########
@@ -15,269 +15,169 @@
* limitations under the License.
*/
Review Comment:
missing test case general:
- Trees do not unmount on drawer toggle- Not tested; behavior contradicts
claim
- Version data retained during pending - Not implemented or tested
- Missing guid → no "undefined" DOM attrs - Not implemented or tested
- TreeSkeletonLoader negative/undefined bounds - Only 3 basic tests; no
edge-case tests
- Escape key closes popover - Fallback only; primary path is backdrop click
- Popover height snapping near viewport bottom - No test
- relationshipSearch=false hides module + popover -Partial (icon hidden); no
popover negative test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
