You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
We have a Flutter native schematic render which we want to use now (leverages the ELK package standalone).
To do this we need to migrate our baseline Dart to 3.6.0 which brings in a few issues with how async is handled.
Related Issue(s)
Testing
Ran our dart test and a set of new confapp tests for the native application.
Backwards-compatibility
Is this a breaking change that will not be backwards-compatible? If yes, how so?
Yes. Dart 3.6 will require an upgrade for dependent packages.
We have Axi5 signal widths fixed to match protocol widths which may affect past usage.
Documentation
Does the change require any updates to documentation? If so, where? Are they included?
Minor updates. One major new update was tips on building good components.
The reason will be displayed to describe this comment to others. Learn more.
I'm comfortable with confapp evolving as ROHD gains the features needed for the full debugger, including keeping the planned SystemC support. I've included the confapp bugs found during review alongside the SystemC logo correction and copyright comments.
This PR must build and publish a working confapp site after merging to main. The base-URL and deployment-asset defects below must be fixed here, and the required CI gates must pass. The last-tab and narrow-window bugs may be fixed here or filed as separate follow-up issues.
This is an initial review; a deeper implementation pass is continuing. Reproductions used b501326 with Flutter 3.47.0. I checked the comment targets against current head 0305cbc; the relevant behavior is unchanged. Production deployment has not yet been verified.
The reason will be displayed to describe this comment to others. Learn more.
The previous base-URL, asset-generation, final-tab, narrow-window, logo, copyright, and container-bootstrap fixes are present. Both CI check jobs pass. Locally, the production builds, all 15 confapp tests, and the 11 modified reduction-tree tests pass; the built confapp renders native and Yosys schematics, and the documentation redirect renders its standalone schematic.
One baseline deployment blocker remains: the WASM build minifies the runtime type names used to find ROHD source assets, so the default component's source pane reports that no source is available despite the files being bundled. Please fix and verify that production-browser path before merging. Local testing used Flutter 3.47.0; CI uses 3.47.2, and actual deployment has not been run.
The final targeted checks also found a missing source mapping for the One-hot Converter's alternate direction and a component-switch-during-generation exception. The mapping gap belongs naturally in the source lookup fix; the generation race can be fixed here or tracked separately.
Modernizes Confapp, migrates schematic rendering to native ROHD netlists, adopts a Dart/Flutter workspace, and updates supporting tests, tooling, and documentation.
Changes:
Adds native schematic viewers, source navigation, theming, and browser integrations.
Upgrades Dart, Flutter, dependencies, CI, and workspace tooling.
Updates lint compliance, tests, documentation, and legacy schematic tooling.
The ROHD source asset tree is gitignored, but this standalone debug script never regenerates it. On a fresh checkout, flutter run therefore fails when resolving the assets/rohd_src/ entries. Run tool/generate_confapp_assets.sh before dependency resolution, as the CI build/test scripts already do.
Generate ROHD assets in standalone release script
tool/confapp_web_release.sh:29
The ROHD source asset tree is gitignored, but this standalone release script never regenerates it. On a fresh checkout, flutter run therefore fails when resolving the assets/rohd_src/ entries. Run the asset generator before dependency resolution.
Generate ROHD assets in standalone WASM script
tool/confapp_web_wasm.sh:29
The ROHD source asset tree is gitignored, but this standalone WASM script never regenerates it. On a fresh checkout, flutter run therefore fails when resolving the assets/rohd_src/ entries. Run the asset generator before dependency resolution.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Production context-menu handling conflicts with the tested lifecycle, while new viewer and cross-probing code remains unintegrated or insufficiently covered.
Global context-menu handler bypasses suppression lifecycle
confapp/web/index.html:48
This unconditional page-level handler permanently disables the native context menu everywhere, bypassing the new reference-counted BrowserContextMenuSuppression lifecycle. Consequently, the production page cannot reproduce the tested behavior where the menu is restored after detach(), and unrelated controls lose their browser menu. Remove this global listener and let browserContextMenuSuppressor own suppression while the custom menu is active.
Standalone viewer lacks coverage for parsing and load failures
doc/schematic_viewer/lib/main.dart:60
The new standalone viewer's URL parsing, network failure handling, and rendered-state transitions have no widget coverage, although the existing Flutter application has comprehensive widget tests for analogous page behavior. Add tests for a missing json parameter, a successful load, malformed JSON, and an HTTP failure before relying on this in documentation deployment.
Fix typo in test name: trival to trivial
test/arithmetic/dotproduct_test.dart:258
Correct the test name typo from “trival” to “trivial.”
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Stable serialization can be mutated after validation, publication exclusions can leak local dependency state, and documented fresh-checkout commands omit required asset generation.
Exclude generated dependency-state files from package archives
.pubignore:33
The new dependency-source helper creates root pubspec_overrides.yaml.disabled* backups and .confapp_dependency_sources, but these are not excluded here. Because .pubignore replaces .gitignore during publishing, local checkout paths and source selections can be included in the package archive. Exclude all newly generated root dependency-state files.
Copy choiceLabels to preserve validation invariants
choiceLabels is retained by reference even though this API promises stable serialized values. A caller can mutate the original map (or knob.choiceLabels) after construction, bypassing the uniqueness/type-label validation and changing previously persisted JSON values. Store an unmodifiable copy so the constructor’s invariants remain valid.
Document required asset generation before the Flutter build
confapp/DEVELOPER.md:82
This fresh-checkout run sequence omits the required asset-generation step. confapp/assets/rohd_src/ is gitignored while confapp/pubspec.yaml declares it, so following these commands directly can fail the Flutter build with missing assets. Generate the assets before entering confapp, as the CI wrappers do.
Update stale dart:html rationale for current browser implementation
confapp/DEVELOPER.md:112
This statement is stale: Confapp does not import dart:html; its browser implementation uses dart:js_interop/package:web, and this document now advertises an experimental Wasm command above. As written, the rationale contradicts both the code and the documented target support.
The reason will be displayed to describe this comment to others. Learn more.
The previous Type-label documentation request is addressed, and both required CI jobs are green. Production builds and normal-path browser smoke checks passed at abe3721baad18791975aef0a594b0b15749329ba; actual GitHub Pages publication was not exercised.
The remaining feedback covers async Yosys failure handling (a follow-up in the existing worker-error thread), preserving wide arithmetic regression coverage, and bumping Confapp to 0.1.0 while retaining publish_to: none.
The reason will be displayed to describe this comment to others. Learn more.
Code review complete at 4b71a13eafec351967b175a445e183daf42e2762. The previous merge-blocking feedback is addressed, including the Confapp version bump and restored wide randomized arithmetic coverage. No new merge blocker was found in the final check.
The known async Yosys failure remains an explicit, non-blocking release follow-up.
Both required CI jobs are still running. Please wait for them to pass before merging.
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
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.
mkorbel1
left a comment
Description & Motivation
We have a Flutter native schematic render which we want to use now (leverages the ELK package standalone).
To do this we need to migrate our baseline Dart to 3.6.0 which brings in a few issues with how async is handled.
Related Issue(s)
Testing
Ran our dart test and a set of new confapp tests for the native application.
Backwards-compatibility
Yes. Dart 3.6 will require an upgrade for dependent packages.
We have Axi5 signal widths fixed to match protocol widths which may affect past usage.
Documentation
Minor updates. One major new update was tips on building good components.