Skip to content

fix: codify space-detection commit visibility - #613

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

fix: 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
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 children must land in the same SceneCommit and undo step as the wall edit that triggered them.

Adds wiki/architecture/space-detection.md (linked from the architecture index) describing why initSpaceDetectionSync must stay a synchronous scene-store subscriber—not subscribeSceneCommits—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. initSpaceDetectionSync gains 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.

@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.

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