Skip to content

feat(completion): trigger import completion on space - #460

Merged
16bit-ykiko merged 3 commits into
mainfrom
feat/import-completion-trigger
Jul 17, 2026
Merged

feat(completion): trigger import completion on space#460
16bit-ykiko merged 3 commits into
mainfrom
feat/import-completion-trigger

Conversation

@16bit-ykiko

@16bit-ykiko 16bit-ykiko commented Jun 14, 2026

Copy link
Copy Markdown
Member

Summary

  • Register space as a completion trigger character so that typing import immediately opens the module name suggestion list (same UX as #include < triggering header suggestions).
  • Gate space triggers on both sides:
    • Server: space-triggered requests outside an import context are answered with an empty list instead of falling through to a full completion build, so clients without request-side filtering (nvim, zed) are not charged a heavyweight completion for every space keystroke.
    • VS Code extension: middleware short-circuits space triggers unless the line prefix is import or export import , avoiding the request round-trip entirely in the common case.
  • Add FIXME/TODO annotations for known limitations (textual import detection, self-import not excluded, module map refreshed on save only).

Approach

Follows the same pattern as TypeScript/Haxe language extensions (vscode#67714): the server advertises space as a trigger character and the client does a cheap string check to avoid flooding the server — with the addition that the server independently enforces the same rule, so the UX guarantee does not depend on any particular editor integration.

Test plan

  • Unit tests: detect_completion_context (dotted module prefix, partition prefix, empty partition, leading whitespace, export import, multiline import, mid-line cursor truncation) and complete_module_import (dotted/partition prefix filtering, prefix-equals-full-name).
  • Integration tests: server capability advertises " " in triggerCharacters; space trigger on an import line serves module completions; space trigger elsewhere returns an empty list.
  • VS Code e2e: space-triggered completion outside an import line yields no server items (middleware gate).
  • Full suites green locally: unit 1008 passed, integration 285 passed, smoke 3/3, vscode e2e 7 passing in all three scenarios (RelWithDebInfo).

Summary by CodeRabbit

  • New Features
    • Improved module import completion with support for dotted and partition-style prefixes.
    • Added space-triggered completion for import and export import contexts.
    • Extended completion trigger characters to include space and *.
  • Bug Fixes
    • Prevented module/include completion candidates from appearing on space-triggered requests in non-import contexts.
  • Tests
    • Expanded unit, integration, and VS Code e2e coverage for space-trigger gating and additional import patterns.

@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: da668d0a-3087-4e38-8568-8b3d51661d65

📥 Commits

Reviewing files that changed from the base of the PR and between 59dc9c1 and f8dd539.

📒 Files selected for processing (2)
  • src/server/service/feature_router.cpp
  • tests/integration/features/test_completion.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/integration/features/test_completion.py
  • src/server/service/feature_router.cpp

📝 Walkthrough

Walkthrough

Adds space as an LSP completion trigger, propagates trigger context through the server, filters VS Code space-triggered requests by import context, and expands unit, integration, and end-to-end completion coverage.

Changes

Space-triggered completion

Layer / File(s) Summary
Completion trigger routing
src/server/service/feature_router.*, src/server/transport/lsp_client.cpp
The server advertises space as a completion trigger, forwards optional trigger characters, and returns no results for space-triggered requests outside import contexts.
VS Code completion filtering
editors/vscode/src/extension.ts, editors/vscode/src/test/e2e.test.ts
Middleware permits space-triggered completion only after import or export import , with an end-to-end test covering non-import code.
Completion request and context coverage
tests/tools/client.py, tests/integration/features/*, tests/unit/syntax/completion_tests.cpp, tests/integration/features/test_server.py, src/syntax/completion.cpp
Test clients send trigger-character contexts; tests cover import, include, non-import, module-prefix, and advertised-trigger behavior, alongside completion context TODO/FIXME notes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant VSCode
  participant LSPClient
  participant FeatureRouter
  participant CompletionContext

  VSCode->>LSPClient: completion request with trigger character
  LSPClient->>FeatureRouter: completion(position, trigger_character)
  FeatureRouter->>CompletionContext: detect preamble context
  FeatureRouter-->>LSPClient: filtered or normal completion result
  LSPClient-->>VSCode: completion response
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: enabling import completion when space triggers completion.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/import-completion-trigger

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 299e28f7ad

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/server/service/lsp_client.cpp Outdated
Register space as a completion trigger character so that typing
`import ` immediately shows module name suggestions without needing
to type the first character of the module name.

- Server: add space to trigger_characters in CompletionOptions.
- Extension: middleware gate filters space triggers, only forwarding
  to the server when the line is `import ` or `export import `.
- Tests: cover dotted modules, partition prefixes, and empty-prefix
  cases for detect_completion_context and complete_module_import.
- Add FIXME/TODO annotations for known issues (self-import in results,
  textual detection false positives, incremental refresh).
Space is advertised as a completion trigger to every client, but the
import-line filtering lived only in the VS Code middleware; nvim and
zed would fire a full completion build on every space keystroke.
Answer space-triggered requests outside import contexts with an empty
list in FeatureRouter::completion so all clients are protected.

Also assert the advertised trigger characters in test_capabilities,
cover both sides of the gate with integration tests, exercise the
middleware in the vscode e2e suite, and apply review nits (comment
placement, test naming, mid-line cursor case).
@16bit-ykiko
16bit-ykiko force-pushed the feat/import-completion-trigger branch from 299e28f to 59dc9c1 Compare July 17, 2026 06:11

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 59dc9c1993

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/server/service/feature_router.cpp Outdated
A space typed while the cursor is still in an include context (e.g.
the trailing space in '#include <vector> ') previously reached the
include branch and performed command resolution and directory
enumeration before the gate. Hoist the context detection and check
the gate first so a space trigger only ever serves import contexts.
@16bit-ykiko
16bit-ykiko merged commit edddc61 into main Jul 17, 2026
22 checks passed
@16bit-ykiko
16bit-ykiko deleted the feat/import-completion-trigger branch July 17, 2026 07:21
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