Fix intermittent happy-dom teardown race in JSX clean queue#337
Merged
brianmhunt merged 16 commits intomainfrom Apr 20, 2026
Merged
Fix intermittent happy-dom teardown race in JSX clean queue#337brianmhunt merged 16 commits intomainfrom
brianmhunt merged 16 commits intomainfrom
Conversation
JsxObserver.detachAndDispose defers node cleanup through a 25ms setTimeout batch (jsxClean.ts). In the cli-happy-dom vitest project, DOM globals are torn down between test files — if that timer is still pending at teardown, `cleanNode` fires against a dead global and throws `ReferenceError: Element is not defined` from `node instanceof Element` (packages/utils/src/dom/disposal.ts:95). All tests pass, but the trailing unhandled exception fails the CI job. Fix: - Export a new `flushJsxCleanNow()` from @tko/utils.jsx that cancels the pending timer and drains the queue synchronously. - Register it as an `afterEach` hook in builds/knockout/helpers/vitest-setup.js, gated on `isHappyDom()` so the browser matrix is untouched. 10 consecutive cli-happy-dom runs pass clean; browser suite (2699 tests) unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 32 minutes and 36 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR fixes a race condition in JSX cleanup by introducing a configurable Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Queue as queueCleanNode
participant Config as options.jsxCleanBatchSize
participant Batch as Batch Processor
participant Timer as Timer (25ms)
participant DOM as DOM Cleanup
par Synchronous Path (batch size = 0)
App->>Queue: queueCleanNode(node)
Queue->>Config: Check jsxCleanBatchSize
Config-->>Queue: 0 (synchronous)
Queue->>Batch: flushAll()
Batch->>DOM: cleanNode(node)
DOM-->>Batch: cleanup complete
Batch-->>Queue: queue empty
Queue-->>App: immediate return
and Asynchronous Path (batch size > 0)
App->>Queue: queueCleanNode(node)
Queue->>Config: Check jsxCleanBatchSize
Config-->>Queue: 1000 (async batches)
Queue->>Timer: scheduleBatch(25ms)
Timer-->>Batch: timer fires
Batch->>DOM: cleanNode(batch of N nodes)
DOM-->>Batch: N nodes cleaned
alt Queue has more nodes
Batch->>Timer: reschedule next batch
else Queue empty
Batch-->>Queue: complete
end
Queue-->>App: async return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Remove the isHappyDom() branch in vitest-setup.js. flushJsxCleanNow is a no-op when the queue is empty, so running it in afterEach is unconditionally safe — browsers just get slightly more deterministic test hygiene. - Extract drainBatch() shared by flushCleanQueue (scheduled, one batch + re-trigger) and flushJsxCleanNow (manual, full sync drain). Browser suite (2699) and 5× cli-happy-dom (2672 each) both clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mocha-test-helpers.js registers its own afterEach that runs user `_cleanups` (registered via the global `after(fn)`). Those cleanups often call ko.cleanNode / similar paths that can enqueue fresh nodes into the JSX clean queue. Vitest runs same-scope afterEach hooks in registration order. Previously mine registered first (drained), then mocha-helpers' ran (could re-enqueue), leaving a non-empty queue whose 25ms timer could fire inside the next test. Moving the flush hook to after the mocha-helpers import makes mine the last-registered at module scope, so it runs after any test-registered cleanups push work into the queue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The import-ordering approach (afterEach-registered-last) was fragile — a reshuffle of imports in vitest-setup.js would silently reintroduce the race. Move the knob to the API surface: - Add `options.jsxCleanBatchSize: number = 1000`. Set to `0` to disable batching entirely — jsx cleanup then runs synchronously on detach. - jsxClean.ts routes `queueCleanNode` through `flushAll()` (sync drain) or `scheduleBatch()` (25ms timer with `options.jsxCleanBatchSize` as the per-tick limit) based on the option. - Remove the hardcoded `MAX_CLEAN_AT_ONCE = 1000` constant (default on the option matches the old value, production behavior unchanged). - Remove `flushJsxCleanNow` export — no consumer needs it anymore. - vitest-setup.js sets `options.jsxCleanBatchSize = 0` on both the source singleton and the bundled `ko.options` (browser.min.js has its own Options instance). No afterEach hook, no hook-ordering gymnastics, no environment branching. Browser suite (2699) unchanged; cli-happy-dom passes 10× clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous commit added `jsxCleanBatchSize` directly to the core Options
class in packages/utils/src/options.ts — wrong-way dependency, since
@tko/utils has no concept of JSX and shouldn't carry per-plugin option
fields.
Move the option to its owning package (@tko/utils.jsx/src/jsxClean.ts):
- `declare module '@tko/utils' { interface Options { jsxCleanBatchSize: number } }`
- `defineOption('jsxCleanBatchSize', { default: 1000 })`
Matches the `strictEquality` pattern in
packages/utils.parser/src/operators.ts.
Add a "Configurable runtime options" section to AGENTS.md under Package
Conventions codifying the rule: configurable options go via
defineOption in their owning package, not as fields on the core Options
class. Prevents agents (and humans) from reaching for the core class
next time.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AGENTS.md keeps a one-line pointer; the full pattern (when to use defineOption vs a core Options field, the declare-module augmentation, the strictEquality example) now lives at tko.io/public/agents/options.md alongside the other agent-facing docs (contract.md, guide.md, soul.md, testing.md). Keeps AGENTS.md short and centralizes long-form guidance where other agent references already live. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ng") "Disables batching" was technically wrong — setting jsxCleanBatchSize to 0 doesn't just collapse the batch size to one, it skips the setTimeout entirely so cleanup runs synchronously on detach. Async vs sync is the real distinction. Updated the JSDoc on defineOption, the vitest-setup comment, and the changeset wording. Also fixed the changeset: the option lives in @tko/utils.jsx (registered via defineOption) — previous text implied it was a core @tko/utils field. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `while + splice(0, queue.length) + for` nesting was theatrical — batching only earns its keep when there's a yield between iterations. In the sync path there isn't one, so collapse to a single while-shift loop. The while itself is still needed (cleanNode side effects can re-enqueue). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Root cause of the CI flake in #337: defineOption unconditionally initialized its backing value to config.default, clobbering anything a caller had already assigned to options[name]. In the happy-dom setup, `options.jsxCleanBatchSize = 0` was written in vitest-setup (as a plain property, since defineOption had not yet run). When a spec later imported utils.jsx, jsxClean.ts's `defineOption('jsxCleanBatchSize', { default: 1000 })` replaced that plain property with a getter closing over 1000 — erasing the 0 and reinstating the async batch path. Fix: read any existing own-property data descriptor before Object.defineProperty replaces it, and use that value as the initial `_value` if present. Falls through to config.default when no prior assignment exists — i.e. the normal case is unchanged. Why this matters beyond the test race: anyone setting ko.options.X before the defining package loaded would see their value silently discarded. The fix makes the module-load order no longer affect option initialization. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Silent-preserve (previous commit) hid the real bug. A pre-assignment
to ko.options.<name> before the defining package loads almost always
means one of:
- load-order confusion (writing to the option before importing its
owner — symptom was the #337 happy-dom race)
- a typo on the option name (the write goes nowhere useful)
Both deserve a loud failure. defineOption now throws if the target
property already exists as a plain data descriptor, with a message
pointing at the likely fix ("import the owning package first, or
check for a typo").
Consequence: vitest-setup.js needs to import @tko/utils.jsx (which
transitively loads jsxClean.ts → runs defineOption) before assigning
options.jsxCleanBatchSize = 0. The dependency is now visible at the
top of the file rather than implicit in module-load order.
Full browser suite (2699) and cli-happy-dom (2672 × 3) pass clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous commits attempted to solve the "setup writes before defineOption runs" problem by modifying defineOption — first silently preserving pre-set values, then throwing on pre-sets. Both created fragile import-order contracts at the test-setup layer. Better answer: use `beforeAll` in vitest-setup. Hooks run after all module-load side effects have completed, so defineOption registrations have already happened by the time the hook assigns the option. No import-order dance, no silent preserve, no throw. Revert defineOption to its original behavior (minimal change to core). Move the jsxCleanBatchSize assignment — and the bundled ko.options mirror — into a single beforeAll block. Full browser suite (2699) and cli-happy-dom (2672 × 5) pass clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Covers both branches of the queueCleanNode path: Sync (jsxCleanBatchSize = 0): - cleans synchronously on enqueue - drains multiple queued nodes in a single call - does not schedule a timer (advancing the clock fires nothing) Batched (jsxCleanBatchSize > 0, including the 1000 default): - defers cleanup until the 25ms timer fires - a second enqueue doesn't re-trigger the already-pending timer - processes at most batchSize nodes per tick (verified with size=3) - re-schedules itself to drain the remainder (verified with size=2 across 5 nodes → 3 ticks) Uses sinon.useFakeTimers so the 25ms wait is deterministic and can't flake under CI teardown timing (which was the bug this PR started from). 7 new tests; full browser suite now 2706, cli-happy-dom 2679. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfcfccb732
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+40
to
+44
| const nodes = cleanNodeQueue.splice(0, options.jsxCleanBatchSize) | ||
| for (const node of nodes) { | ||
| cleanNode(node) | ||
| } | ||
| triggerCleanTimeout() | ||
| scheduleBatch() |
There was a problem hiding this comment.
When options.jsxCleanBatchSize is changed from a positive value to 0 while a batch timer is already scheduled, flushBatch executes with splice(0, 0), cleans nothing, and then immediately re-schedules itself because the queue is still non-empty. This creates a perpetual 25ms timer loop where queued nodes are never disposed, so callbacks never run and cleanup can leak indefinitely unless another queueCleanNode call happens to trigger flushAll. Because jsxCleanBatchSize is now a runtime option, this transition is a realistic configuration path and needs an explicit guard.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate an intermittent cli-happy-dom Vitest failure caused by deferred JSX node cleanup running after happy-dom tears down DOM globals, by making JSX cleanup behavior configurable and adjusting test setup accordingly.
Changes:
- Introduces
options.jsxCleanBatchSize(viadefineOption) to control JSX cleanup batching, including a synchronous mode (0). - Adds a new spec suite covering synchronous vs batched cleanup behavior.
- Updates Vitest setup and agent documentation around defining
ko.options.*options.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/utils.jsx/src/jsxClean.ts |
Adds jsxCleanBatchSize option and implements sync/batched queue draining logic. |
packages/utils.jsx/spec/jsxCleanBehaviors.ts |
Adds tests for queue behavior under different batch sizes. |
builds/knockout/helpers/vitest-setup.js |
Forces JSX cleanup to synchronous mode in tests via beforeAll. |
AGENTS.md |
Documents the defineOption pattern for package-owned ko.options.*. |
tko.io/public/agents/options.md |
Adds a detailed guide for adding runtime options (defineOption vs core field). |
.changeset/fix-jsx-clean-teardown-race.md |
Changeset entry for the new option and the test-environment mitigation. |
Comment on lines
+25
to
+35
| // Run JSX node cleanup synchronously in tests. The default 25ms batch | ||
| // (packages/utils.jsx/src/jsxClean.ts) can otherwise fire a timer after a | ||
| // vitest environment (e.g. happy-dom) tears down DOM globals, surfacing as | ||
| // `ReferenceError: Element is not defined` from `cleanNode`. `0` runs | ||
| // cleanup synchronously on detach. Using beforeAll so it runs after the | ||
| // defining package's module-load side effects, independent of import order. | ||
| beforeAll(() => { | ||
| options.jsxCleanBatchSize = 0 | ||
| // browser.min.js bundles its own Options instance. | ||
| globalThis.ko.options.jsxCleanBatchSize = 0 | ||
| }) |
Comment on lines
+49
to
+53
| // If a timer had been scheduled, advancing the fake clock would fire it | ||
| // and re-invoke the dispose callback — but the dispose callback was | ||
| // already consumed synchronously, and the node is no longer queued. | ||
| clock.tick(100) | ||
| assert.lengthOf(cleaned, 1) |
Comment on lines
+38
to
+45
| function flushBatch() { | ||
| cleanNodeTimeoutID = null | ||
| const nodes = cleanNodeQueue.splice(0, MAX_CLEAN_AT_ONCE) | ||
| const nodes = cleanNodeQueue.splice(0, options.jsxCleanBatchSize) | ||
| for (const node of nodes) { | ||
| cleanNode(node) | ||
| } | ||
| triggerCleanTimeout() | ||
| scheduleBatch() | ||
| } |
| // `while` covers re-enqueues from cleanNode side effects — no batching | ||
| // here since there's no yield to pause between. | ||
| while (cleanNodeQueue.length) { | ||
| cleanNode(cleanNodeQueue.shift()!) |
Aggregated from a three-way review:
- jsxClean.ts flushAll: replace `while (length) cleanNode(shift()!)`
with `while (length) for (const n of splice(0)) cleanNode(n)` — no
non-null assertion, amortized O(n) instead of O(n²).
- jsxClean.ts defineOption JSDoc: trim from 6-line block that narrated
what defineOption does to a single WHY comment.
- vitest-setup.js: compress the teardown-race explanation to 3 lines.
- jsxCleanBehaviors.ts: drop the inline comment that restated the test
name ("does not schedule a timer").
- Changeset: tighten from 22 lines to 12, no loss of information.
Same semantics, fewer words. Browser 2706, cli-happy-dom 2679 pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builds/knockout/helpers/vitest-setup.js`:
- Around line 28-31: The beforeAll setup currently forces synchronous cleanup by
setting options.jsxCleanBatchSize = 0 and
globalThis.ko.options.jsxCleanBatchSize = 0 unconditionally; change it to only
perform those assignments when isHappyDom() returns true so real-browser runs
keep the production batching behavior—update the beforeAll block to check
isHappyDom() and only set options.jsxCleanBatchSize and
globalThis.ko.options.jsxCleanBatchSize inside that conditional.
In `@packages/utils.jsx/src/jsxClean.ts`:
- Around line 34-40: The scheduled flushBatch must re-check
options.jsxCleanBatchSize before splicing to avoid infinite rescheduling when
the size becomes 0/invalid: inside flushBatch (which currently sets
cleanNodeTimeoutID = null and splices cleanNodeQueue) validate that const batch
= Number(options.jsxCleanBatchSize); if batch is not a positive finite integer
then call flushAll() synchronously to process the whole cleanNodeQueue and
return without calling scheduleBatch(); otherwise use splice(0, batch) and
continue as before. Ensure you reference flushBatch, flushAll, scheduleBatch,
cleanNodeQueue and options.jsxCleanBatchSize when applying the change.
In `@tko.io/public/agents/options.md`:
- Around line 1-66: Add the new agent-facing /agents/options.md into the agent
discovery list (append its entry to llms.txt) and insert a link to it from
agents/guide.md; also create a Starlight developer doc that documents the
defineOption pattern including usage examples (showing defineOption registration
and reading ko.options.*), the Options class core-field rule, and mention when
to prefer core Options fields vs defineOption (reference symbols: defineOption,
Options class, ko.options.*). Ensure the Starlight page explains the declare
module augmentation and the optional set(value) side-effect behavior so
developers can follow the exact pattern used in the diff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4f29b6c-75a6-43ed-8827-fb76a9be1d15
📒 Files selected for processing (6)
.changeset/fix-jsx-clean-teardown-race.mdAGENTS.mdbuilds/knockout/helpers/vitest-setup.jspackages/utils.jsx/spec/jsxCleanBehaviors.tspackages/utils.jsx/src/jsxClean.tstko.io/public/agents/options.md
Comment on lines
+28
to
+31
| beforeAll(() => { | ||
| options.jsxCleanBatchSize = 0 | ||
| globalThis.ko.options.jsxCleanBatchSize = 0 | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which configs/suites load this setup file.
set -euo pipefail
rg -n -C3 'vitest-setup\.js|helpers/vitest-setup' .Repository: knockout/tko
Length of output: 1415
🏁 Script executed:
# Read the vitest-setup.js file to confirm the actual code
cat -n builds/knockout/helpers/vitest-setup.jsRepository: knockout/tko
Length of output: 1405
🏁 Script executed:
# Check Vitest documentation patterns and environment detection in similar test configs
# Look for how other configs/tests detect happy-dom or NODE_ENV
rg -n 'process.env|happy-dom|NODE_ENV' builds/knockout vitest.config.tsRepository: knockout/tko
Length of output: 1820
Gate synchronous cleanup to happy-dom only.
The setup file unconditionally sets jsxCleanBatchSize = 0 for both the browser real-browser matrix and the happy-dom environment. Since the teardown race is happy-dom-specific (line 25–27), the browser matrix should remain on production default batching behavior. isHappyDom() is already imported and available; use it to skip the sync cleanup setting except in happy-dom.
Suggested fix
beforeAll(() => {
+ if (!isHappyDom()) return
options.jsxCleanBatchSize = 0
globalThis.ko.options.jsxCleanBatchSize = 0
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeAll(() => { | |
| options.jsxCleanBatchSize = 0 | |
| globalThis.ko.options.jsxCleanBatchSize = 0 | |
| }) | |
| beforeAll(() => { | |
| if (!isHappyDom()) return | |
| options.jsxCleanBatchSize = 0 | |
| globalThis.ko.options.jsxCleanBatchSize = 0 | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builds/knockout/helpers/vitest-setup.js` around lines 28 - 31, The beforeAll
setup currently forces synchronous cleanup by setting options.jsxCleanBatchSize
= 0 and globalThis.ko.options.jsxCleanBatchSize = 0 unconditionally; change it
to only perform those assignments when isHappyDom() returns true so real-browser
runs keep the production batching behavior—update the beforeAll block to check
isHappyDom() and only set options.jsxCleanBatchSize and
globalThis.ko.options.jsxCleanBatchSize inside that conditional.
Comment on lines
+1
to
+66
| # `ko.options.*` — Configurable Runtime Options | ||
|
|
||
| `ko.options` is TKO's runtime configuration object. It is a singleton `Options` | ||
| instance defined in `packages/utils/src/options.ts`. This page documents how | ||
| to add new options and which of the two available mechanisms to reach for. | ||
|
|
||
| ## Two mechanisms — when to use which | ||
|
|
||
| ### `defineOption` (default — almost always use this) | ||
|
|
||
| For options that belong to a specific package (batch sizes, feature toggles, | ||
| plugin behavior), register via `defineOption` inside the owning package. | ||
|
|
||
| ```ts | ||
| // packages/utils.jsx/src/jsxClean.ts | ||
| import { defineOption, options } from '@tko/utils' | ||
|
|
||
| // Extend the Options type so ko.options.<name> is strongly typed. | ||
| declare module '@tko/utils' { | ||
| interface Options { | ||
| jsxCleanBatchSize: number | ||
| } | ||
| } | ||
|
|
||
| // Register at module load, with an optional side-effect setter. | ||
| defineOption('jsxCleanBatchSize', { default: 1000 }) | ||
|
|
||
| // Read wherever the option applies. | ||
| if (options.jsxCleanBatchSize === 0) { /* sync path */ } | ||
| ``` | ||
|
|
||
| Rules: | ||
|
|
||
| - The `declare module '@tko/utils' { interface Options { ... } }` augmentation | ||
| lives in the same file (or at least the same package) that defines the option. | ||
| - `defineOption` registers at module-side-effect time; the option is available | ||
| as soon as the owning package is imported. | ||
| - An optional `set(value)` runs side effects at configuration time (not | ||
| retroactively on already-parsed bindings) — use for options that swap | ||
| implementations or rebuild internal state. | ||
|
|
||
| **Canonical example with a side-effect setter:** | ||
| `packages/utils.parser/src/operators.ts` → `strictEquality` swaps the `==` | ||
| and `!=` operator implementations when the option flips. | ||
|
|
||
| ### Core `Options` class field (rare — only for `@tko/utils` intrinsics) | ||
|
|
||
| Add a field directly to the `Options` class in `packages/utils/src/options.ts` | ||
| **only** when the option is intrinsic to `@tko/utils` itself — something that | ||
| `@tko/utils` uses or enforces without knowing about any downstream package. | ||
|
|
||
| Current core fields include `templateSizeLimit`, `deferUpdates`, | ||
| `useOnlyNativeEvents`, `onError`, `sanitizeHtmlTemplate`. | ||
|
|
||
| Never add a core-class field for an option that belongs to another package. | ||
| Doing so forces `@tko/utils` to carry concepts from downstream packages and | ||
| creates a wrong-way dependency. | ||
|
|
||
| ## Quick test | ||
|
|
||
| > Is the option intrinsic to `@tko/utils`? | ||
|
|
||
| - Yes → core class field. | ||
| - No → `defineOption` in the owning package. | ||
|
|
||
| In practice, new options are almost always the second case. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the new options documentation is discoverable from agent entrypoints
# and mirrored somewhere in the human-facing docs.
set -euo pipefail
printf '\nReferences to options.md / jsxCleanBatchSize / defineOption:\n'
fd -e md -e mdx -e astro . tko.io AGENTS.md 2>/dev/null \
| xargs rg -n -C2 'options\.md|jsxCleanBatchSize|defineOption|ko\.options' || trueRepository: knockout/tko
Length of output: 6411
🏁 Script executed:
# Check agent entrypoints for references to options.md
echo "=== llms.txt content ==="
cat tko.io/public/llms.txt 2>/dev/null || echo "File not found"
echo -e "\n=== agents/guide.md content ==="
cat tko.io/public/agents/guide.md 2>/dev/null || echo "File not found"Repository: knockout/tko
Length of output: 12319
🏁 Script executed:
# Search for Starlight docs that document option registration or defineOption pattern
fd -e md -e mdx . tko.io/src/content/docs 2>/dev/null \
| xargs rg -l 'defineOption|option.*registration|ko\.options.*register' || echo "No Starlight pages found documenting option registration"Repository: knockout/tko
Length of output: 112
Link new options page from agent discovery and add Starlight documentation for the defineOption pattern.
The new agent-facing /agents/options.md page documents defineOption (a new API pattern), but it's missing from llms.txt and agents/guide.md, making it undiscoverable. Per the coding guideline, new APIs and patterns must be documented in both Starlight (for human developers) and the agent guide. Add /agents/options.md to the agent discovery list in llms.txt, link it from agents/guide.md, and create a Starlight page documenting how developers add new options via defineOption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tko.io/public/agents/options.md` around lines 1 - 66, Add the new
agent-facing /agents/options.md into the agent discovery list (append its entry
to llms.txt) and insert a link to it from agents/guide.md; also create a
Starlight developer doc that documents the defineOption pattern including usage
examples (showing defineOption registration and reading ko.options.*), the
Options class core-field rule, and mention when to prefer core Options fields vs
defineOption (reference symbols: defineOption, Options class, ko.options.*).
Ensure the Starlight page explains the declare module augmentation and the
optional set(value) side-effect behavior so developers can follow the exact
pattern used in the diff.
1. Guard flushBatch against mid-flight batchSize flip to 0 All three reviewers (codex, copilot, coderabbit) flagged the same real bug: if a consumer sets jsxCleanBatchSize from positive to 0 while a batch timer is pending, flushBatch's `splice(0, 0)` makes no progress and scheduleBatch re-arms — a 25ms no-op loop with the queue never draining. flushBatch now checks `<= 0` and falls through to flushAll. Regression test added. 2. Strengthen 'does not schedule a timer' test Copilot pointed out that relying on dispose-callback count was a weak signal — cleanNode only fires disposers once, so even if a timer had been armed and fired, `cleaned.length === 1` would still pass. Switch to `clock.countTimers() === 0` for a direct assertion. 3. Link options.md from agent discovery CodeRabbit flagged it as undiscoverable. Added to tko.io/public/llms.txt (the agent entry index) and cross-referenced from agents/guide.md. Skipped: copilot's "gate setup on isHappyDom()" finding — the user explicitly rejected environment gating earlier in this PR. Skipped: copilot's stale O(n²) shift finding — already fixed in the simplify pass before CI ran. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Discoverability finding from CodeRabbit. The companion edit to this file was silently dropped earlier — the Edit tool rejected it (I hadn't Read the file first) and I didn't catch the error before committing the adjacent changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
Per CodeRabbit's follow-up finding: the `<= 0` guard in flushBatch caught the mid-flight 0 case, but not NaN. `NaN <= 0` is false, so a NaN batch size would fall through to `splice(0, NaN)`, which coerces to `splice(0, 0)` — reinstating the exact infinite reschedule loop we were trying to prevent. Switch to `Math.trunc` + `Number.isFinite` so any non-positive or non-finite value routes through `flushAll`. Added a regression test covering the NaN case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Addressing CodeRabbit's three findings: 1. Gate 2. Handle dynamic changes to 3. Link from Thanks CodeRabbit — the NaN catch is genuinely useful. |
brianmhunt
force-pushed
the
fix/jsx-clean-teardown-race
branch
from
a7500b9 to
22dad97
Compare
4 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
cli-happy-domvitest project has been hitting an intermittent CI failure that looks like:```
ReferenceError: Element is not defined
❯ cleanNode packages/utils/dist/dom/disposal.js:74:77
❯ Timeout.flushCleanQueue packages/utils.jsx/dist/jsxClean.js:21:14
❯ listOnTimeout node:internal/timers:605:17
```
All tests pass, but the trailing unhandled exception fails the job.
Root cause
JsxObserver.detachAndDisposedefers node cleanup through a 25mssetTimeoutbatch (packages/utils.jsx/src/jsxClean.ts:15). In happy-dom, vitest tears down DOM globals between test files. If the timer is still pending when teardown runs, the next `flushCleanQueue` call hits `node instanceof Element` (packages/utils/src/dom/disposal.ts:95) against an undefined `Element` and throws.Real browsers don't have this teardown race. Flake was first visible in CI after the happy-dom project landed in #333.
Fix
No guards like `typeof Element !== 'undefined'` in production code — the race is resolved at the teardown layer where it belongs.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
jsxCleanBatchSizeoption to control JSX cleanup batching behavior (default: 1000).Documentation