fix(favorites): persist custom drag-and-drop order for Xtream favorites (#1143)

* fix(favorites): persist custom drag-and-drop order for Xtream favorites

Prepared-statement writes dispatched via drizzle's `.execute()` on the
better-sqlite3 driver return a promise and defer the write to a microtask.
Inside a synchronous `db.transaction(() => ...)` callback (which cannot
await), the transaction commits before that promise settles, so the write
is a silent no-op — no error, no rows changed.

This bit `reorderGlobalFavorites`: the custom favorites order never
persisted for the per-playlist ("This playlist") Xtream scope, which relies
solely on the `favorites.position` column. The global ("All playlists")
scope masked the bug because it also persists an order to the `appState`
`global-favorites-channel-order-v1` key and re-applies it on read.
`removeRecentItemsBatch` had the same latent bug — batch "clear recent
items" silently did nothing.

Switch both writers to synchronous `.run()`. Add regression coverage that
asserts `.run()` (not `.execute()`) is used and would fail on the old
behavior, and document the gotcha in the DB worker architecture doc.

Verified over CDP against a live Electron instance: reorder writes
positions 0..N, and the order survives navigation and a full reload.

Fixes #1137

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(favorites): scope reorder position writes by playlist

The global favorites reorder wrote the new position filtering only by
content_id, so two Xtream playlists holding a favorite with the same
content_id would clobber each other's persisted order (greptile P1).

Thread playlist_id through the whole reorder path — the renderer builder
(UnifiedCollectionItem already carries playlistId), the IPC contract
(ElectronBridgeFavoriteReorderUpdate + inline payload types), the worker
op — and scope the prepared UPDATE by (contentId, playlistId), matching
the favorites composite unique index.

Tests: favorites.operations.spec asserts the playlistId placeholder and
per-row playlistId payload; preload contract fixture updated.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(favorites): include playlist_id in workspace global favorites reorder payload

The workspace global-favorites reorder path still sent updates with only
content_id and position. Since the backend UPDATE is now scoped by
(contentId, playlistId), that payload binds an undefined playlist id and
matches no rows — the DB write silently no-ops (flagged by Greptile P1).

Also scope the prepared-statement example in the sqlite-db-worker gotcha
doc by (contentId, playlistId) so it no longer documents the
cross-playlist rewrite this PR fixes (flagged by Codex P3).

Regression spec asserts the reorder payload carries playlist_id per item
(fails on the old payload shape) and that the appState uid order is
still persisted for non-Xtream items.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
4grayandClaude Fable 5 authored and GitHub committed 2026-07-12 13:24:58 +02:00
1 parent f856226dc3
commit 54265755ee
14 files changed
+309 -18

No files matched your search

+44
View File
@@ -496,6 +496,50 @@ A running Electron app keeps using the worker bundle it already loaded at
startup. This is a common reason a worker fix appears "not working" in manual
verification even when the source patch is correct.
## Gotchas
### Prepared-statement writes inside a transaction must use `.run()`, not `.execute()`
Drizzle's `PreparedQuery.execute()` on the `better-sqlite3` driver returns a
**promise** and defers the actual SQL to a microtask. Our bulk writers run their
statements inside a **synchronous** `db.transaction(() => { ... })` callback,
which cannot `await`. If the statement is dispatched with `.execute()`, the
transaction commits before the deferred promise settles, so the write is a
**silent no-op** — no error, no rows changed.
Always call the synchronous `.run(placeholderValues)` on prepared statements
executed inside a synchronous transaction callback:
```ts
// favorites is playlist-scoped: filter by (contentId, playlistId), otherwise
// a same-contentId favorite in another playlist gets rewritten too.
const stmt = db.update(schema.favorites)
.set({ position: sql<number>`${sql.placeholder('position')}` })
.where(
and(
eq(schema.favorites.contentId, sql.placeholder('contentId')),
eq(schema.favorites.playlistId, sql.placeholder('playlistId'))
)
)
.prepare();
db.transaction(() => {
for (const { content_id, playlist_id, position } of chunk) {
// NOT .execute()
stmt.run({ position, contentId: content_id, playlistId: playlist_id });
}
});
```
This bit `reorderGlobalFavorites` and `removeRecentItemsBatch` (issue #1137):
custom favorites drag-and-drop order silently never persisted for the
per-playlist ("this playlist") view. Global ("all playlists") favorites masked
it because that path also persists an order to the `appState`
`global-favorites-channel-order-v1` key and re-applies it on read, independent
of the DB `position` column. The mocked operations specs did not catch it —
a jest mock records an `.execute()` call the same as a `.run()` call, so the
regression tests explicitly assert `.run()` is used and `.execute()` is not.
## Testing
### Unit coverage added