diff --git a/UX_REVIEW_2026-07-27.md b/UX_REVIEW_2026-07-27.md index 951dc57..29c1e10 100644 --- a/UX_REVIEW_2026-07-27.md +++ b/UX_REVIEW_2026-07-27.md @@ -143,6 +143,81 @@ instability, doubles the pause after each accept, and burns qwen3.5 tokens. or position of any other card; re-check traffic after a one-sentence edit contains only that sentence's chunk; explanations are stable across rounds. +### 2 — DONE (fourth session). Server-side; the client needed nothing. + +Both halves shipped, and they turned out to be one idea. The root cause of +the vanish/reappear was structural: **every pass deleted its whole family +and re-inserted it**, so each round minted new row ids. The rail keys its +cards on `suggestion.id`, so a full remount was guaranteed — new id, new +`created_at` (hence the re-fired arrival chime), and a freshly-worded +explanation from a model that re-reasons every time it's asked. + +**Implemented:** + +- `chunk.go` — splits the document into sentences and hashes each. Newlines + always break; ASCII terminators need trailing whitespace (so `3.50` and + `Ms.` stay whole); `。！？` break outright, since Chinese runs sentences + together with no space and she writes both languages in one document. The + hash normalizes quotes and whitespace runs through the existing + `normalizeForDedup`, so the editor's constant quote rewriting and a + reflowed paragraph cost nothing. Identity is the hash, not the position — + insert a paragraph at the top and every sentence below keeps its cards. +- `reconcile.go` — passes now *reconcile* rather than replace. A row on a + sentence this pass didn't ask about is kept untouched; a row whose + sentence is gone is dropped; a row on a sentence that was re-read survives + only if the model proposed the same edit again, keeping its id, + `created_at` and its **original explanation**. Re-proposals are matched on + `(original, replacement)` normalized — not on type, so a re-labelled edit + keeps the label she's already reading. +- `checked_chunks` (migration `0014`) records which sentences a family has + read. The grammar checkpoint asks only about the difference. **When + nothing changed it doesn't call the model at all** — and doesn't consume + its rate-limit slot, so an idle check can't throttle the next real edit. +- The tone is folded into a sentence's hash, so switching doc type still + re-reads every line: the same sentence gets different advice as an + academic essay than as a journal entry, and cached advice was written for + the old register. +- `replaceMechanics` reconciles too. This mattered more than expected: the + rule pack fires 250 ms after a keystroke (item 3b), so it was re-minting + every local card's id several times a sentence. + +**Deliberately not done:** + +- Only the grammar checkpoint is chunked. Voice is a property of the + document as a whole — a sentence isn't inconsistent with itself — and the + collocation coach is a button she presses asking for a fresh read. Both + still read everything, but both now reconcile, so they keep their ids. +- No client change. With stable ids the existing code already does what the + item asked for: the rail keeps its card DOM, an expanded card survives a + re-check, and the chime (which keys on id) stops re-firing for advice she + is already reading. The one-card optimistic removal on accept was already + there. + +**Sentences the model can't be trusted to have read.** Two guards the plan +didn't anticipate, both found while writing the tests: a finding is +attributed to a sentence the model was *actually shown* before falling back +to the whole document (a short span like "the the" can occur twice, and +crediting the cached copy would drop it); and a cached row whose quoted span +no longer matches byte-for-byte is dropped *and* its sentence re-opened, +rather than caching advice the frontend can't anchor. + +**Verified on the running binary**, not just in tests — per the handoff's +own advice. Against a stand-in model server: three checks over a two- +sentence document, editing only the second. The model received exactly +`She goes to market yesterday.` and never saw the first sentence; the +untouched card kept its id and its first explanation across all three +rounds; the fixed sentence's card was dropped; the idle re-check made zero +model calls. Mechanics identity confirmed the same way (a finding kept its +row id while its span moved). + +Coverage: `chunk_test.go` (splitting, CJK, decimals, cosmetic churn) and +`stability_test.go` (untouched cards keep id + explanation, unchanged +document skips the model, deleted sentence drops its card, tone change +re-opens everything, mechanics rows keep identity). Two existing tests +changed contract deliberately — `TestFickleEditsSuppressed` and +`TestCollocationPassCoexists` both re-checked a document nobody had edited, +which is now a no-op; they edit the text between passes, as she always does. + ## 3. Perceived latency: mask the LLM round-trip Measured ~8–15 s from typing-stop to cards, with only a small "Checking…" @@ -166,6 +241,52 @@ in the status bar. Two independent levers, both worth doing: offline; during a full check, at least one card appears before the last chunk finishes; the status bar shows a running count. +### 3b — DONE (third session). The rules existed; the latency didn't. + +Scoped against the code as the handoff advised, and the handoff was right: +`prose.ts` already carries every rule this item asks for — `articles` +(a/an), `pluralAfterNumber`, `subjectVerbAgreement`, `uncountables` — and +they already surface as real cards via the `mechanics` family. Nothing to +write there. The gap was purely *when* they render: `mechanicsFindings` ran +only inside `runCheck`, behind the same 4000 ms checkpoint debounce as the +LLM, and only reached the screen via the server's reply. So a free, +instant, offline-capable detection was being delivered at network speed on +an LLM-shaped delay. + +**Implemented:** +- `useCheckpoint.ts` — the rule pack gets its own `FAST_MS = 250` fuse, + separate from the 4 s checkpoint. It renders its findings as + *provisional* suggestions with no network at all, then persists them; the + server's reply is authoritative and clears the provisional set. If the + reply never comes (offline, server down) the cards simply stay — which is + the point of a rule pack. +- Provisional cards carry a `local: ` id. The merge + matches on wording, not position, so a card can't flicker into a + duplicate of its own persisted twin while she types around it. +- `resolveServerId` maps a card to the row the API can act on, awaiting the + in-flight submit if she accepts inside that window — so an early accept + still records the keep and plants its word in the garden instead of being + silently dropped. Null means no row exists (offline); the edit has landed + regardless. +- Findings she actions while provisional are remembered client-side + (`actionedRef`), because the detector has no memory between runs. The + server already keeps the equivalent record for persisted rows. +- `runCheck` no longer re-submits mechanics for text the fast pass already + filed; it's now a catch-up path for when that submit failed. +- `App.tsx` — accept/dismiss go through `resolveServerId`; the arrival + chime keys rule-pack cards by wording so one finding doesn't chime twice + (once provisional, once persisted). + +**Deliberately not done:** no distinct "modest style" for unconfirmed local +hits. The rail renders LLM and rule-pack cards identically on purpose (see +the note on `Suggestion.source` in `client.ts`), and a provisional card now +lives for one LAN round-trip. Styling it differently would be a visible +regression against an existing decision, not polish. + +**Still open from item 3:** the incremental-surfacing half (per-chunk LLM +results) and the running count in the status bar — both belong with item 2's +chunking and item 8's status-bar summary. + ## 4. Rail scrolls away from the text With ~7 cards the rail is taller than the viewport; scrolling to reach @@ -181,6 +302,74 @@ takes over when the stack exceeds the viewport. **Acceptance:** with 10+ suggestions, the flagged text stays visible while scrolling the card list; hover-linking still highlights the right span. +### 4 — DONE (fifth session). The cards weren't distant; they were unreachable. + +Measured on the live build before touching anything, and the item understates +its own bug. Her open document: **four cards, 173 px each, all anchored inside +126 px of text** — the stack resolved to tops 4 / 189 / 374 / 559, so ~714 px +of cards beside four lines of prose. And because `.petal-rail` is +`position: absolute`, none of that counts as layout height: the page reported +`scrollHeight === clientHeight`, **no scroll container at all**. On the review's +810 px viewport the lower cards weren't merely severed from their sentence, +they were off-screen with no way to scroll to them. That is the real defect, +and it is why the item read as a scrolling problem. + +**Implemented:** + +- `SuggestionRail.tsx` — the stack reports how far it reaches (`onExtent`), + computed in the same pass that resolves the collision-avoided tops. +- `EditorCore.tsx` — the wrapper takes `minHeight: railExtent + 24`, so the + space the cards occupy becomes real, scrollable page. `minHeight` never + shrinks the column, so a rail that fits beside its text changes nothing. +- The prose moved into its own box, pinned with `position: sticky` while the + stack overhangs it, so scrolling down to reach the lower cards no longer + carries every sentence off the top. The offset is `min(0, port − content)`: + prose shorter than the viewport pins at the top; **taller prose pins by its + bottom edge**, so the last lines — the ones the overhanging cards flag — + stay visible rather than the first. +- The extent is cleared when the last card goes, or the window narrows past + the rail's threshold; otherwise the column keeps the height of a stack that + no longer exists. + +**A trap worth recording.** That prose box must be left at its natural height. +The first version kept the existing `h-full`, so it measured the wrapper — which +this change had just grown to the stack's height — and reported the cards' +height back as the text's own. `railExtent > contentH` was then never true and +the pin could never trip. It typechecked, looked right, and did nothing; only +measuring the running page caught it (`proseHeight: 1424` for a two-line +document). + +**Verified in a real browser at the review's own 1517×810**, driving the local +build with the rule pack from item 3b — which needs no model, so eight cards +appear offline in one paragraph. All three branches exercised: + +- *Overhang, short prose* — 8 cards, stack 1400 px, prose 95 px. Page gained + 675 px of scroll where it previously had none; scrolled to the end, the last + card sits fully in view (770–926) **and the prose is still on screen** (80–175). +- *Overhang, tall prose* — port 225 px, prose 347 px → `top: −146px`. Ordinary + scrolling is untouched (at `scrollTop` 200 the text moves normally with the + page); only at the overhang does it pin, bottom-anchored, last lines visible. +- *No overhang* — the port stays unscrollable and nothing moves. + +Hover-linking re-checked on the last card, the one this fix made reachable at +all: it glows the right span ("It make"), the span is on screen, the card lifts. + +**Known limit, not fixed.** The overlays anchored in wrapper coordinates (gloss +tip, selection bubble, word/misspell cards, confetti) rely on the invariant +noted at `recomputeRail` — "stable under scroll since text and wrapper scroll +together" — which the pin breaks. They are still placed correctly when opened, +because their coordinates come from live rects; they drift only if she scrolls +*while one is open* *and* the column is pinned, i.e. inside the overhang. Left +alone rather than papered over; if it ever bites, the fix is to close or +re-anchor them on scroll. + +**Deliberately not done:** no compaction of the cards. Making crowded cards +drop to a one-line form is the obvious way to shorten the stack, and it is +wrong here — the explanation *is* the teaching, and hiding it from an ESL +writer to save vertical space trades the product's purpose for tidiness. Ten +cards cannot sit beside four lines of text; the answer is to make the overhang +navigable, not to shrink what each card says. + ## 5. Mixed-language spans: offer translation, don't ignore **Status (follow-up session): premise partly wrong — re-scope before @@ -289,6 +478,23 @@ if you're comparing against memory of the live site, that's why. reproducible) and item 5's original premise (re-scoped, much cheaper now). **Untouched:** items 2, 3, 4, 6, 7, 8. +*(Third session: item 3b done — see the subsection under item 3. Item 3's +incremental-surfacing half remains. Untouched: 2, 4, 6, 7, 8.)* + +*(Fourth session: item 2 done — see the subsection under it. **Neither 3b +nor 2 is deployed yet**: both sit on unmerged topic branches +(`feat/instant-local-rules`, then `feat/stable-suggestions` stacked on it) +and `main` is still at `ba06d90`. Untouched: 4, 6, 7, 8. Item 3's +incremental-surfacing half is now cheap — the chunking it was waiting on +exists — but it needs streaming, which the current `/check` shape doesn't +do.)* + +*(Fifth session: item 4 done — see the subsection under it. Still nothing +deployed: `main` remains at `ba06d90`, and 3b → 2 → 4 are now three stacked +topic branches. **Merging and deploying that stack is the obvious next move** +— three sessions of work she hasn't seen. Untouched: 6, 7, 8, item 3's +incremental half, and item 5's re-scoped Translate card type.)* + **Suggested next:** item 3b, the instant local rules layer — but it is **largely already built, in `main`**, and the item as written doesn't know that. Before writing any rules engine, read: diff --git a/internal/db/db.go b/internal/db/db.go index 8e91c31..b5ea7e1 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -497,6 +497,32 @@ CREATE INDEX idx_suggestions_resolved ON suggestions(status, resolved_at); stmt: ` ALTER TABLE suggestions ADD COLUMN source TEXT NOT NULL DEFAULT 'llm'; UPDATE suggestions SET source = 'local' WHERE type = 'mechanics'; +`, + }, + { + // Sentence-level identity, so a re-check stops regenerating the world. + // Every pass used to delete its whole family and re-insert it, which + // meant accepting one edit gave every other card a new id and a newly + // worded explanation — the rail visibly emptied and refilled, and the + // model was asked again about sentences nobody had touched. + // + // `chunk_hash` records which sentence a suggestion belongs to, and + // checked_chunks records which sentences a family has already read. A + // re-check then asks only about the difference and keeps the rest of + // the rows exactly as they are, id and wording included. + // + // Existing rows get '' — "sentence unknown", which reads as in-play, so + // they are simply reconciled on the next pass like any fresh finding. + name: "0014_suggestion_chunk_hash", + stmt: ` +ALTER TABLE suggestions ADD COLUMN chunk_hash TEXT NOT NULL DEFAULT ''; + +CREATE TABLE checked_chunks ( + doc_id TEXT NOT NULL REFERENCES documents(id) ON DELETE CASCADE, + family TEXT NOT NULL, + hash TEXT NOT NULL, + PRIMARY KEY (doc_id, family, hash) +); `, }, } diff --git a/internal/suggestions/chunk.go b/internal/suggestions/chunk.go new file mode 100644 index 0000000..611d9a2 --- /dev/null +++ b/internal/suggestions/chunk.go @@ -0,0 +1,145 @@ +package suggestions + +import ( + "crypto/sha256" + "encoding/hex" + "strings" + "unicode" +) + +// Chunking splits a document into sentence-sized units so a re-check can ask the +// model only about the sentences that actually changed. Accepting one edit used +// to re-run the whole document: every card vanished, came back with a new id and +// a freshly-worded explanation, and spans re-merged into different shapes. The +// sentences she didn't touch have nothing new to say about themselves, so their +// suggestions are simply kept (see reconcilePending). +// +// A chunk's identity is its hash, not its position — she inserts a paragraph at +// the top and every sentence below keeps its suggestions. + +// chunk is one sentence of the document, with the hash that identifies it. +type chunk struct { + text string + hash string +} + +// asciiTerminators end a sentence only when whitespace (or the end of the text) +// follows, so "3.5" and "Ms." don't split mid-word — a wrong split costs only a +// slightly smaller chunk, but a split inside a number would churn its hash on +// every keystroke around it. +const asciiTerminators = ".!?" + +// cjkTerminators end a sentence outright: Chinese runs sentences together with +// no space after 。, and she writes in both languages in one document. +const cjkTerminators = "。！？" + +// closers are swallowed into the sentence they close, so the quote mark travels +// with the sentence rather than opening the next one. +const closers = `)]}"'’”」』` + +// splitChunks divides text into sentences, dropping whitespace-only runs. +// Newlines always break a chunk, so a list or a line of dialogue is its own unit. +// +// `salt` distinguishes two *readings* of the same sentence. The grammar +// checkpoint's advice depends on the document's tone — the same line gets +// different notes as an academic essay than as a journal entry — so switching +// tone must re-open every sentence rather than serve back advice written for the +// old register. +func splitChunks(text, salt string) []chunk { + var out []chunk + runes := []rune(text) + start := 0 + add := func(end int) { + if s := string(runes[start:end]); strings.TrimSpace(s) != "" { + out = append(out, chunk{text: s, hash: hashChunk(s, salt)}) + } + start = end + } + + for i := 0; i < len(runes); i++ { + r := runes[i] + if r == '\n' { + add(i + 1) + continue + } + cjk := strings.ContainsRune(cjkTerminators, r) + if !cjk && !strings.ContainsRune(asciiTerminators, r) { + continue + } + // Swallow a run of terminators ("?!", "…") and any closing punctuation. + j := i + 1 + for j < len(runes) && (strings.ContainsRune(asciiTerminators+cjkTerminators+closers, runes[j])) { + j++ + } + if cjk || j >= len(runes) || unicode.IsSpace(runes[j]) { + add(j) + i = j - 1 + } + } + if start < len(runes) { + add(len(runes)) + } + return out +} + +// hashChunk identifies a sentence by its content under the same normalization +// the suppression logic uses: quote style and whitespace runs churn constantly +// (the editor rewrites quotes as she types, a paragraph reflows) and none of +// that changes what the sentence says, so none of it should cost a re-check. +func hashChunk(s, salt string) string { + sum := sha256.Sum256([]byte(salt + "\x00" + normalizeForDedup(s))) + return hex.EncodeToString(sum[:])[:16] +} + +// hashSet indexes chunks by hash — "is this sentence in the document?" +func hashSet(chunks []chunk) map[string]bool { + out := make(map[string]bool, len(chunks)) + for _, c := range chunks { + out[c.hash] = true + } + return out +} + +// changedChunks returns the chunks whose hash wasn't in the last checked set, +// in document order and deduplicated — a sentence repeated verbatim is one +// question, not two. +func changedChunks(chunks []chunk, checked map[string]bool) []chunk { + seen := make(map[string]bool, len(chunks)) + var out []chunk + for _, c := range chunks { + if checked[c.hash] || seen[c.hash] { + continue + } + seen[c.hash] = true + out = append(out, c) + } + return out +} + +// joinChunks renders a chunk set as the text to hand the model: one sentence per +// line, so two sentences pulled from opposite ends of the document don't read as +// one run-on. +func joinChunks(chunks []chunk) string { + parts := make([]string, 0, len(chunks)) + for _, c := range chunks { + parts = append(parts, strings.TrimSpace(c.text)) + } + return strings.Join(parts, "\n") +} + +// chunkFor names the sentence a suggestion belongs to: the first chunk whose +// text contains the flagged span. Returns "" when the span straddles a sentence +// boundary or the model paraphrased what it quoted — such a row is re-examined +// on every pass rather than cached, which is the safe direction. +func chunkFor(original string, chunks []chunk) string { + o := normalizeForDedup(original) + if o == "" { + return "" + } + for _, c := range chunks { + if strings.Contains(normalizeForDedup(c.text), o) { + return c.hash + } + } + return "" +} diff --git a/internal/suggestions/chunk_test.go b/internal/suggestions/chunk_test.go new file mode 100644 index 0000000..2faab5e --- /dev/null +++ b/internal/suggestions/chunk_test.go @@ -0,0 +1,118 @@ +package suggestions + +import ( + "strings" + "testing" +) + +func texts(chunks []chunk) []string { + out := make([]string, 0, len(chunks)) + for _, c := range chunks { + out = append(out, strings.TrimSpace(c.text)) + } + return out +} + +func TestSplitChunks(t *testing.T) { + cases := []struct { + name string + in string + want []string + }{ + { + name: "plain sentences", + in: "I has two apple. She go to market yesterday! Why?", + want: []string{"I has two apple.", "She go to market yesterday!", "Why?"}, + }, + { + // A decimal must not split, or the sentence's identity would churn + // while she types the number. + name: "decimals stay whole", + in: "It costs 3.50 today. Tomorrow, more.", + want: []string{"It costs 3.50 today.", "Tomorrow, more."}, + }, + { + name: "closing quote travels with its sentence", + in: `He said "early," and left. She stayed.`, + want: []string{`He said "early," and left.`, "She stayed."}, + }, + { + // Chinese runs sentences together with no space after 。 — she writes + // in both languages in one document. + name: "cjk terminators split without a space", + in: "我想说这句话。但是不知道用英语怎么说。", + want: []string{"我想说这句话。", "但是不知道用英语怎么说。"}, + }, + { + name: "newlines break chunks", + in: "A list item\nAnother item\n", + want: []string{"A list item", "Another item"}, + }, + { + name: "blank runs are dropped", + in: "\n\n \nOnly this.\n\n", + want: []string{"Only this."}, + }, + { + name: "trailing fragment is its own chunk", + in: "Done. Still writing", + want: []string{"Done.", "Still writing"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := texts(splitChunks(tc.in, "")) + if len(got) != len(tc.want) { + t.Fatalf("want %q, got %q", tc.want, got) + } + for i := range got { + if got[i] != tc.want[i] { + t.Fatalf("chunk %d: want %q, got %q", i, tc.want[i], got[i]) + } + } + }) + } +} + +// A sentence's identity survives the churn that doesn't change what it says: +// the editor rewrites quotes as she types, and a paragraph reflows. +func TestChunkIdentityIgnoresCosmeticChurn(t *testing.T) { + a := splitChunks(`She said "hello" softly.`, "") + b := splitChunks("She said “hello” softly.", "") + if len(a) != 1 || len(b) != 1 { + t.Fatalf("want one chunk each, got %d and %d", len(a), len(b)) + } + if a[0].hash != b[0].hash { + t.Fatalf("quote/whitespace churn changed the sentence's identity") + } + if same := splitChunks(`She said "hello" softly.`, "academic"); same[0].hash == a[0].hash { + t.Fatalf("a different tone must be a different reading of the sentence") + } +} + +func TestChangedChunksAndLookup(t *testing.T) { + chunks := splitChunks("One thing. Another thing. One thing.", "") + if len(chunks) != 3 { + t.Fatalf("want 3 chunks, got %d", len(chunks)) + } + + // A repeated sentence is one question, not two. + if got := changedChunks(chunks, nil); len(got) != 2 { + t.Fatalf("want 2 distinct changed chunks, got %d", len(got)) + } + + checked := hashSet(chunks[:1]) + changed := changedChunks(chunks, checked) + if len(changed) != 1 || strings.TrimSpace(changed[0].text) != "Another thing." { + t.Fatalf("want only the unread sentence, got %q", texts(changed)) + } + + if chunkFor("Another", chunks) != chunks[1].hash { + t.Fatalf("span was attributed to the wrong sentence") + } + // A span the document doesn't contain has no sentence, so it is never cached. + if chunkFor("nowhere in here", chunks) != "" { + t.Fatalf("unanchorable span should have no chunk") + } +} diff --git a/internal/suggestions/handlers.go b/internal/suggestions/handlers.go index bf1dfaa..fb9a1cc 100644 --- a/internal/suggestions/handlers.go +++ b/internal/suggestions/handlers.go @@ -160,16 +160,21 @@ func (h *Handler) mechanics(w http.ResponseWriter, r *http.Request) { httputil.WriteJSON(w, http.StatusOK, out) } -// replaceMechanics swaps the document's pending offline rows for the supplied -// findings in one transaction, leaving the LLM families and actioned rows -// untouched. Findings the user already accepted or dismissed are suppressed (the -// detector has no memory between runs), and malformed spans are skipped. +// replaceMechanics brings the document's pending offline rows in line with the +// supplied findings in one transaction, leaving the LLM families and actioned +// rows untouched. Findings the user already accepted or dismissed are suppressed +// (the detector has no memory between runs), and malformed spans are skipped. // -// The DELETE is scoped by *source*, not by type: the rule pack owns both the -// mechanics family and its share of the collocation family, and every run is a -// full recompute of the document, so everything it wrote last time goes. Scoping -// by type instead would strand offline collocations the current text no longer -// warrants — the one row nobody would ever replace. +// A finding the detector still reports keeps its existing row — same id, same +// created_at — and only its offsets move. This pass fires 250 ms after a +// keystroke, so deleting and re-inserting the family would hand every card a new +// identity several times a sentence: the rail would remount, a card expanded for +// Ask Petal would collapse under her, and the arrival chime would re-fire. +// +// The scope is *source*, not type: the rule pack owns both the mechanics family +// and its share of the collocation family, and every run is a full recompute of +// the document. Scoping by type instead would strand offline collocations the +// current text no longer warrants — the one row nobody would ever replace. func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) error { tx, err := h.DB.Begin() if err != nil { @@ -177,18 +182,18 @@ func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) er } defer tx.Rollback() - if _, err := tx.Exec( - `DELETE FROM suggestions WHERE doc_id = ? AND status = ? AND source = ?`, - docID, db.SuggestionStatusPending, db.SuggestionSourceLocal, - ); err != nil { + existing, err := loadPending(tx, docID, "source = '"+db.SuggestionSourceLocal+"'") + if err != nil { return err } + index := indexByEdit(existing) sup, err := buildSuppressor(tx, docID) if err != nil { return err } + kept := make(map[string]bool, len(existing)) for _, f := range findings { if f.From < 0 || f.To <= f.From || strings.TrimSpace(f.Original) == "" { continue // malformed span — the client re-anchors by string anyway @@ -196,16 +201,34 @@ func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) er if sup.suppressed(f.Original, f.Replacement) { continue } + typ := localType(f.Type) + if row, ok := index.take(f.Original, f.Replacement, f.From); ok { + kept[row.id] = true + if err := reposition(tx, row, f.From, f.To, ""); err != nil { + return err + } + continue + } if _, err := tx.Exec( `INSERT INTO suggestions (doc_id, from_pos, to_pos, original, replacement, explanation, type, source) VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, docID, f.From, f.To, f.Original, f.Replacement, f.Explanation, - localType(f.Type), db.SuggestionSourceLocal, + typ, db.SuggestionSourceLocal, ); err != nil { return err } } + // Whatever the detector no longer reports, she has fixed. + for _, row := range existing { + if kept[row.id] { + continue + } + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, row.id); err != nil { + return err + } + } + return tx.Commit() } @@ -258,12 +281,65 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R return } - // Nothing to analyze on an empty document — skip the LLM round-trip. + // Nothing to analyze on an empty document — skip the LLM round-trip. The + // family's rows go with the text they were about. if strings.TrimSpace(contentText) == "" { - httputil.WriteJSON(w, http.StatusOK, []db.Suggestion{}) + if err := h.reconcilePending(docID, contentText, nil, scope, nil, nil, false); err != nil { + httputil.ServerError(w, err) + return + } + out, err := h.fetchPending(userID, docID) + if err != nil { + httputil.ServerError(w, err) + return + } + httputil.WriteJSON(w, http.StatusOK, out) return } + // Decide what to ask about before spending anything: a chunked pass asks only + // about the sentences that changed since it last read the document, and when + // none did it doesn't call the model at all — nor consume its rate-limit slot, + // so the next real edit isn't throttled by a check that had nothing to do. + // + // Only a chunked pass consults that record, so only it needs the tone folded + // into a sentence's identity. + salt := "" + if scope.chunked { + salt = tone + } + chunks := splitChunks(contentText, salt) + askText, fresh := contentText, chunks + if scope.chunked { + checked, err := h.checkedChunks(docID, scope.family) + if err != nil { + httputil.ServerError(w, err) + return + } + changed := changedChunks(chunks, checked) + if len(changed) == 0 { + // Every sentence has already been read. Drop the rows whose sentence is + // gone, keep the rest exactly as they are, and answer immediately. + if err := h.reconcilePending(docID, contentText, nil, scope, chunks, nil, false); err != nil { + httputil.ServerError(w, err) + return + } + out, err := h.fetchPending(userID, docID) + if err != nil { + httputil.ServerError(w, err) + return + } + httputil.WriteJSON(w, http.StatusOK, out) + return + } + // When every sentence is new — a first pass, a paste, a tone switch — hand + // over the document verbatim so the model reads it with its paragraphing + // intact. Otherwise send just the delta, one sentence per line. + if len(changed) < len(hashSet(chunks)) { + askText, fresh = joinChunks(changed), changed + } + } + ok, _, slotAt := limiter.Allow(docID) if !ok { // Throttled: return the existing pending set unchanged rather than an @@ -277,7 +353,7 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R return } - raw, err := run(r.Context(), h.Client, contentText, tone, llm.LangFor(pairLang)) + raw, err := run(r.Context(), h.Client, askText, tone, llm.LangFor(pairLang)) if err != nil { // Allow ran before the model call, so a failed pass would otherwise hold // the per-document slot for the full interval — stranding the frontend's @@ -287,7 +363,9 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R return } - if err := h.replacePending(docID, contentText, raw, scope); err != nil { + // A whole-document pass re-read everything, so every one of its rows is up for + // re-proposal; a chunked pass only puts the sentences it asked about in play. + if err := h.reconcilePending(docID, contentText, raw, scope, chunks, fresh, !scope.chunked); err != nil { httputil.ServerError(w, err) return } @@ -308,78 +386,49 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R // inserts. The grammar checkpoint and voice pass each own a disjoint family, so // running one never disturbs the other's pending flags. type pendingScope struct { - deleteWhere string // extra WHERE clause scoping the DELETE to this family + deleteWhere string // extra WHERE clause scoping this pass to its own family forceType string // if set, every inserted row gets this type; else normalizeType + // family keys the sentences this pass has already read (see checked_chunks). + family string + // chunked passes re-read only the sentences that changed. True for the typing- + // cadence grammar checkpoint, which fires constantly and must feel still; + // false for the explicit whole-document passes, where she pressed a button + // asking for a fresh read of everything. + chunked bool } -// Every scope below is confined to source='llm'. The offline rule pack replaces -// its own rows wholesale on each edit (see replaceMechanics) and its findings +// Every scope below is confined to source='llm'. The offline rule pack owns its +// own rows and recomputes them on each edit (see replaceMechanics); its findings // must survive all three model passes — including the collocation coach, which // now shares the collocation family with it. var ( // grammarScope owns the grammar/phrasing/idiom/clarity flags — everything but // the other self-owned families (voice, collocation), which run on their own // cadence/pass and must survive a grammar checkpoint. Notably the offline pass - // writes its rows in the same /check request just before this DELETE runs, so - // the source clause is also what keeps them alive. - grammarScope = pendingScope{deleteWhere: "source = 'llm' AND type NOT IN ('voice','collocation')", forceType: ""} - // voiceScope owns the model's voice flags only. - voiceScope = pendingScope{deleteWhere: "source = 'llm' AND type = 'voice'", forceType: db.SuggestionTypeVoice} + // writes its rows in the same /check request just before this pass reconciles, + // so the source clause is also what keeps them alive. + grammarScope = pendingScope{ + deleteWhere: "source = 'llm' AND type NOT IN ('voice','collocation')", + family: "grammar", + chunked: true, + } + // voiceScope owns the model's voice flags only. Voice is a property of the + // document as a whole — a sentence isn't inconsistent with itself — so this + // pass always reads everything. + voiceScope = pendingScope{ + deleteWhere: "source = 'llm' AND type = 'voice'", + forceType: db.SuggestionTypeVoice, + family: "voice", + } // collocationScope owns the model's collocation flags only — the rule pack's // share of the same family is left standing. - collocationScope = pendingScope{deleteWhere: "source = 'llm' AND type = 'collocation'", forceType: db.SuggestionTypeCollocation} + collocationScope = pendingScope{ + deleteWhere: "source = 'llm' AND type = 'collocation'", + forceType: db.SuggestionTypeCollocation, + family: "collocation", + } ) -// replacePending swaps a document's pending suggestions within one family for a -// fresh batch in a single transaction. Accepted/rejected suggestions and the -// other family's pending rows are left untouched. -// -// Suggestions touching a sentence the user already settled are suppressed from -// the fresh batch (see suppressor): not just the identical edit re-proposed, but -// reversals and re-polishing of the model's own just-accepted output — the -// "fickle, keeps going back and forth on a few sentences" behavior. The model has -// no memory between passes, so without this it re-opens resolved sentences every -// checkpoint. -func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggestion, scope pendingScope) error { - tx, err := h.DB.Begin() - if err != nil { - return err - } - defer tx.Rollback() - - if _, err := tx.Exec( - `DELETE FROM suggestions WHERE doc_id = ? AND status = ? AND `+scope.deleteWhere, - docID, db.SuggestionStatusPending, - ); err != nil { - return err - } - - sup, err := buildSuppressor(tx, docID) - if err != nil { - return err - } - - for _, s := range raw { - if sup.suppressed(s.Original, s.Replacement) { - continue - } - typ := scope.forceType - if typ == "" { - typ = normalizeType(s.Type) - } - from, to := locate(contentText, s.Original) - if _, err := tx.Exec( - `INSERT INTO suggestions (doc_id, from_pos, to_pos, original, replacement, explanation, type, source) - VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, - docID, from, to, s.Original, s.Replacement, s.Explanation, typ, db.SuggestionSourceLLM, - ); err != nil { - return err - } - } - - return tx.Commit() -} - // dedupQuoteReplacer folds every straight/curly single- and double-quote variant // (and backtick/acute accent) onto one canonical character. The editor and the // model both rewrite quotes between passes — a sentence accepted with "…" comes diff --git a/internal/suggestions/handlers_test.go b/internal/suggestions/handlers_test.go index ca2a289..badb3ab 100644 --- a/internal/suggestions/handlers_test.go +++ b/internal/suggestions/handlers_test.go @@ -20,10 +20,17 @@ import ( type stubClient struct { response string calls int + // The full prompt of the most recent call, so a test can assert which + // sentences a chunked pass actually asked about. + lastPrompt string } -func (s *stubClient) Complete(_ context.Context, _ llm.CompletionRequest) (string, error) { +func (s *stubClient) Complete(_ context.Context, req llm.CompletionRequest) (string, error) { s.calls++ + s.lastPrompt = "" + for _, m := range req.Messages { + s.lastPrompt += m.Content + "\n" + } return s.response, nil } @@ -64,6 +71,17 @@ func newTestServer(t *testing.T, client llm.LLMClient) (http.Handler, string, *H return authed, docID, h } +// setDocText rewrites the seeded document, standing in for the writer editing. +// The grammar checkpoint only asks the model about sentences that changed since +// it last read the document, so a test that wants a second real pass has to +// change something first — as she always has. +func setDocText(t *testing.T, h *Handler, docID, text string) { + t.Helper() + if _, err := h.DB.Exec(`UPDATE documents SET content_text = ? WHERE id = ?`, text, docID); err != nil { + t.Fatalf("update doc text: %v", err) + } +} + func do(t *testing.T, srv http.Handler, method, path, body string) *httptest.ResponseRecorder { t.Helper() var r *http.Request @@ -183,6 +201,7 @@ func TestFickleEditsSuppressed(t *testing.T) { ]}`} srv, docID, h := newTestServer(t, client) h.Limit = llm.NewRateLimiter(0) + setDocText(t, h, docID, `He left "early," because of the rain. The cat always have a calm face.`) rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") var got []db.Suggestion @@ -194,6 +213,10 @@ func TestFickleEditsSuppressed(t *testing.T) { do(t, srv, http.MethodPost, "/suggestions/"+s.ID+"/accept", "") } + // Both edits are now in the document, which is what re-opens those sentences + // for a second reading. + setDocText(t, h, docID, `He left "early," due to the rain. The cat always has a calm face.`) + // Reversal of the first accept (note the " → ' quote churn) and a re-polish of // the second accept must both be dropped; only the unrelated edit survives. client.response = `{"suggestions":[ @@ -314,7 +337,9 @@ func TestCollocationPassCoexists(t *testing.T) { t.Fatalf("collocation response should carry all three families, got %+v", got) } - // A grammar checkpoint must NOT wipe the voice or collocation flags. + // A grammar checkpoint must NOT wipe the voice or collocation flags. She fixes + // the flagged sentence, so its own grammar row goes and nothing replaces it. + setDocText(t, h, docID, "I have two apples.") client.response = `{"suggestions":[]}` rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") if rec.Code != http.StatusOK { diff --git a/internal/suggestions/reconcile.go b/internal/suggestions/reconcile.go new file mode 100644 index 0000000..6048bd8 --- /dev/null +++ b/internal/suggestions/reconcile.go @@ -0,0 +1,293 @@ +package suggestions + +import ( + "database/sql" + + "gitea.parodia.dev/drwily/petal/internal/db" + "gitea.parodia.dev/drwily/petal/internal/llm" +) + +// Reconciliation replaces the old "delete the family, insert the new batch" +// shape of every pass. A suggestion the pass proposes again is the *same* +// suggestion: it keeps its row, and therefore its id, its created_at and — most +// visibly — the explanation it was first given. The model re-words its reasoning +// every time it is asked, so re-inserting meant one unchanged mistake carried +// three different explanations in a single sitting. +// +// The id is what the frontend keys its cards on, so a stable id is also what +// keeps the rail from emptying and refilling, a card from collapsing mid-read, +// and the arrival chime from re-firing for advice she has already seen. + +// pendingRow is the part of an existing pending suggestion reconciliation cares +// about. +type pendingRow struct { + id string + original string + replacement string + chunkHash string + from int +} + +// loadPending reads the pending rows a pass owns. `where` is the pass's own +// scoping clause (by source, and for the model passes by family) — the same +// fragment that used to scope its DELETE. +func loadPending(tx *sql.Tx, docID, where string) ([]pendingRow, error) { + rows, err := tx.Query( + `SELECT id, original, replacement, chunk_hash, from_pos FROM suggestions + WHERE doc_id = ? AND status = ? AND `+where, + docID, db.SuggestionStatusPending, + ) + if err != nil { + return nil, err + } + defer rows.Close() + + var out []pendingRow + for rows.Next() { + var r pendingRow + if err := rows.Scan(&r.id, &r.original, &r.replacement, &r.chunkHash, &r.from); err != nil { + return nil, err + } + out = append(out, r) + } + return out, rows.Err() +} + +// editKey identifies an edit by what it proposes, not where: "this exact change +// to this exact text". Normalized like the suppression comparisons, so the +// editor's quote rewriting and a reflowed paragraph don't read as a new edit. +func editKey(original, replacement string) string { + return normalizeForDedup(original) + "\x00" + normalizeForDedup(replacement) +} + +// editIndex matches freshly proposed edits against the rows already standing. +type editIndex struct { + rows []pendingRow + used []bool + byKey map[string][]int +} + +func indexByEdit(rows []pendingRow) *editIndex { + idx := &editIndex{rows: rows, used: make([]bool, len(rows)), byKey: map[string][]int{}} + for i, r := range rows { + k := editKey(r.original, r.replacement) + idx.byKey[k] = append(idx.byKey[k], i) + } + return idx +} + +// take claims the standing row for this edit, if there is one. When a document +// repeats the same mistake, `near` (the fresh span's start) picks the closest +// standing row, so two identical cards keep their own identities instead of +// trading them whenever the text between them grows. +func (i *editIndex) take(original, replacement string, near int) (pendingRow, bool) { + best, bestDist := -1, 0 + for _, n := range i.byKey[editKey(original, replacement)] { + if i.used[n] { + continue + } + d := i.rows[n].from - near + if d < 0 { + d = -d + } + if best < 0 || d < bestDist { + best, bestDist = n, d + } + } + if best < 0 { + return pendingRow{}, false + } + i.used[best] = true + return i.rows[best], true +} + +// reposition updates the advisory offsets (and the sentence a row belongs to) +// without touching anything the writer can see. The frontend re-anchors by +// string at render time, so these only matter for the local-vs-model span +// arbitration in dedupeSpans. +func reposition(tx *sql.Tx, row pendingRow, from, to int, chunkHash string) error { + if row.from == from && row.chunkHash == chunkHash { + return nil + } + _, err := tx.Exec( + `UPDATE suggestions SET from_pos = ?, to_pos = ?, chunk_hash = ? WHERE id = ?`, + from, to, chunkHash, row.id, + ) + return err +} + +// reconcilePending brings a model pass's family in line with what it just +// proposed, sentence by sentence: +// +// - A row on a sentence this pass didn't ask about is kept untouched — that +// is the whole point of chunking. Only its offsets are refreshed. +// - A row on a sentence that no longer exists in the document is dropped: she +// rewrote or deleted it. +// - A row on a sentence the pass *did* ask about survives only if the model +// proposed the same edit again, in which case it keeps its identity. +// +// `fresh` names the sentences the model was asked about (nil when it wasn't +// called at all). inPlayAll marks the whole-document passes — voice and the +// collocation coach — where every row is up for re-proposal because the model +// just re-read everything. +func (h *Handler) reconcilePending( + docID, contentText string, + raw []llm.RawSuggestion, + scope pendingScope, + chunks, fresh []chunk, + inPlayAll bool, +) error { + tx, err := h.DB.Begin() + if err != nil { + return err + } + defer tx.Rollback() + + existing, err := loadPending(tx, docID, scope.deleteWhere) + if err != nil { + return err + } + + present := hashSet(chunks) + asked := hashSet(fresh) + modelRan := inPlayAll || fresh != nil + + // Sentences to hand back to the model next time, because a row we were + // caching on them turned out to be unanchorable (see below). + reopen := map[string]bool{} + + var inPlay []pendingRow + for _, r := range existing { + switch { + // A row whose sentence we can't name is never cached — it is re-examined + // whenever the model speaks, and left alone when it doesn't. + case inPlayAll, r.chunkHash == "" && modelRan, asked[r.chunkHash]: + inPlay = append(inPlay, r) + case r.chunkHash != "" && !present[r.chunkHash]: + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, r.id); err != nil { + return err + } + default: + // Untouched sentence: keep the card exactly as she last saw it. + from, to := locate(contentText, r.original) + if from < 0 { + // The sentence is unchanged in substance but the quoted span no + // longer matches byte for byte — a quote mark the editor rewrote + // inside it, say. The frontend anchors by that string, so this card + // can't be shown; drop it and let the sentence be read again rather + // than cache advice nobody can see. + reopen[r.chunkHash] = true + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, r.id); err != nil { + return err + } + continue + } + if err := reposition(tx, r, from, to, r.chunkHash); err != nil { + return err + } + } + } + for h := range reopen { + delete(present, h) + } + + sup, err := buildSuppressor(tx, docID) + if err != nil { + return err + } + + index := indexByEdit(inPlay) + kept := make(map[string]bool, len(inPlay)) + for _, s := range raw { + if sup.suppressed(s.Original, s.Replacement) { + continue + } + from, to := locate(contentText, s.Original) + // Attribute the finding to a sentence the model was actually shown before + // falling back to the whole document: a short span ("the the") can occur in + // two sentences, and crediting it to the cached one would drop it as advice + // we already have. + hash := chunkFor(s.Original, fresh) + if hash == "" { + hash = chunkFor(s.Original, chunks) + } + // A sentence we didn't ask about already has whatever advice it deserves. + // The model can't normally quote one — it was only shown the delta — but if + // it wanders there anyway, the cached card stands rather than gaining a + // twin. + if !inPlayAll && hash != "" && present[hash] && !asked[hash] { + continue + } + if row, ok := index.take(s.Original, s.Replacement, from); ok { + kept[row.id] = true + if err := reposition(tx, row, from, to, hash); err != nil { + return err + } + continue + } + typ := scope.forceType + if typ == "" { + typ = normalizeType(s.Type) + } + if _, err := tx.Exec( + `INSERT INTO suggestions (doc_id, from_pos, to_pos, original, replacement, explanation, type, source, chunk_hash) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + docID, from, to, s.Original, s.Replacement, s.Explanation, typ, db.SuggestionSourceLLM, hash, + ); err != nil { + return err + } + } + + // Asked about and not proposed again: the model has changed its mind, or she + // has fixed it. + for _, r := range inPlay { + if kept[r.id] { + continue + } + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, r.id); err != nil { + return err + } + } + + // Record the sentences this family has now read. Every sentence still in the + // document has been read by *some* pass: the ones just asked about now, the + // rest in an earlier round. + if scope.chunked { + if _, err := tx.Exec( + `DELETE FROM checked_chunks WHERE doc_id = ? AND family = ?`, docID, scope.family, + ); err != nil { + return err + } + for h := range present { + if _, err := tx.Exec( + `INSERT INTO checked_chunks (doc_id, family, hash) VALUES (?, ?, ?)`, + docID, scope.family, h, + ); err != nil { + return err + } + } + } + + return tx.Commit() +} + +// checkedChunks loads the sentences a family read on its last pass. +func (h *Handler) checkedChunks(docID, family string) (map[string]bool, error) { + rows, err := h.DB.Query( + `SELECT hash FROM checked_chunks WHERE doc_id = ? AND family = ?`, docID, family, + ) + if err != nil { + return nil, err + } + defer rows.Close() + + out := map[string]bool{} + for rows.Next() { + var hash string + if err := rows.Scan(&hash); err != nil { + return nil, err + } + out[hash] = true + } + return out, rows.Err() +} diff --git a/internal/suggestions/stability_test.go b/internal/suggestions/stability_test.go new file mode 100644 index 0000000..20ffa90 --- /dev/null +++ b/internal/suggestions/stability_test.go @@ -0,0 +1,191 @@ +package suggestions + +import ( + "encoding/json" + "net/http" + "strings" + "testing" + + "gitea.parodia.dev/drwily/petal/internal/db" + "gitea.parodia.dev/drwily/petal/internal/llm" +) + +// byOriginal indexes a pending set by the text each card flags. +func byOriginal(in []db.Suggestion) map[string]db.Suggestion { + out := map[string]db.Suggestion{} + for _, s := range in { + out[s.Original] = s + } + return out +} + +// TestUntouchedSentencesKeepTheirCards is the heart of the stability work: she +// edits one sentence, and the cards on every other sentence stay exactly as they +// were — same id (so the rail keeps the card instead of remounting it), same +// explanation (the model re-words its reasoning every time it is asked, and one +// unchanged mistake used to carry three different explanations in a sitting). +// The model is only asked about the sentence that changed. +func TestUntouchedSentencesKeepTheirCards(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has two apple","replacement":"I have two apples","explanation":"first wording","type":"grammar"}, + {"original":"She go to market","replacement":"She goes to market","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + setDocText(t, h, docID, "I has two apple. She go to market yesterday.") + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var first []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &first); err != nil { + t.Fatalf("decode: %v", err) + } + if len(first) != 2 { + t.Fatalf("first pass: want 2, got %d: %+v", len(first), first) + } + kept := byOriginal(first)["I has two apple"] + + // She fixes only the second sentence. The model, asked again, re-words its + // reasoning about the first — which it must never get the chance to do. + setDocText(t, h, docID, "I has two apple. She goes to market yesterday.") + client.response = `{"suggestions":[ + {"original":"I has two apple","replacement":"I have two apples","explanation":"REWORDED","type":"grammar"} + ]}` + rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var second []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &second); err != nil { + t.Fatalf("decode: %v", err) + } + + if strings.Contains(client.lastPrompt, "I has two apple") { + t.Fatalf("untouched sentence was sent to the model:\n%s", client.lastPrompt) + } + if !strings.Contains(client.lastPrompt, "She goes to market") { + t.Fatalf("edited sentence was not sent to the model:\n%s", client.lastPrompt) + } + + now := byOriginal(second)["I has two apple"] + if now.ID != kept.ID { + t.Fatalf("card was remounted: id %q became %q", kept.ID, now.ID) + } + if now.Explanation != "first wording" { + t.Fatalf("explanation drifted: %q", now.Explanation) + } + // The fixed sentence's card is gone, and the model's stray re-proposal for the + // cached sentence did not become a second card. + if len(second) != 1 { + t.Fatalf("want exactly one card left, got %d: %+v", len(second), second) + } +} + +// TestUnchangedDocumentSkipsTheModel proves a check with nothing new to read +// costs nothing: no model call, and every card left standing untouched. This is +// the doc-open and tone-less re-check path. +func TestUnchangedDocumentSkipsTheModel(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has","replacement":"I have","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var first []db.Suggestion + _ = json.Unmarshal(rec.Body.Bytes(), &first) + if len(first) != 1 || client.calls != 1 { + t.Fatalf("first pass: %d cards, %d calls", len(first), client.calls) + } + + rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var second []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &second); err != nil { + t.Fatalf("decode: %v", err) + } + if client.calls != 1 { + t.Fatalf("re-checking an unedited document called the model %d times", client.calls) + } + if len(second) != 1 || second[0].ID != first[0].ID { + t.Fatalf("card did not survive an idle re-check: %+v", second) + } +} + +// TestDeletedSentenceDropsItsCard covers the other half of the skip path: she +// removes a flagged sentence outright, so nothing changed that the model could +// be asked about — but its card must still go. +func TestDeletedSentenceDropsItsCard(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has","replacement":"I have","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + + do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + setDocText(t, h, docID, "") + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var got []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + if len(got) != 0 { + t.Fatalf("card outlived its sentence: %+v", got) + } +} + +// TestToneChangeReopensEverySentence: the checkpoint's advice is written for the +// document's tone, so switching from a journal to an academic essay has to +// re-read sentences that haven't changed a character. +func TestToneChangeReopensEverySentence(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has","replacement":"I have","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + + do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + if _, err := h.DB.Exec(`UPDATE documents SET tone = 'academic' WHERE id = ?`, docID); err != nil { + t.Fatalf("set tone: %v", err) + } + do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + + if client.calls != 2 { + t.Fatalf("tone change did not re-read the document: %d model calls", client.calls) + } +} + +// TestMechanicsFindingsKeepTheirRows: the rule pack re-runs 250 ms after every +// keystroke. A finding it still reports must keep its row, or the rail would +// remount several times a sentence — collapsing a card she has open, and +// re-firing the arrival chime for advice she is already reading. +func TestMechanicsFindingsKeepTheirRows(t *testing.T) { + srv, docID, _ := newTestServer(t, &stubClient{response: `{"suggestions":[]}`}) + body := `{"findings":[ + {"from":0,"to":5,"original":"I has","replacement":"I have","explanation":"agreement"}, + {"from":6,"to":15,"original":"two apple","replacement":"two apples","explanation":"plural"} + ]}` + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/mechanics", body) + var first []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &first); err != nil { + t.Fatalf("decode: %v", err) + } + if len(first) != 2 { + t.Fatalf("want 2 rows, got %d", len(first)) + } + + // She types elsewhere: same findings, shifted spans, one of them now fixed. + rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/mechanics", `{"findings":[ + {"from":20,"to":25,"original":"I has","replacement":"I have","explanation":"agreement"} + ]}`) + var second []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &second); err != nil { + t.Fatalf("decode: %v", err) + } + if len(second) != 1 { + t.Fatalf("want 1 row, got %d: %+v", len(second), second) + } + if second[0].ID != byOriginal(first)["I has"].ID { + t.Fatalf("surviving finding was given a new identity: %+v", second[0]) + } + if second[0].FromPos != 20 { + t.Fatalf("span did not follow the text: %+v", second[0]) + } +} diff --git a/web/src/App.tsx b/web/src/App.tsx index ff2b504..bc3fa11 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -1,7 +1,7 @@ import { useCallback, useEffect, useRef, useState } from 'react' import { api, type DocSummary, type DocUpdate, type Document, type Suggestion, type Tag, type TagColor } from './api/client' import { useAutoSave } from './hooks/useAutoSave' -import { useCheckpoint } from './hooks/useCheckpoint' +import { findingKey, useCheckpoint } from './hooks/useCheckpoint' import { useSpellChecker } from './hooks/useSpellChecker' import { useTags } from './hooks/useTags' import { DocList } from './components/DocList/DocList' @@ -90,6 +90,7 @@ export default function App() { runVoice, runCollocation, removeSuggestion, + resolveServerId, } = useCheckpoint(currentDoc?.id ?? null) // Browser-side spell checker — loads the en-US dictionary once per session. const { checker: spellChecker, addWord } = useSpellChecker() @@ -350,17 +351,20 @@ export default function App() { // Accept applies the replacement in the editor (handled in EditorCore) and // marks the suggestion accepted; dismiss just rejects it. Both drop it locally. + // A rule-pack card can be accepted before its row exists — the edit has already + // landed either way, so a missing id just means there's nothing to file. const handleAccept = useCallback( async (s: Suggestion) => { removeSuggestion(s.id) setAcceptTick((n) => n + 1) try { - await api.acceptSuggestion(s.id) + const id = await resolveServerId(s) + if (id) await api.acceptSuggestion(id) } catch (err) { console.error('accept failed', err) } }, - [removeSuggestion], + [removeSuggestion, resolveServerId], ) // After restoring a version, swap the restored doc into the editor. Bumping @@ -398,12 +402,13 @@ export default function App() { async (s: Suggestion) => { removeSuggestion(s.id) try { - await api.dismissSuggestion(s.id) + const id = await resolveServerId(s) + if (id) await api.dismissSuggestion(id) } catch (err) { console.error('dismiss failed', err) } }, - [removeSuggestion], + [removeSuggestion, resolveServerId], ) // Play a soft sound when freshly-checked suggestions arrive — one per distinct @@ -411,10 +416,14 @@ export default function App() { // a pile-up. We track which ids we've already chimed for, and only chime for // recently-created suggestions so opening a doc with old pending advice stays // silent (the existing set was created in a past session). + // Rule-pack findings are chimed by their wording, not their id: the same fix + // appears first as a provisional card and then as its persisted row, and the + // writer should hear it once. const chimedRef = useRef>(new Set()) useEffect(() => { - const fresh = suggestions.filter((s) => !chimedRef.current.has(s.id)) - fresh.forEach((s) => chimedRef.current.add(s.id)) + const key = (s: Suggestion) => (s.source === 'local' ? `local:${findingKey(s)}` : s.id) + const fresh = suggestions.filter((s) => !chimedRef.current.has(key(s))) + fresh.forEach((s) => chimedRef.current.add(key(s))) const justMade = fresh.filter( (s) => Date.now() - new Date(s.created_at).getTime() < 12_000, ) @@ -509,7 +518,10 @@ export default function App() { <>
{/* Title, then the three chrome pills. Their labels are diff --git a/web/src/components/Editor/EditorCore.tsx b/web/src/components/Editor/EditorCore.tsx index 3e6fef5..0a5e317 100644 --- a/web/src/components/Editor/EditorCore.tsx +++ b/web/src/components/Editor/EditorCore.tsx @@ -15,7 +15,7 @@ import TableHeader from '@tiptap/extension-table-header' import TableCell from '@tiptap/extension-table-cell' import { FontSize } from './FontSize' import type { EditorView } from '@tiptap/pm/view' -import { useCallback, useEffect, useRef, useState } from 'react' +import { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react' import { Toolbar } from '../Toolbar/Toolbar' import { SuggestionCard } from './SuggestionCard' import { SuggestionRail, type RailItem } from './SuggestionRail' @@ -34,6 +34,10 @@ import { speak, speechSupported } from '../../audio/speech' import type { SpellChecker } from '../../hooks/useSpellChecker' import { usePack } from '../../i18n' +// Breathing room left below the last suggestion card when the rail's stack is what +// defines the column's height, so the bottom card doesn't sit flush on the edge. +const RAIL_TAIL = 24 + export interface EditorChange { content: string // Tiptap JSON, stringified content_text: string // flattened plain text for the LLM @@ -229,6 +233,9 @@ export function EditorCore({ // own name, so it says "português" rather than "pt-PT". const pack = usePack() const wrapperRef = useRef(null) + // The text column itself, measured separately from its wrapper: the wrapper is + // grown to cover the card stack, so only this reports the height of the prose. + const contentRef = useRef(null) const [hover, setHover] = useState(null) // The open spelling popover (click a red-underlined word), or null. const [misspell, setMisspell] = useState(null) @@ -266,6 +273,15 @@ export function EditorCore({ // `activeId` is the suggestion currently emphasized (hovered text or card). const [railItems, setRailItems] = useState([]) const [railEnabled, setRailEnabled] = useState(false) + // How far the resolved card stack reaches below the wrapper's top, reported by + // the rail. Cards are absolutely positioned and so contribute no layout height: + // without this the column below the last line of text isn't scrollable and any + // card that lands there is unreachable, not merely far from its sentence. + const [railExtent, setRailExtent] = useState(0) + // Sticky offset for the text column, or null when it should sit in normal flow. + // Set only while the stack overhangs the text: scrolling down to reach the lower + // cards would otherwise carry every sentence off the top of the screen. + const [stickTop, setStickTop] = useState(null) const [railExpandedId, setRailExpandedId] = useState(null) const [activeId, setActiveId] = useState(null) // A stable handle to the latest recompute so the editor's onUpdate (captured @@ -444,6 +460,42 @@ export function EditorCore({ } }, [recomputeRail]) + // The rail only reports its extent while it's mounted, so clear it when the last + // card goes (accepted the lot, or the window narrowed past the rail's threshold) + // — otherwise the column keeps the height of a stack that no longer exists. + useEffect(() => { + if (!railEnabled || railItems.length === 0) setRailExtent(0) + }, [railEnabled, railItems.length]) + + // Decide whether the text column has to be pinned. The rail's cards hang off an + // absolutely-positioned column, so when several suggestions share one short + // paragraph the stack runs far past the last line of text. Growing the wrapper to + // `railExtent` makes that space scrollable (item 4: the lower cards were simply + // unreachable); pinning the prose inside it means scrolling down to read those + // cards keeps the sentences on screen instead of scrolling them away. + // + // The offset is `min(0, port - content)`: prose shorter than the viewport sticks + // at the top, taller prose sticks by its *bottom* edge, so its last lines — the + // ones the overhanging cards flag — stay visible rather than the first. + useLayoutEffect(() => { + const wrapper = wrapperRef.current + const content = contentRef.current + if (!wrapper || !content || !railEnabled || railExtent <= 0) { + setStickTop(null) + return + } + const contentH = content.offsetHeight + // Only pin when the stack actually overhangs the prose; a rail that fits + // beside its text needs nothing, and pinning it would be a change for free. + if (railExtent <= contentH) { + setStickTop(null) + return + } + const port = wrapper.closest('.petal-scrollport') + const portH = port ? port.clientHeight : window.innerHeight + setStickTop(Math.min(0, portH - contentH - RAIL_TAIL)) + }, [railEnabled, railExtent, railItems]) + // Emphasize the flagged text for the active suggestion, mirroring the rail // card ↔ text link both ways. Driven through the decoration plugin (not an // imperative DOM class) so it survives the repaints that fire on every edit. @@ -1082,6 +1134,10 @@ export function EditorCore({
0 ? { minHeight: railExtent + RAIL_TAIL } : undefined} onMouseOver={handleMouseOver} onMouseOut={handleMouseOut} onMouseMove={handleMouseMove} @@ -1093,7 +1149,17 @@ export function EditorCore({ onTouchMove={cancelLongPress} onTouchEnd={cancelLongPress} > - + {/* The prose sits in its own box so it can be measured (and pinned) + independently of the wrapper, which the rail may have grown. The box is + deliberately left at its natural height: sized to the wrapper it would + report the stack's height back as the text's own, and the pin below + could never trip. */} +
+ +
{findOpen && editor && setFindOpen(false)} />} {confetti && } {gloss && ( @@ -1165,6 +1231,7 @@ export function EditorCore({ onHover={setActiveId} onActivate={activateRailCard} onToggleExpand={toggleRailExpand} + onExtent={setRailExtent} /> )}
diff --git a/web/src/components/Editor/SuggestionRail.tsx b/web/src/components/Editor/SuggestionRail.tsx index 3295edd..1b970a5 100644 --- a/web/src/components/Editor/SuggestionRail.tsx +++ b/web/src/components/Editor/SuggestionRail.tsx @@ -28,6 +28,12 @@ interface Props { // A card's body was clicked — scroll its highlight into view and toggle expand. onActivate: (id: string) => void onToggleExpand: (id: string) => void + // How far down the resolved stack reaches (px below the wrapper's top). Cards + // are absolutely positioned, so they add nothing to layout height — a cluster of + // errors in one short paragraph can pile cards hundreds of px past the end of the + // text, with no scrollable space to reach them. The editor uses this to grow the + // column so every card can at least be scrolled to. + onExtent: (bottom: number) => void } // SuggestionRail is the right-margin "comment column": every outstanding @@ -44,6 +50,7 @@ export function SuggestionRail({ onHover, onActivate, onToggleExpand, + onExtent, }: Props) { // Measured resolved tops keyed by suggestion id (after collision avoidance). const [tops, setTops] = useState>({}) @@ -65,13 +72,16 @@ export function SuggestionRail({ const layoutKey = ordered.map((i) => `${i.suggestion.id}:${Math.round(i.anchorTop)}`).join('|') useLayoutEffect(() => { let cursor = -Infinity + let bottom = 0 const next: Record = {} for (const { suggestion, anchorTop } of ordered) { const h = cardRefs.current.get(suggestion.id)?.offsetHeight ?? 96 const top = Math.max(anchorTop, cursor) next[suggestion.id] = top cursor = top + h + CARD_GAP + bottom = top + h } + onExtent(bottom) setTops((prev) => { const ids = Object.keys(next) if (ids.length === Object.keys(prev).length && ids.every((id) => prev[id] === next[id])) return prev diff --git a/web/src/hooks/useCheckpoint.ts b/web/src/hooks/useCheckpoint.ts index a0a8f1f..b4b578a 100644 Binary files a/web/src/hooks/useCheckpoint.ts and b/web/src/hooks/useCheckpoint.ts differ