Skip to content

feat(web): move the phrase pager under the roll, unify the transport (#78) - #94

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

feat(web): move the phrase pager under the roll, unify the transport (#78)#94
PhysShell merged 2 commits into
mainfrom
claude/modest-maxwell-0zecwn

Conversation

@PhysShell

@PhysShell PhysShell commented Jun 20, 2026

Copy link
Copy Markdown
Owner

Curation-ergonomics from the playtest (#78). Asks 1 (jump-to-phrase) and 2 (compare two phrases overlaid) shipped earlier (06f32c5); this does the remaining 3 + 4.

What

Scope

Pure layout/markup (web/static/index.html + app.js) — no JS logic or data-flow change. Element ids are unchanged, so the existing wiring and the pager.js helper tests are untouched. .splitview styling is self-contained, so it renders as its own panel under the roll.

Verification

  • 29 JS helper tests pass (node --test web/test)
  • No dangling element refs after removing the split transport; HTML well-formed; #splitView correctly precedes #capture

⚠️ Needs an in-browser eyeball. The DOM/canvas glue isn't unit-tested (ADR-0024), so the actual look — panel placement under the roll, single transport feel — should be confirmed via ./web/build.sh + serving dist/ (load the sample → Split). Anything off is a quick CSS/spacing follow-up.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95


Generated by Claude Code

Summary by CodeRabbit

  • Refactor
    • Reorganized the phrase panel interface to better group split navigation, comparison, status/details, and downloads.
    • Removed split-view Play/Stop controls so playback is handled via the main player transport.
    • Updated the capture hint text to clarify that phrase chunk navigation and downloads occur in the phrase panel, while playback is controlled above.
    • Improved playback startup so the split phrase display is refreshed/synchronized before playback begins.

…ransport (#78)

Asks 1 (jump-to-phrase) and 2 (compare two phrases overlaid) shipped
earlier (06f32c5); this does the remaining 3 + 4:

- Relocates the #splitView panel (pager, compare, tags, downloads) out of the
  capture form to directly under the piano roll, so paging/comparing happens
  beside the roll without scrolling back and forth.
- Removes the split view's own Play/Stop — they only duplicated the main
  transport's play()/stop(). The main transport sits just above the roll, and
  renderPhrase() keeps `current` synced to the shown phrase, so its Play plays
  whatever phrase (or A/B overlay) is displayed.

Pure layout/markup: no JS logic or data-flow change; element ids are unchanged
so the existing wiring and the pager.js helper tests are untouched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
@coderabbitai

coderabbitai Bot commented Jun 20, 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: 654094eb-3007-49b7-9853-ea25cfe6c790

📥 Commits

Reviewing files that changed from the base of the PR and between 3b7dfbd and bf3bf27.

📒 Files selected for processing (2)
  • web/static/app.js
  • web/static/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
  • web/static/index.html

📝 Walkthrough

Walkthrough

The splitPlay and splitStop phrase playback controls are removed from the split view. Their DOM references are deleted from the els map and their click handlers are removed from bind() in app.js. The splitView section in index.html is repositioned and restructured to include only pager, comparison, and download controls without phrase transport buttons. The main play handler is updated to call renderPhrase() when split mode is visible, ensuring the displayed phrase is synchronized before playback begins. The capture hint text is updated to reference paging and downloading via the phrase panel.

Changes

Split View Play/Stop Control Removal

Layer / File(s) Summary
splitView HTML restructure and hint text update
web/static/index.html
section#splitView is repositioned and restructured to include pager navigation (splitPrev, splitJump, splitNext), phrase comparison (splitCompare), status output (splitInfo, splitTags), and download buttons (splitDownload, splitDownloadEach, splitDownloadAll), without splitPlay/splitStop controls; capture hint paragraph updated to reference paging and downloading from the phrase panel under the roll.
Remove splitPlay/splitStop DOM refs, handlers, and sync main play handler
web/static/app.js
splitPlay and splitStop removed from the els DOM reference map; their click event listeners deleted from bind(); main els.play handler updated to call renderPhrase() before play() when split mode is active, ensuring the split phrase display stays synchronized with playback.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related issues

Possibly related PRs

  • PhysShell/griff#71: Introduced the split phrase pager UI including splitPlay/splitStop wiring in app.js and index.html, which this PR reverts by removing those specific transport controls.
  • PhysShell/griff#82: Both PRs modify the split-view UI wiring around phrase rendering — this PR removes splitPlay/splitStop and updates the main play handler to call renderPhrase(), while the related PR adds splitJump/splitCompare and other phrase navigation enhancements, both affecting the split rendering/playback flow.

Poem

🐇 Hop, hop, the buttons are gone,
No splitPlay, no splitStop to click upon.
The phrase panel lives neat, by the roll it stays,
Download and page through your musical phrase.
One play button now rules them all with grace! 🎵

🚥 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 accurately describes the main changes: moving the phrase pager under the roll and unifying the transport controls by removing duplicates.
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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b7dfbd026

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread web/static/index.html

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@web/static/index.html`:
- Around line 188-190: The text in the HTML file around line 188 incorrectly
states that users can play phrases "from the phrase panel that appears under the
roll," but the play control has been moved to the main transport interface above
the roll. Update the instruction text to clarify that the play control is now
part of the main transport above the roll, not in the phrase panel below it, so
users understand where to find the play functionality when working with cut
phrases.
🪄 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: 70a04148-d4fb-4170-99ab-de368fb6a0d0

📥 Commits

Reviewing files that changed from the base of the PR and between 7acd8a8 and 3b7dfbd.

📒 Files selected for processing (2)
  • web/static/app.js
  • web/static/index.html
💤 Files with no reviewable changes (1)
  • web/static/app.js

Comment thread web/static/index.html Outdated
…ode (#78)

Removing the split view's own Play (which did `renderPhrase(); play()`) lost
its re-sync: if a curator nudged an arrange control (Generate / mode / seed /
…) while the phrase panel was open, `arrange()` overwrote the shared `current`
with the full arrangement, so the global Play auditioned the arrangement while
the panel still showed "phrase N" (Codex P2). The global Play now re-renders
the displayed phrase first when split mode is active.

Also corrects the capture hint: play is the main transport above the roll, not
the phrase panel below it (CodeRabbit).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
@PhysShell
PhysShell merged commit b18a308 into main Jun 20, 2026
2 checks passed
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