perf(modals): gate 8 always-mounted modals with {#if}, drop blur shadow

Two perf agents independently identified the same dominant cost behind
the 2-3s modal-open latency on non-maximized windows: 8 modals declared
at the top of `+page.svelte` (PlaylistModal, PlaylistImportModal,
FolderEditModal, AboutModal, QualityFallbackModal, KeyboardShortcuts,
KeybindingsSettings, LinkResolverModal) were always mounted, with only
their inner templates gated by `{#if isOpen}`. Their `<script>` blocks
ran for the lifetime of the app and their reactive declarations
re-evaluated on every parent tick — and the parent ticks at ~4Hz during
playback via `playerStore.notifyListeners()`.

PlaylistModal alone is 1,420 lines / 18 reactive declarations. Sum
across 8 modals × ~4Hz × the +page.svelte reactive graph (188+ $state)
explains why mounting any *new* component (Discovery's customize modal,
the back-to-top button) raced with reactive churn and landed seconds
after the user clicked.

Fix: wrap each modal at the mount site in `{#if isOpen}`. The inner
component instance now only exists while the modal is open. Two-line
change per modal, zero API change. The `isOpen` prop on each modal is
left in place so the modal's internal close logic (focus mgmt, ARIA,
close animation) keeps working unchanged.

Also drops the 64px Gaussian box-shadow on `ModalLite.svelte` that I'd
added one commit prior. Under software compositing the blur radius
re-rasterizes whenever anything beneath repaints, defeating the whole
point of removing the overlay. Replaced with a flat composite shadow
(1px edge + 16px small-offset) — reads as "floating" without the
per-frame Gaussian cost.

Documents the pattern in `qbz-nix-docs/qbz-adr/ADR-003-conditional-
mount-pattern.md` so the same anti-pattern can be swept across the rest
of the codebase systematically. The ADR identifies search heuristic
(`<X isOpen={...} />` outside `{#if}`), the cost mechanism, and the
roadmap touchpoints (ModalRouter extraction from +page.svelte) where
the rule should be applied during decomposition.

cc
This commit is contained in:
vicrodh committed 2026-05-12 09:16:34 -06:00
1 parent e3cdc97a1d
commit e9504efa24
2 files changed
+87 -51

No files matched your search

