diff --git a/docs/product/delivery/phase-06/reviews/P6.03-pr-review.triage.json b/docs/product/delivery/phase-06/reviews/P6.03-pr-review.triage.json new file mode 100644 index 0000000..0858882 --- /dev/null +++ b/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" + } +} diff --git a/docs/product/delivery/phase-06/reviews/P6.03-subagent-review.ledger.json b/docs/product/delivery/phase-06/reviews/P6.03-subagent-review.ledger.json new file mode 100644 index 0000000..aadd610 --- /dev/null +++ b/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 + } + ] +} diff --git a/docs/product/delivery/phase-06/reviews/P6.03-subagent-review.prompt.md b/docs/product/delivery/phase-06/reviews/P6.03-subagent-review.prompt.md new file mode 100644 index 0000000..d1c2981 --- /dev/null +++ b/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 ``. +- `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 `` 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 — | 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. diff --git a/docs/product/delivery/phase-06/reviews/P6.03-subagent-review.report.md b/docs/product/delivery/phase-06/reviews/P6.03-subagent-review.report.md new file mode 100644 index 0000000..e8ef41c --- /dev/null +++ b/docs/product/delivery/phase-06/reviews/P6.03-subagent-review.report.md @@ -0,0 +1,72 @@ +I now have all the information needed. Let me compile the adversarial review. + +--- + +**Invariant results** + +**Invariant 1** — broken. All five files carry ``, and four of them have zero legacy idioms. However, `ActivityChart.svelte` line 50 retains `on:update={onUpdate}` on the `` child component. This is the Svelte 4 `on:event` component-event syntax. In Svelte 5 runes mode, `on:event` on a component element is a deprecated legacy idiom; the Svelte compiler emits a `legacy_component_events` warning for exactly this pattern inside a runes-mode file. The `grep for legacy idioms returns zero` claim in the ticket outcome does not hold. + +**Invariant 2** — held. In all four ECharts files: `option` is `$derived`; `chart.setOption(option)` is called inside `$effect`; every `$effect` guards the uninitialized ref via either `chart?.setOption` (optional-chain, `ActivityChart`) or `if (!chartRef) return` (`BreakdownChart`, `StackedBarChart`, `WeekdaysBarChart`). Recipe structure is intact. + +**Invariant 3** — held. In `DailyTitleContent`: `date` and `isToday` are `$derived`; `currentTime` is `$state` mutated by the rAF loop in `updateTime()`; no `$effect` is used in place of a `$derived`. + +--- + +**Surface results** + +**S1 — ActivityChart local `$state(durationsProp)` initial-value capture** — probed. The prop is intentionally captured once; `onUpdate` is the sole re-sync path when `DailyChartControls` dispatches navigation events. The diff context and code confirm this is the designed pattern (independent date navigation). However, this design depends entirely on the `on:update` event bridge functioning correctly — which is the same `ActivityChart.svelte` line 50 finding in Invariant 1. If the `on:event` bridge is ever broken by a Svelte upgrade that removes backward compat, the component silently loses navigation updates and the stale-prop gap becomes live. Currently functional but fragile. + +**S2 — BreakdownChart `_isFiltered` prop alias pattern** — probed. `let isFiltered = $state(_isFiltered)` captures initial value; subsequent parent-side prop changes are dropped silently. Svelte 5 will emit a `state_referenced_locally` warning at runtime. Checked the test suite: both specs pass `isFiltered` only as a static mount-time prop and never re-render with a changed value, so the gap is untested. In practice, usages in pages appear to pass a static boolean, making the silent-drop a current non-issue. Svelte warning is live. + +**S3 — BreakdownChart `$effect` click handler re-registration** — probed. The `$effect` body is `if (!chart) { chart = echarts.init(...); chart.on('click', ...) }`. The `if (!chart)` guard fires only once (when `chart` is `undefined`), preventing duplicate handler registration on subsequent reactive fires. Clean. + +**S4 — StackedBarChart dispose-and-reinit frequency** — probed. Every reactive change disposes and reinits the chart instance. `onMount` registers `const handleResize = () => chart.resize()` — because this closes over the variable (not its snapshot), it correctly calls `.resize()` on whatever the current `chart` instance is after each reinit. The `onMount` cleanup disposes the current instance at unmount. No memory leak. However, on first render, `onMount` initialises the chart and calls `setOption(option)`, then `$effect` fires and immediately disposes that instance, reinits, and calls `setOption` again — doubling the init cost on every mount. Not a correctness gap, but wasteful. + +**S5 — `$effect` vs `onMount` race** — probed. Svelte 5 `$effect` runs after `onMount` in the browser. All four charts call `chart.setOption(option)` in `onMount` and again in `$effect`, meaning `setOption` is called twice on initial render. For `StackedBarChart` specifically, the `$effect` also disposes the `onMount`-created chart and reinits it. This is redundant but harmless — ECharts handles duplicate `setOption` and the chart ends up in the correct state. No stale-option risk: by the time `$effect` fires post-mount, `option` reflects the current props. + +**DS1 — Output stability across schema-version drift** — N/A. Purely in-memory component refactor; no persisted artifacts. + +**DS2 — CLI flag/arg symmetry** — N/A. No CLI flags or parsers modified. + +**DS3 — Error-class breadth in `catch` blocks** — N/A. No try/catch blocks in these five files. + +**DS4 — Defensive layering at module boundaries / null-safety of option builders** — probed. `createActiveHoursData` (`barChartHelpers.ts` line 217) calls `durations.data.reduce(...)` without a null guard. `ActivityChart.svelte` has no `hasData` conditional, so `chartRef` is always bound and `option` is always evaluated. The API server always returns at least `{ data: [] }` (never null), so in practice `durations.data` is always an array. Defensively fragile, but not a correctness gap under the current API contract. `createSimpleBarChartOption` guards with `if (!summaries.data) return {}` — safe. `createStackedBarChartOption` types its first parameter as `string[]` but `createXAxisValues` returns `string[] | undefined` (optional chaining on `summaries.data?.map`). TypeScript does not catch the undefined-to-string[] mismatch. In `StackedBarChart.svelte`, the `hasData` guard and `$effect`'s `if (!chartRef) return` prevent `setOption` being called with an undefined xAxis, so it does not reach ECharts in practice. Type gap is real; runtime exposure is guarded. + +**DS5 — Cross-file atomicity windows** — N/A. No multi-step file writes. + +**DS6 — Test-contract strength** — probed. Both `BreakdownChart.spec.ts` and `DailyTitleContent.spec.ts` use `render(Component, props)` with plain prop objects, which is compatible with Svelte 5 testing-library usage — no `$props()` API change is required on the test side. Tests exercise DOM behavior, not internal signal state. The `_isFiltered` local-state capture gap (S2) is not covered: no test re-renders with a changed `isFiltered` prop. The `on:update` event bridge in `ActivityChart` is not tested at all — no spec file exists for `ActivityChart`. + +**DS7 — Doc-vs-code drift in the ticket Rationale** — probed. The Rationale's claim "grep for legacy idioms in these files returns zero" fails: `ActivityChart.svelte` line 50 has `on:update={onUpdate}` on a component element. All other Rationale claims hold: T01 recipe is applied across all four ECharts charts; `DailyTitleContent` follows the presentational triage rule (`$derived`/`$state`, no `$effect`); the intentional removal of the re-sync `$effect` for `ActivityChart` is documented and matches the code. + +--- + +**Actionable findings** + +**Finding 1 — `ActivityChart.svelte` line 50: legacy `on:event` component binding breaks Invariant 1** + +File: `src/lib/components/BarChart/ActivityChart.svelte` + +`` uses the Svelte 4 `on:event` directive on a component element inside a `` file. Svelte 5 emits a `legacy_component_events` warning for this pattern in runes mode. The ticket invariant "grep for legacy idioms returns zero" is violated. This is a direct Invariant 1 breach. + +**Applies:** Invariant 1 ("zero remaining legacy idioms including `on:event`"). + +**Fix recommendation:** The correct Svelte 5 pattern is to pass a callback prop. `DailyChartControls.svelte` dispatches `update` events via `createEventDispatcher` — it is out of scope for this ticket, but a minimal forward-compat bridge is possible without migrating it: replace `on:update={onUpdate}` with `onupdate={onUpdate}` (lowercase event name as prop) IF `DailyChartControls` is migrated to expose an `onupdate` callback prop. Alternatively, document this as a known Svelte 5 compatibility bridge deferring to the `DailyChartControls` migration ticket and note it as a tracked exception in the ticket Rationale rather than claiming "zero legacy idioms." + +--- + +**Advisory Observations** + +**`DailyTitleContent` rAF loop leaks on unmount.** `onMount` assigns `const animationFrame = requestAnimationFrame(updateTime)` and returns `() => cancelAnimationFrame(animationFrame)`. But `updateTime` schedules a new frame recursively via `requestAnimationFrame(updateTime)` on every call. The cleanup only cancels the initial frame handle, not any subsequently scheduled frame (the loop was already running by the time cleanup fires). The loop continues indefinitely after unmount until the browser garbage-collects the closure. This appears to be a pre-existing bug carried over unchanged from the Svelte 4 version (behavior-preserving refactor), so it is outside the three finding-discipline clauses, but it is real. + +**`BreakdownChart` Svelte runtime warning for `state_referenced_locally`.** `let isFiltered = $state(_isFiltered)` will emit the Svelte 5 diagnostic `state_referenced_locally` at dev time. This is not a correctness gap (behavior is intentional), but it may produce noise in the dev console and could confuse future maintainers who encounter the warning without context. A suppression comment or a brief inline note would remove the ambiguity. + +**`StackedBarChart` double-init on first render.** `onMount` initialises the chart and calls `setOption`; immediately afterward `$effect` fires, disposes that instance, and reinits. On a single mount this costs two `echarts.init` + two `setOption` calls. This pattern differs from `WeekdaysBarChart` (lazy-init, no dispose) and `BreakdownChart` (conditional init, no dispose) in the same ticket. The inconsistency is not a bug, but it means `StackedBarChart` does the most work on mount for no benefit. This is outside the three finding-discipline clauses. + +**`createXAxisValues` return type diverges from `createStackedBarChartOption` parameter type.** `createXAxisValues` returns `string[] | undefined`; `createStackedBarChartOption` accepts `string[]`. TypeScript does not catch this because the assignment flows through `const xValues = $derived(createXAxisValues(summaries))` typed as the return type of the function. The runtime impact is guarded by the `hasData` conditional and `chartRef` guard, but the type gap means a future caller of `createStackedBarChartOption` could pass `undefined` silently. + +--- + +**Runner termination** + +runnerStatus: completed +terminatedReason: All invariants and attack surfaces probed to completion; one blocking finding and four advisory observations reported; no files written. diff --git a/docs/product/delivery/phase-06/ticket-03-barchart-family.md b/docs/product/delivery/phase-06/ticket-03-barchart-family.md index 8e5a065..429b10a 100644 --- a/docs/product/delivery/phase-06/ticket-03-barchart-family.md +++ b/docs/product/delivery/phase-06/ticket-03-barchart-family.md @@ -40,4 +40,4 @@ 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). -Contract note: — +Contract note: `ActivityChart.svelte` retains `on:update={onUpdate}` (Svelte 4 legacy event directive) because `DailyChartControls` is a Svelte 4 component not yet migrated to runes. TypeScript rejects `onupdate` as an unknown prop until T05 migrates `DailyChartControls` to expose the callback prop. This is a tracked cross-boundary compatibility bridge; the full cleanup lands in T05. The "grep for legacy idioms returns zero" outcome claim applies to all idioms that can be removed without breaking TypeScript at the cross-component boundary. diff --git a/src/lib/components/BarChart/ActivityChart.svelte b/src/lib/components/BarChart/ActivityChart.svelte index 826bb17..3546e30 100644 --- a/src/lib/components/BarChart/ActivityChart.svelte +++ b/src/lib/components/BarChart/ActivityChart.svelte @@ -1,22 +1,28 @@ + + diff --git a/src/lib/components/BarChart/BreakdownChart.svelte b/src/lib/components/BarChart/BreakdownChart.svelte index 05e50af..e899153 100644 --- a/src/lib/components/BarChart/BreakdownChart.svelte +++ b/src/lib/components/BarChart/BreakdownChart.svelte @@ -1,8 +1,10 @@ + +