diff --git a/src/components/Changes.svelte b/src/components/Changes.svelte index c61bb4dfd..31575fa82 100644 --- a/src/components/Changes.svelte +++ b/src/components/Changes.svelte @@ -19,7 +19,11 @@ cachedListCommits, getDiffText, } from "@app/lib/invoke"; - import { anchorOf, commentCountsByPath } from "@app/lib/pierreComments"; + import { + anchorOf, + commentCountsByPath, + isCommentableStatus, + } from "@app/lib/pierreComments"; import { pluralize } from "@app/lib/utils"; import CobCommitTeaser from "@app/components/CobCommitTeaser.svelte"; @@ -50,6 +54,9 @@ // The view switcher, stuck to the top of the diff's scroll port. It is the // last thing before the two columns, so the description sits above it. tabs?: Snippet; + // Where the comment stepper stands, for the tab bar to render. An index of + // `-1` means it has not been stepped yet. + commentPosition?: { index: number; total: number }; } /* eslint-disable prefer-const */ @@ -68,6 +75,9 @@ onSaveDescription, chrome, tabs, + // Another output binding, like `filesExpanded`: the tab bar renders the + // stepper and reads this, while the stepping itself happens here. + commentPosition = $bindable({ index: -1, total: 0 }), }: Props = $props(); /* eslint-enable prefer-const */ @@ -226,6 +236,90 @@ ); }); + // Every comment on the diff in the order it is rendered — by file, in the + // order the files appear, then down the lines of each. What the tab bar's + // stepper walks. + const orderedComments = $derived.by(() => { + const comments = diffCodeComments; + if (!comments) return []; + const fileOrder = new Map( + diffFiles.map((file, index) => [fileDiffPath(file), index] as const), + ); + return comments.threads + .flatMap(thread => { + const anchor = anchorOf(thread.root.location); + // Dropped for the same two reasons the diff itself drops them: the file + // is not in this diff, or its content moved and an anchor in it is + // ambiguous. Either way there is nothing on screen to step to. + if (!anchor) return []; + const order = fileOrder.get(anchor.path); + if (order === undefined) return []; + if (!isCommentableStatus(fileMeta.statuses.get(anchor.path))) return []; + return [ + { + id: thread.root.id, + anchor, + order, + // A deletion is rendered above an addition on the same line, and + // two comments on one line are ordered oldest first — the same rule + // the annotations themselves are built with. + side: anchor.side === "deletions" ? 0 : 1, + timestamp: thread.root.edits[0].timestamp, + }, + ]; + }) + .sort( + (a, b) => + a.order - b.order || + a.anchor.line - b.anchor.line || + a.side - b.side || + a.timestamp - b.timestamp, + ); + }); + + // Which comment the stepper is on, held by id rather than by position: the + // list shifts as comments are written, deleted or filtered out, and a position + // would quietly come to mean a different comment. One that goes away leaves + // the stepper unset, which reads as `-1` and starts the walk over. + let activeCommentId = $state(); + const commentIndex = $derived( + activeCommentId === undefined + ? -1 + : orderedComments.findIndex(entry => entry.id === activeCommentId), + ); + $effect(() => { + const index = commentIndex; + const total = orderedComments.length; + if (commentPosition.index !== index || commentPosition.total !== total) { + commentPosition = { index, total }; + } + }); + + // Rings the comment just stepped to and then lets it go, so the diff is not + // left holding a stale mark. + let highlightedCommentId = $state(); + let highlightTimer: ReturnType | undefined; + $effect(() => () => clearTimeout(highlightTimer)); + + /// Walk to the next (`1`) or previous (`-1`) comment on the diff. Called from + /// the tab bar, which sits outside this component. + export function stepComment(delta: number) { + const total = orderedComments.length; + if (total === 0) return; + // From nothing, a step down starts at the first comment and a step up at the + // last. From somewhere, both wrap, so a walk never dead-ends at either end + // of a long diff. + const from = commentIndex >= 0 ? commentIndex : delta > 0 ? -1 : 0; + const target = orderedComments[(from + delta + total) % total]; + activeCommentId = target.id; + clearTimeout(highlightTimer); + highlightedCommentId = target.id; + highlightTimer = setTimeout(() => { + highlightedCommentId = undefined; + }, 2400); + diffView?.scrollToAnchor(target.anchor); + } + // Pierre's context-expand markers hydrate a file lazily from these, so // nothing is fetched until the reader expands something. const loadFullFile = $derived( @@ -504,7 +598,10 @@ .commits-column { max-height: 100%; overflow-y: auto; - border: 1px solid var(--color-border-subtle); + /* An outset ring, not a border: the file cards beside it are outlined the + same way, and a real border would sit inside the box and leave the two a + pixel out of line. */ + box-shadow: 0 0 0 1px var(--color-border-subtle); border-radius: var(--border-radius-md); background-color: var(--color-surface-canvas); } @@ -801,6 +898,7 @@ ? path => draftReviewStorage.toggleCheckedFile(draftReviewId, path) : undefined} {commentCounts} + {highlightedCommentId} codeComments={diffCodeComments} commentCommit={isRevisionDiff ? head : undefined} header={diffHeader} diff --git a/src/components/Revision.svelte b/src/components/Revision.svelte index fae5c2a66..40565a7f1 100644 --- a/src/components/Revision.svelte +++ b/src/components/Revision.svelte @@ -111,6 +111,8 @@ chrome?: Snippet; // The view switcher, which the Changes tab sticks to the top of the diff. tabs?: Snippet; + // Where the Changes tab's comment stepper stands, for the tab bar to render. + commentPosition?: { index: number; total: number }; } /* eslint-disable prefer-const */ @@ -133,6 +135,7 @@ onViewChanges, chrome, tabs, + commentPosition = $bindable({ index: -1, total: 0 }), }: Props = $props(); /* eslint-enable prefer-const */ let changes = $state | undefined>(); @@ -143,6 +146,12 @@ changes?.setAllFilesCollapsed(collapsed); } + /// Forwarded to the Changes tab so the tab bar's stepper can walk the comments + /// on the diff. + export function stepComment(delta: number) { + changes?.stepComment(delta); + } + /// Forwarded to the Changes tab so the draft review bar, which lives further /// up in the patch view, can scroll the diff to one of its comments. export async function revealComment(location: CodeLocation) { @@ -1909,5 +1918,6 @@ {chrome} {tabs} bind:showingRevisionDiff - bind:filesExpanded /> + bind:filesExpanded + bind:commentPosition /> {/if} diff --git a/src/views/repo/Patch.svelte b/src/views/repo/Patch.svelte index 179498695..b2c02decf 100644 --- a/src/views/repo/Patch.svelte +++ b/src/views/repo/Patch.svelte @@ -221,11 +221,9 @@ const commentSources = $derived(commentSourcesOf(selectedRevision)); - const visibleCommentCount = $derived( - commentSources - .filter(s => !hiddenCommentSources.has(s.id)) - .reduce((total, s) => total + s.count, 0), - ); + // Where the Changes tab's comment stepper stands. Written by that tab, read by + // the tab bar below, which is rendered here. + let commentPosition = $state({ index: -1, total: 0 }); let commentSourcesExpanded = $state(false); let commitCountsByRevisionId: Record = $state({}); @@ -788,6 +786,21 @@ .tabs-right { margin-left: auto; } + /* Tighter than the row it sits in: the two arrows and the count are one + control, not three. */ + .comment-stepper { + display: flex; + align-items: center; + gap: 0.125rem; + } + .comment-position { + /* Enough for "16 of 16" without the arrows shifting as the number grows + through a walk. */ + min-width: 4.5rem; + text-align: center; + color: var(--color-text-secondary); + font-variant-numeric: tabular-nums; + } .avatar-stack { display: inline-flex; align-items: center; @@ -974,7 +987,10 @@ style:margin-left="auto" style:gap="0.5rem" style:z-index="40"> - {#if isOwnPatch} + + {#if isOwnPatch && !currentReview} - {#if ownDraftReview} - {#if patchView !== "changes"} + + {#if !currentReview} + {#if ownDraftReview} + {#if patchView !== "changes"} + + {/if} + {:else} {/if} - {:else} - {/if} @@ -1137,72 +1158,105 @@ {@const onLatestRevision = selectedRevision.id === latestRevision.id}
{#if commentSources.length > 0} - - {#snippet toggle(onclick)} - + + {#snippet toggle(onclick)} + + {/snippet} + {#snippet popover()}
- - {visibleCommentCount} + style:border="1px solid var(--color-border-subtle)" + style:border-radius="var(--border-radius-sm)" + style:background-color="var(--color-surface-canvas)"> + + {#snippet item(source)} + {@const hidden = hiddenCommentSources.has(source.id)} + { + if (hidden) { + hiddenCommentSources.delete(source.id); + } else { + hiddenCommentSources.add(source.id); + } + }}> + + + {#each source.nids.slice(0, 2) as nid (nid)} + + {/each} + {#if source.nids.length > 2} + + + {/if} + + + {source.name} + +
+ + {source.count} +
+ + {/snippet} +
- Comments - - {/snippet} - {#snippet popover()} -
- - {#snippet item(source)} - {@const hidden = hiddenCommentSources.has(source.id)} - { - if (hidden) { - hiddenCommentSources.delete(source.id); - } else { - hiddenCommentSources.add(source.id); - } - }}> - - - {#each source.nids.slice(0, 2) as nid (nid)} - - {/each} - {#if source.nids.length > 2} - + - {/if} - - - {source.name} - -
- - {source.count} -
- - {/snippet} - -
- {/snippet} - + {/snippet} + + +
{/if} {#if !onLatestRevision}