Fix ko.proxy deleteProperty trap dropping the property key by brianmhunt · Pull Request #336 · knockout/tko

brianmhunt · 2026-04-20T14:29:08Z

Summary

The deleteProperty trap on proxies built by ko.proxy (packages/computed/src/proxy.ts) declared one parameter, named property. Per the Proxy spec, the trap is invoked with (target, property) — so the handler was receiving the internal function(){} target in its one slot, stringifying it, and attempting to delete mirror[stringifiedTarget] / delete object[stringifiedTarget]. Both are no-ops. The real property key was discarded.

Effect: delete proxied.foo silently did nothing. The mirror kept the tracked observable alive, and the underlying object still had .foo.

Latent since the original ko.proxy commit in 2017. No test exercised delete on a proxied object.

Reproduction (pre-fix, stripped of TKO runtime)

before delete: mirror.foo= wrapFoo  object.foo= 1
  trap arg (named "property") is: function function () {}...
  attempting: delete mirror[ function () {} ...]
after delete:  mirror.foo= wrapFoo  object.foo= 1

Fix

One-line: add the missing _target positional parameter so property receives the real key.

Credit

Finding surfaced by @phillipc in #297 (round-2 TypeScript review findings, critical issue #1). Verified independently against current main and confirmed. Shipping as a standalone fix so #297's skill-adoption discussion can proceed without this bug blocking it.

Test plan

Regression test added to packages/computed/spec/proxyBehavior.ts — asserts delete p.b removes b from both the proxy and the underlying object.
Test fails on pre-fix code (reverted locally to confirm: assertion expect('b' in p).to.equal(false) fails because deleted property remains).
Full browser suite green: 2699 passed.
Happy-dom project green for proxyBehavior.ts.
bunx tsc --noEmit clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Fixed an issue where deleting properties on proxied objects did not properly remove them from the underlying storage. Properties would persist on both the proxy and original object. Deletion operations now correctly remove properties and return the expected result.

The deleteProperty handler bound only one parameter, `property`. Per the Proxy spec, the trap is invoked with (target, property), so the handler was actually receiving the internal `function(){}` target and attempting to `delete mirror[stringifiedTarget]` / `delete object[stringifiedTarget]`. Both are no-ops, so `delete proxied.foo` silently kept `foo` on both the mirror store and the underlying object, and its tracked observable stayed alive. Bug latent since 2017 (4d1fe0d, original `ko.proxy` commit). No spec exercised `delete` on a proxied object. Two subsequent commits (2dd14b3 adding `as any`; c8e2666 reformatting) preserved the bug because neither engaged parameter semantics. Fix: add the missing `_target` parameter. Adds a regression test that `delete p.b` removes the property from both the proxy and the underlying object. Finding surfaced by phillipc in #297. All 2699 browser tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-04-20T14:29:18Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d00ba22b-653b-45a7-bbe0-64d293cdfa8e

📥 Commits

Reviewing files that changed from the base of the PR and between 20132bd and bdac39c.

📒 Files selected for processing (3)

.changeset/fix-proxy-delete-property.md
packages/computed/spec/proxyBehavior.ts
packages/computed/src/proxy.ts

📝 Walkthrough

Walkthrough

This change fixes a bug in ko.proxy's deleteProperty trap where the handler signature didn't match the Proxy specification standard, causing properties to not be deleted correctly. The fix updates the trap signature and includes a regression test verifying proper deletion behavior on both the proxy and underlying object.

Changes

Cohort / File(s)	Summary
Proxy deleteProperty Trap Fix `packages/computed/src/proxy.ts`, `packages/computed/spec/proxyBehavior.ts`	Corrected `deleteProperty` trap parameter signature to match Proxy spec standard; added regression test confirming property deletion works correctly on both proxy and underlying target object.
Release Documentation `.changeset/fix-proxy-delete-property.md`	Added changeset entry describing the `deleteProperty` trap fix and its impact on proxy behavior.

Estimated Code Review Effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 A trap's parameter was mixed up,
Deletion's magic went astray,
But now with proper naming set,
The mirror clears, no regrets—
Delete works true, the spec's obeyed! ✨

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/proxy-delete-property

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

brianmhunt · 2026-04-20T14:57:14Z

First CI run failed with an unhandled exception: ReferenceError: Element is not defined in flushCleanQueue (jsxClean.ts) → cleanNode (disposal.js). All 2672 tests passed; the failure was a trailing timer firing after happy-dom tore down globals. Rerun passed clean.

Unrelated to the proxy fix (only touched computed/src/proxy.ts + its spec). The race is in packages/utils.jsx/src/jsxClean.ts:15 — a 25ms setTimeout(flushCleanQueue, 25) can fire after the test runner has torn down happy-dom globals. Worth a separate investigation; I'll flag it.

Copilot

Pull request overview

Fixes ko.proxy’s deleteProperty trap signature so the property key isn’t dropped, making delete proxied.foo actually remove the property and its mirror observable.

Changes:

Fix deleteProperty trap handler signature to accept (target, property) as per the Proxy spec.
Add a regression test covering delete behavior on proxied objects.
Add a changeset documenting the patch-level behavior fix.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
packages/computed/src/proxy.ts	Corrects the `deleteProperty` trap parameters so deletions target the real key.
packages/computed/spec/proxyBehavior.ts	Adds regression coverage ensuring deletions affect both proxy and underlying object.
.changeset/fix-proxy-delete-property.md	Documents the behavioral bugfix for release notes/versioning.

Copilot · 2026-04-20T15:05:52Z

      delete mirror[property as any]
      return delete object[property as any]


deleteProperty deletes from mirror before knowing whether deletion from the underlying object succeeded. If delete object[property] returns false (e.g., non-configurable property), the proxy ends up in an inconsistent state: has/get report the property missing while ownKeys and the underlying object still retain it. Consider deleting from object first and only deleting from mirror when the underlying delete succeeds (and return that boolean).

Suggested change

delete mirror[property as any]

return delete object[property as any]

const deleted = delete object[property as any]

if (deleted) {

delete mirror[property as any]

}

return deleted

brianmhunt mentioned this pull request Apr 20, 2026

Add TypeScript code review skill #297

Closed

brianmhunt marked this pull request as ready for review April 20, 2026 15:03

Copilot AI review requested due to automatic review settings April 20, 2026 15:03

brianmhunt merged commit 802ac55 into main Apr 20, 2026
12 of 13 checks passed

brianmhunt deleted the fix/proxy-delete-property branch April 20, 2026 15:03

Copilot started reviewing on behalf of brianmhunt April 20, 2026 15:03 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

phillipc mentioned this pull request Apr 21, 2026

Add TypeScript code review skill and updated findings plans #350

Open

coderabbitai Bot mentioned this pull request Apr 27, 2026

chore: version packages (v4.1.0) #372

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ko.proxy deleteProperty trap dropping the property key#336

Fix ko.proxy deleteProperty trap dropping the property key#336
brianmhunt merged 1 commit intomainfrom
fix/proxy-delete-property

brianmhunt commented Apr 20, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 20, 2026 •

edited

Loading

Review failed

Walkthrough

Changes

Estimated Code Review Effort

Poem

Uh oh!

brianmhunt commented Apr 20, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		delete mirror[property as any]
		return delete object[property as any]

Conversation

brianmhunt commented Apr 20, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Reproduction (pre-fix, stripped of TKO runtime)

Fix

Credit

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated Code Review Effort

Poem

Uh oh!

brianmhunt commented Apr 20, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

brianmhunt commented Apr 20, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 20, 2026 •

edited

Loading