Round-5 review fixes: two PR #64 regressions, undo/redo, embed, engine, a11y - #65
Merged
Merged
Conversation
Both were introduced by round-4 fixes and neither existing test caught them. Monte Carlo progress stopped updating entirely. The async driver drains yields for a 14ms chunk and then inspects the last one, and it reported progress only when that last yield ended a trial. Once the generator began yielding after every step (the round-4 responsiveness fix), well under 1% of yields ended a trial, so a chunk almost never stopped on one: the dialog sat at 'Running...' with a 0% bar for the whole batch, sweeps and sensitivity runs included. Measured against the pre-PR engine on the same model: 100x200 gave 8 progress callbacks before and 0 after, 1000x300 gave 76 before and 0 after. Every yield already carries the completed-trial count, so the chunk's last yield is always current: report it whether or not it ended a trial. Restored to 8 and 85. Typing into a Delay's or Queue's live Amount field scrambled its schedule. _field commits on the input event, once per keystroke, which was harmless when the handler was a plain setCount but is destructive now that it routes to SimEngine.setLiveCount: typing '12' committed 1 first, physically discarding the units and timers the other 11 stood for, then re-added 11 on a fresh full delay. Measured through the real UI: a batch of 12 due at step 5 arrived split as 1 at step 5 and 11 at step 8. _field takes a commitOn event now, and the amount row passes 'change' for a Delay or Queue so the edit applies once the user has finished typing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SH7ouoNymubArm7VEgALHg
Round-5 findings 3 and 4, which compound into one unrecoverable loss. Snapshots carry the module-level id counter, and loadJSON only ever raises it so that ids handed out since a snapshot cannot collide with it. Restoring a snapshot therefore never reproduces its own text, so _commit's 'did anything actually change?' test always reported a change once an undo had happened, and the next commit, even one that changed nothing at all, pushed an undo entry and cleared the redo stack. Snapshots are now compared with the counter excluded. Arrow-key nudges are coalesced for 400ms so a held key is a single undo step, but the commit was only ever armed on a timer. A Ctrl+Z inside that window stepped straight past the nudge and undid the edit before it: place two pools, nudge, Ctrl+Z, and the second pool disappears. The pending commit then landed and, through the bug above, wiped the redo stack, so it could not be brought back at all. Undo and redo now flush a pending nudge first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SH7ouoNymubArm7VEgALHg
…other leaks Round-5 findings 5, 6 and 7. Embed mode strips the editing chrome but leaves the canvas fully editable, and every edit reached _persistAutosave, which writes the same-origin sim_autosave key. A visitor who so much as dragged a node in someone's embedded diagram had their own saved diagram silently replaced by it, and found the embed's content waiting for them on their next visit with no undo stack left. _persistAutosave is now a no-op in embed mode. The body.embed rules hide the palette, properties panel, Setup rail and the file and analysis controls, but not the overflow menu button, which a max-width: 768px media query shows. Most iframes are narrower than that, so the menu handed back every control those rules had just removed, New diagram and Open file included. It is hidden in embed mode now. The 'open in Simulations' link stripped the embed marker only from the query string, so for the #embed hash form the knowledge base documents, the link pointed straight back at the embed. It now strips the marker from the hash too, including the #d=...&embed shape a shared embed actually has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SH7ouoNymubArm7VEgALHg
…s everything, and a lost baseline Round-5 findings 8, 11 and 12. _acceptable already refuses a trader as a flow target, on the grounds that a trader never holds resources, but the trader's own canAccept rejected only sources and registers. A trader used as another trader's partner therefore took the payment and kept it forever, invisibly: its canvas number, chart value and history entry all report the trade count rather than what it holds, so the units simply left the economy. Measured on the reported model, Gold fell from 100 to 80 over ten steps with the 20 sitting on the partner as a hidden holding that a mid-run save wrote into the file. A gate output weight of 0 means off, which the panel says in as many words and shows as 0%. Deterministic Split routed through _proportionalShares, whose zero-total fallback spreads the amount evenly. That is right for a delay handing a matured batch to its outputs, and the opposite of what a gate's weights mean: a gate with every weight at 0 emptied itself into every output at once, while Random and All correctly held everything. Split now holds too. run() captures the reset baseline at step 0 and the first doStep() captured it again, so anything that changed state between them was recorded as the diagram's authored starting amount. Clicking an interactive node in the gap before the first tick therefore rewrote its starting amount permanently: Reset returned to the post-click value and a save carried it. The baseline is now captured once per run, and an interactive fire at step 0 takes it first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SH7ouoNymubArm7VEgALHg
…rking Round-5 findings 9 and 10. An artificial-player rule keeps its target node id, but the node can be deleted or have its activation switched away from interactive. The dropdown then fell back to displaying the first interactive node in the diagram, so the panel claimed the rule was wired to a node it has nothing to do with while the rule silently never fired. It now shows '(node deleted)' and says the rule will not fire until another node is picked. Forking from a checkpoint did not leave history-scrub mode. Scrub paints a past step's values over the live model, so the canvas kept showing the replayed step's numbers under the checkpoint's step label, with the slider still pointing into a history the fork had just discarded. Measured: replaying step 2 of a run and forking a step-5 checkpoint left the canvas reading 10 under a 'Step 5' label. _forkFrom now exits scrub first and refreshes the scrubber. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SH7ouoNymubArm7VEgALHg
…names Round-5 findings 13, 14 and 15. Keyboard selection moves inside the SVG, so DOM focus never leaves the canvas and assistive technology was told nothing at all: tabbing through a diagram was completely silent, and the properties panel that repaints is neither focused nor a live region. Tab and Shift+Tab now write what they landed on to a visually hidden polite live region: node type, label, current value, and position in the reading order. The zoom readout's visible text is updated live but its accessible name was static markup, so it announced 'Current zoom. Click to reset to 100%' at every zoom level. It now carries the live percentage. The timeline's replay button swaps its icon and its tooltip when replay starts but its aria-label was static markup too, so it announced 'Replay the run' while it was the Pause control. The name now follows the tooltip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SH7ouoNymubArm7VEgALHg
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes all fifteen findings from the fifth multi-agent review, which covered six surfaces no previous round had touched (clipboard/groups, embed mode and share URLs, checkpoints and interactive nodes, numeric edge cases, accessibility) plus round 4's own diff.
Each fix has a test that I verified fails against the unpatched code first. 255 unit tests, smoke green.
Two of these are regressions from PR #64
Both were introduced by round-4 fixes merged yesterday, and no existing test caught either.
Monte Carlo progress stopped updating entirely. The async driver drains yields for a 14ms chunk then inspects the last one, and reported progress only when that last yield ended a trial. Once the generator began yielding after every step (the round-4 responsiveness fix), well under 1% of yields ended a trial, so a chunk almost never stopped on one: the dialog sat at "Running..." with a 0% bar for the whole batch. Measured against the pre-PR engine on the same model, 1000x300 gave 76 progress callbacks before and 0 after; restored to 85.
The smoke test I wrote for that change checked that Cancel appears and the run buttons disable. It never checked the counter still moved.
Typing in a Delay's Amount field scrambled its release schedule.
_fieldcommits on theinputevent, once per keystroke, which was harmless when the handler was a plainsetCountbut is destructive now that it routes toSimEngine.setLiveCount. Typing "12" committed 1 first, physically discarding the units and timers the other 11 stood for. Measured: a batch of 12 due at step 5 arrived split as 1 at step 5 and 11 at step 8.Findings fixed
_persistAutosaveis a no-op in embed modechange, not per keystrokeNotable detail
Findings 3 and 4 compound into unrecoverable loss. Place two nodes, nudge one with an arrow key, press Ctrl+Z inside the 400ms coalescing window: the second node disappears instead of the nudge being undone. The pending commit then lands and, through the id-counter bug, wipes the redo stack, so the node cannot be brought back at all.
Finding 5 is the worst by blast radius. Embed mode strips the editing chrome but leaves the canvas editable, and every edit reached the same-origin
sim_autosavekey. A visitor who so much as dragged a node in someone's embedded diagram had their own saved work silently replaced by it.Finding 11 was already documented as the intended rule.
_acceptablesays in a comment that a trader never holds resources and refuses it as a flow target. The trader's owncanAcceptrejected only sources and registers, so a trader used as another trader's partner took the payment and kept it invisibly:node cli.json the reported model printed10,80,10,0, with the missing 20 stored on a node whose displayed count, chart value and history entry all read 0.Measurements
[0,0,1,1,12,...]on main,[0,0,12,12,...]now, identical to no edit at all.10,80,10,0on main,10,100,0,0now.Testing
node test/run.js— 255 passed (was 251 on main)npm run smoke— passed, no console or page errorsOne note on the review itself
The refute stage returned its first two refutations in five rounds, both substantive. One of them varied a delay's output rate across 0, 1, 3, 5 and 100 and got identical output every time, refuting the finding's premise. That implies something the review did not file: a delay's sole-output rate appears to be inert at every value. Not addressed here.
Generated by Claude Code