mirror of
https://github.com/multica-ai/multica.git
synced 2026-06-30 02:51:19 +02:00
Compare commits
3 Commits
main
...
agent/j/f3
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
41811f0deb | ||
|
|
4b2394e1a9 | ||
|
|
573e2f5f9d |
@@ -1,6 +1,6 @@
|
||||
"use client";
|
||||
|
||||
import { useState, useEffect, useCallback, useMemo, useRef } from "react";
|
||||
import { useState, useEffect, useCallback, useMemo, useRef, useDeferredValue } from "react";
|
||||
import { useDefaultLayout } from "react-resizable-panels";
|
||||
import { useQuery } from "@tanstack/react-query";
|
||||
import { useWorkspaceId } from "@multica/core/hooks";
|
||||
@@ -75,6 +75,17 @@ export function InboxPage() {
|
||||
|
||||
const selected = items.find((i) => (i.issue_id ?? i.id) === selectedKey) ?? null;
|
||||
|
||||
// The detail pane mounts a heavy IssueDetail subtree (keyed by issue_id, so
|
||||
// it fully remounts on switch). Drive it off a deferred copy of the selection
|
||||
// so clicking a row stays urgent — the list highlight + mark-read fire
|
||||
// immediately on `selectedKey` — while the expensive remount renders at
|
||||
// transition priority. React keeps the previous detail painted until the new
|
||||
// one is ready and can drop intermediate work when the user clicks through
|
||||
// the list quickly, which is what removes the "frozen on click" feel.
|
||||
const deferredSelectedKey = useDeferredValue(selectedKey);
|
||||
const detailSelected =
|
||||
items.find((i) => (i.issue_id ?? i.id) === deferredSelectedKey) ?? null;
|
||||
|
||||
// Track the last key we actually resolved against the inbox list. Lets the
|
||||
// fallback effect distinguish "shared-link to a notification not in our
|
||||
// inbox" (never resolved → redirect to the issue page) from "item was in
|
||||
@@ -286,18 +297,21 @@ export function InboxPage() {
|
||||
</div>
|
||||
);
|
||||
|
||||
const detailContent = selected?.issue_id ? (
|
||||
// Key by issue_id (not inbox-item id): a new comment/reaction generates a
|
||||
// new inbox notification for the same issue, and the dedup helper picks the
|
||||
// newest one — keying on its id would remount IssueDetail on every event,
|
||||
// wiping the comment composer draft and resetting scroll position.
|
||||
<ErrorBoundary resetKeys={[selected.issue_id]}>
|
||||
const detailContent = detailSelected?.issue_id ? (
|
||||
// No `key` on IssueDetail: switching issues updates `issueId` in place and
|
||||
// lets React reconcile the heavy subtree (sidebar, pickers, layout) instead
|
||||
// of tearing it down and rebuilding it on every click — the full-page issue
|
||||
// route already drives IssueDetail this way. IssueDetail resets its own
|
||||
// per-issue state on issueId change (description/comment editors are keyed
|
||||
// by id internally, optional-prop + attachment state reset via effects).
|
||||
// ErrorBoundary still resets per issue so one issue's render error doesn't
|
||||
// stick to the next.
|
||||
<ErrorBoundary resetKeys={[detailSelected.issue_id]}>
|
||||
<IssueDetail
|
||||
key={selected.issue_id}
|
||||
issueId={selected.issue_id}
|
||||
issueId={detailSelected.issue_id}
|
||||
defaultSidebarOpen={false}
|
||||
layoutId="multica_inbox_issue_detail_layout"
|
||||
highlightCommentId={selected.details?.comment_id ?? undefined}
|
||||
highlightCommentId={detailSelected.details?.comment_id ?? undefined}
|
||||
onDelete={() => {
|
||||
// Issue deletion CASCADE-deletes the inbox item server-side, and the
|
||||
// issue:deleted WS event prunes it from the inbox cache. Just clear
|
||||
@@ -306,31 +320,31 @@ export function InboxPage() {
|
||||
setSelectedKey("");
|
||||
}}
|
||||
onDone={() => {
|
||||
handleArchive(selected.id);
|
||||
handleArchive(detailSelected.id);
|
||||
}}
|
||||
/>
|
||||
</ErrorBoundary>
|
||||
) : selected ? (
|
||||
) : detailSelected ? (
|
||||
<div className="p-6">
|
||||
<h2 className="text-lg font-semibold">{getInboxDisplayTitle(selected)}</h2>
|
||||
<h2 className="text-lg font-semibold">{getInboxDisplayTitle(detailSelected)}</h2>
|
||||
<p className="mt-1 text-sm text-muted-foreground">
|
||||
{typeLabels[selected.type]} · {timeAgo(selected.created_at)}
|
||||
{typeLabels[detailSelected.type]} · {timeAgo(detailSelected.created_at)}
|
||||
</p>
|
||||
{selected.body && (
|
||||
{detailSelected.body && (
|
||||
<div className="mt-4 whitespace-pre-wrap text-sm leading-relaxed text-foreground/80">
|
||||
{selected.body}
|
||||
{detailSelected.body}
|
||||
</div>
|
||||
)}
|
||||
{selected.type === "quick_create_failed" && selected.details?.original_prompt && (
|
||||
{detailSelected.type === "quick_create_failed" && detailSelected.details?.original_prompt && (
|
||||
<div className="mt-4 rounded-md border bg-muted/40 p-3">
|
||||
<p className="text-xs font-medium text-muted-foreground">
|
||||
{t(($) => $.detail.original_input)}
|
||||
</p>
|
||||
<p className="mt-1 whitespace-pre-wrap text-sm">{selected.details.original_prompt}</p>
|
||||
<p className="mt-1 whitespace-pre-wrap text-sm">{detailSelected.details.original_prompt}</p>
|
||||
</div>
|
||||
)}
|
||||
<div className="mt-4 flex gap-2">
|
||||
{selected.type === "quick_create_failed" && (
|
||||
{detailSelected.type === "quick_create_failed" && (
|
||||
<Button
|
||||
size="sm"
|
||||
onClick={() => {
|
||||
@@ -338,8 +352,8 @@ export function InboxPage() {
|
||||
// user can recover their input in the full editor instead of
|
||||
// retyping. The agent picker hint becomes the assignee
|
||||
// candidate (still editable).
|
||||
const prompt = selected.details?.original_prompt ?? "";
|
||||
const agentId = selected.details?.agent_id;
|
||||
const prompt = detailSelected.details?.original_prompt ?? "";
|
||||
const agentId = detailSelected.details?.agent_id;
|
||||
useIssueDraftStore.getState().setDraft({
|
||||
description: prompt,
|
||||
...(agentId
|
||||
@@ -355,7 +369,7 @@ export function InboxPage() {
|
||||
<Button
|
||||
variant="outline"
|
||||
size="sm"
|
||||
onClick={() => handleArchive(selected.id)}
|
||||
onClick={() => handleArchive(detailSelected.id)}
|
||||
>
|
||||
<Archive className="mr-1.5 h-3.5 w-3.5" />
|
||||
{t(($) => $.detail.archive)}
|
||||
@@ -388,8 +402,10 @@ export function InboxPage() {
|
||||
);
|
||||
}
|
||||
|
||||
// Mobile: show detail full-screen when an item is selected
|
||||
if (selected) {
|
||||
// Mobile: show detail full-screen when an item is selected. Gate on the
|
||||
// deferred selection so the screen swap and its content stay in lockstep
|
||||
// (no blank frame between switching screens and the detail resolving).
|
||||
if (detailSelected) {
|
||||
return (
|
||||
<div className="flex flex-1 flex-col min-h-0">
|
||||
<div className="flex h-12 shrink-0 items-center border-b px-2">
|
||||
|
||||
@@ -536,7 +536,12 @@ describe("IssueDetail (shared)", () => {
|
||||
expect(screen.getByDisplayValue("Implement authentication")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
expect(screen.getByDisplayValue("Add JWT auth to the backend")).toBeInTheDocument();
|
||||
// The description editor is deferred one animation frame past first paint
|
||||
// (useDeferredMount) to keep its Tiptap mount off the synchronous
|
||||
// issue-switch commit, so await it rather than asserting synchronously.
|
||||
expect(
|
||||
await screen.findByDisplayValue("Add JWT auth to the backend"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("opts the description editor into the unmount flush", async () => {
|
||||
@@ -552,6 +557,18 @@ describe("IssueDetail (shared)", () => {
|
||||
expect(description).toHaveAttribute("data-flush-on-unmount", "true");
|
||||
});
|
||||
|
||||
it("hydrates the bottom comment composer after first paint (deferred, not synchronous)", async () => {
|
||||
// The composer's Tiptap editor is mounted one animation frame past paint
|
||||
// (useDeferredMount) so it stays out of the synchronous issue-switch
|
||||
// commit. A static placeholder holds the spot meanwhile; await the real
|
||||
// editor (its placeholder textarea) to confirm it still hydrates.
|
||||
renderIssueDetail();
|
||||
|
||||
expect(
|
||||
await screen.findByPlaceholderText("Leave a comment..."),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("renders the issue title leaf as a link to the issue detail page", async () => {
|
||||
renderIssueDetail();
|
||||
|
||||
@@ -1212,6 +1229,48 @@ describe("IssueDetail (shared)", () => {
|
||||
).toContain("bg-[color-mix(in_srgb,var(--card)_95%,var(--brand)_5%)]");
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps a short highlighted timeline on the flat (non-virtualized) path", async () => {
|
||||
renderIssueDetailWithHighlight("comment-2");
|
||||
|
||||
await waitFor(() => {
|
||||
expect(document.getElementById("comment-comment-2")).not.toBeNull();
|
||||
});
|
||||
// Short list → flat render, no Virtuoso. (Flat render lands precisely.)
|
||||
expect(screen.queryByTestId("virtuoso-mock")).toBeNull();
|
||||
});
|
||||
|
||||
it("virtualizes a long highlighted timeline instead of mounting every comment", async () => {
|
||||
// Reproduces the inbox freeze: a comment-heavy issue opened with a
|
||||
// highlight target used to flat-render every CommentCard synchronously.
|
||||
// Past FLAT_TIMELINE_LIMIT it must hand off to Virtuoso (positioned on
|
||||
// the target) so the page doesn't freeze on entry.
|
||||
const longTimeline: TimelineEntry[] = Array.from({ length: 40 }, (_, i) => ({
|
||||
type: "comment",
|
||||
id: `c-${i}`,
|
||||
actor_type: "member",
|
||||
actor_id: "user-1",
|
||||
content: `Comment ${i}`,
|
||||
parent_id: null,
|
||||
created_at: `2026-01-18T00:${String(i).padStart(2, "0")}:00Z`,
|
||||
updated_at: `2026-01-18T00:${String(i).padStart(2, "0")}:00Z`,
|
||||
comment_type: "comment",
|
||||
})) as TimelineEntry[];
|
||||
mockApiObj.listTimeline.mockResolvedValue(longTimeline);
|
||||
|
||||
renderIssueDetailWithHighlight("c-20");
|
||||
|
||||
// Virtuoso path is taken (the mock renders a virtuoso-mock wrapper), and
|
||||
// the highlight tint still lands on the target row.
|
||||
await waitFor(() => {
|
||||
expect(screen.getByTestId("virtuoso-mock")).toBeInTheDocument();
|
||||
});
|
||||
await waitFor(() => {
|
||||
expect(
|
||||
document.getElementById("comment-c-20")?.querySelector(".ring-2"),
|
||||
).not.toBeNull();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
it("sends empty description when editor is cleared", async () => {
|
||||
|
||||
@@ -30,7 +30,7 @@ import { Button } from "@multica/ui/components/ui/button";
|
||||
import { ResizablePanelGroup, ResizablePanel, ResizableHandle } from "@multica/ui/components/ui/resizable";
|
||||
import { Sheet, SheetContent } from "@multica/ui/components/ui/sheet";
|
||||
import { useIsMobile } from "@multica/ui/hooks/use-mobile";
|
||||
import { ContentEditor, type ContentEditorRef, TitleEditor, useFileDropZone, FileDropOverlay } from "../../editor";
|
||||
import { ContentEditor, type ContentEditorRef, TitleEditor, ReadonlyContent, useFileDropZone, FileDropOverlay } from "../../editor";
|
||||
import { FileUploadButton } from "@multica/ui/components/common/file-upload-button";
|
||||
import {
|
||||
Tooltip,
|
||||
@@ -78,6 +78,7 @@ import { useRecentIssuesStore } from "@multica/core/issues/stores";
|
||||
import { useIssueSelectionStore } from "@multica/core/issues/stores/selection-store";
|
||||
import { BatchActionToolbar } from "./batch-action-toolbar";
|
||||
import { useIssueTimeline } from "../hooks/use-issue-timeline";
|
||||
import { useDeferredMount } from "../hooks/use-deferred-mount";
|
||||
import { useIssueReactions } from "../hooks/use-issue-reactions";
|
||||
import { useIssueSubscribers } from "../hooks/use-issue-subscribers";
|
||||
import { ReactionBar } from "@multica/ui/components/common/reaction-bar";
|
||||
@@ -433,6 +434,15 @@ function TimelineSkeleton() {
|
||||
// activities" line that expands in place.
|
||||
const LAST_ACTIVITY_BLOCK_VISIBLE_LIMIT = 8;
|
||||
|
||||
// Above this many timeline items, a deep-link/highlight target switches from
|
||||
// flat render to a Virtuoso list positioned on the target. Flat render mounts
|
||||
// every CommentCard (markdown + lowlight) synchronously, which is the
|
||||
// multi-second freeze on entering/leaving a comment-heavy issue from the inbox
|
||||
// (inbox notifications always carry a comment_id → highlight). Short timelines
|
||||
// stay flat: the precise scroll/center landing is cheap and pixel-accurate
|
||||
// there, and small lists never froze.
|
||||
const FLAT_TIMELINE_LIMIT = 30;
|
||||
|
||||
// Collapsible wrapper for an activity block. Older blocks default to a single
|
||||
// "N activities" summary line so the timeline isn't dominated by status /
|
||||
// priority / assignee churn; the trailing block stays expanded because it
|
||||
@@ -1061,6 +1071,12 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
|
||||
return items.findIndex((it) => it.id === rootId);
|
||||
}, [items, highlightCommentId, replyToRoot]);
|
||||
|
||||
// Render the highlighted timeline flat only while it is short enough that
|
||||
// mounting every CommentCard up front is cheap. Past the threshold we hand
|
||||
// the highlight off to Virtuoso (positioned on the target below) so we never
|
||||
// synchronously mount a long comment list — the inbox freeze fix.
|
||||
const flatTimeline = !!highlightCommentId && items.length <= FLAT_TIMELINE_LIMIT;
|
||||
|
||||
const {
|
||||
reactions: issueReactions,
|
||||
toggleReaction: handleToggleIssueReaction,
|
||||
@@ -1172,50 +1188,57 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
|
||||
}
|
||||
}
|
||||
|
||||
const el = document.getElementById(`comment-${highlightCommentId}`);
|
||||
const container = scrollContainerEl;
|
||||
if (!el || !container) return;
|
||||
|
||||
didHighlightRef.current = highlightCommentId;
|
||||
|
||||
// Center the target comment WITHIN its own scroll container by driving the
|
||||
// container's scrollTop directly — never native scrollIntoView. Native
|
||||
// scrollIntoView is spec'd to scroll EVERY scrollable ancestor: on a cold
|
||||
// mount where the timeline is still growing (streaming agent), the inner
|
||||
// scroller can't satisfy centering on its own, so the scroll propagates up
|
||||
// and moves the desktop shell's `overflow:hidden` wrapper — shoving the
|
||||
// whole page, header included, off the top with no scrollbar to recover,
|
||||
// until a resize reflows it (#3929). Scoping the scroll to `container`
|
||||
// keeps it contained; re-centering across frames lands the comment
|
||||
// precisely once async heights (markdown, code highlight, streamed replies)
|
||||
// settle, instead of leaning on the ancestor scroll the way native did.
|
||||
let rafId = 0;
|
||||
let frames = 0;
|
||||
let last = -1;
|
||||
const center = () => {
|
||||
const c = container.getBoundingClientRect();
|
||||
const e = el.getBoundingClientRect();
|
||||
const target = Math.max(
|
||||
0,
|
||||
container.scrollTop + (e.top - c.top) - (container.clientHeight - e.height) / 2,
|
||||
);
|
||||
container.scrollTop = target;
|
||||
// Content is still laying out → the centered offset keeps shifting; keep
|
||||
// re-centering until it stabilizes (within 1px) or we hit ~0.5s of frames.
|
||||
if (Math.abs(target - last) > 1 && ++frames < 30) {
|
||||
last = target;
|
||||
rafId = requestAnimationFrame(center);
|
||||
}
|
||||
};
|
||||
rafId = requestAnimationFrame(center);
|
||||
if (flatTimeline) {
|
||||
// Flat render: the target id is in the DOM. Center it WITHIN its own
|
||||
// scroll container by driving the container's scrollTop directly — never
|
||||
// native scrollIntoView. Native scrollIntoView is spec'd to scroll EVERY
|
||||
// scrollable ancestor: on a cold mount where the timeline is still
|
||||
// growing (streaming agent), the inner scroller can't satisfy centering
|
||||
// on its own, so the scroll propagates up and moves the desktop shell's
|
||||
// `overflow:hidden` wrapper — shoving the whole page, header included,
|
||||
// off the top with no scrollbar to recover, until a resize reflows it
|
||||
// (#3929). Scoping the scroll to `container` keeps it contained;
|
||||
// re-centering across frames lands the comment precisely once async
|
||||
// heights (markdown, code highlight, streamed replies) settle.
|
||||
const el = document.getElementById(`comment-${highlightCommentId}`);
|
||||
const container = scrollContainerEl;
|
||||
if (!el || !container) return;
|
||||
|
||||
didHighlightRef.current = highlightCommentId;
|
||||
|
||||
let frames = 0;
|
||||
let last = -1;
|
||||
const center = () => {
|
||||
const c = container.getBoundingClientRect();
|
||||
const e = el.getBoundingClientRect();
|
||||
const target = Math.max(
|
||||
0,
|
||||
container.scrollTop + (e.top - c.top) - (container.clientHeight - e.height) / 2,
|
||||
);
|
||||
container.scrollTop = target;
|
||||
// Content is still laying out → the centered offset keeps shifting;
|
||||
// keep re-centering until it stabilizes (within 1px) or ~0.5s of frames.
|
||||
if (Math.abs(target - last) > 1 && ++frames < 30) {
|
||||
last = target;
|
||||
rafId = requestAnimationFrame(center);
|
||||
}
|
||||
};
|
||||
rafId = requestAnimationFrame(center);
|
||||
} else {
|
||||
// Long timeline: Virtuoso positions itself on the target via
|
||||
// `initialTopMostItemIndex` (it owns the shared scroller, so driving
|
||||
// scrollTop here would fight it). We only apply the highlight tint.
|
||||
didHighlightRef.current = highlightCommentId;
|
||||
}
|
||||
|
||||
setHighlightedId(highlightCommentId);
|
||||
const fade = window.setTimeout(() => setHighlightedId(null), 2500);
|
||||
return () => {
|
||||
cancelAnimationFrame(rafId);
|
||||
if (rafId) cancelAnimationFrame(rafId);
|
||||
clearTimeout(fade);
|
||||
};
|
||||
}, [highlightCommentId, items, targetIdx, scrollContainerEl, replyToRoot, expandedResolved, timelineView, toggleResolvedExpand]);
|
||||
}, [highlightCommentId, items, targetIdx, scrollContainerEl, replyToRoot, expandedResolved, timelineView, toggleResolvedExpand, flatTimeline]);
|
||||
|
||||
// Cmd-F / Ctrl-F on a virtualized timeline only searches what's mounted in
|
||||
// the viewport — off-screen comments are invisible to browser find-in-page.
|
||||
@@ -1239,6 +1262,25 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
|
||||
}, [id, items.length, t]);
|
||||
|
||||
const descEditorRef = useRef<ContentEditorRef>(null);
|
||||
// Defer the two heavy Tiptap editors (description + bottom comment composer)
|
||||
// past first paint so a fresh ProseMirror EditorView is never created inside
|
||||
// the synchronous commit of an issue switch. Until they hydrate, the
|
||||
// description shows as read-only markdown and the composer shows a static
|
||||
// placeholder — both near-zero cost. The render-phase reset in
|
||||
// useDeferredMount re-arms this on every issueId change, including the inbox
|
||||
// pane (now reconciled in place, no remount) and the full-page route.
|
||||
const { ready: descEditorReady, mountNow: mountDescEditor } = useDeferredMount(id);
|
||||
const { ready: commentComposerReady } = useDeferredMount(id);
|
||||
// Only steal focus into the editor when the user actively clicked the
|
||||
// read-only placeholder — never on the automatic post-paint mount, which
|
||||
// would yank focus to the description on every issue switch.
|
||||
const descFocusOnMountRef = useRef(false);
|
||||
useEffect(() => {
|
||||
if (descEditorReady && descFocusOnMountRef.current) {
|
||||
descFocusOnMountRef.current = false;
|
||||
descEditorRef.current?.focus();
|
||||
}
|
||||
}, [descEditorReady]);
|
||||
const { isDragOver: descDragOver, dropZoneProps: descDropZoneProps } = useFileDropZone({
|
||||
onDrop: (files) => files.forEach((f) => descEditorRef.current?.uploadFile(f)),
|
||||
});
|
||||
@@ -1938,6 +1980,30 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
|
||||
)}
|
||||
|
||||
<div {...descDropZoneProps} className="relative mt-5 rounded-lg">
|
||||
{!descEditorReady ? (
|
||||
// Read-only stand-in shown for the one frame before the editor
|
||||
// hydrates (or until the user clicks in). Keeps the heavy Tiptap
|
||||
// mount off the synchronous issue-switch commit; a mousedown
|
||||
// upgrades it immediately so a quick click-to-edit still lands.
|
||||
<div
|
||||
className="min-h-[1.5rem] cursor-text"
|
||||
onMouseDown={() => {
|
||||
descFocusOnMountRef.current = true;
|
||||
mountDescEditor();
|
||||
}}
|
||||
>
|
||||
{issue.description?.trim() ? (
|
||||
<ReadonlyContent
|
||||
content={issue.description}
|
||||
attachments={descEditorAttachments}
|
||||
/>
|
||||
) : (
|
||||
<p className="text-sm text-muted-foreground">
|
||||
{t(($) => $.detail.desc_placeholder)}
|
||||
</p>
|
||||
)}
|
||||
</div>
|
||||
) : (
|
||||
<ContentEditor
|
||||
ref={descEditorRef}
|
||||
key={id}
|
||||
@@ -1975,6 +2041,7 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
|
||||
currentIssueId={id}
|
||||
attachments={descEditorAttachments}
|
||||
/>
|
||||
)}
|
||||
|
||||
<div className="flex items-center gap-1 mt-3">
|
||||
<ReactionBar
|
||||
@@ -2163,46 +2230,21 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
|
||||
{timelineLoading && timelineView.groups.length === 0 ? (
|
||||
<TimelineSkeleton />
|
||||
) : (
|
||||
// Two render modes:
|
||||
// - `highlightCommentId` set (came from inbox deep-link) →
|
||||
// render flat. Every comment mounts, every height is real,
|
||||
// the target id is in the DOM the instant the useEffect
|
||||
// above runs `scrollIntoView`. No virtualization estimate
|
||||
// errors, no spacer reflow drift. Pays cold-mount cost
|
||||
// proportional to items.length (markdown + lowlight per
|
||||
// comment), which is acceptable in the deep-link case —
|
||||
// the user has explicit intent to land on a specific item.
|
||||
// - otherwise → Virtuoso. Browsing mode, virtualization
|
||||
// wins on first-paint perf for long timelines.
|
||||
//
|
||||
// The split is deliberate: virtualization and "land precisely
|
||||
// on a target" have fundamentally opposed contracts (estimated
|
||||
// heights vs real heights). Trying to satisfy both in one
|
||||
// path is what produced the bug history this PR closes.
|
||||
!highlightCommentId ? (
|
||||
!scrollContainerEl ? (
|
||||
// Skeleton while the callback ref populates so the gap
|
||||
// between IssueDetail mount and Virtuoso mount doesn't
|
||||
// flash empty.
|
||||
<TimelineSkeleton />
|
||||
) : (
|
||||
<div className="mt-4">
|
||||
<Virtuoso
|
||||
key={`${wsId}:${id}`}
|
||||
customScrollParent={scrollContainerEl}
|
||||
data={items}
|
||||
increaseViewportBy={{ top: 800, bottom: 800 }}
|
||||
computeItemKey={(_i, item) => `${item.kind}:${item.id}`}
|
||||
skipAnimationFrameInResizeObserver
|
||||
// followOutput intentionally NOT set. Virtuoso treats
|
||||
// it as a sticky "is at bottom" flag and resets
|
||||
// scrollTop to maxScrollTop on every height-change
|
||||
// tick — issue-detail is document-shaped, not chat.
|
||||
itemContent={renderItem}
|
||||
/>
|
||||
</div>
|
||||
)
|
||||
) : (
|
||||
// Three render modes:
|
||||
// - SHORT highlighted timeline (`flatTimeline`) → render flat.
|
||||
// Every comment mounts, every height is real, the target id
|
||||
// is in the DOM the instant the deep-link effect above runs.
|
||||
// No virtualization estimate errors, no spacer reflow drift.
|
||||
// Cheap because the list is short.
|
||||
// - LONG highlighted timeline → Virtuoso positioned on the
|
||||
// target via `initialTopMostItemIndex`. Avoids synchronously
|
||||
// mounting a long comment list (markdown + lowlight per
|
||||
// CommentCard), which is the multi-second freeze on entering
|
||||
// a comment-heavy issue from the inbox. Landing is best-effort
|
||||
// (estimated heights) rather than pixel-perfect — the right
|
||||
// trade for not freezing the page.
|
||||
// - no highlight → Virtuoso browsing mode.
|
||||
flatTimeline ? (
|
||||
<div className="mt-4">
|
||||
{items.map((item, i) => (
|
||||
<Fragment key={`${item.kind}:${item.id}`}>
|
||||
@@ -2210,17 +2252,53 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
|
||||
</Fragment>
|
||||
))}
|
||||
</div>
|
||||
) : !scrollContainerEl ? (
|
||||
// Skeleton while the callback ref populates so the gap between
|
||||
// IssueDetail mount and Virtuoso mount doesn't flash empty.
|
||||
<TimelineSkeleton />
|
||||
) : (
|
||||
<div className="mt-4">
|
||||
<Virtuoso
|
||||
key={`${wsId}:${id}`}
|
||||
customScrollParent={scrollContainerEl}
|
||||
data={items}
|
||||
{...(highlightCommentId && targetIdx >= 0
|
||||
? { initialTopMostItemIndex: { index: targetIdx, align: "center" as const } }
|
||||
: {})}
|
||||
increaseViewportBy={{ top: 800, bottom: 800 }}
|
||||
computeItemKey={(_i, item) => `${item.kind}:${item.id}`}
|
||||
skipAnimationFrameInResizeObserver
|
||||
// followOutput intentionally NOT set. Virtuoso treats
|
||||
// it as a sticky "is at bottom" flag and resets
|
||||
// scrollTop to maxScrollTop on every height-change
|
||||
// tick — issue-detail is document-shaped, not chat.
|
||||
itemContent={renderItem}
|
||||
/>
|
||||
</div>
|
||||
)
|
||||
)}
|
||||
|
||||
{/* Bottom comment input — no avatar, full width */}
|
||||
<div className="mt-4">
|
||||
{/* key={id}: web's /issues/[id] route doesn't remount on
|
||||
issueId change, so without an explicit key the editor
|
||||
keeps the previous issue's in-memory content and the
|
||||
next keystroke would flush it into the new issue's
|
||||
draft key. */}
|
||||
<CommentInput key={id} issueId={id} onSubmit={submitComment} />
|
||||
{/* Deferred one frame past paint (commentComposerReady) so the
|
||||
composer's Tiptap editor isn't created inside the synchronous
|
||||
issue-switch commit; a static placeholder of similar height
|
||||
holds the spot meanwhile (it usually sits below the fold on
|
||||
comment-heavy issues anyway). No manual expand control — it
|
||||
hydrates on its own.
|
||||
key={id}: web's /issues/[id] route doesn't remount on issueId
|
||||
change, so without an explicit key the editor keeps the
|
||||
previous issue's in-memory content and the next keystroke
|
||||
would flush it into the new issue's draft key. */}
|
||||
{commentComposerReady ? (
|
||||
<CommentInput key={id} issueId={id} onSubmit={submitComment} />
|
||||
) : (
|
||||
<div className="relative flex flex-col rounded-lg bg-card pb-8 ring-1 ring-border">
|
||||
<div className="px-3 py-2 text-sm text-muted-foreground">
|
||||
{t(($) => $.comment.leave_comment_placeholder)}
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
100
packages/views/issues/hooks/use-deferred-mount.test.tsx
Normal file
100
packages/views/issues/hooks/use-deferred-mount.test.tsx
Normal file
@@ -0,0 +1,100 @@
|
||||
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
|
||||
import { act, render, renderHook } from "@testing-library/react";
|
||||
import { useDeferredMount } from "./use-deferred-mount";
|
||||
|
||||
// Drive requestAnimationFrame deterministically: queue callbacks and flush
|
||||
// them on demand so the test controls exactly when the deferred mount lands.
|
||||
let rafCallbacks: FrameRequestCallback[] = [];
|
||||
|
||||
beforeEach(() => {
|
||||
rafCallbacks = [];
|
||||
vi.stubGlobal("requestAnimationFrame", (cb: FrameRequestCallback) => {
|
||||
rafCallbacks.push(cb);
|
||||
return rafCallbacks.length;
|
||||
});
|
||||
vi.stubGlobal("cancelAnimationFrame", (id: number) => {
|
||||
rafCallbacks[id - 1] = () => {};
|
||||
});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllGlobals();
|
||||
});
|
||||
|
||||
function flushRaf() {
|
||||
const pending = rafCallbacks;
|
||||
rafCallbacks = [];
|
||||
act(() => {
|
||||
pending.forEach((cb) => cb(0));
|
||||
});
|
||||
}
|
||||
|
||||
describe("useDeferredMount", () => {
|
||||
it("starts not-ready and flips ready after the next animation frame", () => {
|
||||
const { result } = renderHook(() => useDeferredMount());
|
||||
|
||||
expect(result.current.ready).toBe(false);
|
||||
|
||||
flushRaf();
|
||||
|
||||
expect(result.current.ready).toBe(true);
|
||||
});
|
||||
|
||||
it("mountNow forces an immediate ready without waiting for the frame", () => {
|
||||
const { result } = renderHook(() => useDeferredMount());
|
||||
|
||||
expect(result.current.ready).toBe(false);
|
||||
|
||||
act(() => {
|
||||
result.current.mountNow();
|
||||
});
|
||||
|
||||
expect(result.current.ready).toBe(true);
|
||||
});
|
||||
|
||||
it("re-arms the deferral when resetKey changes", () => {
|
||||
const { result, rerender } = renderHook(
|
||||
({ key }: { key: string }) => useDeferredMount(key),
|
||||
{ initialProps: { key: "a" } },
|
||||
);
|
||||
|
||||
flushRaf();
|
||||
expect(result.current.ready).toBe(true);
|
||||
|
||||
rerender({ key: "b" });
|
||||
expect(result.current.ready).toBe(false);
|
||||
|
||||
flushRaf();
|
||||
expect(result.current.ready).toBe(true);
|
||||
});
|
||||
|
||||
it("never reports ready for the first render after a key change (no sync mount-then-unmount)", () => {
|
||||
// Captures the `ready` value on EVERY render, including the synchronous
|
||||
// render(s) React runs while a key change is reconciled — the window
|
||||
// renderHook's settled `result.current` hides. An effect-based reset would
|
||||
// leave this first post-change render ready=true (heavy child mounts, then
|
||||
// unmounts a tick later); the render-phase reset must keep it false.
|
||||
const readyLog: boolean[] = [];
|
||||
function Probe({ k }: { k: string }) {
|
||||
const { ready } = useDeferredMount(k);
|
||||
readyLog.push(ready);
|
||||
return <span>{ready ? "ready" : "pending"}</span>;
|
||||
}
|
||||
|
||||
const { rerender } = render(<Probe k="a" />);
|
||||
flushRaf();
|
||||
// Now settled ready=true for key "a".
|
||||
readyLog.length = 0;
|
||||
|
||||
act(() => {
|
||||
rerender(<Probe k="b" />);
|
||||
});
|
||||
|
||||
// Every render between the key change and the next frame must be pending.
|
||||
expect(readyLog.length).toBeGreaterThan(0);
|
||||
expect(readyLog.some((r) => r === true)).toBe(false);
|
||||
|
||||
flushRaf();
|
||||
expect(readyLog.at(-1)).toBe(true);
|
||||
});
|
||||
});
|
||||
56
packages/views/issues/hooks/use-deferred-mount.ts
Normal file
56
packages/views/issues/hooks/use-deferred-mount.ts
Normal file
@@ -0,0 +1,56 @@
|
||||
"use client";
|
||||
|
||||
import { useCallback, useEffect, useState } from "react";
|
||||
|
||||
/**
|
||||
* Defers mounting a heavy child until just after the first paint of the
|
||||
* surrounding tree. Returns `ready: false` on the initial commit (and on the
|
||||
* first commit after `resetKey` changes) and flips to `true` on the next
|
||||
* animation frame.
|
||||
*
|
||||
* The inbox detail pane remounts the whole `IssueDetail` subtree on every issue
|
||||
* switch (it is keyed by `issue_id`). Creating the description's Tiptap editor
|
||||
* inside that synchronous commit is one of the costliest pieces of work on the
|
||||
* critical switch path — a fresh ProseMirror `EditorView` with ~20 extensions.
|
||||
* Gating it on this hook keeps the editor out of the switch commit, so the new
|
||||
* issue's content paints first and the editor hydrates a frame later. UX is
|
||||
* unchanged; only the timing moves.
|
||||
*
|
||||
* `resetKey` re-arms the deferral when it changes. The reset is derived during
|
||||
* render (the tracked key lives in state and is reconciled in the render body),
|
||||
* NOT in an effect — so the very first render after a key change already
|
||||
* reports `ready: false`. That matters on routes where the host component is
|
||||
* NOT remounted on key change (e.g. the full-page issue route): an
|
||||
* effect-based reset would let the heavy child mount synchronously on that
|
||||
* first render and then immediately unmount, which is the opposite of the goal.
|
||||
*
|
||||
* `mountNow` forces an immediate mount, for when the user interacts with the
|
||||
* placeholder before the deferred frame lands.
|
||||
*/
|
||||
export function useDeferredMount(resetKey?: unknown): {
|
||||
ready: boolean;
|
||||
mountNow: () => void;
|
||||
} {
|
||||
const [ready, setReady] = useState(false);
|
||||
const [trackedKey, setTrackedKey] = useState(resetKey);
|
||||
|
||||
// Reconcile during render (React's "adjust state when a prop changes"
|
||||
// pattern): on a key change, reset the tracked state for subsequent renders.
|
||||
const keyChanged = trackedKey !== resetKey;
|
||||
if (keyChanged) {
|
||||
setTrackedKey(resetKey);
|
||||
setReady(false);
|
||||
}
|
||||
|
||||
useEffect(() => {
|
||||
const raf = requestAnimationFrame(() => setReady(true));
|
||||
return () => cancelAnimationFrame(raf);
|
||||
}, [resetKey]);
|
||||
|
||||
const mountNow = useCallback(() => setReady(true), []);
|
||||
|
||||
// On the render where the key just changed, `ready` still holds the previous
|
||||
// key's value. Derive the returned value from the comparison so even that
|
||||
// pass reports false — the heavy child is never mountable for a stale key.
|
||||
return { ready: keyChanged ? false : ready, mountNow };
|
||||
}
|
||||
Reference in New Issue
Block a user