Open
Conversation
…mocksContract Final validation confirms mock-contract test suite clean and target file deletion verified with zero stale references in src/ scope.
📝 WalkthroughWalkthroughThis PR replaces the legacy hook-based novel state with a Zustand-backed store, adds a bootstrap service, a typed MMKV persistence bridge, chapter action modules, dev plugins, multiple store-centric UI/context migrations, and extensive unit tests and mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant Screen as Novel Screen
participant Store as Novel Store
participant Bootstrap as Bootstrap Service
participant DB as Database
participant Source as Plugin/Source
Screen->>Store: actions.bootstrapNovel(pluginId, novelPath, pageIndex, settings)
activate Store
Store->>Bootstrap: bootstrapNovel(pluginId, novelPath, pageIndex, settings)
activate Bootstrap
Bootstrap->>DB: getChapterCount(pluginId, novelPath)
DB-->>Bootstrap: count
alt count === 0
Bootstrap->>Source: fetchPage(pluginId, novelPath, page="1")
Source-->>Bootstrap: { chapters, novel? }
Bootstrap->>DB: insertNovel/chapters(...)
end
Bootstrap->>DB: getPageChapters(pluginId, novelPath, pageIndex)
DB-->>Bootstrap: chapters + metadata
Bootstrap-->>Store: { novel, pages, chapters, batchInfo, firstUnread }
deactivate Bootstrap
Store->>Store: setState({...})
Store-->>Screen: subscribers notified (selective selectors)
deactivate Store
sequenceDiagram
participant Component as UI Component
participant Selector as Selector fn
participant Store as Novel Store
participant Subscribe as Store.subscribe
Component->>Store: useNovelValue(key) / useNovelActions()
Component->>Subscribe: novelStore.subscribe(listener)
activate Store
Note over Component: On store update
Store->>Selector: selector(state)
Selector-->>Component: selected snapshot
Component->>Component: re-render if snapshot changed
deactivate Store
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/reader/components/ChapterDrawer/index.tsx (1)
190-191:⚠️ Potential issue | 🔴 CriticalBug: Comparing ref object instead of
ref.current.
scrollToIndexis auseRefobject, which is neverundefined. The condition should checkscrollToIndex.current === undefinedinstead.🐛 Proposed fix
- {scrollToIndex === undefined ? ( + {scrollToIndex.current === undefined ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/reader/components/ChapterDrawer/index.tsx` around lines 190 - 191, The conditional is comparing the ref object instead of its value: change the check to test scrollToIndex.current === undefined (not scrollToIndex) so the LoadingScreenV2 branch only renders when the ref has no current value; update any other places in this JSX/logic that read scrollToIndex to use scrollToIndex.current (refer to the scrollToIndex ref and LoadingScreenV2 usage in this component).
🧹 Nitpick comments (14)
src/hooks/persisted/useNovel/useChapterOperations.ts (2)
59-71: Remove unnecessaryasynckeyword from synchronous functions.
transformAndSetChaptersandextendChaptersdon't contain anyawaitexpressions. Theasynckeyword causes them to returnPromise<void>instead ofvoid, which may confuse callers.♻️ Remove async
const transformAndSetChapters = useCallback( - async (chs: ChapterInfo[]) => { + (chs: ChapterInfo[]) => { _setChapters(transformChapters(chs)); }, [transformChapters, _setChapters], ); const extendChapters = useCallback( - async (chs: ChapterInfo[]) => { + (chs: ChapterInfo[]) => { _setChapters(prev => prev.concat(transformChapters(chs))); }, [transformChapters, _setChapters], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovel/useChapterOperations.ts` around lines 59 - 71, Both transformAndSetChapters and extendChapters are declared async but contain no await and thus return Promise<void>; remove the unnecessary async keyword from the function declarations for transformAndSetChapters and extendChapters so they return void; keep their bodies unchanged and retain the same dependencies (transformChapters and _setChapters) in the useCallback calls.
17-26:chaptersparameter is declared but unused.The
chaptersfield inUseChapterOperationsParamsis never referenced in the hook body. Either remove it from the interface or use it if needed.♻️ Remove unused parameter
export interface UseChapterOperationsParams { novel: NovelInfo | undefined; - chapters: ChapterInfo[]; _setChapters: React.Dispatch<React.SetStateAction<ChapterInfo[]>>; transformChapters: (chs: ChapterInfo[]) => ChapterInfo[]; settingsSort: ChapterOrderKey; settingsFilter: ChapterFilterKey[]; currentPage: string; fetching: boolean; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovel/useChapterOperations.ts` around lines 17 - 26, The interface UseChapterOperationsParams declares a chapters field that is never used in the hook; remove the unused property from UseChapterOperationsParams (delete the chapters: ChapterInfo[] entry) and update any call sites that construct objects of this type (calls to useChapterOperations or factories creating UseChapterOperationsParams) to stop providing chapters, or alternatively, if the data is actually needed, wire the existing chapters usage into the hook logic (e.g., inside useChapterOperations reference the passed chapters instead of any other source). Ensure you only change the interface and its callers or integrate the chapters parameter in the useChapterOperations function to keep types consistent.src/hooks/persisted/useNovel/types.ts (1)
7-18: Consider declaring the interface before its default value.
defaultNovelSettingsreferencesNovelSettingsbefore the interface is declared. While TypeScript hoists interface declarations, defining the interface first improves readability.♻️ Suggested reordering
export const LAST_READ_PREFIX = 'LAST_READ_PREFIX'; +export interface NovelSettings { + sort?: ChapterOrderKey; + filter: ChapterFilterKey[]; + showChapterTitles?: boolean; +} + export const defaultNovelSettings: NovelSettings = { showChapterTitles: true, filter: [], }; export const defaultPageIndex = 0; - -export interface NovelSettings { - sort?: ChapterOrderKey; - filter: ChapterFilterKey[]; - showChapterTitles?: boolean; -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovel/types.ts` around lines 7 - 18, Move the NovelSettings interface declaration above the default values so the type is declared before it’s referenced: place the NovelSettings interface first, followed by defaultNovelSettings and defaultPageIndex; update any imports/usages if necessary to reflect the new order, ensuring the symbols NovelSettings and defaultNovelSettings remain unchanged.src/utils/mmkv/zustand-adapter.ts (1)
19-25: Replaceconsole.errorwith a project logger or suppress the lint rule.ESLint flags
console.errorstatements. If logging storage errors is intentional for debugging, either use a project-sanctioned logger utility or add an inline eslint-disable comment with justification.♻️ Option: Inline suppression if console is intentional
} catch (error) { + // eslint-disable-next-line no-console -- Storage adapter error logging for debugging console.error( `[mmkvZustandAdapter] Error getting item for key "${key}":`, error, );Apply the same pattern to lines 36 and 50.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/mmkv/zustand-adapter.ts` around lines 19 - 25, Replace the console.error calls inside the mmkv-zustand adapter error handlers with the project logging utility or explicitly suppress the lint rule with a justified inline comment: locate the catch blocks in the mmkv adapter functions (e.g., getItem, setItem, removeItem in zustand-adapter.ts) and either swap console.error(...) for the shared logger (processLogger or logger) with the same message and error object, or add an eslint-disable-next-line no-console comment plus a brief justification above each console.error (the occurrences at the catch blocks currently around the key-handling logic and the ones at lines corresponding to getItem, setItem, and removeItem).src/plugins/pluginManager.ts (1)
172-174: Consider extracting dev plugin IDs as constants.The dev plugin IDs
'dev-novel'and'dev-paged-novel'are duplicated here and in the registry initialization (lines 90-91). Extracting them as constants would reduce duplication and prevent typos.♻️ Suggested refactor
+const DEV_NOVEL_ID = 'dev-novel'; +const DEV_PAGED_NOVEL_ID = 'dev-paged-novel'; + const plugins: Record<string, Plugin | undefined> = __DEV__ ? { - 'dev-novel': devNovelPlugin, - 'dev-paged-novel': devPagedNovelPlugin, + [DEV_NOVEL_ID]: devNovelPlugin, + [DEV_PAGED_NOVEL_ID]: devPagedNovelPlugin, } : {};Then update the check:
- if (__DEV__ && (pluginId === 'dev-novel' || pluginId === 'dev-paged-novel')) { + if (__DEV__ && (pluginId === DEV_NOVEL_ID || pluginId === DEV_PAGED_NOVEL_ID)) { return plugins[pluginId]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/pluginManager.ts` around lines 172 - 174, Extract the duplicate dev plugin ID strings into shared constants (e.g., DEV_PLUGIN_NOVEL and DEV_PLUGIN_PAGED_NOVEL) and replace the inline literals in both the registry initialization and the conditional that checks pluginId in pluginManager.ts; specifically update the check that currently uses (pluginId === 'dev-novel' || pluginId === 'dev-paged-novel') to reference the new constants and ensure the registry initialization also uses the same constants to avoid duplication and typos.src/hooks/persisted/usePlugins.ts (1)
53-76: Consider extracting dev plugin PluginItem mapping to reduce duplication.The same property mapping pattern is repeated for both dev plugins. A helper function would reduce duplication.
♻️ Suggested refactor
const toPluginItem = (plugin: typeof devNovelPlugin): PluginItem => ({ id: plugin.id, name: plugin.name, site: plugin.site, lang: plugin.lang, version: plugin.version, url: plugin.url, iconUrl: plugin.iconUrl, hasSettings: !!plugin.pluginSettings, }); const devPlugins: PluginItem[] = __DEV__ ? [toPluginItem(devNovelPlugin), toPluginItem(devPagedNovelPlugin)] : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/usePlugins.ts` around lines 53 - 76, Extract the repeated mapping of devNovelPlugin and devPagedNovelPlugin into a small helper (e.g., toPluginItem) that converts a plugin (devNovelPlugin/devPagedNovelPlugin) into a PluginItem by copying id, name, site, lang, version, url, iconUrl and computing hasSettings as !!plugin.pluginSettings; then replace the inline objects in the devPlugins array with calls to that helper (devPlugins, devNovelPlugin, devPagedNovelPlugin, PluginItem, toPluginItem).src/screens/reader/components/ChapterDrawer/index.tsx (1)
43-58: Consider extractinguseNovelDomainValueto a shared utility.This helper function is duplicated across multiple files (
NovelScreenList.tsx,NovelScreen.tsx,useChapter.ts). Extract it to a shared location (e.g.,@hooks/persisted/useNovel/useNovelDomainValue.ts) to reduce duplication and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/reader/components/ChapterDrawer/index.tsx` around lines 43 - 58, Extract the duplicated hook useNovelDomainValue into a shared utility and update callers to import it instead of redefining it; create a new file (e.g., `@hooks/persisted/useNovel/useNovelDomainValue.ts`) that exports useNovelDomainValue with the same signature and implementation (it should reference useNovelContext's novelStore, useCallback for subscribe/getSnapshot, and call novelStore.subscribe and novelStore.getState with the selector), then replace the local definitions in ChapterDrawer (index.tsx), NovelScreenList.tsx, NovelScreen.tsx, and useChapter.ts with an import from the new module to remove duplication and keep behavior identical.src/screens/reader/components/ChapterDrawer/__tests__/ChapterDrawer.test.tsx (1)
135-162: Consider adding more test cases for edge conditions.The current test covers the happy path but doesn't test:
- When
fetchingistrue(should not callgetNextChapterBatch)- When
batch >= total(should not callgetNextChapterBatch)- When
pagesarray is empty or chapter page is not found💡 Additional test cases to consider
it('does not call getNextChapterBatch when fetching is true', async () => { const store = createStore({ fetching: true }); // ... setup and render fireEvent.press(screen.getByTestId('legend-end-reached')); expect(store.state.getNextChapterBatch).not.toHaveBeenCalled(); }); it('does not call getNextChapterBatch when batch >= total', async () => { const store = createStore({ batchInformation: { batch: 1, total: 1, totalChapters: 2 } }); // ... setup and render fireEvent.press(screen.getByTestId('legend-end-reached')); expect(store.state.getNextChapterBatch).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/reader/components/ChapterDrawer/__tests__/ChapterDrawer.test.tsx` around lines 135 - 162, Add unit tests in ChapterDrawer.test.tsx that cover the edge conditions the reviewer suggested: create variants of createStore with fetching: true, with batchInformation where batch >= total, and with pages empty or missing the current chapter page; for each test use mockUseChapterContext and mockUseNovelContext as in the existing test, render <ChapterDrawer />, trigger the end-reached event via fireEvent.press(screen.getByTestId('legend-end-reached')), and assert that store.state.getNextChapterBatch is NOT called; also keep the existing expectation that setPageIndex is invoked where appropriate to ensure selector-backed behavior remains tested.src/screens/novel/components/NovelScreenList.tsx (1)
108-130: Consider consolidating multiple selector subscriptions.Each
useNovelDomainValuecall creates a separate subscription. With 16+ subscriptions in this component, consider using a single combined selector that returns an object with all needed values, then destructure it. This reduces subscription overhead and re-render frequency.♻️ Example consolidated approach
const selectNovelScreenListState = (state: NovelStoreState) => ({ chapters: state.chapters, deleteChapter: state.deleteChapter, fetching: state.fetching, // ... other needed values }); // In component: const { chapters, deleteChapter, fetching, // ... } = useNovelDomainValue(novelStore, selectNovelScreenListState);🤖 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 108 - 130, Multiple calls to useNovelDomainValue (e.g., useNovelDomainValue(novelStore, selectChapters), selectDeleteChapter, selectFetching, selectFirstUnreadChapter, selectLoading, selectNovelSettings, selectPages, selectSetNovel, selectNovel, selectBatchInformation, selectPageIndex, selectOpenPage, selectUpdateChapter, selectRefreshNovel, selectLastRead) create many subscriptions; replace them with a single consolidated selector (e.g., selectNovelScreenListState) that returns an object with all required fields, call useNovelDomainValue once with that selector, and then destructure the returned object into chapters, deleteChapter, fetching, firstUnreadChapter, loading, novelSettings, pages, setNovel, fetchedNovel, batchInformation, pageIndex, openPage, updateChapter, refreshNovel, lastRead to reduce subscription overhead and re-renders.src/screens/novel/__tests__/NovelScreen.test.tsx (1)
43-48: Assert the store boundary here, not the DB helper.This test is supposed to protect the
novelStorecutover, but the progress-reset expectation still hangs off@database/queries/ChapterQueries.updateChapterProgressByIds. That couples the suite to an implementation detail instead of the screen’s public dependency.Possible refactor
const state = { novel: baseNovel, chapters: [], fetching: false, batchInformation: { batch: 0, total: 0, totalChapters: 0 }, getNextChapterBatch: jest.fn(), loadUpToBatch: jest.fn(), setNovel: jest.fn(), bookmarkChapters: jest.fn(), markChaptersRead: jest.fn(), markChaptersUnread: jest.fn(), + updateChapterProgressByIds: jest.fn(), markPreviouschaptersRead: jest.fn(), markPreviousChaptersUnread: jest.fn(), refreshChapters: jest.fn(), deleteChapters: jest.fn(), ...overrides, @@ fireEvent.press(screen.getByTestId('select-read')); fireEvent.press(screen.getByTestId('action-check-outline')); expect(store.state.markChaptersUnread).toHaveBeenCalledTimes(1); - expect(mockUpdateChapterProgressByIds).toHaveBeenCalledWith([11], 0); + expect(store.state.updateChapterProgressByIds).toHaveBeenCalledWith( + [11], + 0, + ); expect(store.state.refreshChapters).toHaveBeenCalledTimes(1); });Also applies to: 223-239, 290-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/novel/__tests__/NovelScreen.test.tsx` around lines 43 - 48, The test currently asserts behavior on the DB helper updateChapterProgressByIds which ties the spec to an implementation detail; instead spy or mock the public store API (novelStore) used by the screen and assert the store boundary was invoked. Replace expectations that reference `@database/queries/ChapterQueries.updateChapterProgressByIds` with assertions against the novelStore method the screen calls (e.g., the store method that resets/updates chapter progress), and update the mocks in the three affected blocks (around lines 43-48, 223-239, 290-305) to stop verifying the DB helper and verify novelStore was called with the expected ids/args; keep the existing jest.mock of getAllUndownloadedAndUnreadChapters/getAllUndownloadedChapters but move the progress-reset assertion to the store-level spy.src/hooks/persisted/useNovel/novelStore.ts (2)
331-335: Address ESLint warning: prefer explicit async handling overvoid.ESLint flags the
void state.getChapters()expression. Consider using.catch()to explicitly handle potential errors, or refactor to make the intent clearer.♻️ Suggested fix
const state = get(); if (state.novel && state.pages.length > 0) { - void state.getChapters(); + state.getChapters().catch(() => { + // Fire-and-forget: errors handled internally + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovel/novelStore.ts` around lines 331 - 335, The expression using void to ignore the Promise from state.getChapters() should be replaced with explicit async/error handling: either call state.getChapters().catch(err => { /* log or handle error */ }) to handle rejection, or make the enclosing callback async and await state.getChapters() inside a try/catch; update the code around get() in novelStore.ts (the section that reads const state = get(); if (state.novel && state.pages.length > 0) { ... }) to use one of these patterns and ensure any errors are logged or handled instead of being swallowed.
340-362: Address ESLint warning and add error handling forfollowNovel.The
void deps.switchNovelToLibrary(...)pattern triggers the ESLint warning. Additionally, ifswitchNovelToLibraryfails, the UI state will remain unchanged, but users won't receive feedback. Consider adding error handling.♻️ Suggested improvement
followNovel: () => { const state = get(); const currentNovel = state.novel; if (!currentNovel || !deps.switchNovelToLibrary) { return; } - void deps - .switchNovelToLibrary(state.novelPath, state.pluginId) + deps + .switchNovelToLibrary(state.novelPath, state.pluginId) .then(() => { set(inner => { if (!inner.novel) { return {}; } return { novel: { ...inner.novel, inLibrary: !inner.novel.inLibrary, }, }; }); + }) + .catch(() => { + // Optionally show error toast via deps }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovel/novelStore.ts` around lines 340 - 362, The followNovel function currently uses "void deps.switchNovelToLibrary(...)" which triggers ESLint and lacks error handling; change followNovel to an async function, await deps.switchNovelToLibrary(state.novelPath, state.pluginId) inside a try/catch, and move the state toggle (set(...)) into the try block so it only runs on success; in the catch block, surface the failure (e.g., call a provided notifier like deps.notifyError or at minimum console.error with the caught error) so users get feedback when switchNovelToLibrary fails and ESLint warnings are resolved.src/screens/novel/NovelContext.tsx (1)
45-94: Consider stabilizingroute.paramsto prevent store recreation.Including
route.paramsdirectly in the dependency array may cause unnecessary store recreation if the params object reference changes between renders while the actualpathandpluginIdvalues remain the same. SincepathandpluginIdare already extracted and used individually, consider removingroute.paramsfrom the deps or extracting theidcheck outside the memo.♻️ Suggested improvement
+ const routeNovel = 'id' in route.params ? (route.params as NovelInfo) : undefined; + const novelStore = useMemo( () => createNovelStore({ pluginId, novelPath: path, - novel: 'id' in route.params ? (route.params as NovelInfo) : undefined, + novel: routeNovel, defaultChapterSort, // ... rest unchanged }), - [defaultChapterSort, path, pluginId, route.params, switchNovelToLibrary], + [defaultChapterSort, path, pluginId, routeNovel, switchNovelToLibrary], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/novel/NovelContext.tsx` around lines 45 - 94, The memo currently includes route.params in the dependency array which can cause unnecessary re-creation of novelStore; remove route.params from the deps and instead derive the NovelInfo check outside the useMemo (e.g. compute a local variable like initialNovel = 'id' in route.params ? (route.params as NovelInfo) : undefined) and pass that stable value (or just rely on path and pluginId) into createNovelStore; update the useMemo dependency array to [defaultChapterSort, path, pluginId, switchNovelToLibrary] so novelStore (created by createNovelStore) only rebuilds when actual identifying values change.src/plugins/dev/DevPagedNovelPlugin.ts (1)
252-363: Consider extracting shared page calculation logic.The page calculation logic (lines 262-294 and 322-354) is duplicated between
parseNovelandparsePage. Consider extracting this into a helper function for maintainability.♻️ Suggested extraction
const calculateTotalPages = async ( novelPath: string, newContent: boolean, newPages: boolean, ): Promise<number> => { const res = await dbManager .select({ count: novelSchema.totalChapters }) .from(novelSchema) .where( and( eq(novelSchema.pluginId, 'dev-paged-novel'), eq(novelSchema.path, novelPath), ), ) .get(); const currentChapters = res?.count || 0; let totalPages = Math.ceil(currentChapters / CHAPTERS_PER_PAGE); if (totalPages < DEFAULT_PAGES) { totalPages = DEFAULT_PAGES; } if (newPages) { totalPages += 2; } else if (newContent) { const extraChapters = Math.min( 5, CHAPTERS_PER_PAGE - (totalPages * CHAPTERS_PER_PAGE - currentChapters), ); if (extraChapters > 0) { totalPages = Math.ceil( (currentChapters + extraChapters) / CHAPTERS_PER_PAGE, ); } } return totalPages; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/dev/DevPagedNovelPlugin.ts` around lines 252 - 363, Both parseNovel and parsePage duplicate the page-calculation logic; extract that logic into a new async helper (e.g., calculateTotalPages) that accepts novelPath, newContent, newPages and returns totalPages by performing the dbManager select on novelSchema, applying CHAPTERS_PER_PAGE and DEFAULT_PAGES rules and the newPages/newContent adjustments; then replace the duplicated blocks in parseNovel and parsePage to call calculateTotalPages(novelPath, newContent, newPages) and use its result for generating chapters and page bounds checks (references: parseNovel, parsePage, dbManager, novelSchema, CHAPTERS_PER_PAGE, DEFAULT_PAGES, Storage keys 'provideNewContent' and 'provideNewPages').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__mocks__/react-native-mmkv.js`:
- Around line 79-101: createTupleHook currently creates a new jest.fn setter on
every call which breaks referential stability and mock call accumulation; change
createTupleHook to look up a per-key setter in a cache (e.g., setterCache Map)
and reuse the same jest.fn for a given key (creating and storing it only if
missing), keeping the existing setter behavior (deleting maps when value ===
undefined and type-based sets otherwise); ensure tests can clear setterCache in
beforeEach when needed.
In `@package.json`:
- Around line 117-118: The package.json currently lists "zustand": "^5.0.12"
which declares a peer dependency excluding React 19; update package
configuration to avoid broken installs by either (A) pinning the project React
peer/dependency to a 18.x range (so keep "zustand": "^5.0.12") or (B) if you
must target React 19, remove or delay the dependency upgrade and add a TODO to
upgrade "zustand" to a version that advertises React 19 support once released
(or use an explicit override/resolution only as a temporary measure); adjust the
package.json "react"/"react-dom" entries or add a comment/issue referencing
"zustand": "^5.0.12" to document the chosen approach.
In `@src/hooks/__tests__/mocks.ts`:
- Around line 56-59: The shared mock object `batchInformation` is missing the
new `totalChapters` property; update the mock in src/hooks/__tests__/mocks.ts so
`batchInformation` includes `totalChapters` (e.g., set to 0) alongside `batch`
and `total` to match the production/bootstrap/store shape used by the code paths
that read `batchInformation.totalChapters`.
- Around line 98-123: The mock store must implement the external-store contract:
update setState to notify subscribers and make the default context use a fresh
store instead of a singleton. In createMockNovelStore, add an internal listeners
array, implement subscribe(listener) to push the listener and return an
unsubscribe function that removes it, keep getState() returning the current
state, and modify setState(nextState) to compute the new partial state
(supporting function or object), update state, then call each listener();
finally, change defaultMockNovelContext/mockUseNovelContext so they create a new
store instance by calling createMockNovelStore() when invoked rather than
reusing a single store across tests.
In `@src/hooks/__tests__/useNovel.test.ts`:
- Around line 37-93: The test seeds only keyContract.settings/lastRead but
asserts that novelPersistence.keys.settings/lastRead are removed; update the
setup in the test for each novel in cachedNovels to also write the legacy keys
using MMKVStorage.set with novelPersistence.keys.settings({...}) and
novelPersistence.keys.lastRead({...}) (and the tracked key via
TRACKED_NOVEL_PREFIX remains), so deleteCachedNovels is exercised against both
current and legacy key builders; keep the rest of the assertions unchanged.
In `@src/hooks/persisted/useNovel/__tests__/keyContract.test.ts`:
- Around line 50-60: The test expectations use an inconsistent prefix naming:
the spec checks for 'NOVEL_SETTINGS_webnovel_...' while other tests expect
suffixes like 'NOVEL_PAGE_INDEX_PREFIX_' and 'LAST_READ_PREFIX_'; inspect key
generation in keyContract.settings and the prefix constants (e.g.,
NOVEL_SETTINGS_PREFIX, NOVEL_PAGE_INDEX_PREFIX, LAST_READ_PREFIX) and make them
consistent by either changing the expected value in this test to the actual
prefix produced by keyContract.settings or adjusting keyContract to emit the
same prefix format used by the other keys so all tests use the same PREFIX
naming convention.
In `@src/hooks/persisted/useNovel/__tests__/persistence.test.ts`:
- Around line 167-173: Rename the test description to accurately state it only
checks runtime export availability (not compile-time type safety); update the
it(...) string that currently reads 'keeps migrate contract usage compile-safe
through stable exports' to something like 'validates that migrate contract
exports remain available at runtime' and keep the existing runtime checks
against novelPersistence.copySettings, novelPersistence.readLastRead, and the
require('@services/migrate/migrateNovel') destructure of migrateNovel unchanged.
In `@src/hooks/persisted/useNovel/bootstrapService.ts`:
- Around line 65-68: The inflight cache (inflightBootstraps) is too coarse and
global: it is keyed only by getBootstrapKey(pluginId, novelPath) and lives at
module scope, causing different reader settings (pageIndex, settingsSort,
settingsFilter) to collide and leaking dedupe across createBootstrapService()
instances. Fix by moving the Map into the createBootstrapService() closure (so
each service instance has its own cache) and extend the getBootstrapKey to
include pageIndex, settingsSort and settingsFilter (serialize them
deterministically) so the key uniquely represents the full bootstrap request
before storing/reading Promises from inflightBootstraps.
- Around line 133-137: The try/catch around the batched chapter fetch (calling
deps.getPageChaptersBatched in bootstrapService) is swallowing errors and
returning an empty array; replace the console.error swallow with proper error
propagation and app logging: remove the catch that returns [] (or rethrow after
logging via the app logger) so failures bubble to the caller, and apply the same
change to the other batched loader blocks referenced (the similar try/catch
blocks around the other batched loaders at the later occurrences), ensuring you
use the application logger instead of console.error and do not convert
exceptions into successful empty results.
- Around line 80-83: The code is incorrectly filtering custom page keys by
coercing to Number (which drops non-numeric keys like "special"); update the
tmpPages assignment and the analogous branch around lines 138-149 to accept any
non-null page id returned by deps.getCustomPages() instead of checking
Number(page). Specifically, in the tmpPages = (await
deps.getCustomPages(tmpNovel.id)).map(c => c.page).filter((page): page is string
=> page !== null) flow (and the similar block referenced), remove any
Number(page) truthiness checks so calculatePages() receives all string keys
(numeric parsing can be done later only where numeric ids are required).
- Around line 151-155: The batch total is computed as a one-based count causing
an off-by-one when batches are zero-indexed; replace uses of
Math.floor(chapterCount / 300) with a zero-based last-batch calculation: use
Math.max(0, Math.ceil(chapterCount / 300) - 1) so the last batch index is
correct (and non-negative) and update the three occurrences (the BatchInfo
construction assigned to batchInformation and the other two spots referenced in
the comment) to use this expression instead of Math.floor(...).
In `@src/hooks/persisted/useNovel/chapterActions.ts`:
- Around line 188-208: The code persists a clamped value but writes the raw
progress into local state; in updateChapterProgressAction capture a single
clamped value (e.g., const clamped = Math.min(progress, 100)) and use that
clamped value both when calling deps.updateChapterProgress(chapterId, clamped)
and when updating state inside mutateChapters (set c.progress to clamped for the
matching chapterId) so DB and UI remain consistent.
- Around line 210-234: deleteChapterAction (and similarly deleteChaptersAction)
currently calls deps.deleteChapter/deps.deleteChapters and uses .then() with no
rejection handling; add error handling so failures don't leave the UI
inconsistent. Update deleteChapterAction and deleteChaptersAction to either
await the promise in an async function or append a .catch() to the existing
promise chain to revert or avoid mutating state when the delete fails and call
deps.showToast with an error message (use deps.getString to build a localized
error message and include the caught error). Ensure you reference
deps.deleteChapter/deps.deleteChapters, mutateChapters, deps.showToast and
deps.getString when implementing the fix so successful deletes still update
isDownloaded and failures surface an error and do not change state.
In `@src/hooks/persisted/useNovel/keyContract.ts`:
- Around line 12-24: The current key builders in keyContract (pageIndex,
settings, lastRead) concatenate NOVEL_*_PREFIX with pluginId and novelPath using
"_" which can cause collisions when inputs contain underscores; update these
functions to use a safer delimiter (e.g., "::") or serialize the identifiers
(e.g., JSON.stringify or encodeURIComponent) before concatenation so the
produced keys are unambiguous, ensuring you update all three methods (pageIndex,
settings, lastRead) to the same scheme and keep the prefix constants unchanged.
In `@src/hooks/persisted/useNovel/persistence.ts`:
- Around line 72-97: readPageIndex and writePageIndex currently accept negative
or fractional numbers; update both to enforce a non-negative integer: in
readPageIndex (using keys.pageIndex, storage.getNumber and storage.getString)
treat any numeric value as valid only if Number.isFinite and >= 0, then
normalize by taking Math.floor (and persist the floored value with storage.set)
before returning; for parsed string values do the same (Number(...) -> check
finite and >= 0 -> Math.floor -> persist -> return); otherwise fallback to
storage.set(key, defaultPageIndex) and return defaultPageIndex; in
writePageIndex apply the same validation/normalization (Number.isFinite(value)
&& value >= 0 ? Math.floor(value) : defaultPageIndex) and persist that with
storage.set.
In `@src/plugins/dev/DevNovelPlugin.ts`:
- Around line 234-250: The code currently applies the "+5 new content" bump
regardless of whether the novel exists because chapterCount is normalized to at
least 20 before applying newChapters; change the logic so the +5 is only applied
when a real row/count exists for that novel. Concretely, keep retrieving res via
dbManager.select(...) and compute a baseCount from res (e.g., const baseCount =
res?.count ?? 0), derive chapterCount = baseCount < 20 && baseCount > 0 ? 20 :
Math.max(baseCount, 0) (or keep existing normalization but only when
baseCount>0), and set nextChapterCount = newChapters && baseCount > 0 ?
chapterCount + 5 : chapterCount so that provideNewContent only adds chapters
when the novel already exists (use the symbols s, newChapters, res, novelSchema,
chapterCount, nextChapterCount to locate the code).
In `@src/screens/novel/NovelScreen.tsx`:
- Around line 59-60: The selector and store/action names have inconsistent
casing for "Chapters": rename the selector selectMarkPreviouschaptersRead to
selectMarkPreviousChaptersRead, rename the store property
markPreviouschaptersRead to markPreviousChaptersRead, and rename the action
markPreviouschaptersReadAction to markPreviousChaptersReadAction; update all
references/usages (selectors, dispatches, imports/exports, tests) to the new
names so identifiers match, and run type checks to fix any remaining imports or
compilation errors.
In `@src/services/plugin/fetch.ts`:
- Around line 48-53: The code currently treats any non-first-page request as a
generic "could not fetch chapters" error; update the logic in the fetch path
that uses plugin.parseNovel so that when page !== '1' (or numeric page > 1) and
plugin.parsePage is not implemented, you throw a distinct error indicating
"paged chapter fetching is unsupported" (or an UnsupportedPaginationError)
instead of the generic failure; keep the existing behavior of calling
plugin.parseNovel(novelPath) and returning res.chapters when page === '1' and
parseNovel exists, but add an explicit branch that checks plugin.parsePage (or
its absence) and page to produce the clearer error referencing plugin.parsePage,
page and novelPath.
In `@src/services/updates/LibraryUpdateQueries.ts`:
- Line 197: Remove the debug console logging statement
console.log(novel.chapters) from the LibraryUpdateQueries update flow; either
delete it or replace it with a gated logger call (e.g., processLogger.debug or
similar) behind a dev-only condition (NODE_ENV or a feature flag) so production
no-console rules and runtime noise are avoided; locate the statement where the
novel variable is handled in LibraryUpdateQueries and update that block
accordingly.
---
Outside diff comments:
In `@src/screens/reader/components/ChapterDrawer/index.tsx`:
- Around line 190-191: The conditional is comparing the ref object instead of
its value: change the check to test scrollToIndex.current === undefined (not
scrollToIndex) so the LoadingScreenV2 branch only renders when the ref has no
current value; update any other places in this JSX/logic that read scrollToIndex
to use scrollToIndex.current (refer to the scrollToIndex ref and LoadingScreenV2
usage in this component).
---
Nitpick comments:
In `@src/hooks/persisted/useNovel/novelStore.ts`:
- Around line 331-335: The expression using void to ignore the Promise from
state.getChapters() should be replaced with explicit async/error handling:
either call state.getChapters().catch(err => { /* log or handle error */ }) to
handle rejection, or make the enclosing callback async and await
state.getChapters() inside a try/catch; update the code around get() in
novelStore.ts (the section that reads const state = get(); if (state.novel &&
state.pages.length > 0) { ... }) to use one of these patterns and ensure any
errors are logged or handled instead of being swallowed.
- Around line 340-362: The followNovel function currently uses "void
deps.switchNovelToLibrary(...)" which triggers ESLint and lacks error handling;
change followNovel to an async function, await
deps.switchNovelToLibrary(state.novelPath, state.pluginId) inside a try/catch,
and move the state toggle (set(...)) into the try block so it only runs on
success; in the catch block, surface the failure (e.g., call a provided notifier
like deps.notifyError or at minimum console.error with the caught error) so
users get feedback when switchNovelToLibrary fails and ESLint warnings are
resolved.
In `@src/hooks/persisted/useNovel/types.ts`:
- Around line 7-18: Move the NovelSettings interface declaration above the
default values so the type is declared before it’s referenced: place the
NovelSettings interface first, followed by defaultNovelSettings and
defaultPageIndex; update any imports/usages if necessary to reflect the new
order, ensuring the symbols NovelSettings and defaultNovelSettings remain
unchanged.
In `@src/hooks/persisted/useNovel/useChapterOperations.ts`:
- Around line 59-71: Both transformAndSetChapters and extendChapters are
declared async but contain no await and thus return Promise<void>; remove the
unnecessary async keyword from the function declarations for
transformAndSetChapters and extendChapters so they return void; keep their
bodies unchanged and retain the same dependencies (transformChapters and
_setChapters) in the useCallback calls.
- Around line 17-26: The interface UseChapterOperationsParams declares a
chapters field that is never used in the hook; remove the unused property from
UseChapterOperationsParams (delete the chapters: ChapterInfo[] entry) and update
any call sites that construct objects of this type (calls to
useChapterOperations or factories creating UseChapterOperationsParams) to stop
providing chapters, or alternatively, if the data is actually needed, wire the
existing chapters usage into the hook logic (e.g., inside useChapterOperations
reference the passed chapters instead of any other source). Ensure you only
change the interface and its callers or integrate the chapters parameter in the
useChapterOperations function to keep types consistent.
In `@src/hooks/persisted/usePlugins.ts`:
- Around line 53-76: Extract the repeated mapping of devNovelPlugin and
devPagedNovelPlugin into a small helper (e.g., toPluginItem) that converts a
plugin (devNovelPlugin/devPagedNovelPlugin) into a PluginItem by copying id,
name, site, lang, version, url, iconUrl and computing hasSettings as
!!plugin.pluginSettings; then replace the inline objects in the devPlugins array
with calls to that helper (devPlugins, devNovelPlugin, devPagedNovelPlugin,
PluginItem, toPluginItem).
In `@src/plugins/dev/DevPagedNovelPlugin.ts`:
- Around line 252-363: Both parseNovel and parsePage duplicate the
page-calculation logic; extract that logic into a new async helper (e.g.,
calculateTotalPages) that accepts novelPath, newContent, newPages and returns
totalPages by performing the dbManager select on novelSchema, applying
CHAPTERS_PER_PAGE and DEFAULT_PAGES rules and the newPages/newContent
adjustments; then replace the duplicated blocks in parseNovel and parsePage to
call calculateTotalPages(novelPath, newContent, newPages) and use its result for
generating chapters and page bounds checks (references: parseNovel, parsePage,
dbManager, novelSchema, CHAPTERS_PER_PAGE, DEFAULT_PAGES, Storage keys
'provideNewContent' and 'provideNewPages').
In `@src/plugins/pluginManager.ts`:
- Around line 172-174: Extract the duplicate dev plugin ID strings into shared
constants (e.g., DEV_PLUGIN_NOVEL and DEV_PLUGIN_PAGED_NOVEL) and replace the
inline literals in both the registry initialization and the conditional that
checks pluginId in pluginManager.ts; specifically update the check that
currently uses (pluginId === 'dev-novel' || pluginId === 'dev-paged-novel') to
reference the new constants and ensure the registry initialization also uses the
same constants to avoid duplication and typos.
In `@src/screens/novel/__tests__/NovelScreen.test.tsx`:
- Around line 43-48: The test currently asserts behavior on the DB helper
updateChapterProgressByIds which ties the spec to an implementation detail;
instead spy or mock the public store API (novelStore) used by the screen and
assert the store boundary was invoked. Replace expectations that reference
`@database/queries/ChapterQueries.updateChapterProgressByIds` with assertions
against the novelStore method the screen calls (e.g., the store method that
resets/updates chapter progress), and update the mocks in the three affected
blocks (around lines 43-48, 223-239, 290-305) to stop verifying the DB helper
and verify novelStore was called with the expected ids/args; keep the existing
jest.mock of getAllUndownloadedAndUnreadChapters/getAllUndownloadedChapters but
move the progress-reset assertion to the store-level spy.
In `@src/screens/novel/components/NovelScreenList.tsx`:
- Around line 108-130: Multiple calls to useNovelDomainValue (e.g.,
useNovelDomainValue(novelStore, selectChapters), selectDeleteChapter,
selectFetching, selectFirstUnreadChapter, selectLoading, selectNovelSettings,
selectPages, selectSetNovel, selectNovel, selectBatchInformation,
selectPageIndex, selectOpenPage, selectUpdateChapter, selectRefreshNovel,
selectLastRead) create many subscriptions; replace them with a single
consolidated selector (e.g., selectNovelScreenListState) that returns an object
with all required fields, call useNovelDomainValue once with that selector, and
then destructure the returned object into chapters, deleteChapter, fetching,
firstUnreadChapter, loading, novelSettings, pages, setNovel, fetchedNovel,
batchInformation, pageIndex, openPage, updateChapter, refreshNovel, lastRead to
reduce subscription overhead and re-renders.
In `@src/screens/novel/NovelContext.tsx`:
- Around line 45-94: The memo currently includes route.params in the dependency
array which can cause unnecessary re-creation of novelStore; remove route.params
from the deps and instead derive the NovelInfo check outside the useMemo (e.g.
compute a local variable like initialNovel = 'id' in route.params ?
(route.params as NovelInfo) : undefined) and pass that stable value (or just
rely on path and pluginId) into createNovelStore; update the useMemo dependency
array to [defaultChapterSort, path, pluginId, switchNovelToLibrary] so
novelStore (created by createNovelStore) only rebuilds when actual identifying
values change.
In
`@src/screens/reader/components/ChapterDrawer/__tests__/ChapterDrawer.test.tsx`:
- Around line 135-162: Add unit tests in ChapterDrawer.test.tsx that cover the
edge conditions the reviewer suggested: create variants of createStore with
fetching: true, with batchInformation where batch >= total, and with pages empty
or missing the current chapter page; for each test use mockUseChapterContext and
mockUseNovelContext as in the existing test, render <ChapterDrawer />, trigger
the end-reached event via
fireEvent.press(screen.getByTestId('legend-end-reached')), and assert that
store.state.getNextChapterBatch is NOT called; also keep the existing
expectation that setPageIndex is invoked where appropriate to ensure
selector-backed behavior remains tested.
In `@src/screens/reader/components/ChapterDrawer/index.tsx`:
- Around line 43-58: Extract the duplicated hook useNovelDomainValue into a
shared utility and update callers to import it instead of redefining it; create
a new file (e.g., `@hooks/persisted/useNovel/useNovelDomainValue.ts`) that exports
useNovelDomainValue with the same signature and implementation (it should
reference useNovelContext's novelStore, useCallback for subscribe/getSnapshot,
and call novelStore.subscribe and novelStore.getState with the selector), then
replace the local definitions in ChapterDrawer (index.tsx), NovelScreenList.tsx,
NovelScreen.tsx, and useChapter.ts with an import from the new module to remove
duplication and keep behavior identical.
In `@src/utils/mmkv/zustand-adapter.ts`:
- Around line 19-25: Replace the console.error calls inside the mmkv-zustand
adapter error handlers with the project logging utility or explicitly suppress
the lint rule with a justified inline comment: locate the catch blocks in the
mmkv adapter functions (e.g., getItem, setItem, removeItem in
zustand-adapter.ts) and either swap console.error(...) for the shared logger
(processLogger or logger) with the same message and error object, or add an
eslint-disable-next-line no-console comment plus a brief justification above
each console.error (the occurrences at the catch blocks currently around the
key-handling logic and the ones at lines corresponding to getItem, setItem, and
removeItem).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b8c4d30-3941-47a4-9d60-e1a364145a33
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (47)
.github/workflows/build.yml.gitignore__mocks__/react-native-mmkv.jspackage.jsonsrc/hooks/__tests__/mocks.tssrc/hooks/__tests__/mocksContract.test.tssrc/hooks/__tests__/useNovel.test.tssrc/hooks/__tests__/useNovelStore.test.tssrc/hooks/persisted/__mocks__/useNovel.tssrc/hooks/persisted/__mocks__/useNovelSettings.tssrc/hooks/persisted/index.tssrc/hooks/persisted/useNovel.tssrc/hooks/persisted/useNovel/__tests__/bootstrapService.test.tssrc/hooks/persisted/useNovel/__tests__/chapterActions.test.tssrc/hooks/persisted/useNovel/__tests__/keyContract.test.tssrc/hooks/persisted/useNovel/__tests__/persistence.test.tssrc/hooks/persisted/useNovel/__tests__/useNovelSettings.test.tssrc/hooks/persisted/useNovel/bootstrapService.tssrc/hooks/persisted/useNovel/chapterActions.tssrc/hooks/persisted/useNovel/contracts.tssrc/hooks/persisted/useNovel/keyContract.tssrc/hooks/persisted/useNovel/novelStore.tssrc/hooks/persisted/useNovel/persistence.tssrc/hooks/persisted/useNovel/types.tssrc/hooks/persisted/useNovel/useChapterOperations.tssrc/hooks/persisted/useNovelSettings.tssrc/hooks/persisted/usePlugins.tssrc/plugins/dev/DevNovelPlugin.tssrc/plugins/dev/DevPagedNovelPlugin.tssrc/plugins/dev/index.tssrc/plugins/pluginManager.tssrc/screens/browse/components/InstalledTab.tsxsrc/screens/novel/NovelContext.tsxsrc/screens/novel/NovelScreen.tsxsrc/screens/novel/__tests__/NovelScreen.test.tsxsrc/screens/novel/components/Info/NovelInfoHeader.tsxsrc/screens/novel/components/NovelScreenList.tsxsrc/screens/novel/components/__tests__/NovelScreenList.test.tsxsrc/screens/reader/components/ChapterDrawer/__tests__/ChapterDrawer.test.tsxsrc/screens/reader/components/ChapterDrawer/index.tsxsrc/screens/reader/hooks/useChapter.tssrc/screens/settings/SettingsAdvancedScreen.tsxsrc/services/migrate/migrateNovel.tssrc/services/plugin/fetch.tssrc/services/updates/LibraryUpdateQueries.tssrc/utils/mmkv/zustand-adapter.tstsconfig.tsbuildinfo
💤 Files with no reviewable changes (1)
- src/screens/browse/components/InstalledTab.tsx
Comment on lines
+79
to
+101
| const createTupleHook = getter => (key, fallback) => | ||
| [ | ||
| getter(key) ?? fallback, | ||
| jest.fn(value => { | ||
| if (value === undefined) { | ||
| strings.delete(key); | ||
| numbers.delete(key); | ||
| booleans.delete(key); | ||
| buffers.delete(key); | ||
| return; | ||
| } | ||
|
|
||
| if (typeof value === 'string') { | ||
| strings.set(key, value); | ||
| } else if (typeof value === 'number') { | ||
| numbers.set(key, value); | ||
| } else if (typeof value === 'boolean') { | ||
| booleans.set(key, value); | ||
| } else { | ||
| buffers.set(key, value); | ||
| } | ||
| }), | ||
| ]; |
There was a problem hiding this comment.
Hook setters are recreated on each call, losing mock call history.
createTupleHook returns a new jest.fn() on every invocation. This means:
- Each component render gets a different setter reference.
- Mock assertions on the setter won't accumulate across renders.
If tests need to assert on setter calls or rely on referential stability, consider caching the setter per key.
🛡️ Optional fix: Cache setters per key
+const setterCache = new Map();
+
const createTupleHook = getter => (key, fallback) => {
+ if (!setterCache.has(key)) {
+ setterCache.set(key, jest.fn(value => {
+ if (value === undefined) {
+ strings.delete(key);
+ numbers.delete(key);
+ booleans.delete(key);
+ buffers.delete(key);
+ return;
+ }
+ if (typeof value === 'string') {
+ strings.set(key, value);
+ } else if (typeof value === 'number') {
+ numbers.set(key, value);
+ } else if (typeof value === 'boolean') {
+ booleans.set(key, value);
+ } else {
+ buffers.set(key, value);
+ }
+ }));
+ }
return [
getter(key) ?? fallback,
- jest.fn(value => {
- // ... setter logic
- }),
+ setterCache.get(key),
];
};Remember to clear setterCache in a beforeEach if needed.
📝 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.
Suggested change
| const createTupleHook = getter => (key, fallback) => | |
| [ | |
| getter(key) ?? fallback, | |
| jest.fn(value => { | |
| if (value === undefined) { | |
| strings.delete(key); | |
| numbers.delete(key); | |
| booleans.delete(key); | |
| buffers.delete(key); | |
| return; | |
| } | |
| if (typeof value === 'string') { | |
| strings.set(key, value); | |
| } else if (typeof value === 'number') { | |
| numbers.set(key, value); | |
| } else if (typeof value === 'boolean') { | |
| booleans.set(key, value); | |
| } else { | |
| buffers.set(key, value); | |
| } | |
| }), | |
| ]; | |
| const setterCache = new Map(); | |
| const createTupleHook = getter => (key, fallback) => { | |
| if (!setterCache.has(key)) { | |
| setterCache.set(key, jest.fn(value => { | |
| if (value === undefined) { | |
| strings.delete(key); | |
| numbers.delete(key); | |
| booleans.delete(key); | |
| buffers.delete(key); | |
| return; | |
| } | |
| if (typeof value === 'string') { | |
| strings.set(key, value); | |
| } else if (typeof value === 'number') { | |
| numbers.set(key, value); | |
| } else if (typeof value === 'boolean') { | |
| booleans.set(key, value); | |
| } else { | |
| buffers.set(key, value); | |
| } | |
| })); | |
| } | |
| return [ | |
| getter(key) ?? fallback, | |
| setterCache.get(key), | |
| ]; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__mocks__/react-native-mmkv.js` around lines 79 - 101, createTupleHook
currently creates a new jest.fn setter on every call which breaks referential
stability and mock call accumulation; change createTupleHook to look up a
per-key setter in a cache (e.g., setterCache Map) and reuse the same jest.fn for
a given key (creating and storing it only if missing), keeping the existing
setter behavior (deleting maps when value === undefined and type-based sets
otherwise); ensure tests can clear setterCache in beforeEach when needed.
Comment on lines
+117
to
+118
| "urlencode": "^2.0.0", | ||
| "zustand": "^5.0.12" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of zustand npm package?
💡 Result:
The latest version of the zustand npm package is 5.0.12.
Citations:
- 1: https://github.com/pmndrs/zustand/releases
- 2: https://www.npmjs.com/package/zustand
- 3: https://github.com/pmndrs/zustand
- 4: https://www.npmjs.com/package/zustand?activeTab=versions
🌐 Web query:
Zustand 5.0.12 React 19 compatibility
💡 Result:
Zustand 5.0.12 is not officially compatible with React 19. The peer dependency for React is specified as ">=18.0.0", which does not include React 19. Official documentation and changelogs for v5.0.12 do not mention React 19 support or updates to peer dependencies. Early discussions (e.g., Nov 2024) show peer dependency conflicts with React 19 RCs when using npm strict mode, requiring --force or --legacy-peer-deps. While some tools like DepFixer claim compatibility for earlier 5.x versions and Zustand works in practice due to minimal React API usage, strict compatibility requires peer deps alignment, which is absent. For React 19, follow best practices like using selectors with 'use' prefix and useShallow for destructuring to work with React Compiler.
Citations:
- 1: https://www.npmjs.com/package/zustand
- 2: https://github.com/pmndrs/zustand/blob/main/package.json
- 3: https://registry.npmjs.org/zustand
- 4: https://depfixer.com/compatibility/react-19-zustand-5
- 5: Peer Dependency Conflicts between react and zustand when upgrade to next15 stable version. pmndrs/zustand#2843
- 6: https://github.com/pmndrs/zustand/releases/tag/v5.0.12
- 7: https://github.com/pmndrs/zustand/releases
Zustand 5.0.12 is not officially compatible with React 19.
Zustand 5.0.12 is the latest stable version, but its peer dependency specifies >=18.0.0, explicitly excluding React 19. Using it with React 19 will require --force or --legacy-peer-deps flags during installation. If this project targets React 19, either upgrade Zustand once React 19 support is released or retain React 18 compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 117 - 118, The package.json currently lists
"zustand": "^5.0.12" which declares a peer dependency excluding React 19; update
package configuration to avoid broken installs by either (A) pinning the project
React peer/dependency to a 18.x range (so keep "zustand": "^5.0.12") or (B) if
you must target React 19, remove or delay the dependency upgrade and add a TODO
to upgrade "zustand" to a version that advertises React 19 support once released
(or use an explicit override/resolution only as a temporary measure); adjust the
package.json "react"/"react-dom" entries or add a comment/issue referencing
"zustand": "^5.0.12" to document the chosen approach.
Comment on lines
+56
to
+59
| batchInformation: { | ||
| batch: 0, | ||
| total: 0, | ||
| }, |
There was a problem hiding this comment.
Include totalChapters in the shared mock batch state.
The new bootstrap/store flow populates batchInformation.totalChapters. Leaving it out here means tests built on this helper see undefined instead of the production shape.
Possible fix
batchInformation: {
batch: 0,
total: 0,
+ totalChapters: 0,
},📝 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.
Suggested change
| batchInformation: { | |
| batch: 0, | |
| total: 0, | |
| }, | |
| batchInformation: { | |
| batch: 0, | |
| total: 0, | |
| totalChapters: 0, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/__tests__/mocks.ts` around lines 56 - 59, The shared mock object
`batchInformation` is missing the new `totalChapters` property; update the mock
in src/hooks/__tests__/mocks.ts so `batchInformation` includes `totalChapters`
(e.g., set to 0) alongside `batch` and `total` to match the
production/bootstrap/store shape used by the code paths that read
`batchInformation.totalChapters`.
Comment on lines
+98
to
+123
| export const createMockNovelStore = ( | ||
| stateOverrides: Record<string, unknown> = {}, | ||
| ) => { | ||
| let state = createMockNovelStoreState(stateOverrides); | ||
|
|
||
| return { | ||
| getState: jest.fn(() => state), | ||
| setState: jest.fn(nextState => { | ||
| const partial = | ||
| typeof nextState === 'function' ? nextState(state) : nextState; | ||
| state = { | ||
| ...state, | ||
| ...partial, | ||
| }; | ||
| }), | ||
| subscribe: jest.fn(() => () => {}), | ||
| }; | ||
| }; | ||
|
|
||
| const defaultMockNovelContext = { | ||
| novelStore: createMockNovelStore(), | ||
| navigationBarHeight: 0, | ||
| statusBarHeight: 0, | ||
| }; | ||
|
|
||
| export const mockUseNovelContext = jest.fn(() => defaultMockNovelContext); |
There was a problem hiding this comment.
Make the mock store obey the external-store contract.
setState() never notifies subscribers, and the default context reuses a singleton store. With useSyncExternalStore, that can hide rerender bugs and leak mutated state between tests.
Possible fix
export const createMockNovelStore = (
stateOverrides: Record<string, unknown> = {},
) => {
let state = createMockNovelStoreState(stateOverrides);
+ const listeners = new Set<() => void>();
return {
getState: jest.fn(() => state),
setState: jest.fn(nextState => {
const partial =
typeof nextState === 'function' ? nextState(state) : nextState;
state = {
...state,
...partial,
};
+ listeners.forEach(listener => listener());
}),
- subscribe: jest.fn(() => () => {}),
+ subscribe: jest.fn(listener => {
+ listeners.add(listener);
+ return () => listeners.delete(listener);
+ }),
};
};
-const defaultMockNovelContext = {
- novelStore: createMockNovelStore(),
- navigationBarHeight: 0,
- statusBarHeight: 0,
-};
-
-export const mockUseNovelContext = jest.fn(() => defaultMockNovelContext);
+const createDefaultMockNovelContext = () => ({
+ novelStore: createMockNovelStore(),
+ navigationBarHeight: 0,
+ statusBarHeight: 0,
+});
+
+export const mockUseNovelContext = jest.fn(() =>
+ createDefaultMockNovelContext(),
+);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/__tests__/mocks.ts` around lines 98 - 123, The mock store must
implement the external-store contract: update setState to notify subscribers and
make the default context use a fresh store instead of a singleton. In
createMockNovelStore, add an internal listeners array, implement
subscribe(listener) to push the listener and return an unsubscribe function that
removes it, keep getState() returning the current state, and modify
setState(nextState) to compute the new partial state (supporting function or
object), update state, then call each listener(); finally, change
defaultMockNovelContext/mockUseNovelContext so they create a new store instance
by calling createMockNovelStore() when invoked rather than reusing a single
store across tests.
Comment on lines
+37
to
+93
| it('clears tracked novel and legacy persistence keys for each cached novel', async () => { | ||
| for (const novel of cachedNovels) { | ||
| MMKVStorage.set(`${TRACKED_NOVEL_PREFIX}_${novel.id}`, 'tracked'); | ||
| MMKVStorage.set( | ||
| keyContract.pageIndex({ | ||
| pluginId: novel.pluginId, | ||
| novelPath: novel.path, | ||
| }), | ||
| 4, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| // #endregion | ||
| // #region progress | ||
|
|
||
| describe('updateChapterProgress', () => { | ||
| it('updates progress in state and caps at 100', async () => { | ||
| const { result } = await renderUseNovel(); | ||
|
|
||
| act(() => result.current.updateChapterProgress(1, 150)); | ||
|
|
||
| expect(_updateChapterProgress).toHaveBeenCalledWith(1, 100); | ||
| expect(result.current.chapters.find(c => c.id === 1)?.progress).toBe(150); | ||
| }); | ||
|
|
||
| it('stores the raw progress value in state', async () => { | ||
| const { result } = await renderUseNovel(); | ||
|
|
||
| act(() => result.current.updateChapterProgress(1, 42)); | ||
|
|
||
| expect(result.current.chapters.find(c => c.id === 1)?.progress).toBe(42); | ||
| }); | ||
| }); | ||
|
|
||
| // #endregion | ||
| // #region delete | ||
|
|
||
| describe('deleteChapter / deleteChapters', () => { | ||
| it('sets isDownloaded to false after deleteChapter', async () => { | ||
| (_deleteChapter as jest.Mock).mockResolvedValue(undefined); | ||
| const downloaded = mockChapters.map(c => ({ ...c, isDownloaded: true })); | ||
| (getPageChaptersBatched as jest.Mock).mockResolvedValue(downloaded); | ||
|
|
||
| const { result } = await renderUseNovel(); | ||
|
|
||
| act(() => result.current.deleteChapter(downloaded[0])); | ||
|
|
||
| expect(_deleteChapter).toHaveBeenCalledWith(PLUGIN_ID, mockNovel.id, 1); | ||
| await waitFor(() => | ||
| expect( | ||
| result.current.chapters.find(c => c.id === 1)?.isDownloaded, | ||
| ).toBe(false), | ||
| MMKVStorage.set( | ||
| keyContract.settings({ | ||
| pluginId: novel.pluginId, | ||
| novelPath: novel.path, | ||
| }), | ||
| JSON.stringify({ filter: [], showChapterTitles: true }), | ||
| ); | ||
| }); | ||
|
|
||
| it('sets isDownloaded to false for all deleted chapters', async () => { | ||
| (_deleteChapters as jest.Mock).mockResolvedValue(undefined); | ||
| const downloaded = mockChapters.map(c => ({ ...c, isDownloaded: true })); | ||
| (getPageChaptersBatched as jest.Mock).mockResolvedValue(downloaded); | ||
|
|
||
| const { result } = await renderUseNovel(); | ||
|
|
||
| act(() => result.current.deleteChapters([downloaded[0], downloaded[1]])); | ||
|
|
||
| await waitFor(() => | ||
| [1, 2].forEach(id => { | ||
| expect( | ||
| result.current.chapters.find(c => c.id === id)?.isDownloaded, | ||
| ).toBe(false); | ||
| MMKVStorage.set( | ||
| keyContract.lastRead({ | ||
| pluginId: novel.pluginId, | ||
| novelPath: novel.path, | ||
| }), | ||
| JSON.stringify({ id: 1 }), | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| // #endregion | ||
| // #region followNovel | ||
|
|
||
| describe('followNovel', () => { | ||
| it('toggles inLibrary on the novel', async () => { | ||
| const { switchNovelToLibrary } = | ||
| require('@components/Context/LibraryContext').useLibraryContext(); | ||
| (switchNovelToLibrary as jest.Mock).mockResolvedValue(undefined); | ||
|
|
||
| const { result } = await renderUseNovel(); | ||
|
|
||
| const before = result.current.novel?.inLibrary; | ||
| } | ||
|
|
||
| act(() => result.current.followNovel()); | ||
| await deleteCachedNovels(); | ||
|
|
||
| await waitFor(() => | ||
| expect(result.current.novel?.inLibrary).toBe(!before), | ||
| for (const novel of cachedNovels) { | ||
| expect(MMKVStorage.contains(`${TRACKED_NOVEL_PREFIX}_${novel.id}`)).toBe( | ||
| false, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| // #endregion | ||
| }); | ||
|
|
||
| // #region deleteCachedNovels | ||
|
|
||
| describe('deleteCachedNovels', () => { | ||
| const cachedNovels: NovelInfo[] = [ | ||
| { id: 10, pluginId: 'p1', path: '/n/1', name: 'N1', inLibrary: false }, | ||
| { id: 11, pluginId: 'p2', path: '/n/2', name: 'N2', inLibrary: false }, | ||
| ]; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| (_getCachedNovels as jest.Mock).mockResolvedValue(cachedNovels); | ||
| (NativeFile.exists as jest.Mock).mockReturnValue(false); | ||
| expect( | ||
| MMKVStorage.contains( | ||
| keyContract.pageIndex({ | ||
| pluginId: novel.pluginId, | ||
| novelPath: novel.path, | ||
| }), | ||
| ), | ||
| ).toBe(false); | ||
| expect( | ||
| MMKVStorage.contains( | ||
| novelPersistence.keys.settings({ | ||
| pluginId: novel.pluginId, | ||
| novelPath: novel.path, | ||
| }), | ||
| ), | ||
| ).toBe(false); | ||
| expect( | ||
| MMKVStorage.contains( | ||
| novelPersistence.keys.lastRead({ | ||
| pluginId: novel.pluginId, | ||
| novelPath: novel.path, | ||
| }), | ||
| ), | ||
| ).toBe(false); | ||
| } |
There was a problem hiding this comment.
Seed the legacy keys you assert on.
This test writes keyContract.settings/lastRead, then verifies cleanup via novelPersistence.keys.*. If those builders diverge later, the settings/last-read assertions still pass because the legacy keys were never present.
Possible fix
MMKVStorage.set(
keyContract.settings({
pluginId: novel.pluginId,
novelPath: novel.path,
}),
JSON.stringify({ filter: [], showChapterTitles: true }),
);
+ MMKVStorage.set(
+ novelPersistence.keys.settings({
+ pluginId: novel.pluginId,
+ novelPath: novel.path,
+ }),
+ JSON.stringify({ filter: [], showChapterTitles: true }),
+ );
MMKVStorage.set(
keyContract.lastRead({
pluginId: novel.pluginId,
novelPath: novel.path,
}),
JSON.stringify({ id: 1 }),
);
+ MMKVStorage.set(
+ novelPersistence.keys.lastRead({
+ pluginId: novel.pluginId,
+ novelPath: novel.path,
+ }),
+ JSON.stringify({ id: 1 }),
+ );📝 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.
Suggested change
| it('clears tracked novel and legacy persistence keys for each cached novel', async () => { | |
| for (const novel of cachedNovels) { | |
| MMKVStorage.set(`${TRACKED_NOVEL_PREFIX}_${novel.id}`, 'tracked'); | |
| MMKVStorage.set( | |
| keyContract.pageIndex({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| 4, | |
| ); | |
| }); | |
| }); | |
| // #endregion | |
| // #region progress | |
| describe('updateChapterProgress', () => { | |
| it('updates progress in state and caps at 100', async () => { | |
| const { result } = await renderUseNovel(); | |
| act(() => result.current.updateChapterProgress(1, 150)); | |
| expect(_updateChapterProgress).toHaveBeenCalledWith(1, 100); | |
| expect(result.current.chapters.find(c => c.id === 1)?.progress).toBe(150); | |
| }); | |
| it('stores the raw progress value in state', async () => { | |
| const { result } = await renderUseNovel(); | |
| act(() => result.current.updateChapterProgress(1, 42)); | |
| expect(result.current.chapters.find(c => c.id === 1)?.progress).toBe(42); | |
| }); | |
| }); | |
| // #endregion | |
| // #region delete | |
| describe('deleteChapter / deleteChapters', () => { | |
| it('sets isDownloaded to false after deleteChapter', async () => { | |
| (_deleteChapter as jest.Mock).mockResolvedValue(undefined); | |
| const downloaded = mockChapters.map(c => ({ ...c, isDownloaded: true })); | |
| (getPageChaptersBatched as jest.Mock).mockResolvedValue(downloaded); | |
| const { result } = await renderUseNovel(); | |
| act(() => result.current.deleteChapter(downloaded[0])); | |
| expect(_deleteChapter).toHaveBeenCalledWith(PLUGIN_ID, mockNovel.id, 1); | |
| await waitFor(() => | |
| expect( | |
| result.current.chapters.find(c => c.id === 1)?.isDownloaded, | |
| ).toBe(false), | |
| MMKVStorage.set( | |
| keyContract.settings({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| JSON.stringify({ filter: [], showChapterTitles: true }), | |
| ); | |
| }); | |
| it('sets isDownloaded to false for all deleted chapters', async () => { | |
| (_deleteChapters as jest.Mock).mockResolvedValue(undefined); | |
| const downloaded = mockChapters.map(c => ({ ...c, isDownloaded: true })); | |
| (getPageChaptersBatched as jest.Mock).mockResolvedValue(downloaded); | |
| const { result } = await renderUseNovel(); | |
| act(() => result.current.deleteChapters([downloaded[0], downloaded[1]])); | |
| await waitFor(() => | |
| [1, 2].forEach(id => { | |
| expect( | |
| result.current.chapters.find(c => c.id === id)?.isDownloaded, | |
| ).toBe(false); | |
| MMKVStorage.set( | |
| keyContract.lastRead({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| JSON.stringify({ id: 1 }), | |
| ); | |
| }); | |
| }); | |
| // #endregion | |
| // #region followNovel | |
| describe('followNovel', () => { | |
| it('toggles inLibrary on the novel', async () => { | |
| const { switchNovelToLibrary } = | |
| require('@components/Context/LibraryContext').useLibraryContext(); | |
| (switchNovelToLibrary as jest.Mock).mockResolvedValue(undefined); | |
| const { result } = await renderUseNovel(); | |
| const before = result.current.novel?.inLibrary; | |
| } | |
| act(() => result.current.followNovel()); | |
| await deleteCachedNovels(); | |
| await waitFor(() => | |
| expect(result.current.novel?.inLibrary).toBe(!before), | |
| for (const novel of cachedNovels) { | |
| expect(MMKVStorage.contains(`${TRACKED_NOVEL_PREFIX}_${novel.id}`)).toBe( | |
| false, | |
| ); | |
| }); | |
| }); | |
| // #endregion | |
| }); | |
| // #region deleteCachedNovels | |
| describe('deleteCachedNovels', () => { | |
| const cachedNovels: NovelInfo[] = [ | |
| { id: 10, pluginId: 'p1', path: '/n/1', name: 'N1', inLibrary: false }, | |
| { id: 11, pluginId: 'p2', path: '/n/2', name: 'N2', inLibrary: false }, | |
| ]; | |
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| (_getCachedNovels as jest.Mock).mockResolvedValue(cachedNovels); | |
| (NativeFile.exists as jest.Mock).mockReturnValue(false); | |
| expect( | |
| MMKVStorage.contains( | |
| keyContract.pageIndex({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| ), | |
| ).toBe(false); | |
| expect( | |
| MMKVStorage.contains( | |
| novelPersistence.keys.settings({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| ), | |
| ).toBe(false); | |
| expect( | |
| MMKVStorage.contains( | |
| novelPersistence.keys.lastRead({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| ), | |
| ).toBe(false); | |
| } | |
| it('clears tracked novel and legacy persistence keys for each cached novel', async () => { | |
| for (const novel of cachedNovels) { | |
| MMKVStorage.set(`${TRACKED_NOVEL_PREFIX}_${novel.id}`, 'tracked'); | |
| MMKVStorage.set( | |
| keyContract.pageIndex({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| 4, | |
| ); | |
| MMKVStorage.set( | |
| keyContract.settings({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| JSON.stringify({ filter: [], showChapterTitles: true }), | |
| ); | |
| MMKVStorage.set( | |
| novelPersistence.keys.settings({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| JSON.stringify({ filter: [], showChapterTitles: true }), | |
| ); | |
| MMKVStorage.set( | |
| keyContract.lastRead({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| JSON.stringify({ id: 1 }), | |
| ); | |
| MMKVStorage.set( | |
| novelPersistence.keys.lastRead({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| JSON.stringify({ id: 1 }), | |
| ); | |
| } | |
| await deleteCachedNovels(); | |
| for (const novel of cachedNovels) { | |
| expect(MMKVStorage.contains(`${TRACKED_NOVEL_PREFIX}_${novel.id}`)).toBe( | |
| false, | |
| ); | |
| expect( | |
| MMKVStorage.contains( | |
| keyContract.pageIndex({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| ), | |
| ).toBe(false); | |
| expect( | |
| MMKVStorage.contains( | |
| novelPersistence.keys.settings({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| ), | |
| ).toBe(false); | |
| expect( | |
| MMKVStorage.contains( | |
| novelPersistence.keys.lastRead({ | |
| pluginId: novel.pluginId, | |
| novelPath: novel.path, | |
| }), | |
| ), | |
| ).toBe(false); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/__tests__/useNovel.test.ts` around lines 37 - 93, The test seeds
only keyContract.settings/lastRead but asserts that
novelPersistence.keys.settings/lastRead are removed; update the setup in the
test for each novel in cachedNovels to also write the legacy keys using
MMKVStorage.set with novelPersistence.keys.settings({...}) and
novelPersistence.keys.lastRead({...}) (and the tracked key via
TRACKED_NOVEL_PREFIX remains), so deleteCachedNovels is exercised against both
current and legacy key builders; keep the rest of the assertions unchanged.
Comment on lines
+72
to
+97
| const readPageIndex = (input: NovelPersistenceInput): number => { | ||
| const key = keys.pageIndex(input); | ||
| const numberValue = storage.getNumber(key); | ||
|
|
||
| if (typeof numberValue === 'number' && Number.isFinite(numberValue)) { | ||
| return numberValue; | ||
| } | ||
|
|
||
| const stringValue = storage.getString(key); | ||
| if (stringValue !== undefined) { | ||
| const parsed = Number(stringValue); | ||
| if (Number.isFinite(parsed)) { | ||
| storage.set(key, parsed); | ||
| return parsed; | ||
| } | ||
| } | ||
|
|
||
| storage.set(key, defaultPageIndex); | ||
| return defaultPageIndex; | ||
| }; | ||
|
|
||
| const writePageIndex = (input: NovelPersistenceInput, value: number) => { | ||
| const key = keys.pageIndex(input); | ||
| const safeValue = Number.isFinite(value) ? value : defaultPageIndex; | ||
| storage.set(key, safeValue); | ||
| }; |
There was a problem hiding this comment.
Reject negative or fractional page indices.
Number.isFinite still accepts values like -1 and 1.5, so the bridge can persist/read indices that later make pages[pageIndex] resolve to undefined and silently fall back to page '1'. Normalize both legacy-string parsing and new writes to a non-negative integer.
🛠️ Proposed fix
const readPageIndex = (input: NovelPersistenceInput): number => {
const key = keys.pageIndex(input);
const numberValue = storage.getNumber(key);
- if (typeof numberValue === 'number' && Number.isFinite(numberValue)) {
+ if (
+ typeof numberValue === 'number' &&
+ Number.isInteger(numberValue) &&
+ numberValue >= 0
+ ) {
return numberValue;
}
const stringValue = storage.getString(key);
if (stringValue !== undefined) {
const parsed = Number(stringValue);
- if (Number.isFinite(parsed)) {
+ if (Number.isInteger(parsed) && parsed >= 0) {
storage.set(key, parsed);
return parsed;
}
}
storage.set(key, defaultPageIndex);
return defaultPageIndex;
};
const writePageIndex = (input: NovelPersistenceInput, value: number) => {
const key = keys.pageIndex(input);
- const safeValue = Number.isFinite(value) ? value : defaultPageIndex;
+ const safeValue =
+ Number.isInteger(value) && value >= 0 ? value : defaultPageIndex;
storage.set(key, safeValue);
};📝 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.
Suggested change
| const readPageIndex = (input: NovelPersistenceInput): number => { | |
| const key = keys.pageIndex(input); | |
| const numberValue = storage.getNumber(key); | |
| if (typeof numberValue === 'number' && Number.isFinite(numberValue)) { | |
| return numberValue; | |
| } | |
| const stringValue = storage.getString(key); | |
| if (stringValue !== undefined) { | |
| const parsed = Number(stringValue); | |
| if (Number.isFinite(parsed)) { | |
| storage.set(key, parsed); | |
| return parsed; | |
| } | |
| } | |
| storage.set(key, defaultPageIndex); | |
| return defaultPageIndex; | |
| }; | |
| const writePageIndex = (input: NovelPersistenceInput, value: number) => { | |
| const key = keys.pageIndex(input); | |
| const safeValue = Number.isFinite(value) ? value : defaultPageIndex; | |
| storage.set(key, safeValue); | |
| }; | |
| const readPageIndex = (input: NovelPersistenceInput): number => { | |
| const key = keys.pageIndex(input); | |
| const numberValue = storage.getNumber(key); | |
| if ( | |
| typeof numberValue === 'number' && | |
| Number.isInteger(numberValue) && | |
| numberValue >= 0 | |
| ) { | |
| return numberValue; | |
| } | |
| const stringValue = storage.getString(key); | |
| if (stringValue !== undefined) { | |
| const parsed = Number(stringValue); | |
| if (Number.isInteger(parsed) && parsed >= 0) { | |
| storage.set(key, parsed); | |
| return parsed; | |
| } | |
| } | |
| storage.set(key, defaultPageIndex); | |
| return defaultPageIndex; | |
| }; | |
| const writePageIndex = (input: NovelPersistenceInput, value: number) => { | |
| const key = keys.pageIndex(input); | |
| const safeValue = | |
| Number.isInteger(value) && value >= 0 ? value : defaultPageIndex; | |
| storage.set(key, safeValue); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/persisted/useNovel/persistence.ts` around lines 72 - 97,
readPageIndex and writePageIndex currently accept negative or fractional
numbers; update both to enforce a non-negative integer: in readPageIndex (using
keys.pageIndex, storage.getNumber and storage.getString) treat any numeric value
as valid only if Number.isFinite and >= 0, then normalize by taking Math.floor
(and persist the floored value with storage.set) before returning; for parsed
string values do the same (Number(...) -> check finite and >= 0 -> Math.floor ->
persist -> return); otherwise fallback to storage.set(key, defaultPageIndex) and
return defaultPageIndex; in writePageIndex apply the same
validation/normalization (Number.isFinite(value) && value >= 0 ?
Math.floor(value) : defaultPageIndex) and persist that with storage.set.
Comment on lines
+234
to
+250
| const s = new Storage('dev-novel'); | ||
| const newChapters = s.get('provideNewContent'); | ||
|
|
||
| const res = await dbManager | ||
| .select({ | ||
| count: novelSchema.totalChapters, | ||
| }) | ||
| .from(novelSchema) | ||
| .where( | ||
| and( | ||
| eq(novelSchema.pluginId, 'dev-novel'), | ||
| eq(novelSchema.path, novelPath), | ||
| ), | ||
| ) | ||
| .get(); | ||
| const chapterCount = !res?.count || res.count < 20 ? 20 : res?.count; | ||
| const nextChapterCount = newChapters ? chapterCount + 5 : chapterCount; |
There was a problem hiding this comment.
Only add “new content” when the novel already exists.
With the current math, a brand-new novel starts at 25 chapters when provideNewContent is enabled, because the base count is normalized to 20 before the extra + 5. That does not match the setting label “on update”.
Possible fix
const res = await dbManager
.select({
count: novelSchema.totalChapters,
})
.from(novelSchema)
@@
)
.get();
- const chapterCount = !res?.count || res.count < 20 ? 20 : res?.count;
- const nextChapterCount = newChapters ? chapterCount + 5 : chapterCount;
+ const existingChapterCount = res?.count ?? 0;
+ const chapterCount =
+ existingChapterCount < 20 ? 20 : existingChapterCount;
+ const nextChapterCount =
+ newChapters && existingChapterCount > 0
+ ? chapterCount + 5
+ : chapterCount;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/dev/DevNovelPlugin.ts` around lines 234 - 250, The code currently
applies the "+5 new content" bump regardless of whether the novel exists because
chapterCount is normalized to at least 20 before applying newChapters; change
the logic so the +5 is only applied when a real row/count exists for that novel.
Concretely, keep retrieving res via dbManager.select(...) and compute a
baseCount from res (e.g., const baseCount = res?.count ?? 0), derive
chapterCount = baseCount < 20 && baseCount > 0 ? 20 : Math.max(baseCount, 0) (or
keep existing normalization but only when baseCount>0), and set nextChapterCount
= newChapters && baseCount > 0 ? chapterCount + 5 : chapterCount so that
provideNewContent only adds chapters when the novel already exists (use the
symbols s, newChapters, res, novelSchema, chapterCount, nextChapterCount to
locate the code).
src/screens/novel/NovelScreen.tsx
Outdated
Comment on lines
+59
to
+60
| const selectMarkPreviouschaptersRead = (state: NovelStoreState) => | ||
| state.markPreviouschaptersRead; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the store property uses consistent casing
ast-grep --pattern 'markPreviouschaptersRead'
ast-grep --pattern 'markPreviousChaptersRead'Repository: CD-Z/lnreader
Length of output: 414
🏁 Script executed:
# Check if selectMarkPreviousChaptersUnread exists
rg -i "selectmarkpreviouschapters" --type ts --type tsx -A 2 -B 2Repository: CD-Z/lnreader
Length of output: 84
🏁 Script executed:
# Also search for any "Unread" related selectors and store properties
rg "PreviousChapters" --type ts --type tsx -A 1 -B 1Repository: CD-Z/lnreader
Length of output: 84
🏁 Script executed:
# Look at the store definition to understand property naming
fd "store" --type ts --type tsx | xargs rg "markPrevious" -A 1 -B 1Repository: CD-Z/lnreader
Length of output: 11762
Fix inconsistent casing in selector and store property names.
selectMarkPreviouschaptersRead and the underlying markPreviouschaptersRead property use lowercase 'c' in "chapters", while selectMarkPreviousChaptersUnread uses uppercase 'C'. Standardize to uppercase 'C' for consistency.
Changes needed:
- Rename
selectMarkPreviouschaptersReadtoselectMarkPreviousChaptersRead(line 59) - Rename
markPreviouschaptersReadproperty in store (novelStore.ts) - Rename
markPreviouschaptersReadActionfunction (chapterActions.ts) - Update all usages across the codebase
🔤 Proposed fix for selector
-const selectMarkPreviouschaptersRead = (state: NovelStoreState) =>
- state.markPreviouschaptersRead;
+const selectMarkPreviousChaptersRead = (state: NovelStoreState) =>
+ state.markPreviousChaptersRead;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/novel/NovelScreen.tsx` around lines 59 - 60, The selector and
store/action names have inconsistent casing for "Chapters": rename the selector
selectMarkPreviouschaptersRead to selectMarkPreviousChaptersRead, rename the
store property markPreviouschaptersRead to markPreviousChaptersRead, and rename
the action markPreviouschaptersReadAction to markPreviousChaptersReadAction;
update all references/usages (selectors, dispatches, imports/exports, tests) to
the new names so identifiers match, and run type checks to fix any remaining
imports or compilation errors.
Comment on lines
+48
to
+53
| if (plugin.parseNovel && page === '1') { | ||
| const res = await plugin.parseNovel(novelPath); | ||
| if (res.chapters) return { chapters: res.chapters }; | ||
| throw new Error(`Could not fetch chapters for novel ${novelPath}`); | ||
| } | ||
| throw new Error(`Could not fetch chapters for novel ${novelPath}`); |
There was a problem hiding this comment.
Differentiate the unsupported non-first-page case.
When parsePage is missing and the caller asks for page > 1, this throws as if the novel fetch failed. The real failure is that paged chapter fetching is unsupported, which is much more actionable for callers and logs.
Possible fix
if (plugin.parseNovel && page === '1') {
const res = await plugin.parseNovel(novelPath);
if (res.chapters) return { chapters: res.chapters };
throw new Error(`Could not fetch chapters for novel ${novelPath}`);
}
- throw new Error(`Could not fetch chapters for novel ${novelPath}`);
+ throw new Error(
+ `Plugin ${pluginId} cannot fetch chapters for page ${page} of ${novelPath} without parsePage()`,
+ );📝 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.
Suggested change
| if (plugin.parseNovel && page === '1') { | |
| const res = await plugin.parseNovel(novelPath); | |
| if (res.chapters) return { chapters: res.chapters }; | |
| throw new Error(`Could not fetch chapters for novel ${novelPath}`); | |
| } | |
| throw new Error(`Could not fetch chapters for novel ${novelPath}`); | |
| if (plugin.parseNovel && page === '1') { | |
| const res = await plugin.parseNovel(novelPath); | |
| if (res.chapters) return { chapters: res.chapters }; | |
| throw new Error(`Could not fetch chapters for novel ${novelPath}`); | |
| } | |
| throw new Error( | |
| `Plugin ${pluginId} cannot fetch chapters for page ${page} of ${novelPath} without parsePage()`, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/plugin/fetch.ts` around lines 48 - 53, The code currently treats
any non-first-page request as a generic "could not fetch chapters" error; update
the logic in the fetch path that uses plugin.parseNovel so that when page !==
'1' (or numeric page > 1) and plugin.parsePage is not implemented, you throw a
distinct error indicating "paged chapter fetching is unsupported" (or an
UnsupportedPaginationError) instead of the generic failure; keep the existing
behavior of calling plugin.parseNovel(novelPath) and returning res.chapters when
page === '1' and parseNovel exists, but add an explicit branch that checks
plugin.parsePage (or its absence) and page to produce the clearer error
referencing plugin.parsePage, page and novelPath.
| await updateNovelTotalPages(novelId, novel.totalPages); | ||
| } | ||
|
|
||
| console.log(novel.chapters); |
There was a problem hiding this comment.
Remove debug console logging from production update flow.
Line 197 adds a raw console.log, which violates the enforced no-console rule (current ESLint failure) and introduces runtime log noise in the library update path. Please remove it or route through a dev-gated logger.
Proposed fix
- console.log(novel.chapters);📝 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.
Suggested change
| console.log(novel.chapters); |
🧰 Tools
🪛 ESLint
[error] 197-197: Unexpected console statement.
(no-console)
🪛 GitHub Check: ESLint
[failure] 197-197:
Unexpected console statement
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/updates/LibraryUpdateQueries.ts` at line 197, Remove the debug
console logging statement console.log(novel.chapters) from the
LibraryUpdateQueries update flow; either delete it or replace it with a gated
logger call (e.g., processLogger.debug or similar) behind a dev-only condition
(NODE_ENV or a feature flag) so production no-console rules and runtime noise
are avoided; locate the statement where the novel variable is handled in
LibraryUpdateQueries and update that block accordingly.
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/novel/NovelScreen.tsx (1)
40-56:⚠️ Potential issue | 🔴 CriticalUpdate all Jest mocks to export
useNovelValueanduseNovelActionsinstead of the non-existentuseNovelContext.The mocks in test files reference
useNovelContext, which is not actually exported fromNovelContext.tsx. The actual exports areuseNovelValueanduseNovelActions. This mismatch causes test failures withTypeError: useNovelValue is not a function.Update the following mock definitions to match the real export surface:
src/screens/novel/__tests__/NovelScreen.test.tsx:31src/screens/novel/components/__tests__/NovelScreenList.test.tsx:9src/hooks/__tests__/mocks.ts:125src/screens/reader/hooks/__tests__/useChapter.test.ts:23src/screens/reader/components/ChapterDrawer/__tests__/ChapterDrawer.test.tsx:13src/hooks/persisted/useNovel/__tests__/useNovelSettings.test.ts:6Each mock should export
useNovelValueanduseNovelActionsmatching the signatures inNovelContext.tsx(e.g.,useNovelValue: (key) => mockValue(key)anduseNovelActions: () => mockActions()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/novel/NovelScreen.tsx` around lines 40 - 56, Update each Jest mock module that currently exports useNovelContext to instead export two functions: useNovelValue and useNovelActions; implement useNovelValue as a function that accepts a key and returns the corresponding mock value (e.g., return mockValue(key)) and implement useNovelActions as a function that returns the mock actions object (e.g., return mockActions()); ensure the exported signatures match the real NovelContext hooks used in production (useNovelValue(key) and useNovelActions()) and replace any references to useNovelContext in tests with these new exports so tests no longer attempt to call a non-existent function.
🧹 Nitpick comments (2)
src/hooks/persisted/useNovelSettings.ts (1)
37-48:setChapterSortdependency array may cause stale closures.The callback references
novelSettings?.showChapterTitlesand_filter, butnovelSettingsobject itself changes on every settings update. Consider whether including the entirenovelSettingsobject would be more robust.🤖 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 37 - 48, The setChapterSort callback can capture stale values because it depends on novelSettings?.showChapterTitles and _filter while novelSettings is a new object on each update; update the dependency array to include novelSettings (the whole object) and _filter so the hook re-creates when settings change, and keep using novelSettings?.showChapterTitles inside the body when calling writeNovelSettings; ensure the dependency list reads [novel, writeNovelSettings, novelSettings, _filter] targeting the setChapterSort function.src/screens/novel/NovelContext.tsx (1)
157-163: Consider whetherlayoutValuememoization is correct.The
useMemohas an empty dependency array, but the value depends onnavigationBarHeightRef.currentandstatusBarHeightRef.currentwhich may change. Since refs don't trigger re-renders, this memo will only compute once. If this is intentional (to capture initial layout values), this is fine. Otherwise, consider if the layout needs to update when insets change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/novel/NovelContext.tsx` around lines 157 - 163, layoutValue is memoized with an empty dependency array but reads navigationBarHeightRef.current and statusBarHeightRef.current, so it won't update when those refs change; either include the values that drive layoutValue in the dependency list (e.g. navigationBarHeightRef.current and statusBarHeightRef.current) or convert the refs into state and update that state when insets change, then memoize using that state; update the code around layoutValue, navigationBarHeightRef, statusBarHeightRef and the useMemo call accordingly so layoutValue reflects changes.
🤖 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/manager/manager.ts`:
- Around line 146-149: The current code calls the provided callback directly
from the useState initializer and from effects with callback excluded from deps,
causing render-time invocation and stale/unsafe closures; fix this by storing
the latest callback in a ref (e.g. callbackRef.current) and updating it inside
an effect, remove direct calls to callback from the useState initializer (do not
call callback from the db.executeSync initialiser), and invoke
callbackRef.current only from committed effects/subscriptions (and from the live
query subscription handlers) so calls are cancelled on unmount; apply the same
pattern to useLiveQueryAsync by moving the initial async callback into an effect
with cancellation and using callbackRef.current for all subsequent invocations
to avoid stale closures.
In `@src/database/queries/NovelQueries.ts`:
- Around line 85-89: The change made getNovelById into a synchronous/blocking
call by using dbManager.getSync; restore the original async contract or add an
explicit sync helper: revert getNovelById to return a Promise (use the
async/dbManager.get or the async API around
dbManager.select().from(novelSchema).where(eq(novelSchema.id, novelId))) so
existing await callers (e.g., downloadChapter.ts and tests) remain async, and if
a sync variant is needed add a new function named getNovelByIdSync that uses
dbManager.getSync; make sure to update or leave callers unchanged depending on
which route you pick.
In `@src/hooks/persisted/useNovel/__tests__/novelStore.chapterState.test.ts`:
- Around line 82-94: The tests assume remove() returns a boolean but the current
API (ChapterTextCacheApi.remove) and its implementation
(novelStore.chapterActions.ts remove) return void; change the tests that call
createChapterTextCache().remove(...) (including the other occurrences around
lines 118-123) to assert post-removal state instead of a boolean — e.g., call
cache.remove(id) then assert cache.read(id) is undefined or cache.has(id) is
false (or that the cache size changed) so the tests align with the existing
remove() contract rather than changing the API/implementation.
- Line 1: The test imports createChapterTextCache from the wrong module
(novelStore.chapterState) which only exports createInitialChapterSlice; update
the import to pull createChapterTextCache from the module that actually exports
it (the module that defines the cache factory) so createChapterTextCache is not
undefined and tests can run; keep the existing import for
createInitialChapterSlice as-is if needed and replace only the source module for
createChapterTextCache.
In `@src/hooks/persisted/useNovel/bootstrapService.ts`:
- Line 274: Remove the debug timing calls: locate the console.time and
console.timeEnd usages (they use the key `bootstrap_${pluginId}_${novelPath}`
and reference the variables pluginId and novelPath) and delete them or replace
them with the project's logger (e.g., processLogger.debug/info) emitting a
single log entry with timing info if timing is required; ensure you update both
occurrences (the one at the start and the corresponding console.timeEnd) so no
raw console.* calls remain.
- Around line 326-332: The returned service object is currently filled with
no-op stubs; replace those stubs by returning the real implementations defined
above—specifically return getChaptersForPage, getNextChapterBatch,
loadUpToBatch, and bootstrapNovel (the actual functions defined earlier in this
module) instead of empty arrow functions so the bootstrap service becomes
functional.
In `@src/hooks/persisted/useNovel/novelStore.actions.ts`:
- Around line 160-180: The followNovel action is double-toggling the inLibrary
flag: deps.switchNovelToLibrary already updates DB and refetches, so remove the
local manual toggle inside set(...) in followNovel; instead call
deps.switchNovelToLibrary(state.novelPath, state.pluginId) and do not modify
inner.novel.inLibrary—let the refetch/update flow reconcile the store, or
alternatively change switchNovelToLibrary to return the new novel state and
assign that returned value to novel if you prefer immediate local update (use
symbols: followNovel, deps.switchNovelToLibrary, get, set, inner.novel,
inLibrary, state.novelPath, state.pluginId).
In `@src/hooks/persisted/useNovel/novelStore.chapterActions.ts`:
- Line 2: The import line in novelStore.chapterActions.ts includes unused type
imports (remove NovelInfo and BatchInfo) — keep only ChapterInfo (and any other
actually used symbols) in the import from '@database/types'; update the import
statement that currently references ChapterInfo, NovelInfo (and BatchInfo) so
ESLint no longer flags unused imports and rely on the function signatures'
inferred types instead.
In `@src/hooks/persisted/useNovelSettings.ts`:
- Around line 19-28: The test's Jest mock for NovelContext only exports
useNovelContext but the hook under test (useNovelSettings) imports useNovelValue
and useNovelAction; update the mock in
src/hooks/persisted/useNovel/__tests__/useNovelSettings.test.ts to also export
useNovelValue and useNovelAction so calls in useNovelSettings resolve during
tests (implement them to read from the mocked novel store/state and actions
similarly to useNovelContext's mock).
In `@src/screens/novel/components/JumpToChapterModal.tsx`:
- Around line 43-47: The code sets maxNumber = batchInformation.totalChapters ??
-1 which produces an invalid upper bound; change maxNumber to be optional (e.g.,
undefined) when batchInformation.totalChapters is not present and update all
logic that uses maxNumber (search validation, numeric parse branches,
placeholder generation) in JumpToChapterModal to treat the upper bound as
unknown rather than -1 so numeric searches still work against loadedChapters and
the placeholder doesn't show "≤ -1"; locate references to maxNumber and the
functions that rely on it (e.g., the numeric matching code around lines ~131-150
and the loadUpToBatch invocation) and branch on maxNumber being undefined to
skip upper-bound checks and render a suitable placeholder text.
In `@src/screens/novel/components/NovelScreenList.tsx`:
- Line 481: Remove the temporary render-time debug log that violates no-console:
delete the console.log('rendering chapter', fetching, chapters.length) statement
inside the NovelScreenList component (the render function where variables
fetching and chapters are referenced) or replace it with a conditional debug
logger tied to environment/config; ensure no direct console.log calls remain in
NovelScreenList.
In `@src/screens/novel/NovelContext.tsx`:
- Line 46: Remove the debug timing statements in NovelContext.tsx — delete
console.time('novel contex') and all other
console.time*/console.timeLog*/console.timeEnd* occurrences (notably the groups
around the earlier suggested lines) and, if timing is needed, replace them with
calls to the app's logging utility or a debug logger that respects NODE_ENV so
these traces are disabled in production; locate the statements by searching for
console.time, console.timeLog and console.timeEnd within the NovelContext
component and update uses accordingly.
- Line 4: Remove the unused imports from NovelContext.tsx: delete useEffect,
useLiveQuery, and useLiveQueryAsync from the import list so only actually used
hooks remain; specifically update the import statement that currently includes
useEffect and the live query hooks (look for identifiers useEffect,
useLiveQuery, useLiveQueryAsync) and remove them to eliminate unused-import
warnings.
In `@src/screens/novel/NovelScreen.tsx`:
- Around line 1-8: Remove the temporary debug code in NovelScreen.tsx: delete
the unused import useSyncExternalStore from the React import list and remove the
render-time console.log calls around lines 44-46 (the inline debug logs in the
NovelScreen component) so the component no longer violates ESLint rules; adjust
the import statement to only include the actually used hooks (Suspense,
useCallback, useMemo, useRef, useState) to keep imports clean.
In `@src/screens/reader/hooks/__tests__/useChapter.test.ts`:
- Around line 27-32: The test currently mocks the wrong hook (useNovelContext)
and injects { novelStore: store }; instead mock useNovelActions from
`@screens/novel/NovelContext` and have those mocks return the actual action
methods the hook expects (the same named functions you destructure from
useNovelActions in the hook under test) instead of a novelStore object; locate
the mock definitions currently returning { novelStore: store } and replace them
so useNovelActions: () => ({ /* implement the action methods used in the hook
(same names as destructured) as jest.fn() or pointing to your test stubs */ })
so the hook receives the expected API.
---
Outside diff comments:
In `@src/screens/novel/NovelScreen.tsx`:
- Around line 40-56: Update each Jest mock module that currently exports
useNovelContext to instead export two functions: useNovelValue and
useNovelActions; implement useNovelValue as a function that accepts a key and
returns the corresponding mock value (e.g., return mockValue(key)) and implement
useNovelActions as a function that returns the mock actions object (e.g., return
mockActions()); ensure the exported signatures match the real NovelContext hooks
used in production (useNovelValue(key) and useNovelActions()) and replace any
references to useNovelContext in tests with these new exports so tests no longer
attempt to call a non-existent function.
---
Nitpick comments:
In `@src/hooks/persisted/useNovelSettings.ts`:
- Around line 37-48: The setChapterSort callback can capture stale values
because it depends on novelSettings?.showChapterTitles and _filter while
novelSettings is a new object on each update; update the dependency array to
include novelSettings (the whole object) and _filter so the hook re-creates when
settings change, and keep using novelSettings?.showChapterTitles inside the body
when calling writeNovelSettings; ensure the dependency list reads [novel,
writeNovelSettings, novelSettings, _filter] targeting the setChapterSort
function.
In `@src/screens/novel/NovelContext.tsx`:
- Around line 157-163: layoutValue is memoized with an empty dependency array
but reads navigationBarHeightRef.current and statusBarHeightRef.current, so it
won't update when those refs change; either include the values that drive
layoutValue in the dependency list (e.g. navigationBarHeightRef.current and
statusBarHeightRef.current) or convert the refs into state and update that state
when insets change, then memoize using that state; update the code around
layoutValue, navigationBarHeightRef, statusBarHeightRef and the useMemo call
accordingly so layoutValue reflects changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89fa0555-ac25-49a7-a7ad-d21e07621f74
📒 Files selected for processing (22)
src/database/manager/manager.tssrc/database/queries/NovelQueries.tssrc/hooks/__tests__/useNovelStore.test.tssrc/hooks/persisted/useNovel/__tests__/novelStore.chapterActions.test.tssrc/hooks/persisted/useNovel/__tests__/novelStore.chapterState.test.tssrc/hooks/persisted/useNovel/bootstrapService.tssrc/hooks/persisted/useNovel/novelStore.actions.tssrc/hooks/persisted/useNovel/novelStore.chapterActions.tssrc/hooks/persisted/useNovel/novelStore.chapterState.tssrc/hooks/persisted/useNovel/novelStore.tssrc/hooks/persisted/useNovel/novelStore.types.tssrc/hooks/persisted/useNovelSettings.tssrc/screens/novel/NovelContext.tsxsrc/screens/novel/NovelScreen.tsxsrc/screens/novel/components/Info/NovelInfoHeader.tsxsrc/screens/novel/components/JumpToChapterModal.tsxsrc/screens/novel/components/NovelScreenList.tsxsrc/screens/reader/components/ChapterDrawer/index.tsxsrc/screens/reader/components/ReaderAppbar.tsxsrc/screens/reader/components/ReaderFooter.tsxsrc/screens/reader/hooks/__tests__/useChapter.test.tssrc/screens/reader/hooks/useChapter.ts
✅ Files skipped from review due to trivial changes (2)
- src/hooks/tests/useNovelStore.test.ts
- src/hooks/persisted/useNovel/novelStore.types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/screens/reader/components/ChapterDrawer/index.tsx
- src/screens/reader/hooks/useChapter.ts
- src/screens/novel/components/Info/NovelInfoHeader.tsx
Comment on lines
+146
to
+149
| const [data, setData] = useState<ReturnValue>(() => { | ||
| const r = db.executeSync(sqlString, params as any[]).rows as ReturnValue; | ||
| if (callback) callback(r); | ||
| return r; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the file
wc -l src/database/manager/manager.tsRepository: CD-Z/lnreader
Length of output: 92
🏁 Script executed:
# Read the file around the mentioned line ranges
cat -n src/database/manager/manager.ts | sed -n '140,200p'Repository: CD-Z/lnreader
Length of output: 2251
🏁 Script executed:
cat -n src/database/manager/manager.ts | sed -n '135,170p'Repository: CD-Z/lnreader
Length of output: 1346
🏁 Script executed:
cat -n src/database/manager/manager.ts | sed -n '168,202p'Repository: CD-Z/lnreader
Length of output: 1293
Make callback delivery lifecycle-safe.
The callback paths have two React-specific correctness issues: the initial calls are triggered from render (useState initializers on lines 146-149 and 179-185), and later reactive updates keep stale callback closures because callback is excluded from both dependency arrays (lines 152-164 and 187-199) with exhaustive-deps suppressed. This means callbacks can run before the component commits, still fire after an abandoned/unmounted async render, or invoke stale logic on later updates. Keep the latest callback in a useRef and invoke it only from committed effects/subscriptions.
Minimal pattern
-import { useEffect, useState } from 'react';
+import { useEffect, useRef, useState } from 'react';
export function useLiveQuery<T extends ExecutableSelect>(
query: T,
fireOn: FireOn,
callback?: (data: Awaited<ReturnType<T['all']>>) => void,
) {
type ReturnValue = Awaited<ReturnType<T['all']>>;
+ const callbackRef = useRef(callback);
+
+ useEffect(() => {
+ callbackRef.current = callback;
+ }, [callback]);
- const [data, setData] = useState<ReturnValue>(() => {
- const r = db.executeSync(sqlString, params as any[]).rows as ReturnValue;
- if (callback) callback(r);
- return r;
- });
+ const [data, setData] = useState<ReturnValue>(
+ () => db.executeSync(sqlString, params as any[]).rows as ReturnValue,
+ );
+
+ useEffect(() => {
+ callbackRef.current?.(data);
+ }, []);
useEffect(() => {
const unsub = db.reactiveExecute({
query: sqlString,
arguments: params as any[],
fireOn,
callback: (result: { rows: ReturnValue }) => {
setData(result.rows);
- if (callback) callback(result.rows);
+ callbackRef.current?.(result.rows);
},
});Apply the same callbackRef.current pattern to useLiveQueryAsync, and move the initial async callback into an effect with cancellation so it can't fire after unmount.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/database/manager/manager.ts` around lines 146 - 149, The current code
calls the provided callback directly from the useState initializer and from
effects with callback excluded from deps, causing render-time invocation and
stale/unsafe closures; fix this by storing the latest callback in a ref (e.g.
callbackRef.current) and updating it inside an effect, remove direct calls to
callback from the useState initializer (do not call callback from the
db.executeSync initialiser), and invoke callbackRef.current only from committed
effects/subscriptions (and from the live query subscription handlers) so calls
are cancelled on unmount; apply the same pattern to useLiveQueryAsync by moving
the initial async callback into an effect with cancellation and using
callbackRef.current for all subsequent invocations to avoid stale closures.
Comment on lines
+85
to
89
| export const getNovelById = (novelId: number): NovelInfo | undefined => { | ||
| return dbManager.getSync( | ||
| dbManager.select().from(novelSchema).where(eq(novelSchema.id, novelId)), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Don't silently flip this shared query to sync.
await getNovelById() will still "work", but this change turns a previously async lookup into a blocking DB read. src/services/download/downloadChapter.ts:80 and the query tests still consume it as async, while src/screens/novel/NovelContext.tsx:81 now runs it on the render path. Please keep the async contract or split this into an explicit getNovelByIdSync helper.
🤖 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 85 - 89, The change made
getNovelById into a synchronous/blocking call by using dbManager.getSync;
restore the original async contract or add an explicit sync helper: revert
getNovelById to return a Promise (use the async/dbManager.get or the async API
around dbManager.select().from(novelSchema).where(eq(novelSchema.id, novelId)))
so existing await callers (e.g., downloadChapter.ts and tests) remain async, and
if a sync variant is needed add a new function named getNovelByIdSync that uses
dbManager.getSync; make sure to update or leave callers unchanged depending on
which route you pick.
| @@ -0,0 +1,219 @@ | |||
| import { createChapterTextCache } from '../novelStore.chapterState'; | |||
There was a problem hiding this comment.
Import the cache factory from the module that actually exports it.
../novelStore.chapterState only exports createInitialChapterSlice in the current diff, so createChapterTextCache() is undefined and every case aborts before reaching its assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/persisted/useNovel/__tests__/novelStore.chapterState.test.ts` at
line 1, The test imports createChapterTextCache from the wrong module
(novelStore.chapterState) which only exports createInitialChapterSlice; update
the import to pull createChapterTextCache from the module that actually exports
it (the module that defines the cache factory) so createChapterTextCache is not
undefined and tests can run; keep the existing import for
createInitialChapterSlice as-is if needed and replace only the source module for
createChapterTextCache.
Comment on lines
+82
to
+94
| it('returns false when removing non-existent chapterId', () => { | ||
| const cache = createChapterTextCache(); | ||
|
|
||
| expect(cache.remove(1)).toBe(false); | ||
| expect(cache.remove(999)).toBe(false); | ||
| }); | ||
|
|
||
| it('returns true when successfully removing existing entry', () => { | ||
| const cache = createChapterTextCache(); | ||
| cache.write(1, 'Chapter content'); | ||
|
|
||
| expect(cache.remove(1)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
These remove() expectations don't match the current production contract.
ChapterTextCacheApi still types remove as void, and the implementation in src/hooks/persisted/useNovel/novelStore.chapterActions.ts:61-88 does not return a boolean. As written, this suite either fails after the import is fixed or forces a breaking API change. Assert on post-removal state instead, or update the type and implementation together.
Also applies to: 118-123
🧰 Tools
🪛 GitHub Actions: Testing
[error] 83-83: novelStore.chapterState › createChapterTextCache (remove operation) failed. TypeError: createChapterTextCache is not a function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/persisted/useNovel/__tests__/novelStore.chapterState.test.ts`
around lines 82 - 94, The tests assume remove() returns a boolean but the
current API (ChapterTextCacheApi.remove) and its implementation
(novelStore.chapterActions.ts remove) return void; change the tests that call
createChapterTextCache().remove(...) (including the other occurrences around
lines 118-123) to assert post-removal state instead of a boolean — e.g., call
cache.remove(id) then assert cache.read(id) is undefined or cache.has(id) is
false (or that the cache size changed) so the tests align with the existing
remove() contract rather than changing the API/implementation.
| settingsSort: ChapterOrderKey; | ||
| settingsFilter: ChapterFilterKey[]; | ||
| }): Promise<BootstrapResult> => { | ||
| console.time(`bootstrap_${pluginId}_${novelPath}`); |
There was a problem hiding this comment.
Remove debug console statements.
console.time and console.timeEnd calls should be removed or replaced with a proper logging utility.
Also applies to: 322-322
🧰 Tools
🪛 ESLint
[error] 274-274: Unexpected console statement.
(no-console)
🪛 GitHub Check: ESLint
[failure] 274-274:
Unexpected console statement
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/persisted/useNovel/bootstrapService.ts` at line 274, Remove the
debug timing calls: locate the console.time and console.timeEnd usages (they use
the key `bootstrap_${pluginId}_${novelPath}` and reference the variables
pluginId and novelPath) and delete them or replace them with the project's
logger (e.g., processLogger.debug/info) emitting a single log entry with timing
info if timing is required; ensure you update both occurrences (the one at the
start and the corresponding console.timeEnd) so no raw console.* calls remain.
| [lastRead], | ||
| ); | ||
|
|
||
| console.log('rendering chapter', fetching, chapters.length); |
There was a problem hiding this comment.
Remove the render-time debug log.
This fires on every list render and is already flagged by no-console.
Suggested cleanup
- console.log('rendering chapter', fetching, chapters.length);📝 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.
Suggested change
| console.log('rendering chapter', fetching, chapters.length); |
🧰 Tools
🪛 ESLint
[error] 481-481: Unexpected console statement.
(no-console)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/novel/components/NovelScreenList.tsx` at line 481, Remove the
temporary render-time debug log that violates no-console: delete the
console.log('rendering chapter', fetching, chapters.length) statement inside the
NovelScreenList component (the render function where variables fetching and
chapters are referenced) or replace it with a conditional debug logger tied to
environment/config; ensure no direct console.log calls remain in
NovelScreenList.
| import React, { | ||
| createContext, | ||
| useContext, | ||
| useEffect, |
There was a problem hiding this comment.
Remove unused imports.
useEffect, useLiveQuery, and useLiveQueryAsync are imported but never used.
🧹 Proposed fix
import React, {
createContext,
useContext,
- useEffect,
useMemo,
useRef,
} from 'react';-import { useLiveQuery, useLiveQueryAsync } from '@database/manager/manager';Also applies to: 25-25
🧰 Tools
🪛 ESLint
[error] 4-4: 'useEffect' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/novel/NovelContext.tsx` at line 4, Remove the unused imports from
NovelContext.tsx: delete useEffect, useLiveQuery, and useLiveQueryAsync from the
import list so only actually used hooks remain; specifically update the import
statement that currently includes useEffect and the live query hooks (look for
identifiers useEffect, useLiveQuery, useLiveQueryAsync) and remove them to
eliminate unused-import warnings.
| export function NovelContextProvider({ | ||
| children, | ||
| export function NovelContextProvider({ children, route }: Props) { | ||
| console.time('novel contex'); |
There was a problem hiding this comment.
Remove debug console statements before merge.
Multiple console.time/console.timeLog/console.timeEnd calls are present for debugging. These should be removed or replaced with a proper logging utility that can be disabled in production.
🧹 Proposed fix
export function NovelContextProvider({ children, route }: Props) {
- console.time('novel contex');
const initialNovel =
'id' in route.params ? (route.params as NovelInfo) : undefined;Remove all other console.time* calls at lines 73-77, 96-102, 134, and 164.
Also applies to: 73-77, 96-102, 134-134, 164-164
🧰 Tools
🪛 ESLint
[error] 46-46: Unexpected console statement.
(no-console)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/novel/NovelContext.tsx` at line 46, Remove the debug timing
statements in NovelContext.tsx — delete console.time('novel contex') and all
other console.time*/console.timeLog*/console.timeEnd* occurrences (notably the
groups around the earlier suggested lines) and, if timing is needed, replace
them with calls to the app's logging utility or a debug logger that respects
NODE_ENV so these traces are disabled in production; locate the statements by
searching for console.time, console.timeLog and console.timeEnd within the
NovelContext component and update uses accordingly.
Comment on lines
+1
to
+8
| import React, { | ||
| Suspense, | ||
| useCallback, | ||
| useMemo, | ||
| useRef, | ||
| useState, | ||
| useSyncExternalStore, | ||
| } from 'react'; |
There was a problem hiding this comment.
Remove the temporary debug code.
useSyncExternalStore is unused, and the render-time console.log already violates the current ESLint rules.
Suggested cleanup
import React, {
Suspense,
useCallback,
useMemo,
useRef,
useState,
- useSyncExternalStore,
} from 'react';
@@
- console.log('Novel render', novel?.name, chapters.length);Also applies to: 44-46
🧰 Tools
🪛 ESLint
[error] 7-7: 'useSyncExternalStore' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/novel/NovelScreen.tsx` around lines 1 - 8, Remove the temporary
debug code in NovelScreen.tsx: delete the unused import useSyncExternalStore
from the React import list and remove the render-time console.log calls around
lines 44-46 (the inline debug logs in the NovelScreen component) so the
component no longer violates ESLint rules; adjust the import statement to only
include the actually used hooks (Suspense, useCallback, useMemo, useRef,
useState) to keep imports clean.
Comment on lines
+27
to
+32
| jest.mock('@hooks/persisted', () => ({ | ||
| useChapterGeneralSettings: () => mockUseChapterGeneralSettings(), | ||
| useLibrarySettings: () => mockUseLibrarySettings(), | ||
| useTracker: () => mockUseTracker(), | ||
| useTrackedNovel: (...args: unknown[]) => mockUseTrackedNovel(...args), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify useChapter contract:"
rg -n "useNovelActions|useNovelContext|markChapterRead|updateChapterProgress|chapterTextCache|setLastRead" \
src/screens/reader/hooks/useChapter.ts
echo
echo "Verify test mocks align with contract:"
rg -n "jest.mock\\('@hooks/persisted'|mockUseNovelContext|mockUseNovelActions|useNovelContext|useNovelActions" \
src/screens/reader/hooks/__tests__/useChapter.test.tsRepository: CD-Z/lnreader
Length of output: 1389
Mock useNovelActions from @screens/novel/NovelContext instead of useNovelContext.
The hook imports useNovelActions at line 37 and destructures actions from it at lines 47–51, but the test mocks the wrong hook (useNovelContext). The mocks at lines 183, 206, 240, and 254 inject { novelStore: store }, which does not match what useNovelActions() should return. This causes tests to fail at render or pass incorrectly.
Suggested fix
-const mockUseNovelContext = jest.fn();
+const mockUseNovelActions = jest.fn();
const mockUseChapterGeneralSettings = jest.fn();
const mockUseLibrarySettings = jest.fn();
const mockUseTracker = jest.fn();
const mockUseTrackedNovel = jest.fn();
const mockUseFullscreenMode = jest.fn();
-jest.mock('@screens/novel/NovelContext', () => ({
- useNovelContext: () => mockUseNovelContext(),
-}));
-
jest.mock('@hooks/persisted', () => ({
useChapterGeneralSettings: () => mockUseChapterGeneralSettings(),
useLibrarySettings: () => mockUseLibrarySettings(),
useTracker: () => mockUseTracker(),
useTrackedNovel: (...args: unknown[]) => mockUseTrackedNovel(...args),
+}));
+
+jest.mock('@screens/novel/NovelContext', () => ({
+ useNovelActions: () => mockUseNovelActions(),
}));Update test setup at lines 183, 206, 240, 254 to return the action methods:
-mockUseNovelContext.mockReturnValue({ novelStore: store });
+mockUseNovelActions.mockReturnValue(store.state);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/reader/hooks/__tests__/useChapter.test.ts` around lines 27 - 32,
The test currently mocks the wrong hook (useNovelContext) and injects {
novelStore: store }; instead mock useNovelActions from
`@screens/novel/NovelContext` and have those mocks return the actual action
methods the hook expects (the same named functions you destructure from
useNovelActions in the hook under test) instead of a novelStore object; locate
the mock definitions currently returning { novelStore: store } and replace them
so useNovelActions: () => ({ /* implement the action methods used in the hook
(same names as destructured) as jest.fn() or pointing to your test stubs */ })
so the hook receives the expected API.
CD-Z
force-pushed
the
master
branch
2 times, most recently
from
2547835 to
06f0f66
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores