docs(07): add code review report

.planning/phases/07-mode-switcher-week-mode/07-REVIEW.md

@@ -0,0 +1,106 @@
+---
+phase: 07-mode-switcher-week-mode
+reviewed: 2026-04-09T12:00:00Z
+depth: standard
+files_reviewed: 9
+files_reviewed_list:
+  - Resources/public/heatmap.css
+  - Resources/public/heatmap.js
+  - Resources/views/widget/heatmap.html.twig
+  - assets/src/heatmap.ts
+  - assets/src/renderers/week.ts
+  - assets/src/renderers/year.ts
+  - assets/src/ui/controls.ts
+  - assets/test/controls.test.ts
+  - assets/test/week.test.ts
+findings:
+  critical: 0
+  warning: 3
+  info: 2
+  total: 5
+status: issues_found
+---
+
+# Phase 7: Code Review Report
+
+**Reviewed:** 2026-04-09T12:00:00Z
+**Depth:** standard
+**Files Reviewed:** 9
+**Status:** issues_found
+
+## Summary
+
+Phase 7 adds a mode switcher (year/week toggle) and metric toggle (hours/count) to the heatmap widget, plus a new week-mode renderer that aggregates time data by weekday. The code is well-structured: clean renderer registry pattern, good separation of concerns between UI controls and renderers, solid test coverage for the new components.
+
+Three warnings found -- two are potential bugs related to tooltip XSS and missing null safety, one is a subtle interaction issue with the renderer lifecycle. Two minor info items for code quality.
+
+## Warnings
+
+### WR-01: Tooltip innerHTML allows XSS via project/weekday names
+
+**File:** `assets/src/shared/tooltip.ts:18`, `assets/src/renderers/week.ts:134`, `assets/src/renderers/year.ts:114`
+**Issue:** `showTooltip` sets `tip.innerHTML = html`, and the callers build HTML strings by interpolating data values. In week.ts, `agg.label` comes from hardcoded weekday names (safe), but in year.ts line 114, `DISPLAY_FORMAT(d.date)` is also safe since it formats a Date object. However, `showTooltip` itself is a shared utility -- any future caller interpolating user-controlled data (e.g., project names from the server) would be vulnerable to stored XSS. The pattern is fragile.
+**Fix:** Consider using `textContent` for data values and building DOM nodes instead of HTML strings, or at minimum add an escaping utility:
+```typescript
+function escapeHtml(s: string): string {
+  const div = document.createElement('div');
+  div.textContent = s;
+  return div.innerHTML;
+}
+```
+
+### WR-02: Renderer destroy() is optional but called unconditionally
+
+**File:** `assets/src/heatmap.ts:69`
+**Issue:** Line 69 calls `renderer.destroy?.()` on the *new* renderer (just fetched via `getRenderer(state.mode)`), not the *previous* renderer. When switching from year to week mode, the year renderer's tooltip is never cleaned up -- `destroy()` is called on the week renderer instance (which hasn't rendered yet, so it's a no-op). The previous mode's tooltip element leaks in the DOM.
+**Fix:** Track the active renderer and destroy it before switching:
+```typescript
+let activeRenderer: ModeRenderer | null = null;
+
+const doRender = () => {
+  if (!state.data) return;
+  activeRenderer?.destroy?.();
+  activeRenderer = getRenderer(state.mode);
+  activeRenderer.render({ /* ... */ });
+  // ...
+};
+```
+
+### WR-03: Color scale domain starts at 0, producing color for zero-value entries
+
+**File:** `assets/src/shared/color-scale.ts:15`
+**Issue:** The quantize scale domain is `[0, maxVal]`, which means a value of exactly 0 maps to the lightest green (`#9be9a8`). In the week renderer this is mitigated by the `hasData` guard (line 115-118 in week.ts), and in year.ts by the `if (!d.entry)` check (line 107). But if a DayEntry exists with `hours: 0` and `count: 0`, it would render with a green fill rather than appearing empty. This is an edge case but a data-truthfulness issue.
+**Fix:** Either filter zero-value entries in the color-scale builder, or add a guard in the renderers:
+```typescript
+// In year.ts, line 107:
+if (!d.entry || (ctx.state.metric === 'hours' ? d.entry.hours : d.entry.count) === 0) return '';
+```
+
+## Info
+
+### IN-01: Duplicate stats-removal logic in doRender
+
+**File:** `assets/src/heatmap.ts:78-83`
+**Issue:** Lines 78-83 have two consecutive `if (state.mode === 'year')` blocks that could be combined into a single if/else:
+```typescript
+if (state.mode === 'year') {
+  renderStats(container, state.data.days);
+  svgArea.scrollLeft = svgArea.scrollWidth;
+} else {
+  const existingStats = container.querySelector('.heatmap-stats');
+  if (existingStats) existingStats.remove();
+}
+```
+**Fix:** Merge the two conditionals as shown above.
+
+### IN-02: `createMetricControl` is a thin wrapper that adds no value
+
+**File:** `assets/src/ui/controls.ts:33-42`
+**Issue:** `createMetricControl` simply calls `createModeControl` with hardcoded hours/count options. This is fine as a convenience function, but the naming is slightly misleading -- it suggests a different control type when it's really just pre-configured mode buttons. No action needed, just noting for awareness.
+**Fix:** None required. Could add a brief doc comment clarifying it's a convenience wrapper.
+
+---
+
+_Reviewed: 2026-04-09T12:00:00Z_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_