Skip to content

feat: migrate drawer components from MUI to SlideoutMenu in untitled UI#26518

Open
Rohit0301 wants to merge 11 commits intomainfrom
untitled-ui-drawer
Open

feat: migrate drawer components from MUI to SlideoutMenu in untitled UI#26518
Rohit0301 wants to merge 11 commits intomainfrom
untitled-ui-drawer

Conversation

@Rohit0301
Copy link
Contributor

@Rohit0301 Rohit0301 commented Mar 16, 2026

Describe your changes:

Fixes

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Migration to SlideoutMenu:
    • Replaced MUI Drawer with SlideoutMenu from @openmetadata/ui-core-components in drawer hooks
    • Removed anchor, height, and zIndex configuration options (SlideoutMenu defaults to right anchor)
  • Drawer hook refactoring:
    • Simplified useDrawer with handleOpenChange callback replacing MUI's onClose handler
    • Updated useDrawerBody to use SlideoutMenu.Content with Tailwind classes
    • Refactored useDrawerFooter and useDrawerHeader to use SlideoutMenu subcomponents
  • Code cleanup:
    • Removed DomainsListPage.constants.ts and associated DRAWER_HEADER_STYLING references
    • Removed extensive JSDoc comments and unused MUI Box wrappers
    • Updated button configurations to use new color API (primary, secondary, tertiary)
  • Bug fixes:
    • Fixed form validation and drawer validation issues in prior commits

This will update automatically on new commits.

@Rohit0301 Rohit0301 self-assigned this Mar 16, 2026
@Rohit0301 Rohit0301 requested a review from a team as a code owner March 16, 2026 10:31
@Rohit0301 Rohit0301 added the safe to test Add this label to run secure Github workflows on PRs label Mar 16, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
65% (58002/89221) 44.79% (30675/68481) 47.76% (9176/19211)

@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2026

🟡 Playwright Results — all passed (30 flaky)

