Circuit-Editor and Visualization Re-Architecting and Tests

[ScottCarda-MS]

Circuit Editor Re-Architecture: Layered Foundation (Data → Actions → Renderer → Editor)

Reviewer TL;DR. This is an internal re-architecture of the
circuit-vis circuit editor. It replaces the four monolithic editor files
with a layered data / actions / renderer / editor structure and includes
view-state persistence and the measurement-dependency confirm UX. No public
API changes and no user-facing behavior regressions. It is a single cohesive
commit because the core is a mutually-dependent refactor that cannot compile in
smaller pieces; see the How to review this efficiently section below.

---

At a glance

Scope	source/npm/qsharp/ux/circuit-vis/** + source/npm/qsharp/test/** only
Size	114 files, ~+25,500 / −3,956
Language	TypeScript (frontend) + .mjs tests. No Rust, no VS Code host changes.
Public API	Unchanged — index.ts exports the same surface
Tests	392 circuit-editor tests across 28 files, all green; plus snapshot fixtures
Behavioral risk	Low for existing flows; new capability is additive

---

What this PR does

1. Splits the monoliths into a layered architecture. The old editor was
   four large files with tangled responsibilities. They are removed and
   replaced with a strict, one-directional layering:

   data/  →  actions/  →  renderer/  →  editor/  →  sqore.ts  →  state-viz/
   (pure)    (pure)       (read data)   (DOM glue)  (entrypoint) (parallel)
   

   The hard rule: data/ and actions/ never touch the DOM. The renderer
   reads data to produce SVG; the editor glues DOM events to action calls.

2. Adds group-aware editing foundations. Editing inside expanded groups,
   the group structure and add/remove/ancestor-refresh/collision-split
   behaviors, single-leg move and control-drag paths, and the dropzone geometry
   the editor is built on.

3. Hardens classical-register / measurement integrity. Moving or deleting a
   measurement that later gates depend on now cascades correctly (with a confirm
   prompt) instead of crashing or leaving dangling classical references.

4. Persists view state. Group expand/collapse and related view state survive
   re-render via the ViewState type threaded through sqore.ts.

5. Adds an in-tree architecture guide. A new
   ux/circuit-vis/ARCHITECTURE.md
   documents the layering, the render/click flows, and the module invariants —
   useful for this review and as durable documentation afterward.

Files removed (old monoliths)

• circuitManipulation.ts
• events.ts
• panel.ts
• draggable.ts (old version)
• contextMenu.ts (old version)

Their responsibilities now live in the layered modules.

---

How to review this efficiently

This PR adds a structural walkthrough alongside the code:
ux/circuit-vis/ARCHITECTURE.md.
Read its TL;DR and module map first — it explains the layering, has diagrams of
a render and a click flow, and lists the invariants. Then follow the order
below.

Suggested order and natural stopping points:

1. Data layer (data/) — small, pure value types (Location,
   CircuitModel, ViewState). The vocabulary everything else is written in.
2. Action layer (actions/ + actions/circuit-actions/) — pure mutations;
   the correctness core. Cross-reference the
   test/circuit-editor/circuit-actions/ suite as you go.
   → Good checkpoint: you've now seen the entire pure core.
3. Renderer (renderer/) — Circuit → SVG; reads data, never mutates.
4. Editor (editor/ + editor/controllers/) — DOM glue. Read the shared
   InteractionContext first, then the controllers.
   → Good checkpoint: trace one end-to-end flow from ARCHITECTURE.md.
5. Entrypoint (sqore.ts, utils.ts, index.ts) — ties it together.
6. state-viz (state-viz/) — parallel subsystem, only import-path updates
   here; safe to skim last.

What to focus your scrutiny on

You don't need prior familiarity with this code to review it well. If you only
have time for a few things, these are the questions worth asking — each one is
answerable by reading a file or two:

• Does the split preserve behavior? This is a refactor, not a rewrite of
  what the editor does. The tests are the contract: if the suite is green and
  the snapshot fixtures are unchanged, existing behavior is preserved. Skim the
  test names in test/circuit-editor/ to see what's covered.
• Are the layers actually kept separate? The whole point of the split is a
  one-way dependency: the pure logic (data/, actions/) computes circuit
  changes, and only the editor/ layer touches the browser/DOM. Spot-check a
  couple of data/ and actions/ files — they should have no DOM or
  browser-API calls. If they don't, the separation holds.
• Is rendering read-only? Drawing a circuit should never change it. Confirm
  the render path in sqore.ts works on a copy, so the caller's circuit object
  is left untouched.
• Does the naming/structure make sense to you as a newcomer? You are exactly
  the audience the new layout is meant to help. If a folder or file name doesn't
  match what's inside, that's useful feedback.

Everything else (cache-consistency details, controller wiring, drag cleanup) is
covered by the test suite.

What you can safely skim

• state-viz/ diffs — mechanical import-path updates from the move.
• Snapshot fixtures under test/circuits-cases/ — generated.

---

Testing

• 392 circuit-editor tests across 28 .mjs files, all passing (data,
  actions/circuit-actions, renderer, controllers, sqore), plus snapshot
  fixtures under test/circuits-cases/.

• The action layer is the most heavily covered: the
  test/circuit-editor/circuit-actions/ suite pins the single-leg move /
  control / classical-ref-remap / group-structure paths.

• Run locally from source/npm/qsharp/:

  # build the UX bundle
  npm run build:ux
  # run the circuit-editor suite
  node --test "test/circuit-editor/**/*.test.mjs"

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

