Conversation
There was a problem hiding this comment.
Pull request overview
Adds a per-language diagnosticsOptions configuration surface to the built-in LSP integrations so consumers can disable validation and/or filter/ignore specific diagnostics (e.g., disable CSS validation while keeping completion/hover).
Changes:
- Introduces
diagnosticsOptions(validate / codesToIgnore / filter) and wires it through the LSP client’s diagnostics registration. - Extends HTML/CSS/JSON/TypeScript language setup to accept typed settings and pass diagnostics options; adds CSS
validPropertieshelper and richer HTML/CSS data provider typing. - Updates workspace document opening flow and adds new examples + README updates.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
types/lsp.d.ts |
Expands public LSP config/types (data providers, CSS valid properties, diagnosticsOptions) and related data model types. |
src/lsp/client.ts |
Adds diagnostics options handling (skip validation, filter markers, ignore codes). |
src/lsp/css/setup.ts |
Passes diagnostics options into basic features; adds validProperties → custom data provider wiring. |
src/lsp/css/worker.ts |
Renames CSS data config interface used by worker create data. |
src/lsp/html/setup.ts |
Adds typed HTML settings, supports custom data providers and diagnostics options. |
src/lsp/json/setup.ts |
Adds typed JSON settings and uses diagnostics options for validation + passes through to diagnostics registration. |
src/lsp/typescript/setup.ts |
Types TS language settings and passes diagnostics options to diagnostics registration; updates workspace model open call signature. |
src/workspace.ts |
Changes _openTextDocument to accept monaco explicitly; modifies openTextDocument editor selection logic. |
src/core.ts |
Updates internal calls to _openTextDocument to pass monaco. |
scripts/dev.ts |
Adds dev-server links for new examples. |
examples/with-diagnostics-options.html |
Demonstrates filtering/ignoring diagnostics and disabling validation via diagnosticsOptions. |
examples/css-valid-properites.html |
Demonstrates CSS validProperties usage. |
README.md |
Documents the new configuration options and provides usage examples. |
Comments suppressed due to low confidence (1)
src/lsp/json/setup.ts:37
validateis set fromdiagnosticsOptionsand then...languageSettingsis spread afterwards, which can overwritevalidate(e.g. iflanguageSettingsstill containsvalidatefor backward-compat). IfdiagnosticsOptions.validateis intended to be authoritative, spreadlanguageSettingsfirst and then setvalidatelast (or explicitly omitvalidatefrom the spread).
settings: {
validate: languageSettings?.diagnosticsOptions?.validate ?? true,
allowComments: false,
comments: "error",
trailingCommas: "error",
schemaRequest: "warning",
schemaValidation: "warning",
...languageSettings,
schemas,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/workspace.ts
Outdated
| const getEditor = async () => { | ||
| const editors = monaco.editor.getEditors(); | ||
| if (editors.length === 0) { | ||
| return new Promise<monacoNS.editor.ICodeEditor>((resolve) => setTimeout(() => resolve(getEditor()), 100)); | ||
| } | ||
| return editors[0]; | ||
| }; | ||
| return this._openTextDocument(monaco, uri, await getEditor(), undefined, content); |
There was a problem hiding this comment.
openTextDocument() now waits until at least one editor exists. Since _openTextDocument() already supports editor being undefined, this can cause openTextDocument() to hang forever in scenarios where no editor is created (e.g. headless usage / tests / custom embedding). Consider passing monaco.editor.getEditors()[0] directly (possibly undefined), or add a bounded timeout / explicit opt-in wait behavior.
| const getEditor = async () => { | |
| const editors = monaco.editor.getEditors(); | |
| if (editors.length === 0) { | |
| return new Promise<monacoNS.editor.ICodeEditor>((resolve) => setTimeout(() => resolve(getEditor()), 100)); | |
| } | |
| return editors[0]; | |
| }; | |
| return this._openTextDocument(monaco, uri, await getEditor(), undefined, content); | |
| const editors = monaco.editor.getEditors(); | |
| const editor = editors.length > 0 ? editors[0] : undefined; | |
| return this._openTextDocument(monaco, uri, editor, undefined, content); |
src/lsp/css/setup.ts
Outdated
| const validProperties = languageSettings?.validProperties; | ||
| if (validProperties) { | ||
| languageSettings.dataProviders = { | ||
| "valid-properties": { | ||
| version: 1.1, | ||
| properties: validProperties.map(property => ({ name: property })), |
There was a problem hiding this comment.
This block mutates the caller-provided languageSettings object by assigning to languageSettings.dataProviders. That can be surprising if the same settings object is reused across calls. Prefer building a local dataProviders object and passing it into createData without mutating the input.
src/lsp/css/setup.ts
Outdated
| version: 1.1, | ||
| properties: validProperties.map(property => ({ name: property })), | ||
| }, | ||
| ...(languageSettings.dataProviders as Record<string, unknown> | undefined), |
There was a problem hiding this comment.
Spreading languageSettings.dataProviders via as Record<string, unknown> weakens type safety and can mask non-object values at compile time. Since dataProviders is already typed as an object map, prefer ...(languageSettings.dataProviders ?? {}) (or clone first) to avoid the cast.
| ...(languageSettings.dataProviders as Record<string, unknown> | undefined), | |
| ...(languageSettings.dataProviders ?? {}), |
README.md
Outdated
| You can also set the diagnostics options for all languages by adding the `diagnosticsOptions` option to the `lsp` object: | ||
|
|
||
| ```js | ||
| lazy({ | ||
| lsp: { |
There was a problem hiding this comment.
The text says diagnostics options can be set "for all languages" by adding diagnosticsOptions to the lsp object, but the example only shows per-language diagnosticsOptions and the types don’t define a top-level lsp.diagnosticsOptions. Either adjust the wording to “for each language” (or similar), or add/implement a true global lsp.diagnosticsOptions that is applied by default.
scripts/dev.ts
Outdated
| Multiple themes - ${server.url}multi-theme | ||
| SSR mode - ${server.url}ssr | ||
| Compare modes - ${server.url}compare | ||
| CSS valid properties - ${server.url}css-valid-properites |
There was a problem hiding this comment.
Typo in example URL: css-valid-properites -> css-valid-properties (and rename the example file accordingly). Keeping the typo makes the dev server output and routes harder to discover/remember.
| CSS valid properties - ${server.url}css-valid-properites | |
| CSS valid properties - ${server.url}css-valid-properties |
src/lsp/css/worker.ts
Outdated
| /** Configures the CSS data types known by the langauge service. */ | ||
| readonly data?: CSSDataConfiguration; | ||
| readonly data?: CSSLanguageServiceOptions; |
There was a problem hiding this comment.
Spelling in comment: langauge -> language.
src/lsp/html/setup.ts
Outdated
| ) { | ||
| const { editor, languages } = monaco; | ||
| const { tabSize, insertSpaces, insertFinalNewline, trimFinalNewlines } = formattingOptions ?? {}; | ||
| const dataProviders = languageSettings?.dataProviders ?? {}; |
There was a problem hiding this comment.
dataProviders is assigned from languageSettings?.dataProviders and then mutated by adding custom-tags. This mutates the caller-provided object, which can create surprising side effects if the same settings object is reused. Consider cloning ({ ...languageSettings?.dataProviders }) before adding derived providers.
| const dataProviders = languageSettings?.dataProviders ?? {}; | |
| const dataProviders = { ...(languageSettings?.dataProviders ?? {}) }; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const editors = monaco.editor.getEditors(); | ||
| const editor = editors.find(e => e.hasWidgetFocus() || e.hasTextFocus()) ?? editors[0]; | ||
| if (editor) { | ||
| await workspace._openTextDocument(monaco, editor, uri); | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Host.openModel is invoked by the TypeScript worker during module resolution (imports). Calling workspace._openTextDocument(monaco, editor, uri) here will switch the visible editor model and push workspace history for every imported file, which is likely to cause unexpected navigation/focus changes while typing. openModel should create/sync the model without mutating any editor instance (e.g., allow _openTextDocument to accept editor as optional/undefined and skip editor.setModel/history updates, or add a dedicated internal method for background model creation).
| const editors = monaco.editor.getEditors(); | |
| const editor = editors.find(e => e.hasWidgetFocus() || e.hasTextFocus()) ?? editors[0]; | |
| if (editor) { | |
| await workspace._openTextDocument(monaco, editor, uri); | |
| return true; | |
| } | |
| return false; | |
| // Ensure the model for the given URI is created/synced without mutating any editor instance. | |
| await workspace._openTextDocument(monaco, undefined as never, uri); | |
| return true; |
src/lsp/html/setup.ts
Outdated
| // register language features | ||
| client.registerEmbedded(languageId, workerWithEmbeddedLanguages, ["css", "javascript", "importmap"]); | ||
| client.registerBasicFeatures(languageId, workerWithEmbeddedLanguages, ["<", "/", "=", '"'], workspace); | ||
| client.registerBasicFeatures( | ||
| languageId, | ||
| workerWithEmbeddedLanguages, | ||
| ["<", "/", "=", '"'], | ||
| workspace, | ||
| languageSettings?.diagnosticsOptions, | ||
| ); | ||
| client.registerAutoComplete(languageId, workerWithEmbeddedLanguages, [">", "/", "="]); | ||
| client.registerColorPresentation(languageId, workerWithEmbeddedLanguages); // css color presentation | ||
| client.registerDocumentLinks(languageId, workerWithEmbeddedLanguages); | ||
|
|
||
| // support embedded languages(css, javascript and importmap) in html files | ||
| client.registerEmbedded(languageId, workerWithEmbeddedLanguages, ["css", "javascript", "importmap"]); | ||
|
|
There was a problem hiding this comment.
registerBasicFeatures() triggers an immediate diagnostics run over existing models. Because registerEmbedded() is called afterwards, embedded models for css/javascript/importmap may not exist yet during the first validation pass, so initial embedded diagnostics can be missing until the user edits. Consider registering embedded-language support before enabling diagnostics, or explicitly triggering a refreshDiagnostics on the master HTML model after registerEmbedded has created/updated embedded models.
| async openTextDocument(uri: string | URL, content?: string, editor?: any): Promise<monacoNS.editor.ITextModel> { | ||
| const monaco = await this._monaco.promise; | ||
| return this._openTextDocument(uri, monaco.editor.getEditors()[0], undefined, content); | ||
| const getEditor = async () => { | ||
| const editors = monaco.editor.getEditors(); | ||
| const editor = editors.find(e => e.hasWidgetFocus() || e.hasTextFocus()) ?? editors[0]; | ||
| if (!editor) { | ||
| return new Promise<monacoNS.editor.ICodeEditor>((resolve) => setTimeout(() => resolve(getEditor()), 100)); | ||
| } | ||
| return editor; | ||
| }; | ||
| return this._openTextDocument(monaco, editor ?? await getEditor(), uri, undefined, content); |
There was a problem hiding this comment.
openTextDocument takes editor?: any, but the public typings expose this parameter as editor.ICodeEditor. Using any here removes type safety and makes it easy to pass the wrong shape. Please type this parameter as monacoNS.editor.ICodeEditor | undefined (or ICodeEditor | null if you want an explicit sentinel) and adjust downstream calls accordingly.
| @@ -418,17 +426,48 @@ export interface LSPConfig { | |||
| schemaValidation?: SeverityLevel; | |||
| /** The severity of problems that occurred when resolving and loading schemas. Default is "warning". */ | |||
| schemaRequest?: SeverityLevel; | |||
| /** Options for the diagnostics. */ | |||
| diagnosticsOptions?: DiagnosticsOptions; | |||
| }; | |||
There was a problem hiding this comment.
The README previously documented lsp.json.validate as the way to disable JSON validation, but the config shape shown here now only exposes json.diagnosticsOptions.validate. If this is an intentional breaking change, it should be called out with a short migration note (or keep json.validate documented as a deprecated alias) to avoid silently breaking existing integrations.
this pr adds feature allow you to set the diagnostics options for all languages by adding the
diagnosticsOptionsoption to thelspoptions:close #57