Add Chart, StatusDot and ConnectAccountButton core components by mdon · Pull Request #669 · BeamLabEU/phoenix_kit

mdon · 2026-07-28T22:02:37Z

Adds three server-rendered core components — Chart, StatusDot,
ConnectAccountButton — plus an optional value_color on stat_card. All
zero-JS except the popup hook, theme-aware via currentColor, and LiveView-native
(assigns change → SVG re-renders).

Extracted from NordSwitch, which runs hand-rolled versions today. Core had no
chart primitives at all, so every consumer either hand-rolled SVG or pulled in a
JS charting library.

Nothing in the ecosystem consumes these yet, which is why the API renames
below were free to make.

Components

line_chart/1 — line/area over {x, y} points; step for interval data
(right-open steps), optional marker_x "now" line, auto or explicit domains,
:empty slot.
sparkline/1 — polyline over a list of numbers.
bar_chart/1 — vertical bars from a zero baseline, native <title>
tooltips, optional per-datum class, aligned labels.
status_dot/1 — semantic coloured dot + optional label + optional pulse.
Fills the gap between nothing and the domain-specific badges in Badge;
phoenix_kit_publishing hand-rolls exactly this today.
connect_account_button/1 — OAuth-popup account linking (distinct from
OAuthProvider, which is app sign-in), driven by a generic PopupLink hook.

What the review pass changed

The components were handed over deliberately untested. Four external review
rounds plus a test suite found, among others:

bar_chart scaled from max(value), so a negative value produced a
negative rect height — SVG discards those, so the bar silently vanished. An
all-negative series divided by an epsilon floor and threw the geometry to
astronomical coordinates.
One bad point took the page down.nil, a string, or a Decimal raised an
uncaught ArithmeticError mid-render — and Decimal is what Ecto hands you for
money, in a kit with a billing module.
connect_account_button used an inline onclick — CSP-hostile in a kit
that removed inline handlers everywhere else — whose return false cancelled
navigation unconditionally, including when the popup was blocked. That is
exactly when the documented "falls back to normal navigation" was meant to save
it. It is a hook now.
Every unconfigured button shared one DOM id, which is invalid HTML and
breaks phx-hook.
The moduledoc's security rationale was wrong — it claimed the local-path
check kept window.opener from third parties. The popup navigates on to the
provider and the opener reference survives. The doc now names the real
mitigation (COOP + postMessage).
Charts carried role="img" with no accessible name; status_dot conveyed
state through colour alone.

Rounds 2, 3 and 4 each found defects in the previous round's fixes — a fix
aimed at a narrow symptom kept re-creating the same class of bug on a path it
did not touch. The round-4 resolution was to fix the cause instead: a collapsed
domain scales to the centre (the same treatment a flat y series already got),
and a path that genuinely paints nothing renders a dot at the point's true
position rather than a fabricated full-width line across a range nobody measured.

API changes, made while they were still free

area_chart → line_chart (area_chart area={false}was a line chart)
a11y attr label → aria_label, so it stops colliding with bar data's label:
hook ConnectAccountPopup → PopupLink — nothing about it is OAuth-specific,
and hook names freeze once hosts copy the bundle

Tests

test/phoenix_kit_web/components/core/{chart,status_dot,connect_account_button,stat_card}_test.exs
plus test/js/popup_link.test.cjs, a Node harness for the hook's decision logic
(which clicks to intercept, where to place the popup, same-origin). Run via
mix test.js, wired into mix precommit, skips itself when node isn't installed.

The chart assertions check the outcome, not the markup shape — assert_visible_mark
rejects a path whose points coincide, assert_bars_within_viewbox rejects a rect
the browser would clip. Both were written after shape-matching assertions passed
on charts that painted nothing.

Full suite: 2310 tests, 0 failures; mix precommit clean.
Notes and known limits: dev_docs/2026-07-28-chart-statusdot-connectbutton-components.md.

Server-rendered SVG chart primitives (area/step chart with now-marker, sparkline, bars) — zero JS, currentColor theme-aware, LiveView-native. StatusDot: generic semantic dot+label (online/offline/live states). ConnectAccountButton: OAuth-popup account-linking pattern for the Integrations system. stat_card gains optional value_color for values whose colour carries information. Extracted from NordSwitch, which will adopt them on release; handoff notes for testing in dev_docs.

