ci: adopt node-ci.yml reusable (org PR #19 landed)#60
Conversation
Swap the hand-rolled frontend install+typecheck+build steps for the org-wide node-ci.yml reusable. The prior adoption attempt failed with "Dependencies lock file is not found" because the reusable hardcoded `cache-dependency-path: package-lock.json` (repo root) — viz's lockfile lives at src/ResQ.Viz.Web/package-lock.json. resq-software/.github#19 parameterised `cache-dependency-path` on the reusable; this PR consumes the merged SHA. Call node-ci.yml directly rather than via required.yml because viz already calls required.yml as `lang: dotnet` for the backend stream; a second `lang: node` dispatch would duplicate the security-scan job. client-budget stays as a companion — the reusable exposes neither bundle outputs nor artifact upload, and the bundle-size ceiling is viz-specific policy. Ref pinned to merge commit 23ce94eabddf963835624451e89baca7ac9db541. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 1 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe CI workflow is restructured to parallelize frontend operations by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
102-123:⚠️ Potential issue | 🟠 MajorMeasure total emitted JS/CSS assets, not just
index-*.Lines 105–106 measure only
index-*.jsandindex-*.css, but Vite 6.0.0 enables code splitting and CSS splitting by default. Thestatglob will either fail if no files match, return a wrong value if multiple files match, or work only when exactly one file is present. Your vite.config.ts comment at lines 28–30 confirms you've experienced chunk proliferation: "Without this, Vite accumulates stale hash-renamed chunks across builds (9+ duplicates of the same 640 KB JS file at one point)." This budget gate cannot reliably measure all emitted assets.Sum all JS/CSS files under
wwwroot/assetsinstead:Proposed fix
set -euo pipefail - js=$(stat -c%s wwwroot/assets/index-*.js) - css=$(stat -c%s wwwroot/assets/index-*.css) + sum_bytes() { + label="$1" + pattern="$2" + stats=$(find wwwroot/assets -type f -name "$pattern" -printf '%s\n' | + awk '{ count += 1; bytes += $1 } END { printf "%d %d", count, bytes }') + set -- $stats + if [ "$1" -eq 0 ]; then + echo "::error::No $label assets found under wwwroot/assets" >&2 + exit 1 + fi + printf '%s\n' "$2" + } + js=$(sum_bytes JS '*.js') + css=$(sum_bytes CSS '*.css')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 102 - 123, The bundle-size step currently uses stat on a single glob (index-*.js / index-*.css) which breaks when there are zero or multiple chunks; change the measurement to sum sizes of all JS files and all CSS files under wwwroot/assets so variables js and css represent total bytes emitted; update the summary table and the budget comparisons that use BUNDLE_JS_BUDGET_BYTES and BUNDLE_CSS_BUDGET_BYTES and keep the same error/notice logic (error messages referencing JS bundle and CSS bundle remain valid) so the step reliably enforces the budget even with Vite code-splitting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 102-123: The bundle-size step currently uses stat on a single glob
(index-*.js / index-*.css) which breaks when there are zero or multiple chunks;
change the measurement to sum sizes of all JS files and all CSS files under
wwwroot/assets so variables js and css represent total bytes emitted; update the
summary table and the budget comparisons that use BUNDLE_JS_BUDGET_BYTES and
BUNDLE_CSS_BUDGET_BYTES and keep the same error/notice logic (error messages
referencing JS bundle and CSS bundle remain valid) so the step reliably enforces
the budget even with Vite code-splitting.
osv-scanner surfaced GHSA-w5hq-g745-h8pq (medium) against uuid@13.0.0, pulled transitively through effect@4.0.0-beta.52 → uuid: ^13.0.0. effect hasn't bumped its caret yet. Add a top-level npm `overrides` entry to force uuid@^14.0.0. uuid's public surface (v4()/validate()/parse()) is unchanged across the 13→14 major, so effect's usage is unaffected. Lockfile regenerated; typecheck + vite build both clean. Bundle size unchanged (776 KB JS, within 800 KB budget). This is an unrelated advisory bundled here only because it landed in osv.dev between main's last build and this branch, and the gate now blocks the CI-reusable-adoption signal we're verifying. Remove the override once `effect` upstream bumps its `uuid: ^13.0.0` range. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
clientjob from hand-rolled inline steps to the org-widenode-ci.ymlreusable.required.ymlref to the merge commit of resq-software/.github#19 — the PR that parameterisedcache-dependency-path, unblocking viz's subdirectory lockfile.client-budgetas a companion job (bundle-size gate + artifact upload — reusable doesn't expose those).Why
First adoption attempt failed with
Dependencies lock file is not foundbecausenode-ci.ymlhardcodedcache-dependency-path: package-lock.jsonat repo root, while viz's lockfile lives atsrc/ResQ.Viz.Web/package-lock.json. Upstream PR #19 made the path configurable; this PR consumes the fix.Consuming
node-ci.ymldirectly (not viarequired.yml) avoids a duplicatesecurity-scandispatch — viz already callsrequired.ymlaslang: dotnetfor the backend stream.Test plan
gates(backend + security scan viarequired.yml) stays green.client(new reusable call) installs + typechecks + builds.client-budgetruns afterclientand uploadsviz-wwwroot-<sha>.requiredaggregate passes.🤖 Generated with Claude Code
Summary by CodeRabbit