Add simple mode version to github releases#560
Add simple mode version to github releases#560oleole39 wants to merge 4 commits intoalam00000:mainfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
MrFreePress
left a comment
MrFreePress
left a comment
There was a problem hiding this comment.
Not approving yet. This release workflow change is reasonable in principle, but the PR explicitly says it has not been tested. Untested release-pipeline changes should not merge blind.
I assume such test is to be performed by the core team? |
|
@oleole39 That's a bot don't mind it. I have left two reviews. Also BUILD_OUTPUT_DIR is read in the dev server middleware paths in vite.config.ts where it'll never be set. I also suggest you to run the workflow on your own branch |
|
Hello @alam00000,
Would you mean "review comments"? I can't find them, did I miss something?
Not sure to get this point. In this PR,
|
📝 WalkthroughWalkthroughCI and build tooling were changed to support a configurable build output directory (default Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as CI Runner
participant Build as Vite / npm scripts
participant Pack as scripts/package-dist.js
participant Release as GitHub Release
GH->>Runner: start build-and-publish job
Runner->>Build: run build (BUILD_OUTPUT_DIR=dist)
Build-->>Runner: produce ./dist
Runner->>Pack: package ./dist -> dist-<version>.zip
Runner->>Build: run build (BUILD_OUTPUT_DIR=dist-simple, SIMPLE_MODE=true)
Build-->>Runner: produce ./dist-simple
Runner->>Pack: package ./dist-simple -> dist-simple-<version>.zip
Runner->>Release: upload files: dist-<version>.zip, dist-simple-<version>.zip
Release-->>GH: release assets uploaded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/build-and-publish.yml (1)
32-37: Consider removing duplicate build work in CI.You currently build twice explicitly, then
npm run packagebuilds again for each package step. That’s extra runtime and increases chance of env drift between steps.Also applies to: 44-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-and-publish.yml around lines 32 - 37, The workflow currently runs two explicit build steps ("Build full distribution" running `npm run build` and "Build simple distribution" running `SIMPLE_MODE=true BUILD_OUTPUT_DIR=dist-simple npm run build`) and later runs `npm run package` which itself rebuilds, causing duplicate work; remove the redundant explicit build steps or convert them to a single build that produces reusable artifacts (upload/download or set outputs) that `npm run package` can consume so packaging steps don't rebuild — update or remove the steps named "Build full distribution" and "Build simple distribution" and ensure `npm run package` runs against the produced artifacts (or disable its internal build) to avoid double-building.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-and-publish.yml:
- Around line 47-48: The "Package simple distribution" step re-runs the build
without simple mode and can overwrite dist-simple; set SIMPLE_MODE=true when
invoking the package/build step so the rebuild produces simple-mode assets (e.g.
run the package step with SIMPLE_MODE=true and BUILD_OUTPUT_DIR=dist-simple) to
ensure the zip contains simple-mode output and not default-mode files.
---
Nitpick comments:
In @.github/workflows/build-and-publish.yml:
- Around line 32-37: The workflow currently runs two explicit build steps
("Build full distribution" running `npm run build` and "Build simple
distribution" running `SIMPLE_MODE=true BUILD_OUTPUT_DIR=dist-simple npm run
build`) and later runs `npm run package` which itself rebuilds, causing
duplicate work; remove the redundant explicit build steps or convert them to a
single build that produces reusable artifacts (upload/download or set outputs)
that `npm run package` can consume so packaging steps don't rebuild — update or
remove the steps named "Build full distribution" and "Build simple distribution"
and ensure `npm run package` runs against the produced artifacts (or disable its
internal build) to avoid double-building.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d5b3b26-a560-4bcc-b6e6-08bf5e71c615
📒 Files selected for processing (7)
.github/workflows/build-and-publish.yml.gitignoreeslint.config.mjsscripts/package-dist.jstsconfig.jsonvite.config.tsvitest.config.ts
| - name: Package simple distribution | ||
| run: BUILD_OUTPUT_DIR=dist-simple npm run package |
There was a problem hiding this comment.
Set SIMPLE_MODE=true during simple packaging, or the “simple” zip can contain default-mode output.
npm run package triggers a build again, so at Line 48 the rebuild runs without simple mode and can overwrite dist-simple with default-mode assets before zipping.
✅ Minimal fix
- - name: Package simple distribution
- run: BUILD_OUTPUT_DIR=dist-simple npm run package
+ - name: Package simple distribution
+ run: SIMPLE_MODE=true BUILD_OUTPUT_DIR=dist-simple npm run package📝 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.
| - name: Package simple distribution | |
| run: BUILD_OUTPUT_DIR=dist-simple npm run package | |
| - name: Package simple distribution | |
| run: SIMPLE_MODE=true BUILD_OUTPUT_DIR=dist-simple npm run package |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-and-publish.yml around lines 47 - 48, The "Package
simple distribution" step re-runs the build without simple mode and can
overwrite dist-simple; set SIMPLE_MODE=true when invoking the package/build step
so the rebuild produces simple-mode assets (e.g. run the package step with
SIMPLE_MODE=true and BUILD_OUTPUT_DIR=dist-simple) to ensure the zip contains
simple-mode output and not default-mode files.
oleole39
force-pushed
the
release_simple_mode_version
branch
from
2b333b8 to
526d644
Compare
|
I've tested the workflow on my fork, at least the As you will notice, build and packaging steps work fine. Still you will want to check the review comments left open in this PR.
|
oleole39
force-pushed
the
release_simple_mode_version
branch
from
526d644 to
57a7f47
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/package-dist.js (1)
11-11: ConstrainBUILD_OUTPUT_DIRto expected values.At Line 11, any directory name is accepted. With Line 61 archiving that directory, a typo or bad env value could package unintended content. Consider whitelisting to
dist/dist-simpleand failing fast.Proposed guard
-const buildOutputDirName = process.env.BUILD_OUTPUT_DIR || 'dist'; +const buildOutputDirName = process.env.BUILD_OUTPUT_DIR || 'dist'; +const allowedOutputDirs = new Set(['dist', 'dist-simple']); +if (!allowedOutputDirs.has(buildOutputDirName)) { + console.error(`❌ Invalid BUILD_OUTPUT_DIR: ${buildOutputDirName}`); + process.exit(1); +}Also applies to: 35-37, 61-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/package-dist.js` at line 11, Reject any BUILD_OUTPUT_DIR values outside the allowed set to avoid packaging unintended files: when reading process.env.BUILD_OUTPUT_DIR into buildOutputDirName, validate it against a whitelist (e.g., 'dist' and 'dist-simple'); if it's empty use the default 'dist', but if provided and not one of the allowed values fail fast (throw or process.exit with a clear error). Apply the same whitelist check wherever buildOutputDirName is used (notably the build steps around the earlier variable usage and the archiving step that references the directory) so archiving only ever runs on an allowed directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/package-dist.js`:
- Around line 14-18: Replace the unnecessary ternary for isSimpleMode with a
boolean assignment and stop inlining environment variables in the shell command;
instead set SIMPLE_MODE via the execSync env option. Specifically, update the
isSimpleMode declaration (currently named isSimpleMode) to a simple boolean and
change the execSync call that runs `npm run build` to pass an env object
(merging process.env) with SIMPLE_MODE set to the boolean/string value so the
command is cross-platform compatible on Windows and Unix.
---
Nitpick comments:
In `@scripts/package-dist.js`:
- Line 11: Reject any BUILD_OUTPUT_DIR values outside the allowed set to avoid
packaging unintended files: when reading process.env.BUILD_OUTPUT_DIR into
buildOutputDirName, validate it against a whitelist (e.g., 'dist' and
'dist-simple'); if it's empty use the default 'dist', but if provided and not
one of the allowed values fail fast (throw or process.exit with a clear error).
Apply the same whitelist check wherever buildOutputDirName is used (notably the
build steps around the earlier variable usage and the archiving step that
references the directory) so archiving only ever runs on an allowed directory.
| const isSimpleMode = (buildOutputDirName === "dist-simple") ? true : false; | ||
|
|
||
| // Run the build command | ||
| try { | ||
| execSync('npm run build', { stdio: 'inherit' }); | ||
| execSync(`SIMPLE_MODE=${isSimpleMode} npm run build`, { stdio: 'inherit' }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how SIMPLE_MODE is consumed (truthy/presence vs strict comparison).
rg -nP --type=ts --type=js -C3 'process\.env\.SIMPLE_MODE|SIMPLE_MODE'Repository: alam00000/bentopdf
Length of output: 3454
Fix cross-platform env variable passing in build command.
The inline SIMPLE_MODE=${isSimpleMode} npm run build syntax does not work on Windows shells. Use the env option in execSync instead for cross-platform compatibility. Also simplify the unnecessary ternary operator.
Proposed fix
-const isSimpleMode = (buildOutputDirName === "dist-simple") ? true : false;
+const isSimpleMode = buildOutputDirName === 'dist-simple';
try {
- execSync(`SIMPLE_MODE=${isSimpleMode} npm run build`, { stdio: 'inherit' });
+ execSync('npm run build', {
+ stdio: 'inherit',
+ env: {
+ ...process.env,
+ ...(isSimpleMode ? { SIMPLE_MODE: 'true' } : {}),
+ },
+ });
console.log('✅ Build completed successfully');
} catch (error) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isSimpleMode = (buildOutputDirName === "dist-simple") ? true : false; | |
| // Run the build command | |
| try { | |
| execSync('npm run build', { stdio: 'inherit' }); | |
| execSync(`SIMPLE_MODE=${isSimpleMode} npm run build`, { stdio: 'inherit' }); | |
| const isSimpleMode = buildOutputDirName === 'dist-simple'; | |
| // Run the build command | |
| try { | |
| execSync('npm run build', { | |
| stdio: 'inherit', | |
| env: { | |
| ...process.env, | |
| ...(isSimpleMode ? { SIMPLE_MODE: 'true' } : {}), | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/package-dist.js` around lines 14 - 18, Replace the unnecessary
ternary for isSimpleMode with a boolean assignment and stop inlining environment
variables in the shell command; instead set SIMPLE_MODE via the execSync env
option. Specifically, update the isSimpleMode declaration (currently named
isSimpleMode) to a simple boolean and change the execSync call that runs `npm
run build` to pass an env object (merging process.env) with SIMPLE_MODE set to
the boolean/string value so the command is cross-platform compatible on Windows
and Unix.
|
@alam00000 Probably this can be closed after 2.8.2 release? |
3 participants
Description
Add config to build and release
dist-simple-${version}.zipin addition to already existingdist-${version}.zip.Fixes #484
Type of change
Please delete options that are not relevant.
🧪 How Has This Been Tested?
The new workflow has not been tested yet.
Checklist:
Expected Results:
Upload one additional asset to the Github release.
Actual Results:
Checklist:
Summary by CodeRabbit