feat: load-your-own-tab web picker + corpus curation tooling (rights, boundaries, manifest) - #66
Merged
Merged
Conversation
Let part A come from a user-loaded MIDI file instead of only the built-in
sample, then arrange a complement over any of its tracks.
- New import-free C-ABI exports input_alloc / load_score / load_len marshal
the uploaded bytes in and a track summary out through linear memory; the
parsed Score is stashed for subsequent arrange() calls.
- arrange() gains a track selector (track < 0 = built-in sample, track >= 0 =
a track of the loaded score), reading ppqn/tempo off the source.
- MIDI only for now: griff-core's MIDI import is always available, so the wasm
stays import-free (WebAssembly.instantiate(bytes, {})); Guitar Pro needs a
getrandom backend that avoids wasm-bindgen and is a follow-up (ADR-0024).
- UI: file input + track <select>; README/ABI updated; tests cover rejecting
non-MIDI bytes and the load -> arrange-over-imported-track path.
Capture per-chunk rights + provenance at curation time — the documented precondition for mass curation (decisions 2026-06-12). Rights status is not derivable from content, so backfilling a grown corpus would mean re-researching provenance per source; the field must exist before the first curation session. - ChunkMeta gains an optional rights: Option<RightsInfo> under the same serde(default, skip_serializing_if) pattern as the other versioned fields, so pre-v7 records round-trip byte-identically. SCHEMA_VERSION -> 7. - RightsInfo = rights_status (public_domain / cc_by / cc_by_sa / copyrighted_composition / unknown), acquisition (community_tab_site / purchased_official / self_transcribed / omr_from_scan / artist_provided), redistributable: bool (a typed fact so novelty.rs and any future export gate filter without scanning prose), and a free-form notes string. - griff curate prompts for the four fields; defaults match the common case (scraped community tab of a copyrighted composition, not redistributable). - Tests: v7 round-trip, pre-v7 loads-as-None, enum round-trips, proptest coverage, and a curate integration test that drives the new prompt.
Close the two remaining corpus-tooling gaps before mass curation. - curate now persists the measured track's detected phrase boundaries (S4) instead of hardcoding an empty list, scaling the detector's tick gaps to the source PPQN exactly as `griff phrases` does. - New `griff manifest <dir>` builds a CorpusManifest from a directory of curated *.chunk.json / *.group.json records and prints a coverage summary: count toward the S7 ~100-phrase gate, cohort mix, rights coverage, and review status; writes <dir>/manifest.json. - Tests: curate persists the detector's boundaries (two_phrases fixture), and manifest assembles both curated chunks at the current schema version.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTwo independent features land together: (1) corpus schema bumped to v7, adding ChangesCorpus Schema v7 + CLI Curation Extensions
WASM wasm-bindgen Refactor and Browser UI
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser JS
participant Load as load_score
participant Loaded as LOADED (thread_local)
participant Arrange as arrange + arrange_to_json
rect rgba(70, 130, 180, 0.5)
note over Browser, Arrange: Stage 1 — MIDI/Guitar Pro Upload
Browser->>Browser: loadFile(file)
Browser->>Load: load_score(bytes)
Load->>Loaded: parse & store Score on success
Load-->>Browser: JSON { error, ppqn, tempo, bars, tracks }
Browser->>Browser: populateTracks(summary)
end
rect rgba(60, 179, 113, 0.5)
note over Browser, Arrange: Stage 2 — Arrangement
Browser->>Arrange: arrange(mode, seed, offset, variation, track)
Arrange->>Loaded: resolve track_index or use sample
Arrange->>Arrange: arrange_to_json(source, track, params)
Arrange-->>Browser: JSON { partA: {name, role, notes}, partB: {...} }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/tests/curate_cmd.rs (1)
87-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPre-existing tests need additional stdin for the new rights prompts.
This test (and
curate_records_gesture_stats,curate_records_style_cohort,curate_ensemble_links,curate_records_complexity) was written before thegather_rightsfunction was added. These tests now need 4 additional blank lines of stdin input to accept the rights defaults, otherwise they will hang waiting for input.Example fix for this test
child .stdin .as_mut() .expect("piped stdin") // id, title, tuning (default), tags (none), flags (default), decision (none) - .write_all(b"p3_001\nPhase Three\n\n\n\n\n") + // + rights: status (default), acquisition (default), redistributable (default), notes (empty) + .write_all(b"p3_001\nPhase Three\n\n\n\n\n\n\n\n\n\n") .expect("write curate answers");Apply similar fixes to lines 152, 210, 251, and 355.
🤖 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 `@cli/tests/curate_cmd.rs` around lines 87 - 93, The tests in curate_cmd.rs were written before the gather_rights function was added and now need additional stdin input to handle the new rights prompts. In the write_all call for the stdin input, add 4 additional newline characters (blank lines) to the end of the byte string to accept the rights defaults. Apply this same fix to the write_all calls in the curate_records_gesture_stats, curate_records_style_cohort, curate_ensemble_links, and curate_records_complexity test functions to prevent the tests from hanging while waiting for input.web/src/lib.rs (1)
146-148:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
json_escapecan still emit invalid JSON for control characters.Line 146 only escapes
\and". Imported track names / error strings can include\n,\r,\t, or other control bytes, which makes the generated JSON invalid and can crashJSON.parseon the JS side.Suggested fix
fn json_escape(s: &str) -> String { - s.replace('\\', "\\\\").replace('"', "\\\"") + let mut out = String::with_capacity(s.len() + 8); + for ch in s.chars() { + match ch { + '\\' => out.push_str("\\\\"), + '"' => out.push_str("\\\""), + '\n' => out.push_str("\\n"), + '\r' => out.push_str("\\r"), + '\t' => out.push_str("\\t"), + c if (c as u32) < 0x20 => { + let _ = write!(out, "\\u{:04x}", c as u32); + } + c => out.push(c), + } + } + out }Also applies to: 206-210, 228-236, 341-345
🤖 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 `@web/src/lib.rs` around lines 146 - 148, The json_escape function only escapes backslashes and double quotes, but JSON also requires escaping control characters like newlines, carriage returns, tabs, and other control bytes. Enhance the json_escape function to include additional replace calls for all required JSON escape sequences: escape newline characters to \n, carriage returns to \r, tabs to \t, and any other control characters that could break JSON parsing. Apply the same comprehensive escaping logic at the other locations mentioned (206-210, 228-236, 341-345) where similar JSON escaping is needed.
🧹 Nitpick comments (1)
web/static/app.js (1)
64-73: ⚡ Quick winAdd an upload size cap before calling into WASM memory allocation.
input_alloc(bytes.length)currently trusts file size. A very large file can grow linear memory aggressively and degrade/freezes the page.Suggested guard
+ const MAX_UPLOAD_BYTES = 8 * 1024 * 1024; // tune as needed + function loadFile(file) { + if (!wasm) return; + if (file.size > MAX_UPLOAD_BYTES) { + els.status.classList.add('error'); + els.status.textContent = `load failed: file too large (${file.size} bytes)`; + return; + } const reader = new FileReader();🤖 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 `@web/static/app.js` around lines 64 - 73, In the loadFile function, add a size validation check on bytes.length before calling wasm.input_alloc(bytes.length) to prevent excessively large files from being processed. If the file size exceeds a reasonable maximum threshold (define an appropriate constant for this limit), either throw an error or reject the file upload with a user-friendly error message to prevent memory exhaustion and page degradation.
🤖 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 `@cli/tests/curate_cmd.rs`:
- Around line 537-539: The stdin input written to the curate command in the test
loop is missing additional blank lines needed for rights prompts. In the
write_all call where format is used to write "{id}\nChunk {id}\n\n\n\n\n\n", add
4 more newline characters to the format string to provide the necessary blank
line responses for the rights prompts that the curate command expects during
execution.
- Around line 461-466: The stdin input in the write_all call is missing values
for the rights prompts that were added to gather_curate_inputs. The current
stdin string provides only 7 values but the gather_rights function now requires
4 additional inputs for rights status, acquisition, redistributable, and notes.
Add 4 more trailing newline characters to the stdin string passed to write_all
in the write_all call to provide the missing input values and prevent the test
from hanging.
In `@web/src/lib.rs`:
- Around line 257-263: The new public API implementations including arrange,
input_alloc, load_score, and load_len functions are currently committed together
with their covering tests in the same commit, which violates project coding
guidelines that require tests to be committed first following the TDD
red-green-refactor pattern and prohibit API implementations from sharing commits
with their tests. Reorder your commits by first creating a commit containing
only the test cases for these functions, then create a separate subsequent
commit containing only the API implementations themselves (the arrange,
input_alloc, load_score, and load_len functions and any supporting
implementation code).
---
Outside diff comments:
In `@cli/tests/curate_cmd.rs`:
- Around line 87-93: The tests in curate_cmd.rs were written before the
gather_rights function was added and now need additional stdin input to handle
the new rights prompts. In the write_all call for the stdin input, add 4
additional newline characters (blank lines) to the end of the byte string to
accept the rights defaults. Apply this same fix to the write_all calls in the
curate_records_gesture_stats, curate_records_style_cohort,
curate_ensemble_links, and curate_records_complexity test functions to prevent
the tests from hanging while waiting for input.
In `@web/src/lib.rs`:
- Around line 146-148: The json_escape function only escapes backslashes and
double quotes, but JSON also requires escaping control characters like newlines,
carriage returns, tabs, and other control bytes. Enhance the json_escape
function to include additional replace calls for all required JSON escape
sequences: escape newline characters to \n, carriage returns to \r, tabs to \t,
and any other control characters that could break JSON parsing. Apply the same
comprehensive escaping logic at the other locations mentioned (206-210, 228-236,
341-345) where similar JSON escaping is needed.
---
Nitpick comments:
In `@web/static/app.js`:
- Around line 64-73: In the loadFile function, add a size validation check on
bytes.length before calling wasm.input_alloc(bytes.length) to prevent
excessively large files from being processed. If the file size exceeds a
reasonable maximum threshold (define an appropriate constant for this limit),
either throw an error or reject the file upload with a user-friendly error
message to prevent memory exhaustion and page degradation.
🪄 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: e7ea29a7-d297-4e90-91af-2ec91bf2682f
📒 Files selected for processing (11)
cli/src/main.rscli/tests/curate_cmd.rscore/src/corpus.rscore/tests/corpus_schema.rscore/tests/similarity.rspreview/tests/curation.rsweb/README.mdweb/src/lib.rsweb/static/app.jsweb/static/index.htmlweb/static/style.css
Comment on lines
+461
to
+466
| child | ||
| .stdin | ||
| .as_mut() | ||
| .expect("piped stdin") | ||
| .write_all(b"bnd_001\nBoundaries\n\n\n\n\n\n") | ||
| .expect("write curate answers"); |
There was a problem hiding this comment.
Stdin input is missing the rights prompts — test will hang.
The gather_curate_inputs function now calls gather_rights which prompts for 4 additional inputs (status, acquisition, redistributable, notes). The current stdin only provides 7 values but 11 are needed.
Proposed fix
child
.stdin
.as_mut()
.expect("piped stdin")
- .write_all(b"bnd_001\nBoundaries\n\n\n\n\n\n")
+ .write_all(b"bnd_001\nBoundaries\n\n\n\n\n\n\n\n\n\n")
.expect("write curate answers");The 4 trailing blank lines accept defaults for: rights status (copyrighted), acquisition (community_tab_site), redistributable (no), and notes (empty).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| child | |
| .stdin | |
| .as_mut() | |
| .expect("piped stdin") | |
| .write_all(b"bnd_001\nBoundaries\n\n\n\n\n\n") | |
| .expect("write curate answers"); | |
| child | |
| .stdin | |
| .as_mut() | |
| .expect("piped stdin") | |
| .write_all(b"bnd_001\nBoundaries\n\n\n\n\n\n\n\n\n\n") | |
| .expect("write curate answers"); |
🤖 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 `@cli/tests/curate_cmd.rs` around lines 461 - 466, The stdin input in the
write_all call is missing values for the rights prompts that were added to
gather_curate_inputs. The current stdin string provides only 7 values but the
gather_rights function now requires 4 additional inputs for rights status,
acquisition, redistributable, and notes. Add 4 more trailing newline characters
to the stdin string passed to write_all in the write_all call to provide the
missing input values and prevent the test from hanging.
Comment on lines
+537
to
+539
| .expect("piped stdin") | ||
| .write_all(format!("{id}\nChunk {id}\n\n\n\n\n\n").as_bytes()) | ||
| .expect("write curate answers"); |
There was a problem hiding this comment.
Stdin input for manifest test curate calls also needs rights prompts.
Each curate invocation in this loop needs 4 additional blank lines for the rights prompts.
Proposed fix
child
.stdin
.as_mut()
.expect("piped stdin")
- .write_all(format!("{id}\nChunk {id}\n\n\n\n\n\n").as_bytes())
+ .write_all(format!("{id}\nChunk {id}\n\n\n\n\n\n\n\n\n\n").as_bytes())
.expect("write curate answers");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .expect("piped stdin") | |
| .write_all(format!("{id}\nChunk {id}\n\n\n\n\n\n").as_bytes()) | |
| .expect("write curate answers"); | |
| .expect("piped stdin") | |
| .write_all(format!("{id}\nChunk {id}\n\n\n\n\n\n\n\n\n\n").as_bytes()) | |
| .expect("write curate answers"); |
🤖 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 `@cli/tests/curate_cmd.rs` around lines 537 - 539, The stdin input written to
the curate command in the test loop is missing additional blank lines needed for
rights prompts. In the write_all call where format is used to write "{id}\nChunk
{id}\n\n\n\n\n\n", add 4 more newline characters to the format string to provide
the necessary blank line responses for the rights prompts that the curate
command expects during execution.
Comment on lines
+257
to
+263
| pub extern "C" fn arrange( | ||
| mode: u32, | ||
| seed: u32, | ||
| offset: i32, | ||
| variation: f32, | ||
| track: i32, | ||
| ) -> *const u8 { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target='web/src/lib.rs'
api_re='^\+pub extern "C" fn (arrange|input_alloc|load_score|load_len)\b'
test_re='^\+\s*#\[test\]|^\+\s*fn (deterministic_for_identical_args|load_rejects_non_midi_bytes|load_then_arrange_round_trips_a_midi_score)\b'
echo "== commit timeline for ${target} =="
git log --reverse --format='%H %ad %s' --date=short -- "$target"
echo
echo "== commits adding API/test lines =="
for sha in $(git log --reverse --format='%H' -- "$target"); do
patch="$(git show --unified=0 --pretty=format: "$sha" -- "$target")"
if printf '%s\n' "$patch" | rg -n "$api_re|$test_re" >/tmp/griff_wasm_matches.txt; then
echo "commit: $sha"
cat /tmp/griff_wasm_matches.txt
echo
fi
doneRepository: PhysShell/griff
Length of output: 763
Split API implementation and tests into separate commits per coding guidelines.
Both the new public API implementations (arrange, input_alloc, load_score, load_len) and their covering tests are present in the same commit (6a3f7bb77624dcddd19ee76cc524cc3bd435493f). This violates the project guidelines: (1) tests must be committed first per TDD red-green-refactor, and (2) new public API implementations must never share a commit with their covering tests.
Reorder commits so tests are added first, then split API implementation into a separate commit.
🤖 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 `@web/src/lib.rs` around lines 257 - 263, The new public API implementations
including arrange, input_alloc, load_score, and load_len functions are currently
committed together with their covering tests in the same commit, which violates
project coding guidelines that require tests to be committed first following the
TDD red-green-refactor pattern and prohibit API implementations from sharing
commits with their tests. Reorder your commits by first creating a commit
containing only the test cases for these functions, then create a separate
subsequent commit containing only the API implementations themselves (the
arrange, input_alloc, load_score, and load_len functions and any supporting
implementation code).
Source: Coding guidelines
PhysShell
changed the title
griff manifest
Close the two remaining corpus-tooling gaps before mass curation.
- curate now persists the measured track's detected phrase boundaries (S4)
instead of hardcoding an empty list, scaling the detector's tick gaps to the
source PPQN exactly as griff phrases does.
- New griff manifest <dir> builds a CorpusManifest from a directory of
curated *.chunk.json / *.group.json records and prints a coverage summary:
count toward the S7 ~100-phrase gate, cohort mix, rights coverage, and review
status; writes <dir>/manifest.json.
- Tests: curate persists the detector's boundaries (two_phrases fixture), and
manifest assembles both curated chunks at the current schema version.…0025) The corpus is swancore-first and swancore tabs are overwhelmingly Guitar Pro, so phone-side curation needs GP loading in the browser. The Rust GP reader (guitarpro -> zip -> time/getrandom) pulls wasm-bindgen with no lean shortcut (a custom getrandom backend fails to compile on wasm32 in getrandom 0.4.2, and time pulls js-sys regardless), so ADR-0025 supersedes ADR-0024's import-free cdylib for the web front. - web build enables `gp`: MIDI and Guitar Pro (.gp3/.gp4/.gp5/.gpx) both import through the shared import_score_auto -- the same parser as the CLI. - exports become two #[wasm_bindgen] functions returning JSON strings (arrange, load_score(bytes)); the manual linear-memory marshalling and the C-ABI alloc/len shims are gone. The page loads the generated ES module. - getrandom uses its wasm_js backend (--cfg getrandom_backend="wasm_js"); build.sh runs wasm-bindgen --target web; wasm-bindgen-cli is pinned to the crate version and installed + cached in CI. - payload grows ~90 KiB -> ~830 KiB (accepted); the lean MIDI-only path still exists behind default-features = false. - docs: ADR-0025 + decisions.log entry + web/README; a host test proves the GP path is wired (a GP seed routes to the GP reader, not the MIDI fallback).
A loaded MIDI/GP track name (or a Debug/Display error string) can contain \n, \r, \t, or other control bytes. The old json_escape escaped only \ and ", so such a name produced invalid JSON and crashed JSON.parse in the browser -- exactly the path that matters now that arbitrary GP/MIDI files load. Escape the RFC 8259 control range: short forms for \n \r \t, \uXXXX for the rest. All JSON string values route through json_escape, so this one fix covers every call site (track names + error fields). Adds a unit test.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
web/src/lib.rs (1)
442-458: 💤 Low valueGP reader routing test depends on specific error message text.
The assertion at line 455 checks that the error contains
"Guitar Pro"to prove the GP reader handled the file. This correctly validates the GP feature is enabled in the WASM build, but the test will break ifgriff_core's GP error message format changes.Consider documenting this coupling or adding a comment noting the dependency on the upstream error format.
🤖 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 `@web/src/lib.rs` around lines 442 - 458, The assertion checking for "Guitar Pro" in the JSON error message within the load_routes_guitar_pro_bytes_to_the_gp_reader test function creates a coupling to griff_core's specific error message formatting. Add a comment above this assertion documenting that it depends on the upstream GP reader's error message format to clearly explain why this specific text check is necessary and warn future maintainers about this external dependency.
🤖 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 @.github/workflows/web.yml:
- Line 40: The `actions/cache@v4` reference uses a version tag instead of a
pinned commit SHA, which weakens supply-chain security. Replace the version tag
`v4` in the `uses: actions/cache@v4` line with a specific commit SHA (e.g., the
full 40-character Git commit hash of the v4 release) to ensure the exact action
code is pinned and cannot be modified unexpectedly.
In `@web/build.sh`:
- Around line 22-27: The current check only verifies that the wasm-bindgen
command exists but does not validate that its version matches the version
specified in Cargo.toml. After confirming the command is available, extract the
installed wasm-bindgen version by running wasm-bindgen with a version flag, then
compare the installed version with the want variable extracted from Cargo.toml.
If the versions do not match, display an error message and exit, ensuring the
build only proceeds when the exact matching version is installed.
In `@web/static/app.js`:
- Around line 52-71: The loadFile function currently only handles errors that
occur during file parsing and WASM execution through the onload handler, but it
does not handle failures in the file read operation itself. Add a reader.onerror
handler to the FileReader instance that provides user feedback when the file
read fails (e.g., due to permissions or disk issues). The error handler should
update els.status with an appropriate error message and add the error class,
similar to how errors are handled in the existing catch block, so the UI
properly communicates read failures to the user.
---
Nitpick comments:
In `@web/src/lib.rs`:
- Around line 442-458: The assertion checking for "Guitar Pro" in the JSON error
message within the load_routes_guitar_pro_bytes_to_the_gp_reader test function
creates a coupling to griff_core's specific error message formatting. Add a
comment above this assertion documenting that it depends on the upstream GP
reader's error message format to clearly explain why this specific text check is
necessary and warn future maintainers about this external dependency.
🪄 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: 755562e2-fa09-4e11-bb01-701306b9fc74
📒 Files selected for processing (10)
.github/workflows/web.ymldocs/adr/0025-guitar-pro-in-browser-needs-wasm-bindgen.mddocs/adr/README.mddocs/decisions.log.mdweb/Cargo.tomlweb/README.mdweb/build.shweb/src/lib.rsweb/static/app.jsweb/static/index.html
✅ Files skipped from review due to trivial changes (3)
- docs/decisions.log.md
- docs/adr/0025-guitar-pro-in-browser-needs-wasm-bindgen.md
- docs/adr/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- web/static/index.html
The browser loads untrusted files. Add an input-size cap (16 MiB; real tabs are far smaller) and refuse a GP6 .gpx whose BCFZ header declares an implausible uncompressed length -- the GP reader does Vec::with_capacity(declared) before decompressing a byte, so an 8-byte file could request gigabytes. Enforced in Rust (load_to_json) and mirrored as a fast pre-check in the browser. Format validation itself already exists (header sniffing in import_score_auto rejects a disguised non-MIDI/GP file cheaply). Also addresses CodeRabbit review on the wasm-bindgen commits: - build.sh verifies the wasm-bindgen CLI version *matches* the pinned crate, not just that it exists (mismatched glue/wasm would break at runtime). - app.js: add a FileReader onerror handler so read failures surface in the UI. - note the GP-routing test's coupling to griff-core's error text.
This was referenced Jun 17, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What
Three related pieces toward driving griff from a phone and growing the corpus.
1. Web playground: load your own MIDI tab (
5381765)input_alloc/load_score/load_lenmarshal the uploaded bytes in and a track summary out through linear memory; the parsedScoreis stashed for subsequentarrange()calls.WebAssembly.instantiate(bytes, {}), ~220 KiB, no wasm-bindgen). Guitar Pro in the browser is a follow-up — itsgetrandompath would pull in wasm-bindgen and break the import-free design (ADR-0024); GP already works ingriff curate.2. Corpus schema v7:
RightsInfo(ca71f86)ChunkMetagains optionalrights—rights_status/acquisition/redistributable/notes— the documented precondition for mass curation (decisions 2026-06-12): rights status is not derivable from content and cannot be backfilled.serde(default, skip_serializing_if));griff curateprompts for the four fields with safe defaults.3. Corpus tooling (
6a3f7bb)curatenow persists the measured track's detected phrase boundaries (S4) instead of hardcoding an empty list, scaling the detector's gaps to the source PPQN asgriff phrasesdoes.griff manifest <dir>builds aCorpusManifestfrom*.chunk.json/*.group.jsonand prints coverage toward the S7 ~100-phrase gate (cohort mix, rights, reviews).Verification
-D warningsclean, rustfmt clean (changed files), full test suite green — including new web load tests, v7 round-trip + proptest, and curate-boundary + manifest integration tests.wasm32-unknown-unknownrelease build is verified import-free.Deploy
Merging to
maintriggers.github/workflows/web.yml→ GitHub Pages (enable Pages → "GitHub Actions" in repo settings if not already), so the picker becomes tappable from a phone.🤖 Generated with Claude Code
Summary by CodeRabbit
manifest.jsonwith coverage summariesarrange,load_score) and revised architecture notes