Merge feat/calm-suggestions: drop no-op suggestions + suppress fickle re-edits

Claude-Session: https://claude.ai/code/session_016Yr6jELuRc7hyzYLccQKZd

internal/llm/checkpoint.go

@@ -89,6 +89,14 @@ func ParseCheckpoint(raw string) ([]RawSuggestion, error) {
 		if s.Original == "" {
 			continue
 		}
+		// Drop no-ops: the model sometimes "flags" a correct sentence and echoes
+		// it verbatim as the replacement, producing a card whose before/after are
+		// identical and whose explanation says it's already fine — a suggestion
+		// that suggests nothing. Awareness-only families (voice) carry an empty
+		// replacement, so this only ever fires on the edit families.
+		if s.Replacement != "" && s.Original == s.Replacement {
+			continue
+		}
 		out = append(out, s)
 	}
 	return out, nil

internal/llm/checkpoint_test.go

@@ -37,6 +37,25 @@ func TestParseCheckpoint(t *testing.T) {
 			raw:     "I could not find any issues!",
 			wantErr: true,
 		},
+		{
+			// A correct sentence echoed verbatim as its own replacement is a card
+			// that suggests nothing — dropped. The second item is a real edit.
+			name: "drops no-op where replacement equals original",
+			raw:  `{"suggestions":[{"original":"It wasn't the most graceful morning.","replacement":"It wasn't the most graceful morning.","explanation":"used correctly here","type":"idiom"},{"original":"teh","replacement":"the","explanation":"typo","type":"grammar"}]}`,
+			want: 1,
+		},
+		{
+			// Whitespace-only difference is still a no-op once both are trimmed.
+			name: "drops no-op after whitespace trim",
+			raw:  `{"suggestions":[{"original":"the cat sat","replacement":"  the cat sat  ","explanation":"already fine","type":"grammar"}]}`,
+			want: 0,
+		},
+		{
+			// Awareness-only voice flags carry an empty replacement and must survive.
+			name: "keeps awareness-only flag with empty replacement",
+			raw:  `{"suggestions":[{"original":"a long run-on passage","replacement":"","explanation":"this drifts from your voice","type":"voice"}]}`,
+			want: 1,
+		},
 		{
 			name: "braces inside string values",
 			raw:  `{"suggestions":[{"original":"use {x}","replacement":"use x","explanation":"drop the braces","type":"clarity"}]}`,

internal/suggestions/handlers.go

@@ -155,7 +155,7 @@ func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) er
 		return err
 	}
 
-	actioned, err := actionedKeys(tx, docID)
+	sup, err := buildSuppressor(tx, docID)
 	if err != nil {
 		return err
 	}
@@ -164,7 +164,7 @@ func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) er
 		if f.From < 0 || f.To <= f.From || strings.TrimSpace(f.Original) == "" {
 			continue // malformed span — the client re-anchors by string anyway
 		}
