Skip to content

feat: add fuzzy file reference autocomplete in editor#282

Merged
dubadub merged 3 commits intomainfrom
feat/editor-file-reference-autocomplete
Mar 23, 2026
Merged

feat: add fuzzy file reference autocomplete in editor#282
dubadub merged 3 commits intomainfrom
feat/editor-file-reference-autocomplete

Conversation

@dubadub
Copy link
Member

@dubadub dubadub commented Mar 23, 2026

Summary

  • Enable @./ file reference autocomplete in the web editor by expanding the trigger regex to include path characters
  • Use LSP textEdit.newText for proper snippet insertion of recipe references
  • Add validFor pattern so CodeMirror keeps the completion popup open while typing path characters, using its built-in fuzzy matcher
  • Bump cooklang-language-server to 0.2.1 which adds server-side fuzzy matching for recipe file references

Test plan

  • Open a recipe in the web editor
  • Type @./ and verify the autocomplete dropdown shows all recipe files
  • Continue typing (e.g. @./pan) and verify results narrow down with fuzzy matching
  • Select a result and verify it inserts correctly with {} brackets

dubadub added 2 commits March 11, 2026 14:56
Update editor.js to support @./ file reference completions:
- Expand trigger regex to include path characters (. / -)
- Use textEdit.newText from LSP for proper snippet insertion
- Add validFor pattern to keep completion open during path typing
- Map FILE completion kind for recipe references

Bump cooklang-language-server to 0.2.1 which adds fuzzy matching
for recipe file references.
@claude
Copy link

claude bot commented Mar 23, 2026

PR Review: feat: add fuzzy file reference autocomplete in editor

Overall this is a clean, focused change. The implementation is straightforward and the commit messages are well-written. A few items worth discussing:


Bug: Regex escape issue in trigger pattern

File: static/js/src/editor.js:44

const match = textBefore.match(/[@#~]([a-zA-Z0-9_.\\/\-]*)$/);

Inside a character class [...], the forward slash / does not need escaping (it's only a regex delimiter concern, and JS regexes don't require escaping / inside [...]). More importantly, the backslash \\ here will match a literal backslash character — which is unnecessary on UNIX/macOS paths and may cause unexpected behavior. Consider simplifying to:

const match = textBefore.match(/[@#~]([a-zA-Z0-9_./\-]*)$/);

Inconsistency: validFor pattern includes space, trigger regex does not

File: static/js/src/editor.js:59

validFor: /^[a-zA-Z0-9_.\\/\- ]*$/,

The validFor pattern includes a space character, but the trigger regex on line 44 does not. This means the completion popup would stay open if the user types a space (CodeMirror uses validFor to decide whether to keep completions live), but a new trigger search would never include a space. While this is unlikely to cause a real issue with file paths, it's inconsistent. The \\ issue applies here as well.


Minor: textEdit position handling

The PR uses item.textEdit?.newText for the inserted text, which is correct. However, LSP textEdit also carries a range field that defines where the replacement should go — which may differ from the from position calculated on line 48 from the prefix length. If the LSP returns a textEdit.range that doesn't align with the CodeMirror-computed from, the insertion could double-insert or misplace text.

This is worth verifying against cooklang-language-server 0.2.1's actual response for @./ completions. If the server always returns the full @./path range in textEdit.range, the from offset should account for the @ character too.


Positive notes

  • The kind === 17 mapping to 'file' is the correct LSP CompletionItemKind.File value — good catch.
  • Preferring textEdit.newTextinsertTextlabel is the right LSP priority order.
  • The validFor approach for keeping CodeMirror's fuzzy filter active during typing is the idiomatic solution here.
  • Dependency bump to cooklang-language-server 0.2.1 is properly reflected in both Cargo.toml and Cargo.lock.
  • README improvement is a nice bonus.

Summary

The core approach is sound. The main things to address before merging are:

  1. The \\ in both regex patterns (matches literal backslash, not intended)
  2. Verify textEdit.range alignment with the computed from offset to avoid insertion bugs

These are relatively minor — the feature will likely work correctly on typical UNIX paths even with the regex quirk. But fixing them would make the implementation more robust.

Remove unnecessary backslash escapes from character classes and
drop the inconsistent space from validFor pattern.
@claude
Copy link

claude bot commented Mar 23, 2026

Code Review

Overall this is a clean, focused change that correctly implements fuzzy file reference autocomplete. The scope is tight and the logic is sound. A few things worth discussing:

Potential Bug: textEdit.range vs client-calculated from

The most notable concern is in how textEdit.newText is used:

// from is calculated client-side as pos - prefix.length (i.e., position after '@')
return {
    from: from,
    options: items.map(item => {
        const text = item.textEdit?.newText || item.insertText || item.label;
        ...
        return { label: item.label, apply: text };  // inserted at client-calculated 'from'
    })
};

When the LSP sends textEdit, it includes both newText and range (the text range to replace). This code takes the newText but discards the range, using the client-calculated from instead. If the LSP's textEdit.range starts at the @ character (to replace the full @./pan sequence), but from points to the character after @, the insertion would produce something like @@./pancakes{} or miss the @ in the replacement. Worth verifying against what cooklang-language-server 0.2.1 actually sends in textEdit.range.

Minor: \- in character classes

Both regexes use \- inside character classes:

/[@#~]([a-zA-Z0-9_./\-]*)$/
/^[a-zA-Z0-9_./\-]*$/

The \- escape is valid but unnecessary — - is already literal when it appears after another character class element at the end. The second fix commit's message says it "Remove unnecessary backslash escapes from character classes" but \- persists in the final state. Either put - at the end unescaped: [a-zA-Z0-9_./-], or keep \- for explicit clarity — just worth being consistent.

Nit: long ternary chain for type

const type = item.kind === 17 ? 'file' : item.kind === 6 ? 'variable' : item.kind === 14 ? 'keyword' : 'text';

This was already heading in this direction before the PR, but now with three branches it's starting to get hard to scan. A small map object would make future additions easier:

const KIND_MAP = { 6: 'variable', 14: 'keyword', 17: 'file' };
const type = KIND_MAP[item.kind] ?? 'text';

Not a blocker, just a readability suggestion.

Everything else looks good

  • . inside [./] is correctly treated as a literal dot (not "any character") — good.
  • validFor: /^[a-zA-Z0-9_./\-]*$/ correctly matches the same character set as the trigger regex, so CodeMirror will keep the dropdown open while the user types path components.
  • Optional chaining item.textEdit?.newText is safe and appropriate.
  • LSP kind 17 → 'file' is correct per the LSP spec.
  • Cargo.lock checksum updated alongside the version bump — clean.
  • README improvement is accurate and useful.

The textEdit.range question is worth a quick check before merging, but otherwise this is solid work.

@dubadub dubadub merged commit 4e8690e into main Mar 23, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant