bug:ENG-20249 fix counter index in getLineInformation - #47
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:
📝 WalkthroughWalkthroughThe change corrects modification-line indexing during lookahead processing. It adds automated package publication, centralizes the Node.js version, updates package metadata, and documents the release process. ChangesLine Index Correction
Release Automation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Release as Published release
participant Workflow as Publish workflow
participant Package as GitHub Packages
Release->>Workflow: Trigger publication
Workflow->>Workflow: Install dependencies, lint, build, and test
Workflow->>Package: Publish with next or latest tag
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 3
🧹 Nitpick comments (1)
README.md (1)
378-382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a version placeholder in the release instructions.
The release procedure is reusable, but Line 381 hardcodes
v4.0.7. After the next version bump, this text can direct maintainers to create the wrong tag. Usev<version>and<version>instead.Proposed change
- targeting `main` with a new tag matching the version you just merged — `v4.0.7` or `4.0.7` both + targeting `main` with a new tag matching the version you just merged — `v<version>` or `<version>` both🤖 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 `@README.md` around lines 378 - 382, Update the release instructions around the GitHub release tag examples to replace the hardcoded 4.0.7 values with version placeholders, using v<version> or <version> while preserving the existing guidance.
🤖 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 @.github/workflows/publish.yml:
- Around line 50-54: Remove the cache and cache-dependency-path settings from
the actions/setup-node step in the publication job, while retaining
node-version-file and leaving caching enabled only in build-test.yml.
- Around line 25-28: Set persist-credentials: false on all checkout steps in
.github/workflows/publish.yml (lines 25-28) and both checkout steps in
.github/workflows/build-test.yml (lines 16 and 35). Update the publish
workflow’s git fetch --no-tags origin main authentication to use a scoped step
or API request after checkout credentials are disabled.
In @.nvmrc:
- Line 1: Update the Node.js version in .nvmrc from the EOL 20.19.4 release to a
currently supported Node.js LTS release so CI and publication workflows using
this file run on the supported version.
---
Nitpick comments:
In `@README.md`:
- Around line 378-382: Update the release instructions around the GitHub release
tag examples to replace the hardcoded 4.0.7 values with version placeholders,
using v<version> or <version> while preserving the existing
guidance.
🪄 Autofix
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: QUIET
Plan: Pro
Run ID: 2c0e0b54-cbe4-41d3-9223-2341502a5b78
📒 Files selected for processing (5)
.github/workflows/build-test.yml.github/workflows/publish.yml.nvmrcREADME.mdpackage.json
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/publish.yml (2)
23-49: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBind checkout to the release event commit.
github.event.release.tag_namecan resolve a moved or recreated tag. The ancestry check does not detect this. Useref: ${{ github.sha }}and verify thatrefs/tags/${TAG_NAME}^{commit}equals$GITHUB_SHA, or restrict release-tag updates with a tag ruleset.🤖 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 @.github/workflows/publish.yml around lines 23 - 49, Update the checkout step to use ref: ${{ github.sha }} instead of the release tag name, then extend the tag verification around TAG_NAME to resolve refs/tags/${TAG_NAME}^{commit} and require it to equal GITHUB_SHA before publishing. Keep the existing version and main-ancestry checks intact.
51-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConfigure registry authentication after dependency installation.
The repository uses a Yarn v1 lockfile.
setup-nodewrites${NODE_AUTH_TOKEN}to.npmrc, but the workflow setsNODE_AUTH_TOKENonly forPublish. Yarn Classic can fail while reading this configuration duringyarn install.Move
registry-urlandscopeto a secondsetup-nodestep immediately beforePublish. KeepNODE_AUTH_TOKENscoped toPublish.🤖 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 @.github/workflows/publish.yml around lines 51 - 59, Update the workflow’s setup-node configuration so the initial step before Install dependencies only selects the Node version from .nvmrc. Add a second setup-node step immediately before Publish containing registry-url and scope, while keeping NODE_AUTH_TOKEN scoped exclusively to Publish.
🤖 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.
Outside diff comments:
In @.github/workflows/publish.yml:
- Around line 23-49: Update the checkout step to use ref: ${{ github.sha }}
instead of the release tag name, then extend the tag verification around
TAG_NAME to resolve refs/tags/${TAG_NAME}^{commit} and require it to equal
GITHUB_SHA before publishing. Keep the existing version and main-ancestry checks
intact.
- Around line 51-59: Update the workflow’s setup-node configuration so the
initial step before Install dependencies only selects the Node version from
.nvmrc. Add a second setup-node step immediately before Publish containing
registry-url and scope, while keeping NODE_AUTH_TOKEN scoped exclusively to
Publish.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro
Run ID: fc6876fd-19fc-42b1-9e67-f45b4ea38d45
📒 Files selected for processing (4)
.github/workflows/build-test.yml.github/workflows/publish.yml.nvmrcREADME.md
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 @.github/workflows/publish.yml:
- Line 6: Update the publish workflow’s workflow_dispatch and prerelease
publication paths to require a prerelease package.json version before publishing
to the next tag. For manual runs that only retag an existing version, replace
npm publish with npm dist-tag add, while preserving the normal publish flow for
creating prerelease packages and preventing stable versions from being published
as next.
🪄 Autofix
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: QUIET
Plan: Pro
Run ID: b2fe16ef-c86c-43bf-9daa-6ded008adf1a
📒 Files selected for processing (1)
.github/workflows/publish.yml
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 @.github/workflows/publish.yml:
- Around line 53-64: Move the VERSION extraction before the PUBLISH_NEXT branch,
then reject versions with a SemVer prerelease suffix when PUBLISH_NEXT is not
true before running npm publish --tag latest. Preserve the existing rejection of
stable versions for the next tag, or replace both checks with equivalent
bidirectional validation.
🪄 Autofix
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: QUIET
Plan: Pro
Run ID: 2f860c84-1e0f-42ab-8d3a-73d0a8a24ade
📒 Files selected for processing (1)
.github/workflows/publish.yml
2 participants
Description
We were incrementing
counterregardless of ifisRetrieveNextwas true or false.isRetrieveNextis only true when the current diff line is 'removed' and the next diff line is 'added' => we then look ahead to try to line up the added/removed lines instead of displaying them on top of each other. IncrementingcounterwhenisRetrieveNextis true causes the "Show x lines ..." button to end up below the added/removed lines when it should be above.Checklist
<label>:ENG-XXXX <description>)Screenshots
Before
After
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores