Add support for commands in diagnostic providers#61
Conversation
🦋 Changeset detectedLatest commit: b16664c The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR extends Lynx diagnostic providers to support command-based quick fixes by renaming DiagnosticFix to DiagnosticAction, adding an optional command field, and routing command execution through Workspace/DiagnosticProvider.executeCommand. It also tightens Workspace behavior by throwing on invalid diagnostics and adds substantial end-user/developer documentation.
Changes:
- Rename the fix model from
DiagnosticFixtoDiagnosticAction, and allow actions to be edit-based and/or command-based. - Add provider-declared commands plus
executeCommand()toDiagnosticProvider, and addexecuteDiagnosticActionCommand()plus command enumeration toWorkspace. - Update the VS Code language server and example provider(s), and add new docs/README content plus tests.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vscode/src/server.ts | Advertises provider commands to the client, generates command-based code actions, and dispatches execute-command requests to the workspace. |
| packages/punctuation-checker/test/quotation/quotation-checker.test.ts | Updates tests to call getDiagnosticActions instead of getDiagnosticFixes. |
| packages/punctuation-checker/test/punctuation-context/punctuation-context-checker.test.ts | Updates tests to call getDiagnosticActions. |
| packages/punctuation-checker/test/paired-punctuation/paired-punctuation-checker.test.ts | Updates tests to call getDiagnosticActions. |
| packages/punctuation-checker/test/allowed-character/allowed-character-checker.test.ts | Updates tests to call getDiagnosticActions. |
| packages/punctuation-checker/test/abstract-checker.test.ts | Updates test scaffolding types from DiagnosticFix to DiagnosticAction. |
| packages/punctuation-checker/src/quotation/quotation-checker.ts | Updates checker fix return types to DiagnosticAction. |
| packages/punctuation-checker/src/punctuation-context/punctuation-context-checker.ts | Updates checker fix return types to DiagnosticAction. |
| packages/punctuation-checker/src/paired-punctuation/paired-punctuation-checker.ts | Updates checker fix return types to DiagnosticAction. |
| packages/punctuation-checker/src/fixes/standard-fixes.ts | Updates standard fix helpers to return DiagnosticAction. |
| packages/punctuation-checker/src/allowed-character/allowed-character-checker.ts | Updates checker fix return types to DiagnosticAction. |
| packages/punctuation-checker/src/abstract-checker.ts | Implements new DiagnosticProvider surface (commands, getDiagnosticActions, executeCommand). |
| packages/examples/src/verse-order-diagnostic-provider.ts | Demonstrates command-based diagnostic actions via excludeVerse and implements executeCommand. |
| packages/examples/src/locales/es/verse-order.json | Adds localized title for the new exclude-verse action. |
| packages/examples/src/locales/en/verse-order.json | Adds localized title for the new exclude-verse action. |
| packages/core/src/workspace/workspace.ts | Adds getDiagnosticActions, command enumeration/execution, and throws on invalid diagnostics. |
| packages/core/src/workspace/workspace.test.ts | Updates dismissal behavior test and adds tests for executeDiagnosticActionCommand. |
| packages/core/src/diagnostic/index.ts | Re-exports DiagnosticAction and removes DiagnosticFix export. |
| packages/core/src/diagnostic/diagnostic-provider.ts | Extends DiagnosticProvider with commands, getDiagnosticActions, and executeCommand. |
| packages/core/src/diagnostic/diagnostic-action.ts | Defines the new DiagnosticAction interface with optional edits and command. |
| docs/new-document-format.md | Adds new documentation for implementing document formats. |
| docs/implementing-a-dismissal-store.md | Adds documentation for implementing a dismissal store. |
| docs/implementing-a-diagnostic-provider.md | Adds documentation for implementing diagnostic providers and command-based actions. |
| README.md | Expands the README with core concepts and usage examples reflecting the new APIs. |
| .changeset/cute-plums-work.md | Publishes a minor version bump for affected packages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await this.diagnosticDismissalStore.addDismissal(uri, diagnostic); | ||
|
|
||
| // Trigger a refresh on the provider that owns this diagnostic | ||
| const provider = this.diagnosticProviders.get(diagnostic.source); | ||
| if (provider != null) { | ||
| await provider.refresh(uri); | ||
| if (provider == null) { |
There was a problem hiding this comment.
dismissDiagnostic() writes the dismissal to diagnosticDismissalStore before verifying that a diagnostic provider exists for diagnostic.source. If the source is invalid, this method will throw after persisting a dismissal entry for a non-existent provider, leaving the store in an inconsistent state. Consider validating the provider first (or rolling back) before calling addDismissal().
| actions.push( | ||
| ...(await workspace.getDiagnosticFixes(params.textDocument.uri, lynxDiagnostic)).map((fix) => ({ | ||
| title: fix.title, | ||
| kind: CodeActionKind.QuickFix, | ||
| diagnostics: [diagnostic], | ||
| isPreferred: fix.isPreferred, | ||
| edit: { | ||
| changes: { | ||
| [params.textDocument.uri]: fix.edits, | ||
| }, | ||
| }, | ||
| })), | ||
| ...(await workspace.getDiagnosticActions(params.textDocument.uri, lynxDiagnostic)).map((lynxAction) => { | ||
| const action: CodeAction = { |
There was a problem hiding this comment.
workspace.getDiagnosticActions() now throws when diagnostic.source doesn’t match a registered provider. VS Code can include diagnostics from other sources in params.context.diagnostics, so this await can reject and cause the entire code action request to fail. Consider filtering to Lynx-owned diagnostics (e.g., known sources) or catching and ignoring provider-not-found errors per diagnostic.
| const command = params.command.split('.')[2]; | ||
| if (await workspace.executeDiagnosticActionCommand(command, uri, diagnostic)) { | ||
| connection.languages.diagnostics.refresh(); | ||
| } |
There was a problem hiding this comment.
This branch assumes params.command is always in the lynx.<provider>.<command> format and blindly reads split('.')[2]. If the command string is malformed (or contains extra . segments), command can be undefined/wrong and executeDiagnosticActionCommand() will throw, potentially failing the request. Consider validating the command prefix/shape (or parsing the last segment) and handling errors from executeDiagnosticActionCommand() to keep the server resilient.
| const command = params.command.split('.')[2]; | |
| if (await workspace.executeDiagnosticActionCommand(command, uri, diagnostic)) { | |
| connection.languages.diagnostics.refresh(); | |
| } | |
| const segments = params.command.split('.'); | |
| const command = segments[segments.length - 1]; | |
| if (!command) { | |
| connection.console.error(`Invalid diagnostic action command: "${params.command}"`); | |
| return; | |
| } | |
| try { | |
| if (await workspace.executeDiagnosticActionCommand(command, uri, diagnostic)) { | |
| connection.languages.diagnostics.refresh(); | |
| } | |
| } catch (error) { | |
| connection.console.error( | |
| `Error executing diagnostic action command "${command}" for "${params.command}": ${String(error)}` | |
| ); | |
| } |
| } | ||
| ``` | ||
|
|
||
| The command string must appear in `this.commands`. `Workspace` ignores commands not declared there. |
There was a problem hiding this comment.
The docs say “Workspace ignores commands not declared” in this.commands, but Workspace.executeDiagnosticActionCommand() actually throws when provider.commands does not contain the command. Please update this sentence to match the current behavior (or change the implementation to ignore).
| The command string must appear in `this.commands`. `Workspace` ignores commands not declared there. | |
| The command string must appear in `this.commands`. `Workspace` only routes diagnostic action commands declared in this set; attempting to execute a command not listed here will result in an error. |
|
|
||
| Called by `Workspace.dismissDiagnostic()` when the user dismisses a diagnostic. | ||
|
|
||
| - Store the dismissal based on `diagnostic.fingerprint`. If `diagnostic.fingerprint` is `null` or `undefined`, return without storing anything. |
There was a problem hiding this comment.
This contract says dismissal stores should silently no-op when diagnostic.fingerprint is null/undefined, but Workspace.dismissDiagnostic() now throws when fingerprint is missing. Consider updating this section to clarify that callers must only pass dismissible diagnostics (and that stores may not see null fingerprints via Workspace).
| - Store the dismissal based on `diagnostic.fingerprint`. If `diagnostic.fingerprint` is `null` or `undefined`, return without storing anything. | |
| - Store the dismissal based on `diagnostic.fingerprint`. Diagnostics without a fingerprint are not dismissible; `Workspace.dismissDiagnostic()` throws if `diagnostic.fingerprint` is `null` or `undefined`, so callers must only pass dismissible diagnostics and store implementations should not expect to see `null`/`undefined` fingerprints via `Workspace`. |
ddaspit
left a comment
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit made 2 comments.
Reviewable status: 0 of 25 files reviewed, 5 unresolved discussions.
| } | ||
| ``` | ||
|
|
||
| The command string must appear in `this.commands`. `Workspace` ignores commands not declared there. |
|
|
||
| Called by `Workspace.dismissDiagnostic()` when the user dismisses a diagnostic. | ||
|
|
||
| - Store the dismissal based on `diagnostic.fingerprint`. If `diagnostic.fingerprint` is `null` or `undefined`, return without storing anything. |
ddaspit
force-pushed
the
diagnostic-command
branch
from
d24a643 to
b16664c
Compare
DiagnosticFixtoDiagnosticActioncommandproperty toDiagnosticActionexecuteCommandtoDiagnosticProviderexecuteDiagnosticActionCommandtoWorkspaceWorkspacemethodsThis change is