Repository navigation
test(demo): off-by-one in parseAddedLines, breaks 2 tests - #1
DorianZheng wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughA one-line fix in ChangesAdded-line calculation fix
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/publish.mjs`:
- Line 21: Remove the off-by-one offset in the hunk parsing logic: `publish.mjs`
currently initializes `newLine` from `hunk[1]` and then adds 1, which shifts
every added line number incorrectly. Update the line-number tracking in the
patch parsing flow so added lines use the hunk header’s right-side start
directly, and verify the downstream `partition()` mapping still matches findings
against `lines.has(f.line)` in `publish.test.mjs` expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const hunk = row.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/) | ||
| if (hunk) { | ||
| newLine = Number.parseInt(hunk[1], 10) | ||
| newLine = Number.parseInt(hunk[1], 10) + 1 // BUG: shifts every added line by one |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The + 1 introduces an off-by-one error that breaks added-line numbering.
The hunk header's newStart (captured in hunk[1]) is already the correct RIGHT-side starting line number. Adding + 1 shifts every subsequent added line up by one. Tracing the test patch @@ -1,3 +1,4 @@:
- With
+ 1:newLinestarts at 2, context advances to 3, so+added-a→3,+added-b→4 → result{3,4}≠ expected{2,3}. - Without
+ 1:newLinestarts at 1, context advances to 2, so+added-a→2,+added-b→3 → result{2,3}✓
This breaks the unit test in test/publish.test.mjs and causes partition() (line 62) to anchor inline comments to wrong lines, since lines.has(f.line) will mismatch findings against shifted line numbers.
🐛 Proposed fix: remove the off-by-one offset
- newLine = Number.parseInt(hunk[1], 10) + 1 // BUG: shifts every added line by one
+ newLine = Number.parseInt(hunk[1], 10)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| newLine = Number.parseInt(hunk[1], 10) + 1 // BUG: shifts every added line by one | |
| newLine = Number.parseInt(hunk[1], 10) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/publish.mjs` at line 21, Remove the off-by-one offset in the hunk parsing
logic: `publish.mjs` currently initializes `newLine` from `hunk[1]` and then
adds 1, which shifts every added line number incorrectly. Update the line-number
tracking in the patch parsing flow so added lines use the hunk header’s
right-side start directly, and verify the downstream `partition()` mapping still
matches findings against `lines.has(f.line)` in `publish.test.mjs` expectations.
DorianZheng
left a comment
DorianZheng
left a comment
There was a problem hiding this comment.
📦 BoxLite review — 1 issue
| const hunk = row.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/) | ||
| if (hunk) { | ||
| newLine = Number.parseInt(hunk[1], 10) | ||
| newLine = Number.parseInt(hunk[1], 10) + 1 // BUG: shifts every added line by one |
There was a problem hiding this comment.
🛑 Off-by-one shifts every added line number
parseAddedLines(PATCH) returns {3,4} instead of {2,3}; ran node --test test/publish.test.mjs → 2/8 fail, causing inline comments to map to wrong lines or hit GitHub's 422 diff-line rejection.
| newLine = Number.parseInt(hunk[1], 10) + 1 // BUG: shifts every added line by one | |
| newLine = Number.parseInt(hunk[1], 10) |
📦 BoxLite review — 1 issue ·
|
Demo: reviewer runs the test suite in a BoxLite microVM.
Summary by CodeRabbit