feat(ui): the four small gaps — player delete, repeat-move, a real channel link, YT search sorting
Items 2, 4, 5 and 7 of the bug hunt, one round. They are independent, but they are all the same kind of thing: an action that exists somewhere else in the app and is missing where you actually want it. **5 — the channel name in the YT player is a link again.** It was a `<button>`, so middle-click and Ctrl/Cmd-click did nothing on a thing that looks exactly like a link (the YouTube icon beside it IS an anchor and opens a tab). It is now a real `<a>` whose href is the same `?chan=<id>` URL `openChannel` pushes, with a plain left click still taking the in-app path — that path does more than navigate, since the player must close and reclaim its own Back-to-close history entry first. The "is this click ours or the browser's" test is now ONE predicate (`isPlainLeftClick`), which VideoCard's card link also uses, so a second such link cannot forget the middle-click arm. **2 — "Remove" from inside the player.** The parent owns the removal (the SAME `removeItem` the row menu and the inline ✕ use: one confirm, one optimistic update, one undo-stack rebase), and the player owns what happens next, because it is the only side that knows which item is playing — its launch queue is frozen at mount. It drops the item, plays what followed, and closes when the list runs out. That forced a real fix in Playlists: the player's mount was derived from `shown` every render, so anything that dropped the launch row yanked the player off the screen mid-playback. That was a latent edge (change the view and it happens) and the normal case for this feature. The parent now snapshots what it launched with, and only onClose takes the player down. **4 — "Move to <the last list you moved to>".** The submenu is two levels deep and moving a run of videos into the same list is the shape this gets used in. The remembered id is a per-account pref; the row is derived from `moveTargets`, which already excludes the list you are looking at, so it cannot offer a move to where the video already is. Remembered only on a SUCCESSFUL move — a shortcut to a move that failed is a shortcut to failing again. Absent, not disabled, when there is nothing to repeat: there is nothing to explain. **7 — the live-YouTube results can be sorted.** Client-side, over the page already loaded. Not a new `order=` param, and this is the decision worth recording: the owner's case is "even one page of results", i.e. reorder what is on screen; a re-query would REPLACE the results, which is not what "sort these" means; and the zero-quota scrape source has no order parameter at all, so a server-side control would silently do nothing (or quietly cost 100 quota units) depending on which source served the page. Only keys a search hit carries are offered — `subscribers`/`priority` describe a subscription, which a search hit is not, and `shuffle` on a ranked list is noise. Its own pref, so sorting a search page never re-sorts the library behind it. Verified in the browser on localdev against the real account: the channel name renders as `<a href="…&chan=…">` and a ctrl-click / middle-click is left to the browser while a plain click still opens the channel in-app; removing from the player stepped 1/3 → 1/2 onto the next video and closed on the last one; the row menu grew "Move to “…”" only after a move and only on a list that is not the target; and the results sort reordered by views 41.7M → 29.8M → 24.2M → 15.8M. (The pane's own ctrl-click arrives without the modifier — a harness limit, so the modifier cases were driven as real MouseEvents.) Scratch playlists and the touched prefs were cleaned up afterwards. 22 vitest, mutation-checked against 14 mutations (no href at all · href without the rest of the query · every click hijacked in-app · the middle-click arm dropped from the predicate · plain click no longer in-app · always jumping to the top of the list · the removed item left in the queue · no clamp when the last item goes · an empty queue not reported as "nothing left" · sort-then-reverse flipping ties · unknown values first · titles compared numerically · shuffle offered on search results · direction ignored) — each red on exactly the test that names it. Item 4 has no test of its own: its rule is enforced by construction, not by a check.
This commit is contained in:
@@ -28,6 +28,7 @@ import {
|
||||
Check,
|
||||
ExternalLink,
|
||||
FolderInput,
|
||||
History,
|
||||
GripVertical,
|
||||
ListPlus,
|
||||
Pencil,
|
||||
@@ -59,6 +60,7 @@ import { useConfirm } from "./ConfirmProvider";
|
||||
import { usePlaylistActions } from "./usePlaylistActions";
|
||||
import { usePlaylists } from "./PlaylistsProvider";
|
||||
import { useMe } from "../lib/useMe";
|
||||
import { usePref } from "../lib/usePref";
|
||||
import {
|
||||
ColumnHead,
|
||||
alignClass,
|
||||
@@ -214,10 +216,24 @@ export default function Playlists() {
|
||||
// re-confirms our own reorder doesn't wipe the undo history — we reset history only when
|
||||
// the actual set of items changes (different playlist, or an add/remove).
|
||||
const lastSetRef = useRef<string>("");
|
||||
// The open player is tracked by video ID, not by a position: `shown` is derived, so a refetch or
|
||||
// a change of view would otherwise slide a different video under the index — or unmount the player
|
||||
// mid-playback. PlayerModal already tracks its own queue by id; this is the parent half of that.
|
||||
const [playingId, setPlayingId] = useState<string | null>(null);
|
||||
// What the player was LAUNCHED with, snapshotted when it opens: the video, the queue and the index
|
||||
// inside it. `shown` is derived (sort / filter / view), and the player freezes its queue and owns
|
||||
// its own cursor from mount — so deriving the mount from `shown` every render meant anything that
|
||||
// dropped the launch row yanked the player off the screen mid-playback. That was a latent edge
|
||||
// (a view change) until "remove this from the playlist" moved INSIDE the player, where it is the
|
||||
// normal case. Only onClose takes the player down now.
|
||||
const [playing, setPlaying] = useState<{
|
||||
video: Video;
|
||||
queue: Video[];
|
||||
index: number;
|
||||
} | null>(null);
|
||||
// Where the last move went. `LS.playlist` is the SELECTED (source) list and `updated_at` moves on
|
||||
// any edit, so neither answers "where do I keep sending things"; this does, and it is a per-account
|
||||
// preference like every other remembered choice. Null until the first move of this account's life.
|
||||
const [lastMoveTargetId, setLastMoveTargetId] = usePref<number | null>(
|
||||
"lastMovePlaylistId",
|
||||
null,
|
||||
);
|
||||
// ONE context menu for the whole table, not one popover per <tr> (see ui/ContextMenu): the row it
|
||||
// belongs to is `rowMenu.target`, and the panel anchors to the pointer.
|
||||
const rowMenu = useContextMenu<Video>();
|
||||
@@ -273,10 +289,23 @@ export default function Playlists() {
|
||||
const [pushing, setPushing] = useState(false);
|
||||
const [reverting, setReverting] = useState(false);
|
||||
|
||||
const onPlay = useCallback((v: Video) => setPlayingId(v.id), []);
|
||||
const onRemove = useCallback((v: Video) => {
|
||||
removeRef.current(v);
|
||||
// Stable (the memoized row callbacks depend on it), so it reads `shown` through the ref rather
|
||||
// than closing over it.
|
||||
const openPlayer = useCallback((id: string) => {
|
||||
const queue = shownRef.current;
|
||||
const index = queue.findIndex((v) => v.id === id);
|
||||
if (index < 0) return;
|
||||
setPlaying({ video: queue[index]!, queue, index });
|
||||
}, []);
|
||||
const onPlay = useCallback((v: Video) => openPlayer(v.id), [openPlayer]);
|
||||
const onRemove = useCallback((v: Video) => {
|
||||
void removeRef.current(v);
|
||||
}, []);
|
||||
// The player's "remove from this playlist" goes through the SAME removeItem the row menu and the
|
||||
// inline ✕ use — one confirm, one optimistic update, one undo-stack rebase, no second copy to
|
||||
// drift. It answers whether the removal actually happened, because the player has to decide
|
||||
// afterwards whether to step to the next item or close.
|
||||
const removeFromPlayer = useCallback((v: Video) => removeRef.current(v), []);
|
||||
|
||||
const storedIndex = useMemo(() => {
|
||||
const m = new Map(items.map((v, i) => [v.id, i]));
|
||||
@@ -478,15 +507,17 @@ export default function Playlists() {
|
||||
order.set(arrayMove(items, oldIndex, newIndex)); // onApply persists
|
||||
}
|
||||
|
||||
async function removeItem(video: Video) {
|
||||
if (selectedId == null) return;
|
||||
/** @returns whether the item was actually removed — the player needs to know, because it must
|
||||
* step to the next item (or close) only when the removal really happened. */
|
||||
async function removeItem(video: Video): Promise<boolean> {
|
||||
if (selectedId == null) return false;
|
||||
const ok = await confirm({
|
||||
title: t("playlists.removeItem"),
|
||||
message: t("playlists.confirmRemove", { title: video.title }),
|
||||
confirmLabel: t("playlists.removeItem"),
|
||||
danger: true,
|
||||
});
|
||||
if (!ok) return;
|
||||
if (!ok) return false;
|
||||
// A removal isn't itself undoable, but it no longer THROWS AWAY the reorder history: rebasing
|
||||
// drops the item from every snapshot, so the stack stays valid for what's left.
|
||||
const next = items.filter((v) => v.id !== video.id);
|
||||
@@ -495,10 +526,12 @@ export default function Playlists() {
|
||||
try {
|
||||
await api.removeFromPlaylist(selectedId, video.id);
|
||||
refreshAll();
|
||||
return true;
|
||||
} catch {
|
||||
// The optimistic removal above didn't stick — resync from the server to restore the row.
|
||||
// api.req already surfaced the error to the user via the global error dialog.
|
||||
refreshAll();
|
||||
return false;
|
||||
}
|
||||
}
|
||||
// Kept in a ref so the memoized column callbacks don't rebuild on every state change.
|
||||
@@ -516,6 +549,9 @@ export default function Playlists() {
|
||||
// ONE request. The two-call version (add there, remove here) leaves the video in BOTH lists
|
||||
// whenever the second call fails — see the endpoint's docstring.
|
||||
await api.moveToPlaylist(selectedId, video.id, target.id);
|
||||
// Remembered only on SUCCESS: a shortcut that points at a move which failed would be a
|
||||
// shortcut to failing again.
|
||||
setLastMoveTargetId(target.id);
|
||||
notify({
|
||||
level: "success",
|
||||
message: t("playlists.movedToast", { title: video.title, name: plName(target) }),
|
||||
@@ -575,7 +611,7 @@ export default function Playlists() {
|
||||
|
||||
// Stable identity: PlayerModal's keydown/scroll-lock effect depends on it, so a fresh closure
|
||||
// every render re-subscribed those listeners and re-ran focusModal() under the open player.
|
||||
const closePlayer = useCallback(() => setPlayingId(null), []);
|
||||
const closePlayer = useCallback(() => setPlaying(null), []);
|
||||
|
||||
// Name the current view in the user's terms: "Title ↑", "grouped by channel", or both.
|
||||
const sortLabel =
|
||||
@@ -601,11 +637,15 @@ export default function Playlists() {
|
||||
: filterActive
|
||||
? t("playlists.dragBlockedFilter")
|
||||
: t("playlists.dragBlockedSort");
|
||||
const playingIndex = playingId ? shown.findIndex((v) => v.id === playingId) : -1;
|
||||
|
||||
// The row menu, built from the row the user right-clicked. Same item shape (and therefore same
|
||||
// rows, same paint, same keyboard model) as any ⋯ ActionMenu in the app.
|
||||
const moveTargets = playlists.filter((p) => p.id !== selectedId);
|
||||
// A one-click repeat of the last move — the submenu is two levels deep, and moving a run of videos
|
||||
// into the same list is the shape this actually gets used in. Absent (not disabled) when there is
|
||||
// nothing to repeat: before the first move, or when the remembered list IS the one you are looking
|
||||
// at, which is not a move at all. Nothing to explain, so nothing to leave behind greyed out.
|
||||
const recentMoveTarget = moveTargets.find((p) => p.id === lastMoveTargetId);
|
||||
const storedOrderEditable = canEdit && !viewActive;
|
||||
const ctxRow = rowMenu.target;
|
||||
const ctxItems: ActionMenuItem[] = !ctxRow
|
||||
@@ -615,17 +655,29 @@ export default function Playlists() {
|
||||
id: "play",
|
||||
label: t("card.play"),
|
||||
icon: <Play className="w-4 h-4" />,
|
||||
onSelect: () => setPlayingId(ctxRow.id),
|
||||
onSelect: () => openPlayer(ctxRow.id),
|
||||
},
|
||||
...comfortPin.item(pinVideo(ctxRow.id), ctxRow.title ?? ""),
|
||||
...(canEdit
|
||||
? ([
|
||||
...(recentMoveTarget
|
||||
? [
|
||||
{
|
||||
id: "move-recent",
|
||||
label: t("playlists.moveToRecent", { name: plName(recentMoveTarget) }),
|
||||
icon: <History className="w-4 h-4" />,
|
||||
separatorBefore: true,
|
||||
onSelect: () => void moveItemTo(ctxRow, recentMoveTarget),
|
||||
},
|
||||
]
|
||||
: []),
|
||||
moveTargets.length
|
||||
? {
|
||||
id: "move",
|
||||
label: t("playlists.moveTo"),
|
||||
icon: <FolderInput className="w-4 h-4" />,
|
||||
separatorBefore: true,
|
||||
// Only ONE separator opens the move block, wherever it starts.
|
||||
separatorBefore: !recentMoveTarget,
|
||||
items: moveTargets.map((pl) => ({
|
||||
id: `move-${pl.id}`,
|
||||
label: plName(pl),
|
||||
@@ -647,7 +699,7 @@ export default function Playlists() {
|
||||
id: "move",
|
||||
label: t("playlists.moveTo"),
|
||||
icon: <FolderInput className="w-4 h-4" />,
|
||||
separatorBefore: true,
|
||||
separatorBefore: !recentMoveTarget,
|
||||
disabled: true,
|
||||
title: t("playlists.moveNoTargets"),
|
||||
onSelect: () => {},
|
||||
@@ -780,7 +832,7 @@ export default function Playlists() {
|
||||
</span>
|
||||
)}
|
||||
<button
|
||||
onClick={() => shown.length && setPlayingId(shown[0]!.id)}
|
||||
onClick={() => shown.length && openPlayer(shown[0]!.id)}
|
||||
disabled={!shown.length}
|
||||
className="inline-flex items-center gap-1.5 text-xs font-semibold px-3 py-1.5 rounded-lg btn-primary disabled:opacity-40 transition"
|
||||
>
|
||||
@@ -957,15 +1009,16 @@ export default function Playlists() {
|
||||
{/* One surface for the whole table, outside the rows — it anchors to the pointer, not the row. */}
|
||||
<ContextMenu menu={rowMenu} ariaLabel={t("playlists.rowMenu")} items={ctxItems} />
|
||||
|
||||
{playingIndex >= 0 && (
|
||||
{playing && (
|
||||
<Suspense fallback={null}>
|
||||
<PlayerModal
|
||||
video={shown[playingIndex]!}
|
||||
queue={shown}
|
||||
startIndex={playingIndex}
|
||||
video={playing.video}
|
||||
queue={playing.queue}
|
||||
startIndex={playing.index}
|
||||
startAt={null}
|
||||
onClose={closePlayer}
|
||||
onState={playerState}
|
||||
onRemoveFromList={canEdit ? removeFromPlayer : undefined}
|
||||
/>
|
||||
</Suspense>
|
||||
)}
|
||||
|
||||
Reference in New Issue
Block a user