highperfocused/multica
1
0
Fork 0
mirror of https://github.com/multica-ai/multica.git synced 2026-06-19 04:38:50 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(views): repair deep-link scroll and isolate comment drafts

Browse Source 
The first virtualization landing had three latent issues that runtime
testing on perf fixtures (10 → 5000 comments) exposed:

1. Deep-link landing position was wrong by ~380px on every issue.
   In customScrollParent mode Virtuoso computes scrollTop from the
   list's internal coordinate space only — it doesn't account for
   sibling content (title editor, description, sub-issues, agent
   card) sitting above the list inside the same scroll parent. The
   useEffect now uses Virtuoso scrollToIndex only to MOUNT the
   target into the DOM, then polls a `data-comment-id` anchor and
   delegates positioning to the browser's scrollIntoView, which
   honors getBoundingClientRect and lands accurately every time.

2. Scroll-up was being yanked back to the deep-link anchor on every
   ResizeObserver tick. Root cause was `followOutput="auto"`, which
   stays "stuck to bottom" once the deep-link lands there and resets
   scrollTop to maxScrollTop on each height change. Issue detail is
   document-shaped, not chat-shaped, so removing followOutput
   altogether is the right tradeoff. Likewise `initialTopMostItemIndex`
   acts as a persistent anchor in customScrollParent mode (Virtuoso
   #458) — dropped entirely and replaced with imperative scroll.
   `defaultItemHeight` is also dropped so Virtuoso probes real
   heights instead of estimating + correcting visually.

3. Reply-comment deep-links from the inbox would short-circuit
   because the reply id isn't in the flat items[] array. Added a
   replyToRoot map so deep-link falls back to the enclosing thread's
   root index, scrolls there, and lets the reply's own ring fire
   once the thread is in view.

Also fixes a latent cross-issue draft leak in `<CommentInput>`:
web's /issues/[id] route doesn't remount IssueDetail on issueId
change, so without an explicit `key={id}` the editor kept the
previous issue's in-memory content and the next keystroke would
flush it under the new issue's draft key. The same fix incidentally
repairs the pre-existing "submit composer from issue A while viewing
issue B" submit-target bug.

Highlight UX polish: bg-brand/5 was too faint to notice; ring upgraded
to ring-brand/60 as the sole signal. transition-colors didn't actually
animate ring/box-shadow — switched to transition-shadow duration-500
ease-out so highlight has visible fade in / fade out. Flash duration
3s → 4s. Polling failure now still sets highlight + warns so a manual
scroll to the target still flashes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Naiyuan Qing
2026-05-11 17:09:22 +08:00
parent cda456716f
commit 0d0d100ee0
4 changed files with 163 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
packages/views/issues/components/comment-card.tsx
View File

@@ -663,7 +663,7 @@ function CommentCardImpl({
{/* Replies */}
{allNestedReplies.map((reply) => (
<div key={reply.id} id={`comment-${reply.id}`} className={cn("border-t border-border/50 px-4 transition-colors duration-700", highlightedCommentId === reply.id && "bg-brand/5")}>
<div key={reply.id} data-comment-id={reply.id} className={cn("border-t border-border/50 px-4 transition-colors duration-700", highlightedCommentId === reply.id && "bg-brand/5")}>
<CommentRow
issueId={issueId}
entry={reply}

15
packages/views/issues/components/comment-input.tsx
View File

@@ -21,10 +21,13 @@ interface CommentInputProps {
function CommentInput({ issueId, onSubmit }: CommentInputProps) {
const { t } = useT("issues");
const editorRef = useRef<ContentEditorRef>(null);
// Initial isEmpty reflects whether a hydrated draft has content; otherwise
// the submit button would be disabled even though the editor shows text.
const initialDraftPreview = useCommentDraftStore.getState().getDraft(`new:${issueId}`);
const [isEmpty, setIsEmpty] = useState(!initialDraftPreview?.trim());
// Read the persisted draft once on mount. ContentEditor only honors
// `defaultValue` at mount time, so this snapshot drives both the editor's
// initial content and the submit-button enable state — without this the
// button would be disabled even though the editor visibly contains text.
const draftKey = `new:${issueId}` as const;
const initialDraft = useCommentDraftStore.getState().getDraft(draftKey);
const [isEmpty, setIsEmpty] = useState(() => !initialDraft?.trim());
const [submitting, setSubmitting] = useState(false);
const [isExpanded, setIsExpanded] = useState(false);
const uploadMapRef = useRef<Map<string, string>>(new Map());
@@ -33,12 +36,10 @@ function CommentInput({ issueId, onSubmit }: CommentInputProps) {
onDrop: (files) => files.forEach((f) => editorRef.current?.uploadFile(f)),
});
// Draft persistence. Hydrate from store on mount via `defaultValue`
// Draft persistence. Hydrate from store on mount via `defaultValue` above
// (ContentEditorRef has no setContent, so this is the only injection point).
// Flush on every onUpdate (debounced upstream) + visibilitychange/pagehide
// so tab close / mobile background doesn't lose work. Cleared on submit.
const draftKey = `new:${issueId}` as const;
const initialDraft = useCommentDraftStore.getState().getDraft(draftKey);
const setDraft = useCommentDraftStore((s) => s.setDraft);
const clearDraft = useCommentDraftStore((s) => s.clearDraft);
useEffect(() => {

37
packages/views/issues/components/issue-detail.test.tsx
View File

@@ -591,28 +591,30 @@ describe("IssueDetail (shared)", () => {
it("scrolls to the highlighted comment after both issue and timeline finish loading", async () => {
renderIssueDetailWithHighlight("comment-2");
// Wait until the comment DOM is rendered.
// Under virtualization the DOM anchor is a `data-comment-id` attribute
// on the item wrapper; we no longer rely on element id="comment-...".
await waitFor(() => {
expect(document.getElementById("comment-comment-2")).not.toBeNull();
expect(
document.querySelector('[data-comment-id="comment-2"]'),
).not.toBeNull();
});
// requestAnimationFrame defers the actual scrollIntoView call.
// The deep-link useEffect polls until the target mounts, then calls
// scrollIntoView on the wrapper element.
await waitFor(() => {
expect(scrollIntoViewSpy).toHaveBeenCalled();
});
const callContext = scrollIntoViewSpy.mock.contexts[0] as HTMLElement;
expect(callContext.id).toBe("comment-comment-2");
expect(callContext.getAttribute("data-comment-id")).toBe("comment-2");
});
it("still scrolls when the timeline is ready before the issue (regression for inbox click)", async () => {
// Reproduces the inbox-click race: timeline data is already in the cache
// (resolved first), but the issue is still pending — so the first render
// sees timeline.length=2 alongside loading=true (skeleton still showing,
// no comment DOM). The scroll effect fires once, fails to find the
// element, and must re-fire when `loading` flips to false. Without
// `loading` in the dep list, that second fire never happens and the
// user lands at the top of the issue.
// Reproduces the inbox-click race: timeline data is in the cache before
// the issue resolves. While `loading` is true the timeline skeleton is
// shown and no comment wrapper is mounted. The deep-link polling loop
// must keep retrying (up to ~320ms of rAFs) until the issue resolves
// and the target's `data-comment-id` appears in the DOM.
let resolveIssue: (value: Issue) => void = () => {};
const issuePromise = new Promise<Issue>((resolve) => {
resolveIssue = resolve;
@@ -621,18 +623,17 @@ describe("IssueDetail (shared)", () => {
renderIssueDetailWithHighlight("comment-2", "issue-1", { seedTimeline: true });
// The skeleton is still showing (issue pending), so even though
// timeline.length>0 the comment DOM is not mounted and no scroll
// can happen yet.
expect(document.getElementById("comment-comment-2")).toBeNull();
expect(
document.querySelector('[data-comment-id="comment-2"]'),
).toBeNull();
expect(scrollIntoViewSpy).not.toHaveBeenCalled();
// Now the issue resolves — comment elements mount, the effect re-runs
// because `loading` is part of its deps, and the scroll fires.
resolveIssue(mockIssue);
await waitFor(() => {
expect(document.getElementById("comment-comment-2")).not.toBeNull();
expect(
document.querySelector('[data-comment-id="comment-2"]'),
).not.toBeNull();
});
await waitFor(() => {
expect(scrollIntoViewSpy).toHaveBeenCalled();

177
packages/views/issues/components/issue-detail.tsx
View File

@@ -580,14 +580,30 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
[timelineView.groups, expandedResolved],
);
// Deep-link target index: when the inbox passes `highlightCommentId`, find
// its position in the flat list so Virtuoso can land on it initially via
// `initialTopMostItemIndex` (rough landing) and we can refine via imperative
// scrollToIndex in a useEffect (precision pass after ResizeObserver).
// Map of reply-comment id → root-comment id, so a deep-link to a reply
// (which lives inside a CommentCard, not in the flat items array) can fall
// back to scrolling the root thread into view. Without this, an inbox
// notification on a reply would land at items[-1] and short-circuit.
const replyToRoot = useMemo(() => {
const map = new Map<string, string>();
for (const [rootId, replies] of timelineView.threadReplies) {
for (const reply of replies) {
map.set(reply.id, rootId);
}
}
return map;
}, [timelineView.threadReplies]);
// Deep-link target index in the flat items array. For root comments this is
// a direct findIndex hit; for reply ids we look up the enclosing root.
const targetIdx = useMemo(() => {
if (!highlightCommentId) return -1;
return items.findIndex((it) => it.id === highlightCommentId);
}, [items, highlightCommentId]);
const direct = items.findIndex((it) => it.id === highlightCommentId);
if (direct >= 0) return direct;
const rootId = replyToRoot.get(highlightCommentId);
if (!rootId) return -1;
return items.findIndex((it) => it.id === rootId);
}, [items, highlightCommentId, replyToRoot]);
const {
reactions: issueReactions,
@@ -666,25 +682,94 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
useEffect(() => {
if (!highlightCommentId || items.length === 0 || targetIdx < 0) return;
if (didHighlightRef.current === highlightCommentId) return;
didHighlightRef.current = highlightCommentId;
let inner = 0;
const outer = requestAnimationFrame(() => {
inner = requestAnimationFrame(() => {
virtuosoRef.current?.scrollToIndex({
index: targetIdx,
align: "center",
behavior: "auto",
});
setHighlightedId(highlightCommentId);
});
});
const timer = setTimeout(() => setHighlightedId(null), 3000);
return () => {
cancelAnimationFrame(outer);
cancelAnimationFrame(inner);
clearTimeout(timer);
// Strategy: Virtuoso's scrollToIndex is asynchronous — it queues an
// internal state change that mounts the target a few frames later. The
// target also isn't in the DOM until Virtuoso settles. So:
// 1. Kick off Virtuoso to start moving toward the target
// 2. Poll for the target's DOM element (max ~20 frames ≈ 320ms)
// 3. Once found, use the browser's scrollIntoView which correctly
// accounts for all sibling content above the list
// 4. After Virtuoso fully settles, scrollIntoView one more time —
// Virtuoso may have overwritten our scroll position
const rafIds: number[] = [];
const timeoutIds: number[] = [];
const cancelled = { value: false };
const findTarget = () => {
// First try the exact id — works for root comments and for replies
// whose enclosing thread is currently expanded.
const direct = scrollContainerEl?.querySelector(
`[data-comment-id="${CSS.escape(highlightCommentId)}"]`,
) as HTMLElement | null;
if (direct) return direct;
// Reply in a collapsed thread: fall back to the root wrapper so we at
// least scroll the card containing the target into view.
const rootId = replyToRoot.get(highlightCommentId);
if (rootId) {
return scrollContainerEl?.querySelector(
`[data-comment-id="${CSS.escape(rootId)}"]`,
) as HTMLElement | null;
}
return null;
};
}, [highlightCommentId, items.length, targetIdx]);
const performScroll = () => {
const el = findTarget();
if (!el) return false;
el.scrollIntoView({ block: "center", behavior: "auto" });
return true;
};
const pollMount = (attemptsLeft: number) => {
if (cancelled.value) return;
if (performScroll()) {
setHighlightedId(highlightCommentId);
didHighlightRef.current = highlightCommentId;
// Done. No reanchor — polling already waits until Virtuoso has
// settled enough to mount the target, so scrollIntoView IS the
// last write. Adding a delayed re-scroll would yank the user back
// if they scrolled away in the meantime.
return;
}
if (attemptsLeft <= 0) {
// Virtuoso didn't mount the target within ~320ms. Still set the
// highlight so when the user manually scrolls there, the card
// flashes. Mark handled so subsequent renders don't keep polling.
// eslint-disable-next-line no-console
console.warn(
`[deep-link] target ${highlightCommentId} did not mount in time; highlight set without scroll`,
);
setHighlightedId(highlightCommentId);
didHighlightRef.current = highlightCommentId;
return;
}
const id = requestAnimationFrame(() => pollMount(attemptsLeft - 1));
rafIds.push(id);
};
const start = requestAnimationFrame(() => {
if (cancelled.value) return;
// Step 1: kick Virtuoso so it begins mounting target index.
virtuosoRef.current?.scrollToIndex({
index: targetIdx,
align: "center",
behavior: "auto",
});
// Step 2-4: start polling for the DOM element.
pollMount(20);
});
rafIds.push(start);
const flash = window.setTimeout(() => setHighlightedId(null), 2000);
timeoutIds.push(flash);
return () => {
cancelled.value = true;
for (const id of rafIds) cancelAnimationFrame(id);
for (const id of timeoutIds) clearTimeout(id);
};
}, [highlightCommentId, items.length, targetIdx, scrollContainerEl]);
// 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.
@@ -1307,28 +1392,31 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
ref={virtuosoRef}
customScrollParent={scrollContainerEl}
data={items}
defaultItemHeight={120}
increaseViewportBy={{ top: 800, bottom: 800 }}
computeItemKey={(_i, item) => `${item.kind}:${item.id}`}
skipAnimationFrameInResizeObserver
// followOutput="auto": only stick to bottom when the user is
// already there. If they scrolled up to read history, a new
// WS comment won't yank them back down. Matches Slack/Discord.
followOutput="auto"
// Default to index 0 (top) when there's no deep-link target,
// so the surrounding page scrolls naturally from the top
// and the user sees title/description first, like the
// original .map. Do NOT use `undefined` here — Virtuoso
// reads .align off the prop directly and crashes on undefined.
initialTopMostItemIndex={
targetIdx >= 0
? { index: targetIdx, align: "center" }
: 0
}
// followOutput intentionally NOT set. Virtuoso treats it as
// a sticky "is at bottom" flag and resets scrollTop to
// maxScrollTop on every ResizeObserver / height-change tick
// — this is what was yanking the user back to scrollTop=299
// whenever they tried to scroll up after a deep-link
// landed on the last item. Issue-detail is document-shaped
// (not a chat), so auto-follow on new comments is not
// critical; users can scroll to bottom themselves.
// Intentionally NOT passing `initialTopMostItemIndex`.
// In customScrollParent mode Virtuoso treats this prop as
// a persistent anchor and resets scrollTop whenever the
// list height changes (ResizeObserver firing on real-card
// measurement, etc.) — which fights against the user when
// they scroll up. Deep-link landing is handled imperatively
// by the useEffect above (scrollToIndex + scrollIntoView).
itemContent={(_i, item) => {
if (item.kind === "resolved-bar") {
return (
<div className="pb-3">
// data-comment-id is the anchor for inbox deep-link;
// see the deep-link useEffect for how scrollIntoView
// finds it after Virtuoso mounts the item.
<div className="pb-3" data-comment-id={item.id}>
<ResolvedThreadBar
entry={item.entry}
replies={timelineView.threadReplies.get(item.id) ?? EMPTY_REPLIES}
@@ -1340,7 +1428,7 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
if (item.kind === "comment") {
const isResolved = !!item.entry.resolved_at;
return (
<div className="pb-3">
<div className="pb-3" data-comment-id={item.id}>
<CommentCard
issueId={id}
entry={item.entry}
@@ -1418,7 +1506,12 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr
{/* Bottom comment input — no avatar, full width */}
<div className="mt-4">
<CommentInput issueId={id} onSubmit={submitComment} />
{/* 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} />
</div>
</div>
</div>