-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
ref(container) run codemod on orphaned components #106846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| // Evenly distributing the cards into columns. | ||
| const columnSize = Math.ceil(cards.length / columnCount); | ||
| for (let i = 0; i < cards.length; i += columnSize) { | ||
| columns.push(<CardColumn key={i}>{cards.slice(i, i + columnSize)}</CardColumn>); | ||
| columns.push( | ||
| <Container column="span 1" key={i}> | ||
| {cards.slice(i, i + columnSize)} | ||
| </Container> | ||
| ); | ||
| } | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: A local function named Container conflicts with an imported component of the same name. This causes the function to incorrectly render the imported component instead of its intended content.
Severity: HIGH
Suggested Fix
Rename the local Container function to avoid the naming collision with the imported component from @sentry/scraps/layout. For example, renaming it to KeyValueDataContainer and updating its export and usage within the file will resolve the shadowing issue.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/components/keyValueData/index.tsx#L209-L219
Potential issue: In `keyValueData/index.tsx`, a local function `Container` is defined,
but there is also an `import {Container}` from `@sentry/scraps/layout` at the module
level. Inside the local `Container` function, it attempts to render a `<Container>`
component. Due to JavaScript's scoping rules, this resolves to the imported `Container`
component from `@sentry/scraps/layout`, not the local function itself. This leads to
incorrect rendering behavior, as the component effectively calls the imported layout
component with props it may not be designed for, breaking the intended layout logic
wherever `KeyValueData.Container` is used.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 6 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| })``; | ||
| export function ScoreWrapper(props: ContainerProps<'div'>) { | ||
| return <Container {...props} />; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScoreWrapper missing flex layout properties
High Severity
The ScoreWrapper component was converted from a Flex component with align="baseline" and maxWidth="100%" to a plain Container without these properties. The child Score component uses flex-shrink: 1, which requires a flex container parent to work correctly. This breaks the intended flex layout and alignment.
| ) : ( | ||
| // Placeholder to maintain layout when no version is selected | ||
| <PlaceholderLink aria-hidden="true" /> | ||
| <Container as="span" minHeight="1.2em" aria-hidden="true" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing display property on span with minHeight
Medium Severity
The placeholder element was changed from a styled span with display: inline-block and min-height: 1.2em to <Container as="span" minHeight="1.2em"> without the display: inline-block. The minHeight property has no effect on inline elements (the default for span), so the layout placeholder won't maintain the intended minimum height.
| details: ( | ||
| <Container as="span" width="20rem"> | ||
| {r.desc} | ||
| </Container> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing display property on span with width
Medium Severity
The role description container was changed from a styled span with display: inline-block and width: 20rem to <Container as="span" width="20rem"> without the display: inline-block. The width property has no effect on inline elements (the default for span), so the role descriptions won't have the intended fixed width constraint.
| </InputGroup.TrailingItems> | ||
| </InputGroup> | ||
| </SearchWrapper> | ||
| </Container> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing container query responsive display property
Medium Severity
The search input container lost its container query that hides it at small widths. The original SearchWrapper had @container (max-width: 500px) { display: none; } to responsively hide the search at narrow container widths, but this was removed when converting to Container. The search input will now always be visible regardless of container width.
| <StyledPlatformIcon platform={projectPlatforms[0]!} size={18} /> | ||
| <BorderOverlay /> | ||
| </IconContainer> | ||
| </Container> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing display grid on icon container
Medium Severity
The icon container lost display: grid when converted to Container. The original IconContainer used grid layout to position the platform icons, but this property wasn't carried over. The child PlatformIconWrapper components use absolute positioning and expect a grid context, so the icon layout will be broken.
Additional Locations (1)
| <Container margin="lg 0" data-separator> | ||
| <Separator /> | ||
| </SeparatorWrapper> | ||
| </Container> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing default display none on separator
Medium Severity
The separator container lost its default display: none property when converted to Container. The original SeparatorWrapper was hidden by default and conditionally shown via a parent selector. Without the default hidden state, separators will always be visible instead of only appearing between navigation sections.
Applies Container codemod to Components that do not have any codeowners