Conversation
📝 WalkthroughWalkthroughNginx frontend template updated: added Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@template/frontend/`{% if not deploy_as_executable %}default.conf.template{%
endif %}.jinja:
- Around line 20-22: The add_header directives for Cache-Control, Pragma, and
Expires (the three add_header lines shown) should include the always parameter
so nginx emits these no-cache headers on all responses (including 4xx/5xx);
update the add_header statements in the template where these headers are set
(both the index and proxy/response header groups) to append always to each
add_header directive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d505d827-e94e-4a34-b469-e8bba823b919
📒 Files selected for processing (1)
template/frontend/{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja
template/frontend/{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja
Outdated
Show resolved
Hide resolved
|
|
||
| # Serve the static site | ||
| location / { | ||
| add_header Cache-Control "no-cache, no-store, must-revalidate"; |
There was a problem hiding this comment.
We've previously encountered the probem that we deploy a new version of the frontend, but then in people's browsers when they navigate they still see the old pages (and we have to tell them to press Ctrl F5 for a hard refresh in the browser to fix it).
I thought that'd be solved by less caching, not more. It looks like this PR is a mix (and I am also new to NGINX).
Is this going to help that problem? Make it worse? Or solve an unrelated issue?
There was a problem hiding this comment.
yes this should fix that. I believe what is happening is that the index.html gets cached by the browser and when we deploy a new version it never updated until we hard refresh. This will stop that (after we do one more hard refresh) since this requires that to ALWAYS ask the server to get the new version. For nuxt resources those have hashes in the file names based on the content so whenever those change a new version will be pulled down since it wont be in the cache.
There was a problem hiding this comment.
We have more pages than just index.html right? But you're saying it won't make the problem with those other pages any worse?
There was a problem hiding this comment.
So the / rule we have is a catch all. It handles all the other html files we have in here as well. And even the favicon
/usr/share/nginx/html # tree
.
├── 200.html
├── 404.html
├── 50x.html
├── _nuxt
│ ├── ARuhl--O.js
│ ├── Be8_aPZ_.js
│ ├── Bk51v8cq.js
│ ├── BkDe5Ddt.js
│ ├── Brvk3suN.js
│ ├── CA9OCOOc.js
│ ├── D21foV_B.js
│ ├── DIC0oAXs.js
│ ├── Dc9gDbTo.js
│ ├── entry.DaHHffCY.css
│ ├── error-404.CaJ5VLt6.css
│ ├── error-500.DULjGOxy.css
│ ├── m05vKvW6.js
│ └── rdkJo02O.js
├── _payload.json
├── device
│ ├── _payload.json
│ └── index.html
├── favicon.ico
└── index.html
2 directories, 22 files
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
template/frontend/{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja (1)
57-59:⚠️ Potential issue | 🟠 MajorMissing
alwaysparameter on cache control headers.The
add_headerdirectives here lack thealwaysparameter, which creates an inconsistency with lines 20-22 and doesn't fully implement the past review feedback. Withoutalways, nginx only sends these headers on successful responses (2xx, 3xx status codes). This means error responses from the backend (404 for missing endpoints, 500 for server errors, etc.) won't receive the no-cache headers and could be cached by browsers or intermediate proxies.Given the commit message "always do it no matter the response code" and the fact that lines 20-22 already include
always, this appears to be an oversight.🔧 Proposed fix to add `always` parameter
location `@proxy` { - add_header Cache-Control "no-cache, no-store, must-revalidate"; - add_header Pragma "no-cache"; - add_header Expires "0"; + add_header Cache-Control "no-cache, no-store, must-revalidate" always; + add_header Pragma "no-cache" always; + add_header Expires "0" always; proxy_pass http://${BACKEND_HOST}:${BACKEND_PORT};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/frontend/`{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja around lines 57 - 59, The three nginx add_header directives (add_header Cache-Control "no-cache, no-store, must-revalidate"; add_header Pragma "no-cache"; add_header Expires "0";) are missing the always parameter so they won't be sent on error responses; update each directive to include always (e.g., add_header Cache-Control "no-cache, no-store, must-revalidate" always;) to match the existing headers that already include always and ensure no-cache headers are attached for all response codes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@template/frontend/`{% if not deploy_as_executable %}default.conf.template{%
endif %}.jinja:
- Around line 12-16: The location block for /_nuxt/ currently sets both expires
1w; and add_header Cache-Control, which results in duplicate Cache-Control
headers; remove the expires directive and replace it with a single add_header
Cache-Control that includes the equivalent max-age plus the desired directives
and the always flag (e.g., add_header Cache-Control "private, immutable,
max-age=604800" always;), updating the location /_nuxt/ block to use only that
add_header to ensure one consistent header.
---
Duplicate comments:
In `@template/frontend/`{% if not deploy_as_executable %}default.conf.template{%
endif %}.jinja:
- Around line 57-59: The three nginx add_header directives (add_header
Cache-Control "no-cache, no-store, must-revalidate"; add_header Pragma
"no-cache"; add_header Expires "0";) are missing the always parameter so they
won't be sent on error responses; update each directive to include always (e.g.,
add_header Cache-Control "no-cache, no-store, must-revalidate" always;) to match
the existing headers that already include always and ensure no-cache headers are
attached for all response codes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f630996-2b98-41e7-a8f8-cc1ffd1b5fd2
📒 Files selected for processing (1)
template/frontend/{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja
template/frontend/{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
template/frontend/{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja (1)
56-58:⚠️ Potential issue | 🟡 MinorAdd
alwaysparameter to proxy cache control headers.The no-cache headers in
location@proxy`` are missing thealwaysparameter, unlike the identical headers in `location /` (lines 19-21). Without `always`, these headers won't be sent for error responses (4xx, 5xx) from the backend, potentially allowing browsers or intermediate caches to cache error pages.🔧 Proposed fix
location `@proxy` { - add_header Cache-Control "no-cache, no-store, must-revalidate"; - add_header Pragma "no-cache"; - add_header Expires "0"; + add_header Cache-Control "no-cache, no-store, must-revalidate" always; + add_header Pragma "no-cache" always; + add_header Expires "0" always; proxy_pass http://${BACKEND_HOST}:${BACKEND_PORT};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/frontend/`{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja around lines 56 - 58, In the location block named "@proxy" update the three add_header directives (add_header Cache-Control, add_header Pragma, add_header Expires) to include the always parameter so they are emitted for error responses as well; locate the add_header lines inside the location `@proxy` block in the template and append "always" to each add_header call to match the behavior used in the location / block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@template/frontend/`{% if not deploy_as_executable %}default.conf.template{%
endif %}.jinja:
- Around line 56-58: In the location block named "@proxy" update the three
add_header directives (add_header Cache-Control, add_header Pragma, add_header
Expires) to include the always parameter so they are emitted for error responses
as well; locate the add_header lines inside the location `@proxy` block in the
template and append "always" to each add_header call to match the behavior used
in the location / block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e97b5c7-6442-48e4-8bd5-0ebc5cf40116
📒 Files selected for processing (1)
template/frontend/{% if not deploy_as_executable %}default.conf.template{% endif %}.jinja
2 participants
Link to Issue or Message thread
#134
Why is this change necessary?
Realized that we did not have any caching setup while in this file.
How does this change address the issue?
Adds DO NOT cache settings on index.html and on anything /_nuxt/ related since they have hashes added to all files we can add some cache headers to allow it to not download them every time. Also on any proxy calls we should NOT cache those either.
What side effects does this change have?
How is this change tested?
Validated on a downstream repo that caching happens as expected
index.html first load
index.html second load
nuxt file first load
nuxt file second load (see cache from memory)
api-docs proxy through nginx you can see the new cache settings
Summary by CodeRabbit