-
-
Notifications
You must be signed in to change notification settings - Fork 21
feat(mr): support viewing unresolved discussions on merge requests #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -427,7 +427,7 @@ const ALL_TOOLS = [ | |
| }, | ||
| { | ||
| name: "list_merge_request_discussions", | ||
| description: "List all discussions (threaded comments) on a merge request", | ||
| description: "List discussions (threaded comments) on a merge request. Pass unresolved_only=true to filter to threads that still have at least one unresolved note.", | ||
| inputSchema: createJsonSchema(ListMergeRequestDiscussionsSchema), | ||
| readOnly: true | ||
| }, | ||
|
|
@@ -1322,6 +1322,16 @@ server.setRequestHandler(CallToolRequestSchema, async (request: CallToolRequest) | |
| } | ||
| ); | ||
|
|
||
| if (args.unresolved_only) { | ||
| const filteredItems = discussions.items.filter(d => | ||
| d.notes.some(n => n.resolvable && n.resolved === false) | ||
| ); | ||
|
Comment on lines
+1326
to
+1328
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The filtering logic here should be consistent with the definition of a resolved thread used in the formatter. A thread is unresolved if at least one of its resolvable notes is not resolved. Using const filteredItems = discussions.items.filter(d =>
d.notes.some(n => n.resolvable && n.resolved !== true)
); |
||
| return formatDiscussionsResponse({ | ||
| count: filteredItems.length, | ||
| items: filteredItems | ||
| }); | ||
| } | ||
|
|
||
| return formatDiscussionsResponse(discussions); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for calculating
unresolvedCountis inconsistent with how the discussion-levelresolvedflag is computed later in the function (lines 347-349). Specifically,unresolvedCountusesn.resolved === false, while the discussion-levelresolvedflag usesn.resolved === truein aneverycheck. If a resolvable note hasresolved: undefined(which is allowed by the schema), the thread will be marked asresolved: falsein the output but counted as "resolved" in the summary stats. Using a consistent check ensures the summary counts match the item flags.