[amcasey]

As discussed, I focused my review on the tests, since I understand the rest is basically just shuffling code around.

I was a little surprised not to see more validation of our handling of bad input, given that users and copilot can directly edit qsc files.

[fedimser]

Here are some findings from AI-assisted review.

1. Bug: View-state can leak across unrelated circuit replacements.
   The new in-place update path always reuses the same Sqore instance in circuit.tsx:84. Sqore explicitly says a clean draw should be used when opening a different circuit in sqore.ts:168, but the component never does that after first mount. This can carry expand/collapse preferences to a different circuit if locations happen to match.

2. Bug: Run button presence is fixed on first shell mount. Toolbox creation is callback-dependent in toolbox.ts:56, while shell mounting is one-time in shell.ts:72. If run callback availability changes later, the button will not appear/disappear accordingly because the panel is not rebuilt.

3. Inconsistency between ARCHITECTURE and implementation: Layering rule is documented as strict, but dependency boundaries are not actually strict.
   Architecture claims no outgoing dependencies from data/actions in ARCHITECTURE.md:30 and ARCHITECTURE.md:60, yet action/data import helpers from utils.ts:4, and that file imports renderer types/constants and DOM-query helpers.

4. There are behavior changes:

   • New public API on draw result: updateCircuit in index.ts:30.
   • In-place circuit replacement flow in UI component (preserves session view state, avoids rendering flicker) in circuit.tsx:79.
   • Measurement move/delete now prompts and cascades dependent-consumer handling in prompts.ts:113 and prompts.ts:162.
   • Group/mixed-control rendering behavior changed (quantum controls on classically controlled bodies/groups) in gateFormatter.ts:755.
   • Toolbox Run button behavior changed to optional rendering (not hidden-by-default) in toolbox.ts:53.
   • Utility lookup behavior now explicitly returns null for out-of-bounds locations (instead of throwing in those paths) in utils.ts:412.

5. These new features can be (in principle) split up in a separate PR:

   • updateCircuit API + in-place update semantics (index.ts:30, circuit.tsx:79).
   • ViewState persistence/rebase system (sqore.ts:101, viewState.ts:45).
   • Measurement-dependent confirmation/cascade UX (prompts.ts:113).
   • New group-control rendering geometry fixes (process.ts:363, gateFormatter.ts:777).

6. Tests that can be added:

   • Add focused updateCircuit lifecycle tests to define and lock behavior for view-state persistence vs reset (same-circuit updates, different-circuit replacement, and handler/options changes).
   • Add editor-shell reconfiguration tests to verify Run button behavior when runCallback availability changes across rerenders.
   • Add action-layer negative tests for malformed/out-of-range/stale locations to ensure add/remove/move are safe no-ops (no throws, no unintended mutation).
   • Add one end-to-end editor smoke test that chains drag, context-menu control add/remove, measurement-with-dependents move/delete confirmation flow, and rerender/update cycle to catch cross-layer regressions.

For 4-5, I asked Copilot to find what this PR does beyond pure refactoring. Ideally, I'd suggest having a PR doing pure refactoring, and then add new features in a follow-up PR(s), to minimize risk of bugs in the large refactoring PR. I am not asking to do the split, just review these items and make sure these do not introduce bugs.

For 6, I asked Copilot to suggest what tests could be added, so feel free to ignore it.

[ScottCarda-MS]

1. Bug: View-state can leak across unrelated circuit replacements.
   The new in-place update path always reuses the same Sqore instance in circuit.tsx:84. Sqore explicitly says a clean draw should be used when opening a different circuit in sqore.ts:168, but the component never does that after first mount. This can carry expand/collapse preferences to a different circuit if locations happen to match.
2. Bug: Run button presence is fixed on first shell mount. Toolbox creation is callback-dependent in toolbox.ts:56, while shell mounting is one-time in shell.ts:72. If run callback availability changes later, the button will not appear/disappear accordingly because the panel is not rebuilt.
3. Inconsistency between ARCHITECTURE and implementation: Layering rule is documented as strict, but dependency boundaries are not actually strict.
   Architecture claims no outgoing dependencies from data/actions in ARCHITECTURE.md:30 and ARCHITECTURE.md:60, yet action/data import helpers from utils.ts:4, and that file imports renderer types/constants and DOM-query helpers.

I fixed Bug 1. 2 is intentional behavior, the host will not change its behavior on whether it supports running or not between redraws. For 3, I have updated to try to make a more clear separation between the DOM-enabled layer and the DOM-disabled layers.