fix: codify space-detection commit visibility - #613
Conversation
Aymericr
left a comment
There was a problem hiding this comment.
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:
- Rebase.
space-detection.tshas grown about 750 lines since you branched and the anchor moved, andspace-detection.test.tsnow importsrunWithSceneCommitNodeIdsfrom../store/history-control, so your new import from that module conflicts. Mechanical, but please confirm the test still passes after. - Say why the real store. Every other test in that file uses the minimal store stand-ins. Driving the real
useScenesingleton 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 restoreuseScenestate in thefinally, not justclearSceneHistory(), so the singleton mutation can't leak into a later test. - Retitle to
test:— there's no behavior change in the diff, andfix: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.
What does this PR do? / ## How to test / ## Screenshots / screen recording / ## Checklist (bun dev, bun check, docs, main-branch boxes)
Preserve
initSpaceDetectionSyncas 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
No runtime behavior change beyond comments and tests; the doc/test guard a collaboration and undo invariant that was already intended.
Overview
Documents and locks in the local commit boundary for wall-driven room reconciliation: derived slabs, ceilings, wall sides, and level
childrenmust land in the sameSceneCommitand undo step as the wall edit that triggered them.Adds
wiki/architecture/space-detection.md(linked from the architecture index) describing whyinitSpaceDetectionSyncmust stay a synchronous scene-store subscriber—notsubscribeSceneCommits—and the host patch rule for collaboration (originator reconciles and mints IDs; peers apply the transmitted graph without re-running detection).Adds an integration test that closes a room with the fourth wall and asserts one local commit contains the full reconciled graph and a single undo removes the wall plus auto surfaces.
initSpaceDetectionSyncgains an inline comment restating the zundo/history-pause timing rationale at the integration point.Reviewed by Cursor Bugbot for commit cfb544a. Bugbot is set up for automated code reviews on this repo. Configure here.