fix(cli): preserve comment threads in merge request timelines
- commit
- 74e73243bb40cf92ead12257aef685eefa8f66dd
- author
- brice · · 2026-10-22 18:36 UTC
- committer
- brice · · 2026-10-22 18:41 UTC
- parent
- 35917d0
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +53 -33
M internal/app/cli_mr.go +35 -33
| @@ -769,48 +769,50 @@ func printMRDetail(w io.Writer, d *ermite.GetMergeRequestResponse, withDiff, sin | |||
| 769 | 769 | } | |
| 770 | 770 | _, _ = fmt.Fprintf(w, "\ncomments (%d)\n", len(d.GetComments())) | |
| 771 | 771 | var timeline []issueTimelineRow | |
| 772 | - | var ordered []*ermite.Comment | |
| 773 | 772 | for _, root := range d.GetComments() { | |
| 774 | 773 | if root.GetReplyTo() != "" { | |
| 775 | 774 | continue | |
| 776 | 775 | } | |
| 777 | - | ordered = append(ordered, root) | |
| 776 | + | thread := []*ermite.Comment{root} | |
| 778 | 777 | for _, reply := range d.GetComments() { | |
| 779 | 778 | if reply.GetReplyTo() == root.GetId() { | |
| 780 | - | ordered = append(ordered, reply) | |
| 779 | + | thread = append(thread, reply) | |
| 781 | 780 | } | |
| 782 | 781 | } | |
| 783 | - | } | |
| 784 | - | for _, c := range ordered { | |
| 785 | - | timeline = append(timeline, issueTimelineRow{at: c.GetCreatedAt(), print: func(w io.Writer) error { | |
| 786 | - | where := "general" | |
| 787 | - | if c.GetPath() != "" { | |
| 788 | - | where = fmt.Sprintf("%s:%d (%s)", clean(c.GetPath()), c.GetLine(), sideName(c.GetSide())) | |
| 789 | - | } | |
| 790 | - | if c.GetStartLine() > 0 { | |
| 791 | - | where = fmt.Sprintf("%s:lines %d-%d (%s)", clean(c.GetPath()), c.GetStartLine(), c.GetLine(), sideName(c.GetSide())) | |
| 792 | - | } | |
| 793 | - | mark := "" | |
| 794 | - | if c.GetEditedAt() != nil { | |
| 795 | - | mark += " (edited)" | |
| 796 | - | } | |
| 797 | - | if c.GetDeleted() { | |
| 798 | - | mark += " [deleted]" | |
| 799 | - | } | |
| 800 | - | if c.GetVerdict() != ermite.Verdict_VERDICT_UNSPECIFIED { | |
| 801 | - | mark += " [" + verdictName(c.GetVerdict()) + "]" | |
| 802 | - | } | |
| 803 | - | if c.GetResolved() { | |
| 804 | - | mark += " [resolved]" | |
| 805 | - | } | |
| 806 | - | prefix := " " | |
| 807 | - | body := indentText(c.GetText()) | |
| 808 | - | if c.GetReplyTo() != "" { | |
| 809 | - | prefix += " " | |
| 810 | - | body = " " + strings.ReplaceAll(body, "\n", "\n ") | |
| 782 | + | timeline = append(timeline, issueTimelineRow{at: root.GetCreatedAt(), print: func(w io.Writer) error { | |
| 783 | + | for _, c := range thread { | |
| 784 | + | where := "general" | |
| 785 | + | if c.GetPath() != "" { | |
| 786 | + | where = fmt.Sprintf("%s:%d (%s)", clean(c.GetPath()), c.GetLine(), sideName(c.GetSide())) | |
| 787 | + | } | |
| 788 | + | if c.GetStartLine() > 0 { | |
| 789 | + | where = fmt.Sprintf("%s:lines %d-%d (%s)", clean(c.GetPath()), c.GetStartLine(), c.GetLine(), sideName(c.GetSide())) | |
| 790 | + | } | |
| 791 | + | mark := "" | |
| 792 | + | if c.GetEditedAt() != nil { | |
| 793 | + | mark += " (edited)" | |
| 794 | + | } | |
| 795 | + | if c.GetDeleted() { | |
| 796 | + | mark += " [deleted]" | |
| 797 | + | } | |
| 798 | + | if c.GetVerdict() != ermite.Verdict_VERDICT_UNSPECIFIED { | |
| 799 | + | mark += " [" + verdictName(c.GetVerdict()) + "]" | |
| 800 | + | } | |
| 801 | + | if c.GetResolved() { | |
| 802 | + | mark += " [resolved]" | |
| 803 | + | } | |
| 804 | + | prefix := " " | |
| 805 | + | body := indentText(c.GetText()) | |
| 806 | + | if c.GetReplyTo() != "" { | |
| 807 | + | prefix += " " | |
| 808 | + | body = " " + strings.ReplaceAll(body, "\n", "\n ") | |
| 809 | + | } | |
| 810 | + | _, err := fmt.Fprintf(w, "%s#%s %s, %s, %s%s\n%s\n", prefix, c.GetId(), clean(c.GetAuthor()), shortTime(c.GetCreatedAt(), "-"), where, mark, body) | |
| 811 | + | if err != nil { | |
| 812 | + | return err | |
| 813 | + | } | |
| 811 | 814 | } | |
| 812 | - | _, err := fmt.Fprintf(w, "%s#%s %s, %s, %s%s\n%s\n", prefix, c.GetId(), clean(c.GetAuthor()), shortTime(c.GetCreatedAt(), "-"), where, mark, body) | |
| 813 | - | return err | |
| 815 | + | return nil | |
| 814 | 816 | }}) | |
| 815 | 817 | } | |
| 816 | 818 | timeline = append(timeline, newIssueTimeline(d.GetIssueEvents(), true)...) | |
| @@ -769,48 +769,50 @@ func printMRDetail(w io.Writer, d *ermite.GetMergeRequestResponse, withDiff, sin | |||||
| 769 | } | 769 | } | ||
| 770 | _, _ = fmt.Fprintf(w, "\ncomments (%d)\n", len(d.GetComments())) | 770 | _, _ = fmt.Fprintf(w, "\ncomments (%d)\n", len(d.GetComments())) | ||
| 771 | var timeline []issueTimelineRow | 771 | var timeline []issueTimelineRow | ||
| 772 | - | var ordered []*ermite.Comment | |||
| 773 | for _, root := range d.GetComments() { | 772 | for _, root := range d.GetComments() { | ||
| 774 | if root.GetReplyTo() != "" { | 773 | if root.GetReplyTo() != "" { | ||
| 775 | continue | 774 | continue | ||
| 776 | } | 775 | } | ||
| 777 | - | ordered = append(ordered, root) | 776 | + | thread := []*ermite.Comment{root} |
| 778 | for _, reply := range d.GetComments() { | 777 | for _, reply := range d.GetComments() { | ||
| 779 | if reply.GetReplyTo() == root.GetId() { | 778 | if reply.GetReplyTo() == root.GetId() { | ||
| 780 | - | ordered = append(ordered, reply) | 779 | + | thread = append(thread, reply) |
| 781 | } | 780 | } | ||
| 782 | } | 781 | } | ||
| 783 | - | } | 782 | + | timeline = append(timeline, issueTimelineRow{at: root.GetCreatedAt(), print: func(w io.Writer) error { |
| 784 | - | for _, c := range ordered { | 783 | + | for _, c := range thread { |
| 785 | - | timeline = append(timeline, issueTimelineRow{at: c.GetCreatedAt(), print: func(w io.Writer) error { | 784 | + | where := "general" |
| 786 | - | where := "general" | 785 | + | if c.GetPath() != "" { |
| 787 | - | if c.GetPath() != "" { | 786 | + | where = fmt.Sprintf("%s:%d (%s)", clean(c.GetPath()), c.GetLine(), sideName(c.GetSide())) |
| 788 | - | where = fmt.Sprintf("%s:%d (%s)", clean(c.GetPath()), c.GetLine(), sideName(c.GetSide())) | 787 | + | } |
| 789 | - | } | 788 | + | if c.GetStartLine() > 0 { |
| 790 | - | if c.GetStartLine() > 0 { | 789 | + | where = fmt.Sprintf("%s:lines %d-%d (%s)", clean(c.GetPath()), c.GetStartLine(), c.GetLine(), sideName(c.GetSide())) |
| 791 | - | where = fmt.Sprintf("%s:lines %d-%d (%s)", clean(c.GetPath()), c.GetStartLine(), c.GetLine(), sideName(c.GetSide())) | 790 | + | } |
| 792 | - | } | 791 | + | mark := "" |
| 793 | - | mark := "" | 792 | + | if c.GetEditedAt() != nil { |
| 794 | - | if c.GetEditedAt() != nil { | 793 | + | mark += " (edited)" |
| 795 | - | mark += " (edited)" | 794 | + | } |
| 796 | - | } | 795 | + | if c.GetDeleted() { |
| 797 | - | if c.GetDeleted() { | 796 | + | mark += " [deleted]" |
| 798 | - | mark += " [deleted]" | 797 | + | } |
| 799 | - | } | 798 | + | if c.GetVerdict() != ermite.Verdict_VERDICT_UNSPECIFIED { |
| 800 | - | if c.GetVerdict() != ermite.Verdict_VERDICT_UNSPECIFIED { | 799 | + | mark += " [" + verdictName(c.GetVerdict()) + "]" |
| 801 | - | mark += " [" + verdictName(c.GetVerdict()) + "]" | 800 | + | } |
| 802 | - | } | 801 | + | if c.GetResolved() { |
| 803 | - | if c.GetResolved() { | 802 | + | mark += " [resolved]" |
| 804 | - | mark += " [resolved]" | 803 | + | } |
| 805 | - | } | 804 | + | prefix := " " |
| 806 | - | prefix := " " | 805 | + | body := indentText(c.GetText()) |
| 807 | - | body := indentText(c.GetText()) | 806 | + | if c.GetReplyTo() != "" { |
| 808 | - | if c.GetReplyTo() != "" { | 807 | + | prefix += " " |
| 809 | - | prefix += " " | 808 | + | body = " " + strings.ReplaceAll(body, "\n", "\n ") |
| 810 | - | body = " " + strings.ReplaceAll(body, "\n", "\n ") | 809 | + | } |
| 810 | + | _, err := fmt.Fprintf(w, "%s#%s %s, %s, %s%s\n%s\n", prefix, c.GetId(), clean(c.GetAuthor()), shortTime(c.GetCreatedAt(), "-"), where, mark, body) | |||
| 811 | + | if err != nil { | |||
| 812 | + | return err | |||
| 813 | + | } | |||
| 811 | } | 814 | } | ||
| 812 | - | _, err := fmt.Fprintf(w, "%s#%s %s, %s, %s%s\n%s\n", prefix, c.GetId(), clean(c.GetAuthor()), shortTime(c.GetCreatedAt(), "-"), where, mark, body) | 815 | + | return nil |
| 813 | - | return err | |||
| 814 | }}) | 816 | }}) | ||
| 815 | } | 817 | } | ||
| 816 | timeline = append(timeline, newIssueTimeline(d.GetIssueEvents(), true)...) | 818 | timeline = append(timeline, newIssueTimeline(d.GetIssueEvents(), true)...) | ||
M internal/app/cli_mr_test.go +18 -0
| @@ -689,3 +689,21 @@ func TestMRShowAutoMerge(t *testing.T) { | |||
| 689 | 689 | res = runCLI(t, envFor(url), "", "mr", "show", "alpha", "1").ok(t) | |
| 690 | 690 | assert.NotContains(t, res.stdout, "auto-merge") | |
| 691 | 691 | } | |
| 692 | + | ||
| 693 | + | func TestMRHistoryPreservesThreads(t *testing.T) { | |
| 694 | + | d := &ermite.GetMergeRequestResponse{MergeRequest: mrOne, Comments: []*ermite.Comment{ | |
| 695 | + | {Id: "a", Text: "root A", CreatedAt: ts("2026-10-01T10:00:00Z")}, | |
| 696 | + | {Id: "b", Text: "root B", CreatedAt: ts("2026-10-01T12:00:00Z")}, | |
| 697 | + | {Id: "reply", ReplyTo: "a", Text: "reply A", CreatedAt: ts("2026-10-01T13:00:00Z")}, | |
| 698 | + | }, IssueEvents: []*ermite.IssueEvent{ | |
| 699 | + | {Id: "1", Kind: ermite.IssueEventKind_ISSUE_EVENT_KIND_RESOLVED, Issue: 2, Actor: "merger", CreatedAt: ts("2026-10-01T11:00:00Z")}, | |
| 700 | + | }} | |
| 701 | + | var out strings.Builder | |
| 702 | + | require.NoError(t, printMRDetail(&out, d, false, false)) | |
| 703 | + | text := out.String() | |
| 704 | + | a, reply, event, b := strings.Index(text, "root A"), strings.Index(text, "reply A"), strings.Index(text, "merger resolved"), strings.Index(text, "root B") | |
| 705 | + | require.NotEqual(t, -1, event) | |
| 706 | + | assert.Less(t, a, reply) | |
| 707 | + | assert.Less(t, reply, event) | |
| 708 | + | assert.Less(t, event, b) | |
| 709 | + | } | |
| @@ -689,3 +689,21 @@ func TestMRShowAutoMerge(t *testing.T) { | |||||
| 689 | res = runCLI(t, envFor(url), "", "mr", "show", "alpha", "1").ok(t) | 689 | res = runCLI(t, envFor(url), "", "mr", "show", "alpha", "1").ok(t) | ||
| 690 | assert.NotContains(t, res.stdout, "auto-merge") | 690 | assert.NotContains(t, res.stdout, "auto-merge") | ||
| 691 | } | 691 | } | ||
| 692 | + | ||||
| 693 | + | func TestMRHistoryPreservesThreads(t *testing.T) { | |||
| 694 | + | d := &ermite.GetMergeRequestResponse{MergeRequest: mrOne, Comments: []*ermite.Comment{ | |||
| 695 | + | {Id: "a", Text: "root A", CreatedAt: ts("2026-10-01T10:00:00Z")}, | |||
| 696 | + | {Id: "b", Text: "root B", CreatedAt: ts("2026-10-01T12:00:00Z")}, | |||
| 697 | + | {Id: "reply", ReplyTo: "a", Text: "reply A", CreatedAt: ts("2026-10-01T13:00:00Z")}, | |||
| 698 | + | }, IssueEvents: []*ermite.IssueEvent{ | |||
| 699 | + | {Id: "1", Kind: ermite.IssueEventKind_ISSUE_EVENT_KIND_RESOLVED, Issue: 2, Actor: "merger", CreatedAt: ts("2026-10-01T11:00:00Z")}, | |||
| 700 | + | }} | |||
| 701 | + | var out strings.Builder | |||
| 702 | + | require.NoError(t, printMRDetail(&out, d, false, false)) | |||
| 703 | + | text := out.String() | |||
| 704 | + | a, reply, event, b := strings.Index(text, "root A"), strings.Index(text, "reply A"), strings.Index(text, "merger resolved"), strings.Index(text, "root B") | |||
| 705 | + | require.NotEqual(t, -1, event) | |||
| 706 | + | assert.Less(t, a, reply) | |||
| 707 | + | assert.Less(t, reply, event) | |||
| 708 | + | assert.Less(t, event, b) | |||
| 709 | + | } | |||