feat: full GitLab integration — parity with GitHub#1120
Open
jcfs wants to merge 12 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full GitLab integration: backend watch/task models and DB schema, store CRUD and reservation semantics, service APIs (watches, merge, search, sync, cleanup), background poller, HTTP + WebSocket routes (plus mock), orchestrator event wiring and startup, and frontend API, types, Zustand state, and hooks. ChangesBackend GitLab Integration
Frontend State and Data Access
Sequence Diagram(s) sequenceDiagram
participant Client
participant WSDispatcher
participant GitLabService
participant Store
participant Poller
Client->>WSDispatcher: WS action (trigger/list/update)
WSDispatcher->>GitLabService: call service handler
GitLabService->>Store: read/write watches/tasks
Poller->>GitLabService: periodic poll -> discoveries
GitLabService->>Client: publish events / return responses
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Claude finished @jcfs's task in 6m 27s —— View job Code Review
FindingsBlocker (must fix before merge)1. Missing backend tests —
2. Missing frontend tests —
3. Label filter silently dropped when
// current:
filter := watch.CustomQuery // filter = customQuery
if filter == "" {
filter = "assignee_username=..."
}
if len(watch.Labels) > 0 {
filter = filter + "&labels=..." // appended to filter, but...
}
issues, err := client.ListIssues(ctx, filter, watch.CustomQuery)
// buildIssueSearchQuery ignores `filter` ↑ because customQuery != ""
if watch.CustomQuery != "" {
q := watch.CustomQuery
if len(watch.Labels) > 0 {
q += "&labels=" + url.QueryEscape(strings.Join(watch.Labels, ","))
}
return client.ListIssues(ctx, "", q)
}
filter := "assignee_username=" + url.QueryEscape(username)
if len(watch.Labels) > 0 {
filter += "&labels=" + url.QueryEscape(strings.Join(watch.Labels, ","))
}
return client.ListIssues(ctx, filter, "")4. Watch events are published but no orchestrator subscriber creates tasks —
Suggestion (recommended, doesn't block)5.
6. Redundant
filter := "reviewer_username=" + url.QueryEscape(username)
if watch.CustomQuery != "" {
filter = "" // customQuery overrides; filter is ignored
}
return client.SearchMRs(ctx, filter, watch.CustomQuery)7.
Summary
Verdict: Blocked — fix blockers first. The infrastructure here is well-built (dedup protocol, cleanup policies, action presets, WS handler symmetry with GitHub) but items 1–2 (missing tests) and item 4 (unsubscribed events → no task creation) need to be addressed before this can ship. Item 3 is a logic bug that would confuse users who set both |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
26 issues found across 34 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/backend/internal/gitlab/store_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/store_watches.go:170">
P1: Normalize `PollIntervalSeconds` in `UpdateReviewWatch` the same way as create, otherwise updates can persist invalid (<=0) intervals.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store_watches.go:416">
P1: `UpdateIssueWatch` should clamp/default `PollIntervalSeconds`; currently invalid values can be persisted on update.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-status.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-status.ts:18">
P1: The initial-effect guard uses `status` truthiness instead of a loaded marker, so `null` results trigger repeated status refetches (disconnect/error loops).</violation>
</file>
<file name="apps/backend/internal/gitlab/service_cleanup.go">
<violation number="1" location="apps/backend/internal/gitlab/service_cleanup.go:59">
P1: Issue-watch fetch errors are defaulted to `CleanupPolicyAuto`, which can delete issue tasks incorrectly when watch metadata retrieval fails transiently. Erroring watches should be treated as unknown and skipped until a later sweep.
(Based on your team's feedback about treating watch fetch errors as unknown during cleanup sweeps.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/mock_client.go">
<violation number="1" location="apps/backend/internal/gitlab/mock_client.go:440">
P2: `SearchProjects` is documented as substring search but currently uses exact equality, causing partial queries to fail in mock tests.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-stats.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-stats.ts:18">
P2: This effect can hammer `/api/v1/gitlab/stats` in a tight loop when the fetch fails (or returns null), because `stats` stays null and the guard never transitions to a loaded state.</violation>
</file>
<file name="apps/backend/internal/gitlab/poller.go">
<violation number="1" location="apps/backend/internal/gitlab/poller.go:38">
P1: Poller lifecycle state is not thread-safe; concurrent Start/Stop can race, leak loops, or block shutdown.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts:16">
P2: Action preset loading is tracked globally instead of per workspace, so one workspace request can block or misreport loading for another.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts:21">
P1: The effect can enter a continuous retry loop when the presets fetch fails, causing repeated requests and unnecessary load.</violation>
</file>
<file name="apps/backend/internal/gitlab/service_sync.go">
<violation number="1" location="apps/backend/internal/gitlab/service_sync.go:15">
P2: TriggerMRSync breaks the optional-store contract by returning an error instead of an empty result when the store is unset.</violation>
</file>
<file name="apps/backend/cmd/kandev/main.go">
<violation number="1" location="apps/backend/cmd/kandev/main.go:443">
P2: GitLab service is started without wiring task cleanup dependencies, so GitLab cleanup operations can fail at runtime with "task deleter not configured".</violation>
</file>
<file name="apps/backend/internal/gitlab/handlers.go">
<violation number="1" location="apps/backend/internal/gitlab/handlers.go:121">
P2: Do not ignore payload parse errors in list/search handlers; malformed WS payloads currently fall through and can trigger unintended unfiltered operations.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts:33">
P1: The `loaded` guard is not workspace-scoped, so changing `workspaceId` can leave stale review watches and skip the needed refetch.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts:36">
P2: Guard async watch fetch callbacks by workspace so an older request cannot overwrite state after `workspaceId` changes.
(Based on your team's feedback about guarding workspace-scoped async fetches.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/action_presets_service.go">
<violation number="1" location="apps/backend/internal/gitlab/action_presets_service.go:11">
P2: `ResetActionPresets` should validate `workspaceID` to avoid silent resets with empty workspace IDs.</violation>
<violation number="2" location="apps/backend/internal/gitlab/action_presets_service.go:38">
P2: Partial updates incorrectly persist default presets for untouched kinds, which can freeze defaults and block future default updates.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts:27">
P1: `loaded` is not scoped per workspace, so changing `workspaceId` can skip refetch and leave stale issue watches from another workspace.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts:30">
P1: Guard the async callbacks so stale responses from a previous workspace cannot overwrite the current workspace state.
(Based on your team's feedback about guarding workspace-scoped async fetches.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/service_search.go">
<violation number="1" location="apps/backend/internal/gitlab/service_search.go:48">
P2: `GetStats` masks GitLab/auth/query failures as zero counts, which can return misleading stats instead of signaling an error.</violation>
</file>
<file name="apps/web/lib/api/domains/gitlab-api.ts">
<violation number="1" location="apps/web/lib/api/domains/gitlab-api.ts:146">
P2: `UpdateReviewWatchRequest` incorrectly allows `workspace_id`, but the backend update schema does not support it. This can lead callers to send unsupported fields that are silently ignored.</violation>
<violation number="2" location="apps/web/lib/api/domains/gitlab-api.ts:195">
P2: `UpdateIssueWatchRequest` should exclude `workspace_id`; allowing it in the type creates a frontend/backend contract mismatch.</violation>
</file>
<file name="apps/backend/internal/gitlab/store.go">
<violation number="1" location="apps/backend/internal/gitlab/store.go:86">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_review_watches` to avoid full scans in polling/list paths.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store.go:116">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_issue_watches` to keep watch polling queries scalable.</violation>
</file>
<file name="apps/backend/internal/gitlab/controller_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/controller_watches.go:173">
P2: Return 404 for missing review watches instead of mapping not-found to 500 in trigger endpoint.</violation>
<violation number="2" location="apps/backend/internal/gitlab/controller_watches.go:252">
P2: Return 404 for missing issue watches instead of treating not-found as internal server errors.</violation>
</file>
<file name="apps/backend/internal/gitlab/pat_client_actions.go">
<violation number="1" location="apps/backend/internal/gitlab/pat_client_actions.go:123">
P2: `ListUserProjects` only fetches the first page (`per_page=100`) and drops additional projects. This can silently hide repositories in project pickers/watch setup for users in many projects.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
26 issues found across 34 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/backend/internal/gitlab/store_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/store_watches.go:170">
P1: Normalize `PollIntervalSeconds` in `UpdateReviewWatch` the same way as create, otherwise updates can persist invalid (<=0) intervals.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store_watches.go:416">
P1: `UpdateIssueWatch` should clamp/default `PollIntervalSeconds`; currently invalid values can be persisted on update.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-status.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-status.ts:18">
P1: The initial-effect guard uses `status` truthiness instead of a loaded marker, so `null` results trigger repeated status refetches (disconnect/error loops).</violation>
</file>
<file name="apps/backend/internal/gitlab/service_cleanup.go">
<violation number="1" location="apps/backend/internal/gitlab/service_cleanup.go:59">
P1: Issue-watch fetch errors are defaulted to `CleanupPolicyAuto`, which can delete issue tasks incorrectly when watch metadata retrieval fails transiently. Erroring watches should be treated as unknown and skipped until a later sweep.
(Based on your team's feedback about treating watch fetch errors as unknown during cleanup sweeps.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/mock_client.go">
<violation number="1" location="apps/backend/internal/gitlab/mock_client.go:440">
P2: `SearchProjects` is documented as substring search but currently uses exact equality, causing partial queries to fail in mock tests.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-stats.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-stats.ts:18">
P2: This effect can hammer `/api/v1/gitlab/stats` in a tight loop when the fetch fails (or returns null), because `stats` stays null and the guard never transitions to a loaded state.</violation>
</file>
<file name="apps/backend/internal/gitlab/poller.go">
<violation number="1" location="apps/backend/internal/gitlab/poller.go:38">
P1: Poller lifecycle state is not thread-safe; concurrent Start/Stop can race, leak loops, or block shutdown.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts:16">
P2: Action preset loading is tracked globally instead of per workspace, so one workspace request can block or misreport loading for another.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts:21">
P1: The effect can enter a continuous retry loop when the presets fetch fails, causing repeated requests and unnecessary load.</violation>
</file>
<file name="apps/backend/internal/gitlab/service_sync.go">
<violation number="1" location="apps/backend/internal/gitlab/service_sync.go:15">
P2: TriggerMRSync breaks the optional-store contract by returning an error instead of an empty result when the store is unset.</violation>
</file>
<file name="apps/backend/cmd/kandev/main.go">
<violation number="1" location="apps/backend/cmd/kandev/main.go:443">
P2: GitLab service is started without wiring task cleanup dependencies, so GitLab cleanup operations can fail at runtime with "task deleter not configured".</violation>
</file>
<file name="apps/backend/internal/gitlab/handlers.go">
<violation number="1" location="apps/backend/internal/gitlab/handlers.go:121">
P2: Do not ignore payload parse errors in list/search handlers; malformed WS payloads currently fall through and can trigger unintended unfiltered operations.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts:33">
P1: The `loaded` guard is not workspace-scoped, so changing `workspaceId` can leave stale review watches and skip the needed refetch.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts:36">
P2: Guard async watch fetch callbacks by workspace so an older request cannot overwrite state after `workspaceId` changes.
(Based on your team's feedback about guarding workspace-scoped async fetches.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/action_presets_service.go">
<violation number="1" location="apps/backend/internal/gitlab/action_presets_service.go:11">
P2: `ResetActionPresets` should validate `workspaceID` to avoid silent resets with empty workspace IDs.</violation>
<violation number="2" location="apps/backend/internal/gitlab/action_presets_service.go:38">
P2: Partial updates incorrectly persist default presets for untouched kinds, which can freeze defaults and block future default updates.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts:27">
P1: `loaded` is not scoped per workspace, so changing `workspaceId` can skip refetch and leave stale issue watches from another workspace.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts:30">
P1: Guard the async callbacks so stale responses from a previous workspace cannot overwrite the current workspace state.
(Based on your team's feedback about guarding workspace-scoped async fetches.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/service_search.go">
<violation number="1" location="apps/backend/internal/gitlab/service_search.go:48">
P2: `GetStats` masks GitLab/auth/query failures as zero counts, which can return misleading stats instead of signaling an error.</violation>
</file>
<file name="apps/web/lib/api/domains/gitlab-api.ts">
<violation number="1" location="apps/web/lib/api/domains/gitlab-api.ts:146">
P2: `UpdateReviewWatchRequest` incorrectly allows `workspace_id`, but the backend update schema does not support it. This can lead callers to send unsupported fields that are silently ignored.</violation>
<violation number="2" location="apps/web/lib/api/domains/gitlab-api.ts:195">
P2: `UpdateIssueWatchRequest` should exclude `workspace_id`; allowing it in the type creates a frontend/backend contract mismatch.</violation>
</file>
<file name="apps/backend/internal/gitlab/store.go">
<violation number="1" location="apps/backend/internal/gitlab/store.go:86">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_review_watches` to avoid full scans in polling/list paths.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store.go:116">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_issue_watches` to keep watch polling queries scalable.</violation>
</file>
<file name="apps/backend/internal/gitlab/controller_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/controller_watches.go:173">
P2: Return 404 for missing review watches instead of mapping not-found to 500 in trigger endpoint.</violation>
<violation number="2" location="apps/backend/internal/gitlab/controller_watches.go:252">
P2: Return 404 for missing issue watches instead of treating not-found as internal server errors.</violation>
</file>
<file name="apps/backend/internal/gitlab/pat_client_actions.go">
<violation number="1" location="apps/backend/internal/gitlab/pat_client_actions.go:123">
P2: `ListUserProjects` only fetches the first page (`per_page=100`) and drops additional projects. This can silently hide repositories in project pickers/watch setup for users in many projects.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| apps/backend/internal/gitlab/service_issue_watches.go | Issue watch CRUD + poller logic; fetchIssues calls GetAuthenticatedUser unconditionally, blocking custom-query watches when the user-info endpoint fails. |
| apps/backend/internal/gitlab/service_watches.go | MR/review watch CRUD + polling; fetchReviewMRs also calls GetAuthenticatedUser unconditionally; initialReviewCheck goroutine uses context.Background() and is untracked (flagged in prior review). |
| apps/backend/internal/gitlab/service_cleanup.go | Cleanup sweep for review/issue tasks; nil store guards are present and correct; logic is clean. |
| apps/backend/internal/gitlab/poller.go | Three-loop background poller; mutex properly guards started/cancel across concurrent Start/Stop calls; shutdown sequence is correct. |
| apps/backend/internal/orchestrator/event_handlers_gitlab.go | GitLab event-to-task pipeline; goroutines use context.Background() and are not tracked in a WaitGroup (flagged in prior review). |
| apps/backend/internal/gitlab/store_watches.go | Full CRUD for all five new SQLite tables; parameterised queries throughout; ON CONFLICT dedup for task reservation is correctly implemented. |
| apps/backend/internal/gitlab/handlers.go | ~35 WebSocket handlers wired; input validation is present on all mutating handlers. |
| apps/backend/internal/gitlab/controller_watches.go | HTTP surface for watches, presets, write actions, projects; parameter extraction and error mapping look correct. |
| apps/backend/internal/gitlab/pat_client_actions.go | PATClient extensions for merge, labels, assignees, projects; API endpoints and payload shapes are correct for GitLab v4. |
| apps/web/lib/api/domains/gitlab-api.ts | Frontend HTTP wrappers for all new backend routes; response types match backend shapes; IDs are URL-encoded on parameterised routes. |
| apps/web/lib/state/slices/gitlab/gitlab-slice.ts | Immer-backed Zustand slice for all new GitLab state; action factories are consistently structured and match the declared types. |
Sequence Diagram
sequenceDiagram
participant Main as main.go
participant Poller as Poller
participant Svc as Service
participant Store as SQLite Store
participant GL as GitLab API
participant Bus as EventBus
participant Orch as Orchestrator
Main->>Poller: Start(ctx)
activate Poller
loop Every 5 min (reviewWatchLoop)
Poller->>Svc: ListAllReviewWatches(ctx)
Svc->>Store: SELECT gitlab_review_watches
Store-->>Svc: "[]*ReviewWatch"
loop per watch (if due)
Poller->>Svc: CheckReviewWatch(ctx, watch)
Svc->>GL: GetAuthenticatedUser + SearchMRs
GL-->>Svc: "[]*MR"
Svc->>Store: HasReviewMRTask (dedup)
Svc->>Store: RecordReviewWatchPoll
Svc-->>Poller: new MRs
Poller->>Bus: Publish(GitLabNewReviewMR)
Bus->>Orch: handleGitLabNewReviewMR
Orch->>Svc: ReserveReviewMRTask
Orch->>Orch: createGitLabReviewTask (goroutine)
Orch->>Svc: AssignReviewMRTaskID
end
end
Main->>Poller: Stop()
deactivate Poller
Reviews (12): Last reviewed commit: "fix(gitlab): address final review round ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Claude finished @jcfs's task in 6m 45s —— View job Code Review
SummaryThe latest commit (475bd15) addressed the two blockers from the previous Claude review: the label-filter bug in Remaining issues found in this pass: Blockers (must fix before merge)
Should fix (won't block CI but are real bugs)
Noted (intentional per PR description, no action required)
|
There was a problem hiding this comment.
5 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/backend/internal/gitlab/handlers.go">
<violation number="1" location="apps/backend/internal/gitlab/handlers.go:121">
P2: Do not ignore payload parse errors in list/search handlers; malformed WS payloads currently fall through and can trigger unintended unfiltered operations.</violation>
</file>
<file name="apps/backend/internal/gitlab/action_presets_service.go">
<violation number="1" location="apps/backend/internal/gitlab/action_presets_service.go:11">
P2: `ResetActionPresets` should validate `workspaceID` to avoid silent resets with empty workspace IDs.</violation>
<violation number="2" location="apps/backend/internal/gitlab/action_presets_service.go:38">
P2: Partial updates incorrectly persist default presets for untouched kinds, which can freeze defaults and block future default updates.</violation>
</file>
<file name="apps/web/lib/api/domains/gitlab-api.ts">
<violation number="1" location="apps/web/lib/api/domains/gitlab-api.ts:146">
P2: `UpdateReviewWatchRequest` incorrectly allows `workspace_id`, but the backend update schema does not support it. This can lead callers to send unsupported fields that are silently ignored.</violation>
<violation number="2" location="apps/web/lib/api/domains/gitlab-api.ts:195">
P2: `UpdateIssueWatchRequest` should exclude `workspace_id`; allowing it in the type creates a frontend/backend contract mismatch.</violation>
</file>
<file name="apps/backend/internal/gitlab/store.go">
<violation number="1" location="apps/backend/internal/gitlab/store.go:86">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_review_watches` to avoid full scans in polling/list paths.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store.go:116">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_issue_watches` to keep watch polling queries scalable.</violation>
</file>
<file name="apps/backend/internal/gitlab/controller_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/controller_watches.go:173">
P2: Return 404 for missing review watches instead of mapping not-found to 500 in trigger endpoint.</violation>
<violation number="2" location="apps/backend/internal/gitlab/controller_watches.go:252">
P2: Return 404 for missing issue watches instead of treating not-found as internal server errors.</violation>
</file>
<file name="apps/backend/internal/gitlab/store_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/store_watches.go:170">
P1: Normalize `PollIntervalSeconds` in `UpdateReviewWatch` the same way as create, otherwise updates can persist invalid (<=0) intervals.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store_watches.go:416">
P1: `UpdateIssueWatch` should clamp/default `PollIntervalSeconds`; currently invalid values can be persisted on update.</violation>
</file>
<file name="apps/backend/internal/gitlab/service_search.go">
<violation number="1" location="apps/backend/internal/gitlab/service_search.go:48">
P2: `GetStats` masks GitLab/auth/query failures as zero counts, which can return misleading stats instead of signaling an error.</violation>
</file>
<file name="apps/backend/internal/gitlab/mock_client.go">
<violation number="1" location="apps/backend/internal/gitlab/mock_client.go:440">
P2: `SearchProjects` is documented as substring search but currently uses exact equality, causing partial queries to fail in mock tests.</violation>
</file>
<file name="apps/backend/internal/gitlab/poller.go">
<violation number="1" location="apps/backend/internal/gitlab/poller.go:38">
P1: Poller lifecycle state is not thread-safe; concurrent Start/Stop can race, leak loops, or block shutdown.</violation>
</file>
<file name="apps/backend/internal/gitlab/service_sync.go">
<violation number="1" location="apps/backend/internal/gitlab/service_sync.go:15">
P2: TriggerMRSync breaks the optional-store contract by returning an error instead of an empty result when the store is unset.</violation>
</file>
<file name="apps/backend/internal/gitlab/pat_client_actions.go">
<violation number="1" location="apps/backend/internal/gitlab/pat_client_actions.go:123">
P2: `ListUserProjects` only fetches the first page (`per_page=100`) and drops additional projects. This can silently hide repositories in project pickers/watch setup for users in many projects.</violation>
</file>
<file name="apps/backend/internal/gitlab/store_watches_test.go">
<violation number="1" location="apps/backend/internal/gitlab/store_watches_test.go:78">
P2: The post-delete assertion drops `GetReviewWatch` errors, which can let the test pass on query failures.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store_watches_test.go:91">
P2: The test ignores an error on the duplicate reserve path, so it can pass even when the operation fails unexpectedly.</violation>
</file>
<file name="apps/backend/internal/gitlab/service_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/service_watches.go:786">
P2: `appendLabelsToQuery` uses a raw substring match for `labels=`; parse query params instead to avoid skipping label injection on false matches.</violation>
</file>
<file name="apps/backend/internal/orchestrator/event_handlers_gitlab.go">
<violation number="1" location="apps/backend/internal/orchestrator/event_handlers_gitlab.go:82">
P2: This adds a new ad-hoc watcher task-creation flow instead of using `WatcherDispatchCoordinator`, reintroducing the `createXIssueTask` pattern that the backend architecture explicitly forbids.</violation>
<violation number="2" location="apps/backend/internal/orchestrator/event_handlers_gitlab.go:83">
P2: Returning early when `gitlabService` is nil drops watch events; this should fail open (skip dedup) instead of skipping task creation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 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.
Inline comments:
In `@apps/backend/internal/gitlab/handlers.go`:
- Around line 553-563: The wsNewDiscussionNote WebSocket handler currently
doesn't validate that the incoming request Body is non-empty, letting empty
discussion notes proceed and cause internal errors; update the handler
(wsNewDiscussionNote) to check req.Body after parseInto and return
badRequest(msg, "project, iid, discussion_id, body required") when req.Body ==
"" (alongside existing checks for Project, IID, and DiscussionID) so empty-body
requests are rejected like the HTTP handler.
In `@apps/backend/internal/gitlab/mock_client.go`:
- Around line 433-444: The mock SearchProjects implementation is too strict
because MockClient.SearchProjects only returns projects when p.PathWithNamespace
or p.Path exactly equals the query; change it to perform partial (substring)
matching so autocomplete/search flows work: normalize the query (trim and
optionally lowercase) and check if strings.Contains(p.PathWithNamespace, query)
|| strings.Contains(p.Path, query) (use case-insensitive comparison if
production does) and append matches to out; keep the same return types and
behavior when query is empty.
In `@apps/backend/internal/gitlab/service_cleanup.go`:
- Around line 147-151: The current check calls
checker.HasUserAuthoredMessage(ctx, t.TaskID) but ignores the error, which can
cause deletion on failure; change the logic in the block that checks
CleanupPolicyAuto to capture the (authored, err) result and if err != nil treat
it as a fail-safe by returning false (i.e., do not proceed with cleanup). Update
the conditional around HasUserAuthoredMessage (referencing checker,
HasUserAuthoredMessage, CleanupPolicyAuto, and t.TaskID) so any error causes an
immediate false return instead of continuing.
- Around line 74-78: The check currently discards the error from
checker.HasUserAuthoredMessage(ctx, t.TaskID) causing a fail-open delete; change
the logic in the CleanupPolicyAuto branch to handle the error explicitly: call
checker.HasUserAuthoredMessage(ctx, t.TaskID), if it returns (true, nil) return
false (preserve), if it returns (false, err) treat this as a safe failure — log
the error and return false to skip deletion (i.e., preserve the task) so
transient errors don't cause user-interacted tasks to be deleted; update the
code around CleanupPolicyAuto and the checker.HasUserAuthoredMessage call
accordingly.
In `@apps/backend/internal/gitlab/service_watches.go`:
- Around line 233-235: Normalize the updated poll interval the same way the
create path does instead of writing raw values: in the update handler where
req.PollIntervalSeconds is checked and assigned to rw.PollIntervalSeconds, read
the incoming value into a local (e.g., v := *req.PollIntervalSeconds) and clamp
it using the same bounds used by create (e.g., if v <= 0 then set to
DefaultPollIntervalSeconds, and if v < MinPollIntervalSeconds set to
MinPollIntervalSeconds, and optionally cap to MaxPollIntervalSeconds if
applicable) before assigning rw.PollIntervalSeconds = v; use the same constants
or helper (MinPollIntervalSeconds, DefaultPollIntervalSeconds or the create-path
clamp function) to ensure consistent behavior.
- Around line 457-470: UpdateIssueWatch currently calls applyIssueWatchPatch(iw,
req) without checking if req is nil, causing a panic when req is nil; add a nil
guard: only call applyIssueWatchPatch when req != nil, and only validate
req.CleanupPolicy when req is non-nil (i.e., move the CleanupPolicy validation
inside the req != nil branch). Locate UpdateIssueWatch and implement the
nil-check around applyIssueWatchPatch and the IsValidCleanupPolicy check so the
function safely updates the issue watch when req is provided and simply persists
the existing iw when req is nil.
- Around line 190-203: UpdateReviewWatch currently calls
applyReviewWatchPatch(rw, req) and dereferences req.CleanupPolicy without
guarding req for nil; add a nil-check at the start of UpdateReviewWatch to
return a clear error (e.g., "nil update request") if req is nil, and only call
applyReviewWatchPatch and evaluate req.CleanupPolicy when req != nil. Reference:
UpdateReviewWatch, applyReviewWatchPatch, IsValidCleanupPolicy,
requireStore().GetReviewWatch/UpdateReviewWatch.
- Around line 786-787: The current check uses strings.Contains(customQuery,
"labels=") which can false-match keys like "mylabels="; update the condition to
detect the literal query key "labels" instead—e.g. parse the query with net/url
(url.ParseQuery(strings.TrimPrefix(customQuery, "?"))) and test for the "labels"
key in the returned map, or use a regex that looks for "(^|&|\\?)labels=";
replace the existing strings.Contains(customQuery, "labels=") usage accordingly
so existence is determined by the actual query key, not a raw substring.
In `@apps/backend/internal/orchestrator/event_handlers_gitlab.go`:
- Line 98: The calls to s.gitlabService.ReleaseReviewMRTask (and the similar
ReleaseReviewIssueTask) currently ignore returned errors; change those call
sites to capture the error, log a clear error message including identifiers
(evt.ReviewWatchID, mr.ProjectPath, mr.IID or issue equivalents) and return or
surface the error so the caller can handle retries/cleanup rather than silently
leaving the dedup-reservation in place; ensure you update both the
ReleaseReviewMRTask and ReleaseReviewIssueTask invocations to check the error
and handle it consistently.
In `@apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts`:
- Around line 20-29: The effect currently swallows errors and never marks
presets as attempted, causing an infinite refetch when workspaceId exists and
presets is still unset; update the .catch handler for
getActionPresets(workspaceId) to set a sentinel value via the existing setter
(e.g., call set(workspaceId, null) or set(workspaceId, [])) to mark that the
fetch was attempted and failed, and keep the .finally(() => setLoading(false))
as-is so loading is cleared; reference the useEffect block and the
functions/variables getActionPresets, set, presets, loading, workspaceId, and
setLoading to locate where to add the catch-side set call.
In `@apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts`:
- Around line 26-33: The effect uses a global loaded/loading flag so switching
workspaceId can skip fetching and show stale watches; make the load state
scope-aware by tracking which workspace was last loaded (e.g., add
lastLoadedWorkspaceId or a map keyed by workspaceId) and update setLoading/set
accordingly: when workspaceId changes clear or compare against
lastLoadedWorkspaceId before returning early, and ensure the useEffect depends
on workspaceId and lastLoadedWorkspaceId so listIssueWatches(workspaceId, ...)
runs whenever the current workspace’s watches haven’t been loaded yet; update
the logic around useEffect, loaded, loading, listIssueWatches, setLoading, and
set to reference the per-workspace loaded marker.
In `@apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts`:
- Around line 32-39: The effect currently gates fetching by a global loaded flag
so when workspaceId changes and loaded is already true the fetch is skipped
leaving stale data; update the logic in the useEffect that references
workspaceId, loaded, loading, listReviewWatches, set and setLoading so that
loaded is scoped to the current workspace (e.g., track loadedForWorkspaceId or
reset loaded when workspaceId changes) and/or include workspaceId in the gating
check—ensure the effect triggers a fetch for a new workspaceId by clearing or
scoping the loaded flag before calling listReviewWatches and then set or
setLoading as before.
In `@apps/web/hooks/domains/gitlab/use-gitlab-stats.ts`:
- Around line 17-24: The effect currently keeps retrying because a failed fetch
sets stats to null (falsy) so the guard (if (stats || loading) return) doesn't
stop subsequent runs; add a failure flag (e.g., statsError with setStatsError)
or a retry counter and set it in the catch of fetchGitLabStats, then include
that flag/counter in the effect's guard and dependency array so the effect
returns when a permanent failure is detected; update the catch handler to call
setStatsError(true) (or increment the retry counter) instead of only
setStats(null), and add statsError (or retry count) to the if-check and
dependency list to prevent endless retries.
In `@apps/web/hooks/domains/gitlab/use-gitlab-status.ts`:
- Around line 17-24: The effect in use-gitlab-status (the useEffect that checks
status/loading and calls fetchGitLabStatus) re-triggers after a failed fetch
because status is set to null; prevent the retry loop by adding and checking a
fetch-attempt flag (e.g., statusTried or statusFetchError) in the hook state and
dependencies, set that flag to true in both .then and .catch handlers (or set an
error flag in .catch), and update the effect condition to return early if
statusTried (or statusFetchError) is true; use the existing symbols status,
loading, setStatus, setStatusLoading, and fetchGitLabStatus to implement this
change so a failed fetch does not immediately re-run.
In `@apps/web/lib/state/slices/gitlab/gitlab-slice.ts`:
- Around line 6-8: The slice's workspace-scoped watch state
(gitlabReviewWatches, gitlabIssueWatches, gitlabMRWatches) uses a single global
loaded/loading/items bag which can serve stale data across workspace switches;
change the shape to key items/loading/loaded by workspaceId (e.g. {
[workspaceId]: { items, loaded, loading } }) or add a currentLoadedWorkspaceId
per watch and invalidate/reset the loaded flag and items when workspaceId
changes, updating all selectors and reducers that read/write gitlab*Watches (and
the corresponding logic referenced in the same file lines ~58-117) so consumers
always fetch fresh lists for the active workspace.
In `@apps/web/lib/state/store.ts`:
- Around line 190-195: Post-slice override only restores taskMRs but misses the
new GitLab fields so merged/hydrated values for gitlabReviewWatches,
gitlabIssueWatches, gitlabMRWatches, gitlabActionPresets, gitlabStats and
gitlabStatus get lost; update the post-slice override in store.ts (the block
that restores from merged state after slice creation) to also copy these
properties from the merged/hydrated state into the store (same pattern used for
taskMRs) so the new GitLab sub-state is preserved on store creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e05f6286-65df-42cd-a3e6-54113d4a9926
📒 Files selected for processing (40)
apps/backend/cmd/kandev/helpers.goapps/backend/cmd/kandev/main.goapps/backend/cmd/kandev/services.goapps/backend/internal/events/types.goapps/backend/internal/gitlab/action_presets_service.goapps/backend/internal/gitlab/action_presets_test.goapps/backend/internal/gitlab/client.goapps/backend/internal/gitlab/controller.goapps/backend/internal/gitlab/controller_watches.goapps/backend/internal/gitlab/handlers.goapps/backend/internal/gitlab/mock_client.goapps/backend/internal/gitlab/mock_controller.goapps/backend/internal/gitlab/noop_client.goapps/backend/internal/gitlab/pat_client_actions.goapps/backend/internal/gitlab/poller.goapps/backend/internal/gitlab/service.goapps/backend/internal/gitlab/service_cleanup.goapps/backend/internal/gitlab/service_merge.goapps/backend/internal/gitlab/service_search.goapps/backend/internal/gitlab/service_sync.goapps/backend/internal/gitlab/service_watches.goapps/backend/internal/gitlab/service_watches_test.goapps/backend/internal/gitlab/store.goapps/backend/internal/gitlab/store_watches.goapps/backend/internal/gitlab/store_watches_test.goapps/backend/internal/gitlab/watch_models.goapps/backend/internal/orchestrator/event_handlers_gitlab.goapps/backend/internal/orchestrator/service.goapps/backend/pkg/websocket/actions.goapps/web/hooks/domains/gitlab/use-gitlab-action-presets.tsapps/web/hooks/domains/gitlab/use-gitlab-issue-watches.tsapps/web/hooks/domains/gitlab/use-gitlab-review-watches.tsapps/web/hooks/domains/gitlab/use-gitlab-stats.tsapps/web/hooks/domains/gitlab/use-gitlab-status.tsapps/web/lib/api/domains/gitlab-api.tsapps/web/lib/state/slices/gitlab/gitlab-slice.test.tsapps/web/lib/state/slices/gitlab/gitlab-slice.tsapps/web/lib/state/slices/gitlab/types.tsapps/web/lib/state/store.tsapps/web/lib/types/gitlab.ts
|
Claude finished @jcfs's task in 5m 45s —— View job Code Review
FindingsBlocker (must fix before merge)1. 2. 3. Poll interval not normalized in update path — Suggestion (recommended, doesn't block)
Summary
Verdict: Blocked — fix blockers first. |
| if username == "" { | ||
| return nil, fmt.Errorf("no authenticated gitlab user") | ||
| } | ||
| filter := watch.CustomQuery |
There was a problem hiding this comment.
Suggestion: dead code in the custom-query branch of fetchReviewMRs
When watch.CustomQuery != "", filter is set to watch.CustomQuery here, then SearchMRs(ctx, filter, watch.CustomQuery) is called with both args being the same value. buildMRSearchQuery returns customQuery verbatim when non-empty, so the filter arg is silently discarded — the assignment on this line is dead code that implies it affects the query but doesn't.
Suggested change
| filter := watch.CustomQuery | |
| filter := "" | |
| if watch.CustomQuery == "" { | |
| filter = "reviewer_username=" + url.QueryEscape(username) | |
| } |
| // If the query already contains `labels=`, it is left alone (the user is | ||
| // explicitly controlling the filter and we don't want to silently double-up). | ||
| func appendLabelsToQuery(customQuery string, labels []string) string { | ||
| if strings.Contains(customQuery, "labels=") { |
There was a problem hiding this comment.
Suggestion: strings.Contains false-matches keys like mylabels=
strings.Contains(customQuery, "labels=") returns true for "mylabels=critical", causing label injection to be silently skipped when the user hasn't actually set the labels key. Use url.ParseQuery for an exact key check:
Suggested change
| if strings.Contains(customQuery, "labels=") { | |
| func appendLabelsToQuery(customQuery string, labels []string) string { | |
| if vals, err := url.ParseQuery(customQuery); err == nil { | |
| if _, ok := vals["labels"]; ok { | |
| return customQuery | |
| } | |
| } | |
| encoded := url.QueryEscape(strings.Join(labels, ",")) | |
| if customQuery == "" { | |
| return "labels=" + encoded | |
| } | |
| return customQuery + "&labels=" + encoded | |
| } |
|
|
||
| // Start kicks off the polling loops. Repeated Start calls are no-ops. | ||
| func (p *Poller) Start(ctx context.Context) { | ||
| if p.started { |
There was a problem hiding this comment.
Suggestion: p.started read/written without synchronization
Start reads p.started here and writes it on line 41; Stop reads (line 52) and writes (line 57) — no mutex guards any of these. go test -race would flag concurrent Start/Stop calls.
Suggested change
| if p.started { | |
| p.mu.Lock() | |
| defer p.mu.Unlock() | |
| if p.started { | |
| return | |
| } |
Add mu sync.Mutex to the Poller struct. In Stop, acquire and release the lock before calling cancel() so wg.Wait() is outside the lock.
| return fmt.Sprintf("gitlab:%s", projectPath) | ||
| } | ||
|
|
||
| var _ = gitlabRepoSlug // reserved for future structured-logging helpers |
There was a problem hiding this comment.
Suggestion: dead code — remove gitlabRepoSlug and blank identifier
gitlabRepoSlug has zero callers; var _ = gitlabRepoSlug exists only to suppress the compiler error. Per the project's no-speculative-code rule (CLAUDE.md): "No speculative code — unused flags/options, 'reserved for future' scaffolding". Delete both and re-add when there is an actual call site.
| last_polled_at DATETIME, | ||
| created_at DATETIME NOT NULL, | ||
| updated_at DATETIME NOT NULL | ||
| ); |
There was a problem hiding this comment.
Suggestion: missing indexes on gitlab_review_watches and gitlab_issue_watches
Both the 5-minute poller (ListAllReviewWatches / ListAllIssueWatches) and the HTTP list endpoints (?workspace_id=X) do full table scans. gitlab_mr_watches already has an index on line 84. Add matching pairs:
Suggested change
| ); | |
| ); | |
| CREATE INDEX IF NOT EXISTS idx_gitlab_review_watches_workspace_id ON gitlab_review_watches(workspace_id); | |
| CREATE INDEX IF NOT EXISTS idx_gitlab_review_watches_enabled ON gitlab_review_watches(enabled); | |
| CREATE TABLE IF NOT EXISTS gitlab_review_mr_tasks ( |
Add the same two lines after the gitlab_issue_watches table definition (~line 133).
| func (c *Controller) httpTriggerReviewWatch(ctx *gin.Context) { | ||
| id := ctx.Param("id") | ||
| mrs, err := c.service.TriggerReviewWatch(ctx.Request.Context(), id) | ||
| if err != nil { |
There was a problem hiding this comment.
Suggestion: 500 instead of 404 for a missing watch
TriggerReviewWatch returns fmt.Errorf("review watch not found: %s", id) when the watch doesn't exist (service_watches.go). That error maps here to http.StatusInternalServerError, making it impossible for clients to distinguish a missing watch from a real server fault.
Suggested change
| if err != nil { | |
| mrs, err := c.service.TriggerReviewWatch(ctx.Request.Context(), id) | |
| if err != nil { | |
| if errors.Is(err, errStoreUnavailable) || strings.Contains(err.Error(), "not found") { | |
| ctx.JSON(http.StatusNotFound, gin.H{responseErrorKey: err.Error()}) | |
| return | |
| } | |
| ctx.JSON(http.StatusInternalServerError, gin.H{responseErrorKey: err.Error()}) | |
| return | |
| } |
Same issue in httpTriggerIssueWatch below (~line 253). Long-term, a sentinel ErrWatchNotFound in watch_models.go + errors.Is would be cleaner than string matching.
| } | ||
| out := make([]Project, 0, len(projects)) | ||
| for _, p := range projects { | ||
| if p.PathWithNamespace == query || p.Path == query { |
There was a problem hiding this comment.
Suggestion: exact match diverges from documented "substring" behavior
The comment on line 432 says "filtered by query substring" but the implementation uses ==. Any autocomplete/search flow that sends a partial query string returns zero results in mock mode, creating false test negatives.
Suggested change
| if p.PathWithNamespace == query || p.Path == query { | |
| if strings.Contains(strings.ToLower(p.PathWithNamespace), strings.ToLower(query)) || | |
| strings.Contains(strings.ToLower(p.Path), strings.ToLower(query)) { | |
| out = append(out, p) | |
| } |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Claude finished @jcfs's task in 3m 51s —— View job Code Review
FindingsGood progress across all prior rounds. The latest commit ( One blocker remains. Blocker (must fix before merge)
Every sibling method in this file that was flagged in prior rounds ( See inline comment — one-line fix. Suggestions (non-blocking)
If the release call fails after a task-create failure, the dedup reservation row stays locked and the same MR/issue will never be retried. The error is currently discarded with
Verdict: Blocked — fix the |
| mr, err := client.FindMRByBranch(ctx, watch.ProjectPath, watch.Branch) | ||
| if err != nil || mr == nil { | ||
| now := time.Now().UTC() | ||
| _ = store.UpdateMRWatchTimestamps(ctx, watch.ID, now, watch.LastNoteAt, watch.LastPipelineState, watch.LastApprovalState) |
There was a problem hiding this comment.
Blocker: nil store dereference — panics on every MR watch poll when the store is unavailable
store := s.requireStore() at line 124 can return nil when NewStore failed at boot (non-fatal path in services.go). store is then used unconditionally at line 130 (store.UpdateMRWatchTimestamps) and line 133 (store.UpdateMRWatchMRIID), both of which panic on nil.
The mrMonitorLoop calls CheckMRWatch every minute for every active MR watch — this crash fires repeatedly on a timer.
Every sibling method in this file (GetReviewWatch, CheckReviewWatch, CheckIssueWatch, DeleteReviewWatch, DeleteIssueWatch) all have explicit nil guards added in prior commits. This one was missed.
Suggested change
| _ = store.UpdateMRWatchTimestamps(ctx, watch.ID, now, watch.LastNoteAt, watch.LastPipelineState, watch.LastApprovalState) | |
| store := s.requireStore() | |
| if store == nil { | |
| return nil, false, errStoreUnavailable | |
| } | |
| // If we don't yet know an iid, try to find it from the branch. |
|
Claude finished @jcfs's task in 2m 4s —— View job Code Review
FindingsGood progress since the last pass. All previous blocker-round items are confirmed resolved:
Blocker (must fix before merge)
Every sibling method ( Suggestion (non-blocking)
If the release call fails after task-create failure, the dedup reservation row stays locked and the same MR/issue can never be retried. The error is discarded with
Verdict: Blocked — one-line fix to |
| if client == nil { | ||
| return nil, false, ErrNoClient | ||
| } | ||
| store := s.requireStore() |
There was a problem hiding this comment.
Blocker: nil store dereference — crashes the MR poller every minute
s.requireStore() can return nil when NewStore failed at boot (non-fatal path in services.go). store is then used unconditionally at lines 130, 133, and 145 — all three panic on nil.
mrMonitorLoop calls CheckMRWatch every minute for every active MR watch, so this crash fires on a recurring timer. CheckReviewWatch and CheckIssueWatch both have the correct nil guard — this one was missed.
Suggested change
| store := s.requireStore() | |
| store := s.requireStore() | |
| if store == nil { | |
| return nil, false, errStoreUnavailable | |
| } |
| taskID, err := s.gitlabReviewTaskCreator.CreateGitLabReviewTask(ctx, evt) | ||
| if err != nil { | ||
| s.logger.Warn("create gitlab review task", zap.Error(err)) | ||
| _ = s.gitlabService.ReleaseReviewMRTask(ctx, evt.ReviewWatchID, mr.ProjectPath, mr.IID) |
There was a problem hiding this comment.
Suggestion: release error silently dropped — can permanently lock dedup row
If ReleaseReviewMRTask fails after a task-create failure, the reservation row stays locked and the same MR will never be retried until the row expires or is manually cleared.
Suggested change
| _ = s.gitlabService.ReleaseReviewMRTask(ctx, evt.ReviewWatchID, mr.ProjectPath, mr.IID) | |
| if relErr := s.gitlabService.ReleaseReviewMRTask(ctx, evt.ReviewWatchID, mr.ProjectPath, mr.IID); relErr != nil { | |
| s.logger.Warn("release gitlab review MR task reservation", zap.Error(relErr)) | |
| } |
Add the watcher/poll/write-action surface on top of the existing GitLab client + service skeleton: * 5 new SQLite tables (mr_watches, review_watches, review_mr_tasks, issue_watches, issue_watch_tasks, action_presets) + CRUD * Client + Service: MergeMR, GetProjectMergeMethods, GetProtectedBranch, ListUserProjects, SearchProjects, SetMRLabels, SetMRAssignees * Service: review/issue/MR watch CRUD + Check/Trigger, GetStats, CleanupAllReview/IssueTasks (cleanup-policy aware), TriggerMRSync, action-presets CRUD with defaults * Background poller (MR / review / issue loops) wired from main.go * ~35 GitLab WS action constants + handlers.go registrations * Mock controller for E2E seeding (MRs, issues, pipelines, discussions, approvals, branches) * eventBus + taskDeleter + taskSessionChecker plumbed through Service * Event types: GitLabMRFeedback / NewReviewMR / NewIssue / TaskMRUpdated
…ooks Extend the frontend GitLab integration on top of the new backend surface: * HTTP API client expansion (gitlab-api.ts): full CRUD for review/issue/MR watches, MR write actions (merge/approve/labels/assignees), MR files/commits/feedback, action presets, projects autocomplete, stats * Store slice expansion: review/issue/MR watches lists, action presets by workspace, stats + status with loading flags. Properties prefixed with gitlab to avoid collision with GitHub slice * Domain hooks: use-gitlab-status, use-gitlab-stats, use-gitlab-review-watches, use-gitlab-issue-watches, use-gitlab-action-presets — mirror github hook shapes * Type model: ReviewWatch, IssueWatch, MRWatch, ActionPresets, Stats, ProjectMergeMethods, MRApproval, Pipeline, MRFeedback, etc. * Backend controller HTTP routes for watches/presets/write-actions/ projects/stats so the frontend HTTP API client has endpoints to hit All passes: backend (build + vet + lint + tests), web (typecheck + lint + 2400 tests).
… add tests Address Claude review blockers on the parity PR: 1. Wire orchestrator subscriptions for GitLab review/issue watch events. Add event_handlers_gitlab.go with handleGitLabNewReviewMR / handleGitLabNewIssue, dedup reservation handshake, and task-creator interfaces (mirrors event_handlers_github.go). main.go now calls orchestratorSvc.SetGitLabService so the dedup APIs are usable 2. Fix silent label-filter drop: when a custom_query is set on an issue watch, labels were being appended to the unused filter arg. Fold labels into customQuery (or default filter) so they actually reach GitLab; extracted appendLabelsToQuery helper with explicit handling of pre-existing labels= clauses 3. Switch event payloads to pointer types (mirrors GitHub) so handlers can type-assert pointer events 4. Backend tests: store_watches_test.go (CRUD + reserve/assign for all 4 new tables + presets), service_watches_test.go (label-filter helper, cleanup policy, project normalisation), action_presets_test.go 5. Frontend slice tests: review/issue watch CRUD round-trips, action presets + stats reducer coverage All green: backend (build+vet+lint+tests), web (typecheck+lint+2403 tests).
…ts, nil guards Address the second round of Claude review blockers: 1. main.go: wire SetTaskDeleter + SetTaskSessionChecker on services.GitLab (mirrors GitHub). Without these, manual cleanup sweeps would always error with "task deleter not configured" 2. service_cleanup.go: default cleanup policy on transient DB error is now CleanupPolicyNever (preserve tasks) instead of CleanupPolicyAuto (silently delete). Genuine "watch was deleted" path also falls under the safe-side default; user can manually delete the orphan tasks 3. service_watches.go: add nil guards on requireStore() so the 6 MR-watch list/get/delete methods return an errStoreUnavailable error instead of panicking when the SQLite store failed to construct at boot 4. use-gitlab-status.ts + use-gitlab-stats.ts: add per-mount attemptedRef so an unreachable GitLab doesn't trigger an infinite re-fetch loop — the previous useEffect re-ran every render because the failure path left status null, satisfying the !status guard
…pdate Address remaining Claude blockers: * service_cleanup.go: HasUserAuthoredMessage transient errors are now preserved (return false → skip delete) instead of silently ignored; the alternative was occasionally deleting tasks a user had touched * service_watches.go: extract clampPollInterval helper applying the same bounds (0 → default, <60 → 60) as the create path; both applyReviewWatchPatch and applyIssueWatchPatch use it so user-supplied zero or tiny values via UpdateXxxWatchRequest no longer hammer GitLab
…atch cache * service_watches.go: applyReviewWatchPatch / applyIssueWatchPatch get nolint:dupl markers — they share shape but per-domain field validation lives in the create paths, so deduplicating via generics would obscure the contract. Restores backend lint to 0 issues * use-gitlab-action-presets.ts: per-workspace attemptedRef set so a failing preset fetch doesn't retry on every render * use-gitlab-review-watches.ts + use-gitlab-issue-watches.ts: track lastFetchedRef per consumer so a workspace switch triggers a refetch. The slice-level loaded flag is shared across instances and can't double as a per-workspace cache key
… paths Round-3 Claude finding: ReviewWatch / IssueWatch / preset / reservation methods still called s.requireStore().X() directly. If NewStore fails at boot (table migration error), the service struct's store is nil and every list/get/update/create/reserve method panics on first request. Apply the same store-nil → errStoreUnavailable pattern from f95a929 to every remaining method: * CreateReviewWatch / CreateIssueWatch * GetReviewWatch / ListReviewWatches / ListAllReviewWatches / UpdateReviewWatch / TriggerReviewWatchAll * GetIssueWatch / ListIssueWatches / ListAllIssueWatches / UpdateIssueWatch / TriggerIssueWatchAll * ReserveReviewMRTask / AssignReviewMRTaskID / ReleaseReviewMRTask * ReserveIssueWatchTask / AssignIssueWatchTaskID / ReleaseIssueWatchTask * lookupReviewPolicy / lookupIssuePolicy (return CleanupPolicyNever)
…anup paths Round-4 Claude finding: 4 remaining panic sites the previous nil-guard pass missed — these run from the background poller (every 5 min) and the manual delete/cleanup flows, so a boot-time NewStore failure would crash the orchestrator process on first tick. * service_watches.go: add store-nil guards to DeleteReviewWatch / DeleteIssueWatch / CheckReviewWatch / CheckIssueWatch * service_cleanup.go: guard CleanupAllReviewTasks / CleanupAllIssueTasks entry points; wrap the trailing DeleteReviewMRTask / DeleteIssueWatchTask calls in `if store := s.requireStore(); store != nil` blocks so the cleanup succeeds even when the dedup-row delete is unreachable
…ive limit CI's --new-from-rev lint caught service_watches.go at 808 lines (limit 800). Split into three focused files: * service_events.go (94 LOC) — publish helpers for MR feedback / new review MR / new issue / watch lifecycle events * service_issue_watches.go (302 LOC) — Issue watch CRUD + Check + Trigger + fetch + helpers * service_reservations.go (59 LOC) — Reserve/Assign/Release dedup handles used by the orchestrator event handlers service_watches.go is now 505 lines and contains MR watch + Review watch only. No behavioral change.
Address actionable inline comments from Claude/CodeRabbit/cubic/Greptile: * appendLabelsToQuery: use url.ParseQuery for exact key match instead of strings.Contains; previously false-matched keys like mylabels= and silently dropped the watch's labels (test added) * fetchReviewMRs: drop dead `filter = watch.CustomQuery` assignment — SearchMRs's buildMRSearchQuery returns customQuery verbatim and ignores filter when customQuery is non-empty * store.go: add workspace_id indexes on gitlab_review_watches and gitlab_issue_watches (the 5-min poller and HTTP list endpoints did full table scans) * mock_client.SearchProjects: switch from == to case-insensitive Contains so partial-query autocomplete returns the seeded project, matching the doc comment * Poller: add sync.Mutex to guard `started` field; Start/Stop are now safe to call concurrently (go test -race would have flagged the previous read/write race) * Add ErrWatchNotFound sentinel; controller_watches.go maps it to HTTP 404 via httpRespondError helper. Update/Trigger handlers previously returned 500 for missing watches, hiding the distinction from real server faults * service_events.go: log Publish errors on watch lifecycle events instead of swallowing them silently * handlers.go: reject malformed JSON payloads in list-style WS handlers rather than silently falling through to broader list/search behavior * event_handlers_gitlab.go: remove dead gitlabRepoSlug helper + blank-identifier suppression
…gs, body validation - CheckMRWatch, UpdateReviewWatch, UpdateIssueWatch: nil guards for store/req - handlers: wsNewDiscussionNote rejects empty body - action_presets: Update reads raw stored row (no default freezing); Reset validates workspace_id - service_search.GetStats: log Warn per sub-call failure before zero - event_handlers_gitlab: log Release* dedup-row errors with context - web/store.ts: post-slice merge restores every GitLab sub-state field; extract buildStateOverrides helper - web/default-state.ts: add GitLab fields to defaultState + mergeInitialState via mergeGitLabFields helper - web/gitlab-api.ts: UpdateXxxWatchRequest now Omit workspace_id Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jcfs
force-pushed
the
feature/full-gitlab-integrat-5h2
branch
from
7068cd7 to
49cd445
Compare
|
Claude finished @jcfs's task in 3m 16s —— View job Code Review
FindingsAll previously flagged blockers are confirmed resolved in commit
Suggestion (recommended, doesn't block)1. Initial-check goroutines use
2. Test assertions silently drop errors — // line 78 — GetReviewWatch error ignored after delete
got3, _ := store.GetReviewWatch(ctx, rw.ID)
// line 91 — second Reserve error ignored
ok2, _ := store.ReserveReviewMRTask(ctx, "watch-1", "group/project", 42, "https://x")The tests check the Summary
Verdict: Ready to merge. All blockers from prior rounds are resolved. The two suggestions are carry-overs (items 1 and 2 were flagged in earlier rounds as suggestions); they don't affect correctness or production safety. |
| if err := store.CreateReviewWatch(ctx, rw); err != nil { | ||
| return nil, fmt.Errorf("create review watch: %w", err) | ||
| } | ||
| go s.initialReviewCheck(context.Background(), rw) |
There was a problem hiding this comment.
Suggestion (carry-over from prior rounds): goroutine uses context.Background() — uncancellable on shutdown
initialReviewCheck is launched with context.Background() and is not tracked by any WaitGroup. After Poller.Stop() returns the goroutine may still be running and publishing events to a draining bus. The same pattern applies to initialIssueCheck in service_issue_watches.go:42.
Low production risk (the check is short-lived), but non-idiomatic and would trip goleak.VerifyTestMain if the package ever adds goroutine-leak testing (which internal/gitlab doesn't have yet — see AGENTS.md on which packages are instrumented).
Suggested change
| go s.initialReviewCheck(context.Background(), rw) | |
| go s.initialReviewCheck(ctx, rw) |
Passing the caller's ctx here (which is HTTP-request-scoped) bounds the goroutine to the request lifetime. A longer-lived service context (set at Service.Start time) would be even better for a recurring background goroutine, but either is better than Background().
| if err := store.DeleteReviewWatch(ctx, rw.ID); err != nil { | ||
| t.Fatalf("DeleteReviewWatch: %v", err) | ||
| } | ||
| got3, _ := store.GetReviewWatch(ctx, rw.ID) |
There was a problem hiding this comment.
Suggestion: error silently dropped — test can pass on unrelated DB failure
The blank identifier drops the error return. If GetReviewWatch fails for any reason other than "not found" (e.g., schema issue), the test still passes because got3 would be nil and the assertion if got3 != nil would succeed.
Suggested change
| got3, _ := store.GetReviewWatch(ctx, rw.ID) | |
| got3, err3 := store.GetReviewWatch(ctx, rw.ID) | |
| if err3 != nil { | |
| t.Fatalf("GetReviewWatch after delete: %v", err3) | |
| } |
Same pattern at line 71 (got2, _ := ...).
Comment on lines
+220
to
+242
| func (s *Service) fetchIssues(ctx context.Context, watch *IssueWatch) ([]*Issue, error) { | ||
| client := s.Client() | ||
| username, err := client.GetAuthenticatedUser(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("resolve gitlab username: %w", err) | ||
| } | ||
| // When a custom_query is set, labels need to be folded into it (the | ||
| // client's buildIssueSearchQuery returns customQuery verbatim and | ||
| // ignores the auxiliary `filter` arg). Otherwise build a default | ||
| // "assigned to me" filter and append labels there. | ||
| filter := "" | ||
| customQuery := watch.CustomQuery | ||
| switch { | ||
| case customQuery != "": | ||
| if len(watch.Labels) > 0 { | ||
| customQuery = appendLabelsToQuery(customQuery, watch.Labels) | ||
| } | ||
| default: | ||
| filter = "assignee_username=" + url.QueryEscape(username) | ||
| if len(watch.Labels) > 0 { | ||
| filter += "&labels=" + url.QueryEscape(strings.Join(watch.Labels, ",")) | ||
| } | ||
| } |
There was a problem hiding this comment.
GetAuthenticatedUser inside the branch that actually needs it. When customQuery != "", username is never referenced, so calling the user-info endpoint unconditionally wastes a round-trip and makes custom-query watches fail if that endpoint is unavailable or the PAT lacks read_user scope.
Suggested change
| func (s *Service) fetchIssues(ctx context.Context, watch *IssueWatch) ([]*Issue, error) { | |
| client := s.Client() | |
| username, err := client.GetAuthenticatedUser(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("resolve gitlab username: %w", err) | |
| } | |
| // When a custom_query is set, labels need to be folded into it (the | |
| // client's buildIssueSearchQuery returns customQuery verbatim and | |
| // ignores the auxiliary `filter` arg). Otherwise build a default | |
| // "assigned to me" filter and append labels there. | |
| filter := "" | |
| customQuery := watch.CustomQuery | |
| switch { | |
| case customQuery != "": | |
| if len(watch.Labels) > 0 { | |
| customQuery = appendLabelsToQuery(customQuery, watch.Labels) | |
| } | |
| default: | |
| filter = "assignee_username=" + url.QueryEscape(username) | |
| if len(watch.Labels) > 0 { | |
| filter += "&labels=" + url.QueryEscape(strings.Join(watch.Labels, ",")) | |
| } | |
| } | |
| func (s *Service) fetchIssues(ctx context.Context, watch *IssueWatch) ([]*Issue, error) { | |
| client := s.Client() | |
| // When a custom_query is set, labels need to be folded into it (the | |
| // client's buildIssueSearchQuery returns customQuery verbatim and | |
| // ignores the auxiliary `filter` arg). Otherwise build a default | |
| // "assigned to me" filter and append labels there. | |
| filter := "" | |
| customQuery := watch.CustomQuery | |
| switch { | |
| case customQuery != "": | |
| if len(watch.Labels) > 0 { | |
| customQuery = appendLabelsToQuery(customQuery, watch.Labels) | |
| } | |
| default: | |
| username, err := client.GetAuthenticatedUser(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("resolve gitlab username: %w", err) | |
| } | |
| filter = "assignee_username=" + url.QueryEscape(username) | |
| if len(watch.Labels) > 0 { | |
| filter += "&labels=" + url.QueryEscape(strings.Join(watch.Labels, ",")) | |
| } | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bring
internal/gitlabto full parity withinternal/github: PR/MR + issue watchers with background polling, write actions (merge, approve, labels/assignees, discussion notes), CI/pipeline status, action presets, and full HTTP + WS surfaces. Frontend gets matching types, API client, store slice, and core domain hooks.Backend (
apps/backend)gitlab_mr_watches,gitlab_review_watches,gitlab_review_mr_tasks,gitlab_issue_watches,gitlab_issue_watch_tasks,gitlab_action_presetsMergeMR,GetProjectMergeMethods,GetProtectedBranch,ListUserProjects,SearchProjects,SetMRLabels,SetMRAssigneesGetStats,CleanupAllReview/IssueTasks(cleanup-policy aware),TriggerMRSync, action presets CRUD with built-in defaultsmain.goGitLabMRFeedback,GitLabNewReviewMR,GitLabNewIssue,GitLabTaskMRUpdatedFrontend (
apps/web)ReviewWatch,IssueWatch,MRWatch,ActionPresets,Stats,ProjectMergeMethods,MRApproval,Pipeline,MRFeedback, etc.gitlab-api.ts): full HTTP wrappers for every new backend routegitlabReviewWatches,gitlabIssueWatches,gitlabMRWatches,gitlabActionPresets,gitlabStats,gitlabStatus(prefixed to avoid GitHub slice collision)useGitLabStatus,useGitLabStats,useGitLabReviewWatches,useGitLabIssueWatches,useGitLabActionPresets— mirror the GitHub hook shapesDecisions
workspace.repositoryfield for provider detection by hostVerification
go build ./...,go vet ./...,go test ./...,make lint— all greenpnpm typecheck,pnpm lint,pnpm test(2400 passing) — all greenTest plan
assigned_to_meMRs → trigger → confirm dedup row appears + event firesOut of scope / follow-ups
/gitlabpage shell andmr-list/issue-listcomponents exist as scaffolding — the new hooks are ready to plug into them in a follow-up)GITLAB_TOKENinjection into the agent runtime (/prskill detection)🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Preview Environment
49cd445