diff --git a/apps/electron-backend/src/app/api/main.preload.spec-data.ts b/apps/electron-backend/src/app/api/main.preload.spec-data.ts index 229524cf8..3b8b5cfab 100644 --- a/apps/electron-backend/src/app/api/main.preload.spec-data.ts +++ b/apps/electron-backend/src/app/api/main.preload.spec-data.ts @@ -34,7 +34,9 @@ const streams = [{ stream_id: 42, name: 'Channel' }]; const favorites = [{ contentId: 1, playlistId }]; const recentlyViewed = [{ contentId: 2, playlistId }]; const categoryIds = [10, 11]; -const reorderUpdates = [{ content_id: 12, position: 1 }]; +const reorderUpdates = [ + { content_id: 12, playlist_id: 'playlist-1', position: 1 }, +]; const recentItemsBatch = [{ contentId: 13, playlistId }]; const playbackData = { contentXtreamId: 42, diff --git a/apps/electron-backend/src/app/api/main.preload.ts b/apps/electron-backend/src/app/api/main.preload.ts index 743220410..0110962a5 100644 --- a/apps/electron-backend/src/app/api/main.preload.ts +++ b/apps/electron-backend/src/app/api/main.preload.ts @@ -710,7 +710,7 @@ const electronApi: ElectronBridgeApi = { dbGetAllGlobalFavorites: () => ipcRenderer.invoke('DB_GET_ALL_GLOBAL_FAVORITES'), dbReorderGlobalFavorites: ( - updates: { content_id: number; position: number }[] + updates: { content_id: number; playlist_id: string; position: number }[] ) => ipcRenderer.invoke('DB_REORDER_GLOBAL_FAVORITES', updates), // Recently viewed (playlist-specific) dbGetRecentItems: (playlistId: string) => 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..90b65e4de 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,61 @@ 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, playlist_id: 'p1', position: 0 }, + { content_id: 10, playlist_id: 'p1', position: 1 }, + { content_id: 20, playlist_id: 'p2', position: 2 }, + ]) + ).resolves.toEqual({ success: true }); + + expect(updatePrepare).toHaveBeenCalledTimes(1); + expect(placeholderMock).toHaveBeenCalledWith('position'); + expect(placeholderMock).toHaveBeenCalledWith('contentId'); + // Regression: favorites are playlist-scoped, so the UPDATE must + // filter by playlistId too — otherwise a same-contentId favorite + // in another playlist gets its position silently rewritten. + expect(placeholderMock).toHaveBeenCalledWith('playlistId'); + 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, + playlistId: 'p1', + }); + expect(updateRun).toHaveBeenNthCalledWith(2, { + position: 1, + contentId: 10, + playlistId: 'p1', + }); + expect(updateRun).toHaveBeenNthCalledWith(3, { + position: 2, + contentId: 20, + playlistId: 'p2', + }); + }); + }); }); 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..8bb66e98d 100644 --- a/apps/electron-backend/src/app/database/operations/favorites.operations.ts +++ b/apps/electron-backend/src/app/database/operations/favorites.operations.ts @@ -140,7 +140,7 @@ function selectGlobalFavoriteRows( export async function reorderGlobalFavorites( db: AppDatabase, - updates: { content_id: number; position: number }[], + updates: { content_id: number; playlist_id: string; position: number }[], control?: OperationControl ): Promise<{ success: boolean }> { if (!Array.isArray(updates) || updates.length === 0) { @@ -152,20 +152,35 @@ export async function reorderGlobalFavorites( // Drizzle's .set() doesn't accept a bare Placeholder — wrap it in an // sql template so the value resolves to SQL at compile time. + // Scope by (contentId, playlistId): the favorites table is + // playlist-scoped, so filtering by contentId alone would also rewrite + // the position of a same-contentId favorite in another playlist. const updateFavoritePosition = db .update(schema.favorites) .set({ position: sql`${sql.placeholder('position')}` }) - .where(eq(schema.favorites.contentId, sql.placeholder('contentId'))) + .where( + and( + eq(schema.favorites.contentId, sql.placeholder('contentId')), + eq(schema.favorites.playlistId, sql.placeholder('playlistId')) + ) + ) .prepare(); for (const chunk of chunkValues(updates, DEFAULT_BATCH_SIZE)) { await checkpointOperation(control); await db.transaction(() => { - for (const { content_id, position } of chunk) { - updateFavoritePosition.execute({ + for (const { content_id, playlist_id, position } of chunk) { + // 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, + playlistId: playlist_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/apps/electron-backend/src/app/events/database/favorites.events.ts b/apps/electron-backend/src/app/events/database/favorites.events.ts index a0c77ae28..2daef247b 100644 --- a/apps/electron-backend/src/app/events/database/favorites.events.ts +++ b/apps/electron-backend/src/app/events/database/favorites.events.ts @@ -39,7 +39,7 @@ handleWorkerRequest('DB_GET_ALL_GLOBAL_FAVORITES', () => ({})); handleWorkerRequest( 'DB_REORDER_GLOBAL_FAVORITES', - (updates: { content_id: number; position: number }[]) => ({ + (updates: { content_id: number; playlist_id: string; position: number }[]) => ({ updates, }) ); diff --git a/apps/electron-backend/src/app/workers/database.worker.ts b/apps/electron-backend/src/app/workers/database.worker.ts index 868a856d5..3cfd67144 100644 --- a/apps/electron-backend/src/app/workers/database.worker.ts +++ b/apps/electron-backend/src/app/workers/database.worker.ts @@ -770,7 +770,7 @@ async function executeRequest(message: DbWorkerRequestMessage) { case 'DB_REORDER_GLOBAL_FAVORITES': { const payload = message.payload as { - updates: { content_id: number; position: number }[]; + updates: { content_id: number; playlist_id: string; position: number }[]; }; return reorderGlobalFavorites(db, payload.updates); } diff --git a/docs/architecture/sqlite-db-worker.md b/docs/architecture/sqlite-db-worker.md index 09693bccd..4aaa17c61 100644 --- a/docs/architecture/sqlite-db-worker.md +++ b/docs/architecture/sqlite-db-worker.md @@ -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`${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 diff --git a/libs/portal/shared/data-access/src/lib/collection/unified-favorites-data.service.ts b/libs/portal/shared/data-access/src/lib/collection/unified-favorites-data.service.ts index 096fdc6a8..4dbf97134 100644 --- a/libs/portal/shared/data-access/src/lib/collection/unified-favorites-data.service.ts +++ b/libs/portal/shared/data-access/src/lib/collection/unified-favorites-data.service.ts @@ -807,12 +807,17 @@ export class UnifiedFavoritesDataService { } private buildXtreamPositionUpdates(items: UnifiedCollectionItem[]) { - const updates: { content_id: number; position: number }[] = []; + const updates: { + content_id: number; + playlist_id: string; + position: number; + }[] = []; for (const item of items) { if (item.sourceType === 'xtream' && item.contentId != null) { updates.push({ content_id: item.contentId, + playlist_id: item.playlistId, position: updates.length, }); } diff --git a/libs/shared/interfaces/src/lib/electron-api.interface.ts b/libs/shared/interfaces/src/lib/electron-api.interface.ts index 73d222fcb..3ad7c5b5e 100644 --- a/libs/shared/interfaces/src/lib/electron-api.interface.ts +++ b/libs/shared/interfaces/src/lib/electron-api.interface.ts @@ -414,6 +414,8 @@ export interface ElectronBridgeGlobalRecentlyAddedItem extends ElectronBridgeXtr export interface ElectronBridgeFavoriteReorderUpdate { content_id: number; + /** Favorites are playlist-scoped — scope the position write per playlist */ + playlist_id: string; position: number; } diff --git a/libs/workspace/shell/feature/src/lib/global-favorites/global-favorites.service.spec.ts b/libs/workspace/shell/feature/src/lib/global-favorites/global-favorites.service.spec.ts new file mode 100644 index 000000000..34ae1087e --- /dev/null +++ b/libs/workspace/shell/feature/src/lib/global-favorites/global-favorites.service.spec.ts @@ -0,0 +1,116 @@ +import { TestBed } from '@angular/core/testing'; +import { Store } from '@ngrx/store'; +import { TranslateService } from '@ngx-translate/core'; +import { DatabaseService, PlaylistsService } from '@iptvnator/services'; +import { UnifiedFavoriteChannel } from '@iptvnator/portal/shared/util'; +import { GlobalFavoritesService } from './global-favorites.service'; + +describe('GlobalFavoritesService', () => { + let service: GlobalFavoritesService; + let electronApi: { + dbReorderGlobalFavorites: jest.Mock; + dbSetAppState: jest.Mock; + }; + + const makeChannel = ( + overrides: Partial & + Pick + ): UnifiedFavoriteChannel => ({ + name: 'Channel', + logo: null, + playlistName: 'Playlist', + addedAt: new Date(0).toISOString(), + position: 0, + ...overrides, + }); + + beforeEach(() => { + electronApi = { + dbReorderGlobalFavorites: jest + .fn() + .mockResolvedValue({ success: true }), + dbSetAppState: jest.fn().mockResolvedValue({ success: true }), + }; + Object.defineProperty(window, 'electron', { + value: electronApi as unknown as Window['electron'], + configurable: true, + }); + + TestBed.configureTestingModule({ + providers: [ + GlobalFavoritesService, + { provide: Store, useValue: { select: jest.fn() } }, + { provide: DatabaseService, useValue: {} }, + { provide: PlaylistsService, useValue: {} }, + { + provide: TranslateService, + useValue: { instant: (key: string) => key }, + }, + ], + }); + service = TestBed.inject(GlobalFavoritesService); + }); + + afterEach(() => { + Object.defineProperty(window, 'electron', { + value: undefined, + configurable: true, + }); + }); + + describe('reorder', () => { + it('sends playlist-scoped position updates for Xtream favorites', async () => { + // The backend UPDATE filters by (contentId, playlistId); a payload + // without playlist_id silently matches no rows (PR #1143 review). + const channels: UnifiedFavoriteChannel[] = [ + makeChannel({ + uid: 'xtream::playlist-b::20', + sourceType: 'xtream', + playlistId: 'playlist-b', + contentId: 202, + }), + makeChannel({ + uid: 'm3u::playlist-m::url', + sourceType: 'm3u', + playlistId: 'playlist-m', + }), + makeChannel({ + uid: 'xtream::playlist-a::10', + sourceType: 'xtream', + playlistId: 'playlist-a', + contentId: 101, + }), + ]; + + await service.reorder(channels); + + expect(electronApi.dbReorderGlobalFavorites).toHaveBeenCalledWith([ + { content_id: 202, playlist_id: 'playlist-b', position: 0 }, + { content_id: 101, playlist_id: 'playlist-a', position: 1 }, + ]); + }); + + it('persists the full uid order and skips the DB write without Xtream items', async () => { + const channels: UnifiedFavoriteChannel[] = [ + makeChannel({ + uid: 'm3u::playlist-m::url', + sourceType: 'm3u', + playlistId: 'playlist-m', + }), + makeChannel({ + uid: 'stalker::playlist-s::5', + sourceType: 'stalker', + playlistId: 'playlist-s', + }), + ]; + + await service.reorder(channels); + + expect(electronApi.dbReorderGlobalFavorites).not.toHaveBeenCalled(); + expect(electronApi.dbSetAppState).toHaveBeenCalledWith( + 'global-favorites-channel-order-v1', + JSON.stringify(['m3u::playlist-m::url', 'stalker::playlist-s::5']) + ); + }); + }); +}); diff --git a/libs/workspace/shell/feature/src/lib/global-favorites/global-favorites.service.ts b/libs/workspace/shell/feature/src/lib/global-favorites/global-favorites.service.ts index bfea67259..83506948e 100644 --- a/libs/workspace/shell/feature/src/lib/global-favorites/global-favorites.service.ts +++ b/libs/workspace/shell/feature/src/lib/global-favorites/global-favorites.service.ts @@ -141,6 +141,9 @@ export class GlobalFavoritesService { ) .map((ch, index) => ({ content_id: ch.contentId, + // The backend UPDATE is scoped by (contentId, playlistId) — + // without the playlist id the write matches no rows. + playlist_id: ch.playlistId, position: index, }));