Skip to content

Expo 55 upgrade#28

Closed
CD-Z wants to merge 9 commits intomasterfrom
expo-55-upgrade
Closed

Expo 55 upgrade#28
CD-Z wants to merge 9 commits intomasterfrom
expo-55-upgrade

Conversation

@CD-Z
Copy link
Copy Markdown
Owner

@CD-Z CD-Z commented Mar 28, 2026

Summary by CodeRabbit

  • Updates

    • Upgraded React, React Native, and Expo-related tooling for compatibility and performance.
    • Refreshed app theme to EdgeToEdge with improved status bar appearance.
    • Improved build configuration and Gradle/tooling versions.
  • Bug Fixes

    • Fixed database initialization/migration edge cases.
    • Corrected several hook dependency issues for more reliable state updates.
    • Stabilized cookie and local storage operations.
  • Chores

    • Added Android/IOS build and config adjustments and a new Android cleanup script.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 28, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3c089b1a-6ae6-4405-97da-b0320cd47269

📥 Commits

Reviewing files that changed from the base of the PR and between 7817a9a and 9f68441.

📒 Files selected for processing (2)
  • android/app/src/debug/AndroidManifest.xml
  • android/settings.gradle

📝 Walkthrough

Walkthrough

Upgrades RN/React/Expo tooling, integrates Expo native build/autolinking across Android/iOS, migrates env vars from @env to react-native-config, replaces MMKV API/init and cookie library, adjusts Gradle/Metro/Babel configs, adds conditional DB migration SQL, and performs several UI style and minor hook dependency tweaks.

Changes

