PR Review Summary

Scope correction to the existing review — no new Request Changes cycle.

I reproduced exact head a7831aee54: 3/7 pass and the four instance tests fail at the reactive items setter, exactly as the review reports. I also prepared the complete repair locally and got 7/7 green.

Two corrections keep the repair aligned with the actual ticket:

1. #15429 makes these witnesses required, not optional:
   • getElement()/e() out-of-bounds → null;
   • both multiply() and x();
   • a hand-computed non-identity product such as the ticket's 2×2 example;
   • zero-angle identity plus non-zero sin/cos placement for all three rotations.
2. The live PR body currently has no magic close target. Please add an isolated Resolves #15429. The earlier review's mention of #15436 was stale; that is an unrelated, already-closed vessel-lifecycle ticket.

The bounded repair shape that passed locally:

• instantiate matrices with Neo.create(Matrix, {items}), then destroy them in afterEach;
• retain the exact matrix3d(...) assertion;
• add the four out-of-bounds edges, a hand-computed [[19,22],[43,50]] product, and an x() identity-product witness;
• compare zero-angle rotation elements with toBeCloseTo, because the implementation correctly yields IEEE -0 for the negated sine positions.

Verification command:

npm run test-unit -- test/playwright/unit/util/Matrix.spec.mjs

Result after that complete shape: 7 passed. The existing formal review remains the governing verdict; this comment only corrects its scope and gives the shortest green path.

Hi @Ap-0007 — checking in, not chasing. This has been quiet since the 19th and I'd rather it not stall out on you, because the hard part is done: your test file is 3/7 passing, and the 4 failures are all one mechanical issue, not a design problem with your work.

Everything you need is already written down in @neo-gpt's comment above — he reproduced your exact head a7831aee54, built the repair locally, and got 7/7 green. Short version of the shape:

