-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
Closes marimo-team/marimo#8211
Problem
When accepting an autocomplete suggestion for a variable containing an underscore (e.g., REF_VALUE), the result was duplicated text like REFREF_VALUE instead of the expected REF_VALUE.
Root Cause
In requestCompletion(), prefixMatch(items) generates a regex from the common prefix of all completion items. When the LSP returns items like ["REF_VALUE", "RESULT", "RETURN"], the common prefix is just "RE", producing a regex /(R|RE)$/.
When the user typed REF (which extends beyond the common prefix RE), context.matchBefore(/(R|RE)$/) returned null because "REF" doesn't end with just "R" or "RE". Since prefixMatch returned a non-null regex, the fallback pattern /[a-zA-Z0-9]+/ was never tried.
With token = null, pos stayed at context.pos (cursor position after REF), so the completion text REF_VALUE was inserted at the cursor instead of replacing the typed prefix -- producing REFREF_VALUE.
Fix
- When
matchBefore(match)returns null, fall back to a general word-character regex using?? - Added
_to the fallback regex (/[a-zA-Z0-9_]+/) for consistency with identifier characters
## Problem
When accepting an autocomplete suggestion for a variable containing an underscore (e.g., `REF_VALUE`), the result was duplicated text like `REFREF_VALUE` instead of the expected `REF_VALUE`.
## Root Cause
In `requestCompletion()`, `prefixMatch(items)` generates a regex from the **common prefix** of all completion items. When the LSP returns items like `["REF_VALUE", "RESULT", "RETURN"]`, the common prefix is just `"RE"`, producing a regex `/(R|RE)$/`.
When the user typed `REF` (which extends beyond the common prefix `RE`), `context.matchBefore(/(R|RE)$/)` returned `null` because `"REF"` doesn't end with just `"R"` or `"RE"`. Since `prefixMatch` returned a non-null regex, the fallback pattern `/[a-zA-Z0-9]+/` was never tried.
With `token = null`, `pos` stayed at `context.pos` (cursor position after `REF`), so the completion text `REF_VALUE` was **inserted** at the cursor instead of **replacing** the typed prefix -- producing `REFREF_VALUE`.
## Fix
- When `matchBefore(match)` returns null, fall back to a general word-character regex using `??`
- Added `_` to the fallback regex (`/[a-zA-Z0-9_]+/`) for consistency with identifier characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes an autocompletion edge case in the CodeMirror LSP plugin where accepting a completion for identifiers containing underscores could insert duplicated text instead of replacing the already-typed prefix.
Changes:
- Add a fallback
matchBeforepattern when the prefix-based regex fails to match the typed text. - Expand the fallback identifier regex to include underscores (
_). - Add new
prefixMatchtests covering underscore-related label sets and the "typed beyond common prefix" scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/plugin.ts | Adjusts token detection for completion replacement by falling back to a broader identifier match when the prefix regex yields no token. |
| src/tests/utils.test.ts | Adds additional test coverage around prefixMatch behavior for underscore-containing labels and partial common prefixes. |
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const token = match | ||
| ? context.matchBefore(match) | ||
| : // Fallback to matching any character | ||
| context.matchBefore(/[a-zA-Z0-9]+/); | ||
| ? // Try prefix-based match, then fall back to general word match | ||
| (context.matchBefore(match) ?? | ||
| context.matchBefore(/[a-zA-Z0-9_]+/)) | ||
| : // Fallback to matching any word character | ||
| context.matchBefore(/[a-zA-Z0-9_]+/); |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new fallback logic in requestCompletion changes how from is computed when context.matchBefore(match) returns null, but there's no test exercising this scenario. Consider adding a unit test in src/__tests__/language-server-plugin.test.ts that mocks context.matchBefore to return null for the prefix regex and a token for the fallback regex, then asserts the returned CompletionResult.from uses the fallback token's from (preventing duplicated insertions like REFREF_VALUE).
| const token = match | ||
| ? context.matchBefore(match) | ||
| : // Fallback to matching any character | ||
| context.matchBefore(/[a-zA-Z0-9]+/); | ||
| ? // Try prefix-based match, then fall back to general word match | ||
| (context.matchBefore(match) ?? | ||
| context.matchBefore(/[a-zA-Z0-9_]+/)) | ||
| : // Fallback to matching any word character |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation inside the nullish-coalescing expression is inconsistent with the surrounding formatting (the second context.matchBefore line is less indented). Running pnpm run lint (Biome) or adjusting indentation will keep formatting consistent and avoid churn in future edits.
| expect("a").toMatch(result as RegExp); | ||
| }); | ||
|
|
||
| it("should match partial prefix for underscore variables", () => { |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test name is misleading: this case is about the user typing beyond the common prefix (e.g., REF vs common prefix RE), not specifically about underscores (the underscore isn't relevant to the assertion). Renaming the test to reflect the "typed text extends beyond common prefix" scenario would make intent clearer.
| it("should match partial prefix for underscore variables", () => { | |
| it("should handle typed text extending beyond common prefix", () => { |
Closes #8211
Upgrades codemirror-languageserver to pick up
marimo-team/codemirror-languageserver#37
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>