Adds the analysis page the old app never had — how consistently the brews ferment, and how long each one takes.
This is the third and last of the split PRs. PR1 rebuilt the UI and app foundation (#4), PR2 added the active beers page and run metadata (#5).
What it shows
Attenuation by brew — what fraction of the original gravity was fermented away, for every finished brew.
Fermentation curves — the typical shape of a fermentation, as a median curve with the min-max envelope behind it. Plotting every brew individually gave 16 near-identical lines and an unreadable legend.
Every finished brew — OG, FG, ABV, attenuation, days in the fermenter, days to reach 1.010, finish date.
Readings that need a second look — readings outside the plausible hydrometer range.
Headline figures: average attenuation and its spread, average brew time, average time to 1.010.
Data quality — one run was corrupting the averages
Live run 12 has a final gravity of 900. That works out as 338% attenuation and dragged the average to 103.2%. That is not a real result, so:
Attenuation is rejected when the final gravity is not physically possible for the original gravity.
Runs failing that check are excluded from the averages rather than silently distorting them, and are listed on the page as needing review.
The average now reads 87.5% ± 7.4pp across 16 brews.
Chart bugs found while building this
These are the kind of thing that looks fine in a screenshot and is wrong:
The gravity charts had their y axis reversed. With reverse: true the high reading sits at the bottom, so the fermentation curve climbed left to right — it read as gravity rising as the beer fermented. A brewer reads a fermentation as a descending line. The point-scale run chart on the run detail page had the same bug. Both fixed.
The curve chart's x axis was treated as categorical. The x values are numeric (0–100), and Chart.js defaults a line chart's x axis to category, which collapsed every point onto index 0 and rendered the curve as a sliver at the origin. Now explicitly linear.
The curve was interpolated by exact key lookup, so it only worked when every brew happened to be read at the same progress points. It now interpolates between a brew's actual readings — 21 points across the full range.
Long beer names were clipped at the left edge of the attenuation chart ("...x crying out loud"). Names are trimmed to 18 characters with an ellipsis; the tooltip and the table carry the full name.
Verification
127 tests pass (up from 105).
The two chart regressions are covered by reading charts.js and asserting the gravity axis is not reversed and that long names are trimmed. A screenshot test would not have caught either — these changes silently revert.
Browser-verified against the real Mongo data: the analysis page renders, both charts draw, the median curve descends, and the attenuation figures read 87.5% / ±7.4pp.
Test plan
127 tests pass
Browser check against real Mongo: /analysis renders, both charts draw data
The fermentation curve descends left to right (verified by pixel position, not by eye)
The attenuation chart labels are no longer clipped
The suspect run (run 12) is excluded from the averages and surfaced for review
Not yet deployed. The CI workflow deploys from master on merge — I will confirm the rollout and the live page afterwards.
## Summary
Adds the analysis page the old app never had — how consistently the brews ferment, and how long each one takes.
This is the third and last of the split PRs. PR1 rebuilt the UI and app foundation (#4), PR2 added the active beers page and run metadata (#5).
### What it shows
- **Attenuation by brew** — what fraction of the original gravity was fermented away, for every finished brew.
- **Fermentation curves** — the typical shape of a fermentation, as a median curve with the min-max envelope behind it. Plotting every brew individually gave 16 near-identical lines and an unreadable legend.
- **Every finished brew** — OG, FG, ABV, attenuation, days in the fermenter, days to reach 1.010, finish date.
- **Readings that need a second look** — readings outside the plausible hydrometer range.
- Headline figures: average attenuation and its spread, average brew time, average time to 1.010.
### Data quality — one run was corrupting the averages
Live run 12 has a final gravity of `900`. That works out as 338% attenuation and dragged the average to 103.2%. That is not a real result, so:
- Attenuation is rejected when the final gravity is not physically possible for the original gravity.
- Runs failing that check are **excluded from the averages** rather than silently distorting them, and are listed on the page as needing review.
- The average now reads **87.5% ± 7.4pp** across 16 brews.
### Chart bugs found while building this
These are the kind of thing that looks fine in a screenshot and is wrong:
- **The gravity charts had their y axis reversed.** With `reverse: true` the high reading sits at the *bottom*, so the fermentation curve climbed left to right — it read as gravity **rising** as the beer fermented. A brewer reads a fermentation as a descending line. The point-scale run chart on the run detail page had the same bug. Both fixed.
- **The curve chart's x axis was treated as categorical.** The x values are numeric (0–100), and Chart.js defaults a line chart's x axis to category, which collapsed every point onto index 0 and rendered the curve as a sliver at the origin. Now explicitly linear.
- **The curve was interpolated by exact key lookup**, so it only worked when every brew happened to be read at the same progress points. It now interpolates between a brew's actual readings — 21 points across the full range.
- **Long beer names were clipped** at the left edge of the attenuation chart (`"...x crying out loud"`). Names are trimmed to 18 characters with an ellipsis; the tooltip and the table carry the full name.
### Verification
- 127 tests pass (up from 105).
- The two chart regressions are covered by reading `charts.js` and asserting the gravity axis is not reversed and that long names are trimmed. A screenshot test would not have caught either — these changes silently revert.
- Browser-verified against the real Mongo data: the analysis page renders, both charts draw, the median curve descends, and the attenuation figures read 87.5% / ±7.4pp.
## Test plan
- [x] 127 tests pass
- [x] Browser check against real Mongo: `/analysis` renders, both charts draw data
- [x] The fermentation curve descends left to right (verified by pixel position, not by eye)
- [x] The attenuation chart labels are no longer clipped
- [x] The suspect run (run 12) is excluded from the averages and surfaced for review
- [ ] **Not yet deployed.** The CI workflow deploys from `master` on merge — I will confirm the rollout and the live page afterwards.
Adds the analysis page the old app never had: how consistently the brews
ferment, and how long each one takes.
What it shows
-------------
- **Attenuation by brew** - what fraction of the original gravity was
fermented away, for every finished brew.
- **Fermentation curves** - the typical shape of a fermentation, drawn as a
median curve with the min-max envelope behind it. Plotting every brew
individually gave 16 near-identical lines and an unreadable legend.
- **Every finished brew** - OG, FG, ABV, attenuation, days in the fermenter,
days to reach 1.010, and the finish date.
- **Readings that need a second look** - readings outside the plausible
hydrometer range.
- Headline figures: average attenuation and its spread, average brew time,
average time to 1.010.
Data quality
------------
One live run (run 12) has a final gravity of 900, which worked out as 338%
attenuation and dragged the average to 103.2%. That is not a real result,
so:
- Attenuation is rejected when the final gravity is not physically possible
for the original gravity.
- Runs that fail that check are EXCLUDED from the averages rather than
silently distorting them, and are listed on the page as needing review.
- The average now reads 87.5% +/- 7.4 percentage points across 16 brews.
Chart bugs found while building this
------------------------------------
- **The gravity charts had their y axis reversed.** With `reverse: true`
the high reading sits at the BOTTOM, so the fermentation curve climbed
left to right - it read as gravity *rising* as the beer fermented. A
brewer reads a fermentation as a descending line. The point-scale run
chart on the run detail page had the same bug. Both are fixed.
- **The curve chart's x axis was treated as categorical.** The x values are
numeric (0-100), and Chart.js defaults a line chart's x axis to
category, which collapsed every point onto index 0 and rendered the
curve as a sliver at the origin. The axis is now explicitly linear.
- **The curve was interpolated by exact key lookup**, so it only worked when
every brew happened to be read at the same progress points. It now
interpolates between a brew's actual readings, and the curve carries 21
points across the full range.
- **Long beer names were clipped** at the left edge of the attenuation
chart ("...x crying out loud"). Names are trimmed to 18 characters with
an ellipsis; the tooltip and the table carry the full name.
Tests
-----
127 (up from 105). The two chart regressions are covered by reading
`charts.js` and asserting the gravity axis is not reversed and that long
names are trimmed - these are exactly the kind of change that silently
reverts, and they are invisible to a screenshot test.
Consolidated Review Report: feat(analysis): attenuation, fermentation curves and brew timings
1. Executive Summary
This Pull Request introduces a comprehensive Analysis module designed to provide longitudinal tracking of brewing fermentation and attenuation. The implementation is technically sophisticated, utilizing linear interpolation and median-and-envelope visualization to normalize inconsistent sampling data.
From a Code Quality perspective, the PR is high-caliber, demonstrating strong domain knowledge and statistical rigor. The Infrastructure impact is negligible, as no IaC changes were made. However, the Security review has flagged several "blind spots" typical of new route additions, specifically regarding XSS, authorization, and NoSQL injection, which require verification since the source code was not explicitly provided for the security audit.
2. Prioritized Issue List
Priority
Domain
Issue
Description
🔴Critical
Security
Authorization Gap
The new /analysis route may be public. Verification is required to ensure @login_required or similar middleware is implemented.
🟠High
Security
Potential XSS
User-defined brew names are rendered in tooltips and tables. Verification of Jinja2 escaping and charts.js DOM manipulation is needed.
🟠High
Security
NoSQL Injection
New MongoDB queries in analysis.py must be verified to ensure they use parameterized filters rather than user-supplied strings.
🟡Medium
Code
Performance Scaling
Interpolation logic is O(n). While fine for 16 brews, it may lag with hundreds of brews.
🟡Medium
Code
Edge Case Stability
Potential for JS errors or empty states when users have insufficient data (0-1 brews) to calculate medians.
🔵Low
Infra
Cache Busting
Significant changes to charts.js may lead to UI bugs for returning users due to browser caching.
🔵Low
Security
Validation Placement
Sanity checks for Gravity (FG) are currently at the presentation layer rather than the ingestion layer.
3. Domain-Specific Recommendations
💻 Code Quality & Logic
Optimization: Implement caching for the interpolation and aggregation results. If the dataset grows, calculate these as a background task or a materialized view rather than on every page load.
Frontend Payload: Ensure the backend sends only the aggregated arrays (median, min, max) to the frontend, rather than the full raw data for every interpolated brew.
UI Resilience: Implement a "Not enough data to plot" empty state in analysis.html to prevent the application from attempting to calculate medians of empty sets.
Testing: Move beyond testing the implementation (e.g., checking a boolean flag for axis reversal) to testing the behavior via snapshot tests or integration tests for the chart config.
🛡️ Security
Input Sanitization: Immediately verify that all brew names are escaped. Avoid .innerHTML in charts.js; use .textContent or library-native tooltip handlers.
Access Control: Confirm that the analysis.py view is wrapped in the application's standard authorization decorators.
Upstream Validation: Move the "physically possible" gravity check (e.g., FG < 900) to the API endpoint where readings are first saved. This prevents "dirty data" from affecting other potential modules.
CSP: Implement or update the Content Security Policy (CSP) to ensure only trusted scripts (Chart.js) are executed.
⚙️ Infrastructure & Platform
Observability: Add specific monitoring/alerts for the /analysis endpoint to catch 5xx errors related to interpolation logic in production.
Database Indexing: Verify that MongoDB indexes exist for the fields used in the aggregation (e.g., run_id, timestamp, or brew_date).
Asset Management: Implement versioning or cache-busting (e.g., charts.js?v=[hash]) in the deployment pipeline to ensure clients receive the updated visualization logic.
4. Positive Aspects of the PR
Domain Expertise: The correction of the Y-axis reversal and the use of median-and-envelope curves shows a deep understanding of both brewing science and data visualization.
Statistical Integrity: The decision to exclude corrupt data from averages rather than clamping values ensures the resulting analysis remains scientifically valid.
User Experience: Label truncation (18 chars) and the inclusion of a "Readings that need a second look" section demonstrate a high level of empathy for the end-user.
Test Coverage: A significant and commendable increase in test coverage (from 105 to 127 tests).
5. Overall Assessment & Recommendation
Recommendation: APPROVED WITH MINOR CHANGES
The functional and architectural quality of this PR is excellent. The logic is sound, the UX is thoughtful, and the tests are comprehensive. However, because this introduces a new entry point (/analysis) and renders user-supplied data, it cannot be fully approved until the Security Verification items (Authorization and XSS) are confirmed.
Final Action Items for Developer:
Verify Security: Confirm route protection and output encoding.
Graceful Failures: Add a "No Data" state for the charts.
Payload Optimization: Ensure only aggregated data is sent to charts.js.
## PR Review Results
# Consolidated Review Report: `feat(analysis): attenuation, fermentation curves and brew timings`
## 1. Executive Summary
This Pull Request introduces a comprehensive Analysis module designed to provide longitudinal tracking of brewing fermentation and attenuation. The implementation is technically sophisticated, utilizing linear interpolation and median-and-envelope visualization to normalize inconsistent sampling data.
From a **Code Quality** perspective, the PR is high-caliber, demonstrating strong domain knowledge and statistical rigor. The **Infrastructure** impact is negligible, as no IaC changes were made. However, the **Security** review has flagged several "blind spots" typical of new route additions, specifically regarding XSS, authorization, and NoSQL injection, which require verification since the source code was not explicitly provided for the security audit.
---
## 2. Prioritized Issue List
| Priority | Domain | Issue | Description |
| :--- | :--- | :--- | :--- |
| 🔴 **Critical** | Security | **Authorization Gap** | The new `/analysis` route may be public. Verification is required to ensure `@login_required` or similar middleware is implemented. |
| 🟠 **High** | Security | **Potential XSS** | User-defined brew names are rendered in tooltips and tables. Verification of Jinja2 escaping and `charts.js` DOM manipulation is needed. |
| 🟠 **High** | Security | **NoSQL Injection** | New MongoDB queries in `analysis.py` must be verified to ensure they use parameterized filters rather than user-supplied strings. |
| 🟡 **Medium** | Code | **Performance Scaling** | Interpolation logic is $O(n)$. While fine for 16 brews, it may lag with hundreds of brews. |
| 🟡 **Medium** | Code | **Edge Case Stability** | Potential for JS errors or empty states when users have insufficient data (0-1 brews) to calculate medians. |
| 🔵 **Low** | Infra | **Cache Busting** | Significant changes to `charts.js` may lead to UI bugs for returning users due to browser caching. |
| 🔵 **Low** | Security | **Validation Placement** | Sanity checks for Gravity (FG) are currently at the presentation layer rather than the ingestion layer. |
---
## 3. Domain-Specific Recommendations
### 💻 Code Quality & Logic
* **Optimization:** Implement caching for the interpolation and aggregation results. If the dataset grows, calculate these as a background task or a materialized view rather than on every page load.
* **Frontend Payload:** Ensure the backend sends only the aggregated arrays (median, min, max) to the frontend, rather than the full raw data for every interpolated brew.
* **UI Resilience:** Implement a "Not enough data to plot" empty state in `analysis.html` to prevent the application from attempting to calculate medians of empty sets.
* **Testing:** Move beyond testing the *implementation* (e.g., checking a boolean flag for axis reversal) to testing the *behavior* via snapshot tests or integration tests for the chart config.
### 🛡️ Security
* **Input Sanitization:** Immediately verify that all brew names are escaped. Avoid `.innerHTML` in `charts.js`; use `.textContent` or library-native tooltip handlers.
* **Access Control:** Confirm that the `analysis.py` view is wrapped in the application's standard authorization decorators.
* **Upstream Validation:** Move the "physically possible" gravity check (e.g., FG < 900) to the API endpoint where readings are first saved. This prevents "dirty data" from affecting other potential modules.
* **CSP:** Implement or update the Content Security Policy (CSP) to ensure only trusted scripts (Chart.js) are executed.
### ⚙️ Infrastructure & Platform
* **Observability:** Add specific monitoring/alerts for the `/analysis` endpoint to catch `5xx` errors related to interpolation logic in production.
* **Database Indexing:** Verify that MongoDB indexes exist for the fields used in the aggregation (e.g., `run_id`, `timestamp`, or `brew_date`).
* **Asset Management:** Implement versioning or cache-busting (e.g., `charts.js?v=[hash]`) in the deployment pipeline to ensure clients receive the updated visualization logic.
---
## 4. Positive Aspects of the PR
* **Domain Expertise:** The correction of the Y-axis reversal and the use of median-and-envelope curves shows a deep understanding of both brewing science and data visualization.
* **Statistical Integrity:** The decision to exclude corrupt data from averages rather than clamping values ensures the resulting analysis remains scientifically valid.
* **User Experience:** Label truncation (18 chars) and the inclusion of a "Readings that need a second look" section demonstrate a high level of empathy for the end-user.
* **Test Coverage:** A significant and commendable increase in test coverage (from 105 to 127 tests).
---
## 5. Overall Assessment & Recommendation
**Recommendation: APPROVED WITH MINOR CHANGES**
The functional and architectural quality of this PR is excellent. The logic is sound, the UX is thoughtful, and the tests are comprehensive. However, because this introduces a new entry point (`/analysis`) and renders user-supplied data, it cannot be fully approved until the **Security Verification** items (Authorization and XSS) are confirmed.
**Final Action Items for Developer:**
1. **Verify Security:** Confirm route protection and output encoding.
2. **Graceful Failures:** Add a "No Data" state for the charts.
3. **Payload Optimization:** Ensure only aggregated data is sent to `charts.js`.
The RUNBOOK still said the workflow did not deploy to the cluster. It does
now - it builds, pushes, applies the manifests, rolls the pods and verifies
the app from inside the pod. The manual deploy path is documented alongside
it, along with the dry-run step that catches the schema errors kubectl apply
silently swallows.
Review finding, and it was right. The PR claimed that runs with impossible
readings were "excluded from the averages". That was only true of the
attenuation average. Run 12's bad final gravity was still poisoning three
other statistics:
- **Brew time** - its 56 days were folded into the average, making a
typical brew look like 13.7 days when it is 10.9.
- **Time to 1.010** - its 56 days did the same to that figure (8.4d).
- **The history page's ABV** - an 18.64% ABV, which made run 12 the
"strongest" beer in the history and pulled the average up.
- **The fermentation curve** - its curve plunges from 1.042 to 0.900, which
dragged the median line down and stretched the min-max envelope to cover
a gravity below water.
One bad reading was reaching four different statistics.
What changed
------------
- The suspect runs are computed once and excluded from every aggregate:
the attenuation, brew-time and time-to-1.010 averages, the fastest and
slowest brews, the history page's ABV average, and its strongest and
lightest.
- `/api/runs` now carries a `suspect` flag, and the charts skip the flagged
runs - the attenuation chart and the fermentation curve.
- The suspect run stays in the table, with a "not counted" chip and a tinted
row, so it can be corrected. It is excluded from the figures, not
hidden. The card header says "16 brews (15 counted)" and the prose says
how many brews the figures are based on.
Tests
-----
133 (up from 127). New coverage: the suspect run is excluded from the brew
time and time-to-1.010 averages, it is still listed for review, it is
flagged in the API, it is not the strongest brew in the history, and the
charts filter it out.
The `logged_in` fixture moved to conftest.py, where both test modules can
use it.
The claim "excluded from the averages" was only true of the attenuation average. Run 12's bad final gravity was still reaching three other statistics:
Brew time — its 56 days were folded in, making a typical brew look like 13.7 days when it is 10.9.
Time to 1.010 — its 56 days did the same to that figure (8.4d).
The history page's ABV — an 18.64% ABV made run 12 the "strongest" beer in the history and pulled the average up.
The fermentation curve — its curve plunges from 1.042 to 0.900, dragging the median line down and stretching the min–max envelope to cover a gravity below water.
One bad reading, four statistics. Fixed in bbd9fe1.
What changed
The suspect runs are computed once and excluded from every aggregate: the attenuation, brew-time and time-to-1.010 averages, the fastest and slowest brews, the history page's ABV average, and its strongest and lightest.
/api/runs now carries a suspect flag, and the charts skip the flagged runs.
The suspect run stays in the table with a "not counted" chip and a tinted row, so it can be corrected — excluded from the figures, not hidden. The card header reads "16 brews (15 counted)" and the prose says how many brews the figures are based on.
Tests
133 (up from 127). New coverage: the suspect run is excluded from the brew-time and time-to-1.010 averages, it is still listed for review, it is flagged in the API, it is not the strongest brew in the history, and the charts filter it out. The logged_in fixture moved to conftest.py so both test modules can use it.
Verified against the live data
Average attenuation 87.5%, spread ±7.4pp
Average brew time 10.9d (was 13.7d with the bad run folded in)
Time to 1.010 8.4d (was 11.5d)
Table header: 16 brews (15 counted)
Run 12's row renders with the amber "not counted" chip and a tinted background
Thanks — good catch, and it was right.
The claim "excluded from the averages" was only true of the attenuation average. Run 12's bad final gravity was still reaching three other statistics:
- **Brew time** — its 56 days were folded in, making a typical brew look like 13.7 days when it is **10.9**.
- **Time to 1.010** — its 56 days did the same to that figure (**8.4d**).
- **The history page's ABV** — an 18.64% ABV made run 12 the "strongest" beer in the history and pulled the average up.
- **The fermentation curve** — its curve plunges from 1.042 to 0.900, dragging the median line down and stretching the min–max envelope to cover a gravity below water.
One bad reading, four statistics. Fixed in `bbd9fe1`.
### What changed
- The suspect runs are computed once and excluded from **every** aggregate: the attenuation, brew-time and time-to-1.010 averages, the fastest and slowest brews, the history page's ABV average, and its strongest and lightest.
- `/api/runs` now carries a `suspect` flag, and the charts skip the flagged runs.
- The suspect run **stays in the table** with a "not counted" chip and a tinted row, so it can be corrected — excluded from the figures, not hidden. The card header reads "16 brews (15 counted)" and the prose says how many brews the figures are based on.
### Tests
133 (up from 127). New coverage: the suspect run is excluded from the brew-time and time-to-1.010 averages, it is still listed for review, it is flagged in the API, it is not the strongest brew in the history, and the charts filter it out. The `logged_in` fixture moved to `conftest.py` so both test modules can use it.
### Verified against the live data
- Average attenuation **87.5%**, spread **±7.4pp**
- Average brew time **10.9d** (was 13.7d with the bad run folded in)
- Time to 1.010 **8.4d** (was 11.5d)
- Table header: **16 brews (15 counted)**
- Run 12's row renders with the amber "not counted" chip and a tinted background
The re-review returned APPROVED with two low-severity notes.
- The prose said "Across 15 finished brews", which reads as though there
are only 15 brews. It now says "Across 15 of 16 finished brews", so the
exclusion is visible in the sentence itself and not only in the card
header.
- `/api/runs/<run_id>` did not carry the `suspect` flag that
`/api/runs` does, so a consumer of the detail endpoint could not tell an
impossible run from a good one. It does now, with a test.
134 tests.
hermes
merged commit 21e7553ab4 into master2026-11-09 14:18:45 +10:00
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.
Summary
Adds the analysis page the old app never had — how consistently the brews ferment, and how long each one takes.
This is the third and last of the split PRs. PR1 rebuilt the UI and app foundation (#4), PR2 added the active beers page and run metadata (#5).
What it shows
Data quality — one run was corrupting the averages
Live run 12 has a final gravity of
900. That works out as 338% attenuation and dragged the average to 103.2%. That is not a real result, so:Chart bugs found while building this
These are the kind of thing that looks fine in a screenshot and is wrong:
reverse: truethe high reading sits at the bottom, so the fermentation curve climbed left to right — it read as gravity rising as the beer fermented. A brewer reads a fermentation as a descending line. The point-scale run chart on the run detail page had the same bug. Both fixed."...x crying out loud"). Names are trimmed to 18 characters with an ellipsis; the tooltip and the table carry the full name.Verification
charts.jsand asserting the gravity axis is not reversed and that long names are trimmed. A screenshot test would not have caught either — these changes silently revert.Test plan
/analysisrenders, both charts draw datamasteron merge — I will confirm the rollout and the live page afterwards.Adds the analysis page the old app never had: how consistently the brews ferment, and how long each one takes. What it shows ------------- - **Attenuation by brew** - what fraction of the original gravity was fermented away, for every finished brew. - **Fermentation curves** - the typical shape of a fermentation, drawn as a median curve with the min-max envelope behind it. Plotting every brew individually gave 16 near-identical lines and an unreadable legend. - **Every finished brew** - OG, FG, ABV, attenuation, days in the fermenter, days to reach 1.010, and the finish date. - **Readings that need a second look** - readings outside the plausible hydrometer range. - Headline figures: average attenuation and its spread, average brew time, average time to 1.010. Data quality ------------ One live run (run 12) has a final gravity of 900, which worked out as 338% attenuation and dragged the average to 103.2%. That is not a real result, so: - Attenuation is rejected when the final gravity is not physically possible for the original gravity. - Runs that fail that check are EXCLUDED from the averages rather than silently distorting them, and are listed on the page as needing review. - The average now reads 87.5% +/- 7.4 percentage points across 16 brews. Chart bugs found while building this ------------------------------------ - **The gravity charts had their y axis reversed.** With `reverse: true` the high reading sits at the BOTTOM, so the fermentation curve climbed left to right - it read as gravity *rising* as the beer fermented. A brewer reads a fermentation as a descending line. The point-scale run chart on the run detail page had the same bug. Both are fixed. - **The curve chart's x axis was treated as categorical.** The x values are numeric (0-100), and Chart.js defaults a line chart's x axis to category, which collapsed every point onto index 0 and rendered the curve as a sliver at the origin. The axis is now explicitly linear. - **The curve was interpolated by exact key lookup**, so it only worked when every brew happened to be read at the same progress points. It now interpolates between a brew's actual readings, and the curve carries 21 points across the full range. - **Long beer names were clipped** at the left edge of the attenuation chart ("...x crying out loud"). Names are trimmed to 18 characters with an ellipsis; the tooltip and the table carry the full name. Tests ----- 127 (up from 105). The two chart regressions are covered by reading `charts.js` and asserting the gravity axis is not reversed and that long names are trimmed - these are exactly the kind of change that silently reverts, and they are invisible to a screenshot test.PR received — starting review, sit tight 🫡
PR Review Results
Consolidated Review Report:
feat(analysis): attenuation, fermentation curves and brew timings1. Executive Summary
This Pull Request introduces a comprehensive Analysis module designed to provide longitudinal tracking of brewing fermentation and attenuation. The implementation is technically sophisticated, utilizing linear interpolation and median-and-envelope visualization to normalize inconsistent sampling data.
From a Code Quality perspective, the PR is high-caliber, demonstrating strong domain knowledge and statistical rigor. The Infrastructure impact is negligible, as no IaC changes were made. However, the Security review has flagged several "blind spots" typical of new route additions, specifically regarding XSS, authorization, and NoSQL injection, which require verification since the source code was not explicitly provided for the security audit.
2. Prioritized Issue List
/analysisroute may be public. Verification is required to ensure@login_requiredor similar middleware is implemented.charts.jsDOM manipulation is needed.analysis.pymust be verified to ensure they use parameterized filters rather than user-supplied strings.O(n). While fine for 16 brews, it may lag with hundreds of brews.charts.jsmay lead to UI bugs for returning users due to browser caching.3. Domain-Specific Recommendations
💻 Code Quality & Logic
analysis.htmlto prevent the application from attempting to calculate medians of empty sets.🛡️ Security
.innerHTMLincharts.js; use.textContentor library-native tooltip handlers.analysis.pyview is wrapped in the application's standard authorization decorators.⚙️ Infrastructure & Platform
/analysisendpoint to catch5xxerrors related to interpolation logic in production.run_id,timestamp, orbrew_date).charts.js?v=[hash]) in the deployment pipeline to ensure clients receive the updated visualization logic.4. Positive Aspects of the PR
5. Overall Assessment & Recommendation
Recommendation: APPROVED WITH MINOR CHANGES
The functional and architectural quality of this PR is excellent. The logic is sound, the UX is thoughtful, and the tests are comprehensive. However, because this introduces a new entry point (
/analysis) and renders user-supplied data, it cannot be fully approved until the Security Verification items (Authorization and XSS) are confirmed.Final Action Items for Developer:
charts.js.Thanks — good catch, and it was right.
The claim "excluded from the averages" was only true of the attenuation average. Run 12's bad final gravity was still reaching three other statistics:
One bad reading, four statistics. Fixed in
bbd9fe1.What changed
/api/runsnow carries asuspectflag, and the charts skip the flagged runs.Tests
133 (up from 127). New coverage: the suspect run is excluded from the brew-time and time-to-1.010 averages, it is still listed for review, it is flagged in the API, it is not the strongest brew in the history, and the charts filter it out. The
logged_infixture moved toconftest.pyso both test modules can use it.Verified against the live data
hermes referenced this pull request2026-11-09 16:04:37 +10:00