Use chart data from bundle (after chart-transform plugin effects) - #12
Use chart data from bundle (after chart-transform plugin effects)#12jphinspace wants to merge 2 commits into
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughTab View now builds alphaTab scores directly from renderer bundles instead of server-side GP5 conversion. New quantization and score-builder modules support timing, tuning, notation, techniques, and rests, while ChangesBundle-driven alphaTab rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RendererBundle
participant screenjs
participant ScoreBuilder
participant AlphaTabApi
participant Highway
RendererBundle->>screenjs: provide chart bundle and current time
screenjs->>ScoreBuilder: buildScoreFromBundle(bundle)
ScoreBuilder-->>screenjs: return alphaTab Score
screenjs->>AlphaTabApi: render score
AlphaTabApi-->>screenjs: emit render completion
screenjs->>Highway: synchronize marker and visibility
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin.json`:
- Around line 4-8: Update the plugin manifest and screen.js host-loading
integration so screen.js is loaded according to the host’s classic-script
contract rather than relying on scriptType: "module". Remove or avoid the static
import in screen.js, and use a compatible loading pattern that works when the
host ignores the manifest’s module setting.
In `@screen.js`:
- Around line 838-858: Move the bundle.notes identity check from draw() into
_tvCursorLoop so single-player mode detects chart updates while highway
rendering is hidden. Read the current bundle/notes from window.highway using the
same guard pattern as the existing getBeats() check, then reuse the existing
_tvCurrentNotesRef, _tvPendingNotesRef, _tvFailedNotesRef, mount, and token
guards before calling _tvRenderFromBundle.
In `@src/chart-quantize.js`:
- Around line 166-175: Update the tuning-table construction around the tunings
calculation to validate each selected tuningOffsets value and use 0 whenever it
is non-finite, while preserving finite offsets unchanged. Ensure the resulting
tunings array always contains numeric values before it is returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da3c4667-6ac4-4fa4-a828-d86489bb765f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.gitignore.specify/memory/constitution.mdREADME.mdpackage.jsonplugin.jsonrequirements.txtroutes.pyrs2gp.pyscreen.jssrc/chart-quantize.jssrc/score-builder.jstest/chart-quantize.test.mjstest/score-builder.test.mjstests/conftest.pytests/test_rs2gp_helpers.py
💤 Files with no reviewable changes (5)
- tests/test_rs2gp_helpers.py
- rs2gp.py
- requirements.txt
- tests/conftest.py
- routes.py
| "version": "4.0.0", | ||
| "type": "visualization", | ||
| "private": false, | ||
| "script": "screen.js", | ||
| "routes": "routes.py", | ||
| "scriptType": "module", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Any leftover Python / server-route artifacts from the removed GP5 route?
fd -e py -e pyi . || true
rg -n --hidden -g '!node_modules' -e 'routes' -e 'pyguitarpro' -e 'guitarpro' -e 'gp5' -i . || true
# Confirm the manifest keys the host is expected to read
fd -t f 'plugin.json' --exec cat {}Repository: got-feedBack/feedBack-plugin-tabview
Length of output: 1346
🌐 Web query:
Grafana plugin manifest scriptType module scriptType classic visualization plugin manifest documentation
💡 Result:
In Grafana plugin development, the terms "module" and "classic" do not refer to a specific scriptType field in the plugin.json manifest. Instead, they relate to how plugins are loaded and structured [1][2]. The plugin.json file is the required metadata manifest for all Grafana plugins [3][4]. While it does not contain a scriptType field, it does define a loadingStrategy (found in internal plugin metadata) that dictates how frontend assets are loaded [5][1]. Key Concepts: 1. Loading Strategies: Grafana defines a PluginLoadingStrategy which can be set to either fetch or script [1]. This strategy signals to the underlying system (e.g., SystemJS) how to load the frontend JavaScript assets [1]. 2. Module vs. Classic (Context): - The term "module" typically refers to the modern standard for Grafana plugins, where module.ts (compiled to module.js) serves as the entry point [4][2]. This file exports the plugin implementation (such as a class extending PanelPlugin) [4][6]. - "Classic" often refers to older versions or legacy plugin architectures (e.g., those using PanelCtrl or older AngularJS-based approaches) [7]. Modern development relies on the current plugin SDK and standard ESM/module-based loading [4][2]. 3. Plugin Manifest (plugin.json): - This file is mandatory for all plugins [3]. It contains essential metadata such as the plugin id, name, and type (e.g., panel, datasource, app) [3][4]. It does not configure the loading script type explicitly via a scriptType manifest key [3][8]. For current development, you should follow the official Grafana Plugin Tools documentation, which focuses on modern React-based plugins using the standard module.ts entry point [4][2]. There is no requirement to configure a "script type" in the manifest, as the build process handles the necessary module format [4].
Citations:
- 1: https://github.com/grafana/grafana/blob/main/packages/grafana-data/src/types/plugin.ts
- 2: https://grafana.com/developers/plugin-tools/key-concepts/anatomy-of-a-plugin/
- 3: https://grafana.com/developers/plugin-tools/reference/plugin-json
- 4: https://grafana.com/developers/plugin-tools/tutorials/build-a-panel-plugin
- 5: https://github.com/grafana/grafana/blob/cd174d5c/apps/plugins/kinds/meta.cue
- 6: https://grafana.com/developers/plugin-tools/tutorials/build-an-app-plugin
- 7: https://grafana.com/docs/grafana/v5.2/plugins/developing/development/
- 8: https://github.com/grafana/grafana/blob/48a6b2d9/apps/plugins/kinds/meta.cue
🌐 Web query:
site:grafana.com developers plugin-tools reference plugin.json scriptType
💡 Result:
Based on the official Grafana Plugin Tools documentation, there is no field named scriptType in the plugin.json file [1][2][3]. The plugin.json file is a mandatory metadata file used to define plugin configuration, such as its id, type (e.g., "app", "datasource", "panel", "renderer"), name, and various dependencies or includes [1][2][3]. If you are looking for information on how to configure build scripts or tooling configurations, those are typically managed within package.json (for npm scripts) or via configuration files in the project root, such as webpack.config.ts or files within the.config/ directory [4][5]. If you are encountering a reference to scriptType, it may be from an outdated version of the Grafana plugin framework, a third-party tool, or a misunderstanding of a different configuration property. For current development, please refer to the official plugin.json reference guide provided by Grafana [1].
Citations:
- 1: https://grafana.com/developers/plugin-tools/reference/plugin-json
- 2: https://grafana.com/docs/grafana/v6.4/plugins/developing/plugin.json/
- 3: https://grafana.com/developers/plugin-tools/tutorials/build-an-app-plugin
- 4: https://grafana.com/developers/plugin-tools/how-to-guides/extend-configurations
- 5: https://grafana.com/developers/plugin-tools/
🌐 Web query:
grafana visualization plugin script scriptType classic module plugin.json
💡 Result:
In Grafana plugin development, the plugin.json file serves as the mandatory metadata configuration for all plugins [1][2][3]. While the specific terms "classic" or "module" do not define distinct plugin types in the current Grafana plugin schema, they reflect the evolution of how plugins are structured, loaded, and built [2][4][5]. Plugin Structure and Loading The module field in plugin.json specifies the entry point for the plugin's frontend code, typically pointing to module.js (compiled from src/module.ts) [2][4][5]. Modern Grafana plugins use a build process that outputs this module file, which Grafana loads to initialize the plugin [2][6]. Evolution of Plugin Loading Grafana internally tracks how plugins are loaded via a loadingStrategy [4]. This strategy dictates how frontend assets are handled: 1. module: In modern development, plugins are built as modules that Grafana loads as a bundle [2][5]. 2. script: Historically, or in specific integration scenarios, plugins might have relied on different loading mechanisms, sometimes referred to as "classic" (often associated with older Angular-based plugins or manual script inclusion) [4][7]. Current Standards The term "classic" is generally deprecated in modern Grafana plugin documentation, which now emphasizes using the create-plugin tool to scaffold plugins [6][3]. This tool generates a standard configuration that expects a modern module-based structure [5][6]. If you are working with an older plugin, you may encounter references to "classic" loading styles, but current official documentation focuses on the standard module-based approach where the plugin.json defines the entry point and the build system handles the module creation [2][5]. For definitive schema information, refer to the official plugin.json schema, which defines the expected structure for current plugin development [1][2].
Citations:
- 1: https://grafana.com/developers/plugin-tools/reference/plugin-json
- 2: https://grafana.com/developers/plugin-tools/tutorials/build-a-panel-plugin
- 3: https://grafana.com/developers/plugin-tools/tutorials/build-an-app-plugin
- 4: https://github.com/grafana/grafana/blob/main/packages/grafana-data/src/types/plugin.ts
- 5: https://grafana.com/developers/plugin-tools/key-concepts/anatomy-of-a-plugin/
- 6: https://grafana.com/developers/plugin-tools/
- 7: https://grafana.com/docs/grafana/v6.4/plugins/developing/development/
🌐 Web query:
"scriptType: \"module\"" "plugin.json" "script" visualization grafana
💡 Result:
In the context of Grafana plugin development, there is no standardized or required scriptType: "module" field within the plugin.json file [1][2][3]. The plugin.json file is primarily used to store essential plugin metadata such as its unique id, type, name, version, and dependencies [1][4][5]. The frontend entry point for a Grafana plugin is defined by the module file, typically named module.ts (or module.js after compilation) [6][4][7]. Grafana automatically discovers and loads this file when the plugin is initialized [4][7]. While some internal Grafana structures, such as the PluginMeta interface found in @grafana/data, contain a loadingStrategy field with options like PluginLoadingStrategy.fetch or PluginLoadingStrategy.script [8], this configuration is generally handled by the Grafana plugin framework and build tools (such as @grafana/toolkit or the newer create-plugin tool) rather than manual entry in the plugin.json file [5][9]. If you are encountering a scriptType: "module" reference, it may be related to custom build configurations, specific SystemJS setups, or an experimental feature not included in the core plugin metadata schema [1][2]. For standard plugin development, follow the official Grafana Plugin Tools documentation to ensure your plugin.json and module.ts files are correctly structured [1][7][10].
Citations:
- 1: https://grafana.com/developers/plugin-tools/reference/plugin-json
- 2: https://grafana.com/docs/grafana/v6.4/plugins/developing/plugin.json/
- 3: https://grafana.com/docs/grafana/v6.5/plugins/developing/plugin-review-guidelines/
- 4: https://github.com/grafana/tutorials/blob/master/content/tutorials/shared/plugin-anatomy.md
- 5: https://grafana.com/developers/plugin-tools/key-concepts/anatomy-of-a-plugin/
- 6: https://grafana.com/docs/grafana/v5.2/plugins/developing/development/
- 7: https://grafana.com/developers/plugin-tools/tutorials/build-a-panel-plugin
- 8: https://github.com/grafana/grafana/blob/main/packages/grafana-data/src/types/plugin.ts
- 9: https://grafana.com/developers/plugin-tools/key-concepts/anatomy-of-a-plugin
- 10: https://grafana.com/developers/plugin-tools/tutorials/build-an-app-plugin
No orphaned server route artifacts remain.
The manifest’s added scriptType does not address the host-loader contract for screen.js; if the host ignores it, the static import will fail when injected as a classic script.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin.json` around lines 4 - 8, Update the plugin manifest and screen.js
host-loading integration so screen.js is loaded according to the host’s
classic-script contract rather than relying on scriptType: "module". Remove or
avoid the static import in screen.js, and use a compatible loading pattern that
works when the host ignores the manifest’s module setting.
| // Rebuilds whenever bundle.notes' identity differs — covers a | ||
| // song switch and a chart-transform provider rerunning, since | ||
| // highway.js restages a fresh array either way. Guarded | ||
| // against retry storms: skip while in flight or previously | ||
| // failed for this exact ref. | ||
| const notesRef = bundle.notes || null; | ||
| const chartChanged = notesRef !== _tvCurrentNotesRef; | ||
| const buildInFlight = _tvPendingNotesRef === notesRef; | ||
| const previouslyFailed = _tvFailedNotesRef === notesRef; | ||
| if (chartChanged && !buildInFlight && !previouslyFailed) { | ||
| // Defense-in-depth: _tvRenderFromBundle also checks this, | ||
| // but avoids a wasted token bump while the mount is | ||
| // transiently null. | ||
| if (_resolveMount(_tvHighwayCanvas)) { | ||
| const myToken = ++_tvInitToken; | ||
| _tvLastTick = -1; | ||
| _tvFetchAndInit(filename, arrIdx, myToken); | ||
| _tvLastTick = -9999; | ||
| _tvRenderFromBundle(bundle, myToken); | ||
| // fall through — cursor sync below will be a no-op | ||
| // until _tvReady flips true again after the re-init. | ||
| // until _tvReady flips true again after the re-render. | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does the host keep pumping the active renderer's draw() once setVisible(false) gates the highway?
fd -t f 'highway.js' --exec rg -n -C4 -e 'setVisible' -e '_visible' -e 'draw\(' {} \; | head -80
# Any accessor the cursor loop could use to read the current notes/bundle?
fd -t f 'highway.js' --exec rg -n -e 'getBeats' -e 'getNotes' -e 'getBundle' -e 'return\s+bundle' {} \;Repository: got-feedBack/feedBack-plugin-tabview
Length of output: 174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files =="
git ls-files | sed -n '1,200p'
echo "== locate screen/highway/readme =="
fd -t f '(^screen\.js$|^highway\.js$|^README\.md$)'Repository: got-feedBack/feedBack-plugin-tabview
Length of output: 1474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== screen.js size =="
wc -l screen.js
echo "== outlines/highway references in screen.js =="
rg -n -C 3 'highway\.js|setVisible|getBeats|_tvCursorLoop|draw\\(|bundle\.notes|highway' screen.js README.md
echo "== relevant screen.js sections =="
sed -n '630,680p' screen.js
sed -n '820,870p' screen.js
sed -n '1,80p' screen.js
echo "== README around line 46 =="
sed -n '36,56p' README.mdRepository: got-feedBack/feedBack-plugin-tabview
Length of output: 435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant screen.js references =="
rg -n -C 3 'highway-js|setVisible:|setVisible|getBeats|_tvCursorLoop|draw\(|bundle\.notes|highway|init\(' screen.js README.md || true
echo "== relevant screen.js sections =="
sed -n '630,680p' screen.js
sed -n '820,870p' screen.js
sed -n '1,80p' screen.js
sed -n '36,56p' README.md
echo "== all highway-related terms with escaped regex =="
rg -n -C 2 'highway|setVisible|draw\(' screen.js README.mdRepository: got-feedBack/feedBack-plugin-tabview
Length of output: 37620
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== renderer renderFinished listeners in the repo =="
rg -n -C 3 'renderFinished|on\(.*render|\.on|addEventListener\(' screen.js README.md package.json plugin.json || true
echo "== visibility setter behavior in codebase =="
rg -n -C 3 'function setVisible|setVisible\s*[=:]|_visible|visibility\.change|visibilitychange|highway:visibility' . || true
echo "== single-player render finished and set visible call sites =="
sed -n '460,500p' screen.js
sed -n '813,828p' screen.jsRepository: got-feedBack/feedBack-plugin-tabview
Length of output: 10440
Move the chart change dirty-check into the single-player cursor loop
In single-player mode, init() finishes, the highway is set to setVisible(false), and only _tvCursorLoop continues running. That loop currently only updates the cursor from window.highway; a changed bundle.notes identity later is only detected in draw(), so the tab can stay stale after mastery-slider moves or chart-transform reruns, contrary to the documented behavior. Expose/read the current bundle/notes from window.highway in the loop with the same guard shape as the existing getBeats() check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@screen.js` around lines 838 - 858, Move the bundle.notes identity check from
draw() into _tvCursorLoop so single-player mode detects chart updates while
highway rendering is hidden. Read the current bundle/notes from window.highway
using the same guard pattern as the existing getBeats() check, then reuse the
existing _tvCurrentNotesRef, _tvPendingNotesRef, _tvFailedNotesRef, mount, and
token guards before calling _tvRenderFromBundle.
Signed-off-by: Joe <jphinspace@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
rs2gp.py+routes.py) with building the alphaTabScoredirectly from the renderer bundle in the browser (src/chart-quantize.js+src/score-builder.js).AlphaTabApiinstance across chart rebuilds instead of destroying/recreating it, and drives its own boundsLookup-based marker instead of alphaTab's built-in cursor.Notes
chart-quantize.js) has no alphaTab dependency and is unit-tested directly; alphaTab object construction (score-builder.js) is tested against the real@coderline/alphatabpackage in Node.pyguitarpro) and a server route.Why drop the GP5 path?
The GP5 file was converted server-side from the original arrangement XML, so it had no visibility into chart-transform plugins that modify notes/strings/tuning client-side. Those changes happened after conversion, so Tab View could never see them. Building the
Scorefrom the same live bundle every other renderer draws from is the only way to pick up a transform's output. GP5's format also caps out at 7 strings, which doesn't fit extended-range instruments the bundle already supports.Test plan
npm test— 38 tests passingSummary by CodeRabbit