Circuit-Editor and Visualization Re-Architecting and Tests by ScottCarda-MS · Pull Request #3425 · microsoft/qdk

ScottCarda-MS · 2026-07-08T17:40:32Z

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

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.
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.
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.
Persists view state. Group expand/collapse and related view state survive
re-render via the ViewState type threaded through sqore.ts.
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:

Data layer (data/) — small, pure value types (Location,
CircuitModel, ViewState). The vocabulary everything else is written in.
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.
Renderer (renderer/) — Circuit → SVG; reads data, never mutates.
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.
Entrypoint (sqore.ts, utils.ts, index.ts) — ties it together.
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.

…g references to planning docs

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.

amcasey · 2026-07-14T17:00:55Z

+});
+
+// ---------------------------------------------------------------------------
+// removeOperation


I may have overlooked tests for the following:

Removing the only (remaining) gate

Removing a gate at an invalid location

Take a look at these tests:

removeOperation of the only remaining gate empties the circuit

removeOperation at an out-of-bounds location is a safe no-op

removeOperation at an in-range but empty interior slot is a safe no-op

amcasey · 2026-07-14T17:01:21Z

+const unitary = (/** @type {string} */ g) => gate(g, 0);
+
+// ---------------------------------------------------------------------------
+// addOperation


What happens if I add an operation at 0,1000 or 1000,0?

They will return null. Look at these tests:

addOperation: an op index one past the column's append slot returns null

addOperation: a column index one past the trailing-append slot returns null"

amcasey · 2026-07-14T17:05:14Z

+  removeOperation(model, "0,0-0,1");
+
+  // Outer group remains; the X inside its single child column is gone.
+  expectOp(at(model, "0,0"), { Group: { children: [[{ H: 0 }]] } });


What happens if you remove the H too?

It should prune any empty groups, but I realize I only tested that for the move-actions, and not for the pure delete-action. Updated!
removeOperation: removing a group's last child prunes the emptied group

amcasey · 2026-07-14T17:06:19Z

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+// Remove-mutator tests on grouped shapes, driven through the public


The file name seems to imply that there will also be add tests?

Some existing tests were spread across other files. I've moved them here to consolidate and also added some tests in-line with the addRemove test file.

amcasey · 2026-07-14T17:08:42Z

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+// Ancestor `.targets` cache refresh after a child mutation: when


I had trouble following these tests (just my own ignorance) but I'm wondering if the following are covered (here or elsewhere):

Moving a gate into a parent group

Moving a gate into a grandparent group

Moving a gate into a child group

Moving a gate into a sibling group

Or possibly we don't allow any of those changes.

Moving a gate "into" its parent group is just moving the gate inside the group it already is in. Here is an example test: moveOperation: moving an internal gate to its group's trailing inner-column slot keeps it inside the group

Good idea, added a new test: moveOperation: promoting a gate into its grandparent group lands it beside the parent group

This doesn't make sense to me. We don't currently support moving groups (look to a future PR for that), but even when we do, you are asking about a group moving into itself. I'm not sure how we'd express that in the current API. If you have more thoughts on this, let's follow up on this offline.

Good idea, added a new test: moveOperation: moving a gate into a sibling group relocates it across scopes

amcasey · 2026-07-14T17:47:21Z

+
+  const prompt = getOpenPrompt();
+  assert.ok(prompt);
+  prompt.okButton.click();


Is there coverage of what happens when cancel is clicked?

There's a test above this one that checks for cancel behavior for this scenario, and cancel is generally tested throughout the tests here. Maybe I'm missing what you are asking about?

amcasey · 2026-07-14T17:49:37Z