The three new components shipped untested by design — the handoff asked for exactly this pass. 121 component tests later, four real defects: - bar_chart scaled from max(value), so a negative value produced a NEGATIVE rect height. SVG discards those outright, so the bar silently vanished rather than rendering below a baseline, and an all-negative series divided by the 1.0e-9 floor and threw the geometry to astronomical numbers. Bars are measured from a zero baseline now, which is also what makes mixed-sign data readable. - A single data point produced a bare `M x,y` path, which is valid and paints nothing. One datum is a real case (the first slot of the day, a metric with one sample), so it holds its value across the x range — the same reading step mode already gave it. - The charts carried `role="img"` with no accessible name, which is an unlabelled graphic to a screen reader. There is an optional `label` now, rendered as both `<title>` and `aria-label`; unlabelled charts are explicitly marked decorative instead. - connect_account_button used an inline onclick. Beyond being CSP-hostile in a kit that removed inline handlers everywhere else, its `return false` cancelled navigation unconditionally — including when the popup was blocked, which is precisely when the documented "falls back to normal navigation" was supposed to save it. The button simply did nothing. It is a ConnectAccountPopup hook now, which preventDefaults only once window.open has returned a window, leaves modifier-clicks alone, and centres the popup on the active monitor. Two hardening changes on top: the button only accepts a local path, since the popup deliberately keeps `window.opener` for the callback to reload the opener, and its derived DOM id is slugified so a window name with spaces can't produce an id LiveView cannot look up. stat_card's value_color strips `;` so a computed colour cannot append a second declaration, and the attribute is spread conditionally because HEEx renders `style={nil}` as an empty `style=""`.

A reversed domain like x_domain={{10, 0}} drove the span negative, and the 1.0e-9 floor then turned that into coordinates around 1.0e12 — the chart disappeared off-canvas rather than drawing. Both axes normalise now. Unsorted data rendered as a zigzag that reads like a bug in the chart rather than in the caller's data; a line chart only goes left-to-right, so the series is sorted by x defensively.

The hook's real logic — which clicks to intercept, and where to place the popup — was unreachable from the Elixir suite. Both are now pure functions exported for Node, covering the cases that actually bite: modifier-clicks and middle-clicks must stay the user's own (intercepting them breaks open-in-new-tab), a blocked popup must fall through to plain navigation, and the geometry must centre on the monitor the browser window is on rather than flinging the popup to the primary display. Adds test/js + a `mix test.js` alias wired into precommit, mirroring the harness phoenix_kit_dashboards already uses. It skips itself when node isn't installed rather than failing precommit over optional tooling.

External review of the new components, applied. The two that mattered: One bad point took the whole page down. `data={[{0, nil}, …]}` raised an uncaught ArithmeticError mid-render, and so did any `Decimal` — which is what Ecto hands you for money, in a kit that ships a billing module. Points are normalised now: tuples or `%{x:, y:}` maps, Decimals converted, anything non-numeric dropped, and the `:empty` slot when nothing usable survives. A drawing primitive should degrade, not crash the LiveView. Every unconfigured connect_account_button shared one DOM id, because `window_name` defaulted to a constant. Two buttons on an integrations page — the moduledoc's own example — is exactly the colliding case, and phx-hook requires uniqueness. The default now derives from `href`, which is already required to be local and differs per provider, so distinct buttons get distinct ids and distinct popups for free. Accessibility: status_dot conveyed its state through colour alone, silent to a screen reader and indistinguishable to red/green colour-blind users. Unlabelled dots now carry a visually-hidden state name kept in step with the colour. (The previous test asserted that silence was correct, which is how it survived the first pass.) The pulse animation respects prefers-reduced-motion. API, while it is still free to change — nothing consumes these yet: area_chart is line_chart (`area_chart area={false}` was a line chart with a misleading name), the a11y attr is `aria_label` so it stops colliding with bar data's own `label:` key, the hook is the generic `PopupLink` since nothing about it is OAuth-specific, and all three charts accept `:rest`. Bars gained native `<title>` tooltips, a zero baseline when data spans both signs, per-datum `class`, a rendered `id`, and labels sized to their own slot instead of drifting to the container edges under `justify-between`. Gridlines got the `vector-effect` the line already had. A single sparkline value draws a flat line rather than nothing. The hook refuses cross-origin hrefs itself rather than trusting the Elixir guard — the bundle is copied verbatim into hosts and the hook name is public — and treats an already-closed popup as blocked. Also corrected a wrong security claim in the moduledoc: the local-path check is NOT what keeps `window.opener` from a third party. The popup navigates on to the provider and the opener reference survives, so the provider's origin can reach `opener.location` — reverse tabnabbing. The doc now says what actually mitigates it (COOP plus postMessage).

