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>

apps/electron-backend/src/app/database/operations/favorites.operations.spec.ts

@@ -4,6 +4,10 @@ const eqMock = jest.fn((left: unknown, right: unknown) => ({
     right,
 }));
 const whereMock = jest.fn();
+const placeholderMock = jest.fn((name: string) => ({
+    kind: 'placeholder',
+    name,
+}));
 
 jest.mock('drizzle-orm', () => ({
     and: jest.fn((...conditions: unknown[]) => ({ kind: 'and', conditions })),
@@ -11,11 +15,22 @@ jest.mock('drizzle-orm', () => ({
     desc: jest.fn((value: unknown) => ({ kind: 'desc', value })),
     eq: (left: unknown, right: unknown) => eqMock(left, right),
     inArray: jest.fn(),
-    sql: jest.fn(),
+    sql: Object.assign(
+        jest.fn((strings: TemplateStringsArray, ...values: unknown[]) => ({
+            kind: 'sql',
+            strings: Array.from(strings ?? []),
+            values,
+        })),
+        { placeholder: (name: string) => placeholderMock(name) }
+    ),
 }));
 
 import type { AppDatabase } from '../database.types';
-import { getGlobalFavorites } from './favorites.operations';
+import { createDbMock } from './operations.test-helpers';
+import {
+    getGlobalFavorites,
+    reorderGlobalFavorites,
+} from './favorites.operations';
 
 function createGlobalFavoritesDbMock(rows: unknown[]) {
     const query = {
@@ -48,6 +63,7 @@ describe('favorites.operations', () => {
     beforeEach(() => {
         eqMock.mockClear();
         whereMock.mockClear();
+        placeholderMock.mockClear();
     });
 
     it('filters live global favorites after scanning the small favorites set', async () => {
@@ -76,4 +92,54 @@ describe('favorites.operations', () => {
             }),
         ]);
     });
+
+    describe('reorderGlobalFavorites', () => {
+        it('short-circuits without touching the db when there are no updates', async () => {
+            const { db, update, transaction } = createDbMock();
+
+            await expect(reorderGlobalFavorites(db, [])).resolves.toEqual({
+                success: true,
+            });
+
+            expect(update).not.toHaveBeenCalled();
+            expect(transaction).not.toHaveBeenCalled();
+        });
+
+        it('runs (not executes) the prepared position update per favorite inside a transaction', async () => {
+            const { db, updateRun, updateExecute, updatePrepare, transaction } =
+                createDbMock();
+
+            await expect(
+                reorderGlobalFavorites(db, [
+                    { content_id: 30, position: 0 },
+                    { content_id: 10, position: 1 },
+                    { content_id: 20, position: 2 },
+                ])
+            ).resolves.toEqual({ success: true });
+
+            expect(updatePrepare).toHaveBeenCalledTimes(1);
+            expect(placeholderMock).toHaveBeenCalledWith('position');
+            expect(placeholderMock).toHaveBeenCalledWith('contentId');
+            expect(transaction).toHaveBeenCalledTimes(1);
+
+            // Regression (issue #1137): the prepared UPDATE must be dispatched
+            // with synchronous `.run()`. On the better-sqlite3 driver
+            // `.execute()` defers the write to a promise that never settles
+            // inside the synchronous transaction callback, so favorites
+            // positions silently never persist and the custom order is lost.
+            expect(updateExecute).not.toHaveBeenCalled();
+            expect(updateRun).toHaveBeenNthCalledWith(1, {
+                position: 0,
+                contentId: 30,
+            });
+            expect(updateRun).toHaveBeenNthCalledWith(2, {
+                position: 1,
+                contentId: 10,
+            });
+            expect(updateRun).toHaveBeenNthCalledWith(3, {
+                position: 2,
+                contentId: 20,
+            });
+        });
+    });
 });

apps/electron-backend/src/app/database/operations/favorites.operations.ts

@@ -163,7 +163,13 @@ export async function reorderGlobalFavorites(
 
         await db.transaction(() => {
             for (const { content_id, position } of chunk) {
-                updateFavoritePosition.execute({
+                // Must be .run() (synchronous), NOT .execute(): on the
+                // better-sqlite3 driver .execute() defers the write to a
+                // resolved promise, which never settles inside this
+                // synchronous transaction callback — the UPDATE would be a
+                // silent no-op and the custom favorites order would never
+                // persist (issue #1137).
+                updateFavoritePosition.run({
                     position,
                     contentId: content_id,
                 });

apps/electron-backend/src/app/database/operations/operations.test-helpers.ts

@@ -112,14 +112,31 @@ export function createDbMock(selectResultsByCall: unknown[][] = []) {
     const insertValues = jest.fn().mockResolvedValue(undefined);
     const insert = jest.fn().mockReturnValue({ values: insertValues });
 
-    const updateWhere = jest.fn().mockResolvedValue(undefined);
+    // Prepared-statement writes MUST use `.run()` (synchronous) rather than
+    // `.execute()` inside a synchronous `db.transaction()` callback — the
+    // better-sqlite3 driver's `.execute()` defers to a promise that never
+    // settles before COMMIT, silently dropping the write (issue #1137). The
+    // mock exposes both so specs can assert the correct method is used.
+    const updateRun = jest.fn();
+    const updateExecute = jest.fn().mockResolvedValue(undefined);
+    const updatePrepare = jest
+        .fn()
+        .mockReturnValue({ run: updateRun, execute: updateExecute });
+    const updateWhere = jest.fn().mockReturnValue({
+        prepare: updatePrepare,
+        then: (
+            resolve: (value: unknown) => void,
+            reject: (reason: unknown) => void
+        ) => Promise.resolve(undefined).then(resolve, reject),
+    });
     const updateSet = jest.fn().mockReturnValue({ where: updateWhere });
     const update = jest.fn().mockReturnValue({ set: updateSet });
 
+    const deleteRun = jest.fn();
     const deleteExecute = jest.fn().mockResolvedValue(undefined);
     const deletePrepare = jest
         .fn()
-        .mockReturnValue({ execute: deleteExecute });
+        .mockReturnValue({ execute: deleteExecute, run: deleteRun });
     const deleteResult = {
         prepare: deletePrepare,
         then: (
@@ -152,6 +169,7 @@ export function createDbMock(selectResultsByCall: unknown[][] = []) {
         deleteExecute,
         deleteFn,
         deletePrepare,
+        deleteRun,
         deleteWhere,
         insert,
         insertValues,
@@ -159,6 +177,9 @@ export function createDbMock(selectResultsByCall: unknown[][] = []) {
         select,
         transaction,
         update,
+        updatePrepare,
+        updateRun,
+        updateExecute,
         updateSet,
         updateWhere,
     };

apps/electron-backend/src/app/database/operations/recently-viewed.operations.spec.ts

@@ -200,8 +200,8 @@ describe('recently-viewed.operations', () => {
             expect(transaction).not.toHaveBeenCalled();
         });
 
-        it('executes one prepared placeholder delete per item inside a transaction', async () => {
-            const { db, deleteExecute, deletePrepare, transaction } =
+        it('runs one prepared placeholder delete per item inside a transaction', async () => {
+            const { db, deleteRun, deleteExecute, deletePrepare, transaction } =
                 createDbMock();
 
             await expect(
@@ -215,11 +215,16 @@ describe('recently-viewed.operations', () => {
             expect(mockDrizzle.sql.placeholder).toHaveBeenCalledWith('contentId');
             expect(mockDrizzle.sql.placeholder).toHaveBeenCalledWith('playlistId');
             expect(transaction).toHaveBeenCalledTimes(1);
-            expect(deleteExecute).toHaveBeenNthCalledWith(1, {
+            // Regression (issue #1137): the prepared delete must be dispatched
+            // with synchronous `.run()`. `.execute()` defers to a promise that
+            // never settles inside the synchronous transaction callback, so the
+            // batch delete would silently no-op.
+            expect(deleteExecute).not.toHaveBeenCalled();
+            expect(deleteRun).toHaveBeenNthCalledWith(1, {
                 contentId: 1,
                 playlistId: 'playlist-1',
             });
-            expect(deleteExecute).toHaveBeenNthCalledWith(2, {
+            expect(deleteRun).toHaveBeenNthCalledWith(2, {
                 contentId: 2,
                 playlistId: 'playlist-2',
             });

apps/electron-backend/src/app/database/operations/recently-viewed.operations.ts

@@ -177,7 +177,12 @@ export async function removeRecentItemsBatch(
 
     await db.transaction(() => {
         for (const { contentId, playlistId } of items) {
-            stmt.execute({ contentId, playlistId });
+            // .run() (synchronous), NOT .execute(): the better-sqlite3
+            // driver's .execute() defers the write to a resolved promise
+            // that never settles inside this synchronous transaction
+            // callback, so the DELETE would silently do nothing. See the
+            // matching note in favorites.operations.ts (issue #1137).
+            stmt.run({ contentId, playlistId });
         }
     });
 

docs/architecture/sqlite-db-worker.md

@@ -496,6 +496,42 @@ 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
+const stmt = db.update(schema.favorites)
+    .set({ position: sql<number>`${sql.placeholder('position')}` })
+    .where(eq(schema.favorites.contentId, sql.placeholder('contentId')))
+    .prepare();
+
+db.transaction(() => {
+    for (const { content_id, position } of chunk) {
+        stmt.run({ position, contentId: content_id }); // NOT .execute()
+    }
+});
+```
+
+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