diff --git a/apps/web/src/components/chat/RightPanelSurfaceContent.tsx b/apps/web/src/components/chat/RightPanelSurfaceContent.tsx index f0d8afa5f319c..d0cd7cf018095 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={(revealUrl) => { + useRightPanelStore.getState().settlePullRequestReveal(threadRef, surface.id, revealUrl); + }} 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..e7287c8ff7a17 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -126,6 +126,9 @@ import { isStackedPullRequestBase, pullRequestActionNeedsHostRefresh, pullRequestCheckoutCommand, + pullRequestCommentLinkMayLoad, + pullRequestCommentLinkTarget, + pullRequestConversationItems, pullRequestFindingKey, pullRequestHandoffLabels, PULL_REQUEST_MERGE_METHOD_LABELS, @@ -273,6 +276,8 @@ export function PullRequestDetailPanel({ onGuideShown, initialTab = "summary", onTabChange, + revealUrl, + onRevealSettled, }: { environmentId: EnvironmentId; shortcutsEnabled: boolean; @@ -299,6 +304,10 @@ 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; + /** 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(); @@ -356,6 +365,16 @@ export function PullRequestDetailPanel({ detail, } = usePullRequestDetail(environmentId, reference, matchingListEntry); const [selectedTab, setTab] = useState(initialTab); + const revealItems = + revealUrl === undefined || detail === null + ? 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( environmentId, (settings) => settings.reviewGuidesEnabled, @@ -561,6 +580,15 @@ 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 revealReadFailed = revealMissedAt !== null && activityQuery.error !== null; const refreshDetail = () => { detailQuery.refresh(); activityQuery.refresh(); @@ -601,13 +629,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); @@ -617,6 +650,58 @@ export function PullRequestDetailPanel({ appliedForcedToken.current = forcedRefreshToken; refreshFromHostOnRequest(); }, [forcedRefreshToken]); + // 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, + ); + // 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?.(url); + }; + const showRevealedComment = useEffectEvent(() => { + if (revealCommentId === null) { + 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 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. `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; + } + // A later click on the same link gets its own host read. + revealHostRead.current = null; + // 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 || 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, }); @@ -1751,6 +1836,10 @@ export function PullRequestDetailPanel({ reference={reference} detail={detail} activityPending={activityPending} + revealCommentId={tab === "summary" ? revealCommentId : null} + {...(onRevealSettled && revealUrl !== undefined + ? { onCommentRevealed: () => onRevealSettled(revealUrl) } + : {})} 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..953cf79b32c06 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={(revealUrl) => { + if (rightPanelRef === null) return; + useRightPanelStore + .getState() + .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 // 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 5b298d23c429b..d7bf18172a325 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -28,6 +28,8 @@ import { groupPullRequestTimelineConversations, filterPullRequestConversationItems, pagePullRequestConversationItems, + pullRequestCommentLinkMayLoad, + pullRequestCommentLinkTarget, pullRequestConversationItemComments, pullRequestConversationItems, appendHandoff, @@ -570,6 +572,43 @@ 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" }); + 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); + 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, + ); + } + 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 537de10a2a8c1..54ce6d32609b9 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -628,21 +628,88 @@ 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; +} + +/** `#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); +} + +// 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 + * 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 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)] : []; + }), + ), + ]); + return kinds.has(anchorKind(anchor)); +} + 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..40af91eb9018f 100644 --- a/apps/web/src/rightPanelStore.test.ts +++ b/apps/web/src/rightPanelStore.test.ts @@ -1009,6 +1009,45 @@ 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", async () => { + 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) }); + // 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 1f736d4f119ae..22623716a988f 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,8 @@ interface RightPanelStoreState { surfaceId: string, detailTab: PullRequestDetailTab, ) => 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, @@ -293,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, @@ -752,16 +765,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 +795,20 @@ export const useRightPanelStore = create()( ), })); }), + 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 !== revealUrl) return state; + return automaticUpdate(state, threadKey, (current) => ({ + ...current, + surfaces: current.surfaces.map((surface) => + surface.id === surfaceId ? withoutReveal(surface) : surface, + ), + })); + }), openFile: (ref, requestedPath, line) => set((state) => userAction(state, scopedThreadKey(ref), (current) => { @@ -1164,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( @@ -1195,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)