fix(editor): the playability-lint popover can be closed again - #313
Conversation
|
Warning Review limit reached
Next review available in: 14 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 (5)
📝 WalkthroughWalkthroughThe playability-lint popover now includes an explicit close button, hides reliably through its ChangesPlayability lint popover
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
cbf3e1f to
f02f03f
Compare
The lint popover's base style (#plugin-editor .editor-lint-pop) sets
display:flex at id+class specificity, which outranks the bare .hidden
utility every close path relies on. So re-clicking the warning chip,
clicking away, Escape, and clicking an issue row all added the class
correctly while the popover stayed visible on screen — it survived until
a new chart was made or the app restarted (as reported by a tester whose
charts flag legato jumps). The bug shipped with the lint feature itself:
the menu, transport, and mixer popovers each carry an explicit .hidden
override; this one never did.
- add the `.editor-lint-pop.hidden { display: none; }` override
- add an explicit visible close affordance: a ✕ button in the popover
header (aria-labelled, closes via the same toggle path so focus
returns to the chip)
- tests/lint_pop_close.test.mjs: a CSS guard pinning the override (fails
on main) + DOM-stub tests for the toggle round-trip, the ✕ close, and
stray-click behavior — all four fail on main
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01929LgKdJMyPGLf8N1WpEVW
Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
…l as a slide Found while runtime-verifying the popover fix: the slide-reach check gated on Number.isInteger(t.slide_to) only, but every note that has been through a save/load round trip carries slide_to: -1 (the wire's "no slide" default) — so every loaded note above fret 3 flagged as "Legato jump: slide N→-1 spans N+1 frets". That sentinel spam is the "warns about legato related stuff" in the tester report. Gate on slide_to >= 0, the same read draw.js / commands.js / context-menu.js already use. Verified against a real pack through the host: the lint chip drops from 2 issues (1 bogus) to the 1 genuine hammer-on jump. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01929LgKdJMyPGLf8N1WpEVW Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
f02f03f to
ac5b962
Compare
The bug (tester report)
That window is the playability-lint popover (
#editor-lint-pop). This PR fixes both halves of the report: the popover that can't be closed, and the reason it's full of legato warnings in the first place.Fix 1 — the popover can be closed again (CSS specificity)
The popover's base rule
is id+class specificity (1,1,0), which outranks the host's bare Tailwind utility
.hidden { display: none }(0,1,0). Every close path — re-clicking the ⚠ chip, click-away, Escape, clicking an issue row — correctly adds thehiddenclass, but it has no visual effect: the popover stays on screen until the screen is rebuilt (new chart / restart), exactly as reported. The bug shipped with the lint feature itself: the editor's other popovers each carry an explicit override (.editor-menu-drop.hidden,.editor-transport-menu.hidden,.editor-mixer-drawer.hidden) — the lint pop was the one missing it.assets/v3-theme.css: add#plugin-editor .editor-lint-pop.hidden { display: none; }, with a comment naming the specificity trap.editorToggleLintPopover()so the focus round-trip (close → focus returns to the chip) is preserved.Fix 2 — the "legato related stuff" spam (the −1 sentinel)
Found while runtime-verifying fix 1 against a real pack: the legato-jump slide check gated on
Number.isInteger(t.slide_to)only — but every note that has been through a save/load round trip carriesslide_to: -1(the wire's "no slide" default), so every loaded note above fret 3 flagged asLegato jump: slide N→-1 spans N+1 frets. That's why the tester's charts "typically" warn about legato. The rule now gates onslide_to >= 0, the same readdraw.js/commands.js/context-menu.jsalready use.Tests
tests/lint_pop_close.test.mjs(new) — a CSS guard pinning the.hiddenoverride + DOM-stub tests for the toggle round-trip, the ✕ close, and stray-click behavior. All four fail on main.tests/playability_lint.test.mjs— new assertion: a note withslide_to: -1must not flag. Fails on main.npm test: 210 pass / 0 fail.npm run lint: 0 errors.Runtime verification (real host)
Served this branch through the core host with a fixture pack whose Lead part has a genuine legato jump (fret 3 → 10 hammer-on), driven with Playwright:
slide 10→-1row)🤖 Generated with Claude Code
https://claude.ai/code/session_01929LgKdJMyPGLf8N1WpEVW