Skip to content

test: codify space-detection commit visibility - #613

Open
mvanhorn wants to merge 1 commit into
pascalorg:mainfrom
mvanhorn:fix/586-space-detection-commit-visibility
Open

test: codify space-detection commit visibility#613
mvanhorn wants to merge 1 commit into
pascalorg:mainfrom
mvanhorn:fix/586-space-detection-commit-visibility

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

What does this PR do? / ## How to test / ## Screenshots / screen recording / ## Checklist (bun dev, bun check, docs, main-branch boxes)

Preserve initSpaceDetectionSync as a synchronous scene-store subscriber and add a focused rationale at that integration point explaining that reconciliation must finish within the triggering local transaction before its commit snapshot is emitted. Add an architecture page that defines the two-sided replication contract: local reconciliation output is included in the originating commit, while host patches consume those nodes under read-only mode rather than re-running reconciliation. Link that page from the architecture index so future changes to space detection, history pausing, or commit delivery encounter the invariant during design review.

Fixes #586


Note

Low Risk
Production behavior is unchanged aside from documentation and a regression test; risk is mainly future edits that move reconciliation off the synchronous subscriber or break host-patch consumption.

Overview
Documents and locks in when wall-driven room reconciliation must run relative to zundo’s SceneCommit snapshot, without changing reconciliation logic in production code.

Adds an inline rationale at initSpaceDetectionSync that reconciliation must stay on the synchronous scene-store subscriber so history-paused derived writes (auto slabs/ceilings, wall sides, level children) land in the same local commit and undo step as the triggering wall edit—not in subscribeSceneCommits, which runs after the snapshot boundary.

Introduces wiki/architecture/space-detection.md (linked from the architecture index) describing the two-sided contract: local edits reconcile and mint IDs in one complete local commit; peers apply host patches without re-running detection or adding local history.

Adds an integration test on the real useScene singleton that closing the fourth wall produces exactly one local commit whose snapshot matches live nodes (including generated surfaces and interior/exterior sides) and undoes the wall plus derived nodes together.

Reviewed by Cursor Bugbot for commit 1d573d3. Bugbot is set up for automated code reviews on this repo. Configure here.

@Aymericr Aymericr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what #586 was asking for, and the diagnosis is right — the comment at initSpaceDetectionSync names the actual mechanism (zundo snapshots after subscribers return), and the test is the guard that would have gone red on #554 instead of us catching the missing slab and ceiling by measurement. The wiki page covering both directions, including the origin: 'host' / readOnly half, is more than I expected.

Three things before I merge:

  1. Rebase. space-detection.ts has grown about 750 lines since you branched and the anchor moved, and space-detection.test.ts now imports runWithSceneCommitNodeIds from ../store/history-control, so your new import from that module conflicts. Mechanical, but please confirm the test still passes after.
  2. Say why the real store. Every other test in that file uses the minimal store stand-ins. Driving the real useScene singleton is the right call here — a stub can't exercise the zundo boundary you're asserting — but it looks like an oversight without a line saying so. Please also restore useScene state in the finally, not just clearSceneHistory(), so the singleton mutation can't leak into a later test.
  3. Retitle to test: — there's no behavior change in the diff, and fix: will read wrong in the changelog.

One note so it doesn't surprise you: my follow-up on #586 widened the ask to an equivalence harness (scoped reconciliation must produce the same spaces and surfaces as a full detectSpacesFromWalls). That's for the TopologyIndex work when it returns, not a condition on this. Your commit-visibility assertion stands on its own and I want it in main now.

Adds the regression guard that would have gone red on pascalorg#554: asserts the
derived slab/ceiling writes land inside the originating local SceneCommit
rather than after it, plus a comment at initSpaceDetectionSync naming the
zundo snapshot boundary that makes the synchronous store subscription the
required home for reconciliation.

The test drives the real useScene singleton rather than the minimal store
stand-ins the rest of the file uses, because a stub cannot exercise that
boundary. It captures and restores the singleton's state in the finally
block so the mutation cannot leak into a later test.

No behavior change, hence test: rather than fix:.
@mvanhorn
mvanhorn force-pushed the fix/586-space-detection-commit-visibility branch from cfb544a to 1d573d3 Compare August 30, 2026 09:18
@mvanhorn

Copy link
Copy Markdown
Contributor Author

All three done.

Rebased. Onto current main (81 commits). Two conflicts, both from the growth you flagged: space-detection.test.ts now also imports runWithSceneCommitNodeIds from history-control, merged with the subscribeSceneCommits / SceneCommit imports; and in space-detection.ts the new adoptSceneBaseline / non-local commit block landed where my comment goes, so the comment now sits directly above sceneStore.subscribe where it explains that choice. Confirmed after: 62/62 in space-detection.test.ts.

Said why the real store. The test now carries a line stating that the real useScene singleton is required to exercise zundo's commit snapshot boundary, since a stand-in cannot. It also captures useScene.getState() up front and restores it with useScene.setState(previousSceneState, true) in the finally, alongside clearSceneHistory(), so the singleton mutation cannot leak into a later test.

Retitled to test:. Agreed on the changelog reading — there is no behavior change in the diff.

Noted on the equivalence harness being scoped to the TopologyIndex work rather than this PR.

@Aymericr Aymericr changed the title fix: codify space-detection commit visibility test: codify space-detection commit visibility Aug 31, 2026
@Aymericr

Copy link
Copy Markdown
Contributor

Verified the rebase — parent is 67ea40ef, single commit, and both conflict areas came through clean: one adoptSceneBaseline, one non-local commit block, and the comment sitting directly above sceneStore.subscribe where it now belongs (space-detection.ts:2430). The finally restore with useScene.setState(previousSceneState, true) is exactly what I asked for, and the one-line justification at the top of the test reads right.

One thing left, and it's yours: the red quality check is formatting only. The import you hand-merged at packages/core/src/lib/space-detection.test.ts:6 is 109 chars against our lineWidth: 100, and biome wants it split. bun run check --write (or just break the import) fixes it. It also matters beyond lint: bun run check runs before bun run test in the quality job, so CI has never actually executed the suite on this head — once the format fix lands and quality goes green, that's the 62/62 confirmation I need.

I retitled the PR to test: to match the commit (with a second commit on the branch, the squash title comes from the PR title, and fix: would have snuck back into the changelog).

Fix the line and I'll merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reconciliation-generated nodes have no defined path to collaborators

2 participants