Bookhoard/bookhoard
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 12 Wiki Activity

chore: remove obsolete REFACTORING_PLAN.md

Browse Source
This commit is contained in:
john-okeefe
2026-02-16 16:51:26 -05:00
parent 81c72e706a
commit e25dc106fa
1 changed files with 0 additions and 474 deletions
Download Patch File Download Diff File Expand all files Collapse all files
REFACTORING_PLAN.md
-474
View File
@@ -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 != "" {
<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?