-
Notifications
You must be signed in to change notification settings - Fork 31
fix(ui/completion): refactor completion engine integration and source… #165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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
This PR refactors the completion engine integration to standardize the interface across different completion systems (nvim-cmp, blink.cmp, and vim's built-in completion). The refactoring introduces a base CompletionEngine class that provides common functionality and a unified API for all engines.
Key Changes:
- Introduced
CompletionEnginebase class with standardized methods for setup, trigger, and completion callbacks - Refactored all completion engines to inherit from the base class and use centralized priority handling
- Added engine factory pattern in completion.lua for consistent engine instantiation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| lua/opencode/ui/input_window.lua | Added type annotation cast and removed commented code for cleaner implementation |
| lua/opencode/ui/completion/engines/vim_complete.lua | Converted to class-based structure inheriting from CompletionEngine, using shared priority handling |
| lua/opencode/ui/completion/engines/nvim_cmp.lua | Refactored to class-based structure with standardized trigger and completion methods |
| lua/opencode/ui/completion/engines/blink_cmp.lua | Converted to class-based structure with dual interface pattern supporting both engine and provider modes |
| lua/opencode/ui/completion.lua | Added engine factory with configuration mapping and centralized engine lifecycle management |
Comments suppressed due to low confidence (1)
lua/opencode/ui/completion/engines/blink_cmp.lua:151
- The Source:get_completions method doesn't use the refactored priority handling like the BlinkCmpEngine. It directly fetches completion_sources and manually builds items with priority formatting, rather than using the base class's get_completion_items method which returns wrapped items with normalized priorities. This means the Source provider path (used by blink.cmp) and the BlinkCmpEngine path diverge in their implementation, creating code duplication and potential inconsistencies in priority handling.
local items = {}
for _, source in ipairs(completion_sources) do
local source_items = source.complete(context):await()
for i, item in ipairs(source_items) do
local insert_text = item.insert_text or item.label
table.insert(items, {
label = item.label,
kind = item.kind,
kind_icon = item.kind_icon,
kind_hl = item.kind_hl,
detail = item.detail,
documentation = item.documentation,
filterText = item.filter_text or item.label,
insertText = insert_text,
sortText = string.format('%02d_%02d_%02d_%s', source.priority or 999, item.priority or 999, i, item.label),
score_offset = -(source.priority or 999) * 1000 + (item.priority or 999),
data = {
original_item = item,
},
})
end
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
… integration and source priority handling
… priority handling - Standardized trigger, setup, and completion callbacks for all engines. - Fix blink opening path completion sometimes this should fix #130
- Change trigger pattern to match from end of string only, preventing false triggers - Remove redundant CompletionEngine require calls in blink_cmp source methods
8dd1332 to
965bc68
Compare
…sues - Fix blink_cmp execute to use correct method call syntax - Add augroup to vim_complete for proper autocmd cleanup
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
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
lua/opencode/ui/completion/engines/blink_cmp.lua:147
- The Source:get_completions method manually iterates through completion sources instead of using the centralized engine:get_completion_items method. This creates inconsistency with nvim_cmp and vim_complete engines which both use the centralized method. This duplication makes the code harder to maintain and could lead to behavior differences between engines. Consider refactoring to use a shared approach, possibly by making the engine instance accessible to the Source object through a closure or module-level variable, similar to how nvim_cmp handles it.
local items = {}
for _, source in ipairs(completion_sources) do
local source_items = source.complete(context):await()
for i, item in ipairs(source_items) do
local insert_text = item.insert_text or item.label
table.insert(items, {
label = item.label,
kind = item.kind,
kind_icon = item.kind_icon,
kind_hl = item.kind_hl,
detail = item.detail,
documentation = item.documentation,
filterText = item.filter_text or item.label,
insertText = insert_text,
sortText = string.format('%02d_%02d_%02d_%s', source.priority or 999, item.priority or 999, i, item.label),
score_offset = -(source.priority or 999) * 1000 + (item.priority or 999),
data = {
original_item = item,
},
})
end
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… priority handling
/won't trigger commands sometimes #130