ATLAS-5324: Enhance Collapsed Sidebar with Module Icons, Interactive Tree Tooltips, and Active State Markers - #688
Open
Brijesh619 wants to merge 17 commits into
Open
ATLAS-5324: Enhance Collapsed Sidebar with Module Icons, Interactive Tree Tooltips, and Active State Markers#688Brijesh619 wants to merge 17 commits into
Brijesh619 wants to merge 17 commits into
Conversation
Contributor
Author
|
Contributor
Author
|
pawarprasad123
self-requested a review
July 8, 2026 06:59
Brijesh619
force-pushed
the
ATLAS-5324
branch
3 times, most recently
from
July 8, 2026 13:49
fdc0a62 to
95130fa
Compare
Contributor
Author
|
Brijesh619
changed the title
Contributor
Author
|
Contributor
Author
|
CC. @mneethiraj |
Contributor
Author
|
Contributor
Author
|
Contributor
Author
|
Contributor
Author
|
Contributor
Author
|
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
Contributor
Author
|
…Tree Tooltips, and Active State Markers
Contributor
Author
|
…Tree Tooltips, and Active State Markers
…Tree Tooltips, and Active State Markers
Contributor
Author
|
| <Typography variant="body2" className="sidebar-version-text"> | ||
| {isVersionLoading ? ( | ||
| <CircularProgress size={12} className="sidebar-version-loader" /> | ||
| ) : versionError ? ( |
Contributor
There was a problem hiding this 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.
| builder.addCase(fetchVersionData.pending, (state) => { | ||
| state.versionData.loading = true; | ||
| state.versionData.error = null; | ||
| }), |
Contributor
There was a problem hiding this 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.
| id={entity.guid} | ||
| data-cy={entity.guid} | ||
| src={imageUrl} | ||
| id={entity.guid ? String(entity.guid) : undefined} |
Contributor
There was a problem hiding this 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.
| expect(sessionSlice.fetchVersionData).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should pass loading state to tree components', () => { |
Contributor
There was a problem hiding this 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.
| @@ -15,269 +15,169 @@ | |||
| * limitations under the License. | |||
| */ | |||
Contributor
There was a problem hiding this 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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR significantly enhances the user experience of the collapsed navigation sidebar by introducing interactive module icons and sleek popover trees. This allows users to access complex navigation (like Business Metadata, Glossary, etc.) without fully expanding the main sidebar, preserving valuable screen real-estate for the main dashboard.
Additionally, it introduces several critical layout stability, styling, strict-typing, and rendering optimizations to ensure these interactions feel instant, seamless, and production-ready:
Sidebar & Layout Enhancements:
SideBarBody.tsx): Fixed a major layout bug where collapsing the sidebar caused the main dashboard content to shift left underneath the mini-sidebar, cutting off the left half of the page cards (e.g., theOverviewcard). Refactored the<Main>styled container to eliminate legacy negative margins (marginLeft: -20%) andpadding: theme.spacing(3), converting<Main>into a flex item (flexGrow: 1,minWidth: 0). It now dynamically adapts next to both the 20% expanded drawer and 60px mini-drawer without cutting off content or causing whitespace gaps around the header.SideBarBody): Fixed a performance regression where toggling the sidebar open/closed caused the entire tree component (and data) to unmount and remount. Refactored<Paper>wrappers to use SCSS visibility hooks (.sidebar-wrapper--hidden) instead of inline display toggles, ensuring the data trees remain mounted, instantly available, and completely stable during interactions.sessionSlice.tswhere the sidebar version footer flashed empty during refetches. It now retains previous data duringpendingactions for a seamless UI experience.TreeSkeletonLoader): Added anisPopovercontext flag so the popover tree dynamically renders a minimal 2-row loading skeleton instead of the default 7-row skeleton, perfectly matching the popover bounds without visual snapping. Fortified the skeleton loader to safely handle negative bounds and explicitundefinedfallbacks.Rendering, Strict Typing & Code Quality Optimizations:
anyRefactoring (AGENTS.mdCompliance): Refactored all dashboard charts (ClassificationDistributionCard,EntityStatusDonut,EntityTypeBarChart,MessageConsumptionChart, andEntityStatsChart) to completely eliminateanyand unsafe type assertions:(v: any) => numberFormatWithComma(v as number)with safe runtime type guards(v: unknown) => (typeof v === "number" ? numberFormatWithComma(v) : "").LegendPayloadtype inEntityStatsChart.tsxto fix aTS2305error caused byrechartsomitting the export.chartDatatuples inEntityStatusDonutasArray<{ name: string; value: number; color: string }>to resolve RechartsPiePropsgeneric overload mismatches.Record<any, any>with strictRecord<string, unknown>interfaces insessionSlice.ts, cascading safe string castings downstream (e.g.,timezonesandVersion) to completely avoid unrenderableunknownReact nodes.globalSessionData?.relationshipSearchsafely as a boolean (Boolean(...)) inSideBarBody.tsxand strictly typedtoastIdinTreeNodeIcons.tsxasuseRef<number | string | null>(null)with proper null guards aroundtoast.dismiss().EntityDisplayImage): Fixed an underlying bottleneck where layout shifts caused the Search Data Table to remount cells, triggering heavy network fetch requests for entity icons and flashing loading skeletons. Completely refactoredEntityDisplayImageto drop ReactuseStatetracking in favor of synchronous browser-native<img onError>fallbacks.idanddata-cyassignments with safe string casting to prevent invalid"undefined"DOM attributes when an entity is missing itsguid.sxprops) and inline styling (style={{...}}) across components likeQuickSearch.tsx,TreeNodeIcons.tsx,SideBarTree.tsx, andSideBarBody.tsx. Replaced hardcodedrgba(255,255,255,0.08)styling inTreeSkeletonLoader.tsxwith themed SCSS CSS variable fallbackvar(--skeleton-bg, rgba(255,255,255,0.08)). All layout configurations (Drawer, Popover, module hover states, footers) have been fully abstracted into shared SCSS classes (globalSearch.scss,sidebar.scss), utilizing proper modifiers like.sidebar-tree-box--hidden.How was this patch tested?
Note - Sidebar Resize Logic Removal:
The
draggerRefand drawer resizing mouse event listeners (handleMouseDown,handleMouseMove, etc.) were intentionally removed in this PR. This resize logic was pre-existing dead code onmasterthat lacked a functional DOM dragger element, did not actually work, and needlessly bound unmounting event listeners. Removing it cleanly aligns with this PR's sidebar cleanup and prevents unused state bloat.Manual UI Testing:
Escapekey to close active Popovers) functions natively.Automated Test Suite Expansion (4,865 Passing Tests):
SideBarBody.test.tsxto strictly assert that components do not unmount when toggling drawer states or closing popovers, leveraging strict.toHaveClass('sidebar-wrapper--hidden')assertions rather than inline style checks.SideBarBody.test.tsxto explicitly assert the arguments passed intomockNavigate(e.g.,'/search') rather than just checking if the logo remained in the document.TreeSkeletonLoader.test.tsxverifying safe behavior against negative values andundefined. Added tests inEntityDisplayImage.test.tsxverifying that missingguids yield cleannullDOM attributes rather than"undefined"strings.sessionSlice.test.ts.TextEncoderandTextDecoderpolyfills globally insetupTests.tsandsetupTests.simple.tsto seamlessly supportreact-router-dommodule upgrades in the JSDom environment.fetchmocks for image rendering inEntityDisplayImage.test.tsxand properly implemented nativefireEvent.errorDOM fallback simulation.About.test.tsxto ensure users see explicit "Unknown (failed to fetch version)" text upon a failed API call.useNavigatemocks inEntityStatusDonut.test.tsx.relationshipSearchglobals so states are cleanly restored without leaking across test boundaries.SidebarSearchInput.test.tsx&TreeSkeletonLoader.test.tsx) for accessibility and edge cases.Performance & Type Verification:
npm run typecheckandnpm run testlocally and verified that the refactored native DOM logic, SCSS migrations, layout fixes, and TypeScript strict mode types pass with 0 errors.