fix(modal): prevent fullscreen prop override

[Salmaan-M]

Notes for Reviewers

Description

This PR fixes an issue where the Modal component's internal fullscreen state could be overridden by external props.

Previously:

allowed consumers passing fullScreen or fullWidth props to override the internal fullscreen toggle state managed by the component itself.

This caused fullscreen toggle behavior to fail in downstream consumers such as Meshery's Registry modal.

Changes Made

Filtered conflicting fullScreen and fullWidth props from external props
Preserved external overrides for unrelated props
Kept internal fullscreen state authoritative

Testing

Verified fullscreen toggle works correctly locally
Verified dialog now receives MuiDialog-paperFullScreen
Confirmed no unintended changes to unrelated props

Signed commits

This PR fixes #1617

Signed commits

• Yes, I signed my commits.

[gemini-code-assist]

Code Review

This pull request updates the Modal component to destructure props and exclude fullScreen and fullWidth into restProps, which is now spread at the beginning of StyledDialog to prevent overriding internal properties. The reviewer suggested formatting the destructuring assignment on a single line to fix inconsistent indentation and improve readability.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[saurabhraghuvanshii]

@Salmaan-M, any type of Loom recording demonstrating your changes would help us understand what you fixed and what the current behavior is.

[Salmaan-M]

lv_0_20260608225642.mp4

This video demonstrates how prop reordering helps in fixing the [UI] Full screen issue @saurabhraghuvanshii

[saurabhraghuvanshii]

Thanks @Salmaan-M lgtm!!

[saurabhraghuvanshii]

learned new thing today from you.

[Salmaan-M]

learned new thing today from you.

what is it? 🙂 @saurabhraghuvanshii

[saurabhraghuvanshii]

learned new thing today from you.

what is it? 🙂 @saurabhraghuvanshii

secret!!

[Salmaan-M]

learned new thing today from you.

what is it? 🙂 @saurabhraghuvanshii

secret!!

Come on yaarr? make me happy

[banana-three-join]

What happens if you don't ignore the fullwidth? Can you still open and close the modal? Because I believe the prop that is overriding the internal fullscreen serves as the initial state for whether the fullscreen should be open or closed on the first interation and then, the fullscreen state is handled by the component itself. This means that by ignoring the initial fullscreen prop, we might have a modal to start on fullscreen by default but since we are ignoring it, we can never have that initial state. (Which if that's the case, it's not stated in the prop type for the modal anyways so that would also need to be fixed)

[Salmaan-M]

That's a very good point.

During debugging, what I observed was that the issue occurred because external fullScreen={false} continued overriding the internal fullscreen toggle state even after interaction.

My intention with filtering fullScreen/fullWidth was to prevent conflicting state ownership once the component starts managing fullscreen internally via:

const [fullScreen, setFullScreen] = useState(false);

However, you're right that this also removes the possibility of consumers intentionally providing an initial fullscreen state through props.

I agree that there are really two separate concerns here:

1. Initial fullscreen state
2. Ongoing fullscreen control after internal toggle state takes over

If supporting an initial fullscreen state is desirable behavior, then a better approach may be:

• initialize internal state from the incoming prop once,
• but avoid continuously allowing external props to override the internal toggle state after interaction.

Something like:

const [fullScreen, setFullScreen] = useState(props.fullScreen ?? false);

while still preventing later prop precedence conflicts.

I think this approach better preserves the initial-state capability while still addressing the runtime override issue that caused the fullscreen toggle bug. I'll prototype this approach and validate the behavior locally. What's your say @banana-three-join

[banana-three-join]

@Salmaan-M yeah the idea seems about right but maybe the variable should have a more descriptive name like openFullscreenOnStart={boolean} instead of fullscreen

In regards of the desired behavior, that would be my best guest since as I mentioned earlier, the typing of the modal itself lacks a fullscreen prop. I would say that the prop also needs to be added to the typing so it's usage is a little bit clearer since at the end of the day, it has it's own internal state to manage whether the modal is in fullscreen mode or not

[rishiraj38]

@Salmaan-M FYI
https://discuss.layer5.io/t/how-to-test-sistent-components-for-development-environment/7532

[Salmaan-M]

@banana-three-join That makes sense to me.

Using a more explicit prop like openFullscreenOnStart would separate the "initial fullscreen state" concern from the internal fullscreen toggle state much more clearly and avoid overloading the inherited MUI fullScreen prop semantics.

I also agree that the typing/API should make this behavior explicit instead of implicitly inheriting it through DialogProps.

I'll prototype this approach locally and validate the behavior in Meshery as well.

[rishiraj38]

@Salmaan-M, how's the progress on the issue?

[Salmaan-M]

I prototyped an openFullscreenOnStart prop locally. My thinking is that this makes the initialization behavior explicit while keeping fullscreen ownership inside the component. Does this align with the intended API direction before I update the implementation further? @banana-three-join @rishiraj38

[banana-three-join]

Yep, go ahead and make the change @Salmaan-M

[KhushamBansal]

@Salmaan-M Are you working on fixing this issue?

[Salmaan-M]

@KhushamBansal yes, I am working on it.