From 8a0affd74ead6fb6c43de3ad6d9efb766dc70834 Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 10 Jun 2026 02:33:13 +0000 Subject: [PATCH] fix(ui): stop markdown file viewer from re-rendering and remounting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file viewer flashed, lost code-block scroll position, and cleared text selection on markdown files (PAP-10767). Two compounding causes: - FileViewerSheet ran a 75ms `elapsedMs` ticker that only drives the loading skeleton, but it never stopped after the file loaded — re-rendering the whole sheet ~13x/second. Now the ticker runs only while a preview is still loading. - MarkdownBody rebuilt its react-markdown `remarkPlugins` array and `components` map on every render. Because react-markdown uses the `components` values as component *types*, new function identities each render made React unmount and remount the entire rendered tree (discarding scroll/selection, causing the flash). Both are now memoized. Adds a regression test asserting markdown DOM nodes survive a parent re-render. Co-Authored-By: Claude Opus 4.8 --- ui/src/components/FileViewerSheet.tsx | 38 +++++--- .../components/MarkdownBody.rerender.test.tsx | 87 +++++++++++++++++++ ui/src/components/MarkdownBody.tsx | 82 ++++++++++------- 3 files changed, 160 insertions(+), 47 deletions(-) create mode 100644 ui/src/components/MarkdownBody.rerender.test.tsx diff --git a/ui/src/components/FileViewerSheet.tsx b/ui/src/components/FileViewerSheet.tsx index 19fe58829a5..554e082c237 100644 --- a/ui/src/components/FileViewerSheet.tsx +++ b/ui/src/components/FileViewerSheet.tsx @@ -517,20 +517,6 @@ export function FileViewerSheet({ const copyFeedbackTimerRef = useRef(null); const resizeCleanupRef = useRef<(() => void) | null>(null); - useEffect(() => { - if (!state) { - setElapsedMs(0); - return; - } - const now = Date.now(); - setElapsedMs(0); - const interval = window.setInterval(() => { - setElapsedMs(Date.now() - now); - }, 75); - return () => window.clearInterval(interval); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [state?.path, state?.workspace, state?.projectId, state?.workspaceId]); - const resolveQuery = useQuery({ queryKey: state ? queryKeys.issues.fileResource(issueId, state) @@ -554,6 +540,30 @@ export function FileViewerSheet({ staleTime: 30_000, }); + // `elapsedMs` only drives the progressive loading skeleton (see LoadingView). + // Run the 75ms ticker *only* while a preview is still loading — leaving it + // running after content arrives would re-render the whole sheet ~13x/second, + // which forces the markdown body to re-render and discards scroll position + // and text selection, producing visible flashing (PAP-10767). + const isLoadingPreview = + (resolveQuery.isFetching && !resolveQuery.data) || + (canPreview && contentQuery.isFetching && !contentQuery.data); + + useEffect(() => { + if (!state) { + setElapsedMs(0); + return; + } + if (!isLoadingPreview) return; + const now = Date.now(); + setElapsedMs(0); + const interval = window.setInterval(() => { + setElapsedMs(Date.now() - now); + }, 75); + return () => window.clearInterval(interval); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [state?.path, state?.workspace, state?.projectId, state?.workspaceId, isLoadingPreview]); + useEffect(() => { if (resolveQuery.isError) { const normalized = normalizeError(resolveQuery.error); diff --git a/ui/src/components/MarkdownBody.rerender.test.tsx b/ui/src/components/MarkdownBody.rerender.test.tsx new file mode 100644 index 00000000000..3843768d247 --- /dev/null +++ b/ui/src/components/MarkdownBody.rerender.test.tsx @@ -0,0 +1,87 @@ +// @vitest-environment jsdom + +import { flushSync } from "react-dom"; +import { createRoot } from "react-dom/client"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { ThemeProvider } from "../context/ThemeContext"; +import { MarkdownBody } from "./MarkdownBody"; + +vi.mock("@/lib/router", () => ({ + Link: ({ + children, + to, + ...props + }: { children: React.ReactNode; to: string } & React.ComponentProps<"a">) => ( + {children} + ), +})); + +vi.mock("../api/issues", () => ({ + issuesApi: { + get: vi.fn(), + }, +})); + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +(globalThis as any).IS_REACT_ACT_ENVIRONMENT = true; + +let root: ReturnType | null = null; +let container: HTMLDivElement | null = null; + +afterEach(() => { + if (root) { + flushSync(() => root?.unmount()); + } + root = null; + container?.remove(); + container = null; +}); + +const SAMPLE = "Some text\n\n```ts\nconst answer = 42;\n```\n\nAnd a [link](https://example.com)."; + +function tree(children: string, queryClient: QueryClient) { + return ( + + + {children} + + + ); +} + +describe("MarkdownBody re-render stability (PAP-10767)", () => { + it("preserves rendered DOM nodes across a parent re-render with unchanged props", () => { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + + flushSync(() => root?.render(tree(SAMPLE, queryClient))); + + const preBefore = container.querySelector("pre"); + const codeBefore = container.querySelector("pre code"); + const anchorBefore = container.querySelector("a"); + expect(preBefore).not.toBeNull(); + expect(codeBefore).not.toBeNull(); + expect(anchorBefore).not.toBeNull(); + + // Re-render the identical tree. Before the memoization fix, MarkdownBody + // rebuilt its react-markdown `components` map on every render, giving each + // custom element (pre/code/a/...) a brand-new component *type* — which made + // React unmount and remount the whole subtree, discarding scroll position + // and text selection and producing the visible flashing in the file viewer. + flushSync(() => root?.render(tree(SAMPLE, queryClient))); + + const preAfter = container.querySelector("pre"); + const codeAfter = container.querySelector("pre code"); + const anchorAfter = container.querySelector("a"); + + // Same DOM node instances ⇒ React updated in place rather than remounting. + expect(preAfter).toBe(preBefore); + expect(codeAfter).toBe(codeBefore); + expect(anchorAfter).toBe(anchorBefore); + }); +}); diff --git a/ui/src/components/MarkdownBody.tsx b/ui/src/components/MarkdownBody.tsx index 60052cc0942..7afed5b2bae 100644 --- a/ui/src/components/MarkdownBody.tsx +++ b/ui/src/components/MarkdownBody.tsx @@ -1,4 +1,4 @@ -import { isValidElement, useCallback, useEffect, useId, useRef, useState, type ReactNode } from "react"; +import { isValidElement, useCallback, useEffect, useId, useMemo, useRef, useState, type ReactNode } from "react"; import { useQuery } from "@tanstack/react-query"; import { Check, Copy, ExternalLink, Github, WrapText } from "lucide-react"; import Markdown, { defaultUrlTransform, type Components, type Options } from "react-markdown"; @@ -581,23 +581,37 @@ export function MarkdownBody({ // may lack a CompanyProvider. A null context (or no companies yet) leaves // knownPrefixes undefined, which keeps issue auto-linking permissive. const company = useOptionalCompany(); - const knownPrefixes = company?.companies.length - ? company.companies.map((c) => c.issuePrefix) - : undefined; - const remarkPlugins: NonNullable = [remarkGfm]; - if (enableWikiLinks) { - remarkPlugins.push(createRemarkWikiLinks({ wikiLinkRoot, resolveWikiLinkHref })); - } - if (linkWorkspaceFileRefs) { - remarkPlugins.push(remarkWorkspaceFileRefs); - } - if (linkIssueReferences) { - remarkPlugins.push([remarkLinkIssueReferences, { knownPrefixes }]); - } - if (softBreaks) { - remarkPlugins.push(remarkSoftBreaks); - } - const components: Components = { + const companies = company?.companies; + // Stable identity so it can feed the memoized remark plugins without + // re-creating them (and forcing a full markdown re-parse) every render. + const knownPrefixes = useMemo( + () => (companies?.length ? companies.map((c) => c.issuePrefix) : undefined), + [companies], + ); + // react-markdown treats the values of `components` as component *types* and + // the `remarkPlugins` array by identity. Rebuilding either on every render + // forces react-markdown to unmount/remount the rendered tree, which discards + // scroll position and text selection and causes visible flashing when a + // parent re-renders frequently (see PAP-10767). Memoize both so re-renders + // that don't change the inputs are cheap and non-destructive. + const remarkPlugins = useMemo>(() => { + const plugins: NonNullable = [remarkGfm]; + if (enableWikiLinks) { + plugins.push(createRemarkWikiLinks({ wikiLinkRoot, resolveWikiLinkHref })); + } + if (linkWorkspaceFileRefs) { + plugins.push(remarkWorkspaceFileRefs); + } + if (linkIssueReferences) { + plugins.push([remarkLinkIssueReferences, { knownPrefixes }]); + } + if (softBreaks) { + plugins.push(remarkSoftBreaks); + } + return plugins; + }, [enableWikiLinks, wikiLinkRoot, resolveWikiLinkHref, linkWorkspaceFileRefs, linkIssueReferences, knownPrefixes, softBreaks]); + const components = useMemo(() => { + const map: Components = { p: ({ node: _node, style: paragraphStyle, children: paragraphChildren, ...paragraphProps }) => (

{paragraphChildren} @@ -726,22 +740,24 @@ export function MarkdownBody({ ); }, - }; - if (resolveImageSrc || onImageClick) { - components.img = ({ node: _node, src, alt, ...imgProps }) => { - const resolved = resolveImageSrc && src ? resolveImageSrc(src) : null; - const finalSrc = resolved ?? src; - return ( - {alt { e.preventDefault(); onImageClick(finalSrc); } : undefined} - style={onImageClick ? { cursor: "pointer", ...(imgProps.style as React.CSSProperties | undefined) } : imgProps.style as React.CSSProperties | undefined} - /> - ); }; - } + if (resolveImageSrc || onImageClick) { + map.img = ({ node: _node, src, alt, ...imgProps }) => { + const resolved = resolveImageSrc && src ? resolveImageSrc(src) : null; + const finalSrc = resolved ?? src; + return ( + {alt { e.preventDefault(); onImageClick(finalSrc); } : undefined} + style={onImageClick ? { cursor: "pointer", ...(imgProps.style as React.CSSProperties | undefined) } : imgProps.style as React.CSSProperties | undefined} + /> + ); + }; + } + return map; + }, [theme, linkIssueReferences, resolveImageSrc, onImageClick]); return (