Suggestions: stop re-nagging resolved edits + fix typographic anchoring
Two fixes for the "accept Petal's change, then it nags about the same sentence moments later" report: - replacePending now suppresses any freshly-generated suggestion whose original->replacement matches one the user already accepted or dismissed for that doc. The model has no memory between passes, so without this it re-proposes the identical edit on the next checkpoint. - findRange anchored suggestions by exact string match, which missed whenever the model echoed an `original` with plain ASCII (straight quotes, --, ...) while the document held the Typography-converted glyphs (curly quotes, em-dash, single-char ellipsis). The miss meant no highlight AND a silent no-op on accept, which then fed the re-nag above. foldTypography canonicalizes those variants (with a source index map for length changes) so matching survives the mismatch. Covered by a server-side regression test (accept+dismiss then re-check returns nothing) and frontend unit tests for the fold and anchoring. Claude-Session: https://claude.ai/code/session_016Yr6jELuRc7hyzYLccQKZd
This commit is contained in:
@@ -154,6 +154,14 @@ var (
|
|||||||
// replacePending swaps a document's pending suggestions within one family for a
|
// replacePending swaps a document's pending suggestions within one family for a
|
||||||
// fresh batch in a single transaction. Accepted/rejected suggestions and the
|
// fresh batch in a single transaction. Accepted/rejected suggestions and the
|
||||||
// other family's pending rows are left untouched.
|
// 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 {
|
func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggestion, scope pendingScope) error {
|
||||||
tx, err := h.DB.Begin()
|
tx, err := h.DB.Begin()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -168,7 +176,15 @@ func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggest
|
|||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
actioned, err := actionedKeys(tx, docID)
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
|
||||||
for _, s := range raw {
|
for _, s := range raw {
|
||||||
|
if _, seen := actioned[suggestionKey(s.Original, s.Replacement)]; seen {
|
||||||
|
continue
|
||||||
|
}
|
||||||
typ := scope.forceType
|
typ := scope.forceType
|
||||||
if typ == "" {
|
if typ == "" {
|
||||||
typ = normalizeType(s.Type)
|
typ = normalizeType(s.Type)
|
||||||
@@ -186,6 +202,39 @@ func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggest
|
|||||||
return tx.Commit()
|
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
|
// listForDoc returns the document's current pending suggestions (used when the
|
||||||
// editor loads a document, before any new checkpoint fires).
|
// editor loads a document, before any new checkpoint fires).
|
||||||
func (h *Handler) listForDoc(w http.ResponseWriter, r *http.Request) {
|
func (h *Handler) listForDoc(w http.ResponseWriter, r *http.Request) {
|
||||||
|
|||||||
@@ -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
|
// TestVoicePassCoexists proves the voice pass and the grammar checkpoint own
|
||||||
// disjoint pending families: running one never wipes the other's flags, and
|
// disjoint pending families: running one never wipes the other's flags, and
|
||||||
// each endpoint returns the unified pending set.
|
// each endpoint returns the unified pending set.
|
||||||
|
|||||||
54
web/src/components/Editor/SuggestionHighlight.test.ts
Normal file
54
web/src/components/Editor/SuggestionHighlight.test.ts
Normal file
@@ -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()
|
||||||
|
})
|
||||||
|
})
|
||||||
@@ -42,21 +42,87 @@ export function mapOffset(block: PMNode, blockPos: number, targetOffset: number)
|
|||||||
return result
|
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
|
// 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
|
// 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
|
// user may have edited or removed it since the checkpoint ran). Matching is done
|
||||||
// accept flow can resolve the same span to replace.
|
// 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 {
|
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
|
let result: { from: number; to: number } | null = null
|
||||||
doc.descendants((node, pos) => {
|
doc.descendants((node, pos) => {
|
||||||
if (result) return false
|
if (result) return false
|
||||||
if (!node.isTextblock) return true // keep descending to the textblock
|
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) {
|
if (idx >= 0) {
|
||||||
result = {
|
result = {
|
||||||
from: mapOffset(node, pos, idx),
|
from: mapOffset(node, pos, map[idx]),
|
||||||
to: mapOffset(node, pos, idx + search.length),
|
to: mapOffset(node, pos, map[idx + needle.length]),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return false // never descend into a textblock's inline children
|
return false // never descend into a textblock's inline children
|
||||||
|
|||||||
Reference in New Issue
Block a user