Fix ComponentSystemRegistry.remove leaving the system in the list

[willeastcott]

Description

ComponentSystemRegistry.remove() never removed the system from list.

The array lookup was evaluated after the id had already been deleted:

delete this[id];

// Update the component system array
const index = this.list.indexOf(this[id]);  // indexOf(undefined) -> -1
if (index !== -1) {
    this.list.splice(index, 1);
}

this[id] is undefined by the time indexOf runs, so the splice never
executed and the removed system stayed in the array. Since destroy() iterates
list, an already-removed system was still destroyed on app teardown.

This PR performs the splice before deleting the id — the fix is a pure reorder.

There are no in-tree callers of remove(), so this only affects code that
unregisters a custom component system.

Adds test/framework/components/registry.test.mjs covering add(), remove()
and destroy(). Three of its eight cases fail without the fix, including one
asserting that a removed system is not destroyed on teardown.

Type declaration tidy-up

While in the file, three annotations that were producing weak declarations:

• add() and remove() were typed {object}, with the real type only conveyed
  by an {@link ComponentSystem} in the description, because the base class was
  missing from the file's @import block — only the concrete subclasses were
  listed. They now emit add(system: ComponentSystem) rather than
  add(system: object), and the redundant {@link} is dropped since the type
  annotation links.
• list emitted as any[]; it is now annotated ComponentSystem[].
• destroy() carried no JSDoc at all, unlike add() and remove().

list and destroy() are tagged @ignore, so the documented API surface is
unchanged — verified by building the docs and confirming neither appears on the
ComponentSystemRegistry page. This deliberately does not touch the question of
whether list should become public API; that belongs with #8086.

Checklist

• I have read the contributing guidelines
• My code follows the project's coding standards
• This PR focuses on a single change

[github-actions]

Build size report

This PR changes the size of the minified bundles.

Bundle	Minified	Gzip	Brotli
playcanvas.min.js	2391.2 KB —	615.0 KB —	477.4 KB (−0.0 KB, −0.00%)
playcanvas.min.mjs	2388.6 KB —	613.6 KB (+0.0 KB, +0.00%)	476.6 KB (+0.0 KB, +0.01%)

[Copilot]

🟡 Changes recommended

remove()’s existence check can be bypassed by inherited properties (e.g. constructor), so it may not throw for unregistered ids and should verify the id is an own property of the registry instance.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes ComponentSystemRegistry.remove() leaving removed component systems in list, which could cause removed systems to be destroyed later during registry teardown.

Changes:

• Reorders remove() to splice the system out of list before deleting the registry entry.
• Adds unit tests covering add(), remove(), and destroy() behavior for the registry.

File summaries

File	Description
src/framework/components/registry.js	Fixes remove() ordering so the removed system is actually removed from list.
test/framework/components/registry.test.mjs	Adds coverage to prevent regressions in registry add/remove/destroy behavior.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[willeastcott]

Good catch, fixed in d6f19d7 — remove() now also requires the id to be an own property. Note an own-property check alone would have regressed the common case: the class declares a field per known system id (render, camera, ...) and those are own properties holding undefined, so hasOwn alone stops throwing for unregistered-but-declared ids. Both conditions are needed, and there is now a test for each.

🤖 Addressed by Claude Code

[willeastcott]

Correction: this has been backed out in fae660f. The finding is valid, but the guard used Object.hasOwn, which is not yet used anywhere else in the engine, so we are keeping this PR to the ordering fix rather than introducing it here. remove() still no-ops rather than throwing for an id that resolves to an inherited member — tracked separately.

🤖 Addressed by Claude Code