From 1bdd1da800555d2ac284d64fee53b0b8234de2e4 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 11 Sep 2026 20:23:53 -0700 Subject: [PATCH] feat: address the CodeRabbit review on the Edit with AI fix by leaving the caret after text written at a collapsed position instead of in front of it (an insertion maps to itself unless the position associates rightwards, so continuing to type went before the insert), splitting the model's blocks one separator at a time so a blank paragraph the author used as spacing survives a rewrite instead of being swallowed by a greedy newline run, carrying a link destination that holds a paren or a space through in markdown's angle-bracket form rather than dropping the link, putting the author's boundary whitespace back on the model's trimmed answer in both hosts so a selection ending on a space does not glue the rewrite to the next word and "did anything change?" compares exactly what was written, and sizing the completion cap from the passage's own rune count so an 8000-rune CJK body is not truncated by a cap chosen for English --- internal/api/handler/generation_edit.go | 30 ++++++++-- internal/api/handler/generation_edit_test.go | 55 +++++++++++++++-- web/src/components/app/ai/RichTextAIEdit.tsx | 11 +++- web/src/components/app/ai/TextareaAIEdit.tsx | 18 +++--- .../components/app/ai/richTextAIEdit.test.tsx | 29 +++++++++ .../components/app/ai/richTextPassage.test.ts | 33 +++++++++- web/src/components/app/ai/richTextPassage.ts | 60 ++++++++++++++----- 7 files changed, 199 insertions(+), 37 deletions(-) diff --git a/internal/api/handler/generation_edit.go b/internal/api/handler/generation_edit.go index 5576525e..13da9c4d 100644 --- a/internal/api/handler/generation_edit.go +++ b/internal/api/handler/generation_edit.go @@ -37,12 +37,32 @@ const creditsPerEdit = 1 const ( editMaxTextLen = 8000 editMaxInstructionLen = 2000 - // editMaxTokens caps the completion. The passage alone may be editMaxTextLen - // characters, roughly 2k tokens, and an edit returns the whole passage, so - // the writing assistant's 1024 cap truncated a long rewrite mid-sentence. - editMaxTokens = 3072 + // editTokenFloor and editTokenCeiling bound the completion. An edit returns + // the WHOLE passage, so a flat cap truncates: the writing assistant's 1024 + // cut a long rewrite mid-sentence, and any fixed number is wrong for one + // script or the other, since a rune of English is about a third of a token + // and a rune of Chinese is about one. The cap is derived per request from + // the passage instead. + editTokenFloor = 1024 + editTokenHeadroom = 512 + editTokenCeiling = 8192 ) +// editCompletionTokens sizes the completion for the passage being edited: room +// for every rune to come back as its own token, plus headroom for an +// instruction that lengthens the copy. Only what the model actually generates +// is billed, so the ceiling costs nothing until it is used. +func editCompletionTokens(passage string) int { + want := utf8.RuneCountInString(passage) + editTokenHeadroom + if want < editTokenFloor { + return editTokenFloor + } + if want > editTokenCeiling { + return editTokenCeiling + } + return want +} + const ( editFenceBegin = "<<>>" editFenceEnd = "<<>>" @@ -147,7 +167,7 @@ func (h *Handler) GenerateEdit(c *gin.Context) { System: generation.BuildEditRules(voice), Prompt: buildEditPrompt(req), Model: model, - MaxTokens: editMaxTokens, + MaxTokens: editCompletionTokens(req.Text), }) if gerr != nil { if !local { diff --git a/internal/api/handler/generation_edit_test.go b/internal/api/handler/generation_edit_test.go index 7af5f0d5..6892dd7b 100644 --- a/internal/api/handler/generation_edit_test.go +++ b/internal/api/handler/generation_edit_test.go @@ -180,10 +180,8 @@ func TestGenerateEditSendsTheEditPromptAndReturnsTheText(t *testing.T) { if !strings.Contains(provider.got.Prompt, "Instruction: fix the grammar") { t.Errorf("wrong user prompt:\n%s", provider.got.Prompt) } - // An edit returns the whole passage, and the passage may be editMaxTextLen - // characters, so the writing assistant's 1024-token cap would truncate it. - if provider.got.MaxTokens < 2048 { - t.Errorf("max tokens %d truncates a full-body rewrite", provider.got.MaxTokens) + if provider.got.MaxTokens != editTokenFloor { + t.Errorf("a short passage should get the floor, got %d", provider.got.MaxTokens) } var body struct { @@ -200,3 +198,52 @@ func TestGenerateEditSendsTheEditPromptAndReturnsTheText(t *testing.T) { t.Errorf("unexpected response: %+v", body) } } + +// An edit returns the WHOLE passage, so the completion cap has to cover it. A +// flat cap is wrong for one script or the other: 8000 runes of English is about +// 2k tokens and 8000 runes of Chinese is about 8k. +func TestEditCompletionTokensCoversThePassage(t *testing.T) { + if got := editCompletionTokens("short"); got != editTokenFloor { + t.Errorf("short passage: got %d, want the %d floor", got, editTokenFloor) + } + // A full-length CJK passage: every rune may come back as its own token. + long := strings.Repeat("文", editMaxTextLen) + if got := editCompletionTokens(long); got < editMaxTextLen { + t.Errorf("a %d-rune passage would be truncated at %d tokens", editMaxTextLen, got) + } + if got := editCompletionTokens(long); got > editTokenCeiling { + t.Errorf("got %d, above the %d ceiling", got, editTokenCeiling) + } + // Between the two, the cap tracks the passage plus headroom. + mid := strings.Repeat("a", 3000) + if got := editCompletionTokens(mid); got != 3000+editTokenHeadroom { + t.Errorf("got %d, want %d", got, 3000+editTokenHeadroom) + } +} + +// The limits count characters, not bytes: the same body must be editable in +// every language, and a byte cap is three times stricter for Cyrillic or CJK. +func TestEditLimitsCountRunes(t *testing.T) { + gin.SetMode(gin.TestMode) + provider := &editProvider{} + h := &Handler{FeatureGateService: editGate{}, CreditService: editCredits{}, AIProvider: provider} + + // 5000 runes of Cyrillic is 10000 bytes: under the rune cap, over a byte one. + passage := strings.Repeat("ф", 5000) + body, err := json.Marshal(map[string]string{"text": passage, "instruction": "shorten"}) + if err != nil { + t.Fatal(err) + } + + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Set(middleware.OrganizationIDKey, uuid.New()) + c.Request = httptest.NewRequest(http.MethodPost, "/generation/edit", strings.NewReader(string(body))) + c.Request.Header.Set("Content-Type", "application/json") + + h.GenerateEdit(c) + + if rec.Code != http.StatusOK { + t.Fatalf("a 5000-rune Cyrillic passage was refused: %d %s", rec.Code, rec.Body.String()) + } +} diff --git a/web/src/components/app/ai/RichTextAIEdit.tsx b/web/src/components/app/ai/RichTextAIEdit.tsx index 81b5ab23..9f364ef2 100644 --- a/web/src/components/app/ai/RichTextAIEdit.tsx +++ b/web/src/components/app/ai/RichTextAIEdit.tsx @@ -20,7 +20,14 @@ import type { AppError } from "@/lib/api/client/normalizeError"; import buildError from "@/lib/helper/buildError"; import AIEditPopover, { type AIEditPhase } from "./AIEditPopover"; import { AI_CARD_WIDTH, clampCardLeft } from "./floatingBounds"; -import { clampContext, passageAIConfigs, passageHTML, passageText, replacePassage } from "./richTextPassage"; +import { + clampContext, + passageAIConfigs, + passageHTML, + passageText, + replacePassage, + restoreEdges, +} from "./richTextPassage"; interface EditorRange { from: number; @@ -154,7 +161,7 @@ export default function RichTextAIEdit({ editor }: { editor: Editor }) { tokens: number, ) => { setUsage({ charged, tokens }); - const html = passageHTML(text, target.aiConfigs); + const html = passageHTML(restoreEdges(target.text, text), target.aiConfigs); const newTo = replacePassage(editor, target.from, target.to, html); editor.commands.focus(); lastRun.current = { instruction, prevHTML, range: target }; diff --git a/web/src/components/app/ai/TextareaAIEdit.tsx b/web/src/components/app/ai/TextareaAIEdit.tsx index f12ad096..3afe4ee4 100644 --- a/web/src/components/app/ai/TextareaAIEdit.tsx +++ b/web/src/components/app/ai/TextareaAIEdit.tsx @@ -19,7 +19,7 @@ import buildError from "@/lib/helper/buildError"; import { Kbd } from "@/components/ui/shortcut-tooltip"; import AIEditPopover, { type AIEditPhase } from "./AIEditPopover"; import { AI_CARD_WIDTH, clampCardLeft } from "./floatingBounds"; -import { clampContext } from "./richTextPassage"; +import { clampContext, restoreEdges } from "./richTextPassage"; import textareaRangeRect, { textareaRangeRects, type LineRect, @@ -217,16 +217,16 @@ export default function TextareaAIEdit({ tokens: number, ) => { setUsage({ charged, tokens }); - // A model that hands the passage back untouched must say so rather - // than report a rewrite nobody can see (issue #432). The server - // trims what it returns, so the comparison trims both sides or a - // selection with a trailing space never matches. - setChanged(text.trim() !== target.text.trim()); + // The server returns its answer trimmed, so the author's own edges + // go back on before anything is written or compared: what lands in + // the box IS what "did it change?" is answered from (issue #432). + const applied = restoreEdges(target.text, text); + setChanged(applied !== target.text); const prefix = prevValue.slice(0, target.start); const suffix = prevValue.slice(target.end); const cap = (s: string) => (maxLen ? s.slice(0, maxLen) : s); typewriter.run( - text, + applied, (partial) => { const next = cap(prefix + partial + suffix); expectedValue.current = next; @@ -240,8 +240,8 @@ export default function TextareaAIEdit({ } }, () => { - const newRange = { start: target.start, end: target.start + text.length, text }; - lastRun.current = { instruction, prevValue, start: target.start, newLen: text.length }; + const newRange = { start: target.start, end: target.start + applied.length, text: applied }; + lastRun.current = { instruction, prevValue, start: target.start, newLen: applied.length }; frozen.current = newRange; const ta = textareaRef.current; if (ta) { diff --git a/web/src/components/app/ai/richTextAIEdit.test.tsx b/web/src/components/app/ai/richTextAIEdit.test.tsx index 871d38b3..e68b3bf9 100644 --- a/web/src/components/app/ai/richTextAIEdit.test.tsx +++ b/web/src/components/app/ai/richTextAIEdit.test.tsx @@ -152,6 +152,15 @@ describe("Edit with AI in the campaign body", () => { expect(saved.html).toBe(editor.getHTML()); }); + it("wraps a destination the plain markdown form cannot hold", async () => { + const url = "https://en.wikipedia.org/wiki/Foo_(bar)"; + const { editor } = mountBody(`

