WIP: Refactor Out Deprecations#1
Open
martindale wants to merge 12 commits into
Open
Conversation
* Add tests for Promise, objectPath * refactor(honkit): drop q, object-path, moment, urijs Replace q with a native Promise shim (promise.ts), object-path with safeObjectPath for config paths, moment with dayjs in defaultFilters, and urijs-free git+ URL parsing in git.ts. Contract tests from the prior commit still pass (promise.contract, objectPath.contract). Remove promise.env-load.test.ts — it only covered Q.longStackSupport at load time (PR honkit#508 discussion r3231797881). Drop jest.contract coverageThreshold so test:contract passes with the shim under existing tests. Add direct safeObjectPath unit tests. object-path remains a devDependency for the baseline contract only. Suggested PR description update: - Runtime removals: q, object-path, moment, urijs - Additions: dayjs, js-yaml ^4, safeObjectPath helper, native promise surface - Equivalence: contract tests + snapshot-honkit / integration suite - fabric-http: run npm install in /Users/eric/fabric-http to capture transitive deprecation warnings motivating consumers
* fix: address review feedback from PR honkit#508 Follow-up to honkit#508 addressing Copilot review comments. - safeObjectPath: reject whole path containing __proto__/constructor/prototype instead of filtering segments (prevented `__proto__.x` from collapsing to `x`) - safeObjectPath: use Object.prototype.hasOwnProperty.call so lookups do not descend into inherited prototype properties - safeObjectPath: setAtPath now throws on non-object scalar intermediates and returns undefined, matching object-path semantics - safeObjectPath: accept `string | string[]` paths to match object-path API - encodeConfig: align with new setAtPath void return type - defaultFilters: load dayjs advancedFormat plugin so `Do` ordinal token works (e.g. `MMMM Do YYYY` now renders `May 16th 2026`) - package.json: alphabetize dayjs position in dependencies - git.ts: fix comment wording "infos" -> "information" - tests: cover new own-property guard, unsafe path rejection, and array path * fix(safeObjectPath): check enumerability not just ownership `hasOwnProperty` returns true for non-enumerable own properties, so the prior `hasOwn` guard let `getAtPath` and `setAtPath` still descend into non-enumerable own data. Switch to `Object.prototype.propertyIsEnumerable` so descent is genuinely limited to own enumerable properties, matching the documented contract. Adds tests covering non-enumerable own reads (treated as missing) and non-enumerable own intermediates in writes (existing value left untouched). * fix(safeObjectPath): match object-path own-check; throw on null intermediate Adjust the safeObjectPath contract so the divergence from object-path is limited to the path-level rejection of __proto__/constructor/prototype. - Switch back to hasOwnProperty for own-property descent. object-path itself uses hasOwnProperty (does not check enumerability), so propertyIsEnumerable was stricter than the original library. Matching object-path is the intended contract. - setAtPath now throws when an intermediate value is null. object-path threw here via the native "Cannot set properties of null" TypeError; the prior implementation silently overwrote null with a fresh container. - Missing/undefined intermediates are still auto-created (object-path behavior); only existing scalar values (including null) throw. Tests updated to cover the null-intermediate throw on both the new helper and the object-path baseline. * fix(safeObjectPath): treat array path entries as literal keys When `path` is a `string[]`, each entry is a caller-supplied property name, including the empty string. Previously the segment normalizer trimmed and filtered empty entries uniformly, which rewrote paths like `["", "key"]` into just `["key"]` and addressed an unrelated property. Now whitespace trimming and empty-segment filtering only apply to string paths (where they remove dot-split artifacts). Array entries are passed through verbatim, matching object-path's lookup behavior.
Co-authored-by: azu <19714+azu@users.noreply.github.com>
Switch the published Docker image from `honkit/honkit` on Docker Hub to `ghcr.io/honkit/honkit` on GitHub Container Registry. This removes the need for Docker Hub credentials (DOCKER_HUB_USER/DOCKER_HUB_TOKEN) and uses the built-in GITHUB_TOKEN for authentication. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bumps [brace-expansion](https://github.com/juliangruber/brace-expansion) from 1.1.12 to 5.0.6. - [Release notes](https://github.com/juliangruber/brace-expansion/releases) - [Commits](juliangruber/brace-expansion@v1.1.12...v5.0.6) --- updated-dependencies: - dependency-name: brace-expansion dependency-version: 5.0.6 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: azu <19714+azu@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR modernizes HonKit's build infrastructure and core dependencies by migrating container publishing to GitHub Container Registry, replacing external libraries (q, moment, urijs, object-path, cp/cpr, tiny-lr) with native or minimal alternatives, and updating build tooling from browserify to esbuild. ChangesRelease Infrastructure and Dependency Modernization
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 8 medium |
| ErrorProne | 2 high |
| Security | 4 critical 4 high |
| CodeStyle | 1 minor |
| Complexity | 3 medium |
🟢 Metrics 204 complexity · 2 duplication
Metric Results Complexity 204 Duplication 2
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
martindale
force-pushed
the
feature/deprecations
branch
from
9a78d03 to
874a52b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/honkit/src/utils/__tests__/promise.contract.test.ts (1)
111-120: ⚡ Quick winThis test pre-binds away the behavior it is trying to verify.
Because
ctx.run.bind(ctx)is already bound before it reachesnfbind, this still passes even ifnfbindstops preservingthis. Use the unbound method and invoke the wrapper with.call(ctx)so the contract is actually exercised.Proposed fix
- const bound = PromiseUtil.nfbind(ctx.run.bind(ctx)); - await expect(bound()).resolves.toBe(3); + const bound = PromiseUtil.nfbind(ctx.run); + await expect(bound.call(ctx)).resolves.toBe(3);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/honkit/src/utils/__tests__/promise.contract.test.ts` around lines 111 - 120, The test "nfbind binds this" incorrectly pre-binds the method before passing it to PromiseUtil.nfbind; update the test to pass the unbound method (use PromiseUtil.nfbind(ctx.run)) and then invoke the returned wrapper with .call(ctx) (e.g., const bound = PromiseUtil.nfbind(ctx.run); await expect(bound.call(ctx)).resolves.toBe(3)); this ensures nfbind must preserve the original this when invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/README.md`:
- Line 13: The markdown examples currently include a leading shell prompt "$"
which triggers MD014; remove the leading "$" from each command example (e.g.,
change "$ docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit
honkit --help" and the other similar lines) so the README contains plain command
lines without prompt symbols, keeping output semantics the same while satisfying
markdownlint.
In `@packages/honkit/src/api/deprecate.ts`:
- Around line 109-113: In deprecateRenamedMethod, ensure the resolved target
from getAtPath is callable before wrapping it: after computing fn (from
getAtPath(instance, newName)), check typeof fn === "function" (or fn instanceof
Function) and if not throw a clear TypeError/RangeError that includes the book,
key, oldName and newName so the failure is fast and informative; then pass the
validated fn into deprecateMethod and assign to instance[oldName] as before.
In `@packages/honkit/src/utils/git.ts`:
- Around line 121-129: The current code uses the first regex match
(gitSuffix.exec(giturl)) which picks the earliest ".git" and misparses URLs with
intermediate segments like "team.git/repo.git"; change the logic to locate the
last ".git" repository boundary instead: compute the last match (e.g., using
Array.from(giturl.matchAll(gitSuffix)) and taking the final match or using
giturl.lastIndexOf('.git') and validating the following char is '/' or end) and
then set host and filepath from that last match index (update uses of m.index to
the last match index) so host = giturl.slice(0, lastIndex + 4) and filepath =
giturl.slice(lastIndex + 4).
In `@packages/honkit/src/utils/promise.ts`:
- Around line 202-218: The current some() coerces every non-List input into
Immutable.List, which strips keys from keyed Immutable.Iterable (e.g.,
Map/OrderedMap) and breaks key-dependent predicates; change the conversion logic
so that if arr is an Immutable.Iterable you keep it as-is and only convert plain
JS arrays to Immutable.List, then call reduce on that Iterable (so keyed
iterables preserve their keys when iter is invoked). Update the code paths
around the list variable and the reduce call in function some to use the
original Immutable.Iterable when Immutable.Iterable.isIterable(arr) is true,
otherwise wrap plain arrays with Immutable.List, and keep using
extendPromise(Promise.resolve(undefined)) as the initial accumulator.
In `@README.md`:
- Line 61: Edit the README sentence that currently reads "Honkit provide docker
image at [ghcr.io/honkit/honkit]" and correct the subject-verb agreement and
casing by changing it to "HonKit provides docker image at
[ghcr.io/honkit/honkit]"; locate the exact string "Honkit provide docker image"
and replace it with "HonKit provides docker image" to fix the grammar and proper
project name capitalization.
- Around line 65-69: Update the fenced code block containing the three docker
commands so it includes a language identifier (e.g., add ```bash at the opening
fence) instead of a plain ```; specifically modify the block that currently
wraps the docker pull/run lines to start with ```bash and keep the closing ```
unchanged so markdownlint MD040 is satisfied.
---
Nitpick comments:
In `@packages/honkit/src/utils/__tests__/promise.contract.test.ts`:
- Around line 111-120: The test "nfbind binds this" incorrectly pre-binds the
method before passing it to PromiseUtil.nfbind; update the test to pass the
unbound method (use PromiseUtil.nfbind(ctx.run)) and then invoke the returned
wrapper with .call(ctx) (e.g., const bound = PromiseUtil.nfbind(ctx.run); await
expect(bound.call(ctx)).resolves.toBe(3)); this ensures nfbind must preserve the
original this when invoked.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a2de399-6f21-4637-8e34-0a0f6977bed1
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlreports/install.logis excluded by!**/*.log
📒 Files selected for processing (33)
.github/workflows/release.ymlREADME.mddocker/README.mdexamples/benchmark/package.jsonexamples/book/package.jsonexamples/multiple-languages/package.jsonpackage.jsonpackages/@honkit/asciidoc/package.jsonpackages/@honkit/cleaning-tools/package.jsonpackages/@honkit/honkit-plugin-fontsettings/package.jsonpackages/@honkit/honkit-plugin-highlight/package.jsonpackages/@honkit/html/package.jsonpackages/@honkit/internal-test-utils/package.jsonpackages/@honkit/markdown-legacy/package.jsonpackages/@honkit/markdown/package.jsonpackages/@honkit/markup-it/package.jsonpackages/@honkit/theme-default/package.jsonpackages/@honkit/theme/package.jsonpackages/honkit/jest.contract.config.jspackages/honkit/package.jsonpackages/honkit/src/api/deprecate.tspackages/honkit/src/api/encodeConfig.tspackages/honkit/src/constants/defaultFilters.tspackages/honkit/src/models/fs.tspackages/honkit/src/models/page.tspackages/honkit/src/utils/__tests__/objectPath.contract.test.tspackages/honkit/src/utils/__tests__/promise.contract.test.tspackages/honkit/src/utils/__tests__/safeObjectPath.tspackages/honkit/src/utils/git.tspackages/honkit/src/utils/promise.tspackages/honkit/src/utils/safeObjectPath.tsreports/README.mdscripts/report-install-ci.sh
| Show help | ||
|
|
||
| $ docker run -v `pwd`:`pwd` -w `pwd` --rm -it honkit/honkit honkit --help | ||
| $ docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit honkit --help |
There was a problem hiding this comment.
Remove leading $ from command examples to satisfy markdownlint.
These lines trigger MD014; dropping prompt symbols keeps docs lint-clean without changing meaning.
🧹 Proposed fix
- $ docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit honkit --help
+ docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit honkit --help
- $ docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit honkit build
+ docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit honkit build
- $ docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit honkit pdf
+ docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit honkit pdf
- $ docker run -it --init -p 4000:4000 -v `pwd`:`pwd` -w `pwd` --rm ghcr.io/honkit/honkit honkit serve
+ docker run -it --init -p 4000:4000 -v `pwd`:`pwd` -w `pwd` --rm ghcr.io/honkit/honkit honkit serveAlso applies to: 17-17, 21-21, 25-25
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Dollar signs used before commands without showing output
(MD014, commands-show-output)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker/README.md` at line 13, The markdown examples currently include a
leading shell prompt "$" which triggers MD014; remove the leading "$" from each
command example (e.g., change "$ docker run -v `pwd`:`pwd` -w `pwd` --rm -it
ghcr.io/honkit/honkit honkit --help" and the other similar lines) so the README
contains plain command lines without prompt symbols, keeping output semantics
the same while satisfying markdownlint.
Comment on lines
109
to
113
| function deprecateRenamedMethod(book, key, instance, oldName, newName, msg) { | ||
| msg = msg || `"${oldName}" is deprecated, use "${newName}()" instead`; | ||
| const fn = objectPath.get(instance, newName); | ||
| const fn = getAtPath(instance as Record<string, unknown>, newName) as (...args: unknown[]) => unknown; | ||
|
|
||
| instance[oldName] = deprecateMethod(book, key, fn, msg); |
There was a problem hiding this comment.
Validate the renamed target resolves to a callable before wrapping.
Line 111 can resolve a non-function/undefined, which then crashes later in deprecateMethod at call time (fn.apply). Fail fast here with a clear error.
Proposed fix
function deprecateRenamedMethod(book, key, instance, oldName, newName, msg) {
msg = msg || `"${oldName}" is deprecated, use "${newName}()" instead`;
- const fn = getAtPath(instance as Record<string, unknown>, newName) as (...args: unknown[]) => unknown;
+ const fn = getAtPath(instance as Record<string, unknown>, newName);
+ if (!is.fn(fn)) {
+ throw new TypeError(
+ `Cannot deprecate "${oldName}": "${newName}" does not resolve to a function`
+ );
+ }
- instance[oldName] = deprecateMethod(book, key, fn, msg);
+ instance[oldName] = deprecateMethod(book, key, fn, msg);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function deprecateRenamedMethod(book, key, instance, oldName, newName, msg) { | |
| msg = msg || `"${oldName}" is deprecated, use "${newName}()" instead`; | |
| const fn = objectPath.get(instance, newName); | |
| const fn = getAtPath(instance as Record<string, unknown>, newName) as (...args: unknown[]) => unknown; | |
| instance[oldName] = deprecateMethod(book, key, fn, msg); | |
| function deprecateRenamedMethod(book, key, instance, oldName, newName, msg) { | |
| msg = msg || `"${oldName}" is deprecated, use "${newName}()" instead`; | |
| const fn = getAtPath(instance as Record<string, unknown>, newName); | |
| if (!is.fn(fn)) { | |
| throw new TypeError( | |
| `Cannot deprecate "${oldName}": "${newName}" does not resolve to a function` | |
| ); | |
| } | |
| instance[oldName] = deprecateMethod(book, key, fn, msg); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/honkit/src/api/deprecate.ts` around lines 109 - 113, In
deprecateRenamedMethod, ensure the resolved target from getAtPath is callable
before wrapping it: after computing fn (from getAtPath(instance, newName)),
check typeof fn === "function" (or fn instanceof Function) and if not throw a
clear TypeError/RangeError that includes the book, key, oldName and newName so
the failure is fast and informative; then pass the validated fn into
deprecateMethod and assign to instance[oldName] as before.
Comment on lines
+121
to
+129
| // Match a real repository ".git" suffix (not e.g. "gist.github.com" where ".git" appears inside the hostname) | ||
| const gitSuffix = /\.git(\/|$)/; | ||
| const m = gitSuffix.exec(giturl); | ||
| if (!m) { | ||
| return null; | ||
| } | ||
|
|
||
| // Recreate pathname without the real filename | ||
| uri.path(`${fileParts[0]}.git`); | ||
| const host = giturl.slice(0, m.index + 4); | ||
| let filepath = giturl.slice(m.index + 4); |
There was a problem hiding this comment.
Select the last .git repository boundary, not the first.
Using the first .git(/|$) match misparses valid URLs with namespace segments ending in .git (e.g., .../team.git/repo.git/...), which breaks clone/resolve.
Proposed fix
- const gitSuffix = /\.git(\/|$)/;
- const m = gitSuffix.exec(giturl);
- if (!m) {
+ const gitSuffix = /\.git(\/|$)/g;
+ let m: RegExpExecArray | null = null;
+ let repoMatch: RegExpExecArray | null = null;
+ while ((m = gitSuffix.exec(giturl)) !== null) {
+ repoMatch = m;
+ }
+ if (!repoMatch) {
return null;
}
- const host = giturl.slice(0, m.index + 4);
- let filepath = giturl.slice(m.index + 4);
+ const host = giturl.slice(0, repoMatch.index + 4);
+ let filepath = giturl.slice(repoMatch.index + 4);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Match a real repository ".git" suffix (not e.g. "gist.github.com" where ".git" appears inside the hostname) | |
| const gitSuffix = /\.git(\/|$)/; | |
| const m = gitSuffix.exec(giturl); | |
| if (!m) { | |
| return null; | |
| } | |
| // Recreate pathname without the real filename | |
| uri.path(`${fileParts[0]}.git`); | |
| const host = giturl.slice(0, m.index + 4); | |
| let filepath = giturl.slice(m.index + 4); | |
| // Match a real repository ".git" suffix (not e.g. "gist.github.com" where ".git" appears inside the hostname) | |
| const gitSuffix = /\.git(\/|$)/g; | |
| let m: RegExpExecArray | null = null; | |
| let repoMatch: RegExpExecArray | null = null; | |
| while ((m = gitSuffix.exec(giturl)) !== null) { | |
| repoMatch = m; | |
| } | |
| if (!repoMatch) { | |
| return null; | |
| } | |
| const host = giturl.slice(0, repoMatch.index + 4); | |
| let filepath = giturl.slice(repoMatch.index + 4); |
🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 123-123: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/honkit/src/utils/git.ts` around lines 121 - 129, The current code
uses the first regex match (gitSuffix.exec(giturl)) which picks the earliest
".git" and misparses URLs with intermediate segments like "team.git/repo.git";
change the logic to locate the last ".git" repository boundary instead: compute
the last match (e.g., using Array.from(giturl.matchAll(gitSuffix)) and taking
the final match or using giturl.lastIndexOf('.git') and validating the following
char is '/' or end) and then set host and filepath from that last match index
(update uses of m.index to the last match index) so host = giturl.slice(0,
lastIndex + 4) and filepath = giturl.slice(lastIndex + 4).
Comment on lines
+202
to
+218
| function some<T>( | ||
| arr: Immutable.List<T> | unknown[] | Immutable.Iterable<T, unknown>, | ||
| iter: (el: T, i: unknown) => unknown | ||
| ) { | ||
| const list = Immutable.List.isList(arr) ? (arr as Immutable.List<T>) : Immutable.List(arr as T[]); | ||
|
|
||
| return list.reduce( | ||
| (prev: ExtendedPromise<unknown>, elem: T, i: number) => { | ||
| return prev.then((val) => { | ||
| if (val) { | ||
| return val; | ||
| } | ||
| return iter(elem, i); | ||
| }); | ||
| }, | ||
| extendPromise(Promise.resolve(undefined)) | ||
| ); |
There was a problem hiding this comment.
Preserve keyed Immutable iterables in some().
Line 206 coerces every non-List input into Immutable.List, so Immutable.Map/OrderedMap lose their original keys before iter runs. That changes the exported Immutable.Iterable contract and can return the wrong match for key-dependent predicates.
Proposed fix
function some<T>(
arr: Immutable.List<T> | unknown[] | Immutable.Iterable<T, unknown>,
iter: (el: T, i: unknown) => unknown
) {
- const list = Immutable.List.isList(arr) ? (arr as Immutable.List<T>) : Immutable.List(arr as T[]);
+ const list = Immutable.Iterable.isIterable(arr) ? arr : Immutable.List(arr as T[]);
return list.reduce(
(prev: ExtendedPromise<unknown>, elem: T, i: number) => {
return prev.then((val) => {
if (val) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function some<T>( | |
| arr: Immutable.List<T> | unknown[] | Immutable.Iterable<T, unknown>, | |
| iter: (el: T, i: unknown) => unknown | |
| ) { | |
| const list = Immutable.List.isList(arr) ? (arr as Immutable.List<T>) : Immutable.List(arr as T[]); | |
| return list.reduce( | |
| (prev: ExtendedPromise<unknown>, elem: T, i: number) => { | |
| return prev.then((val) => { | |
| if (val) { | |
| return val; | |
| } | |
| return iter(elem, i); | |
| }); | |
| }, | |
| extendPromise(Promise.resolve(undefined)) | |
| ); | |
| function some<T>( | |
| arr: Immutable.List<T> | unknown[] | Immutable.Iterable<T, unknown>, | |
| iter: (el: T, i: unknown) => unknown | |
| ) { | |
| const list = Immutable.Iterable.isIterable(arr) ? arr : Immutable.List(arr as T[]); | |
| return list.reduce( | |
| (prev: ExtendedPromise<unknown>, elem: T, i: number) => { | |
| return prev.then((val) => { | |
| if (val) { | |
| return val; | |
| } | |
| return iter(elem, i); | |
| }); | |
| }, | |
| extendPromise(Promise.resolve(undefined)) | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/honkit/src/utils/promise.ts` around lines 202 - 218, The current
some() coerces every non-List input into Immutable.List, which strips keys from
keyed Immutable.Iterable (e.g., Map/OrderedMap) and breaks key-dependent
predicates; change the conversion logic so that if arr is an Immutable.Iterable
you keep it as-is and only convert plain JS arrays to Immutable.List, then call
reduce on that Iterable (so keyed iterables preserve their keys when iter is
invoked). Update the code paths around the list variable and the reduce call in
function some to use the original Immutable.Iterable when
Immutable.Iterable.isIterable(arr) is true, otherwise wrap plain arrays with
Immutable.List, and keep using extendPromise(Promise.resolve(undefined)) as the
initial accumulator.
| ## Docker support | ||
|
|
||
| Honkit provide docker image at [honkit/honkit](https://hub.docker.com/r/honkit/honkit). | ||
| Honkit provide docker image at [ghcr.io/honkit/honkit](https://github.com/honkit/honkit/pkgs/container/honkit). |
There was a problem hiding this comment.
Fix subject-verb agreement in the Docker support sentence.
Line 61 should read “HonKit provides …” for correct user-facing grammar.
✏️ Proposed fix
-Honkit provide docker image at [ghcr.io/honkit/honkit](https://github.com/honkit/honkit/pkgs/container/honkit).
+HonKit provides a Docker image at [ghcr.io/honkit/honkit](https://github.com/honkit/honkit/pkgs/container/honkit).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Honkit provide docker image at [ghcr.io/honkit/honkit](https://github.com/honkit/honkit/pkgs/container/honkit). | |
| HonKit provides a Docker image at [ghcr.io/honkit/honkit](https://github.com/honkit/honkit/pkgs/container/honkit). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 61, Edit the README sentence that currently reads "Honkit
provide docker image at [ghcr.io/honkit/honkit]" and correct the subject-verb
agreement and casing by changing it to "HonKit provides docker image at
[ghcr.io/honkit/honkit]"; locate the exact string "Honkit provide docker image"
and replace it with "HonKit provides docker image" to fix the grammar and proper
project name capitalization.
Comment on lines
65
to
69
| ``` | ||
| docker pull honkit/honkit | ||
| docker run -v `pwd`:`pwd` -w `pwd` --rm -it honkit/honkit honkit build | ||
| docker run -v `pwd`:`pwd` -w `pwd` --rm -it honkit/honkit honkit pdf | ||
| docker pull ghcr.io/honkit/honkit | ||
| docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit honkit build | ||
| docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit honkit pdf | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced command block.
The fenced block is missing a language marker, which triggers markdownlint (MD040).
🧩 Proposed fix
-```
+```bash
docker pull ghcr.io/honkit/honkit
docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit honkit build
docker run -v `pwd`:`pwd` -w `pwd` --rm -it ghcr.io/honkit/honkit honkit pdf</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 65-65: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @README.md around lines 65 - 69, Update the fenced code block containing the
three docker commands so it includes a language identifier (e.g., add bash at the opening fence) instead of a plain ; specifically modify the block that
currently wraps the docker pull/run lines to start with bash and keep the closing unchanged so markdownlint MD040 is satisfied.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Replace cpr/cp directory copies with Node fs helpers, swap tiny-lr for livereload (no raw-body chain), and require Node >=20. Refresh lockfile and reports/install.log after report:install.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/honkit/src/cli/serve.ts`:
- Around line 200-205: The current check only prevents creating the livereload
server but doesn't disable live injection elsewhere; update the logic so lrport
=== 0 is treated as fully disabling live reload by propagating that state to the
rest of the flow (e.g., set kwargs.live = false or clear hasLiveReloading)
before returning; locate the block around hasWatch / hasLiveReloading /
kwargs.lrport and ensure startServer() and generateBook() see live as disabled
(or make generateBook()/startServer() also check kwargs.lrport === 0 before
injecting the livereload plugin) so no livereload snippet/plugin is injected
when lrport is 0.
In `@packages/honkit/src/utils/fs.ts`:
- Around line 197-199: The current delete-first branch swallows all errors by
appending .catch(() => undefined) to fs.promises.rm(dest, { recursive: true,
force: true }), which hides permission/I/O failures; remove the catch and let
the promise reject so callers see real errors (i.e., replace await
fs.promises.rm(dest, { recursive: true, force: true }).catch(() => undefined)
with a plain await fs.promises.rm(dest, { recursive: true, force: true }) so
opts.deleteFirst, fs.promises.rm and dest are used but failures are not silently
ignored).
- Around line 196-206: Detect and block self- or nested-destination copies at
the start of copyDirRecursive: resolve src and dest with path.resolve and if
they are equal (resolvedSrc === resolvedDest) throw or return early to avoid
deleting the source when opts.deleteFirst is true; also detect if resolvedDest
is inside resolvedSrc (resolvedDest.startsWith(resolvedSrc + path.sep)) and fail
fast (throw an error) to prevent recursing into a destination nested under the
source. Additionally, when iterating entries in copyDirRecursive (function
copyDirRecursive), skip any entry whose resolved source path equals or is inside
resolvedDest to avoid copying into the newly-created destination directory.
Ensure references to src, dest, opts.deleteFirst, and function name
copyDirRecursive are used so the checks are added in the right place.
In `@reports/README.md`:
- Line 6: Update the table row that starts with "Fresh install log (wiped
`node_modules`, regenerated lockfile)" so the output path cell shows
`reports/install.log` instead of `install.log` to match the actual script
output; locate and edit the Markdown table row containing the text `Fresh
install log` and replace the backticked `install.log` with
`reports/install.log`.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b712da8-b53b-485b-9f30-3f5163ffaaaa
⛔ Files ignored due to path filters (3)
packages/honkit/src/__tests__/__snapshots__/snapshot-honkit.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yamlreports/install.logis excluded by!**/*.log
📒 Files selected for processing (18)
.github/workflows/test.ymlexamples/benchmark/book.jsonexamples/benchmark/package.jsonpackage.jsonpackages/@honkit/html/package.jsonpackages/@honkit/html/src/dom.tspackages/@honkit/html/src/index.tspackages/@honkit/markup-it/package.jsonpackages/@honkit/theme-default/package.jsonpackages/@honkit/theme/package.jsonpackages/honkit/package.jsonpackages/honkit/src/cli/serve.tspackages/honkit/src/output/modifiers/annotateText.tspackages/honkit/src/output/modifiers/fetchRemoteImages.tspackages/honkit/src/output/modifiers/inlineSvg.tspackages/honkit/src/output/modifiers/modifyHTML.tspackages/honkit/src/utils/fs.tsreports/README.md
💤 Files with no reviewable changes (1)
- examples/benchmark/package.json
✅ Files skipped from review due to trivial changes (3)
- packages/honkit/src/output/modifiers/fetchRemoteImages.ts
- packages/@honkit/html/package.json
- packages/@honkit/html/src/dom.ts
Comment on lines
+200
to
+205
| if (!hasWatch || !hasLiveReloading || kwargs.lrport === 0) { | ||
| return; | ||
| } | ||
|
|
||
| lrServer = tinylr({}); | ||
|
|
||
| return Promise.nfcall(lrServer.listen.bind(lrServer), kwargs.lrport).then(() => { | ||
| console.log("Live reload server started on port:", kwargs.lrport); | ||
| console.log("Press CTRL+C to quit ..."); | ||
| console.log(""); | ||
| lrServer = livereload.createServer({ | ||
| port: kwargs.lrport |
There was a problem hiding this comment.
Propagate lrport === 0 as a real live-reload disable.
Line 200 only skips the server bootstrap. startServer() / generateBook() still treat kwargs["live"] as enabled, so --lrport=0 still injects the livereload plugin into generated pages while no LiveReload server exists.
Suggested fix
function startServer(args, kwargs) {
@@
- const hasLiveReloading = kwargs["live"];
+ const hasLiveReloading = kwargs["live"] && kwargs.lrport !== 0;
@@
exec: function (args, kwargs) {
server = new Server();
const hasWatch = kwargs["watch"];
- const hasLiveReloading = kwargs["live"];
+ const hasLiveReloading = kwargs["live"] && kwargs.lrport !== 0;
@@
- if (!hasWatch || !hasLiveReloading || kwargs.lrport === 0) {
+ if (!hasWatch || !hasLiveReloading) {
return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/honkit/src/cli/serve.ts` around lines 200 - 205, The current check
only prevents creating the livereload server but doesn't disable live injection
elsewhere; update the logic so lrport === 0 is treated as fully disabling live
reload by propagating that state to the rest of the flow (e.g., set kwargs.live
= false or clear hasLiveReloading) before returning; locate the block around
hasWatch / hasLiveReloading / kwargs.lrport and ensure startServer() and
generateBook() see live as disabled (or make generateBook()/startServer() also
check kwargs.lrport === 0 before injecting the livereload plugin) so no
livereload snippet/plugin is injected when lrport is 0.
Comment on lines
+196
to
+206
| async function copyDirRecursive(src, dest, opts) { | ||
| if (opts.deleteFirst) { | ||
| await fs.promises.rm(dest, { recursive: true, force: true }).catch(() => undefined); | ||
| } | ||
| await fs.promises.mkdir(dest, { recursive: true }); | ||
| const entries = await fs.promises.readdir(src, { withFileTypes: true }); | ||
| for (const entry of entries) { | ||
| const from = path.join(src, entry.name); | ||
| const to = path.join(dest, entry.name); | ||
| if (entry.isDirectory()) { | ||
| await copyDirRecursive(from, to, { ...opts, deleteFirst: false }); |
There was a problem hiding this comment.
Block self-copies before mutating the destination.
If src and dest resolve to the same path, Line 198 deletes the source tree when deleteFirst is set. If dest is nested under src, Lines 200-206 recurse into the directory being created and can keep copying until path-length or disk limits stop it.
🛑 Proposed guard
async function copyDirRecursive(src, dest, opts) {
+ const resolvedSrc = path.resolve(src);
+ const resolvedDest = path.resolve(dest);
+ const relativeDest = path.relative(resolvedSrc, resolvedDest);
+ const destIsSameOrNested =
+ relativeDest === "" ||
+ (!relativeDest.startsWith("..") && !path.isAbsolute(relativeDest));
+
+ if (destIsSameOrNested) {
+ throw new Error("Destination must not be the source directory or a child of it");
+ }
+
if (opts.deleteFirst) {
await fs.promises.rm(dest, { recursive: true, force: true }).catch(() => undefined);
}🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 203-203: packages/honkit/src/utils/fs.ts#L203
Detected possible user input going into a path.join or path.resolve function.
[failure] 204-204: packages/honkit/src/utils/fs.ts#L204
Detected possible user input going into a path.join or path.resolve function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/honkit/src/utils/fs.ts` around lines 196 - 206, Detect and block
self- or nested-destination copies at the start of copyDirRecursive: resolve src
and dest with path.resolve and if they are equal (resolvedSrc === resolvedDest)
throw or return early to avoid deleting the source when opts.deleteFirst is
true; also detect if resolvedDest is inside resolvedSrc
(resolvedDest.startsWith(resolvedSrc + path.sep)) and fail fast (throw an error)
to prevent recursing into a destination nested under the source. Additionally,
when iterating entries in copyDirRecursive (function copyDirRecursive), skip any
entry whose resolved source path equals or is inside resolvedDest to avoid
copying into the newly-created destination directory. Ensure references to src,
dest, opts.deleteFirst, and function name copyDirRecursive are used so the
checks are added in the right place.
Comment on lines
+197
to
+199
| if (opts.deleteFirst) { | ||
| await fs.promises.rm(dest, { recursive: true, force: true }).catch(() => undefined); | ||
| } |
There was a problem hiding this comment.
Don't swallow deleteFirst failures.
Line 198 ignores every fs.promises.rm error, so permission or I/O failures silently turn into a merge with stale destination contents. force: true already covers the missing-path case.
🧹 Proposed fix
if (opts.deleteFirst) {
- await fs.promises.rm(dest, { recursive: true, force: true }).catch(() => undefined);
+ await fs.promises.rm(dest, { recursive: true, force: true });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (opts.deleteFirst) { | |
| await fs.promises.rm(dest, { recursive: true, force: true }).catch(() => undefined); | |
| } | |
| if (opts.deleteFirst) { | |
| await fs.promises.rm(dest, { recursive: true, force: true }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/honkit/src/utils/fs.ts` around lines 197 - 199, The current
delete-first branch swallows all errors by appending .catch(() => undefined) to
fs.promises.rm(dest, { recursive: true, force: true }), which hides
permission/I/O failures; remove the catch and let the promise reject so callers
see real errors (i.e., replace await fs.promises.rm(dest, { recursive: true,
force: true }).catch(() => undefined) with a plain await fs.promises.rm(dest, {
recursive: true, force: true }) so opts.deleteFirst, fs.promises.rm and dest are
used but failures are not silently ignored).
|
|
||
| | Report | Command | | ||
| | --- | --- | | ||
| | Fresh install log (wiped `node_modules`, regenerated lockfile) | `pnpm run report:install` → `install.log` | |
There was a problem hiding this comment.
Correct output path in the report table.
The table says install.log, but the script writes to reports/install.log. Please align this row to avoid confusion.
Suggested doc fix
-| Fresh install log (wiped `node_modules`, regenerated lockfile) | `pnpm run report:install` → `install.log` |
+| Fresh install log (wiped `node_modules`, regenerated lockfile) | `pnpm run report:install` → `reports/install.log` |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | Fresh install log (wiped `node_modules`, regenerated lockfile) | `pnpm run report:install` → `install.log` | | |
| | Fresh install log (wiped `node_modules`, regenerated lockfile) | `pnpm run report:install` → `reports/install.log` | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@reports/README.md` at line 6, Update the table row that starts with "Fresh
install log (wiped `node_modules`, regenerated lockfile)" so the output path
cell shows `reports/install.log` instead of `install.log` to match the actual
script output; locate and edit the Markdown table row containing the text `Fresh
install log` and replace the backticked `install.log` with
`reports/install.log`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Container for work on eliminating all deprecations. Initial work adds two new commands,
report:installand a CI-safe equivalentreport:install-cias well as areports/directory to contain the reference artifact.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation