Conversation
…e api-extractor configuration; add LICENSE file
- Introduced a new DjVu library for handling DjVu files. - Implemented the `makeDJVU` function to create a DjVu book object with metadata and table of contents. - Added `DjvuRender` class to manage rendering of DjVu documents, including handling scrolling and chapter navigation. - Enhanced PDF rendering logic to accommodate DjVu pages, including caching and rendering optimizations. - Updated existing PDF rendering to support new parameters for page rendering.
…PageToImage compatibility
…mance and memory management
…pter handling logic
…improved performance
…for improved visibility
…n-scroll reader modes
…clarity in OCRCacheUtil
Contributor
|
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 AGPLv3 license and expanded README; removes api-extractor config and adjusts Rollup; introduces bionic-reading DOM processing and bionic flag; adds fake OCR progress and OCR/init flow changes; expands note highlighting (mobile-aware icons, delegation); minor DOM batching and logging tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Reader UI
participant Render as PdfTextRender
participant OCR as OCR Worker (Tesseract/Paddle)
participant DOM as Document / bionicUtil
participant Notes as noteUtil
UI->>Render: open PDF / request text
Render->>OCR: startFakeProgress() + load models
OCR-->>Render: models ready
Render->>Render: parse pages, makePDF()
Render->>DOM: render text into iframe(s)
alt window.isBionic === "yes"
DOM->>DOM: processDocumentBody() (HalfBold)
end
UI->>Notes: user selects text / adds note
Notes->>DOM: highlightRange(..., isNote, isMobile)
Notes->>UI: add note icon (touch/click handlers)
Render->>UI: progress updates (fake -> complete)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the rendering core to “2.3.0”, adding bionic-reading text processing, enhancing highlight rendering (notes + mobile behavior), and improving OCR initialization UX/logging.
Changes:
- Add bionic-reading support via a new DOM post-processor and a new
isBionicrender config flag. - Extend highlight rendering to support note icons and mobile/swipe behavior for EPUB + PDF highlights.
- Improve OCR resource caching/logging and add “fake progress” during OCR engine initialization; broaden parsing behavior based on
extension.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/pdfUtil.ts | Optimize PDF container DOM insertion using a DocumentFragment. |
| src/utils/ocrCacheUtil.ts | Switch cache/network logs from console.log to console.info. |
| src/utils/noteUtil.ts | Add note icon rendering + mobile/swipe handling to highlights; extend highlight APIs. |
| src/utils/layoutUtil.ts | Invoke bionic processing during text transform; refine image sizing constraints. |
| src/utils/bionicUtil.ts | New utility that half-bolds words by rewriting text nodes into <b> + text nodes. |
| src/renders/PdfTextRender.ts | Add extension handling and fake progress during OCR engine init; adjust external OCR calls. |
| src/renders/PdfRender.ts | Pass note/mobile flags into PDF highlight rendering. |
| src/renders/GeneralRender.ts | Add isBionic config and propagate highlight flags for note/mobile behavior. |
| src/libs/textProcessor.ts | Refactor startWithJUAN checks to reuse a shared regexp and extend patterns. |
| src/libs/pdf.js | Update renderPage invocation to pass an explicit false flag. |
| src/libs/epub.js | Improve URL normalization and cover detection; adjust cover loading logic. |
| rollup.config.js | Reformat config and change output paths for desktop/mobile builds. |
| README.md | Replace build notes with a proper README (intro/build/deps/license). |
| api-extractor.json | Remove API Extractor configuration. |
| LICENSE | Add GNU AGPL v3 license text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/utils/noteUtil.ts
Outdated
Comment on lines
+352
to
+353
| "px; z-index:1; " + | ||
| (isMobile ? "pointer-events: none;" : "") |
There was a problem hiding this comment.
On mobile, pointer-events: none on clickNode makes the click/ontouchend listeners below ineffective (the element won't receive pointer/touch events). If the goal is to avoid interfering with swipe, consider leaving pointer events enabled and relying on the window.isSwiping early-return in the handler, or conditionally disabling only during an active swipe.
Suggested change
| "px; z-index:1; " + | |
| (isMobile ? "pointer-events: none;" : "") | |
| "px; z-index:1; " |
Comment on lines
+69
to
+70
| var collection = element.body.getElementsByTagName("*"); | ||
| for (var i = 0; collection[i] !== undefined; i++) { |
There was a problem hiding this comment.
getElementsByTagName("*") returns a live HTMLCollection. This loop mutates the DOM by inserting <b> nodes while iterating the live collection, which can cause elements to be skipped or processed unpredictably. Consider iterating a static snapshot (e.g., querySelectorAll("*") / Array.from(...)) before mutating, or traverse via a TreeWalker.
Suggested change
| var collection = element.body.getElementsByTagName("*"); | |
| for (var i = 0; collection[i] !== undefined; i++) { | |
| const collection = element.body.querySelectorAll("*"); | |
| for (let i = 0; i < collection.length; i++) { |
Comment on lines
472
to
+486
| async parse() { | ||
| try { | ||
| // 安装 fetch 拦截器以自动缓存所有 OCR 相关资源 | ||
| if (this.isScannedPDF === "yes") { | ||
| ocrCache.installGlobalFetchInterceptor(); | ||
| } | ||
|
|
||
| let blob = new Blob([this.pdfBuffer]); | ||
| let file = new File([blob], "book", { | ||
| lastModified: new Date().getTime(), | ||
| type: blob.type, | ||
| }); | ||
| if (await isPDF(file)) { | ||
| this.book = await makePDF(file, this.password); | ||
| } | ||
|
|
||
| this.book = await makePDF(file, this.password); | ||
|
|
There was a problem hiding this comment.
renderTo() introduces a non-PDF path when extension !== "pdf" (parsing via GeneralParser), but parse() now unconditionally calls makePDF(...). This will fail for non-PDF inputs and makes the extension !== "pdf" branch unreachable/buggy. Either restore the PDF check/guard here or route non-PDF parsing through the appropriate loader based on extension.
Comment on lines
8
to
15
| const getDesktopOutputPath = (filename) => { | ||
| const basePath = process.platform === 'win32' | ||
| ? 'D:\\Project\\koodo-reader' | ||
| : '/Users/troyeguo/Documents/Project/koodo-reader'; | ||
| return path.join(basePath, 'src', 'assets', 'lib', filename); | ||
| const basePath = "D:\\Project\\koodo-reader"; | ||
| return path.join(basePath, "src", "assets", "lib", filename); | ||
| }; | ||
| const getMobileOutputPath = (filename) => { | ||
| const basePath = process.platform === 'win32' | ||
| ? 'D:\\Project\\koodo-reader-expo' | ||
| : '/Users/troyeguo/Documents/Project/koodo-reader-expo'; | ||
| return path.join(basePath, 'assets', 'lib', filename); | ||
| const basePath = "D:\\Project\\koodo-reader-expo"; | ||
| return path.join(basePath, "assets", "lib", filename); | ||
| }; |
There was a problem hiding this comment.
getDesktopOutputPath / getMobileOutputPath are now hard-coded to D:\\Project\\..., which will break builds on non-Windows machines and on CI environments where those paths don't exist. Consider deriving these from environment variables or outputting to dist/ (or a repo-relative path) instead of a developer-specific absolute path.
src/utils/noteUtil.ts
Outdated
Comment on lines
+154
to
+155
| "; " + | ||
| (isMobile ? "pointer-events: none;" : "") |
There was a problem hiding this comment.
pointer-events: none on mobile will prevent the attached click/ontouchend handlers on this highlight node from ever firing, making highlights non-interactive on mobile. If the intent is to avoid blocking swipe gestures, consider keeping pointer events enabled and relying on the existing window.isSwiping guard, or only disabling pointer events when !isNote (so note highlights remain tappable).
Suggested change
| "; " + | |
| (isMobile ? "pointer-events: none;" : "") | |
| ";" |
src/utils/noteUtil.ts
Outdated
| "position: absolute;" + | ||
| "left:" + | ||
| (Math.min(rect.left, rect.x) + | ||
| doc.body.scrollLeft + |
There was a problem hiding this comment.
iconNode positioning uses doc.body.scrollLeft, but earlier in this function you compute scrollLeft as doc.body.scrollLeft || doc.documentElement.scrollLeft. Using doc.body.scrollLeft here can misplace the icon in browsers where scrolling is on documentElement. Use the same scrollLeft variable for consistency.
Suggested change
| doc.body.scrollLeft + | |
| scrollLeft + |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
src/utils/noteUtil.ts (1)
389-392: Inconsistent scroll position retrieval.Line 390 uses
doc.body.scrollLeftdirectly, while line 272 uses the fallback patterndoc.body.scrollLeft || doc.documentElement.scrollLeft. For consistency and cross-browser safety, use the same pattern here.♻️ Suggested fix
iconNode.setAttribute( "style", "position: absolute;" + "left:" + (Math.min(rect.left, rect.x) + - doc.body.scrollLeft + + scrollLeft + rect.width - 18) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 389 - 392, The expression computing the note X coordinate uses doc.body.scrollLeft directly (in the arithmetic around Math.min(rect.left, rect.x) + doc.body.scrollLeft + rect.width - 18), which is inconsistent with the fallback pattern used elsewhere (doc.body.scrollLeft || doc.documentElement.scrollLeft); update that occurrence to use the same fallback (doc.body.scrollLeft || doc.documentElement.scrollLeft) so cross-browser scroll position is retrieved consistently when computing the position in the function that contains this Math.min(...) expression in src/utils/noteUtil.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 1: Remove the UTF-8 BOM from the start of the README by editing the first
line (the "## Introduction" heading) to ensure it begins with a plain ASCII '#'
character; open the file in a UTF-8 aware editor or save without BOM and
re-save/write the file so the invisible BOM character is removed, then re-run
linters to verify the static-analysis warning is gone.
- Line 37: Replace the hardcoded dev-branch URL in the README sentence "This
library is distributed under the terms of
https://github.com/koodo-reader/kookit/blob/dev/LICENSE" with a stable
repo-relative link (e.g. ./LICENSE or /LICENSE) so the LICENSE reference always
points to the repository's LICENSE file regardless of branch; update the line
containing that sentence in README.md to use the relative path.
In `@rollup.config.js`:
- Around line 9-14: The current getOutputPath and getMobileOutputPath functions
use hardcoded Windows absolute paths; change them to compute project-relative,
portable output locations (e.g., based on __dirname or process.cwd()) so builds
write into the package's dist/assets tree instead of local machine folders;
update getOutputPath to return a path under the package's dist (matching
declared entrypoints like dist/kookit.cjs.js, dist/kookit.esm.js) and update
getMobileOutputPath to return a path inside the package (e.g., dist or
assets/lib) so published consumers can find files and CI builds are portable;
modify the functions getOutputPath and getMobileOutputPath accordingly.
In `@src/libs/epub.js`:
- Around line 517-528: The manifest cover-selection logic can throw when an item
has a null mediaType; in the two searches for this.cover (the first find using
item.href and item.mediaType.startsWith and the fallback find using
item.mediaType.startsWith), add a null-safe check (e.g., optional chaining or
explicit typeof/string check) before calling startsWith on item.mediaType so
items with no media-type won't cause a TypeError; update the expressions that
reference item.mediaType in the two find callbacks to guard against
null/undefined.
- Around line 1010-1016: In getCover(), you're loading the cover blob twice
(once into coverBlob to check size and again when creating the Blob) which
wastes I/O; change the return branch to reuse the already-loaded coverBlob (use
coverBlob as the source for new Blob and keep cover.mediaType) and keep the same
null/size checks (i.e., if cover?.href && coverBlob && coverBlob.size > 0 then
return new Blob([coverBlob], { type: cover.mediaType }) else return null) so you
avoid calling loadBlob(cover.href) a second time.
In `@src/renders/PdfTextRender.ts`:
- Around line 78-85: The parse() path for non-PDF extensions currently calls
makePDF(file, this.password) unconditionally, which will break on non-PDF
buffers; modify PdfTextRender.parse to check the file buffer with the existing
isPDF(buffer) (or an isPDF() helper) before calling makePDF, and only invoke
makePDF/any PDF-specific logic (PDFDataRangeTransport / pdfjsLib.getDocument)
when isPDF returns true; for non-PDF cases skip makePDF and proceed with the
existing non-PDF parsing flow (or throw a clear error), ensuring makePDF is only
called from the PDF-guarded branch.
In `@src/utils/bionicUtil.ts`:
- Around line 37-38: Replace usage of innerHTML on the created <b> element to
prevent HTML injection: in the function that creates the bold element (where
const bold = document.createElement("b") and bold.innerHTML = word.slice(0,
length) are used), set bold.textContent = word.slice(0, length) instead of
assigning to innerHTML; do the same for any other DOM nodes in this module that
currently use innerHTML to insert user-supplied text (e.g., the element created
alongside bold), ensuring all text is inserted via textContent to avoid
parsing/exec of markup.
In `@src/utils/layoutUtil.ts`:
- Around line 280-282: transformText can run multiple times and calling
processDocumentBody(doc) repeatedly mutates nodes; make bionic processing
idempotent by skipping already-processed documents: before calling
processDocumentBody(doc) (where window.isBionic is checked), detect a processing
marker (e.g., a module-level WeakSet<Document> or a DOM flag like
document.documentElement.dataset.bionicProcessed) and return early if present,
and after successful processing add that marker; update references in
transformText/processDocumentBody to honor the marker so nodes are only
transformed once.
In `@src/utils/noteUtil.ts`:
- Around line 250-268: The recursive requestAnimationFrame loop in
waitAndHighlight (triggered when isMobile && window.isSwiping) lacks a safeguard
and can run forever; modify waitAndHighlight to track attempts (e.g., a retry
counter or timestamp) and stop retrying after a configurable maxRetries or
timeout, then call highlightRange fallback or abort gracefully; update the logic
around requestAnimationFrame(waitAndHighlight) and the waitAndHighlight closure
to reference window.isSwiping, maxRetries/timeout, and highlightRange so the
loop exits reliably even if window.isSwiping never becomes false.
---
Nitpick comments:
In `@src/utils/noteUtil.ts`:
- Around line 389-392: The expression computing the note X coordinate uses
doc.body.scrollLeft directly (in the arithmetic around Math.min(rect.left,
rect.x) + doc.body.scrollLeft + rect.width - 18), which is inconsistent with the
fallback pattern used elsewhere (doc.body.scrollLeft ||
doc.documentElement.scrollLeft); update that occurrence to use the same fallback
(doc.body.scrollLeft || doc.documentElement.scrollLeft) so cross-browser scroll
position is retrieved consistently when computing the position in the function
that contains this Math.min(...) expression in src/utils/noteUtil.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1794f906-6dda-4f00-b787-55535ca54749
📒 Files selected for processing (15)
LICENSEREADME.mdapi-extractor.jsonrollup.config.jssrc/libs/epub.jssrc/libs/pdf.jssrc/libs/textProcessor.tssrc/renders/GeneralRender.tssrc/renders/PdfRender.tssrc/renders/PdfTextRender.tssrc/utils/bionicUtil.tssrc/utils/layoutUtil.tssrc/utils/noteUtil.tssrc/utils/ocrCacheUtil.tssrc/utils/pdfUtil.ts
💤 Files with no reviewable changes (1)
- api-extractor.json
| @@ -1,5 +1,37 @@ | |||
| //打包 ES5 兼容模块 | |||
| ## Introduction | |||
Contributor
There was a problem hiding this comment.
Remove the UTF-8 BOM from the first line.
There’s an invisible BOM character before the heading, which can trip linters/parsers and likely explains the static-analysis warning.
🧹 Proposed fix
-## Introduction
+## Introduction📝 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
| ## Introduction | |
| ## Introduction |
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: Ensure spelling is correct
Context: ## Introduction This library is built with TypeScript an...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 1, Remove the UTF-8 BOM from the start of the README by
editing the first line (the "## Introduction" heading) to ensure it begins with
a plain ASCII '#' character; open the file in a UTF-8 aware editor or save
without BOM and re-save/write the file so the invisible BOM character is
removed, then re-run linters to verify the static-analysis warning is gone.
|
|
||
| ## License | ||
|
|
||
| This library is distributed under the terms of [GNU AGPL v3](https://github.com/koodo-reader/kookit/blob/dev/LICENSE) |
Contributor
There was a problem hiding this comment.
Use a stable relative link for the local LICENSE file.
The current URL is pinned to dev, which can become stale. Prefer a repo-relative link.
🔗 Proposed fix
-This library is distributed under the terms of [GNU AGPL v3](https://github.com/koodo-reader/kookit/blob/dev/LICENSE)
+This library is distributed under the terms of [GNU AGPL v3](./LICENSE)📝 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
| This library is distributed under the terms of [GNU AGPL v3](https://github.com/koodo-reader/kookit/blob/dev/LICENSE) | |
| This library is distributed under the terms of [GNU AGPL v3](./LICENSE) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 37, Replace the hardcoded dev-branch URL in the README
sentence "This library is distributed under the terms of
https://github.com/koodo-reader/kookit/blob/dev/LICENSE" with a stable
repo-relative link (e.g. ./LICENSE or /LICENSE) so the LICENSE reference always
points to the repository's LICENSE file regardless of branch; update the line
containing that sentence in README.md to use the relative path.
Comment on lines
+9
to
+14
| const basePath = "D:\\Project\\koodo-reader"; | ||
| return path.join(basePath, "src", "assets", "lib", filename); | ||
| }; | ||
| const getMobileOutputPath = (filename) => { | ||
| const basePath = process.platform === 'win32' | ||
| ? 'D:\\Project\\koodo-reader-expo' | ||
| : '/Users/troyeguo/Documents/Project/koodo-reader-expo'; | ||
| return path.join(basePath, 'assets', 'lib', filename); | ||
| const basePath = "D:\\Project\\koodo-reader-expo"; | ||
| return path.join(basePath, "assets", "lib", filename); |
Contributor
There was a problem hiding this comment.
Build output paths are hardcoded to local Windows directories (release blocker).
Using absolute local paths here makes builds non-portable and writes artifacts outside the package. It also conflicts with declared package entrypoints (dist/kookit.cjs.js, dist/kookit.esm.js, etc.), so published consumers won’t find expected files.
🔧 Proposed fix
const getDesktopOutputPath = (filename) => {
- const basePath = "D:\\Project\\koodo-reader";
- return path.join(basePath, "src", "assets", "lib", filename);
+ return path.join("dist", filename);
};
const getMobileOutputPath = (filename) => {
- const basePath = "D:\\Project\\koodo-reader-expo";
- return path.join(basePath, "assets", "lib", filename);
+ return path.join("dist", filename);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const basePath = "D:\\Project\\koodo-reader"; | |
| return path.join(basePath, "src", "assets", "lib", filename); | |
| }; | |
| const getMobileOutputPath = (filename) => { | |
| const basePath = process.platform === 'win32' | |
| ? 'D:\\Project\\koodo-reader-expo' | |
| : '/Users/troyeguo/Documents/Project/koodo-reader-expo'; | |
| return path.join(basePath, 'assets', 'lib', filename); | |
| const basePath = "D:\\Project\\koodo-reader-expo"; | |
| return path.join(basePath, "assets", "lib", filename); | |
| const getDesktopOutputPath = (filename) => { | |
| return path.join("dist", filename); | |
| }; | |
| const getMobileOutputPath = (filename) => { | |
| return path.join("dist", filename); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rollup.config.js` around lines 9 - 14, The current getOutputPath and
getMobileOutputPath functions use hardcoded Windows absolute paths; change them
to compute project-relative, portable output locations (e.g., based on __dirname
or process.cwd()) so builds write into the package's dist/assets tree instead of
local machine folders; update getOutputPath to return a path under the package's
dist (matching declared entrypoints like dist/kookit.cjs.js, dist/kookit.esm.js)
and update getMobileOutputPath to return a path inside the package (e.g., dist
or assets/lib) so published consumers can find files and CI builds are portable;
modify the functions getOutputPath and getMobileOutputPath accordingly.
src/renders/PdfTextRender.ts
Outdated
Comment on lines
+78
to
+85
| } else { | ||
| if (!this.book) { | ||
| await this.parse(); | ||
| } | ||
| let parser = new GeneralParser(this.book); | ||
| this.chapterList = await parser.getChapter(this.book.toc); | ||
| this.chapterDocList = await parser.getChapterDoc(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examine PdfTextRender.ts to understand the parse() method and extension handling
echo "=== Checking parse() method implementation ==="
rg -n "parse\(\)" src/renders/PdfTextRender.ts -A 5
echo ""
echo "=== Checking if makePDF is called in parse() ==="
rg -n "makePDF" src/renders/PdfTextRender.ts -B 3 -A 3
echo ""
echo "=== Checking isPDF function and its usage ==="
rg -n "isPDF" src/renders/PdfTextRender.ts -B 2 -A 2
echo ""
echo "=== Searching for isPDF definition across entire codebase ==="
rg -n "isPDF" --type ts -B 2 -A 3
echo ""
echo "=== Checking the extension property and branch logic ==="
rg -n "extension" src/renders/PdfTextRender.ts -B 1 -A 1 | head -40Repository: koodo-reader/kookit
Length of output: 4930
🏁 Script executed:
#!/bin/bash
# Get the complete renderTo method to see the full branching logic
echo "=== Complete renderTo method (lines 50-100) ==="
sed -n '50,100p' src/renders/PdfTextRender.ts | cat -n
echo ""
echo "=== Check the libs/pdf file for isPDF and makePDF ==="
fd "pdf" src/libs --type f
echo ""
echo "=== Get isPDF and makePDF definitions ==="
rg -n "export.*isPDF|export.*makePDF" src/libs --type ts -A 8Repository: koodo-reader/kookit
Length of output: 2341
🏁 Script executed:
#!/bin/bash
# Check the pdf.js file since the search didn't return TS definitions
echo "=== Content of src/libs/pdf.js ==="
cat src/libs/pdf.js | head -100
echo ""
echo "=== Check full parse() method ==="
sed -n '472,510p' src/renders/PdfTextRender.ts | cat -n
echo ""
echo "=== Check if there's any isPDF guard before parse() is called ==="
sed -n '40,95p' src/renders/PdfTextRender.ts | cat -nRepository: koodo-reader/kookit
Length of output: 7595
🏁 Script executed:
#!/bin/bash
# Search for makePDF and isPDF definitions in pdf.js
echo "=== Search for isPDF and makePDF in pdf.js ==="
rg -n "isPDF|makePDF" src/libs/pdf.js -A 15
echo ""
echo "=== Get file size to understand scope ==="
wc -l src/libs/pdf.js
echo ""
echo "=== Check if there are any guards in parse before calling makePDF ==="
sed -n '472,495p' src/renders/PdfTextRender.ts | cat -nRepository: koodo-reader/kookit
Length of output: 2012
Non-PDF files will fail when calling makePDF in the parse() method.
The code path for extension !== "pdf" calls this.parse() unconditionally, which in turn calls makePDF(file, this.password) without any safeguard. However, makePDF is designed specifically for PDF files—it creates a PDFDataRangeTransport and calls pdfjsLib.getDocument(), which expects valid PDF structure (magic bytes %PDF). Non-PDF files (EPUB, TXT, images) will cause runtime errors.
An isPDF() function exists in the codebase (checks for PDF magic bytes), but it is not being used as a guard in the parse() method before calling makePDF. This allows non-PDF buffers to be passed to PDF-specific processing code, causing failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renders/PdfTextRender.ts` around lines 78 - 85, The parse() path for
non-PDF extensions currently calls makePDF(file, this.password) unconditionally,
which will break on non-PDF buffers; modify PdfTextRender.parse to check the
file buffer with the existing isPDF(buffer) (or an isPDF() helper) before
calling makePDF, and only invoke makePDF/any PDF-specific logic
(PDFDataRangeTransport / pdfjsLib.getDocument) when isPDF returns true; for
non-PDF cases skip makePDF and proceed with the existing non-PDF parsing flow
(or throw a clear error), ensuring makePDF is only called from the PDF-guarded
branch.
Comment on lines
+280
to
+282
| if (window.isBionic === "yes") { | ||
| processDocumentBody(doc); | ||
| } |
Contributor
There was a problem hiding this comment.
Make bionic processing idempotent per document.
tranformText may run multiple times during re-layout, so Line 281 can repeatedly mutate already-processed nodes and progressively distort text/DOM.
💡 Proposed fix
- if (window.isBionic === "yes") {
- processDocumentBody(doc);
- }
+ if (
+ window.isBionic === "yes" &&
+ !doc.body?.hasAttribute("data-kookit-bionic-processed")
+ ) {
+ processDocumentBody(doc);
+ doc.body?.setAttribute("data-kookit-bionic-processed", "true");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (window.isBionic === "yes") { | |
| processDocumentBody(doc); | |
| } | |
| if ( | |
| window.isBionic === "yes" && | |
| !doc.body?.hasAttribute("data-kookit-bionic-processed") | |
| ) { | |
| processDocumentBody(doc); | |
| doc.body?.setAttribute("data-kookit-bionic-processed", "true"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/layoutUtil.ts` around lines 280 - 282, transformText can run
multiple times and calling processDocumentBody(doc) repeatedly mutates nodes;
make bionic processing idempotent by skipping already-processed documents:
before calling processDocumentBody(doc) (where window.isBionic is checked),
detect a processing marker (e.g., a module-level WeakSet<Document> or a DOM flag
like document.documentElement.dataset.bionicProcessed) and return early if
present, and after successful processing add that marker; update references in
transformText/processDocumentBody to honor the marker so nodes are only
transformed once.
Comment on lines
+250
to
+268
| if (isMobile && window.isSwiping) { | ||
| const waitAndHighlight = () => { | ||
| if (window.isSwiping) { | ||
| requestAnimationFrame(waitAndHighlight); | ||
| } else { | ||
| highlightRange( | ||
| range, | ||
| colorCode, | ||
| noteKey, | ||
| handleNoteClick, | ||
| doc, | ||
| isNote, | ||
| isMobile | ||
| ); | ||
| } | ||
| }; | ||
| requestAnimationFrame(waitAndHighlight); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Minor: Recursive RAF without timeout could spin indefinitely.
If window.isSwiping never becomes false due to a bug elsewhere, this recursive requestAnimationFrame loop will continue indefinitely, consuming resources. Consider adding a maximum retry count or timeout as a safeguard.
🛡️ Suggested safeguard
export const highlightRange = (
range: any,
colorCode: string,
noteKey: string,
handleNoteClick: any,
doc: any,
isNote: boolean = false,
isMobile: boolean = false
) => {
if (isMobile && window.isSwiping) {
+ let retryCount = 0;
+ const maxRetries = 100; // ~1.6 seconds at 60fps
const waitAndHighlight = () => {
- if (window.isSwiping) {
+ if (window.isSwiping && retryCount < maxRetries) {
+ retryCount++;
requestAnimationFrame(waitAndHighlight);
} else {
highlightRange(
range,
colorCode,
noteKey,
handleNoteClick,
doc,
isNote,
isMobile
);
}
};
requestAnimationFrame(waitAndHighlight);
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.
Suggested change
| if (isMobile && window.isSwiping) { | |
| const waitAndHighlight = () => { | |
| if (window.isSwiping) { | |
| requestAnimationFrame(waitAndHighlight); | |
| } else { | |
| highlightRange( | |
| range, | |
| colorCode, | |
| noteKey, | |
| handleNoteClick, | |
| doc, | |
| isNote, | |
| isMobile | |
| ); | |
| } | |
| }; | |
| requestAnimationFrame(waitAndHighlight); | |
| return; | |
| } | |
| if (isMobile && window.isSwiping) { | |
| let retryCount = 0; | |
| const maxRetries = 100; // ~1.6 seconds at 60fps | |
| const waitAndHighlight = () => { | |
| if (window.isSwiping && retryCount < maxRetries) { | |
| retryCount++; | |
| requestAnimationFrame(waitAndHighlight); | |
| } else { | |
| highlightRange( | |
| range, | |
| colorCode, | |
| noteKey, | |
| handleNoteClick, | |
| doc, | |
| isNote, | |
| isMobile | |
| ); | |
| } | |
| }; | |
| requestAnimationFrame(waitAndHighlight); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/noteUtil.ts` around lines 250 - 268, The recursive
requestAnimationFrame loop in waitAndHighlight (triggered when isMobile &&
window.isSwiping) lacks a safeguard and can run forever; modify waitAndHighlight
to track attempts (e.g., a retry counter or timestamp) and stop retrying after a
configurable maxRetries or timeout, then call highlightRange fallback or abort
gracefully; update the logic around requestAnimationFrame(waitAndHighlight) and
the waitAndHighlight closure to reference window.isSwiping, maxRetries/timeout,
and highlightRange so the loop exits reliably even if window.isSwiping never
becomes false.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/utils/noteUtil.ts (1)
250-268:⚠️ Potential issue | 🟡 MinorAdd a bailout to the swipe wait loop.
Line 250-Line 268 can loop forever if
window.isSwipingnever flips back, which can keep scheduling RAF indefinitely.🛡️ Proposed safeguard
if (isMobile && window.isSwiping) { + const start = performance.now(); + const timeoutMs = 2000; const waitAndHighlight = () => { if (window.isSwiping) { - requestAnimationFrame(waitAndHighlight); + if (performance.now() - start < timeoutMs) { + requestAnimationFrame(waitAndHighlight); + } } else { highlightRange( range, colorCode, noteKey, handleNoteClick, doc, isNote, isMobile ); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 250 - 268, The swim-wait loop using requestAnimationFrame in the isMobile && window.isSwiping branch (function waitAndHighlight) can run forever if window.isSwiping never becomes false; add a bailout: record a start time or frame counter when waitAndHighlight is first scheduled, and if a max elapsed time (e.g., 300–500ms) or max frames is exceeded, stop scheduling more RAFs and either call highlightRange immediately or abort gracefully; also capture the RAF id so you can call cancelAnimationFrame if you decide to bail early. Make the change inside the existing waitAndHighlight/requestAnimationFrame logic that references waitAndHighlight, window.isSwiping, highlightRange, and requestAnimationFrame.src/renders/PdfTextRender.ts (1)
473-473:⚠️ Potential issue | 🔴 CriticalGuard
makePDFwithisPDFbefore invoking PDF.js parsing.Line 473 unconditionally calls
makePDF. Insrc/libs/pdf.js(around Line 326-Line 396),makePDFgoes straight into PDF.js transport/getDocument flow, which will throw on non-PDF buffers.🔧 Proposed fix
- this.book = await makePDF(file, this.password); + if (!isPDF(file)) { + throw new Error("Invalid PDF input: expected PDF binary."); + } + this.book = await makePDF(file, this.password);#!/bin/bash # Verify current guard behavior and helper contracts. rg -n "export const isPDF|export const makePDF|PDFDataRangeTransport|getDocument" src/libs/pdf.js -C3 rg -n "async parse\\(|makePDF\\(|isPDF\\(" src/renders/PdfTextRender.ts -C4 rg -nP --type=ts -C2 "new\\s+PdfTextRender\\("Expected result:
parse()should checkisPDF(...)beforemakePDF(...), or enforce that only guaranteed-PDF buffers can reach this class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renders/PdfTextRender.ts` at line 473, The code calls makePDF(...) unconditionally in PdfTextRender (eg. this.book = await makePDF(file, this.password);) which can throw for non-PDF data; update the parse() (or the constructor path that invokes makePDF) to first call isPDF(file) and only call makePDF when isPDF returns true, otherwise handle the non-PDF case (reject/throw a clear error or return gracefully) so PDF.js transport/getDocument is never invoked on non-PDF buffers; reference the isPDF and makePDF symbols and modify the PdfTextRender.parse (or where makePDF is called) to implement this guard.
🧹 Nitpick comments (2)
src/utils/bionicUtil.ts (2)
9-54: Consider usinglet/constinstead ofvarfor block scoping.The function uses
varthroughout (lines 11, 17, 18, 25, 34), which has function scope and can lead to unexpected behavior. Modern TypeScript/JavaScript prefersletfor mutable variables andconstfor constants.♻️ Proposed refactor
const HalfBold = (parentElement) => { /* iterating through all children of the parent*/ - for (var i = 0; parentElement.childNodes[i] !== undefined; i++) { + for (let i = 0; parentElement.childNodes[i] !== undefined; i++) { /* if the child is a text element*/ if ( parentElement.childNodes[i].nodeName === "#text" && parentElement.childNodes[i].textContent.trim().length !== 0 ) { - var recentNode = parentElement.childNodes[i]; - var newNodeCount = 0; + let recentNode = parentElement.childNodes[i]; + let newNodeCount = 0; /* add bold and non-bold elements*/ parentElement.childNodes[i].textContent .split(/(\s+|\S+)/) // eslint-disable-next-line .forEach((word) => { if (word.length === 0) return; - var trimmedWordLength = word.trim().length; + const trimmedWordLength = word.trim().length; if (trimmedWordLength === 0) { - let textNode = document.createTextNode(word); + const textNode = document.createTextNode(word); insertAfter(textNode, recentNode); newNodeCount++; recentNode = textNode; return; } - var length = Math.floor(trimmedWordLength / 2); + let length = Math.floor(trimmedWordLength / 2); if (length === 0) length = 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/bionicUtil.ts` around lines 9 - 54, The HalfBold function uses var for function-scoped variables; update declarations to modern block-scoped keywords: change the loop index to let i, replace var recentNode and var newNodeCount with let recentNode and let newNodeCount, make trimmedWordLength a const, make length a let (since it's reassigned), and use const for bold and use let (or const when appropriate) for textNode inside the forEach callback; ensure the insertAfter and parentElement.removeChild logic remains unchanged and keep the eslint-disable comment if needed.
56-63: Consider addingCODE,PRE, andTEXTAREAto ignored tags.Code blocks and preformatted text typically shouldn't have bionic reading applied, as it would break code readability and formatting. Similarly, textarea content manipulation can cause issues.
♻️ Proposed addition
var ignoreTags = { B: true, META: true, LINK: true, SCRIPT: true, STYLE: true, + CODE: true, + PRE: true, + TEXTAREA: true, + NOSCRIPT: true, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/bionicUtil.ts` around lines 56 - 63, Update the ignoreTags map in bionicUtil.ts to also include CODE, PRE, and TEXTAREA so code blocks and preformatted/textarea content are skipped; locate the ignoreTags variable and add entries for "CODE": true, "PRE": true, and "TEXTAREA": true so functions that check ignoreTags (e.g., the bionic processing routine referencing ignoreTags) will bypass those nodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renders/PdfTextRender.ts`:
- Around line 512-514: The finally block is forcing progress to 100% even when
initialization failed; modify the flow so stopInitProgress() and
showOCRProgress(1) (calls found near stopInitProgress and showOCRProgress in
PdfTextRender) are only invoked when initialization succeeds—e.g., introduce a
boolean flag (initSucceeded) set to true on successful init or move the
showOCRProgress(1) and stopInitProgress() calls out of the finally and into the
success path (or add a conditional check) so failed startups do not report 100%
completion.
In `@src/utils/bionicUtil.ts`:
- Around line 2-6: The insertAfter function incorrectly checks
existingNode.nextSibling !== undefined (nextSibling is null when absent), so
update the logic in insertAfter: either remove the conditional and always call
existingNode.parentNode.insertBefore(newNode, existingNode.nextSibling) (since
insertBefore accepts null and appends), or change the check to
existingNode.nextSibling !== null; refer to the insertAfter function and
existingNode.nextSibling when applying the fix.
- Around line 73-74: When iterating over collection and encountering an iframe
(the if-check using collection[i].nodeName === "IFRAME"), wrap the access to
collection[i].contentDocument in a try-catch so a cross-origin SecurityError
doesn't abort processing; inside try call
processDocumentBody(collection[i].contentDocument) only if contentDocument is
non-null, and in catch simply skip that iframe (optionally log/debug the
exception). This change should be made around the iframe handling in the loop
that calls processDocumentBody to ensure other nodes continue to be processed.
In `@src/utils/noteUtil.ts`:
- Around line 154-155: The style string unconditionally appends "pointer-events:
none;" when isMobile is true, which prevents click/touch handlers from firing;
update the style construction in src/utils/noteUtil.ts so that pointer-events is
not set to none for interactive highlight elements (remove the unconditional
"pointer-events: none;" or set "pointer-events: auto;" when the node should
remain interactive), and ensure the logic that builds the overlay/tooltip style
(the concatenation using isMobile) only disables pointer events for
non-interactive backdrops while leaving highlight nodes able to receive
touch/click events; adjust both occurrences that use isMobile (the concatenation
around lines where pointer-events is added) so mobile taps can open highlights.
- Around line 122-127: The reducer that computes topRightPdfRect currently
prefers the leftmost rect on ties; update its tie-breaker to prefer the rect
with the greater right coordinate instead of smaller left. Concretely, in the
reduce callback for topRightPdfRect (and the analogous reducer elsewhere in this
file), change the tie condition from comparing left (< best.left) to comparing
right (> best.right) so that when r.top === best.top you pick the rect with the
larger right value; keep the same null initial value and other top-comparison
logic.
- Line 389: The icon X-position computation uses doc.body.scrollLeft directly
but the code already computes a normalized scrollLeft variable earlier; update
the expression in src/utils/noteUtil.ts (the line computing (Math.min(rect.left,
rect.x) + doc.body.scrollLeft + rect.width - 5) + ...) to use the normalized
scrollLeft variable instead of doc.body.scrollLeft so horizontal scroll is
applied consistently (replace doc.body.scrollLeft with scrollLeft).
---
Duplicate comments:
In `@src/renders/PdfTextRender.ts`:
- Line 473: The code calls makePDF(...) unconditionally in PdfTextRender (eg.
this.book = await makePDF(file, this.password);) which can throw for non-PDF
data; update the parse() (or the constructor path that invokes makePDF) to first
call isPDF(file) and only call makePDF when isPDF returns true, otherwise handle
the non-PDF case (reject/throw a clear error or return gracefully) so PDF.js
transport/getDocument is never invoked on non-PDF buffers; reference the isPDF
and makePDF symbols and modify the PdfTextRender.parse (or where makePDF is
called) to implement this guard.
In `@src/utils/noteUtil.ts`:
- Around line 250-268: The swim-wait loop using requestAnimationFrame in the
isMobile && window.isSwiping branch (function waitAndHighlight) can run forever
if window.isSwiping never becomes false; add a bailout: record a start time or
frame counter when waitAndHighlight is first scheduled, and if a max elapsed
time (e.g., 300–500ms) or max frames is exceeded, stop scheduling more RAFs and
either call highlightRange immediately or abort gracefully; also capture the RAF
id so you can call cancelAnimationFrame if you decide to bail early. Make the
change inside the existing waitAndHighlight/requestAnimationFrame logic that
references waitAndHighlight, window.isSwiping, highlightRange, and
requestAnimationFrame.
---
Nitpick comments:
In `@src/utils/bionicUtil.ts`:
- Around line 9-54: The HalfBold function uses var for function-scoped
variables; update declarations to modern block-scoped keywords: change the loop
index to let i, replace var recentNode and var newNodeCount with let recentNode
and let newNodeCount, make trimmedWordLength a const, make length a let (since
it's reassigned), and use const for bold and use let (or const when appropriate)
for textNode inside the forEach callback; ensure the insertAfter and
parentElement.removeChild logic remains unchanged and keep the eslint-disable
comment if needed.
- Around line 56-63: Update the ignoreTags map in bionicUtil.ts to also include
CODE, PRE, and TEXTAREA so code blocks and preformatted/textarea content are
skipped; locate the ignoreTags variable and add entries for "CODE": true, "PRE":
true, and "TEXTAREA": true so functions that check ignoreTags (e.g., the bionic
processing routine referencing ignoreTags) will bypass those nodes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54dc90d5-43e6-42d9-872a-60bf83b91bd7
📒 Files selected for processing (5)
src/libs/epub.jssrc/renders/PdfRender.tssrc/renders/PdfTextRender.tssrc/utils/bionicUtil.tssrc/utils/noteUtil.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/libs/epub.js
- src/renders/PdfRender.ts
Comment on lines
+512
to
+514
| stopInitProgress(); | ||
| showOCRProgress(1); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t force progress to 100% in finally on init failure.
Line 513 and Line 575 run even when initialization throws, so UI can show completion for failed startup.
✅ Proposed fix
- const stopInitProgress = this.startFakeProgress(0.85, 30000);
+ const stopInitProgress = this.startFakeProgress(0.85, 30000);
+ let initSucceeded = false;
try {
// ...
await worker.load();
this.worker = worker;
+ initSucceeded = true;
} finally {
stopInitProgress();
- showOCRProgress(1);
+ if (initSucceeded) showOCRProgress(1);
}- const stopInitProgress = this.startFakeProgress(0.85, 60000);
+ const stopInitProgress = this.startFakeProgress(0.85, 60000);
+ let initSucceeded = false;
try {
// ...
this.worker = localOCR;
+ initSucceeded = true;
} finally {
stopInitProgress();
- showOCRProgress(1);
+ if (initSucceeded) showOCRProgress(1);
}Also applies to: 574-576
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renders/PdfTextRender.ts` around lines 512 - 514, The finally block is
forcing progress to 100% even when initialization failed; modify the flow so
stopInitProgress() and showOCRProgress(1) (calls found near stopInitProgress and
showOCRProgress in PdfTextRender) are only invoked when initialization
succeeds—e.g., introduce a boolean flag (initSucceeded) set to true on
successful init or move the showOCRProgress(1) and stopInitProgress() calls out
of the finally and into the success path (or add a conditional check) so failed
startups do not report 100% completion.
Comment on lines
+2
to
+6
| const insertAfter = (newNode, existingNode) => { | ||
| if (existingNode.nextSibling !== undefined) | ||
| existingNode.parentNode.insertBefore(newNode, existingNode.nextSibling); | ||
| else existingNode.parentNode.appendChild(newNode); | ||
| }; |
Contributor
There was a problem hiding this comment.
Incorrect null check: nextSibling returns null, not undefined.
The DOM property nextSibling returns null when there is no next sibling, not undefined. The condition !== undefined will always be true, so line 5 is dead code. The code still works because insertBefore(node, null) appends to the end, but the logic is misleading.
🔧 Proposed fix
const insertAfter = (newNode, existingNode) => {
- if (existingNode.nextSibling !== undefined)
+ if (existingNode.nextSibling !== null)
existingNode.parentNode.insertBefore(newNode, existingNode.nextSibling);
else existingNode.parentNode.appendChild(newNode);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/bionicUtil.ts` around lines 2 - 6, The insertAfter function
incorrectly checks existingNode.nextSibling !== undefined (nextSibling is null
when absent), so update the logic in insertAfter: either remove the conditional
and always call existingNode.parentNode.insertBefore(newNode,
existingNode.nextSibling) (since insertBefore accepts null and appends), or
change the check to existingNode.nextSibling !== null; refer to the insertAfter
function and existingNode.nextSibling when applying the fix.
Comment on lines
+73
to
+74
| if (collection[i].nodeName === "IFRAME") { | ||
| processDocumentBody(collection[i].contentDocument); |
Contributor
There was a problem hiding this comment.
Wrap cross-origin iframe access in try-catch to prevent SecurityError.
Accessing contentDocument on a cross-origin iframe throws a SecurityError. While same-origin iframes will work, cross-origin ones will crash the entire processing loop.
🔒 Proposed fix
if (collection[i].nodeName === "IFRAME") {
- processDocumentBody(collection[i].contentDocument);
+ try {
+ processDocumentBody(collection[i].contentDocument);
+ } catch (e) {
+ // Cross-origin iframe - skip processing
+ }
} 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.
Suggested change
| if (collection[i].nodeName === "IFRAME") { | |
| processDocumentBody(collection[i].contentDocument); | |
| if (collection[i].nodeName === "IFRAME") { | |
| try { | |
| processDocumentBody(collection[i].contentDocument); | |
| } catch (e) { | |
| // Cross-origin iframe - skip processing | |
| } | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/bionicUtil.ts` around lines 73 - 74, When iterating over collection
and encountering an iframe (the if-check using collection[i].nodeName ===
"IFRAME"), wrap the access to collection[i].contentDocument in a try-catch so a
cross-origin SecurityError doesn't abort processing; inside try call
processDocumentBody(collection[i].contentDocument) only if contentDocument is
non-null, and in catch simply skip that iframe (optionally log/debug the
exception). This change should be made around the iframe handling in the loop
that calls processDocumentBody to ensure other nodes continue to be processed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/utils/noteUtil.ts (3)
249-267:⚠️ Potential issue | 🟠 Major
requestAnimationFrameretry loop can run forever when swiping never resets.Line 249-266 has unbounded RAF retries; if
window.isSwipinggets stuck, this never exits.Suggested safeguard
if (isMobile && window.isSwiping) { + let retryCount = 0; + const maxRetries = 120; // ~2s at 60fps const waitAndHighlight = () => { - if (window.isSwiping) { + if (window.isSwiping && retryCount < maxRetries) { + retryCount++; requestAnimationFrame(waitAndHighlight); - } else { + } else if (!window.isSwiping) { highlightRange( range, colorCode, noteKey, handleNoteClick, doc, isNote, isMobile ); } }; requestAnimationFrame(waitAndHighlight); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 249 - 267, The current wait loop using requestAnimationFrame in the isMobile && window.isSwiping branch (waitAndHighlight calling highlightRange) can run forever if window.isSwiping never resets; modify waitAndHighlight to add a bounded safeguard (e.g., track a retry counter or start timestamp and compare against MAX_WAIT_FRAMES or MAX_WAIT_MS) and bail out after the limit by calling highlightRange (or aborting safely) and cancelling further RAFs (store and use cancelAnimationFrame on the RAF id); update the symbols waitAndHighlight, requestAnimationFrame, window.isSwiping, and highlightRange to implement this timeout/limit and ensure no lingering RAF loop remains.
465-468:⚠️ Potential issue | 🟡 MinorUse normalized
scrollLeftfor icon positioning.Line 466 bypasses the normalized
scrollLeftvalue computed at Line 271, which can misplace the icon when horizontal scroll is ondocumentElement.Suggested fix
(Math.min(rect.left, rect.x) + - doc.body.scrollLeft + + scrollLeft + rect.width - 15) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 465 - 468, The icon-position calculation is using doc.body.scrollLeft directly (see the expression with rect, doc.body.scrollLeft), which bypasses the normalized horizontal scroll value computed earlier (the variable holding the normalized scrollLeft at Line 271); replace doc.body.scrollLeft with that normalized scrollLeft variable so the icon uses the same scroll normalization logic as the rest of the function (locate the normalized scrollLeft variable near Line 271 and update the expression that computes the X position using rect and rect.width).
122-127:⚠️ Potential issue | 🟡 MinorTop-right anchor still selects the leftmost rect.
Line 124 and Line 304 use
r.left < best.left, which picks top-left on ties instead of top-right.Suggested fix
- const topRightPdfRect = filteredRects.reduce<any>((best, r) => { + const topRightPdfRect = filteredRects.reduce<any>((best, r) => { if (!best) return r; - if (r.top < best.top || (r.top === best.top && r.left < best.left)) + if (r.top < best.top || (r.top === best.top && r.right > best.right)) return r; return best; }, null); - const topRightRect = validRects.reduce<DOMRect | null>((best, r) => { + const topRightRect = validRects.reduce<DOMRect | null>((best, r) => { if (!best) return r; - if (r.top < best.top || (r.top === best.top && r.left < best.left)) + if (r.top < best.top || (r.top === best.top && r.right > best.right)) return r; return best; }, null);Also applies to: 302-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 122 - 127, The top-right selection comparator in the reduce for topRightPdfRect (and the duplicate block around the other reduce) currently uses r.left < best.left which chooses the leftmost rect on ties; change the tie-breaker to prefer the larger left value (r.left > best.left) so ties on top pick the rightmost rect, and ensure both occurrences (the reduce initializing best to null and the one at 302-307) are updated consistently to use the new comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/noteUtil.ts`:
- Around line 357-390: The delegated body click handler that computes absX/absY
and calls handleNoteClick for .kookit-note[data-key] can run for the same user
action as the icon-level click handler, causing handleNoteClick twice; fix by
adding a dedupe guard: in the icon-level click handler (the handler around lines
~477-486) either call e.stopPropagation()/e.stopImmediatePropagation() or set a
custom flag on the event (e.__kookitNoteHandled = true); then in the body
delegated click handler (the function that uses delegateDownX/delegateDownY and
queries ".kookit-note[data-key]") check for that flag (or for
event.defaultPrevented or target.closest('.kookit-note-icon')) and return early
if present so handleNoteClick is only invoked once.
---
Duplicate comments:
In `@src/utils/noteUtil.ts`:
- Around line 249-267: The current wait loop using requestAnimationFrame in the
isMobile && window.isSwiping branch (waitAndHighlight calling highlightRange)
can run forever if window.isSwiping never resets; modify waitAndHighlight to add
a bounded safeguard (e.g., track a retry counter or start timestamp and compare
against MAX_WAIT_FRAMES or MAX_WAIT_MS) and bail out after the limit by calling
highlightRange (or aborting safely) and cancelling further RAFs (store and use
cancelAnimationFrame on the RAF id); update the symbols waitAndHighlight,
requestAnimationFrame, window.isSwiping, and highlightRange to implement this
timeout/limit and ensure no lingering RAF loop remains.
- Around line 465-468: The icon-position calculation is using
doc.body.scrollLeft directly (see the expression with rect,
doc.body.scrollLeft), which bypasses the normalized horizontal scroll value
computed earlier (the variable holding the normalized scrollLeft at Line 271);
replace doc.body.scrollLeft with that normalized scrollLeft variable so the icon
uses the same scroll normalization logic as the rest of the function (locate the
normalized scrollLeft variable near Line 271 and update the expression that
computes the X position using rect and rect.width).
- Around line 122-127: The top-right selection comparator in the reduce for
topRightPdfRect (and the duplicate block around the other reduce) currently uses
r.left < best.left which chooses the leftmost rect on ties; change the
tie-breaker to prefer the larger left value (r.left > best.left) so ties on top
pick the rightmost rect, and ensure both occurrences (the reduce initializing
best to null and the one at 302-307) are updated consistently to use the new
comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 013757c4-05ff-4d7f-92de-3cb470fdf182
📒 Files selected for processing (1)
src/utils/noteUtil.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/utils/noteUtil.ts (4)
122-127:⚠️ Potential issue | 🟡 MinorTop-right reducer still selects the left-side rect on tie.
At Line 124 and Line 304, the tie-breaker uses
r.right < best.right, which picks the smaller/rightmost boundary incorrectly for a top-right anchor.Suggested fix
- if (r.top < best.top || (r.top === best.top && r.right < best.right)) + if (r.top < best.top || (r.top === best.top && r.right > best.right)) return r;- if (r.top < best.top || (r.top === best.top && r.right < best.right)) + if (r.top < best.top || (r.top === best.top && r.right > best.right)) return r;Also applies to: 302-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 122 - 127, The reducer that computes topRightPdfRect is using the wrong tie-breaker (r.right < best.right) so it selects the left-side rect on ties; update the reduce callback used on filteredRects (and the duplicate occurrence) to choose the rect with the larger right boundary when tops are equal (i.e., use r.right > best.right) so the true top-right anchor is selected—locate the reduce calls that assign topRightPdfRect (and the other similar variable) and flip the comparison in the tie-breaker accordingly.
465-467:⚠️ Potential issue | 🟡 MinorUse normalized
scrollLeftfor icon X placement.At Line 466,
doc.body.scrollLeftbypasses the normalizedscrollLeftused elsewhere and can misplace icons when scrolling is ondocumentElement.Suggested fix
(Math.min(rect.left, rect.x) + - doc.body.scrollLeft + + scrollLeft + rect.width - 15) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 465 - 467, The X position calculation for the icon uses doc.body.scrollLeft directly, which bypasses the normalized scrollLeft used elsewhere and can misplace icons; update the expression that builds the X (the line using Math.min(rect.left, rect.x) + doc.body.scrollLeft + rect.width - … in src/utils/noteUtil.ts) to use the normalized scrollLeft helper or scrollingElement fallback (e.g. use a getScrollLeft(doc) or doc.scrollingElement?.scrollLeft || doc.documentElement.scrollLeft || doc.body.scrollLeft) so the same normalized scroll offset is applied for icon placement.
249-267:⚠️ Potential issue | 🟡 MinorMobile swipe RAF loop can run forever without a guard.
At Line 249-267, if
window.isSwipinggets stuck true, this keeps scheduling frames indefinitely. Add a timeout/retry ceiling.Suggested safeguard
if (isMobile && window.isSwiping) { + let retryCount = 0; + const maxRetries = 120; // ~2s at 60fps const waitAndHighlight = () => { - if (window.isSwiping) { + if (window.isSwiping && retryCount < maxRetries) { + retryCount++; requestAnimationFrame(waitAndHighlight); } else { highlightRange( range, colorCode, noteKey, handleNoteClick, doc, isNote, isMobile ); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 249 - 267, The RAF loop started in the isMobile && window.isSwiping branch (the waitAndHighlight function using requestAnimationFrame) can run forever if window.isSwiping never becomes false; add a guard (e.g., a frame counter or timeout based on performance.now()) inside waitAndHighlight to stop retrying after a max threshold (like MAX_FRAMES or MAX_MS), ensure you cancel further RAF scheduling and then either call highlightRange(range, colorCode, noteKey, handleNoteClick, doc, isNote, isMobile) as a fallback or bail safely, and keep references to window.isSwiping, waitAndHighlight, requestAnimationFrame and highlightRange when implementing this ceiling so the loop cannot continue indefinitely.
357-390:⚠️ Potential issue | 🟠 MajorIcon click can invoke
handleNoteClicktwice.At Line 357-390 (delegated body click) and Line 477-486 (icon click), one user click can trigger both handlers.
Suggested dedupe guard in delegated click
doc.body.addEventListener( "click", (e: MouseEvent) => { + const target = e.target as HTMLElement | null; + if (target?.closest(".kookit-note[data-key]")) return; if ( Math.abs(e.clientX - delegateDownX) > 5 || Math.abs(e.clientY - delegateDownY) > 5 ) return;Also applies to: 477-486
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 357 - 390, The delegated body "click" listener currently finds notes by geometry and calls handleNoteClick, which can run in addition to the explicit icon click handler and cause double-invocation; fix by adding a dedupe guard: in the icon click handler set either event.stopPropagation() or mark the clicked element (e.g., element.dataset.noteClickHandled = "1") before calling handleNoteClick, and in the delegated "click" listener (the anonymous function that computes absX/absY and iterates notes) check for that marker or check event.target/its ancestors for the icon class (or dataset) and skip calling handleNoteClick if present; update both the delegated listener and the icon click handler to use the same marker or propagation behavior so handleNoteClick is only invoked once.
🧹 Nitpick comments (1)
src/utils/noteUtil.ts (1)
310-430: Avoid stale callback capture in one-time delegated listeners.Delegated listeners are registered once (Line 310-312) and close over the first
handleNoteClick. If the callback reference changes later, handlers may call outdated logic.Refactor to keep latest callback
if (!(doc.body as any).__kookitDelegated) { (doc.body as any).__kookitDelegated = true; + (doc.body as any).__kookitHandleNoteClick = handleNoteClick; let delegateDownX = 0; let delegateDownY = 0; @@ - handleNoteClick({ target: el }); + (doc.body as any).__kookitHandleNoteClick?.({ target: el }); break; @@ - handleNoteClick({ target: el }); + (doc.body as any).__kookitHandleNoteClick?.({ target: el }); e.preventDefault(); e.stopPropagation(); break; @@ } + (doc.body as any).__kookitHandleNoteClick = handleNoteClick;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 310 - 430, The delegated body listeners are registered once and close over the initial handleNoteClick, causing stale callbacks if that reference changes; fix by introducing a mutable holder for the current handler (e.g., handleNoteClickRef with a .current property) and have the event listeners call handleNoteClickRef.current({...}) instead of directly calling handleNoteClick, update handleNoteClickRef.current whenever the handler changes, and keep the __kookitDelegated guard and the listeners on doc.body as-is so they always invoke the latest handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/utils/noteUtil.ts`:
- Around line 122-127: The reducer that computes topRightPdfRect is using the
wrong tie-breaker (r.right < best.right) so it selects the left-side rect on
ties; update the reduce callback used on filteredRects (and the duplicate
occurrence) to choose the rect with the larger right boundary when tops are
equal (i.e., use r.right > best.right) so the true top-right anchor is
selected—locate the reduce calls that assign topRightPdfRect (and the other
similar variable) and flip the comparison in the tie-breaker accordingly.
- Around line 465-467: The X position calculation for the icon uses
doc.body.scrollLeft directly, which bypasses the normalized scrollLeft used
elsewhere and can misplace icons; update the expression that builds the X (the
line using Math.min(rect.left, rect.x) + doc.body.scrollLeft + rect.width - … in
src/utils/noteUtil.ts) to use the normalized scrollLeft helper or
scrollingElement fallback (e.g. use a getScrollLeft(doc) or
doc.scrollingElement?.scrollLeft || doc.documentElement.scrollLeft ||
doc.body.scrollLeft) so the same normalized scroll offset is applied for icon
placement.
- Around line 249-267: The RAF loop started in the isMobile && window.isSwiping
branch (the waitAndHighlight function using requestAnimationFrame) can run
forever if window.isSwiping never becomes false; add a guard (e.g., a frame
counter or timeout based on performance.now()) inside waitAndHighlight to stop
retrying after a max threshold (like MAX_FRAMES or MAX_MS), ensure you cancel
further RAF scheduling and then either call highlightRange(range, colorCode,
noteKey, handleNoteClick, doc, isNote, isMobile) as a fallback or bail safely,
and keep references to window.isSwiping, waitAndHighlight, requestAnimationFrame
and highlightRange when implementing this ceiling so the loop cannot continue
indefinitely.
- Around line 357-390: The delegated body "click" listener currently finds notes
by geometry and calls handleNoteClick, which can run in addition to the explicit
icon click handler and cause double-invocation; fix by adding a dedupe guard: in
the icon click handler set either event.stopPropagation() or mark the clicked
element (e.g., element.dataset.noteClickHandled = "1") before calling
handleNoteClick, and in the delegated "click" listener (the anonymous function
that computes absX/absY and iterates notes) check for that marker or check
event.target/its ancestors for the icon class (or dataset) and skip calling
handleNoteClick if present; update both the delegated listener and the icon
click handler to use the same marker or propagation behavior so handleNoteClick
is only invoked once.
---
Nitpick comments:
In `@src/utils/noteUtil.ts`:
- Around line 310-430: The delegated body listeners are registered once and
close over the initial handleNoteClick, causing stale callbacks if that
reference changes; fix by introducing a mutable holder for the current handler
(e.g., handleNoteClickRef with a .current property) and have the event listeners
call handleNoteClickRef.current({...}) instead of directly calling
handleNoteClick, update handleNoteClickRef.current whenever the handler changes,
and keep the __kookitDelegated guard and the listeners on doc.body as-is so they
always invoke the latest handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c8a265d-e4b4-4547-9e26-6d658ae74301
📒 Files selected for processing (1)
src/utils/noteUtil.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/utils/noteUtil.ts (4)
465-467:⚠️ Potential issue | 🟡 MinorUse normalized
scrollLeftfor icon placement.Line 466 bypasses the already-normalized
scrollLeft, so icon X can be wrong when scrolling is ondocumentElement.Proposed fix
- (Math.min(rect.left, rect.x) + doc.body.scrollLeft + rect.width - 15) + + (Math.min(rect.left, rect.x) + scrollLeft + rect.width - 15) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 465 - 467, The icon X calculation uses doc.body.scrollLeft directly, bypassing the normalized scrollLeft used elsewhere and causing wrong placement when scrolling is on documentElement; update the expression in the icon placement logic (the Math.min(...)+ doc.body.scrollLeft + rect.width - ... computation in src/utils/noteUtil.ts) to use the previously computed normalized scrollLeft variable (e.g., scrollLeft or normalizedScrollLeft used earlier in the function) so the same normalized value is used consistently for X positioning.
249-267:⚠️ Potential issue | 🟡 MinorSwipe wait loop has no termination guard.
If
window.isSwipingnever clears, this RAF recursion can run indefinitely.Proposed safeguard
if (isMobile && window.isSwiping) { + let retries = 0; + const maxRetries = 180; // ~3s at 60fps const waitAndHighlight = () => { - if (window.isSwiping) { + if (window.isSwiping && retries < maxRetries) { + retries++; requestAnimationFrame(waitAndHighlight); } else { highlightRange( range, colorCode, noteKey, handleNoteClick, doc, isNote, isMobile ); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 249 - 267, The RAF recursion in waitAndHighlight (triggered when isMobile && window.isSwiping) lacks a termination guard and can loop forever if window.isSwiping never clears; modify waitAndHighlight to include a bailout condition (e.g., max frame count or timeout using a start timestamp) that stops calling requestAnimationFrame and optionally calls a safe fallback or logs a warning; keep the existing call to highlightRange(range, colorCode, noteKey, handleNoteClick, doc, isNote, isMobile) only when swipe ends and ensure the guard references the same identifiers (window.isSwiping, waitAndHighlight, requestAnimationFrame) so the loop always exits after the configured limit.
158-182:⚠️ Potential issue | 🟠 Major
handleNoteClickcan fire twice from mixed direct + delegated handlers.PDF nodes/icons invoke direct click/touch handlers, and body-level delegated handlers also fire for the same interaction path.
Proposed fix
doc.body.addEventListener( "click", (e: MouseEvent) => { + const target = e.target as HTMLElement | null; + if (target?.closest(".kookit-note[data-key]")) return; ... }, true );doc.body.addEventListener( "touchend", (e: TouchEvent) => { + const target = e.target as HTMLElement | null; + if (target?.closest(".kookit-note[data-key]")) return; ... }, true );Also applies to: 202-226, 357-390, 393-429
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 158 - 182, The node-level click/touch handlers (newNode.addEventListener("click", ...), newNode.ontouchend) call handleNoteClick while a body-level delegated handler also runs, causing duplicate invocations; modify these handlers to mark the event as handled and stop further handling by calling event.stopImmediatePropagation() (and keep event.preventDefault()/event.stopPropagation() on touch) or set a short-lived flag on the event target (e.g., dataset.__noteHandled) and check that flag in the delegated handlers before calling handleNoteClick; apply the same change consistently to the other occurrences referenced (lines around 202-226, 357-390, 393-429) so only one handler invokes handleNoteClick per interaction.
122-127:⚠️ Potential issue | 🟡 MinorTop-right anchor tie-breaker is still reversed.
Both reducers use
r.right < best.right, which selects the more-left rect on same-row ties. For a top-right anchor, this should prefer greaterright.Proposed fix
- if (r.top < best.top || (r.top === best.top && r.right < best.right)) + if (r.top < best.top || (r.top === best.top && r.right > best.right)) return r;Also applies to: 302-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/noteUtil.ts` around lines 122 - 127, The top-right anchor reducer (variable topRightPdfRect using filteredRects.reduce) currently uses r.right < best.right which selects the more-left rect on row ties; change the tie-breaker to r.right > best.right so it prefers the rect with the greater right coordinate. Update the identical comparator in the other reducer that computes the top-right anchor (the same reduce pattern around the other occurrence) to use r.right > best.right as well so same-row ties pick the rightmost rect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/noteUtil.ts`:
- Around line 310-430: The delegated listeners set under (doc.body as
any).__kookitDelegated close over the first handleNoteClick and become stale;
change the pattern to store the current handler on the DOM (e.g., (doc.body as
any).__kookitHandleNoteClick) and have all delegated listeners call that
property at invocation time (lookup and invoke (doc.body as
any).__kookitHandleNoteClick({ target: el }) instead of the closed-over
handleNoteClick); ensure highlightRange (or whatever installs the handler)
updates this DOM property whenever a new handler is provided and keep the single
__kookitDelegated flag so listeners are only bound once.
---
Duplicate comments:
In `@src/utils/noteUtil.ts`:
- Around line 465-467: The icon X calculation uses doc.body.scrollLeft directly,
bypassing the normalized scrollLeft used elsewhere and causing wrong placement
when scrolling is on documentElement; update the expression in the icon
placement logic (the Math.min(...)+ doc.body.scrollLeft + rect.width - ...
computation in src/utils/noteUtil.ts) to use the previously computed normalized
scrollLeft variable (e.g., scrollLeft or normalizedScrollLeft used earlier in
the function) so the same normalized value is used consistently for X
positioning.
- Around line 249-267: The RAF recursion in waitAndHighlight (triggered when
isMobile && window.isSwiping) lacks a termination guard and can loop forever if
window.isSwiping never clears; modify waitAndHighlight to include a bailout
condition (e.g., max frame count or timeout using a start timestamp) that stops
calling requestAnimationFrame and optionally calls a safe fallback or logs a
warning; keep the existing call to highlightRange(range, colorCode, noteKey,
handleNoteClick, doc, isNote, isMobile) only when swipe ends and ensure the
guard references the same identifiers (window.isSwiping, waitAndHighlight,
requestAnimationFrame) so the loop always exits after the configured limit.
- Around line 158-182: The node-level click/touch handlers
(newNode.addEventListener("click", ...), newNode.ontouchend) call
handleNoteClick while a body-level delegated handler also runs, causing
duplicate invocations; modify these handlers to mark the event as handled and
stop further handling by calling event.stopImmediatePropagation() (and keep
event.preventDefault()/event.stopPropagation() on touch) or set a short-lived
flag on the event target (e.g., dataset.__noteHandled) and check that flag in
the delegated handlers before calling handleNoteClick; apply the same change
consistently to the other occurrences referenced (lines around 202-226, 357-390,
393-429) so only one handler invokes handleNoteClick per interaction.
- Around line 122-127: The top-right anchor reducer (variable topRightPdfRect
using filteredRects.reduce) currently uses r.right < best.right which selects
the more-left rect on row ties; change the tie-breaker to r.right > best.right
so it prefers the rect with the greater right coordinate. Update the identical
comparator in the other reducer that computes the top-right anchor (the same
reduce pattern around the other occurrence) to use r.right > best.right as well
so same-row ties pick the rightmost rect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e03418d-b502-4ad0-8c7e-41ef3868ae6b
📒 Files selected for processing (1)
src/utils/noteUtil.ts
Comment on lines
+310
to
+430
| if (!(doc.body as any).__kookitDelegated) { | ||
| (doc.body as any).__kookitDelegated = true; | ||
| let delegateDownX = 0; | ||
| let delegateDownY = 0; | ||
| doc.body.addEventListener( | ||
| "mousemove", | ||
| (e: MouseEvent) => { | ||
| const bodyScrollTop = | ||
| doc.body.scrollTop || doc.documentElement.scrollTop; | ||
| const bodyScrollLeft = | ||
| doc.body.scrollLeft || doc.documentElement.scrollLeft; | ||
| const absX = e.clientX + bodyScrollLeft; | ||
| const absY = e.clientY + bodyScrollTop; | ||
| const notes = doc.body.querySelectorAll( | ||
| ".kookit-note[data-key]" | ||
| ) as NodeListOf<HTMLElement>; | ||
| let overHighlight = false; | ||
| for (let n = 0; n < notes.length; n++) { | ||
| const el = notes[n]; | ||
| const s = el.style; | ||
| const left = parseFloat(s.left); | ||
| const top = parseFloat(s.top); | ||
| const w = parseFloat(s.width); | ||
| const h = parseFloat(s.height); | ||
| if ( | ||
| absX >= left && | ||
| absX <= left + w && | ||
| absY >= top && | ||
| absY <= top + h | ||
| ) { | ||
| overHighlight = true; | ||
| break; | ||
| } | ||
| } | ||
| doc.body.style.cursor = overHighlight ? "pointer" : ""; | ||
| }, | ||
| true | ||
| ); | ||
| doc.body.addEventListener( | ||
| "mousedown", | ||
| (e: MouseEvent) => { | ||
| delegateDownX = e.clientX; | ||
| delegateDownY = e.clientY; | ||
| }, | ||
| true | ||
| ); | ||
| doc.body.addEventListener( | ||
| "click", | ||
| (e: MouseEvent) => { | ||
| if ( | ||
| Math.abs(e.clientX - delegateDownX) > 5 || | ||
| Math.abs(e.clientY - delegateDownY) > 5 | ||
| ) | ||
| return; | ||
| const bodyScrollTop = | ||
| doc.body.scrollTop || doc.documentElement.scrollTop; | ||
| const bodyScrollLeft = | ||
| doc.body.scrollLeft || doc.documentElement.scrollLeft; | ||
| const absX = e.clientX + bodyScrollLeft; | ||
| const absY = e.clientY + bodyScrollTop; | ||
| const notes = doc.body.querySelectorAll( | ||
| ".kookit-note[data-key]" | ||
| ) as NodeListOf<HTMLElement>; | ||
| for (let n = 0; n < notes.length; n++) { | ||
| const el = notes[n]; | ||
| const s = el.style; | ||
| const left = parseFloat(s.left); | ||
| const top = parseFloat(s.top); | ||
| const w = parseFloat(s.width); | ||
| const h = parseFloat(s.height); | ||
| if ( | ||
| absX >= left && | ||
| absX <= left + w && | ||
| absY >= top && | ||
| absY <= top + h | ||
| ) { | ||
| handleNoteClick({ target: el }); | ||
| break; | ||
| } | ||
| } | ||
| }, | ||
| true | ||
| ); | ||
| doc.body.addEventListener( | ||
| "touchend", | ||
| (e: TouchEvent) => { | ||
| if (window.isSwiping) return; | ||
| const touch = e.changedTouches[0]; | ||
| if (!touch) return; | ||
| const bodyScrollTop = | ||
| doc.body.scrollTop || doc.documentElement.scrollTop; | ||
| const bodyScrollLeft = | ||
| doc.body.scrollLeft || doc.documentElement.scrollLeft; | ||
| const absX = touch.clientX + bodyScrollLeft; | ||
| const absY = touch.clientY + bodyScrollTop; | ||
| const notes = doc.body.querySelectorAll( | ||
| ".kookit-note[data-key]" | ||
| ) as NodeListOf<HTMLElement>; | ||
| for (let n = 0; n < notes.length; n++) { | ||
| const el = notes[n]; | ||
| const s = el.style; | ||
| const left = parseFloat(s.left); | ||
| const top = parseFloat(s.top); | ||
| const w = parseFloat(s.width); | ||
| const h = parseFloat(s.height); | ||
| if ( | ||
| absX >= left && | ||
| absX <= left + w && | ||
| absY >= top && | ||
| absY <= top + h | ||
| ) { | ||
| handleNoteClick({ target: el }); | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| break; | ||
| } | ||
| } | ||
| }, | ||
| true | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Delegated listeners capture a stale handleNoteClick reference.
Listeners are registered once (__kookitDelegated) but close over the first callback. Later calls to highlightRange with a different handler won’t be used.
Proposed fix
+ (doc.body as any).__kookitHandleNoteClick = handleNoteClick;
if (!(doc.body as any).__kookitDelegated) {
(doc.body as any).__kookitDelegated = true;
...
doc.body.addEventListener("click", (e: MouseEvent) => {
+ const clickHandler = (doc.body as any).__kookitHandleNoteClick;
...
- handleNoteClick({ target: el });
+ clickHandler?.({ target: el });
}, true);
doc.body.addEventListener("touchend", (e: TouchEvent) => {
+ const clickHandler = (doc.body as any).__kookitHandleNoteClick;
...
- handleNoteClick({ target: el });
+ clickHandler?.({ target: el });
}, true);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/noteUtil.ts` around lines 310 - 430, The delegated listeners set
under (doc.body as any).__kookitDelegated close over the first handleNoteClick
and become stale; change the pattern to store the current handler on the DOM
(e.g., (doc.body as any).__kookitHandleNoteClick) and have all delegated
listeners call that property at invocation time (lookup and invoke (doc.body as
any).__kookitHandleNoteClick({ target: el }) instead of the closed-over
handleNoteClick); ensure highlightRange (or whatever installs the handler)
updates this DOM property whenever a new handler is provided and keep the single
__kookitDelegated flag so listeners are only bound once.
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores