Skip to content

Commit b9e83dd

Browse files
VJ-yadavclaude
VJ-yadav
and
claude
committed
fix: address CodeRabbit review findings on GitLab MR review
- Replace process.exit(1) with thrown errors so top-level finally block runs Telemetry.shutdown() on failure (matches github.ts pattern) - Deduplicate review comments: detect existing marker note and update via PUT instead of always creating a new one - Remove duplicate output printing (subscribeSessionEvents already renders the completed assistant message) - Fix GITLAB_INSTANCE_URL env var precedence: env now overrides the URL parsed from the MR link, enabling self-hosted proxy/mirror use - Remove unused postMRDiscussion/DiscussionPosition dead code; add updateMRNote helper used by the dedup logic Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
1 parent bdabf46 commit b9e83dd

1 file changed

Lines changed: 33 additions & 52 deletions

File tree

‎packages/opencode/src/cli/cmd/gitlab.ts‎

Lines changed: 33 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -181,30 +181,19 @@ async function postMRNote(
181181
)
182182
}
183183

184-
interface DiscussionPosition {
185-
position_type: "text"
186-
base_sha: string
187-
head_sha: string
188-
start_sha: string
189-
new_path: string
190-
old_path: string
191-
new_line: number | null
192-
old_line: number | null
193-
}
194-
195-
async function postMRDiscussion(
184+
async function updateMRNote(
196185
instanceUrl: string,
197186
projectId: string,
198187
mrIid: number,
199188
token: string,
189+
noteId: number,
200190
body: string,
201-
position: DiscussionPosition,
202-
): Promise<unknown> {
203-
return gitlabApi(
191+
): Promise<GitLabNote> {
192+
return gitlabApi<GitLabNote>(
204193
instanceUrl,
205-
`/projects/${projectId}/merge_requests/${mrIid}/discussions`,
194+
`/projects/${projectId}/merge_requests/${mrIid}/notes/${noteId}`,
206195
token,
207-
{ method: "POST", body: { body, position } },
196+
{ method: "PUT", body: { body } },
208197
)
209198
}
210199

@@ -273,10 +262,6 @@ function resolveToken(): string {
273262
return token
274263
}
275264

276-
function resolveInstanceUrl(): string {
277-
return (process.env["GITLAB_INSTANCE_URL"] || "https://gitlab.com").replace(/\/+$/, "")
278-
}
279-
280265
// ---------------------------------------------------------------------------
281266
// CLI command
282267
// ---------------------------------------------------------------------------
@@ -315,16 +300,17 @@ export const GitlabReviewCommand = cmd({
315300
// Parse MR URL
316301
const parsed = parseGitLabMRUrl(mrUrl)
317302
if (!parsed) {
318-
UI.error(`Invalid GitLab MR URL: ${mrUrl}`)
319-
UI.error("Expected format: https://gitlab.com/org/repo/-/merge_requests/123")
320-
process.exit(1)
303+
throw new Error(
304+
`Invalid GitLab MR URL: ${mrUrl}\nExpected format: https://gitlab.com/org/repo/-/merge_requests/123`,
305+
)
321306
}
322307

323308
// Resolve auth and instance
324309
const token = resolveToken()
325-
const envInstanceUrl = resolveInstanceUrl()
326-
// Prefer instance URL from the MR URL itself
327-
const instanceUrl = parsed.instanceUrl || envInstanceUrl
310+
// GITLAB_INSTANCE_URL env var overrides the URL parsed from the MR link,
311+
// allowing self-hosted proxies or internal mirrors to reroute API calls.
312+
const envInstanceUrl = process.env["GITLAB_INSTANCE_URL"]?.replace(/\/+$/, "")
313+
const instanceUrl = envInstanceUrl || parsed.instanceUrl
328314
const projectId = encodeURIComponent(parsed.projectPath)
329315
const mrIid = parsed.mrIid
330316

@@ -335,19 +321,17 @@ export const GitlabReviewCommand = cmd({
335321
args.model || process.env["MODEL"] || process.env["ALTIMATE_MODEL"] || "anthropic/claude-sonnet-4-20250514"
336322
const { providerID, modelID } = Provider.parseModel(modelStr)
337323
if (!providerID.length || !modelID.length) {
338-
UI.error(`Invalid model format: ${modelStr}. Use "provider/model" (e.g. anthropic/claude-sonnet-4-20250514).`)
339-
process.exit(1)
324+
throw new Error(
325+
`Invalid model format: ${modelStr}. Use "provider/model" (e.g. anthropic/claude-sonnet-4-20250514).`,
326+
)
340327
}
341328

342329
// Fetch MR data
343330
UI.println("Fetching MR metadata and changes...")
344331
const [mrData, notes] = await Promise.all([
345332
fetchMRChanges(instanceUrl, projectId, mrIid, token),
346333
fetchMRNotes(instanceUrl, projectId, mrIid, token),
347-
]).catch((err: unknown) => {
348-
UI.error(err instanceof Error ? err.message : String(err))
349-
return process.exit(1) as never
350-
})
334+
])
351335

352336
UI.println(` Title: ${mrData.title}`)
353337
UI.println(` Author: ${mrData.author.username}`)
@@ -373,27 +357,24 @@ export const GitlabReviewCommand = cmd({
373357
// Subscribe to session events for live output
374358
subscribeSessionEvents(session)
375359

376-
const reviewText = await runReview(session.id, variant, providerID, modelID, reviewPrompt).catch(
377-
(err: unknown) => {
378-
UI.error(err instanceof Error ? err.message : String(err))
379-
return process.exit(1) as never
380-
},
381-
)
382-
383-
// Output review
384-
UI.empty()
385-
UI.println(UI.markdown(reviewText))
386-
UI.empty()
360+
// subscribeSessionEvents() already renders the completed assistant message,
361+
// so we only need the text for posting — no duplicate printing here.
362+
const reviewText = await runReview(session.id, variant, providerID, modelID, reviewPrompt)
387363

388-
// Post to GitLab
364+
// Post to GitLab (deduplicate: update existing review note if present)
389365
if (shouldPost) {
390-
UI.println("Posting review to GitLab MR...")
391366
const commentBody = `<!-- altimate-code-review -->\n${reviewText}`
392-
const note = await postMRNote(instanceUrl, projectId, mrIid, token, commentBody).catch((err: unknown) => {
393-
UI.error(`Failed to post review: ${err instanceof Error ? err.message : String(err)}`)
394-
return process.exit(1) as never
395-
})
396-
UI.println(`Review posted as note #${note.id}: ${mrData.web_url}#note_${note.id}`)
367+
const existingReview = notes.find((n) => n.body.startsWith("<!-- altimate-code-review -->"))
368+
369+
if (existingReview) {
370+
UI.println("Updating existing review note on GitLab MR...")
371+
const note = await updateMRNote(instanceUrl, projectId, mrIid, token, existingReview.id, commentBody)
372+
UI.println(`Review updated (note #${note.id}): ${mrData.web_url}#note_${note.id}`)
373+
} else {
374+
UI.println("Posting review to GitLab MR...")
375+
const note = await postMRNote(instanceUrl, projectId, mrIid, token, commentBody)
376+
UI.println(`Review posted as note #${note.id}: ${mrData.web_url}#note_${note.id}`)
377+
}
397378
}
398379

399380
UI.println("Done.")
@@ -499,4 +480,4 @@ function subscribeSessionEvents(session: { id: SessionID; title: string; version
499480
}
500481

501482
// Re-export API helpers for potential use in CI/CD integrations
502-
export { fetchMRMetadata, fetchMRChanges, fetchMRNotes, postMRNote, postMRDiscussion }
483+
export { fetchMRMetadata, fetchMRChanges, fetchMRNotes, postMRNote, updateMRNote }

0 commit comments

Comments
 (0)