ref(slideoverpanel) refactor using core primitives #107127

JonasBa · 2026-01-27T23:51:02Z

Rewrite SlideOverPanel using design system primitives and remove unused bottom sizing.

It is a tad unfortunate that left size configs use relative positioning vs fixed, but I assume this has something to do with our sidebar (nevertheless, we need a more robust solution)

I have likewise removed the transition prop from this component as the escape hatch to the entire transition config requires proper encapsulation

JonasBa · 2026-01-27T23:57:28Z

@cursoragent review

cursor · 2026-01-27T23:57:38Z

Now let me provide a comprehensive code review of this PR.

Code Review: SlideOverPanel Refactor using Core Primitives

Overview

This PR successfully refactors SlideOverPanel to use core layout primitives (Surface and Container), which is a great step toward consistency and maintainability in the design system.

Strengths ✅

Cleaner Architecture: Replacing the custom styled component with Surface and Container primitives significantly simplifies the implementation and makes it more consistent with the design system.
Reduced Complexity: The refactor removes ~110 lines of styled-components CSS in favor of declarative props, making the code more readable and maintainable.
Better Prop API: The changes improve the component API:
- position → placement (more semantic)
- panelWidth → width (consistent with layout primitives)
- ariaLabel removed (should be handled by consumers)
Type Safety Improvements: Using Omit<React.ComponentProps<typeof MotionSurface>, 'children' | 'transition'> provides better TypeScript integration with the underlying Surface component.
Responsive Design Enhancement: The new implementation uses responsive props for positioning (e.g., height: {'2xs': 'calc(100vh - ${theme.space.lg})', sm: '100%'}), which is more flexible than media queries.
Extended Primitives: Adding overscrollBehavior support to Container is a useful addition that benefits the entire design system.

Concerns & Questions 🤔

Breaking Changes - Bottom Placement Removed

The original implementation supported three placements: right, left, and bottom. The refactored version only supports right and left:

const BOTTOM_SIDE_PANEL_HEIGHT = '50vh';

const COLLAPSED_STYLES = {
  bottom: {
    transform: `translateX(0) translateY(${BOTTOM_SIDE_PANEL_HEIGHT})`,
    opacity: 0,
  },
  right: {transform: `translateX(${RIGHT_SIDE_PANEL_WIDTH}) translateY(0)`, opacity: 0},
  left: {transform: `translateX(-${LEFT_SIDE_PANEL_WIDTH}) translateY(0)`, opacity: 0},
};

Issue: The COLLAPSED_STYLES still includes bottom, but getSlideoutPlacementStyles() doesn't handle it, and the TypeScript type excludes it. This creates confusion and dead code.

Recommendation: Either:

Add back support for bottom placement, or
Remove bottom from COLLAPSED_STYLES and OPEN_STYLES objects
Search codebase to ensure no consumers are using position="bottom"

Type Casting Concern

        {...(props as any)}

Issue: The as any cast bypasses type safety. The comment mentions conflicts with HTMLMotionProps, but this could hide real type issues.

Recommendation: Consider a more precise type assertion or explicitly type the conflicting props.

Accessibility Regression

The original component had:
- aria-label prop (explicit accessibility label)
- aria-hidden={false} (explicitly not hidden)
The new version doesn't explicitly handle these. While consumers can pass these via spread props, it's less discoverable.

Recommendation: Consider keeping aria-label as an explicit prop for better accessibility guidance, or document in the component's JSDoc that consumers should provide it.
Missing data-test-id Prop

The original supported data-test-id as a direct prop. The new version requires consumers to know it comes from spread props.

Recommendation: Keep commonly-used testing props as explicit in the interface for better DX.
Hard-coded zIndex

        style={{zIndex: theme.zIndex.drawer}}

Issue: This uses inline style which overrides any zIndex passed via props and breaks the declarative prop model.

Recommendation: Consider adding zIndex support to Container/Surface primitives, or find another way to set this without inline styles.

Documentation Update Needed

The MDX documentation still references the old API in some places. For example:

import SlideOverDocumentation from '!!type-loader!@sentry/scraps/slideOverPanel';

The examples look correct, but verify that all references are updated.

Minor Suggestions 💡

Consistent Width Types: The width handling is a bit complex:

        width:
          typeof props.width === 'string'
            ? {
                '2xs': `calc(100vw - ${theme.space.lg})`,
                sm: props.width ?? RIGHT_SIDE_PANEL_WIDTH,
              }
            : props.width,

Consider documenting what non-string width values are expected (responsive objects?).

Export ContainerProps: The code imports ContainerProps but consumers might not know about this type:

import {Surface, type ContainerProps} from '@sentry/scraps/layout';

Verify this is properly exported from the package.

Testing Recommendations 🧪

Visual Regression Tests: Ensure the refactored component looks identical to the original
Animation Tests: Verify transitions work correctly with the new framer-motion integration
Responsive Tests: Test the responsive positioning behavior at various breakpoints
Accessibility Tests: Verify ARIA attributes are properly set by consumers

Overall Assessment 🎯

This is a solid refactor that improves code quality and maintainability. The main concerns are:

Remove dead code related to bottom placement
Fix the as any type cast
Consider accessibility prop ergonomics
Address the inline zIndex style

Recommendation: Address the breaking changes and type safety concerns before merging. The architectural direction is excellent.

JonasBa · 2026-01-27T23:57:44Z

@sentry review

JonasBa · 2026-01-27T23:59:00Z

static/app/components/core/slideOverPanel/slideOverPanel.tsx

-        aria-hidden={false}
-        aria-label={ariaLabel ?? 'slide out drawer'}


Bad aria is worse than no aria

JonasBa · 2026-01-28T16:10:07Z

static/app/components/core/slideOverPanel/slideOverPanel.tsx

+        transition={theme.motion.framer.spring.moderate}
+        // Polymorphism is not very well supported by emotion and in this case conflicts with the
+        // HTMLMotionProps type on the onAnimationStart and onDrag members.
+        {...(props as any)}


I'm trying to find a better solution to this, but framer-motion types do not appear to play well with polymorphism

the big problem with an object spread of as any is that it silences all type errors on the whole “object”, as it widens the complete type to any, and in case of a react component, the affected “object” is all props passed to it.

That means we will never get reported any type-errors of other props passed to MotionSurface here. For example, if I change overflow="auto" to overflow="foobar”, there is no error. This makes future refactorings really hard as we rely on the type checker.

The better workaround is to do {...(props as Record<string, unknown>)}. It silences the props you’re spreading, but not all other props.

I have a blogpost on this too: https://tkdodo.eu/blog/beware-the-leaking-any#when-any-is-leaking

- Add missing ariaLabel parameter to waitForDrawerToHide calls - Remove unused within imports from test files

static/app/components/globalDrawer/components.tsx

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

cursor · 2026-01-28T19:54:37Z

static/app/components/core/slideOverPanel/slideOverPanel.tsx

