fix: add range offset for insertion suggestion #768
Conversation
🦋 Changeset detectedLatest commit: 3d8c635 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughreportDifference in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✏️ Tip: You can disable this entire section by setting 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 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 69b4661 in 2 minutes and 6 seconds. Click for details.
- Reviewed
36lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. eslint-plugin-prettier.js:109
- Draft comment:
Prefer using a ternary (insertText.length ? 1 : 0) for clarity instead of Math.min(insertText.length, 1). - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. eslint-plugin-prettier.js:125
- Draft comment:
Ensure that adding the ghost offset (end + insertSuggestionOffset) does not exceed the source text length. Consider clamping the index if the suggestion is at the very end of the file. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is making a speculative suggestion about potential bounds checking. The diff shows thatinsertSuggestionOffsetis at most 1 (fromMath.min(insertText.length, 1)), so we're adding at most 1 toend. Theendvalue comes fromoffset + deleteText.length, which should be valid indices from the difference calculation. Without seeing evidence thatgetLocFromIndexfails on out-of-bounds indices, or that the difference calculation can produce invalid offsets, this seems speculative. The comment uses "Consider" language which suggests uncertainty. According to the rules, I should not keep speculative comments that say "If X, then Y is an issue" - only if it's definitely an issue. I might be missing that ESLint'sgetLocFromIndexor the ponyfill implementation could fail or produce incorrect results with out-of-bounds indices. The ponyfill implementation shown doesn't have explicit bounds checking, so passing an index beyond the text length might produce unexpected results. While the ponyfill might not have explicit bounds checking, there's no evidence in the diff that this is actually causing a problem or that the indices being passed would ever be out of bounds. The difference objects should come from valid text comparisons. This remains a speculative concern without concrete evidence of failure. This comment is speculative and uses "Consider" language without demonstrating that there's an actual bug. It suggests defensive programming but doesn't show strong evidence that the issue occurs. According to the rules, speculative comments should be removed.
Workflow ID: wflow_8LTebGtVsneEozlD
You can customize 
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eslint-plugin-prettier.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dotcarmen
PR: prettier/eslint-plugin-prettier#743
File: eslint-plugin-prettier.js:0-0
Timestamp: 2025-06-14T15:04:53.678Z
Learning: In the getLocFromOffset function for eslint-plugin-prettier.js, the correct implementation uses `const withSentinel = [-1, 0, ...lineIndexes]` to create an array of line start positions without variable shadowing, and calculates column as `offset - withSentinel[line]` without additional offsets.
📚 Learning: 2025-06-14T15:04:53.678Z
Learnt from: dotcarmen
PR: prettier/eslint-plugin-prettier#743
File: eslint-plugin-prettier.js:0-0
Timestamp: 2025-06-14T15:04:53.678Z
Learning: In the getLocFromOffset function for eslint-plugin-prettier.js, the correct implementation uses `const withSentinel = [-1, 0, ...lineIndexes]` to create an array of line start positions without variable shadowing, and calculates column as `offset - withSentinel[line]` without additional offsets.
Applied to files:
eslint-plugin-prettier.js
🔇 Additional comments (1)
eslint-plugin-prettier.js (1)
124-127: Verify boundary handling and REPLACE operation behavior.The concern about boundary handling when
offsetis at the end of the file is valid. Analysis reveals:
insertSuggestionOffsetisMath.min(insertText.length, 1), so it's either 0 or 1- When an INSERT operation occurs at the end of file,
loc.endbecomesgetLocFromIndex(sourceCode, sourceCode.text.length + 1), which is out of bounds- The
getLocFromIndexponyfill clamps to the last line and returns a column value equal to(out-of-bounds-index) - (last-line-start), which can exceed the actual line length- No tests currently exist for INSERT, DELETE, or REPLACE operations, including boundary cases
While ESLint may accept such positions, this behavior should be explicitly tested to confirm it doesn't cause issues in editors or downstream tools. Additionally, if REPLACE operations intentionally extend the location range beyond deleted text (as mentioned in the original comment), this should be clarified and tested.
commit: |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where ESLint's dropdown menu for code actions doesn't work correctly when Prettier suggests inserting characters at a zero-width range. The fix separates the highlight range (used for visual indication) from the replace range (used for the actual fix).
Changes:
- Split the single
rangevariable intohighlightRange(for visualization) andreplaceRange(for fixes) - Added a minimum width of 1 character to
highlightRangefor insertion suggestions usingMath.max(deleteText.length, 1) - Added a changeset documenting this patch-level fix
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| eslint-plugin-prettier.js | Separates highlight range from replace range, ensuring insertion suggestions have a visible 1-character highlight width while maintaining correct fix behavior |
| .changeset/smart-terms-sneeze.md | Documents the fix as a patch-level change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const deleteText = 'deleteText' in difference ? difference.deleteText : ''; | ||
| /** @type {AST.Range} */ | ||
| const range = [offset, offset + deleteText.length]; | ||
| const highlightRange = [offset, offset + Math.max(deleteText.length, 1)]; |
There was a problem hiding this comment.
The change to use Math.max(deleteText.length, 1) for highlightRange will modify the behavior of INSERT operations, changing the end position from the insertion point to insertion point + 1. Existing tests in test/prettier.mjs (lines 244-258 and 262-293) expect INSERT operations to have matching start and end columns (e.g., column: 27, endColumn: 27), but this change will make endColumn be column + 1. The tests need to be updated to reflect this new behavior, or this PR will cause test failures.
I tried on VSCode, there is no such issue, so I believe it should be resolved on Zed's ESLint integration instead. |
What does VSCode have to do with it? The guys from Zed said that this plugin returns indexes [0, 0] (see screenshots with debugging). How does the Zed cursor know that edits should be suggested at this point? It's logically simple. |
|
As an example, take a look at how the eslint plugin react hooks works: the array does not contain the required dependencies, but they return the indexes of the array to which the dependencies need to be added. The same logic should be here, but with an empty symbol plus ghost offset. Wdyt? 
|
The editor experience is powered by
The offsets contains |
2 participants
The issue has started from there: zed-industries/zed#41049
The editor receives a zero range when characters from prettier need to be added. This causes the dropdown menu with actions (fixes) to work incorrectly. I added 1 "ghost" offset index, now it works properly (see video below).
Screen.Recording.2025-10-30.at.13.45.26.mov
Important
Adds a 'ghost' offset in
reportDifference()ineslint-plugin-prettier.jsto fix dropdown menu behavior for insertion suggestions.reportDifference()ineslint-plugin-prettier.js.insertTextis present, ensuring correct dropdown menu behavior.reportDifference()to calculateendwithinsertSuggestionOffset.getLocFromIndex()to determinestartandendpositions with the new offset.This description was created by
for 69b4661. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.