A dismissed card stays dismissed, even offline
The server already suppressed every span she had accepted or dismissed, on
both the LLM reconcile and the mechanics pass, with tests either side. What
had no memory was the half that never asks it: item 3b's rule pack renders
250 ms after a keystroke with no network, and its record of "she already
answered this" was a set cleared on every document switch and added to only
for cards dismissed while still provisional.
So dismissing a persisted rule-pack card recorded nothing client-side and the
next keystroke put it straight back until the server's reply removed it again;
and after a reload the client knew nothing at all — permanently so with the
server unreachable, which is the case the rule pack exists for.
GET /docs/{id}/settled hands over the normalized originals of the document's
actioned rows, scoped through documents because an original quotes her
sentence. The client seeds a SettledSpans from it on open and adds to it for
every card that leaves, keyed on the original alone the way the server keys
it. The load adds rather than assigns, so a dismissal made while it is in
flight survives it.
normalizeForDedup now exists in both languages, compared across a network
boundary, so the same nine cases are asserted on both sides and each test
names the other.
Also: the status-bar count — "🌸 5片花瓣待打磨 · 5 petals to polish" beside the
word count, from the packs, hidden at zero. An empty rail already says nothing
is waiting; a badge announcing it after every check is a verdict, which the
review's non-goals rule out.
Verified in Chrome at 1517x810 with the server killed: a new violation was
detected, underlined and counted with no network, while the dismissed span
stayed gone.
This commit is contained in:
+133
-6
@@ -755,6 +755,112 @@ written down as such rather than covered by a test that would pass regardless.
|
||||
the word count — the gentle version of Grammarly's score. No numeric
|
||||
grade, per the north star. Acceptance: count updates live with the rail.
|
||||
|
||||
### 8 — dismissal persistence and the status-bar summary DONE (ninth session).
|
||||
|
||||
The two the handoff picked. They turned out to be opposite shapes: one was
|
||||
almost entirely built and needed a small piece in an unexpected place; the
|
||||
other was new but tiny.
|
||||
|
||||
**Dismissal persistence was already true of everything the server stores.**
|
||||
`buildSuppressor` indexes accepted *and* rejected rows and is wired into both
|
||||
the LLM reconcile and `replaceMechanics`, with tests either side
|
||||
(`TestResolvedSuggestionsNotReproposed`, `TestMechanicsActionedSuppression`).
|
||||
The item's own acceptance criterion — dismiss → edit elsewhere → recheck →
|
||||
it doesn't return — held before this session started.
|
||||
|
||||
**What wasn't true was the half that never asks the server.** Item 3b gave the
|
||||
rule pack a 250 ms fuse that renders findings with no network at all, and the
|
||||
detector reads the text alone, so something has to tell it what she has
|
||||
already answered. That memory was `actionedRef`: a set of
|
||||
`original + replacement` keys, added to *only* for cards dismissed while still
|
||||
provisional, and cleared on every document switch. Two consequences, both
|
||||
real:
|
||||
|
||||
- Dismiss a **persisted** rule-pack card and nothing recorded it client-side.
|
||||
The next keystroke re-detected it and put it back on screen; the server's
|
||||
reply then removed it again. A flicker every few keystrokes, on a card she
|
||||
had just answered.
|
||||
- After a **reload** the client knew nothing at all — and with the server
|
||||
unreachable, which is the case the rule pack exists for, the reply that
|
||||
would have corrected it never comes. The dismissed card simply stays.
|
||||
|
||||
**Implemented:**
|
||||
|
||||
- `GET /docs/{id}/settled` (`handlers.go`) — the normalized originals of every
|
||||
accepted or dismissed row on the document. Scoped through `documents` like
|
||||
`fetchPending`, and for the same reason: an `original` is a verbatim
|
||||
quotation of her sentence, so an unscoped read here leaks prose to anyone
|
||||
holding a doc id. Wrapped in an object rather than returned as a bare array,
|
||||
so it can grow a field later.
|
||||
- `lib/settled.ts` — `SettledSpans`, and a TypeScript `normalizeForDedup`
|
||||
mirroring the Go one. Keyed on the **original alone**, which is how the
|
||||
server keys it: dismissing an edit settles the span, not one rewrite of it.
|
||||
- `useCheckpoint.ts` — the set is seeded from that endpoint when the document
|
||||
opens and added to by `removeSuggestion` for *every* card that leaves, not
|
||||
just provisional ones. The load `add`s rather than assigns, so a card she
|
||||
dismisses while the fetch is in flight isn't forgotten when it lands.
|
||||
|
||||
**The status-bar summary** is `petalsToPolish(n)` in the three packs plus six
|
||||
lines in `StatusBar.tsx`. Two decisions worth keeping:
|
||||
|
||||
- **Nothing is shown at zero.** An empty rail already says there is nothing
|
||||
waiting; a badge that appears after every check to announce it is a verdict
|
||||
on each pass, which is the pressure this review's own non-goals rule out.
|
||||
- **Native half first**, against the item's example, which wrote it
|
||||
English-first. Everything else in Petal leads with the pair language — it is
|
||||
the order the packs use and the one item 6 settled — and the status bar is
|
||||
not the place to be inconsistent about it.
|
||||
|
||||
**A duplicated function, deliberately, with the duplication tested.** The
|
||||
server normalizes the spans it sends and the client normalizes the findings it
|
||||
compares against them, across a network boundary, in two languages. A drift
|
||||
there is silent — a dismissed card quietly coming back — so the same nine
|
||||
cases are asserted on both sides (`TestNormalizeMatchesTheClient` and the head
|
||||
of `settled.test.ts`), each naming the other and saying: add to both or
|
||||
neither.
|
||||
|
||||
**Verified in a real browser at the review's own 1517×810**, on a local build
|
||||
with no model (the rule pack needs none), against a fresh database:
|
||||
|
||||
- Five findings in one paragraph; the bar read `🌸 5片花瓣待打磨 · 5 petals to
|
||||
polish`. Dismissed "a apple" → **4**, live, and the underline went with it.
|
||||
- Typed elsewhere so the 250 ms pass ran: the dismissed card **did not come
|
||||
back** — the in-session half.
|
||||
- **Reloaded.** `/settled` fires alongside `/suggestions` at doc open (both at
|
||||
269 ms). Typed again: still gone, though the text still contains "a apple"
|
||||
and the detector had flagged that exact string twenty minutes earlier.
|
||||
- **Killed the server and kept typing.** A *new* violation ("a office") was
|
||||
detected, underlined and counted with no network at all — proving the local
|
||||
pass really was running — while the dismissed span stayed gone, and the bar
|
||||
went to 5 next to "Couldn't save". That is the case the whole item is worth
|
||||
anything for, and it is the one the old code could not have passed.
|
||||
|
||||
**An observation, not fixed, and not this item's:** "He walk to a office" got
|
||||
a card for the article and none for the verb. `subjectVerbAgreement` in
|
||||
`prose.ts` catches "She have" but not "He walk", so it is narrower than item
|
||||
3b's summary of it implies. Untouched here — a rule-pack gap belongs with
|
||||
whoever next opens `prose.ts`.
|
||||
|
||||
**A trap worth recording, and it is the eighth session's trap wearing a
|
||||
different hat.** The first local run served a bundle hash that didn't match
|
||||
`web/dist` — because a *stale petal from an earlier session was still holding
|
||||
the port* and the new process died unbound. Same failure mode as last time,
|
||||
different cause: the check that catches it is the same one, comparing the
|
||||
served `index-*.js` against `dist/index.html` **before** believing anything on
|
||||
screen.
|
||||
|
||||
Coverage: `settled_test.go` (accepted and dismissed both settle, pending never
|
||||
does, normalization collapses two spellings into one, the empty case is a list
|
||||
and not a `null` the client would throw on, and the mirrored normalize table),
|
||||
an isolation subtest proving a stranger reads no settled span from her
|
||||
document, `settled.test.ts` (the mirrored table, plus the in-flight-dismissal
|
||||
case the `add`-don't-assign choice exists for), and an `i18n.test.ts` case that
|
||||
every pack counts in both halves, keeps its English half in English, and knows
|
||||
one from many.
|
||||
|
||||
**Still open in item 8:** Accept All per category, and the keyboard triage
|
||||
flow. Both untouched.
|
||||
|
||||
---
|
||||
|
||||
## Explicit non-goals (from this review)
|
||||
@@ -837,6 +943,18 @@ restart that silently failed left an old binary serving an old bundle through
|
||||
two rounds of measurement, and nothing about the running app looked wrong.
|
||||
Untouched: 8, item 3's incremental half.)*
|
||||
|
||||
*(Ninth session: item 8's dismissal persistence and status-bar summary done —
|
||||
see the subsection under item 8. Two things to carry forward. First, **"the
|
||||
server already does this" is not the same as "Petal does this"**: every
|
||||
suppression the item asked for was in place and tested, and the defect lived
|
||||
entirely in the 250 ms pass that by design never asks the server. Any item
|
||||
whose answer is "the server handles it" should now be checked against the
|
||||
offline path too, because since item 3b there is always one. Second, **a stale
|
||||
server from an earlier session can hold the port**, so a new binary dies
|
||||
unbound and the old bundle keeps serving — the eighth session's lesson with a
|
||||
different cause, and the same bundle-hash check catches it. Untouched: item
|
||||
8's Accept All and keyboard flow, item 3's incremental half.)*
|
||||
|
||||
**Migration 0015 on the live database.** It rebuilds the suggestions table, so
|
||||
unlike 0014 it could have dropped her rows. Backed up first — and the backup
|
||||
had to be the whole WAL set (`petal.db`, `-wal`, `-shm` in
|
||||
@@ -851,12 +969,21 @@ CHECK, and every existing row still carrying the label she has already read (the
|
||||
seven `clarity` rows include the mislabelled Chinese one — by design, only new
|
||||
findings get the new type; if you want that card relabelled, edit the sentence).
|
||||
|
||||
**Suggested next (eighth session onward):** only **item 8**'s four small ones
|
||||
and item 3's incremental half remain. **Dismissal persistence** is still the one
|
||||
with real value now that item 2 gives suggestions stable identity across checks;
|
||||
the status-bar summary is the cheapest. Item 3's incremental surfacing needs
|
||||
streaming, which the current `/check` response shape doesn't do — it remains the
|
||||
largest of what's left.
|
||||
**Suggested next (ninth session onward):** three things remain in the whole
|
||||
review. **Accept All per category** is the one with real value left — five
|
||||
tense fixes are still five clicks, and item 2's stable ids make a batch safe to
|
||||
reason about; the one design question it has to answer is that its undo must be
|
||||
a single step. **Keyboard triage** is next, and is bigger than it looks: item 7
|
||||
made the anchored popover the primary surface in both layouts, so "cycle the
|
||||
underlines" now means driving that popover, not the rail. **Item 3's
|
||||
incremental surfacing** is still the largest — it needs a streaming `/check`,
|
||||
which the current response shape doesn't do — and it is the only one left that
|
||||
changes how the app *feels* rather than what it can do.
|
||||
|
||||
*(Superseded, kept for the reading list: the eighth session recommended
|
||||
dismissal persistence and the status-bar summary, both now done. Its reasoning
|
||||
— that stable identity from item 2 made dismissal worth doing — was right, but
|
||||
for a different reason than it supposed: see the subsection under item 8.)*
|
||||
|
||||
*(Superseded, kept for the reading list: the seventh session recommended item 6,
|
||||
which is now done.)* **item 6** was the obvious pick —
|
||||
|
||||
Reference in New Issue
Block a user