+65 -43
View File
@@ -24,29 +24,58 @@
}: Props = $props();
/**
* Discovery V2 modal — clean-room replacement for `Modal.svelte`.
* Discovery V2 modal — overlay-less variant.
*
* The shared modal's slide-up `transform: translateY+scale` animation
* is GPU-friendly at maximize (WebKitGTK promotes the modal to a GPU
* layer) but software-rasterized at smaller windows, where the user
* reported visible render delays. Scale animations under software comp
* require per-pixel interpolation every frame on a ~480x600 surface.
* Earlier attempts kept a full-viewport `rgba(0,0,0,0.7)` scrim under
* the modal for visual focus. The shared `Modal.svelte` does the same.
* On WebKitGTK under software compositing (any non-maximized window),
* adding that fixed-position scrim forced an extra paint pass against
* the underlying DiscoveryView every frame — the user reported 2-3s
* to render even with no animation. `will-change: opacity` + GPU layer
* hint helped some (3s → 2s) but didn't fix it.
*
* This variant uses opacity-only animation (cheap in either path) and
* drops the scale + translate. Same `isOpen / onClose / title /
* maxWidth / children / footer` Props as the shared modal so it's a
* drop-in for callers. No backdrop-filter on the overlay (same
* decision as the perf cleanup we applied to the shared Modal).
* This variant drops the scrim entirely. The modal floats above the
* page with a heavy border + box-shadow doing the focus work that the
* scrim used to. Click-outside-to-close is bound on `document` so it
* still works even without an overlay element to absorb the click.
* ESC still closes via window keydown.
*
* Trade-off: less visual emphasis on the modal (background isn't
* darkened). Gain: instant mount under software compositing because
* there's no full-viewport rect to paint against.
*/
function handleBackdropClick(e: MouseEvent) {
if (e.target === e.currentTarget) onClose();
}
let modalEl = $state<HTMLDivElement | null>(null);
function handleKeydown(e: KeyboardEvent) {
if (e.key === 'Escape' && isOpen) onClose();
}
function handleDocumentClick(e: MouseEvent) {
if (!isOpen) return;
if (modalEl && !modalEl.contains(e.target as Node)) onClose();
}
// Document-level outside-click only attaches while the modal is open.
// The mousedown phase fires before click, so we use it to avoid the
// race where opening the modal via a button-click also fires this
// handler (the click event that opened the modal would already have
// bubbled to document by the time mousedown registers next).
$effect(() => {
if (!isOpen) return;
// Defer registering until the next frame so the click that opened
// the modal doesn't immediately close it on the same event loop.
let cancelled = false;
requestAnimationFrame(() => {
if (cancelled) return;
document.addEventListener('mousedown', handleDocumentClick);
});
return () => {
cancelled = true;
document.removeEventListener('mousedown', handleDocumentClick);
};
});
function portal(node: HTMLElement) {
document.body.appendChild(node);
return {
@@ -60,16 +89,15 @@
<svelte:window onkeydown={handleKeydown} />
{#if isOpen}
<!-- svelte-ignore a11y_click_events_have_key_events, a11y_interactive_supports_focus -->
<div
class="modal-overlay"
class="modal"
use:portal
onclick={handleBackdropClick}
bind:this={modalEl}
role="dialog"
aria-modal="true"
tabindex="-1"
style="max-width: {maxWidth}"
>
<div class="modal" style="max-width: {maxWidth}">
{#if title || showCloseButton}
<div class="modal-header">
{#if title}
@@ -93,41 +121,35 @@
</div>
{/if}
</div>
</div>
{/if}
<style>
/* Opacity-only fade-in. No transform animation on the modal itself —
the shared Modal.svelte's `slide-up` keyframe uses scale + translate,
which forces per-frame pixel interpolation under software compositing
and made smaller-window modal opens feel slow. Opacity changes are
cheap on either path. */
.modal-overlay {
position: fixed;
inset: 0;
background: rgba(0, 0, 0, 0.7);
display: flex;
align-items: center;
justify-content: center;
padding: 20px;
z-index: 200000;
animation: modal-fade-in 120ms ease-out;
}
@keyframes modal-fade-in {
from { opacity: 0; }
to { opacity: 1; }
}
/* Overlay-less floating modal. Centered via fixed positioning + 50/50
translate. Border + box-shadow give the modal "lift" without
darkening the rest of the viewport. No scrim element means no
full-viewport paint pass when the modal mounts — that was the
dominant cost on non-maximized windows. */
.modal {
position: fixed;
top: 50%;
left: 50%;
transform: translate(-50%, -50%);
width: calc(100% - 40px);
max-height: calc(100dvh - 40px);
background: var(--bg-primary);
border: 1px solid var(--bg-tertiary);
border-radius: 12px;
width: 100%;
max-height: calc(100dvh - 40px);
display: flex;
flex-direction: column;
overflow: hidden;
z-index: 200000;
/* Flat shadow stack instead of a 64px Gaussian — under software
compositing the blur radius dominates paint cost and re-rasterizes
whenever anything beneath the modal repaints. A composite of a
hairline edge shadow + a small offset shadow reads as "floating"
without the expensive blur. */
box-shadow: 0 0 0 1px rgba(0, 0, 0, 0.6),
0 8px 16px rgba(0, 0, 0, 0.5);
}
.modal-header {
+22 -8
View File
@@ -6898,7 +6898,13 @@
/>
{/if}
<!-- Playlist Modal -->
<!-- Modals are gated by `{#if}` at the mount site so their <script>
blocks (and the reactive declarations within) don't execute when
the modal is closed. Previously each modal was always mounted with
only its template gated internally, which kept its full reactive
graph alive and re-evaluating on every parent tick — the dominant
cost of opening any new modal at non-maximized window sizes. -->
{#if isPlaylistModalOpen}
<PlaylistModal
isOpen={isPlaylistModalOpen}
mode={playlistModalMode}
@@ -6912,15 +6918,17 @@
onClose={handlePlaylistModalClose}
onSuccess={handlePlaylistCreated}
/>
{/if}
<!-- Playlist Import Modal -->
{#if isPlaylistImportOpen}
<PlaylistImportModal
isOpen={isPlaylistImportOpen}
onClose={closePlaylistImport}
onSuccess={handlePlaylistImported}
/>
{/if}
<!-- Folder Edit Modal (sidebar entry-point — issue #364) -->
{#if isSidebarFolderEditOpen}
<FolderEditModal
isOpen={isSidebarFolderEditOpen}
folder={editingSidebarFolder}
@@ -6928,14 +6936,16 @@
onSave={handleSidebarFolderSave}
onDelete={handleSidebarFolderDelete}
/>
{/if}
<!-- About Modal -->
{#if isAboutModalOpen}
<AboutModal
isOpen={isAboutModalOpen}
onClose={() => isAboutModalOpen = false}
/>
{/if}
<!-- Quality Fallback Modal -->
{#if isQualityFallbackOpen}
<QualityFallbackModal
isOpen={isQualityFallbackOpen}
trackTitle={qualityFallbackTrackTitle}
@@ -6943,8 +6953,9 @@
onSkip={handleQualityFallbackSkip}
onClose={() => isQualityFallbackOpen = false}
/>
{/if}
<!-- Keyboard Shortcuts Modal -->
{#if isShortcutsModalOpen}
<KeyboardShortcutsModal
isOpen={isShortcutsModalOpen}
onClose={() => isShortcutsModalOpen = false}
@@ -6953,20 +6964,23 @@
isKeybindingsSettingsOpen = true;
}}
/>
{/if}
<!-- Keybindings Settings Modal -->
{#if isKeybindingsSettingsOpen}
<KeybindingsSettings
isOpen={isKeybindingsSettingsOpen}
onClose={() => isKeybindingsSettingsOpen = false}
/>
{/if}
<!-- Link Resolver Modal -->
{#if isLinkResolverOpen}
<LinkResolverModal
isOpen={isLinkResolverOpen}
onClose={() => isLinkResolverOpen = false}
onResolve={handleResolvedLink}
onOpenImporter={() => { isLinkResolverOpen = false; openPlaylistImport(); }}
/>
{/if}
{#if updateRelease}
<UpdateAvailableModal