Repository navigation
feat(editor): add opt-in AI inline completion - #1455
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of What changed since the last review
Files Reviewed (2 files)
Previous Review Summary (commit 257be94)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 257be94)Status: No Issues Found | Recommendation: Merge Reviewed commit What was checked
Non-blocking observations (not posted inline)
Files Reviewed (35 files)
Reviewed by claude-sonnet-5.5 · Input: 254 · Output: 1.4K · Cached: 168.1K Review guidance: REVIEW.md from base branch |
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for making completion opt-in on both sides and keeping accepted text separate from query execution. I reviewed eeb3a051d4d3c123fc0fb5dc466cb3b939902725, including the active-namespace metadata selection, cancellation ownership, editor lifecycle and fenced-response handling.
The 83 targeted frontend tests and 23 matching Rust tests passed, as did typecheck, targeted source ESLint and the production frontend build. I found no evident issue in this pass. I will exercise the actual Monaco ghost text, Tab/Escape/undo, context switches and the disabled/failure paths in the real app, then run the full required checks before merging. Provider fixtures will stay local.
|
Tested Exercised automatic ghost text, Escape, Tab, one-step undo, normal autocomplete/indentation priority, a tab switch during a delayed response, provider failure/backoff and disabled frontend/backend opt-in. Query history stayed empty; acceptance never produced a result set. Captured HTTP payloads contained only the bounded SQL context, dialect and cached table names, with no result-row data. |
Resolves the conflicts with API Route (TabularisDB#1459) and the inline completion and optional-key changes (TabularisDB#1455, TabularisDB#1457). Both providers are kept in every list, with Opper after Requesty and API Route after Opper. In ai.rs the Opper model fetch stays as step 7, followed by main's new custom-openai discovery and the API Route catalog. Inline completion goes through dispatch_provider, so it reaches Opper without further changes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Closes #1414.
SQL consoles can now display optional AI continuations as Monaco ghost text. The feature defaults off and uses the provider/model already selected in AI settings, including Ollama and custom OpenAI-compatible local endpoints. Tab/Escape retain Monaco's normal acceptance/dismissal behavior, autocomplete and indentation keep priority, and suggestions contain only an insertion: accepting them never invokes query execution.
Enabling the setting discloses the payload: at most 4,000 characters before the cursor, 1,000 after it and 8,000 characters of already-cached table/schema names. Metadata is selected for the active connection/database/schema and allowlists names only. No result rows, table comments, connection credentials or additional database queries are added. SQL literals remain part of the user's disclosed editor context.
Requests debounce for 600 ms, stop during IME composition and cancel on edits, cursor/model/editor/context/provider changes or focus loss. Backend cancellation is scoped by runtime and web session, handles cancellation arriving before generation, and drops the provider future. Requests time out after 20 seconds; errors leave normal editing intact and back off for 30 seconds. Version/cursor checks discard stale responses. Provider responses must contain a bounded JSON completion string, preserving whitespace without accepting commands or extra edits. The inline parser also applies the existing response cleaner so fenced JSON works through the ChatGPT path as well as the other provider helpers.
Desktop and web expose the same implementation through two typed sensitive commands. The persisted opt-in is checked in the backend too. Built-in and external database drivers use the same path, with their declared SQL dialect and cached metadata; no driver ids are special-cased. All 13 locales include the setting, privacy disclosure and key guidance. Embedded notebook/modal editors remain unchanged in this milestone.
Validation:
git diff --check.cargo check --locked --no-default-features --bin tabularisis locally blocked by vendored OpenSSL requiring a missing Perl executable. Full application/RPC tests and the new browser test have not been run locally and require CI. No paid/live provider requests or interactive desktop smoke test were performed.GitNexus upstream impacts and staged change detection were run. Its final graph classified all 300 flows as CRITICAL, including unrelated grid flows attributed to generic test-interface methods. The actual 35-file diff and command/editor call sites were inspected directly; the graph's broad classification is not treated as reliable coverage.
Coordination with #1337: this PR targets current
mainand reuses its existing provider dispatcher, without adding a provider adapter or credential store. #1337 changes the signatures ofresolve_modelanddispatch_provider; whichever PR lands second must rebase and adjust the two calls inai/inline.rs(plus resolve shared AI settings/command registration conflicts). No provider-refactor commits are bundled here.Manual check: enable AI inline completion in AI settings after reviewing the disclosure, choose a configured provider/model, connect to a database and type in a SQL console. Ordinary completion widgets take priority. Accept visible ghost text with Tab, verify one Undo restores the previous SQL, and confirm no query runs. Repeat with Escape, a tab/connection/model switch, IME input and an unavailable provider. Disable the setting and confirm provider requests stop.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Follow-up to the non-blocking review observations: the full GitHub CI, including Rust and all three browser engines, passed on 257be94. The fenced-JSON normalization regression was reproduced locally and now passes with the other three backend harness tests and Clippy. The follow-up commit still requires its fresh CI run. The provider-side token budget remains a shared-dispatcher improvement; the 20-second timeout is not a token or cost cap.