Add new chart-transform plugin capability - #1000
Conversation
Chart-transform plugin capability Signed-off-by: Joe <jphinspace@gmail.com>
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a ChangesChart transform capability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Plugin
participant Coordinator as chart-transform coordinator
participant Highway
participant Renderer as Renderers and getters
Plugin->>Coordinator: Register and select provider
Coordinator->>Highway: Install transform
Highway->>Plugin: Request synchronous transform
Plugin-->>Highway: Return transformed chart
Highway->>Renderer: Expose effective chart data
Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/highway_3d/screen.js (1)
5594-5606: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid caching
bundle.tuningby reference.The fast-path checks
_lastSyncBundleTuningRef === bundleTunRefto skip label updates. As per coding guidelines,bundleis mutable and its identity must not be compared across frames. If a plugin mutates thebundle.tuningarray in-place, this reference check will incorrectly pass and the tuning labels will fail to update.Consider comparing the array contents directly (e.g., using a fixed-size scratch array) to maintain the zero-allocation fast path while respecting in-place mutations.
🤖 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 `@plugins/highway_3d/screen.js` around lines 5594 - 5606, Update the tuning-label fast path around _lastSyncBundleTuningRef so it does not rely on bundle.tuning or bundleTunRef identity. Compare the relevant tuning array contents using a reusable fixed-size scratch representation, preserving zero allocations and ensuring in-place mutations invalidate the cache and refresh labels.Source: Coding guidelines
🤖 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 `@docs/capability-recipes.md`:
- Around line 516-518: Update the recipe manifest’s compatibility value in the
documented capability entry from “degrade-noop” to “none” so it matches the
runtime registration in chart-transform.js and the domain documentation. Leave
the ownership and safety fields unchanged.
In `@plugins/highway_3d/screen.js`:
- Around line 861-865: Unify tuning and capo fallback behavior across
plugins/highway_3d/screen.js: in the tuning/capo initialization at lines
861-865, require Array.isArray(bundle.tuning) before using it and otherwise fall
back to songInfo.tuning; at lines 5555-5561, change the capo fallback from an
empty string to 0; and at lines 5596-5597, change the capo fallback from NaN to
0 so cache signatures match rendered output.
In `@static/highway.js`:
- Around line 1597-1602: Guard _restageChartTransform so it returns without
invoking the provider until the chart-ready state is established, while
preserving provider attachment during highway:created. Ensure readiness is
checked before processing notes or songInfo, and allow the normal ready path to
rerun the transform and clear any stale transform-failed diagnostic.
In `@tests/js/highway_chart_transform.test.js`:
- Around line 55-59: Strengthen the test around _reportChartTransformFailure by
invoking the catch path with a sentinel error and asserting the emitted payload
contains only the approved fields. Replace the narrow doesNotMatch check for
reason: e with an assertion against the actual reported detail, rejecting any
exception-derived field such as error, error.message, or equivalent while
preserving the local console.error assertion.
---
Outside diff comments:
In `@plugins/highway_3d/screen.js`:
- Around line 5594-5606: Update the tuning-label fast path around
_lastSyncBundleTuningRef so it does not rely on bundle.tuning or bundleTunRef
identity. Compare the relevant tuning array contents using a reusable fixed-size
scratch representation, preserving zero allocations and ensuring in-place
mutations invalidate the cache and refresh labels.
🪄 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: bdfddc5d-492c-4819-a3db-13bc3af37d47
📒 Files selected for processing (18)
CHANGELOG.mdCLAUDE.mddocs/capability-domains.mddocs/capability-recipes.mddocs/capability-roadmap.mddocs/capability-safety-matrix.mdplugins/capability_inspector/screen.jsplugins/highway_3d/CLAUDE.mdplugins/highway_3d/plugin.jsonplugins/highway_3d/screen.jsstatic/capabilities.jsstatic/capabilities/chart-transform.jsstatic/highway.jsstatic/js/highway-draw.jsstatic/v3/index.htmltests/js/chart_transform_domain.test.jstests/js/highway_chart_transform.test.jstests/js/highway_chord_render_cache.test.js
Signed-off-by: Joe <jphinspace@gmail.com>
|
Outside diff range comments (1) intentionally skipped:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Byron Gamatos <xasiklas@gmail.com> # Conflicts: # CHANGELOG.md # plugins/highway_3d/plugin.json # plugins/highway_3d/screen.js
What
Apologies if this change is pre-emptive, since I know you all have a lot on your plate at the moment!
The idea behind this change is to expose more chart data to plugins, including access & modification of the placements of notes, strings, hand positions, and other chart data, all without modifying the original.
The motivation for this change was to unblock my alternate-tuning plugin (shift all the notes to a different tuning) but it could also be used for things like chord simplification (alternate voicing or root-notes-only), alternative dynamic difficulty implementations, new practice modes, and so on.
This would remove the need for these types of plugins to have to fork and maintain their own note highways/renderers. This change also provides data for note-detection to use to support the transformed notes.
Resolves #952.
feedpak surface
got-feedback/feedpak-spec#___(once it merges, re-run this PR's checks and the gate goes green)Checklist
CHANGELOG.md[Unreleased]updated (user-visible changes)git commit -s)Summary by CodeRabbit
New Features
Bug Fixes
Documentation