feat(statistics-deck): Month view (Room, Desk, Laptop) (#136) #147

Merged

gabogg merged 3 commits from feat/deck-month-view into master 2026-09-27 10:09:26 +00:00

Conversation 4 Commits 3 Files changed 5 +1869 -2

gabogg commented 2026-09-26 23:05:08 +00:00

Owner

Copy link

Closes #136

Problem

The statistics deck shell (#133, PR #144) has a Month tab, but it only showed placeholder panels. RFC #80 §4 defines what the Month view shows: six ranked KPIs and five ranked graphs, laid out separately for each of the three profiles, with the data-quality marker on per-day graphs.

Approach

• New view module app/static/js/src/ui/statistics_deck/views/month_view.js. It calls registerView('month', renderMonth) when imported, and index.html imports it next to the StatisticsDeck import. Everything that maps route JSON to KPIs or graph inputs is an exported pure function: mapMonthKpis (driven by a KPI spec table), comparisonLine, line, hasLastYear, monthDaySeries, runningTotals, previousTotalReason, weekdayAverages, mapEntrances, peakDay, barTiers, and the config builders perDayBarConfig, runningTotalConfig and weekdayConfig.
• Layouts (MONTH_LAYOUT), each smaller profile keeping the top of the ranking:
  • Room: 6 KPIs. Lead row: the month day by day (2/3 of the width) and the running total (1/3). Supporting row: peak per day, average by weekday, entrance share.
  • Desk: 4 KPIs. The day-by-day graph across the full width, then the running total and peak per day below it.
  • Laptop: 3 KPIs and the day-by-day graph.
• Data.
  • The summary route supplies the KPIs, records and comparisons.
  • The shell supplies the daily rows: ctx.loadDays() for the month and ctx.loadPeriodDays(ctx.previousPeriod()) for the previous month (#150). Both reject on failure, and the affected cells then show an error.
  • The entrances route is fetched only by the profiles that show those graphs; Laptop fetches only the summary and the month's daily rows.
  • The view keeps no cache of its own.
  • Charts are built with ctx.createChart (deck locale on the ticks), and numbers use ctx.locale with a memoised Intl.NumberFormat.
  • Any failure while filling the view, including an unreadable payload, ends in the cells' error notice instead of LOADING.
• Marker.
  • The day-by-day and peak graphs pass per-day tiers to the shell's deckQualityMarks plugin.
  • A local sdmDayMarks plugin shades Closed Days as closed (not missing). It hatches open days with no data in the shared Unreliable style, because they have no bar for the shared plugin to mark. It also draws a short bar at the top of each holiday.
  • The running total includes marked days. Segments ending on an Estimate day are dashed amber; segments ending on an Unreliable day are dotted grey. The previous month's line is withheld when the route reports previous.reason === 'PARTIAL_PERIOD' (RFC §5.3), and the legend gives that reason.
  • The average by weekday includes marked days and excludes Closed Days.
• Comparisons. A comparison shows the change, or "—" with its ComparisonReason text (never 0%). The "PER DAY" tag appears only when the route returns basis: DAILY_AVERAGE. The last-year line appears only when last year has data (RFC §5.1); otherwise it is hidden, not shown as "—".
• Markup. <data value> wraps only real numbers. The covered-days note and the "+N MORE" line use the shell's formatCoverage and withData.
• CSS is in the new file app/static/css/statistics-deck-month.css, with view-scoped sdm-* classes and colours from tokens. i18n (es and en, full parity) is under statisticsDeck.views.month.* and statisticsDeck.comparison.reason.<CODE> (all 10 codes), placed at the end of each statisticsDeck block.

Verification

• tests/frontend/test_statistics_deck_month.test.js has 34 node:test cases. They cover:
  • months of 28, 29, 30 and 31 days;
  • running-total alignment when the months differ in length (the shorter line ends), and the previous line withheld on PARTIAL_PERIOD;
  • weekday averages (marked days included, Closed Days and no-data days left out);
  • Estimate, Unreliable and missing days, holidays and Closed Days in the per-day series, the tiers passed to the plugin, and the day-marks plugin drawing;
  • "—" with a reason for every kind of missing reference, and last year hidden without data;
  • a partial month compared by daily average above the threshold and shown as "—" below it;
  • records never coming from an Unreliable day, with the peak's day matched by timestamp;
  • <data> only on numbers, and the counts in the coverage and "+N MORE" notes;
  • es/en number formatting;
  • each profile's KPI and graph set in a fake DOM, per-route error panels, a rejected loadDays, an unreadable payload, a throwing chart builder (no unhandled rejection), and cleanup;
  • the fixed Room split and token-only colours in the CSS.
• TZ=America/Caracas pytest -q: 416 passed. ruff check ., ruff format --check . and scripts/check_docs.py are clean.
• Real browser (Chromium through Playwright, against the app on the copied dev DB):
  • Checked Room, Desk and Laptop at 1920×1080, 1920×960 and 760×600, for two months: August 2026 (31 days, with Estimate, Unreliable, missing, holiday and Closed days) and a partial February 2026 (18 of 28 days covered, with the comparison withheld as PARTIAL_PERIOD).
  • All 18 runs had no page, root, row, cell or graph overflow and a 38 px top bar. I also looked at the screenshots.
  • Not seen live: the dev DB has no closed month with data. /periods?granularity=month returns [], and the live Month tab shows the shell's "no closed periods with data" notice, which I checked. For the layout runs, Playwright served the month routes from fixtures. The login, shell, top bar and badge were live.

Decisions

1. Daily average comparison. The daily average shows the previous month's value (previous_month_daily_average, e.g. PREV. MONTH 968/DAY), or "—" with its reason. The route always sends this value, even when the visitors comparison is withheld as PARTIAL_PERIOD, so the tile can put two daily averages side by side over unequal coverage. The view does not compute a percentage from them.
2. Entrance share. Each entrance shows this month's share bar and share %, followed by the route's per-entrance previous-month visitors change (or "—" with a reason such as NEW_CAMERA), labelled VISITORS VS PREV. MONTH. I did not derive a previous-month share on the client, because an entrance present only last month is missing from the response and would skew the total. At most 8 entrances are listed, with a "+N MORE" line for the rest.
3. Highest peak day label. The browser clock must not format facility time, so the day label comes from the daily row whose peak_timestamp_epoch matches the route's peak exactly (tie-proof). That row must be covered and not excluded. Without a timestamp, the label falls back to the first reliable row with the same value.
4. Running total with no previous-month data. The axis is this month's length, and the legend says "PREV. MONTH — no data for the previous period".
5. Room split. The Month view's Room profile always uses one fixed 17/50/33 split (.sd-root[data-granularity="month"] .sd-view[data-profile="room"]), not varied by viewport. It holds down to 600 px of height, where the shell's 12% KPI row cannot fit a value and two lines. RFC §3.5 allows a view to adjust its split.
6. Colours. Visitors are cyan (ingress), the peak is white, and the previous month is muted grey. Holidays are marked with a white top bar. Amber is used only for Estimate marks and for the partial-coverage note, and red is not used.

Known duplication left for consolidation

These are local to month_view.js and are candidates to share with the Day and Week views: KPI tile rendering (kpiTile, lineEl, valueEl), line, comparisonLine, reasonText, formatNumber, formatPercent, formatChange, formatDwell, and the entrance share list (mapEntrances plus fillEntrances). The i18n keys under statisticsDeck.comparison.reason.* may collide with the sibling PRs.

The shell modules were not changed. The branch is rebased on #150 (fix/deck-shell-view-support), so until #150 merges this PR's diff against master also shows #150's commits.

🤖 Generated with Claude Code

Closes #136 ## Problem The statistics deck shell (#133, PR #144) has a Month tab, but it only showed placeholder panels. RFC #80 §4 defines what the Month view shows: six ranked KPIs and five ranked graphs, laid out separately for each of the three profiles, with the data-quality marker on per-day graphs. ## Approach - **New view module** `app/static/js/src/ui/statistics_deck/views/month_view.js`. It calls `registerView('month', renderMonth)` when imported, and index.html imports it next to the StatisticsDeck import. Everything that maps route JSON to KPIs or graph inputs is an exported pure function: `mapMonthKpis` (driven by a KPI spec table), `comparisonLine`, `line`, `hasLastYear`, `monthDaySeries`, `runningTotals`, `previousTotalReason`, `weekdayAverages`, `mapEntrances`, `peakDay`, `barTiers`, and the config builders `perDayBarConfig`, `runningTotalConfig` and `weekdayConfig`. - **Layouts** (`MONTH_LAYOUT`), each smaller profile keeping the top of the ranking: - **Room**: 6 KPIs. Lead row: the month day by day (2/3 of the width) and the running total (1/3). Supporting row: peak per day, average by weekday, entrance share. - **Desk**: 4 KPIs. The day-by-day graph across the full width, then the running total and peak per day below it. - **Laptop**: 3 KPIs and the day-by-day graph. - **Data.** - The summary route supplies the KPIs, records and comparisons. - The shell supplies the daily rows: `ctx.loadDays()` for the month and `ctx.loadPeriodDays(ctx.previousPeriod())` for the previous month (#150). Both reject on failure, and the affected cells then show an error. - The entrances route is fetched only by the profiles that show those graphs; Laptop fetches only the summary and the month's daily rows. - The view keeps no cache of its own. - Charts are built with `ctx.createChart` (deck locale on the ticks), and numbers use `ctx.locale` with a memoised `Intl.NumberFormat`. - Any failure while filling the view, including an unreadable payload, ends in the cells' error notice instead of LOADING. - **Marker.** - The day-by-day and peak graphs pass per-day `tiers` to the shell's `deckQualityMarks` plugin. - A local `sdmDayMarks` plugin shades Closed Days as closed (not missing). It hatches open days with no data in the shared Unreliable style, because they have no bar for the shared plugin to mark. It also draws a short bar at the top of each holiday. - The running total includes marked days. Segments ending on an Estimate day are dashed amber; segments ending on an Unreliable day are dotted grey. The previous month's line is withheld when the route reports `previous.reason === 'PARTIAL_PERIOD'` (RFC §5.3), and the legend gives that reason. - The average by weekday includes marked days and excludes Closed Days. - **Comparisons.** A comparison shows the change, or "—" with its ComparisonReason text (never 0%). The "PER DAY" tag appears only when the route returns `basis: DAILY_AVERAGE`. The last-year line appears only when last year has data (RFC §5.1); otherwise it is hidden, not shown as "—". - **Markup.** `<data value>` wraps only real numbers. The covered-days note and the "+N MORE" line use the shell's `formatCoverage` and `withData`. - **CSS** is in the new file `app/static/css/statistics-deck-month.css`, with view-scoped `sdm-*` classes and colours from tokens. **i18n** (es and en, full parity) is under `statisticsDeck.views.month.*` and `statisticsDeck.comparison.reason.<CODE>` (all 10 codes), placed at the end of each `statisticsDeck` block. ## Verification - `tests/frontend/test_statistics_deck_month.test.js` has 34 node:test cases. They cover: - months of 28, 29, 30 and 31 days; - running-total alignment when the months differ in length (the shorter line ends), and the previous line withheld on PARTIAL_PERIOD; - weekday averages (marked days included, Closed Days and no-data days left out); - Estimate, Unreliable and missing days, holidays and Closed Days in the per-day series, the tiers passed to the plugin, and the day-marks plugin drawing; - "—" with a reason for every kind of missing reference, and last year hidden without data; - a partial month compared by daily average above the threshold and shown as "—" below it; - records never coming from an Unreliable day, with the peak's day matched by timestamp; - `<data>` only on numbers, and the counts in the coverage and "+N MORE" notes; - es/en number formatting; - each profile's KPI and graph set in a fake DOM, per-route error panels, a rejected `loadDays`, an unreadable payload, a throwing chart builder (no unhandled rejection), and cleanup; - the fixed Room split and token-only colours in the CSS. - `TZ=America/Caracas pytest -q`: 416 passed. `ruff check .`, `ruff format --check .` and `scripts/check_docs.py` are clean. - **Real browser** (Chromium through Playwright, against the app on the copied dev DB): - Checked Room, Desk and Laptop at 1920×1080, 1920×960 and 760×600, for two months: August 2026 (31 days, with Estimate, Unreliable, missing, holiday and Closed days) and a partial February 2026 (18 of 28 days covered, with the comparison withheld as PARTIAL_PERIOD). - All 18 runs had no page, root, row, cell or graph overflow and a 38 px top bar. I also looked at the screenshots. - **Not seen live:** the dev DB has no closed month with data. `/periods?granularity=month` returns `[]`, and the live Month tab shows the shell's "no closed periods with data" notice, which I checked. For the layout runs, Playwright served the month routes from fixtures. The login, shell, top bar and badge were live. ## Decisions 1. **Daily average comparison.** The daily average shows the previous month's value (`previous_month_daily_average`, e.g. `PREV. MONTH 968/DAY`), or "—" with its reason. The route always sends this value, even when the visitors comparison is withheld as PARTIAL_PERIOD, so the tile can put two daily averages side by side over unequal coverage. The view does not compute a percentage from them. 2. **Entrance share.** Each entrance shows this month's share bar and share %, followed by the route's per-entrance previous-month **visitors** change (or "—" with a reason such as NEW_CAMERA), labelled `VISITORS VS PREV. MONTH`. I did not derive a previous-month share on the client, because an entrance present only last month is missing from the response and would skew the total. At most 8 entrances are listed, with a "+N MORE" line for the rest. 3. **Highest peak day label.** The browser clock must not format facility time, so the day label comes from the daily row whose `peak_timestamp_epoch` matches the route's peak exactly (tie-proof). That row must be covered and not excluded. Without a timestamp, the label falls back to the first reliable row with the same value. 4. **Running total with no previous-month data.** The axis is this month's length, and the legend says "PREV. MONTH — no data for the previous period". 5. **Room split.** The Month view's Room profile always uses one fixed 17/50/33 split (`.sd-root[data-granularity="month"] .sd-view[data-profile="room"]`), not varied by viewport. It holds down to 600 px of height, where the shell's 12% KPI row cannot fit a value and two lines. RFC §3.5 allows a view to adjust its split. 6. **Colours.** Visitors are cyan (ingress), the peak is white, and the previous month is muted grey. Holidays are marked with a white top bar. Amber is used only for Estimate marks and for the partial-coverage note, and red is not used. ## Known duplication left for consolidation These are local to `month_view.js` and are candidates to share with the Day and Week views: KPI tile rendering (`kpiTile`, `lineEl`, `valueEl`), `line`, `comparisonLine`, `reasonText`, `formatNumber`, `formatPercent`, `formatChange`, `formatDwell`, and the entrance share list (`mapEntrances` plus `fillEntrances`). The i18n keys under `statisticsDeck.comparison.reason.*` may collide with the sibling PRs. The shell modules were not changed. The branch is rebased on #150 (`fix/deck-shell-view-support`), so until #150 merges this PR's diff against master also shows #150's commits. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit 2026-09-26 23:05:09 +00:00

feat(statistics-deck): Month view (Room, Desk, Laptop) (#136) ... 128ab914f2

Register the Month view with the deck shell. KPIs: visitors vs the
previous month (and last year when it has data), daily average, best
day, average visit, highest peak, weekend share. Graphs: the month day
by day, running total vs the previous month aligned by day of month,
peak people inside per day, average by weekday, entrance share with the
route's previous-month comparison. Room shows 6 KPIs and 5 graphs, Desk
4 and 3, Laptop 3 and 1.

Per-day graphs carry the shared Estimate/Unreliable marks, shade Closed
Days as closed, hatch open days with no data and flag holidays; the
running total styles segments that come from marked days. A missing
reference shows "—" with its ComparisonReason text, never 0%.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gabogg commented 2026-09-26 23:08:26 +00:00

Author

Owner

Copy link

Code review: PR #147 (feat/deck-month-view @ 128ab91 vs master @ 08b6d1c)

Standards sources: AGENTS.md, docs/standards/code-standards.md, docs/standards/ui-design-guidelines.md, ADR 0002, the deck shell's exported helpers, and the Fowler smell baseline. Duplication across the Day, Week and Month views is known and will be consolidated after merge, so it isn't flagged here. Spec: #136 and RFC §3–§5.

Standards

Verified:

• 27 node tests pass.
• No XSS: all text goes through h(... text:).
• Charts are destroyed in cleanup, with a disposed guard.
• No facility times are formatted from the browser clock.
• At most 4 requests per render; Laptop fetches only the summary and loadDays.
• Shell helpers (registerView, createDeckChart, deckQualityMarks tiers, classifyDay, hatchPattern) are used correctly.

#	Pri	Finding
S-1	P2	The fetch chain Promise.allSettled([...]).then(... fillMonth ...) has no .catch. A mapping error on an unexpected payload becomes an unhandled rejection, and every cell stays on LOADING.
S-2	P2	L655 rebuilds the shell's formatCoverage and drops <data> (ui-design-guidelines §3.3). The "+N more" text (L721) has the same gap. Use withData(doc, formatCoverage(period, tr)).
S-3	P3	L544 period.business_days === 0 re-implements the shell's isClosedPeriod().
S-4	P3	rootStyle()/token() (L330–353) copy charts.js. The CSS hard-codes rgba(69, 79, 94, 0.35) (L182) instead of a token (§2.1). Extend the shell's COLOR_TOKENS.
S-5	P3	monthUrls().previousDays rebuilds the daily-route URL, and cachedJson is a second per-URL cache next to the shell's loadDays. Let ctx.loadDays(period) take another period.
S-6	P3	"Closed periods do not change", but Partial months are rendered and cached for the whole page life, so they can go stale.
S-7	P3	Possible Duplicated Code: dayByDayConfig and peakPerDayConfig differ only in field and colour. Use one perDayBarConfig(series, field, color).
S-8	P3	Possible Data Clumps: the {label, value, missing, reason, perDay, change} literal appears 6 times, and mapMonthKpis repeats one if/else pattern six times. Use a line() factory or a table.
S-9	P3	Chart.js ticks format numbers in the browser locale (no options.locale), unlike formatNumber. Intl.NumberFormat is also rebuilt on every call; memoise it.
S-10	P3	§3.3: <data value=""> wraps non-numeric text ("—", "1.234/día" with a null change). Use <data> only for real numbers.
S-11	P3	Middle Man: export const renderMonthView = renderMonth;. L103 has a redundant Math.floor.

Spec

Verified:

• The six KPIs and five graphs are in rank order per profile (Room 6/5, Desk 4/3, Laptop 3/1).
• "—" with a reason, never 0%; "PER DAY" only when basis is DAILY_AVERAGE.
• Weekend share and records come from the route, which skips Unreliable days.
• Holidays are marked; Closed Days are shaded as closed; missing days are hatched.
• No confidence margins; es/en parity; months of 28–31 days.

#	Pri	Finding
C-1	P2	"Partial periods compare by daily average only when the route returns basis: DAILY_AVERAGE" / RFC §5.3 "otherwise the change is not shown": runningTotals draws last month's cumulative line even when summary.previous.reason === 'PARTIAL_PERIOD', putting raw totals from unequal coverage side by side.
C-2	P3	Unrequested scope: the view's own 40-entry response cache (same root as S-5/S-6). The 8-entrance cap is fine for zero-scroll.
C-3	P3	views.month.noData is added in both languages but never used.
C-4	P3	Decision 1 is acceptable (the route always sends previous_month_daily_average, and no change is shown on PARTIAL_PERIOD). The PR's claim that the coverage rule "cannot be bypassed" is inaccurate.
C-5	P3	Decision 2: "Entrance share vs previous month". The change shown is the route's visitors change under a share bar labelled "VS PREV. MONTH", so it reads as a change in share. Label it as a visitors change.
C-6	P3	Decision 3: correct, but matching peak_timestamp_epoch === peak.timestamp_epoch would be exact and tie-proof.
C-7	—	Decision 4 is sound. No action.
C-8	P3	Decision 5: allowed by §3.5 ("Each view may adjust its split as long as the rule in 3.1 holds"), and the profile content stays Room. But a height-driven @media switch sits uneasily with §2/§3.3. Prefer one fixed Room split that holds down to about 600 px, or record the allowance in the RFC.

---

Summary. Standards: 11 findings, worst S-1 (an unhandled rejection leaves every cell on LOADING). Spec: 7 actionable findings, worst C-1 (the running total compares raw totals across unequal coverage). First pass, so all findings get fixed on this branch. S-4, S-5 and S-9 build on a small shell-support PR that is being prepared (color tokens, ctx.loadDays(period), chart locale).

🤖 Generated with Claude Code

# Code review: PR #147 (`feat/deck-month-view` @ 128ab91 vs `master` @ 08b6d1c) **Standards** sources: AGENTS.md, `docs/standards/code-standards.md`, `docs/standards/ui-design-guidelines.md`, ADR 0002, the deck shell's exported helpers, and the Fowler smell baseline. Duplication across the Day, Week and Month views is known and will be consolidated after merge, so it isn't flagged here. **Spec**: #136 and RFC §3–§5. ## Standards Verified: - 27 node tests pass. - No XSS: all text goes through `h(... text:)`. - Charts are destroyed in cleanup, with a `disposed` guard. - No facility times are formatted from the browser clock. - At most 4 requests per render; Laptop fetches only the summary and `loadDays`. - Shell helpers (`registerView`, `createDeckChart`, `deckQualityMarks` tiers, `classifyDay`, `hatchPattern`) are used correctly. | # | Pri | Finding | |---|---|---| | S-1 | **P2** | The fetch chain `Promise.allSettled([...]).then(... fillMonth ...)` has no `.catch`. A mapping error on an unexpected payload becomes an unhandled rejection, and every cell stays on LOADING. | | S-2 | **P2** | L655 rebuilds the shell's `formatCoverage` and drops `<data>` (ui-design-guidelines §3.3). The "+N more" text (L721) has the same gap. Use `withData(doc, formatCoverage(period, tr))`. | | S-3 | P3 | L544 `period.business_days === 0` re-implements the shell's `isClosedPeriod()`. | | S-4 | P3 | `rootStyle()`/`token()` (L330–353) copy `charts.js`. The CSS hard-codes `rgba(69, 79, 94, 0.35)` (L182) instead of a token (§2.1). Extend the shell's `COLOR_TOKENS`. | | S-5 | P3 | `monthUrls().previousDays` rebuilds the daily-route URL, and `cachedJson` is a second per-URL cache next to the shell's `loadDays`. Let `ctx.loadDays(period)` take another period. | | S-6 | P3 | "Closed periods do not change", but Partial months are rendered and cached for the whole page life, so they can go stale. | | S-7 | P3 | Possible Duplicated Code: `dayByDayConfig` and `peakPerDayConfig` differ only in field and colour. Use one `perDayBarConfig(series, field, color)`. | | S-8 | P3 | Possible Data Clumps: the `{label, value, missing, reason, perDay, change}` literal appears 6 times, and `mapMonthKpis` repeats one if/else pattern six times. Use a `line()` factory or a table. | | S-9 | P3 | Chart.js ticks format numbers in the browser locale (no `options.locale`), unlike `formatNumber`. `Intl.NumberFormat` is also rebuilt on every call; memoise it. | | S-10 | P3 | §3.3: `<data value="">` wraps non-numeric text ("—", "1.234/día" with a null change). Use `<data>` only for real numbers. | | S-11 | P3 | Middle Man: `export const renderMonthView = renderMonth;`. L103 has a redundant `Math.floor`. | ## Spec Verified: - The six KPIs and five graphs are in rank order per profile (Room 6/5, Desk 4/3, Laptop 3/1). - "—" with a reason, never 0%; "PER DAY" only when `basis` is `DAILY_AVERAGE`. - Weekend share and records come from the route, which skips Unreliable days. - Holidays are marked; Closed Days are shaded as closed; missing days are hatched. - No confidence margins; es/en parity; months of 28–31 days. | # | Pri | Finding | |---|---|---| | C-1 | **P2** | *"Partial periods compare by daily average only when the route returns basis: DAILY_AVERAGE"* / RFC §5.3 *"otherwise the change is not shown"*: `runningTotals` draws last month's cumulative line even when `summary.previous.reason === 'PARTIAL_PERIOD'`, putting raw totals from unequal coverage side by side. | | C-2 | P3 | Unrequested scope: the view's own 40-entry response cache (same root as S-5/S-6). The 8-entrance cap is fine for zero-scroll. | | C-3 | P3 | `views.month.noData` is added in both languages but never used. | | C-4 | P3 | Decision 1 is acceptable (the route always sends `previous_month_daily_average`, and no change is shown on PARTIAL_PERIOD). The PR's claim that the coverage rule "cannot be bypassed" is inaccurate. | | C-5 | P3 | Decision 2: *"Entrance share vs previous month"*. The change shown is the route's **visitors** change under a share bar labelled "VS PREV. MONTH", so it reads as a change in share. Label it as a visitors change. | | C-6 | P3 | Decision 3: correct, but matching `peak_timestamp_epoch === peak.timestamp_epoch` would be exact and tie-proof. | | C-7 | — | Decision 4 is sound. No action. | | C-8 | P3 | Decision 5: allowed by §3.5 (*"Each view may adjust its split as long as the rule in 3.1 holds"*), and the profile content stays Room. But a height-driven `@media` switch sits uneasily with §2/§3.3. Prefer one fixed Room split that holds down to about 600 px, or record the allowance in the RFC. | --- **Summary.** Standards: 11 findings, worst **S-1** (an unhandled rejection leaves every cell on LOADING). Spec: 7 actionable findings, worst **C-1** (the running total compares raw totals across unequal coverage). First pass, so all findings get fixed on this branch. S-4, S-5 and S-9 build on a small shell-support PR that is being prepared (color tokens, `ctx.loadDays(period)`, chart locale). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request from a commit 2026-09-26 23:13:33 +00:00

fix(statistics-deck): shell support for the period views (#134-#136 reviews)

gabogg referenced this pull request 2026-09-26 23:13:34 +00:00

fix(statistics-deck): shell support for the period views #150

gabogg referenced this pull request 2026-09-27 09:28:43 +00:00

fix(statistics-deck): shell support for the period views #150

gabogg referenced this pull request 2026-09-27 09:37:41 +00:00

fix(statistics-deck): shell support for the period views #150

gabogg referenced this pull request from a commit 2026-09-27 09:48:20 +00:00

fix(statistics-deck): address first-pass review of the Month view (#147)

gabogg force-pushed feat/deck-month-view from 128ab914f2 to 5d97b52ea5 2026-09-27 09:48:20 +00:00 Compare

gabogg commented 2026-09-27 09:49:45 +00:00

Author

Owner

Copy link

First-pass review: all findings fixed in 5d97b52

The branch is rebased on #150 (fix/deck-shell-view-support) and builds on its shell APIs. Until #150 merges, the diff against master also shows #150's commits.

#	Fix
S-1	The fetch chain ends in .catch. Any failure while filling the view, such as an unreadable payload or a throwing chart builder, replaces every cell with the error notice. Tests cover a throwing payload getter and a throwing createChart, the latter with no unhandled rejection.
S-2	The covered-days note uses withData(doc, formatCoverage(period, tr)), and "+N MORE" uses withData too. A test checks their <data> values.
S-3	Dropped the view's no-period and closed-period guards; the shell (#150) owns those notices.
S-4	Dropped rootStyle()/token(). The view uses the shell's dim/primary/cyan, and monthColors only adds the closed shade from dim. The CSS swatch uses color-mix(var(--color-text-dim) 35%). A test asserts that the CSS has no rgba(.
S-5	Daily rows now come from ctx.loadDays() and ctx.loadPeriodDays(ctx.previousPeriod()). monthUrls().previousDays and cachedJson are gone. A rejected loadDays shows the error in the per-day graphs, and this is tested.
S-6	The view keeps no cache of its own, so it keeps no Partial period (see C-2).
S-7	A single perDayBarConfig(series, field, color, colors) replaces dayByDayConfig/peakPerDayConfig.
S-8	A line() factory builds every sub-line, and mapMonthKpis is driven by a KPI_SPECS table.
S-9	Charts are built with ctx.createChart (deck locale on the ticks). Numbers use ctx.locale, and Intl.NumberFormat is memoised per locale and number of decimals.
S-10	<data value> is used only for real numbers, via a number field on each line; "—" and day labels are <span>. A test checks that every <data> value is numeric.
S-11	renderMonth is exported directly (no alias), and the redundant Math.floor is gone.
C-1	previousTotalReason(summary): when summary.previous.reason === 'PARTIAL_PERIOD', the previous month's line is not drawn and the legend reads "PREV. MONTH — too few days covered to compare". Tested as a unit and rendered.
C-2	Removed the view's response cache.
C-3	Removed views.month.noData (es and en). A test asserts it is absent.
C-4	Corrected the PR text (Decision 1).
C-5	The entrance change is labelled VISITORS VS PREV. MONTH, both in the column note and on each comparison line.
C-6	peakDay matches peak_timestamp_epoch exactly among covered, non-excluded rows. It falls back to the value only when the route gives no timestamp. Tested with a three-way tie that includes an excluded day.
C-7	No action.
C-8	Replaced the height-driven @media with one fixed Room split for the month, .sd-root[data-granularity="month"] .sd-view[data-profile="room"] at 17/50/33. It is overridden only by the shell's existing stack-and-scroll rule below 760 px width. A test asserts that the CSS has no height media query.
Maintainer	Last year is hidden when it has no data: hasLastYear is false for a missing last_year, for NO_DATA_LAST_YEAR, and for a null reference_visitors. Tested.

Verification:

• The month tests grew from 27 to 34, all passing.
• TZ=America/Caracas pytest -q: 416 passed. ruff check, ruff format --check and check_docs are clean, and the pre-commit hook passed.
• Playwright on port 8903: Room, Desk and Laptop at 1920×1080, 1920×960 and 760×600, for a 31-day August and a partial February, with month routes from fixtures. All 18 runs had no page, root, row, cell or graph overflow and a 38 px top bar. I looked at the screenshots.
• The live Month tab shows the shell's "no closed periods with data" notice, because the dev DB has no closed month.

One process note: while correcting the description for C-4, I briefly overwrote it with another PR's text (my scratchpad file had been replaced). It has been restored with the corrections.

🤖 Generated with Claude Code

## First-pass review: all findings fixed in `5d97b52` The branch is rebased on #150 (`fix/deck-shell-view-support`) and builds on its shell APIs. Until #150 merges, the diff against master also shows #150's commits. | # | Fix | |---|---| | S-1 | The fetch chain ends in `.catch`. Any failure while filling the view, such as an unreadable payload or a throwing chart builder, replaces every cell with the error notice. Tests cover a throwing payload getter and a throwing `createChart`, the latter with no unhandled rejection. | | S-2 | The covered-days note uses `withData(doc, formatCoverage(period, tr))`, and "+N MORE" uses `withData` too. A test checks their `<data>` values. | | S-3 | Dropped the view's no-period and closed-period guards; the shell (#150) owns those notices. | | S-4 | Dropped `rootStyle()`/`token()`. The view uses the shell's `dim`/`primary`/`cyan`, and `monthColors` only adds the closed shade from `dim`. The CSS swatch uses `color-mix(var(--color-text-dim) 35%)`. A test asserts that the CSS has no `rgba(`. | | S-5 | Daily rows now come from `ctx.loadDays()` and `ctx.loadPeriodDays(ctx.previousPeriod())`. `monthUrls().previousDays` and `cachedJson` are gone. A rejected `loadDays` shows the error in the per-day graphs, and this is tested. | | S-6 | The view keeps no cache of its own, so it keeps no Partial period (see C-2). | | S-7 | A single `perDayBarConfig(series, field, color, colors)` replaces `dayByDayConfig`/`peakPerDayConfig`. | | S-8 | A `line()` factory builds every sub-line, and `mapMonthKpis` is driven by a `KPI_SPECS` table. | | S-9 | Charts are built with `ctx.createChart` (deck locale on the ticks). Numbers use `ctx.locale`, and `Intl.NumberFormat` is memoised per locale and number of decimals. | | S-10 | `<data value>` is used only for real numbers, via a `number` field on each line; "—" and day labels are `<span>`. A test checks that every `<data>` value is numeric. | | S-11 | `renderMonth` is exported directly (no alias), and the redundant `Math.floor` is gone. | | C-1 | `previousTotalReason(summary)`: when `summary.previous.reason === 'PARTIAL_PERIOD'`, the previous month's line is not drawn and the legend reads "PREV. MONTH — too few days covered to compare". Tested as a unit and rendered. | | C-2 | Removed the view's response cache. | | C-3 | Removed `views.month.noData` (es and en). A test asserts it is absent. | | C-4 | Corrected the PR text (Decision 1). | | C-5 | The entrance change is labelled `VISITORS VS PREV. MONTH`, both in the column note and on each comparison line. | | C-6 | `peakDay` matches `peak_timestamp_epoch` exactly among covered, non-excluded rows. It falls back to the value only when the route gives no timestamp. Tested with a three-way tie that includes an excluded day. | | C-7 | No action. | | C-8 | Replaced the height-driven `@media` with one fixed Room split for the month, `.sd-root[data-granularity="month"] .sd-view[data-profile="room"]` at 17/50/33. It is overridden only by the shell's existing stack-and-scroll rule below 760 px width. A test asserts that the CSS has no height media query. | | Maintainer | Last year is hidden when it has no data: `hasLastYear` is false for a missing `last_year`, for `NO_DATA_LAST_YEAR`, and for a null `reference_visitors`. Tested. | **Verification:** - The month tests grew from 27 to 34, all passing. - `TZ=America/Caracas pytest -q`: 416 passed. `ruff check`, `ruff format --check` and `check_docs` are clean, and the pre-commit hook passed. - Playwright on port 8903: Room, Desk and Laptop at 1920×1080, 1920×960 and 760×600, for a 31-day August and a partial February, with month routes from fixtures. All 18 runs had no page, root, row, cell or graph overflow and a 38 px top bar. I looked at the screenshots. - The live Month tab shows the shell's "no closed periods with data" notice, because the dev DB has no closed month. One process note: while correcting the description for C-4, I briefly overwrote it with another PR's text (my scratchpad file had been replaced). It has been restored with the corrections. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request from a commit 2026-09-27 09:57:33 +00:00

fix(statistics-deck): address first-pass review of the Month view (#147)

gabogg force-pushed feat/deck-month-view from 5d97b52ea5 to 64e1351127 2026-09-27 09:57:33 +00:00 Compare

gabogg referenced this pull request 2026-09-27 10:00:42 +00:00

docs(agents): per-worktree scratch subfolders for parallel agents #154

gabogg commented 2026-09-27 10:01:18 +00:00

Author

Owner

Copy link

Code review, second pass: PR #147 (Month) @ 64e1351, rebased on master

All first-pass fixes (S-1–S-11, C-1–C-8) are verified; 77 node tests pass.

• Shell API (#150): adopted correctly (loadDays/loadPeriodDays(previousPeriod())/createChart/locale/currentDeckColors); the shell owns the empty, error and closed states; no workarounds are left.
• Maintainer decisions:
  • The last year is hidden when it has no data.
  • The Room split is one fixed 17/50/33, with no height @media.
  • The entrance change is labelled VISITORS VS PREV. MONTH.

Standards

#	Pri	Finding
S2-1	P3	showError calls chart.destroy() without the try/catch that cleanup uses. A throwing destroy inside .catch brings back an unhandled rejection.
S2-2	P3	showError clears each graph's body but not its note, so leftover legends and coverage notes sit beside the error.

Spec

#	Pri	Finding
C2-1	P2	RFC §5.3 "compared by daily average": when a partial month passes the coverage threshold, the route compares by daily average (basis: DAILY_AVERAGE), but the running total still draws raw cumulative totals over unequal coverage. For example, with 25 of 31 days covered, the line ends below last month while the tile reads "+5% PER DAY". Withhold the line, or mark it not comparable, on DAILY_AVERAGE too, and add a test.

Verdict: mergeable after the C2-1 fix. The P3s are filed as a follow-up issue.

🤖 Generated with Claude Code

# Code review, second pass: PR #147 (Month) @ 64e1351, rebased on master All first-pass fixes (S-1–S-11, C-1–C-8) are verified; 77 node tests pass. - **Shell API (#150):** adopted correctly (`loadDays`/`loadPeriodDays(previousPeriod())`/`createChart`/`locale`/`currentDeckColors`); the shell owns the empty, error and closed states; no workarounds are left. - **Maintainer decisions:** - The last year is hidden when it has no data. - The Room split is one fixed 17/50/33, with no height `@media`. - The entrance change is labelled `VISITORS VS PREV. MONTH`. ## Standards | # | Pri | Finding | |---|---|---| | S2-1 | P3 | `showError` calls `chart.destroy()` without the try/catch that cleanup uses. A throwing destroy inside `.catch` brings back an unhandled rejection. | | S2-2 | P3 | `showError` clears each graph's body but not its note, so leftover legends and coverage notes sit beside the error. | ## Spec | # | Pri | Finding | |---|---|---| | C2-1 | **P2** | RFC §5.3 *"compared by daily average"*: when a partial month passes the coverage threshold, the route compares by daily average (`basis: DAILY_AVERAGE`), but the running total still draws raw cumulative totals over unequal coverage. For example, with 25 of 31 days covered, the line ends below last month while the tile reads "+5% PER DAY". Withhold the line, or mark it not comparable, on `DAILY_AVERAGE` too, and add a test. | **Verdict:** mergeable after the C2-1 fix. The P3s are filed as a follow-up issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request 2026-09-27 10:02:10 +00:00

follow-up(statistics-deck): P3 cleanups from the Day/Week/Month view reviews #155

gabogg referenced this pull request 2026-09-27 10:02:13 +00:00

refactor(statistics-deck): consolidate the period views' shared helpers into the shell #156

gabogg commented 2026-09-27 10:06:49 +00:00

Author

Owner

Copy link

Second-pass review: C2-1 fixed in 3ba8726

#	Fix
C2-1	previousTotalReason now also withholds last month's running-total line when the route compares by daily average (previous.basis === 'DAILY_AVERAGE'), not only on PARTIAL_PERIOD. The legend reads "PREV. MONTH — compared per day, not as a total", and the Visitors tile still shows the route's "+x% PER DAY". A new i18n key, views.month.comparedPerDay, carries that reason in es and en. It is covered by a unit test and a rendered test (a partial month covering 25 of 31 days, compared by daily average).
withData	Checked, no change needed. The Month view passes only whole counts through withData: "18 / 31 DAYS" and "+2 MORE". Every formatted figure (decimals, thousands separators, percentages) is already a single <data value={raw}>. The existing test checks the <data> values ['18','31'] and ['2'], and that every <data> value is numeric.

S2-1 and S2-2 are left for the filed follow-up issue.

Verification:

• Month tests: 35, all passing. All frontend tests: 164, all passing.
• TZ=America/Caracas pytest -q: 416 passed.
• ruff check, ruff format --check and check_docs are clean.

🤖 Generated with Claude Code

## Second-pass review: C2-1 fixed in `3ba8726` | # | Fix | |---|---| | C2-1 | `previousTotalReason` now also withholds last month's running-total line when the route compares by daily average (`previous.basis === 'DAILY_AVERAGE'`), not only on `PARTIAL_PERIOD`. The legend reads "PREV. MONTH — compared per day, not as a total", and the Visitors tile still shows the route's "+x% PER DAY". A new i18n key, `views.month.comparedPerDay`, carries that reason in es and en. It is covered by a unit test and a rendered test (a partial month covering 25 of 31 days, compared by daily average). | | `withData` | Checked, no change needed. The Month view passes only whole counts through `withData`: "18 / 31 DAYS" and "+2 MORE". Every formatted figure (decimals, thousands separators, percentages) is already a single `<data value={raw}>`. The existing test checks the `<data>` values ['18','31'] and ['2'], and that every `<data>` value is numeric. | S2-1 and S2-2 are left for the filed follow-up issue. **Verification:** - Month tests: 35, all passing. All frontend tests: 164, all passing. - `TZ=America/Caracas pytest -q`: 416 passed. - `ruff check`, `ruff format --check` and `check_docs` are clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request from a commit 2026-09-27 10:06:49 +00:00

fix(statistics-deck): withhold the running total on a daily-average comparison (#147 review)

gabogg added 1 commit 2026-09-27 10:06:50 +00:00

fix(statistics-deck): withhold the running total on a daily-average comparison (#147 review) ... 3ba8726714

RFC §5.3 compares a partial month by daily average. When the route
returns basis DAILY_AVERAGE, raw cumulative totals over unequal coverage
are not comparable either, so the previous month's running-total line is
withheld with "—" and the reason "compared per day, not as a total",
as it already was on PARTIAL_PERIOD.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gabogg merged commit 0c6c3da61a into master 2026-09-27 10:09:26 +00:00

gabogg deleted branch feat/deck-month-view 2026-09-27 10:09:27 +00:00

gabogg referenced this pull request from a commit 2026-09-27 10:09:28 +00:00

feat(statistics-deck): Month view (Room, Desk, Laptop) (#136) (#147)

gabogg referenced this pull request from a commit 2026-09-27 14:27:15 +00:00

test(statistics-deck): Week expects the canonical comparison-reason texts

gabogg referenced this pull request 2026-09-27 14:27:24 +00:00

feat(statistics-deck): Week view (Room, Desk, Laptop) (#135) #148

gabogg referenced this pull request from a commit 2026-09-27 14:36:21 +00:00

test(statistics-deck): Day expects the canonical comparison-reason texts

gabogg referenced this pull request from a commit 2026-09-27 14:36:50 +00:00

Merge master into feat/deck-day-view (Month #147, Week #148)

gabogg referenced this pull request 2026-09-27 14:36:59 +00:00

feat(statistics-deck): Day view (Room, Desk, Laptop) (#134) #149

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

blocked

Waits on the issues listed in its "Blocked by:" line before implementation

  

bug

Category: something is broken; behaviour differs from the spec or docs

  

enhancement

Category: new feature or improvement, including refactors, chores and docs

  

high-priority

Address before ordinary-priority work; independent of triage readiness and blockers

  

low-priority

Address after ordinary-priority work; independent of triage readiness and blockers

  

needs-info

Waiting on reporter for more information

  

needs-triage

Maintainer needs to evaluate this issue

  

ready-for-agent

Fully specified, ready for an AFK agent

  

ready-for-human

Requires human implementation

  

referenced

Previously closed issue reopened or referenced by an open issue; visible history marker

  

research

Needs investigation (internet or local resources) before it can be triaged down; used alongside a category and a triage role

  

wontfix

Will not be actioned

No labels

blocked

bug

enhancement

high-priority

low-priority

needs-info

needs-triage

ready-for-agent

ready-for-human

referenced

research

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gabogg/hikcentral!147

No description provided.