See the page.

`); + reply = { text: "unused", credits_charged: 1, tokens_used: 10 }; + await select(editor, 1, editor.state.doc.content.size - 1); + await rewrite(); + expect(sent?.text).toBe(`See [the page](<${url}>).`); + }); + it("replaces the whole selection and reports the new body upward", async () => { const { editor, saved } = mountBody("

Original one.

Original two.

"); reply = { text: "New one.\n\nNew two.", credits_charged: 1, tokens_used: 10 }; @@ -210,6 +219,15 @@ describe("Edit with AI in the campaign body", () => { expect(screen.queryByText("Rewritten")).toBeNull(); }); + it("gives back the space the selection ended on", async () => { + const { editor } = mountBody("

One two three four.

"); + // "two three " including the trailing space; the server trims its answer. + reply = { text: "TWO AND THREE", credits_charged: 1, tokens_used: 10 }; + await select(editor, 5, 15); + await rewrite(); + expect(editor.getHTML()).toBe("

One TWO AND THREE four.

"); + }); + it("undoes the rewrite back to the body that was there", async () => { const { editor, saved } = mountBody("

Original one.

"); reply = { text: "New one.", credits_charged: 1, tokens_used: 10 }; @@ -252,6 +270,17 @@ describe("Write with AI at the caret", () => { expect(saved.html).toBe(editor.getHTML()); }); + it("leaves the caret after what it wrote, not in front of it", async () => { + const { editor } = mountBody("

One four.

"); + written = { text: "two three ", credits_charged: 1, tokens_used: 10 }; + await writeAtCaret(editor, 5, "add the middle"); + expect(editor.getHTML()).toBe("

One two three four.

"); + // Continuing to type has to continue the sentence, which it cannot do + // from a caret parked in front of the insertion. + expect(editor.state.selection.empty).toBe(true); + expect(editor.state.selection.from).toBe(15); + }); + it("lands the merge variable it was told to use as a chip", async () => { const { editor } = mountBody("

Hello.

"); written = { text: "Hi {{.FirstName}}.", credits_charged: 1, tokens_used: 10 }; diff --git a/web/src/components/app/ai/richTextPassage.test.ts b/web/src/components/app/ai/richTextPassage.test.ts index 754a4725..492ca212 100644 --- a/web/src/components/app/ai/richTextPassage.test.ts +++ b/web/src/components/app/ai/richTextPassage.test.ts @@ -3,7 +3,7 @@ // treated as markup. import { describe, it, expect } from "vitest"; -import { passageHTML } from "./richTextPassage"; +import { passageHTML, restoreEdges } from "./richTextPassage"; describe("passageHTML", () => { it("starts a paragraph on a blank line and breaks on a single newline", () => { @@ -69,4 +69,35 @@ describe("passageHTML", () => { it("keeps an empty line as an empty paragraph", () => { expect(passageHTML("")).toBe("


"); }); + + it("keeps a blank paragraph the author used as spacing", () => { + // Two separators in a row is an empty block between two others; a + // greedy split swallowed it and the spacing disappeared on every edit. + expect(passageHTML("one\n\n\n\ntwo")).toBe("

one


two

"); + }); + + it("drops a stray newline at a block edge rather than rendering a break", () => { + expect(passageHTML("one\n\n\ntwo")).toBe("

one

two

"); + }); + + it("keeps a destination that carries a paren or a space", () => { + expect(passageHTML("read [the article]() now")).toBe( + '

read the article now

', + ); + }); +}); + +describe("restoreEdges", () => { + it("gives back the spaces the model trimmed off", () => { + expect(restoreEdges(" word ", "REWRITE")).toBe(" REWRITE "); + expect(restoreEdges("word", "REWRITE")).toBe("REWRITE"); + }); + + it("makes an unchanged answer compare equal to what was selected", () => { + expect(restoreEdges("Already fine. ", "Already fine.")).toBe("Already fine. "); + }); + + it("leaves an all-whitespace selection alone", () => { + expect(restoreEdges(" ", "anything")).toBe(" "); + }); }); diff --git a/web/src/components/app/ai/richTextPassage.ts b/web/src/components/app/ai/richTextPassage.ts index 3ba64b1b..f3a55bba 100644 --- a/web/src/components/app/ai/richTextPassage.ts +++ b/web/src/components/app/ai/richTextPassage.ts @@ -30,8 +30,10 @@ import { FIELD_TOKEN_RE, FORM_LINK_RE } from "@/lib/templateVars"; const AI_TOKEN_SOURCE = "\\[\\[ai:[A-Za-z0-9_-]{1,64}\\]\\]"; // A markdown link, as passageText writes one. The destination may be a merge -// token ({{.UnsubscribeLink}}), so it is anything without a space or a paren. -const MD_LINK_SOURCE = "\\[[^\\]\\n]*\\]\\([^)\\s]+\\)"; +// token ({{.UnsubscribeLink}}), so it is anything without a space or a paren, +// or anything at all inside angle brackets: a URL is allowed both, and a +// Wikipedia article ending in ")" is the common one. +const MD_LINK_SOURCE = "\\[[^\\]\\n]*\\]\\((?:<[^>\\n]*>|[^)\\s]+)\\)"; const CONDITIONAL_SOURCE = "\\{\\{\\s*if\\s[\\s\\S]*?\\{\\{\\s*end\\s*\\}\\}"; // One scan over the model's text, widest structure first: a conditional wraps @@ -47,7 +49,7 @@ const TOKEN_SOURCE = [ ].join("|"); const AI_TOKEN_RE = /^\[\[ai:([A-Za-z0-9_-]{1,64})\]\]$/; -const MD_LINK_RE = /^\[([^\]\n]*)\]\(([^)\s]+)\)$/; +const MD_LINK_RE = /^\[([^\]\n]*)\]\((?:<([^>\n]*)>|([^)\s]+))\)$/; // Only a destination that is actually a destination becomes an anchor, so a // "[note](see below)" the author wrote stays the text they wrote. const HREF_RE = /^(https?:\/\/|mailto:|tel:|\{\{)/i; @@ -71,10 +73,11 @@ function plainHTML(s: string): string { function chipHTML(token: string, aiConfigs: Map): string { const md = token.match(MD_LINK_RE); if (md) { - if (!HREF_RE.test(md[2])) return plainHTML(token); + const href = md[2] ?? md[3] ?? ""; + if (!HREF_RE.test(href)) return plainHTML(token); // The label is copy like any other, so the tokens inside it are chipped // too. It cannot hold another link: the pattern stops at a "]". - return `${inlineHTML(md[1], aiConfigs)}`; + return `${inlineHTML(md[1], aiConfigs)}`; } const ai = token.match(AI_TOKEN_RE); if (ai) { @@ -114,16 +117,29 @@ export function clampContext(text: string): string { return points.length <= CONTEXT_LIMIT ? text : points.slice(0, CONTEXT_LIMIT).join(""); } +// The model returns its answer trimmed, so an edit of a selection that started +// or ended on a space would eat that space and glue the rewrite to the word +// next to it. The author's edges are put back, which also makes "did anything +// change?" an exact comparison rather than a trimmed one. +export function restoreEdges(original: string, edited: string): string { + const lead = /^\s*/.exec(original)?.[0] ?? ""; + const trail = /\s*$/.exec(original)?.[0] ?? ""; + return original.trim() === "" ? original : lead + edited.trim() + trail; +} + function linkHref(node: PMNode): string { return String(node.marks.find((m) => m.type.name === "link")?.attrs.href ?? ""); } // A link is only written as markdown when the markdown reads back as the same -// link. A "]" in the label or a ")" in the destination would come back as -// literal brackets in the body, which is worse than an unmarked run of words. +// link; anything else would come back as literal brackets in the body, which is +// worse than an unmarked run of words. A destination carrying a ")" or a space +// goes in angle brackets, the markdown form for exactly that. function markdownLink(label: string, href: string): string { - const safe = !label.includes("]") && !label.includes("\n") && !/[)\s]/.test(href); - return safe ? `[${label}](${href})` : label; + if (label.includes("]") || label.includes("\n") || !href) return label; + if (!/[)\s<>]/.test(href)) return `[${label}](${href})`; + if (href.includes(">") || href.includes("\n")) return label; + return `[${label}](<${href}>)`; } // passageText reads a document range as the model should see it: chips as their @@ -186,10 +202,17 @@ export function passageAIConfigs(editor: Editor, from: number, to: number): Map< // passageHTML turns model text back into editor HTML: a blank line starts a // paragraph, a single newline is a line break, and every token becomes its chip. +// +// The split consumes ONE separator per break, the exact inverse of the "\n\n" +// passageText joins blocks with, so an empty paragraph between two others comes +// back as an empty paragraph instead of being swallowed by a greedy \n{2,}. An +// odd newline left over at a block's edge is the model's own spacing noise and +// is dropped rather than rendered as a stray break. export function passageHTML(text: string, aiConfigs?: Map): string { const configs = aiConfigs ?? new Map(); return text - .split(/\n{2,}/) + .split("\n\n") + .map((block) => block.replace(/^\n+|\n+$/g, "")) .map((block) => `

