Add Issue Comments functionality #11
Conversation
…ion support This commit implements full GitHub Issue Comments API support including: Core Comment Operations: - List all comments on an issue - Get a specific comment by ID - Create new comments on issues - Update existing comments - Delete comments Comment Reactions: - List all reactions on a comment - Add reactions to comments (supports +1, -1, laugh, confused, heart, hooray, rocket, eyes) - Remove reactions from comments Implementation Details: - Created Comment and Reaction data classes with fromArray/toArray methods - Created ManagesIssueComments trait with all comment operations - Created ManagesIssueCommentsInterface contract - Updated IssuesService and IssuesServiceInterface to include comment management - Added comprehensive unit tests for Comment and Reaction data classes - All tests passing (22 tests, 81 assertions) - Code formatted with Laravel Pint Resolves #8
WalkthroughThis PR adds comprehensive GitHub Issue Comments API support by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/Unit/Data/ReactionTest.php (2)
8-28: Consider assertingcreatedAtfield.The test validates
id,content, and nesteduser, but doesn't assert thecreatedAtproperty. Adding an assertion would ensure the date parsing works correctly.expect($reaction->id)->toBe(123); expect($reaction->content)->toBe('+1'); expect($reaction->user)->toBeInstanceOf(User::class); expect($reaction->user->login)->toBe('testuser'); + expect($reaction->createdAt)->toBeInstanceOf(\DateTime::class); + expect($reaction->createdAt->format('Y-m-d'))->toBe('2023-01-01'); });
30-46: Consider assertingcreated_atin output array.The test verifies most fields but omits the
created_atkey from assertions.expect($array['user'])->toBeArray(); expect($array['user']['login'])->toBe('testuser'); + expect($array['created_at'])->toBe('2023-01-01T12:00:00+00:00'); });tests/Unit/Data/CommentTest.php (2)
8-33: Consider assertingcreatedAtandupdatedAtfields.The test validates most properties but omits assertions for the DateTime fields. Adding these would ensure date parsing works correctly.
expect($comment->htmlUrl)->toBe('https://github.com/owner/repo/issues/comments/123'); expect($comment->issueUrl)->toBe('https://api.github.com/repos/owner/repo/issues/1'); + expect($comment->createdAt)->toBeInstanceOf(\DateTime::class); + expect($comment->updatedAt)->toBeInstanceOf(\DateTime::class); });
35-56: Consider asserting timestamp fields in output.The test could verify
created_atandupdated_atare correctly formatted in the output array.expect($array['html_url'])->toBe('https://github.com/owner/repo/issues/comments/123'); expect($array['issue_url'])->toBe('https://api.github.com/repos/owner/repo/issues/1'); + expect($array['created_at'])->toBe('2023-01-01T12:00:00+00:00'); + expect($array['updated_at'])->toBe('2023-01-02T12:00:00+00:00'); });src/Traits/ManagesIssueComments.php (2)
57-64: Consider treating 404-on-delete as success (idempotent delete), depending on library conventions.
If callers might “delete-if-exists”, returningfalseon 404 can be noisy; optionally normalize404 => truefor idempotency (but only if this matches the rest of the SDK).$response = $this->connector->send( $this->connector->delete("/repos/{$owner}/{$repo}/issues/comments/{$commentId}") ); - return $response->successful(); + return $response->successful() || $response->status() === 404;(Apply similarly for
removeCommentReactionif desired.)Also applies to: 90-97
79-88: Constrain$contentto allowed reaction values (enum/constant) to prevent avoidable 422s.
Right now any string can be passed; consider an enum (PHP 8.1+) or constants onReaction/a dedicatedReactionContenttype.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Contracts/IssuesServiceInterface.php(1 hunks)src/Contracts/ManagesIssueCommentsInterface.php(1 hunks)src/Data/Comment.php(1 hunks)src/Data/Reaction.php(1 hunks)src/Services/IssuesService.php(1 hunks)src/Traits/ManagesIssueComments.php(1 hunks)tests/Unit/Data/CommentTest.php(1 hunks)tests/Unit/Data/ReactionTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/Data/Comment.php (4)
src/Data/Issue.php (1)
Issue(9-86)src/Data/Reaction.php (3)
__construct(11-16)fromArray(18-26)toArray(28-36)src/Services/IssuesService.php (1)
__construct(21-23)src/Data/User.php (1)
User(7-38)
tests/Unit/Data/ReactionTest.php (2)
src/Data/Reaction.php (3)
Reaction(9-37)fromArray(18-26)toArray(28-36)src/Data/User.php (1)
User(7-38)
src/Traits/ManagesIssueComments.php (3)
src/Data/Comment.php (2)
Comment(9-46)fromArray(21-32)src/Data/Reaction.php (2)
Reaction(9-37)fromArray(18-26)src/Contracts/ManagesIssueCommentsInterface.php (8)
listComments(16-16)getComment(18-18)createComment(20-20)updateComment(22-22)deleteComment(24-24)listCommentReactions(29-29)addCommentReaction(31-31)removeCommentReaction(33-33)
tests/Unit/Data/CommentTest.php (2)
src/Data/Comment.php (3)
Comment(9-46)fromArray(21-32)toArray(34-45)src/Data/User.php (1)
User(7-38)
src/Data/Reaction.php (1)
src/Data/User.php (1)
User(7-38)
🔇 Additional comments (12)
src/Services/IssuesService.php (1)
10-10: LGTM!The
ManagesIssueCommentstrait integration follows the established pattern used by other traits (ManagesIssueAssignees,ManagesIssueLabels,ManagesIssues). Clean and consistent approach.Also applies to: 17-17
tests/Unit/Data/ReactionTest.php (1)
48-68: Good parameterized coverage for reaction types.The test correctly validates all eight GitHub reaction types (
+1,-1,laugh,confused,heart,hooray,rocket,eyes).src/Contracts/IssuesServiceInterface.php (1)
7-7: LGTM!The interface extension follows the established pattern, maintaining alphabetical ordering of parent interfaces.
src/Contracts/ManagesIssueCommentsInterface.php (1)
11-34: Well-designed interface covering all required operations.The interface comprehensively covers the GitHub Issue Comments API with clear method signatures. PHPDoc generics for Collection return types provide good IDE support.
src/Data/Reaction.php (3)
9-16: LGTM - Consistent with existing data class patterns.The readonly class follows the established patterns in the codebase (User, Issue). Properties are well-typed.
18-26: Follows established deserialization pattern.Direct array access is consistent with other data classes (
User,Issue). For API responses with a known schema, this fail-fast approach is appropriate.
28-36: Clean serialization with consistent key naming.The
toArray()method properly converts camelCase properties to snake_case keys and uses ISO 8601 format for timestamps.src/Data/Comment.php (3)
9-19: LGTM - Clean readonly data class.The Comment class follows established patterns with appropriate property types. Good use of readonly class for immutability.
21-32: Consistent deserialization from API response format.The
fromArray()method correctly maps snake_case API keys to camelCase properties and handles nested User objects.
34-45: Symmetric serialization maintains API compatibility.The
toArray()method correctly reverses thefromArray()transformation, producing output compatible with the GitHub API format.src/Traits/ManagesIssueComments.php (2)
26-34: Comment CRUD request/response mapping looks consistent and clean.
Paths and verbs match the intended endpoints;Comment::fromArray($response->json())keeps the trait straightforward.Also applies to: 35-55
66-88: Verify the connector package handles required GitHub API headers for reactions endpoints.GitHub reactions endpoints require the
Accept: application/vnd.github+jsonheader (the legacy "squirrel-girl" preview is no longer needed as of December 2025). It's also recommended to includeX-GitHub-Api-Version: 2022-11-28. SinceConduitUI\GitHubConnector\Connectoris an external dependency (conduit-ui/connector: ^1.0), confirm it sets these headers correctly when making reactions API calls.
| /** | ||
| * @return \Illuminate\Support\Collection<int, \ConduitUI\Issue\Data\Comment> | ||
| */ | ||
| public function listComments(string $owner, string $repo, int $issueNumber): Collection | ||
| { | ||
| $response = $this->connector->send( | ||
| $this->connector->get("/repos/{$owner}/{$repo}/issues/{$issueNumber}/comments") | ||
| ); | ||
|
|
||
| return collect($response->json()) | ||
| ->map(fn (array $data) => Comment::fromArray($data)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub REST API "List issue comments" endpoint per_page page pagination parameters
💡 Result:
-
Endpoint: GET /repos/{owner}/{repo}/issues/{issue_number}/comments. [1]
-
Pagination query params:
- per_page — number of items per page. Default: 30. Maximum: 100. [1][2]
- page — page number to fetch. Default: 1. [1][2]
-
How to iterate: responses include a Link header (rel="next", rel="last", etc.) to follow additional pages; you can also request specific pages via ?per_page=NN&page=MM. [2]
Example: GET https://api.github.com/repos/OWNER/REPO/issues/123/comments?per_page=100&page=2. [1][2]
Sources:
[1] GitHub REST API — List issue comments (issues/comments) (GitHub Docs).
[2] Using pagination in the REST API (GitHub Docs).
🏁 Script executed:
# Search for pagination patterns in the codebase
rg -i "per_page|pagination" --type php -A 2 -B 2Repository: conduit-ui/issue
Length of output: 42
🏁 Script executed:
# Check the connector class to understand how it handles requests
fd connector -t f | head -20Repository: conduit-ui/issue
Length of output: 42
🏁 Script executed:
# Look for other list methods in the trait to see if pagination is already used
ast-grep --pattern 'public function list$_($_) { $$$ }'Repository: conduit-ui/issue
Length of output: 42
🏁 Script executed:
# Show fd connector output
fd connector -t fRepository: conduit-ui/issue
Length of output: 42
🏁 Script executed:
# Show ast-grep output for list methods
ast-grep --pattern 'public function list$_($_) { $$$ }' -A 5Repository: conduit-ui/issue
Length of output: 42
🏁 Script executed:
# Find and examine the connector class
fd -t f -i connector | grep -E '\.(php|ts|js)$'Repository: conduit-ui/issue
Length of output: 42
🏁 Script executed:
# List all public methods in ManagesIssueComments trait
rg "public function" src/Traits/ManagesIssueComments.phpRepository: conduit-ui/issue
Length of output: 846
🏁 Script executed:
# Find where connector is defined/imported
rg "connector" src/Traits/ManagesIssueComments.php -B 5 -A 2Repository: conduit-ui/issue
Length of output: 2847
🏁 Script executed:
# Search for connector class definition in the codebase
fd -type f -name "*onnector*.php"Repository: conduit-ui/issue
Length of output: 230
🏁 Script executed:
# Check the class that uses this trait to understand connector origin
rg "ManagesIssueComments" --type php -B 5 -A 5Repository: conduit-ui/issue
Length of output: 3253
🏁 Script executed:
# Look for composer.json to see dependencies
cat composer.json 2>/dev/null || echo "No composer.json found"Repository: conduit-ui/issue
Length of output: 2747
List comments and list comment reactions need pagination support—consumers will silently miss data beyond the first page.
GitHub's "list issue comments" and "list comment reactions" endpoints are paginated with per_page (default: 30, max: 100) and page (default: 1) query parameters. Both listComments() and listCommentReactions() currently lack pagination parameters, so callers cannot request additional pages or customize page size.
Update both methods to accept pagination parameters, for example:
- public function listComments(string $owner, string $repo, int $issueNumber): Collection
+ public function listComments(string $owner, string $repo, int $issueNumber, int $perPage = 30, int $page = 1): Collection
{
$response = $this->connector->send(
- $this->connector->get("/repos/{$owner}/{$repo}/issues/{$issueNumber}/comments")
+ $this->connector->get("/repos/{$owner}/{$repo}/issues/{$issueNumber}/comments", [
+ 'per_page' => $perPage,
+ 'page' => $page,
+ ])
);
}Apply the same change to listCommentReactions(). Also update the ManagesIssueCommentsInterface contract to reflect these new parameters.
🤖 Prompt for AI Agents
In src/Traits/ManagesIssueComments.php around lines 13 to 24, listComments
currently fetches only the first page of issue comments; update it to accept
optional pagination parameters (int $page = 1, int $perPage = 30) and pass them
as query params to the connector (e.g. ?page=...&per_page=...), ensuring
per_page max handling can be left to caller or validated; apply the same change
to listCommentReactions() (add same signature and forward pagination query
params) and update the ManagesIssueCommentsInterface contract to declare the new
parameters and return type unchanged.
|
Features already shipped in #14 (bundled with gate workflow) |
Summary
Features Implemented
Core Comment Operations
listComments()getComment()createComment()updateComment()deleteComment()Comment Reactions
listCommentReactions()addCommentReaction()(supports +1, -1, laugh, confused, heart, hooray, rocket, eyes)removeCommentReaction()Implementation Details
New Files
src/Data/Comment.php- Comment data class with fromArray/toArray methodssrc/Data/Reaction.php- Reaction data class with fromArray/toArray methodssrc/Traits/ManagesIssueComments.php- Trait implementing all comment operationssrc/Contracts/ManagesIssueCommentsInterface.php- Interface defining comment management contracttests/Unit/Data/CommentTest.php- Unit tests for Comment data classtests/Unit/Data/ReactionTest.php- Unit tests for Reaction data class (includes dataset for all reaction types)Modified Files
src/Services/IssuesService.php- Added ManagesIssueComments traitsrc/Contracts/IssuesServiceInterface.php- Extended with ManagesIssueCommentsInterfaceTest Plan
GitHub API Endpoints Covered
GET /repos/{owner}/{repo}/issues/{issue_number}/commentsGET /repos/{owner}/{repo}/issues/comments/{comment_id}POST /repos/{owner}/{repo}/issues/{issue_number}/commentsPATCH /repos/{owner}/{repo}/issues/comments/{comment_id}DELETE /repos/{owner}/{repo}/issues/comments/{comment_id}GET /repos/{owner}/{repo}/issues/comments/{comment_id}/reactionsPOST /repos/{owner}/{repo}/issues/comments/{comment_id}/reactionsDELETE /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions/{reaction_id}Resolves #8
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.