Conversation
…nd pnpm-lock.yaml
…atabase initialization logic. Change drizzle-orm version to 0.45.1 and adjust related database queries for improved error handling and async operations. Clean up unused code and update types for better type safety.
📝 WalkthroughWalkthroughThis PR replaces raw Expo SQLite usage with Drizzle ORM + op-sqlite, adds drizzle-kit migrations and schema, introduces a DbManager with task queue and reactive queries, migrates all query modules to ORM (async), adds extensive in-memory DB tests, and updates build/test tooling to support .sql imports and Jest. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (UI)
participant InitHook as useInitDatabase
participant DrizzleKit as drizzle-kit
participant DbManager as DbManager (Drizzle)
participant OpSqlite as op-sqlite
App->>InitHook: call useInitDatabase()
InitHook->>DrizzleKit: run migrations
DrizzleKit->>OpSqlite: execute SQL migration statements
OpSqlite-->>DrizzleKit: migration results
InitHook->>DbManager: runDatabaseBootstrap (pragmas, triggers, seed)
DbManager->>OpSqlite: PRAGMA / CREATE TRIGGER / INSERT defaults
OpSqlite-->>DbManager: success
DbManager-->>InitHook: bootstrap complete
InitHook-->>App: state.success = true
App->>DbManager: request data (e.g., getChapters)
DbManager->>OpSqlite: execute query via Drizzle
OpSqlite-->>DbManager: rows
DbManager-->>App: return results (+ reactive subscriptions)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/screens/settings/SettingsTrackerScreen.tsx (1)
280-291:⚠️ Potential issue | 🟡 MinorMissing error handling on the MyAnimeList revalidate handler — will produce an unhandled async rejection.
mangaUpdatesTracker.revalidate(line 90 ofmangaUpdates.ts) throws a plain string'Session expired, please re-authenticate'when the session is invalid. TheonPressasync callback at lines 280-291 has notry-catch, so this rejection surfaces as an unhandled promise rejection with zero user feedback and potentially a crash in Hermes.The Kitsu handler immediately below (lines 298-313) shows the correct pattern: wrap in
try/catch, show a toast, and callremoveTracker. Apply the same treatment here:🐛 Proposed fix
onPress={async () => { const trackerAuth = getTrackerAuth('MyAnimeList'); const revalidate = getTracker('MyAnimeList')?.revalidate; if (revalidate && trackerAuth) { - const auth = await revalidate(trackerAuth.auth); - setTracker('MyAnimeList', auth); + try { + const auth = await revalidate(trackerAuth.auth); + setTracker('MyAnimeList', auth); + } catch { + showToast( + 'Failed to refresh MyAnimeList session. Please log in again.', + ); + removeTracker('MyAnimeList'); + } } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/settings/SettingsTrackerScreen.tsx` around lines 280 - 291, The MyAnimeList revalidate call inside the onPress handler (getTrackerAuth('MyAnimeList') / getTracker('MyAnimeList')?.revalidate / setTracker) must be wrapped in a try/catch like the Kitsu handler: await revalidate(trackerAuth.auth) inside try, setTracker on success, and in catch show a user-facing toast with the error (handle plain string errors) and call removeTracker('MyAnimeList') when the error indicates session expiration; ensure no unhandled promise rejections by awaiting and catching any thrown value.src/screens/Categories/components/AddCategoryModal.tsx (1)
67-78:⚠️ Potential issue | 🟠 Major
updateCategoryis not awaited —finalize()executes before the edit is persisted, causing a race condition.
updateCategory(line 72) is an async function returningPromise<void>, but it's not awaited. Meanwhile,createCategory(line 74) is correctly awaited. This inconsistency meansfinalize()runs immediately afterupdateCategoryis called but before the database write completes. As a result,onSuccess()triggers a parent screen refresh against stale data, and the modal closes before the category edit is persisted.🐛 Proposed fix
- if (isEditMode && category) { - updateCategory(category?.id, categoryName); - } else { - await createCategory(categoryName); - } + if (isEditMode && category) { + await updateCategory(category?.id, categoryName); + } else { + await createCategory(categoryName); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Categories/components/AddCategoryModal.tsx` around lines 67 - 78, The edit flow calls updateCategory(category?.id, categoryName) without awaiting it, so finalize() runs before the edit persists; change the onPress handler to await updateCategory when isEditMode is true (matching the existing await for createCategory) so finalize() executes only after the promise resolves; locate the logic around isEditMode, category, updateCategory, createCategory, categoryName and ensure updateCategory is awaited and any thrown errors are propagated/handled consistently before calling finalize().App.tsx (1)
66-109:⚠️ Potential issue | 🔴 CriticalAdd a loading guard to prevent
Mainfrom rendering until database migration completes.The component tree renders immediately even while
state.successisfalseandstate.errorisundefined(i.e., migration in progress). Although a task queue exists for write operations, all read operations (select,selectDistinct,get,all, etc.) bypass it and execute directly against the drizzle instance. IfMainor its children issue any DB read queries before migration finishes, they will fail or return stale/missing data.Add a guard to return a loading indicator while
!state.success && !state.error:if (!state.success && !state.error) { return null; // or a loading indicator }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@App.tsx` around lines 66 - 109, Add a render guard in the App component to prevent Main from mounting while migration is in progress: when useInitDatabase() returns a state where state.success is false and state.error is undefined (i.e., !state.success && !state.error), return a loading placeholder (or null) instead of proceeding to the Suspense/GestureHandlerRootView tree so no component (including Main) can perform DB reads before migration completes.src/screens/Categories/components/CategoryCard.tsx (1)
73-74:⚠️ Potential issue | 🟡 Minor
disabledcondition onTextis inconsistent with all other guards — category ID 1 can still trigger the edit modal.Every other control in this component was updated from
category.id === 2tocategory.id <= 2, but theTextdisabledprop was missed. As a result, tapping the category name for the first system category (ID 1) still opensAddCategoryModal.🐛 Proposed fix
- disabled={category.id === 2} + disabled={category.id <= 2}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Categories/components/CategoryCard.tsx` around lines 73 - 74, The Text component's disabled prop in CategoryCard is using the old check (category.id === 2) causing category ID 1 to remain interactive; update the disabled condition to match other guards (use category.id <= 2) so the category name cannot open AddCategoryModal for system categories, and verify the onPress/open handler (AddCategoryModal trigger) respects this updated disabled state.src/services/backup/utils.ts (1)
219-231:⚠️ Potential issue | 🟠 MajorAdd
awaitto_restoreCategorycall — Promise rejection is silently ignored.
_restoreCategoryis an async function that returnsPromise<void>. Calling it withoutawaitcauses three critical issues:
- Promise rejections from DB write failures are not caught by the
try/catchblock and propagate as unhandled rejections.categoryCount++executes immediately without waiting for the operation to complete, producing incorrect restore counts.- All category writes execute in parallel without back-pressure, bypassing the task-queue serialization that the DB manager enforces.
Fix
- _restoreCategory(category); + await _restoreCategory(category); categoryCount++;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/backup/utils.ts` around lines 219 - 231, The loop currently calls the async helper _restoreCategory without awaiting it, which causes unhandled rejections, incorrect counting, and parallel execution; change the loop so you await _restoreCategory(category) inside the try block, increment categoryCount only after the awaited call succeeds, and keep the catch to increment failedCategoryCount and showToast (using getString) so promise rejections are properly caught and category writes run serially.src/screens/settings/SettingsRepositoryScreen/SettingsRepositoryScreen.tsx (1)
70-74:⚠️ Potential issue | 🟡 MinorUnhandled async call in
useEffect— promise rejection will be unhandled.
upsertRepository(params.url)is an async function called withoutawaitor.catch(). If it throws (e.g., DB write fails, URL validation throws), the rejection goes unhandled.Proposed fix
useEffect(() => { if (params?.url) { - upsertRepository(params.url); + upsertRepository(params.url).catch(() => {}); } }, [params, upsertRepository]);Or wrap in an IIFE with proper error handling/toast.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/settings/SettingsRepositoryScreen/SettingsRepositoryScreen.tsx` around lines 70 - 74, The useEffect currently calls the async upsertRepository(params.url) without awaiting or catching rejections; update the effect to invoke an async IIFE (or call upsertRepository().catch(...)) and wrap the await upsertRepository(params.url) in try/catch so any thrown error is handled (e.g., show a user toast/error message or console.error). Make the change inside the same effect that references params?.url and upsertRepository so rejections from upsertRepository are properly caught and reported.src/hooks/persisted/useNovel.ts (1)
272-276:⚠️ Potential issue | 🟡 MinorInconsistent filter reference: uses
novelSettings.filterinstead ofsettingsFilter.Line 274 passes
novelSettings.filterdirectly to_getFirstUnreadChapter, while the rest ofgetChapters(line 239) andrefreshChapters(line 550) usesettingsFilter. The same inconsistency appears inloadUpToBatchat line 342 and its dependency array at line 358. This bypasses the memoization added in lines 111-114 and may cause unnecessary re-renders or stale closures.Proposed fix
const unread = await _getFirstUnreadChapter( novel.id, - novelSettings.filter, + settingsFilter, page, );Also apply the same change in
loadUpToBatch(line 342) and update its dependency array (line 358) fromnovelSettings.filtertosettingsFilter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovel.ts` around lines 272 - 276, Replace direct uses of novelSettings.filter with the memoized settingsFilter when calling _getFirstUnreadChapter and in loadUpToBatch and its dependency array to ensure you use the memoized value; specifically, change the argument passed to _getFirstUnreadChapter from novelSettings.filter to settingsFilter where used inside getChapters, refreshChapters and loadUpToBatch, and update loadUpToBatch's dependency array to reference settingsFilter instead of novelSettings.filter so memoization and stable closures work as intended.
🟠 Major comments (17)
src/screens/reader/hooks/useChapter.ts-132-133 (1)
132-133:⚠️ Potential issue | 🟠 MajorChange fallback from
''to'1'to match how chapters are actually stored in the database.When inserting chapters without an explicit
pagevalue,insertChaptersdefaults to'1'(line 48 of ChapterQueries.ts:page: c.page || '1'). All chapters in the database havepage ≥ '1'.However, the calls to
getNextChapterandgetPrevChapterusechap.page ?? ''. Whenchap.pageis null, this passes an empty string, whichcastInt('')converts to0in SQLite. Since no chapters havepage = 0, the query conditions fail:
- getPrevChapter:
page = 0 AND position < XORpage < 0→ matches nothing, breaks previous-chapter navigation- getNextChapter:
page = 0 AND position > XORpage > 0 AND position = 0→ only the second condition matches, returning the first chapter of page 1 (often incorrect)The fallback should be
'1'to match the default used during insertion and the actual stored values.Proposed fix
- getNextChapter(chap.novelId, chap.position!, chap.page ?? ''), - getPrevChapter(chap.novelId, chap.position!, chap.page ?? ''), + getNextChapter(chap.novelId, chap.position!, chap.page ?? '1'), + getPrevChapter(chap.novelId, chap.position!, chap.page ?? '1'),Also update the prefetch calls for consistency (lines 165 and 187):
- chap.page ?? '', + chap.page ?? '1',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/reader/hooks/useChapter.ts` around lines 132 - 133, Calls to getNextChapter and getPrevChapter pass chap.page ?? '' which becomes page=0 in SQLite; change the fallback to '1' so it matches how chapters are inserted (page defaults to '1'). Update the two places where getNextChapter(chap.novelId, chap.position!, chap.page ?? '') and getPrevChapter(chap.novelId, chap.position!, chap.page ?? '') are called to use chap.page ?? '1', and make the same change for the nearby prefetch calls that currently use chap.page ?? '' so all navigation/prefetch logic uses '1' as the default page.package.json-70-70 (1)
70-70:⚠️ Potential issue | 🟠 MajorUsing pre-release Drizzle ORM beta — major version gap from stable.
Both
drizzle-ormanddrizzle-kitare pinned to1.0.0-beta.13-f728631, while the latest stable releases are0.45.1and0.31.9respectively. This represents a jump across the entire 0.x stable series to a 1.0.0 pre-release. Pre-release versions can introduce breaking changes and lack stability guarantees. Ensure your team is actively monitoring the Drizzle 1.0.0 release timeline and prepared for potential migration work when stable is released.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 70, The package.json currently pins "drizzle-orm" and "drizzle-kit" to the 1.0.0-beta.13-f728631 pre-release; change these dependencies to stable versions or add explicit rationale: either (A) roll back to the latest stable versions ("drizzle-orm": "0.45.1", "drizzle-kit": "0.31.9") in package.json, or (B) add a comment in the repository (README or a dependencies.md) documenting why the 1.0.0-beta is used, the risks, and who is responsible for monitoring upstream releases, and ensure the CI or upgrade plan references the 1.0.0-beta.13-f728631 string so reviewers can find the usage.src/screens/novel/components/NovelScreenList.tsx-272-274 (1)
272-274:⚠️ Potential issue | 🟠 MajorFix
updateNovelPagecall to passnovel.nameinstead ofnovel.pathas the second argument.The function signature expects
novelNameas the second parameter andnovelPathas the third, but the current call passesnovel.pathfor both. Change the second argument fromnovel.pathtonovel.name:Corrected call
updateNovelPage(pluginId, novel.name, novel.path, novel.id, page, { downloadNewChapters, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/novel/components/NovelScreenList.tsx` around lines 272 - 274, The call to updateNovelPage uses novel.path for both novelName and novelPath; change the second argument to novel.name so the call becomes updateNovelPage(pluginId, novel.name, novel.path, novel.id, page, { downloadNewChapters }) — update the invocation where updateNovelPage is called to pass novel.name as the second parameter and keep novel.path as the third, leaving novel.id, page and the options object unchanged.src/database/queryStrings/populate.ts-6-9 (1)
6-9:⚠️ Potential issue | 🟠 Major
createCategoryDefaultQueryis evaluated at import time —getStringmay return untranslated keys.The template literal is evaluated when the module is first imported, which means
getString('categories.default')andgetString('categories.local')run at that point. If this module is imported (directly or transitively) before the i18n system is fully initialized, the embedded strings will be raw keys or empty, and theINSERT OR IGNOREwill permanently seed incorrect category names.Consider making this a function so the query is built lazily:
Proposed fix
-// if category with id = 2 exists, nothing in db.ts file is executed -export const createCategoryDefaultQuery = ` -INSERT OR IGNORE INTO Category (id, name, sort) VALUES - (1, '${escapeSqlString(getString('categories.default'))}', 1), - (2, '${escapeSqlString(getString('categories.local'))}', 2) -`; +// if category with id = 2 exists, nothing in db.ts file is executed +export const createCategoryDefaultQuery = () => ` +INSERT OR IGNORE INTO Category (id, name, sort) VALUES + (1, '${escapeSqlString(getString('categories.default'))}', 1), + (2, '${escapeSqlString(getString('categories.local'))}', 2) +`;#!/bin/bash # Check how createCategoryDefaultQuery is consumed — does the call-site # already guard against early import, or would changing to a function break callers? rg -n "createCategoryDefaultQuery" --type=ts -C 3#!/bin/bash # Check if i18n / getString initialization happens before any db init code rg -n "i18n\.(init|configure|start|setup|locale)" --type=ts -C 2 rg -n "getString\(" src/database/ --type=ts -l🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queryStrings/populate.ts` around lines 6 - 9, createCategoryDefaultQuery is built at module import time so getString(...) runs too early; change the exported const into a function (e.g. export function createCategoryDefaultQuery()) that constructs and returns the template string using escapeSqlString(getString('categories.default')) and escapeSqlString(getString('categories.local')) so the i18n lookups happen lazily at call time; update all call sites that reference createCategoryDefaultQuery to invoke it as a function and keep the same string format (including INSERT OR IGNORE INTO Category ...) so seeding uses the correct translated values.App.tsx-91-93 (1)
91-93:⚠️ Potential issue | 🟠 Major
resetErroris a no-op — user has no recovery path from a DB init error.The
ErrorFallbackcomponent renders a "Restart the application" button wired toresetError. Passing() => nullmakes that button non-functional, leaving the user stuck on the error screen with no way to recover. TheuseInitDatabasehook has no reset mechanism, and once an error occurs, there is no way to retry the database migration.Implement proper error recovery: either expose a reset/retry function from
useInitDatabasethat can be called viaresetError, or provide an app restart capability (noreact-native-restartlibrary is currently available in the project).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@App.tsx` around lines 91 - 93, The ErrorFallback's reset button is a no-op because App.tsx passes resetError={() => null}; update useInitDatabase to expose a reset/retry function (e.g., retryInit or resetInit) that clears the error state and re-runs the DB init/migration logic, then pass that function into ErrorFallback as resetError; specifically modify the hook (useInitDatabase) to return an object like { state, retryInit } and in App.tsx replace resetError={() => null} with resetError={retryInit} so the user can retry DB initialization from the UI.src/screens/settings/SettingsRepositoryScreen/components/DeleteRepositoryModal.tsx-39-43 (1)
39-43:⚠️ Potential issue | 🟠 MajorAdd error handling to prevent the modal from getting stuck on DB failure.
If
deleteRepositoryByIdthrows (e.g., DB write failure), neithercloseModal()noronSuccess()will be invoked, leaving the modal open with no feedback to the user.🛡️ Proposed fix
- onPress={async () => { - await deleteRepositoryById(repository.id); - closeModal(); - onSuccess(); - }} + onPress={async () => { + try { + await deleteRepositoryById(repository.id); + closeModal(); + onSuccess(); + } catch (e) { + // surface error to user (e.g. showToast) + console.error('Failed to delete repository', e); + } + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/settings/SettingsRepositoryScreen/components/DeleteRepositoryModal.tsx` around lines 39 - 43, Wrap the async deleteRepositoryById call in try/catch and ensure closeModal() is always called (use finally) so the modal can't get stuck; on success call onSuccess() in the try block, and in the catch surface an error to the user (e.g., show an Alert/toast) and log the error so failures are visible. Target the onPress handler where deleteRepositoryById(repository.id), closeModal(), and onSuccess() are referenced and modify flow to try -> await deleteRepositoryById(...); onSuccess(); catch -> show user-facing error and log; finally -> closeModal().src/hooks/persisted/useSettings.ts-55-55 (1)
55-55:⚠️ Potential issue | 🟠 MajorMissing MMKV migration for
defaultChapterSortformat change.The stored value format changed from raw SQL strings (
'ORDER BY id ASC','ORDER BY chapterId ASC') toChapterOrderKeyenum keys ('positionAsc','positionDesc'). Existing users upgrading will have old string values deserialized from MMKV, which no longer match valid keys. Code comparing against'positionAsc'(e.g., DefaultChapterSortModal.tsx, SettingsGeneralScreen.tsx) will silently fail for these users, breaking chapter sort functionality.A migration guard is needed in
useAppSettingsto convert legacy values to the new format. Consider the proposed fix:🔧 Proposed fix
+const LEGACY_SORT_MAP: Record<string, ChapterOrderKey> = { + 'ORDER BY id ASC': 'positionAsc', + 'ORDER BY id DESC': 'positionDesc', + 'ORDER BY chapterId ASC': 'positionAsc', + 'ORDER BY chapterId DESC': 'positionDesc', +}; + export const useAppSettings = () => { const [appSettings = initialAppSettings, setSettings] = useMMKVObject<AppSettings>(APP_SETTINGS); + // One-time migration: convert legacy SQL sort literals to ChapterOrderKey + const migratedSettings = React.useMemo(() => { + const legacyMapped = LEGACY_SORT_MAP[appSettings.defaultChapterSort as string]; + if (legacyMapped) { + const migrated = { ...appSettings, defaultChapterSort: legacyMapped }; + setSettings(migrated); + return migrated; + } + return appSettings; + }, [appSettings, setSettings]); const setAppSettings = (values: Partial<AppSettings>) => - setSettings({ ...appSettings, ...values }); + setSettings({ ...migratedSettings, ...values }); return { - ...appSettings, + ...migratedSettings, setAppSettings, }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useSettings.ts` at line 55, The persisted defaultChapterSort value may be an old SQL string; update the migration in the useAppSettings hook (in src/hooks/persisted/useSettings.ts) to detect legacy values like 'ORDER BY id ASC' / 'ORDER BY chapterId ASC' / 'ORDER BY id DESC' etc., map them to the corresponding ChapterOrderKey enum keys (e.g., 'positionAsc', 'positionDesc', etc.), and overwrite the stored MMKV value so subsequent reads return the new enum key; implement this guard immediately after reading the raw persisted defaultChapterSort (before any comparisons elsewhere) and use the same MMKV get/set methods used by the hook to persist the normalized value.src/database/queries/__tests__/testDbManager.ts-121-130 (1)
121-130:⚠️ Potential issue | 🟠 Major
write()in test implementation doesn't wrap in actual transactions — test isolation relies onclearAllTables()instead of rollback guarantees.
write()passesdrizzleDbdirectly without wrapping in a transaction. Unlike the productionDbManager.write()which usesdb.transaction(async tx => {...}), the test version has no rollback mechanism. While the comment claims better-sqlite3 can't handle async transactions, the production code demonstrates this is not the issue—it successfully wraps async operations in transactions.This creates two problems:
- Test contract violation:
IDbManager.write()documents atomicity guarantees, but the test implementation doesn't provide them. If a write() call fails partway, changes persist unsafely.- Reliance on
clearAllTables()instead of rollback: Tests work around this by callingclearAllTables()inbeforeEach, which clears the in-memory database between tests. However, this masks the missing atomicity—a test that exercises multi-step writes with partial failure may produce misleading results that wouldn't occur in production.Wrap
write()in an actual transaction similar to the production implementation:await this.db.transaction(async tx => fn(tx)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/__tests__/testDbManager.ts` around lines 121 - 130, The test DbManager.write() implementation currently calls fn(drizzleDb) directly, violating IDbManager.write()'s atomicity guarantee and relying on clearAllTables() for isolation; change write() to wrap the call in an actual transaction (use the same pattern as production: call db.transaction(async tx => fn(tx)) so fn receives the transactional TransactionParameter), and ensure transaction() delegates to write() unchanged; reference drizzleDb, write(), transaction(), IDbManager.write(), clearAllTables() when locating the code to update.src/screens/novel/components/NovelBottomSheet.tsx-49-55 (1)
49-55:⚠️ Potential issue | 🟠 MajorFilter key
'read'is semantically inverted relative to the "Unread" label.The filter key
'read'maps to the SQL conditionunread=0(showing chapters that are read), but the Checkbox label displays "Unread". When users check this box, they expect to filter for unread chapters but instead get read chapters shown.To fix this, either:
- Change the filter key from
'read'to'unread'and adjust the SQL mapping accordingly, OR- Change the label to "Read" instead of "Unread"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/novel/components/NovelBottomSheet.tsx` around lines 49 - 55, The Checkbox in NovelBottomSheet uses the filter key 'read' while the label uses the "Unread" string, causing an inverted filter; update the filter key to 'unread' (replace usages of 'read' passed to getChapterFilterState and cycleChapterFilter in the Checkbox) and update the underlying SQL/mapper that currently interprets 'read' as unread=0 so that 'unread' maps to the correct SQL condition (e.g., unread=1), ensuring getChapterFilterState('unread') and cycleChapterFilter('unread') reflect the correct semantics for the "novelScreen.bottomSheet.filters.unread" label.src/hooks/persisted/useNovelSettings.ts-79-86 (1)
79-86:⚠️ Potential issue | 🟠 MajorStale
ChapterFilterObject: the ref is created once but never re-synced when_filterorsetChapterFilterchange.The
if (!filterManager.current)guard ensures theChapterFilterObjectis only instantiated once, but:
- If
_filterchanges externally, the manager's internalMapstill reflects the original filter state.- The captured
setChapterFiltercallback may close over staleshowChapterTitles/_sortvalues, causing those to be overwritten with old data when the filter is cycled.Since
_filterandsetChapterFilterare listed in the effect's deps but the guard prevents re-creation, the deps array is effectively misleading.Proposed fix: re-create the filter manager when dependencies change
useEffect(() => { - if (!filterManager.current) { - filterManager.current = new ChapterFilterObject( - _filter, - setChapterFilter, - ); - } + filterManager.current = new ChapterFilterObject( + _filter, + setChapterFilter, + ); }, [_filter, setChapterFilter]);This does mean the object is recreated on each filter change, but
ChapterFilterObjectis lightweight and this ensures correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovelSettings.ts` around lines 79 - 86, The effect currently only constructs ChapterFilterObject once because of the guard (filterManager.current), which causes the manager to become stale when _filter or setChapterFilter change; remove the "if (!filterManager.current)" guard and always assign filterManager.current = new ChapterFilterObject(_filter, setChapterFilter) inside the useEffect (so it is re-created when _filter or setChapterFilter change), ensuring ChapterFilterObject, its internal Map, and the captured setChapterFilter are kept in sync with the latest props; keep the same dependency array ([_filter, setChapterFilter]) so the object is recreated on changes.src/hooks/persisted/useNovel.ts-233-234 (1)
233-234:⚠️ Potential issue | 🟠 MajorString/number type mismatch: fallback should be
'1'not1.
pagesisstring[], sopages[pageIndex]isstring | undefined. The fallback1(number) creates a type mismatch—pagewill be anumberwhen the fallback fires, but downstream calls likegetPageChaptersBatchedand_getPageChaptersexpect astring.Proposed fix
- const page = pages[pageIndex] ?? 1; + const page = pages[pageIndex] ?? '1';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovel.ts` around lines 233 - 234, In getChapters, pages is a string[] so the fallback value must be a string; change the fallback from the numeric 1 to the string '1' (or coerce via String(...)) so that page is always a string when passed to getPageChaptersBatched and _getPageChapters; update the expression around pages[pageIndex] in getChapters accordingly to ensure type consistency.src/services/updates/LibraryUpdateQueries.ts-146-151 (1)
146-151:⚠️ Potential issue | 🟠 MajorFix non-null assertion on
releaseTimeto match insert/update pattern.Line 148:
ne(chapterSchema.releaseTime, releaseTime!)uses a non-null assertion, butreleaseTimefromChapterItemis optional (releaseTime?: string). This creates an inconsistency with the insert and update operations above, which correctly usereleaseTime || null. The comparison should follow the same pattern to ensure consistent database query behavior.Proposed fix
or( ne(chapterSchema.name, name), - ne(chapterSchema.releaseTime, releaseTime!), + ne(chapterSchema.releaseTime, releaseTime || null), ne(chapterSchema.page, chapterPage), ne(chapterSchema.position, position), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/updates/LibraryUpdateQueries.ts` around lines 146 - 151, The comparison uses a non-null assertion on releaseTime (ne(chapterSchema.releaseTime, releaseTime!)) while inserts/updates use releaseTime || null; update the OR condition to compare chapterSchema.releaseTime against releaseTime || null instead of releaseTime! so it matches the insert/update pattern (ensure the same null-coalescing behavior where releaseTime may be undefined). This change should be made where ne is called in the or(...) block referencing chapterSchema.releaseTime and the incoming releaseTime from ChapterItem.src/services/migrate/migrateNovel.ts-77-97 (1)
77-97:⚠️ Potential issue | 🟠 MajorExplicit cleanup needed for source novel's chapters before deletion.
Line 96 deletes the source novel, but the
Chapterschema has no foreign key constraint orON DELETE CASCADEdefined. The source novel's chapters (stored infromChapters) remain in the database as orphaned rows.Add an explicit chapter deletion before deleting the source novel:
Proposed fix
+ await tx.delete(chapterSchema).where(eq(chapterSchema.novelId, fromNovel.id)); await tx.delete(novelSchema).where(eq(novelSchema.id, fromNovel.id));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/migrate/migrateNovel.ts` around lines 77 - 97, Before deleting the source novel (fromNovel) you must explicitly remove its chapters to avoid orphaned Chapter rows; inside the same dbManager.write transaction (where you already update novelSchema and novelCategorySchema) add a delete call using the chapter schema (e.g., tx.delete(chapterSchema).where(eq(chapterSchema.novelId, fromNovel.id))) to remove all fromChapters for fromNovel.id, then proceed to delete the novel row; keep all operations in the single transaction so the chapter deletion and novel deletion are atomic.src/services/updates/LibraryUpdateQueries.ts-44-58 (1)
44-58:⚠️ Potential issue | 🟠 MajorMissing
awaiton database operations within async transaction callback — inconsistent with async pattern and drizzle-orm/op-sqlite limitations.Lines 45, 66, and 134 do not await
.run()statements. The code usesasync txcallbacks but inconsistently awaits operations:.get()calls on lines 103 and 120 are awaited, while.run()calls are not. According to drizzle-orm documentation, op-sqlite has a known limitation where async transaction callbacks are not reliably awaited before the transaction commits.To fix this inconsistency, either:
- Add
awaitto all.run()statements (lines 45, 66, 134) to match the async callback pattern, or- Convert the transaction callback to synchronous (remove
asynckeyword and allawaitstatements), which is the officially recommended pattern for op-sqlite.Option 1: Add await to .run() calls
await dbManager.write(async tx => { - tx.update(novelSchema) + await tx.update(novelSchema) .set({...}) .where(eq(novelSchema.id, novelId)) .run(); });Apply the same
awaitto lines 66 and 134.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/updates/LibraryUpdateQueries.ts` around lines 44 - 58, The transaction callback passed to dbManager.write contains .run() calls that are not awaited (e.g., the tx.update(novelSchema).set(...).where(eq(novelSchema.id, novelId)).run() call and the other .run() calls referenced), which can cause op-sqlite transactions to commit before operations finish; either add await before each .run() inside the async tx callback (ensure you await the tx.update(...).run(), the other tx.*.run() calls at the same file, and keep the callback async), or convert the dbManager.write callback to a synchronous function (remove the async keyword and any awaits) per op-sqlite recommendation so transactions are executed synchronously.src/database/queries/CategoryQueries.ts-90-102 (1)
90-102:⚠️ Potential issue | 🟠 Major
createCategoryreturn type is unsound —.returning().get()can returnundefinedDrizzle's
.get()has return typeT | undefined, sorowcan beundefinedif the insert returns no rows. The function signaturePromise<CategoryRow>hides this, causing a potential null-dereference at every call site that doesn't guard the result.🛡️ Proposed fix
-export const createCategory = async ( - categoryName: string, -): Promise<CategoryRow> => { +export const createCategory = async ( + categoryName: string, +): Promise<CategoryRow | undefined> => { return await dbManager.write(async tx => { const categoryCount = await tx.$count(categorySchema); const row = await tx .insert(categorySchema) .values({ name: categoryName, sort: categoryCount + 1 }) .returning() .get(); return row; }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/CategoryQueries.ts` around lines 90 - 102, The createCategory function currently assumes tx.insert(...).returning().get() always returns a value; update createCategory to handle the possible undefined result from .get() by checking the returned row and throwing a clear error if it's undefined (so the function can safely keep the Promise<CategoryRow> signature), e.g. after calling .get() on the insert result in createCategory check if (!row) throw new Error("Failed to create category") otherwise return row; reference the createCategory function and the tx.insert(...).returning().get() call on categorySchema when making this change.src/database/queries/NovelQueries.ts-207-220 (1)
207-220:⚠️ Potential issue | 🟠 MajorPervasive missing
awaitontxoperations — same root cause asCategoryQueries.ts
removeNovelsFromLibrary(lines 211–218),updateNovelInfo(line 307),pickCustomNovelCover(line 338),updateNovelCategoryById(lines 353–356), the nested loops inupdateNovelCategories(lines 385–414), and all ops in_restoreNovelAndChapters(lines 429–443) calltx.update/insert/delete(…).run()withoutawait. The sibling functionswitchNovelToLibraryQueryin this same file correctly usesawait tx.update(…).run(), confirming the expectation. If.run()is async, these calls are all fire-and-forget — writes can silently not execute within the transaction.🔧 Example fix for `removeNovelsFromLibrary`
await dbManager.write(async tx => { - tx.update(novelSchema) + await tx.update(novelSchema) .set({ inLibrary: false }) .where(inArray(novelSchema.id, novelIds)) .run(); - tx.delete(novelCategorySchema) + await tx.delete(novelCategorySchema) .where(inArray(novelCategorySchema.novelId, novelIds)) .run(); });Apply
awaitconsistently before everytx.*(…).run()call. The same pattern applies toChapterQueries.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/NovelQueries.ts` around lines 207 - 220, The transaction operations in NovelQueries.ts are missing awaited .run() calls and must be made synchronous to ensure they complete within the transaction: add await before every tx.update(...).run(), tx.delete(...).run(), tx.insert(...).run() inside removeNovelsFromLibrary, updateNovelInfo, pickCustomNovelCover, updateNovelCategoryById, the nested loops in updateNovelCategories, and all ops in _restoreNovelAndChapters (mirror the pattern used in switchNovelToLibraryQuery); apply the same fix for all tx.*.run() calls in ChapterQueries.ts so no tx operation is fire-and-forget.src/database/queries/CategoryQueries.ts-187-194 (1)
187-194:⚠️ Potential issue | 🟠 MajorAdd missing
awaittotx.update().run()calls inupdateCategoryOrderInDband_restoreCategoryThe
tx.update(categorySchema).run()calls at lines 189-192 are not awaited inside thedbManager.writecallback. In drizzle-orm,.run()returns a Promise, and missingawaitallows the transaction to commit before updates are persisted. The same issue exists in_restoreCategoryat lines 234 and 240 withtx.insert().run().🔧 Proposed fix for `updateCategoryOrderInDb`
await dbManager.write(async tx => { for (const category of categories) { - tx.update(categorySchema) + await tx.update(categorySchema) .set({ sort: category.sort }) .where(eq(categorySchema.id, category.id)) .run(); } });Apply the same
awaitin_restoreCategoryat lines 234 and 240. This pattern also appears throughoutNovelQueries.tsandChapterQueries.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/CategoryQueries.ts` around lines 187 - 194, The transaction callback is not awaiting the individual query Promises so the transaction may commit before updates/inserts complete; in updateCategoryOrderInDb add await to each tx.update(categorySchema).set(...).where(...).run() call (or collect and await Promise.all for the loop) and in _restoreCategory add await before each tx.insert(...).run(); ensure the dbManager.write callback remains async and apply the same fix pattern to similar occurrences in NovelQueries.ts and ChapterQueries.ts.
| export default { | ||
| journal: { entries: [] }, | ||
| migrations: { | ||
| '20251222152612_past_mandrill': m0000, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the drizzle meta journal and config
fd "_journal.json" --type f && cat drizzle/meta/_journal.json 2>/dev/null || echo "No _journal.json found"
# Check the actual migrations.js file
cat -n drizzle/migrations.js 2>/dev/null || echo "No migrations.js found"
# Check the drizzle.config.ts driver setting
fd "drizzle.config" --type f -exec cat {} \;
# Check how migrations are invoked at runtime
fd "useMigrations\|drizzle\|migration" --type f -e ts -e tsx -e js | head -20Repository: CD-Z/lnreader
Length of output: 644
🏁 Script executed:
# Find and read drizzle.config files
find . -name "drizzle.config.*" -type f -exec cat {} \;
# Check for migration invocation in TypeScript/JavaScript files
rg -t ts -t tsx -t js "useMigrations|migrate|drizzle.*migrate" -B2 -A5 | head -100
# Also check if there's any schema definition that might give us context
rg -t ts -t tsx "schema\|Database" -B2 -A5 | head -100Repository: CD-Z/lnreader
Length of output: 291
🏁 Script executed:
# Search for migration runner, db initialization, and useMigrations
rg "useMigrations|migrate|drizzle|Database|initDb" -t js -t ts --max-count 50 -B2 -A5
# Check the schema file to understand the database setup
cat src/database/schema/index.ts 2>/dev/null || echo "Schema not found"
# Look for database initialization or setup files
fd -e ts -e js | grep -i "db\|database\|init" | head -20Repository: CD-Z/lnreader
Length of output: 50369
🏁 Script executed:
# Find database initialization code in production (not test)
rg "useInitDatabase|initDb|db\.migrate|useMigrations" -t js -A 10 | grep -v test | head -80
# Look for app initialization or root component
fd -e ts -e tsx | grep -i "app\|root\|index" | grep -v test | grep -v node_modules | head -20
# Check the db manager implementation
cat src/database/db.ts 2>/dev/null | head -100Repository: CD-Z/lnreader
Length of output: 3428
🏁 Script executed:
# Get the full useInitDatabase hook
cat -n src/database/db.ts | tail -100Repository: CD-Z/lnreader
Length of output: 3162
🌐 Web query:
drizzle-orm op-sqlite migrate empty journal entries behavior
💡 Result:
In Drizzle ORM’s OP-SQLite migrator, the migration runner is driven entirely by journal.entries:
- It iterates
journal.entriesand, for each entry, looks up the SQL text in themigrationsmap under the keym${idx.padStart(4,'0')}(e.g.m0000). If the key is missing, it throwsMissing migration: ${tag}. [1] - If
journal.entriesis empty ([]),readMigrationFiles()returns an empty migrations list, somigrate()effectively becomes a no-op (nothing to apply). [1]
What happens if migrations/journal exist but look “empty”:
- If a migration SQL string is present but empty (
"") (often caused by bundling/import issues in React Native), the code still parses it into[""](split by the breakpoint comment) and passes it along—so you can end up “running” a migration that does nothing but can still be considered in the migration flow. [1] - OP-SQLite migrations must be bundled into the app (not read from the filesystem). Drizzle’s OP-SQLite setup explicitly relies on bundling SQL (e.g., via inline-import + Metro/Babel config). If that step is wrong, it’s plausible to import
""instead of real SQL. [2] - Separately: custom migrations generated by Drizzle Kit can be intentionally empty until you fill in SQL yourself (
generate --custom). That’s expected and not an OP-SQLite-specific bug. [3]
Rule of thumb:
journal.entries = []⇒ “no migrations exist” (no-op).journal.entrieshas items but your SQL imports are""⇒ you likely have a bundling/import problem (or an intentionally empty custom migration).
Sources
[1] OP-SQLite migrator implementation (UNPKG sourcemap shows journal.entries iteration, m#### lookup, missing-migration error, and SQL split behavior) (app.unpkg.com)
[2] Drizzle OP-SQLite docs (migrations must be bundled; inline-import / Metro/Babel setup) (orm.drizzle.team)
[3] Drizzle discussion excerpt indicating empty custom migration files are expected until you write SQL (gitmemories.com)
Citations:
- 1: https://app.unpkg.com/drizzle-orm%400.31.4/files/op-sqlite/migrator.js.map?utm_source=openai
- 2: https://orm.drizzle.team/docs/connect-op-sqlite?utm_source=openai
- 3: https://gitmemories.com/drizzle-team/drizzle-orm/issues/646?utm_source=openai
Empty journal.entries prevents migration execution — the database schema will not be created at runtime.
OP-SQLite's migration runner iterates journal.entries to determine which migrations to apply. When entries: [], the migrate() function becomes a no-op and the bundled SQL for 20251222152612_past_mandrill is never invoked, leaving all tables (Category, Chapter, Novel, NovelCategory, Repository) uninitialized. Every database operation will fail.
The useInitDatabase hook calls migrate(drizzleDb, migrations) at app startup (line 126 in src/database/db.ts), which completes successfully but creates no schema. Drizzle Kit with driver: 'expo' auto-generates the migrations.js file with populated entries—regenerate it by running drizzle-kit generate.
Expected journal entry format
export default {
- journal: { entries: [] },
+ journal: {
+ entries: [
+ {
+ idx: 0,
+ when: 1703274972,
+ tag: '20251222152612_past_mandrill',
+ breakpoints: true,
+ },
+ ],
+ },
migrations: {
'20251222152612_past_mandrill': m0000,
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default { | |
| journal: { entries: [] }, | |
| migrations: { | |
| '20251222152612_past_mandrill': m0000, | |
| }, | |
| export default { | |
| journal: { | |
| entries: [ | |
| { | |
| idx: 0, | |
| when: 1703274972, | |
| tag: '20251222152612_past_mandrill', | |
| breakpoints: true, | |
| }, | |
| ], | |
| }, | |
| migrations: { | |
| '20251222152612_past_mandrill': m0000, | |
| }, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@drizzle/migrations.js` around lines 5 - 9, The migrations manifest currently
has an empty journal.entries which causes migrate(drizzleDb, migrations) (called
from useInitDatabase) to skip all bundled migrations so the schema (tables like
Category, Chapter, Novel, NovelCategory, Repository) is never created;
regenerate the migrations manifest so journal.entries contains the recorded
migration(s) (for example the entry for '20251222152612_past_mandrill' and its
metadata) by running drizzle-kit generate (with the expo driver) to produce a
proper migrations.js and replace the current object so the migrate() runner will
execute the bundled migration m0000.
| export const deleteDownloads = async ( | ||
| chapters: DownloadedChapter[], | ||
| ): Promise<void> => { | ||
| if (!chapters?.length) { | ||
| return; | ||
| } | ||
| chapters.forEach(chapter => { | ||
| deleteDownloadedFiles(chapter.pluginId, chapter.novelId, chapter.id); | ||
| }); | ||
| await dbManager.write(async tx => { | ||
| tx.update(chapterSchema).set({ isDownloaded: false }).run(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Critical: deleteDownloads update has no WHERE clause — marks ALL chapters as not downloaded
Line 199 runs tx.update(chapterSchema).set({ isDownloaded: false }).run() with no predicate. Every chapter in the database will have isDownloaded set to false regardless of what was passed to the function.
🐛 Proposed fix
await dbManager.write(async tx => {
- tx.update(chapterSchema).set({ isDownloaded: false }).run();
+ await tx
+ .update(chapterSchema)
+ .set({ isDownloaded: false })
+ .where(inArray(chapterSchema.id, chapters.map(c => c.id)))
+ .run();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const deleteDownloads = async ( | |
| chapters: DownloadedChapter[], | |
| ): Promise<void> => { | |
| if (!chapters?.length) { | |
| return; | |
| } | |
| chapters.forEach(chapter => { | |
| deleteDownloadedFiles(chapter.pluginId, chapter.novelId, chapter.id); | |
| }); | |
| await dbManager.write(async tx => { | |
| tx.update(chapterSchema).set({ isDownloaded: false }).run(); | |
| }); | |
| }; | |
| export const deleteDownloads = async ( | |
| chapters: DownloadedChapter[], | |
| ): Promise<void> => { | |
| if (!chapters?.length) { | |
| return; | |
| } | |
| chapters.forEach(chapter => { | |
| deleteDownloadedFiles(chapter.pluginId, chapter.novelId, chapter.id); | |
| }); | |
| await dbManager.write(async tx => { | |
| await tx | |
| .update(chapterSchema) | |
| .set({ isDownloaded: false }) | |
| .where(inArray(chapterSchema.id, chapters.map(c => c.id))) | |
| .run(); | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/database/queries/ChapterQueries.ts` around lines 189 - 201, The update in
deleteDownloads currently has no WHERE clause and will clear isDownloaded for
all rows; change it to update only the chapters passed in by collecting their
ids (and/or pluginId+novelId if needed) and use a predicate on
tx.update(chapterSchema).where(...) (e.g., .where(chapterSchema.id.in(ids)))
before .set({ isDownloaded: false }).run(); also ensure you gather ids from the
chapters param and use dbManager.write with that predicate; keep references to
deleteDownloads, deleteDownloadedFiles, dbManager.write, tx.update, and
chapterSchema to locate where to apply the change.
| await dbManager.write(async tx => { | ||
| tx.update(chapterSchema) | ||
| .set({ | ||
| readTime: sql`datetime('now','localtime')`, | ||
| }) | ||
| .where(eq(chapterSchema.id, chapterId)) | ||
| .run(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the HistoryQueries.ts file
find . -name "HistoryQueries.ts" -type fRepository: CD-Z/lnreader
Length of output: 97
🏁 Script executed:
# Check the structure to understand the repo layout
git ls-files | grep -E "(database|queries)" | head -20Repository: CD-Z/lnreader
Length of output: 844
🏁 Script executed:
# Look for manager.d.ts
find . -name "manager.d.ts" -type fRepository: CD-Z/lnreader
Length of output: 92
🏁 Script executed:
# Read HistoryQueries.ts to check the actual code
cat -n ./src/database/queries/HistoryQueries.tsRepository: CD-Z/lnreader
Length of output: 2520
🏁 Script executed:
# Read manager.d.ts to check the type definition
cat -n ./src/database/manager/manager.d.tsRepository: CD-Z/lnreader
Length of output: 11423
🏁 Script executed:
# Check how other query files use tx.update() or tx.insert() to see the pattern
rg -n 'write\(async' src/database/queries --type=ts -A 10 | head -80Repository: CD-Z/lnreader
Length of output: 5481
🏁 Script executed:
# Look specifically for await patterns with tx operations
rg -n 'await tx\.' src/database/queries --type=ts | head -20Repository: CD-Z/lnreader
Length of output: 283
🏁 Script executed:
# Check the manager.ts implementation to understand the write method behavior
cat -n ./src/database/manager/manager.ts | head -150Repository: CD-Z/lnreader
Length of output: 6037
🏁 Script executed:
# Check NovelQueries.ts line 211 and surrounding context
sed -n '210,220p' src/database/queries/NovelQueries.tsRepository: CD-Z/lnreader
Length of output: 391
🏁 Script executed:
# Check if there are patterns where tx operations are returned (not awaited but returned from callback)
rg -n 'return tx\.' src/database/queries --type=ts -B 2 -A 2Repository: CD-Z/lnreader
Length of output: 39
🏁 Script executed:
# Get a full picture: check all instances in NovelQueries where tx.update/insert/delete are used without await
cat -n src/database/queries/NovelQueries.ts | sed -n '64,76p'Repository: CD-Z/lnreader
Length of output: 534
🏁 Script executed:
# Verify the exact pattern: count how many missing-await cases exist across all query files
rg -n 'tx\.(update|insert|delete).*\.run\(\)' src/database/queries --type=ts -B 2 | grep -E '(\.ts|await|tx\.)'Repository: CD-Z/lnreader
Length of output: 2351
🏁 Script executed:
# Check drizzle-orm patterns by looking at how the return value is used in write callbacks
rg -n 'write\(async' src/database/queries --type=ts -A 15 | grep -E '(write|return|await)' | head -40Repository: CD-Z/lnreader
Length of output: 3183
🏁 Script executed:
# Check the actual context around the missing-await issues to understand scope
cat -n src/database/queries/HistoryQueries.tsRepository: CD-Z/lnreader
Length of output: 2520
🏁 Script executed:
# Verify what `.run()` returns by checking drizzle-orm types in node_modules or imports
rg -n 'from.*drizzle-orm' src/database/queries/HistoryQueries.tsRepository: CD-Z/lnreader
Length of output: 126
🏁 Script executed:
# Look for documentation or tests showing the pattern
cat -n src/database/queries/__tests__/HistoryQueries.test.ts 2>/dev/null | head -60Repository: CD-Z/lnreader
Length of output: 2319
🏁 Script executed:
# Let me check if there are similar patterns that DO use await, to confirm the expectation
rg -n 'await tx\.(update|insert|delete).*\.run' src/database/queries --type=ts -B 1 -A 1Repository: CD-Z/lnreader
Length of output: 315
🏁 Script executed:
# Check CategoryQueries to see if it has a different pattern since it seemed to have more awaits
cat -n src/database/queries/CategoryQueries.ts | sed -n '110,170p'Repository: CD-Z/lnreader
Length of output: 2143
🏁 Script executed:
# Verify by checking what the test actually validates
cat -n src/database/queries/__tests__/HistoryQueries.test.ts | sed -n '28,60p'Repository: CD-Z/lnreader
Length of output: 1519
🏁 Script executed:
# Check if .run() is ever chained with .then() or used in ways that suggest it returns a Promise
rg -n '\.run\(\)\.' src/database --type=ts | head -20Repository: CD-Z/lnreader
Length of output: 39
🏁 Script executed:
# Look at the test database implementation to see if it has special handling
cat -n src/database/queries/__tests__/testDbManager.ts | sed -n '100,130p'Repository: CD-Z/lnreader
Length of output: 1159
🏁 Script executed:
# Check how batch() in manager.ts handles `.run()` without await
cat -n src/database/manager/manager.ts | sed -n '96,112p'Repository: CD-Z/lnreader
Length of output: 642
🏁 Script executed:
# Let me verify the actual drizzle-orm behavior by checking imports and understanding the 'async' mode better
rg -n "SQLiteTransaction\|async mode\|await.*run" src/database --type=ts | head -30Repository: CD-Z/lnreader
Length of output: 39
🏁 Script executed:
# Check if there's a difference in how better-sqlite3 (sync) vs async databases handle .run()
cat -n src/database/db.ts | head -50Repository: CD-Z/lnreader
Length of output: 1808
🏁 Script executed:
# Verify the driver type and check op-sqlite documentation references
cat -n src/database/db.ts | sed -n '1,45p'Repository: CD-Z/lnreader
Length of output: 1653
🏁 Script executed:
# Check package.json to understand the actual driver being used
cat package.json | grep -A 2 -B 2 "op-sqlite\|better-sqlite\|sqlite"Repository: CD-Z/lnreader
Length of output: 661
🏁 Script executed:
# Let me search for any documentation or comments about async behavior in transactions
rg -n "async.*transaction\|Promise.*run" src/database --type=ts -i | head -20Repository: CD-Z/lnreader
Length of output: 39
🏁 Script executed:
# Let me check if there's any error handling or test failures related to these missing awaits
git log --oneline --all -20 2>/dev/null | head -10 || echo "git history not available"Repository: CD-Z/lnreader
Length of output: 712
🏁 Script executed:
# Check if there are any TypeScript strict mode settings that would catch this
cat tsconfig.json 2>/dev/null | head -30Repository: CD-Z/lnreader
Length of output: 960
🏁 Script executed:
# Let me verify one more time by looking at a working example (RepositoryQueries) side-by-side with a non-working one
echo "=== REPOSITORY QUERIES (WITH AWAIT) ===" && sed -n '33,50p' src/database/queries/RepositoryQueries.ts && echo -e "\n=== HISTORY QUERIES (WITHOUT AWAIT) ===" && sed -n '33,42p' src/database/queries/HistoryQueries.tsRepository: CD-Z/lnreader
Length of output: 884
🏁 Script executed:
# Final confirmation: check if there are any recent commits or PRs that acknowledge this pattern issue
rg -n "await.*tx\." src/database/queries --type=ts | wc -lRepository: CD-Z/lnreader
Length of output: 58
🏁 Script executed:
# Count the total missing awaits to understand scope
rg -n '^\s+tx\.(update|insert|delete).*\.run\(\)' src/database/queries --type=ts | wc -lRepository: CD-Z/lnreader
Length of output: 58
Missing await on tx.update(...).run() — operations may silently fail outside transaction boundary.
In async mode (op-sqlite driver), .run() returns a Promise. Without await, the write callback resolves before the UPDATE statement executes, causing the operation to execute outside the transaction or be lost entirely.
The same issue affects:
deleteChapterHistory(line 51)deleteAllHistory(line 63)
Fix by adding await before each tx.update(...).run() call.
🐛 Proposed fix
export const insertHistory = async (chapterId: number): Promise<void> => {
await dbManager.write(async tx => {
- tx.update(chapterSchema)
+ await tx.update(chapterSchema)
.set({
readTime: sql`datetime('now','localtime')`,
})
.where(eq(chapterSchema.id, chapterId))
.run();
});
}; export const deleteChapterHistory = async (
chapterId: number,
): Promise<void> => {
await dbManager.write(async tx => {
- tx.update(chapterSchema)
+ await tx.update(chapterSchema)
.set({ readTime: null })
.where(eq(chapterSchema.id, chapterId))
.run();
});
}; export const deleteAllHistory = async (): Promise<void> => {
await dbManager.write(async tx => {
- tx.update(chapterSchema).set({ readTime: null }).run();
+ await tx.update(chapterSchema).set({ readTime: null }).run();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await dbManager.write(async tx => { | |
| tx.update(chapterSchema) | |
| .set({ | |
| readTime: sql`datetime('now','localtime')`, | |
| }) | |
| .where(eq(chapterSchema.id, chapterId)) | |
| .run(); | |
| }); | |
| await dbManager.write(async tx => { | |
| await tx.update(chapterSchema) | |
| .set({ | |
| readTime: sql`datetime('now','localtime')`, | |
| }) | |
| .where(eq(chapterSchema.id, chapterId)) | |
| .run(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/database/queries/HistoryQueries.ts` around lines 34 - 41, The transaction
callbacks in HistoryQueries.ts call tx.update(...).run() without awaiting the
returned Promise, so the dbManager.write callback can resolve before the SQL
executes; update each affected function (the one that sets readTime using
tx.update(chapterSchema)...where(eq(chapterSchema.id, chapterId)), plus
deleteChapterHistory and deleteAllHistory) to prefix the tx.update(...).run()
calls with await so the statements execute inside the write transaction and the
write callback only resolves after the operations complete.
| const { insertId } = await dbManager.write(async tx => { | ||
| return tx | ||
| .insert(chapterSchema) | ||
| .values({ | ||
| novelId, | ||
| name, | ||
| path: NOVEL_STORAGE + '/local/' + novelId + '/' + fakeId, | ||
| releaseTime, | ||
| position: fakeId, | ||
| isDownloaded: true, | ||
| }) | ||
| .run(); | ||
| }); | ||
|
|
||
| if (insertId !== undefined && insertId >= 0) { | ||
| let chapterText: string = ''; | ||
| chapterText = NativeFile.readFile(decodePath(path)); | ||
| if (!chapterText) { | ||
| return []; | ||
| } | ||
| const novelDir = NOVEL_STORAGE + '/local/' + novelId; | ||
| const novelDir = `${NOVEL_STORAGE}/local/${novelId}`; | ||
| chapterText = chapterText.replace( | ||
| /[=](?<= href=| src=)(["'])([^]*?)\1/g, | ||
| (_, __, $2: string) => { | ||
| return `="file://${novelDir}/${$2.split(/[/\\]/).pop()}"`; | ||
| }, | ||
| ); | ||
| NativeFile.mkdir(novelDir + '/' + insertedChapter.lastInsertRowId); | ||
| NativeFile.writeFile( | ||
| novelDir + '/' + insertedChapter.lastInsertRowId + '/index.html', | ||
| chapterText, | ||
| ); | ||
| NativeFile.mkdir(novelDir + '/' + insertId); | ||
| NativeFile.writeFile(`${novelDir}/${insertId}/index.html`, chapterText); | ||
| return; | ||
| } | ||
| throw new Error(getString('advancedSettingsScreen.chapterInsertFailed')); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/services/epub/import.ts | head -120Repository: CD-Z/lnreader
Length of output: 4154
🏁 Script executed:
sed -n '110,200p' src/services/epub/import.tsRepository: CD-Z/lnreader
Length of output: 2488
🏁 Script executed:
rg -n "chapterSchema|\.path" src/services/epub/import.ts
rg -n "chapterSchema|readFile" --include="*.ts" --include="*.tsx" | head -30Repository: CD-Z/lnreader
Length of output: 769
🏁 Script executed:
rg -n "chapter.*path" -t ts | head -40
rg -n "readFile" -t ts | head -20Repository: CD-Z/lnreader
Length of output: 2303
🏁 Script executed:
sed -n '105,120p' src/screens/reader/ReaderScreen.tsx
sed -n '100,125p' src/screens/reader/hooks/useChapter.tsRepository: CD-Z/lnreader
Length of output: 1283
🏁 Script executed:
sed -n '105,125p' src/screens/reader/hooks/useChapter.ts
rg -n "WebviewScreen" -A 3 src/screens/reader/ReaderScreen.tsx | head -20Repository: CD-Z/lnreader
Length of output: 848
🏁 Script executed:
rg -n "SELECT.*chapter" -A 5 -t ts | head -40
rg -n "chapter.novelId\|chapter.id" src/screens/reader/hooks/useChapter.ts | head -10Repository: CD-Z/lnreader
Length of output: 39
🏁 Script executed:
cat src/database/schema/chapter.tsRepository: CD-Z/lnreader
Length of output: 1208
🏁 Script executed:
rg -n "getChapter\|selectChapter" -A 10 -t ts src/database/queries/ChapterQueries.ts | head -40
sed -n '1,100p' src/database/queries/ChapterQueries.tsRepository: CD-Z/lnreader
Length of output: 2658
🏁 Script executed:
rg -n "chapter.id\|chapter\.id" src/screens/reader/hooks/useChapter.ts
sed -n '130,160p' src/screens/reader/hooks/useChapter.tsRepository: CD-Z/lnreader
Length of output: 1141
🏁 Script executed:
sed -n '100,120p' src/screens/reader/hooks/useChapter.tsRepository: CD-Z/lnreader
Length of output: 725
🏁 Script executed:
rg -n "WebviewScreen" src/screens/reader/ReaderScreen.tsx -B 2 -A 8Repository: CD-Z/lnreader
Length of output: 379
🏁 Script executed:
rg -n "position" src/screens/reader/hooks/useChapter.ts | head -20Repository: CD-Z/lnreader
Length of output: 438
🏁 Script executed:
rg -n "chapter.path\|ExportNovel" src/screens/novel/components/ExportNovelAsEpubButton.tsx -B 2 -A 5 | head -50Repository: CD-Z/lnreader
Length of output: 39
🏁 Script executed:
sed -n '50,100p' src/screens/reader/hooks/useChapter.tsRepository: CD-Z/lnreader
Length of output: 1759
🏁 Script executed:
rg -n "chapterText\|loadChapterText" src/screens/reader/hooks/useChapter.ts -B 2 -A 10 | head -60Repository: CD-Z/lnreader
Length of output: 39
Critical path mismatch: DB stores fakeId-based path but file is written to insertId-based directory.
The chapter record is inserted with path: NOVEL_STORAGE + '/local/' + novelId + '/' + fakeId (line 82), where fakeId is the 0-based loop index passed in from importEpub (line 157). However, the actual HTML file is written to ${novelDir}/${insertId}/index.html (line 104), where insertId is the auto-incremented database row ID.
Because fakeId (0, 1, 2, …) and insertId (auto-increment, e.g., 7, 8, 9, …) diverge, the stored path value is incorrect. While the reader screen works around this by using chapter.id directly to construct file paths, the stored path field is still wrong and causes issues:
- The unique constraint
(novelId, path)relies on the incorrect path value, potentially allowing duplicates or preventing legitimate re-imports - The WebviewScreen feature (line 115, ReaderScreen.tsx) uses the stored path, which will be broken for local chapters
Fix: use insertId consistently for both the stored DB path and the file system directory. Since insertId isn't known until after insert, update the path field after the insert using the returned insertId, or generate a deterministic ID (e.g., UUID) before insert.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/epub/import.ts` around lines 76 - 107, The DB row is inserted
using fakeId for path but the file is written using insertId, causing mismatch;
update the code so the stored path and filesystem directory use the same
identifier (either generate a deterministic id before insert or perform an
update after insert). Concretely: after the insert into chapterSchema that
returns insertId, compute the correct path using NOVEL_STORAGE + '/local/' +
novelId + '/' + insertId (or use a UUID generated before inserting and use that
instead of fakeId), then either update the inserted row's path field within
dbManager.write or include the pre-generated id in the initial insert so
NativeFile.writeFile and the DB path use the same id (referencing insertId,
fakeId, chapterSchema, NOVEL_STORAGE).
| "@components": ["./src/components/index"], | ||
| "@components/*": ["./src/components/*"], | ||
| "@database/*": ["./src/database/*"], | ||
| "@hooks/*": ["./src/hooks/*"], | ||
| "@hooks": ["./src/hooks/index"], | ||
| "@screens/*": ["./src/screens/*"], | ||
| "@strings/*": ["./strings/*"], | ||
| "@theme/*": ["./src/theme/*"], | ||
| "@utils/*": ["./src/utils/*"], | ||
| "@plugins/*": ["./src/plugins/*"], | ||
| "@services/*": ["./src/services/*"], | ||
| "@navigators/*": ["./src/navigators/*"], | ||
| "@native/*": ["./src/native/*"], | ||
| "@api/*": ["./src/api/*"], | ||
| "@type/*": ["./src/type/*"], | ||
| "@specs/*": ["./specs/*"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if babel.config.js exists and show full content or relevant sections
echo "=== babel.config.js ==="
if [ -f babel.config.js ]; then
cat -n babel.config.js
else
echo "babel.config.js not found"
fi
echo ""
echo "=== metro.config.js ==="
if [ -f metro.config.js ]; then
cat -n metro.config.js
else
echo "metro.config.js not found"
fi
echo ""
echo "=== tsconfig.json path aliases (lines 19-34) ==="
if [ -f tsconfig.json ]; then
sed -n '19,34p' tsconfig.json
else
echo "tsconfig.json not found"
fiRepository: CD-Z/lnreader
Length of output: 4411
Add missing @native alias to babel.config.js module-resolver config.
The tsconfig.json defines @native/* → ./src/native/* (line 30), but babel.config.js does not include a corresponding alias entry. This mismatch will cause runtime Cannot find module errors at runtime when attempting to import from @native. Add '@native': './src/native' to the alias object in babel.config.js (lines 15–28).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tsconfig.json` around lines 19 - 34, tsconfig.json defines the `@native/`* path
but babel.config.js's module-resolver alias object is missing that mapping,
causing runtime import errors; open babel.config.js and add an alias entry for
'@native': './src/native' (alongside the other entries in the alias object) so
imports using `@native/`* resolve the same as tsconfig.json (use the same base
path style as the existing aliases).
- Add missing await to all stats query functions - Remove unnecessary type casts on query results - Fix downloaded-only filter to use gt(0) instead of eq(1)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/database/manager/queue.ts (1)
16-19:concurrencyis stored but never honoured indrain().
drain()always processes one task at a time regardless ofthis.options.concurrency. Either remove the option fromQueueOptions/ the constructor to prevent callers from setting a value that has no effect, or add the parallel-slot logic todrain().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/manager/queue.ts` around lines 16 - 19, The QueueOptions.concurrency value is ignored: update DbTaskQueue.drain() to honor this.options.concurrency by running up to concurrency tasks in parallel instead of one-by-one — add an activeCount (or slots) field and when drain() is called start min(queue.length, this.options.concurrency) workers that pop tasks from the queue (DbTask<any> items) and run them, decrement activeCount when each finishes and start the next queued task until queue is empty, then set active = false; alternatively remove concurrency from QueueOptions and the constructor so callers cannot pass a useless value.src/database/queries/ChapterQueries.ts (1)
312-341: Inconsistent use of.all()terminal across sibling selectors.
getNovelChapters(line 318),getUnreadNovelChapters(line 328), andgetAllUndownloadedChapters(line 341) return the query builder directly (relying on the thenable), whilegetAllUndownloadedAndUnreadChapters(line 356) and most other selectors in this file explicitly call.all(). Drizzle provides dynamic mode via.$dynamic(); the query builders themselves are thenables, so both patterns execute correctly. However, explicitly calling.all()is the idiomatic pattern used throughout the file and makes intent clearer.♻️ Suggested alignment
export const getNovelChapters = async ( novelId: number, ): Promise<ChapterInfo[]> => dbManager .select() .from(chapterSchema) - .where(eq(chapterSchema.novelId, novelId)); + .where(eq(chapterSchema.novelId, novelId)) + .all(); export const getUnreadNovelChapters = async ( novelId: number, ): Promise<ChapterInfo[]> => dbManager .select() .from(chapterSchema) .where( and(eq(chapterSchema.novelId, novelId), eq(chapterSchema.unread, true)), - ); + ) + .all(); export const getAllUndownloadedChapters = async ( novelId: number, ): Promise<ChapterInfo[]> => dbManager .select() .from(chapterSchema) .where( and( eq(chapterSchema.novelId, novelId), eq(chapterSchema.isDownloaded, false), ), - ); + ) + .all();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/ChapterQueries.ts` around lines 312 - 341, The three query helpers getNovelChapters, getUnreadNovelChapters, and getAllUndownloadedChapters currently return the query builder (thenable) instead of the explicit result; update each to call .all() on the dbManager query so they return Promise<ChapterInfo[]> like the rest of the file (e.g., matching getAllUndownloadedAndUnreadChapters and other selectors), preserving the existing function signatures and types.src/database/queries/LibraryQueries.ts (3)
26-26:castInt(0)is unnecessary for a literal integer
castInt(signature:castInt(value: number | string | AnyColumn)) emitsCAST(${value} AS INTEGER)— intended for column references or string expressions that need type coercion. Wrapping the literal0in a SQL CAST expression adds noise without benefit. The ORM'sgtoperator handles plain JS numbers directly:♻️ Proposed simplification
- gt(novelSchema.chaptersDownloaded, castInt(0)), + gt(novelSchema.chaptersDownloaded, 0),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/LibraryQueries.ts` at line 26, The comparison uses castInt(0) unnecessarily in the gt expression; replace gt(novelSchema.chaptersDownloaded, castInt(0)) with gt(novelSchema.chaptersDownloaded, 0) so the ORM can handle the literal number directly—update the code in LibraryQueries where novelSchema.chaptersDownloaded and castInt are used to remove the cast for the literal zero.
23-23: Consider refactoringfilterandsortOrderparameters to use typed ORM expressions instead ofsql.raw()strings.While the current callers only pass values from internal
LibraryFilterandLibrarySortOrderenums (making the code functionally safe), the function signature accepts arbitrary strings without type-level guarantees. This creates a maintenance hazard for future developers who might pass untrusted input.Instead of accepting raw SQL strings, migrate to parameterized ORM expressions:
- For
sortOrder: Accept pre-built expressions (e.g.,asc(novelSchema.name),desc(novelSchema.chaptersDownloaded))- For
filter: Accept ORM condition expressions (e.g.,gt(novelSchema.chaptersDownloaded, 0))This eliminates the anti-pattern entirely and aligns with Drizzle ORM best practices for type-safe query construction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/LibraryQueries.ts` at line 23, The current query builder accepts arbitrary strings via the filter and sortOrder parameters and passes them into sql.raw(), which is unsafe and bypasses type checks; change the API so sortOrder accepts typed Drizzle expression builders (e.g., asc(novelSchema.name) | desc(novelSchema.chaptersDownloaded)) and filter accepts Drizzle condition expressions (e.g., gt(novelSchema.chaptersDownloaded, 0) or and(...)). Remove usage of sql.raw(filter) and sql.raw(sortOrder) and instead pass these typed expressions directly into the query builder in the function that constructs the library query, update the parameter types to reflect the Drizzle expression/condition types, and update callers that currently map LibraryFilter/LibrarySortOrder enums to return the appropriate asc/desc or condition expressions.
35-37: Align query builder calls with Drizzle's documented pattern — reassign results from dynamic queriesThe current code discards return values from builder methods. While this works today through in-place mutation, Drizzle's official documentation recommends explicitly reassigning builder results:
query = query.orderBy(...). This is both more explicit and future-proof against any potential builder immutability changes.
- Line 36:
query.orderBy(sql.raw(sortOrder))— reassign result- Line 57:
categoryIdQuery.where(eq(novelCategorySchema.categoryId, categoryId))— reassign result- Change both
constdeclarations toletto permit reassignment♻️ Proposed changes
if (sortOrder) { - query.orderBy(sql.raw(sortOrder)); + query = query.orderBy(sql.raw(sortOrder)); }if (categoryId) { - categoryIdQuery.where(eq(novelCategorySchema.categoryId, categoryId)); + categoryIdQuery = categoryIdQuery.where(eq(novelCategorySchema.categoryId, categoryId)); }Also change
const→letfor both builder variables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/LibraryQueries.ts` around lines 35 - 37, The query builder calls in LibraryQueries.ts currently rely on in-place mutation; update the code to follow Drizzle's recommended immutable pattern by changing the builder declarations from const to let (for the variables that hold the builders) and reassign the results of dynamic calls — e.g., replace query.orderBy(sql.raw(sortOrder)) with query = query.orderBy(sql.raw(sortOrder)) and replace categoryIdQuery.where(eq(novelCategorySchema.categoryId, categoryId)) with categoryIdQuery = categoryIdQuery.where(eq(novelCategorySchema.categoryId, categoryId)) so the modified builder is stored back into the same variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/database/queries/ChapterQueries.ts`:
- Around line 260-275: The exported function name markPreviuschaptersRead has a
typo—rename it to markPreviousChaptersRead and update its declaration and export
(the function identifier in ChapterQueries.ts) so it matches the correctly
spelled counterpart markPreviousChaptersUnread; also search for and update all
call sites that import or reference markPreviuschaptersRead to use
markPreviousChaptersRead to avoid breaking consumers.
---
Duplicate comments:
In `@src/database/queries/ChapterQueries.ts`:
- Around line 193-205: The update in deleteDownloads currently clears
isDownloaded for all rows because tx.update(chapterSchema).set({ isDownloaded:
false }).run() has no WHERE; change it to only affect the chapters passed in by
building a predicate using the chapters' ids (and pluginId/novelId if needed)
and call tx.update(chapterSchema).where(<predicate>).set({ isDownloaded: false
}).run(); also ensure you use the same identifying fields as used by
deleteDownloadedFiles (pluginId, novelId, id) when constructing the WHERE clause
so only those specific chapters are updated.
---
Nitpick comments:
In `@src/database/manager/queue.ts`:
- Around line 16-19: The QueueOptions.concurrency value is ignored: update
DbTaskQueue.drain() to honor this.options.concurrency by running up to
concurrency tasks in parallel instead of one-by-one — add an activeCount (or
slots) field and when drain() is called start min(queue.length,
this.options.concurrency) workers that pop tasks from the queue (DbTask<any>
items) and run them, decrement activeCount when each finishes and start the next
queued task until queue is empty, then set active = false; alternatively remove
concurrency from QueueOptions and the constructor so callers cannot pass a
useless value.
In `@src/database/queries/ChapterQueries.ts`:
- Around line 312-341: The three query helpers getNovelChapters,
getUnreadNovelChapters, and getAllUndownloadedChapters currently return the
query builder (thenable) instead of the explicit result; update each to call
.all() on the dbManager query so they return Promise<ChapterInfo[]> like the
rest of the file (e.g., matching getAllUndownloadedAndUnreadChapters and other
selectors), preserving the existing function signatures and types.
In `@src/database/queries/LibraryQueries.ts`:
- Line 26: The comparison uses castInt(0) unnecessarily in the gt expression;
replace gt(novelSchema.chaptersDownloaded, castInt(0)) with
gt(novelSchema.chaptersDownloaded, 0) so the ORM can handle the literal number
directly—update the code in LibraryQueries where novelSchema.chaptersDownloaded
and castInt are used to remove the cast for the literal zero.
- Line 23: The current query builder accepts arbitrary strings via the filter
and sortOrder parameters and passes them into sql.raw(), which is unsafe and
bypasses type checks; change the API so sortOrder accepts typed Drizzle
expression builders (e.g., asc(novelSchema.name) |
desc(novelSchema.chaptersDownloaded)) and filter accepts Drizzle condition
expressions (e.g., gt(novelSchema.chaptersDownloaded, 0) or and(...)). Remove
usage of sql.raw(filter) and sql.raw(sortOrder) and instead pass these typed
expressions directly into the query builder in the function that constructs the
library query, update the parameter types to reflect the Drizzle
expression/condition types, and update callers that currently map
LibraryFilter/LibrarySortOrder enums to return the appropriate asc/desc or
condition expressions.
- Around line 35-37: The query builder calls in LibraryQueries.ts currently rely
on in-place mutation; update the code to follow Drizzle's recommended immutable
pattern by changing the builder declarations from const to let (for the
variables that hold the builders) and reassign the results of dynamic calls —
e.g., replace query.orderBy(sql.raw(sortOrder)) with query =
query.orderBy(sql.raw(sortOrder)) and replace
categoryIdQuery.where(eq(novelCategorySchema.categoryId, categoryId)) with
categoryIdQuery = categoryIdQuery.where(eq(novelCategorySchema.categoryId,
categoryId)) so the modified builder is stored back into the same variable.
| export const markPreviuschaptersRead = async ( | ||
| chapterId: number, | ||
| novelId: number, | ||
| ): Promise<void> => { | ||
| await dbManager.write(async tx => { | ||
| tx.update(chapterSchema) | ||
| .set({ unread: false }) | ||
| .where( | ||
| and( | ||
| lte(chapterSchema.id, chapterId), | ||
| eq(chapterSchema.novelId, novelId), | ||
| ), | ||
| ) | ||
| .run(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Typo in exported function name: markPreviuschaptersRead → markPreviousChaptersRead.
The counterpart markPreviousChaptersUnread (line 277) is spelled correctly; this export is missing the o. Callers would need to use the misspelled name.
✏️ Proposed fix
-export const markPreviuschaptersRead = async (
+export const markPreviousChaptersRead = async (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/database/queries/ChapterQueries.ts` around lines 260 - 275, The exported
function name markPreviuschaptersRead has a typo—rename it to
markPreviousChaptersRead and update its declaration and export (the function
identifier in ChapterQueries.ts) so it matches the correctly spelled counterpart
markPreviousChaptersUnread; also search for and update all call sites that
import or reference markPreviuschaptersRead to use markPreviousChaptersRead to
avoid breaking consumers.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @CD-Z. * #23 (comment) The following files were modified: * `src/components/NovelCover.tsx` * `src/database/manager/manager.ts` * `src/database/queries/__tests__/setup.ts` * `src/database/queries/__tests__/testData.ts` * `src/database/queries/__tests__/testDb.ts` * `src/database/queries/__tests__/testDbManager.ts` * `src/database/utils/parser.ts` * `src/hooks/persisted/useNovel.ts` * `src/screens/novel/NovelContext.tsx` * `src/screens/reader/hooks/useChapter.ts`
1 participant
Summary by CodeRabbit
New Features
Bug Fixes