Cohort / File(s) Summary
Gradle & Build infra
android/build.gradle, android/gradle/wrapper/gradle-wrapper.properties, android/gradlew, android/settings.gradle
Upgrade Gradle wrapper to 9.0.0, add expo-root-project plugin, convert plugin includes to Node-resolved dynamic includes, and integrate Expo autolinking settings.
Android app & manifest
android/app/src/main/java/.../MainApplication.kt, android/app/src/main/AndroidManifest.xml, android/app/src/debug/AndroidManifest.xml
Switch to ExpoReactHostFactory default host, add `locale
Android resources & theme
android/app/src/main/res/values/styles.xml, android/app/src/main/res/values/strings.xml
Theme changed to Theme.EdgeToEdge, windowLightStatusBar added, explicit bar colors removed, and XML prologs/trailing newlines cleaned.
Metro & Babel
metro.config.js, babel.config.js
Import getDefaultConfig from expo/metro-config, keep merge from RN metro config; convert Babel config export to ES module and remove react-native-dotenv plugin mapping.
Package & scripts
package.json, .gitignore
Add clean:android script, bump React/React-Native/Expo and many deps, replace cookie lib, and add .sisyphus/ to .gitignore.
Env config migration
env.d.ts, src/screens/more/About.tsx, src/services/Trackers/aniList.ts, src/services/Trackers/myAnimeList.ts
Migrate from @env to react-native-config; consolidate env values into NativeConfig and read via Config.
MMKV & storage API changes
src/utils/mmkv/mmkv.ts, src/plugins/helpers/storage.ts, src/plugins/pluginManager.ts, src/hooks/persisted/*
Switch MMKV initialization to createMMKV(), replace .delete calls with .remove across storage and hooks.
Database & migrations
drizzle/migrations.js, src/database/db.ts, src/database/queries/__tests__/testDbManager.ts
Remove journal export, add runtime SQLite PRAGMA schema introspection and conditional ALTER TABLE for __drizzle_migrations; adjust test SQL placeholder import alias.
iOS project & bridging
ios/LNReader.xcodeproj/project.pbxproj, ios/LNReader/LNReader-Bridging-Header.h
Set SWIFT_VERSION = 5.0 in build configs and add #import <Expo/Expo.h> to bridging header; minor pbxproj comment formatting tweaks.
Cookie manager swap
src/screens/settings/SettingsAdvancedScreen.tsx
Swap import from @react-native-cookies/cookies@preeternal/react-native-cookie-manager, usage (clearAll) retained.
UI/style refinements
src/components/.../BottomSheet.tsx, src/components/List/List.tsx, src/screens/.../*.tsx
Extract inline styles to StyleSheet entries, adjust modal backgroundStyle to array, change some icon colors to theme.primary, and minor style renames.
Hook deps & minor logic
src/hooks/persisted/useNovel.ts, src/hooks/persisted/useNovelSettings.ts, src/screens/novel/components/NovelScreenList.tsx
Update dependency arrays to include pages.length and setSelected; remove ESLint file directive.
Mocks & tests
__mocks__/index.js, __mocks__/react-native-nitro-modules.js, src/database/queries/__tests__/testDbManager.ts
Change mocks to initialize react-native-nitro-modules, convert mock to Jest factory with ES-module default shape, and update test placeholder import usage.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~70 minutes

Possibly related PRs

  • Pnpm and upgrade #17 — Overlapping dependency and tooling upgrades (package.json, metro/babel, Gradle and RN/Expo tooling).
  • Rock #18 — Related Android and Metro integration changes (MainApplication, settings.gradle, metro.config).
  • Pnpm and upgrade #14 — Similar build/config migrations touching Gradle, Metro, and package management.

Poem

🐰 In burrows of code I nibbled and hopped,
Upgraded the rails where packages dropped.
Cookies and storage took a new route,
Env vars now whisper from Config, no doubt—
Hop, patch, commit — my backlog's well-topped! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'Expo 55 upgrade' clearly summarizes the main objective of the changeset, which is upgrading the Expo SDK from version 54 to version 55 across all platform configurations (iOS, Android, web, and dependencies).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch expo-55-upgrade

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/plugins/helpers/storage.ts (1)

68-70: ⚠️ Potential issue | 🔴 Critical

Change store.delete() to store.remove() for react-native-mmkv v4.x compatibility.

The delete() method was renamed to remove() in react-native-mmkv v4.x (the project uses v4.3.0) because delete is a reserved keyword in C++. The code on line 69 calls the non-existent store.delete() method, which will cause a runtime error. This should be changed to store.remove(this.#pluginID + PLUGIN_STORAGE + key). Note that pluginManager.ts line 129 has the same issue.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/plugins/helpers/storage.ts` around lines 68 - 70, Replace the erroneous
call to the removed API by changing the delete method in the Storage helper (the
class method named delete(key: string) that currently calls
store.delete(this.#pluginID + PLUGIN_STORAGE + key)) to call store.remove(...)
instead; update the same pattern found in pluginManager.ts where
store.delete(...) is used so both usages call store.remove(this.#pluginID +
PLUGIN_STORAGE + key) to be compatible with react-native-mmkv v4.x.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@android/app/src/main/AndroidManifest.xml`:
- Line 22: The AndroidManifest application tag currently sets
android:usesCleartextTraffic="true" which enables HTTP app-wide; remove or set
that attribute to false and instead add a network security config via
android:networkSecurityConfig="@xml/network_security_config" on the Application
element, then create a res/xml/network_security_config.xml that permits
cleartext only for the specific domains that need it (using domain-config with
cleartextTrafficPermitted=true for those hosts and a default disallow rule);
update the Application element (symbol: android:usesCleartextTraffic and
android:networkSecurityConfig) and add the new network security config XML
resource to restrict cleartext to approved hosts.
- Line 22: The manifest currently sets android:allowBackup="true" on the
<application> entry (android:name=".MainApplication"); add explicit backup rules
instead of leaving the default by either setting
android:fullBackupContent="@xml/backup_rules" (or
android:dataExtractionRules="@xml/backup_rules" on newer APIs) or disable
backups by setting android:allowBackup="false". Create an XML resource (e.g.,
res/xml/backup_rules.xml) that explicitly excludes MMKV storage,
SharedPreferences, download queues and any plugin/session folders (use exclude
tags for the MMKV directory and prefs) and reference it via
android:fullBackupContent/android:dataExtractionRules on the same <application>
tag so persisted app data is not inadvertently restored.

In `@android/gradle/wrapper/gradle-wrapper.properties`:
- Line 3: Update the Gradle distribution URL in gradle-wrapper.properties by
changing the distributionUrl value (the distributionUrl property) from
gradle-9.0.0-bin.zip to gradle-9.3.1-bin.zip so the project uses Gradle 9.3.1
required by React Native 0.83 / Expo SDK 55; ensure no other characters or
escaping are altered.

In `@android/settings.gradle`:
- Around line 25-27: The current settings.gradle unconditionally calls
expoAutolinking.rnConfigCommand via
extensions.configure(com.facebook.react.ReactSettingsExtension) { ex ->
ex.autolinkLibrariesFromCommand(expoAutolinking.rnConfigCommand) }, which
bypasses the documented EXPO_USE_COMMUNITY_AUTOLINKING escape hatch; update this
block to check the EXPO_USE_COMMUNITY_AUTOLINKING environment variable (or
System.getenv("EXPO_USE_COMMUNITY_AUTOLINKING")) and only call
ex.autolinkLibrariesFromCommand(expoAutolinking.rnConfigCommand) when the env
var is not set (or is false/empty), otherwise skip calling rnConfigCommand so
community autolinking is used while keeping Expo autolinking for Expo modules.

In `@package.json`:
- Line 161: The package.json currently pins "react-test-renderer" to 19.2.4
which mismatches the React version and breaks peer dependency checks; update the
"react-test-renderer" entry in package.json to 19.2.0 so it aligns with the
installed "react" version (and re-run npm/yarn install to update lockfile),
ensuring the dependency name "react-test-renderer" is the only change.

In `@src/hooks/__tests__/useNovel.test.ts`:
- Line 81: Remove the stray debug console.log call causing the ESLint no-console
failure: delete the statement console.log('--- Starting test ---'); from the
useNovel.test.ts test file so the test suite does not emit console output and
the lint pipeline passes; ensure no other console.* debug statements remain in
that test file.

In `@src/navigators/Main.tsx`:
- Around line 50-54: Remove the debug console.log in MainNavigator (the stray
console.log that prints isOnboarded and MMKVStorage.getAllKeys()). Replace it by
either deleting it entirely or moving any necessary debug output into a one-time
development-only logger: wrap logging in a __DEV__ check and call it from a
useEffect inside the MainNavigator component so it runs once, and do not log
MMKVStorage.getAllKeys() (avoid printing storage keys); alternatively use the
project’s Logger.debug method if available instead of console.log.

In `@src/plugins/helpers/storage.ts`:
- Line 76: The line with the typo `consoele.log('Clearing all plugin storage for
pluginID:', this.#pluginID);` inside the clearAll method should be removed (do
not replace with console.log to avoid ESLint no-console); eliminate this debug
statement entirely from the clearAll implementation (refer to the clearAll
method and the this.#pluginID field) — if a persisted log is required instead,
use the project's logger API (e.g., processLogger or existing module logger)
rather than console.

In `@src/screens/onboarding/OnboardingScreen.tsx`:
- Around line 65-69: Remove the debug console.log call in OnboardingScreen.tsx
(the console.log that prints 'complete onboarding' along with
MMKVStorage.getAllKeys() and MMKVStorage.getBoolean('IS_ONBOARDED')); either
delete the statement entirely or replace it with a safe, environment-gated
logger (e.g., use the project's logger at debug level and avoid logging all MMKV
keys or any sensitive data) so the no-console ESLint rule and data-leak concerns
are resolved.

In `@src/screens/settings/SettingsAdvancedScreen.tsx`:
- Line 28: Remove the debug console.log in SettingsAdvancedScreen (the lone
console.log('Cookies cleared') call) since it violates the no-console rule and
the toast already provides user feedback; locate the console.log inside the
SettingsAdvancedScreen component (the clear-cookies handler) and delete that
statement, then run lint/tests to verify no remaining console usages.

In `@src/utils/mmkv/mmkv.ts`:
- Around line 1-3: The mock for react-native-mmkv is missing createMMKV which
causes MMKVStorage = createMMKV() to throw; update the mock at
__mocks__/react-native-mmkv.js to export a createMMKV function that returns an
object implementing all methods used by code/tests (getString, set, remove,
getBoolean, getAllKeys, getNumber, addOnValueChangedListener, clearAll — and
delete if referenced), and ensure NitroModules is still exported; this will let
MMKVStorage (created via createMMKV()) initialize without errors in tests.

---

Outside diff comments:
In `@src/plugins/helpers/storage.ts`:
- Around line 68-70: Replace the erroneous call to the removed API by changing
the delete method in the Storage helper (the class method named delete(key:
string) that currently calls store.delete(this.#pluginID + PLUGIN_STORAGE +
key)) to call store.remove(...) instead; update the same pattern found in
pluginManager.ts where store.delete(...) is used so both usages call
store.remove(this.#pluginID + PLUGIN_STORAGE + key) to be compatible with
react-native-mmkv v4.x.
🪄 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: 5c2858ff-ff03-4d45-acd8-d3a2e1f30ca7

📥 Commits

Reviewing files that changed from the base of the PR and between 44e035a and 894e8bb.

⛔ Files ignored due to path filters (2)
  • android/gradle/wrapper/gradle-wrapper.jar is excluded by !**/*.jar
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (22)
  • .gitignore
  • android/app/src/main/AndroidManifest.xml
  • android/app/src/main/java/com/rajarsheechatterjee/LNReader/MainApplication.kt
  • android/app/src/main/res/values/strings.xml
  • android/app/src/main/res/values/styles.xml
  • android/build.gradle
  • android/gradle/wrapper/gradle-wrapper.properties
  • android/gradlew
  • android/settings.gradle
  • ios/LNReader.xcodeproj/project.pbxproj
  • ios/LNReader/LNReader-Bridging-Header.h
  • metro.config.js
  • package.json
  • src/hooks/__tests__/useNovel.test.ts
  • src/hooks/persisted/useNovel.ts
  • src/hooks/persisted/usePlugins.ts
  • src/hooks/persisted/useTrackedNovel.ts
  • src/navigators/Main.tsx
  • src/plugins/helpers/storage.ts
  • src/screens/onboarding/OnboardingScreen.tsx
  • src/screens/settings/SettingsAdvancedScreen.tsx
  • src/utils/mmkv/mmkv.ts

@CD-Z CD-Z force-pushed the expo-55-upgrade branch from 894e8bb to efa8935 Compare March 30, 2026 15:25
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
package.json (1)

62-64: Move build tooling packages to devDependencies.

@react-native/codegen, @react-native/gradle-plugin, and @react-native/assets-registry are build-time tools used only during Android build and asset bundling. They have no runtime usage in the application and should be moved to devDependencies to keep production bundle size minimal and follow best practices.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package.json` around lines 62 - 64, The three packages
"@react-native/codegen", "@react-native/gradle-plugin", and
"@react-native/assets-registry" are build-time tools and should be moved from
"dependencies" to "devDependencies" in package.json; edit package.json to remove
those three entries under dependencies and add them (with the same versions)
under devDependencies, then run your package manager (npm/yarn/pnpm) to update
lockfiles so the change is reflected.
🤖 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/utils/mmkv/mmkv.ts`:
- Line 4: Remove the debug console.log that prints
platformContext?.getBaseDirectory() from the module (it runs at import time and
violates the no-console rule); delete that line from src/utils/mmkv/mmkv.ts or
replace it with the project's logger (e.g., use an existing logger.debug call)
if persistent diagnostics are required, ensuring no direct console usage remains
in the module.
- Around line 1-3: The mock for react-native-mmkv must export the MMKV class and
platformContext so imports in src/utils/mmkv/mmkv.ts succeed; update
__mocks__/react-native-mmkv.js to add an exported MockMMKV (named MMKV) and a
platformContext export alongside the existing NitroModules.createHybridObject
export (keep createHybridObject as a jest.fn or similar) so that MMKVStorage =
new MMKV() and any use of platformContext resolve in tests.

---

Nitpick comments:
In `@package.json`:
- Around line 62-64: The three packages "@react-native/codegen",
"@react-native/gradle-plugin", and "@react-native/assets-registry" are
build-time tools and should be moved from "dependencies" to "devDependencies" in
package.json; edit package.json to remove those three entries under dependencies
and add them (with the same versions) under devDependencies, then run your
package manager (npm/yarn/pnpm) to update lockfiles so the change is reflected.
🪄 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: 28e5a4f7-f326-4720-84d2-eaf1862460af

📥 Commits

Reviewing files that changed from the base of the PR and between 894e8bb and efa8935.

⛔ Files ignored due to path filters (2)
  • android/gradle/wrapper/gradle-wrapper.jar is excluded by !**/*.jar
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (16)
  • .gitignore
  • android/app/src/main/AndroidManifest.xml
  • android/app/src/main/java/com/rajarsheechatterjee/LNReader/MainApplication.kt
  • android/app/src/main/res/values/strings.xml
  • android/app/src/main/res/values/styles.xml
  • android/build.gradle
  • android/gradle/wrapper/gradle-wrapper.properties
  • android/gradlew
  • android/settings.gradle
  • babel.config.js
  • ios/LNReader.xcodeproj/project.pbxproj
  • ios/LNReader/LNReader-Bridging-Header.h
  • metro.config.js
  • package.json
  • src/screens/settings/SettingsAdvancedScreen.tsx
  • src/utils/mmkv/mmkv.ts
✅ Files skipped from review due to trivial changes (8)
  • ios/LNReader/LNReader-Bridging-Header.h
  • android/build.gradle
  • android/app/src/main/res/values/strings.xml
  • android/gradle/wrapper/gradle-wrapper.properties
  • android/gradlew
  • .gitignore
  • android/settings.gradle
  • android/app/src/main/java/com/rajarsheechatterjee/LNReader/MainApplication.kt
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/screens/settings/SettingsAdvancedScreen.tsx
  • metro.config.js
  • android/app/src/main/res/values/styles.xml
  • android/app/src/main/AndroidManifest.xml
  • ios/LNReader.xcodeproj/project.pbxproj

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
drizzle/migrations.js (1)

3-8: Fix inconsistent indentation and formatting.

The export statement has unusual leading indentation (2 spaces before export default) and inconsistent brace placement. This appears to be auto-generated, but the formatting should be cleaned up.

♻️ Proposed formatting fix
-  export default {
-    migrations: {
-      "20251222152612_past_mandrill": m0000
-}
-  }
-  
+export default {
+  migrations: {
+    "20251222152612_past_mandrill": m0000,
+  },
+};
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@drizzle/migrations.js` around lines 3 - 8, Fix the inconsistent indentation
and brace placement around the default export: reformat the object so there is
no leading whitespace before "export default", align opening and closing braces
consistently, and ensure the migrations object lists the entry
"20251222152612_past_mandrill": m0000 with standard indentation; update the
block containing the export default { migrations: {
"20251222152612_past_mandrill": m0000 } } so braces and commas follow project
style (single-line or multi-line consistent) and spacing is normalized.
src/database/db.ts (2)

126-139: Inconsistent use of db vs _db references.

Line 128 uses db.executeRawSync while lines 133 and 136 use _db.executeRawSync. While both reference the same underlying database instance (per lines 27 and 33), this inconsistency reduces readability. Consider using _db consistently within this function since it's the local constant.

♻️ Proposed fix for consistency
     // To resolve issue in drizzle before beta 16
-    const results = db.executeRawSync(
+    const results = _db.executeRawSync(
       `PRAGMA table_info(__drizzle_migrations);`,
     );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/database/db.ts` around lines 126 - 139, The block mixes two database
variables (db and _db) which hurts readability; update the code in the
migration-check section so it uses the same local constant (_db) throughout:
call _db.executeRawSync for the PRAGMA query and for both ALTER TABLE statements
(replace db.executeRawSync with _db.executeRawSync) so all four calls
consistently use the local _db variable used elsewhere in this function.

131-131: Add type safety for PRAGMA result parsing.

The type assertion row: unknown[] is loose. The PRAGMA table_info returns rows where index 1 is the column name, which is correct, but a more explicit type or comment would improve maintainability.

♻️ Suggested improvement
-    const resolved = results.some((row: unknown[]) => row[1] === 'applied_at');
+    // PRAGMA table_info returns: [cid, name, type, notnull, dflt_value, pk]
+    const resolved = results.some(
+      (row: [number, string, string, number, unknown, number]) => row[1] === 'applied_at'
+    );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/database/db.ts` at line 131, The loose cast row: unknown[] should be
tightened: define a concise tuple/type for PRAGMA table_info rows (e.g., type
PragmaColumn = [number, string, string, number, string | null, number]) or at
least perform a runtime/type guard on results before indexing; update the line
with results.some to use a type-guarded callback (or cast to PragmaColumn[])
that checks Array.isArray(row) and typeof row[1] === 'string' before comparing
to 'applied_at' so the variable resolved and the source results are type-safe
and avoid indexing unknown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@drizzle/20251222152612_past_mandrill/migration.sql`:
- Around line 1-2: The SQL uses a misspelled and unsupported clause "IF
EXITS"/"IF EXISTS" with ALTER TABLE (table '__drizzle_migrations'), causing a
syntax error; remove the invalid clause and correct the typo so the statements
use valid SQLite ALTER TABLE syntax: run ALTER TABLE '__drizzle_migrations' ADD
COLUMN 'applied_at' text; and ALTER TABLE '__drizzle_migrations' ADD COLUMN
'name' text; (or, if you need safety, perform an existence check against
sqlite_master before issuing the ALTER TABLE).

---

Nitpick comments:
In `@drizzle/migrations.js`:
- Around line 3-8: Fix the inconsistent indentation and brace placement around
the default export: reformat the object so there is no leading whitespace before
"export default", align opening and closing braces consistently, and ensure the
migrations object lists the entry "20251222152612_past_mandrill": m0000 with
standard indentation; update the block containing the export default {
migrations: { "20251222152612_past_mandrill": m0000 } } so braces and commas
follow project style (single-line or multi-line consistent) and spacing is
normalized.

In `@src/database/db.ts`:
- Around line 126-139: The block mixes two database variables (db and _db) which
hurts readability; update the code in the migration-check section so it uses the
same local constant (_db) throughout: call _db.executeRawSync for the PRAGMA
query and for both ALTER TABLE statements (replace db.executeRawSync with
_db.executeRawSync) so all four calls consistently use the local _db variable
used elsewhere in this function.
- Line 131: The loose cast row: unknown[] should be tightened: define a concise
tuple/type for PRAGMA table_info rows (e.g., type PragmaColumn = [number,
string, string, number, string | null, number]) or at least perform a
runtime/type guard on results before indexing; update the line with results.some
to use a type-guarded callback (or cast to PragmaColumn[]) that checks
Array.isArray(row) and typeof row[1] === 'string' before comparing to
'applied_at' so the variable resolved and the source results are type-safe and
avoid indexing unknown.
🪄 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: fc6b9409-baea-4dd5-ab8a-b3ef1ab38744

📥 Commits

Reviewing files that changed from the base of the PR and between efa8935 and d4797a1.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (16)
  • babel.config.js
  • drizzle/20251222152612_past_mandrill/migration.sql
  • drizzle/migrations.js
  • env.d.ts
  • package.json
  • src/database/db.ts
  • src/hooks/persisted/useNovel.ts
  • src/hooks/persisted/usePlugins.ts
  • src/hooks/persisted/useTrackedNovel.ts
  • src/plugins/helpers/storage.ts
  • src/plugins/pluginManager.ts
  • src/screens/more/About.tsx
  • src/screens/novel/components/JumpToChapterModal.tsx
  • src/services/Trackers/aniList.ts
  • src/services/Trackers/myAnimeList.ts
  • src/utils/mmkv/mmkv.ts
✅ Files skipped from review due to trivial changes (3)
  • src/services/Trackers/aniList.ts
  • src/services/Trackers/myAnimeList.ts
  • src/screens/novel/components/JumpToChapterModal.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/utils/mmkv/mmkv.ts
  • babel.config.js

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/hooks/persisted/useNovel.ts (1)

279-289: Redundant dependency: both settingsFilter and novelSettings.filter are in the array.

The callback uses settingsFilter (line 239) and novelSettings.filter (line 274) separately. Since settingsFilter is memoized from novelSettings.filter (lines 111-114), including both is redundant — when novelSettings.filter changes, settingsFilter also updates. Consider using only one consistently throughout the callback.

♻️ Suggested refactor: Use settingsFilter consistently
       const unread = await _getFirstUnreadChapter(
         novel.id,
-        novelSettings.filter,
+        settingsFilter,
         page,
       );
       setFirstUnreadChapter(unread ?? undefined);
     }
   }, [
     pages,
     pageIndex,
     novel,
     settingsSort,
     settingsFilter,
     setChapters,
-    novelSettings.filter,
     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 279 - 289, The dependency array
contains both settingsFilter and novelSettings.filter which is redundant because
settingsFilter is a memoized derivation of novelSettings.filter; pick one and
use it consistently: replace any direct uses of novelSettings.filter inside the
callback with settingsFilter (or remove settingsFilter and always derive from
novelSettings.filter) and then remove the duplicate from the dependency array so
only the chosen identifier (settingsFilter or novelSettings.filter) remains;
update the useEffect/useCallback dependency array that currently lists pages,
pageIndex, novel, settingsSort, settingsFilter, settingsFilter (remove
novelSettings.filter), setChapters, pluginId, novelPath and ensure the
referenced variable names (settingsFilter or novelSettings.filter) match the
actual variables used in the callback body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/hooks/persisted/useNovel.ts`:
- Around line 279-289: The dependency array contains both settingsFilter and
novelSettings.filter which is redundant because settingsFilter is a memoized
derivation of novelSettings.filter; pick one and use it consistently: replace
any direct uses of novelSettings.filter inside the callback with settingsFilter
(or remove settingsFilter and always derive from novelSettings.filter) and then
remove the duplicate from the dependency array so only the chosen identifier
(settingsFilter or novelSettings.filter) remains; update the
useEffect/useCallback dependency array that currently lists pages, pageIndex,
novel, settingsSort, settingsFilter, settingsFilter (remove
novelSettings.filter), setChapters, pluginId, novelPath and ensure the
referenced variable names (settingsFilter or novelSettings.filter) match the
actual variables used in the callback body.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c40ea5c9-bfe0-4454-a0ed-c454e529911d

📥 Commits

Reviewing files that changed from the base of the PR and between d4797a1 and 7817a9a.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (16)
  • __mocks__/index.js
  • __mocks__/react-native-nitro-modules.js
  • babel.config.js
  • drizzle/migrations.js
  • package.json
  • src/components/BottomSheet/BottomSheet.tsx
  • src/components/List/List.tsx
  • src/database/db.ts
  • src/database/queries/__tests__/testDbManager.ts
  • src/hooks/persisted/useNovel.ts
  • src/hooks/persisted/useNovelSettings.ts
  • src/screens/Categories/components/CategoryCard.tsx
  • src/screens/novel/components/ChapterItem.tsx
  • src/screens/novel/components/Info/NovelInfoHeader.tsx
  • src/screens/novel/components/NovelScreenList.tsx
  • src/screens/reader/components/ReaderBottomSheet/ReaderSheetPreferenceItem.tsx
💤 Files with no reviewable changes (1)
  • drizzle/migrations.js
✅ Files skipped from review due to trivial changes (4)
  • src/screens/novel/components/Info/NovelInfoHeader.tsx
  • src/database/queries/tests/testDbManager.ts
  • src/screens/Categories/components/CategoryCard.tsx
  • src/hooks/persisted/useNovelSettings.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/database/db.ts
  • babel.config.js
  • package.json

@CD-Z CD-Z closed this Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant