chore(cspell): add cleye to cspell and update .gitignore#255
Conversation
🦋 Changeset detectedLatest commit: 9db42c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughMigrates Git hooks from Husky to Lefthook, adds a new Changes
Sequence DiagramsequenceDiagram
participant CLI as User/CLI
participant Fetcher as Fetcher (ky)
participant Parser as Cheerio
participant Inline as Inline processors
participant ScriptProc as Script handler (oxc-parser)
participant Registry as Import Registry
participant FS as File System
CLI->>Fetcher: fetch(url)
Fetcher-->>Parser: return HTML
Parser->>Inline: run processors (img, link, svgUse)
Inline->>Fetcher: fetch assets (images, css, svgs, scripts)
Parser->>ScriptProc: detect and rewrite imports
ScriptProc->>Registry: register module blobs / import-map entries
Registry-->>Parser: inject import-map <script>
Parser->>FS: write single-file HTML
FS-->>CLI: exit(0)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
More templates
@stephansama/ai-commit-msg
@stephansama/alfred-kaomoji
@stephansama/astro-iconify-svgmap
@stephansama/auto-readme
@stephansama/catppuccin-jsonresume-theme
@stephansama/catppuccin-opml
@stephansama/catppuccin-rss
@stephansama/catppuccin-typedoc
@stephansama/catppuccin-xsl
create-stephansama-example
@stephansama/find-makefile-targets
@stephansama/github-env
@stephansama/multipublish
@stephansama/prettier-plugin-handlebars
@stephansama/remark-asciinema
@stephansama/single-file
@stephansama/svelte-social-share-links
@stephansama/typed-env
@stephansama/typed-events
@stephansama/typed-nocodb-api
@stephansama/typed-templates
@stephansama/types-github-action-env
@stephansama/types-lhci
commit: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new package, @stephansama/single-file, which creates a single HTML file from a website URL by inlining assets like SVGs, images, stylesheets, and scripts. It also replaces Husky with Lefthook for git hooks management and updates various workspace dependencies. Feedback focuses on critical logic errors where return null is used instead of continue in loops, which would prematurely terminate the execution. Additionally, the regex-based script transformation is noted as brittle for namespace imports, and several code cleanups are suggested regarding unused variables and redundant array copies.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.config/lefthook.yml:
- Around line 7-15: The pre-commit Lefthook config runs file-mutating jobs in
parallel causing race conditions; update the pre-commit hook so that the
mutating commands (auto-readme -vg and lint-staged -v) are either run
sequentially or explicitly re-stage their changes by adding stage_fixed: true to
those job entries, while leaving the read-only pnpm --workspace-root run
scripts:lint-examples run as a parallel job; locate the entries named "Update
README" and "Lint staged" referencing auto-readme and lint-staged and apply
either removal of parallelism or add stage_fixed: true to each to ensure mutated
files are re-staged deterministically.
In `@core/single-file/package.json`:
- Around line 21-30: Update the package.json exports to include types conditions
and correct type entrypoints: ensure each export record under "exports" (the "."
entry with "import"/"require") also has a corresponding "types" condition
pointing to the appropriate declaration file (e.g., "./dist/index.d.ts" for ESM
import and "./dist/index.d.cts" or CJS-specific dts for require if generated),
keep the top-level "types": "./dist/index.d.ts" as a fallback, and verify
tsdown.config.ts (exports: true, dts: true) produced matching dist/index.d.ts
and dist/index.d.cts files before committing the change.
In `@core/single-file/README.md`:
- Line 27: Replace the empty "## Usage" section with a concise example showing
both how to run the CLI and how to call the package programmatically: add a
short CLI snippet demonstrating install/run (e.g., npx <package> or node
./bin.js <args>) and a minimal API snippet showing how to import and call the
library (e.g., import { createSingleFile } from '<package>' and call
createSingleFile(...) or the actual exported function names in your codebase),
plus one example of expected output or flags; keep it brief and copy-paste ready
under the "## Usage" heading.
- Line 12: The "Table of contents" heading uses '##### Table of contents' which
skips heading levels and triggers markdownlint MD001; update that heading to an
appropriate level that follows the document structure (for example change '#####
Table of contents' to '## Table of contents' or to the correct H2/H3 level
consistent with the surrounding H1) so heading levels are sequential and MD001
is resolved.
In `@core/single-file/src/index.ts`:
- Around line 56-66: The network fetch via ky.get(argv._.url) lacks
timeout/retry and is unprotected, so any fetch error (404, network blip,
non-HTML redirect) will throw and abort; update the call to use ky.get with
options (e.g., timeout: 30_000 and retry: { limit: 2 }) and wrap the fetch and
subsequent pageResponse.text() logic in a try/catch around the block that
constructs pageResponse and page, using debugWarn (or debugInfo.log) to log the
URL (argv._.url) and the error and then continue/return gracefully instead of
throwing; ensure you check Content-Type only after a successful response and
treat non-HTML responses as a logged warning rather than an uncaught exception
so the CLI can proceed.
- Around line 218-219: The broad regex replacement on scriptSrc is misplaced and
unsafe: remove the global scriptSrc.replace(/\bimport(?!\s*\()/g, "const") call
(which currently sits inside the for loop) and instead perform AST-guided
rewrites using the existing staticImports parse result (e.g., iterate
staticImports[i].start/staticImports[i].end for each script and replace only the
exact import tokens/segments discovered by the parser), doing this before the
outer for-loop processes scripts so string literals, comments, object keys, and
import.meta are not altered; use the staticImports locations to substitute the
import token with "const" or otherwise rewrite only valid static import
declarations.
- Around line 161-170: The inlining of fetched stylesheet (in the "stylesheet"
case) and the inlined SVG fragment can allow raw content containing closing tags
like </style> or </svg> to break out of the container; update the "stylesheet"
branch (the code that currently does $(link).replaceWith(html`<style> ${linkSrc}
</style>`) ) to either: 1) set the content via Cheerio text node so Cheerio
handles escaping (e.g., replaceWith($("<style>").text(linkSrc))) or 2) sanitize
the string by replacing occurrences of "</style" with "<\/style" (and similarly
replace "</svg" with "<\/svg" for the SVG inlining code) before embedding; apply
the same fix to the code that inlines SVG fragments to prevent tag-termination
injection.
- Around line 82-91: The selector construction uses unsanitized user input
`hash` in $$(`symbol#${hash}`), which can break or be hijacked; replace the
ID-style selector with an attribute selector that matches the symbol by its id
using an escaped/quoted value (e.g. select by [id="..."]), and apply the same
change where `hash` is used in the selector later (both occurrences around the
`$$(...)` calls at the sites that assign `symbol` and the second use at Line
~114); ensure you escape or properly quote the `hash` value before interpolation
so characters like `.`, `[`, `:`, or whitespace cannot alter the CSS selector
behavior and then continue to use `symbol.attr("viewBox")`, `symbol.html()`,
`$(svgUse).parent().attr(...)` and `$(svgUse).replaceWith(...)` as before.
- Around line 229-242: The Set created from imported doesn't dedupe because
imported contains distinct {key,value} objects; replace the Set approach with a
key-based dedupe (e.g., build a Map or object keyed by current.key) before
fetching so each unique module key is fetched once and assigned to
importSources; specifically, iterate over imported, skip falsy current.key, and
if the key is not already present in your dedupe map add current.value, then
fetch once using ky.get(new URL(value, argv._.url)).text() and populate
importSources[current.key].
- Around line 79-81: The early "return null" inside the SVG <use> loop aborts
run() instead of skipping that iteration; change the behavior in the loop that
contains "const [_, hash] = svgUse.attribs.href.split('#'); if (!hash) return
null;" to continue the for-loop instead of returning, and do the same fix in the
similar image loop (the one around the "img" handling with the same split/if
pattern). Also make the split-check robust (e.g., verify the split produced a
hash before using it) so you don't dereference undefined when no "#" is present.
- Around line 239-256: The registry creation and fetch logic have two bugs: a
base-URL mismatch when re-fetching imports and unsafe key quoting in the
generated registry. Fix the fetch path by resolving each import specifier
against the original script URL (use new URL(i.value, src).href when populating
importSources instead of resolving against argv._.url/dirname) so fetches target
the same origin/subpath as the script; and when building registryString,
serialize the registry key with JSON.stringify(file) (instead of raw quotes) so
keys are safely escaped; keep using escapeScript for the source body and ensure
registryString uses the JSON-serialized key and the existing Blob URL
expression.
In `@pnpm-workspace.yaml`:
- Around line 7-20: Remove the unused "__versions" YAML block from
pnpm-workspace.yaml; locate the top-level key named "__versions" and delete that
entire sequence (the list entries beneath it) so the file no longer contains the
outdated versions anchor and cannot drift from the inlined catalog values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 009042f9-09ca-4af6-a99d-f4920c5378f5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.config/.cspell.json.config/lefthook.yml.gitignore.husky/commit-msg.husky/pre-commit.husky/prepare-commit-msgREADME.mdcore/single-file/README.mdcore/single-file/cli.mjscore/single-file/package.jsoncore/single-file/src/index.tscore/single-file/tsconfig.jsoncore/single-file/tsdown.config.tscore/single-file/typedoc.jsonpackage.jsonpnpm-workspace.yamltsconfig.base.json
💤 Files with no reviewable changes (4)
- .husky/prepare-commit-msg
- .husky/commit-msg
- README.md
- .husky/pre-commit
| pre-commit: | ||
| parallel: true | ||
| jobs: | ||
| - run: auto-readme -vg | ||
| name: Update README | ||
| - run: lint-staged -v | ||
| name: Lint staged | ||
| - run: pnpm --workspace-root run scripts:lint-examples | ||
| name: Lint examples |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
lefthook parallel jobs stage_fixed file mutation ordering
💡 Result:
In Lefthook, particularly for pre-commit hooks using parallel jobs (via parallel: true at the hook level or within group: parallel: true in the jobs feature added in v1.10.0), combined with stage_fixed: true on multiple jobs/commands that mutate the same files (e.g., linters/formatters like ESLint and Prettier with --fix), there is a risk of file mutation ordering issues. Key behaviors: - Jobs/commands with stage_fixed: true run git add on the relevant files (filtered by glob, exclude, etc., using {staged_files} or custom files) automatically after their execution. This only applies to pre-commit hooks. - When parallel: true, multiple such jobs execute concurrently. Lefthook performs git add concurrently for each job after its command completes. - For overlapping files mutated by multiple jobs: The final file content is determined by whichever job finishes last (the "winner"), as tools typically overwrite files. Lefthook then stages the final versions concurrently, so the last mutation persists in the index. This can lead to unpredictable results or race conditions if jobs interfere (e.g., one formatter overriding another). Official docs show examples with multiple parallel stage_fixed: true jobs on different root/ glob paths (e.g., backend vs. frontend), implying safety when paths don't overlap. For overlapping files, run sequentially (parallel: false or piped: true groups) or use non-overlapping filters to avoid mutations on the same files. To enforce ordering: - Use jobs with named groups: Sequential top-level jobs run in definition order; parallel within groups. - Unnamed jobs append in definition order. Example safe config (non-overlapping): pre-commit: parallel: true jobs: - name: migrate root: backend/ glob: db/migrations/* group: piped: true jobs: - run: bundle install - run: rails db:migrate - run: yarn lint --fix {staged_files} root: frontend/ stage_fixed: true # Safe if no overlap For overlapping mutations, prefer sequential execution or tools that don't conflict.
Citations:
- 1: https://lefthook.dev/configuration/jobs
- 2: Make
parallel: truework with priorities evilmartians/lefthook#846 - 3: https://lefthook.dev/configuration/stage_fixed.html
- 4: https://lefthook.dev/configuration/stage_fixed
- 5: https://lefthook.dev/configuration/group
- 6: http://lefthook.dev/configuration/jobs.html
- 7: What is the behavior when there are multiple `stage_fixed` commands and `parrallel: true` is used? evilmartians/lefthook#779
Parallel pre-commit jobs with overlapping file mutations create race conditions.
Both auto-readme -vg and lint-staged -v modify README and source files. When run in parallel with parallel: true, whichever job finishes last determines the final file state, and concurrent git add operations become unpredictable. Additionally, your current config does not use stage_fixed: true, so Lefthook won't automatically re-stage file changes—mutations are made to disk but remain unstaged.
Either run these jobs sequentially (remove parallel: true or move them to separate sequential groups), or add stage_fixed: true to each file-mutating job to ensure Lefthook re-stages their output. The read-only lint-examples job can remain parallel.
🛠️ Suggested structure
pre-commit:
- parallel: true
jobs:
- run: auto-readme -vg
name: Update README
+ stage_fixed: true
- run: lint-staged -v
name: Lint staged
+ stage_fixed: true
- run: pnpm --workspace-root run scripts:lint-examples
name: Lint examplesAlternatively, keep parallel: true at hook level and use sequential grouping for file-mutating jobs only:
pre-commit:
parallel: true
jobs:
- group: piped
jobs:
- run: auto-readme -vg
name: Update README
stage_fixed: true
- run: lint-staged -v
name: Lint staged
stage_fixed: true
- run: pnpm --workspace-root run scripts:lint-examples
name: Lint examples🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.config/lefthook.yml around lines 7 - 15, The pre-commit Lefthook config
runs file-mutating jobs in parallel causing race conditions; update the
pre-commit hook so that the mutating commands (auto-readme -vg and lint-staged
-v) are either run sequentially or explicitly re-stage their changes by adding
stage_fixed: true to those job entries, while leaving the read-only pnpm
--workspace-root run scripts:lint-examples run as a parallel job; locate the
entries named "Update README" and "Lint staged" referencing auto-readme and
lint-staged and apply either removal of parallelism or add stage_fixed: true to
each to ensure mutated files are re-staged deterministically.
| "exports": { | ||
| ".": { | ||
| "import": "./dist/index.js", | ||
| "require": "./dist/index.cjs" | ||
| }, | ||
| "./package.json": "./package.json" | ||
| }, | ||
| "main": "./dist/index.cjs", | ||
| "module": "./dist/index.js", | ||
| "types": "./dist/index.d.cts", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
package.json exports types condition moduleResolution nodenext best practice
💡 Result:
Best practices for package.json exports with "types" conditions and moduleResolution: "nodenext": Use conditional exports in package.json to specify different entry points for JS runtimes and TypeScript. For NodeNext (modern Node.js + TypeScript alignment), set "moduleResolution": "nodenext" (or "node16") in tsconfig.json. This respects package.json "exports", "type": "module", and requires explicit .js extensions in ESM imports. Key pattern for exports (order matters - most specific first): json { "name": "my-package", "type": "module", "exports": { ".": { "types": "./dist/index.d.ts", // TypeScript entry - MUST precede "default" "default": "./dist/index.js" // Runtime fallback (Node.js uses this) }, "./subpath": { "types": "./dist/subpath.d.ts", "default": "./dist/subpath.js" } } } - "types" condition: TypeScript (with nodenext) checks this first for .d.ts/.d.ts files. Use .d.ts for ESM types, .d.cts for CJS. - "default": Universal fallback for Node.js and other runtimes. - For dual CJS/ESM support: Add "import": "./esm/index.js", "require": "./cjs/index.cjs" before "default". - Node.js conditions (in priority order): import/require > node > default. TypeScript overlays "types" early. - Always include "types" before "default" - if after, TypeScript ignores it. - For subpaths: Use "./subpath": {...} to expose specific files; unlisted paths are blocked (encapsulation). - Fallback: Keep top-level "types": "./dist/index.d.ts" and "main": "./dist/index.js" for older tools/Node. Consumers: Use "module": "nodenext", "moduleResolution": "nodenext" for full Node.js compatibility. Explicit extensions in ESM: import {x} from "./file.js". This ensures type safety, runtime consistency, and works across Node.js, TypeScript, bundlers (with caveats on conditions). Validate with publint or arethetypeswrong.com.
Citations:
- 1: https://hirok.io/posts/package-json-exports
- 2: https://nodejs.org/dist/latest-v19.x/docs/api/packages.html
- 3: https://stackoverflow.com/questions/70296652/how-can-i-use-exports-in-package-json-for-nested-submodules-and-typescript
- 4: http://nodejs.org/docs/latest/api/packages.html
- 5: https://nodejs.org/api/packages.html
- 6: http://typescript.p2hp.com/docs/handbook/esm-node.html
- 7: https://www.typescriptlang.org/docs/handbook/modules/reference.html
- 8: https://typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html
🏁 Script executed:
# Check for tsdown config and build output structure
find . -name "tsdown.config.*" -o -name "dist" -type d | head -20Repository: stephansama/packages
Length of output: 1359
🏁 Script executed:
# Check current build/types configuration files
fd -t f "(tsconfig|build|vite|rollup)" . --max-depth 3Repository: stephansama/packages
Length of output: 1482
🏁 Script executed:
# Look at the actual dist directory if it exists and check what type files are there
if [ -d "core/single-file/dist" ]; then
ls -la core/single-file/dist/ | grep -E "\.(d\.ts|d\.cts|js|cjs|mjs)$"
fiRepository: stephansama/packages
Length of output: 46
🏁 Script executed:
# Check the full package.json structure for context
cat core/single-file/package.json | head -40Repository: stephansama/packages
Length of output: 1108
🏁 Script executed:
# Check tsdown.config.ts to understand type emission setup
cat core/single-file/tsdown.config.tsRepository: stephansama/packages
Length of output: 264
🏁 Script executed:
# Also check if there's a src/index.ts or similar
ls -la core/single-file/src/ 2>/dev/null || ls -la core/single-file/ | grep -E "\.(ts|js|mjs)$" | head -20Repository: stephansama/packages
Length of output: 232
exports block is missing types conditions — breaks type resolution under modern moduleResolution modes.
With moduleResolution: "node16" | "nodenext" | "bundler", TypeScript resolves types from conditional exports and ignores the top-level "types" field when "exports" is present. The current block has only import/require, preventing type resolution. Additionally, pointing to a single .d.cts file serves CJS-flavored declarations to ESM consumers, which is incorrect.
Since tsdown.config.ts has exports: true and dts: true configured, tsdown should auto-generate proper dual .d.ts/.d.cts exports. Verify that the generated dist/ files match your build output, then update package.json to include types conditions in each export variant:
Expected structure
"exports": {
".": {
- "import": "./dist/index.js",
- "require": "./dist/index.cjs"
+ "import": {
+ "types": "./dist/index.d.ts",
+ "default": "./dist/index.js"
+ },
+ "require": {
+ "types": "./dist/index.d.cts",
+ "default": "./dist/index.cjs"
+ }
},Keep top-level "types": "./dist/index.d.ts" for fallback compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/single-file/package.json` around lines 21 - 30, Update the package.json
exports to include types conditions and correct type entrypoints: ensure each
export record under "exports" (the "." entry with "import"/"require") also has a
corresponding "types" condition pointing to the appropriate declaration file
(e.g., "./dist/index.d.ts" for ESM import and "./dist/index.d.cts" or
CJS-specific dts for require if generated), keep the top-level "types":
"./dist/index.d.ts" as a fallback, and verify tsdown.config.ts (exports: true,
dts: true) produced matching dist/index.d.ts and dist/index.d.cts files before
committing the change.
| const pageResponse = await ky.get(argv._.url); | ||
|
|
||
| debugInfo.log(`loading ${argv._.url}`); | ||
|
|
||
| if (!pageResponse?.headers?.get("Content-Type")?.includes("text/html")) { | ||
| throw new Error( | ||
| `requested url \`${argv._.url}\` must be an html page.`, | ||
| ); | ||
| } | ||
|
|
||
| const page = await pageResponse.text(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
No timeout / error handling on network fetches — one 404 aborts the whole run.
ky.get has no timeout override here and every failure (redirects to non-HTML, 404 on a referenced asset, network blip) will throw and crash the CLI mid-pipeline. For a tool that walks an entire page's asset graph, consider:
- Configuring a reasonable timeout + retry policy per call (
ky.get(url, { timeout: 30_000, retry: { limit: 2 } })). - Wrapping per-asset loops in try/catch so a single broken asset logs a warning (
debugWarn) and continues, rather than aborting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/single-file/src/index.ts` around lines 56 - 66, The network fetch via
ky.get(argv._.url) lacks timeout/retry and is unprotected, so any fetch error
(404, network blip, non-HTML redirect) will throw and abort; update the call to
use ky.get with options (e.g., timeout: 30_000 and retry: { limit: 2 }) and wrap
the fetch and subsequent pageResponse.text() logic in a try/catch around the
block that constructs pageResponse and page, using debugWarn (or debugInfo.log)
to log the URL (argv._.url) and the error and then continue/return gracefully
instead of throwing; ensure you check Content-Type only after a successful
response and treat non-HTML responses as a logged warning rather than an
uncaught exception so the CLI can proceed.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (8)
core/single-file/src/index.ts (5)
62-75:⚠️ Potential issue | 🟠 Major
dynamicImportscomputed but never merged intoimports.Line 68 initializes
imports = [...staticImports], so anyimport("./foo.js")in fetched scripts is never added to the registry and will fall back to a network request at runtime — defeating the "single file" goal.🛠️ Fix
- const imports = [...staticImports]; + const imports = [...staticImports, ...dynamicImports];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/single-file/src/index.ts` around lines 62 - 75, The code computes dynamicImports from result.module.dynamicImports but never merges them into imports, so dynamic imports aren't registered; update the assembly logic to include dynamicImports when building imports (e.g., combine staticImports and dynamicImports before mapping to allImports), ensuring the mapping that creates allImports (using key: i.value and value: path.join(dirname, i.value)) includes entries derived from dynamicImports, and then push those into imported as currently done for allImports.
119-132:⚠️ Potential issue | 🟡 MinorRegistry keys are interpolated with bare
"quotes — breaks on specifiers containing".
escapeScripthandles the backtick-wrapped body but the key is injected as'"' + file + '"'. UseJSON.stringify(file)for the key.🛠️ Fix
- const registryString = Object.entries(importSources) - .map(([file, source]) => { - const escaped = utilities.escapeScript(source); - return ( - '"' + - file + - '": URL.createObjectURL(new Blob([`' + - escaped + - "`], {type: 'text/javascript'}))" - ); - }) + const registryString = Object.entries(importSources) + .map(([file, source]) => + `${JSON.stringify(file)}: URL.createObjectURL(new Blob([\`${utilities.escapeScript(source)}\`], {type: 'text/javascript'}))`, + ) .join(",\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/single-file/src/index.ts` around lines 119 - 132, The registry keys in registryString are inserted using plain quotes which breaks when a specifier contains a double-quote; instead use a safe JSON-quoted key: when building registryString (which iterates importSources and uses utilities.escapeScript to escape the module body), replace the manual '"'+file+'"' interpolation with JSON.stringify(file) so keys are properly escaped; keep the rest of the returned string (the Blob/URL.createObjectURL part) unchanged and ensure registryScript still composes from registryString.
35-45:⚠️ Potential issue | 🟡 MinorNo timeout / content-type / error handling on the top-level page fetch.
A network blip, non-HTML redirect, or 404 throws out of
run()with an unhandled rejection. Wrap in try/catch, configure{ timeout, retry }, and surface a clean CLI error instead of a stack trace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/single-file/src/index.ts` around lines 35 - 45, The top-level ky.get call and subsequent content-type check inside run() can throw unhandled rejections; wrap the entire fetch + header check + pageResponse.text() block in a try/catch, call ky.get with explicit options (e.g., { timeout: <ms>, retry: <count> }) to avoid hangs, check response.ok and ensure headers.get("Content-Type") includes "text/html" before calling pageResponse.text(), and on any error throw or log a clean CLI-friendly Error (not raw stack) so run() surfaces a readable message instead of crashing; update references to ky.get, pageResponse, page, and the surrounding run() block accordingly.
106-117:⚠️ Potential issue | 🔴 Critical
new Set(imported)does not dedupe freshly-allocated objects.
importedis built from{ key, value }literals (Line 73), soSetretains every occurrence. Same modules are re-fetched N times andwindow.registryends up with duplicate keys (later silently overwrites earlier). Also,valueis resolved against the script's URL's pathname viapath.join(Line 73) but re-fetched againstargv._.url(Line 115), so scripts served from a different origin/CDN are fetched from the wrong host.🛠️ Dedupe by key + fix base URL
- const setImports = new Set(imported); - const importSources: Record<string, string> = {}; - - log.info(`found imports ${JSON.stringify([...setImports], undefined, 2)}`); - - for (const current of setImports) { - if (!current.key) continue; - - importSources[current.key] = await ky - .get(new URL(current.value, argv._.url)) - .text(); - } + const deduped = new Map(imported.map((i) => [i.key, i])); + const importSources: Record<string, string> = {}; + + log.info(`found imports ${JSON.stringify([...deduped.values()], undefined, 2)}`); + + for (const current of deduped.values()) { + if (!current.key) continue; + importSources[current.key] = await ky.get(current.value).text(); + }And upstream (Line 73), build
valueas an absolute URL vianew URL(i.value, src).hrefso the fetch uses the script's origin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/single-file/src/index.ts` around lines 106 - 117, The code creates setImports from imported objects which doesn't dedupe by key and also fetches using argv._.url as base causing wrong origin; instead, dedupe by key (e.g., build a Map from imported items keyed by item.key or iterate imported and skip duplicates) and fetch each unique module once, storing results in importSources[moduleKey]; when resolving the fetch URL use the module's absolute URL (the value should be created earlier as new URL(i.value, src).href) or call new URL(current.value, currentBase) where currentBase is the script's src, not argv._.url, then use ky.get(resolvedUrl).text() to populate importSources.
77-98:⚠️ Potential issue | 🔴 CriticalRegex-based import rewriting remains unsafe on multiple axes.
new RegExp(\import\s*['"]${escaped}['"]`, "g")and thefrom` variant are built from variable input (ast-grep flag) and also miss namespace/default imports, multi-line imports, and mixed quotes across lines.- Line 98
\bimport(?!\s*\()rewrites the wordimportinside strings, comments, object keys, and — critically —import.meta(lookahead only guards(), so any script usingimport.meta.urlbecomesconst.meta.urland crashes at runtime. The "Move this OUTSIDE and BEFORE" TODO also indicates misplaced code.Use the oxc AST node ranges you already have (
staticImports[i].start/end) to rewrite only actual import declarations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/single-file/src/index.ts` around lines 77 - 98, Stop using regex to rewrite imports and the global word-replace that breaks import.meta; instead use the AST ranges you already have (staticImports[i].start / staticImports[i].end) to replace only real import declarations. For each entry in staticImports (or map imports[] -> staticImports by value), build the replacement text (e.g., `await import(window.registry["${current.value}"])` or the appropriate RHS for `from` imports) and splice it into scriptSrc by slicing and concatenation at the exact start/end offsets; perform replacements in descending order of start index to keep offsets valid. Remove the importRegex/fromRegex replacers and delete the final `scriptSrc.replace(/\bimport(?!\s*\()/g, "const")` call so strings/comments/import.meta are untouched. Ensure you keep any surrounding punctuation/assignment semantics when replacing `from`-style imports by using the exact node range from staticImports to determine whether to replace the whole declaration or just the module specifier.core/single-file/package.json (1)
21-30:⚠️ Potential issue | 🟡 Minor
exportsmissingtypesconditions;typesfield points to.d.ctsfor an ESM-first package.Under
moduleResolution: node16|nodenext|bundler, TS resolves types via conditional exports and ignores the top-leveltypeswhenexportsis present. Also, pointing the top-leveltypesat./dist/index.d.ctsserves CJS-flavored declarations to ESM consumers.🛠️ Fix
"exports": { ".": { - "import": "./dist/index.js", - "require": "./dist/index.cjs" + "import": { "types": "./dist/index.d.ts", "default": "./dist/index.js" }, + "require": { "types": "./dist/index.d.cts", "default": "./dist/index.cjs" } }, "./package.json": "./package.json" }, "main": "./dist/index.cjs", "module": "./dist/index.js", - "types": "./dist/index.d.cts", + "types": "./dist/index.d.ts",Verify tsdown emits both
index.d.tsandindex.d.cts(it does whendts: truewith dualformat).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/single-file/package.json` around lines 21 - 30, The package.json exports block lacks a "types" conditional and the top-level "types" field points to index.d.cts which serves CJS declarations to ESM consumers; update the exports map to include a types condition (e.g. add an entry under "exports" for "." with "types": "./dist/index.d.ts" for ESM and/or a conditional mapping that serves "./dist/index.d.cts" for require/CJS), and ensure the top-level "types" either points to the ESM declaration ("./dist/index.d.ts") or is removed in favor of the conditional export; also verify your build (tsdown) emits both index.d.ts and index.d.cts when using dual formats so the referenced files exist.core/single-file/src/inline.ts (2)
34-50:⚠️ Potential issue | 🟡 MinorCSS selector injection via unsanitized
hash.
hashis parsed from page-suppliedimg.srcand interpolated directly intosymbol#${hash}. A hash containing.,[,:, or whitespace will either throw from cheerio's selector engine or match the wrong node. Same issue at Line 129.🛠️ Fix
-const symbol = $$(`symbol#${hash}`); +const symbol = $$("symbol").filter((_, el) => $$(el).attr("id") === hash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/single-file/src/inline.ts` around lines 34 - 50, The code builds a Cheerio selector using an unsanitized page-supplied value `hash` in the expression `$$('symbol#${hash}')`, which can throw or match incorrectly for hashes containing CSS-special characters; fix by not using string selector interpolation — locate the places that call `$$('symbol#${hash}')` (the `symbol` lookup near the `img` handling and the second occurrence later) and replace them with a safe alternative such as selecting by attribute or using Cheerio’s filter/`find` with a function: find symbols via `$$('symbol').filter(...)` or `$$('[id]').filter(el => el.attribs.id === hash)` so you match the element by exact id value without interpreting `hash` as a CSS selector, then proceed to read `attr("viewBox")`/`html()` and set `img.src` as before.
92-99:⚠️ Potential issue | 🟠 MajorStylesheet content can break out of
<style>tag.Embedding
linkSrcverbatim via adedented template literal means any stylesheet containing</style>(inside a CSS string literal or comment) closes the tag and spills into the document. Additionally,dedentstrips leading whitespace from CSS, which is harmless semantically but alters source attributions/source maps.Safer approach: let cheerio serialize the text node so it escapes properly.
🛠️ Fix
- case "stylesheet": { - $(link).replaceWith( - html`<style> - ${linkSrc} - </style>`, - ); - break; - } + case "stylesheet": { + $(link).replaceWith($("<style>").text(linkSrc)); + break; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/single-file/src/inline.ts` around lines 92 - 99, The stylesheet branch currently injects raw CSS via the html`<style>${linkSrc}</style>` template (and uses dedent), which allows a stylesheet containing "</style>" to break out and also mutates whitespace; instead, replace that insertion with a proper Cheerio text node so the CSS is serialized/escaped rather than injected raw: in the case "stylesheet" handler, stop using html`...`/dedent and create a <style> element via Cheerio (reference the same link variable and linkSrc) and set its text content with Cheerio's .text(...) or equivalent API so the CSS is escaped and whitespace is preserved as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/single-file/package.json`:
- Around line 42-54: The package.json currently lists "cleye": "catalog:cli"
under devDependencies but src/index.ts imports { cli } from "cleye" at runtime;
move the "cleye": "catalog:cli" entry from devDependencies into dependencies in
package.json (remove the devDependencies entry and add the same "catalog:cli"
spec under dependencies) so the published package includes cleye for consumers,
then update the lockfile/install artifacts accordingly.
In `@core/single-file/src/import-map.ts`:
- Around line 19-34: The cache key currently uses the raw file arg causing
collisions when the same specifier is resolved from different directories; in
loadImport() use the resolvedFile (the joined path) as the key into the imports
Map instead of file, and when setting the cache populate the stored object's
path (or path?: string) with resolvedFile; update the initial imports.get lookup
to use the same resolvedFile variable so lookups and sets are consistent and
avoid cross-dir collisions.
- Around line 24-25: In loadImport, wrap the ky.get(resolvedFile) call in a
try/catch, call ky.get(resolvedFile, { timeout: 30_000, retry: { limit: 2 } }),
and in the try-block inspect the response Content-Type (or resolvedFile
extension) to decide whether to call response.arrayBuffer() for binary assets
(images, fonts, icons) or response.text() for CSS/JS; feed the resulting
ArrayBuffer directly to downstream consumers (avoid passing text into str2ab for
binaries) and for text paths continue using response.text() and str2ab as
before; in the catch block surface/return the error (or a clear failure value)
so callers of loadImport can decide to skip or fail the build.
In `@core/single-file/src/inline.ts`:
- Around line 104-108: The exported InlineFunction named "script" is an empty
no-op (for loop with only a comment) and also accepts an unused parameter
"baseUrl"; either remove the export until you implement script inlining or make
the stub explicit by adding a TODO with a tracking issue ID and mark "baseUrl"
as intentionally unused (e.g., rename or document) so consumers don't await a
dead transformer; update the export accordingly in inline.ts and ensure index.ts
won't call it if removed.
- Around line 129-138: The code currently throws an Error when symbol.html() is
falsy, which aborts the whole run; instead match the surrounding pattern by
logging a warning and continuing: in the block using symbol =
$$(`symbol#${hash}`) and inner = symbol.html(), replace the throw with a call to
log.warn (or use the existing log/warn function used earlier) that includes the
hash and context (e.g., "missing symbol for hash ...; skipping") and then
continue the loop; also make the analogous change in the other SVG-fragment
branch where symbol.html() can be empty (the img/SVG branch referenced at Line
34) so it logs a warning and skips or inserts a safe fallback rather than
producing an empty data:image/svg+xml URI.
In `@core/single-file/src/log.ts`:
- Around line 17-24: The current mapping creates a base debugger then rebinds
debug.extend to preserve color which is overly complex; instead, for each entry
in DEBUG_NAMESPACES (used to produce error, info, warn) directly create a
debugger with obug.createDebug using a composed namespace (combine
DEBUG_BASE_NAMESPACE and the namespace string) and merged options
({...commonDebugOptions, color: index+1}), removing the debug.extend.bind usage
and the intermediate currentDebugger variable so the array directly contains the
three named debuggers.
In `@core/single-file/src/utilities.ts`:
- Around line 45-51: str2ab reconstructs bytes from a UTF-8 decoded string and
corrupts non-ASCII bytes; instead, update the code that fetches binary assets
(in import-map.ts) to call response.arrayBuffer() and pass that ArrayBuffer
directly to bufferToDataUri (or remove/stop using str2ab entirely); ensure
places that currently call str2ab(str) switch to using the fetched ArrayBuffer
so binary assets (images/fonts/SVG) are preserved exactly.
- Around line 20-33: The isProbablyUrl function currently blacklists strings
containing "function" or "{" which causes false negatives; replace the blacklist
approach with a positive validation: keep the newline guard, remove
str.includes("{") and str.includes("function"), and instead attempt to parse the
input using new URL(str, base) (use a safe base like "http://example" for
relative paths) and accept only when the resulting URL has protocol http(s) or
when the input is a valid root-relative/relative path pattern; ensure the
returned boolean covers absolute https?:\/\/, root-relative ("/..."), and
relative ("./", "../", or simple filenames) cases so callers in isProbablyUrl
(used by inline handlers such as img, link, svgUse and script[src]) won't skip
valid asset URLs.
---
Duplicate comments:
In `@core/single-file/package.json`:
- Around line 21-30: The package.json exports block lacks a "types" conditional
and the top-level "types" field points to index.d.cts which serves CJS
declarations to ESM consumers; update the exports map to include a types
condition (e.g. add an entry under "exports" for "." with "types":
"./dist/index.d.ts" for ESM and/or a conditional mapping that serves
"./dist/index.d.cts" for require/CJS), and ensure the top-level "types" either
points to the ESM declaration ("./dist/index.d.ts") or is removed in favor of
the conditional export; also verify your build (tsdown) emits both index.d.ts
and index.d.cts when using dual formats so the referenced files exist.
In `@core/single-file/src/index.ts`:
- Around line 62-75: The code computes dynamicImports from
result.module.dynamicImports but never merges them into imports, so dynamic
imports aren't registered; update the assembly logic to include dynamicImports
when building imports (e.g., combine staticImports and dynamicImports before
mapping to allImports), ensuring the mapping that creates allImports (using key:
i.value and value: path.join(dirname, i.value)) includes entries derived from
dynamicImports, and then push those into imported as currently done for
allImports.
- Around line 119-132: The registry keys in registryString are inserted using
plain quotes which breaks when a specifier contains a double-quote; instead use
a safe JSON-quoted key: when building registryString (which iterates
importSources and uses utilities.escapeScript to escape the module body),
replace the manual '"'+file+'"' interpolation with JSON.stringify(file) so keys
are properly escaped; keep the rest of the returned string (the
Blob/URL.createObjectURL part) unchanged and ensure registryScript still
composes from registryString.
- Around line 35-45: The top-level ky.get call and subsequent content-type check
inside run() can throw unhandled rejections; wrap the entire fetch + header
check + pageResponse.text() block in a try/catch, call ky.get with explicit
options (e.g., { timeout: <ms>, retry: <count> }) to avoid hangs, check
response.ok and ensure headers.get("Content-Type") includes "text/html" before
calling pageResponse.text(), and on any error throw or log a clean CLI-friendly
Error (not raw stack) so run() surfaces a readable message instead of crashing;
update references to ky.get, pageResponse, page, and the surrounding run() block
accordingly.
- Around line 106-117: The code creates setImports from imported objects which
doesn't dedupe by key and also fetches using argv._.url as base causing wrong
origin; instead, dedupe by key (e.g., build a Map from imported items keyed by
item.key or iterate imported and skip duplicates) and fetch each unique module
once, storing results in importSources[moduleKey]; when resolving the fetch URL
use the module's absolute URL (the value should be created earlier as new
URL(i.value, src).href) or call new URL(current.value, currentBase) where
currentBase is the script's src, not argv._.url, then use
ky.get(resolvedUrl).text() to populate importSources.
- Around line 77-98: Stop using regex to rewrite imports and the global
word-replace that breaks import.meta; instead use the AST ranges you already
have (staticImports[i].start / staticImports[i].end) to replace only real import
declarations. For each entry in staticImports (or map imports[] -> staticImports
by value), build the replacement text (e.g., `await
import(window.registry["${current.value}"])` or the appropriate RHS for `from`
imports) and splice it into scriptSrc by slicing and concatenation at the exact
start/end offsets; perform replacements in descending order of start index to
keep offsets valid. Remove the importRegex/fromRegex replacers and delete the
final `scriptSrc.replace(/\bimport(?!\s*\()/g, "const")` call so
strings/comments/import.meta are untouched. Ensure you keep any surrounding
punctuation/assignment semantics when replacing `from`-style imports by using
the exact node range from staticImports to determine whether to replace the
whole declaration or just the module specifier.
In `@core/single-file/src/inline.ts`:
- Around line 34-50: The code builds a Cheerio selector using an unsanitized
page-supplied value `hash` in the expression `$$('symbol#${hash}')`, which can
throw or match incorrectly for hashes containing CSS-special characters; fix by
not using string selector interpolation — locate the places that call
`$$('symbol#${hash}')` (the `symbol` lookup near the `img` handling and the
second occurrence later) and replace them with a safe alternative such as
selecting by attribute or using Cheerio’s filter/`find` with a function: find
symbols via `$$('symbol').filter(...)` or `$$('[id]').filter(el => el.attribs.id
=== hash)` so you match the element by exact id value without interpreting
`hash` as a CSS selector, then proceed to read `attr("viewBox")`/`html()` and
set `img.src` as before.
- Around line 92-99: The stylesheet branch currently injects raw CSS via the
html`<style>${linkSrc}</style>` template (and uses dedent), which allows a
stylesheet containing "</style>" to break out and also mutates whitespace;
instead, replace that insertion with a proper Cheerio text node so the CSS is
serialized/escaped rather than injected raw: in the case "stylesheet" handler,
stop using html`...`/dedent and create a <style> element via Cheerio (reference
the same link variable and linkSrc) and set its text content with Cheerio's
.text(...) or equivalent API so the CSS is escaped and whitespace is preserved
as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2bbe834-6d2e-4c96-beb1-b40016d680bf
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
core/single-file/package.jsoncore/single-file/src/import-map.tscore/single-file/src/index.tscore/single-file/src/inline.tscore/single-file/src/log.tscore/single-file/src/utilities.tspnpm-workspace.yaml
| "dependencies": { | ||
| "cheerio": "catalog:", | ||
| "dedent": "catalog:", | ||
| "he": "catalog:", | ||
| "ky": "catalog:", | ||
| "obug": "catalog:cli", | ||
| "oxc-parser": "catalog:" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/he": "catalog:", | ||
| "cleye": "catalog:cli", | ||
| "tsdown": "catalog:" | ||
| }, |
There was a problem hiding this comment.
cleye is under devDependencies but imported at runtime by src/index.ts.
core/single-file/src/index.ts does import { cli } from "cleye" which ends up in the published dist/index.js. Consumers installing @stephansama/single-file won't have cleye hoisted and the CLI will fail at runtime with Cannot find module 'cleye'.
Move cleye to dependencies (keep the catalog:cli reference).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/single-file/package.json` around lines 42 - 54, The package.json
currently lists "cleye": "catalog:cli" under devDependencies but src/index.ts
imports { cli } from "cleye" at runtime; move the "cleye": "catalog:cli" entry
from devDependencies into dependencies in package.json (remove the
devDependencies entry and add the same "catalog:cli" spec under dependencies) so
the published package includes cleye for consumers, then update the
lockfile/install artifacts accordingly.
| const response = await ky.get(resolvedFile); | ||
| const data = await response.text(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does ky v2.0.1 support response.arrayBuffer() and the retry/timeout options on ky.get?
💡 Result:
Yes, ky v2.0.1 supports response.arrayBuffer and the retry/timeout options on ky.get.
Citations:
- 1: https://github.com/sindresorhus/ky/releases
- 2: https://github.com/sindresorhus/ky/releases/tag/v2.0.0
- 3: https://github.com/sindresorhus/ky
- 4: https://www.npmjs.com/package/ky
- 5: https://github.com/sindresorhus/ky
- 6: https://www.pregnancycentre.org
- 7: https://tessl.io/registry/tessl/npm-ky/1.10.0/files/docs/responses.md
- 8: https://github.com/sindresorhus/ky/blob/main/source/types/ResponsePromise.ts
🏁 Script executed:
# Find package files to check ky version
find . -name "package*.json" -type f | head -20Repository: stephansama/packages
Length of output: 784
🏁 Script executed:
# Check the full import-map.ts file
find . -path "*/core/single-file/src/import-map.ts" -type fRepository: stephansama/packages
Length of output: 100
🏁 Script executed:
# Find str2ab function to understand UTF-8 handling
rg "str2ab" -A 5 -B 2Repository: stephansama/packages
Length of output: 1946
🏁 Script executed:
# Check if there's error handling at a higher level around the import-map usage
rg "import-map" -t ts -A 3 -B 3Repository: stephansama/packages
Length of output: 475
🏁 Script executed:
# Read the full import-map.ts file
cat -n ./core/single-file/src/import-map.tsRepository: stephansama/packages
Length of output: 1070
🏁 Script executed:
# Check ky version in single-file package.json
cat ./core/single-file/package.json | grep -A 5 -B 5 '"ky"'Repository: stephansama/packages
Length of output: 298
🏁 Script executed:
# Check if there's error handling in the calling code
rg "importMap\." -A 10 -B 5 core/single-file/src/Repository: stephansama/packages
Length of output: 4602
🏁 Script executed:
# Find the catalog version definition (root pnpm-workspace or similar)
find . -name "pnpm-workspace.yaml" -o -name ".pnpmfile.cjs" -o -name "pnpm-lock.yaml" | head -5Repository: stephansama/packages
Length of output: 102
🏁 Script executed:
# Check root package.json for catalog definition
cat ./package.json | grep -A 30 '"catalog"'Repository: stephansama/packages
Length of output: 46
🏁 Script executed:
# Alternative: check if there's a catalog defined in any package
rg '"catalog"' --type json -B 2 -A 2 | head -50Repository: stephansama/packages
Length of output: 46
🏁 Script executed:
# Check where loadImport is called to see if error handling is present at call sites
rg "loadImport" -B 3 -A 10 core/single-file/src/Repository: stephansama/packages
Length of output: 3451
🏁 Script executed:
# Check pnpm-workspace.yaml for catalog definitions
cat ./pnpm-workspace.yamlRepository: stephansama/packages
Length of output: 3748
🏁 Script executed:
# Extract ky version from pnpm-lock.yaml
rg '"ky"' ./pnpm-lock.yaml -A 2 | head -20Repository: stephansama/packages
Length of output: 46
🏁 Script executed:
# Check if inline.ts has try-catch blocks around loadImport calls
rg "try|catch" core/single-file/src/inline.ts -B 5 -A 5Repository: stephansama/packages
Length of output: 46
🏁 Script executed:
# Read inline.ts to check the full context of loadImport calls
head -200 ./core/single-file/src/inline.ts | tail -100Repository: stephansama/packages
Length of output: 2428
Add timeout/retry options and error handling to ky.get(), and use response.arrayBuffer() for binary assets.
The loadImport() function has three critical issues:
-
No resilience:
ky.get(resolvedFile)lacks timeout and retry configuration. A single 404 or transient network failure aborts the entire pipeline. Add{ timeout: 30_000, retry: { limit: 2 } }to handle flaky assets. -
No error handling: Missing try-catch allows unhandled rejections to propagate. Let callers decide whether to skip failed assets or fail the build.
-
Binary data corruption: Using
response.text()for all content types, then passing the result tostr2ab()corrupts binary assets (images, fonts, icons). The UTF-8 decoded text passed tocharCodeAt()loses fidelity for bytes > 0x7F. Useresponse.arrayBuffer()for binary content (images, fonts) andresponse.text()only for CSS/JS.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/single-file/src/import-map.ts` around lines 24 - 25, In loadImport, wrap
the ky.get(resolvedFile) call in a try/catch, call ky.get(resolvedFile, {
timeout: 30_000, retry: { limit: 2 } }), and in the try-block inspect the
response Content-Type (or resolvedFile extension) to decide whether to call
response.arrayBuffer() for binary assets (images, fonts, icons) or
response.text() for CSS/JS; feed the resulting ArrayBuffer directly to
downstream consumers (avoid passing text into str2ab for binaries) and for text
paths continue using response.text() and str2ab as before; in the catch block
surface/return the error (or a clear failure value) so callers of loadImport can
decide to skip or fail the build.
| const symbol = $$(`symbol#${hash}`); | ||
| const viewBox = symbol.attr("viewBox") || "0 0 24 24"; | ||
| const inner = symbol.html(); | ||
| if (!inner) { | ||
| throw new Error("unable to parse parent"); | ||
| } | ||
|
|
||
| $(current).parent().attr("viewBox", viewBox); | ||
| $(current).replaceWith(inner); | ||
| } |
There was a problem hiding this comment.
throw aborts the entire run on a single missing symbol — inconsistent with surrounding warn+continue pattern.
Earlier in the same loop, missing hash produces log.warn(...); continue. If symbol.html() returns null (hash not found in fetched SVG) we throw, killing the whole CLI before writeFile. Logging and continuing keeps the tool resilient against one broken asset reference.
🛠️ Fix
- if (!inner) {
- throw new Error("unable to parse parent");
- }
+ if (!inner) {
+ log.error(`symbol#${hash} not found in ${src}`);
+ continue;
+ }Also applies at Line 34 (img SVG-fragment branch) where an unresolved symbol.html() currently produces a data:image/svg+xml,... URI with an empty body — consider explicit logging there too.
📝 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 symbol = $$(`symbol#${hash}`); | |
| const viewBox = symbol.attr("viewBox") || "0 0 24 24"; | |
| const inner = symbol.html(); | |
| if (!inner) { | |
| throw new Error("unable to parse parent"); | |
| } | |
| $(current).parent().attr("viewBox", viewBox); | |
| $(current).replaceWith(inner); | |
| } | |
| const symbol = $$(`symbol#${hash}`); | |
| const viewBox = symbol.attr("viewBox") || "0 0 24 24"; | |
| const inner = symbol.html(); | |
| if (!inner) { | |
| log.error(`symbol#${hash} not found in ${src}`); | |
| continue; | |
| } | |
| $(current).parent().attr("viewBox", viewBox); | |
| $(current).replaceWith(inner); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/single-file/src/inline.ts` around lines 129 - 138, The code currently
throws an Error when symbol.html() is falsy, which aborts the whole run; instead
match the surrounding pattern by logging a warning and continuing: in the block
using symbol = $$(`symbol#${hash}`) and inner = symbol.html(), replace the throw
with a call to log.warn (or use the existing log/warn function used earlier)
that includes the hash and context (e.g., "missing symbol for hash ...;
skipping") and then continue the loop; also make the analogous change in the
other SVG-fragment branch where symbol.html() can be empty (the img/SVG branch
referenced at Line 34) so it logs a warning and skips or inserts a safe fallback
rather than producing an empty data:image/svg+xml URI.
| export const [error, info, warn] = DEBUG_NAMESPACES.map((namespace, index) => { | ||
| const currentDebugger = obug.createDebug(DEBUG_BASE_NAMESPACE, { | ||
| ...commonDebugOptions, | ||
| color: index + 1, | ||
| }); | ||
| const extend = debug.extend.bind(currentDebugger); | ||
| return extend(namespace); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
obug createDebug extend options color inheritance
💡 Result:
In the obug JavaScript debugging library (a lightweight fork of debug with TypeScript/ESM support), createDebug(namespace, options) creates a debugger instance. The options object supports a color property (string or number) to set custom debug log colors. The extend(subnamespace) method creates a child debugger that inherits all options (including color, useColors, formatters, etc.) from the parent. Example from official docs: const debug = createDebug('my-namespace', { useColors: true, color: 2, // custom color index // other options... }); debug('message'); // colored output const sub = debug.extend('sub-namespace'); // inherits color: 2 and all options sub('sub message'); // same color/formatting as parent console.log(sub.color); // 2 (inherited) All options in createDebug are optional. Child debuggers via extend automatically inherit the parent's full configuration for consistent namespace hierarchies.
Citations:
- 1: https://github.com/sxzz/obug
- 2: https://npmx.dev/package/obug
- 3: https://jsr.io/@sxzz/obug/doc/all_symbols
- 4: https://registry.npmjs.org/obug
Refactor to simpler, more readable pattern — current approach is unnecessarily complex.
The code creates currentDebugger with custom color, then rebinds the base debug.extend to it. While this approach does preserve the color through extend's option inheritance (per obug's documented behavior), it's more convoluted than necessary. Direct namespace construction is clearer:
♻️ Proposed refactor
-export const [error, info, warn] = DEBUG_NAMESPACES.map((namespace, index) => {
- const currentDebugger = obug.createDebug(DEBUG_BASE_NAMESPACE, {
- ...commonDebugOptions,
- color: index + 1,
- });
- const extend = debug.extend.bind(currentDebugger);
- return extend(namespace);
-});
+export const [error, info, warn] = DEBUG_NAMESPACES.map((namespace, index) =>
+ obug.createDebug(`${DEBUG_BASE_NAMESPACE}:${namespace}`, {
+ ...commonDebugOptions,
+ color: index + 1,
+ }),
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const [error, info, warn] = DEBUG_NAMESPACES.map((namespace, index) => { | |
| const currentDebugger = obug.createDebug(DEBUG_BASE_NAMESPACE, { | |
| ...commonDebugOptions, | |
| color: index + 1, | |
| }); | |
| const extend = debug.extend.bind(currentDebugger); | |
| return extend(namespace); | |
| }); | |
| export const [error, info, warn] = DEBUG_NAMESPACES.map((namespace, index) => | |
| obug.createDebug(`${DEBUG_BASE_NAMESPACE}:${namespace}`, { | |
| ...commonDebugOptions, | |
| color: index + 1, | |
| }), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/single-file/src/log.ts` around lines 17 - 24, The current mapping
creates a base debugger then rebinds debug.extend to preserve color which is
overly complex; instead, for each entry in DEBUG_NAMESPACES (used to produce
error, info, warn) directly create a debugger with obug.createDebug using a
composed namespace (combine DEBUG_BASE_NAMESPACE and the namespace string) and
merged options ({...commonDebugOptions, color: index+1}), removing the
debug.extend.bind usage and the intermediate currentDebugger variable so the
array directly contains the three named debuggers.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/smooth-bars-relate.md:
- Line 5: Update the changeset file .changeset/smooth-bars-relate.md to include
a short heading and a richer description: add a heading line at the top, then
list key features (e.g., "CLI for creating single-file HTML bundles"), main
capabilities (e.g., import mapping, inlining CSS/JS/images), and a short usage
context or example sentence describing when to use the package; keep it concise
but informative so consumers understand what the major release provides.
- Around line 1-3: The changeset currently sets a "major" bump for the initial
release which may be inappropriate; verify whether the public API (e.g., the
exported run() function, CLI flags, and package exports) is stable and if not
change the bump to "minor" or "patch". Open the .changeset/smooth-bars-relate.md
entry and, after confirming API stability of run() and any CLI/exports, replace
"major" with "minor" (or "patch" if very early) and update any accompanying
release notes to reflect the chosen level; if you decide to keep "major" add a
brief justification in the changeset that the API is stable and intended for
1.0.0 semantics. Ensure the decision is recorded so reviewers know why
run()/CLI/exports were considered stable or not.
In `@core/single-file/src/import-map.ts`:
- Line 96: The script emitted by createImportMap currently assigns the map to
window.registry (using registryString) but runtime import rewrites in inline.ts
expect window[WINDOW_KEY] (importMap.WINDOW_KEY, e.g. "imports"); change the
emitted assignment so the script populates window[WINDOW_KEY] (or
window["imports"]) instead of window.registry by using the WINDOW_KEY constant
when emitting the snippet that contains registryString so the runtime lookup in
inline.ts (window["${importMap.WINDOW_KEY}"][moduleRequest]) finds the entries.
In `@core/single-file/src/inline.ts`:
- Around line 114-153: The script handler (exported script) rewrites
static/dynamic imports to await
import(window[importMap.WINDOW_KEY][imported.moduleRequest]) but never loads or
registers those moduleRequest keys; fix by resolving each imported.moduleRequest
against the parent src (use utilities.isUrl or new URL(src, base) logic), call
importMap.loadImport({ dirname: resolvedDirname, file: resolvedUrl }) for each
resolved import (and recursively for its imports) and register the loaded module
under the same key emitted in the rewrite (imported.moduleRequest) so
window[importMap.WINDOW_KEY][imported.moduleRequest] is defined; implement a
visited Set in the recursive loader to avoid cycles and ensure the
key-normalization scheme used by writeImportMap and the rewriter matches (store
under imported.moduleRequest or consistently rewrite lookups to the
resolvedUrl).
In `@core/single-file/src/utilities.ts`:
- Around line 1-7: The function bufferToDataUri is declared async but does not
perform any asynchronous work; remove the async keyword from the bufferToDataUri
declaration to avoid forcing callers to await, or alternatively implement a
truly async path (e.g., using Blob + FileReader) if you need non-blocking base64
encoding; update the function signature (bufferToDataUri) accordingly and ensure
all call sites are adjusted to no longer await if you remove async.
- Around line 22-32: The heuristic in isProbablyUrl is using includes with
reversed arguments (obviousQueries.some(query => query.includes(str))), which
incorrectly tests whether each token contains the whole input; change it to test
whether the input contains any token (obviousQueries.some(token =>
str.includes(token)) or equivalently str.includes(token)) so that inputs
containing "function", "{" or "\n" are rejected as intended; update the
obviousQueries check inside isProbablyUrl and ensure str is treated as a string
before calling includes (e.g., keep the existing !str guard) so the heuristic
behaves correctly for inline JS and single-character inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9809032-4dbd-46f8-90dd-7d153556128b
📒 Files selected for processing (6)
.changeset/smooth-bars-relate.mdcore/single-file/src/import-map.tscore/single-file/src/index.tscore/single-file/src/inline.tscore/single-file/src/log.tscore/single-file/src/utilities.ts
| "@stephansama/single-file": major | ||
| --- | ||
|
|
||
| created single file cli package |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider enhancing the changeset description.
For a major release, a more detailed description would help consumers understand what the package does. Consider adding:
- Key features (CLI for creating single-file HTML bundles)
- Main capabilities (import mapping, inlining resources)
- Usage context
Additionally, markdownlint suggests starting with a heading, though this is optional for changeset files.
📝 Example enhanced description
---
"@stephansama/single-file": major
---
-created single file cli package
+# Initial Release
+
+Created single-file CLI package that bundles web pages into standalone HTML files with inlined resources and import mapping support.📝 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.
| created single file cli package | |
| --- | |
| "@stephansama/single-file": major | |
| --- | |
| # Initial Release | |
| Created single-file CLI package that bundles web pages into standalone HTML files with inlined resources and import mapping support. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/smooth-bars-relate.md at line 5, Update the changeset file
.changeset/smooth-bars-relate.md to include a short heading and a richer
description: add a heading line at the top, then list key features (e.g., "CLI
for creating single-file HTML bundles"), main capabilities (e.g., import
mapping, inlining CSS/JS/images), and a short usage context or example sentence
describing when to use the package; keep it concise but informative so consumers
understand what the major release provides.
|
|
||
| const registryString = Array.from(registryImports).join("\n"); | ||
|
|
||
| return `<script type="module">\nwindow.registry = {\n${registryString}\n};\n</script>`; |
There was a problem hiding this comment.
Registry written to window.registry but consumers read window[WINDOW_KEY] (= window.imports).
inline.ts line 137 rewrites every static/dynamic import to await import(window["${importMap.WINDOW_KEY}"][moduleRequest]), i.e. window["imports"][...]. This emitted snippet, however, assigns to window.registry. The runtime registry is therefore never populated under the key the rewritten scripts look up, so every rewritten import resolves to undefined and throws at runtime.
🛠️ Proposed fix
- return `<script type="module">\nwindow.registry = {\n${registryString}\n};\n</script>`;
+ return `<script type="module">\nwindow[${JSON.stringify(WINDOW_KEY)}] = {\n${registryString}\n};\n</script>`;📝 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.
| return `<script type="module">\nwindow.registry = {\n${registryString}\n};\n</script>`; | |
| return `<script type="module">\nwindow[${JSON.stringify(WINDOW_KEY)}] = {\n${registryString}\n};\n</script>`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/single-file/src/import-map.ts` at line 96, The script emitted by
createImportMap currently assigns the map to window.registry (using
registryString) but runtime import rewrites in inline.ts expect
window[WINDOW_KEY] (importMap.WINDOW_KEY, e.g. "imports"); change the emitted
assignment so the script populates window[WINDOW_KEY] (or window["imports"])
instead of window.registry by using the WINDOW_KEY constant when emitting the
snippet that contains registryString so the runtime lookup in inline.ts
(window["${importMap.WINDOW_KEY}"][moduleRequest]) finds the entries.
| export const script: InlineFunction = async ($, baseUrl) => { | ||
| for (const script of $("script[src]")) { | ||
| log.info(`loading \`script\` ${script.attribs.src}`); | ||
| const src = utilities.isUrl(script.attribs.src, baseUrl); | ||
| if (!src) continue; | ||
|
|
||
| const dirname = src.startsWith("http") | ||
| ? undefined | ||
| : path.posix.dirname(new URL(src).pathname); | ||
|
|
||
| let scriptSrc = await importMap.loadImport({ dirname, file: src }); | ||
|
|
||
| const parsed = await oxc.parse(src, scriptSrc); | ||
|
|
||
| for (const imported of parsed.module.staticImports) { | ||
| const entries = imported.entries | ||
| .map((entry) => { | ||
| return `${entry.importName}${entry.localName ? " as " + entry.localName : ""}`; | ||
| }) | ||
| .join(","); | ||
|
|
||
| scriptSrc = [ | ||
| scriptSrc.slice(0, imported.start), | ||
| js`const {${entries}}=await import(window["${importMap.WINDOW_KEY}"]["${imported.moduleRequest}"]);`, | ||
| scriptSrc.slice(imported.end), | ||
| ].join(""); | ||
| } | ||
|
|
||
| for (const imported of parsed.module.dynamicImports) { | ||
| scriptSrc = [ | ||
| scriptSrc.slice(0, imported.start), | ||
| js`await import(window["${importMap.WINDOW_KEY}"]["${imported.moduleRequest}"]);`, | ||
| scriptSrc.slice(imported.end), | ||
| ].join(""); | ||
| } | ||
|
|
||
| $(script).removeAttr("src"); | ||
| $(script).text(scriptSrc); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Imports referenced by the rewritten code are never loaded into the registry.
The handler only calls importMap.loadImport({ dirname, file: src }) for the outer <script src> (line 124). The static/dynamic imports discovered by oxc.parse are rewritten to await import(window["imports"][moduleRequest]), but nothing in this loop (or anywhere else in the pipeline) fetches those module specifiers and registers them under imported.moduleRequest. As a result:
window.importswill never containimported.moduleRequestkeys, so every rewrittenawait import(undefined)throws at runtime.- Even if loading were added,
loadImportstores entries under the rawfileargument (the resolved full URL), while the rewritten lookups use the unresolved specifier (e.g.'./foo.js'). Keys would still mismatch — you need to either store undermoduleRequestor rewrite the lookup key to the resolved URL used during loading.
A working flow needs to (a) resolve each moduleRequest against src, (b) recursively loadImport/parse it, and (c) ensure the registry key used at write-time matches the lookup key emitted here.
Want me to sketch a recursive loader (with a visited set to avoid cycles) and a key-normalization scheme that keeps writeImportMap and the rewriter in sync?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/single-file/src/inline.ts` around lines 114 - 153, The script handler
(exported script) rewrites static/dynamic imports to await
import(window[importMap.WINDOW_KEY][imported.moduleRequest]) but never loads or
registers those moduleRequest keys; fix by resolving each imported.moduleRequest
against the parent src (use utilities.isUrl or new URL(src, base) logic), call
importMap.loadImport({ dirname: resolvedDirname, file: resolvedUrl }) for each
resolved import (and recursively for its imports) and register the loaded module
under the same key emitted in the rewrite (imported.moduleRequest) so
window[importMap.WINDOW_KEY][imported.moduleRequest] is defined; implement a
visited Set in the recursive loader to avoid cycles and ensure the
key-normalization scheme used by writeImportMap and the rewriter matches (store
under imported.moduleRequest or consistently rewrite lookups to the
resolvedUrl).
| export async function bufferToDataUri( | ||
| buffer: ArrayBuffer, | ||
| mime?: null | string, | ||
| ) { | ||
| const base64 = Buffer.from(buffer).toString("base64"); | ||
| return `data:${mime || "image/png"};base64,${base64}`; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
bufferToDataUri is async but never awaits.
Buffer.from(...).toString("base64") is fully synchronous, so the async keyword adds no value and forces every call site into an await chain. Drop async (or use the WHATWG Blob/FileReader path if you actually want async base64 encoding).
♻️ Proposed refactor
-export async function bufferToDataUri(
- buffer: ArrayBuffer,
- mime?: null | string,
-) {
+export function bufferToDataUri(buffer: ArrayBuffer, mime?: null | string) {
const base64 = Buffer.from(buffer).toString("base64");
return `data:${mime || "image/png"};base64,${base64}`;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/single-file/src/utilities.ts` around lines 1 - 7, The function
bufferToDataUri is declared async but does not perform any asynchronous work;
remove the async keyword from the bufferToDataUri declaration to avoid forcing
callers to await, or alternatively implement a truly async path (e.g., using
Blob + FileReader) if you need non-blocking base64 encoding; update the function
signature (bufferToDataUri) accordingly and ensure all call sites are adjusted
to no longer await if you remove async.
| export function isProbablyUrl(str: string) { | ||
| if (!str || obviousQueries.some((query) => query.includes(str))) { | ||
| return false; | ||
| } | ||
|
|
||
| // allow: | ||
| // - absolute URLs | ||
| // - root-relative (/foo.js) | ||
| // - relative (./foo.js, foo.js) | ||
| return /^(https?:\/\/|\/|\.\/|\.\.\/|[a-zA-Z0-9_\-./]+$)/.test(str); | ||
| } |
There was a problem hiding this comment.
Reversed includes arguments break the heuristic filter.
obviousQueries.some((query) => query.includes(str)) returns true only when str is a substring of "\n", "{", or "function". The intent is clearly the opposite — to detect when str contains one of those tokens.
Consequences:
- Inline JS like
function() { ... }is not rejected (since"function".includes("function() { ... }")isfalse), so it falls through to the regex (which matches it via the[a-zA-Z0-9_\-./]+$branch — wait, it won't, because of(/space — but it still gets evaluated andisUrlwill then attemptnew URL(...)). - Single-character valid-URL strings such as
"f","n","u","{"are incorrectly rejected (they are substrings of"function"/"\n"/"{").
🛠️ Proposed fix
- if (!str || obviousQueries.some((query) => query.includes(str))) {
+ if (!str || obviousQueries.some((query) => str.includes(query))) {
return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/single-file/src/utilities.ts` around lines 22 - 32, The heuristic in
isProbablyUrl is using includes with reversed arguments
(obviousQueries.some(query => query.includes(str))), which incorrectly tests
whether each token contains the whole input; change it to test whether the input
contains any token (obviousQueries.some(token => str.includes(token)) or
equivalently str.includes(token)) so that inputs containing "function", "{" or
"\n" are rejected as intended; update the obviousQueries check inside
isProbablyUrl and ensure str is treated as a string before calling includes
(e.g., keep the existing !str guard) so the heuristic behaves correctly for
inline JS and single-character inputs.
1 participant
Checklist
mainhave been mergedmain