Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues remain around frame queue draining, repeated filesystem probes, and incomplete WebP animation detection.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR reduces idle CPU usage by making animated media visibility-aware and fixing conversation scroll pinning.
Changes:
- Pause or skip off-screen animated media.
- Correct bottom-scroll targeting to prevent repaint loops.
- Add regression tests for scrolling and animation behavior.
File summaries
| File | Summary | Review notes |
|---|---|---|
src/ui/picker.rs |
Adds visibility-aware sticker playback. | Cache animation classification and improve WebP detection to avoid repeated I/O and missed animations. |
src/ui/conversation.rs |
Updates media rendering and bottom pinning. | No final comments. |
src/demo.rs |
Adds settled-scroll regression coverage. | No final comments. |
src/animation.rs |
Adds visibility and pause controls with tests. | Ensure queued decoded frames are drained even when items become off-screen. |
Review details
Suppressed comments (2)
src/animation.rs:96
- The UI callers now skip
animation::frameentirely when a row or tile is off-screen, so a decode that finishes after the item is scrolled away is pushed intoInboxbut never drained here. If no other visible animation invokes this function, all decoded frames remain queued, potentially retaining substantial image memory until that item returns or another animation is rendered. Add a UI-thread poll/drain independent of a visible path, or otherwise service hidden rows without starting new decodes.
if !visible {
return Frame::Unavailable;
src/ui/picker.rs:735
movesonly searches the first 64 bytes for anANIMchunk, while imported WebP bytes are preserved as-is. A valid animated WebP with metadata before that chunk will be classified as static and sent tosticker_picture, which still uses egui's animated image loader, so that sticker can retain the repaint loop this change is meant to remove. Detect animation from the WebP container/decoder rather than this fixed prefix heuristic.
let animated = moves(path);
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@copilot please fix the merge conflicts in this pull request. |
|
@MrVoicer please fix the merge conflicts in this pull request. |
e243da5 to
4613930
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate issues in src/animation.rs remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/animation.rs:688
- This assertion only checks whether the last pass requested an immediate repaint. It does not verify the future repaint delay set by
request_repaint_after, so a paused animation could still schedule the continuous timer this test is meant to catch and the test would pass. Assert the root viewport'srepaint_delayis comfortably above the idle threshold, as the clipped-animation and UI idle tests do.
assert!(
!ctx.requested_repaint_last_pass(),
"a paused frame leaves the event loop idle: {:?}",
ctx.repaint_causes()
);
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
|
@copilot please review the latest changes in 56bee1a. |
|
@crmne Done. I rebased the branch onto current main and resolved the merge conflicts. The PR is mergeable now, and I also addressed the latest review feedback in 56bee1a. Quality, Ubuntu, macOS, and Windows ARM64 checks pass. The Windows default test suite also passes, but its all-features build is being cancelled by the workflow hard 40-minute timeout rather than a test failure. |
There was a problem hiding this comment.
🟡 Changes recommended
A critical animation-state issue and a moderate WebP classification issue remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/ui/picker.rs:1187
- This new probe only examines the first 64 bytes for the literal
ANIM.webp_bytespreserves existing WebP bytes, and a valid extended WebP can place metadata chunks beforeANIM, so such an animated sticker is classified as static and falls through tosticker_picture, which uses egui's generic image loader and can reintroduce animation-driven repaints. Parse the WebPVP8Xanimation flag or walk the RIFF chunks instead of relying on this fixed prefix.
/// Checks a WebP header for animation without decoding the file.
fn probe_motion(path: &Path) -> bool {
let mut head = [0u8; 64];
let Ok(mut file) = std::fs::File::open(path) else {
return false;
src/ui/picker.rs:1106
- This changes the user-visible playback rule to require both focus and hover, but
README.md:107-108still says animated stickers and GIFs play whenever their message or picker tile is merely visible. Please update that documentation to state the new focus-and-hover requirement, as described by this PR.
// Decode only visible animated stickers. Keep a still
// first frame until the focused pointer hovers the tile.
let animated = moves(ui.ctx(), path);
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
| if !animate { | ||
| playing.animating = false; | ||
| return Frame::Ready(playing.frames[0].0.clone()); |
Summary
This complements the scroll-loop fix merged in #33 by removing the remaining animation-driven repaint paths.
Scope
PR #33 fixed the primary continuous scroll repaint loop responsible for the original near-100% idle CPU report. This rebased PR no longer changes conversation scrolling. It is limited to animated-media playback and picker behavior.
The intentional UI tradeoff is that an animation shows a still first frame until the pointer hovers it while ZapFast is focused.
Connected-session measurement
Measured with optimized v0.14 builds, an isolated copy of the same linked profile, and an established WhatsApp TLS connection:
Each sample covered about 12 seconds. The focused difference is modest and can include normal UI variance; the regression tests establish the important invariant that paused and clipped animations do not schedule continuous frames.
Validation
cargo fmt --all --checkcargo clippy --locked --all-targets -- -D warningscargo clippy --locked --all-targets --all-features -- -D warningscargo test --locked --all-targets: 288 passed, 7 ignoredcargo test --locked --all-targets --all-features: 290 passed, 7 ignoredRUSTDOCFLAGS="-D warnings" cargo doc --locked --all-features --no-deps