-
Notifications
You must be signed in to change notification settings - Fork 15
change the return to be object instead of array #1443
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?
Conversation
WalkthroughSystematically updates 21+ GitHub loader functions to wrap their return values in objects with a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Tagging OptionsShould a new tag be published when this PR is merged?
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
github/loaders/listOrgMembers.ts (1)
24-25: LGTM! Consider adding response validation.The data wrapping is correctly implemented. As an optional future enhancement, consider validating
response.okorresponse.statusbefore calling.json()to provide clearer error handling.github/loaders/listRepoContributors.ts (1)
28-29: Approve with optional type annotation suggestion.The wrapping pattern is correct and consistent with the PR objective.
For consistency with other loaders like
listOrgRepos.ts, consider adding an explicit return type annotation:const loader = async ( props: Props, _req: Request, ctx: AppContext, -) => { +): Promise<{ data: any }> => {github/loaders/listRepoPulls.ts (1)
28-29: Approve with optional type annotation suggestion.The return value is correctly wrapped in a
dataproperty.Consider adding an explicit return type annotation for consistency:
const loader = async ( props: Props, _req: Request, ctx: AppContext, -) => { +): Promise<{ data: any }> => {github/loaders/listRepoCollaborators.ts (1)
28-29: Approve with optional type annotation suggestion.The wrapping logic is correct and aligns with the MCP protocol requirement.
Consider adding an explicit return type annotation:
const loader = async ( props: Props, _req: Request, ctx: AppContext, -) => { +): Promise<{ data: any }> => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
github/loaders/getRepoStatsCommitActivity.ts(1 hunks)github/loaders/getRepoTrafficPopularPaths.ts(1 hunks)github/loaders/getRepoTrafficPopularReferrers.ts(1 hunks)github/loaders/listAuthenticatedUserRepos.ts(1 hunks)github/loaders/listOrgMembers.ts(1 hunks)github/loaders/listOrgOutsideCollaborators.ts(1 hunks)github/loaders/listOrgRepos.ts(1 hunks)github/loaders/listOrgTeams.ts(1 hunks)github/loaders/listPublicGists.ts(1 hunks)github/loaders/listRepoBranches.ts(1 hunks)github/loaders/listRepoCollaborators.ts(1 hunks)github/loaders/listRepoCommits.ts(1 hunks)github/loaders/listRepoContributors.ts(1 hunks)github/loaders/listRepoIssues.ts(3 hunks)github/loaders/listRepoPulls.ts(1 hunks)github/loaders/listRepoTeams.ts(1 hunks)github/loaders/listRepoWorkflowRuns.ts(1 hunks)github/loaders/listRepoWorkflows.ts(1 hunks)github/loaders/listTeamMembers.ts(1 hunks)github/loaders/listWorkflowRunJobs.ts(1 hunks)github/loaders/searchIssuesAndPRs.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
github/loaders/listAuthenticatedUserRepos.ts (1)
github/utils/types.ts (1)
Repository(22-33)
github/loaders/listPublicGists.ts (1)
github/utils/types.ts (1)
GistSimple(44-62)
github/loaders/listOrgRepos.ts (1)
github/utils/types.ts (1)
Repository(22-33)
github/loaders/listRepoIssues.ts (1)
github/utils/types.ts (1)
GithubIssueClean(74-86)
🔇 Additional comments (19)
github/loaders/listOrgOutsideCollaborators.ts (1)
24-25: LGTM! Consistent with PR-wide pattern.The implementation correctly wraps the response in
{ data }, aligning with MCP protocol requirements and maintaining consistency across all updated loaders.github/loaders/listRepoWorkflowRuns.ts (1)
34-35: LGTM! Consistent implementation.The data wrapping pattern is correctly applied, matching the structure used across all other loaders in this PR.
github/loaders/listTeamMembers.ts (1)
26-27: LGTM! Properly implements the data wrapper pattern.github/loaders/searchIssuesAndPRs.ts (1)
28-29: LGTM! Consistent with other loaders.github/loaders/listRepoWorkflows.ts (1)
27-28: LGTM! Correctly applies the data wrapper.github/loaders/listOrgTeams.ts (1)
24-25: LGTM! Final loader correctly updated.All 8 loaders now consistently return
{ data }wrapped responses, successfully addressing the MCP protocol requirement.github/loaders/listRepoBranches.ts (1)
26-27: Verify impact on external consumers or establish that this is a new API not yet in use.The breaking change is confirmed by git history (
return await response.json()→return { data }), but no downstream consumers were found within this repository. Since the loader is invoked through the framework's dynamic invocation pattern (ctx.invoke), static detection of all call sites is not possible.If this API is exposed externally or used by other projects, ensure those consumers have been updated to handle the wrapped return structure. If this is newly added or internal-only, document the expected return format for future consumers.
github/loaders/listPublicGists.ts (1)
17-22: LGTM! Good type safety with explicit return type.The explicit return type
Promise<{ data: GistSimple[] }>provides clear type safety for consumers. The implementation correctly wraps the response.github/loaders/listAuthenticatedUserRepos.ts (1)
19-24: LGTM! Type safety maintained with explicit return type.The implementation correctly wraps the repository array in a
{ data }object with proper type annotation.github/loaders/listRepoTeams.ts (1)
26-27: LGTM! Consistent with PR-wide pattern.The response wrapping is correctly implemented.
github/loaders/getRepoTrafficPopularReferrers.ts (1)
20-21: LGTM! Correctly wraps traffic data.The implementation follows the consistent pattern across all loaders.
github/loaders/getRepoTrafficPopularPaths.ts (1)
20-21: LGTM! Consistent implementation.The response wrapping correctly follows the PR pattern.
github/loaders/getRepoStatsCommitActivity.ts (1)
20-24: LGTM! Correctly preserves 202 status handling.The implementation properly maintains the dual return type:
{ status: 202 }when stats aren't ready, and{ data }for successful responses. This ensures consistency while preserving the existing behavior for the 202 edge case.github/loaders/listWorkflowRunJobs.ts (1)
29-30: LGTM! Consistent with PR-wide pattern.The implementation correctly wraps workflow run jobs data in a
{ data }object.github/loaders/listRepoCommits.ts (1)
30-31: Verify PR scope: github loaders not currently consumed anywhereCannot locate any consumers of the github loaders being modified in this PR. No direct imports, no manifest files, and no mod.ts entry point for the github integration were found despite comprehensive search. Either this PR is adding new loaders before integration setup, or consumers exist outside this repository. Manual verification needed to confirm whether the stated concern about breaking changes across 21+ loaders actually applies.
github/loaders/listOrgRepos.ts (1)
27-32: LGTM! Return shape correctly updated for MCP protocol compliance.The loader now wraps the repository array in a
dataproperty as required. The explicit return type annotationPromise<{ data: Repository[] }>provides clear type safety for consumers.Note: This is a breaking change—consumers must now access repositories via
result.datainstead of using the result directly.github/loaders/listRepoIssues.ts (3)
45-45: LGTM! Return type correctly reflects both success and error shapes.The updated return type
Promise<{ data: GithubIssueClean[] } | { error: true; message: string }>accurately describes the loader's behavior, allowing consumers to discriminate between successful results (withdata) and error responses (witherrorandmessage).
90-90: LGTM! Single issue path correctly wraps result indataproperty.The single issue retrieval path now returns
{ data: mapIssues([issue]) }, consistent with the new return shape.
110-110: LGTM! Multi-issue path correctly wraps result indataproperty.The multi-issue retrieval path now returns
{ data: mapped }, maintaining consistency with the updated return shape while preserving all error handling logic.
MCP protocol does not accept array
Summary by CodeRabbit