A computed value surfaced float noise straight into the tooltip: 0.1 + 0.2 rendered as "0.30000000000000004". Display is rounded and trailing zeros dropped, so whole numbers read as whole numbers and money reads as money; the geometry keeps full precision.

The handoff said "deliberately untested" and asked for a review pass; this records what that pass found and changed, what was deliberately not addressed, and where the next consumers are.

Second review sweep. The two worst findings were mine, both introduced by round 1: `normalize_domain/1` fell back to a constant `{0, 1}` for an unusable bound — which recreated, for a different input, exactly the bug the comment above it claims to fix. `x_domain={{0, nil}}` over a 0..1440 series produced coordinates around 691,200: finite, so nothing caught it, and off-canvas, so the chart was simply gone. It falls back to the data's own domain now. The documented sizing contract did not work, and the moduledoc example was itself the failing case: the wrapper had no height, so the svg's `h-full` resolved against an auto-height parent and the chart fell back to its viewBox ratio. The wrapper carries the height now, and bar_chart lays out as a column so an optional label row can't overflow the box the caller gave it. Also from the sweep: - Dropping a bad sparkline sample re-spaced the axis, so removing one point silently changed the SHAPE of the line rather than leaving a gap. Points are indexed before the reject, so survivors keep their x. - A flat sparkline sat on the floor while the line chart centred the same data — a steady metric read as "at its minimum". - The step tail emitted the viewBox width unconditionally, so data overshooting an explicit x_domain drew forward past the edge and then back leftward across the chart. - `label=""` — an ordinary result of `label={@device.name}` — left a dot with no accessible name at all: the empty string is truthy, so it rendered an empty visible span AND suppressed the hidden one. - bar_chart accepted only `%{value:}` maps, silently rendering a blank box for the tuple and string-keyed shapes line_chart takes. - An all-negative series drew no zero line, though its baseline sits at the top where nothing implies it; an all-zero series rendered nothing at all, since a zero-height rect paints nothing. - Tooltip rounding collapsed small magnitudes to "0" — a rate or a sub-cent price. There is a `value_format` fun now, which is also the only sane answer for units, currency and locale. - The popup clamped its position to 0, throwing it back to the primary display for anyone whose browser sits on a monitor left of or above it. - The COOP advice was wrong: `same-origin-allow-popups` is the value that KEEPS the opener relationship. The doc now says the only real fix is to stop using `window.opener` — open with noopener and let the callback broadcast on PubSub. - status_dot's colour and announced word were separate lookup tables free to drift; one resolver returns both. The vocabulary is gettext'd and overridable via `state_label`. - stat_card's `:rounded` attr had been declared and documented for a long time while the class was hardcoded. Seven tests passed for the wrong reason and now don't: the finite-number guard skipped "NaN" entirely (its regex excluded N and I) and contributed no numbers at all for a sparkline; the step assertion was satisfied by a non-step render; `=~ "error"` was satisfied by the `bg-error` class; and the hook's blocked-popup branch — the entire point of the round-1 rewrite — had no coverage until now.

Wiring up stat_card's long-dead :rounded attr introduced a subtler bug than the one it fixed: Tailwind scans SOURCE for literal class names, so an interpolated `rounded-#{@Rounded}` is invisible to it and the CSS is simply never generated. The scanner would have seen the bare string `rounded-`, which is not a class at all. The tokens map to written-out class names now, and the attr carries a `values:` list so a typo fails at compile time instead of silently rendering a square card.