+  assert.equal(el().scrollTop, 10);
+});
+
+test("mousemove near the left edge scrolls left", () => {


Do we have some reason to believe the left and right edges might have different scrolling behavior? Some of these tests seem redundant.

Yes, the left edge is special and has special behavior because we expect users to drag gates from the toolbox from the left side onto the circuit. The special behavior is that we disable scrolling at first from the left when dragging from the toolbar, but once the drag gesture has gotten significantly past the left edge, then it becomes enabled again and the user can scroll along the left edge like a normal edge.

amcasey · 2026-07-14T17:54:22Z

+  const toolbox = createToolboxElement(() => {});
+
+  const buttons = toolbox.querySelectorAll(".svg-run-button");
+  assert.equal(buttons.length, 1, "exactly one Run button expected");


Is there product code that could generate more than one button and then simplifies away the redundant ones? Why are we concerned that there might be too many Run buttons?

It is really just asserting that there is a button in this scenario, because the previous test covers the scenario where there is no button.

amcasey · 2026-07-14T17:57:53Z

+- `angleExpression.ts` — parse/normalize/evaluate gate rotation
+  angle expressions (handles `pi`/`π` and simple arithmetic).
+
+The hard rule that drives the layering: **`data/` and `actions/`


Is there a way we can enforce that they don't touch the dom?

Since the tests that run off of these layers don't have a DOM, if someone adds a querySelector call, for example, to the logic of these layers, the tests will complain about not having a document to query. Sort of an indirect way to enforce it though.
I could add lints to enforce this.

I looked into adding lints. It is doable, but I kind of thing it is overkill for this.

fedimser · 2026-07-15T17:47:17Z

Here are some findings from AI-assisted review.

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

fedimser · 2026-07-15T16:55:43Z

Please include in this doc explanation why it's called "Sqore".

is this an abbreviation or pun on "score"? If I don't know the origin of a term, I will be wondering about it every time I see word "sqore" which is distracting.

I think it is better to just explain what I can here instead of clutter the doc with historical context. "Sqore" was the original name of the circuit visualization library when we inherited it. This used to be a separate tool developed by another team under our org. I think it is a reference to musical scores but used a "q" because that's common in quantum to insert "q" into the name of things.
I'm not opposed to changing the name, but nothing specific comes to mind, and I thought it was harmless for now to keep it the original name.

circuit-editor foundation re-arch

c2cd30d

github-advanced-security AI found potential problems Jul 8, 2026

View reviewed changes

ScottCarda-MS added 2 commits July 8, 2026 11:41

Added the architecture markdown file

28ff077

Merge branch 'main' into sccarda/ce-foundation

384c38d

ScottCarda-MS changed the title ~~circuit-editor foundation re-arch~~ Circuit-Editor and Visualization Re-Architecting and Tests Jul 8, 2026

ScottCarda-MS requested a review from Copilot July 8, 2026 19:11

Copilot started reviewing on behalf of ScottCarda-MS July 8, 2026 19:11 View session

Copilot AI reviewed Jul 8, 2026

ScottCarda-MS marked this pull request as ready for review July 8, 2026 19:22

ScottCarda-MS requested review from amcasey and billti as code owners July 8, 2026 19:22

ScottCarda-MS added 6 commits July 9, 2026 11:16

make the mermaid diagrams more readable

8568700

add some missing content to the Architecture file

f0da218

decided the benchmarking should be excluded, cleaned up some remainin…

a086285

…g references to planning docs

consolidate data model tests

9d26c36

consolidated/removed some tests in the action/view layers

099257a

dropped a couple of view layer tests

e346b0e

amcasey reviewed Jul 14, 2026

View reviewed changes

ARCHITECTURE file to replace old README

5af8dea

fedimser requested changes Jul 15, 2026

View reviewed changes

fedimser reviewed Jul 15, 2026

View reviewed changes

Comment thread source/npm/qsharp/ux/circuit-vis/actions/circuit-actions/classicalRefs.ts Outdated

fedimser requested changes Jul 15, 2026

View reviewed changes

fedimser approved these changes Jul 15, 2026

View reviewed changes

ScottCarda-MS added 6 commits July 15, 2026 15:13

Don't wrap comments below 100 cols

2bcd93d

comments don't wrap under 100 cols in tests

5739711

condensed comment

22d93c4

added some addRemove tests

5d7db8a

added some movement tests

f41ddcf

reworded

2cc4e21

ScottCarda-MS added 9 commits July 16, 2026 14:12

remove empty out groups

f8cae3d

remove unused import

f535adb

Move/added some group-add tests

935bc2e

add test for moving a doomed circuit wire

d4c2059

more instructions about the geometry constants in the test file

57c6560

Remove local parseLocation in test.

d8dd8b6

small rename

10e8bea

trim some tests

ca557f9

reflow README

91a3f56

Uh oh!

Conversation

ScottCarda-MS commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

At a glance

What this PR does

Files removed (old monoliths)

How to review this efficiently

What to focus your scrutiny on

What you can safely skim

Testing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedimser commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ScottCarda-MS commented Jul 8, 2026 •

edited

Loading

fedimser commented Jul 15, 2026 •

edited

Loading