Merged
Conversation
feat: add nginx reverse proxy and fix docker build env args
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an internal Docker network and nginx reverse proxy; modifies the web service to expose port 3000 internally, add build args and a healthcheck; introduces a Next.js /api/health route and a small Dockerfile runtime package install change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Nginx as Nginx (container)
participant Web as Web (container)
participant Docker as Docker (host)
Client->>Nginx: HTTP request (port 80)
Nginx->>Web: Proxy request -> http://web:3000
Web-->>Nginx: Response
Nginx-->>Client: Response
Docker->>Web: Healthcheck GET /api/health
Web-->>Docker: 200 { "status":"ok" }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable the changed files summary in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
nginx/default.conf (1)
8-9: Only sendConnection: upgradewhen the client is actually upgrading.This currently marks every proxied request as an upgrade attempt. NGINX's WebSocket guidance uses a conditional header value derived from
$http_upgrade, which is safer for mixed HTTP and WebSocket traffic. (nginx.org)♻️ Suggested change
+map $http_upgrade $connection_upgrade { + default upgrade; + '' close; +}- proxy_set_header Connection 'upgrade'; + proxy_set_header Connection $connection_upgrade;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nginx/default.conf` around lines 8 - 9, The config currently always sends Connection: 'upgrade' for every proxied request; change to send Connection only when the client requests an upgrade by using a conditional variable (e.g. define a map from $http_upgrade to $connection_upgrade and then set proxy_set_header Connection $connection_upgrade) and keep proxy_set_header Upgrade $http_upgrade as-is; update the nginx config to add the map block referencing $http_upgrade and replace the hardcoded proxy_set_header Connection 'upgrade' with proxy_set_header Connection $connection_upgrade so normal HTTP requests are not marked as WebSocket upgrades.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 36-38: The current short-form depends_on for the nginx service
only enforces start order and doesn't wait for the web service to be ready;
change nginx's depends_on to the long-form that uses condition: service_healthy
and add a healthcheck stanza to the web service (use an HTTP readiness probe or
command that matches the app, e.g., GET /health or appropriate
container-specific check) so Docker Compose will wait for the web service to be
healthy before starting nginx; reference the depends_on entry for nginx and the
web service definition when making these changes.
In `@nginx/default.conf`:
- Around line 1-6: The proxy_pass currently uses a literal host so nginx only
resolves `web` at config load; update the `server{ ... location / { proxy_pass
... } }` block to enable runtime DNS re-resolution by adding a resolver
directive and switching `proxy_pass` to use a variable or a resolvable upstream
(e.g., declare `resolver` and use `proxy_pass http://$upstream;` or define an
`upstream web { server web:3000 resolve; }`), so nginx will re-resolve the `web`
service at runtime after container recreation.
---
Nitpick comments:
In `@nginx/default.conf`:
- Around line 8-9: The config currently always sends Connection: 'upgrade' for
every proxied request; change to send Connection only when the client requests
an upgrade by using a conditional variable (e.g. define a map from $http_upgrade
to $connection_upgrade and then set proxy_set_header Connection
$connection_upgrade) and keep proxy_set_header Upgrade $http_upgrade as-is;
update the nginx config to add the map block referencing $http_upgrade and
replace the hardcoded proxy_set_header Connection 'upgrade' with
proxy_set_header Connection $connection_upgrade so normal HTTP requests are not
marked as WebSocket upgrades.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd91fad7-33dc-4618-8493-57fe5bd0df70
📒 Files selected for processing (2)
docker-compose.ymlnginx/default.conf
fix: api endpoints due to the pre built docker image
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 24-29: The healthcheck's test command uses the GNU wget flag
"--no-verbose" which BusyBox wget in the node:20-alpine image doesn't support;
either remove that flag and use a BusyBox-compatible wget invocation (e.g.,
change the healthcheck test to ["CMD-SHELL","wget --spider --tries=1
http://127.0.0.1:3000/api/health || exit 1"]) or switch to curl by installing
curl in the runner stage (add RUN apk add --no-cache curl to the Dockerfile
runner stage) and update the healthcheck test to use curl (e.g.,
["CMD-SHELL","curl -fS http://127.0.0.1:3000/api/health || exit 1"]). Ensure you
update the docker-compose.yml healthcheck test accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fa8f195-149b-43b0-b7ea-122eb8fde033
📒 Files selected for processing (3)
app/api/health/route.tsdocker-compose.ymlnginx/default.conf
🚧 Files skipped from review as they are similar to previous changes (1)
- nginx/default.conf
fix(docker-compose): update healthcheck to use BusyBox-compatible wge…
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docker-compose.yml (1)
13-13: Remove fixedcontainer_nameunless external integration requires it.Fixed
container_nameprevents scaling beyond one container and bypasses Docker Compose's project-name namespacing, creating collision risks if multiple projects run on the same host. Compose already provides stable DNS through service names.Also applies to: 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` at line 13, Remove the fixed container_name entries so Compose can manage instance names and allow scaling; specifically delete the lines setting "container_name: systemcraft_web" (and the other container_name line mentioned, e.g., "container_name: systemcraft_db") from the docker-compose.yml service definitions, leaving only the service names so Docker Compose provides stable DNS and project-scoped names unless an external integration explicitly requires a fixed container_name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 6-18: The build is silently accepting empty NEXT_PUBLIC_FIREBASE_*
args; update the args block so each build-arg uses the required-value expansion
form (e.g., replace NEXT_PUBLIC_FIREBASE_API_KEY=${NEXT_PUBLIC_FIREBASE_API_KEY}
with
NEXT_PUBLIC_FIREBASE_API_KEY=${NEXT_PUBLIC_FIREBASE_API_KEY:?NEXT_PUBLIC_FIREBASE_API_KEY
is required}) for every NEXT_PUBLIC_FIREBASE_* entry in the args list so
docker-compose will fail fast when any required Firebase env var is missing
during compose rendering.
---
Nitpick comments:
In `@docker-compose.yml`:
- Line 13: Remove the fixed container_name entries so Compose can manage
instance names and allow scaling; specifically delete the lines setting
"container_name: systemcraft_web" (and the other container_name line mentioned,
e.g., "container_name: systemcraft_db") from the docker-compose.yml service
definitions, leaving only the service names so Docker Compose provides stable
DNS and project-scoped names unless an external integration explicitly requires
a fixed container_name.
- docker-compose.yml
fix: Nextjs environment variables in the
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docker-compose.yml (2)
13-13: Dropcontainer_nameunless you truly need a fixed global name.These names prevent horizontal scaling — Compose will not scale services beyond one container when
container_nameis set. Thenginxservice can already reach the app via thewebservice name onsystemcraft_net, so fixed container names are unnecessary here.Suggested diff
- container_name: systemcraft_web @@ - container_name: systemcraft_nginxAlso applies to: 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` at line 13, Remove the hard-coded container_name entries (the container_name: systemcraft_web and container_name: systemcraft_nginx occurrences) from docker-compose.yml so Compose can scale services horizontally; leave service names (web, nginx) and their network configuration (systemcraft_net) intact so inter-service communication still uses the service hostname rather than a fixed container name.
38-40: Mount the Nginx config read-only.Nginx only needs to read this file. Making the bind mount read-only reduces risk if the proxy container is ever compromised.
Suggested diff
- - ./nginx/default.conf:/etc/nginx/conf.d/default.conf + - ./nginx/default.conf:/etc/nginx/conf.d/default.conf:ro🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 38 - 40, Update the bind mount in the volumes block so the Nginx config file is mounted read-only: locate the mapping "./nginx/default.conf:/etc/nginx/conf.d/default.conf" in the volumes section and append the read-only flag to the mount (use the :ro option) so the container can read but not modify the host config file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 24-25: The healthcheck uses wget but the runner stage image lacks
it; modify the Dockerfile's runner stage (before the USER directive) to install
wget by adding an apk install for wget (e.g., RUN apk add --no-cache wget) so
the healthcheck command (healthcheck test using wget) can run successfully and
not leave the container unhealthy.
---
Nitpick comments:
In `@docker-compose.yml`:
- Line 13: Remove the hard-coded container_name entries (the container_name:
systemcraft_web and container_name: systemcraft_nginx occurrences) from
docker-compose.yml so Compose can scale services horizontally; leave service
names (web, nginx) and their network configuration (systemcraft_net) intact so
inter-service communication still uses the service hostname rather than a fixed
container name.
- Around line 38-40: Update the bind mount in the volumes block so the Nginx
config file is mounted read-only: locate the mapping
"./nginx/default.conf:/etc/nginx/conf.d/default.conf" in the volumes section and
append the read-only flag to the mount (use the :ro option) so the container can
read but not modify the host config file.
fix: necessary refinements to our Docker and Nginx configurations
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
1 participant
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.
Summary by CodeRabbit
New Features
Chores