gabogg/hikcentral
Watch
1
0
Fork
You've already forked hikcentral
 0
Code Issues 113 Pull requests 8 Projects Releases Packages Wiki Activity Actions

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

Merged
gabogg merged 3 commits from feat/deck-week-view into master 2026-09-27 14:27:27 +00:00
Conversation 4 Commits 3 Files changed 5 +1903
gabogg commented 2026-09-26 23:07:01 +00:00
Owner
Copy link

Closes #135

Problem

The deck shell (#133, PR #144) has no Week view: the WEEK tab shows placeholders. The RFC (§3, §4, §4.1, §5) asks for a Week view in three layout profiles. It must show ranked KPIs and graphs, honest comparisons ("—" with a reason, never 0%), the data-quality marks on every per-day graph, and no scrolling from the Laptop minimum (760 px) up.

Approach

  • New view module app/static/js/src/ui/statistics_deck/views/week_view.js calls registerView('week', …). index.html gets one import line and one stylesheet link.
  • Pure, exported data mapping, tested without a DOM: weekKpis, comparisonView, alignByWeekday / visitorsByDay / dwellByDay, dayMarks / dayTicks, heatmapModel, entranceShareRows, reliableRecordDate / peakDay, and the Chart.js configs.
  • Profiles (WEEK_LAYOUTS). Each profile is its own layout, and each smaller one keeps the top of both rankings:
    • Room: 6 KPIs, then visitors by day (3fr) and day × hour (2fr), then average visit by day and entrance share.
    • Desk: 4 KPIs, then visitors by day across the full width, then day × hour and average visit.
    • Laptop: 3 KPIs, then visitors by day.
  • Data:
    • The KPIs come from the summary route.
    • The day rows come from the shell's ctx.loadDays(). The previous week comes from ctx.loadPeriodDays(ctx.previousPeriod()) and the same ISO week last year from ctx.loadPeriodDays(ctx.periodFor(last_year.reference_start)) (shell support from #150). The last-year load happens only when the summary carries last_year, which the route omits when that week has no data.
    • bucket=hour (a direct fetchJson) feeds the heatmap. The entrances route feeds entrance share. The summary, hourly and entrances answers are cached per page for complete weeks only. A Partial week is fetched afresh on each render.
    • A profile fetches only the routes its graphs need. Each figure loads on its own, so a failed route, failed day rows included, shows "could not be loaded" in its own cell only.
    • Numbers and chart ticks use the deck locale (ctx.locale, ctx.createChart).
  • Marks:
    • Bars and points use the shell's deckQualityMarks (dashed amber for Estimate, grey hatching for Unreliable).
    • An open day without data has no bar to hatch, so a small local plugin hatches its whole column with the shell's hatchPattern / elementBox.
    • On the heatmap, gap-hour cells and Unreliable rows carry the shell's .sd-swatch-unreliable hatch, and Estimate day labels carry .sd-swatch-estimate. An open day without data is a single hatched "no data" band.
    • x labels carry CLOSED / HOLIDAY and the day's ◇ / ◆. A Closed Day is drawn as closed (no bar, a CLOSED row on the heatmap), not as missing.
  • Zero-scroll: KPI text is sized with container-query units, graph rows use minmax(0, …), and the heatmap and entrance lists shrink their rows. Below 760 px the graph rows stack like the shell's.

Verification

  • TZ=America/Caracas pytest -q: 416 passed. Node frontend suite: 159 tests, 33 of them in tests/frontend/test_statistics_deck_week.test.js.
  • ruff check ., ruff format --check . and python3 scripts/check_docs.py are clean.
  • Real Chromium (Playwright) against the dev DB on port 8902, logged in as operator, at 1920×1080, 1920×960 and 760×600 in Room, Desk and Laptop, in both es and en: no page, root, view, row or cell overflow in any of the 9 cases, and the top bar is 38 px.
    • Live data (week 38: 1 covered day out of 7, all Unreliable): KPIs show — with "no reliable day" for records and — + PARTIAL_PERIOD vs the previous week. There is no last-year line. Every day is hatched, and days without data are "no data" bands on the heatmap.
    • Live data has too few days to show every case, so the same run was repeated with the statistics routes stubbed by a rich fixture. The fixture has an Estimate day with a gap, an excluded day, a missing day, a holiday, a Closed Day, a partial week compared by daily average, last year present, and 9 entrances including a new one. I checked the screenshots by eye.

The tests cover every Acceptance item:

  • Data mapping for all six KPIs.
  • A partial week: covered-day count, DAILY_AVERAGE basis, and — + PARTIAL_PERIOD below the coverage floor.
  • — with a reason for NO_DATA_PREVIOUS_PERIOD, CLOSED_PERIOD, ZERO_REFERENCE_VISITORS and PARTIAL_PERIOD, and no last-year line when last year has no data.
  • An excluded busiest day is never shown while the total still includes it, and records come only from reliable days (busiest day and peak).
  • Three-series alignment by weekday with missing and out-of-order rows.
  • Per-day Estimate / Unreliable / missing marks, holidays and Closed Days.
  • Heatmap gap hatching.
  • Profile KPI and graph counts and per-profile fetches.
  • Chart cleanup and a stale render after dispose.
  • es/en parity, and every ComparisonReason code, which the test reads from app/schemas/statistics.py.

Decisions

  • Last year (maintainer decision C-6). When the route omits last_year, the Visitors tile shows no last-year line and the graph draws no last-year series or legend entry (RFC §5.1 "last year only when it has data"), consistent with the Day view. A last year that exists but whose change is withheld still shows — with its reason.
  • Previous week without data. The graph draws no empty line. The legend names it with — and its reason instead.
  • Records guard. Records come from the route (#129). The view also refuses a busiest day or peak that falls on a day the shell classifies Unreliable. That is cheap insurance, and it makes the acceptance item testable in the client.
  • Peak day, no clock time. The highest peak is dated by matching peak_timestamp_epoch against the daily rows, so no facility time is formatted from the browser clock. Without a timestamp the peak is shown undated.
  • Heatmap as HTML, not canvas. It is a CSS grid, so the hatching reuses the shared CSS style, the numbers are testable in the fake DOM, and it shrinks without a redraw. Columns are the server's hour labels, trimmed to the span that has visitors or a gap. Closed-day staff passages do not widen that span.
  • Entrance share. Share only (RFC §4). At most 7 rows: every entrance when there are 7 or fewer, otherwise the largest 6 and one row summing the rest.
  • Daily average tile. A partial week shows the shell's formatCoverage text (5 / 7 DAYS). A complete week shows its open-day count. The Visitors tile adds the coverage only for a partial week.
  • Graph count. 4 graphs in Room (the RFC allows 4–5).

Known duplication left for consolidation (maintainer)

These are local to week_view.js on purpose, per the coordination rules:

  • formatNumber / formatChange / formatPercent
  • comparisonView / reasonText (the "—" plus reason text)
  • the KPI tile rendering
  • the entrance-share list (entranceShareRows, renderEntrances)
  • the missing-column plugin
  • the <data> text parts (trParts, numberParts)
  • the small route cache for complete weeks

statisticsDeck.comparison.reason.* is shared-intent i18n that the Day and Month PRs may also add.

Outside the view files

  • app/static/index.html: one <link> and one import.
  • app/static/js/i18n.js: statisticsDeck.views.week.* and statisticsDeck.comparison.reason.* at the end of both statisticsDeck blocks.
  • No shell module was changed. The branch is stacked on #150 (fix/deck-shell-view-support) and uses its view APIs.

🤖 Generated with Claude Code

Closes #135 ## Problem The deck shell (#133, PR #144) has no Week view: the WEEK tab shows placeholders. The RFC (§3, §4, §4.1, §5) asks for a Week view in three layout profiles. It must show ranked KPIs and graphs, honest comparisons ("—" with a reason, never 0%), the data-quality marks on every per-day graph, and no scrolling from the Laptop minimum (760 px) up. ## Approach - **New view module** `app/static/js/src/ui/statistics_deck/views/week_view.js` calls `registerView('week', …)`. index.html gets one import line and one stylesheet link. - **Pure, exported data mapping**, tested without a DOM: `weekKpis`, `comparisonView`, `alignByWeekday` / `visitorsByDay` / `dwellByDay`, `dayMarks` / `dayTicks`, `heatmapModel`, `entranceShareRows`, `reliableRecordDate` / `peakDay`, and the Chart.js configs. - **Profiles** (`WEEK_LAYOUTS`). Each profile is its own layout, and each smaller one keeps the top of both rankings: - Room: 6 KPIs, then visitors by day (3fr) and day × hour (2fr), then average visit by day and entrance share. - Desk: 4 KPIs, then visitors by day across the full width, then day × hour and average visit. - Laptop: 3 KPIs, then visitors by day. - **Data**: - The KPIs come from the summary route. - The day rows come from the shell's `ctx.loadDays()`. The previous week comes from `ctx.loadPeriodDays(ctx.previousPeriod())` and the same ISO week last year from `ctx.loadPeriodDays(ctx.periodFor(last_year.reference_start))` (shell support from #150). The last-year load happens only when the summary carries `last_year`, which the route omits when that week has no data. - `bucket=hour` (a direct `fetchJson`) feeds the heatmap. The entrances route feeds entrance share. The summary, hourly and entrances answers are cached per page for complete weeks only. A Partial week is fetched afresh on each render. - A profile fetches only the routes its graphs need. Each figure loads on its own, so a failed route, failed day rows included, shows "could not be loaded" in its own cell only. - Numbers and chart ticks use the deck locale (`ctx.locale`, `ctx.createChart`). - **Marks**: - Bars and points use the shell's `deckQualityMarks` (dashed amber for Estimate, grey hatching for Unreliable). - An open day without data has no bar to hatch, so a small local plugin hatches its whole column with the shell's `hatchPattern` / `elementBox`. - On the heatmap, gap-hour cells and Unreliable rows carry the shell's `.sd-swatch-unreliable` hatch, and Estimate day labels carry `.sd-swatch-estimate`. An open day without data is a single hatched "no data" band. - x labels carry CLOSED / HOLIDAY and the day's ◇ / ◆. A Closed Day is drawn as closed (no bar, a CLOSED row on the heatmap), not as missing. - **Zero-scroll**: KPI text is sized with container-query units, graph rows use `minmax(0, …)`, and the heatmap and entrance lists shrink their rows. Below 760 px the graph rows stack like the shell's. ## Verification - `TZ=America/Caracas pytest -q`: 416 passed. Node frontend suite: 159 tests, 33 of them in `tests/frontend/test_statistics_deck_week.test.js`. - `ruff check .`, `ruff format --check .` and `python3 scripts/check_docs.py` are clean. - Real Chromium (Playwright) against the dev DB on port 8902, logged in as operator, at 1920×1080, 1920×960 and 760×600 in Room, Desk and Laptop, in both `es` and `en`: no page, root, view, row or cell overflow in any of the 9 cases, and the top bar is 38 px. - Live data (week 38: 1 covered day out of 7, all Unreliable): KPIs show `—` with "no reliable day" for records and `—` + PARTIAL_PERIOD vs the previous week. There is no last-year line. Every day is hatched, and days without data are "no data" bands on the heatmap. - Live data has too few days to show every case, so the same run was repeated with the statistics routes stubbed by a rich fixture. The fixture has an Estimate day with a gap, an excluded day, a missing day, a holiday, a Closed Day, a partial week compared by daily average, last year present, and 9 entrances including a new one. I checked the screenshots by eye. The tests cover every Acceptance item: - Data mapping for all six KPIs. - A partial week: covered-day count, DAILY_AVERAGE basis, and `—` + PARTIAL_PERIOD below the coverage floor. - `—` with a reason for NO_DATA_PREVIOUS_PERIOD, CLOSED_PERIOD, ZERO_REFERENCE_VISITORS and PARTIAL_PERIOD, and no last-year line when last year has no data. - An excluded busiest day is never shown while the total still includes it, and records come only from reliable days (busiest day and peak). - Three-series alignment by weekday with missing and out-of-order rows. - Per-day Estimate / Unreliable / missing marks, holidays and Closed Days. - Heatmap gap hatching. - Profile KPI and graph counts and per-profile fetches. - Chart cleanup and a stale render after dispose. - es/en parity, and every `ComparisonReason` code, which the test reads from `app/schemas/statistics.py`. ## Decisions - **Last year (maintainer decision C-6).** When the route omits `last_year`, the Visitors tile shows no last-year line and the graph draws no last-year series or legend entry (RFC §5.1 "last year only when it has data"), consistent with the Day view. A last year that exists but whose change is withheld still shows `—` with its reason. - **Previous week without data.** The graph draws no empty line. The legend names it with `—` and its reason instead. - **Records guard.** Records come from the route (#129). The view also refuses a busiest day or peak that falls on a day the shell classifies Unreliable. That is cheap insurance, and it makes the acceptance item testable in the client. - **Peak day, no clock time.** The highest peak is dated by matching `peak_timestamp_epoch` against the daily rows, so no facility time is formatted from the browser clock. Without a timestamp the peak is shown undated. - **Heatmap as HTML, not canvas.** It is a CSS grid, so the hatching reuses the shared CSS style, the numbers are testable in the fake DOM, and it shrinks without a redraw. Columns are the server's hour labels, trimmed to the span that has visitors or a gap. Closed-day staff passages do not widen that span. - **Entrance share.** Share only (RFC §4). At most 7 rows: every entrance when there are 7 or fewer, otherwise the largest 6 and one row summing the rest. - **Daily average tile.** A partial week shows the shell's `formatCoverage` text (`5 / 7 DAYS`). A complete week shows its open-day count. The Visitors tile adds the coverage only for a partial week. - **Graph count.** 4 graphs in Room (the RFC allows 4–5). ## Known duplication left for consolidation (maintainer) These are local to `week_view.js` on purpose, per the coordination rules: - `formatNumber` / `formatChange` / `formatPercent` - `comparisonView` / `reasonText` (the "—" plus reason text) - the KPI tile rendering - the entrance-share list (`entranceShareRows`, `renderEntrances`) - the missing-column plugin - the `<data>` text parts (`trParts`, `numberParts`) - the small route cache for complete weeks `statisticsDeck.comparison.reason.*` is shared-intent i18n that the Day and Month PRs may also add. ## Outside the view files - `app/static/index.html`: one `<link>` and one `import`. - `app/static/js/i18n.js`: `statisticsDeck.views.week.*` and `statisticsDeck.comparison.reason.*` at the end of both `statisticsDeck` blocks. - No shell module was changed. The branch is stacked on #150 (`fix/deck-shell-view-support`) and uses its view APIs. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
feat(statistics-deck): Week view in Room, Desk and Laptop profiles (#135)
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m9s
Details
93d196c105 
Registers the Week view with the deck shell: six ranked KPIs (visitors vs
the same week last year, vs the previous week, daily average, weekend share,
busiest day, highest peak) and four ranked graphs (visitors by day vs the
previous week and last year, day x hour intensity, average visit by day,
entrance share). Room shows 6 KPIs / 4 graphs, Desk 4 / 3, Laptop 3 / 1.

Missing references show a dash with their ComparisonReason, partial weeks
show their covered-day count and compare by daily average only when the
route says so, records never come from an Unreliable day, per-day graphs
carry the shell's Estimate / Unreliable marks, gap hours are hatched on the
heatmap, and holidays and Closed Days are labelled on every per-day graph.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-26 23:14:04 +00:00
Author
Owner
Copy link

Code review: PR #148 (feat/deck-week-view @ 93d196c 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: #135 and RFC §3–§5.

Standards

Verified: node --check passes and the 24 tests pass; there is no innerHTML; no facility time is formatted with the browser clock; zero radius, tabular numbers and the 1 px grid gap are in place.

# Pri Finding
S-1 P2 L301 rebuilds the shell's formatCoverage without withData. The numbers in the KPI sub-lines are also plain text, not <data> (§3.3/§7).
S-2 P2 viewColors() (L418–430) copies charts.js's private token lookup to add cyan and primary. Extend COLOR_TOKENS instead (being added by #150).
S-3 P3 L498 period.business_days === 0 re-implements isClosedPeriod().
S-4 P2 §3.3: the busiest-day tile shows a day while its <data value> holds a visitor count. Dash tiles and null shares emit <data value=""> for "—". Use <span> when there's no real number.
S-5 P2 Chart.js options.locale isn't set, so ticks use the browser locale, not the deck language.
S-6 P2 The module-level cache keeps summary, previous-week, hour and entrance data for the whole page, including the open Partial week, which then goes stale. "Never change once closed" isn't enforced.
S-7 P2 The shell's loadDays resolves [] on failure, so requests.days.catch(() => []) is dead code. A failed days fetch draws empty charts instead of the cell's "could not be loaded" message.
S-8 P3 Possible Duplicated Code / Repeated Switches: the tier→glyph ternary appears twice (L157, L647), and tierClass() switches on tier a third time.
S-9 P3 Possible Duplicated Code: dailyUrl plus a second cache duplicate the shell's loadDays. The previous week is fetched twice when the user steps back to it.
S-10 P3 Possible Duplicated Code: dwellChartConfig and visitorsChartConfig build the same plugins block twice.
S-11 P3 Possible Mysterious Name: L322 peakShown = peak && !(peakAt.found && !peakAt.date) hides a three-way state. Name it (e.g. peakAt.hiddenAsUnreliable).
S-12 P3 heatmapModel: an open day with no row gets level-0 cells with no hatching, so it reads as zero traffic.
S-13 P3 §2.1: the CSS hard-codes rgba(234,234,234,.7) and rgba(124,135,151,.55), with two hatch gradients of different periods (5 px vs 6 px).

Spec

Verified:

  • Every KPI and graph is in rank order per profile (Room 6/4, Desk 4/3 with a full-width lead graph, Laptop 3/1).
  • "—" with a reason, with es/en copy for all ten codes.
  • minmax(0, …) row splits; weekend share, Closed Days and holidays handled correctly.
  • The records guard is redundant but harmless; dating the peak by timestamp is sound.
# Pri Finding
C-1 P3 "a missing reference shows '—' with its reason": the previous-week line and legend are always drawn, even when that week has no data, so an empty line shows with no reason.
C-2 P3 Unrequested scope: RFC §4 Week lists only "entrance share", with no change. The PR adds a per-entrance change vs the previous week, a new "≈" symbol outside the §4.1 ◇/◆ set, and ◇/◆ glyphs on x-axis labels.
C-3 P3 "Mark graphs with the shell's shared Estimate / Unreliable styles": the heatmap copies the gradient and dashed-amber values literally instead of reusing .sd-swatch-*.
C-4 P3 With a null peak.timestamp_epoch, peakDay falls back to matching peak_people_inside, so a tie can date the peak to the wrong day. Leave it undated instead.
C-5 P3 The PR says "largest 7 entrances with the rest summed", but the code shows the top 6 plus "other".
C-6 P2 (maintainer decision) RFC §5.1 "last year only when it has data" vs KPI #1 "vs the same week last year": the Visitors tile always shows "— NO DATA FOR LAST YEAR" when the route omits last_year, which will be true for the whole first year. The Day view (#149) hides the line instead. The two views must agree.

Summary. Standards: 13 findings, worst S-6/S-7 (a stale Partial-week cache; failed day loads drawn as empty charts). Spec: 6 findings, worst C-6 (last-year line policy, pending the maintainer's decision). First pass, so all findings get fixed on this branch. S-2, S-5 and S-9 build on the shell-support PR #150.

🤖 Generated with Claude Code

# Code review: PR #148 (`feat/deck-week-view` @ 93d196c 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**: #135 and RFC §3–§5. ## Standards Verified: `node --check` passes and the 24 tests pass; there is no `innerHTML`; no facility time is formatted with the browser clock; zero radius, tabular numbers and the 1 px grid gap are in place. | # | Pri | Finding | |---|---|---| | S-1 | **P2** | L301 rebuilds the shell's `formatCoverage` without `withData`. The numbers in the KPI sub-lines are also plain text, not `<data>` (§3.3/§7). | | S-2 | **P2** | `viewColors()` (L418–430) copies `charts.js`'s private token lookup to add cyan and primary. Extend `COLOR_TOKENS` instead (being added by #150). | | S-3 | P3 | L498 `period.business_days === 0` re-implements `isClosedPeriod()`. | | S-4 | **P2** | §3.3: the busiest-day tile shows a day while its `<data value>` holds a visitor count. Dash tiles and null shares emit `<data value="">` for "—". Use `<span>` when there's no real number. | | S-5 | **P2** | Chart.js `options.locale` isn't set, so ticks use the browser locale, not the deck language. | | S-6 | **P2** | The module-level `cache` keeps summary, previous-week, hour and entrance data for the whole page, including the open Partial week, which then goes stale. "Never change once closed" isn't enforced. | | S-7 | **P2** | The shell's `loadDays` resolves `[]` on failure, so `requests.days.catch(() => [])` is dead code. A failed days fetch draws empty charts instead of the cell's "could not be loaded" message. | | S-8 | P3 | Possible Duplicated Code / Repeated Switches: the tier→glyph ternary appears twice (L157, L647), and `tierClass()` switches on tier a third time. | | S-9 | P3 | Possible Duplicated Code: `dailyUrl` plus a second cache duplicate the shell's `loadDays`. The previous week is fetched twice when the user steps back to it. | | S-10 | P3 | Possible Duplicated Code: `dwellChartConfig` and `visitorsChartConfig` build the same `plugins` block twice. | | S-11 | P3 | Possible Mysterious Name: L322 `peakShown = peak && !(peakAt.found && !peakAt.date)` hides a three-way state. Name it (e.g. `peakAt.hiddenAsUnreliable`). | | S-12 | P3 | `heatmapModel`: an open day with no row gets level-0 cells with no hatching, so it reads as zero traffic. | | S-13 | P3 | §2.1: the CSS hard-codes `rgba(234,234,234,.7)` and `rgba(124,135,151,.55)`, with two hatch gradients of different periods (5 px vs 6 px). | ## Spec Verified: - Every KPI and graph is in rank order per profile (Room 6/4, Desk 4/3 with a full-width lead graph, Laptop 3/1). - "—" with a reason, with es/en copy for all ten codes. - `minmax(0, …)` row splits; weekend share, Closed Days and holidays handled correctly. - The records guard is redundant but harmless; dating the peak by timestamp is sound. | # | Pri | Finding | |---|---|---| | C-1 | P3 | *"a missing reference shows '—' with its reason"*: the previous-week line and legend are always drawn, even when that week has no data, so an empty line shows with no reason. | | C-2 | P3 | Unrequested scope: RFC §4 Week lists only "entrance share", with no change. The PR adds a per-entrance change vs the previous week, a new "≈" symbol outside the §4.1 ◇/◆ set, and ◇/◆ glyphs on x-axis labels. | | C-3 | P3 | *"Mark graphs with the shell's shared Estimate / Unreliable styles"*: the heatmap copies the gradient and dashed-amber values literally instead of reusing `.sd-swatch-*`. | | C-4 | P3 | With a null `peak.timestamp_epoch`, `peakDay` falls back to matching `peak_people_inside`, so a tie can date the peak to the wrong day. Leave it undated instead. | | C-5 | P3 | The PR says "largest 7 entrances with the rest summed", but the code shows the top 6 plus "other". | | C-6 | **P2 (maintainer decision)** | RFC §5.1 *"last year only when it has data"* vs KPI #1 *"vs the same week last year"*: the Visitors tile always shows "— NO DATA FOR LAST YEAR" when the route omits `last_year`, which will be true for the whole first year. The Day view (#149) hides the line instead. The two views must agree. | --- **Summary.** Standards: 13 findings, worst **S-6/S-7** (a stale Partial-week cache; failed day loads drawn as empty charts). Spec: 6 findings, worst **C-6** (last-year line policy, pending the maintainer's decision). First pass, so all findings get fixed on this branch. S-2, S-5 and S-9 build on the shell-support PR #150. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gabogg force-pushed feat/deck-week-view from 93d196c105
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m9s
Details
to 5eb0543296
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m4s
Details
2026-09-27 09:49:23 +00:00 Compare
gabogg commented 2026-09-27 09:50:18 +00:00
Author
Owner
Copy link

First-pass review fixes: all findings addressed in 5eb0543

The branch is rebased onto origin/fix/deck-shell-view-support (#150) and force-pushed with --force-with-lease. The fixes use #150's view APIs. The PR description is updated to match the code.

# Fix (commit 5eb0543) Test
S-1 KPI lines are part arrays (trParts, numberParts), so every number renders as <data value>. The Partial coverage is the shell's formatCoverage text, with its counts as <data>. A complete week shows its open-day count. text lines keep their numbers as <data> parts, KPI sub-lines render their numbers as <data>…, partial-week test
S-2 Deleted viewColors(). Uses currentDeckColors(), whose cyan / primary / dim come from #150. profile render tests
S-3 Dropped the no-period and closed-period guards. The shell no longer calls the view in those states. (guard tests removed)
S-4 A number value is <data>. The busiest day, and any "—" tile or null share, is a <span>, so no <data value=""> remains. KPI sub-lines render…, entrance rows show … a missing share is a dash <span>
S-5 Charts are built with ctx.createChart (deck locale on ticks). formatNumber / formatChange use Intl.NumberFormat(ctx.locale). formatting test (en-US / es-VE)
S-6 The route cache holds complete weeks only. A Partial week is fetched afresh on every render. complete weeks are cached per page; a Partial week is fetched afresh
S-7 loadDays now rejects. The lead and dwell graphs show "could not be loaded", and the KPIs still render from the summary. failed day rows show "could not be loaded"…
S-8 One TIER_GLYPH map and one TIER_SWATCH map replace the ternaries and tierClass(). ticks / heatmap tests
S-9 Removed dailyUrl and the second days cache. The reference weeks use ctx.loadPeriodDays(ctx.previousPeriod()) and ctx.periodFor(last_year.reference_start). Only the bucket=hour call remains a direct fetchJson (hourlyUrl). profile render tests assert the shell loader calls
S-10 A shared dayMarkPlugins(marks) / barConfig builds the plugins block for both per-day graphs. per-day marks test compares both configs
S-11 peakDay returns {date, hiddenAsUnreliable}, and the tile reads that named flag. peak test
S-12 An open day without data, or with no row at all, is missing in heatmapModel and renders as one hatched "NO DATA" band, not zero-traffic cells. an open day without data or without a row is a missing band…, swatch render test
S-13 Removed the hard-coded rgba(...) values and both hatch gradients from the Week CSS. Legend lines use tokens. The hatch and estimate styles come from the shell's .sd-swatch-*. swatch render test; CSS has no colour literals outside var() fallbacks
C-1 referenceState and visitorsLegend: a previous week without counted days draws no line. The legend reads "PREVIOUS WEEK — " (CLOSED_PERIOD when every day was closed), or "— COULD NOT BE LOADED" on failure. a reference week without data is not drawn…, a previous week without data draws no line…
C-2 Entrance share shows name, bar and share only. The per-entrance change and the ≈ symbol are removed, along with the changeVsPrevious copy. The ◇/◆ on x labels stay (§4.1). entrance mapping and render tests
C-3 Heatmap gap and Unreliable cells use .sd-swatch-unreliable, Estimate day labels use .sd-swatch-estimate, and the gap legend key is an .sd-swatch. the heatmap uses the shell's shared swatches…
C-4 A peak with a null timestamp is shown undated ("PEOPLE INSIDE"), with no match on the count. peak test
C-5 The PR text now says: at most 7 rows, all when ≤ 7, else the largest 6 plus one "N other entrances" row. That matches ENTRANCE_ROWS. entrance test covers 6 and 9 entrances
C-6 No last-year data: no line on the Visitors tile, no last-year series or legend entry, and no last-year load. A last year whose change is withheld still shows "— VS LAST YEAR · ". no data last year: the visitors tile hides the last-year line, no last-year data: no line, no legend entry and no last-year fetch

Verification

  • TZ=America/Caracas pytest -q: 416 passed.
  • Node frontend suite: 159 passed, 33 of them Week tests.
  • ruff check ., ruff format --check . and check_docs.py are clean.
  • Playwright (Chromium) on port 8902 with the dev DB, and again with the stubbed rich fixture. Room, Desk and Laptop at 1920×1080, 1920×960 and 760×600, in es and en: no page, root, view, row or cell overflow, and the top bar is 38 px in all cases. I looked at the screenshots.

🤖 Generated with Claude Code

## First-pass review fixes: all findings addressed in `5eb0543` The branch is rebased onto `origin/fix/deck-shell-view-support` (#150) and force-pushed with `--force-with-lease`. The fixes use #150's view APIs. The PR description is updated to match the code. | # | Fix (commit `5eb0543`) | Test | |---|---|---| | S-1 | KPI lines are part arrays (`trParts`, `numberParts`), so every number renders as `<data value>`. The Partial coverage is the shell's `formatCoverage` text, with its counts as `<data>`. A complete week shows its open-day count. | `text lines keep their numbers as <data> parts`, `KPI sub-lines render their numbers as <data>…`, partial-week test | | S-2 | Deleted `viewColors()`. Uses `currentDeckColors()`, whose `cyan` / `primary` / `dim` come from #150. | profile render tests | | S-3 | Dropped the no-period and closed-period guards. The shell no longer calls the view in those states. | (guard tests removed) | | S-4 | A number value is `<data>`. The busiest day, and any "—" tile or null share, is a `<span>`, so no `<data value="">` remains. | `KPI sub-lines render…`, `entrance rows show … a missing share is a dash <span>` | | S-5 | Charts are built with `ctx.createChart` (deck locale on ticks). `formatNumber` / `formatChange` use `Intl.NumberFormat(ctx.locale)`. | formatting test (en-US / es-VE) | | S-6 | The route cache holds complete weeks only. A Partial week is fetched afresh on every render. | `complete weeks are cached per page; a Partial week is fetched afresh` | | S-7 | `loadDays` now rejects. The lead and dwell graphs show "could not be loaded", and the KPIs still render from the summary. | `failed day rows show "could not be loaded"…` | | S-8 | One `TIER_GLYPH` map and one `TIER_SWATCH` map replace the ternaries and `tierClass()`. | ticks / heatmap tests | | S-9 | Removed `dailyUrl` and the second days cache. The reference weeks use `ctx.loadPeriodDays(ctx.previousPeriod())` and `ctx.periodFor(last_year.reference_start)`. Only the `bucket=hour` call remains a direct `fetchJson` (`hourlyUrl`). | profile render tests assert the shell loader calls | | S-10 | A shared `dayMarkPlugins(marks)` / `barConfig` builds the plugins block for both per-day graphs. | per-day marks test compares both configs | | S-11 | `peakDay` returns `{date, hiddenAsUnreliable}`, and the tile reads that named flag. | peak test | | S-12 | An open day without data, or with no row at all, is `missing` in `heatmapModel` and renders as one hatched "NO DATA" band, not zero-traffic cells. | `an open day without data or without a row is a missing band…`, swatch render test | | S-13 | Removed the hard-coded `rgba(...)` values and both hatch gradients from the Week CSS. Legend lines use tokens. The hatch and estimate styles come from the shell's `.sd-swatch-*`. | swatch render test; CSS has no colour literals outside `var()` fallbacks | | C-1 | `referenceState` and `visitorsLegend`: a previous week without counted days draws no line. The legend reads "PREVIOUS WEEK — <reason>" (CLOSED_PERIOD when every day was closed), or "— COULD NOT BE LOADED" on failure. | `a reference week without data is not drawn…`, `a previous week without data draws no line…` | | C-2 | Entrance share shows name, bar and share only. The per-entrance change and the `≈` symbol are removed, along with the `changeVsPrevious` copy. The ◇/◆ on x labels stay (§4.1). | entrance mapping and render tests | | C-3 | Heatmap gap and Unreliable cells use `.sd-swatch-unreliable`, Estimate day labels use `.sd-swatch-estimate`, and the gap legend key is an `.sd-swatch`. | `the heatmap uses the shell's shared swatches…` | | C-4 | A peak with a null timestamp is shown undated ("PEOPLE INSIDE"), with no match on the count. | peak test | | C-5 | The PR text now says: at most 7 rows, all when ≤ 7, else the largest 6 plus one "N other entrances" row. That matches `ENTRANCE_ROWS`. | entrance test covers 6 and 9 entrances | | C-6 | No last-year data: no line on the Visitors tile, no last-year series or legend entry, and no last-year load. A last year whose change is withheld still shows "— VS LAST YEAR · <reason>". | `no data last year: the visitors tile hides the last-year line`, `no last-year data: no line, no legend entry and no last-year fetch` | **Verification** - `TZ=America/Caracas pytest -q`: 416 passed. - Node frontend suite: 159 passed, 33 of them Week tests. - `ruff check .`, `ruff format --check .` and `check_docs.py` are clean. - Playwright (Chromium) on port 8902 with the dev DB, and again with the stubbed rich fixture. Room, Desk and Laptop at 1920×1080, 1920×960 and 760×600, in es and en: no page, root, view, row or cell overflow, and the top bar is 38 px in all cases. I looked at the screenshots. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gabogg commented 2026-09-27 10:01:19 +00:00
Author
Owner
Copy link

Code review, second pass: PR #148 (Week) @ 5eb0543

All first-pass fixes (S-1–S-13, C-1–C-6) are verified; 76 node tests pass.

  • Shell API (#150): adopted correctly. The only direct fetch left is the hourly heatmap.
  • Maintainer decisions:
    • With no last year there is no tile line, no graph series, no legend entry, and no fetch.
    • The per-entrance change and "≈" are removed.

Standards

# Pri Finding
S2-1 P3 The view refetches Partial weeks ("can still gain data"), while the shell caches their day rows ("do not change within a page's life"). After a backfill, the KPIs refresh but the graphs stay stale. Pick one premise; following the shell is simplest.
S2-2 P3 Leftover fallbacks: ctx.locale || deckLocale(tr.lang) is dead, and WEEK_LAYOUTS[profile] || room is inconsistent with weekRequests.
S2-3 P3 numberParts repeats the digit-split regex of the shell's withData. Candidate for the planned consolidation.

Spec

# Pri Finding
C2-1 P3 When the summary fetch fails, the legend shows "LAST YEAR — COULD NOT BE LOADED" without knowing that last year exists. Name it only when the summary said it exists.

Verdict: mergeable. No P1 or P2; the P3s are filed as a follow-up issue.

🤖 Generated with Claude Code

# Code review, second pass: PR #148 (Week) @ 5eb0543 All first-pass fixes (S-1–S-13, C-1–C-6) are verified; 76 node tests pass. - **Shell API (#150):** adopted correctly. The only direct fetch left is the hourly heatmap. - **Maintainer decisions:** - With no last year there is no tile line, no graph series, no legend entry, and no fetch. - The per-entrance change and "≈" are removed. ## Standards | # | Pri | Finding | |---|---|---| | S2-1 | P3 | The view refetches Partial weeks ("can still gain data"), while the shell caches their day rows ("do not change within a page's life"). After a backfill, the KPIs refresh but the graphs stay stale. Pick one premise; following the shell is simplest. | | S2-2 | P3 | Leftover fallbacks: `ctx.locale \|\| deckLocale(tr.lang)` is dead, and `WEEK_LAYOUTS[profile] \|\| room` is inconsistent with `weekRequests`. | | S2-3 | P3 | `numberParts` repeats the digit-split regex of the shell's `withData`. Candidate for the planned consolidation. | ## Spec | # | Pri | Finding | |---|---|---| | C2-1 | P3 | When the **summary** fetch fails, the legend shows "LAST YEAR — COULD NOT BE LOADED" without knowing that last year exists. Name it only when the summary said it exists. | **Verdict:** mergeable. No P1 or P2; the P3s are filed as a follow-up issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gabogg force-pushed feat/deck-week-view from 5eb0543296
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m4s
Details
to 1e482a5a6f
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m7s
Details
2026-09-27 14:27:15 +00:00 Compare
gabogg commented 2026-09-27 14:27:24 +00:00
Author
Owner
Copy link

Rebased onto master after the Month view (#147) merged. I resolved the index.html and i18n.js conflicts: both views' CSS and imports are kept, and views.week sits next to views.month. For statisticsDeck.comparison.reason.*, master's Month wording is kept as the canonical set, per the maintainer's decision. The Week tests now assert that set (1e482a5). The full suite passes on top of master (416), and ruff and check_docs are clean.

**Rebased onto master** after the Month view (#147) merged. I resolved the `index.html` and `i18n.js` conflicts: both views' CSS and imports are kept, and `views.week` sits next to `views.month`. For `statisticsDeck.comparison.reason.*`, master's Month wording is kept as the canonical set, per the maintainer's decision. The Week tests now assert that set (1e482a5). The full suite passes on top of master (416), and ruff and check_docs are clean.
gabogg merged commit a365404068 into master 2026-09-27 14:27:27 +00:00
gabogg deleted branch feat/deck-week-view 2026-09-27 14:27:27 +00:00
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
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!148
No description provided.