Screeps/screeps-deploy-action
4
1
Fork 0
Code Issues 1 Pull Requests Actions Packages Releases 4 Activity

chore: configure pre-commit stages and add build hook #83

Closed
Philipp wants to merge 6 commits from feature/pre-commit-build into main
pull from: feature/pre-commit-build
merge into: Screeps:main
Conversation 5 Commits 6 Files Changed 2
+23 -2
Philipp commented 2026-05-16 16:43:32 +02:00
Owner
Copy Link
Copy Source
No description provided.
Philipp self-assigned this 2026-05-16 16:43:32 +02:00
Philipp added 2 commits 2026-05-16 16:43:33 +02:00
chore: configure pre-commit stages and add build hook
Lint / pre-commit Linting (push) Failing after 44s
Details
Test / Run Tests (push) Successful in 4m36s
Details
3abc837f78
chore: remove pre-push stage from pre-commit config
Lint / pre-commit Linting (push) Failing after 55s
Details
Test / Run Tests (push) Successful in 1m2s
Details
28eb7c2971
Philipp requested review from AutoReview 2026-05-16 16:44:02 +02:00
AutoReview approved these changes 2026-05-16 16:44:14 +02:00
AutoReview left a comment
Member
Copy Link
Copy 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.

**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.
.pre-commit-config.yaml
@@ -47,0 +51,4 @@
- id: build
name: rebuild-distribution
entry: npm run build
language: system
AutoReview commented 2026-05-16 16:44:13 +02:00
Member
Copy Link
Copy 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.
.pre-commit-config.yaml
@@ -47,0 +52,4 @@
name: rebuild-distribution
entry: npm run build
language: system
files: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$
AutoReview commented 2026-05-16 16:44:13 +02:00
Member
Copy Link
Copy Source

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).
Philipp added 1 commit 2026-05-16 16:47:17 +02:00
chore: add stage-distribution hook to ensure dist/ is committed
Lint / pre-commit Linting (push) Failing after 36s
Details
Test / Run Tests (push) Successful in 1m6s
Details
e5ceb46096
AutoReview approved these changes 2026-05-16 16:47:25 +02:00
AutoReview left a comment
Member
Copy Link
Copy Source

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.

**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.
.pre-commit-config.yaml
@@ -47,0 +54,4 @@
language: system
files: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$
pass_filenames: false
stages: [pre-commit]
AutoReview commented 2026-05-16 16:47:25 +02:00
Member
Copy Link
Copy Source

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`.
.pre-commit-config.yaml
@@ -47,0 +61,4 @@
language: system
files: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$
pass_filenames: false
stages: [pre-commit]
AutoReview commented 2026-05-16 16:47:25 +02:00
Member
Copy Link
Copy Source

This is a good solution to ensure the compiled distribution is always included in the commit that modified the source.

This is a good solution to ensure the compiled distribution is always included in the commit that modified the source.
Philipp added 1 commit 2026-05-16 16:48:00 +02:00
chore: exclude no-commit-to-branch from manual stage
Lint / pre-commit Linting (push) Failing after 54s
Details
Test / Run Tests (push) Successful in 1m9s
Details
8cf0be7fff
AutoReview approved these changes 2026-05-16 16:48:06 +02:00
AutoReview left a comment
Member
Copy Link
Copy Source

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.

**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.
.pre-commit-config.yaml
@@ -47,0 +55,4 @@
language: system
files: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$
pass_filenames: false
stages: [pre-commit]
AutoReview commented 2026-05-16 16:48:06 +02:00
Member
Copy Link
Copy Source

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`.
Philipp marked this conversation as resolved
.pre-commit-config.yaml
@@ -47,0 +62,4 @@
language: system
files: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$
pass_filenames: false
stages: [pre-commit]
AutoReview commented 2026-05-16 16:48:06 +02:00
Member
Copy Link
Copy Source

Same as above, monitor.js is referenced here but is missing from the file tree.

Same as above, `monitor.js` is referenced here but is missing from the file tree.
Philipp marked this conversation as resolved
Philipp added 1 commit 2026-05-16 16:50:03 +02:00
chore: set github action/workflow checks to manual stage
Lint / pre-commit Linting (push) Failing after 44s
Details
Test / Run Tests (push) Successful in 51s
Details
d674506e49
AutoReview approved these changes 2026-05-16 16:50:09 +02:00
AutoReview left a comment
Member
Copy Link
Copy Source

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.

**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.
.pre-commit-config.yaml
@@ -47,0 +54,4 @@
- id: build
name: rebuild-distribution
entry: npm run build
language: system
AutoReview commented 2026-05-16 16:50:09 +02:00
Member
Copy Link
Copy Source

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.
.pre-commit-config.yaml
@@ -47,0 +61,4 @@
- id: add-dist
name: stage-distribution
entry: git add dist/
language: system
AutoReview commented 2026-05-16 16:50:09 +02:00
Member
Copy Link
Copy Source

Same here, monitor.js is referenced but does not exist in the file tree.

Same here, `monitor.js` is referenced but does not exist in the file tree.
Philipp added 1 commit 2026-05-16 16:52:44 +02:00
chore: update lint workflow to include manual stage hooks
Lint / pre-commit Linting (push) Successful in 42s
Details
Test / Run Tests (push) Successful in 53s
Details
1aea1ba3b1
AutoReview approved these changes 2026-05-16 16:52:52 +02:00
AutoReview left a comment
Member
Copy Link
Copy Source

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.

**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.
.pre-commit-config.yaml
@@ -45,2 +49,4 @@
- id: mixed-line-ending
args: [--fix=lf]
- repo: local
AutoReview commented 2026-05-16 16:52:52 +02:00
Member
Copy Link
Copy Source

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.
.pre-commit-config.yaml
@@ -47,0 +56,4 @@
entry: npm run build
language: system
files: ^(index\.js|monitor\.js|package\.json|package-lock\.json)$
pass_filenames: false
AutoReview commented 2026-05-16 16:52:52 +02:00
Member
Copy Link
Copy Source

Same as above, monitor.js is referenced here but is missing from the file tree.

Same as above, `monitor.js` is referenced here but is missing from the file tree.
Philipp closed this pull request 2026-05-16 16:56:20 +02:00
All checks were successful
Lint / pre-commit Linting (push) Successful in 42s
Required
Details
Test / Run Tests (push) Successful in 53s
Required
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
AutoReview
Labels
Clear labels
bug
Something is not working
 chore
duplicate
This issue or pull request already exists
 enhancement
New feature
 help wanted
Need some help
 invalid
Something is wrong
 question
More information is needed
 wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
AutoReview (LLMReview) Philipp (Philipp Horstenkamp) Renovate (Renovate Bot) runner (Act Runner)
No Assignees Philipp
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Screeps/screeps-deploy-action#83
Delete Branch "feature/pre-commit-build"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?