tenseleyflow/shithub / 736c0b3

Browse files

S23: review web handlers + Conversation/Files tab review surfaces

Authored by mfwolffe <wolffemf@dukes.jmu.edu>
SHA
736c0b33d7d5d56740574b7d689cb6fc70e1c2b2
Parents
a3cf168
Tree
858dd3a

2 changed files

StatusFile+-
A internal/web/handlers/repo/pull_review.go 322 0
M internal/web/handlers/repo/pulls.go 67 4
internal/web/handlers/repo/pull_review.goadded
@@ -0,0 +1,322 @@
1
+// SPDX-License-Identifier: AGPL-3.0-or-later
2
+
3
+package repo
4
+
5
+import (
6
+	"errors"
7
+	"net/http"
8
+	"strconv"
9
+	"strings"
10
+
11
+	"github.com/go-chi/chi/v5"
12
+
13
+	"github.com/tenseleyFlow/shithub/internal/auth/policy"
14
+	"github.com/tenseleyFlow/shithub/internal/pulls"
15
+	"github.com/tenseleyFlow/shithub/internal/pulls/review"
16
+	pullsdb "github.com/tenseleyFlow/shithub/internal/pulls/sqlc"
17
+	"github.com/tenseleyFlow/shithub/internal/web/middleware"
18
+	"github.com/tenseleyFlow/shithub/internal/worker"
19
+)
20
+
21
+// MountPullReview registers the S23 review-related routes under the
22
+// existing pulls router. All routes require auth (writes only).
23
+func (h *Handlers) MountPullReview(r chi.Router) {
24
+	r.Group(func(r chi.Router) {
25
+		r.Use(middleware.RequireUser)
26
+		// Inline review comments + threads.
27
+		r.Post("/{owner}/{repo}/pulls/{number}/review-comments", h.prCommentCreate)
28
+		r.Post("/{owner}/{repo}/pulls/{number}/review-comments/{cid}/reply", h.prCommentReply)
29
+		r.Post("/{owner}/{repo}/pulls/{number}/review-comments/{cid}/edit", h.prCommentEdit)
30
+		r.Post("/{owner}/{repo}/pulls/{number}/review-comments/{cid}/resolve", h.prCommentResolve)
31
+		r.Post("/{owner}/{repo}/pulls/{number}/review-comments/{cid}/reopen", h.prCommentReopen)
32
+
33
+		// Reviews.
34
+		r.Post("/{owner}/{repo}/pulls/{number}/reviews", h.prReviewSubmit)
35
+		r.Post("/{owner}/{repo}/pulls/{number}/reviews/{rid}/dismiss", h.prReviewDismiss)
36
+
37
+		// Reviewer requests.
38
+		r.Post("/{owner}/{repo}/pulls/{number}/reviewers", h.prReviewerRequest)
39
+	})
40
+}
41
+
42
+// reviewDeps materializes the review.Deps from the handler set.
43
+func (h *Handlers) reviewDeps() review.Deps {
44
+	return review.Deps{Pool: h.d.Pool, Logger: h.d.Logger}
45
+}
46
+
47
+// kickMergeability enqueues a pr:mergeability job. Called after every
48
+// review-side write so the merge gate state reflects truth quickly.
49
+func (h *Handlers) kickMergeability(r *http.Request, prID int64) {
50
+	if _, err := worker.Enqueue(r.Context(), h.d.Pool, worker.KindPRMergeability,
51
+		map[string]any{"pr_id": prID}, worker.EnqueueOptions{}); err != nil {
52
+		h.d.Logger.WarnContext(r.Context(), "pulls: enqueue mergeability", "error", err)
53
+	}
54
+	_ = worker.Notify(r.Context(), h.d.Pool)
55
+}
56
+
57
+func (h *Handlers) prCommentCreate(w http.ResponseWriter, r *http.Request) {
58
+	row, owner, ok := h.loadRepoAndAuthorize(w, r, policy.ActionPullReview)
59
+	if !ok {
60
+		return
61
+	}
62
+	pr, ok := h.loadPullByNumber(w, r, row.ID)
63
+	if !ok {
64
+		return
65
+	}
66
+	if err := r.ParseForm(); err != nil {
67
+		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "form parse")
68
+		return
69
+	}
70
+	viewer := middleware.CurrentUserFromContext(r.Context())
71
+	line, _ := strconv.Atoi(r.PostFormValue("line"))
72
+	pos, _ := strconv.Atoi(r.PostFormValue("position"))
73
+	cur, _ := strconv.Atoi(r.PostFormValue("current_position"))
74
+	if cur == 0 {
75
+		cur = pos
76
+	}
77
+	pending := r.PostFormValue("pending") == "on"
78
+	if _, err := review.AddComment(r.Context(), h.reviewDeps(), review.CommentParams{
79
+		PRIssueID:         pr.IID,
80
+		AuthorUserID:      viewer.ID,
81
+		FilePath:          r.PostFormValue("file_path"),
82
+		Side:              r.PostFormValue("side"),
83
+		OriginalCommitSHA: r.PostFormValue("commit_sha"),
84
+		OriginalLine:      int32(line),
85
+		OriginalPosition:  int32(pos),
86
+		CurrentPosition:   int32(cur),
87
+		Body:              r.PostFormValue("body"),
88
+		Pending:           pending,
89
+	}); err != nil {
90
+		h.handleReviewWriteError(w, r, err)
91
+		return
92
+	}
93
+	h.redirectPullTab(w, r, owner.Username, row.Name, pr.INumber, "files")
94
+}
95
+
96
+func (h *Handlers) prCommentReply(w http.ResponseWriter, r *http.Request) {
97
+	row, owner, ok := h.loadRepoAndAuthorize(w, r, policy.ActionPullReview)
98
+	if !ok {
99
+		return
100
+	}
101
+	pr, ok := h.loadPullByNumber(w, r, row.ID)
102
+	if !ok {
103
+		return
104
+	}
105
+	cid, err := strconv.ParseInt(chi.URLParam(r, "cid"), 10, 64)
106
+	if err != nil {
107
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
108
+		return
109
+	}
110
+	if err := r.ParseForm(); err != nil {
111
+		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "form parse")
112
+		return
113
+	}
114
+	viewer := middleware.CurrentUserFromContext(r.Context())
115
+	if _, err := review.AddComment(r.Context(), h.reviewDeps(), review.CommentParams{
116
+		PRIssueID:    pr.IID,
117
+		AuthorUserID: viewer.ID,
118
+		Body:         r.PostFormValue("body"),
119
+		InReplyToID:  cid,
120
+	}); err != nil {
121
+		h.handleReviewWriteError(w, r, err)
122
+		return
123
+	}
124
+	h.redirectPullTab(w, r, owner.Username, row.Name, pr.INumber, "files")
125
+}
126
+
127
+func (h *Handlers) prCommentEdit(w http.ResponseWriter, r *http.Request) {
128
+	row, owner, ok := h.loadRepoAndAuthorize(w, r, policy.ActionPullReview)
129
+	if !ok {
130
+		return
131
+	}
132
+	pr, ok := h.loadPullByNumber(w, r, row.ID)
133
+	if !ok {
134
+		return
135
+	}
136
+	cid, err := strconv.ParseInt(chi.URLParam(r, "cid"), 10, 64)
137
+	if err != nil {
138
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
139
+		return
140
+	}
141
+	c, err := pullsdb.New().GetPRReviewComment(r.Context(), h.d.Pool, cid)
142
+	if err != nil || c.PrIssueID != pr.IID {
143
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
144
+		return
145
+	}
146
+	viewer := middleware.CurrentUserFromContext(r.Context())
147
+	if !c.AuthorUserID.Valid || c.AuthorUserID.Int64 != viewer.ID {
148
+		h.d.Render.HTTPError(w, r, http.StatusForbidden, "only the author can edit")
149
+		return
150
+	}
151
+	if err := r.ParseForm(); err != nil {
152
+		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "form parse")
153
+		return
154
+	}
155
+	if err := review.EditComment(r.Context(), h.reviewDeps(), cid, r.PostFormValue("body")); err != nil {
156
+		h.handleReviewWriteError(w, r, err)
157
+		return
158
+	}
159
+	h.redirectPullTab(w, r, owner.Username, row.Name, pr.INumber, "files")
160
+}
161
+
162
+func (h *Handlers) prCommentResolve(w http.ResponseWriter, r *http.Request) {
163
+	h.commentResolveOrReopen(w, r, true)
164
+}
165
+
166
+func (h *Handlers) prCommentReopen(w http.ResponseWriter, r *http.Request) {
167
+	h.commentResolveOrReopen(w, r, false)
168
+}
169
+
170
+func (h *Handlers) commentResolveOrReopen(w http.ResponseWriter, r *http.Request, resolve bool) {
171
+	row, owner, ok := h.loadRepoAndAuthorize(w, r, policy.ActionPullReview)
172
+	if !ok {
173
+		return
174
+	}
175
+	pr, ok := h.loadPullByNumber(w, r, row.ID)
176
+	if !ok {
177
+		return
178
+	}
179
+	cid, err := strconv.ParseInt(chi.URLParam(r, "cid"), 10, 64)
180
+	if err != nil {
181
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
182
+		return
183
+	}
184
+	viewer := middleware.CurrentUserFromContext(r.Context())
185
+	if resolve {
186
+		err = review.Resolve(r.Context(), h.reviewDeps(), viewer.ID, cid)
187
+	} else {
188
+		err = review.Reopen(r.Context(), h.reviewDeps(), cid)
189
+	}
190
+	if err != nil {
191
+		h.handleReviewWriteError(w, r, err)
192
+		return
193
+	}
194
+	h.redirectPullTab(w, r, owner.Username, row.Name, pr.INumber, "files")
195
+}
196
+
197
+func (h *Handlers) prReviewSubmit(w http.ResponseWriter, r *http.Request) {
198
+	row, owner, ok := h.loadRepoAndAuthorize(w, r, policy.ActionPullReview)
199
+	if !ok {
200
+		return
201
+	}
202
+	pr, ok := h.loadPullByNumber(w, r, row.ID)
203
+	if !ok {
204
+		return
205
+	}
206
+	if err := r.ParseForm(); err != nil {
207
+		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "form parse")
208
+		return
209
+	}
210
+	viewer := middleware.CurrentUserFromContext(r.Context())
211
+	prAuthor := int64(0)
212
+	if pr.IAuthorUserID.Valid {
213
+		prAuthor = pr.IAuthorUserID.Int64
214
+	}
215
+	if _, err := review.Submit(r.Context(), h.reviewDeps(), review.SubmitParams{
216
+		PRIssueID:      pr.IID,
217
+		AuthorUserID:   viewer.ID,
218
+		State:          strings.TrimSpace(r.PostFormValue("state")),
219
+		Body:           r.PostFormValue("body"),
220
+		PRAuthorUserID: prAuthor,
221
+	}); err != nil {
222
+		h.handleReviewWriteError(w, r, err)
223
+		return
224
+	}
225
+	// Refresh mergeability so the merge button reflects the new state.
226
+	h.kickMergeability(r, pr.IID)
227
+	h.redirectPull(w, r, owner.Username, row.Name, pr.INumber)
228
+}
229
+
230
+func (h *Handlers) prReviewDismiss(w http.ResponseWriter, r *http.Request) {
231
+	row, owner, ok := h.loadRepoAndAuthorize(w, r, policy.ActionPullMerge)
232
+	if !ok {
233
+		return
234
+	}
235
+	pr, ok := h.loadPullByNumber(w, r, row.ID)
236
+	if !ok {
237
+		return
238
+	}
239
+	rid, err := strconv.ParseInt(chi.URLParam(r, "rid"), 10, 64)
240
+	if err != nil {
241
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
242
+		return
243
+	}
244
+	if err := r.ParseForm(); err != nil {
245
+		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "form parse")
246
+		return
247
+	}
248
+	viewer := middleware.CurrentUserFromContext(r.Context())
249
+	if err := review.Dismiss(r.Context(), h.reviewDeps(), viewer.ID, rid, r.PostFormValue("reason")); err != nil {
250
+		h.handleReviewWriteError(w, r, err)
251
+		return
252
+	}
253
+	h.kickMergeability(r, pr.IID)
254
+	h.redirectPull(w, r, owner.Username, row.Name, pr.INumber)
255
+}
256
+
257
+func (h *Handlers) prReviewerRequest(w http.ResponseWriter, r *http.Request) {
258
+	row, owner, ok := h.loadRepoAndAuthorize(w, r, policy.ActionPullReview)
259
+	if !ok {
260
+		return
261
+	}
262
+	pr, ok := h.loadPullByNumber(w, r, row.ID)
263
+	if !ok {
264
+		return
265
+	}
266
+	if err := r.ParseForm(); err != nil {
267
+		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "form parse")
268
+		return
269
+	}
270
+	username := strings.TrimSpace(r.PostFormValue("username"))
271
+	target, err := h.uq.GetUserByUsername(r.Context(), h.d.Pool, username)
272
+	if err != nil {
273
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "user not found")
274
+		return
275
+	}
276
+	viewer := middleware.CurrentUserFromContext(r.Context())
277
+	if _, err := review.Request(r.Context(), h.reviewDeps(), review.RequestParams{
278
+		PRIssueID:         pr.IID,
279
+		RequestedUserID:   target.ID,
280
+		RequestedByUserID: viewer.ID,
281
+	}); err != nil {
282
+		h.handleReviewWriteError(w, r, err)
283
+		return
284
+	}
285
+	h.redirectPull(w, r, owner.Username, row.Name, pr.INumber)
286
+}
287
+
288
+func (h *Handlers) handleReviewWriteError(w http.ResponseWriter, r *http.Request, err error) {
289
+	switch {
290
+	case errors.Is(err, review.ErrEmptyBody):
291
+		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "comment body required")
292
+	case errors.Is(err, review.ErrBodyTooLong):
293
+		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "comment too long")
294
+	case errors.Is(err, review.ErrAuthorCannotApprove):
295
+		h.d.Render.HTTPError(w, r, http.StatusForbidden, "you can't approve your own PR")
296
+	case errors.Is(err, review.ErrInvalidState):
297
+		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "review state must be comment, approve, or request_changes")
298
+	case errors.Is(err, review.ErrCommentNotOnPR):
299
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "comment not found")
300
+	case errors.Is(err, review.ErrAlreadyResolved):
301
+		h.d.Render.HTTPError(w, r, http.StatusConflict, "thread already resolved")
302
+	case errors.Is(err, review.ErrNotResolved):
303
+		h.d.Render.HTTPError(w, r, http.StatusConflict, "thread is not resolved")
304
+	case errors.Is(err, review.ErrReviewerLimitReached):
305
+		h.d.Render.HTTPError(w, r, http.StatusConflict, "reviewer limit (20) reached")
306
+	case errors.Is(err, review.ErrReviewerAlreadyPending):
307
+		h.d.Render.HTTPError(w, r, http.StatusConflict, "reviewer already requested")
308
+	case errors.Is(err, pulls.ErrPRNotFound):
309
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
310
+	default:
311
+		h.d.Logger.WarnContext(r.Context(), "review: write", "error", err)
312
+		h.d.Render.HTTPError(w, r, http.StatusInternalServerError, "")
313
+	}
314
+}
315
+
316
+func (h *Handlers) redirectPullTab(w http.ResponseWriter, r *http.Request, owner, repo string, number int64, tab string) {
317
+	url := "/" + owner + "/" + repo + "/pulls/" + strconv.FormatInt(number, 10)
318
+	if tab != "" && tab != "conversation" {
319
+		url += "/" + tab
320
+	}
321
+	http.Redirect(w, r, url, http.StatusSeeOther)
322
+}
internal/web/handlers/repo/pulls.gomodified
@@ -43,6 +43,9 @@ func (h *Handlers) MountPulls(r chi.Router) {
4343
 		r.Post("/{owner}/{repo}/pulls/{number}/ready", h.pullSetReady)
4444
 		r.Post("/{owner}/{repo}/pulls/{number}/merge", h.pullMerge)
4545
 	})
46
+	// S23 review surface — its own group so the auth-required wrapper
47
+	// is shared cleanly without rewriting this file's existing one.
48
+	h.MountPullReview(r)
4649
 }
4750
4851
 func (h *Handlers) pullsDeps() pulls.Deps {
@@ -296,9 +299,42 @@ func (h *Handlers) pullView(w http.ResponseWriter, r *http.Request) {
296299
 		}
297300
 		cs = append(cs, cr)
298301
 	}
302
+	// Reviews + reviewer requests for the Conversation sidebar.
303
+	reviews, _ := h.pq.ListPRReviews(r.Context(), h.d.Pool, pr.IID)
304
+	type reviewRow struct {
305
+		R          pullsdb.PrReview
306
+		AuthorName string
307
+	}
308
+	rs := make([]reviewRow, 0, len(reviews))
309
+	for _, rv := range reviews {
310
+		rr := reviewRow{R: rv}
311
+		if rv.AuthorUserID.Valid {
312
+			if u, err := h.uq.GetUserByID(r.Context(), h.d.Pool, rv.AuthorUserID.Int64); err == nil {
313
+				rr.AuthorName = u.Username
314
+			}
315
+		}
316
+		rs = append(rs, rr)
317
+	}
318
+	requests, _ := h.pq.ListPRReviewRequests(r.Context(), h.d.Pool, pr.IID)
319
+	type reqRow struct {
320
+		R        pullsdb.PrReviewRequest
321
+		Username string
322
+	}
323
+	reqs := make([]reqRow, 0, len(requests))
324
+	for _, rq := range requests {
325
+		rr := reqRow{R: rq}
326
+		if rq.RequestedUserID.Valid {
327
+			if u, err := h.uq.GetUserByID(r.Context(), h.d.Pool, rq.RequestedUserID.Int64); err == nil {
328
+				rr.Username = u.Username
329
+			}
330
+		}
331
+		reqs = append(reqs, rr)
332
+	}
299333
 	h.renderPullPage(w, r, "conversation", map[string]any{
300
-		"Comments": cs,
301
-		"Events":   events,
334
+		"Comments":       cs,
335
+		"Events":         events,
336
+		"Reviews":        rs,
337
+		"ReviewRequests": reqs,
302338
 	})
303339
 }
304340
@@ -342,9 +378,36 @@ func (h *Handlers) pullFiles(w http.ResponseWriter, r *http.Request) {
342378
 			diffHTML = renderCompareDiff(patch)
343379
 		}
344380
 	}
381
+	// Per-file inline review threads. v1 groups by file_path; the
382
+	// Files tab shows them collapsed under each section. Position-
383
+	// mapped comments display inline; outdated ones are hidden by
384
+	// default behind the "Show outdated" toggle.
385
+	type commentRow struct {
386
+		C          pullsdb.PrReviewComment
387
+		AuthorName string
388
+	}
389
+	threadsByFile := map[string][]commentRow{}
390
+	for _, f := range files {
391
+		rows, _ := h.pq.ListPRReviewCommentsForFile(r.Context(), h.d.Pool, pullsdb.ListPRReviewCommentsForFileParams{
392
+			PrIssueID: pr.IID,
393
+			FilePath:  f.Path,
394
+		})
395
+		out := make([]commentRow, 0, len(rows))
396
+		for _, c := range rows {
397
+			cr := commentRow{C: c}
398
+			if c.AuthorUserID.Valid {
399
+				if u, err := h.uq.GetUserByID(r.Context(), h.d.Pool, c.AuthorUserID.Int64); err == nil {
400
+					cr.AuthorName = u.Username
401
+				}
402
+			}
403
+			out = append(out, cr)
404
+		}
405
+		threadsByFile[f.Path] = out
406
+	}
345407
 	h.renderPullPage(w, r, "files", map[string]any{
346
-		"Files":    files,
347
-		"DiffHTML": diffHTML,
408
+		"Files":          files,
409
+		"DiffHTML":       diffHTML,
410
+		"ThreadsByFile":  threadsByFile,
348411
 	})
349412
 }
350413