Audit fixes - #326
Merged
Merged
Conversation
P0 - data loss in shipping features:
- Snapshots now store the diagram document verbatim (no lossy Graph
round-trip); restore preserves block sizes/shapes, child diagrams,
annotations, waypoints, and connections. Legacy Graph-format
snapshots are flattened to the JS connection format on restore.
- Multi-page diagrams always full-save: the delta patch only covered
the active page, leaving a stale pages array that reverted edits on
reload. Page operations now register as unsaved changes.
- Snapshot store tracks its document scope and refuses cross-scope
persistence, so one document's history can no longer overwrite
another's (save_named_diagram previously wrote both scopes).
P1 - correctness:
- Physical properties: Fusion reports mass in kg and volume in cm3
already; removed the /1000 conversions (values were 1000x off).
selection.py also read a nonexistent props.boundingBox; it now uses
occurrence.boundingBox.
- Navigate Up persists child-diagram edits via the parentBlockId
recorded on the hierarchy stack entry; Python create_child_diagram
stamps metadata.parentBlockId.
- Delta diffs: Python id-keyed list diff now mirrors the JS strategy
(whole-list replace on membership/order change) instead of emitting
modify ops at new indices against old order - reorder+modify used to
corrupt data. JS index-based diff now emits removals in reverse
order so multi-element shrinks apply cleanly.
- Save As resolves slug collisions ("My Design!" vs "My Design?") with
a numeric suffix instead of silently overwriting.
P2 - robustness and quality:
- Diagnostics no longer create/delete geometry in the user's document.
- Component sync restores the originally active document after the
activate-to-resolve fallback.
- Rule checks: mA->mW conversion honors declared rail voltage;
results carry block/connection ids so click-to-highlight works;
bulk/singular severities aligned.
- ValidationError gained a severity field; cycles and empty names are
warnings (feedback loops are legitimate).
- SVG export honors the canonical "kind" key; CSV import tolerates
empty/malformed coordinates.
- Workspace-activation handler is unregistered on stop(); sys.path
insert guarded against reload duplication.
- Snapshot store trims oldest entries to a 1.5 MB budget instead of
letting oversized attribute writes lose the whole history.
- Removed 344 lines of unreachable Milestone-13 entry-point functions.
- Assembly-time type multipliers apply (lowercase lookup); BOM and
assembly generators no longer mutate the input diagram.
- block_fingerprint includes rotation.
- docs/schema.json aligned with the real document format (metadata,
groups, namedStubs, annotations, pages at root; child diagrams are
full diagrams).
Adds 56 regression tests; full suite is now 763 passing (hypothesis
and jsonschema extras required). Diagnostics count 32 -> 30.
Co-Authored-By: Claude Fable 5 <[email protected]>
Schema/versioning consistency: - Standardize the document-format identifier on "system-blocks-v2" across the Python core, fsb_core Graph model, and the JS editor (schemaVersion "1.0" remains the sole migration key). Drop the vestigial metadata.version "2.0" marker from new JS diagrams and child diagrams. - Wire diagram_data.migrate_diagram into the bridge load and snapshot-restore paths so Python-side loads are migration-safe regardless of the JS-side migration. Dependency alignment: - Move jsonschema from hard dependencies to the dev/test extras: the add-in runs inside Fusion's bundled Python where nothing can be pip-installed, and the code already treats it as optional. CI's test job installs via .[test], which now carries it. - Document the test setup (pip install -e .[test]) in the README. Polish: - select_multiple_occurrences no longer logs an error on the normal ESC-to-finish gesture. - Component thumbnail placeholders only ellipsize names that were actually truncated. - When the palette is unavailable, only warnings/errors fall back to a blocking message box; info/success toasts are logged instead. - File log level defaults to INFO, overridable via the SYSTEM_BLOCKS_LOG_LEVEL environment variable (set "debug" when collecting logs for a bug report). - Ignore runtime logs/ and exports/ directories. Adds 12 regression tests; full suite is now 775 passing. Co-Authored-By: Claude Fable 5 <[email protected]>
Restructure around what a Fusion user needs: plain-language feature overview, hand-holding install steps with expectations at each step, a 2-minute first-diagram walkthrough, task-based recipes for everyday features, and a troubleshooting section (including the two-saves nuance and log file locations). Developer material (test setup, baselines, code layout, debug logging) moves to a clearly separated section at the end. Instructions verified against the UI code: connection points appear after entering Connect mode (not on hover), and the unwired component Sync claim was removed. Co-Authored-By: Claude Fable 5 <[email protected]>
1. Zombie pages after deleting down to one page: the delta-save guard only forced full saves while more than one page existed, but the patched exportDiagram omits the `pages` key at exactly one page. Deleting down to a single page and then delta-saving left the stale multi-page `pages` array in the stored document - reload resurrected the deleted page and reverted the post-delete edits. Page operations now set a _pagesNeedFullSave flag that forces full saves until a successful full/named save (or a fresh load) confirms the stored document matches the live page state. 2. deletePage never captured the active page's live state before reloading it, so deleting a NON-active page silently reverted any edits made on the active page since the last page switch. It now calls _saveCurrentPageState() first, matching addPage and _duplicatePage. Both found in the post-merge review pass; JS multi-page flow has no automated harness, so these were verified by control-flow trace. Co-Authored-By: Claude Fable 5 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR consolidates a set of “audit” hardening fixes across the Fusion add-in bridge, JS editor, and Python core—focused on making persistence/versioning safer, preventing silent data loss, and aligning document/schema behaviors across layers.
Changes:
- Strengthen persistence + version control: verbatim document snapshots (no Graph round-trip), snapshot-store size trimming, scope-guarded snapshot persistence, and collision-safe named-document slugging.
- Fix editor/save correctness edge cases: force full saves when multi-page state can’t be captured by deltas; mark/clear page-level “dirty” state; ensure child-diagram Navigate Up persistence is reliable.
- Improve data correctness and diagnostics: voltage-aware power budget conversion, consistent rule result highlighting IDs + severity, safer exports/imports, and logging/notification behavior adjustments.
Reviewed changes
Copilot reviewed 40 out of 41 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_version_control.py | Adds coverage for verbatim document snapshots + persistence/compare/trim behaviors. |
| tests/test_snapshot_scope.py | Pins snapshot-store scope guards to prevent cross-document history overwrite. |
| tests/test_serialization.py | Tests connection flattening from Graph format into JS import contract. |
| tests/test_selection.py | Updates selection tests for Fusion units + bounding box source; adds ESC finish regression. |
| tests/test_rule_checks.py | Adds rule-check regressions for rail voltage and highlight IDs + bulk severity. |
| tests/test_requirements.py | Ensures rotation affects block fingerprinting for snapshot diffs. |
| tests/test_notifications.py | Adds coverage for palette-not-available notification fallback behavior. |
| tests/test_named_documents.py | Adds regression tests for slug collision avoidance and Save As semantics. |
| tests/test_logging_util.py | Adds tests for env-var-controlled logging level resolution. |
| tests/test_import.py | Adds CSV coordinate parsing tolerance tests. |
| tests/test_hierarchy.py | Tests that child diagrams record parentBlockId metadata. |
| tests/test_export_reports.py | Tests SVG styling key handling and “report generation must not mutate input”. |
| tests/test_diagram_data.py | Updates expected canonical schema identifier to system-blocks-v2. |
| tests/test_delta.py | Adds/updates delta patch regressions for id-keyed and index-keyed list safety. |
| tests/test_core_validation.py | Adds severity-level tests (warnings vs errors) for validation findings. |
| tests/test_cad.py | Adds tests for type multipliers + thumbnail ellipsis behavior. |
| src/utils/delta-utils.js | Fixes index-based list removals by emitting reverse-order removes. |
| src/ui/toolbar-manager.js | Makes child-diagram parentBlockId authoritative and logs persistence failures. |
| src/main-coordinator.js | Tracks page-level dirty state and forces full save when needed; fixes delete-page state capture. |
| src/interface/python-bridge.js | Skips delta-save when pages require a full save; clears page-dirty on full/named save. |
| src/diagram/rules.py | Adds rail-voltage parsing and highlight IDs; adjusts logic-level bulk severity; enriches power-budget/implementation results. |
| src/diagram/hierarchy.py | Records metadata.parentBlockId when creating child diagrams in Python. |
| src/diagram/export.py | Uses canonical connection kind/type for styling; hardens CSV coord parsing. |
| src/diagram/core.py | Standardizes schema to system-blocks-v2; clarifies schema vs schemaVersion roles. |
| src/diagram/cad.py | Fixes thumbnail ellipsis logic; avoids diagram mutation in BOM/assembly generation; normalizes type lookup. |
| src/core/diagram-editor.js | Standardizes schema to system-blocks-v2; removes unused metadata.version; improves unsaved-change detection for page ops. |
| README.md | Reworks user-facing docs and adds developer instructions (tests, logging env var). |
| pyproject.toml | Removes hard runtime deps (Fusion bundled Python); moves jsonschema to optional deps. |
| Fusion_System_Blocks.py | Adds snapshot-store scope guard + size trimming; verbatim snapshots; named-doc slug collision avoidance; notification fallback changes; event handler cleanup; schema migration on load. |
| fusion_addin/selection.py | Treats ESC as finish gesture; fixes Fusion physical units assumptions and boundingBox source. |
| fusion_addin/logging_util.py | Adds env-var log level override with default INFO. |
| fusion_addin/diagnostics.py | Removes invasive diagnostics that modified user documents; documents new non-invasive approach. |
| fsb_core/version_control.py | Adds verbatim dict snapshots + restore_document; adds trim_to_json_size; refactors append/prune. |
| fsb_core/validation.py | Adds severity field to ValidationError and marks cycles/empty names as warnings. |
| fsb_core/serialization.py | Adds flatten_connections_for_js to normalize legacy/nested connections for JS editor. |
| fsb_core/models.py | Includes rotation in block fingerprinting. |
| fsb_core/delta.py | Makes id-keyed list diffs safe by replacing whole list on reorder/membership changes. |
| docs/schema.json | Expands schema to recognize additional top-level keys (pages, annotations, etc.) and relaxes childDiagram shape. |
| docs/FUSION_MANUAL_TEST_PLAN.md | Updates automated baseline test count. |
| docs/DETAILED_TESTING_DOCUMENTATION.md | Updates automated baseline test count. |
| .gitignore | Ignores runtime-generated logs/exports folders. |
Comment on lines
+570
to
+575
| suffix = 2 | ||
| while True: | ||
| candidate = f"{base[:60]}_{suffix}" | ||
| if candidate not in taken or taken[candidate] == label: | ||
| return candidate | ||
| suffix += 1 |
Comment on lines
+161
to
+172
| try: | ||
| selection = self._ui.selectEntity( | ||
| f"{prompt} ({len(selections)} selected, ESC to finish)", | ||
| "Occurrences", | ||
| ) | ||
| except Exception: | ||
| if _LOGGER is not None: | ||
| _LOGGER.debug( | ||
| "select_multiple_occurrences finished with %d selection(s)", | ||
| len(selections), | ||
| ) | ||
| break |
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
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.
No description provided.