diff --git a/REFACTORING_PLAN.md b/REFACTORING_PLAN.md deleted file mode 100644 index d06acd5..0000000 --- a/REFACTORING_PLAN.md +++ /dev/null @@ -1,474 +0,0 @@ -# Type Duplication Refactoring Plan - -**Document Purpose**: Consolidate duplicate type definitions across templates/ and handlers/ packages to align with templ's design philosophy and reduce maintenance burden. - -**Status**: Planning Phase -**Last Updated**: 2026-02-12 -**Separation from**: IMPLEMENTATION_EXACT.md (this is independent refactoring) - -**Core Principle**: **API wins** - When there's ambiguity between template types and API/handler types, the API/handler types take precedence. - ---- - -## Executive Summary - -**Problem**: Codebase maintains parallel type systems: -- `templates/types.go` - Template-specific types -- `handlers/*.go` - API response types - -**Core Principle**: **API wins - when in doubt, API/handler types take precedence** - -### Why "API wins"? - -1. **API is public contract**: Handler types define your public API surface - these are more stable -2. **Templates are presentation**: HTML templates can format any Go type - they're flexible -3. **Single source of truth**: One authoritative type definition vs two parallel definitions -4. **Json serialization**: Handler types already have `json:"..."` tags for API responses -5. **Future API clients**: If you add mobile apps or CLI tools, they need handler types anyway - -### Trade-offs - -**Pro**: Simpler codebase, one type to maintain, automatic API/HTML consistency -**Con**: Templates may have unused fields visible in scope (acceptable - templ handles this well) - -**Impact**: -- ~40 lines of duplicate type definitions -- Unnecessary conversion functions -- Maintenance burden (update 2 places for 1 change) -- Violates templ's design philosophy - -**Solution**: Use handler types directly in templates, eliminate template-specific duplicates. - ---- - -## Current State Analysis - -### Duplicates Found - -| Template Type | Handler Type | Overlap | Action | -|---------------|---------------|----------|--------| -| `DeviceData` | `DeviceInfo` | 90% | **DELETE** DeviceData | -| `BookData` | `collections.BookInfo` | 100% | **DELETE** BookData | -| `CollectionDetailData` | `CollectionData` | 100% | **DELETE** CollectionDetailData | -| `ProgressItemData` | `ProgressWithMedia` | 70% | **MERGE** (different fields) | -| `QueueItemData` | `QueueItemResponse` | 60% | **KEEP** (template uses subset) | - -### Already Using Handler Types (Good Patterns) - -✅ `Conflicts` template uses `handlers.ConflictDetailResponse` -✅ `Queue` template uses `handlers.QueueItemResponse` - -**This is the pattern we want everywhere.** - ---- - -## Refactoring Plan - -### Phase 1: Eliminate Complete Duplicates - -#### 1.1 Delete CollectionDetailData - -**Files to Modify**: -- `templates/types.go` (lines 26-32) - DELETE struct -- `templates/collection_rules.templ` (line 3) - Change signature -- `templates/collections.templ` (line 213) - Change signature - -**Changes**: - -**Delete** from `templates/types.go`: -```go -type CollectionDetailData struct { - ID string - Name string - Description string - Color string - Icon string -} -``` - -**Update** `templates/collection_rules.templ:3`: -```templ -// BEFORE: -templ CollectionRules(user User, collection CollectionDetailData) { - -// AFTER: -templ CollectionRules(user User, collection templates.CollectionData) { -``` - -**Update** `templates/collections.templ:213`: -```templ -// BEFORE: -templ CollectionDetail(user User, collection CollectionDetailData, books []BookData) { - -// AFTER: -templ CollectionDetail(user User, collection templates.CollectionData, books []collections.BookInfo) { -``` - -**Test**: `go test ./templates/...` after template regeneration - ---- - -#### 1.2 Delete BookData - -**Files to Modify**: -- `templates/types.go` (lines 34-39) - DELETE struct -- `templates/collections.templ` (line 213) - Change books param - -**Changes**: - -**Delete** from `templates/types.go`: -```go -type BookData struct { - MediaItemID string - Title string - Author string - CoverImagePath string -} -``` - -**Update** `templates/collections.templ:213`: -```templ -// BEFORE: -templ CollectionDetail(user User, collection templates.CollectionData, books []BookData) { - -// AFTER: -templ CollectionDetail(user User, collection templates.CollectionData, books []collections.BookInfo) { -``` - -**Test**: View collection detail page, verify books display correctly - ---- - -#### 1.3 Delete DeviceData and ConvertDevices - -**Files to Modify**: -- `templates/types.go` (lines 48-57) - DELETE struct -- `templates/devices.templ` (line 3) - Change signature -- `internal/router/helpers.go` (lines 46-67) - DELETE function -- `internal/router/frontend.go` (lines 161-169) - Remove conversion - -**Changes**: - -**Delete** from `templates/types.go`: -```go -type DeviceData struct { - ID string - DeviceName string - DeviceType string - SyncEnabled bool - AutoSync bool - SyncFrequency int32 - LastSync string - LastSeen string -} -``` - -**Delete** from `internal/router/helpers.go`: -```go -func convertDevices(deviceInfos []handlers.DeviceInfo) []templates.DeviceData { - // ... entire function (lines 46-67) -} -``` - -**Update** `templates/devices.templ:3`: -```templ -// BEFORE: -templ Devices(user User, devices []DeviceData, pendingRegistrations []PendingRegistrationData) { - -// AFTER: -templ Devices(user User, devices []handlers.DeviceInfo, pendingRegistrations []PendingRegistrationData) { -``` - -**Update** `templates/devices.templ` template logic: - -Find these patterns and update: -```templ -// BEFORE: -if device.LastSync != "" { - { device.LastSync } -} - -// AFTER: -if device.LastSync != nil { - { device.LastSync.Format("2006-01-02 15:04") } -} -``` - -**Update** `internal/router/frontend.go:161-169`: -```go -// BEFORE: -deviceData, err := cfg.DeviceHandler.GetDevicesData(c) -if err != nil { - return c.HTML(http.StatusInternalServerError, "Error loading devices") -} -pendingData, err := cfg.DeviceHandler.GetPendingRegistrationsData(c) -if err != nil { - return c.HTML(http.StatusInternalServerError, "Error loading pending") -} -deviesList := convertDevices(deviceData) -pendingList := convertPending(pendingData) - -// AFTER: -deviceData, err := cfg.DeviceHandler.GetDevicesData(c) -if err != nil { - return c.HTML(http.StatusInternalServerError, "Error loading devices") -} -pendingData, err := cfg.DeviceHandler.GetPendingRegistrationsData(c) -if err != nil { - return c.HTML(http.StatusInternalServerError, "Error loading pending") -} -// No conversion - use handler data directly -``` - -**Test**: Devices page renders correctly with proper date formatting - ---- - -### Phase 2: Eliminate Remaining Template Types - -#### 2.1 ProgressItemData → Use ProgressWithMedia - -**Current Issue**: -- `handlers.ProgressWithMedia` exists as API response type -- `templates.ProgressItemData` duplicates it with minor variations - -**Decision**: **API wins** - Use `handlers.ProgressWithMedia` everywhere - -**Files to Modify**: -- `internal/handlers/progress.go` - Update `ProgressWithMedia` struct -- `templates/types.go` - DELETE `ProgressItemData` -- `templates/progress.templ` - Use `handlers.ProgressWithMedia` - -**Changes**: - -**Add fields to** `internal/handlers/progress.go:ProgressWithMedia`: -```go -// ADD fields to existing struct: -type ProgressWithMedia struct { - MediaItemID uuid.UUID `json:"media_item_id"` - Title string `json:"title"` - Author string `json:"author"` - CoverImagePath string `json:"cover_image_path"` - Percentage float64 `json:"percentage"` - CurrentPage int32 `json:"current_page"` - TotalPages int32 `json:"total_pages"` - LastReadAt time.Time `json:"last_read_at"` - Epubcfi string `json:"epubcfi"` - LastSyncDevice string `json:"last_sync_device"` - DeviceName string `json:"device_name,omitempty"` // ADD - DeviceType string `json:"device_type,omitempty"` // ADD - DeviceIcon string `json:"device_icon,omitempty"` // ADD -} -``` - -**Update** `internal/handlers/progress.go:GetAllProgress()` to populate new device fields. - -**Delete** from `templates/types.go` (lines 99-112): -```go -type ProgressItemData struct { - // ... delete entire struct -} -``` - -**Update** `templates/progress.templ:3`: -```templ -// BEFORE: -templ Progress(user User, progressData []ProgressItemData) { - -// AFTER: -templ Progress(user User, progressData []handlers.ProgressWithMedia) { -``` - -**Test**: Progress page displays device names and icons - -**Delete** from `templates/types.go` (lines 99-112): -```go -type ProgressItemData struct { - // ... delete entire struct -} -``` - -**Update** `templates/progress.templ:3`: -```templ -// BEFORE: -templ Progress(user User, progressData []ProgressItemData) { - -// AFTER: -templ Progress(user User, progressData []handlers.ProgressWithMedia) { -``` - -**Test**: Progress page displays device names and icons - ---- - -### Phase 3: Update Get*Data Functions - -**Issue**: Several handler functions return handler types but aren't used directly by templates due to conversion layer. - -**Files to Modify**: -- `internal/router/frontend.go` - Remove conversions - -**Changes**: - -**Devices** (already done in Phase 1.3): -- `GetDevicesData()` returns `[]handlers.DeviceInfo` -- Template uses directly - -**Progress**: -```go -// frontend.go - update progress route -protected.GET("/progress-page", func(c echo.Context) error { - // ... - progressData, err := cfg.Handler.GetAllProgressData(c) - // Returns []handlers.ProgressWithMedia directly - no conversion - // ... -}) -``` - -**Collections**: -```go -// collections.go - update to return handler types -func (h *CollectionHandler) GetCollectionBooksData(c echo.Context) ([]collections.BookInfo, error) { - // Already returns BookInfo - just update template -} -``` - ---- - -## Summary Table - -| Phase | Files Changed | Lines Removed | Lines Added | Net Change | -|-------|--------------|---------------|--------------|-------------| -| 1.1 CollectionDetailData | 3 | 7 | 2 | -5 | -| 1.2 BookData | 3 | 6 | 0 | -6 | -| 1.3 DeviceData | 4 | 24 | 5 | -19 | -| 2.1 ProgressItemData | 4 | 14 | 3 | -11 | -| **Total** | **14** | **51** | **10** | **-41** | - -**Note**: QueueItemData already uses handler type (good pattern). Conflicts already uses handler type (good pattern). - ---- - -## Testing Checklist - -After each phase: - -### Phase 1.1 (CollectionDetailData) -- [ ] View collection rules page -- [ ] Edit collection rules -- [ ] View collection detail page -- [ ] Add books to collection - -### Phase 1.2 (BookData) -- [ ] View collection detail page -- [ ] Browse books in collection -- [ ] Book covers display correctly - -### Phase 1.3 (DeviceData) -- [ ] Devices page loads -- [ ] Device cards display -- [ ] Last sync/last seen dates formatted -- [ ] Device type icons show correctly - -### Phase 2.1 (ProgressItemData) -- [ ] Progress page loads -- [ ] Progress entries show device names/icons -- [ ] Percentage display works -- [ ] Cover images display - ---- - -## Rollback Plan - -If refactoring breaks functionality: - -1. **Git stash** changes before starting -2. **Commit after each successful phase** -3. **Revert individual commits** if needed -4. **Keep conversion functions** commented out temporarily - -```bash -# Before starting -git stash push -m "pre-refactor-state" - -# After Phase 1.1 -git commit -m "refactor: delete CollectionDetailData, use CollectionData" - -# If Phase 1.2 breaks -git revert HEAD -# Continue with 1.3 -``` - ---- - -## Benefits - -### Code Quality -- ✅ Aligns with templ's design philosophy -- ✅ Single source of truth for types -- ✅ Reduced code duplication -- ✅ **API-first**: Handler types are authoritative - -### Maintenance -- ✅ Update types in 1 place, not 2 -- ✅ Less conversion logic to maintain -- ✅ Clearer data flow -- ✅ **Template changes automatically get API updates** - -### Consistency -- ✅ All templates use handlers types (like Queue/Conflicts already do) -- ✅ API and HTML use same type definitions -- ✅ 41 fewer lines of code -- ✅ **"API wins"** principle eliminates ambiguity - ---- - -## Future Considerations - -### For IMPLEMENTATION_EXACT.md -After this refactoring, adding `AuthToken` to devices is simpler: -- Add `AuthToken string` to `handlers.DeviceInfo` struct -- Add to `GetDevicesData()` function return values -- Template automatically gets it (no conversion layer) - -This **solves the critical issue** found in IMPLEMENTATION_EXACT.md analysis automatically. - -### For New Features -- Define handler types with json tags first -- Templates use same types directly -- Only create template-specific types for: - - Computed/derived fields not in API - - Combining data from multiple sources - - Simplifying complex nested structures - ---- - -## Execution Order - -1. **Run tests** to ensure baseline: `go test ./...` -2. **Create feature branch**: `git checkout -b refactor/types-consolidation` -3. **Execute Phase 1.1** (CollectionDetailData) - DELETE template type, use API type -4. **Commit and test** -5. **Execute Phase 1.2** (BookData) - DELETE template type, use API type -6. **Commit and test** -7. **Execute Phase 1.3** (DeviceData) - DELETE template type, use API type -8. **Commit and test** -9. **Execute Phase 2.1** (ProgressItemData) - DELETE template type, enhance API type -10. **Final commit and test** -11. **Update IMPLEMENTATION_EXACT.md** to reference handler types only - -**Total Estimated Time**: 2-3 hours with testing - -**Key Mindset**: Every change removes a template type, never the reverse. API types are authoritative. - ---- - -## Questions for User - -1. **Progress template updates**: Since we're using `handlers.ProgressWithMedia` (API type) in templates, any template formatting changes (like date formatting) should be done in the template itself. Is this acceptable? - -2. **Testing**: Do you have automated template tests, or is this manual testing only? - -3. **Implementation plan**: Should I update IMPLEMENTATION_EXACT.md after this refactoring to reference `handlers.DeviceInfo` instead of `DeviceData`? - -4. **Json tags on handler types**: Templates don't need `json:"..."` tags - should we document that these are for API responses only?