Open
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
This comment was marked as resolved.
This comment was marked as resolved.
📝 WalkthroughWalkthroughEstablishes end-to-end testing infrastructure: multi-project Jest setup, extensive Jest mocks for native modules/database/hooks, centralized test utilities/providers, new tests and docs, ESLint scoping for tests, CI workflow to run tests, and small bugfixes/renames for persisted keys and hook APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/persisted/useNovel.ts (1)
86-88:⚠️ Potential issue | 🔴 CriticalCritical: MMKV key contains embedded newlines and whitespace from template literal formatting.
The multi-line template literal produces a key like
"\n NOVEL_PAGE_INDEX_PREFIX_pluginId_path\n "instead of a clean key. This causes a mismatch with the key constructed indeleteCachedNovels(line 681), which uses a single-line string without whitespace. Stored values will never be cleaned up, and any code outside this hook that references the same prefix will fail to find the entry.Proposed fix
- const [pageIndex = defaultPageIndex, setPageIndex] = useMMKVNumber(` - ${NOVEL_PAGE_INDEX_PREFIX}_${pluginId}_${novelPath} - `); + const [pageIndex = defaultPageIndex, setPageIndex] = useMMKVNumber( + `${NOVEL_PAGE_INDEX_PREFIX}_${pluginId}_${novelPath}`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovel.ts` around lines 86 - 88, The MMKV key passed to useMMKVNumber is being created with a multi-line template literal which embeds newlines/whitespace; replace it with a single-line key exactly matching the format used elsewhere (e.g., in deleteCachedNovels) by constructing the key as a single-line string combining NOVEL_PAGE_INDEX_PREFIX, pluginId and novelPath (for example via `${NOVEL_PAGE_INDEX_PREFIX}_${pluginId}_${novelPath}` or string concatenation) so no extra whitespace/newlines are included; update the call site using useMMKVNumber to use that single-line key.jest.config.js (1)
82-84:⚠️ Potential issue | 🟠 Major
resetMocks: trueresets mock function implementations between tests, causing mocks with default return values to returnundefinedafter the first test.Jest's
resetMockscallsjest.resetAllMocks()between each test, which removes mock function implementations in addition to clearing call counts. The mock functions created in yourjest.mock()factories—such asreadFile: jest.fn(() => ''),exists: jest.fn(() => true), and others in__mocks__/nativeModules.js—will lose their implementations after the first test and returnundefinedfor subsequent tests.Use
clearMocks: truealone (which preserves implementations while clearing call counts) combined withrestoreMocks: trueif spy restoration is needed.Proposed fix
clearMocks: true, - resetMocks: true, restoreMocks: true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jest.config.js` around lines 82 - 84, The Jest config currently sets resetMocks: true which calls jest.resetAllMocks() and strips implementations from mocked functions (e.g., the factory mocks in __mocks__/nativeModules.js like readFile, exists), causing them to return undefined after the first test; change the config to remove or set resetMocks to false and rely on clearMocks: true (and keep restoreMocks: true if you need spies restored) so mocks have their implementations preserved between tests while call counts are cleared—update the jest.config.js to eliminate resetMocks so the jest.mock() factory implementations (readFile, exists, etc.) remain intact.
♻️ Duplicate comments (1)
src/hooks/persisted/__mocks__/useNovelSettings.ts (1)
2-2: SameNOVEL_SETTINSG_PREFIXtypo as inuseNovel.tsBoth files need to be corrected together (same fix as noted in
useNovel.tsLine 2).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/__mocks__/useNovelSettings.ts` at line 2, Fix the typo in the exported constant name NOVEL_SETTINSG_PREFIX (should be NOVEL_SETTINGS_PREFIX) in both modules where it's defined/used (the mock useNovelSettings and the useNovel implementation) so the constant name and any imports/uses match; update the export declaration and all references/imports of NOVEL_SETTINSG_PREFIX to NOVEL_SETTINGS_PREFIX to ensure consistency across useNovel.ts and __mocks__/useNovelSettings.ts.
🧹 Nitpick comments (8)
src/hooks/persisted/__mocks__/useDownload.ts (1)
7-17: Consider wrapping withjest.fn()for consistency and easier per-test overrides.Other hook mocks in this PR (e.g.,
useHistory,useCategories,useUpdates) are wrapped withjest.fn(() => ...), allowing tests to override return values via.mockReturnValue(...). This mock is a plain function, which makes per-test customization harder.Suggested change
-export default function useDownload() { - return { +const useDownload = jest.fn(() => ({ downloadQueue: mockDownloadQueue, downloadingChapterIds: mockDownloadingChapterIds, - resumeDowndload: jest.fn(), + resumeDownload: jest.fn(), downloadChapter: jest.fn(), downloadChapters: jest.fn(), pauseDownload: jest.fn(), cancelDownload: jest.fn(), - }; -} +})); + +export default useDownload;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/__mocks__/useDownload.ts` around lines 7 - 17, Replace the plain function export of useDownload with a jest mock function so tests can override its return value; specifically, change the default export of useDownload to be jest.fn(() => ({ downloadQueue: mockDownloadQueue, downloadingChapterIds: mockDownloadingChapterIds, resumeDowndload: jest.fn(), downloadChapter: jest.fn(), downloadChapters: jest.fn(), pauseDownload: jest.fn(), cancelDownload: jest.fn() })), keeping the same returned symbols so callers (useDownload, mockDownloadQueue, mockDownloadingChapterIds, resumeDowndload, downloadChapter, downloadChapters, pauseDownload, cancelDownload) continue to work but allow per-test .mockReturnValue overrides.package.json (1)
159-159:react-test-rendereris deprecated in React 19.Per React 19's upgrade guide,
react-test-rendereris deprecated and@testing-library/react-native(already added at Line 132) is the recommended replacement. Ifreact-test-rendereris only here as a transitive peer dependency forjest-expo, consider whether it can be omitted or noted as a temporary requirement. Otherwise, prefer migrating any direct usages to@testing-library/react-native.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 159, The dependency "react-test-renderer" is deprecated for React 19; remove or mark it as temporary in package.json and migrate any direct test usage to "@testing-library/react-native" (already present) or keep it only if required transitively by "jest-expo"; inspect test files for imports of "react-test-renderer" and replace them with "@testing-library/react-native" APIs (or remove the explicit dependency if it's only a transitive peer for jest-expo) so package.json no longer lists "react-test-renderer" as a top-level dependency unless truly necessary.src/hooks/persisted/__mocks__/useNovel.ts (1)
50-52: Redundant dual export ofuseNovel
export { useNovel }on Line 51 followed immediately byexport default useNovelon Line 52 is redundant. The named export is sufficient alongside the default, but having it spelled out twice adds noise. Consider keeping only the default export (matching the pattern inuseTrackedNovel.ts) unless consumers specifically rely on the named import.♻️ Proposed simplification
export const deleteCachedNovels = jest.fn(); export { mockNovel, mockChapters }; -export { useNovel }; export default useNovel;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/__mocks__/useNovel.ts` around lines 50 - 52, Remove the redundant named export for useNovel: delete the line "export { useNovel }" and keep the default export "export default useNovel" (ensuring mockNovel and mockChapters exports remain unchanged); also search for any consumers importing { useNovel } and update them to use the default import if necessary to match useTrackedNovel.ts's pattern..github/workflows/testing.yml (1)
47-48: Optional: split RN and DB tests into separate CI steps/jobs.Running
test:rnandtest:dbseparately improves failure visibility and triage speed without changing test coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/testing.yml around lines 47 - 48, Replace the single CI step named "Run Tests" that invokes "pnpm run test" with two separate CI steps (or jobs) that run "pnpm run test:rn" and "pnpm run test:db" respectively so RN and DB tests are executed independently; ensure each step has a clear name like "Run RN Tests" and "Run DB Tests" and preserves the same environment/setup/checkout actions used by the original step to avoid changing test behavior.src/hooks/persisted/useNovel.ts (3)
244-248:"teaser"label inconsole.errorappears to be a debug artifact.Consider replacing with a descriptive message like
'Error fetching chapters'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovel.ts` around lines 244 - 248, Replace the debug artifact label in the catch block inside useNovel.ts where getPageChaptersBatched(...) is called: change console.error('teaser', error) to a clear descriptive message (e.g., "Error fetching chapters" or "Error fetching chapters in useNovel") so the log references the operation (getPageChaptersBatched/newChapters) and includes the error object for context.
368-372: UseforEachinstead ofmapfor side-effect-only iteration.
_chapters.map(...)on line 370 discards the return value —forEachexpresses intent more clearly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovel.ts` around lines 368 - 372, The bookmarkChapters callback is using _chapters.map for side-effects, discarding the return value; change the iteration in the useCallback named bookmarkChapters to use _chapters.forEach(_chapter => _bookmarkChapter(_chapter.id)) instead of _chapters.map(...) so intent is clear and no unused array is produced; keep the same parameters and _bookmarkChapter call.
593-602:setFetching(false)is called redundantly in bothcatchandfinally.The
finallyblock already handles this — the call incatch(line 598) is redundant.Proposed fix
getChapters() .catch(e => { if (__DEV__) console.error(e); showToast(e.message); - setFetching(false); }) .finally(() => { setFetching(false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/persisted/useNovel.ts` around lines 593 - 602, The catch block in the getChapters() promise chain redundantly calls setFetching(false) even though the finally block already does this; remove the setFetching(false) call from the catch callback (preserve the __DEV__ console.error(e) and showToast(e.message) behavior) and rely on the existing finally(() => setFetching(false)) to clear fetching state, referencing the getChapters() call and the setFetching function.__mocks__/react-navigation.ts (1)
13-13: Stale/misleading comment.This comment says
__tests__/jest.setup.tsbut the file is__mocks__/react-navigation.ts. Remove or correct it to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__mocks__/react-navigation.ts` at line 13, The top-of-file comment "// __tests__/jest.setup.ts" in __mocks__/react-navigation.ts is stale/misleading; remove or replace it with a correct description (e.g., "// __mocks__/react-navigation.ts" or a brief purpose note) so the header matches the actual filename and purpose; update any similar header comment in the file (search for that exact string) to avoid confusion.
🤖 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 1-9: Update the misleading top-line comment to state that this
mock is for "react-native-mmkv" (not "react-native-nitro-modules") and briefly
explain why the mock exports NitroModules.createHybridObject (e.g., because
react-native-mmkv v3 re-exports or depends on NitroModules), so readers
understand why module.exports exposes NitroModules and the createHybridObject
jest.fn; keep the rest of the mock unchanged and reference NitroModules and
createHybridObject in the comment for clarity.
In `@__mocks__/react-navigation.ts`:
- Around line 20-23: The mock returns new jest.fn() instances on each call;
hoist stable mock functions to module scope and have useNavigation return that
same object so references for navigate and setOptions are stable across calls;
additionally export the hoisted mocks (e.g., navigateMock, setOptionsMock or
navigationMock) so tests can import and assert on the same jest.fn() instances
referenced by components that call useNavigation().
In `@__tests-modules__/test-utils.tsx`:
- Around line 29-44: The renderNovel helper leaks the non-standard route prop
into RNTL's render options; change the options destructuring to separate route
from the other render options (e.g., const { route, ...renderOptions } = options
|| {}), pass route into NovelContextProvider (route as NovelScreenProps['route']
| ChapterScreenProps['route']) and spread only renderOptions into render (i.e.,
{ wrapper: AllTheProviders, ...renderOptions }) so route is not forwarded to
`@testing-library/react-native`'s render.
In `@src/components/__tests__/Button.test.tsx`:
- Around line 60-64: The test "uses theme from useTheme hook" is only a smoke
test; update it to assert a theme-dependent output by either mocking the
useTheme hook or rendering Button inside the real ThemeProvider with a
deterministic theme and then asserting a style or prop that depends on theme
(e.g., backgroundColor, textColor, or a themed testID). Locate the test case for
Button and modify it to import/mock useTheme (or wrap with ThemeProvider),
render <Button title="Test" onPress={() => {}} />, then assert the expected
themed value (for example expect(getByText('Test')).toHaveStyle({ color:
'EXPECTED_COLOR' }) or expect(getByTestId('button-root')).toHaveProp('style',
expect.objectContaining({ backgroundColor: 'EXPECTED_BG' }))). Ensure you
reference the Button component and useTheme in your change so the test fails on
theme regressions.
In `@src/components/__tests__/LoadingMoreIndicator.test.tsx`:
- Around line 45-57: The tests in LoadingMoreIndicator.test.tsx are only
asserting truthy snapshots but claim to check theme behavior; update the two
specs so they actually assert theme-driven output: for the "renders
ActivityIndicator with theme color" test render LoadingMoreIndicator with
mockTheme and assert the rendered ActivityIndicator's color prop equals
mockTheme.primary (locate the ActivityIndicator inside the rendered tree from
the LoadingMoreIndicator component); for the "receives theme prop correctly"
test either rename it to a smoke-test or instead render with customTheme and
assert the ActivityIndicator color equals customTheme.primary (use
mockTheme/customTheme and the LoadingMoreIndicator component names to find the
elements to inspect).
In `@src/hooks/__mocks__/index.ts`:
- Around line 14-16: The mock for NOVEL_STORAGE in src/hooks/__mocks__/index.ts
is an object but runtime expects a path string; update the jest.mock to export
NOVEL_STORAGE as a realistic path string (e.g., '/path/to/novel' or a relative
path used by tests) so code that treats NOVEL_STORAGE as a path (lookups, joins,
fs calls) behaves the same in tests; ensure this change updates any dependent
mocks or test setup that reference NOVEL_STORAGE.
In `@src/hooks/__tests__/useNovel.test.ts`:
- Around line 333-335: The forEach callbacks in the test are returning the
assertion value which triggers lint/suspicious/useIterableCallbackReturn; update
the callbacks used on result.current.chapters.filter(...) and the other
occurrences (lines near the mentioned ranges) to use block bodies that call
assertions without returning (e.g., change arrow callbacks to use {
expect(...).toBe(...); } or use a normal function body), ensuring functions like
the ones operating on result.current.chapters and their c => ... callbacks do
not implicitly return the assertion result.
- Around line 396-403: The test name claims progress is capped at 100 but the
state assertion checks for 150 — make them consistent by asserting the state is
capped at 100: call result.current.updateChapterProgress(1, 150) as before,
assert _updateChapterProgress was calledWith(1, 100) and change the chapters
state assertion to expect progress === 100 (inspect
result.current.chapters.find(c => c.id === 1)?.progress); update the test name
if you instead prefer to keep the 150 assertion.
In `@src/hooks/persisted/__mocks__/useNovel.ts`:
- Line 2: Rename the misspelled constant NOVEL_SETTINSG_PREFIX to
NOVEL_SETTINGS_PREFIX in this mock and the corresponding definition in
useNovelSettings.ts, then update all imports/consumers (tests, mocks, and any
code referencing NOVEL_SETTINSG_PREFIX) to use NOVEL_SETTINGS_PREFIX; run a
project-wide find-and-replace or codemod to ensure consistency and update any
TypeScript types/exports so the symbol name change doesn't break imports.
In `@src/hooks/persisted/__mocks__/usePlugins.ts`:
- Around line 1-18: The mock currently exports an object containing usePlugins
instead of exporting the mocked function itself; replace the exported object
with a default export of a jest.fn named (for clarity) mockUsePlugins that
returns the same shape (keep properties like filteredAvailablePlugins,
filteredInstalledPlugins, lastUsedPlugin, pinnedPlugins, languagesFilter,
setLastUsedPlugin, refreshPlugins, toggleLanguageFilter, installPlugin,
uninstallPlugin, updatePlugin, togglePinPlugin, isPinned) so imports of
usePlugins receive the function directly (update the export to export default
mockUsePlugins and remove the wrapper object).
In `@src/hooks/persisted/__mocks__/useTracker.ts`:
- Around line 18-30: The exported useTracker should be converted from a plain
function to a jest.fn() mock and its child mock methods hoisted so the same
jest.fn() instances are reused across renders; replace the current export of
function useTracker() with an exported const useTracker = jest.fn(() => ({ ...
})) and create top-level jest.fn() instances for setTracker, removeTracker,
getTrackerAuth, isTrackerAuthenticated, getAuthenticatedTrackers and
setActiveTracker (and reuse those variables inside the jest.fn() return) so
callers can spy/reset useTracker and assertions on the mock functions remain
stable across re-renders.
In `@tsconfig.json`:
- Line 35: Remove the trailing comma after the "@test-utils" entry in the
tsconfig "paths" object so the JSON is valid; locate the "@test-utils":
["./__tests-modules__/test-utils"] entry and ensure it is the final property
without a trailing comma.
---
Outside diff comments:
In `@jest.config.js`:
- Around line 82-84: The Jest config currently sets resetMocks: true which calls
jest.resetAllMocks() and strips implementations from mocked functions (e.g., the
factory mocks in __mocks__/nativeModules.js like readFile, exists), causing them
to return undefined after the first test; change the config to remove or set
resetMocks to false and rely on clearMocks: true (and keep restoreMocks: true if
you need spies restored) so mocks have their implementations preserved between
tests while call counts are cleared—update the jest.config.js to eliminate
resetMocks so the jest.mock() factory implementations (readFile, exists, etc.)
remain intact.
In `@src/hooks/persisted/useNovel.ts`:
- Around line 86-88: The MMKV key passed to useMMKVNumber is being created with
a multi-line template literal which embeds newlines/whitespace; replace it with
a single-line key exactly matching the format used elsewhere (e.g., in
deleteCachedNovels) by constructing the key as a single-line string combining
NOVEL_PAGE_INDEX_PREFIX, pluginId and novelPath (for example via
`${NOVEL_PAGE_INDEX_PREFIX}_${pluginId}_${novelPath}` or string concatenation)
so no extra whitespace/newlines are included; update the call site using
useMMKVNumber to use that single-line key.
---
Duplicate comments:
In `@src/hooks/persisted/__mocks__/useNovelSettings.ts`:
- Line 2: Fix the typo in the exported constant name NOVEL_SETTINSG_PREFIX
(should be NOVEL_SETTINGS_PREFIX) in both modules where it's defined/used (the
mock useNovelSettings and the useNovel implementation) so the constant name and
any imports/uses match; update the export declaration and all references/imports
of NOVEL_SETTINSG_PREFIX to NOVEL_SETTINGS_PREFIX to ensure consistency across
useNovel.ts and __mocks__/useNovelSettings.ts.
---
Nitpick comments:
In `@__mocks__/react-navigation.ts`:
- Line 13: The top-of-file comment "// __tests__/jest.setup.ts" in
__mocks__/react-navigation.ts is stale/misleading; remove or replace it with a
correct description (e.g., "// __mocks__/react-navigation.ts" or a brief purpose
note) so the header matches the actual filename and purpose; update any similar
header comment in the file (search for that exact string) to avoid confusion.
In @.github/workflows/testing.yml:
- Around line 47-48: Replace the single CI step named "Run Tests" that invokes
"pnpm run test" with two separate CI steps (or jobs) that run "pnpm run test:rn"
and "pnpm run test:db" respectively so RN and DB tests are executed
independently; ensure each step has a clear name like "Run RN Tests" and "Run DB
Tests" and preserves the same environment/setup/checkout actions used by the
original step to avoid changing test behavior.
In `@package.json`:
- Line 159: The dependency "react-test-renderer" is deprecated for React 19;
remove or mark it as temporary in package.json and migrate any direct test usage
to "@testing-library/react-native" (already present) or keep it only if required
transitively by "jest-expo"; inspect test files for imports of
"react-test-renderer" and replace them with "@testing-library/react-native" APIs
(or remove the explicit dependency if it's only a transitive peer for jest-expo)
so package.json no longer lists "react-test-renderer" as a top-level dependency
unless truly necessary.
In `@src/hooks/persisted/__mocks__/useDownload.ts`:
- Around line 7-17: Replace the plain function export of useDownload with a jest
mock function so tests can override its return value; specifically, change the
default export of useDownload to be jest.fn(() => ({ downloadQueue:
mockDownloadQueue, downloadingChapterIds: mockDownloadingChapterIds,
resumeDowndload: jest.fn(), downloadChapter: jest.fn(), downloadChapters:
jest.fn(), pauseDownload: jest.fn(), cancelDownload: jest.fn() })), keeping the
same returned symbols so callers (useDownload, mockDownloadQueue,
mockDownloadingChapterIds, resumeDowndload, downloadChapter, downloadChapters,
pauseDownload, cancelDownload) continue to work but allow per-test
.mockReturnValue overrides.
In `@src/hooks/persisted/__mocks__/useNovel.ts`:
- Around line 50-52: Remove the redundant named export for useNovel: delete the
line "export { useNovel }" and keep the default export "export default useNovel"
(ensuring mockNovel and mockChapters exports remain unchanged); also search for
any consumers importing { useNovel } and update them to use the default import
if necessary to match useTrackedNovel.ts's pattern.
In `@src/hooks/persisted/useNovel.ts`:
- Around line 244-248: Replace the debug artifact label in the catch block
inside useNovel.ts where getPageChaptersBatched(...) is called: change
console.error('teaser', error) to a clear descriptive message (e.g., "Error
fetching chapters" or "Error fetching chapters in useNovel") so the log
references the operation (getPageChaptersBatched/newChapters) and includes the
error object for context.
- Around line 368-372: The bookmarkChapters callback is using _chapters.map for
side-effects, discarding the return value; change the iteration in the
useCallback named bookmarkChapters to use _chapters.forEach(_chapter =>
_bookmarkChapter(_chapter.id)) instead of _chapters.map(...) so intent is clear
and no unused array is produced; keep the same parameters and _bookmarkChapter
call.
- Around line 593-602: The catch block in the getChapters() promise chain
redundantly calls setFetching(false) even though the finally block already does
this; remove the setFetching(false) call from the catch callback (preserve the
__DEV__ console.error(e) and showToast(e.message) behavior) and rely on the
existing finally(() => setFetching(false)) to clear fetching state, referencing
the getChapters() call and the setFetching function.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.eslintrc.js.github/workflows/testing.ymlTESTING.md__mocks__/database.js__mocks__/index.js__mocks__/nativeModules.js__mocks__/react-native-mmkv.js__mocks__/react-navigation.ts__tests-modules__/test-utils.tsx__tests__/jest.setup.tsbabel.config.jsjest.config.jspackage.jsonsrc/components/Context/__mocks__/LibraryContext.tsxsrc/components/__tests__/Button.test.tsxsrc/components/__tests__/EmptyView.test.tsxsrc/components/__tests__/LoadingMoreIndicator.test.tsxsrc/components/__tests__/mocks.tssrc/hooks/__mocks__/index.tssrc/hooks/__tests__/mocks.tssrc/hooks/__tests__/useNovel.test.tssrc/hooks/persisted/__mocks__/useCategories.tssrc/hooks/persisted/__mocks__/useDownload.tssrc/hooks/persisted/__mocks__/useHistory.tssrc/hooks/persisted/__mocks__/useImport.tssrc/hooks/persisted/__mocks__/useNovel.tssrc/hooks/persisted/__mocks__/useNovelSettings.tssrc/hooks/persisted/__mocks__/usePlugins.tssrc/hooks/persisted/__mocks__/useSelfHost.tssrc/hooks/persisted/__mocks__/useSettings.tssrc/hooks/persisted/__mocks__/useTheme.tssrc/hooks/persisted/__mocks__/useTrackedNovel.tssrc/hooks/persisted/__mocks__/useTracker.tssrc/hooks/persisted/__mocks__/useUpdates.tssrc/hooks/persisted/__mocks__/useUserAgent.tssrc/hooks/persisted/useNovel.tssrc/services/plugin/__mocks__/fetch.tstsconfig.jsontsconfig.tsbuildinfo
Comment on lines
+29
to
+44
| const renderNovel = ( | ||
| ui: React.ReactElement, | ||
| options?: { | ||
| route?: NovelScreenProps['route'] | ChapterScreenProps['route']; | ||
| }, | ||
| ) => { | ||
| const { route } = options || {}; | ||
| return render( | ||
| <NovelContextProvider | ||
| route={route as NovelScreenProps['route'] | ChapterScreenProps['route']} | ||
| > | ||
| {ui} | ||
| </NovelContextProvider>, | ||
| { wrapper: AllTheProviders, ...options }, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
route leaks into RNTL render options.
After destructuring route from options, the entire options object (still containing route) is spread into render(…, { wrapper, ...options }). This passes the non-standard route key to @testing-library/react-native's render, which may cause warnings or be silently ignored.
Use rest destructuring to separate route from the render-compatible options:
Proposed fix
const renderNovel = (
ui: React.ReactElement,
options?: {
route?: NovelScreenProps['route'] | ChapterScreenProps['route'];
+ } & Record<string, unknown>,
- },
) => {
- const { route } = options || {};
+ const { route, ...restOptions } = options || {};
return render(
<NovelContextProvider
route={route as NovelScreenProps['route'] | ChapterScreenProps['route']}
>
{ui}
</NovelContextProvider>,
- { wrapper: AllTheProviders, ...options },
+ { wrapper: AllTheProviders, ...restOptions },
);
};📝 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 renderNovel = ( | |
| ui: React.ReactElement, | |
| options?: { | |
| route?: NovelScreenProps['route'] | ChapterScreenProps['route']; | |
| }, | |
| ) => { | |
| const { route } = options || {}; | |
| return render( | |
| <NovelContextProvider | |
| route={route as NovelScreenProps['route'] | ChapterScreenProps['route']} | |
| > | |
| {ui} | |
| </NovelContextProvider>, | |
| { wrapper: AllTheProviders, ...options }, | |
| ); | |
| }; | |
| const renderNovel = ( | |
| ui: React.ReactElement, | |
| options?: { | |
| route?: NovelScreenProps['route'] | ChapterScreenProps['route']; | |
| } & Record<string, unknown>, | |
| ) => { | |
| const { route, ...restOptions } = options || {}; | |
| return render( | |
| <NovelContextProvider | |
| route={route as NovelScreenProps['route'] | ChapterScreenProps['route']} | |
| > | |
| {ui} | |
| </NovelContextProvider>, | |
| { wrapper: AllTheProviders, ...restOptions }, | |
| ); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests-modules__/test-utils.tsx` around lines 29 - 44, The renderNovel
helper leaks the non-standard route prop into RNTL's render options; change the
options destructuring to separate route from the other render options (e.g.,
const { route, ...renderOptions } = options || {}), pass route into
NovelContextProvider (route as NovelScreenProps['route'] |
ChapterScreenProps['route']) and spread only renderOptions into render (i.e., {
wrapper: AllTheProviders, ...renderOptions }) so route is not forwarded to
`@testing-library/react-native`'s render.
Comment on lines
+60
to
+64
| it('uses theme from useTheme hook', () => { | ||
| const { toJSON } = render(<Button title="Test" onPress={() => {}} />); | ||
|
|
||
| expect(toJSON()).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
Theme test is currently a smoke test, not a behavior test.
This assertion only checks render success. Please assert at least one theme-dependent output so hook/theme regressions are caught.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/__tests__/Button.test.tsx` around lines 60 - 64, The test
"uses theme from useTheme hook" is only a smoke test; update it to assert a
theme-dependent output by either mocking the useTheme hook or rendering Button
inside the real ThemeProvider with a deterministic theme and then asserting a
style or prop that depends on theme (e.g., backgroundColor, textColor, or a
themed testID). Locate the test case for Button and modify it to import/mock
useTheme (or wrap with ThemeProvider), render <Button title="Test" onPress={()
=> {}} />, then assert the expected themed value (for example
expect(getByText('Test')).toHaveStyle({ color: 'EXPECTED_COLOR' }) or
expect(getByTestId('button-root')).toHaveProp('style', expect.objectContaining({
backgroundColor: 'EXPECTED_BG' }))). Ensure you reference the Button component
and useTheme in your change so the test fails on theme regressions.
Comment on lines
+45
to
+57
| it('renders ActivityIndicator with theme color', () => { | ||
| const { toJSON } = render(<LoadingMoreIndicator theme={mockTheme} />); | ||
|
|
||
| expect(toJSON()).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('receives theme prop correctly', () => { | ||
| const customTheme = { ...mockTheme, primary: '#ff0000' }; | ||
| const { toJSON } = render(<LoadingMoreIndicator theme={customTheme} />); | ||
|
|
||
| const json = toJSON(); | ||
| expect(json).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
Tests don’t verify the behavior described by their names.
Both cases only check truthy JSON, so regressions in theme color handling would still pass. Please either assert theme-driven output explicitly or rename these to smoke-test wording.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/__tests__/LoadingMoreIndicator.test.tsx` around lines 45 - 57,
The tests in LoadingMoreIndicator.test.tsx are only asserting truthy snapshots
but claim to check theme behavior; update the two specs so they actually assert
theme-driven output: for the "renders ActivityIndicator with theme color" test
render LoadingMoreIndicator with mockTheme and assert the rendered
ActivityIndicator's color prop equals mockTheme.primary (locate the
ActivityIndicator inside the rendered tree from the LoadingMoreIndicator
component); for the "receives theme prop correctly" test either rename it to a
smoke-test or instead render with customTheme and assert the ActivityIndicator
color equals customTheme.primary (use mockTheme/customTheme and the
LoadingMoreIndicator component names to find the elements to inspect).
Comment on lines
+396
to
+403
| 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); | ||
| }); |
There was a problem hiding this comment.
Test name and assertion intent are inconsistent.
The case says progress is capped at 100, but the state assertion expects 150. Rename the test or adjust assertions so intent is unambiguous.
Suggested fix (rename for current assertions)
- it('updates progress in state and caps at 100', async () => {
+ it('caps persisted progress at 100 while keeping raw state progress', async () => {📝 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('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('caps persisted progress at 100 while keeping raw state progress', 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); | |
| }); |
🤖 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 396 - 403, The test name
claims progress is capped at 100 but the state assertion checks for 150 — make
them consistent by asserting the state is capped at 100: call
result.current.updateChapterProgress(1, 150) as before, assert
_updateChapterProgress was calledWith(1, 100) and change the chapters state
assertion to expect progress === 100 (inspect result.current.chapters.find(c =>
c.id === 1)?.progress); update the test name if you instead prefer to keep the
150 assertion.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
__mocks__/react-navigation.js (1)
1-29: File does more than its name suggests — consider renaming or splitting.This file mocks
react-native-worklets, sets upreact-native-gesture-handler, initializesreact-native-reanimated, and mocks@react-navigation/native. The filenamereact-navigation.jsdoesn't reflect the broader scope of native-module setup it performs.Consider renaming it (e.g.,
navigation-and-animation-setup.js) or splitting the non-navigation setup into a separate file for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__mocks__/react-navigation.js` around lines 1 - 29, The file __mocks__/react-navigation.js includes unrelated setup for worklets, gesture-handler, and reanimated; split or rename it so its name matches contents: either rename to something like navigation-and-animation-setup.js or separate into two files (e.g., react-navigation.mock.js containing jest.mock('@react-navigation/native') and exports mockNavigate/mockSetOptions, and native-modules-setup.js which requires 'react-native-gesture-handler/jestSetup', calls setUpTests() from 'react-native-reanimated', and mocks 'react-native-worklets'). Ensure references to mockNavigate and mockSetOptions remain exported from the navigation mock and keep the jest.mock('@react-navigation/native') block unchanged.
🤖 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/hooks/persisted/__mocks__/useDownload.ts`:
- Around line 1-2: Remove the unused exported constants from the mock: delete
DOWNLOAD_QUEUE and CHAPTER_DOWNLOADING from
src/hooks/persisted/__mocks__/useDownload.ts so the mock only contains
exports/tests rely on; ensure no tests import these symbols before removing by
grepping for DOWNLOAD_QUEUE or CHAPTER_DOWNLOADING, and if any test needs them,
keep or replace with minimal mock values in the mock module.
- Line 10: The mock's property name doesn't match the real hook export: the real
hook currently exports resumeDowndload (typo with double 'd') while the mock
defines resumeDownload; update either the real hook or the mock so names
match—recommended: fix the typo in the real module by renaming the symbol
resumeDowndload to resumeDownload in the hook (update both occurrences where
resumeDowndload is declared and exported/returned), or if you prefer the quick
fix, change the mock's property name to resumeDowndload so tests receive the
real export.
In `@src/hooks/persisted/__mocks__/useNovelSettings.ts`:
- Around line 22-23: The mock currently exports useNovelSettings as the default
which breaks callers that import it as a named export; change the mock to export
the hook as a named export (export const useNovelSettings = ...) instead of
default, keeping or exporting defaultNovelSettings as a named export as well if
needed (e.g., export { defaultNovelSettings }); update the export lines so they
match the real module's shape: named export for useNovelSettings and named
export for defaultNovelSettings.
In `@src/hooks/persisted/useNovel.ts`:
- Around line 575-583: The catch on getNovel() currently swallows errors; change
it to accept the error (e.g., .catch((err) => { ... })) and mirror the file's
existing pattern: in dev mode log the error (use the same dev-check used
elsewhere, e.g., __DEV__ or isDev, and console.error or the module logger) and
surface a user-facing toast (use the same toast API used around line 618, e.g.,
toast.show/toast.error) so failures are visible; keep setLoading(false) in
finally so loading state still clears. Ensure you reference getNovel, the catch
callback, setLoading, and the toast/log utilities already used in this module.
- Line 234: The pages array can shrink leaving pageIndex out-of-bounds and
causing currentPage to be undefined; add a useEffect in useNovel.ts that watches
pages and pageIndex and resets the persisted pageIndex state (the pageIndex
setter used by your MMKV-persisted hook) whenever pages.length <= pageIndex by
setting pageIndex to Math.max(0, pages.length - 1) (or 0 if pages is empty);
ensure the effect runs after pages changes so subsequent reads like const page =
pages[pageIndex] ?? '1' and other accesses at currentPage are safe and the
persisted value is updated.
---
Nitpick comments:
In `@__mocks__/react-navigation.js`:
- Around line 1-29: The file __mocks__/react-navigation.js includes unrelated
setup for worklets, gesture-handler, and reanimated; split or rename it so its
name matches contents: either rename to something like
navigation-and-animation-setup.js or separate into two files (e.g.,
react-navigation.mock.js containing jest.mock('@react-navigation/native') and
exports mockNavigate/mockSetOptions, and native-modules-setup.js which requires
'react-native-gesture-handler/jestSetup', calls setUpTests() from
'react-native-reanimated', and mocks 'react-native-worklets'). Ensure references
to mockNavigate and mockSetOptions remain exported from the navigation mock and
keep the jest.mock('@react-navigation/native') block unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
__mocks__/react-native-mmkv.js__mocks__/react-navigation.jsjest.config.jssrc/hooks/persisted/__mocks__/useDownload.tssrc/hooks/persisted/__mocks__/useNovel.tssrc/hooks/persisted/__mocks__/useNovelSettings.tssrc/hooks/persisted/__mocks__/usePlugins.tssrc/hooks/persisted/__mocks__/useTracker.tssrc/hooks/persisted/useNovel.tssrc/hooks/persisted/useNovelSettings.tssrc/services/migrate/migrateNovel.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/persisted/mocks/useNovel.ts
- src/hooks/persisted/mocks/useTracker.ts
|
|
||
| const getChapters = useCallback(async () => { | ||
| const page = pages[pageIndex] ?? 1; | ||
| const page = pages[pageIndex] ?? '1'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "useNovel.ts" --exec wc -l {} +Repository: CD-Z/lnreader
Length of output: 251
🏁 Script executed:
cat -n src/hooks/persisted/useNovel.ts | head -250 | tail -50Repository: CD-Z/lnreader
Length of output: 1740
🏁 Script executed:
cat -n src/hooks/persisted/useNovel.ts | sed -n '220,250p'Repository: CD-Z/lnreader
Length of output: 1103
🏁 Script executed:
cat -n src/hooks/persisted/useNovel.ts | sed -n '310,330p'Repository: CD-Z/lnreader
Length of output: 674
🏁 Script executed:
cat -n src/hooks/persisted/useNovel.ts | sed -n '1,100p'Repository: CD-Z/lnreader
Length of output: 4303
🏁 Script executed:
cat -n src/hooks/persisted/useNovel.ts | sed -n '100,200p'Repository: CD-Z/lnreader
Length of output: 3365
🏁 Script executed:
rg -n "useEffect|pageIndex" src/hooks/persisted/useNovel.tsRepository: CD-Z/lnreader
Length of output: 552
🏁 Script executed:
cat -n src/hooks/persisted/useNovel.ts | sed -n '566,650p'Repository: CD-Z/lnreader
Length of output: 2401
🏁 Script executed:
cat -n src/hooks/persisted/useNovel.ts | sed -n '650,693p'Repository: CD-Z/lnreader
Length of output: 1527
🏁 Script executed:
rg -n "pages\[pageIndex\]" src/hooks/persisted/useNovel.tsRepository: CD-Z/lnreader
Length of output: 232
🏁 Script executed:
cat -n src/hooks/persisted/useNovel.ts | sed -n '280,295p'Repository: CD-Z/lnreader
Length of output: 545
🏁 Script executed:
cat -n src/hooks/persisted/useNovel.ts | sed -n '156,162p'Repository: CD-Z/lnreader
Length of output: 222
🏁 Script executed:
rg -n "setPageIndex\|openPage" src/hooks/persisted/useNovel.ts -A 2Repository: CD-Z/lnreader
Length of output: 39
Reset pageIndex when pages array changes to prevent state inconsistency.
At lines 234, 291, and 323, pages[pageIndex] can return undefined if pageIndex is out of bounds. While lines 234 and 323 have a fallback to '1', the pageIndex state itself remains invalid. This causes state divergence: the pages array is valid but pageIndex is stale, and currentPage (line 89) becomes undefined. Since pageIndex is persisted via MMKV, it survives when the novel changes or pages are recalculated, but the array size can shrink.
Add a useEffect to validate and reset pageIndex when the pages array changes:
Suggested fix
+ useEffect(() => {
+ if (pages.length === 0) return;
+ if (pageIndex < 0 || pageIndex >= pages.length) {
+ setPageIndex(defaultPageIndex);
+ }
+ }, [pageIndex, pages, setPageIndex]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/persisted/useNovel.ts` at line 234, The pages array can shrink
leaving pageIndex out-of-bounds and causing currentPage to be undefined; add a
useEffect in useNovel.ts that watches pages and pageIndex and resets the
persisted pageIndex state (the pageIndex setter used by your MMKV-persisted
hook) whenever pages.length <= pageIndex by setting pageIndex to Math.max(0,
pages.length - 1) (or 0 if pages is empty); ensure the effect runs after pages
changes so subsequent reads like const page = pages[pageIndex] ?? '1' and other
accesses at currentPage are safe and the persisted value is updated.
Comment on lines
+575
to
+583
| getNovel() | ||
| .catch(() => { | ||
| // Error is handled - novel stays undefined and loading becomes false | ||
| }) | ||
| .finally(() => { | ||
| //? Sometimes loading state changes doesn't trigger rerender causing NovelScreen to be in endless loading state | ||
| setLoading(false); | ||
| // getNovel(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file and the specific lines mentioned
wc -l src/hooks/persisted/useNovel.tsRepository: CD-Z/lnreader
Length of output: 92
🏁 Script executed:
# View the context around lines 575-583
sed -n '570,590p' src/hooks/persisted/useNovel.tsRepository: CD-Z/lnreader
Length of output: 649
🏁 Script executed:
# Check imports and available utilities in the file
head -50 src/hooks/persisted/useNovel.tsRepository: CD-Z/lnreader
Length of output: 2014
🏁 Script executed:
# Search for showToast usage in this file or similar files
rg "showToast" src/hooks/ -A 2 -B 2Repository: CD-Z/lnreader
Length of output: 2582
🏁 Script executed:
# Check for error handling patterns in other hooks
rg "\.catch\(" src/hooks/ -A 3 | head -50Repository: CD-Z/lnreader
Length of output: 1381
🏁 Script executed:
# Look for getString usage
rg "getString" src/hooks/persisted/useNovel.tsRepository: CD-Z/lnreader
Length of output: 309
🏁 Script executed:
# Check if __DEV__ is used elsewhere in the file
rg "__DEV__" src/hooks/persisted/useNovel.tsRepository: CD-Z/lnreader
Length of output: 95
Don't swallow initial novel-load failures silently.
At lines 575-578, the catch block discards the error without any feedback. This is inconsistent with the error handling pattern used elsewhere in the same file (e.g., around line 618), where errors are logged in dev mode and surfaced to users via toast. Fetch/DB failures here become invisible, making production debugging difficult.
Suggested fix
getNovel()
- .catch(() => {
- // Error is handled - novel stays undefined and loading becomes false
- })
+ .catch(error => {
+ if (__DEV__) console.error(error);
+ showToast(
+ error instanceof Error
+ ? error.message
+ : getString('updatesScreen.unableToGetNovel'),
+ );
+ })
.finally(() => {
//? Sometimes loading state changes doesn't trigger rerender causing NovelScreen to be in endless loading state
setLoading(false);
// getNovel();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/persisted/useNovel.ts` around lines 575 - 583, The catch on
getNovel() currently swallows errors; change it to accept the error (e.g.,
.catch((err) => { ... })) and mirror the file's existing pattern: in dev mode
log the error (use the same dev-check used elsewhere, e.g., __DEV__ or isDev,
and console.error or the module logger) and surface a user-facing toast (use the
same toast API used around line 618, e.g., toast.show/toast.error) so failures
are visible; keep setLoading(false) in finally so loading state still clears.
Ensure you reference getNovel, the catch callback, setLoading, and the toast/log
utilities already used in this module.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hooks/persisted/__mocks__/useDownload.ts`:
- Around line 1-12: The mock currently uses module-level mutable state
(mockDownloadQueue, mockDownloadingChapterIds) and creates handler fns
inconsistently, causing cross-test leakage; change useDownload to
mockImplementation that creates a fresh downloadQueue array, a new Set for
downloadingChapterIds, and new jest.fn() handlers on each invocation (i.e.
inside the jest.fn(() => ({ ... })) factory) so each call/re-render gets
isolated state and stable call tracking for resumeDownload, downloadChapter,
downloadChapters, pauseDownload and cancelDownload; optionally add and export a
reset helper if tests need programmatic resets.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/hooks/persisted/__mocks__/useDownload.tssrc/hooks/persisted/__mocks__/useNovelSettings.tssrc/hooks/persisted/useDownload.ts
Comment on lines
+1
to
+12
| const mockDownloadQueue: any[] = []; | ||
| const mockDownloadingChapterIds = new Set<number>(); | ||
|
|
||
| const useDownload = jest.fn(() => ({ | ||
| downloadQueue: mockDownloadQueue, | ||
| downloadingChapterIds: mockDownloadingChapterIds, | ||
| resumeDownload: jest.fn(), | ||
| downloadChapter: jest.fn(), | ||
| downloadChapters: jest.fn(), | ||
| pauseDownload: jest.fn(), | ||
| cancelDownload: jest.fn(), | ||
| })); |
There was a problem hiding this comment.
Prevent cross-test state leakage and unstable mock call tracking.
The mock keeps mutable module-level state and creates new jest.fn() handlers on every hook call. This can leak state between tests and make call assertions inconsistent across rerenders/invocations.
💡 Suggested fix
-const mockDownloadQueue: any[] = [];
-const mockDownloadingChapterIds = new Set<number>();
-
-const useDownload = jest.fn(() => ({
- downloadQueue: mockDownloadQueue,
- downloadingChapterIds: mockDownloadingChapterIds,
- resumeDownload: jest.fn(),
- downloadChapter: jest.fn(),
- downloadChapters: jest.fn(),
- pauseDownload: jest.fn(),
- cancelDownload: jest.fn(),
-}));
+const createUseDownloadMockState = () => ({
+ downloadQueue: [] as any[],
+ downloadingChapterIds: new Set<number>(),
+ resumeDownload: jest.fn(),
+ downloadChapter: jest.fn(),
+ downloadChapters: jest.fn(),
+ pauseDownload: jest.fn(),
+ cancelDownload: jest.fn(),
+});
+
+let mockState = createUseDownloadMockState();
+
+const useDownload = jest.fn(() => mockState);
+
+export const resetUseDownloadMock = () => {
+ mockState = createUseDownloadMockState();
+};📝 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 mockDownloadQueue: any[] = []; | |
| const mockDownloadingChapterIds = new Set<number>(); | |
| const useDownload = jest.fn(() => ({ | |
| downloadQueue: mockDownloadQueue, | |
| downloadingChapterIds: mockDownloadingChapterIds, | |
| resumeDownload: jest.fn(), | |
| downloadChapter: jest.fn(), | |
| downloadChapters: jest.fn(), | |
| pauseDownload: jest.fn(), | |
| cancelDownload: jest.fn(), | |
| })); | |
| const createUseDownloadMockState = () => ({ | |
| downloadQueue: [] as any[], | |
| downloadingChapterIds: new Set<number>(), | |
| resumeDownload: jest.fn(), | |
| downloadChapter: jest.fn(), | |
| downloadChapters: jest.fn(), | |
| pauseDownload: jest.fn(), | |
| cancelDownload: jest.fn(), | |
| }); | |
| let mockState = createUseDownloadMockState(); | |
| const useDownload = jest.fn(() => mockState); | |
| export const resetUseDownloadMock = () => { | |
| mockState = createUseDownloadMockState(); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/persisted/__mocks__/useDownload.ts` around lines 1 - 12, The mock
currently uses module-level mutable state (mockDownloadQueue,
mockDownloadingChapterIds) and creates handler fns inconsistently, causing
cross-test leakage; change useDownload to mockImplementation that creates a
fresh downloadQueue array, a new Set for downloadingChapterIds, and new
jest.fn() handlers on each invocation (i.e. inside the jest.fn(() => ({ ... }))
factory) so each call/re-render gets isolated state and stable call tracking for
resumeDownload, downloadChapter, downloadChapters, pauseDownload and
cancelDownload; optionally add and export a reset helper if tests need
programmatic resets.
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
Documentation
Tests
Chores
Bug Fixes