From a55a138a7f2485f71d10ec4e4d76031fe92a0f3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C5=ABdolfs=20O=C5=A1i=C5=86=C5=A1?= Date: Wed, 25 Jun 2025 12:16:24 +0200 Subject: [PATCH] Improve code review usability This patch moves the revision list from the tab bar drop-down to a sidebar on wide screens and above the current revision on narrow screens. This way all revisions are visible at all times. We also move and nest the revision reviews into that list so that one can see whether there's anything actionable at a glance. Other tweaks: - added comment counts to revisions and reviews - displayed unresolved comment thread counts per review - swapped icons for accepted/rejected and neutral reviews to avoid clashing with resolved/unresolved comment icons - made label and assignee sections more compact with hover-triggered "+" buttons - added author and status sections to the sidebar - enabled patch state changes from the title bar - made "Review revision" the primary action and "Checkout patch" secondary - added "Draft" label to draft patches - capitalized all badges for consistent UI text --- src/components/AssigneeInput.svelte | 116 +++-- src/components/Changes.svelte | 2 +- src/components/CheckoutPatchButton.svelte | 12 +- src/components/Diff.svelte | 4 +- src/components/FileDiff.svelte | 10 +- src/components/Icon.svelte | 72 ++++ src/components/LabelInput.svelte | 125 ++++-- src/components/PatchMetadata.svelte | 144 +++++++ src/components/PatchStateButton.svelte | 8 +- src/components/PatchStateButtonCompact.svelte | 120 ++++++ src/components/PatchTimeline.svelte | 4 +- src/components/Review.svelte | 10 +- src/components/ReviewTeaser.svelte | 158 ------- src/components/Reviews.svelte | 112 ----- src/components/Revision.svelte | 16 +- src/components/RevisionBadges.svelte | 31 +- src/components/RevisionReviews.svelte | 124 ++++++ src/components/Revisions.svelte | 113 +++++ src/lib/utils.ts | 6 +- src/views/repo/Patch.svelte | 395 +++++++----------- 20 files changed, 933 insertions(+), 649 deletions(-) create mode 100644 src/components/PatchMetadata.svelte create mode 100644 src/components/PatchStateButtonCompact.svelte delete mode 100644 src/components/ReviewTeaser.svelte delete mode 100644 src/components/Reviews.svelte create mode 100644 src/components/RevisionReviews.svelte create mode 100644 src/components/Revisions.svelte diff --git a/src/components/AssigneeInput.svelte b/src/components/AssigneeInput.svelte index 0c244b5ce..126c46107 100644 --- a/src/components/AssigneeInput.svelte +++ b/src/components/AssigneeInput.svelte @@ -94,9 +94,14 @@ -
-
-
Assignees
+
+ + + {#if allowedToEdit} +
+ {#if showInput} + + { + inputValue = ""; + showInput = false; + }} + name="cross" /> + {/if} +
+ {/if} +
+ +{#if showInput} +
+ + {#if !valid && validationMessage} +
+ {validationMessage}
{/if}
+{/if} +{#if updatedAssignees.length > 0}
{#if allowedToEdit} {#each updatedAssignees as assignee} @@ -163,32 +211,10 @@ {/if} {/each} - {#if updatedAssignees.length === 0 && !showInput} -
Not assigned to anyone.
- {/if} {:else} {#each updatedAssignees as assignee} - {:else} -
Not assigned to anyone.
{/each} {/if}
- - {#if showInput} -
- - {#if !valid && validationMessage} -
- {validationMessage} -
- {/if} -
- {/if} -
+{/if} diff --git a/src/components/Changes.svelte b/src/components/Changes.svelte index e748738ad..2db557e2c 100644 --- a/src/components/Changes.svelte +++ b/src/components/Changes.svelte @@ -171,7 +171,7 @@ -
base
+
Base
{#each commits.reverse() as commit} diff --git a/src/components/CheckoutPatchButton.svelte b/src/components/CheckoutPatchButton.svelte index f2fe5330e..80c985812 100644 --- a/src/components/CheckoutPatchButton.svelte +++ b/src/components/CheckoutPatchButton.svelte @@ -2,9 +2,9 @@ import { formatOid } from "@app/lib/utils"; import Border from "@app/components/Border.svelte"; - import Button from "@app/components/Button.svelte"; import Command from "@app/components/Command.svelte"; import Icon from "@app/components/Icon.svelte"; + import NakedButton from "@app/components/NakedButton.svelte"; import Popover from "@app/components/Popover.svelte"; interface Props { @@ -31,13 +31,15 @@ popoverPositionTop="3rem" bind:expanded={popoverExpanded}> {#snippet toggle(onclick)} - + + Checkout patch + {/snippet} {#snippet popover()} toggleCommentExpand(thread.root.id)} /> {:else} toggleCommentExpand(thread.root.id)} /> {/if} {/if} @@ -439,7 +439,7 @@ {#if thread.root.resolved}
added + Added {:else if file.status === "deleted"} - deleted + Deleted {:else if file.status === "moved"} - moved + Moved {:else if file.status === "copied"} - copied + Copied {/if} {/snippet} @@ -120,7 +120,7 @@ {#if commentsOfThisFile && commentsOfThisFile.threads.length > 0} {#if unresolvedThreads > 0}
- + {unresolvedThreads}
{/if} diff --git a/src/components/Icon.svelte b/src/components/Icon.svelte index d551c042f..6778a10b5 100644 --- a/src/components/Icon.svelte +++ b/src/components/Icon.svelte @@ -71,12 +71,15 @@ | "plus" | "reply" | "repo" + | "review" | "revision" | "seedling" | "seedling-filled" | "settings" + | "stop" | "sun" | "thumb-up" + | "unresolve" | "user" | "warning"; } @@ -1163,6 +1166,35 @@ + {:else if name === "review"} + + + + + + + + + + + + + + + + + + + + + + + + + + + + {:else if name === "revision"} @@ -1262,6 +1294,20 @@ + {:else if name === "stop"} + + + + + + + + + + + + + {:else if name === "sun"} @@ -1306,6 +1352,32 @@ + {:else if name === "unresolve"} + + + + + + + + + + + + + + + + + + + + + + + + + {:else if name === "user"} diff --git a/src/components/LabelInput.svelte b/src/components/LabelInput.svelte index 44e1efdfc..42139f092 100644 --- a/src/components/LabelInput.svelte +++ b/src/components/LabelInput.svelte @@ -73,9 +73,14 @@ -
-
-
Labels
+
+ + + {#if allowedToEdit} +
+ {#if showInput} + + { + inputValue = ""; + showInput = false; + }} + name="cross" /> + {/if} +
+ {/if} +
+ +{#if showInput} +
+ + {#if !valid && validationMessage} +
+ {validationMessage}
{/if}
+{/if} +{#if updatedLabels.length > 0}
{#if allowedToEdit} {#each updatedLabels as label} {/each} - {#if updatedLabels.length === 0 && !showInput} -
No labels.
- {/if} {:else} {#each updatedLabels as label}
- - {#if showInput} -
- - {#if !valid && validationMessage} -
- {validationMessage} -
- {/if} -
- {/if} -
+{/if} diff --git a/src/components/PatchMetadata.svelte b/src/components/PatchMetadata.svelte new file mode 100644 index 000000000..e884f8db0 --- /dev/null +++ b/src/components/PatchMetadata.svelte @@ -0,0 +1,144 @@ + + + + +
+
+
Author
+ +
+ +
+
Status
+ { + void saveState(newState); + }} /> +
+ +
+ delegate.did), + patch.author.did, + )} + labels={patch.labels} + submitInProgress={labelSaveInProgress} + save={saveLabels} /> +
+ +
+ delegate.did), + patch.author.did, + )} + assignees={patch.assignees} + submitInProgress={assigneesSaveInProgress} + save={saveAssignees} /> +
+
diff --git a/src/components/PatchStateButton.svelte b/src/components/PatchStateButton.svelte index 27d7a7223..87a3d3f49 100644 --- a/src/components/PatchStateButton.svelte +++ b/src/components/PatchStateButton.svelte @@ -33,6 +33,7 @@ align-items: center; justify-content: center; font-size: var(--font-size-small); + background-color: none; } button:disabled { cursor: inherit; @@ -50,7 +51,12 @@ popoverPositionLeft="0" bind:expanded={popoverExpanded}> {#snippet toggle(onclick)} - + {/snippet} + {#snippet popover()} + + + {#snippet item(state)} + { + onSelect(state); + closeFocused(); + }}> + + + {capitalize(state.status)} + + + {/snippet} + + + {/snippet} + diff --git a/src/components/PatchTimeline.svelte b/src/components/PatchTimeline.svelte index 292e15095..32345cf08 100644 --- a/src/components/PatchTimeline.svelte +++ b/src/components/PatchTimeline.svelte @@ -251,7 +251,7 @@
{#if op.verdict === "accept"}
- +
@@ -262,7 +262,7 @@
{:else if op.verdict === "reject"}
- +
diff --git a/src/components/Review.svelte b/src/components/Review.svelte index 6a593d24f..95cfbd1d0 100644 --- a/src/components/Review.svelte +++ b/src/components/Review.svelte @@ -345,11 +345,15 @@ 's + styleFontWeight="var(--font-weight-medium)" />'s review {#if "draft" in review} - draft + + Draft + {/if} - review {#if "draft" in review}
diff --git a/src/components/ReviewTeaser.svelte b/src/components/ReviewTeaser.svelte deleted file mode 100644 index 97ee08a02..000000000 --- a/src/components/ReviewTeaser.svelte +++ /dev/null @@ -1,158 +0,0 @@ - - - - - -
{ - void push({ - resource: "repo.patch", - rid, - patch: patchId, - status, - reviewId: review.id, - }); - }}> -
- -
-
-
-
- - {#if "draft" in review} - draft review - {:else} - published review - -
- {formatTimestamp(review.timestamp)} -
- {/if} -
- -
- {#if review.labels.length > 0} -
- {#each review.labels as label} -
- {/if} - {#if review.comments.length > 0} -
- {review.comments.length} -
- {/if} -
-
- {#if review.summary?.trim()} -
- -
- {/if} -
-
diff --git a/src/components/Reviews.svelte b/src/components/Reviews.svelte deleted file mode 100644 index 18f51e464..000000000 --- a/src/components/Reviews.svelte +++ /dev/null @@ -1,112 +0,0 @@ - - - - -
-
-
- (hideReviews = !hideReviews)}> - - -
- Reviews - {reviews.length} - -
-
- -
- { - const id = draftReviewStorage.create(rid, revision.id); - - void push({ - resource: "repo.patch", - rid, - patch: patchId, - reviewId: id, - status, - }); - }} - title={hasOwnReview ? "You already published a review" : undefined}> - - Review - -
-
- -
- {#each reviews as review, idx} - - {/each} -
-
diff --git a/src/components/Revision.svelte b/src/components/Revision.svelte index 90813fa0d..59e4c5662 100644 --- a/src/components/Revision.svelte +++ b/src/components/Revision.svelte @@ -2,7 +2,6 @@ import type { Author } from "@bindings/cob/Author"; import type { Config } from "@bindings/config/Config"; import type { Embed } from "@bindings/cob/thread/Embed"; - import type { PatchStatus } from "@app/views/repo/router"; import type { Revision } from "@bindings/cob/patch/Revision"; import type { Thread } from "@bindings/cob/thread/Thread"; @@ -17,7 +16,6 @@ import Changes from "@app/components/Changes.svelte"; import CommentComponent from "@app/components/Comment.svelte"; import Discussion from "@app/components/Discussion.svelte"; - import Reviews from "@app/components/Reviews.svelte"; interface Props { rid: string; @@ -25,19 +23,11 @@ patchId: string; revision: Revision; config: Config; - status: PatchStatus | undefined; loadPatch: () => Promise; } - const { - rid, - repoDelegates, - patchId, - revision, - config, - status, - loadPatch, - }: Props = $props(); + const { rid, repoDelegates, patchId, revision, config, loadPatch }: Props = + $props(); const commentThreads = $derived( ((revision.discussion && @@ -219,8 +209,6 @@
- - -{#if revision.id === revisions.slice(-1)[0].id} - - Latest - -{/if} -{#if revision.id === revisions[0].id} - - Initial - +{#if revisions.length > 1} + {#if revision.id === revisions[0].id} + + Initial + + {:else if revision.id === revisions.slice(-1)[0].id} + + Latest + + {/if} {/if} diff --git a/src/components/RevisionReviews.svelte b/src/components/RevisionReviews.svelte new file mode 100644 index 000000000..3504374ca --- /dev/null +++ b/src/components/RevisionReviews.svelte @@ -0,0 +1,124 @@ + + + + +{#each reviews as review} +
+ { + void push({ + resource: "repo.patch", + rid, + patch: patch.id, + status, + reviewId: review.id, + }); + }}> +
+
+ +
+ + 's + + review + {#if "draft" in review} + + Draft + + {/if} +
+ {#if review.comments.length > 0} + {@const unresolved = unresolvedCommentsCount(review)} + {#if unresolved > 0} +
+ + {unresolved} +
+ {/if} + {#if unresolved === 0 || review.comments.length > unresolved} +
+ {review.comments.length} +
+ {/if} + {/if} +
+
+ +
+{/each} diff --git a/src/components/Revisions.svelte b/src/components/Revisions.svelte new file mode 100644 index 000000000..7ba1b7758 --- /dev/null +++ b/src/components/Revisions.svelte @@ -0,0 +1,113 @@ + + + + +{#each revisionAuthors as author} +
+
+ + 's revisions + +
+ {#each orderBy( revisions.filter(r => { + return r.author.did === author.did; + }), "timestamp", ["asc"], ) as revision} +
+ { + selectRevision(revision); + }}> +
+ {#if patch.state.status === "merged" && patch.state.revision === revision.id} +
+ +
+ {:else} + + {/if} + + {revision.id.substring(0, 4)} + + + + {#if revision.description[0].body.trim()} + {revision.description[0].body} + {:else} + + No description. + + {/if} + +
+ {#if revision.discussion && revision.discussion.length > 0} +
+ {revision.discussion.length} +
+ {/if} +
+
+ +
+ + {/each} +
+{/each} diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 5d15aaa6f..202bc5452 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -241,11 +241,11 @@ export function gravatarURL(email: string): string { export function verdictIcon(verdict: Review["verdict"]) { if (verdict === "accept") { - return "comment-checkmark"; + return "thumb-up"; } else if (verdict === "reject") { - return "comment-cross"; + return "stop"; } else { - return "comment"; + return "review"; } } diff --git a/src/views/repo/Patch.svelte b/src/views/repo/Patch.svelte index 19100d7c7..a79feb67b 100644 --- a/src/views/repo/Patch.svelte +++ b/src/views/repo/Patch.svelte @@ -1,6 +1,5 @@