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

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

Merged
gabogg merged 3 commits from fix/deck-shell-view-support into master 2026-09-27 09:57:07 +00:00
Conversation 3 Commits 3 Files changed 5 +189 -14
gabogg commented 2026-09-26 23:13:34 +00:00
Owner
Copy link

Problem

Reviewing the period-view PRs (#147 Month, #148 Week, #149 Day) surfaced gaps in the shell from #144 that every view hits:

  1. Empty states. Once a view is registered, the shell called it even with no period, a failed period load, or a fully closed period. Views couldn't tell a load failure from "no periods".
  2. Another period's days. Views need the previous period's daily rows, so they built their own URLs and caches next to the shell's loadDays.
  3. Locale. Chart.js ticks used the browser locale instead of the deck language.
  4. Tokens. Views needed colours (cyan, text primary and dim) that charts.js didn't resolve, so each re-read the tokens itself.

API for views (render(ctx))

ctx member Behaviour
period Always a real, open period. The shell renders its own notice for a failed load, no periods, or a closed period, and doesn't call the view.
loadDays() Daily rows of the shown period, cached per page. Rejects when the fetch fails (not cached), so a cell can show "could not be loaded".
loadPeriodDays(period) Daily rows of another period through the same cache. Rejects for a missing period.
periodFor(start) / previousPeriod() Route-shaped {granularity, start, end} periods (end exclusive), built with periodEnd/previousPeriodStart from periods.js. No need for the paged period list.
createChart(canvas, config) createDeckChart with options.locale set to the deck language; a view's own locale wins.
locale es-VE / en-US, for Intl.NumberFormat.

currentDeckColors() / resolveDeckColors() also resolve dim, primary and cyan.

Adoption checklist for #147, #148 and #149

  • Call ctx.createChart(...) instead of createDeckChart(ctx.Chart, ...) and deckChartOptions directly.
  • Replace hand-built daily-route URLs and view caches (Month previousDays, Week dailyUrl) with ctx.loadPeriodDays(ctx.previousPeriod()) / ctx.periodFor(start).
  • Handle the loadDays rejection in the cell.
  • Drop the view-owned no-period / closed guards and token re-reads.

Verification

  • 43 deck frontend tests pass (7 new here): shell-owned empty/error/closed states, explicit vs other-period loading with one cache, rejection and retry, missing-period rejection, month/year period edges, and locale on real chart configs.
  • The full pytest suite passes.
  • Two review passes: #150 comments 2723 (first review, fix mapping after it) and the second-pass comment.

🤖 Generated with Claude Code

## Problem Reviewing the period-view PRs (#147 Month, #148 Week, #149 Day) surfaced gaps in the shell from #144 that every view hits: 1. **Empty states.** Once a view is registered, the shell called it even with no period, a failed period load, or a fully closed period. Views couldn't tell a load failure from "no periods". 2. **Another period's days.** Views need the previous period's daily rows, so they built their own URLs and caches next to the shell's `loadDays`. 3. **Locale.** Chart.js ticks used the browser locale instead of the deck language. 4. **Tokens.** Views needed colours (`cyan`, text `primary` and `dim`) that `charts.js` didn't resolve, so each re-read the tokens itself. ## API for views (`render(ctx)`) | ctx member | Behaviour | |---|---| | `period` | Always a real, open period. The shell renders its own notice for a failed load, no periods, or a closed period, and doesn't call the view. | | `loadDays()` | Daily rows of the shown period, cached per page. **Rejects** when the fetch fails (not cached), so a cell can show "could not be loaded". | | `loadPeriodDays(period)` | Daily rows of another period through the same cache. Rejects for a missing period. | | `periodFor(start)` / `previousPeriod()` | Route-shaped `{granularity, start, end}` periods (`end` exclusive), built with `periodEnd`/`previousPeriodStart` from `periods.js`. No need for the paged period list. | | `createChart(canvas, config)` | `createDeckChart` with `options.locale` set to the deck language; a view's own `locale` wins. | | `locale` | `es-VE` / `en-US`, for `Intl.NumberFormat`. | `currentDeckColors()` / `resolveDeckColors()` also resolve `dim`, `primary` and `cyan`. ## Adoption checklist for #147, #148 and #149 - [ ] Call `ctx.createChart(...)` instead of `createDeckChart(ctx.Chart, ...)` and `deckChartOptions` directly. - [ ] Replace hand-built daily-route URLs and view caches (Month `previousDays`, Week `dailyUrl`) with `ctx.loadPeriodDays(ctx.previousPeriod())` / `ctx.periodFor(start)`. - [ ] Handle the `loadDays` rejection in the cell. - [ ] Drop the view-owned no-period / closed guards and token re-reads. ## Verification - 43 deck frontend tests pass (7 new here): shell-owned empty/error/closed states, explicit vs other-period loading with one cache, rejection and retry, missing-period rejection, month/year period edges, and locale on real chart configs. - The full `pytest` suite passes. - Two review passes: #150 comments 2723 (first review, fix mapping after it) and the second-pass comment. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fix(statistics-deck): shell support for the period views (#134-#136 reviews)
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m8s
Details
33cbe9b626 
Gaps found while reviewing the Day/Week/Month view PRs (#147, #148):

- A registered view now renders only a real, open period. The shell keeps
  its own notices for a failed period load, no periods and a fully closed
  period, so views no longer mistake a load failure for "no data".
- ctx.loadDays(other) loads another period's daily rows (e.g. the previous
  month) through the shell's single cache instead of a per-view one.
- ctx.locale / deckLocale(lang) let graph ticks and numbers follow the
  deck language; deckChartOptions passes `locale` through.
- COLOR_TOKENS gains dim, primary, cyan and surface, so views read every
  colour from design tokens.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-27 09:28:43 +00:00
Author
Owner
Copy link

Code review: PR #150 (fix/deck-shell-view-support @ 33cbe9b vs merge base 08b6d1c)

Small PR, both axes in one review pass. Standards: AGENTS.md, code-standards.md, ui-design-guidelines.md, ADR 0002, and the Fowler smell baseline. Spec: the four problems in the PR description, which came from the #147 and #148 reviews, checked against how the three view PRs would adopt the new API.

Standards

No hard violations. The hex fallbacks match the §2.1 tokens, and 39 tests pass.

# Pri Finding
S-1 P2 loadDays: (other = period) => …: a default parameter only kicks in for undefined, so ctx.loadDays(periods.find(...)) on a miss silently returns the shown period's days as the "other" period. Make the other-period call explicit.
S-2 P3 The cache comment ("closed business days do not change") conflates the glossary's Closed Day with a complete period. The claim holds, but reword it.
S-3 P3 Possible Duplicated Code: LOCALES = { es, en } repeats DECK_LANGS, so a new language would silently fall back to es-VE.
S-4 P3 Possible Speculative Generality: no view uses the surface token.
S-5 P3 Test gaps: the "no periods" path, cache reuse, and undefined vs null for loadDays. The locale test is shallow.

Spec

# Pri Finding
C-1 P2 Problem 2 is only partly solved: views have no way to get the other period's object. ctx exposes neither the period list nor a previous-period helper, and the previous period may not be in the loaded (paged) list at all. Views would have to hand-build {granularity, start, end} against an undocumented contract.
C-2 P2 A failed days fetch still resolves to [], so views can't show "could not be loaded" (#148 S-7 depends on this).
C-3 P3 Locale is solved but awkward: every view must add options.locale to every chart. Set it once, e.g. through a ctx-bound chart factory.
— — Problems 1 (empty states) and 4 (tokens) are solved; the views' guards and token copies can be deleted. No scope creep beyond S-4.

Summary. Standards: 5 findings, worst S-1 (loadDays(undefined) returns the wrong period). Spec: 3 findings, worst C-1/C-2 (no way to build the other period; failures look like no data). First pass, so all findings get fixed on this branch.

🤖 Generated with Claude Code

# Code review: PR #150 (`fix/deck-shell-view-support` @ 33cbe9b vs merge base 08b6d1c) Small PR, both axes in one review pass. **Standards**: AGENTS.md, `code-standards.md`, `ui-design-guidelines.md`, ADR 0002, and the Fowler smell baseline. **Spec**: the four problems in the PR description, which came from the #147 and #148 reviews, checked against how the three view PRs would adopt the new API. ## Standards No hard violations. The hex fallbacks match the §2.1 tokens, and 39 tests pass. | # | Pri | Finding | |---|---|---| | S-1 | **P2** | `loadDays: (other = period) => …`: a default parameter only kicks in for `undefined`, so `ctx.loadDays(periods.find(...))` on a miss silently returns the **shown** period's days as the "other" period. Make the other-period call explicit. | | S-2 | P3 | The cache comment ("closed business days do not change") conflates the glossary's *Closed Day* with a complete period. The claim holds, but reword it. | | S-3 | P3 | Possible Duplicated Code: `LOCALES = { es, en }` repeats `DECK_LANGS`, so a new language would silently fall back to es-VE. | | S-4 | P3 | Possible Speculative Generality: no view uses the `surface` token. | | S-5 | P3 | Test gaps: the "no periods" path, cache reuse, and `undefined` vs `null` for `loadDays`. The locale test is shallow. | ## Spec | # | Pri | Finding | |---|---|---| | C-1 | **P2** | Problem 2 is only partly solved: views have no way to get the other period's object. `ctx` exposes neither the period list nor a previous-period helper, and the previous period may not be in the loaded (paged) list at all. Views would have to hand-build `{granularity, start, end}` against an undocumented contract. | | C-2 | **P2** | A failed days fetch still resolves to `[]`, so views can't show "could not be loaded" (#148 S-7 depends on this). | | C-3 | P3 | Locale is solved but awkward: every view must add `options.locale` to every chart. Set it once, e.g. through a ctx-bound chart factory. | | — | — | Problems 1 (empty states) and 4 (tokens) are solved; the views' guards and token copies can be deleted. No scope creep beyond S-4. | --- **Summary.** Standards: 5 findings, worst **S-1** (`loadDays(undefined)` returns the wrong period). Spec: 3 findings, worst **C-1/C-2** (no way to build the other period; failures look like no data). First pass, so all findings get fixed on this branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gabogg commented 2026-09-27 09:32:58 +00:00
Author
Owner
Copy link

Review fixes: 1c77ee1

Every finding from the review is addressed.

# Resolution
S-1 (P2) Fixed. ctx.loadDays() takes no argument and always loads the shown period; ctx.loadPeriodDays(period) is the explicit call for another period. Tested.
S-2 Fixed. The comment now says that every listed period is a complete (closed) period, Partial ones included, so its rows don't change within a page's life.
S-3 Fixed. DECK_LANGS = Object.keys(LOCALES), so one map drives both.
S-4 Fixed. The unused surface token is dropped.
S-5 Fixed. New tests for the no-periods path, cache reuse across two loadDays() calls, the explicit other-period call, rejection and retry, and the locale on real chart configs.
C-1 (P2) Fixed. periods.js exports periodEnd, previousPeriodStart and periodFor, mirroring the server's exclusive ends (month and year boundaries tested). ctx.periodFor(start) and ctx.previousPeriod() return route-shaped {granularity, start, end} objects without the paged list.
C-2 (P2) Fixed. loadDays rejects when the fetch fails and doesn't cache the failure. The detail panel already catches it and shows "could not be loaded"; views can now do the same. Tested (fail, then retry).
C-3 Fixed. ctx.createChart(canvas, config) wraps createDeckChart and sets options.locale from the deck language, so views don't set it per chart. Tested in both languages.

Verification: 43 deck tests pass (5 new); the full pytest suite passes.

🤖 Generated with Claude Code

## Review fixes: 1c77ee1 Every finding from the [review](https://git.gaboggamer.online/gabogg/hikcentral/pulls/150#issuecomment-2723) is addressed. | # | Resolution | |---|---| | S-1 (P2) | **Fixed.** `ctx.loadDays()` takes no argument and always loads the shown period; `ctx.loadPeriodDays(period)` is the explicit call for another period. Tested. | | S-2 | **Fixed.** The comment now says that every listed period is a complete (closed) period, Partial ones included, so its rows don't change within a page's life. | | S-3 | **Fixed.** `DECK_LANGS = Object.keys(LOCALES)`, so one map drives both. | | S-4 | **Fixed.** The unused `surface` token is dropped. | | S-5 | **Fixed.** New tests for the no-periods path, cache reuse across two `loadDays()` calls, the explicit other-period call, rejection and retry, and the locale on real chart configs. | | C-1 (P2) | **Fixed.** `periods.js` exports `periodEnd`, `previousPeriodStart` and `periodFor`, mirroring the server's exclusive ends (month and year boundaries tested). `ctx.periodFor(start)` and `ctx.previousPeriod()` return route-shaped `{granularity, start, end}` objects without the paged list. | | C-2 (P2) | **Fixed.** `loadDays` rejects when the fetch fails and doesn't cache the failure. The detail panel already catches it and shows "could not be loaded"; views can now do the same. Tested (fail, then retry). | | C-3 | **Fixed.** `ctx.createChart(canvas, config)` wraps `createDeckChart` and sets `options.locale` from the deck language, so views don't set it per chart. Tested in both languages. | **Verification:** 43 deck tests pass (5 new); the full `pytest` suite passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fix(statistics-deck): address code-review findings for PR #150
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m5s
Details
1c77ee1bae 
- ctx.loadDays() is the shown period only; ctx.loadPeriodDays(period)
  loads another one explicitly, so an undefined argument can no longer
  silently return the shown period's days.
- ctx.periodFor(start) / ctx.previousPeriod() build route-shaped periods
  ({granularity, start, end exclusive}) with periodEnd/previousPeriodStart,
  without depending on the paged period list.
- loadDays rejects when the fetch fails (not cached), so views can show
  "could not be loaded"; the detail panel already catches it.
- ctx.createChart(canvas, config) sets the deck locale on every graph.
- DECK_LANGS derives from the locale map; the unused `surface` token is
  dropped; the cache comment speaks of complete periods, not Closed Days.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fix(statistics-deck): loadPeriodDays rejects for a missing period (#150 review)
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m4s
Details
ef77fc63de 
A find() miss passed undefined, and period.granularity threw
synchronously, escaping Promise.allSettled in the views.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-27 09:37:41 +00:00
Author
Owner
Copy link

Code review, second pass: PR #150 (@ 1c77ee1)

All 8 first-pass findings (S-1–S-5, C-1–C-3) are verified fixed, with no regressions; 43/43 deck tests pass. periodEnd/previousPeriodStart match the server's strategies, including the month and year edges. A view's own options.locale wins in createChart. There is no import cycle. Every loadDays caller handles the rejection.

Standards

# Pri Finding Outcome
S2-1 P3 loadPeriodDays(undefined) threw synchronously (on a find miss), escaping Promise.allSettled. Fixed in ef77fc6. An exception to the P3-to-issue rule: the view PRs are adopting this API right now.
S2-2 P3 Client period helpers don't validate alignment as the server does. Filed as #153
S2-3 P3 periodFor/previousPeriod read this.granularity at call time. Filed as #153

Spec

# Pri Finding Outcome
C2-1 P2 The PR description documented the pre-review API. Fixed: the description is rewritten with the ctx API table.
C2-2 P3 The adoption steps for #147, #148 and #149 were implicit. Fixed: there is now an adoption checklist in the description.

Summary. No P1. The one P2 is fixed; the remaining P3s are filed. Ready to merge on the maintainer's go.

🤖 Generated with Claude Code

# Code review, second pass: PR #150 (@ 1c77ee1) All 8 first-pass findings (S-1–S-5, C-1–C-3) are verified fixed, with no regressions; 43/43 deck tests pass. `periodEnd`/`previousPeriodStart` match the server's strategies, including the month and year edges. A view's own `options.locale` wins in `createChart`. There is no import cycle. Every `loadDays` caller handles the rejection. ## Standards | # | Pri | Finding | Outcome | |---|---|---|---| | S2-1 | P3 | `loadPeriodDays(undefined)` threw synchronously (on a `find` miss), escaping `Promise.allSettled`. | **Fixed in ef77fc6.** An exception to the P3-to-issue rule: the view PRs are adopting this API right now. | | S2-2 | P3 | Client period helpers don't validate alignment as the server does. | Filed as https://git.gaboggamer.online/gabogg/hikcentral/issues/153 | | S2-3 | P3 | `periodFor`/`previousPeriod` read `this.granularity` at call time. | Filed as https://git.gaboggamer.online/gabogg/hikcentral/issues/153 | ## Spec | # | Pri | Finding | Outcome | |---|---|---|---| | C2-1 | **P2** | The PR description documented the pre-review API. | **Fixed:** the description is rewritten with the ctx API table. | | C2-2 | P3 | The adoption steps for #147, #148 and #149 were implicit. | **Fixed:** there is now an adoption checklist in the description. | **Summary.** No P1. The one P2 is fixed; the remaining P3s are filed. Ready to merge on the maintainer's go. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gabogg merged commit 5bcdae741f into master 2026-09-27 09:57:07 +00:00
gabogg deleted branch fix/deck-shell-view-support 2026-09-27 09:57:07 +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!150
No description provided.