fix(ui): make the advice pop-out fail-safe against a stale stylesheet (#22)

fix(ui): make the advice pop-out fail-safe against a stale stylesheet (#22)

src/flask/app_factory.py

@@ -87,6 +87,31 @@ def create_app(config: dict | None = None) -> Flask:
     if config:
         app.config.update(config)
 
+    # The pages must not be cached by the browser.
+
+    # A phone held on to a cached copy of the brewing page and showed
+    # the old inline advice instead of the button and the pop-out, so
+    # the change was not visible until the cache was cleared by hand.
+    @app.after_request
+    def _no_store(response):
+        if response.mimetype == "text/html":
+            response.headers["Cache-Control"] = "no-store, must-revalidate"
+            response.headers["Pragma"] = "no-cache"
+        elif response.mimetype in ("text/css", "application/javascript",
+                                    "text/javascript"):
+            # The static files must NEVER be reused from a cache.
+
+            # Firefox kept serving a stale stylesheet even with
+            # `no-cache`, which meant the page had the new markup and
+            # the old CSS: the advice was expanded inline in the table
+            # instead of in a pop-out, and the long and short labels
+            # were both showing ("BREWING ADVICEADVICE").
+
+            # `no-store` is the only one that means "do not keep this".
+            response.headers["Cache-Control"] = "no-store, must-revalidate"
+            response.headers["Pragma"] = "no-cache"
+        return response
+
     csrf = CSRFProtect(app)
 
     # ------------------------------------------------------------------ #

src/flask/mongo/repository.py

@@ -18,6 +18,7 @@ Two collections are used:
 from __future__ import annotations
 
 import logging
+import re
 import os
 import threading
 from typing import Any
@@ -232,7 +233,8 @@ class BeerRepository:
     def set_reading_advice(self, reading_id: str, advice: str,
                             ok: bool = True,
                             expected_sg: int | None = None,
-                            expected_token: str | None = None) -> int:
+                            expected_token: str | None = None,
+                            expected_comment: str | None = None) -> int:
         """Attach the LLM's advice to a reading.
 
         The advice lives in its own field, so the brewer's own comment is
@@ -286,6 +288,31 @@ class BeerRepository:
             # A direct call with a gravity to check against.
             query["sg"] = str(expected_sg)
 
+        if expected_comment is not None:
+            # The comment the advice was written for.
+            #
+            # This is a SECOND, independent guard.  The token covers
+            # every edit the application makes, but a document edited
+            # outside the application - by a script, or by hand in a
+            # database tool - would keep its empty token and a stale
+            # thread's write would still match on the gravity alone.
+            #
+            # The advice is generated from the comment as well as the
+            # gravity, so the comment is part of what the advice
+            # describes.  Matching on it means a stale write can only
+            # land if BOTH the gravity and the comment are unchanged.
+            #
+            # The stored comment and the one the view sees can differ by
+            # trailing whitespace, so a regex is used to match either.
+            #
+            # `^comment\s*$` matches the stored value with any trailing
+            # whitespace, and the exact value too.  It is anchored, so it
+            # cannot match a longer comment that merely starts the same
+            # way.
+            query["comment"] = {
+                "$regex": "^" + re.escape(expected_comment.strip()) + r"\s*$"
+            }
+
         # `$in` matches either form.  A plain list would be an exact
         # ARRAY match in Mongo, which is not what is wanted here.
         result = self.readings.update_one(
@@ -296,7 +323,13 @@ class BeerRepository:
                 "advice_at": _now_iso(),
             }},
         )
-        return result.modified_count
+        # `matched_count`, not `modified_count`.
+        #
+        # `modified_count` is 0 when the document matched but the text
+        # was already identical - which is a successful write, not a
+        # refused one.  The caller logs "the advice was not stored" when
+        # this is 0, so it has to mean "the guard refused it".
+        return result.matched_count
 
     def set_final_reading(self, beer_run_id: str, reading_date: str,
                           final: bool) -> int:

src/flask/static/css/app.css

@@ -484,9 +484,14 @@ table.data .num { font-family: var(--mono); text-align: right; white-space: nowr
 table.data .comment { color: var(--text-muted); font-size: .84rem; }
 
 /* A heading that is too long for a phone column.  The long form shows
-   on a desktop and the short form on a phone - see the media query. */
+   on a desktop and the short form on a phone - see the media query.
-table.data .short { display: none; }
+
-table.data .advice-trigger .short { display: none; }
+   The rule is on `.short` alone, not on `table.data .short`, so it
+   applies everywhere the pattern is used - in a table heading, in the
+   advice button, and on the Active Beers page.  When only the table
+   was covered, the advice button could show both forms at once and
+   read "Read the adviceAdvice". */
+.short { display: none; }
 
 /* On a narrow screen the header wraps onto two rows: the brand and the
    sign-out on the first, the nav tabs on the second where they get the
@@ -570,19 +575,17 @@ table.data .advice-trigger .short { display: none; }
         overflow-wrap: break-word;
     }
 
-    /* The heading gets a short form, because a heading that is cut off
+    /* The long form gives way to the short form on a phone, because a
-       is worse than a heading that is brief. */
+       heading that is cut off is worse than a heading that is brief.
-    table.data .col-abv .long,
+
-    table.data .col-advice .long,
+       The rule is on the span itself rather than on the column, so it
-    table.data .col-finished .long,
+       applies everywhere the pattern is used - in a table heading, in
-    table.data .col-comment .long {
+       the advice button, and on the Active Beers page. */
+    .long {
         display: none;
     }
 
-    table.data .col-abv .short,
+    .short {
-    table.data .col-advice .short,
-    table.data .col-finished .short,
-    table.data .col-comment .short {
         display: inline;
     }
 
@@ -594,14 +597,6 @@ table.data .advice-trigger .short { display: none; }
         gap: .2rem;
     }
 
-    table.data .advice-trigger .long {
-        display: none;
-    }
-
-    table.data .advice-trigger .short {
-        display: inline;
-    }
-
     /* The action column holds a button. */
     table.data .col-action {
         white-space: nowrap;
@@ -1004,10 +999,21 @@ table.data .advice-trigger .short { display: none; }
 
 /* The script opens and closes the panel, and places it.  The CSS only
    decides whether it is shown. */
+/* The panel is opened by the script, which removes the `hidden`
+   attribute and adds `is-open`.
+
+   The `hidden` attribute is what makes this fail-safe: if the
+   stylesheet is stale and does not know about `.advice-pop` at all,
+   the panel is STILL hidden, so the advice cannot appear expanded
+   inside the table cell. */
 .advice-pop.is-open {
     display: block;
 }
 
+.advice-pop[hidden] {
+    display: none;
+}
+
 /* On a phone there is no hover, so the trigger is a button the script
    toggles, and the panel is pinned to the middle of the screen so it is
    readable whatever the table's width. */

src/flask/static/js/app.js

@@ -131,6 +131,11 @@
                         clearTimeout(closeTimer);
                         closeTimer = null;
                     }
+                    /* The `hidden` attribute is removed as well as the
+                       class added, so the panel is shown even if the
+                       stylesheet is an older one that only knows about
+                       the class. */
+                    panel.removeAttribute("hidden");
                     panel.classList.add("is-open");
                     trigger.setAttribute("aria-expanded", "true");
                     place();
@@ -138,6 +143,9 @@
 
                 function close() {
                     panel.classList.remove("is-open");
+                    /* Put the attribute back, so a stale stylesheet
+                       cannot leave the advice expanded in the cell. */
+                    panel.setAttribute("hidden", "");
                     trigger.setAttribute("aria-expanded", "false");
                 }
 

src/flask/templates/brews.html

@@ -126,7 +126,7 @@
                                                 {% endif %}
                                             </span>
                                         </button>
-                                        <div class="advice-pop" role="tooltip">
+                                        <div class="advice-pop" role="tooltip" hidden>
                                             <div class="advice-pop__head">
                                                 <span class="advice__label">Brewing advice</span>
                                                 <button type="button" class="advice-pop__close"

src/flask/views/brews.py

@@ -106,7 +106,15 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None
         # wrong number.
         saved = next((r for r in readings if r.get("_id") == str(reading_id)), None)
         if saved is None:
-            saved = dict(readings[-1]) if readings else {"date": "", "sg": None}
+            # The reading is not in the run.  Advising about the newest
+            # reading instead would describe the wrong number, and the
+            # conditional write would then refuse it anyway - so it is
+            # better to say so plainly and stop.
+            log.info(
+                "no advice for reading %s: it is not one of the run's readings",
+                reading_id,
+            )
+            return
         latest = dict(saved)
         latest["_id"] = reading_id
 
@@ -119,6 +127,11 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None
     # cannot overwrite a correction with advice about the old value.
     advice_sg = latest.get("sg")
     advice_token = latest.get("advice_token")
+    # The comment is part of what the advice describes, so it is a
+    # second, independent guard on the write.  The token covers every
+    # edit the application makes; this also covers a document edited
+    # outside it.
+    advice_comment = latest.get("comment") or ""
 
     def write(read_id: str, ok: bool, text: str) -> None:
         # The client is SHARED, not built per reading.  A MongoClient
@@ -137,6 +150,7 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None
                 read_id, text, ok,
                 expected_sg=advice_sg,
                 expected_token=advice_token,
+                expected_comment=advice_comment,
             )
             if not written:
                 # The reading was corrected while the advice was being
@@ -145,8 +159,8 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None
                 # reading will land in its own turn.
                 log.info(
                     "the advice for reading %s was not stored: the reading "
-                    "has been edited since the advice was generated, so the "
+                    "has been edited since the advice was generated, so its "
-                    "advice no longer matches it",
+                    "editing token or comment no longer matches the advice",
                     read_id,
                 )
         except Exception:  # noqa: BLE001 - never let the thread die noisily

tests/test_app.py

@@ -714,3 +714,15 @@ def test_the_api_exposes_the_attenuation_and_curve(logged_in, repo):
     assert len(entry["curve"]) == 2
     assert entry["curve"][0]["progress"] == 0.0
     assert entry["curve"][1]["progress"] == 1.0
+
+
+def test_the_pages_are_not_cached(client):
+    """The HTML pages must not be cached by the browser.
+
+    A phone held on to a cached copy of the brewing page and kept
+    showing the old inline advice after the button and the pop-out had
+    been deployed, so the change was invisible until the cache was
+    cleared by hand.
+    """
+    response = client.get("/login")
+    assert "no-store" in response.headers.get("Cache-Control", "")

tests/test_brew_advice.py

@@ -1049,6 +1049,142 @@ def test_the_tables_keep_their_columns_on_a_phone(client, repo):
     assert "Cider raspberry elderberry" in body
 
 
+def test_the_long_and_short_labels_are_both_in_the_page(client, repo):
+    """The long and the short label are both in the markup.
+
+    CSS picks which one shows - the long form on a desktop, the short
+    form on a phone.  If only one is in the markup the other cannot be
+    shown at all, and if the CSS rule misses one of them the page shows
+    "Read the adviceAdvice".
+
+    This is a markup assertion, not a CSS assertion: it makes sure
+    both forms exist for the stylesheet to choose between.
+    """
+    repo.users.insert_one({"username": "beer", "password": "beer_pass"})
+    client.post("/login", data={"username": "beer", "password": "beer_pass"},
+                follow_redirects=True)
+
+    seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "")])
+    doc = repo.readings.find_one({"beer_run_id": "1"})
+    repo.set_reading_advice(str(doc["_id"]), "Warm it to 21C and wait.", ok=True)
+
+    body = client.get("/brews").get_data(as_text=True)
+
+    # The heading has both forms.
+    assert '<span class="long">Brewing advice</span>' in body
+    assert '<span class="short">Advice</span>' in body
+
+    # The button has both forms.
+    assert '<span class="long">Read the advice</span>' in body
+    assert '<span class="short">Advice</span>' in body
+
+
+def test_stale_advice_cannot_overwrite_a_comment_only_correction(repo):
+    """A comment-only correction still invalidates the old advice.
+
+    The gravity alone is not enough to tell two generations apart - a
+    comment-only correction leaves the gravity unchanged.  The editing
+    token is what tells them apart, because every edit writes a fresh
+    one.
+    """
+    repo.readings.insert_one({
+        "_id": "readingone",
+        "beer_run_id": "1",
+        "date": "2026-01-01",
+        "sg": "1040",
+        "comment": "original comment",
+        "final_reading": "False",
+        "advice_token": "token-one",
+    })
+
+    # The slow thread holds the token it was given.
+    stale_token = "token-one"
+
+    # The brewer corrects only the comment.
+    repo.readings.update_one(
+        {"_id": "readingone"},
+        {"$set": {"comment": "corrected comment", "advice_token": "token-two"},
+         "$unset": {"advice": "", "advice_ok": "", "advice_at": ""}},
+    )
+
+    # The fresh advice lands first, carrying the new token.
+    assert repo.set_reading_advice(
+        "readingone", "FRESH ADVICE", ok=True, expected_token="token-two") == 1
+
+    # The slow thread lands afterwards, carrying the old token.  It must
+    # be refused.
+    assert repo.set_reading_advice(
+        "readingone", "STALE ADVICE", ok=True, expected_token=stale_token) == 0
+
+    doc = repo.readings.find_one({"_id": "readingone"})
+    assert doc["advice"] == "FRESH ADVICE"
+    assert doc["comment"] == "corrected comment"
+
+
+def test_stale_advice_cannot_overwrite_a_comment_edited_outside_the_app(repo):
+    """The comment is a guard in its own right, not just the token.
+
+    The token covers every edit the application makes.  A document
+    edited outside it - by a script, or by hand - keeps its empty
+    token, so a stale thread's write would still match on the gravity
+    alone.  Matching the comment as well closes that.
+    """
+    repo.readings.insert_one({
+        "_id": "outsideedit01",
+        "beer_run_id": "1",
+        "date": "2026-01-01",
+        "sg": "1040",
+        "comment": "original comment",
+        "final_reading": "False",
+        # No advice_token - a legacy reading.
+    })
+
+    # The advice is written for the original comment.
+    assert repo.set_reading_advice(
+        "outsideedit01", "FRESH ADVICE", ok=True,
+        expected_token="", expected_comment="original comment") == 1
+
+    # Somebody edits the comment directly in the database, leaving the
+    # token empty - so the token cannot tell the generations apart.
+    repo.readings.update_one(
+        {"_id": "outsideedit01"},
+        {"$set": {"comment": "edited outside the app"}},
+    )
+
+    # The slow thread lands.  It must be refused, even though the token
+    # still matches.
+    assert repo.set_reading_advice(
+        "outsideedit01", "STALE ADVICE", ok=True,
+        expected_token="", expected_comment="original comment") == 0
+
+    doc = repo.readings.find_one({"_id": "outsideedit01"})
+    assert doc["advice"] == "FRESH ADVICE"
+    assert doc["comment"] == "edited outside the app"
+
+
+def test_the_advice_pop_out_is_hidden_in_the_markup(client, repo):
+    """The pop-out carries the `hidden` attribute.
+
+    A stale stylesheet - one that does not know about `.advice-pop` at
+    all - would otherwise leave the advice expanded inline in the table
+    cell, running down the column and off the screen.  The `hidden`
+    attribute makes the markup fail-safe: the panel stays hidden until
+    the script opens it.
+    """
+    repo.users.insert_one({"username": "beer", "password": "beer_pass"})
+    client.post("/login", data={"username": "beer", "password": "beer_pass"},
+                follow_redirects=True)
+
+    seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "")])
+    doc = repo.readings.find_one({"beer_run_id": "1"})
+    repo.set_reading_advice(str(doc["_id"]), "Warm it to 21C and wait.", ok=True)
+
+    body = client.get("/brews").get_data(as_text=True)
+
+    assert 'class="advice-pop"' in body
+    assert 'hidden' in body.split('class="advice-pop"')[1][:60]
+
+
 def test_a_failed_advice_is_marked_as_such(client, repo):
     """A generation failure reads differently from real advice."""
     repo.users.insert_one({"username": "beer", "password": "beer_pass"})