From 0810188dac5d439551deb1c1d9982363ccf34372 Mon Sep 17 00:00:00 2001 From: "Md.Sadiq" Date: Sat, 7 Nov 2026 22:46:00 +0530 Subject: [PATCH 1/8] fix(web): comment links open the pull request panel at the linked comment A link to a pull request comment (#issuecomment-, #pullrequestreview-, #discussion_r, /files#r, GitLab #note_) opened the PR in the panel but dropped the fragment, so the panel showed the top of the PR. The link handler now hands the panel the clicked link as a one-shot revealUrl on the surface. The panel finds the comment in its loaded conversation, switches to Summary, pages it in, shows it past the comment filter, opens a collapsed section or resolved thread around it without animating, then holds it below the Comments heading while the content settles and marks it. The reveal is cleared once shown, so returning to the tab or reopening a closed one does not scroll again. Fixes #32547 --- .../chat/RightPanelSurfaceContent.tsx | 4 + .../pullRequest/PullRequestDetailPanel.tsx | 44 +++++ .../pullRequest/PullRequestSummaryTab.tsx | 150 ++++++++++++++++-- .../pullRequest/PullRequestsPage.tsx | 7 + .../pullRequestDetail.logic.test.ts | 24 +++ .../pullRequest/pullRequestDetail.logic.ts | 51 ++++-- apps/web/src/lib/openPullRequestLink.ts | 2 + apps/web/src/rightPanelStore.test.ts | 22 +++ apps/web/src/rightPanelStore.ts | 42 +++-- 9 files changed, 316 insertions(+), 30 deletions(-) diff --git a/apps/web/src/components/chat/RightPanelSurfaceContent.tsx b/apps/web/src/components/chat/RightPanelSurfaceContent.tsx index f0d8afa5f319c..02f3f0d07779a 100644 --- a/apps/web/src/components/chat/RightPanelSurfaceContent.tsx +++ b/apps/web/src/components/chat/RightPanelSurfaceContent.tsx @@ -153,6 +153,10 @@ export function RightPanelSurfaceContent(props: { onTabChange={(tab) => { useRightPanelStore.getState().setPullRequestDetailTab(threadRef, surface.id, tab); }} + revealUrl={surface.revealUrl} + onRevealSettled={() => { + useRightPanelStore.getState().settlePullRequestReveal(threadRef, surface.id); + }} context={pullRequestPanelContext( { projectId: pullRequest.projectId, diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 9d0f6193f7e3f..625cf94582505 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -126,6 +126,8 @@ import { isStackedPullRequestBase, pullRequestActionNeedsHostRefresh, pullRequestCheckoutCommand, + pullRequestCommentLinkTarget, + pullRequestConversationItems, pullRequestFindingKey, pullRequestHandoffLabels, PULL_REQUEST_MERGE_METHOD_LABELS, @@ -273,6 +275,8 @@ export function PullRequestDetailPanel({ onGuideShown, initialTab = "summary", onTabChange, + revealUrl, + onRevealSettled, }: { environmentId: EnvironmentId; shortcutsEnabled: boolean; @@ -299,6 +303,9 @@ export function PullRequestDetailPanel({ onGuideShown?: (() => void) | undefined; initialTab?: PullRequestDetailTab; onTabChange?: ((tab: PullRequestDetailTab) => void) | undefined; + /** A link that may name one of this pull request's comments, waiting to be shown. */ + revealUrl?: string | undefined; + onRevealSettled?: (() => void) | undefined; }) { const environmentConfigs = useServerConfigs(); const projects = useProjects(); @@ -356,6 +363,15 @@ export function PullRequestDetailPanel({ detail, } = usePullRequestDetail(environmentId, reference, matchingListEntry); const [selectedTab, setTab] = useState(initialTab); + const revealCommentId = + revealUrl === undefined || detail === null + ? null + : (pullRequestCommentLinkTarget( + pullRequestConversationItems(detail.comments, detail.reviewThreads), + revealUrl, + )?.commentId ?? null); + // A comment link shows the conversation holding it. + if (revealCommentId !== null && !tabOnPage && selectedTab !== "summary") setTab("summary"); const reviewGuidesEnabled = useEnvironmentSettings( environmentId, (settings) => settings.reviewGuidesEnabled, @@ -561,6 +577,32 @@ export function PullRequestDetailPanel({ : "Checking stack membership…"; const activityPending = activityQuery.isPending && activity === null; const activityError = activity === null ? activityQuery.error : null; + // Loaded activity without the comment, keyed by load so a refetch is checked again. + const revealMissedAt = + revealUrl !== undefined && + revealCommentId === null && + detail !== null && + !activityQuery.isPending + ? activityQuery.dataUpdatedAt + : null; + const revealRefreshedFor = useRef(null); + const showRevealedComment = useEffectEvent(() => { + if (revealCommentId === null) { + // Cached activity can predate the comment, so load it once more before giving up. + if (revealUrl !== undefined && revealRefreshedFor.current !== revealUrl) { + revealRefreshedFor.current = revealUrl; + return activityQuery.refresh(); + } + return onRevealSettled?.(); + } + // selectTab's store writes; the local tab already switched during render. + if (tabOnPage) useGuideRunStore.getState().showPageDetailTab(guideKey, "summary"); + onTabChange?.("summary"); + }); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- the tab and the pending link live in stores outside the panel + useEffect(() => { + if (revealCommentId !== null || revealMissedAt !== null) showRevealedComment(); + }, [revealCommentId, revealMissedAt]); const refreshDetail = () => { detailQuery.refresh(); activityQuery.refresh(); @@ -1751,6 +1793,8 @@ export function PullRequestDetailPanel({ reference={reference} detail={detail} activityPending={activityPending} + revealCommentId={tab === "summary" ? revealCommentId : null} + {...(onRevealSettled ? { onCommentRevealed: onRevealSettled } : {})} checksStale={checksStale} activityError={activityError} pendingFinding={handoff} diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index b98a48a817b0e..029b01b30ea26 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -29,6 +29,7 @@ import { pullRequestEnvironment } from "~/state/pullRequests"; import { useLocalStorage } from "~/hooks/useLocalStorage"; import { cn } from "~/lib/utils"; import { useOpenLink } from "~/browser/useOpenLink"; +import { observeResize } from "~/lib/observeResize"; import { RelativeTime } from "~/components/RelativeTime"; import { Button } from "../ui/button"; @@ -260,6 +261,8 @@ function CollapsedComment({ replies = null, moreReplies = null, onAddToChat, + revealCount = 0, + articleProps, }: { comment: PullRequestComment; editing: CommentEditing; @@ -272,12 +275,27 @@ function CollapsedComment({ replies?: ReactNode; moreReplies?: ReactNode; onAddToChat?: (() => void) | undefined; + /** Opens the conversation each time a link reveals a comment in it. */ + revealCount?: number; + articleProps?: LinkedArticleProps | undefined; }) { - const [open, setOpen] = useState(false); + // A reveal opens at once: the opening animation would move the comment after it is scrolled to. + const [open, setOpen] = useState(revealCount > 0 && "revealed"); + const [seenRevealCount, setSeenRevealCount] = useState(revealCount); + if (revealCount !== seenRevealCount) { + setSeenRevealCount(revealCount); + if (revealCount > 0) setOpen("revealed"); + } const statusTriggerRef = useRef(null); return ( - -
+ +
@@ -316,7 +334,7 @@ function CollapsedComment({ ) : null}
- + {open ? (
{body === null && !editing.canEdit(comment) ? null : ( @@ -332,6 +350,50 @@ function CollapsedComment({ ); } +interface LinkedArticleProps { + readonly ref: (node: HTMLElement | null) => void; + readonly tabIndex: -1; + readonly "data-linked-comment": true; + readonly className: string; +} + +const USER_SCROLL_INTENT = ["wheel", "touchstart", "pointerdown", "keydown"] as const; + +/** + * Holds a linked comment just below the sticky Comments heading while the conversations around it + * open and their markdown loads, until the user scrolls or another comment is linked. Only the + * summary scroller moves; scrollIntoView would also scroll the panel's clipped ancestors. + */ +function holdCommentInView(node: HTMLElement) { + const scroller = node.closest("[data-pull-request-summary-scroll]"); + if (!scroller) return; + const controller = new AbortController(); + const stop = () => { + unobserve(); + controller.abort(); + }; + const align = () => { + if (!node.isConnected || !node.hasAttribute("data-linked-comment")) return stop(); + const heading = node.closest("[data-pull-request-summary-section]")?.firstElementChild; + const headingHeight = heading?.getBoundingClientRect().height ?? 0; + scroller.scrollTop += + node.getBoundingClientRect().top - scroller.getBoundingClientRect().top - headingHeight - 8; + }; + const unobserve = observeResize([...scroller.children], align); + for (const type of USER_SCROLL_INTENT) { + scroller.addEventListener(type, stop, { passive: true, signal: controller.signal }); + } + align(); + // Activity can load after the click; by then the user may be typing elsewhere. + const active = document.activeElement; + if ( + !(active instanceof HTMLElement) || + !(active.isContentEditable || active.matches("input, textarea, select")) + ) { + node.focus({ preventScroll: true }); + } +} + function MetaRow({ icon, label, @@ -356,16 +418,25 @@ function Section({ title, defaultOpen = true, keepMounted = false, + revealCount = 0, actions, children, }: { title: string; defaultOpen?: boolean; keepMounted?: boolean; + /** Reopens the section each time a link reveals something in it. */ + revealCount?: number; actions?: ReactNode; children: ReactNode; }) { - const [open, setOpen] = useState(defaultOpen); + // Opened by a reveal without animating, like a revealed conversation. + const [open, setOpen] = useState(defaultOpen); + const [seenRevealCount, setSeenRevealCount] = useState(revealCount); + if (revealCount !== seenRevealCount) { + setSeenRevealCount(revealCount); + setOpen("revealed"); + } const headingRef = useRef(null); const setOpenWithScrollAnchor = (nextOpen: boolean) => { if (!nextOpen) { @@ -387,7 +458,7 @@ function Section({ }; return ( } data-pull-request-summary-section @@ -408,7 +479,7 @@ function Section({ {actions}
- +
{children}
 @@ -431,6 +502,8 @@ export function PullRequestSummaryTab({ checksStale = false, activityError, pendingFinding, + revealCommentId = null, + onCommentRevealed, fixFindingLabel = "Fix in a thread", fixCheckLabel = "Fix", onFixFinding, @@ -447,6 +520,9 @@ export function PullRequestSummaryTab({ checksStale?: boolean; activityError: string | null; pendingFinding?: string | null; + /** A linked comment to scroll to and mark, until onCommentRevealed reports it shown. */ + revealCommentId?: string | null; + onCommentRevealed?: () => void; fixFindingLabel?: string; fixCheckLabel?: string; onFixFinding?: (finding: PullRequestFinding) => void; @@ -464,7 +540,23 @@ export function PullRequestSummaryTab({ CommentFilterSchema, ); const items = pullRequestConversationItems(detail.comments, detail.reviewThreads); - const filteredItems = filterPullRequestConversationItems(items, commentFilter); + // The linked comment stays marked after it is shown, until another link replaces it. + const [linked, setLinked] = useState<{ + readonly commentId: string; + readonly revealCount: number; + readonly scrolled: boolean; + } | null>(null); + const [seenRevealCommentId, setSeenRevealCommentId] = useState(null); + const linkedCommentId = revealCommentId ?? linked?.commentId ?? null; + const linkedItem = + linkedCommentId === null + ? undefined + : items.find((item) => + pullRequestConversationItemComments(item).some( + (comment) => comment.id === linkedCommentId, + ), + ); + const filteredItems = filterPullRequestConversationItems(items, commentFilter, linkedItem); const commentCount = items.reduce( (count, item) => count + pullRequestConversationItemComments(item).length, 0, @@ -475,6 +567,38 @@ export function PullRequestSummaryTab({ (count, item) => count + pullRequestConversationItemComments(item).length, 0, ); + if (revealCommentId !== seenRevealCommentId) { + setSeenRevealCommentId(revealCommentId); + if (revealCommentId !== null && linkedItem !== undefined) { + setLinked({ + commentId: revealCommentId, + revealCount: (linked?.revealCount ?? 0) + 1, + scrolled: false, + }); + // Page in whole pages, so the older-conversations button keeps its usual step. + const fromNewest = filteredItems.length - filteredItems.indexOf(linkedItem); + if (fromNewest > shownConversations) { + setShown({ + url: detail.url, + count: Math.ceil(fromNewest / COMMENT_PAGE) * COMMENT_PAGE, + }); + } + } + } + const linkedArticleProps = (comment: PullRequestComment): LinkedArticleProps | undefined => + linked?.commentId === comment.id + ? { + ref: (node) => { + if (node === null || linked.scrolled) return; + holdCommentInView(node); + setLinked({ ...linked, scrolled: true }); + onCommentRevealed?.(); + }, + tabIndex: -1, + "data-linked-comment": true, + className: "border-primary/60 ring-1 ring-primary/40 outline-none", + } + : undefined; const { shown: recentItems, hidden: hiddenConversationCount } = pagePullRequestConversationItems( filteredItems, shownConversations, @@ -643,10 +767,15 @@ export function PullRequestSummaryTab({ : { kind: "thread", thread }; // A reply goes with its conversation, from the card that heads it. const chat = reply ? null : chatSubject(comment, thread); + const articleProps = linkedArticleProps(comment); return (
@@ -719,6 +848,8 @@ export function PullRequestSummaryTab({ moreReplies={moreReplies} onAddToChat={onAddToChat && chat !== null ? () => onAddToChat(chat) : undefined} reactionBar={reactionBarFor(comment)} + revealCount={item === linkedItem ? (linked?.revealCount ?? 0) : 0} + articleProps={linkedArticleProps(comment)} /> ); } @@ -936,6 +1067,7 @@ export function PullRequestSummaryTab({
diff --git a/apps/web/src/components/pullRequest/PullRequestsPage.tsx b/apps/web/src/components/pullRequest/PullRequestsPage.tsx index 2af8383bebcd4..a1e052a3e2956 100644 --- a/apps/web/src/components/pullRequest/PullRequestsPage.tsx +++ b/apps/web/src/components/pullRequest/PullRequestsPage.tsx @@ -1551,6 +1551,13 @@ export function PullRequestsPage() { } refreshToken={detailRefreshToken} tabOnPage + revealUrl={renderedPullRequestSurface.revealUrl} + onRevealSettled={() => { + if (rightPanelRef === null) return; + useRightPanelStore + .getState() + .settlePullRequestReveal(rightPanelRef, renderedPullRequestSurface.id); + }} onActed={(action, phase = "done", reply) => { // From every row held, not the ones on screen: a pull request closed from an open // list has left the screen, and reopening it has to find it anyway. diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index d23c4fdfae43a..5eaed1aceb527 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -35,6 +35,7 @@ import { groupPullRequestTimelineConversations, filterPullRequestConversationItems, pagePullRequestConversationItems, + pullRequestCommentLinkTarget, pullRequestConversationItemComments, pullRequestConversationItems, appendHandoff, @@ -577,6 +578,29 @@ describe("review thread conversations", () => { ); }); + it("finds the conversation a comment link names and keeps it past the filter", () => { + const botOnly = { ...comment("bot-only", 7), author: bot }; + const items = pullRequestConversationItems([...comments, botOnly], [thread]); + expect( + pullRequestCommentLinkTarget(items, "https://example.test/pull/1/files?w=1#reply"), + ).toEqual({ item: items[0], commentId: "reply" }); + const github = { ...reply, url: "https://github.com/o/r/pull/1#discussion_r42" }; + const githubItems = pullRequestConversationItems([github], []); + expect( + pullRequestCommentLinkTarget(githubItems, "https://github.com/o/r/pull/1/files#r42"), + ).toEqual({ item: githubItems[0], commentId: "reply" }); + for (const url of ["https://example.test/pull/1", "https://example.test/pull/1#gone", "#"]) { + expect(pullRequestCommentLinkTarget(items, url), url).toBeNull(); + } + const linked = pullRequestCommentLinkTarget(items, "https://example.test/pull/1#bot-only"); + expect(linked?.commentId).toBe("bot-only"); + expect( + filterPullRequestConversationItems(items, { bots: false, resolved: false }, linked?.item).map( + (item) => item.id, + ), + ).toEqual(["thread", "c1", "bot-only"]); + }); + it("keeps bot review verdicts when bot comments are hidden", () => { const review = (id: string, reviewState: string) => ({ ...comment(id, 9), diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index bcc46b63a0476..baa8fc10df872 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -628,21 +628,50 @@ function isPullRequestVerdict(reviewState: string | null): boolean { return outcome === "approved" || outcome === "changes-requested"; } +// GitHub's Files view names a review comment `#r1`; its conversation names it `#discussion_r1`. +const commentAnchor = (url: string) => + URL.parse(url)?.hash.replace(/^#r(?=\d+$)/u, "#discussion_r"); + +/** + * The conversation holding the comment a link points at. Hosts name a comment in the link's + * fragment (`#issuecomment-1`, `#note_1`), so a link to the pull request itself finds none. + */ +export function pullRequestCommentLinkTarget( + items: ReadonlyArray, + url: string, +): { readonly item: PullRequestConversationItem; readonly commentId: string } | null { + const anchor = commentAnchor(url); + if (!anchor) return null; + for (const item of items) { + const comment = pullRequestConversationItemComments(item).find( + (entry) => entry.url !== null && commentAnchor(entry.url) === anchor, + ); + if (comment) return { item, commentId: comment.id }; + } + return null; +} + export function filterPullRequestConversationItems( items: ReadonlyArray, filter: { readonly bots: boolean; readonly resolved: boolean }, + linked?: PullRequestConversationItem, ): ReadonlyArray { - return items.filter((item) => - // Matching any member keeps the finding beside a human reply to a bot. - pullRequestConversationItemComments(item).some( - (comment) => - // A bot's approval or change request is a verdict, not chatter. - (filter.bots || - comment.author?.isBot !== true || - isPullRequestVerdict(comment.reviewState)) && - (filter.resolved || - !isPullRequestCommentFinished(comment, item.kind === "thread" ? item.thread : undefined)), - ), + return items.filter( + (item) => + item === linked || + // Matching any member keeps the finding beside a human reply to a bot. + pullRequestConversationItemComments(item).some( + (comment) => + // A bot's approval or change request is a verdict, not chatter. + (filter.bots || + comment.author?.isBot !== true || + isPullRequestVerdict(comment.reviewState)) && + (filter.resolved || + !isPullRequestCommentFinished( + comment, + item.kind === "thread" ? item.thread : undefined, + )), + ), ); } diff --git a/apps/web/src/lib/openPullRequestLink.ts b/apps/web/src/lib/openPullRequestLink.ts index 31c04b6263e5f..7d5e7150b28d4 100644 --- a/apps/web/src/lib/openPullRequestLink.ts +++ b/apps/web/src/lib/openPullRequestLink.ts @@ -106,6 +106,8 @@ export function useOpenChangeRequestLink( ...(host === undefined ? {} : { host }), repository, url: targetUrl, + // Hosts name a comment in the fragment, as in `#issuecomment-1`. + ...(URL.parse(targetUrl)?.hash ? { revealUrl: targetUrl } : {}), number, }); if (!resolvedThreadRef) { diff --git a/apps/web/src/rightPanelStore.test.ts b/apps/web/src/rightPanelStore.test.ts index ec0f4e87c12e8..a59e94cc19b37 100644 --- a/apps/web/src/rightPanelStore.test.ts +++ b/apps/web/src/rightPanelStore.test.ts @@ -1009,6 +1009,28 @@ describe("rightPanelStore", () => { selectActiveRightPanelSurface(useRightPanelStore.getState().byThreadKey, refA), ).toMatchObject({ url }); expect(state.surfaces[1]).not.toHaveProperty("url"); + expect(state.surfaces[0]).not.toHaveProperty("revealUrl"); + }); + + it("holds a comment link until the panel settles it, and a later link replaces it", () => { + const pr = { projectId: "project-a", repository: "pingdotgg/t3code", number: 4909 }; + const id = pullRequestSurfaceId(pr); + const url = "https://github.com/pingdotgg/t3code/pull/4909"; + const surface = () => + selectActiveRightPanelSurface(useRightPanelStore.getState().byThreadKey, refA); + const comment = (n: number) => `${url}#issuecomment-${n}`; + const store = useRightPanelStore.getState(); + store.openPullRequest(refA, { ...pr, url: comment(1), revealUrl: comment(1) }); + expect(surface()).toMatchObject({ url: comment(1), revealUrl: comment(1) }); + store.openPullRequest(refA, { ...pr, url }); + expect(surface()).not.toHaveProperty("revealUrl"); + store.openPullRequest(refA, { ...pr, url: comment(2), revealUrl: comment(2) }); + store.settlePullRequestReveal(refA, id); + expect(surface()).toMatchObject({ url: comment(2) }); + expect(surface()).not.toHaveProperty("revealUrl"); + // Reopening a closed tab hands back its surface, whose url still names the comment. + store.openPullRequest(refA, { ...pr, url: comment(2) }); + expect(surface()).not.toHaveProperty("revealUrl"); }); it("restores each environment, thread and PR surface's detail tab after reload and URL reopen", async () => { diff --git a/apps/web/src/rightPanelStore.ts b/apps/web/src/rightPanelStore.ts index 1f736d4f119ae..ff8aa57ad44f4 100644 --- a/apps/web/src/rightPanelStore.ts +++ b/apps/web/src/rightPanelStore.ts @@ -73,6 +73,8 @@ export type RightPanelSurface = repository: string; number: number; url?: string; + /** A comment link waiting for the panel to show its comment; cleared once shown. */ + revealUrl?: string; detailTab?: PullRequestDetailTab; } | { id: "pull-requests"; kind: "pull-requests" } @@ -148,6 +150,8 @@ interface RightPanelStoreState { repository: string; number: number; url?: string; + /** A clicked link that names a comment; reopening a surface passes only its `url`. */ + revealUrl?: string; detailTab?: PullRequestDetailTab; }, ) => void; @@ -156,6 +160,7 @@ interface RightPanelStoreState { surfaceId: string, detailTab: PullRequestDetailTab, ) => void; + settlePullRequestReveal: (ref: ScopedThreadRef, surfaceId: string) => void; openTerminal: (ref: ScopedThreadRef, terminalId: string) => void; splitTerminal: ( ref: ScopedThreadRef, @@ -752,16 +757,17 @@ export const useRightPanelStore = create()( userAction(state, scopedThreadKey(ref), (current) => { const surface = pullRequestSurface(target); const next = upsertSurface(current, surface); - return target.url - ? { - ...next, - surfaces: next.surfaces.map((entry) => - entry.id === surface.id && entry.kind === "pull-request" - ? { ...entry, ...surface } - : entry, - ), - } - : next; + const { url, revealUrl } = target; + if (!url && !revealUrl) return next; + return { + ...next, + surfaces: next.surfaces.map((entry) => { + if (entry.id !== surface.id || entry.kind !== "pull-request") return entry; + // A later link replaces a comment not yet shown. + const { revealUrl: _previous, ...rest } = entry; + return { ...rest, ...surface, ...(revealUrl ? { revealUrl } : {}) }; + }), + }; }), ), setPullRequestDetailTab: (ref, surfaceId, detailTab) => @@ -781,6 +787,22 @@ export const useRightPanelStore = create()( ), })); }), + settlePullRequestReveal: (ref, surfaceId) => + set((state) => { + const threadKey = scopedThreadKey(ref); + const existing = state.byThreadKey[threadKey]?.surfaces.find( + (surface) => surface.id === surfaceId, + ); + if (existing?.kind !== "pull-request" || existing.revealUrl === undefined) return state; + return automaticUpdate(state, threadKey, (current) => ({ + ...current, + surfaces: current.surfaces.map((surface) => { + if (surface.id !== surfaceId || surface.kind !== "pull-request") return surface; + const { revealUrl: _settled, ...rest } = surface; + return rest; + }), + })); + }), openFile: (ref, requestedPath, line) => set((state) => userAction(state, scopedThreadKey(ref), (current) => { From 606a3e290f7dbf74695f634e2980c18d4f8c0c27 Mon Sep 17 00:00:00 2001 From: "Md.Sadiq" Date: Sat, 7 Nov 2026 22:52:04 +0530 Subject: [PATCH 2/8] fix(web): a comment link missing from cached activity rereads the host activityQuery.refresh() reads the server's activity cache again, so a comment posted after that cache filled was still missing and the link settled without showing it. The single retry now invalidates the host cache first, through the panel's existing refreshFromHost. --- .../pullRequest/PullRequestDetailPanel.tsx | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 625cf94582505..d74a24165d118 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -585,24 +585,6 @@ export function PullRequestDetailPanel({ !activityQuery.isPending ? activityQuery.dataUpdatedAt : null; - const revealRefreshedFor = useRef(null); - const showRevealedComment = useEffectEvent(() => { - if (revealCommentId === null) { - // Cached activity can predate the comment, so load it once more before giving up. - if (revealUrl !== undefined && revealRefreshedFor.current !== revealUrl) { - revealRefreshedFor.current = revealUrl; - return activityQuery.refresh(); - } - return onRevealSettled?.(); - } - // selectTab's store writes; the local tab already switched during render. - if (tabOnPage) useGuideRunStore.getState().showPageDetailTab(guideKey, "summary"); - onTabChange?.("summary"); - }); - // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- the tab and the pending link live in stores outside the panel - useEffect(() => { - if (revealCommentId !== null || revealMissedAt !== null) showRevealedComment(); - }, [revealCommentId, revealMissedAt]); const refreshDetail = () => { detailQuery.refresh(); activityQuery.refresh(); @@ -659,6 +641,24 @@ export function PullRequestDetailPanel({ appliedForcedToken.current = forcedRefreshToken; refreshFromHostOnRequest(); }, [forcedRefreshToken]); + const revealRefreshedFor = useRef(null); + const showRevealedComment = useEffectEvent(() => { + if (revealCommentId === null) { + // Cached activity can predate the comment, so read the host once more before giving up. + if (revealUrl !== undefined && revealRefreshedFor.current !== revealUrl) { + revealRefreshedFor.current = revealUrl; + return void refreshFromHost(); + } + return onRevealSettled?.(); + } + // selectTab's store writes; the local tab already switched during render. + if (tabOnPage) useGuideRunStore.getState().showPageDetailTab(guideKey, "summary"); + onTabChange?.("summary"); + }); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- the tab and the pending link live in stores outside the panel + useEffect(() => { + if (revealCommentId !== null || revealMissedAt !== null) showRevealedComment(); + }, [revealCommentId, revealMissedAt]); const runAction = useAtomCommand(pullRequestEnvironment.runAction, { reportFailure: false, }); From bb219211b86c49ac6d6cd5d04de28d45ca148b89 Mon Sep 17 00:00:00 2001 From: "Md.Sadiq" Date: Sat, 7 Nov 2026 23:04:04 +0530 Subject: [PATCH 3/8] fix(web): a repeated comment link gets its own host read The once-per-link refresh guard kept the URL after the reveal settled, so clicking the same link again on the mounted panel skipped the host read and settled against stale activity. The guard now clears once a reveal is shown or given up. --- apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index d74a24165d118..1ff4e1a43d357 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -649,8 +649,11 @@ export function PullRequestDetailPanel({ revealRefreshedFor.current = revealUrl; return void refreshFromHost(); } + revealRefreshedFor.current = null; return onRevealSettled?.(); } + // A later click on the same link gets its own host read. + revealRefreshedFor.current = null; // selectTab's store writes; the local tab already switched during render. if (tabOnPage) useGuideRunStore.getState().showPageDetailTab(guideKey, "summary"); onTabChange?.("summary"); From e75f14199b5dfcdae9a7527dbe33be90989c2671 Mon Sep 17 00:00:00 2001 From: "Md.Sadiq" Date: Sat, 7 Nov 2026 23:27:38 +0530 Subject: [PATCH 4/8] fix(web): a comment link gives up only on a read after the host invalidation A missing comment waited for any activity update after asking the host to reread, so a read already in flight could settle the link against stale comments. The miss now settles only on a read that finished after the invalidation, and settles at once when the invalidation fails, since the refetch could only repeat the cached read. --- .../pullRequest/PullRequestDetailPanel.tsx | 42 ++++++++++++++----- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 1ff4e1a43d357..ef085676f9412 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -625,13 +625,18 @@ export function PullRequestDetailPanel({ const invalidate = useAtomCommand(pullRequestEnvironment.invalidate, { reportFailure: false }); const [isInvalidating, setIsInvalidating] = useState(false); const refreshing = isInvalidating || detailQuery.isPending; - const refreshFromHost = async () => { + /** Resolves whether the host cache was invalidated before the refetch. */ + const refetchFromHost = async () => { setIsInvalidating(true); // Atom commands settle without throwing, so reset always runs. - await invalidate({ environmentId, input: { reference } }); + const invalidated = await invalidate({ environmentId, input: { reference } }); refreshDetail(); setRefreshToken((token) => token + 1); setIsInvalidating(false); + return invalidated._tag === "Success"; + }; + const refreshFromHost = async () => { + await refetchFromHost(); }; const refreshFromHostOnRequest = useEffectEvent(() => void refreshFromHost()); const appliedForcedToken = useRef(forcedRefreshToken); @@ -641,19 +646,36 @@ export function PullRequestDetailPanel({ appliedForcedToken.current = forcedRefreshToken; refreshFromHostOnRequest(); }, [forcedRefreshToken]); - const revealRefreshedFor = useRef(null); + // Cached activity can predate the comment, so a miss waits for one host read. `after` is when + // that read's invalidation finished; null while it runs. + const revealHostRead = useRef<{ readonly url: string; readonly after: number | null } | null>( + null, + ); + const settleRevealMiss = () => { + revealHostRead.current = null; + onRevealSettled?.(); + }; const showRevealedComment = useEffectEvent(() => { if (revealCommentId === null) { - // Cached activity can predate the comment, so read the host once more before giving up. - if (revealUrl !== undefined && revealRefreshedFor.current !== revealUrl) { - revealRefreshedFor.current = revealUrl; - return void refreshFromHost(); + if (revealUrl === undefined) return; + const read = revealHostRead.current; + if (read?.url !== revealUrl) { + const request = { url: revealUrl, after: null }; + revealHostRead.current = request; + void refetchFromHost().then((invalidated) => { + if (revealHostRead.current !== request) return; + // Without an invalidation the refetch could only repeat the cached read. + if (!invalidated) return settleRevealMiss(); + revealHostRead.current = { url: request.url, after: Date.now() }; + }); + return; } - revealRefreshedFor.current = null; - return onRevealSettled?.(); + // A read that finished before the invalidation cannot show the comment is missing. + if (read.after !== null && (revealMissedAt ?? 0) > read.after) settleRevealMiss(); + return; } // A later click on the same link gets its own host read. - revealRefreshedFor.current = null; + revealHostRead.current = null; // selectTab's store writes; the local tab already switched during render. if (tabOnPage) useGuideRunStore.getState().showPageDetailTab(guideKey, "summary"); onTabChange?.("summary"); From 9de3953169293b211b34c6b4ac5eea11dce1406d Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 10 Nov 2026 14:20:21 +0000 Subject: [PATCH 5/8] fix(web): settle a comment-link reveal only for its own request A host read started for one comment link could clear a newer link once it settled, non-comment fragments forced a host refetch, a pending link survived reload and reopen, and a failed activity refetch left it pending for good. - settlePullRequestReveal takes the link it settles and is a no-op once the surface holds another; the panel drops its pending read on unmount. - Only a fragment shaped like a loaded comment's anchor (`#issuecomment-`, `#discussion_r`, `#note_`) asks the host again; others settle at once. - revealUrl is stripped from persisted state and from closed-tab snapshots. - A failed activity refetch after the host read settles the link. Co-Authored-By: Claude Opus 5.5 --- .../chat/RightPanelSurfaceContent.tsx | 4 +- .../pullRequest/PullRequestDetailPanel.tsx | 47 +++++++++++++------ .../pullRequest/PullRequestsPage.tsx | 4 +- .../pullRequestDetail.logic.test.ts | 11 +++++ .../pullRequest/pullRequestDetail.logic.ts | 25 ++++++++++ apps/web/src/rightPanelStore.test.ts | 21 ++++++++- apps/web/src/rightPanelStore.ts | 35 +++++++++----- 7 files changed, 116 insertions(+), 31 deletions(-) diff --git a/apps/web/src/components/chat/RightPanelSurfaceContent.tsx b/apps/web/src/components/chat/RightPanelSurfaceContent.tsx index 02f3f0d07779a..d0cd7cf018095 100644 --- a/apps/web/src/components/chat/RightPanelSurfaceContent.tsx +++ b/apps/web/src/components/chat/RightPanelSurfaceContent.tsx @@ -154,8 +154,8 @@ export function RightPanelSurfaceContent(props: { useRightPanelStore.getState().setPullRequestDetailTab(threadRef, surface.id, tab); }} revealUrl={surface.revealUrl} - onRevealSettled={() => { - useRightPanelStore.getState().settlePullRequestReveal(threadRef, surface.id); + onRevealSettled={(revealUrl) => { + useRightPanelStore.getState().settlePullRequestReveal(threadRef, surface.id, revealUrl); }} context={pullRequestPanelContext( { diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index ef085676f9412..7546afca71ecd 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -126,6 +126,7 @@ import { isStackedPullRequestBase, pullRequestActionNeedsHostRefresh, pullRequestCheckoutCommand, + pullRequestCommentLinkMayLoad, pullRequestCommentLinkTarget, pullRequestConversationItems, pullRequestFindingKey, @@ -305,7 +306,8 @@ export function PullRequestDetailPanel({ onTabChange?: ((tab: PullRequestDetailTab) => void) | undefined; /** A link that may name one of this pull request's comments, waiting to be shown. */ revealUrl?: string | undefined; - onRevealSettled?: (() => void) | undefined; + /** Reports the link it was given shown or missing, so a newer link is left pending. */ + onRevealSettled?: ((revealUrl: string) => void) | undefined; }) { const environmentConfigs = useServerConfigs(); const projects = useProjects(); @@ -363,13 +365,14 @@ export function PullRequestDetailPanel({ detail, } = usePullRequestDetail(environmentId, reference, matchingListEntry); const [selectedTab, setTab] = useState(initialTab); - const revealCommentId = + const revealItems = revealUrl === undefined || detail === null ? null - : (pullRequestCommentLinkTarget( - pullRequestConversationItems(detail.comments, detail.reviewThreads), - revealUrl, - )?.commentId ?? null); + : pullRequestConversationItems(detail.comments, detail.reviewThreads); + const revealCommentId = + revealItems === null || revealUrl === undefined + ? null + : (pullRequestCommentLinkTarget(revealItems, revealUrl)?.commentId ?? null); // A comment link shows the conversation holding it. if (revealCommentId !== null && !tabOnPage && selectedTab !== "summary") setTab("summary"); const reviewGuidesEnabled = useEnvironmentSettings( @@ -585,6 +588,7 @@ export function PullRequestDetailPanel({ !activityQuery.isPending ? activityQuery.dataUpdatedAt : null; + const revealReadFailed = revealMissedAt !== null && activityQuery.error !== null; const refreshDetail = () => { detailQuery.refresh(); activityQuery.refresh(); @@ -651,27 +655,37 @@ export function PullRequestDetailPanel({ const revealHostRead = useRef<{ readonly url: string; readonly after: number | null } | null>( null, ); - const settleRevealMiss = () => { + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a host read outliving the panel must not settle a later link + useEffect( + () => () => { + revealHostRead.current = null; + }, + [], + ); + const settleReveal = (url: string) => { revealHostRead.current = null; - onRevealSettled?.(); + onRevealSettled?.(url); }; const showRevealedComment = useEffectEvent(() => { if (revealCommentId === null) { - if (revealUrl === undefined) return; + if (revealUrl === undefined || revealItems === null) return; const read = revealHostRead.current; if (read?.url !== revealUrl) { + if (!pullRequestCommentLinkMayLoad(revealItems, revealUrl)) return settleReveal(revealUrl); const request = { url: revealUrl, after: null }; revealHostRead.current = request; void refetchFromHost().then((invalidated) => { if (revealHostRead.current !== request) return; // Without an invalidation the refetch could only repeat the cached read. - if (!invalidated) return settleRevealMiss(); + if (!invalidated) return settleReveal(request.url); revealHostRead.current = { url: request.url, after: Date.now() }; }); return; } // A read that finished before the invalidation cannot show the comment is missing. - if (read.after !== null && (revealMissedAt ?? 0) > read.after) settleRevealMiss(); + if (read.after !== null && (revealReadFailed || (revealMissedAt ?? 0) > read.after)) { + settleReveal(revealUrl); + } return; } // A later click on the same link gets its own host read. @@ -682,8 +696,11 @@ export function PullRequestDetailPanel({ }); // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- the tab and the pending link live in stores outside the panel useEffect(() => { - if (revealCommentId !== null || revealMissedAt !== null) showRevealedComment(); - }, [revealCommentId, revealMissedAt]); + if (revealCommentId !== null || revealMissedAt !== null || revealReadFailed) { + // oxlint-disable-next-line react/set-state-in-effect -- a missed comment starts a host read, which marks itself refreshing before it awaits + showRevealedComment(); + } + }, [revealCommentId, revealMissedAt, revealReadFailed]); const runAction = useAtomCommand(pullRequestEnvironment.runAction, { reportFailure: false, }); @@ -1819,7 +1836,9 @@ export function PullRequestDetailPanel({ detail={detail} activityPending={activityPending} revealCommentId={tab === "summary" ? revealCommentId : null} - {...(onRevealSettled ? { onCommentRevealed: onRevealSettled } : {})} + {...(onRevealSettled && revealUrl !== undefined + ? { onCommentRevealed: () => onRevealSettled(revealUrl) } + : {})} checksStale={checksStale} activityError={activityError} pendingFinding={handoff} diff --git a/apps/web/src/components/pullRequest/PullRequestsPage.tsx b/apps/web/src/components/pullRequest/PullRequestsPage.tsx index a1e052a3e2956..953cf79b32c06 100644 --- a/apps/web/src/components/pullRequest/PullRequestsPage.tsx +++ b/apps/web/src/components/pullRequest/PullRequestsPage.tsx @@ -1552,11 +1552,11 @@ export function PullRequestsPage() { refreshToken={detailRefreshToken} tabOnPage revealUrl={renderedPullRequestSurface.revealUrl} - onRevealSettled={() => { + onRevealSettled={(revealUrl) => { if (rightPanelRef === null) return; useRightPanelStore .getState() - .settlePullRequestReveal(rightPanelRef, renderedPullRequestSurface.id); + .settlePullRequestReveal(rightPanelRef, renderedPullRequestSurface.id, revealUrl); }} onActed={(action, phase = "done", reply) => { // From every row held, not the ones on screen: a pull request closed from an open diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index 22db45a9de669..aae6eac3f0474 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -28,6 +28,7 @@ import { groupPullRequestTimelineConversations, filterPullRequestConversationItems, pagePullRequestConversationItems, + pullRequestCommentLinkMayLoad, pullRequestCommentLinkTarget, pullRequestConversationItemComments, pullRequestConversationItems, @@ -582,6 +583,16 @@ describe("review thread conversations", () => { expect( pullRequestCommentLinkTarget(githubItems, "https://github.com/o/r/pull/1/files#r42"), ).toEqual({ item: githubItems[0], commentId: "reply" }); + const issueComment = { ...reply, url: "https://github.com/o/r/pull/1#issuecomment-1" }; + const issueItems = pullRequestConversationItems([issueComment], []); + const pull = "https://github.com/o/r/pull/1"; + expect(pullRequestCommentLinkMayLoad(issueItems, `${pull}#issuecomment-999`)).toBe(true); + expect(pullRequestCommentLinkMayLoad(githubItems, `${pull}/files#r999`)).toBe(true); + for (const fragment of ["#diff-abc", "#issue-1", "#event-1", "#partial-pull-merging", ""]) { + expect(pullRequestCommentLinkMayLoad(issueItems, `${pull}/files${fragment}`), fragment).toBe( + false, + ); + } for (const url of ["https://example.test/pull/1", "https://example.test/pull/1#gone", "#"]) { expect(pullRequestCommentLinkTarget(items, url), url).toBeNull(); } diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index 986a39285a277..b8bc8d6b32e1b 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -651,6 +651,31 @@ export function pullRequestCommentLinkTarget( return null; } +/** `#issuecomment-1` -> `#issuecomment-`; linear, unlike a backtracking `/\d+$/`. */ +function anchorKind(anchor: string) { + let end = anchor.length; + while (end > 0 && anchor.charCodeAt(end - 1) >= 48 && anchor.charCodeAt(end - 1) <= 57) end--; + return anchor.slice(0, end); +} + +/** + * Whether a link no loaded comment matches could name a newer one: its fragment is shaped like a + * loaded comment's, so `#diff-…` or `#event-…` never asks the host again. + */ +export function pullRequestCommentLinkMayLoad( + items: ReadonlyArray, + url: string, +): boolean { + const anchor = commentAnchor(url); + if (!anchor) return false; + const kind = anchorKind(anchor); + return items.some((item) => + pullRequestConversationItemComments(item).some( + (comment) => comment.url !== null && anchorKind(commentAnchor(comment.url) ?? "") === kind, + ), + ); +} + export function filterPullRequestConversationItems( items: ReadonlyArray, filter: { readonly bots: boolean; readonly resolved: boolean }, diff --git a/apps/web/src/rightPanelStore.test.ts b/apps/web/src/rightPanelStore.test.ts index a59e94cc19b37..40af91eb9018f 100644 --- a/apps/web/src/rightPanelStore.test.ts +++ b/apps/web/src/rightPanelStore.test.ts @@ -1012,7 +1012,7 @@ describe("rightPanelStore", () => { expect(state.surfaces[0]).not.toHaveProperty("revealUrl"); }); - it("holds a comment link until the panel settles it, and a later link replaces it", () => { + it("holds a comment link until the panel settles it, and a later link replaces it", async () => { const pr = { projectId: "project-a", repository: "pingdotgg/t3code", number: 4909 }; const id = pullRequestSurfaceId(pr); const url = "https://github.com/pingdotgg/t3code/pull/4909"; @@ -1025,12 +1025,29 @@ describe("rightPanelStore", () => { store.openPullRequest(refA, { ...pr, url }); expect(surface()).not.toHaveProperty("revealUrl"); store.openPullRequest(refA, { ...pr, url: comment(2), revealUrl: comment(2) }); - store.settlePullRequestReveal(refA, id); + // A host read started for an earlier link settles only that link. + store.settlePullRequestReveal(refA, id, comment(1)); + expect(surface()).toMatchObject({ revealUrl: comment(2) }); + store.settlePullRequestReveal(refA, id, comment(2)); expect(surface()).toMatchObject({ url: comment(2) }); expect(surface()).not.toHaveProperty("revealUrl"); // Reopening a closed tab hands back its surface, whose url still names the comment. store.openPullRequest(refA, { ...pr, url: comment(2) }); expect(surface()).not.toHaveProperty("revealUrl"); + store.openPullRequest(refA, { ...pr, url: comment(3), revealUrl: comment(3) }); + await reloadRightPanelStore(); + expect(surface()).toMatchObject({ url: comment(3) }); + expect(surface()).not.toHaveProperty("revealUrl"); + store.openPullRequest(refA, { ...pr, url: comment(4), revealUrl: comment(4) }); + store.closeSurface(refA, id); + await reopenClosedView(useClosedViewStore.getState().entries[0]!, { + workspaceAvailable: true, + openPreview: () => { + throw new Error("A PR tab must not create a browser session"); + }, + }); + expect(surface()).toMatchObject({ url: comment(4) }); + expect(surface()).not.toHaveProperty("revealUrl"); }); it("restores each environment, thread and PR surface's detail tab after reload and URL reopen", async () => { diff --git a/apps/web/src/rightPanelStore.ts b/apps/web/src/rightPanelStore.ts index ff8aa57ad44f4..22623716a988f 100644 --- a/apps/web/src/rightPanelStore.ts +++ b/apps/web/src/rightPanelStore.ts @@ -160,7 +160,8 @@ interface RightPanelStoreState { surfaceId: string, detailTab: PullRequestDetailTab, ) => void; - settlePullRequestReveal: (ref: ScopedThreadRef, surfaceId: string) => void; + /** Clears the surface's pending link only while it is still `revealUrl`. */ + settlePullRequestReveal: (ref: ScopedThreadRef, surfaceId: string, revealUrl: string) => void; openTerminal: (ref: ScopedThreadRef, terminalId: string) => void; splitTerminal: ( ref: ScopedThreadRef, @@ -298,6 +299,13 @@ export function pullRequestSurface(target: { }; } +/** A pending comment link belongs to the click that made it, not to a reload or a reopen. */ +const withoutReveal = (surface: RightPanelSurface): RightPanelSurface => { + if (surface.kind !== "pull-request" || surface.revealUrl === undefined) return surface; + const { revealUrl: _pending, ...rest } = surface; + return rest; +}; + const upsertSurface = ( current: ThreadRightPanelState, surface: RightPanelSurface, @@ -787,20 +795,18 @@ export const useRightPanelStore = create()( ), })); }), - settlePullRequestReveal: (ref, surfaceId) => + settlePullRequestReveal: (ref, surfaceId, revealUrl) => set((state) => { const threadKey = scopedThreadKey(ref); const existing = state.byThreadKey[threadKey]?.surfaces.find( (surface) => surface.id === surfaceId, ); - if (existing?.kind !== "pull-request" || existing.revealUrl === undefined) return state; + if (existing?.kind !== "pull-request" || existing.revealUrl !== revealUrl) return state; return automaticUpdate(state, threadKey, (current) => ({ ...current, - surfaces: current.surfaces.map((surface) => { - if (surface.id !== surfaceId || surface.kind !== "pull-request") return surface; - const { revealUrl: _settled, ...rest } = surface; - return rest; - }), + surfaces: current.surfaces.map((surface) => + surface.id === surfaceId ? withoutReveal(surface) : surface, + ), })); }), openFile: (ref, requestedPath, line) => @@ -1186,8 +1192,15 @@ export const useRightPanelStore = create()( ), partialize: (state) => ({ byThreadKey: Object.fromEntries( - Object.entries(state.byThreadKey).filter( - ([threadKey]) => !isPullRequestsPanelKey(threadKey), + Object.entries(state.byThreadKey).flatMap(([threadKey, threadState]) => + isPullRequestsPanelKey(threadKey) + ? [] + : [ + [ + threadKey, + { ...threadState, surfaces: threadState.surfaces.map(withoutReveal) }, + ], + ], ), ), threadPanelVisibilityByThreadKey: Object.fromEntries( @@ -1217,7 +1230,7 @@ useRightPanelStore.subscribe((next, previous) => { ...removed.filter((surface) => surface.id !== before.activeSurfaceId), ...removed.filter((surface) => surface.id === before.activeSurfaceId), ]; - for (const surface of closedInOrder) { + for (const surface of closedInOrder.map(withoutReveal)) { if ( surface.kind === "terminal" || (surface.kind === "preview" && surface.resourceId !== null) From f52b3915f7b5df153066f8575a12580073814db8 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 10 Nov 2026 14:22:12 +0000 Subject: [PATCH 6/8] fix(web): a comment link on a pull request with no loaded comments still rereads the host Co-Authored-By: Claude Opus 5.5 --- .../pullRequest/pullRequestDetail.logic.test.ts | 1 + .../pullRequest/pullRequestDetail.logic.ts | 12 ++++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index aae6eac3f0474..2899c3603d2aa 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -588,6 +588,7 @@ describe("review thread conversations", () => { const pull = "https://github.com/o/r/pull/1"; expect(pullRequestCommentLinkMayLoad(issueItems, `${pull}#issuecomment-999`)).toBe(true); expect(pullRequestCommentLinkMayLoad(githubItems, `${pull}/files#r999`)).toBe(true); + expect(pullRequestCommentLinkMayLoad([], `${pull}#issuecomment-1`)).toBe(true); for (const fragment of ["#diff-abc", "#issue-1", "#event-1", "#partial-pull-merging", ""]) { expect(pullRequestCommentLinkMayLoad(issueItems, `${pull}/files${fragment}`), fragment).toBe( false, diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index b8bc8d6b32e1b..b0a8a16aec886 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -668,12 +668,16 @@ export function pullRequestCommentLinkMayLoad( ): boolean { const anchor = commentAnchor(url); if (!anchor) return false; - const kind = anchorKind(anchor); - return items.some((item) => - pullRequestConversationItemComments(item).some( - (comment) => comment.url !== null && anchorKind(commentAnchor(comment.url) ?? "") === kind, + const kinds = new Set( + items.flatMap((item) => + pullRequestConversationItemComments(item).flatMap((comment) => { + const loaded = comment.url === null ? null : commentAnchor(comment.url); + return loaded ? [anchorKind(loaded)] : []; + }), ), ); + // With no loaded anchor to compare, the link may name the first comment. + return kinds.size === 0 || kinds.has(anchorKind(anchor)); } export function filterPullRequestConversationItems( From df232a10d932491c0b70604a08e315b20f7d4179 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 10 Nov 2026 15:37:47 +0000 Subject: [PATCH 7/8] fix(web): any known comment fragment rereads the host, not only kinds already loaded Co-Authored-By: Claude Opus 5.5 --- .../pullRequestDetail.logic.test.ts | 3 +++ .../pullRequest/pullRequestDetail.logic.ts | 21 +++++++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index 2899c3603d2aa..d7bf18172a325 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -589,6 +589,9 @@ describe("review thread conversations", () => { expect(pullRequestCommentLinkMayLoad(issueItems, `${pull}#issuecomment-999`)).toBe(true); expect(pullRequestCommentLinkMayLoad(githubItems, `${pull}/files#r999`)).toBe(true); expect(pullRequestCommentLinkMayLoad([], `${pull}#issuecomment-1`)).toBe(true); + for (const fragment of ["#discussion_r42", "/files#r42", "#pullrequestreview-42"]) { + expect(pullRequestCommentLinkMayLoad(issueItems, `${pull}${fragment}`), fragment).toBe(true); + } for (const fragment of ["#diff-abc", "#issue-1", "#event-1", "#partial-pull-merging", ""]) { expect(pullRequestCommentLinkMayLoad(issueItems, `${pull}/files${fragment}`), fragment).toBe( false, diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index b0a8a16aec886..54ce6d32609b9 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -658,9 +658,18 @@ function anchorKind(anchor: string) { return anchor.slice(0, end); } +// GitHub, GitLab, Forgejo and Bitbucket comment fragments; a newer comment may be the first of its kind. +const COMMENT_ANCHOR_KINDS = [ + "#issuecomment-", + "#discussion_r", + "#pullrequestreview-", + "#note_", + "#comment-", +]; + /** * Whether a link no loaded comment matches could name a newer one: its fragment is shaped like a - * loaded comment's, so `#diff-…` or `#event-…` never asks the host again. + * comment's, so `#diff-…` or `#event-…` never asks the host again. */ export function pullRequestCommentLinkMayLoad( items: ReadonlyArray, @@ -668,16 +677,16 @@ export function pullRequestCommentLinkMayLoad( ): boolean { const anchor = commentAnchor(url); if (!anchor) return false; - const kinds = new Set( - items.flatMap((item) => + const kinds = new Set([ + ...COMMENT_ANCHOR_KINDS, + ...items.flatMap((item) => pullRequestConversationItemComments(item).flatMap((comment) => { const loaded = comment.url === null ? null : commentAnchor(comment.url); return loaded ? [anchorKind(loaded)] : []; }), ), - ); - // With no loaded anchor to compare, the link may name the first comment. - return kinds.size === 0 || kinds.has(anchorKind(anchor)); + ]); + return kinds.has(anchorKind(anchor)); } export function filterPullRequestConversationItems( From 9b9b813985b8a18c3c5b4dc0ab6aa75627633b99 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 10 Nov 2026 15:53:05 +0000 Subject: [PATCH 8/8] fix(web): a comment-link miss read in the invalidation's millisecond still settles Co-Authored-By: Claude Opus 5.5 --- .../src/components/pullRequest/PullRequestDetailPanel.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 7546afca71ecd..e7287c8ff7a17 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -682,8 +682,9 @@ export function PullRequestDetailPanel({ }); return; } - // A read that finished before the invalidation cannot show the comment is missing. - if (read.after !== null && (revealReadFailed || (revealMissedAt ?? 0) > read.after)) { + // A read that finished before the invalidation cannot show the comment is missing. `after` is + // stamped once the refetch was requested, so a read in that same millisecond is the new one. + if (read.after !== null && (revealReadFailed || (revealMissedAt ?? 0) >= read.after)) { settleReveal(revealUrl); } return;