fix(web): fix GitLab MR inline comment 400 errors on context lines and renames#1149
Open
fatmcgav wants to merge 7 commits intosourcebot-dev:mainfrom
Open
fix(web): fix GitLab MR inline comment 400 errors on context lines and renames#1149fatmcgav wants to merge 7 commits intosourcebot-dev:mainfrom
fatmcgav wants to merge 7 commits intosourcebot-dev:mainfrom
Conversation
…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 |
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 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 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
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
189-275: LGTM — solid coverage of the new behavior.The three new tests cover the key paths introduced by the fix:
- Renamed file →
oldPathfromoldFilename,newPathfromfilename.- Context line →
oldLinepresent and correctly mapped via the snippet.- Added line →
oldLineomitted (explicitly asserted withnot.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 Agents
Verify 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.
🤖 Prompt for all review comments with AI agents
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.
🪄 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: 404e348e-5c58-40f3-92d7-1812490e8ca2
📒 Files selected for processing (4)
packages/web/src/features/agents/review-agent/nodes/generatePrReview.tspackages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.tspackages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.tspackages/web/src/features/agents/review-agent/types.ts
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>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts (1)
78-96: Mirror paths for added/deleted files to preserve inline review positioning.GitLab's discussions API requires both
old_pathandnew_pathwhen usingposition_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_pathandnew_pathto the new path- Deleted file: set both
old_pathandnew_pathto the old pathConsider normalizing paths before the conditional checks instead of omitting them:
♻️ Proposed handling for new/deleted files
- 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; + }🤖 Prompt for AI Agents
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).
🤖 Prompt for all review comments with AI agents
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
📒 Files selected for processing (1)
packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts
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>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts (1)
86-103: Direct type the position object to eliminate the redundant double cast and improve type safety.The
DiscussionNotePositiontype is correctly derived from the gitbeaker SDK's parameter types. Typingpositiondirectly asDiscussionNotePositioninstead ofRecord<string, string>with a subsequentas unknown as DiscussionNotePositioncast restores the type safety benefit of having this derived type. SinceoldLineis optional in the position schema, use a spread operator for conditional inclusion rather than post-assignment.♻️ Suggested refactor
const 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 }, );🤖 Prompt for AI Agents
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.
🤖 Prompt for all review comments with AI agents
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
📒 Files selected for processing (2)
packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.tspackages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts
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>
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.
Two bugs caused
MergeRequestDiscussions.createto return 400 Bad Request for a subset of inline comments.Missing
oldPathon modified filesGitLab's Discussions API requires both
oldPathandnewPathin the position object for any file that exists in both the base and head commits. Previously onlynewPathwas set, causing GitLab to reject comments on modified files.oldPathis now always included, falling back tofilenamewhen no rename is present.Missing
oldLinefor context (unchanged) linesGitLab requires
oldLinein addition tonewLinewhen 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/newSnippetfrom 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.oldLineis only included when the line is confirmed to be a context line; added lines continue to usenewLineonly.Rename support
oldFilenameis now propagated throughgeneratePrReview.ts→sourcebot_file_diff_review→gitlabPushMrReviews, so renamed files correctly pass the old path asoldPath.Files changed
types.tsoldFilename?: stringtosourcebot_file_diff_review_schemageneratePrReview.tsoldFilename: file_diff.fromwhen building review objectsgitlabPushMrReviews.tsoldPath, context-lineoldLinemapping viabuildContextLineMapgitlabPushMrReviews.test.tsoldPathto position assertion; new tests for renames and context vs added line behaviourRelated: #1104
Summary by CodeRabbit
New Features
Bug Fixes