From 40c1dfa9da4d9d149b64725ef032095f9e7f4752 Mon Sep 17 00:00:00 2001 From: "f.albany" Date: Wed, 4 Nov 2026 14:09:58 +0100 Subject: [PATCH] feat(editor): clear stale feedback on edit + debounced live validation (#102 #100) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One PR per the review plan: the clearing fix (#102) is the first three lines of the same update that then prolongs into live validation (#100) — splitting them would publish a misleading intermediate state. ## Issue #102 — editing clears stale feedback The document-change listener now also drops `status` and `issues` the moment the doc changes: a Test/Save banner or issue list can never outlive the content it described. ## Issue #100 — debounced live validation `VALIDATION_DEBOUNCE_MS = 800` (deliberately not a fixed 2s poll): - Every doc change schedules one deferred `Test` cycle 800ms after the last change; rapid typing coalesces (pending timer is canceled and re-scheduled). - No-ops elided: text identical to the last validation request (`lastValidatedTextRef`) is not re-validated; nothing runs while the compliance gate or the Save As dialog is parked (`parkedRef`). - In-flight coexistence: if a validation round trip is still flying at fire time, the schedule RESCHEDULES instead of disappearing — the latest text is validated once it lands. - The explicit Test button cancels the pending timer and validates immediately (records its text in the dedupe ref like every cycle). - Hygiene: pending timers are canceled on view destroy/unmount — a pre-existing failing test (`test_save_invalid_shows_gate_with_force_option`, watch-listed incl. 5 spurious validate calls from just such leaked timers) proved the regression risk; the TDD round surfaced and fixed it. - No backend change: `POST /api/config/validate` is stateless. ## TDD evidence - RED: 5 of 6 new tests failed for the right reasons (no clearing, no debounce timing, no gate skip, no in-flight re-check). The sixth (`test_explicit_test_cancels_pending_debounce`) passed vacuously and only becomes a true guard post-implementation — kept as a regression pin, noted honestly. - Two test-side bugs fixed during the cycle themselves: clawed the coalesce-timing arithmetic, scoped the exact Save button name. - GREEN: ConfigEditorModal.test.tsx 29/29, full web suite 223/223 (14 files), `tsc -b` clean, pre-commit hooks green. - `dist/` intentionally NOT rebuilt: open PR #107 rebuilds it too; the last branch merged onto feat/yaml-config-editor should re-run vite build (CI does per repo packaging hook). Closes #102, closes #100 --- .../src/components/ConfigEditorModal.test.tsx | 152 ++++++++++++++++++ .../web/src/components/ConfigEditorModal.tsx | 85 +++++++++- 2 files changed, 236 insertions(+), 1 deletion(-) diff --git a/src/profiles/web/src/components/ConfigEditorModal.test.tsx b/src/profiles/web/src/components/ConfigEditorModal.test.tsx index 2208a86..1836b20 100644 --- a/src/profiles/web/src/components/ConfigEditorModal.test.tsx +++ b/src/profiles/web/src/components/ConfigEditorModal.test.tsx @@ -755,3 +755,155 @@ describe("ConfigEditorModal", () => { expect(saveSpy).not.toHaveBeenCalled(); }); }); + +/** + * Editor feedback tests (issues #102 + #100): editing must clear stale + * Test/validation feedback immediately, and validation must re-run + * automatically ~800ms after the user stops typing. + */ +describe("ConfigEditorModal live validation (issues #102/#100)", () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it("test_editing_clears_stale_validation_feedback", async () => { + validateSpy.mockResolvedValue({ valid: false, issues: [{ severity: "error", message: "boom" }] }); + const user = userEvent.setup(); + renderModal(); + + const view = await readyEditorView(); + // Run Test: the banner + issue list describe the pre-edit document. + await user.click(screen.getByRole("button", { name: /test/i })); + await waitFor(() => expect(screen.getByTestId("editor-status")).toBeTruthy()); + expect(screen.getByTestId("editor-issues")).toBeTruthy(); + + // One keystroke invalidates that feedback — it must vanish immediately. + await act(async () => { + view.dispatch({ changes: { from: 0, insert: "# edited\n" } }); + }); + expect(screen.queryByTestId("editor-status")).toBeNull(); + expect(screen.queryByTestId("editor-issues")).toBeNull(); + }); + + it("test_live_validation_fires_after_debounce", async () => { + renderModal(); + const view = await readyEditorView(); + // Freeze time only after the editor is ready: RTL waitFor relies on + // real timers while probing the mounted view. + vi.useFakeTimers(); + validateSpy.mockResolvedValue({ valid: true, issues: [] }); + + await act(async () => { + view.dispatch({ changes: { from: 0, insert: "# edited\n" } }); + }); + // A pause shorter than the debounce fires nothing. + await act(async () => { + await vi.advanceTimersByTimeAsync(799); + }); + expect(validateSpy).not.toHaveBeenCalled(); + + // At 800ms after the last change, exactly one validation leaves — + // carrying the current document text. + await act(async () => { + await vi.advanceTimersByTimeAsync(1); + }); + expect(validateSpy).toHaveBeenCalledTimes(1); + expect(validateSpy.mock.calls[0][1]).toContain("# edited\n"); + }); + + it("test_live_validation_coalesces_rapid_typing", async () => { + renderModal(); + const view = await readyEditorView(); + vi.useFakeTimers(); + validateSpy.mockResolvedValue({ valid: true, issues: [] }); + + // Three changes inside the debounce window coalesce into one request. + await act(async () => { + view.dispatch({ changes: { from: 0, insert: "a\n" } }); + await vi.advanceTimersByTimeAsync(300); + view.dispatch({ changes: { from: 1, insert: "b\n" } }); + await vi.advanceTimersByTimeAsync(300); + view.dispatch({ changes: { from: 2, insert: "c\n" } }); + await vi.advanceTimersByTimeAsync(800); + }); + expect(validateSpy).toHaveBeenCalledTimes(1); + expect(validateSpy.mock.calls[0][1]).toContain("c\n"); + }); + + it("test_explicit_test_cancels_pending_debounce", async () => { + renderModal(); + const view = await readyEditorView(); + vi.useFakeTimers(); + validateSpy.mockResolvedValue({ valid: true, issues: [] }); + + await act(async () => { + view.dispatch({ changes: { from: 0, insert: "# edited\n" } }); + }); + // The explicit Test button short-circuits the pending debounce. + await act(async () => { + screen.getByRole("button", { name: /test/i }).click(); + await vi.advanceTimersByTimeAsync(0); + }); + expect(validateSpy).toHaveBeenCalledTimes(1); + + // The canceled timer must not re-fire later. + await act(async () => { + await vi.advanceTimersByTimeAsync(1000); + }); + expect(validateSpy).toHaveBeenCalledTimes(1); + }); + + it("test_live_validation_rechecks_after_in_flight_completes", async () => { + renderModal(); + const view = await readyEditorView(); + vi.useFakeTimers(); + let resolveFirst!: (value: { valid: boolean; issues: never[] }) => void; + validateSpy.mockImplementationOnce( + () => new Promise((resolve) => { resolveFirst = resolve; }), + ); + + await act(async () => { + view.dispatch({ changes: { from: 0, insert: "x\n" } }); + await vi.advanceTimersByTimeAsync(800); + }); + expect(validateSpy).toHaveBeenCalledTimes(1); // in flight + + // A second edit while validation is in flight must not be swallowed — + // it re-schedules and re-fires once the round trip lands. + await act(async () => { + view.dispatch({ changes: { from: 0, insert: "y\n" } }); + await vi.advanceTimersByTimeAsync(800); + }); + expect(validateSpy).toHaveBeenCalledTimes(1); + + await act(async () => { + resolveFirst({ valid: true, issues: [] }); + await vi.advanceTimersByTimeAsync(800); + }); + expect(validateSpy).toHaveBeenCalledTimes(2); + }); + + it("test_live_validation_skipped_while_gate_parked", async () => { + // The compliance gate (invalid save confirm) is a blocking modal: no + // auto-validation churn behind it. + validateSpy.mockResolvedValue({ + valid: false, + issues: [{ severity: "error", message: "boom" }], + }); + const user = userEvent.setup(); + renderModal(); + const view = await readyEditorView(); + + await user.click(screen.getByRole("button", { name: /^save$/i })); + expect(await screen.findByRole("dialog", { name: /save with validation issues\?/i })).toBeTruthy(); + + vi.useFakeTimers(); + validateSpy.mockResolvedValue({ valid: true, issues: [] }); + await act(async () => { + view.dispatch({ changes: { from: 0, insert: "# edited\n" } }); + await vi.advanceTimersByTimeAsync(2000); + }); + // Only the save pre-validation ran — no live cycle behind the gate. + expect(validateSpy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/profiles/web/src/components/ConfigEditorModal.tsx b/src/profiles/web/src/components/ConfigEditorModal.tsx index 3475f19..ccba619 100644 --- a/src/profiles/web/src/components/ConfigEditorModal.tsx +++ b/src/profiles/web/src/components/ConfigEditorModal.tsx @@ -51,6 +51,9 @@ type PendingAction = "new" | "revert"; /** Kinds of the async toolbar actions — the dispatch runner's key. */ type ToolbarActionKind = "new" | "test" | "revert" | "apply" | "save" | "save-as"; +/** Debounce window for live validation after the last document change (issue #100). */ +const VALIDATION_DEBOUNCE_MS = 800; + /** Target chosen in the Save As dialog, handed back to the save-as flow. */ type SaveAsTarget = { filename: string; directory: string; overwrite: boolean }; @@ -181,6 +184,25 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa const actionSeqRef = useRef(0); /** In-flight registry: action kind → seq of its live dispatch. A kind already registered rejects duplicate dispatches (double-click Apply fires one request). */ const inFlightActionsRef = useRef>(new Map()); + /** Pending live-validation timer (issue #100); null when idle. */ + const validationTimerRef = useRef(null); + /** + * Doc text of the most recent validation request. The live cycle is + * scheduled only for texts differing from it — coalescing keystrokes + * into one request, and no-op revalidation of an unchanged document. + */ + const lastValidatedTextRef = useRef(null); + /** + * True while a blocking dialog (compliance gate / Save As) sits behind + * the editor: no auto-validation churn behind a modal. Latest-ref so + * the CodeMirror listener closure (mounted once per doc) reads live. + */ + const parkedRef = useRef(false); + /** Latest-run indirection for the listener: always dispatches current. */ + const runValidationRef = useRef<() => void>(() => {}); + /** Latest ref for the scheduling helper itself (see updateListener). */ + const scheduleValidationRef = useRef<(text: string) => void>(() => {}); + parkedRef.current = gate !== null || saveAsPrompt !== null; const handleDialogKeyDown = useModalA11y({ isOpen: open, @@ -201,6 +223,13 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa setPendingAction(null); setGate(null); setSaveAsPrompt(null); + // Drop any pending live-validation cycle (issue #100): a stale timer + // must not fire into a closed session. + if (validationTimerRef.current !== null) { + clearTimeout(validationTimerRef.current); + validationTimerRef.current = null; + } + lastValidatedTextRef.current = null; // Invalidate in-flight toolbar actions so a resolution that lands // after close/reopen cannot leak into the next session's state. actionSeqRef.current += 1; @@ -213,6 +242,7 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa if (cancelled) return; const content = toEditorText(data.content); loadedTextRef.current = content; + lastValidatedTextRef.current = content; setLoad({ status: "ready", data: { ...data, content } }); }) .catch((err: unknown) => { @@ -225,6 +255,40 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa }; }, [open]); + /** Drop any pending live-validation cycle. */ + const cancelPendingValidation = useCallback(() => { + if (validationTimerRef.current !== null) { + clearTimeout(validationTimerRef.current); + validationTimerRef.current = null; + } + }, []); + + /** + * Schedule a live validation (issue #100): one deferred cycle + * ``VALIDATION_DEBOUNCE_MS`` after the last change. No-ops are elided: + * text identical to the last validation (coalesced typing), and the + * parked state (no auto-validation churn behind a modal). A cycle + * already in flight rejects duplicate dispatches — in that case the + * schedule RESCHEDULES instead of disappearing, so the latest text is + * still validated after the round trip lands. + */ + const scheduleLiveValidation = useCallback( + (text: string) => { + if (parkedRef.current) return; + if (text === lastValidatedTextRef.current) return; + cancelPendingValidation(); + validationTimerRef.current = window.setTimeout(() => { + validationTimerRef.current = null; + if (inFlightActionsRef.current.has("test")) { + scheduleValidationRef.current(text); + return; + } + runValidationRef.current(); + }, VALIDATION_DEBOUNCE_MS); + }, + [cancelPendingValidation], + ); + // Mount the CodeMirror view once the content is on screen; destroy it on // close/unmount. Depends on `load` so the host div is guaranteed mounted. useEffect(() => { @@ -240,6 +304,12 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa EditorView.updateListener.of((update) => { if (update.docChanged) { setDirty(isDirty(update.view, loadedTextRef.current)); + // Feedback is invalidated by the very edit that changed the + // document (issue #102): a stale Test/Save banner or issue + // list must never outlive the content it described. + setStatus(null); + setIssues([]); + scheduleLiveValidation(update.state.doc.toString()); } }), ], @@ -249,6 +319,9 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa return () => { view.destroy(); editorViewRef.current = null; + // A pending live-validation timer would fire into a destroyed view — + // and leak across tests and sessions. Drop it with the mount. + cancelPendingValidation(); }; }, [open, load]); @@ -387,8 +460,14 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa () => runToolbarAction("test", async (seq) => { if (load.status !== "ready") return; + // An explicit Test cancels any pending live cycle (its result + // would describe the same document anyway) and records the text + // it is asking about, so the debounce skips a no-op repeat. + cancelPendingValidation(); + const text = currentDocText(); + lastValidatedTextRef.current = text; try { - const result = await validateConfigContent(load.data.filename, currentDocText()); + const result = await validateConfigContent(load.data.filename, text); if (seq !== actionSeqRef.current) return; // superseded — drop stale results setIssues(result.issues); setStatus( @@ -408,6 +487,10 @@ export function ConfigEditorModal({ open, onClose, onApplied }: ConfigEditorModa [load, runToolbarAction], ); + /** Latest-run indirections for the CodeMirror listener closures. */ + runValidationRef.current = () => void runValidation(); + scheduleValidationRef.current = scheduleLiveValidation; + /** * Apply action: trial the current text as the session configuration. * Success fires `onApplied` with the applied AppConfigData; a 400