fix: modals being stuck opaque on firefox by jog1t · Pull Request #4344 · rivet-dev/rivet
PR Review: fix/firefox-modals
This PR fixes Firefox-specific issues with modal animations by replacing Tailwind animation utilities with explicit CSS keyframes in a CSS module, and removes some search param cleanup from modal close handlers.
dialog.module.css + dialog.tsx — Animation Refactor
Positive: The approach of using explicit CSS keyframe animations is more reliable across browsers, and correctly uses Radix UI's data-state attribute to trigger exit animations before unmount. Using fill-mode: both (via the shorthand) is actually more thorough than the old fill-mode: forwards.
Observation — CSS Nesting: The module uses native CSS nesting (&[data-state="closed"]), which is supported by Firefox 117+ and all modern browsers. This should be fine given the target audience, but it's worth verifying the CSS build pipeline handles it if a transpilation step exists.
Observation — duration-200 leftover: DialogContent still has the duration-200 Tailwind class, which sets transition-duration: 200ms but the animation duration is now controlled entirely by the CSS module (0.2s). This is harmless but slightly redundant.
Missing — prefers-reduced-motion: The CSS module doesn't include a @media (prefers-reduced-motion: reduce) block to disable animations for users who prefer reduced motion. The old Tailwind utilities may have benefited from any global reduced-motion handling. Consider adding:
@media (prefers-reduced-motion: reduce) { .overlay, .content { animation: none; } }
(Noting that other UI components in this codebase also lack this, so it may be a pre-existing gap rather than a regression.)
_cloud.tsx — Search Param Cleanup Removal
This is the part that needs more explanation. The PR removes clearing of organization, dc, config, and displayName search params when their respective modals close, while still clearing the modal param.
Concern — Stale URL params: After closing a modal, the related params will now persist in the URL. For example, after closing the "create-project" modal, ?modal=undefined&organization=xyz remains in the URL. If a user copies the URL, shares it, or navigates back, these stale params could produce unexpected behavior depending on how other parts of the app interpret them.
Question: What specifically was the Firefox bug these removals fix? The PR description doesn't explain this. If the root cause was that the navigate() call in onOpenChange was interfering with the Radix UI exit animation (triggering a re-render mid-animation), a potentially cleaner fix would be to delay the navigation until after the animation completes, rather than leaving stale params in the URL.
General
- The PR description is empty and all checklist items are unchecked — please fill out the description explaining the specific Firefox behavior being fixed and how it was verified.
- No tests are included, which is understandable for a CSS/animation fix, but some manual verification notes would help reviewers understand the fix was confirmed in Firefox.
- Other components with similar Tailwind animation patterns (
sheet.tsx,popover.tsx,tooltip.tsx) still use the same approach. If the root cause is a Tailwind/Firefox incompatibility, those may need similar treatment.