---
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_