m5stack-dashboard/CODE_REVIEW_PR1.md

# Code Review – PR 1: "Feature/improve UI"

**Datum:** 2026-08-02  
**Review-State:** OPEN (nicht gemerged)  
**Commits im PR:** 2 ("improve ui", "update build scripts")

---

## 1. ✅ Was gut ist

- **Neues `include/date_utils.h`**: Saubere Extrahierung der Datumsformatierung in einen wiederverwendbaren Helper (`DateUtils::formatShortDE`). Sakamoto-Algorithmus für Wochentag-Berechnung ohne NTP ist korrekt implementiert. Eliminiert die vorherige Code-Duplizierung von `formatEventTime` in `home_screen.cpp` und `calendar_detail_screen.cpp`.
- **Gewitter-Icon**: Neue `drawThunderstorm` / `drawLightningBolt` Funktion mit farbenfroher Akzentfarbe (`ACCENT_STORM`). Unterscheidet sich optisch klar von einfacher Bewölkung. Überload `drawWeatherIcon(symbol, description, ...)` mit Rückwärtskompatibilität via Default-Overload ist sauber gelöst.
- **Verbessertes Kalender-Icon**: Ersetzt das vorherige checkbox-ähnliche Symbol durch ein echtes Kalenderblatt-Icon (Header-Balken, Heftklammern, Tagespunkt). Deutlicher UI-Qualitätsgewinn.
- **CI-Workflow-Fix**: `.github/workflows/build.yml` generiert automatisch eine Wegwerf-`secrets.h` aus `secrets.h.example` für Builds. Bricht kein Build mehr in CI, weil `secrets.h` per `.gitignore` ausgeschlossen ist. Kommentar erklärt die Motivation gut.
- **UI-Feinabstimmung**: Kalender-Detail und MVG-Screen erhalten mehr Zeilenabstand (`rowH` Konstante), Trennlinien und farbige Datumsangaben. Visuelle Hierarchie ist klarer.
- **`drawCloud` color-Parameter**: Neuer Default-Parameter (`Theme::TEXT_DIM`) ermöglicht farbige Wolken für das Gewitter-Icon, ohne die bestehenden Aufrufer zu brechen.
- **`.gitignore` Cleanup**: Eintrag für `.venv-platformio/` neu organisiert, Kommentar hinzugefügt.
- **Manuelle Retries**: Durch `forceRefresh=true` bei Button-Druck (via `goToScreen`) wird automatisch ein Retry ausgelöst – entspricht SPEC.

## 2. ⚠️ Was verbessert werden sollte

- **Kommentar-Entfernung zu radikal**: Viele wertvolle Design- und Dokumentationskommentare wurden gelöscht (z.B. in `http_client.h`, `weather_api.h`, `screen_base.h`, `home_screen.h`). Diese Kommentare erklären *warum* der Code so funktioniert, nicht nur *was* er tut. Die SPEC.md bleibt zwar als Referenz, aber inline-Dokumentation auf Ebene der Header wäre wertvoll. Insbesondere die Erklärung des Retry-Mechanismus in `http_client.h` fehlt jetzt.
  - **Dateien betroffen:** `src/api/http_client.h`, `src/api/weather_api.h`, `src/api/calendar_api.h`, `src/api/departures_api.h`, `src/screens/screen_base.h`, `src/screens/home_screen.h`
  - **Fix:** Kurze Doc-Strings / Design-Kommentare wiederherstellen oder als `/** ... */` Dokumentation ergänzen.

- **`wifiConnected` Flag nie verwendet** (`src/main.cpp:16`): Das Flag wird in `connectWifi()` gesetzt, aber nirgends gelesen. Wenn WiFi ausfällt, gibt es keine Reconnect-Logik. In `loop()` wird der Verbindungsstatus nicht geprüft, bevor API- Calls gemacht werden – das wird durch den `WiFi.status()`-Check in `httpGet` abgefangen, aber ein dedizierter Reconnect-Versuch oder eine Fehlermeldung wäre robuster.
  - **Fix:** `wifiConnected` entweder nutzen (z.B. für UI-Statusanzeige) oder entfernen. Optional: Hintergrund-Reconnect in `loop()` implementieren.

- **Keine JSON-Key-Validierung**: In `weather_api.cpp`, `calendar_api.cpp`, `departures_api.cpp` werden `doc["events"]`, `doc["forecast"]`, `doc["departures"]` direkt iteriert, ohne zu prüfen, ob der Schlüssel existiert oder ob `doc` erfolgreich deserialisiert wurde (nur auf `err` geprüft, aber nicht auf `doc.containsKey(...)`). Bei einer unerwarteten API-Antwort (z.B. Fehler-JSON statt Daten-JSON) würde dies zu einem leeren Array oder undefiniertem Verhalten führen.
  - **Dateien betroffen:** `src/api/weather_api.cpp`, `src/api/calendar_api.cpp`, `src/api/departures_api.cpp`
  - **Fix:** Vor Iteration `if (!doc.containsKey("events")) { data.valid = false; return data; }` prüfen.