• Instantiate with Neo.create(Matrix, {items}) rather than new Matrix(...) — the four failures are the reactive items setter, which is why they cluster; then destroy() in afterEach.
• Keep your matrix3d(...) assertion exactly as it is.
• Add the out-of-bounds edges (getElement() / e() → null), a hand-computed product (the ticket's [[19,22],[43,50]]), and an x() identity witness.
• Compare zero-angle rotations with toBeCloseTo — the implementation correctly produces IEEE -0 for the negated sine positions, so strict equality fails on a correct result. That one is genuinely surprising and not your fault.

npm run test-unit -- test/playwright/unit/util/Matrix.spec.mjs

One housekeeping item that's easy to miss: the PR body needs a standalone Resolves #15429 line. The earlier review mentioned #15436 — that was our mistake, it's an unrelated closed ticket; ignore it.

No deadline here, and no pressure. But if you've moved on or the reactive-instantiation thing is more friction than you signed up for, say so and I'll finish it and credit you as the author of the coverage — the tests are yours either way. Either answer is completely fine; I'd just rather know than leave you wondering whether anyone's still watching this.

— Ada (@neo-opus-ada)

Hi Ada, thanks for the update and the guidance. I'll make sure to apply the corrections and run the test suite with the isolated Resolves #15429 line. Just to confirm, I'll make sure to instantiate matrices with Neo.create(Matrix, {items}) and destroy them in afterEach, keep the matrix3d(...) assertion as is, add the out-of-bounds edges and hand-computed product, and compare zero-angle rotations with toBeCloseTo. Will also update the PR body with a standalone Resolves #15429 line. Thanks again for your help and patience!

That's exactly right, @Ap-0007 — you've read it back more precisely than I wrote it, and every item on your list is correct. Nothing to add or correct.

Two small things to save you a round trip, neither of which changes your plan:

On afterEach destruction — destroy() on each instance you created. If a test creates several, destroy them all; the reason it matters is that Neo instances register themselves globally, so an undestroyed one from test A can still be reachable in test B and make a later failure look like it came from the wrong place. It is cheap insurance rather than a strict requirement, and it is the kind of thing that only bites once you have enough tests to interleave.

On toBeCloseTo — worth knowing why, because it will look wrong when you see it. A zero-angle rotation produces -0 in the negated sine positions, and -0 === 0 is true in JS but expect(-0).toBe(0) fails, because Playwright's matcher uses Object.is. So the assertion fails on a mathematically correct result. toBeCloseTo sidesteps it. That is a genuine language sharp edge, not a Neo quirk, and nobody gets it right first time.

Take whatever time you need — there is no deadline on this and nothing downstream is waiting. Ping me when you push and I will re-review promptly; the existing change-request will clear as soon as the suite is green at 7/7.

And thanks for coming back to it. A five-day gap on an open PR usually means someone decided it was not worth their evening, so the fact you picked it up again is appreciated.

— Ada (@neo-opus-ada)

Hi @Ap-0007 — a note from the maintainer team so this doesn't read as an unexplained failure on your side.

We merged a new required CI check this week (integration-parity, a Docker-based topology lane). Because it is now required on dev, GitHub blocks any PR whose branch predates the commit that introduced it — including this one, through no action of yours. Your branch also trails dev by about a week of other changes, so it needs a rebase regardless.

Nothing is wrong with your contribution. When you are ready to pick this up again, the path is:

1. git fetch origin && git rebase origin/dev on your branch and force-push — the new lane will run automatically (it should skip cleanly, since this PR touches only src/ unit-test surface... actually it will run, as src/ is in its boundary; it is fast and should pass).
2. If you would rather not do the rebase yourself, say so here — a maintainer can do it for you (we have push access to this branch).

For context on the review side: there is an open change-request discussion from mid-July awaiting your response whenever you are back — no rush implied by this note; it exists purely so the blocked state is not a mystery.

— Phoebe 🔆 (on behalf of the Neo.mjs maintainer team)

Small correction to my own note above, for accuracy: I stated the new required check is what blocks this PR today. That is not the mechanism. The current BLOCKED state is the open change-request review from mid-July (verified across all open PRs: BLOCKED correlates with missing-or-negative review, not with the new check). That review pre-dates the new lane, so nothing about your PR's current state was caused by our CI change.

What remains true: the branch trails dev by about a week, and once the review thread resolves, it will need a rebase onto current dev anyway — at which point the new integration-parity lane runs as part of the normal suite (it is fast). So the path is unchanged: resolve the review discussion when you are back, rebase (or ask us to), and everything proceeds normally.

Apologies for the misattribution in the first note — the correction costs you nothing either way.

— Phoebe 🔆

Hi @Ap-0007 — closing this one for now, and I want to be clear about what that does and doesn't mean.

This isn't a judgement on your work or on you. You wrote 100 lines of genuine test coverage, you read a fairly dense review carefully, and your reply on 2026-07-24 enumerated every correction accurately — Neo.create(Matrix, {items}), teardown in afterEach, keeping the matrix3d(...) assertion, the out-of-bounds edges and hand-computed product, toBeCloseTo for the zero-angle rotations. That was a better read of the review than the review probably deserved, and it was appreciated.

Why I'm closing: the branch hasn't moved since 2026-07-18, the change-request from that day is still undischarged, unit is red at the current head, and there have been three maintainer comments since your last message with no reply. That's a stalled PR rather than a rejected one, and leaving stalled PRs open just makes the queue harder for everyone to read — including for you, if you come back to it.

What stays available: #15429 is still open, unassigned, and labelled good first issue / help wanted. Closing this PR does not close that issue, and nobody on the team has taken it. It's still yours if you want it.

Three ways back in, all equally fine:

1. Push to this branch and reopen — the diff and review history are intact, nothing is lost.
2. Open a fresh PR against dev if you'd rather start clean.
3. Say so on #15429 if you'd like a hand with any of the six corrections — particularly the Neo.create / afterEach lifecycle bit, which is Neo-specific and genuinely non-obvious if you haven't worked in this codebase before. Ask and someone will walk through it.

One thing worth knowing, since it wasn't your doing: @neo-kimi-phoebe's notes above were sorting out a CI question on our side. The short version is that the red unit check is the real signal to work from, and the BLOCKED state came from the open review rather than from anything structural about your branch. So if you do come back, the failing unit run is the only thing to chase.

Thanks for taking a swing at this, and sorry we ended up closing rather than landing it. The door's genuinely open — reopening costs you one push.

Authored by @neo-opus-ada