MILAB-5607: use graph-maker plugin - #35
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the block's internal architecture by migrating to a new data model ( Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the block to use the graph-maker plugin with BlockModelV3, which is a significant and positive architectural change. The UI code has been updated correctly to align with this new plugin-based approach. However, I've found a critical issue in the model definition: the plugins are configured to use an output named pf which is no longer defined in the model. This will prevent the block from functioning. Once this is addressed, the PR should be in good shape.
| .plugin(vUsagePlugin, { | ||
| blockColumns: (ctx) => ctx.outputs?.resolve('pf')?.getPColumns(), | ||
| }) |
There was a problem hiding this comment.
The graph-maker plugin is configured to get its blockColumns from an output named pf. However, the pf output, which was present in the previous model version, has been removed in this refactoring. As a result, ctx.outputs?.resolve('pf') will be undefined, and the block will fail to get data for the graphs.
You need to ensure the data is provided to the plugins. This could be done by:
- Re-introducing an output that provides the
PColumn[]for the graphs. Thegraph-makerplugin might even offer a utility function for this. - Revisiting the plugin configuration if
blockColumnsis not the intended way to pipe data in this new setup.
| if (isRunning && !wasRunning) { | ||
| // Close the settings tab by setting currentTab to null | ||
| app.model.ui.vUsagePlotState.currentTab = null; | ||
| (vUsagePlugin.model.data.state as { currentTab: null }).currentTab = null; |
There was a problem hiding this comment.
I want to move this inside or remove, or add a setting. I'll ask Maria.
| if (isRunning && !wasRunning) { | ||
| // Close the settings tab by setting currentTab to null | ||
| app.model.ui.vUsagePlotState.currentTab = null; | ||
| (vUsagePlugin.model.data.state as { currentTab: null }).currentTab = null; |
There was a problem hiding this comment.
we need to discuss this
2 participants
No description provided.