From 008f3038326d7ef97900141bc839f04ee176ddc9 Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Sun, 25 Oct 2026 11:42:04 +0000 Subject: [PATCH 1/2] Mobile: fix the playlist reorder crash when the list changes mid-drag (LUMEN-4) Sentry LUMEN-4 was a fatal "Cannot read property 'key' of undefined" in the playlist screen. react-native-reorderable-list 0.18 reports a drop as indices into the rows it showed when the drag began, and before that its markCells reads keyExtractor(data[i]) for every index between the two, with no bounds check against the current data. A refetch that shortened the list mid-drag (the save after the previous drag, a focus refetch, the TIDAL auto-download poll) left those indices past the end, so the shared keyExtractor read `undefined.key` inside a runOnJS callback. A refetch that only shifted rows moved the wrong track. - SteadyReorderableList wraps ReorderableList: from drag start until the drop is handled it keeps showing the rows the drag began on, then replays the drop onto the latest data by row key (rebaseDrop) and reports that move. A drop whose row is gone reports nothing. - The auto-download refetchInterval pauses while reorder mode is on. - keyExtractor falls back to the index for a missing row. - onReorder ignores indices outside localTracks (playlistAfterDrop), so a reorder can't send undefined track ids. The override/retire semantics are unchanged. Co-Authored-By: Claude Opus 5.5 --- mobile/app/(tabs)/(playlists)/[id].tsx | 27 ++- mobile/components/steady-reorderable-list.tsx | 77 +++++++++ mobile/lib/reorder-drop.ts | 69 ++++++++ mobile/tests/reorder-drop.test.ts | 125 ++++++++++++++ mobile/tests/steady-reorderable-list.test.ts | 154 ++++++++++++++++++ 5 files changed, 438 insertions(+), 14 deletions(-) create mode 100644 mobile/components/steady-reorderable-list.tsx create mode 100644 mobile/lib/reorder-drop.ts create mode 100644 mobile/tests/reorder-drop.test.ts create mode 100644 mobile/tests/steady-reorderable-list.test.ts diff --git a/mobile/app/(tabs)/(playlists)/[id].tsx b/mobile/app/(tabs)/(playlists)/[id].tsx index c7e6bb7e..e5d51fa4 100644 --- a/mobile/app/(tabs)/(playlists)/[id].tsx +++ b/mobile/app/(tabs)/(playlists)/[id].tsx @@ -7,7 +7,7 @@ import { View, } from "react-native"; import { FlashList, type ListRenderItemInfo } from "@shopify/flash-list"; -import ReorderableList, { +import { useIsActive, useReorderableDrag, type ReorderableListReorderEvent, @@ -21,7 +21,6 @@ import { api, compareSortableTracks, errorMessage, - movePlaylistEntry, playlistEntryToTrack as entryToTrack, playlistAutoDownloadRefetchMs, playlistPermissions, @@ -65,7 +64,9 @@ import { SwipeableTrackRow, type SwipeRowProps, } from "../../../components/swipe/swipeable-track-row"; +import { SteadyReorderableList } from "../../../components/steady-reorderable-list"; import { qk } from "../../../lib/query-keys"; +import { playlistAfterDrop, rowKeyOrIndex } from "../../../lib/reorder-drop"; import { listPlaybackQueue, usePlayQueue } from "../../../lib/use-play-queue"; import { useTrackSort } from "../../../lib/use-track-sort"; import { @@ -127,14 +128,16 @@ export default function PlaylistDetailScreen() { enabled: !!userId && !!id, }); + const [reorderMode, setReorderMode] = useState(false); const tracksQuery = useQuery({ queryKey: playlistTracksQueryKey, queryFn: ({ signal }) => api.listPlaylistTracks(id!, { signal }), enabled: !!userId && !!id, // While auto-download saves tracks, refetch so rows flip to their // library copies; after a failure, look again when its retry is due. + // Not while reordering: rows swapped under a drag move the wrong one. refetchInterval: (query) => - playlistQuery.data?.tidal_auto_download + playlistQuery.data?.tidal_auto_download && !reorderMode ? playlistAutoDownloadRefetchMs(query.state.data) : false, }); @@ -157,7 +160,6 @@ export default function PlaylistDetailScreen() { // server list. The override is tied to the snapshot it was made from, so the // next fetched snapshot replaces it without an effect having to reset it. const [override, setOverride] = useState(null); - const [reorderMode, setReorderMode] = useState(false); const { sortKey, sortAsc, onSelectSort, onToggleSortDirection, resetSort } = useTrackSort(); const serverData = tracksQuery.data; @@ -340,9 +342,11 @@ export default function PlaylistDetailScreen() { const onReorder = useCallback( ({ from, to }: ReorderableListReorderEvent) => { - if (!serverData) return; + // The list replays a drop onto `rowModels`, which mirror `localTracks`; + // an index outside them is ignored rather than saved. + const next = playlistAfterDrop(localTracks, from, to); + if (!serverData || !next) return; void Haptics.impactAsync(Haptics.ImpactFeedbackStyle.Medium); - const next = movePlaylistEntry(localTracks, from, to); const edit = { base: serverData, tracks: next }; setOverride(edit); reorderTracks( @@ -566,11 +570,6 @@ export default function PlaylistDetailScreen() { queuedForServer, ]); - const keyExtractor = useCallback( - (item: PlaylistTrackRowModel) => item.key, - [], - ); - const renderDraggableItem = useCallback( ({ item }: { item: PlaylistTrackRowModel }) => ( {showReorderMode ? ( - + {...TRACK_LIST_PERFORMANCE_PROPS} data={rowModels} onReorder={onReorder} - keyExtractor={keyExtractor} + keyExtractor={rowKeyOrIndex} renderItem={renderDraggableItem} ListHeaderComponent={header} // No automatic inset here: the drag math reads the raw scroll @@ -690,7 +689,7 @@ export default function PlaylistDetailScreen() { {...dockScroll} data={displayModels} renderItem={renderReadOnlyItem} - keyExtractor={keyExtractor} + keyExtractor={rowKeyOrIndex} ListHeaderComponent={header} contentInsetAdjustmentBehavior="automatic" style={{ backgroundColor: theme.color.bg }} diff --git a/mobile/components/steady-reorderable-list.tsx b/mobile/components/steady-reorderable-list.tsx new file mode 100644 index 00000000..ca1c3ca2 --- /dev/null +++ b/mobile/components/steady-reorderable-list.tsx @@ -0,0 +1,77 @@ +import { useCallback, useLayoutEffect, useRef, useState } from "react"; +import ReorderableList, { + type ReorderableListDragEndEvent, + type ReorderableListProps, + type ReorderableListReorderEvent, +} from "react-native-reorderable-list"; +import { scheduleOnRN } from "react-native-worklets"; +import { rebaseDrop } from "../lib/reorder-drop"; + +type SteadyReorderableListProps = Omit< + ReorderableListProps, + "keyExtractor" | "onDragStart" | "onDragEnd" +> & { + keyExtractor: (item: T, index: number) => string; +}; + +/** + * A ReorderableList whose rows hold still from the start of a drag until its + * drop is handled. + * + * The library reports a drop as indices into the rows it showed when the + * drag began, and before that reads `keyExtractor(data[i])` for each index + * in between without checking it against the current data. New data + * mid-drag (a refetch) then moved the wrong row, and a shorter list crashed + * the app (Sentry LUMEN-4). So the list keeps the rows it started the drag + * on, and `onReorder` gets the drop replayed onto the latest `data`, which + * the list shows from then on. A drop whose row is gone reports nothing. + */ +export function SteadyReorderableList({ + data, + keyExtractor, + onReorder, + ...props +}: SteadyReorderableListProps) { + const [held, setHeld] = useState(null); + const shown = held ?? data; + // The drag callbacks run from the UI thread, and the drop from the closure + // the list captured when the finger lifted, so they read the latest render + // from here. + const latest = useRef({ shown, data, keyExtractor, onReorder }); + useLayoutEffect(() => { + latest.current = { shown, data, keyExtractor, onReorder }; + }); + + const hold = useCallback(() => setHeld(latest.current.shown), []); + const release = useCallback(() => setHeld(null), []); + const onDragStart = useCallback(() => { + "worklet"; + scheduleOnRN(hold); + }, [hold]); + const onDragEnd = useCallback( + ({ from, to }: ReorderableListDragEndEvent) => { + "worklet"; + // A row put back where it was never reaches onReorder. + if (from === to) scheduleOnRN(release); + }, + [release], + ); + const handleReorder = useCallback(({ from, to }: ReorderableListReorderEvent) => { + const { shown: dragged, data: current, keyExtractor: key, onReorder: report } = + latest.current; + setHeld(null); + const move = rebaseDrop(dragged.map(key), current.map(key), from, to); + if (move) report(move); + }, []); + + return ( + + {...props} + data={shown} + keyExtractor={keyExtractor} + onReorder={handleReorder} + onDragStart={onDragStart} + onDragEnd={onDragEnd} + /> + ); +} diff --git a/mobile/lib/reorder-drop.ts b/mobile/lib/reorder-drop.ts new file mode 100644 index 00000000..ba3010b3 --- /dev/null +++ b/mobile/lib/reorder-drop.ts @@ -0,0 +1,69 @@ +import { movePlaylistEntry } from "@music-library/core"; + +/** + * Drops from react-native-reorderable-list, checked against the rows a list + * holds when they land. + * + * The list reports a drop as indices into the rows it showed when the drag + * began, one drop animation after the finger lifts. A refetch can replace + * those rows in the meantime, so the indices may point at other rows, or past + * the end of a shorter list. + */ + +export type ReorderMove = { from: number; to: number }; + +/** Whether `index` is a slot in `list`. */ +export function isIndexIn(list: readonly unknown[], index: number): boolean { + return Number.isInteger(index) && index >= 0 && index < list.length; +} + +/** + * A list key for a row that may be missing. The list library reads keys by + * index, and a stale index past the end of the data hands it `undefined`. + */ +export function rowKeyOrIndex(row: { key: string } | undefined, index: number): string { + return row?.key ?? String(index); +} + +/** + * The move a drop makes on `current`, where `from` and `to` index `dragged` + * (the row keys the drag began on). The dragged row goes under the row it + * was dropped under; when that row is gone, or the drop was at the top, it + * goes above the row it was dropped above. Null when there's nothing to + * move: an index outside `dragged`, a dragged row that's gone, no neighbour + * left to place it by, or a row already where the drop puts it. + */ +export function rebaseDrop( + dragged: readonly string[], + current: readonly string[], + from: number, + to: number, +): ReorderMove | null { + if (from === to || !isIndexIn(dragged, from) || !isIndexIn(dragged, to)) return null; + const fromNow = current.indexOf(dragged[from]); + if (fromNow < 0) return null; + // Both lists without the dragged row: the drop puts it between + // others[to - 1] and others[to], and the move's `to` indexes `rest` the + // same way (out first, then back in). + const others = dragged.filter((_, i) => i !== from); + const rest = current.filter((_, i) => i !== fromNow); + const below = to > 0 ? rest.indexOf(others[to - 1]) : -1; + const above = below < 0 && to < others.length ? rest.indexOf(others[to]) : -1; + const toNow = below >= 0 ? below + 1 : above; + if (toNow < 0 || toNow === fromNow) return null; + return { from: fromNow, to: toNow }; +} + +/** + * The playlist after moving the row at `from` to `to`, or null unless both + * are distinct slots in `entries`: a reorder built from an index outside the + * list would send the server an undefined track id. + */ +export function playlistAfterDrop( + entries: readonly T[], + from: number, + to: number, +): T[] | null { + if (from === to || !isIndexIn(entries, from) || !isIndexIn(entries, to)) return null; + return movePlaylistEntry(entries, from, to); +} diff --git a/mobile/tests/reorder-drop.test.ts b/mobile/tests/reorder-drop.test.ts new file mode 100644 index 00000000..1f023548 --- /dev/null +++ b/mobile/tests/reorder-drop.test.ts @@ -0,0 +1,125 @@ +import { describe, expect, it } from "vitest"; +import { + isIndexIn, + playlistAfterDrop, + rebaseDrop, + rowKeyOrIndex, +} from "../lib/reorder-drop"; + +const BAD_INDICES = [-1, 5, 1.5, Number.NaN, Number.POSITIVE_INFINITY]; + +/** `keys` after moving `from` to `to` the way the list and the server do. */ +function applyMove(keys: readonly string[], move: { from: number; to: number }): string[] { + const out = [...keys]; + const [key] = out.splice(move.from, 1); + out.splice(move.to, 0, key); + return out; +} + +describe("rebaseDrop", () => { + const rows = ["a", "b", "c", "d", "e"]; + + it("passes a drop through when the rows didn't change", () => { + expect(rebaseDrop(rows, rows, 4, 1)).toEqual({ from: 4, to: 1 }); + expect(rebaseDrop(rows, rows, 0, 4)).toEqual({ from: 0, to: 4 }); + expect(rebaseDrop(rows, [...rows], 2, 0)).toEqual({ from: 2, to: 0 }); + }); + + it("replays a drop onto a list that shrank under the drag (LUMEN-4)", () => { + // Dragging the last row up to second place while a refetch drops "c". + // The library's indices (4, 1) now run past the end of the 4-row list. + const current = ["a", "b", "d", "e"]; + const move = rebaseDrop(rows, current, 4, 1); + expect(move).toEqual({ from: 3, to: 1 }); + expect(applyMove(current, move!)).toEqual(["a", "e", "b", "d"]); + }); + + it("moves the dragged row, not whatever took its index", () => { + // "a" vanished, so index 2 is now "d"; the drag was "c" to the bottom. + const current = ["b", "c", "d", "e"]; + const move = rebaseDrop(rows, current, 2, 4); + expect(applyMove(current, move!)).toEqual(["b", "d", "e", "c"]); + }); + + it("goes above the row it was dropped above when the one it was dropped under is gone", () => { + // "b" to between "c" and "d", but "c" is gone by the drop. + const current = ["a", "b", "d", "e"]; + const move = rebaseDrop(rows, current, 1, 2); + expect(move).toBeNull(); // "b" already sits right above "d" + const moved = rebaseDrop(rows, ["b", "a", "d", "e"], 1, 2); + expect(applyMove(["b", "a", "d", "e"], moved!)).toEqual(["a", "b", "d", "e"]); + }); + + it("puts a drop at the top above the row that was first", () => { + const current = ["x", "a", "b", "d", "e"]; + const move = rebaseDrop(rows, current, 3, 0); + expect(applyMove(current, move!)).toEqual(["x", "d", "a", "b", "e"]); + }); + + it("drops the move when the dragged row is gone", () => { + expect(rebaseDrop(rows, ["a", "b", "c", "d"], 4, 1)).toBeNull(); + }); + + it("drops the move when both its new neighbours are gone", () => { + expect(rebaseDrop(rows, ["c", "d", "e"], 4, 1)).toBeNull(); + }); + + it("ignores indices outside the rows the drag began on", () => { + for (const bad of BAD_INDICES) { + expect(rebaseDrop(rows, rows, bad, 1)).toBeNull(); + expect(rebaseDrop(rows, rows, 1, bad)).toBeNull(); + } + expect(rebaseDrop(rows, rows, 2, 2)).toBeNull(); + expect(rebaseDrop([], [], 0, 0)).toBeNull(); + }); + + it("only ever reports in-range moves of the dragged row, however the list shrank", () => { + // Every drop on five rows, against every list left after removing rows. + for (let mask = 0; mask < 1 << rows.length; mask++) { + const current = rows.filter((_, i) => !(mask & (1 << i))); + for (let from = 0; from < rows.length; from++) { + for (let to = 0; to < rows.length; to++) { + const move = rebaseDrop(rows, current, from, to); + if (!move) continue; + expect(isIndexIn(current, move.from)).toBe(true); + expect(isIndexIn(current, move.to)).toBe(true); + expect(move.from).not.toBe(move.to); + expect(current[move.from]).toBe(rows[from]); + } + } + } + }); +}); + +describe("rowKeyOrIndex", () => { + it("keys a row by its key, and a missing one by its index", () => { + expect(rowKeyOrIndex({ key: "t1:2026" }, 0)).toBe("t1:2026"); + // What the library hands keyExtractor for a stale index past the end. + expect(rowKeyOrIndex(undefined, 4)).toBe("4"); + }); +}); + +describe("playlistAfterDrop", () => { + const entries = [ + { position: 0, track_id: "a" }, + { position: 1, track_id: "b" }, + { position: 3, track_id: "c" }, + ]; + + it("moves a row and keeps the server's slots", () => { + expect(playlistAfterDrop(entries, 2, 0)).toEqual([ + { position: 0, track_id: "c" }, + { position: 1, track_id: "a" }, + { position: 3, track_id: "b" }, + ]); + }); + + it("ignores out-of-range indices instead of building a reorder with undefined ids", () => { + for (const bad of [-1, 3, 0.5, Number.NaN]) { + expect(playlistAfterDrop(entries, bad, 0)).toBeNull(); + expect(playlistAfterDrop(entries, 0, bad)).toBeNull(); + } + expect(playlistAfterDrop(entries, 1, 1)).toBeNull(); + expect(playlistAfterDrop([], 0, 0)).toBeNull(); + }); +}); diff --git a/mobile/tests/steady-reorderable-list.test.ts b/mobile/tests/steady-reorderable-list.test.ts new file mode 100644 index 00000000..928b42dc --- /dev/null +++ b/mobile/tests/steady-reorderable-list.test.ts @@ -0,0 +1,154 @@ +import type { ReactElement } from "react"; +import type { + ReorderableListDragEndEvent, + ReorderableListDragStartEvent, + ReorderableListReorderEvent, +} from "react-native-reorderable-list"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { SteadyReorderableList } from "../components/steady-reorderable-list"; + +// Just enough React to render the list outside a renderer: hook cells kept by +// call order across renders, and layout effects run once a render is done. +const hooks = vi.hoisted(() => ({ + cells: [] as unknown[], + cursor: 0, + layoutEffects: [] as (() => void)[], +})); +vi.mock("react", async (importOriginal) => ({ + ...(await importOriginal()), + useState: (initial: T) => { + const cell = hooks.cursor++; + if (!(cell in hooks.cells)) hooks.cells[cell] = initial; + return [hooks.cells[cell], (next: T) => (hooks.cells[cell] = next)]; + }, + useRef: (initial: T) => { + const cell = hooks.cursor++; + if (!(cell in hooks.cells)) hooks.cells[cell] = { current: initial }; + return hooks.cells[cell]; + }, + useCallback: (fn: T) => fn, + useLayoutEffect: (effect: () => void) => { + hooks.layoutEffects.push(effect); + }, +})); +// Worklets run inline; the UI thread is out of the picture here. +vi.mock("react-native-worklets", () => ({ + scheduleOnRN: (fn: (...args: unknown[]) => void, ...args: unknown[]) => fn(...args), +})); +vi.mock("react-native-reorderable-list", () => ({ default: () => null })); + +type Row = { key: string }; +type ListProps = { + data: Row[]; + keyExtractor: (item: Row, index: number) => string; + onReorder: (event: ReorderableListReorderEvent) => void; + onDragStart: (event: ReorderableListDragStartEvent) => void; + onDragEnd: (event: ReorderableListDragEndEvent) => void; +}; + +const rows = (...keys: string[]): Row[] => keys.map((key) => ({ key })); +const onReorder = vi.fn<(event: ReorderableListReorderEvent) => void>(); + +/** Renders the list with `data` and returns what it hands ReorderableList. */ +function render(data: Row[], report = onReorder): ListProps { + hooks.cursor = 0; + const element = SteadyReorderableList({ + data, + keyExtractor: (row) => row.key, + onReorder: report, + renderItem: () => null, + }) as ReactElement; + for (const effect of hooks.layoutEffects.splice(0)) effect(); + return element.props; +} + +/** + * What react-native-reorderable-list 0.18 does when a drop lands + * (`reorder` → `markCells` in ReorderableListCore): read the key of every + * row between the two indices, then report the move. + */ +function landDrop(list: ListProps, from: number, to: number) { + for (let i = Math.min(from, to); i <= Math.max(from, to); i++) { + list.keyExtractor(list.data[i], i); + } + list.onReorder({ from, to }); +} + +beforeEach(() => { + hooks.cells = []; + hooks.layoutEffects = []; + onReorder.mockReset(); +}); + +describe("SteadyReorderableList", () => { + it("shows its data while nothing is dragged", () => { + const five = rows("a", "b", "c", "d", "e"); + expect(render(five).data).toBe(five); + const four = rows("a", "b", "c", "d"); + expect(render(four).data).toBe(four); + }); + + it("holds the rows a drag began on while a refetch shrinks the list (LUMEN-4)", () => { + const five = rows("a", "b", "c", "d", "e"); + render(five).onDragStart({ index: 4 }); + // A refetch drops "c" mid-drag: indices 4 and up no longer exist. + const four = rows("a", "b", "d", "e"); + let list = render(four); + expect(list.data).toBe(five); + + // Last row up to second place. Before the fix the library read + // keyExtractor(four[4]) here and crashed on `undefined.key`. + list.onDragEnd({ from: 4, to: 1 }); + expect(() => landDrop(list, 4, 1)).not.toThrow(); + expect(onReorder).toHaveBeenCalledExactlyOnceWith({ from: 3, to: 1 }); + + // Once the drop is handled the list takes the refetched rows. + list = render(four); + expect(list.data).toBe(four); + }); + + it("replays a drop onto rows a refetch reordered", () => { + const before = rows("a", "b", "c", "d"); + render(before).onDragStart({ index: 0 }); + const after = rows("b", "a", "c", "d"); + const list = render(after); + expect(list.data).toBe(before); + list.onDragEnd({ from: 0, to: 3 }); + landDrop(list, 0, 3); + expect(onReorder).toHaveBeenCalledExactlyOnceWith({ from: 1, to: 3 }); + }); + + it("reports nothing when the dragged row is gone by the drop", () => { + render(rows("a", "b", "c")).onDragStart({ index: 2 }); + const left = rows("a", "b"); + const list = render(left); + list.onDragEnd({ from: 2, to: 0 }); + landDrop(list, 2, 0); + expect(onReorder).not.toHaveBeenCalled(); + expect(render(left).data).toBe(left); + }); + + it("lets go when a row is put back where it was, which never reaches onReorder", () => { + render(rows("a", "b", "c")).onDragStart({ index: 1 }); + const fresh = rows("a", "b", "c", "d"); + const list = render(fresh); + expect(list.data).not.toBe(fresh); + list.onDragEnd({ from: 1, to: 1 }); + expect(render(fresh).data).toBe(fresh); + expect(onReorder).not.toHaveBeenCalled(); + }); + + it("calls the latest onReorder with the latest rows", () => { + const list = render(rows("a", "b", "c")); + list.onDragStart({ index: 0 }); + const latest = vi.fn(); + render(rows("a", "c", "b"), latest); + // The library calls the onReorder it captured when the finger lifted, + // from an older render. + list.onDragEnd({ from: 0, to: 2 }); + landDrop(list, 0, 2); + expect(onReorder).not.toHaveBeenCalled(); + // "a" goes under "c", in the order the playlist has now. + expect(latest).toHaveBeenCalledExactlyOnceWith({ from: 0, to: 1 }); + }); +}); From 256f4da65dc24e0b9b37518b66d2fe651f0a5a41 Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Sun, 25 Oct 2026 11:57:16 +0000 Subject: [PATCH 2/2] Mobile: number repeated row keys; let go of a held drag after the drop settles - One add can save a track twice with the same added_at and adder, so their row keys matched and a drop moved the first copy instead of the dragged one. numberRepeatedKeys suffixes repeats in list order. (CodeRabbit) - The hold now lets go a little after the drop animation, unless onReorder arrives first. The library calls onReorder neither for a row put back in its slot nor when the finger lifts exactly on the new slot. The first case released mid-animation; the second left the list holding stale rows until the next drop. The rows a drag's indices refer to sit in a ref, so a late drop still replays correctly, and a new drag cancels the previous drag's timer. (Fable round 1, two lows) Co-Authored-By: Claude Opus 5.5 --- mobile/app/(tabs)/(playlists)/[id].tsx | 25 +++++---- mobile/components/steady-reorderable-list.tsx | 49 +++++++++++------ mobile/lib/reorder-drop.ts | 16 +++++- mobile/tests/reorder-drop.test.ts | 22 ++++++++ mobile/tests/steady-reorderable-list.test.ts | 53 +++++++++++++++++-- 5 files changed, 136 insertions(+), 29 deletions(-) diff --git a/mobile/app/(tabs)/(playlists)/[id].tsx b/mobile/app/(tabs)/(playlists)/[id].tsx index e5d51fa4..164e1554 100644 --- a/mobile/app/(tabs)/(playlists)/[id].tsx +++ b/mobile/app/(tabs)/(playlists)/[id].tsx @@ -66,7 +66,11 @@ import { } from "../../../components/swipe/swipeable-track-row"; import { SteadyReorderableList } from "../../../components/steady-reorderable-list"; import { qk } from "../../../lib/query-keys"; -import { playlistAfterDrop, rowKeyOrIndex } from "../../../lib/reorder-drop"; +import { + numberRepeatedKeys, + playlistAfterDrop, + rowKeyOrIndex, +} from "../../../lib/reorder-drop"; import { listPlaybackQueue, usePlayQueue } from "../../../lib/use-play-queue"; import { useTrackSort } from "../../../lib/use-track-sort"; import { @@ -282,15 +286,16 @@ export default function PlaylistDetailScreen() { if (!canEdit && reorderMode) setReorderMode(false); }, [canEdit, reorderMode]); - const rowModels = useMemo( - () => - localTracks.map((entry) => ({ - key: playlistTrackKey(entry), - entry, - track: entryToTrack(entry), - })), - [localTracks], - ); + const rowModels = useMemo(() => { + // One add can save a track twice with the same timestamp and adder, and + // a drop is matched to its row by key. + const keys = numberRepeatedKeys(localTracks.map(playlistTrackKey)); + return localTracks.map((entry, i) => ({ + key: keys[i], + entry, + track: entryToTrack(entry), + })); + }, [localTracks]); // What the list actually shows; the play queue follows this order too. const displayModels = useMemo( () => diff --git a/mobile/components/steady-reorderable-list.tsx b/mobile/components/steady-reorderable-list.tsx index ca1c3ca2..41887053 100644 --- a/mobile/components/steady-reorderable-list.tsx +++ b/mobile/components/steady-reorderable-list.tsx @@ -1,12 +1,16 @@ -import { useCallback, useLayoutEffect, useRef, useState } from "react"; +import { useCallback, useEffect, useLayoutEffect, useRef, useState } from "react"; import ReorderableList, { - type ReorderableListDragEndEvent, type ReorderableListProps, type ReorderableListReorderEvent, } from "react-native-reorderable-list"; import { scheduleOnRN } from "react-native-worklets"; import { rebaseDrop } from "../lib/reorder-drop"; +/** The library's default `animationDuration`: how long a dropped row takes to settle. */ +const DROP_ANIMATION_MS = 200; +/** Slack after the drop animation for the library's onReorder to reach JS. */ +export const LET_GO_AFTER_DROP_MS = 300; + type SteadyReorderableListProps = Omit< ReorderableListProps, "keyExtractor" | "onDragStart" | "onDragEnd" @@ -34,33 +38,48 @@ export function SteadyReorderableList({ }: SteadyReorderableListProps) { const [held, setHeld] = useState(null); const shown = held ?? data; + const letGoMs = (props.animationDuration ?? DROP_ANIMATION_MS) + LET_GO_AFTER_DROP_MS; // The drag callbacks run from the UI thread, and the drop from the closure // the list captured when the finger lifted, so they read the latest render // from here. - const latest = useRef({ shown, data, keyExtractor, onReorder }); + const latest = useRef({ shown, data, keyExtractor, onReorder, letGoMs }); useLayoutEffect(() => { - latest.current = { shown, data, keyExtractor, onReorder }; + latest.current = { shown, data, keyExtractor, onReorder, letGoMs }; }); + // The rows the current drag's indices refer to, kept even if the hold + // lets go before its drop arrives. + const dragRows = useRef(null); + const letGoTimer = useRef>(undefined); + useEffect(() => () => clearTimeout(letGoTimer.current), []); - const hold = useCallback(() => setHeld(latest.current.shown), []); - const release = useCallback(() => setHeld(null), []); + const hold = useCallback(() => { + clearTimeout(letGoTimer.current); + dragRows.current = latest.current.shown; + setHeld(dragRows.current); + }, []); + // The library calls onReorder once the dropped row has settled, except for + // a row put back where it was, or a finger lifted exactly on the new slot. + // Let go a little after the drop animation unless the drop came first. + const settle = useCallback(() => { + clearTimeout(letGoTimer.current); + letGoTimer.current = setTimeout(() => setHeld(null), latest.current.letGoMs); + }, []); const onDragStart = useCallback(() => { "worklet"; scheduleOnRN(hold); }, [hold]); - const onDragEnd = useCallback( - ({ from, to }: ReorderableListDragEndEvent) => { - "worklet"; - // A row put back where it was never reaches onReorder. - if (from === to) scheduleOnRN(release); - }, - [release], - ); + const onDragEnd = useCallback(() => { + "worklet"; + scheduleOnRN(settle); + }, [settle]); const handleReorder = useCallback(({ from, to }: ReorderableListReorderEvent) => { + clearTimeout(letGoTimer.current); const { shown: dragged, data: current, keyExtractor: key, onReorder: report } = latest.current; + const rows = dragRows.current ?? dragged; + dragRows.current = null; setHeld(null); - const move = rebaseDrop(dragged.map(key), current.map(key), from, to); + const move = rebaseDrop(rows.map(key), current.map(key), from, to); if (move) report(move); }, []); diff --git a/mobile/lib/reorder-drop.ts b/mobile/lib/reorder-drop.ts index ba3010b3..37cccf3f 100644 --- a/mobile/lib/reorder-drop.ts +++ b/mobile/lib/reorder-drop.ts @@ -25,13 +25,27 @@ export function rowKeyOrIndex(row: { key: string } | undefined, index: number): return row?.key ?? String(index); } +/** + * `keys` with each repeat numbered in list order ("k", "k#1", "k#2"), so rows + * that share a key still get distinct ones. + */ +export function numberRepeatedKeys(keys: readonly string[]): string[] { + const seen = new Map(); + return keys.map((key) => { + const count = seen.get(key) ?? 0; + seen.set(key, count + 1); + return count === 0 ? key : `${key}#${count}`; + }); +} + /** * The move a drop makes on `current`, where `from` and `to` index `dragged` * (the row keys the drag began on). The dragged row goes under the row it * was dropped under; when that row is gone, or the drop was at the top, it * goes above the row it was dropped above. Null when there's nothing to * move: an index outside `dragged`, a dragged row that's gone, no neighbour - * left to place it by, or a row already where the drop puts it. + * left to place it by, or a row already where the drop puts it. Keys must + * be unique (see `numberRepeatedKeys`). */ export function rebaseDrop( dragged: readonly string[], diff --git a/mobile/tests/reorder-drop.test.ts b/mobile/tests/reorder-drop.test.ts index 1f023548..81dabe7a 100644 --- a/mobile/tests/reorder-drop.test.ts +++ b/mobile/tests/reorder-drop.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { isIndexIn, + numberRepeatedKeys, playlistAfterDrop, rebaseDrop, rowKeyOrIndex, @@ -91,6 +92,27 @@ describe("rebaseDrop", () => { }); }); +describe("numberRepeatedKeys", () => { + it("numbers repeats in list order and leaves unique keys alone", () => { + expect(numberRepeatedKeys(["a", "b", "a", "c", "a"])).toEqual([ + "a", + "b", + "a#1", + "c", + "a#2", + ]); + expect(numberRepeatedKeys([])).toEqual([]); + }); + + it("lets a drop move the copy that was dragged, not the first one", () => { + // The same track saved twice by one add: equal keys before numbering. + const keys = numberRepeatedKeys(["a", "b", "a"]); + const move = rebaseDrop(keys, keys, 2, 0); + expect(move).toEqual({ from: 2, to: 0 }); + expect(applyMove(["a1", "b", "a2"], move!)).toEqual(["a2", "a1", "b"]); + }); +}); + describe("rowKeyOrIndex", () => { it("keys a row by its key, and a missing one by its index", () => { expect(rowKeyOrIndex({ key: "t1:2026" }, 0)).toBe("t1:2026"); diff --git a/mobile/tests/steady-reorderable-list.test.ts b/mobile/tests/steady-reorderable-list.test.ts index 928b42dc..d543bfcf 100644 --- a/mobile/tests/steady-reorderable-list.test.ts +++ b/mobile/tests/steady-reorderable-list.test.ts @@ -4,11 +4,15 @@ import type { ReorderableListDragStartEvent, ReorderableListReorderEvent, } from "react-native-reorderable-list"; -import { beforeEach, describe, expect, it, vi } from "vitest"; -import { SteadyReorderableList } from "../components/steady-reorderable-list"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + LET_GO_AFTER_DROP_MS, + SteadyReorderableList, +} from "../components/steady-reorderable-list"; // Just enough React to render the list outside a renderer: hook cells kept by // call order across renders, and layout effects run once a render is done. +// Mount effects only clean up, so they're skipped. const hooks = vi.hoisted(() => ({ cells: [] as unknown[], cursor: 0, @@ -27,6 +31,7 @@ vi.mock("react", async (importOriginal) => ({ return hooks.cells[cell]; }, useCallback: (fn: T) => fn, + useEffect: () => {}, useLayoutEffect: (effect: () => void) => { hooks.layoutEffects.push(effect); }, @@ -74,11 +79,16 @@ function landDrop(list: ListProps, from: number, to: number) { list.onReorder({ from, to }); } +/** How long after a lift the list lets go if no drop arrives. */ +const LET_GO_MS = 200 + LET_GO_AFTER_DROP_MS; + beforeEach(() => { + vi.useFakeTimers(); hooks.cells = []; hooks.layoutEffects = []; onReorder.mockReset(); }); +afterEach(() => vi.useRealTimers()); describe("SteadyReorderableList", () => { it("shows its data while nothing is dragged", () => { @@ -128,16 +138,53 @@ describe("SteadyReorderableList", () => { expect(render(left).data).toBe(left); }); - it("lets go when a row is put back where it was, which never reaches onReorder", () => { + it("lets go once a row put back where it was has settled, which never reaches onReorder", () => { render(rows("a", "b", "c")).onDragStart({ index: 1 }); const fresh = rows("a", "b", "c", "d"); const list = render(fresh); expect(list.data).not.toBe(fresh); list.onDragEnd({ from: 1, to: 1 }); + // Not while the row is still sliding back into its slot. + vi.advanceTimersByTime(LET_GO_MS - 1); + expect(render(fresh).data).not.toBe(fresh); + vi.advanceTimersByTime(1); expect(render(fresh).data).toBe(fresh); expect(onReorder).not.toHaveBeenCalled(); }); + it("lets go when the library drops a real move without calling onReorder", () => { + // ReorderableListCore skips `reorder` when the finger lifts exactly on + // the new slot; the hold mustn't outlive that drag. + render(rows("a", "b", "c")).onDragStart({ index: 0 }); + const fresh = rows("a", "b", "c", "d"); + render(fresh).onDragEnd({ from: 0, to: 2 }); + vi.advanceTimersByTime(LET_GO_MS); + expect(render(fresh).data).toBe(fresh); + }); + + it("doesn't let go of the next drag on the last one's timer", () => { + const first = rows("a", "b", "c"); + render(first).onDragStart({ index: 1 }); + render(rows("a", "b", "c", "d")).onDragEnd({ from: 1, to: 1 }); + vi.advanceTimersByTime(LET_GO_MS - 100); + // A new drag on the still-held rows, before the first one let go. + render(rows("a", "b", "c", "d")).onDragStart({ index: 0 }); + vi.advanceTimersByTime(LET_GO_MS); + expect(render(rows("a", "b", "c", "d")).data).toBe(first); + }); + + it("replays a drop that arrives after the hold let go onto the rows the drag began on", () => { + const list = render(rows("a", "b", "c")); + list.onDragStart({ index: 0 }); + const fresh = rows("x", "a", "b", "c"); + render(fresh).onDragEnd({ from: 0, to: 2 }); + vi.advanceTimersByTime(LET_GO_MS); + expect(render(fresh).data).toBe(fresh); + // A busy JS thread delivers the drop late: indices still refer to a, b, c. + list.onReorder({ from: 0, to: 2 }); + expect(onReorder).toHaveBeenCalledExactlyOnceWith({ from: 1, to: 3 }); + }); + it("calls the latest onReorder with the latest rows", () => { const list = render(rows("a", "b", "c")); list.onDragStart({ index: 0 });