diff --git a/apps/electron-backend/src/app/database/operations/favorites.operations.spec.ts b/apps/electron-backend/src/app/database/operations/favorites.operations.spec.ts index 6db591360..0024409f1 100644 --- a/apps/electron-backend/src/app/database/operations/favorites.operations.spec.ts +++ b/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, + }); + }); + }); }); diff --git a/apps/electron-backend/src/app/database/operations/favorites.operations.ts b/apps/electron-backend/src/app/database/operations/favorites.operations.ts index c0b402a63..ebf5dba17 100644 --- a/apps/electron-backend/src/app/database/operations/favorites.operations.ts +++ b/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, }); diff --git a/apps/electron-backend/src/app/database/operations/operations.test-helpers.ts b/apps/electron-backend/src/app/database/operations/operations.test-helpers.ts index cf6b3c49f..6a9c6904f 100644 --- a/apps/electron-backend/src/app/database/operations/operations.test-helpers.ts +++ b/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, }; diff --git a/apps/electron-backend/src/app/database/operations/recently-viewed.operations.spec.ts b/apps/electron-backend/src/app/database/operations/recently-viewed.operations.spec.ts index a1a151a8c..4dce217f1 100644 --- a/apps/electron-backend/src/app/database/operations/recently-viewed.operations.spec.ts +++ b/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', }); diff --git a/apps/electron-backend/src/app/database/operations/recently-viewed.operations.ts b/apps/electron-backend/src/app/database/operations/recently-viewed.operations.ts index d1df9adff..fee6df185 100644 --- a/apps/electron-backend/src/app/database/operations/recently-viewed.operations.ts +++ b/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 }); } }); diff --git a/docs/architecture/sqlite-db-worker.md b/docs/architecture/sqlite-db-worker.md index 09693bccd..455caaead 100644 --- a/docs/architecture/sqlite-db-worker.md +++ b/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`${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