Live in-browser test runner at /tests#353
Merged
brianmhunt merged 22 commits intomainfrom Apr 22, 2026
Merged
Conversation
Dark-factory motivation: AI agents working on TKO should be able to navigate to `https://tko.io/tests` in any browser, watch the spec suite execute against real TKO + real DOM, and read pass/fail without cloning the repo or running Vitest locally. POC scope: `packages/observable/spec/**/*.ts` only. Verified locally with 113 passes, 0 failures, ~0.01s runtime. New pieces ---------- - `builds/knockout/helpers/browser-setup.js` — Mocha-oriented counterpart to the existing `vitest-setup.js`. Imports chai + sinon as ES modules (bundled by esbuild), exposes them as globals for spec compatibility, defines `isHappyDom = () => false` (real browser), and registers a Mocha `before()` hook that forces `ko.options.jsxCleanBatchSize = 0` so the 25ms JSX cleanup timer does not race test teardown. - `tko.io/scripts/bundle-tests.mjs` — esbuild bundler. Reads the spec glob list (SCOPE array), generates an in-memory entry via esbuild's `stdin` option (no `_entry.ts` file written to `public/`, which would otherwise be copied into `dist/tests/` at build time and served as a static asset), and emits `public/tests/bundle.js` as an IIFE with `globalName: 'tkoTests'`. chai/sinon/@tko/* are bundled; `mocha` is external so the page's `<script>` tag-loaded Mocha browser build supplies it. - `tko.io/src/pages/tests.astro` — Mocha HTML report host. Uses `is:inline` on every `<script>` so Astro does not rewrite them into ES modules (Astro's default), which breaks the mocha → mocha.setup → ko → bundle → mocha.run ordering. Loads mocha JS and CSS from unpkg (pinned major version, no fallback by design — stale mirror would undermine the "visit and see live" promise). - `tko.io/.gitignore` — added `public/tests/` so the generated bundle stays out of the tree. - `tko.io/package.json` `prebuild` — appended the bundler so deploy-docs.yml regenerates the bundle whenever the site builds. Bun honours the npm `prebuild` lifecycle, so `bun run build` runs it automatically. Follow-ups (not in this commit) ------------------------------- - Expand SCOPE to the other 25 packages; each needs a smoke run. - Consider cache-busting filename (`bundle-<hash>.js`) once scope grows — browsers cache `/tests/bundle.js` aggressively on repeat visits and a code change without a cache-bust would show stale results to an agent. - Document the page in `agents/testing.md` so agents know to use it. Adversarial pass: Explore subagent. Flagged 8 items. Real fix: `_entry.ts` was being written into `public/` and then copied into `dist/tests/_entry.ts` at build time, leaking spec file paths as a served asset — resolved by switching esbuild to `stdin`, which keeps the entry in memory. Rejected as false positive: "prebuild not in build pipeline" (bun honours npm lifecycle hooks, verified by the regenerated bundle after `bun run build`) and "tsconfig exclusion affects esbuild resolution" (reviewer self-corrected). Remaining items (global-namespace collision, cache thrashing at large scope, unpkg CDN fallback) accepted as POC-appropriate and noted above as follow-ups. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Scope expanded from the observable-only POC to every package's
specs plus the two bundled builds — `packages/*/spec/**/*.ts` +
`builds/{reference,knockout}/spec/**/*.js`. Matches `ALL_SPECS`
in `vitest.config.ts`.
Blocker and fix
---------------
The first attempt at full scope registered only 976 tests out of
the ~2500 `it()` calls that exist in the tree. Root cause: a
couple of specs under `builds/reference/spec/` import from `..`
which resolves to `builds/reference/src/index.ts`, and that file
references the compile-time constant `BUILD_VERSION`. `tools/
build.ts` injects it at build time with `--define:BUILD_VERSION
='"${version}"'`; Vitest configures the same define under the
hood. esbuild in this bundler did not, so the first reference
threw `BUILD_VERSION is not defined` at module-top, aborting the
surrounding IIFE and dropping every subsequent spec's
`describe`/`it` registration.
Fix: `bundle-tests.mjs` now reads the root `package.json`
version and passes `define: { BUILD_VERSION: JSON.stringify(
version) }` to esbuild, mirroring `tools/build.ts`. After the
fix: 2669 tests registered in 324 suites (was 976 tests, 109
suites). 2327 pass, 178 fail — investigating.
Other changes in this commit
----------------------------
- `bundle-tests.mjs` — scope expanded; added `EXCLUDE` regex to
drop `__screenshots__/`, `fixtures/`, and `helpers/` siblings
that live under `spec/` directories but aren't themselves
specs (the first expanded run tried to import Playwright
screenshot PNGs as TypeScript).
- `builds/knockout/helpers/browser-setup.js`
- Imports `./mocha-test-helpers.js` so specs get the globals
that module exposes (`prepareTestNode`, `restoreAfter`, the
`globalThis.after` cleanup-stack override, `expectContainHtml`,
etc.). Placed after `mocha.setup('bdd')` per its own
`beforeEach(...)` at module-top — that call needs Mocha's BDD
UI already active.
- Added a Vitest-context shim for `it`. Several specs use
`function (ctx: any) { if (isHappyDom()) return ctx.skip(...) }`
which works under Vitest because `ctx` is the test context.
Mocha sees arity 1 and waits for `done()`; the shim wraps
`it` and `it.only` so 1-arg specs whose source uses
`.skip(` and never `done(` get invoked with a stub ctx
`{ skip: reason => this.skip(reason) }` and their arity is
hidden from Mocha. Genuine Mocha done-callback specs
unchanged.
- `tko.io/src/pages/tests.astro`
- `mocha.setup({ timeout: 10000 })` to match
`testTimeout: 10000` in `vitest.config.ts`. Default 2000ms
was too tight for async binding/component specs.
- Pre-mocha error collector (`window.__tkoErrors`) so the next
bundle-load crash (if any) is visible to a developer opening
devtools instead of silently truncating the suite.
Follow-ups
----------
- Classify the 178 remaining failures. Expect roughly three
buckets: (a) test-context semantic gaps between Mocha and
Vitest we haven't shimmed; (b) sensitivity to JSX/DOM timing
that Vitest's browser mode smooths; (c) real behavior drift
worth surfacing on the page for agents anyway.
- Bundle cache-busting (3.1 MB at a stable URL is aggressively
cached; stale bundle after a code change would show wrong
pass/fail to a visitor).
- Document `/tests` in `tko.io/public/agents/testing.md` once
the failure list is under control.
Adversarial pass: verified against the live running page — tree
walk shows 324 suites / 2669 tests, the stats header matches
(`passes: 2327, failures: 178, duration: 27.3s`), and the three
remaining `__tkoErrors` entries are all async-rejection leakage
from intentionally-failing component specs (`ERRORS_ON_PURPOSE2`,
`Some error`, `bottomItem not found`) — not bundle-load crashes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
/tests now supports running specs against any published
`@tko/build.knockout` or `@tko/build.reference` version from
npm, not just the in-tree build. Dark-factory motivation: an
agent (or maintainer) should be able to say "is this bug in
4.0.1 or current main?" by visiting a URL, without cloning or
installing anything.
Architecture: split the spec bundle in two
-------------------------------------------
`tko.io/scripts/bundle-tests.mjs` now produces:
- `public/tests/build-bundle.js`
Specs from `builds/{knockout,reference}/spec/**/*.{js,ts}`
(60 specs today). These touch only the `ko.*` / `tko.*`
globals — zero `@tko/*` imports — so they are *version-
portable*: they can run against whichever build the page
loads via <script>.
- `public/tests/source-bundle.js`
Specs from `packages/*/spec/**/*.ts` (85 specs today).
These import from `@tko/*` and `../src`, so the bundler
inlines a copy of that source. Tightly coupled to the
in-tree TKO — only valid when the page also loads the
in-tree `/lib/ko.js`.
Splitting both sidesteps the class-identity collision that
dual-copies of TKO modules produce: mixing a script-tag-loaded
`ko.templateEngine` with a bundled-source one breaks every
`instanceof` check the suite does. The two suites now each
operate inside a single module graph.
Page UI
-------
`tko.io/src/pages/tests.astro` rewritten end-to-end:
- Mode radio: `in-tree dev` (default), `build.knockout`,
`build.reference`. Selects which ko to load.
- Version picker: populated from the npm registry
(`https://registry.npmjs.org/@tko/build.<pkg>`), defaults
to `latest`. Disabled in dev mode.
- Suite toggles: `build specs` and/or `source specs`. Source
specs auto-disabled in build mode — selecting them there
re-introduces the class-identity collision.
- Run button: rebuilds the query string (`?mode=build&
pkg=knockout&ver=4.0.1`) and reloads the page. State is
bookmarkable, pasteable, shareable between agents/humans.
- Live stats bar: pass / fail / pending / completion % /
total, ticking every 250ms so the tail end of a slow run is
observable instead of black-boxed.
- Errors drawer: surfaces every `window.error` and
`unhandledrejection` captured during the run so
bundle-load crashes are visible instead of hidden behind
Mocha's "only X tests ran" truncation.
- Footer: verbatim resolved config (`mode: build · pkg:
@tko/build.knockout@4.0.1 · suites: build`) and
`window.__tkoRunnerConfig` available for console poking.
- Theme: dark UI that doesn't fight the Mocha report.
Verified locally
----------------
- `?` (dev): loads `/lib/ko.js` + both bundles. 2669 tests
registered, 2428 pass / 181 fail / 39 pending. Footer:
`mode: in-tree dev · suites: build, source`.
- `?mode=build&pkg=knockout`: loads
`https://unpkg.com/@tko/build.knockout@4.0.1/dist/
browser.min.js` + build bundle only. 956 pass / 3 fail /
7.88s against the published 4.0.1.
The extra 178 dev-mode failures vs 3 build-mode failures is
the class-identity gap that the split is designed to make
visible and eventually close — agents can now tell which
failures are "real regression in the package source" versus
"dev-harness collision" at a glance.
Other changes in this commit
----------------------------
- `builds/knockout/helpers/browser-setup.js`: reverted an
earlier attempt to import the knockout build as an ES
module. Keeping the global script-tag path for dev mode
since it preserves the build's initialization side effects
in the correct order.
Follow-ups
----------
- Cache-bust `build-bundle.js` / `source-bundle.js` once
scope stabilizes; the bundles are served at stable URLs
and will be aggressively cached by repeat-visit browsers.
- Reduce the 178 dev-mode failures by either making source
specs import a shared ko surface instead of bundling their
own copies, or by shipping a "dev-ko" bundle that the
source specs can reuse.
- Document `/tests` + the versioned URL contract in
`tko.io/public/agents/testing.md`.
- Diff view: run the same spec set against two versions and
highlight the delta.
Adversarial pass: verified against the live running page for
both dev mode and `?mode=build&pkg=knockout`. Config summary
in the footer matches the resolved URL; version picker
populates from the npm registry; source-specs checkbox
disables correctly in build mode. Screenshot captured on
both modes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
unpkg is single-region (NY) with known caching + cold-start
latency; jsdelivr is multi-CDN (Cloudflare + Fastly) with
global POPs and better uptime. Same URL shape, pinned-version
+ `latest` semantics identical, zero API change.
Also considered esm.run — pointless for this use case. TKO
ships a pre-bundled IIFE `browser.min.js` at
`dist/browser.min.js`; the on-the-fly ESM transform that
esm.run provides would just parse + re-emit that IIFE.
Swapped URLs:
- Mocha browser build + CSS:
`https://unpkg.com/mocha@10/...`
→ `https://cdn.jsdelivr.net/npm/mocha@10/...`
- Versioned TKO builds (`build.knockout`, `build.reference`):
`https://unpkg.com/@tko/build.${pkg}@${ver}/dist/browser.min.js`
→ `https://cdn.jsdelivr.net/npm/@tko/build.${pkg}@${ver}/dist/browser.min.js`
Left alone:
- `https://registry.npmjs.org/@tko/build.*` — authoritative
registry metadata, jsdelivr doesn't proxy this endpoint.
Verified locally: `/tests?mode=build&pkg=knockout` loads the
jsdelivr-served bundle; `window.ko` wires up; stats
`✓ 977 · ✖ 4 · ○ 21` match the prior unpkg run, confirming
the swap is behaviorally a no-op.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Prior commit (407d59f) claimed "TKO ships a pre-bundled IIFE browser.min.js" and implied that was the only format. Not accurate. Every `@tko/build.*` package ships: - `dist/browser.js` + `dist/browser.min.js` — IIFE (script tag) - `dist/index.mjs` — ESM - `dist/index.cjs` — CommonJS - `dist/common.js` — common/browser-safe The reason `/tests` still uses the IIFE path (via jsdelivr) is not that ESM isn't available — it's that the specs in `builds/{knockout,reference}/spec/*.js` reference `ko.*` as a global (`ko.utils.compareArrays(...)`, etc.), and a script- tag IIFE sets `window.ko` as a side effect. Using esm.run / jsdelivr's ESM endpoint would resolve the same version but return a module object; we'd still need a shim (`import(url).then(m => window.ko = m.default)`) to restore the global that the specs assume. No behavior difference, just an extra step. Revisit if/when specs move to importing ko explicitly — at that point esm.run becomes the better fit because it skips the IIFE wrapper and lets esbuild-like dedupe happen across the page + bundle graphs. No code change in this commit; correcting the record in the git log. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dev-mode source specs had two separate copies of every shared
TKO class (templateEngine, BindingHandler, domData keys, …):
one from the script-tagged `/lib/ko.js`, one bundled from
`@tko/*` imports. Every `instanceof` check that crossed the
boundary failed — 71 "templateEngine must inherit from
ko.templateEngine" errors plus a long tail of downstream
fallout.
This commit drives dev-mode toward a single esbuild module
graph so class identities are preserved end-to-end.
Three changes:
1. **`bundle-tests.mjs`: source bundle inlines the ko build.**
The source-bundle entry now starts with
`import ko from '../../../builds/knockout/src/index.ts';
globalThis.ko = ko`. The build bundle (which only touches
`ko.*` globals) still relies on an external <script>-loaded
ko and is unchanged. Marked per-build via an `inlineKo`
config flag so nothing bleeds between the two.
2. **`bundle-tests.mjs`: explicit `@tko/*` alias.**
Root `tsconfig.json`'s `compilerOptions.paths` map
`@tko/*` → `packages/*/index.ts`, but `builds/` is in the
tsconfig `exclude` list — so imports originating from
`builds/knockout/src/index.ts` (our inline-ko entry) fell
back to the `package.json "module"` field and resolved to
each package's pre-built `dist/index.js`. That inlines a
SECOND copy of every class the specs also bring in via the
tsconfig paths. Result: `templateEngine` / `templateEngine2`
in the output. Fixed by building an explicit `alias` map
at bundler startup that forces every `@tko/*` to its
source `index.ts` regardless of origin.
3. **`bundle-tests.mjs`: new `distToSrcPlugin` esbuild plugin.**
Even with the alias, specs pervasively do
`import { … } from '../dist'` to pull in the package's
built bundle (`packages/bind/spec/bindingAttributeBehaviors
.ts:22`, `packages/binding.core/spec/eventBehaviors.ts:14`,
and dozens more). That relative import isn't an `@tko/*`
alias — it's a direct file reference to the compiled
artifact, which has its own inlined class instances.
Plugin catches `/\.\.\/dist$/` at `onResolve` time and
redirects to the same package's `../src/index.ts` so both
the alias path and the relative path converge on one file.
4. **`tests.astro`: drop the external ko `<script>` when the
source bundle is loading.** The source bundle now brings
its own ko; loading `/lib/ko.js` on top would re-introduce
the second module graph the first three changes worked to
remove.
Verified effects
----------------
Grep of `source-bundle.js` for `templateEngine2` goes from 10
occurrences to 0. Bundle size drops from 2.3 MB → 2.1 MB
(deduped classes). All 71 `templateEngine must inherit from
ko.templateEngine` failures are gone.
Trade-off
---------
Raw failure count moved from ~180 to ~236. The extra ~60
failures are specs that previously depended on dual-class
coincidences to "pass" under the old harness — now those
misalignments surface as real assertion failures instead of
being masked by class-identity crashes. Dark-factory wants
signal over flattery; surfacing real drift is the goal.
Next bucket to work through:
- 31 "Unable to process binding 'text'" — binding-context
variable lookups (`$parent`, `$customProp`) failing inside
descendant bindings; likely context-setup leakage between
specs, not a registration gap.
- 14 "Cannot find filter by the name of: tail / uppercase" —
`@tko/filter.punches` filters registered on the ko inline
instance but the parser spec has its own local filter map
that doesn't see them. Source-level spec issue.
- ~60 assertion drift (e.g. "expected +0 to deeply equal 1"
×20, "expected [Function] to throw" ×16). Each needs
individual triage — no single common cause.
Adversarial pass: verified class dedup in the bundle (grep
`templateEngine2` = 0), templateEngine failures gone from the
Mocha report, dev-mode loads source-bundle without
`/lib/ko.js`, build-mode still uses jsdelivr-loaded ko.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adversarial review converged on the conclusion that the source-spec path under Mocha is a dark-factory hole: Vitest's per-file module isolation is load-bearing for ~50 specs in packages/* that mutate shared singletons (options.filters, options.bindingGlobals, testNode, sinon timers, …), and Mocha's global scope exposes every one of those leaks as a downstream assertion failure. Chasing them spec-by-spec via `restoreAfter` is a multi-day sink with no clear end. The build-bundle path does not have that problem — those specs touch only `ko.*` globals and run cleanly against any published `@tko/build.*` version. That is also the unique value /tests provides over `bunx vitest run`: a URL that lets an agent (or a human) verify a published version in a real browser without cloning the repo. Public-facing UI changes: - `/tests` now defaults to build mode against `build.knockout`. - Radio group drops the "in-tree dev" option; two builds remain (`build.knockout`, `build.reference`). - Suite toggles removed from the UI. build-bundle is the only user-facing suite. - Tagline updated to match: "Pick a published @tko/build.* version. Specs run live in your browser against it." Maintainer path preserved: - `?mode=dev&suites=source` still works for local iteration; hidden `<input>` elements keep the existing loader logic reachable. Not linked from the UI. - `source-bundle.js` still generated by the prebuild; nothing downstream depends on its absence from the page. No functional regressions on the user-facing path — the live page still renders the CDN-loaded ko + build-bundle identically (956 pass / 3 fail / 7.44s on 4.0.1). Follow-up (C): wire CI's Vitest browser matrix to emit an HTML report, host it at /tests/ci-report/ as a static artifact, and add a link to it from /tests. That covers the "does main pass right now" question without re-implementing an isolation-aware runner in the browser. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Keeping a set of strict-improvement changes from the
state-leak chase before rewriting the runner for
iframe-per-spec isolation:
- `packages/utils.parser/spec/identifierBehaviors.ts`:
`restoreAfter(options, 'bindingGlobals')` before mutating
the Ramanujan test value. Under Vitest's per-file isolation
the leak is invisible; this keeps it invisible under any
shared-scope runner too.
- `packages/builder/spec/builderBehaviors.ts`:
`restoreAfter(options, 'filters')` + `'bindingProviderInstance'`
around the throwaway Builder construction. Same motive —
`new Builder({...})` mutates shared `@tko/utils` options as a
side effect; this localizes the damage.
- `builds/knockout/helpers/browser-setup.js`:
- Register `@tko/filter.punches` punctuation filters on the
shared `options.filters` so ad-hoc `new Parser()` instances
created by specs can resolve `| uppercase`, `| tail`, etc.
(Builder registers them on `knockout.options` but Parser
reads the module-global `options`.)
- `afterEach(sinon.restore)` so unscoped `sinon.spy(obj,
'method')` / `useFakeTimers()` state stops leaking into
the next spec.
- `tko.io/scripts/bundle-tests.mjs`:
- Explicit `@tko/*` alias map pinned to `packages/<name>/
src/index.ts` so imports from `builds/` (outside the
tsconfig `paths` scope) also hit source, not dist.
- New `distToSrcPlugin` that rewrites `../dist` and
`../dist/<subpath>` imports in `packages/*/spec/*.ts` to
the matching `../src/*.ts` file. Before this, most specs
carried two copies of every shared TKO class (one from
source, one from built dist) — every `instanceof` check
that crossed the boundary failed.
Net effect on the dev-mode runner: 236 fails → ~219, and the
dual-class noise dropped out (templateEngine-must-inherit
went from 71 to 0). The remaining failures aren't addressable
by more `restoreAfter` patches — they need real per-spec
isolation, which is the next commit's job.
Adversarial pass: three parallel subagents reviewed the
approach. Verdict: abandon spec-by-spec state patching;
switch to iframe-per-spec isolation in the runner. Next
commit will do that. These hygiene fixes survive because
they're correct regardless of runner.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pivot from single-Mocha-over-shared-scope to one iframe per
spec file, fresh module graph each. Vitest-style isolation in
the browser, delivered at /tests.
Shape
-----
- `tko.io/scripts/bundle-tests.mjs`
Emits two output sets side by side:
- `public/tests/build-bundle.js` — version-portable IIFE of
`builds/{knockout,reference}/spec/` against a CDN-loaded
`@tko/build.*` for the "does v4.0.1 still pass?" flow.
- `public/tests/source/<slug>.js` — one ESM chunk per spec
file under `packages/*/spec/**/*.ts` AND
`builds/{knockout,reference}/spec/**/*.{js,ts}`, with
`format: esm` + `splitting: true` so shared deps dedupe
into chunks. A generated `public/tests/source/_wrappers/
<slug>.ts` is the esbuild entry: inlines the knockout
build first (one module graph with the spec's `@tko/*`
imports), then the shared browser-setup, then the spec.
- `public/tests/source/manifest.json` — every slug/file/suite
triple, consumed by the parent page to enumerate iframes.
- `tko.io/public/tests-frame.html` (new static file)
Per-spec iframe harness. Loads Mocha as a classic script,
`mocha.setup('bdd')`, dynamic-imports `/tests/source/
<slug>.js` from a URL param, runs Mocha, streams runner
events to the parent via `postMessage` (`start`, `pass`,
`fail`, `pending`, `end`, `import-error`). Lives at root
because Astro's `/tests/` page route would 404 a nested
`public/tests/frame.html`.
- `tko.io/src/pages/tests.astro`
Rewritten. The entire chrome + live report is a TKO view
model — observables, pureComputeds, `data-bind` every
binding (text, foreach, visible, submit, click, attr,
style, options, value, textInput). Dogfoods the framework
on its own docs site.
- Header chips (pass/fail/pending/total/elapsed) tick live
at 250ms.
- Progress bar tracks done/total.
- Per-spec suite rows with click-to-expand; failed suites
auto-expand with icon + title + duration + red error box.
- URL-driven config: `?suite=build|source`, `?pkg=knockout|
reference`, `?ver=<v>|latest`, `?grep=<rx>`, `?pool=<N>`.
- Default: source mode (iframe-per-spec).
Gotchas fixed along the way
---------------------------
- Page uses `/lib/ko.js` (knockout-compat) for the UI, not
`/lib/tko.js` (reference). Reference auto-discovers custom
elements anywhere in the document; specs register
`<test-component>` against the CDN ko and reference-page
would try to resolve them too.
- Page script wraps in an IIFE; otherwise top-level `const ko`
collides with the `ko` identifier the knockout IIFE leaves
in global scope.
- `applyBindings` targets `#app`, not `document.body`, so
Mocha + iframe hosts sit outside the bound region.
- Iframe harness lives at `/tests-frame.html`, not under
`/tests/` — Astro's page route for `/tests/` 404s any
nested `public/tests/*.html` file in dev.
Numbers
-------
Single-bundle source mode before this commit: ~219 failures
out of ~2669 tests — most state leaks cascading through the
shared module scope.
Iframe-per-spec source mode after this commit: **48 failures
out of 2528 tests** (full corpus — source + build specs all
isolated), **16 seconds** end to end with pool=4. The 170+
failure delta was harness noise, not product drift.
The remaining 48 are actual spec behaviour to triage — not
state-leak cascades. Next bucket if/when we want to push the
page toward green.
Adversarial pass: three parallel subagents agreed this was
the right pivot (two independently recommended iframe
isolation or abandoning source mode). Verified against the
live running page — page header reads "source specs · pool 4",
chips tick to 2448/48/38, 2528/2528, elapsed 16.0s.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four of the five buckets identified by the parallel subagent
triage land here. Each root cause is genuinely distinct, so
each fix is scoped to exactly its cohort.
Bucket A (6 fails · "ko is not defined" at iframe import time)
---------------------------------------------------------------
Six `builds/knockout/spec/*.js` files reference `ko.*` at
module-top (template-engine registrations, option captures,
jQuery-related setup). The auto-generated wrapper assigned
`globalThis.ko = ko` as a top-level statement AFTER the
spec imports, but ESM hoists imports over top-level code —
so the spec module evaluated with `ko` still undefined and
threw `ReferenceError` before any `describe` registered.
Fix: pre-module `<script src="/lib/ko.js">` in
`public/tests-frame.html`. Synchronous IIFE runs before the
`<script type="module">` block, putting `ko` on the global
before any spec module evaluates. The wrapper's later
reassignment to the source-bundled ko still wins for test
bodies that rely on class-identity across the spec's
`@tko/*` imports.
Buckets D + E (34 fails · `.focus()` in hidden iframes)
-------------------------------------------------------
`packages/binding.core/spec/hasfocusBehaviors.ts` (30) and
`packages/binding.foreach/spec/eachBehavior.ts` focus block
(4) all fail with "expected false to equal true" or
"<body> vs <input>" on `document.activeElement` assertions.
Chromium's focus model requires a focusable target window;
`.iframe-hidden` was `width:0;height:0;opacity:0` which made
the iframe unfocusable, and every `.focus()` inside silently
failed. Playwright CI masks this by auto-focusing the page.
Fix:
- CSS: `.iframe-hidden` becomes `position:absolute;left:
-10000px;top:-10000px;width:800px;height:600px;border:0`.
Off-screen but with real layout + normal opacity + default
pointer-events. Browser treats the iframe window as
focusable; OS never sees it.
- Runner: after each iframe `load`, call
`iframe.contentWindow?.focus()`. Chromium then routes
focus events normally within the iframe.
Bucket C (3 fails · component-slot whitespace)
----------------------------------------------
`packages/binding.component/spec/componentBindingBehaviors.
ts` slot tests compared `.innerText.trim()` against
normalized strings, but `.trim()` only strips leading/
trailing whitespace; the template source has literal
newlines between slotted nodes that stay as text nodes in
a real browser's DOM. Changed those three assertions to
`.innerText.replace(/\s+/g, ' ').trim()` — the same
pattern already in use elsewhere in the same file.
Bucket F (3 fails · `<!-- else -->` whitespace)
-----------------------------------------------
`packages/binding.if/spec/elseBehaviors.ts` intentionally
keeps whitespace around `<!-- else -->` so the comment
parses as a binding; but the selected branch then carries
that leading/trailing space into a text node. Assertions
used `expect(.innerText).to.equal('abc')` against actual
`'abc '`. Added `.trim()` to the three affected
assertions (already HappyDOM-skipped for the same reason).
Deferred (2 fails, separate commits)
------------------------------------
Bucket B (1): `builds/reference/spec/bindingGlobalsBehavior.js`
— wrapper inlines the knockout build first which sets
`options.bindingGlobals = globalThis`, leaving the reference
spec seeing `Window` prototype instead of `null`. Needs
per-spec bundler dispatch (knockout build vs reference
build) rather than always-knockout; deferring to keep this
commit scoped to harness + spec-hygiene fixes.
Bucket G (1): `packages/utils.parser/spec/parserBehaviors.
ts` anonymous-function test. Parser source still throws on
the `function` token (Parser.ts:736); spec expects a throw
but gets none under the iframe runner. Needs trace to
determine whether iframe context swallows the throw or
whether a code path is skipping it; deferring to look at
separately.
Adversarial pass: three parallel `Explore` subagents
triaged the 48-fail bucket independently. They converged
on the same classifications (class-identity vs focus vs
whitespace vs init-order) and their proposed fixes match
the ones landed here. Each subagent's false positives
(e.g. one proposed a `ko-global.ts` helper module; user
pointed out a `<script>` tag in the HTML is simpler)
corrected before implementation.
Expected effect: 48 iframe-mode failures drop to ~2
(buckets B + G).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
All 42 remaining focus-related failures resolved. Source mode now runs clean end-to-end: 2708 passing, 0 failing, 42 pending (pre-existing `it.skip`s), 11.9s total runtime. Root cause was twofold: 1. **esbuild chunk ordering moved ko/setup AFTER spec modules.** The previous per-spec wrapper embedded `import ko from …; import browser-setup; import spec` + a top-level `globalThis.ko = ko`. ESM hoisting already put top-level code after all imports, but esbuild's code-splitter then reordered the *import* lines too — the spec's chunk (with `beforeEach(prepareTestNode)` at chunk-top) landed above the ko + browser-setup chunks. `prepareTestNode` was undefined when the spec chunk evaluated, and a `ReferenceError` cascade killed 6 specs plus whatever they transitively imported. Fix: pull ko + browser-setup out of every wrapper into a dedicated IIFE `public/tests/source/setup.js` loaded via a classic `<script>` tag in `tests-frame.html` BEFORE the spec's `<script type="module">`. Order is no longer at the mercy of the bundler — the IIFE finishes synchronously, spec module then evaluates with all globals ready. (Mocha itself also loads ahead of setup.js now, since browser-setup.js registers `before()` / `afterEach()` hooks at module-top and needs the BDD UI already installed.) 2. **Chromium denies `iframe.contentWindow.focus()` system focus to off-screen / zero-area iframes.** The old `.iframe-hidden` CSS (`width:0;height:0;opacity:0` or `left:-10000px;width:800px`) got the iframe rendered but not in the viewport; `document.hasFocus()` inside stayed false; `focusin` / `focusout` events never fired when specs called `element.focus()`; `document.activeElement` assertions failed against expected input nodes. 34 tests under `packages/binding.core/spec/hasfocusBehaviors.ts`, `builds/knockout/spec/defaultBindings/hasfocusBehaviors.js`, and the focus block of `packages/binding.foreach/spec/eachBehavior.ts` all cascaded from this. Adversarial research converged (testing-library#553, Cypress#8111, jsdom#2996): "focus events don't fire in unfocused windows" is a known Chromium/Blink invariant, not a harness bug. The only workable answer is to give the iframe a real viewport-intersecting layout box. Fix: added a visible `#workarea` region at the bottom of `/tests`. Spec iframes mount there — full width, 360px tall, bordered, white background — and are naturally focusable. The label above the box names the currently- running spec file so a visitor watches each one flash by in the sandbox. Visible running doubles as a dark-factory signal: no more "is anything happening?" question. A synthetic focus-event shim in `browser-setup.js` was explored in parallel (patches `HTMLElement.prototype.focus` / `.blur` to dispatch `focusin` / `focusout` after native call, gated on `window.parent !== window`). Kept as a belt-and-braces layer — harmless when the workarea gives real focus, catches any future regression if layout changes demote the iframe below focusability threshold. (Also tried: pool=1 + 1×1 iframe, pool=1 + offscreen, `iframe.contentWindow.focus()` + `iframe.focus()`, all before landing on "just give it a real layout box".) Other changes bundled here: - Three whitespace-sensitive specs (bucket C + F): `.innerText.trim()` → `.innerText.replace(/\s+/g,' ').trim()` in component-slot + if/else tests. Real-browser DOM keeps template-source newlines as text nodes; assertions that only stripped edges failed on the internal whitespace. - `ko.js` `<script>` tag in `tests-frame.html` provides a prebuilt `globalThis.ko` before any module evaluation (the wrapper's source ko then reassigns over it for class- identity in spec bodies). Specs that touch `ko.*` at module-top now find it ready. - `pool=1` default — each iframe holds sole system focus for the duration of its spec. Pool>1 caused last-focused iframe to win; the other N-1 concurrent specs lost focus mid-run. `?pool=4` still works via URL if someone wants parallel for a non-focus subset. - Three-agent parallel adversarial triage of the 48-fail bucket before any fix landed. Each agent investigated a different axis (ecosystem research, fix architectures, diagnostic drilldown); the converging recommendation was the workarea + setup-IIFE combo. Saved a lot of whack-a-mole. - New memory file `reference_playwright_cli.md` records that browser automation in this repo is `bunx @playwright/cli` (not `playwright-cli`, not `npx playwright`) so future sessions stop guessing. Adversarial pass: three parallel Explore subagents reviewed root causes independently; web research confirmed the focus-in-unfocused-window invariant across browsers. Live verification via `bunx @playwright/cli` on the running page: 2708 pass / 0 fail / 42 pending (pre-existing skips) across 145 spec files in 11.9s. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Simplify-skill review surfaced a mix of quality + efficiency
issues in the previous commit. Applied the real fixes and
restored parallelism for specs that don't need focus.
Split runner (the big one)
--------------------------
Running the full suite with `pool=1` was correct but slow —
unnecessary for the 140 of 145 specs that never touch focus.
Bundler now grep-scans each spec for focus patterns
(`hasfocus`, `.focus(`, `.blur(`, `focusin`, `focusout`,
`activeElement`) and emits `needsFocus: bool` into
`public/tests/source/manifest.json`. Runner splits into two
phases:
- Phase 1: the 140 non-focus specs run in parallel (default
pool=4) inside a tiny off-viewport hidden host. No focus
manipulation — Chromium's focus-contention problem
(several concurrent `contentWindow.focus()` calls, only
last winner) is irrelevant when nobody asks for focus.
- Phase 2: the 5 focus-needing specs run serially inside
the visible #workarea. Each gets sole system focus for
the duration of its spec; `contentWindow.focus()` lands,
`focusin` / `focusout` events fire, `document.activeElement`
updates.
5 files tagged today:
packages/binding.core/spec/hasfocusBehaviors.ts
packages/binding.core/spec/textInputBehaviors.ts
packages/binding.foreach/spec/eachBehavior.ts
builds/knockout/spec/defaultBindings/hasfocusBehaviors.js
builds/knockout/spec/defaultBindings/textInputBehaviors.js
Other simplify-skill findings fixed
-----------------------------------
- `page.stats.done` changed from `ko.observable(0)` + manual
increments to `ko.pureComputed(() => pass()+fail()+pending())`.
Removes three call sites that were keeping a derivable
value in sync.
- `tick` interval now stops itself once `stats.end()` is set.
Previously kept firing 4× / sec indefinitely after the suite
finished, re-evaluating `elapsedLabel` with identical output.
- `runOne`'s 30 s safety `setTimeout` is now tracked in
`safetyTimer` and `clearTimeout`'d inside `finish()`. The
old version left up to 145 pending timers alive concurrently
across a run (pool=1 × 145 specs); each was a no-op by the
time it fired but still occupied event-loop + memory.
- `bundle-tests.mjs` now runs `buildBuildBundle`,
`buildSetupBundle`, and `buildSourceBundles` via
`Promise.all` instead of awaiting each. Independent esbuild
invocations; ~2-3× faster rebuild end-to-end.
- Wrapper file writes skip if content is already identical —
previously updated mtime on every rebuild and churned
downstream file watchers for no reason.
- Focus shim in `browser-setup.js` is now idempotent: guards
on `HTMLElement.prototype.__tkoFocusPatched` so re-importing
`browser-setup.js` doesn't wrap the wrap.
Simplify findings skipped (not worth it)
----------------------------------------
- "Slug path helper" — three `path.relative(...).replace(/\\/
g, '/')` call sites. One-liner duplication; a helper adds
indirection without clarity.
- "Shared message-type constants" — six strings across sender +
receiver. Hoisting to a `messages.js` for 6 strings is overkill
when both endpoints already type them the same way.
- "`<section>` wrapper in workarea markup" — semantic label +
layout spacing; removing it requires re-styling the label
row, no net win.
- "Focus shim hot-path guard on `document.hasFocus()`" — can't
reliably skip synthetic dispatch (both native and synthetic
paths must fire when system focus is unclear), and the cost
is tiny compared to the rest of a focus spec.
- "Setup bundle 870 KB re-parsed per iframe" — inherent to the
inlined ko build we need for class identity; splitting would
re-introduce the esbuild chunk-ordering hazard that the
setup-as-IIFE architecture exists to avoid.
Adversarial pass: three parallel Explore subagents reviewed
the diff for reuse, quality, and efficiency. Aggregated
findings drove this commit. Live verification via
`bunx @playwright/cli` on the running page: 2708 pass / 0
fail / 42 pending in 10.1 s (was 11.9 s in the pool=1-
everything variant).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add "Tests" pill next to the "Playground" pill in the site header. Same chip styling. Gives any visitor (human or agent) a one-click path to the live in-browser spec runner. 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 41 minutes and 45 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 (2)
📝 WalkthroughWalkthroughAdds an in-browser Mocha test ecosystem: a browser test setup module that exposes test globals and patches focus/it semantics, per-spec iframe harness and bundling tooling, a /tests UI to run source/build suites, and small spec fixes to improve isolation and whitespace resilience. Changes
Sequence DiagramssequenceDiagram
actor User
participant TestPage as tko.io /tests Page
participant Manifest as /tests/source/manifest.json
participant Iframe as Spec Iframe
participant Parent as Parent Window
User->>TestPage: Load /tests?suite=source
TestPage->>Manifest: Fetch manifest.json
Manifest-->>TestPage: Return specs with needsFocus flag
rect rgba(200,150,100,0.5)
Note over TestPage,Iframe: Phase 1 — Parallel offscreen (no focus)
loop for each non-focus spec
TestPage->>Iframe: Create tiny offscreen iframe (spec=slug)
Iframe->>Iframe: Load /tests/source/setup.js and browser-setup
Iframe->>Iframe: import /tests/source/{slug}.js
Iframe->>Iframe: Run Mocha tests
Iframe-->>Parent: postMessage(pass/fail/pending/end)
Parent->>TestPage: Aggregate results
end
end
rect rgba(150,200,100,0.5)
Note over TestPage,Iframe: Phase 2 — Serial visible (focus required)
loop for each focus-requiring spec
TestPage->>Iframe: Create visible iframe, focus it (spec=slug)
Iframe->>Iframe: Load setup + spec, run Mocha with focus events patched
Iframe-->>Parent: postMessage(pass/fail/pending/end)
Parent->>TestPage: Update UI & stats
end
end
TestPage->>User: Display final aggregated results
sequenceDiagram
actor User
participant TestPage as tko.io /tests Page
participant NPM as npm Registry
participant CDN as jsDelivr
participant Mocha as Mocha (browser)
User->>TestPage: Load /tests?suite=build
TestPage->>NPM: Fetch `@tko/build.<pkg>` versions
NPM-->>TestPage: Return versions
TestPage->>CDN: Load Mocha and selected `@tko/build` bundle
CDN-->>TestPage: Scripts loaded
TestPage->>TestPage: Load /tests/build-bundle.js
TestPage->>Mocha: mocha.run()
loop each spec in bundle
Mocha->>Mocha: Execute test
Mocha-->>TestPage: Emit runner events (pass/fail/pending/end)
TestPage->>User: Update aggregated stats
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 585c32828e
ℹ️ 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".
|
|
||
| self.onSubmit = () => { | ||
| const next = new URLSearchParams() | ||
| if (self.suite() !== 'build') next.set('suite', self.suite()) |
There was a problem hiding this comment.
When the page is opened in build mode (/tests?suite=build), submitting the form drops the suite query param because this branch only writes it when suite !== 'build'. On reload, Page() defaults to source if suite is absent, so clicking Run immediately exits build mode and ignores the selected build/version settings. This breaks the primary build-mode interaction loop.
Useful? React with 👍 / 👎.
Comment on lines
+193
to
+194
| for (const spec of specs) { | ||
| lines.push(`import '${path.relative(outputDir, spec).replace(/\\/g, '/')}'`) |
There was a problem hiding this comment.
This loop inlines every build spec into build-bundle.js, including builds/reference specs that import local modules (.. and ../dist/browser.min). Because those imports are bundled from the checkout, /tests?suite=build&pkg=...&ver=... is no longer testing only the selected CDN artifact; part of the run executes local build code, which can mask or misattribute published-version regressions.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new in-browser Mocha runner to the tko.io docs site at /tests, including a bundling pipeline and harness pages so the TKO spec suite can be executed directly in a real browser (either against in-tree source or against a published build via jsDelivr).
Changes:
- Add
/testsparent page UI (TKO-driven) plustests-frame.htmliframe harness to execute specs and aggregate results. - Add
tko.io/scripts/bundle-tests.mjsand wire it intotko.ioprebuild to generatepublic/tests/*bundles + a manifest. - Add browser-specific spec setup + a few spec hygiene fixes (state restoration, whitespace normalization) uncovered by the runner.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tko.io/src/pages/tests.astro | New /tests page UI + orchestrator for build/source modes and iframe scheduling. |
| tko.io/src/components/Header.astro | Adds “Tests” link to site header nav. |
| tko.io/scripts/bundle-tests.mjs | New bundler script producing build/source bundles and a manifest for the runner. |
| tko.io/public/tests-frame.html | New per-spec iframe harness loading mocha + setup + the spec chunk and posting results to parent. |
| tko.io/package.json | Extends prebuild to generate the /tests bundles. |
| tko.io/.gitignore | Ignores generated public/tests/ output. |
| packages/utils.parser/spec/identifierBehaviors.ts | Restores mutated options to prevent cross-spec state leakage in the new runner. |
| packages/builder/spec/builderBehaviors.ts | Restores shared options fields to prevent cross-spec state leakage in the new runner. |
| packages/binding.if/spec/elseBehaviors.ts | Makes innerText assertions whitespace-tolerant in real browsers. |
| packages/binding.component/spec/componentBindingBehaviors.ts | Makes innerText assertions whitespace-tolerant in real browsers. |
| builds/knockout/helpers/browser-setup.js | Adds browser-mode test setup (globals, focus polyfill, sinon cleanup, ctx-style it shim). |
Comment on lines
+465
to
+466
| const rx = new RegExp(page.grep().trim(), 'i') | ||
| specs = specs.filter(s => rx.test(s.file) || rx.test(s.slug)) |
There was a problem hiding this comment.
grep is compiled via new RegExp(page.grep().trim(), 'i') without guarding against invalid patterns. An invalid regex (e.g. () will throw and abort source-mode execution. Wrap the RegExp construction in try/catch and surface a friendly validation error (or treat the input as a literal substring match when invalid).
Suggested change
| const rx = new RegExp(page.grep().trim(), 'i') | |
| specs = specs.filter(s => rx.test(s.file) || rx.test(s.slug)) | |
| const grep = page.grep().trim() | |
| try { | |
| const rx = new RegExp(grep, 'i') | |
| specs = specs.filter(s => rx.test(s.file) || rx.test(s.slug)) | |
| } catch (e) { | |
| const literal = grep.toLowerCase() | |
| specs = specs.filter(s => | |
| s.file.toLowerCase().includes(literal) || s.slug.toLowerCase().includes(literal) | |
| ) | |
| } |
Comment on lines
+473
to
+493
| window.addEventListener('message', e => { | ||
| const m = e.data | ||
| if (!m || !m.slug) return | ||
| const spec = specs.find(s => s.slug === m.slug) | ||
| const key = spec?.file || m.slug | ||
| const suite = page.ensureSuite(key) | ||
| if (m.type === 'start') { | ||
| page.stats.total(page.stats.total() + m.total) | ||
| suite.running(true) | ||
| } else if (m.type === 'pass') { | ||
| page.recordTest(key, 'pass', m.title, m.duration) | ||
| } else if (m.type === 'fail') { | ||
| page.recordTest(key, 'fail', m.title, m.duration, m.err, m.stack) | ||
| } else if (m.type === 'pending') { | ||
| page.recordTest(key, 'pending', m.title) | ||
| } else if (m.type === 'end') { | ||
| suite.running(false) | ||
| } else if (m.type === 'import-error') { | ||
| page.recordTest(key, 'fail', '(iframe import error)', 0, m.err) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The message event handler accepts data from any origin/source and trusts e.data fields. Since iframes use postMessage(..., '*'), another window/frame could spoof results or prematurely end a spec by posting a matching {slug,type} message. Please restrict handling to e.origin === location.origin and e.source belonging to one of the runner iframes (track iframe.contentWindow per slug), and consider using a non-wildcard targetOrigin for postMessage.
Comment on lines
+6
to
+21
| // Build mode (default) | ||
| // Load a published `@tko/build.*` from jsdelivr and run | ||
| // `tests/build-bundle.js`. Single Mocha run against the | ||
| // built artifact. Version-portable. | ||
| // | ||
| // Source mode (URL opt-in: `?suite=source`) | ||
| // Spawn one iframe per spec file in `tests/source/manifest.json`. | ||
| // Each iframe has its own window + module graph — fresh ko, | ||
| // fresh options, fresh DOM — so state never leaks between | ||
| // specs. Mocha events stream into the parent via postMessage; | ||
| // the parent aggregates into a TKO view model and data-binds | ||
| // the reactive report. | ||
| // | ||
| // URL params: | ||
| // ?suite=build|source default: build | ||
| // ?pkg=knockout|reference build mode only; default: knockout |
There was a problem hiding this comment.
Header comment disagrees with runtime behavior: it says build mode is the default and source mode is opt-in, but the code defaults suite to 'source' and the PR description also states source is default. Please update the comment (and the URL params block) to match the actual defaults to avoid confusion.
Comment on lines
+200
to
+203
| <div class="suite-head" data-bind="click: toggle"> | ||
| <span class="name" data-bind="text: file"></span> | ||
| <span class="meta" data-bind="text: pass() + ' / ' + total()"></span> | ||
| </div> |
There was a problem hiding this comment.
.suite-head is a clickable <div> with no keyboard affordances (no role/tabindex/keydown handling). This makes the suite expand/collapse inaccessible to keyboard and assistive tech users. Prefer a <button> for the header (or add role="button", tabindex="0", and Enter/Space key handling).
Suggested change
| <div class="suite-head" data-bind="click: toggle"> | |
| <span class="name" data-bind="text: file"></span> | |
| <span class="meta" data-bind="text: pass() + ' / ' + total()"></span> | |
| </div> | |
| <button type="button" class="suite-head" data-bind="click: toggle, attr: { 'aria-expanded': open }"> | |
| <span class="name" data-bind="text: file"></span> | |
| <span class="meta" data-bind="text: pass() + ' / ' + total()"></span> | |
| </button> |
Comment on lines
+144
to
+164
| /* Spec iframes need full viewport-sized layout for Chromium | ||
| to grant `iframe.contentWindow.focus()` true system focus. | ||
| Tiny (1×1 or 0×0) or off-viewport (left:-10000px) iframes | ||
| get denied — `document.hasFocus()` inside stays false, | ||
| `focusin`/`focusout` events never fire, 34 hasfocus tests | ||
| fail. Full-viewport iframe with `opacity:0` + | ||
| `pointer-events:none` + `z-index:-1` gives the iframe the | ||
| layout area Chromium needs while remaining invisible and | ||
| non-interactive to the user. Pool=1 means at most one | ||
| such iframe is live at a time, so nothing stacks on top | ||
| of the page chrome. */ | ||
| .iframe-hidden { | ||
| position: fixed; | ||
| inset: 0; | ||
| width: 100vw; | ||
| height: 100vh; | ||
| border: 0; | ||
| opacity: 0; | ||
| pointer-events: none; | ||
| z-index: -1; | ||
| } |
There was a problem hiding this comment.
The CSS class .iframe-hidden is defined but never applied; hidden iframes are currently styled via iframe.style.cssText in runOne(). Consider removing the unused class (or switching runOne() to apply it) to keep the page CSS aligned with the implementation.
Suggested change
| /* Spec iframes need full viewport-sized layout for Chromium | |
| to grant `iframe.contentWindow.focus()` true system focus. | |
| Tiny (1×1 or 0×0) or off-viewport (left:-10000px) iframes | |
| get denied — `document.hasFocus()` inside stays false, | |
| `focusin`/`focusout` events never fire, 34 hasfocus tests | |
| fail. Full-viewport iframe with `opacity:0` + | |
| `pointer-events:none` + `z-index:-1` gives the iframe the | |
| layout area Chromium needs while remaining invisible and | |
| non-interactive to the user. Pool=1 means at most one | |
| such iframe is live at a time, so nothing stacks on top | |
| of the page chrome. */ | |
| .iframe-hidden { | |
| position: fixed; | |
| inset: 0; | |
| width: 100vw; | |
| height: 100vh; | |
| border: 0; | |
| opacity: 0; | |
| pointer-events: none; | |
| z-index: -1; | |
| } |
Comment on lines
+27
to
+28
| the key of a spec module under /tests/source/. The spec's | ||
| wrapper imports the knockout build, browser-setup, and the |
There was a problem hiding this comment.
The block comment says the per-spec wrapper imports knockout + browser-setup + the spec, but the current architecture loads /tests/source/setup.js via a classic <script> tag and the wrapper only imports the spec module. Please update this comment to reflect the current setup so future changes don't rely on outdated assumptions.
Suggested change
| the key of a spec module under /tests/source/. The spec's | |
| wrapper imports the knockout build, browser-setup, and the | |
| the key of a spec module under /tests/source/. This iframe | |
| first loads `/tests/source/setup.js` via a classic `<script>` | |
| tag, then the inline module script imports only the selected |
Comment on lines
+25
to
+62
| import { promises as fs } from 'node:fs' | ||
| import path from 'node:path' | ||
| import { fileURLToPath } from 'node:url' | ||
| import { glob } from 'node:fs/promises' | ||
| import * as esbuild from 'esbuild' | ||
|
|
||
| const scriptDir = path.dirname(fileURLToPath(import.meta.url)) | ||
| const siteRoot = path.resolve(scriptDir, '..') | ||
| const repoRoot = path.resolve(siteRoot, '..') | ||
| const outputDir = path.join(siteRoot, 'public', 'tests') | ||
| const sourceOutputDir = path.join(outputDir, 'source') | ||
|
|
||
| // Exclude non-spec siblings that end up under `spec/` directories. | ||
| const EXCLUDE = /[\\/](__screenshots__|fixtures|helpers)[\\/]/ | ||
|
|
||
| // `../dist` → `../src` rewrite plugin. Specs pervasively import | ||
| // from the package's built bundle; redirecting to source keeps a | ||
| // single module graph. | ||
| const distToSrcPlugin = { | ||
| name: 'dist-to-src', | ||
| setup(build) { | ||
| build.onResolve({ filter: /(^|\/)\.\.\/dist(\/[^/]+)?$/ }, args => { | ||
| if (!args.importer.includes(`${path.sep}packages${path.sep}`)) return | ||
| const match = args.path.match(/\.\.\/dist(?:\/([^/]+))?$/) | ||
| const subpath = match?.[1] | ||
| const src = subpath | ||
| ? path.resolve(path.dirname(args.importer), '../src', subpath + '.ts') | ||
| : path.resolve(path.dirname(args.importer), '../src/index.ts') | ||
| return { path: src } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| async function collectSpecs(patterns) { | ||
| const specs = [] | ||
| for (const pattern of patterns) { | ||
| for await (const match of glob(pattern, { cwd: repoRoot })) { | ||
| if (EXCLUDE.test(match)) continue |
There was a problem hiding this comment.
bundle-tests.mjs relies on glob from node:fs/promises. The repo doesn't pin a Node version for the docs build workflow (deploy-docs.yml doesn't run setup-node), so this can break depending on the runner's preinstalled Node version. Consider using a dependency with a stable API (e.g. fast-glob/tinyglobby) or run this script under Bun and use Bun.Glob, so the runtime is pinned by .tool-versions.
Comment on lines
+446
to
+447
| const koUrl = `https://cdn.jsdelivr.net/npm/@tko/build.${page.pkg()}@${page.ver()}/dist/browser.min.js` | ||
| await loadScript(koUrl) |
There was a problem hiding this comment.
In build mode, pkg and ver come straight from location.search and are interpolated into a jsDelivr script URL. As-is, a crafted query string could cause this page to load and execute an arbitrary third-party script. Please validate/allowlist pkg (knockout|reference) and constrain ver (e.g., latest or a semver-like value) before constructing koUrl, falling back to safe defaults on invalid input.
![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: 7
🤖 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/browser-setup.js`:
- Around line 1-173: Run the Biome formatter on this JavaScript module and
commit the resulting changes so lint-and-typecheck no longer reports formatting
diffs; specifically, format the file that defines globals like
sharedOptions.filters, HE.__tkoFocusPatched, the before() hook adjusting
ko.options.jsxCleanBatchSize, and the it-wrapping logic (wrap/origIt/wrappedIt),
then stage and commit the formatted file so the CI lint step passes.
In `@tko.io/public/tests-frame.html`:
- Around line 71-82: Replace all uses of parent.postMessage(..., '*') in the
test harness with a same-origin targetOrigin (e.g. window.location.origin) so
messages are only delivered to the embedding /tests page; update every call site
referenced in this diff — the runner.on('pass', ...), runner.on('fail', ...),
runner.on('pending', ...), runner.on('end', ...), the initial
parent.postMessage({ type: 'start', ... }), and the catch block
parent.postMessage({ type: 'import-error', ... }) — to pass the same-origin
string instead of '*'.
In `@tko.io/src/pages/tests.astro`:
- Around line 473-493: The message handler registered with
window.addEventListener('message', ...) trusts any postMessage payload; tighten
it by validating the event origin and source before processing: check
event.origin (and optionally event.source === expectedIframe.contentWindow or
verify a known allowed origins list) and only proceed when the origin/source
match; then perform the existing logic that looks up specs (specs.find(...)),
computes key, calls page.ensureSuite(key), updates suite.running(...) and calls
page.recordTest(...). Apply the same validation in the other matching listener
block that handles messages on lines 538-543 so only messages from the intended
iframe are accepted.
- Line 325: The inline parseInt can produce NaN (e.g. ?pool=abc) and Math.max(1,
NaN) yields NaN, so replace the expression used to set self.pool with a
safe-parse + clamp: read qs.get('pool'), parseInt into a local variable (p), if
p is NaN default to a sensible DEFAULT_POOL (e.g. 4), then clamp to a MAX_POOL
(e.g. 16) using Math.min/Math.max before assigning to self.pool; update any
usage that relies on self.pool (e.g. the worker creation logic referenced around
the code that creates workers at line creating zero workers) to use this
validated value. Ensure you reference the symbols qs.get, parseInt, and
self.pool when making the change.
- Around line 371-378: The onSubmit handler currently omits the suite query when
self.suite() === 'build', which drops suite=build on submit; update the onSubmit
function (the self.onSubmit block) to always include the suite parameter (e.g.,
call next.set('suite', self.suite()) unconditionally) so the current suite
selection (including 'build') is preserved when setting location.search.
- Around line 464-466: Wrap the RegExp construction in a try/catch so malformed
grep values from page.grep() don't throw and abort the runner: attempt const rx
= new RegExp(page.grep().trim(), 'i') inside a try block, and in the catch set
specs to [] (or otherwise filter nothing) and record the failure (e.g.,
console.error or push an error to the page/test runner) so the runner continues
and reports a useful error; keep the existing specs = specs.filter(s =>
rx.test(s.file) || rx.test(s.slug)) logic unchanged when the RegExp succeeds.
- Line 545: The safety timeout currently calls safetyTimer = setTimeout(finish,
30000) which lets a hung iframe silently complete; change the timeout handler to
record the timeout as a failure for the current spec/iframe before finishing —
e.g., replace the direct finish call with a small wrapper that logs or pushes a
failure (use the existing finish, recordFailure or testResults append logic and
the current spec/iframe identifier used in this file) and then calls finish(),
and ensure safetyTimer still clears appropriately; reference symbols:
safetyTimer, finish, and the current spec/iframe identifier used in this file.
🪄 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: 1727307e-879e-4452-9138-ccc51ba0d644
📒 Files selected for processing (11)
builds/knockout/helpers/browser-setup.jspackages/binding.component/spec/componentBindingBehaviors.tspackages/binding.if/spec/elseBehaviors.tspackages/builder/spec/builderBehaviors.tspackages/utils.parser/spec/identifierBehaviors.tstko.io/.gitignoretko.io/package.jsontko.io/public/tests-frame.htmltko.io/scripts/bundle-tests.mjstko.io/src/components/Header.astrotko.io/src/pages/tests.astro
Comment on lines
+71
to
+82
| runner.on('pass', t => parent.postMessage({ type: 'pass', slug, title: t.fullTitle(), duration: t.duration }, '*')) | ||
| runner.on('fail', (t, err) => parent.postMessage({ | ||
| type: 'fail', slug, title: t.fullTitle(), duration: t.duration, | ||
| err: (err?.message || String(err)), | ||
| stack: err?.stack | ||
| }, '*')) | ||
| runner.on('pending', t => parent.postMessage({ type: 'pending', slug, title: t.fullTitle() }, '*')) | ||
| runner.on('end', () => parent.postMessage({ type: 'end', slug, stats: runner.stats }, '*')) | ||
| parent.postMessage({ type: 'start', slug, total: runner.total }, '*') | ||
| } catch (err) { | ||
| document.getElementById('err').textContent = 'import failed: ' + (err?.message || err) | ||
| parent.postMessage({ type: 'import-error', slug, err: err?.message || String(err) }, '*') |
Contributor
There was a problem hiding this comment.
Constrain postMessage to the same origin.
The harness is intended to report only to the same-origin /tests page, so avoid broadcasting test titles, failures, and stacks to any embedding parent.
🔒 Proposed tightening
+ const post = message => parent.postMessage(message, location.origin)
const runner = mocha.run()
- runner.on('pass', t => parent.postMessage({ type: 'pass', slug, title: t.fullTitle(), duration: t.duration }, '*'))
- runner.on('fail', (t, err) => parent.postMessage({
+ runner.on('pass', t => post({ type: 'pass', slug, title: t.fullTitle(), duration: t.duration }))
+ runner.on('fail', (t, err) => post({
type: 'fail', slug, title: t.fullTitle(), duration: t.duration,
err: (err?.message || String(err)),
stack: err?.stack
- }, '*'))
- runner.on('pending', t => parent.postMessage({ type: 'pending', slug, title: t.fullTitle() }, '*'))
- runner.on('end', () => parent.postMessage({ type: 'end', slug, stats: runner.stats }, '*'))
- parent.postMessage({ type: 'start', slug, total: runner.total }, '*')
+ }))
+ runner.on('pending', t => post({ type: 'pending', slug, title: t.fullTitle() }))
+ runner.on('end', () => post({ type: 'end', slug, stats: runner.stats }))
+ post({ type: 'start', slug, total: runner.total })
} catch (err) {
document.getElementById('err').textContent = 'import failed: ' + (err?.message || err)
- parent.postMessage({ type: 'import-error', slug, err: err?.message || String(err) }, '*')
+ parent.postMessage({ type: 'import-error', slug, err: err?.message || String(err) }, location.origin)
}📝 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
| runner.on('pass', t => parent.postMessage({ type: 'pass', slug, title: t.fullTitle(), duration: t.duration }, '*')) | |
| runner.on('fail', (t, err) => parent.postMessage({ | |
| type: 'fail', slug, title: t.fullTitle(), duration: t.duration, | |
| err: (err?.message || String(err)), | |
| stack: err?.stack | |
| }, '*')) | |
| runner.on('pending', t => parent.postMessage({ type: 'pending', slug, title: t.fullTitle() }, '*')) | |
| runner.on('end', () => parent.postMessage({ type: 'end', slug, stats: runner.stats }, '*')) | |
| parent.postMessage({ type: 'start', slug, total: runner.total }, '*') | |
| } catch (err) { | |
| document.getElementById('err').textContent = 'import failed: ' + (err?.message || err) | |
| parent.postMessage({ type: 'import-error', slug, err: err?.message || String(err) }, '*') | |
| const runner = mocha.run() | |
| const post = message => parent.postMessage(message, location.origin) | |
| runner.on('pass', t => post({ type: 'pass', slug, title: t.fullTitle(), duration: t.duration })) | |
| runner.on('fail', (t, err) => post({ | |
| type: 'fail', slug, title: t.fullTitle(), duration: t.duration, | |
| err: (err?.message || String(err)), | |
| stack: err?.stack | |
| })) | |
| runner.on('pending', t => post({ type: 'pending', slug, title: t.fullTitle() })) | |
| runner.on('end', () => post({ type: 'end', slug, stats: runner.stats })) | |
| post({ type: 'start', slug, total: runner.total }) | |
| } catch (err) { | |
| document.getElementById('err').textContent = 'import failed: ' + (err?.message || err) | |
| parent.postMessage({ type: 'import-error', slug, err: err?.message || String(err) }, location.origin) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tko.io/public/tests-frame.html` around lines 71 - 82, Replace all uses of
parent.postMessage(..., '*') in the test harness with a same-origin targetOrigin
(e.g. window.location.origin) so messages are only delivered to the embedding
/tests page; update every call site referenced in this diff — the
runner.on('pass', ...), runner.on('fail', ...), runner.on('pending', ...),
runner.on('end', ...), the initial parent.postMessage({ type: 'start', ... }),
and the catch block parent.postMessage({ type: 'import-error', ... }) — to pass
the same-origin string instead of '*'.
Ten findings across chatgpt-codex-connector, Copilot, and
CodeRabbit. Everything lands here except the `node:fs/promises`
`glob` portability concern, which stays as-is (the docs
build already runs under Bun via bun.lock / mise).
1. Biome format [CodeRabbit, blocker]
- `bun run format:fix` on `browser-setup.js`. CI should pass.
- Pre-existing `packages/binding.component/spec/
componentBindingBehaviors.ts` formatting drift also
picked up in the same pass (unrelated to this PR).
2. `suite=build` dropped on Run submit [Codex, P1]
- Page default flipped to `source` earlier in this PR, but
`onSubmit` was still writing `suite` only when
`suite() !== 'build'`. Clicking Run from build mode
therefore stripped the param and reloaded into source.
Inverted the guard: skip writing only when
`suite() === 'source'` (the default).
3. Arbitrary-script XSS via `pkg` / `ver` [Copilot, security]
- Both values were URL-trusted and string-interpolated
straight into the jsdelivr `<script>` src. Added
allowlist validation: `pkg ∈ {knockout, reference}` (else
`knockout`), `ver` matches `/^[\w.-]{1,40}$/` (else
`latest`), `suite ∈ {build, source}` (else `source`). All
three pin to safe defaults on anything unexpected.
4. Unscoped postMessage listeners [Copilot, security]
- Both the top-level source-mode listener and the per-
runOne listener trusted every message and keyed on slug
alone. A nested iframe / opened window could therefore
spoof results or prematurely end a spec. Added two
guards: `e.origin === location.origin` and a `liveFrames`
Set of known contentWindows populated on iframe load /
drained on finish. Messages from anywhere else are
dropped. Per-iframe handler additionally pins
`e.source === iframe.contentWindow`.
5. Invalid grep regex aborts source mode [Copilot]
- `new RegExp(page.grep().trim(), 'i')` was wrapped in a
try/catch; on `SyntaxError` we fall back to case-
insensitive substring matching against `file` and
`slug`. A typed `(` no longer kills the run.
6. Build-mode bundle mixes local source with CDN artifact [Codex, P2]
- `builds/{knockout,reference}/spec/` specs like
`iifeBehavior.js` import `../dist/browser.min`, and a
few reference specs import from `..`. Those relative
imports dragged local checkout code into
`build-bundle.js`, so `/tests?suite=build&pkg=X&ver=Y`
was partly executing local code instead of the selected
CDN version.
- Bundler now greps each candidate spec for
`from\s+['"]\.\.?\/` and drops any that matches.
Remaining corpus covers behavior that exercises only
`ko.*` globals — which is what "test version X" actually
means. Skipped specs still run in source mode.
7. Header comment contradicted runtime default [Copilot]
- Comment said "default: build"; code defaults to source.
Fixed to match.
8. Unused `.iframe-hidden` CSS class [Copilot]
- The class was defined in the `<style>` block but never
applied (iframe styles moved inline into `runOne`).
Removed the dead rule and updated the surrounding
comment.
9. Outdated tests-frame.html comment [Copilot]
- Block comment still claimed the spec wrapper imports
ko + browser-setup + the spec. Actually the wrapper
imports only the spec; ko + setup load via the classic
`<script src="/tests/source/setup.js">` tag above.
Comment updated.
10. `<div class="suite-head">` inaccessible [Copilot, a11y]
- Changed to `<button type="button">` with
`aria-expanded` bound to the observable `open` state.
Button reset styles added so the tree looks the same.
Verified with `bunx @playwright/cli` on the live page after
the fixes: 2708 pass / 0 fail / 42 pending (pre-existing
skips) / 10.4s. Unchanged behavior.
Skipped
- "Use fast-glob / tinyglobby / Bun.Glob instead of
node:fs/promises `glob`." The docs pipeline already runs
under Bun (see `bun.lock`, `mise` pinning); Node-version
drift isn't a realistic risk in this repo's CI. Re-visit
if `deploy-docs.yml` ever switches off Bun.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tko.io/src/pages/tests.astro (2)
327-327:⚠️ Potential issue | 🟠 MajorClamp and default the iframe pool size.
parseIntcan returnNaN, andMath.max(1, NaN)remainsNaN; then the hidden queue creates zero workers. Very large values can also spawn excessive iframe concurrency.🐛 Proposed fix
- self.pool = Math.max(1, parseInt(qs.get('pool') || '4', 10)) + const requestedPool = parseInt(qs.get('pool') || '4', 10) + self.pool = Number.isFinite(requestedPool) + ? Math.min(16, Math.max(1, requestedPool)) + : 4#!/bin/bash # Demonstrate why invalid pool input currently collapses worker creation. node - <<'NODE' for (const raw of ['abc', '0', '4', '1000000']) { const pool = Math.max(1, parseInt(raw || '4', 10)) console.log({ raw, pool, workerCount: Array.from({ length: pool }).length }) } NODE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tko.io/src/pages/tests.astro` at line 327, The self.pool initialization using Math.max(1, parseInt(qs.get('pool') || '4', 10)) can produce NaN or unboundedly large values; update the setter in tests.astro (the line that assigns self.pool) to first parse with Number.parseInt or parseInt, then if Number.isNaN(result) fall back to a default (e.g. 4), and finally clamp the value between a minimum of 1 and a reasonable maximum (e.g. 64) before assigning to self.pool so invalid or extreme qs inputs can't create zero or excessive iframe workers.
570-570:⚠️ Potential issue | 🟠 MajorRecord iframe timeouts as failures.
The timeout currently removes the iframe silently, so a hung spec can disappear and the run can finish without explaining which file timed out.
🐛 Proposed fix
- safetyTimer = setTimeout(finish, 30000) + safetyTimer = setTimeout(() => { + const key = spec.file || spec.slug + page.stats.total(Math.max(page.stats.total(), page.stats.done() + 1)) + page.recordTest(key, 'fail', '(iframe timeout)', 30000, 'No end/import-error event after 30000ms') + page.ensureSuite(key).running(false) + finish() + }, 30000)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tko.io/src/pages/tests.astro` at line 570, The timeout currently just calls finish silently; change the safetyTimer callback so that instead of silently removing the iframe it records a failure and provides context: invoke finish with an explicit timeout/failure error (including the spec/file identifier or URL) so the run records which iframe timed out, and ensure the same logic still clears/destroys the iframe afterwards; update the setTimeout call (safetyTimer = setTimeout(...)) to call finish with an Error or failure result rather than a silent completion and preserve existing iframe cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tko.io/scripts/bundle-tests.mjs`:
- Around line 193-198: Update the RELATIVE_IMPORT pattern used in
bundle-tests.mjs so it matches all relative-import forms (e.g., "from './...'",
bare static "import '../...'", "export '../...'", dynamic "import('./...')", and
require('./...')) rather than only "from" clauses; modify the RELATIVE_IMPORT
constant to a single regex that recognizes the keywords "from", "import" (both
bare and call-form), "export", and "require" when followed by a quote containing
./ or ../, keeping the rest of the logic that reads specs (allSpecs -> specs)
unchanged and leaving the surrounding loop and fs.readFile calls intact.
In `@tko.io/src/pages/tests.astro`:
- Around line 511-513: The import-error branch doesn't increment the overall
test count before recording a failure, so update the handler for m.type ===
'import-error' to increment the total (the same counter updated on 'start'
events) before calling page.recordTest; specifically, ensure you increment the
same stats.total variable (or the function that updates totals) prior to
invoking recordTest(key, 'fail', '(iframe import error)', 0, m.err) so
totals/progress remain consistent.
- Line 450: Build-mode currently passes raw page.grep() into window.mocha.grep()
which can throw for invalid regexes; change the call to mirror source-mode
behavior by trimming the grep text, then attempt to construct/use a RegExp for
grep inside a try/catch (using page.grep() and window.mocha.grep), and on
exception fall back to a substring match handler (e.g., a function that checks
indexOf on the test fullTitle) so invalid patterns don't throw — reference the
page.grep() call and window.mocha.grep usage and align with the fallback logic
used around lines 470–480 in source mode.
---
Duplicate comments:
In `@tko.io/src/pages/tests.astro`:
- Line 327: The self.pool initialization using Math.max(1,
parseInt(qs.get('pool') || '4', 10)) can produce NaN or unboundedly large
values; update the setter in tests.astro (the line that assigns self.pool) to
first parse with Number.parseInt or parseInt, then if Number.isNaN(result) fall
back to a default (e.g. 4), and finally clamp the value between a minimum of 1
and a reasonable maximum (e.g. 64) before assigning to self.pool so invalid or
extreme qs inputs can't create zero or excessive iframe workers.
- Line 570: The timeout currently just calls finish silently; change the
safetyTimer callback so that instead of silently removing the iframe it records
a failure and provides context: invoke finish with an explicit timeout/failure
error (including the spec/file identifier or URL) so the run records which
iframe timed out, and ensure the same logic still clears/destroys the iframe
afterwards; update the setTimeout call (safetyTimer = setTimeout(...)) to call
finish with an Error or failure result rather than a silent completion and
preserve existing iframe cleanup.
🪄 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: 15ea1627-9327-45a9-b056-8c8030151643
📒 Files selected for processing (5)
builds/knockout/helpers/browser-setup.jspackages/binding.component/spec/componentBindingBehaviors.tstko.io/public/tests-frame.htmltko.io/scripts/bundle-tests.mjstko.io/src/pages/tests.astro
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/binding.component/spec/componentBindingBehaviors.ts
- tko.io/public/tests-frame.html
- builds/knockout/helpers/browser-setup.js
Comment on lines
+193
to
+198
| const RELATIVE_IMPORT = /from\s+['"]\.\.?\// | ||
| const specs = [] | ||
| for (const spec of allSpecs) { | ||
| const src = await fs.readFile(spec, 'utf8') | ||
| if (RELATIVE_IMPORT.test(src)) continue | ||
| specs.push(spec) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show build specs with relative imports that the current regex misses.
python3 - <<'PY'
from pathlib import Path
import re
current = re.compile(r"from\s+['\"]\.\.?/")
patterns = [
re.compile(r"^\s*import\s+['\"]\.\.?/", re.M),
re.compile(r"^\s*import\s+.*?\s+from\s+['\"]\.\.?/", re.M),
re.compile(r"^\s*export\s+.*?\s+from\s+['\"]\.\.?/", re.M),
re.compile(r"\bimport\s*\(\s*['\"]\.\.?/"),
re.compile(r"\brequire\s*\(\s*['\"]\.\.?/"),
]
for base in (Path("builds/knockout/spec"), Path("builds/reference/spec")):
if not base.exists():
continue
for path in sorted([*base.rglob("*.js"), *base.rglob("*.ts")]):
text = path.read_text(encoding="utf8")
if any(p.search(text) for p in patterns) and not current.search(text):
print(path)
PYRepository: knockout/tko
Length of output: 93
🏁 Script executed:
cat -n builds/reference/spec/iifeBehavior.js | head -50Repository: knockout/tko
Length of output: 489
🏁 Script executed:
# Check what patterns iifeBehavior.js actually uses
rg -n "(?:import|require|export)" builds/reference/spec/iifeBehavior.js | head -20Repository: knockout/tko
Length of output: 218
🏁 Script executed:
# Validate the proposed regex pattern works correctly
python3 - <<'PY'
import re
# Current regex
current = re.compile(r"from\s+['\"]\.\.?/")
# Proposed regex from the fix
proposed = re.compile(
r"(?:^\s*import\s+['\"]\.\.?\/|^\s*import\s+.*?\s+from\s+['\"]\.\.?\/|^\s*export\s+.*?\s+from\s+['\"]\.\.?\/|\bimport\s*\(\s*['\"]\.\.?\/|\brequire\s*\(\s*['\"]\.\.?\/)",
re.M
)
# Test cases
test_cases = [
"from '../helper'", # Should match both
"import '../helper'", # Should match proposed, not current
"import { x } from '../helper'", # Should match both
"export { x } from '../helper'", # Should match proposed, not current
"await import('../helper')", # Should match proposed, not current
"require('../helper')", # Should match proposed, not current
"import('./helper')", # Should match proposed, not current
"from './helper'", # Should match both
]
for test in test_cases:
c_match = "✓" if current.search(test) else "✗"
p_match = "✓" if proposed.search(test) else "✗"
print(f"{c_match} current | {p_match} proposed: {test}")
PYRepository: knockout/tko
Length of output: 425
Fix regex to catch all relative import patterns without breaking existing matches.
The issue is real: builds/reference/spec/iifeBehavior.js uses import '../dist/browser.min' (line 4), a bare import statement that the current regex misses. However, the proposed regex introduces a critical regression—it stops matching from imports entirely.
Test results show:
- Current regex ✓ matches
from '../...'andfrom './...' - Proposed regex ✗ misses both patterns
The fix needs to preserve the original from pattern and add the missing import forms:
- const RELATIVE_IMPORT = /from\s+['"]\.\.?\//
+ const RELATIVE_IMPORT = /(?:from\s+['"]\.\.?\/|^(?:import|export)\s+['"]\.\.?\/|import\s*\(\s*['"]\.\.?\/|require\s*\(\s*['"]\.\.?\/)/mThis catches: from '...', bare import '../...', export '../...', dynamic import(), and require() without dropping existing functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tko.io/scripts/bundle-tests.mjs` around lines 193 - 198, Update the
RELATIVE_IMPORT pattern used in bundle-tests.mjs so it matches all
relative-import forms (e.g., "from './...'", bare static "import '../...'",
"export '../...'", dynamic "import('./...')", and require('./...')) rather than
only "from" clauses; modify the RELATIVE_IMPORT constant to a single regex that
recognizes the keywords "from", "import" (both bare and call-form), "export",
and "require" when followed by a quote containing ./ or ../, keeping the rest of
the logic that reads specs (allSpecs -> specs) unchanged and leaving the
surrounding loop and fs.readFile calls intact.
Pin the work-area sandbox to the bottom of the viewport so it stays visible as the suite tree scrolls above. Added body padding-bottom to reserve room for the fixed panel — the tail of a long suite list is no longer hidden behind the sandbox. Workarea height trimmed from 360px to 240px (still tall enough for most spec DOM) and the section gets a solid panel background + top border so scroll content doesn't bleed visually beneath it. Verified: 2708 pass / 0 fail / 42 pending / 10.5s. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cut 5s of dead wait: five focus-preservation specs in binding.foreach's eachBehavior waited on a literal 1s setTimeout for foreach's rAF-scheduled re-render; one rAF is enough. Source-mode now 2708/0/42 in ~5.3s (was 11.9s). Work area becomes a 320×200 translucent (opacity 0.6) floating panel pinned bottom-right. Description (why serial-window-focus is required + "disappears when tests complete") overlays the iframe directly with no split header strip. Self-removes on completion. Opacity lifts to 1 on hover. Brand polish: TKO wordmark in the header now links to /, body font switched to Inter, accent recolored to brand blue (#0f62fe / hover #3b82f6).
- Clamp `pool` query param (NaN → 4, capped at 16) so invalid
inputs don't spawn zero workers or excessive concurrency.
- Wrap build-mode `mocha.grep()` in try/catch with substring
fallback, matching source-mode behavior for malformed regex.
- Bump stats.total before recording import-error failures so
progress totals stay consistent with done count.
- Safety timeout now records a failure (with spec identifier)
instead of silently finishing a hung iframe.
- Expand RELATIVE_IMPORT regex in bundle-tests.mjs to catch
bare `import '../x'`, `export ... from`, `import('./x')`, and
`require('./x')` — not just `from` clauses.
- TKO wordmark uses dark-mode color `#dce8ff` (matches landing's
dark-theme logo) instead of accent blue, since /tests is
permanently dark.
- Fix dated claim that window.ko comes from /lib/ko.js <script>;
it's set by the bundled IIFE in /tests/source/setup.js.
- Drop "34 tests fail" specific count (historical, pre-polyfill).
- Collapse duplicate phrasings ("isHappyDom is never true here"
was in both the header and the body).
- Shorten the punctuation-filter, sinon-restore, and ctx-arg
shim comments to one concise paragraph each.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tko.io/src/pages/tests.astro`:
- Around line 456-459: The workarea section remains visible outside "source"
mode; update the same mode check that hides the `#mocha` reporter to also hide the
focus workarea by setting document.getElementById('workarea-section').hidden
based on page.suite() (e.g., hidden = page.suite() !== 'source'), keeping the
existing `#workarea` mounting behavior intact; locate the code around the existing
document.getElementById('mocha') line and add the analogous hide logic for
'workarea-section'.
- Around line 4-12: The header comment currently declares "Build mode (default)"
and refers to "?suite=source" as opt-in, but elsewhere the page actually
defaults to source; update the documentation so the "Two modes:" block matches
the implementation by changing the "Build mode (default)" text to indicate that
Source mode is the default and Build is opt-in (or vice‑versa to match runtime),
and adjust the example URL token reference (change "?suite=source" to the
correct opt‑in token, e.g. "?suite=build", or explicitly state which query value
triggers the non‑default mode) — search for the strings "Two modes:", "Build
mode (default)", "?suite=source", and "tests/source/manifest.json" to locate and
edit the mismatched descriptions.
- Around line 595-623: The per-frame message handler can miss messages because
liveFrames.add(...) is only called in the iframe 'load' handler; move the
registration earlier by adding the iframe's contentWindow to liveFrames as soon
as the iframe is created (or register a placeholder/temporary id) and register
the window.message listener (onMsg) before appending the iframe, then keep the
existing cleanup in finish (which deletes from liveFrames and removes the onMsg
listener) so frames are tracked immediately and still cleaned up when finish()
runs; update the code that currently calls liveFrames.add(...) inside the
iframe.addEventListener('load', ...) block accordingly.
- Around line 497-502: The catch block passes a predicate function to
window.mocha.grep which expects a RegExp; instead escape the user-provided
buildGrep into a literal case-insensitive RegExp and pass that to
window.mocha.grep. Specifically, in the catch for window.mocha.grep(new
RegExp(buildGrep, 'i')), build a safe escaped pattern from buildGrep (e.g.,
replace special regex chars) or otherwise create a RegExp using the escaped
string and call window.mocha.grep(escapedRegex) so grep receives a RegExp rather
than a function; refer to window.mocha.grep and the buildGrep/needle handling in
the current block when making the change.
🪄 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: 9ec06355-5fb7-4399-a8f4-82c530dc0e7b
📒 Files selected for processing (4)
builds/knockout/helpers/browser-setup.jspackages/binding.foreach/spec/eachBehavior.tstko.io/scripts/bundle-tests.mjstko.io/src/pages/tests.astro
✅ Files skipped from review due to trivial changes (1)
- tko.io/scripts/bundle-tests.mjs
Comment on lines
+4
to
+12
| // Two modes: | ||
| // | ||
| // Build mode (default) | ||
| // Load a published `@tko/build.*` from jsdelivr and run | ||
| // `tests/build-bundle.js`. Single Mocha run against the | ||
| // built artifact. Version-portable. | ||
| // | ||
| // Source mode (URL opt-in: `?suite=source`) | ||
| // Spawn one iframe per spec file in `tests/source/manifest.json`. |
Contributor
There was a problem hiding this comment.
Update the mode documentation to match the actual default.
Lines 6 and 11 still describe build mode as default/source as opt-in, but Line 20 and Line 356 default to source. This can mislead anyone using the URL contract.
📝 Proposed fix
-// Build mode (default)
+// Build mode (URL opt-in: `?suite=build`)
@@
-// Source mode (URL opt-in: `?suite=source`)
+// Source mode (default)📝 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
| // Two modes: | |
| // | |
| // Build mode (default) | |
| // Load a published `@tko/build.*` from jsdelivr and run | |
| // `tests/build-bundle.js`. Single Mocha run against the | |
| // built artifact. Version-portable. | |
| // | |
| // Source mode (URL opt-in: `?suite=source`) | |
| // Spawn one iframe per spec file in `tests/source/manifest.json`. | |
| // Two modes: | |
| // | |
| // Build mode (URL opt-in: `?suite=build`) | |
| // Load a published `@tko/build.*` from jsdelivr and run | |
| // `tests/build-bundle.js`. Single Mocha run against the | |
| // built artifact. Version-portable. | |
| // | |
| // Source mode (default) | |
| // Spawn one iframe per spec file in `tests/source/manifest.json`. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tko.io/src/pages/tests.astro` around lines 4 - 12, The header comment
currently declares "Build mode (default)" and refers to "?suite=source" as
opt-in, but elsewhere the page actually defaults to source; update the
documentation so the "Two modes:" block matches the implementation by changing
the "Build mode (default)" text to indicate that Source mode is the default and
Build is opt-in (or vice‑versa to match runtime), and adjust the example URL
token reference (change "?suite=source" to the correct opt‑in token, e.g.
"?suite=build", or explicitly state which query value triggers the non‑default
mode) — search for the strings "Two modes:", "Build mode (default)",
"?suite=source", and "tests/source/manifest.json" to locate and edit the
mismatched descriptions.
Comment on lines
+456
to
+459
| // Show/hide the out-of-tree mocha host based on mode. | ||
| // (The #workarea always exists; iframes mount into it.) | ||
| document.getElementById('mocha').hidden = page.suite() !== 'build' | ||
|
|
Contributor
There was a problem hiding this comment.
Hide the focus workarea outside source mode.
In build mode, the #workarea-section is rendered but never used or removed, leaving the “Focus-requiring specs” panel stuck on screen over the Mocha reporter.
🛠️ Proposed fix
// Show/hide the out-of-tree mocha host based on mode.
// (The `#workarea` always exists; iframes mount into it.)
document.getElementById('mocha').hidden = page.suite() !== 'build'
+ document.getElementById('workarea-section').hidden = page.suite() !== 'source'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tko.io/src/pages/tests.astro` around lines 456 - 459, The workarea section
remains visible outside "source" mode; update the same mode check that hides the
`#mocha` reporter to also hide the focus workarea by setting
document.getElementById('workarea-section').hidden based on page.suite() (e.g.,
hidden = page.suite() !== 'source'), keeping the existing `#workarea` mounting
behavior intact; locate the code around the existing
document.getElementById('mocha') line and add the analogous hide logic for
'workarea-section'.
Comment on lines
+497
to
+502
| try { | ||
| window.mocha.grep(new RegExp(buildGrep, 'i')) | ||
| } catch { | ||
| const needle = buildGrep.toLowerCase() | ||
| window.mocha.grep(t => (t.fullTitle?.() || t.title || '').toLowerCase().includes(needle)) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify Mocha 10 browser grep matcher expectations.
curl -fsSL https://cdn.jsdelivr.net/npm/mocha@10/mocha.js \
| rg -n -A8 -B8 "grep\\.test|Mocha\\.prototype\\.grep|grep: function"Repository: knockout/tko
Length of output: 2368
🏁 Script executed:
cat -n tko.io/src/pages/tests.astro | sed -n '490,510p'Repository: knockout/tko
Length of output: 1101
🏁 Script executed:
# Verify the regex escape pattern is the standard approach for literal RegExp construction
web_search_result=$(curl -s "https://www.regular-expressions.info/escape.html" | grep -i "escape" | head -5)
python3 << 'EOF'
import re
# Test the escape pattern from the proposed fix
pattern = r"[.*+?^${}()|[\]\\]"
test_string = r"\$&"
buildGrep = "test.suite[invalid](pattern)"
# Apply the escape
escaped = buildGrep.replace(pattern, test_string)
print(f"Original: {buildGrep}")
print(f"Escaped: {escaped}")
# Verify it can be used in RegExp
try:
regex = re.compile(escaped, re.IGNORECASE)
print(f"RegExp creation: SUCCESS")
print(f"Test match for original string: {regex.search(buildGrep) is not None}")
except Exception as e:
print(f"RegExp creation: FAILED - {e}")
# Standard regex escape characters that should be escaped in a literal pattern
special_chars = r".*+?^${}()|[]\"
print(f"\nSpecial regex chars needing escape: {special_chars}")
EOFRepository: knockout/tko
Length of output: 204
🏁 Script executed:
python3 << 'EOF'
# Test the escape pattern from the proposed fix
buildGrep = "test.suite[invalid](pattern)"
# Apply the escape using Python equivalent of JS pattern
# JS: /[.*+?^${}()|[\]\\]/g with replacement \\$&
escaped = buildGrep.replace(r'[.*+?^${}()|[\]\\]', r'\\$&')
print(f"Original: {buildGrep}")
print(f"Escaped pattern attempt: {escaped}")
# Better: use standard regex escape approach
import re
escaped_correct = re.escape(buildGrep)
print(f"\nUsing re.escape(): {escaped_correct}")
# Verify both can create valid RegExp
try:
# Test 1: manual escape (proposed fix pattern)
pattern1 = buildGrep.replace(r'[.*+?^${}()|[\]\\]', r'\\$&')
regex1 = re.compile(pattern1, re.IGNORECASE)
print(f"Manual escape RegExp: SUCCESS")
except Exception as e:
print(f"Manual escape RegExp: FAILED - {e}")
try:
# Test 2: re.escape (standard approach)
regex2 = re.compile(re.escape(buildGrep), re.IGNORECASE)
print(f"re.escape() RegExp: SUCCESS")
except Exception as e:
print(f"re.escape() RegExp: FAILED - {e}")
# Show the special chars that need escaping in JavaScript regex
print("\nSpecial regex characters that must be escaped:")
special = ".*+?^${}()|[]\\".split()
print(f"Characters: {', '.join([c for c in '.*+?^${}()|[]\\\\'])}")
EOFRepository: knockout/tko
Length of output: 290
🏁 Script executed:
python3 << 'EOF'
import re
buildGrep = "test.suite[invalid](pattern)"
# Test the escape pattern from the proposed fix
# This is the JavaScript approach: /[.*+?^${}()|[\]\\]/g
escaped = buildGrep
for char in r'.*+?^${}()|[]\\":
escaped = escaped.replace(char, '\\' + char)
print(f"Original: {buildGrep}")
print(f"Escaped (manual): {escaped}")
# Test with Python's re.escape for comparison
escaped_py = re.escape(buildGrep)
print(f"Escaped (re.escape): {escaped_py}")
# Verify both work as literal patterns
try:
regex1 = re.compile(escaped, re.IGNORECASE)
print(f"Manual escape RegExp: SUCCESS")
print(f" Matches original string: {regex1.search(buildGrep) is not None}")
except Exception as e:
print(f"Manual escape RegExp: FAILED - {e}")
try:
regex2 = re.compile(escaped_py, re.IGNORECASE)
print(f"re.escape() RegExp: SUCCESS")
print(f" Matches original string: {regex2.search(buildGrep) is not None}")
except Exception as e:
print(f"re.escape() RegExp: FAILED - {e}")
print("\nSpecial regex characters that must be escaped:")
special_chars = r'.*+?^${}()|[]\'
print(f"Characters: {special_chars}")
EOFRepository: knockout/tko
Length of output: 196
🏁 Script executed:
python3 << 'EOF'
import re
# Test the proposed escape pattern
buildGrep = "test.suite[invalid](pattern)"
# JavaScript pattern: /[.*+?^${}()|[\]\\]/g with replacement \\$&
# This is equivalent to escaping these characters: . * + ? ^ $ { } ( ) | [ ] \
special_chars = '.^$*+?{}[]|()\\'
escaped = buildGrep
for char in special_chars:
escaped = escaped.replace(char, '\\' + char)
print("Original pattern:", buildGrep)
print("Escaped pattern:", escaped)
# Test it works as a RegExp
try:
regex = re.compile(escaped, re.IGNORECASE)
print("RegExp creation: SUCCESS")
print("Matches original string:", regex.search(buildGrep) is not None)
except Exception as e:
print("RegExp creation: FAILED -", e)
# Compare with Python's standard approach
escaped_py = re.escape(buildGrep)
print("\nPython re.escape() result:", escaped_py)
print("Are they equivalent:", escaped == escaped_py)
EOFRepository: knockout/tko
Length of output: 303
🏁 Script executed:
python3 << 'EOF'
import re
buildGrep = "test.suite[invalid](pattern)"
# The escape pattern works - it just needs to match literally
special_chars = '.^$*+?{}[]|()\\'
escaped = buildGrep
for char in special_chars:
escaped = escaped.replace(char, '\\' + char)
print("Original:", buildGrep)
print("Escaped regex pattern:", escaped)
# When we use this escaped pattern in a regex, it matches the literal string
regex = re.compile(escaped, re.IGNORECASE)
# The regex pattern itself contains backslashes, but when matched against the original string:
print("\nMatching test:")
print("regex.search(original string):", regex.search(buildGrep))
# This is the expected behavior - the backslashes in the pattern escape the special chars
# So the regex matches literal . [ ] ( ) etc in the input
# Now test with a simpler case to show it works
simple = "test.js"
escaped_simple = simple.replace('.', r'\.')
regex_simple = re.compile(escaped_simple)
print("\nSimple test - match 'test.js':", bool(regex_simple.search("test.js")))
print("Should NOT match 'testXjs':", bool(regex_simple.search("testXjs")))
EOFRepository: knockout/tko
Length of output: 279
Pass a literal RegExp to mocha.grep() instead of a function predicate.
Mocha's browser grep() method expects a RegExp with a .test() method. The current catch block passes a predicate function, which will fail at runtime when Mocha tries to call .test() on it. Escape the invalid pattern into a literal case-insensitive RegExp instead.
Proposed fix
if (buildGrep) {
try {
window.mocha.grep(new RegExp(buildGrep, 'i'))
} catch {
- const needle = buildGrep.toLowerCase()
- window.mocha.grep(t => (t.fullTitle?.() || t.title || '').toLowerCase().includes(needle))
+ const escaped = buildGrep.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
+ window.mocha.grep(new RegExp(escaped, 'i'))
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tko.io/src/pages/tests.astro` around lines 497 - 502, The catch block passes
a predicate function to window.mocha.grep which expects a RegExp; instead escape
the user-provided buildGrep into a literal case-insensitive RegExp and pass that
to window.mocha.grep. Specifically, in the catch for window.mocha.grep(new
RegExp(buildGrep, 'i')), build a safe escaped pattern from buildGrep (e.g.,
replace special regex chars) or otherwise create a RegExp using the escaped
string and call window.mocha.grep(escapedRegex) so grep receives a RegExp rather
than a function; refer to window.mocha.grep and the buildGrep/needle handling in
the current block when making the change.
Comment on lines
+595
to
+623
| iframe.addEventListener('load', () => { | ||
| if (iframe.contentWindow) liveFrames.add(iframe.contentWindow) | ||
| if (grantFocus) { | ||
| try { | ||
| iframe.focus() | ||
| iframe.contentWindow?.focus() | ||
| } catch {} | ||
| } | ||
| }) | ||
| let done = false | ||
| let safetyTimer | ||
| const finish = () => { | ||
| if (done) return | ||
| done = true | ||
| clearTimeout(safetyTimer) | ||
| window.removeEventListener('message', onMsg) | ||
| if (iframe.contentWindow) liveFrames.delete(iframe.contentWindow) | ||
| iframe.remove() | ||
| resolve() | ||
| } | ||
| const onMsg = e => { | ||
| if (e.origin !== location.origin) return | ||
| if (e.source !== iframe.contentWindow) return | ||
| const m = e.data | ||
| if (m?.slug !== spec.slug) return | ||
| if (m.type === 'end' || m.type === 'import-error') finish() | ||
| } | ||
| window.addEventListener('message', onMsg) | ||
| host.appendChild(iframe) |
Contributor
There was a problem hiding this comment.
Register live iframes before they can post results.
liveFrames.add(...) currently runs from the iframe load event, but the child can post start/test/end messages while its module script is running before the parent load handler fires. The global aggregator then drops those messages at Line 547, while the per-frame listener can still finish the spec, producing silently missing results.
🐛 Proposed fix
iframe.addEventListener('load', () => {
- if (iframe.contentWindow) liveFrames.add(iframe.contentWindow)
if (grantFocus) {
try {
iframe.focus()
iframe.contentWindow?.focus()
} catch {}
@@
}
window.addEventListener('message', onMsg)
host.appendChild(iframe)
+ if (iframe.contentWindow) liveFrames.add(iframe.contentWindow)
safetyTimer = setTimeout(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tko.io/src/pages/tests.astro` around lines 595 - 623, The per-frame message
handler can miss messages because liveFrames.add(...) is only called in the
iframe 'load' handler; move the registration earlier by adding the iframe's
contentWindow to liveFrames as soon as the iframe is created (or register a
placeholder/temporary id) and register the window.message listener (onMsg)
before appending the iframe, then keep the existing cleanup in finish (which
deletes from liveFrames and removes the onMsg listener) so frames are tracked
immediately and still cleaned up when finish() runs; update the code that
currently calls liveFrames.add(...) inside the iframe.addEventListener('load',
...) block accordingly.
added 2 commits
April 22, 2026 08:54
- Add plans/browser-test-runner.md documenting the /tests page: context, two-phase iframe scheduling, bundling flow, files, verification, and known gaps. - Promote the plans/ directory in AGENTS.md: add a "Before you start" checklist at the top of the agent-context section so the plans/ convention is not buried after Release Process. - Enrich the Plans section with explicit triggers (when to write one, when to skip) so future agents don't have to infer scope.
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
New `/tests` page ships a live, in-browser Mocha runner for the TKO spec suite. Visit the URL in any real browser and watch every spec execute against real TKO + real DOM. 2708 pass / 0 fail / 42 pending (pre-existing `it.skip`s) across 145 spec files in ~10s.
Two modes:
"Tests" pill added to the top nav next to "Playground".
Architecture
Bundler (`tko.io/scripts/bundle-tests.mjs`) — emits three outputs:
Iframe harness (`public/tests-frame.html`) — classic script loads mocha, `mocha.setup('bdd')`, `setup.js` (globals + ko), then dynamic-imports the per-spec ESM chunk. Mocha runner events post back to parent via `postMessage`.
Parent page (`src/pages/tests.astro`) — TKO-written view model (`ko.observable`, `pureComputed`, `data-bind`; dogfood), receives `postMessage` events, renders reactive suite tree + chips + progress bar.
Split queue for concurrency:
Notable gotchas shipped with fixes
esbuild chunk ordering reorders ESM imports. Earlier attempts put ko + browser-setup as imports in each per-spec wrapper; the code-splitter hoisted the spec's chunk above the ko + setup chunks. Fix: move ko + setup into a dedicated IIFE loaded by `<script>` tag, outside the module graph entirely.
Chromium gates `contentWindow.focus()` system focus on iframe layout. Off-screen / 1×1 / opacity:0 iframes never pass `document.hasFocus() === true` inside; `focusin` / `focusout` events never fire. Fix: visible `#workarea` region at the bottom of the page renders the focus-requiring iframe full-size with real layout.
Scoped Astro CSS doesn't apply to dynamically-created iframes. Switched iframe styles to inline `style.cssText` rather than `className`.
Specs referenced `ko.*` at module top. ESM imports hoist; wrapper's `globalThis.ko = ko` top-level statement ran after the spec. Fix: classic `<script src="/lib/ko.js">` in the frame loads a knockout IIFE synchronously before any module.
Dev-mode class-identity collisions. Wrapper's imports hit `packages//src/index.ts` via explicit esbuild `alias`; a `distToSrcPlugin` rewrites `../dist` imports (used pervasively in source specs) to `../src` so one module graph wins.
Spec hygiene fixes that surfaced as the isolation improved: `restoreAfter(options, 'bindingGlobals')` in `packages/utils.parser/spec/identifierBehaviors.ts`, `restoreAfter(options, 'filters'/'bindingProviderInstance')` in `packages/builder/spec/builderBehaviors.ts`, whitespace-tolerant assertions in `packages/binding.if/spec/elseBehaviors.ts` and `packages/binding.component/spec/componentBindingBehaviors.ts`.
UI
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores