Fixes the findings from the deeper review of PR #12 that the first review did not catch. All five are real bugs in the deployed app.
1. The band was not the band (MEDIUM)
The shaded range was drawn as one dataset with fill: "+1", which fills to the next dataset — and the next dataset was the median line, not the maximum. So the band shaded min-to-median, the upper half of the range was a hairline that was never shaded, and the tooltip said "range min to max" while the picture showed something else.
Now two datasets: the min curve filled down to a max curve, with the max hidden from the legend. Verified by pixel scan at 60% progress — shaded pixels at both 64 and 73 with the median line between them at 67–70.
2. Rising gravity (MEDIUM)
attenuation_percent rejected a final gravity below 1.000 and one implying >100% attenuation, but a run whose gravity went up produced a negative attenuation, which was averaged into the headline figure and plotted as a negative bar.
Verified against the live code: OG 1010 → FG 1020 gave -100.0%. Now rejected, and reported by implausible_runs as "the gravity went up, which is usually a mis-ordered pair of readings".
An undated reading sorted to the front of the run, so it became the OG — putting the OG after the FG and producing a negative attenuation.
The OG now comes from the earliest dated reading.
days_to_target skips readings it cannot place on the timeline instead of giving up on the whole run.
Verified: readings 1050 (dated), 1005 (undated), 1010 (dated) gave None instead of 24 days, and an OG of 1005 instead of 1050.
4. Active runs on the curve (HIGH)
The curve chart filtered on curve.length > 1 only, so it plotted active runs too — a partial fermentation on a chart meant to describe how a completed one behaves. It now excludes them, along with the suspect runs.
5. Four fetches, one payload (LOW)
/api/runs carries every reading of every run, and four charts each fetched it independently — so the analysis page made two identical round-trips of the whole collection. There is now one shared fetch.
Also
The "Readings that need a second look" table now links to the run, so a suspect reading can be corrected in one click.
days_to_1015 and the curves variable were computed and never used. Removed.
A reading below 1.000 now says what it is: "beer cannot be less dense than water, so this is almost certainly a typo (a 1.012 reading entered as 012?)".
I did not tighten the form's NumberRange to 990 as suggested. The existing behaviour is deliberate — an out-of-range reading is saved with a warning, because rejecting it would lose the brewer's actual observation, and seeing it recorded is how they spot the typo. I made the warning name the likely cause instead.
Test plan
142 tests pass (up from 134)
New coverage: rising-gravity attenuation, rising-gravity reporting, OG from the first dated reading, days_to_target skipping an undated reading, the curve excluding active runs, the band being filled to the max, and the shared fetch
Live data re-verified: 87.5% ±7.4pp, 10.9d, 8.4d, 16 finished / 15 counted, no negative attenuations
Band verified by canvas pixel scan, not by eye
## Summary
Fixes the findings from the deeper review of PR #12 that the first review did not catch. All five are real bugs in the **deployed** app.
### 1. The band was not the band (MEDIUM)
The shaded range was drawn as one dataset with `fill: "+1"`, which fills to the **next** dataset — and the next dataset was the *median line*, not the maximum. So the band shaded min-to-median, the upper half of the range was a hairline that was never shaded, and the tooltip said "range min to max" while the picture showed something else.
Now two datasets: the min curve filled down to a max curve, with the max hidden from the legend. Verified by pixel scan at 60% progress — shaded pixels at both 64 and 73 with the median line between them at 67–70.
### 2. Rising gravity (MEDIUM)
`attenuation_percent` rejected a final gravity below 1.000 and one implying >100% attenuation, but a run whose gravity **went up** produced a *negative* attenuation, which was averaged into the headline figure and plotted as a negative bar.
Verified against the live code: OG 1010 → FG 1020 gave **-100.0%**. Now rejected, and reported by `implausible_runs` as "the gravity went up, which is usually a mis-ordered pair of readings".
### 3. Undated readings (LOW, but it caused #2)
An undated reading sorted to the front of the run, so it became the OG — putting the OG *after* the FG and producing a negative attenuation.
- The OG now comes from the earliest **dated** reading.
- `days_to_target` skips readings it cannot place on the timeline instead of giving up on the whole run.
Verified: readings 1050 (dated), 1005 (undated), 1010 (dated) gave `None` instead of 24 days, and an OG of 1005 instead of 1050.
### 4. Active runs on the curve (HIGH)
The curve chart filtered on `curve.length > 1` only, so it plotted **active** runs too — a partial fermentation on a chart meant to describe how a completed one behaves. It now excludes them, along with the suspect runs.
### 5. Four fetches, one payload (LOW)
`/api/runs` carries every reading of every run, and four charts each fetched it independently — so the analysis page made two identical round-trips of the whole collection. There is now one shared fetch.
### Also
- The "Readings that need a second look" table now **links to the run**, so a suspect reading can be corrected in one click.
- `days_to_1015` and the `curves` variable were computed and never used. Removed.
- A reading below 1.000 now says what it is: *"beer cannot be less dense than water, so this is almost certainly a typo (a 1.012 reading entered as 012?)"*.
> I did **not** tighten the form's `NumberRange` to 990 as suggested. The existing behaviour is deliberate — an out-of-range reading is *saved* with a warning, because rejecting it would lose the brewer's actual observation, and seeing it recorded is how they spot the typo. I made the warning name the likely cause instead.
## Test plan
- [x] 142 tests pass (up from 134)
- [x] New coverage: rising-gravity attenuation, rising-gravity reporting, OG from the first dated reading, `days_to_target` skipping an undated reading, the curve excluding active runs, the band being filled to the max, and the shared fetch
- [x] Live data re-verified: 87.5% ±7.4pp, 10.9d, 8.4d, 16 finished / 15 counted, no negative attenuations
- [x] Band verified by canvas pixel scan, not by eye
A second, deeper review of PR #12 found five things the first review did
not. All are fixed here.
The band was not the band
-------------------------
The fermentation curve's shaded range was drawn as ONE dataset with
`fill: "+1"`, which fills to the NEXT dataset - and the next dataset was
the median line, not the maximum. So the band shaded min-to-median, the
upper half of the range was drawn as a hairline but never shaded, and the
tooltip said "range min to max" while the picture showed something else.
The band is now two datasets - the min curve filled down to a max curve -
with the max hidden from the legend.
Rising gravity
--------------
`attenuation_percent` rejected a final gravity below 1.000 and one
implying more than 100% attenuation, but a run whose gravity went UP
produced a NEGATIVE attenuation - which was then averaged into the
headline figure and plotted as a negative bar. Verified against the live
code: OG 1010 -> FG 1020 gave -100.0%. Now rejected, and reported by
`implausible_runs` with the reason "the gravity went up, which is usually
a mis-ordered pair of readings".
Undated readings
----------------
An undated reading sorted to the front of the run, so it became the
original gravity - which put the OG after the FG and produced a negative
attenuation. The OG now comes from the earliest *dated* reading, and
`days_to_target` skips the readings it cannot place on the timeline rather
than giving up on the whole run. Verified: readings 1050 (dated), 1005
(undated), 1010 (dated) gave None instead of 24 days, and an OG of 1005
instead of 1050.
Active runs on the curve
------------------------
The curve chart filtered on `curve.length > 1` only, so it plotted the
active runs as well - a partial fermentation on a chart meant to describe
how a completed one behaves. It now excludes them, along with the
suspect runs.
Four fetches, one payload
-------------------------
`/api/runs` carries every reading of every run, and four charts each
fetched it independently - so the analysis page made two identical
round-trips of the whole collection. There is now one shared fetch.
Also
----
- The "Readings that need a second look" table links to the run, so a
suspect reading can be corrected in one click.
- `days_to_1015` and the `curves` variable were computed and never used.
Removed.
- A reading below 1.000 now says what it is - "beer cannot be less dense
than water, so this is almost certainly a typo (a 1.012 reading entered
as 012?)". The reading is still SAVED: rejecting it would lose the
brewer's actual observation, and seeing it recorded is how they spot the
mistake.
142 tests (up from 134).
This Pull Request (PR) implements critical bug fixes for the brewing application's analysis module, focusing on data visualization accuracy, brewing domain logic (attenuation and gravity readings), and frontend performance.
The Code Review is highly positive, praising the architectural decisions and the rigor of the testing. The Infrastructure Review is approved, noting that while no IaC changes were made, the API consolidation improves platform efficiency. However, the Security Review has raised a "Needs Improvement" flag, primarily due to the introduction of direct resource links which may be vulnerable to Insecure Direct Object References (IDOR) and concerns regarding the persistence of out-of-range data.
2. Prioritized List of Issues
Priority
Domain
Issue
Description
Medium
Security
Potential IDOR Vulnerability
New links to specific "runs" for correction may allow users to access/edit runs belonging to other users if ownership checks are missing in brews.py.
Low
Security
Input Validation Strategy
Allowing "out-of-range" readings to be saved to the database deviates from strict validation principles and could cause downstream calculation errors.
Low
Security/Infra
API Payload Scaling (DoS)
Consolidating four fetches into one /api/runs call increases the response size. Large datasets could lead to memory exhaustion or browser hangs.
Low
Code
API Caching/Summarization
The current "fetch all" approach is efficient now but may require a summarized analysis endpoint or caching as the dataset grows.
Low
Code
Type Safety in Charts
Potential for future developers to accidentally re-enable "hidden" datasets in charts.js.
3. Domain-Specific Recommendations
💻 Code Quality & Logic
Maintainability: The transition to explicit Min/Max datasets in Chart.js is a strong move; ensure that the "hidden" datasets are clearly commented to prevent accidental reactivation.
Data Integrity: The logic to filter for the earliest dated reading for Original Gravity (OG) is a high-impact win.
Performance: While the consolidated fetch is a great immediate fix, consider moving toward a backend-driven "analysis summary" endpoint to avoid sending raw reading data for every single run to the client.
🛡️ Security
Authorization:Immediate Action Required: Verify that src/flask/views/brews.py implements a strict ownership check (e.g., if run.user_id != current_user.id: abort(403)) for the new "click to correct" functionality.
Hard Limits: Implement a "sanity check" on the server side. While the UI should allow users to save "implausible" data for their own records, the server should reject extreme values (e.g., 10^{10}) to prevent database corruption or calculation crashes.
Pagination: Consider adding pagination or date-filtering to the /api/runs endpoint to mitigate potential Denial of Service (DoS) risks associated with large payloads.
⚙️ Infrastructure & Platform
Observability: Implement custom logging or Prometheus metrics for "implausible runs." Monitoring the frequency of these events can help the team identify systemic user input issues or hardware calibration problems.
Caching: Implement Cache-Control headers for the consolidated /api/runs endpoint, as historical brewing data is largely static.
CI Integration: Ensure the 8 new test cases are fully integrated into the CI pipeline to prevent regressions in gravity calculation logic.
4. Positive Aspects of the PR
Exceptional Testing Rigor: The increase in test coverage and the use of "pixel scans" for visual verification demonstrate a high level of professional diligence.
User-Centric Design: The decision to allow out-of-range data to be saved (with a warning) rather than blocking the user shows a mature understanding of the user's workflow.
Performance Gains: Successfully addressed a frontend "N+1" problem, significantly reducing network overhead and Time to Interactive (TTI).
Clean-up: Proactive removal of dead code (days_to_1015, curves) improves overall codebase maintainability.
5. Overall Assessment & Recommendation
Overall Rating: APPROVED WITH MINOR CHANGES
The PR is functionally excellent and provides significant value to the end-user and the system's performance. However, it cannot be fully merged until the security concerns regarding Resource Ownership (IDOR) are addressed.
Required Actions before Merge:
Provide evidence or implement code that ensures users can only edit their own "runs" via the new links.
Confirm that the server-side logic handles the "out-of-range" data safely to prevent crashes.
Once the authorization checks are verified, this PR is highly recommended for deployment.
## PR Review Results
# Consolidated Review Report: Analysis Module Updates
## 1. Executive Summary
This Pull Request (PR) implements critical bug fixes for the brewing application's analysis module, focusing on data visualization accuracy, brewing domain logic (attenuation and gravity readings), and frontend performance.
The **Code Review** is highly positive, praising the architectural decisions and the rigor of the testing. The **Infrastructure Review** is approved, noting that while no IaC changes were made, the API consolidation improves platform efficiency. However, the **Security Review** has raised a "Needs Improvement" flag, primarily due to the introduction of direct resource links which may be vulnerable to Insecure Direct Object References (IDOR) and concerns regarding the persistence of out-of-range data.
---
## 2. Prioritized List of Issues
| Priority | Domain | Issue | Description |
| :--- | :--- | :--- | :--- |
| **Medium** | Security | **Potential IDOR Vulnerability** | New links to specific "runs" for correction may allow users to access/edit runs belonging to other users if ownership checks are missing in `brews.py`. |
| **Low** | Security | **Input Validation Strategy** | Allowing "out-of-range" readings to be saved to the database deviates from strict validation principles and could cause downstream calculation errors. |
| **Low** | Security/Infra | **API Payload Scaling (DoS)** | Consolidating four fetches into one `/api/runs` call increases the response size. Large datasets could lead to memory exhaustion or browser hangs. |
| **Low** | Code | **API Caching/Summarization** | The current "fetch all" approach is efficient now but may require a summarized analysis endpoint or caching as the dataset grows. |
| **Low** | Code | **Type Safety in Charts** | Potential for future developers to accidentally re-enable "hidden" datasets in `charts.js`. |
---
## 3. Domain-Specific Recommendations
### 💻 Code Quality & Logic
* **Maintainability:** The transition to explicit Min/Max datasets in Chart.js is a strong move; ensure that the "hidden" datasets are clearly commented to prevent accidental reactivation.
* **Data Integrity:** The logic to filter for the earliest *dated* reading for Original Gravity (OG) is a high-impact win.
* **Performance:** While the consolidated fetch is a great immediate fix, consider moving toward a backend-driven "analysis summary" endpoint to avoid sending raw reading data for every single run to the client.
### 🛡️ Security
* **Authorization:** **Immediate Action Required:** Verify that `src/flask/views/brews.py` implements a strict ownership check (e.g., `if run.user_id != current_user.id: abort(403)`) for the new "click to correct" functionality.
* **Hard Limits:** Implement a "sanity check" on the server side. While the UI should allow users to save "implausible" data for their own records, the server should reject extreme values (e.g., $10^{10}$) to prevent database corruption or calculation crashes.
* **Pagination:** Consider adding pagination or date-filtering to the `/api/runs` endpoint to mitigate potential Denial of Service (DoS) risks associated with large payloads.
### ⚙️ Infrastructure & Platform
* **Observability:** Implement custom logging or Prometheus metrics for "implausible runs." Monitoring the frequency of these events can help the team identify systemic user input issues or hardware calibration problems.
* **Caching:** Implement `Cache-Control` headers for the consolidated `/api/runs` endpoint, as historical brewing data is largely static.
* **CI Integration:** Ensure the 8 new test cases are fully integrated into the CI pipeline to prevent regressions in gravity calculation logic.
---
## 4. Positive Aspects of the PR
* **Exceptional Testing Rigor:** The increase in test coverage and the use of "pixel scans" for visual verification demonstrate a high level of professional diligence.
* **User-Centric Design:** The decision to allow out-of-range data to be saved (with a warning) rather than blocking the user shows a mature understanding of the user's workflow.
* **Performance Gains:** Successfully addressed a frontend "N+1" problem, significantly reducing network overhead and Time to Interactive (TTI).
* **Clean-up:** Proactive removal of dead code (`days_to_1015`, `curves`) improves overall codebase maintainability.
---
## 5. Overall Assessment & Recommendation
**Overall Rating: APPROVED WITH MINOR CHANGES**
The PR is functionally excellent and provides significant value to the end-user and the system's performance. However, it cannot be fully merged until the security concerns regarding **Resource Ownership (IDOR)** are addressed.
**Required Actions before Merge:**
1. Provide evidence or implement code that ensures users can only edit their own "runs" via the new links.
2. Confirm that the server-side logic handles the "out-of-range" data safely to prevent crashes.
Once the authorization checks are verified, this PR is highly recommended for deployment.
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
Fixes the findings from the deeper review of PR #12 that the first review did not catch. All five are real bugs in the deployed app.
1. The band was not the band (MEDIUM)
The shaded range was drawn as one dataset with
fill: "+1", which fills to the next dataset — and the next dataset was the median line, not the maximum. So the band shaded min-to-median, the upper half of the range was a hairline that was never shaded, and the tooltip said "range min to max" while the picture showed something else.Now two datasets: the min curve filled down to a max curve, with the max hidden from the legend. Verified by pixel scan at 60% progress — shaded pixels at both 64 and 73 with the median line between them at 67–70.
2. Rising gravity (MEDIUM)
attenuation_percentrejected a final gravity below 1.000 and one implying >100% attenuation, but a run whose gravity went up produced a negative attenuation, which was averaged into the headline figure and plotted as a negative bar.Verified against the live code: OG 1010 → FG 1020 gave -100.0%. Now rejected, and reported by
implausible_runsas "the gravity went up, which is usually a mis-ordered pair of readings".3. Undated readings (LOW, but it caused #2)
An undated reading sorted to the front of the run, so it became the OG — putting the OG after the FG and producing a negative attenuation.
days_to_targetskips readings it cannot place on the timeline instead of giving up on the whole run.Verified: readings 1050 (dated), 1005 (undated), 1010 (dated) gave
Noneinstead of 24 days, and an OG of 1005 instead of 1050.4. Active runs on the curve (HIGH)
The curve chart filtered on
curve.length > 1only, so it plotted active runs too — a partial fermentation on a chart meant to describe how a completed one behaves. It now excludes them, along with the suspect runs.5. Four fetches, one payload (LOW)
/api/runscarries every reading of every run, and four charts each fetched it independently — so the analysis page made two identical round-trips of the whole collection. There is now one shared fetch.Also
days_to_1015and thecurvesvariable were computed and never used. Removed.Test plan
days_to_targetskipping an undated reading, the curve excluding active runs, the band being filled to the max, and the shared fetchPR received — starting review, sit tight 🫡
PR Review Results
Consolidated Review Report: Analysis Module Updates
1. Executive Summary
This Pull Request (PR) implements critical bug fixes for the brewing application's analysis module, focusing on data visualization accuracy, brewing domain logic (attenuation and gravity readings), and frontend performance.
The Code Review is highly positive, praising the architectural decisions and the rigor of the testing. The Infrastructure Review is approved, noting that while no IaC changes were made, the API consolidation improves platform efficiency. However, the Security Review has raised a "Needs Improvement" flag, primarily due to the introduction of direct resource links which may be vulnerable to Insecure Direct Object References (IDOR) and concerns regarding the persistence of out-of-range data.
2. Prioritized List of Issues
brews.py./api/runscall increases the response size. Large datasets could lead to memory exhaustion or browser hangs.charts.js.3. Domain-Specific Recommendations
💻 Code Quality & Logic
🛡️ Security
src/flask/views/brews.pyimplements a strict ownership check (e.g.,if run.user_id != current_user.id: abort(403)) for the new "click to correct" functionality.10^{10}) to prevent database corruption or calculation crashes./api/runsendpoint to mitigate potential Denial of Service (DoS) risks associated with large payloads.⚙️ Infrastructure & Platform
Cache-Controlheaders for the consolidated/api/runsendpoint, as historical brewing data is largely static.4. Positive Aspects of the PR
days_to_1015,curves) improves overall codebase maintainability.5. Overall Assessment & Recommendation
Overall Rating: APPROVED WITH MINOR CHANGES
The PR is functionally excellent and provides significant value to the end-user and the system's performance. However, it cannot be fully merged until the security concerns regarding Resource Ownership (IDOR) are addressed.
Required Actions before Merge:
Once the authorization checks are verified, this PR is highly recommended for deployment.
hermes referenced this pull request2026-11-09 16:04:37 +10:00