From 6de413376f527dfbfe76fad166b7b9e2b24ecdc6 Mon Sep 17 00:00:00 2001
From: Ilyas Hallak <ilhallak@gmail.com>
Date: Fri, 21 Nov 2025 22:07:41 +0100
Subject: [PATCH] Clean up documentation and remove debug settings

- Move documentation files from root to docs/ folder
- Remove DEBUG-only settings from ReadingSettingsView (Safari Reader Mode, Auto-mark as read)
---
 {documentation => docs}/401.md                |   0
 {documentation => docs}/Architecture.md       |   0
 docs/Tag-Sync-Review.md                       | 283 ++++++++++++++++++
 docs/Tags-Sync.md                             | 203 +++++++++++++
 {documentation => docs}/tabbar.md             |   0
 readeck/UI/Settings/ReadingSettingsView.swift |   5 +-
 6 files changed, 487 insertions(+), 4 deletions(-)
 rename {documentation => docs}/401.md (100%)
 rename {documentation => docs}/Architecture.md (100%)
 create mode 100644 docs/Tag-Sync-Review.md
 create mode 100644 docs/Tags-Sync.md
 rename {documentation => docs}/tabbar.md (100%)

diff --git a/documentation/401.md b/docs/401.md
similarity index 100%
rename from documentation/401.md
rename to docs/401.md
diff --git a/documentation/Architecture.md b/docs/Architecture.md
similarity index 100%
rename from documentation/Architecture.md
rename to docs/Architecture.md
diff --git a/docs/Tag-Sync-Review.md b/docs/Tag-Sync-Review.md
new file mode 100644
index 0000000..0e86347
--- /dev/null
+++ b/docs/Tag-Sync-Review.md
@@ -0,0 +1,283 @@
+# Code Review - Tag Management Refactoring
+
+**Commit**: ec5706c - Refactor tag management to use Core Data with configurable sorting
+**Date**: 2025-11-08
+**Files Changed**: 31 files (+747, -264)
+
+## Overview
+
+This review covers a comprehensive refactoring of the tag management system, migrating from API-based tag loading to a Core Data-first approach with background synchronization.
+
+---
+
+## ✅ Strengths
+
+### Architecture & Design
+
+1. **Clean Architecture Compliance**
+   - New `SyncTagsUseCase` properly separates concerns
+   - ViewModels now only interact with UseCases, not Repositories
+   - Proper dependency injection through UseCaseFactory
+
+2. **Performance Improvements**
+   - Cache-first strategy provides instant UI response
+   - Background sync eliminates UI blocking
+   - Reduced server load through local caching
+   - SwiftUI `@FetchRequest` provides automatic reactive updates
+
+3. **Offline Support**
+   - Tags work completely offline using Core Data
+   - Share Extension uses cached tags (no network required)
+   - Graceful degradation when server is unreachable
+
+4. **User Experience**
+   - Configurable sorting (by count/alphabetically)
+   - Clear sorting indicators in UI
+   - Proper localization (EN/DE)
+   - "Most used tags" in Share Extension for quick access
+
+### Code Quality
+
+1. **Consistency**
+   - Consistent use of `@MainActor` for UI updates
+   - Proper async/await patterns throughout
+   - Clear naming conventions
+
+2. **Documentation**
+   - Comprehensive commit message
+   - Inline documentation for complex logic
+   - `Tags-Sync.md` documentation created
+
+3. **Testing Support**
+   - Mock implementations added for all new UseCases
+   - Testable architecture with clear boundaries
+
+---
+
+## ⚠️ Issues & Concerns
+
+### Critical
+
+None identified.
+
+### Major
+
+1. **LabelsRepository Duplication** (Priority: HIGH)
+   - `LabelsRepository` is instantiated multiple times in different factories
+   - Not using lazy singleton pattern
+   - Could lead to multiple concurrent API calls
+
+   **Location**:
+   - `DefaultUseCaseFactory.makeGetLabelsUseCase()` - line 101
+   - `DefaultUseCaseFactory.makeSyncTagsUseCase()` - line 107
+
+   **Impact**: Inefficient, potential race conditions
+
+2. **Missing Error Handling** (Priority: MEDIUM)
+   - `syncTags()` silently swallows all errors with `try?`
+   - No user feedback if sync fails
+   - No retry mechanism
+
+   **Locations**:
+   - `AddBookmarkViewModel.syncTags()` - line 69
+   - `BookmarkLabelsViewModel.syncTags()` - line 45
+
+3. **Legacy Code Not Fully Removed** (Priority: LOW)
+   - `AddBookmarkViewModel.loadAllLabels()` still exists but unused
+   - `BookmarkLabelsViewModel.allLabels` property unused
+   - `LegacyTagManagementView` marked deprecated but not removed
+
+   **Impact**: Code bloat, confusion for future developers
+
+### Minor
+
+1. **Hardcoded Values**
+   - Share Extension: `fetchLimit: 150` hardcoded in view
+   - Should be a constant
+
+   **Location**: `ShareBookmarkView.swift:143`
+
+2. **Inconsistent Localization Approach**
+   - Share Extension uses `"Most used tags"` directly in code
+   - Should use `.localized` extension like main app
+
+   **Location**: `ShareBookmarkView.swift:145`
+
+3. **Missing Documentation**
+   - `CoreDataTagManagementView` has no class-level documentation
+   - Complex `@FetchRequest` initialization not explained
+
+   **Location**: `CoreDataTagManagementView.swift:4`
+
+4. **Code Duplication**
+   - Tag sync logic duplicated in `GetLabelsUseCase` and `SyncTagsUseCase`
+   - Both just call `labelsRepository.getLabels()`
+
+   **Locations**:
+   - `GetLabelsUseCase.execute()` - line 14
+   - `SyncTagsUseCase.execute()` - line 19
+
+---
+
+## 🔍 Specific File Reviews
+
+### ShareBookmarkViewModel.swift
+**Status**: ✅ Good
+**Changes**: Removed 92 lines of label fetching logic
+
+- ✅ Properly simplified by removing API logic
+- ✅ Uses Core Data via `addCustomTag()` helper
+- ✅ Clean separation of concerns
+- ⚠️ Could add logging for Core Data fetch failures
+
+### CoreDataTagManagementView.swift
+**Status**: ✅ Good
+**Changes**: New file, 255 lines
+
+- ✅ Well-structured with clear sections
+- ✅ Proper use of `@FetchRequest`
+- ✅ Flexible with optional parameters
+- ⚠️ Needs class/struct documentation
+- ⚠️ `availableTagsTitle` parameter could be better named (`customSectionTitle`?)
+
+### SyncTagsUseCase.swift
+**Status**: ⚠️ Needs Improvement
+**Changes**: New file, 21 lines
+
+- ✅ Follows UseCase pattern correctly
+- ✅ Good documentation comment
+- ⚠️ Essentially duplicates `GetLabelsUseCase`
+- 💡 Could be merged or one could wrap the other
+
+### LabelsRepository.swift
+**Status**: ✅ Excellent
+**Changes**: Enhanced with batch updates and conflict detection
+
+- ✅ Excellent cache-first + background sync implementation
+- ✅ Proper batch operations
+- ✅ Silent failure handling
+- ✅ Efficient Core Data updates (only saves if changed)
+
+### AddBookmarkView.swift
+**Status**: ✅ Good
+**Changes**: Migrated to CoreDataTagManagementView
+
+- ✅ Clean migration from old TagManagementView
+- ✅ Proper use of AppSettings for sort order
+- ✅ Clear UI with sort indicator
+- ⚠️ `.onAppear` and `.task` mixing removed - good!
+
+### Settings Integration
+**Status**: ✅ Excellent
+**Changes**: New TagSortOrder setting with persistence
+
+- ✅ Clean domain model separation
+- ✅ Proper persistence in SettingsRepository
+- ✅ Good integration with AppSettings
+- ✅ UI properly reflects settings changes
+
+---
+
+## 📋 TODO List - Improvements
+
+### High Priority
+
+- [ ] **Refactor LabelsRepository instantiation**
+  - Create lazy singleton in DefaultUseCaseFactory
+  - Reuse same instance for GetLabelsUseCase and SyncTagsUseCase
+  - Add comment explaining why singleton is safe here
+
+- [ ] **Add error handling to sync operations**
+  - Log errors instead of silently swallowing
+  - Consider adding retry logic with exponential backoff
+  - Optional: Show subtle indicator when sync fails
+
+- [ ] **Remove unused legacy code**
+  - Delete `AddBookmarkViewModel.loadAllLabels()`
+  - Delete `BookmarkLabelsViewModel.allLabels` property
+  - Remove `LegacyTagManagementView.swift` entirely (currently just deprecated)
+
+### Medium Priority
+
+- [ ] **Extract constants**
+  - Create `Constants.Tags.maxShareExtensionTags = 150`
+  - Create `Constants.Tags.fetchBatchSize = 20`
+  - Reference in CoreDataTagManagementView and ShareBookmarkView
+
+- [ ] **Improve localization consistency**
+  - Use `.localized` extension in ShareBookmarkView
+  - Ensure all user-facing strings are localized
+
+- [ ] **Add documentation**
+  - Document `CoreDataTagManagementView` with usage examples
+  - Explain `@FetchRequest` initialization pattern
+  - Add example of how to use `availableTagsTitle` parameter
+
+### Low Priority
+
+- [ ] **Consolidate UseCases**
+  - Consider if `SyncTagsUseCase` is necessary
+  - Option 1: Make `GetLabelsUseCase` have a `syncOnly` parameter
+  - Option 2: Have `SyncTagsUseCase` wrap `GetLabelsUseCase`
+  - Document decision either way
+
+- [ ] **Add unit tests**
+  - Test `SyncTagsUseCase` with mock repository
+  - Test `CoreDataTagManagementView` sort order changes
+  - Test tag sync triggers in ViewModels
+
+- [ ] **Performance monitoring**
+  - Add metrics for tag sync duration
+  - Track cache hit rate
+  - Monitor Core Data batch operation performance
+
+- [ ] **Improve parameter naming**
+  - Rename `availableTagsTitle` to `customSectionTitle` or `sectionHeaderTitle`
+  - More descriptive than "available tags"
+
+---
+
+## 🎯 Summary
+
+### Overall Assessment: ✅ **EXCELLENT**
+
+This refactoring successfully achieves its goals:
+- ✅ Improved performance through caching
+- ✅ Better offline support
+- ✅ Cleaner architecture
+- ✅ Enhanced user experience
+
+### Risk Level: **LOW**
+
+The changes are well-structured and follow established patterns. The main risks are:
+1. Repository instantiation inefficiency (easily fixed)
+2. Silent error handling (minor, can be improved later)
+
+### Recommendation: **APPROVE with minor follow-ups**
+
+The code is production-ready. The identified improvements are optimizations and cleanups that can be addressed in follow-up commits without blocking deployment.
+
+---
+
+## 📊 Metrics
+
+- **Lines Added**: 747
+- **Lines Removed**: 264
+- **Net Change**: +483 lines
+- **Files Modified**: 31
+- **New Files**: 7
+- **Deleted Files**: 0 (1 renamed)
+- **Test Coverage**: Mocks added ✅
+
+---
+
+## 🏆 Best Practices Demonstrated
+
+1. ✅ Clean Architecture principles
+2. ✅ SOLID principles (especially Single Responsibility)
+3. ✅ Proper async/await usage
+4. ✅ SwiftUI best practices (@FetchRequest, @Published)
+5. ✅ Comprehensive localization
+6. ✅ Backwards compatibility (deprecated instead of deleted)
+7. ✅ Documentation and commit hygiene
+8. ✅ Testability through dependency injection
diff --git a/docs/Tags-Sync.md b/docs/Tags-Sync.md
new file mode 100644
index 0000000..0c30ce4
--- /dev/null
+++ b/docs/Tags-Sync.md
@@ -0,0 +1,203 @@
+# Tags Synchronization
+
+This document describes how tags (labels) are synchronized and updated throughout the readeck app.
+
+## Overview
+
+The app uses a **cache-first strategy** with background synchronization to ensure fast UI responses while keeping data up-to-date with the server.
+
+## Architecture
+
+### Components
+
+1. **Core Data Storage** (`TagEntity`)
+   - Local persistent storage for all tags
+   - Fields: `name` (String), `count` (Int32)
+   - Used as the single source of truth for all UI components
+
+2. **LabelsRepository**
+   - Manages tag synchronization between API and Core Data
+   - Implements cache-first loading strategy
+
+3. **CoreDataTagManagementView**
+   - SwiftUI view component for tag management
+   - Uses `@FetchRequest` to directly query Core Data
+   - Automatically updates when Core Data changes
+
+4. **LabelsView**
+   - Full-screen tag list view
+   - Accessible via "More" → "Tags" tab
+   - Triggers manual tag synchronization
+
+## Synchronization Flow
+
+### When Tags are Fetched
+
+Tags are synchronized in the following scenarios:
+
+#### 1. Opening the Tags Tab
+**Trigger**: User navigates to "More" → "Tags"
+**Location**: `LabelsView.swift:43-46`
+
+```swift
+.onAppear {
+    Task {
+        await viewModel.loadLabels()
+    }
+}
+```
+
+**Process**:
+1. Immediately loads tags from Core Data (instant response)
+2. Starts background API call to fetch latest tags
+3. Updates Core Data if API call succeeds
+4. Silently fails if server is unreachable (keeps cached data)
+
+#### 2. Background Sync Strategy
+**Implementation**: `LabelsRepository.getLabels()`
+
+The repository uses a two-phase approach:
+
+**Phase 1: Instant Response**
+```swift
+let cachedLabels = try await loadLabelsFromCoreData()
+```
+- Returns immediately with cached data
+- Ensures UI is never blocked
+
+**Phase 2: Background Update**
+```swift
+Task.detached(priority: .background) {
+    let dtos = try await self.api.getBookmarkLabels()
+    try? await self.saveLabels(dtos)
+}
+```
+- Runs asynchronously in background
+- Updates Core Data with latest server data
+- Silent failure - no error shown to user if sync fails
+
+#### 3. Adding a New Bookmark
+**Trigger**: User opens "Add Bookmark" sheet
+**Location**: `AddBookmarkView.swift:61-66`
+
+```swift
+.onAppear {
+    viewModel.checkClipboard()
+    Task {
+        await viewModel.syncTags()
+    }
+}
+```
+
+**Process**:
+1. Triggers background sync when view appears
+2. `CoreDataTagManagementView` shows cached tags immediately
+3. View automatically updates via `@FetchRequest` when sync completes
+
+#### 4. Editing Bookmark Labels
+**Trigger**: User opens "Manage Labels" sheet from bookmark detail
+**Location**: `BookmarkLabelsView.swift:49-53`
+
+```swift
+.onAppear {
+    Task {
+        await viewModel.syncTags()
+    }
+}
+```
+
+**Process**:
+1. Triggers background sync when view appears
+2. `CoreDataTagManagementView` shows cached tags immediately
+3. View automatically updates via `@FetchRequest` when sync completes
+
+#### 5. Share Extension
+
+Tags are **not** synced in the Share Extension:
+- Uses cached tags from Core Data only
+- No API calls to minimize extension launch time
+- Relies on tags synced by main app
+
+**Reason**: Share Extensions should be fast and lightweight. Tags are already synchronized by the main app when opening tags tab or managing bookmark labels.
+
+### When Core Data Updates
+
+Core Data tag updates trigger automatic UI refreshes in all views using `@FetchRequest`:
+- `CoreDataTagManagementView`
+- `LabelsView`
+
+This happens when:
+- Background sync completes successfully
+- New tags are created via bookmark operations
+- Tag counts change due to bookmark label modifications
+
+## Tag Display Configuration
+
+### Share Extension
+- **Fixed sorting**: Always by usage count (`.byCount`)
+- **Display limit**: Top 150 tags
+- **Label**: "Most used tags"
+- **Rationale**: Quick access to most frequently used tags for fast bookmark creation
+
+### Main App
+- **User-configurable sorting**: Either by usage count or alphabetically
+- **Display limit**: All tags (no limit)
+- **Setting location**: Settings → Appearance → Tag Sort Order
+- **Labels**:
+  - "Sorted by usage count" (when `.byCount`)
+  - "Sorted alphabetically" (when `.alphabetically`)
+
+## Data Persistence
+
+### Core Data Updates
+Tags in Core Data are updated through:
+
+1. **Batch sync** (`LabelsRepository.saveLabels`)
+   - Compares existing tags with new data from server
+   - Updates counts for existing tags
+   - Inserts new tags
+   - Only saves if changes detected
+
+2. **Efficiency optimizations**:
+   - Batch fetch of existing entities
+   - Dictionary-based lookups for fast comparison
+   - Conditional saves to minimize disk I/O
+
+## Error Handling
+
+### Network Failures
+- **Behavior**: Silent failure
+- **User Experience**: App continues to work with cached data
+- **Rationale**: Tags are not critical for app functionality; offline access is prioritized
+
+### Core Data Errors
+- **Read errors**: UI shows empty state or cached data
+- **Write errors**: Logged but do not block UI operations
+
+## Implementation Notes
+
+### Deprecated Components
+- `LegacyTagManagementView.swift`: Old API-based tag management (marked for removal)
+- `TagManagementView.swift`: Deleted, replaced by `CoreDataTagManagementView.swift`
+
+### Key Differences: New vs Old Approach
+
+**Old (LegacyTagManagementView)**:
+- Fetched tags from API on every view appearance
+- Slower initial load
+- Required network connectivity
+- More server load
+
+**New (CoreDataTagManagementView)**:
+- Uses Core Data with `@FetchRequest`
+- Instant UI response
+- Works offline
+- Automatic UI updates via SwiftUI reactivity
+- Reduced server load through background sync
+
+## Future Considerations
+
+1. **Offline tag creation**: Currently, new tags can be created offline but won't sync until server is reachable
+2. **Tag deletion**: Not implemented in current version
+3. **Tag renaming**: Not implemented in current version
+4. **Conflict resolution**: Tags created offline with same name as server tags will merge on sync
diff --git a/documentation/tabbar.md b/docs/tabbar.md
similarity index 100%
rename from documentation/tabbar.md
rename to docs/tabbar.md
diff --git a/readeck/UI/Settings/ReadingSettingsView.swift b/readeck/UI/Settings/ReadingSettingsView.swift
index 1cb03c2..89cbc05 100644
--- a/readeck/UI/Settings/ReadingSettingsView.swift
+++ b/readeck/UI/Settings/ReadingSettingsView.swift
@@ -31,10 +31,7 @@ struct ReadingSettingsView: View {
                         .padding(.top, 2)
                 }
 
-                #if DEBUG
-                Toggle("Safari Reader Mode", isOn: $viewModel.enableReaderMode)
-                Toggle("Automatically mark articles as read", isOn: $viewModel.autoMarkAsRead)
-                #endif
+                
             } header: {
                 Text("Reading Settings")
             }