feat(settings): add custom code settings screen#30
Open
CD-Z wants to merge 46 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds snippet-based custom JS/CSS and text removal/replacement rules, a browser selection overlay, keyboard-aware editors/modals, a tabbed Custom Code settings screen with preview, and refactors the reader WebView to use composed hooks for TTS, custom code, and text modifications. ChangesCustom Code & Text Manipulation Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Common/ToggleButton.tsx (1)
34-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDisabled state is functionally applied but not visually/accessibly communicated.
Pressableis disabled (Line 50), but the icon/style still looks active. Add a disabled visual treatment and accessibility state so users can tell the control is unavailable.🎯 Suggested tweak
export const ToggleButton: React.FC<ToggleButtonProps> = ({ @@ <Pressable android_ripple={{ color: theme.rippleColor }} style={[ styles.toggleButtonPressable, getToggleButtonPressableStyle(selected, theme), + disabled && styles.disabledPressable, ]} onPress={onPress} disabled={disabled} + accessibilityState={{ disabled: !!disabled }} > @@ const styles = StyleSheet.create({ @@ toggleButtonPressable: { padding: 8, alignItems: 'center', justifyContent: 'center', }, + disabledPressable: { + opacity: 0.5, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Common/ToggleButton.tsx` around lines 34 - 55, ToggleButton's disabled prop is set on Pressable but there's no visual or accessibility indication; update the ToggleButton component to apply a disabled visual treatment and accessibility state: when disabled is true, pass accessibilityState={{ disabled: true }} to Pressable, add a disabled style (e.g. lower opacity or use theme.disabled/onSurfaceVariant) by extending styles.toggleButtonPressable or via getToggleButtonPressableStyle(selected, theme, disabled), and change the MaterialCommunityIcons color to use the disabled color when disabled instead of theme.primary/theme.onSurface so users can both see and programmatically detect the control is unavailable.src/screens/reader/components/WebViewReader.tsx (1)
35-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let malformed WebView messages crash the reader.
Both the dev logger and the dispatcher unconditionally
JSON.parsethe payload. With plugin JS and user custom JS now injected, one plain-stringpostMessageis enough to throw before the reader can recover.Suggested fix
-const onLogMessage = (payload: { nativeEvent: { data: string } }) => { - const dataPayload = JSON.parse(payload.nativeEvent.data); - if (dataPayload) { - if (dataPayload.type === 'console') { - /* eslint-disable no-console */ - console.info(`[Console] ${JSON.stringify(dataPayload.msg, null, 2)}`); - } - } -}; +const onLogMessage = (dataPayload: unknown) => { + if ( + dataPayload && + typeof dataPayload === 'object' && + 'type' in dataPayload && + dataPayload.type === 'console' + ) { + /* eslint-disable no-console */ + console.info(`[Console] ${JSON.stringify((dataPayload as { msg?: unknown }).msg, null, 2)}`); + } +}; @@ onMessage={(ev: { nativeEvent: { data: string } }) => { - __DEV__ && onLogMessage(ev); - const event: WebViewPostEvent = JSON.parse(ev.nativeEvent.data); + let event: WebViewPostEvent; + try { + event = JSON.parse(ev.nativeEvent.data); + } catch { + return; + } + __DEV__ && onLogMessage(event); switch (event.type) {Also applies to: 135-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/reader/components/WebViewReader.tsx` around lines 35 - 43, The onLogMessage handler currently does an unconditional JSON.parse of payload.nativeEvent.data which will throw on malformed/plain-string messages; wrap the JSON.parse in a try/catch and bail out gracefully (e.g., return) if parsing fails or the parsed value is not an object, and validate the presence/type of dataPayload.type before accessing it; apply the same defensive parsing/validation to the other WebView message handler referenced (the handler around lines 135-138) so malformed postMessage payloads do not crash the reader and you can optionally log a warning when a message is ignored.
🧹 Nitpick comments (3)
src/components/TextInput/index.tsx (1)
9-12: ⚡ Quick win
value?: nevermakes the component uncontrolled-only and is an undocumented constraint.Any caller that needs to reset or programmatically update the field (e.g., switching between snippets without unmounting) must rely on a
keyprop — this is a non-obvious coupling. Consider removing theneverguard and delegating the choice of controlled vs. uncontrolled to the caller.♻️ Proposed change
interface TextInputProps extends RNTextInputProps { error?: boolean; - value?: never; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TextInput/index.tsx` around lines 9 - 12, The TextInputProps interface currently forces uncontrolled-only usage by declaring value?: never; remove that line so TextInputProps extends RNTextInputProps without overriding value (or explicitly type value to match RNTextInputProps, e.g., value?: string | undefined) to allow callers to choose controlled vs uncontrolled behavior; update any related JSDoc/comments for the TextInput component to reflect that controlled usage is supported and remove any code that relied on value being forbidden.src/hooks/persisted/useSettings.ts (1)
131-136: ⚡ Quick winExport
CodeSnippet— it's part of a public interface but inaccessible to consumers.
CodeSnippetis used in the exportedChapterReaderSettingsinterface, but is itself unexported. Any screen or hook that creates or manipulatescodeSnippetsJS/codeSnippetsCSSentries must either redeclare the type inline or useany, defeating the purpose of the type.♻️ Proposed fix
-type CodeSnippet = { +export type CodeSnippet = { name: string; code: string; lang: 'js' | 'css'; active: boolean; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/persisted/useSettings.ts` around lines 131 - 136, The CodeSnippet type is used in the exported ChapterReaderSettings but is declared unexported; export it so consumers can reference the exact type (export type CodeSnippet = { name: string; code: string; lang: 'js' | 'css'; active: boolean; }). Update the declaration of CodeSnippet to be exported and ensure any references (ChapterReaderSettings, codeSnippetsJS, codeSnippetsCSS) use the exported type rather than redeclaring or falling back to any.src/screens/settings/SettingsCustomCodeScreen/Components/SettingsWebView.tsx (1)
82-114: 🏗️ Heavy liftMMKV listener and
webViewSourcememo fight each other on every settings change.
webViewSourcelistssettings(line 313) — the entireuseChapterReaderSettings()object — as a dependency. Because MMKV mutations causesettingsto produce a new object, every setting change triggers a full WebView HTML rebuild and reload. This races with (and wins over) the MMKV listener's JS injection at lines 86-90, making theCHAPTER_READER_SETTINGSinjection branch a no-op. The visible symptom is a WebView flash/blank-screen on every settings change in the preview.The two mechanisms are redundant and conflicting. Pick one:
- Option A (simpler): Remove
settingsfromwebViewSourcedeps and let the MMKV listener drive runtime updates.- Option B: Remove the MMKV listener's
CHAPTER_READER_SETTINGSbranch entirely and accept that the WebView does a full reload per change (acceptable for a settings preview).Also applies to: 216-316
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsCustomCodeScreen/Components/SettingsWebView.tsx` around lines 82 - 114, The webview is being rebuilt on every settings change because webViewSource depends on the entire useChapterReaderSettings() object, which races with the MMKV listener updates; remove the redundant runtime conflict by deleting the settings dependency from webViewSource (so webViewSource no longer includes the useChapterReaderSettings() object) and rely on the existing MMKV listener (MMKVStorage.addOnValueChangedListener) to inject updates via webViewRef.current?.injectJavaScript for CHAPTER_READER_SETTINGS; keep the CHAPTER_READER_SETTINGS injection branch in the listener and ensure webViewSource still includes any static values it needs but not the full settings object referenced by useChapterReaderSettings().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@android/app/src/main/assets/js/textRemover.js`:
- Line 5: this.hidden is initialized to van.state(true) but never updated, so
external observers always see hidden=true; update this.hidden.val inside the UI
toggles: in showSelectionUI() set this.hidden.val(false) and in
hideSelectionUI() set this.hidden.val(true), and also ensure any code paths that
change isUIActive or the element.style.display likewise synchronize
this.hidden.val to the current visibility so window.textRemover.hidden reflects
real state (update references in showSelectionUI, hideSelectionUI and any other
place that sets isUIActive or toggles the element).
In `@src/hooks/persisted/useNovel/useNovelData.ts`:
- Around line 97-101: The console.error calls inside the try-catch blocks for
fetching chapters in the useNovelData hook violate the no-console lint rule and
cause CI failures. Replace these console.error calls with either silent error
handling or use the project's approved logging utility to properly log errors
without breaking lint. Apply this fix to the error handling around
getPageChaptersBatched at lines corresponding to 97-101, 157-159, and 197-199.
- Around line 115-118: The batch `total` calculation in setBatchInformation
currently uses Math.floor and overcounts when chapterCount is divisible by 300;
update the calculation for the `total` field in the setBatchInformation call
(the batch/total/totalChapters object) to compute the number of 300-chapter
batches correctly by using Math.ceil(chapterCount / 300) (or equivalently
Math.floor((chapterCount + 299) / 300)) so that exact multiples of 300 do not
produce an extra empty batch.
In `@src/screens/reader/components/Hooks/useCustomCode.ts`:
- Around line 14-19: The generated JS embeds user-provided snippet.name directly
into a template literal which allows backticks or ${...} to break the script;
update useCustomCode so snippet.name is safely serialized before embedding (for
example, use JSON.stringify(snippet.name) or an equivalent escaping routine) and
interpolate the serialized value into the returned template string used in the
try/catch alert, ensuring the alert message uses the escaped/serialized name
instead of raw snippet.name.
In `@src/screens/reader/components/Hooks/useTextModifications.ts`:
- Around line 51-67: handleReplaceSave currently ignores saves when a
replacement already exists because it only writes when selectedTextForReplace is
not a key in readerSettings.replaceText; change it to always update the map so
existing rules are overwritten: inside handleReplaceSave (referencing
selectedTextForReplace, readerSettings.replaceText, replacementText and
setChapterReaderSettings) remove the conditional that checks for key existence
and always set newReplaceText[selectedTextForReplace] = replacementText, then
call setChapterReaderSettings({ replaceText: newReplaceText }); optionally
follow by closing the modal and clearing selection
(setReplaceModalVisible(false), setSelectedTextForReplace(''),
setReplacementText('')) to complete the UX.
- Around line 16-34: The persisted slash-delimited regex parsing in
useTextModifications (when iterating readerSettings.removeText and
readerSettings.replaceText and updating chText) can throw if the pattern or
flags are malformed; wrap the RegExp creation/usage in a guard: validate flags
against the allowed set or attempt new RegExp inside a try/catch, and on error
skip that rule (optionally log a warning) so malformed entries don’t throw
during render. Ensure this change covers both the removeText loop and the
replaceText loop where new RegExp(m[1], m[2]) is used.
In `@src/screens/reader/components/Hooks/useTTS.ts`:
- Around line 160-175: Clamp and validate the start index before storing and
before using it in the AppState 'change' resume path: ensure
ttsQueueIndexRef.current (the value derived from startIndex/postMessage) is
clamped to [0, tts.allReadableElements.length - 1] and ignore or set to 0 when
it is -1 or out of range; update the code paths around the AppState subscription
(the block using appStateRef.current, isTTSReadingRef.current and
webViewRef.current?.injectJavaScript) and the other resume block referenced
around lines 220-235 so the injected JS uses a validated index and you never
call classList.add on a null currentElement.
In `@src/screens/reader/components/WebViewReader.tsx`:
- Around line 155-158: The current check blocks 0 because `event.data && typeof
event.data === 'number'` treats 0 as falsy; update the condition in the message
handler inside WebViewReader.tsx to accept zero by using a numeric type check
(e.g., replace the condition with `typeof event.data === 'number'`) so calls to
saveProgress(event.data) correctly persist a 0 position; locate the handler
referencing saveProgress and change the if-statement accordingly.
- Around line 285-290: The injected metadata in function fn (novelName,
chapterName, sourceId assigned from novel.name, chapter.name, novel.pluginId,
etc.) is inserted raw and can break the script or enable code injection; instead
serialize/escape each external value before embedding: replace direct
interpolation with a serialized form (e.g., use JSON.stringify on novel.name,
chapter.name, novel.pluginId and the numeric IDs) and ensure any "</script>"
substring is safely escaped/neutralized so it cannot close the script tag;
update the assignments for novelName, chapterName, sourceId (and keep
chapterId/novelId as numbers) to use the serialized/escaped values.
In `@src/screens/settings/SettingsCustomCodeScreen/Components/CodeInput.tsx`:
- Around line 117-130: The TextInput is using defaultValue={code} which yields
stale text when the same CodeInput instance is reused; update the caller
(CodeRoute) to force remounting by passing a unique key (e.g., snippet
id/name/index) to CodeInput, or alternatively remove the custom TextInput's
value?: never restriction and make AnimatedTextInput a controlled component here
by passing value={code} and onChangeText={setCode}; locate the AnimatedTextInput
usage in CodeInput and update CodeRoute to add a per-snippet key (preferred) or
change the TextInput type to allow value and convert AnimatedTextInput to
controlled mode if you prefer that approach.
In `@src/screens/settings/SettingsCustomCodeScreen/Components/dummies.ts`:
- Around line 1-85: The embedded huge data URI inside the dummyHTML string
causes bundle bloat; remove the inline base64 from the dummyHTML constant and
instead reference a lightweight placeholder (or a small external URL) and load
the heavy data only when needed via lazy/dynamic loading (e.g., provide a
function like getDummyHTML or loadDummyImage that injects the full data URI at
runtime or performs a dynamic import/fetch), keeping the dummyHTML variable free
of large inline payloads.
In
`@src/screens/settings/SettingsCustomCodeScreen/Components/SettingsWebView.tsx`:
- Around line 257-262: initialReaderConfig is being built with
chapterGeneralSettings set to the reader settings variable; replace the
copy-paste bug by passing the general settings value returned by
useChapterGeneralSettings() instead of the reader settings from
useChapterReaderSettings(). Locate the initialReaderConfig construction
(property chapterGeneralSettings) and swap the value from settings to
generalSettings (the identifier returned by useChapterGeneralSettings()), making
sure JSON.stringify serializes the correct generalSettings variable.
- Around line 339-352: The speak branch never runs because WebViewPostEvent.data
is typed as an object but sometimes contains a raw string; update the guard in
the case 'speak' block to accept both a string and an object with a text
property (e.g. if (typeof event.data === 'string' || (event.data && typeof
(event.data as any).text === 'string')) ), extract the text (const text = typeof
event.data === 'string' ? event.data : (event.data as any).text) and pass that
to Speech.speak, keeping the existing Speech.speak options and the
webViewRef.current?.injectJavaScript('tts.next?.()') callbacks; alternatively,
adjust the WebViewPostEvent type to allow string|{[k:string]:string|number} so
the original typeof check can be valid.
- Around line 327-356: The onMessage handler calls JSON.parse on
ev.nativeEvent.data without error handling, which will crash on malformed JSON;
wrap the parse in a try/catch inside the onMessage handler (the function
handling ev in SettingsWebView.tsx that parses into WebViewPostEvent) and on
parse failure log the error (use __DEV__ && onLogMessage or process logger
equivalent) and return early (optionally notify the webview via
webViewRef.current?.injectJavaScript). Ensure subsequent switch cases
(hide/next/prev/save/speak/stop-speak) only run when parsing succeeds and
preserve existing use of Speech.speak and settings.tts values.
In `@src/screens/settings/SettingsCustomCodeScreen/index.tsx`:
- Around line 62-65: handleSnippetSaved sets index directly via setIndex(0),
bypassing handleTabChange and leaving appBarHiddenState at 1; change
handleSnippetSaved to call handleTabChange(0) (after clearing editing state via
setEditingSnippet(null)) so the app bar visibility is restored when returning to
the Settings tab, ensuring handleTabChange is in scope where handleSnippetSaved
is defined.
In `@src/screens/settings/SettingsCustomCodeScreen/Modals/ReplaceItemModal.tsx`:
- Around line 87-101: When saving, short-circuit and reject duplicates before
mutating settings: in replace mode (when showReplace is true) check if
replaceText already has a key equal to text and that key !== editing; if so call
setError(['Item already exists','']) and return false instead of overwriting
replaceText[text]; in remove mode, when editing is truthy check whether
removeText already contains text at an index other than the current editing
index (i.e., exists elsewhere) and if so setError and return false; only after
these validations update replaceText/removeText and call setSettings (also avoid
mutating existing replaceText/removeText in place—use a shallow copy before
modifying).
- Around line 223-248: The TextInput controls in ReplaceItemModal are currently
uncontrolled (using defaultValue) which causes stale values when the modal stays
mounted; change the two TextInput props from defaultValue={text} and
defaultValue={replacementText} to value={text} and value={replacementText} and
keep onChangeText calling setText and setReplacementText so the inputs become
controlled, and update the onDismiss handler (which currently calls
modal.setFalse(); setError(undefined);) to call cancel() (which resets form
state) instead of only clearing the error to ensure the form and refs (textRef,
replaceTextRef) are fully reset when the modal is dismissed.
In `@src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx`:
- Around line 167-171: The Button rendering in CodeRoute.tsx (the Button with
style={styles.button} and title={getString('readerSettings.openJSFile')}) is a
no-op because it lacks an onPress handler; either wire it to the proper
file-open action (add the onPress callback that triggers the existing
openJSFile/openFile handler or navigation action) or make it non-interactive
until implemented by setting disabled={true} or removing the button entirely so
it isn't presented as actionable; also ensure accessibility: use a clear
accessibilityLabel and visual disabled styling if you choose to disable it.
- Around line 45-47: The current edit-mode detection treats any non-null
editingSnippet as editing, which misclassifies index === -1 (create new) as edit
mode; update the check used by isEditing to require editingSnippet !==
null/undefined AND editingSnippet.index !== -1 (e.g., use editingSnippet?.index
!== -1) so editIndex and editIsJS derive correctly (editIndex should still use
editingSnippet.index when truly editing, otherwise use snippetIndex; editIsJS
should fall back to dLang when creating). Apply the same corrected gating
wherever the component later repeats this logic (the block around lines 140-153)
so language toggles and JS fallback behave correctly for new snippets.
- Around line 98-119: The current logic mutates the existing snippets array
in-place (touching snippets, snippets.push, and modifying snippets[editIndex])
before calling setSettings, which breaks memoization; instead create a new array
copy and update that immutably: for the edit branch produce a shallow copy of
snippets, replace the edited index with a new object, and call setSettings with
that new array; for the create branch build a new array [...snippets,
newSnippet] and pass that to setSettings; update references to codeSnippetsJS /
codeSnippetsCSS and keep symbols isEditing, editIndex, snippets, setSettings
unchanged so callers remain consistent.
---
Outside diff comments:
In `@src/components/Common/ToggleButton.tsx`:
- Around line 34-55: ToggleButton's disabled prop is set on Pressable but
there's no visual or accessibility indication; update the ToggleButton component
to apply a disabled visual treatment and accessibility state: when disabled is
true, pass accessibilityState={{ disabled: true }} to Pressable, add a disabled
style (e.g. lower opacity or use theme.disabled/onSurfaceVariant) by extending
styles.toggleButtonPressable or via getToggleButtonPressableStyle(selected,
theme, disabled), and change the MaterialCommunityIcons color to use the
disabled color when disabled instead of theme.primary/theme.onSurface so users
can both see and programmatically detect the control is unavailable.
In `@src/screens/reader/components/WebViewReader.tsx`:
- Around line 35-43: The onLogMessage handler currently does an unconditional
JSON.parse of payload.nativeEvent.data which will throw on
malformed/plain-string messages; wrap the JSON.parse in a try/catch and bail out
gracefully (e.g., return) if parsing fails or the parsed value is not an object,
and validate the presence/type of dataPayload.type before accessing it; apply
the same defensive parsing/validation to the other WebView message handler
referenced (the handler around lines 135-138) so malformed postMessage payloads
do not crash the reader and you can optionally log a warning when a message is
ignored.
---
Nitpick comments:
In `@src/components/TextInput/index.tsx`:
- Around line 9-12: The TextInputProps interface currently forces
uncontrolled-only usage by declaring value?: never; remove that line so
TextInputProps extends RNTextInputProps without overriding value (or explicitly
type value to match RNTextInputProps, e.g., value?: string | undefined) to allow
callers to choose controlled vs uncontrolled behavior; update any related
JSDoc/comments for the TextInput component to reflect that controlled usage is
supported and remove any code that relied on value being forbidden.
In `@src/hooks/persisted/useSettings.ts`:
- Around line 131-136: The CodeSnippet type is used in the exported
ChapterReaderSettings but is declared unexported; export it so consumers can
reference the exact type (export type CodeSnippet = { name: string; code:
string; lang: 'js' | 'css'; active: boolean; }). Update the declaration of
CodeSnippet to be exported and ensure any references (ChapterReaderSettings,
codeSnippetsJS, codeSnippetsCSS) use the exported type rather than redeclaring
or falling back to any.
In
`@src/screens/settings/SettingsCustomCodeScreen/Components/SettingsWebView.tsx`:
- Around line 82-114: The webview is being rebuilt on every settings change
because webViewSource depends on the entire useChapterReaderSettings() object,
which races with the MMKV listener updates; remove the redundant runtime
conflict by deleting the settings dependency from webViewSource (so
webViewSource no longer includes the useChapterReaderSettings() object) and rely
on the existing MMKV listener (MMKVStorage.addOnValueChangedListener) to inject
updates via webViewRef.current?.injectJavaScript for CHAPTER_READER_SETTINGS;
keep the CHAPTER_READER_SETTINGS injection branch in the listener and ensure
webViewSource still includes any static values it needs but not the full
settings object referenced by useChapterReaderSettings().
🪄 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: 54e9038f-bd4a-4621-9c21-2a2446486770
📒 Files selected for processing (35)
android/app/src/main/assets/js/textRemover.jssrc/components/Common.tsxsrc/components/Common/ToggleButton.tsxsrc/components/IconButtonV2/AnimatedIconButton.tsxsrc/components/Modal/KeyboardAvoidingModal.tsxsrc/components/TextInput/index.tsxsrc/components/index.tssrc/hooks/common/useKeyboardHeight.tssrc/hooks/persisted/useNovel/useNovelData.tssrc/hooks/persisted/usePlugins.tssrc/hooks/persisted/useSettings.tssrc/navigators/MoreStack.tsxsrc/navigators/types/index.tssrc/plugins/dev/DevNovelPlugin.tssrc/plugins/dev/DevPagedNovelPlugin.tssrc/plugins/dev/index.tssrc/plugins/pluginManager.tssrc/screens/browse/components/InstalledTab.tsxsrc/screens/novel/components/NovelBottomSheet.tsxsrc/screens/reader/components/Hooks/useCustomCode.tssrc/screens/reader/components/Hooks/useTTS.tssrc/screens/reader/components/Hooks/useTextModifications.tssrc/screens/reader/components/WebViewReader.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/CodeInput.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/ListItems.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/SelfHidingAppbar.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/SettingsWebView.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/dummies.tssrc/screens/settings/SettingsCustomCodeScreen/Modals/ReplaceItemModal.tsxsrc/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsxsrc/screens/settings/SettingsCustomCodeScreen/Routes/SettingsRoute.tsxsrc/screens/settings/SettingsCustomCodeScreen/index.tsxsrc/screens/settings/SettingsScreen.tsxsrc/services/plugin/fetch.tssrc/services/updates/LibraryUpdateQueries.ts
💤 Files with no reviewable changes (2)
- src/screens/browse/components/InstalledTab.tsx
- src/services/updates/LibraryUpdateQueries.ts
CD-Z
force-pushed
the
custom_code_settings_page_clean
branch
from
d7af67b to
409ce50
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Common/ToggleButton.tsx (1)
32-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a visual affordance for the disabled state.
The
disabledprop correctly blocks press interactions, but there's no style change whendisabledistrue— the icon renders identically to its enabled state, which is confusing to users.Consider applying a reduced opacity or dimming the icon color:
🎨 Proposed fix
<View style={styles.toggleButtonContainer}> <Pressable android_ripple={{ color: theme.rippleColor }} style={[ styles.toggleButtonPressable, getToggleButtonPressableStyle(selected, theme), + disabled ? styles.disabled : undefined, ]} onPress={onPress} disabled={disabled} >toggleColorButtonPressable: { ... }, + disabled: { + opacity: 0.38, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Common/ToggleButton.tsx` around lines 32 - 51, The ToggleButton component currently ignores the disabled visual state; update the Pressable/style logic in ToggleButton to include a disabled visual affordance (e.g., reduced opacity or dimmed icon color) when the disabled prop is true by adding a disabled-specific style to the style array (styles.toggleButtonPressable + getToggleButtonPressableStyle(selected, theme)) or by passing a computed color/opacity into the rendered icon; change the code paths around the Pressable and icon rendering in ToggleButton so that when disabled is true you apply a lower opacity or a muted color from the theme (and add a corresponding styles.toggleButtonDisabled entry if needed) while preserving the existing onPress disabling behavior.
♻️ Duplicate comments (1)
src/screens/settings/SettingsCustomCodeScreen/Components/CodeInput.tsx (1)
106-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
defaultValuecan still desync displayed code from selected snippet.At Line 108, uncontrolled input state can go stale when
codechanges without remount. If this was intentionally addressed in the caller, please ensureCodeInputis keyed per snippet instance.#!/bin/bash # Verify CodeInput is remounted per snippet in CodeRoute usage. FILE=$(fd -i '^CodeRoute\.tsx$' src | head -n 1) if [ -z "$FILE" ]; then echo "CodeRoute.tsx not found under src/" exit 1 fi echo "Inspecting: $FILE" rg -n -C4 '<CodeInput|key=' "$FILE"Expected: each
<CodeInput ... />inCodeRoute.tsxhas a stable per-snippetkey(e.g., id/name/index).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsCustomCodeScreen/Components/CodeInput.tsx` around lines 106 - 109, The AnimatedTextInput in CodeInput uses defaultValue which can desync when the prop code changes; convert it to a controlled input by replacing defaultValue={code} with value={code} (keeping onChangeText={setCode}) inside the CodeInput component, or if you intentionally rely on remount semantics instead, ensure every <CodeInput ... /> usage in CodeRoute supplies a stable per-snippet key (e.g., snippet id/name/index) so the component remounts when switching snippets.
🧹 Nitpick comments (3)
android/app/src/main/assets/js/textRemover.js (1)
149-156: ⚡ Quick win
selectionchangefires continuously during drag — repeated layout thrashing on every event.
showSelectionUI()callsgetComputedStyle(...),getBoundingClientRect(), and writes toui.style.*on everyselectionchangeevent, including during live drag-selection where events fire at high frequency. This causes forced synchronous layout (read-write-read cycles) on the render thread for each event.Consider debouncing the repositioning logic, or at minimum separating the one-time DOM creation from the per-event layout pass:
+ let repositionTimer = null; + document.addEventListener('selectionchange', function () { const selectedText = getSelectedText(); if (selectedText) { - showSelectionUI(); + clearTimeout(repositionTimer); + repositionTimer = setTimeout(showSelectionUI, 50); } else if (!isUIActive) { hideSelectionUI(); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/assets/js/textRemover.js` around lines 149 - 156, The selectionchange handler currently calls showSelectionUI() on every event (via document.addEventListener('selectionchange')) which triggers repeated getComputedStyle/getBoundingClientRect and style writes and causes layout thrashing; change the logic so getSelectedText() still triggers visibility checks but split creation vs layout: ensure showSelectionUI() only performs one-time DOM creation when UI is not present (use isUIActive to gate creation) and move expensive measurements/position updates into a debounced or rAF-throttled function (e.g., a new repositionSelectionUI() invoked via requestAnimationFrame or a short debounce) so high-frequency selectionchange events only schedule a single layout pass rather than performing synchronous read-write-read on every event; keep hideSelectionUI() behavior unchanged.src/screens/settings/SettingsCustomCodeScreen/index.tsx (2)
80-106: 💤 Low valueMemoize
renderSceneto matchrenderTabBarand avoid scene re-mounts.
renderSceneis recreated on every render, whilerenderTabBaris wrapped inuseCallback. Passing a fresh function toTabViewon each render can defeat its internal memoization and cause avoidable scene work. Wrapping it inuseCallback(witheditingSnippetandindexin deps) keeps it consistent withrenderTabBar.Proposed refactor
- const renderScene = ({ - route, - jumpTo, - }: SceneRendererProps & { - route: { - key: string; - title: string; - }; - }) => { - switch (route.key) { - case 'first': - return <SettingsRoute onEditSnippet={handleEditSnippet} />; - case 'second': - return ( - <CodeRoute - jumpTo={jumpTo} - editingSnippet={editingSnippet} - onSnippetSaved={handleSnippetSaved} - isActive={index === 1} - /> - ); - case 'third': - return <SettingsWebView />; - default: - return null; - } - }; + const renderScene = React.useCallback( + ({ + route, + jumpTo, + }: SceneRendererProps & { route: { key: string; title: string } }) => { + switch (route.key) { + case 'first': + return <SettingsRoute onEditSnippet={handleEditSnippet} />; + case 'second': + return ( + <CodeRoute + jumpTo={jumpTo} + editingSnippet={editingSnippet} + onSnippetSaved={handleSnippetSaved} + isActive={index === 1} + /> + ); + case 'third': + return <SettingsWebView />; + default: + return null; + } + }, + [editingSnippet, index], + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsCustomCodeScreen/index.tsx` around lines 80 - 106, renderScene is recreated on every render which breaks TabView's memoization; wrap renderScene in useCallback and include editingSnippet and index in its dependency array to mirror renderTabBar's behavior. Locate the renderScene function and convert it to a const wrapped with useCallback, preserving the switch cases that return SettingsRoute, CodeRoute (keep jumpTo, editingSnippet, onSnippetSaved, isActive props and isActive computed from index), and SettingsWebView, and ensure handleEditSnippet remains referenced; this keeps TabView stable and prevents unnecessary scene remounts.
152-160: 💤 Low valueRemove unsupported
collapsableprop fromTabView.The
collapsableprop is not part of the react-native-tab-view 4.3.0 API and will be ignored. If optimization of the wrapping container is needed, applycollapsableto aViewwrapper instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsCustomCodeScreen/index.tsx` around lines 152 - 160, The TabView usage passes an unsupported prop `collapsable`; remove `collapsable={false}` from the TabView component in SettingsCustomCodeScreen so the component only receives valid props (index/routes/navigationState, renderScene, renderTabBar, onIndexChange, initialLayout, lazy). If you need the optimization, wrap TabView with a React Native View and apply `collapsable={false}` to that wrapper instead; locate the TabView JSX in the SettingsCustomCodeScreen component (where renderScene and handleTabChange are used) to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@android/app/src/main/assets/js/textRemover.js`:
- Around line 115-121: The code assumes window.getSelection() is non-null;
update getSelectedText, removeSelectedText, and replaceSelectedText to guard
against a null selection: first call const selection = window.getSelection(); if
(!selection) return '' (for getSelectedText) or return/do nothing (for
removeSelectedText/replaceSelectedText), and only then access
selection.rangeCount, selection.removeAllRanges(), or selection.getRangeAt(...).
Ensure every call site uses this null-check so no property is accessed on a null
selection.
- Around line 74-76: The code in showSelectionUI reads reader.hidden.val and
reader.generalSettings.val.verticalSeekbar directly, which can throw a
ReferenceError if reader isn't defined; update showSelectionUI to guard access
to reader (e.g., check typeof reader !== 'undefined' or use optional chaining
like reader?.hidden?.val and reader?.generalSettings?.val?.verticalSeekbar) and
provide safe defaults for avoidUI and avoidScrollbar when reader is absent so
the overlay still positions without throwing; ensure the logic references the
same symbols (avoidUI, avoidScrollbar, reader.hidden.val,
reader.generalSettings.val.verticalSeekbar) so behavior is preserved when reader
exists.
In `@src/screens/settings/SettingsCustomCodeScreen/index.tsx`:
- Around line 20-24: Replace hardcoded tab titles in the routes array and the
app bar title with localized strings via getString: change the routes entries'
title values from 'Settings' and 'Example' to getString('settings') and
getString('example') and change the 'Code' and app bar 'Custom Code' usages to
getString('code') and getString('custom_code') (or whatever canonical keys you
choose); update or add those keys in strings.json if they don't exist; locate
the titles by looking for the routes constant and the component rendering the
app bar title (search for the literal "Custom Code") and replace with the
getString(...) calls.
- Around line 57-64: handleEditSnippet currently forces new snippets to JS by
overriding isJS when snippetIndex === -1; remove that conditional override in
SettingsCustomCodeScreen's handleEditSnippet so the passed isJS is honored (keep
setting editingSnippet.index = snippetIndex and editingSnippet.isJS = isJS) and
then update the caller in SettingsRoute (where onEditSnippet is invoked with -1)
to provide the desired language for new snippets (e.g., add separate "Add JS"
and "Add CSS" actions or a language chooser that calls onEditSnippet(-1, true)
or onEditSnippet(-1, false)); ensure you update any UI that created the
hardcoded onPress={() => onEditSnippet?.(-1, true)} to pass the correct boolean.
---
Outside diff comments:
In `@src/components/Common/ToggleButton.tsx`:
- Around line 32-51: The ToggleButton component currently ignores the disabled
visual state; update the Pressable/style logic in ToggleButton to include a
disabled visual affordance (e.g., reduced opacity or dimmed icon color) when the
disabled prop is true by adding a disabled-specific style to the style array
(styles.toggleButtonPressable + getToggleButtonPressableStyle(selected, theme))
or by passing a computed color/opacity into the rendered icon; change the code
paths around the Pressable and icon rendering in ToggleButton so that when
disabled is true you apply a lower opacity or a muted color from the theme (and
add a corresponding styles.toggleButtonDisabled entry if needed) while
preserving the existing onPress disabling behavior.
---
Duplicate comments:
In `@src/screens/settings/SettingsCustomCodeScreen/Components/CodeInput.tsx`:
- Around line 106-109: The AnimatedTextInput in CodeInput uses defaultValue
which can desync when the prop code changes; convert it to a controlled input by
replacing defaultValue={code} with value={code} (keeping onChangeText={setCode})
inside the CodeInput component, or if you intentionally rely on remount
semantics instead, ensure every <CodeInput ... /> usage in CodeRoute supplies a
stable per-snippet key (e.g., snippet id/name/index) so the component remounts
when switching snippets.
---
Nitpick comments:
In `@android/app/src/main/assets/js/textRemover.js`:
- Around line 149-156: The selectionchange handler currently calls
showSelectionUI() on every event (via
document.addEventListener('selectionchange')) which triggers repeated
getComputedStyle/getBoundingClientRect and style writes and causes layout
thrashing; change the logic so getSelectedText() still triggers visibility
checks but split creation vs layout: ensure showSelectionUI() only performs
one-time DOM creation when UI is not present (use isUIActive to gate creation)
and move expensive measurements/position updates into a debounced or
rAF-throttled function (e.g., a new repositionSelectionUI() invoked via
requestAnimationFrame or a short debounce) so high-frequency selectionchange
events only schedule a single layout pass rather than performing synchronous
read-write-read on every event; keep hideSelectionUI() behavior unchanged.
In `@src/screens/settings/SettingsCustomCodeScreen/index.tsx`:
- Around line 80-106: renderScene is recreated on every render which breaks
TabView's memoization; wrap renderScene in useCallback and include
editingSnippet and index in its dependency array to mirror renderTabBar's
behavior. Locate the renderScene function and convert it to a const wrapped with
useCallback, preserving the switch cases that return SettingsRoute, CodeRoute
(keep jumpTo, editingSnippet, onSnippetSaved, isActive props and isActive
computed from index), and SettingsWebView, and ensure handleEditSnippet remains
referenced; this keeps TabView stable and prevents unnecessary scene remounts.
- Around line 152-160: The TabView usage passes an unsupported prop
`collapsable`; remove `collapsable={false}` from the TabView component in
SettingsCustomCodeScreen so the component only receives valid props
(index/routes/navigationState, renderScene, renderTabBar, onIndexChange,
initialLayout, lazy). If you need the optimization, wrap TabView with a React
Native View and apply `collapsable={false}` to that wrapper instead; locate the
TabView JSX in the SettingsCustomCodeScreen component (where renderScene and
handleTabChange are used) to make the change.
🪄 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: 9f4c7a2a-9309-4d5b-8a3e-ab17b2a62d58
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
App.tsxandroid/app/src/main/assets/js/textRemover.jspackage.jsonsrc/components/Common.tsxsrc/components/Common/ToggleButton.tsxsrc/components/IconButtonV2/AnimatedIconButton.tsxsrc/components/Modal/KeyboardAvoidingModal.tsxsrc/components/TextInput/index.tsxsrc/components/index.tssrc/hooks/common/useKeyboardHeight.tssrc/hooks/persisted/useSettings.tssrc/navigators/MoreStack.tsxsrc/navigators/types/index.tssrc/screens/reader/components/Hooks/useCustomCode.tssrc/screens/reader/components/Hooks/useTTS.tssrc/screens/reader/components/Hooks/useTextModifications.tssrc/screens/reader/components/WebViewReader.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/CodeInput.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/ListItems.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/SelfHidingAppbar.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/SettingsWebView.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/dummies.tssrc/screens/settings/SettingsCustomCodeScreen/Modals/ReplaceItemModal.tsxsrc/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsxsrc/screens/settings/SettingsCustomCodeScreen/Routes/SettingsRoute.tsxsrc/screens/settings/SettingsCustomCodeScreen/index.tsxsrc/screens/settings/SettingsReaderScreen/SettingsReaderScreen.tsxsrc/screens/settings/SettingsReaderScreen/tabs/AdvancedTab.tsxsrc/screens/settings/SettingsScreen.tsx
💤 Files with no reviewable changes (1)
- src/screens/settings/SettingsReaderScreen/tabs/AdvancedTab.tsx
✅ Files skipped from review due to trivial changes (3)
- App.tsx
- src/navigators/types/index.ts
- src/navigators/MoreStack.tsx
🚧 Files skipped from review as they are similar to previous changes (19)
- src/components/TextInput/index.tsx
- src/screens/reader/components/Hooks/useCustomCode.ts
- src/hooks/common/useKeyboardHeight.ts
- src/screens/settings/SettingsCustomCodeScreen/Components/ListItems.tsx
- src/screens/settings/SettingsScreen.tsx
- src/screens/reader/components/Hooks/useTextModifications.ts
- src/components/index.ts
- src/screens/settings/SettingsCustomCodeScreen/Components/dummies.ts
- src/screens/settings/SettingsCustomCodeScreen/Components/SettingsWebView.tsx
- src/components/IconButtonV2/AnimatedIconButton.tsx
- src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx
- src/screens/settings/SettingsCustomCodeScreen/Components/SelfHidingAppbar.tsx
- src/hooks/persisted/useSettings.ts
- src/components/Modal/KeyboardAvoidingModal.tsx
- src/components/Common.tsx
- src/screens/reader/components/WebViewReader.tsx
- src/screens/settings/SettingsCustomCodeScreen/Routes/SettingsRoute.tsx
- src/screens/settings/SettingsCustomCodeScreen/Modals/ReplaceItemModal.tsx
- src/screens/reader/components/Hooks/useTTS.ts
There was a problem hiding this comment.
Actionable comments posted: 9
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/components/ExportNovelAsEpubButton.tsx (1)
123-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize
novel.name/pluginId/idbefore interpolating into the EPUB JS.
novel.nameis interpolated directly into a quoted JS string at Line 124, so a"or</script>in the title corrupts the EPUB JS.pluginIdandidgo in unquoted (Lines 126, 128), so any non-numeric pluginId becomes invalid syntax. UseJSON.stringifyfor safe embedding (the same fix recommended forWebViewReader.tsx).🛡️ Proposed fix
return ` - let novelName = "${novel.name}"; + let novelName = ${JSON.stringify(novel.name)}; let chapterName = ""; - let sourceId = ${novel.pluginId}; + let sourceId = ${JSON.stringify(novel.pluginId)}; let chapterId = ""; - let novelId = ${novel.id}; + let novelId = ${JSON.stringify(novel.id)}; let html = document.querySelector("chapter").innerHTML;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/novel/components/ExportNovelAsEpubButton.tsx` around lines 123 - 132, The template JS returned in ExportNovelAsEpubButton.tsx injects novel.name, novel.pluginId and novel.id directly into the string causing syntax injection/invalid JS when values contain quotes or non-numeric IDs; change the interpolation to embed safe JSON-serialized values (use JSON.stringify on novel.name, novel.pluginId and novel.id) before inserting into the template so novelName, sourceId and novelId are always valid JS literals (mirror the fix used in WebViewReader.tsx).
♻️ Duplicate comments (2)
src/screens/reader/components/WebViewReader.tsx (2)
290-302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize injected metadata before embedding it in the script.
Reiterating the prior finding:
novel.name,chapter.name, andnovel.pluginIdare interpolated raw into a quoted JS string. A", backslash, or</script>in any of these external values can break parsing or escape the script context inside the WebView. UseJSON.stringifyfor each.🛡️ Proposed fix
- function fn(){ - let novelName = "${novel.name}"; - let chapterName = "${chapter.name}"; - let sourceId = "${novel.pluginId}"; - let chapterId =${chapter.id}; - let novelId =${chapter.novelId}; + function fn(){ + let novelName = ${JSON.stringify(novel.name)}; + let chapterName = ${JSON.stringify(chapter.name)}; + let sourceId = ${JSON.stringify(novel.pluginId)}; + let chapterId = ${JSON.stringify(chapter.id)}; + let novelId = ${JSON.stringify(chapter.novelId)}; const qs = (s) => document.querySelector(s); let html = qs("#LNReader-chapter").innerHTML; ${customJS}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/reader/components/WebViewReader.tsx` around lines 290 - 302, The injected metadata (novel.name, chapter.name, novel.pluginId, etc.) are interpolated raw into the inline script (inside function fn) and can break the WebView/script if they contain quotes, backslashes, or "</script>"; replace the direct string interpolations by serializing each value with JSON.stringify when building novelName, chapterName, sourceId (and optionally chapterId/novelId) so the generated JS receives safe, quoted JS literals; ensure the stringified values are used where fn, novelName, chapterName, sourceId, chapterId, novelId and customJS are referenced and keep the DOMContentLoaded listener intact.
159-163:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAccept
0as a valid save position.Same issue flagged previously:
event.data && typeof event.data === 'number'skips0, so a "top of chapter" progress save is silently dropped. Drop the truthiness check and rely on thetypeofguard.🐛 Proposed fix
case 'save': - if (event.data && typeof event.data === 'number') { + if (typeof event.data === 'number') { saveProgress(event.data); } break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/reader/components/WebViewReader.tsx` around lines 159 - 163, In the 'case "save"' branch inside WebViewReader.tsx, the current guard `event.data && typeof event.data === 'number'` incorrectly rejects 0; change the check to only verify the type (e.g., `typeof event.data === 'number'`) before calling saveProgress(event.data) so zero is accepted as a valid position; locate the switch branch handling 'save' and update the conditional there (keeping saveProgress as the call target).
🧹 Nitpick comments (4)
src/screens/settings/SettingsReaderScreen/SettingsReaderScreen.tsx (1)
14-55: 💤 Low valueRedundant
Speech.stopcleanup withSettingsReaderWebView.
SettingsReaderWebViewalready stops speech in its own unmount effect. Since that component is unmounted when this screen unmounts, the screen'sSpeech.stop()cleanup (and theexpo-speechimport) is redundant and can be removed to keep the responsibility in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsReaderScreen/SettingsReaderScreen.tsx` around lines 14 - 55, Remove the redundant speech cleanup from the screen: delete the "import * as Speech from 'expo-speech';" statement and remove the useEffect teardown that calls Speech.stop() in SettingsReaderScreen; leave speech control to SettingsReaderWebView (which already stops speech on unmount) and ensure no other references to Speech remain in SettingsReaderScreen (remove or update any unused imports/usages accordingly).src/hooks/persisted/useSettings.ts (2)
374-379: ⚡ Quick winCast hides residual
customJS/customCSSinstead of stripping them.
chapterReaderSettingsis built fromstoredSettings(aMigrationChapterReaderSettings), so the legacycustomJS/customCSSfields can still be present on the returned object until MMKV rehydrates. Theas ChapterReaderSettingscast just suppresses the type, but consumers spreading these settings (e.g., back intosetSettings) will round-trip the legacy fields. Strip them explicitly when constructing the return value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/persisted/useSettings.ts` around lines 374 - 379, The returned object currently casts chapterReaderSettings to ChapterReaderSettings which hides legacy fields (customJS/customCSS) coming from storedSettings/MigrationChapterReaderSettings; instead explicitly remove those keys when building the returned object so they cannot be propagated (e.g., create the return object by copying chapterReaderSettings but deleting customJS and customCSS before spreading, then include setChapterReaderSettings, saveCustomReaderTheme, deleteCustomReaderTheme) to ensure consumers (like setSettings) won't round-trip legacy fields.
129-134: ⚡ Quick winConsider exporting
CodeSnippet.This type is consumed by
useCustomCode,SettingsCustomCodeScreen,ExportNovelAsEpubButton,SettingsReaderWebView, etc. Keeping it unexported forces those modules to redeclare orany-type the structure.♻️ Proposed change
-type CodeSnippet = { +export type CodeSnippet = { name: string; code: string; lang: 'js' | 'css'; active: boolean; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/persisted/useSettings.ts` around lines 129 - 134, The CodeSnippet type is currently unexported which forces other modules (useCustomCode, SettingsCustomCodeScreen, ExportNovelAsEpubButton, SettingsReaderWebView) to re-declare or use any; export the type by adding an export to the declaration (export type CodeSnippet = {...}) so these consumers can import the shared type; update any import sites to import { CodeSnippet } where needed.src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx (1)
26-58: ⚡ Quick winHoist hardcoded
novel/chapterconstants out of the component body.These objects are recreated on every render and used as the
JSON.stringifypayload forinitialReaderConfig, which forces theWebViewsource.htmlstring to differ on every render and may cause the WebView to reload more than necessary. Move them to module scope so the references are stable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx` around lines 26 - 58, The hardcoded novel and chapter objects are defined inside the SettingsReaderWebView component causing new references each render and making the JSON.stringify payload for initialReaderConfig (used in the WebView source.html) change every time; move the novel and chapter constants to module scope (top-level) so they are stable across renders and update any references in SettingsReaderWebView/initialReaderConfig to use those top-level novel and chapter identifiers, ensuring the WebView source.html string remains stable between renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/persisted/useSettings.ts`:
- Around line 304-309: The guard that checks storedSettings arrays mistakenly
checks codeSnippetsCSS twice so codeSnippetsJS is never validated; update the
condition in useSettings (where storedSettings and setSettings are used) to
validate both Array.isArray(storedSettings.codeSnippetsCSS) and
Array.isArray(storedSettings.codeSnippetsJS) and, when either is invalid, call
setSettings({...storedSettings, codeSnippetsCSS: [], codeSnippetsJS: []}) so
subsequent code (e.g., the migration that calls
storedSettings.codeSnippetsJS.push) will not throw.
- Around line 298-337: The migration and state writes in
useChapterReaderSettings are being run during render and mutate storedSettings;
move all migration/persist logic into a useEffect that runs once (or when
storedSettings changes) and only call setSettings from that effect. Inside the
render return derive a pure normalized object (do not mutate storedSettings)
that ensures codeSnippetsJS and codeSnippetsCSS are arrays and that maps
customJS/customCSS into new snippet objects for read-only use; in the effect
compare that normalized value to storedSettings and call setSettings with a new
object (no in-place push) to persist the migration exactly once, referencing
useChapterReaderSettings, storedSettings, setSettings, codeSnippetsJS/CSS and
customJS/customCSS to locate the code to change.
In `@src/screens/novel/components/ExportNovelAsEpubButton.tsx`:
- Around line 104-116: customJS is being recomputed every render and injects raw
snippet.name into a single-quoted JS string, which breaks the generated EPUB JS
and defeats the epubJavaScript memo; wrap the customJS construction in a useMemo
that depends on readerSettings.codeSnippetsJS (or a stable derived key) and
escape snippet.name when interpolating into the JS (e.g., serialize it safely
with JSON.stringify or an equivalent escape) and change the message to "Error
executing {name}:"; update references to customJS so epubJavaScript uses the
memoized value.
In `@src/screens/reader/components/WebViewReader.tsx`:
- Around line 312-323: The modal in WebViewReader.tsx uses hardcoded strings
("Replace Text", "Text to replace", "Replace with"); replace these with
localized calls using getString(...) (e.g., getString('replaceText.title'),
getString('replaceText.textToReplace'), getString('replaceText.replaceWith')) in
the Dialog title prop and the two TextInput label props (which reference
selectedTextForReplace and the replacement input). Add corresponding keys to the
app's translation resource files so the labels are translated like the rest of
the UI.
In `@src/screens/reader/ReaderScreen.tsx`:
- Around line 74-77: maxBottom is misnamed because the ref records the last
non-zero bottom inset rather than the highest observed value; either rename
maxBottom to lastBottomInset (ref = useRef(bottom)) and keep the existing update
logic, or if you intended to store the maximum observed inset change the
conditional to only update when bottom > maxBottom.current (i.e., compare with
>) so the ref truly holds the maximum; update all references to maxBottom
accordingly (useRef, conditional update, and any usages) to keep names/behavior
consistent in ReaderScreen.tsx.
In
`@src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx`:
- Around line 140-151: preparedDummyHTML currently uses String.prototype.replace
which only replaces the first match, so readerSettings.removeText and
readerSettings.replaceText will not affect all occurrences; update the logic
inside the useMemo that builds resultHtml (references: preparedDummyHTML,
dummyHTML, readerSettings.removeText, readerSettings.replaceText) to use
String.prototype.replaceAll for each text entry or construct a global RegExp
(escaping the text) and call resultHtml.replace(regex, replacement) so every
occurrence is removed/replaced at runtime.
- Around line 94-130: The CSS string webViewCSS emits --StatusBar-currentHeight
without units; update the template where --StatusBar-currentHeight is defined
(using StatusBar.currentHeight) to append "px" so the variable becomes a valid
CSS length (e.g., --StatusBar-currentHeight: ${StatusBar.currentHeight}px;),
ensuring any consumers like padding-top: var(--StatusBar-currentHeight) are
applied correctly; verify the change in SettingsReaderWebView.tsx around the
webViewCSS definition and keep other readerSettings vars unchanged.
- Around line 167-173: The hide-toggle is injecting the current hidden value and
then flipping state, causing the WebView to receive a one-tick-old value; change
the handler to compute the next state first (e.g. nextHidden = !hidden) and
inject that via webViewRef.current?.injectJavaScript(`reader.hidden.val =
${nextHidden}`) then call setHidden(nextHidden) (references: hidden state
variable, webViewRef, injectJavaScript, and setHidden).
- Around line 64-88: The generated customJS embeds snippet.name directly into a
single-quoted JS string inside the try/catch which breaks if the name contains
quotes, backslashes, or newlines; update the template in the customJS useMemo
(the snippet mapping that builds the try/catch and alert) to embed the name
using a safe serializer (e.g., JSON.stringify(snippet.name)) so it’s properly
escaped, and factor this snippet-wrapping logic into a shared exported helper
(used by SettingsReaderWebView's customJS, ExportNovelAsEpubButton, and
useCustomCode) to avoid duplication.
---
Outside diff comments:
In `@src/screens/novel/components/ExportNovelAsEpubButton.tsx`:
- Around line 123-132: The template JS returned in ExportNovelAsEpubButton.tsx
injects novel.name, novel.pluginId and novel.id directly into the string causing
syntax injection/invalid JS when values contain quotes or non-numeric IDs;
change the interpolation to embed safe JSON-serialized values (use
JSON.stringify on novel.name, novel.pluginId and novel.id) before inserting into
the template so novelName, sourceId and novelId are always valid JS literals
(mirror the fix used in WebViewReader.tsx).
---
Duplicate comments:
In `@src/screens/reader/components/WebViewReader.tsx`:
- Around line 290-302: The injected metadata (novel.name, chapter.name,
novel.pluginId, etc.) are interpolated raw into the inline script (inside
function fn) and can break the WebView/script if they contain quotes,
backslashes, or "</script>"; replace the direct string interpolations by
serializing each value with JSON.stringify when building novelName, chapterName,
sourceId (and optionally chapterId/novelId) so the generated JS receives safe,
quoted JS literals; ensure the stringified values are used where fn, novelName,
chapterName, sourceId, chapterId, novelId and customJS are referenced and keep
the DOMContentLoaded listener intact.
- Around line 159-163: In the 'case "save"' branch inside WebViewReader.tsx, the
current guard `event.data && typeof event.data === 'number'` incorrectly rejects
0; change the check to only verify the type (e.g., `typeof event.data ===
'number'`) before calling saveProgress(event.data) so zero is accepted as a
valid position; locate the switch branch handling 'save' and update the
conditional there (keeping saveProgress as the call target).
---
Nitpick comments:
In `@src/hooks/persisted/useSettings.ts`:
- Around line 374-379: The returned object currently casts chapterReaderSettings
to ChapterReaderSettings which hides legacy fields (customJS/customCSS) coming
from storedSettings/MigrationChapterReaderSettings; instead explicitly remove
those keys when building the returned object so they cannot be propagated (e.g.,
create the return object by copying chapterReaderSettings but deleting customJS
and customCSS before spreading, then include setChapterReaderSettings,
saveCustomReaderTheme, deleteCustomReaderTheme) to ensure consumers (like
setSettings) won't round-trip legacy fields.
- Around line 129-134: The CodeSnippet type is currently unexported which forces
other modules (useCustomCode, SettingsCustomCodeScreen, ExportNovelAsEpubButton,
SettingsReaderWebView) to re-declare or use any; export the type by adding an
export to the declaration (export type CodeSnippet = {...}) so these consumers
can import the shared type; update any import sites to import { CodeSnippet }
where needed.
In
`@src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx`:
- Around line 26-58: The hardcoded novel and chapter objects are defined inside
the SettingsReaderWebView component causing new references each render and
making the JSON.stringify payload for initialReaderConfig (used in the WebView
source.html) change every time; move the novel and chapter constants to module
scope (top-level) so they are stable across renders and update any references in
SettingsReaderWebView/initialReaderConfig to use those top-level novel and
chapter identifiers, ensuring the WebView source.html string remains stable
between renders.
In `@src/screens/settings/SettingsReaderScreen/SettingsReaderScreen.tsx`:
- Around line 14-55: Remove the redundant speech cleanup from the screen: delete
the "import * as Speech from 'expo-speech';" statement and remove the useEffect
teardown that calls Speech.stop() in SettingsReaderScreen; leave speech control
to SettingsReaderWebView (which already stops speech on unmount) and ensure no
other references to Speech remain in SettingsReaderScreen (remove or update any
unused imports/usages accordingly).
🪄 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: 62bc8e6c-3626-47a6-82d1-f3872e690ca3
📒 Files selected for processing (11)
android/app/src/main/assets/css/toolWrapper.cssandroid/app/src/main/assets/js/textRemover.jssrc/hooks/persisted/useSettings.tssrc/screens/novel/components/ExportNovelAsEpubButton.tsxsrc/screens/reader/ReaderScreen.tsxsrc/screens/reader/components/WebViewReader.tsxsrc/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsxsrc/screens/settings/SettingsCustomCodeScreen/index.tsxsrc/screens/settings/SettingsReaderScreen/SettingsReaderScreen.tsxsrc/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsxsrc/screens/settings/SettingsReaderScreen/components/dummy.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/assets/js/textRemover.js
- src/screens/settings/SettingsCustomCodeScreen/index.tsx
- src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
android/app/src/main/assets/js/textRemover.js (2)
48-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
window.getSelection()null check still missing.
window.getSelection()can returnnullin sandboxed/detached document contexts on Android WebView. Lines 48-49 (showSelectionUI), 127 (getSelectedText), 142 (removeSelectedText), and 155 (replaceSelectedText) all dereference the result without guarding.🛡️ Proposed fix
function showSelectionUI() { const ui = createSelectionUI(); // Get selection bounds const selection = window.getSelection(); - if (selection.rangeCount > 0) { + if (selection && selection.rangeCount > 0) {function getSelectedText() { const selection = window.getSelection(); - if (selection.rangeCount > 0) { + if (selection && selection.rangeCount > 0) { return selection.toString().trim(); } return ''; }hideSelectionUI(); - window.getSelection().removeAllRanges(); + window.getSelection()?.removeAllRanges(); }Also applies to: 125-131, 141-142, 153-155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/assets/js/textRemover.js` around lines 48 - 49, The code calls window.getSelection() in showSelectionUI, getSelectedText, removeSelectedText, and replaceSelectedText without guarding for a null return; update each function to first assign const selection = window.getSelection(); then check if (!selection) { /* return safe default or no-op (e.g., return null or false) */ } before accessing selection.rangeCount or selection.getRangeAt, and ensure callers handle the safe default; this prevents dereferencing null in sandboxed/detached WebView contexts.
87-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
readerglobal is still accessed without an existence guard.The previous review flagged this —
reader.generalSettings.val.verticalSeekbarandreader.hidden.valare dereferenced directly. IftextRemover.jsruns beforereaderis defined in the WebView (injection-order races), this throws aReferenceErrorand the overlay silently fails to position. Same risk on Line 98.🛡️ Proposed guard
- const avoidScrollbar = reader.generalSettings.val.verticalSeekbar - ? 0 - : 42; - const avoidUI = !reader.hidden.val ? 46 + avoidScrollbar : 0; + const hasReader = typeof reader !== 'undefined'; + const avoidScrollbar = hasReader && reader.generalSettings?.val?.verticalSeekbar + ? 0 + : 42; + const avoidUI = hasReader && !reader.hidden?.val ? 46 + avoidScrollbar : 0; topPosition = viewportHeight - bottomSafeArea - avoidUI - 4; ui.style.top = topPosition + 'px'; ui.style.bottom = 'auto'; } else { // Selection is in bottom half, position UI at top (accounting for status bar) topPosition = Math.max(topSafeArea, statusBarHeight + 20); - const avoidUI = !reader.hidden.val ? 34 : 0; + const avoidUI = (typeof reader !== 'undefined' && !reader.hidden?.val) ? 34 : 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/assets/js/textRemover.js` around lines 87 - 99, The code dereferences the global reader (reader.generalSettings.val.verticalSeekbar and reader.hidden.val) without guarding for its existence, causing a ReferenceError if the WebView injects this script before reader is defined; update the positioning logic in textRemover.js to check for reader (and nested properties) before accessing them — e.g., use existence checks or optional chaining and sensible defaults when computing avoidScrollbar and avoidUI, and apply the same guard where reader.hidden.val is used before setting ui.style.top and ui.style.bottom (references: reader, reader.generalSettings.val.verticalSeekbar, reader.hidden.val, topPosition, ui.style.top).src/screens/reader/components/Hooks/useTextModifications.ts (1)
82-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace flow still cannot update an existing rule.
Re-selecting text that already has a replacement opens the modal but
handleReplaceSavesilently no-ops because of theif (!(selectedTextForReplace in newReplaceText))guard. Users have no way to edit or correct an existing replacement, and the function still returnstrue, so the caller treats it as success. Also consider seedingreplacementTextfrom the existing entry when opening the modal (Line 75) so the user can edit rather than start from empty.🐛 Proposed fix
} else if (action === 'replace') { - // Show modal for user to enter replacement text setSelectedTextForReplace(text); - setReplacementText(''); + setReplacementText(readerSettings.replaceText[text] ?? ''); setReplaceModalVisible(true); } @@ const handleReplaceSave = React.useCallback(() => { if (!selectedTextForReplace) return false; const newReplaceText = { ...readerSettings.replaceText }; - if (!(selectedTextForReplace in newReplaceText)) { - newReplaceText[selectedTextForReplace] = replacementText; - setChapterReaderSettings({ replaceText: newReplaceText }); - } + newReplaceText[selectedTextForReplace] = replacementText; + setChapterReaderSettings({ replaceText: newReplaceText }); setReplaceModalVisible(false); return true; }, [If
handleTextActionis updated to also seed from existing rules, addreaderSettings.replaceTextto its dependency array.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/reader/components/Hooks/useTextModifications.ts` around lines 82 - 91, handleReplaceSave currently refuses to update an existing replacement because of the guard if (!(selectedTextForReplace in newReplaceText)) and still returns true; change it so that it always writes the replacement (assign newReplaceText[selectedTextForReplace] = replacementText) and calls setChapterReaderSettings({ replaceText: newReplaceText }) to update or create the rule, then close the modal via setReplaceModalVisible(false) and return true only on successful save. Also seed the modal's replacementText state when opening the modal (where handleTextAction / modal-open logic sets selectedTextForReplace) by looking up readerSettings.replaceText[selectedTextForReplace] so the user can edit existing entries, and if you update handleTextAction to read from readerSettings.replaceText add readerSettings.replaceText to its dependency array.
🧹 Nitpick comments (1)
src/screens/reader/components/Hooks/useTextModifications.ts (1)
105-111: ⚡ Quick win
eventTextActionlacks defensive checks and consistent memoization.The function has several issues:
- When
event.datais an empty object,Object.keys(event.data)[0]isundefined. The subsequent castas stringmasks this, silently passing'undefined'tohandleTextAction.- Only the first key is consumed; additional fields are silently dropped.
- The function is not memoized despite
handleTextActionbeing memoized, creating inconsistency.- Explicit action narrowing to
'remove' | 'replace'would prevent unknown payloads from reaching the handler.♻️ Proposed refactor
- function eventTextAction(event: WebViewPostEvent) { - if (event.data) { - const action = Object.keys(event.data)[0]; - const text = event.data[action]; - handleTextAction(action as string, String(text)); - } - } + const eventTextAction = React.useCallback( + (event: WebViewPostEvent) => { + const data = event?.data; + if (!data || typeof data !== 'object') return; + const [action] = Object.keys(data); + if (action !== 'remove' && action !== 'replace') return; + const text = data[action]; + if (typeof text !== 'string' || !text) return; + handleTextAction(action, text); + }, + [handleTextAction], + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/reader/components/Hooks/useTextModifications.ts` around lines 105 - 111, eventTextAction currently reads only the first key, casts undefined to string, and isn't memoized; update eventTextAction (used with WebViewPostEvent) to be wrapped in useCallback and perform defensive checks: ensure event.data exists and has entries, iterate over Object.entries(event.data) instead of taking only the first key, validate each key is one of the allowed actions ('remove' | 'replace') before calling handleTextAction, and coerce the value safely to string; skip any unknown keys so malformed payloads don't reach handleTextAction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Modal/KeyboardAvoidingModal.tsx`:
- Around line 63-68: The computed availableHeight in KeyboardAvoidingModal.tsx
(variable availableHeight used to set maxHeight/transform) can go negative when
the keyboard is large; clamp it before returning (e.g., const
clampedAvailableHeight = Math.max(0, availableHeight) or use a sensible
minModalHeight) and use clampedAvailableHeight for maxHeight and any transform
calculations so the modal never collapses into a negative/ unusable height.
In `@src/screens/reader/components/Hooks/useTextModifications.ts`:
- Line 24: In useTextModifications, replace all console.warn/console.error calls
(e.g., the 'Invalid regex flags in removeText:' log) with the project's logger
API (or processLogger) and include the caught error variable instead of leaving
'e' unused; for example, change console.warn('Invalid regex flags in
removeText:', text) to logger.warn('Invalid regex flags in removeText', { text,
error: e }) and in other catch blocks pass the error into the logger so 'e' is
used; if a project logger isn't available, add a single-line eslint exemption
(// eslint-disable-next-line no-console) at each console call and remove or use
the unused 'e' by including it in the log message.
---
Duplicate comments:
In `@android/app/src/main/assets/js/textRemover.js`:
- Around line 48-49: The code calls window.getSelection() in showSelectionUI,
getSelectedText, removeSelectedText, and replaceSelectedText without guarding
for a null return; update each function to first assign const selection =
window.getSelection(); then check if (!selection) { /* return safe default or
no-op (e.g., return null or false) */ } before accessing selection.rangeCount or
selection.getRangeAt, and ensure callers handle the safe default; this prevents
dereferencing null in sandboxed/detached WebView contexts.
- Around line 87-99: The code dereferences the global reader
(reader.generalSettings.val.verticalSeekbar and reader.hidden.val) without
guarding for its existence, causing a ReferenceError if the WebView injects this
script before reader is defined; update the positioning logic in textRemover.js
to check for reader (and nested properties) before accessing them — e.g., use
existence checks or optional chaining and sensible defaults when computing
avoidScrollbar and avoidUI, and apply the same guard where reader.hidden.val is
used before setting ui.style.top and ui.style.bottom (references: reader,
reader.generalSettings.val.verticalSeekbar, reader.hidden.val, topPosition,
ui.style.top).
In `@src/screens/reader/components/Hooks/useTextModifications.ts`:
- Around line 82-91: handleReplaceSave currently refuses to update an existing
replacement because of the guard if (!(selectedTextForReplace in
newReplaceText)) and still returns true; change it so that it always writes the
replacement (assign newReplaceText[selectedTextForReplace] = replacementText)
and calls setChapterReaderSettings({ replaceText: newReplaceText }) to update or
create the rule, then close the modal via setReplaceModalVisible(false) and
return true only on successful save. Also seed the modal's replacementText state
when opening the modal (where handleTextAction / modal-open logic sets
selectedTextForReplace) by looking up
readerSettings.replaceText[selectedTextForReplace] so the user can edit existing
entries, and if you update handleTextAction to read from
readerSettings.replaceText add readerSettings.replaceText to its dependency
array.
---
Nitpick comments:
In `@src/screens/reader/components/Hooks/useTextModifications.ts`:
- Around line 105-111: eventTextAction currently reads only the first key, casts
undefined to string, and isn't memoized; update eventTextAction (used with
WebViewPostEvent) to be wrapped in useCallback and perform defensive checks:
ensure event.data exists and has entries, iterate over
Object.entries(event.data) instead of taking only the first key, validate each
key is one of the allowed actions ('remove' | 'replace') before calling
handleTextAction, and coerce the value safely to string; skip any unknown keys
so malformed payloads don't reach handleTextAction.
🪄 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: a5c4da0b-8da4-438c-bc52-7f1743137bc3
📒 Files selected for processing (6)
android/app/src/main/assets/js/textRemover.jssrc/components/Modal/KeyboardAvoidingModal.tsxsrc/screens/reader/components/Hooks/useCustomCode.tssrc/screens/reader/components/Hooks/useTextModifications.tssrc/screens/reader/components/WebViewReader.tsxsrc/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/screens/reader/components/Hooks/useCustomCode.ts
- src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx
- src/screens/reader/components/WebViewReader.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx (3)
53-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat
index === -1as create mode, not edit mode.The
isEditingflag istruewhenevereditingSnippetis non-null, including wheneditingSnippet.index === -1(create new snippet). This disables the language toggles (lines 190, 197) even in create mode. Gate edit mode oneditingSnippet?.index !== -1instead.🔧 Proposed fix
- const isEditing = editingSnippet !== null && editingSnippet !== undefined; + const isEditing = + editingSnippet !== null && + editingSnippet !== undefined && + editingSnippet.index !== -1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx` around lines 53 - 55, The isEditing flag currently treats any non-null editingSnippet as edit mode (const isEditing = editingSnippet !== null && editingSnippet !== undefined), which incorrectly marks index === -1 (create mode) as editing and disables language toggles; change the gating to check editingSnippet.index !== -1 (e.g., const isEditing = editingSnippet != null && editingSnippet.index !== -1) and update dependent computed values (editIndex and editIsJS) to use this revised isEditing so create-mode (index === -1) falls back to snippetIndex and dLang respectively.
136-137: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace the snippet object instead of mutating it.
The shallow copy
[...snippets]preserves the original object references. MutatingnewSnippets[editIndex].nameand.codedirectly modifies the original snippet object, which can break memoization inuseCustomCodeif it relies on referential equality.♻️ Proposed fix
// Editing existing snippet if (isEditing && editIndex !== undefined && editIndex !== -1) { - newSnippets[editIndex].name = title; - newSnippets[editIndex].code = code; + newSnippets[editIndex] = { + ...newSnippets[editIndex], + name: title, + code, + }; setSettings({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx` around lines 136 - 137, The code mutates an existing snippet object by setting newSnippets[editIndex].name and .code which preserves the original reference from snippets and can break referential-equality checks in useCustomCode; instead replace the whole snippet object at editIndex with a new object (e.g., create newSnippets by shallow-copying snippets and assign newSnippets[editIndex] = { ...newSnippets[editIndex], name: title, code } or build the array via snippets.map when index === editIndex) so the array and the modified item are new references.
238-242:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire up or disable the "Open JS File" button.
This button has no
onPresshandler, so tapping it does nothing. Either implement the file-opening logic or setdisabled={true}until the feature is ready.🔧 Proposed fix (disable until implemented)
<Button style={styles.button} title={getString('readerSettings.openJSFile')} mode="outlined" + disabled={true} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx` around lines 238 - 242, The "Open JS File" Button in CodeRoute.tsx (Button with style={styles.button} and title={getString('readerSettings.openJSFile')}) has no onPress handler; either implement the handler (e.g., add a function like handleOpenJSFile/openJSFile in the same component that performs the file-open logic and pass it as onPress={handleOpenJSFile}) or disable the control while unimplemented by adding disabled={true} to the Button; update any related imports or state used by handleOpenJSFile and keep the Button reference (styles.button/getString('readerSettings.openJSFile')) so the UI remains consistent.src/screens/reader/components/Hooks/useTextModifications.ts (2)
85-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the guard that prevents updating existing replacements.
Lines 89-92 only save the replacement when the key doesn't already exist in
newReplaceText. This makes replacement rules immutable after creation—users cannot correct or change a previous replacement.🛠️ Proposed fix
const handleReplaceSave = React.useCallback(() => { if (!selectedTextForReplace) return false; const newReplaceText = { ...readerSettings.replaceText }; - if (!(selectedTextForReplace in newReplaceText)) { - newReplaceText[selectedTextForReplace] = replacementText; - setChapterReaderSettings({ replaceText: newReplaceText }); - } + newReplaceText[selectedTextForReplace] = replacementText; + setChapterReaderSettings({ replaceText: newReplaceText }); setReplaceModalVisible(false); return true; }, [🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/reader/components/Hooks/useTextModifications.ts` around lines 85 - 100, The guard in handleReplaceSave prevents updating existing replacement entries; remove the conditional that checks if (selectedTextForReplace in newReplaceText) so that newReplaceText[selectedTextForReplace] is always assigned to replacementText and setChapterReaderSettings({ replaceText: newReplaceText }) is called, then close the modal and return true; this ensures selectedTextForReplace, newReplaceText, replacementText, and setChapterReaderSettings are updated even when the key already exists.
75-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPre-fill the replacement text when editing an existing rule.
When the user selects text that already has a replacement, line 78 always sets
replacementTextto an empty string. This hides the current value and prevents users from editing it.🛠️ Proposed fix
} else if (action === 'replace') { - // Show modal for user to enter replacement text setSelectedTextForReplace(text); - setReplacementText(''); + setReplacementText(readerSettings.replaceText[text] ?? ''); setReplaceModalVisible(true); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/reader/components/Hooks/useTextModifications.ts` around lines 75 - 80, In useTextModifications, when handling action === 'replace' you clear the existing replacement with setReplacementText(''), which prevents editing an existing rule; instead look up the current replacement for the selected text (from the rules state/collection used by this hook) and call setReplacementText(existingReplacement || '') before setReplaceModalVisible(true), leaving setSelectedTextForReplace(text) as-is so the modal is pre-filled with the existing replacement for that selection.src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx (2)
207-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHide toggling is still one step behind.
The WebView gets the current
hiddenvalue and only then state flips, so the preview stays inverted until the next tap. ComputenextHiddenonce and use it for both operations.🛠️ Proposed fix
case 'hide': - if (hidden) { - webViewRef.current?.injectJavaScript('reader.hidden.val = true'); - } else { - webViewRef.current?.injectJavaScript('reader.hidden.val = false'); - } - setHidden(!hidden); + const nextHidden = !hidden; + webViewRef.current?.injectJavaScript( + `reader.hidden.val = ${nextHidden}`, + ); + setHidden(nextHidden); break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx` around lines 207 - 213, In the 'hide' branch (case 'hide') compute the nextHidden value once (e.g., const nextHidden = !hidden) and use nextHidden for both the injected JS call (via webViewRef.current?.injectJavaScript) and the state update (setHidden(nextHidden)) so the WebView and React state are synced immediately instead of being one tap behind.
75-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGenerated snippet error handler is still invalid JS.
JSON.stringify(snippet.name)is still embedded inside a single-quoted literal, and\nbecomes a real newline in the generated source. With any active JS snippet, this can make the injectedfn()script fail to parse before the snippet even runs.🛠️ Proposed fix
try { ${snippet.code} } catch (error) { - alert('Error loading executing ${JSON.stringify( - snippet.name, - )}:\n' + error); + alert( + 'Error executing ' + + ${JSON.stringify(snippet.name)} + + '\\n' + + error, + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx` around lines 75 - 82, The generated error alert embeds JSON.stringify(snippet.name) inside single quotes and contains a real newline, breaking the injected JS; in SettingsReaderWebView.tsx update the alert call inside the template returned with snippet.code so that snippet.name is concatenated (not inside the quoted literal) and the newline is escaped — e.g. replace alert('Error loading executing ${JSON.stringify(snippet.name)}:\n' + error); with a form that evaluates JSON.stringify(snippet.name) outside the quoted string and uses '\\n' (for example: alert('Error loading executing ' + ${JSON.stringify(snippet.name)} + '\\n' + error);) so the produced fn() script parses correctly.
🧹 Nitpick comments (3)
src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx (1)
277-304: 💤 Low valueRemove unused style definitions.
The styles
fakeTextInput,topField,codeField, andbottomFieldare defined but never used. ThescrollContentstyle is an empty object. Consider removing these to reduce clutter.♻️ Cleanup suggestion
scrollContent: {}, - text: { - flex: 1, - }, + text: { flex: 1 }, button: { marginHorizontal: 8, flexBasis: '40%', flex: 1, }, snippetName: { marginTop: 8, marginBottom: 16, }, - fakeTextInput: { - borderRadius: 4, - borderWidth: 1, - borderStyle: 'solid', - paddingHorizontal: 16, - paddingVertical: 10, - marginHorizontal: 1, - marginVertical: 2, - }, - topField: { - borderBottomLeftRadius: 0, - borderBottomRightRadius: 0, - borderBottomWidth: 0, - flexGrow: 1, - }, - codeField: { - verticalAlign: 'top', - flexGrow: 1, - borderRadius: 0, - borderTopWidth: 0, - borderBottomWidth: 0, - }, - bottomField: { - flexGrow: 1, - borderTopLeftRadius: 0, - borderTopRightRadius: 0, - borderTopWidth: 0, - }, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx` around lines 277 - 304, Remove the unused style definitions from the Stylesheet: delete the fakeTextInput, topField, codeField, and bottomField entries and remove the empty scrollContent object from the styles block in CodeRoute.tsx so the styles object only contains styles actually referenced by the component; search for those symbol names (fakeTextInput, topField, codeField, bottomField, scrollContent) to confirm they aren’t used elsewhere before deleting.src/screens/reader/components/Hooks/useTextModifications.ts (2)
108-114: 💤 Low valueConsider stronger typing for WebView event data.
Lines 110-112 use
Object.keysand type assertions to extract the action and text. Ifevent.datahas a known shape (e.g.,{ remove?: string; replace?: string }), adding a type guard or interface would catch errors earlier.♻️ Example type-safe approach
function eventTextAction(event: WebViewPostEvent) { if (!event.data) return; if ('remove' in event.data && typeof event.data.remove === 'string') { handleTextAction('remove', event.data.remove); } else if ('replace' in event.data && typeof event.data.replace === 'string') { handleTextAction('replace', event.data.replace); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/reader/components/Hooks/useTextModifications.ts` around lines 108 - 114, The eventTextAction handler currently uses Object.keys and type assertions on event.data which is brittle; update it to use a typed shape for WebViewPostEvent (e.g., an interface with optional fields like remove?: string, replace?: string) and add explicit type guards that check for the presence and typeof the string before calling handleTextAction('remove' | 'replace', text). Locate eventTextAction and replace the generic key extraction with guarded branches that validate event.data.remove and event.data.replace (or a discriminated union) so you avoid unsafe casts and guarantee correct types passed to handleTextAction.
1-1: ⚡ Quick winPrefer targeted logging over file-level eslint-disable.
Disabling
no-consolefor the entire file hides future violations. Consider using the project's logger API (if available) for theconsole.warncalls at lines 21 and 28, or adding per-line exemptions instead of a blanket disable.♻️ Proposed refactor
If a project logger is available:
-/* eslint-disable no-console */ import { useChapterReaderSettings } from '@hooks/persisted/useSettings'; import React, { useCallback, useMemo, useState } from 'react'; import { WebViewPostEvent } from '../WebViewReader'; +import { logger } from '@utils/logger'; // adjust import path as neededThen replace
console.warncalls withlogger.warn.Alternatively, use per-line exemptions:
-/* eslint-disable no-console */ import { useChapterReaderSettings } from '@hooks/persisted/useSettings';And at each console call site:
if (hasInvalidFlags) { + // eslint-disable-next-line no-console console.warn('Invalid regex flags in removeText:', text); return text; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/reader/components/Hooks/useTextModifications.ts` at line 1, Remove the file-level "/* eslint-disable no-console */" in useTextModifications.ts and either replace the two console.warn calls (the warnings inside the useTextModifications hook at the sites currently using console.warn) with the project's logger.warn API, or if a project logger isn't available add per-line eslint disable comments directly above those two calls to limit scope; update references to the unique symbol useTextModifications (and the specific console.warn call sites) so the warning output uses logger.warn or has a targeted eslint exemption.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/screens/reader/components/Hooks/useTextModifications.ts`:
- Around line 15-33: The saveRegex inside useTextModifications builds a RegExp
from user-controlled regex[1] which can trigger ReDoS; before constructing the
RegExp, validate the pattern with the safe-regex-test helper (e.g., import and
call the library's validator on regex[1] and flags) and if it reports unsafe,
warn and return the original text; additionally enforce a max pattern length
(reject very long patterns) and keep the existing try/catch as a final guard so
only syntactically-safe, length-limited, and library-validated patterns are
compiled in new RegExp(...) inside saveRegex.
In `@src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx`:
- Line 70: The component currently indexes snippets with editIndex without
bounds checking, which can yield undefined and break the save logic; update all
places that read snippets[editIndex] (e.g., the initial selection and the save
handler such as handleSave/onSave) to first verify typeof editIndex === 'number'
and editIndex >= 0 && editIndex < snippets.length, returning null or aborting
the save when out of range, and normalize any undefined to null before further
processing.
In
`@src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx`:
- Around line 204-205: The onMessage handler in SettingsReaderWebView.tsx
currently calls JSON.parse(ev.nativeEvent.data) directly into a WebViewPostEvent
and will crash on malformed/non-JSON messages; wrap the parse in a try/catch
(using the existing local names ev.nativeEvent.data and event/WebViewPostEvent)
and if parsing fails, log/ignore the message and return early so you "fail
closed" before any switch or access to event.type; additionally validate that
the parsed object has a type string before proceeding with the existing switch
logic.
---
Duplicate comments:
In `@src/screens/reader/components/Hooks/useTextModifications.ts`:
- Around line 85-100: The guard in handleReplaceSave prevents updating existing
replacement entries; remove the conditional that checks if
(selectedTextForReplace in newReplaceText) so that
newReplaceText[selectedTextForReplace] is always assigned to replacementText and
setChapterReaderSettings({ replaceText: newReplaceText }) is called, then close
the modal and return true; this ensures selectedTextForReplace, newReplaceText,
replacementText, and setChapterReaderSettings are updated even when the key
already exists.
- Around line 75-80: In useTextModifications, when handling action === 'replace'
you clear the existing replacement with setReplacementText(''), which prevents
editing an existing rule; instead look up the current replacement for the
selected text (from the rules state/collection used by this hook) and call
setReplacementText(existingReplacement || '') before
setReplaceModalVisible(true), leaving setSelectedTextForReplace(text) as-is so
the modal is pre-filled with the existing replacement for that selection.
In `@src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx`:
- Around line 53-55: The isEditing flag currently treats any non-null
editingSnippet as edit mode (const isEditing = editingSnippet !== null &&
editingSnippet !== undefined), which incorrectly marks index === -1 (create
mode) as editing and disables language toggles; change the gating to check
editingSnippet.index !== -1 (e.g., const isEditing = editingSnippet != null &&
editingSnippet.index !== -1) and update dependent computed values (editIndex and
editIsJS) to use this revised isEditing so create-mode (index === -1) falls back
to snippetIndex and dLang respectively.
- Around line 136-137: The code mutates an existing snippet object by setting
newSnippets[editIndex].name and .code which preserves the original reference
from snippets and can break referential-equality checks in useCustomCode;
instead replace the whole snippet object at editIndex with a new object (e.g.,
create newSnippets by shallow-copying snippets and assign newSnippets[editIndex]
= { ...newSnippets[editIndex], name: title, code } or build the array via
snippets.map when index === editIndex) so the array and the modified item are
new references.
- Around line 238-242: The "Open JS File" Button in CodeRoute.tsx (Button with
style={styles.button} and title={getString('readerSettings.openJSFile')}) has no
onPress handler; either implement the handler (e.g., add a function like
handleOpenJSFile/openJSFile in the same component that performs the file-open
logic and pass it as onPress={handleOpenJSFile}) or disable the control while
unimplemented by adding disabled={true} to the Button; update any related
imports or state used by handleOpenJSFile and keep the Button reference
(styles.button/getString('readerSettings.openJSFile')) so the UI remains
consistent.
In
`@src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx`:
- Around line 207-213: In the 'hide' branch (case 'hide') compute the nextHidden
value once (e.g., const nextHidden = !hidden) and use nextHidden for both the
injected JS call (via webViewRef.current?.injectJavaScript) and the state update
(setHidden(nextHidden)) so the WebView and React state are synced immediately
instead of being one tap behind.
- Around line 75-82: The generated error alert embeds
JSON.stringify(snippet.name) inside single quotes and contains a real newline,
breaking the injected JS; in SettingsReaderWebView.tsx update the alert call
inside the template returned with snippet.code so that snippet.name is
concatenated (not inside the quoted literal) and the newline is escaped — e.g.
replace alert('Error loading executing ${JSON.stringify(snippet.name)}:\n' +
error); with a form that evaluates JSON.stringify(snippet.name) outside the
quoted string and uses '\\n' (for example: alert('Error loading executing ' +
${JSON.stringify(snippet.name)} + '\\n' + error);) so the produced fn() script
parses correctly.
---
Nitpick comments:
In `@src/screens/reader/components/Hooks/useTextModifications.ts`:
- Around line 108-114: The eventTextAction handler currently uses Object.keys
and type assertions on event.data which is brittle; update it to use a typed
shape for WebViewPostEvent (e.g., an interface with optional fields like
remove?: string, replace?: string) and add explicit type guards that check for
the presence and typeof the string before calling handleTextAction('remove' |
'replace', text). Locate eventTextAction and replace the generic key extraction
with guarded branches that validate event.data.remove and event.data.replace (or
a discriminated union) so you avoid unsafe casts and guarantee correct types
passed to handleTextAction.
- Line 1: Remove the file-level "/* eslint-disable no-console */" in
useTextModifications.ts and either replace the two console.warn calls (the
warnings inside the useTextModifications hook at the sites currently using
console.warn) with the project's logger.warn API, or if a project logger isn't
available add per-line eslint disable comments directly above those two calls to
limit scope; update references to the unique symbol useTextModifications (and
the specific console.warn call sites) so the warning output uses logger.warn or
has a targeted eslint exemption.
In `@src/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsx`:
- Around line 277-304: Remove the unused style definitions from the Stylesheet:
delete the fakeTextInput, topField, codeField, and bottomField entries and
remove the empty scrollContent object from the styles block in CodeRoute.tsx so
the styles object only contains styles actually referenced by the component;
search for those symbol names (fakeTextInput, topField, codeField, bottomField,
scrollContent) to confirm they aren’t used elsewhere before deleting.
🪄 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: fab0a2df-f17b-4d3f-92d1-cb7c9f9014ea
📒 Files selected for processing (13)
android/app/src/main/assets/js/textRemover.jssrc/components/Common/ToggleButton.tsxsrc/components/Modal/KeyboardAvoidingModal.tsxsrc/hooks/persisted/useSettings.tssrc/screens/reader/ReaderScreen.tsxsrc/screens/reader/components/Hooks/useTextModifications.tssrc/screens/reader/components/WebViewReader.tsxsrc/screens/settings/SettingsCustomCodeScreen/Routes/CodeRoute.tsxsrc/screens/settings/SettingsCustomCodeScreen/Routes/SettingsRoute.tsxsrc/screens/settings/SettingsCustomCodeScreen/index.tsxsrc/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsxstrings/languages/en/strings.jsonstrings/types/index.ts
✅ Files skipped from review due to trivial changes (2)
- strings/types/index.ts
- strings/languages/en/strings.json
🚧 Files skipped from review as they are similar to previous changes (8)
- src/screens/settings/SettingsCustomCodeScreen/Routes/SettingsRoute.tsx
- src/components/Common/ToggleButton.tsx
- src/hooks/persisted/useSettings.ts
- src/screens/reader/ReaderScreen.tsx
- src/screens/settings/SettingsCustomCodeScreen/index.tsx
- src/screens/reader/components/WebViewReader.tsx
- android/app/src/main/assets/js/textRemover.js
- src/components/Modal/KeyboardAvoidingModal.tsx
Comment on lines
+204
to
+205
| onMessage={(ev: { nativeEvent: { data: string } }) => { | ||
| const event: WebViewPostEvent = JSON.parse(ev.nativeEvent.data); |
There was a problem hiding this comment.
Handle malformed WebView messages defensively.
One non-JSON postMessage from bundled reader code or a user snippet will throw here and break the preview bridge. Fail closed before switching on event.type.
🛠️ Proposed fix
onMessage={(ev: { nativeEvent: { data: string } }) => {
- const event: WebViewPostEvent = JSON.parse(ev.nativeEvent.data);
+ let event: WebViewPostEvent;
+ try {
+ event = JSON.parse(ev.nativeEvent.data);
+ } catch {
+ return;
+ }
switch (event.type) {📝 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
| onMessage={(ev: { nativeEvent: { data: string } }) => { | |
| const event: WebViewPostEvent = JSON.parse(ev.nativeEvent.data); | |
| onMessage={(ev: { nativeEvent: { data: string } }) => { | |
| let event: WebViewPostEvent; | |
| try { | |
| event = JSON.parse(ev.nativeEvent.data); | |
| } catch { | |
| return; | |
| } | |
| switch (event.type) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx`
around lines 204 - 205, The onMessage handler in SettingsReaderWebView.tsx
currently calls JSON.parse(ev.nativeEvent.data) directly into a WebViewPostEvent
and will crash on malformed/non-JSON messages; wrap the parse in a try/catch
(using the existing local names ev.nativeEvent.data and event/WebViewPostEvent)
and if parsing fails, log/ignore the message and return early so you "fail
closed" before any switch or access to event.type; additionally validate that
the parsed object has a type string before proceeding with the existing switch
logic.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
src/screens/reader/components/Hooks/useTextModifications.ts (1)
31-48: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow existing replacement rules to be updated.
Reselecting text with an existing replacement opens a blank modal, then save is a no-op because Line 45 skips existing keys.
Proposed fix
} else if (action === 'replace') { // Show modal for user to enter replacement text setSelectedTextForReplace(text); - setReplacementText(''); + setReplacementText(readerSettings.replaceText[text] ?? ''); setReplaceModalVisible(true); } }, - [readerSettings.removeText, setChapterReaderSettings], + [readerSettings.removeText, readerSettings.replaceText, setChapterReaderSettings], ); @@ const newReplaceText = { ...readerSettings.replaceText }; - if (!(selectedTextForReplace in newReplaceText)) { - newReplaceText[selectedTextForReplace] = replacementText; - setChapterReaderSettings({ replaceText: newReplaceText }); - } + newReplaceText[selectedTextForReplace] = replacementText; + setChapterReaderSettings({ replaceText: newReplaceText });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/reader/components/Hooks/useTextModifications.ts` around lines 31 - 48, The replacement flow in useTextModifications.handleReplaceSave only adds new keys and skips existing ones, so existing replacement rules cannot be edited after reselecting text. Update the logic in handleReplaceSave to always write the current replacementText into readerSettings.replaceText for selectedTextForReplace, and ensure the replace modal is populated with the existing value when reopening so edits are visible before saving.src/hooks/persisted/useSettings.ts (1)
312-338: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist the legacy JS/CSS migration in one immutable update.
This still mutates
storedSettings.codeSnippets*and callssetSettingstwice from the same stale object. When both legacy fields exist, the second write can re-persistcustomJS, causing repeat migration/duplicate snippets.Proposed fix
useEffect(() => { - if (storedSettings.customJS) { - storedSettings.codeSnippetsJS.push({ + if (!storedSettings.customJS && !storedSettings.customCSS) return; + + const nextSettings: MigrationChapterReaderSettings = { + ...storedSettings, + customJS: undefined, + customCSS: undefined, + codeSnippetsJS: [...storedSettings.codeSnippetsJS], + codeSnippetsCSS: [...storedSettings.codeSnippetsCSS], + }; + + if (storedSettings.customJS) { + nextSettings.codeSnippetsJS.push({ active: true, code: storedSettings.customJS, lang: 'js', name: 'Custom JS', }); - setSettings({ - ...storedSettings, - customJS: undefined, - codeSnippetsJS: storedSettings.codeSnippetsJS, - }); } + if (storedSettings.customCSS) { - storedSettings.codeSnippetsCSS.push({ + nextSettings.codeSnippetsCSS.push({ active: true, code: storedSettings.customCSS, lang: 'css', name: 'Custom CSS', }); - setSettings({ - ...storedSettings, - customCSS: undefined, - codeSnippetsCSS: storedSettings.codeSnippetsCSS, - }); } + + setSettings(nextSettings); }, [setSettings, storedSettings]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/persisted/useSettings.ts` around lines 312 - 338, The migration logic in useSettings is mutating storedSettings.codeSnippetsJS/CSS and issuing two separate setSettings calls from the same snapshot, which can re-save legacy fields and duplicate snippets when both customJS and customCSS are present. Update the useEffect block to build one new settings object immutably from storedSettings, append any legacy JS/CSS snippets into copied arrays, clear both customJS and customCSS on that single object, and call setSettings only once.src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx (2)
157-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInject the next hidden state, not the current one.
The handler sends
hiddento the WebView and only then flips state, so the preview receives a one-tick-old value.Proposed fix
case 'hide': onPress?.(); - if (hidden) { - webViewRef.current?.injectJavaScript('reader.hidden.val = true'); - } else { - webViewRef.current?.injectJavaScript('reader.hidden.val = false'); - } - setHidden(!hidden); + { + const nextHidden = !hidden; + webViewRef.current?.injectJavaScript( + `reader.hidden.val = ${nextHidden}`, + ); + setHidden(nextHidden); + } break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx` around lines 157 - 164, The hide action in SettingsReaderWebView is sending the current hidden value to the WebView before state updates, so the injected preview lags by one toggle. Update the 'hide' case in the onMessage handler to compute the next hidden state first, pass that next value into webViewRef.current?.injectJavaScript, and then call setHidden with the same next value so the WebView and React state stay in sync.
154-156: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard malformed WebView messages before switching on
type.A non-JSON message from bundled scripts or custom snippets will throw and break the preview bridge.
Proposed fix
nestedScrollEnabled={true} onMessage={(ev: { nativeEvent: { data: string } }) => { - const event: WebViewPostEvent = JSON.parse(ev.nativeEvent.data); + let event: WebViewPostEvent; + try { + event = JSON.parse(ev.nativeEvent.data); + } catch { + return; + } + if (!event || typeof event.type !== 'string') return; switch (event.type) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx` around lines 154 - 156, The onMessage handler in SettingsReaderWebView currently parses ev.nativeEvent.data directly and then switches on event.type, which will throw on malformed or non-JSON WebView messages. Update the handler to safely parse the payload first, guard against JSON.parse failures and invalid shapes before reaching the switch, and only proceed when a valid WebViewPostEvent is present. Use the existing onMessage callback and WebViewPostEvent typing to locate the fix.
🧹 Nitpick comments (3)
src/screens/novel/components/EditInfoModal.tsx (2)
74-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant nested
<Portal>.
KeyboardAvoidingModalalready wraps its content in its own<Portal>internally, so this outer<Portal>wrapper (leftover from the previousModalusage) is now redundant.The component returns
<Portal><Modal visible={visible} onDismiss={onDismiss} {...props} style={styles.modalWrapper}>.♻️ Proposed cleanup
return ( - <Portal> - <KeyboardAvoidingModal + <KeyboardAvoidingModal title={getString('novelScreen.edit.info')} ... - </KeyboardAvoidingModal> - </Portal> + </KeyboardAvoidingModal> );Also applies to: 213-215
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/novel/components/EditInfoModal.tsx` around lines 74 - 90, Remove the redundant outer Portal wrapper around KeyboardAvoidingModal in EditInfoModal, since KeyboardAvoidingModal already renders its own Portal internally. Update the component return so it directly renders KeyboardAvoidingModal and keep the existing props and handlers (onSave, onCancel, onReset, hideModal) unchanged.
79-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
hideModal()call inonSave.Per
KeyboardAvoidingModal'sdismisslogic, returningtruefromonSavealready triggersonDismiss()(which calls theonDismiss={hideModal}prop). The explicithideModal()here fires it a second time.The dismiss function checks
if (cb?.() === false) return;then callsonDismiss();.♻️ Proposed simplification
onSave={() => { setNovel(novelInfo); updateNovelInfo(novelInfo); - hideModal(); return true; }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/novel/components/EditInfoModal.tsx` around lines 79 - 84, The onSave handler in EditInfoModal redundantly calls hideModal after save, even though returning true already causes KeyboardAvoidingModal’s dismiss flow to invoke onDismiss via the existing onDismiss={hideModal} prop. Remove the explicit hideModal() from the onSave callback and keep the save/update logic plus return true so the modal is dismissed only once.src/screens/settings/SettingsCustomCodeScreen/Components/SimpleCodeEditor.tsx (1)
6-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnnecessary escape characters inside character classes.
\[doesn't need escaping inside a[...]character class (only],^,-, and\need special handling). Flagged by ESLint at line 23; the same pattern is duplicated in theJS_TOKEN_REGEXstring at line 15.Proposed fix
-const JS_PUNCTUATION = /^[\[\]{}()^$?.!:,;=+\-*/<>&|~]+$/; +const JS_PUNCTUATION = /^[[\]{}()^$?.!:,;=+\-*/<>&|~]+$/;Also applies to: 23-23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsCustomCodeScreen/Components/SimpleCodeEditor.tsx` around lines 6 - 19, The JS_TOKEN_REGEX pattern in SimpleCodeEditor includes unnecessary escapes inside a character class, triggering the ESLint warning. Update the character class token pattern used in the regex string so only special class characters are escaped, and keep the same change consistent wherever JS_TOKEN_REGEX is defined/used in SimpleCodeEditor to avoid duplicate lint issues.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/screens/novel/components/EditInfoModal.tsx`:
- Around line 79-89: The Save/Reset handlers in EditInfoModal are firing the
async updateNovelInfo DB write without waiting for it or handling failures.
Update the onSave and onReset callbacks to await updateNovelInfo, wrap the write
in try/catch, and only call hideModal or commit the optimistic state update once
the write succeeds; if it fails, keep the modal open or surface an error so the
UI does not drift from the DB.
- Around line 86-89: The onReset handler in EditInfoModal is writing changes to
the database immediately by calling updateNovelInfo(initialNovelInfo), which
should not happen for a reset action. Update the onReset path so it only
restores the local edit state via setNovelInfo(initialNovelInfo) and does not
persist until onSave/dismiss is used; keep the behavior consistent with the
modal’s save flow and the existing updateNovelInfo and dismiss handlers.
In `@src/screens/settings/SettingsCustomCodeScreen/CodeSnippetsScreen.tsx`:
- Around line 107-111: The import action in CodeSnippetsScreen is still wired to
showToast('Not implemented'), so the UI exposes a broken control. Update the
IconButtonV2 usage in CodeSnippetsScreen to either remove/hide the
file-import-outline button until the feature exists, or render it disabled with
clear non-actionable copy, and keep the rest of the settings screen behavior
unchanged.
In `@src/screens/settings/SettingsCustomCodeScreen/Components/CodeInput.tsx`:
- Around line 85-91: The CodeInput component is receiving an external validation
error from SnippetEditor but never renders it, so users don’t see why saving
fails. Update CodeInput and its editor/rendering path to accept and display the
error prop passed in from SnippetEditor, using the existing CodeInputProps and
the CodeInput/CodeInput editor UI so the validation message is visible alongside
the input.
- Around line 106-112: The CodeInput debounce in setAndAnalyzeCode can still
fire after the component unmounts, leading to analyzeCode/setError running on an
unmounted editor. Add a cleanup in CodeInput (using the existing debounce ref)
to clear any pending timeout on unmount, and make sure the timeout is always
cancelled before scheduling a new one.
In
`@src/screens/settings/SettingsCustomCodeScreen/Components/SimpleCodeEditor.tsx`:
- Around line 193-236: The SimpleCodeEditor component is driving TextInput with
highlighted children instead of a controlled value, which can break editable
behavior in React Native. Update SimpleCodeEditor to pass the editor content
through the TextInput value prop (using the existing value prop) and stop
rendering highlightedCode as nested children. If syntax highlighting is still
needed, move it to a separate overlay/view and keep TextInput plain and
controlled.
In `@src/utils/customCode.ts`:
- Around line 48-69: The safeApplyRegex helper currently constructs and executes
a RegExp directly from user-controlled input, which can cause ReDoS in the
reader/preview path. Update safeApplyRegex to mitigate unsafe patterns before
calling RegExp and text.replace, either by validating the pattern with a
safe-regex checker, rejecting/escaping risky input, or adding a bounded
execution guard for the match/replacement flow. Keep the existing invalid-flag
and invalid-pattern handling, and apply the fix within safeApplyRegex and any
caller path that feeds removeText/replaceText settings into it.
- Around line 84-110: Guard applyTextModifications against empty-string
replacement keys so it cannot corrupt HTML by splitting on ''. In
applyTextModifications, add a check in the Object.entries(replaceText) loop to
skip or reject entries where the key is empty before calling safeApplyRegex or
split/join. Keep the existing regex-string handling intact, and make the
protection local to applyTextModifications so any future caller is safe.
---
Duplicate comments:
In `@src/hooks/persisted/useSettings.ts`:
- Around line 312-338: The migration logic in useSettings is mutating
storedSettings.codeSnippetsJS/CSS and issuing two separate setSettings calls
from the same snapshot, which can re-save legacy fields and duplicate snippets
when both customJS and customCSS are present. Update the useEffect block to
build one new settings object immutably from storedSettings, append any legacy
JS/CSS snippets into copied arrays, clear both customJS and customCSS on that
single object, and call setSettings only once.
In `@src/screens/reader/components/Hooks/useTextModifications.ts`:
- Around line 31-48: The replacement flow in
useTextModifications.handleReplaceSave only adds new keys and skips existing
ones, so existing replacement rules cannot be edited after reselecting text.
Update the logic in handleReplaceSave to always write the current
replacementText into readerSettings.replaceText for selectedTextForReplace, and
ensure the replace modal is populated with the existing value when reopening so
edits are visible before saving.
In
`@src/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsx`:
- Around line 157-164: The hide action in SettingsReaderWebView is sending the
current hidden value to the WebView before state updates, so the injected
preview lags by one toggle. Update the 'hide' case in the onMessage handler to
compute the next hidden state first, pass that next value into
webViewRef.current?.injectJavaScript, and then call setHidden with the same next
value so the WebView and React state stay in sync.
- Around line 154-156: The onMessage handler in SettingsReaderWebView currently
parses ev.nativeEvent.data directly and then switches on event.type, which will
throw on malformed or non-JSON WebView messages. Update the handler to safely
parse the payload first, guard against JSON.parse failures and invalid shapes
before reaching the switch, and only proceed when a valid WebViewPostEvent is
present. Use the existing onMessage callback and WebViewPostEvent typing to
locate the fix.
---
Nitpick comments:
In `@src/screens/novel/components/EditInfoModal.tsx`:
- Around line 74-90: Remove the redundant outer Portal wrapper around
KeyboardAvoidingModal in EditInfoModal, since KeyboardAvoidingModal already
renders its own Portal internally. Update the component return so it directly
renders KeyboardAvoidingModal and keep the existing props and handlers (onSave,
onCancel, onReset, hideModal) unchanged.
- Around line 79-84: The onSave handler in EditInfoModal redundantly calls
hideModal after save, even though returning true already causes
KeyboardAvoidingModal’s dismiss flow to invoke onDismiss via the existing
onDismiss={hideModal} prop. Remove the explicit hideModal() from the onSave
callback and keep the save/update logic plus return true so the modal is
dismissed only once.
In
`@src/screens/settings/SettingsCustomCodeScreen/Components/SimpleCodeEditor.tsx`:
- Around line 6-19: The JS_TOKEN_REGEX pattern in SimpleCodeEditor includes
unnecessary escapes inside a character class, triggering the ESLint warning.
Update the character class token pattern used in the regex string so only
special class characters are escaped, and keep the same change consistent
wherever JS_TOKEN_REGEX is defined/used in SimpleCodeEditor to avoid duplicate
lint issues.
🪄 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: d518045a-3d90-46a4-bf7f-293a4db23337
📒 Files selected for processing (27)
App.tsxandroid/app/src/main/assets/js/textRemover.jssrc/components/Common/ToggleButton.tsxsrc/components/Modal/KeyboardAvoidingModal.tsxsrc/components/Switch/SwitchItem.tsxsrc/hooks/persisted/useSettings.tssrc/navigators/MoreStack.tsxsrc/navigators/types/index.tssrc/screens/novel/components/EditInfoModal.tsxsrc/screens/novel/components/ExportNovelAsEpubButton.tsxsrc/screens/reader/ReaderScreen.tsxsrc/screens/reader/components/Hooks/useCustomCode.tssrc/screens/reader/components/Hooks/useTextModifications.tssrc/screens/reader/components/WebViewReader.tsxsrc/screens/settings/SettingsCustomCodeScreen/CodeSnippetsScreen.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/CodeInput.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/ListItems.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/SimpleCodeEditor.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/Snippet.tsxsrc/screens/settings/SettingsCustomCodeScreen/Modals/ReplaceItemModal.tsxsrc/screens/settings/SettingsCustomCodeScreen/SnippetEditor.tsxsrc/screens/settings/SettingsCustomCodeScreen/index.tsxsrc/screens/settings/SettingsReaderScreen/components/SettingsReaderWebView.tsxsrc/utils/customCode.tsstrings/languages/en/strings.jsonstrings/types/index.tstsconfig.json
💤 Files with no reviewable changes (2)
- tsconfig.json
- src/screens/settings/SettingsCustomCodeScreen/Components/ListItems.tsx
✅ Files skipped from review due to trivial changes (2)
- strings/types/index.ts
- strings/languages/en/strings.json
🚧 Files skipped from review as they are similar to previous changes (10)
- src/components/Common/ToggleButton.tsx
- src/navigators/MoreStack.tsx
- App.tsx
- src/screens/reader/ReaderScreen.tsx
- src/screens/novel/components/ExportNovelAsEpubButton.tsx
- src/navigators/types/index.ts
- src/components/Modal/KeyboardAvoidingModal.tsx
- android/app/src/main/assets/js/textRemover.js
- src/screens/settings/SettingsCustomCodeScreen/Modals/ReplaceItemModal.tsx
- src/screens/reader/components/WebViewReader.tsx
Comment on lines
+79
to
+89
| onSave={() => { | ||
| setNovel(novelInfo); | ||
| updateNovelInfo(novelInfo); | ||
| hideModal(); | ||
| return true; | ||
| }} | ||
| onCancel={hideModal} | ||
| onReset={() => { | ||
| setNovelInfo(initialNovelInfo); | ||
| updateNovelInfo(initialNovelInfo); | ||
| }} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Unhandled async write on Save/Reset.
updateNovelInfo is an async DB write, but neither call here is awaited or wrapped in try/catch. The modal already closes / UI already reflects the new state optimistically (setNovel(novelInfo)) before the write settles, so a failed write would silently leave the UI and DB out of sync.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/screens/novel/components/EditInfoModal.tsx` around lines 79 - 89, The
Save/Reset handlers in EditInfoModal are firing the async updateNovelInfo DB
write without waiting for it or handling failures. Update the onSave and onReset
callbacks to await updateNovelInfo, wrap the write in try/catch, and only call
hideModal or commit the optimistic state update once the write succeeds; if it
fails, keep the modal open or surface an error so the UI does not drift from the
DB.
Comment on lines
+86
to
+89
| onReset={() => { | ||
| setNovelInfo(initialNovelInfo); | ||
| updateNovelInfo(initialNovelInfo); | ||
| }} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
onReset persists to the database immediately.
Unlike onSave, onReset doesn't go through dismiss() (the button calls it directly), so the modal stays open — but it still calls updateNovelInfo(initialNovelInfo), writing to the DB before the user has confirmed anything via Save. A "reset" action is normally expected to only restore the local edit buffer, not commit a write.
🐛 Proposed fix
onReset={() => {
setNovelInfo(initialNovelInfo);
- updateNovelInfo(initialNovelInfo);
}}📝 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
| onReset={() => { | |
| setNovelInfo(initialNovelInfo); | |
| updateNovelInfo(initialNovelInfo); | |
| }} | |
| onReset={() => { | |
| setNovelInfo(initialNovelInfo); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/screens/novel/components/EditInfoModal.tsx` around lines 86 - 89, The
onReset handler in EditInfoModal is writing changes to the database immediately
by calling updateNovelInfo(initialNovelInfo), which should not happen for a
reset action. Update the onReset path so it only restores the local edit state
via setNovelInfo(initialNovelInfo) and does not persist until onSave/dismiss is
used; keep the behavior consistent with the modal’s save flow and the existing
updateNovelInfo and dismiss handlers.
Add forceFocused prop to TextInput and CodeInput and use the gesture-handler TextInput. Replace measure/useAnimatedRef logic with a keyboard-height based maxHeight calc, add keyboard-avoiding wrapper and focusedField tracking with timeouts to better manage focus. Introduce a self-hiding app bar that reacts to keyboard on the Code tab.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/screens/settings/SettingsCustomCodeScreen/SnippetEditor.tsx (1)
52-58: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAvoid mutating the existing snippet object in place.
[...snippets]is a shallow copy, sonewSnippets[snippetIndex!]is still the same object reference held in persisted state. Assigning.codemutates the original object beforesetSettingsruns, which can defeat reference-equality checks in downstream memoization/selectors and makes state changes non-obvious.♻️ Proposed fix
if (isEditing) { - const newSnippets = [...snippets]; - newSnippets[snippetIndex!].code = code; + const newSnippets = snippets.map((s, i) => + i === snippetIndex ? { ...s, code } : s, + ); setSettings({ [language === 'js' ? 'codeSnippetsJS' : 'codeSnippetsCSS']: newSnippets, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/settings/SettingsCustomCodeScreen/SnippetEditor.tsx` around lines 52 - 58, The update logic in SnippetEditor’s editing branch is mutating a persisted snippet object in place because `[...snippets]` only makes a shallow copy. Fix this by creating a new snippet object for `newSnippets[snippetIndex!]` instead of assigning to `.code` directly, then pass the updated array to `setSettings` so the `isEditing` path preserves immutability and reference equality for downstream selectors/memoization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/screens/settings/SettingsCustomCodeScreen/Components/SimpleCodeEditor.tsx`:
- Around line 442-445: The `SimpleCodeEditor` render logic is violating Hooks
rules by calling `useStableLineModels` only inside the `if (!lines)` branch, and
it also updates `setLines` during render. Refactor the component to compute the
line models unconditionally, derive an `effectiveLines` value from either the
incoming `lines` or the hook result, and render from that. Move the
`setLines?.(effectiveLines.length)` notification into a `useEffect` tied to the
computed lines so the parent update happens after render; also ensure
`useEffect` is imported in `SimpleCodeEditor.tsx`.
- Around line 473-524: The editor scroll sync is incomplete because the
TextInput is not wiring the existing onScroll handler, so scrollY never changes
and the highlighted overlay drifts. Update SimpleCodeEditor to pass onScroll
into the input element that renders the editable code, and ensure it updates the
scrollY Animated.Value used by the overlay transform. Keep the fix localized to
SimpleCodeEditor and the input props handling around handleChangeText, scrollY,
and the highlightedCodeProps setup.
---
Nitpick comments:
In `@src/screens/settings/SettingsCustomCodeScreen/SnippetEditor.tsx`:
- Around line 52-58: The update logic in SnippetEditor’s editing branch is
mutating a persisted snippet object in place because `[...snippets]` only makes
a shallow copy. Fix this by creating a new snippet object for
`newSnippets[snippetIndex!]` instead of assigning to `.code` directly, then pass
the updated array to `setSettings` so the `isEditing` path preserves
immutability and reference equality for downstream selectors/memoization.
🪄 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: 0217379b-2197-479f-8039-077a427f2540
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
package.jsonsrc/screens/settings/SettingsCustomCodeScreen/CodeSnippetsScreen.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/CodeInput.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/ListItems.tsxsrc/screens/settings/SettingsCustomCodeScreen/Components/SimpleCodeEditor.tsxsrc/screens/settings/SettingsCustomCodeScreen/Modals/ReplaceItemModal.tsxsrc/screens/settings/SettingsCustomCodeScreen/SnippetEditor.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- src/screens/settings/SettingsCustomCodeScreen/Components/ListItems.tsx
- src/screens/settings/SettingsCustomCodeScreen/Modals/ReplaceItemModal.tsx
Comment on lines
+442
to
+445
| if (!lines) { | ||
| lines = useStableLineModels(value!); | ||
| setLines?.(lines.length); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Rules-of-Hooks violation: useStableLineModels is called conditionally.
Calling a hook inside if (!lines) means the hook is invoked in some renders and not others whenever a caller alternates between passing lines vs value, which breaks React's hook-ordering invariant. Additionally, setLines?.(lines.length) runs a parent state update during render, which triggers React's "cannot update a component while rendering a different component" warning.
Compute the model unconditionally and derive the effective lines, and move the setLines notification into an effect.
🔧 Proposed fix
- const opacityStyle = useMemo(() => extractOpacityStyle(style), [style]);
- const textStyle = useMemo(() => extractTextStyle(style), [style]);
- const contentPadding = useMemo(() => extractContentPadding(style), [style]);
- const lineHeight = useMemo(() => getLineHeight(style), [style]);
- if (!lines) {
- lines = useStableLineModels(value!);
- setLines?.(lines.length);
- }
+ const opacityStyle = useMemo(() => extractOpacityStyle(style), [style]);
+ const textStyle = useMemo(() => extractTextStyle(style), [style]);
+ const contentPadding = useMemo(() => extractContentPadding(style), [style]);
+ const lineHeight = useMemo(() => getLineHeight(style), [style]);
+ const computed = useStableLineModels(value ?? '');
+ const effectiveLines = lines ?? computed;
+ useEffect(() => {
+ if (!lines) setLines?.(effectiveLines.length);
+ }, [lines, effectiveLines.length, setLines]);Then render effectiveLines.map(...). (Add useEffect to the React imports.)
Run to confirm no other callers depend on the render-time setLines behavior:
#!/bin/bash
rg -nP '<MemoizedHighlightedCode' -A6 src
rg -nP '\bsetLines\b' -C3 src🧰 Tools
🪛 ESLint
[error] 443-443: React Hook "useStableLineModels" is called conditionally. React Hooks must be called in the exact same order in every component render.
(react-hooks/rules-of-hooks)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/screens/settings/SettingsCustomCodeScreen/Components/SimpleCodeEditor.tsx`
around lines 442 - 445, The `SimpleCodeEditor` render logic is violating Hooks
rules by calling `useStableLineModels` only inside the `if (!lines)` branch, and
it also updates `setLines` during render. Refactor the component to compute the
line models unconditionally, derive an `effectiveLines` value from either the
incoming `lines` or the hook result, and render from that. Move the
`setLines?.(effectiveLines.length)` notification into a `useEffect` tied to the
computed lines so the parent update happens after render; also ensure
`useEffect` is imported in `SimpleCodeEditor.tsx`.
Source: Linters/SAST tools
Comment on lines
+473
to
+524
| onScroll, | ||
| scrollEnabled = true, | ||
| lines: _lines, | ||
| value, | ||
| mode, | ||
| style, | ||
| isDark, | ||
| setLines, | ||
| startLine, | ||
| ...props | ||
| }: SimpleCodeEditorProps & Omit<MemoizedHighlightedCode, 'hide'>) { | ||
| const hideHighlight = highlightMode === 'off'; | ||
| const showInput = highlightMode !== 'on'; | ||
| const scrollY = useRef(new Animated.Value(0)).current; | ||
| let lines = _lines; | ||
|
|
||
| const highlightedCodeProps = { | ||
| lines, | ||
| value, | ||
| mode, | ||
| style, | ||
| hide: hideHighlight, | ||
| isDark, | ||
| setLines, | ||
| startLine, | ||
| }; | ||
|
|
||
| const negativeScrollY = useMemo(() => { | ||
| return Animated.multiply(scrollY, -1); | ||
| }, [scrollY]); | ||
|
|
||
| const handleChangeText = useCallback( | ||
| (text: string) => { | ||
| onChangeText?.(text); | ||
| }, | ||
| [onChangeText], | ||
| ); | ||
|
|
||
| return ( | ||
| <View style={[styles.container, containerStyle]}> | ||
| <Animated.View | ||
| pointerEvents="none" | ||
| style={[ | ||
| StyleSheet.absoluteFill, | ||
| styles.highlightLayer, | ||
| { | ||
| transform: [{ translateY: negativeScrollY }], | ||
| }, | ||
| ]} | ||
| > | ||
| <MemoizedHighlightedCode {...highlightedCodeProps} /> | ||
| </Animated.View> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='src/screens/settings/SettingsCustomCodeScreen/Components/SimpleCodeEditor.tsx'
echo '--- file size ---'
wc -l "$file"
echo '--- outline ---'
ast-grep outline "$file" --view expanded || true
echo '--- search for scroll-related identifiers in file ---'
rg -n "onScroll|scrollY|Animated\.event|scrollEventThrottle|translateY|contentOffset" "$file" || true
echo '--- relevant slices ---'
sed -n '430,560p' "$file"Repository: CD-Z/lnreader
Length of output: 5768
Wire onScroll into the input. scrollY never updates because the TextInput doesn’t receive an onScroll, so the highlight overlay stays fixed and drifts out of sync as the editor scrolls.
🧰 Tools
🪛 ESLint
[error] 473-473: 'onScroll' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 487-487: 'lines' is never reassigned. Use 'const' instead.
(prefer-const)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/screens/settings/SettingsCustomCodeScreen/Components/SimpleCodeEditor.tsx`
around lines 473 - 524, The editor scroll sync is incomplete because the
TextInput is not wiring the existing onScroll handler, so scrollY never changes
and the highlighted overlay drifts. Update SimpleCodeEditor to pass onScroll
into the input element that renders the editable code, and ensure it updates the
scrollY Animated.Value used by the overlay transform. Keep the fix localized to
SimpleCodeEditor and the input props handling around handleChangeText, scrollY,
and the highlightedCodeProps setup.
Source: Linters/SAST tools
CD-Z
force-pushed
the
custom_code_settings_page_clean
branch
from
c558419 to
8f03941
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