feat: VS Code extension MVP — ground-up rebuild with daemon architecture#559
feat: VS Code extension MVP — ground-up rebuild with daemon architecture#559joshsmithxrm wants to merge 417 commits intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request marks a significant architectural shift for the VS Code extension, moving to a daemon-based model for all core operations. This change enables a more robust and feature-rich development experience within VS Code, offering advanced querying capabilities, comprehensive environment and profile management, and a foundation for future tooling. The focus is on delivering a powerful, integrated environment for Power Platform developers. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This is an impressive and substantial pull request that successfully rebuilds the VS Code extension from the ground up with a new daemon-based architecture. The scope of work is extensive, introducing major features like Dataverse notebooks, a data explorer, and solution browsing, while also making significant architectural improvements for robustness and security. The code quality is high, and the detailed planning documents are a great addition. My review focuses on a few key areas, including resource management in asynchronous operations, the end-to-end testing setup, and data handling logic to ensure the new foundation is as solid as possible.
Note: Security Review did not run due to the size of the PR.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
68e42e8 to
09eb073
Compare
- SqlQueryService now throws DmlConfirmationRequired (not DmlBlocked)
when safetyResult.RequiresConfirmation is true, eliminating the
fragile message-sniff (ex.Message.Contains("--confirm")) in the handler
- RpcMethodHandler catches DmlConfirmationRequired and DmlBlocked as
separate clauses with their correct error data shapes
- Add catch-all PpdsException clause that maps to ExecutionFailed so
unexpected PpdsException codes don't propagate unhandled
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…/fetch InjectTopAttribute is still used by the query/fetch RPC handler for raw FetchXML TOP injection. The spec incorrectly listed it for deletion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…asks 8-12) Integrate the 8 ppds:* query hints into the shared SqlQueryService so they work across TUI, CLI, and VS Code. QueryHintParser was already complete and tested but never called in production code — this wires it into PrepareExecutionAsync and ExplainAsync. Changes: - Add ForceClientAggregation/NoLock to QueryPlanOptions - Create QueryExecutionOptions record (BypassPlugins/BypassFlows) - Add ExecutionOptions to QueryPlanContext - Add IQueryExecutor.ExecuteFetchXmlAsync overload with DIM pattern - Override in QueryExecutor using RetrieveMultipleRequest for bypass params - FetchXmlScanNode: inject no-lock="true", route to new executor overload - ExecutionPlanBuilder: pass noLock, add ForceClientAggregation routing to PlanClientSideAggregate with ClientAggregateNode - SqlQueryService: parse hints, apply USE_TDS/MAX_ROWS/MAXDOP/NOLOCK/ HASH_GROUP/BYPASS_PLUGINS/BYPASS_FLOWS overrides to plan options Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… SqlQueryResult New QueryDataSource record identifies environments that contributed data to a query result. SqlQueryResult gains DataSources (list of contributing environments) and AppliedHints (hint names that were active). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds envColor: string | null to QueryPanelHostToWebview and SolutionsPanelHostToWebview updateEnvironment messages to carry the configured environment color from host to webview. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sPanel Adds an environmentColor field to both panels, fetches it from daemon.envConfigGet() after initialization and environment picker selection, and includes it in all updateEnvironment postMessage calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…toolbars Adds CSS rules in shared.css mapping envColor values to left-border accents on .toolbar, and updates both webview updateEnvironment handlers to set/remove the data-env-color attribute accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…an tree Implements Task 19 — walks the QueryPlanResult tree after planning to collect RemoteScanNode labels into DataSources and emits active hint names in AppliedHints on SqlQueryResult and SqlQueryStreamChunk (final chunk). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…reExecutionAsync/ExplainAsync Covers AC-04 through AC-13 and AC-21 across three test files: - SqlQueryServiceTests: USE_TDS routing, NOLOCK FetchXML injection, MAX_ROWS cap, MAXDOP pool cap, BATCH_SIZE passthrough, hint overrides caller settings, malformed/unknown hints silently ignored, ExplainAsync plan reflection - QueryExecutorTests: BYPASS_PLUGINS sets BypassCustomPluginExecution parameter, BYPASS_FLOWS sets SuppressCallbackRegistrationExpanderJob parameter, null options falls through to standard RetrieveMultiple path - ExecutionPlanBuilderTests: HASH_GROUP forces ClientAggregateNode over FetchXML aggregate pushdown, non-aggregate queries unaffected Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cross-env banner - Add QueryDataSourceDto DTO and DataSources/AppliedHints fields to QueryResultResponse (C#) - Map SqlQueryResult.DataSources and AppliedHints into RPC response in QuerySqlAsync (only when 2+ sources) - Add dataSources/appliedHints to QueryResultResponse TypeScript interface - Add #data-source-banner element to QueryPanel HTML template - Wire banner show/hide logic in handleQueryResult based on dataSources presence - Add .data-source-banner/.data-source-label/.data-source-tag CSS classes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eResolutionService When environments.json has duplicate labels, ProfileResolutionService throws. Catch the ArgumentException and degrade gracefully — cross-env queries are disabled but single-env queries proceed normally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…sages
VS Code sends internal messages like {vscodeScheduleAsyncWork: 1} to
webview iframes. These lack the 'command' property and hit the
assertNever exhaustive switch default, causing uncaught errors. Add a
guard to skip messages without 'command' before entering the switch.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…top override The extension sends top:100 as a default on every query. Previously, TopOverride always clobbered the SQL's own TOP clause, so SELECT TOP 5 returned 100 rows. Now TopOverride only applies when the SQL has no explicit TOP — the user's intent takes precedence. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…across all skills Retrospective found the previous session shipped broken features (TDS lies, TOP override bug) because it never visually verified its work — only ran mock-based unit tests. These changes close that gap: - NEW /qa command: dispatches a fresh agent with no source code access to test the running product via CDP/CLI/MCP. Loops until all checks pass. - CLAUDE.md: add verification and QA gate rules to Workflow section - /gates: add post-gate reminder that mechanical gates are insufficient for UI - /implement: invoke /qa at phase gates for extension, CLI, and MCP changes - /verify: make Phase B mandatory for query/data-display changes - @webview-panels: add visual verification to quality gates, update file tree (error-handler.ts, selection-utils.ts), document data-env-color theming - @retrospective: add $ARGUMENTS support, default to latest session instead of hard-coded "2 days ago" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…TOP in ExplainAsync - QueryMode now always reports "dataverse" since ITdsQueryExecutor is never registered in DI — was falsely reporting "tds" based on user toggle - Commented out TDS Read Replica menu item that exposed non-functional feature - ExplainAsync now guards explicit TOP against MaxResultRows hint override, matching the existing guard in PrepareExecutionAsync Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ocking When running in F5 development mode, the daemon now copies build output to a temp directory before spawning. This prevents dotnet build from failing with "file in use" errors while the daemon holds locks on the original binaries. The temp directory is cleaned up on dispose. Also update /gates with file-locking recovery: if solution build fails due to locked files, retry with individual changed projects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move scripts/*.js → tools/*.mjs (ESM, consistent with webview-cdp.mjs) - Rename tests/tui-e2e/ → tests/PPDS.Tui.E2eTests/ (matches PPDS.*.Tests convention) - Move docs/specs/query-engine-v2.md → specs/ (one spec directory, not two) - Remove empty docs/specs/ and scripts/ directories - Update all references: package.json, CI workflow, skills, specs, CONTRIBUTING Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- specs/README.md: remove dead IMPLEMENTATION_PLAN.md link, fix broken vault relative path - specs/query-parity.md: fix AC-27 contradiction (InjectTopAttribute is retained, not deleted) - CLAUDE.md: remove stale docs/superpowers/plans/ warning - PrefetchScanNodeTests: remove ConfigureAwait(false) (xUnit1030) - FetchXmlGeneratorTests: discard unused expectedAttr param (xUnit1026) - webview-cdp skill: remove stale agent-browser reference Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…implementation plan - Add Phase 5 to specs/query-parity.md covering DI registration, execution mode reporting, TDS failure behavior (fail, don't fall back), and UI updates - Add implementation plan at docs/plans/2026-03-16-tds-endpoint-wiring.md - 18 acceptance criteria (AC-30 through AC-48) - Design decision: fail with clear error when TDS can't be used, never silently substitute Dataverse Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… DI registration Chunks 1-3 of TDS Endpoint Wiring plan: - Add ErrorCodes.Query.TdsIncompatible and TdsConnectionFailed - Add QueryErrorCode.TdsIncompatible and TdsConnectionFailed (Dataverse layer) - Create QueryExecutionMode enum (Dataverse, Tds) - Add ExecutionMode property to SqlQueryResult and SqlQueryStreamChunk - Fix TdsQueryExecutor to throw QueryExecutionException (D4 compliance) - Add TDS compatibility pre-check in SqlQueryService.PrepareExecutionAsync - Set ExecutionMode from plan tree in ExecuteAsync and ExecuteStreamingAsync - Catch TDS connection failures and wrap in PpdsException(TdsConnectionFailed) - Register ITdsQueryExecutor in daemon DI (per-environment singleton) - Register ITdsQueryExecutor in CLI DI (transient, same auth pattern) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…fix TUI status Chunk 4 of TDS Endpoint Wiring plan: - RpcMethodHandler maps SqlQueryResult.ExecutionMode to queryMode RPC field - Add TdsIncompatible and TdsConnectionFailed catch clauses in RPC error chain - Un-comment TDS Read Replica menu item in VS Code query panel - TUI status label shows "via TDS" or "via Dataverse" from streaming chunk Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- AC-38: verify ExecutionMode.Tds maps to queryMode "tds" in RPC response - AC-38: verify queryMode JSON serialization for tds/dataverse/null - AC-44: verify streaming final chunk carries ExecutionMode - AC-44: verify non-final chunks have null ExecutionMode - AC-34/35: verify ExecutionMode set correctly for TDS and Dataverse routes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TUI was missing DML confirmation handling — when SqlQueryService threw PpdsException(DmlConfirmationRequired), it fell through to the generic error handler showing raw "requires --confirm" message with no way to confirm. Now catches DmlConfirmationRequired, shows a confirmation dialog, and retries with IsConfirmed=true if user chooses "Execute". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This is a massive and impressive pull request that rebuilds the VS Code extension from the ground up with a new daemon architecture. The changes include significant improvements to the development process, with new AI skills for quality gates, verification, and design. The code itself is being migrated to a more robust structure with TypeScript webviews and typed messaging. My review focuses on ensuring the new infrastructure is sound and consistent. I've found a few minor areas for improvement, mostly in the documentation and scripts.
…-check and connection failure tests - Extract duplicated token provider wiring from DaemonConnectionPoolManager and ServiceRegistration into shared TdsQueryExecutorFactory.Create() - Add 4 missing tests from TDS implementation plan: - ExecuteAsync_TdsWithDml_ThrowsTdsIncompatible - ExecuteAsync_TdsWithIncompatibleEntity_ThrowsTdsIncompatible - ExecuteAsync_NoTds_DmlDoesNotThrowTdsIncompatible - ExecuteAsync_TdsConnectionFails_ThrowsTdsConnectionFailed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Regenerate package-lock.json to match package.json after rebase (fixes npm ci failure in CI). Reduce Linux status line from two node processes to one, using basename for directory extraction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comprehensive spec covering 6 panels (Import Jobs, Connection References, Environment Variables, Plugin Traces, Metadata Browser, Web Resources) across all 4 PPDS surfaces (Daemon RPC, VS Code, TUI, MCP). Includes 73 acceptance criteria, issue reconciliation mapping 27 existing issues, and phased work breakdown with worktree strategy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add typecheck step to CI build and publish workflows (esbuild doesn't type-check, so type errors could slip through) - Fix stale extension path in Claude pre-commit hook (src/extension → src/PPDS.Extension) - Propagate OperationCanceledException in QueryExplainAsync instead of swallowing it in bare catch - Clear ExplainDocumentProvider singleton on dispose to prevent stale reference after extension reload - Add escapeHtml/escapeAttr to context menu rendering for defense-in-depth XSS protection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add extension/TS paths to ci-gate.yml so extension-only PRs trigger required status checks - Make discovery cache thread-safe with Volatile.Read/Write and store-list-before-expiry ordering - Add 3-failure threshold to daemon heartbeat before kill to tolerate transient network hiccups - Fix E2E smoke test: replace vacuous if-guard with explicit assertion, correct view container selector ID - Wrap sync-over-async RemoteExecutorFactory in Task.Run to prevent thread pool starvation on cache miss Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Complete rebuild of the VS Code extension from the ground up, replacing the self-contained approach with a thin UI layer that delegates all operations to the
ppds servedaemon via JSON-RPC.Architecture
All business logic stays in Application Services. The extension is UI-only.
Not yet ported from legacy (tracked for 0.6.0 stable)
Daemon-side changes
Test plan
.vsixlocally and verify extension activates.ppdsnbnotebook, execute SQL query, verify results rendernpm testin extension/)vsce package --pre-releaseproduces clean.vsix🤖 Generated with Claude Code