Skip to content

Fix action menu modals rendering inside dropdown containers#16478

Merged
charlesBochet merged 4 commits intomainfrom
fix/action-menu-modals-fullscreen-rendering
Dec 15, 2025
Merged

Fix action menu modals rendering inside dropdown containers#16478
charlesBochet merged 4 commits intomainfrom
fix/action-menu-modals-fullscreen-rendering

Conversation

@abdulrahmancodes
Copy link
Copy Markdown
Contributor

@abdulrahmancodes abdulrahmancodes commented Dec 10, 2025

Closes #16363

  • Updated modals to open in the full screen by default
Screenshot 2025-12-11 at 12 15 15 AM

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Comment on lines 233 to 236
const effectiveContainer = ignoreContainer
? document.body || null
: container;
const isInContainer = isDefined(effectiveContainer);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The modal backdrop incorrectly receives position: absolute instead of position: fixed for full-screen modals because isInContainer is miscalculated when effectiveContainer defaults to document.body.
Severity: HIGH | Confidence: High

🔍 Detailed Analysis

The change to default ignoreContainer to true and set effectiveContainer to document.body introduces a logical bug. The variable isInContainer is now always true for full-screen modals, as it checks isDefined(effectiveContainer). This incorrectly applies position: absolute to the modal backdrop instead of the intended position: fixed. Consequently, the backdrop may not cover the entire viewport when the page is scrolled and can lead to z-index and layering issues, contradicting the goal of making modals full-screen by default.

💡 Suggested Fix

The calculation for isInContainer should be updated. It should not be based on whether effectiveContainer is defined. Instead, it should reflect whether the modal is meant to be constrained within a specific parent element, which is the case when the container prop is passed and ignoreContainer is false.

🤖 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:
packages/twenty-front/src/modules/ui/layout/modal/components/Modal.tsx#L233-L236

Potential issue: The change to default `ignoreContainer` to `true` and set
`effectiveContainer` to `document.body` introduces a logical bug. The variable
`isInContainer` is now always `true` for full-screen modals, as it checks
`isDefined(effectiveContainer)`. This incorrectly applies `position: absolute` to the
modal backdrop instead of the intended `position: fixed`. Consequently, the backdrop may
not cover the entire viewport when the page is scrolled and can lead to z-index and
layering issues, contradicting the goal of making modals full-screen by default.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 6894383

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

@charlesBochet charlesBochet merged commit 31467f2 into main Dec 15, 2025
68 checks passed
@charlesBochet charlesBochet deleted the fix/action-menu-modals-fullscreen-rendering branch December 15, 2025 17:36
@twenty-eng-sync
Copy link
Copy Markdown

Hey @charlesBochet! After you've done the QA of your Pull Request, you can mark it as done here. Thank you!

@github-actions
Copy link
Copy Markdown
Contributor

Thanks @abdulrahmancodes for your contribution!
This marks your 96th PR on the repo. You're top 1% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

NotYen pushed a commit to NotYen/twenty-ym that referenced this pull request Dec 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Confirmation modal of restore records is not displayed correctly

2 participants