Skip to content

feat(core): cap over-long phrase segments at MAX_PHRASE_BARS (#76, part 1) - #83

Merged
PhysShell merged 2 commits into
mainfrom
claude/modest-maxwell-0zecwn
Jun 19, 2026
Merged

feat(core): cap over-long phrase segments at MAX_PHRASE_BARS (#76, part 1)#83
PhysShell merged 2 commits into
mainfrom
claude/modest-maxwell-0zecwn

Conversation

@PhysShell

@PhysShell PhysShell commented Jun 19, 2026

Copy link
Copy Markdown
Owner

First of three slices for #76 (phrase-segmentation quality). This one tackles the over-long phrases — the playtest's "30+ bar phrases that don't fit a single rhythmic pattern."

What

  • New pure helper cap_segment_bars(segments, max_bars) in core/src/split.rs: subdivides any segment longer than the cap into consecutive ≤max_bars sub-ranges, leaving shorter ones intact. Composes after bar_segments (only adds cuts; still sorted, non-overlapping, full coverage).
  • MAX_PHRASE_BARS = 16 default cap (generous — splits only clearly over-long phrases; documented as tunable).
  • Both split paths apply it: web/src/lib.rs (split_to_json) and cli/src/main.rs (phrase_chunks).

Approach

Genuine red → green (this is core algorithm, not glue): 14f48a7 adds failing tests + an unimplemented!() stub; 52a1789 implements it and wires the callers.

Verification

  • core split tests 8/8 (4 new cap cases: zero-cap passthrough, within-cap whole, over-long split + remainder, exact multiple, per-segment independence); cli green; clippy -D warnings clean.
  • End-to-end (rebuilt wasm + DGD GPX): every phrase span is ≤ 16 bars, no page errors.

Honest note on scope

On the DGD guitar track the longest detected phrase is 12 bars, so the cap is a safety net there rather than a visible change — it enforces the invariant and kicks in on the longer-phrased material the playtest hit. The genuinely-visible junk on that track is the two 1-bar phrases, which is the issue's trivial-phrase part — coming next as part 2 (a min-bars/min-notes drop). Novelty dedup is part 3.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95


Generated by Claude Code

Summary by CodeRabbit

Release Notes

  • Improvements
    • Enhanced griff split phrase curation to automatically cap over-long phrases exceeding 16 bars into smaller segments. This prevents excessively long phrases from being processed as single chunks, improving curation performance and reliability.

claude added 2 commits June 19, 2026 11:04
Failing tests + stub for cap_segment_bars: a detected phrase longer than
MAX_PHRASE_BARS should subdivide into cap-sized sub-phrases — the playtest's
"30+ bar phrases that don't fit a single rhythmic pattern." Impl follows.
…split paths (#76)

cap_segment_bars subdivides any segment longer than the cap (default 16 bars)
into cap-sized sub-phrases; the web (split_to_json) and CLI (phrase_chunks) split
builders apply it right after bar_segments. Enforces "no phrase is a 30-bar blob"
— the playtest's over-long-phrase complaint.

core split 8/8; cli green; clippy clean. End-to-end on the DGD guitar track every
phrase span is ≤ 16 bars (longest detected is 12, so the cap is a safety net there
rather than a visible change) — it kicks in on the longer-phrased material the
playtest hit. The issue's other two parts — trivial-phrase drop and novelty dedup
— follow as separate PRs.
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e6a6d388-9511-45cf-9cdc-1b20575d0be5

📥 Commits

Reviewing files that changed from the base of the PR and between dea284a and 52a1789.

📒 Files selected for processing (3)
  • cli/src/main.rs
  • core/src/split.rs
  • web/src/lib.rs

📝 Walkthrough

Walkthrough

core/src/split.rs gains a MAX_PHRASE_BARS constant (16) and a cap_segment_bars function that subdivides bar-range segments exceeding a given maximum into consecutive sub-ranges. Both the CLI (phrase_chunks) and web (split_to_json) auto-split paths are updated to wrap their bar_segments output with cap_segment_bars(MAX_PHRASE_BARS).

Changes

Phrase segment bar-length capping

Layer / File(s) Summary
MAX_PHRASE_BARS constant and cap_segment_bars implementation
core/src/split.rs
Defines MAX_PHRASE_BARS = 16 with documentation, adds cap_segment_bars that splits over-length segments into consecutive sub-ranges (early-returning when max_bars == 0), and adds unit tests covering disabled cap, within-cap, over-long, exact-multiple, non-zero offset, and multi-segment cases.
CLI and web integration
cli/src/main.rs, web/src/lib.rs
Imports cap_segment_bars and MAX_PHRASE_BARS and wraps the bar_segments(...) call with cap_segment_bars(..., MAX_PHRASE_BARS) in both phrase_chunks and split_to_json.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

Possibly related PRs

  • PhysShell/griff#70: Introduced the phrase-splitting pipeline in cli/src/main.rs around bar_segments that this PR extends with bar-length capping.
  • PhysShell/griff#71: Modified the web split_to_json bar-segmentation path that this PR also updates to add cap_segment_bars.

Poem

🐇 Hoppin' through the bars, one-two-three,
No phrase too long shall escape from me!
Sixteen bars max, then snip-snip-snip,
cap_segment_bars keeps phrases crisp.
The rabbit cheers — a tidy split! 🎵

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: introducing phrase segment capping at MAX_PHRASE_BARS, and correctly contextualizes it as part 1 of a multi-part effort (#76).
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/modest-maxwell-0zecwn

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants