docs(06): add code review report
This commit is contained in:
parent
abe3ec6f72
commit
76a7571275
1 changed files with 97 additions and 0 deletions
97
.planning/phases/06-renderer-architecture/06-REVIEW.md
Normal file
97
.planning/phases/06-renderer-architecture/06-REVIEW.md
Normal file
|
|
@ -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_
|
||||||
Loading…
Add table
Reference in a new issue