In the rebuild-distribution hook, it is recommended to add git add dist/ or similar to the entry or via a separate hook. Currently, npm run build generates files in dist/, but these modified files are not automatically staged. This means the built distribution might not be included in the commit that triggered the build, defeating the purpose of an automated pre-commit build hook.
Consider adding monitor.js to the package.json build script or verifying if it should be an entry point, as it is listed in the files filter for the pre-commit hook but typically ncc (used in your build script) bundles everything from a single entry point.
Nitpick:
Removed a typo readme.mde$ from the end-of-file-fixer exclusion; this is a good cleanup.
The PR correctly sets up default_stages to allow manual execution of hooks and introduces a local build hook to keep the distribution in sync with the source.
**Summary**
* **Critical:** None.
* **Testing:** No changes to tests required.
* **Improvement:**
* In the `rebuild-distribution` hook, it is recommended to add `git add dist/` or similar to the `entry` or via a separate hook. Currently, `npm run build` generates files in `dist/`, but these modified files are not automatically staged. This means the built distribution might not be included in the commit that triggered the build, defeating the purpose of an automated pre-commit build hook.
* Consider adding `monitor.js` to the `package.json` build script or verifying if it should be an entry point, as it is listed in the `files` filter for the pre-commit hook but typically `ncc` (used in your build script) bundles everything from a single entry point.
* **Nitpick:**
* Removed a typo `readme.mde$` from the `end-of-file-fixer` exclusion; this is a good cleanup.
The PR correctly sets up `default_stages` to allow manual execution of hooks and introduces a local build hook to keep the distribution in sync with the source.
You've added monitor.js to the list of files that trigger a rebuild. However, looking at package.json, the build script only targets index.js. If monitor.js is not imported by index.js, changes to it won't be reflected in the dist/ folder via the current build command.
You've added `monitor.js` to the list of files that trigger a rebuild. However, looking at `package.json`, the build script only targets `index.js`. If `monitor.js` is not imported by `index.js`, changes to it won't be reflected in the `dist/` folder via the current build command.
Since npm run build updates the dist/ directory, these changes won't be included in the current commit unless they are staged.
You might want to change the entry to: entry: /bin/bash -c "npm run build && git add dist/"
Or ensure that developers are aware they need to stage dist/ if the hook fails (though usually, pre-commit hooks should ideally handle the staging if they modify files).
Since `npm run build` updates the `dist/` directory, these changes won't be included in the current commit unless they are staged.
You might want to change the entry to:
`entry: /bin/bash -c "npm run build && git add dist/"`
Or ensure that developers are aware they need to stage `dist/` if the hook fails (though usually, pre-commit hooks should ideally handle the staging if they modify files).
Improvement: The monitor.js file is included in the pre-commit trigger for the build hook, but it is not found in the repository root and is not referenced in the npm run build command (which targets index.js).
Nitpick: Fixed a small typo in the exclusion list (readme.mde$).
The author has successfully addressed previous feedback regarding the need to stage dist/ changes automatically by adding the add-dist hook. Although monitor.js is still missing from the root or build command, this isn't a blocking issue since the primary index.js and package.json logic is correct.
**Summary**
* **Critical:** None.
* **Testing:** None.
* **Improvement:** The `monitor.js` file is included in the pre-commit trigger for the build hook, but it is not found in the repository root and is not referenced in the `npm run build` command (which targets `index.js`).
* **Nitpick:** Fixed a small typo in the exclusion list (`readme.mde$`).
The author has successfully addressed previous feedback regarding the need to stage `dist/` changes automatically by adding the `add-dist` hook. Although `monitor.js` is still missing from the root or build command, this isn't a blocking issue since the primary `index.js` and `package.json` logic is correct.
The file monitor.js is not present in the repository root. If this is a future file, keep it; otherwise, this entry will never trigger the hook for that specific file. Additionally, the build script in package.json only compiles index.js.
The file `monitor.js` is not present in the repository root. If this is a future file, keep it; otherwise, this entry will never trigger the hook for that specific file. Additionally, the `build` script in `package.json` only compiles `index.js`.
Testing: No changes to tests required for this configuration update.
Improvement: The inclusion of monitor.js in the files regex for the rebuild-distribution and stage-distribution hooks is redundant as the file does not exist in the repository root and is not part of the npm run build command (which only targets index.js).
Nitpick: None.
The PR addresses previous feedback by automating the staging of the dist/ directory after a build, ensuring that compiled assets stay in sync with source changes during the commit process. While monitor.js is missing, it does not impede the functionality of the existing hooks.
**Summary**
* **Critical:** None.
* **Testing:** No changes to tests required for this configuration update.
* **Improvement:** The inclusion of `monitor.js` in the `files` regex for the `rebuild-distribution` and `stage-distribution` hooks is redundant as the file does not exist in the repository root and is not part of the `npm run build` command (which only targets `index.js`).
* **Nitpick:** None.
The PR addresses previous feedback by automating the staging of the `dist/` directory after a build, ensuring that compiled assets stay in sync with source changes during the commit process. While `monitor.js` is missing, it does not impede the functionality of the existing hooks.
monitor.js does not exist in the repository root. If this is not a planned file, it should be removed from the regex to avoid confusion. Additionally, the build script in package.json currently only processes index.js.
`monitor.js` does not exist in the repository root. If this is not a planned file, it should be removed from the regex to avoid confusion. Additionally, the `build` script in `package.json` currently only processes `index.js`.
Improvement: The regex for the rebuild-distribution and stage-distribution hooks includes monitor.js, which does not exist in the repository root. While harmless, it should be removed to avoid confusion. Additionally, as noted in previous reviews, the npm run build command only targets index.js.
Nitpick: None.
The PR effectively addresses the need to ensure the dist/ folder is updated and staged automatically whenever source files are changed, preventing desynchronization between the source code and the compiled action. Previous feedback regarding the staging of files has been addressed by adding the add-dist hook.
**Summary**
* **Critical:** None.
* **Testing:** None.
* **Improvement:** The regex for the `rebuild-distribution` and `stage-distribution` hooks includes `monitor.js`, which does not exist in the repository root. While harmless, it should be removed to avoid confusion. Additionally, as noted in previous reviews, the `npm run build` command only targets `index.js`.
* **Nitpick:** None.
The PR effectively addresses the need to ensure the `dist/` folder is updated and staged automatically whenever source files are changed, preventing desynchronization between the source code and the compiled action. Previous feedback regarding the staging of files has been addressed by adding the `add-dist` hook.
monitor.js is included in the file filter but does not exist in the repository. If there are no plans to add this file, it should be removed from the regex.
`monitor.js` is included in the file filter but does not exist in the repository. If there are no plans to add this file, it should be removed from the regex.
Testing: No changes to tests required for this configuration change.
Improvement:
In .pre-commit-config.yaml, the hooks build and add-dist reference monitor.js in the files regex, but this file does not exist in the repository root. This has been noted in previous reviews but remains unaddressed.
The build script in package.json only processes index.js. If monitor.js is intended to be a source file, it should be added to the repository or the regex should be cleaned up to avoid confusion.
Nitpick: None.
The PR effectively sets up pre-commit stages to distinguish between fast local checks and manual/CI checks, and ensures the dist/ folder is updated and staged automatically upon changes to source files. Since the core functionality is correct and the monitor.js issue is non-breaking, I am approving the changes.
**Summary**
* **Critical:** None.
* **Testing:** No changes to tests required for this configuration change.
* **Improvement:**
* In `.pre-commit-config.yaml`, the hooks `build` and `add-dist` reference `monitor.js` in the `files` regex, but this file does not exist in the repository root. This has been noted in previous reviews but remains unaddressed.
* The `build` script in `package.json` only processes `index.js`. If `monitor.js` is intended to be a source file, it should be added to the repository or the regex should be cleaned up to avoid confusion.
* **Nitpick:** None.
The PR effectively sets up pre-commit stages to distinguish between fast local checks and manual/CI checks, and ensures the `dist/` folder is updated and staged automatically upon changes to source files. Since the core functionality is correct and the `monitor.js` issue is non-breaking, I am approving the changes.
monitor.js is still included in the regex but does not exist in the repository. As noted in previous reviews, if there are no immediate plans for this file, it's cleaner to remove it.
`monitor.js` is still included in the regex but does not exist in the repository. As noted in previous reviews, if there are no immediate plans for this file, it's cleaner to remove it.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
AutoReview
Summary
rebuild-distributionhook, it is recommended to addgit add dist/or similar to theentryor via a separate hook. Currently,npm run buildgenerates files indist/, but these modified files are not automatically staged. This means the built distribution might not be included in the commit that triggered the build, defeating the purpose of an automated pre-commit build hook.monitor.jsto thepackage.jsonbuild script or verifying if it should be an entry point, as it is listed in thefilesfilter for the pre-commit hook but typicallyncc(used in your build script) bundles everything from a single entry point.readme.mde$from theend-of-file-fixerexclusion; this is a good cleanup.The PR correctly sets up
default_stagesto allow manual execution of hooks and introduces a local build hook to keep the distribution in sync with the source.@@ -47,0 +51,4 @@- id: buildname: rebuild-distributionentry: npm run buildlanguage: systemYou've added
monitor.jsto the list of files that trigger a rebuild. However, looking atpackage.json, the build script only targetsindex.js. Ifmonitor.jsis not imported byindex.js, changes to it won't be reflected in thedist/folder via the current build command.@@ -47,0 +52,4 @@name: rebuild-distributionentry: npm run buildlanguage: systemfiles: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$Since
npm run buildupdates thedist/directory, these changes won't be included in the current commit unless they are staged.You might want to change the entry to:
entry: /bin/bash -c "npm run build && git add dist/"Or ensure that developers are aware they need to stage
dist/if the hook fails (though usually, pre-commit hooks should ideally handle the staging if they modify files).Summary
monitor.jsfile is included in the pre-commit trigger for the build hook, but it is not found in the repository root and is not referenced in thenpm run buildcommand (which targetsindex.js).readme.mde$).The author has successfully addressed previous feedback regarding the need to stage
dist/changes automatically by adding theadd-disthook. Althoughmonitor.jsis still missing from the root or build command, this isn't a blocking issue since the primaryindex.jsandpackage.jsonlogic is correct.@@ -47,0 +54,4 @@language: systemfiles: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$pass_filenames: falsestages: [pre-commit]The file
monitor.jsis not present in the repository root. If this is a future file, keep it; otherwise, this entry will never trigger the hook for that specific file. Additionally, thebuildscript inpackage.jsononly compilesindex.js.@@ -47,0 +61,4 @@language: systemfiles: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$pass_filenames: falsestages: [pre-commit]This is a good solution to ensure the compiled distribution is always included in the commit that modified the source.
Summary
monitor.jsin thefilesregex for therebuild-distributionandstage-distributionhooks is redundant as the file does not exist in the repository root and is not part of thenpm run buildcommand (which only targetsindex.js).The PR addresses previous feedback by automating the staging of the
dist/directory after a build, ensuring that compiled assets stay in sync with source changes during the commit process. Whilemonitor.jsis missing, it does not impede the functionality of the existing hooks.@@ -47,0 +55,4 @@language: systemfiles: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$pass_filenames: falsestages: [pre-commit]monitor.jsdoes not exist in the repository root. If this is not a planned file, it should be removed from the regex to avoid confusion. Additionally, thebuildscript inpackage.jsoncurrently only processesindex.js.@@ -47,0 +62,4 @@language: systemfiles: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$pass_filenames: falsestages: [pre-commit]Same as above,
monitor.jsis referenced here but is missing from the file tree.Summary
rebuild-distributionandstage-distributionhooks includesmonitor.js, which does not exist in the repository root. While harmless, it should be removed to avoid confusion. Additionally, as noted in previous reviews, thenpm run buildcommand only targetsindex.js.The PR effectively addresses the need to ensure the
dist/folder is updated and staged automatically whenever source files are changed, preventing desynchronization between the source code and the compiled action. Previous feedback regarding the staging of files has been addressed by adding theadd-disthook.@@ -47,0 +54,4 @@- id: buildname: rebuild-distributionentry: npm run buildlanguage: systemmonitor.jsis included in the file filter but does not exist in the repository. If there are no plans to add this file, it should be removed from the regex.@@ -47,0 +61,4 @@- id: add-distname: stage-distributionentry: git add dist/language: systemSame here,
monitor.jsis referenced but does not exist in the file tree.Summary
.pre-commit-config.yaml, the hooksbuildandadd-distreferencemonitor.jsin thefilesregex, but this file does not exist in the repository root. This has been noted in previous reviews but remains unaddressed.buildscript inpackage.jsononly processesindex.js. Ifmonitor.jsis intended to be a source file, it should be added to the repository or the regex should be cleaned up to avoid confusion.The PR effectively sets up pre-commit stages to distinguish between fast local checks and manual/CI checks, and ensures the
dist/folder is updated and staged automatically upon changes to source files. Since the core functionality is correct and themonitor.jsissue is non-breaking, I am approving the changes.@@ -45,2 +49,4 @@- id: mixed-line-endingargs: [--fix=lf]- repo: localmonitor.jsis still included in the regex but does not exist in the repository. As noted in previous reviews, if there are no immediate plans for this file, it's cleaner to remove it.@@ -47,0 +56,4 @@entry: npm run buildlanguage: systemfiles: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$pass_filenames: falseSame as above,
monitor.jsis referenced here but is missing from the file tree.Pull request closed