mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-26 10:26:26 +01:00
fix(ui): stop markdown file viewer from re-rendering and remounting
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 <noreply@anthropic.com>
This commit is contained in:
1 parent
f8a9fd481a
commit
8a0affd74e
3 files changed
+160
-47
No files matched your search
@@ -517,20 +517,6 @@ export function FileViewerSheet({
|
||||
const copyFeedbackTimerRef = useRef<number | null>(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);
|
||||
|
||||
@@ -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">) => (
|
||||
<a href={to} {...props}>{children}</a>
|
||||
),
|
||||
}));
|
||||
|
||||
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<typeof createRoot> | 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 (
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<ThemeProvider>
|
||||
<MarkdownBody>{children}</MarkdownBody>
|
||||
</ThemeProvider>
|
||||
</QueryClientProvider>
|
||||
);
|
||||
}
|
||||
|
||||
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);
|
||||
});
|
||||
});
|
||||
@@ -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<Options["remarkPlugins"]> = [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<NonNullable<Options["remarkPlugins"]>>(() => {
|
||||
const plugins: NonNullable<Options["remarkPlugins"]> = [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<Components>(() => {
|
||||
const map: Components = {
|
||||
p: ({ node: _node, style: paragraphStyle, children: paragraphChildren, ...paragraphProps }) => (
|
||||
<p {...paragraphProps} style={mergeWrapStyle(paragraphStyle as React.CSSProperties | undefined)}>
|
||||
{paragraphChildren}
|
||||
@@ -726,22 +740,24 @@ export function MarkdownBody({
|
||||
</a>
|
||||
);
|
||||
},
|
||||
};
|
||||
if (resolveImageSrc || onImageClick) {
|
||||
components.img = ({ node: _node, src, alt, ...imgProps }) => {
|
||||
const resolved = resolveImageSrc && src ? resolveImageSrc(src) : null;
|
||||
const finalSrc = resolved ?? src;
|
||||
return (
|
||||
<img
|
||||
{...imgProps}
|
||||
src={finalSrc}
|
||||
alt={alt ?? ""}
|
||||
onClick={onImageClick && finalSrc ? (e) => { 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 (
|
||||
<img
|
||||
{...imgProps}
|
||||
src={finalSrc}
|
||||
alt={alt ?? ""}
|
||||
onClick={onImageClick && finalSrc ? (e) => { 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 (
|
||||
<div
|
||||
|
||||
Reference in new issue
Block a user