Third review round found the previous round's fixes had their own defects: - A single data point painted nothing again under the default (auto) domain. The auto x-domain collapses x_min == x_max, so every x mapped to 0 and the 'held value across the range' fix produced M0,y L0,y — a zero-length segment. Degenerate x-spans now widen to the full width. - Flooring a zero bar's height to a 1px hairline left its y at the baseline, so the bar spanned baseline..baseline+1 — one pixel below the viewBox, and clipped away. Negative bars had the same shape. y is derived from the floored height now. - A :label without a String.Chars impl (a tuple, a map) raised Protocol.UndefinedError mid-render and took the page down over a tooltip. - The y padding was an absolute 1.0e-9, below one ULP past ~1e7, so large flat series lost their padding and sank onto the axis. It is relative now. Tests assert the outcome rather than the markup shape: assert_visible_segment rejects a path whose endpoints coincide, and assert_bars_within_viewbox rejects a rect the browser would clip.

Fourth review round, against the third round's fixes. Rather than patch the symptoms again, this fixes the cause. A collapsed domain has no left or right, so it now scales to the CENTRE — the same treatment a flat y series already got — and a path that genuinely paints nothing renders a dot at the point's true position. That replaces the previous 'widen a zero-length path to full width' hack, which had two failure modes of its own: it painted a full-width line for a datum outside an explicit x_domain, asserting a reading across a range that was never measured, and it collapsed any two points sharing an x to the first y — silently dropping the second value while still looking plausible. Also fixed: - A small negative value among large positives puts the baseline on the bottom edge, so the 1px hairline hung from height..height+1 and was clipped away. Bars are clamped into the viewBox on both sides now. - The y padding was magnitude-relative, which swamped the range: 1.0e12 counters differing by 100 flattened onto one row of pixels. It is a fraction of the range whenever there is one, and falls back to magnitude only for a genuinely flat series. - label_text/1 rescued only Protocol.UndefinedError, but to_string/1 raises ArgumentError for a keyword list — List.Chars is implemented for lists, it just fails on non-iodata. It also guarded the tooltip but not the rendered label, so show_labels turned the same datum back into a page crash. - Values at the float ceiling overflowed while being padded, raising mid-render.

PR #668 (SMTP transport settings + optional queue hook): - Fix the dialyzer failure that broke `mix precommit` — the sender-address check had an unreachable catch-all clause, so `mix dialyzer` halted with exit 2 and the new `test.js` alias step never ran. - Make SMTP `username`/`password` optional. The PR added `auth: never` for "relays that authenticate by IP" and `security: none` for a plaintext internal relay, but marking the login required made both unreachable: the form refused to submit and `connected?/1` refused to call the connection configured, so mail silently fell back to the built-in mailer. The probe stops forcing `auth: always` when there is no login to prove. - Persist blanks on the personal integrations form the way the website form does (drop for `:password` only), so a cleared CA bundle or timeout no longer keeps its old value behind an empty field. - Read `:type` softly on both save paths, matching the hardening applied to `setup_field/1` — a provider field map without it rendered fine and then raised KeyError on submit. - Reject an SMTP timeout with a unit ("30s", "30 minutes") instead of silently using the number `Integer.parse/1` found. - Document the five new SMTP error reasons on `deliver_via_integration/3`, and warn on `maybe_enqueue/2` that `skip_queue: true` still re-runs `intercept_before_send/2`. PR #669 (Chart, StatusDot, ConnectAccountButton): - Skip `mix test.js` when the glob matches nothing — `node --test` with no file arguments walks the whole CWD, deps/ and _build/ included. Reviews: dev_docs/pull_requests/2026/{668-smtp-transport-settings-queue-hook, 669-chart-statusdot-connectbutton-components}/CLAUDE_REVIEW.md Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Independent re-check of release 1.7.217 (d7008e0) against each review's findings: every claimed fix confirmed present in the released code with file:line evidence, gates re-run (mix quality.ci exit 0, format clean, touched unit suites + test.js green). Records the CHANGELOG gap: the #667 auth fixes ship in 1.7.217 with no changelog entry.

mdon added 11 commits July 29, 2026 00:59

Update the components doc to an after-action record
adc5c8d
The handoff said "deliberately untested" and asked for a review pass; this records what that pass found and changed, what was deliberately not addressed, and where the next consumers are.

ddon merged commit 7ebd76b into BeamLabEU:mainJul 28, 2026

Navigation Menu

Uh oh!

Add Chart, StatusDot and ConnectAccountButton core components - #669

Add Chart, StatusDot and ConnectAccountButton core components#669
ddon merged 11 commits into
BeamLabEU:mainfrom
mdon:main

mdon commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

mdon commented Jul 28, 2026

Components

What the review pass changed

API changes, made while they were still free

Tests

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants