chore(env): Cache node dependencies#5096
Merged
SoloJacobs merged 1 commit intoprometheus:mainfrom Mar 26, 2026
Merged
Conversation
ultrotter
approved these changes
Mar 20, 2026
SoloJacobs
force-pushed
the
cache-node_modules
branch
from
19839c9 to
f913955
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughGitHub Actions workflows updated to use the repository root Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (4)
ui/app/Makefile (1)
14-15: Complete the local toolchain dependency wiring.
node_modulesonly trackspackage-lock.json, so incremental local builds will not rerunnpm ciwhenpackage.jsonchanges, anddev-serverdoes not ensure dependencies exist before invokingnpx.♻️ Suggested fix
-node_modules: package-lock.json +node_modules: package.json package-lock.json npm ci-dev-server: +dev-server: node_modules npx elm reactorAlso applies to: 35-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/Makefile` around lines 14 - 15, The Makefile's node_modules target only depends on package-lock.json so changes to package.json won't re-run npm ci and the dev-server target can run npx without ensuring dependencies; update the node_modules target to depend on both package.json and package-lock.json (and any other manifest like package-lock.json) and make the dev-server target (or its recipe) depend on node_modules so npm ci is executed before any npx invocation; modify the targets referenced as node_modules and dev-server (also update the duplicate block at the other occurrence around lines 35-36) to add these dependencies and ensure npm ci runs when manifests change..github/workflows/ci.yml (1)
60-65: Avoid callingnpmbefore the job has finished bootstrapping Node.This adds a new dependency on
npmbeforeprometheus/promciandsetup_environmentrun. If those are what provide Node in this container job, cache setup fails first. Caching~/.npmdirectly keeps the step independent of toolchain initialization.♻️ Suggested simplification
- - id: cache-paths - run: echo "npm-cache=$(npm config get cache)" >> $GITHUB_OUTPUT - uses: actions/cache@v4 with: - path: ${{ steps.cache-paths.outputs.npm-cache }} + path: ~/.npm🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 60 - 65, The cache step currently runs the step "cache-paths" which executes `npm config get cache` before Node is guaranteed to be installed; remove the dependency on invoking `npm` early by eliminating the "cache-paths" step and hardcoding the cache path (for example `~/.npm` or `${{ env.HOME }}/.npm`) in the `actions/cache@v4` `path` input so the cache step does not require Node/npm to be present and only references the `actions/cache@v4` step/output.template/Makefile (2)
4-6: Minor inconsistency: command uses./inline-cssbut dependency listsinline-css.js.The target depends on
inline-css.js, but the command runsnode ./inline-csswithout the extension. Node.js will resolve this correctly, but for consistency and clarity, consider using the full filename.♻️ Suggested fix for consistency
email.tmpl: email.html inline-css.js node_modules `@echo` ">> inline css for html email template" - node ./inline-css + node ./inline-css.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Makefile` around lines 4 - 6, The Makefile target email.tmpl is inconsistent: its dependency lists inline-css.js but the recipe runs node ./inline-css; update the recipe to run node ./inline-css.js (or alternatively change the dependency to match the executed name) so the filename is consistent; edit the email.tmpl target's recipe in the Makefile to call node ./inline-css.js to match the inline-css.js dependency.
1-6: Consider adding acleantarget fornode_modules.The static analysis tool flags missing
all,clean, andtesttargets. Whileallandtestmay not apply to this specialized sub-Makefile, acleantarget to removenode_moduleswould be useful for local development and could complement the parent Makefile's clean targets.♻️ Optional addition
.PHONY: clean clean: rm -rf node_modules🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Makefile` around lines 1 - 6, Add a PHONY clean target to the Makefile that removes node_modules so local developers and parent Makefile clean flows can purge installed deps; modify the file containing the node_modules and email.tmpl targets to define .PHONY: clean and a clean rule that rm -rf node_modules, ensuring the target name is exactly clean to match tooling expectations and does not conflict with the existing node_modules or email.tmpl targets.
🤖 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/ci.yml:
- Around line 62-67: Replace the mutable reference "uses: actions/cache@v4" with
an immutable commit SHA for the actions/cache action (i.e., "uses:
actions/cache@<commit-sha>") so the workflow is SHA-pinned like the other
actions; update the exact line containing uses: actions/cache@v4 and keep the
existing inputs (path: ${{ steps.cache-paths.outputs.npm-cache }}, key: ${{
runner.os }}-node-${{ hashFiles('**/package-lock.json') }}, restore-keys)
unchanged.
In @.github/workflows/ui-ci.yml:
- Around line 25-27: The pull_request path filter for the UI CI workflow
currently only matches ui/** so changes to the repo root .nvmrc (referenced by
the node-version-file: './.nvmrc' setting) will not trigger the job; update the
workflow's pull_request.paths array to include '/.nvmrc' (or an equivalent
pattern that matches the repo-root .nvmrc) so that changes to that file will run
the UI CI job.
In `@ui/app/CONTRIBUTING.md`:
- Around line 11-15: Update the CONTRIBUTING.md snippet that currently instructs
contributors to run "npm ci" so it explicitly directs them to run that command
from the ui/app directory (either by adding a note saying run the install
command inside ui/app or by showing the command preceded by changing into
ui/app), modifying the code block and/or adjacent sentence in CONTRIBUTING.md to
remove ambiguity about the required working directory.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 60-65: The cache step currently runs the step "cache-paths" which
executes `npm config get cache` before Node is guaranteed to be installed;
remove the dependency on invoking `npm` early by eliminating the "cache-paths"
step and hardcoding the cache path (for example `~/.npm` or `${{ env.HOME
}}/.npm`) in the `actions/cache@v4` `path` input so the cache step does not
require Node/npm to be present and only references the `actions/cache@v4`
step/output.
In `@template/Makefile`:
- Around line 4-6: The Makefile target email.tmpl is inconsistent: its
dependency lists inline-css.js but the recipe runs node ./inline-css; update the
recipe to run node ./inline-css.js (or alternatively change the dependency to
match the executed name) so the filename is consistent; edit the email.tmpl
target's recipe in the Makefile to call node ./inline-css.js to match the
inline-css.js dependency.
- Around line 1-6: Add a PHONY clean target to the Makefile that removes
node_modules so local developers and parent Makefile clean flows can purge
installed deps; modify the file containing the node_modules and email.tmpl
targets to define .PHONY: clean and a clean rule that rm -rf node_modules,
ensuring the target name is exactly clean to match tooling expectations and does
not conflict with the existing node_modules or email.tmpl targets.
In `@ui/app/Makefile`:
- Around line 14-15: The Makefile's node_modules target only depends on
package-lock.json so changes to package.json won't re-run npm ci and the
dev-server target can run npx without ensuring dependencies; update the
node_modules target to depend on both package.json and package-lock.json (and
any other manifest like package-lock.json) and make the dev-server target (or
its recipe) depend on node_modules so npm ci is executed before any npx
invocation; modify the targets referenced as node_modules and dev-server (also
update the duplicate block at the other occurrence around lines 35-36) to add
these dependencies and ensure npm ci runs when manifests change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33d6c776-0002-4af9-8eb7-461d3e091491
⛔ Files ignored due to path filters (2)
template/package-lock.jsonis excluded by!**/package-lock.jsonui/app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/ci.yml.github/workflows/ui-ci.yml.nvmrctemplate/.gitignoretemplate/Dockerfiletemplate/Makefiletemplate/package.jsonui/Dockerfileui/app/.gitignoreui/app/CONTRIBUTING.mdui/app/Makefileui/app/package.json
💤 Files with no reviewable changes (2)
- template/Dockerfile
- ui/Dockerfile
SoloJacobs
force-pushed
the
cache-node_modules
branch
from
f913955 to
d3919c7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
template/Makefile (2)
1-6: Consider adding acleantarget for completeness.The static analysis tool flagged missing standard phony targets. While
allandtestmay not be relevant here, acleantarget would be helpful for removing generated artifacts andnode_modules:♻️ Optional: Add clean target
+.PHONY: clean + node_modules: package-lock.json npm ci email.tmpl: email.html inline-css.js node_modules `@echo` ">> inline css for html email template" node ./inline-css + +clean: + rm -rf node_modules email.tmpl🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Makefile` around lines 1 - 6, Add a standard clean target to the Makefile: declare .PHONY: clean and implement clean to remove generated artifacts and dependencies (e.g., delete node_modules and the generated email template(s) produced by the email.tmpl rule such as email.tmpl and/or email.html), so that running make clean tears down what node_modules and the email.tmpl rule create.
1-2: Consider using a stamp file for more robust dependency tracking.Using a directory (
node_modules) as a Make target works for the common case, but directory timestamps can behave unexpectedly (e.g., if a file inside is modified without adding/removing entries). A stamp file pattern is more reliable:♻️ Optional: Use stamp file pattern
-node_modules: package-lock.json - npm ci +.node_modules.stamp: package-lock.json + npm ci + touch $@ + +node_modules: .node_modules.stampAlso add
.node_modules.stampto.gitignoreif you adopt this pattern.That said, the current implementation should work fine for the typical workflow where
npm ciis triggered bypackage-lock.jsonchanges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Makefile` around lines 1 - 2, The Makefile target currently uses the directory target "node_modules" which can be unreliable; change the rule to use a stamp file (e.g., ".node_modules.stamp") as the target: run "npm ci" then update the stamp file so the target is a regular file dependency on "package-lock.json"; also add the stamp file (".node_modules.stamp") to .gitignore. Locate the existing "node_modules: package-lock.json" rule and replace it with a pattern that creates and touches ".node_modules.stamp" after running npm ci.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@template/Makefile`:
- Around line 1-6: Add a standard clean target to the Makefile: declare .PHONY:
clean and implement clean to remove generated artifacts and dependencies (e.g.,
delete node_modules and the generated email template(s) produced by the
email.tmpl rule such as email.tmpl and/or email.html), so that running make
clean tears down what node_modules and the email.tmpl rule create.
- Around line 1-2: The Makefile target currently uses the directory target
"node_modules" which can be unreliable; change the rule to use a stamp file
(e.g., ".node_modules.stamp") as the target: run "npm ci" then update the stamp
file so the target is a regular file dependency on "package-lock.json"; also add
the stamp file (".node_modules.stamp") to .gitignore. Locate the existing
"node_modules: package-lock.json" rule and replace it with a pattern that
creates and touches ".node_modules.stamp" after running npm ci.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4dab71c1-25bb-400e-9e4a-f77e65103f16
⛔ Files ignored due to path filters (2)
template/package-lock.jsonis excluded by!**/package-lock.jsonui/app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/ci.yml.github/workflows/ui-ci.yml.nvmrctemplate/.gitignoretemplate/Dockerfiletemplate/Makefiletemplate/package.jsonui/Dockerfileui/app/.gitignoreui/app/CONTRIBUTING.mdui/app/Makefileui/app/package.json
💤 Files with no reviewable changes (2)
- ui/Dockerfile
- template/Dockerfile
✅ Files skipped from review due to trivial changes (5)
- template/.gitignore
- ui/app/.gitignore
- ui/app/CONTRIBUTING.md
- template/package.json
- ui/app/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ui-ci.yml
- .github/workflows/ci.yml
- ui/app/Makefile
ultrotter
reviewed
Mar 24, 2026
| DOCKER_RUN_CURRENT_USER := docker run --user=$(shell id -u $(USER)):$(shell id -g $(USER)) | ||
| DOCKER_CMD := $(DOCKER_RUN_CURRENT_USER) --rm -t -v $(PWD):/app -w /app $(DOCKER_IMG) | ||
| node_modules: package-lock.json | ||
| npm ci |
Contributor
There was a problem hiding this comment.
Some option of doing this in a container would still be lovely if possible... Can we allow both ways?
Contributor
There was a problem hiding this comment.
What would stop someone from running the make command from inside a container? Would that work or are there other constraints that would not be satisfied by that approach?
SoloJacobs
force-pushed
the
cache-node_modules
branch
2 times, most recently
from
dd1f689 to
8d994b2
Compare
This (together with moving to `npm ci`) should hopefully get rid of this sporadic failure we have: ``` Errors occurred while compiling your configuration for elm-review. I need your configuration to compile in order to know how to analyze your files. Hopefully the compiler error below will help you figure out how to fix it. make: *** [Makefile:32: test] Error 1 ``` At the very least, this makes as better citizens of the internet. Note: `nvm` picks up its configuration, even if running from a subdirectory. Signed-off-by: Solomon Jacobs <solomonjacobs@protonmail.com>
SoloJacobs
force-pushed
the
cache-node_modules
branch
from
8d994b2 to
6d08e07
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacks on top of #5092
Pull Request Checklist
Which user-facing changes does this PR introduce?
Summary by CodeRabbit