diff --git a/.planning/phases/06-renderer-architecture/06-REVIEW.md b/.planning/phases/06-renderer-architecture/06-REVIEW.md new file mode 100644 index 0000000..dba0f48 --- /dev/null +++ b/.planning/phases/06-renderer-architecture/06-REVIEW.md @@ -0,0 +1,97 @@ +--- +phase: 06-renderer-architecture +reviewed: 2026-04-09T14:00:00Z +depth: standard +files_reviewed: 10 +files_reviewed_list: + - assets/src/heatmap.ts + - assets/src/renderers/registry.ts + - assets/src/renderers/types.ts + - assets/src/renderers/year.ts + - assets/src/shared/color-scale.ts + - assets/src/shared/date-utils.ts + - assets/src/shared/stats.ts + - assets/src/shared/tooltip.ts + - assets/src/state.ts + - assets/src/types.ts +findings: + critical: 0 + warning: 3 + info: 2 + total: 5 +status: issues_found +--- + +# Phase 06: Code Review Report + +**Reviewed:** 2026-04-09T14:00:00Z +**Depth:** standard +**Files Reviewed:** 10 +**Status:** issues_found + +## Summary + +The renderer architecture refactor is clean and well-structured. The extraction of shared utilities (tooltip, color-scale, stats, date-utils) into focused modules is solid, and the renderer registry pattern is simple and appropriate. No security vulnerabilities or critical bugs found. The issues below are defensive coding gaps and a minor cleanup item. + +## Warnings + +### WR-01: Unchecked JSON.parse can crash init + +**File:** `assets/src/heatmap.ts:21` +**Issue:** `JSON.parse(projectsJson)` will throw a `SyntaxError` if the `data-projects` attribute contains malformed JSON. This would crash the entire `init()` function, leaving the heatmap unrendered with no user-facing error. +**Fix:** +```typescript +let projects: ProjectOption[] = []; +if (projectsJson) { + try { + projects = JSON.parse(projectsJson); + } catch { + console.error('KimaiHeatmap: invalid data-projects JSON'); + } +} +``` + +### WR-02: Resize listener never removed -- leaks on repeated init() + +**File:** `assets/src/heatmap.ts:107-112` +**Issue:** The `resize` event listener is added but never removed. If `init()` is called multiple times on the same or different containers (e.g., SPA navigation, widget reload), each call adds another listener. The old listeners reference stale closures and DOM elements. +**Fix:** Return a cleanup function or store the listener reference for removal: +```typescript +const onResize = () => { + clearTimeout(resizeTimer); + resizeTimer = setTimeout(() => { + if (state.data) doRender(); + }, 200); +}; +window.addEventListener('resize', onResize); +// Store for cleanup: return { destroy: () => window.removeEventListener('resize', onResize) }; +``` + +### WR-03: innerHTML usage in stats renders data into DOM unsanitized + +**File:** `assets/src/shared/stats.ts:79` +**Issue:** `statsDiv.innerHTML = parts.join('')` injects content built from `DISPLAY_FORMAT(d)` output and numeric values. Currently safe because the data sources are controlled (d3 date formatter output and numbers), but the innerHTML pattern is fragile -- if a future change introduces user-controlled strings into `parts`, it becomes an XSS vector. The same pattern exists in `tooltip.ts:18` with `tip.innerHTML = html`. +**Fix:** Consider using `textContent` with structured DOM creation instead of innerHTML, or document the safety invariant with a comment so future maintainers know the constraint. + +## Info + +### IN-01: Unused import -- timeMonth in date-utils.ts + +**File:** `assets/src/shared/date-utils.ts:1` +**Issue:** `timeMonth` is imported from `d3-time` but never used in this module. It is used in `renderers/year.ts` which imports it directly. +**Fix:** Remove `timeMonth` from the import: +```typescript +import { timeMonday, timeSunday, timeDay } from 'd3-time'; +``` + +### IN-02: console.error calls as runtime logging + +**File:** `assets/src/heatmap.ts:14,95,125` +**Issue:** Three `console.error` calls serve as runtime error reporting. These are appropriate for a Kimai plugin context (no structured logging available), but worth noting as intentional rather than debug artifacts. +**Fix:** No action needed. If the project later adopts a logging abstraction, consolidate these. + +--- + +_Reviewed: 2026-04-09T14:00:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_