Files
melo-app/CODE_REVIEW.md
T

533 lines
18 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# 🔴 Melo App — Vollständiges Code-Review
> **Geprüft:** 20.07.2026 | **Projekt:** ~/Projects/melo_app/ | **Dustin (Baka) — Kiel**
> **Prinzip:** Ponytail — minimaler Code, maximale Wirkung, max 500 Zeilen pro Datei
---
## 📋 Kritische Bugs (MÜSSEN SOFORT GEFIXT WERDEN)
### 🔴 [CRIT-1] Memory Leak: MiniPlayer-Streams werden nie gecancelled
**Datei:** `widgets/mini_player.dart` · Zeilen 2032
```dart
// initState abonniert Streams, aber speichert kein StreamSubscription
_player.positionStream.listen((pos) { ... });
_player.stateStream.listen((state) { ... });
```
**Problem:** `initState` abonniert `positionStream` und `stateStream` via `.listen()`, aber es gibt **kein** `dispose()`-Override. Die `StreamSubscription`-Objekte werden nirgends gespeichert, also können sie nie gecancelled werden. Jedes Mal wenn der MiniPlayer neu gebaut wird (z.B. bei setState im Parent), leakt ein neues Paar Subscriptions.
**Fix:**
```dart
StreamSubscription? _posSub;
StreamSubscription? _stateSub;
@override
void initState() {
super.initState();
_posSub = _player.positionStream.listen((pos) { ... });
_stateSub = _player.stateStream.listen((state) { ... });
}
@override
void dispose() {
_posSub?.cancel();
_stateSub?.cancel();
super.dispose();
}
```
---
### 🔴 [CRIT-2] Null-Crash: `widget.song.id!` kann explodieren
**Datei:** `widgets/metadaten_dialog.dart` · Zeile 88
```dart
await _db.metadatenAktualisieren(
widget.song.id!, // ← CRASH wenn id null ist!
...
);
```
**Problem:** `Song.id` ist `int?`. Wenn ein Song (z.B. frisch gescannter oder Beispielsong ohne DB-ID) den Metadaten-Editor öffnet, crasht die App mit `null check used on null value`.
**Fix:**
```dart
final id = widget.song.id;
if (id == null) {
if (mounted) ScaffoldMessenger.of(context).showSnackBar(
const SnackBar(content: Text('Song-ID fehlt — bitte App neustarten')),
);
return;
}
await _db.metadatenAktualisieren(id, ...);
```
---
### 🔴 [CRIT-3] `dauerSekunden` ist IMMER 0 (kein Metadaten-Parsing)
**Datei:** `services/musik_scanner.dart` · Zeilen 3444
```dart
final song = Song(
titel: _dateiNameOhneEndung(pfad),
kuenstler: 'Unbekannt', // ← keinerlei Tag-Parsing
dauerSekunden: 0, // ← IMMER 0!
...
);
```
**Problem:** Es wird nie die echte Audio-Dauer ausgelesen. Weder ID3-Tags noch MediaStore-Metadaten. Alle gescannten Songs haben `dauerSekunden: 0``dauerFormatiert` zeigt `"0:00"`.
**Fix:** Nutze eine Audio-Metadaten-Bibliothek wie `flutter_media_metadata` oder `audio_metadata_reader`, oder implementiere einen nativen Method-Channel für MediaStore-Query. Minimal-Fix: verwende `just_audio` kurz zum Öffnen der Datei um Duration zu lesen.
---
### 🔴 [CRIT-4] `positionAktualisieren` flutet DB mit immer neuen Einträgen
**Datei:** `database/db_helper.dart` · Zeilen 126135
```dart
Future<void> positionAktualisieren(int songId, int position) async {
final d = await db;
await d.update('songs', {'zuletzt_position': position},
where: 'id = ?', whereArgs: [songId]);
await d.insert('wiedergabe_verlauf', { // ← IMMER INSERT!
'song_id': songId,
'position': position,
'zuletzt_abgespielt': DateTime.now().toIso8601String(),
});
}
```
**Problem:** Bei jedem Positions-Update wird ein NEUER History-Eintrag inserted, ohne den alten zu löschen. Nach 30 Minuten Musik hören mit 1-Sekunden-Takt → 1800 Einträge pro Song.
**Fix Variante A (einfach):** Lösche alten Eintrag vor neuem Insert:
```dart
await d.delete('wiedergabe_verlauf', where: 'song_id = ?', whereArgs: [songId]);
await d.insert('wiedergabe_verlauf', { ... });
```
**Fix Variante B (besser):** Nur bei signifikanten Änderungen speichern (alle 30 Sekunden statt bei jedem Tick).
---
### 🔴 [CRIT-5] `FlutterErrorBoundary` tut GAR NICHTS
**Datei:** `main.dart` · Zeilen 4862
```dart
class _FlutterErrorBoundaryState extends State<FlutterErrorBoundary> {
@override
Widget build(BuildContext context) {
return widget.child; // ← gibt einfach child zurück, kein Error Handling!
}
}
```
**Problem:** Das "Error Boundary" hat keinen Error-Catcher (`FlutterError.onError` oder `ErrorWidget.builder` oder `runZonedGuarded`). Es fängt genau null Fehler. Wird aber zum Glück auch nicht verwendet (der `AppWrapper` wird nie benutzt).
**Fix:** Entweder korrekt implementieren (mit `runZonedGuarded` oder `ErrorWidget.builder`) oder — besser — den ganzen `AppWrapper` + `FlutterErrorBoundary` rauswerfen (Ponytail!).
---
## 📋 Schwere Probleme
### 🟠 [MAJ-1] Download-Fehlermeldungen werden brutal abgeschnitten
**Datei:** `services/download_service.dart` · Zeilen 56, 75, 105, 127
```dart
_fehlermeldung = 'Timeout/Fehler bei Video-Info: ${e.toString().substring(0, 80)}';
```
**Problem:** `.substring(0, 80)` und `.substring(0, 100)` zerstören Fehlerdetails. YouTube-Fehler (Rate Limits, Region-Locks, Copyright Claims) werden sinnlos.
**Fix:** Entweder gar nicht truncaten, oder erst auf UI-Ebene truncaten. Die Fehlermeldung sollte vollständig geloggt werden:
```dart
debugPrint('Download-Fehler (vollständig): $e');
_fehlermeldung = e.toString(); // UI zeigt nur ersten Teil, Log hat alles
```
---
### 🟠 [MAJ-2] MiniPlayer `dispose()` tut nichts (lebt als Memory Leak II)
**Datei:** `widgets/mini_player.dart` · Zeile 131
Kein `dispose()`-Override vorhanden! Die Klasse endet mit `Widget _playBtn() { ... }`. Dadurch werden nicht nur Streams nicht gecancelled (CRIT-1), sondern auch keine Ressourcen freigegeben.
---
### 🟠 [MAJ-3] `_scanneViaMediaStore()` gibt IMMER `[]` zurück
**Datei:** `services/musik_scanner.dart` · Zeilen 121126
```dart
Future<List<String>> _scanneViaMediaStore() async {
// Nutzt Android's MediaStore Query
// Wird über Method Channel in native Android implementiert
// Für v1: Fallback auf Dateisystem-Suche
return []; // ← TODO seit Version 1
}
```
**Problem:** Der MediaStore-Pfad ist nie implementiert. Der Fallback auf Dateisystem-Suche ist ineffizient und findet keine Musik in App-spezifischen Verzeichnissen. Auf Android 11+ (API 30+) wird der Dateisystem-Zugriff zudem stark eingeschränkt.
---
### 🟠 [MAJ-4] `_p.positionStream` referenziert möglicherweise alten Player
**Datei:** `services/player_service.dart` · Zeilen 3233
```dart
Stream<Duration> get positionStream => _p.positionStream; // _p initiiert lazy
Stream<PlayerState> get stateStream => _p.playerStateStream;
```
**Problem:** `_p` initialisiert den `AudioPlayer` lazy beim ersten Zugriff. Wenn die Streams vor dem ersten `spiele()`-Aufruf abonniert werden, hängen sie an einem Player, der später ersetzt/reinitialisiert werden könnte. Der getter erzeugt keinen neuen Player, also ist das nur ein Problem wenn `_player` jemals auf null gesetzt wird (was nie passiert). Aber: `_p` wird von diesen Gettern aufgerufen → bei jedem Stream-Zugriff wird der `_player != null` Check gemacht. Das ist ok, aber es ist ein wartungsintensives Pattern.
---
### 🟠 [MAJ-5] `AppWrapper` ist totes Code-Gewebe
**Datei:** `main.dart` · Zeilen 3446
`AppWrapper` wird nirgends verwendet. `MeloApp` geht direkt zu `Scaffold(body: MeloHome())`. Der Wrapper wurde offenbar geplant aber nie aktiviert.
---
### 🟠 [MAJ-6] Tag-Filter wird in Song-Liste komplett ignoriert
**Datei:** `screens/home_screen.dart` · Zeilen 374393 + 431434
`_aktiverTag` wird zwar gesetzt (Zeile 382), aber in `_songListe()` (Zeile 431434) wird die Song-Liste ungefiltert angezeigt:
```dart
itemCount: _songs.length, // ← immer alle Songs, egal welcher Tag aktiv
```
Die `_beispielTags` sind zudem hardcoded (nicht aus der DB) — das Tag-System hat keinen echten Filter.
---
## 📋 Code-Qualität & Anti-Patterns
### 🟡 [QUAL-1] Singleton-Overkill (5 von 6 Klassen sind Singletons)
| Klasse | Singleton? | Warum problematisch |
|--------|-----------|---------------------|
| `DbHelper` | ✅ | Testen unmöglich (kein Mock) |
| `PlayerService` | ✅ | State hält zwischen Tests |
| `FavoritenService` | ✅ | s.o. |
| `DownloadService` | ✅ | s.o. |
| `MusikScanner` | ✅ | s.o. |
**Fix:** Dependency Injection via Konstruktor. Services sollten das DB-Objekt injiziert bekommen statt `DbHelper()` direkt aufzurufen.
---
### 🟡 [QUAL-2] HomeScreen Monolith — 519 Zeilen in einer Datei
**Datei:** `screens/home_screen.dart` · 519 Zeilen
Enthält:
- State Management
- Header-Widget
- Statistik-Widget
- Tag-Leiste
- Song-Liste
- Song-Tile
- Bottom Nav
- Download-Dialog (komplette UI + Timer-Logik!)
- Such-Dialog (komplette UI!)
- Scan-Logik
- Beispieldaten/Seed-Daten
**Ponytail-Prinzip verletzt!** Max 500 Zeilen pro Datei fast erreicht, aber die Verantwortlichkeiten sind vermischt.
**Fix:**
- Extrahiere `MeloHeader`, `StatistikCard`, `TagLeiste`, `SongTile` in eigene Widget-Dateien (`widgets/`)
- Extrahiere Such- und Download-Dialoge in eigene Methoden oder Dateien
- Entferne Beispiel-Song-Seeding (gehört in einen dev-only Seeder)
---
### 🟡 [QUAL-3] `loeschen()` löscht in falscher Reihenfolge
**Datei:** `database/db_helper.dart` · Zeilen 221229
```dart
await d.delete('wiedergabe_verlauf');
await d.delete('song_tags');
await d.delete('playlist_songs');
await d.delete('playlists');
await d.delete('tags');
await d.delete('songs');
```
**Problem:** Die Tabellen mit `ON DELETE CASCADE` werden vor den Eltern-Tabellen gelöscht. Theoretisch korrekt (CASCADE ist auf FK definiert), aber die Reihenfolge ist inkonsistent: `playlists` wird vor `tags` gelöscht, obwohl beide Eltern sind. Außerdem: **keine Transaktion!** Wenn ein Löschen fehlschlägt, hat man eine korrupte Datenbank.
**Fix:** Alles in eine Transaktion packen:
```dart
await d.transaction((txn) async {
await txn.delete('wiedergabe_verlauf');
await txn.delete('song_tags');
await txn.delete('playlist_songs');
await txn.delete('playlists');
await txn.delete('tags');
await txn.delete('songs');
});
```
---
### 🟡 [QUAL-4] Doppelter try-catch in `musik_scanner.dart`
**Datei:** `services/musik_scanner.dart` · Zeilen 2965
Zwei verschachtelte try-catch Blöcke die fast identischen Code enthalten. Der äußere try-catch fängt alles, und der innere ist ein "vielleicht klappt es beim zweiten Versuch" — ohne ersichtlichen Grund.
**Fix:** Einfach einen try-catch:
```dart
try {
final file = File(pfad);
final stat = await file.stat();
gefunden.add(/* Song erstellen */);
} catch (e) {
debugPrint('Datei nicht lesbar: $pfad$e');
}
```
---
### 🟡 [QUAL-5] `favoritenIds()` ist ineffizient (lädt alle Songs nur für IDs)
**Datei:** `services/favoriten_service.dart` · Zeilen 4549
```dart
Future<Set<int>> favoritenIds() async {
final songs = await _db.songsDerPlaylist(_favoritenPlaylistId!);
return songs.where((s) => s.id != null).map((s) => s.id!).toSet();
}
```
**Problem:** Lädt komplette Song-Objekte (mit allen Feldern) aus der DB, nur um die IDs zu bekommen. Bei 1000 Songs werden 1000 `Song.fromMap()`-Aufrufe gemacht.
**Fix:** Dedizierte DB-Query:
```dart
Future<Set<int>> favoritenIds() async {
final d = await _db.db;
final rows = await d.rawQuery(
'SELECT song_id FROM playlist_songs WHERE playlist_id = ?',
[_favoritenPlaylistId],
);
return rows.map((r) => r['song_id'] as int).toSet();
}
```
---
## 📋 Überschüssiger Code (Ponytail-Prinzip)
### 🧹 [PONY-1] 4 redundante Zeilen im `_p` Getter
**Datei:** `services/player_service.dart` · Zeilen 1625
```dart
AudioPlayer get _p {
if (_player == null) {
try {
_player = AudioPlayer();
} catch (e) {
debugPrint('AudioPlayer Init Fehler: $e');
_player = AudioPlayer(); // ← gleicher Code nochmal??
}
}
return _player!;
}
```
Der doppelte `AudioPlayer()`-Aufruf im catch-Block ist sinnlos. Wenn der erste fehlschlägt, schlägt der zweite auch fehl. Einfach:
```dart
AudioPlayer get _p => _player ??= AudioPlayer();
```
---
### 🧹 [PONY-2] Beispiel-Songs + Tags aus home_screen.dart raus
**Datei:** `screens/home_screen.dart` · Zeilen 3240, 5267
Die `_beispielTags` (hardcoded) und die Beispiel-Song-Seeding (Zeilen 5367) gehören nicht in den produktiven Screen. Das ist Dev-Code.
**Fix:** Entweder in einen `DevDataSeeder()` auslagern oder per `--dart-define` steuern.
---
### 🧹 [PONY-3] Download-Dialog als StatefulBuilder im HomeScreen
**Datei:** `screens/home_screen.dart` · Zeilen 183245
Der komplette Download-Dialog mit Timer-Polling ist im HomeScreen vergraben. Das sind ~60 Zeilen UI + Logik.
**Fix:** Eigene Widget-Datei `widgets/download_dialog.dart` mit `state`-haltendem Widget, das den Timer managed.
---
### 🧹 [PONY-4] `_header()` + `_statistik()` + `_tagLeiste()` → eigene Widgets
**Datei:** `screens/home_screen.dart` · Zeilen 280393
Diese drei Methoden sind alle UI-only und könnten als eigene Widgets in `widgets/` leben.
---
## 📋 Security & Robustheit
### 🔒 [SEC-1] `Permission.storage` ist auf Android 33+ deprecated
**Datei:** `services/musik_scanner.dart` · Zeile 19
```dart
final status = await Permission.storage.request();
```
**Problem:** Ab API 33 (Android 13) gibt es `READ_MEDIA_AUDIO` statt `READ_EXTERNAL_STORAGE`. `Permission.storage` fragt nach der alten Permission, die auf neueren Geräten ignoriert wird.
**Fix:**
```dart
import 'dart:io' show Platform;
// Oder besser über permission_handler Android-specific
await Permission.audio.request();
```
---
### 🔒 [SEC-2] Kein Retry-Mechanismus bei Netzwerkfehlern
**Datei:** `services/download_service.dart` · Zeilen 32131
Bei Timeout oder Verbindungsabbruch wird einfach `null` zurückgegeben. Kein Retry, kein exponentielles Backoff.
**Fix:** 2-3 Retry-Versuche mit steigendem Timeout:
```dart
for (int versuch = 0; versuch < 3; versuch++) {
try {
return await _downloadAttempt(url);
} catch (e) {
if (versuch == 2) rethrow;
await Future.delayed(Duration(seconds: 2 * (versuch + 1)));
}
}
```
---
### 🔒 [SEC-3] Kein Cancel-Mechanismus beim YouTube-Download
**Datei:** `services/download_service.dart` · Zeilen 90108
Einmal gestartet, kann der Download nicht abgebrochen werden. Der Nutzer muss warten oder die App killen.
**Fix:** `StreamSubscription` speichern und `cancel()` anbieten:
```dart
StreamSubscription? _downloadSub;
void cancelDownload() => _downloadSub?.cancel();
```
---
## 📋 Missing Features
### 📌 [FEAT-1] **4 von 5 Bottom-Nav-Tabs sind leer**
`home_screen.dart` Zeilen 509515: Navigation hat 5 Einträge, aber keine `_selectedIndex` und keine `IndexedStack` oder `switch`-Logik. Alle Tabs außer "Musik" zeigen denselben Screen.
**Zu implementieren:**
- `Downloads` → Zeige heruntergeladene Songs + Download-Buttons
- `Tags` → Tag-Verwaltung (erstellen, löschen, Songs zuweisen)
- `Favoriten` → Zeige Favoriten-Playlist
- `Einstellungen` → Theme, Cache, Info
---
### 📌 [FEAT-2] **Keine Audio-Service Integration**
`audio_service: ^0.18.15` ist als Dependency eingetragen, aber wird nirgends importiert oder verwendet. `PlayerService` ist standalone ohne Background-Playback.
---
### 📌 [FEAT-3] **`setWarteschlange` ohne Automatische Wiedergabe**
`player_service.dart` Zeilen 8387: Die Warteschlange wird gesetzt, aber nach dem letzten Song stoppt die Wiedergabe (kein Loop, kein Shuffle, keine Queue-Weiterverarbeitung).
---
## 📋 Zusammenfassung: Priority-TODO-Liste
### 🔴 MUST FIX (sofort — vor Release)
| # | Datei | Zeile | Issue |
|---|-------|-------|-------|
| 1 | `widgets/mini_player.dart` | 20-32 | **Memory Leak:** Streams nie gecancelled |
| 2 | `widgets/metadaten_dialog.dart` | 88 | **Null-Crash:** `song.id!` kann crashen |
| 3 | `services/musik_scanner.dart` | 38 | **`dauerSekunden: 0`** — alle Songs haben Dauer 0 |
| 4 | `database/db_helper.dart` | 126-135 | **DB-Flut:** positionAktualisieren inserted immer neu |
| 5 | `main.dart` | 48-62 | **`FlutterErrorBoundary`** tut nichts (entfernen oder fixen) |
| 6 | `widgets/mini_player.dart` | 131 | **Kein `dispose()`** — Leak #2 |
### 🟠 SHOULD FIX (nächster Sprint)
| # | Datei | Zeile | Issue |
|---|-------|-------|-------|
| 7 | `services/download_service.dart` | 56,75,105,127 | Fehlermeldungen abgeschnitten (substring) |
| 8 | `services/musik_scanner.dart` | 121-126 | `_scanneViaMediaStore()` gibt `[]` zurück |
| 9 | `main.dart` | 34-46 | `AppWrapper` totes Gewebe (entfernen) |
| 10 | `screens/home_screen.dart` | 382 | Tag-Filter tut nichts |
| 11 | `screens/home_screen.dart` | 32-40 | Hardcoded Beispiel-Tags (aus DB laden!) |
| 12 | `services/download_service.dart` | 90-108 | Kein Cancel-Mechanismus |
| 13 | `services/download_service.dart` | — | Kein Retry bei Netzwerkfehlern |
| 14 | `services/musik_scanner.dart` | 19-20 | Permission.storage deprecated (API 33+) |
| 15 | `screens/home_screen.dart` | 509-515 | 4/5 Bottom-Nav-Tabs leer |
| 16 | `services/favoriten_service.dart` | 45-49 | `favoritenIds()` lädt unnötig alle Songs |
### 🟡 NICE TO IMPROVE (Code-Qualität)
| # | Datei | Zeile | Issue |
|---|-------|-------|-------|
| 17 | `screens/home_screen.dart` | 1-519 | **Monolith** — in Einzeldateien aufteilen |
| 18 | `services/player_service.dart` | 16-25 | Redundanter try-catch im `_p` Getter |
| 19 | `database/db_helper.dart` | 221-229 | `loeschen()` ohne Transaktion |
| 20 | `services/musik_scanner.dart` | 29-65 | Doppelter try-catch Block |
| 21 | — | — | **Singleton-Overkill** → Dependency Injection |
| 22 | `screens/home_screen.dart` | 183-245 | Download-Dialog im HomeScreen (auslagern) |
| 23 | `services/player_service.dart` | 83-87 | setWarteschlange ohne Weiterschaltung |
| 24 | — | — | `audio_service` nie verwendet (Background-Playback) |
---
## 📊 Statistik
| Metrik | Wert |
|--------|------|
| **Dateien** | 11 (12 mit pubspec.yaml) |
| **Gesamt-LOC (Dart)** | 1.613 |
| **Größte Datei** | `home_screen.dart` — 519 Zeilen (32%) |
| **Singleton-Klassen** | 5/6 (83%) |
| **Kritische Bugs** | **6** |
| **Schwere Probleme** | **6** |
| **Code-Qualität** | **5** |
| **Ponytail-Verletzungen** | **4** (→ extrahieren) |
| **Security** | **3** |
| **Fehlende Features** | **4** (davon 1 komplett leer) |
---
> **Fazit:** Die App hat ein solides Grundgerüst, aber leidet unter klassischen "Solo-Dev"-Problemen: Memory Leaks, Null-Safety-Lücken, Singleton-Overkill und ein Monolith-Screen. Der größte Hebel ist die Aufteilung von `home_screen.dart` (→ 3-4 eigene Widgets) und das Fixen der 6 kritischen Bugs. Danach: MediaStore-Integration für echte Song-Dauer und die 4 leeren Tabs befüllen.
> "Weniger Code, mehr Wirkung" — **Ponytail-Prinzip.** Der MiniPlayer könnte mit dispose-Fix und gekürztem Code locker 30 Zeilen verlieren. Der HomeScreen sollte bei ~250 Zeilen landen.