feat(ui): prove persistent canvas::Cache on the now-playing waveform#137
Conversation
Canvas is feature-gated in Iced 0.14 and absent from the default set; the persistent-cache waveform (#131) needs iced::widget::canvas. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pure, Iced-free logic for the now-playing waveform: per-id bar heights and the RenderKey (sound id + quantized progress bucket) that decides when the canvas::Cache is invalidated. Bucketing prevents per-frame cache thrash from sub-pixel progress jitter. (#131) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The cache lives in this module's own state and is cleared ONLY when sync() observes a render-key change (different sound or a crossed progress bucket) — never rebuilt in view(). This is the persistent-cache lifecycle ADR-009 mandates proving on a low-risk surface before #13/#92. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Static bars draw through the persistent cache (one tessellation per sound); the playhead is a per-frame overlay that never invalidates the cached bars. Replaces the manual container-width progress bar. (#131) Note: canvas widget constructed via canvas::Canvas::new(program) rather than the free fn canvas(program) since the iced::widget::canvas import shadows the helper in scope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add `now_playing: NowPlaying` field to HonkHonk struct. Initialize in `new` and `new_for_test`. Call `sync` at the end of `update` so the waveform cache stays in step with playback state. Fix the stale `view_now_playing` call site to pass `&self.now_playing` as first arg. All cache-lifecycle logic lives in `ui::now_playing::NowPlaying`; app.rs holds only the minimal glue (field + 2 inits + 1 sync + arg fix). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add two regression tests that assert the canvas::Cache is cleared exactly once on state change, not on every frame — the observable proxy for no-per-frame re-tessellation (ADR-009). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes ADR-009 decision step 4 — now_playing.rs is the reference impl for the persistent canvas::Cache lifecycle the tile grid must follow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y docs Fix 1: reduce PROGRESS_BUCKETS from 240 to WAVEFORM_BARS (48) so the cache is cleared only when played_to actually crosses a bar boundary — the only cache-sensitive event. Previously the cache was cleared ~5× more often than the bar geometry changed. The smooth playhead overlay uses raw progress and is unaffected. Fix 2: update PROGRESS_BUCKETS doc to clarify it is a max bucket index (inclusive), with PROGRESS_BUCKETS+1 distinct values, and why it equals the bar count. Fix 3: add comment in WaveformProgram::draw noting why the played/unplayed bar split baked into cached geometry is correct, and what would break it. Fix 4: add clarity comments — bounds.size() triggers resize re-tessellation; canvas::Canvas::new struct form avoids shadowed helper name; sub-bucket jitter test documents its bucket-width dependency. Fix 5: hash the sound id once in samples() and mix in bar index via wrapping integer arithmetic instead of re-hashing the id 48 times per frame. Co-Authored-By: Claude <noreply@anthropic.com>
Records the ordered TDD plan executed for the now-playing waveform canvas-cache work, consistent with docs/superpowers/plans/ convention. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 17 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a persistent ChangesPersistent canvas::Cache waveform rendering
Sequence Diagram(s)sequenceDiagram
participant App as HonkHonk
participant NP as NowPlaying
participant Cache as canvas::Cache
participant WP as WaveformProgram
rect rgba(70, 130, 180, 0.5)
note over App,Cache: update() — per message
App->>NP: sync(playing, progress)
NP->>NP: RenderKey::matches(playing, progress)?
alt render key changed
NP->>Cache: clear()
NP->>NP: store new RenderKey
else same render key
NP-->>App: no-op (cache preserved)
end
end
rect rgba(34, 139, 34, 0.5)
note over App,WP: view() — every frame
App->>NP: &now_playing into view_now_playing
NP->>WP: construct WaveformProgram(&cache, samples, progress)
WP->>Cache: cache.draw() → tessellate waveform bars (skipped if cache valid)
WP->>WP: draw playhead overlay line (every frame)
WP-->>App: canvas::Canvas element
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
NowPlaying::sync called render_key() on every update(), allocating an owned id String even on the common no-change frame. Add an allocation-free RenderKey::matches and only build the owned key when it actually changes (i.e. on a cache clear). A test pins matches ≡ render_key equality. Addresses the Info-1 finding from the #137 local code review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Local code review (Claude Opus 4.8)CodeRabbit hit its review rate limit / org credit cap on this PR (see the rate-limit notice above), so this is a stand-in review of the diff for correctness and reuse/simplification/efficiency. CI is fully green on the reviewed HEAD. 🔴 Critical: none · 🟠 Warning: none · 🔵 Info: 2 Correctness — cache lifecycle is sound ✅Focused on the most bug-prone part: whether cached geometry can go stale.
🔵 Info-1 —
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/superpowers/plans/2026-06-22-issue-131-persistent-canvas-cache.md`:
- Around line 97-98: The documentation file is missing the `matches()` method in
the RenderKey impl block, which is a critical optimization for allocation-free
comparisons in the hot path. Add the `matches()` method to the RenderKey impl
block after the `render_key` function definition, implementing the comparison
logic that checks if the bucket equals progress_bucket(progress) and the id
matches the playing parameter. Additionally, add the test function
`matches_is_allocation_free_equivalent_of_render_key` to the test suite to
validate that matches() behaves as the allocation-free equivalent of comparing
the owned RenderKey.
- Around line 283-291: The sync() method currently allocates a new RenderKey on
every call via render_key() for comparison, which is wasteful in the common
case. Replace the current implementation to use the RenderKey::matches() method
instead, checking if the existing self.key matches the provided playing and
progress parameters without allocating a new key. Only call render_key() to
create and store a new key when the comparison fails and the cache needs to be
cleared, following the allocation-free pattern demonstrated in the proposed
implementation.
- Around line 122-131: The `samples()` function currently re-hashes the id
string for each bar within the loop closure passed to `std::array::from_fn`,
which is inefficient. Refactor by moving the id hashing outside the loop to
compute it once upfront, then for each bar index, mix in the index value using
cheap wrapping arithmetic (specifically integer multiplication) rather than
hashing the index again. This reduces the hashing cost from O(n) to O(1) and
aligns with the proven implementation pattern referenced in the comment.
- Line 119: The PROGRESS_BUCKETS constant is hardcoded to 240 instead of being
derived from WAVEFORM_BARS as intended by the PR objectives. Change the
PROGRESS_BUCKETS constant definition from the hardcoded numeric value 240 to
WAVEFORM_BARS as u16 to ensure progress bucket resolution matches the bar count
(48) and achieve the intended reduction in cache invalidations. This aligns the
implementation with the optimization goal of reducing unnecessary cache clears.
In `@src/app.rs`:
- Around line 160-162: The now_playing field and its related wiring (at lines
321, 363, 960-965, and 1225) are being added directly to src/app.rs, which
violates the guideline that this file is off-limits for additions due to its
existing size violation. Refactor by first splitting src/app.rs to extract the
appropriate module surface for now-playing waveform cache management, then add
the now_playing field and all associated wiring to the newly extracted module
boundary instead of the main app.rs file.
In `@src/ui/now_playing.rs`:
- Around line 93-97: In the now_playing.rs file, the comparison operator in the
color assignment condition is using `x <= played_to` which causes the first bar
to render as played even at 0% progress. Change the less-than-or-equal operator
to a strict less-than operator (from `<=` to `<`) in the condition `if x <=
played_to` so that when progress is 0.0, no bars appear as played.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a23a4c51-fd1c-4ba6-82d5-f3166e86f9ef
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomldocs/adr/009-canvas-sticker-tiles-rejected.mddocs/superpowers/plans/2026-06-22-issue-131-persistent-canvas-cache.mdpackaging/flatpak/cargo-sources.jsonsrc/app.rssrc/ui/mod.rssrc/ui/now_playing.rssrc/ui/waveform.rs
CodeRabbit review: the now-playing waveform used `x <= played_to`, so at 0% progress (played_to == 0.0) the first bar still rendered as played. Use strict `<` so zero progress shows zero played bars. Also add an "as-planned, not as-shipped" banner to the #131 plan doc so #13/#92 implementers copy the proven pattern from the shipped code + ADR-009 (PROGRESS_BUCKETS = WAVEFORM_BARS = 48, hash-once samples, allocation-free RenderKey::matches) rather than the older plan snippets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolves the app.rs overlap with #131 (#137): the effects_ui and now_playing fields + their constructor inits now coexist; and audio/effects/mod.rs combines #131's neighbours with this branch's `layout` module alongside #136's `formant` modules — all additive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Why
ADR-009 (decision step 4) mandates proving the correct persistent-
canvas::Cachepattern on a low-risk surface before the tile grid (#13/#92). PR #96 failed by callingCache::new()insideview()— which Iced re-runs everyMessage, throwing the cache away each frame (~6000 path ops/frame at 200 tiles). This PR proves the persistent-cache lifecycle on the now-playing waveform so the grid can copy a known-good pattern instead of repeating PR #96's regression.What
The now-playing bar's manual
container-width progress bar is replaced with a canvas-rendered waveform whose geometry is cached and reused across frames:src/ui/waveform.rs(new, pure, Iced-free): deterministic per-id barsamples, a quantizedprogress_bucket, and theRenderKey(sound id + bucket) that decides when the cache is invalidated. Keeping it renderer-free makes the cache-lifecycle decision unit-testable without a GPU.src/ui/now_playing.rs:NowPlayingowns the persistentcanvas::Cacheand the lastRenderKey.sync(playing, progress)clears the cache only when the key changes — it is the single place the cache is ever cleared.WaveformProgramdraws the static bars throughcache.draw(..)(one tessellation per sound) and the moving playhead as a cheap per-frame overlay that never invalidates the cached bars.src/app.rs:HonkHonkholds onenow_playing: NowPlayingfield and makes a single delegatingsync(..)call inupdate(). No cache-lifecycle logic lives inapp.rs(it is over the 400-line budget) — all of it stays in the now-playing module.docs/adr/009-...md: records the proven pattern (cache in persistent state, render-key invalidation, quantized inputs, cheap moving elements as overlays) as the reference for feat(ui): canvas sticker tiles + hand-drawn glyphs #13/feat(ui): sticker tile hover + playing animations (PR2 of #13) #92.The progress bucket resolution is tied to the bar count (
WAVEFORM_BARS), since the cached bars are the only cache-sensitive content and the smooth playhead reads raw progress — so cache clears happen at most once per bar boundary, not per frame.The
canvasIced feature is enabled (it is not in the default set); the resultingCargo.lockadditions (the MIT/Apache-2.0lyontessellation family +float_next_after) were vendored intopackaging/flatpak/cargo-sources.jsonvia the repo's generator to keep the #121 Flatpak freshness gate green.Design decisions
Ambiguous calls made (the issue body was the only spec; brainstorming was skipped):
SoundEntrycarries no real waveform/PCM data, and decoding audio to draw a real waveform is out of scope for a cache-pattern proof. A per-id deterministic sample array gives stable, distinct-looking bars that legitimately only need re-tessellation when the sound changes — exactly the property the cache pattern needs to demonstrate. Real PCM-derived waveforms can replacesamples()later without touching the cache lifecycle.(sound id, progress bucket); bucket count = bar count (48). Bucketing prevents sub-pixel progress jitter from thrashing the cache. Tying the resolution toWAVEFORM_BARS(rather than an arbitrary 240) means a cache clear happens only when a bar's played/unplayed color could actually change, keeping the smooth playhead motion in the un-cached overlay.Frameoverlay, not part of the cached geometry. This is the core of the pattern: frequently-moving elements are drawn fresh each frame so the expensive tessellation (the bars) stays cached.syncruns inupdate(), notview().syncneeds&mut self(it stores the key);view()is&self. Every code path that mutatesself.playing/self.progressroutes throughupdate(), andsyncruns after the message match, so the cache is always fresh before the nextview().canvas::Canvas::new(program)instead of the freecanvas(program)fn, becauseuse iced::widget::canvas;shadows the helper name — a correct equivalent.Testing
ui::waveform,ui::now_playing); rendering output intentionally not unit-tested per ADR-009cargo clippy --all-targets -- -D warningsclean;cargo fmt --checkcleancargo build --releasecleannow_playing.rs313 lines,waveform.rs119 (both < 400); no cache logic added to over-budgetapp.rsHONKHONK_RENDERER=software): the waveform renders and the playhead animates without stutter/flicker — deferred to a human on a desktop session (CI/agent env is headless, no display/GPU). The automated reuse-contract tests + the headlessbenches/grid_render.rscanvas layout/draw path stand in for the per-frame-reuse acceptance.🤖 Co-authored by Claude Opus 4.8. Closes #131.
Summary by CodeRabbit