From c3260e8e01e4704b57137d787ffe9cfec7941cbc Mon Sep 17 00:00:00 2001 From: Rajarshee Chatterjee Date: Sat, 1 Aug 2026 17:10:29 +0530 Subject: [PATCH] fix: Resolve Chapter Download Queue and Removal Issues (Closes #1937) --- .../BackgroundTaskScheduler.kt | 5 ++ .../NativeBackgroundTasksModule.kt | 1 + .../TaskActionReceiver.kt | 3 +- src/database/queries/ChapterQueries.ts | 28 +++++++---- .../queries/__tests__/ChapterQueries.test.ts | 39 ++++++++++++++- src/hooks/persisted/useDownload.ts | 5 +- src/screens/library/hooks/useLibrary.ts | 25 ++++++++++ src/screens/more/DownloadsScreen.tsx | 18 +++++-- .../more/components/RemoveDownloadsDialog.tsx | 2 +- .../novel/components/NovelScreenList.tsx | 26 ++++++++-- .../__tests__/NovelScreenList.test.tsx | 48 +++++++++++++++++++ .../novel/hooks/useDownloadReconciliation.ts | 12 ++++- .../__tests__/taskDefinitions.test.ts | 34 +++++++++++++ src/services/backgroundTasks/index.ts | 1 + .../backgroundTasks/taskDefinitions.ts | 19 ++++++++ 15 files changed, 242 insertions(+), 24 deletions(-) diff --git a/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/BackgroundTaskScheduler.kt b/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/BackgroundTaskScheduler.kt index 471bafb1..6ef1018e 100644 --- a/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/BackgroundTaskScheduler.kt +++ b/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/BackgroundTaskScheduler.kt @@ -9,6 +9,7 @@ import androidx.work.ExistingWorkPolicy import androidx.work.NetworkType import androidx.work.OneTimeWorkRequestBuilder import androidx.work.WorkManager +import androidx.work.await import java.util.UUID object BackgroundTaskScheduler { @@ -36,6 +37,10 @@ object BackgroundTaskScheduler { return request.id } + suspend fun cancel(context: Context, taskId: String) { + WorkManager.getInstance(context).cancelAllWorkByTag(taskId).await() + } + suspend fun enqueueLibraryUpdate( context: Context, title: String, diff --git a/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/NativeBackgroundTasksModule.kt b/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/NativeBackgroundTasksModule.kt index 2c4e8750..45d010f9 100644 --- a/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/NativeBackgroundTasksModule.kt +++ b/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/NativeBackgroundTasksModule.kt @@ -107,6 +107,7 @@ class NativeBackgroundTasksModule : Module() { if (TaskExecutionRegistry.isActive(taskId)) { emitInterruption(taskId, "cancel") } + BackgroundTaskScheduler.cancel(appContext.reactContext!!, taskId) dao.updateCheckpoint(taskId, null, System.currentTimeMillis()) TaskNotificationFactory.dismiss(appContext.reactContext!!, taskId) } diff --git a/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/TaskActionReceiver.kt b/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/TaskActionReceiver.kt index e94698ed..3770ab89 100644 --- a/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/TaskActionReceiver.kt +++ b/modules/native-background-tasks/android/src/main/java/expo/modules/nativebackgroundtasks/TaskActionReceiver.kt @@ -33,6 +33,7 @@ class TaskActionReceiver : BroadcastReceiver() { if (TaskExecutionRegistry.isActive(taskId)) { NativeBackgroundTasksModule.emitInterruption(taskId, "cancel") } + BackgroundTaskScheduler.cancel(context, taskId) TaskNotificationFactory.dismiss(context, taskId) } } @@ -41,4 +42,4 @@ class TaskActionReceiver : BroadcastReceiver() { } } } -} \ No newline at end of file +} diff --git a/src/database/queries/ChapterQueries.ts b/src/database/queries/ChapterQueries.ts index eb7e251f..e83fcff9 100644 --- a/src/database/queries/ChapterQueries.ts +++ b/src/database/queries/ChapterQueries.ts @@ -245,7 +245,6 @@ export const increaseTimeSpent = async ( }); }; -// TODO: Remove the need for the chapters array, as it could lead to not deleting the downloaded files but just marking them as not downloaded /* Deletes all downloaded chapters from the database */ @@ -255,19 +254,28 @@ export const deleteDownloads = async ( if (!chapters?.length) { return; } - chapters.forEach(chapter => { - deleteDownloadedFiles(chapter.pluginId, chapter.novelId, chapter.id); - }); + await Promise.all( + chapters.map(chapter => + deleteDownloadedFiles(chapter.pluginId, chapter.novelId, chapter.id), + ), + ); + const chapterIds = chapters.map(chapter => chapter.id); await dbManager.write(async tx => { - await tx.update(chapterSchema).set({ isDownloaded: false }).run(); + await tx + .update(chapterSchema) + .set({ isDownloaded: false }) + .where(inArray(chapterSchema.id, chapterIds)) + .run(); }); }; export const deleteReadChaptersFromDb = async (): Promise => { const chapters = await getReadDownloadedChapters(); - chapters?.forEach(chapter => { - deleteDownloadedFiles(chapter.pluginId, chapter.novelId, chapter.id); - }); + await Promise.all( + chapters.map(chapter => + deleteDownloadedFiles(chapter.pluginId, chapter.novelId, chapter.id), + ), + ); const chapterIds = chapters?.map(chapter => chapter.id); if (chapterIds?.length) { await dbManager.write(async tx => { @@ -460,7 +468,8 @@ export const getAllUndownloadedChapters = async ( eq(chapterSchema.novelId, novelId), eq(chapterSchema.isDownloaded, false), ), - ); + ) + .orderBy(asc(castInt(chapterSchema.page)), asc(chapterSchema.position)); /** * @deprecated, use getNovelChapters with whereConditions instead */ @@ -477,6 +486,7 @@ export const getAllUndownloadedAndUnreadChapters = async ( eq(chapterSchema.unread, true), ), ) + .orderBy(asc(castInt(chapterSchema.page)), asc(chapterSchema.position)) .all(); export const getChapter = async (chapterId: number) => diff --git a/src/database/queries/__tests__/ChapterQueries.test.ts b/src/database/queries/__tests__/ChapterQueries.test.ts index 37a89acb..2a04fd80 100644 --- a/src/database/queries/__tests__/ChapterQueries.test.ts +++ b/src/database/queries/__tests__/ChapterQueries.test.ts @@ -509,9 +509,14 @@ describe('ChapterQueries', () => { await insertTestChapter(testDb, novelId, { isDownloaded: true, }); + const untouchedChapterId = await insertTestChapter(testDb, novelId, { + isDownloaded: true, + }); const chapters = await getNovelChapters(novelId); - const downloadedChapters = chapters.filter(c => c.isDownloaded); + const downloadedChapters = chapters.filter( + c => c.isDownloaded && c.id !== untouchedChapterId, + ); await deleteDownloads( downloadedChapters.map(c => ({ id: c.id, @@ -521,7 +526,14 @@ describe('ChapterQueries', () => { ); const updatedChapters = await getNovelChapters(novelId); - expect(updatedChapters.every(c => c.isDownloaded === false)).toBe(true); + expect( + updatedChapters + .filter(c => c.id !== untouchedChapterId) + .every(c => c.isDownloaded === false), + ).toBe(true); + expect( + updatedChapters.find(c => c.id === untouchedChapterId)?.isDownloaded, + ).toBe(true); }); }); @@ -729,6 +741,18 @@ describe('ChapterQueries', () => { expect(result).toHaveLength(2); expect(result.every(c => c.isDownloaded === false)).toBe(true); }); + + it('returns chapters in reading order', async () => { + const testDb = getTestDb(); + const novelId = await insertTestNovel(testDb, { inLibrary: true }); + await insertTestChapter(testDb, novelId, { position: 3 }); + await insertTestChapter(testDb, novelId, { position: 1 }); + await insertTestChapter(testDb, novelId, { position: 2 }); + + const result = await getAllUndownloadedChapters(novelId); + + expect(result.map(chapter => chapter.position)).toEqual([1, 2, 3]); + }); }); describe('getAllUndownloadedAndUnreadChapters', () => { @@ -754,6 +778,17 @@ describe('ChapterQueries', () => { expect(result[0].isDownloaded).toBe(false); expect(result[0].unread).toBe(true); }); + + it('returns unread chapters in reading order', async () => { + const testDb = getTestDb(); + const novelId = await insertTestNovel(testDb, { inLibrary: true }); + await insertTestChapter(testDb, novelId, { position: 5, unread: true }); + await insertTestChapter(testDb, novelId, { position: 2, unread: true }); + + const result = await getAllUndownloadedAndUnreadChapters(novelId); + + expect(result.map(chapter => chapter.position)).toEqual([2, 5]); + }); }); describe('getChapter', () => { diff --git a/src/hooks/persisted/useDownload.ts b/src/hooks/persisted/useDownload.ts index 830e41fa..1a3d0eca 100644 --- a/src/hooks/persisted/useDownload.ts +++ b/src/hooks/persisted/useDownload.ts @@ -20,7 +20,10 @@ export default function useDownload() { const downloadQueue = useMemo( () => queue?.filter(t => t.task?.name === 'DOWNLOAD_CHAPTER') || [], [queue], - ) as { task: DownloadChapterTask; meta: BackgroundTaskMetadata }[]; + ) as (QueuedBackgroundTask & { + task: DownloadChapterTask; + meta: BackgroundTaskMetadata; + })[]; const downloadingChapterIds = useMemo( () => diff --git a/src/screens/library/hooks/useLibrary.ts b/src/screens/library/hooks/useLibrary.ts index ff968ece..7662d7e1 100644 --- a/src/screens/library/hooks/useLibrary.ts +++ b/src/screens/library/hooks/useLibrary.ts @@ -12,6 +12,7 @@ import { switchNovelToLibraryQuery } from '@database/queries/NovelQueries'; import { BACKGROUND_TASKS_STORE_KEY, BackgroundTask, + getDownloadProgressKey, QueuedBackgroundTask, } from '@services/backgroundTasks'; import { useMMKVObject } from 'react-native-mmkv'; @@ -120,6 +121,30 @@ export const useLibrary = (): UseLibraryReturnType => { const [taskQueue] = useMMKVObject<(BackgroundTask | QueuedBackgroundTask)[]>( BACKGROUND_TASKS_STORE_KEY, ); + const downloadProgressKey = useMemo( + () => getDownloadProgressKey(taskQueue), + [taskQueue], + ); + const previousDownloadProgressKeyRef = useRef(downloadProgressKey); + const hadDownloadTasksRef = useRef(downloadProgressKey.length > 0); + + useEffect(() => { + if (downloadProgressKey.length > 0) { + if ( + hadDownloadTasksRef.current && + previousDownloadProgressKeyRef.current !== downloadProgressKey + ) { + void getLibrary(); + } + hadDownloadTasksRef.current = true; + } else if (hadDownloadTasksRef.current) { + hadDownloadTasksRef.current = false; + void getLibrary(); + } + + previousDownloadProgressKeyRef.current = downloadProgressKey; + }, [downloadProgressKey, getLibrary]); + const restoreTasksCount = useMemo( () => taskQueue?.filter(t => { diff --git a/src/screens/more/DownloadsScreen.tsx b/src/screens/more/DownloadsScreen.tsx index 5a53dc47..7f92cbdc 100644 --- a/src/screens/more/DownloadsScreen.tsx +++ b/src/screens/more/DownloadsScreen.tsx @@ -130,10 +130,20 @@ const Downloads = ({ navigation }: DownloadsScreenProps) => { { - deleteDownloads(chapters); - setChapters([]); - hideDialog(); + onSubmit={async () => { + try { + await deleteDownloads(chapters); + setChapters([]); + } catch (error) { + showToast( + error instanceof Error + ? error.message + : getString('novelScreen.deleteChapterError'), + ); + await getChapters(); + } finally { + hideDialog(); + } }} /> diff --git a/src/screens/more/components/RemoveDownloadsDialog.tsx b/src/screens/more/components/RemoveDownloadsDialog.tsx index 9a41da01..ecc62491 100644 --- a/src/screens/more/components/RemoveDownloadsDialog.tsx +++ b/src/screens/more/components/RemoveDownloadsDialog.tsx @@ -4,7 +4,7 @@ import { getString } from '@i18n/translations'; interface RemoveDownloadsDialogProps { dialogVisible: boolean; hideDialog: () => void; - onSubmit: () => void; + onSubmit: () => void | Promise; } const RemoveDownloadsDialog = ({ diff --git a/src/screens/novel/components/NovelScreenList.tsx b/src/screens/novel/components/NovelScreenList.tsx index 4e84f511..46058fb2 100644 --- a/src/screens/novel/components/NovelScreenList.tsx +++ b/src/screens/novel/components/NovelScreenList.tsx @@ -34,6 +34,7 @@ import { useSaveNovelCover } from '../hooks/useSaveNovelCover'; import { NovelScreenProps } from '@navigators/types'; import { useDownloadReconciliation } from '../hooks/useDownloadReconciliation'; import NovelFloatingActions from './NovelFloatingActions'; +import { getDownloadProgressKey } from '@services/backgroundTasks/taskDefinitions'; type NovelScreenListProps = { headerOpacity: SharedValue; @@ -103,14 +104,29 @@ const NovelScreenList = ({ const theme = useTheme(); const { top: topInset, bottom: bottomInset } = useSafeAreaInsets(); - const { downloadingChapterIds, downloadingNovelIds, downloadChapter } = - useDownload(); + const { + downloadQueue, + downloadingChapterIds, + downloadingNovelIds, + downloadChapter, + } = useDownload(); - // Queue removal can mean success, failure, or cancellation. Reconcile once - // after this novel's queue settles instead of reloading for every task. + // Reconcile as chapters complete and once more when the queue settles so + // downloaded state remains accurate after success, failure, or cancellation. const isNovelDownloading = novel.id !== 'NO_ID' && downloadingNovelIds.has(novel.id); - useDownloadReconciliation(isNovelDownloading, getChapters); + const novelDownloadProgressKey = useMemo( + () => + novel.id === 'NO_ID' + ? '' + : getDownloadProgressKey(downloadQueue, novel.id), + [downloadQueue, novel.id], + ); + useDownloadReconciliation( + isNovelDownloading, + novelDownloadProgressKey, + getChapters, + ); const [isFabExtended, setIsFabExtended] = useState(true); const [showScrollToTop, setShowScrollToTop] = useState(false); diff --git a/src/screens/novel/components/__tests__/NovelScreenList.test.tsx b/src/screens/novel/components/__tests__/NovelScreenList.test.tsx index 7b2cb4f5..b66ded04 100644 --- a/src/screens/novel/components/__tests__/NovelScreenList.test.tsx +++ b/src/screens/novel/components/__tests__/NovelScreenList.test.tsx @@ -6,6 +6,7 @@ const mockUseNovelActions = jest.fn(); const mockDownloadChapter = jest.fn(); let mockDownloadingChapterIds = new Set(); let mockDownloadingNovelIds = new Set(); +let mockDownloadQueue: any[] = []; jest.mock('../../NovelContext', () => ({ useNovelValue: (key: string) => mockUseNovelValue(key), @@ -20,6 +21,7 @@ jest.mock('@hooks/persisted', () => ({ refreshNovelMetadata: false, }), useDownload: () => ({ + downloadQueue: mockDownloadQueue, downloadingChapterIds: mockDownloadingChapterIds, downloadingNovelIds: mockDownloadingNovelIds, downloadChapter: mockDownloadChapter, @@ -283,6 +285,7 @@ describe('NovelScreenList (task 12 context boundary cutover)', () => { jest.clearAllMocks(); mockDownloadingChapterIds = new Set(); mockDownloadingNovelIds = new Set(); + mockDownloadQueue = []; }); it('uses novelStore selector actions', () => { @@ -348,6 +351,51 @@ describe('NovelScreenList (task 12 context boundary cutover)', () => { expect(store.state.updateChapter).not.toHaveBeenCalled(); }); + it('reconciles chapter state as a novel download makes progress', () => { + const store = createStore(); + mockDownloadingNovelIds = new Set([baseNovel.id]); + mockDownloadQueue = [ + { + id: 'download-1', + task: { + name: 'DOWNLOAD_CHAPTER', + data: { novelId: baseNovel.id, chapters: [] }, + }, + state: 'running', + meta: { progress: 0, isRunning: true }, + }, + ]; + wireStoreSelectors(store); + + const view = renderList(); + expect(store.state.getChapters).not.toHaveBeenCalled(); + + mockDownloadQueue = [ + { + ...mockDownloadQueue[0], + meta: { progress: 0.5, isRunning: true }, + }, + ]; + view.rerender( + , + ); + + expect(store.state.getChapters).toHaveBeenCalledTimes(1); + }); + it('uses selector-backed page navigation action from novelStore', () => { const store = createStore(); wireStoreSelectors(store); diff --git a/src/screens/novel/hooks/useDownloadReconciliation.ts b/src/screens/novel/hooks/useDownloadReconciliation.ts index 1764fd01..816e2aa3 100644 --- a/src/screens/novel/hooks/useDownloadReconciliation.ts +++ b/src/screens/novel/hooks/useDownloadReconciliation.ts @@ -2,19 +2,29 @@ import { useEffect, useRef } from 'react'; export const useDownloadReconciliation = ( isNovelDownloading: boolean, + downloadProgressKey: string, reloadChapters: () => Promise, ) => { const hadNovelDownloadsRef = useRef(false); + const previousProgressKeyRef = useRef(''); useEffect(() => { if (isNovelDownloading) { + if ( + hadNovelDownloadsRef.current && + previousProgressKeyRef.current !== downloadProgressKey + ) { + void reloadChapters(); + } hadNovelDownloadsRef.current = true; + previousProgressKeyRef.current = downloadProgressKey; return; } if (hadNovelDownloadsRef.current) { hadNovelDownloadsRef.current = false; + previousProgressKeyRef.current = ''; void reloadChapters(); } - }, [isNovelDownloading, reloadChapters]); + }, [downloadProgressKey, isNovelDownloading, reloadChapters]); }; diff --git a/src/services/backgroundTasks/__tests__/taskDefinitions.test.ts b/src/services/backgroundTasks/__tests__/taskDefinitions.test.ts index e8a71a3e..43ba17f3 100644 --- a/src/services/backgroundTasks/__tests__/taskDefinitions.test.ts +++ b/src/services/backgroundTasks/__tests__/taskDefinitions.test.ts @@ -6,6 +6,7 @@ import { fromNativeTaskRecord, getBackgroundTaskQueueName, getBackgroundTaskTitle, + getDownloadProgressKey, willTaskWaitInQueue, } from '../taskDefinitions'; @@ -153,4 +154,37 @@ describe('background task definitions', () => { }, }); }); + + it('derives progress keys only for the requested novel downloads', () => { + const createDownload = ( + id: string, + novelId: number, + progress?: number, + ) => ({ + id, + task: { + name: 'DOWNLOAD_CHAPTER' as const, + data: { + novelName: `Novel ${novelId}`, + novelId, + chapters: [{ chapterId: novelId, chapterName: 'Chapter 1' }], + }, + }, + state: 'running' as const, + meta: { + name: `Novel ${novelId}`, + isRunning: true, + progress, + progressText: 'Chapter 1', + }, + }); + const tasks: (ReturnType | BackgroundTask)[] = [ + { name: 'UPDATE_LIBRARY' }, + createDownload('first', 1, 0.5), + createDownload('second', 2), + ]; + + expect(getDownloadProgressKey(tasks, 1)).toBe('first:running:0.5'); + expect(getDownloadProgressKey(tasks, 2)).toBe('second:running:pending'); + }); }); diff --git a/src/services/backgroundTasks/index.ts b/src/services/backgroundTasks/index.ts index 4f180c2d..151ec94e 100644 --- a/src/services/backgroundTasks/index.ts +++ b/src/services/backgroundTasks/index.ts @@ -9,6 +9,7 @@ export { configureAutomaticBackups, } from './automaticBackupSchedule'; export { runHeadlessBackgroundTask } from './headlessTask'; +export { getDownloadProgressKey } from './taskDefinitions'; export type { BackgroundTask, BackgroundTaskEnqueuer, diff --git a/src/services/backgroundTasks/taskDefinitions.ts b/src/services/backgroundTasks/taskDefinitions.ts index f63c15d7..ba86e457 100644 --- a/src/services/backgroundTasks/taskDefinitions.ts +++ b/src/services/backgroundTasks/taskDefinitions.ts @@ -25,6 +25,25 @@ const MAX_CONCURRENT_DOWNLOADS = 3; export const allowsDuplicateTask = (task: BackgroundTask) => MULTIPLICABLE_TASKS.includes(task.name); +export const getDownloadProgressKey = ( + tasks: (QueuedBackgroundTask | BackgroundTask)[] | undefined, + novelId?: number, +) => + (tasks ?? []) + .filter((task): task is QueuedBackgroundTask => 'task' in task) + .filter( + task => + task.task.name === 'DOWNLOAD_CHAPTER' && + (novelId === undefined || task.task.data.novelId === novelId), + ) + .map( + task => + `${task.id}:${task.state ?? 'queued'}:${ + task.meta.progress ?? 'pending' + }`, + ) + .join('|'); + export const getBackgroundTaskQueueName = (task: BackgroundTask) => task.name === 'DOWNLOAD_CHAPTER' ? `${BACKGROUND_TASK_QUEUE_PREFIX}:download:${