-
Notifications
You must be signed in to change notification settings - Fork 21
use nginx for handling and proxy request to API and Web UI #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdded an nginx reverse-proxy service to Docker Compose, removed host port mappings from Changes
Sequence DiagramsequenceDiagram
participant Client
participant nginx as nginx-proxy<br/>(port 80)
participant api as swetrix-api<br/>(port 5005)
participant app as swetrix<br/>(port 3000)
Client->>nginx: HTTP request (port 80)
rect rgba(200,220,240,0.5)
Note over nginx: Route decision
alt Path starts with /backend
nginx->>api: proxy_pass to swetrix-api:5005 (HTTP/1.1)
api-->>nginx: response
else Other paths (root)
nginx->>app: proxy_pass to swetrix:3000 (HTTP/1.1)
app-->>nginx: response
end
end
nginx-->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @nginx/config:
- Around line 6-12: The /api/ location block is missing several proxy headers
and operational directives; inside the location /api/ block add proxy_set_header
X-Forwarded-Proto $scheme and proxy_set_header Upgrade $http_upgrade along with
proxy_set_header Connection $connection_upgrade (and ensure you add the
top-level map $http_upgrade -> $connection_upgrade if WebSocket support is
required), and also add client_max_body_size with an appropriate limit plus
timeout directives proxy_connect_timeout, proxy_send_timeout, and
proxy_read_timeout to make the proxy resilient.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nginx/config (1)
14-20: Add missing proxy headers and consider operational directives.The root location has the same gaps as the /api/ location:
Major concerns:
- X-Forwarded-Proto header missing (critical for HTTPS setups)
- Connection and Upgrade headers missing (required if the frontend uses WebSockets for real-time updates)
Recommended improvements:
- Add client_max_body_size directive
- Add timeout configurations for better reliability
🔎 Proposed enhancements
location / { proxy_pass http://swetrix:3000; proxy_http_version 1.1; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-Proto $scheme; + proxy_set_header X-Forwarded-Host $host; + + # WebSocket support (if needed) + proxy_set_header Upgrade $http_upgrade; + proxy_set_header Connection $connection_upgrade; + + # Operational settings + client_max_body_size 10M; + proxy_connect_timeout 60s; + proxy_send_timeout 60s; + proxy_read_timeout 60s; }
🧹 Nitpick comments (2)
compose.yaml (2)
117-119: Replace deprecatedlinkswithdepends_on.The
linksdirective is deprecated in Docker Compose v2+. Usedepends_oninstead for service dependencies, which provides the same networking functionality with clearer semantics.🔎 Proposed refactor
nginx-proxy: image: nginx:1.29.4-alpine restart: always - links: - - "swetrix-api" - - "swetrix" + depends_on: + swetrix: + condition: service_healthy + swetrix-api: + condition: service_healthy ports: - "80:80"
114-123: Consider adding a health check for the nginx-proxy service.Adding a health check improves service orchestration and enables better monitoring of the reverse proxy.
🔎 Proposed enhancement
nginx-proxy: image: nginx:1.29.4-alpine restart: always links: - "swetrix-api" - "swetrix" ports: - "80:80" volumes: - ./nginx/config:/etc/nginx/conf.d/default.conf + healthcheck: + test: ["CMD-SHELL", "wget --no-verbose --tries=1 --spider http://localhost:80/ || exit 1"] + interval: 30s + timeout: 10s + retries: 3 + start_period: 10s
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
compose.yamlnginx/config
🔇 Additional comments (3)
nginx/config (1)
2-3: LGTM! Standard listen configuration.The IPv4 and IPv6 listen directives on port 80 are correctly configured for the single-domain architecture.
compose.yaml (2)
9-9: LGTM! API_URL correctly configured for single-domain setup.The hardcoded
/apivalue aligns with the nginx routing configuration that proxies/api/requests to the backend API service.
115-115: No action required. The image versionnginx:1.29.4-alpineis valid—nginx 1.29.4 (mainline) was released on December 9, 2025. If using a mainline release in production is a concern, consider pinning to the latest stable version (1.28.1 as of January 2026) instead, but the current version specification itself is not invalid.
|
wow that look fantastic! thanks for the PR, will be merged before the v5 release soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@compose.yaml`:
- Line 9: The frontend is configured with API_URL=/backend but nginx only
proxies location /api/ to the API service, causing requests to /backend to hit
the frontend; fix by either changing API_URL to "/api" in the compose
environment (update the API_URL value) or add a matching nginx location block
(add location /backend { proxy_pass http://swetrix-api:5005/; } in nginx/config)
so that requests to /backend are forwarded to swetrix-api:5005.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@compose.yaml`:
- Line 9: In the swetrix-fe container environment block in compose.yaml, replace
the unsupported environment key BASE_URL with API_URL so the frontend can locate
the backend (i.e., change the mapping from BASE_URL=${BASE_URL} to
API_URL=${BASE_URL} or set API_URL to the public Swetrix API base URL); update
the environment entry for the swetrix-fe service accordingly.
In `@README.md`:
- Around line 23-25: Replace the non-descriptive link text "here" that points to
https://docs.swetrix.com/selfhosting/configuring with a descriptive phrase like
"Self-hosting configuration guide" (or "Swetrix self-hosting configuration
guide") so the sentence "A table explaining what each value means can be found
here" becomes accessible and lint-compliant; update the markdown link text while
keeping the same URL and leave the surrounding sentences about BASE_URL and
docker unchanged to satisfy MD059.
🧹 Nitpick comments (1)
compose.yaml (1)
114-128: Consider mounting the nginx config read-only.
Prevents accidental writes in the container and signals intent.🔒 Suggested change
- - ./nginx/config:/etc/nginx/conf.d/default.conf + - ./nginx/config:/etc/nginx/conf.d/default.conf:ro
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nginx/config`:
- Around line 19-27: The proxy headers use undefined variables
$connection_upgrade and $proxy_add_forwarded; fix by either (A) replacing the
non-standard Forwarded header (proxy_set_header Forwarded) with a supported
header or removing it and set Connection to a static value (e.g., "upgrade" when
appropriate), or (B, preferred) add a map for $http_upgrade to define
$connection_upgrade at the http level (via a map directive in the http block or
top of this conf.d include) and remove/replace $proxy_add_forwarded with a
constructed Forwarded value or omit it; update the proxy_set_header lines
(proxy_set_header Connection and proxy_set_header Forwarded) accordingly so
Connection uses the defined $connection_upgrade and Forwarded does not reference
$proxy_add_forwarded.
♻️ Duplicate comments (1)
nginx/config (1)
37-58: Same undefined variable issues apply here.The
$connection_upgradeand$proxy_add_forwardedissues flagged in the/backend/block apply equally to this location block. Apply the same fix to both locations.
🧹 Nitpick comments (1)
nginx/config (1)
29-35: Consider adding timeout and body size limits.The proxy buffering configuration looks good. However, for production resilience, consider adding:
client_max_body_sizeto limit upload sizes- Timeout directives to prevent hanging connections
♻️ Proposed additions
# Proxy buffering (prevents truncated responses for large files) proxy_buffering on; proxy_buffer_size 256k; proxy_buffers 32 512k; proxy_busy_buffers_size 4m; proxy_max_temp_file_size 1024m; + + # Operational limits + client_max_body_size 10m; + proxy_connect_timeout 60s; + proxy_send_timeout 60s; + proxy_read_timeout 60s; }
This fix issue Swetrix/swetrix#460, by using nginx to proxy request then forward to either API or FE. It help Swetrix can be deploy on single domain with API is on
/api/instead of separate domainSummary by CodeRabbit
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.