feat(ui): add in-app notices#185
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces a generalized in-app notice system replacing the ad hoc ChangesNotice queue feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AudioEvent
participant HonkHonk
participant NoticeQueue
participant Subscription
participant NoticeLayer
AudioEvent->>HonkHonk: Error / SourceFirstRun
HonkHonk->>NoticeQueue: push(Notice, Instant::now())
Subscription->>HonkHonk: NoticeTick(Instant)
HonkHonk->>NoticeQueue: expire(now)
HonkHonk->>NoticeLayer: view_notice_layer(notices)
NoticeLayer->>HonkHonk: Message::DismissNotice(id)
HonkHonk->>NoticeQueue: dismiss(id)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Persistent error notices never auto-expire, and the queue had no cap, no dedup, and rendered in a plain Column. A re-emitting fault (a broken sink honked repeatedly, device churn) grew the queue and the on-screen stack without limit, pushing notices and their close buttons off-screen with no way to clear them. - Coalesce identical notices (same level/title/body), refreshing the expiry in place instead of stacking duplicates. - Cap the queue at NoticeQueue::MAX_NOTICES, evicting the oldest beyond it. - Wrap the toast stack in a height-bounded scrollable so overflow stays reachable and dismissable. Addresses the CRITICAL flagged in the #185 review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/ui/notice.rs (1)
132-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests only assert
Some/None, not rendered content — light coverage against the "no view-rendering tests" intent.Both tests just check that
view_notice_layerreturnsSome/Nonebased on queue state rather than exercising app-state behavior directly onNoticeQueue(which is already covered thoroughly insrc/app/notices.rs). Consider whether this duplicate smoke-check pulls its weight here, given the guideline to keep tests focused on audio engine/config/library/app-state and avoid Iced view-rendering tests.🤖 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 `@src/ui/notice.rs` around lines 132 - 157, The tests in notice_layer_is_absent_for_empty_queue and notice_layer_builds_for_queued_notice only smoke-check view_notice_layer returning Some/None, which duplicates NoticeQueue coverage without validating rendered UI. Remove or replace these Iced view-rendering assertions in src/ui/notice.rs, and keep tests focused on app-state behavior in crate::app::notices::NoticeQueue or other non-view logic instead.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.
Inline comments:
In `@src/app/notices.rs`:
- Line 76: cargo fmt is failing in this file, so reformat the affected sections
to match rustfmt expectations. Update the long find closure and the multi-line
assert_eq!/assert! blocks in the notice-related tests and helpers so they follow
standard formatting, using the existing notice handling code around the
coalescing logic in the relevant functions as the reference points. After
adjusting those spots, run cargo fmt again to ensure the file passes the
formatting check.
- Around line 75-97: The notice coalescing logic in `NoticeQueue::push`
refreshes `expires_at` in place but leaves the existing entry at its original
deque position. Update the `push` path so that when
`self.entries.iter_mut().find(...)` matches an existing notice, you remove that
`QueuedNotice` and reinsert it at the back with the refreshed expiry, keeping
recency aligned with the cap eviction behavior.
---
Nitpick comments:
In `@src/ui/notice.rs`:
- Around line 132-157: The tests in notice_layer_is_absent_for_empty_queue and
notice_layer_builds_for_queued_notice only smoke-check view_notice_layer
returning Some/None, which duplicates NoticeQueue coverage without validating
rendered UI. Remove or replace these Iced view-rendering assertions in
src/ui/notice.rs, and keep tests focused on app-state behavior in
crate::app::notices::NoticeQueue or other non-view logic instead.
🪄 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: 711fe8b9-6901-4bd9-84aa-5ceabb9a5fd1
📒 Files selected for processing (5)
src/app/mod.rssrc/app/notice_tests.rssrc/app/notices.rssrc/ui/mod.rssrc/ui/notice.rs
A re-emitted notice refreshed its expiry in place but kept its original deque position, so oldest-first cap eviction could victimize the most actively re-emitting notice while stale ones survived. Coalescing now moves the refreshed notice to the back, aligning recency with eviction. Also applies the cargo fmt cleanup that was failing the CI lint job. Addresses CodeRabbit review on PR #185. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Remaining review items:
The CI lint failure was a pure |
Both advisories are DoS-via-untrusted-XML in quick-xml < 0.41. The sole consumer is wayland-scanner, a build-time proc-macro parsing vendored Wayland protocol XML; no untrusted XML is parsed at runtime, so neither issue is reachable in the shipped binary. wayland-scanner 0.31.10 (latest) still pins quick-xml ^0.39 — drop these ignores when it moves to 0.41. Closes #192. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
anyhow 1.0.102 is flagged unsound (Error::downcast_mut UB after Error::context); 1.0.103 is the patched release already on main. Regenerates the Flatpak cargo-sources.json for the lock change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…opped) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…opped) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # deny.toml
Closes #156
Summary
Notice/NoticeQueuemodel for info, warning, and error messages.source_noticestorage path with the general notice mechanism.Design decisions
src/app/and the Iced component insrc/ui/, so view code stays presentational and business logic stays in the MVU update path.AudioEvent::Erroras a persistentAudio errornotice while preserving the existing structured log call.Test plan
cargo fmt -- --checkcargo test noticecargo testcargo clippy -- -D warningsSummary by CodeRabbit
New Features
Bug Fixes