feat: replace AJV with vscode-json-languageservice - #474

packages/codemirror/__mocks__/@sqlrooms/utils.ts (1)

1-2: Update the file header comment for accuracy.

Line 2 says only the React rendering function is mocked, but this file also mocks getMonospaceFont.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/codemirror/__mocks__/`@sqlrooms/utils.ts around lines 1 - 2, Update
the file header comment to accurately reflect what is mocked: mention that both
the React rendering function and getMonospaceFont are mocked (not just the React
rendering function). Reference the mock file
packages/codemirror/__mocks__/@sqlrooms/utils.ts and the exported symbol
getMonospaceFont (and the React render function) so the header clearly states
both exports are mocked and the rest are no-ops.

packages/codemirror/src/utils/validate-json-schema.ts (2)

92-106: Use DiagnosticSeverity enum values instead of magic numbers.

The convertSeverity function uses literal numbers (1, 2, 3, 4) instead of the imported DiagnosticSeverity enum, reducing readability and maintainability.

♻️ Proposed fix to use enum values

 function convertSeverity(
   vsSeverity: DiagnosticSeverity | undefined,
 ): Diagnostic['severity'] {
   switch (vsSeverity) {
-    case 1:
+    case DiagnosticSeverity.Error:
       return 'error';
-    case 2:
+    case DiagnosticSeverity.Warning:
       return 'warning';
-    case 3:
+    case DiagnosticSeverity.Information:
       return 'info';
-    case 4:
+    case DiagnosticSeverity.Hint:
       return 'hint';
     default:
       return 'warning';
   }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/codemirror/src/utils/validate-json-schema.ts` around lines 92 - 106,
The convertSeverity function currently compares vsSeverity to magic numbers
(1-4); replace those numeric literals with the corresponding DiagnosticSeverity
enum members (DiagnosticSeverity.Error, DiagnosticSeverity.Warning,
DiagnosticSeverity.Information, DiagnosticSeverity.Hint) so the switch checks
enum values and returns the same string mapping ('error', 'warning', 'info',
'hint'), and keep the default return as 'warning'; update the switch cases in
convertSeverity to use the DiagnosticSeverity enum identifiers to improve
readability and maintainability.

---

57-62: React root leak in renderMessage callbacks.

Each diagnostic's renderMessage callback creates a new React root via renderComponentToDomElement. These roots are never unmounted when CodeMirror removes the diagnostic tooltips. With many diagnostics (e.g., a malformed JSON file), this could accumulate orphaned roots.

This is consistent with the pattern used elsewhere in this PR, but worth noting for awareness.

Also applies to: 76-80

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/codemirror/src/utils/validate-json-schema.ts` around lines 57 - 62,
renderMessage currently creates a new React root for each diagnostic by calling
renderComponentToDomElement(DiagnosticTooltip, ...) and never unmounts it,
leaking roots; change renderMessage to create the React root into the tooltip
DOM element and return a cleanup/unmount callback (or reuse a single root) so
the React tree is unmounted when CodeMirror disposes the tooltip; update both
occurrences (renderMessage at lines shown and the similar block around 76-80) to
call the appropriate unmount method (or keep a shared root) for the
DiagnosticTooltip instead of leaving roots orphaned.

packages/codemirror/src/utils/create-json-schema-validator.ts (1)

11-22: Consider adding a timeout for external schema fetches.

The schemaRequestService uses fetch without a timeout or AbortSignal. If an external schema URL is slow or unresponsive, validation could hang indefinitely.

⏱️ Proposed fix to add fetch timeout

     schemaRequestService: async (uri: string) => {
       try {
-        const response = await fetch(uri);
+        const controller = new AbortController();
+        const timeoutId = setTimeout(() => controller.abort(), 10000); // 10s timeout
+        try {
+          const response = await fetch(uri, { signal: controller.signal });
+          clearTimeout(timeoutId);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/codemirror/src/utils/create-json-schema-validator.ts` around lines
11 - 22, The schemaRequestService implementation uses fetch without a timeout,
so slow/unresponsive schema URIs can hang validation; modify the
schemaRequestService async function to create an AbortController, start a timer
(e.g., timeoutMs constant or parameter) that calls controller.abort() after the
timeout, pass controller.signal to fetch(uri, { signal: controller.signal }),
clear the timer on success, and convert AbortError to a clear error message
before throwing; update references to schemaRequestService in
create-json-schema-validator to ensure the timeout is applied for all external
schema fetches.

