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.

[KhushamBansal]

@Salmaan-M Any updates??

[Salmaan-M]

@Salmaan-M Any updates??

I've completed the implementation changes in Sistent based on the latest review feedback. The fullscreen state is now initialized through the explicit openFullscreenOnStart prop rather than continuously relying on the inherited fullScreen prop, which avoids the internal/external state override that was causing the fullscreen toggle to stop working.

I'm currently validating the change in Meshery. While testing, I ran into a local development environment issue related to the linked Sistent package and Turbopack/module resolution, so I'm resolving that before I can complete end-to-end verification in the Registry modal.

I'll post another update once I've finished validation and can confirm the behavior in Meshery. @KhushamBansal

[Salmaan-M]

Implemented the openFullscreenOnStart approach discussed earlier.

The modal now initializes its internal fullscreen state from an explicit openFullscreenOnStart prop while keeping fullscreen ownership within the component. This preserves the ability to start a modal in fullscreen mode while preventing external fullScreen / fullWidth props from continuously overriding the internal toggle state after interaction.

I've updated the implementation and pushed the changes for review. @banana-three-join

[banana-three-join]

Why the introduction of openFullscreenOnStart, now fullscreen shouldn't be utilized anymore so const { fullScreen: _ignoredFullScreen, fullWidth: _ignoredFullWidth, ...restProps } = props; serves as a guardrail for users still utilizing fullscreen.

The problem is that there's a lot of components which are currently utilizing this configuration making it a breaking change due to builds not being able to proceed due to TS errors.

Honestly the more I think about it, we might just be better off by just do the de-constructing, renaming the fullscreen prop to initialFullscreen pass it as a default value to the fullScreen state, and just pass the rest of the props to the StyledDialog because while small in paper, this change would require an update in every single component importing this component.

Or maybe someone has another idea so we don't go with a ducktape type fix.

[Salmaan-M]

That's a fair point. My reasoning for introducing openFullscreenOnStart was to separate the "initial fullscreen state" from the internally managed fullscreen state and make that behavior explicit.

However, I agree that introducing a new prop increases migration cost and would require updating existing consumers that currently rely on fullScreen.

Using the existing fullScreen prop only as the initial value for the internal state, while still de-structuring it before passing props to StyledDialog, seems like a cleaner non-breaking approach. That would preserve the current API while preventing external prop updates from continuously overriding the internal toggle state.

I'm happy to update the implementation in that direction if that's the preferred API. @banana-three-join