Conversation
… snapping conditions
Contributor
📝 WalkthroughWalkthroughAdds batch translation state and workflows, moves block-detection utilities to a shared module, inlines image-marker/asset precache logic, changes note highlighting from overlays to inline wrapped spans with icon-only rendering, refines large-file title fallback, and adjusts touch/scroll behaviors for translation modes. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as GeneralRender
participant DOM as Document/NoteUtil
participant Translator as TranslationService
participant Window as window/transMap
Renderer->>DOM: getBatchTransTexts() -> collect texts (audioText/DOM)
Renderer->>Window: read transMap to filter untranslated items
Renderer->>Translator: send batch request (<=100 items, <=10000 chars)
Translator-->>Renderer: return translated texts
Renderer->>Window: handleBatchTransResult() -> update transMap entries
Renderer->>DOM: set data-kookit-translation attrs, remove loading flags
alt fullTranslationMode == "target"
Renderer->>DOM: apply font-size:0 on target elements (hide source)
end
DOM-->>Renderer: showNoteHighlightBatch applies inline spans/icons
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 reader utilities to improve touch/scroll behavior, note highlighting reliability, and introduces infrastructure for “full translation” rendering.
Changes:
- Refactors note highlighting to use inline span wrappers (instead of absolute overlays), adds tooltip class adjustments, and introduces a batch highlight renderer.
- Adds selection metadata (
sentence) to touch selection events and tweaks page snap/drag behavior. - Introduces “full translation” support by tagging block nodes for translation and propagating translation results back into the DOM.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/touchUtil.ts | Stops drag state during animations; adds sentence extraction for selection events. |
| src/utils/noteUtil.ts | Switches to inline highlight spans, adds batch highlight API, updates tooltip/icon behavior, adjusts highlight clearing. |
| src/utils/navigationUtil.ts | Moves block helpers to common, tweaks scroll positioning logic based on translation mode. |
| src/utils/layoutUtil.ts | Integrates asset precaching with translation-host tagging and image markers. |
| src/utils/common.ts | Centralizes shared DOM helpers (getBlockElement, isParentBlock). |
| src/renders/GeneralRender.ts | Uses batch note highlight rendering; adds translation state/map and translation result application. |
| src/libs/textProcessor.ts | Adds a performance shortcut for very large TXT files with low title density. |
| src/helpers/styleHelper.ts | Adds CSS for inline highlights/icons and translation pseudo-elements; changes default CSS API to accept bookKey. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+230
to
+231
| // Split on sentence-ending punctuation to find the sentence | ||
| let sentences = fullText.split(/(?<=[.!?。！？])\s*/); |
There was a problem hiding this comment.
getSelectionSentence splits sentences using a RegExp lookbehind /(?<=[.!?。！？])\s*/, which isn’t supported in ES6 targets and will throw a SyntaxError in older JS engines (your tsconfig target is es6). Consider rewriting this split without lookbehind (e.g., match sentence boundaries with a capturing group or iterate with a global regexp) so selection handling doesn’t break at runtime.
Suggested change
| // Split on sentence-ending punctuation to find the sentence | |
| let sentences = fullText.split(/(?<=[.!?。！？])\s*/); | |
| // Match sentence-like chunks without using RegExp lookbehind so this | |
| // remains compatible with ES6-targeted runtimes. | |
| let sentences = | |
| fullText | |
| .match(/[^.!?。！？]+[.!?。！？]*/g) | |
| ?.map((s) => s.trim()) | |
| .filter(Boolean) || []; |
src/utils/noteUtil.ts
Outdated
| "px; height:" + | ||
| rect.height + | ||
| "px; z-index: -1; " | ||
| "position: relative; display: width: 16px; height: 16px;" + |
There was a problem hiding this comment.
The inline note icon style string is malformed: "display: width: 16px; height: 16px;" sets display to an invalid value and never sets width, so the icon sizing/placement will be inconsistent across browsers. Update the style to use valid properties (e.g., display: inline-block; width: 16px; height: 16px;) and consider whether it should be positioned absolutely/with margins to avoid shifting the text flow.
Suggested change
| "position: relative; display: width: 16px; height: 16px;" + | |
| "position: relative; display: inline-block; width: 16px; height: 16px;" + |
Comment on lines
355
to
374
| const iconNode = document.createElement("div"); | ||
| iconNode.setAttribute( | ||
| "style", | ||
| "position: absolute;" + | ||
| "left:" + | ||
| (rect.left + | ||
| parseFloat(getComputedStyle(docLayer as Element).marginLeft) + | ||
| rect.width - | ||
| 15) + | ||
| "px; top:" + | ||
| (rect.top - 15) + | ||
| "px;" + | ||
| "width: 16px; height: 16px; z-index: 2; cursor: pointer; font-size: 14px; line-height: 1;" | ||
| ); | ||
| iconNode.setAttribute("class", "kookit-note"); | ||
| iconNode.setAttribute("class", "kookit-note kookit-note-icon"); | ||
| iconNode.setAttribute("data-key", noteKey); | ||
| iconNode.textContent = "📋"; | ||
| iconNode.addEventListener("click", (event) => { | ||
| if (event && event.target) { | ||
| if ( | ||
| (event.target as any).dataset && | ||
| (event.target as any).dataset.key | ||
| ) { | ||
| handleNoteClick(event); | ||
| } | ||
| } | ||
| }); | ||
| iconNode.ontouchend = (event) => { | ||
| if (window.isSwiping) { | ||
| return; | ||
| } | ||
| if (event && event.target) { | ||
| if ( | ||
| (event.target as any).dataset && | ||
| (event.target as any).dataset.key | ||
| ) { | ||
| handleNoteClick(event); | ||
| } | ||
| } | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| }; | ||
| // No textContent — use CSS ::before to display icon, avoiding | ||
| // interference with rangy character-offset calculations | ||
| pageElement.appendChild(iconNode); | ||
| } |
There was a problem hiding this comment.
In showPDFHighlight, the note icon element (kookit-note-icon) is no longer given any click/touch handler. Since it’s appended as a separate element (not inside the highlight div), clicking the icon won’t trigger handleNoteClick. Either re-add the click/touch handlers for the icon, or set pointer-events: none on the icon so clicks fall through to the underlying .kookit-note highlight overlay.
Comment on lines
+160
to
+162
| window.transMap[originalText] = { | ||
| id, | ||
| }; |
There was a problem hiding this comment.
window.transMap is keyed by originalText, but multiple blocks can share identical textContent. In that case this assignment overwrites earlier entries, leaving some elements stuck with kookit-translation-loading and never updated by handleBatchTransResult. Use a unique key per element (e.g., id → { sourceText, ... }) or store an array of ids per sourceText so all matching nodes get updated.
Suggested change
| window.transMap[originalText] = { | |
| id, | |
| }; | |
| let existingTransEntry = window.transMap[originalText]; | |
| if (existingTransEntry) { | |
| if (Array.isArray(existingTransEntry.ids)) { | |
| existingTransEntry.ids.push(id); | |
| } else { | |
| existingTransEntry.ids = existingTransEntry.id | |
| ? [existingTransEntry.id, id] | |
| : [id]; | |
| } | |
| } else { | |
| window.transMap[originalText] = { | |
| sourceText: originalText, | |
| id, | |
| ids: [id], | |
| }; | |
| } |
Comment on lines
+1454
to
+1463
| element.setAttribute( | ||
| "style", | ||
| element.getAttribute("style") + ";font-size:0px !important;" | ||
| ); | ||
| let childElements = element.querySelectorAll("*"); | ||
| childElements.forEach((child) => { | ||
| child.setAttribute( | ||
| "style", | ||
| child.getAttribute("style") + ";font-size:0px !important;" | ||
| ); |
There was a problem hiding this comment.
element.getAttribute("style") (and the same call on children) can return null, which will produce a literal "null;font-size:0px ..." style attribute. Default to an empty string (or better: toggle a CSS class) before concatenating, and avoid repeatedly appending the same font-size:0px fragment if this method can be called more than once.
![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: 5
🧹 Nitpick comments (5)
src/utils/common.ts (2)
42-48: Remove commented-out code.This block of dead code reduces readability. If it's no longer needed, delete it; if it represents an alternative approach worth preserving, move it to a comment explaining the rationale or a separate branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/common.ts` around lines 42 - 48, Remove the dead commented-out code that declares and uses blockElementList and the conditional on its length in src/utils/common.ts; locate the commented lines referencing blockElementList, blockRegex, and children and delete them, or if the logic is intentionally preserved for context, replace the commented block with a short explanatory comment stating why the alternative approach was kept (e.g., "alternative: filter children by blockRegex into blockElementList to check length") and move it to a higher-level comment or a separate branch.
30-36: Add type annotation for theElementparameter.The parameter name
Elementshadows the globalElementtype and lacks a type annotation. Consider renaming and typing it for clarity.Suggested fix
-export const getBlockElement = (Element) => { +export const getBlockElement = (container: Element | Document): HTMLElement[] => { return Array.from( - Element.querySelectorAll( + container.querySelectorAll( "h1,h2,h3,h4,h5,h6,p,div,ul,dl,ol,li,dt,dd,pre,blockquote,address,kookitmarker" ) ) as HTMLElement[]; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/common.ts` around lines 30 - 36, The parameter currently named Element shadows the global type and has no type annotation; rename it (e.g., to root or container) and add an explicit DOM type such as Element | Document (or Element | Document | DocumentFragment if you need fragments) so getBlockElement signature becomes getBlockElement(root: Element | Document) and update any internal references from Element to the new name; keep the existing querySelectorAll call and return cast as HTMLElement[].src/utils/layoutUtil.ts (1)
152-155: Replace deprecatedsubstrwithsubstring.
String.prototype.substris deprecated. Usesubstringorsliceinstead.Suggested fix
let id = node.getAttribute("id") || - "kookit-trans-" + Math.random().toString(36).substr(2, 9); + "kookit-trans-" + Math.random().toString(36).substring(2, 11);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/layoutUtil.ts` around lines 152 - 155, The code assigns an id using Math.random().toString(36).substr(2, 9) which uses deprecated String.prototype.substr; update the id assignment in the block that references node.getAttribute and node.setAttribute so it uses substring or slice instead (e.g., Math.random().toString(36).substring(2, 11) or .slice(2, 11)) to produce the same 9-char suffix and then call node.setAttribute("id", id) as before.src/renders/GeneralRender.ts (1)
1453-1464: Consider accessibility implications offont-size: 0pxapproach.Setting
font-size: 0pxon original text effectively hides it visually while keeping it in the DOM. This may cause issues with:
- Screen readers still announcing the original text
- Text selection including hidden characters
An alternative like
visibility: hiddenwith absolute positioning, or usingaria-hidden="true", might provide better accessibility semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renders/GeneralRender.ts` around lines 1453 - 1464, The current approach in GeneralRender (check fullTranslationMode, the element variable, childElements from element.querySelectorAll, and the setAttribute calls) hides original text by setting font-size:0px which can break accessibility; instead, mark the original DOM nodes as hidden to assistive tech and visually hide them via a more accessible pattern: set aria-hidden="true" on element and each child element (using the same querySelectorAll/forEach loop), and replace the inline font-size:0px mutation with a non-intrusive visual-hide style (e.g., add a known CSS class or set style properties like visibility:hidden and position:absolute if needed) while preserving semantics — update the code paths that call element.setAttribute("style", ...) to set aria-hidden and apply the accessible visual-hide approach consistently for both element and childElements.src/helpers/styleHelper.ts (1)
26-28: Consider breaking up this complex CSS template literal for readability.This single-line template literal with multiple nested ternaries is difficult to read and maintain. Breaking it into intermediate variables or multiple lines would improve clarity.
Suggested refactor
+ // Build translation host ::after CSS + const translationEnabled = fullTranslationMode === "both" || fullTranslationMode === "target"; + const translationDisplay = translationEnabled ? this.getCustomCss(ConfigService) : "display:none;"; + const targetOnlyStyles = fullTranslationMode === "target" + ? `font-size: ${ConfigService.getReaderConfig("fontSize") || 18}px !important; text-indent: 2rem !important;` + : ""; cssRules.push( - `.kookit-translation-host::after{content: attr(data-kookit-translation);display:block;${fullTranslationMode === "both" || fullTranslationMode === "target" ? this.getCustomCss(ConfigService) : "display:none;"}${fullTranslationMode === "target" ? "font-size: " + (ConfigService.getReaderConfig("fontSize") || 18) + "px !important; text-indent: 2rem !important;" : ""} }` + `.kookit-translation-host::after{content: attr(data-kookit-translation);display:block;${translationDisplay}${targetOnlyStyles}}` );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers/styleHelper.ts` around lines 26 - 28, The CSS template literal pushed into cssRules via cssRules.push(...) is overly complex; refactor by extracting parts into clear intermediate variables (e.g., baseSelector = ".kookit-translation-host::after{content: attr(data-kookit-translation);display:block;"; customCss = (fullTranslationMode === "both" || fullTranslationMode === "target") ? this.getCustomCss(ConfigService) : ""; sizeAndIndent = fullTranslationMode === "target" ? "font-size: " + (ConfigService.getReaderConfig("fontSize") || 18) + "px !important; text-indent: 2rem !important;" : ""; then build the final string by joining those parts and push that into cssRules); keep references to cssRules.push, fullTranslationMode, this.getCustomCss, and ConfigService so the code is easy to locate and the conditional logic remains identical.
🤖 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/libs/textProcessor.ts`:
- Around line 85-98: The conditional inside the larger lines.length > 10000
block mistakenly rechecks lines.length > 1000 (which is always true) and
redeclares startIndex/endIndex/relevantLines, so update the condition to test
the intended narrower slice (e.g., use relevantLines.length > 1000 or a
different threshold like titlesInSlice.length < X) and rename the inner
variables to avoid shadowing (e.g., sliceStart, sliceEnd, narrowLines) while
keeping the same logic that builds htmlParts and returns the fallback HTML;
adjust only the condition and variable names in this block where startIndex,
endIndex, relevantLines, titlesInSlice, and htmlParts are used.
In `@src/utils/layoutUtil.ts`:
- Around line 143-165: The code in handlePrecacheAssets assumes window.transMap
exists when assigning window.transMap[originalText] inside the block gated by
window.fullTranslationMode; add a defensive null-safety check to ensure
window.transMap is initialized before use (e.g., if not present, set
window.transMap = {}), then assign window.transMap[originalText] = { id };
reference this change around the loop that creates ids and sets attributes (look
for getBlockElement, node.setAttribute, and window.transMap usage) and ensure
this complements how GeneralRender initializes window.transMap and does not
remove that initialization.
In `@src/utils/noteUtil.ts`:
- Around line 626-630: The inline style string set on iconNode via
iconNode.setAttribute is malformed ("display: width: 16px;") and should be
changed to valid CSS; update the style to include a proper display value (e.g.,
"display: inline-block;") followed by "width: 16px; height: 16px;" and preserve
the rest of the rules (z-index, font-size, line-height, cursor, pointer-events)
so the final string reads something like "position: relative; display:
inline-block; width: 16px; height: 16px; z-index: 2; font-size: 14px;
line-height: 1; cursor: pointer; pointer-events: auto;" when calling
iconNode.setAttribute.
In `@src/utils/touchUtil.ts`:
- Around line 595-600: The issue: getSelectionSentence(doc) is called after
awaiting render.getHightlightCoords(...) causing sentence to be re-read from
live selection and diverge from selectedText if the selection changes; fix by
"freezing" the sentence before any await. In both triggerSelectionMenu() and
addAppleTouchEvent(), call and store const sentence = getSelectionSentence(doc)
(and const selectedText = ...) before awaiting render.getHightlightCoords(...),
then use those frozen variables when building the payload (event/select-text,
selectedText, sentence, position, range) so the sent sentence always matches
selectedText. Ensure you update both locations mentioned.
- Around line 217-244: getSelectionSentence currently falls back to returning
fullText from the commonAncestorContainer which can be much larger than the
selection; instead extract the text that the range actually covers (use
range.cloneContents() or iterate text nodes in range to build the coveredText)
and find the surrounding sentence inside that coveredText (use selectedText to
locate the sentence), avoid using fullText from commonAncestorContainer as a
fallback, and enforce a safe maximum length (truncate coveredText/returned
sentence) before sending via postMessage; update the logic around sel, range,
commonAncestorContainer, fullText, selectedText and sentences accordingly.
---
Nitpick comments:
In `@src/helpers/styleHelper.ts`:
- Around line 26-28: The CSS template literal pushed into cssRules via
cssRules.push(...) is overly complex; refactor by extracting parts into clear
intermediate variables (e.g., baseSelector =
".kookit-translation-host::after{content:
attr(data-kookit-translation);display:block;"; customCss = (fullTranslationMode
=== "both" || fullTranslationMode === "target") ?
this.getCustomCss(ConfigService) : ""; sizeAndIndent = fullTranslationMode ===
"target" ? "font-size: " + (ConfigService.getReaderConfig("fontSize") || 18) +
"px !important; text-indent: 2rem !important;" : ""; then build the final string
by joining those parts and push that into cssRules); keep references to
cssRules.push, fullTranslationMode, this.getCustomCss, and ConfigService so the
code is easy to locate and the conditional logic remains identical.
In `@src/renders/GeneralRender.ts`:
- Around line 1453-1464: The current approach in GeneralRender (check
fullTranslationMode, the element variable, childElements from
element.querySelectorAll, and the setAttribute calls) hides original text by
setting font-size:0px which can break accessibility; instead, mark the original
DOM nodes as hidden to assistive tech and visually hide them via a more
accessible pattern: set aria-hidden="true" on element and each child element
(using the same querySelectorAll/forEach loop), and replace the inline
font-size:0px mutation with a non-intrusive visual-hide style (e.g., add a known
CSS class or set style properties like visibility:hidden and position:absolute
if needed) while preserving semantics — update the code paths that call
element.setAttribute("style", ...) to set aria-hidden and apply the accessible
visual-hide approach consistently for both element and childElements.
In `@src/utils/common.ts`:
- Around line 42-48: Remove the dead commented-out code that declares and uses
blockElementList and the conditional on its length in src/utils/common.ts;
locate the commented lines referencing blockElementList, blockRegex, and
children and delete them, or if the logic is intentionally preserved for
context, replace the commented block with a short explanatory comment stating
why the alternative approach was kept (e.g., "alternative: filter children by
blockRegex into blockElementList to check length") and move it to a higher-level
comment or a separate branch.
- Around line 30-36: The parameter currently named Element shadows the global
type and has no type annotation; rename it (e.g., to root or container) and add
an explicit DOM type such as Element | Document (or Element | Document |
DocumentFragment if you need fragments) so getBlockElement signature becomes
getBlockElement(root: Element | Document) and update any internal references
from Element to the new name; keep the existing querySelectorAll call and return
cast as HTMLElement[].
In `@src/utils/layoutUtil.ts`:
- Around line 152-155: The code assigns an id using
Math.random().toString(36).substr(2, 9) which uses deprecated
String.prototype.substr; update the id assignment in the block that references
node.getAttribute and node.setAttribute so it uses substring or slice instead
(e.g., Math.random().toString(36).substring(2, 11) or .slice(2, 11)) to produce
the same 9-char suffix and then call node.setAttribute("id", id) as before.
🪄 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: 51e910be-27f1-48b0-b6ca-aa36ebd1d2cc
📒 Files selected for processing (8)
src/helpers/styleHelper.tssrc/libs/textProcessor.tssrc/renders/GeneralRender.tssrc/utils/common.tssrc/utils/layoutUtil.tssrc/utils/navigationUtil.tssrc/utils/noteUtil.tssrc/utils/touchUtil.ts
Comment on lines
+85
to
+98
| if (lines.length > 1000 && titlesInSlice.length < 3) { | ||
| // If the file is very large and no titles are found, we might want to skip title processing to save time | ||
| const startIndex = Math.max(targetLineIndex - 500, 0); | ||
| const endIndex = Math.min(targetLineIndex + 500, lines.length); | ||
| const relevantLines = lines.slice(startIndex, endIndex); // Process only the relevant slice | ||
|
|
||
| for (const item of relevantLines) { | ||
| htmlParts.push(`<p>${item}</p>`); // Push to array | ||
| } | ||
| // Fallback if no HTML was generated | ||
| return `<h1 style="opacity: 0; font-size: 5px;">Title</h1>${htmlParts.join( | ||
| "" | ||
| )}`; | ||
| } |
Contributor
There was a problem hiding this comment.
Condition lines.length > 1000 is always true in this context.
This check is inside the lines.length > 10000 block (line 56), so lines.length > 1000 will always be true. Did you intend to check a different variable, such as relevantLines.length > 1000 or perhaps a different threshold for titlesInSlice.length?
Additionally, startIndex, endIndex, and relevantLines are redeclared here (shadowing lines 77-79), which reduces clarity. Consider using different variable names for the narrower window.
Suggested clarification
- if (lines.length > 1000 && titlesInSlice.length < 3) {
+ if (titlesInSlice.length < 3) {
// If the file is very large and no titles are found, we might want to skip title processing to save time
- const startIndex = Math.max(targetLineIndex - 500, 0);
- const endIndex = Math.min(targetLineIndex + 500, lines.length);
- const relevantLines = lines.slice(startIndex, endIndex); // Process only the relevant slice
+ const fallbackStart = Math.max(targetLineIndex - 500, 0);
+ const fallbackEnd = Math.min(targetLineIndex + 500, lines.length);
+ const fallbackLines = lines.slice(fallbackStart, fallbackEnd);
- for (const item of relevantLines) {
+ for (const item of fallbackLines) {
htmlParts.push(`<p>${item}</p>`); // Push to array
}📝 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 (lines.length > 1000 && titlesInSlice.length < 3) { | |
| // If the file is very large and no titles are found, we might want to skip title processing to save time | |
| const startIndex = Math.max(targetLineIndex - 500, 0); | |
| const endIndex = Math.min(targetLineIndex + 500, lines.length); | |
| const relevantLines = lines.slice(startIndex, endIndex); // Process only the relevant slice | |
| for (const item of relevantLines) { | |
| htmlParts.push(`<p>${item}</p>`); // Push to array | |
| } | |
| // Fallback if no HTML was generated | |
| return `<h1 style="opacity: 0; font-size: 5px;">Title</h1>${htmlParts.join( | |
| "" | |
| )}`; | |
| } | |
| if (titlesInSlice.length < 3) { | |
| // If the file is very large and no titles are found, we might want to skip title processing to save time | |
| const fallbackStart = Math.max(targetLineIndex - 500, 0); | |
| const fallbackEnd = Math.min(targetLineIndex + 500, lines.length); | |
| const fallbackLines = lines.slice(fallbackStart, fallbackEnd); | |
| for (const item of fallbackLines) { | |
| htmlParts.push(`<p>${item}</p>`); // Push to array | |
| } | |
| // Fallback if no HTML was generated | |
| return `<h1 style="opacity: 0; font-size: 5px;">Title</h1>${htmlParts.join( | |
| "" | |
| )}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libs/textProcessor.ts` around lines 85 - 98, The conditional inside the
larger lines.length > 10000 block mistakenly rechecks lines.length > 1000 (which
is always true) and redeclares startIndex/endIndex/relevantLines, so update the
condition to test the intended narrower slice (e.g., use relevantLines.length >
1000 or a different threshold like titlesInSlice.length < X) and rename the
inner variables to avoid shadowing (e.g., sliceStart, sliceEnd, narrowLines)
while keeping the same logic that builds htmlParts and returns the fallback
HTML; adjust only the condition and variable names in this block where
startIndex, endIndex, relevantLines, titlesInSlice, and htmlParts are used.
Comment on lines
+143
to
+165
| if ( | ||
| window.fullTranslationMode === "both" || | ||
| window.fullTranslationMode === "target" | ||
| ) { | ||
| let nodeList = getBlockElement(chapterDoc.body).filter( | ||
| (item) => !isParentBlock(item) | ||
| ); | ||
| for (let node of nodeList) { | ||
| if (node.textContent && node.textContent?.trim()) { | ||
| let id = | ||
| node.getAttribute("id") || | ||
| "kookit-trans-" + Math.random().toString(36).substr(2, 9); | ||
| node.setAttribute("id", id); | ||
| node.classList.add("kookit-translation-host"); | ||
| node.classList.add("kookit-translation-loading"); | ||
| node.setAttribute("data-kookit-translation", ""); | ||
| let originalText = node.textContent || ""; | ||
| window.transMap[originalText] = { | ||
| id, | ||
| }; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the initialization order and usage of window.transMap
echo "=== Checking window.transMap initialization ==="
rg -n "window\.transMap\s*=" --type ts
echo ""
echo "=== Checking where handlePrecacheAssets is called ==="
rg -n "handlePrecacheAssets" --type ts -A 2Repository: koodo-reader/kookit
Length of output: 670
🏁 Script executed:
# Get context around where handlePrecacheAssets is called
sed -n '95,115p' src/utils/layoutUtil.ts
# Get context around where window.transMap is initialized
sed -n '100,120p' src/renders/GeneralRender.ts
# Check what calls the function that contains handlePrecacheAssets at line 105
rg -n "const.*=.*await handlePrecacheAssets|handlePrecacheAssets.*await" src/utils/layoutUtil.ts -B 5Repository: koodo-reader/kookit
Length of output: 1568
🏁 Script executed:
# Find who calls handleOneChapterDoc
rg -n "handleOneChapterDoc" --type ts -B 2 -A 2
# Get more context on GeneralRender class definition and constructor
rg -n "class GeneralRender|constructor" src/renders/GeneralRender.ts -A 1
# Check if window.transMap is defined/declared elsewhere
rg -n "window\.transMap" --type tsRepository: koodo-reader/kookit
Length of output: 1498
🏁 Script executed:
# Find where GeneralRender is instantiated
rg -n "new GeneralRender" --type ts
# Check if there's a module-level instance or export
rg -n "export.*GeneralRender|GeneralRender.*=" --type ts | head -20
# Get full context of the GeneralRender class and its constructor
sed -n '39,130p' src/renders/GeneralRender.tsRepository: koodo-reader/kookit
Length of output: 3001
🏁 Script executed:
# Find who imports and uses navigationUtil
rg -n "navigationUtil|handleOneChapterDoc" --type ts --type tsx | grep -E "(import|from)" | head -10
# Search for the main app entry point and initialization
find . -name "index.tsx" -o -name "App.tsx" -o -name "main.ts" | head -5
# Check if there's a setup/init function
rg -n "new GeneralRender\|export.*GeneralRender" --type ts -B 2 -A 2Repository: koodo-reader/kookit
Length of output: 90
🏁 Script executed:
# Find who imports navigationUtil
rg -n "navigationUtil" --type ts | grep "import" | head -10
# Find app entry points
fd -e ts -e js | grep -E "(index|main|app|entry)" | head -10
# Search for where GeneralRender is instantiated or used
rg -n "new GeneralRender\|export.*GeneralRender" --type ts -B 2 -A 2
# Check window.transMap access points to see if there's a safety check
rg -n "window\.transMap" --type ts -B 2 -A 2Repository: koodo-reader/kookit
Length of output: 800
🏁 Script executed:
# Check the main index.ts file to understand module exports
cat src/index.ts
# Check if GeneralRender is imported in index.ts
rg -n "GeneralRender" src/index.ts -B 2 -A 2
# Search for any checks on window.transMap before accessing it
rg -n "window\.transMap\?" --type ts
# Check the complete layoutUtil.ts snippet around line 160
sed -n '140,170p' src/utils/layoutUtil.tsRepository: koodo-reader/kookit
Length of output: 1997
🏁 Script executed:
# Check the full handlePrecacheAssets function to see if window.transMap is initialized there
sed -n '111,200p' src/utils/layoutUtil.ts
# Check which render classes are exported and if they extend GeneralRender
rg -n "class.*Render.*extends" --type ts
# Look for any defensive checks or initialization of window.transMap elsewhere
rg -n "window\.transMap\s*=" --type ts -B 1 -A 1Repository: koodo-reader/kookit
Length of output: 4405
🏁 Script executed:
# Check if there's any initialization of window.transMap outside of GeneralRender
rg -n "window\.transMap" --type ts -B 3 -A 3
# Check if handlePrecacheAssets is exported and could be called independently
rg -n "export.*handlePrecacheAssets" --type ts -B 2 -A 2
# Verify the complete initialization sequence - check if window.fullTranslationMode is always initialized with window.transMap
rg -n "window\.fullTranslationMode" --type tsRepository: koodo-reader/kookit
Length of output: 1804
Add null-safety check for window.transMap before access.
The code accesses window.transMap[originalText] without verifying it exists. While window.transMap is initialized in the GeneralRender constructor alongside window.fullTranslationMode, the condition if (window.fullTranslationMode === "both" || window.fullTranslationMode === "target") does not guarantee window.transMap is defined—it only prevents execution if the mode is falsy or "no". Since handlePrecacheAssets is exported and could be called before any GeneralRender instance is created, add a defensive check or ensure initialization:
if (window.transMap) {
window.transMap[originalText] = { id };
} else {
window.transMap = window.transMap || {};
window.transMap[originalText] = { id };
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/layoutUtil.ts` around lines 143 - 165, The code in
handlePrecacheAssets assumes window.transMap exists when assigning
window.transMap[originalText] inside the block gated by
window.fullTranslationMode; add a defensive null-safety check to ensure
window.transMap is initialized before use (e.g., if not present, set
window.transMap = {}), then assign window.transMap[originalText] = { id };
reference this change around the loop that creates ids and sets attributes (look
for getBlockElement, node.setAttribute, and window.transMap usage) and ensure
this complements how GeneralRender initializes window.transMap and does not
remove that initialization.
Comment on lines
+217
to
+244
| const getSelectionSentence = (doc: any): string => { | ||
| let sel = doc.getSelection(); | ||
| if (!sel || !sel.toString().trim()) return ""; | ||
| try { | ||
| let range = sel.getRangeAt(0); | ||
| let container = range.commonAncestorContainer; | ||
| // Walk up to a text-containing element | ||
| let el: Node | null = | ||
| container.nodeType === Node.TEXT_NODE | ||
| ? container.parentElement | ||
| : container; | ||
| let fullText = (el as Element)?.textContent || ""; | ||
| let selectedText = sel.toString().trim(); | ||
| // Split on sentence-ending punctuation to find the sentence | ||
| let sentences = fullText.split(/(?<=[.!?。！？])\s*/); | ||
| for (let s of sentences) { | ||
| if (s.includes(selectedText)) { | ||
| return s.trim(); | ||
| } | ||
| } | ||
| // Fallback: return the whole text content of the container | ||
| return fullText.trim(); | ||
| } catch { | ||
| // ignore | ||
| } | ||
|
|
||
| return ""; | ||
| }; |
Contributor
There was a problem hiding this comment.
Don’t fall back to the entire ancestor text.
If the selection spans multiple inline nodes, commonAncestorContainer can widen well beyond the selected sentence. Returning fullText.trim() then sends the wrong value and an unbounded payload over postMessage.
🧩 Suggested fix
const getSelectionSentence = (doc: any): string => {
let sel = doc.getSelection();
if (!sel || !sel.toString().trim()) return "";
try {
let range = sel.getRangeAt(0);
@@
- let fullText = (el as Element)?.textContent || "";
- let selectedText = sel.toString().trim();
+ let fullText = (el as Element)?.textContent || "";
+ let selectedText = sel.toString().replace(/\s+/g, " ").trim();
// Split on sentence-ending punctuation to find the sentence
let sentences = fullText.split(/(?<=[.!?。！？])\s*/);
for (let s of sentences) {
- if (s.includes(selectedText)) {
+ if (s.replace(/\s+/g, " ").includes(selectedText)) {
return s.trim();
}
}
- // Fallback: return the whole text content of the container
- return fullText.trim();
+ // Fallback: keep the payload bounded when no sentence match is found
+ return selectedText;
} catch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/touchUtil.ts` around lines 217 - 244, getSelectionSentence
currently falls back to returning fullText from the commonAncestorContainer
which can be much larger than the selection; instead extract the text that the
range actually covers (use range.cloneContents() or iterate text nodes in range
to build the coveredText) and find the surrounding sentence inside that
coveredText (use selectedText to locate the sentence), avoid using fullText from
commonAncestorContainer as a fallback, and enforce a safe maximum length
(truncate coveredText/returned sentence) before sending via postMessage; update
the logic around sel, range, commonAncestorContainer, fullText, selectedText and
sentences accordingly.
Comment on lines
595
to
600
| event: "select-text", | ||
| selectedText: selectedText, | ||
| sentence: getSelectionSentence(doc), | ||
| position: position, | ||
| range: charRange, | ||
| }) |
Contributor
There was a problem hiding this comment.
Freeze sentence before the async highlight lookup.
These payloads are built after await render.getHightlightCoords(...), but getSelectionSentence(doc) rereads the live selection at send time. If the selection collapses or shifts during that await, selectedText and sentence diverge.
🧩 Suggested fix
var range = iWin.getSelection().getRangeAt(0);
+ const sentence = getSelectionSentence(doc);
...
window.ReactNativeWebView.postMessage(
JSON.stringify({
event: "select-text",
selectedText: selectedText,
- sentence: getSelectionSentence(doc),
+ sentence: sentence,
position: position,
range: charRange,
})
);Apply the same change in both triggerSelectionMenu() and addAppleTouchEvent().
Also applies to: 846-851
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/touchUtil.ts` around lines 595 - 600, The issue:
getSelectionSentence(doc) is called after awaiting
render.getHightlightCoords(...) causing sentence to be re-read from live
selection and diverge from selectedText if the selection changes; fix by
"freezing" the sentence before any await. In both triggerSelectionMenu() and
addAppleTouchEvent(), call and store const sentence = getSelectionSentence(doc)
(and const selectedText = ...) before awaiting render.getHightlightCoords(...),
then use those frozen variables when building the payload (event/select-text,
selectedText, sentence, position, range) so the sent sentence always matches
selectedText. Ensure you update both locations mentioned.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renders/GeneralRender.ts (1)
1301-1321:⚠️ Potential issue | 🔴 CriticalNon-footnote cross-chapter MOBI links are not navigated.
At line 1321, the code returns
{ handled: true }for non-footnote MOBI links that reference a different chapter (whenresult.index !== currentPosition.chapterDocIndex), but there is no call togoToChapter()or equivalent navigation. This silently consumes the link event without navigating to the target chapter.Compare this with footnote handling (lines 1301–1319), which performs proper anchor resolution and node positioning, and with other cross-chapter links (lines 1327–1331), which call
goToChapter(). The non-footnote case should similarly navigate to the target chapter usinggoToChapter(result.index, ...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renders/GeneralRender.ts` around lines 1301 - 1321, The non-footnote MOBI link case currently returns `{ handled: true }` and swallows cross-chapter navigation; update the non-footnote branch so when `result.index !== currentPosition.chapterDocIndex` it calls `goToChapter(result.index, /* appropriate position/anchor */)` (similar to how footnotes build the `node` via `result.anchor` and how other cross-chapter links call `goToChapter`) or otherwise resolve the anchor and navigate to that chapter using `chapterDocList`/`result.anchor` before returning; ensure you still return the proper object shape (e.g., handled/ isJump/ href/node) after navigation.
🧹 Nitpick comments (1)
src/renders/GeneralRender.ts (1)
1437-1471:handleBatchTransResultsilently fails if element not found.When
elementis null (line 1447), the translation text is still stored intransMapbut no DOM update occurs. Consider logging a warning for debugging purposes.Additionally, the
querySelectorAll("*")on line 1459 could be expensive for elements with many descendants. Consider if a more targeted approach is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renders/GeneralRender.ts` around lines 1437 - 1471, The method handleBatchTransResult stores translations in this.transMap even when the corresponding DOM node can't be found, and it also uses querySelectorAll("*") which can be expensive; update handleBatchTransResult to log a warning (e.g., via console.warn or the project's logger) when doc.querySelector("#" + this.transMap[sourceText].id) returns null so missing DOM updates are detectable, and replace the broad element.querySelectorAll("*") traversal (used when this.fullTranslationMode === "target") with a more targeted/limited selection (e.g., only direct children or a bounded selector) to avoid expensive full-descendant walks—refer to symbols handleBatchTransResult, this.transMap, this.fullTranslationMode, element, and querySelectorAll in your change.
🤖 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/GeneralRender.ts`:
- Around line 956-970: The getBatchTransTexts method uses filter with a
side-effecting totalLength accumulator which can yield inconsistent results;
replace that filter with an explicit loop over the sliced array (after
audioText() and .slice(0,100)) that iterates items, trims them, skips empty
strings and items already present in this.transMap (check
this.transMap[item]?.text), and pushes each item while maintaining totalLength
and breaking once totalLength + item.length would exceed 10000; this ensures
deterministic selection and preserves the existing slice(0,100) and final
trimming logic.
---
Outside diff comments:
In `@src/renders/GeneralRender.ts`:
- Around line 1301-1321: The non-footnote MOBI link case currently returns `{
handled: true }` and swallows cross-chapter navigation; update the non-footnote
branch so when `result.index !== currentPosition.chapterDocIndex` it calls
`goToChapter(result.index, /* appropriate position/anchor */)` (similar to how
footnotes build the `node` via `result.anchor` and how other cross-chapter links
call `goToChapter`) or otherwise resolve the anchor and navigate to that chapter
using `chapterDocList`/`result.anchor` before returning; ensure you still return
the proper object shape (e.g., handled/ isJump/ href/node) after navigation.
---
Nitpick comments:
In `@src/renders/GeneralRender.ts`:
- Around line 1437-1471: The method handleBatchTransResult stores translations
in this.transMap even when the corresponding DOM node can't be found, and it
also uses querySelectorAll("*") which can be expensive; update
handleBatchTransResult to log a warning (e.g., via console.warn or the project's
logger) when doc.querySelector("#" + this.transMap[sourceText].id) returns null
so missing DOM updates are detectable, and replace the broad
element.querySelectorAll("*") traversal (used when this.fullTranslationMode ===
"target") with a more targeted/limited selection (e.g., only direct children or
a bounded selector) to avoid expensive full-descendant walks—refer to symbols
handleBatchTransResult, this.transMap, this.fullTranslationMode, element, and
querySelectorAll in your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf7b4ccb-eed9-4e25-9380-282db8550610
📒 Files selected for processing (5)
.gitignoresrc/helpers/styleHelper.tssrc/renders/GeneralRender.tssrc/utils/noteUtil.tssrc/utils/touchUtil.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/touchUtil.ts
Comment on lines
+956
to
+970
| async getBatchTransTexts() { | ||
| let restTexts: string[] = (await this.audioText()) as string[]; | ||
|
|
||
| restTexts = restTexts.slice(0, 100); | ||
| //同时确保总字数不超过10000字 | ||
| let totalLength = 0; | ||
| restTexts = restTexts.filter((item) => { | ||
| totalLength += item.length; | ||
| return totalLength <= 10000; | ||
| }); | ||
| restTexts = restTexts.filter( | ||
| (item) => !this.transMap[item] || !this.transMap[item].text | ||
| ); | ||
| return restTexts.filter((item) => item.trim().length > 0); | ||
| } |
Contributor
There was a problem hiding this comment.
Potential issue: filter after length accumulation may produce inconsistent results.
The filtering logic on lines 962-965 accumulates totalLength for all items but then filters based on whether totalLength <= 10000. This means the filter callback increments totalLength for every item, even those that exceed the limit, which could lead to fewer items than expected.
Consider using a more explicit loop:
Proposed fix
async getBatchTransTexts() {
let restTexts: string[] = (await this.audioText()) as string[];
restTexts = restTexts.slice(0, 100);
- //同时确保总字数不超过10000字
- let totalLength = 0;
- restTexts = restTexts.filter((item) => {
- totalLength += item.length;
- return totalLength <= 10000;
- });
+ // Ensure total character count doesn't exceed 10000
+ let totalLength = 0;
+ const limitedTexts: string[] = [];
+ for (const item of restTexts) {
+ if (totalLength + item.length > 10000) break;
+ totalLength += item.length;
+ limitedTexts.push(item);
+ }
+ restTexts = limitedTexts;
restTexts = restTexts.filter(
(item) => !this.transMap[item] || !this.transMap[item].text
);
return restTexts.filter((item) => item.trim().length > 0);
}📝 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
| async getBatchTransTexts() { | |
| let restTexts: string[] = (await this.audioText()) as string[]; | |
| restTexts = restTexts.slice(0, 100); | |
| //同时确保总字数不超过10000字 | |
| let totalLength = 0; | |
| restTexts = restTexts.filter((item) => { | |
| totalLength += item.length; | |
| return totalLength <= 10000; | |
| }); | |
| restTexts = restTexts.filter( | |
| (item) => !this.transMap[item] || !this.transMap[item].text | |
| ); | |
| return restTexts.filter((item) => item.trim().length > 0); | |
| } | |
| async getBatchTransTexts() { | |
| let restTexts: string[] = (await this.audioText()) as string[]; | |
| restTexts = restTexts.slice(0, 100); | |
| // Ensure total character count doesn't exceed 10000 | |
| let totalLength = 0; | |
| const limitedTexts: string[] = []; | |
| for (const item of restTexts) { | |
| if (totalLength + item.length > 10000) break; | |
| totalLength += item.length; | |
| limitedTexts.push(item); | |
| } | |
| restTexts = limitedTexts; | |
| restTexts = restTexts.filter( | |
| (item) => !this.transMap[item] || !this.transMap[item].text | |
| ); | |
| return restTexts.filter((item) => item.trim().length > 0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renders/GeneralRender.ts` around lines 956 - 970, The getBatchTransTexts
method uses filter with a side-effecting totalLength accumulator which can yield
inconsistent results; replace that filter with an explicit loop over the sliced
array (after audioText() and .slice(0,100)) that iterates items, trims them,
skips empty strings and items already present in this.transMap (check
this.transMap[item]?.text), and pushes each item while maintaining totalLength
and breaking once totalLength + item.length would exceed 10000; this ensures
deterministic selection and preserves the existing slice(0,100) and final
trimming logic.
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
Bug Fixes