Skip to content

ARCHITECTURE_REVIEW.md

Latest commit

 

History

History
443 lines (350 loc) · 13.1 KB

ARCHITECTURE_REVIEW.md

File metadata and controls

443 lines (350 loc) · 13.1 KB
Raw

TimeLab Architecture Review & Improvement Plan

Executive Summary

TimeLab demonstrates strong foundational architecture with excellent TypeScript configuration, modern SCSS theming system, and clean domain separation. However, several critical areas require immediate attention to improve maintainability, type safety, and code clarity.

Priority Assessment

  • Critical Issues: 4 items requiring immediate attention
  • High Priority: 6 items for next development cycle
  • Medium Priority: 5 items for future improvements
  • Overall Architecture Grade: B+ (Strong foundation, needs refinement)

Architecture Analysis

Current Strengths

  • Modular Directory Structure: Clear separation between src/app/, src/charts/, src/data/, src/ui/, src/platform/, src/domain/
  • Import Aliases: Well-configured path aliases (@/, @app, @domain, etc.)
  • Domain-Driven Design: Pure domain types with no external dependencies
  • Platform Abstraction: Clean separation of browser-specific APIs

Critical Architectural Issues

1. Directory Structure Inconsistencies

Location: Various locations throughout src/

Problem: Mixed organizational patterns with some files in unexpected locations

src/components/dropdown.ts     # Should be in src/ui/
src/uploads/index.ts          # Unclear purpose/location
src/stats/panel.ts            # Single file directory

Recommendation: Consolidate related functionality

src/
├── app/           # Application bootstrap and initialization
├── charts/        # Time series chart components
├── data/          # Data management and processing
├── domain/        # Pure business logic and types
├── platform/      # Browser APIs and platform-specific code
├── shared/        # Utility functions and common types
├── ui/            # UI components and interactions
└── workers/       # Web workers (if any)

2. Circular Dependencies Risk

Problem: Some modules have potential circular import issues

Recommendation: Implement dependency analysis and strict layering rules:

  • Domain layer: No external dependencies
  • Data layer: Can depend on Domain
  • UI layer: Can depend on Domain and Data
  • App layer: Can depend on all others

SCSS Architecture Assessment

Excellent Patterns Found

  • 7-1 Architecture: Well-structured SCSS organization
  • Semantic Theming: Clean abstraction of CSS custom properties
  • Theme System: Robust theme switching via data attributes
  • Design Tokens: Proper separation of raw tokens and semantic variables

Areas for Improvement

  1. Component Path Structure: Deep nesting in components/components/ is confusing
  2. Import Redundancy: Some files import unused dependencies

Recommendation: Flatten component structure

src/styles/
├── abstracts/         # Variables, mixins, functions
├── base/             # Reset, typography, base elements  
├── components/       # Component styles (flattened)
├── layout/           # Grid, header, sidebar
├── themes/           # Theme variants
└── main.scss         # Main entry point

TypeScript Code Quality Issues

Critical Type Safety Issues

1. Extensive any Usage in ECharts Integration

File: src/charts/timeSeries.ts:622-624, 630, 636, 649

Current Problem:

(this.chart as any).getZr().on('mousedown', this.handleDrawingMouseDown.bind(this));
private handleDrawingMouseDown(event: any): void {
    const dataPoint = (this.chart as any).convertFromPixel({ gridIndex: 0 }, pixelPoint);
}

Solution: Create proper type definitions

interface EChartsWithZr extends ECharts {
    getZr(): {
        on(event: string, handler: (e: MouseEvent) => void): void;
    };
    convertFromPixel(finder: { gridIndex: number }, pixel: [number, number]): [number, number] | null;
}

private handleDrawingMouseDown(event: MouseEvent): void {
    const chart = this.chart as EChartsWithZr;
    const dataPoint = chart.convertFromPixel({ gridIndex: 0 }, pixelPoint);
}

2. Event System Type Safety

File: src/services/projectService.ts:20

Current Problem:

private eventListeners: Map<keyof ProjectServiceEvents, Array<(data: any) => void>> = new Map();

