feat(explain): let plugins register their own EXPLAIN parsers - #688
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| return () => { | ||
| cancelled = true; | ||
| for (const format of new Set(loadedExplainFormats)) { |
There was a problem hiding this comment.
[SUGGESTION]: Reload teardown temporarily unregisters still-enabled plugins' EXPLAIN parsers
The effect cleanup unregisters every format in loadedExplainFormats (all parsers loaded during the previous pass), so when the enabled set changes the parsers belonging to plugins that are still enabled are removed too, and only re-registered after the async get_plugin_manifest → read_plugin_file → eval loop completes. An EXPLAIN issued for a still-enabled raw-explain plugin in that window reaches parseRawExplain → getExplainParser returns null and throws "No EXPLAIN parser registered for format …". The gap is brief and only during a settings change, but it is a user-visible transient failure on the raw path. Consider unregistering only the formats of plugins that are no longer enabled (or re-registering still-enabled parsers before the first await) to keep them available across the reload.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Merge (1 suggestion to consider) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (incremental: commit 7cb24f3 since 6159ac7)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit 6159ac7)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 6159ac7)Status: 2 Issues Found | Recommendation: Merge (2 suggestions to consider) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (11 files)
Fix these issues in Kilo Cloud Previous review (commit ba0463d)Status: 1 Issue Found | Recommendation: Merge (1 suggestion to consider) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (25 files)
Reviewed by glm-5.2 · Input: 66.2K · Output: 34.6K · Cached: 3.9M |
A plugin manifest may declare the first Tabularis release it can run on, but the host never checked it: an older build loaded the plugin and failed later, for example in Visual EXPLAIN with "No EXPLAIN parser registered". Add a small semver gate that refuses the plugin up front with a message naming both versions. It runs in load_plugin_from_dir, where the error reaches the startup error banner, and in download_and_install before the bundle leaves the temp dir, covering installs by URL or local file that bypass the marketplace filter. Missing, empty or non-semver floors are logged and treated as compatible so a typo cannot brick loading.
The raw view only knew JSON and plain text. SQL Server SHOWPLAN arrives as a single line of XML, which Monaco showed as one wrapped paragraph. Detect XML from the leading tag, switch the editor language and indent one node per line, keeping quoted attribute values containing '>' intact and leaf text on the same line as its tags. Already multi-line XML, JSON and text pass through unchanged. Also add two loader tests for the cancellation path: no registration when the enabled set changes during a module read, and no read at all when cancelled before the first module.
| ); | ||
| }); | ||
|
|
||
| it("stops before evaluating a bundle once loading is cancelled", async () => { |
There was a problem hiding this comment.
[SUGGESTION]: This test doesn't actually pin the pre-evaluateBundle cancellation guard it's named for
The test name claims loading "stops before evaluating a bundle", but none of its three assertions observe whether evaluateBundle ran. The bundle source returned here has no eval-time side effect (it only assigns __tabularis_explain_parser__), and registerExplainParser is gated by a separate checkpoint (pluginExplainLoader.ts:130). So if the post-read / pre-evaluate guard at pluginExplainLoader.ts:103 were deleted, the plugin JS would still execute, yet formats would stay [], invokeMock stay called once, and getExplainParser(\"cancelled-format\") stay null — this test stays green. (The sibling test at line 234 correctly pins the pre-read guard via expect(invokeMock).not.toHaveBeenCalled().)
Consider making the bundle source emit an observable side effect on evaluation (for example, set a sentinel) and asserting it did not fire, so this test actually guards the security-relevant "don't run cancelled plugin bundles" property.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
… a warning toast A hard refusal makes it impossible to test a plugin that already declares the next host release as its floor, which is exactly the situation while that release is being prepared. In debug builds the gate now loads the plugin anyway, logs the mismatch and queues a runtime warning; release builds keep refusing. The frontend drains the queue through get_plugin_runtime_warnings on startup and whenever the enabled plugin set changes, which is when plugins are loaded through install or enable, and shows each warning as a toast in the bottom-right corner with the plugin id in its title.
| // The marketplace hides incompatible releases, but archives can also be | ||
| // installed by URL or from a local file, so gate the host version here | ||
| // while the bundle is still in the temp dir. | ||
| if let Err(e) = super::runtime_version::enforce_min_runtime_version( |
There was a problem hiding this comment.
[SUGGESTION]: Installing an incompatible plugin on a dev build queues the runtime warning twice
download_and_install now calls enforce_min_runtime_version, which in a development build returns Ok via a DevOverride and push_runtime_warnings. The install_plugin command then immediately hot-loads the plugin through load_plugin_from_dir (commands.rs:221), which calls enforce_min_runtime_version a second time and pushes the same warning again. Two identical PluginRuntimeWarning entries for one plugin land in the queue, so PluginRuntimeWarningToasts renders two identical dev-override toasts for a single install. Consider having the load path own the warning (e.g., make the install gate in download_and_install only refuse in release builds and skip queueing) or dedupe.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Summary
Lets a driver plugin ship its own Visual EXPLAIN parser instead of having to pre-parse plans into the host's
ExplainPlanshape in Rust. A plugin can now return a rawexplain_querypayload tagged with anengineand aformat, declare a TypeScript parser bundle for that format in.tabularium, and the desktop loads and registers it at runtime.This is the core half of the SQL Server plugin completion work. The plugin side is TabularisDB/tabularis-sqlserver-plugin#3, and the standalone site side is TabularisDB/explain-plan#2.
What changed
@tabularis/explain(0.1.0 to 0.2.0)registerExplainParser,unregisterExplainParser,getExplainParser,listExplainParsersand theRegisteredExplainParserdescriptor (engine,format,label,parse, optionalsniff).parseRawExplainand source detection dispatch by format instead of a hard-coded switch.Backend
PluginManifestgains an optionalexplain_parsersarray (engine,format,module, optionallabel), mirrored inplugins/manifest.schema.jsonandtabularium-extensions.schema.json. Built-in drivers reportNone.explain_queryresult structurally:engine,formatandpayloadmust all be strings.original_querymay be omitted ornulland is filled from the request. Anything else keeps the historical parsed-plan path, so existing plugins are unaffected.ExplainOutputdocs updated to describe both plugin shapes.plugins/runtime_version.rs: the host now enforcesmin_runtime_version.load_plugin_from_dirrefuses an incompatible plugin with a message naming both versions, which surfaces in the startup error banner, anddownload_and_installrejects the archive while it is still in the temp dir, covering installs by URL or local file that bypass the marketplace filter. Missing, empty or non-semver floors are logged and treated as compatible. Development builds (debug_assertions) load the plugin anyway and queue a warning that the frontend shows as a bottom-right toast via the newget_plugin_runtime_warningscommand, so a plugin declaring the next release as its floor stays testable. Comparison follows semver precedence, so a0.23.0-nightly.1host does not satisfy a0.23.0floor while0.23.1-3does.Frontend
pluginExplainLoader.tsreads each declared module once through the existingread_plugin_filecommand (which already rejects absolute paths and..), evaluates the IIFE the same way UI extension bundles are evaluated, resolves the export by exactengineandformat, applies the manifest label and registers it. Read, evaluation and descriptor failures are logged per plugin and skipped; parser exceptions during actual parsing still surface through Visual EXPLAIN's normal error handling.PluginSlotProviderexposes the explain API aswindow.__TABULARIS_EXPLAIN__for externalized bundles, and unregisters the formats it loaded before every reload so disable and re-enable cycles are deterministic. Plugins are processed in sorted id order.src/utils/explainRaw.tsdetects XML from the leading tag, switches the Monaco language toxmland indents a single-line document one node per line (quoted attribute values containing>stay intact, leaf text stays inline). SQL Server SHOWPLAN arrives as one line, so without this the raw tab was a wrapped paragraph. JSON, plain text and already formatted XML pass through unchanged.PLUGIN_GUIDE.mdsection 3c documents the manifest field, the IIFE contract (__tabularis_explain_parser__global, externalized@tabularis/explain, default export of one descriptor or an array) and the rawexplain_queryresult shape.Compatibility
min_runtime_versionto the first release that ships it (planned as 0.23.0). Older hosts that include this PR refuse such a plugin at install and load time instead of failing in Visual EXPLAIN.explain_parsersmanifest field is additive. The registry validator schema needs the same addition before such a plugin can be submitted.Verification
packages/explain/tests/registry.test.ts: registration, replacement, unregistration, detection order and built-in parity.tests/utils/pluginExplainLoader.test.ts: single read per shared module, array exports, manifest-order registration, export matching, label override, isolation of throwing bundles and invalid exports, reload after unregistration, cancellation during and before a module read.tests/utils/explainRaw.test.ts: language detection, XML indentation including>inside attribute values, declarations, comments and CDATA, pass-through for JSON, text and multi-line XML.src-tauri/src/plugins/tests.rs:min_runtime_versiondeserialization and the runtime gate (no floor, equal or newer host, older host message, prerelease hosts, non-semver values, development override verdict, warning queue drained once).tests/components/plugins/PluginRuntimeWarningToasts.test.tsx: one toast per queued warning with the plugin id in the title, nothing on an empty queue or outside Tauri, re-drain when the enabled plugin set changes.src-tauri/src/plugins/driver.rsandplugins/tests.rs: raw result detection,original_queryvalidation, fallback to parsed plan for malformed raw objects, manifest deserialization with and withoutexplain_parsers.testandtest-postgresCI jobs pass.Manual validation
SHOWPLAN_XMLplan and an actualSTATISTICS XMLplan through the plugin-providedsqlserver-showplan-xmlparser.