Sign in

Search issue titles and bodies, change requests and comments. Private repositories you cannot read are not included.
vole
Clone

Read only, no credentials:

https://rodent.software/vole.git

Write over SSH with a registered key:

git clone ssh://git@rodent.software:2222/vole.git

A free and open-source forge that lives in your Git repo.

Build out change review and threaded discussions

f6f4b1db24292d1df877680714952a9c568e2b78 · Keenan Nemetz · 15d ago · parent 564acd4b 

Build out change review and threaded discussions

- Add Conversation, Files and Runs tabs to change requests. Files uses the
  code-browser shape with a changed-file tree and one selected-file diff;
  Runs lists the head commit's workflow results.
- Show anchored review comments beneath the line, with a collapsed excerpt
  read from the exact blob that was reviewed, and return posted anchors to
  their line note.
- Add comment threads: any comment can be replied to, the UI flattens the
  descendants under one root, reply context links back to the parent, and
  reply targets feed a single composer at the foot of the discussion.
- Collapse reply groups while keeping the root visible, and scroll to the
  composer when Reply is selected.
- Add comment context menus with Reply, Copy text, Copy link, Edit and
  Delete. Add web edit/delete routes with author or admin checks; deletion
  leaves a tombstone so replies keep their thread.
- Support --reply-to in the issue and change CLI commands, and treat a
  missing parent as a tombstone rather than an invalid tree.
- Fix the diff comment gutter, action button colours, reply labels, and
  keep the Files tree and diff pane aligned.

Update metadata validation, web templates, CSS, JavaScript, tests, and the
CLI/change-request/issue docs.

server/web/requests.go +293−59

