Add a comment stepper to the Changes tab

Walks the comments on the diff in the order they are rendered, merged
into the control that filters which of them are shown.

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<string | undefined>();
+  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<string | undefined>();
+  let highlightTimer: ReturnType<typeof setTimeout> | 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}

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<ReturnType<typeof Changes> | 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}

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<string, number> = $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}
+        <!-- Not on the review view: the page's own Delete is for the review, and
+             a second Delete up here would be one word for two different
+             targets. -->
+        {#if isOwnPatch && !currentReview}
           <Popover
             popoverPadding="0"
             placement="bottom-end"
@@ -1036,40 +1052,45 @@
           patchId={patch.id}
           selectedRevisionId={selectedRevision.id}
           {tab} />
-        {#if ownDraftReview}
-          {#if patchView !== "changes"}
+        <!-- Not on the review view: reviewing is what that view already is, and
+             it carries its own actions on the page. -->
+        {#if !currentReview}
+          {#if ownDraftReview}
+            {#if patchView !== "changes"}
+              <Button
+                variant="secondary"
+                onclick={() => setView("changes")}
+                title="Continue your review of this revision">
+                <Icon name="comment" />
+                <span
+                  class="txt-body-m-regular global-hide-on-medium-desktop-down">
+                  Continue review
+                </span>
+              </Button>
+            {/if}
+          {:else}
             <Button
               variant="secondary"
-              onclick={() => setView("changes")}
-              title="Continue your review of this revision">
+              disabled={hasOwnPublishedReviewOnSelected}
+              onclick={() => {
+                draftReviewStorage.create(
+                  repo.rid,
+                  patch.id,
+                  selectedRevision.id,
+                  config.publicKey,
+                );
+                setView("changes");
+              }}
+              title={hasOwnPublishedReviewOnSelected
+                ? "You already reviewed this revision. You can still add comments on the changes."
+                : "Start a review of this revision"}>
               <Icon name="comment" />
               <span
                 class="txt-body-m-regular global-hide-on-medium-desktop-down">
-                Continue review
+                Review
               </span>
             </Button>
           {/if}
-        {:else}
-          <Button
-            variant="secondary"
-            disabled={hasOwnPublishedReviewOnSelected}
-            onclick={() => {
-              draftReviewStorage.create(
-                repo.rid,
-                patch.id,
-                selectedRevision.id,
-                config.publicKey,
-              );
-              setView("changes");
-            }}
-            title={hasOwnPublishedReviewOnSelected
-              ? "You already reviewed this revision. You can still add comments on the changes."
-              : "Start a review of this revision"}>
-            <Icon name="comment" />
-            <span class="txt-body-m-regular global-hide-on-medium-desktop-down">
-              Review
-            </span>
-          </Button>
         {/if}
       </div>
     </Topbar>
@@ -1137,72 +1158,105 @@
           {@const onLatestRevision = selectedRevision.id === latestRevision.id}
           <div class="tabs-right">
             {#if commentSources.length > 0}
-              <Popover
-                popoverPadding="0"
-                placement="bottom-start"
-                bind:expanded={commentSourcesExpanded}>
-                {#snippet toggle(onclick)}
-                  <Button
-                    variant="outline"
-                    {onclick}
-                    active={commentSourcesExpanded}
-                    title="Choose which comments are shown on the diff">
+              <!-- One control: the arrows walk the comments on the diff and the
+                   count between them opens the filter that decides which are
+                   there to walk. -->
+              <div class="comment-stepper">
+                <Button
+                  variant="naked"
+                  title="Previous comment"
+                  disabled={commentPosition.total === 0}
+                  onclick={() => revisionComponent?.stepComment(-1)}>
+                  <Icon name="arrow-up" />
+                </Button>
+                <Popover
+                  popoverPadding="0"
+                  placement="bottom-start"
+                  bind:expanded={commentSourcesExpanded}>
+                  {#snippet toggle(onclick)}
+                    <Button
+                      variant="outline"
+                      {onclick}
+                      active={commentSourcesExpanded}
+                      title="Choose which comments are shown on the diff">
+                      <div
+                        class="global-flex txt-body-m-regular"
+                        style:gap="0.375rem">
+                        <Icon name="comment" />
+                        <span class="comment-position">
+                          {#if commentPosition.total === 0}
+                            <!-- Hiding every source is the reader's own doing
+                                 and says how to get them back; anything else
+                                 that leaves the diff bare is just "none". -->
+                            {hiddenCommentSources.size === commentSources.length
+                              ? "All hidden"
+                              : "none"}
+                          {:else}
+                            {commentPosition.index >= 0
+                              ? commentPosition.index + 1
+                              : "–"}
+                            of
+                            {commentPosition.total}
+                          {/if}
+                        </span>
+                      </div>
+                    </Button>
+                  {/snippet}
+                  {#snippet popover()}
                     <div
-                      class="global-flex txt-body-m-regular"
-                      style:gap="0.375rem">
-                      <Icon name="comment" />
-                      {visibleCommentCount}
+                      style:border="1px solid var(--color-border-subtle)"
+                      style:border-radius="var(--border-radius-sm)"
+                      style:background-color="var(--color-surface-canvas)">
+                      <DropdownList items={commentSources}>
+                        {#snippet item(source)}
+                          {@const hidden = hiddenCommentSources.has(source.id)}
+                          <DropdownListItem
+                            selected={!hidden}
+                            styleGap="0.5rem"
+                            onclick={() => {
+                              if (hidden) {
+                                hiddenCommentSources.delete(source.id);
+                              } else {
+                                hiddenCommentSources.add(source.id);
+                              }
+                            }}>
+                            <Icon name={hidden ? "eye-slash" : "eye"} />
+                            <span class="avatar-stack">
+                              {#each source.nids.slice(0, 2) as nid (nid)}
+                                <UserAvatar nodeId={nid} styleWidth="1rem" />
+                              {/each}
+                              {#if source.nids.length > 2}
+                                <span class="avatar-overflow">+</span>
+                              {/if}
+                            </span>
+                            <span
+                              style:color={hidden
+                                ? "var(--color-text-tertiary)"
+                                : undefined}>
+                              {source.name}
+                            </span>
+                            <div
+                              class="global-flex"
+                              style:margin-left="auto"
+                              style:padding-left="1rem"
+                              style:color="var(--color-text-tertiary)">
+                              <Icon name="comment" />
+                              {source.count}
+                            </div>
+                          </DropdownListItem>
+                        {/snippet}
+                      </DropdownList>
                     </div>
-                    Comments
-                  </Button>
-                {/snippet}
-                {#snippet popover()}
-                  <div
-                    style:border="1px solid var(--color-border-subtle)"
-                    style:border-radius="var(--border-radius-sm)"
-                    style:background-color="var(--color-surface-canvas)">
-                    <DropdownList items={commentSources}>
-                      {#snippet item(source)}
-                        {@const hidden = hiddenCommentSources.has(source.id)}
-                        <DropdownListItem
-                          selected={!hidden}
-                          styleGap="0.5rem"
-                          onclick={() => {
-                            if (hidden) {
-                              hiddenCommentSources.delete(source.id);
-                            } else {
-                              hiddenCommentSources.add(source.id);
-                            }
-                          }}>
-                          <Icon name={hidden ? "eye-slash" : "eye"} />
-                          <span class="avatar-stack">
-                            {#each source.nids.slice(0, 2) as nid (nid)}
-                              <UserAvatar nodeId={nid} styleWidth="1rem" />
-                            {/each}
-                            {#if source.nids.length > 2}
-                              <span class="avatar-overflow">+</span>
-                            {/if}
-                          </span>
-                          <span
-                            style:color={hidden
-                              ? "var(--color-text-tertiary)"
-                              : undefined}>
-                            {source.name}
-                          </span>
-                          <div
-                            class="global-flex"
-                            style:margin-left="auto"
-                            style:padding-left="1rem"
-                            style:color="var(--color-text-tertiary)">
-                            <Icon name="comment" />
-                            {source.count}
-                          </div>
-                        </DropdownListItem>
-                      {/snippet}
-                    </DropdownList>
-                  </div>
-                {/snippet}
-              </Popover>
+                  {/snippet}
+                </Popover>
+                <Button
+                  variant="naked"
+                  title="Next comment"
+                  disabled={commentPosition.total === 0}
+                  onclick={() => revisionComponent?.stepComment(1)}>
+                  <Icon name="arrow-down" />
+                </Button>
+              </div>
             {/if}
             {#if !onLatestRevision}
               <Button
@@ -1475,7 +1529,8 @@
         chrome={patchHeader}
         {tabs}
         bind:showingRevisionDiff
-        bind:filesExpanded />
+        bind:filesExpanded
+        bind:commentPosition />
     {/snippet}
 
     {#if changesPane}