BUGFIX/HCMPRE-0010 #3009
BUGFIX/HCMPRE-0010 #3009
Conversation
📝 WalkthroughWalkthroughAdds build-time ARGs GIT_COMMIT and GIT_BRANCH (exported as ENV) and changes the Dockerfile WORK_DIR to /var/web/health-dss; adds an nginx server block serving /dashboard-ui static assets from /var/web with try_files fallback. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev
participant Docker as Docker Build
participant Build as Build Stage
participant Image as Final Image
Dev->>Docker: docker build --build-arg GIT_COMMIT=... --build-arg GIT_BRANCH=...
Docker->>Build: supply ARGs (GIT_COMMIT, GIT_BRANCH)
Build-->>Image: ENV GIT_COMMIT / ENV GIT_BRANCH baked into image
Note right of Image: WORK_DIR=/var/web/health-dss
Dev->>Image: docker run image
sequenceDiagram
participant Client
participant Nginx as nginx (container)
participant FS as Filesystem (/var/web)
Client->>Nginx: GET /dashboard-ui/path-or-root
Nginx->>FS: lookup /var/web/dashboard-ui/path-or-root
alt file exists
FS-->>Nginx: return static asset
Nginx-->>Client: 200 + asset
else not found
Nginx->>FS: return /var/web/dashboard-ui/index.html (try_files fallback)
FS-->>Nginx: index.html
Nginx-->>Client: 200 + index.html
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
health/micro-ui/web/health-dss/Dockerfile (2)
1-3: Minor Dockerfile hygiene: collapse APK operations to reduce layers and avoid redundantapk update.Using
apk add --no-cachemakesapk updateredundant. You can merge the two RUNs into one (and include the symlink step if you prefer), e.g.:-FROM node:14-alpine3.16 AS build -RUN apk update -RUN apk add --no-cache 'git>2.30.0' +FROM node:14-alpine3.16 AS build +RUN apk add --no-cache 'git>2.30.0' python3 make g++ \ + && ln -sf python3 /usr/bin/python
1-1: Consider upgrading from Node 14 (EOL) to a maintained LTS base image to avoid toolchain friction.Node 14 is EOL and often causes node-gyp/node-sass pain. If feasible for this app, consider moving to node:18-alpine or node:20-alpine, and aligning node-sass (or migrate to sass/dart-sass). This is optional but will reduce maintenance overhead.
Also applies to: 38-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
health/micro-ui/web/health-dss/Dockerfile(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build dashboard-ui
health/micro-ui/web/health-dss/Dockerfile
[error] 39-39: Docker image build failed at RUN yarn install: node-sass build failed due to missing Python; node-gyp could not find Python (log shows: Can't find Python executable 'python').
⏰ Context from checks skipped due to timeout of 10000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
health/micro-ui/web/health-dss/Dockerfile (1)
46-46: WORK_DIR changed to /var/web/health-dss — verify nginx.conf and any ops scripts reflect this.
- The COPY targets now use ${WORK_DIR}/ … ensure the nginx.conf you copy from /app/web/health-dss/nginx.conf points its root to /var/web/health-dss (or whatever your desired doc root is).
- Check any Helm charts, k8s probes, or sidecar scripts that may have previously referenced /var/web/dashboard-ui.
If everything already references ${WORK_DIR}, you’re good.
| RUN apk update | ||
| RUN apk add --no-cache 'git>2.30.0' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Build failing: node-gyp can’t find Python (node-sass). Install python3, make, g++ and add python symlink.
The pipeline error indicates node-sass native compilation is failing due to missing Python. On Alpine, node-gyp requires python3 and build tools, and often a python symlink.
Apply this diff to fix the build:
-RUN apk update
-RUN apk add --no-cache 'git>2.30.0'
+RUN apk add --no-cache 'git>2.30.0' python3 make g++
+# node-gyp expects `python`; provide a symlink to python3
+RUN ln -sf python3 /usr/bin/pythonOptional (if you hit further linking issues): add libc6-compat.
-RUN apk add --no-cache 'git>2.30.0' python3 make g++
+RUN apk add --no-cache 'git>2.30.0' python3 make g++ libc6-compatNote: You can also reduce layers by dropping the separate apk update since --no-cache fetches fresh indexes.
Also applies to: 39-39
🧰 Tools
🪛 Hadolint (2.12.0)
[warning] 3-3: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
[info] 3-3: Multiple consecutive RUN instructions. Consider consolidation.
(DL3059)
🤖 Prompt for AI Agents
In health/micro-ui/web/health-dss/Dockerfile around lines 2-3 (and also apply to
line 39), the build fails because node-gyp/node-sass need Python and build tools
on Alpine; remove the separate `apk update` and instead install python3, make,
g++ (and optionally libc6-compat) in one apk add --no-cache command, then create
a /usr/bin/python symlink pointing to python3 so node-gyp can find it; ensure
these packages are installed in the same layer as other build dependencies to
avoid extra layers.
| ARG GIT_COMMIT=unknown | ||
| ARG GIT_BRANCH=unknown |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Build args for VCS metadata added — ensure CI supplies them and consider surfacing in final image.
Good addition. Two follow-ups:
- Make sure your CI/build system passes GIT_COMMIT and GIT_BRANCH (else the image will carry "unknown").
- If you intend these to be available at runtime (final nginx image), you need to re-declare them in the final stage; ENV from the build stage does not propagate across stages.
Proposed change to expose in the final stage (after the second FROM):
FROM nginx:mainline-alpine
#FROM ghcr.io/egovernments/nginx:mainline-alpine
+ARG GIT_COMMIT=unknown
+ARG GIT_BRANCH=unknown
+ENV GIT_COMMIT=$GIT_COMMIT
+ENV GIT_BRANCH=$GIT_BRANCH
ENV WORK_DIR=/var/web/health-dss🤖 Prompt for AI Agents
In health/micro-ui/web/health-dss/Dockerfile around lines 5 to 6, the Dockerfile
defines build ARGs GIT_COMMIT and GIT_BRANCH but does not ensure they are
supplied by CI or propagated into the final image; update your CI to pass these
build-args and, in the final stage (after the second FROM), re-declare the ARGs
and set corresponding ENVs so the values are available at runtime (e.g., add ARG
GIT_COMMIT and ARG GIT_BRANCH in the final stage and then set ENV
GIT_COMMIT=$GIT_COMMIT and ENV GIT_BRANCH=$GIT_BRANCH).
| ENV GIT_COMMIT=$GIT_COMMIT | ||
| ENV GIT_BRANCH=$GIT_BRANCH |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
ENV set only in build stage won’t be present in the final image.
If your React/Vue build pipeline consumes these during build, you’re fine. If you expect to read them at container runtime (served by nginx), re-declare them in the final stage as shown in my other comment.
🤖 Prompt for AI Agents
In health/micro-ui/web/health-dss/Dockerfile around lines 8 to 9, the ENV
variables GIT_COMMIT and GIT_BRANCH are only set in the build stage so they
won’t be available in the final image at runtime; to fix, re-declare the same
ENV GIT_COMMIT and ENV GIT_BRANCH lines in the final stage (after the final
FROM) or convert them to build-time ARG and propagate them into the final stage
(ARG in build stage, ARG in final stage, then ENV from that ARG) so the
variables are present in the deployed container.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
health/micro-ui/web/package.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/health-dss/nginx.conf(1 hunks)
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/health-dss/nginx.conf (2)
10-10: SPA fallback looks correct.
try_files $uri $uri/ /dashboard-ui/index.html;is the right pattern for client-side routing. Keep as-is (also works with alias).
4-4: No relocation needed forunderscores_in_headersThe
underscores_in_headersdirective is valid in bothhttpandservercontexts (https://nginx.org/r/underscores_in_headers). It can remain at line 4 of
health/micro-ui/web/micro-ui-internals/packages/modules/health-dss/nginx.conf.
| @@ -0,0 +1,12 @@ | |||
| server | |||
| { | |||
| listen 80; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Harden vhost selection: set default_server and an explicit server_name.
To avoid accidental vhost collisions if more server blocks exist, declare default_server and a catch-all server_name.
- listen 80;
+ listen 80 default_server;
+ server_name _;🤖 Prompt for AI Agents
In health/micro-ui/web/micro-ui-internals/packages/modules/health-dss/nginx.conf
around line 3, the server block only has "listen 80;" which risks vhost
collisions; update the listen directive to include "default_server" and add an
explicit catch-all server_name (e.g. "_" or a specific hostname) to the server
block so this vhost is the default and will not accidentally match other virtual
hosts.
| location /dashboard-ui | ||
| { | ||
| root /var/web; | ||
| index index.html index.htm; | ||
| try_files $uri $uri/ /dashboard-ui/index.html; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: add caching headers for static assets and prevent caching index.html.
For hashed bundle assets, long-lived caching improves performance. Avoid caching index.html to ensure users get the latest app shell.
Add these sibling locations in the same server block (outside the existing location):
# Long cache for static assets (adjust extensions to your build artifacts)
location ~* \.(?:js|css|png|jpg|jpeg|gif|svg|ico|woff2?|ttf|map)$ {
add_header Cache-Control "public, max-age=31536000, immutable";
expires 1y;
}
# Do not cache the SPA shell
location = /dashboard-ui/index.html {
add_header Cache-Control "no-store";
}🤖 Prompt for AI Agents
In health/micro-ui/web/micro-ui-internals/packages/modules/health-dss/nginx.conf
around lines 6 to 11, add two sibling location blocks in the same server block
(outside the existing /dashboard-ui location): one location matching static
asset extensions (js, css, png, jpg, jpeg, gif, svg, ico, woff2, woff, ttf, map
— adjust extensions to your build artifacts) that sets long-lived caching
headers (e.g., add_header Cache-Control "public, max-age=31536000, immutable";
and expires 1y;) and a separate exact-match location for
/dashboard-ui/index.html that sets no-store (e.g., add_header Cache-Control
"no-store";) so hashed bundles are cached long-term while the SPA shell is
always fetched fresh.
💡 Verification agent
🧩 Analysis chain
Verify docroot mapping: likely mismatch between Nginx root and Docker deployment path (may cause 404s).
Given the Dockerfile change (AI summary) to deploy built assets under /var/web/health-dss, serving /dashboard-ui with root /var/web; will look under /var/web/dashboard-ui, not /var/web/health-dss. If assets are indeed under /var/web/health-dss, switch to alias and tighten the location. Otherwise, confirm assets land in /var/web/dashboard-ui.
Apply if assets are under /var/web/health-dss:
- location /dashboard-ui
+ location ^~ /dashboard-ui/
{
- root /var/web;
+ alias /var/web/health-dss/;
index index.html index.htm;
try_files $uri $uri/ /dashboard-ui/index.html;
}Run this script to confirm where assets are copied in the final image:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching Dockerfiles for health-dss workdir and copy destinations..."
fd -a Dockerfile | xargs -I{} sh -c 'echo "---- {} ----"; rg -n -C2 --no-heading -e "WORKDIR" -e "COPY" -e "/var/web" -e "health-dss" "{}"'
echo
echo "Confirming nginx.conf path(s) referenced and current content:"
fd -a nginx.conf | xargs -I{} sh -c 'echo "---- {} ----"; sed -n "1,120p" "{}"'Length of output: 15134
Fix Nginx docroot for health-dss assets
The Dockerfile for health-dss copies your built files into /var/web/health-dss, but the current nginx.conf uses root /var/web; under /dashboard-ui, causing Nginx to look in /var/web/dashboard-ui (and 404). Update the location block to use alias and strip the URI prefix:
• File: health/micro-ui/web/health-dss/nginx.conf
• Lines: 6–11
- location /dashboard-ui
- {
- root /var/web;
+ location ^~ /dashboard-ui/ {
+ alias /var/web/health-dss/;
index index.html index.htm;
try_files $uri $uri/ /dashboard-ui/index.html;
}This change maps /dashboard-ui/* requests to the actual /var/web/health-dss directory.
📝 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.
| location /dashboard-ui | |
| { | |
| root /var/web; | |
| index index.html index.htm; | |
| try_files $uri $uri/ /dashboard-ui/index.html; | |
| } | |
| location ^~ /dashboard-ui/ { | |
| alias /var/web/health-dss/; | |
| index index.html index.htm; | |
| try_files $uri $uri/ /dashboard-ui/index.html; | |
| } |
🤖 Prompt for AI Agents
In health/micro-ui/web/micro-ui-internals/packages/modules/health-dss/nginx.conf
around lines 6 to 11, the location /dashboard-ui block uses root /var/web which
resolves to /var/web/dashboard-ui and 404s; change the block to use alias
/var/web/health-dss/ and ensure the location has a trailing slash (location
/dashboard-ui/ { ... }) so Nginx strips the /dashboard-ui prefix, keep index
directives, and use try_files $uri $uri/ /dashboard-ui/index.html to fall back
to the app entrypoint.
BUGFIX/HCMPRE-0010
Summary by CodeRabbit
New Features
Chores