fix(web): fix GitLab MR inline comment 400 errors on context lines and renames - #1149
Merged
brendan-kellam merged 11 commits intoJun 8, 2026
Merged
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
…d renames Two bugs caused `MergeRequestDiscussions.create` to return 400 Bad Request for a subset of inline comments. --- ### Missing `oldPath` on modified files GitLab's Discussions API requires both `oldPath` and `newPath` in the position object for any file that exists in both the base and head commits. Previously only `newPath` was set, causing GitLab to reject comments on modified files. `oldPath` is now always included, falling back to `filename` when no rename is present. ### Missing `oldLine` for context (unchanged) lines GitLab requires `oldLine` in addition to `newLine` when the target line is a context line (unchanged in the diff). Without it the API returns 400 for those lines while accepting added-line comments — explaining why only *some* comments in a given MR failed. The fix parses `oldSnippet`/`newSnippet` from the diff payload to build a new→old line number map. Context lines appear at the same index in both snippets, so zipping them gives the mapping. `oldLine` is only included when the line is confirmed to be a context line; added lines continue to use `newLine` only. ### Rename support `oldFilename` is now propagated through `generatePrReview.ts` → `sourcebot_file_diff_review` → `gitlabPushMrReviews`, so renamed files correctly pass the old path as `oldPath`. --- ### Files changed | File | Change | |------|--------| | `types.ts` | Added `oldFilename?: string` to `sourcebot_file_diff_review_schema` | | `generatePrReview.ts` | Sets `oldFilename: file_diff.from` when building review objects | | `gitlabPushMrReviews.ts` | Adds `oldPath`, context-line `oldLine` mapping via `buildContextLineMap` | | `gitlabPushMrReviews.test.ts` | Adds `oldPath` to position assertion; new tests for renames and context vs added line behaviour |
Copy link
Copy Markdown
Contributor
|
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 reviews.auto_review.auto_pause_after_reviewed_commits setting. Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRecords original filenames in generated reviews and enriches GitLab MR inline comment positions by adding oldPath and mapping new-file context lines to corresponding old-file oldLine values when available. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent (review generator)
participant Parser as Diff Parser
participant API as GitLab API
Agent->>Parser: Provide PR diffs (files, hunks)
Parser-->>Agent: newLine -> oldLine mappings per file
Agent->>API: Post inline discussion (newPath, oldPath, newLine, oldLine?)
API-->>Agent: Success / 400 error (fallback to general MR note)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts (1)🤖 Prompt for all review comments with AI agents189-275: LGTM — solid coverage of the new behavior.
The three new tests cover the key paths introduced by the fix:
- Renamed file → oldPath from oldFilename, newPath from filename.
- Context line → oldLine present and correctly mapped via the snippet.
- Added line → oldLine omitted (explicitly asserted with not.toHaveProperty).
Nit: the test at line 214 is titled "passes oldLine for context lines and omits it for added lines" but the "omits for added lines" assertion actually lives in the separate test at line 250. Consider renaming to something like 'passes oldLine for context lines' for clarity.
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts` around lines 189 - 275, Rename the test whose description string is "passes oldLine for context lines and omits it for added lines" to a clearer name like "passes oldLine for context lines" in the gitlabPushMrReviews.test file so the title reflects its actual assertions (it only verifies context-line behavior); locate the test by that exact description string and update it accordingly.
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts`:
- Around line 14-22: The for-loop parsing diff lines uses several single-line if
statements without braces (inside the function handling variable snippet, using
colonIdx, lineNum, content and result.push), so update each single-line if
(lines checking startsWith('@@'), colonIdx === -1, isNaN(lineNum), and
content.startsWith(' ')) to use block form with curly braces and move their
single statement bodies onto the next line; ensure each conditional wraps its
corresponding continue or result.push(lineNum) in braces to comply with the
coding guideline.
- Line 78: The MR discussion position is sometimes sent with literal "/dev/null"
file paths (e.g., when using fileDiffReview.oldFilename or
fileDiffReview.filename in gitlabPushMrReviews.ts), which GitLab rejects; update
the code that builds sourcebot_file_diff_review objects (in generatePrReview.ts)
or add a guard in gitlabPushMrReviews.ts before creating the position so that
any path equal to "/dev/null" is treated as missing (filter to undefined/null or
omit the oldPath/newPath keys) — mirror the same "/dev/null" filtering logic
used in getDiffApi.ts (lines ~70-71) so positions sent to the MR discussions API
never contain the literal "/dev/null".
---
Nitpick comments:
In
`@packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts`:
- Around line 189-275: Rename the test whose description string is "passes
oldLine for context lines and omits it for added lines" to a clearer name like
"passes oldLine for context lines" in the gitlabPushMrReviews.test file so the
title reflects its actual assertions (it only verifies context-line behavior);
locate the test by that exact description string and update it accordingly.
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: 404e348e-5c58-40f3-92d7-1812490e8ca2
📥 CommitsReviewing files that changed from the base of the PR and between f49932d and 2afa854.
📒 Files selected for processing (4)- packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts
- packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts
- packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts
- packages/web/src/features/agents/review-agent/types.ts
Sorry, something went wrong.
Spread oldPath/newPath as optional object entries instead of passing string | undefined, which is not assignable to the gitbeaker DiscussionNotePositionOptions type. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Spreading union types (`{} | { oldPath: string }`) into an object literal
causes TypeScript to widen the result to a union that doesn't satisfy the
gitbeaker discriminated union type. Also, passing `string | undefined` for
optional position properties fails due to the `Record<string, unknown>` base
type camelizing to `Record<string, {}>`, which excludes `undefined`.
Fix by building the position as a `Record<string, string>` imperatively
(only ever string values, properties conditionally added) and casting to the
position type derived from the `Gitlab` client via `Parameters<>` — no
`as any` and no direct import from `@gitbeaker/core`.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ℹ️ Review info ⚙️ Run configuration
packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts (1)🤖 Prompt for all review comments with AI agents78-96: Mirror paths for added/deleted files to preserve inline review positioning.
GitLab's discussions API requires both old_path and new_path when using position_type: 'text'. The current logic omits these paths when they equal '/dev/null', causing the API call to fail and fall back to posting reviews as general MR notes instead of inline comments.
For proper inline positioning:
- New file: set both old_path and new_path to the new path
- Deleted file: set both old_path and new_path to the old path
Consider normalizing paths before the conditional checks instead of omitting them:
♻️ Proposed handling for new/deleted files🤖 Prompt for AI Agents- const resolvedOldPath = fileDiffReview.oldFilename ?? fileDiffReview.filename; + const rawOldPath = fileDiffReview.oldFilename ?? fileDiffReview.filename; + const newPathIsReal = fileDiffReview.filename !== '/dev/null'; + const oldPathIsReal = rawOldPath !== '/dev/null'; + // Added file: mirror newPath into oldPath. Deleted file: mirror oldPath into newPath. + const resolvedOldPath = oldPathIsReal ? rawOldPath : fileDiffReview.filename; + const resolvedNewPath = newPathIsReal ? fileDiffReview.filename : rawOldPath; for (const review of fileDiffReview.reviews) { const oldLine = fileContextMap?.get(review.line_end); const position: Record<string, string> = { positionType: 'text', baseSha: base_sha, headSha: head_sha, startSha: start_sha, newLine: String(review.line_end), }; - if (resolvedOldPath !== '/dev/null') { - position['oldPath'] = resolvedOldPath; - } - if (fileDiffReview.filename !== '/dev/null') { - position['newPath'] = fileDiffReview.filename; - } + if (resolvedOldPath !== '/dev/null') { + position['oldPath'] = resolvedOldPath; + } + if (resolvedNewPath !== '/dev/null') { + position['newPath'] = resolvedNewPath; + }Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts` around lines 78 - 96, The position object building in gitlabPushMrReviews.ts omits oldPath/newPath when paths are '/dev/null', which breaks GitLab inline positioning; update the logic around resolvedOldPath, fileDiffReview.filename and the position map so that for added files (old is '/dev/null') you set both oldPath and newPath to the new path, and for deleted files (new is '/dev/null') you set both oldPath and newPath to the old path, and otherwise set oldPath to resolvedOldPath and newPath to fileDiffReview.filename; keep the existing setting of baseSha/headSha/startSha/newLine/oldLine and ensure the position keys use the exact names expected by the API (oldPath/newPath).
Verify each finding against the current code and only fix it if needed. Nitpick comments: In `@packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts`: - Around line 78-96: The position object building in gitlabPushMrReviews.ts omits oldPath/newPath when paths are '/dev/null', which breaks GitLab inline positioning; update the logic around resolvedOldPath, fileDiffReview.filename and the position map so that for added files (old is '/dev/null') you set both oldPath and newPath to the new path, and for deleted files (new is '/dev/null') you set both oldPath and newPath to the old path, and otherwise set oldPath to resolvedOldPath and newPath to fileDiffReview.filename; keep the existing setting of baseSha/headSha/startSha/newLine/oldLine and ensure the position keys use the exact names expected by the API (oldPath/newPath).
ℹ️ Review info ⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f716fb8-66ab-4a9b-baab-2bac77b59046
📥 CommitsReviewing files that changed from the base of the PR and between 3da3f91 and 0957570.
📒 Files selected for processing (1)- packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts
Sorry, something went wrong.
GitLab requires both oldPath and newPath in the position object. Previously they were omitted when the path was '/dev/null', breaking inline positioning for added and deleted files. Now for added files (old path is /dev/null) both are set to the new path, and for deleted files (new path is /dev/null) both are set to the old path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ℹ️ Review info ⚙️ Run configuration
packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts (1)🤖 Prompt for all review comments with AI agents86-103: Direct type the position object to eliminate the redundant double cast and improve type safety.
The DiscussionNotePosition type is correctly derived from the gitbeaker SDK's parameter types. Typing position directly as DiscussionNotePosition instead of Record<string, string> with a subsequent as unknown as DiscussionNotePosition cast restores the type safety benefit of having this derived type. Since oldLine is optional in the position schema, use a spread operator for conditional inclusion rather than post-assignment.
♻️ Suggested refactor🤖 Prompt for AI Agentsconst oldLine = fileContextMap?.get(review.line_end); - const position: Record<string, string> = { + const position: DiscussionNotePosition = { positionType: 'text', baseSha: base_sha, headSha: head_sha, startSha: start_sha, oldPath, newPath, newLine: String(review.line_end), + ...(oldLine !== undefined ? { oldLine: String(oldLine) } : {}), }; - if (oldLine !== undefined) { - position['oldLine'] = String(oldLine); - } try { await gitlabClient.MergeRequestDiscussions.create( projectId, prPayload.number, review.review, - { position: position as unknown as DiscussionNotePosition }, + { position }, );Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts` around lines 86 - 103, The position object is currently declared as Record<string,string> and then force-cast to DiscussionNotePosition; instead declare position with the derived DiscussionNotePosition type and build it so optional fields are conditionally included (use a spread for oldLine) to preserve type safety and remove the redundant "as unknown as DiscussionNotePosition" cast; update the code that calls MergeRequestDiscussions.create to pass the strongly typed position variable directly and remove the double cast.
Verify each finding against the current code and only fix it if needed. Nitpick comments: In `@packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts`: - Around line 86-103: The position object is currently declared as Record<string,string> and then force-cast to DiscussionNotePosition; instead declare position with the derived DiscussionNotePosition type and build it so optional fields are conditionally included (use a spread for oldLine) to preserve type safety and remove the redundant "as unknown as DiscussionNotePosition" cast; update the code that calls MergeRequestDiscussions.create to pass the strongly typed position variable directly and remove the double cast.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63c4ae99-0ff7-40f9-a9aa-7f4aa9837547
📥 CommitsReviewing files that changed from the base of the PR and between 0957570 and a57f68f.
📒 Files selected for processing (2)- packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts
- packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts
- packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts
Sorry, something went wrong.
Now that oldPath/newPath are always plain strings, declare position directly as DiscussionNotePosition and use a single conditional spread for the optional oldLine field. Removes the redundant `as unknown as DiscussionNotePosition` cast. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor
Author
|
@brendan-kellam @msukkari Hopefully this one is a quick win to merge :) |
Sorry, something went wrong.
brendan-kellam
approved these changes
Jun 8, 2026
brendan-kellam
left a comment
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
lgtm thanks!
Sorry, something went wrong.
brendan-kellam
approved these changes
Jun 8, 2026
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
Reviewers
2 participants
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.
Two bugs caused MergeRequestDiscussions.create to return 400 Bad Request for a subset of inline comments.
Missing oldPath on modified files
GitLab's Discussions API requires both oldPath and newPath in the position object for any file that exists in both the base and head commits. Previously only newPath was set, causing GitLab to reject comments on modified files. oldPath is now always included, falling back to filename when no rename is present.
Missing oldLine for context (unchanged) lines
GitLab requires oldLine in addition to newLine when the target line is a context line (unchanged in the diff). Without it the API returns 400 for those lines while accepting added-line comments — explaining why only some comments in a given MR failed.
The fix parses oldSnippet/newSnippet from the diff payload to build a new→old line number map. Context lines appear at the same index in both snippets, so zipping them gives the mapping. oldLine is only included when the line is confirmed to be a context line; added lines continue to use newLine only.
Rename support
oldFilename is now propagated through generatePrReview.ts → sourcebot_file_diff_review → gitlabPushMrReviews, so renamed files correctly pass the old path as oldPath.
Files changed
Related: #1104
Summary by CodeRabbit
New Features
Bug Fixes