Replace Karma with Vitest, add Bun support#303
Merged
brianmhunt merged 35 commits intomainfrom Apr 14, 2026
Merged
Conversation
Add mise/asdf .tool-versions pinning bun@latest and node@24. Add bun.lock generated from existing package-lock.json. Add modern tooling migration plan. bun install: 225 packages in 819ms. All tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSwitches repo tooling and CI from Node/Karma/Mocha to Bun/Vitest/Playwright, adds Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CI as CI runner
participant Bun as Bun (oven-sh)
participant Vitest as Vitest (bunx)
participant Playwright as Playwright
participant Browser as Headless Browser
Dev->>CI: push / PR triggers workflow
CI->>Bun: setup Bun (oven-sh/setup-bun)
CI->>Bun: run `bun install`
CI->>Bun: run `bunx playwright install --with-deps`
CI->>Vitest: run `bunx vitest run` (VITEST_BROWSERS)
Vitest->>Playwright: request browser instances
Playwright->>Browser: spawn headless browsers
Browser-->>Vitest: execute tests, return results
Vitest-->>CI: emit test results/status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 1
🧹 Nitpick comments (1)
plans/modern-tooling.md (1)
35-35: Keep an npm CI lane during Phase 1 to preserve the “additive” contract.Switching CI install fully to Bun this early can stop detecting npm path regressions while npm is still declared supported. Consider running both install paths until Phase 3 removes Make/npm dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plans/modern-tooling.md` at line 35, The proposal to "Update CI to use `bun install` instead of `npm install`" should be changed to keep an npm CI lane during Phase 1: update CI workflows to run both install paths (one job using `bun install`, another using `npm install`) so regressions against the npm path are detected until Phase 3; ensure job names or matrix entries clearly reflect the installer (e.g., "install:bun" and "install:npm") and gate the single-installer switch behind the Phase 3 removal plan.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.tool-versions:
- Around line 1-2: Replace non-deterministic runtime entries in .tool-versions:
change the "bun latest" and "node 24" lines to pinned patch-level versions
(e.g., "bun 1.2.9" and "node 24.11.1" or your team's chosen exact versions) so
CI and local environments are reproducible; update any related documentation or
CI config that assumes the previous loose pins.
---
Nitpick comments:
In `@plans/modern-tooling.md`:
- Line 35: The proposal to "Update CI to use `bun install` instead of `npm
install`" should be changed to keep an npm CI lane during Phase 1: update CI
workflows to run both install paths (one job using `bun install`, another using
`npm install`) so regressions against the npm path are detected until Phase 3;
ensure job names or matrix entries clearly reflect the installer (e.g.,
"install:bun" and "install:npm") and gate the single-installer switch behind the
Phase 3 removal plan.
🪄 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: 25451265-6e19-4320-898b-58ca389972ac
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
.tool-versionsplans/modern-tooling.md
Comment on lines
+1
to
+2
| bun latest | ||
| node 24 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For asdf/mise .tool-versions, what are the recommended practices for reproducible builds when specifying Bun and Node versions? Is using "latest" considered non-deterministic?
💡 Result:
For reproducible builds with asdf/mise .tool-versions specifying Bun and Node versions, use exact version numbers (e.g., "bun 1.1.3" or "node 20.11.0") rather than fuzzy prefixes like "bun 1" or "latest". Exact versions ensure every team member and CI run installs the identical tool version, eliminating variability. Fuzzy matching (e.g., "node 20") resolves to the latest patch in the major/minor at install time, which can lead to inconsistencies if new patches release between installs. "latest" is explicitly non-deterministic: it fetches whatever is current at runtime, varying across machines, dates, and network conditions. asdf rejects "latest" in .tool-versions for determinism; mise supports it but warns against it for reproducibility. Best practices: - Pin exact versions in .tool-versions (asdf/mise compatible) or mise.toml: e.g., node 20.11.0, bun 1.1.3. - Commit .tool-versions to git for team consistency. - Use mise install (or asdf install) to install from the file. - For ultimate reproducibility, enable mise.lock (mise-only): mise settings lockfile=true; mise lock. This pins exact download URLs and checksums, like package-lock.json. Examples: .tool-versions: node 20.11.0 bun 1.1.3 mise.toml: [tools] node = "20.11.0" bun = "1.1.3" Using "latest" is considered non-deterministic and not recommended for reproducible builds.
Citations:
- 1: https://mise.jdx.dev/faq.html
- 2: https://www.devtoolsguide.com/version-managers-guide
- 3: https://asdf-vm.com/manage/configuration.html
- 4: https://oneuptime.com/blog/post/2026-01-25-mise-tool-version-management/view
- 5: https://mise.jdx.dev/dev-tools/mise-lock.html
- 6: https://mise.jdx.dev/cli/use.html
- 7: https://mise.jdx.dev/walkthrough.html
- 8: https://mise.jdx.dev/dev-tools/
- 9: Support "latest" in .tool-versions asdf-vm/asdf#1012
Pin exact Bun and Node versions (including patch level) for reproducible builds.
Using bun latest is non-deterministic and breaks reproducibility across CI and local setups. Using node 24 (without patch version) will drift with each new Node 24.x release, causing silent inconsistencies between installations. Pin exact versions: bun 1.2.9 and node 24.11.1 (or current equivalents).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.tool-versions around lines 1 - 2, Replace non-deterministic runtime entries
in .tool-versions: change the "bun latest" and "node 24" lines to pinned
patch-level versions (e.g., "bun 1.2.9" and "node 24.11.1" or your team's chosen
exact versions) so CI and local environments are reproducible; update any
related documentation or CI config that assumes the previous loose pins.
![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: d500b47e7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1
to
+2
| bun latest | ||
| node 24 |
There was a problem hiding this comment.
Using bun latest and node 24 leaves runtime selection floating, so contributors can get different Bun/Node versions over time even with the same repository checkout. That makes installs and test behavior non-reproducible and can introduce hard-to-diagnose regressions when upstream runtime releases change; this file should pin concrete versions instead of moving targets.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,140 @@ | |||
| # Plan: Modern Tooling Migration | |||
There was a problem hiding this comment.
This plan omits required audit fields from the repository governance rules: AGENTS.md (line 25) and AI_COMPLIANCE.md §9 require explicit risk class, tools/commands used, validation outcomes, and follow-up owner for substantial AI-assisted work. Without those fields in plans/modern-tooling.md, reviewers lack the mandatory traceability metadata needed for compliant review/approval.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds initial Bun support and documents a phased plan to modernize the repo’s tooling, aiming to keep current npm/Makefile workflows working while enabling faster installs and future migrations.
Changes:
- Add
.tool-versionsto enable mise/asdf-based tool selection for Bun and Node. - Add
bun.lockto supportbun installalongside existing npm lockfile. - Add
plans/modern-tooling.mddescribing a phased migration plan (Bun, tsgo, Biome, knip).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.tool-versions |
Introduces tool version selection for Bun and Node. |
plans/modern-tooling.md |
Documents a phased “modern tooling” migration plan and verification expectations. |
|
|
||
| **One PR. Low risk.** | ||
|
|
||
| - Add `.tool-versions` with `bun latest` (and `node 24` for compatibility) |
| @@ -0,0 +1,2 @@ | |||
| bun latest | |||
| @@ -0,0 +1,2 @@ | |||
| bun latest | |||
| node 24 | |||
Add Phase 5 (@web/test-runner replaces Karma) to the modern tooling plan. Remove node from .tool-versions since Bun provides Node compatibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Vitest with Playwright runs 1642 tests across 83 files in 7.2s (vs Karma's 28s for the same suite). Zero test rewrites — existing Mocha/Chai/Sinon code works as-is. Fixes: - this.timeout() calls incompatible with Vitest (removed, use config) - Function.bind this-context tests (accept null/undefined/globalThis) - dummyTemplateEngine: import unwrap for eval visibility in ESM - Add Bun prerequisite to AGENTS.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1645 tests across 86 files in 3s (vs Karma's 28s). Headless Chromium via Playwright. Existing Mocha/Chai/Sinon code unchanged. builds/knockout (955 JS tests) still uses Karma — its helper registers beforeEach/afterEach at module scope which requires Karma's global loading model. TODO: refactor helper for Vitest. Fixes for Vitest compatibility: - Remove this.timeout() calls (use vitest config instead) - Accept null/undefined/globalThis in Function.bind this-context tests - Import unwrap in dummyTemplateEngine for eval visibility in ESM - Computed write callback this-context: accept null/undefined/globalThis Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Convert mocha-test-helpers.js from IIFE to ESM. Replace this.restoreAfter/this.after with global functions backed by a per-test cleanup array (Vitest doesn't support Mocha this context). 2505 of 2558 tests pass (15 remaining failures in AMD loader mocks, with-statement eval, and cross-window tests — edge cases). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
vitest.config.ts (1)
17-19: Consider multi-browser instances for parity with legacy coverage.Configuring only Chromium narrows browser-surface validation. Consider adding Firefox/WebKit here (or document that those remain covered in a separate Karma lane during migration).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.config.ts` around lines 17 - 19, The test config currently specifies a single browser instance in the instances array ({ browser: 'chromium' }), which limits cross-browser coverage; update the instances configuration in vitest.config.ts to include additional browser entries (e.g., { browser: 'firefox' } and { browser: 'webkit' }) so the instances array contains chromium, firefox, and webkit, or alternatively add a comment/docstring near the instances declaration indicating that Firefox/WebKit coverage is intentionally handled by the separate Karma lane if you prefer not to run them here.plans/modern-tooling.md (1)
105-139: Clarify the primary browser-test runner in the plan.Phase 5 currently documents
@web/test-runner, while this PR introduces a Vitest browser runner config. Please explicitly document whether Vitest is transitional or the long-term target to avoid split migration guidance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plans/modern-tooling.md` around lines 105 - 139, The plan currently centers `@web/test-runner` (Phase 5, web-test-runner.config.mjs, web-test-runner --browsers chromium firefox webkit) but your PR introduces a Vitest browser runner config; update the document to explicitly state which runner is the primary long-term target versus which is transitional: either replace references to `@web/test-runner` with Vitest everywhere if Vitest is the chosen long-term runner, or add a short note in Phase 5 and the Sub-PRs section clarifying that Vitest is a temporary/experimental bridge (and keep `@web/test-runner` as the final target), and adjust Sub-PR 1–4 and CI/Makefile/AGENTS.md steps to reflect the chosen approach so guidance is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/binding.foreach/spec/eachBehavior.ts`:
- Around line 447-449: The file fails Prettier formatting; run Prettier with the
project's configuration (no semicolons, single quotes, no trailing commas, 120
char width) and reformat this spec around the describe('DOM move capabilities')
/ it('sorting complex data moves 1 DOM node') block so CI style checks pass;
ensure you save the formatted file and include only stylistic changes (no logic
edits).
In `@vitest.config.ts`:
- Around line 1-24: The vitest.config.ts file needs to be reformatted with the
project's Prettier rules (no semicolons, single quotes, no trailing commas,
print width 120). Run Prettier (or your editor format) on the file and ensure
imports/exports and the defineConfig block (symbols: defineConfig, playwright,
export default) follow those rules so CI Prettier check passes.
---
Nitpick comments:
In `@plans/modern-tooling.md`:
- Around line 105-139: The plan currently centers `@web/test-runner` (Phase 5,
web-test-runner.config.mjs, web-test-runner --browsers chromium firefox webkit)
but your PR introduces a Vitest browser runner config; update the document to
explicitly state which runner is the primary long-term target versus which is
transitional: either replace references to `@web/test-runner` with Vitest
everywhere if Vitest is the chosen long-term runner, or add a short note in
Phase 5 and the Sub-PRs section clarifying that Vitest is a
temporary/experimental bridge (and keep `@web/test-runner` as the final target),
and adjust Sub-PR 1–4 and CI/Makefile/AGENTS.md steps to reflect the chosen
approach so guidance is consistent.
In `@vitest.config.ts`:
- Around line 17-19: The test config currently specifies a single browser
instance in the instances array ({ browser: 'chromium' }), which limits
cross-browser coverage; update the instances configuration in vitest.config.ts
to include additional browser entries (e.g., { browser: 'firefox' } and {
browser: 'webkit' }) so the instances array contains chromium, firefox, and
webkit, or alternatively add a comment/docstring near the instances declaration
indicating that Firefox/WebKit coverage is intentionally handled by the separate
Karma lane if you prefer not to run them here.
🪄 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: 32b0b552-3c56-4f11-a07d-6043731813fe
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
AGENTS.mdpackage.jsonpackages/binding.foreach/spec/eachBehavior.tspackages/binding.template/helpers/dummyTemplateEngine.tspackages/computed/spec/computedObservableBehaviors.tspackages/utils/spec/utilsBehaviors.tsplans/modern-tooling.mdvitest.config.ts
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
- Replace spec.restoreAfter with global restoreAfter (2 files) - Replace with-statement eval with new Function in templatingBehaviors - Convert done() callbacks to async/await (textInputBehaviors, valueBehaviors) All 143 files pass: 2679 tests, 42 skipped, 0 failures in ~4s. 2 expected unhandled errors from onErrorBehaviors (deliberate throws). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Delete tools/karma.conf.js, remove 7 karma devDependencies and 3 @web/test-runner deps (unused), strip karma config blocks from 27 package.json files, update Makefile and build.mk test targets to use bunx vitest, update AGENTS.md to document Vitest. Remove __karma__ runtime check from mocha-test-helpers.js. 143 files, 2679 tests, 4 seconds. -1790 lines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix switchJQueryState to default disableJQueryUsage=true (was inverted) - Remove mocha, @types/mocha, nyc devDependencies (replaced by Vitest) - Collapse 4 identical test targets into one Make rule - Remove per-package test targets from build.mk (Vitest runs from root) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update AGENTS.md (project structure, build commands), README.md (test runner, test environment), AI_COMPLIANCE.md (high-risk paths), and skills/plan-creation/SKILL.md to reference Vitest instead of Karma. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
brianmhunt
changed the title
- Add oven-sh/setup-bun to all test/lint workflows - Replace npm install with bun install - Install Playwright chromium in test workflows - Collapse 3-variant test matrix to single vitest run - Fix prettier on eachBehavior.ts and vitest.config.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove 'mocha' from tsconfig types (dep was removed, vitest provides globals) - Add dangerouslyIgnoreUnhandledErrors for onErrorBehaviors deliberate throws Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The onErrorBehaviors tests deliberately throw to test TKO's error
handling. Use addEventListener('error') with preventDefault() to
stop Vitest's error catcher from seeing them, instead of the
blanket dangerouslyIgnoreUnhandledErrors flag.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
vitest/globals conflicts with Chai's expect type. Use @types/mocha instead (types only — mocha runtime not installed) which provides describe/it/beforeEach without conflicting with Chai. Also fix: - Convert 5 done() callbacks to async/await in eachBehavior.ts - Remove incorrect boolean args from assert.ok/notOk in NativeProviderBehaviors - Remove dangerouslyIgnoreUnhandledErrors (replaced by proper error capture) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add VITEST_BROWSERS env var (defaults to chromium for local dev). CI sets chromium,firefox to test both browsers via Playwright. Install both browsers in CI workflows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix npx→bunx in lint-and-typecheck.yml - Add setup-bun to publish-check.yml - Restore setup-node in test-headless.yml (needed for make/esbuild) - Add WebKit (Safari) to browser test matrix: chromium,firefox,webkit - Delete stale package-lock.json (Bun uses bun.lock) - Update Makefile deps from package-lock.json to bun.lock - Fix process.env type error in vitest.config.ts - Fix stale IIFE comment in vitest-setup.js Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Switch build.mk from npx esbuild to bunx esbuild. Remove setup-node from test and lint workflows — Bun provides everything needed. Keep Node only in publish-check (npm pack) and release (npm trusted publishing). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 @.github/workflows/test-headless.yml:
- Around line 16-26: The workflow lacks an explicit Node provisioning step
before the Run Build step (which calls make and therefore node/npx/npm); add a
new step named "Setup Node" using actions/setup-node@v4 with the repository's
required node version (e.g., from package.json engines or .nvmrc) and place it
before the "Run Build" step so make runs against a pinned Node runtime rather
than the runner's default.
In `@builds/knockout/helpers/mocha-test-helpers.js`:
- Around line 154-156: The helper switchJQueryState() currently hard-sets
ko.options.disableJQueryUsage = disableJQueryUsage = true which unconditionally
disables the jQuery path; change it to restore the previous toggle behavior by
accepting or reading a boolean argument or existing test state and assign that
to ko.options.disableJQueryUsage and the local disableJQueryUsage variable
instead of always setting true so tests can enable or disable the jQuery-backed
branch; update the switchJQueryState reference and any callers to pass the
desired boolean (or preserve the original toggle semantics) so
`@tko/build.knockout` compatibility is retained.
- Around line 180-185: The afterEach cleanup loop currently stops on the first
thrown error and leaks remaining restorations; modify the afterEach handler (the
function that iterates over _cleanups) to always run every cleanup callback in
reverse order, capturing the first thrown exception (from any _cleanups[index]()
call), continue invoking the rest, then after the loop reset _cleanups = [] and
rethrow the first captured error if one occurred; ensure you catch errors from
each individual cleanup invocation and only preserve the first one for
rethrowing after all cleanups run.
In `@builds/knockout/spec/components/componentBindingBehaviors.js`:
- Around line 793-796: The teardown loop inside the after(function() { ... })
currently iterates i from 0 to 3 and unregisters 'component-0'..'component-3',
leaving 'component-4' registered; update the loop bounds so it iterates i from 1
through 4 inclusive and calls ko.components.unregister('component-' + i) for
each, ensuring 'component-1'..'component-4' are all unregistered.
In `@builds/knockout/spec/defaultBindings/textInputBehaviors.js`:
- Around line 190-198: The test currently only exercises the modern "input"
path; preserve coverage for the retained legacy IE path by adding an assertion
that simulates the old propertychange behavior: after setting
testNode.childNodes[0].value = "some user-entered value", call
ko.utils.triggerEvent(testNode.childNodes[0], "propertychange") (or add a
separate spec that triggers "propertychange") and assert myobservable() equals
"some user-entered value"; update or duplicate the spec named "Should update
observable on input event (on supported browsers) or propertychange event (on
old IE)" to include this legacy trigger so the propertyChange handler in
value.ts remains tested.
In `@builds/knockout/spec/onErrorBehaviors.js`:
- Around line 64-79: The current beforeEach blindly suppresses all window
'error' and 'unhandledrejection' events via the errorCapture handler and
window.onerror, which can mask unrelated test failures; change errorCapture (and
the window.onerror handler usage) to only preventDefault for known/intentional
errors (e.g., match event.message/event.error/event.reason or check an explicit
test-scoped flag like expectedErrorActive) and record all captured events into
an array rather than discarding them; in afterEach, remove the listeners as
before, inspect the captured events array and assert there are no unexpected
errors (or fail the test if any captured event does not match the expected
pattern), and still clear errorCapture and reset windowOnErrorCount.
In `@Makefile`:
- Around line 23-24: The Makefile collapsed test targets so running "make
test-headless-ff" or "make test-headless-jquery" falls back to chromium due to
vitest.config.ts; restore per-target behavior by either (A) wiring explicit
environment vars/mode per target (e.g., set VITEST_BROWSERS or the mode in the
test-headless-ff and test-headless-jquery targets) so each target invokes bunx
vitest run with the correct browser, or (B) remove the stale targets
(test-headless-ff, test-headless-jquery) and update any callers/docs to use the
canonical test targets so failures are visible; update the Makefile targets
named test-headless, test-headless-ff, and test-headless-jquery and ensure
vitest.config.ts behavior is respected by exporting/setting VITEST_BROWSERS in
the target recipes.
🪄 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: f71251db-85a6-4e82-84a1-3e9caa3d1c0b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (71)
.github/workflows/lint-and-typecheck.yml.github/workflows/main-build.yml.github/workflows/test-headless.yml.gitignore.vscode/launch.jsonAGENTS.mdAI_COMPLIANCE.mdMakefileREADME.mdbuilds/knockout/helpers/mocha-test-helpers.jsbuilds/knockout/helpers/vitest-setup.jsbuilds/knockout/package.jsonbuilds/knockout/spec/asyncBehaviors.jsbuilds/knockout/spec/bindingAttributeBehaviors.jsbuilds/knockout/spec/bindingDependencyBehaviors.jsbuilds/knockout/spec/bindingGlobalsBehaviors.jsbuilds/knockout/spec/bindingPreprocessingBehaviors.jsbuilds/knockout/spec/components/componentBindingBehaviors.jsbuilds/knockout/spec/components/customElementBehaviors.jsbuilds/knockout/spec/components/defaultLoaderBehaviors.jsbuilds/knockout/spec/components/loaderRegistryBehaviors.jsbuilds/knockout/spec/defaultBindings/foreachBehaviors.jsbuilds/knockout/spec/defaultBindings/ifBehaviors.jsbuilds/knockout/spec/defaultBindings/ifnotBehaviors.jsbuilds/knockout/spec/defaultBindings/textBehaviors.jsbuilds/knockout/spec/defaultBindings/textInputBehaviors.jsbuilds/knockout/spec/defaultBindings/withBehaviors.jsbuilds/knockout/spec/dependentObservableBehaviors.jsbuilds/knockout/spec/domNodeDisposalBehaviors.jsbuilds/knockout/spec/nodePreprocessingBehaviors.jsbuilds/knockout/spec/observableArrayBehaviors.jsbuilds/knockout/spec/observableBehaviors.jsbuilds/knockout/spec/onErrorBehaviors.jsbuilds/knockout/spec/subscribableBehaviors.jsbuilds/knockout/spec/taskBehaviors.jsbuilds/knockout/spec/templatingBehaviors.jsbuilds/knockout/spec/utilsDomBehaviors.jsbuilds/reference/package.jsonpackage.jsonpackages/bind/package.jsonpackages/binding.component/package.jsonpackages/binding.core/package.jsonpackages/binding.core/spec/valueBehaviors.tspackages/binding.foreach/package.jsonpackages/binding.foreach/spec/eachBehavior.tspackages/binding.if/package.jsonpackages/binding.template/package.jsonpackages/builder/package.jsonpackages/computed/package.jsonpackages/filter.punches/package.jsonpackages/lifecycle/package.jsonpackages/observable/package.jsonpackages/provider.attr/package.jsonpackages/provider.bindingstring/package.jsonpackages/provider.component/package.jsonpackages/provider.databind/package.jsonpackages/provider.multi/package.jsonpackages/provider.mustache/package.jsonpackages/provider.native/package.jsonpackages/provider.native/spec/NativeProviderBehaviors.tspackages/provider.virtual/package.jsonpackages/provider/package.jsonpackages/utils.component/package.jsonpackages/utils.functionrewrite/package.jsonpackages/utils.jsx/package.jsonpackages/utils.parser/package.jsonpackages/utils/package.jsonskills/plan-creation/SKILL.mdtools/build.mktools/karma.conf.jsvitest.config.ts
💤 Files with no reviewable changes (28)
- packages/binding.component/package.json
- builds/reference/package.json
- packages/binding.core/package.json
- packages/builder/package.json
- packages/lifecycle/package.json
- packages/provider.attr/package.json
- packages/bind/package.json
- packages/provider.virtual/package.json
- packages/provider.bindingstring/package.json
- builds/knockout/package.json
- packages/binding.template/package.json
- packages/binding.if/package.json
- packages/filter.punches/package.json
- packages/provider.multi/package.json
- packages/provider.mustache/package.json
- packages/utils.component/package.json
- packages/provider/package.json
- packages/binding.foreach/package.json
- packages/provider.native/package.json
- packages/utils/package.json
- packages/utils.jsx/package.json
- packages/provider.databind/package.json
- packages/observable/package.json
- packages/provider.component/package.json
- packages/utils.functionrewrite/package.json
- packages/computed/package.json
- packages/utils.parser/package.json
- tools/karma.conf.js
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- AI_COMPLIANCE.md
- .vscode/launch.json
- README.md
- packages/provider.native/spec/NativeProviderBehaviors.ts
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- packages/binding.foreach/spec/eachBehavior.ts
- vitest.config.ts
Comment on lines
+16
to
26
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v2 | ||
|
|
||
| - name: Install dependencies | ||
| run: npm install | ||
| run: bun install | ||
|
|
||
| - name: Install Playwright browsers | ||
| run: bunx playwright install chromium firefox | ||
|
|
||
| - name: Run Build | ||
| run: make |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that this workflow reaches commands that require Node tooling.
set -euo pipefail
echo "Workflow step invoking make:"
rg -n -C2 'run:\s*make\b' .github/workflows/test-headless.yml
echo
echo "Root Makefile Node/npm/npx definitions and usages:"
rg -n -C2 '^(NPX|NODE|NPM|LERNA)\s*:=' Makefile
rg -n -C2 '\b(node|npx|npm)\b' Makefile
echo
echo "Package build makefile Node/npx usages:"
rg -n -C2 '\b(node|npx)\b' tools/build.mkRepository: knockout/tko
Length of output: 1130
🏁 Script executed:
# Check other workflow files for Node setup patterns
echo "=== main-build.yml setup steps ==="
head -50 .github/workflows/main-build.yml | grep -A5 -B5 "setup-node\|Setup Node"
echo
echo "=== lint-and-typecheck.yml setup steps ==="
head -50 .github/workflows/lint-and-typecheck.yml | grep -A5 -B5 "setup-node\|Setup Node"
echo
echo "=== Check for Node version config in package.json ==="
jq '.engines' package.json 2>/dev/null || echo "No engines field"
echo
echo "=== Check for Node version config in .nvmrc ==="
cat .nvmrc 2>/dev/null || echo "No .nvmrc file"
echo
echo "=== Check test-headless.yml full workflow ==="
cat .github/workflows/test-headless.ymlRepository: knockout/tko
Length of output: 897
🏁 Script executed:
# List all workflow files
echo "=== All workflow files ==="
ls -la .github/workflows/
echo
echo "=== main-build.yml content ==="
cat .github/workflows/main-build.yml 2>/dev/null || echo "File not found"
echo
echo "=== lint-and-typecheck.yml content ==="
cat .github/workflows/lint-and-typecheck.yml 2>/dev/null || echo "File not found"
echo
echo "=== Search for setup-node in all workflows ==="
grep -r "setup-node" .github/workflows/ 2>/dev/null || echo "No setup-node found in any workflow"Repository: knockout/tko
Length of output: 3028
Re-add explicit Node setup before make.
This workflow runs make, which invokes node, npx, and npm through the Makefiles, but does not explicitly provision Node. This makes it depend on whatever Node version is preinstalled on ubuntu-latest, creating a drift risk from the repository's toolchain.
Minimal fix
- name: Setup Bun
uses: oven-sh/setup-bun@v2
+
+ - name: Setup Node.js
+ uses: actions/setup-node@v6
+ with:
+ node-version: 24
- name: Install dependencies
run: bun install🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test-headless.yml around lines 16 - 26, The workflow lacks
an explicit Node provisioning step before the Run Build step (which calls make
and therefore node/npx/npm); add a new step named "Setup Node" using
actions/setup-node@v4 with the repository's required node version (e.g., from
package.json engines or .nvmrc) and place it before the "Run Build" step so make
runs against a pinned Node runtime rather than the runner's default.
Comment on lines
+154
to
+156
| function switchJQueryState() { | ||
| ko.options.disableJQueryUsage = disableJQueryUsage = true | ||
| } |
Contributor
There was a problem hiding this comment.
Don't hard-disable the jQuery path in this harness.
switchJQueryState() now always forces ko.options.disableJQueryUsage = true, so the Vitest browser run can no longer exercise the jQuery-backed branch that @tko/build.knockout still supports. Please keep an equivalent toggle or secondary project for the jQuery-enabled path, otherwise this migration drops compatibility coverage.
As per coding guidelines: Preserve backwards compatibility in @tko/build.knockout while allowing modern patterns in @tko/build.reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builds/knockout/helpers/mocha-test-helpers.js` around lines 154 - 156, The
helper switchJQueryState() currently hard-sets ko.options.disableJQueryUsage =
disableJQueryUsage = true which unconditionally disables the jQuery path; change
it to restore the previous toggle behavior by accepting or reading a boolean
argument or existing test state and assign that to ko.options.disableJQueryUsage
and the local disableJQueryUsage variable instead of always setting true so
tests can enable or disable the jQuery-backed branch; update the
switchJQueryState reference and any callers to pass the desired boolean (or
preserve the original toggle semantics) so `@tko/build.knockout` compatibility is
retained.
Comment on lines
+190
to
+198
| it('Should update observable on input event (on supported browsers) or propertychange event (on old IE)', function () { | ||
| var myobservable = new ko.observable(123); | ||
| testNode.innerHTML = "<input data-bind='textInput: someProp' />"; | ||
| ko.applyBindings({ someProp: myobservable }, testNode); | ||
| expect(testNode.childNodes[0].value).to.deep.equal("123"); | ||
|
|
||
| testNode.childNodes[0].value = "some user-entered value"; // setting the value triggers the propertychange event on IE | ||
| if (!ieVersion || ieVersion >= 9) { | ||
| ko.utils.triggerEvent(testNode.childNodes[0], "input"); | ||
| } | ||
| if (ieVersion === 9) { | ||
| // IE 9 responds to the event asynchronously (see #1788) | ||
| setTimeout(function () { | ||
| try { | ||
| expect(myobservable()).to.deep.equal("some user-entered value"); | ||
| done(); | ||
| } catch (error) { | ||
| done(error); | ||
| } | ||
| }, 50); | ||
| } else { | ||
| expect(myobservable()).to.deep.equal("some user-entered value"); | ||
| done(); | ||
| } | ||
| testNode.childNodes[0].value = "some user-entered value"; | ||
| ko.utils.triggerEvent(testNode.childNodes[0], "input"); | ||
| expect(myobservable()).to.deep.equal("some user-entered value"); |
Contributor
There was a problem hiding this comment.
Keep coverage for the retained legacy propertychange path.
This spec now only proves the input branch, even though the title still claims old-IE propertychange coverage. That makes regressions in the retained compatibility path invisible here. Please either add a separate forced-legacy-event assertion or narrow the test name to the new contract.
Based on learnings: in packages/binding.core/src/value.ts, the IE10/11 autocomplete hack and related propertyChange handlers are intentionally retained, and builds/knockout changes should preserve backwards compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builds/knockout/spec/defaultBindings/textInputBehaviors.js` around lines 190
- 198, The test currently only exercises the modern "input" path; preserve
coverage for the retained legacy IE path by adding an assertion that simulates
the old propertychange behavior: after setting testNode.childNodes[0].value =
"some user-entered value", call ko.utils.triggerEvent(testNode.childNodes[0],
"propertychange") (or add a separate spec that triggers "propertychange") and
assert myobservable() equals "some user-entered value"; update or duplicate the
spec named "Should update observable on input event (on supported browsers) or
propertychange event (on old IE)" to include this legacy trigger so the
propertyChange handler in value.ts remains tested.
- Replace all npx/npm with bunx/bun in Makefile - Fix lerna invocation to use bunx - Format vitest.config.ts - Fix stale package-lock.json reference in install target Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Pin bun 1.3.12 in .tool-versions (was 'latest'), remove node - Wire VITEST_BROWSERS per Makefile target (ff→firefox, default→chromium) - Fix cleanup loop: catch errors, run all cleanups, rethrow first - Fix component teardown: unregister 1-4 not 0-3 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
phillipc
reviewed
Apr 14, 2026
phillipc
reviewed
Apr 14, 2026
| import sinon from 'sinon' | ||
|
|
||
| // Set globals that builds/knockout specs and mocha-test-helpers.js expect | ||
| globalThis.chai = chai |
Member
There was a problem hiding this comment.
Chai and Sinon are ES-Modules, we can import it foreach file if is needed; instead make it global
Member
Author
There was a problem hiding this comment.
The builds/knockout specs are plain JS files that reference chai, expect, and sinon as globals — they don't have import statements. Converting 57 spec files to ESM imports would be a much larger change. The setup file sets globals so the existing specs work without modification. Agree it's not ideal long-term — noted as a follow-up in the plan.
phillipc
reviewed
Apr 14, 2026
| return; // Nothing to test. Run the specs with jQuery referenced for this to do anything. | ||
| } | ||
|
|
||
| this.restoreAfter(ko.options, 'useOnlyNativeEvents'); |
Member
Author
There was a problem hiding this comment.
Yes — this saves and restores ko.options.useOnlyNativeEvents so the test's modification doesn't leak into subsequent tests. It was this.restoreAfter(...) before the Vitest migration, converted to the global restoreAfter() since Vitest doesn't support Mocha's this context.
phillipc
reviewed
Apr 14, 2026
| | Phase | Status | PR | Notes | | ||
| |-------|--------|-----|-------| | ||
| | 1. Bun + Vitest | CI green | #303 | Bun replaces npm. Vitest replaces Karma. 2679 tests, 143 files, ~4s. Chromium+Firefox+WebKit in CI. | | ||
| | 2. tsgo | Blocked on Karma removal | — | Needs `moduleResolution: "bundler"` which requires `@tko/*` paths fix in tsconfig. Ready to implement once #303 merges. | |
phillipc
reviewed
Apr 14, 2026
|
|
||
| **One PR. Medium risk — touches all linted files, but output should be identical.** | ||
|
|
||
| Biome is a single Rust-native tool that replaces both ESLint and Prettier. |
Member
There was a problem hiding this comment.
We shouldn't do this. ESLint and Prettier are significantly longer on the market and more widespread
Member
Author
There was a problem hiding this comment.
Thanks @phillipc. Biome aligns better with our objectives here, with two faster OODA loops:
- biome runs much much faster, so AI/UI/TUI feedback is faster (important if you're running it millions of times, which AI should)
- biome is advancing rapidly in response to the needs of the TS/JS ecosystem, eslint and prettier are relatively stagnated
Use $(BUNX) consistently instead of bare bunx. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Makefile (3)
21-28: Add.PHONYdeclarations for non-file targets.Targets like
test,test-headless,test-headless-ff,test-headless-jquery,all,clean, andinstalldon't produce files and should be declared.PHONY. This prevents Make from skipping them if a file with the same name exists.Proposed fix - add near the top of the file
BUNX := bunx LERNA := $(BUNX) lerna DOCKER := docker +.PHONY: all test test-headless test-headless-ff test-headless-jquery \ + format format-fix tsc eslint eslint-fix dts docker-build \ + repackage bump publish-unpublished install outdated-list \ + outdated-upgrade sweep clean link default # Some make settings SHELL := bash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 21 - 28, Add a .PHONY declaration listing the non-file Make targets so Make won't treat same-named files as up-to-date; include targets test, test-headless, test-headless-ff, test-headless-jquery, test, all, clean, and install (or any other top-level phony targets defined) near the top of the Makefile so targets like test, test-headless, test-headless-ff, test-headless-jquery, all, clean, and install are declared phony.
14-14: Remove duplicatepackage.jsontarget.The empty
package.json:target appears twice (lines 14 and 68). While harmless, it's redundant and may confuse maintainers.Proposed fix
package.json: bun.lock: package.json packages/*/package.json bun install -package.json: - install: bun.lockAlso applies to: 68-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 14, Remove the duplicate Makefile target named "package.json:" by keeping a single declaration and deleting the redundant one; locate both occurrences of the target label "package.json:" and remove the second (or merge any differing prerequisites/rules if present) so only one "package.json:" target remains in the Makefile.
27-28: Either clarify the purpose oftest-headless-jqueryor remove it.This target sets
VITEST_BROWSERS=chromium, which is the default fallback invitest.config.ts. The codebase does contain jQuery-related test code (e.g., inpackages/binding.foreach/spec/eachBehavior.ts), but this Makefile target provides no special jQuery-specific configuration or test filtering. The target appears to be redundant or misleading about its intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 27 - 28, The Makefile target named test-headless-jquery is misleading because it only sets VITEST_BROWSERS=chromium (the default in vitest.config.ts) and doesn't actually select jQuery tests; either remove the test-headless-jquery target entirely, or modify it to explicitly run the jQuery-related specs (e.g., add a test filter/grep for the jQuery tests such as packages/binding.foreach/spec/eachBehavior.ts or set a specific env var that vitest uses to include jQuery fixtures) and update the target name/description to reflect that behavior; ensure the target references VITEST_BROWSERS only if needed and document why it's distinct from the default vitest run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 70: The Makefile defines the install target twice (install: node_modules
and install: bun.lock), causing one definition to be ignored; consolidate them
into a single install target by merging the prerequisites (e.g., install:
node_modules bun.lock) or choose the correct prerequisite and remove the
duplicate rule so the install target has a single, deterministic dependency
chain; update or remove any associated recipe lines under the duplicate target
to avoid leftover dead code and keep the install target name consistent.
---
Nitpick comments:
In `@Makefile`:
- Around line 21-28: Add a .PHONY declaration listing the non-file Make targets
so Make won't treat same-named files as up-to-date; include targets test,
test-headless, test-headless-ff, test-headless-jquery, test, all, clean, and
install (or any other top-level phony targets defined) near the top of the
Makefile so targets like test, test-headless, test-headless-ff,
test-headless-jquery, all, clean, and install are declared phony.
- Line 14: Remove the duplicate Makefile target named "package.json:" by keeping
a single declaration and deleting the redundant one; locate both occurrences of
the target label "package.json:" and remove the second (or merge any differing
prerequisites/rules if present) so only one "package.json:" target remains in
the Makefile.
- Around line 27-28: The Makefile target named test-headless-jquery is
misleading because it only sets VITEST_BROWSERS=chromium (the default in
vitest.config.ts) and doesn't actually select jQuery tests; either remove the
test-headless-jquery target entirely, or modify it to explicitly run the
jQuery-related specs (e.g., add a test filter/grep for the jQuery tests such as
packages/binding.foreach/spec/eachBehavior.ts or set a specific env var that
vitest uses to include jQuery fixtures) and update the target name/description
to reflect that behavior; ensure the target references VITEST_BROWSERS only if
needed and document why it's distinct from the default vitest run.
🪄 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: 1c4c3ea2-0973-43cd-b2a4-f33e6ac40a42
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.github/workflows/lint-and-typecheck.yml.github/workflows/main-build.yml.github/workflows/publish-check.yml.github/workflows/test-headless.yml.tool-versionsMakefilebuilds/knockout/helpers/mocha-test-helpers.jsbuilds/knockout/helpers/vitest-setup.jsbuilds/knockout/spec/components/componentBindingBehaviors.jspackages/builder/package.jsonplans/modern-tooling.mdtools/build.mkvitest.config.ts
✅ Files skipped from review due to trivial changes (4)
- .tool-versions
- vitest.config.ts
- plans/modern-tooling.md
- builds/knockout/helpers/vitest-setup.js
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/builder/package.json
- .github/workflows/lint-and-typecheck.yml
- .github/workflows/main-build.yml
- .github/workflows/test-headless.yml
- builds/knockout/spec/components/componentBindingBehaviors.js
- builds/knockout/helpers/mocha-test-helpers.js
Remove stale `install: node_modules` (line 70). Keep only `install: bun.lock` (line 78). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Switch test workflows to mcr.microsoft.com/playwright:v1.59.1-noble container which has Chromium, Firefox, and WebKit pre-installed. Eliminates the 5+ minute playwright install step. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Playwright Docker image doesn't include unzip, which oven-sh/setup-bun needs to extract the Bun binary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Firefox refuses to launch when $HOME is owned by a different user. GitHub Actions containers run as root but $HOME is /github/home. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Lerna topologically sorts builds based on declared dependencies. These packages had undeclared runtime @tko/* imports, causing race conditions during parallel builds: - @tko/provider needs @tko/observable - @tko/provider.native needs @tko/observable - @tko/binding.component needs @tko/provider.native - @tko/utils needs @tko/provider This was always a latent bug masked by timing under npm. Bun's faster startup makes the race more likely to trigger. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The import of Provider in options.ts is used only as a type annotation. Change to `import type` and remove the runtime dependency that created a utils→provider→utils cycle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Only preventDefault on errors matching 'ERRORS_ON_PURPOSE' or 'Some error'. Unexpected errors will propagate to Vitest normally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
brianmhunt
force-pushed
the
modern-tooling/phase-1-bun
branch
from
214586e to
f942929
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/main-build.yml:
- Around line 25-26: Replace the CI step that runs the plain bun install with a
locked install to prevent lockfile mutation: in the GitHub Actions step named
"Install dependencies" change the command that currently runs "bun install" to
use "bun install --frozen-lockfile" so the job fails if bun.lock is out of sync
instead of silently repairing it.
In `@Makefile`:
- Around line 84-86: The clean Makefile target currently removes the checked-in
lockfile (the 'clean' rule contains "rm -f bun.lock"), which should not be
deleted; update the Makefile so the 'clean' target no longer deletes bun.lock
(remove or comment out the "rm -f bun.lock" command in the clean rule) and keep
cleaning limited to build artifacts like node_modules/, or alternatively provide
a separate non-default target (e.g., "clean-all" or "dist-clean") that
explicitly removes the lockfile if you need that behavior.
- Around line 16-18: The Makefile currently treats the node_modules and bun.lock
targets as prerequisites-only, so "make install" can succeed without populating
node_modules; add a recipe that runs "bun install" when node_modules is missing
or when bun.lock changes. Specifically, update the node_modules target (and/or
the install target) to include a shell recipe invoking "bun install" (or the
project-specific install command) so that targets node_modules, all::
node_modules bun.lock, and install will actually execute installation when
needed.
🪄 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: 33942a74-ac64-4440-bf51-719348e43ac9
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/workflows/main-build.yml.github/workflows/test-headless.ymlMakefilepackages/binding.component/package.jsonpackages/provider.native/package.jsonpackages/provider/package.jsonpackages/utils/src/options.ts
✅ Files skipped from review due to trivial changes (1)
- packages/utils/src/options.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/provider.native/package.json
- packages/binding.component/package.json
- packages/provider/package.json
- .github/workflows/test-headless.yml
- Add bun install recipe to node_modules target (was missing) - Use bun install --frozen-lockfile in all CI workflows - Don't delete committed bun.lock in make clean Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
brianmhunt
pushed a commit
that referenced
this pull request
Apr 14, 2026
Remove AI_COMPLIANCE.md, AI_GLOSSARY.md, skills/plan-creation/, governance-specific plans, and the governance section from AGENTS.md. Thank you @phillipc for the effort here — the intent to bring structure to AI-assisted work is appreciated. We should have given clearer guidance earlier about what the project needs. The issue is scope: 1790 lines of compliance policy, glossary, and approval matrices add overhead that doesn't match TKO's scale as a small open-source project. Every AI agent session was forced to read ~1400 extra lines before starting work, and the 4-layer precedence hierarchy complicated what should be a single reference file. The glossary content is valuable — we'll extract the TKO-specific parts into agents/glossary.md for llms.txt. The useful AGENTS.md updates (package layout, build targets) were already preserved in the Vitest migration (#303). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 tasks
brianmhunt
pushed a commit
that referenced
this pull request
Apr 14, 2026
package-lock.json was deleted in #303. The deploy workflow used npm ci with cache-dependency-path: package-lock.json, causing all deployments to fail since the Bun migration. Switch to bun install --frozen-lockfile, remove Node setup (Bun handles everything). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 tasks
Member
|
@brianmhunt: import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; Now we can replace chai, mocha, sinon, too. It seems to be a small AI loop.. |
This was referenced Apr 15, 2026
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
.tool-versionsandbun.lockplans/modern-tooling.md)Test results
143 files, 2679 tests passed, 42 skipped, 0 failures in ~4 seconds.
Karma took ~28 seconds for the same suite.
What changed
vitest.config.ts,.tool-versions,bun.lock,builds/knockout/helpers/vitest-setup.jstools/karma.conf.js(-230 lines)builds/knockout/helpers/mocha-test-helpers.js(IIFE → ESM, this.restoreAfter → global)Compatibility
make test/make test-headlessstill work (now callbunx vitest run)bun installreplacesnpm install(819ms vs ~30s)Test plan
bunx vitest run— 143 files, 2679 passed, 42 skipped🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores
Documentation