feat(web,analytics): /analytics needs-attention section

[jakubretajczykBD]

Summary

• Renders the /analytics "needs attention" section ([IMPL] Frontend — /analytics "needs attention" section (coverage gaps, stock at risk, stuck value) #1989): coverage gaps, stock at risk, and value stuck in failed syncs, mounted into the #1986 shell directly under the trust header.
• Consumes the already-shipped GET /analytics/needs-attention ([IMPL] Backend — "needs attention" aggregates: coverage gaps, stock at risk, value stuck in failed syncs #1983) as-is — no backend changes in this PR.
• Either the open categories render or a single all-clear line does, never both, per the design mockup's rule (docs/plans/mockups/analytics-ledger-2003.html, frame 02).
• Each open row deep-links into the flow that resolves it: coverage → the unified bulk-publish wizard (pre-filled with every gapped product/variant, plus the connection when unambiguous), stock at risk → the product detail page, failed syncs → the orders list filtered to the needs_attention health bucket.
• Ambiguous multi-connection cases (coverage items missing from different channels, or stock items with different buffers) fall back to a connection-agnostic headline rather than naming the wrong channel.
• The failed-sync total renders currency-neutral in both the mixed and non-mixed case — the DTO carries no currency field at all today (confirmed by the pre-implement gate), which is a documented interim pending Order-time FX rate snapshot + reporting-currency stamping #2049's reporting-currency stamping.

Branch basis

This branch is stacked on 1986-analytics-page-shell (draft PR #2115, itself based on 1985-order-analytics-read-model), not main — it mounts AnalyticsNeedsAttention into the AnalyticsPage shell #1986 ships, which doesn't exist on main yet. Merge order: #1985 → #1986 → this PR.

Docs

• Implementation plan: docs/plans/implementation-plan-analytics-needs-attention.md
• Pre-implement gate (verdict: READY): docs/plans/analysis/ANALYSIS-analytics-needs-attention.md

Test plan

• pnpm type-check (web) — clean
• pnpm lint (web) — 0 errors (pre-existing warnings elsewhere, untouched)
• pnpm test — left to CI per this session's convention
• Manual verification in browser once [IMPL] Frontend — /analytics route shell: page scaffold, date-range control, trust header #1986 lands

Also included: #1990 tech-review fixes (PR #2171, already merged to main)

This branch merges origin/1990-sales-analytics-trend-plan, so the KPI-strip/by-channel-table code from the now-merged #2171 lives here too. Since #2171 can no longer be amended, its outstanding tech-review findings are fixed in this PR instead:

• toExclusiveEndInstant anchored on UTC midnight instead of local midnight — matches the controller's UTC-parsed from, avoids an off-by-hours window depending on the client's timezone. Added a non-UTC-timezone spec.
• Sparkline aria-labels now derive from the actual selected date range instead of a hardcoded "last 7 days".
• Registered the analytics KPI card's 152px-min-height / 3-column-desktop geometry as a documented carve-out in docs/frontend-ui-style-guide.md (Density table + parity matrix) — previously undocumented, against the guide's own "never introduce a row height that isn't on this list" rule.
• Fixed a "Data order" planned-tag typo -> "Planned", and a raw-px .section-infotip font-size -> the rem token equivalent.

Closes #1989

[piotrswierzy]

Tech Lead review — #2120 (/analytics needs-attention section)

Clean FE slice: correct layering (pages → features → shared), barrel-only cross-feature import (useConnectionsQuery from '../../connections'), TanStack Query for server state, pure copy derivations isolated in lib/ with their own spec, tokens-only CSS, StatusBadge carrying text + dot (never colour alone), and the three deep-links verified against their real consumers (?health=needs_attention is in OrderHealthValues; ?productIds=/&variantIds=/&connectionId= match bulk-create-wizard-page.tsx). CI is green. One issue blocks.

---

🔴 BLOCKING

1. The headline asserts a channel over data that only covers a 20-item sample — features/analytics/lib/needs-attention-copy.lib.ts

NeedsAttentionService caps its preview arrays at DEFAULT_AGGREGATE_LIMIT = 20, while coverageGapsTotalCount / stockAtRiskTotalCount are the true totals. deriveCoverageHeadline checks items.every(... === 1) over the ≤20 sample and then renders that conclusion against totalCount:

412 variants missing from Allegro

when only the 20 sampled variants were verified to be Allegro's — the other 392 may sit on any channel. deriveStockHeadline has the identical shape for the shared-buffer branch (buffer 2 claimed for a total the sample cannot speak for).

This is the defect class the repo repeatedly refuses to ship (#2075's "a false statement about their own catalogue"; ADR-041 §54's never-silently-decline rule). Fix: only take the connection-named branch when items.length === totalCount; otherwise fall through to the connection-agnostic headline that already exists. One added conjunct in each sharesOne* predicate, plus a spec.

---

🟡 IMPORTANT

2. "Publish now" silently seeds ≤20 of N — analytics-needs-attention.tsx

Same root cause: the row says 412, the link pre-fills the sampled 20. The wizard is a review flow so nothing wrong gets published, but the operator clicks through, publishes, and the row still reads 412 with no explanation. At minimum say so in sub ("showing the first 20"), or point at a list surface that carries all of them.

3. Grammar bug pinned by a test — needs-attention-copy.lib.ts

The ambiguous branch singularises the noun but hardcodes the plural verb: 1 variant have a listing gap on at least one channel. needs-attention-copy.lib.test.ts: asserts that exact string, so the test locks the bug in. Reword to a verb-free form (… with a listing gap on at least one channel) and update the assertion.

4. en-US-pinned number formatting — formatCurrencyNeutral

docs/frontend-architecture.md § i18n states useNumberFormat() exists specifically to replace toLocaleString('en-US') so numbers follow locale. The helper is pure so the hook isn't reachable here — thread a BCP 47 tag in from the caller (getBcp47Locale) rather than hardcoding.

---

🟢 SUGGESTION

5. MAX_WIZARD_IDS = 100 is unreachable — the source arrays are already capped at 20 server-side. It also slices productIds and variantIds independently, which (if ever reached) could emit a variantId whose product was dropped. Either drop the constant or derive both from one filtered item list.

6. Coverage/stock hidden behind an order-ingestion gate — analytics-page.tsx. AnalyticsNeedsAttention sits in the else of connections.every(status === 'never-ingested'). Coverage gaps and stock-at-risk are listing facts, not order facts, so a fresh install with a full catalogue and no orders yet sees neither — arguably exactly when they matter most.

7. List semantics — .attention-list is divs; <ul>/<li> would give screen-reader users the item count for free, matching the guide's accessibility section.

---

Verdict

❌ Request changes — item 1 only. Items 2–4 should land in the same pass; 5–7 are optional. Nothing architectural is wrong here.

(Note: this targets 1986-analytics-page-shell-plan, not main — merge order #1985 → #1986 → this.)

[piotrswierzy]

Tech-lead review (/pr-review) — re-review after the fix round

Verdict: 🔄 Approve with changes — the prior BLOCKING and IMPORTANT findings are correctly fixed. One new IMPORTANT: the same false-channel claim the BLOCKING fix removed from the headline is still present in the deep link.

The fix round

The sample-vs-total BLOCKING fix is exactly right and well argued. items is a ≤20 preview while totalCount is the truth, so items.length === totalCount now gates every connection-named headline in both deriveCoverageHeadline and deriveStockHeadline, with the reasoning recorded in the module header and tied to the repo's own precedents (#2075, ADR-041 §54). Deriving productIds/variantIds from the same item list rather than two independent slices — so a variantId can't outlive its own product — is the right response to that suggestion too.

🟡 IMPORTANT

The connectionId deep-link parameter uses a weaker predicate than the headline, and can name a channel the copy deliberately refused to name.

analytics-needs-attention.tsx:

const missingConnectionIds = uniqueValues(
  summary.coverageGaps
    .filter((item) => item.missingFromConnectionIds.length === 1)
    .map((item) => item.missingFromConnectionIds[0])
);
if (missingConnectionIds.length === 1) params.set('connectionId', missingConnectionIds[0]);

Compare deriveCoverageHeadline, which requires all three of items.length === totalCount, items.every(len === 1), and one distinct id. The link requires only the last. So with 20 sampled gaps where 19 are missing from Allegro only and one is missing from both Allegro and Erli, the headline correctly falls back to "…with a listing gap on at least one channel" — and the link still pins connectionId=<Allegro>. Worse, the items.length === totalCount guard is absent entirely, so a 20-of-340 sample that happens to be uniform pins a channel for a population it cannot speak for.

The operator then lands in the bulk wizard pre-filled with a destination that the sentence above the button explicitly declined to assert. That is the same defect the BLOCKING fix removed, one function over. Reuse one predicate for both — have deriveCoverageHeadline return the resolved connection id (or null) alongside the copy, and build the link from that.

🟢 SUGGESTIONS

1. "Publish now" doesn't say it publishes a sample. The partial-sample disclosure is a — showing the first 20 of 340 suffix on the sub line, while the headline still reads "340 variants missing…" and the button still reads "Publish now". An operator who clicks, reviews 20 rows and submits has reasonable grounds to believe the 340 are handled. Carrying it into the label (Publish first 20) puts the caveat on the control that acts, not two lines above it.

2. formatCurrencyNeutral renders money as a bare number. "1,234.56 of orders never reached a destination" parses as a count on first read. The interim is documented and the reason is real (the DTO has no currency field), but a neutral unit marker — or leading with the order count — would keep the sentence unambiguous until #2049 lands. Worth noting that #2151/#2172 (in flight on this same stack) now carry currency and unconvertedCurrency on their own DTOs, so the fix for FailedSyncValueSummary has a shape to copy.

3. const checkedCount = 3; sits directly above a list of exactly three if blocks. Derive it, or the all-clear line will one day claim three checks after a fourth is added.

Positive observations

• The either/or rule (open items or the all-clear line, never both, and a resolved category renders nothing) is implemented as stated, with the mockup frame cited.
• Every open row deep-links into the flow that resolves it rather than into a list the operator then has to navigate — coverage → the unified wizard, stock → product detail, failed syncs → /orders?health=needs_attention. That is the difference between a dashboard and a work queue.
• The ambiguity fallbacks are the right shape throughout: connection-agnostic headline rather than a wrong channel, "buffers vary by channel" rather than one buffer, and mixedCurrency refusing to render a single total with the reason in the copy ("a single total would misrepresent this sum").
• useConnectionsQuery resolved once into a memoised Map lookup rather than per row, matching the ConnectionCell rule in the style guide.
• Loading / error / empty all rendered deliberately, with a real Retry.

Notes — merge mechanics

Two process points, both worth resolving before this merges:

• This branch carries #1990/#2171's already-merged code and its outstanding tech-review fixes. Bundling them is declared in the PR body, which is the right instinct, but it means those fixes — the toExclusiveEndInstant UTC anchoring in particular, which is a real correctness bug — are now gated behind #1985 → #1986 → this PR landing. A fix to merged main code should go to main directly; consider extracting them into a small standalone PR so they aren't held hostage by a four-deep stack.
• The base is stale: this PR's base sha is b9acec2 while #2098's head is 85be7cc on the same branch name. Merge the newer 1986-analytics-page-shell-plan in before assessing conflicts — analytics-page.tsx, features/analytics/index.ts and index.css are all touched by both.
• pnpm test and manual browser verification are both unchecked. The unit coverage here is good, but the deep-link URL construction — the subject of the IMPORTANT above — is exactly the part a browser pass would have caught.

[piotrswierzy]

Re-review (/pr-review) — 375dca5 → 55e7a09

Verdict: 🔄 One IMPORTANT still open. The currency suggestion was resolved better than I proposed; the deep-link finding is untouched.

Suggestion 2 — resolved, and the fix is better than the suggestion

I proposed a unit marker or leading with the count. The change removes the bare number entirely, and the reasoning is the stronger argument:

a bare number formatted with no currency symbol ("6,120.64 of orders never reached a destination") reads as a real, currency-denominated figure — exactly the false claim this repo's currency-neutral-total precedent exists to avoid making, just less obviously so than the mixed-currency case.

Both branches now lead with the count and differ only in the qualifier, which also collapses the mixed/non-mixed asymmetry that made the two headlines read as different kinds of fact. Keeping totalValue on the wire for other consumers while this headline declines to read it is the right split.

(The getBcp47Locale / useTranslation removal is a consequence of no longer formatting a number, not a locale regression — worth stating since it looks like one in isolation.)

🟡 IMPORTANT — still open: the deep link can name a channel the copy refused to name

Unchanged at analytics-needs-attention.tsx:105-116:

const missingConnectionIds = uniqueValues(
  summary.coverageGaps
    .filter((item) => item.missingFromConnectionIds.length === 1)
    .map((item) => item.missingFromConnectionIds[0])
);
if (missingConnectionIds.length === 1) params.set('connectionId', missingConnectionIds[0]);

deriveCoverageHeadline requires three conditions (items.length === totalCount, items.every(len === 1), one distinct id); the link requires only the third. Two live consequences:

• 19 of 20 sampled gaps missing from Allegro only, one missing from Allegro and Erli → the headline correctly falls back to "…on at least one channel", and the link still pins connectionId=<Allegro>.
• The items.length === totalCount guard is absent entirely, so a uniform 20-of-340 sample pins a channel for a population it cannot speak for — the exact reasoning the BLOCKING fix added to the headline.

The operator lands in the bulk wizard pre-filled with a destination the sentence above the button explicitly declined to assert. Fix is the same shape as before: have deriveCoverageHeadline return the resolved connection id (or null) alongside the copy, and build the link from that, so one predicate serves both.

Still open from the first pass

• Suggestion 1 — "Publish now" still doesn't say it seeds only the sampled ≤20; the caveat lives on the sub line while the button acts. Lower priority than the above.
• const checkedCount = 3; still hardcoded above three if blocks.
• Merge mechanics — this branch still carries #1990/#2171's already-merged code and its outstanding review fixes, including the toExclusiveEndInstant UTC correction. Those remain gated behind a five-deep stack. Extracting them to a small PR against main is still worth doing; note #2098 has since landed its own toUtcRangeInstants, so the two UTC-boundary fixes now need to be reconciled rather than merged blind.

[piotrswierzy]

Re-review (/pr-review) — 55e7a09 → c9dc74f

Verdict: ✅ Approve. The remaining IMPORTANT is fixed, exactly as proposed.

The deep-link predicate

deriveCoverageHeadline now returns CoverageHeadlineResult extends AttentionRowCopy with a connectionId: string | null, and the caller destructures it rather than re-deriving:

const { connectionId, ...copy } = deriveCoverageHeadline(...);
// `connectionId` is read from the headline's OWN resolution, never
// re-derived here — a separately-computed, weaker predicate could pin a
// channel into this link that the headline above explicitly declined to
// name (#2120 re-review, IMPORTANT).
if (connectionId) params.set('connectionId', connectionId);

One predicate, two consumers, and it inherits all three conditions (items.length === totalCount, every(len === 1), one distinct id) for free. The type-level doc is the part that will keep it fixed:

A caller building a deep link MUST read this field rather than re-deriving its own "single missing connection" predicate … the same false-claim-about-your-own-catalogue defect the sample-vs-total rule above exists to prevent, one field over.

Both surfaces now refuse to name a channel under the same conditions, which was the point.

Remaining, unchanged and minor

• "Publish now" still doesn't say it seeds only the sampled ≤20 — the caveat is on the sub line, the button is what acts.
• const checkedCount = 3; still hardcoded above three if blocks.

Neither blocks.

Merge mechanics — still the real risk here

This branch continues to carry #1990/#2171's already-merged code and its outstanding review fixes, including the toExclusiveEndInstant UTC correction. Since my last pass, #2098 landed its own toUtcRangeInstants covering the same local-day-in / UTC-instant-out conversion. Those two now overlap and need reconciling deliberately rather than merged blind — most cleanly by dropping the #2171 fix here in favour of #2098's, if they agree, or by naming which one is authoritative if they don't.

Stack position unchanged: base 1986-analytics-page-shell-plan (#2098), with #2014's migration-timestamp verification still gating the chain.

[piotrswierzy]

Tech-lead review — ✅ Approve

Scope: 33 files, +3862/−9. Needs-attention section, sales KPI view-model, copy helpers, GapMark, page wiring.

Two honesty rules that carry this PR

1. The sample-vs-total rule is the right fix and is correctly generalised. items is a preview capped at DEFAULT_AGGREGATE_LIMIT (20) while totalCount is the real total, so a connection-named headline is only sound when items.length === totalCount. Naming a channel off a sample that can't speak for the remainder is the same class of defect as #2075's "no categories match" and ADR-041 §54's silent decline — a confident, specific, false statement about the operator's own data. Falling through to the connection-agnostic headline on any partial sample is the correct degradation, and the docblock ties it to the precedent rather than treating it as a local judgment call.

2. FailedSyncValueSummary.totalValue is never rendered, including in the non-mixed case. This is the subtler half and it went the right way: a bare number with no currency symbol ("6,120.64 of orders never reached a destination") reads to an operator as a real denominated figure, so the non-mixed branch was making the same false claim as the mixed one, just less visibly. Always rendering the currency-agnostic count — and leaving totalValue on the wire for unrelated consumers rather than ripping it out — is the proportionate call.

Both are consistent with the currency discipline in #2151/#2172 below them in the stack, which matters because these figures share a page.

Also right

• Copy helpers isolated from render so wording can be revised without touching component tests (+181 / +191 spec lines across the two libs).
• GapMark distinguishes "a real figure with a caveat" from a whole-card planned treatment — those genuinely are different states and collapsing them would overstate what's missing.
• Feature-barrel discipline maintained; orders-list-page.test.tsx touched only for shared test-util additions. ✅

🟡 IMPORTANT — GapMark's tooltip is unreachable without a mouse

apps/web/src/features/analytics/components/gap-mark.tsx

<span className="gap-mark" title={title}>†</span>

The dagger is the element's entire content, so its accessible name is the glyph — screen readers announce "dagger" or nothing useful — and a native title on a non-interactive, non-focusable <span> never surfaces for a keyboard user. The reason the figure isn't backed by data is therefore mouse-only, which the style guide's Accessibility section rules out ("keyboard navigable … accessible tables and status labels", and the repo's general rule that a mark must not be the only signal).

Note this is not the .listing-cell__reason precedent (#2231), where title carries an overflow of text that is also rendered in full in the per-product panel — here there is no other surface carrying the reason.

Cheapest fix keeping the visual identical:

<span className="gap-mark" role="img" aria-label={title} title={title}>†</span>

tabIndex={0} would additionally make it focusable, but a native title still won't render on focus in most browsers — if the reason is worth showing on focus, the Tooltip primitive (shared/ui/tooltip.tsx, wraps @radix-ui/react-tooltip, hover and focus with a delay group) is the sanctioned route and is already in the catalogue. Given a dagger is a footnote marker rather than a control, aria-label alone is proportionate for this PR; the Tooltip upgrade is a fine follow-up.

Not blocking — it's an added affordance on a new surface, degrading to a rendered figure that is still correct.

Checks

• Pure libs in lib/, hooks in hooks/, page composes; no raw fetch(); features → shared only. ✅
• CSS additions token-driven (+249 in index.css), no raw hex spotted. ✅
• Style guide updated (+4). ✅

Approving, with the aria-label worth folding in before merge. Sits on #2098 → #2172 → #2151 → #2014.