${inlineHTML(block, configs) || "
"}

`) .join(""); } @@ -200,17 +223,22 @@ function parseSlice(editor: Editor, html: string): Slice { }); } -// replacePassage swaps a range for parsed HTML the way a paste would, leaving -// the result selected. Returns the end of the new content, or null when the -// document did not change — a rewrite that changed nothing must not be reported -// as one that did. +// replacePassage swaps a range for parsed HTML the way a paste would. A +// replacement is left selected, for review; an insertion at a caret leaves the +// caret after what was written, so the next keystroke continues it. Returns the +// end of the new content, or null when the document did not change — a rewrite +// that changed nothing must not be reported as one that did. export function replacePassage(editor: Editor, from: number, to: number, html: string): number | null { + const inserting = from === to; const tr = editor.state.tr.replaceRange(from, to, parseSlice(editor, html)); // docChanged only says a step ran: replacing a passage with the same words // still produces one. The documents themselves are compared instead. if (tr.doc.eq(editor.state.doc)) return null; - const end = tr.mapping.map(to, -1); - tr.setSelection(TextSelection.between(tr.doc.resolve(Math.min(from, end)), tr.doc.resolve(end))); + // An inserted-at position maps to itself unless it associates rightwards, + // which would leave the caret in front of the text just written. + const end = tr.mapping.map(to, inserting ? 1 : -1); + const $end = tr.doc.resolve(end); + tr.setSelection(inserting ? TextSelection.near($end, -1) : TextSelection.between(tr.doc.resolve(Math.min(from, end)), $end)); editor.view.dispatch(tr.scrollIntoView()); return end; }