fix(ui/completion): refactor completion engine integration and source… #165

sudo-tee · 2026-01-06T15:19:36Z

… priority handling

Standardized trigger, setup, and completion callbacks for all engines.
Fix blink opening path completion sometimes this should fix / won't trigger commands sometimes #130

… priority handling - Standardized trigger, setup, and completion callbacks for all engines. - Fix blink opening path completion sometimes this should fix #130

Copilot

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 CompletionEngine base 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.

Copilot · 2026-01-06T15:24:23Z

lua/opencode/ui/completion/engines/blink_cmp.lua

+    local triggers = CompletionEngine.get_trigger_characters()
+    local trigger_chars = table.concat(vim.tbl_map(vim.pesc, triggers), '')
    local trigger_char, trigger_match = before_cursor:match('([' .. trigger_chars .. '])([%w_/%-%.]*)$')



The trigger matching pattern here differs from the pattern used in the base CompletionEngine.parse_trigger method. The base class uses .*([...])([%w_%-%.]*) (note the trailing *) while this uses ([...])([%w_/%-%.]*) $ (with $ anchor and includes /). This inconsistency could lead to different behavior between engines. Consider using the base class's parse_trigger method for consistency, or if the differences are intentional (adding / for file paths), document why this pattern is needed specifically for blink.

Suggested change

local triggers = CompletionEngine.get_trigger_characters()

local trigger_chars = table.concat(vim.tbl_map(vim.pesc, triggers), '')

local trigger_char, trigger_match = before_cursor:match('([' .. trigger_chars .. '])([%w_/%-%.]*)$')

local trigger_char, trigger_match = CompletionEngine.parse_trigger(before_cursor)

Copilot · 2026-01-06T15:24:23Z

lua/opencode/ui/completion/engines/blink_cmp.lua

+    local CompletionEngine = require('opencode.ui.completion.engines.base')
+    local engine = CompletionEngine.new('blink_cmp')
+    engine:on_complete(item.data.original_item)


Creating a new engine instance here is incorrect. This creates a fresh engine with no completion sources, which means on_complete won't be able to find the appropriate source to call. The engine should be passed or accessed as a shared instance (similar to how nvim_cmp.lua handles this with the 'engine' variable captured in the closure). This could lead to completion callbacks not working properly.

Copilot · 2026-01-06T15:24:24Z

lua/opencode/ui/completion/engines/blink_cmp.lua

+-- Export module with dual interface:
+-- - For our engine system: use BlinkCmpEngine methods
+-- - For blink.cmp provider system: override 'new' to return Source instance
+local M = BlinkCmpEngine

-  blink.add_source_provider('opencode_mentions', {
-    module = 'opencode.ui.completion.engines.blink_cmp',
-    async = true,
-  })
-
-  return true
-end
+-- Save the engine constructor before overriding
+M.create = BlinkCmpEngine.new

+-- Override 'new' for blink.cmp compatibility (when blink loads this as a source)
 M.new = Source.new


The module export pattern here is confusing and could lead to unexpected behavior. Setting M = BlinkCmpEngine makes the module return the class itself, then overriding M.new changes the class's new method. This means when someone imports this module and calls .new(), they get a Source instance instead of a BlinkCmpEngine instance, unless they use the .create() method. This dual interface is documented in the comment but is error-prone. Consider using a more explicit pattern like returning a table with separate engine and source fields, or documenting this pattern more prominently.

Copilot · 2026-01-06T15:24:24Z

lua/opencode/ui/completion.lua

 local M = {}

 local completion_sources = {}
+local _current_engine = nil


Both the local variable _current_engine (line 4) and the module field M._current_engine (line 70) are defined but only M._current_engine is used. The local variable declaration at the module level serves no purpose and could be confusing. Consider removing the local variable declaration and only using M._current_engine, or document why both exist.

Suggested change

local _current_engine = nil

M._current_engine = nil

Copilot · 2026-01-06T15:24:24Z

lua/opencode/ui/completion/engines/vim_complete.lua

+local VimCompleteEngine = setmetatable({}, { __index = CompletionEngine })
+VimCompleteEngine.__index = VimCompleteEngine

 local completion_active = false


The completion_active flag is a module-level variable shared across all VimCompleteEngine instances. If multiple instances are created (though unlikely in practice), they would share this state, leading to incorrect behavior. Consider making completion_active an instance variable (self.completion_active) to properly encapsulate state per engine instance.

Copilot · 2026-01-06T15:24:25Z

lua/opencode/ui/completion/engines/vim_complete.lua

  vim.api.nvim_create_autocmd('FileType', {
    pattern = 'opencode',
    callback = function(args)
      local buf = args.buf
-      vim.api.nvim_create_autocmd('TextChangedI', { buffer = buf, callback = M._update })
-      vim.api.nvim_create_autocmd('CompleteDone', { buffer = buf, callback = M.on_complete })
+      vim.api.nvim_create_autocmd('TextChangedI', { buffer = buf, callback = function() self:_update() end })
+      vim.api.nvim_create_autocmd('CompleteDone', { buffer = buf, callback = function() self:_on_complete_done() end })
    end,
  })


The autocmd callbacks capture 'self' in closures, but the FileType autocmd is created without an augroup or 'once' flag. This means every time setup() is called, new TextChangedI and CompleteDone autocmds are registered for each opencode buffer. If setup() is called multiple times, this will create duplicate autocmds, leading to the callbacks being invoked multiple times per event. Consider wrapping these in an augroup with clear = true, or using the 'once' option for the FileType autocmd.

Copilot

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

fix(ui/completion): refactor completion engine integration and source…

8dd1332

… priority handling - Standardized trigger, setup, and completion callbacks for all engines. - Fix blink opening path completion sometimes this should fix #130

sudo-tee requested a review from Copilot January 6, 2026 15:19

Copilot started reviewing on behalf of sudo-tee January 6, 2026 15:20 View session

Copilot AI reviewed Jan 6, 2026

View reviewed changes

epheien pushed a commit to epheien/opencode.nvim that referenced this pull request Jan 8, 2026

Merge PR sudo-tee#165: fix(ui/completion): refactor completion engine…

8c57a04

… integration and source priority handling

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(ui/completion): refactor completion engine integration and source… #165

fix(ui/completion): refactor completion engine integration and source… #165

sudo-tee commented Jan 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fix(ui/completion): refactor completion engine integration and source… #165

Are you sure you want to change the base?

fix(ui/completion): refactor completion engine integration and source… #165

Conversation

sudo-tee commented Jan 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants