Repository navigation
Show Call health as a card with a minute of ping and receive - #147
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
unRekable
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅ (0 critical, 0 warnings, 4 cosmetic suggestions) — read the full diff plus surrounding panel/chart code.
💡 Suggestions
- panel.rs:138 — the X axis spaces samples by ring index, not by time, so "The last 60 seconds" is really "the last 60 polls". Given a 1 s poll cadence that is fine, but a timestamp (or a short doc note) would make the label precise when mediad answers slowly for a stretch.
- panel.rs:435 — KPIs paint
0 ms,0 kb/sand0.0%in green before any measurement arrives (rtt_msisNone->unwrap_or(0)), which reads as perfect health in the first second of a call. A neutral color whileNonewould say "not yet". - panel.rs:251 — the chart footer shows the raw last sample while the Ping KPI above is steady-smoothed, so right after a spike the two numbers in one card can disagree for a beat. Harmless; aligning would be tidier.
- panel.rs:380 —
health.hints()is built twice here; the second call can reuse the first binding.
✅ Looks Good
- The 60-sample ring is exactly right: pops front only when at capacity, so the window is precisely a minute with no off-by-one and no unbounded growth; cleared on every end-of-call and error path.
- The
--shotseeding of the chart is feature-gated and returns before any real poll task starts — screenshot data cannot leak into real builds or real calls. - Degenerate cases are all handled: empty ring early-return, divide-by-zero-proof scaling, single-sample and all-zero data keep points inside the canvas.
- Compared against the old card, the rewrite drops nothing: channel/bitrate, ping/jitter/loss, sending/receiving/codec/path all survive, and the 288->320 width is consistent in both entry points.
- The ring test pins the exact off-by-one behavior (70 samples -> window 60 with correct head and tail), and the chart only paints while the card is mounted, so the cost-when-hidden is unchanged from before.
CI note / manual check: the failing Windows job is sending_echoes_once_and_failures_say_why timing out in chatd --test discord — this PR's diff is GUI-only with zero chatd/transport changes. I re-ran that exact test from a checkout of this branch: it passes (~4 min). Treat it as an unrelated Windows timing flake and re-run the job. I also verified the open checklist item in code: the 1 s poll feeds the chart ring regardless of card visibility, so the graph does fill over time — worth one real-network session to confirm visually and then tick that box.
| window.paint_quad(fill(Bounds::new(point(px(origin_x), px(y)), size(px(width), px(1.))), theme::hairline())); | ||
| } | ||
| let count = samples.len(); | ||
| let span = (count.saturating_sub(1)).max(1) as f32; |
There was a problem hiding this comment.
💡 Suggestion: x_at spaces samples by ring position, so the axis is sample-index rather than time — with a slow stretch of mediad responses, the "60 seconds" compresses silently. A timestamp on ChartSample (or a doc note) would make the label precise; low priority given the 1 s poll cadence.
There was a problem hiding this comment.
Done: samples carry Instant and the X axis is time-based.
| .mb(px(10.)) | ||
| .flex() | ||
| .gap(px(8.)) | ||
| .child(kpi("Ping", format!("{}", health.rtt_ms.unwrap_or(0)), "ms")) |
There was a problem hiding this comment.
💡 Suggestion: with rtt_ms still None, the unwrap_or(0) zeros render in the positive/green tone, which reads as "excellent connection" during the first second of a call. Remaining plain/neutral until the first real sample would match the old card's instinct.
There was a problem hiding this comment.
Done: unmeasured KPIs show — in soft text instead of green zeros.
| .type_micro() | ||
| .text_color(theme::text_soft()) | ||
| .child("−60s") | ||
| .child(format!("Now · {now_rtt} ms · {now_recv} kb/s")), |
There was a problem hiding this comment.
💡 Suggestion: the footer value comes from the raw last sample while the KPI above comes from the smoothed health.rtt_ms, so after a spike the card shows two different ping numbers for a beat. Reading both from the same source would keep them consistent.
There was a problem hiding this comment.
Done: chart footer uses the same smoothed ping (and current recv) as the KPI strip.
| .child(health::bars(grade, 14.)) | ||
| .child(div().type_ui().font_weight(theme::SEMIBOLD).text_color(theme::text()).child("Call health")), | ||
| ) | ||
| .children(health.hints().into_iter().take(1).map(|hint| { |
There was a problem hiding this comment.
💡 Suggestion: health.hints() is called twice in this render (with take(1) here, again below); building it once and reusing the binding is cleaner. Pure style today, but it also avoids a divergence if the two call sites ever stop matching.
There was a problem hiding this comment.
Done: hints are built once and reused.
| } | ||
|
|
||
| /// Append one poll to the chart, dropping the oldest past [`CHART_SECONDS`]. | ||
| pub(in crate::gui) fn push_chart(samples: &mut VecDeque<ChartSample>, stats: &CallStats) { |
There was a problem hiding this comment.
✅ Nice: popping the front only when the ring is full-before-push keeps the window at exactly 60 samples with no off-by-one, no unbounded growth, and VecDeque of a copy-able struct keeps it cheap. The test pins this precisely (70 samples -> correct head 10 / tail 69).
Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
--shotso screenshots are not empty).Screenshot
Test plan
cargo test -p tinyclient --features gui --bin tinyclient chart_keepscargo test -p tinyclient --features gui --bin tinyclient panel::tests--shotwithTINYCLIENT_SHOT_CALL=tiny_joined --mode tiny --tiny-menu stats