Repository navigation
fix(manager): isA() walks past component.Base to any ancestor (#19130) - #19154
Conversation
…#19130) isA() short-circuited to false as soon as the walk reached Neo.component.Base and the requested ancestor was not Neo.core.Base. That encoded an assumption that component.Base extends core.Base directly, which stopped being true once Neo.component.Abstract was inserted between them: isA('Neo.button.Base', 'Neo.component.Abstract') returned false for every component subclass. Drop the short-circuit. The loop already ends at Neo.core.Base, so the walk now sees every class between a descendant and the root, whatever sits there. The memoization is unchanged. The new spec pins the parentClassName chain, the ancestor case that used to fail, a set of non-ancestor cases and the isAQueryMap behaviour.
neo-fable
left a comment
neo-fable
left a comment
There was a problem hiding this comment.
PR Micro-Review
Opening: Thank you — specifically for removing the false short-circuit instead of special-casing Neo.component.Abstract, and for the memoization test that pins the answer cache the ticket asked you to keep. To watch the engine you just fixed run, set up the Engine from a clone — three commands — then open apps/workstation and drag a tab out of the browser; why that works. Your agent needs neither our Knowledge Base nor Memory Core — neo-agent-skills is on npm.
Class: contained — a curated good-first-issue fix (#19130) in one manager method plus its unit spec; the concept (walk the registered parentClassName chain to Neo.core.Base) is the ticket's, and the diff teaches no new one.
Verdict: Approved
Glance: The claim holds against the source: Neo.component.Base extends Neo.component.Abstract, which extends Neo.core.Base, so the removed guard ("component.Base directly extends core.Base") answered false for any ancestor above component.Base other than core.Base. With it gone the walk ends at core.Base as before and the cache is written after the loop, unchanged. Because the ticket asked for the failing output and the body summarises it instead, I ran the new spec myself: against dev's ClassHierarchy.mjs it reads 2 failed / 4 passed ("isA() finds ancestors that sit between component.Base and core.Base" and "isA() memoizes its results", the second because the cached Abstract answer was false); with this head's ClassHierarchy.mjs it reads 6 passed. CI is green at 4a495ae4d6 (unit, components, e2e shards), MERGEABLE/CLEAN. The ticket's "Done when" list is covered: self → true, direct parent, transitive ancestor, unrelated and unknown → false, and the cache — the "second identical call" item is witnessed through the map entries rather than a second call's return value, which pins the same fact. One thing I looked for and did not find: any other caller relying on the old early false (a git grep isA( over src shows lookups by descendant/ancestor only, none keyed on the shortcut).
Findings: None. No required actions — eligible for human merge.
- Origin Session ID: 091a1019-7f73-4117-b281-99190ceb224f
🖖 Mnemosyne · Fable 5.1 · Claude Code
3 participants
Does this PR resolve an issue? (Required)
Resolves #19130
isA() stopped walking as soon as it reached Neo.component.Base unless the ancestor asked for was Neo.core.Base, which assumed component.Base extends core.Base directly. Since Neo.component.Abstract now sits between them, isA('Neo.button.Base', 'Neo.component.Abstract') returned false. This drops that short-circuit so the walk continues to core.Base like any other chain, and adds a ClassHierarchy unit spec; two of its tests fail on dev and pass with the fix.
Written with Claude Code (Claude Opus).
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
The PR fulfills this requirement:
devbranch, not themainbranch