bookhoard/REFACTORING_PLAN.md

# 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 != "" {
    <span>{ device.LastSync }</span>
}

// AFTER:
if device.LastSync != nil {
    <span>{ device.LastSync.Format("2006-01-02 15:04") }</span>
}
```

**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?