diff --git a/.planning/phases/07-mode-switcher-week-mode/07-REVIEW.md b/.planning/phases/07-mode-switcher-week-mode/07-REVIEW.md new file mode 100644 index 0000000..2cf9eab --- /dev/null +++ b/.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_