Skip to content

Commit 58cd74d

Browse files
xcorailxcorail
committed
Make ListDiscussions more user-friendly by using category name filter, not id
1 parent ae5f1b2 commit 58cd74d

File tree

3 files changed

+136
-25
lines changed

3 files changed

+136
-25
lines changed

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -620,7 +620,7 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
620620
- **list_discussions** - List discussions for a repository
621621
- `owner`: Repository owner (string, required)
622622
- `repo`: Repository name (string, required)
623-
- `categoryId`: Filter by category ID (string, optional)
623+
- `category`: Filter by category name (string, optional)
624624
- `since`: Filter by date (ISO 8601 timestamp) (string, optional)
625625
- `first`: Pagination - Number of records to retrieve (number, optional)
626626
- `last`: Pagination - Number of records to retrieve from the end (number, optional)

‎pkg/github/discussions.go‎

Lines changed: 72 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,56 @@ import (
1414
"github.com/shurcooL/githubv4"
1515
)
1616

17+
// GetAllDiscussionCategories retrieves all discussion categories for a repository
18+
// by paginating through all pages and returns them as a map where the key is the
19+
// category name and the value is the category ID.
20+
func GetAllDiscussionCategories(ctx context.Context, client *githubv4.Client, owner, repo string) (map[string]string, error) {
21+
categories := make(map[string]string)
22+
var after string
23+
hasNextPage := true
24+
25+
for hasNextPage {
26+
// Prepare GraphQL query with pagination
27+
var q struct {
28+
Repository struct {
29+
DiscussionCategories struct {
30+
Nodes []struct {
31+
ID githubv4.ID
32+
Name githubv4.String
33+
}
34+
PageInfo struct {
35+
HasNextPage githubv4.Boolean
36+
EndCursor githubv4.String
37+
}
38+
} `graphql:"discussionCategories(first: 100, after: $after)"`
39+
} `graphql:"repository(owner: $owner, name: $repo)"`
40+
}
41+
42+
vars := map[string]interface{}{
43+
"owner": githubv4.String(owner),
44+
"repo": githubv4.String(repo),
45+
"after": githubv4.String(after),
46+
}
47+
48+
if err := client.Query(ctx, &q, vars); err != nil {
49+
return nil, fmt.Errorf("failed to query discussion categories: %w", err)
50+
}
51+
52+
// Add categories to the map
53+
for _, category := range q.Repository.DiscussionCategories.Nodes {
54+
categories[string(category.Name)] = fmt.Sprint(category.ID)
55+
}
56+
57+
// Check if there are more pages
58+
hasNextPage = bool(q.Repository.DiscussionCategories.PageInfo.HasNextPage)
59+
if hasNextPage {
60+
after = string(q.Repository.DiscussionCategories.PageInfo.EndCursor)
61+
}
62+
}
63+
64+
return categories, nil
65+
}
66+
1767
func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {
1868
return mcp.NewTool("list_discussions",
1969
mcp.WithDescription(t("TOOL_LIST_DISCUSSIONS_DESCRIPTION", "List discussions for a repository")),
@@ -29,8 +79,8 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
2979
mcp.Required(),
3080
mcp.Description("Repository name"),
3181
),
32-
mcp.WithString("categoryId",
33-
mcp.Description("Category ID filter"),
82+
mcp.WithString("category",
83+
mcp.Description("Category filter (name)"),
3484
),
3585
mcp.WithString("since",
3686
mcp.Description("Filter by date (ISO 8601 timestamp)"),
@@ -68,17 +118,17 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
68118
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
69119
// Decode params
70120
var params struct {
71-
Owner string
72-
Repo string
73-
CategoryID string
74-
Since string
75-
Sort string
76-
Direction string
77-
First int32
78-
Last int32
79-
After string
80-
Before string
81-
Answered bool
121+
Owner string
122+
Repo string
123+
Category string
124+
Since string
125+
Sort string
126+
Direction string
127+
First int32
128+
Last int32
129+
After string
130+
Before string
131+
Answered bool
82132
}
83133
if err := mapstructure.Decode(request.Params.Arguments, &params); err != nil {
84134
return mcp.NewToolResultError(err.Error()), nil
@@ -116,11 +166,19 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
116166
} `graphql:"discussions(categoryId: $categoryId, orderBy: {field: $sort, direction: $direction}, first: $first, after: $after, last: $last, before: $before, answered: $answered)"`
117167
} `graphql:"repository(owner: $owner, name: $repo)"`
118168
}
169+
categories, err := GetAllDiscussionCategories(ctx, client, params.Owner, params.Repo)
170+
if err != nil {
171+
return mcp.NewToolResultError(fmt.Sprintf("failed to get discussion categories: %v", err)), nil
172+
}
173+
var categoryID githubv4.ID = categories[params.Category]
174+
if categoryID == "" && params.Category != "" {
175+
return mcp.NewToolResultError(fmt.Sprintf("category '%s' not found", params.Category)), nil
176+
}
119177
// Build query variables
120178
vars := map[string]interface{}{
121179
"owner": githubv4.String(params.Owner),
122180
"repo": githubv4.String(params.Repo),
123-
"categoryId": githubv4.ID(params.CategoryID),
181+
"categoryId": categoryID,
124182
"sort": githubv4.DiscussionOrderField(params.Sort),
125183
"direction": githubv4.OrderDirection(params.Direction),
126184
"first": githubv4.Int(params.First),

‎pkg/github/discussions_test.go‎

Lines changed: 63 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,48 @@ func Test_ListDiscussions(t *testing.T) {
4242
assert.Contains(t, toolDef.InputSchema.Properties, "repo")
4343
assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo"})
4444

45-
// GraphQL query struct
45+
// mock for the call to list all categories: query struct, variables, response
46+
var q_cat struct {
47+
Repository struct {
48+
DiscussionCategories struct {
49+
Nodes []struct {
50+
ID githubv4.ID
51+
Name githubv4.String
52+
}
53+
PageInfo struct {
54+
HasNextPage githubv4.Boolean
55+
EndCursor githubv4.String
56+
}
57+
} `graphql:"discussionCategories(first: 100, after: $after)"`
58+
} `graphql:"repository(owner: $owner, name: $repo)"`
59+
}
60+
61+
vars_cat := map[string]interface{}{
62+
"owner": githubv4.String("owner"),
63+
"repo": githubv4.String("repo"),
64+
"after": githubv4.String(""),
65+
}
66+
67+
vars_cat_invalid := map[string]interface{}{
68+
"owner": githubv4.String("invalid"),
69+
"repo": githubv4.String("repo"),
70+
"after": githubv4.String(""),
71+
}
72+
73+
mockResp_cat := githubv4mock.DataResponse(map[string]any{
74+
"repository": map[string]any{
75+
"discussionCategories": map[string]any{
76+
"nodes": []map[string]any{
77+
{"id": "123", "name": "CategoryOne"},
78+
{"id": "456", "name": "CategoryTwo"},
79+
},
80+
},
81+
},
82+
})
83+
84+
mockResp_cat_invalid := githubv4mock.ErrorResponse("repository not found")
85+
86+
// mock for the call to ListDiscussions: query struct, variables, response
4687
var q struct {
4788
Repository struct {
4889
Discussions struct {
@@ -88,7 +129,7 @@ func Test_ListDiscussions(t *testing.T) {
88129
varsListWithCategory := map[string]interface{}{
89130
"owner": githubv4.String("owner"),
90131
"repo": githubv4.String("repo"),
91-
"categoryId": githubv4.ID("12345"),
132+
"categoryId": githubv4.ID("123"),
92133
"sort": githubv4.DiscussionOrderField(""),
93134
"direction": githubv4.OrderDirection(""),
94135
"first": githubv4.Int(0),
@@ -98,6 +139,9 @@ func Test_ListDiscussions(t *testing.T) {
98139
"answered": githubv4.Boolean(false),
99140
}
100141

142+
catMatcher := githubv4mock.NewQueryMatcher(q_cat, vars_cat, mockResp_cat)
143+
catMatcherInvalid := githubv4mock.NewQueryMatcher(q_cat, vars_cat_invalid, mockResp_cat_invalid)
144+
101145
tests := []struct {
102146
name string
103147
vars map[string]interface{}
@@ -106,6 +150,7 @@ func Test_ListDiscussions(t *testing.T) {
106150
expectError bool
107151
expectedIds []int64
108152
errContains string
153+
catMatcher githubv4mock.Matcher
109154
}{
110155
{
111156
name: "list all discussions",
@@ -117,6 +162,7 @@ func Test_ListDiscussions(t *testing.T) {
117162
response: mockResponseListAll,
118163
expectError: false,
119164
expectedIds: []int64{1, 2, 3},
165+
catMatcher: catMatcher,
120166
},
121167
{
122168
name: "invalid owner or repo",
@@ -128,18 +174,20 @@ func Test_ListDiscussions(t *testing.T) {
128174
response: mockErrorRepoNotFound,
129175
expectError: true,
130176
errContains: "repository not found",
177+
catMatcher: catMatcherInvalid,
131178
},
132179
{
133180
name: "list discussions with category",
134181
vars: varsListWithCategory,
135182
reqParams: map[string]interface{}{
136-
"owner": "owner",
137-
"repo": "repo",
138-
"categoryId": "12345",
183+
"owner": "owner",
184+
"repo": "repo",
185+
"category": "CategoryOne", // This should match the ID "123" in the mock response
139186
},
140187
response: mockResponseCategory,
141188
expectError: false,
142189
expectedIds: []int64{1},
190+
catMatcher: catMatcher,
143191
},
144192
{
145193
name: "list discussions with since date",
@@ -152,6 +200,7 @@ func Test_ListDiscussions(t *testing.T) {
152200
response: mockResponseListAll,
153201
expectError: false,
154202
expectedIds: []int64{2, 3},
203+
catMatcher: catMatcher,
155204
},
156205
{
157206
name: "both first and last parameters provided",
@@ -165,6 +214,7 @@ func Test_ListDiscussions(t *testing.T) {
165214
response: mockResponseListAll, // response doesn't matter since error occurs before GraphQL call
166215
expectError: true,
167216
errContains: "only one of 'first' or 'last' may be specified",
217+
catMatcher: catMatcher,
168218
},
169219
{
170220
name: "after with last parameters provided",
@@ -178,6 +228,7 @@ func Test_ListDiscussions(t *testing.T) {
178228
response: mockResponseListAll, // response doesn't matter since error occurs before GraphQL call
179229
expectError: true,
180230
errContains: "'after' cannot be used with 'last'. Did you mean to use 'before' instead?",
231+
catMatcher: catMatcher,
181232
},
182233
{
183234
name: "before with first parameters provided",
@@ -191,6 +242,7 @@ func Test_ListDiscussions(t *testing.T) {
191242
response: mockResponseListAll, // response doesn't matter since error occurs before GraphQL call
192243
expectError: true,
193244
errContains: "'before' cannot be used with 'first'. Did you mean to use 'after' instead?",
245+
catMatcher: catMatcher,
194246
},
195247
{
196248
name: "both after and before parameters provided",
@@ -204,13 +256,14 @@ func Test_ListDiscussions(t *testing.T) {
204256
response: mockResponseListAll, // response doesn't matter since error occurs before GraphQL call
205257
expectError: true,
206258
errContains: "only one of 'after' or 'before' may be specified",
259+
catMatcher: catMatcher,
207260
},
208261
}
209262

210263
for _, tc := range tests {
211264
t.Run(tc.name, func(t *testing.T) {
212265
matcher := githubv4mock.NewQueryMatcher(q, tc.vars, tc.response)
213-
httpClient := githubv4mock.NewMockedHTTPClient(matcher)
266+
httpClient := githubv4mock.NewMockedHTTPClient(matcher, tc.catMatcher)
214267
gqlClient := githubv4.NewClient(httpClient)
215268
_, handler := ListDiscussions(stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper)
216269

@@ -416,10 +469,10 @@ func Test_ListDiscussionCategories(t *testing.T) {
416469
vars := map[string]interface{}{
417470
"owner": githubv4.String("owner"),
418471
"repo": githubv4.String("repo"),
419-
"first": githubv4.Int(0),
420-
"last": githubv4.Int(0),
421-
"after": githubv4.String(""),
422-
"before": githubv4.String(""),
472+
"first": githubv4.Int(0), // Default to 100 categories
473+
"last": githubv4.Int(0), // Not used, but required by schema
474+
"after": githubv4.String(""), // Not used, but required by schema
475+
"before": githubv4.String(""), // Not used, but required by schema
423476
}
424477
mockResp := githubv4mock.DataResponse(map[string]any{
425478
"repository": map[string]any{

0 commit comments

Comments
 (0)