Calendar/architecture/code-review-2025-01.md

Critical Code Review - Calendar Plantempus

Date: January 2025
Reviewer: Code Analysis Assistant
Scope: Full TypeScript/JavaScript codebase

Executive Summary

This code review identifies 14+ critical issues that impact maintainability, performance, and the ability to add new features (especially month view). The codebase shows signs of rapid development without architectural planning, resulting in significant technical debt.

---

🚨 CRITICAL ISSUES

1. Inconsistent File Structure

Severity: High
Impact: Development confusion, build issues

Problems:

• Duplicate TypeScript/JavaScript files exist (src/utils/PositionUtils.js and .ts)
• Mixed compiled and source code in wwwroot/js/
• Legacy files in root directory (calendar-*.js)

Evidence:

src/utils/PositionUtils.js  (JavaScript)
src/utils/PositionUtils.ts  (TypeScript)
calendar-grid-manager.js    (Root legacy file)

Recommendation: Delete all .js files in src/, remove legacy root files, keep only TypeScript sources.

---

2. Event System Overcomplexity

Severity: Critical
Impact: Impossible to maintain, performance degradation

Problems:

• Two overlapping event systems (EventTypes.ts with 102 events + CalendarState.ts)
• Unclear separation of concerns
• Legacy events marked as "removed" but still present

Evidence:

// EventTypes.ts - 102 constants!
export const EventTypes = {
  CONFIG_UPDATE: 'calendar:configupdate',
  CALENDAR_TYPE_CHANGED: 'calendar:calendartypechanged',
  // ... 100 more events
}

// CalendarState.ts - Another event system
export const StateEvents = {
  CALENDAR_STATE_CHANGED: 'calendar:state:changed',
  // ... more events
}

Recommendation: Consolidate to ~20 core events with clear ownership.

---

3. Missing Error Handling

Severity: High
Impact: Silent failures, poor user experience

Problems:

• No try-catch blocks in critical paths
• No error boundaries for component failures
• DateCalculator assumes all inputs are valid

Evidence:

// DateCalculator.ts - No validation
getISOWeekStart(date: Date): Date {
  const monday = new Date(date); // What if date is invalid?
  const currentDay = monday.getDay();
  // ... continues without checks
}

Recommendation: Add comprehensive error handling and validation.

---

4. Memory Leak Potential

Severity: Critical
Impact: Browser performance degradation over time

Problems:

• Event listeners never cleaned up
• DOM references held indefinitely
• Multiple DateCalculator instances created

Evidence:

// ViewManager.ts - No cleanup
constructor(eventBus: IEventBus) {
  this.eventBus = eventBus;
  this.setupEventListeners(); // Never removed!
}
// No destroy() method exists

Recommendation: Implement proper cleanup in all managers.

---

5. Single Responsibility Violations

Severity: High
Impact: Unmaintainable code, difficult to test

Problems:

• GridManager: Handles rendering, events, styling, positioning (311 lines!)
• CalendarConfig: Config, state management, and factory logic mixed
• NavigationRenderer: DOM manipulation and event rendering

Evidence:

// GridManager doing everything:
- subscribeToEvents()
- render()
- setupGridInteractions()
- getClickPosition()
- scrollToHour()
- minutesToTime()

Recommendation: Split into focused, single-purpose classes.

---

6. Dependency Injection Missing

Severity: Medium
Impact: Untestable code, tight coupling

Problems:

• Hard-coded singleton imports everywhere
• calendarConfig imported directly in 15+ files
• Circular dependencies through EventBus

Evidence:

import { calendarConfig } from '../core/CalendarConfig'; // Singleton
import { eventBus } from '../core/EventBus'; // Another singleton

Recommendation: Use constructor injection pattern.

---

7. Performance Problems

Severity: Medium
Impact: Sluggish UI, especially with many events

Problems:

• document.querySelector called repeatedly
• No caching of DOM elements
• Full re-renders on every change

Evidence:

// Called multiple times per render:
const scrollableContent = document.querySelector('swp-scrollable-content');

Recommendation: Cache DOM queries, implement selective rendering.

---

8. Type Safety Issues

Severity: High
Impact: Runtime errors, hidden bugs

Problems:

• any types used extensively
• Type casting to bypass checks
• Missing interfaces for data structures

Evidence:

private allDayEvents: any[] = []; // No type safety
(header as any).dataset.today = 'true'; // Bypassing TypeScript

Recommendation: Define proper TypeScript interfaces for all data.

---

🔧 TECHNICAL DEBT

9. Redundant Code

• Duplicate date logic in DateCalculator and PositionUtils
• Headers rendered in multiple places
• Similar event handling patterns copy-pasted

10. Testability Issues

• No dependency injection makes mocking impossible
• Direct DOM manipulation prevents unit testing
• Global state makes tests brittle

11. Documentation Problems

• Mixed Danish/English comments
• Missing JSDoc for public APIs
• Outdated comments that don't match code

---

⚡ ARCHITECTURE ISSUES

12. Massive Interfaces

• CalendarTypes.ts: Too many interfaces in one file
• EventTypes: 102 constants is unmanageable
• Manager interfaces too broad

13. Coupling Problems

• High coupling between managers
• Everything communicates via events (performance hit)
• All components depend on global config

14. Naming Inconsistency

• Mixed language conventions
• Unclear event names (REFRESH_REQUESTED vs CALENDAR_REFRESH_REQUESTED)
• swp- prefix unexplained

---

📊 METRICS

Code Quality Scores

• Maintainability: 3/10
• Testability: 2/10
• Performance: 5/10
• Type Safety: 4/10
• Architecture: 3/10

File Statistics

• Total TypeScript files: 24
• Total JavaScript files: 8 (should be 0)
• Average file size: ~200 lines (acceptable)
• Largest file: GridManager.ts (311 lines)
• Event types defined: 102 (should be ~20)

---

🎯 RECOMMENDATIONS

Immediate Actions (Week 1)

1. Remove duplicate files - Clean up .js duplicates
2. Add error boundaries - Prevent cascade failures
3. Fix memory leaks - Add cleanup methods

Short Term (Month 1)

4. Consolidate events - Reduce to core 20 events
5. Implement DI - Remove singleton dependencies
6. Split mega-classes - Apply Single Responsibility

Long Term (Quarter 1)

7. Add comprehensive tests - Aim for 80% coverage
8. Performance optimization - Virtual scrolling, caching
9. Complete documentation - JSDoc all public APIs

---

Impact on Month View Implementation

Without refactoring:

• 🔴 ~2000 lines of new code
• 🔴 3-4 weeks implementation
• 🔴 High bug risk

With minimal refactoring:

• ✅ ~500 lines of new code
• ✅ 1 week implementation
• ✅ Reusable components

---

Conclusion

The codebase requires significant refactoring to support new features efficiently. The identified issues, particularly the lack of strategy pattern and hardcoded week/day assumptions, make adding month view unnecessarily complex.

Priority: Focus on minimal refactoring that enables month view (Strategy pattern, config split, event consolidation) before attempting to add new features.