From 79c14c6e6f7c6903feb15305c3c593ec59557e5c Mon Sep 17 00:00:00 2001 From: Rod Begbie Date: Tue, 28 Jul 2026 20:17:33 -0700 Subject: [PATCH 1/5] fix(lint-framework): coalesce lint requests dropped while one is in flight `requestLintUpdate` used `lintRequested` as a single-flight mutex, but a request arriving while one was in flight was discarded rather than queued. The default delay is 0, so there is no debounce to coalesce them either. A burst of input could therefore leave the rendered lints belonging to a stale prefix of the text, recovered only by the 1000ms safety-net timer. That staleness is invisible on screen, because `remapLintToCurrentSource` keeps the highlight correctly positioned. It is not invisible to the ignore path: `LintContext` hashes the tokens following a lint, so a lint computed against a prefix carries a `context_hash` that never matches the one derived from the final text. Dismissing such a lint records a hash that matches nothing, and the highlight comes straight back and stays. Track a `lintDirty` flag and re-run once after the in-flight pass releases the mutex. Two placement details matter. The re-run must happen after that release, or it hits the same guard and is dropped in turn. And it belongs inside the `finally`, so that a pass which threw still hands off the input that arrived while it was running -- otherwise a rejected lint strands exactly the work this change exists to preserve, and recovery falls back to the 1000ms timer. That `finally` also fixes a pre-existing bug: a rejected `lintProvider` previously left `lintRequested` stuck at true, permanently stopping all linting. Refs #3911 Co-Authored-By: Claude Opus 5 Entire-Checkpoint: ffb36eab0913 --- .../lint-framework/src/lint/LintFramework.ts | 83 ++++++++++++------- 1 file changed, 51 insertions(+), 32 deletions(-) diff --git a/packages/lint-framework/src/lint/LintFramework.ts b/packages/lint-framework/src/lint/LintFramework.ts index 2ac3c67e0c..656bfc38db 100644 --- a/packages/lint-framework/src/lint/LintFramework.ts +++ b/packages/lint-framework/src/lint/LintFramework.ts @@ -48,6 +48,8 @@ export default class LintFramework { private targets: Set; private scrollableAncestors: Set; private lintRequested = false; + /** Set when a lint request arrives while one is already in flight. */ + private lintDirty = false; private renderRequested = false; private lintDelayTimer: number | null = null; private lastInputAt = 0; @@ -149,51 +151,68 @@ export default class LintFramework { // Avoid duplicate requests in the queue if (!immediate) { this.lintRequested = true; + // This pass reads the text as it is now, so it already covers every + // request made before it started. + this.lintDirty = false; } - const lintResults = await Promise.all( - this.onScreenTargets().map(async (target) => { - if (!document.contains(target)) { - this.targets.delete(target); - return { target: null as HTMLElement | null, lints: {} }; - } + try { + const lintResults = await Promise.all( + this.onScreenTargets().map(async (target) => { + if (!document.contains(target)) { + this.targets.delete(target); + return { target: null as HTMLElement | null, lints: {} }; + } - const { text, isCM, newLineIndices } = this.getTargetText(target); + const { text, isCM, newLineIndices } = this.getTargetText(target); - if (!text || text.length > 120000) { - return { target: null as HTMLElement | null, lints: {} }; - } + if (!text || text.length > 120000) { + return { target: null as HTMLElement | null, lints: {} }; + } - const language = getTargetLanguage(target); - let lintsBySource = await this.lintProvider(text, window.location.hostname, { - forceAllHeadings: isHeading(target), - language, - }); + const language = getTargetLanguage(target); + let lintsBySource = await this.lintProvider(text, window.location.hostname, { + forceAllHeadings: isHeading(target), + language, + }); - if (isCM) { - // We're about to modify a reference, so let's work on a copy. - lintsBySource = window.structuredClone(lintsBySource); + if (isCM) { + // We're about to modify a reference, so let's work on a copy. + lintsBySource = window.structuredClone(lintsBySource); - for (const lints of Object.values(lintsBySource)) { - for (const lint of lints) { - const offset_start = newLineIndices.findIndex((i) => i > lint.span.start); - const offset_end = newLineIndices.findIndex((i) => i > lint.span.end); + for (const lints of Object.values(lintsBySource)) { + for (const lint of lints) { + const offset_start = newLineIndices.findIndex((i) => i > lint.span.start); + const offset_end = newLineIndices.findIndex((i) => i > lint.span.end); - lint.span.start -= offset_start; - lint.span.end -= offset_end; + lint.span.start -= offset_start; + lint.span.end -= offset_end; + } } } - } - return { target: target as HTMLElement, lints: lintsBySource }; - }), - ); + return { target: target as HTMLElement, lints: lintsBySource }; + }), + ); - this.lastLints = lintResults.filter((r) => r.target != null) as any; - if (!immediate) { - this.lintRequested = false; + this.lastLints = lintResults.filter((r) => r.target != null) as any; + this.requestRender(); + } finally { + if (!immediate) { + this.lintRequested = false; + + // Must run after the flag above is cleared, or this call hits the + // same guard, marks the framework dirty again and is dropped. It + // lives in the `finally` so that a pass which threw still hands off + // the input that arrived while it was running. + if (this.lintDirty) { + this.lintDirty = false; + void this.requestLintUpdate(); + } + } } - this.requestRender(); + } else if (!immediate && this.lintRequested) { + this.lintDirty = true; } } From 57b19c32219095bd59ab70c9beb50fd1309b8499 Mon Sep 17 00:00:00 2001 From: Rod Begbie Date: Tue, 28 Jul 2026 20:50:41 -0700 Subject: [PATCH 2/5] test(lint-framework): cover lint request coalescing with vitest `packages/lint-framework` had no test harness at all, so the scheduling fix in the parent commit had no automated proof and the only coverage was the Playwright suite -- a poor instrument for this, since the flaky test it addresses already passes roughly two runs in three. Add vitest following the `harper.js` and `obsidian-plugin` precedent: browser mode via `@vitest/browser-playwright`, headless chromium. Two tests drive a lint provider whose responses resolve on demand, so a lint can be held in flight deliberately: - requests dropped mid-flight produce exactly one follow-up lint, and it sees the final text - a rejected provider does not leave the framework permanently wedged Both fail against the pre-fix scheduler and pass with it. Every wait is bounded to ten animation frames. `LintFramework` polls itself every second to cover editors that fail to emit events, and an earlier draft of these tests waited long enough for that poll to rescue them -- passing against unfixed code. Staying well under a second is what makes a pass mean the framework re-linted deliberately. Wire the suite into `just test-lintframework` and the CI matrix; without that the package's tests would never run. Refs #3911 Co-Authored-By: Claude Opus 5 Entire-Checkpoint: b35af98d196a --- .github/workflows/just_checks.yml | 1 + justfile | 12 +- packages/lint-framework/package.json | 8 +- .../src/lint/LintFramework.test.ts | 152 ++++++++++++++++++ packages/lint-framework/vite.config.ts | 11 ++ pnpm-lock.yaml | 73 ++++----- 6 files changed, 208 insertions(+), 49 deletions(-) create mode 100644 packages/lint-framework/src/lint/LintFramework.test.ts diff --git a/.github/workflows/just_checks.yml b/.github/workflows/just_checks.yml index ff89b3da53..322f33de04 100644 --- a/.github/workflows/just_checks.yml +++ b/.github/workflows/just_checks.yml @@ -23,6 +23,7 @@ jobs: test-rust, test-harperjs, test-vscode, + test-lintframework, test-chrome-plugin, test-firefox-plugin, test-obsidian, diff --git a/justfile b/justfile index 66c6bbc9a1..1b22cf7d09 100644 --- a/justfile +++ b/justfile @@ -124,6 +124,16 @@ test-obsidian: build-obsidian pnpm playwright install pnpm test +alias test-lint-framework := test-lintframework +test-lintframework: build-lint-framework + #!/usr/bin/env bash + set -eo pipefail + + pnpm install + cd "{{justfile_directory()}}/packages/lint-framework" + pnpm playwright install + pnpm test + alias dev-wordpress := dev-wp dev-wp: build-harperjs #!/usr/bin/env bash @@ -496,7 +506,7 @@ test-rust: cargo test -q # Test everything. -test: test-rust test-harperjs test-vscode test-obsidian test-chrome-plugin test-firefox-plugin +test: test-rust test-harperjs test-vscode test-obsidian test-lintframework test-chrome-plugin test-firefox-plugin # Use `harper-cli` to parse a provided file and print out the resulting tokens. parse file: diff --git a/packages/lint-framework/package.json b/packages/lint-framework/package.json index 858a142581..514404dddb 100644 --- a/packages/lint-framework/package.json +++ b/packages/lint-framework/package.json @@ -18,7 +18,7 @@ "scripts": { "build": "tsc && vite build -l warn", "dev": "vite", - "test": "echo 'no tests'" + "test": "vitest run" }, "dependencies": { "@fortawesome/fontawesome-svg-core": "^7.1.0", @@ -35,6 +35,10 @@ "type-fest": "^4.37.0", "typescript": "catalog:", "vite": "^6.1.0", - "vite-plugin-dts": "^4.5.0" + "vite-plugin-dts": "^4.5.0", + "@vitest/browser": "^4.0.16", + "@vitest/browser-playwright": "^4.0.16", + "vitest": "^4.0.16", + "@playwright/test": "1.60.0" } } diff --git a/packages/lint-framework/src/lint/LintFramework.test.ts b/packages/lint-framework/src/lint/LintFramework.test.ts new file mode 100644 index 0000000000..73e4d26945 --- /dev/null +++ b/packages/lint-framework/src/lint/LintFramework.test.ts @@ -0,0 +1,152 @@ +import { afterEach, describe, expect, it } from 'vitest'; +import LintFramework from './LintFramework'; +import type { UnpackedLintGroups } from './unpackLint'; + +/** + * `LintFramework` polls itself every second to cover editors that fail to emit + * events. Every wait here stays far below that, so a passing assertion means + * the framework re-linted deliberately rather than being rescued by the poll. + */ +const TICKS = 10; + +/** Let queued microtasks and one animation frame run. */ +async function tick() { + await new Promise((resolve) => requestAnimationFrame(() => resolve())); + await Promise.resolve(); +} + +async function waitTicks(count = TICKS) { + for (let i = 0; i < count; i++) { + await tick(); + } +} + +/** Wait for `count` lint requests, giving up well before the one-second poll. */ +async function waitForCalls(calls: string[], count: number) { + for (let i = 0; i < TICKS && calls.length < count; i++) { + await tick(); + } +} + +/** + * A lint provider whose responses are resolved by hand, so a test can hold a + * lint "in flight" and decide exactly when it completes. + */ +function deferredProvider() { + const calls: string[] = []; + const pending: (() => void)[] = []; + + const provider = (text: string): Promise => { + calls.push(text); + return new Promise((resolve) => { + pending.push(() => resolve({})); + }); + }; + + return { + provider, + calls, + /** Complete the oldest outstanding request. */ + async resolveNext() { + const next = pending.shift(); + if (next == null) { + throw new Error('No outstanding lint request to resolve.'); + } + next(); + await tick(); + }, + }; +} + +const PROVIDER_FAILURE = 'lint provider exploded'; + +/** + * `update()` calls `requestLintUpdate()` without awaiting it, so a rejected + * lint surfaces as an unhandled rejection rather than reaching a caller. That + * is existing behaviour and out of scope here; swallow the one failure this + * test provokes so it does not read as an unrelated error. + */ +function suppressProviderFailure(event: PromiseRejectionEvent) { + if (event.reason instanceof Error && event.reason.message === PROVIDER_FAILURE) { + event.preventDefault(); + } +} + +const targets: HTMLTextAreaElement[] = []; + +function makeTextarea(value: string): HTMLTextAreaElement { + const el = document.createElement('textarea'); + el.value = value; + el.style.width = '400px'; + el.style.height = '100px'; + document.body.appendChild(el); + targets.push(el); + return el; +} + +afterEach(() => { + for (const el of targets.splice(0)) { + el.remove(); + } +}); + +describe('LintFramework lint scheduling', () => { + it('re-lints with the final text as soon as the in-flight pass finishes', async () => { + const editor = makeTextarea('T'); + const { provider, calls, resolveNext } = deferredProvider(); + const fw = new LintFramework(provider, {}); + + await fw.addTarget(editor); + await waitForCalls(calls, 1); + expect(calls).toEqual(['T']); + + // Further input arrives while the first lint is still in flight. The + // single-flight guard drops each of these. + editor.value = 'This is a mistaek.'; + fw.update(); + fw.update(); + fw.update(); + await waitTicks(); + expect(calls).toEqual(['T']); + + await resolveNext(); + await waitForCalls(calls, 2); + + // Exactly one follow-up, seeing the text as it now stands. + expect(calls).toEqual(['T', 'This is a mistaek.']); + }); + + it('releases the in-flight guard when a lint request rejects', async () => { + const editor = makeTextarea('First.'); + const calls: string[] = []; + let failNext = true; + + const provider = async (text: string): Promise => { + calls.push(text); + if (failNext) { + failNext = false; + throw new Error(PROVIDER_FAILURE); + } + return {}; + }; + + const fw = new LintFramework(provider, {}); + + try { + window.addEventListener('unhandledrejection', suppressProviderFailure); + + await fw.addTarget(editor); + await waitForCalls(calls, 1); + expect(calls).toEqual(['First.']); + + // A rejection must not leave the framework permanently wedged. + editor.value = 'Second.'; + fw.update(); + await waitForCalls(calls, 2); + + expect(calls).toEqual(['First.', 'Second.']); + } finally { + window.removeEventListener('unhandledrejection', suppressProviderFailure); + } + }); +}); diff --git a/packages/lint-framework/vite.config.ts b/packages/lint-framework/vite.config.ts index dbc681ea9a..9ce7746c1c 100644 --- a/packages/lint-framework/vite.config.ts +++ b/packages/lint-framework/vite.config.ts @@ -1,8 +1,19 @@ +/// +import { playwright } from '@vitest/browser-playwright'; import { resolve } from 'path'; import { defineConfig } from 'vite'; import dts from 'vite-plugin-dts'; export default defineConfig({ + test: { + browser: { + provider: playwright(), + enabled: true, + headless: true, + screenshotFailures: false, + instances: [{ browser: 'chromium' }], + }, + }, build: { lib: { entry: resolve(__dirname, 'src/index.ts'), diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index fa3f8406e2..d2a6820ceb 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -369,12 +369,21 @@ importers: specifier: ^2.1.1 version: 2.1.1 devDependencies: + '@playwright/test': + specifier: 1.60.0 + version: 1.60.0 '@types/chrome': specifier: 0.1.27 version: 0.1.27 '@types/virtual-dom': specifier: ^2.1.4 version: 2.1.4 + '@vitest/browser': + specifier: ^4.0.16 + version: 4.0.16(msw@2.7.3(@types/node@22.13.10)(typescript@5.9.3))(vite@6.3.5(@types/node@22.13.10)(jiti@2.6.1)(lightningcss@1.32.0)(sass@1.85.1)(terser@5.39.0)(tsx@4.20.6)(yaml@2.7.0))(vitest@4.0.16) + '@vitest/browser-playwright': + specifier: ^4.0.16 + version: 4.0.16(msw@2.7.3(@types/node@22.13.10)(typescript@5.9.3))(playwright@1.60.0)(vite@6.3.5(@types/node@22.13.10)(jiti@2.6.1)(lightningcss@1.32.0)(sass@1.85.1)(terser@5.39.0)(tsx@4.20.6)(yaml@2.7.0))(vitest@4.0.16) type-fest: specifier: ^4.37.0 version: 4.37.0 @@ -387,6 +396,9 @@ importers: vite-plugin-dts: specifier: ^4.5.0 version: 4.5.3(@types/node@22.13.10)(rollup@4.53.3)(typescript@5.9.3)(vite@6.3.5(@types/node@22.13.10)(jiti@2.6.1)(lightningcss@1.32.0)(sass@1.85.1)(terser@5.39.0)(tsx@4.20.6)(yaml@2.7.0)) + vitest: + specifier: ^4.0.16 + version: 4.0.16(@types/node@22.13.10)(@vitest/browser-playwright@4.0.16)(@vitest/ui@4.0.16)(jiti@2.6.1)(jsdom@20.0.3)(lightningcss@1.32.0)(msw@2.7.3(@types/node@22.13.10)(typescript@5.9.3))(sass@1.85.1)(terser@5.39.0)(tsx@4.20.6)(yaml@2.7.0) packages/obsidian-plugin: dependencies: @@ -593,7 +605,7 @@ importers: version: 5.43.12 svelte-check: specifier: ^4.1.5 - version: 4.3.4(picomatch@4.0.2)(svelte@5.43.12)(typescript@5.9.3) + version: 4.3.4(picomatch@4.0.3)(svelte@5.43.12)(typescript@5.9.3) tailwindcss: specifier: ^4.1.16 version: 4.2.4 @@ -1693,11 +1705,11 @@ packages: '@esbuild-kit/core-utils@3.3.2': resolution: {integrity: sha512-sPRAnw9CdSsRmEtnsl2WXWdyquogVpB3yZ3dgwJfe8zrOzTsV7cJvmwrKVa+0ma5BoiGJ+BoqkMvawbayKUsqQ==} - deprecated: 'Merged into tsx: https://tsx.is' + deprecated: 'Merged into tsx: https://tsx.hirok.io' '@esbuild-kit/esm-loader@2.6.5': resolution: {integrity: sha512-FxEMIkJKnodyA1OaCUoEvbYRkoZlLZ4d/eXFu9Fh8CbBBgP5EmZxrfTRyN0qpXZ4vOvqnE5YdRdcrmUUXuU+dA==} - deprecated: 'Merged into tsx: https://tsx.is' + deprecated: 'Merged into tsx: https://tsx.hirok.io' '@esbuild/aix-ppc64@0.21.5': resolution: {integrity: sha512-1SDgH6ZSPTlggy1yI6+Dbkiz8xzpHJEVAlF/AM1tHPLsf5STom9rwtjE4hKAF20FfXXNTFqEYXyJNWh1GiZedQ==} @@ -6793,9 +6805,6 @@ packages: es-module-lexer@0.10.5: resolution: {integrity: sha512-+7IwY/kiGAacQfY+YBhKMvEmyAJnw5grTUgjG85Pe7vcUI/6b7pZjZG8nQ7+48YhzEAEqrEgD2dCz/JIK+AYvw==} - es-module-lexer@1.6.0: - resolution: {integrity: sha512-qqnD1yMU6tk/jnaMosogGySTZP8YtUgAffA9nMN+E/rjxcfRQ6IEk7IiozUjgxKoFHBGjTLnrHB/YC45r/59EQ==} - es-module-lexer@1.7.0: resolution: {integrity: sha512-jEQoCwk8hyb2AZziIOLhDqpm5+2ww5uIE6lkO/6jcOCusfk6LhMHpXXfBLXTZ7Ydyt0j4VoUQv6uGNYbdW+kBA==} @@ -12537,18 +12546,6 @@ packages: utf-8-validate: optional: true - ws@8.18.1: - resolution: {integrity: sha512-RKW2aJZMXeMxVpnZ6bck+RswznaxmzdULiBr6KY7XkTnW8uvt0iT9H5DkHUChXrc+uurzwa0rVI16n/Xzjdz1w==} - engines: {node: '>=10.0.0'} - peerDependencies: - bufferutil: ^4.0.1 - utf-8-validate: '>=5.0.2' - peerDependenciesMeta: - bufferutil: - optional: true - utf-8-validate: - optional: true - ws@8.18.3: resolution: {integrity: sha512-PEIGCY5tSlUt50cqyMXfCzX+oOPqN0vuGqWzbcJ2xvnkzkq46oOpz7dQaTDBdfICb4N14+GARUDw2XV2N4tvzg==} engines: {node: '>=10.0.0'} @@ -15930,10 +15927,10 @@ snapshots: '@rollup/pluginutils': 5.1.4(rollup@4.35.0) commondir: 1.0.1 estree-walker: 2.0.2 - fdir: 6.4.3(picomatch@4.0.2) + fdir: 6.4.3(picomatch@4.0.3) is-reference: 1.2.1 - magic-string: 0.30.17 - picomatch: 4.0.2 + magic-string: 0.30.21 + picomatch: 4.0.3 optionalDependencies: rollup: 4.35.0 @@ -20689,8 +20686,6 @@ snapshots: es-module-lexer@0.10.5: {} - es-module-lexer@1.6.0: {} - es-module-lexer@1.7.0: {} es-object-atoms@1.1.1: @@ -21290,9 +21285,9 @@ snapshots: dependencies: pend: 1.2.0 - fdir@6.4.3(picomatch@4.0.2): + fdir@6.4.3(picomatch@4.0.3): optionalDependencies: - picomatch: 4.0.2 + picomatch: 4.0.3 fdir@6.4.4(picomatch@4.0.2): optionalDependencies: @@ -22960,7 +22955,7 @@ snapshots: whatwg-encoding: 2.0.0 whatwg-mimetype: 3.0.0 whatwg-url: 11.0.0 - ws: 8.18.1 + ws: 8.18.3 xml-name-validator: 4.0.0 transitivePeerDependencies: - bufferutil @@ -24972,7 +24967,7 @@ snapshots: debug: 4.4.0 devtools-protocol: 0.0.1367902 typed-query-selector: 2.12.0 - ws: 8.18.1 + ws: 8.18.3 transitivePeerDependencies: - bare-buffer - bufferutil @@ -24986,7 +24981,7 @@ snapshots: debug: 4.4.0 devtools-protocol: 0.0.1413902 typed-query-selector: 2.12.0 - ws: 8.18.1 + ws: 8.18.3 transitivePeerDependencies: - bare-buffer - bufferutil @@ -26242,18 +26237,6 @@ snapshots: brace: 0.11.1 sirv-cli: 1.0.14 - svelte-check@4.3.4(picomatch@4.0.2)(svelte@5.43.12)(typescript@5.9.3): - dependencies: - '@jridgewell/trace-mapping': 0.3.25 - chokidar: 4.0.3 - fdir: 6.4.4(picomatch@4.0.2) - picocolors: 1.1.1 - sade: 1.8.1 - svelte: 5.43.12 - typescript: 5.9.3 - transitivePeerDependencies: - - picomatch - svelte-check@4.3.4(picomatch@4.0.3)(svelte@5.43.12)(typescript@5.6.3): dependencies: '@jridgewell/trace-mapping': 0.3.25 @@ -27005,7 +26988,7 @@ snapshots: open: 10.1.0 perfect-debounce: 1.0.0 picocolors: 1.1.1 - sirv: 3.0.1 + sirv: 3.0.2 vite: 6.3.5(@types/node@22.13.10)(jiti@2.6.1)(lightningcss@1.32.0)(sass@1.85.1)(terser@5.39.0)(tsx@4.20.6)(yaml@2.7.0) transitivePeerDependencies: - rollup @@ -27287,7 +27270,7 @@ snapshots: sockjs: 0.3.24 spdy: 4.0.2 webpack-dev-middleware: 5.3.4(webpack@5.98.0) - ws: 8.18.1 + ws: 8.18.3 optionalDependencies: webpack: 5.98.0(webpack-cli@5.1.4) webpack-cli: 5.1.4(webpack-bundle-analyzer@4.10.2)(webpack-dev-server@4.15.2)(webpack@5.98.0) @@ -27316,7 +27299,7 @@ snapshots: browserslist: 4.24.4 chrome-trace-event: 1.0.4 enhanced-resolve: 5.18.1 - es-module-lexer: 1.6.0 + es-module-lexer: 1.7.0 eslint-scope: 5.1.1 events: 3.3.0 glob-to-regexp: 0.4.1 @@ -27591,8 +27574,6 @@ snapshots: ws@8.13.0: {} - ws@8.18.1: {} - ws@8.18.3: {} x-is-array@0.1.0: {} @@ -27631,7 +27612,7 @@ snapshots: y-protocols: 1.0.6(yjs@13.6.24) yjs: 13.6.24 optionalDependencies: - ws: 8.18.1 + ws: 8.18.3 transitivePeerDependencies: - bufferutil - supports-color From 3368543b660f82703f29a9f1f3c6af8520882c45 Mon Sep 17 00:00:00 2001 From: Rod Begbie Date: Tue, 28 Jul 2026 20:53:50 -0700 Subject: [PATCH 3/5] test(chrome-ext): let linting settle before ignoring a suggestion `testCanIgnoreSuggestion` acted on the first highlight to appear. That highlight can belong to a lint computed against a prefix of the typed text, because input events arriving mid-lint are coalesced into a follow-up pass. It looks identical on screen -- `remapLintToCurrentSource` keeps it correctly positioned -- but its context hash covers different trailing tokens. Ignoring it records a hash matching nothing, so the next pass returns the lint unfiltered and the highlight comes back and stays. That is the assertion which has been failing intermittently on Firefox. Wait for the follow-up pass once highlights first appear. The scheduling fix earlier in this branch is what makes waiting sufficient: before it, a dropped request was never re-run, so no amount of waiting converged. The interval clears `LintFramework`'s one-second self-poll, the slowest path by which a pass over the final text can arrive. Only this helper needs it -- it is the one whose assertion depends on context hashes agreeing across the action. Refs #3911 Co-Authored-By: Claude Opus 5 Entire-Checkpoint: 797267b3e02e --- packages/chrome-plugin/tests/testUtils.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/chrome-plugin/tests/testUtils.ts b/packages/chrome-plugin/tests/testUtils.ts index d265f0664a..be76417583 100644 --- a/packages/chrome-plugin/tests/testUtils.ts +++ b/packages/chrome-plugin/tests/testUtils.ts @@ -8,6 +8,13 @@ type ScreenPoint = { y: number; }; +/** + * How long to let linting settle once highlights first appear. Comfortably + * clears `LintFramework`'s one-second self-poll, which is the slowest path by + * which a pass over the final text can arrive. + */ +const LINT_SETTLE_MS = 1500; + export async function getBackground(context: BrowserContext) { return ( context.serviceWorkers()[0] ?? @@ -290,6 +297,14 @@ export async function testCanIgnoreSuggestion( // Ensure the test text produces only the spelling lint we intend to ignore. await expect(getHarperHighlights(page)).toHaveCount(1); + // That first highlight can come from a lint computed against a *prefix* of + // the text, because input events arriving mid-lint are coalesced into a + // follow-up pass. It looks identical on screen, but its context hash covers + // different trailing tokens, so ignoring it records a hash that never + // matches the real lint and the highlight returns. Wait for the follow-up + // pass to settle before acting on anything. + await page.waitForTimeout(LINT_SETTLE_MS); + // Open the popup for the highlight and click Ignore. const opened = await clickHarperHighlight(page); expect(opened).toBe(true); From d820fb4faac858f6f1cc1790abc232a5ea7926ee Mon Sep 17 00:00:00 2001 From: Rod Begbie Date: Tue, 28 Jul 2026 22:11:14 -0700 Subject: [PATCH 4/5] test(lint-framework): cover the error path of lint request coalescing Review of #3913 pointed out that the coalesced follow-up was skipped when the in-flight pass rejected -- the dirty check sat after the `try`/`finally`, so the exception propagated past it. That is fixed in the parent commit by moving the check inside the `finally`; this adds the test that pins it. The new case fails against the previous placement and passes with the fix, while the other two pass either way, so it isolates the error path precisely. Relax the existing rejection test to assert on order rather than an exact call count. Coalescing plus ambient page events -- the window listeners cover scroll, resize and selectionchange -- can legitimately add passes, so an exact count asserts something the framework never promised. It still fails against unfixed code. Also correct the `LINT_SETTLE_MS` comment. It justified the interval by the framework's one-second self-poll, which this wait cannot reliably cover: it starts at an arbitrary phase relative to that timer. The real justification is that the follow-up pass is event-driven and needs only a lint plus a render. Refs #3911 Co-Authored-By: Claude Opus 5 Entire-Checkpoint: 94085980577d --- packages/chrome-plugin/tests/testUtils.ts | 14 +++- .../src/lint/LintFramework.test.ts | 71 +++++++++++++++---- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/packages/chrome-plugin/tests/testUtils.ts b/packages/chrome-plugin/tests/testUtils.ts index be76417583..a7af7437a0 100644 --- a/packages/chrome-plugin/tests/testUtils.ts +++ b/packages/chrome-plugin/tests/testUtils.ts @@ -9,9 +9,17 @@ type ScreenPoint = { }; /** - * How long to let linting settle once highlights first appear. Comfortably - * clears `LintFramework`'s one-second self-poll, which is the slowest path by - * which a pass over the final text can arrive. + * How long to let linting settle once highlights first appear. + * + * The follow-up pass over the final text is event-driven: `LintFramework` + * coalesces requests that arrive mid-lint and issues one as soon as the + * in-flight pass releases. This only has to cover that pass plus a render, + * which is why a modest interval is enough. + * + * It is deliberately not sized against the framework's one-second self-poll. + * This wait starts at an arbitrary phase relative to that timer, so it could + * not reliably cover it anyway -- and relying on the poll is the behaviour the + * coalescing fix exists to avoid. */ const LINT_SETTLE_MS = 1500; diff --git a/packages/lint-framework/src/lint/LintFramework.test.ts b/packages/lint-framework/src/lint/LintFramework.test.ts index 73e4d26945..3a434d6510 100644 --- a/packages/lint-framework/src/lint/LintFramework.test.ts +++ b/packages/lint-framework/src/lint/LintFramework.test.ts @@ -34,27 +34,37 @@ async function waitForCalls(calls: string[], count: number) { */ function deferredProvider() { const calls: string[] = []; - const pending: (() => void)[] = []; + const pending: { resolve: () => void; reject: (reason: Error) => void }[] = []; const provider = (text: string): Promise => { calls.push(text); - return new Promise((resolve) => { - pending.push(() => resolve({})); + return new Promise((resolve, reject) => { + pending.push({ resolve: () => resolve({}), reject }); }); }; + async function settleOldest(outcome: 'resolve' | 'reject') { + const next = pending.shift(); + if (next == null) { + throw new Error('No outstanding lint request to settle.'); + } + + if (outcome === 'resolve') { + next.resolve(); + } else { + next.reject(new Error(PROVIDER_FAILURE)); + } + + await tick(); + } + return { provider, calls, /** Complete the oldest outstanding request. */ - async resolveNext() { - const next = pending.shift(); - if (next == null) { - throw new Error('No outstanding lint request to resolve.'); - } - next(); - await tick(); - }, + resolveNext: () => settleOldest('resolve'), + /** Fail the oldest outstanding request. */ + rejectNext: () => settleOldest('reject'), }; } @@ -116,6 +126,36 @@ describe('LintFramework lint scheduling', () => { expect(calls).toEqual(['T', 'This is a mistaek.']); }); + it('still issues the queued follow-up when the in-flight pass rejects', async () => { + const editor = makeTextarea('T'); + const { provider, calls, rejectNext } = deferredProvider(); + const fw = new LintFramework(provider, {}); + + try { + window.addEventListener('unhandledrejection', suppressProviderFailure); + + await fw.addTarget(editor); + await waitForCalls(calls, 1); + expect(calls).toEqual(['T']); + + // Input arrives mid-pass and is coalesced into a follow-up. + editor.value = 'This is a mistaek.'; + fw.update(); + await waitTicks(); + expect(calls).toEqual(['T']); + + // The pass then fails. Releasing the guard is not enough on its own: the + // queued work has to be handed off too, or the input that arrived during + // a failed lint is silently forgotten until the one-second poll. + await rejectNext(); + await waitForCalls(calls, 2); + + expect(calls).toEqual(['T', 'This is a mistaek.']); + } finally { + window.removeEventListener('unhandledrejection', suppressProviderFailure); + } + }); + it('releases the in-flight guard when a lint request rejects', async () => { const editor = makeTextarea('First.'); const calls: string[] = []; @@ -137,14 +177,19 @@ describe('LintFramework lint scheduling', () => { await fw.addTarget(editor); await waitForCalls(calls, 1); - expect(calls).toEqual(['First.']); + expect(calls[0]).toBe('First.'); // A rejection must not leave the framework permanently wedged. editor.value = 'Second.'; fw.update(); await waitForCalls(calls, 2); - expect(calls).toEqual(['First.', 'Second.']); + // Assert on order, not on an exact count. Ambient page events -- the + // window listeners cover scroll, resize and selectionchange -- can + // legitimately queue further passes, and a coalesced follow-up may add + // one more. What matters is that the failed pass was followed by one + // seeing the new text. + expect(calls.slice(0, 2)).toEqual(['First.', 'Second.']); } finally { window.removeEventListener('unhandledrejection', suppressProviderFailure); } From eaac704b492d154a9c270cd76e994287a4b27b42 Mon Sep 17 00:00:00 2001 From: Rod Begbie Date: Tue, 28 Jul 2026 22:17:50 -0700 Subject: [PATCH 5/5] test(lint-framework): bound scheduling waits by wall clock, not frame count Review of #3913 noted that the ten-animation-frame waits only stayed under `LintFramework`'s one-second self-poll because rAF happens to run fast. requestAnimationFrame is throttled when a page is backgrounded and stretches under load, so a fixed frame count can silently exceed the poll -- at which point the poll supplies the follow-up the test is looking for and a broken scheduler passes. A silent false pass is worse than a flake. Bound every wait by wall-clock time instead, and assert on *when* the follow-up arrived rather than only that it did. A lint produced by the poll is a second late by construction, so it cannot satisfy a 400ms budget however slowly the machine is running. The budget must stay below the poll interval for any of this to hold, so say so where the constant is defined -- raising it past a second is precisely what would restore the failure mode. Refs #3911 Co-Authored-By: Claude Opus 5 Entire-Checkpoint: 2707dcf7ac8b --- .../src/lint/LintFramework.test.ts | 50 ++++++++++++++----- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/packages/lint-framework/src/lint/LintFramework.test.ts b/packages/lint-framework/src/lint/LintFramework.test.ts index 3a434d6510..725c781d9d 100644 --- a/packages/lint-framework/src/lint/LintFramework.test.ts +++ b/packages/lint-framework/src/lint/LintFramework.test.ts @@ -4,10 +4,22 @@ import type { UnpackedLintGroups } from './unpackLint'; /** * `LintFramework` polls itself every second to cover editors that fail to emit - * events. Every wait here stays far below that, so a passing assertion means - * the framework re-linted deliberately rather than being rescued by the poll. + * events, so that poll can supply a lint the framework never scheduled itself. + * Every wait here is bounded by wall-clock time well below one second, and the + * tests additionally assert on *when* a follow-up arrived. A lint produced by + * the poll is a second late by construction and cannot satisfy that, however + * slowly the machine happens to be running. + * + * Frame counting was tried first and is not good enough: `requestAnimationFrame` + * is throttled when a page is backgrounded and stretches under load, so a fixed + * number of frames can silently exceed the poll interval. That turns a broken + * scheduler into a passing test, which is worse than a flake. + * + * This value MUST stay below the poll interval in `LintFramework`'s + * constructor. Raising it above one second is what would let the poll satisfy + * these tests, quietly restoring the failure mode described above. */ -const TICKS = 10; +const SETTLE_BUDGET_MS = 400; /** Let queued microtasks and one animation frame run. */ async function tick() { @@ -15,15 +27,18 @@ async function tick() { await Promise.resolve(); } -async function waitTicks(count = TICKS) { - for (let i = 0; i < count; i++) { +/** Wait for `count` lint requests, giving up well before the one-second poll. */ +async function waitForCalls(calls: string[], count: number) { + const deadline = performance.now() + SETTLE_BUDGET_MS; + while (calls.length < count && performance.now() < deadline) { await tick(); } } -/** Wait for `count` lint requests, giving up well before the one-second poll. */ -async function waitForCalls(calls: string[], count: number) { - for (let i = 0; i < TICKS && calls.length < count; i++) { +/** Give the framework room to act when we are asserting that it does not. */ +async function quietPeriod() { + const deadline = performance.now() + SETTLE_BUDGET_MS / 2; + while (performance.now() < deadline) { await tick(); } } @@ -34,10 +49,13 @@ async function waitForCalls(calls: string[], count: number) { */ function deferredProvider() { const calls: string[] = []; + /** When each entry in `calls` was requested, for asserting promptness. */ + const callTimes: number[] = []; const pending: { resolve: () => void; reject: (reason: Error) => void }[] = []; const provider = (text: string): Promise => { calls.push(text); + callTimes.push(performance.now()); return new Promise((resolve, reject) => { pending.push({ resolve: () => resolve({}), reject }); }); @@ -61,6 +79,7 @@ function deferredProvider() { return { provider, calls, + callTimes, /** Complete the oldest outstanding request. */ resolveNext: () => settleOldest('resolve'), /** Fail the oldest outstanding request. */ @@ -103,7 +122,7 @@ afterEach(() => { describe('LintFramework lint scheduling', () => { it('re-lints with the final text as soon as the in-flight pass finishes', async () => { const editor = makeTextarea('T'); - const { provider, calls, resolveNext } = deferredProvider(); + const { provider, calls, callTimes, resolveNext } = deferredProvider(); const fw = new LintFramework(provider, {}); await fw.addTarget(editor); @@ -116,19 +135,24 @@ describe('LintFramework lint scheduling', () => { fw.update(); fw.update(); fw.update(); - await waitTicks(); + await quietPeriod(); expect(calls).toEqual(['T']); + const releasedAt = performance.now(); await resolveNext(); await waitForCalls(calls, 2); // Exactly one follow-up, seeing the text as it now stands. expect(calls).toEqual(['T', 'This is a mistaek.']); + + // And issued off the back of the pass completing, not by the one-second + // poll -- which could not have produced it this quickly. + expect(callTimes[1] - releasedAt).toBeLessThan(SETTLE_BUDGET_MS); }); it('still issues the queued follow-up when the in-flight pass rejects', async () => { const editor = makeTextarea('T'); - const { provider, calls, rejectNext } = deferredProvider(); + const { provider, calls, callTimes, rejectNext } = deferredProvider(); const fw = new LintFramework(provider, {}); try { @@ -141,16 +165,18 @@ describe('LintFramework lint scheduling', () => { // Input arrives mid-pass and is coalesced into a follow-up. editor.value = 'This is a mistaek.'; fw.update(); - await waitTicks(); + await quietPeriod(); expect(calls).toEqual(['T']); // The pass then fails. Releasing the guard is not enough on its own: the // queued work has to be handed off too, or the input that arrived during // a failed lint is silently forgotten until the one-second poll. + const releasedAt = performance.now(); await rejectNext(); await waitForCalls(calls, 2); expect(calls).toEqual(['T', 'This is a mistaek.']); + expect(callTimes[1] - releasedAt).toBeLessThan(SETTLE_BUDGET_MS); } finally { window.removeEventListener('unhandledrejection', suppressProviderFailure); }