packages/codemirror/__tests__/vega-schema-integration.test.ts (1)

7-13: Network-dependent test may be flaky in CI.

The test fetches the Vega-Lite schema from an external URL on every run. This introduces network dependency that could cause intermittent test failures in restricted environments or during GitHub outages.

Consider caching the schema locally or mocking the fetch in CI, while keeping the real fetch for local development verification.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/codemirror/__tests__/vega-schema-integration.test.ts` around lines 7
- 13, The beforeAll block currently fetches the Vega-Lite schema from the
network via fetch into the schema variable, which makes the test flaky in CI;
change the test to use a deterministic local fixture or a mocked fetch: add a
committed JSON fixture of v5.json and in the test file load that fixture (or
conditionally replace global fetch with a jest mock that returns the fixture
when process.env.CI is set), keeping the existing fetch path for local
development if desired; update the beforeAll to read the local file or use the
mocked response for fetch so tests no longer depend on external network calls.

packages/sql-editor/src/codemirror/extensions/completion.ts (1)

52-60: Potential React root memory leak with tooltip DOM elements.

Each completion item creates a React root via renderComponentToDomElement that is never unmounted when CodeMirror discards the tooltip. While the utility documents this as intentional, frequent autocomplete usage could accumulate orphaned React roots.

Consider whether the completion frequency warrants a cleanup mechanism, or whether a simpler non-React approach (plain DOM or HTML string) might be more appropriate for transient tooltips.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sql-editor/src/codemirror/extensions/completion.ts` around lines 52
- 60, The tooltip DOM returned by info() currently uses
renderComponentToDomElement to mount a React root for FunctionDocumentation (via
renderComponentToDomElement) but never unmounts it, leaking roots when
CodeMirror discards tooltips; modify the completion creation so either (A) you
avoid mounting a persistent React root by returning a plain DOM/HTML string from
info() instead of renderComponentToDomElement, or (B) keep a reference to the
mounted root/node and ensure you call the React unmount API (e.g.,
root.unmount() or ReactDOM.unmountComponentAtNode) when CodeMirror removes the
tooltip (hook into the tooltip/tooltipClose or completion UI lifecycle),
updating the code paths around renderComponentToDomElement and the info() return
to perform this cleanup.

packages/codemirror/src/utils/validate-json-schema.ts (1)

92-106: Use DiagnosticSeverity enum constants for clarity.

Magic numbers reduce readability. The DiagnosticSeverity enum is already imported.

♻️ Proposed refactor

 function convertSeverity(
   vsSeverity: DiagnosticSeverity | undefined,
 ): Diagnostic['severity'] {
   switch (vsSeverity) {
-    case 1:
+    case DiagnosticSeverity.Error:
       return 'error';
-    case 2:
+    case DiagnosticSeverity.Warning:
       return 'warning';
-    case 3:
+    case DiagnosticSeverity.Information:
       return 'info';
-    case 4:
+    case DiagnosticSeverity.Hint:
       return 'hint';
     default:
       return 'warning';
   }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/codemirror/src/utils/validate-json-schema.ts` around lines 92 - 106,
The switch in convertSeverity uses numeric magic values; replace the case labels
1,2,3,4 with the corresponding DiagnosticSeverity enum constants (e.g.,
DiagnosticSeverity.Error, DiagnosticSeverity.Warning,
DiagnosticSeverity.Information, DiagnosticSeverity.Hint) so the mapping to
Diagnostic['severity'] is clear and maintain the existing default return
'warning'; update only the case labels in convertSeverity to use these enum
members.