- **Fehler-Anzeige-Duplizierung**: Der Retry-Icon + "Keine Verbindung"-Block ist identisch in `weather_detail_screen.cpp`, `calendar_detail_screen.cpp` und `mvg_screen.cpp` (3× dupliziert).
  - **Dateien betroffen:** `src/screens/weather_detail_screen.cpp:26-31`, `src/screens/calendar_detail_screen.cpp:39-44`, `src/screens/mvg_screen.cpp:39-44`
  - **Fix:** Gemeinsame Hilfsfunktion `drawErrorState(int x, int y, const char* label)` in eine shared Header-Datei (z.B. `src/screens/screen_utils.h`) extrahieren. `home_screen.cpp` hat bereits eine lokale `drawErrorState` – diese sollte nach oben hinausgehoben werden.

- **Heißer Crash bei `drawWeatherIcon` mit leerem Symbol**: `drawWeatherIcon` ruft `symbol.charAt(0)` und `symbol.charAt(1)` auf. Obwoohl `isNight` und `condition` mit Length-Checks abgesichert sind, wird das `default`-Case im Switch bei unbekannten Symbolen auf `drawCloud` geleitet – das ist eine implizite Annahme, die nicht dokumentiert ist.
  - **Datei betroffen:** `src/icons/icons.h:53` (PR-Version)

- **`mvg_screen.cpp` Zeilenbegrenzung hardcoded**: `y > 212` Hardcoded-Wert ist nicht mit `Theme::SCREEN_H` verknüpft. Bei Anzeigegrößen-Wechsel müsste dies manuell angepasst werden.
  - **Datei betroffen:** `src/screens/mvg_screen.cpp` (PR-Version)

## 3. 🛑 Was blockierend ist (muss gefixt werden)

- **Kein Test-Framework vorhanden**: Es gibt keinerlei Unit-Tests, keine Mock-Infrastruktur, keine Assertions. Insbesondere für `date_utils.h` (kritische Datumsberechnung) und `text_utils.h` (UTF-8-Transliteration) wären automatisierte Tests wertvoll. Ohne Tests ist nicht nachweisbar, dass z.B. der Sakamoto-Algorithmus korrekt funktioniert oder dass `sanitizeGermanText` alle Umlautfälle abdeckt.
  - **Betroffene Dateien:** `include/date_utils.h` (neu), `include/text_utils.h`
  - **Blockade:** Unit-Tests für `DateUtils::formatShortDE` und `DateUtils::computeWeekdayMonBased` hinzufügen (inkl. bekannte Test-Datumswerte). Test-Framework wählen (z.B. ArduinoUnity via PlatformIO, oder host-seitig mit Catch2 + Mock für `String`).

- **`http_client.cpp` – `http.begin()` Return-Wert ignoriert**: `http.begin(url)` gibt `bool` zurück (true = success). Der Rückgabewert wird ignoriert. Bei einer ungültigen URL oder fehlschlagender Verbindungsinitialisierung würde `http.GET()` dann undefiniertes Verhalten zeigen.
  - **Datei betroffen:** `src/api/http_client.cpp:16`
  - **Fix:** `if (!http.begin(url)) { result.success = false; http.end(); return result; }` hinzufügen.

- **`http_client.cpp` – Stack-Größe bei `DynamicJsonDocument`**: 8–16 KB `DynamicJsonDocument` auf dem Stack plus die gesamte HTTP-Antwort im `String body` → potenzieller Stack-Overflow bei großen API-Antworten. Der `String` wächst dynamisch im Heap, aber `res.body` nach `http.getString()` ist bereits vollständig im RAM geladen. Bei sehr großen Antworten (z.B. 10 Kalendertermine mit langen Description-Feldern) könnte der verfügbare RAM auf dem ESP32 knapp werden.
  - **Datei betroffen:** `src/api/weather_api.cpp:15`, `src/api/calendar_api.cpp:15`, `src/api/departures_api.cpp:15`
  - **Nicht blockierend**, da 8–16 KB üblich sind, aber `serializeJson`-Stream-Ansatz in einem Follow-up-PR erwägen.

---

## Checkliste für Follow-up

1. Unit-Tests schreiben für `date_utils.h` und `text_utils.h`
2. `http.begin()` Return-Wert in `http_client.cpp` prüfen
3. JSON-Key-Validierung in allen drei API-Clients hinzufügen
4. Duplizierte Error-Anzeige in eine shared Utility-Funktion extrahieren
5. Entfernte Design-Kommentare in Header-Dateien als Doc-Strings wiederherstellen
6. `wifiConnected` Flag entweder nutzen oder entfernen
7. Hardcoded `y > 212` in `mvg_screen.cpp` durch `Theme::SCREEN_H`-Ableitung ersetzen
8. Optional: Reconnection-Logik für WiFi-Ausfall in `loop()` implementieren