Skip to content

ref(layout) use layout.main#112651

Open
JonasBa wants to merge 2 commits intomasterfrom
jb/pageframe/layout-main
Open

ref(layout) use layout.main#112651
JonasBa wants to merge 2 commits intomasterfrom
jb/pageframe/layout-main

Conversation

@JonasBa
Copy link
Copy Markdown
Member

@JonasBa JonasBa commented Apr 10, 2026

Layout.Page is already defined at the root most OrgLayout, re-using it here causes main > main DOM structure and with page-frame feature enabled, causes double border on logs and some errors page.

@JonasBa JonasBa requested a review from a team April 10, 2026 06:08
@JonasBa JonasBa requested a review from a team as a code owner April 10, 2026 06:08
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 10, 2026
}

return (
<ConstrainedPage
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing flex growth causes constrained page to collapse

High Severity

Switching from Layout.Page to Layout.Main drops the flex="1" that Layout.Page provides via its Stack wrapper. Layout.Main renders a plain Container with no flex-grow behavior. In the constrained path, ConstrainedMain applies contain: size (which makes the element's intrinsic size zero) and minHeight="0". Without flex="1" to grow within the parent flex container (Layout.Page), this element collapses to zero height, making the logs page content invisible. The non-constrained path is also affected — the content won't fill remaining vertical space — though at least children remain visible. A flex="1" prop needs to be passed to Layout.Main in both paths.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 46689a5. Configure here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a valid feedback

Image

but other than that, it looks good!

Comment thread static/app/views/explore/components/viewportConstrainedPage.tsx Outdated
Make the errors page main wrapper and viewport-constrained logs wrapper explicit flex columns so explore body sections can grow to fill the remaining page height.

This preserves Layout.Main's existing behavior for the rest of the app while fixing the logs and errors pages that depend on flex-based body sizing.

Co-Authored-By: Codex <codex@openai.com>
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a 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 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6b4b5bb. Configure here.

display: flex;
flex-direction: column;
min-height: 0;
`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated styled component across two files

Low Severity

ErrorsPageMain is an exact duplicate of FlexMain from viewportConstrainedPage.tsx — same base component (Layout.Main), same CSS (display: flex; flex-direction: column; min-height: 0). The errors page could reuse FlexMain (if exported) or use ViewportConstrainedPage with constrained={false} instead of duplicating the styled component.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6b4b5bb. Configure here.

Comment on lines +42 to +46
const FlexMain = styled(Layout.Main)`
display: flex;
flex-direction: column;
min-height: 0;
`;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn’t we use a Flex with render prop instead of custom styled ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should add a lint rule just to prevent myself from doing this at this point

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

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants