Fix/devtools popup menu material UI - #702
Conversation
9ccd646 to
5fce04c
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5fce04c to
4821521
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The deployed web bundle uses an incorrect base path, and extension availability reporting is unreliable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Migrates DevTools UI code to material_ui, adds embedded trace support, and prepares the standalone web application for GitHub Pages deployment.
Changes:
- Updates Material UI imports, lint compliance, and related tests.
- Adds embedded netlist trace parsing and shared extension helpers.
- Adds standalone web build and documentation deployment integration.
File summaries
| File | Description |
|---|---|
rohd_extension/src/signal_viewer_registry.ts |
Isolates signal delivery failures. |
rohd_extension/dart/lib/flc_data.dart |
Adds embedded netlist trace parsing. |
rohd_extension/dart/lib/dtd_service.dart |
Applies lint formatting and logging clarification. |
rohd_devtools_extension/tool/test_devtools_install.dart |
Checks the binary asset manifest. |
rohd_devtools_extension/tool/gh_actions/build_app.sh |
Builds and validates the standalone web bundle. |
rohd_devtools_extension/test/view/rohd_devtools_page_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/ui/vm_connection_form_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/ui/signal_table_text_field_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/ui/signal_table_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/ui/signal_details_card_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/ui/presentation_widgets_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/ui/module_tree_details_navbar_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/ui/diagnostic_properties_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/ui/devtools_connection_host_lifecycle_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/ui/devtool_appbar_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/modules/tree_structure/tree_structure_page_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/modules/tree_structure/model_tree_card_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/test/const/app_theme_test.dart |
Migrates test Material imports. |
rohd_devtools_extension/README.md |
Documents standalone web builds. |
rohd_devtools_extension/pubspec.yaml |
Adds the Material UI dependency. |
packages/rohd_devtools_widgets/test/signal_value_format_registry_test.dart |
Uses the root occurrence constant. |
packages/rohd_devtools_widgets/test/rohd_extension_status_test.dart |
Reformats status assertions. |
packages/rohd_devtools_widgets/test/markdown_help_button_test.dart |
Migrates imports and formatting. |
packages/rohd_devtools_widgets/test/logic_type_utils_test.dart |
Reformats logic utility tests. |
packages/rohd_devtools_widgets/test/export_toast_test.dart |
Migrates imports and formatting. |
packages/rohd_devtools_widgets/test/export_button_test.dart |
Migrates imports and formatting. |
packages/rohd_devtools_widgets/test/cross_probe_service_test.dart |
Reformats immutability assertion. |
packages/rohd_devtools_widgets/test/cross_probe_menu_test.dart |
Updates navigation availability expectations. |
packages/rohd_devtools_widgets/test/cross_probe_button_test.dart |
Migrates imports and formatting. |
packages/rohd_devtools_widgets/test/capture_boundary_test.dart |
Migrates imports and null handling. |
packages/rohd_devtools_widgets/test/bit_field_utils_test.dart |
Migrates imports and formatting. |
packages/rohd_devtools_widgets/test/bit_expansion_menu_test.dart |
Migrates imports and null assertions. |
packages/rohd_devtools_widgets/test/app_bar_overlay_test.dart |
Migrates imports and formatting. |
packages/rohd_devtools_widgets/pubspec.yaml |
Raises SDK requirements and adds assets/dependency. |
packages/rohd_devtools_widgets/lib/src/signal_value_format_registry.dart |
Applies lint refactoring. |
packages/rohd_devtools_widgets/lib/src/save_png_web.dart |
Cascades anchor operations. |
packages/rohd_devtools_widgets/lib/src/rohd_extension_status.dart |
Adds constructor documentation and formatting. |
packages/rohd_devtools_widgets/lib/src/rohd_extension_client.dart |
Uses a package-qualified import. |
packages/rohd_devtools_widgets/lib/src/markdown_help_button.dart |
Migrates UI imports and async handling. |
packages/rohd_devtools_widgets/lib/src/logic_type_utils.dart |
Reformats chained logic operations. |
packages/rohd_devtools_widgets/lib/src/export_toast.dart |
Migrates Material imports. |
packages/rohd_devtools_widgets/lib/src/export_button.dart |
Adds diagnostics and documentation. |
packages/rohd_devtools_widgets/lib/src/cross_probe_service.dart |
Expands control flow and documentation. |
packages/rohd_devtools_widgets/lib/src/cross_probe_menu.dart |
Changes confirmed-format menu behavior. |
packages/rohd_devtools_widgets/lib/src/cross_probe_button.dart |
Adds diagnostics and import migration. |
packages/rohd_devtools_widgets/lib/src/capture_boundary.dart |
Migrates Material imports. |
packages/rohd_devtools_widgets/lib/src/bit_field_utils.dart |
Refactors dialogs and parser formatting. |
packages/rohd_devtools_widgets/lib/src/bit_expansion_menu.dart |
Refactors popup menu helpers. |
packages/rohd_devtools_widgets/lib/src/app_bar_overlay.dart |
Adds diagnostics and import migration. |
packages/rohd_devtools_widgets/lib/rohd_devtools_widgets.dart |
Reorganizes public exports. |
packages/rohd_devtools_widgets/assets/extension/module_info_helper.js |
Adds shared extension query helpers. |
packages/rohd_devtools_widgets/analysis_options.yaml |
Inherits project analysis configuration. |
rohd_devtools_extension/Makefile |
Adds web build and cleanup targets. |
lib/rohd_devtools/view/tree_structure_page.dart |
Migrates Material imports. |
lib/rohd_devtools/view/rohd_devtools_page.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/vm_connection_form.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/standalone_app_shell.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/signal_table.dart |
Migrates imports and updates copyright. |
lib/rohd_devtools/ui/signal_table_text_field.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/signal_details_card.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/schematic_icon.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/platform_icon.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/module_tree_details_navbar.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/module_tree_card.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/devtools_help_button.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/devtools_connection_host.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/devtool_appbar.dart |
Migrates Material imports. |
lib/rohd_devtools/ui/details_help_button.dart |
Migrates Material imports. |
lib/rohd_devtools/const/app_theme.dart |
Migrates Material imports. |
lib/main.dart |
Migrates the extension entry point. |
lib/main_standalone.dart |
Migrates the standalone entry point. |
rohd_devtools_extension/analysis_options.yaml |
Excludes generated platform directories. |
analysis_options.yaml |
Expands root analyzer exclusions. |
.github/workflows/general.yml |
Builds and deploys the standalone artifact. |
.github/workflows/build_devtool.yml |
Updates the Flutter SDK version. |
Review details
- Files reviewed: 75/75 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Embedded trace indexing can misresolve malformed file tables, and deployment documentation contains an incorrect base path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 76/76 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Mixed source-frame ordering and invalid embedded line handling can produce incorrect navigation locations.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
rohd_devtools_extension/packages/rohd_devtools_widgets/assets/extension/module_info_helper.js:178
- This reverses every returned frame, but
rohd.lookupSignalappends output-language positions in their declared order after the innermost-first ROHD stack. As a result, selectingsvreverses multiple SV locations, and an unfiltered lookup also reverses those output locations. Reverse only the ROHD stack while preserving output-frame order.
rohd_extension/dart/lib/flc_data.dart:334 FlcFrame.lineis documented as 1-based, but this validation accepts zero and negative line numbers. Those malformed frames are then treated as line 1 by the navigation layer rather than being skipped, producing an incorrect source location. Reject non-positive lines alongside invalid file indexes.
rohd_extension/dart/lib/flc_data.dart:297- This documentation says the factory parses one separated attribute, but the implementation expects a complete module table whose values still contain
attributes.rohd.src_trace, and it parses every module. Documenting the actual input shape is important because passing separated trace maps as described silently returns empty data.
- Files reviewed: 76/76 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Both web artifact checks use the wrong Flutter manifest filename and will fail valid builds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 76/76 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The embedded trace API accepts invalid source positions and contradicts its documented input contract, while the guide recommends a testing-only API.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
rohd_extension/dart/lib/flc_data.dart:298
- This factory does not accept separated attributes as its name and documentation claim: it forwards
modulestofromNetlistJson, which still requires each value to contain anattributeswrapper. A caller passing a module'srohd.src_traceattributes gets empty data. Either change the API to accept the module name plus attribute/trace map, or rename and document it as accepting the completemodulesmap.
rohd_extension/dart/lib/flc_data.dart:339 FlcFrame.lineand.columnare documented as 1-based, but this parser accepts zero or negative numeric values (for example0:0:-1) and stores them unchanged. Validateline >= 1and any parsed column>= 1before creating the frame so malformed embedded traces cannot produce invalid editor locations.
- Files reviewed: 77/77 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Embedded trace validation and module-query failure reporting contain correctness issues, and one documented example fails on a fresh checkout.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
rohd_devtools_extension/packages/rohd_devtools_widgets/assets/extension/module_info_helper.js:151
extensionAvailablewas already determined byresolveFlcPathWithAvailability, but any later query failure resets it tofalseand tells users to install the extension. An installed extension with a malformed FLC or query error is therefore misreported and clients hide all source actions; preserve the known availability and report the actual query failure.
rohd_extension/dart/lib/flc_data.dart:339FlcFramedocuments 1-based coordinates, but zero/negative lines and zero/negative parsed columns pass through here. Such malformed traces become apparently valid lookup results and navigate to an incorrect location; reject non-positive lines and normalize non-positive columns just like an unparsable column.
doc/user_guide/_docs/A21-generation.md:51- This example fails on a fresh checkout because
File.writeAsStringdoes not create the missingbuild/parent directory. The equivalent root README example creates the parent first; do the same here so the documented command is runnable.
- Files reviewed: 78/78 changed files
- Comments generated: 0 new
- Review effort level: Balanced
mkorbel1
left a comment
There was a problem hiding this comment.
Looks good to me. The UI migration, source-navigation support, and combined documentation/app deployment look reasonable. Relying on your functional testing and the passing Run Checks and Build Devtools checks; I have not rerun the UI tests or builds independently. One non-blocking nit on root-hosted base-path validation, which does not affect the configured ROHD deployment.
…com/desmonddak/rohd into no-logic-array/01-fix-devtools-popup-menu-material-ui
There was a problem hiding this comment.
🟡 Changes recommended
Trace validation and shared helper errors can produce invalid navigation results and misleading extension status.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
rohd_devtools_extension/packages/rohd_devtools_widgets/assets/extension/module_info_helper.js:178
- Reversing the entire response also reverses output-language positions and, for an unfiltered lookup, moves them ahead of the ROHD stack.
flc_service.ts:392-416returns the ROHD stack first (innermost-first) and appends output positions in declared order; reverse only the ROHD subset so SV/SystemC alternatives retain their intended order.
const filteredFrames = format
? typedFrames.filter(frame => (frame?.type ?? 'rohd') === format)
: typedFrames;
// Reverse to outermost-first order (the ROHD extension returns
// innermost-first, matching raw stack-trace order).
return filteredFrames.reverse();
- Files reviewed: 77/77 changed files
- Comments generated: 4
- Review effort level: Balanced
Description & Motivation
We are migrating devtools to use the latest analysis options and discovered some deprecated symbols.
Also, we found a Javascript file was needed in the shared widget support area for future widgets (module_info.js)
Importantly, we amended this PR to deploy the web app so that it is invokeable from our git page.
The assembled deployment tree becomes:
The existing deployment action publishes all of
doc/api/to thedocsbranch:Consequently, the nested build directory becomes a top-level URL beneath the ROHD Pages site:
The key point is that documentation and the compiled web app are deployed together in one operation. This prevents one deployment from erasing the other.
Related Issue(s)
None
Testing
Ran existing tests.
Backwards-compatibility
No
Documentation
None needed. This is lint cleanup, moving away from deprecated Flutter classes, and centralizing a resource file.