refactor: abstract VCS interface, decouple reviewer from GitLab - #18
Conversation
Introduce internal/vcs package with platform-agnostic types (MRChanges, DiffVersion, Comment, InlineCommentRequest) that replace direct use of gitlab.* types in the reviewer engine. Architecture: - internal/vcs: new package with platform-agnostic VCS types - internal/reviewer: VCSClient interface now uses vcs.* types - internal/gitlab: client methods return vcs.* types via toVCS() converters - internal/gitlab: types.go retains JSON tags for API deserialization - cmd/main.go: remains the only composition root that imports gitlab The reviewer package no longer imports internal/gitlab. This enables adding GitHub/Bitbucket support by implementing VCSClient against the same vcs.* types. Also consolidates repetitive flag/env config tests into table-driven tests (CodeRabbit nit from PR #17).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change introduces shared VCS types, converts GitLab responses at the client boundary, updates reviewer contracts and tests, and consolidates configuration-loading tests with additional validation cases. ChangesShared VCS abstraction
Configuration test consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Reviewer
participant GitLabClient
participant GitLabAPI
Reviewer->>GitLabClient: Request MR changes, versions, or notes
GitLabClient->>GitLabAPI: Fetch GitLab response
GitLabAPI-->>GitLabClient: Return GitLab response structs
GitLabClient->>GitLabClient: Convert responses with toVCS()
GitLabClient-->>Reviewer: Return shared VCS values
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/reviewer/reviewer_test.go (1)
1017-1019: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocal var
vcsshadows the newly-importedvcspackage.Each of these tests declares
vcs := &mockVCS{...}, which now shadows the package identifiervcsimported at Line 12 for the rest of the enclosing test function. It still compiles today because the initializer'svcs.DiffVersionreference is resolved before the new variable comes into scope, but it's a readability trap and blocks any later same-function use of thevcspackage.internal/reviewer/output_test.goalready renamed the equivalent local variable fromvcstomockClientfor this exact reason — apply the same rename here for consistency.♻️ Suggested rename (repeat for each occurrence)
- vcs := &mockVCS{ - mrVersions: []vcs.DiffVersion{{ID: 1, HeadSHA: "abc123", BaseSHA: "def456", StartSHA: "ghi789"}}, - } + mockClient := &mockVCS{ + mrVersions: []vcs.DiffVersion{{ID: 1, HeadSHA: "abc123", BaseSHA: "def456", StartSHA: "ghi789"}}, + }Then update the corresponding assertions (e.g.
vcs.getMRVersionsCalls→mockClient.getMRVersionsCalls) and theNewWithDiffSource(cfg, mm, vcs, ds)call site in each test.Also applies to: 1081-1087, 1142-1146, 1232-1238
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/reviewer/reviewer_test.go` around lines 1017 - 1019, Rename each local mockVCS variable named vcs in the affected tests to mockClient, including the declarations at all listed occurrences. Update every corresponding reference, assertion, and NewWithDiffSource argument to use mockClient while preserving the imported vcs.DiffVersion package references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/reviewer/reviewer_test.go`:
- Around line 1017-1019: Rename each local mockVCS variable named vcs in the
affected tests to mockClient, including the declarations at all listed
occurrences. Update every corresponding reference, assertion, and
NewWithDiffSource argument to use mockClient while preserving the imported
vcs.DiffVersion package references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 822c1a06-9f03-4b03-a4f7-9dc1af6cfc34
📒 Files selected for processing (10)
internal/config/config_test.gointernal/gitlab/client.gointernal/gitlab/client_test.gointernal/gitlab/types.gointernal/reviewer/interfaces.gointernal/reviewer/output.gointernal/reviewer/output_test.gointernal/reviewer/reviewer.gointernal/reviewer/reviewer_test.gointernal/vcs/types.go
…rter tests - Rename local 'vcs' variables to 'mockClient' in reviewer_test.go to avoid shadowing the vcs package import (consistency with output_test.go which already used this pattern) - Add explicit unit tests for toVCS() converters in types_test.go: MRChangesResponse, DiffVersion, Note, plus empty-input edge case (gitlab coverage 77.7% → 78.2%)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/gitlab/types_test.go (1)
8-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert to table-driven tests per path instructions.
All four test functions use repetitive sequential
if/t.Errorfblocks instead of table-driven cases witht.Runsubtests. Table-driven style would reduce duplication, name each assertion group as a labeled case, and let all sub-cases report independently on failure (e.g., separatingMRChangesResponse.toVCSfield checks fromDiffEntrysub-checks, and givingTestMRChangesResponse_toVCS_Emptyits own case within a shared table fortoVCS).As per path instructions, "Verify table-driven tests, proper cleanup, and race condition safety" for files matching
**/*_test.go.♻️ Example restructure for TestMRChangesResponse_toVCS
func TestMRChangesResponse_toVCS(t *testing.T) { - resp := &MRChangesResponse{...} - got := resp.toVCS() - if got.ID != 42 { ... } - ... + tests := []struct { + name string + resp *MRChangesResponse + want *vcs.MRChanges + }{ + { + name: "renamed and new files", + resp: &MRChangesResponse{...}, + want: &vcs.MRChanges{...}, + }, + { + name: "empty response", + resp: &MRChangesResponse{}, + want: &vcs.MRChanges{Changes: []vcs.DiffEntry{}}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.resp.toVCS() + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("toVCS() mismatch (-want +got):\n%s", diff) + } + }) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/gitlab/types_test.go` around lines 8 - 154, Convert the four tests around MRChangesResponse.toVCS, DiffVersion.toVCS, and Note.toVCS into table-driven tests using named cases and t.Run subtests. Group related assertions into labeled cases, including separate MRChangesResponse field and DiffEntry checks, and include the empty-response scenario in the shared toVCS table where appropriate. Preserve all existing coverage and expected values while allowing independent subcase failures.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/gitlab/types_test.go`:
- Around line 8-154: Convert the four tests around MRChangesResponse.toVCS,
DiffVersion.toVCS, and Note.toVCS into table-driven tests using named cases and
t.Run subtests. Group related assertions into labeled cases, including separate
MRChangesResponse field and DiffEntry checks, and include the empty-response
scenario in the shared toVCS table where appropriate. Preserve all existing
coverage and expected values while allowing independent subcase failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d7018c53-38e3-4649-9338-454a17ef7175
📒 Files selected for processing (2)
internal/gitlab/types_test.gointernal/reviewer/reviewer_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/reviewer/reviewer_test.go
Address CodeRabbit nit: convert sequential if/Errorf assertions into table-driven tests with t.Run subtests and cmp.Diff for cleaner output.
1 participant
Summary
Introduces
internal/vcspackage with platform-agnostic types, fully decoupling the reviewer engine from GitLab-specific types. This is the foundation for GitHub/Bitbucket support.Also addresses the CodeRabbit nit from PR #17 — consolidates repetitive flag/env tests into table-driven tests.
Architecture
New:
internal/vcs/types.goPlatform-agnostic types that the reviewer engine uses:
MRChanges— file changes from a merge/pull requestDiffEntry— single file changeDiffVersion— point-in-time diff snapshotComment— comment on a PR/MRInlineCommentRequest/InlineCommentPosition— inline comment payloadChanged:
internal/reviewer/interfaces.goVCSClientinterface now usesvcs.*types:Changed:
internal/gitlab/types.go: AddedtoVCS()converter methods onMRChangesResponse,DiffVersion,Noteclient.go: Public methods now returnvcs.*typesChanged:
internal/reviewer/reviewer.goandoutput.go: Replaced allgitlab.*type references withvcs.*internal/gitlabChanged: Tests
vcs.*typesVerification
-raceflaggitlabimports ininternal/reviewer/(verified via grep)What This Enables (Future PRs)
Summary by CodeRabbit
Bug Fixes
comment-modeandchunk-strategyvalues are rejected.Refactor
Tests