diff --git a/REPAIRS.md b/REPAIRS.md index 5a199ca..ea5e416 100644 --- a/REPAIRS.md +++ b/REPAIRS.md @@ -94,20 +94,36 @@ one commit. ### The Ora note runs two sentences together on screen - **Found by:** C3, on `chunk-c3`, while merging `origin/main` (`07834fa`). -- **Not claimed, and deliberately not fixed here.** It is a one-character change - in a line C3 also edits, which is exactly the shape R3 warns about: a shared - defect buried in a feature diff cannot be reviewed or reverted on its own. +- **Left open by C3, deliberately.** It is a one-character change in a line C3 + also edits, which is exactly the shape R3 warns about: a shared defect buried + in a feature diff cannot be reviewed or reverted on its own. +- **Claimed by:** `ui-improvements-post-refactor`, at base `98177fc`. - **Symptom:** in `settings/page.tsx`, `{SETTINGS_SYSTEM_CONTRIBUTES.ora}` is - followed by ` Switching it on sends...` on the same line, and JSX drops that - leading space. The rendered note reads + followed by ` Switching it on sends...` on the same line, and the leading + space is dropped. The rendered note reads "...you have to switch on.Switching it on sends...", with the DOM showing `switch on.Switching`. Introduced by S9 (#93); no check reads rendered copy, so CI is green on it. -- **Scope:** the only `{expr} Text` pair in that file, and the file uses `{" "}` - nowhere, so this is a one-off rather than a pattern. -- **Fix:** `{SETTINGS_SYSTEM_CONTRIBUTES.ora}{" "}` — or move the following word - onto its own line, which is what makes JSX keep the gap. Worth a look at S9's - other screens for the same pair before closing it. +- **Cause — not what the symptom looks like.** "JSX drops a leading space" is + not true, and a session that believes it will go looking for the wrong thing. + JSX keeps the leading space on the first line of a text node; four probe + routes against this app's own toolchain confirmed it. What drops the space is + an **HTML entity elsewhere in the same text node** — here `Ora's`, two + lines further down. Same paragraph with the entity spelled out as `Oras` + keeps its space; with `'` it loses it. It is an SWC behaviour, and it + needs no newline: a single-line `{X} Ora's` loses the space too. +- **Only leading whitespace is affected.** A trailing space before an + expression survives the entity — `previous {rangeDays} days.` in + `pages/[id]/page.tsx` renders correctly and is NOT a defect. Recorded because + it is the first thing a sweep turns up. +- **Scope — swept, one site.** Parsing every `.tsx` for a text node that + carries an entity AND begins with a mid-line space next to an expression + returns exactly this one. So a one-off in fact, though not for the reason + first recorded: the file does use `{" "}`, three lines below the defect, which + is why the retention sentence beside it has always rendered correctly. +- **Fix:** `{SETTINGS_SYSTEM_CONTRIBUTES.ora}{" "}` with the sentence moved to + the next line. Verified in the rendered DOM, not just the diff: + `switch on. Switching`. ## Landed diff --git a/src/app/(app)/issues/page.tsx b/src/app/(app)/issues/page.tsx index 2651b38..2cff8dc 100644 --- a/src/app/(app)/issues/page.tsx +++ b/src/app/(app)/issues/page.tsx @@ -2,14 +2,19 @@ import { useMemo, useState } from "react"; import { useRouter, useSearchParams } from "next/navigation"; +import Link from "next/link"; import { DEFAULT_ISSUE_SORT, + DEFAULT_SORT_DIRECTION, ISSUE_SORTS, ISSUE_SORT_LABEL, parseIssueSort, + parseSortDirection, + reverseDirection, useIssuesView, useStore, type IssueSort, + type SortDirection, } from "@/components/store"; import { COUNTED_QUEUES, DESTINATION_LABEL, DESTINATION_PATH, QUEUE_LABEL, parseQueue, type Queue } from "@/lib/vocabulary"; import { PageHeader } from "@/components/page-header"; @@ -54,10 +59,44 @@ import { WATCH_EMPTY } from "@/lib/watch-copy"; * another, and neither is a preference worth storing. */ -/** Column headers, drawn on the same six tracks as the rows below them. */ -const COLUMN_HEADERS = ["State", "Diagnosis", "Scope", "Confidence", "Impact", "Effort"] as const; +/** Which way round the active column currently reads. */ +const DIRECTION_GLYPH: Record = { asc: "\u2191", desc: "\u2193" }; +const DIRECTION_WORD: Record = { asc: "ascending", desc: "descending" }; -function ColumnHeaders() { +/** + * Column headers, drawn on the same six tracks as the rows below them — and the + * sort control for the column each one heads. + * + * The header IS the sort rather than a caption above one: + * + * - The label comes from `ISSUE_SORT_LABEL`, so a column and the Sort menu + * read one map and cannot end up calling the same ordering two things. + * - The first click sorts the column in ITS OWN default direction rather than + * a uniform descending — a triage list wants the lifecycle from `new` and + * effort from the cheapest. The second click reverses it, and the arrow + * says which way it currently reads. + * + * Newest and What changed head no column: there is no date column to head. The + * menu is what keeps them reachable, which is why it stays. + */ +const COLUMN_SORTS = [ + "state", + "cause", + "pages", + "confidence", + "impact", + "effort", +] as const satisfies readonly IssueSort[]; + +function ColumnHeaders({ + sort, + direction, + hrefFor, +}: { + sort: IssueSort; + direction: SortDirection; + hrefFor: (next: { sort: IssueSort; dir?: SortDirection }) => string; +}) { return (
- {COLUMN_HEADERS.map((label, index) => ( - = 4 ? NUMERIC_CELL : TRUNCATE_CELL}> - {label} - - ))} + {COLUMN_SORTS.map((key, index) => { + const active = key === sort; + const numeric = index >= 4; + // An inactive column opens in its own default; the active one reverses. + const next = active ? reverseDirection(direction) : DEFAULT_SORT_DIRECTION[key]; + return ( + + {/* Numeric columns are right-aligned, so their arrow leads rather + than trails; the label still ends at the column edge. */} + {numeric && active ? : null} + {ISSUE_SORT_LABEL[key]} + {!numeric && active ? : null} + + ); + })}
); } @@ -91,21 +165,30 @@ export default function IssuesPage() { const queue = parseQueue(searchParams.get("queue")); const sort = parseIssueSort(searchParams.get("sort")); - const view = useIssuesView(queue, sort); + const direction = parseSortDirection(searchParams.get("dir"), sort); + const view = useIssuesView(queue, sort, direction); // The tail starts folded. Opening it is the one action this list offers, and // it is not a commitment to anything — see the note on the fold below. const [tailOpen, setTailOpen] = useState(false); const linkTo = useMemo( - () => (next: { queue?: Queue; sort?: IssueSort }) => { + () => (next: { queue?: Queue; sort?: IssueSort; dir?: SortDirection }) => { const params = new URLSearchParams(); params.set("queue", next.queue ?? queue); const nextSort = next.sort ?? sort; + // Naming a sort without a direction means "start it the way it reads by + // default" — which is how the menu behaves, and how the first click on a + // column behaves. Staying on the current sort keeps the current + // direction, so changing queue does not silently un-reverse the list. + const nextDir = next.dir ?? (nextSort === sort ? direction : DEFAULT_SORT_DIRECTION[nextSort]); if (nextSort !== DEFAULT_ISSUE_SORT) params.set("sort", nextSort); + // Only a direction that is not the sort's own default reaches the URL, so + // the common link stays short and a reversed one is explicit. + if (nextDir !== DEFAULT_SORT_DIRECTION[nextSort]) params.set("dir", nextDir); return pathFor(`${DESTINATION_PATH.issues}?${params.toString()}`); }, - [pathFor, queue, sort], + [pathFor, queue, sort, direction], ); // The same counts the tabs badge, stated in a sentence. One selector behind @@ -204,7 +287,7 @@ export default function IssuesPage() { /> - +
{view.groups.map((group) => ( diff --git a/src/app/(app)/settings/page.tsx b/src/app/(app)/settings/page.tsx index ec7462a..8203fe5 100644 --- a/src/app/(app)/settings/page.tsx +++ b/src/app/(app)/settings/page.tsx @@ -572,7 +572,8 @@ function ConnectedSystemsGroup({ disabled }: { disabled: boolean }) {

{EVIDENCE_SOURCE_LABEL.ora}

- {SETTINGS_SYSTEM_CONTRIBUTES.ora} Switching it on sends the live web address of each watched page to + {SETTINGS_SYSTEM_CONTRIBUTES.ora}{" "} + Switching it on sends the live web address of each watched page to Ora, whose scans are public: the result enters Ora's directory and anyone can read it. Webflow staging addresses are never sent.{" "} {/* Draft, pending legal review. Rendered rather than withheld: a diff --git a/src/app/globals.css b/src/app/globals.css index 3780386..042a69e 100644 --- a/src/app/globals.css +++ b/src/app/globals.css @@ -2730,3 +2730,145 @@ textarea:focus-visible, border: 0; } } + +/* ── The issues list row ────────────────────────────────────────────────── */ + +/* + * The row is a grid of cells, not one big link, because two of its cells are + * links to somewhere else and a nested anchor is invalid. The case link instead + * stretches its own hit area over the whole row, which keeps the row-sized + * click target without a row-sized announcement. + */ +.issue-row { + position: relative; + border-top: 1px solid var(--border-hairline); + background: var(--surface-card); + color: var(--text-body); +} + +/* A member row inside a remediation group. */ +.issue-row--nested { + background: var(--surface-page); +} + +.issue-row__open { + color: inherit; + text-decoration: none; +} + +/* The stretched hit area. Not a second link — the same one, made row-sized. */ +.issue-row__open::after { + content: ""; + position: absolute; + inset: 0; +} + +.issue-row:hover { + background: var(--surface-raised); +} + +.issue-row__open:focus-visible { + outline: none; +} + +/* The focus ring belongs on the row, because the row is what the link covers. */ +.issue-row:has(.issue-row__open:focus-visible) { + outline: 2px solid var(--focus-ring); + outline-offset: -2px; +} + +/* + * Above the stretched link, so they keep their own targets. Without the + * stacking context these sit under it and every click opens the case. + */ +.issue-row__page, +.issue-row .info-tip { + position: relative; + z-index: 1; +} + +.issue-row__page { + color: inherit; + text-decoration: none; +} + +.issue-row__page:hover, +.issue-row__page:focus-visible { + color: var(--action-primary-ink); + text-decoration: underline; +} + + +/* Keeps the cause labels on one line down the list where a row has no second + layer to show. Same width as the control it stands in for. */ +.issue-row__tip-spacer { + flex: 0 0 auto; + width: 18px; + height: 18px; +} + +/* ── The information tip ────────────────────────────────────────────────── */ + +.info-tip { + flex: 0 0 auto; + display: inline-flex; +} + +.info-tip__button { + display: inline-flex; + align-items: center; + justify-content: center; + width: 18px; + height: 18px; + padding: 0; + border: none; + border-radius: 4px; + background: transparent; + color: var(--text-muted); + line-height: 1; + cursor: pointer; +} + +.info-tip__button:hover, +.info-tip__button[aria-expanded="true"] { + background: var(--surface-input); + color: var(--text-body); +} + +.info-tip__button:focus-visible { + outline: 2px solid var(--focus-ring); + outline-offset: 1px; +} + +.info-tip__panel { + /* + * There is no width token to reach for: this is the app's first tooltip, and + * the only measures the stylesheet had were `68ch` for running prose — three + * times this, and meant for a paragraph that owns its column. 20rem is the + * width at which a sentence or two breaks into a shape you take in at a + * glance rather than read across. Named so the next tip does not pick its + * own number. + */ + --info-tip-max-width: 20rem; + + position: fixed; + z-index: 120; + /* As wide as the text needs, and no wider than the measure above. */ + width: max-content; + max-width: var(--info-tip-max-width); + padding: 9px 11px; + border: 1px solid var(--border-hairline); + border-radius: 8px; + background: var(--surface-card); + box-shadow: var(--shadow-popover); + color: var(--text-body); + /* The row is a single nowrap line; the panel is prose and has to wrap out of + everything the row set on it. */ + white-space: normal; + text-align: left; + text-transform: none; + letter-spacing: normal; + font-size: 12.5px; + font-weight: 400; + line-height: 1.5; +} diff --git a/src/components/case-pages.tsx b/src/components/case-pages.tsx index 23b97d7..d5aaf8d 100644 --- a/src/components/case-pages.tsx +++ b/src/components/case-pages.tsx @@ -1,6 +1,7 @@ "use client"; -import { useState } from "react"; +import { useState, type CSSProperties } from "react"; +import Link from "next/link"; import { applicabilityOf, exclusionReasonOf, @@ -12,6 +13,7 @@ import { applicabilityActionLabel, type ExclusionReason } from "@/lib/vocabulary import { excludedNote, pagesCount } from "@/lib/case-copy"; import { formatImpact } from "@/lib/impact-format"; import { ExclusionReasonPicker } from "@/components/exclusion-reason-picker"; +import { useStore } from "@/components/store"; /** * The pages this case covers, and which of them it counts (4b). @@ -64,6 +66,7 @@ export function CasePages({ onInclude, impactByPage, }: CasePagesProps) { + const { pathFor } = useStore(); const [choosingFor, setChoosingFor] = useState(null); const included = includedPages(issue); const excluded = excludedPageIds(issue); @@ -92,6 +95,28 @@ export function CasePages({ const reason = exclusionReasonOf(issue, pageId); const impact = formatImpact(impactByPage?.[pageId] ?? 0); const path = pagePaths?.[pageId]; + const label = pageTitles[pageId] ?? path ?? pageId; + /** + * Only a page the store still knows gets a link. + * + * `issue.pageIds` is the case's own record of what it covers, and a + * page can leave the watchlist while the case that named it stays. + * Falling back to the raw id and linking it anyway would send the + * reader to a 404 — a row that does not navigate is the better of + * the two failures, so the name still renders, just as text. + */ + const known = pageTitles[pageId] !== undefined || path !== undefined; + const nameStyle: CSSProperties = { + display: "block", + fontSize: 13, + color: isExcluded ? "var(--text-muted)" : "var(--text-body)", + // The reading stays. Struck through says "not counted"; + // removing it would say "never measured", which is a lie. + textDecoration: isExcluded ? "line-through" : "none", + whiteSpace: "nowrap", + overflow: "hidden", + textOverflow: "ellipsis", + }; return (

-
- {pageTitles[pageId] ?? path ?? pageId} -
+ {known ? ( + + {label} + + ) : ( +
{label}
+ )} {isExcluded && reason ? (
{excludedNote(reason)} diff --git a/src/components/info-tip.tsx b/src/components/info-tip.tsx new file mode 100644 index 0000000..d29475e --- /dev/null +++ b/src/components/info-tip.tsx @@ -0,0 +1,192 @@ +"use client"; + +import { Info } from "@phosphor-icons/react"; +import { useCallback, useEffect, useId, useLayoutEffect, useRef, useState } from "react"; + +/** + * An icon that says more, on hover and on demand. + * + * This replaced a disclosure that expanded the row in place. The disclosure was + * correct in the abstract and wrong here: the row it sat in is itself a link, + * so the reader was asked to hit an 18px control inside a target that navigates + * away if they miss — and the reward for hitting it was the list reflowing + * under the pointer. A tip that appears over the row costs no layout and no + * accuracy. + * + * It is a TOGGLETIP rather than a tooltip, because it answers to a click as + * well as a hover, and the two behave differently on purpose: + * + * hover opens while the pointer is on the icon or the panel, and closes + * when it leaves both. + * click pins it open, and clicking again puts it away. Space and Enter are + * the same event on a real ` + + {/* + The live region is always mounted and the text is put INTO it when the + tip opens, which is what makes a screen reader read it out. A panel that + only appears in the DOM is a panel nothing announces: `aria-expanded` + reports that something opened, and never says what it said. + */} + + {open ? ( + + {text} + + ) : null} + + + ); +} diff --git a/src/components/issue-group.tsx b/src/components/issue-group.tsx index c928a87..3f3860b 100644 --- a/src/components/issue-group.tsx +++ b/src/components/issue-group.tsx @@ -7,9 +7,9 @@ import { ISSUE_ROW_GAP, IssueRow, NUMERIC_CELL, + PageScope, TRUNCATE_CELL, formatGroupImpact, - scopeLineOf, } from "@/components/issue-row"; /** @@ -49,7 +49,6 @@ export function IssueGroup({ group, basePath, pageTitles, nested = false }: Issu // The remediation's first step, which is what the shared fix starts with. It // is data, from the case; nothing is authored here. const fix = group.remediation.steps.find((step) => step.trim() !== "") ?? group.primary.title; - const scope = scopeLineOf(group.pageIds, group.primary.strategies, pageTitles); return (
@@ -74,8 +73,17 @@ export function IssueGroup({ group, basePath, pageTitles, nested = false }: Issu {fix} + {/* The pages link here for the same reason they do on a row: this is + the same column, and a header that names a page the reader cannot + open teaches them the column is not clickable. */} - {`${group.cases.length} cases · ${scope}`} + {`${group.cases.length} cases · `} + {/* The weakest member confidence, as a word. See the note in issue-row. */} diff --git a/src/components/issue-row.tsx b/src/components/issue-row.tsx index 3b2f201..183fa5c 100644 --- a/src/components/issue-row.tsx +++ b/src/components/issue-row.tsx @@ -1,11 +1,16 @@ +"use client"; + import Link from "next/link"; import type { CSSProperties } from "react"; import type { IssueCase } from "@/lib/issue-case"; +import type { Strategy } from "@/lib/types"; import { EFFORT_LABEL, formatGroupImpact, formatImpact } from "@/lib/impact-format"; -import { scopeLineOf } from "@/lib/scope-line"; +import { PAGE_SCOPE_NAME_LIMIT, SCOPE_SEPARATOR, deviceScopeOf, pageScopeNames, scopeLineOf } from "@/lib/scope-line"; +import { causeLineOf, diagnosisLineOf } from "@/lib/case-copy"; import { CONFIDENCE_LABEL } from "@/lib/vocabulary"; -import { caseHref } from "@/lib/paths"; +import { caseHref, pageHref } from "@/lib/paths"; import { StatusChip } from "@/components/status-chip"; +import { InfoTip } from "@/components/info-tip"; /** * One case, at the depth the list needs: state, diagnosis, scope, confidence, @@ -26,19 +31,37 @@ import { StatusChip } from "@/components/status-chip"; * the six columns line up down the whole list. Indentation is padding inside * the first cell, never a change to this template — a nested row that shifts * the tracks stops lining up with everything else. + * + * NO `max-content` HERE, and that is the point rather than an oversight. + * + * Every row is its own grid. `max-content` is resolved per grid, so a template + * that says "as wide as the content" makes each row as wide as ITS content: the + * effort column measured 29px on a row reading "Days" and 71px on one reading + * "No estimate", the state column 72px against 82px, and the two `fr` columns + * inherited the difference. Sharing the template gave six columns that lined up + * nowhere except by accident. The fix is tracks whose size does not depend on + * what is in them. + * + * The four fixed widths are the longest label each column can hold, measured + * from the rendered list and rounded up: nothing is clipped today, and anything + * longer ellipses through `TRUNCATE_CELL` rather than pushing the row wider. + * They are `minmax(floor, fixed)` rather than bare pixels so a narrow viewport + * takes the space back in the same order for every row — deterministic, because + * the template no longer reads the content. */ export const ISSUE_ROW_COLUMNS = [ - // state — a chip carries meaning, so it keeps a floor and is never clipped - "minmax(72px, max-content)", - // diagnosis — takes the space the others do not need, and ellipses + // state — a chip carries meaning, so it keeps its floor and is never clipped + "minmax(72px, 104px)", + // cause — takes the space the others do not need, and ellipses "minmax(0, 1fr)", - // scope — shrinks alongside the diagnosis, at a smaller share of it + // pages — shrinks alongside the cause, at a smaller share of it "minmax(0, 0.62fr)", - // confidence, impact, effort — content-sized while there is room, and able to - // give it back rather than push the row past the viewport - "minmax(0, max-content)", - "minmax(88px, max-content)", - "minmax(0, max-content)", + // confidence, impact, effort. Confidence is sized by its HEADER rather than + // its values: "Probable" is 52px and "CONFIDENCE" is not, and a column header + // that ellipses is a column nobody can identify. + "minmax(0, 100px)", + "minmax(88px, 96px)", + "minmax(0, 88px)", ].join(" "); export const ISSUE_ROW_GAP = 14; @@ -58,6 +81,7 @@ export const ISSUE_ROW_NEST_INDENT = 18; */ export { EFFORT_LABEL, formatGroupImpact, formatImpact }; export { scopeLineOf }; +export { causeLineOf, diagnosisLineOf }; /** One line, ellipsed. Applied to every free-text cell in the list. */ export const TRUNCATE_CELL: CSSProperties = { @@ -89,38 +113,127 @@ export interface IssueRowProps { nested?: boolean; } +/** + * The pages a case covers, each one a link to its own detail. + * + * The naming rule is `scope-line`'s, not this file's: two pages are named, more + * than two become a count. A count is not a page, so there is nothing to link + * on that branch — which is the honest outcome rather than a link to whichever + * page happened to be first. + */ +export function PageScope({ + pageIds, + strategies, + basePath, + pageTitles, +}: { + pageIds: readonly string[]; + strategies: readonly Strategy[]; + basePath: string; + pageTitles: Record; +}) { + const names = pageScopeNames(pageIds, pageTitles); + const devices = deviceScopeOf(strategies); + const named = names.length > 0 && names.length <= PAGE_SCOPE_NAME_LIMIT; + + return ( + <> + {named + ? names.map((page, index) => ( + + {index > 0 ? ", " : null} + + {page.title} + + + )) + : names.length > 0 + ? `${names.length} pages` + : null} + {names.length > 0 && devices ? SCOPE_SEPARATOR : null} + {devices} + + ); +} + export function IssueRow({ issue, basePath, pageTitles, nested = false }: IssueRowProps) { const impact = formatImpact(issue.impactMs); - // The case's own plain sentence where it has one. `fromRec` leaves this empty - // rather than authoring copy, and the stored title is what the source called - // it — the honest fallback, not a second diagnosis. - const diagnosis = issue.diagnosis || issue.title; + // What KIND of problem this is, in three or four words — the column a list of + // four dozen rows is actually scanned on. + const cause = causeLineOf(issue); + // The sentence itself, behind the tip rather than truncated at the column + // edge. `case-copy` owns the title fallback, because the cause sort and this + // both read it. + const diagnosis = diagnosisLineOf(issue); + // Where the classifier did not recognise the audit, `causeLineOf` already + // fell back to this same sentence — so there is nothing further to say, and a + // control that shows you the text you are looking at is worse than no + // control. The 18px is still reserved, so the labels start on one line down + // the whole list whether or not a row has more to give. + const hasSecondLayer = diagnosis !== "" && diagnosis !== cause; return ( - `. + + It could not stay one: the pages in it are links to somewhere else, and an + anchor inside an anchor is invalid — the browser closes the outer one and + the row silently becomes two links with a gap between them. The tip's + trigger is a button, which has the same problem. + + So the case link is a normal link on the cause, and `.issue-row__open` + stretches its hit area over the whole row in CSS. One focusable link per + row rather than a row-sized target that reads its six cells aloud, which + is the announcement the UX audit called out; the page links and the tip + sit above it and keep their own targets. + + The background and the rule above it are CSS rather than inline, because + an inline background beats a `:hover` rule in the stylesheet and the row + would never light up. + */ +
- - {diagnosis} + {/* NOT `TRUNCATE_CELL` on this cell or on the link inside it. The + stretched hit area is a positioned `::after` on the link, and + `overflow: hidden` anywhere above it clips that box back to the + cell — which is a whole-row target that only covers one column. + The truncation moves to the span around the text instead, where it + still ellipses and no longer clips anything. */} + + {hasSecondLayer ? ( + + ) : ( + - {scopeLineOf(issue.pageIds, issue.strategies, pageTitles)} + {/* The word, in the row's secondary text token — never a strength hue. @@ -139,6 +252,6 @@ export function IssueRow({ issue, basePath, pageTitles, nested = false }: IssueR {EFFORT_LABEL[issue.effort]} - +
); } diff --git a/src/components/object-detail-header.tsx b/src/components/object-detail-header.tsx index 8cb2b6c..bef47ff 100644 --- a/src/components/object-detail-header.tsx +++ b/src/components/object-detail-header.tsx @@ -22,12 +22,15 @@ import type { ReactNode } from "react"; * state + date what it is now, and since when * title the object in its own words, at most two lines * explanation one paragraph — why this is here, in prose - * actions stacked right, so they never separate the title from its text - * metadata BELOW everything, because it is reference and not the point + * actions right of the title, in a row, so they never separate the + * title from its text + * metadata directly under the explanation, in the title's own column * - * Metadata sits last on purpose. A strip of chips above the title makes the - * reader parse a taxonomy before they have been told what the problem is, and - * the taxonomy only means anything once they have. + * Metadata sits after the prose on purpose. A strip of chips above the title + * makes the reader parse a taxonomy before they have been told what the + * problem is, and the taxonomy only means anything once they have. It shares + * the title's column rather than spanning the header, so it stays beneath the + * sentence it qualifies instead of being pushed below the actions. */ export interface ObjectDetailHeaderProps { @@ -41,9 +44,11 @@ export interface ObjectDetailHeaderProps { /** One paragraph. If it needs two, one of them belongs in the body. */ explanation?: string; /** - * Stacked at the right. Unlike `PageHeader` this takes a node rather than + * In a row at the right. Unlike `PageHeader` this takes a node rather than * one action, because an object legitimately offers a decision and its * opposite — Accept and Dismiss are a pair, not a primary and a runner-up. + * A pair reads as a pair side by side; stacked, the second looked like a + * consequence of the first. */ actions?: ReactNode; /** Reference detail, rendered below the paragraph. */ @@ -116,14 +121,20 @@ export function ObjectDetailHeader({ {explanation}

) : null} + + {metadata ?
{metadata}
: null}
{actions ? (
) : null}
- - {metadata ?
{metadata}
: null} ); } diff --git a/src/components/store.tsx b/src/components/store.tsx index e48f147..460ebe5 100644 --- a/src/components/store.tsx +++ b/src/components/store.tsx @@ -16,6 +16,7 @@ import { byWorstMeasured, casesInQueue, groupByRemediation, + hasMeasuredImpact, type Effort, type IssueCase, type RemediationGroup, @@ -23,7 +24,18 @@ import { import { issueCasesFrom, lastRunAtOf } from "@/lib/issue-cases"; import type { CaseDecision, CaseDecisionRequest } from "@/lib/case-decisions"; import { partitionByImpact } from "@/lib/impact-format"; -import { APPLICABILITY_LABEL, COUNTED_QUEUES, ISSUE_ACTION_LABEL, type ExclusionReason, type Queue } from "@/lib/vocabulary"; +import { + APPLICABILITY_LABEL, + CONFIDENCES, + COUNTED_QUEUES, + ISSUE_ACTION_LABEL, + WORK_STATES, + type Confidence, + type ExclusionReason, + type Queue, + type WorkState, +} from "@/lib/vocabulary"; +import { causeLineOf } from "@/lib/case-copy"; import { normalizeNativeElementControls } from "@/lib/nativeElements"; import { localISODate } from "@/lib/ui"; import { withBasePath } from "@/lib/paths"; @@ -1250,7 +1262,16 @@ export { partitionByImpact }; /* ── Sorting ────────────────────────────────────────────────────────────── */ -export const ISSUE_SORTS = ["impact", "newest", "changed", "effort"] as const; +export const ISSUE_SORTS = [ + "impact", + "newest", + "changed", + "state", + "cause", + "pages", + "confidence", + "effort", +] as const; export type IssueSort = (typeof ISSUE_SORTS)[number]; /** Impact is the default, because it is the only one that ranks by consequence. */ @@ -1260,6 +1281,10 @@ export const ISSUE_SORT_LABEL: Record = { impact: "Impact", newest: "Newest", changed: "What changed", + state: "State", + cause: "Cause", + pages: "Pages", + confidence: "Confidence", effort: "Effort", }; @@ -1267,9 +1292,65 @@ export function parseIssueSort(value: string | null | undefined): IssueSort { return (ISSUE_SORTS as readonly string[]).includes(value ?? "") ? (value as IssueSort) : DEFAULT_ISSUE_SORT; } +/* ── Direction ──────────────────────────────────────────────────────────── */ + +export const SORT_DIRECTIONS = ["asc", "desc"] as const; +export type SortDirection = (typeof SORT_DIRECTIONS)[number]; + +/** + * The direction each sort reads in when you first ask for it. + * + * Not a uniform default, because "descending" is not a useful opening move for + * half of these: a triage list wants the lifecycle from `new`, effort from the + * cheapest, confidence from the strongest, and impact from the largest. These + * are the directions the sorts already had before they could be reversed, kept + * as the first click so nothing about the list changed under anyone. + */ +export const DEFAULT_SORT_DIRECTION: Record = { + impact: "desc", + newest: "desc", + changed: "desc", + state: "asc", + cause: "asc", + pages: "desc", + confidence: "asc", + effort: "asc", +}; + +export function parseSortDirection( + value: string | null | undefined, + sort: IssueSort, +): SortDirection { + return (SORT_DIRECTIONS as readonly string[]).includes(value ?? "") + ? (value as SortDirection) + : DEFAULT_SORT_DIRECTION[sort]; +} + +/** The direction a second click on an already-sorted column asks for. */ +export function reverseDirection(direction: SortDirection): SortDirection { + return direction === "asc" ? "desc" : "asc"; +} + /** Least work first, so a sort by effort surfaces what can be cleared today. */ const EFFORT_ORDER: Record = { minutes: 0, hours: 1, days: 2, unknown: 3 }; +/** + * State and confidence rank in the registry's own declared order, derived from + * its arrays rather than restated here. + * + * Both arrays are already ordered the way a reader wants to read them — the + * lifecycle from `new` to `dismissed`, and confidence from `confirmed` down to + * `unclear` — so indexing them IS the comparator. Copying the numbers out would + * be R1's F10 again: a rule stated in one place and duplicated in a constant + * that no longer changes with it. A state added to the registry lands in the + * right position here without this file being touched. + */ +const indexOrder = (values: readonly T[]): Record => + Object.fromEntries(values.map((value, index) => [value, index])) as Record; + +const STATE_ORDER: Record = indexOrder(WORK_STATES); +const CONFIDENCE_ORDER: Record = indexOrder(CONFIDENCES); + /** The calendar day of an ISO stamp, for comparing a detection to a run. */ const dayOf = (iso: string): string => iso.slice(0, 10); @@ -1286,12 +1367,29 @@ export function sortRemediationGroups( groups: readonly RemediationGroup[], sort: IssueSort, lastRunAt?: string, + direction: SortDirection = DEFAULT_SORT_DIRECTION[sort], ): RemediationGroup[] { + // Applied to the PRIMARY key only. Everything after the first `||` is a + // tie-break, and a tie-break that flipped with the header would reshuffle + // rows the reader did not ask to reorder. + const sign = direction === DEFAULT_SORT_DIRECTION[sort] ? 1 : -1; const lastRunDay = lastRunAt ? dayOf(lastRunAt) : undefined; const inLastRun = (group: RemediationGroup): number => lastRunDay && group.detectedAt && dayOf(group.detectedAt) >= lastRunDay ? 0 : 1; const byNewest = (a: RemediationGroup, b: RemediationGroup) => b.detectedAt.localeCompare(a.detectedAt); + + /** + * Rule 18, split out of `byWorstMeasured` so it can sit OUTSIDE the sign. + * + * Reversing "largest saving first" has to give "smallest MEASURED first", not + * "no reading first". An absent measurement is not a small one, and a + * direction toggle is not permission to rank it as zero — which is exactly + * what negating the combined comparator would have done. + */ + const measuredFirst = (a: RemediationGroup, b: RemediationGroup) => + Number(hasMeasuredImpact(b.impactMs)) - Number(hasMeasuredImpact(a.impactMs)); + const byImpactSize = (a: RemediationGroup, b: RemediationGroup) => b.impactMs - a.impactMs; // The id tie-break keeps the order stable when the sort key matches, so a // re-render never reshuffles equal rows. const byId = (a: RemediationGroup, b: RemediationGroup) => a.primary.id.localeCompare(b.primary.id); @@ -1301,10 +1399,22 @@ export function sortRemediationGroups( // ordered by a zero they never measured. Newest and What changed rank on a // date every case carries, so no measurement stands in for a missing one. const compare: Record number> = { - impact: (a, b) => byWorstMeasured(a, b) || byNewest(a, b) || byId(a, b), - newest: (a, b) => byNewest(a, b) || byId(a, b), - changed: (a, b) => inLastRun(a) - inLastRun(b) || byNewest(a, b) || byId(a, b), - effort: (a, b) => EFFORT_ORDER[a.effort] - EFFORT_ORDER[b.effort] || byWorstMeasured(a, b) || byId(a, b), + impact: (a, b) => measuredFirst(a, b) || sign * byImpactSize(a, b) || byNewest(a, b) || byId(a, b), + newest: (a, b) => sign * byNewest(a, b) || byId(a, b), + changed: (a, b) => sign * (inLastRun(a) - inLastRun(b)) || byNewest(a, b) || byId(a, b), + state: (a, b) => sign * (STATE_ORDER[a.state] - STATE_ORDER[b.state]) || byWorstMeasured(a, b) || byId(a, b), + // The text the row actually shows, so the order a reader sees is the order + // they could have worked out from the screen. + cause: (a, b) => sign * causeLineOf(a.primary).localeCompare(causeLineOf(b.primary)) || byId(a, b), + // Broadest first: the count is the reason to look, and one fix covering six + // pages is the case this sort exists to surface. + pages: (a, b) => sign * (b.pageIds.length - a.pageIds.length) || byWorstMeasured(a, b) || byId(a, b), + confidence: (a, b) => + sign * (CONFIDENCE_ORDER[a.confidence] - CONFIDENCE_ORDER[b.confidence]) || byWorstMeasured(a, b) || byId(a, b), + // The effort tie-break keeps rule 18 inside each band in both directions: + // reversing to "hardest first" must not float an unmeasured finding to the + // top of its band. + effort: (a, b) => sign * (EFFORT_ORDER[a.effort] - EFFORT_ORDER[b.effort]) || byWorstMeasured(a, b) || byId(a, b), }; return [...groups].sort(compare[sort]); @@ -1339,7 +1449,11 @@ export interface IssuesView { * things a person should be able to link someone to, and neither is a * preference worth persisting. */ -export function useIssuesView(queue: Queue, sort: IssueSort): IssuesView { +export function useIssuesView( + queue: Queue, + sort: IssueSort, + direction: SortDirection = DEFAULT_SORT_DIRECTION[sort], +): IssuesView { const { recs, pages, performanceThresholds, caseDecisions } = useStore(); return useMemo(() => { // The decisions log is part of the derivation's input, not a filter applied @@ -1356,12 +1470,12 @@ export function useIssuesView(queue: Queue, sort: IssueSort): IssuesView { cases, counts: queueCountsOf(cases), inQueue, - groups: sortRemediationGroups(groupByRemediation(inline, at), sort, lastRunAt), - tail: sortRemediationGroups(groupByRemediation(tail, at), sort, lastRunAt), + groups: sortRemediationGroups(groupByRemediation(inline, at), sort, lastRunAt, direction), + tail: sortRemediationGroups(groupByRemediation(tail, at), sort, lastRunAt, direction), tailCases: tail, minimumSavingsMs, pageTitles: Object.fromEntries(pages.map((page) => [page.id, page.title])), lastRunAt, }; - }, [recs, pages, caseDecisions, performanceThresholds, queue, sort]); + }, [recs, pages, caseDecisions, performanceThresholds, queue, sort, direction]); } diff --git a/src/lib/__tests__/info-tip.test.ts b/src/lib/__tests__/info-tip.test.ts new file mode 100644 index 0000000..2d4b554 --- /dev/null +++ b/src/lib/__tests__/info-tip.test.ts @@ -0,0 +1,92 @@ +import { readFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; + +const moduleDir = path.dirname(fileURLToPath(import.meta.url)); +const source = readFileSync(path.resolve(moduleDir, "../../components/info-tip.tsx"), "utf8"); +const styles = readFileSync(path.resolve(moduleDir, "../../app/globals.css"), "utf8"); + +/** + * The tip's accessibility contract, asserted from its source. + * + * These are properties of the markup rather than of a render, which is the same + * shape as the object-header tests in `case-applicability.test.ts` and the same + * reason: there is no DOM in this suite, and the things worth protecting here + * are structural. Each one is a way the tip has already been built wrong once, + * or a way the next edit could quietly break it. + */ +describe("the information tip", () => { + it("activates from the keyboard because it is a real button, not because of a key handler", () => { + /** + * Space and Enter on a `