GITGUD

neiam /gitgud

Git Gud

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

⭐

Log in to mark this repository.

Give pull requests a dedicated code review screen

7360f1e · Gabriel Morell · 2026-09-10 14:33

7 files +418 -81
Message 
{commit_body(@commit)}

Files changed

added lib/git_gud_web/components/pr_nav.ex
+69 −0
@@ -0,0 +1,69 @@
1 +defmodule GitGudWeb.PrNav do
2 + @moduledoc """
3 + Tabs across a pull request's three pages: the conversation, the diff,
4 + and the history.
5 +
6 + The diff moved off the conversation page onto `/files` — reviewing
7 + code and reading the discussion want different amounts of screen, and
8 + a long diff buried the comment box underneath it.
9 + """
10 + use GitGudWeb, :html
11 +
12 + attr :handle, :string, required: true
13 + attr :repo, :map, required: true
14 + attr :pr, :map, required: true
15 + attr :active, :atom, required: true, values: [:conversation, :files, :history]
16 + attr :files_count, :integer, default: nil
17 + attr :comments_count, :integer, default: nil
18 + attr :events_count, :integer, default: nil
19 +
20 + def nav(assigns) do
21 + ~H"""
22 + <nav class="flex gap-1 border-b border-base-300 text-sm">
23 + <.tab
24 + navigate={~p"/r/#{@handle}/#{@repo.name}/pulls/#{@pr.number}"}
25 + active?={@active == :conversation}
26 + label="Conversation"
27 + count={@comments_count}
28 + />
29 + <.tab
30 + navigate={~p"/r/#{@handle}/#{@repo.name}/pulls/#{@pr.number}/files"}
31 + active?={@active == :files}
32 + label="Files changed"
33 + count={@files_count}
34 + />
35 + <.tab
36 + navigate={~p"/r/#{@handle}/#{@repo.name}/pulls/#{@pr.number}/history"}
37 + active?={@active == :history}
38 + label="History"
39 + count={@events_count}
40 + />
41 + </nav>
42 + """
43 + end
44 +
45 + attr :navigate, :string, required: true
46 + attr :active?, :boolean, required: true
47 + attr :label, :string, required: true
48 + attr :count, :integer, default: nil
49 +
50 + defp tab(assigns) do
51 + ~H"""
52 + <.link
53 + navigate={@navigate}
54 + aria-current={@active? && "page"}
55 + class={[
56 + "px-3 py-2 -mb-px border-b-2 transition-colors",
57 + if(@active?,
58 + do: "border-primary font-medium",
59 + else: "border-transparent opacity-70 hover:opacity-100"
60 + )
61 + ]}
62 + >
63 + {@label}<span :if={is_integer(@count) and @count > 0} class="badge badge-xs badge-ghost ml-1">
64 + {@count}
65 + </span>
66 + </.link>
67 + """
68 + end
69 +end
added lib/git_gud_web/live/pr_live/files.ex
+174 −0
@@ -0,0 +1,174 @@
1 +defmodule GitGudWeb.PrLive.Files do
2 + @moduledoc """
3 + The code review screen: a pull request's diff, full width, with the
4 + line comments anchored in it.
5 +
6 + Split out of the conversation page so a long diff no longer pushes
7 + the discussion off the bottom, and so reviewing gets the whole
8 + viewport.
9 + """
10 +
11 + use GitGudWeb, :live_view
12 +
13 + alias GitGud.Events
14 + alias GitGud.PullRequests
15 + alias GitGud.PullRequests.PrComment
16 + alias GitGud.Repositories
17 + alias GitGud.Repositories.Storage
18 +
19 + @impl true
20 + def mount(%{"owner" => owner, "name" => name, "number" => num}, _session, socket) do
21 + repo =
22 + Repositories.get_repository_by_path!(owner, name)
23 + |> GitGud.Repo.preload([:owner, :organization])
24 +
25 + pr = PullRequests.get_pull_request!(repo, String.to_integer(num))
26 + {:ok, stats} = Repositories.get_diff_stats(repo, pr.base_sha, pr.head_sha)
27 + diff_files = load_diff(repo, pr)
28 +
29 + {:ok,
30 + socket
31 + |> assign(:repo, repo)
32 + |> assign(:handle, Storage.repo_handle(repo))
33 + |> GitGudWeb.RepoLive.Header.assign_chrome(repo)
34 + |> assign(:pr, pr)
35 + |> assign(:stats, stats)
36 + |> assign(:diff_files, diff_files)
37 + |> assign(:loaded_diff_idx, GitGudWeb.DiffComponents.default_loaded(diff_files))
38 + |> assign(:commenting_on, nil)
39 + |> assign(:page_title, "Files · ##{pr.number} #{pr.title}")}
40 + end
41 +
42 + defp load_diff(repo, pr) do
43 + case Repositories.get_diff(repo, pr.base_sha, pr.head_sha) do
44 + {:ok, files} -> files
45 + _ -> []
46 + end
47 + end
48 +
49 + defp reload(socket) do
50 + pr = PullRequests.get_pull_request!(socket.assigns.repo, socket.assigns.pr.number)
51 + assign(socket, :pr, pr)
52 + end
53 +
54 + defp viewer(socket), do: socket.assigns.current_scope && socket.assigns.current_scope.user
55 +
56 + defp line_comment?(%{file_path: p, line: l}) when is_binary(p) and is_integer(l), do: true
57 + defp line_comment?(_), do: false
58 +
59 + defp line_comments(pr) do
60 + pr.comments
61 + |> Enum.filter(&line_comment?/1)
62 + |> Enum.reject(&PrComment.deleted?/1)
63 + |> Enum.group_by(&{&1.file_path, &1.line})
64 + end
65 +
66 + defp conversation_count(pr), do: Enum.count(pr.comments, &(not line_comment?(&1)))
67 +
68 + @impl true
69 + def handle_event("expand_diff_file", params, socket),
70 + do: {:noreply, GitGudWeb.DiffComponents.expand(socket, params)}
71 +
72 + def handle_event("comment_on_line", %{"path" => path, "line" => line}, socket) do
73 + if viewer(socket) do
74 + {:noreply, assign(socket, :commenting_on, {path, String.to_integer(line)})}
75 + else
76 + {:noreply, put_flash(socket, :error, "Sign in to comment.")}
77 + end
78 + end
79 +
80 + def handle_event("cancel_line_comment", _params, socket) do
81 + {:noreply, assign(socket, :commenting_on, nil)}
82 + end
83 +
84 + def handle_event("save_line_comment", %{"path" => path, "line" => line, "body" => body}, socket) do
85 + pr = socket.assigns.pr
86 +
87 + case viewer(socket) do
88 + nil ->
89 + {:noreply, put_flash(socket, :error, "Sign in to comment.")}
90 +
91 + user ->
92 + attrs = %{
93 + "body" => String.trim(body),
94 + "file_path" => path,
95 + "line" => String.to_integer(line),
96 + # Pin the head this was written against, so a later push
97 + # doesn't silently re-anchor it to different code.
98 + "commit_sha" => pr.head_sha
99 + }
100 +
101 + case PullRequests.add_comment(pr, user, attrs) do
102 + {:ok, comment} ->
103 + :ok =
104 + Events.record(pr, "comment_added", user, %{
105 + comment_id: comment.id,
106 + file_path: path,
107 + line: attrs["line"]
108 + })
109 +
110 + {:noreply, socket |> assign(:commenting_on, nil) |> reload()}
111 +
112 + {:error, _cs} ->
113 + {:noreply, put_flash(socket, :error, "Could not save that comment.")}
114 + end
115 + end
116 + end
117 +
118 + @impl true
119 + def render(assigns) do
120 + ~H"""
121 + <Layouts.app flash={@flash} current_scope={@current_scope}>
122 + <div class="space-y-4">
123 + <GitGudWeb.RepoLive.Header.header
124 + repo={@repo}
125 + handle={@handle}
126 + current_scope={@current_scope}
127 + has_packages?={@has_packages?}
128 + can_admin?={@can_admin?}
129 + latest_run={@latest_run}
130 + open_pulls={@open_pulls}
131 + />
132 +
133 + <header>
134 + <h1 class="text-2xl font-semibold">
135 + #{@pr.number} — {@pr.title}
136 + </h1>
137 + <p class="text-xs opacity-60 font-mono mt-1">
138 + {@pr.source_ref} → {@pr.target_ref}
139 + </p>
140 + </header>
141 +
142 + <GitGudWeb.PrNav.nav
143 + handle={@handle}
144 + repo={@repo}
145 + pr={@pr}
146 + active={:files}
147 + files_count={length(@diff_files)}
148 + comments_count={conversation_count(@pr)}
149 + events_count={Events.count_for(@pr)}
150 + />
151 +
152 + <section class="flex gap-4 text-sm">
153 + <span><strong>{@stats.files_changed}</strong> files</span>
154 + <span class="text-success">+{@stats.insertions}</span>
155 + <span class="text-error">-{@stats.deletions}</span>
156 + </section>
157 +
158 + <p :if={@diff_files == []} class="py-12 text-center text-sm opacity-60">
159 + No changes to show.
160 + </p>
161 +
162 + <GitGudWeb.DiffComponents.diff
163 + comments={line_comments(@pr)}
164 + can_comment?={@current_scope != nil and @current_scope.user != nil}
165 + commenting_on={@commenting_on}
166 + files={@diff_files}
167 + theme={@editor_theme}
168 + loaded_idx={@loaded_diff_idx}
169 + />
170 + </div>
171 + </Layouts.app>
172 + """
173 + end
174 +end
modified lib/git_gud_web/live/pr_live/history.ex
+12 −7
@@ -52,17 +52,22 @@ defmodule GitGudWeb.PrLive.History do
52 52
53 53 <header>
54 54 <p class="text-xs opacity-60">
55 − <.link
56 − navigate={~p"/r/#{@handle}/#{@repo.name}/pulls/#{@pr.number}"}
57 − class="link link-hover"
58 − >
59 − Back to #{@pr.number}
55 + <.link navigate={~p"/r/#{@handle}/#{@repo.name}/pulls"} class="link link-hover">
56 + Back to pull requests
60 57 </.link>
61 58 </p>
62 − <h1 class="text-2xl font-semibold">History</h1>
63 − <p class="text-sm opacity-70 mt-1">#{@pr.number} — {@pr.title}</p>
59 + <h1 class="text-2xl font-semibold">#{@pr.number} — {@pr.title}</h1>
64 60 </header>
65 61
62 + <GitGudWeb.PrNav.nav
63 + handle={@handle}
64 + repo={@repo}
65 + pr={@pr}
66 + active={:history}
67 + comments_count={Enum.count(@pr.comments, &is_nil(&1.file_path))}
68 + events_count={length(@events)}
69 + />
70 +
66 71 <.timeline events={@events} />
67 72 </div>
68 73 </Layouts.app>
modified lib/git_gud_web/live/pr_live/show.ex
+30 −70
@@ -60,7 +60,6 @@ defmodule GitGudWeb.PrLive.Show do
60 60 |> assign(:comment_body, "")
61 61 |> assign(:editing_title?, false)
62 62 |> assign(:editing_comment_id, nil)
63 − |> assign(:commenting_on, nil)
64 63 |> assign(:title_error, nil)
65 64 |> assign(:editing_body?, false)
66 65 |> assign(:event_count, Events.count_for(pr))
@@ -109,12 +108,12 @@ defmodule GitGudWeb.PrLive.Show do
109 108 defp conversation_comments(pr),
110 109 do: Enum.reject(pr.comments, &line_comment?/1)
111 110
112 − defp line_comments(pr) do
113 − pr.comments
114 − |> Enum.filter(&line_comment?/1)
115 − |> Enum.reject(&PrComment.deleted?/1)
116 − |> Enum.group_by(&{&1.file_path, &1.line})
117 − end
111 + defp line_comment_count(pr),
112 + do:
113 + pr.comments
114 + |> Enum.filter(&line_comment?/1)
115 + |> Enum.reject(&PrComment.deleted?/1)
116 + |> length()
118 117
119 118 defp maybe_probe(pr) do
120 119 if pr.state == "open" do
@@ -262,7 +261,6 @@ defmodule GitGudWeb.PrLive.Show do
262 261 socket
263 262 |> assign(:editing_title?, false)
264 263 |> assign(:editing_comment_id, nil)
265 − |> assign(:commenting_on, nil)
266 264 |> assign(:title_error, nil)
267 265 |> assign(:editing_body?, false)
268 266 |> assign(:event_count, Events.count_for(pr))
@@ -289,56 +287,6 @@ defmodule GitGudWeb.PrLive.Show do
289 287 {:noreply, socket |> assign(:event_count, Events.count_for(pr)) |> reload()}
290 288 end
291 289
292 − def handle_event("comment_on_line", %{"path" => path, "line" => line}, socket) do
293 − if socket.assigns.current_scope && socket.assigns.current_scope.user do
294 − {:noreply, assign(socket, :commenting_on, {path, String.to_integer(line)})}
295 − else
296 − {:noreply, put_flash(socket, :error, "Sign in to comment.")}
297 − end
298 − end
299 −
300 − def handle_event("cancel_line_comment", _params, socket) do
301 − {:noreply, assign(socket, :commenting_on, nil)}
302 − end
303 −
304 − def handle_event("save_line_comment", %{"path" => path, "line" => line, "body" => body}, socket) do
305 − pr = socket.assigns.pr
306 −
307 − case socket.assigns.current_scope && socket.assigns.current_scope.user do
308 − nil ->
309 − {:noreply, put_flash(socket, :error, "Sign in to comment.")}
310 −
311 − user ->
312 − attrs = %{
313 − "body" => String.trim(body),
314 − "file_path" => path,
315 − "line" => String.to_integer(line),
316 − # Pin the comment to the head it was written against, so a
317 − # later push doesn't silently re-anchor it to different code.
318 − "commit_sha" => pr.head_sha
319 − }
320 −
321 − case PullRequests.add_comment(pr, user, attrs) do
322 − {:ok, comment} ->
323 − :ok =
324 − Events.record(pr, "comment_added", user, %{
325 − comment_id: comment.id,
326 − file_path: path,
327 − line: attrs["line"]
328 − })
329 −
330 − {:noreply,
331 − socket
332 − |> assign(:commenting_on, nil)
333 − |> assign(:event_count, Events.count_for(pr))
334 − |> reload()}
335 −
336 − {:error, _cs} ->
337 − {:noreply, put_flash(socket, :error, "Could not save that comment.")}
338 − end
339 − end
340 − end
341 −
342 290 def handle_event("edit_comment", %{"id" => id}, socket) do
343 291 case find_comment(socket, id) do
344 292 nil ->
@@ -378,7 +326,6 @@ defmodule GitGudWeb.PrLive.Show do
378 326 {:noreply,
379 327 socket
380 328 |> assign(:editing_comment_id, nil)
381 − |> assign(:commenting_on, nil)
382 329 |> assign(:event_count, Events.count_for(socket.assigns.pr))
383 330 |> reload()}
384 331
@@ -402,7 +349,6 @@ defmodule GitGudWeb.PrLive.Show do
402 349 {:noreply,
403 350 socket
404 351 |> assign(:editing_comment_id, nil)
405 − |> assign(:commenting_on, nil)
406 352 |> assign(:event_count, Events.count_for(socket.assigns.pr))
407 353 |> reload()}
408 354 else
@@ -722,22 +668,36 @@ defmodule GitGudWeb.PrLive.Show do
722 668 <% end %>
723 669 </section>
724 670
671 + <GitGudWeb.PrNav.nav
672 + handle={@handle}
673 + repo={@repo}
674 + pr={@pr}
675 + active={:conversation}
676 + files_count={length(@diff_files)}
677 + comments_count={length(conversation_comments(@pr))}
678 + events_count={@event_count}
679 + />
680 +
725 681 <section class="flex gap-4 text-sm">
726 682 <span><strong>{@stats.files_changed}</strong> files</span>
727 683 <span class="text-success">+{@stats.insertions}</span>
728 684 <span class="text-error">-{@stats.deletions}</span>
729 685 </section>
730 686
731 − <section>
732 − <h2 class="font-semibold mb-2">Files changed</h2>
733 − <GitGudWeb.DiffComponents.diff
734 − comments={line_comments(@pr)}
735 − can_comment?={@current_scope != nil and @current_scope.user != nil}
736 − commenting_on={@commenting_on}
737 − files={@diff_files}
738 − theme={@editor_theme}
739 − loaded_idx={@loaded_diff_idx}
740 − />
687 + <section :if={@diff_files != []}>
688 + <.link
689 + navigate={~p"/r/#{@handle}/#{@repo.name}/pulls/#{@pr.number}/files"}
690 + class="flex items-center justify-between rounded-lg border border-base-300 p-3 transition-colors hover:border-primary/50"
691 + >
692 + <span class="flex items-center gap-2 text-sm">
693 + <.icon name="hero-document-text" class="size-4 text-primary" />
694 + Review {length(@diff_files)} changed file(s)
695 + <span :if={line_comment_count(@pr) > 0} class="badge badge-xs badge-ghost">
696 + {line_comment_count(@pr)} line comment(s)
697 + </span>
698 + </span>
699 + <.icon name="hero-arrow-right-micro" class="size-4 opacity-60" />
700 + </.link>
741 701 </section>
742 702
743 703 <section :if={@pr.state == "closed"} class="card bg-base-200 p-4 space-y-2">
modified lib/git_gud_web/router.ex
+1 −0
@@ -217,6 +217,7 @@ defmodule GitGudWeb.Router do
217 217 live "/r/:owner/:name/pulls", PrLive.Index, :index
218 218 live "/r/:owner/:name/pulls/:number", PrLive.Show, :show
219 219 live "/r/:owner/:name/pulls/:number/history", PrLive.History, :index
220 + live "/r/:owner/:name/pulls/:number/files", PrLive.Files, :index
220 221 live "/r/:owner/:name/wiki", WikiLive.Index, :index
221 222 live "/r/:owner/:name/wiki/:slug", WikiLive.Show, :show
222 223 live "/r/:owner/:name/actions", WorkflowLive.Index, :index
added test/git_gud_web/live/pr_live/files_page_test.exs
+120 −0
@@ -0,0 +1,120 @@
1 +defmodule GitGudWeb.PrLive.FilesPageTest do
2 + @moduledoc """
3 + The dedicated code review screen, and the tabs between it, the
4 + conversation and the history.
5 + """
6 +
7 + use GitGudWeb.ConnCase, async: false
8 +
9 + import Phoenix.LiveViewTest
10 + import GitGud.ForgeFixtures
11 +
12 + alias GitGud.PullRequests
13 + alias GitGud.Repositories
14 +
15 + defp handle(repo),
16 + do: Repositories.Storage.repo_handle(GitGud.Repo.preload(repo, [:owner, :organization]))
17 +
18 + defp base(repo, pr), do: ~p"/r/#{handle(repo)}/#{repo.name}/pulls/#{pr.number}"
19 +
20 + test "the files page renders the diff", %{conn: conn} do
21 + {user, repo} = repository_fixture()
22 + pr = pr_with_branches(repo, user)
23 +
24 + {:ok, _lv, html} = live(log_in_user(conn, user), base(repo, pr) <> "/files")
25 +
26 + assert html =~ "feature.txt"
27 + assert html =~ "Files changed"
28 + end
29 +
30 + test "the conversation page no longer carries the diff", %{conn: conn} do
31 + {user, repo} = repository_fixture()
32 + pr = pr_with_branches(repo, user)
33 +
34 + {:ok, lv, html} = live(log_in_user(conn, user), base(repo, pr))
35 +
36 + refute has_element?(lv, "button[phx-click=comment_on_line]")
37 + assert html =~ "Review 1 changed file(s)"
38 + end
39 +
40 + test "the summary link leads to the files page", %{conn: conn} do
41 + {user, repo} = repository_fixture()
42 + pr = pr_with_branches(repo, user)
43 +
44 + {:ok, lv, _html} = live(log_in_user(conn, user), base(repo, pr))
45 +
46 + assert {:error, {:live_redirect, %{to: to}}} =
47 + lv |> element("a", "Review 1 changed file(s)") |> render_click()
48 +
49 + assert to == base(repo, pr) <> "/files"
50 + end
51 +
52 + test "the tabs are present on all three pages", %{conn: conn} do
53 + {user, repo} = repository_fixture()
54 + pr = pr_with_branches(repo, user)
55 +
56 + for {suffix, active} <- [
57 + {"", "Conversation"},
58 + {"/files", "Files changed"},
59 + {"/history", "History"}
60 + ] do
61 + {:ok, lv, html} = live(log_in_user(conn, user), base(repo, pr) <> suffix)
62 +
63 + assert html =~ "Conversation"
64 + assert html =~ "Files changed"
65 + assert html =~ "History"
66 + assert has_element?(lv, ~s{a[aria-current="page"]}, active)
67 + end
68 + end
69 +
70 + test "the tab counts reflect the split between the two comment lists", %{conn: conn} do
71 + {user, repo} = repository_fixture()
72 + pr = pr_with_branches(repo, user)
73 +
74 + {:ok, _} = PullRequests.add_comment(pr, user, %{"body" => "plain one"})
75 + {:ok, _} = PullRequests.add_comment(pr, user, %{"body" => "plain two"})
76 +
77 + {:ok, _} =
78 + PullRequests.add_comment(pr, user, %{
79 + "body" => "on a line",
80 + "file_path" => "feature.txt",
81 + "line" => 1,
82 + "commit_sha" => pr.head_sha
83 + })
84 +
85 + {:ok, lv, _html} = live(log_in_user(conn, user), base(repo, pr))
86 +
87 + # Two conversation comments, not three.
88 + assert has_element?(lv, "a", "Conversation")
89 + assert render(lv) =~ "1 line comment(s)"
90 + end
91 +
92 + test "line comments still work from the files page", %{conn: conn} do
93 + {user, repo} = repository_fixture()
94 + pr = pr_with_branches(repo, user)
95 +
96 + {:ok, lv, _html} = live(log_in_user(conn, user), base(repo, pr) <> "/files")
97 +
98 + render_hook(lv, "save_line_comment", %{
99 + "path" => "feature.txt",
100 + "line" => "1",
101 + "body" => "From the files page"
102 + })
103 +
104 + reloaded = PullRequests.get_pull_request!(repo, pr.number)
105 + assert Enum.any?(reloaded.comments, &(&1.body == "From the files page"))
106 + end
107 +
108 + test "an empty diff says so rather than rendering nothing", %{conn: conn} do
109 + {user, repo} = repository_fixture()
110 + pr = pr_with_branches(repo, user)
111 +
112 + # Point both ends at the same tree so there is no diff.
113 + {:ok, flat} =
114 + pr |> Ecto.Changeset.change(base_sha: pr.head_sha) |> GitGud.Repo.update()
115 +
116 + {:ok, _lv, html} = live(log_in_user(conn, user), base(repo, flat) <> "/files")
117 +
118 + assert html =~ "No changes to show."
119 + end
120 +end
modified test/git_gud_web/live/pr_live/line_comment_test.exs
+12 −4
@@ -17,6 +17,11 @@ defmodule GitGudWeb.PrLive.LineCommentTest do
17 17 alias GitGud.Repositories
18 18
19 19 defp pr_path(repo, pr) do
20 + handle = Repositories.Storage.repo_handle(GitGud.Repo.preload(repo, [:owner, :organization]))
21 + ~p"/r/#{handle}/#{repo.name}/pulls/#{pr.number}/files"
22 + end
23 +
24 + defp conversation_path(repo, pr) do
20 25 handle = Repositories.Storage.repo_handle(GitGud.Repo.preload(repo, [:owner, :organization]))
21 26 ~p"/r/#{handle}/#{repo.name}/pulls/#{pr.number}"
22 27 end
@@ -102,11 +107,14 @@ defmodule GitGudWeb.PrLive.LineCommentTest do
102 107 line_comment!(pr, user, "feature.txt", 1, "Inline only")
103 108 {:ok, _} = PullRequests.add_comment(pr, user, %{"body" => "Conversation only"})
104 109
105 − {:ok, _lv, html} = live(log_in_user(conn, user), pr_path(repo, pr))
110 + {:ok, _files, files_html} = live(log_in_user(conn, user), pr_path(repo, pr))
111 + {:ok, _conv, conv_html} = live(log_in_user(conn, user), conversation_path(repo, pr))
106 112
107 − # Both are present, but the inline one appears once — in the diff.
108 − assert html =~ "Conversation only"
109 − assert length(String.split(html, "Inline only")) == 2
113 + # The line comment is in the diff; the plain one is in the conversation.
114 + assert files_html =~ "Inline only"
115 + refute files_html =~ "Conversation only"
116 + assert conv_html =~ "Conversation only"
117 + refute conv_html =~ "Inline only"
110 118 end
111 119
112 120 test "a deleted line comment stops rendering in the diff", %{conn: conn} do

Parents: 1a92edd