The screenshot you sent showed the old inline advice - a long paragraph in the table cell - with the button label reading "Read the adviceAdvice".
That was Firefox's cache, serving a copy of the page from before PR #16. The deployed pod had the new markup all along (verified: advice-pop__text is in the template inside the pod, and the live server returns the button + a hidden position: fixed pop-out).
Two things were needed.
The fix
1. The pages must not be cached. They had no cache headers at all, so Firefox was free to reuse them indefinitely. The HTML now sends:
2. The label duplication was a real bug that a stale stylesheet exposed. The rule that hides the short label was scoped to table.data .short, so it did not apply to the advice button's label:
table.data.short{display:none;}
The button's label is not inside table.data for CSS purposes on every page, so a stylesheet that was not quite the current one showed both the long and the short form at once - "Read the advice" + "Advice".
The rule is now on .short alone:
.short{display:none;}/* base - desktop shows the long form */@media(max-width:720px){.long{display:none;}/* phone shows the short form */.short{display:inline;}}
It now applies wherever the pattern is used - a table heading, the advice button, the Active Beers page - so only one form can ever show.
Verified
Measured in the browser at 390px and 1280px:
Phone (390px): the button reads Advice, the headings read RUN / NAME / GRAVITY / ABV / NOTE / ADVICE
Desktop (1280px): the button reads Read the advice, the headings read RUN / NAME / GRAVITY / EST. ABV / COMMENT / BREWING ADVICE
No element shows both forms - checked every .long / .short pair on the page
The advice is not in the table - 0 .advice blocks inside table.data, the pop-out is position: fixed and hidden until opened
The pop-out still opens on hover on a desktop and on tap on a phone
192 tests pass, including a new one that asserts both label forms are in the markup, and one that asserts the pages send no-store.
Test plan
192 tests pass
Browser-measured at 390px and 1280px on /brews
Every .long / .short pair checked for both being visible
The cache header checked on the response
On your phone: the fix stops this happening again, but the copy Firefox already has may still be served once. A hard refresh (Ctrl+Shift+R) or clearing the cache for that site will pull the current page.
## Why
The screenshot you sent showed the old inline advice - a long paragraph in the table cell - with the button label reading **"Read the adviceAdvice"**.
That was **Firefox's cache**, serving a copy of the page from before PR #16. The deployed pod had the new markup all along (verified: `advice-pop__text` is in the template inside the pod, and the live server returns the button + a hidden `position: fixed` pop-out).
Two things were needed.
## The fix
**1. The pages must not be cached.** They had no cache headers at all, so Firefox was free to reuse them indefinitely. The HTML now sends:
```
Cache-Control: no-store, must-revalidate
Pragma: no-cache
```
**2. The label duplication was a real bug that a stale stylesheet exposed.** The rule that hides the short label was scoped to `table.data .short`, so it did not apply to the advice button's label:
```css
table.data .short { display: none; }
```
The button's label is not inside `table.data` for CSS purposes on every page, so a stylesheet that was not quite the current one showed both the long and the short form at once - "Read the advice" + "Advice".
The rule is now on `.short` alone:
```css
.short { display: none; } /* base - desktop shows the long form */
@media (max-width: 720px) {
.long { display: none; } /* phone shows the short form */
.short { display: inline; }
}
```
It now applies wherever the pattern is used - a table heading, the advice button, the Active Beers page - so only one form can ever show.
## Verified
Measured in the browser at 390px and 1280px:
- **Phone (390px):** the button reads `Advice`, the headings read `RUN / NAME / GRAVITY / ABV / NOTE / ADVICE`
- **Desktop (1280px):** the button reads `Read the advice`, the headings read `RUN / NAME / GRAVITY / EST. ABV / COMMENT / BREWING ADVICE`
- **No element shows both forms** - checked every `.long` / `.short` pair on the page
- **The advice is not in the table** - 0 `.advice` blocks inside `table.data`, the pop-out is `position: fixed` and hidden until opened
- **The pop-out still opens on hover** on a desktop and on tap on a phone
192 tests pass, including a new one that asserts both label forms are in the markup, and one that asserts the pages send `no-store`.
## Test plan
- [x] 192 tests pass
- [x] Browser-measured at 390px and 1280px on `/brews`
- [x] Every `.long` / `.short` pair checked for both being visible
- [x] The cache header checked on the response
**On your phone:** the fix stops this happening again, but the copy Firefox already has may still be served once. A hard refresh (Ctrl+Shift+R) or clearing the cache for that site will pull the current page.
Firefox served a cached copy of the brewing page after the advice
became a button and a pop-out, so the phone kept showing the old
inline advice - a long paragraph in the table cell, with the button
label reading "Read the adviceAdvice".
Two separate causes:
The pages had no cache headers, so Firefox was free to reuse them.
They now send Cache-Control: no-store, must-revalidate.
The rule that hides the short label was scoped to `table.data .short`,
so it did not apply to the advice button's label. A stale stylesheet
then showed both forms at once. The rule is now on `.short` alone, so
it applies wherever the pattern is used - a table heading, the advice
button, the Active Beers page - and only ever one form can show.
Verified at 390px and 1280px: the phone shows "Advice", the desktop
shows "Read the advice" and "BREWING ADVICE", and no element ever
shows both.
192 tests pass.
PR Title:fix(ui): stop the pages being cached, and the labels duplicating Status:✅Approved with Minor Recommendations
1. Executive Summary
This Pull Request addresses two primary issues: a browser caching problem resulting in stale UI and a CSS bug causing duplicate labels in responsive views.
Across all three review domains (Code, Security, and Infrastructure), the consensus is that the PR is technically sound, well-tested, and safe to merge. The implementation effectively solves the reported bugs. The primary point of discussion across reviews is the "blanket" approach to disabling caching via no-store, which—while secure and effective—introduces minor concerns regarding long-term server performance and architectural layering.
2. Prioritized Findings & Issues
Priority
Issue
Domain
Description
Low
Global Caching Strategy
Infra / Sec
Using no-store globally in the app factory may increase server load and degrade performance for static-like pages.
Low
Architectural Layering
Infra
Cache headers are handled in the Python application rather than at the Ingress/CDN layer.
Low
Generic CSS Naming
Code
.short and .long classes are very generic and may cause collisions as the project scales.
Low
Legacy Header Usage
Sec / Infra
Pragma: no-cache is an HTTP/1.0 legacy header and is largely redundant.
3. Domain-Specific Recommendations
💻 Code Quality
CSS Naming: Transition from generic classes (.short, .long) to a more specific naming convention (e.g., .label-short or BEM .responsive-text--short) to avoid future style conflicts.
Selective Caching: As the app grows, move away from a global factory setting for cache headers. Implement a decorator or route-specific handler to allow certain static pages to be cached while keeping dynamic pages fresh.
🛡️ Security
Header Optimization: If the pages do not contain sensitive PII, consider moving from no-store to Cache-Control: no-cache, max-age=0. This allows caching but forces revalidation, balancing security with performance.
CSP Implementation: While not a flaw in this PR, it is recommended to ensure a strong Content Security Policy (CSP) is active to complement the UI changes and prevent style/script injection.
⚙️ Infrastructure
Offload to Ingress: For long-term maintainability, move Cache-Control logic from app_factory.py to the Kubernetes Ingress manifest (e.g., NGINX annotations). This separates delivery concerns from business logic.
Header Cleanup: Phase out Pragma: no-cache unless support for legacy HTTP/1.0 clients is a documented requirement.
4. Positive Highlights
The review team highlighted several exemplary aspects of this PR:
Rigorous Testing: The author went beyond the fix by adding regression tests for both HTTP headers and DOM elements, ensuring the fix is durable.
DRY Principles: The CSS refactor successfully decoupled styling from specific HTML elements, creating a reusable responsive pattern.
Defensive Posture: The choice of no-store is the safest possible security configuration for preventing sensitive data persistence in browser caches.
Surgical Precision: The changes are focused and address the bugs without introducing unnecessary "scope creep."
5. Final Assessment
Overall Rating: ✅ Approved
The PR is ready for merge. The identified issues are categorized as Low Priority and are primarily focused on long-term architectural optimization rather than immediate correctness or security risks. The developer is encouraged to note the suggestions regarding Ingress-level header management and CSS naming for future iterations.
Verdict: Merge as-is; track long-term infrastructure improvements as technical debt.
## PR Review Results
# Unified Review Synthesis Report
**PR Title:** `fix(ui): stop the pages being cached, and the labels duplicating`
**Status:** ✅ **Approved with Minor Recommendations**
---
## 1. Executive Summary
This Pull Request addresses two primary issues: a browser caching problem resulting in stale UI and a CSS bug causing duplicate labels in responsive views.
Across all three review domains (Code, Security, and Infrastructure), the consensus is that the PR is **technically sound, well-tested, and safe to merge**. The implementation effectively solves the reported bugs. The primary point of discussion across reviews is the "blanket" approach to disabling caching via `no-store`, which—while secure and effective—introduces minor concerns regarding long-term server performance and architectural layering.
---
## 2. Prioritized Findings & Issues
| Priority | Issue | Domain | Description |
| :--- | :--- | :--- | :--- |
| **Low** | **Global Caching Strategy** | Infra / Sec | Using `no-store` globally in the app factory may increase server load and degrade performance for static-like pages. |
| **Low** | **Architectural Layering** | Infra | Cache headers are handled in the Python application rather than at the Ingress/CDN layer. |
| **Low** | **Generic CSS Naming** | Code | `.short` and `.long` classes are very generic and may cause collisions as the project scales. |
| **Low** | **Legacy Header Usage** | Sec / Infra | `Pragma: no-cache` is an HTTP/1.0 legacy header and is largely redundant. |
---
## 3. Domain-Specific Recommendations
### 💻 Code Quality
* **CSS Naming:** Transition from generic classes (`.short`, `.long`) to a more specific naming convention (e.g., `.label-short` or BEM `.responsive-text--short`) to avoid future style conflicts.
* **Selective Caching:** As the app grows, move away from a global factory setting for cache headers. Implement a decorator or route-specific handler to allow certain static pages to be cached while keeping dynamic pages fresh.
### 🛡️ Security
* **Header Optimization:** If the pages do not contain sensitive PII, consider moving from `no-store` to `Cache-Control: no-cache, max-age=0`. This allows caching but forces revalidation, balancing security with performance.
* **CSP Implementation:** While not a flaw in this PR, it is recommended to ensure a strong Content Security Policy (CSP) is active to complement the UI changes and prevent style/script injection.
### ⚙️ Infrastructure
* **Offload to Ingress:** For long-term maintainability, move `Cache-Control` logic from `app_factory.py` to the Kubernetes Ingress manifest (e.g., NGINX annotations). This separates delivery concerns from business logic.
* **Header Cleanup:** Phase out `Pragma: no-cache` unless support for legacy HTTP/1.0 clients is a documented requirement.
---
## 4. Positive Highlights
The review team highlighted several exemplary aspects of this PR:
* **Rigorous Testing:** The author went beyond the fix by adding regression tests for both HTTP headers and DOM elements, ensuring the fix is durable.
* **DRY Principles:** The CSS refactor successfully decoupled styling from specific HTML elements, creating a reusable responsive pattern.
* **Defensive Posture:** The choice of `no-store` is the safest possible security configuration for preventing sensitive data persistence in browser caches.
* **Surgical Precision:** The changes are focused and address the bugs without introducing unnecessary "scope creep."
---
## 5. Final Assessment
**Overall Rating: ✅ Approved**
The PR is ready for merge. The identified issues are categorized as **Low Priority** and are primarily focused on long-term architectural optimization rather than immediate correctness or security risks. The developer is encouraged to note the suggestions regarding Ingress-level header management and CSS naming for future iterations.
**Verdict:** Merge as-is; track long-term infrastructure improvements as technical debt.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Why
The screenshot you sent showed the old inline advice - a long paragraph in the table cell - with the button label reading "Read the adviceAdvice".
That was Firefox's cache, serving a copy of the page from before PR #16. The deployed pod had the new markup all along (verified:
advice-pop__textis in the template inside the pod, and the live server returns the button + a hiddenposition: fixedpop-out).Two things were needed.
The fix
1. The pages must not be cached. They had no cache headers at all, so Firefox was free to reuse them indefinitely. The HTML now sends:
2. The label duplication was a real bug that a stale stylesheet exposed. The rule that hides the short label was scoped to
table.data .short, so it did not apply to the advice button's label:The button's label is not inside
table.datafor CSS purposes on every page, so a stylesheet that was not quite the current one showed both the long and the short form at once - "Read the advice" + "Advice".The rule is now on
.shortalone:It now applies wherever the pattern is used - a table heading, the advice button, the Active Beers page - so only one form can ever show.
Verified
Measured in the browser at 390px and 1280px:
Advice, the headings readRUN / NAME / GRAVITY / ABV / NOTE / ADVICERead the advice, the headings readRUN / NAME / GRAVITY / EST. ABV / COMMENT / BREWING ADVICE.long/.shortpair on the page.adviceblocks insidetable.data, the pop-out isposition: fixedand hidden until opened192 tests pass, including a new one that asserts both label forms are in the markup, and one that asserts the pages send
no-store.Test plan
/brews.long/.shortpair checked for both being visibleOn your phone: the fix stops this happening again, but the copy Firefox already has may still be served once. A hard refresh (Ctrl+Shift+R) or clearing the cache for that site will pull the current page.
PR received — starting review, sit tight 🫡
PR Review Results
Unified Review Synthesis Report
PR Title:
fix(ui): stop the pages being cached, and the labels duplicatingStatus: ✅ Approved with Minor Recommendations
1. Executive Summary
This Pull Request addresses two primary issues: a browser caching problem resulting in stale UI and a CSS bug causing duplicate labels in responsive views.
Across all three review domains (Code, Security, and Infrastructure), the consensus is that the PR is technically sound, well-tested, and safe to merge. The implementation effectively solves the reported bugs. The primary point of discussion across reviews is the "blanket" approach to disabling caching via
no-store, which—while secure and effective—introduces minor concerns regarding long-term server performance and architectural layering.2. Prioritized Findings & Issues
no-storeglobally in the app factory may increase server load and degrade performance for static-like pages..shortand.longclasses are very generic and may cause collisions as the project scales.Pragma: no-cacheis an HTTP/1.0 legacy header and is largely redundant.3. Domain-Specific Recommendations
💻 Code Quality
.short,.long) to a more specific naming convention (e.g.,.label-shortor BEM.responsive-text--short) to avoid future style conflicts.🛡️ Security
no-storetoCache-Control: no-cache, max-age=0. This allows caching but forces revalidation, balancing security with performance.⚙️ Infrastructure
Cache-Controllogic fromapp_factory.pyto the Kubernetes Ingress manifest (e.g., NGINX annotations). This separates delivery concerns from business logic.Pragma: no-cacheunless support for legacy HTTP/1.0 clients is a documented requirement.4. Positive Highlights
The review team highlighted several exemplary aspects of this PR:
no-storeis the safest possible security configuration for preventing sensitive data persistence in browser caches.5. Final Assessment
Overall Rating: ✅ Approved
The PR is ready for merge. The identified issues are categorized as Low Priority and are primarily focused on long-term architectural optimization rather than immediate correctness or security risks. The developer is encouraged to note the suggestions regarding Ingress-level header management and CSS naming for future iterations.
Verdict: Merge as-is; track long-term infrastructure improvements as technical debt.