jellytau/SVELTE_CODE_REVIEW.md

# 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)