You flagged it: the table blew out on mobile because of the long advice text, and the data was fine but the advice was unreadable.
The advice is several hundred characters. In a table cell it made the table wider than the phone, so the advice column was clipped off the right edge and you had to pan the table sideways to read it.
What changed
The advice is now a pop-out. A compact trigger in the cell, with the full text in a floating panel:
Desktop — opens on hover, closes when you move away.
Phone — opens on tap, centred on the screen, because a phone has no hover.
The trigger is a real <button>, so it works with a keyboard, and Escape closes it.
The panel is position: fixed and anchored by the script. My first attempt positioned it absolutely inside the cell, and the table is a scroll container — so the panel was clipped by the container's own overflow and the advice was sliced in half by the card's edge, with the bottom border cut off. Fixed positioning takes it out of the table entirely. It flips above the trigger when there is not enough room below.
The tables now reflow into stacked cards on a phone. Each value carries its column name as a label. A seven-column table does not fit a phone, and it used to scroll sideways inside its wrapper, so the columns on the right were off the screen entirely.
The header wraps onto two rows so the nav tabs are not clipped mid-word, and the tabs are tightened so all four fit without scrolling.
Also small: the point scale says "points" rather than "pts" (it was an unlabelled line on the phone), the reading count on the beer card no longer orphans its word, and the three stat captions on a beer card share a baseline.
Verified
Measured in the browser at 390px and at 1280px, on every page:
No page overflows — /brews, /active, /history, /analysis and /runs/<id> all have scrollWidth == innerWidth.
The table fits — 597px wide in a 390px viewport before, 351px after.
The panel is fully visible — all four borders intact, the complete text readable, on screen at both widths.
The panel is centred on a phone — 275px gaps above and below.
The desktop layout is untouched — the table is still 806px, the header row still shows.
183 tests pass.
Test plan
183 tests pass
A new regression test that the advice in the table is a trigger with the text in a panel, so it cannot silently go back to being the text
Browser-measured at 390px and 1280px on every page
The panel opens on hover on a desktop and on tap on a phone
No overflow on any page
## Why
You flagged it: the table blew out on mobile because of the long advice text, and the data was fine but the advice was unreadable.
The advice is several hundred characters. In a table cell it made the table wider than the phone, so the advice column was clipped off the right edge and you had to pan the table sideways to read it.
## What changed
**The advice is now a pop-out.** A compact trigger in the cell, with the full text in a floating panel:
- **Desktop** — opens on hover, closes when you move away.
- **Phone** — opens on tap, centred on the screen, because a phone has no hover.
- The trigger is a real `<button>`, so it works with a keyboard, and `Escape` closes it.
**The panel is `position: fixed` and anchored by the script.** My first attempt positioned it absolutely inside the cell, and the table is a scroll container — so the panel was clipped by the container's own overflow and the advice was sliced in half by the card's edge, with the bottom border cut off. Fixed positioning takes it out of the table entirely. It flips above the trigger when there is not enough room below.
**The tables now reflow into stacked cards on a phone.** Each value carries its column name as a label. A seven-column table does not fit a phone, and it used to scroll sideways inside its wrapper, so the columns on the right were off the screen entirely.
**The header wraps onto two rows** so the nav tabs are not clipped mid-word, and the tabs are tightened so all four fit without scrolling.
Also small: the point scale says "points" rather than "pts" (it was an unlabelled line on the phone), the reading count on the beer card no longer orphans its word, and the three stat captions on a beer card share a baseline.
## Verified
Measured in the browser at 390px and at 1280px, on every page:
- **No page overflows** — `/brews`, `/active`, `/history`, `/analysis` and `/runs/<id>` all have `scrollWidth == innerWidth`.
- **The table fits** — 597px wide in a 390px viewport before, 351px after.
- **The panel is fully visible** — all four borders intact, the complete text readable, on screen at both widths.
- **The panel is centred on a phone** — 275px gaps above and below.
- **The desktop layout is untouched** — the table is still 806px, the header row still shows.
183 tests pass.
## Test plan
- [x] 183 tests pass
- [x] A new regression test that the advice in the table is a trigger with the text in a panel, so it cannot silently go back to being the text
- [x] Browser-measured at 390px and 1280px on every page
- [x] The panel opens on hover on a desktop and on tap on a phone
- [x] No overflow on any page
The brewing advice is several hundred characters. In a table cell it
pushed the table past the width of a phone, so the column was clipped
off the right edge and the advice could not be read without sideways
scrolling.
The advice is now a compact trigger in the cell and the text is in a
floating panel that opens on hover (desktop) or on tap (phone).
The panel is position:fixed and anchored by the script. Absolutely
positioned inside the cell it was clipped by the table's own scroll
container, and the advice was sliced in half by the card's edge. It
flips above the trigger when there is no room below.
The tables now reflow into stacked cards on a narrow screen, with each
value labelled by its column name. A seven column table does not fit a
phone, and it used to scroll sideways inside its wrapper, so the
columns on the right were off the screen.
Also: the header wraps onto two rows so the nav tabs are not clipped,
the point scale says 'points' rather than 'pts', and the reading count
on the beer card does not orphan its word.
Unified Code Review Report: Mobile Pop-out & Table Responsiveness
1. Executive Summary
This Pull Request addresses a critical UX failure regarding horizontal overflow of "advice" text in data tables on mobile devices. The implementation introduces a sophisticated "pop-out" panel using position: fixed to bypass container clipping, converts tables to stacked cards for mobile viewports, and implements keyboard accessibility.
While the architectural approach is sound and the UX improvements are significant, there is a potential high-risk security vulnerability regarding DOM-based XSS that must be verified before merging. Additionally, there are opportunities to optimize the CSS footprint and ensure JavaScript stability during window resizing.
2. Prioritized List of Issues
🔴 Critical / High Priority
Potential DOM-based XSS (Security): The JavaScript logic in app.js that populates the pop-out panel must be verified. If .innerHTML is used to inject advice text from the DOM/data-attributes, it opens a vector for script injection. (Action Required)
🟡 Medium Priority
Fixed Positioning Stability (Code): The panel is "anchored by script." If coordinates are not recalculated on window.onresize or scroll, the panel will be misaligned.
Event Listener Memory Leaks (Code): Global listeners for the Escape key and hover events in app.js must be properly cleaned up to avoid "zombie" listeners.
CSS Scalability (Code): An addition of 262 lines of CSS for one feature suggests a lack of reusable utility classes or an inefficient approach to the "stacked card" layout.
🔵 Low Priority
UX Hover Flicker (Code): Risk of "flickering" if the pop-out panel does not have a small buffer zone or is not itself hoverable.
Asset Optimization (Infrastructure): Growing static asset sizes (app.css/app.js) may eventually impact mobile load times without minification or CDN caching.
3. Domain-Specific Recommendations
💻 Code & Architecture
JavaScript Logic:
Wrap positioning logic in a function triggered by both the trigger event and the window.onresize event.
Ensure the Escape key listener is removed when the panel is closed.
CSS Strategy:
Refactor the "stacked card" layout using CSS Grid/Flexbox with a single media query and data-label attributes to reduce the CSS footprint.
Extract repetitive table styles into a shared .table-responsive-card utility class.
Interaction:
Implement a "buffer" or make the panel itself a hover target to prevent the panel from closing prematurely when the user moves the mouse from the trigger to the content.
🛡️ Security
Injection Prevention:Mandatory: Replace any instance of .innerHTML with .textContent or .innerText when populating the pop-out panel in app.js.
Template Safety: Confirm that the Jinja2 templates (brews.html, active.html) are not using the |safe filter on the advice text.
CSP Alignment: Ensure that the use of script-driven inline styles does not conflict with the existing Content Security Policy (CSP).
🏗️ Infrastructure
Deployment Pipeline: Integrate headless browser testing (e.g., Playwright or Selenium) into the CI pipeline to automate the verification of layout responsiveness.
Build Process: Introduce a minification step (e.g., Vite, Webpack, or a simple CSS/JS minifier) to reduce payload size for mobile clients.
Caching: Ensure Flask production settings include proper Cache-Control headers for these updated static assets.
4. Positive Aspects of the PR
Technical Precision: The transition to position: fixed correctly solves the overflow: hidden clipping issue inherent in table layouts.
Accessibility Focus: Excellent use of <button> elements for triggers and the inclusion of Escape key support for keyboard users.
Verification Rigor: High attention to detail in testing specific viewport widths (390px vs 1280px) and validating scrollWidth.
Testing Maturity: The addition of a dedicated regression test (test_brew_advice.py) ensures long-term stability of the fix.
5. Overall Assessment & Recommendation
Recommendation: ⚠️ APPROVED WITH MINOR CHANGES (Conditional)
The PR is high-quality from a functional and UX perspective. However, it cannot be merged until the security concern regarding .innerHTML is addressed.
Required Actions before Merge:
Security Audit: Developer must confirm the use of .textContent in app.js.
Logic Update: Implement window.onresize handling for the fixed panel positioning.
Cleanup: Confirm that the Escape listener is managed to prevent memory leaks.
## PR Review Results
# Unified Code Review Report: Mobile Pop-out & Table Responsiveness
## 1. Executive Summary
This Pull Request addresses a critical UX failure regarding horizontal overflow of "advice" text in data tables on mobile devices. The implementation introduces a sophisticated "pop-out" panel using `position: fixed` to bypass container clipping, converts tables to stacked cards for mobile viewports, and implements keyboard accessibility.
While the architectural approach is sound and the UX improvements are significant, there is a **potential high-risk security vulnerability** regarding DOM-based XSS that must be verified before merging. Additionally, there are opportunities to optimize the CSS footprint and ensure JavaScript stability during window resizing.
---
## 2. Prioritized List of Issues
### 🔴 Critical / High Priority
* **Potential DOM-based XSS (Security):** The JavaScript logic in `app.js` that populates the pop-out panel must be verified. If `.innerHTML` is used to inject advice text from the DOM/data-attributes, it opens a vector for script injection. **(Action Required)**
### 🟡 Medium Priority
* **Fixed Positioning Stability (Code):** The panel is "anchored by script." If coordinates are not recalculated on `window.onresize` or `scroll`, the panel will be misaligned.
* **Event Listener Memory Leaks (Code):** Global listeners for the `Escape` key and hover events in `app.js` must be properly cleaned up to avoid "zombie" listeners.
* **CSS Scalability (Code):** An addition of 262 lines of CSS for one feature suggests a lack of reusable utility classes or an inefficient approach to the "stacked card" layout.
### 🔵 Low Priority
* **UX Hover Flicker (Code):** Risk of "flickering" if the pop-out panel does not have a small buffer zone or is not itself hoverable.
* **Asset Optimization (Infrastructure):** Growing static asset sizes (`app.css`/`app.js`) may eventually impact mobile load times without minification or CDN caching.
---
## 3. Domain-Specific Recommendations
### 💻 Code & Architecture
* **JavaScript Logic:**
* Wrap positioning logic in a function triggered by both the `trigger event` and the `window.onresize` event.
* Ensure the `Escape` key listener is removed when the panel is closed.
* **CSS Strategy:**
* Refactor the "stacked card" layout using **CSS Grid/Flexbox** with a single media query and `data-label` attributes to reduce the CSS footprint.
* Extract repetitive table styles into a shared `.table-responsive-card` utility class.
* **Interaction:**
* Implement a "buffer" or make the panel itself a hover target to prevent the panel from closing prematurely when the user moves the mouse from the trigger to the content.
### 🛡️ Security
* **Injection Prevention:** **Mandatory:** Replace any instance of `.innerHTML` with `.textContent` or `.innerText` when populating the pop-out panel in `app.js`.
* **Template Safety:** Confirm that the Jinja2 templates (`brews.html`, `active.html`) are not using the `|safe` filter on the advice text.
* **CSP Alignment:** Ensure that the use of script-driven inline styles does not conflict with the existing Content Security Policy (CSP).
### 🏗️ Infrastructure
* **Deployment Pipeline:** Integrate headless browser testing (e.g., Playwright or Selenium) into the CI pipeline to automate the verification of layout responsiveness.
* **Build Process:** Introduce a minification step (e.g., Vite, Webpack, or a simple CSS/JS minifier) to reduce payload size for mobile clients.
* **Caching:** Ensure Flask production settings include proper `Cache-Control` headers for these updated static assets.
---
## 4. Positive Aspects of the PR
* **Technical Precision:** The transition to `position: fixed` correctly solves the `overflow: hidden` clipping issue inherent in table layouts.
* **Accessibility Focus:** Excellent use of `<button>` elements for triggers and the inclusion of `Escape` key support for keyboard users.
* **Verification Rigor:** High attention to detail in testing specific viewport widths (390px vs 1280px) and validating `scrollWidth`.
* **Testing Maturity:** The addition of a dedicated regression test (`test_brew_advice.py`) ensures long-term stability of the fix.
---
## 5. Overall Assessment & Recommendation
**Recommendation: ⚠️ APPROVED WITH MINOR CHANGES (Conditional)**
The PR is high-quality from a functional and UX perspective. However, it **cannot be merged** until the security concern regarding `.innerHTML` is addressed.
**Required Actions before Merge:**
1. **Security Audit:** Developer must confirm the use of `.textContent` in `app.js`.
2. **Logic Update:** Implement `window.onresize` handling for the fixed panel positioning.
3. **Cleanup:** Confirm that the `Escape` listener is managed to prevent memory leaks.
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
You flagged it: the table blew out on mobile because of the long advice text, and the data was fine but the advice was unreadable.
The advice is several hundred characters. In a table cell it made the table wider than the phone, so the advice column was clipped off the right edge and you had to pan the table sideways to read it.
What changed
The advice is now a pop-out. A compact trigger in the cell, with the full text in a floating panel:
<button>, so it works with a keyboard, andEscapecloses it.The panel is
position: fixedand anchored by the script. My first attempt positioned it absolutely inside the cell, and the table is a scroll container — so the panel was clipped by the container's own overflow and the advice was sliced in half by the card's edge, with the bottom border cut off. Fixed positioning takes it out of the table entirely. It flips above the trigger when there is not enough room below.The tables now reflow into stacked cards on a phone. Each value carries its column name as a label. A seven-column table does not fit a phone, and it used to scroll sideways inside its wrapper, so the columns on the right were off the screen entirely.
The header wraps onto two rows so the nav tabs are not clipped mid-word, and the tabs are tightened so all four fit without scrolling.
Also small: the point scale says "points" rather than "pts" (it was an unlabelled line on the phone), the reading count on the beer card no longer orphans its word, and the three stat captions on a beer card share a baseline.
Verified
Measured in the browser at 390px and at 1280px, on every page:
/brews,/active,/history,/analysisand/runs/<id>all havescrollWidth == innerWidth.183 tests pass.
Test plan
PR received — starting review, sit tight 🫡
PR Review Results
Unified Code Review Report: Mobile Pop-out & Table Responsiveness
1. Executive Summary
This Pull Request addresses a critical UX failure regarding horizontal overflow of "advice" text in data tables on mobile devices. The implementation introduces a sophisticated "pop-out" panel using
position: fixedto bypass container clipping, converts tables to stacked cards for mobile viewports, and implements keyboard accessibility.While the architectural approach is sound and the UX improvements are significant, there is a potential high-risk security vulnerability regarding DOM-based XSS that must be verified before merging. Additionally, there are opportunities to optimize the CSS footprint and ensure JavaScript stability during window resizing.
2. Prioritized List of Issues
🔴 Critical / High Priority
app.jsthat populates the pop-out panel must be verified. If.innerHTMLis used to inject advice text from the DOM/data-attributes, it opens a vector for script injection. (Action Required)🟡 Medium Priority
window.onresizeorscroll, the panel will be misaligned.Escapekey and hover events inapp.jsmust be properly cleaned up to avoid "zombie" listeners.🔵 Low Priority
app.css/app.js) may eventually impact mobile load times without minification or CDN caching.3. Domain-Specific Recommendations
💻 Code & Architecture
trigger eventand thewindow.onresizeevent.Escapekey listener is removed when the panel is closed.data-labelattributes to reduce the CSS footprint..table-responsive-cardutility class.🛡️ Security
.innerHTMLwith.textContentor.innerTextwhen populating the pop-out panel inapp.js.brews.html,active.html) are not using the|safefilter on the advice text.🏗️ Infrastructure
Cache-Controlheaders for these updated static assets.4. Positive Aspects of the PR
position: fixedcorrectly solves theoverflow: hiddenclipping issue inherent in table layouts.<button>elements for triggers and the inclusion ofEscapekey support for keyboard users.scrollWidth.test_brew_advice.py) ensures long-term stability of the fix.5. Overall Assessment & Recommendation
Recommendation: ⚠️ APPROVED WITH MINOR CHANGES (Conditional)
The PR is high-quality from a functional and UX perspective. However, it cannot be merged until the security concern regarding
.innerHTMLis addressed.Required Actions before Merge:
.textContentinapp.js.window.onresizehandling for the fixed panel positioning.Escapelistener is managed to prevent memory leaks.