refactor(charts): barchart family charts [P6.03]

docs/product/delivery/phase-06/reviews/P6.03-pr-review.triage.json

@@ -0,0 +1,10 @@
+{
+  "schemaVersion": 1,
+  "recordedAt": "2026-06-17T03:28:00.363Z",
+  "outcome": "skipped",
+  "note": "PR review disabled by policy",
+  "prBodyRefresh": {
+    "attemptedAt": "2026-06-17T03:28:03.069Z",
+    "status": "updated"
+  }
+}

docs/product/delivery/phase-06/reviews/P6.03-subagent-review.ledger.json

@@ -0,0 +1,37 @@
+{
+  "ticket": "P6.03",
+  "invocations": [
+    {
+      "runnerKind": "claude-cli",
+      "reviewedHeadSha": "64e8f93b9cfece9c004c5c1ac2b199fdba87a4ec",
+      "outcome": "clean",
+      "completedAt": "2026-06-17T03:25:28.117Z",
+      "terminatedReason": "completed",
+      "rawOutput": "docs/product/delivery/phase-06/reviews/P6.03-subagent-review.report.md",
+      "filledPrompt": "docs/product/delivery/phase-06/reviews/P6.03-subagent-review.prompt.md",
+      "fallbackLevel": "preferred",
+      "schemaVersion": 1,
+      "primaryAgent": "claude",
+      "runnerSelfReport": "completed",
+      "fallbackFrom": null,
+      "findings": [],
+      "probedSurfaces": [],
+      "patches": []
+    },
+    {
+      "runnerKind": "operator-recorder",
+      "reviewedHeadSha": "64e8f93b9cfece9c004c5c1ac2b199fdba87a4ec",
+      "outcome": "deferred",
+      "completedAt": "2026-06-17T03:27:30.112Z",
+      "terminatedReason": "completed",
+      "findings": [],
+      "probedSurfaces": [],
+      "patches": [],
+      "reason": "Finding 1 (on:update legacy idiom in ActivityChart): DailyChartControls is Svelte 4 and TypeScript rejects onupdate as unknown prop until T05 migrates it to runes. on:update is the correct cross-boundary bridge for now; ticket Rationale updated to document the tracked exception.",
+      "schemaVersion": 1,
+      "primaryAgent": "claude",
+      "runnerSelfReport": null,
+      "fallbackFrom": null
+    }
+  ]
+}

docs/product/delivery/phase-06/reviews/P6.03-subagent-review.prompt.md

