From 00b8f8462088711551834e72cbfc7277bbc3fcf5 Mon Sep 17 00:00:00 2001 From: Mads Taanquist Date: Tue, 28 Jul 2026 22:39:44 +0200 Subject: [PATCH] Diff each scene revision once when loading a review The Review tab re-anchored every comment and suggested edit with its own character diff of the scene it sits in. A story whose scenes have moved on since the notes were left pays that diff once per anchor: on a 45k-character scene it is around 260ms each, and the load runs them back to back on one thread, so the page hangs before it renders. The threads and suggestions on a scene almost all share the same pair of texts, so an anchor mapper now diffs each pair once and maps every anchor through that one result, shared between listThreads and listSuggestions for the page. On a restored production story (20 scenes, 98 threads, 53 suggestions) that takes the load's 26 diffs down to 5, and the load itself from about 4.2s to 570ms. reanchorRange and reanchorPoint keep their signatures and behaviour; the offset maths moved into shared helpers so the cached and uncached paths cannot drift, and the new tests check the mapper against them exhaustively. Co-Authored-By: Claude Opus 5 (1M context) --- src/lib/review-anchor.test.ts | 60 +++++++++++++- src/lib/review-anchor.ts | 83 ++++++++++++++++--- src/lib/server/export-reviews.ts | 4 +- src/lib/server/review.ts | 26 ++++-- .../api/assistant/review-reply/+server.ts | 13 ++- src/routes/review/[token]/+page.server.ts | 9 +- .../stories/[id]/review/+page.server.ts | 9 +- 7 files changed, 170 insertions(+), 34 deletions(-) diff --git a/src/lib/review-anchor.test.ts b/src/lib/review-anchor.test.ts index 0dabdc5..736957d 100644 --- a/src/lib/review-anchor.test.ts +++ b/src/lib/review-anchor.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { reanchorPoint, reanchorRange } from './review-anchor'; +import { createAnchorMapper, reanchorPoint, reanchorRange } from './review-anchor'; const BASE = 'The quick brown fox jumps over the lazy dog.'; // Anchor "brown fox" = [10, 19). @@ -84,3 +84,61 @@ describe('reanchorPoint', () => { expect(reanchorPoint(BASE, BASE, BASE.length + 1)).toBeNull(); }); }); + +describe('createAnchorMapper', () => { + // The mapper reuses one diff per pair of texts, so what it returns has to + // stay identical to re-anchoring each anchor on its own. + const CASES: Array<[string, string]> = [ + [BASE, BASE], + [BASE, 'A very ' + BASE], + [BASE, BASE.replace('lazy dog', 'sleeping cat')], + [BASE, BASE.replace('brown fox', 'brown furry fox')], + [BASE, BASE.replace('brown fox', 'red vixen')], + [BASE, BASE.replace('brown fox ', '')] + ]; + + it('matches reanchorRange on every offset', () => { + const mapper = createAnchorMapper(); + for (const [base, current] of CASES) { + for (let start = 0; start < base.length; start++) { + for (let end = start; end <= base.length + 1; end++) { + expect(mapper.range(base, current, start, end)).toEqual( + reanchorRange(base, current, start, end) + ); + } + } + } + }); + + it('matches reanchorPoint on every position', () => { + const mapper = createAnchorMapper(); + for (const [base, current] of CASES) { + for (let position = -1; position <= base.length + 1; position++) { + expect(mapper.point(base, current, position)).toEqual( + reanchorPoint(base, current, position) + ); + } + } + }); + + it('keeps separate results for different current texts against one base', () => { + const mapper = createAnchorMapper(); + const shifted = 'A very ' + BASE; + const rewritten = BASE.replace('brown fox', 'red vixen'); + expect(mapper.range(BASE, shifted, START, END)).toEqual({ start: START + 7, end: END + 7 }); + expect(mapper.range(BASE, rewritten, START, END)).toBeNull(); + // Repeating the first pair still maps through its own diff, not the second's. + expect(mapper.range(BASE, shifted, START, END)).toEqual({ start: START + 7, end: END + 7 }); + }); + + it('diffs each pair of texts once however many anchors map through it', () => { + const mapper = createAnchorMapper(); + const current = BASE.replace('lazy dog', 'sleeping cat'); + const first = mapper.range(BASE, current, START, END); + const t0 = performance.now(); + for (let i = 0; i < 1000; i++) mapper.range(BASE, current, START, END); + // A thousand cached maps cost far less than a thousand diffs would. + expect(performance.now() - t0).toBeLessThan(50); + expect(mapper.range(BASE, current, START, END)).toEqual(first); + }); +}); diff --git a/src/lib/review-anchor.ts b/src/lib/review-anchor.ts index bb41dcc..429df21 100644 --- a/src/lib/review-anchor.ts +++ b/src/lib/review-anchor.ts @@ -1,4 +1,4 @@ -import { diffChars } from 'diff'; +import { diffChars, type Change } from 'diff'; // Re-anchors a review thread's character range after the text changed. // Comments pin a [start, end) range against the revision they were made on; @@ -15,12 +15,31 @@ export function reanchorRange( ): { start: number; end: number } | null { if (start < 0 || end > baseText.length || start >= end) return null; if (baseText === currentText) return { start, end }; + return mapRange(diffChars(baseText, currentText), start, end); +} + +// Re-anchors a single position (a pure insertion point). The position +// survives if it lies in or at the boundary of unchanged text. +export function reanchorPoint( + baseText: string, + currentText: string, + position: number +): number | null { + if (position < 0 || position > baseText.length) return null; + if (baseText === currentText) return position; + return mapPoint(diffChars(baseText, currentText), position); +} +function mapRange( + parts: Change[], + start: number, + end: number +): { start: number; end: number } | null { let basePos = 0; let currentPos = 0; let newStart = -1; let newEnd = -1; - for (const part of diffChars(baseText, currentText)) { + for (const part of parts) { const length = part.value.length; if (part.added) { currentPos += length; @@ -45,19 +64,10 @@ export function reanchorRange( return { start: newStart, end: newEnd }; } -// Re-anchors a single position (a pure insertion point). The position -// survives if it lies in or at the boundary of unchanged text. -export function reanchorPoint( - baseText: string, - currentText: string, - position: number -): number | null { - if (position < 0 || position > baseText.length) return null; - if (baseText === currentText) return position; - +function mapPoint(parts: Change[], position: number): number | null { let basePos = 0; let currentPos = 0; - for (const part of diffChars(baseText, currentText)) { + for (const part of parts) { const length = part.value.length; if (part.added) { currentPos += length; @@ -75,3 +85,50 @@ export function reanchorPoint( } return null; } + +// Every thread and suggestion anchored to the same scene revision re-anchors +// against the same two texts, and a character diff of a long scene costs +// hundreds of milliseconds. A mapper diffs each pair of texts once and maps +// every anchor through that one result. Make one per request and hand it to +// everything that re-anchors, so the cost is per revision rather than per +// comment. +export type AnchorMapper = { + range( + baseText: string, + currentText: string, + start: number, + end: number + ): { start: number; end: number } | null; + point(baseText: string, currentText: string, position: number): number | null; +}; + +export function createAnchorMapper(): AnchorMapper { + // Nested by base text then current text, so neither key has to be + // concatenated into a third copy of the scene. + const cache = new Map>(); + const diff = (baseText: string, currentText: string): Change[] => { + let byCurrent = cache.get(baseText); + if (!byCurrent) { + byCurrent = new Map(); + cache.set(baseText, byCurrent); + } + let parts = byCurrent.get(currentText); + if (!parts) { + parts = diffChars(baseText, currentText); + byCurrent.set(currentText, parts); + } + return parts; + }; + return { + range(baseText, currentText, start, end) { + if (start < 0 || end > baseText.length || start >= end) return null; + if (baseText === currentText) return { start, end }; + return mapRange(diff(baseText, currentText), start, end); + }, + point(baseText, currentText, position) { + if (position < 0 || position > baseText.length) return null; + if (baseText === currentText) return position; + return mapPoint(diff(baseText, currentText), position); + } + }; +} diff --git a/src/lib/server/export-reviews.ts b/src/lib/server/export-reviews.ts index 238cd85..b3f2d2a 100644 --- a/src/lib/server/export-reviews.ts +++ b/src/lib/server/export-reviews.ts @@ -2,7 +2,7 @@ import { and, eq, isNull } from 'drizzle-orm'; import type { Database } from './auth.ts'; import { scenes } from './db/schema.ts'; import { listThreads } from './review.ts'; -import { reanchorRange } from '../review-anchor.ts'; +import { createAnchorMapper } from '../review-anchor.ts'; import type { ExportReviewThread, ReviewLoader } from './export.ts'; // Long selections are trimmed: the excerpt locates the thread, the scene @@ -15,7 +15,7 @@ const EXCERPT_MAX = 240; // export builders does not drag in the review module. export function reviewLoader(db: Database): ReviewLoader { return async (storyId) => { - const threads = await listThreads(db, storyId, reanchorRange); + const threads = await listThreads(db, storyId, createAnchorMapper().range); if (threads.length === 0) return []; const sceneRows = await db .select({ id: scenes.id, title: scenes.title, bodyMd: scenes.bodyMd }) diff --git a/src/lib/server/review.ts b/src/lib/server/review.ts index 753bd12..a5bd80b 100644 --- a/src/lib/server/review.ts +++ b/src/lib/server/review.ts @@ -15,7 +15,7 @@ import { import { hashToken } from './tokens.ts'; import { signToken, verifyToken } from './crypto.ts'; import { recordRevision } from './revisions.ts'; -import { reanchorPoint, reanchorRange } from '../review-anchor.ts'; +import { createAnchorMapper, type AnchorMapper } from '../review-anchor.ts'; import { wordCount } from '../word-count.ts'; import { normaliseAssistantName } from './llm/prompts/persona.ts'; import { EMAIL_RE } from './signup.ts'; @@ -634,24 +634,26 @@ export type SuggestionView = { }; // Re-anchors the proposed range onto the current text: a real range maps -// through reanchorRange, a pure insertion point through reanchorPoint. +// through the mapper's range, a pure insertion point through its point. function mapSuggestionRange( + mapper: AnchorMapper, baseText: string, currentText: string, start: number, end: number ): { start: number; end: number } | null { if (start === end) { - const point = reanchorPoint(baseText, currentText, start); + const point = mapper.point(baseText, currentText, start); return point === null ? null : { start: point, end: point }; } - return reanchorRange(baseText, currentText, start, end); + return mapper.range(baseText, currentText, start, end); } // Where a decided suggestion now sits, so the Done view can point at it: the // accepted replacement (the new text it left behind) or the rejected original // (still in place). Null when there is nothing to mark or it cannot be found. function decidedAnchor( + mapper: AnchorMapper, status: 'accepted' | 'rejected', baseText: string, currentText: string, @@ -662,20 +664,24 @@ function decidedAnchor( if (status === 'rejected') { // Rejection left the text untouched, so the original passage maps as-is. if (start === end) return null; // a rejected insertion left no text - return reanchorRange(baseText, currentText, start, end); + return mapper.range(baseText, currentText, start, end); } // Accepted: the replacement now occupies the spot the original held. if (replacement.length === 0) return null; // a deletion leaves nothing to mark - const point = reanchorPoint(baseText, currentText, start); + const point = mapper.point(baseText, currentText, start); if (point === null) return null; const at = { start: point, end: point + replacement.length }; return at.end <= currentText.length ? at : null; } +// The mapper is shared with listThreads when both run for the same story, so +// each scene revision is diffed once for the whole page rather than once per +// thread and once per suggestion. export async function listSuggestions( db: Database, storyId: string, - viewer?: ReviewViewer + viewer?: ReviewViewer, + mapper: AnchorMapper = createAnchorMapper() ): Promise { const rows = await db .select({ @@ -701,8 +707,9 @@ export async function listSuggestions( // anchors to where it now sits, so the Done view can point at it. const anchor = row.suggestion.status === 'pending' - ? mapSuggestionRange(row.baseBody, row.currentBody, rangeStart, rangeEnd) + ? mapSuggestionRange(mapper, row.baseBody, row.currentBody, rangeStart, rangeEnd) : decidedAnchor( + mapper, row.suggestion.status, row.baseBody, row.currentBody, @@ -819,7 +826,10 @@ export async function decideSuggestion( .where(eq(scenes.id, row.scene.id)) .for('update'); if (!scene) return { ok: false, reason: 'That scene does not exist.' }; + // One anchor against the text just read under the lock, so there is + // nothing for a shared mapper to reuse. const anchor = mapSuggestionRange( + createAnchorMapper(), row.baseBody, scene.bodyMd, row.suggestion.rangeStart, diff --git a/src/routes/api/assistant/review-reply/+server.ts b/src/routes/api/assistant/review-reply/+server.ts index 7944d8b..9f4aff7 100644 --- a/src/routes/api/assistant/review-reply/+server.ts +++ b/src/routes/api/assistant/review-reply/+server.ts @@ -13,7 +13,7 @@ import { assembleContext, buildSystemMessage } from '$lib/server/llm/context/ass import { buildReviewReplyMessage, excerptAround } from '$lib/server/llm/prompts/review-reply'; import { complete } from '$lib/server/llm/gateway'; import { addComment, listSuggestions, listThreads } from '$lib/server/review'; -import { reanchorRange } from '$lib/review-anchor'; +import { createAnchorMapper } from '$lib/review-anchor'; import { isUuid } from '$lib/slug'; import type { ChatMessage } from '$lib/server/llm/providers/types'; @@ -48,8 +48,11 @@ export const POST: RequestHandler = async ({ request, locals }) => { const layout = await requireAssistantGate(userId, story.id); - // The full thread view carries the re-anchored range and display names. - const threads = await listThreads(db, story.id, reanchorRange, { userId }); + // The full thread view carries the re-anchored range and display names. One + // mapper covers the threads and the suggestion lookup below, so a scene + // revision is diffed once rather than once per anchor. + const anchors = createAnchorMapper(); + const threads = await listThreads(db, story.id, anchors.range, { userId }); const thread = threads.find((t) => t.id === threadId); if (!thread) error(404, 'That thread does not exist.'); if (!thread.comments[0]?.isAssistant) { @@ -57,7 +60,9 @@ export const POST: RequestHandler = async ({ request, locals }) => { } const suggestion = thread.suggestionId - ? ((await listSuggestions(db, story.id)).find((s) => s.id === thread.suggestionId) ?? null) + ? ((await listSuggestions(db, story.id, undefined, anchors)).find( + (s) => s.id === thread.suggestionId + ) ?? null) : null; const [scene] = await db diff --git a/src/routes/review/[token]/+page.server.ts b/src/routes/review/[token]/+page.server.ts index 11ace6c..61dfdcc 100644 --- a/src/routes/review/[token]/+page.server.ts +++ b/src/routes/review/[token]/+page.server.ts @@ -22,7 +22,7 @@ import { } from '$lib/server/review'; import { gatherStory } from '$lib/server/export'; import { reviewMentionData } from '$lib/server/mention-entities'; -import { reanchorRange } from '$lib/review-anchor'; +import { createAnchorMapper } from '$lib/review-anchor'; import { rateLimit } from '$lib/server/rate-limit'; import { notifyReviewActivity } from '$lib/server/notify'; @@ -88,6 +88,9 @@ export const load: PageServerLoad = async ({ params, cookies, locals }) => { sceneIds: scenes.map((scene) => scene.id), restrictToMentioned: true }); + // Threads and suggestions re-anchor against the same scene revisions, so one + // mapper covers both and each revision is diffed once for the page. + const anchors = createAnchorMapper(); return { state: 'review' as const, storyTitle: resolved.storyTitle, @@ -95,8 +98,8 @@ export const load: PageServerLoad = async ({ params, cookies, locals }) => { canSuggest: resolved.invitation.canSuggest, chapters: content.chapters, scenes, - threads: await listThreads(db, storyId, reanchorRange, { reviewerId: reviewer.id }), - suggestions: await listSuggestions(db, storyId, { reviewerId: reviewer.id }), + threads: await listThreads(db, storyId, anchors.range, { reviewerId: reviewer.id }), + suggestions: await listSuggestions(db, storyId, { reviewerId: reviewer.id }, anchors), mentionEntities: mentions.entities, mentionMembers: mentions.storyMembers, mentionPins: mentions.pins diff --git a/src/routes/stories/[id]/review/+page.server.ts b/src/routes/stories/[id]/review/+page.server.ts index 7e690c9..83f9000 100644 --- a/src/routes/stories/[id]/review/+page.server.ts +++ b/src/routes/stories/[id]/review/+page.server.ts @@ -20,7 +20,7 @@ import { gatherStory } from '$lib/server/export'; import { storyPreferences } from '$lib/server/preferences'; import { storyPageSetup } from '$lib/server/page-setup'; import { reviewMentionData } from '$lib/server/mention-entities'; -import { reanchorRange } from '$lib/review-anchor'; +import { createAnchorMapper } from '$lib/review-anchor'; import { queueSceneMentions } from '$lib/server/jobs'; import { assistantLayout, saveStoryLlmOverride } from '$lib/server/llm/config'; import { listChat } from '$lib/server/llm/chat-history'; @@ -38,6 +38,9 @@ export const load: PageServerLoad = async ({ params, locals }) => { // navigation as the editor. The author's review takes the full cast // (restrictToMentioned is false), so reviewMentionData needs no scene ids and // nothing here waits on gatherStory. + // Threads and suggestions re-anchor against the same scene revisions, so one + // mapper covers both and each revision is diffed once for the page. + const anchors = createAnchorMapper(); const [ content, mentions, @@ -56,8 +59,8 @@ export const load: PageServerLoad = async ({ params, locals }) => { restrictToMentioned: false }), listTrashedScenes(db, story.id), - listThreads(db, story.id, reanchorRange, { userId: locals.user!.id }), - listSuggestions(db, story.id, { userId: locals.user!.id }), + listThreads(db, story.id, anchors.range, { userId: locals.user!.id }), + listSuggestions(db, story.id, { userId: locals.user!.id }, anchors), storyPreferences(db, locals.user!.id, story.id), storyPageSetup(db, story.id), assistantLayout(db, locals.user!.id, story.id)