Skip to content

fix(library): render album/artist + popover through a portal #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
InstaZDLL merged 1 commit into from
May 20, 2026
Merged

fix(library): render album/artist + popover through a portal #77

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 80 additions & 3 deletions src/components/views/LibraryView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
useRef,
useState,
} from "react";
import { createPortal } from "react-dom";
import { useVirtualizer } from "@tanstack/react-virtual";
import {
Music2,
Expand Down Expand Up @@ -1325,14 +1326,30 @@ interface AddToPlaylistPopoverProps {
* "+ to playlist" action is a bulk add with no symmetric remove.
*/
memberPlaylistIds?: ReadonlySet<number>;
/**
* Trigger element the popover anchors to. When provided, the popover
* is rendered through a portal at `document.body` and positioned via
* `getBoundingClientRect`, escaping every ancestor stacking context
* (virtualizer rows use `transform`, which traps `z-index` inside).
* Required for album / artist grids where the popover would otherwise
* paint under the row below it.
*/
anchorEl?: HTMLElement | null;
}

/**
* Tiny popover anchored to the trigger button. Lists every playlist of
* the active profile (resolved color tile + name) plus a "create new"
* shortcut at the bottom. Picking a row calls `onPick(playlistId)`.
*
* Stops `onDoubleClick` from bubbling to the parent `<li>` so clicking a
* When `anchorEl` is supplied, the popover is rendered via React portal
* to `document.body` and positioned absolutely against the anchor's
* client rect. Without it, the popover falls back to absolute positioning
* inside its parent — only safe where the parent isn't sitting inside a
* `transform`-clipped stacking context (TrackTable rows qualify; album /
* artist grids don't).
*
* Stops `onDoubleClick` from bubbling to the parent so clicking a
* playlist doesn't accidentally start playback of the row underneath.
*/
function AddToPlaylistPopover({
Expand All @@ -1341,13 +1358,54 @@ function AddToPlaylistPopover({
onCreate,
t,
memberPlaylistIds,
anchorEl,
}: AddToPlaylistPopoverProps) {
return (
// Portal mode: track the anchor's viewport rect so the popover follows
// it on scroll / resize / virtualization recycling. `null` rect = first
// render before the layout effect runs; we keep the popover invisible
// until we know where it goes so it never flashes at (0,0).
const POPOVER_WIDTH = 224; // matches `w-56`
const [rect, setRect] = useState<DOMRect | null>(null);
useLayoutEffect(() => {
if (!anchorEl) return;
const update = () => setRect(anchorEl.getBoundingClientRect());
update();
const ro = new ResizeObserver(update);
ro.observe(anchorEl);
window.addEventListener("scroll", update, true);
window.addEventListener("resize", update);
return () => {
ro.disconnect();
window.removeEventListener("scroll", update, true);
window.removeEventListener("resize", update);
};
}, [anchorEl]);

const inner = (
<div
data-add-to-playlist-popover
role="menu"
onDoubleClick={(e) => e.stopPropagation()}
className="absolute top-full right-0 mt-1 z-50 w-56 rounded-xl border border-zinc-200 bg-white shadow-lg dark:border-zinc-700 dark:bg-surface-dark-elevated dark:shadow-black/40 overflow-hidden animate-fade-in"
style={
anchorEl
? rect
? {
position: "fixed",
// Anchor the right edge to the trigger's right edge,
// matching the in-flow `right-0` behaviour. Drop 4 px
// below the trigger to match `mt-1`.
top: rect.bottom + 4,
left: rect.right - POPOVER_WIDTH,
width: POPOVER_WIDTH,
}
: { position: "fixed", visibility: "hidden" }
: undefined
}
className={`${
anchorEl
? "z-100"
: "absolute top-full right-0 mt-1 z-50 w-56"
} rounded-xl border border-zinc-200 bg-white shadow-lg dark:border-zinc-700 dark:bg-surface-dark-elevated dark:shadow-black/40 overflow-hidden animate-fade-in`}
>
<div className="text-[10px] font-bold tracking-widest text-zinc-400 uppercase px-3 pt-3 pb-2">
{t("trackActions.addToPlaylist")}
Comment on lines 1385 to 1411
Copy link
Copy Markdown

@qodo-code-review qodo-code-review Bot May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Popover clicks trigger tile 🐞 Bug ≡ Correctness

AddToPlaylistPopover only stops double-click propagation; in portal mode, React synthetic events
still bubble to the originating component tree, so clicking a playlist row can also fire the
album/artist tile onClick. This can navigate/open the album/artist while the user is trying to add
to a playlist.
Agent Prompt 
### Issue description
When the popover is rendered with `createPortal`, React’s event system still bubbles events through the original React tree. Since the popover only stops `onDoubleClick`, normal `click` events from menu items can bubble to the album/artist tile `onClick` handlers and trigger navigation/selection unintentionally.

### Issue Context
- Popover root currently has `onDoubleClick={(e) => e.stopPropagation()}` only.
- Album and artist tiles are clickable (`onClick={() => onAlbumClick(...)}` / `onArtistClick(...)}`).

### Fix Focus Areas
- src/components/views/LibraryView.tsx[1385-1445]

### Implementation notes
- Add `onClick={(e) => e.stopPropagation()}` (and optionally `onMouseDown={(e) => e.stopPropagation()}`) to the popover container (`data-add-to-playlist-popover`).
- Alternatively (or additionally), stop propagation in each playlist row button’s `onClick` before calling `onPick`/`onCreate`.
- Keep the existing outside-click handling working (your `closest([...])` checks will still behave correctly).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Expand Down Expand Up @@ -1409,6 +1467,7 @@ function AddToPlaylistPopover({
</div>
</div>
);
return anchorEl ? createPortal(inner, document.body) : inner;
}

interface AlbumGridProps {
Expand All @@ -1435,6 +1494,10 @@ function AlbumGrid({
"use no memo";
const unknown = t("library.table.unknown");
const [openMenuAlbumId, setOpenMenuAlbumId] = useState<number | null>(null);
// Map album.id → the `+` button DOM node. The popover uses the live
// node to compute its portal position via `getBoundingClientRect`,
// sidestepping every ancestor stacking context.
const triggerRefs = useRef<Map<number, HTMLButtonElement>>(new Map());
const [contextMenu, setContextMenu] = useState<{
albumId: number;
x: number;
Expand Down Expand Up @@ -1576,6 +1639,10 @@ function AlbumGrid({
<button
type="button"
data-add-to-playlist-trigger
ref={(el) => {
if (el) triggerRefs.current.set(album.id, el);
else triggerRefs.current.delete(album.id);
}}
onClick={(e) => {
e.stopPropagation();
setOpenMenuAlbumId(isMenuOpen ? null : album.id);
Expand Down Expand Up @@ -1608,6 +1675,7 @@ function AlbumGrid({
<AddToPlaylistPopover
playlists={playlists}
trackId={album.id}
anchorEl={triggerRefs.current.get(album.id) ?? null}
onPick={(playlistId) => {
onAddToPlaylist(playlistId, album.id);
setOpenMenuAlbumId(null);
Expand Down Expand Up @@ -1719,6 +1787,10 @@ function ArtistList({
}: ArtistListProps) {
"use no memo";
const [openMenuArtistId, setOpenMenuArtistId] = useState<number | null>(null);
// See AlbumGrid: the `+` button DOM nodes feed the popover's portal
// positioning, which is the only way to escape the virtualizer's
// transform-based stacking context.
const triggerRefs = useRef<Map<number, HTMLButtonElement>>(new Map());

// Virtual-grid plumbing — see AlbumGrid for the rationale; same math
// applies to the artist tiles (same `minmax(180px,1fr)` + gap-5).
Expand Down Expand Up @@ -1855,6 +1927,10 @@ function ArtistList({
<button
type="button"
data-add-to-playlist-trigger
ref={(el) => {
if (el) triggerRefs.current.set(artist.id, el);
else triggerRefs.current.delete(artist.id);
}}
onClick={(e) => {
e.stopPropagation();
setOpenMenuArtistId(isMenuOpen ? null : artist.id);
Expand Down Expand Up @@ -1886,6 +1962,7 @@ function ArtistList({
<AddToPlaylistPopover
playlists={playlists}
trackId={artist.id}
anchorEl={triggerRefs.current.get(artist.id) ?? null}
onPick={(playlistId) => {
onAddToPlaylist(playlistId, artist.id);
setOpenMenuArtistId(null);
Expand Down
Loading