krz/gitbay

web: merge request list rows show check state and comment count !419

merged merged by cmc on 2026-09-19 02:17 UTC · krz/gitbay:stack-230 into main

8 files changed, +258 −4

Layout: unified · split

.gitbay/wiki/Users.org +4
@@ -420,6 +420,10 @@ its change forward: =mr close 4 --by 7= records it and both pages show
420it, and =mr edit 4 --superseded-by 7|none= sets or clears it 420it, and =mr edit 4 --superseded-by 7|none= sets or clears it
421afterwards, refused on anything but a closed merge request. 421afterwards, refused on anything but a closed merge request.
422 422
423The web list shows each request's combined check state and comment
424count alongside its title, so open work needing attention stands out
425without opening it.
426
423Semantics worth knowing: 427Semantics worth knowing:
424 428
425- the MR head lives in the *target* repository as 429- the MR head lives in the *target* repository as
e2e/mrweb_test.go +49
@@ -238,6 +238,55 @@ func TestMRWebCreate(t *testing.T) {
238 } 238 }
239} 239}
240 240
241// TestMRListRows checks that the merge request list shows each row's
242// combined check state and comment count (#230).
243func TestMRListRows(t *testing.T) {
244 inst := startInstanceWith(t, "[web]\nmode = \"accounts\"\n")
245 aliceKey := inst.newKey(t, "alice")
246 inst.admin(t, "admin", "user", "create", "alice",
247 "--key", aliceKey+".pub", "--email", "alice@example.test", "--verified")
248 if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/lib"); code != 0 {
249 t.Fatalf("repo create: %s", errOut)
250 }
251 env := inst.gitEnv(aliceKey)
252 work := t.TempDir()
253 mustGit(t, work, env, "clone", inst.sshURL("alice/lib"), "w")
254 dir := filepath.Join(work, "w")
255 os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a\n"), 0o644)
256 mustGit(t, dir, env, "checkout", "-q", "-b", "main")
257 mustGit(t, dir, env, "add", ".")
258 mustGit(t, dir, env, "commit", "-q", "-m", "base")
259 mustGit(t, dir, env, "push", "-q", "origin", "main")
260 mustGit(t, dir, env, "checkout", "-q", "-b", "topic")
261 os.WriteFile(filepath.Join(dir, "b.txt"), []byte("b\n"), 0o644)
262 mustGit(t, dir, env, "add", ".")
263 mustGit(t, dir, env, "commit", "-q", "-m", "topic work")
264 mustGit(t, dir, env, "push", "-q", "origin", "topic")
265 sha := strings.TrimSpace(mustGit(t, dir, env, "rev-parse", "topic"))
266
267 if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "create", "alice/lib",
268 "--source", "topic", "--target", "main", "--title", "feature"); code != 0 {
269 t.Fatalf("mr create: %s", errOut)
270 }
271 if _, errOut, code := inst.ssh(t, aliceKey, "", "status", "set", "alice/lib", sha,
272 "--context", "ci/test", "--state", "success"); code != 0 {
273 t.Fatalf("status set: %s", errOut)
274 }
275 if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "comment", "alice/lib", "1",
276 "--message", "hi"); code != 0 {
277 t.Fatalf("mr comment: %s", errOut)
278 }
279
280 alice := inst.login(t, aliceKey)
281 _, body := browserGet(t, alice, inst.base()+"/alice/lib/mrs")
282 if !strings.Contains(body, `class="chip check-success"`) {
283 t.Errorf("no check chip on the list:\n%s", body)
284 }
285 if !strings.Contains(body, `>1 <span class="vh">comments</span>`) {
286 t.Errorf("no comment count on the list:\n%s", body)
287 }
288}
289
241// TestMRWebDiffThreads opens a review thread on a diff line and replies to 290// TestMRWebDiffThreads opens a review thread on a diff line and replies to
242// it from the browser. The CLI's view of the threads afterwards is what 291// it from the browser. The CLI's view of the threads afterwards is what
243// proves the page dispatched mr diff-comment rather than writing its own 292// proves the page dispatched mr diff-comment rather than writing its own
internal/httpd/mrsrow_test.go added +72
@@ -0,0 +1,72 @@
1package httpd
2
3import (
4 "strings"
5 "testing"
6
7 "gitbay.org/gitbay/internal/web"
8)
9
10// mrsPageData mirrors the anonymous struct the mrs handler renders with.
11type mrsPageData struct {
12 repoPage
13 State string
14 Query string
15 Filters []listFilter
16 MRs []mrRow
17 Older string
18}
19
20func renderMRs(t *testing.T, rows []mrRow, state string) string {
21 t.Helper()
22 var sb strings.Builder
23 if err := web.Render(&sb, "mrs.html", mrsPageData{
24 repoPage: testRepoPage(), State: state, MRs: rows,
25 }); err != nil {
26 t.Fatalf("render: %v", err)
27 }
28 return sb.String()
29}
30
31// Each row carries its head's combined check state as a chip linking to
32// the diff, so a reviewer can tell what needs attention without opening
33// every request (#230).
34func TestMRListRowShowsCheck(t *testing.T) {
35 out := renderMRs(t, []mrRow{{MR: testMR("open"), Check: "success"}}, "open")
36 if !strings.Contains(out, `class="chip check-success"`) {
37 t.Errorf("no check chip:\n%s", out)
38 }
39 if !strings.Contains(out, `/krz/gitbay/mrs/42?view=diff`) {
40 t.Errorf("check chip does not link to the diff:\n%s", out)
41 }
42}
43
44// No checks have reported yet: no chip, not an empty one.
45func TestMRListRowHidesEmptyCheck(t *testing.T) {
46 out := renderMRs(t, []mrRow{{MR: testMR("open")}}, "open")
47 if strings.Contains(out, "check-") {
48 t.Errorf("empty check rendered a chip:\n%s", out)
49 }
50}
51
52// The comment count folds conversation and diff-thread comments (see
53// store.MRCommentCounts) and is hidden, not zero, when there are none.
54func TestMRListRowShowsCommentCount(t *testing.T) {
55 out := renderMRs(t, []mrRow{{MR: testMR("open"), Comments: 3}}, "open")
56 if !strings.Contains(out, `title="3 comments"`) {
57 t.Errorf("no plural comment count:\n%s", out)
58 }
59 if !strings.Contains(out, `>3 <span class="vh">comments</span>`) {
60 t.Errorf("no comment count text:\n%s", out)
61 }
62
63 one := renderMRs(t, []mrRow{{MR: testMR("open"), Comments: 1}}, "open")
64 if !strings.Contains(one, `title="1 comment"`) {
65 t.Errorf("comment count not singular for 1:\n%s", one)
66 }
67
68 none := renderMRs(t, []mrRow{{MR: testMR("open")}}, "open")
69 if strings.Contains(none, "vh\">comments</span>") {
70 t.Errorf("zero comments rendered a count:\n%s", none)
71 }
72}
internal/httpd/web.go +29 −2
@@ -1790,6 +1790,15 @@ func (s *Server) canEditItem(r *http.Request, repo store.Repo, author string) bo
1790 return policy.CanWrite(u, repo, grant) 1790 return policy.CanWrite(u, repo, grant)
1791} 1791}
1792 1792
1793// mrRow is one row of the merge request list: the MR plus its head's
1794// combined check state and its comment count. Errors gathering either
1795// fall back to zero values (#230) — the list must still render.
1796type mrRow struct {
1797 store.MR
1798 Check string
1799 Comments int
1800}
1801
1793func (s *Server) mrs(w http.ResponseWriter, r *http.Request) { 1802func (s *Server) mrs(w http.ResponseWriter, r *http.Request) {
1794 p, ok := s.repoFor(w, r, "") 1803 p, ok := s.repoFor(w, r, "")
1795 if !ok { 1804 if !ok {
@@ -1818,15 +1827,33 @@ func (s *Server) mrs(w http.ResponseWriter, r *http.Request) {
1818 mrs = mrs[:listPage] 1827 mrs = mrs[:listPage]
1819 older = olderLink(r, mrs[len(mrs)-1].Number) 1828 older = olderLink(r, mrs[len(mrs)-1].Number)
1820 } 1829 }
1830 shas := make([]string, len(mrs))
1831 ids := make([]int64, len(mrs))
1832 for i, m := range mrs {
1833 shas[i] = m.HeadSHA
1834 ids[i] = m.ID
1835 }
1836 checks, err := s.st.CombinedStatusFor(p.Repo.ID, shas)
1837 if err != nil {
1838 checks = map[string]string{}
1839 }
1840 comments, err := s.st.MRCommentCounts(p.Repo.ID, ids)
1841 if err != nil {
1842 comments = map[int64]int{}
1843 }
1844 rows := make([]mrRow, len(mrs))
1845 for i, m := range mrs {
1846 rows[i] = mrRow{MR: m, Check: checks[m.HeadSHA], Comments: comments[m.ID]}
1847 }
1821 s.render(w, "mrs.html", struct { 1848 s.render(w, "mrs.html", struct {
1822 repoPage 1849 repoPage
1823 State string 1850 State string
1824 Query string 1851 Query string
1825 Filters []listFilter 1852 Filters []listFilter
1826 MRs []store.MR 1853 MRs []mrRow
1827 Older string 1854 Older string
1828 }{p, state, mf.Search, 1855 }{p, state, mf.Search,
1829 activeFilters(state, [][2]string{{"author", mf.Author}, {"milestone", mf.Milestone}}), mrs, older}) 1856 activeFilters(state, [][2]string{{"author", mf.Author}, {"milestone", mf.Milestone}}), rows, older})
1830} 1857}
1831 1858
1832func (s *Server) mr(w http.ResponseWriter, r *http.Request) { 1859func (s *Server) mr(w http.ResponseWriter, r *http.Request) {
internal/store/mrs.go +49
@@ -487,6 +487,55 @@ func (s *Store) ListMRComments(mrID int64) ([]IssueComment, error) {
487 return out, rows.Err() 487 return out, rows.Err()
488} 488}
489 489
490// MRCommentCounts totals, per MR, conversation comments plus diff-thread
491// roots — what the list page shows as one comment count. System comments,
492// diff-thread replies, and pending (unpublished) diff comments do not
493// count. The list handler asks for every row on a page in one call rather
494// than one query per MR.
495func (s *Store) MRCommentCounts(repoID int64, mrIDs []int64) (map[int64]int, error) {
496 out := map[int64]int{}
497 if len(mrIDs) == 0 {
498 return out, nil
499 }
500 ph := "?" + strings.Repeat(",?", len(mrIDs)-1)
501 args := make([]any, 0, len(mrIDs)+1)
502 args = append(args, repoID)
503 for _, id := range mrIDs {
504 args = append(args, id)
505 }
506 add := func(query string) error {
507 rows, err := s.DB.Query(query, args...)
508 if err != nil {
509 return err
510 }
511 defer rows.Close()
512 for rows.Next() {
513 var mrID int64
514 var n int
515 if err := rows.Scan(&mrID, &n); err != nil {
516 return err
517 }
518 out[mrID] += n
519 }
520 return rows.Err()
521 }
522 if err := add(`
523 SELECT c.mr_id, COUNT(*) FROM mr_comments c
524 JOIN merge_requests m ON m.id = c.mr_id
525 WHERE m.repo_id = ? AND c.kind <> 'system' AND c.mr_id IN (` + ph + `)
526 GROUP BY c.mr_id`); err != nil {
527 return nil, err
528 }
529 if err := add(`
530 SELECT c.mr_id, COUNT(*) FROM mr_diff_comments c
531 JOIN merge_requests m ON m.id = c.mr_id
532 WHERE m.repo_id = ? AND c.reply_to IS NULL AND c.pending = 0 AND c.mr_id IN (` + ph + `)
533 GROUP BY c.mr_id`); err != nil {
534 return nil, err
535 }
536 return out, nil
537}
538
490func (s *Store) AddMRReview(mrID, reviewerID int64, verdict, headSHA string) error { 539func (s *Store) AddMRReview(mrID, reviewerID int64, verdict, headSHA string) error {
491 _, err := s.DB.Exec( 540 _, err := s.DB.Exec(
492 "INSERT INTO mr_reviews (mr_id, reviewer_id, verdict, head_sha) VALUES (?, ?, ?, ?)", 541 "INSERT INTO mr_reviews (mr_id, reviewer_id, verdict, head_sha) VALUES (?, ?, ?, ?)",
internal/store/mrs_test.go +50
@@ -184,3 +184,53 @@ func TestPreferredVerifiedEmailDeterministicTiebreak(t *testing.T) {
184 t.Fatalf("tiebreak should be alphabetical: %q, %v", addr, err) 184 t.Fatalf("tiebreak should be alphabetical: %q, %v", addr, err)
185 } 185 }
186} 186}
187
188// MRCommentCounts folds conversation comments and diff-thread roots into
189// one count per MR, for the list page. System comments, diff-thread
190// replies, and pending (unpublished) diff comments do not count.
191func TestMRCommentCounts(t *testing.T) {
192 s, repoID, uid := mrFixture(t)
193 mr1, err := s.MRByNumber(repoID, 1)
194 if err != nil {
195 t.Fatal(err)
196 }
197 if _, err := s.CreateMR(repoID, uid, repoID, "feature2", "main", "t2", "", "def456", "md", false); err != nil {
198 t.Fatal(err)
199 }
200 mr2, err := s.MRByNumber(repoID, 2)
201 if err != nil {
202 t.Fatal(err)
203 }
204
205 if err := s.AddMRComment(mr1.ID, uid, "hi", "md"); err != nil {
206 t.Fatal(err)
207 }
208 if err := s.AddMRSystemComment(mr1.ID, uid, "merged"); err != nil {
209 t.Fatal(err)
210 }
211 rootID, err := s.AddDiffComment(mr1.ID, uid, "abc123", "file.txt", "new", 1, "root", 0, false)
212 if err != nil {
213 t.Fatal(err)
214 }
215 if _, err := s.AddDiffComment(mr1.ID, uid, "abc123", "file.txt", "new", 1, "reply", rootID, false); err != nil {
216 t.Fatal(err)
217 }
218 if _, err := s.AddDiffComment(mr1.ID, uid, "abc123", "file.txt", "new", 2, "pending root", 0, true); err != nil {
219 t.Fatal(err)
220 }
221
222 counts, err := s.MRCommentCounts(repoID, []int64{mr1.ID, mr2.ID})
223 if err != nil {
224 t.Fatal(err)
225 }
226 if counts[mr1.ID] != 2 {
227 t.Fatalf("mr1 count = %d, want 2 (1 comment + 1 diff root)", counts[mr1.ID])
228 }
229 if counts[mr2.ID] != 0 {
230 t.Fatalf("mr2 count = %d, want 0", counts[mr2.ID])
231 }
232
233 if empty, err := s.MRCommentCounts(repoID, nil); err != nil || len(empty) != 0 {
234 t.Fatalf("MRCommentCounts(nil) = %v, %v", empty, err)
235 }
236}
internal/web/static/style.css +1
@@ -906,6 +906,7 @@ ul.issuelist li:hover { background: var(--hover); }
906ul.issuelist li.empty { display: block; } 906ul.issuelist li.empty { display: block; }
907ul.issuelist p { margin: 0; } 907ul.issuelist p { margin: 0; }
908ul.issuelist .issuemain { flex: 1 1 auto; min-width: 0; } 908ul.issuelist .issuemain { flex: 1 1 auto; min-width: 0; }
909ul.issuelist .issueside { display: flex; gap: var(--sp-2); align-items: center; flex: none; }
909ul.issuelist .title a { color: var(--fg); font-weight: 500; } 910ul.issuelist .title a { color: var(--fg); font-weight: 500; }
910ul.issuelist .title a:hover { color: var(--link); } 911ul.issuelist .title a:hover { color: var(--link); }
911 912
internal/web/templates/mrs.html +4 −2
@@ -17,12 +17,14 @@
17</div> 17</div>
18{{if .Viewer}}<p class="meta"><a href="/{{.Repo.OwnerName}}/{{.Repo.Name}}/mrs/new">New merge request</a></p>{{end}} 18{{if .Viewer}}<p class="meta"><a href="/{{.Repo.OwnerName}}/{{.Repo.Name}}/mrs/new">New merge request</a></p>{{end}}
19<ul class="issuelist"> 19<ul class="issuelist">
20{{range .MRs}}<li> 20{{range .MRs}}{{$n := .Number}}<li>
21 <div class="issuemain"> 21 <div class="issuemain">
22 <p class="title"><a href="/{{$.Repo.OwnerName}}/{{$.Repo.Name}}/mrs/{{.Number}}">{{.Title}}</a></p> 22 <p class="title"><a href="/{{$.Repo.OwnerName}}/{{$.Repo.Name}}/mrs/{{.Number}}">{{.Title}}</a></p>
23 <p class="meta">!{{.Number}} by <a href="/{{.Author}}">{{.Author}}</a> · {{if .SourcePath}}{{.SourcePath}}:{{end}}{{.SourceRef}} → {{.TargetRef}}</p> 23 <p class="meta">!{{.Number}} by <a href="/{{.Author}}">{{.Author}}</a> · {{if .SourcePath}}{{.SourcePath}}:{{end}}{{.SourceRef}} → {{.TargetRef}}</p>
24 </div> 24 </div>
25 {{if .Draft}}<span class="chip chip-neutral">draft</span> {{end}}{{if eq $.State "all"}}<span class="chip chip-{{.State}}">{{.State}}</span>{{end}} 25 <div class="issueside">
26 {{if .Draft}}<span class="chip chip-neutral">draft</span> {{end}}{{if eq $.State "all"}}<span class="chip chip-{{.State}}">{{.State}}</span> {{end}}{{with .Check}}<a class="chip check-{{.}}" href="/{{$.Repo.OwnerName}}/{{$.Repo.Name}}/mrs/{{$n}}?view=diff" title="checks: {{.}}">{{.}}</a> {{end}}{{if .Comments}}<span class="chip chip-neutral" title="{{.Comments}} comment{{if ne .Comments 1}}s{{end}}">{{.Comments}} <span class="vh">comments</span></span>{{end}}
27 </div>
26</li> 28</li>
27{{else}}{{if .Query}}<li class="empty">no {{if ne .State "all"}}{{.State}} {{end}}merge requests matching “{{.Query}}”</li> 29{{else}}{{if .Query}}<li class="empty">no {{if ne .State "all"}}{{.State}} {{end}}merge requests matching “{{.Query}}”</li>
28{{else}}<li class="empty">no {{if ne .State "all"}}{{.State}} {{end}}merge requests — open one with <code>gitbay mr create {{.Repo.OwnerName}}/{{.Repo.Name}} --source ... --target {{.Repo.DefaultBranch}}</code></li>{{end}}{{end}} 30{{else}}<li class="empty">no {{if ne .State "all"}}{{.State}} {{end}}merge requests — open one with <code>gitbay mr create {{.Repo.OwnerName}}/{{.Repo.Name}} --source ... --target {{.Repo.DefaultBranch}}</code></li>{{end}}{{end}}