pawarprasad123 commented on code in PR #688:
URL: https://github.com/apache/atlas/pull/688#discussion_r3795605031
##########
dashboard/src/views/SideBar/SideBarTree/__tests__/SideBarTree.test.tsx:
##########
@@ -2181,4 +2188,76 @@ describe('SideBarTree', () => {
expect(mockSetIsEmptyServicetype).not.toHaveBeenCalled()
})
})
+
+ describe('Reviewer Requested Tests', () => {
+ it('should render skeleton loader with 2 rows when isPopover is
true', async () => {
+ renderComponent({ loader: true, isPopover: true })
+
+ await waitFor(() => {
+ const loader =
screen.getByTestId('tree-skeleton-loader')
+ expect(loader).toBeInTheDocument()
+ expect(loader).toHaveAttribute('data-count',
'2')
+ })
+ })
+
+ it('should persist selected state from URL params for custom
filters when reopened in popover', async () => {
Review Comment:
The test claims URL-based persistence but uses the wrong query param and
passes because all nodes auto-expand:
##########
dashboard/src/views/SideBar/SideBarBody.tsx:
##########
@@ -246,38 +360,117 @@ const SideBarBody = (props: {
backgroundColor: "#034858",
}}
>
- {/* Collapsed sidebar logo */}
+ {/* Collapsed sidebar logo and module icons */}
{!open && (
- <div
- style={{
- width: "100%",
- textAlign: "center",
- paddingLeft: "12px",
- display: "flex",
- alignItems: "center",
- justifyContent: "center",
- minHeight: "64px",
- cursor: "pointer",
- boxSizing: "border-box",
- }}
- role="button"
- tabIndex={0}
- aria-label="Atlas home — refresh dashboard"
- onClick={handleAtlasLogoClick}
- onKeyDown={handleAtlasLogoKeyDown}
- data-cy="apache-atlas-logo-collapsed"
+ <Stack
+ alignItems="center"
+ sx={{ width: "100%", flex: 1, minHeight: 0, overflowY: "auto",
overflowX: "hidden", boxSizing: "border-box", pb: "60px" }}
>
- <img
- src={apacheAtlasLogo}
- alt="Apache Atlas logo"
- style={{
- width: "29px",
- height: "auto",
- maxWidth: "100%",
- display: "block",
+ <div
+ className="collapsed-logo-container"
+ role="button"
+ tabIndex={0}
+ aria-label="Atlas home — refresh dashboard"
Review Comment:
Module IconButtons lack aria-expanded={activePopover === m.id} and
aria-haspopup="dialog". Add for screen readers.
##########
dashboard/src/views/SideBar/SideBarTree/__tests__/SideBarTree.test.tsx:
##########
@@ -2181,4 +2188,76 @@ describe('SideBarTree', () => {
expect(mockSetIsEmptyServicetype).not.toHaveBeenCalled()
})
})
+
+ describe('Reviewer Requested Tests', () => {
+ it('should render skeleton loader with 2 rows when isPopover is
true', async () => {
+ renderComponent({ loader: true, isPopover: true })
+
+ await waitFor(() => {
+ const loader =
screen.getByTestId('tree-skeleton-loader')
+ expect(loader).toBeInTheDocument()
+ expect(loader).toHaveAttribute('data-count',
'2')
+ })
+ })
+
+ it('should persist selected state from URL params for custom
filters when reopened in popover', async () => {
+ const treeData = [
+ { id: 'customFilter1', label: 'Custom Filter
1', children: [] }
+ ]
+ renderComponent({
+ treeData,
+ treeName: 'CustomFilters',
+ isPopover: true
+ }, {},
['/search/searchResult?searchType=BASIC&isCF=true&type=customFilter1'])
Review Comment:
Use customFilter=customFilter1 (not type=). Assert selection highlight
(selectedNodeCustomFilter), not just expansion — all nodes expand via
expandedItemsMemo.
SidebarTree.tsx line 421
const nodeIdFromCustomFilter = searchParams.get("customFilter");
##########
dashboard/src/views/SideBar/SideBarBody.tsx:
##########
@@ -210,30 +322,32 @@ const SideBarBody = (props: {
<Drawer
sx={{
- width: position,
+ width: open ? defaultDrawerWidth : "60px",
flexShrink: 0,
minHeight: "calc(100vh - 64px)",
- minWidth: "30px",
- ...(open == false && {
- transform: `translateX(calc(-${position} + 30px)) !important`,
+ minWidth: "60px",
+ transition: "width 0.2s",
+ ...(!open && {
+ transform: "none !important",
+ visibility: "visible !important",
}),
- ...(open == false && { visibility: "visible !important" }),
-
"& .MuiDrawer-paper": {
background: "#034858",
boxSizing: "border-box",
overflow: "hidden",
position: "fixed",
top: "0",
- transition: "none !important",
- ...(open == false && {
- transform: `translateX(30px) !important`,
+ left: "0",
+ width: open ? defaultDrawerWidth : "60px",
+ transition: "width 0.2s",
+ ...(!open && {
+ transform: "none !important",
+ visibility: "visible !important",
}),
- ...(open == false && { visibility: "visible !important" }),
},
}}
PaperProps={{
- style: { width: position, minWidth: "30px" },
+ style: { width: open ? defaultDrawerWidth : "60px", minWidth: "60px"
},
}}
variant="persistent"
anchor="left"
Review Comment:
Search expand button: add aria-label="Expand sidebar search".
##########
dashboard/src/views/Layout/About.tsx:
##########
@@ -24,31 +24,9 @@ import {
Stack,
Typography
} from "@mui/material";
-import { serverError } from "@utils/Utils";
-import { useEffect, useRef, useState } from "react";
const About = () => {
- const [versionData, setVersionData] = useState<any>({});
- const [loader, setLoader] = useState(false);
- const toastId = useRef(null);
-
- useEffect(() => {
- fetchVersionDetails();
- }, []);
-
- const fetchVersionDetails = async () => {
- setLoader(true);
- try {
- const versionResp = await getVersion();
- const { data = {} } = versionResp || {};
- setVersionData(data);
- setLoader(false);
- } catch (error) {
- setLoader(false);
- console.error(`Error occur while fetching version details`, error);
- serverError(error, toastId);
- }
- };
+ const { data: versionData, loading: loader } = useAppSelector((state: any)
=> state.session.versionData);
Review Comment:
Consider showing fallback text or error message when
state.session.versionData.error is set.
##########
dashboard/src/components/__tests__/EntityDisplayImage.test.tsx:
##########
@@ -15,271 +15,119 @@
* limitations under the License.
*/
-
-/**
- * Unit tests for EntityDisplayImage component
- *
- * Coverage Target: 100%
- * - Statements: 100%
- * - Branches: 100%
- * - Functions: 100%
- * - Lines: 100%
- */
-
import React from 'react'
-import { render, waitFor, act } from '@testing-library/react'
+import { render, fireEvent } from '@testing-library/react'
import DisplayImage from '../EntityDisplayImage'
-
-// Import Utils to spy on it
import * as Utils from '../../utils/Utils'
const mockGetEntityIconPath = jest.fn()
-// Mock the Utils module
jest.mock('../../utils/Utils', () => ({
- getEntityIconPath: jest.fn()
+ getEntityIconPath: jest.fn()
}))
-const mockFetch = (contentType: string | null, shouldReject?: boolean) => {
- if (shouldReject) {
- (global as any).fetch = jest.fn().mockRejectedValue(new
Error('fetch failed'))
- return
- }
- (global as any).fetch = jest.fn().mockResolvedValue({
- ok: true,
- headers: {
- get: jest.fn((header: string) => {
- return header === 'Content-Type' ? (contentType
|| '') : null
- })
- }
- })
-}
-
describe('EntityDisplayImage', () => {
- const entity = { guid: 'entity-1' }
-
- beforeEach(() => {
- jest.clearAllMocks()
-
- // Set up the mock implementation for getEntityIconPath
- ;(Utils.getEntityIconPath as jest.Mock).mockImplementation(({
entityData, errorUrl }: { entityData: any, errorUrl?: string }) => {
- const result = errorUrl ? `${errorUrl}-fallback` :
`/icons/${entityData.guid}.png`
- mockGetEntityIconPath({ entityData, errorUrl })
- return result
- })
- })
-
- it('renders cached image when content-type is image', async () => {
- mockFetch('image/png')
-
- const { container } = render(
- <DisplayImage entity={entity} width={20} height={20} />
- )
-
- // Wait for Skeleton to disappear and image to appear
- await waitFor(() => {
- const skeleton =
container.querySelector('.MuiSkeleton-root')
- expect(skeleton).not.toBeInTheDocument()
- }, { timeout: 10000, interval: 100 })
-
- await waitFor(() => {
- const img = container.querySelector('img')
- expect(img).toBeInTheDocument()
-
expect(img?.getAttribute('src')).toBe('/icons/entity-1.png')
- expect(img?.getAttribute('alt')).toBe('Entity Icon')
- expect(img?.getAttribute('id')).toBe('entity-1')
- expect(img?.getAttribute('data-cy')).toBe('entity-1')
- }, { timeout: 10000 })
- }, 20000)
-
- it('renders fallback image when content-type is not image', async () =>
{
- mockFetch('text/plain')
-
- const { container } = render(
- <DisplayImage entity={entity} width={20} height={20} />
- )
-
- await waitFor(() => {
- const img = container.querySelector('img')
- expect(img).toBeInTheDocument()
-
expect(img?.getAttribute('src')).toBe('/icons/entity-1.png-fallback')
- }, { timeout: 10000 })
- }, 20000)
-
- it('renders fallback image when content-type is null', async () => {
- mockFetch(null)
-
- const { container} = render(
- <DisplayImage entity={entity} width={20} height={20} />
- )
-
- await waitFor(() => {
- const img = container.querySelector('img')
- expect(img).toBeInTheDocument()
-
expect(img?.getAttribute('src')).toBe('/icons/entity-1.png-fallback')
- }, { timeout: 10000 })
- }, 20000)
-
- it('renders fallback image when fetch throws', async () => {
- mockFetch('image/png', true)
-
- const { container } = render(
- <DisplayImage entity={entity} width={20} height={20} />
- )
-
- await waitFor(() => {
- const img = container.querySelector('img')
- expect(img).toBeInTheDocument()
-
expect(img?.getAttribute('src')).toBe('/icons/entity-1.png-fallback')
- }, { timeout: 10000 })
- }, 20000)
-
- it('renders Avatar when avatarDisplay is provided and image is cached',
async () => {
- mockFetch('image/png')
-
- const { container } = render(
- <DisplayImage
- entity={entity}
- width={30}
- height={30}
- avatarDisplay={true}
- />
- )
-
- await waitFor(() => {
- const avatar =
container.querySelector('img[alt="entityImg"]')
- expect(avatar).toBeTruthy()
-
expect(avatar?.getAttribute('src')).toBe('/icons/entity-1.png')
- }, { timeout: 10000 })
- }, 20000)
-
- it('renders Avatar when avatarDisplay is provided and image is not
cached', async () => {
- mockFetch('text/plain')
-
- const { container } = render(
- <DisplayImage
- entity={entity}
- width={30}
- height={30}
- avatarDisplay={true}
- />
- )
-
- await waitFor(() => {
- const avatar =
container.querySelector('img[alt="entityImg"]')
- expect(avatar).toBeTruthy()
-
expect(avatar?.getAttribute('src')).toBe('/icons/entity-1.png-fallback')
- }, { timeout: 10000 })
- }, 20000)
-
- it('renders Skeleton when imageUrl is undefined', () => {
- mockGetEntityIconPath.mockReturnValue(undefined)
-
- const { container } = render(
- <DisplayImage entity={entity} width={20} height={20} />
- )
-
- const skeleton = container.querySelector('div')
- expect(skeleton).toBeTruthy()
- })
-
- it('handles isProcess prop', async () => {
- mockFetch('image/png')
-
- const entityWithProcess = { guid: 'entity-2', isProcess: true }
- render(
- <DisplayImage entity={entityWithProcess} width={20}
height={20} isProcess={true} />
- )
-
- await waitFor(() => {
- expect(mockGetEntityIconPath).toHaveBeenCalledWith(
- expect.objectContaining({
- entityData: expect.objectContaining({
isProcess: true })
- })
- )
- })
- }, 20000)
-
- it('handles entity without isProcess prop but with isProcess passed',
async () => {
- mockFetch('image/png')
-
- render(
- <DisplayImage entity={entity} width={20} height={20}
isProcess={false} />
- )
-
- await waitFor(() => {
- expect(mockGetEntityIconPath).toHaveBeenCalledWith(
- expect.objectContaining({
- entityData: expect.objectContaining({
isProcess: false })
- })
- )
- })
- }, 20000)
-
- it('sets checkEntityImage cache when image is valid', async () => {
- mockFetch('image/jpeg')
-
- const { container } = render(
- <DisplayImage entity={entity} width={20} height={20} />
- )
-
- await waitFor(() => {
- const img = container.querySelector('img')
- expect(img).toBeInTheDocument()
-
expect(img?.getAttribute('src')).toBe('/icons/entity-1.png')
- }, { timeout: 10000 })
- }, 20000)
-
- it('handles different image content types', async () => {
- const contentTypes = ['image/gif', 'image/webp',
'image/svg+xml']
-
- for (const contentType of contentTypes) {
- mockFetch(contentType)
- const { container, unmount } = render(
- <DisplayImage entity={{ guid:
`entity-${contentType}` }} width={20} height={20} />
- )
-
- await waitFor(() => {
- const img = container.querySelector('img')
- expect(img).toBeTruthy()
- }, { timeout: 10000 })
- unmount()
- }
- }, 30000)
-
- it('handles errorUrl in getEntityIconPath when fetch fails', async ()
=> {
- mockFetch('image/png', true)
- ;(Utils.getEntityIconPath as jest.Mock).mockImplementation(({
entityData, errorUrl }: { entityData: any, errorUrl?: string }) => {
- if (errorUrl) return `${errorUrl}-error`
- return `/icons/${entityData.guid}.png`
- })
-
- const { container } = render(
- <DisplayImage entity={entity} width={20} height={20} />
- )
-
- await waitFor(() => {
- const img = container.querySelector('img')
- expect(img).toBeInTheDocument()
- expect(img?.getAttribute('src')).toContain('-error')
- }, { timeout: 10000 })
- }, 20000)
-
- it('handles errorUrl in getEntityIconPath when content-type is not
image', async () => {
- mockFetch('application/json')
- ;(Utils.getEntityIconPath as jest.Mock).mockImplementation(({
entityData, errorUrl }: { entityData: any, errorUrl?: string }) => {
- if (errorUrl) return `${errorUrl}-error`
- return `/icons/${entityData.guid}.png`
- })
-
- const { container } = render(
- <DisplayImage entity={entity} width={20} height={20} />
- )
-
- await waitFor(() => {
- const img = container.querySelector('img')
- expect(img).toBeInTheDocument()
- expect(img?.getAttribute('src')).toContain('-error')
- }, { timeout: 10000 })
- }, 20000)
+ const entity = { guid: 'entity-1' }
+
+ beforeEach(() => {
+ jest.clearAllMocks()
+
+ ;(Utils.getEntityIconPath as jest.Mock).mockImplementation(({ entityData,
errorUrl }: { entityData: any, errorUrl?: string }) => {
+ const result = errorUrl ? `${errorUrl}-fallback` :
`/icons/${entityData.guid}.png`
+ mockGetEntityIconPath({ entityData, errorUrl })
+ return result
+ })
+ })
+
+ it('renders primary image instantly', () => {
+ const { container } = render(
+ <DisplayImage entity={entity} width={20} height={20} />
+ )
+
+ const img = container.querySelector('img')
+ expect(img).toBeInTheDocument()
+ expect(img?.getAttribute('src')).toBe('/icons/entity-1.png')
+ expect(img?.getAttribute('alt')).toBe('Entity Icon')
+ expect(img?.getAttribute('id')).toBe('entity-1')
+ expect(img?.getAttribute('data-cy')).toBe('entity-1')
+ })
+
+ it('switches to fallback image when native onError is triggered', () => {
Review Comment:
LINE -55-68
No test that onerror = null prevents infinite loop when fallback also fails
No test for empty primaryUrl / fallbackUrl
##########
dashboard/src/views/SideBar/SideBarBody.tsx:
##########
@@ -101,61 +101,138 @@ const DrawerHeader = styled("div")(({ theme }) => ({
marginBottom: "1rem",
}));
+
const SideBarBody = (props: {
- loading: boolean;
- handleOpenModal: any;
- handleOpenAboutModal: any;
+ handleOpenModal: () => void;
+ handleOpenAboutModal: () => void;
}) => {
const location = useLocation();
const routes = useRoutes(AppRoutes as RouteObject[]);
const history = useHistory();
const dispatch = useAppDispatch();
- const { loading: loader, handleOpenModal, handleOpenAboutModal } = props;
+ const { handleOpenModal, handleOpenAboutModal } = props;
const navigate = useNavigate();
- const { loading } = useSelector((state: TypeHeaderState) =>
state.typeHeader);
const { relationshipSearch = {} } = globalSessionData || {};
const [open, setOpen] = useState(true);
const [searchTerm, setSearchTerm] = useState<string>("");
-
- const handleDrawerOpen = () => {
- setOpen(!open);
+ const { data: versionData, loading: isVersionLoading, error: versionError }
= useAppSelector((state) => state.session?.versionData || {});
+ const searchParams = new URLSearchParams(location.search);
+
+ const getActiveModule = () => {
+ if (searchParams.get("isCF") === "true") return "customFilters";
+ if (location.pathname.includes("/glossary") || !!searchParams.get("gtype")
|| !!searchParams.get("term") || !!searchParams.get("category")) return
"glossary";
+ if (location.pathname.includes("/administrator/businessMetadata")) return
"businessMetadata";
+ if (!!searchParams.get("tag") ||
location.pathname.includes("/tag/tagAttribute")) return "classification";
+ if (!!searchParams.get("relationshipName") ||
location.pathname.includes("/relationshipDetailPage")) return "relationships";
+ if (!!searchParams.get("type") ||
location.pathname.includes("/detailPage")) return "entities";
+ return null;
};
- const [position, setPosition] = useState<string |
number>(defaultDrawerWidth);
- const draggerRef = useRef<HTMLDivElement>(null);
- const headerRef = useRef<HTMLDivElement>(null);
- const windowWidth = window.innerWidth;
- const minPosition = 300;
- const maxPosition = windowWidth * 0.6;
+ const activeModule = getActiveModule();
+
+ const isCustomFilterActive = activeModule === "customFilters";
+ const isGlossaryActive = activeModule === "glossary";
+ const isBusinessMetadataActive = activeModule === "businessMetadata";
+ const isClassificationActive = activeModule === "classification";
+ const isRelationshipActive = activeModule === "relationships";
+ const isEntitiesActive = activeModule === "entities";
+
+ const modules = useMemo(() => [
+ { id: "entities", title: "Entities", isActive: isEntitiesActive, iconUrl:
"/img/sidebar-icons/icon-entities.svg", Component: EntitiesTree, isVisible:
true },
+ { id: "classification", title: "Classifications", isActive:
isClassificationActive, iconUrl: "/img/sidebar-icons/icon-classifications.svg",
Component: ClassificationTree, isVisible: true },
+ { id: "glossary", title: "Glossary", isActive: isGlossaryActive, iconUrl:
"/img/sidebar-icons/icon-glossary.svg", Component: GlossaryTree, isVisible:
true },
+ { id: "businessMetadata", title: "Business Metadata", isActive:
isBusinessMetadataActive, iconUrl:
"/img/sidebar-icons/icon-business-metadata.svg", Component:
BusinessMetadataTree, isVisible: true },
+ { id: "relationships", title: "Relationships", isActive:
isRelationshipActive, iconUrl: "/img/sidebar-icons/icon-relationships.svg",
Component: RelationshipsTree, isVisible: !!relationshipSearch },
+ { id: "customFilters", title: "Custom Filters", isActive:
isCustomFilterActive, iconUrl: "/img/sidebar-icons/icon-custom-filters.svg",
Component: CustomFiltersTree, isVisible: true }
+ ], [
+ isEntitiesActive,
+ isClassificationActive,
+ isGlossaryActive,
+ isBusinessMetadataActive,
+ isRelationshipActive,
+ isCustomFilterActive,
+ relationshipSearch
+ ]);
+
+ const [popoverAnchor, setPopoverAnchor] = useState<HTMLButtonElement |
null>(null);
+ const [activePopover, setActivePopover] = useState<string | null>(null);
+ const [popoverMaxHeight, setPopoverMaxHeight] = useState<string>('calc(100vh
- 100px)');
+ const [isBottomHalf, setIsBottomHalf] = useState<boolean>(false);
+ const popoverTimeoutRef = useRef<NodeJS.Timeout | null>(null);
+
+ useEffect(() => {
+ return () => {
+ if (popoverTimeoutRef.current) {
+ clearTimeout(popoverTimeoutRef.current);
+ }
+ };
+ }, []);
- const handleMouseMove = (e: MouseEvent) => {
- let newPosition = e.clientX;
+ const handlePopoverOpen = (event: React.MouseEvent<HTMLButtonElement>, id:
string) => {
+ const target = event.currentTarget;
+
+ const openNewPopover = () => {
+ setPopoverAnchor(target);
+ setActivePopover(id);
+
+ // Calculate remaining screen height from the anchor to the bottom
+ const rect = target.getBoundingClientRect();
+ const spaceBelow = window.innerHeight - rect.top - 24;
Review Comment:
setTimeout(openNewPopover, 0) when switching popovers works but may flicker.
Consider MUI TransitionProps or keeping anchor and swapping content without
unmount.
##########
dashboard/src/components/SidebarSearchInput.tsx:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import React, { ChangeEvent } from "react";
+import { Paper, InputBase, Stack } from "@mui/material";
+import ClearIcon from "@mui/icons-material/Clear";
+import { IconButton } from "@components/muiComponents";
+
+interface SidebarSearchInputProps {
+ searchTerm: string;
+ onChange: (value: string) => void;
+ dataCy?: string;
+}
+
+export const SidebarSearchInput: React.FC<SidebarSearchInputProps> = ({
+ searchTerm,
+ onChange,
+ dataCy
+}) => (
+ <Paper
+ sx={{
+ width: "100%",
+ paddingLeft: "8px",
+ display: "flex",
+ alignItems: "center"
+ }}
+ className="sidebar-searchbar"
+ >
+ <InputBase
+ fullWidth
+ sx={{ color: "rgba(0, 0, 0, 0.7)" }}
+ placeholder="Search"
+ inputProps={{ "aria-label": "search" }}
+ value={searchTerm}
+ onChange={(e: ChangeEvent<HTMLInputElement>) => onChange(e.target.value)}
+ data-cy={dataCy}
+ endAdornment={
+ <Stack direction="row" alignItems="center" gap="4px">
+ {searchTerm.length > 0 && (
+ <IconButton
+ aria-label="Clear search"
+ size="small"
+ onClick={() => onChange("")}
+ edge="end"
+ sx={{ padding: "4px" }}
+ >
+ <ClearIcon fontSize="small" sx={{ color: "rgba(0, 0, 0, 0.4)" }}
/>
+ </IconButton>
+ )}
+ <img
+ src="/img/sidebar-icons/icon-search.svg"
Review Comment:
line 65-71
Inline styles for search icon — consider moving to sidebar.scss for
consistency.
--
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]
