Commit `46a896187a`

46a896187ab1b8bc7f1540f455a311dfdcc6b5ed

parent: 9c7f3c8bef

Verified · cmc ci/build: success ci/test: success ci/vuln: success

cmc <hello@cleberg.net> · 2026-09-04 17:24 UTC

mr: a review is composed and submitted as one thing

A diff comment posted the moment it was written, so a reviewer reading a
change either published half-formed thoughts one at a time — each its own
notification to the author — or kept them somewhere else until they were
done.

`mr diff-comment --pending` holds one back. `mr review` publishes the
batch with the verdict, and says how many went with it; `mr review
--discard` throws away what has not been submitted. The web's compose box
grows an "Add to review" button beside "Comment", and a pending thread is
drawn dashed and says only its author can see it.

Three things follow from pending comments being private:

They notify nobody when written. The review is the announcement, and it
names its own size — "reviewed !3: request_changes, with 4 comment(s)" —
so the person reading the mail knows what they are opening.

They do not gate a merge. An unresolved thread blocks merging, and a
thread only its author can see is one nobody else could resolve, so
leaving pending roots in that count would let any reviewer stall a merge
request invisibly.

Discard deletes only pending rows. A published comment is part of the
conversation and is not something its author can quietly take back.

Second of the three stages #111 asks for. Range-diff is next.

Ref #111

Layout: unified · split

cmd/gitbay/main.go +2 −2

		@@ -500,11 +500,11 @@ func mrCmd() *cobra.Command {
500	500	pass("diff", "show the diff", passOpts{server: []string{"mr", "diff"}, needsRepo: true}),
501	501	local("checkout", "fetch and check out the MR head locally: gitbay mr checkout <n>", cmdMRCheckout),
502	502	pass("comment", "comment on a merge request", passOpts{server: []string{"mr", "comment"}, needsRepo: true, stdinOK: true, editor: "comment"}),
503		pass("diff-comment", "comment on a diff line: --path <f> --line <l> [--old] [--reply <id>]", passOpts{server: []string{"mr", "diff-comment"}, needsRepo: true, stdinOK: true, editor: "comment"}),
	503	pass("diff-comment", "comment on a diff line: --path <f> --line <l> [--old] [--pending] [--reply <id>]", passOpts{server: []string{"mr", "diff-comment"}, needsRepo: true, stdinOK: true, editor: "comment"}),
504	504	pass("threads", "review threads on an MR", passOpts{server: []string{"mr", "threads"}, needsRepo: true}),
505	505	pass("resolve", "resolve a review thread: <n> <thread-id>", passOpts{server: []string{"mr", "resolve"}, needsRepo: true}),
506	506	pass("unresolve", "reopen a review thread: <n> <thread-id>", passOpts{server: []string{"mr", "unresolve"}, needsRepo: true}),
507		pass("review", "review: --approve\|--request-changes\|--comment", passOpts{server: []string{"mr", "review"}, needsRepo: true}),
	507	pass("review", "submit a review: --approve\|--request-changes\|--comment, or --discard a pending batch", passOpts{server: []string{"mr", "review"}, needsRepo: true}),
508	508	pass("merge", "merge: [--strategy ff\|merge\|squash\|rebase]", passOpts{server: []string{"mr", "merge"}, needsRepo: true}),
509	509	pass("close", "close without merging", passOpts{server: []string{"mr", "close"}, needsRepo: true}),
510	510	pass("draft", "mark as work in progress", passOpts{server: []string{"mr", "draft"}, needsRepo: true}),

e2e/pendingreview_test.go added +149

		@@ -0,0 +1,149 @@
	1	package e2e
	2
	3	import (
	4	"encoding/json"
	5	"os"
	6	"path/filepath"
	7	"strings"
	8	"testing"
	9	)
	10
	11	// TestPendingReviewBatch is #111's second stage: a reviewer composes a
	12	// review and submits it as a unit, instead of every comment landing in
	13	// the author's inbox the moment it is typed.
	14	func TestPendingReviewBatch(t *testing.T) {
	15	inst := startInstance(t)
	16	authorKey := inst.newKey(t, "author")
	17	reviewerKey := inst.newKey(t, "reviewer")
	18	inst.admin(t, "admin", "user", "create", "author", "--key", authorKey+".pub")
	19	inst.admin(t, "admin", "user", "create", "reviewer", "--key", reviewerKey+".pub")
	20	if _, errOut, code := inst.ssh(t, authorKey, "", "repo", "create", "author/lib"); code != 0 {
	21	t.Fatalf("repo create: %s", errOut)
	22	}
	23	if _, _, code := inst.ssh(t, authorKey, "", "repo", "access", "grant", "author/lib", "reviewer", "write"); code != 0 {
	24	t.Fatal("grant failed")
	25	}
	26	if _, _, code := inst.ssh(t, authorKey, "", "repo", "settings", "require-resolved", "author/lib", "on"); code != 0 {
	27	t.Fatal("require-resolved failed")
	28	}
	29
	30	env := inst.gitEnv(authorKey)
	31	work := t.TempDir()
	32	mustGit(t, work, env, "clone", inst.sshURL("author/lib"), "w")
	33	dir := filepath.Join(work, "w")
	34	os.WriteFile(filepath.Join(dir, "a.go"), []byte("package lib\n"), 0o644)
	35	mustGit(t, dir, env, "checkout", "-q", "-b", "main")
	36	mustGit(t, dir, env, "add", ".")
	37	mustGit(t, dir, env, "commit", "-q", "-m", "base")
	38	mustGit(t, dir, env, "push", "-q", "origin", "main")
	39	mustGit(t, dir, env, "checkout", "-q", "-b", "feat")
	40	os.WriteFile(filepath.Join(dir, "a.go"), []byte("package lib\n\nvar A = 1\nvar B = 2\n"), 0o644)
	41	mustGit(t, dir, env, "add", ".")
	42	mustGit(t, dir, env, "commit", "-q", "-m", "add A and B")
	43	mustGit(t, dir, env, "push", "-q", "origin", "feat")
	44	if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "create", "author/lib",
	45	"--source", "feat", "--target", "main", "--title", "'add vars'"); code != 0 {
	46	t.Fatalf("mr create: %s", errOut)
	47	}
	48
	49	// Two comments held back.
	50	for _, line := range []string{"3", "4"} {
	51	if _, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "diff-comment", "author/lib", "1",
	52	"--path", "a.go", "--line", line, "--pending", "--message", "'name it better'"); code != 0 {
	53	t.Fatalf("pending comment on line %s: %s", line, errOut)
	54	}
	55	}
	56
	57	// The author sees none of it, and their inbox is untouched.
	58	if out := threadsFor(t, inst, authorKey, "1"); len(out) != 0 {
	59	t.Fatalf("author sees unsubmitted comments: %v", out)
	60	}
	61	if got := inbox(t, inst, authorKey); strings.Contains(got, "commented on") {
	62	t.Fatalf("an unsubmitted comment reached the author's inbox:\n%s", got)
	63	}
	64	// The reviewer sees their own.
	65	if out := threadsFor(t, inst, reviewerKey, "1"); len(out) != 2 {
	66	t.Fatalf("reviewer sees %d of their own pending threads, want 2", len(out))
	67	}
	68
	69	// And an unsubmitted thread must not gate the merge: nobody else can
	70	// see it, so nobody else could resolve it.
	71	if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "1"); code != 0 {
	72	t.Fatalf("pending thread blocked a merge: %s", errOut)
	73	}
	74
	75	// Same again on a second MR, this time submitted.
	76	mustGit(t, dir, env, "checkout", "-q", "-b", "feat2", "main")
	77	os.WriteFile(filepath.Join(dir, "b.go"), []byte("package lib\n\nvar C = 3\n"), 0o644)
	78	mustGit(t, dir, env, "add", ".")
	79	mustGit(t, dir, env, "commit", "-q", "-m", "add C")
	80	mustGit(t, dir, env, "push", "-q", "origin", "feat2")
	81	if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "create", "author/lib",
	82	"--source", "feat2", "--target", "main", "--title", "'add C'"); code != 0 {
	83	t.Fatalf("mr create: %s", errOut)
	84	}
	85	if _, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "diff-comment", "author/lib", "2",
	86	"--path", "b.go", "--line", "3", "--pending", "--message", "'C needs a doc comment'"); code != 0 {
	87	t.Fatalf("pending comment: %s", errOut)
	88	}
	89	out, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "review", "author/lib", "2", "--request-changes", "--json")
	90	if code != 0 {
	91	t.Fatalf("review: %s", errOut)
	92	}
	93	if !strings.Contains(out, `"published":1`) {
	94	t.Fatalf("review did not publish the batch: %s", out)
	95	}
	96	if n := len(threadsFor(t, inst, authorKey, "2")); n != 1 {
	97	t.Fatalf("author sees %d threads after the review, want 1", n)
	98	}
	99	// One notification for the review, carrying the count — not one per
	100	// comment as they were written.
	101	got := inbox(t, inst, authorKey)
	102	if !strings.Contains(got, "with 1 comment") {
	103	t.Fatalf("review notification does not mention the batch:\n%s", got)
	104	}
	105	// Now it gates.
	106	if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "2"); code != 4 \|\|
	107	!strings.Contains(errOut, "threads resolved") {
	108	t.Fatalf("published thread did not gate: exit %d, %s", code, errOut)
	109	}
	110
	111	// Discard throws away only what has not been submitted.
	112	if _, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "diff-comment", "author/lib", "2",
	113	"--path", "b.go", "--line", "1", "--pending", "--message", "'never mind'"); code != 0 {
	114	t.Fatalf("pending comment: %s", errOut)
	115	}
	116	out, errOut, code = inst.ssh(t, reviewerKey, "", "mr", "review", "author/lib", "2", "--discard", "--json")
	117	if code != 0 \|\| !strings.Contains(out, `"discarded":1`) {
	118	t.Fatalf("discard: exit %d, %s, %s", code, errOut, out)
	119	}
	120	if n := len(threadsFor(t, inst, reviewerKey, "2")); n != 1 {
	121	t.Fatalf("discard removed a published comment: %d threads remain", n)
	122	}
	123	// A verdict and a discard together is a usage error, not a guess.
	124	if _, _, code := inst.ssh(t, reviewerKey, "", "mr", "review", "author/lib", "2", "--approve", "--discard"); code != 2 {
	125	t.Fatalf("--approve --discard exit %d, want 2", code)
	126	}
	127	}
	128
	129	// threadsFor returns the diff-comment thread ids this account can see on
	130	// one merge request. Pending threads belong to their author alone, so who
	131	// asks changes the answer — which is the whole point.
	132	func threadsFor(t testing.T, inst instance, key, n string) []int64 {
	133	t.Helper()
	134	out, errOut, code := inst.ssh(t, key, "", "mr", "threads", "author/lib", n, "--json")
	135	if code != 0 {
	136	t.Fatalf("mr threads: %s", errOut)
	137	}
	138	var env struct {
	139	Data []struct {
	140	ID int64 `json:"id"`
	141	} `json:"data"`
	142	}
	143	json.Unmarshal([]byte(out), &env)
	144	var ids []int64
	145	for _, d := range env.Data {
	146	ids = append(ids, d.ID)
	147	}
	148	return ids
	149	}

