diff --git a/docs/product/delivery/phase-06/implementation-plan.md b/docs/product/delivery/phase-06/implementation-plan.md index 9b63f3de..1ef9e589 100644 --- a/docs/product/delivery/phase-06/implementation-plan.md +++ b/docs/product/delivery/phase-06/implementation-plan.md @@ -66,17 +66,6 @@ Semantic note: `afterUpdate` fires after _any_ component update; `$effect` fires - `ticket-07-routes-and-remaining-leaves.md` - `ticket-08-global-flip-and-retro.md` -## File-to-ticket map (51 files) - -- **T01 (1):** `BarChart/VerticalBarChart.svelte` -- **T02 (9):** `Container.svelte`, `ChartTitle.svelte`, `common/ChartContainer.svelte`, `common/BigChartContainer.svelte`, `Stats/StatPanelItem.svelte`, `PageTransition.svelte`, `assets/svg/{Moon,Sun,System}.svelte` -- **T03 (5):** `BarChart/ActivityChart.svelte`, `BarChart/BreakdownChart.svelte`, `BarChart/StackedBarChart.svelte`, `BarChart/WeekdaysBarChart.svelte`, `BarChart/DailyTitleContent.svelte` -- **T04 (8):** `LineChart/LineChart.svelte`, `PieChart/PieChart.svelte`, `ScatterPlot/ScatterPlot.svelte`, `Treemap/Treemap.svelte`, `TimelineChart/TimelineChart.svelte`, `GaugeChart/DailyGauge.svelte`, `AiTokenBarChart/AiTokenBarChart.svelte`, `AiLinesPieChart/AiLinesPieChart.svelte` -- **T05 (4):** `DateRangeSelect.svelte`, `BarChart/DailyChartControls.svelte`, `GaugeChart/DailyGaugeControls.svelte`, `DarkModeToggle.svelte` -- **T06 (7):** `Navbar/{Navbar,NavDropdown,NavEnd,NavLink,NavLinks,NavLogo,NavMenu}.svelte` -- **T07 (17):** `routes/+error.svelte`, `routes/+layout.svelte`, `routes/+page.svelte`, `routes/AiStatPanel.svelte`, `routes/account/+page.svelte`, `routes/iterations/+page.svelte`, `routes/iterations/[iterationId]/+page.svelte`, `routes/login-redirect/+page.svelte`, `routes/login/+page.svelte`, `routes/projects/+page.svelte`, `routes/projects/[projectName]/+page.svelte`, `Footer.svelte`, `ProjectList.svelte`, `EmptyState.svelte`, `Stats/StatsPanel.svelte`, `common/NinjaSpinner.svelte`, `common/SearchInput.svelte` -- **T08 (0 conversions):** `svelte.config.js` + strip all `` directives + retrospective - ## Exit Condition 1. Repo-wide grep for all six legacy idioms returns zero hits across `src`. @@ -112,3 +101,14 @@ Semantic note: `afterUpdate` fires after _any_ component update; `$effect` fires ## Phase Closeout Retrospective: `required` (per product plan — `runes: true` is a durable architectural boundary changing every future component PR's assumptions; captures reusable migration patterns/gotchas). Written in P6.08. + +## File-to-ticket map (51 files) + +- **T01 (1):** `BarChart/VerticalBarChart.svelte` +- **T02 (9):** `Container.svelte`, `ChartTitle.svelte`, `common/ChartContainer.svelte`, `common/BigChartContainer.svelte`, `Stats/StatPanelItem.svelte`, `PageTransition.svelte`, `assets/svg/{Moon,Sun,System}.svelte` +- **T03 (5):** `BarChart/ActivityChart.svelte`, `BarChart/BreakdownChart.svelte`, `BarChart/StackedBarChart.svelte`, `BarChart/WeekdaysBarChart.svelte`, `BarChart/DailyTitleContent.svelte` +- **T04 (8):** `LineChart/LineChart.svelte`, `PieChart/PieChart.svelte`, `ScatterPlot/ScatterPlot.svelte`, `Treemap/Treemap.svelte`, `TimelineChart/TimelineChart.svelte`, `GaugeChart/DailyGauge.svelte`, `AiTokenBarChart/AiTokenBarChart.svelte`, `AiLinesPieChart/AiLinesPieChart.svelte` +- **T05 (4):** `DateRangeSelect.svelte`, `BarChart/DailyChartControls.svelte`, `GaugeChart/DailyGaugeControls.svelte`, `DarkModeToggle.svelte` +- **T06 (7):** `Navbar/{Navbar,NavDropdown,NavEnd,NavLink,NavLinks,NavLogo,NavMenu}.svelte` +- **T07 (17):** `routes/+error.svelte`, `routes/+layout.svelte`, `routes/+page.svelte`, `routes/AiStatPanel.svelte`, `routes/account/+page.svelte`, `routes/iterations/+page.svelte`, `routes/iterations/[iterationId]/+page.svelte`, `routes/login-redirect/+page.svelte`, `routes/login/+page.svelte`, `routes/projects/+page.svelte`, `routes/projects/[projectName]/+page.svelte`, `Footer.svelte`, `ProjectList.svelte`, `EmptyState.svelte`, `Stats/StatsPanel.svelte`, `common/NinjaSpinner.svelte`, `common/SearchInput.svelte` +- **T08 (0 conversions):** `svelte.config.js` + strip all `` directives + retrospective diff --git a/docs/product/delivery/phase-06/reviews/P6.01-pr-review.triage.json b/docs/product/delivery/phase-06/reviews/P6.01-pr-review.triage.json new file mode 100644 index 00000000..c37c65b5 --- /dev/null +++ b/docs/product/delivery/phase-06/reviews/P6.01-pr-review.triage.json @@ -0,0 +1,10 @@ +{ + "schemaVersion": 1, + "recordedAt": "2026-06-17T02:57:28.055Z", + "outcome": "skipped", + "note": "PR review disabled by policy", + "prBodyRefresh": { + "attemptedAt": "2026-06-17T02:57:31.125Z", + "status": "updated" + } +} diff --git a/docs/product/delivery/phase-06/reviews/P6.01-subagent-review.ledger.json b/docs/product/delivery/phase-06/reviews/P6.01-subagent-review.ledger.json new file mode 100644 index 00000000..032a3dff --- /dev/null +++ b/docs/product/delivery/phase-06/reviews/P6.01-subagent-review.ledger.json @@ -0,0 +1,22 @@ +{ + "ticket": "P6.01", + "invocations": [ + { + "runnerKind": "claude-cli", + "reviewedHeadSha": "56a0efa8cefdc188af62e73a556263ed5fa9764a", + "outcome": "clean", + "completedAt": "2026-06-17T02:56:52.814Z", + "terminatedReason": "completed", + "rawOutput": "docs/product/delivery/phase-06/reviews/P6.01-subagent-review.report.md", + "filledPrompt": "docs/product/delivery/phase-06/reviews/P6.01-subagent-review.prompt.md", + "fallbackLevel": "preferred", + "schemaVersion": 1, + "primaryAgent": "claude", + "runnerSelfReport": "completed", + "fallbackFrom": null, + "findings": [], + "probedSurfaces": [], + "patches": [] + } + ] +} diff --git a/docs/product/delivery/phase-06/reviews/P6.01-subagent-review.prompt.md b/docs/product/delivery/phase-06/reviews/P6.01-subagent-review.prompt.md new file mode 100644 index 00000000..51cdfd2e --- /dev/null +++ b/docs/product/delivery/phase-06/reviews/P6.01-subagent-review.prompt.md @@ -0,0 +1,167 @@ +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:** +- `BarChart/VerticalBarChart.svelte` uses runes only: `export let` → `$props()`, `$:` derived values → `$derived`, `afterUpdate(() => chart.setOption(option))` → `$effect(() => { chart?.setOption(option) })`. +- File carries `` so it compiles in rune mode under the default mixed compiler. +- The documented ECharts recipe (in this ticket's Rationale and the implementation plan) is validated and ready for T03/T04 to apply. +- `grep` for legacy idioms in this file returns zero. +- Chart still initializes on mount, redraws on data change, and the `chart.on('click', …)` story-branch handler still works. + +**Rationale notes:** +- `afterUpdate` fired after every component update; `$effect` fires only when `option`'s reactive dependencies change. This is the intended behavior (redraw on data change) and is the single highest-risk conversion in the phase. +- The `$effect` guard `chart?.setOption` exists because `onMount` init and the effect can interleave differently than `afterUpdate` did. +- No net-new characterization tests (deferred per product plan). Manual smoke + existing tests are the guard. +- `Red: skip` — behavior-preserving conversion. + +### Files touched + +Implementation: +- `src/lib/components/BarChart/VerticalBarChart.svelte` + +Tests: +- (none changed) + +### Invariants to hold + +1. The chart must redraw when `summaries` or `title` props change — specifically, `chart.setOption(option)` must be called whenever the reactive `option` value changes after initial mount. +2. No "setOption before init" runtime error must occur — `chart` must be defined before `setOption` is called, or the call is safely guarded. +3. No legacy idioms (`export let`, `$:`, `afterUpdate`) must remain in `VerticalBarChart.svelte`. + +### Attack surfaces to probe + +1. **`$effect` vs `onMount` ordering** — does the `$effect` run before `chart` is initialized by `onMount`, causing a silent no-op on the very first render? If the initial `onMount` call to `chart.setOption(option)` is the only initialization call and the effect never fires after that on fresh data, the chart might miss first-render data display. +2. **`$derived` chain integrity** — `option` is derived from `source` and `storyBranches`, each `$derived` from `summaries`. If any step in the chain is not truly reactive (breaks the `$derived` dependency chain), the chart won't redraw on prop change. +3. **`chart?.setOption` optional-chaining guard** — in the case where the `$effect` runs while `chart` is still undefined (pre-mount), the call is silently swallowed. Is there any scenario where a data update arrives after component destroy (chart disposed) that would cause a redundant setOption call on a disposed chart instance? +4. **Click handler and resize cleanup unchanged** — the `onMount` body was preserved verbatim; verify the click handler `chart.on('click', …)` and `return () => { chart.dispose(); window.removeEventListener(…) }` cleanup are intact and haven't been accidentally affected. + +#### Diff-derived attack surfaces + +1. **Output stability across schema-version drift** — does any persisted artifact, CLI stdout shape, or on-disk JSON format change in a way that breaks consumers reading prior-version output? +2. **CLI flag/arg symmetry** — for every added or changed CLI flag, is the parser, help text, validator, and downstream consumer updated together? +3. **Error-class breadth in `catch` blocks** — does each new or modified `catch` distinguish recoverable from non-recoverable errors? +4. **Defensive layering at module boundaries** — at every new cross-module call, does the callee revalidate inputs it cannot trust the caller to have normalized? +5. **Cross-file atomicity windows** — does any multi-step write leave an observable partially-committed window if interrupted? +6. **Test-contract strength** — do new tests assert the stable machine-readable contract before asserting prose, and do they cover both the omitted-hook and supplied-hook paths? +7. **Doc-vs-code drift in the ticket Rationale** — does the ticket's `## Rationale`, scope contract, or referenced docs describe behavior that does not match what the diff actually does? + +### Diff context + +The change converts `src/lib/components/BarChart/VerticalBarChart.svelte` from Svelte 4 legacy syntax to Svelte 5 runes: + +1. Added `` at top of file. +2. Removed `afterUpdate` from the Svelte import; kept `onMount`. +3. `export let summaries: SummariesResult` + `export let title = 'Branches vs Time'` → `let { summaries, title = 'Branches vs Time' }: { summaries: SummariesResult; title?: string } = $props()` +4. Four `$:` reactive statements → `const … = $derived(…)`: `available_branches`, `storyBranches`, `source`, `option` — all chained correctly (`storyBranches` from `available_branches`, `option` from `source` and `storyBranches`). +5. `afterUpdate(() => { chart.setOption(option) })` → `$effect(() => { chart?.setOption(option) })` with optional chaining guard. +6. `onMount` body is unchanged: initializes `chart` via `echarts.init`, adds resize listener, calls `chart.setOption(option)` once, sets up click handler, returns cleanup. +7. Template unchanged. + +--- + +### 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 — see +`docs/template/delivery/subagent-review-report-template.md` for the full +rules. 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.** A `---` + inside the `Advisory Observations` body breaks the all-bullets parser + check, causes fallback to paragraph mode, and preserves `- ` prefixes + on every observation key — creating verbatim-match churn in the + downstream dispositions file and forcing `---` itself to be triaged as + a fake observation. Just omit `---`. +- **`Runner termination` must be the section heading**, not `**runnerStatus:** + \`completed\`` or any other inline key-value variant. Write it as the bold + span `**Runner termination**` on its own line, then `runnerStatus:` and + `terminatedReason:` as plain-text lines below. Any other format leaves the + termination block inside the `Advisory Observations` body. +- 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 — that visually mimics a + section heading and splits one labeled observation into two parsed + observations. + +**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`: one of `completed | rate_limit | sandbox_denied | runner_unavailable`. +`terminatedReason`: one short sentence explaining why this status was reported. + +`completed` means you finished the review per this template. The other three values are +honest failure modes — the CLI refuses to record `outcome: clean` for any non-`completed` +`terminatedReason`, so do not claim `completed` if you stopped early. diff --git a/docs/product/delivery/phase-06/reviews/P6.01-subagent-review.report.md b/docs/product/delivery/phase-06/reviews/P6.01-subagent-review.report.md new file mode 100644 index 00000000..e115240a --- /dev/null +++ b/docs/product/delivery/phase-06/reviews/P6.01-subagent-review.report.md @@ -0,0 +1,48 @@ +**Invariant results** + +**I1 — chart redraws when summaries or title changes:** Held. When `summaries` changes, the full `$derived` chain recomputes (`available_branches` → `storyBranches` + `source` → `option`), and `$effect` fires calling `chart?.setOption(option)`. The `title` prop does NOT feed into `option` — it is rendered via the Svelte template (`{title}`) and does not trigger `setOption`. This is correct: `createVerticalBarChartOption` takes no title parameter, and the chart title is intentionally outside the ECharts canvas. The invariant's specific clause ("whenever the reactive option value changes") holds; `title`-only changes don't change `option` and are not expected to trigger `setOption`. + +**I2 — no "setOption before init" runtime error:** Held. In Svelte 5, `onMount` callbacks are flushed before the first effects batch. `chart` is assigned in `onMount` (line 38), guaranteeing it is defined when `$effect` first fires. The `chart?.setOption` optional-chaining guard covers the gap in principle; in practice the guarded case cannot occur under normal Svelte 5 lifecycle. + +**I3 — no legacy idioms remain:** Held. `grep` for `afterUpdate`, `export let`, and `$:` in `VerticalBarChart.svelte` returns zero matches. `` is present at line 1. `afterUpdate` is absent from the import list (only `onMount` imported from svelte). + +**Surface results** + +**S1 — `$effect` vs `onMount` ordering:** Probed. Svelte 5 processes `onMount` callbacks before flushing effects on initial mount. `chart` is initialized on line 38 before `$effect` ever runs. No "setOption before init" path exists. A secondary consequence: `$effect` fires once on initial mount (after `onMount`) calling `setOption` a second time with the same `option` object that `onMount` already applied. ECharts `setOption` is additive/merge-safe, so this double call is benign. See Advisory Observations. + +**S2 — `$derived` chain integrity:** Probed. `available_branches` reads `summaries.data` directly. `storyBranches` reads `available_branches`. `source` reads `summaries`. `option` reads `source` and `storyBranches`. All four are `const` `$derived` declarations — no intermediate mutation that could break the dependency graph. Changing `summaries` invalidates the full chain in one reactive pass and causes `$effect` to re-run. Chain is sound. + +**S3 — `chart?.setOption` optional-chaining guard — pre-mount and post-dispose paths:** Probed. Pre-mount: cannot occur in Svelte 5 (onMount runs first). Post-dispose: `chart.dispose()` is called in the `onMount` cleanup function. After disposal, the local variable `chart` still holds a truthy (non-null) reference to the disposed ECharts instance; the `?.` guard would NOT shortcircuit if `$effect` fired after dispose. However, Svelte 5 destroys effects synchronously during component teardown before the `onMount` cleanup runs, so the `$effect` cannot fire on a disposed instance in practice. Guard is correct for its stated purpose; the post-dispose path is unreachable. + +**S4 — Click handler and resize cleanup unchanged:** Probed. `chart.on('click', async (params) => {...})` block at lines 44–54 is intact. Cleanup return function at lines 56–59 calls `chart.dispose()` and `window.removeEventListener('resize', handleResize)` exactly as in the pre-conversion file. No regression. + +**SD1 — Output stability across schema-version drift:** N/A — this is a pure UI component conversion; no serialized artifacts, CLI stdout, or on-disk JSON is produced or consumed by this diff. + +**SD2 — CLI flag/arg symmetry:** N/A — no CLI flags or arguments are involved. + +**SD3 — Error-class breadth in catch blocks:** N/A — no `try/catch` blocks exist in the changed file. + +**SD4 — Defensive layering at module boundaries:** Probed. `createVerticalBarChartDatasetSource` guards `summaries.data` falsy at line 13 (returns `[]`). `createVerticalBarChartOption` accepts that empty array without error. `getStoryBranches` and `getStoryIdFromShortName` are called with derived values; no new trust boundary introduced. No defensive gap found. + +**SD5 — Cross-file atomicity windows:** N/A — no multi-step file writes in this change. + +**SD6 — Test-contract strength:** N/A per the ticket's explicit "Red: skip" decision documented in the Rationale. No new tests were added and none were required. Existing `BarChart` helper tests and smoke e2e remain the guard. + +**SD7 — Doc-vs-code drift in the ticket Rationale:** Probed. The ticket Rationale in `ticket-01-echarts-recipe-proof.md` describes: `afterUpdate(() => chart.setOption(option))` → `$effect(() => { chart?.setOption(option) })`, `onMount` body unchanged, `` added. All three match the diff exactly. The implementation-plan recipe at lines 41–42 matches the implementation. No drift detected. + +**Actionable findings** + +None. + +**Advisory Observations** + +- **Double `setOption` on initial mount.** `onMount` calls `chart.setOption(option)` unconditionally (line 41), then `$effect` fires on its first run — after `onMount` but still on the same initial render — and calls `chart?.setOption(option)` again with the same object. ECharts merges options on repeated calls and this is visually harmless, but it is a quiet redundancy. The recipe in the implementation plan documents the pattern as "setOption once in onMount + $effect for subsequent updates," which could suggest the first `$effect` run is expected to be a no-op; it is not. If T03/T04 apply this recipe verbatim to 12 sibling charts, all of them will double-call on mount. No production impact, but worth a line in the T03/T04 Rationale so future reviewers don't mistake it for a bug. + +- **`chart?.setOption` guard scope is narrower than it appears.** The optional chaining protects against `chart === undefined`, but after `chart.dispose()` the variable is still a truthy disposed instance — `?.` would not shortcircuit. The guard's actual safety window is only the pre-mount interval, which Svelte 5's `onMount`-before-effects ordering makes unreachable. The code is correct; the guard's meaning could mislead a future reader into believing it also protects the post-dispose case. + +- **Invariant wording in the review prompt includes "title change" but `title` does not feed into the ECharts option.** The prompt's invariant 1 opens with "redraws when `summaries` or `title` props change" — this is imprecise. `title` is rendered in the Svelte template via `` and is intentionally outside ECharts. A `title`-only change does NOT call `setOption`, and should not. The ticket outcome's wording ("redraws on data change") is accurate; the imprecision is in the review prompt, not in the code or ticket doc. + +**Runner termination** + +runnerStatus: completed +terminatedReason: All invariants probed and held; all attack surfaces (ticket-spec-derived and diff-derived) resolved; no actionable findings to report. diff --git a/src/lib/components/BarChart/VerticalBarChart.svelte b/src/lib/components/BarChart/VerticalBarChart.svelte index 3e7fe13d..f59b4c56 100644 --- a/src/lib/components/BarChart/VerticalBarChart.svelte +++ b/src/lib/components/BarChart/VerticalBarChart.svelte @@ -1,6 +1,8 @@ + +