diff --git a/packages/app/e2e/browser/agent-timeline-scroll-smoothness.spec.ts b/packages/app/e2e/browser/agent-timeline-scroll-smoothness.spec.ts index b45007fa0b..ee4201ea75 100644 --- a/packages/app/e2e/browser/agent-timeline-scroll-smoothness.spec.ts +++ b/packages/app/e2e/browser/agent-timeline-scroll-smoothness.spec.ts @@ -34,6 +34,7 @@ test("scroll detector distinguishes entered image growth from a simultaneous vie const after: ScrollFrame = { ...before, at: 1400, + scrollTop: 900, wheelTotal: 100, lastWheelAt: 1050, rows: [ @@ -48,6 +49,7 @@ test("scroll detector distinguishes entered image growth from a simultaneous vie { ...after, wheelTotal: 1000, + scrollTop: 0, rows: after.rows.map((row) => ({ ...row, top: row.top + 100 })), }, ]), @@ -66,6 +68,70 @@ test("scroll detector distinguishes entered image growth from a simultaneous vie ).toHaveLength(1); }); +test("scroll detector accounts for delayed wheel input and growth below the new reader", () => { + const before: ScrollFrame = { + at: 1000, + scrollTop: 2000, + scrollHeight: 5000, + viewportHeight: 800, + virtualized: true, + loading: false, + rows: [ + { id: "new-reader", top: -520, height: 278 }, + { id: "image", top: -242, height: 184 }, + { id: "old-reader", top: -58, height: 130 }, + ], + anchor: "old-reader", + wheelTotal: 0, + lastWheelAt: 0, + inputFinishedAt: null, + imageLoads: 0, + mounted: 0, + unmounted: 0, + }; + const wheel = { ...before, at: 1100, wheelTotal: 480, lastWheelAt: 1099 }; + const after: ScrollFrame = { + ...wheel, + at: 1220, + scrollTop: 1520, + scrollHeight: 5041, + anchor: "new-reader", + rows: [ + { id: "new-reader", top: -40, height: 278 }, + { id: "image", top: 238, height: 225 }, + { id: "old-reader", top: 463, height: 130 }, + ], + }; + expect(findScrollJumps([before, wheel, after])).toEqual([]); + // The previous 480 px input has already moved the viewport; the recent + // budget includes it, but only the next 480 px belongs to this frame. + const steadyBefore = { ...before, at: 1050, wheelTotal: 480, lastWheelAt: 1049 }; + const steadyAfter = { + ...after, + at: 1100, + wheelTotal: 960, + lastWheelAt: 1099, + scrollHeight: 6000, + rows: [ + { id: "new-reader", top: -40, height: 278 }, + { id: "image", top: 238, height: 1184 }, + { id: "old-reader", top: 1422, height: 130 }, + ], + }; + expect(findScrollJumps([steadyBefore, steadyAfter])).toEqual([]); + + expect( + findScrollJumps([ + before, + wheel, + { + ...after, + rows: after.rows.map((row) => ({ id: row.id, height: row.height, top: row.top + 100 })), + }, + ]), + ).toHaveLength(1); +}); + for (const cadence of scrollCadences) { test(`varied timeline preserves reading position during ${cadence.name} upward scrolling`, async ({ page, diff --git a/packages/app/e2e/support/helpers/timeline-scroll-smoothness.ts b/packages/app/e2e/support/helpers/timeline-scroll-smoothness.ts index 63ef1c10a0..f550b02fdc 100644 --- a/packages/app/e2e/support/helpers/timeline-scroll-smoothness.ts +++ b/packages/app/e2e/support/helpers/timeline-scroll-smoothness.ts @@ -278,12 +278,13 @@ export function findScrollJumps(frames: ScrollFrame[]) { if (!previous?.anchor) return []; // Wheel input can move the reading line onto an image before it expands. // Follow that intended row, not text now below the image. - const inputDelta = current.wheelTotal - previous.wheelTotal; - const availableScroll = Math.min(inputDelta, previous.scrollTop); + const recent = frames.findLast((frame) => frame.at <= previous.at - 100); + const wheelBudget = current.wheelTotal - (recent?.wheelTotal ?? 0); + const frameScroll = Math.max(0, previous.scrollTop - current.scrollTop); + const availableScroll = Math.min(frameScroll, wheelBudget, previous.scrollTop); const readingLine = 8 - availableScroll; const currentRows = new Map(current.rows.map((row) => [row.id, row])); const intendedRow = previous.rows.find((row) => row.top + row.height > readingLine); - const intendedHeight = intendedRow && currentRows.get(intendedRow.id)?.height; const anchor = previous.anchor; const before = currentRows.has(anchor) ? previous.rows.find((row) => row.id === anchor)! @@ -298,18 +299,21 @@ export function findScrollJumps(frames: ScrollFrame[]) { // A busy main thread can deliver wheel movement hundreds of milliseconds // after its event. Only assert idle stability after the driver stops input. const idle = current.inputFinishedAt !== null && previous.at - current.inputFinishedAt > 250; - const recent = frames.findLast((frame) => frame.at <= previous.at - 100); - const wheelBudget = current.wheelTotal - (recent?.wheelTotal ?? 0); - // Growth displaces only later rows. It never grants extra movement to the - // image itself, and its top must still follow the input budget. - const enteredRowGrowth = - inputDelta > 0 && + // Wheel events can precede their scroll update, or include input already + // applied in earlier frames. Locate the reader with this frame's scroll, + // bounded by recent input. Growth below it can move the old anchor without + // moving the reading line. + const readerFollowsInput = intendedRow && - intendedHeight !== undefined && - before.top >= intendedRow.top + intendedRow.height && - Math.abs(currentRows.get(intendedRow.id)!.top - intendedRow.top - availableScroll) <= 32 - ? Math.max(0, intendedHeight - intendedRow.height) - : 0; + currentRows.has(intendedRow.id) && + Math.abs(currentRows.get(intendedRow.id)!.top - intendedRow.top - availableScroll) <= 32; + const enteredRowGrowth = readerFollowsInput + ? previous.rows.reduce((growth, row) => { + if (row.top < intendedRow.top || row.top >= before.top) return growth; + const height = currentRows.get(row.id)?.height ?? row.height; + return growth + Math.max(0, height - row.height); + }, 0) + : 0; const excessForward = movement > wheelBudget + enteredRowGrowth + 32; // Upward wheel input moves the same text DOWN the viewport. A negative move // is a reversal, independent of legitimate scrollTop compensation on prepend. diff --git a/packages/app/src/components/markdown/html-ish.test.ts b/packages/app/src/components/markdown/html-ish.test.ts index 37e3aeddb5..394ea47ed8 100644 --- a/packages/app/src/components/markdown/html-ish.test.ts +++ b/packages/app/src/components/markdown/html-ish.test.ts @@ -113,6 +113,85 @@ describe("splitHtmlishMarkdown", () => { ]); }); + it("renders a linked picture inside a heading without exposing its HTML", () => { + const source = [ + '

', + '', + '', + 'Retrigger', + "

", + "", + "Confidence Score: 5/5", + "", + "
SummaryNo outstanding findings.
", + ].join("\n"); + + expect(splitHtmlishMarkdown(source)).toEqual([ + { + kind: "inlineImage", + alt: "Retrigger", + src: "https://example.com/default.svg", + href: "https://example.com/retrigger?id=73851569", + width: 85, + height: 24, + }, + { kind: "markdown", text: "\n\nConfidence Score: 5/5\n\n" }, + { kind: "details", summary: "Summary", body: "No outstanding findings." }, + ]); + }); + + it.each([ + ["", ""], + ["

", "

"], + ['', ""], + ])("uses the picture fallback without activating unsafe links (%s)", (wrapper, closing) => { + const picture = + 'Image'; + + expect(splitHtmlishMarkdown(`${wrapper}${picture}${closing}`)).toEqual([ + { kind: "inlineImage", src: "https://example.com/default.svg", alt: "Image" }, + ]); + }); + + it("keeps picture examples literal inside inline and fenced code", () => { + const picture = + ''; + const source = `Use \`${picture}\`\n\n\`\`\`html\n${picture}\n\`\`\``; + expect(splitHtmlishMarkdown(source)).toEqual([{ kind: "markdown", text: source }]); + }); + + it.each(["p", "div"])("renders a linked picture inside %s", (wrapper) => { + const source = `<${wrapper}>Image`; + expect(splitHtmlishMarkdown(source)).toEqual([ + { + kind: "inlineImage", + src: "https://example.com/default.svg", + alt: "Image", + href: "https://example.com/page", + }, + ]); + }); + + it.each(["h2", "p", "div"])("keeps text after an image-only %s on a separate row", (wrapper) => { + const source = `<${wrapper}>ImageNext section`; + expect(splitHtmlishMarkdown(source)).toEqual([ + { kind: "inlineImage", src: "https://example.com/image.svg", alt: "Image" }, + { kind: "markdown", text: "Next section" }, + ]); + }); + + it("handles deeply nested image wrappers without exhausting the stack", () => { + const source = `${"
".repeat(10000)}Image${"
".repeat(10000)}`; + expect(splitHtmlishMarkdown(source)).toEqual([ + { + kind: "inlineImage", + src: "https://example.com/image.svg", + alt: "Image", + href: "https://example.com/page", + }, + ]); + }); + it("preserves inline image parts inside details bodies", () => { expect( splitHtmlishMarkdown( diff --git a/packages/app/src/components/markdown/html-ish.ts b/packages/app/src/components/markdown/html-ish.ts index 93a88cd4b8..346a0dfc13 100644 --- a/packages/app/src/components/markdown/html-ish.ts +++ b/packages/app/src/components/markdown/html-ish.ts @@ -29,7 +29,7 @@ const FENCE_LINE_RE = /^ {0,3}([`~]{3,})[^\n\r]*(?:\r?\n|$)/gm; const BACKTICK_RUN_RE = /`+/g; const SAFE_IMAGE_SRC_RE = /^(https?:\/\/|data:image\/(?:png|gif|jpe?g);base64,)/i; const SAFE_LINK_HREF_RE = /^(https?:\/\/|#(?:$|[\w-]))/i; -const VOID_HTML_TAGS = new Set(["br", "img"]); +const VOID_HTML_TAGS = new Set(["br", "img", "source"]); const MARKDOWN_TAG_WRAPPERS: Readonly> = { b: ["**", "**"], del: ["~~", "~~"], @@ -140,25 +140,73 @@ function parseInlineImageAt(tokens: HtmlToken[], start: number): InlineImagePars return image ? { part: image, end: start + 1 } : null; } - if (token.name !== "a") { + if (token.name !== "a" && token.name !== "picture" && !isImageBlockWrapper(token)) { return null; } - const closeIndex = findMatchingClose(tokens, start, "a"); - if (closeIndex === null) { - return null; - } + return parseWrappedImageAt(tokens, start); +} - const image = getSingleImageChild(tokens.slice(start + 1, closeIndex)); - if (!image) { - return null; - } +function parseWrappedImageAt(tokens: HtmlToken[], start: number): InlineImageParseResult | null { + // Walk an image-only wrapper chain once. Recursing over copied child slices + // makes deeply nested review HTML quadratic and can exhaust the JS stack. + const wrappers: string[] = []; + let image: MarkdownInlineImagePart | null = null; + let href: string | undefined; + let hasLink = false; - const inlineImage = imageTokenToInlineImage(image, safeHref(token.attributes.href)); - return inlineImage ? { part: inlineImage, end: closeIndex + 1 } : null; + for (let index = start; index < tokens.length; index += 1) { + const child = tokens[index]; + if (child.kind === "comment" || isWhitespaceText(child)) { + continue; + } + if (child.kind !== "tag") { + return null; + } + if (child.closing) { + if (!image || wrappers.pop() !== child.name) { + return null; + } + if (wrappers.length === 0) { + return { part: image, end: index + 1 }; + } + continue; + } + // Pictures use their img fallback, not responsive source selection. + if (child.name === "source" && wrappers.at(-1) === "picture") { + continue; + } + if (image) { + return null; + } + if (child.name === "img") { + image = imageTokenToInlineImage(child, href); + if (!image) { + return null; + } + continue; + } + if (child.name !== "a" && child.name !== "picture" && !isImageBlockWrapper(child)) { + return null; + } + if (child.name === "a" && !hasLink) { + href = safeHref(child.attributes.href); + hasLink = true; + } + wrappers.push(child.name); + } + return null; +} + +function isImageBlockWrapper(token: HtmlToken | undefined): boolean { + return isHeadingTag(token) || isOpenTag(token, "p") || isOpenTag(token, "div"); } function flowsWithFollowingText(tokens: HtmlToken[], start: number, end: number): boolean { + if (isImageBlockWrapper(tokens[start])) { + return false; + } + const previous = tokens[start - 1]; // At line start if nothing precedes this image, or the preceding token ends with only // whitespace since the last newline (covers a bare space between two images on the same line). @@ -191,6 +239,9 @@ function flowsWithFollowingText(tokens: HtmlToken[], start: number, end: number) } // An inline image tag — skip over it (the image itself and its possible wrapping close tag). if (token.kind === "tag") { + if (isImageBlockWrapper(token)) { + return false; + } const imageResult = parseInlineImageAt(tokens, cursor); if (imageResult) { cursor = imageResult.end;