internal/control/diffcomment.go +24 −12

		@@ -17,7 +17,7 @@ import (
17	17	func init() {
18	18	register(Command{Path: []string{"mr", "diff-comment"},
19	19	Summary: "comment on a diff line",
20		Usage: "mr diff-comment <owner/name> <n> --path <file> --line <l> [--old] [--reply <id>] [--message <m> \| --file -]",
	20	Usage: "mr diff-comment <owner/name> <n> --path <file> --line <l> [--old] [--pending] [--reply <id>] [--message <m> \| --file -]",
21	21	ReadsStdin: true, Run: runDiffComment})
22	22	register(Command{Path: []string{"mr", "threads"},
23	23	Summary: "review threads on an MR",
		@@ -31,8 +31,9 @@ func init() {
31	31	}
32	32
33	33	func runDiffComment(c *Ctx, args []string) int {
34		f, err := parseFlags(args, flagSpec{Values: []string{"--path", "--line", "--reply", "--message", "--file"}, Bools: []string{"--old"}, MaxPos: -1,
35		Usage: "mr diff-comment <owner/name> <n> --path <file> --line <l> [--old] [--reply <id>] [--message <m> \| --file -]"})
	34	f, err := parseFlags(args, flagSpec{Values: []string{"--path", "--line", "--reply", "--message", "--file"},
	35	Bools: []string{"--old", "--pending"}, MaxPos: -1,
	36	Usage: "mr diff-comment <owner/name> <n> --path <file> --line <l> [--old] [--pending] [--reply <id>] [--message <m> \| --file -]"})
36	37	if err != nil {
37	38	return c.fail(protocol.ExitUsage, "%v", err)
38	39	}
		@@ -95,24 +96,35 @@ func runDiffComment(c *Ctx, args []string) int {
95	96	}
96	97	}
97	98
98		id, err := c.Store.AddDiffComment(mr.ID, c.User.ID, mr.HeadSHA, path, side, line, body, replyTo)
	99	pending := f.Has("--pending")
	100	id, err := c.Store.AddDiffComment(mr.ID, c.User.ID, mr.HeadSHA, path, side, line, body, replyTo, pending)
99	101	if err != nil {
100	102	if errors.Is(err, store.ErrNotFound) {
101	103	return c.fail(protocol.ExitNotFound, "%v", err)
102	104	}
103	105	return c.failErr(err)
104	106	}
105		if parts, err := c.Store.MRParticipants(mr.ID); err == nil {
106		notify(c, parts, notice{repo: repo, kind: "mr",
107		subject: mrSubject(repo, mr.Number, mr.Title),
108		action: fmt.Sprintf("commented on %s:%d in !%d", path, line, mr.Number),
109		excerpt: body, path: fmt.Sprintf("%s/mrs/%d", repo.Path(), mr.Number)})
	107	// A pending comment is not part of the conversation yet, so it does
	108	// not reach anyone's inbox. `mr review` is what says it out loud.
	109	if !pending {
	110	if parts, err := c.Store.MRParticipants(mr.ID); err == nil {
	111	notify(c, parts, notice{repo: repo, kind: "mr",
	112	subject: mrSubject(repo, mr.Number, mr.Title),
	113	action: fmt.Sprintf("commented on %s:%d in !%d", path, line, mr.Number),
	114	excerpt: body, path: fmt.Sprintf("%s/mrs/%d", repo.Path(), mr.Number)})
	115	}
110	116	}
111		return c.emit(map[string]any{"id": id, "thread": firstNonZero(replyTo, id)}, func(w io.Writer) {
	117	return c.emit(map[string]any{"id": id, "thread": firstNonZero(replyTo, id), "pending": pending}, func(w io.Writer) {
	118	what := "thread %d opened on %s:%d in %s!%d\n"
112	119	if replyTo != 0 {
113	120	fmt.Fprintf(w, "replied to thread %d on %s!%d\n", replyTo, repo.Path(), mr.Number)
114	121	} else {
115		fmt.Fprintf(w, "thread %d opened on %s:%d in %s!%d\n", id, path, line, repo.Path(), mr.Number)
	122	fmt.Fprintf(w, what, id, path, line, repo.Path(), mr.Number)
	123	}
	124	if pending {
	125	n := c.Store.CountPendingComments(mr.ID, c.User.ID)
	126	fmt.Fprintf(w, "pending: %d comment(s) in this review, submit with `gitbay mr review %s %d --comment`\n",
	127	n, repo.Path(), mr.Number)
116	128	}
117	129	})
118	130	}
		@@ -132,7 +144,7 @@ func runMRThreads(c *Ctx, args []string) int {
132	144	if len(args) != 2 {
133	145	return c.fail(protocol.ExitUsage, "usage: mr threads <owner/name> <n>")
134	146	}
135		comments, err := c.Store.ListDiffComments(mr.ID)
	147	comments, err := c.Store.ListDiffComments(mr.ID, c.User.ID)
136	148	if err != nil {
137	149	return c.fail(protocol.ExitFailure, "%v", err)
138	150	}

internal/control/mr.go +47 −10

		@@ -67,7 +67,7 @@ func init() {
67	67	ReadsStdin: true, Run: runMRComment})
68	68	register(Command{Path: []string{"mr", "review"},
69	69	Summary: "review",
70		Usage: "mr review <owner/name> <n> --approve\|--request-changes\|--comment", Run: runMRReview})
	70	Usage: "mr review <owner/name> <n> --approve\|--request-changes\|--comment\|--discard", Run: runMRReview})
71	71	register(Command{Path: []string{"mr", "merge"},
72	72	Summary: "merge",
73	73	Usage: "mr merge <owner/name> <n> [--strategy ff\|merge\|squash\|rebase]", Run: runMRMerge})
		@@ -319,9 +319,9 @@ func runMRCreate(c *Ctx, args []string) int {
319	319	}
320	320
321	321	type mrOut struct {
322		Number int64 `json:"number"`
323		Title string `json:"title"`
324		State string `json:"state"`
	322	Number int64 `json:"number"`
	323	Title string `json:"title"`
	324	State string `json:"state"`
325	325	// Draft is an open merge request not asking to be merged yet.
326	326	Draft bool `json:"draft,omitempty"`
327	327	Author string `json:"author"`
		@@ -690,7 +690,7 @@ func runMRComment(c *Ctx, args []string) int {
690	690	}
691	691
692	692	func runMRReview(c *Ctx, args []string) int {
693		verdict := ""
	693	verdict, discard := "", false
694	694	var rest []string
695	695	for _, a := range args {
696	696	switch a {
		@@ -700,12 +700,18 @@ func runMRReview(c *Ctx, args []string) int {
700	700	verdict = "request_changes"
701	701	case "--comment":
702	702	verdict = "comment"
	703	case "--discard":
	704	discard = true
703	705	default:
704	706	rest = append(rest, a)
705	707	}
706	708	}
707		if verdict == "" {
708		return c.fail(protocol.ExitUsage, "usage: mr review <owner/name> <n> --approve\|--request-changes\|--comment")
	709	const usage = "mr review <owner/name> <n> --approve\|--request-changes\|--comment\|--discard"
	710	if discard && verdict != "" {
	711	return c.fail(protocol.ExitUsage, "--discard throws the batch away; it takes no verdict")
	712	}
	713	if verdict == "" && !discard {
	714	return c.fail(protocol.ExitUsage, "usage: %s", usage)
709	715	}
710	716	repo, mr, code := mrRef(c, rest, policy.CanRead)
711	717	if code >= 0 {
		@@ -717,19 +723,40 @@ func runMRReview(c *Ctx, args []string) int {
717	723	if mr.State != "open" {
718	724	return c.fail(protocol.ExitUsage, "MR !%d is %s", mr.Number, mr.State)
719	725	}
	726	// Throwing the batch away is not a review, so it stops here: no
	727	// verdict, no event, nobody told about comments nobody ever saw.
	728	if discard {
	729	n, err := c.Store.DiscardPendingComments(mr.ID, c.User.ID)
	730	if err != nil {
	731	return c.fail(protocol.ExitFailure, "%v", err)
	732	}
	733	return c.emit(map[string]any{"number": mr.Number, "discarded": n}, func(w io.Writer) {
	734	fmt.Fprintf(w, "discarded %d pending comment(s) on %s!%d\n", n, repo.Path(), mr.Number)
	735	})
	736	}
720	737	if err := c.Store.AddMRReview(mr.ID, c.User.ID, verdict, mr.HeadSHA); err != nil {
721	738	return c.fail(protocol.ExitFailure, "%v", err)
722	739	}
	740	// The batch the reviewer composed becomes visible with the verdict,
	741	// which is what makes it one review rather than a trickle.
	742	published, err := c.Store.PublishPendingComments(mr.ID, c.User.ID)
	743	if err != nil {
	744	return c.fail(protocol.ExitFailure, "%v", err)
	745	}
723	746	c.Store.RecordEvent(repo.ID, c.User.ID, "mr.reviewed",
724	747	fmt.Sprintf(`{"number":%d,"verdict":%q}`, mr.Number, verdict))
725	748	if parts, err := c.Store.MRParticipants(mr.ID); err == nil {
726	749	notify(c, parts, notice{repo: repo, kind: "mr",
727	750	subject: mrSubject(repo, mr.Number, mr.Title),
728		action: fmt.Sprintf("reviewed !%d: %s", mr.Number, verdict),
	751	action: reviewAction(mr.Number, verdict, published),
729	752	path: fmt.Sprintf("%s/mrs/%d", repo.Path(), mr.Number)})
730	753	}
731		return c.emit(map[string]any{"number": mr.Number, "verdict": verdict}, func(w io.Writer) {
732		fmt.Fprintf(w, "reviewed %s!%d: %s\n", repo.Path(), mr.Number, verdict)
	754	return c.emit(map[string]any{"number": mr.Number, "verdict": verdict, "published": published}, func(w io.Writer) {
	755	fmt.Fprintf(w, "reviewed %s!%d: %s", repo.Path(), mr.Number, verdict)
	756	if published > 0 {
	757	fmt.Fprintf(w, " (%d comment(s))", published)
	758	}
	759	fmt.Fprintln(w)
733	760	})
734	761	}
735	762
		@@ -1269,3 +1296,13 @@ func runMRClose(c *Ctx, args []string) int {
1269	1296	fmt.Fprintf(w, "closed %s!%d\n", repo.Path(), mr.Number)
1270	1297	})
1271	1298	}
	1299
	1300	// reviewAction is what a review notification says it was. A verdict with
	1301	// a batch behind it is a different thing from a bare verdict, and the
	1302	// person reading the mail is deciding whether to open it.
	1303	func reviewAction(number int64, verdict string, published int64) string {
	1304	if published > 0 {
	1305	return fmt.Sprintf("reviewed !%d: %s, with %d comment(s)", number, verdict, published)
	1306	}
	1307	return fmt.Sprintf("reviewed !%d: %s", number, verdict)
	1308	}

internal/httpd/mractions.go +5

		@@ -107,6 +107,11 @@ func (s Server) mrDiffCommentSubmit(w http.ResponseWriter, r http.Request, u s
107	107	extra = append(extra, "--old")
108	108	}
109	109	}
	110	// "Add to review" holds the comment back until the verdict; "Comment"
	111	// posts it now, which is what the form did before there was a choice.
	112	if r.FormValue("pending") == "on" {
	113	extra = append(extra, "--pending")
	114	}
110	115	msg, code := s.runControlStdinCode(u, mrArgs(r, "diff-comment", append(extra, "--file", "-")...), body)
111	116	s.done(w, r, code, msg, s.mrDiffRedirect)
112	117	}

internal/httpd/web.go +11 −4

		@@ -1257,9 +1257,13 @@ func renderReadme(name string, raw []byte) template.HTML {
1257	1257	}
1258	1258
1259	1259	type diffThread struct {
1260		ID int64
1261		Resolved string
1262		Stale bool
	1260	ID int64
	1261	Resolved string
	1262	Stale bool
	1263	// Pending marks a thread in the viewer's own unsubmitted review. Only
	1264	// they are shown it, and the page says so, since it looks exactly
	1265	// like a posted one otherwise.
	1266	Pending bool
1263	1267	CanResolve bool
1264	1268	Comments []renderedComment
1265	1269	}
		@@ -1294,6 +1298,7 @@ func attachThreads(files []diffFile, comments []store.DiffComment, headSHA strin
1294	1298	for _, cm := range comments {
1295	1299	if cm.ReplyTo == 0 {
1296	1300	threads[cm.ID] = &diffThread{ID: cm.ID, Resolved: cm.ResolvedBy, Stale: cm.HeadSHA != headSHA,
	1301	Pending: cm.Pending,
1297	1302	CanResolve: rights.canResolve(cm.Author),
1298	1303	Comments: []renderedComment{{Author: cm.Author, CreatedAt: cm.CreatedAt, BodyHTML: md(cm.Body, "md")}}}
1299	1304	anchors[cm.ID] = anchor{cm.Path, cm.Side, cm.Line}
		@@ -1742,7 +1747,9 @@ func (s Server) mr(w http.ResponseWriter, r http.Request) {
1742	1747	comments, _ := s.st.ListMRComments(m.ID)
1743	1748	reviews, _ := s.st.ListMRReviews(m.ID)
1744	1749	checks, combined, _ := s.st.ChecksForCommit(p.Repo.ID, m.HeadSHA)
1745		diffComments, _ := s.st.ListDiffComments(m.ID)
	1750	// The viewer sees their own unsubmitted review comments and nobody
	1751	// else's.
	1752	diffComments, _ := s.st.ListDiffComments(m.ID, s.webViewer(r).ID)
1746	1753
1747	1754	headRef := fmt.Sprintf("refs/merge-requests/%d/head", m.Number)
1748	1755	var files []diffFile

internal/store/diffcomments.go +59 −13

		@@ -16,12 +16,15 @@ type DiffComment struct {
16	16	Body string
17	17	ReplyTo int64 // 0 for thread roots
18	18	ResolvedBy string
19		CreatedAt string
	19	// Pending marks a comment in a review its author has not submitted.
	20	// Only they can see it, and `mr review` publishes it.
	21	Pending bool
	22	CreatedAt string
20	23	}
21	24
22	25	// AddDiffComment creates a thread root (replyTo 0) or a reply. Replies
23	26	// inherit the root's anchor and must belong to the same MR.
24		func (s *Store) AddDiffComment(mrID, authorID int64, headSHA, path, side string, line int64, body string, replyTo int64) (int64, error) {
	27	func (s *Store) AddDiffComment(mrID, authorID int64, headSHA, path, side string, line int64, body string, replyTo int64, pending bool) (int64, error) {
25	28	if replyTo != 0 {
26	29	var rootMR int64
27	30	var rootReply sql.NullInt64
		@@ -51,25 +54,27 @@ func (s *Store) AddDiffComment(mrID, authorID int64, headSHA, path, side string,
51	54	reply = replyTo
52	55	}
53	56	res, err := s.DB.Exec(`
54		INSERT INTO mr_diff_comments (mr_id, author_id, head_sha, path, side, line, body, reply_to)
55		VALUES (?, ?, ?, ?, ?, ?, ?, ?)`,
56		mrID, authorID, headSHA, path, side, line, body, reply)
	57	INSERT INTO mr_diff_comments (mr_id, author_id, head_sha, path, side, line, body, reply_to, pending)
	58	VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`,
	59	mrID, authorID, headSHA, path, side, line, body, reply, pending)
57	60	if err != nil {
58	61	return 0, err
59	62	}
60	63	return res.LastInsertId()
61	64	}
62	65
63		// ListDiffComments returns every diff comment on an MR, roots and replies,
64		// oldest first.
65		func (s *Store) ListDiffComments(mrID int64) ([]DiffComment, error) {
	66	// ListDiffComments returns the diff comments on an MR that viewer may
	67	// see: everything published, plus their own pending ones. viewer 0 is an
	68	// anonymous reader, who sees only what is published.
	69	func (s *Store) ListDiffComments(mrID, viewer int64) ([]DiffComment, error) {
66	70	rows, err := s.DB.Query(`
67	71	SELECT c.id, u.username, c.head_sha, c.path, c.side, c.line, c.body,
68		COALESCE(c.reply_to, 0), COALESCE(r.username, ''), c.created_at
	72	COALESCE(c.reply_to, 0), COALESCE(r.username, ''), c.pending, c.created_at
69	73	FROM mr_diff_comments c
70	74	JOIN users u ON u.id = c.author_id
71	75	LEFT JOIN users r ON r.id = c.resolved_by
72		WHERE c.mr_id = ? ORDER BY c.id`, mrID)
	76	WHERE c.mr_id = ?1 AND (c.pending = 0 OR c.author_id = ?2)
	77	ORDER BY c.id`, mrID, viewer)
73	78	if err != nil {
74	79	return nil, err
75	80	}
		@@ -78,7 +83,7 @@ func (s *Store) ListDiffComments(mrID int64) ([]DiffComment, error) {
78	83	for rows.Next() {
79	84	var c DiffComment
80	85	if err := rows.Scan(&c.ID, &c.Author, &c.HeadSHA, &c.Path, &c.Side, &c.Line, &c.Body,
81		&c.ReplyTo, &c.ResolvedBy, &c.CreatedAt); err != nil {
	86	&c.ReplyTo, &c.ResolvedBy, &c.Pending, &c.CreatedAt); err != nil {
82	87	return nil, err
83	88	}
84	89	out = append(out, c)
		@@ -121,10 +126,51 @@ func (s *Store) DiffCommentAuthor(mrID, id int64) (int64, error) {
121	126	}
122	127
123	128	// UnresolvedThreadCount counts unresolved thread roots on an MR.
	129	//
	130	// Pending roots are excluded: an unsubmitted comment is one reviewer's
	131	// note to themselves, and blocking a merge on it would let anyone stall
	132	// a merge request with a thread nobody else can see or resolve.
124	133	func (s *Store) UnresolvedThreadCount(mrID int64) (int, error) {
125	134	var n int
126		err := s.DB.QueryRow(
127		"SELECT COUNT(*) FROM mr_diff_comments WHERE mr_id = ? AND reply_to IS NULL AND resolved_at IS NULL",
	135	err := s.DB.QueryRow(`
	136	SELECT COUNT(*) FROM mr_diff_comments
	137	WHERE mr_id = ? AND reply_to IS NULL AND resolved_at IS NULL AND pending = 0`,
128	138	mrID).Scan(&n)
129	139	return n, err
130	140	}
	141
	142	// PublishPendingComments makes an author's pending comments on an MR
	143	// visible, and reports how many. This is what `mr review` does with the
	144	// batch the reviewer composed.
	145	func (s *Store) PublishPendingComments(mrID, authorID int64) (int64, error) {
	146	res, err := s.DB.Exec(
	147	"UPDATE mr_diff_comments SET pending = 0 WHERE mr_id = ? AND author_id = ? AND pending = 1",
	148	mrID, authorID)
	149	if err != nil {
	150	return 0, err
	151	}
	152	return res.RowsAffected()
	153	}
	154
	155	// DiscardPendingComments deletes an author's unsubmitted comments. Only
	156	// pending rows: a published comment is part of the conversation and is
	157	// not something its author can quietly take back.
	158	func (s *Store) DiscardPendingComments(mrID, authorID int64) (int64, error) {
	159	res, err := s.DB.Exec(
	160	"DELETE FROM mr_diff_comments WHERE mr_id = ? AND author_id = ? AND pending = 1",
	161	mrID, authorID)
	162	if err != nil {
	163	return 0, err
	164	}
	165	return res.RowsAffected()
	166	}
	167
	168	// CountPendingComments is how many unsubmitted comments an author holds
	169	// on an MR, for the reminder that they have a review in progress.
	170	func (s *Store) CountPendingComments(mrID, authorID int64) int {
	171	var n int
	172	s.DB.QueryRow(
	173	"SELECT COUNT(*) FROM mr_diff_comments WHERE mr_id = ? AND author_id = ? AND pending = 1",
	174	mrID, authorID).Scan(&n)
	175	return n
	176	}

internal/store/migrations/0038_pending_review.down.sql added +2

		@@ -0,0 +1,2 @@
	1	DROP INDEX mr_diff_comments_pending;
	2	ALTER TABLE mr_diff_comments DROP COLUMN pending;

internal/store/migrations/0038_pending_review.up.sql added +13

		@@ -0,0 +1,13 @@
	1	-- A review composed as a unit and submitted at once. A diff comment used
	2	-- to post the moment it was written, so a reviewer reading a change had
	3	-- to either publish half-formed thoughts one at a time or keep them
	4	-- somewhere else until they were done (#111).
	5	--
	6	-- pending marks a comment belonging to a review its author has not
	7	-- submitted yet: visible to them alone, and published by `mr review`
	8	-- along with the verdict. Default 0, so every comment that already
	9	-- exists is published, which is what it was.
	10	ALTER TABLE mr_diff_comments ADD COLUMN pending INTEGER NOT NULL DEFAULT 0;
	11	-- The unresolved-thread merge gate and every listing filter on this, per
	12	-- viewer, and one reviewer's pending set is small against the table.
	13	CREATE INDEX mr_diff_comments_pending ON mr_diff_comments(mr_id, author_id, pending);

internal/store/pending_test.go added +120

		@@ -0,0 +1,120 @@
	1	package store
	2
	3	import "testing"
	4
	5	func pendingFixture(t testing.T) (Store, int64, int64, int64) {
	6	t.Helper()
	7	s := open(t)
	8	if err := s.MigrateUp(); err != nil {
	9	t.Fatal(err)
	10	}
	11	author, err := s.CreateUser("cmc", true)
	12	if err != nil {
	13	t.Fatal(err)
	14	}
	15	other, err := s.CreateUser("kim", false)
	16	if err != nil {
	17	t.Fatal(err)
	18	}
	19	repoID, err := s.CreateRepo("user", author, "lib", "public")
	20	if err != nil {
	21	t.Fatal(err)
	22	}
	23	n, err := s.CreateMR(repoID, author, repoID, "feature", "main", "t", "", "abc123", "md", false)
	24	if err != nil {
	25	t.Fatal(err)
	26	}
	27	mr, err := s.MRByNumber(repoID, n)
	28	if err != nil {
	29	t.Fatal(err)
	30	}
	31	return s, mr.ID, author, other
	32	}
	33
	34	// A pending comment belongs to the reviewer composing it and to nobody
	35	// else, until they submit.
	36	func TestPendingCommentsArePrivate(t *testing.T) {
	37	s, mrID, author, other := pendingFixture(t)
	38	if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", 3, "half a thought", 0, true); err != nil {
	39	t.Fatal(err)
	40	}
	41	if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", 9, "said out loud", 0, false); err != nil {
	42	t.Fatal(err)
	43	}
	44
	45	mine, err := s.ListDiffComments(mrID, other)
	46	if err != nil {
	47	t.Fatal(err)
	48	}
	49	if len(mine) != 2 {
	50	t.Fatalf("author of the pending comment sees %d, want both", len(mine))
	51	}
	52	theirs, _ := s.ListDiffComments(mrID, author)
	53	if len(theirs) != 1 \|\| theirs[0].Body != "said out loud" {
	54	t.Fatalf("someone else sees %+v", theirs)
	55	}
	56	anon, _ := s.ListDiffComments(mrID, 0)
	57	if len(anon) != 1 {
	58	t.Fatalf("anonymous reader sees %d, want the published one only", len(anon))
	59	}
	60	if n := s.CountPendingComments(mrID, other); n != 1 {
	61	t.Fatalf("pending count = %d", n)
	62	}
	63	}
	64
	65	// An unsubmitted thread must not gate a merge: nobody else can see it,
	66	// so nobody else could resolve it.
	67	func TestPendingThreadsDoNotBlockMerges(t *testing.T) {
	68	s, mrID, _, other := pendingFixture(t)
	69	if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", 3, "pending", 0, true); err != nil {
	70	t.Fatal(err)
	71	}
	72	n, err := s.UnresolvedThreadCount(mrID)
	73	if err != nil {
	74	t.Fatal(err)
	75	}
	76	if n != 0 {
	77	t.Fatalf("pending thread counted against the merge gate (%d)", n)
	78	}
	79
	80	if _, err := s.PublishPendingComments(mrID, other); err != nil {
	81	t.Fatal(err)
	82	}
	83	if n, _ := s.UnresolvedThreadCount(mrID); n != 1 {
	84	t.Fatalf("published thread does not gate (%d)", n)
	85	}
	86	}
	87
	88	func TestPublishAndDiscardPending(t *testing.T) {
	89	s, mrID, author, other := pendingFixture(t)
	90	for i := 0; i < 3; i++ {
	91	if _, err := s.AddDiffComment(mrID, other, "abc123", "a.go", "new", int64(i+1), "note", 0, true); err != nil {
	92	t.Fatal(err)
	93	}
	94	}
	95	// Another reviewer's batch is untouched by either operation.
	96	if _, err := s.AddDiffComment(mrID, author, "abc123", "b.go", "new", 1, "mine", 0, true); err != nil {
	97	t.Fatal(err)
	98	}
	99
	100	n, err := s.PublishPendingComments(mrID, other)
	101	if err != nil \|\| n != 3 {
	102	t.Fatalf("published %d (%v), want 3", n, err)
	103	}
	104	if got := s.CountPendingComments(mrID, author); got != 1 {
	105	t.Fatalf("the other reviewer's batch was published too (%d left)", got)
	106	}
	107	// Publishing again is a no-op, not a double publish.
	108	if n, _ := s.PublishPendingComments(mrID, other); n != 0 {
	109	t.Fatalf("second publish moved %d rows", n)
	110	}
	111
	112	// Discard removes only what is still pending.
	113	if n, _ := s.DiscardPendingComments(mrID, author); n != 1 {
	114	t.Fatalf("discarded %d, want 1", n)
	115	}
	116	all, _ := s.ListDiffComments(mrID, other)
	117	if len(all) != 3 {
	118	t.Fatalf("discard took published comments with it: %d remain", len(all))
	119	}
	120	}

internal/web/static/style.css +2

		@@ -1194,6 +1194,8 @@ details.editbox input[type="text"] { width: 100%; }
1194	1194	margin: var(--sp-2) 0 var(--sp-2) var(--sp-6);
1195	1195	}
1196	1196	.thread.resolved { border-left-color: var(--ok); opacity: 0.75; }
	1197	/* an unsubmitted comment is the viewer's own note, not the conversation */
	1198	.thread.pending { border-left-color: var(--neutral); border-left-style: dashed; }
1197	1199	.thread.stale { border-left-color: var(--warn); }
1198	1200	.thread p.commenthead { font-size: var(--fs-2); margin: var(--sp-2) 0 0; }
1199	1201	.thread .when { color: var(--muted); }

internal/web/templates/layout.html +5 −2

		@@ -143,7 +143,9 @@
143	143	<input type="hidden" name="line" value="{{if eq .Class "del"}}{{.OldLine}}{{else}}{{.NewLine}}{{end}}">
144	144	<input type="hidden" name="side" value="{{if eq .Class "del"}}old{{else}}new{{end}}">
145	145	<p><textarea name="body" aria-label="Comment on {{.Path}}" rows="3" placeholder="Comment on this line" autofocus></textarea></p>
146		<p><button type="submit">Comment</button> <a href="{{$base}}?view=diff">Cancel</a></p>
	146	<p><button type="submit">Comment</button>
	147	<button type="submit" name="pending" value="on">Add to review</button>
	148	<a href="{{$base}}?view=diff">Cancel</a></p>
147	149	</form></td></tr>
148	150	{{end}}{{end}}{{range .Threads}}<tr class="threadrow"><td colspan="3">{{template "thread" dict "T" . "Base" $base "Viewer" $viewer "Class" ""}}</td></tr>
149	151	{{end}}{{end}}
		@@ -157,7 +159,8 @@
157	159
158	160	{{/* thread renders one review thread with its reply and resolve controls.
159	161	Class carries "stale" for threads whose anchor is gone. */}}
160		{{define "thread"}}{{$t := .T}}<div class="thread{{if $t.Resolved}} resolved{{end}}{{if .Class}} {{.Class}}{{end}}" id="thread-{{$t.ID}}">
	162	{{define "thread"}}{{$t := .T}}<div class="thread{{if $t.Resolved}} resolved{{end}}{{if $t.Pending}} pending{{end}}{{if .Class}} {{.Class}}{{end}}" id="thread-{{$t.ID}}">
	163	{{if $t.Pending}}<p class="threadstate">pending — only you can see this until you submit your review</p>{{end}}
161	164	{{if or $t.Resolved (and .Class $t.Stale)}}<p class="threadstate">{{if and .Class $t.Stale}}stale{{end}}{{if $t.Resolved}}{{if and .Class $t.Stale}} · {{end}}resolved by {{$t.Resolved}}{{end}}</p>{{end}}
162	165	{{range $t.Comments}}<p class="commenthead"><strong>{{.Author}}</strong> <span class="when">{{when .CreatedAt}}</span></p><div class="rendered">{{.BodyHTML}}</div>{{end}}
163	166	{{if .Viewer}}<details class="threadreply"><summary>Reply</summary>

Commit 46a896187a

Commit `46a896187a`