diff --git a/src/components/PierreDiff.svelte b/src/components/PierreDiff.svelte index 35c3e6d79..0ee80ae23 100644 --- a/src/components/PierreDiff.svelte +++ b/src/components/PierreDiff.svelte @@ -307,6 +307,21 @@ // but then there are no anchors to make room for either, so it stays zero. const reserveOnHeader = $derived(header !== undefined); + // One pixel between the bottom of the reserved gap and the first file, where a + // sticky bar hangs in that gap with nothing under it. The bar is opaque and + // paints above the diff, so the first card's outset ring — which sits in the + // row above the card — would otherwise be drawn underneath it and the card + // would come out with no top edge. Only needed where the bar is the last thing + // in the gap: with a `filesHeader` below it, that content is transparent and + // the ring shows through. + // + // A pixel is also the most that can safely go on `layout.paddingTop` — see the + // note on it below for why anything larger slides Pierre's render window out of + // step with the screen. + const firstItemGap = $derived( + stickyTop !== undefined && filesHeader === undefined ? 1 : 0, + ); + // The column caps its height at the scroll port's, which it cannot express in // CSS: it hangs off a zero-height anchor, so a percentage has nothing to // resolve against. @@ -901,7 +916,9 @@ // so they drift under their own header and jump in blocks as the window // re-quantises. The header's height has no such gap in it. layout: { - paddingTop: reserveOnHeader ? 0 : stickyTopHeight + filesHeaderHeight, + paddingTop: reserveOnHeader + ? firstItemGap + : stickyTopHeight + filesHeaderHeight + firstItemGap, paddingBottom: 8, gap: 8, }, @@ -1254,6 +1271,13 @@ void canComment; void filesHeaderHeight; void stickyTopHeight; + // The column appearing or going away changes how wide every file card is, + // and with word wrap on that changes how tall its lines are. Nothing in the + // options themselves moves, so this is here for the re-render: rendered + // items reconcile their measured heights into the layout model, and the rest + // do so as they scroll in. + void overlayLeft; + void overlayLeftWidth; const instance = view; if (!instance) { return; diff --git a/src/components/ReviewCommentList.svelte b/src/components/ReviewCommentList.svelte index 94a411abe..4cbfa1c36 100644 --- a/src/components/ReviewCommentList.svelte +++ b/src/components/ReviewCommentList.svelte @@ -9,7 +9,6 @@ import { anchorOf, formatAnchorLines } from "@app/lib/pierreComments"; import { formatResolvedCaption, - pluralize, publicKeyFromDid, truncateId, } from "@app/lib/utils"; @@ -42,11 +41,6 @@ : "Marked as resolved"; } - const threads = $derived(groups.flatMap(group => group.threads)); - const resolved = $derived( - threads.filter(thread => thread.root.resolved === true).length, - ); - /// `NodeId` would be the obvious thing here, but it carries a popover — and /// therefore a button — which cannot live inside the button each card already /// is. The avatar and the alias are the parts that matter at this size. @@ -85,30 +79,12 @@
-
- - {threads.length} - {pluralize("comment", threads.length)} · {resolved} resolved -
- {#each groups as group (group.path)}
{#each group.threads as thread (thread.root.id)} diff --git a/src/components/ReviewPage.svelte b/src/components/ReviewPage.svelte index 93705b1e0..513d54913 100644 --- a/src/components/ReviewPage.svelte +++ b/src/components/ReviewPage.svelte @@ -22,7 +22,12 @@ import type { CodeComments } from "@app/lib/codeComments"; import { resolutionsByComment } from "@app/lib/commentResolutions"; import { diffOptions } from "@app/lib/diffOptions.svelte"; - import { fileDiffPath, fileMetaOf, fullFileLoader } from "@app/lib/diffText"; + import { + fileDiffPath, + fileMetaOf, + fullFileLoader, + gitStatusEntries, + } from "@app/lib/diffText"; import { nodeRunning } from "@app/lib/events"; import { cachedGetDiff, @@ -39,6 +44,7 @@ authorForNodeId, didFromPublicKey, formatTimestamp, + pluralize, publicKeyFromDid, revisionPosition, verdictAction, @@ -48,6 +54,7 @@ import { announce } from "@app/components/AnnounceSwitch.svelte"; import Button from "@app/components/Button.svelte"; + import DiffOptionsButton from "@app/components/DiffOptionsButton.svelte"; import Discussion from "@app/components/Discussion.svelte"; import DropdownList from "@app/components/DropdownList.svelte"; import DropdownListItem from "@app/components/DropdownListItem.svelte"; @@ -57,6 +64,7 @@ import Markdown from "@app/components/Markdown.svelte"; import NodeId from "@app/components/NodeId.svelte"; import PierreDiff from "@app/components/PierreDiff.svelte"; + import PierreTree from "@app/components/PierreTree.svelte"; import Popover, { closeFocused } from "@app/components/Popover.svelte"; import ReviewCommentList from "@app/components/ReviewCommentList.svelte"; @@ -74,17 +82,9 @@ status: Patch["state"]["status"] | undefined; // The tab this review was opened from, so leaving it returns there. fromView?: PatchView; - // An output binding, as on the Changes tab: the topbar renders the toggle - // and reads this, while the collapsing happens here. - filesExpanded?: boolean; - // Whether there is anything to collapse. The view shows only the files a - // comment is anchored in, so a review with none has no file column at all - // and the topbar's toggle would act on nothing. - hasFiles?: boolean; } - /* eslint-disable prefer-const */ - let { + const { config, loadPatch, patch, @@ -95,12 +95,7 @@ rid, status, fromView, - // Only ever written by `setAllFilesCollapsed`, which the topbar calls. - // eslint-disable-next-line no-useless-assignment - filesExpanded = $bindable(true), - hasFiles = $bindable(false), }: Props = $props(); - /* eslint-enable prefer-const */ const isOwnPublishedReview = $derived( review.author.did === didFromPublicKey(config.publicKey), @@ -415,12 +410,15 @@ // nothing about is context they can already read on the Changes tab. const commentedPaths = $derived(new Set(fileGroups.map(g => g.path))); - $effect(() => { - const next = commentedPaths.size > 0; - if (hasFiles !== next) { - hasFiles = next; - } - }); + // A review with no line comments has nothing for the columns or the bar over + // them to be about: what is left is the summary, the discussion and a note + // saying so. It also means the file filter is out of reach there, which is the + // trade for not framing an empty page. + const hasLineComments = $derived(commentedPaths.size > 0); + const lineCommentThreads = $derived(fileGroups.flatMap(g => g.threads)); + const resolvedLineComments = $derived( + lineCommentThreads.filter(thread => thread.root.resolved === true).length, + ); const resolutions = $derived(resolutionsByComment(activity)); function resolvedBy(commentId: string) { @@ -498,9 +496,33 @@ let diffView = $state | undefined>(); - /// Forwarded to the diff so the topbar, which the patch view renders, can - /// collapse or expand every file. - export function setAllFilesCollapsed(collapsed: boolean) { + // Shared by the comment column and the left half of the sticky bar above it, + // so the bar's divider lands on the column's edge. + const commentColumnWidth = "19rem"; + + let filesExpanded = $state(true); + // The column is a way around the review, not part of reading it, so it can be + // put away to give the diff the full width. + let sideColumnShown = $state(true); + // What that column shows: the review's comments, or the files it touches. + let sideColumnMode = $state<"comments" | "tree">("comments"); + // A review is about the lines it comments on, so those files are what the diff + // shows by default; the rest of the revision is a click away for the context it + // does not mention. + let onlyCommentedFiles = $state(true); + + // Left unset to show the whole revision — `PierreDiff` reads no filter at all + // as "every file". + const visiblePaths = $derived( + onlyCommentedFiles ? commentedPaths : undefined, + ); + // The tree lists exactly what the diff is showing, in the diff's own order. + const treeFiles = $derived( + gitStatusEntries(diffFiles).filter( + entry => visiblePaths === undefined || visiblePaths.has(entry.path), + ), + ); + function setAllFilesCollapsed(collapsed: boolean) { filesExpanded = !collapsed; diffView?.setAllCollapsed(collapsed); } @@ -726,14 +748,25 @@ than level with their edges. */ /* Frames the scroll port itself, so the header inside it pins to a border that does not move — the same arrangement as the commits column. */ - .comment-column { + /* A definite height, handed down by the diff: the column has to scroll on its + own once it outgrows the port, and a percentage here resolves against an + indefinite box and is dropped. */ + .side-column { max-height: var(--app-diff-overlay-height, 100%); overflow-y: auto; /* 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); + /* Square at the top, where the header in the sticky bar above continues it. */ + border-radius: 0 0 var(--border-radius-md) var(--border-radius-md); + } + /* The tree scrolls inside itself and stretches to fill the column, so it needs + a height rather than a cap — and no scrolling of its own out here. */ + .side-column.tree { + height: var(--app-diff-overlay-height, 100%); + overflow: hidden; + padding: 0.5rem 0; background-color: var(--color-surface-canvas); } .no-line-comments { @@ -770,6 +803,59 @@ .verdict-reject { color: var(--color-feedback-error-text); } + /* The row above both columns. No rule of its own: the only thing drawn in it + is the comment column's header, so past that header it is empty space with + the controls floating at its right. Opaque, since the diff scrolls under it. */ + .sticky-bar { + display: flex; + align-items: center; + /* The one gap in the row is the one between the two columns, which is what + separates the comment column's header from the file column's first + control. */ + gap: 0.5rem; + height: 2.5rem; + background-color: var(--color-surface-canvas); + } + /* The comment column's heading, which is what it was before it moved up here — + same height, padding and weight, and outlined like the column so the two read + as one box that happens to be split across a sticky boundary. Rounded at the + top only; the column drops its own top corners to meet it. + + The outline is an outset ring, as everything else in this layout is: the + column below is offset by exactly one pixel (see `--app-sticky-gap`), which + is the row this ring's underside occupies, so the two coincide into the one + line that divides the header from the list. */ + .sticky-bar-summary { + flex-shrink: 0; + display: flex; + align-items: center; + gap: 0.5rem; + align-self: stretch; + padding: 0 0.5rem; + border-radius: var(--border-radius-md) var(--border-radius-md) 0 0; + box-shadow: 0 0 0 1px var(--color-border-subtle); + font: var(--txt-body-m-regular); + color: var(--color-text-secondary); + } + .sticky-bar-summary .icon { + display: grid; + width: 1rem; + height: 1rem; + place-items: center; + } + /* Joined, so the pair reads as one control with one of its two sides on — the + same borderless switch the diff settings popover uses for its pairs. */ + .sticky-bar-modes { + display: flex; + align-items: center; + } + .sticky-bar-actions { + display: flex; + align-items: center; + gap: 0.5rem; + margin-left: auto; + padding-left: 0.5rem; + } {#snippet chrome()} @@ -949,7 +1035,7 @@ reactOnComment={codeActions.reactOnComment} flush /> - {#if loadedDiff && commentedPaths.size === 0} + {#if loadedDiff && !hasLineComments}
This review has no line comments.
@@ -957,16 +1043,121 @@
{/snippet} -{#snippet commentColumn()} -
- + +{#snippet stickyBar()} +
+ {#if sideColumnShown} +
+ {#if sideColumnMode === "comments"} + + {lineCommentThreads.length} + {pluralize("comment", lineCommentThreads.length)} · + {resolvedLineComments} resolved + {:else} + + {treeFiles.length} + {pluralize("file", treeFiles.length)} + {/if} +
+ {/if} + + + +
+ + +
+ + {#if commentedPaths.size < diffFiles.length} + + {/if} +
+ + +
{/snippet} +{#snippet sideColumn()} + {#if sideColumnMode === "comments"} +
+ +
+ {:else} + +
+ file.path)} + gitStatus={treeFiles} + onSelect={path => diffView?.scrollToFile(path)} /> +
+ {/if} +{/snippet} + @@ -985,9 +1176,10 @@ ? path => getDiffText(rid, reviewedRevision.base, reviewedRevision.head, 3, path) : undefined} - includePaths={commentedPaths} - overlayLeft={commentedPaths.size > 0 ? commentColumn : undefined} - overlayLeftWidth="19rem" + includePaths={visiblePaths} + overlayLeft={hasLineComments && sideColumnShown ? sideColumn : undefined} + overlayLeftWidth={commentColumnWidth} + stickyTop={hasLineComments ? stickyBar : undefined} codeComments={reviewCodeComments} commentCommit={reviewedRevision?.head} {highlightedCommentId}