-		if _, seen := actioned[suggestionKey(f.Original, f.Replacement)]; seen {
+		if sup.suppressed(f.Original, f.Replacement) {
 			continue
 		}
 		if _, err := tx.Exec(
@@ -289,13 +289,12 @@ var (
 // 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.
+// 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 {
@@ -310,13 +309,13 @@ func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggest
 		return err
 	}
 
-	actioned, err := actionedKeys(tx, docID)
+	sup, err := buildSuppressor(tx, docID)
 	if err != nil {
 		return err
 	}
 
 	for _, s := range raw {
-		if _, seen := actioned[suggestionKey(s.Original, s.Replacement)]; seen {
+		if sup.suppressed(s.Original, s.Replacement) {
 			continue
 		}
 		typ := scope.forceType
@@ -336,37 +335,131 @@ 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) {
+// 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
+// back flagged with '…' — so without folding, byte-identical text reads as a
+// different edit and the suppression below misses it. (This normalization is for
+// dedup ONLY; the frontend still anchors on the verbatim `original`.)
+var dedupQuoteReplacer = strings.NewReplacer(
+	"‘", "'", "’", "'", "‚", "'", "‛", "'", // single curly
+	"“", "'", "”", "'", "„", "'", "″", "'", // double curly
+	"\"", "'", "`", "'", "´", "'", // straight double, backtick, acute
+)
+
+// normalizeForDedup canonicalizes a string for suppression comparisons: quotes
+// folded (above) and runs of whitespace collapsed to single spaces (so a reflowed
+// paragraph still matches). Used only to decide what to suppress, never to alter
+// stored or rendered text.
+func normalizeForDedup(s string) string {
+	return strings.Join(strings.Fields(dedupQuoteReplacer.Replace(s)), " ")
+}
+
+// suppressor decides which fresh suggestions to drop because the user has already
+// settled the sentence they touch. The model has no memory between passes, so on
+// every checkpoint it re-examines the current text and proposes edits — including
+// ones that re-open a sentence the user already resolved. Three families of those
+// are suppressed (all compared under normalizeForDedup):
+//
+//   - pairs: the identical edit, re-proposed verbatim (the original "accept it,
+//     then it nags again" case; also covers a silent no-op accept that left the
+//     text unchanged).
+//   - actionedOrig: any edit whose original is a span the user already accepted or
+//     dismissed an edit on — "you already decided about this exact sentence."
+//   - acceptedRepl: any edit whose original is text the user accepted AS a
+//     replacement — i.e. the model re-touching its own just-accepted output, which
+//     is how the reversals and endless re-polishing arise (accept "due to the
+//     rain", next pass proposes changing "due to the rain" back). Guarded to
+//     multi-word spans so word-level fixes aren't swept up as collateral.
+//   - acceptedReplList holds the same accepted replacements for a containment
+//     check: the model evades the exact acceptedRepl match by re-flagging a
+//     *sub-clause* of an accepted sentence (flag "she was…due to the rain" instead
+//     of the whole sentence). When one of the new original / an accepted
+//     replacement contains the other and the shorter side is substantial
+//     (>= minContainWords words), it's the same settled span and is dropped.
+type suppressor struct {
+	pairs            map[string]struct{}
+	actionedOrig     map[string]struct{}
+	acceptedRepl     map[string]struct{}
+	acceptedReplList []string
+}
+
+// minContainWords is the floor for the containment check: the shorter of the two
+// spans must be at least this many words before a substring relationship counts
+// as "the same settled text." High enough that an incidental common phrase ("the
+// rain") can't suppress an unrelated sentence, low enough to catch a re-flagged
+// clause.
+const minContainWords = 4
+
+// suppressed reports whether a fresh suggestion should be dropped as already
+// settled. An empty original is never suppressed here (it can't anchor anyway and
+// is dropped upstream).
+func (s suppressor) suppressed(original, replacement string) bool {
+	o := normalizeForDedup(original)
+	if o == "" {
+		return false
+	}
+	if _, ok := s.pairs[o+"\x00"+normalizeForDedup(replacement)]; ok {
+		return true
+	}
+	if _, ok := s.actionedOrig[o]; ok {
+		return true
+	}
+	if strings.ContainsRune(o, ' ') {
+		if _, ok := s.acceptedRepl[o]; ok {
+			return true
+		}
+	}
+	// Containment: the model re-flagged a sub-clause of (or a window around) an
+	// accepted span. Suppress when one contains the other and the shorter side is
+	// a substantial multi-word run.
+	for _, r := range s.acceptedReplList {
+		shorter, longer := o, r
+		if len(r) < len(o) {
+			shorter, longer = r, o
+		}
+		if len(strings.Fields(shorter)) >= minContainWords && strings.Contains(longer, shorter) {
+			return true
+		}
+	}
+	return false
+}
+
+// buildSuppressor loads the document's accepted/rejected edits and indexes them
+// into the three suppression families described on suppressor.
+func buildSuppressor(tx *sql.Tx, docID string) (suppressor, error) {
 	rows, err := tx.Query(
-		`SELECT original, replacement FROM suggestions
+		`SELECT original, replacement, status FROM suggestions
 		  WHERE doc_id = ? AND status IN (?, ?)`,
 		docID, db.SuggestionStatusAccepted, db.SuggestionStatusRejected,
 	)
 	if err != nil {
-		return nil, err
+		return suppressor{}, 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{}{}
+	s := suppressor{
+		pairs:        make(map[string]struct{}),
+		actionedOrig: make(map[string]struct{}),
+		acceptedRepl: make(map[string]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)
+	for rows.Next() {
+		var original, replacement, status string
+		if err := rows.Scan(&original, &replacement, &status); err != nil {
+			return suppressor{}, err
+		}
+		o := normalizeForDedup(original)
+		r := normalizeForDedup(replacement)
+		s.pairs[o+"\x00"+r] = struct{}{}
+		if o != "" {
+			s.actionedOrig[o] = struct{}{}
+		}
+		if status == db.SuggestionStatusAccepted && r != "" {
+			s.acceptedRepl[r] = struct{}{}
+			s.acceptedReplList = append(s.acceptedReplList, r)
+		}
+	}
+	return s, rows.Err()
 }
 
 // listForDoc returns the document's current pending suggestions (used when the

internal/suggestions/handlers_test.go

@@ -165,6 +165,48 @@ func TestResolvedSuggestionsNotReproposed(t *testing.T) {
 	}
 }
 
+// TestFickleEditsSuppressed proves the suppressor kills the "keeps going back and
+// forth on a few sentences" behavior seen live on the Missing Key doc: a later
+// pass that (a) reverses an edit the user just accepted — even with the editor's
+// double→single quote churn that defeats a byte-exact match — or (b) re-polishes
+// the model's own accepted output. A genuinely new edit on a fresh sentence still
+// survives.
+func TestFickleEditsSuppressed(t *testing.T) {
+	client := &stubClient{response: `{"suggestions":[
+		{"original":"He left \"early,\" because of the rain.","replacement":"He left \"early,\" due to the rain.","explanation":"smoother","type":"phrasing"},
+		{"original":"The cat always have a calm face.","replacement":"The cat always has a calm face.","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 got []db.Suggestion
+	_ = json.Unmarshal(rec.Body.Bytes(), &got)
+	if len(got) != 2 {
+		t.Fatalf("first pass: want 2, got %d", len(got))
+	}
+	for _, s := range got {
+		do(t, srv, http.MethodPost, "/suggestions/"+s.ID+"/accept", "")
+	}
+
+	// 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":[
+		{"original":"He left 'early,' due to the rain.","replacement":"He left 'early,' because of the rain.","explanation":"reverts the accepted edit","type":"phrasing"},
+		{"original":"The cat always has a calm face.","replacement":"The cat always seems to have a calm face.","explanation":"re-polishes accepted output","type":"phrasing"},
+		{"original":"due to the rain","replacement":"because of the rain","explanation":"sub-clause reversal of the accepted span","type":"phrasing"},
+		{"original":"She drived home.","replacement":"She drove home.","explanation":"past tense","type":"grammar"}
+	]}`
+	rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "")
+	var again []db.Suggestion
+	if err := json.Unmarshal(rec.Body.Bytes(), &again); err != nil {
+		t.Fatalf("decode: %v", err)
+	}
+	if len(again) != 1 || again[0].Original != "She drived home." {
+		t.Fatalf("want only the new edit to survive, 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.

web/src/components/Editor/AskPetal.tsx

@@ -34,9 +34,12 @@ export function AskPetal({ suggestionId, explanation }: Props) {
     if (el) el.scrollTop = el.scrollHeight
   }, [messages])
 
-  // Focus the input when the panel opens.
+  // Focus the input when the panel opens. preventScroll: the card is already on
+  // screen as an absolutely-positioned overlay, and a default focus() would make
+  // the browser scroll its ancestor to "reveal" the input — jumping the document
+  // to the top.
   useEffect(() => {
-    inputRef.current?.focus()
+    inputRef.current?.focus({ preventScroll: true })
   }, [])
 
   // Fetch the Chinese translation of the explanation to seed the first bubble.
@@ -95,7 +98,7 @@ export function AskPetal({ suggestionId, explanation }: Props) {
       console.error('Ask Petal chat failed:', err)
     } finally {
       setStreaming(false)
-      inputRef.current?.focus()
+      inputRef.current?.focus({ preventScroll: true })
     }
   }
 

web/src/components/Editor/EditorCore.tsx

@@ -580,9 +580,37 @@ export function EditorCore({
     [onDismiss, closeCard],
   )
 
-  // Click a red-underlined word to open its spelling popover, anchored under the
-  // word. posAtCoords→wordAt resolves the exact PM span (robust to the same
-  // misspelling appearing elsewhere); nspell supplies the corrections.
+  // openMisspellAt resolves the word at a document position and, if the checker
+  // flags it as misspelled, opens the spelling popover anchored under the word.
+  // Returns true if it opened a card. We resolve the word from coordinates and
+  // the checker rather than from the `.petal-misspelling` DOM span: clicking a
+  // word moves the caret into it, which fires the decoration rebuild that
+  // deliberately un-underlines the caret word (see SpellCheck's caret exemption),
+  // so by the time a click/contextmenu handler runs the span is already gone.
+  const openMisspellAt = useCallback(
+    (pos: number): boolean => {
+      if (!editor || !spellChecker) return false
+      const range = wordAt(editor.state.doc, pos)
+      if (!range || spellChecker.correct(range.word)) return false
+      const wrapper = wrapperRef.current
+      if (!wrapper) return false
+      const start = editor.view.coordsAtPos(range.from)
+      const end = editor.view.coordsAtPos(range.to)
+      const wrapRect = wrapper.getBoundingClientRect()
+      const cardWidth = 240
+      const left = Math.max(0, Math.min(start.left - wrapRect.left, wrapper.clientWidth - cardWidth))
+      const top = end.bottom - wrapRect.top + 6
+      // Opening a spelling popover supersedes any AI-suggestion or lookup card.
+      closeCard()
+      setWordInfo(null)
+      setMisspell({ ...range, suggestions: spellChecker.suggest(range.word), top, left })
+      return true
+    },
+    [editor, spellChecker, closeCard],
+  )
+
+  // Click a misspelled word to open its spelling popover, anchored under the
+  // word; nspell supplies the corrections.
   //
   // Tapping an AI-suggestion highlight also opens its card here — on touch there's
   // no hover, so the tap is the only way in (mouse users still get hover).
@@ -603,26 +631,16 @@ export function EditorCore({
         }
         return
       }
-      if (!editor || !spellChecker) return
-      const target = (e.target as HTMLElement).closest('.petal-misspelling') as HTMLElement | null
-      if (!target) return
-      const wrapper = wrapperRef.current
-      if (!wrapper) return
+      if (!editor) return
+      // Only act on clicks in the editor text itself — the floating cards/popovers
+      // are children of this same wrapper, and a click on one shouldn't resolve a
+      // (hidden) word behind it.
+      if (!(e.target as HTMLElement).closest('.petal-prose')) return
       const coords = editor.view.posAtCoords({ left: e.clientX, top: e.clientY })
       if (!coords) return
-      const range = wordAt(editor.state.doc, coords.pos)
-      if (!range) return
-      const elRect = target.getBoundingClientRect()
-      const wrapRect = wrapper.getBoundingClientRect()
-      const cardWidth = 240
-      const left = Math.max(0, Math.min(elRect.left - wrapRect.left, wrapper.clientWidth - cardWidth))
-      const top = elRect.bottom - wrapRect.top + 6
-      // Opening a spelling popover supersedes any AI-suggestion hover card.
-      closeCard()
-      setWordInfo(null)
-      setMisspell({ ...range, suggestions: spellChecker.suggest(range.word), top, left })
+      openMisspellAt(coords.pos)
     },
-    [editor, spellChecker, closeCard, openCardFor, railEnabled],
+    [editor, openMisspellAt, openCardFor, railEnabled],
   )
 
   const replaceMisspelling = useCallback(
@@ -757,8 +775,10 @@ export function EditorCore({
       .catch((err) => console.error('vocab save failed', err))
   }, [wordInfo, exampleAt, docId])
 
-  // Right-click a word to look it up. Right-clicking off any word falls through
-  // to the native menu (so copy/paste-by-menu still works — see the Selection fix).
+  // Right-click a misspelled word for spelling corrections (the familiar "did you
+  // mean" gesture); right-click any other word to look it up. Right-clicking off
+  // any word falls through to the native menu (so copy/paste-by-menu still works
+  // — see the Selection fix).
   const handleContextMenu = useCallback(
     (e: React.MouseEvent) => {
       if (!editor) return
@@ -766,9 +786,11 @@ export function EditorCore({
       if (!coords) return
       if (!wordAt(editor.state.doc, coords.pos)) return
       e.preventDefault()
+      // A misspelled word offers corrections first; otherwise look it up.
+      if (openMisspellAt(coords.pos)) return
       openWordLookup(coords.pos)
     },
-    [editor, openWordLookup],
+    [editor, openMisspellAt, openWordLookup],
   )
 
   // Touch has no hover or right-click, so a long-press (~500ms without moving)