From 41af83a8f6093c1c14ae562be41876080186d1cc Mon Sep 17 00:00:00 2001 From: Duane Nykamp Date: Thu, 30 Apr 2026 22:35:59 -0500 Subject: [PATCH 1/5] feat!: replace isAccessibleCallback with diagnosticsSummaryCallback Co-authored-by: Copilot --- .changeset/diagnostics-summary-callback.md | 9 +++ .../src/EditorViewer/EditorViewer.tsx | 34 +++++++++-- .../doenetml/src/EditorViewer/diagnostics.ts | 3 + packages/doenetml/src/doenetml.tsx | 8 ++- .../editorViewerAccessibilityReport.cy.js | 56 +++++++++++++++++-- packages/test-cypress/src/CypressTest.tsx | 21 +++---- 6 files changed, 110 insertions(+), 21 deletions(-) create mode 100644 .changeset/diagnostics-summary-callback.md diff --git a/.changeset/diagnostics-summary-callback.md b/.changeset/diagnostics-summary-callback.md new file mode 100644 index 000000000..732474b02 --- /dev/null +++ b/.changeset/diagnostics-summary-callback.md @@ -0,0 +1,9 @@ +--- +"@doenet/doenetml": patch +"@doenet/standalone": patch +"@doenet/doenetml-iframe": patch +"@doenet/vscode-extension": patch +"doenet-vscode-extension": patch +--- + +Replace `isAccessibleCallback` with `diagnosticsSummaryCallback` in `DoenetEditor`. The new callback receives an object with counts for `warningsCount`, `errorsCount`, `infosCount`, `accessibilityLevel1Count`, and `accessibilityLevel2Count` instead of a single boolean. The callback is only invoked after diagnostics have been received from the viewer. diff --git a/packages/doenetml/src/EditorViewer/EditorViewer.tsx b/packages/doenetml/src/EditorViewer/EditorViewer.tsx index 3d172b6f6..8f250b466 100644 --- a/packages/doenetml/src/EditorViewer/EditorViewer.tsx +++ b/packages/doenetml/src/EditorViewer/EditorViewer.tsx @@ -47,7 +47,7 @@ export function EditorViewer({ doenetmlChangeCallback, immediateDoenetmlChangeCallback, documentStructureCallback, - isAccessibleCallback, + diagnosticsSummaryCallback, id: specifiedId, readOnly = false, showFormatter = true, @@ -71,7 +71,9 @@ export function EditorViewer({ doenetmlChangeCallback?: Function; immediateDoenetmlChangeCallback?: Function; documentStructureCallback?: Function; - isAccessibleCallback?: (isAccessible: boolean) => void; + diagnosticsSummaryCallback?: ( + diagnosticsSummary: Record, + ) => void; id?: string; readOnly?: boolean; showFormatter?: boolean; @@ -131,11 +133,14 @@ export function EditorViewer({ const [infoPanelIsOpen, setInfoPanelIsOpen] = useState(false); const [diagnostics, setDiagnostics] = useState([]); + const [receivedDiagnosticsFromViewer, setReceivedDiagnosticsFromViewer] = + useState(false); const [showInfoAnnotations, setShowInfoAnnotations] = useState(false); /** Receives diagnostics from DocViewer and stores them for panel/LSP sync. */ function setDiagnosticsCallback(newDiagnostics: DiagnosticRecord[]) { setDiagnostics(newDiagnostics); + setReceivedDiagnosticsFromViewer(true); } useEffect(() => { @@ -155,6 +160,9 @@ export function EditorViewer({ errors: errorsObjs, infos: infoObjs, accessibility: accessibilityObjs, + warningsCount, + errorsCount, + infosCount, accessibilityLevel1Count, accessibilityLevel2Count, } = useMemo( @@ -167,8 +175,26 @@ export function EditorViewer({ ); useEffect(() => { - isAccessibleCallback?.(accessibilityLevel1Count === 0); - }, [accessibilityLevel1Count, isAccessibleCallback]); + if (!receivedDiagnosticsFromViewer) { + return; + } + + diagnosticsSummaryCallback?.({ + warningsCount, + errorsCount, + infosCount, + accessibilityLevel1Count, + accessibilityLevel2Count, + }); + }, [ + warningsCount, + errorsCount, + infosCount, + accessibilityLevel1Count, + accessibilityLevel2Count, + receivedDiagnosticsFromViewer, + diagnosticsSummaryCallback, + ]); const [responses, setResponses] = useState< { diff --git a/packages/doenetml/src/EditorViewer/diagnostics.ts b/packages/doenetml/src/EditorViewer/diagnostics.ts index f37d43e72..46ccc4842 100644 --- a/packages/doenetml/src/EditorViewer/diagnostics.ts +++ b/packages/doenetml/src/EditorViewer/diagnostics.ts @@ -145,6 +145,9 @@ export function mergeDiagnosticsByType({ errors, infos, accessibility, + warningsCount: warnings.length, + errorsCount: errors.length, + infosCount: infos.length, accessibilityLevel1Count: accessibility.filter( (diagnostic) => diagnostic.level === 1, ).length, diff --git a/packages/doenetml/src/doenetml.tsx b/packages/doenetml/src/doenetml.tsx index 331c18f01..5cae773b2 100644 --- a/packages/doenetml/src/doenetml.tsx +++ b/packages/doenetml/src/doenetml.tsx @@ -323,7 +323,7 @@ export function DoenetEditor({ doenetmlChangeCallback, immediateDoenetmlChangeCallback, documentStructureCallback, - isAccessibleCallback, + diagnosticsSummaryCallback, id, readOnly = false, showFormatter = true, @@ -353,7 +353,9 @@ export function DoenetEditor({ doenetmlChangeCallback?: Function; immediateDoenetmlChangeCallback?: Function; documentStructureCallback?: Function; - isAccessibleCallback?: (isAccessible: boolean) => void; + diagnosticsSummaryCallback?: ( + diagnosticsSummary: Record, + ) => void; id?: string; readOnly?: boolean; showFormatter?: boolean; @@ -425,7 +427,7 @@ export function DoenetEditor({ doenetmlChangeCallback={doenetmlChangeCallback} immediateDoenetmlChangeCallback={immediateDoenetmlChangeCallback} documentStructureCallback={documentStructureCallback} - isAccessibleCallback={isAccessibleCallback} + diagnosticsSummaryCallback={diagnosticsSummaryCallback} id={id} readOnly={readOnly} showFormatter={showFormatter} diff --git a/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js b/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js index 93d4017be..cca985c8a 100644 --- a/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js +++ b/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js @@ -89,7 +89,7 @@ describe( ); }); - it("isAccessibleCallback reports false when level 1 accessibility issues exist", () => { + it("diagnosticsSummaryCallback reports level 1 accessibility issue", () => { postDoenetML(` Low contrast text @@ -98,11 +98,14 @@ describe( cy.get("#p112").should("contain.text", "Low contrast text"); cy.window().should((win) => { - expect(win.returnIsAccessibleCallbackValue()).eq(false); + expect( + win.returnDiagnosticsSummaryCallbackValue() + .accessibilityLevel1Count, + ).eq(1); }); }); - it("isAccessibleCallback reports true when no level 1 accessibility issues exist", () => { + it("diagnosticsSummaryCallback reports no level 1 accessibility issue", () => { postDoenetML(` Good contrast text @@ -111,7 +114,52 @@ describe( cy.get("#p113").should("contain.text", "Good contrast text"); cy.window().should((win) => { - expect(win.returnIsAccessibleCallbackValue()).eq(true); + expect( + win.returnDiagnosticsSummaryCallbackValue() + .accessibilityLevel1Count, + ).eq(0); + }); + }); + + it("diagnosticsSummaryCallback reports warning", () => { + postDoenetML(` + hello + $abc +`); + + cy.get("#t").should("contain.text", "hello"); + + cy.window().should((win) => { + expect( + win.returnDiagnosticsSummaryCallbackValue().warningsCount, + ).eq(1); + expect( + win.returnDiagnosticsSummaryCallbackValue().errorsCount, + ).eq(0); + expect( + win.returnDiagnosticsSummaryCallbackValue().infosCount, + ).eq(0); + }); + }); + + it("diagnosticsSummaryCallback reports error", () => { + postDoenetML(` + hello + +`); + + cy.get("#t").should("contain.text", "hello"); + + cy.window().should((win) => { + expect( + win.returnDiagnosticsSummaryCallbackValue().warningsCount, + ).eq(0); + expect( + win.returnDiagnosticsSummaryCallbackValue().errorsCount, + ).eq(1); + expect( + win.returnDiagnosticsSummaryCallbackValue().infosCount, + ).eq(0); }); }); diff --git a/packages/test-cypress/src/CypressTest.tsx b/packages/test-cypress/src/CypressTest.tsx index 8cdd32c4b..14bf8755c 100644 --- a/packages/test-cypress/src/CypressTest.tsx +++ b/packages/test-cypress/src/CypressTest.tsx @@ -110,18 +110,17 @@ export function CypressTest() { const [includeVariantSelector, setIncludeVariantSelector] = useState( testSettings.includeVariantSelector, ); - const [isAccessible, setIsAccessible] = useState(null); - const isAccessibleRef = useRef(null); - isAccessibleRef.current = isAccessible; + const diagnosticsSummaryRef = useRef | null>(null); useEffect(() => { - (window as any).returnIsAccessibleCallbackValue = (): - | boolean - | null => { - return isAccessibleRef.current; + (window as any).returnDiagnosticsSummaryCallbackValue = (): Record< + string, + number + > | null => { + return diagnosticsSummaryRef.current; }; return () => { - delete (window as any).returnIsAccessibleCallbackValue; + delete (window as any).returnDiagnosticsSummaryCallbackValue; }; }, []); @@ -590,8 +589,10 @@ export function CypressTest() { showAnswerResponseButton={answerResponseCounts !== undefined} answerResponseCounts={answerResponseCounts} readOnly={readOnly} - isAccessibleCallback={(nextIsAccessible: boolean) => { - setIsAccessible(nextIsAccessible); + diagnosticsSummaryCallback={( + nextDiagnosticsSummary: Record, + ) => { + diagnosticsSummaryRef.current = nextDiagnosticsSummary; }} /> ); From 97f95679da7ec4f494c1d46d7fc4a449c9d0f699 Mon Sep 17 00:00:00 2001 From: Duane Nykamp Date: Thu, 30 Apr 2026 23:01:24 -0500 Subject: [PATCH 2/5] type Diagnostic summary, harden tests Co-authored-by: Copilot --- .../src/EditorViewer/EditorViewer.tsx | 3 +- .../doenetml/src/EditorViewer/diagnostics.ts | 17 +++++++++- packages/doenetml/src/doenetml.tsx | 3 +- .../editorViewerAccessibilityReport.cy.js | 32 +++++++------------ 4 files changed, 32 insertions(+), 23 deletions(-) diff --git a/packages/doenetml/src/EditorViewer/EditorViewer.tsx b/packages/doenetml/src/EditorViewer/EditorViewer.tsx index 8f250b466..99b2de84c 100644 --- a/packages/doenetml/src/EditorViewer/EditorViewer.tsx +++ b/packages/doenetml/src/EditorViewer/EditorViewer.tsx @@ -24,6 +24,7 @@ import { ViewerControlsBar } from "./ViewerControlsBar"; import "./editor-viewer.css"; import { useTabStore } from "@ariakit/react"; import { setVariantsFromCallback } from "../utils/variants"; +import type { DiagnosticsSummary } from "./diagnostics"; import { mergeDiagnosticsByType, toAdditionalDiagnosticsForLsp, @@ -72,7 +73,7 @@ export function EditorViewer({ immediateDoenetmlChangeCallback?: Function; documentStructureCallback?: Function; diagnosticsSummaryCallback?: ( - diagnosticsSummary: Record, + diagnosticsSummary: DiagnosticsSummary, ) => void; id?: string; readOnly?: boolean; diff --git a/packages/doenetml/src/EditorViewer/diagnostics.ts b/packages/doenetml/src/EditorViewer/diagnostics.ts index 46ccc4842..6cdb1b02f 100644 --- a/packages/doenetml/src/EditorViewer/diagnostics.ts +++ b/packages/doenetml/src/EditorViewer/diagnostics.ts @@ -1,4 +1,5 @@ import { + AccessibilityRecord, DiagnosticRecord, ErrorRecord, InfoRecord, @@ -13,6 +14,15 @@ import { /** CodeMirror/LSP diagnostic with optional editor mark class metadata. */ export type EditorLspDiagnostic = Diagnostic & { markClass?: string }; +/** Aggregated counts emitted for editor diagnostics callbacks. */ +export type DiagnosticsSummary = { + warningsCount: number; + errorsCount: number; + infosCount: number; + accessibilityLevel1Count: number; + accessibilityLevel2Count: number; +}; + /** * Converts a single editor diagnostic into an LSP-compatible diagnostic. * Accessibility diagnostics are surfaced as warnings with custom source/markClass. @@ -101,7 +111,12 @@ export function mergeDiagnosticsByType({ }: { initialDiagnostics: DiagnosticRecord[]; diagnostics: DiagnosticRecord[]; -}) { +}): { + warnings: WarningRecord[]; + errors: ErrorRecord[]; + infos: InfoRecord[]; + accessibility: AccessibilityRecord[]; +} & DiagnosticsSummary { const warnings = [ ...initialDiagnostics.filter( (diagnostic): diagnostic is WarningRecord => diff --git a/packages/doenetml/src/doenetml.tsx b/packages/doenetml/src/doenetml.tsx index 5cae773b2..d2584136e 100644 --- a/packages/doenetml/src/doenetml.tsx +++ b/packages/doenetml/src/doenetml.tsx @@ -4,6 +4,7 @@ import React, { useCallback, useEffect, useRef, useState } from "react"; import { DocViewer } from "./Viewer/DocViewer"; import { MathJaxContext } from "better-react-mathjax"; import { mathjaxConfig, isErrorRecord, isWarningRecord } from "@doenet/utils"; +import type { DiagnosticsSummary } from "./EditorViewer/diagnostics"; import type { DiagnosticRecord, ErrorRecord, @@ -354,7 +355,7 @@ export function DoenetEditor({ immediateDoenetmlChangeCallback?: Function; documentStructureCallback?: Function; diagnosticsSummaryCallback?: ( - diagnosticsSummary: Record, + diagnosticsSummary: DiagnosticsSummary, ) => void; id?: string; readOnly?: boolean; diff --git a/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js b/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js index cca985c8a..3c3406222 100644 --- a/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js +++ b/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js @@ -100,7 +100,7 @@ describe( cy.window().should((win) => { expect( win.returnDiagnosticsSummaryCallbackValue() - .accessibilityLevel1Count, + ?.accessibilityLevel1Count, ).eq(1); }); }); @@ -116,7 +116,7 @@ describe( cy.window().should((win) => { expect( win.returnDiagnosticsSummaryCallbackValue() - .accessibilityLevel1Count, + ?.accessibilityLevel1Count, ).eq(0); }); }); @@ -130,15 +130,11 @@ describe( cy.get("#t").should("contain.text", "hello"); cy.window().should((win) => { - expect( - win.returnDiagnosticsSummaryCallbackValue().warningsCount, - ).eq(1); - expect( - win.returnDiagnosticsSummaryCallbackValue().errorsCount, - ).eq(0); - expect( - win.returnDiagnosticsSummaryCallbackValue().infosCount, - ).eq(0); + const diagnosticsSummary = + win.returnDiagnosticsSummaryCallbackValue(); + expect(diagnosticsSummary?.warningsCount).eq(1); + expect(diagnosticsSummary?.errorsCount).eq(0); + expect(diagnosticsSummary?.infosCount).eq(0); }); }); @@ -151,15 +147,11 @@ describe( cy.get("#t").should("contain.text", "hello"); cy.window().should((win) => { - expect( - win.returnDiagnosticsSummaryCallbackValue().warningsCount, - ).eq(0); - expect( - win.returnDiagnosticsSummaryCallbackValue().errorsCount, - ).eq(1); - expect( - win.returnDiagnosticsSummaryCallbackValue().infosCount, - ).eq(0); + const diagnosticsSummary = + win.returnDiagnosticsSummaryCallbackValue(); + expect(diagnosticsSummary?.warningsCount).eq(0); + expect(diagnosticsSummary?.errorsCount).eq(1); + expect(diagnosticsSummary?.infosCount).eq(0); }); }); From 018c5cfc69a27e21548c303dbfd5809ee05a1e4c Mon Sep 17 00:00:00 2001 From: Duane Nykamp Date: Thu, 30 Apr 2026 23:23:32 -0500 Subject: [PATCH 3/5] fix: memoize diagnosticsSummary and reset ref on document change Addresses two PR review comments: 1. Memoize the diagnosticsSummary object in EditorViewer via useMemo so its reference is stable when counts don't change, avoiding potential render loops when consumers store the object in state. 2. Reset diagnosticsSummaryRef in CypressTest when doenetMLstring changes, ensuring stale summary values from previous test runs aren't returned to subsequent tests. --- .../src/EditorViewer/EditorViewer.tsx | 37 +++++++++++++------ packages/test-cypress/src/CypressTest.tsx | 5 +++ 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/packages/doenetml/src/EditorViewer/EditorViewer.tsx b/packages/doenetml/src/EditorViewer/EditorViewer.tsx index 99b2de84c..c81039e58 100644 --- a/packages/doenetml/src/EditorViewer/EditorViewer.tsx +++ b/packages/doenetml/src/EditorViewer/EditorViewer.tsx @@ -175,24 +175,37 @@ export function EditorViewer({ [initialDiagnostics, diagnostics], ); - useEffect(() => { - if (!receivedDiagnosticsFromViewer) { - return; - } - - diagnosticsSummaryCallback?.({ + /** + * Memoize the diagnosticsSummary object so its reference is stable when + * diagnostic counts don't change. This prevents unnecessary re-invocations + * of the effect callback and avoids potential render loops if a consumer + * stores this object in React state and passes an inline callback. + */ + const diagnosticsSummary = useMemo( + () => ({ warningsCount, errorsCount, infosCount, accessibilityLevel1Count, accessibilityLevel2Count, - }); + }), + [ + warningsCount, + errorsCount, + infosCount, + accessibilityLevel1Count, + accessibilityLevel2Count, + ], + ); + + useEffect(() => { + if (!receivedDiagnosticsFromViewer) { + return; + } + + diagnosticsSummaryCallback?.(diagnosticsSummary); }, [ - warningsCount, - errorsCount, - infosCount, - accessibilityLevel1Count, - accessibilityLevel2Count, + diagnosticsSummary, receivedDiagnosticsFromViewer, diagnosticsSummaryCallback, ]); diff --git a/packages/test-cypress/src/CypressTest.tsx b/packages/test-cypress/src/CypressTest.tsx index 14bf8755c..f649b4af6 100644 --- a/packages/test-cypress/src/CypressTest.tsx +++ b/packages/test-cypress/src/CypressTest.tsx @@ -124,6 +124,11 @@ export function CypressTest() { }; }, []); + // Reset diagnosticsSummaryRef when new DoenetML is posted to avoid stale values + useEffect(() => { + diagnosticsSummaryRef.current = null; + }, [doenetMLstring]); + const solutionDisplayMode = "button"; // requestedVariantIndex is undefined by default so that viewer From 7862f380d1062e4cffe6ab503b2f76423bbdac7a Mon Sep 17 00:00:00 2001 From: Duane Nykamp Date: Fri, 1 May 2026 09:11:12 -0500 Subject: [PATCH 4/5] add comments explaining guard Co-authored-by: Copilot --- packages/doenetml/src/EditorViewer/EditorViewer.tsx | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/doenetml/src/EditorViewer/EditorViewer.tsx b/packages/doenetml/src/EditorViewer/EditorViewer.tsx index c81039e58..2d0b2e698 100644 --- a/packages/doenetml/src/EditorViewer/EditorViewer.tsx +++ b/packages/doenetml/src/EditorViewer/EditorViewer.tsx @@ -134,6 +134,10 @@ export function EditorViewer({ const [infoPanelIsOpen, setInfoPanelIsOpen] = useState(false); const [diagnostics, setDiagnostics] = useState([]); + // Track whether we've received diagnostics from the viewer at least once. + // This is used to gate when we call the diagnosticsSummaryCallback to avoid sending + // just the initial diagnostics on load, which would be misleading since that + // does not include most of the diagnostics, which are generated by the viewer. const [receivedDiagnosticsFromViewer, setReceivedDiagnosticsFromViewer] = useState(false); const [showInfoAnnotations, setShowInfoAnnotations] = useState(false); @@ -199,6 +203,10 @@ export function EditorViewer({ ); useEffect(() => { + // On initial load of the editor, don't call `diagnosticsSummaryCallback` + // until the viewer has sent diagnostics so avoid sending just the initial diagnostics. + // We do not need to reset `receivedDiagnosticsFromViewer`, as this guard is just + // needed to avoid calling `diagnosticsSummaryCallback` on the initial load of the editor. if (!receivedDiagnosticsFromViewer) { return; } From 023d2d3600733f51c162ec52e999c6808ae3bdc2 Mon Sep 17 00:00:00 2001 From: Duane Nykamp Date: Fri, 1 May 2026 10:08:19 -0500 Subject: [PATCH 5/5] narrow to import type --- packages/doenetml/src/EditorViewer/diagnostics.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/doenetml/src/EditorViewer/diagnostics.ts b/packages/doenetml/src/EditorViewer/diagnostics.ts index 6cdb1b02f..d039d7d69 100644 --- a/packages/doenetml/src/EditorViewer/diagnostics.ts +++ b/packages/doenetml/src/EditorViewer/diagnostics.ts @@ -1,11 +1,11 @@ -import { +import type { AccessibilityRecord, DiagnosticRecord, ErrorRecord, InfoRecord, WarningRecord, - isAccessibilityRecord, } from "@doenet/utils"; +import { isAccessibilityRecord } from "@doenet/utils"; import { Diagnostic, DiagnosticSeverity,