Skip to content

Commit 1342b0c

Browse files
committed
render attention first: unresolved sections, activity order
Output splits into an unresolved and a resolved section — a file with both kinds of threads appears in each — so what needs action reads first. Inside a section, files order as the change screen names them (patchset level, commit message, then paths); threads within a file follow the time of their latest comment; comments within a thread follow history. Comment ids break ties only on equal timestamps. Thread grouping and resolved state stay the change screen's replica; only the presentation order is the tool's own.
1 parent c1eb096 commit 1342b0c

10 files changed

Lines changed: 203 additions & 52 deletions

internal/tools/get-change-comments.go

Lines changed: 121 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,10 @@ func getChangeComments(c *gerritclient.Client) Tool {
4040
"shows — grouped by file and reconstructed into threads with their resolved " +
4141
"state. The calling account's unpublished draft comments are included, marked " +
4242
"draft=\"true\" and invisible to anyone else; thread state accounts for them, " +
43-
"matching what this account's Gerrit UI shows. Comment ids are the reply " +
44-
"anchors for post_comments; filter status=unresolved to see what still needs " +
45-
"action.",
43+
"matching what this account's Gerrit UI shows. Unresolved threads render " +
44+
"before resolved ones; threads follow their latest activity and comments " +
45+
"their history. Comment ids are the reply anchors for post_comments; filter " +
46+
"status=unresolved to see what still needs action.",
4647
}, func(ctx context.Context, _ *mcp.CallToolRequest, in getChangeCommentsInput,
4748
) (*mcp.CallToolResult, any, error) {
4849
if in.Status == "" {
@@ -132,11 +133,16 @@ func draftCount(all []changeComment) int {
132133
type thread struct {
133134
comments []changeComment
134135
unresolved bool
136+
// root is the comment that anchors the thread in the UI's grouping; its
137+
// path and timestamp place the thread in the output.
138+
root changeComment
135139
}
136140

137-
// pathPatchsetLevel is Gerrit's virtual path for patchset-level comments;
138-
// the change screen pins it before every real file.
139-
const pathPatchsetLevel = "/PATCHSET_LEVEL"
141+
// Gerrit's virtual file paths; the change screen pins them before real files.
142+
const (
143+
pathPatchsetLevel = "/PATCHSET_LEVEL"
144+
pathCommitMsg = "/COMMIT_MSG"
145+
)
140146

141147
// buildThreads groups comments the way the change screen does — a
142148
// bug-compatible replica of polygerrit's createCommentThreads (stable-3.13
@@ -177,17 +183,34 @@ func buildThreads(all []changeComment) []thread {
177183
threads := make([]thread, 0, len(acc))
178184

179185
for _, members := range acc {
186+
// State and grouping follow the UI's ordering; only after both are
187+
// fixed do the comments re-sort chronologically for display.
180188
last := members[len(members)-1]
189+
root := members[0]
190+
191+
slices.SortStableFunc(members, compareChrono)
181192

182193
threads = append(threads, thread{
183194
comments: members,
184195
unresolved: last.Unresolved != nil && *last.Unresolved,
196+
root: root,
185197
})
186198
}
187199

188200
return threads
189201
}
190202

203+
// compareChrono orders comments by time, falling back to comment id only on
204+
// equal timestamps — the display order of comments within a thread and of
205+
// threads within a file.
206+
func compareChrono(a, b changeComment) int {
207+
if c := compareUpdated(a.Updated, b.Updated); c != 0 {
208+
return c
209+
}
210+
211+
return strings.Compare(a.ID, b.ID)
212+
}
213+
191214
// sanitiseRanges mirrors polygerrit's pre-sort pass: a rangeless reply
192215
// inherits its parent's range so same-location comments sort together. The
193216
// pass is deliberately order-dependent — a reply processed before its parent
@@ -353,62 +376,121 @@ func matchesStatus(t thread, status string) bool {
353376
}
354377
}
355378

356-
// renderComments groups threads under the file path of their root comment,
357-
// in sorted path order.
379+
// renderComments emits attention first: an unresolved section, then a
380+
// resolved one — a file with both kinds of threads appears in each. Inside a
381+
// section files order as the change screen names them (patchset level, then
382+
// the commit message, then paths); threads within a file follow the time of
383+
// their latest comment; comments within a thread follow history. Comment ids
384+
// break ties everywhere.
358385
func renderComments(in getChangeCommentsInput, threads []thread, drafts int) string {
359-
byFile := map[string][]thread{}
386+
var unresolved, resolved []thread
360387

361388
for _, t := range threads {
362-
path := t.comments[0].path
389+
if !matchesStatus(t, in.Status) {
390+
continue
391+
}
363392

364-
byFile[path] = append(byFile[path], t)
393+
if t.unresolved {
394+
unresolved = append(unresolved, t)
395+
} else {
396+
resolved = append(resolved, t)
397+
}
365398
}
366399

367-
paths := make([]string, 0, len(byFile))
368-
for path := range byFile {
369-
paths = append(paths, path)
400+
var sections []string
401+
402+
if s := renderSection("unresolved", unresolved); s != "" {
403+
sections = append(sections, s)
370404
}
371405

372-
slices.Sort(paths)
406+
if s := renderSection("resolved", resolved); s != "" {
407+
sections = append(sections, s)
408+
}
373409

374-
var (
375-
rendered []string
376-
threadCount int
410+
root := llmxml.NewElement("comments",
411+
llmxml.Attr("change", in.Change),
412+
llmxml.Attr("filter", in.Status),
413+
llmxml.Attr("threads", len(unresolved)+len(resolved)),
414+
llmxml.Attr("drafts", drafts),
377415
)
378416

379-
for _, path := range paths {
380-
var fileThreads []string
417+
if len(sections) == 0 {
418+
return root.String()
419+
}
381420

382-
for _, t := range byFile[path] {
383-
if !matchesStatus(t, in.Status) {
384-
continue
385-
}
421+
return root.WrapText(strings.Join(sections, "\n")).String()
422+
}
386423

387-
threadCount++
424+
// renderSection wraps one resolution state's threads, grouped by the file
425+
// path of each thread's root comment.
426+
func renderSection(name string, threads []thread) string {
427+
if len(threads) == 0 {
428+
return ""
429+
}
388430

389-
fileThreads = append(fileThreads, renderThread(t))
390-
}
431+
byFile := map[string][]thread{}
432+
for _, t := range threads {
433+
byFile[t.root.path] = append(byFile[t.root.path], t)
434+
}
391435

392-
if len(fileThreads) == 0 {
393-
continue
436+
paths := make([]string, 0, len(byFile))
437+
for path := range byFile {
438+
paths = append(paths, path)
439+
}
440+
441+
slices.SortStableFunc(paths, compareRenderPaths)
442+
443+
rendered := make([]string, 0, len(paths))
444+
445+
for _, path := range paths {
446+
fileThreads := byFile[path]
447+
slices.SortStableFunc(fileThreads, compareThreads)
448+
449+
lines := make([]string, 0, len(fileThreads))
450+
for _, t := range fileThreads {
451+
lines = append(lines, renderThread(t))
394452
}
395453

396454
rendered = append(rendered, llmxml.NewElement("file", llmxml.Attr("path", path)).
397-
WrapText(strings.Join(fileThreads, "\n")).String())
455+
WrapText(strings.Join(lines, "\n")).String())
398456
}
399457

400-
root := llmxml.NewElement("comments",
401-
llmxml.Attr("change", in.Change),
402-
llmxml.Attr("filter", in.Status),
403-
llmxml.Attr("threads", threadCount),
404-
llmxml.Attr("drafts", drafts),
405-
)
458+
return llmxml.NewElement(name, llmxml.Attr("count", len(threads))).
459+
WrapText(strings.Join(rendered, "\n")).String()
460+
}
406461

407-
if len(rendered) == 0 {
408-
return root.String()
462+
// Display ranks for file paths: the change level first, the commit message
463+
// second, everything else by path.
464+
const (
465+
rankPatchsetLevel = iota
466+
rankCommitMsg
467+
rankFilePath
468+
)
469+
470+
// compareRenderPaths orders files for display by rank, then by path.
471+
func compareRenderPaths(a, b string) int {
472+
rank := func(p string) int {
473+
switch p {
474+
case pathPatchsetLevel:
475+
return rankPatchsetLevel
476+
case pathCommitMsg:
477+
return rankCommitMsg
478+
default:
479+
return rankFilePath
480+
}
409481
}
410482

411-
return root.WrapText(strings.Join(rendered, "\n")).String()
483+
if ra, rb := rank(a), rank(b); ra != rb {
484+
return ra - rb
485+
}
486+
487+
return strings.Compare(a, b)
488+
}
489+
490+
// compareThreads orders threads within a file by the time of their latest
491+
// comment — the conversation in the order it last moved.
492+
func compareThreads(a, b thread) int {
493+
return compareChrono(a.comments[len(a.comments)-1], b.comments[len(b.comments)-1])
412494
}
413495

414496
func renderThread(t thread) string {

internal/tools/post-comments.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ func validateReplyTargets(ctx context.Context, c *gerritclient.Client, in postCo
222222
// commentable on every change.
223223
func magicPath(path string) bool {
224224
switch path {
225-
case "/COMMIT_MSG", "/PATCHSET_LEVEL", "/MERGE_LIST":
225+
case pathCommitMsg, pathPatchsetLevel, "/MERGE_LIST":
226226
return true
227227
default:
228228
return false

internal/tools/read-tools_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,34 @@ func Test_GetChangeComments(t *testing.T) {
278278
golden(t, "change-comments-ui-fork", out)
279279
})
280280

281+
t.Run("threads follow their latest comment, not their root", func(t *testing.T) {
282+
t.Parallel()
283+
284+
// Thread A starts first but its reply is the newest activity; thread
285+
// B started later and never moved. B's conversation last moved
286+
// earlier, so B renders first.
287+
const published = ")]}'\n" + `{
288+
"core/scanner.go": [
289+
{"id": "a-root", "line": 1, "patch_set": 1, "message": "First opened", "unresolved": false,
290+
"updated": "2026-07-01 10:00:00.000000000",
291+
"author": {"_account_id": 8, "name": "Bob", "username": "bob"}},
292+
{"id": "a-reply", "in_reply_to": "a-root", "line": 1, "patch_set": 1, "message": "Late reply",
293+
"unresolved": false,
294+
"updated": "2026-07-01 15:00:00.000000000",
295+
"author": {"_account_id": 7, "name": "Alice", "username": "alice"}},
296+
{"id": "b-root", "line": 2, "patch_set": 1, "message": "Second opened", "unresolved": false,
297+
"updated": "2026-07-01 11:00:00.000000000",
298+
"author": {"_account_id": 8, "name": "Bob", "username": "bob"}}
299+
]
300+
}`
301+
302+
cs := commentsSession(t, published, emptyDraftsJSON)
303+
304+
out := callTool(t, cs, "get_change_comments", map[string]any{"change": "123"})
305+
306+
golden(t, "change-comments-latest-order", out)
307+
})
308+
281309
t.Run("same-timestamp fork orders by comment id", func(t *testing.T) {
282310
t.Parallel()
283311

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,14 @@
11
<comments change="123" filter="all" threads="3" drafts="0">
2+
<unresolved count="1">
3+
<file path="core/scanner.go">
4+
<thread resolved="false">
5+
<comment id="c3" author="Bob (bob)" date="2026-07-02T09:00:00Z" patch_set="2" lines="20-25">
6+
This block races
7+
</comment>
8+
</thread>
9+
</file>
10+
</unresolved>
11+
<resolved count="2">
212
<file path="core/scanner.go">
313
<thread resolved="true">
414
<comment id="c1" author="Bob (bob)" date="2026-07-01T10:00:00Z" patch_set="1" line="10">
@@ -8,11 +18,6 @@ Is this nil-safe?
818
Fixed in ps2
919
</comment>
1020
</thread>
11-
<thread resolved="false">
12-
<comment id="c3" author="Bob (bob)" date="2026-07-02T09:00:00Z" patch_set="2" lines="20-25">
13-
This block races
14-
</comment>
15-
</thread>
1621
</file>
1722
<file path="docs/readme.md">
1823
<thread resolved="true">
@@ -21,4 +26,5 @@ Orphan reply
2126
</comment>
2227
</thread>
2328
</file>
29+
</resolved>
2430
</comments>

internal/tools/testdata/change-comments-drafts.golden

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
<comments change="123" filter="unresolved" threads="1" drafts="1">
2+
<unresolved count="1">
23
<file path="core/scanner.go">
34
<thread resolved="false">
45
<comment id="c1" author="Bob (bob)" date="2026-07-01T10:00:00Z" patch_set="1" line="10">
@@ -12,4 +13,5 @@ Still crashes on empty input
1213
</comment>
1314
</thread>
1415
</file>
16+
</unresolved>
1517
</comments>
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
<comments change="123" filter="all" threads="2" drafts="0">
2+
<resolved count="2">
3+
<file path="core/scanner.go">
4+
<thread resolved="true">
5+
<comment id="b-root" author="Bob (bob)" date="2026-07-01T11:00:00Z" patch_set="1" line="2">
6+
Second opened
7+
</comment>
8+
</thread>
9+
<thread resolved="true">
10+
<comment id="a-root" author="Bob (bob)" date="2026-07-01T10:00:00Z" patch_set="1" line="1">
11+
First opened
12+
</comment>
13+
<comment id="a-reply" author="Alice (alice)" date="2026-07-01T15:00:00Z" patch_set="1" line="1" in_reply_to="a-root">
14+
Late reply
15+
</comment>
16+
</thread>
17+
</file>
18+
</resolved>
19+
</comments>
Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,20 @@
11
<comments change="123" filter="all" threads="2" drafts="0">
2-
<file path="core/new-name.go">
3-
<thread resolved="true">
4-
<comment id="r2" author="Alice (alice)" date="2026-07-02T10:00:00Z" patch_set="2" line="5" in_reply_to="r1">
5-
Done
6-
</comment>
7-
</thread>
8-
</file>
2+
<unresolved count="1">
93
<file path="core/old-name.go">
104
<thread resolved="false">
115
<comment id="r1" author="Bob (bob)" date="2026-07-01T10:00:00Z" patch_set="1" line="5">
126
Rename this too
137
</comment>
148
</thread>
159
</file>
10+
</unresolved>
11+
<resolved count="1">
12+
<file path="core/new-name.go">
13+
<thread resolved="true">
14+
<comment id="r2" author="Alice (alice)" date="2026-07-02T10:00:00Z" patch_set="2" line="5" in_reply_to="r1">
15+
Done
16+
</comment>
17+
</thread>
18+
</file>
19+
</resolved>
1620
</comments>

internal/tools/testdata/change-comments-tiebreak.golden

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
<comments change="123" filter="all" threads="1" drafts="0">
2+
<unresolved count="1">
23
<file path="core/scanner.go">
34
<thread resolved="false">
45
<comment id="c1" author="Bob (bob)" date="2026-07-01T10:00:00Z" patch_set="1" line="10">
@@ -12,4 +13,5 @@ Not fixed
1213
</comment>
1314
</thread>
1415
</file>
16+
</unresolved>
1517
</comments>

0 commit comments

Comments
 (0)