✅ 3323 passed · ❌ 0 failed · 🟡 30 flaky · ⏭️ 183 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 451 0 4 2
🟡 Shard 2 302 0 3 1
🟡 Shard 3 653 0 8 33
🟡 Shard 4 721 0 6 47
🟡 Shard 5 588 0 3 67
🟡 Shard 6 608 0 6 33
🟡 30 flaky test(s) (passed on retry)
  • Features/DataAssetRulesDisabled.spec.ts › Verify the Drive Service entity item action after rules disabled (shard 1, 1 retry)
  • Features/CustomizeDetailPage.spec.ts › Ml Model - customization should work (shard 1, 1 retry)
  • Flow/Tour.spec.ts › Tour should work from welcome screen (shard 1, 1 retry)
  • Flow/Tour.spec.ts › Tour should work from URL directly (shard 1, 1 retry)
  • Features/ColumnBulkOperations.spec.ts › should search columns with server-side API call (shard 2, 1 retry)
  • Features/ColumnBulkOperations.spec.ts › should cancel selection and disable edit button (shard 2, 1 retry)
  • Features/ColumnBulkOperations.spec.ts › should discard changes when closing drawer without saving (shard 2, 1 retry)
  • Features/BulkImport.spec.ts › Database (shard 3, 1 retry)
  • Features/CustomMetric.spec.ts › Table custom metric (shard 3, 1 retry)
  • Features/DataQuality/TestCaseImportExportE2eFlow.spec.ts › EditAll User: Complete export-import-validate flow (shard 3, 1 retry)
  • Features/DataQuality/TestCaseIncidentPermissions.spec.ts › User with TEST_CASE.EDIT_ALL can see edit icon on incidents (shard 3, 1 retry)
  • Features/DataQuality/TestCaseResultPermissions.spec.ts › User with TEST_CASE.EDIT_ALL can see edit action on test case (shard 3, 1 retry)
  • Features/Permissions/EntityPermissions.spec.ts › Dashboard deny entity-specific permission operations (shard 3, 1 retry)
  • Features/Permissions/EntityPermissions.spec.ts › Directory deny common operations permissions (shard 3, 1 retry)
  • Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 3, 1 retry)
  • Pages/Customproperties-part2.spec.ts › entityReferenceList shows item count, scrollable list, no expand toggle (shard 4, 1 retry)
  • Pages/DescriptionVisibility.spec.ts › Data Product truncates long description and end of text is not visible before expand (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Rename domain with subdomains attached verifies subdomain accessibility (shard 4, 1 retry)
  • Pages/DomainUIInteractions.spec.ts › Delete domain with subdomains shows warning (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Inactive Announcement create & delete (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Glossary Term Add, Update and Remove for child entities (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Announcement create, edit & delete (shard 5, 1 retry)
  • Pages/Entity.spec.ts › Tier Add, Update and Remove (shard 5, 1 retry)
  • Pages/EntityDataSteward.spec.ts › Glossary Term Add, Update and Remove (shard 5, 1 retry)
  • Pages/Glossary.spec.ts › Add, Update and Verify Data Glossary Term (shard 6, 1 retry)
  • Pages/Glossary.spec.ts › Column dropdown drag-and-drop functionality for Glossary Terms table (shard 6, 1 retry)
  • Pages/HyperlinkCustomProperty.spec.ts › should display URL when no display text is provided (shard 6, 1 retry)
  • Pages/InputOutputPorts.spec.ts › Remove single output port (shard 6, 1 retry)
  • Pages/ODCSImportExport.spec.ts › Multi-object ODCS contract - object selector shows all schema objects (shard 6, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@siddhant1
Copy link
Member

@gitar-bot do an in depth frontend review of this refactor

karanh37
karanh37 previously approved these changes Mar 20, 2026
@gitar-bot
Copy link

gitar-bot bot commented Mar 20, 2026

@gitar-bot do an in depth frontend review of this refactor

Here is the in-depth frontend review of this MUI → SlideoutMenu drawer refactor.


In-Depth Frontend Review

Critical Bugs

1. useFormDrawer submit handler is missing await — async onSubmit silently fails

useFormDrawer.tsx:132–142

onClick: () => {          // ← sync function
  try {
    setIsSubmitting(true);
    onSubmit({} as T);    // ← Promise returned but NOT awaited
  } catch {
    // this catch will never fire for rejected promises
  } finally {
    setIsSubmitting(false); // fires immediately, before API call completes
  }
}

For any async onSubmit (the overwhelming majority of form submissions), isSubmitting resets to false the instant the Promise is created, not when it resolves. The submit button re-enables while the request is still in flight, users can double-submit, and any API error is silently swallowed as an unhandled rejection. useFormDrawerWithRef.handleSubmit (line 191) correctly uses async/await — the base useFormDrawer must match:

onClick: async () => {
  try {
    setIsSubmitting(true);
    await onSubmit({} as T);
  } catch {
    // errors handled by caller
  } finally {
    setIsSubmitting(false);
  }
}

2. Overlay loader does not actually overlay — children remain visible during loading

useDrawerBody.tsx:29–36 — The <div> with tw:absolute tw:inset-0 needs its parent to be position: relative for absolute to work. SlideoutMenu.Content renders a plain <div role="main"> with no position: relative, so the overlay div will escape its intended bounds and cover content outside the drawer. Either add tw:relative to a wrapper, or simply hide children while loading:

<SlideoutMenu.Content className={cx('tw:relative', className)}>
  {loading && (
    <div className="tw:absolute tw:inset-0 tw:z-10 tw:flex tw:items-center tw:justify-center tw:bg-primary/60">
      <Loader />
    </div>
  )}
  {children}
</SlideoutMenu.Content>

3. Loading overlay color is tw:bg-primary/60 — this is the brand primary colour at 60% opacity

useDrawerBody.tsx:31 — The old implementation used rgba(255,255,255,0.8) (white). The new value tw:bg-primary/60 maps to the design system's primary brand colour (blue/purple) at 60% opacity, which will produce a strongly tinted dark overlay instead of the expected translucent white scrim. This is a visual regression. It should be tw:bg-white/80 or the correct design token for a white overlay background.


Design/Architecture Issues

4. useCompositeDrawer clones React elements by destructuring .type and .props — fragile and bypasses refs

useCompositeDrawer.tsx:65–70

const compositeDrawer = useMemo(() => {
  const DrawerComponent = baseDrawer.drawer;
  const Component = DrawerComponent.type;   // extracts Menu function
  return <Component {...DrawerComponent.props}>{drawerContent}</Component>;
}, [baseDrawer.drawer, drawerContent]);

This pattern manually reconstructs a React element by pulling type and props off a memoized element. It will silently break if baseDrawer.drawer ever wraps in a Fragment, is a forwardRef, or is null. Refs attached to baseDrawer.drawer are also lost. The correct approach is React.cloneElement:

const compositeDrawer = useMemo(
  () => React.cloneElement(baseDrawer.drawer, {}, drawerContent),
  [baseDrawer.drawer, drawerContent]
);

Or better yet, simply pass children through config.children in useDrawer and avoid cloning entirely — the compositeDrawer pattern could render {drawer} with content composed as children.

5. useAssetSelectionDrawer has an assetDrawer in the useEffect dependency array — unstable reference causes infinite loops

useAssetSelectionDrawer.tsx:83–89

useEffect(() => {
  if (open) {
    assetDrawer.openDrawer();
  } else {
    assetDrawer.closeDrawer();
  }
}, [open, assetDrawer]); // ← assetDrawer is a new object every render

assetDrawer is the full return value of useCompositeDrawer, which is a plain object created fresh each render. This makes the effect fire on every render. The fix is to destructure the stable callbacks:

const { openDrawer, closeDrawer } = assetDrawer;
useEffect(() => {
  if (open) openDrawer();
  else closeDrawer();
}, [open, openDrawer, closeDrawer]);

6. closeOnEscape / closeOnBackdrop semantics are inconsistent with the new API

useDrawer.tsx:60–62

isDismissable={config.closeOnBackdrop !== false}
isKeyboardDismissDisabled={config.closeOnEscape === false}

These map correctly in isolation, but note: when isKeyboardDismissDisabled={false} (the default), SlideoutMenu defers escape-key handling to react-aria, which calls onOpenChange(false)closeDrawer(). If a caller also sets onClose on the DrawerConfig, it will be called twice — once from handleOpenChange → closeDrawer and once from config.onClose registered inside closeDrawer. Audit all callers that set both closeOnEscape and onClose.


Performance Issues

7. Buttons in useDrawerFooter use label as React key — breaks when labels are i18n translated and change

useDrawerFooter.tsx:91key={button.label} relies on the label string being stable. If the label changes (e.g., i18n re-init, dynamic labels), React will unmount and remount the button instead of reconciling in place. A stable testId would be a better key:

key={button.testId ?? button.label}

8. useMemo in drawer hooks captures the entire config object indirectly, defeating memoization

useDrawer.tsx:56–77 — The drawer useMemo lists config.closeOnBackdrop, config.closeOnEscape, and config.width individually in the dep array, which is correct. However config.children can be a freshly created element (e.g., <MyForm /> rendered inline), meaning the drawer re-creates every render. This is expected but worth documenting — callers should memoize children.


API/TypeScript Issues

9. DrawerConfig still has no anchor prop — but the type comments and doc-strings still reference it

Multiple callers previously passed anchor: 'right', which was removed from DrawerConfig. The removal is correct, but the public-facing comments in useFormDrawer.tsx (lines 56–73) still describe props that no longer exist. More importantly, if any external consumer (outside this PR's files) was passing anchor or header.sx, TypeScript will correctly error — but a full audit of consumers outside the changed files is needed.

10. DrawerBodyConfig.loadingMessage removed — silent breaking change for existing callers

loadingMessage was removed from DrawerBodyConfig. Any caller that previously passed loadingMessage will now get a TypeScript error. The CI analysis shows no test failures, but this should be confirmed with a project-wide grep for loadingMessage usage.

11. useFormDrawer submit passes {} as T — dangerous type cast

useFormDrawer.tsx:135onSubmit({} as T) is called in the base path of useFormDrawer.onClick. The empty object forced to type T could cause runtime errors if onSubmit accesses fields on its argument. This codepath is meant to be overridden by useFormDrawerWithRef.handleSubmit, but if someone uses useFormDrawer directly (without a formRef), they'll receive {} instead of actual form values. The comment says "Don't close drawer here" but doesn't explain why {} is safe — if it isn't, this should throw.


Accessibility

12. SlideoutMenu.Header always renders a CloseButtonshowCloseButton={false} has no effect

useDrawerHeader.tsx:29 — When showCloseButton is false, onClose is passed as undefined to SlideoutMenu.Header. However, inspecting slideout-menu.tsx:155–158, the CloseButton is always rendered regardless of whether onClose is undefined. A CloseButton with no onClick is a tabbable, focusable element that does nothing — this is both a UX bug and an accessibility violation (keyboard users will tab to a button that has no action). If the intent is to hide the button, the Header's onClose prop should conditionally suppress rendering:

<SlideoutMenu.Header onClose={showCloseButton ? onClose : undefined}>

This is what the code does — but since SlideoutMenu.Header always renders the button regardless, showCloseButton={false} is silently broken. This needs to be fixed in the SlideoutMenu.Header implementation:

// In slideout-menu.tsx Header
{onClose !== undefined && (
  <CloseButton size="md" className="..." onClick={onClose} />
)}

13. Typography as="h4" inside SlideoutMenu.Header adds extra <div class="prose"> wrapper

useDrawerHeader.tsx:32Typography always wraps content in <div class="prose">. Inside a <header> element, this produces: <header> <div> <div class="prose"> <h4>Title</h4> </div> </div> </header>. The extra prose div breaks header flex layout because the tw:flex tw:items-center tw:gap-2 tw:flex-1 is on the outer <div> while <h4> is nested two levels deeper. The prose wrapper may also conflict with the header's own styling. Verify the visual output carefully.


Summary Table

# Severity File Issue
1 🔴 Critical useFormDrawer.tsx:132 Missing await on onSubmit — double-submit, spinner resets early
2 🔴 Critical useDrawerBody.tsx:29 Parent not relative — absolute overlay escapes drawer bounds
3 🔴 Critical useDrawerBody.tsx:31 tw:bg-primary/60 is brand colour, not white — wrong overlay colour
4 🟠 High useCompositeDrawer.tsx:65 Manual .type/.props clone is fragile — use React.cloneElement
5 🟠 High useAssetSelectionDrawer.tsx:83 assetDrawer in useEffect deps causes infinite re-runs
6 🟡 Medium useDrawer.tsx:60 Double-firing onClose when both onClose and isKeyboardDismissible are active
7 🟡 Medium useDrawerFooter.tsx:91 Button key={button.label} unstable under i18n
8 🟡 Medium useDrawerHeader.tsx:29 showCloseButton={false} silently broken — SlideoutMenu.Header always renders CloseButton
9 🟡 Medium useDrawerHeader.tsx:32 Extra <div class="prose"> inside header breaks flex layout
10 🟡 Medium useFormDrawer.tsx:135 {} as T type cast — dangerous for direct useFormDrawer callers
11 🟢 Low DrawerBodyConfig loadingMessage / padding removal is a silent breaking change for existing callers
12 🟢 Low DrawerConfig anchor removal should be verified against all non-PR consumers

@gitar-bot
Copy link

gitar-bot bot commented Mar 20, 2026

Code Review ✅ Approved 5 resolved / 5 findings

Migrates drawer components from MUI to SlideoutMenu in the untitled UI library, resolving five issues including dynamic Tailwind class generation, loading state overlay behavior, anchor configuration handling, async form submission, and typography defaults. All findings have been addressed.

✅ 5 resolved
Bug: Dynamic Tailwind classes won't generate CSS at build time

📄 openmetadata-ui/src/main/resources/ui/src/components/common/atoms/drawer/useDrawer.tsx:29 📄 openmetadata-ui/src/main/resources/ui/src/components/common/atoms/drawer/useDrawer.tsx:74
The getWidthClassName function constructs Tailwind arbitrary-value classes dynamically via template literals:

const value = typeof width === 'number' ? `${width}px` : width;
return `tw:max-w-[${value}] tw:w-[${value}]`;

Tailwind v4 (used in this project) performs static analysis of source files at build time. Dynamically constructed class names like tw:w-[500px] are never seen by the scanner, so no CSS will be generated. This means the drawer width prop will silently have no effect — all drawers will render at their default width.

Moreover, SlideoutMenu already exposes a native width prop that applies style={{ maxWidth: width }} directly. The type cast on line 74 (as Record<string, unknown>) is also a code smell — it's working around the fact that the approach is fighting the component's API.

Fix: Use SlideoutMenu's built-in width prop instead of generating Tailwind classes.

Bug: Loading state no longer overlays content in drawer body

📄 openmetadata-ui/src/main/resources/ui/src/components/common/atoms/drawer/useDrawerBody.tsx:29
The old useDrawerBody rendered the loader as an absolute-positioned overlay with a semi-transparent white backdrop (rgba(255,255,255,0.8)), visually obscuring the content below while loading. The new implementation renders <Loader /> and {children} as siblings in normal document flow:

<SlideoutMenu.Content className={className}>
  {loading && <Loader />}
  {children}
</SlideoutMenu.Content>

This means during loading, users see both the spinner AND the stale/partial content simultaneously, which is a UX regression. Consider either hiding children while loading or wrapping the content in a relative container with an overlay loader.

Quality: DrawerConfig.anchor is declared but silently ignored

📄 openmetadata-ui/src/main/resources/ui/src/components/common/atoms/drawer/useDrawer.tsx:18
The DrawerConfig interface still declares anchor?: 'left' | 'right' | 'top' | 'bottom', and all existing callers pass anchor: 'right'. However, SlideoutMenu is hardcoded to slide in from the right only — the anchor prop is never forwarded. This means a future caller passing anchor: 'left' would compile without error but the drawer would still open from the right. Similarly, height was removed from DrawerConfig but anchor remains — creating a misleading API surface.

Consider removing anchor from DrawerConfig (or restricting it to 'right') to avoid confusion.

Bug: Removing await breaks async onSubmit and isSubmitting state

📄 openmetadata-ui/src/main/resources/ui/src/components/common/atoms/drawer/useFormDrawer.tsx:132
The await was removed from the onSubmit({} as T) call and the handler changed from async to synchronous. If onSubmit returns a Promise (which is the common case for form submissions making API calls), the catch block will no longer catch rejected promises, and setIsSubmitting(false) in finally will execute immediately—before the async operation completes. This means:

  1. isSubmitting resets to false instantly, so the loading spinner disappears and the button re-enables while the request is still in flight.
  2. Promise rejections from onSubmit become unhandled, potentially causing uncaught promise rejection errors.
  3. Users can double-submit because the button is no longer disabled during the actual async work.
Bug: Typography default removal is a breaking change for all callers

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/foundations/typography.tsx:77 📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/foundations/typography.tsx:78 📄 openmetadata-ui/src/main/resources/ui/src/components/common/atoms/drawer/useDrawerHeader.tsx:32
Removing the default values for size (was "text-sm") and weight (was "regular") from Typography is a cross-cutting behavioral change. Every existing <Typography> usage that omits these props — including the new <Typography as="h4"> in useDrawerHeader.tsx and many others throughout stories, Popover, Tabs, etc. — will no longer receive any Tailwind size/weight utility class. Instead of the previous explicit tw:text-sm tw:font-normal, these elements will now rely entirely on inherited CSS or the prose wrapper's styles, which may produce different visual results.

This is a shared foundation component, so the blast radius extends well beyond this PR's drawer migration. If the intent is to let semantic elements like <h4> use their natural sizing, consider making the defaults conditional on the as prop rather than removing them entirely.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants