GITGUD

neiam /gitgud

Git Gud

public · Issues · Pulls · Labels · Forks · Compare · Actions success · Packages

⭐

Log in to mark this repository.

Rework the approval UI around a standing verdict per reviewer

ff90733 · Gabriel Morell · 2026-09-09 22:41

5 files +322 -17
Message 
{commit_body(@commit)}

Files changed

modified lib/git_gud/events/event.ex
+1 −0
@@ -13,6 +13,7 @@ defmodule GitGud.Events.Event do
13 13 labeled unlabeled
14 14 moderated unmoderated
15 15 comment_added comment_edited comment_deleted
16 + reviewed
16 17 )
17 18
18 19 schema "events" do
modified lib/git_gud/pull_requests.ex
+35 −0
@@ -685,6 +685,41 @@ defmodule GitGud.PullRequests do
685 685 end
686 686 end
687 687
688 + @doc """
689 + The standing verdict per reviewer: their most recent review, newest
690 + first.
691 +
692 + Reviews stay append-only — that log *is* the review history — so
693 + "changing your review" means submitting another one, and this is what
694 + decides which one currently counts.
695 +
696 + Federated reviews carry a source actor instead of a local reviewer,
697 + so they group on that rather than collapsing into one anonymous
698 + bucket.
699 + """
700 + def latest_reviews(%PullRequest{} = pr) do
701 + pr.reviews
702 + |> Enum.group_by(&reviewer_key/1)
703 + |> Enum.map(fn {_key, reviews} -> Enum.max_by(reviews, & &1.id) end)
704 + |> Enum.sort_by(& &1.id, :desc)
705 + end
706 +
707 + @doc "The viewer's current review on `pr`, if they have one."
708 + def review_by(%PullRequest{} = pr, %User{id: uid}) do
709 + pr.reviews
710 + |> Enum.filter(&(&1.reviewer_id == uid))
711 + |> case do
712 + [] -> nil
713 + reviews -> Enum.max_by(reviews, & &1.id)
714 + end
715 + end
716 +
717 + def review_by(_pr, _user), do: nil
718 +
719 + defp reviewer_key(%{reviewer_id: id}) when is_integer(id), do: {:user, id}
720 + defp reviewer_key(%{source_actor_id: id}) when is_integer(id), do: {:actor, id}
721 + defp reviewer_key(review), do: {:review, review.id}
722 +
688 723 @doc """
689 724 Insert a PR review that arrived via federation. `attrs` must include
690 725 `:state`, `:activity_url`; `:body` optional. Idempotent on
modified lib/git_gud_web/components/event_components.ex
+19 −0
@@ -143,6 +143,12 @@ defmodule GitGudWeb.EventComponents do
143 143 defp diff_class("-"), do: "bg-error/15 text-error-content"
144 144 defp diff_class(_), do: "opacity-70"
145 145
146 + defp review_word("approved"), do: "approved"
147 + defp review_word("changes_requested"), do: "changes requested"
148 + defp review_word("commented"), do: "commented"
149 + defp review_word(_), do: nil
150 +
151 + defp dot_class("reviewed"), do: "bg-accent"
146 152 defp dot_class(k) when k in ["comment_added", "comment_edited"], do: "bg-info"
147 153 defp dot_class("comment_deleted"), do: "bg-error"
148 154 defp dot_class("merged"), do: "bg-secondary"
@@ -201,6 +207,19 @@ defmodule GitGudWeb.EventComponents do
201 207 defp describe(%{kind: "labeled"}), do: "added a label."
202 208 defp describe(%{kind: "unlabeled"}), do: "removed a label."
203 209
210 + defp describe(%{kind: "reviewed", data: %{"to" => to} = data}) do
211 + assigns = %{to: review_word(to), from: review_word(data["from"])}
212 +
213 + ~H"""
214 + <span :if={@from}>
215 + changed their review from {@from} to <span class="font-medium">{@to}</span>.
216 + </span>
217 + <span :if={is_nil(@from)}>reviewed: <span class="font-medium">{@to}</span>.</span>
218 + """
219 + end
220 +
221 + defp describe(%{kind: "reviewed"}), do: "reviewed this."
222 +
204 223 defp describe(%{kind: "comment_added"}), do: "commented."
205 224 defp describe(%{kind: "comment_edited"}), do: "edited a comment."
206 225 defp describe(%{kind: "comment_deleted"}), do: "deleted a comment."
modified lib/git_gud_web/live/pr_live/show.ex
+90 −17
@@ -133,13 +133,31 @@ defmodule GitGudWeb.PrLive.Show do
133 133 defp assign_comment_form(socket),
134 134 do: assign(socket, :comment_form, to_form(PrComment.changeset(%PrComment{}, %{})))
135 135
136 − defp assign_review_form(socket),
137 − do:
138 − assign(
139 − socket,
140 − :review_form,
141 − to_form(PrReview.changeset(%PrReview{}, %{state: "approved"}))
142 − )
136 + defp assign_review_form(socket) do
137 + mine = my_review(socket.assigns)
138 +
139 + assign(
140 + socket,
141 + :review_form,
142 + to_form(PrReview.changeset(%PrReview{}, %{state: (mine && mine.state) || "approved"}))
143 + )
144 + end
145 +
146 + defp my_review(%{current_scope: %{user: %{} = user}, pr: pr}),
147 + do: PullRequests.review_by(pr, user)
148 +
149 + defp my_review(_assigns), do: nil
150 +
151 + defp can_review?(%{current_scope: %{user: %{}}}), do: true
152 + defp can_review?(_assigns), do: false
153 +
154 + defp review_badge("approved"), do: "badge-success"
155 + defp review_badge("changes_requested"), do: "badge-error"
156 + defp review_badge(_), do: "badge-ghost"
157 +
158 + defp review_word("approved"), do: "approved"
159 + defp review_word("changes_requested"), do: "requested changes"
160 + defp review_word(_), do: "commented"
143 161
144 162 @impl true
145 163 def handle_event("expand_diff_file", params, socket),
@@ -410,11 +428,31 @@ defmodule GitGudWeb.PrLive.Show do
410 428 end
411 429
412 430 def handle_event("add_review", %{"pr_review" => attrs}, socket) do
413 − user = socket.assigns.current_scope.user
431 + case socket.assigns.current_scope && socket.assigns.current_scope.user do
432 + nil ->
433 + {:noreply, put_flash(socket, :error, "Sign in to review.")}
434 +
435 + user ->
436 + previous = my_review(socket.assigns)
437 + pr = socket.assigns.pr
438 +
439 + case PullRequests.add_review(pr, user, attrs) do
440 + {:ok, review} ->
441 + :ok =
442 + Events.record(pr, "reviewed", user, %{
443 + from: previous && previous.state,
444 + to: review.state
445 + })
414 446
415 − case PullRequests.add_review(socket.assigns.pr, user, attrs) do
416 − {:ok, _} -> {:noreply, socket |> assign_review_form() |> reload()}
417 − {:error, cs} -> {:noreply, assign(socket, :review_form, to_form(cs))}
447 + {:noreply,
448 + socket
449 + |> assign(:event_count, Events.count_for(pr))
450 + |> reload()
451 + |> assign_review_form()}
452 +
453 + {:error, cs} ->
454 + {:noreply, assign(socket, :review_form, to_form(cs))}
455 + end
418 456 end
419 457 end
420 458
@@ -619,16 +657,49 @@ defmodule GitGudWeb.PrLive.Show do
619 657
620 658 <section>
621 659 <h2 class="font-semibold mb-2">Reviews</h2>
622 − <ul class="space-y-2">
623 − <li :for={r <- @pr.reviews} class="border border-base-300 rounded p-3 text-sm">
624 − <p class="opacity-60 text-xs mb-1">
625 − {reviewer_name(r.reviewer)} · {r.state} · {format_dt(r.inserted_at)}
660 +
661 + <p :if={@pr.reviews == []} class="text-sm opacity-60">No reviews yet.</p>
662 +
663 + <%!-- The standing verdict per reviewer. Reviews stay
664 + append-only, so this is the latest each of them left. --%>
665 + <ul :if={@pr.reviews != []} class="space-y-2">
666 + <li
667 + :for={r <- PullRequests.latest_reviews(@pr)}
668 + class="border border-base-300 rounded p-3 text-sm"
669 + >
670 + <p class="opacity-60 text-xs mb-1 flex items-center gap-1.5">
671 + <span class={["badge badge-xs", review_badge(r.state)]}>{review_word(r.state)}</span>
672 + {reviewer_name(r.reviewer)} · {format_dt(r.inserted_at)}
626 673 </p>
627 674 <p :if={r.body}>{r.body}</p>
628 675 </li>
629 676 </ul>
630 677
631 − <.form for={@review_form} id="review-form" phx-submit="add_review" class="mt-3 space-y-2">
678 + <details
679 + :if={length(@pr.reviews) > length(PullRequests.latest_reviews(@pr))}
680 + class="mt-2 text-xs"
681 + >
682 + <summary class="cursor-pointer opacity-60 hover:opacity-100 select-none">
683 + Show all {length(@pr.reviews)} reviews
684 + </summary>
685 + <ul class="mt-2 space-y-1">
686 + <li :for={r <- Enum.reverse(@pr.reviews)} class="opacity-70">
687 + <span class={["badge badge-xs", review_badge(r.state)]}>{review_word(r.state)}</span>
688 + {reviewer_name(r.reviewer)} · {format_dt(r.inserted_at)}
689 + </li>
690 + </ul>
691 + </details>
692 +
693 + <.form
694 + :if={can_review?(assigns)}
695 + for={@review_form}
696 + id="review-form"
697 + phx-submit="add_review"
698 + class="mt-3 space-y-2"
699 + >
700 + <p :if={my_review(assigns)} class="text-xs opacity-70">
701 + You {review_word(my_review(assigns).state)}. Submitting again changes your review.
702 + </p>
632 703 <.input
633 704 field={@review_form[:state]}
634 705 type="select"
@@ -640,7 +711,9 @@ defmodule GitGudWeb.PrLive.Show do
640 711 ]}
641 712 />
642 713 <.input field={@review_form[:body]} type="textarea" rows="3" label="Body (optional)" />
643 − <button type="submit" class="btn btn-xs btn-primary">Submit review</button>
714 + <button type="submit" class="btn btn-xs btn-primary">
715 + {if my_review(assigns), do: "Update review", else: "Submit review"}
716 + </button>
644 717 </.form>
645 718 </section>
646 719
added test/git_gud_web/live/pr_live/review_test.exs
+177 −0
@@ -0,0 +1,177 @@
1 +defmodule GitGudWeb.PrLive.ReviewTest do
2 + @moduledoc """
3 + The approval UI: leaving a review, changing your mind, and the way
4 + both land in the PR's history.
5 + """
6 +
7 + use GitGudWeb.ConnCase, async: false
8 +
9 + import Phoenix.LiveViewTest
10 + import GitGud.AccountsFixtures
11 + import GitGud.ForgeFixtures
12 +
13 + alias GitGud.Events
14 + alias GitGud.PullRequests
15 + alias GitGud.Repositories
16 +
17 + defp pr_path(repo, pr) do
18 + handle = Repositories.Storage.repo_handle(GitGud.Repo.preload(repo, [:owner, :organization]))
19 + ~p"/r/#{handle}/#{repo.name}/pulls/#{pr.number}"
20 + end
21 +
22 + defp submit_review(lv, state, body \\ "") do
23 + lv
24 + |> form("#review-form", %{"pr_review" => %{"state" => state, "body" => body}})
25 + |> render_submit()
26 + end
27 +
28 + test "a reviewer can leave a verdict", %{conn: conn} do
29 + {owner, repo} = repository_fixture(%{visibility: "public"})
30 + pr = pr_with_branches(repo, owner)
31 +
32 + reviewer = user_fixture()
33 + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr))
34 +
35 + html = submit_review(lv, "approved", "Looks good")
36 +
37 + assert html =~ "approved"
38 + assert html =~ "Looks good"
39 + end
40 +
41 + test "submitting again changes the standing verdict", %{conn: conn} do
42 + {owner, repo} = repository_fixture(%{visibility: "public"})
43 + pr = pr_with_branches(repo, owner)
44 +
45 + reviewer = user_fixture()
46 + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr))
47 +
48 + _ = submit_review(lv, "approved", "Fine by me")
49 + html = submit_review(lv, "changes_requested", "Actually, hold on")
50 +
51 + # Only the latest counts as this reviewer's verdict.
52 + reloaded = PullRequests.get_pull_request!(repo, pr.number)
53 + assert [latest] = PullRequests.latest_reviews(reloaded)
54 + assert latest.state == "changes_requested"
55 +
56 + # But both rows survive as the log.
57 + assert length(reloaded.reviews) == 2
58 + assert html =~ "Show all 2 reviews"
59 + end
60 +
61 + test "the form says what you already left and offers to update it", %{conn: conn} do
62 + {owner, repo} = repository_fixture(%{visibility: "public"})
63 + pr = pr_with_branches(repo, owner)
64 +
65 + reviewer = user_fixture()
66 + {:ok, lv, html} = live(log_in_user(conn, reviewer), pr_path(repo, pr))
67 +
68 + assert html =~ "Submit review"
69 + refute html =~ "Update review"
70 +
71 + html = submit_review(lv, "changes_requested", "Needs work")
72 +
73 + assert html =~ "You requested changes"
74 + assert html =~ "Update review"
75 + end
76 +
77 + test "two reviewers each keep their own verdict", %{conn: conn} do
78 + {owner, repo} = repository_fixture(%{visibility: "public"})
79 + pr = pr_with_branches(repo, owner)
80 +
81 + one = user_fixture()
82 + two = user_fixture()
83 +
84 + {:ok, lv, _html} = live(log_in_user(conn, one), pr_path(repo, pr))
85 + _ = submit_review(lv, "approved")
86 +
87 + {:ok, lv2, _html} = live(log_in_user(conn, two), pr_path(repo, pr))
88 + _ = submit_review(lv2, "changes_requested")
89 +
90 + reloaded = PullRequests.get_pull_request!(repo, pr.number)
91 + states = reloaded |> PullRequests.latest_reviews() |> Enum.map(& &1.state) |> Enum.sort()
92 +
93 + assert states == ["approved", "changes_requested"]
94 + end
95 +
96 + test "anonymous visitors get no review form", %{conn: conn} do
97 + {owner, repo} = repository_fixture(%{visibility: "public"})
98 + pr = pr_with_branches(repo, owner)
99 +
100 + {:ok, lv, _html} = live(conn, pr_path(repo, pr))
101 +
102 + refute has_element?(lv, "#review-form")
103 + end
104 +
105 + test "an anonymous submission is refused rather than crashing", %{conn: conn} do
106 + {owner, repo} = repository_fixture(%{visibility: "public"})
107 + pr = pr_with_branches(repo, owner)
108 +
109 + {:ok, lv, _html} = live(conn, pr_path(repo, pr))
110 +
111 + render_hook(lv, "add_review", %{"pr_review" => %{"state" => "approved"}})
112 +
113 + assert PullRequests.get_pull_request!(repo, pr.number).reviews == []
114 + end
115 +
116 + test "a first review is recorded in the history", %{conn: conn} do
117 + {owner, repo} = repository_fixture(%{visibility: "public"})
118 + pr = pr_with_branches(repo, owner)
119 +
120 + reviewer = user_fixture()
121 + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr))
122 + _ = submit_review(lv, "approved")
123 +
124 + assert [event] = Events.list_for(pr)
125 + assert event.kind == "reviewed"
126 + assert event.data["to"] == "approved"
127 + assert event.data["from"] == nil
128 +
129 + {:ok, _hist, html} = live(log_in_user(conn, reviewer), pr_path(repo, pr) <> "/history")
130 + assert html =~ "reviewed:"
131 + assert html =~ "approved"
132 + end
133 +
134 + test "changing a review records the transition", %{conn: conn} do
135 + {owner, repo} = repository_fixture(%{visibility: "public"})
136 + pr = pr_with_branches(repo, owner)
137 +
138 + reviewer = user_fixture()
139 + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr))
140 + _ = submit_review(lv, "approved")
141 + _ = submit_review(lv, "changes_requested")
142 +
143 + assert [_first, second] = Events.list_for(pr)
144 + assert second.data["from"] == "approved"
145 + assert second.data["to"] == "changes_requested"
146 +
147 + {:ok, _hist, html} = live(log_in_user(conn, reviewer), pr_path(repo, pr) <> "/history")
148 + assert html =~ "changed their review from"
149 + assert html =~ "changes requested"
150 + end
151 +
152 + test "review events interleave with everything else", %{conn: conn} do
153 + {owner, repo} = repository_fixture(%{visibility: "public"})
154 + pr = pr_with_branches(repo, owner)
155 +
156 + reviewer = user_fixture()
157 + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr))
158 + _ = submit_review(lv, "approved")
159 +
160 + {:ok, owner_lv, _html} = live(log_in_user(conn, owner), pr_path(repo, pr))
161 + _ = owner_lv |> element("button", "Close") |> render_click()
162 +
163 + kinds = pr |> Events.list_for() |> Enum.map(& &1.kind)
164 + assert kinds == ["reviewed", "closed"]
165 + end
166 +
167 + test "the full log stays hidden while every reviewer has one review", %{conn: conn} do
168 + {owner, repo} = repository_fixture(%{visibility: "public"})
169 + pr = pr_with_branches(repo, owner)
170 +
171 + reviewer = user_fixture()
172 + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr))
173 + html = submit_review(lv, "approved")
174 +
175 + refute html =~ "Show all"
176 + end
177 +end

Parents: 51e1414