Skip to content

fix: reduce cognitive complexity and eliminate duplicated string literals in discussions.go#75

Open
devin-ai-integration[bot] wants to merge 1 commit intomainfrom
devin/1769472333-sonarqube-fixes
Open

fix: reduce cognitive complexity and eliminate duplicated string literals in discussions.go#75
devin-ai-integration[bot] wants to merge 1 commit intomainfrom
devin/1769472333-sonarqube-fixes

Conversation

@devin-ai-integration
Copy link

@devin-ai-integration devin-ai-integration bot commented Jan 27, 2026

Closes: N/A (SonarQube code quality improvements)

Summary

This PR addresses two SonarQube High severity issues in pkg/github/discussions.go:

  1. go:S1192 - String literals should not be duplicated
  2. go:S3776 - Cognitive Complexity of functions should not be too high (ListDiscussions had complexity 27, allowed 15)

Changes

Constants for duplicated strings:

  • descRepoOwner, descRepoName - parameter descriptions used across 4 functions
  • errFailedGetGQLClient - error message format used 4 times
  • categoryLabelFormat - category label format used 3 times

Refactored ListDiscussions to reduce cognitive complexity:

  • Extracted discussionNode type to eliminate duplicated inline struct definitions
  • Added mapDiscussionNodesToIssues() helper to consolidate the node-to-Issue mapping logic
  • Added fetchDiscussionsWithCategory() and fetchDiscussionsWithoutCategory() helpers to separate the GraphQL query logic

Human Review Checklist

  • Verify the extracted discussionNode struct works correctly with the githubv4 GraphQL library (the struct tags are preserved)
  • Confirm error handling behavior is preserved - errors from fetch functions are now handled after the if/else block
  • Check that all existing tests still pass (they do locally)

Tradeoffs

  • Added ~70 lines of new code (helper functions and type) while removing ~105 lines of duplicated code
  • The helper functions are package-private and specific to discussions, which is appropriate for this use case

Link to Devin run: https://app.devin.ai/sessions/4969ec2d384d4c6c93496a25d86475b1
Requested by: Shawn Azman (@ShawnAzman)


Open with Devin

…rals in discussions.go

- Add constants for duplicated string literals (descRepoOwner, descRepoName, errFailedGetGQLClient, categoryLabelFormat)
- Extract discussionNode type to reduce code duplication
- Extract mapDiscussionNodesToIssues helper function for mapping GraphQL nodes to GitHub Issue objects
- Extract fetchDiscussionsWithCategory and fetchDiscussionsWithoutCategory helper functions
- Refactor ListDiscussions function to use the new helper functions, reducing cognitive complexity from 27 to under 15

Fixes SonarQube issues:
- go:S1192 (String literals should not be duplicated)
- go:S3776 (Cognitive Complexity of functions should not be too high)

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration
Copy link
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@sonarqubecloud
Copy link

Copy link
Author

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Pagination parameters in ListDiscussionCategories are validated but never used

The ListDiscussionCategories function accepts pagination parameters (first, last, after, before) in its tool definition and validates them, but the GraphQL query completely ignores these parameters and always fetches first: 100.

Root Cause

The function defines pagination parameters (lines 300-315), decodes them (lines 319-326), and validates them (lines 331-343), but the GraphQL query on line 356 hardcodes first: 100 and the vars map (lines 359-362) only includes owner and repo:

var q struct {
    Repository struct {
        DiscussionCategories struct {
            Nodes []struct {
                ID   githubv4.ID
                Name githubv4.String
            }
        } `graphql:"discussionCategories(first: 100)"`  // Hardcoded!
    } `graphql:"repository(owner: $owner, name: $repo)"`
}
vars := map[string]interface{}{
    "owner": githubv4.String(params.Owner),
    "repo":  githubv4.String(params.Repo),
    // params.First, params.Last, params.After, params.Before are never used!
}

Impact: Users who try to paginate through discussion categories will find that their pagination parameters are silently ignored. The API always returns the first 100 categories regardless of what pagination parameters are provided. This is misleading and could cause issues for repositories with more than 100 discussion categories.

(Refers to line 356)

Prompt for agents
The ListDiscussionCategories function needs to be updated to actually use the pagination parameters (first, last, after, before) that are defined, decoded, and validated. The fix requires:

1. Modify the GraphQL query struct to use variables for pagination instead of hardcoding `first: 100`. The struct tag should be changed to something like `graphql:"discussionCategories(first: $first, after: $after)"` or use conditional query building.

2. Add the pagination parameters to the vars map:
   - If params.First is set, add it to vars
   - If params.Last is set, add it to vars
   - If params.After is set, add it to vars
   - If params.Before is set, add it to vars

3. Consider adding PageInfo to the response so users can actually use the pagination cursors.

Note: The githubv4 library may require different query structs for different pagination directions (first/after vs last/before), similar to how ListDiscussions handles the category filter with separate query functions.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant