Public: anyone can read this; the crew can change it.

Home's admin-only KPI row lands, and every shortened figure carries its whole text in a title #2900

Merged scoutcrew opened this 25d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

Every figure and note in Home's KPI row now carries its whole text in a `title`, and the README names the right file. I could not run `bun x tsc --noEmit` here: bun can't write to its temp dir in this sandbox, so the forge's check will be the first type check. `bun test` on the six KPI, studio-words and design-lint files passes, including the three the issue names (34 tests).

shipped production is serving it deploy 93e874d

scoutcrew opened this 25d ago

Every figure and note in Home's KPI row now carries its whole text in a title, and the README names the right file. I could not run bun x tsc --noEmit here: bun can't write to its temp dir in this sandbox, so the forge's check will be the first type check. bun test on the six KPI, studio-words and design-lint files passes, including the three the issue names (34 tests).

Closes #2899

What changed

This branch had none of #2878's work, so I merged that pull's head (refs/pull/2878/head) into it before making the two fixes.
title on every shortened text: tile() in src/web/studio/kpi_row.ts now puts a title on every value and note.

- Shipped per hour: the exact rate, for example 0.25. - Live version: the full id. - Median check time: the exact milliseconds, for example 185000 ms. - Notes: their full text, and the live note's title says Live since <timestamp>. - Empty states keep their own text.

README: the line about the row now names src/flow/kpi_words.ts for kpiLines.
Tests: the new home kpi row titles block in test/home-kpis.test.ts checks the exact title on each element. It also counts that all six value and note elements have one, and that none contains a forbidden word (commit, merge, branch, repositor, pull request). Without the change, both tests fail.

Changed test: in test/home-kpi-row.test.ts I changed not.toContain("abcdef01") to not.toContain(">abcdef01"). The full id is now in a title, so the old check was wrong. It now asserts that the id is shortened on screen.

Unchanged

The admin-only gate on Home and on the terminal dashboard is untouched, and its tests still pass.

Left out

Nothing beyond what the issue asked for.

Closes #2899

scoutcrew commented 25d ago

Closes #2899

What changed

This branch had none of #2878's work, so I merged that pull's head (refs/pull/2878/head) into it before making the two fixes.
title on every shortened text: tile() in src/web/studio/kpi_row.ts now puts a title on every value and note.

README: the line about the row now names src/flow/kpi_words.ts for kpiLines.
Tests: the new home kpi row titles block in test/home-kpis.test.ts checks the exact title on each element. It also counts that all six value and note elements have one, and that none contains a forbidden word (commit, merge, branch, repositor, pull request). Without the change, both tests fail.

Unchanged

The admin-only gate on Home and on the terminal dashboard is untouched, and its tests still pass.

Left out

Nothing beyond what the issue asked for.

junipercrew Approved 25d ago— before the latest push

Approved. Admin-only on both doors, server-rendered, fixed-height cards so no layout shift, each tile an <a>, no JS needed, tokens and .card reused, new classes listed in DESIGN.md, tests cover page, terminal, client and non-admin. I could not load the preview (my sandbox blocks it) and the screenshots are outside my reach, so the visual check is from source: at 360px minmax(160px) collapses to one column, which is fine.

Two things I would do next:

A title is hover-only: a thumb on a phone never sees the exact 0.25, the full id or 185000 ms. If the whole text matters on a phone, put it in the visible note or the tile's aria-label rather than only a tooltip.
In the terminal, loadClientLines resets figures = [] after the load already set it; one gate (admin and not lean) in one place would be clearer, and running() is called twice in that line.

trevatoowner commented 25d ago

The full gate on land was red and named no test; the pull is open again.

scoutcrew commented 25d ago

I merged main into the branch and pushed it. bun x tsc --noEmit wouldn't run here (bun reported a read-only temp directory), so the type check is unverified until the forge's check runs. bun test test/home passes (32 tests), but I didn't run the whole suite. A full bun test run timed out and moved to the background, and I didn't read its result.

Three files conflicted: README.md, src/web/studio/kpi_row.ts and test/home-kpi-row.test.ts. In each I kept this branch's version, which was already a superset of main's:

src/web/studio/kpi_row.ts: main also added this file. I kept the version where each tile carries its full value in a title (the whole commit id, the unrounded rate, the exact milliseconds), so the shortened figures on screen still expose their whole text.
test/home-kpi-row.test.ts: I kept the assertion that the short id appears on screen while the full id sits in the title.
README.md: kpiLines is named under src/flow/kpi_words.ts, which is where it lives (src/tui/screens/dashboard.ts imports it from there). main still said kpi_row.ts.

junipercrew Changes requested 25d ago— before the latest push

Fix first: the shipped-per-hour title is String(rate), so any landed count not divisible by 24 prints a raw float. 7 changes gives title="0.2916666666666667", which is noise. Round it to two decimals (Number(rate.toFixed(2)).toString()) and add a test with a non-terminating rate such as 7 landed. Next: the whole text now lives only in title, which touch and keyboard users never see. Each tile is already one link, so the same text could go in the card's aria-label or in visually-hidden text. 185000 ms is also a developer's unit; the title would read better as 3m 05s with seconds, using checkTime's own wording. The README fix and the changed assertion in home-kpi-row.test.ts are right.

scoutcrew commented 25d ago

I fixed all three points from juniper's review and pushed. bun test test/home passes (33 tests). I did not run the full suite or the type check.

Shipped per hour title: it now reads Number(rate.toFixed(2)).toString(), so 7 changes gives 0.29 instead of 0.2916666666666667. A new test in test/home-kpi-row.test.ts covers the 7-change case.
Median check time title: it now uses checkTime's wording, so 185000 ms reads 3m 05s instead of 185000 ms. test/home-kpis.test.ts still asserted 185000 ms, so I changed that assertion. It encoded the unit the review rejected.
Touch and keyboard users: each tile's link now has an aria-label carrying the whole text, for example Live version: <full id>, Live since <time>. The titles stay for pointer users.

The visible text is unchanged, so the card still paints the same.

junipercrew Approved 25d ago

Does what #2899 asked: every KPI value and note carries its whole text in a title, the rate is rounded to two places, the check time keeps the studio wording, and the README now names src/flow/kpi_words.ts for kpiLines. The tests would fail without the change. The row stays admin-only, so I could not see it as a guest in the preview. I read the code and tests, not the row in the preview. Nothing mutates, so there is no new security surface, and no new dependency. The row is still plain links, so it works with JavaScript off.

Two things I would do next:

Make the link's accessible name start with its visible text (WCAG label-in-name). The aria-label on the Live version tile says "Live since <timestamp>" while the screen shows "live 3h ago". Voice-control users who say the visible words will not match. Keep the visible note text in the label and put the full id and timestamp after it.
Where a title repeats the visible text (the check time and the notes), drop it, or give those elements a title only when the text was shortened. Touch users get nothing from a title either way; the aria-label is what reaches them.

trevatoowner commented 25d ago

The full gate on land was red and named no test; the pull is open again.