core: add Stack and Separator component #97052
Conversation
core: add separator component core: add core separator core: add separator
github-actions
bot
added
the
Scope: Frontend
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| query: window.matchMedia(`(min-width: ${theme.breakpoints[bp]})`), | ||
| }); |
There was a problem hiding this comment.
Potential bug: The `useResponsivePropValue` hook accesses `window.matchMedia` without a safety check, which will cause a crash during server-side rendering (SSR).
- Description: The
useResponsivePropValuehook directly callswindow.matchMediaduring component initialization. In a server-side rendering (SSR) environment, where thewindowobject is not available, this will throw aReferenceError: window is not defined. This error will cause the server rendering process to crash whenever a component using this hook, such asStack, is rendered. Other parts of the codebase already use safe access patterns liketypeof window !== 'undefined'or optional chaining (window.matchMedia?.()), which this new code fails to implement. - Suggested fix: Add a check to ensure
windowis defined before callingwindow.matchMedia. Use optional chaining (window.matchMedia?.()) or a condition liketypeof window !== 'undefined'. The hook should return a default orundefinedvalue during server-side rendering to prevent the crash.
severity: 0.9, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #97052 +/- ##
=======================================
Coverage 80.67% 80.67%
=======================================
Files 8549 8549
Lines 376208 376208
Branches 24443 24443
=======================================
Hits 303503 303503
Misses 72333 72333
Partials 372 372 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Responsive Prop Handling Mismatch
The getOrientationFromDirection function has a type mismatch: it expects the raw StackLayoutProps['direction'] (which can include responsive objects), but it's called with the resolved string value from useResponsivePropValue. Additionally, useResponsivePropValue can return undefined if all responsive breakpoints are undefined (e.g., direction={{sm: undefined}}). Passing this undefined value to getOrientationFromDirection results in a TypeError with the message "No Stack Direction was provided".
static/app/components/core/layout/stack.tsx#L153-L196
static/app/components/core/layout/stack.tsx#L40-L42
sentry/static/app/components/core/layout/stack.tsx
Lines 40 to 42 in 2f51328
Adds a Stack and Separator component. The Stack.Separator is essentially just a wrapper around the Separator that omits the orientation prop. We want to do this to avoid possible user error where they would end up displaying a horizontal separator in a horizontal row. The orientation management is handled via OrientationContext from inside the Stack Component. An alternative could have been to use cloneElement with a bit of detection for the Separator element and provide the prop, but I think context is fine given that the context consumer is scoped to the Stack.Separator component.
Adds a Stack and Separator component.
The Stack.Separator is essentially just a wrapper around the Separator that omits the orientation prop. We want to do this to avoid possible user error where they would end up displaying a horizontal separator in a horizontal row.
The orientation management is handled via OrientationContext from inside the Stack Component. An alternative could have been to use cloneElement with a bit of detection for the Separator element and provide the prop, but I think context is fine given that the context consumer is scoped to the Stack.Separator component.