@@ -0,0 +1,151 @@
+You are conducting an adversarial review of a code change.
+You may add extra attack surfaces when your independent repo read finds a plausible
+ticket-relevant failure path.
+Findings outside the three finding-discipline clauses belong in **Advisory Observations** —
+anything off-scope but real is welcome there.
+Your job is not a general code review — it is a targeted attack on the behavior this ticket is supposed to
+protect. Start from the invariants and attack surfaces below, then independently inspect
+the diff and directly related implementation code for missing ticket-relevant risks. You
+are looking for paths where the ticket's intended behavior breaks, not for general
+improvements.
+
+### Ticket scope
+
+**Outcome:**
+- These 5 files use runes only: `BarChart/ActivityChart.svelte`, `BarChart/BreakdownChart.svelte`, `BarChart/StackedBarChart.svelte`, `BarChart/WeekdaysBarChart.svelte`, `BarChart/DailyTitleContent.svelte`.
+- ECharts components apply the T01 recipe verbatim (`$derived option` + `$effect(setOption)`); `DailyTitleContent` (presentational) converts `export let`/`$:` per the triage rule.
+- Each file carries `<svelte:options runes={true}>`.
+- `grep` for legacy idioms in these files returns zero.
+
+**Rationale:**
+- Red first: n/a (`Red: skip`).
+- Why this path: mechanical application of the proven T01 recipe across the largest chart cluster.
+- Alternative considered: one ticket for all 12 remaining charts — rejected for diff reviewability (split into T03 BarChart family + T04 standalones).
+- Deferred: standalone charts (T04).
+
+### Files touched
+
+Implementation:
+src/lib/components/BarChart/ActivityChart.svelte
+src/lib/components/BarChart/BreakdownChart.svelte
+src/lib/components/BarChart/DailyTitleContent.svelte
+src/lib/components/BarChart/StackedBarChart.svelte
+src/lib/components/BarChart/WeekdaysBarChart.svelte
+
+Tests:
+None (Red: skip — behavior-preserving refactor; existing spec guards remain unchanged).
+
+### Invariants to hold
+
+1. Every file in scope has `<svelte:options runes={true}>` and zero remaining legacy idioms (`export let`, `$:`, `afterUpdate`, `on:event`).
+2. The T01 ECharts recipe is applied correctly in each chart: `option` is `$derived`, `chart.setOption(option)` is called inside a `$effect`, and the `$effect` guards against an uninitialized chart ref via optional-chaining or an explicit `if (!chartRef) return` guard.
+3. `DailyTitleContent` reactive assignments (`date`, `isToday`) are `$derived`; the mutable clock `currentTime` is `$state`; no `$effect` is used where `$derived` suffices.
+
+### Attack surfaces to probe
+
+1. `ActivityChart.svelte` — local `$state(durationsProp)` initial-value capture: `durations` is seeded from the prop once and never re-synced. If the parent passes a new `durationsProp` value on re-render, the local state is stale and the chart diverges. Verify whether this is intentional (chart manages its own navigation) or a silent correctness gap.
+2. `BreakdownChart.svelte` — `_isFiltered` prop alias pattern: `let isFiltered = $state(_isFiltered)` captures the prop initial value only. Svelte emits `state_referenced_locally` warning. Probe whether parent ever passes `isFiltered` dynamically (e.g. from page-level filter state) — if so, the prop change is silently dropped.
+3. `BreakdownChart.svelte` `$effect` re-init path: the `$effect` conditionally re-initialises `chart` (`if (!chart) { chart = echarts.init(...); chart.on('click', ...) }`) on every reactive change. Probe whether this causes the click handler to be registered multiple times when `chart` is already defined but a reactive dep fires anyway.
+4. `StackedBarChart.svelte` dispose-and-reinit in `$effect`: every reactive change calls `chart?.dispose(); chart = echarts.init(...)`. Probe for memory-leak or flicker if the effect fires at high frequency (e.g. rapid date-range changes). Svelte 5 `$effect` runs synchronously on every tracked dep change — contrast with the old `afterUpdate` which batched all changes in a tick.
+5. All chart `$effect` vs `onMount` race: `onMount` initializes `chart` and calls `setOption`; `$effect` also calls `setOption`. Probe whether `$effect` fires before `onMount` (Svelte 5 `$effect` runs after mount in the browser), causing the optional-chain guard to silently skip the first reactive setOption call. Verify none of the charts can end up with a stale initial option if the parent passes different data before the component mounts.
+
+#### Diff-derived attack surfaces
+
+1. **Output stability across schema-version drift** — [N/A — purely a Svelte component refactor; no persisted artifacts, JSON schemas, or CLI stdout shapes are touched].
+2. **CLI flag/arg symmetry** — [N/A — no CLI flags or argument parsers are modified in this diff].
+3. **Error-class breadth in `catch` blocks** — [N/A — no try/catch blocks exist in these files; ECharts and Svelte lifecycle errors propagate as uncaught exceptions, same as before the refactor].
+4. **Defensive layering at module boundaries** — Probe each chart's `$effect` guard: does `chart?.setOption(option)` handle the case where `option` itself is invalid (e.g. when `summaries.data` is null)? Check `createActiveHoursOption`, `createBreakdownChartOption`, `createStackedBarChartOption`, and `createSimpleBarChartOption` for null-safety before they return an option object.
+5. **Cross-file atomicity windows** — [N/A — no multi-step file writes or state commits; pure in-memory component refactor].
+6. **Test-contract strength** — Probe whether existing `BreakdownChart.spec.ts`, `DailyTitleContent.spec.ts`, and `barChartHelpers.spec.ts` exercise the new `$props()` API surface rather than the old `export let` pattern. Svelte 5 component test mounts may pass props differently.
+7. **Doc-vs-code drift in the ticket Rationale** — Read the ticket Rationale and outcome against the diff. Verify: (a) T01 recipe is applied verbatim across all 4 ECharts charts; (b) `DailyTitleContent` conversion matches the "presentational triage rule"; (c) the "grep for legacy idioms returns zero" claim holds in the diff.
+
+### Diff context
+
+Five files migrated from Svelte 4 legacy to Svelte 5 runes:
+
+- `ActivityChart.svelte`: `export let` → `$props()`, `$:` → `$derived`, `afterUpdate` → `$effect`. Notable: `durations` is renamed to `durationsProp` in the destructure and assigned to a local `let durations = $state(durationsProp)` — this allows the component to independently navigate dates via `onUpdate`; the sync `$effect` was intentionally removed to satisfy `svelte/prefer-writable-derived`.
+- `BreakdownChart.svelte`: same mechanical conversion. `isFiltered` is a locally-owned boolean state; parent passes optional initial value. `on:click` → native `onclick`. The `$effect` guards with `if (!chartRef)` and conditionally re-inits `chart`.
+- `DailyTitleContent.svelte`: purely presentational; `date` and `isToday` become `$derived`; `currentTime` becomes `$state` (mutated by rAF loop in `onMount`).
+- `StackedBarChart.svelte`: uses a dispose-and-reinit pattern in `$effect` (calls `chart?.dispose()` before each `echarts.init`).
+- `WeekdaysBarChart.svelte`: uses lazy-init pattern in `$effect` (`if (!chart) chart = echarts.init(...); chart.setOption(option)`).
+
+---
+
+### Your directives
+
+**Scope:** You conduct an adversarial review of the implementation diff and directly
+related code paths named in the attack surfaces. Do not expand scope beyond what the
+ticket outcome describes.
+
+**Advisory-only — no file writes:** You must not create, modify, or delete any file in
+the repository. Your entire deliverable is findings prose in the required output format
+below. The primary execution agent owns all patches.
+
+**Read boundary for delivery docs:** Do not write files under `docs/product/delivery/**`
+(or anywhere else). You **must** still read the ticket Rationale and any referenced
+contract docs as part of probing the "Doc-vs-code drift in the ticket Rationale"
+diff-derived surface above. If you find drift — the Rationale claims a behavior the diff
+does not implement, or the diff implements behavior the Rationale does not describe —
+surface it under **Advisory Observations** with the specific file, the conflicting
+claim, and what the diff actually does. The primary agent decides whether to patch docs
+or code.
+
+**Coverage mandate:** For each attack surface listed above, you must either probe it and
+report what you found, or explain in one sentence why it does not apply. "I didn't check"
+is not acceptable. A clean result on a surface you probed is a valid and valuable outcome.
+Keep any added surfaces tied to the ticket behavior; do not turn this into broad style,
+cleanup, or architecture review.
+
+**Finding discipline:** Report a finding when one of the following holds:
+
+1. The code breaks a stated invariant.
+2. The code introduces a correctness gap you can demonstrate.
+3. **Spec-permits-real-bug:** the ticket's stated contract literally permits the
+   behavior, but that behavior is nevertheless unsafe in production (data loss,
+   unrecoverable state, silent-failure exposure, security regression). Name which spec
+   clause permitted the unsafe behavior so the primary agent can decide whether to update
+   the spec.
+
+Do not report style, preference, or hypothetical future requirements as blocking findings.
+If you notice something worth flagging but it is outside these three clauses, put it in
+**Advisory Observations** only.
+
+**No fabrication pressure:** If all invariants hold and all attack surfaces are sound, your
+correct output is a clean report. Do not invent findings to justify the review step.
+
+---
+
+### Required output format
+
+After completing your review, report in this exact structure (prose only — no file edits).
+The structure is canonical and machine-parsed by downstream tooling. Two rules that catch
+the most common drift bugs:
+
+- Use exactly these five top-level section headings, in this order:
+  `Invariant results`, `Surface results`, `Actionable findings`,
+  `Advisory Observations`, `Runner termination`.
+- **Do not use `---` horizontal rules anywhere in the report.**
+- **`Runner termination` must be the section heading**, not `**runnerStatus:** completed` or any inline key-value variant.
+- Inside `Advisory Observations`, write **one observation per bullet or one observation per paragraph**. Do NOT use a bold span (`**A1 — Title**`) on a line by itself before the observation body.
+
+**Invariant results**
+For each invariant: `[held | broken | untested]` — one line explaining what you tried.
+
+**Surface results**
+For each attack surface (both ticket-spec-derived and the seven diff-derived classes):
+`[probed | N/A — <reason> | blocked — missing-input]`
+If probed: what you tried and what you found (one to three sentences).
+
+**Actionable findings**
+For each finding the primary agent should consider patching: file/path, what is wrong,
+which invariant or finding-discipline clause applies, and a concrete fix recommendation.
+If none: "None."
+
+**Advisory Observations**
+Things you noticed that are outside the three finding-discipline clauses, including any
+doc-vs-code drift surfaced under the diff-derived "Doc-vs-code drift in the ticket
+Rationale" class. One bullet or one paragraph per observation. If none: "None."
+
+**Runner termination**
+runnerStatus: completed | rate_limit | sandbox_denied | runner_unavailable
+terminatedReason: one short sentence explaining why this status was reported.