diff --git a/packages/views/issues/components/comment-card.tsx b/packages/views/issues/components/comment-card.tsx index 13260437d..29d9fe072 100644 --- a/packages/views/issues/components/comment-card.tsx +++ b/packages/views/issues/components/comment-card.tsx @@ -663,7 +663,7 @@ function CommentCardImpl({ {/* Replies */} {allNestedReplies.map((reply) => ( -
+
(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>(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(() => { diff --git a/packages/views/issues/components/issue-detail.test.tsx b/packages/views/issues/components/issue-detail.test.tsx index 01cf11856..82d07840a 100644 --- a/packages/views/issues/components/issue-detail.test.tsx +++ b/packages/views/issues/components/issue-detail.test.tsx @@ -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((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(); diff --git a/packages/views/issues/components/issue-detail.tsx b/packages/views/issues/components/issue-detail.tsx index 3c10bf6f7..318dffa29 100644 --- a/packages/views/issues/components/issue-detail.tsx +++ b/packages/views/issues/components/issue-detail.tsx @@ -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(); + 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 ( -
+ // 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. +
+
- + {/* 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. */} +