Solution: Proper generic constraints

private eventListeners: Map<
    keyof ProjectServiceEvents, 
    Array<(data: ProjectServiceEvents[keyof ProjectServiceEvents]) => void>
> = new Map();

3. Async Promise Handling in Event Listeners

Files: src/ui/projectModal.ts:197, 370

Problem: Unhandled promises in event handlers

button.addEventListener('click', async () => {
    await someAsyncOperation(); // @typescript-eslint/no-misused-promises
});

Solution: Explicit promise handling

button.addEventListener('click', (event) => {
    void handleAsyncClick(event);
});

async function handleAsyncClick(event: Event): Promise<void> {
    try {
        await someAsyncOperation();
    } catch (error) {
        // Handle error appropriately
    }
}

Memory Management Issues

Event Listener Cleanup

File: src/data/dataManager.ts

Problem: Missing event listener cleanup

constructor() {
    window.addEventListener('timelab:dataFilesChanged', this.handleDataFilesChanged.bind(this));
    // Missing cleanup - potential memory leak
}

Solution: Proper lifecycle management

private boundHandler = this.handleDataFilesChanged.bind(this);

constructor() {
    window.addEventListener('timelab:dataFilesChanged', this.boundHandler);
}

destroy(): void {
    window.removeEventListener('timelab:dataFilesChanged', this.boundHandler);
    this.callbacks.clear();
}

Data Management & API Design

Current State Analysis

  • Strengths: Clean Result pattern, typed error hierarchy, defensive copying
  • Weaknesses: Inconsistent error handling, singleton pattern issues, global state dependencies

Critical Improvements Needed

1. Standardize Error Handling Pattern

Problem: Mixed error handling approaches throughout codebase

Current Inconsistency:

// Some functions use Result pattern
const result = await dataManager.getDataSources();
if (isErr(result)) { ... }

// Others use try/catch
try {
    const sources = await dataManager.getDataSources();
} catch (error) { ... }

Recommendation: Standardize on Result pattern

// All async operations should return Result<T, E>
async getDataSources(): Promise<Result<DataSource[], DataError>> {
    try {
        const sources = await this.storage.getData();
        return ok(sources);
    } catch (error) {
        return err(new DataError('Failed to load data sources', error));
    }
}

2. Improve Testability

Problem: Singleton services are difficult to test

export const projectService = new ProjectService(); // Hard to test

Solution: Dependency injection pattern

export function createProjectService(): ProjectService {
    return new ProjectService();
}

let projectServiceInstance: ProjectService | null = null;

export function getProjectService(): ProjectService {
    if (!projectServiceInstance) {
        projectServiceInstance = createProjectService();
    }
    return projectServiceInstance;
}

export function setProjectServiceForTesting(service: ProjectService): void {
    projectServiceInstance = service;
}

Implementation Roadmap

Phase 1: Critical Fixes (Week 1-2)

Priority: Critical | Effort: High

  1. Remove any usage in ECharts integration

    • Create proper type definitions for ECharts API
    • Replace all any casts with typed interfaces
    • Files: src/charts/timeSeries.ts

  2. Fix async promise handling

    • Add proper error handling to event listeners
    • Implement explicit promise handling patterns
    • Files: src/ui/projectModal.ts, src/ui/labelModal.ts

  3. Implement memory leak prevention

    • Add cleanup methods to all services
    • Proper event listener management
    • Files: src/data/dataManager.ts, src/services/projectService.ts

  4. Standardize error handling

    • Convert all error handling to Result pattern
    • Remove inconsistent try/catch usage
    • Files: src/main.ts, all service files

Phase 2: High Priority Improvements (Week 3-4)

Priority: High | Effort: Medium

  1. Improve directory structure

    • Move misplaced files to appropriate directories
    • Consolidate single-file directories
    • Files: Various organizational moves

  2. Enhance type safety in event system

    • Remove any from event listener types
    • Implement proper generic constraints
    • Files: src/services/projectService.ts

  3. Add comprehensive integration tests

    • Test key workflows end-to-end
    • Mock external dependencies properly
    • Files: New test files

  4. Improve SCSS architecture

    • Flatten component directory structure
    • Remove unused imports
    • Files: src/styles/components/

