feat(web,analytics): top products table with per-channel breakdown (#1991)

[jakubretajczykBD]

Summary

• Adds the /analytics top-products table ([IMPL] Frontend — /analytics top-products table with inline per-channel split #1991): one row per product, per-channel units split, revenue/units sort toggle, and a "Publish" affordance for channels the product isn't listed on yet.
• Fixes a labeling gap found while manually testing against seeded dev data: a channel absent from the per-product sales breakdown was always rendered Not listed, even when the product was genuinely listed there and simply had no sale in the selected date range. Now only a channel actually missing from missingFromConnectionIds gets the Not listed + Publish treatment; a listed-but-quiet channel renders the same real, full-weight 0 a channel with sales would.

Test plan

• pnpm --filter @openlinker/web type-check
• pnpm --filter @openlinker/web lint (0 errors)
• pnpm --filter @openlinker/web exec vitest run src/features/analytics/components/product-sales-table.test.tsx (8/8 passing)
• Manual verification in browser against dev stack

Closes #1991

🤖 Generated with Claude Code

[piotrswierzy]

Tech-lead review — 🔄 Approve with changes

Clean, well-reasoned FE slice. Dependency direction is correct (pages → features → shared, cross-feature via ../../connections / ../../products barrels only), no raw fetch, no raw hex in the new CSS, align: 'right' respects the #2023 DataTable carve-out, and the connections lookup is one batched useConnectionsQuery rather than per-row (#1996/#2027). The pure view-model split (top-products-view-model.ts + its unit test) and the header comments explaining why "Not listed" ≠ 0 are exactly the shape the docs ask for. No BLOCKING findings.

IMPORTANT

1. Units total contradicts the column the server sorted on — product-sales-table.tsx / top-products-view-model.ts
totalUnits() re-sums row.channels[], but the Revenue column reads row.revenue directly and the backend ranks by row.units (#2172). Two provenance rules in one row: if channels[] is ever narrower than the full split (it is assembled by a separate getProductChannelBreakdown read), the displayed total silently disagrees with the sort order the header arrow claims. Use row.units for the total — the server figure is the authoritative one — and keep the channel sum for the per-channel cells only.

2. The "Publish" affordance is not permission-gated — ChannelCell
This is a write affordance in the sense of docs/frontend-architecture.md § Access Control And UI Visibility: it should go through useWriteAccess + ReadOnlyLock (disabled-with-tooltip in demo mode, hidden otherwise), not render unconditionally. A demo/read-only viewer currently gets an unguarded CTA into the bulk-create wizard.

3. On touch, the AC's distinction disappears — index.css @media (hover: none)
That block sets .cell-not-listed__label { opacity: 0 } and shows the chip. So on mobile/tablet a not-listed channel renders only a "Publish" chip and the words "Not listed" are never visible — the very distinction #1991's AC asks for is desktop-only. On touch, show both (label + chip stacked or inline), not the chip alone.

4. Chip announces a toggle for a one-shot action — ChannelCell
shared/ui/chip.tsx is documented as the filter-bar primitive and hard-codes aria-pressed={active}, so this Publish control is exposed to AT as a toggle button that is permanently "not pressed". Use Button with className="button--xs" instead (style guide § Buttons). While there: buildPublishHref already computes a URL but it is only fed to navigate() — rendering it as an <a> gives middle-click / open-in-new-tab for free, and the channel cell is not inside the row anchor (linkifyFirstCell covers the first cell only), so nesting is not a concern.

5. analytics feature slug is missing from .eslintrc.js — grep -c analytics .eslintrc.js → 0.
docs/frontend-architecture.md § Feature Public Surface, step 2: a feature exposing a public barrel must be enumerated in both no-restricted-imports pattern groups (features/** and plugins/**) for every canonical subdirectory (analytics/api, analytics/hooks, analytics/components, analytics/lib, analytics/types) — otherwise the rule silently fails open and deep imports into this feature are unenforced. The barrel predates this PR (it arrives with the stacked base), so fix it wherever in the stack you prefer, but it should not reach main unregistered.

6. Merge ordering. This consumes GET /analytics/top-products from #2172, which is still open and itself stacked on #1987/#1985; this PR's base is 1990-sales-analytics-trend-plan. Land in stack order or /analytics renders the error state in production. Worth stating in the PR body.

SUGGESTIONS

• total, unresolvedProductCount and unconvertedOrderCount are fetched and never surfaced, and DEFAULT_LIMIT = 20 has no pagination — at minimum a "Top 20 of N" caption, so an operator knows the list is truncated. unresolvedProductCount is exactly the "handled explicitly, not dropped silently" signal #1988 built.
• .cell-not-listed hover rules key on bare tr:hover / tr:focus-within. Style guide § CSS Implementation Standard prefers explicit component classes — scope to .data-table__row:hover so a future non-DataTable <tr> can't inherit the swap.
• productsById is rebuilt on every render from a Map over productQueries; harmless at 20 rows, but useMemo keyed on query.data would match the surrounding code's posture.

Positives worth keeping

File headers that record the decisions (Revenue-vs-Net-sales, unconvertedCurrency fallback per ADR-040, why the chip is position: absolute and not a flex sibling, product-id-not-SKU as the join key) are unusually good and will save the next reader real time. The test that pins "listed-but-quiet renders a real 0" is the right regression to have written.

[piotrswierzy]

Tech-lead review (/pr-review)

Verdict: 🔄 Approve with changes — two IMPORTANT findings, both the same shape: this is the sole consumer of two backend fields that exist specifically so this table can avoid asserting something false, and neither is read.

Summary

Good slice. Correct folder shape and public barrel, the analytics slug added to both no-restricted-imports pattern groups per frontend-architecture.md § Feature Public Surface (with a comment saying why), pure view-model helpers with no React, TanStack Query for the read, and the write affordance gated through useWriteAccess + ReadOnlyLock — row two of the access-control table, which is the right primitive for an action rather than AccessGate.

The labelling fix described in the PR body is the substantive win, and the docblock argues it properly: absence from row.channels is ambiguous (no sale in range or not listed), so the rendering keys off missingFromConnectionIds, which is range-independent. A listed-but-quiet channel now renders a real, full-weight tabular 0. That is the distinction #1991's AC asks for.

🟡 IMPORTANT

1. coverageGapAvailable is never read — and this table is the reason it exists.

TopProductsService treats the coverage-gap enrichment as best-effort: on failure every row gets an empty missingFromConnectionIds and the response carries coverageGapAvailable: false. #2172 added that field with exactly this consumer in mind:

an empty missingFromConnectionIds on every row is otherwise indistinguishable from "listed on every channel", the opposite of the truth when the enrichment itself failed.

git grep coverageGapAvailable -- apps/web returns nothing: it is absent from top-products.types.ts and from the component. It is present on the DTO (top-products-response.dto.ts:162). So when the enrichment fails, isMissingFrom is false for every cell, no "Not listed" flag renders anywhere, and the table silently tells the operator every product is listed on every channel — the precise false claim the flag was built to prevent, delivered by the surface that was supposed to prevent it.

Add it to the types and suppress the "Not listed"/Publish treatment (or the whole channel-gap column) when false, with copy saying the check could not run.

2. unresolvedProductCount is typed but never rendered.

It reaches top-products.types.ts:44 and the test fixtures, and stops there. #1988's AC is "line items that cannot be resolved to a catalogue product are handled explicitly rather than dropped silently", and TopProductsService.resolveCatalog goes out of its way to diff the requested ids against what came back rather than letting a naive join shrink the page. That care ends at the DTO boundary: a row whose product no longer resolves renders with name: null/sku: null and nothing tells the operator why, while the count that would explain it goes unused. A single line under the table ("N products could not be resolved to a catalogue entry") closes it.

🟢 SUGGESTION

The header notes revenue is a reporting-currency figure with no VAT/returns netting, and the currency fallbacks are handled carefully — including the previous-era currency: null case. Worth also carrying #2172's newer caveat into the UI: this is line revenue (unit price × quantity), so it will not reconcile with the KPI strip's order-level revenue on the same page, and ranking is blind to unconvertedRevenue, so a product selling entirely in unstamped orders ranks at 0 and may be missing from the page altogether. Both are stated on the DTO now; the table is where an operator would notice the discrepancy.

Positive observations

• deriveChannelColumns uses first-seen order rather than sorted, so toggling revenue/units doesn't reshuffle the columns — and it unions missingFromConnectionIds as well as channels[], with the reason: a capable connection nobody on the page has sold on would otherwise never get a column, making its "not listed" flag unrenderable.
• The @media (hover: none) branch stacking label + action permanently, so the label-vs-action distinction isn't desktop-only. Easy to forget on a hover-swap pattern.
• The absolute-positioned action with the note that an opacity: 0 flex sibling still occupies layout and misaligns the label — that's a bug someone already hit, written down.
• The productId join-key note (Allegro sets sku = offer.id, so a SKU-keyed roll-up would split one product across channels) is the kind of thing that would otherwise be rediscovered in production.

Notes

• Five deep: base is 1989-needs-attention-plan (#2120), which still has an open IMPORTANT of its own, on top of #2098 → #2172 → #2151 → #2014. #2014's migration-timestamp verification gates the lot.
• mergeable_state: behind.
• Manual browser verification is still unchecked — and the hover/touch swap and the coverage-gap treatment are both things only a browser pass exercises.

[piotrswierzy]

Re-review (/pr-review) — af5a7c8 → 23649d1

Verdict: ✅ Approve. Both IMPORTANT findings resolved, each with a test naming the finding. One suggestion outstanding, minor.

IMPORTANT 1 — coverageGapAvailable now gates the flag

ChannelCell short-circuits to the real 0 before consulting isMissingFrom, which is the correct order — the enrichment's failure mode is an unreliable [] on every row, so the flag has to be suppressed rather than merely reinterpreted. The reasoning is written at the branch:

when the enrichment failed, missingFromConnectionIds is an unreliable [] on EVERY row, not evidence of being listed everywhere, so trusting it would render "Not listed" as a false claim

And the table says so rather than degrading silently: "Listing-coverage check unavailable — channel columns show sales only, never 'Not listed'." Reusing .data-table__footnote from ChannelSalesTable instead of inventing a pattern is the right call.

IMPORTANT 2 — unresolvedProductCount surfaced

A separate footnote, deliberately not folded into the coverage-gap message, with the reason: the two are independent facts (a product can fail to resolve regardless of whether the coverage-gap enrichment ran). That's correct — merging them would imply a causal link that doesn't exist.

The lessons.md entry is the most valuable thing in this delta

Worth calling out explicitly: the NULL-currency guard I asked for on #2172 was applied incorrectly at first, and you found it. unconvertedOrZeroTotal was a bare 'X IS DISTINCT FROM :p OR Y = 0' string, so splicing it into WHERE ${...} AND rec."currency" IS NULL parsed as X OR (Y AND currency IS NULL) — AND binds tighter — making the guard's left branch true for nearly every unstamped row and driving unconverted_currency to NULL far more often than the data warranted. Silently, with no error.

Two things about how that was handled are right:

• The fix is at the definition site ('(… OR …)'), not at the call sites that need it today — confirmed at both occurrences. The rule as written says why: "a sibling call site added later, or an existing one edited, inherits the same trap silently."
• It was caught by the int-spec against real Postgres, and the entry states plainly that a mocked-repository unit spec cannot observe operator precedence at all. That is the strongest argument yet for the int-spec coverage this stack has been carrying as unchecked boxes.

I asked for that guard without flagging the interpolation hazard, so the finding is entirely yours. The generalised rule — a const holding a raw-SQL boolean must be self-parenthesised — is worth more than the bug.

🟢 Still open (suggestion, unchanged)

The line-revenue caveat isn't surfaced in the UI. #2172's DTO now spells out that revenue is line revenue (unit price × quantity) that "will not sum to the order-level revenue reported by GET /analytics/sales", and that ranking is blind to unconvertedRevenue so a product selling entirely in unstamped orders ranks at 0 and may be absent from the page. Both facts live on the DTO; this table sits on the same screen as the KPI strip an operator would compare it against. A footnote in the same style as the two you just added would close it.

Notes

• Still five deep (base 1989-needs-attention-plan / #2120, which has one open IMPORTANT of its own). #2014's migration-timestamp check remains the gate for the chain.
• Manual browser verification still unchecked — the hover/touch swap and the new footnotes are browser-only behaviours.

[piotrswierzy]

Tech-lead review — ✅ Approve

Scope: 27 files, +2211/−32. Top-products table view-model + hook, date-range/trust lib extensions, page wiring, a lessons.md entry, and a two-line SQL fix in OrderLineItemRepository.

The SQL precedence fix is the most valuable thing in this PR — and it is in the wrong PR

-  'rec."reportingCurrency" IS DISTINCT FROM :reportingCurrency OR rec."totalAmount" = 0'
+  '(rec."reportingCurrency" IS DISTINCT FROM :reportingCurrency OR rec."totalAmount" = 0)'

The diagnosis is exactly right. ${unconvertedOrZeroTotal} AND rec."currency" IS NULL splices textually, AND binds tighter than OR, so the guard parsed as X OR (Y AND Z) instead of (X OR Y) AND Z. Because X was true for nearly every unstamped row, the COUNT(*) FILTER (…) = 0 arm failed even for a clean single-currency bucket and the whole CASE fell to ELSE NULL — unconverted_currency reading NULL far more often than the data warranted, with no error anywhere. That is precisely the "silently wrong, never throws" shape this read model is otherwise so careful about.

The lessons.md entry draws the right general rule (parenthesise at the point of definition, not at the call sites that happen to need it today, because a sibling call site added later inherits the trap silently) and the right testing rule (a mocked-repository unit spec cannot observe operator precedence; only an int-spec against real Postgres pins it). Both are worth having in the ledger.

🟡 IMPORTANT — move the fix down the stack to #2172. unconvertedOrZeroTotal is introduced by #2172, which sits two PRs below this one. As the stack currently stands, #2172 merges to main carrying the bug, and it is only corrected when #2191 lands — leaving a window where the shipped /analytics/top-products endpoint under-labels unconvertedCurrency for no reason anyone would think to look for. The fix is two characters per site plus the comment; cherry-picking it into #2172 (with the int-spec that catches it, per the Source line) costs nothing and removes the window. The lessons.md entry can stay here.

That is a stack-sequencing point, not a defect in this PR — the fix itself is correct and well-explained.

Also right

• top-products-view-model.ts kept pure and separately tested (+70), matching the sales-analytics-view-model / needs-attention-copy shape established below it.
• The date-range.lib and ingestion-trust.lib extensions land as additions with their specs extended in the same commit, so the #2098 contract (single conversion point through toUtcRangeInstants) isn't quietly bypassed by the new consumer.
• One hook per endpoint, feature barrel updated, page composes only. ✅

Correction to my own review of #2172

In that review I suggested guarding the empty-productIds case. getProductChannelBreakdown already returns [] before building a query — the guard exists at the repository, which is the right layer. Disregard that suggestion; the second one (documenting why the two calls are sequential) still stands.

Approving. Top of the stack: #2014 → #2151 → #2172 → #2098 → #2120 → #2191.

[jakubretajczykBD]

Re: tech-lead review — SQL precedence fix sequencing

The unconvertedOrZeroTotal parenthesization fix in this PR's e7b33027a was diagnosed correctly, but as noted, it belongs two levels down the stack in #2172, not here.

That fix is already present and pushed on #2172's branch (1988-top-products-analytics, commit 648a940a0fb41bb3a7024ad3ff5b73d3634aefeb, "fix(orders): parenthesize unconvertedOrZeroTotal in top-products currency guard") — landed there independently before this PR's own copy was written, since this branch had already diverged from that point in the stack.

Net effect: the same two-line fix currently exists in both PRs. It is idempotent (re-parenthesizing an already-parenthesized expression is a no-op), so there's no correctness risk from the duplication, but once #2172 merges and this branch rebases onto the updated stack, the local copy in order-line-item.repository.ts here becomes a redundant no-op diff and can be dropped at that point rather than carried to main twice.

Keeping the docs/lessons.md entry here per the review's suggestion — the general lesson (parenthesize shared SQL predicates at the point of definition; pin operator-precedence bugs with an int-spec against real Postgres, not a mocked unit spec) is not stack-position-specific and is fine to record once, in the PR where it was written up.

Stack: #2014 → #2151 → #2172 → #2098 → #2120 → #2191 (this PR).