Cockpit band legibility, plus the CI gate that should have caught it - #110
Conversation
AGENTS.md documents `cargo fmt --check`, `cargo clippy --all-targets -D warnings` and `cargo test --workspace` as this repo's gate, but no workflow ran them — only the two cockpit web jobs. Nothing caught a lint or test regression on `main`, which is how 14 clippy findings accumulated in the cockpit alone. `cargo doc` builds but does not deny yet: rustdoc flags five pre-existing findings in griff-core (see the step's comment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014r7EtXCXS4eR3uS9Z5GXt5
…through Behaviour-preserving throughout; the new CI gate now holds the line. Mechanical: ten absolute paths hoisted into `use` declarations, a `const fn`, an `Option::map_or_else`, and a doc first paragraph split in two. Structural, where the lint had a point: - `generate_window` ran to 134 lines against an 80-line limit. Its lower half — the set's provenance line, the ranked rows, the keep actions — is now `generate_candidates`, which only reads the panel and reports the user's choice back through `show` / `keep` / `open`, exactly as the inline code did. - `main` parsed its three flags inline, with a `match` per flag whose `None` arm bailed. That is now `parse_args() -> Result<Args, ExitCode>`, so `main` reads as the sequence it is: parse, load, open, run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014r7EtXCXS4eR3uS9Z5GXt5
…ction instead of dimming the rest Turns the four red tests green. The band's cells carry the class name — `scene::resolve_band` centres it in each section's span — but `glyph_color` had no `BandFill` arm, so the cockpit painted a bare colour block where the `ratatui` preview prints "Riff" or "Breakdown". Two frontends, one `Scene`, different information: the divergence ADR-0016 exists to prevent. It also left the classification encoded by colour alone, which WCAG 1.4.1 forbids and which the Breakdown-red / Clean-green pair makes unreadable to a deuteranope. The label is now drawn, in white on the deep hues and near-black ink on the bright ones — every pairing clears 4.5:1. The de-emphasis is inverted while we are here. The class hues are dark on this surface (Breakdown clears the 3:1 floor for meaningful graphics by 0.09), so dimming the *unselected* sections with `gamma_multiply(0.55)` drove them under it — Riff landed at 1.54:1 — and left the selected section darker, quieter, than its neighbours. Selection now lifts its hue 35% toward white: every section keeps its colour and clears 3:1, and the active one is the brightest thing in the band. `LABEL_FAINT` goes with it: at 3.06:1 on the panel it was under the text floor, and the `SEC` header it drew now shares the gutter's `LABEL_DIM` (5.6:1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014r7EtXCXS4eR3uS9Z5GXt5
The colour-mapping tests pin what `glyph_color` returns; none of them prove the glyph reaches a shape. This runs one CPU frame over the demo score and reads the painted text back out of `output.shapes`. Falsified against the old mapping, the frame paints "SECF#5C5C4C3" — the header and the pitch labels, and not one class letter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014r7EtXCXS4eR3uS9Z5GXt5
…oses `rust-version = "1.74"` described nothing: `egui`/`eframe` 0.34 demand 1.92, so no 1.74 user could ever have built the cockpit. The decisions log already recorded why 1.74 was picked (it was the maintainer's own toolchain) and that the reason had lapsed. 1.92 is the true floor, not current stable — an MSRV should be the lowest version that works, not the newest that happens to be installed. Verified by `cargo +1.92 check --workspace --all-targets`, and a CI job now builds on whatever the manifest claims, so the number cannot rot again. ADR-0010 still cites 1.74 in passing; ADRs are a historical record, so the correction lives in the decisions log rather than in its text. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014r7EtXCXS4eR3uS9Z5GXt5
|
Warning Review limit reached
Next review available in: 20 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 (11)
📝 WalkthroughWalkthroughThe pull request raises the MSRV to Rust 1.92, adds CI validation, refactors cockpit CLI and corpus-loading code, updates cockpit rendering and generation controls, and applies test and sorting cleanups. ChangesToolchain Policy and CI
Cockpit Refactor and Rendering
Code Quality Maintenance
Estimated code review effort: 4 (Complex) | ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/ci.yml (2)
35-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrefer a specialized Rust caching action.
Manual caching of Cargo's
target/and~/.cargo/directories without an eviction strategy typically leads to unbounded cache growth, as old artifacts (from updated dependencies or stale incremental compilations) are never removed. Consider using a dedicated community action, likeSwatinem/rust-cache, which automatically cleans up unused artifacts, ignores unneeded.cratefiles, and optimally hashes cache keys.
.github/workflows/ci.yml#L35-L42: Replace the manualactions/cacheblock in thecheckjob withuses: Swatinem/rust-cache@v2..github/workflows/ci.yml#L82-L89: Replace the manualactions/cacheblock in themsrvjob withuses: Swatinem/rust-cache@v2. The action automatically includes the job name in its internal cache key to prevent conflicts across different toolchain runs.🤖 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 @.github/workflows/ci.yml around lines 35 - 42, Replace the manual actions/cache blocks in .github/workflows/ci.yml lines 35-42 for the check job and lines 82-89 for the msrv job with Swatinem/rust-cache@v2, preserving each job’s existing workflow structure.
53-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnforce locked dependencies in CI.
Running Cargo commands without the
--lockedflag allows Cargo to silently fetch newer versions of transitive dependencies if they are updated upstream. This can break CI reproducibility and introduce spurious failures. It is highly recommended to enforce--lockedacross all verifiable CI builds.
.github/workflows/ci.yml#L53-L61: Append the--lockedflag to thecargo clippy,cargo test, andcargo doccommands..github/workflows/ci.yml#L97-L98: Append the--lockedflag to thecargo checkcommand in themsrvjob.🤖 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 @.github/workflows/ci.yml around lines 53 - 61, Enforce locked dependencies for all specified CI builds by appending --locked to the cargo clippy, cargo test, and cargo doc commands at .github/workflows/ci.yml:53-61, and to the cargo check command in the msrv job at .github/workflows/ci.yml:97-98.cockpit/src/main.rs (1)
24-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood extraction — but add unit tests now that parsing is testable.
parse_argswas previously untestable inline logic inmain; it's now a standalone function that can be driven directly with aVec<String>-style input (or by refactoring to accept an iterator instead of always readingenv::args()), enabling coverage of the--corpus/--outmissing-directory error paths,-h/--help, and positional-input fallback. As per coding guidelines, refactors should be backed by characterization tests to guard against silent behavior drift going forward.🤖 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 `@cockpit/src/main.rs` around lines 24 - 80, Refactor parse_args to accept an injectable iterator or Vec<String>-style argument source instead of always reading env::args(), while preserving its current parsing behavior and ExitCode results. Add unit tests covering missing directories for --corpus and --out, both help flags, positional input fallback, and successful corpus/out assignment.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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 35-42: Replace the manual actions/cache blocks in
.github/workflows/ci.yml lines 35-42 for the check job and lines 82-89 for the
msrv job with Swatinem/rust-cache@v2, preserving each job’s existing workflow
structure.
- Around line 53-61: Enforce locked dependencies for all specified CI builds by
appending --locked to the cargo clippy, cargo test, and cargo doc commands at
.github/workflows/ci.yml:53-61, and to the cargo check command in the msrv job
at .github/workflows/ci.yml:97-98.
In `@cockpit/src/main.rs`:
- Around line 24-80: Refactor parse_args to accept an injectable iterator or
Vec<String>-style argument source instead of always reading env::args(), while
preserving its current parsing behavior and ExitCode results. Add unit tests
covering missing directories for --corpus and --out, both help flags, positional
input fallback, and successful corpus/out assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abc9e557-b6e4-41fd-b9c7-a84672fb6ce3
📒 Files selected for processing (12)
.github/workflows/ci.ymlCargo.tomlcockpit/src/generation.rscockpit/src/lib.rscockpit/src/main.rscore/src/pitch.rscore/tests/shuffle_window.rsdocs/decisions.log.mddocs/glossary.mdfuzz/Cargo.tomlui-core/src/dock.rsui-core/src/generate.rs
Clippy reads `rust-version` from the manifest and holds back any lint whose fix needs a newer compiler. Raising the MSRV from 1.74 to 1.92 therefore unlocked twelve findings that had been suppressed for years — the gate added in this branch catches them, which is the gate working. - `f64::midpoint` / `u16::midpoint` in three places clippy flagged as overflow-capable `(a + b) / 2`; - `Option::is_none_or` for a `map_or(true, …)`; - `const fn` on eight accessors and two reducer steps. All behaviour-preserving. Verified on the MSRV itself: `cargo +1.92 check --workspace --all-targets`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014r7EtXCXS4eR3uS9Z5GXt5
The demo's first section classifies as Breakdown, and the band boots with section 0 selected. Since the band's de-emphasis flipped from dimming-the-rest to lifting-the-selection, the boot frame paints the selected Breakdown at lift(#cf1322, 0.35) = #df656f — the base red the signature expected is no longer anywhere in the frame (0 px), which is what turned the smoke gate red on this branch. Point the signature at the lifted fill instead; its 12-step tolerance also covers the theme token #e0666f that replaces the computed lift one branch up (ADR-0028). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
1 participant
The cockpit painted the section band as bare colour blocks. The band's cells carry the class name —
scene::resolve_bandcentres "Riff" or "Breakdown" in each section's span, and theratatuipreview prints it — but the cockpit'sglyph_colorhad noBandFillarm, so it dropped the glyph. SameScene, two renderers, different information: the divergence ADR-0016 exists to prevent. It also left the classification encoded by colour alone (WCAG 1.4.1), and Breakdown-red against Clean-green is precisely the pair a deuteranope cannot separate.Fixing it changed the design, not just the code. The class hues are dark on this surface — Breakdown clears the 3:1 floor for meaningful graphics by 0.09 — so dimming the unselected sections with
gamma_multiply(0.55)drove them under it (Riff landed at 1.54:1) and left the selected section darker, and so quieter, than its neighbours. Selection now lifts its hue 35% toward white: every section keeps a legible colour and clears 3:1, and the active one is the brightest thing in the band. The class label is painted on top, in white on the deep hues and near-black ink on the bright ones — every pairing ≥4.5:1.LABEL_FAINT(3.06:1 on the panel, under the text floor) is gone with it.Nothing caught this because nothing could: there was no lint or test gate in CI.
AGENTS.mddocumentscargo clippy --all-targets -- -D warningsandcargo test --workspace, but only the two cockpit web jobs ever ran. So this PR adds the gate — and then clears the 19 clippy findings that had accumulated behind it, 14 of them in the cockpit alone (including a 134-linegenerate_windowand inline flag parsing inmain, both split; the rest mechanical and behaviour-preserving).The MSRV was fiction too:
rust-version = "1.74"in a workspace whoseegui/eframe0.34 demand 1.92, so no 1.74 user could ever have built the cockpit. Raised to 1.92 — the true floor, not current stable — verified withcargo +1.92 check --workspace --all-targets, with a CI job so it cannot rot again. The decisions log already recorded that 1.74 was the maintainer's own toolchain and that the reason had lapsed.Commits
94afb61324fec793ea109fc654f2"SECF#5C5C4C3", not one class letter984d9edThe four contrast/label tests were committed red before the fix (in
331dcc4, already on the branch) and are green now.Verification
cargo fmt --checkclean · workspace clippy 0 errors (was 19) · cockpit 34/34. The one workspace test that fails,missing_file_golden, snapshots an English OS error string and fails on any non-English machine — pre-existing, untouched, and worth its own fix.Follow-ups deliberately left out: the five rustdoc warnings in
griff-core(they're why the doc step doesn't-D warningsyet), that locale-dependent golden, and the shared theme tokens (ADR-0028, next branch).🤖 Generated with Claude Code
https://claude.ai/code/session_014r7EtXCXS4eR3uS9Z5GXt5
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests