Conversation
…sure correct style merging
Enable cross-page EPUB/HTML selection when the finger or caret stays at the screen edge. Keep PDF scroll pinning on the legacy path. Co-authored-by: Cursor <cursoragent@cursor.com>
Ignore caret edge signals after a turn and rely on pointer position so the selection end rect cannot immediately trigger the opposite page. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace full-height edge strips with tight corner ellipses and ignore off-screen caret positions so cross-page selection requires a 500ms corner hold. Co-authored-by: Cursor <cursoragent@cursor.com>
… and background no longer obscures text. Co-authored-by: Cursor <cursoragent@cursor.com>
- wrap id with CSS.escape to handle ids containing dots or special chars - align with existing CSS.escape usage in GeneralRender and navigationUtil
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds text-rule DOM processing, selection-based auto page-turning, PDF background and crop handling, highlight style refactoring, page animation updates, code highlighting, CSS changes, and title, footnote, layout, and link fixes. ChangesReader Feature Expansion
Sequence Diagram(s)sequenceDiagram
participant touchUtil as touchUtil
participant selectionAutoTurn as selectionAutoTurn
participant render as render
participant doc as iframe document
touchUtil->>selectionAutoTurn: onSelectStart() / onTouchMove()
selectionAutoTurn->>doc: read selection and caret geometry
selectionAutoTurn->>selectionAutoTurn: arm dwell timer
selectionAutoTurn->>render: next() / prev()
selectionAutoTurn->>selectionAutoTurn: applyScrollPin() / clear state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/libs/textProcessor.ts (1)
192-196: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize
linebefore theparserRegexfast path.
txtToHtml()now passes raw lines intoisTitle(), but this branch still skipscleanText(). Any custom chapter regex that previously matched trimmed/sanitized lines will now miss titles and can force the no-title fallback.Proposed fix
export const isTitle = (line: any, parserRegex: string = "") => { + const cleanedLine = cleanText(line); if (parserRegex) { - return new RegExp(parserRegex).test(line); + return new RegExp(parserRegex).test(cleanedLine); } - line = cleanText(line); // Clean once per line return ( - line && - line.length < 40 && - !isContain(line) && - (isStartWithChars(line) || - (line.startsWith("第") && startWithDI(line)) || - (line.startsWith("卷") && startWithJUAN(line)) || - (line.indexOf("第") > -1 && - line.lastIndexOf("第") < 7 && - startWithDI(line.substr(line.indexOf("第"))))) + cleanedLine && + cleanedLine.length < 40 && + !isContain(cleanedLine) && + (isStartWithChars(cleanedLine) || + (cleanedLine.startsWith("第") && startWithDI(cleanedLine)) || + (cleanedLine.startsWith("卷") && startWithJUAN(cleanedLine)) || + (cleanedLine.indexOf("第") > -1 && + cleanedLine.lastIndexOf("第") < 7 && + startWithDI(cleanedLine.substr(cleanedLine.indexOf("第"))))) ); };🤖 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/libs/textProcessor.ts` around lines 192 - 196, The isTitle() helper currently returns early on the parserRegex fast path without normalizing the input, so raw lines from txtToHtml() can miss matches that previously worked on cleaned text. Update isTitle() to apply cleanText() before testing parserRegex, and keep the same normalized value for both the custom regex branch and the default title detection logic so chapter matching stays consistent.src/utils/noteUtil.ts (1)
737-741: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude note icons in delegated note events.
The icon inserted at Line 788 has
data-key, but the delegated click/touch lookup only targets.kookit-note[data-key]; tapping the icon will not open the note.Suggested fix
+ const noteTargetSelector = ".kookit-note[data-key],.kookit-note-icon[data-key]"; + if (!(doc.body as any).__kookitDelegated) {- const target = (e.target as HTMLElement)?.closest?.( - ".kookit-note[data-key]" - ) as HTMLElement | null; + const target = (e.target as HTMLElement)?.closest?.( + noteTargetSelector + ) as HTMLElement | null;- const target = (el as HTMLElement)?.closest?.( - ".kookit-note[data-key]" - ) as HTMLElement | null; + const target = (el as HTMLElement)?.closest?.( + noteTargetSelector + ) as HTMLElement | null;iconNode.setAttribute("data-key", noteKey); + if (isNote && noteContent) { + iconNode.setAttribute("data-note-content", noteContent); + }Also applies to: 753-762, 777-789
🤖 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/utils/noteUtil.ts` around lines 737 - 741, The delegated note event lookup in noteUtil.ts only matches the .kookit-note[data-key] wrapper, so clicks/taps on the injected note icon are ignored. Update the event targeting logic used by the delegated handlers (including the blocks around handleNoteClick / touch handling) to also recognize the icon element inserted near the note rendering code, using its data-key or a shared selector so both the note container and icon open the note.
🤖 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/helpers/styleHelper.ts`:
- Line 165: The aside hiding rule in styleHelper should fully hide
footnote/note/endnote/rearnote content instead of only shrinking text, since
non-text children can still occupy space. Update the CSS generated in the
relevant styleHelper rule to use a full hiding approach such as display:none or
the prior off-screen hiding strategy for the aside[epub\\:type=...] selectors
and .hide so note/sidebar artifacts do not affect layout.
In `@src/libs/textProcessor.ts`:
- Around line 158-160: The full-file TXT rendering path in textProcessor.ts is
using the raw line in the heading branch, which makes small-file behavior
inconsistent with the large-file path. Update the loop in the branch that builds
htmlParts so the <h1> output uses the same normalized text as the large-file
logic, ideally via cleanText(item), while keeping the existing
isTitle(parserRegex) check unchanged.
In `@src/renders/GeneralRender.ts`:
- Line 118: The animation sentinel migration is incomplete in GeneralRender:
record() already treats "none" as the disabled state, but scrollToText() and any
related animation checks still use the old empty-string comparison, which leaves
the default path behaving like animation is enabled. Update the animation guard
in scrollToText() (and the matching check around the other referenced spot) to
use this.animation !== "none" consistently with the constructor assignment
this.animation = config.animation || "none", so non-mobile scroll-to-text no
longer waits unnecessarily.
- Around line 1215-1220: The page index is being passed into addPageAnimation,
but the returned flip controller still starts with pageNum at 0, so the current
page is never applied. Update the GeneralRender flow that creates pageAnimation
to call setPageNum on the returned controller with the computed current page
index, or change addPageAnimation so it initializes from pageIndex directly. Use
the addPageAnimation and setPageNum symbols to locate the fix.
In `@src/renders/PdfRender.ts`:
- Line 48: The crop defaults are being overwritten by a partial config in
PdfRender, which can leave pdfCrop sides undefined and break the math used
later. In PdfRender, merge config.pdfCrop with the full default crop object
before assigning this.pdfCrop, so cropRatio, visibleWidth, and scale always read
numeric values even when config.pdfCrop is partial.
In `@src/utils/animationUtil.ts`:
- Around line 67-78: `addPageAnimation` accepts `pageIndex` but currently
ignores it during initialization, so the book always starts at page 0. Seed the
initial page state from `pageIndex` when setting up the flip stack, and ensure
the value is passed into the initialization path that creates the page elements
so `GeneralRender` can open on the correct page.
In `@src/utils/layoutUtil.ts`:
- Around line 315-318: The optional Highlight.js asset loading in
fetchHighlightAsset should be isolated so failures do not abort transformText
before applyTextRules can run. Update fetchHighlightAsset and the highlight path
in transformText to check the fetch response status and fail closed on non-OK
responses, returning no highlight script instead of passing invalid text into
the highlighter. Keep the fallback behavior contained around fetchHighlightAsset
so missing or bad assets do not break chapter rendering.
In `@src/utils/pdfUtil.ts`:
- Line 296: The mimical flip handler result is being ignored in pdfUtil’s
page-turn flow, so iOS can fall through into the immediate scroll path after the
animation is already handled. Update the page-turn logic around
playMimicalFlip() to check its return value and exit early when it returns true,
keeping the rest of the scroll/page-move code from running in that case.
- Around line 65-69: `applyPdfCrop()` only adjusts the scroll-mode container
height for top/bottom crop, so left/right crop is ignored even though the iframe
is horizontally scaled. Update the height calculation in `pdfUtil.ts` to account
for `pdfCrop.left` and `pdfCrop.right` alongside `pdfCrop.top`/`bottom`, and use
the same crop-aware scaling logic already applied to the iframe so the visible
page height stays correct when `overflow: hidden` is enabled.
In `@src/utils/selectionAutoTurn.ts`:
- Around line 264-270: The dwell timer in noteCorner is not restarted when
engagedCorner changes, so a pending timer from the previous corner can block the
new corner from arming. Update the selectionAutoTurn flow around noteCorner and
armDwell so that switching corners clears or replaces any existing dwell timer
before starting a new one, ensuring the latest corner always gets armed.
In `@src/utils/textRuleUtil.ts`:
- Around line 152-158: The applyTextRules flow is applying every rule with scope
"book" through applyRule without verifying the current document belongs to that
book, which can leak book-specific changes. Update applyTextRules in
textRuleUtil to honor the active book context by checking the document’s current
bookKey before applying any "book" scoped rule, and only call applyRule for
matching book rules; keep "all" rules unaffected.
- Around line 136-142: The regex construction in textRuleUtil is unsafe because
`new RegExp(rule.pattern, "g")` can throw or create pathological patterns, and
`scope: "book"` rules are currently applied without checking `bookKey`. Update
the rule handling around the regex creation block to validate/guard
`rule.pattern` before instantiating `RegExp`, skip or safely reject invalid
patterns, and add a `bookKey` check so book-scoped rules only apply when the
current book matches the rule scope.
---
Outside diff comments:
In `@src/libs/textProcessor.ts`:
- Around line 192-196: The isTitle() helper currently returns early on the
parserRegex fast path without normalizing the input, so raw lines from
txtToHtml() can miss matches that previously worked on cleaned text. Update
isTitle() to apply cleanText() before testing parserRegex, and keep the same
normalized value for both the custom regex branch and the default title
detection logic so chapter matching stays consistent.
In `@src/utils/noteUtil.ts`:
- Around line 737-741: The delegated note event lookup in noteUtil.ts only
matches the .kookit-note[data-key] wrapper, so clicks/taps on the injected note
icon are ignored. Update the event targeting logic used by the delegated
handlers (including the blocks around handleNoteClick / touch handling) to also
recognize the icon element inserted near the note rendering code, using its
data-key or a shared selector so both the note container and icon open the note.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d618a89-f004-4e3d-b924-a440cca18c65
📒 Files selected for processing (15)
rollup.config.jssrc/helpers/styleHelper.tssrc/libs/html.tssrc/libs/pdf.jssrc/libs/textProcessor.tssrc/renders/GeneralRender.tssrc/renders/PdfRender.tssrc/utils/animationUtil.tssrc/utils/layoutUtil.tssrc/utils/navigationUtil.tssrc/utils/noteUtil.tssrc/utils/pdfUtil.tssrc/utils/selectionAutoTurn.tssrc/utils/textRuleUtil.tssrc/utils/touchUtil.ts
💤 Files with no reviewable changes (1)
- rollup.config.js
| // Hide aside elements | ||
| cssRules.push( | ||
| `aside[epub\\:type="footnote"],aside[epub\\:type="note"],aside[epub\\:type="endnote"],aside[epub\\:type="rearnote"],.hide{position: absolute; left: -9999px; top: -9999px;}` | ||
| `aside[epub\\:type="footnote"],aside[epub\\:type="note"],aside[epub\\:type="endnote"],aside[epub\\:type="rearnote"],.hide{font-size:0px !important;}` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a full hiding rule for footnote asides.
font-size:0px leaves non-text children and layout boxes visible. Use display:none or the previous off-screen hiding strategy so note/sidebar artifacts do not occupy reader space.
Suggested fix
- `aside[epub\\:type="footnote"],aside[epub\\:type="note"],aside[epub\\:type="endnote"],aside[epub\\:type="rearnote"],.hide{font-size:0px !important;}`
+ `aside[epub\\:type="footnote"],aside[epub\\:type="note"],aside[epub\\:type="endnote"],aside[epub\\:type="rearnote"],.hide{display:none !important;}`📝 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.
| `aside[epub\\:type="footnote"],aside[epub\\:type="note"],aside[epub\\:type="endnote"],aside[epub\\:type="rearnote"],.hide{font-size:0px !important;}` | |
| `aside[epub\\:type="footnote"],aside[epub\\:type="note"],aside[epub\\:type="endnote"],aside[epub\\:type="rearnote"],.hide{display:none !important;}` |
🤖 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/helpers/styleHelper.ts` at line 165, The aside hiding rule in styleHelper
should fully hide footnote/note/endnote/rearnote content instead of only
shrinking text, since non-text children can still occupy space. Update the CSS
generated in the relevant styleHelper rule to use a full hiding approach such as
display:none or the prior off-screen hiding strategy for the
aside[epub\\:type=...] selectors and .hide so note/sidebar artifacts do not
affect layout.
| for (const item of lines) { | ||
| const cleanedItem = cleanText(item); // Clean once per line | ||
| if (cleanedItem && isTitle(cleanedItem, parserRegex)) { | ||
| htmlParts.push(`<h1>${cleanedItem}</h1>`); // Push to array | ||
| if (item && isTitle(item, parserRegex)) { | ||
| htmlParts.push(`<h1>${item}</h1>`); // Push to array |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep <h1> text normalized in the full-file path.
This branch now renders the raw line, while the large-file branch at Lines 143-146 still renders cleanText(item). Small TXT files will therefore show decorative separators or leading/trailing whitespace in headings that disappear for large TXT files.
Proposed fix
for (const item of lines) {
if (item && isTitle(item, parserRegex)) {
- htmlParts.push(`<h1>${item}</h1>`); // Push to array
+ htmlParts.push(`<h1>${cleanText(item)}</h1>`); // Push to array
} else {📝 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.
| for (const item of lines) { | |
| const cleanedItem = cleanText(item); // Clean once per line | |
| if (cleanedItem && isTitle(cleanedItem, parserRegex)) { | |
| htmlParts.push(`<h1>${cleanedItem}</h1>`); // Push to array | |
| if (item && isTitle(item, parserRegex)) { | |
| htmlParts.push(`<h1>${item}</h1>`); // Push to array | |
| for (const item of lines) { | |
| if (item && isTitle(item, parserRegex)) { | |
| htmlParts.push(`<h1>${cleanText(item)}</h1>`); // Push to 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/libs/textProcessor.ts` around lines 158 - 160, The full-file TXT
rendering path in textProcessor.ts is using the raw line in the heading branch,
which makes small-file behavior inconsistent with the large-file path. Update
the loop in the branch that builds htmlParts so the <h1> output uses the same
normalized text as the large-file logic, ideally via cleanText(item), while
keeping the existing isTitle(parserRegex) check unchanged.
| const pageAnimation = addPageAnimation( | ||
| progress.totalPage, | ||
| this.isDarkMode, | ||
| backgroundColor || this.backgroundColor, | ||
| Math.max(0, Math.floor(progress.currentPage || 1) - 1) | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply the current page index after creating the flip controller.
The supplied addPageAnimation implementation accepts pageIndex but initializes pageNum to 0 and never uses the parameter. Call setPageNum on the returned controller, or update the utility to initialize from pageIndex.
Suggested local fix
+ const pageIndex = Math.max(0, Math.floor(progress.currentPage || 1) - 1);
const pageAnimation = addPageAnimation(
progress.totalPage,
this.isDarkMode,
backgroundColor || this.backgroundColor,
- Math.max(0, Math.floor(progress.currentPage || 1) - 1)
+ pageIndex
);
if (!pageAnimation) return;
+ pageAnimation.setPageNum?.(pageIndex);📝 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.
| const pageAnimation = addPageAnimation( | |
| progress.totalPage, | |
| this.isDarkMode, | |
| backgroundColor || this.backgroundColor, | |
| Math.max(0, Math.floor(progress.currentPage || 1) - 1) | |
| ); | |
| const pageIndex = Math.max(0, Math.floor(progress.currentPage || 1) - 1); | |
| const pageAnimation = addPageAnimation( | |
| progress.totalPage, | |
| this.isDarkMode, | |
| backgroundColor || this.backgroundColor, | |
| pageIndex | |
| ); | |
| if (!pageAnimation) return; | |
| pageAnimation.setPageNum?.(pageIndex); |
🤖 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/renders/GeneralRender.ts` around lines 1215 - 1220, The page index is
being passed into addPageAnimation, but the returned flip controller still
starts with pageNum at 0, so the current page is never applied. Update the
GeneralRender flow that creates pageAnimation to call setPageNum on the returned
controller with the computed current page index, or change addPageAnimation so
it initializes from pageIndex directly. Use the addPageAnimation and setPageNum
symbols to locate the fix.
| super({ ...config, convertChinese: "Default", format: "PDF" }); | ||
| this.pdfBuffer = pdfBuffer; | ||
| this.isStartFromEven = config.isStartFromEven || "no"; | ||
| this.pdfCrop = config.pdfCrop || { top: 0, bottom: 0, left: 0, right: 0 }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Merge crop defaults before using crop values in math.
config.pdfCrop can be partial because config is any; replacing the whole default object leaves sides like bottom/right as undefined, which turns cropRatio, visibleWidth, and scale into NaN.
Proposed fix
- this.pdfCrop = config.pdfCrop || { top: 0, bottom: 0, left: 0, right: 0 };
+ this.pdfCrop = {
+ top: 0,
+ bottom: 0,
+ left: 0,
+ right: 0,
+ ...(config.pdfCrop || {}),
+ };Also applies to: 1038-1057
🤖 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/renders/PdfRender.ts` at line 48, The crop defaults are being overwritten
by a partial config in PdfRender, which can leave pdfCrop sides undefined and
break the math used later. In PdfRender, merge config.pdfCrop with the full
default crop object before assigning this.pdfCrop, so cropRatio, visibleWidth,
and scale always read numeric values even when config.pdfCrop is partial.
| const cropRatio = (100 - pdfCrop.bottom - pdfCrop.top) / 100; | ||
|
|
||
| iframeContainer.style.paddingTop = `${(1 / aspectRatio) * cropRatio * 100}%`; | ||
| iframeContainer.style.marginBottom = `2%`; | ||
| iframeContainer.style.overflow = "hidden"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Account for horizontal crop in scroll-mode page height.
applyPdfCrop() scales the iframe by 100 / (100 - left - right), but this container height only applies top/bottom. A left/right crop will scale the page taller while overflow: hidden clips the top/bottom content.
Proposed fix
- const cropRatio = (100 - pdfCrop.bottom - pdfCrop.top) / 100;
+ const visibleHeightRatio = (100 - pdfCrop.bottom - pdfCrop.top) / 100;
+ const visibleWidthRatio = (100 - pdfCrop.left - pdfCrop.right) / 100;
- iframeContainer.style.paddingTop = `${(1 / aspectRatio) * cropRatio * 100}%`;
+ iframeContainer.style.paddingTop = `${(1 / aspectRatio) * (visibleHeightRatio / visibleWidthRatio) * 100}%`;📝 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.
| const cropRatio = (100 - pdfCrop.bottom - pdfCrop.top) / 100; | |
| iframeContainer.style.paddingTop = `${(1 / aspectRatio) * cropRatio * 100}%`; | |
| iframeContainer.style.marginBottom = `2%`; | |
| iframeContainer.style.overflow = "hidden"; | |
| const visibleHeightRatio = (100 - pdfCrop.bottom - pdfCrop.top) / 100; | |
| const visibleWidthRatio = (100 - pdfCrop.left - pdfCrop.right) / 100; | |
| iframeContainer.style.paddingTop = `${(1 / aspectRatio) * (visibleHeightRatio / visibleWidthRatio) * 100}%`; | |
| iframeContainer.style.marginBottom = `2%`; | |
| iframeContainer.style.overflow = "hidden"; |
🤖 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/utils/pdfUtil.ts` around lines 65 - 69, `applyPdfCrop()` only adjusts the
scroll-mode container height for top/bottom crop, so left/right crop is ignored
even though the iframe is horizontally scaled. Update the height calculation in
`pdfUtil.ts` to account for `pdfCrop.left` and `pdfCrop.right` alongside
`pdfCrop.top`/`bottom`, and use the same crop-aware scaling logic already
applied to the iframe so the visible page height stays correct when `overflow:
hidden` is enabled.
| }, 1000); | ||
| } | ||
| } | ||
| playMimicalFlip(animation, isMobile, delta, flipToNextPage, flipToPrevPage); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return when the mimical flip handler consumes the page turn.
playMimicalFlip() returns true when it handles the animation. Ignoring that makes iOS continue into the immediate scroll path, causing the page to move underneath the mimical flip.
Proposed fix
- playMimicalFlip(animation, isMobile, delta, flipToNextPage, flipToPrevPage);
+ if (
+ playMimicalFlip(animation, isMobile, delta, flipToNextPage, flipToPrevPage)
+ ) {
+ return;
+ }📝 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.
| playMimicalFlip(animation, isMobile, delta, flipToNextPage, flipToPrevPage); | |
| if ( | |
| playMimicalFlip(animation, isMobile, delta, flipToNextPage, flipToPrevPage) | |
| ) { | |
| return; | |
| } |
🤖 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/utils/pdfUtil.ts` at line 296, The mimical flip handler result is being
ignored in pdfUtil’s page-turn flow, so iOS can fall through into the immediate
scroll path after the animation is already handled. Update the page-turn logic
around playMimicalFlip() to check its return value and exit early when it
returns true, keeping the rest of the scroll/page-move code from running in that
case.
| const noteCorner = (corner: Corner | null) => { | ||
| if (isAutoTurning) return; | ||
| if (corner) { | ||
| if (engagedCorner !== corner) { | ||
| engagedCorner = corner; | ||
| armDwell(corner); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restart the dwell timer when the engaged corner changes.
If the pointer moves from one corner to the other before the first 500ms timer fires, armDwell() returns because a timer already exists; the old timer then exits, and the new corner never gets armed.
Suggested fix
if (corner) {
if (engagedCorner !== corner) {
+ if (autoTurnTimer) {
+ clearTimeout(autoTurnTimer);
+ autoTurnTimer = null;
+ }
engagedCorner = corner;
armDwell(corner);
}📝 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.
| const noteCorner = (corner: Corner | null) => { | |
| if (isAutoTurning) return; | |
| if (corner) { | |
| if (engagedCorner !== corner) { | |
| engagedCorner = corner; | |
| armDwell(corner); | |
| } | |
| const noteCorner = (corner: Corner | null) => { | |
| if (isAutoTurning) return; | |
| if (corner) { | |
| if (engagedCorner !== corner) { | |
| if (autoTurnTimer) { | |
| clearTimeout(autoTurnTimer); | |
| autoTurnTimer = null; | |
| } | |
| engagedCorner = corner; | |
| armDwell(corner); | |
| } |
🤖 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/utils/selectionAutoTurn.ts` around lines 264 - 270, The dwell timer in
noteCorner is not restarted when engagedCorner changes, so a pending timer from
the previous corner can block the new corner from arming. Update the
selectionAutoTurn flow around noteCorner and armDwell so that switching corners
clears or replaces any existing dwell timer before starting a new one, ensuring
the latest corner always gets armed.
| const regex = | ||
| rule.matchType === "regex" | ||
| ? new RegExp(rule.pattern, "g") | ||
| : new RegExp( | ||
| rule.pattern.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"), | ||
| "g" | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== textRuleUtil ==\n'
sed -n '1,240p' src/utils/textRuleUtil.ts
printf '\n== TextRule references ==\n'
rg -n "type TextRule|interface TextRule|TextRule" src --glob '!**/node_modules/**' | sed -n '1,220p'Repository: koodo-reader/kookit
Length of output: 5342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== GeneralRender.ts excerpt ==\n'
sed -n '1,180p' src/renders/GeneralRender.ts
printf '\n== layoutUtil.ts excerpt around applyTextRules ==\n'
sed -n '420,480p' src/utils/layoutUtil.ts
printf '\n== storage / settings references for textRules ==\n'
rg -n "textRules|bookKey|scope: \"book\"|scope: \"all\"" src --glob '!**/node_modules/**' | sed -n '1,260p'Repository: koodo-reader/kookit
Length of output: 8502
Guard regex rules before applying them.
new RegExp(rule.pattern, "g")can throw on a bad saved rule and stop rendering; pathological regexes can also stall the render thread.scope: "book"rules are applied unconditionally here becausebookKeyis never checked.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 137-137: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(rule.pattern, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 137-137: Do not use variable for regular expressions
Context: new RegExp(rule.pattern, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
[warning] 138-141: Do not use variable for regular expressions
Context: new RegExp(
rule.pattern.replace(/[.*+?^${}()|[]\]/g, "\$&"),
"g"
)
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
🤖 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/utils/textRuleUtil.ts` around lines 136 - 142, The regex construction in
textRuleUtil is unsafe because `new RegExp(rule.pattern, "g")` can throw or
create pathological patterns, and `scope: "book"` rules are currently applied
without checking `bookKey`. Update the rule handling around the regex creation
block to validate/guard `rule.pattern` before instantiating `RegExp`, skip or
safely reject invalid patterns, and add a `bookKey` check so book-scoped rules
only apply when the current book matches the rule scope.
Source: Linters/SAST tools
| export const applyTextRules = (doc: Document, rules: TextRule[]) => { | ||
| clearTextRules(doc); | ||
| for (let i = 0; i < rules.length; i++) { | ||
| const rule = rules[i]; | ||
| if (rule.scope === "all" || rule.scope === "book") { | ||
| applyRule(doc, rule); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor book scope before applying rules.
Line 156 applies every "book" rule to the current document, but no current bookKey is checked. This can leak book-specific replace/delete rules into unrelated books.
Suggested contract fix
-export const applyTextRules = (doc: Document, rules: TextRule[]) => {
+export const applyTextRules = (
+ doc: Document,
+ rules: TextRule[],
+ currentBookKey: string = ""
+) => {
clearTextRules(doc);
for (let i = 0; i < rules.length; i++) {
const rule = rules[i];
- if (rule.scope === "all" || rule.scope === "book") {
+ if (
+ rule.scope === "all" ||
+ (rule.scope === "book" && rule.bookKey === currentBookKey)
+ ) {
applyRule(doc, rule);
}
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const applyTextRules = (doc: Document, rules: TextRule[]) => { | |
| clearTextRules(doc); | |
| for (let i = 0; i < rules.length; i++) { | |
| const rule = rules[i]; | |
| if (rule.scope === "all" || rule.scope === "book") { | |
| applyRule(doc, rule); | |
| } | |
| export const applyTextRules = ( | |
| doc: Document, | |
| rules: TextRule[], | |
| currentBookKey: string = "" | |
| ) => { | |
| clearTextRules(doc); | |
| for (let i = 0; i < rules.length; i++) { | |
| const rule = rules[i]; | |
| if ( | |
| rule.scope === "all" || | |
| (rule.scope === "book" && rule.bookKey === currentBookKey) | |
| ) { | |
| applyRule(doc, rule); | |
| } |
🤖 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/utils/textRuleUtil.ts` around lines 152 - 158, The applyTextRules flow is
applying every rule with scope "book" through applyRule without verifying the
current document belongs to that book, which can leak book-specific changes.
Update applyTextRules in textRuleUtil to honor the active book context by
checking the document’s current bookKey before applying any "book" scoped rule,
and only call applyRule for matching book rules; keep "all" rules unaffected.
1 participant
Summary by CodeRabbit