diff --git a/internal/suggestions/handlers.go b/internal/suggestions/handlers.go index 0a060a5..c1a6a33 100644 --- a/internal/suggestions/handlers.go +++ b/internal/suggestions/handlers.go @@ -154,6 +154,14 @@ var ( // 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 the user already accepted or dismissed are suppressed from the +// fresh batch: the model has no memory between passes, so without this it would +// re-propose the identical edit on the very next checkpoint — re-nagging a +// sentence the user already resolved. This matters most when an accept silently +// no-ops (the `original` text couldn't be anchored in the editor, so the doc +// never changed): the sentence is unaltered, yet the user shouldn't see the same +// card again. func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggestion, scope pendingScope) error { tx, err := h.DB.Begin() if err != nil { @@ -168,7 +176,15 @@ func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggest return err } + actioned, err := actionedKeys(tx, docID) + if err != nil { + return err + } + for _, s := range raw { + if _, seen := actioned[suggestionKey(s.Original, s.Replacement)]; seen { + continue + } typ := scope.forceType if typ == "" { typ = normalizeType(s.Type) @@ -186,6 +202,39 @@ func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggest return tx.Commit() } +// actionedKeys returns the set of original→replacement keys the user has already +// accepted or rejected for this document, so a fresh checkpoint can skip +// re-proposing them. Keyed on the trimmed original+replacement pair, matching the +// normalization ParseCheckpoint applies, so a genuinely different edit on the same +// sentence is not suppressed. +func actionedKeys(tx *sql.Tx, docID string) (map[string]struct{}, error) { + rows, err := tx.Query( + `SELECT original, replacement FROM suggestions + WHERE doc_id = ? AND status IN (?, ?)`, + docID, db.SuggestionStatusAccepted, db.SuggestionStatusRejected, + ) + if err != nil { + return nil, err + } + defer rows.Close() + + keys := make(map[string]struct{}) + for rows.Next() { + var original, replacement string + if err := rows.Scan(&original, &replacement); err != nil { + return nil, err + } + keys[suggestionKey(original, replacement)] = struct{}{} + } + return keys, rows.Err() +} + +// suggestionKey is the dedup identity for a suggestion: its trimmed original and +// replacement text. Two suggestions with the same key are "the same edit." +func suggestionKey(original, replacement string) string { + return strings.TrimSpace(original) + "\x00" + strings.TrimSpace(replacement) +} + // listForDoc returns the document's current pending suggestions (used when the // editor loads a document, before any new checkpoint fires). func (h *Handler) listForDoc(w http.ResponseWriter, r *http.Request) { diff --git a/internal/suggestions/handlers_test.go b/internal/suggestions/handlers_test.go index 3be21a0..db97bc6 100644 --- a/internal/suggestions/handlers_test.go +++ b/internal/suggestions/handlers_test.go @@ -127,6 +127,44 @@ func TestCheckpointFlow(t *testing.T) { } } +// TestResolvedSuggestionsNotReproposed proves a checkpoint won't re-nag a +// sentence the user already accepted or dismissed: the model returns the same +// raw batch on the next pass (it has no memory), but the resolved edits are +// suppressed. This is the "accept it, then it nags again moments later" bug. +func TestResolvedSuggestionsNotReproposed(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has","replacement":"I have","explanation":"subject-verb agreement","type":"grammar"}, + {"original":"two apple","replacement":"two apples","explanation":"plural noun","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + // Zero the checkpoint floor so the second pass runs instead of being throttled. + h.Limit = llm.NewRateLimiter(0) + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var got []db.Suggestion + _ = json.Unmarshal(rec.Body.Bytes(), &got) + if len(got) != 2 { + t.Fatalf("first pass: want 2, got %d", len(got)) + } + + // Accept one, dismiss the other. + do(t, srv, http.MethodPost, "/suggestions/"+got[0].ID+"/accept", "") + do(t, srv, http.MethodPost, "/suggestions/"+got[1].ID+"/dismiss", "") + + // Second checkpoint returns the identical raw batch — both must be suppressed. + rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + if rec.Code != http.StatusOK { + t.Fatalf("second check: code=%d body=%s", rec.Code, rec.Body) + } + var again []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &again); err != nil { + t.Fatalf("decode: %v", err) + } + if len(again) != 0 { + t.Fatalf("resolved suggestions should not be re-proposed, got %d: %+v", len(again), again) + } +} + // TestVoicePassCoexists proves the voice pass and the grammar checkpoint own // disjoint pending families: running one never wipes the other's flags, and // each endpoint returns the unified pending set. diff --git a/web/src/components/Editor/SuggestionHighlight.test.ts b/web/src/components/Editor/SuggestionHighlight.test.ts new file mode 100644 index 0000000..dec7746 --- /dev/null +++ b/web/src/components/Editor/SuggestionHighlight.test.ts @@ -0,0 +1,54 @@ +import { describe, it, expect } from 'vitest' +import { foldTypography, findRange } from './SuggestionHighlight' +import { Schema, type Node as PMNode } from '@tiptap/pm/model' + +// A minimal doc/paragraph/text schema, enough to exercise findRange's textblock +// walk without pulling in the full editor. +const schema = new Schema({ + nodes: { + doc: { content: 'block+' }, + paragraph: { group: 'block', content: 'inline*', toDOM: () => ['p', 0] }, + text: { group: 'inline' }, + }, +}) + +// Build a single-paragraph doc with the given plaintext. +const para = (text: string): PMNode => schema.node('doc', null, [schema.node('paragraph', null, text ? [schema.text(text)] : [])]) + +describe('foldTypography', () => { + it('folds curly quotes back to straight ASCII', () => { + expect(foldTypography('“He’s here,” she said').folded).toBe('"He\'s here," she said') + }) + + it('folds dashes and ellipsis, keeping a source-index map across length changes', () => { + const { folded, map } = foldTypography('wait—really...') + expect(folded).toBe('wait—really…') + // The em dash is already one glyph; the `...` collapsed 3 source chars to 1, + // so the trailing sentinel still points just past the last source char. + expect(map[map.length - 1]).toBe('wait—really...'.length) + }) + + it('treats `--` and `...` typed-but-unconverted as their canonical glyphs', () => { + expect(foldTypography('a--b...c').folded).toBe('a—b…c') + }) +}) + +describe('findRange typographic anchoring', () => { + it('anchors a model echo with straight quotes onto curly-quote document text', () => { + const doc = para('She said “hello” to me') + const range = findRange(doc, '“hello”'.replace(/[“”]/g, '"')) // model echoed "hello" + expect(range).not.toBeNull() + // The matched span must cover the curly-quoted region in the real document. + expect(doc.textBetween(range!.from - 1, range!.to - 1)).toContain('hello') + }) + + it('anchors an ASCII `--` echo onto an em-dash in the document', () => { + const doc = para('I waited—forever') + const range = findRange(doc, 'waited--forever') + expect(range).not.toBeNull() + }) + + it('still returns null when the text genuinely is not present', () => { + expect(findRange(para('nothing to see'), 'absent phrase')).toBeNull() + }) +}) diff --git a/web/src/components/Editor/SuggestionHighlight.ts b/web/src/components/Editor/SuggestionHighlight.ts index 79bcca1..13d4f4b 100644 --- a/web/src/components/Editor/SuggestionHighlight.ts +++ b/web/src/components/Editor/SuggestionHighlight.ts @@ -42,21 +42,87 @@ export function mapOffset(block: PMNode, blockPos: number, targetOffset: number) return result } +// foldTypography canonicalizes the typographic variants that the editor's +// Typography input rules introduce (curly quotes, em/en dashes, single-glyph +// ellipsis) and the unicode spaces a paste can carry. The model echoes a +// suggestion's `original` from the same text but often normalizes these back to +// plain ASCII — straight quotes, `--`, `...` — so an exact match would miss and +// the suggestion could be neither highlighted nor applied. +// +// It returns the folded string plus `map`, where map[k] is the source index at +// which folded character k begins. A multi-character source run (`...`, `--`) +// folds to one glyph, so lengths differ; map projects a match in folded space +// back onto real document offsets. map has a trailing sentinel (= source length) +// so map[start + folded.length] yields the offset just past the match. +export function foldTypography(text: string): { folded: string; map: number[] } { + let folded = '' + const map: number[] = [] + let i = 0 + while (i < text.length) { + // Multi-character ASCII sequences the Typography rules would have replaced. + if (text.startsWith('...', i)) { + folded += '…' + map.push(i) + i += 3 + continue + } + if (text[i] === '-' && text[i + 1] === '-') { + folded += '—' + map.push(i) + i += 2 + continue + } + folded += foldChar(text[i]) + map.push(i) + i++ + } + map.push(text.length) + return { folded, map } +} + +// foldChar maps a single character onto its canonical form (length-preserving). +function foldChar(c: string): string { + switch (c) { + case '‘': + case '’': + case '‚': + case '‛': + return "'" + case '“': + case '”': + case '„': + case '‟': + return '"' + case '–': // en dash → em dash, the canonical form `--` also folds to + return '—' + case ' ': // non-breaking space + case ' ': // thin space + case ' ': // narrow no-break space + return ' ' + default: + return c + } +} + // findRange locates the first occurrence of `search` within a single textblock // and returns its ProseMirror range, or null if the string isn't present (the -// user may have edited or removed it since the checkpoint ran). Exported so the -// accept flow can resolve the same span to replace. +// user may have edited or removed it since the checkpoint ran). Matching is done +// in canonical typographic space (see foldTypography) so curly-quote/dash/ellipsis +// differences between the model's echo and the live document don't strand the +// anchor. Exported so the accept flow can resolve the same span to replace. export function findRange(doc: PMNode, search: string): { from: number; to: number } | null { - if (!search) return null + const needle = foldTypography(search).folded + if (!needle) return null let result: { from: number; to: number } | null = null doc.descendants((node, pos) => { if (result) return false if (!node.isTextblock) return true // keep descending to the textblock - const idx = node.textContent.indexOf(search) + const { folded, map } = foldTypography(node.textContent) + const idx = folded.indexOf(needle) if (idx >= 0) { result = { - from: mapOffset(node, pos, idx), - to: mapOffset(node, pos, idx + search.length), + from: mapOffset(node, pos, map[idx]), + to: mapOffset(node, pos, map[idx + needle.length]), } } return false // never descend into a textblock's inline children