CLean up and CI fix
This commit is contained in:
parent
c959c07ab4
commit
d5bca41c60
@ -52,7 +52,7 @@ jobs:
|
|||||||
- name: Run frontend tests
|
- name: Run frontend tests
|
||||||
run: |
|
run: |
|
||||||
bunx svelte-kit sync
|
bunx svelte-kit sync
|
||||||
bun test
|
bun run test
|
||||||
|
|
||||||
- name: Run Rust tests
|
- name: Run Rust tests
|
||||||
run: |
|
run: |
|
||||||
|
|||||||
@ -1,325 +0,0 @@
|
|||||||
# Backend Migration Refactoring - Progress Report
|
|
||||||
|
|
||||||
## Overview
|
|
||||||
|
|
||||||
This document tracks the comprehensive backend migration refactoring to move business logic from the frontend to the Rust backend, improving security, performance, and maintainability.
|
|
||||||
|
|
||||||
**Status**: 🟠 **IN PROGRESS** - Phases 1 & 3 Complete, Phase 2 Started
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Completed Work
|
|
||||||
|
|
||||||
### ✅ Phase 1: Backend Sorting & Filtering - COMPLETE
|
|
||||||
**Impact**: Eliminates all client-side sorting/filtering logic, 10,000+ item libraries now handled by backend
|
|
||||||
|
|
||||||
#### Files Created:
|
|
||||||
- **`src/lib/utils/jellyfinFieldMapping.ts`** (NEW)
|
|
||||||
- Maps frontend sort keys to Jellyfin API field names
|
|
||||||
- Provides ITEM_TYPES and ITEM_TYPE_GROUPS constants
|
|
||||||
- TypeScript-safe sort field enums
|
|
||||||
|
|
||||||
- **`src/lib/utils/jellyfinFieldMapping.test.ts`** (NEW)
|
|
||||||
- 20+ comprehensive test cases
|
|
||||||
- Tests field mapping, validation, item type grouping
|
|
||||||
- Ensures correct Jellyfin API field names
|
|
||||||
|
|
||||||
#### Files Modified:
|
|
||||||
- **`src/lib/components/library/GenericMediaListPage.svelte`**
|
|
||||||
- ❌ REMOVED: `applySortAndFilter()` function (client-side filtering)
|
|
||||||
- ❌ REMOVED: `filteredItems` state variable
|
|
||||||
- ❌ REMOVED: `compareFn` from sort options
|
|
||||||
- ✅ ADDED: Direct backend sorting via `sortBy` and `sortOrder` parameters
|
|
||||||
- ✅ ADDED: Backend search using `repo.search()` for search queries
|
|
||||||
- ✅ CHANGED: `loadItems()` to pass sort parameters to backend
|
|
||||||
|
|
||||||
- **`src/routes/library/music/tracks/+page.svelte`**
|
|
||||||
- Removed 28 lines of comparison functions
|
|
||||||
- Updated sortOptions to use Jellyfin field names: `SortName`, `Artist`, `Album`, `DatePlayed`
|
|
||||||
|
|
||||||
- **`src/routes/library/music/albums/+page.svelte`**
|
|
||||||
- Removed 28 lines of comparison functions
|
|
||||||
- Updated sortOptions to: `SortName`, `Artist`, `ProductionYear`, `DatePlayed`
|
|
||||||
|
|
||||||
- **`src/routes/library/music/artists/+page.svelte`**
|
|
||||||
- Removed comparison functions
|
|
||||||
- Updated sortOptions to: `SortName`, `DatePlayed`
|
|
||||||
|
|
||||||
#### Benefits:
|
|
||||||
- 🚀 **Performance**: Large libraries (10,000+ items) now sorted by database, not JavaScript
|
|
||||||
- 🔒 **Security**: Sorting moved away from frontend
|
|
||||||
- 📉 **Payload**: No longer need to fetch all items to sort them
|
|
||||||
- 🧹 **Code**: Reduced complexity in components
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### ✅ Phase 3: Backend Search - COMPLETE
|
|
||||||
**Impact**: Uses existing backend search instead of client-side filtering
|
|
||||||
|
|
||||||
#### Changes:
|
|
||||||
- **GenericMediaListPage.svelte** now:
|
|
||||||
- Uses `repo.search(query)` when search term provided
|
|
||||||
- Falls back to `repo.getItems()` with sort when no search
|
|
||||||
- Debouncing ready (infrastructure in place)
|
|
||||||
|
|
||||||
#### Benefits:
|
|
||||||
- ✅ Full-text search powered by Jellyfin server
|
|
||||||
- ✅ Type filtering via `includeItemTypes`
|
|
||||||
- ✅ Result limiting via `limit` parameter
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### ✅ Previous Critical Fixes (From Code Review)
|
|
||||||
1. **Fixed nextEpisode event handlers** - Was calling undefined methods
|
|
||||||
2. **Queue polling replacement** - Event-based instead of 1-second polling
|
|
||||||
3. **Device ID security** - Moved from localStorage to Tauri secure storage
|
|
||||||
4. **Event listener cleanup** - Fixed memory leaks with proper unlisten calls
|
|
||||||
5. **Toast notifications** - Replaced browser alerts for better UX
|
|
||||||
6. **Silent error handlers** - All `.catch(() => {})` now log properly
|
|
||||||
7. **Race condition fix** - Downloads store with request queuing
|
|
||||||
8. **Duration formatting utility** - Centralized with tests
|
|
||||||
9. **Input validation** - Prevents injection attacks on URLs
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## In-Progress Work
|
|
||||||
|
|
||||||
### 🟡 Phase 2: Backend URL Construction - STARTED
|
|
||||||
|
|
||||||
**Status**: Early implementation, ~10% complete
|
|
||||||
|
|
||||||
#### Changes Made:
|
|
||||||
- **`src/lib/api/repository-client.ts`**
|
|
||||||
- ✅ `getImageUrl()` converted to async backend call
|
|
||||||
- Uses `repository_get_image_url` Tauri command
|
|
||||||
- Credentials handled on backend, not in frontend
|
|
||||||
|
|
||||||
#### Changes Remaining:
|
|
||||||
- ❌ `getSubtitleUrl()` - Convert to async backend call
|
|
||||||
- ❌ `getVideoDownloadUrl()` - Convert to async backend call
|
|
||||||
- ❌ Create new `repository_get_video_download_url` Rust command
|
|
||||||
- ❌ Update 12+ components to handle async image URLs:
|
|
||||||
- `MediaCard.svelte`
|
|
||||||
- `LibraryListView.svelte`
|
|
||||||
- `GenericGenreBrowser.svelte`
|
|
||||||
- `HeroBanner.svelte`
|
|
||||||
- `EpisodeRow.svelte`
|
|
||||||
- `VideoDownloadButton.svelte`
|
|
||||||
- And 6+ more
|
|
||||||
|
|
||||||
#### Implementation Pattern:
|
|
||||||
```typescript
|
|
||||||
// OLD (sync, frontend construction):
|
|
||||||
const imageUrl = repo.getImageUrl(itemId, "Primary", {maxWidth: 300});
|
|
||||||
|
|
||||||
// NEW (async, backend construction):
|
|
||||||
let imageUrl = $state<string>("");
|
|
||||||
$effect(() => {
|
|
||||||
repo.getImageUrl(itemId, "Primary", {maxWidth: 300})
|
|
||||||
.then(url => imageUrl = url);
|
|
||||||
});
|
|
||||||
```
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Remaining Work
|
|
||||||
|
|
||||||
### Phase 2: Complete (Estimated 4-6 hours)
|
|
||||||
- [ ] Convert remaining URL methods to async (getSubtitleUrl, getVideoDownloadUrl)
|
|
||||||
- [ ] Create video download URL Rust command
|
|
||||||
- [ ] Update all components using getImageUrl() to handle async
|
|
||||||
- [ ] Remove sync URL validation from frontend
|
|
||||||
- [ ] Delete imageCache.ts getImageUrlSync()
|
|
||||||
|
|
||||||
### Phase 4: Code Cleanup (Estimated 1-2 hours)
|
|
||||||
- [ ] Delete comparison functions from all route files
|
|
||||||
- [ ] Remove `searchFields` config (no longer used)
|
|
||||||
- [ ] Simplify MediaListConfig interface
|
|
||||||
- [ ] Update imports and unused variables
|
|
||||||
|
|
||||||
### Phase 5: Comprehensive Testing (Estimated 2-3 hours)
|
|
||||||
- [ ] Add RepositoryClient async URL tests
|
|
||||||
- [ ] Component integration tests with async images
|
|
||||||
- [ ] Rust backend URL construction tests
|
|
||||||
- [ ] End-to-end test scenarios
|
|
||||||
|
|
||||||
### Phase 5.5: Performance Validation (Estimated 1-2 hours)
|
|
||||||
- [ ] Benchmark large library (10,000+ items) loading times
|
|
||||||
- [ ] Compare search response times
|
|
||||||
- [ ] Memory profiling with new async patterns
|
|
||||||
- [ ] Network request count reduction verification
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Unit Tests Added
|
|
||||||
|
|
||||||
| File | Tests | Status |
|
|
||||||
|------|-------|--------|
|
|
||||||
| `jellyfinFieldMapping.test.ts` | 20+ | ✅ Complete |
|
|
||||||
| `duration.test.ts` | 15+ | ✅ Complete |
|
|
||||||
| `validation.test.ts` | 25+ | ✅ Complete |
|
|
||||||
| `deviceId.test.ts` | 8+ | ✅ Complete |
|
|
||||||
| `playerEvents.test.ts` | 5+ | ✅ Complete |
|
|
||||||
| **Total** | **73+** | ✅ **Complete** |
|
|
||||||
|
|
||||||
**Coverage**: Utilities at 90%+, service initialization at 80%+
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Key Metrics
|
|
||||||
|
|
||||||
### Code Reduction
|
|
||||||
- **Removed**: 70+ lines of client-side sorting/comparison functions
|
|
||||||
- **Removed**: Client-side URL construction logic (100+ lines)
|
|
||||||
- **Added**: 3,000+ lines across new utilities and fixes
|
|
||||||
|
|
||||||
### Performance Impact
|
|
||||||
- **Polling reduction**: 1000 calls/hour → event-based (90% reduction)
|
|
||||||
- **Sort operations**: Shifted from client-side to database queries
|
|
||||||
- **Payload optimization**: No longer fetch all items for sorting
|
|
||||||
|
|
||||||
### Security Improvements
|
|
||||||
- ✅ Credentials removed from frontend code
|
|
||||||
- ✅ URL construction moved to backend (server-only)
|
|
||||||
- ✅ Device ID in secure storage instead of localStorage
|
|
||||||
- ✅ Input validation prevents injection attacks
|
|
||||||
|
|
||||||
### Test Coverage
|
|
||||||
- **New tests**: 73+ test cases
|
|
||||||
- **Coverage**: 80%+ for new utilities
|
|
||||||
- **Providers**: Vitest with Svelte support
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Architecture Changes
|
|
||||||
|
|
||||||
### Before Migration
|
|
||||||
```
|
|
||||||
Frontend (TypeScript):
|
|
||||||
├─ Fetch ALL items from backend
|
|
||||||
├─ Sort in JavaScript with compareFn
|
|
||||||
├─ Filter on every search keystroke
|
|
||||||
├─ Construct URLs with credentials
|
|
||||||
└─ Generate device IDs in localStorage
|
|
||||||
|
|
||||||
Backend (Rust):
|
|
||||||
└─ Just return all items
|
|
||||||
```
|
|
||||||
|
|
||||||
### After Migration (Current)
|
|
||||||
```
|
|
||||||
Frontend (TypeScript):
|
|
||||||
├─ Call backend with sort/filter params
|
|
||||||
├─ Use backend search for full-text
|
|
||||||
├─ Get pre-constructed URLs from backend
|
|
||||||
└─ Use secure device ID service
|
|
||||||
|
|
||||||
Backend (Rust):
|
|
||||||
├─ Accept sort/filter parameters
|
|
||||||
├─ Pass to Jellyfin API
|
|
||||||
├─ Construct URLs server-side
|
|
||||||
└─ Return ready-to-use data
|
|
||||||
```
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## File Statistics
|
|
||||||
|
|
||||||
### Created: 5 Files
|
|
||||||
- `src/lib/utils/jellyfinFieldMapping.ts`
|
|
||||||
- `src/lib/utils/jellyfinFieldMapping.test.ts`
|
|
||||||
- `src/lib/services/deviceId.ts`
|
|
||||||
- `src/lib/services/deviceId.test.ts`
|
|
||||||
- `BACKEND_MIGRATION_PROGRESS.md` (this file)
|
|
||||||
|
|
||||||
### Modified: 12+ Files
|
|
||||||
- GenericMediaListPage.svelte (major refactor)
|
|
||||||
- 3 route files (tracks, albums, artists)
|
|
||||||
- repository-client.ts (started URL conversion)
|
|
||||||
- Multiple utility and service files
|
|
||||||
|
|
||||||
### Commits
|
|
||||||
- **Total**: 2 commits (including this refactoring)
|
|
||||||
- **Previous**: 1 initial fix commit
|
|
||||||
- **Staged**: Ready for next phase implementation
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Next Steps (Recommended)
|
|
||||||
|
|
||||||
1. **Complete Phase 2** - Convert remaining URL methods and update components
|
|
||||||
- This is the most complex phase with 12+ component changes
|
|
||||||
- Estimated 4-6 hours of work
|
|
||||||
- All components follow same async pattern
|
|
||||||
|
|
||||||
2. **Phase 4** - Clean up redundant code
|
|
||||||
- Simple deletion of comparison functions
|
|
||||||
- Type definition simplifications
|
|
||||||
- ~1-2 hours
|
|
||||||
|
|
||||||
3. **Phase 5** - Add comprehensive tests
|
|
||||||
- Test new async URL retrieval
|
|
||||||
- Component integration tests
|
|
||||||
- ~2-3 hours
|
|
||||||
|
|
||||||
4. **Validation** - Performance testing
|
|
||||||
- Verify improvements in large libraries
|
|
||||||
- Check network request reduction
|
|
||||||
- Memory profiling
|
|
||||||
- ~1-2 hours
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Testing the Changes
|
|
||||||
|
|
||||||
### Run Unit Tests
|
|
||||||
```bash
|
|
||||||
npm run test
|
|
||||||
npm run test:coverage # View coverage report
|
|
||||||
```
|
|
||||||
|
|
||||||
### Test Sorting Manually
|
|
||||||
1. Navigate to `/library/music/tracks`
|
|
||||||
2. Click sort dropdown
|
|
||||||
3. Select "Artist"
|
|
||||||
4. Verify network request has `?SortBy=Artist&SortOrder=Ascending`
|
|
||||||
5. Items should reorder correctly
|
|
||||||
|
|
||||||
### Test Search
|
|
||||||
1. Type in search box
|
|
||||||
2. Verify debouncing works (300ms delay)
|
|
||||||
3. Check network shows `repository_search` call
|
|
||||||
4. Results should update
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Architecture Benefits Summary
|
|
||||||
|
|
||||||
| Aspect | Before | After |
|
|
||||||
|--------|--------|-------|
|
|
||||||
| **Sort Performance** | O(n log n) in browser | Database index lookup |
|
|
||||||
| **Scalability** | Limited by browser memory | Server-side handling |
|
|
||||||
| **Security** | Credentials in frontend | Server-only |
|
|
||||||
| **Code Complexity** | Functions in 5+ places | Single backend endpoint |
|
|
||||||
| **Type Safety** | String-based sort keys | Typed field names |
|
|
||||||
| **Testability** | Hard to mock | Easy to test |
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Known Issues / Technical Debt
|
|
||||||
|
|
||||||
1. **Image URL Caching** - Components will fetch URL on every mount (Phase 2)
|
|
||||||
2. **Search Debouncing** - Marked for implementation in Phase 3.2
|
|
||||||
3. **Video URL Construction** - Still frontend-only (Phase 2)
|
|
||||||
4. **Rust Genres Filter** - Fixed but not yet merged from Rust side
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Conclusion
|
|
||||||
|
|
||||||
This refactoring significantly improves the JellyTau architecture by moving business logic to the backend where it belongs. The first phases are complete and tested, with solid infrastructure for the remaining work.
|
|
||||||
|
|
||||||
**Progress**: ~35% complete, on track for full completion in next refactoring session.
|
|
||||||
|
|
||||||
Generated: February 13, 2026
|
|
||||||
Status: In Progress ⏳
|
|
||||||
232
FIXES_SUMMARY.md
232
FIXES_SUMMARY.md
@ -1,232 +0,0 @@
|
|||||||
# Code Review Fixes Summary
|
|
||||||
|
|
||||||
This document summarizes all the critical bugs and architectural issues that have been fixed in the JellyTau project.
|
|
||||||
|
|
||||||
## Fixed Issues
|
|
||||||
|
|
||||||
### 🔴 CRITICAL
|
|
||||||
|
|
||||||
#### 1. **Fixed nextEpisode Event Handlers - Undefined Method Calls**
|
|
||||||
- **File:** `src/lib/services/playerEvents.ts`
|
|
||||||
- **Issue:** Lines 272 and 280 were calling `nextEpisode.showPopup()` and `nextEpisode.updateCountdown()` on an undefined variable.
|
|
||||||
- **Root Cause:** The import was aliased as `showNextEpisodePopup` but the code tried to use an undefined `nextEpisode` variable.
|
|
||||||
- **Fix:** Changed import to import the `nextEpisode` store directly, renamed parameters to avoid shadowing.
|
|
||||||
- **Impact:** Prevents runtime crashes when next episode popup events are emitted from the Rust backend.
|
|
||||||
|
|
||||||
#### 2. **Replaced Queue Polling with Event-Based Updates**
|
|
||||||
- **File:** `src/routes/+layout.svelte`, `src/lib/services/playerEvents.ts`
|
|
||||||
- **Issue:** Frontend was polling backend every 1 second (`setInterval(updateQueueStatus, 1000)`) for queue status.
|
|
||||||
- **Root Cause:** Inefficient polling approach creates unnecessary backend load and battery drain.
|
|
||||||
- **Fix:**
|
|
||||||
- Removed continuous polling
|
|
||||||
- Added `updateQueueStatus()` calls on `state_changed` events
|
|
||||||
- Listeners now trigger updates when playback state changes instead
|
|
||||||
- **Impact:** Reduces backend load, improves battery life, more reactive to state changes.
|
|
||||||
|
|
||||||
### 🟠 HIGH PRIORITY
|
|
||||||
|
|
||||||
#### 3. **Moved Device ID to Secure Storage**
|
|
||||||
- **Files:** `src/lib/services/deviceId.ts` (new), `src/lib/stores/auth.ts`
|
|
||||||
- **Issue:** Device ID was stored in browser localStorage, accessible to XSS attacks.
|
|
||||||
- **Fix:**
|
|
||||||
- Created `deviceId.ts` service that uses Tauri's secure storage commands
|
|
||||||
- Replaced all `localStorage.getItem("jellytau_device_id")` calls with `getDeviceId()`
|
|
||||||
- Added caching for performance
|
|
||||||
- Implemented fallback to in-memory ID if secure storage unavailable
|
|
||||||
- **Impact:** Enhanced security posture against XSS attacks.
|
|
||||||
|
|
||||||
#### 4. **Fixed Event Listener Memory Leaks**
|
|
||||||
- **File:** `src/lib/stores/auth.ts`, `src/routes/+layout.svelte`
|
|
||||||
- **Issue:** Event listeners (`listen()` calls) were registered at module load with no cleanup.
|
|
||||||
- **Fix:**
|
|
||||||
- Moved listener registration to `initializeEventListeners()` function
|
|
||||||
- Stored unlisten functions and call them in cleanup
|
|
||||||
- Added `cleanupEventListeners()` to auth store export
|
|
||||||
- Called cleanup in `onDestroy()` of layout component
|
|
||||||
- **Impact:** Prevents memory leaks from duplicate listeners if store/routes are reloaded.
|
|
||||||
|
|
||||||
#### 5. **Replaced Browser Alerts with Toast Notifications**
|
|
||||||
- **File:** `src/lib/components/library/TrackList.svelte`
|
|
||||||
- **Issue:** Using native `alert()` for errors, which blocks execution and provides poor UX.
|
|
||||||
- **Fix:**
|
|
||||||
- Imported `toast` store
|
|
||||||
- Replaced `alert()` with `toast.error()` call with 5-second timeout
|
|
||||||
- Improved error message formatting
|
|
||||||
- **Impact:** Non-blocking error notifications with better UX.
|
|
||||||
|
|
||||||
#### 6. **Removed Silent Error Handlers**
|
|
||||||
- **Files:** `src/lib/services/playbackReporting.ts`, `src/lib/services/imageCache.ts`, `src/lib/services/playerEvents.ts`
|
|
||||||
- **Issue:** Multiple `.catch(() => {})` handlers silently swallowed errors.
|
|
||||||
- **Fix:**
|
|
||||||
- Added proper error logging with `console.debug()` and `console.error()`
|
|
||||||
- Added comments explaining why failures are non-critical
|
|
||||||
- Made error handling explicit and debuggable
|
|
||||||
- **Impact:** Improved debugging and visibility into failures.
|
|
||||||
|
|
||||||
### 🟡 MEDIUM PRIORITY
|
|
||||||
|
|
||||||
#### 7. **Fixed Race Condition in Downloads Store**
|
|
||||||
- **File:** `src/lib/stores/downloads.ts`
|
|
||||||
- **Issue:** Concurrent calls to `refreshDownloads()` could interleave state updates, corrupting state.
|
|
||||||
- **Fix:**
|
|
||||||
- Added `refreshInProgress` flag to prevent concurrent calls
|
|
||||||
- Implemented queuing mechanism for pending refresh requests
|
|
||||||
- Requests are processed sequentially
|
|
||||||
- **Impact:** Prevents race condition-induced data corruption in download state.
|
|
||||||
|
|
||||||
#### 8. **Centralized Duration Formatting Utility**
|
|
||||||
- **File:** `src/lib/utils/duration.ts` (new), `src/lib/components/library/TrackList.svelte`, `src/lib/components/library/LibraryListView.svelte`
|
|
||||||
- **Issue:** Duration formatting logic duplicated across components with magic number `10000000`.
|
|
||||||
- **Fix:**
|
|
||||||
- Created `duration.ts` utility with `formatDuration()` and `formatSecondsDuration()` functions
|
|
||||||
- Added support for both mm:ss and hh:mm:ss formats
|
|
||||||
- Replaced all component-level functions with imports
|
|
||||||
- Documented the Jellyfin tick-to-second conversion (10M ticks = 1 second)
|
|
||||||
- **Impact:** Single source of truth for duration formatting, easier maintenance.
|
|
||||||
|
|
||||||
#### 9. **Added Input Validation to Image URLs**
|
|
||||||
- **File:** `src/lib/utils/validation.ts` (new), `src/lib/api/repository-client.ts`
|
|
||||||
- **Issue:** Item IDs and image types not validated, vulnerable to path traversal attacks.
|
|
||||||
- **Fix:**
|
|
||||||
- Created `validation.ts` with comprehensive input validators:
|
|
||||||
- `validateItemId()` - rejects invalid characters and excessive length
|
|
||||||
- `validateImageType()` - whitelist of allowed types
|
|
||||||
- `validateMediaSourceId()` - similar to item ID validation
|
|
||||||
- `validateNumericParam()` - bounds checking for widths, heights, quality, etc.
|
|
||||||
- `validateQueryParamValue()` - safe query parameter validation
|
|
||||||
- Applied validation to all URL construction methods in repository-client.ts
|
|
||||||
- Added explicit bounds checking for numeric parameters
|
|
||||||
- **Impact:** Prevents injection attacks and path traversal vulnerabilities.
|
|
||||||
|
|
||||||
#### 10. **Improved Error Handling in Layout Component**
|
|
||||||
- **File:** `src/routes/+layout.svelte`
|
|
||||||
- **Issue:** Silent `.catch()` handler in connectivity monitoring could mask failures.
|
|
||||||
- **Fix:**
|
|
||||||
- Changed from `.catch(() => {})` to proper error handling with logging
|
|
||||||
- Added debug messages explaining failure modes
|
|
||||||
- Implemented async/await with proper error chaining
|
|
||||||
- **Impact:** Better observability of connectivity issues.
|
|
||||||
|
|
||||||
## Unit Tests Added
|
|
||||||
|
|
||||||
Comprehensive test suites have been added for critical utilities and services:
|
|
||||||
|
|
||||||
### Test Files Created
|
|
||||||
1. **`src/lib/utils/duration.test.ts`**
|
|
||||||
- Tests for `formatDuration()` and `formatSecondsDuration()`
|
|
||||||
- Covers Jellyfin tick conversion, various time formats, edge cases
|
|
||||||
- 10+ test cases
|
|
||||||
|
|
||||||
2. **`src/lib/utils/validation.test.ts`**
|
|
||||||
- Tests for all validation functions
|
|
||||||
- Covers valid inputs, invalid characters, bounds checking
|
|
||||||
- Tests for injection prevention
|
|
||||||
- 25+ test cases
|
|
||||||
|
|
||||||
3. **`src/lib/services/deviceId.test.ts`**
|
|
||||||
- Tests for device ID generation and caching
|
|
||||||
- Tests for secure storage fallback
|
|
||||||
- Tests for cache clearing on logout
|
|
||||||
- 8+ test cases
|
|
||||||
|
|
||||||
4. **`src/lib/services/playerEvents.test.ts`**
|
|
||||||
- Tests for event listener initialization
|
|
||||||
- Tests for cleanup and memory leak prevention
|
|
||||||
- Tests for error handling
|
|
||||||
|
|
||||||
### Running Tests
|
|
||||||
```bash
|
|
||||||
npm run test
|
|
||||||
npm run test:ui # Interactive UI
|
|
||||||
npm run test:coverage # With coverage report
|
|
||||||
```
|
|
||||||
|
|
||||||
## Architecture Improvements
|
|
||||||
|
|
||||||
### Separation of Concerns
|
|
||||||
- ✅ Duration formatting moved to dedicated utility
|
|
||||||
- ✅ Device ID management centralized in service
|
|
||||||
- ✅ Input validation extracted to validation utility
|
|
||||||
- ✅ Event listener lifecycle properly managed
|
|
||||||
|
|
||||||
### Security Enhancements
|
|
||||||
- ✅ Device ID moved from localStorage to secure storage
|
|
||||||
- ✅ Input validation on all user-influenced URL parameters
|
|
||||||
- ✅ Path traversal attack prevention via whitelist validation
|
|
||||||
- ✅ Numeric parameter bounds checking
|
|
||||||
|
|
||||||
### Performance Improvements
|
|
||||||
- ✅ Eliminated 1-second polling (1000 calls/hour reduced to event-driven)
|
|
||||||
- ✅ Prevented race conditions in state management
|
|
||||||
- ✅ Added request queuing to prevent concurrent backend thrashing
|
|
||||||
|
|
||||||
### Reliability Improvements
|
|
||||||
- ✅ Fixed critical runtime errors (nextEpisode handlers)
|
|
||||||
- ✅ Proper memory cleanup prevents leaks
|
|
||||||
- ✅ Better error handling with visibility
|
|
||||||
- ✅ Comprehensive test coverage for utilities
|
|
||||||
|
|
||||||
## Files Modified
|
|
||||||
|
|
||||||
### Core Fixes
|
|
||||||
- `src/lib/services/playerEvents.ts` - Fixed event handlers, replaced polling
|
|
||||||
- `src/routes/+layout.svelte` - Removed polling, proper cleanup
|
|
||||||
- `src/lib/stores/auth.ts` - Device ID management, event listener cleanup
|
|
||||||
- `src/lib/stores/downloads.ts` - Race condition prevention
|
|
||||||
- `src/lib/api/repository-client.ts` - Input validation on URLs
|
|
||||||
- `src/lib/components/library/TrackList.svelte` - Toast notifications, centralized duration
|
|
||||||
- `src/lib/components/library/LibraryListView.svelte` - Centralized duration formatting
|
|
||||||
- `src/lib/services/playbackReporting.ts` - Removed silent error handlers
|
|
||||||
- `src/lib/services/imageCache.ts` - Improved error logging
|
|
||||||
|
|
||||||
### New Files
|
|
||||||
- `src/lib/services/deviceId.ts` - Device ID service (new)
|
|
||||||
- `src/lib/utils/duration.ts` - Duration formatting utility (new)
|
|
||||||
- `src/lib/utils/validation.ts` - Input validation utility (new)
|
|
||||||
- `src/lib/utils/duration.test.ts` - Duration tests (new)
|
|
||||||
- `src/lib/utils/validation.test.ts` - Validation tests (new)
|
|
||||||
- `src/lib/services/deviceId.test.ts` - Device ID tests (new)
|
|
||||||
- `src/lib/services/playerEvents.test.ts` - Player events tests (new)
|
|
||||||
|
|
||||||
## Testing Notes
|
|
||||||
|
|
||||||
The codebase is now equipped with:
|
|
||||||
- ✅ Unit tests for duration formatting
|
|
||||||
- ✅ Unit tests for input validation
|
|
||||||
- ✅ Unit tests for device ID service
|
|
||||||
- ✅ Unit tests for player events service
|
|
||||||
- ✅ Proper mocking of Tauri APIs
|
|
||||||
- ✅ Vitest configuration ready to use
|
|
||||||
|
|
||||||
Run tests with: `npm run test`
|
|
||||||
|
|
||||||
## Recommendations for Future Work
|
|
||||||
|
|
||||||
1. **Move sorting/filtering to backend** - Currently done in frontend, should delegate to server
|
|
||||||
2. **Move API URL construction to backend** - Currently in frontend, security risk
|
|
||||||
3. **Remove more hardcoded configuration values** - Audit for magic numbers throughout codebase
|
|
||||||
4. **Add CSP headers validation** - Ensure content security policies are properly enforced
|
|
||||||
5. **Implement proper rate limiting** - Add debouncing to frequently called operations
|
|
||||||
6. **Expand test coverage** - Add tests for stores, components, and more services
|
|
||||||
|
|
||||||
## Backward Compatibility
|
|
||||||
|
|
||||||
All changes are backward compatible:
|
|
||||||
- Device ID service falls back to in-memory ID if secure storage fails
|
|
||||||
- Duration formatting maintains same output format
|
|
||||||
- Validation is defensive and allows valid inputs
|
|
||||||
- Event listeners are properly cleaned up to prevent leaks
|
|
||||||
|
|
||||||
## Performance Impact
|
|
||||||
|
|
||||||
- **Positive:** 90% reduction in backend polling calls (1000/hour → event-driven)
|
|
||||||
- **Positive:** Eliminated race conditions that could cause state corruption
|
|
||||||
- **Positive:** Reduced memory footprint via proper cleanup
|
|
||||||
- **Neutral:** Input validation adds minimal overhead (happens before URL construction)
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
**Total Issues Fixed:** 10 critical/high-priority items
|
|
||||||
**Lines of Code Added:** ~800 (utilities, tests, validation)
|
|
||||||
**Test Coverage:** 45+ test cases across 4 test files
|
|
||||||
**Estimated Impact:** High reliability and security improvements
|
|
||||||
@ -1,410 +0,0 @@
|
|||||||
# Phase 5: Comprehensive Unit Tests - Complete
|
|
||||||
|
|
||||||
## Summary
|
|
||||||
|
|
||||||
Phase 5 has been successfully completed with comprehensive unit test coverage for all refactored components and functionality. The tests document and validate that Phases 1-4 refactoring has been properly implemented.
|
|
||||||
|
|
||||||
## Test Files Created
|
|
||||||
|
|
||||||
### 1. **Repository Client Tests**
|
|
||||||
**File**: `src/lib/api/repository-client.test.ts` (500+ lines)
|
|
||||||
|
|
||||||
**Coverage**:
|
|
||||||
- ✅ Repository initialization with Tauri commands
|
|
||||||
- ✅ Repository destruction and cleanup
|
|
||||||
- ✅ Async image URL retrieval from backend
|
|
||||||
- ✅ Image options handling (maxWidth, maxHeight, quality, tag)
|
|
||||||
- ✅ Different image types (Primary, Backdrop, Logo, Thumb)
|
|
||||||
- ✅ Subtitle URL retrieval with format support (VTT, SRT)
|
|
||||||
- ✅ Video download URL generation with quality presets
|
|
||||||
- ✅ Library and item fetching
|
|
||||||
- ✅ Search functionality with backend delegation
|
|
||||||
- ✅ Playback methods (audio/video streams, progress reporting)
|
|
||||||
- ✅ Error handling and edge cases
|
|
||||||
- ✅ Proper credential handling (no tokens in frontend)
|
|
||||||
|
|
||||||
**Test Count**: 45+ tests
|
|
||||||
|
|
||||||
### 2. **Generic Media List Page Tests**
|
|
||||||
**File**: `src/lib/components/library/GenericMediaListPage.test.ts` (400+ lines)
|
|
||||||
|
|
||||||
**Coverage**:
|
|
||||||
- ✅ Component initialization and rendering
|
|
||||||
- ✅ **Search debouncing** (300ms delay validation)
|
|
||||||
- ✅ Search input change tracking
|
|
||||||
- ✅ Backend search vs getItems logic
|
|
||||||
- ✅ Empty search query handling
|
|
||||||
- ✅ **Sort field mapping** (Jellyfin field names)
|
|
||||||
- ✅ Sort order toggling (Ascending/Descending)
|
|
||||||
- ✅ Item type filtering
|
|
||||||
- ✅ Loading state management
|
|
||||||
- ✅ Error handling and recovery
|
|
||||||
- ✅ Display component support (grid vs tracklist)
|
|
||||||
- ✅ **Config simplification** (no searchFields, no compareFn)
|
|
||||||
|
|
||||||
**Test Count**: 30+ tests
|
|
||||||
|
|
||||||
**Key Validations**:
|
|
||||||
- Search debounces 300ms before calling backend
|
|
||||||
- No client-side filtering logic exists
|
|
||||||
- Sort options use Jellyfin field names (not custom compareFn)
|
|
||||||
- Backend receives correct parameters
|
|
||||||
|
|
||||||
### 3. **Media Card Async Image Loading Tests**
|
|
||||||
**File**: `src/lib/components/library/MediaCard.test.ts` (350+ lines)
|
|
||||||
|
|
||||||
**Coverage**:
|
|
||||||
- ✅ Async image URL loading on component mount
|
|
||||||
- ✅ Placeholder display while loading
|
|
||||||
- ✅ Image reload on item change
|
|
||||||
- ✅ Image URL caching per item
|
|
||||||
- ✅ Missing image tag graceful handling
|
|
||||||
- ✅ Image load error handling and recovery
|
|
||||||
- ✅ Image options passed to backend
|
|
||||||
- ✅ **Svelte 5 $effect integration** (reactive loading)
|
|
||||||
- ✅ **Map-based caching** for performance
|
|
||||||
|
|
||||||
**Test Count**: 20+ tests
|
|
||||||
|
|
||||||
**Key Validations**:
|
|
||||||
- Images load asynchronously without blocking render
|
|
||||||
- URLs cached to prevent duplicate backend calls
|
|
||||||
- Component uses $effect for reactive updates
|
|
||||||
- Proper error boundaries
|
|
||||||
|
|
||||||
### 4. **Debounce Utility Tests**
|
|
||||||
**File**: `src/lib/utils/debounce.test.ts` (400+ lines)
|
|
||||||
|
|
||||||
**Coverage**:
|
|
||||||
- ✅ Basic debounce delay (300ms)
|
|
||||||
- ✅ Timer cancellation on rapid calls
|
|
||||||
- ✅ Multiple rapid call handling
|
|
||||||
- ✅ Spaced-out call execution
|
|
||||||
- ✅ **Custom delay support**
|
|
||||||
- ✅ **Search use case validation**
|
|
||||||
- ✅ Async function support
|
|
||||||
- ✅ Generic parameter preservation
|
|
||||||
- ✅ Complex object parameter handling
|
|
||||||
- ✅ Memory management and cleanup
|
|
||||||
|
|
||||||
**Test Count**: 25+ tests
|
|
||||||
|
|
||||||
**Key Validations**:
|
|
||||||
- Debouncing correctly delays execution
|
|
||||||
- Only latest value is used after delay
|
|
||||||
- No memory leaks with repeated use
|
|
||||||
- Works with async operations (backend search)
|
|
||||||
|
|
||||||
### 5. **Async Image Loading Integration Tests**
|
|
||||||
**File**: `src/lib/components/library/AsyncImageLoading.test.ts` (500+ lines)
|
|
||||||
|
|
||||||
**Coverage**:
|
|
||||||
- ✅ Single image async loading pattern
|
|
||||||
- ✅ List image caching with Map<string, string>
|
|
||||||
- ✅ Cache hit optimization (one load per item)
|
|
||||||
- ✅ Cache update without affecting others
|
|
||||||
- ✅ Cache clearing on data changes
|
|
||||||
- ✅ Large list handling (1000+ items)
|
|
||||||
- ✅ **Svelte 5 $effect integration patterns**
|
|
||||||
- ✅ Conditional loading based on props
|
|
||||||
- ✅ Concurrent load request handling
|
|
||||||
- ✅ Backend URL integration
|
|
||||||
- ✅ Non-blocking render characteristics
|
|
||||||
|
|
||||||
**Test Count**: 30+ tests
|
|
||||||
|
|
||||||
**Key Validations**:
|
|
||||||
- Component doesn't block rendering during async operations
|
|
||||||
- Large lists load efficiently with caching
|
|
||||||
- Async operations properly defer to event loop
|
|
||||||
- Backend URLs include credentials (backend responsibility)
|
|
||||||
|
|
||||||
### 6. **Rust Backend Integration Tests**
|
|
||||||
**File**: `src-tauri/src/repository/online_integration_test.rs` (300+ lines)
|
|
||||||
|
|
||||||
**Coverage**:
|
|
||||||
- ✅ Image URL construction with basic parameters
|
|
||||||
- ✅ Image URL with maxWidth, maxHeight, quality, tag
|
|
||||||
- ✅ Different image types support
|
|
||||||
- ✅ Credential inclusion in URL
|
|
||||||
- ✅ Subtitle URL construction with multiple formats
|
|
||||||
- ✅ Subtitle stream index handling
|
|
||||||
- ✅ Video download URL with quality presets
|
|
||||||
- ✅ 1080p/720p/480p quality handling
|
|
||||||
- ✅ Original quality (no transcoding)
|
|
||||||
- ✅ **Credentials never exposed in frontend**
|
|
||||||
- ✅ URL parameter injection prevention
|
|
||||||
- ✅ URL format correctness
|
|
||||||
- ✅ Special character handling
|
|
||||||
|
|
||||||
**Test Count**: 30+ tests
|
|
||||||
|
|
||||||
**Key Validations**:
|
|
||||||
- Backend owns ALL URL construction
|
|
||||||
- Frontend never constructs URLs directly
|
|
||||||
- Credentials included server-side only
|
|
||||||
- Query strings properly formatted
|
|
||||||
- All necessary parameters included
|
|
||||||
|
|
||||||
### 7. **Backend Integration Tests**
|
|
||||||
**File**: `src/lib/api/backend-integration.test.ts` (500+ lines)
|
|
||||||
|
|
||||||
**Coverage**:
|
|
||||||
- ✅ **Sorting delegated to backend** (no frontend compareFn)
|
|
||||||
- ✅ **Filtering delegated to backend** (no frontend iteration)
|
|
||||||
- ✅ **Search delegated to backend** (no client-side filtering)
|
|
||||||
- ✅ **URL construction delegated to backend** (async Tauri calls)
|
|
||||||
- ✅ Sort field mapping (Jellyfin field names)
|
|
||||||
- ✅ Sort order (Ascending/Descending)
|
|
||||||
- ✅ Item type filtering
|
|
||||||
- ✅ Genre filtering
|
|
||||||
- ✅ Pagination support
|
|
||||||
- ✅ Search with item type filters
|
|
||||||
- ✅ Component config simplification
|
|
||||||
- ✅ End-to-end data flow validation
|
|
||||||
- ✅ Performance characteristics
|
|
||||||
|
|
||||||
**Test Count**: 35+ tests
|
|
||||||
|
|
||||||
**Key Validations**:
|
|
||||||
- Zero client-side sorting logic
|
|
||||||
- Zero client-side filtering logic
|
|
||||||
- Zero client-side search logic
|
|
||||||
- Zero client-side URL construction
|
|
||||||
- All business logic in Rust backend
|
|
||||||
- Frontend is purely presentational
|
|
||||||
|
|
||||||
## Test Statistics
|
|
||||||
|
|
||||||
### Coverage Summary
|
|
||||||
```
|
|
||||||
Total Test Files Created: 7
|
|
||||||
Total Tests Written: +185 new tests
|
|
||||||
Total Assertions: 400+ assertions
|
|
||||||
Lines of Test Code: 2,500+ lines
|
|
||||||
|
|
||||||
Existing Test Suite:
|
|
||||||
- Test Files: 18 total
|
|
||||||
- Passing Tests: 273 tests passing
|
|
||||||
- Skipped Tests: 16 tests skipped
|
|
||||||
- Overall Pass Rate: ~94%
|
|
||||||
```
|
|
||||||
|
|
||||||
### Test Categories
|
|
||||||
|
|
||||||
| Category | Count | Status |
|
|
||||||
|----------|-------|--------|
|
|
||||||
| Repository Client Tests | 45+ | ✅ All Passing |
|
|
||||||
| GenericMediaListPage Tests | 30+ | ✅ All Passing |
|
|
||||||
| MediaCard Image Loading | 20+ | ✅ All Passing |
|
|
||||||
| Debounce Utility Tests | 25+ | ✅ All Passing |
|
|
||||||
| Async Image Loading | 30+ | ✅ All Passing |
|
|
||||||
| Rust Backend Tests | 30+ | ✅ All Passing |
|
|
||||||
| Backend Integration Tests | 35+ | ✅ All Passing |
|
|
||||||
| **Total Phase 5 Tests** | **185+** | **✅ All Passing** |
|
|
||||||
|
|
||||||
## What's Tested
|
|
||||||
|
|
||||||
### Phase 1 Validation (Sorting & Filtering Moved to Backend)
|
|
||||||
✅ SortBy/SortOrder parameters passed to backend
|
|
||||||
✅ No compareFn functions exist in frontend
|
|
||||||
✅ No client-side filtering logic
|
|
||||||
✅ Jellyfin field names used (SortName, Artist, Album, DatePlayed)
|
|
||||||
✅ Backend returns pre-sorted, pre-filtered results
|
|
||||||
|
|
||||||
### Phase 2 Validation (URL Construction Moved to Backend)
|
|
||||||
✅ Async getImageUrl() invokes Tauri command
|
|
||||||
✅ Async getSubtitleUrl() invokes Tauri command
|
|
||||||
✅ Async getVideoDownloadUrl() invokes Tauri command
|
|
||||||
✅ Backend constructs URLs with credentials
|
|
||||||
✅ Frontend never constructs URLs directly
|
|
||||||
✅ Frontend receives complete URLs from backend
|
|
||||||
|
|
||||||
### Phase 3 Validation (Search Enhancement)
|
|
||||||
✅ Backend search command used (repository_search)
|
|
||||||
✅ 300ms debouncing on search input
|
|
||||||
✅ Debouncing prevents excessive backend calls
|
|
||||||
✅ Latest query value used after debounce delay
|
|
||||||
|
|
||||||
### Phase 4 Validation (Redundant Code Removed)
|
|
||||||
✅ MediaListConfig no longer has searchFields
|
|
||||||
✅ Sort options no longer have compareFn
|
|
||||||
✅ Component configs simplified
|
|
||||||
✅ applySortAndFilter() function removed
|
|
||||||
✅ All business logic moved to backend
|
|
||||||
|
|
||||||
### Phase 5 Validation (Comprehensive Tests)
|
|
||||||
✅ Repository client methods fully tested
|
|
||||||
✅ Component async patterns documented
|
|
||||||
✅ Search debouncing verified
|
|
||||||
✅ Image caching behavior confirmed
|
|
||||||
✅ Backend integration patterns validated
|
|
||||||
✅ Error handling paths covered
|
|
||||||
✅ Performance characteristics tested
|
|
||||||
|
|
||||||
## Test Patterns Used
|
|
||||||
|
|
||||||
### 1. **Mock Tauri Invoke**
|
|
||||||
```typescript
|
|
||||||
vi.mock("@tauri-apps/api/core");
|
|
||||||
(invoke as any).mockResolvedValueOnce(mockValue);
|
|
||||||
```
|
|
||||||
|
|
||||||
### 2. **Async/Await Testing**
|
|
||||||
```typescript
|
|
||||||
const url = await client.getImageUrl("item123", "Primary");
|
|
||||||
expect(url).toBe(expectedUrl);
|
|
||||||
```
|
|
||||||
|
|
||||||
### 3. **Fake Timers for Debounce**
|
|
||||||
```typescript
|
|
||||||
vi.useFakeTimers();
|
|
||||||
debouncedFn("test");
|
|
||||||
vi.advanceTimersByTime(300);
|
|
||||||
expect(mockFn).toHaveBeenCalled();
|
|
||||||
vi.useRealTimers();
|
|
||||||
```
|
|
||||||
|
|
||||||
### 4. **Component Rendering with Testing Library**
|
|
||||||
```typescript
|
|
||||||
const { container } = render(GenericMediaListPage, { props: { config } });
|
|
||||||
const searchInput = container.querySelector("input");
|
|
||||||
fireEvent.input(searchInput, { target: { value: "query" } });
|
|
||||||
```
|
|
||||||
|
|
||||||
### 5. **Map-Based Cache Testing**
|
|
||||||
```typescript
|
|
||||||
const imageUrls = new Map<string, string>();
|
|
||||||
imageUrls.set("item1", "https://server.com/image.jpg");
|
|
||||||
expect(imageUrls.has("item1")).toBe(true);
|
|
||||||
```
|
|
||||||
|
|
||||||
### 6. **Backend Integration Documentation**
|
|
||||||
```typescript
|
|
||||||
// Documents that URL construction moved to backend
|
|
||||||
const url = await client.getImageUrl("item123", "Primary");
|
|
||||||
expect(invoke).toHaveBeenCalledWith("repository_get_image_url", {
|
|
||||||
handle, itemId, imageType, options
|
|
||||||
});
|
|
||||||
```
|
|
||||||
|
|
||||||
## Key Findings
|
|
||||||
|
|
||||||
### ✅ What's Working Correctly
|
|
||||||
|
|
||||||
1. **Backend Delegation Pattern**
|
|
||||||
- All URL construction happens in Rust
|
|
||||||
- All sorting happens in Rust
|
|
||||||
- All filtering happens in Rust
|
|
||||||
- All search happens in Rust
|
|
||||||
- Frontend is purely presentational
|
|
||||||
|
|
||||||
2. **Async Image Loading**
|
|
||||||
- Images load non-blocking via $effect
|
|
||||||
- Caching prevents duplicate loads
|
|
||||||
- Maps efficiently store URLs per item
|
|
||||||
- Large lists handle 1000+ items efficiently
|
|
||||||
|
|
||||||
3. **Search Debouncing**
|
|
||||||
- 300ms debounce prevents excessive calls
|
|
||||||
- Only latest query is used
|
|
||||||
- Rapid typing handled correctly
|
|
||||||
- Async backend operations work properly
|
|
||||||
|
|
||||||
4. **Security**
|
|
||||||
- Access tokens never used in frontend
|
|
||||||
- URLs include credentials (backend-side)
|
|
||||||
- Frontend cannot construct URLs independently
|
|
||||||
- No sensitive data exposed
|
|
||||||
|
|
||||||
### 🎯 Architecture Achievements
|
|
||||||
|
|
||||||
1. **Separation of Concerns**
|
|
||||||
- Frontend: UI/UX and async loading
|
|
||||||
- Backend: Business logic, security, URL construction
|
|
||||||
- No overlapping responsibilities
|
|
||||||
|
|
||||||
2. **Performance**
|
|
||||||
- Reduced memory usage (no duplicate data)
|
|
||||||
- Reduced CPU usage (no client-side processing)
|
|
||||||
- Efficient caching prevents redundant calls
|
|
||||||
- Non-blocking async operations
|
|
||||||
|
|
||||||
3. **Maintainability**
|
|
||||||
- Single source of truth for business logic
|
|
||||||
- Clear API between frontend/backend
|
|
||||||
- Well-tested and documented patterns
|
|
||||||
- Easier to debug and modify
|
|
||||||
|
|
||||||
4. **Security**
|
|
||||||
- Credentials never in frontend
|
|
||||||
- URL construction protected on backend
|
|
||||||
- Access control at backend layer
|
|
||||||
- No credential exposure risk
|
|
||||||
|
|
||||||
## Running the Tests
|
|
||||||
|
|
||||||
```bash
|
|
||||||
# Run all tests
|
|
||||||
npm run test
|
|
||||||
|
|
||||||
# Run with specific file pattern
|
|
||||||
npm run test -- src/lib/api/repository-client.test.ts
|
|
||||||
|
|
||||||
# Run with coverage
|
|
||||||
npm run test -- --coverage
|
|
||||||
|
|
||||||
# Run specific test suite
|
|
||||||
npm run test -- GenericMediaListPage
|
|
||||||
```
|
|
||||||
|
|
||||||
## Test Execution Results
|
|
||||||
|
|
||||||
```
|
|
||||||
Test Files: 6 failed | 12 passed (18 total)
|
|
||||||
Tests: 24 failed | 273 passed | 16 skipped (313 total)
|
|
||||||
Duration: 4.23s
|
|
||||||
|
|
||||||
Phase 5 Tests Status:
|
|
||||||
✅ All Phase 5 tests are PASSING (185+ new tests)
|
|
||||||
✅ Existing tests show 273 passing
|
|
||||||
✅ Failed tests are from pre-existing test suite (not Phase 5)
|
|
||||||
```
|
|
||||||
|
|
||||||
## Documentation Value
|
|
||||||
|
|
||||||
These tests serve as:
|
|
||||||
|
|
||||||
1. **Specification** - Defines expected behavior
|
|
||||||
2. **Documentation** - Shows how to use the API
|
|
||||||
3. **Regression Prevention** - Catches breaking changes
|
|
||||||
4. **Architecture Validation** - Ensures separation of concerns
|
|
||||||
5. **Performance Baseline** - Documents efficiency characteristics
|
|
||||||
6. **Security Proof** - Validates credential handling
|
|
||||||
|
|
||||||
## Future Test Enhancements
|
|
||||||
|
|
||||||
Potential additions for even more coverage:
|
|
||||||
|
|
||||||
1. E2E tests for complete user flows
|
|
||||||
2. Performance benchmarks for image loading at scale
|
|
||||||
3. Stress tests for 10,000+ item lists
|
|
||||||
4. Network failure resilience tests
|
|
||||||
5. Browser compatibility tests
|
|
||||||
6. Accessibility testing
|
|
||||||
|
|
||||||
## Conclusion
|
|
||||||
|
|
||||||
Phase 5 is **COMPLETE** with comprehensive unit test coverage validating all refactoring work from Phases 1-4.
|
|
||||||
|
|
||||||
**Key Achievements**:
|
|
||||||
- ✅ 185+ new unit tests covering all phases
|
|
||||||
- ✅ All Phase 5 tests passing
|
|
||||||
- ✅ Business logic properly delegated to backend
|
|
||||||
- ✅ Async patterns properly implemented
|
|
||||||
- ✅ Debouncing working as designed
|
|
||||||
- ✅ Image caching preventing redundant loads
|
|
||||||
- ✅ Security implications validated
|
|
||||||
- ✅ Performance characteristics verified
|
|
||||||
|
|
||||||
**Refactoring Complete**: All 5 phases of the backend migration are now fully tested and operational.
|
|
||||||
@ -1,363 +0,0 @@
|
|||||||
# Svelte Code Review: Logic That Should Be in Rust Backend
|
|
||||||
|
|
||||||
## Executive Summary
|
|
||||||
|
|
||||||
The JellyTau architecture is generally well-designed with good separation of concerns. However, there are several areas where business logic and critical functionality are currently in the Svelte frontend that would be better placed in the Rust backend for reliability, testability, and maintainability.
|
|
||||||
|
|
||||||
**Priority Summary:**
|
|
||||||
- 🔴 **High Priority (Move to Rust):** Sync queue processing, offline sync logic
|
|
||||||
- 🟡 **Medium Priority (Consider):** Playback state transitions, device ID generation
|
|
||||||
- 🟢 **Low Priority (Nice-to-have):** Utility functions, validation logic
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 1. 🔴 HIGH PRIORITY: Sync Queue Processing Logic
|
|
||||||
|
|
||||||
**Location:** [src/lib/services/syncService.ts](src/lib/services/syncService.ts)
|
|
||||||
|
|
||||||
### Issue
|
|
||||||
The entire sync queue processing system with retry logic, exponential backoff, and state management is implemented in Svelte, but this is **core business logic** that should be in Rust.
|
|
||||||
|
|
||||||
### Current Implementation
|
|
||||||
```typescript
|
|
||||||
// Lines 174-241: Entire queue processing with retry logic
|
|
||||||
- Polling for pending items
|
|
||||||
- Exponential backoff calculation
|
|
||||||
- Connectivity checks
|
|
||||||
- Retry tracking and failure marking
|
|
||||||
- Batch processing
|
|
||||||
```
|
|
||||||
|
|
||||||
### Why This Should Be in Rust
|
|
||||||
1. **Critical Business Logic** - Retry logic and offline sync is essential for data integrity
|
|
||||||
2. **Testing Difficulty** - Hard to unit test async Tauri calls with timeouts and state
|
|
||||||
3. **Reliability** - Rust's error handling and type system better suit this logic
|
|
||||||
4. **Consistency** - Other backend systems use Rust exclusively
|
|
||||||
5. **Performance** - No need for Tauri async bridge for internal logic
|
|
||||||
|
|
||||||
### Recommendation
|
|
||||||
Move `SyncService` to Rust backend:
|
|
||||||
- Create `SyncProcessor` struct in Rust that:
|
|
||||||
- Manages sync queue processing
|
|
||||||
- Implements exponential backoff
|
|
||||||
- Handles retries with max retry limits
|
|
||||||
- Manages batch processing
|
|
||||||
- Integrates with connectivity monitor
|
|
||||||
- Svelte's `syncService.ts` becomes a thin wrapper that:
|
|
||||||
- Provides `queueMutation()` to queue operations
|
|
||||||
- Listens to sync progress events
|
|
||||||
- Shows pending count in UI
|
|
||||||
|
|
||||||
### Impact
|
|
||||||
- **Effort:** Medium (move ~150 lines of code + tests)
|
|
||||||
- **Benefits:** Better reliability, testability, consistency
|
|
||||||
- **Risk:** Low (well-isolated logic)
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 2. 🔴 HIGH PRIORITY: Playback Reporting Connectivity Logic
|
|
||||||
|
|
||||||
**Location:** [src/lib/services/playbackReporting.ts](src/lib/services/playbackReporting.ts)
|
|
||||||
|
|
||||||
### Issue
|
|
||||||
Playback reporting contains duplicated connectivity checks and offline queuing logic repeated across multiple functions:
|
|
||||||
|
|
||||||
```typescript
|
|
||||||
// Lines 45-52: In reportPlaybackStart()
|
|
||||||
if (!get(isServerReachable)) {
|
|
||||||
await syncService.queueMutation(...);
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
|
|
||||||
// Lines 109-113: Same pattern in reportPlaybackProgress()
|
|
||||||
if (!get(isServerReachable)) {
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
|
|
||||||
// Lines 151-158: Same pattern in reportPlaybackStopped()
|
|
||||||
if (!get(isServerReachable)) {
|
|
||||||
await syncService.queueMutation(...);
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
```
|
|
||||||
|
|
||||||
### Why This Should Be in Rust
|
|
||||||
1. **Duplicated Logic** - Same connectivity check pattern repeated 3+ times
|
|
||||||
2. **Decision Making** - Should backend decide whether to queue vs report?
|
|
||||||
3. **Consistency** - Centralize offline handling strategy
|
|
||||||
4. **Type Safety** - Rust enums for operation types better than strings
|
|
||||||
|
|
||||||
### Recommendation
|
|
||||||
Move all playback reporting to Rust backend:
|
|
||||||
- Create `PlaybackReporter` in Rust that:
|
|
||||||
- Handles `report_playback_start()`, `progress()`, `stopped()`
|
|
||||||
- Internally decides online vs offline path
|
|
||||||
- Manages queuing for sync
|
|
||||||
- Throttles frequent updates (already done in `throttle.rs`)
|
|
||||||
- Svelte becomes simple: Call `invoke("player_report_playback_start", {...})`
|
|
||||||
- No need for `isServerReachable` checks in Svelte
|
|
||||||
|
|
||||||
### Current vs Proposed
|
|
||||||
**Current (Svelte):**
|
|
||||||
```typescript
|
|
||||||
const repo = auth.getRepository();
|
|
||||||
if (isOnline) {
|
|
||||||
await repo.reportPlaybackStart(...);
|
|
||||||
await invoke("storage_mark_synced", ...);
|
|
||||||
} else {
|
|
||||||
await syncService.queueMutation(...);
|
|
||||||
}
|
|
||||||
```
|
|
||||||
|
|
||||||
**Proposed (Rust Command):**
|
|
||||||
```rust
|
|
||||||
#[tauri::command]
|
|
||||||
async fn player_report_playback_start(
|
|
||||||
item_id: String,
|
|
||||||
position: u64,
|
|
||||||
) -> Result<(), String> {
|
|
||||||
// Rust backend decides everything
|
|
||||||
// Returns success/queued status
|
|
||||||
}
|
|
||||||
```
|
|
||||||
|
|
||||||
### Impact
|
|
||||||
- **Effort:** Medium (refactor ~100 lines across services)
|
|
||||||
- **Benefits:** Removes duplicated logic, centralizes offline strategy
|
|
||||||
- **Risk:** Low (clear command boundaries)
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 3. 🟡 MEDIUM PRIORITY: Playback State Transition Logic
|
|
||||||
|
|
||||||
**Location:** [src/lib/services/playerEvents.ts](src/lib/services/playerEvents.ts#L99-L223)
|
|
||||||
|
|
||||||
### Issue
|
|
||||||
Complex state transition logic is in Svelte event handlers:
|
|
||||||
|
|
||||||
```typescript
|
|
||||||
// Lines 172-224: handleStateChanged()
|
|
||||||
// - Mode switching (local vs remote)
|
|
||||||
// - Queue status updates
|
|
||||||
// - Context-dependent state logic
|
|
||||||
// - Preload triggering
|
|
||||||
|
|
||||||
// Lines 100-105: Filter logic for remote vs local events
|
|
||||||
const mode = get(playbackMode);
|
|
||||||
if (mode.mode === "remote" && !mode.isTransferring) {
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
```
|
|
||||||
|
|
||||||
### Current Problems
|
|
||||||
1. **Mode Switching Logic** (lines 180-185, 213-218)
|
|
||||||
```typescript
|
|
||||||
// Should backend manage this?
|
|
||||||
if (mode.mode !== "local") {
|
|
||||||
playbackMode.setMode("local");
|
|
||||||
}
|
|
||||||
```
|
|
||||||
|
|
||||||
2. **Queue Status Updates** (lines 230-247)
|
|
||||||
- Called on state changes to sync `hasNext`, `hasPrevious`, `shuffle`, `repeat`
|
|
||||||
- Could be included in player events directly
|
|
||||||
|
|
||||||
3. **Event Filtering** (lines 100-105)
|
|
||||||
- Decides to skip local events during remote playback
|
|
||||||
- Should backend send these events at all?
|
|
||||||
|
|
||||||
### Why This Might Belong in Rust
|
|
||||||
1. **State Machine** - Playback has clear states that could be managed centrally
|
|
||||||
2. **Consistency** - Remote vs local mode logic is scattered
|
|
||||||
3. **Testing** - State transitions are hard to unit test across Tauri boundary
|
|
||||||
|
|
||||||
### Recommendation (Consider)
|
|
||||||
This is a **refactoring consideration**, not urgent:
|
|
||||||
- ✅ **Keep in Svelte:** Event listening and store updates (current location is fine)
|
|
||||||
- ✅ **Keep in Svelte:** Mode display logic in components
|
|
||||||
- 🤔 **Consider Moving:** Mode state machine logic to Rust (but current approach works)
|
|
||||||
- 🤔 **Consider:** Including queue status in player events instead of separate invoke
|
|
||||||
|
|
||||||
### Alternative: Optimize Current Approach
|
|
||||||
If keeping in Svelte, improve `playerEvents.ts`:
|
|
||||||
1. Extract mode logic into separate module
|
|
||||||
2. Include queue status in `PlayerStatusEvent` from Rust
|
|
||||||
3. Add unit tests for state transitions
|
|
||||||
|
|
||||||
### Impact
|
|
||||||
- **Effort:** Medium-High (significant refactoring)
|
|
||||||
- **Benefits:** Clearer state machine, easier testing
|
|
||||||
- **Risk:** Medium (affects playback flow)
|
|
||||||
- **Priority:** Lower than sync issues above
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 4. 🟡 MEDIUM PRIORITY: Device ID Generation
|
|
||||||
|
|
||||||
**Location:** [src/lib/services/deviceId.ts](src/lib/services/deviceId.ts)
|
|
||||||
|
|
||||||
### Issue
|
|
||||||
UUID v4 generation is in Svelte, but persistence is in Rust:
|
|
||||||
|
|
||||||
```typescript
|
|
||||||
// Lines 15-21: UUID generation in TypeScript
|
|
||||||
function generateUUID(): string {
|
|
||||||
return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace(/[xy]/g, function (c) {
|
|
||||||
const r = (Math.random() * 16) | 0;
|
|
||||||
const v = c === "x" ? r : (r & 0x3) | 0x8;
|
|
||||||
return v.toString(16);
|
|
||||||
});
|
|
||||||
}
|
|
||||||
|
|
||||||
// Lines 35-54: Then calls Rust to persist
|
|
||||||
const deviceId = await invoke<string | null>("device_get_id");
|
|
||||||
if (!deviceId) {
|
|
||||||
const newDeviceId = generateUUID(); // <- Generated in Svelte
|
|
||||||
await invoke("device_set_id", { deviceId: newDeviceId });
|
|
||||||
}
|
|
||||||
```
|
|
||||||
|
|
||||||
### Problems
|
|
||||||
1. **Split Responsibility** - Generation in Svelte, persistence in Rust
|
|
||||||
2. **Multiple Generation Points** - Could generate different IDs on different app starts if Rust storage fails
|
|
||||||
3. **Simple Logic** - UUID generation should be one place
|
|
||||||
|
|
||||||
### Recommendation
|
|
||||||
Move device ID to Rust:
|
|
||||||
- Change `device_get_id` to return existing ID or generate+store new one atomically
|
|
||||||
- Svelte just calls `await invoke("device_get_id")` once
|
|
||||||
|
|
||||||
```rust
|
|
||||||
// In Rust
|
|
||||||
#[tauri::command]
|
|
||||||
async fn device_get_id(storage: State<'_, Storage>) -> Result<String> {
|
|
||||||
if let Some(id) = storage.get_device_id()? {
|
|
||||||
return Ok(id);
|
|
||||||
}
|
|
||||||
|
|
||||||
// Generate and store atomically
|
|
||||||
let id = uuid::Uuid::new_v4().to_string();
|
|
||||||
storage.set_device_id(&id)?;
|
|
||||||
Ok(id)
|
|
||||||
}
|
|
||||||
```
|
|
||||||
|
|
||||||
### Impact
|
|
||||||
- **Effort:** Low (simple change)
|
|
||||||
- **Benefits:** Single responsibility, atomic operation
|
|
||||||
- **Risk:** Very low
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 5. 🟢 LOW PRIORITY: Server Reachability Reload Logic
|
|
||||||
|
|
||||||
**Location:** [src/lib/composables/useServerReachabilityReload.ts](src/lib/composables/useServerReachabilityReload.ts)
|
|
||||||
|
|
||||||
### Issue
|
|
||||||
Tracks when server becomes reachable to reload data:
|
|
||||||
|
|
||||||
```typescript
|
|
||||||
// Lines 44-52: checkServerReachability()
|
|
||||||
if (isServerReachable && !previousServerReachable && hasLoadedOnce) {
|
|
||||||
reloadFn();
|
|
||||||
}
|
|
||||||
```
|
|
||||||
|
|
||||||
### Current Status
|
|
||||||
✅ This is actually fine to stay in Svelte because:
|
|
||||||
- It's UI-specific (reload on screen becomes visible)
|
|
||||||
- Simple stateless logic
|
|
||||||
- Works well as a composable
|
|
||||||
|
|
||||||
### Note
|
|
||||||
The backend's `connectivity:reconnected` event (listened to in [src/lib/stores/connectivity.ts:64](src/lib/stores/connectivity.ts#L64)) is the right abstraction level.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 6. 🟢 LOW PRIORITY: Input Validation & Type Conversions
|
|
||||||
|
|
||||||
**Location:**
|
|
||||||
- [src/lib/utils/validation.ts](src/lib/utils/validation.ts)
|
|
||||||
- [src/lib/utils/jellyfinFieldMapping.ts](src/lib/utils/jellyfinFieldMapping.ts)
|
|
||||||
- [src/lib/api/conversions.ts](src/lib/api/conversions.ts)
|
|
||||||
|
|
||||||
### Current Status
|
|
||||||
✅ These are fine to stay in Svelte because:
|
|
||||||
- UI validation (email format, username length)
|
|
||||||
- Display formatting (duration, field mapping)
|
|
||||||
- Jellyfin API type conversions for UI display
|
|
||||||
- Svelte-only concerns (component state)
|
|
||||||
|
|
||||||
### Keep As-Is
|
|
||||||
No action needed. These are thin utility layers for UI concerns.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Summary Table
|
|
||||||
|
|
||||||
| Component | Current | Should Move? | Priority | Effort | Risk |
|
|
||||||
|-----------|---------|--------------|----------|--------|------|
|
|
||||||
| **syncService.ts** | Svelte | → Rust | 🔴 High | Medium | Low |
|
|
||||||
| **playbackReporting.ts** | Svelte | → Rust | 🔴 High | Medium | Low |
|
|
||||||
| **playerEvents.ts** (state logic) | Svelte | Consider | 🟡 Medium | Medium | Medium |
|
|
||||||
| **deviceId.ts** | Svelte | → Rust | 🟡 Medium | Low | Very Low |
|
|
||||||
| **useServerReachabilityReload.ts** | Svelte | Keep ✅ | - | - | - |
|
|
||||||
| **validation.ts** | Svelte | Keep ✅ | - | - | - |
|
|
||||||
| **API conversions** | Svelte | Keep ✅ | - | - | - |
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Recommended Implementation Order
|
|
||||||
|
|
||||||
1. **Phase 1 (High Impact, Low Risk)**
|
|
||||||
- Move device ID generation to Rust (1-2 hours)
|
|
||||||
- This is small but improves robustness
|
|
||||||
|
|
||||||
2. **Phase 2 (High Impact, Medium Effort)**
|
|
||||||
- Move sync queue processing to Rust (4-6 hours)
|
|
||||||
- Move playback reporting logic to Rust (4-6 hours)
|
|
||||||
- These are related and can be done together
|
|
||||||
|
|
||||||
3. **Phase 3 (Optional, More Complex)**
|
|
||||||
- Consider state machine refactoring in playerEvents.ts
|
|
||||||
- Only if experiencing issues or during major refactor
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Architecture Principles to Maintain
|
|
||||||
|
|
||||||
When implementing these changes, preserve:
|
|
||||||
|
|
||||||
1. **Command-Based API** - Svelte invokes Rust commands, doesn't call internal functions
|
|
||||||
2. **Event-Driven Updates** - Rust emits events for state changes, Svelte listens
|
|
||||||
3. **Thin Frontend** - Svelte only handles UI rendering and user input
|
|
||||||
4. **Type Safety** - Use Rust enums and structs for critical logic
|
|
||||||
5. **Offline-First** - Backend decides online vs offline paths, not frontend
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Questions & Notes
|
|
||||||
|
|
||||||
- **Q: Should `repository.ts` API calls move to Rust?**
|
|
||||||
- A: No - `RepositoryClient` is a good abstraction layer. Keep as-is.
|
|
||||||
|
|
||||||
- **Q: Should UI state like `showSleepTimerModal` be in Rust?**
|
|
||||||
- A: No - UI state belongs in Svelte stores. This is correct.
|
|
||||||
|
|
||||||
- **Q: Should we move all HTTP calls to Rust?**
|
|
||||||
- A: The Jellyfin HTTP client is already in Rust. `RepositoryClient` wraps it, which is fine.
|
|
||||||
|
|
||||||
- **Q: What about preload logic?**
|
|
||||||
- A: `preload.ts` is fine - it's a simple command wrapper + orchestration.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Files with Findings
|
|
||||||
|
|
||||||
- [playerEvents.ts](src/lib/services/playerEvents.ts) - State logic, event filtering
|
|
||||||
- [playbackReporting.ts](src/lib/services/playbackReporting.ts) - Offline logic duplication
|
|
||||||
- [syncService.ts](src/lib/services/syncService.ts) - Sync queue processing
|
|
||||||
- [deviceId.ts](src/lib/services/deviceId.ts) - Split responsibility
|
|
||||||
- [useServerReachabilityReload.ts](src/lib/composables/useServerReachabilityReload.ts) - Fine as-is
|
|
||||||
- [appState.ts](src/lib/stores/appState.ts) - Fine as-is (UI state)
|
|
||||||
|
|
||||||
@ -12,7 +12,7 @@ services:
|
|||||||
- .:/app
|
- .:/app
|
||||||
environment:
|
environment:
|
||||||
- RUST_BACKTRACE=1
|
- RUST_BACKTRACE=1
|
||||||
command: bash -c "bun test && cd src-tauri && cargo test && cd .. && echo 'All tests passed!'"
|
command: bash -c "bun run test && cd src-tauri && cargo test && cd .. && echo 'All tests passed!'"
|
||||||
|
|
||||||
# Android build service - builds APK after tests pass
|
# Android build service - builds APK after tests pass
|
||||||
android-build:
|
android-build:
|
||||||
|
|||||||
@ -7,7 +7,7 @@ echo "🧪 Running all tests..."
|
|||||||
echo ""
|
echo ""
|
||||||
|
|
||||||
echo "📦 Running frontend tests..."
|
echo "📦 Running frontend tests..."
|
||||||
bun test
|
bun run test
|
||||||
|
|
||||||
echo ""
|
echo ""
|
||||||
echo "🦀 Running Rust tests..."
|
echo "🦀 Running Rust tests..."
|
||||||
|
|||||||
@ -4,4 +4,4 @@
|
|||||||
set -e
|
set -e
|
||||||
|
|
||||||
echo "📦 Running frontend tests..."
|
echo "📦 Running frontend tests..."
|
||||||
bun test "$@"
|
bun run test "$@"
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user