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; }