Migrate to Vite, add PWA/offline support, and fix table-of-contents bug (v2.1.0) #2
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a Vite/PWA-based build and runtime setup with ESLint config, service worker, Workbox bundle, and npm scripts. Introduces a new reader UI shell (HTML/CSS), modularizes app logic (book loading, library, IndexedDB, UI glue), replaces root index.html with external assets, updates manifest files, and switches license to MIT. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant H as HTML (UI)
participant M as main.js
participant B as book.js
participant E as epubjs
U->>H: Select EPUB file
H->>M: change(file-input)
M->>B: openBook(event)
B->>B: Validate .epub / MIME
B->>B: Read as ArrayBuffer
B->>E: new ePub(buffer)
B->>E: generate TOC + locations
E-->>B: TOC, locations ready
B-->>M: Display promise
M-->>H: Update viewer, enable controls
Note over H,M: Loading shown/hidden and errors handled centrally
sequenceDiagram
autonumber
actor U as User
participant H as HTML (UI)
participant M as main.js
participant L as library.js
participant IDB as indexedDB.js
participant FS as File System Access API
participant B as book.js
U->>H: Click "Library"
H->>M: click(library-button)
M->>L: openLibrary()
L->>IDB: getStoredLibraryHandle()
alt handle exists and granted
L->>FS: iterate directory for *.epub
else no handle or denied
alt Directory picker available
L->>FS: showDirectoryPicker()
L->>IDB: storeLibraryHandle(handle)
L->>FS: iterate *.epub
else Fallback
L->>H: trigger hidden file input
end
end
L-->>H: Render library grid
U->>H: Click library item
H->>L: select entry
L->>B: openBookFromEntry(entry)
B-->>H: Load and display book
sequenceDiagram
autonumber
participant H as HTML
participant R as registerSW.js
participant SW as sw.js (Workbox)
participant WB as workbox-3bd99cbd.js
H-->>R: window load
R->>SW: navigator.serviceWorker.register('./sw.js')
SW->>SW: skipWaiting() / clientsClaim()
SW->>WB: precacheAndRoute(assets)
SW->>H: Handle navigation requests (NetworkFirst)
SW->>H: Serve assets (CacheFirst)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (5)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
1 similar comment
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Docstrings generation was requested by @NellowTCS. * #2 (comment) The following files were modified: * `Build/src/book.js` * `Build/src/indexedDB.js` * `Build/src/library.js` * `Build/src/main.js` * `workbox-3bd99cbd.js`
|
Note Generated docstrings for this pull request at #3 |
📝 Add docstrings to `v2.1.0`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Build/.github/dependabot.yml (1)
1-12: Dependabot config won’t be picked up from Build/.github — move it to .github/dependabot.yml at repo rootGitHub only discovers dependabot.yml under the repository’s .github directory at the root. Placing it under Build/.github prevents Dependabot from running.
Suggested action:
- Move this file to .github/dependabot.yml at the repository root (same content is fine).
- Optional: add labels/reviewers and a timezone/weekday for scheduling to match your triage cadence.
Build/LICENSE (1)
1-22: License mismatch (MIT here vs GPLv3 noted elsewhere) — resolve to a single licenseAI summary indicates Build/README.md mentions GPLv3 while this file states MIT. Mixed licensing creates legal ambiguity for contributors and downstream users.
Actions:
- Decide the definitive license (MIT vs GPLv3).
- Ensure a single LICENSE at repo root reflects it.
- Align Build/package.json "license" field and any README badges/text.
- Remove duplicate/contradictory license files.
🧹 Nitpick comments (36)
Build/export.mjs (1)
24-37: Optional: add basic error handling/logging around copy loopImproves debuggability during CI exports.
- fs.copySync(srcPath, destPath, { overwrite: true }); - console.log('Copied:', item); + try { + fs.copySync(srcPath, destPath, { overwrite: true, errorOnExist: false }); + console.log('Copied:', item); + } catch (e) { + console.error('Copy failed:', { item, srcPath, destPath, error: e.message }); + process.exitCode = 1; + }Build/src/indexedDB.js (3)
1-11: Guard for unsupported browsers and blocked upgrades.Add a feature check and handle onblocked to fail fast with a clear error.
function getDB() { return new Promise((resolve, reject) => { + if (!('indexedDB' in window)) { + return reject(new Error('IndexedDB is not supported in this browser')); + } const request = indexedDB.open("htmlreader-db", 1); request.onupgradeneeded = e => { const db = e.target.result; db.createObjectStore("handles", { keyPath: "name" }); }; request.onsuccess = e => resolve(e.target.result); request.onerror = e => reject(e.target.error); + request.onblocked = () => reject(new Error('IndexedDB upgrade blocked by another open tab')); }); }
12-21: Resolve on transaction completion, not request success.Commit durability is only guaranteed on tx completion. Also surface tx abort errors.
export async function storeLibraryHandle(handle) { const db = await getDB(); return new Promise((resolve, reject) => { const tx = db.transaction("handles", "readwrite"); const store = tx.objectStore("handles"); - const req = store.put({ name: "library", handle }); - req.onsuccess = () => resolve(); - req.onerror = e => reject(e.target.error); + store.put({ name: "library", handle }).onerror = e => reject(e.target.error); + tx.oncomplete = () => resolve(); + tx.onabort = () => reject(tx.error || new Error('Transaction aborted')); + tx.onerror = () => reject(tx.error); }); }
22-31: Return the value after the read-only transaction completes.Avoid reading the value before the tx commits; also handle aborts.
export async function getStoredLibraryHandle() { const db = await getDB(); return new Promise((resolve, reject) => { const tx = db.transaction("handles", "readonly"); const store = tx.objectStore("handles"); - const req = store.get("library"); - req.onsuccess = () => resolve(req.result ? req.result.handle : null); - req.onerror = e => reject(e.target.error); + const req = store.get("library"); + let value = null; + req.onsuccess = () => { value = req.result ? req.result.handle : null; }; + req.onerror = e => reject(e.target.error); + tx.oncomplete = () => resolve(value); + tx.onabort = () => reject(tx.error || new Error('Transaction aborted')); + tx.onerror = () => reject(tx.error); }); }Build/package.json (1)
1-45: Move Vite to devDependencies and add safety metadata.Vite is a build-time tool; keeping it in dependencies bloats installs. Also, add engines/private to prevent accidental publish and document Node requirement.
{ "name": "htmlreader", "version": "2.1.0", "description": "A quick HTML-based ePub Reader that works offline.", "main": "index.js", + "private": true, + "engines": { "node": ">=18" }, "scripts": { "test": "echo \"Error: no test specified\" && exit 1", "dev": "vite", "build": "vite build", "preview": "vite preview", "export": "node export.mjs" }, "repository": { "type": "git", "url": "git+https://github.com/HTMLToolkit/HTMLReader.git" }, "keywords": [ "offline", "pwa", "html-css-javascript", "htmltoolkit" ], "author": "NellowTCS", "license": "MIT", "bugs": { "url": "https://github.com/HTMLToolkit/HTMLReader/issues" }, "homepage": "https://github.com/HTMLToolkit/HTMLReader#readme", "dependencies": { "epubjs": "^0.3.93", - "jszip": "^3.10.1", - "vite": "^7.1.3" + "jszip": "^3.10.1" }, "devDependencies": { + "vite": "^7.1.3", "@eslint/css": "^0.11.0", "@eslint/js": "^9.34.0", "@eslint/json": "^0.13.2", "@eslint/markdown": "^7.2.0", "eslint": "^9.34.0", "fs-extra": "^11.3.1", "globals": "^16.3.0", "rimraf": "^6.0.1", "vite-plugin-pwa": "^1.0.3" } }Build/README.md (2)
62-63: Outdated reference to Webpack after migration to Vite.Replace mention of Webpack with Vite to avoid confusion.
- Due to the Webpack build being rather large, I recommend using the website, but you do you :D. </br> + Due to the Vite build output being optimized for deployment, I recommend using the website, but you do you :D. </br>
156-158: Add languages to fenced code blocks (markdownlint MD040).Tag shell code fences for better readability and lint compliance.
- ``` + ```bash ./build.sh ``` - ``` + ```bash ./build-all.sh ```Also applies to: 160-162
Build/vite.config.mjs (2)
9-19: Ensure SW registration is injected and app name matches branding.
- Consider explicitly setting injectRegister to avoid relying on defaults.
- “HTMLReader” may conflict with README branding.
plugins: [ VitePWA({ - registerType: 'autoUpdate', + registerType: 'autoUpdate', + injectRegister: 'auto', includeAssets: ['robots.txt'], manifest: { - name: 'HTMLReader', - short_name: 'HTMLReader', + name: 'HTMLRunner', + short_name: 'HTMLRunner', start_url: './', display: 'standalone', background_color: '#f5f5f5', theme_color: '#2196F3', },
26-37: Tweak Workbox strategies to reduce staleness and include fonts.JSON often benefits from SWR; add fonts/images.
runtimeCaching: [ { urlPattern: /.*\.(js|css|html)$/, handler: 'NetworkFirst', options: { cacheName: 'app-shell' }, }, { - urlPattern: /.*\.(png|ico|json)$/, - handler: 'CacheFirst', - options: { cacheName: 'assets' }, + urlPattern: /.*\.(png|jpg|jpeg|svg|ico|webp)$/, + handler: 'CacheFirst', + options: { cacheName: 'assets' }, }, + { + urlPattern: /.*\.(json)$/, + handler: 'StaleWhileRevalidate', + options: { cacheName: 'api-json' }, + }, + { + urlPattern: /.*\.(woff2?|ttf|otf)$/, + handler: 'CacheFirst', + options: { cacheName: 'fonts' }, + }, ],Build/eslint.config.mjs (1)
8-15: Ignore build artifacts.Add a top-level ignore to keep lint fast and noise-free.
export default defineConfig([ + { ignores: ["dist", "node_modules"] }, { files: ["**/*.{js,mjs,cjs}"], plugins: { js }, extends: ["js/recommended"], languageOptions: { globals: globals.browser } }, { files: ["**/*.json"], plugins: { json }, language: "json/json", extends: ["json/recommended"] }, { files: ["**/*.jsonc"], plugins: { json }, language: "json/jsonc", extends: ["json/recommended"] }, { files: ["**/*.json5"], plugins: { json }, language: "json/json5", extends: ["json/recommended"] }, { files: ["**/*.md"], plugins: { markdown }, language: "markdown/gfm", extends: ["markdown/recommended"] }, { files: ["**/*.css"], plugins: { css }, language: "css/css", extends: ["css/recommended"] }, ]);Build/.github/workflows/codeql.yml (1)
45-50: Scope languages appropriately.If the repo has no Actions code, consider removing “actions” from the matrix to reduce CI time.
Build/src/style.css (2)
112-116: Duplicate z-index; earlier value is dead.Remove the first z-index to avoid confusion.
transition: transform 0.3s ease; - z-index: 10; display: flex; flex-direction: column; z-index: 1010;
212-225: Consider prefers-color-scheme for better UX.Optional: add a dark theme block keyed off the user’s OS setting.
Build/styles.css (2)
112-120: Prefer class-based visibility toggles over inline style selectors.
:not([style*="display: none"])is brittle; toggle a class instead.-.editor-container:not([style*="display: none"]) { - display: block; -} +/* Add/remove .visible via JS */ +.editor-container.visible { + display: block; +}JS (outside this file):
- Replace element.style.display toggles with element.classList.toggle('visible', true/false).
1-14: Namespace styles to avoid collisions.These global element selectors (
body,.controls,.header) may conflict with the book UI. Consider prefixing with a scope class on the app root (e.g.,.editor-app body→.editor-app .header, etc.).assets/index-D7CYcCYQ.css (1)
1-1: Nit: prefer translateX for clarity.translate(-100%) works, but translateX(-100%) reads clearer in team code reviews.
Apply this diff:
-transform:translate(-100%); +transform:translateX(-100%);workbox-3bd99cbd.js (1)
2-2: Optional: remove source map reference or ship the map.If the map isn’t deployed, the browser will 404 it on every load.
Apply this diff to remove the reference (or commit the map):
-//# sourceMappingURL=workbox-3bd99cbd.js.mapregisterSW.js (1)
1-1: Service worker scope and Vite base are consistent — no immediate change required.
Found base: './' in Build/vite.config.mjs and "scope":"./" in manifest.webmanifest; the service worker registers with scope './' — consistent. If deploying under a subpath (e.g., GitHub Pages repo), set Vite base to '//' and update or omit the explicit serviceWorker scope.index.html (1)
4-4: Tighten CSP for production (now that assets are bundled).The current CSP allows 'unsafe-inline' and many CDNs. If runtime no longer needs them, restrict to self and required schemes.
Apply this diff (example hardened CSP—adjust as needed):
- <meta http-equiv="Content-Security-Policy" content="default-src 'self' 'unsafe-inline' 'unsafe-eval' data: blob: https://cdnjs.cloudflare.com https://cdn.jsdelivr.net https://code.jquery.com https://unpkg.com https://d3js.org https://threejs.org https://cdn.plot.ly https://stackpath.bootstrapcdn.com https://maps.googleapis.com https://cdn.tailwindcss.com https://ajax.googleapis.com https://kit.fontawesome.com https://cdn.datatables.net https://maxcdn.bootstrapcdn.com https://code.highcharts.com; frame-src 'self'; child-src 'self'; manifest-src 'self'; worker-src 'self'; upgrade-insecure-requests; block-all-mixed-content;"> + <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'; style-src 'self'; img-src 'self' data: blob:; font-src 'self'; connect-src 'self'; frame-src 'self'; worker-src 'self'; manifest-src 'self'; upgrade-insecure-requests; block-all-mixed-content">Build/src/main.js (1)
30-30: Use closeToc() for the “Close” button instead of toggle.Deterministic close avoids accidental re-open on edge cases.
Apply this diff:
-closeTocButton.addEventListener('click', toggleToc); +closeTocButton.addEventListener('click', closeToc);sw.js (3)
1-1: Avoid overlapping HTML caching with NavigationRoute.You already handle navigations via NavigationRoute to index.html. The regex route that includes html (/(js|css|html)$/ with NetworkFirst) can conflict or be redundant. Drop html from that pattern and prefer StaleWhileRevalidate for JS/CSS to improve performance.
Apply conceptually:
- e.registerRoute(/.*\.(js|css|html)$/, new e.NetworkFirst({cacheName:"app-shell",plugins:[]}), "GET") + e.registerRoute(/.*\.(js|css)$/, new e.StaleWhileRevalidate({cacheName:"app-shell",plugins:[]}), "GET")
1-1: Don’t cache JSON with CacheFirst.CacheFirst on JSON (includes manifest.webmanifest) risks long-lived staleness. Restrict to images/icons or add expiration.
Example:
- e.registerRoute(/.*\.(png|ico|json)$/, new e.CacheFirst({cacheName:"assets",plugins:[]}), "GET") + e.registerRoute(/.*\.(png|ico)$/, new e.CacheFirst({ + cacheName:"assets", + plugins:[new e.ExpirationPlugin({maxEntries:100, maxAgeSeconds: 30*24*3600})] +}), "GET")
2-2: Remove source map comment in production.Keep maps in builds you control; otherwise drop them to avoid leaking internals.
-//# sourceMappingURL=sw.js.map +// (source map comment removed for production)Build/index.html (4)
4-4: CSP is overly permissive; tighten for prod."default-src 'self' 'unsafe-inline' 'unsafe-eval' data: blob:" plus many CDNs is broad. Remove unused hosts, drop 'unsafe-eval', and prefer nonces/hashes for any required inline. Keep worker-src/manifest-src explicit.
Example minimal baseline:
-<meta http-equiv="Content-Security-Policy" content="default-src 'self' 'unsafe-inline' 'unsafe-eval' data: blob: https://cdnjs.cloudflare.com ... ; frame-src 'self'; child-src 'self'; manifest-src 'self'; worker-src 'self'; upgrade-insecure-requests; block-all-mixed-content;"> +<meta http-equiv="Content-Security-Policy" content=" + default-src 'self' data: blob:; + script-src 'self'; + style-src 'self'; + img-src 'self' data: blob:; + connect-src 'self'; + frame-src 'self'; + worker-src 'self'; + manifest-src 'self'; + upgrade-insecure-requests; block-all-mixed-content;">
65-67: Move the module script inside the body.The script tag is outside . Place it before for valid markup and to avoid CSP/UA quirks.
-</body> -<script type="module" src="./src/main.js"></script> -</html> +<script type="module" src="./src/main.js"></script> +</body> +</html>
17-24: Add ARIA wiring for toggles and hidden controls.Provide aria-controls/aria-expanded on TOC/Library buttons and install button for better a11y. Also consider disabling file inputs until JS wires listeners.
-<button id="toc-button" disabled>Table of Contents</button> +<button id="toc-button" aria-controls="toc-container" aria-expanded="false" disabled>Table of Contents</button> -<button id="library-button">Library</button> +<button id="library-button" aria-controls="library-container" aria-expanded="false">Library</button> -<button id="install-button" hidden>Install App</button> +<button id="install-button" aria-hidden="true" hidden>Install App</button>
30-36: Constrain current-page input at runtime.Set max to total pages when locations are generated to enable native validation and better UX.
Hook in code (book.js after generateLocations):
currentPageInput.max = String(book.locations.length());Build/src/book.js (4)
87-90: Enable TOC button only when TOC exists.Some EPUBs lack nav/toc. Gate enabling accordingly.
- prevButton.disabled = false; - nextButton.disabled = false; - tocButton.disabled = false; + prevButton.disabled = false; + nextButton.disabled = false; + try { + const toc = await book.navigation.toc; + tocButton.disabled = !toc || toc.length === 0; + } catch { tocButton.disabled = true; }
127-137: Robust TOC label extraction.item.label can be an object in some EPUB.js versions. Fallback to a safe string.
- tocItem.textContent = item.label; + const label = typeof item.label === 'string' + ? item.label + : (item.label?.trim?.() ?? item.label?.text ?? item.label?.content ?? 'Untitled'); + tocItem.textContent = label;
45-48: Error string from FileReader may be opaque.Ensure a useful message.
- reader.onerror = function(e) { - hideLoading(); - showError('Error reading file: ' + e.target.error); - }; + reader.onerror = function(e) { + hideLoading(); + const err = e?.target?.error; + showError('Error reading file: ' + (err?.message || String(err || 'unknown error'))); + };
10-11: Remove unused currentLocation or use it.Either use currentLocation or drop it and the eslint disable.
-// eslint-disable-next-line no-unused-vars -let currentLocation = 0; +let currentLocationCfi = null;Build/src/library.js (5)
21-25: Case-insensitive EPUB filter.Ensure “.EPUB” matches.
- if (entry.kind === 'file' && entry.name.endsWith('.epub')) { + if (entry.kind === 'file' && entry.name.toLowerCase().endsWith('.epub')) { files.push(entry); }
39-51: Load library items concurrently for snappier UI.Serial EPUB parsing is slow with many files.
- for (const entry of fileEntries) { - const item = await createLibraryItem(entry); - libraryContent.appendChild(item); - } + const items = await Promise.all(fileEntries.map(entry => createLibraryItem(entry))); + for (const item of items) libraryContent.appendChild(item);
71-86: Free resources after cover/metadata extraction.Destroy tempBook to release memory; also guard for failures.
- const tempBook = ePub(arrayBuffer); + const tempBook = ePub(arrayBuffer); // Attempt to retrieve cover image URL const coverUrl = await tempBook.coverUrl(); @@ - const metadata = await tempBook.loaded.metadata; + const metadata = await tempBook.loaded.metadata; @@ - } catch (err) { + } catch (err) { console.error('Error loading cover for', fileEntry.name, err); - } + } finally { + try { await tempBook?.destroy?.(); } catch {} + }
96-107: Overlay re-use is fine; consider click-to-dismiss.Optional: clicking overlay could close the active panel (TOC/Library) for better UX.
overlay.addEventListener('click', () => toggleLibrary(false));
33-37: Filter non-EPUBs in fallback.When using file input, filter for .epub to avoid noise.
export function handleLibraryFiles(e) { - const files = Array.from(e.target.files); + const files = Array.from(e.target.files).filter(f => f.name.toLowerCase().endsWith('.epub')); displayLibraryGrid(files); toggleLibrary(true); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
Build/favicon.pngis excluded by!**/*.pngBuild/package-lock.jsonis excluded by!**/package-lock.jsonBuild/public/source.svgis excluded by!**/*.svgapple-touch-icon-180x180.pngis excluded by!**/*.pngassets/index-D-qNE1dZ.js.mapis excluded by!**/*.mapfavicon.icois excluded by!**/*.icofavicon.pngis excluded by!**/*.pngmaskable-icon-512x512.pngis excluded by!**/*.pngpwa-192x192.pngis excluded by!**/*.pngpwa-512x512.pngis excluded by!**/*.pngpwa-64x64.pngis excluded by!**/*.pngsource.svgis excluded by!**/*.svgsw.js.mapis excluded by!**/*.mapworkbox-3bd99cbd.js.mapis excluded by!**/*.map
📒 Files selected for processing (26)
Build/.github/dependabot.yml(1 hunks)Build/.github/workflows/codeql.yml(1 hunks)Build/.github/workflows/static.yml(1 hunks)Build/.gitignore(1 hunks)Build/LICENSE(1 hunks)Build/README.md(1 hunks)Build/eslint.config.mjs(1 hunks)Build/export.mjs(1 hunks)Build/index.html(1 hunks)Build/manifest.json(1 hunks)Build/package.json(1 hunks)Build/service-worker.js(1 hunks)Build/src/book.js(1 hunks)Build/src/indexedDB.js(1 hunks)Build/src/library.js(1 hunks)Build/src/main.js(1 hunks)Build/src/style.css(1 hunks)Build/styles.css(1 hunks)Build/vite.config.mjs(1 hunks)assets/index-D7CYcCYQ.css(1 hunks)index.html(2 hunks)manifest.webmanifest(1 hunks)registerSW.js(1 hunks)service-worker.js(0 hunks)sw.js(1 hunks)workbox-3bd99cbd.js(1 hunks)
💤 Files with no reviewable changes (1)
- service-worker.js
🧰 Additional context used
🧬 Code graph analysis (4)
Build/src/main.js (2)
Build/src/book.js (8)
currentPageInput(18-18)overlay(19-19)openBook(27-50)prevPage(143-145)nextPage(147-149)goToPage(151-158)toggleToc(166-169)closeToc(171-174)Build/src/library.js (4)
overlay(9-9)openLibrary(11-31)toggleLibrary(96-107)handleLibraryFiles(33-37)
Build/src/library.js (3)
Build/src/book.js (4)
file(28-28)file(59-59)arrayBuffer(60-60)openBookFromEntry(54-69)Build/src/main.js (1)
showError(49-52)Build/src/indexedDB.js (2)
getStoredLibraryHandle(22-31)storeLibraryHandle(12-21)
workbox-3bd99cbd.js (1)
sw.js (5)
e(1-1)n(1-1)r(1-1)o(1-1)l(1-1)
Build/src/book.js (2)
Build/src/main.js (4)
overlay(15-15)showError(49-52)showLoading(43-45)hideLoading(46-48)Build/src/library.js (2)
overlay(9-9)toggleLibrary(96-107)
🪛 Biome (2.1.2)
Build/src/style.css
[error] 115-115: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
z-index is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
workbox-3bd99cbd.js
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🪛 markdownlint-cli2 (0.17.2)
Build/README.md
156-156: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
160-160: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (11)
Build/service-worker.js (1)
1-15: ```shell
#!/bin/bash
set -euo pipefailecho "PWD: $(pwd)"
echo
echo "=== Top-level files ==="
ls -la | sed -n '1,200p'
echoecho "=== Searching for service-worker files & references ==="
rg -n --hidden --no-ignore -S "service-?worker|navigator.serviceWorker|virtual:pwa-register|vite-plugin-pwa|VitePWA|registerSW" || true
echoecho "=== Show Build/service-worker.js if present ==="
if [ -f Build/service-worker.js ]; then
echo "---- Build/service-worker.js (first 200 lines) ----"
sed -n '1,200p' Build/service-worker.js
else
echo "Build/service-worker.js not found"
fi
echoecho "=== List Build/ (if exists) ==="
if [ -d Build ]; then
ls -la Build | sed -n '1,200p'
else
echo "Build/ directory not found"
fi
echoecho "=== Show vite config and package.json snippets ==="
rg -n --hidden --no-ignore -S "vite.config|vite.config|VitePWA|vite-plugin-pwa" || true
if [ -f vite.config.js ]; then
echo "---- vite.config.js ----"
sed -n '1,200p' vite.config.js
elif [ -f vite.config.ts ]; then
echo "---- vite.config.ts ----"
sed -n '1,200p' vite.config.ts
else
echo "No vite.config.js/ts found at repo root"
fi
echo
if [ -f package.json ]; then
echo "---- package.json (dependencies/devDependencies) ----"
sed -n '1,240p' package.json
else
echo "package.json not found"
fi
echoecho "=== Searching for SW registration in HTML/JS files ==="
rg -n --hidden --no-ignore -S "navigator.serviceWorker.register|serviceWorker.register|registerServiceWorker|/sw.js|/service-worker.js|/serviceworker.js|/workbox" || trueecho
echo "=== Searching for skipWaiting/clients.claim/caches.match usage ==="
rg -n --hidden --no-ignore -S "skipWaiting|clients.claim|caches.match|caches.open|event.waitUntil" || true</blockquote></details> <details> <summary>Build/.gitignore (1)</summary><blockquote> `1-42`: **LGTM** Covers node, build outputs, env, IDE, OS, and cache artifacts appropriately for Build/. </blockquote></details> <details> <summary>Build/.github/workflows/static.yml (1)</summary><blockquote> `1-46`: ```shell #!/bin/bash set -euo pipefail echo "PWD: $(pwd)" echo echo "Top-level entries:" ls -la | sed -n '1,200p' echo echo "Check workflow locations:" if [ -f Build/.github/workflows/static.yml ]; then echo "FOUND: Build/.github/workflows/static.yml" echo "---- snippet ----" sed -n '1,200p' Build/.github/workflows/static.yml else echo "MISSING: Build/.github/workflows/static.yml" fi echo if [ -f .github/workflows/static.yml ]; then echo "FOUND: .github/workflows/static.yml (repo root)" echo "---- snippet ----" sed -n '1,200p' .github/workflows/static.yml else echo "MISSING: .github/workflows/static.yml (repo root)" fi echo echo "Check build script:" if [ -f Build/Buildscripts/build.sh ]; then echo "FOUND: Build/Buildscripts/build.sh" ls -l Build/Buildscripts/build.sh if [ -x Build/Buildscripts/build.sh ]; then echo "Executable: yes"; else echo "Executable: no"; fi echo "---- head ----" sed -n '1,200p' Build/Buildscripts/build.sh else echo "MISSING: Build/Buildscripts/build.sh" fi echo echo "Search repo for 'Build/dist' and 'dist' references (limited):" if command -v rg >/dev/null 2>&1; then rg -n --hidden --no-ignore -S "Build/dist|/dist|\bdist\b" || true else grep -nR -E "Build/dist|/dist|\bdist\b" --exclude-dir=.git || true fi echo echo "Search for package.json / package-lock.json locations:" if command -v rg >/dev/null 2>&1; then rg -n --hidden --no-ignore -S "package-lock.json|package.json" || true else grep -nR -E "package-lock.json|package.json" --exclude-dir=.git || true fi echo echo "Search for upload-pages-artifact / deploy-pages usage:" if command -v rg >/dev/null 2>&1; then rg -n --hidden --no-ignore -S "upload-pages-artifact|deploy-pages" || true else grep -nR -E "upload-pages-artifact|deploy-pages" --exclude-dir=.git || true fiBuild/manifest.json (1)
2-4: Build/manifest.json — make start_url/scope relative and update icon pathsManifest currently uses absolute /HTMLRunner/ URLs (start_url and icon src) which will break on GitHub Pages subpaths. Repo contains ./pwa-192x192.png and ./pwa-512x512.png; update Build/manifest.json as below and ensure these icons are emitted to the final site root (or adjust paths to match Vite output).
{ - "name": "HTMLRunner", - "short_name": "HTMLRunner", - "start_url": "/HTMLRunner/", + "name": "HTMLRunner", + "short_name": "HTMLRunner", + "id": "./", + "start_url": "./", "display": "standalone", "background_color": "#f5f5f5", "theme_color": "#2196F3", + "scope": "./", "icons": [ { - "src": "/HTMLRunner/favicon.png", + "src": "pwa-192x192.png", "sizes": "192x192", "type": "image/png" }, { - "src": "/HTMLRunner/favicon.png", + "src": "pwa-512x512.png", "sizes": "512x512", "type": "image/png" } ] }Build/vite.config.mjs (1)
41-43: PWA source asset presentpublic/source.svg found at Build/public/source.svg — no action required.
workbox-3bd99cbd.js (1)
1-1: Vendor bundle: exclude minified Workbox from linters/formattersMinified vendor code (workbox-.js, sw.js, assets/**/.js) triggers "used before declaration" lint errors — don't edit vendor bundles; add linter/formatter ignore rules.
Sandbox attempt to create biome.json failed with "Permission denied" — add the ignore file or equivalent config into the repo.
Example biome.json:
{ "$schema": "https://biomejs.dev/schemas/1.7.0/schema.json", "files": { "ignore": [ "workbox-*.js", "sw.js", "assets/**/*.js" ] } }index.html (1)
10-10: Avoid double SW registration with vite-plugin-pwaindex.html includes a manual register script with id "vite-plugin-pwa:register-sw" (index.html:10). I only find VitePWA referenced in Build/vite.config.mjs and Build/package.json — confirm whether VitePWA is enabled in the repository's active vite.config; if it is, remove the manual registerSW.js <script> or disable the plugin's injected register to prevent double-registration.
sw.js (2)
1-1: Workbox setup looks solid.skipWaiting, clientsClaim, precacheAndRoute, cleanupOutdatedCaches, and NavigationRoute are correctly wired for an SPA.
1-1: SW precache verified — it matches the root index.html; Build/index.html is a separate build artifactPrecache includes assets/index-D-qNE1dZ.js, assets/index-D7CYcCYQ.css and index.html (see sw.js / sw.js.map); root index.html imports ./assets/index-D-qNE1dZ.js and ./assets/index-D7CYcCYQ.css and registers ./sw.js (registerSW.js). Build/index.html (Build/index.html:66) imports ./src/main.js — confirm your server serves the repo root index.html; if you instead serve Build/index.html, update the service worker precache/registration accordingly.
Build/index.html (1)
1-67: Verify: controls wired in src/main.js (file missing from repo)Build/index.html references ./src/main.js (Build/index.html:66) but src/main.js isn't present in the repository (an assets source map lists ../../src/main.js). I couldn't confirm wiring — ensure main.js registers event listeners for: toc-button, open-button, library-button, file-input, library-input, prev-button, next-button, close-toc, close-library, close-error, and overlay (overlay clicks).
Build/src/library.js (1)
88-91: Click handler remains valid after book.js fix.With openBookFromEntry accepting File or FileSystemHandle, this works across both directory and multi-file flows.
…storage and build_library
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
CodeRabbit Generated Unit Tests: Add Jest/Vitest tests for IndexedDB storage and build_library
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Build/.github/workflows/static.yml (1)
38-46: Add Configure Pages step to avoid deployment quirks.GitHub recommends configuring Pages before uploading artifacts. Add
actions/configure-pages@v5prior to upload.- name: Run build script run: | chmod +x Build/Buildscripts/build.sh ./Build/Buildscripts/build.sh + - name: Configure GitHub Pages + uses: actions/configure-pages@v5 + - name: Upload GitHub Pages artifact uses: actions/upload-pages-artifact@v3 with: path: 'Build/dist'Build/README.md (1)
1-211: README content doesn’t match this repository/appThis doc describes “HTMLRunner” (a code editor with CodeMirror/Prettier/Webpack), while the repo/PR provide “HTMLReader” (EPUB reader with Vite/PWA). This will mislead users.
Proposed fixes:
- Replace title and content to reflect HTMLReader features (EPUB viewer, library, TOC, offline/PWA).
- Update links from NellowTCS/HTMLRunner to HTMLToolkit/HTMLReader.
- If this README targets a different project, move it to that repo or to /docs/ with clear scope.
🧹 Nitpick comments (37)
Build/.gitignore (6)
19-22: Keep a tracked example env file; tighten local env ignoresAdd exceptions so
.env.example(and variants) remain committed while still ignoring actual secrets.# Environment variables .env .env.* +!.env.example +!.env.*.example
4-10: Include pnpm/lerna debug logsCovers occasional logs even if primary workflow is npm.
*.log npm-debug.log* yarn-debug.log* yarn-error.log* +pnpm-debug.log* +lerna-debug.log*
11-15: Optional: add Vite SSR/SSG output dirsHarmless if unused; prevents accidental commits if you experiment with SSR/SSG.
dist/ build/ out/ +dist-ssr/ +vite-ssg-dist/
24-25: Optionally allow VS Code recommendations to be trackedUseful to share team editor settings/extensions while still ignoring the rest of
.vscode/..vscode/ +!.vscode/extensions.json +!.vscode/settings.json
31-34: Minor OS cruft additions (macOS)Catches a few more Finder artifacts across subfolders.
.DS_Store Thumbs.db +._* +Icon?
38-42: Optional: ignore generic tool cachesSome tools/plugins create a
.cache/at project root..npm/ +.cache/ .eslintcacheregisterSW.js (1)
1-1: Harden SW registration and avoid brittle scope assumptionsDrop the explicit scope to let the browser infer from sw.js location, and add error handling/logging.
Apply:
-if('serviceWorker' in navigator) {window.addEventListener('load', () => {navigator.serviceWorker.register('./sw.js', { scope: './' })})} +if ('serviceWorker' in navigator) { + window.addEventListener('load', () => { + navigator.serviceWorker.register('./sw.js') + .then(reg => console.log('SW registered:', reg.scope)) + .catch(err => console.error('SW registration failed:', err)); + }); +}Please confirm that sw.js is emitted next to index.html in the final deploy path so the inferred scope matches your base URL.
Build/LICENSE (1)
1-22: Avoid duplicate license filesYou already keep a root LICENSE (export script preserves it). Keeping Build/LICENSE too can confuse tooling. Consider removing the Build/ copy and rely on the root file.
Build/export.mjs (1)
24-37: Be explicit about copying dotfiles and add minimal error handlingfs.copySync will skip permissions errors silently. Wrap in try/catch and ensure dotfiles in dist are copied as-is.
Apply:
-fs.readdirSync(dist).forEach(item => { +fs.readdirSync(dist, { withFileTypes: true }).forEach(entry => { + const item = entry.name; const srcPath = path.join(dist, item); const destPath = path.join(root, item); @@ - fs.copySync(srcPath, destPath, { overwrite: true }); - console.log('Copied:', item); + try { + fs.copySync(srcPath, destPath, { overwrite: true, errorOnExist: false }); + console.log('Copied:', item); + } catch (e) { + console.error('Copy failed:', item, e); + process.exitCode = 1; + } });Build/eslint.config.mjs (1)
1-15: Optional: Trim redundancy.You don’t need to repeat
pluginswhen using the plugins’ own recommended configs; they already include the proper language and rulesets.Build/.github/workflows/static.yml (1)
28-36: Cache Node dependencies to speed up builds.- name: Setup Node.js uses: actions/setup-node@v4 with: node-version: '20' + cache: 'npm' + cache-dependency-path: 'Build/package-lock.json'Build/vite.config.mjs (2)
26-38: Tighten caching strategies for better UX offline/online.Use NetworkFirst for documents, StaleWhileRevalidate for JS/CSS, CacheFirst for images; JSON often benefits from SWR to avoid stale data.
workbox: { - runtimeCaching: [ - { - urlPattern: /.*\.(js|css|html)$/, - handler: 'NetworkFirst', - options: { cacheName: 'app-shell' }, - }, - { - urlPattern: /.*\.(png|ico|json)$/, - handler: 'CacheFirst', - options: { cacheName: 'assets' }, - }, - ], + runtimeCaching: [ + { urlPattern: ({request}) => request.destination === 'document', + handler: 'NetworkFirst', options: { cacheName: 'app-shell' } }, + { urlPattern: ({request}) => ['script','style','worker'].includes(request.destination), + handler: 'StaleWhileRevalidate', options: { cacheName: 'assets' } }, + { urlPattern: /.*\.(png|ico|svg|jpg|jpeg|webp|gif)$/i, + handler: 'CacheFirst', options: { cacheName: 'images' } }, + { urlPattern: /.*\.json$/i, + handler: 'StaleWhileRevalidate', options: { cacheName: 'api' } }, + ], },
41-42: AvoidallowedHosts: trueunless you need it.It disables host checks in dev and can enable DNS-rebinding during local testing. Prefer a specific allowlist or omit it.
Build/.github/workflows/codeql.yml (1)
58-63: Exclude built artifacts to reduce noise and runtime.Consider ignoring
Build/dist/**and vendored bundles in CodeQL. Add a config file and reference it here.- name: Initialize CodeQL uses: github/codeql-action/init@v3 with: languages: ${{ matrix.language }} build-mode: ${{ matrix.build-mode }} + config-file: .github/codeql/codeql-config.ymlExample
.github/codeql/codeql-config.yml:paths-ignore: - Build/dist/** - **/workbox-*.jsBuild/styles.css (3)
1-8: Mobile vh fix.Use
100dvhto avoid Safari URL bar issues; keepheight: 100vhas fallback.body { - font-family: Arial, sans-serif; - background-color: #f5f5f5; - height: 100vh; + font-family: Arial, sans-serif; + background-color: #f5f5f5; + min-height: 100dvh; + height: 100vh; overflow: hidden; margin: 0; padding: 0; }
246-249: Respect reduced motion.Disable spinner/animations when users prefer reduced motion.
@keyframes spin { 0% { transform: rotate(0deg); } 100% { transform: rotate(360deg); } } + +@media (prefers-reduced-motion: reduce) { + * { animation: none !important; transition: none !important; } +}
118-121: Avoid style-attribute selectors for display toggling.
.editor-container:not([style*="display: none"])is brittle. Prefer a class toggle (.is-active) from JS.-.editor-container:not([style*="display: none"]) { - display: block; -} +.editor-container.is-active { + display: block; +}sw.js (3)
1-1: Consider update flow: skipWaiting + clientsClaim can break in-flight sessionsUnconditional self.skipWaiting() + clientsClaim() may swap SW while a page still uses old assets, causing 404s/mixed versions. Prefer an “update available” prompt, then call skipWaiting via postMessage.
I can provide the message-channel glue for a user prompt.
1-1: JS/CSS strategy: prefer StaleWhileRevalidate for faster loadsFor hashed assets, SWR generally yields better UX than NetworkFirst.
- new e.NetworkFirst({ cacheName:"app-shell", plugins:[...] }) + new e.StaleWhileRevalidate({ cacheName:"app-shell", plugins:[...] })
2-2: Source map reference without map fileIf sw.js.map isn’t published, drop the comment to avoid 404 noise in devtools.
-//# sourceMappingURL=sw.js.map +/* source map intentionally omitted in deploy */assets/index-D7CYcCYQ.css (3)
1-1: Respect prefers-reduced-motionDisable transitions/animations for users who prefer reduced motion.
+@media (prefers-reduced-motion: reduce) { + * { animation: none !important; transition: none !important; } +}
1-1: Viewport height on mobileheight:100vh can jump under mobile URL bars. Prefer 100dvh.
- body{...height:100vh;...} + body{...height:100vh;...} /* existing */ + @supports (height: 100dvh) { + body{height:100dvh;} +}
1-1: Small usability nit: disabled button contrastDisabled text on #718096 may be low on some displays. Consider increasing contrast.
Build/README.md (1)
156-162: Markdownlint: missing fenced code languagesAdd language hints for code blocks to satisfy MD040 and improve highlighting.
-``` +```bash ./build.sh-``` +```bash ./build-all.shindex.html (1)
10-10: Defer SW registration script to end of bodyNot critical, but moving registerSW.js to just before can avoid blocking in some browsers; Vite PWA supports late registration.
Build/src/indexedDB.js (3)
46-54: Optional: mirror read flow to close the transaction cleanlyReadonly gets can still use tx.oncomplete; minor robustness/readability win.
export async function getStoredLibraryHandle() { const db = await getDB(); return new Promise((resolve, reject) => { - const tx = db.transaction("handles", "readonly"); - const store = tx.objectStore("handles"); - const req = store.get("library"); - req.onsuccess = () => resolve(req.result ? req.result.handle : null); - req.onerror = e => reject(e.target.error); + const tx = db.transaction("handles", "readonly"); + const store = tx.objectStore("handles"); + const req = store.get("library"); + req.onerror = e => reject(e.target.error); + tx.oncomplete = () => resolve(req.result ? req.result.handle : null); }); }
10-20: Guard createObjectStore to avoid upgrade errors on re-runsIf you ever bump the version and re-run upgrades, defensively check existence.
- db.createObjectStore("handles", { keyPath: "name" }); + if (!db.objectStoreNames.contains("handles")) { + db.createObjectStore("handles", { keyPath: "name" }); + }
22-26: Note on File System Access handles portabilityStoring FileSystem*Handle works in Chromium via structured clone; Safari/Firefox lack support. Consider a fallback (e.g., persist file names/IDs) and feature-detect before calling storeLibraryHandle.
I can add a small wrapper that no-ops or stores metadata when window.showDirectoryPicker is unavailable.
Build/src/main.js (3)
30-31: Close button should always close (not toggle) the TOCUsing toggle on the Close button can reopen the TOC in edge cases. Call closeToc() directly.
- closeTocButton.addEventListener('click', toggleToc); + closeTocButton.addEventListener('click', closeToc);
1-2: Break ES module cycle between main ⇄ bookbook.js imports showLoading/showError/hideLoading from main.js, while main.js imports functions from book.js. This works today but is fragile. Consider moving the UI helpers to a tiny ui.js to remove the cycle.
Example:
- import { openBook, prevPage, nextPage, goToPage, toggleToc, closeToc } from "./book"; + import { openBook, prevPage, nextPage, goToPage, toggleToc, closeToc } from "./book"; + import { showLoading, hideLoading, showError, hideError } from "./ui"; - export function showLoading() { + // ui.js + export function showLoading() { loadingMessage.classList.add('show'); }Also applies to: 47-76
24-33: Optional: guard missing DOM nodesIf markup drifts, addEventListener on null will throw. Consider null-checks or early assert in dev.
Build/index.html (1)
65-67: Script tag outside bodyPlace the module script inside or for valid HTML.
-</body> -<script type="module" src="./src/main.js"></script> -</html> + <script type="module" src="./src/main.js"></script> +</body> +</html>Build/src/style.css (1)
227-237: Modal z-index can be obscured by TOC.toc-container is 1010; .message is 25, so TOC can cover error/loading modals. Raise .message above panels.
.message { ... - z-index: 25; + z-index: 2000; }Build/src/book.js (2)
57-58: Allow re-selecting the same fileReset the input value after starting the read so selecting the same EPUB again fires change.
reader.readAsArrayBuffer(file); + // Allow selecting the same file again next time + if (e?.target) e.target.value = '';
235-239: Ignore arrow-key nav when typing in inputsPrevent page turns while the page-number input is focused.
function handleKeyEvents(e) { if (!book || !rendition) return; + const tag = document.activeElement && document.activeElement.tagName; + if (tag === 'INPUT' || tag === 'TEXTAREA') return; if (e.key === 'ArrowLeft') prevPage(); if (e.key === 'ArrowRight') nextPage(); }Build/src/library.js (2)
61-73: Parallelize library item renderingcreateLibraryItem is awaited serially; for large libraries this is slow. Build items in parallel and append via a fragment.
async function displayLibraryGrid(fileEntries) { libraryContent.innerHTML = ''; if (fileEntries.length === 0) { const msg = document.createElement('div'); msg.textContent = 'No EPUB files found.'; libraryContent.appendChild(msg); return; } - for (const entry of fileEntries) { - const item = await createLibraryItem(entry); - libraryContent.appendChild(item); - } + const items = await Promise.all(fileEntries.map(createLibraryItem)); + const frag = document.createDocumentFragment(); + for (const item of items) frag.appendChild(item); + libraryContent.appendChild(frag); }
20-40: Persisted directory handle may need permission refreshWhen reusing a stored handle, request permission if needed to avoid NotAllowedError.
export async function openLibrary() { try { // Try to retrieve stored library directory handle let dirHandle = await getStoredLibraryHandle(); if (!dirHandle) { // If no stored handle, prompt user dirHandle = await window.showDirectoryPicker(); await storeLibraryHandle(dirHandle); } + // Ensure read permission on the handle + if (dirHandle.requestPermission && dirHandle.queryPermission) { + const status = await dirHandle.queryPermission({ mode: 'read' }); + if (status !== 'granted') { + const granted = await dirHandle.requestPermission({ mode: 'read' }); + if (granted !== 'granted') throw new Error('Permission denied for selected directory.'); + } + } const files = []; for await (const entry of dirHandle.values()) { if (entry.kind === 'file' && entry.name.endsWith('.epub')) { files.push(entry); } } displayLibraryGrid(files); toggleLibrary(true); } catch (err) { showError('Failed to open library: ' + err.message); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
Build/favicon.pngis excluded by!**/*.pngBuild/package-lock.jsonis excluded by!**/package-lock.jsonBuild/public/source.svgis excluded by!**/*.svgapple-touch-icon-180x180.pngis excluded by!**/*.pngassets/index-D-qNE1dZ.js.mapis excluded by!**/*.mapfavicon.icois excluded by!**/*.icofavicon.pngis excluded by!**/*.pngmaskable-icon-512x512.pngis excluded by!**/*.pngpwa-192x192.pngis excluded by!**/*.pngpwa-512x512.pngis excluded by!**/*.pngpwa-64x64.pngis excluded by!**/*.pngsource.svgis excluded by!**/*.svgsw.js.mapis excluded by!**/*.mapworkbox-3bd99cbd.js.mapis excluded by!**/*.map
📒 Files selected for processing (26)
Build/.github/dependabot.yml(1 hunks)Build/.github/workflows/codeql.yml(1 hunks)Build/.github/workflows/static.yml(1 hunks)Build/.gitignore(1 hunks)Build/LICENSE(1 hunks)Build/README.md(1 hunks)Build/eslint.config.mjs(1 hunks)Build/export.mjs(1 hunks)Build/index.html(1 hunks)Build/manifest.json(1 hunks)Build/package.json(1 hunks)Build/service-worker.js(1 hunks)Build/src/book.js(1 hunks)Build/src/indexedDB.js(1 hunks)Build/src/library.js(1 hunks)Build/src/main.js(1 hunks)Build/src/style.css(1 hunks)Build/styles.css(1 hunks)Build/vite.config.mjs(1 hunks)assets/index-D7CYcCYQ.css(1 hunks)index.html(2 hunks)manifest.webmanifest(1 hunks)registerSW.js(1 hunks)service-worker.js(0 hunks)sw.js(1 hunks)workbox-3bd99cbd.js(1 hunks)
💤 Files with no reviewable changes (1)
- service-worker.js
🧰 Additional context used
🧬 Code graph analysis (4)
Build/src/main.js (2)
Build/src/book.js (11)
tocButton(15-15)prevButton(16-16)nextButton(17-17)currentPageInput(18-18)overlay(19-19)openBook(35-58)prevPage(198-200)nextPage(207-209)goToPage(222-229)toggleToc(246-249)closeToc(256-259)Build/src/library.js (4)
overlay(9-9)openLibrary(20-40)toggleLibrary(135-146)handleLibraryFiles(45-49)
Build/src/book.js (2)
Build/src/main.js (4)
overlay(15-15)showError(65-68)showLoading(47-49)hideLoading(55-57)Build/src/library.js (4)
overlay(9-9)file(101-103)toggleLibrary(135-146)arrayBuffer(104-104)
Build/src/library.js (3)
Build/src/book.js (4)
file(36-36)file(76-76)arrayBuffer(77-77)openBookFromEntry(71-86)Build/src/main.js (1)
showError(65-68)Build/src/indexedDB.js (2)
getStoredLibraryHandle(46-55)storeLibraryHandle(27-36)
workbox-3bd99cbd.js (1)
sw.js (5)
e(1-1)n(1-1)r(1-1)o(1-1)l(1-1)
🪛 markdownlint-cli2 (0.17.2)
Build/README.md
156-156: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
160-160: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Biome (2.1.2)
Build/src/style.css
[error] 115-115: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
z-index is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
workbox-3bd99cbd.js
[error] 31-31: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 31-31: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (10)
Build/service-worker.js (1)
9-15: Add navigation fallback + activate handler; ensure only one registered service worker
- Add a navigation fallback (serve index.html for navigation requests) and an activate handler that deletes old caches to Build/service-worker.js to prevent offline 404s and cache bloat.
- Repo also contains sw.js at the repo root — confirm which SW is actually built/registered (sw.js vs Build/service-worker.js) and remove or unify the duplicate to avoid runtime conflicts.
Build/package.json (1)
1-12: Mark package private, add Node engine, move Vite to devDependenciesApply:
{ "name": "htmlreader", "version": "2.1.0", "description": "A quick HTML-based ePub Reader that works offline.", "main": "index.js", + "private": true, + "engines": { "node": ">=18.0.0" }, "scripts": { @@ "dependencies": { - "epubjs": "^0.3.93", - "jszip": "^3.10.1", - "vite": "^7.1.3" + "epubjs": "^0.3.93", + "jszip": "^3.10.1" }, "devDependencies": { @@ - "rimraf": "^6.0.1", - "vite-plugin-pwa": "^1.0.3" + "rimraf": "^6.0.1", + "vite-plugin-pwa": "^1.0.3", + "vite": "^7.1.3" } }File: Build/package.json (lines 1–12, 28–45). If CI uses npm ci --omit=dev, ensure export/build steps run only in workflows that install devDependencies.
Build/manifest.json (1)
2-4: ```shell
#!/bin/bash
set -euo pipefail
echo "=== repo root: $(pwd) ==="
echo
echo "=== Searching for manifest files and references ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore -S 'manifest(.webmanifest|.json)?' || true
else
grep -RIn --exclude-dir=node_modules --exclude-dir=dist --exclude-dir=build -e 'manifest.webmanifest' -e 'manifest.json' -e 'rel="manifest"' . || true
fi
echo
for f in Build/manifest.json manifest.webmanifest public/manifest.webmanifest src/manifest.webmanifest app/manifest.webmanifest; do
if [ -f "$f" ]; then
echo "=== SHOW: $f ==="
sed -n '1,240p' "$f"
echo
fi
done
echo "=== Searching for HTMLRunner / HTMLReader occurrences ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore -S 'HTMLRunner|HTMLReader' || true
else
grep -RIn --exclude-dir=node_modules --exclude-dir=dist --exclude-dir=build -e 'HTMLRunner' -e 'HTMLReader' . || true
fi
echo
echo "=== Find manifest link in HTML files (rel="manifest") ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore -S '<link[^>]+rel=["'"'"']manifest["'"'"']|manifest.webmanifest' || true
else
grep -RIn --exclude-dir=node_modules --exclude-dir=dist --exclude-dir=build -e 'rel="manifest"' -e "rel='manifest'" -e 'manifest.webmanifest' . || true
fi
for f in index.html public/index.html src/index.html; do
if [ -f "$f" ]; then
echo "=== SHOW: $f ==="
sed -n '1,240p' "$f"
echo
fi
done</blockquote></details> <details> <summary>Build/vite.config.mjs (1)</summary><blockquote> `20-24`: **Verified: Build/public/source.svg exists.** pwaAssets.image path is valid; no change required. </blockquote></details> <details> <summary>workbox-3bd99cbd.js (1)</summary><blockquote> `1-69`: **Verify there’s no double Workbox registration with VitePWA.** rg in Build/** returned no matches — cannot confirm. Ensure the service worker does not import both this bundle (workbox-3bd99cbd.js) and the VitePWA-generated Workbox runtime. Run these checks and paste the output: ```bash rg -n --no-config -S "(precacheAndRoute|registerRoute|workbox-3bd99cbd|workbox-)" -g "Build/**" rg -n --no-config -S "(precacheAndRoute|registerRoute|workbox-3bd99cbd|workbox-)" rg -n --no-config -S "VitePWA|vite-plugin-pwa|vitepwa"sw.js (1)
1-1: Confirmed: workbox-3bd99cbd.js present next to sw.js in deploy root.
ls output shows both sw.js and workbox-3bd99cbd.js at the site root; no action required.index.html (1)
12-14: Icons & manifest present — no action required.
manifest.webmanifest, favicon.ico, source.svg and apple-touch-icon-180x180.png exist at the repository root; index.html link paths are correct.Build/src/main.js (1)
35-37: Overlay click handler: robust no-op orderingLooks good: close TOC, close Library, then hide Error. Order avoids leaving overlay open. LGTM.
Build/index.html (1)
23-23: PWA wiring check (manifest/SW)install-button exists but no manifest link or SW registration here. If offline is a goal in this PR, ensure index.html links the web manifest and registers the service worker (or confirm Vite injects both at build).
Build/src/library.js (1)
123-125: Good UX: re-click remains enabledNot using { once: true } lets users retry on transient failures. Nice touch.
NellowTCS
changed the title
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
…library, main, indexedDB, style
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
CodeRabbit Generated Unit Tests: Add Jest/JSDOM unit tests for book, library, main, indexedDB, style
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (13)
manifest.webmanifest (1)
1-1: Do not edit — ensure values come from VitePWA config and emit a single manifestThis file is generated; align name/short_name/start_url/scope/id via Build/vite.config.mjs and avoid committing multiple manifests. If another manifest exists or different identity (“HTMLRunner”), remove/disable it in the generator to prevent install/update confusion.
Run to confirm there’s only one served manifest and no conflicting identities:
#!/bin/bash rg -n -S --glob '!node_modules/**' '(manifest\.webmanifest|manifest\.json|"name":|"short_name":|"start_url":|"scope":|"id":)'registerSW.js (1)
1-1: Generated SW registration — improve in source or use VitePWA auto-registerSince this is minified/generated, make changes in the source or switch to VitePWA’s injectRegister/virtual:pwa-register for update UX and error handling. Also ensure you don’t double-register if injectRegister is enabled.
#!/bin/bash rg -n --glob '!node_modules/**' 'virtual:pwa-register|injectRegister|serviceWorker\.register\(' BuildBuild/export.mjs (1)
3-3: rimraf API misuse — rimraf.sync will throw with rimraf@6Use the named sync export.
Apply:
-import { rimraf } from 'rimraf'; +import { rimrafSync } from 'rimraf';Build/src/style.css (1)
112-116: Fix TOC z-index (under overlay) and remove duplicate property..toc-container has two z-index declarations (10 and 1010). This causes lint noise and, more importantly, your built CSS currently ships z-index:10, which places the TOC under .overlay (z-index:15). Set a single z-index aligned with .library-container (20) so both sit above the overlay and below .message (25).
Apply:
.toc-container { position: fixed; top: 0; left: 0; width: 300px; height: 100%; background-color: white; box-shadow: 2px 0 5px rgba(0, 0, 0, 0.1); transform: translateX(-100%); transition: transform 0.3s ease; - z-index: 10; display: flex; flex-direction: column; - z-index: 1010; + /* Above .overlay (15), below .message (25), aligned with .library-container */ + z-index: 20; }Quick check script to ensure both source and built CSS reflect the change:
#!/bin/bash rg -n -C1 'toc-container.*z-index' Build/src/style.css assets/*.cssAlso applies to: 212-220
Build/src/indexedDB.js (1)
32-35: Resolve on transaction completion, not request success.req.onsuccess can fire before the write commits. Resolve on tx.oncomplete and reject on tx.onerror/tx.onabort.
- const req = store.put({ name: "library", handle }); - req.onsuccess = () => resolve(); - req.onerror = e => reject(e.target.error); + store.put({ name: "library", handle }); + tx.oncomplete = () => resolve(); + tx.onabort = tx.onerror = e => reject(tx.error || (e && e.target && e.target.error));assets/index-D7CYcCYQ.css (1)
1-1: Built CSS still has .toc-container z-index:10 (regression).Don’t edit minified assets; propagate the source fix and rebuild. Ensure the emitted CSS sets .toc-container z-index to 20 to sit above .overlay (15).
Verification:
#!/bin/bash rg -nP '\.toc-container[^{]*\{[^}]*z-index:\s*10' assets/*.css -C0sw.js (1)
1-1: Add cache expiration and quota safety to runtime cachesBoth runtime caches are unbounded; add ExpirationPlugin with sensible caps and purgeOnQuotaError to avoid uncontrolled growth.
- e.registerRoute(/.*\.(js|css|html)$/,new e.NetworkFirst({cacheName:"app-shell",plugins:[]}),"GET"), + e.registerRoute(/.*\.(js|css|html)$/,new e.NetworkFirst({ + cacheName:"app-shell", + plugins:[new e.ExpirationPlugin({maxEntries:50,maxAgeSeconds:7*24*60*60,purgeOnQuotaError:true})] + }),"GET"), - e.registerRoute(/.*\.(png|ico|json)$/,new e.CacheFirst({cacheName:"assets",plugins:[]}),"GET") + e.registerRoute(/.*\.(png|ico|json)$/,new e.CacheFirst({ + cacheName:"assets", + plugins:[new e.ExpirationPlugin({maxEntries:150,maxAgeSeconds:30*24*60*60,purgeOnQuotaError:true})] + }),"GET")Build/src/main.js (1)
47-76: Break circular dependency of UI helpers (move to ui.js)book.js/library.js importing helpers from main.js while main imports them creates a cycle; extract helpers to Build/src/ui.js and import from there.
- export function showLoading() { + // Build/src/ui.js + export function showLoading() { loadingMessage.classList.add('show'); } - export function hideLoading() { + export function hideLoading() { loadingMessage.classList.remove('show'); } - export function showError(message) { + export function showError(message) { errorText.textContent = message; errorMessage.classList.add('show'); } - export function hideError() { + export function hideError() { errorMessage.classList.remove('show'); }Then in this file:
-// (keep function bodies or import them) -export { showLoading, hideLoading, showError, hideError }; +import { showLoading, hideLoading, showError, hideError } from './ui'; +export { showLoading, hideLoading, showError, hideError };And update Build/src/book.js and Build/src/library.js to import from './ui' instead of './main'.
Build/index.html (1)
4-4: CSP is overly permissive and may block EPUB iframes; tighten and allow blob/data framesRemove 'unsafe-eval', prune unused CDNs, and allow blob:/data: for iframes used by epub.js.
- <meta http-equiv="Content-Security-Policy" content="default-src 'self' 'unsafe-inline' 'unsafe-eval' data: blob: https://cdnjs.cloudflare.com https://cdn.jsdelivr.net https://code.jquery.com https://unpkg.com https://d3js.org https://threejs.org https://cdn.plot.ly https://stackpath.bootstrapcdn.com https://maps.googleapis.com https://cdn.tailwindcss.com https://ajax.googleapis.com https://kit.fontawesome.com https://cdn.datatables.net https://maxcdn.bootstrapcdn.com https://code.highcharts.com; frame-src 'self'; child-src 'self'; manifest-src 'self'; worker-src 'self'; upgrade-insecure-requests; block-all-mixed-content;"> + <meta http-equiv="Content-Security-Policy" content=" + default-src 'self' blob: data:; + script-src 'self'; + style-src 'self' 'unsafe-inline'; + img-src 'self' blob: data:; + connect-src 'self'; + frame-src 'self' blob: data:; + worker-src 'self'; + manifest-src 'self'; + upgrade-insecure-requests; block-all-mixed-content;">Build/src/library.js (1)
24-34: Handle missing Directory Picker and re-check permissions before listing.Without feature detection and permission re-check, Safari/iOS and revoked permissions will fail at runtime. Add a graceful fallback and request permission for persisted handles.
- if (!dirHandle) { - // If no stored handle, prompt user - dirHandle = await window.showDirectoryPicker(); - await storeLibraryHandle(dirHandle); - } + if (!dirHandle) { + // If no stored handle, prompt user (with fallback) + if (!('showDirectoryPicker' in window)) { + document.getElementById('library-input')?.click(); + return; + } + dirHandle = await window.showDirectoryPicker(); + await storeLibraryHandle(dirHandle); + } + // Re-check permissions for persisted handles + if (dirHandle.queryPermission && dirHandle.requestPermission) { + const perm = await dirHandle.queryPermission({ mode: 'read' }); + if (perm !== 'granted') { + const res = await dirHandle.requestPermission({ mode: 'read' }); + if (res !== 'granted') { + document.getElementById('library-input')?.click(); + return; + } + } + } - const files = []; - for await (const entry of dirHandle.values()) { - if (entry.kind === 'file' && entry.name.endsWith('.epub')) { + const files = []; + for await (const entry of (dirHandle.values?.() ?? dirHandle.entries?.() ?? [])) { + const name = entry.name?.toLowerCase?.() ?? ''; + if (entry.kind === 'file' && name.endsWith('.epub')) { files.push(entry); } }Build/src/book.js (3)
127-127: Prevent duplicate keyup handler registration.Remove before add to ensure a single listener.
- window.addEventListener('keyup', handleKeyEvents); + window.removeEventListener('keyup', handleKeyEvents); + window.addEventListener('keyup', handleKeyEvents);
76-78: Support both FileSystemHandle and File when opening from library.Clicking items from the file-input fallback passes a File (no getFile), causing a crash. Use a conditional.
- const file = await entry.getFile(); + const file = (entry && typeof entry.getFile === 'function') ? await entry.getFile() : entry;
101-107: Clean up previous book/rendition and listeners before loading a new one.Prevents memory leaks and duplicate key handlers.
- if (book) { - book = null; - rendition = null; - viewer.innerHTML = ''; - } + if (book || rendition) { + try { window.removeEventListener('keyup', handleKeyEvents); } catch {} + try { await rendition?.destroy?.(); } catch {} + try { await book?.destroy?.(); } catch {} + viewer.innerHTML = ''; + book = null; + rendition = null; + }
🧹 Nitpick comments (30)
Build/vite.config.mjs (3)
12-19: Add manifest id/scope and keep identity centralizedSet id/scope so the generated manifest matches your deploy base (“./”) and avoid editing the generated webmanifest.
Apply:
manifest: { - name: 'HTMLReader', - short_name: 'HTMLReader', - start_url: './', - display: 'standalone', - background_color: '#f5f5f5', - theme_color: '#2196F3', + id: './', + name: 'HTMLReader', + short_name: 'HTMLReader', + start_url: './', + scope: './', + display: 'standalone', + background_color: '#f5f5f5', + theme_color: '#2196F3', },
25-37: Tighten Workbox defaults for SPA + cache hygieneConsider adding cleanupOutdatedCaches and a navigation fallback.
Apply:
workbox: { - runtimeCaching: [ + cleanupOutdatedCaches: true, + navigateFallback: 'index.html', + runtimeCaching: [ { urlPattern: /.*\.(js|css|html)$/, handler: 'NetworkFirst', options: { cacheName: 'app-shell' }, }, { - urlPattern: /.*\.(png|ico|json)$/, + urlPattern: /.*\.(png|ico|json|webmanifest)$/, handler: 'CacheFirst', options: { cacheName: 'assets' }, }, ], },
9-19: Avoid double service-worker registration — pick one methodBuild/vite.config.mjs (VitePWA) currently has no injectRegister and ./registerSW.js manually registers ./sw.js; choose one:
- Keep manual registration (keep ./registerSW.js) — set injectRegister to 'none':
- VitePWA({ + VitePWA({ + injectRegister: 'none',
- Let the plugin handle registration (remove/stop generating ./registerSW.js) — set injectRegister to 'auto':
- VitePWA({ + VitePWA({ + injectRegister: 'auto',registerSW.js appears autogenerated — do not edit it directly; change the generator or VitePWA injectRegister instead.
Build/package.json (2)
6-12: Optional: avoid a failing default test scriptEither wire up an actual runner or make the script non-failing to keep CI green.
Example:
- "test": "echo \"Error: no test specified\" && exit 1", + "test": "echo \"No tests in npm script; see separate PRs\"",
1-45: Optional hardening: mark package private and pin Node enginePrevents accidental publish and ensures Node compatibility (ESM, rimraf@6).
Apply:
{ "name": "htmlreader", "version": "2.1.0", + "private": true, + "engines": { "node": ">=18.0.0" },tests/unit/style.test.js (2)
16-21: Search misses Build/ paths; will always fall back to inline fixtureInclude Build/ directories so the test validates the real CSS when present.
Apply:
- const roots = ['.', 'src', 'public', 'assets', 'styles', 'style', 'css']; + const roots = ['.', 'Build', 'Build/src', 'Build/assets', 'src', 'public', 'assets', 'styles', 'style', 'css'];
330-347: Naive CSS parsing is fine for fixture; guard against catastrophic backtrackingRestrict regex to top-level and trim early to speed up large stylesheets.
Apply:
- const withoutComments = cssText.replace(/\/\*[\s\S]*?\*\//g, ''); + const withoutComments = cssText.replace(/\/\*[\s\S]*?\*\//g, '').replace(/\s+/g, ' ');Build/src/style.css (1)
42-59: Add visible focus styles for keyboard accessibility.Provide outlines for interactive elements to meet a11y expectations without relying on mouse hover.
button { background-color: #2196F3; color: white; border: none; padding: 0.5rem 1rem; border-radius: 4px; cursor: pointer; font-size: 0.9rem; } + +/* Visible focus ring for keyboard users */ +button:focus-visible, +.toc-item:focus-visible, +#close-toc:focus-visible, +#close-library:focus-visible, +#prev-button:focus-visible, +#next-button:focus-visible { + outline: 3px solid #0ea5e9; + outline-offset: 2px; +}Also applies to: 137-145
Build/src/indexedDB.js (1)
13-16: Guard object store creation for future upgrades.Be defensive to avoid InvalidStateError on multi-version upgrades.
- request.onupgradeneeded = e => { - const db = e.target.result; - db.createObjectStore("handles", { keyPath: "name" }); - }; + request.onupgradeneeded = e => { + const db = e.target.result; + if (!db.objectStoreNames.contains("handles")) { + db.createObjectStore("handles", { keyPath: "name" }); + } + };index.html (1)
4-4: Tighten CSP now that assets are bundled.Drop 'unsafe-inline'/'unsafe-eval' and broad CDN allowlists; keep only origins you use. Prefer HTTP header, but meta is OK for now.
- <meta http-equiv="Content-Security-Policy" content="default-src 'self' 'unsafe-inline' 'unsafe-eval' data: blob: https://cdnjs.cloudflare.com https://cdn.jsdelivr.net https://code.jquery.com https://unpkg.com https://d3js.org https://threejs.org https://cdn.plot.ly https://stackpath.bootstrapcdn.com https://maps.googleapis.com https://cdn.tailwindcss.com https://ajax.googleapis.com https://kit.fontawesome.com https://cdn.datatables.net https://maxcdn.bootstrapcdn.com https://code.highcharts.com; frame-src 'self'; child-src 'self'; manifest-src 'self'; worker-src 'self'; upgrade-insecure-requests; block-all-mixed-content;"> + <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'; style-src 'self'; img-src 'self' data: blob:; font-src 'self'; connect-src 'self'; worker-src 'self'; manifest-src 'self'; base-uri 'self'; frame-ancestors 'none'; upgrade-insecure-requests;">If any inline scripts/styles remain, replace with external files or add nonces/hashes accordingly.
workbox-3bd99cbd.js (1)
1-71: Don’t vendor Workbox; let VitePWA generate it.Checked-in Workbox bundles drift from plugin versions, trigger lints, and complicate updates. Remove from source and rely on vite-plugin-pwa output, or move under a vendored/ path and exclude from lint/CodeQL. If you must keep it, pin version and add provenance; also avoid sourcemap 404s.
Minimal mitigation to prevent sourcemap 404:
-//# sourceMappingURL=workbox-3bd99cbd.js.map +/* source map intentionally omitted to avoid 404s in production */sw.js (1)
1-1: Enable navigation preload for faster first paint on slow networksOptional, but improves latency while SW boots.
define(["./workbox-3bd99cbd"],function(e){"use strict"; self.skipWaiting(),e.clientsClaim(), +self.addEventListener("activate", (event)=>{event.waitUntil(self.registration.navigationPreload.enable())}), e.precacheAndRoute(Build/src/main.js (1)
24-41: Guard DOM bindings to avoid crashes in tests/non-DOM contextsUse optional chaining so unit tests (jsdom/node) don’t throw when elements are absent.
-openButton.addEventListener('click', () => fileInput.click()); -fileInput.addEventListener('change', openBook); -prevButton.addEventListener('click', prevPage); -nextButton.addEventListener('click', nextPage); -currentPageInput.addEventListener('change', goToPage); -tocButton.addEventListener('click', toggleToc); -closeTocButton.addEventListener('click', toggleToc); -libraryButton.addEventListener('click', openLibrary); -closeLibraryButton.addEventListener('click', () => toggleLibrary(false)); -overlay.addEventListener('click', () => { +openButton?.addEventListener('click', () => fileInput?.click()); +fileInput?.addEventListener('change', openBook); +prevButton?.addEventListener('click', prevPage); +nextButton?.addEventListener('click', nextPage); +currentPageInput?.addEventListener('change', goToPage); +tocButton?.addEventListener('click', toggleToc); +closeTocButton?.addEventListener('click', toggleToc); +libraryButton?.addEventListener('click', openLibrary); +closeLibraryButton?.addEventListener('click', () => toggleLibrary(false)); +overlay?.addEventListener('click', () => { closeToc(); toggleLibrary(false); hideError(); }); -closeErrorButton.addEventListener('click', hideError); +closeErrorButton?.addEventListener('click', hideError); -libraryInput.addEventListener('change', handleLibraryFiles); +libraryInput?.addEventListener('change', handleLibraryFiles);Build/index.html (1)
65-67: Place the module script inside (valid HTML)The script tag is outside the body. Move it before .
-</body> -<script type="module" src="./src/main.js"></script> -</html> + <script type="module" src="./src/main.js"></script> +</body> +</html>tests/helpers/indexeddb-mock.spec.js (1)
135-161: Consider deduping IndexedDB fakes across test suitesYou now have two separate fakes (this file and tests/build_indexedDB.test.js). Extract a shared helper to reduce drift.
tests/unit/book.test.js (3)
115-117: Avoid brittle exact-string assertion for error message.Match by intent to prevent failures if copy changes.
- expect(mainMocks.showError).toHaveBeenCalledWith('The selected file is not a valid EPUB file.'); + expect(mainMocks.showError).toHaveBeenCalledWith(expect.stringMatching(/valid\s+epub/i));
89-106: Restore global FileReader to avoid bleed-through across suites.Capture and restore the native FileReader (if present).
- beforeEach(async () => { + const RealFileReader = global.FileReader; + beforeEach(async () => { jest.resetModules(); setupDomSkeleton(); global.FileReader = MockFileReader; @@ - afterEach(() => { + afterEach(() => { jest.clearAllMocks(); + global.FileReader = RealFileReader; });
136-139: Unify async flush into a helper to reduce flakiness.Replace repeated microtask/macrotask flushes with a single helper.
+ const flush = async () => { await Promise.resolve(); await new Promise((r) => setTimeout(r, 0)); }; @@ - await Promise.resolve(); - await new Promise(setImmediate); + await flush(); @@ - await Promise.resolve(); - await new Promise(setImmediate); + await flush(); @@ - await Promise.resolve(); - await new Promise(setImmediate); + await flush(); @@ - await Promise.resolve(); - await new Promise(setImmediate); + await flush();Also applies to: 228-230, 351-353, 367-369
tests/unit/main.test.js (1)
75-77: Normalize import fallbacks across tests.Several blocks still hardcode '../../src/main.js' with limited fallbacks. Use the helper proposed above to reduce brittleness.
Also applies to: 92-97, 109-114, 129-134, 143-148, 160-175, 177-188, 193-202, 204-216, 218-229, 231-244, 246-257, 260-272
tests/unit/indexedDB.test.js (3)
197-201: Remove dead variables.These locals are unused and can be dropped.
- const origOnSuccessSetter = Object.getOwnPropertyDescriptor(Object.getPrototypeOf(req), "onsuccess")?.set; - // Instead, wrap by overriding after creation; simpler approach: intercept onsuccess event: - const origThen = req.onsuccess; + // Intercept onsuccess event after creation:
237-241: Remove unused local.origThen is unused.
- const origThen = req.onsuccess; - req.onsuccess = null; + req.onsuccess = null;
159-163: Consider restoring original indexedDB.open in afterEach for isolation.You restore it within tests, but a safety net in afterEach helps if a test aborts early.
Also applies to: 275-290
tests/unit/library.test.js (1)
44-47: epubjs interop: prefer stable ESM default resolution.Accessing require('epubjs') in ESM-heavy setups can be flaky. Consider storing the ctor during jest.mock factory and exporting it for direct use, or use import() consistently.
tests/build_library.test.js (2)
331-354: Assertions depend on a specific cover placeholder implementation.If placeholder is a data URL, assert non-empty src or presence of element rather than prefix to reduce brittleness across implementations.
- expect(img.src.startsWith('data:image/png')).toBe(true); + expect(img.src).toBeTruthy();
486-521: Minor: remove superfluous blank lines to tighten tests.Several blocks have multiple empty lines that reduce readability.
Build/src/library.js (4)
69-73: Render items in parallel to avoid N serial awaits.This loop loads covers sequentially and slows large libraries. Batch with Promise.all.
- for (const entry of fileEntries) { - const item = await createLibraryItem(entry); - libraryContent.appendChild(item); - } + const items = await Promise.all(fileEntries.map(createLibraryItem)); + items.forEach(item => libraryContent.appendChild(item));
99-120: Await epubjs ready and dispose temporary Book to prevent leaks.Initialize fully before reading cover/metadata and always destroy the temp instance.
- try { + let tempBook; + try { // If using the File System Access API: const file = (typeof fileEntry.getFile === 'function') ? await fileEntry.getFile() : fileEntry; const arrayBuffer = await file.arrayBuffer(); - const tempBook = ePub(arrayBuffer); + tempBook = ePub(arrayBuffer); + await tempBook.ready; // Attempt to retrieve cover image URL const coverUrl = await tempBook.coverUrl(); @@ - const metadata = await tempBook.loaded.metadata; + const metadata = await tempBook.loaded.metadata; if (metadata.title) { titleDiv.textContent = metadata.title; } } catch (err) { console.error('Error loading cover for', fileEntry.name, err); + } finally { + try { await tempBook?.destroy?.(); } catch {} }
90-97: Add alt text for accessibility.Ensure the cover image has meaningful alt text; update when title is known.
const img = document.createElement('img'); img.className = 'library-cover'; img.src = ''; + img.alt = 'Cover'; @@ - if (metadata.title) { - titleDiv.textContent = metadata.title; - } + if (metadata.title) { + titleDiv.textContent = metadata.title; + img.alt = `Cover: ${metadata.title}`; + }Also applies to: 115-117
45-49: Guard against missing files collection on input events.Avoid crashes when e.target or files is undefined.
-export function handleLibraryFiles(e) { - const files = Array.from(e.target.files); +export function handleLibraryFiles(e) { + const files = Array.from(e?.target?.files ?? []); displayLibraryGrid(files); toggleLibrary(true); }Build/src/book.js (1)
223-229: Use the cached locations reference for consistency.Minor consistency/readability tweak.
- if (pageNumber >= 0 && pageNumber < book.locations.length()) { - const cfi = book.locations.cfiFromLocation(pageNumber); + if (pageNumber >= 0 && pageNumber < locations.length()) { + const cfi = locations.cfiFromLocation(pageNumber); rendition.display(cfi); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
Build/package-lock.jsonis excluded by!**/package-lock.jsonBuild/public/source.svgis excluded by!**/*.svgapple-touch-icon-180x180.pngis excluded by!**/*.pngassets/index-D-qNE1dZ.js.mapis excluded by!**/*.mapfavicon.icois excluded by!**/*.icofavicon.pngis excluded by!**/*.pngmaskable-icon-512x512.pngis excluded by!**/*.pngpwa-192x192.pngis excluded by!**/*.pngpwa-512x512.pngis excluded by!**/*.pngpwa-64x64.pngis excluded by!**/*.pngsource.svgis excluded by!**/*.svgsw.js.mapis excluded by!**/*.mapworkbox-3bd99cbd.js.mapis excluded by!**/*.map
📒 Files selected for processing (27)
Build/.gitignore(1 hunks)Build/eslint.config.mjs(1 hunks)Build/export.mjs(1 hunks)Build/index.html(1 hunks)Build/package.json(1 hunks)Build/src/book.js(1 hunks)Build/src/indexedDB.js(1 hunks)Build/src/library.js(1 hunks)Build/src/main.js(1 hunks)Build/src/style.css(1 hunks)Build/vite.config.mjs(1 hunks)assets/index-D7CYcCYQ.css(1 hunks)index.html(2 hunks)manifest.json(0 hunks)manifest.webmanifest(1 hunks)registerSW.js(1 hunks)service-worker.js(0 hunks)sw.js(1 hunks)tests/build_indexedDB.test.js(1 hunks)tests/build_library.test.js(1 hunks)tests/helpers/indexeddb-mock.spec.js(1 hunks)tests/unit/book.test.js(1 hunks)tests/unit/indexedDB.test.js(1 hunks)tests/unit/library.test.js(1 hunks)tests/unit/main.test.js(1 hunks)tests/unit/style.test.js(1 hunks)workbox-3bd99cbd.js(1 hunks)
💤 Files with no reviewable changes (2)
- service-worker.js
- manifest.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-12T02:12:23.004Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: registerSW.js:1-1
Timestamp: 2025-09-12T02:12:23.004Z
Learning: Files identified as minified or generated (like registerSW.js) should not be manually edited, as changes will be overwritten during the build process. Suggestions for such files should target the source files instead.
Applied to files:
registerSW.jsmanifest.webmanifest
📚 Learning: 2025-09-12T02:11:18.442Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: Build/export.mjs:13-15
Timestamp: 2025-09-12T02:11:18.442Z
Learning: In HTMLReader project, there is no CNAME file in the root directory - the suggestion to preserve CNAME in Build/export.mjs was incorrect.
Applied to files:
Build/package.jsonBuild/index.htmlBuild/export.mjs
📚 Learning: 2025-09-12T02:13:17.615Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: manifest.webmanifest:1-1
Timestamp: 2025-09-12T02:13:17.615Z
Learning: The manifest.webmanifest file in the HTMLReader repository is a generated file and should not be manually edited.
Applied to files:
manifest.webmanifest
📚 Learning: 2025-09-12T03:51:20.249Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: assets/index-D7CYcCYQ.css:1-1
Timestamp: 2025-09-12T03:51:20.249Z
Learning: When reviewing minified CSS files (like assets/index-D7CYcCYQ.css), accessibility and other suggestions should be applied to the source CSS files instead, as minified files are typically build artifacts that get overwritten.
Applied to files:
assets/index-D7CYcCYQ.css
🧬 Code graph analysis (12)
tests/unit/book.test.js (1)
Build/src/book.js (5)
rendition(7-7)viewer(24-24)toc(176-176)tocContent(23-23)tocContainer(22-22)
tests/unit/library.test.js (3)
Build/src/book.js (1)
openBookFromEntry(71-86)Build/src/library.js (1)
dirHandle(23-23)Build/src/main.js (1)
showError(65-68)
tests/build_library.test.js (2)
Build/src/library.js (4)
handleLibraryFiles(45-49)toggleLibrary(135-146)dirHandle(23-23)img(90-90)tests/unit/library.test.js (1)
mod(45-45)
tests/build_indexedDB.test.js (1)
tests/build_library.test.js (1)
candidates(32-39)
tests/unit/main.test.js (1)
Build/src/main.js (5)
showLoading(47-49)hideLoading(55-57)showError(65-68)hideError(74-76)fileInput(6-6)
tests/helpers/indexeddb-mock.spec.js (1)
tests/build_indexedDB.test.js (3)
db(124-124)storeLibraryHandle(168-168)getStoredLibraryHandle(169-169)
Build/src/indexedDB.js (1)
tests/unit/indexedDB.test.js (1)
request(98-123)
Build/src/main.js (2)
Build/src/book.js (11)
tocButton(15-15)prevButton(16-16)nextButton(17-17)currentPageInput(18-18)overlay(19-19)openBook(35-58)prevPage(198-200)nextPage(207-209)goToPage(222-229)toggleToc(246-249)closeToc(256-259)Build/src/library.js (4)
overlay(9-9)openLibrary(20-40)toggleLibrary(135-146)handleLibraryFiles(45-49)
tests/unit/style.test.js (3)
tests/unit/library.test.js (1)
main(52-52)Build/src/book.js (2)
viewer(24-24)toc(176-176)Build/src/library.js (1)
msg(64-64)
Build/src/library.js (3)
Build/src/book.js (4)
file(36-36)file(76-76)arrayBuffer(77-77)openBookFromEntry(71-86)Build/src/main.js (1)
showError(65-68)Build/src/indexedDB.js (2)
getStoredLibraryHandle(46-55)storeLibraryHandle(27-36)
Build/src/book.js (2)
Build/src/main.js (4)
overlay(15-15)showError(65-68)showLoading(47-49)hideLoading(55-57)Build/src/library.js (4)
overlay(9-9)file(101-103)toggleLibrary(135-146)arrayBuffer(104-104)
workbox-3bd99cbd.js (1)
sw.js (5)
e(1-1)n(1-1)r(1-1)o(1-1)l(1-1)
🪛 Biome (2.1.2)
workbox-3bd99cbd.js
[error] 31-31: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 31-31: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
Build/src/style.css
[error] 115-115: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
z-index is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🔇 Additional comments (5)
Build/.gitignore (1)
1-42: LGTM — sensible ignores for Node/Vite projectsCovers common outputs, caches, IDE/OS files. No action needed.
Build/vite.config.mjs (1)
20-24: pwaAssets source path — confirmed
Build/public/source.svg exists; no change required.Build/eslint.config.mjs (1)
8-15: Incorrect — string-basedextendsare valid in flat config; the current code is not broken.Flat config accepts
extendsentries as strings (pluginName/configName) as well as config objects/arrays;extends: ["js/recommended"]withplugins: { js }is supported, and importing/usingjs.configs.recommendedis an optional alternative (not a required fix). (eslint.org)Likely an incorrect or invalid review comment.
Build/index.html (1)
66-66: SW registration present — confirm it's included in the Build outputregisterSW.js exists at repo root and root index.html includes <script id="vite-plugin-pwa:register-sw" src="./registerSW.js"></script>; sw.js and workbox files are present. The Build/index.html in this PR currently only references <script type="module" src="./src/main.js"></script> — confirm the PWA registration script is injected into Build/index.html or bundled into the production build so offline support works.
Build/src/book.js (1)
246-259: Centralize overlay ownership — Build/src/book.js and Build/src/library.js both mutate #overlayBoth modules call overlay.classList.* (Build/src/book.js:246–259 — toggleToc/closeToc; Build/src/library.js:136–145). Tests also touch overlay (tests/unit/book.test.js:281–284, 378–381). Concurrent toggles can desync the UI.
- Action: centralize overlay control (single owner/shared controller) OR
- If keeping multiple owners: avoid classList.toggle; use explicit add/remove with contains() guards before changing.
| // Clean repo root (skip hidden files/folders) | ||
| fs.readdirSync(root).forEach(file => { | ||
| if (!keep.has(file) && !file.startsWith('.')) { // <-- skip hidden | ||
| rimraf.sync(path.join(root, file)); | ||
| console.log('Deleted:', file); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard before destructive deletes; bail if dist missing/empty
Script can wipe repo root and then fail when dist is absent. Add checks and use rimrafSync.
Apply:
// Clean repo root (skip hidden files/folders)
-fs.readdirSync(root).forEach(file => {
- if (!keep.has(file) && !file.startsWith('.')) { // <-- skip hidden
- rimraf.sync(path.join(root, file));
- console.log('Deleted:', file);
- }
-});
+if (!fs.existsSync(dist)) {
+ console.error(`Build/export.mjs: dist not found at ${dist}`);
+ process.exit(1);
+}
+if (fs.readdirSync(dist).length === 0) {
+ console.error(`Build/export.mjs: dist is empty at ${dist}`);
+ process.exit(1);
+}
+fs.readdirSync(root).forEach(file => {
+ if (!keep.has(file) && !file.startsWith('.')) {
+ rimrafSync(path.join(root, file));
+ console.log('Deleted:', file);
+ }
+});
// Copy each item inside dist individually
-fs.readdirSync(dist).forEach(item => {
+fs.readdirSync(dist).forEach(item => {Optional: export to a temp dir and rename into place for atomicity on failure.
Also applies to: 25-37
🤖 Prompt for AI Agents
In Build/export.mjs around lines 16 to 22 (and also apply same guards to lines
25 to 37), add a pre-check that the target 'dist' path exists and is not empty
before performing any destructive deletes, and if the check fails immediately
bail with a clear error message instead of continuing; replace
asynchronous/uncertain deletion calls with the synchronous rimraf API
(rimraf.sync or rimrafSync depending on the imported name) to ensure deletions
complete predictably, and wrap the deletion loop in a guard that only iterates
when the pre-check passes; optionally implement a safe deployment pattern by
exporting to a temporary directory and renaming into place atomically on success
to avoid partial state on failure.
| "htmltoolkit" | ||
| ], | ||
| "author": "NellowTCS", | ||
| "license": "MIT", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
License mismatch check
If root LICENSE is GPL-3.0 but this declares MIT, align repo-wide to one license to avoid compliance issues.
🏁 Script executed:
#!/bin/bash
rg -n --glob '!node_modules/**' -i '^license' Build/package.json
[ -f LICENSE ] && head -n1 LICENSE || trueLength of output: 46
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Build/package.json (lines 1-200) ----"
if [ -f Build/package.json ]; then
sed -n '1,200p' Build/package.json
else
echo "Build/package.json not found"
fi
echo
echo "---- LICENSE (first 80 lines) ----"
if [ -f LICENSE ]; then
sed -n '1,200p' LICENSE | sed -n '1,80p'
else
echo "LICENSE not found"
fiLength of output: 5120
Resolve license mismatch: make repository license consistent (GPL-3.0 vs MIT)
Root LICENSE is GNU GPL v3 but Build/package.json lists "license": "MIT" — this is a compliance risk. Align the repo by either updating Build/package.json's license to match the root (e.g., "GPL-3.0-only" or "GPL-3.0-or-later") or replacing the root LICENSE with the intended MIT text and updating all package metadata/README accordingly.
Files: Build/package.json (license field), ./LICENSE (repo root).
🤖 Prompt for AI Agents
In Build/package.json at line 24, the "license" field ("MIT") conflicts with the
repository root LICENSE (GNU GPL v3); update Build/package.json to use the
correct SPDX identifier that matches the root (e.g., "GPL-3.0-only" or
"GPL-3.0-or-later") so metadata and source license are consistent;
alternatively, if the project intent is MIT, replace the root LICENSE file with
the MIT text and update any README/package metadata accordingly—pick one
approach, apply the change consistently across package.json files and the root
LICENSE, and run a package/CI check to ensure the SPDX string is valid.
| import { openBook, prevPage, nextPage, goToPage, toggleToc, closeToc } from "./book"; | ||
| import { openLibrary, handleLibraryFiles, toggleLibrary } from "./library"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Register the Service Worker; PWA/offline won’t work without it
No registration is present. If using vite-plugin-pwa, import and call virtual:pwa-register; otherwise manually register sw.js.
+// PWA registration (vite-plugin-pwa)
+import { registerSW } from 'virtual:pwa-register';
+registerSW({ immediate: true });If not using the plugin:
+if ('serviceWorker' in navigator) {
+ window.addEventListener('load', () => {
+ navigator.serviceWorker.register('/sw.js').catch(console.error);
+ });
+}📝 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.
| import { openBook, prevPage, nextPage, goToPage, toggleToc, closeToc } from "./book"; | |
| import { openLibrary, handleLibraryFiles, toggleLibrary } from "./library"; | |
| import { openBook, prevPage, nextPage, goToPage, toggleToc, closeToc } from "./book"; | |
| import { openLibrary, handleLibraryFiles, toggleLibrary } from "./library"; | |
| if ('serviceWorker' in navigator) { | |
| window.addEventListener('load', () => { | |
| navigator.serviceWorker.register('/sw.js').catch(console.error); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In Build/src/main.js around lines 1 to 3 the app imports modules but does not
register a service worker, so PWA/offline will not work; add SW registration
immediately after the imports — if the project uses vite-plugin-pwa, import the
virtual register function (virtual:pwa-register) and call it to enable the
plugin-managed SW lifecycle, otherwise add a navigator.serviceWorker
registration that checks for navigator.serviceWorker availability and registers
'/sw.js' with success/error logging and optional update handling; place this
registration after the existing imports and ensure it only runs in production
(or when window is defined).
tests/build_indexedDB.test.js
Outdated
| // Dynamically import the module under test from likely locations. | ||
| // This keeps tests flexible without adding config changes. | ||
| async function loadModule() { | ||
| const candidates = [ | ||
| '../src/build_indexedDB.js', | ||
| '../build_indexedDB.js', | ||
| '../src/utils/build_indexedDB.js', | ||
| '../app/build_indexedDB.js', | ||
| ]; | ||
| let lastErr; | ||
| for (const p of candidates) { | ||
| try { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| const mod = await import(p); | ||
| return mod; | ||
| } catch (e) { | ||
| lastErr = e; | ||
| } | ||
| } | ||
| throw lastErr || new Error('Could not locate module under test'); | ||
| } |
There was a problem hiding this comment.
Module under test likely mis-resolved; add correct candidate paths (e.g., Build/src/indexedDB.js)
As written, none of the candidates include “Build/src/indexedDB.js”; tests will throw “Could not locate module under test”.
- const candidates = [
- '../src/build_indexedDB.js',
- '../build_indexedDB.js',
- '../src/utils/build_indexedDB.js',
- '../app/build_indexedDB.js',
- ];
+ const candidates = [
+ // Common locations/names
+ '../Build/src/indexedDB.js',
+ '../src/indexedDB.js',
+ '../lib/indexedDB.js',
+ '../app/indexedDB.js',
+ // Fallback legacy names
+ '../src/build_indexedDB.js',
+ '../build_indexedDB.js',
+ '../src/utils/build_indexedDB.js',
+ '../app/build_indexedDB.js',
+ ];📝 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.
| // Dynamically import the module under test from likely locations. | |
| // This keeps tests flexible without adding config changes. | |
| async function loadModule() { | |
| const candidates = [ | |
| '../src/build_indexedDB.js', | |
| '../build_indexedDB.js', | |
| '../src/utils/build_indexedDB.js', | |
| '../app/build_indexedDB.js', | |
| ]; | |
| let lastErr; | |
| for (const p of candidates) { | |
| try { | |
| // eslint-disable-next-line no-await-in-loop | |
| const mod = await import(p); | |
| return mod; | |
| } catch (e) { | |
| lastErr = e; | |
| } | |
| } | |
| throw lastErr || new Error('Could not locate module under test'); | |
| } | |
| // Dynamically import the module under test from likely locations. | |
| // This keeps tests flexible without adding config changes. | |
| async function loadModule() { | |
| const candidates = [ | |
| // Common locations/names | |
| '../Build/src/indexedDB.js', | |
| '../src/indexedDB.js', | |
| '../lib/indexedDB.js', | |
| '../app/indexedDB.js', | |
| // Fallback legacy names | |
| '../src/build_indexedDB.js', | |
| '../build_indexedDB.js', | |
| '../src/utils/build_indexedDB.js', | |
| '../app/build_indexedDB.js', | |
| ]; | |
| let lastErr; | |
| for (const p of candidates) { | |
| try { | |
| // eslint-disable-next-line no-await-in-loop | |
| const mod = await import(p); | |
| return mod; | |
| } catch (e) { | |
| lastErr = e; | |
| } | |
| } | |
| throw lastErr || new Error('Could not locate module under test'); | |
| } |
🤖 Prompt for AI Agents
In tests/build_indexedDB.test.js around lines 175-195 the dynamic import
candidate list is missing the actual build output path so the module can't be
resolved; add the correct build candidates such as 'Build/src/indexedDB.js' (and
optionally 'Build/indexedDB.js' and 'build/indexedDB.js') to the candidates
array (keeping the existing order and error capture) so the loader will find the
compiled module during tests.
tests/build_library.test.js
Outdated
| function loadModule() { | ||
| const candidates = [ | ||
| './build_library.js', | ||
| './src/build_library.js', | ||
| './lib/build_library.js', | ||
| './app/build_library.js', | ||
| './frontend/build_library.js', | ||
| './client/build_library.js', | ||
| ]; |
There was a problem hiding this comment.
Module discovery points to build_library.js which doesn’t exist; target library.js instead.
Also, current loadModule requires the module before applying mocks (ordering bug). Split resolution from loading and use require.resolve to avoid executing the module early.
-function loadModule() {
- const candidates = [
- './build_library.js',
- './src/build_library.js',
- './lib/build_library.js',
- './app/build_library.js',
- './frontend/build_library.js',
- './client/build_library.js',
- ];
- for (const p of candidates) {
- try {
- // Use require to allow reloading between tests
- // eslint-disable-next-line global-require, import/no-dynamic-require
- return { mod: require(p), path: p };
- } catch (e) {
- // continue
- }
- }
- throw new Error('Could not locate build_library module. Ensure the file is named build_library.js and is in the project root or src/.');
-}
+const path = require('node:path');
+const fs = require('node:fs');
+function resolveModulePath() {
+ const candidates = [
+ './library.js',
+ './src/library.js',
+ './lib/library.js',
+ './app/library.js',
+ './frontend/library.js',
+ './client/library.js',
+ '../../src/library.js',
+ '../../Build/src/library.js',
+ ];
+ for (const p of candidates) {
+ const abs = path.isAbsolute(p) ? p : path.join(__dirname, p);
+ if (fs.existsSync(abs)) return abs;
+ try { return require.resolve(p, { paths: [__dirname] }); } catch { /* try next */ }
+ }
+ throw new Error('Could not locate library module. Ensure the file is named library.js and is in src/ or Build/src/.');
+}Also applies to: 58-59
🤖 Prompt for AI Agents
In tests/build_library.test.js around lines 31-39 (and similarly at lines
58-59): the module discovery currently looks for non-existent build_library.js
and also requires the module immediately (causing mocks to be applied too late).
Update the candidates array entries to target "library.js" paths instead of
"build_library.js"; separate resolution from loading by using require.resolve to
pick the correct file path first, apply any test mocks/stubs, then call
require(resolvedPath) to load/execute the module. Ensure the same
require.resolve approach is used at the other occurrence on lines 58-59 so
resolution happens before requiring and mocks are in place.
tests/unit/indexedDB.test.js
Outdated
| const candidates = [ | ||
| "src/indexedDB.js", | ||
| "src/utils/indexedDB.js", | ||
| "src/lib/indexedDB.js", | ||
| "indexedDB.js", | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Build/src to candidate paths to locate the module.
Without this, the suite may fail to import in repos that place code under Build/.
- const candidates = [
- "src/indexedDB.js",
- "src/utils/indexedDB.js",
- "src/lib/indexedDB.js",
- "indexedDB.js",
- ];
+ const candidates = [
+ "src/indexedDB.js",
+ "Build/src/indexedDB.js",
+ "src/utils/indexedDB.js",
+ "src/lib/indexedDB.js",
+ "indexedDB.js",
+ ];Also applies to: 149-157
🤖 Prompt for AI Agents
In tests/unit/indexedDB.test.js around lines 133-138 (and similarly for lines
149-157), the candidate paths array for locating the module misses repositories
that place compiled sources under Build/src (or build/src); update the arrays to
include the build path variants such as "build/src/indexedDB.js" and
"Build/src/indexedDB.js" (or the project’s canonical casing) so the test can
resolve the module in repos that output to Build/src.
tests/unit/library.test.js
Outdated
| const LIB_PATH = '../../src/library'; // Will be auto-adjusted by the script below if needed | ||
|
|
||
| // Mock sibling modules used by the library (paths will be auto-adjusted by script if needed) | ||
| jest.mock('../../src/indexedDB', () => ({ | ||
| storeLibraryHandle: jest.fn(), | ||
| getStoredLibraryHandle: jest.fn(), | ||
| })); | ||
| jest.mock('../../src/book', () => ({ | ||
| openBookFromEntry: jest.fn(), | ||
| })); | ||
| jest.mock('../../src/main', () => ({ | ||
| showError: jest.fn(), | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Path brittleness: allow Build/src and duplicate mocks accordingly.
LIB_PATH and mocks are tied to ../../src. Add Build/src fallbacks to avoid breakage when files move.
-const LIB_PATH = '../../src/library'; // Will be auto-adjusted by the script below if needed
+const LIB_CANDIDATES = ['../../src/library.js', '../../Build/src/library.js', '../../library.js'];
@@
-jest.mock('../../src/indexedDB', () => ({
+// Support both src/ and Build/src/ so whichever the module imports resolves to a mocked module
+const idbFactory = () => ({
storeLibraryHandle: jest.fn(),
getStoredLibraryHandle: jest.fn(),
-}));
-jest.mock('../../src/book', () => ({
+});
+jest.mock('../../src/indexedDB', idbFactory);
+jest.mock('../../Build/src/indexedDB', idbFactory);
+const bookFactory = () => ({
openBookFromEntry: jest.fn(),
-}));
-jest.mock('../../src/main', () => ({
+});
+jest.mock('../../src/book', bookFactory);
+jest.mock('../../Build/src/book', bookFactory);
+const mainFactory = () => ({
showError: jest.fn(),
-}));
+});
+jest.mock('../../src/main', mainFactory);
+jest.mock('../../Build/src/main', mainFactory);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const LIB_PATH = '../../src/library'; // Will be auto-adjusted by the script below if needed | |
| // Mock sibling modules used by the library (paths will be auto-adjusted by script if needed) | |
| jest.mock('../../src/indexedDB', () => ({ | |
| storeLibraryHandle: jest.fn(), | |
| getStoredLibraryHandle: jest.fn(), | |
| })); | |
| jest.mock('../../src/book', () => ({ | |
| openBookFromEntry: jest.fn(), | |
| })); | |
| jest.mock('../../src/main', () => ({ | |
| showError: jest.fn(), | |
| })); | |
| const LIB_CANDIDATES = ['../../src/library.js', '../../Build/src/library.js', '../../library.js']; | |
| // Mock sibling modules used by the library (paths will be auto-adjusted by script if needed) | |
| // Support both src/ and Build/src/ so whichever the module imports resolves to a mocked module | |
| const idbFactory = () => ({ | |
| storeLibraryHandle: jest.fn(), | |
| getStoredLibraryHandle: jest.fn(), | |
| }); | |
| jest.mock('../../src/indexedDB', idbFactory); | |
| jest.mock('../../Build/src/indexedDB', idbFactory); | |
| const bookFactory = () => ({ | |
| openBookFromEntry: jest.fn(), | |
| }); | |
| jest.mock('../../src/book', bookFactory); | |
| jest.mock('../../Build/src/book', bookFactory); | |
| const mainFactory = () => ({ | |
| showError: jest.fn(), | |
| }); | |
| jest.mock('../../src/main', mainFactory); | |
| jest.mock('../../Build/src/main', mainFactory); |
🤖 Prompt for AI Agents
In tests/unit/library.test.js around lines 11 to 23, the test hardcodes
../../src paths for LIB_PATH and jest.mock calls which breaks when source files
are moved to Build/src; update the file to attempt requiring '../../src' paths
first and if that fails fall back to '../../Build/src' equivalents, and
duplicate each jest.mock for the Build/src path so mocks exist for both
locations (same mock implementations for indexedDB, book, and main); ensure
LIB_PATH resolution uses the same fallback logic so imports and mocks remain
consistent.
tests/unit/library.test.js
Outdated
| async function loadLibraryModule() { | ||
| jest.resetModules(); // ensure module reads the fresh DOM at import time | ||
| setupDOM(); | ||
| return await import(LIB_PATH); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Resolve library module via candidates, not a single hard-coded path.
-async function loadLibraryModule() {
+async function loadLibraryModule() {
jest.resetModules(); // ensure module reads the fresh DOM at import time
setupDOM();
- return await import(LIB_PATH);
+ let lastErr;
+ for (const p of LIB_CANDIDATES) {
+ try { return await import(p); } catch (e) { lastErr = e; }
+ }
+ throw lastErr || new Error('Failed to locate library module');
}📝 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.
| async function loadLibraryModule() { | |
| jest.resetModules(); // ensure module reads the fresh DOM at import time | |
| setupDOM(); | |
| return await import(LIB_PATH); | |
| } | |
| async function loadLibraryModule() { | |
| jest.resetModules(); // ensure module reads the fresh DOM at import time | |
| setupDOM(); | |
| let lastErr; | |
| for (const p of LIB_CANDIDATES) { | |
| try { return await import(p); } catch (e) { lastErr = e; } | |
| } | |
| throw lastErr || new Error('Failed to locate library module'); | |
| } |
🤖 Prompt for AI Agents
In tests/unit/library.test.js around lines 77 to 81, the test currently imports
the library using a single hard-coded LIB_PATH which can break in different
environments; change loadLibraryModule to accept or define an ordered list of
candidate paths, iterate over candidates and attempt dynamic import for each
until one succeeds, resetting modules and calling setupDOM before each attempt,
and if none succeed throw a clear error listing tried candidates; ensure the
resolved successful path is returned so downstream tests use the actual resolved
module.
tests/unit/main.test.js
Outdated
| test('showLoading adds "show" class to #loading-message', async () => { | ||
| mockBookModule(); | ||
| mockLibraryModule(); | ||
| const mod = hasUnstableMockModule | ||
| ? await import('../../src/main.js').catch(async () => await import('../..//main.js')).catch(async () => await import('../../main.js')) | ||
| : require('../../src/main.js'); | ||
|
|
||
| // Some repositories may place the file at different relative paths; try fallbacks | ||
| const { showLoading } = mod || (await import('../../main.js')); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
DRY the import path fallback and include Build/src.
Path handling repeats and misses Build/src. Introduce a single loader and reuse.
- mockBookModule();
- mockLibraryModule();
- const mod = hasUnstableMockModule
- ? await import('../../src/main.js').catch(async () => await import('../..//main.js')).catch(async () => await import('../../main.js'))
- : require('../../src/main.js');
-
- // Some repositories may place the file at different relative paths; try fallbacks
- const { showLoading } = mod || (await import('../../main.js'));
+ mockBookModule();
+ mockLibraryModule();
+ const importMain = async () => {
+ const candidates = ['../../src/main.js', '../../Build/src/main.js', '../../main.js'];
+ for (const p of candidates) {
+ try { return hasUnstableMockModule ? await import(p) : require(p); } catch { /* try next */ }
+ }
+ throw new Error('main module not found');
+ };
+ const mod = await importMain();
+ const { showLoading } = mod;Repeat this replacement for other tests in this file that import main.js.
📝 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.
| test('showLoading adds "show" class to #loading-message', async () => { | |
| mockBookModule(); | |
| mockLibraryModule(); | |
| const mod = hasUnstableMockModule | |
| ? await import('../../src/main.js').catch(async () => await import('../..//main.js')).catch(async () => await import('../../main.js')) | |
| : require('../../src/main.js'); | |
| // Some repositories may place the file at different relative paths; try fallbacks | |
| const { showLoading } = mod || (await import('../../main.js')); | |
| test('showLoading adds "show" class to #loading-message', async () => { | |
| mockBookModule(); | |
| mockLibraryModule(); | |
| const importMain = async () => { | |
| const candidates = ['../../src/main.js', '../../Build/src/main.js', '../../main.js']; | |
| for (const p of candidates) { | |
| try { return hasUnstableMockModule ? await import(p) : require(p); } catch { /* try next */ } | |
| } | |
| throw new Error('main module not found'); | |
| }; | |
| const mod = await importMain(); | |
| const { showLoading } = mod; |
🤖 Prompt for AI Agents
In tests/unit/main.test.js around lines 72 to 81, the import fallback logic for
main.js is repeated and omits the Build/src path; replace the repeated inline
fallback with a single reusable loader function that attempts imports/require
from an ordered list of candidate paths (e.g. '../../src/main.js',
'../../main.js', '../../Build/src/main.js', '../../build/src/main.js' etc.),
returning the loaded module or throwing a clear error; update the test to call
that loader and destructure showLoading from its result, and apply the same
loader replacement to other tests in this file that import main.js.
tests/unit/main.test.js
Outdated
| test('showError handles empty/undefined message gracefully', async () => { | ||
| mockBookModule(); | ||
| mockLibraryModule(); | ||
| const mod = hasUnstableMockModule | ||
| ? await import('../../src/main.js').catch(async () => await import('../../main.js')) | ||
| : require('../../src/main.js'); | ||
|
|
||
| const { showError } = mod || (await import('../../main.js')); | ||
| const text = document.getElementById('error-text'); | ||
|
|
||
| expect(() => showError(undefined)).not.toThrow(); | ||
| expect(text.textContent).toBe(''); // undefined coerces to '' when set on textContent | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
showError(undefined) expectation doesn’t match current DOM behavior.
Setting textContent = undefined typically yields "undefined". If the goal is better UX, fix the implementation to coerce nullish to empty string; otherwise adjust the test.
Proposed change in Build/src/main.js:
export function showError(message) {
- errorText.textContent = message;
+ errorText.textContent = message ?? '';
errorMessage.classList.add('show');
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/unit/main.test.js around lines 126-138 the test assumes
showError(undefined) will set the error-text element's textContent to an empty
string, but setting textContent = undefined yields the string "undefined";
update the implementation instead of the test: in src/main.js (or
build/src/main.js) change showError to coerce nullish values to an empty string
(e.g. set textContent = message ?? '' or if (message == null) message = ''
before assigning) and keep the existing test expectation.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (9)
assets/index-D7CYcCYQ.css (1)
1-1: Minified build artifact — skip source-level review hereApply styling/accessibility tweaks in the source CSS (e.g., Build/src/style.css); minified assets will be overwritten on build. Using stored learning for generated/minified files.
Build/src/indexedDB.js (1)
27-35: Resolved: write resolves on tx.oncomplete — LGTMYou addressed the earlier race by resolving on
tx.oncomplete. This fixes false positives on failed commits.Build/src/style.css (1)
101-116: TOC z-index duplication fixed — nice cleanupSingle
z-index: 1010;remains; avoids override traps.workbox-3bd99cbd.js (1)
1-69: Prefer generated Workbox from VitePWA over vendoringKeeping a vendored bundle complicates upgrades and lint. Let the plugin emit workbox at build, or clearly vendor and exclude from lint/CodeQL.
If you keep it, pin version/provenance and exclude this file from linting.
sw.js (1)
1-1: Bound the runtime caches with ExpirationPluginPrevent unbounded growth for app-shell and assets caches.
- e.registerRoute(/.*\.(js|css|html)$/,new e.NetworkFirst({cacheName:"app-shell",plugins:[]}),"GET"), - e.registerRoute(/.*\.(png|ico|json)$/,new e.CacheFirst({cacheName:"assets",plugins:[]}),"GET") + e.registerRoute(/.*\.(js|css|html)$/,new e.NetworkFirst({ + cacheName:"app-shell", + plugins:[new e.ExpirationPlugin({maxEntries:50,maxAgeSeconds:7*24*60*60,purgeOnQuotaError:!0})] + }),"GET"), + e.registerRoute(/.*\.(png|ico|json)$/,new e.CacheFirst({ + cacheName:"assets", + plugins:[new e.ExpirationPlugin({maxEntries:150,maxAgeSeconds:30*24*60*60,purgeOnQuotaError:!0})] + }),"GET")registerSW.js (1)
1-1: Don't edit generated file; make SW lifecycle changes in source (vite-plugin-pwa config or app code).This file is minified/generated and will be overwritten. If you want error handling/update UX: configure vite-plugin-pwa (e.g., registerType: 'autoUpdate') or use virtual:pwa-register in app code to surface update/offline events. Also ensure you don’t double-register if both this script and app code register a SW.
Would you like a small patch showing virtual:pwa-register usage in src/main.js with a minimal “update available” toast?
Build/src/main.js (1)
1-2: Break circular dependency with book/library by moving UI helpers to ui.js.book/library depend on helpers exported here while this module imports them, creating an ES module cycle. Split helpers into a thin ui.js both import.
Minimal patch for this file:
@@ -import { openBook, prevPage, nextPage, goToPage, toggleToc, closeToc } from "./book"; -import { openLibrary, handleLibraryFiles, toggleLibrary } from "./library"; +import { openBook, prevPage, nextPage, goToPage, toggleToc, closeToc } from "./book"; +import { openLibrary, handleLibraryFiles, toggleLibrary } from "./library"; +import { showLoading, hideLoading, showError, hideError } from "./ui"; @@ -export function showLoading() { - loadingMessage.classList.add('show'); -} @@ -export function hideLoading() { - loadingMessage.classList.remove('show'); -} @@ -export function showError(message) { - errorText.textContent = message ?? ''; - errorMessage.classList.add('show'); -} @@ -export function hideError() { - errorMessage.classList.remove('show'); -} +// helpers now imported from ./uiAnd add a new Build/src/ui.js:
const loadingMessage = document.getElementById('loading-message'); const errorMessage = document.getElementById('error-message'); const errorText = document.getElementById('error-text'); export function showLoading() { loadingMessage.classList.add('show'); } export function hideLoading() { loadingMessage.classList.remove('show'); } export function showError(message) { errorText.textContent = message ?? ''; errorMessage.classList.add('show'); } export function hideError() { errorMessage.classList.remove('show'); }Update imports in book.js and library.js from "./main" to "./ui".
Also applies to: 47-76
Build/src/library.js (1)
35-41: Handle denied permissions by clearing the stored handleOtherwise you’ll keep failing on subsequent opens. Clear and let the next call re-prompt.
- if (perm !== 'granted') { - const res = await dirHandle.requestPermission({ mode: 'read' }); - if (res !== 'granted') throw new Error('Read permission was denied for the library directory.'); - } + if (perm !== 'granted') { + const res = await dirHandle.requestPermission({ mode: 'read' }); + if (res !== 'granted') { + await storeLibraryHandle(null); + throw new Error('Read permission was denied for the library directory.'); + } + }Build/src/book.js (1)
101-106: Dispose previous book/rendition and detach listeners before reloadPrevents leaks and duplicate handlers on reopen. This aligns with prior feedback.
- if (book) { - book = null; - rendition = null; - viewer.innerHTML = ''; - } + if (book) { + // Remove global listeners first + window.removeEventListener('keyup', handleKeyEvents); + // Best-effort cleanup + try { await book.destroy?.(); } catch {} + try { rendition?.destroy?.(); } catch {} + viewer.innerHTML = ''; + book = null; + rendition = null; + }Outside this hunk, keep a named relocated handler so it can be removed when cleaning up:
// Define once at module scope let onRelocated; … onRelocated = (location) => { currentLocation = location.start.cfi; if (locations) { const pageNumber = book.locations.locationFromCfi(location.start.cfi); if (Number.isFinite(pageNumber)) currentPageInput.value = pageNumber + 1; } }; rendition.on('relocated', onRelocated); // And on cleanup: rendition?.off?.('relocated', onRelocated);
🧹 Nitpick comments (37)
Build/package.json (2)
29-45: Move Vite to devDependencies; add engines/privateVite is a build tool—keep it in devDependencies. Also add an engines guard and mark the package private to prevent accidental publish.
Apply:
"dependencies": { - "epubjs": "^0.3.93", - "jszip": "^3.10.1", - "vite": "^7.1.3" + "epubjs": "^0.3.93", + "jszip": "^3.10.1" }, "devDependencies": { + "vite": "^7.1.3", "@eslint/css": "^0.11.0", "@eslint/js": "^9.34.0", "@eslint/json": "^0.13.2", "@eslint/markdown": "^7.2.0", "eslint": "^9.34.0", "fs-extra": "^11.3.1", "globals": "^16.3.0", "jest": "^30.1.3", "rimraf": "^6.0.1", "vite-plugin-pwa": "^1.0.3" }And near the top:
{ "name": "htmlreader", "version": "2.1.0", + "private": true, + "engines": { "node": ">=18.0.0" },
6-12: Replace failing placeholder test scriptCurrent "test" exits with failure and can break CI. Either wire up Jest or make the script a no-op until tests are ready.
Apply one:
Option A (use Jest if tests exist):
- "test": "echo \"Error: no test specified\" && exit 1", + "test": "jest",Option B (temporary no-op):
- "test": "echo \"Error: no test specified\" && exit 1", + "test": "echo \"(no tests yet)\"",Build/export.mjs (1)
17-28: Hoist dist existence check out of the loopThe guard runs on every iteration. Move it before deleting to simplify flow and avoid repeated sync I/O.
Apply:
-// Clean repo root (skip hidden files/folders) -fs.readdirSync(root).forEach(file => { - if (!fs.existsSync(dist) || fs.readdirSync(dist).length === 0) { - console.error('ERROR: dist is missing or empty:', dist); - process.exit(1); - } - - if (!keep.has(file) && !file.startsWith('.')) { // <-- skip hidden - rimrafSync(path.join(root, file)); - console.log('Deleted:', file); - } -}); +// Ensure dist is present before destructive ops +if (!fs.existsSync(dist) || fs.readdirSync(dist).length === 0) { + console.error('ERROR: dist is missing or empty:', dist); + process.exit(1); +} + +// Clean repo root (skip hidden files/folders) +fs.readdirSync(root).forEach(file => { + if (!keep.has(file) && !file.startsWith('.')) { // <-- skip hidden + rimrafSync(path.join(root, file)); + console.log('Deleted:', file); + } +});Build/vite.config.mjs (2)
12-19: Add manifest id for install stabilityA stable "id" helps PWA installs remain consistent across paths.
manifest: { + id: './', name: 'HTMLReader', short_name: 'HTMLReader', start_url: './', display: 'standalone', background_color: '#f5f5f5', theme_color: '#2196F3', },
25-36: Broaden runtime caching to include fonts/SVG/webpCache common static assets in one rule.
- { - urlPattern: /.*\.(png|ico|json)$/, + { + urlPattern: /.*\.(png|ico|json|svg|webp|woff2|ttf)$/, handler: 'CacheFirst', options: { cacheName: 'assets' }, },Build/eslint.config.mjs (1)
8-14: ESLint flat config looks correct; ignore build outputAdd ignores to avoid linting generated files.
export default defineConfig([ + { ignores: ['dist/**'] }, js.configs.recommended, { files: ["**/*.{js,mjs,cjs}"], languageOptions: { globals: globals.browser } }, json.configs.recommended, markdown.configs.recommended, css.configs.recommended, ]);Build/src/indexedDB.js (2)
10-19: Harden DB open: handle blocked/versionchange and close old connectionsGuard against stale tabs blocking upgrades and ensure connections close on future version bumps.
Apply:
function getDB() { return new Promise((resolve, reject) => { const request = indexedDB.open("htmlreader-db", 1); request.onupgradeneeded = e => { const db = e.target.result; db.createObjectStore("handles", { keyPath: "name" }); + // Close this connection if a future upgrade happens while it's open. + db.onversionchange = () => db.close(); }; - request.onsuccess = e => resolve(e.target.result); + request.onsuccess = e => { + const db = e.target.result; + // Also set onversionchange on normal open. + db.onversionchange = () => db.close(); + resolve(db); + }; request.onerror = e => reject(e.target.error); + // Surface the common "blocked" case (another tab with old version). + request.onblocked = () => reject(new Error("IndexedDB open blocked by another tab/session. Close other tabs and retry.")); }); }
47-55: Optional: resolve reads on tx.oncomplete to avoid premature successRare, but a transaction can abort after a successful request. Resolve after
tx.oncompletefor symmetry with writes.export async function getStoredLibraryHandle() { const db = await getDB(); return new Promise((resolve, reject) => { const tx = db.transaction("handles", "readonly"); const store = tx.objectStore("handles"); - const req = store.get("library"); - req.onsuccess = () => resolve(req.result ? req.result.handle : null); - req.onerror = e => reject(e.target.error); + const req = store.get("library"); + let out = null; + req.onsuccess = () => { out = req.result ? req.result.handle : null; }; + tx.oncomplete = () => resolve(out); + tx.onabort = tx.onerror = e => reject(tx.error || (e && e.target && e.target.error)); }); }Build/src/style.css (1)
146-162: Respect reduced-motion preferences for slide-in panelsDisable transitions for users who prefer reduced motion.
+@media (prefers-reduced-motion: reduce) { + .toc-container, + .library-container { + transition: none; + } +}Also applies to: 244-264
workbox-3bd99cbd.js (1)
70-71: Avoid 404 on missing source mapEither commit the map or remove the reference.
-//# sourceMappingURL=workbox-3bd99cbd.js.map +/* source map intentionally omitted to avoid 404s in production */sw.js (1)
2-2: Remove sourcemap reference or commit the mapAvoid console 404s in production.
-//# sourceMappingURL=sw.js.map +/* source map intentionally omitted */Build/src/main.js (1)
30-30: Use closeToc for the Close button (avoid accidental reopen).The close button should call closeToc, not toggleToc.
Apply:
- closeTocButton.addEventListener('click', toggleToc); + closeTocButton.addEventListener('click', closeToc);index.html (1)
4-4: Tighten CSP for bundled assets; drop broad allowances and unsafe- in production.*With Vite-bundled local assets, you can reduce attack surface.
Proposed production CSP:
<meta http-equiv="Content-Security-Policy" content=" default-src 'self'; script-src 'self'; style-src 'self'; img-src 'self' data: blob:; font-src 'self'; connect-src 'self'; manifest-src 'self'; worker-src 'self'; base-uri 'self'; frame-ancestors 'none'; object-src 'none'">Note: keep a looser CSP only for dev/HMR.
Build/tests/unit/library.test.js (2)
11-11: Stale comment; no auto-adjust script exists.Either remove “auto-adjusted by the script below” or add the path-fallback logic (like in main.test).
Apply:
-const LIB_PATH = '../../src/library'; // Will be auto-adjusted by the script below if needed +const LIB_PATH = '../../src/library';
221-229: Add a permission-denied test path for openLibrary.Covers the branch where queryPermission/requestPermission returns/requests “denied”.
Example test:
test('permission denied shows error and does not throw', async () => { const LibraryModule = await loadLibraryModule(); const { getStoredLibraryHandle, showError } = getMockedDeps(); const dirHandle = { queryPermission: jest.fn().mockResolvedValue('denied'), requestPermission: jest.fn().mockResolvedValue('denied'), async *values() {} }; getStoredLibraryHandle.mockResolvedValue(dirHandle); await expect(LibraryModule.openLibrary()).resolves.toBeUndefined(); expect(showError).toHaveBeenCalledWith(expect.stringContaining('Read permission')); });Build/tests/unit/main.test.js (2)
49-63: Ensure mocks match import specifiers of the module under test.Using './book' and './library' may not intercept when importing '../../src/main.js'. Mock the exact specifiers used by main.js.
Apply:
- jest.unstable_mockModule('./book', () => bookMocks); + jest.unstable_mockModule('../../src/book.js', () => bookMocks); @@ - jest.mock('./book', () => bookMocks); + jest.mock('../../src/book', () => bookMocks);And similarly for library:
- jest.unstable_mockModule('./library', () => libraryMocks); + jest.unstable_mockModule('../../src/library.js', () => libraryMocks); @@ - jest.mock('./library', () => libraryMocks); + jest.mock('../../src/library', () => libraryMocks);
72-87: Reduce import fallback noise with a small helper.A helper to try paths once will simplify tests and avoid repeated chains.
Example:
async function importMain() { if (hasUnstableMockModule) { try { return await import('../../src/main.js'); } catch {} try { return await import('../../main.js'); } catch {} } return require('../../src/main.js'); }Then use: const mod = await importMain();
Also applies to: 89-105, 106-125, 140-158, 160-176, 177-188, 190-202, 204-216, 218-229, 231-245, 246-257, 259-273
Build/tests/unit/indexedDB.test.js (3)
18-31: Consider extracting mock implementations to a shared module.The
IDBRequestMockimplementation is duplicated across test files (Build/tests/unit/indexedDB.test.js,Build/tests/helpers/indexeddb-mock.spec.js, andBuild/tests/build_indexedDB.test.js). This violates the DRY principle and makes maintenance harder.Consider creating a shared test utilities module to house these mock implementations:
// Build/tests/test-utils/indexeddb-mocks.js export class IDBRequestMock { constructor(executor) { this.onsuccess = null; this.onerror = null; executor( (result) => queueMicrotask(() => this.onsuccess && this.onsuccess({ target: { result, source: undefined } })), (error) => queueMicrotask(() => this.onerror && this.onerror({ target: { error } })) ); } } // ... other shared mock classesThen import and use in your test files:
import { IDBRequestMock } from '../test-utils/indexeddb-mocks.js';
133-157: Module loading logic could be more robust.The dynamic module loading uses a fallback chain but might silently continue with the wrong module if multiple exist. Also, the error handling could be clearer about which paths were tried.
Consider improving the module resolution logic:
beforeAll(async () => { const candidates = [ "src/indexedDB.js", "src/utils/indexedDB.js", "src/lib/indexedDB.js", "indexedDB.js", ]; - let lastErr; + const errors = []; for (const p of candidates) { try { // eslint-disable-next-line no-await-in-loop moduleUnderTest = await import(require("path").isAbsolute(p) ? p : ("../".repeat(2) + p)); + console.log(`Successfully loaded module from: ${p}`); break; } catch (e) { - lastErr = e; + errors.push({ path: p, error: e.message }); } } if (!moduleUnderTest) { // Fallback to the file colocated under tests/unit if repository places code there (for the challenge context). try { moduleUnderTest = await import("./indexedDB.js"); } catch (_) { - throw lastErr || new Error("Failed to locate module exporting storeLibraryHandle/getStoredLibraryHandle"); + const triedPaths = errors.map(e => e.path).join(', '); + throw new Error(`Failed to locate module. Tried paths: ${triedPaths}`); } } });
194-228: Complex test setup for simulating errors could be simplified.The error simulation through extensive monkey-patching creates fragile tests. The approach of wrapping
onsuccesssetters and intercepting handlers is complex and hard to maintain.Consider using a more direct approach by setting flags on your mock before the operation:
// In your mock setup class ConfigurableIDBMock { constructor() { this.nextPutError = null; this.nextGetError = null; } // In ObjectStoreMock put(value) { return new IDBRequestMock((resolve, reject) => { if (this.mock.nextPutError) { const error = this.mock.nextPutError; this.mock.nextPutError = null; reject(error); } else { // normal put logic } }); } } // In test test("propagates put error", async () => { global.indexedDB.mock.nextPutError = new Error("put failed"); await expect(storeLibraryHandle({ any: "thing" })).rejects.toThrow("put failed"); });Build/tests/helpers/indexeddb-mock.spec.js (1)
173-203: Test creates complex class hierarchies for error simulation.The error injection tests create entire class hierarchies (
ThrowOnPutStore,ThrowOnPutDB,ThrowOnPutFactory) just to simulate a single error condition. This approach is overly complex.Consider a simpler approach by adding error injection directly to the base mock:
-class ThrowOnPutStore extends FakeObjectStore { - put(value) { - const req = new FakeIDBRequest(); - queueMicrotask(() => req.fail(new Error('put failed'))); - return req; - } -} -// ... more classes ... +// Add to existing FakeObjectStore +put(value) { + const req = new FakeIDBRequest(); + queueMicrotask(() => { + if (this._state._throwOnNextPut) { + this._state._throwOnNextPut = false; + req.fail(new Error('put failed')); + } else { + // normal logic + } + }); + return req; +} it('propagates put errors', async () => { - // All the class setup... - global.indexedDB = new ThrowOnPutFactory(); + global.indexedDB.instances.get('htmlreader-db').stateMap._throwOnNextPut = true; await expect(storeLibraryHandle({ id: 7 })).rejects.toThrow('put failed'); });Build/tests/build_library.test.js (1)
14-24: Compatibility shim could be cleaner.The Jest/Vitest compatibility approach using conditional globals works but could be more robust.
Consider using a more explicit compatibility layer:
-const isVitest = typeof vi !== 'undefined'; -// eslint-disable-next-line no-undef -const jestLike = isVitest ? vi : jest; +// Create a test framework adapter +const getTestFramework = () => { + if (typeof vi !== 'undefined') return { name: 'vitest', mock: vi }; + if (typeof jest !== 'undefined') return { name: 'jest', mock: jest }; + throw new Error('No test framework detected (jest or vitest required)'); +}; + +const testFramework = getTestFramework(); +const jestLike = testFramework.mock;Build/index.html (1)
30-31: Semantic issue: duplicate file inputs for similar purposes.Having both
file-inputandlibrary-inputwith similar purposes (opening EPUB files) could be confusing. Consider consolidating or clearly differentiating their purposes.Consider using a single input with dynamic behavior or clearer naming:
-<input type="file" id="file-input" class="file-input" accept=".epub"> -<!-- Fallback multiple file input for library import --> -<input type="file" id="library-input" class="file-input" accept=".epub" multiple> +<input type="file" id="single-file-input" class="file-input" accept=".epub" title="Open single EPUB"> +<input type="file" id="bulk-import-input" class="file-input" accept=".epub" multiple title="Import multiple EPUBs to library">Build/tests/build_indexedDB.test.js (1)
235-239: Test for undefined storage is potentially misleading.Storing
undefinedand expecting to retrieveundefined(notnull) tests an edge case that might not reflect real usage. IndexedDB typically doesn't distinguish between undefined and absence.Consider documenting the expected behavior more clearly or adjusting expectations:
it('supports undefined handle value and returns undefined (not null) when present', async () => { + // Note: This tests that the module preserves undefined values, + // though in practice library handles should always be defined objects await storeLibraryHandle(undefined); const retrieved = await getStoredLibraryHandle(); expect(retrieved).toBeUndefined(); });Build/tests/unit/book.test.js (3)
313-346: Reapply the library mock after jest.resetModules to avoid real module side effectsAfter reset, the previous jest.mock for '../../src/library' is cleared; importing the real module risks DOM assumptions and circular init. Remock it here before importing the SUT.
jest.resetModules(); setupDomSkeleton(); global.FileReader = MockFileReader; -jest.doMock('epubjs', () => { +jest.doMock('epubjs', () => { // ... }); -const mainMocks2 = await import('../../src/main'); -await import('../../src/library'); -const mod = await import('../../src/book.js'); +jest.doMock('../../src/library', () => ({ toggleLibrary: jest.fn() })); +const mainMocks2 = await import('../../src/main'); +const mod = await import('../../src/book.js');
136-143: Add a regression test to detect duplicate keyup handlersSimulate one keyup and assert nextPage is called once to guard against event listener duplication.
// Add near navigation controls tests test('keyup ArrowRight triggers nextPage once', async () => { const file = new Blob([new Uint8Array([1])], { type: 'application/epub+zip' }); Object.defineProperty(file, 'name', { value: 'ok.epub' }); bookModule.openBook({ target: { files: [file] } }); await Promise.resolve(); await new Promise(setImmediate); const spy = jest.spyOn(bookModule, 'nextPage'); window.dispatchEvent(new KeyboardEvent('keyup', { key: 'ArrowRight' })); expect(spy).toHaveBeenCalledTimes(1); });Also applies to: 223-258, 361-389
136-143: Prefer a dedicated flushPromises helper over setImmediateMinor stability improvement across environments.
const flushPromises = () => new Promise(queueMicrotask); // then: await flushPromises();Build/src/library.js (6)
43-47: Filter EPUBs case-insensitively
.EPUBfiles are missed.- if (entry.kind === 'file' && entry.name.endsWith('.epub')) { + if (entry.kind === 'file' && entry.name.toLowerCase().endsWith('.epub')) { files.push(entry); }
50-52: Don’t show an error on user cancel (AbortError)Treat cancellations as a no-op.
- } catch (err) { - showError('Failed to open library: ' + err.message); - } + } catch (err) { + if (err?.name !== 'AbortError') { + showError('Failed to open library: ' + err.message); + } + }
59-63: Filter file-input selection to EPUBs, tooAligns with directory scanning behavior.
-export function handleLibraryFiles(e) { - const files = Array.from(e.target.files); +export function handleLibraryFiles(e) { + const files = Array.from(e.target.files).filter(f => + (f.name || '').toLowerCase().endsWith('.epub') + ); displayLibraryGrid(files); toggleLibrary(true); }
75-87: Render items in parallel to avoid O(n) serial I/OParallelize cover/metadata loading and then append in order.
- for (const entry of fileEntries) { - const item = await createLibraryItem(entry); - libraryContent.appendChild(item); - } + const items = await Promise.all(fileEntries.map(createLibraryItem)); + for (const item of items) libraryContent.appendChild(item);
101-142: Accessibility + resource cleanup for cover preview
- Add alt text, lazy-loading, and async decoding.
- Dispose the temporary ePub instance to avoid leaks.
const img = document.createElement('img'); img.className = 'library-cover'; - img.src = ''; + img.src = ''; + img.alt = 'EPUB cover'; + img.loading = 'lazy'; + img.decoding = 'async'; @@ - try { + let tempBook; + try { @@ - const tempBook = ePub(arrayBuffer); + tempBook = ePub(arrayBuffer); @@ - } catch (err) { + } catch (err) { console.error('Error loading cover for', fileEntry.name, err); + } finally { + try { await tempBook?.destroy?.(); } catch {} }
149-160: Shared overlay element may clash with TOC overlayLibrary and TOC both toggle #overlay. Consider separate overlays or a ref-counted overlay manager to prevent UI flicker when both features interact.
Build/src/book.js (4)
35-41: Make EPUB validation case-insensitive and robust to missing MIME typeSome browsers omit type; uppercase extensions should pass.
-export function openBook(e) { +export function openBook(e) { const file = e.target.files[0]; if (!file) return; - if (file.type !== 'application/epub+zip' && !file.name.endsWith('.epub')) { + const name = (file.name || '').toLowerCase(); + const isEpub = file.type === 'application/epub+zip' || name.endsWith('.epub'); + if (!isEpub) { showError('The selected file is not a valid EPUB file.'); return; }
53-56: Normalize FileReader error messageAvoid “[object DOMError]” in UI.
- reader.onerror = function(e) { - hideLoading(); - showError('Error reading file: ' + e.target.error); - }; + reader.onerror = function(e) { + hideLoading(); + const msg = e?.target?.error?.message || e?.target?.error || 'Unknown error'; + showError(`Error reading file: ${msg}`); + };
120-126: Guard against invalid page index from locationFromCfiBe defensive if the adapter returns -1/undefined.
- if (locations) { - const pageNumber = book.locations.locationFromCfi(location.start.cfi); - currentPageInput.value = pageNumber + 1; - } + if (locations) { + const idx = book.locations.locationFromCfi(location.start.cfi); + if (Number.isFinite(idx) && idx >= 0) currentPageInput.value = idx + 1; + }
248-261: Overlay coupling between TOC and LibraryBoth modules toggle #overlay; simultaneous interactions may conflict. Consider a dedicated overlay per feature or a central overlay controller.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
Build/package-lock.jsonis excluded by!**/package-lock.jsonBuild/public/source.svgis excluded by!**/*.svgapple-touch-icon-180x180.pngis excluded by!**/*.pngassets/index-D-qNE1dZ.js.mapis excluded by!**/*.mapfavicon.icois excluded by!**/*.icofavicon.pngis excluded by!**/*.pngmaskable-icon-512x512.pngis excluded by!**/*.pngpwa-192x192.pngis excluded by!**/*.pngpwa-512x512.pngis excluded by!**/*.pngpwa-64x64.pngis excluded by!**/*.pngsource.svgis excluded by!**/*.svgsw.js.mapis excluded by!**/*.mapworkbox-3bd99cbd.js.mapis excluded by!**/*.map
📒 Files selected for processing (28)
Build/.gitignore(1 hunks)Build/eslint.config.mjs(1 hunks)Build/export.mjs(1 hunks)Build/index.html(1 hunks)Build/package.json(1 hunks)Build/src/book.js(1 hunks)Build/src/indexedDB.js(1 hunks)Build/src/library.js(1 hunks)Build/src/main.js(1 hunks)Build/src/style.css(1 hunks)Build/tests/build_indexedDB.test.js(1 hunks)Build/tests/build_library.test.js(1 hunks)Build/tests/helpers/indexeddb-mock.spec.js(1 hunks)Build/tests/unit/book.test.js(1 hunks)Build/tests/unit/indexedDB.test.js(1 hunks)Build/tests/unit/library.test.js(1 hunks)Build/tests/unit/main.test.js(1 hunks)Build/tests/unit/style.test.js(1 hunks)Build/vite.config.mjs(1 hunks)LICENSE(1 hunks)assets/index-D7CYcCYQ.css(1 hunks)index.html(2 hunks)manifest.json(0 hunks)manifest.webmanifest(1 hunks)registerSW.js(1 hunks)service-worker.js(0 hunks)sw.js(1 hunks)workbox-3bd99cbd.js(1 hunks)
💤 Files with no reviewable changes (2)
- service-worker.js
- manifest.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-12T02:13:17.615Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: manifest.webmanifest:1-1
Timestamp: 2025-09-12T02:13:17.615Z
Learning: The manifest.webmanifest file in the HTMLReader repository is a generated file and should not be manually edited.
Applied to files:
manifest.webmanifest
📚 Learning: 2025-09-12T02:12:23.004Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: registerSW.js:1-1
Timestamp: 2025-09-12T02:12:23.004Z
Learning: Files identified as minified or generated (like registerSW.js) should not be manually edited, as changes will be overwritten during the build process. Suggestions for such files should target the source files instead.
Applied to files:
manifest.webmanifestregisterSW.js
📚 Learning: 2025-09-12T02:11:18.442Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: Build/export.mjs:13-15
Timestamp: 2025-09-12T02:11:18.442Z
Learning: In HTMLReader project, there is no CNAME file in the root directory - the suggestion to preserve CNAME in Build/export.mjs was incorrect.
Applied to files:
Build/index.htmlBuild/package.jsonBuild/export.mjs
📚 Learning: 2025-09-12T03:51:20.249Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: assets/index-D7CYcCYQ.css:1-1
Timestamp: 2025-09-12T03:51:20.249Z
Learning: When reviewing minified CSS files (like assets/index-D7CYcCYQ.css), accessibility and other suggestions should be applied to the source CSS files instead, as minified files are typically build artifacts that get overwritten.
Applied to files:
assets/index-D7CYcCYQ.css
🧬 Code graph analysis (12)
Build/tests/build_indexedDB.test.js (1)
Build/tests/build_library.test.js (1)
candidates(32-39)
Build/src/main.js (2)
Build/src/book.js (8)
currentPageInput(18-18)overlay(19-19)openBook(35-58)prevPage(200-202)nextPage(209-211)goToPage(224-231)toggleToc(248-251)closeToc(258-261)Build/src/library.js (4)
overlay(9-9)openLibrary(20-53)toggleLibrary(149-160)handleLibraryFiles(59-63)
Build/tests/unit/indexedDB.test.js (1)
Build/tests/build_indexedDB.test.js (3)
db(124-124)storeLibraryHandle(168-168)getStoredLibraryHandle(169-169)
Build/tests/unit/main.test.js (1)
Build/src/main.js (5)
showLoading(47-49)hideLoading(55-57)showError(65-68)hideError(74-76)fileInput(6-6)
Build/tests/build_library.test.js (2)
Build/src/library.js (4)
handleLibraryFiles(59-63)toggleLibrary(149-160)dirHandle(23-23)img(104-104)Build/tests/unit/library.test.js (1)
mod(45-45)
Build/tests/unit/book.test.js (1)
Build/src/book.js (8)
file(36-36)file(76-76)rendition(7-7)viewer(24-24)toc(178-178)overlay(19-19)tocContent(23-23)tocContainer(22-22)
Build/src/indexedDB.js (1)
Build/tests/unit/indexedDB.test.js (1)
request(98-123)
Build/src/book.js (3)
Build/src/main.js (4)
overlay(15-15)showError(65-68)showLoading(47-49)hideLoading(55-57)Build/src/library.js (4)
overlay(9-9)file(115-117)toggleLibrary(149-160)arrayBuffer(118-118)Build/tests/unit/book.test.js (2)
ePub(34-34)ePub(85-85)
Build/tests/unit/style.test.js (2)
Build/src/book.js (5)
file(36-36)file(76-76)viewer(24-24)toc(178-178)overlay(19-19)Build/src/library.js (1)
msg(78-78)
workbox-3bd99cbd.js (1)
sw.js (5)
e(1-1)n(1-1)r(1-1)o(1-1)l(1-1)
Build/tests/unit/library.test.js (3)
Build/src/book.js (5)
book(6-6)overlay(19-19)openBookFromEntry(71-86)file(36-36)file(76-76)Build/src/library.js (1)
dirHandle(23-23)Build/src/main.js (1)
showError(65-68)
Build/src/library.js (3)
Build/src/book.js (4)
file(36-36)file(76-76)arrayBuffer(77-77)openBookFromEntry(71-86)Build/src/main.js (1)
showError(65-68)Build/src/indexedDB.js (2)
getStoredLibraryHandle(47-56)storeLibraryHandle(27-36)
🪛 Biome (2.1.2)
workbox-3bd99cbd.js
[error] 31-31: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 31-31: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (10)
LICENSE (1)
1-21: MIT license switch looks consistent repo-wideMatches Build/package.json's "license": "MIT". No issues spotted.
manifest.webmanifest (1)
1-1: Generated manifest acknowledged; identity matches Vite configPer your note, this file is generated—no manual edits. Name/short_name align with Build/vite.config.mjs. Ensure only one manifest is shipped after build (the generated one).
Using prior learning that manifest.webmanifest here is generated.
Build/.gitignore (1)
1-42: Gitignore set is solid for Vite appCovers node_modules, dist/build outputs, logs, env files, IDE/OS cruft. Looks good.
Build/tests/unit/style.test.js (2)
485-496: Good: asserts last-wins for duplicate decls and real file fallbackParsing merges duplicate props, so both old (fixture) and cleaned CSS pass. Solid resilience.
13-21: Ignore ESM change — repo is CommonJS; require() is valid. package.json has "type": "commonjs", so replacing require() with ESM imports is unnecessary.Likely an incorrect or invalid review comment.
Build/src/main.js (1)
1-3: SW registration: confirm single source of truth.index.html injects registerSW.js. Ensure main.js does not also register a SW to avoid double registration. Keep it in one place (plugin-injected script or virtual:pwa-register), not both.
Build/tests/unit/main.test.js (1)
126-138: Expectation relies on showError(nullish) → '' behavior.Production code should coalesce message (message ?? '') to satisfy this and avoid “undefined” in UI. See suggested patch in src/main.js.
Build/index.html (1)
4-13: Good CSP configuration for PWA security!The Content-Security-Policy is well-configured with appropriate restrictions while allowing necessary blob: and data: URLs for EPUB rendering. The policy follows security best practices with
upgrade-insecure-requestsandblock-all-mixed-content.Build/tests/build_indexedDB.test.js (1)
9-9: Good cross-platform compatibility with defer function!The
deferfunction provides compatibility between environments that may or may not havequeueMicrotask, ensuring tests work across different JavaScript runtimes.Build/tests/unit/book.test.js (1)
1-7: Solid, thorough test coverage for the book moduleMocks, DOM scaffold, and API surface coverage look great.
| window.removeEventListener('keyup', handleKeyEvents); | ||
| window.addEventListener('keyup', handleKeyEvents); | ||
| window.addEventListener('keyup', handleKeyEvents); | ||
| // Set the book title in header if available |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Duplicate keyup listener causes double navigation
You add the same handler twice; users will see two page turns per key press.
- window.removeEventListener('keyup', handleKeyEvents);
- window.addEventListener('keyup', handleKeyEvents);
- window.addEventListener('keyup', handleKeyEvents);
+ window.removeEventListener('keyup', handleKeyEvents);
+ window.addEventListener('keyup', handleKeyEvents);Run to confirm only one addEventListener remains after the fix:
🏁 Script executed:
#!/bin/bash
rg -nP "addEventListener\\(\\s*['\"]keyup['\"]\\s*,\\s*handleKeyEvents" Build/src/book.jsLength of output: 113
Remove duplicate 'keyup' listener — causes double navigation
rg shows two occurrences in Build/src/book.js at lines 128–129; delete the extra window.addEventListener('keyup', handleKeyEvents) so only one handler is registered.
🤖 Prompt for AI Agents
In Build/src/book.js around lines 127 to 130, there are two identical calls to
window.addEventListener('keyup', handleKeyEvents) which results in the handler
being registered twice; remove the duplicate (the second addEventListener at
line 129) leaving a single window.addEventListener('keyup', handleKeyEvents)
after the corresponding window.removeEventListener('keyup', handleKeyEvents) so
the keyup handler is only registered once.
| export function showError(message) { | ||
| errorText.textContent = message; | ||
| errorMessage.classList.add('show'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Coalesce error message to avoid “undefined” appearing; aligns with tests and UX.
Ensures showError(undefined) sets empty text instead of "undefined".
Apply:
-export function showError(message) {
- errorText.textContent = message;
- errorMessage.classList.add('show');
-}
+export function showError(message) {
+ errorText.textContent = message ?? '';
+ errorMessage.classList.add('show');
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function showError(message) { | |
| errorText.textContent = message; | |
| errorMessage.classList.add('show'); | |
| } | |
| export function showError(message) { | |
| errorText.textContent = message ?? ''; | |
| errorMessage.classList.add('show'); | |
| } |
🤖 Prompt for AI Agents
In Build/src/main.js around lines 65 to 68, showError currently assigns the
incoming message directly which causes "undefined" to appear when called with
undefined; update the function to coalesce falsy/undefined messages to an empty
string (e.g., use the nullish coalescing operator or equivalent) so
errorText.textContent is set to '' when message is undefined or null, then add
the 'show' class as before.
Build/tests/build_indexedDB.test.js
Outdated
| async function loadModule() { | ||
| const candidates = [ | ||
| '../src/build_indexedDB.js', | ||
| '../build_indexedDB.js', | ||
| '../src/utils/build_indexedDB.js', | ||
| '../app/build_indexedDB.js', | ||
| ]; | ||
| let lastErr; | ||
| for (const p of candidates) { | ||
| try { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| const mod = await import(p); | ||
| return mod; | ||
| } catch (e) { | ||
| lastErr = e; | ||
| } | ||
| } | ||
| throw lastErr || new Error('Could not locate module under test'); | ||
| } |
There was a problem hiding this comment.
Module path resolution differs from other test files.
The module path candidates use a different naming pattern (build_indexedDB.js) compared to the actual module name (indexedDB.js according to the AI summary). This will cause the module loading to fail.
Fix the module paths to match the actual module name:
async function loadModule() {
const candidates = [
- '../src/build_indexedDB.js',
- '../build_indexedDB.js',
- '../src/utils/build_indexedDB.js',
- '../app/build_indexedDB.js',
+ '../src/indexedDB.js',
+ '../indexedDB.js',
+ '../src/utils/indexedDB.js',
+ '../app/indexedDB.js',
];📝 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.
| async function loadModule() { | |
| const candidates = [ | |
| '../src/build_indexedDB.js', | |
| '../build_indexedDB.js', | |
| '../src/utils/build_indexedDB.js', | |
| '../app/build_indexedDB.js', | |
| ]; | |
| let lastErr; | |
| for (const p of candidates) { | |
| try { | |
| // eslint-disable-next-line no-await-in-loop | |
| const mod = await import(p); | |
| return mod; | |
| } catch (e) { | |
| lastErr = e; | |
| } | |
| } | |
| throw lastErr || new Error('Could not locate module under test'); | |
| } | |
| async function loadModule() { | |
| const candidates = [ | |
| '../src/indexedDB.js', | |
| '../indexedDB.js', | |
| '../src/utils/indexedDB.js', | |
| '../app/indexedDB.js', | |
| ]; | |
| let lastErr; | |
| for (const p of candidates) { | |
| try { | |
| // eslint-disable-next-line no-await-in-loop | |
| const mod = await import(p); | |
| return mod; | |
| } catch (e) { | |
| lastErr = e; | |
| } | |
| } | |
| throw lastErr || new Error('Could not locate module under test'); | |
| } |
🤖 Prompt for AI Agents
In Build/tests/build_indexedDB.test.js around lines 177 to 195, the test's
module candidate paths reference "build_indexedDB.js" but the actual module is
named "indexedDB.js"; update the candidates array to use the correct module
filename in each path (e.g. '../src/indexedDB.js', '../indexedDB.js',
'../src/utils/indexedDB.js', '../app/indexedDB.js') so the dynamic import can
locate and load the module under test, leaving the existing try/catch/lastErr
logic unchanged.
Build/tests/build_library.test.js
Outdated
| function loadModule() { | ||
| const candidates = [ | ||
| './build_library.js', | ||
| './src/build_library.js', | ||
| './lib/build_library.js', | ||
| './app/build_library.js', | ||
| './frontend/build_library.js', | ||
| './client/build_library.js', | ||
| ]; | ||
| for (const p of candidates) { | ||
|
|
||
| try { | ||
|
|
||
| // Use require to allow reloading between tests | ||
|
|
||
| // eslint-disable-next-line global-require, import/no-dynamic-require | ||
|
|
||
| return { mod: require(p), path: p }; | ||
|
|
||
| } catch (e) { | ||
|
|
||
| // continue | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| throw new Error('Could not locate build_library module. Ensure the file is named build_library.js and is in the project root or src/.'); | ||
| } |
There was a problem hiding this comment.
Module loading uses require() which may not work with ES modules.
The function uses require() to load modules dynamically, but the codebase appears to use ES modules (import/export). This could fail in ESM environments.
Update the module loading to support both CommonJS and ES modules:
function loadModule() {
const candidates = [
'./build_library.js',
'./src/build_library.js',
'./lib/build_library.js',
'./app/build_library.js',
'./frontend/build_library.js',
'./client/build_library.js',
];
for (const p of candidates) {
try {
- // Use require to allow reloading between tests
- // eslint-disable-next-line global-require, import/no-dynamic-require
- return { mod: require(p), path: p };
+ // Try ES module import first, fall back to require
+ try {
+ const mod = await import(p);
+ return { mod, path: p };
+ } catch (importErr) {
+ // Fallback to require for CommonJS
+ // eslint-disable-next-line global-require, import/no-dynamic-require
+ return { mod: require(p), path: p };
+ }
} catch (e) {
// continue
}
}
throw new Error('Could not locate build_library module. Ensure the file is named build_library.js and is in the project root or src/.');
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Build/tests/build_library.test.js around lines 31 to 59, the loader currently
uses require() which fails in ESM environments; update it to first try dynamic
import() (using a file:// URL or path resolved via import.meta.url) and fall
back to require() for CommonJS, and when using import() unwrap a default export
(return mod.default if present); also ensure tests can reload modules by
clearing require.cache for require() paths and by appending a cache-busting
query/timestamp to the import() specifier or using a fresh Blob/URL so repeated
imports re-evaluate the module.
Build/tests/build_library.test.js
Outdated
|
|
||
| const evt = { target: { files } }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Invalid event object structure for file input.
The test passes { target: { files } } where files is a plain array, but the actual DOM FileList is array-like but not an actual array. The handler uses Array.from(e.target.files) which could behave differently.
Create a more realistic FileList mock:
-// e.target.files should be array-like; we provide directly as Array with target.files
-const evt = { target: { files } };
+// Create a FileList-like object
+const fileList = Object.assign(files, {
+ item: (i) => files[i],
+ length: files.length
+});
+const evt = { target: { files: fileList } };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const evt = { target: { files } }; | |
| // Create a FileList-like object | |
| const fileList = Object.assign(files, { | |
| item: (i) => files[i], | |
| length: files.length | |
| }); | |
| const evt = { target: { files: fileList } }; |
🤖 Prompt for AI Agents
In Build/tests/build_library.test.js around lines 496-498, the test uses const
evt = { target: { files } } where files is a plain array; replace this with a
realistic FileList mock (an array-like object with numeric indices for each
File, a length property, and an item(index) method) so
Array.from(e.target.files) behaves like the browser FileList; update evt to use
that mock and ensure any referenced files are assigned to numeric keys and
length correctly.
| class FakeIDBRequest { | ||
| constructor() { | ||
| this.onsuccess = null; | ||
| this.onerror = null; | ||
| } | ||
| succeed(result) { | ||
| this.result = result; | ||
| if (typeof this.onsuccess === 'function') this.onsuccess({ target: this }); | ||
| } | ||
| fail(error) { | ||
| this.error = error; | ||
| if (typeof this.onerror === 'function') this.onerror({ target: this }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mock implementation differs from the one in unit tests.
The FakeIDBRequest class here uses synchronous callbacks while IDBRequestMock in the unit tests uses queueMicrotask. This inconsistency could lead to different timing behaviors in tests.
Align the implementations to use consistent async behavior:
class FakeIDBRequest {
constructor() {
this.onsuccess = null;
this.onerror = null;
}
succeed(result) {
this.result = result;
- if (typeof this.onsuccess === 'function') this.onsuccess({ target: this });
+ if (typeof this.onsuccess === 'function') {
+ queueMicrotask(() => this.onsuccess({ target: this }));
+ }
}
fail(error) {
this.error = error;
- if (typeof this.onerror === 'function') this.onerror({ target: this });
+ if (typeof this.onerror === 'function') {
+ queueMicrotask(() => this.onerror({ target: this }));
+ }
}
}📝 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.
| class FakeIDBRequest { | |
| constructor() { | |
| this.onsuccess = null; | |
| this.onerror = null; | |
| } | |
| succeed(result) { | |
| this.result = result; | |
| if (typeof this.onsuccess === 'function') this.onsuccess({ target: this }); | |
| } | |
| fail(error) { | |
| this.error = error; | |
| if (typeof this.onerror === 'function') this.onerror({ target: this }); | |
| } | |
| } | |
| class FakeIDBRequest { | |
| constructor() { | |
| this.onsuccess = null; | |
| this.onerror = null; | |
| } | |
| succeed(result) { | |
| this.result = result; | |
| if (typeof this.onsuccess === 'function') { | |
| queueMicrotask(() => this.onsuccess({ target: this })); | |
| } | |
| } | |
| fail(error) { | |
| this.error = error; | |
| if (typeof this.onerror === 'function') { | |
| queueMicrotask(() => this.onerror({ target: this })); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In Build/tests/helpers/indexeddb-mock.spec.js around lines 16 to 29, the
FakeIDBRequest currently invokes onsuccess/onerror synchronously which differs
from the unit test IDBRequestMock that uses queueMicrotask; change the
succeed(result) and fail(error) methods to set this.result/this.error
immediately and then invoke the corresponding callback inside queueMicrotask so
the callbacks run asynchronously and mirror the timing behavior of the unit
tests.
| it('handles rapid consecutive writes and ensures last-write-wins', async () => { | ||
| const writes = []; | ||
| for (let i = 0; i < 10; i++) { | ||
| writes.push(storeLibraryHandle({ seq: i })); | ||
| } | ||
| await Promise.all(writes); | ||
| const result = await getStoredLibraryHandle(); | ||
| expect(result).toEqual({ seq: 9 }); | ||
| }); |
There was a problem hiding this comment.
Race condition test assumes sequential execution.
The test for "rapid consecutive writes" assumes all writes complete in order, but since they're executed in parallel with Promise.all, there's no guarantee that seq: 9 will be the last one written.
The test should ensure sequential execution to guarantee ordering:
it('handles rapid consecutive writes and ensures last-write-wins', async () => {
- const writes = [];
- for (let i = 0; i < 10; i++) {
- writes.push(storeLibraryHandle({ seq: i }));
- }
- await Promise.all(writes);
+ // Execute writes sequentially to ensure ordering
+ for (let i = 0; i < 10; i++) {
+ await storeLibraryHandle({ seq: i });
+ }
const result = await getStoredLibraryHandle();
expect(result).toEqual({ seq: 9 });
});Or if testing concurrent writes, adjust the assertion:
it('handles concurrent writes (last-write-wins semantics undefined)', async () => {
const writes = [];
for (let i = 0; i < 10; i++) {
writes.push(storeLibraryHandle({ seq: i }));
}
await Promise.all(writes);
const result = await getStoredLibraryHandle();
// Result could be any of the written values
expect(result).toHaveProperty('seq');
expect(result.seq).toBeGreaterThanOrEqual(0);
expect(result.seq).toBeLessThan(10);
});🤖 Prompt for AI Agents
In Build/tests/helpers/indexeddb-mock.spec.js around lines 252 to 260, the
"rapid consecutive writes" test runs writes in parallel with Promise.all so
ordering is not guaranteed; to fix it either (A) make the writes sequential to
assert last-write-wins by awaiting each storeLibraryHandle inside the loop (or
chain them with reduce) so seq:9 is guaranteed to be last, or (B) if you intend
to test concurrent behavior, rename the test to reflect concurrency and relax
the assertion to only check that result has a seq property within 0–9 rather
than asserting it equals 9.
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (10)
registerSW.js (1)
1-1: Generated artifact — prefer configuring registration via VitePWA.Per your note (generated/minified), don’t edit this file. Consider configuring VitePWA to inject the registration (injectRegister: 'auto') so registration is gated to production and update UX can be handled centrally. I’ll propose the change in Build/vite.config.mjs.
workbox-3bd99cbd.js (1)
70-71: Avoid sourcemap 404s in productionEither commit workbox-3bd99cbd.js.map or strip the comment to avoid console 404s.
Apply this diff:
-//# sourceMappingURL=workbox-3bd99cbd.js.map +/* source map intentionally omitted */Build/src/main.js (2)
1-3: Register the Service Worker so PWA/offline actually worksNo SW registration is present. If you’re using vite-plugin-pwa, register via the virtual module; otherwise do a manual register.
vite-plugin-pwa:
import { openBook, prevPage, nextPage, goToPage, toggleToc, closeToc } from "./book"; import { openLibrary, handleLibraryFiles, toggleLibrary } from "./library"; +// PWA registration (vite-plugin-pwa) +import { registerSW } from 'virtual:pwa-register'; +registerSW({ immediate: true });Manual fallback:
if ('serviceWorker' in navigator) { window.addEventListener('load', () => { navigator.serviceWorker.register('/sw.js').catch(console.error); }); }
47-76: Break the circular import: move UI helpers to a ui.js modulebook.js/library.js consume showLoading/hideLoading/showError/hideError from main.js while main.js imports those modules — that ES module cycle can yield undefined exports at runtime. Extract these helpers into Build/src/ui.js and import them in all three modules.
Apply this diff to main.js (remove exports and import instead):
+import { showLoading, hideLoading, showError, hideError } from './ui'; @@ -export function showLoading() { - loadingMessage.classList.add('show'); -} +// moved to ./ui @@ -export function hideLoading() { - loadingMessage.classList.remove('show'); -} +// moved to ./ui @@ -export function showError(message) { - errorText.textContent = message; - errorMessage.classList.add('show'); -} +// moved to ./ui @@ -export function hideError() { - errorMessage.classList.remove('show'); -} +// moved to ./uiNew file Build/src/ui.js:
const loadingMessage = document.getElementById('loading-message'); const errorMessage = document.getElementById('error-message'); const errorText = document.getElementById('error-text'); export function showLoading() { loadingMessage?.classList.add('show'); } export function hideLoading() { loadingMessage?.classList.remove('show'); } export function showError(message) { if (errorText) errorText.textContent = String(message ?? ''); errorMessage?.classList.add('show'); } export function hideError() { errorMessage?.classList.remove('show'); }Update imports in book.js and library.js:
- import { showLoading, hideLoading, showError, hideError } from './main'; + import { showLoading, hideLoading, showError, hideError } from './ui';Run this to locate any remaining cycles/usages:
#!/bin/bash rg -nP --type=js -C1 "from\s+['\"]\./main['\"]" Build/srcsw.js (1)
1-1: Add cache expiration to bound runtime caches (Workbox ExpirationPlugin).Unbounded NetworkFirst/CacheFirst caches can balloon. Add ExpirationPlugin with sensible caps and purge-on-quota-error.
- e.registerRoute(/.*\.(js|css|html)$/,new e.NetworkFirst({cacheName:"app-shell",plugins:[]}),"GET"), + e.registerRoute(/.*\.(js|css|html)$/,new e.NetworkFirst({ + cacheName:"app-shell", + plugins:[new e.ExpirationPlugin({maxEntries:50,maxAgeSeconds:7*24*60*60,purgeOnQuotaError:!0})] + }),"GET"), - e.registerRoute(/.*\.(png|ico|json)$/,new e.CacheFirst({cacheName:"assets",plugins:[]}),"GET") + e.registerRoute(/.*\.(png|ico|json)$/,new e.CacheFirst({ + cacheName:"assets", + plugins:[new e.ExpirationPlugin({maxEntries:150,maxAgeSeconds:30*24*60*60,purgeOnQuotaError:!0})] + }),"GET")assets/index-D7CYcCYQ.css (1)
1-1: Build artifact: apply CSS changes in source, not here.This minified file will be regenerated. Please ensure the z-index fix and any a11y focus styles live in Build/src/style.css.
Run to verify z-index in source:
#!/usr/bin/env bash rg -nC2 -g 'Build/src/style.css' -e '\.toc-container' -e 'z-index'index.html (1)
4-4: Tighten CSP for bundled assets; drop unsafe-inline/eval and broad CDNsAssets are local now; the current policy keeps unnecessary risk. Propose a stricter production CSP.
- <meta http-equiv="Content-Security-Policy" content="default-src 'self' 'unsafe-inline' 'unsafe-eval' data: blob: https://cdnjs.cloudflare.com https://cdn.jsdelivr.net https://code.jquery.com https://unpkg.com https://d3js.org https://threejs.org https://cdn.plot.ly https://stackpath.bootstrapcdn.com https://maps.googleapis.com https://cdn.tailwindcss.com https://ajax.googleapis.com https://kit.fontawesome.com https://cdn.datatables.net https://maxcdn.bootstrapcdn.com https://code.highcharts.com; frame-src 'self'; child-src 'self'; manifest-src 'self'; worker-src 'self'; upgrade-insecure-requests; block-all-mixed-content;"> + <meta http-equiv="Content-Security-Policy" content=" + default-src 'self'; + script-src 'self'; + style-src 'self'; + img-src 'self' data: blob:; + connect-src 'self'; + font-src 'self' data:; + worker-src 'self'; + manifest-src 'self'; + frame-src 'self'; + base-uri 'self'; + form-action 'self'; + upgrade-insecure-requests; + ">Note: Keep a looser dev-time CSP (Vite HMR may require eval) but ship the strict one for production.
Build/src/library.js (1)
20-53: Wrap directory scan with loading indicator and handle denied permission gracefully.Show a spinner during scan; if permission is denied, fall back to file-input and avoid leaving a broken stored handle.
export async function openLibrary() { try { + showLoading(); // Try to retrieve stored library directory handle let dirHandle = await getStoredLibraryHandle(); if (!dirHandle) { // If no stored handle, prompt user if (!('showDirectoryPicker' in window)) { // Fallback: trigger multiple file input flow document.getElementById('library-input')?.click(); return; } dirHandle = await window.showDirectoryPicker(); await storeLibraryHandle(dirHandle); } // Permissions for persisted handles can be lost between sessions if (dirHandle.queryPermission && dirHandle.requestPermission) { const perm = await dirHandle.queryPermission({ mode: 'read' }); if (perm !== 'granted') { const res = await dirHandle.requestPermission({ mode: 'read' }); - if (res !== 'granted') throw new Error('Read permission was denied for the library directory.'); + if (res !== 'granted') { + // Optional: clear stored handle so we don't fail every time + // await clearStoredLibraryHandle(); // implement in indexedDB.js + document.getElementById('library-input')?.click(); + return; + } } } const files = []; for await (const entry of dirHandle.values()) { - if (entry.kind === 'file' && entry.name.endsWith('.epub')) { + if (entry.kind === 'file' && (entry.name?.toLowerCase?.() || '').endsWith('.epub')) { files.push(entry); } } displayLibraryGrid(files); toggleLibrary(true); } catch (err) { showError('Failed to open library: ' + err.message); + } finally { + hideLoading(); } }If you want, I can add clearStoredLibraryHandle() to indexedDB.js.
Build/src/book.js (2)
127-130: Duplicate keyup listener registration causes double navigation.You remove then add the listener twice; keys will fire handlers twice.
window.removeEventListener('keyup', handleKeyEvents); window.addEventListener('keyup', handleKeyEvents); - window.addEventListener('keyup', handleKeyEvents);
101-106: Clean up previous book/rendition and handlers to prevent leaks.Simply nulling references leaks listeners and resources.
async function loadBook(bookData, startLocation) { if (book) { - book = null; - rendition = null; - viewer.innerHTML = ''; + try { window.removeEventListener('keyup', handleKeyEvents); } catch {} + try { await rendition?.destroy?.(); } catch {} + try { await book?.destroy?.(); } catch {} + viewer.innerHTML = ''; + book = null; + rendition = null; }
🧹 Nitpick comments (28)
Build/.gitignore (1)
4-10: Nit: Add pnpm debug logs to ignores.Include pnpm’s debug logs for parity with npm/yarn.
# Logs logs/ *.log npm-debug.log* yarn-debug.log* yarn-error.log* +pnpm-debug.log*Build/package.json (1)
1-12: Mark package private and add basic lint script (ESM optional).Prevents accidental publish, enables linting from scripts; ESM “type” matches .mjs usage.
{ "name": "htmlreader", "version": "2.1.0", "description": "A quick HTML-based ePub Reader that works offline.", - "main": "index.js", + "private": true, + "type": "module", + "main": "index.js", "scripts": { "test": "echo \"Error: no test specified\" && exit 1", + "lint": "eslint .", "dev": "vite", "build": "vite build", "preview": "vite preview", "export": "node export.mjs" }, @@ "author": "NellowTCS", "license": "MIT",Optional: consider Vitest over Jest to align with Vite tooling.
Also applies to: 23-25
Build/eslint.config.mjs (1)
8-14: Ignore build output to reduce lint noise.Add ignores so ESLint skips dist/assets outputs.
-export default defineConfig([ +export default defineConfig([ + { ignores: ["dist/**", "assets/**"] }, js.configs.recommended, { files: ["**/*.{js,mjs,cjs}"], languageOptions: { globals: globals.browser } }, json.configs.recommended, markdown.configs.recommended, css.configs.recommended, ]);Build/vite.config.mjs (1)
9-12: Let VitePWA inject SW registration; enable safe updates and SPA fallback.This removes the need for a generated registerSW.js, gates registration to prod, and improves update behavior.
plugins: [ VitePWA({ + injectRegister: 'auto', registerType: 'autoUpdate', includeAssets: ['robots.txt'], manifest: { @@ - workbox: { + workbox: { + clientsClaim: true, + skipWaiting: true, + navigateFallback: 'index.html', runtimeCaching: [ { urlPattern: /.*\.(js|css|html)$/, handler: 'NetworkFirst', options: { cacheName: 'app-shell' }, }, { urlPattern: /.*\.(png|ico|json)$/, handler: 'CacheFirst', options: { cacheName: 'assets' }, }, ], }, }), ],If you also want to test SW in dev, add:
devOptions: { enabled: true }.Also applies to: 25-39
Build/export.mjs (1)
17-23: Hoist dist checks before destructive deletes.The dist existence/emptiness check runs inside the loop; move it before deleting anything for clarity and to avoid repeated I/O.
-// Clean repo root (skip hidden files/folders) -fs.readdirSync(root).forEach(file => { - if (!fs.existsSync(dist) || fs.readdirSync(dist).length === 0) { - console.error('ERROR: dist is missing or empty:', dist); - process.exit(1); - } - +// Validate dist before destructive operations +if (!fs.existsSync(dist) || fs.readdirSync(dist).length === 0) { + console.error('ERROR: dist is missing or empty:', dist); + process.exit(1); +} + +// Clean repo root (skip hidden files/folders) +fs.readdirSync(root).forEach(file => { if (!keep.has(file) && !file.startsWith('.')) { // <-- skip hidden rimrafSync(path.join(root, file)); console.log('Deleted:', file); } });Build/src/style.css (1)
244-264: Respect reduced motion preferences for slide-in panelsConsider adding a prefers-reduced-motion override to disable/shorten the transform animations for .toc-container and .library-container.
Example to append at end of file:
@media (prefers-reduced-motion: reduce) { .toc-container, .library-container { transition: none; } }Build/tests/unit/style.test.js (1)
156-171: Fixture still encodes the prior duplicate z-index patternThe inlineCssFixture retains two z-index declarations for .toc-container, which we just removed in the real CSS. It doesn’t break the test but it encodes an anti-pattern. Recommend aligning the fixture with the fixed CSS (single z-index: 1010) and keeping the assertion.
Also applies to: 485-496
workbox-3bd99cbd.js (1)
1-1: Silence linters for vendored/minified Workbox bundleBiome flagged “used before declaration” in this file; that’s normal for the upstream build. Exclude this file from linting (preferred) or add a file-level disable header.
Options:
- Add to .biomeignore/.eslintignore:
workbox-*.js- Or prepend:
+/* eslint-disable */ +/* biome-ignore-file: vendored Workbox bundle */ define(["exports"],function(t){"use strict";Build/src/main.js (1)
4-20: Optional: harden against missing DOM nodesIf index.html elements drift, add null checks before addEventListener to avoid TypeErrors at startup.
Also applies to: 23-41
Build/tests/unit/indexedDB.test.js (2)
193-223: Simplify and de-risk request.onsuccess monkey-patchThis override works but is brittle. Keep the original onsuccess reference before nulling, then wrap it. Also remove unused variables.
Apply this diff:
- const originalOpen = global.indexedDB.open; + const originalOpen = global.indexedDB.open; global.indexedDB.open = function(name, version) { const req = originalOpen(name, version); - const origOnSuccessSetter = Object.getOwnPropertyDescriptor(Object.getPrototypeOf(req), "onsuccess")?.set; - // Instead, wrap by overriding after creation; simpler approach: intercept onsuccess event: - const origThen = req.onsuccess; - req.onsuccess = null; + // Capture whatever the module sets as onsuccess, then wrap it. + let originalHandler = req.onsuccess; + req.onsuccess = null; // module will set its own handler shortly queueMicrotask(() => { - // After db creation, replace transaction.objectStore to a faulty one - const oldHandler = req.onsuccess; + // After module assigns onsuccess, wrap it + const oldHandler = req.onsuccess || originalHandler; req.onsuccess = (e) => { const db = e.target.result; const origTx = db.transaction.bind(db); db.transaction = (storeName, mode) => { const tx = origTx(storeName, mode); const origOS = tx.objectStore.bind(tx); tx.objectStore = (name) => { const os = origOS(name); const origPut = os.put.bind(os); os.put = (val) => origPut({ ...val, name: undefined }); // remove keyPath to trigger error return os; }; return tx; }; oldHandler && oldHandler(e); }; }); return req; };
236-269: Mirror the wrapper pattern in the get-error test to reduce flakinessUse the same “capture then wrap” approach here to avoid racing with the module’s handler assignment.
sw.js (1)
1-1: Avoid double-handling precached HTML.Your NetworkFirst route matches HTML (including index.html), which is also precached. To reduce duplicate cache churn, exclude index.html (or drop “html” from this route).
- e.registerRoute(/.*\.(js|css|html)$/, new e.NetworkFirst({...}), "GET") + e.registerRoute(/.*\.(js|css)$/, new e.NetworkFirst({...}), "GET")Build/src/indexedDB.js (1)
47-55: Optional: align get flow with tx completion.For readonly it’s usually fine, but resolving on tx.oncomplete avoids edge timing issues and keeps symmetry with store.
- return new Promise((resolve, reject) => { - const tx = db.transaction("handles", "readonly"); - const store = tx.objectStore("handles"); - const req = store.get("library"); - req.onsuccess = () => resolve(req.result ? req.result.handle : null); - req.onerror = e => reject(e.target.error); - }); + return new Promise((resolve, reject) => { + const tx = db.transaction("handles", "readonly"); + const store = tx.objectStore("handles"); + const req = store.get("library"); + let out = null; + req.onsuccess = () => { out = req.result ? req.result.handle : null; }; + req.onerror = e => reject(e.target.error); + tx.oncomplete = () => resolve(out); + tx.onabort = tx.onerror = e => reject(tx.error || (e && e.target && e.target.error)); + });assets/index-D7CYcCYQ.css (1)
1-1: Add visible focus styles (in source CSS).Improve keyboard accessibility with :focus-visible outlines on interactive elements.
+/* In Build/src/style.css */ +button:focus-visible, +.toc-item:focus-visible, +#close-toc:focus-visible, +#close-library:focus-visible, +#prev-button:focus-visible, +#next-button:focus-visible { + outline: 3px solid #0ea5e9; + outline-offset: 2px; +}Build/tests/helpers/indexeddb-mock.spec.js (1)
65-74: FakeTransaction lacks tx events; code that waits on tx.oncomplete may hang.If the helper under test resolves on tx.oncomplete (like production), emit tx.oncomplete after request microtasks.
class FakeTransaction { constructor(state) { this._state = state; this.mode = null; + this.oncomplete = null; + this.onabort = null; + this.onerror = null; } objectStore(name) { if (name !== 'handles') throw new Error('Unknown store ' + name); - return new FakeObjectStore(this._state); + const tx = this; + const store = new FakeObjectStore(this._state); + return { + put(v) { + const req = store.put(v); + queueMicrotask(() => tx.oncomplete && tx.oncomplete()); + return req; + }, + get(k) { + const req = store.get(k); + queueMicrotask(() => tx.oncomplete && tx.oncomplete()); + return req; + } + }; } }If the helper intentionally resolves on req.onsuccess, ignore this.
Build/index.html (1)
64-73: Optional a11y: make messages announce to AT and buttons descriptive.Add aria-live and explicit labels.
- <div class="message" id="loading-message"> + <div class="message" id="loading-message" role="status" aria-live="polite"> ... - <div class="message" id="error-message"> + <div class="message" id="error-message" role="alert" aria-live="assertive"> ... - <button id="close-error">Close</button> + <button id="close-error" aria-label="Close error message">Close</button>Build/tests/build_indexedDB.test.js (1)
154-166: Global pollution cleanupYou replace globalThis.indexedDB and add __idbMock but never restore originals. Restore in afterAll to avoid leaks across suites.
Object.defineProperty(globalThis, 'indexedDB', { @@ }); Object.defineProperty(globalThis, '__idbMock', { @@ }); } + +afterAll(() => { + delete globalThis.__idbMock; + // Optional: restore indexedDB if it existed + // globalThis.indexedDB = originalIndexedDB; +});Build/tests/unit/library.test.js (1)
13-23: Jest-only mocks reduce portability to VitestProvide a tiny shim so the same tests run under Vitest.
-// Mock sibling modules used by the library (paths will be auto-adjusted by script if needed) -jest.mock('../../src/indexedDB', () => ({ +const mocker = typeof vi !== 'undefined' ? vi : jest; +// Mock sibling modules used by the library (paths will be auto-adjusted by script if needed) +mocker.mock('../../src/indexedDB', () => ({ storeLibraryHandle: jest.fn(), getStoredLibraryHandle: jest.fn(), })); -jest.mock('../../src/book', () => ({ +mocker.mock('../../src/book', () => ({ openBookFromEntry: jest.fn(), })); -jest.mock('../../src/main', () => ({ +mocker.mock('../../src/main', () => ({ showError: jest.fn(), }));index.html (1)
10-14: Minor HTML readability nitSplit tags onto separate lines for diffs and clarity; also add defer to registerSW to avoid parser blocking.
-<link rel="manifest" href="./manifest.webmanifest"><script id="vite-plugin-pwa:register-sw" src="./registerSW.js"></script> +<link rel="manifest" href="./manifest.webmanifest"> +<script id="vite-plugin-pwa:register-sw" src="./registerSW.js" defer></script>Build/tests/unit/main.test.js (1)
72-81: Reduce duplication: create a small import helperThe repeated import-with-fallback code is noisy. Optional, but simplifies the tests.
+const loadMain = async () => { + if (hasUnstableMockModule) { + try { return await import('../../src/main.js'); } catch (_) {} + try { return await import('../../main.js'); } catch (e) { throw e; } + } else { + try { return require('../../src/main.js'); } catch (_) { return require('../../main.js'); } + } +}; @@ - const mod = hasUnstableMockModule - ? await import('../../src/main.js').catch(async () => await import('../..//main.js')).catch(async () => await import('../../main.js')) - : require('../../src/main.js'); + const mod = await loadMain();Apply similarly in other tests.
Build/tests/unit/book.test.js (2)
136-139: Avoid Node-only setImmediate in tests; use portable microtask flush.setImmediate isn’t guaranteed in jsdom/Vitest. Prefer two microtasks to flush promises.
Apply these diffs:
- await Promise.resolve(); - await new Promise(setImmediate); + await Promise.resolve(); + await Promise.resolve();Repeat the same replacement at the other await sites in this file.
Also applies to: 197-198, 228-230, 367-369
313-358: Ensure mocks persist after jest.resetModules in this test.This test reconfigures only epubjs after jest.resetModules; if Jest ever stops hoisting earlier mocks across resets, main and library will become real modules and call real DOM code. To harden, explicitly re-mock main here.
Example:
jest.doMock('epubjs', () => { // ... }); + +jest.doMock('../../src/main', () => ({ + showLoading: jest.fn(), + hideLoading: jest.fn(), + showError: jest.fn(), +}));Build/src/library.js (3)
75-87: Build items concurrently to speed up large libraries.Sequential awaits slow down rendering. Resolve items in parallel, then append.
- for (const entry of fileEntries) { - const item = await createLibraryItem(entry); - libraryContent.appendChild(item); - } + const items = await Promise.all(fileEntries.map(createLibraryItem)); + for (const item of items) libraryContent.appendChild(item);
101-112: Default cover to a lightweight placeholder to avoid broken image icons.Initialize with a tiny inline SVG; replace when a real cover loads.
const img = document.createElement('img'); img.className = 'library-cover'; - img.src = ''; + img.src = 'data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="160" height="240"><rect width="100%" height="100%" fill="%23eee"/><text x="50%" y="50%" dominant-baseline="middle" text-anchor="middle" fill="%23999" font-family="sans-serif" font-size="14">No Cover</text></svg>';
149-160: Shared overlay class between Library and TOC can conflict.Both modules toggle the same overlay element/class; opening one can inadvertently close the other.
Consider separate overlay elements or namespaced classes (e.g., overlay--library vs overlay--toc), or a small overlay manager to reference-count open states.
Build/src/book.js (3)
38-41: Accept case-insensitive .epub and odd file types.Some EPUBs lack the canonical MIME type; check extension case-insensitively.
- if (file.type !== 'application/epub+zip' && !file.name.endsWith('.epub')) { + const name = (file.name?.toLowerCase?.() || ''); + if (file.type !== 'application/epub+zip' && !name.endsWith('.epub')) {
44-56: Flatten loading/hide logic and improve FileReader error message.Use finally to avoid duplication and show a clearer error.
reader.onload = function(e) { const bookData = e.target.result; - loadBook(bookData).then(() => { - hideLoading(); - }).catch(err => { - hideLoading(); - showError('Error loading book: ' + err.message); - }); + loadBook(bookData) + .catch(err => showError('Error loading book: ' + err.message)) + .finally(() => hideLoading()); }; reader.onerror = function(e) { - hideLoading(); - showError('Error reading file: ' + e.target.error); + hideLoading(); + const err = e.target?.error; + showError('Error reading file: ' + (err?.message || String(err))); };
248-261: Overlay coupling: TOC and Library share the same overlay element.Toggling here also affects the Library overlay. Risk of inconsistent UI state.
Consider decoupling overlays or centralizing overlay state in a small controller so feature overlays don’t fight each other.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
Build/package-lock.jsonis excluded by!**/package-lock.jsonBuild/public/source.svgis excluded by!**/*.svgapple-touch-icon-180x180.pngis excluded by!**/*.pngassets/index-D-qNE1dZ.js.mapis excluded by!**/*.mapfavicon.icois excluded by!**/*.icofavicon.pngis excluded by!**/*.pngmaskable-icon-512x512.pngis excluded by!**/*.pngpwa-192x192.pngis excluded by!**/*.pngpwa-512x512.pngis excluded by!**/*.pngpwa-64x64.pngis excluded by!**/*.pngsource.svgis excluded by!**/*.svgsw.js.mapis excluded by!**/*.mapworkbox-3bd99cbd.js.mapis excluded by!**/*.map
📒 Files selected for processing (28)
Build/.gitignore(1 hunks)Build/eslint.config.mjs(1 hunks)Build/export.mjs(1 hunks)Build/index.html(1 hunks)Build/package.json(1 hunks)Build/src/book.js(1 hunks)Build/src/indexedDB.js(1 hunks)Build/src/library.js(1 hunks)Build/src/main.js(1 hunks)Build/src/style.css(1 hunks)Build/tests/build_indexedDB.test.js(1 hunks)Build/tests/build_library.test.js(1 hunks)Build/tests/helpers/indexeddb-mock.spec.js(1 hunks)Build/tests/unit/book.test.js(1 hunks)Build/tests/unit/indexedDB.test.js(1 hunks)Build/tests/unit/library.test.js(1 hunks)Build/tests/unit/main.test.js(1 hunks)Build/tests/unit/style.test.js(1 hunks)Build/vite.config.mjs(1 hunks)LICENSE(1 hunks)assets/index-D7CYcCYQ.css(1 hunks)index.html(2 hunks)manifest.json(0 hunks)manifest.webmanifest(1 hunks)registerSW.js(1 hunks)service-worker.js(0 hunks)sw.js(1 hunks)workbox-3bd99cbd.js(1 hunks)
💤 Files with no reviewable changes (2)
- service-worker.js
- manifest.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-12T02:13:17.615Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: manifest.webmanifest:1-1
Timestamp: 2025-09-12T02:13:17.615Z
Learning: The manifest.webmanifest file in the HTMLReader repository is a generated file and should not be manually edited.
Applied to files:
manifest.webmanifest
📚 Learning: 2025-09-12T02:12:23.004Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: registerSW.js:1-1
Timestamp: 2025-09-12T02:12:23.004Z
Learning: Files identified as minified or generated (like registerSW.js) should not be manually edited, as changes will be overwritten during the build process. Suggestions for such files should target the source files instead.
Applied to files:
manifest.webmanifestregisterSW.js
📚 Learning: 2025-09-12T02:11:18.442Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: Build/export.mjs:13-15
Timestamp: 2025-09-12T02:11:18.442Z
Learning: In HTMLReader project, there is no CNAME file in the root directory - the suggestion to preserve CNAME in Build/export.mjs was incorrect.
Applied to files:
Build/package.jsonBuild/index.htmlBuild/export.mjs
📚 Learning: 2025-09-12T03:51:20.249Z
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLReader#2
File: assets/index-D7CYcCYQ.css:1-1
Timestamp: 2025-09-12T03:51:20.249Z
Learning: When reviewing minified CSS files (like assets/index-D7CYcCYQ.css), accessibility and other suggestions should be applied to the source CSS files instead, as minified files are typically build artifacts that get overwritten.
Applied to files:
assets/index-D7CYcCYQ.css
🧬 Code graph analysis (11)
Build/tests/unit/main.test.js (1)
Build/src/main.js (5)
showLoading(47-49)hideLoading(55-57)showError(65-68)hideError(74-76)fileInput(6-6)
Build/tests/build_library.test.js (2)
Build/src/library.js (4)
handleLibraryFiles(59-63)toggleLibrary(149-160)dirHandle(23-23)img(104-104)Build/tests/unit/library.test.js (1)
mod(45-45)
Build/src/indexedDB.js (1)
Build/tests/unit/indexedDB.test.js (1)
request(98-123)
Build/tests/unit/library.test.js (3)
Build/src/book.js (4)
book(6-6)openBookFromEntry(71-86)file(36-36)file(76-76)Build/src/library.js (1)
dirHandle(23-23)Build/src/main.js (1)
showError(65-68)
Build/tests/unit/style.test.js (2)
Build/src/book.js (5)
file(36-36)file(76-76)viewer(24-24)toc(178-178)overlay(19-19)Build/src/library.js (1)
msg(78-78)
Build/src/main.js (2)
Build/src/book.js (11)
tocButton(15-15)prevButton(16-16)nextButton(17-17)currentPageInput(18-18)overlay(19-19)openBook(35-58)prevPage(200-202)nextPage(209-211)goToPage(224-231)toggleToc(248-251)closeToc(258-261)Build/src/library.js (4)
overlay(9-9)openLibrary(20-53)toggleLibrary(149-160)handleLibraryFiles(59-63)
Build/tests/unit/book.test.js (1)
Build/src/book.js (8)
file(36-36)file(76-76)rendition(7-7)viewer(24-24)toc(178-178)overlay(19-19)tocContent(23-23)tocContainer(22-22)
Build/src/library.js (3)
Build/src/book.js (5)
file(36-36)file(76-76)arrayBuffer(77-77)metadata(132-132)openBookFromEntry(71-86)Build/src/main.js (1)
showError(65-68)Build/src/indexedDB.js (2)
getStoredLibraryHandle(47-56)storeLibraryHandle(27-36)
Build/tests/helpers/indexeddb-mock.spec.js (1)
Build/tests/build_indexedDB.test.js (3)
db(124-124)storeLibraryHandle(168-168)getStoredLibraryHandle(169-169)
workbox-3bd99cbd.js (1)
sw.js (5)
e(1-1)n(1-1)r(1-1)o(1-1)l(1-1)
Build/src/book.js (3)
Build/src/main.js (4)
overlay(15-15)showError(65-68)showLoading(47-49)hideLoading(55-57)Build/src/library.js (4)
overlay(9-9)file(115-117)toggleLibrary(149-160)arrayBuffer(118-118)Build/tests/unit/book.test.js (2)
ePub(34-34)ePub(85-85)
🪛 Biome (2.1.2)
workbox-3bd99cbd.js
[error] 31-31: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 31-31: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (11)
LICENSE (1)
1-22: LGTM — license aligned to MIT.manifest.webmanifest (1)
1-1: Generated file — verify only one manifest is referenced at runtime.Generated by vite-plugin-pwa; keep edits in the build config. After building, confirm the built HTML (e.g., dist/index.html) contains exactly one and that only the VitePWA-generated manifest.webmanifest is present to avoid install/update confusion.
Suggested local checks:
# run after build rg -n --hidden --no-ignore -S "<link[^>]+rel=['\"]manifest['\"]" dist build public || true rg -n --hidden --no-ignore -S '"name"\s*:\s*"HTMLReader"|"short_name"\s*:\s*"HTMLReader"' --glob '**/manifest*.webmanifest' || trueBuild/src/style.css (1)
101-115: TOC z-index fix looks goodThe duplicate z-index was removed and the final stacking level is a single 1010 value. This keeps the TOC above the overlay and other UI reliably.
Build/src/indexedDB.js (1)
27-35: Good: resolve on tx.oncomplete (correct IDB semantics).Build/index.html (2)
4-13: CSP looks sane for PWA + EPUB iframes.Tight and blob/data allowed where needed. Nice.
14-18: Add manifest link & theme-color to Build/index.htmlindex.html already includes and (index.html:10–11); Build/index.html (lines 14–18) lacks them — add those tags to Build/index.html or update the build pipeline to inject them before the stylesheet link.
Build/tests/build_indexedDB.test.js (1)
235-239: Storingundefinedas a handle is a brittle contractIDB entries being explicitly
undefinedis unusual and may differ across implementations. Confirm this is intended; otherwise, prefer normalizing to null and adjusting expectations.Would you like me to update the test to assert
nullfor “no value” and add an explicit test for storing falsy-but-valid values (e.g., 0)?Build/tests/unit/library.test.js (1)
148-160: Assertion coverage looks solidGood check for empty selection messaging and ensuring the library still opens. No changes requested.
Build/tests/build_library.test.js (1)
282-355: Happy-path assertions are solidFiltering to .epub, metadata title, and cover URL checks are good and realistic. No changes requested.
Build/tests/unit/main.test.js (2)
126-138: Undefined message expectation may be browser-dependentSetting textContent to undefined often yields 'undefined' (string) in browsers. Either coerce in code (preferred) or relax the test.
If you want empty text, update main.js:
-export function showError(message) { - errorText.textContent = message; +export function showError(message) { + errorText.textContent = message ?? ''; errorMessage.classList.add('show'); }And keep the test as-is.
160-175: Good event wiring test for open buttonClick → file input click spy is clear and robust. No changes requested.
| request.onupgradeneeded = e => { | ||
| const db = e.target.result; | ||
| db.createObjectStore("handles", { keyPath: "name" }); | ||
| }; | ||
| request.onsuccess = e => resolve(e.target.result); | ||
| request.onerror = e => reject(e.target.error); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard object store creation on upgrade to prevent future version crashes.
Calling createObjectStore unconditionally will throw if you bump DB version later and the store already exists. Check objectStoreNames first.
- request.onupgradeneeded = e => {
- const db = e.target.result;
- db.createObjectStore("handles", { keyPath: "name" });
- };
+ request.onupgradeneeded = e => {
+ const db = e.target.result;
+ if (!db.objectStoreNames.contains("handles")) {
+ db.createObjectStore("handles", { keyPath: "name" });
+ }
+ };📝 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.
| request.onupgradeneeded = e => { | |
| const db = e.target.result; | |
| db.createObjectStore("handles", { keyPath: "name" }); | |
| }; | |
| request.onsuccess = e => resolve(e.target.result); | |
| request.onerror = e => reject(e.target.error); | |
| request.onupgradeneeded = e => { | |
| const db = e.target.result; | |
| if (!db.objectStoreNames.contains("handles")) { | |
| db.createObjectStore("handles", { keyPath: "name" }); | |
| } | |
| }; | |
| request.onsuccess = e => resolve(e.target.result); | |
| request.onerror = e => reject(e.target.error); |
🤖 Prompt for AI Agents
In Build/src/indexedDB.js around lines 13 to 18, the onupgradeneeded handler
unconditionally calls createObjectStore which will throw if the "handles" store
already exists in a later version; wrap the creation with a guard that checks
e.target.result.objectStoreNames.contains("handles") (or similar) and only call
createObjectStore when the store is absent, leaving onsuccess and onerror
behavior unchanged.
| import { storeLibraryHandle, getStoredLibraryHandle } from "./indexedDB"; | ||
| import { openBookFromEntry } from "./book" | ||
| import ePub from "epubjs"; | ||
| import { showError } from "./main"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also import showLoading/hideLoading for long scans.
Large directories can stall the UI; show a spinner while scanning/reading covers.
-import { showError } from "./main";
+import { showError, showLoading, hideLoading } from "./main";📝 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.
| import { showError } from "./main"; | |
| import { showError, showLoading, hideLoading } from "./main"; |
🤖 Prompt for AI Agents
In Build/src/library.js around line 4, the module only imports showError but the
reviewer requested adding showLoading and hideLoading to indicate long scans;
add named imports for showLoading and hideLoading from "./main" and wrap the
long-running scanning/cover-reading logic to call showLoading() before starting
and hideLoading() in all exit paths (success, error, or early return) so the
spinner appears during directory scans and is always dismissed; ensure
hideLoading is called in catch/finally or after promise resolution to avoid
leaving the UI stalled.
| export function handleLibraryFiles(e) { | ||
| const files = Array.from(e.target.files); | ||
| displayLibraryGrid(files); | ||
| toggleLibrary(true); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Filter non-EPUB files from file-input fallback.
Currently all selected files are shown.
-export function handleLibraryFiles(e) {
- const files = Array.from(e.target.files);
+export function handleLibraryFiles(e) {
+ const files = Array.from(e.target.files).filter(f => (f.name?.toLowerCase?.() || '').endsWith('.epub'));
displayLibraryGrid(files);
toggleLibrary(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.
| export function handleLibraryFiles(e) { | |
| const files = Array.from(e.target.files); | |
| displayLibraryGrid(files); | |
| toggleLibrary(true); | |
| } | |
| export function handleLibraryFiles(e) { | |
| const files = Array.from(e.target.files).filter(f => (f.name?.toLowerCase?.() || '').endsWith('.epub')); | |
| displayLibraryGrid(files); | |
| toggleLibrary(true); | |
| } |
🤖 Prompt for AI Agents
In Build/src/library.js around lines 59-63, the handler currently takes all
selected files and displays them; update it to filter the FileList for only
files with an .epub extension (case-insensitive) before calling
displayLibraryGrid and toggleLibrary. Specifically, convert e.target.files to an
array, filter by checking file.name endsWith '.epub' (normalize case), then pass
that filtered array to displayLibraryGrid and proceed to toggleLibrary(true);
ensure no other behavior changes.
| try { | ||
| // If using the File System Access API: | ||
| const file = (typeof fileEntry.getFile === 'function') | ||
| ? await fileEntry.getFile() | ||
| : fileEntry; | ||
| const arrayBuffer = await file.arrayBuffer(); | ||
| const tempBook = ePub(arrayBuffer); | ||
| // Attempt to retrieve cover image URL | ||
| const coverUrl = await tempBook.coverUrl(); | ||
| if (coverUrl) { | ||
| img.src = coverUrl; | ||
| } else { | ||
| // Use a generic placeholder if no cover | ||
| img.src = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAMgAAADICAMAAACahl6sAAAAM1BMVEX///+hoaGcnJzPz8/Nzc3FxcXn5+fQ0NDy8vL29vbw8PDv7+/d3d2+vr6UlJSakGz1AAACNklEQVR4nO3d2ZKDIBAFUa8El//+uvLFT6qkSpknG/JpLve86o3QF8AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD8S/w66a8vEcn8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ5+n/wP2S/3mmugUsAAAAASUVORK5CYII='; | ||
| } | ||
| const metadata = await tempBook.loaded.metadata; | ||
| if (metadata.title) { | ||
| titleDiv.textContent = metadata.title; | ||
| } | ||
| } catch (err) { | ||
| console.error('Error loading cover for', fileEntry.name, err); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Lazy-load epubjs and dispose temp books to reduce bundle size and leaks.
- Use dynamic import to code-split epubjs.
- Call destroy() when done to release resources.
- const arrayBuffer = await file.arrayBuffer();
- const tempBook = ePub(arrayBuffer);
+ const arrayBuffer = await file.arrayBuffer();
+ const { default: ePub } = await import('epubjs');
+ const tempBook = ePub(arrayBuffer);
// Attempt to retrieve cover image URL
const coverUrl = await tempBook.coverUrl();
if (coverUrl) {
img.src = coverUrl;
} else {
// Use a generic placeholder if no cover
img.src = 'data:image/png;base64,iVBORw0K...';
}
const metadata = await tempBook.loaded.metadata;
if (metadata.title) {
titleDiv.textContent = metadata.title;
}
+ await tempBook.destroy?.();📝 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.
| try { | |
| // If using the File System Access API: | |
| const file = (typeof fileEntry.getFile === 'function') | |
| ? await fileEntry.getFile() | |
| : fileEntry; | |
| const arrayBuffer = await file.arrayBuffer(); | |
| const tempBook = ePub(arrayBuffer); | |
| // Attempt to retrieve cover image URL | |
| const coverUrl = await tempBook.coverUrl(); | |
| if (coverUrl) { | |
| img.src = coverUrl; | |
| } else { | |
| // Use a generic placeholder if no cover | |
| img.src = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAMgAAADICAMAAACahl6sAAAAM1BMVEX///+hoaGcnJzPz8/Nzc3FxcXn5+fQ0NDy8vL29vbw8PDv7+/d3d2+vr6UlJSakGz1AAACNklEQVR4nO3d2ZKDIBAFUa8El//+uvLFT6qkSpknG/JpLve86o3QF8AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD8S/w66a8vEcn8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ5+n/wP2S/3mmugUsAAAAASUVORK5CYII='; | |
| } | |
| const metadata = await tempBook.loaded.metadata; | |
| if (metadata.title) { | |
| titleDiv.textContent = metadata.title; | |
| } | |
| } catch (err) { | |
| console.error('Error loading cover for', fileEntry.name, err); | |
| } | |
| try { | |
| // If using the File System Access API: | |
| const file = (typeof fileEntry.getFile === 'function') | |
| ? await fileEntry.getFile() | |
| : fileEntry; | |
| const arrayBuffer = await file.arrayBuffer(); | |
| const { default: ePub } = await import('epubjs'); | |
| const tempBook = ePub(arrayBuffer); | |
| // Attempt to retrieve cover image URL | |
| const coverUrl = await tempBook.coverUrl(); | |
| if (coverUrl) { | |
| img.src = coverUrl; | |
| } else { | |
| // Use a generic placeholder if no cover | |
| img.src = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAMgAAADICAMAAACahl6sAAAAM1BMVEX///+hoaGcnJzPz8/Nzc3FxcXn5+fQ0NDy8vL29vbw8PDv7+/d3d2+vr6UlJSakGz1AAACNklEQVR4nO3d2ZKDIBAFUa8El//+uvLFT6qkSpknG/JpLve86o3QF8AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD8S/w66a8vEcn8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ58n8eHS6HQ5+n/wP2S/3mmugUsAAAAASUVORK5CYII='; | |
| } | |
| const metadata = await tempBook.loaded.metadata; | |
| if (metadata.title) { | |
| titleDiv.textContent = metadata.title; | |
| } | |
| await tempBook.destroy?.(); | |
| } catch (err) { | |
| console.error('Error loading cover for', fileEntry.name, err); | |
| } |
🤖 Prompt for AI Agents
In Build/src/library.js around lines 113 to 134, the code eagerly requires
epubjs and never frees the temporary book which increases bundle size and can
leak memory; change to dynamically import epubjs when needed (e.g. const { ePub
} = await import('epubjs')) so epubjs is code-split, create tempBook from that
ePub, and ensure you always call tempBook.destroy() (or the library's
equivalent) in a finally block after extracting coverUrl and metadata to release
resources; also guard for cases where tempBook may not have been created before
calling destroy().
| closeTocButton.addEventListener('click', toggleToc); | ||
| libraryButton.addEventListener('click', openLibrary); |
There was a problem hiding this comment.
Close button should always close, not toggle
Clicking the TOC close button should call closeToc() to avoid accidentally reopening when state gets out of sync.
Apply this diff:
-closeTocButton.addEventListener('click', toggleToc);
+closeTocButton.addEventListener('click', closeToc);📝 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.
| closeTocButton.addEventListener('click', toggleToc); | |
| libraryButton.addEventListener('click', openLibrary); | |
| closeTocButton.addEventListener('click', closeToc); | |
| libraryButton.addEventListener('click', openLibrary); |
🤖 Prompt for AI Agents
In Build/src/main.js around lines 30-31 the TOC close button is wired to
toggleToc which can reopen the TOC if state is out of sync; change the event
listener to call closeToc() instead (i.e., replace
closeTocButton.addEventListener('click', toggleToc) with
closeTocButton.addEventListener('click', closeToc)) so the button always closes
the TOC, leaving the libraryButton listener as-is.
| class ThrowOnGetDB extends FakeDB { | ||
| createObjectStore() { return new ThrowOnGetStore(this._state); } | ||
| transaction() { return new ThrowOnGetStore(this._state); } | ||
| } |
There was a problem hiding this comment.
Same issue for ThrowOnGetDB.transaction().
class ThrowOnGetDB extends FakeDB {
createObjectStore() { return new ThrowOnGetStore(this._state); }
- transaction() { return new ThrowOnGetStore(this._state); }
+ transaction() {
+ return { objectStore: () => new ThrowOnGetStore(this._state) };
+ }
}📝 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.
| class ThrowOnGetDB extends FakeDB { | |
| createObjectStore() { return new ThrowOnGetStore(this._state); } | |
| transaction() { return new ThrowOnGetStore(this._state); } | |
| } | |
| class ThrowOnGetDB extends FakeDB { | |
| createObjectStore() { return new ThrowOnGetStore(this._state); } | |
| transaction() { | |
| return { objectStore: () => new ThrowOnGetStore(this._state) }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In Build/tests/helpers/indexeddb-mock.spec.js around lines 216 to 219, the
ThrowOnGetDB.transaction() implementation incorrectly returns a ThrowOnGetStore
instance; update the method to return a ThrowOnGetTransaction instance (e.g.,
return new ThrowOnGetTransaction(this._state)) so transaction() provides the
correct mock object for transactions.
Build/tests/unit/library.test.js
Outdated
| const LIB_PATH = '../../src/library'; // Will be auto-adjusted by the script below if needed | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import path lacks extension and no fallback — fragile under ESM
Dynamic import without “.js” can fail. Add extension and fallbacks to keep the test portable.
-const LIB_PATH = '../../src/library'; // Will be auto-adjusted by the script below if needed
+const LIB_PATHS = ['../../src/library.js', '../../library.js', '../src/library.js'];And update the loader:
-async function loadLibraryModule() {
- jest.resetModules(); // ensure module reads the fresh DOM at import time
- setupDOM();
- return await import(LIB_PATH);
-}
+async function loadLibraryModule() {
+ jest.resetModules();
+ setupDOM();
+ let lastErr;
+ for (const p of LIB_PATHS) {
+ try { return await import(p); } catch (e) { lastErr = e; }
+ }
+ throw lastErr || new Error('Could not load library module');
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const LIB_PATH = '../../src/library'; // Will be auto-adjusted by the script below if needed | |
| const LIB_PATHS = ['../../src/library.js', '../../library.js', '../src/library.js']; | |
| async function loadLibraryModule() { | |
| jest.resetModules(); | |
| setupDOM(); | |
| let lastErr; | |
| for (const p of LIB_PATHS) { | |
| try { return await import(p); } catch (e) { lastErr = e; } | |
| } | |
| throw lastErr || new Error('Could not load library module'); | |
| } |
🤖 Prompt for AI Agents
In Build/tests/unit/library.test.js around lines 11-12, the import path uses
'../../src/library' without a file extension which is brittle under ESM; update
imports to use explicit '.js' extensions and add fallback attempts (try
'../../src/library.js' then '../../src/library/index.js' or similar) when
dynamically importing in the test so it works in both dev and built outputs, and
update the test loader logic to try those paths in order and throw a clear error
if none succeed.
Build/tests/unit/main.test.js
Outdated
| const mockBookModule = () => { | ||
| if (hasUnstableMockModule) { | ||
| jest.unstable_mockModule('./book', () => bookMocks); | ||
| } else { | ||
| jest.mock('./book', () => bookMocks); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Mocks target wrong specifiers; won’t intercept main’s ./book import
Mock the resolved paths main.js actually imports (e.g., ../../src/book.js) so Jest/Vitest can intercept correctly.
-const mockBookModule = () => {
- if (hasUnstableMockModule) {
- jest.unstable_mockModule('./book', () => bookMocks);
- } else {
- jest.mock('./book', () => bookMocks);
- }
-};
+const mockBookModule = () => {
+ if (hasUnstableMockModule) {
+ jest.unstable_mockModule('../../src/book.js', () => bookMocks);
+ } else {
+ jest.mock('../../src/book.js', () => bookMocks);
+ }
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mockBookModule = () => { | |
| if (hasUnstableMockModule) { | |
| jest.unstable_mockModule('./book', () => bookMocks); | |
| } else { | |
| jest.mock('./book', () => bookMocks); | |
| } | |
| }; | |
| const mockBookModule = () => { | |
| if (hasUnstableMockModule) { | |
| jest.unstable_mockModule('../../src/book.js', () => bookMocks); | |
| } else { | |
| jest.mock('../../src/book.js', () => bookMocks); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In Build/tests/unit/main.test.js around lines 49-55, the test mocks target
'./book' which does not match the actual specifier emitted in the built main.js,
so the mock is not applied; update the jest.mock / jest.unstable_mockModule
calls to use the same resolved specifier main imports (e.g., '../../src/book.js'
or the exact relative path emitted in the bundle), ensuring the mock path
matches the built file's import string so Jest/Vitest can intercept it
correctly.
Build/tests/unit/main.test.js
Outdated
| const mockLibraryModule = () => { | ||
| if (hasUnstableMockModule) { | ||
| jest.unstable_mockModule('./library', () => libraryMocks); | ||
| } else { | ||
| jest.mock('./library', () => libraryMocks); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same issue for library mock
Point the mock at the actual resolved path.
-const mockLibraryModule = () => {
- if (hasUnstableMockModule) {
- jest.unstable_mockModule('./library', () => libraryMocks);
- } else {
- jest.mock('./library', () => libraryMocks);
- }
-};
+const mockLibraryModule = () => {
+ if (hasUnstableMockModule) {
+ jest.unstable_mockModule('../../src/library.js', () => libraryMocks);
+ } else {
+ jest.mock('../../src/library.js', () => libraryMocks);
+ }
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mockLibraryModule = () => { | |
| if (hasUnstableMockModule) { | |
| jest.unstable_mockModule('./library', () => libraryMocks); | |
| } else { | |
| jest.mock('./library', () => libraryMocks); | |
| } | |
| }; | |
| const mockLibraryModule = () => { | |
| if (hasUnstableMockModule) { | |
| jest.unstable_mockModule('../../src/library.js', () => libraryMocks); | |
| } else { | |
| jest.mock('../../src/library.js', () => libraryMocks); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In Build/tests/unit/main.test.js around lines 57 to 63, the mock calls pass the
relative module string instead of the module's resolved path; update both
branches so they call jest.unstable_mockModule(require.resolve('./library'), ()
=> libraryMocks) and jest.mock(require.resolve('./library'), () => libraryMocks)
respectively (i.e. replace './library' with require.resolve('./library')) so the
mock targets the actual resolved module path.
Build/tests/unit/style.test.js
Outdated
| const roots = ['.', 'src', 'public', 'assets', 'styles', 'style', 'css']; | ||
| const candidates = []; | ||
|
|
||
| function walk(dir, depth = 0) { | ||
| if (depth > 3) return; | ||
| let entries = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tests are always falling back to the inline CSS; include Build/ in scan roots
findCssPath doesn’t search the Build/ tree where the real stylesheet lives, so assertions validate the fixture instead of the actual CSS. Expand roots and (optionally) the depth to make the test meaningful.
Apply this diff:
- const roots = ['.', 'src', 'public', 'assets', 'styles', 'style', 'css'];
+ const roots = ['.', 'Build', 'build', 'dist', 'src', 'public', 'assets', 'styles', 'style', 'css'];
@@
- function walk(dir, depth = 0) {
- if (depth > 3) return;
+ function walk(dir, depth = 0) {
+ if (depth > 5) 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.
| const roots = ['.', 'src', 'public', 'assets', 'styles', 'style', 'css']; | |
| const candidates = []; | |
| function walk(dir, depth = 0) { | |
| if (depth > 3) return; | |
| let entries = []; | |
| const roots = ['.', 'Build', 'build', 'dist', 'src', 'public', 'assets', 'styles', 'style', 'css']; | |
| const candidates = []; | |
| function walk(dir, depth = 0) { | |
| if (depth > 5) return; | |
| let entries = []; |
🤖 Prompt for AI Agents
In Build/tests/unit/style.test.js around lines 19 to 24, the test's walk/search
roots omit the Build/ directory so findCssPath falls back to inline fixture;
update the roots array to include 'Build' (and 'Build/styles' if desired) and
increase the max depth (e.g. from 3 to 4 or 5) so the walker will traverse into
the Build tree and discover the real stylesheet; keep the rest of the walk logic
unchanged.
Fix table of contents bug.
Migrate to Vite.
use npm packages and create vite app for offline functionality
Summary by CodeRabbit