The second review of PR #12 (which came back APPROVED) still carried findings I hadn't acted on. Two of them are MEDIUM and both are live on the deployed app — the page was contradicting itself.
The ABV chart plotted the suspect run
Run 12's impossible reading gives it an ABV of 18.64%. The chart's y axis went to 18.64, so the fifteen real bars were squashed into the bottom ~45% of the plot — while the "Strongest 8.53%" tile on the same page showed the correct figure. The chart now leaves the suspect runs out.
The attenuation chart plotted the active runs
An active run's attenuation is measured against its current gravity, so a brew that has just started shows a 0% bar. That is the absence of a result, not a result — and it sat directly above the table of finished brews. The chart now leaves the active runs out.
Verified against the live data:
ABV chart bars: bars=15 max=8.53 (was 16 bars, max 18.64)
attenuation chart bars: bars=15 min=76.2 (was 17 bars, min 0)
A suspect active run no longer vanishes
The API flags suspect runs over every run, so the charts leave them out — but the analysis page only looked at the finished ones. An active run with a bad reading would be dropped from the charts and never listed in "Readings that need a second look", so it would disappear with no explanation anywhere. The analysis page now lists suspect active runs too.
The figures say what they're based on
The analysis prose counted the non-suspect runs, not the ones that actually contributed a figure to the mean. It now counts the latter.
The history page's "Average ABV — across finished brews" was averaging 15 of 16 without saying so, while the analysis page said "16 brews (15 counted)". It now says "across 15 of 16 finished brews".
/api/runs/<run_id> now carries the suspect and active flags that /api/runs does.
Test plan
148 tests pass (up from 142)
New coverage: the ABV chart excluding the suspect runs, the attenuation chart excluding the active runs, a suspect active run still being listed, the history ABV count, the analysis prose count, and the detail API's flags
Browser-verified against the real Mongo data: both charts' bar counts and ranges
Not yet deployed — the workflow deploys from master on merge; I will confirm the rollout afterwards.
## Summary
The second review of PR #12 (which came back APPROVED) still carried findings I hadn't acted on. Two of them are MEDIUM and both are **live on the deployed app** — the page was contradicting itself.
### The ABV chart plotted the suspect run
Run 12's impossible reading gives it an ABV of 18.64%. The chart's y axis went to 18.64, so the fifteen real bars were squashed into the bottom ~45% of the plot — while the **"Strongest 8.53%"** tile on the *same page* showed the correct figure. The chart now leaves the suspect runs out.
### The attenuation chart plotted the active runs
An active run's attenuation is measured against its **current** gravity, so a brew that has just started shows a **0% bar**. That is the absence of a result, not a result — and it sat directly above the table of finished brews. The chart now leaves the active runs out.
Verified against the live data:
```
ABV chart bars: bars=15 max=8.53 (was 16 bars, max 18.64)
attenuation chart bars: bars=15 min=76.2 (was 17 bars, min 0)
```
### A suspect active run no longer vanishes
The API flags suspect runs over **every** run, so the charts leave them out — but the analysis page only looked at the **finished** ones. An active run with a bad reading would be dropped from the charts and never listed in "Readings that need a second look", so it would disappear with no explanation anywhere. The analysis page now lists suspect active runs too.
### The figures say what they're based on
- The analysis prose counted the **non-suspect** runs, not the ones that actually **contributed a figure** to the mean. It now counts the latter.
- The history page's "Average ABV — across finished brews" was averaging **15 of 16** without saying so, while the analysis page said "16 brews (15 counted)". It now says "across 15 of 16 finished brews".
- `/api/runs/<run_id>` now carries the `suspect` and `active` flags that `/api/runs` does.
## Test plan
- [x] 148 tests pass (up from 142)
- [x] New coverage: the ABV chart excluding the suspect runs, the attenuation chart excluding the active runs, a suspect active run still being listed, the history ABV count, the analysis prose count, and the detail API's flags
- [x] Browser-verified against the real Mongo data: both charts' bar counts and ranges
- [ ] **Not yet deployed** — the workflow deploys from `master` on merge; I will confirm the rollout afterwards.
The second review of PR #12 (after the suspect-run fix) found that the
two BAR charts were still plotting runs they should not have been, while
the curve chart and the tiles had been corrected. The page was
contradicting itself.
The ABV chart plotted the suspect run
--------------------------------------
Run 12's impossible reading gives it an ABV of 18.64%. The chart's y
axis went to 18.64, so the fifteen real bars were squashed into the
bottom ~45% of the plot - while the "Strongest 8.53%" tile on the same
page showed the correct figure. The chart now leaves the suspect runs
out.
The attenuation chart plotted the active runs
----------------------------------------------
An active run's attenuation is measured against its CURRENT gravity, so
a brew that has just started shows a 0% bar. That is the absence of a
result, not a result, and it sat directly above the table of finished
brews. The chart now leaves the active runs out.
Verified against the live data:
ABV chart bars: bars=15 max=8.53 (was 16 bars, max 18.64)
attenuation chart bars: bars=15 min=76.2 (was 17 bars, min 0)
A suspect active run no longer vanishes
---------------------------------------
The API flags suspect runs over EVERY run, so the charts leave them out.
But the analysis page only looked at the finished ones, so an active run
with a bad reading would be dropped from the charts and never listed in
"Readings that need a second look" - it would disappear with no
explanation anywhere. The analysis page now lists suspect active runs
too.
The figures say what they are based on
--------------------------------------
- The analysis prose counted the non-suspect runs, not the ones that
actually contributed a figure to the mean. It now counts the latter.
- The history page's "Average ABV — across finished brews" was averaging
15 of 16 without saying so, while the analysis page said "16 brews (15
counted)". It now says "across 15 of 16 finished brews".
- `/api/runs/<run_id>` carries the `suspect` and `active` flags that
`/api/runs` does.
148 tests (up from 142).
PR Title:fix(charts): leave the suspect and active runs out of the bar charts Overall Status:✅APPROVED WITH MINOR SUGGESTIONS
1. Executive Summary
This Pull Request focuses on improving the data integrity and visual accuracy of the application's analytics. The primary goal was to eliminate "noise" (suspect/active runs) that skewed chart axes and caused contradictions between text-based summaries and visual representations.
All three review domains (Code, Security, and Infrastructure) have reached a consensus: the PR is high-quality, well-tested, and solves a critical user-trust issue. No critical security vulnerabilities or infrastructure risks were identified. The review team highlighted a shared concern regarding the centralization of business logic (filtering) to ensure consistency between the API and the Frontend.
2. Prioritized Issues & Observations
Priority
Domain
Issue
Description
Medium
Code/Security
Logic Duplication
Filtering logic for "suspect" runs is implemented in charts.js. If the API continues to send "dirty" data that the frontend must filter, there is a risk of business logic divergence and "UI-only security."
Low
Code
UI String Hardcoding
New descriptive strings in analysis.html and history.html may be hardcoded, potentially leading to maintenance overhead.
Low
Infra/CI
CD Risk
The current workflow deploys from master directly to production without a staging/canary phase, relying solely on automated tests.
Low
Security
Input Validation
Need to ensure <run_id> in brews.py is strictly validated against MongoDB ObjectId formats to prevent malformed query crashes.
3. Domain-Specific Recommendations
💻 Code Quality & Maintainability
Backend-Driven Filtering: Transition from client-side filtering in charts.js to API-side filtering. Introduce query parameters (e.g., /api/runs?exclude_suspect=true) to ensure the "source of truth" for what constitutes a "suspect run" resides in one place.
Template Helpers: Instead of hardcoding phrasing like "across X of Y finished brews," implement a Jinja2 helper function or template filter to maintain consistency across analysis.html and history.html.
Type Safety: Verify that suspect and active flags are strictly Booleans across both the list and detail endpoints to prevent frontend casting errors.
🛡️ Security Posture
Centralized Validation: Ensure the logic determining "suspect" status is located in a single service layer in the Flask backend, not replicated in the UI.
API Hardening: Explicitly validate the run_id parameter in src/flask/views/brews.py to mitigate NoSQL injection or application-level crashes.
Security Testing: Add a specific test case to verify if a "suspect" run should be completely inaccessible via API or merely hidden from visualizations.
⚙️ Infrastructure & Reliability
Environment Strategy: To reduce the risk of the "merge-to-master" CD pipeline, consider implementing Preview Environments (ephemeral namespaces) to allow browser verification on deployed code before the final merge.
Ingestion Validation: Implement a schema validation layer at the API ingestion point to flag "impossible" data (e.g., the 18.64% ABV example) before it is persisted to MongoDB.
4. Positive Highlights
Attention to Detail: The developer didn't just fix the chart; they identified and fixed a secondary bug where suspect active runs were "vanishing" from the UI entirely.
Robust Testing: A significant effort was made to expand the test suite (test_analysis.py), increasing the total count of passing tests and reducing regression risk.
API Parity: Correctly identified and closed the gap between the collection endpoint and the detail endpoint (/api/runs/<run_id>).
UX Improvement: The update to the UI prose increases transparency, informing the user exactly why certain data points are excluded.
5. Final Assessment
Recommendation:Approved
The PR is technically sound and provides immediate value to the end-user. While the recommendations regarding centralized filtering and CI/CD improvements are important for long-term architectural health, they do not block the current release.
Post-Merge Action:
Monitor the production rollout to ensure MongoDB data behaves as expected in the live environment.
Schedule a follow-up task to migrate filtering logic from charts.js to the Flask API.
## PR Review Results
# Unified Code Review Synthesis Report
**PR Title:** `fix(charts): leave the suspect and active runs out of the bar charts`
**Overall Status:** ✅ **APPROVED WITH MINOR SUGGESTIONS**
---
## 1. Executive Summary
This Pull Request focuses on improving the data integrity and visual accuracy of the application's analytics. The primary goal was to eliminate "noise" (suspect/active runs) that skewed chart axes and caused contradictions between text-based summaries and visual representations.
All three review domains (Code, Security, and Infrastructure) have reached a consensus: the PR is high-quality, well-tested, and solves a critical user-trust issue. No critical security vulnerabilities or infrastructure risks were identified. The review team highlighted a shared concern regarding the **centralization of business logic** (filtering) to ensure consistency between the API and the Frontend.
---
## 2. Prioritized Issues & Observations
| Priority | Domain | Issue | Description |
| :--- | :--- | :--- | :--- |
| **Medium** | Code/Security | **Logic Duplication** | Filtering logic for "suspect" runs is implemented in `charts.js`. If the API continues to send "dirty" data that the frontend must filter, there is a risk of business logic divergence and "UI-only security." |
| **Low** | Code | **UI String Hardcoding** | New descriptive strings in `analysis.html` and `history.html` may be hardcoded, potentially leading to maintenance overhead. |
| **Low** | Infra/CI | **CD Risk** | The current workflow deploys from `master` directly to production without a staging/canary phase, relying solely on automated tests. |
| **Low** | Security | **Input Validation** | Need to ensure `<run_id>` in `brews.py` is strictly validated against MongoDB ObjectId formats to prevent malformed query crashes. |
---
## 3. Domain-Specific Recommendations
### 💻 Code Quality & Maintainability
* **Backend-Driven Filtering:** Transition from client-side filtering in `charts.js` to API-side filtering. Introduce query parameters (e.g., `/api/runs?exclude_suspect=true`) to ensure the "source of truth" for what constitutes a "suspect run" resides in one place.
* **Template Helpers:** Instead of hardcoding phrasing like *"across X of Y finished brews,"* implement a Jinja2 helper function or template filter to maintain consistency across `analysis.html` and `history.html`.
* **Type Safety:** Verify that `suspect` and `active` flags are strictly Booleans across both the list and detail endpoints to prevent frontend casting errors.
### 🛡️ Security Posture
* **Centralized Validation:** Ensure the logic determining "suspect" status is located in a single service layer in the Flask backend, not replicated in the UI.
* **API Hardening:** Explicitly validate the `run_id` parameter in `src/flask/views/brews.py` to mitigate NoSQL injection or application-level crashes.
* **Security Testing:** Add a specific test case to verify if a "suspect" run should be completely inaccessible via API or merely hidden from visualizations.
### ⚙️ Infrastructure & Reliability
* **Environment Strategy:** To reduce the risk of the "merge-to-master" CD pipeline, consider implementing **Preview Environments** (ephemeral namespaces) to allow browser verification on deployed code before the final merge.
* **Ingestion Validation:** Implement a schema validation layer at the API ingestion point to flag "impossible" data (e.g., the 18.64% ABV example) *before* it is persisted to MongoDB.
---
## 4. Positive Highlights
* **Attention to Detail:** The developer didn't just fix the chart; they identified and fixed a secondary bug where suspect active runs were "vanishing" from the UI entirely.
* **Robust Testing:** A significant effort was made to expand the test suite (`test_analysis.py`), increasing the total count of passing tests and reducing regression risk.
* **API Parity:** Correctly identified and closed the gap between the collection endpoint and the detail endpoint (`/api/runs/<run_id>`).
* **UX Improvement:** The update to the UI prose increases transparency, informing the user exactly why certain data points are excluded.
---
## 5. Final Assessment
**Recommendation:** **Approved**
The PR is technically sound and provides immediate value to the end-user. While the recommendations regarding centralized filtering and CI/CD improvements are important for long-term architectural health, they do not block the current release.
**Post-Merge Action:**
* Monitor the production rollout to ensure MongoDB data behaves as expected in the live environment.
* Schedule a follow-up task to migrate filtering logic from `charts.js` to the Flask API.
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
The second review of PR #12 (which came back APPROVED) still carried findings I hadn't acted on. Two of them are MEDIUM and both are live on the deployed app — the page was contradicting itself.
The ABV chart plotted the suspect run
Run 12's impossible reading gives it an ABV of 18.64%. The chart's y axis went to 18.64, so the fifteen real bars were squashed into the bottom ~45% of the plot — while the "Strongest 8.53%" tile on the same page showed the correct figure. The chart now leaves the suspect runs out.
The attenuation chart plotted the active runs
An active run's attenuation is measured against its current gravity, so a brew that has just started shows a 0% bar. That is the absence of a result, not a result — and it sat directly above the table of finished brews. The chart now leaves the active runs out.
Verified against the live data:
A suspect active run no longer vanishes
The API flags suspect runs over every run, so the charts leave them out — but the analysis page only looked at the finished ones. An active run with a bad reading would be dropped from the charts and never listed in "Readings that need a second look", so it would disappear with no explanation anywhere. The analysis page now lists suspect active runs too.
The figures say what they're based on
/api/runs/<run_id>now carries thesuspectandactiveflags that/api/runsdoes.Test plan
masteron merge; I will confirm the rollout afterwards.PR received — starting review, sit tight 🫡
PR Review Results
Unified Code Review Synthesis Report
PR Title:
fix(charts): leave the suspect and active runs out of the bar chartsOverall Status: ✅ APPROVED WITH MINOR SUGGESTIONS
1. Executive Summary
This Pull Request focuses on improving the data integrity and visual accuracy of the application's analytics. The primary goal was to eliminate "noise" (suspect/active runs) that skewed chart axes and caused contradictions between text-based summaries and visual representations.
All three review domains (Code, Security, and Infrastructure) have reached a consensus: the PR is high-quality, well-tested, and solves a critical user-trust issue. No critical security vulnerabilities or infrastructure risks were identified. The review team highlighted a shared concern regarding the centralization of business logic (filtering) to ensure consistency between the API and the Frontend.
2. Prioritized Issues & Observations
charts.js. If the API continues to send "dirty" data that the frontend must filter, there is a risk of business logic divergence and "UI-only security."analysis.htmlandhistory.htmlmay be hardcoded, potentially leading to maintenance overhead.masterdirectly to production without a staging/canary phase, relying solely on automated tests.<run_id>inbrews.pyis strictly validated against MongoDB ObjectId formats to prevent malformed query crashes.3. Domain-Specific Recommendations
💻 Code Quality & Maintainability
charts.jsto API-side filtering. Introduce query parameters (e.g.,/api/runs?exclude_suspect=true) to ensure the "source of truth" for what constitutes a "suspect run" resides in one place.analysis.htmlandhistory.html.suspectandactiveflags are strictly Booleans across both the list and detail endpoints to prevent frontend casting errors.🛡️ Security Posture
run_idparameter insrc/flask/views/brews.pyto mitigate NoSQL injection or application-level crashes.⚙️ Infrastructure & Reliability
4. Positive Highlights
test_analysis.py), increasing the total count of passing tests and reducing regression risk./api/runs/<run_id>).5. Final Assessment
Recommendation: Approved
The PR is technically sound and provides immediate value to the end-user. While the recommendations regarding centralized filtering and CI/CD improvements are important for long-term architectural health, they do not block the current release.
Post-Merge Action:
charts.jsto the Flask API.