Stephansama/ste 14 multiple deploy targets#187
Conversation
This new package provides a CLI tool to facilitate publishing JavaScript packages to multiple package registries or providers. It includes: - core logic for multi-provider publishing - a build script for bundling and schema generation - CLI entry point - configuration schema (using zod) - initial documentation
The `multipublish` tool now supports publishing packages to npm registries with two distinct strategies: - **`.npmrc` strategy**: Creates or updates a root-level `.npmrc` file temporarily with registry and authentication token details. This is useful for scenarios where `publishConfig` in `package.json` is not desired or conflicts with other configurations. The `.npmrc` file is cleaned up after publishing. - **`package.json` strategy**: Sets the `publishConfig.registry` field directly in the package's `package.json` file. This is suitable when the registry information can reside within the package's manifest. The `package.json` is reverted to its original state after publishing. Additionally, this change: - Ensures the `version` field in `jsr.json` is updated before publishing. - Introduces the `dedent` package for cleaner multiline string definitions.
🦋 Changeset detectedLatest commit: 0991446 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
⌛ 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. 📒 Files selected for processing (6)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new core/multipublish package: CLI, build scripts, TypeScript sources, zod schemas, catalog loaders, package-manager detection, publish orchestration for JSR and npm (multiple strategies), tests, docs, workspace/catalog entries, and package manifest and tooling configs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
User->>CLI: run multipublish [args]
CLI->>Args: parseArgs()
CLI->>Config: loadConfig(args)
CLI->>Monorepo: getPackages()
CLI->>ReleaseLoader: loadReleases(args/stdin)
loop per released package
CLI->>Detect: detectPackageManager()
loop per configured platform
alt platform == jsr
Publish->>JSR: loadConfig & update version
Publish->>Catalog: optional updatePackageJsonWithCatalog
Publish->>Agent: run jsr publish command
Publish->>FS: cleanup temp files
else platform == npm
alt strategy == .npmrc
Publish->>FS: write temporary .npmrc with token
else strategy == package.json
Publish->>Package: modify publishConfig.registry
end
Publish->>Agent: run npm/pnpm/yarn publish (dry-run possible)
Publish->>FS: cleanup temp files
end
end
end
CLI-->>User: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Summary of ChangesHello @stephansama, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new core package, Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
initial release of `@stephansama/multipublish`, a cli tool for publishing packages to multiple registries. features include: - npm publishing strategies: supports .npmrc file modification or package.json's publishconfig.registry. - jsr support: automatically updates `version` in `jsr.json` prior to publishing. - configuration: fully configurable via a zod schema.
More templates
@stephansama/ai-commit-msg
@stephansama/alfred-kaomoji
@stephansama/astro-iconify-svgmap
@stephansama/auto-readme
@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/svelte-social-share-links
@stephansama/typed-events
@stephansama/typed-templates
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Fix all issues with AI agents
In @core/multipublish/package.json:
- Around line 63-66: The package.json's "publishConfig" object currently sets
"registry": "http://localhost:487", which will override npm's public registry
and break public publishing; remove the local registry entry (or set "registry"
to the correct public registry) from the "publishConfig" object in
core/multipublish/package.json, or implement an environment/CI-driven override
(e.g., rely on NPM_CONFIG_REGISTRY or CI .npmrc) and document that behavior so
local dev can use a local registry without shipping the local URL in
publishConfig.
- Around line 59-61: The peer dependency "jsr": ">=0" in package.json is too
permissive; update the peerDependencies entry for jsr to a sensible minimum
compatible range (e.g., set "jsr": "^0.13.0" or if wider compatibility is
desired "jsr": "^0.12.0") to prevent accepting breaking versions, then run your
install/lockfile update to reflect the change; target the peerDependencies block
in package.json and change the jsr version string accordingly.
In @core/multipublish/README.md:
- Line 25: The README's "Usage" section is empty; add a concise usage guide that
describes the CLI entrypoint and primary commands, e.g., list commands like
"multipublish init", "multipublish publish", "multipublish status" (or the
actual command names implemented in the CLI), list common flags/options (e.g.,
--config, --dry-run, --verbose, --target), document configuration file format
and relevant environment variables, and include 2–3 short examples showing
typical invocation sequences (init -> configure -> publish and a dry-run
example). Keep the section organized with subheadings "Commands", "Options",
"Configuration", and "Examples" and ensure each command references the actual
function/command names implemented in the codebase so users can map docs to the
CLI implementation.
- Line 10: The "Table of contents" heading is using an h5 (#####) but should be
h2 to follow the top-level document title; update the heading text "##### Table
of contents" to "## Table of contents" in the core/multipublish/README.md so the
document hierarchy increments correctly.
In @core/multipublish/src/args.ts:
- Around line 7-15: The "output" option in the args object has an unclear
description ("use changesets"); update the args definition so the output
property's description accurately reflects its behavior (for example "Output
mode", "Display output", or a specific action like "Generate changesets") or, if
the flag actually controls changeset usage, rename the property from output to
something explicit like changesets or generateChangesets; adjust any references
to args.output elsewhere (call sites/parsing) to match the new name and ensure
the type/signature remains consistent with Options.
In @core/multipublish/src/index.ts:
- Line 12: The parsed args variable is unused: remove the call to parseArgs()
and the unused args variable from the top-level initialization, or actually
consume it—update the code that calls parseArgs() (the args variable) to pass
its values into the existing logic (for example use args.config to override the
config path, args.output to control changesets behavior, or other flags your CLI
expects) and replace any hardcoded values with those args; ensure parseArgs() is
only invoked within the same scope where its result is referenced (function
index.ts top-level where const args = await parseArgs() currently appears).
In @core/multipublish/src/jsr.ts:
- Around line 29-38: The JsrSchema type alias is declared before jsrSchema; move
the type alias declaration to immediately after the jsrSchema constant so
JsrSchema = z.infer<typeof jsrSchema> follows the schema definition, keeping the
existing names (jsrSchema and JsrSchema) unchanged and ensuring readability and
conventional ordering.
In @core/multipublish/src/publish.ts:
- Around line 21-27: The npmPublishCommand map contains empty strings for bun
and deno which can lead to execSync("") being called; update the code that
invokes execSync to first look up npmPublishCommand[packageManager] (using the
npmPublishCommand constant and AgentName keys) and validate that the command is
a non-empty string, throwing a clear Error like "npm publishing is not supported
with <packageManager>" for unsupported managers before calling
cp.execSync(command, { stdio: 'inherit' }); ensure this guard is added wherever
execSync is used for publishing.
- Around line 121-124: The current write of package.json uses
JSON.stringify(pkg.packageJson) which produces minified output; update the write
to serialize with indentation and a trailing newline (e.g.,
JSON.stringify(pkg.packageJson, null, 2) + "\n") before calling fsp.writeFile on
packageJsonPath so package.json remains human-readable and produces cleaner
diffs; keep the assignment pkg.packageJson.version = pkg.newVersion unchanged
and continue to use packageJsonPath and fsp.writeFile for persistence.
- Around line 68-73: The JSR publish command is hard-coded to always include
"--dry-run" which prevents real publishes; update the logic around
util.chdir(...) so the cp.execSync invocation builds the command using
jsrPublishCommand[packageManager] plus optional flags controlled by a new
boolean parameter or config (e.g., dryRun or publishMode) rather than always
appending "--dry-run"; ensure the code that calls the publish function (or the
surrounding options object) can set this flag, and then pass either "
--allow-dirty --dry-run" or " --allow-dirty" to cp.execSync accordingly
(referencing jsrPublishCommand, packageManager, util.chdir, and cp.execSync to
locate the change).
- Around line 50-51: The code computes pkgRoot using path.dirname(pkg.dir) which
yields the parent directory instead of the package directory; change the pkgRoot
calculation to use the package directory itself (e.g., pkg.dir or
path.resolve(pkg.dir)) before calling util.loadJsrConfigFile so
util.loadJsrConfigFile(pkgRoot) reads/writes the JSR config in the correct
package directory (update the reference to pkgRoot where used).
- Around line 96-100: The code assumes every package name has a scope by
splitting pkg.packageJson.name and checking scope.startsWith("@"), which throws
a confusing error for unscoped packages like "lodash"; update the logic in
publish.ts where scope is derived (const scope =
pkg.packageJson.name.split("/").at(0)) to first detect whether the name contains
a "/" (i.e., is scoped) and only validate startsWith("@") for scoped names, and
otherwise allow or explicitly handle unscoped package names with a clearer error
message referencing pkg.packageJson.name and the scope variable.
- Around line 126-132: The cleanup call uses util.gitClean(".npmrc") with a
relative path while npmrcFile is the repo-root absolute path, and
packageJsonPath is being passed to util.gitClean which will not restore tracked
changes; update the code to call util.gitClean(npmrcFile) when config.strategy
=== ".npmrc" (so the correct absolute .npmrc is removed) and replace
util.gitClean(packageJsonPath) with a proper restore for tracked files (e.g.,
util.gitCheckout(packageJsonPath) or run git checkout -- <packageJsonPath>) so
version bumps in packageJsonPath aren’t discarded by a clean operation.
In @core/multipublish/src/types.ts:
- Around line 1-17: Replace the loose string "type" fields with a string literal
union to enforce allowed bump kinds: update ChangesetsRelease's "type: string"
and Release's "type: string" to something like type: 'major' | 'minor' | 'patch'
(add any other known values your code expects such as 'none' or 'premajor'),
then fix any call sites or tests that pass other strings and run the typechecker
to ensure compatibility; target the ChangesetsRelease and Release type
declarations in this file and update any related imports/usages across the
codebase accordingly.
In @core/multipublish/src/util.ts:
- Around line 61-65: transformPkgJsonForJsr currently writes a minified file
with JSON.stringify(parsed) and duplicates writing logic that publishPlatform
also may perform; update transformPkgJsonForJsr to not perform redundant I/O and
to preserve formatting by serializing parsed with JSON.stringify(parsed, null,
2) (or using the same formatting helper used in publish.ts), and instead of
unconditionally writing the file inside transformPkgJsonForJsr either return the
parsed JsrSchema and let publishPlatform handle writing (remove the
fsp.writeFile call) or centralize the write into a single helper that both
transformPkgJsonForJsr and publishPlatform call (using JSR_CONFIG_FILENAME and
jsrTransformer.parse to produce the content).
- Around line 39-41: The gitClean function currently interpolates filename into
a shell string which permits command injection; change gitClean to call
cp.execFileSync (not execSync) and pass "git" as the command with arguments
["clean", "-f", filename] (and preserve { stdio: "inherit" }) so the filename is
passed as an argv element rather than interpreted by a shell; validate or
restrict filename if necessary (e.g., ensure it is a basename or contains no
path traversal) before calling execFileSync.
- Around line 15-23: The chdir helper can leave the process in the wrong
directory if callback() throws; update the chdir function to save cwd, call
process.chdir(newDir), then await callback() inside a try block and always call
process.chdir(cwd) in a finally block so the original working directory is
restored even on exceptions (rethrow any error after restoring).
- Around line 57-58: Wrap the JSON parsing in a try/catch to handle malformed
JSON from fsp.readFile; catch the SyntaxError thrown by JSON.parse(file) and
rethrow or return a clearer error that includes the filename (configFile) and
the original error message (or error) so callers can identify which config
failed and why—update the logic around the existing JSON.parse(file) call in the
same function to perform this safe-parse and emit a descriptive error.
- Line 46: Add an "engines" field to the package.json for the multipublish
package to declare Node.js 22+ compatibility: open the package.json for the
multipublish package and add "engines": { "node": ">=22.0.0" } at the top level
so consumers and installers are informed that fs.globSync (used in util.ts)
requires Node 22.0.0 or newer.
In @core/multipublish/test.js:
- Line 1: Delete the unused test file that only contains an empty console.log();
remove the entire file (the placeholder test.js) from the repository so no empty
debug/artifact remains; if future tests are needed, replace it with proper test
scaffolding using the project's test framework (e.g., Vitest).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.config/.cspell.json.gitignoreREADME.mdcore/multipublish/README.mdcore/multipublish/build.mjscore/multipublish/cli.mjscore/multipublish/package.jsoncore/multipublish/src/args.tscore/multipublish/src/config.tscore/multipublish/src/detect.tscore/multipublish/src/index.tscore/multipublish/src/jsr.tscore/multipublish/src/publish.tscore/multipublish/src/schema.tscore/multipublish/src/types.tscore/multipublish/src/util.tscore/multipublish/test.jscore/multipublish/tsconfig.jsoncore/multipublish/typedoc.jsonpackage.jsonpnpm-workspace.yaml
🧰 Additional context used
🧬 Code graph analysis (8)
core/multipublish/cli.mjs (2)
core/find-makefile-targets/cli.mjs (1)
mod(5-5)core/auto-readme/cli.mjs (1)
mod(5-5)
core/multipublish/src/index.ts (3)
core/multipublish/src/args.ts (1)
parseArgs(17-31)core/multipublish/src/config.ts (1)
loadConfig(12-20)core/multipublish/src/publish.ts (1)
publishPlatform(37-139)
pnpm-workspace.yaml (1)
scripts/generate-examples.js (1)
pkg(39-44)
core/multipublish/build.mjs (2)
core/multipublish/src/schema.ts (1)
configSchema(20-30)core/auto-readme/build.mjs (1)
build(22-31)
core/multipublish/src/publish.ts (2)
core/multipublish/src/detect.ts (2)
AgentName(3-3)detectPackageManager(7-17)core/multipublish/src/schema.ts (3)
Config(19-19)jsrPlatformOptionsSchema(4-10)npmPlatformOptionsSchema(13-17)
core/multipublish/src/args.ts (2)
core/multipublish/src/util.ts (1)
MODULE_NAME(12-12)core/auto-readme/src/args.ts (1)
parseArgs(36-50)
README.md (1)
core/auto-readme/src/plugin.ts (2)
pkg(121-150)heading(123-149)
core/multipublish/package.json (2)
scripts/generate-examples.js (1)
pkg(39-44)core/auto-readme/build.mjs (1)
build(22-31)
🪛 GitHub Check: Examples / Test
core/multipublish/test.js
[warning] 1-1:
Unexpected console statement. Only these console methods are allowed: warn, error, info
🪛 markdownlint-cli2 (0.18.1)
core/multipublish/README.md
10-10: Heading levels should only increment by one level at a time
Expected: h2; Actual: h5
(MD001, heading-increment)
🔇 Additional comments (27)
.config/.cspell.json (1)
28-28: LGTM!The addition of "multipublish" to the spell checker's ignore list is appropriate and correctly placed in alphabetical order to support the new package introduced in this PR.
.gitignore (1)
13-13: LGTM!The additions appropriately exclude temporary build and runtime artifacts generated by the multipublish workflow.
Also applies to: 20-20
README.md (1)
57-57: LGTM!The multipublish package entry follows the established format and is properly integrated into the packages table.
core/multipublish/typedoc.json (1)
1-12: LGTM!The TypeDoc configuration appropriately targets source files and excludes test and build artifacts.
pnpm-workspace.yaml (1)
44-45: All packages are current with no known vulnerabilities.The catalog additions for
@manypkg/find-root(^3.1.0),@manypkg/get-packages(^3.1.0), andjsr(^0.13.5) are current versions and free from known security vulnerabilities, appropriately supporting the multipublish package's multi-registry publishing functionality.package.json (2)
46-46: LGTM! Consistent use of catalog pattern.Moving @manypkg/get-packages to the catalog specifier aligns with the workspace's centralized dependency management pattern, consistent with other dependencies like prettier, typescript, and vitest.
37-37: No issues found — dotenv-cli removal is complete.Verification confirms that all references to
with-envanddotenv-clihave been removed. The version scriptchangeset version && pnpm install --lockfile-onlycontains only standard package management commands and requires no environment variable wrapper.core/multipublish/tsconfig.json (1)
1-4: LGTM! Standard TypeScript configuration.The configuration properly extends the base TypeScript settings and includes the source directory. This follows the established pattern for packages in the monorepo.
core/multipublish/cli.mjs (1)
1-5: LGTM! Follows established CLI patterns.The CLI entry point follows the same pattern as other packages in the workspace (e.g., core/auto-readme/cli.mjs). The use of dynamic import and the
run()method is consistent with the existing codebase.core/multipublish/src/detect.ts (2)
1-3: LGTM!The type extraction pattern using TypeScript utility types is clean and idiomatic.
5-17: LGTM!The caching strategy and error handling are appropriate for a CLI tool. The bun exclusion is properly placed after caching the result.
core/multipublish/src/index.ts (2)
1-10: LGTM!All imports are properly organized and used in the run function.
19-42: LGTM!The release mapping and publishing orchestration logic is clear and correct. Error handling for missing packages is appropriate.
core/multipublish/src/args.ts (2)
1-6: LGTM!Imports are clean and properly organized.
17-31: LGTM!The parseArgs implementation follows the established pattern in the codebase and correctly enables verbose logging when requested.
core/multipublish/build.mjs (3)
1-11: LGTM!The import setup and initial build steps are properly configured.
21-32: LGTM!The build function configuration follows established patterns in the codebase and provides sensible defaults.
13-19: No action needed. Thez.toJSONSchemamethod is a built-in feature of Zod 4 (documented at https://zod.dev/json-schema) and is correctly used in the code. The import statement on line 4 (import * as z from "zod") correctly exposes this API.Likely an incorrect or invalid review comment.
core/multipublish/src/config.ts (4)
1-6: LGTM!Imports and module-level constants are properly organized.
8-10: LGTM!The default configuration is properly typed and provides a sensible default with experimental JSR generation enabled.
12-20: LGTM!The config loading logic properly uses cosmiconfig with appropriate fallback to defaults and schema validation.
22-30: LGTM!The search places configuration provides comprehensive coverage of common config file locations and formats.
core/multipublish/package.json (1)
28-30: Mismatched types file extension for ESM package.For an ESM package (
"type": "module"), the primary types field typically uses.d.tsor.d.mts. Using.d.ctsfortypeswhilemodulepoints to.jsmay cause TypeScript resolution issues in some configurations.Verify that the build output generates
config/schema.d.ctsand that TypeScript correctly resolves types when importing this package.core/multipublish/src/jsr.ts (2)
3-13: Export schema may not handle all valid package.json export conditions.The schema only handles
import.defaultandrequire.defaultnested structures. Real-world package.json exports often include other conditions liketypes,node,browser, or directimport/requirestrings without thedefaultnesting.Consider whether exports like this should be supported:
{ ".": { "import": "./dist/index.mjs", "require": "./dist/index.cjs" } }If so, the schema needs adjustment to handle both nested and flat condition values.
40-47: LGTM!The
convertPkgJsonExportsToJsrfunction correctly handles both string and object exports, extracting theimport.defaultpath for JSR compatibility.core/multipublish/src/schema.ts (2)
19-19: Verify intent of usingz.inputvsz.inferfor Config type.
z.inputproduces the type before transforms/defaults are applied (fields with.default()become optional).z.inferproduces the output type (fields with defaults become required).If
Configrepresents validated configuration after parsing,z.inferis more appropriate. If it represents user-provided input before parsing,z.inputis correct.Check usage in
config.tsandpublish.tsto confirm whetherConfigis used for pre-parse input or post-parse output.
20-30: LGTM!The
configSchemacorrectly models the platform configuration with support for both simple string literals and tuple configurations with options. The union type structure allows flexible user input while maintaining type safety.
| pnpm install @stephansama/multipublish | ||
| ``` | ||
|
|
||
| ## Usage |
There was a problem hiding this comment.
Add usage documentation for the CLI tool.
The Usage section is currently empty. Please document the basic commands, configuration options, and examples for users to understand how to use this multipublish CLI tool.
Would you like me to help draft initial usage documentation based on the CLI implementation?
🤖 Prompt for AI Agents
In @core/multipublish/README.md at line 25, The README's "Usage" section is
empty; add a concise usage guide that describes the CLI entrypoint and primary
commands, e.g., list commands like "multipublish init", "multipublish publish",
"multipublish status" (or the actual command names implemented in the CLI), list
common flags/options (e.g., --config, --dry-run, --verbose, --target), document
configuration file format and relevant environment variables, and include 2–3
short examples showing typical invocation sequences (init -> configure ->
publish and a dry-run example). Keep the section organized with subheadings
"Commands", "Options", "Configuration", and "Examples" and ensure each command
references the actual function/command names implemented in the codebase so
users can map docs to the CLI implementation.
![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 multipublish package, a utility for publishing packages to multiple registries. The overall structure and approach are solid, leveraging tools like changesets, cosmiconfig, and yargs. However, I've found several critical issues that would prevent the tool from functioning as intended, such as unused command-line arguments, incorrect path handling for JSR publishing, and a hardcoded --dry-run flag. There are also some medium-severity issues related to resource management and configuration handling that should be addressed to improve robustness and usability.
introduces a new `--dry` argument to allow users to perform a dry run of the publishing process without actually publishing packages. this applies to both npm and jsr platforms. this change also includes: - refactoring jsr configuration generation to separate transformation from disk writing, improving flexibility and reducing side effects. - adding `fast-glob` dependency to enhance file globbing capabilities. - enabling `provenance` for all npm publishes for improved security. - enhancing `git clean` command robustness by using `execFileSync` and explicitly marking the filename. - memoizing argument parsing for efficiency. - removing a redundant `test.js` file.
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In @core/multipublish/package.json:
- Line 39: The package.json "detect" npm script references a non-existent file
"./test.js", causing the script to fail; update core/multipublish package.json
by either removing the "detect" script key entirely or changing its command to
point to the correct test file or tool (e.g., replace "node ./test.js" with the
proper script/command used elsewhere) so the "detect" npm script no longer calls
a removed file.
In @core/multipublish/src/args.ts:
- Line 13: The dry-run flag is declared with the wrong type; change the option
definition dry: { alias: "d", description: "Perform a dry run", type: "string" }
in args.ts to use type: "boolean" (i.e., type: "boolean") so CLI parsing yields
a boolean, and then update usages like the coercion in publish.ts (currently
using !!args.dry) to directly use args.dry (or remove the double-bang) so the
code treats the flag as a boolean.
In @core/multipublish/src/publish.ts:
- Around line 30-36: The jsrPublishCommand object has an empty string for the
bun key which can cause silent failures; update jsrPublishCommand (the bun
entry) to use the proper bun publish invocation (mirror the fix used in
npmPublishCommand) by replacing the empty value with the correct command string
for running jsr publish via bun (e.g., the equivalent of "bunx jsr publish" or
the project's established bun invocation), keeping the object typed as satisfies
Record<AgentName, string>.
- Line 66: The JSR config is written unformatted; update the write to use a
pretty-printed JSON string (e.g., JSON.stringify(jsr.config, null, 2)) and
include a trailing newline when calling fsp.writeFile so the file is
human-readable and editor-friendly—replace the call to
fsp.writeFile(jsr.filename, JSON.stringify(jsr.config)) with one that writes the
formatted string (and newline) for jsr.filename using the existing fsp.writeFile
invocation.
- Around line 101-105: The current extraction of scope via
pkg.packageJson.name.split("/").at(0) can yield undefined for empty or malformed
package names; update the scope handling in publish.ts to defensively check that
pkg.packageJson.name is a non-empty string and that .split("/").at(0) returned a
defined value before calling .startsWith("@"); if the name is missing or scope
is undefined, throw a clear error (e.g., "invalid package name" or "missing
scope") or handle it appropriately so the subsequent startsWith check cannot
throw.
- Around line 22-28: The npmPublishCommand map currently contains empty strings
for bun and deno which leads to silent failures when code (e.g., execSync) runs
an empty command; update npmPublishCommand to either provide the correct publish
commands for 'bun' and 'deno' or remove them from the map and, importantly, add
a validation check where the command is used (referencing npmPublishCommand and
the place calling execSync) to throw a clear error like "unsupported package
manager: <agent>" if the resolved command is missing/empty; alternatively filter
unsupported managers during detection so execSync is only ever passed a
non-empty command.
- Line 52: The code mistakenly sets pkgRoot using path.dirname(pkg.dir) which
moves the root up to the workspace; change it so pkgRoot is assigned directly
from pkg.dir (remove the path.dirname call) so loadJsrConfigFile() and any
publish logic (references to pkgRoot and loadJsrConfigFile()) run from the
actual package directory rather than the workspace root.
In @core/multipublish/src/util.ts:
- Around line 25-31: getChangesetReleases currently interpolates tmpFile into a
shell command which risks command injection; change the cp.execSync call to use
execFileSync (or cp.spawnSync) with the command "changeset" and an args array
["status","--output", tmpFile] (i.e., pass tmpFile as an argument rather than
via string interpolation) and remove { stdio: "inherit" } or keep it as-is for
I/O behavior; this ensures the tmpFile is treated as an argument, not a shell
string, preventing shell interpretation.
- Around line 54-57: When no config file is found the code returns { config:
null, filename: configFile } which sets filename to undefined; instead omit the
filename property so it matches the optional filename?: string in the return
type. In the if (!configFile) branch (reference: configFile variable and the
anonymous return object), change the return to just return { config: null } so
callers receive no filename property when none exists.
- Around line 15-23: The chdir function does not restore the original working
directory if callback throws; wrap the callback invocation in a try/finally so
process.chdir(cwd) always runs. Update the chdir function (in util.ts) to
capture cwd, call process.chdir(newDir), then use try { await callback(); }
finally { process.chdir(cwd); } to guarantee restoration even on exceptions.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
core/multipublish/package.jsoncore/multipublish/src/args.tscore/multipublish/src/publish.tscore/multipublish/src/util.ts
🧰 Additional context used
🧬 Code graph analysis (1)
core/multipublish/src/util.ts (2)
core/multipublish/src/types.ts (1)
Changeset(1-1)core/multipublish/src/jsr.ts (2)
JsrSchema(29-29)jsrTransformer(22-27)
🔇 Additional comments (2)
core/multipublish/src/args.ts (1)
9-23: LGTM: Effective memoization pattern.The lazy initialization and caching of parsed arguments is a clean approach that prevents redundant parsing.
core/multipublish/src/util.ts (1)
39-43: LGTM: Secure command execution.The use of
execFileSyncwith an explicit array of arguments prevents command injection vulnerabilities. This is the recommended approach as demonstrated in the PR description's mention of "improves git clean robustness."
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @core/multipublish/package.json:
- Line 39: The "detect" npm script currently runs "./test.js" which mismatches
intent and name; either rename the script file or the npm script for clarity: if
the file implements detection logic, rename test.js to detect.js and update the
"detect" script to "node ./detect.js"; if it is actually a test, rename the
script key to "test" (or remove the entry if unused) and consider moving test.js
into a tests/ folder; update any references to "detect" or "./test.js" elsewhere
to keep names consistent.
- Around line 33-35: The files array entries in package.json currently use a
leading "./" (e.g., "./dist", "./config", "./cli.mjs"); update those entries to
the idiomatic form without the "./" prefix (dist, config, cli.mjs) by editing
the files array in core/multipublish/package.json so each entry is the relative
path without the leading dot-slash.
- Line 58: zod is used at runtime in src/schema.ts and src/jsr.ts but was only
listed as a devDependency; move the "zod": "catalog:schema" entry out of
devDependencies into dependencies in package.json so consumers get zod when
installing; update the dependencies block to include "zod": "catalog:schema" and
remove it from devDependencies, then run a quick install/test to verify imports
in src/schema.ts and src/jsr.ts resolve at runtime.
- Around line 22-30: Update the package export and entry fields so consumers get
the built implementation instead of schema files: change the "exports" root
entry to point "." -> "import": "./dist/index.js" and "require":
"./dist/index.cjs" (keep "./package.json" mapping), set "main" to
"./dist/index.cjs", "module" to "./dist/index.js", and "types" to
"./dist/index.d.ts" so importing the package returns the runtime that exports
the run() function rather than the config/schema.* files.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
core/multipublish/package.json
🔇 Additional comments (1)
core/multipublish/package.json (1)
56-61: Remove the jsr entry from peerDependencies.The
jsrpackage is invoked as a CLI tool through package managers (e.g.,npx jsr publish,pnpm dlx jsr publish) and is not imported as a module in the codebase. It should not be inpeerDependencies. ThedevDependenciesentry is sufficient for development tooling needs.If
jsris required for the package to function at development time, keep it indevDependenciesonly. Otherwise, remove both entries if they're no longer needed.Likely an incorrect or invalid review comment.
| "./dist", | ||
| "./config", | ||
| "./cli.mjs" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing the ./ prefix from files array entries.
While the ./ prefix is valid, it's more idiomatic to omit it in the files array. Standard practice uses relative paths without the prefix.
♻️ Proposed refactor
"files": [
- "./dist",
- "./config",
- "./cli.mjs"
+ "dist",
+ "config",
+ "cli.mjs"
],📝 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.
| "./dist", | |
| "./config", | |
| "./cli.mjs" | |
| "dist", | |
| "config", | |
| "cli.mjs" |
🤖 Prompt for AI Agents
In @core/multipublish/package.json around lines 33 - 35, The files array entries
in package.json currently use a leading "./" (e.g., "./dist", "./config",
"./cli.mjs"); update those entries to the idiomatic form without the "./" prefix
(dist, config, cli.mjs) by editing the files array in
core/multipublish/package.json so each entry is the relative path without the
leading dot-slash.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @core/multipublish/src/index.ts:
- Line 12: The call to parseArgs() in index.ts initializes argument state but
its return value is ignored, which makes the caching pattern opaque; update the
code to either await and use getArgs() consistently or change parseArgs() to
return the parsed args and propagate that value instead of relying on hidden
state. Concretely, replace the unused await parseArgs() pattern by one of: a)
call const args = await getArgs() wherever args are needed, or b) refactor
parseArgs() to return the parsed args and pass that returned value into
downstream functions (referencing parseArgs() and getArgs() to locate the
logic), or at minimum add a short comment next to the await parseArgs() call
explaining the caching behavior so callers understand the implicit state
initialization.
- Around line 37-41: Add progress logging around the nested publish loop: before
calling publishPlatform(pkg, platform) log which package and platform are about
to be published (include pkg.name or pkg identifier and platform), on success
log completion for that package+platform, and on error include the
package/platform context in the error log inside the catch surrounding
publishPlatform; reference the loop over releasedPackages and config.platforms
and the publishPlatform(pkg, platform) call so you add logs immediately
before/after that invocation and in its error handling to give clear progress
and troubleshooting info.
- Around line 37-41: The nested publish loops over releasedPackages and
config.platforms call publishPlatform without error handling, so a single
failure halts all subsequent publishes; wrap each await publishPlatform(pkg,
platform) in a try/catch inside the inner loop (or in a helper like publishOne)
to catch and log the error (including pkg and platform identifiers) and continue
to the next platform/package, and optionally accumulate failures in an array to
return or report after the loops so the process can proceed for remaining items.
In @core/multipublish/src/publish.ts:
- Around line 107-113: The .npmrc append logic in
core/multipublish/src/publish.ts currently concatenates npmrcTemplate into
existingNpmrcFile (see npmrcTemplate usage and writeFile call), which can create
duplicate entries; modify the code to detect and merge existing entries before
writing: either parse existingNpmrcFile (e.g., with an ini parser) and
set/overwrite keys for `${scope}:registry` and `//${host}/:_authToken` then
stringify back, or at minimum check for presence of the specific registry and
auth lines (e.g., search for `${scope}:registry=${config.registry}` and the auth
token key) and only append npmrcTemplate when those lines are missing; ensure
the updated content is what you write with fsp.writeFile.
- Around line 22-36: The exported maps npmPublishCommand and jsrPublishCommand
contain empty-string entries for unsupported agents (e.g., "bun" and "deno")
which can cause confusing behavior; either remove those unsupported keys from
npmPublishCommand/jsrPublishCommand so the maps only include real commands, or
add a runtime guard in the publisher (e.g., inside publishPlatform or wherever
packageManager is resolved) that looks up the command, checks for a falsy value,
and throws a clear error like “Publishing not supported for package manager:
${packageManager}” before attempting to run it.
- Around line 64-65: The JSON is being written without indentation when updating
jsr.config.version and calling fsp.writeFile(jsr.filename,
JSON.stringify(jsr.config)); change the write to serialize with indentation
(e.g., use JSON.stringify(jsr.config, null, 2)) and include a trailing newline
so the file is human-readable and produces cleaner git diffs; update the call
site that writes jsr.filename accordingly.
- Line 105: The code calls new URL(config.registry) which will throw for
malformed registries; wrap the instantiation around a try-catch (surround the
line const { host } = new URL(config.registry) in try { ... } catch (err) { /*
log or rethrow with a clearer message */ }) and handle the error (e.g., return a
user-friendly error or throw a ValidationError) so the publish flow doesn't
crash silently; alternatively, enforce a valid registry earlier by updating
npmPlatformOptionsSchema to use Zod's .url() validator for the registry field so
config.registry is guaranteed valid before publish.ts uses it.
- Around line 100-103: The scope extraction currently uses
pkg.packageJson.name.split("/").at(0) and only checks scope?.startsWith("@");
update validation to first verify that pkg.packageJson.name includes a "/"
(e.g., name.includes("/") or indexOf("/") !== -1) so you only treat names with a
slash as scoped packages, then extract scope (keep using .split("/").at(0) or
split()[0]) and ensure the extracted scope starts with "@"; if the name lacks a
"/", throw a clear error mentioning the package name and the expected
"@scope/package" format (referencing pkg.packageJson.name and scope variables).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
core/multipublish/src/index.tscore/multipublish/src/publish.tscore/multipublish/src/types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
core/multipublish/src/index.ts (3)
core/multipublish/src/args.ts (1)
parseArgs(26-40)core/multipublish/src/config.ts (1)
loadConfig(12-20)core/multipublish/src/publish.ts (1)
publishPlatform(38-147)
core/multipublish/src/publish.ts (3)
core/multipublish/src/detect.ts (2)
AgentName(3-3)detectPackageManager(7-17)core/multipublish/src/schema.ts (3)
Config(19-19)jsrPlatformOptionsSchema(4-10)npmPlatformOptionsSchema(13-17)core/multipublish/src/args.ts (1)
getArgs(22-24)
🔇 Additional comments (4)
core/multipublish/src/types.ts (1)
1-14: LGTM!The type definition is clean and appropriately structures changeset and release data for the multipublish workflow.
core/multipublish/src/publish.ts (2)
17-20: LGTM!The npmrc template correctly formats the registry and auth token configuration.
116-146: LGTM!The package.json strategy, version updates, publish command execution, and cleanup logic are all implemented correctly. Good error handling for unsupported platform types.
core/multipublish/src/index.ts (1)
19-35: LGTM!The package mapping logic is clear and includes proper error handling for missing packages.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @core/multipublish/src/index.ts:
- Line 12: The standalone await getArgs() call in index.ts hides intent; either
capture its return (e.g., const args = await getArgs()) if the parsed args will
be used later, or explicitly document the cache-priming behavior with a short
comment above the call (mentioning getArgs()’s memoization) so future readers
know it’s intentional rather than an accidental unused await.
- Line 1: Remove the shebang line "#!/usr/bin/env node" from the TypeScript
source (the top of index.ts) — it should not be in compiled .ts files — and
ensure the CLI entry script (cli.mjs) contains that shebang instead; simply
delete the shebang from index.ts and add/verify the exact "#!/usr/bin/env node"
line at the top of cli.mjs.
In @core/multipublish/src/util.ts:
- Around line 52-56: The gitClean function currently calls
cp.execFileSync("git", ["clean", "-f", "--", filename], ...) without handling
errors; wrap the call in a try/catch inside gitClean, and in the catch either
(a) rethrow a new Error with contextual details (including filename and the
original error message) if cleanup must be enforced, or (b) log the failure
(using the project logger or console) and return a boolean/void to allow
graceful continuation; ensure you reference and update gitClean's
signature/return accordingly and include clear, contextual error text in the log
or thrown error.
- Around line 28-36: The chdir utility can leave the process in the wrong
working directory if the callback throws; change chdir to save the original cwd,
switch to newDir, then run the callback inside a try block and restore the
original cwd in a finally block so process.chdir(cwd) always runs (use await
callback() to support async callbacks); keep references to the chdir function
and its parameters when making this change.
- Around line 38-44: In getChangesetReleases replace the unnecessary fsp.rm call
that passes recursive: true when removing a single temp file (remove the
recursive option, consider using force: true instead if you need idempotent
removal) and add validation around the JSON parsing: catch JSON.parse errors
and/or validate the parsed object against a Changeset schema (e.g., using a
changesetSchema.parse(parsed) or an explicit shape check) so malformed output is
handled and a clear error is thrown; update getChangesetReleases to read the
file, remove the tmpFile without recursive, then parse and validate before
returning.
- Around line 58-74: In loadJsrConfigFile: stop building the glob with string
concatenation—use path.join(basePath, "{deno,jsr}.json{,c}") to construct the
pattern robustly (import path if needed); after reading and JSON.parse the file,
validate the parsed object against JsrSchema (or run it through the schema
validator/parse function) and throw or return a clear error if validation fails,
and ensure the returned filename is undefined (not the falsy configFile) when no
file is found so the return shape is consistent.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
core/multipublish/src/index.tscore/multipublish/src/util.ts
🧰 Additional context used
🧬 Code graph analysis (2)
core/multipublish/src/index.ts (3)
core/multipublish/src/args.ts (1)
getArgs(22-24)core/multipublish/src/config.ts (1)
loadConfig(12-20)core/multipublish/src/publish.ts (1)
publishPlatform(38-147)
core/multipublish/src/util.ts (1)
core/multipublish/src/jsr.ts (2)
JsrSchema(29-29)jsrTransformer(22-27)
🔇 Additional comments (5)
core/multipublish/src/index.ts (2)
19-35: LGTM! Well-structured release-to-package mapping.The mapping logic correctly matches releases with their corresponding packages and provides clear error messaging when a release references a non-existent package. The data merging using the spread operator appropriately combines package metadata with version information.
37-41: Verify error handling behavior for partial publishing failures.The nested loops publish sequentially without try-catch blocks, meaning the first failure will halt the entire publishing process. Verify that this all-or-nothing behavior is intended. Consider whether partial failures should be handled differently (e.g., collecting errors and reporting all failures at the end, or implementing retry logic).
core/multipublish/src/util.ts (3)
10-26: LGTM! Clear type definitions and constants.The Changeset type accurately models the changeset status output structure, and the module constants are appropriately defined.
46-50: LGTM! Proper temp directory handling.The function correctly ensures the temp directory exists and returns the appropriate path.
76-78: LGTM! Clean transformation using Zod schema.The function appropriately delegates to the jsrTransformer for validation and transformation, ensuring type safety.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @core/multipublish/package.json:
- Around line 32-36: The package.json "files" array currently includes "./dist"
but package.exports doesn’t expose built artifacts; either remove "./dist" from
the "files" array if you don’t intend to publish built output, or add proper
entries in "exports" that point to the compiled outputs in "./dist" (e.g., map
the package entrypoints used by build.mjs / src/index.ts to their corresponding
files in dist and ensure "main"/"module"/"types" or "exports" entries reference
those dist paths) so published packages include and resolve the built files.
- Around line 21-30: The package exports currently point the root "." to the
schema files instead of the library entry; update the "exports" root (".") to
expose the built library at "./dist/index.js" (and "./dist/index.cjs" for
require if needed) so consumers import the package's primary API (e.g., the
run() function), move the schema files to a sub-export like "./schema" that maps
to "./config/schema.js" and "./config/schema.cjs", set "main" to
"./dist/index.cjs" and "module" to "./dist/index.js" to reflect the build
output, and change the "types" entry from ".d.cts" to ".d.ts" to match "type":
"module".
In @core/multipublish/src/util.ts:
- Around line 43-49: The getChangesetReleases function calls fsp.rm(tmpFile, {
recursive: true }) to delete a single temporary file; replace the misleading
recursive option with either fsp.rm(tmpFile) or fsp.rm(tmpFile, { force: true })
to correctly remove a file (use force:true if you want to ignore missing-file
errors). Update the call that references tmpFile in getChangesetReleases
accordingly.
- Around line 28-41: The chdir helper swallows errors in its catch block which
hides callback failures; update the catch in function chdir to log the error
(using console.error or a logger) and then re-throw the caught error (throw e)
so callers can observe failures, keeping the existing finally block to restore
the original cwd after re-throwing.
- Around line 63-79: The function loadJsrConfigFile currently returns
JSON.parse(file) without validating it against the JsrSchema; replace the direct
JSON.parse usage with validation via the Zod schema (import jsrSchema), e.g. use
jsrSchema.safeParse(JSON.parse(file)) and if parsing fails either throw a
descriptive error containing validation.errors or log and return { config: null,
filename: configFile } to preserve the function contract; on success return {
config: result.data, filename: configFile } so callers receive a validated
JsrSchema instance.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
core/multipublish/package.jsoncore/multipublish/src/util.ts
🧰 Additional context used
🧬 Code graph analysis (2)
core/multipublish/package.json (3)
scripts/generate-examples.js (1)
pkg(39-44)core/auto-readme/build.mjs (1)
build(22-31)scripts/meta.js (1)
pkg(9-12)
core/multipublish/src/util.ts (1)
core/multipublish/src/jsr.ts (2)
JsrSchema(29-29)jsrTransformer(22-27)
🔇 Additional comments (6)
core/multipublish/src/util.ts (4)
10-23: LGTM: Well-structured type definition.The Changeset type clearly models the changeset status output with appropriate nested structures for changesets and releases.
51-55: LGTM: Correct implementation.Properly creates the temporary directory with the
recursiveoption and returns the joined path.
57-61: LGTM: Secure implementation with proper argument handling.Uses
execFileSyncto avoid shell injection risks and includes the--separator to prevent filenames from being interpreted as flags.
81-83: LGTM: Proper validation with Zod transformer.Correctly uses
jsrTransformer.parse()to validate and transform the package.json data.core/multipublish/package.json (2)
37-41: Inconsistency: Missing "detect" script mentioned in summary.The AI-generated summary mentions a "detect" script, but it's not present in the
scriptssection. Onlybuild,dev, andlintscripts are defined.
42-66: LGTM! Dependencies and configuration are well-structured.The dependency management using catalog references is appropriate for a monorepo setup. The
jsrpeer dependency aligns with the package's purpose of publishing to JSR. ThepackageManagerspecification andpublishConfigsettings follow best practices.
| "files": [ | ||
| "./dist", | ||
| "./config", | ||
| "./cli.mjs" | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examine build configuration and package.json exports
cd core/multipublish || exit 1
echo "=== package.json exports and files ==="
jq '.exports, .files, .bin' package.json
echo ""
echo "=== build.mjs contents ==="
if [ -f build.mjs ]; then
cat build.mjs
else
echo "build.mjs not found"
fi
echo ""
echo "=== Directory listing ==="
ls -laRepository: stephansama/packages
Length of output: 1723
Remove ./dist from the files array or add corresponding exports.
The dist directory is built by build.mjs (compiling src/index.ts), but the exports configuration doesn't reference it. Either remove ./dist from files if it's not intended to be published, or add exports that point to the built files in dist.
🤖 Prompt for AI Agents
In @core/multipublish/package.json around lines 32 - 36, The package.json
"files" array currently includes "./dist" but package.exports doesn’t expose
built artifacts; either remove "./dist" from the "files" array if you don’t
intend to publish built output, or add proper entries in "exports" that point to
the compiled outputs in "./dist" (e.g., map the package entrypoints used by
build.mjs / src/index.ts to their corresponding files in dist and ensure
"main"/"module"/"types" or "exports" entries reference those dist paths) so
published packages include and resolve the built files.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/multipublish/src/catalog.test.ts`:
- Around line 99-134: The test coverage for updatePackageJsonWithCatalog is
incomplete: add tests for the bun agent path and for devDependencies (and
combined dependencies+devDependencies) so the function's behavior is fully
validated; specifically, add one test that spies on catalogLoadMap.bun to return
a catalog and verifies writeFile updates packageJson.devDependencies when
entries use "catalog:", another that verifies both dependencies and
devDependencies are replaced when both contain "catalog:" entries, and ensure
you import updatePackageJsonWithCatalog and assert fsp.writeFile was called with
the correct JSON stringified package.json in each case.
- Around line 78-96: Update the two tests that currently use bare .toThrow() to
assert specific error messages from loadVersion: replace expect(...).toThrow()
with expect(...).toThrowError(/regex/) or toThrowError("exact message") that
matches the error thrown when a catalog dependency is missing ("catalog
dependency not found" or similar) and when a named catalog is missing ("named
catalog 'missing' not found" or the actual phrasing thrown by loadVersion).
Ensure the assertions reference loadVersion and the dependency/version inputs
used in the tests so failures clearly indicate the intended error case.
♻️ Duplicate comments (1)
core/multipublish/src/catalog.test.ts (1)
111-118: Consider using a partial type instead ofas any.Using
as anybypasses type safety. Consider using a typed mock object.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
core/multipublish/src/catalog.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
core/multipublish/src/catalog.test.ts (1)
core/multipublish/src/catalog.ts (5)
catalogSchema(11-14)CatalogSchema(10-10)loadVersion(40-63)catalogLoadMap(17-20)updatePackageJsonWithCatalog(65-87)
🔇 Additional comments (2)
core/multipublish/src/catalog.test.ts (2)
1-13: LGTM!The imports are properly organized with type imports separated from value imports. The mock setup correctly preserves the original module exports while only stubbing
writeFile, which is the appropriate pattern for partial mocking in vitest.
15-37: LGTM!The schema tests properly cover both catalog patterns: default (
catalog) and named (catalogs). The assertions verify the schema parses and preserves the input correctly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| it("should throw if catalog dependency not found", () => { | ||
| expect(() => { | ||
| loadVersion({ | ||
| catalogs, | ||
| dependency: "bar", | ||
| version: "catalog:", | ||
| }); | ||
| }).toThrow(); | ||
| }); | ||
|
|
||
| it("should throw if named catalog not found", () => { | ||
| expect(() => { | ||
| loadVersion({ | ||
| catalogs, | ||
| dependency: "foo", | ||
| version: "catalog:missing", | ||
| }); | ||
| }).toThrow(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider matching specific error messages for more precise assertions.
The error case tests use bare .toThrow() which passes for any thrown error. For more precise testing, consider matching the expected error message pattern.
Suggested improvement
it("should throw if catalog dependency not found", () => {
expect(() => {
loadVersion({
catalogs,
dependency: "bar",
version: "catalog:",
});
- }).toThrow();
+ }).toThrow(/no catalog version found.*dependency.*bar/);
});
it("should throw if named catalog not found", () => {
expect(() => {
loadVersion({
catalogs,
dependency: "foo",
version: "catalog:missing",
});
- }).toThrow();
+ }).toThrow(/no catalog version found.*named catalog.*missing/);
});📝 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.
| it("should throw if catalog dependency not found", () => { | |
| expect(() => { | |
| loadVersion({ | |
| catalogs, | |
| dependency: "bar", | |
| version: "catalog:", | |
| }); | |
| }).toThrow(); | |
| }); | |
| it("should throw if named catalog not found", () => { | |
| expect(() => { | |
| loadVersion({ | |
| catalogs, | |
| dependency: "foo", | |
| version: "catalog:missing", | |
| }); | |
| }).toThrow(); | |
| }); | |
| it("should throw if catalog dependency not found", () => { | |
| expect(() => { | |
| loadVersion({ | |
| catalogs, | |
| dependency: "bar", | |
| version: "catalog:", | |
| }); | |
| }).toThrow(/no catalog version found.*dependency.*bar/); | |
| }); | |
| it("should throw if named catalog not found", () => { | |
| expect(() => { | |
| loadVersion({ | |
| catalogs, | |
| dependency: "foo", | |
| version: "catalog:missing", | |
| }); | |
| }).toThrow(/no catalog version found.*named catalog.*missing/); | |
| }); |
🤖 Prompt for AI Agents
In `@core/multipublish/src/catalog.test.ts` around lines 78 - 96, Update the two
tests that currently use bare .toThrow() to assert specific error messages from
loadVersion: replace expect(...).toThrow() with
expect(...).toThrowError(/regex/) or toThrowError("exact message") that matches
the error thrown when a catalog dependency is missing ("catalog dependency not
found" or similar) and when a named catalog is missing ("named catalog 'missing'
not found" or the actual phrasing thrown by loadVersion). Ensure the assertions
reference loadVersion and the dependency/version inputs used in the tests so
failures clearly indicate the intended error case.
| describe("updatePackageJsonWithCatalog", () => { | ||
| it("should update package.json for pnpm", async () => { | ||
| const fsp = await import("node:fs/promises"); | ||
| const { catalogLoadMap, updatePackageJsonWithCatalog } = | ||
| await import("./catalog"); | ||
|
|
||
| vi.spyOn(catalogLoadMap, "pnpm").mockResolvedValue({ | ||
| catalog: { | ||
| foo: "1.0.0", | ||
| }, | ||
| }); | ||
|
|
||
| const pkg = { | ||
| dir: "/path/to/pkg", | ||
| packageJson: { | ||
| dependencies: { | ||
| foo: "catalog:", | ||
| }, | ||
| }, | ||
| } as any; | ||
|
|
||
| await updatePackageJsonWithCatalog(pkg, "pnpm"); | ||
|
|
||
| expect(fsp.writeFile).toHaveBeenCalledWith( | ||
| "/path/to/pkg/package.json", | ||
| JSON.stringify( | ||
| { | ||
| dependencies: { | ||
| foo: "1.0.0", | ||
| }, | ||
| }, | ||
| undefined, | ||
| 2, | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider expanding test coverage for updatePackageJsonWithCatalog.
The test only covers:
pnpmagent (butcatalogLoadMapalso hasbun)dependenciesfield (but the function also processesdevDependencies)
Consider adding test cases for:
- The
bunagent path - Packages with
devDependenciescontaining catalog references - Packages with both
dependenciesanddevDependencies
Example additional test case for devDependencies
it("should update devDependencies for pnpm", async () => {
const fsp = await import("node:fs/promises");
const { catalogLoadMap, updatePackageJsonWithCatalog } =
await import("./catalog");
vi.spyOn(catalogLoadMap, "pnpm").mockResolvedValue({
catalog: {
vitest: "2.0.0",
},
});
const pkg = {
dir: "/path/to/pkg",
packageJson: {
devDependencies: {
vitest: "catalog:",
},
},
} as any;
await updatePackageJsonWithCatalog(pkg, "pnpm");
expect(fsp.writeFile).toHaveBeenCalledWith(
"/path/to/pkg/package.json",
JSON.stringify(
{
devDependencies: {
vitest: "2.0.0",
},
},
undefined,
2,
),
);
});🤖 Prompt for AI Agents
In `@core/multipublish/src/catalog.test.ts` around lines 99 - 134, The test
coverage for updatePackageJsonWithCatalog is incomplete: add tests for the bun
agent path and for devDependencies (and combined dependencies+devDependencies)
so the function's behavior is fully validated; specifically, add one test that
spies on catalogLoadMap.bun to return a catalog and verifies writeFile updates
packageJson.devDependencies when entries use "catalog:", another that verifies
both dependencies and devDependencies are replaced when both contain "catalog:"
entries, and ensure you import updatePackageJsonWithCatalog and assert
fsp.writeFile was called with the correct JSON stringified package.json in each
case.
Adds a new configuration option `commitJsrVersionUpdate` to control whether a commit is created when a `jsr.json` file's version is updated during the JSR publishing process. This change also refactors test mocks to improve clarity and maintainability, and introduces the `es-toolkit` dependency for utility functions in tests. The new option `commitJsrVersionUpdate` defaults to `false`.
…lities Adds unit tests for the `chdir`, `gitClean`, and `readStdin` utility functions. The `chdir` test verifies correct directory switching and restoration. The `gitClean` test ensures a specified file is successfully removed. The `readStdin` tests cover both TTY and non-TTY scenarios, with `process.stdin` mocked to simulate input streams for robust testing.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/multipublish/src/catalog.test.ts`:
- Around line 7-20: The mock for catalogLoadMap is incorrectly defined as a
single function, but the real export is an object with bun and pnpm functions;
update the hoisted mocks to make catalogLoadMap an object with bun: vi.fn() and
pnpm: vi.fn() (keep writeFile as vi.fn()) so calls like
updatePackageJsonWithCatalog(..., "pnpm") can access catalogLoadMap["pnpm"];
adjust the vi.mock for "./catalog" to return that object mock.catalogLoadMap
instead of replacing it with a single function.
In `@core/multipublish/src/util.test.ts`:
- Around line 44-52: The test for gitClean is calling the real git binary and
depends on being inside a git repo; mock child_process.execFileSync like in
publish.test.ts (see the mocking at lines that stub execFileSync) so the test
doesn't execute git. Replace the direct call to gitClean(tempFile) with setup
that stubs child_process.execFileSync to a no-op or to return a preset buffer,
then assert execFileSync was called with expected args and that the temp file
removal behavior is verified (or simulate file removal in the mock); ensure the
mock is restored/cleared after the test.
♻️ Duplicate comments (9)
core/multipublish/README.md (3)
10-10: Fix heading level to follow proper hierarchy.The heading jumps from h1 to h5, which violates markdown best practices. Use h2 (
##) instead for proper document structure.📝 Proposed fix
-##### Table of contents +## Table of contents
91-93: Consider more concise wording.The phrase "In order to use" can be simplified to "To use" for brevity.
✏️ Proposed change
-In order to use this with changesets, please update your version script with a preversion script that calls the `multipublish` CLI. +To use this with changesets, update your version script with a preversion script that calls the `multipublish` CLI.
148-148: Fix malformed emphasis markers in generated content.The line contains spaces and backslashes inside emphasis markers (
*or\*anditems.\*) causing rendering issues. If this is auto-generated by a documentation tool, the generator needs adjustment.📝 Proposed fix
-*Array of `'jsr' | 'npm'` *or\* _Tuple:_<ol><li>`'jsr'`</li><li>[JsrPlatformOptions](`#jsrplatformoptions`)</li></ol> _or_ _Tuple:_<ol><li>`'npm'`</li><li>[NpmPlatformOptions](`#npmplatformoptions`)</li></ol> items.\* +*Array of `'jsr' | 'npm'` or Tuple:*<ol><li>`'jsr'`</li><li>[JsrPlatformOptions](`#jsrplatformoptions`)</li></ol> *or Tuple:*<ol><li>`'npm'`</li><li>[NpmPlatformOptions](`#npmplatformoptions`)</li></ol> *items.*core/multipublish/src/catalog.test.ts (2)
85-103: Consider matching specific error messages for more precise assertions.The error case tests use bare
.toThrow()which passes for any thrown error. For more precise testing, consider matching the expected error message pattern to catch regressions in error handling.Suggested improvement
it("should throw if catalog dependency not found", () => { expect(() => { catalog.loadVersion({ catalogs, dependency: "bar", version: "catalog:", }); - }).toThrow(); + }).toThrow(/catalog/i); }); it("should throw if named catalog not found", () => { expect(() => { catalog.loadVersion({ catalogs, dependency: "foo", version: "catalog:missing", }); - }).toThrow(); + }).toThrow(/catalog/i); });
106-141: Test coverage is limited to pnpm with dependencies only.The test correctly validates the happy path for
updatePackageJsonWithCatalogwith pnpm. However, as noted in past reviews, consider expanding coverage to include:
- The
bunagent pathdevDependenciescontaining catalog references- Packages with both
dependenciesanddevDependenciescore/multipublish/src/publish.test.ts (1)
153-185: Consider adding a more specific assertion on the writtenjsr.jsoncontent.The test verifies the file path and command execution but uses
expect.any(String)for the file content. Adding a more specific assertion would catch regressions in the JSR config generation logic.Suggested improvement
expect(mocks.writeFile).toHaveBeenCalledWith( "/path/to/pkg/jsr.json", - expect.any(String), + expect.stringContaining('"name": "@scope/pkg"'), );core/multipublish/package.json (3)
21-30: Fix the main export configuration to expose the package's primary functionality.The
exportsfield currently points to./config/schema.js/schema.cjs, which only exposes schema definitions. Based on the build configuration, the main library code (including therun()function) is built to./dist/index.js. Users importing this package will only get schema types, not the CLI functionality.Additionally, the
typesfield uses.d.cts(CommonJS-specific), which may be incompatible with the package's"type": "module"configuration.📝 Proposed fix
"exports": { ".": { - "import": "./config/schema.js", - "require": "./config/schema.cjs" + "import": "./dist/index.js", + "require": "./dist/index.cjs" }, + "./schema": { + "import": "./config/schema.js", + "require": "./config/schema.cjs" + }, "./package.json": "./package.json" }, - "main": "./config/schema.cjs", - "module": "./config/schema.js", - "types": "./config/schema.d.cts", + "main": "./dist/index.cjs", + "module": "./dist/index.js", + "types": "./dist/index.d.ts",
32-36: Consider removing./prefix from files array entries.While valid, it's more idiomatic to omit the
./prefix in thefilesarray.♻️ Proposed refactor
"files": [ - "./dist", - "./config", - "./cli.mjs" + "dist", + "config", + "cli.mjs" ],
60-61: Consider tightening the jsr peer dependency range.The
">=0.10"constraint still accepts any future version changes. Since jsr is actively developed and currently at 0.13.5, tighten to"^0.13.0"to prevent breaking changes while allowing compatible patch updates.Suggested fix
"peerDependencies": { - "jsr": ">=0.10" + "jsr": "^0.13.0" }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
core/multipublish/README.mdcore/multipublish/package.jsoncore/multipublish/src/catalog.test.tscore/multipublish/src/publish.test.tscore/multipublish/src/util.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
core/multipublish/src/publish.test.ts (1)
core/multipublish/src/publish.ts (1)
publishPlatform(33-168)
core/multipublish/src/util.test.ts (1)
core/multipublish/src/util.ts (4)
chdir(8-21)gitClean(23-27)npmrcTemplate(29-43)readStdin(45-51)
core/multipublish/src/catalog.test.ts (1)
core/multipublish/src/catalog.ts (1)
CatalogSchema(10-10)
🪛 LanguageTool
core/multipublish/README.md
[style] ~91-~91: Consider a more concise word here.
Context: ...ing the config option. ### Changesets In order to use this with changesets, please update...
(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
core/multipublish/README.md
10-10: Heading levels should only increment by one level at a time
Expected: h2; Actual: h5
(MD001, heading-increment)
148-148: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🔇 Additional comments (14)
core/multipublish/README.md (1)
31-101: LGTM! Good documentation coverage.The Usage section now includes comprehensive documentation covering configuration file formats, JSON schema reference, GitHub NPM Registry setup, JSR publishing requirements, and Changesets integration with practical examples.
core/multipublish/src/util.test.ts (5)
1-7: LGTM! Imports are well-structured.The imports correctly bring in es-toolkit's merge for deep merging mock objects, Node.js core modules, Vitest utilities, and the functions under test.
9-24: LGTM! Process stdin mocking is well-implemented.The hoisted mocks pattern with
vi.hoisted()ensures mocks are available before module loading, and themergeutility correctly preserves the original process module while overriding only the stdin properties needed for testing.
26-42: LGTM! The chdir test correctly validates directory switching behavior.The test verifies that:
- The callback executes in the target directory
- The working directory is restored after callback completion
Using
fs.realpathSyncto resolve symlinks ensures reliable path comparisons across platforms.
55-71: LGTM! The npmrcTemplate test validates the expected output format.The test correctly verifies that the template generates properly formatted
.npmrccontent with scope, registry, and auth token placeholders replaced.
74-91: LGTM! The readStdin tests cover both TTY and non-TTY scenarios.The tests correctly verify:
- Returns
nullwhen stdin is a TTY (interactive terminal)- Reads and returns content when stdin is not a TTY (piped input)
The async iterator mock pattern is appropriate for simulating stream input.
core/multipublish/src/catalog.test.ts (2)
22-44: LGTM! Schema parsing tests cover both default and named catalog structures.The tests correctly validate that
catalogSchemaparses:
- Default catalog format (
{ catalog: { ... } })- Named catalogs format (
{ catalogs: { name: { ... } } })
46-83: LGTM! The loadVersion tests cover the main use cases.The tests verify:
- Passthrough when version doesn't use catalog
- Default catalog resolution via
"catalog:"- Named catalog resolution via
"catalog:legacy"core/multipublish/src/publish.test.ts (4)
1-68: LGTM! Comprehensive mock setup for the publish test suite.The mocking strategy is well-organized:
- All mocks are hoisted for consistent availability
- Each module is mocked with appropriate partial overrides
- Internal modules (args, detect, util, jsr) are correctly stubbed to isolate the unit under test
69-79: LGTM! Proper test lifecycle management.The
beforeEachcorrectly sets up default mock return values, andafterEachproperly cleans up with bothvi.resetAllMocks()andvi.unstubAllEnvs(). This addresses the previous concern about environment variable leakage on test failure.
82-118: LGTM! npm package.json strategy test validates the expected behavior.The test correctly verifies:
package.jsonis written withpublishConfig.registryadded- The publish command is executed with
--no-git-checksflag for pnpm
120-151: LGTM! Environment variable handling is now robust.The test correctly uses
vi.stubEnv("NPM_TOKEN", "test-token")instead of directprocess.envmanipulation. Combined withvi.unstubAllEnvs()inafterEach, this ensures proper cleanup even if the test fails.core/multipublish/package.json (2)
42-52: LGTM! Dependencies are correctly specified.The runtime dependencies include all necessary packages for configuration loading (cosmiconfig, yaml), CLI parsing (yargs), package management detection, and schema validation (zod). The catalog-based versioning allows centralized version management.
64-67: LGTM! publishConfig is correctly configured for public publishing.The configuration enables public access and provenance attestation for supply chain security. The local registry URL issue from previous reviews has been addressed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| describe("gitClean", () => { | ||
| it("should remove the specified file", () => { | ||
| const tempFile = "dummy-file-for-testing.tmp"; | ||
| fs.writeFileSync(tempFile, "delete me"); | ||
|
|
||
| gitClean(tempFile); | ||
|
|
||
| expect(fs.existsSync(tempFile)).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the util.ts file and check gitClean implementation
fd "util.ts" core/multipublish/src --type fRepository: stephansama/packages
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Get content of util.test.ts to see the full test and surrounding context
cat -n core/multipublish/src/util.test.ts | head -100Repository: stephansama/packages
Length of output: 3024
🏁 Script executed:
#!/bin/bash
# Get content of util.ts to see gitClean implementation
cat -n core/multipublish/src/util.tsRepository: stephansama/packages
Length of output: 1632
🏁 Script executed:
#!/bin/bash
# Check for test setup files and vitest configuration
fd "vitest\|test\|setup" core/multipublish --type f -e ts -e js -e json | grep -E "(setup|config|vitest)" | head -20Repository: stephansama/packages
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check for CI configuration and see how tests are run
fd "\.yml\|\.yaml\|package\.json" --type f | head -10Repository: stephansama/packages
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check if there are other git-related tests to see patterns
rg "gitClean\|execFileSync" core/multipublish --type ts -A 3 -B 3Repository: stephansama/packages
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for package.json and config files at root
ls -la | grep -E "package\.json|vitest|jest|tsconfig"Repository: stephansama/packages
Length of output: 268
🏁 Script executed:
#!/bin/bash
# Search more broadly for gitClean usage in entire repo
rg "gitClean" --type tsRepository: stephansama/packages
Length of output: 864
🏁 Script executed:
#!/bin/bash
# Check if test runs in a git repo context (look for git-related test setup)
rg "git\|execFileSync" core/multipublish/src --type ts -B 2 -A 2Repository: stephansama/packages
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check the vitest config to see if there are any global setup considerations
cat vitest.config.tsRepository: stephansama/packages
Length of output: 405
🏁 Script executed:
#!/bin/bash
# Look at how publish.test.ts mocks gitClean to understand the pattern
rg "gitClean" core/multipublish/src/publish.test.ts -B 5 -A 5Repository: stephansama/packages
Length of output: 277
🏁 Script executed:
#!/bin/bash
# Check if there's any beforeEach or global setup that ensures git repo context
cat -n core/multipublish/src/util.test.ts | grep -E "beforeEach|beforeAll|afterEach|afterAll|setup"Repository: stephansama/packages
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check the entire publish.test.ts to see the mocking pattern more clearly
head -100 core/multipublish/src/publish.test.ts | cat -nRepository: stephansama/packages
Length of output: 2989
Test lacks isolation from git environment and should mock execFileSync.
This test creates a real file and calls gitClean which executes an actual git clean -f command. The test will fail if git is unavailable or not in a git repository, making it fragile and unsuitable for isolated test runs. Unlike the mocking pattern already established in publish.test.ts, this test has no isolation from the environment.
Mock child_process.execFileSync (as done in publish.test.ts lines 29-32) to isolate the test from external dependencies.
🤖 Prompt for AI Agents
In `@core/multipublish/src/util.test.ts` around lines 44 - 52, The test for
gitClean is calling the real git binary and depends on being inside a git repo;
mock child_process.execFileSync like in publish.test.ts (see the mocking at
lines that stub execFileSync) so the test doesn't execute git. Replace the
direct call to gitClean(tempFile) with setup that stubs
child_process.execFileSync to a no-op or to return a preset buffer, then assert
execFileSync was called with expected args and that the temp file removal
behavior is verified (or simulate file removal in the mock); ensure the mock is
restored/cleared after the test.
adds comprehensive unit tests for the main `run` function within the multipublish package. these tests verify the correct execution flow and function calls based on the `versionJsr` argument, ensuring that either platform publishing or jsr config updates are triggered as expected. all external dependencies are mocked to ensure isolated and reliable testing.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/multipublish/src/index.test.ts`:
- Around line 38-70: Add tests for the run() iteration logic by mocking
getPackages to return multiple package objects and getArgs to include multiple
platforms (e.g., ["npm","jsr"]), then assert publishPlatform is invoked for each
package-platform pair (check call count and that each call receives the correct
packageJson/name and platform string) and that updateJsrConfigVersion is invoked
for each package when args.versionJsr is true; use the existing mocks
(getPackages, getArgs, publishPlatform, updateJsrConfigVersion) and
afterEach(vi.clearAllMocks) pattern to isolate tests and verify there are no
off-by-one or early-exit issues.
- Around line 62-69: The test only checks that updateJsrConfigVersion was
called; enhance it to verify the actual arguments by preparing the expected
package data (or deriving it from the existing test fixtures), then assert
mocks.updateJsrConfigVersion was called with that data after await run(); keep
the existing mocks.getArgs.mockResolvedValueOnce({ versionJsr: true }) and also
assert mocks.publishPlatform was not called to preserve current behavior, e.g.
add an
expect(mocks.updateJsrConfigVersion).toHaveBeenCalledWith(expectedPackageData)
using the same package shape the code should pass.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
core/multipublish/src/index.test.ts
🔇 Additional comments (3)
core/multipublish/src/index.test.ts (3)
1-4: LGTM!Imports are appropriate for a vitest unit test file.
5-25: LGTM!Good use of
vi.hoistedto ensure mocks are defined before module resolution. The mock data structure is comprehensive and covers the necessary dependencies for testing therunfunction flow.
27-36: LGTM!Module mocking is properly set up for both external dependencies and internal modules.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| describe("run", () => { | ||
| afterEach(vi.clearAllMocks); | ||
|
|
||
| it("should call necessary functions and publish platforms", async () => { | ||
| await run(); | ||
|
|
||
| expect(mocks.findRoot).toHaveBeenCalledOnce(); | ||
| expect(mocks.getArgs).toHaveBeenCalledOnce(); | ||
| expect(mocks.loadConfig).toHaveBeenCalledOnce(); | ||
| expect(mocks.getPackages).toHaveBeenCalledOnce(); | ||
| expect(mocks.loadReleases).toHaveBeenCalledOnce(); | ||
| expect(mocks.updateJsrConfigVersion).not.toHaveBeenCalled(); | ||
| expect(mocks.publishPlatform).toHaveBeenCalledOnce(); | ||
| expect(mocks.publishPlatform).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| packageJson: expect.objectContaining({ | ||
| name: "pkg1", | ||
| version: "1.0.0", | ||
| }), | ||
| }), | ||
| "npm", | ||
| ); | ||
| }); | ||
|
|
||
| it("should call updateJsrConfigVersion if args.versionJsr is true", async () => { | ||
| mocks.getArgs.mockResolvedValueOnce({ versionJsr: true }); | ||
|
|
||
| await run(); | ||
|
|
||
| expect(mocks.updateJsrConfigVersion).toHaveBeenCalledOnce(); | ||
| expect(mocks.publishPlatform).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding tests for multiple packages and platforms.
The current tests cover the basic flow with a single package and platform. To ensure the iteration logic in run() works correctly, consider adding tests that:
- Mock multiple packages to verify
publishPlatformandupdateJsrConfigVersionare called for each package. - Mock multiple platforms (e.g.,
["npm", "jsr"]) to verify publishing occurs for each platform.
This would help catch off-by-one errors or early-exit bugs in the iteration logic.
🤖 Prompt for AI Agents
In `@core/multipublish/src/index.test.ts` around lines 38 - 70, Add tests for the
run() iteration logic by mocking getPackages to return multiple package objects
and getArgs to include multiple platforms (e.g., ["npm","jsr"]), then assert
publishPlatform is invoked for each package-platform pair (check call count and
that each call receives the correct packageJson/name and platform string) and
that updateJsrConfigVersion is invoked for each package when args.versionJsr is
true; use the existing mocks (getPackages, getArgs, publishPlatform,
updateJsrConfigVersion) and afterEach(vi.clearAllMocks) pattern to isolate tests
and verify there are no off-by-one or early-exit issues.
| it("should call updateJsrConfigVersion if args.versionJsr is true", async () => { | ||
| mocks.getArgs.mockResolvedValueOnce({ versionJsr: true }); | ||
|
|
||
| await run(); | ||
|
|
||
| expect(mocks.updateJsrConfigVersion).toHaveBeenCalledOnce(); | ||
| expect(mocks.publishPlatform).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider verifying the arguments passed to updateJsrConfigVersion.
The test asserts that updateJsrConfigVersion is called, but doesn't verify it receives the correct package data. This would strengthen the test by ensuring the function is called with the expected arguments.
♻️ Suggested improvement
it("should call updateJsrConfigVersion if args.versionJsr is true", async () => {
mocks.getArgs.mockResolvedValueOnce({ versionJsr: true });
await run();
expect(mocks.updateJsrConfigVersion).toHaveBeenCalledOnce();
+ expect(mocks.updateJsrConfigVersion).toHaveBeenCalledWith(
+ expect.objectContaining({
+ packageJson: expect.objectContaining({
+ name: "pkg1",
+ version: "1.0.0",
+ }),
+ }),
+ );
expect(mocks.publishPlatform).not.toHaveBeenCalled();
});📝 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.
| it("should call updateJsrConfigVersion if args.versionJsr is true", async () => { | |
| mocks.getArgs.mockResolvedValueOnce({ versionJsr: true }); | |
| await run(); | |
| expect(mocks.updateJsrConfigVersion).toHaveBeenCalledOnce(); | |
| expect(mocks.publishPlatform).not.toHaveBeenCalled(); | |
| }); | |
| it("should call updateJsrConfigVersion if args.versionJsr is true", async () => { | |
| mocks.getArgs.mockResolvedValueOnce({ versionJsr: true }); | |
| await run(); | |
| expect(mocks.updateJsrConfigVersion).toHaveBeenCalledOnce(); | |
| expect(mocks.updateJsrConfigVersion).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| packageJson: expect.objectContaining({ | |
| name: "pkg1", | |
| version: "1.0.0", | |
| }), | |
| }), | |
| ); | |
| expect(mocks.publishPlatform).not.toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
In `@core/multipublish/src/index.test.ts` around lines 62 - 69, The test only
checks that updateJsrConfigVersion was called; enhance it to verify the actual
arguments by preparing the expected package data (or deriving it from the
existing test fixtures), then assert mocks.updateJsrConfigVersion was called
with that data after await run(); keep the existing
mocks.getArgs.mockResolvedValueOnce({ versionJsr: true }) and also assert
mocks.publishPlatform was not called to preserve current behavior, e.g. add an
expect(mocks.updateJsrConfigVersion).toHaveBeenCalledWith(expectedPackageData)
using the same package shape the code should pass.
this commit introduces and configures @stephansama/multipublish to manage package releases across multiple platforms, specifically jsr and npm. the changes include: - adding a new `.config/.multipublishrc.json` file to define publishing platforms and their respective configurations. - updating the `release.yml` github actions workflow to pipe the list of published packages from `changesets` directly into `multipublish` for processing. this ensures that `multipublish` handles the actual deployment to the configured registries. - granting `packages: write` permission in the github actions workflow, which is necessary for publishing to npm and jsr registries. - adding `@stephansama/multipublish` as a dev dependency to the workspace.
the exportSchema in jsr.ts now supports direct string values for the import and require fields within an export map entry. previously, these fields were strictly expected to be objects containing a default property. the convertPkgJsonExportsToJsr function has been updated to correctly handle both the string and object formats when converting package.json exports to the jsr format. additionally, the catalogLoadMap mock in catalog.test.ts was refactored to an object, enabling package manager-specific mock implementations for 'bun' and 'pnpm'. this improves the flexibility and isolation of tests.
The core/multipublish/cli.mjs script has been made executable to allow direct invocation. The @stephansama/multipublish package was moved from devDependencies to dependencies to ensure its availability for project build and publishing workflows.
The `commitJsrVersionUpdate` configuration option has been removed from the JSR platform options. This change simplifies the configuration by consolidating the version commit behavior under the `--versionJsr` command-line flag. The README has been updated to reflect this new approach.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.config/.multipublishrc.json:
- Around line 11-17: Update the multipublish config to use the public npm
registry auth variable: change the tokenEnvironmentKey value from "GITHUB_TOKEN"
to "NODE_AUTH_TOKEN" so the registry entry that points to
"https://registry.npmjs.org/" uses the correct authentication variable expected
by multipublish and your CI workflow.
In @.github/workflows/release.yml:
- Around line 77-80: Change the GitHub Actions step named "f" to a descriptive
name that reflects its purpose (for example, replace name: f with name: "Publish
to multiple platforms" or "Publish multiple packages"), leaving the if condition
and run block (steps.changesets.outputs.published and the echo ... |
multipublish command) unchanged so the step intent remains clear while improving
readability.
In `@core/multipublish/cli.mjs`:
- Around line 1-5: The bootstrap script currently imports "./dist/index.js" and
calls mod.run() without handling failures, so add a rejection handler to the
dynamic import and the run invocation: wrap import("./dist/index.js") promise
chain with .catch to log a clear error (including the error object) and exit
non‑zero, and also ensure any thrown/rejected value from mod.run() is caught and
reported (either by awaiting/then/catch on mod.run() or handling its returned
promise), referencing the import("./dist/index.js") call and mod.run()
invocation.
In `@core/multipublish/src/jsr.ts`:
- Around line 51-65: The loadConfig function currently reads configFile and uses
JSON.parse which will throw on .jsonc files matched by the glob; update
loadConfig to detect/read configFile content and parse it with a JSONC-aware
parser (e.g., use jsonc-parser or a small strip-comments helper) instead of
JSON.parse, then pass the result to jsrSchema.parse; also catch parse errors and
rethrow or wrap them with a descriptive message that includes the filename and
original error to aid debugging (refer to loadConfig, configFile, files, and
jsrSchema.parse).
♻️ Duplicate comments (6)
core/multipublish/src/catalog.test.ts (2)
85-103: Consider matching specific error messages for more precise assertions.The error case tests use bare
.toThrow()which passes for any thrown error. For more precise testing, consider matching the expected error message pattern.
106-143: Consider expanding test coverage forupdatePackageJsonWithCatalog.The test only covers the
pnpmagent withdependencies. Consider adding tests for:
- The
bunagent path- Packages with
devDependenciescontaining catalog referencescore/multipublish/src/jsr.ts (4)
22-27: Version validation was flagged in a previous review.The suggestion to add
z.string().min(1)or semver validation for theversionfield remains applicable.
38-49: Type inference ordering was flagged in a previous review.The suggestion to reorder the type alias after the schema definition remains applicable.
67-80: Input mutation was flagged in a previous review.The suggestion to document the mutation behavior or return a modified copy remains applicable.
99-110: Defensive handling was flagged in a previous review.The suggestion to add fallback to
value.require?.defaultwhenvalue.importis unavailable remains applicable for robustness against edge cases.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.config/.multipublishrc.json.github/workflows/release.ymlcore/multipublish/cli.mjscore/multipublish/src/catalog.test.tscore/multipublish/src/jsr.tspackage.json
🧰 Additional context used
🧬 Code graph analysis (2)
core/multipublish/src/catalog.test.ts (1)
core/multipublish/src/catalog.ts (1)
CatalogSchema(10-10)
core/multipublish/src/jsr.ts (1)
core/multipublish/src/schema.ts (1)
JsrPlatformOptionsSchema(3-3)
🔇 Additional comments (9)
.github/workflows/release.yml (1)
19-19: LGTM:packages: writepermission added.This permission is correctly scoped for publishing to GitHub Packages/npm registry.
package.json (1)
46-49: LGTM: Workspace dependencies updated correctly.The
catalog:specifier for@manypkg/get-packagesandworkspace:*for the new@stephansama/multipublishpackage are appropriate for pnpm workspace integration..config/.multipublishrc.json (1)
1-10: LGTM: JSR platform configuration.The experimental flags for catalog updates and JSR generation are appropriately configured.
core/multipublish/src/catalog.test.ts (2)
7-20: LGTM: Mock structure correctly updated.The
catalogLoadMapmock now properly reflects the object structure withbunandpnpmfunction properties, addressing the previous critical issue.
22-44: LGTM: Schema parsing tests.Tests correctly validate both default
catalogand namedcatalogsparsing behavior.core/multipublish/src/jsr.ts (4)
1-8: LGTM!Imports are appropriately scoped and necessary for the module's functionality.
9-20: LGTM!The
exportSchemacorrectly handles the various forms of package.json exports: simple string paths, record of strings, and nested conditional export objects withimport/requirekeys.
29-36: LGTM!The transformer correctly maps validated
package.jsonfields to the JSR configuration format.
82-97: The glob pattern allows.jsoncfiles, butJSON.parseon line 64 will fail on JSONC with comments.The
loadConfigfunction used byupdateJsrConfigVersionexplicitly searches for{deno,jsr}.json{,c}(line 52), suggesting.jsoncsupport was intended. However,JSON.parse(file)on line 64 does not support JSONC comments and will throw an error if a.jsoncfile contains comments. This meansupdateJsrConfigVersionwill crash before the write operation, not silently strip comments. Consider using a JSONC parser (likejsoncor addingparse5) to properly support the glob pattern, or restrict the glob to.jsononly if JSONC support is not needed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| [ | ||
| "npm", | ||
| { | ||
| "registry": "https://registry.npmjs.org/", | ||
| "tokenEnvironmentKey": "GITHUB_TOKEN" | ||
| } | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the multipublish source code and understand how tokenEnvironmentKey is used
find . -type f -name "*.json" -o -name "*.ts" -o -name "*.js" | grep -E "(multipublish|publish)" | head -20Repository: stephansama/packages
Length of output: 766
🏁 Script executed:
# Check the workflow file to see what tokens are set
fd -t f -e "yml" -e "yaml" | xargs grep -l "NODE_AUTH_TOKEN\|GITHUB_TOKEN" 2>/dev/null | head -10Repository: stephansama/packages
Length of output: 46
🏁 Script executed:
# Search for how tokenEnvironmentKey is consumed in the codebase
rg -n "tokenEnvironmentKey" --type-list --type ts --type jsRepository: stephansama/packages
Length of output: 453
🏁 Script executed:
# Search for tokenEnvironmentKey usage
rg -n "tokenEnvironmentKey" --type ts --type jsRepository: stephansama/packages
Length of output: 427
🏁 Script executed:
# Find workflow files
fd -t f \( -name "*.yml" -o -name "*.yaml" \) | head -20Repository: stephansama/packages
Length of output: 294
🏁 Script executed:
# Check the config file to understand the structure
cat .config/.multipublishrc.jsonRepository: stephansama/packages
Length of output: 384
🏁 Script executed:
# Find workflow files with correct syntax
find . -name "*.yml" -o -name "*.yaml" | grep -i workflowRepository: stephansama/packages
Length of output: 95
🏁 Script executed:
# Also look in .github/workflows
fd -type f -path "*/.github/workflows/*"Repository: stephansama/packages
Length of output: 234
🏁 Script executed:
# Check the publish.ts to understand token usage better
sed -n '105,115p' core/multipublish/src/publish.tsRepository: stephansama/packages
Length of output: 387
🏁 Script executed:
# Check the release workflow to see what tokens are configured
cat .github/workflows/release.ymlRepository: stephansama/packages
Length of output: 2499
🏁 Script executed:
# Check git history for the config file to see if there's context about GITHUB_TOKEN choice
git log --oneline -20 .config/.multipublishrc.jsonRepository: stephansama/packages
Length of output: 127
🏁 Script executed:
# Search for comments about GITHUB_TOKEN or npm token usage in the codebase
rg -n "GITHUB_TOKEN|npm.*token|token.*npm" -i core/multipublish/src/ --context 3Repository: stephansama/packages
Length of output: 1884
🏁 Script executed:
# Check if there's documentation about multipublish token configuration
find . -name "*.md" -o -name "README*" | xargs grep -l "multipublish\|tokenEnvironmentKey" 2>/dev/nullRepository: stephansama/packages
Length of output: 141
🏁 Script executed:
# Check multipublish README for token documentation
cat core/multipublish/README.mdRepository: stephansama/packages
Length of output: 4920
🏁 Script executed:
# Also check the main README
grep -A 10 -B 5 "multipublish\|tokenEnvironmentKey\|GITHUB_TOKEN\|NODE_AUTH_TOKEN" README.mdRepository: stephansama/packages
Length of output: 4851
Change tokenEnvironmentKey to match the public npm registry authentication.
The config uses GITHUB_TOKEN for registry.npmjs.org, but per the multipublish documentation, GITHUB_TOKEN is intended only for the GitHub NPM Registry (npm.pkg.github.com). The public npm registry requires NODE_AUTH_TOKEN, which is the schema default and matches the workflow setup.
🐛 Suggested fix
[
"npm",
{
"registry": "https://registry.npmjs.org/",
- "tokenEnvironmentKey": "GITHUB_TOKEN"
+ "tokenEnvironmentKey": "NODE_AUTH_TOKEN"
}
]📝 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.
| [ | |
| "npm", | |
| { | |
| "registry": "https://registry.npmjs.org/", | |
| "tokenEnvironmentKey": "GITHUB_TOKEN" | |
| } | |
| ] | |
| [ | |
| "npm", | |
| { | |
| "registry": "https://registry.npmjs.org/", | |
| "tokenEnvironmentKey": "NODE_AUTH_TOKEN" | |
| } | |
| ] |
🤖 Prompt for AI Agents
In @.config/.multipublishrc.json around lines 11 - 17, Update the multipublish
config to use the public npm registry auth variable: change the
tokenEnvironmentKey value from "GITHUB_TOKEN" to "NODE_AUTH_TOKEN" so the
registry entry that points to "https://registry.npmjs.org/" uses the correct
authentication variable expected by multipublish and your CI workflow.
| #!/usr/bin/env node | ||
|
|
||
| "use strict"; | ||
|
|
||
| import("./dist/index.js").then((mod) => mod.run()); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding error handling for import/run failures.
If the build hasn't been run or run() throws, users will see an uncaught promise rejection without helpful context. Consider catching and reporting errors gracefully:
♻️ Suggested improvement
#!/usr/bin/env node
"use strict";
-import("./dist/index.js").then((mod) => mod.run());
+import("./dist/index.js")
+ .then((mod) => mod.run())
+ .catch((err) => {
+ console.error("multipublish failed:", err.message);
+ process.exitCode = 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.
| #!/usr/bin/env node | |
| "use strict"; | |
| import("./dist/index.js").then((mod) => mod.run()); | |
| #!/usr/bin/env node | |
| "use strict"; | |
| import("./dist/index.js") | |
| .then((mod) => mod.run()) | |
| .catch((err) => { | |
| console.error("multipublish failed:", err.message); | |
| process.exitCode = 1; | |
| }); |
🤖 Prompt for AI Agents
In `@core/multipublish/cli.mjs` around lines 1 - 5, The bootstrap script currently
imports "./dist/index.js" and calls mod.run() without handling failures, so add
a rejection handler to the dynamic import and the run invocation: wrap
import("./dist/index.js") promise chain with .catch to log a clear error
(including the error object) and exit non‑zero, and also ensure any
thrown/rejected value from mod.run() is caught and reported (either by
awaiting/then/catch on mod.run() or handling its returned promise), referencing
the import("./dist/index.js") call and mod.run() invocation.
| export async function loadConfig(basePath: string) { | ||
| const files = await fg(basePath + "/{deno,jsr}.json{,c}"); | ||
| if (files.length > 1) { | ||
| throw new Error("please only have one deno or jsr configuration file"); | ||
| } | ||
|
|
||
| const configFile = files.at(0); | ||
| if (!configFile) { | ||
| console.info("no jsr config file found"); | ||
| return { config: null, filename: undefined }; | ||
| } | ||
|
|
||
| const file = await fsp.readFile(configFile, { encoding: "utf8" }); | ||
| return { config: jsrSchema.parse(JSON.parse(file)), filename: configFile }; | ||
| } |
There was a problem hiding this comment.
JSONC files cannot be parsed with JSON.parse().
The glob pattern on line 52 matches .jsonc files ({deno,jsr}.json{,c}), but JSON.parse() on line 64 cannot handle JSON with comments or trailing commas. This will throw a SyntaxError for valid .jsonc config files.
Use a JSONC-aware parser like jsonc-parser or strip comments before parsing.
🔧 Proposed fix using jsonc-parser
+import { parse as parseJsonc } from "jsonc-parser";
+
export async function loadConfig(basePath: string) {
const files = await fg(basePath + "/{deno,jsr}.json{,c}");
if (files.length > 1) {
throw new Error("please only have one deno or jsr configuration file");
}
const configFile = files.at(0);
if (!configFile) {
console.info("no jsr config file found");
return { config: null, filename: undefined };
}
const file = await fsp.readFile(configFile, { encoding: "utf8" });
- return { config: jsrSchema.parse(JSON.parse(file)), filename: configFile };
+ try {
+ const parsed = parseJsonc(file);
+ return { config: jsrSchema.parse(parsed), filename: configFile };
+ } catch (e) {
+ throw new Error(`Failed to parse config file: ${configFile}`, { cause: e });
+ }
}Additionally, the previous review flagged that JSON.parse errors should be wrapped with a descriptive message—this remains applicable.
🤖 Prompt for AI Agents
In `@core/multipublish/src/jsr.ts` around lines 51 - 65, The loadConfig function
currently reads configFile and uses JSON.parse which will throw on .jsonc files
matched by the glob; update loadConfig to detect/read configFile content and
parse it with a JSONC-aware parser (e.g., use jsonc-parser or a small
strip-comments helper) instead of JSON.parse, then pass the result to
jsrSchema.parse; also catch parse errors and rethrow or wrap them with a
descriptive message that includes the filename and original error to aid
debugging (refer to loadConfig, configFile, files, and jsrSchema.parse).
renamed the "JSR" section to "JSR Configuration" for improved clarity regarding setup requirements. added new subsections under "Changesets" to provide detailed guidance: - "JSR": updated the `preversion` script example to include `--useChangesetStatus` for proper JSR versioning. - "Published packages": included a github actions workflow snippet demonstrating how to run `multipublish` to target other registries immediately following a `changeset publish` step.
1 participant
Checklist
mainhave been mergedmain