@@ -8,6 +8,7 @@ import (
8 8 "strconv"
9 9 "strings"
10 10
11 "github.com/rodent-software/vole/core/git"
11 12 "github.com/rodent-software/vole/forge/access"
12 13 "github.com/rodent-software/vole/forge/repo"
13 14 "github.com/rodent-software/vole/metadata"
@@ -207,6 +208,23 @@ type requestCI struct {
207 208 CanAllow bool
208 209 }
209 210
211 type commentSnippetLine struct {
212 No int
213 Text string
214 Anchor bool
215 }
216
217 // commentSnippet is a few lines around an anchored review comment. It comes
218 // from the blob the comment names, not the current file, so a stale comment
219 // still shows the code the reviewer actually saw.
220 type commentSnippet struct {
221 Path string
222 Line int
223 Side string
224 Lines []commentSnippetLine
225 FilesURL string
226 }
227
210 228 type requestCommentView struct {
211 229 metadata.Comment
212 230 Body template.HTML
@@ -214,6 +232,19 @@ type requestCommentView struct {
214 232 // written about. A stale comment keeps its place in the thread and loses its
215 233 // marker on the diff.
216 234 Current bool
235 Snippet *commentSnippet
236 Replies []requestCommentView
237 // ReplyToName names the account this comment answers. ReplyToDeleted marks a
238 // parent that is a tombstone, so the UI can say so instead of showing no one.
239 ReplyToName string
240 ReplyToDeleted bool
241 CanReply bool
242 CanEdit bool
243 CanDelete bool
244 Source string
245 CSRF string
246 EditURL string
247 DeleteURL string
217 248 }
218 249
219 250 // requestSidebarView is the data the change sidebar fragments need. One type for
@@ -240,15 +271,25 @@ type commentTarget struct {
240 271 Side string
241 272 }
242 273
274 // requestInlineCommentView is the row-shaped composer inserted under a diff
275 // line. It carries the anchor in hidden fields so the posted comment lands on
276 // the same line the reviewer clicked.
277 type requestInlineCommentView struct {
278 Page
279 Name string
280 Request *metadata.Request
281 Target *commentTarget
282 }
283
243 284 type requestPageView struct {
244 285 requestSidebarView
286 Tab string
245 287 Description template.HTML
246 288 CommentTarget *commentTarget
247 289 LineCommentURL string
248 Files []diffFile
290 FileCount int
249 291 Added int
250 292 Removed int
251 Truncated bool
252 293 AlreadyMerged bool
253 294 Comments []requestCommentView
254 295 Conflicts []string
@@ -378,15 +419,132 @@ func (w *Web) requestComments(m *repo.Meta, req *metadata.Request) []requestComm
378 419 blobs[key] = blob
379 420 return blob
380 421 }
422 snippetBlobs := map[string][]string{}
381 423 comments := make([]requestCommentView, 0, len(req.Comments))
382 424 for _, cm := range req.Comments {
383 rc := requestCommentView{Comment: cm, Body: w.renderMarkdown(m.Name, cm.Body)}
384 if cm.Anchor != nil {
425 rc := requestCommentView{Comment: cm, Body: w.renderMarkdown(m.Name, cm.Body), Source: cm.Body}
426 if cm.Anchor != nil && !cm.Deleted {
385 427 rc.Current = currentBlob(cm.Anchor.Side, cm.Anchor.Path) == cm.Anchor.Blob
428 rc.Snippet = buildCommentSnippet(m.Git(), cm.Anchor, snippetBlobs)
429 if rc.Snippet != nil {
430 rc.Snippet.FilesURL = "/r/" + m.Name + "/requests/" + req.ID +
431 "/files/" + pathURL(rc.Snippet.Path) + "#inline-c-" + cm.ID
432 }
386 433 }
387 434 comments = append(comments, rc)
388 435 }
389 return comments
436 return threadRequestComments(comments)
437 }
438
439 // threadRequestComments puts every descendant under its top-level root. Replies
440 // may answer any comment, but the page shows one level; a missing parent becomes
441 // a tombstone so replies to a deleted comment stay in their thread.
442 func threadRequestComments(comments []requestCommentView) []requestCommentView {
443 nodes := make(map[string]requestCommentView, len(comments))
444 for _, c := range comments {
445 nodes[c.ID] = c
446 }
447 for _, c := range comments {
448 if c.ReplyTo != "" {
449 if _, ok := nodes[c.ReplyTo]; !ok {
450 nodes[c.ReplyTo] = requestCommentView{Comment: metadata.Comment{ID: c.ReplyTo, Deleted: true}}
451 }
452 }
453 }
454 ids := make([]string, 0, len(nodes))
455 for id := range nodes {
456 ids = append(ids, id)
457 }
458 sort.Strings(ids)
459 children := make(map[string][]string, len(nodes))
460 var roots []string
461 for _, id := range ids {
462 c := nodes[id]
463 if c.ReplyTo == "" || c.ReplyTo == id {
464 roots = append(roots, id)
465 continue
466 }
467 if _, ok := nodes[c.ReplyTo]; ok {
468 children[c.ReplyTo] = append(children[c.ReplyTo], id)
469 } else {
470 roots = append(roots, id)
471 }
472 }
473 out := make([]requestCommentView, 0, len(roots))
474 for _, id := range roots {
475 root := nodes[id]
476 seen := map[string]bool{id: true}
477 var replies []string
478 var walk func(string)
479 walk = func(parent string) {
480 for _, child := range children[parent] {
481 if seen[child] {
482 continue
483 }
484 seen[child] = true
485 replies = append(replies, child)
486 walk(child)
487 }
488 }
489 walk(id)
490 sort.Strings(replies)
491 for _, rid := range replies {
492 reply := nodes[rid]
493 if parent, ok := nodes[reply.ReplyTo]; ok {
494 reply.ReplyToName = parent.Author
495 reply.ReplyToDeleted = parent.Deleted
496 }
497 root.Replies = append(root.Replies, reply)
498 }
499 out = append(out, root)
500 }
501 return out
502 }
503
504 // commentSnippet reads the anchored blob and returns the lines around the
505 // comment, with the anchored line marked. The blob cache is per page, so many
506 // comments on one file cost one cat-file.
507 func buildCommentSnippet(g *git.Repo, a *metadata.Anchor, cache map[string][]string) *commentSnippet {
508 const contextLines = 3
509 if a == nil || a.Blob == "" || a.Line <= 0 {
510 return nil
511 }
512 lines, ok := cache[a.Blob]
513 if !ok {
514 data, err := g.BlobBySHA(a.Blob)
515 if err != nil || len(data) > 1<<20 {
516 cache[a.Blob] = nil
517 return nil
518 }
519 text := string(data)
520 if strings.IndexByte(text, 0) >= 0 {
521 cache[a.Blob] = nil
522 return nil
523 }
524 lines = strings.Split(text, "\n")
525 if len(lines) > 0 && lines[len(lines)-1] == "" {
526 lines = lines[:len(lines)-1]
527 }
528 cache[a.Blob] = lines
529 }
530 if a.Line > len(lines) {
531 return nil
532 }
533 start := a.Line - contextLines
534 if start < 1 {
535 start = 1
536 }
537 end := a.Line + contextLines
538 if end > len(lines) {
539 end = len(lines)
540 }
541 out := &commentSnippet{Path: a.Path, Line: a.Line, Side: a.Side}
542 for no := start; no <= end; no++ {
543 out.Lines = append(out.Lines, commentSnippetLine{
544 No: no, Text: lines[no-1], Anchor: no == a.Line,
545 })
546 }
547 return out
390 548 }
391 549
392 550 // requestDiscussionView is the comment thread fragment, separate from the
@@ -403,10 +561,36 @@ type requestDiscussionView struct {
403 561
404 562 func (w *Web) requestDiscussion(r *http.Request, m *repo.Meta, req *metadata.Request, target *commentTarget, errMsg string) requestDiscussionView {
405 563 acl, _ := m.ACL()
564 canDiscuss := acl != nil && w.canDiscuss(r, acl)
565 comments := w.requestComments(m, req)
566 w.markRequestCommentActions(r, m, req.ID, comments, canDiscuss)
406 567 return requestDiscussionView{
407 568 Page: w.page(r, ""), Name: m.Name, Request: req,
408 Comments: w.requestComments(m, req), CommentTarget: target,
409 CanDiscuss: acl != nil && w.canDiscuss(r, acl), ThreadError: errMsg,
569 Comments: comments, CommentTarget: target,
570 CanDiscuss: canDiscuss, ThreadError: errMsg,
571 }
572 }
573
574 func (w *Web) markRequestCommentActions(r *http.Request, m *repo.Meta, requestID string, comments []requestCommentView, canDiscuss bool) {
575 pr := w.principal(r)
576 acl, _ := m.ACL()
577 admin := pr != nil && acl != nil && acl.RoleFor(pr.account.ID).AtLeast(access.Admin)
578 csrf := ""
579 if pr != nil {
580 csrf = pr.session.CSRF
581 }
582 for i := range comments {
583 c := &comments[i]
584 c.CSRF = csrf
585 c.CanReply = canDiscuss && !c.Deleted
586 c.CanEdit = !c.Deleted && pr != nil && (c.Author == pr.account.Name || admin)
587 c.CanDelete = c.CanEdit
588 if c.CanEdit {
589 base := "/r/" + m.Name + "/requests/" + requestID + "/comments/" + c.ID
590 c.EditURL = base + "/edit"
591 c.DeleteURL = base + "/delete"
592 }
593 w.markRequestCommentActions(r, m, requestID, c.Replies, canDiscuss)
410 594 }
411 595 }
412 596
@@ -424,6 +608,42 @@ func commentTargetFromQuery(r *http.Request) *commentTarget {
424 608 return &commentTarget{Path: path, Line: line, Side: side}
425 609 }
426 610
611 // RequestCommentForm returns the inline composer for one diff line. The link in
612 // the diff is a normal request fallback; with HTMX the row is inserted directly
613 // beneath the line instead of sending the reviewer to the bottom of the page.
614 func (w *Web) RequestCommentForm(rw http.ResponseWriter, r *http.Request) {
615 pr, ok := w.requireHTMLUser(rw, r)
616 if !ok {
617 return
618 }
619 m, ok := w.open(rw, r)
620 if !ok {
621 return
622 }
623 acl, err := m.ACL()
624 if err != nil || !acl.CanDiscuss(pr.account.ID) {
625 http.Error(rw, "you do not have permission to comment here", http.StatusForbidden)
626 return
627 }
628 req, err := metadata.GetRequest(m.Git(), r.PathValue("id"))
629 if err != nil {
630 http.NotFound(rw, r)
631 return
632 }
633 target := commentTargetFromQuery(r)
634 if target == nil {
635 http.Error(rw, "choose a line to comment on", http.StatusBadRequest)
636 return
637 }
638 if _, err := w.commentAnchor(m, req, target.Path, target.Line, target.Side); err != nil {
639 http.Error(rw, err.Error(), http.StatusBadRequest)
640 return
641 }
642 w.render(rw, "request_inline_comment", requestInlineCommentView{
643 Page: w.page(r, ""), Name: m.Name, Request: req, Target: target,
644 }, 0)
645 }
646
427 647 // commentAnchor resolves a line anchor against the content the reviewer saw.
428 648 // The blob is what later answers "has this comment gone stale?"; storing only a
429 649 // path and line would make the comment slide to whatever now occupies that line.
@@ -469,56 +689,9 @@ func (w *Web) RequestView(rw http.ResponseWriter, r *http.Request) {
469 689 return
470 690 }
471 691
472 // The diff is computed from git on every view: it is cheap for one branch and
473 // always matches what the approver is being asked to approve. Three-dot range:
474 // a review is about what the branch adds since it diverged, not about every
475 // commit main gained meanwhile.
476 var files []diffFile
477 truncated := false
478 alreadyMerged := false
479 if req.Head != "" {
480 if patch, cut, err := m.Git().Diff(req.Base+"..."+req.Head, "", diffCap); err == nil {
481 files = parseDiff(patch)
482 truncated = cut
483 }
484 // An empty diff means one of two very different things, so say which.
485 if in, err := m.Git().IsAncestor(req.Head, req.Base); err == nil && in {
486 alreadyMerged = true
487 }
488 }
489
490 // The review thread. Comments arrive by push, so they cross the same markdown
491 // boundary every other pushed body does — sanitised, never rendered raw.
492 //
493 // An anchored comment names a line and the blob it saw. Whether it is still
494 // about that content is answered here rather than trusted from the file: one
495 // rev-parse per distinct (side, path) is a handful for a review thread, and it
496 // is exact where the diff's abbreviated ids would need prefix matching.
692 files, _, alreadyMerged := w.requestDiff(m, req)
497 693 comments := w.requestComments(m, req)
498
499 // A marker on the line each current anchored comment is about. The side decides
500 // which path and which line number: a comment on a deleted line belongs to the
501 // file as it stood at the merge base.
502 for i := range files {
503 f := &files[i]
504 for h := range f.Hunks {
505 lines := f.Hunks[h].Lines
506 for l := range lines {
507 for _, rc := range comments {
508 if rc.Anchor == nil || !rc.Current {
509 continue
510 }
511 path, no := f.NewPath, lines[l].NewNo
512 if rc.Anchor.Side == "old" {
513 path, no = f.OldPath, lines[l].OldNo
514 }
515 if path == rc.Anchor.Path && no == rc.Anchor.Line {
516 lines[l].Comments = append(lines[l].Comments, diffComment{ID: rc.ID, Author: rc.Author})
517 }
518 }
519 }
520 }
521 }
694 attachRequestComments(files, comments)
522 695 added, removed := diffSummary(files)
523 696
524 697 // Would it land? A diff says what changed; it cannot say whether the branch
@@ -537,18 +710,79 @@ func (w *Web) RequestView(rw http.ResponseWriter, r *http.Request) {
537 710 }
538 711 }
539 712
713 sidebar := w.requestSidebar(r, m, req, m.Name+" · "+req.Title, "")
714 w.markRequestCommentActions(r, m, req.ID, comments, sidebar.CanDiscuss)
540 715 w.render(rw, "request", requestPageView{
541 requestSidebarView: w.requestSidebar(r, m, req, m.Name+" · "+req.Title, ""),
716 requestSidebarView: sidebar,
717 Tab: "conversation",
542 718 Description: w.renderMarkdown(m.Name, req.Description),
543 719 CommentTarget: commentTargetFromQuery(r),
544 720 LineCommentURL: "/r/" + m.Name + "/requests/" + req.ID,
545 Files: files,
721 FileCount: len(files),
546 722 Added: added,
547 723 Removed: removed,
548 Truncated: truncated,
549 724 AlreadyMerged: alreadyMerged,
550 725 Comments: comments,
551 726 Conflicts: conflicts,
552 727 MergeChecked: mergeChecked,
553 728 }, 0)
554 729 }
730
731 // requestDiff computes the three-dot patch for a change and answers whether it
732 // is already merged. The diff is cheap for one branch and always matches what
733 // the approver is being asked to approve.
734 func (w *Web) requestDiff(m *repo.Meta, req *metadata.Request) (files []diffFile, truncated, alreadyMerged bool) {
735 if req.Head == "" {
736 return nil, false, false
737 }
738 if patch, cut, err := m.Git().Diff(req.Base+"..."+req.Head, "", diffCap); err == nil {
739 files = parseDiff(patch)
740 truncated = cut
741 }
742 // An empty diff means one of two very different things, so say which.
743 if in, err := m.Git().IsAncestor(req.Head, req.Base); err == nil && in {
744 alreadyMerged = true
745 }
746 return files, truncated, alreadyMerged
747 }
748
749 // reviewPathFor maps an anchor's path to the file row a review URL should open.
750 // A comment on the old side of a rename names the old path, while the Files tree
751 // keys the change by its new path.
752 func (w *Web) reviewPathFor(m *repo.Meta, req *metadata.Request, path string) string {
753 files, _, _ := w.requestDiff(m, req)
754 for _, f := range files {
755 if f.NewPath == path || f.OldPath == path {
756 return reviewFilePath(f)
757 }
758 }
759 return path
760 }
761
762 // attachRequestComments puts each current anchored comment beneath the line it
763 // names. A comment on a deleted line belongs to the file as it stood at the
764 // merge base; stale comments stay in the discussion and lose their line note.
765 func attachRequestComments(files []diffFile, comments []requestCommentView) {
766 for i := range files {
767 f := &files[i]
768 for h := range f.Hunks {
769 lines := f.Hunks[h].Lines
770 for l := range lines {
771 for _, rc := range comments {
772 if rc.Anchor == nil || rc.Deleted || !rc.Current {
773 continue
774 }
775 path, no := f.NewPath, lines[l].NewNo
776 if rc.Anchor.Side == "old" {
777 path, no = f.OldPath, lines[l].OldNo
778 }
779 if path == rc.Anchor.Path && no == rc.Anchor.Line {
780 lines[l].Comments = append(lines[l].Comments, diffComment{
781 ID: rc.ID, Author: rc.Author, Created: rc.Created, Body: rc.Body,
782 })
783 }
784 }
785 }
786 }
787 }
788 }