Phase 3: Medium Priority Enhancements (Week 5-6)

Priority: Medium | Effort: Medium

  1. Implement dependency injection

    • Remove singleton pattern issues
    • Improve testability
    • Files: All service classes

  2. Add template literal types

    • Enhance theme system typing
    • Better configuration object types
    • Files: Theme and configuration files

  3. Create centralized data service

    • Consolidate data management logic
    • Improve API consistency
    • Files: New src/data/dataService.ts

  4. Enhance performance patterns

    • Add memoization where appropriate
    • Implement proper caching
    • Files: Chart and data processing files

Phase 4: Polish & Documentation (Week 7-8)

Priority: Low | Effort: Low

  1. Remove unnecessary null checks

    • Clean up ESLint warnings
    • Improve type flow analysis
    • Files: Various TypeScript files

  2. Add utility types

    • Create common type patterns
    • Implement branded types for IDs
    • Files: New utility type files

  3. Update documentation

    • Reflect architectural changes
    • Add developer guides
    • Files: README.md, CLAUDE.md

Best Practices Guide

TypeScript Standards

Type Safety

// ✅ Good: Proper typing
interface ChartConfig {
    xAxis: 'time' | 'category' | 'value';
    yAxis: 'value' | 'log';
}

// ❌ Bad: Any usage
const config: any = { xAxis: 'time', yAxis: 'value' };

Error Handling

// ✅ Good: Result pattern
async function loadData(): Promise<Result<Data, DataError>> {
    try {
        const data = await fetch('/api/data');
        return ok(data);
    } catch (error) {
        return err(new DataError('Failed to load', error));
    }
}

// ❌ Bad: Throwing exceptions
async function loadData(): Promise<Data> {
    const data = await fetch('/api/data'); // Unhandled errors
    return data;
}

Memory Management

// ✅ Good: Proper cleanup
class ChartComponent {
    private resizeObserver: ResizeObserver | null = null;
    
    constructor() {
        this.resizeObserver = new ResizeObserver(() => {});
    }
    
    destroy(): void {
        this.resizeObserver?.disconnect();
        this.resizeObserver = null;
    }
}

// ❌ Bad: No cleanup
class ChartComponent {
    constructor() {
        new ResizeObserver(() => {}); // Memory leak
    }
}

SCSS Standards

Theme Usage

// ✅ Good: Semantic variables
.component {
    background: $color-bg-primary;
    color: $color-text-primary;
    border: 1px solid $color-border-primary;
}

// ❌ Bad: Direct CSS custom properties
.component {
    background: var(--color-bg-primary);
    color: var(--color-text-primary);
}

Import Organization

// ✅ Good: Organized imports
@use '../abstracts/tokens' as *;
@use '../semantic' as *;
@use '../abstracts/theme-utils' as *;

// ❌ Bad: Disorganized imports
@import 'random-file';
@use 'another-file';

Architecture Principles

  1. Separation of Concerns: Keep business logic, UI, and data access separate
  2. Dependency Direction: Dependencies should flow inward toward domain
  3. Type Safety: Prefer compile-time safety over runtime checks
  4. Error Handling: Use Result pattern for predictable error handling
  5. Memory Management: Always clean up resources and event listeners
  6. Testability: Design for easy testing and mocking

Conclusion

TimeLab has a solid architectural foundation with modern TypeScript and SCSS patterns. The primary focus should be on eliminating type safety issues, standardizing error handling, and improving memory management. Following the phased implementation roadmap will systematically address these issues while maintaining development velocity.

The most critical items—removing any usage, fixing async promise handling, and implementing memory cleanup—should be addressed immediately to prevent technical debt accumulation. The remaining improvements can be implemented incrementally without disrupting ongoing development.

With these improvements, TimeLab will achieve enterprise-grade code quality while maintaining its current architectural strengths.