fix: handle promise rejection when toggling fullscreen#2091
Open
cpvalente wants to merge 3 commits into
Open
Conversation
The toggle function from Mantine's useFullscreen hook is async and can
reject when requestFullscreen() fails (eg. due to browser security
policy, extensions blocking fullscreen, or similar restrictions).
Using it directly as onClick={toggle} left the rejected Promise
unhandled, causing a browser 'Uncaught (in promise) DOMException' error.
The fix wraps the toggle call in a try/catch inside a dedicated
handleToggleFullscreen callback. The navigation menu is also closed
before the fullscreen request is made, which avoids any potential
focus-management interference from the open dialog and gives better UX.
https://claude.ai/code/session_01HQ9UzKrKDR3h9JuSUYzr4b
Mantine's useFullscreen was only used in one place, and its toggle() returns an async Promise that leaks into callers. The new useFullscreen hook in common/hooks: - Exposes isSupported so the call site has a single source of truth (removes the separate supportsFullscreen constant from externals.ts) - toggle() is a plain void callback — errors from requestFullscreen / exitFullscreen are caught internally, so it is always safe to use directly as an onClick handler without any wrapping - toggle() is a no-op when isSupported is false - Targets document.documentElement only (the ref-based API from Mantine was never used in this project) - No vendor-prefix shims — all supported browsers implement the standard Fullscreen API https://claude.ai/code/session_01HQ9UzKrKDR3h9JuSUYzr4b
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The toggle function from Mantine's useFullscreen hook is async and can
reject when requestFullscreen() fails (eg. due to browser security
policy, extensions blocking fullscreen, or similar restrictions).
Using it directly as onClick={toggle} left the rejected Promise
unhandled, causing a browser 'Uncaught (in promise) DOMException' error.
The fix wraps the toggle call in a try/catch inside a dedicated
handleToggleFullscreen callback. The navigation menu is also closed
before the fullscreen request is made, which avoids any potential
focus-management interference from the open dialog and gives better UX.
https://claude.ai/code/session_01HQ9UzKrKDR3h9JuSUYzr4b