+      return {
+        position: {'2xs': 'fixed', sm: 'relative'},
+        width: isResponsive(props.width)
+          ? (props.width ?? LEFT_SIDE_PANEL_WIDTH)


Unreachable fallback code in left placement width logic

Low Severity

The fallback ?? LEFT_SIDE_PANEL_WIDTH in the responsive branch is unreachable. The isResponsive function returns true only when props.width is an object that is not null. Since the nullish coalescing operator (??) only triggers for null or undefined, and we've already confirmed props.width is a non-null object in this branch, LEFT_SIDE_PANEL_WIDTH can never be used. The right case correctly handles this by just using props.width directly without the redundant fallback.

The SlideOverPanel refactor removed the ariaLabel prop, causing tests that queried drawers using getByLabelText() or getByRole('complementary') to fail. Updated all affected tests to use data-test-id attributes instead. Changes: - Added test IDs to drawer components (schema hints, automations, monitors) - Updated all failing test files to query by test ID instead of aria label - Follows the pattern established in globalDrawer/index.spec.tsx

TkDodo · 2026-01-29T10:08:42Z

static/app/components/core/layout/styles.tsx

+export function isResponsive(
+  prop: unknown
+): prop is Partial<Record<BreakpointSize, any>> {


if this function should do an isResponsive check, I guess the input needs to be tightened, as unknown an accept anything really. Otherwise, this is just an isObject check with no guarantees that we actually get a Record<BreakpointSize, any> back.

TkDodo · 2026-01-29T10:14:44Z

static/app/components/core/slideOverPanel/slideOverPanel.tsx

+        transition={theme.motion.framer.spring.moderate}
+        // Polymorphism is not very well supported by emotion and in this case conflicts with the
+        // HTMLMotionProps type on the onAnimationStart and onDrag members.
+        {...(props as any)}


the big problem with an object spread of as any is that it silences all type errors on the whole “object”, as it widens the complete type to any, and in case of a react component, the affected “object” is all props passed to it.

That means we will never get reported any type-errors of other props passed to MotionSurface here. For example, if I change overflow="auto" to overflow="foobar”, there is no error. This makes future refactorings really hard as we rely on the type checker.

The better workaround is to do {...(props as Record<string, unknown>)}. It silences the props you’re spreading, but not all other props.

I have a blogpost on this too: https://tkdodo.eu/blog/beware-the-leaking-any#when-any-is-leaking

TkDodo · 2026-01-29T10:43:19Z

static/app/components/events/breadcrumbs/breadcrumbsDrawer.spec.tsx

+  await screen.findByRole('textbox', {name: 'Search All Breadcrumbs'});
+
+  // Get all complementary elements and find the one with breadcrumb content
+  const drawers = screen.getAllByRole('complementary');


can we not label the complementary by some header content? Like for example, the BreadCrumbsDrawer has a header with a text:

sentry/static/app/components/events/breadcrumbs/breadcrumbsDrawer.tsx

Line 227 in c857f0f

<Header>{t('Breadcrumbs')}</Header>

so I’d expect to find this drawer with screen.getByRole('complementary', {name: 'Breadcrumbs'}); which also makes the most sense from a user perspective, as that's how they'd see it.

If we don’t to change the API towards <Drawer header={t('Breadcrumbs')}>, we could expose a Breadcrumbs.Header component that does the association

TkDodo · 2026-01-29T10:44:46Z

static/app/views/automations/components/automationBuilderDrawerForm.tsx


  return (
-    <DrawerBody>
+    <DrawerBody data-test-id="create-automation-drawer">


this is used to identify the drawer but why not just use the complementary role that we already have?

TkDodo · 2026-01-29T10:45:33Z

static/app/views/automations/components/editConnectedMonitors.spec.tsx

-    const drawer = await screen.findByRole('complementary', {
-      name: 'Connect Monitors',
-    });
+    const drawer = await screen.findByTestId('connect-monitors-drawer');


this definitely reads like a step backwards. I agree that we shouldn’t use aria-label, but using aria-labelledby pointing to the Header seems doable and worth it.

gggritso

Very cool! I only took a high-level glance (I can look closer if you'd like) and I like the API changes a lot. There's a UI regression in Dashboards I pointed to, though. If there's anything I can help with there, let me know!

gggritso · 2026-01-29T16:26:14Z

static/app/views/dashboards/widgetBuilder/components/widgetBuilderSlideout.tsx

-    <SlideOverPanel
-      position="left"
-      data-test-id="widget-slideout"
-      transitionProps={animationTransitionSettings}


Unfortunately, this causes a UI regression. You can see for yourself:

Open any dashboard

Click "Add Widget"

Click "Create Custom Widget"

You'll see two UI issues:

The widget preview and the Widget Builder fly in at different speeds, which is jarring

The Widget Builder animation is awkward because it over bounces and temporarily shows a black bar on its left side as it flies in and reveals the backdrop underneath

I ran out of time to fix this a few months ago, but I think the panel needs some extra margin to prevent the second UI issue. The first UI issue could be fixed by changing the animation of the preview, but it's also a bit jarring for two huge elements to fly in that fast. I don't have a strong opinion on that, though.

JonasBa · 2026-01-29T17:28:27Z

@TkDodo @gggritso thanks for the great feedback, it's a good test to try and refactor some of these components in our codebase and find where the rough edges are. Motion integration is a sharp edge, that I'm going to try and resolve in a better way, similar thing with the animation, maybe bounce animation is not the right one in this instance, or we need to make the other part of the UI use the same animation then.

I appreciate both of your feedbacks though, it's a good test to see what breaks :)

scttcper · 2026-01-29T17:53:59Z

the drawers on the issue details pages appear on the wrong side of the page. not sure if the other feedback already mentioned that.

JonasBa added 2 commits January 27, 2026 15:14

ref(slideoverpanel) refactor usage

ab1f8f0

ref(slideoverpanel) remove styled entirely

457bde6

JonasBa changed the title ~~ref(slideoverpanel) refactor usage~~ Jan 27, 2026

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jan 27, 2026

ref(slideoverpanel) remove ability to override transition

96c07b9

vercel bot deployed to Preview January 27, 2026 23:53 View deployment

vercel bot deployed to Preview January 27, 2026 23:56 View deployment

JonasBa commented Jan 27, 2026

View reviewed changes

vercel bot deployed to Preview January 28, 2026 00:15 View deployment

ref(slideoverpanel) properly guard responsive values

e50ff26

JonasBa force-pushed the jb/core/overlaypannel branch from 1c6e041 to e50ff26 Compare January 28, 2026 00:16

vercel bot deployed to Preview January 28, 2026 00:19 View deployment

match previous styles

100915a

vercel bot deployed to Preview January 28, 2026 04:09 View deployment

JonasBa commented Jan 28, 2026

View reviewed changes

JonasBa and others added 2 commits January 28, 2026 09:18

ref(slideoverpanel) fix test

4a7bac6

🛠️ apply pre-commit fixes

1768983

vercel bot deployed to Preview January 28, 2026 17:23 View deployment

JonasBa and others added 2 commits January 28, 2026 09:30

fix(tests): Fix TypeScript errors in test files

392fa2b

- Add missing ariaLabel parameter to waitForDrawerToHide calls - Remove unused within imports from test files

🛠️ apply pre-commit fixes

ec133f0

vercel bot deployed to Preview January 28, 2026 17:35 View deployment

fix tests

782763b

vercel bot deployed to Preview January 28, 2026 19:07 View deployment

Merge branch 'master' into jb/core/overlaypannel

27fc82d

vercel bot deployed to Preview January 28, 2026 19:45 View deployment

JonasBa marked this pull request as ready for review January 28, 2026 19:47

JonasBa requested a review from a team as a code owner January 28, 2026 19:47

JonasBa requested review from a team as code owners January 28, 2026 19:48

sentry bot reviewed Jan 28, 2026

View reviewed changes

static/app/components/globalDrawer/components.tsx Show resolved Hide resolved

cursor bot reviewed Jan 28, 2026

View reviewed changes

JonasBa requested review from a team as code owners January 28, 2026 20:08

vercel bot deployed to Preview January 28, 2026 20:10 View deployment

TkDodo reviewed Jan 29, 2026

View reviewed changes

gggritso reviewed Jan 29, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ref(slideoverpanel) refactor using core primitives #107127

ref(slideoverpanel) refactor using core primitives #107127

JonasBa commented Jan 27, 2026 •

edited

Loading

JonasBa commented Jan 27, 2026

cursor bot commented Jan 27, 2026 •

edited

Loading

JonasBa commented Jan 27, 2026

JonasBa Jan 27, 2026

JonasBa Jan 28, 2026

TkDodo Jan 29, 2026

Uh oh!

cursor bot left a comment

cursor bot Jan 28, 2026

TkDodo Jan 29, 2026

TkDodo Jan 29, 2026

TkDodo Jan 29, 2026

TkDodo Jan 29, 2026

TkDodo Jan 29, 2026

gggritso left a comment

gggritso Jan 29, 2026

JonasBa commented Jan 29, 2026

scttcper commented Jan 29, 2026

Labels

5 participants

		aria-hidden={false}
		aria-label={ariaLabel ?? 'slide out drawer'}

Uh oh!

ref(slideoverpanel) refactor using core primitives #107127

Are you sure you want to change the base?

ref(slideoverpanel) refactor using core primitives #107127

Conversation

JonasBa commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JonasBa commented Jan 27, 2026

cursor bot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: SlideOverPanel Refactor using Core Primitives

Overview

Strengths ✅

Concerns & Questions 🤔

Minor Suggestions 💡

Testing Recommendations 🧪

Overall Assessment 🎯

JonasBa commented Jan 27, 2026

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

cursor bot Jan 28, 2026

Choose a reason for hiding this comment

Unreachable fallback code in left placement width logic

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gggritso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonasBa commented Jan 29, 2026

scttcper commented Jan 29, 2026

Labels

5 participants

JonasBa commented Jan 27, 2026 •

edited

Loading

cursor bot commented Jan 27, 2026 •

edited

Loading