kimai-plugin-heatmap/.planning/phases/07-mode-switcher-week-mode/07-REVIEW.md

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
07-mode-switcher-week-mode	2026-04-09T12:00:00Z	standard	9	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	critical warning info total 0 3 2 5	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:

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:

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:

// 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:

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