[FE] Révise la stabilité, réactivité et performance du frontend
- Corrige fuites mémoire SSE via nettoyage des listeners (P0) - Remplace shallowRef<Map> par reactive(new Map()) pour réactivité native Vue (P1) - Ajoute timeout/AbortController aux requêtes fetch dans l'API client (P4) - Déboucle les watch() de useTabs avec debounce unique et try/finally sur isRestoringFromStorage (P7) - Limite notifications à 5 + nettoyage des timers dans UI store (P15) - Protège routes debug avec import dynamique uniquement en DEV + wildcard 403 (P8) - Déplace SVG par défaut dans assets/default-cover.svg et import ?raw (P10) - Ajoute @media prefers-reduced-motion aux animations CSS globales - Factorise styles drawer-btns avec .drawer-icon-classe + ajoute --opacity-disabled (P13) - Encode les clés de cache browse avec encodeURIComponent + ':' séparateur (P14) - Implémente pagination infinite scroll dans browseContainer/loadMore + Supprime formatMsToShortTime alias (P9) - Corrige truncate() pour éviter dépassement maxLength si suffix >=maxLength (P10) - Valide structure des commandes PMOPlayer avant traitement + Met à jour version Cargo.toml et lock (0.3.39)
This commit is contained in:
327
Blackboard/Todo/Frontend_Review.md
Normal file
327
Blackboard/Todo/Frontend_Review.md
Normal file
@@ -0,0 +1,327 @@
|
||||
** Ce travail devra être réalisé en suivant scrupuleusement les consignes listées dans le fichier [@Rules_optimal.md](file:///Users/coissac/Sync/maison/Petite_maisons/src/pmomusic/Blackboard/Rules_optimal.md) **
|
||||
|
||||
## Contexte
|
||||
|
||||
Ce document est le résultat d'une revue de code complète du frontend Vue.js/TypeScript du Control Point
|
||||
(`pmoapp/webapp/src/`). L'application est fonctionnelle mais présente plusieurs classes de problèmes
|
||||
qui peuvent causer des fuites mémoire, des incohérences de réactivité Vue 3, et des difficultés de
|
||||
maintenance à mesure que l'app grandit.
|
||||
|
||||
**Périmètre** : uniquement `pmoapp/webapp/src/` (composants, services, composables, stores, utils, CSS).
|
||||
|
||||
---
|
||||
|
||||
## Problèmes identifiés
|
||||
|
||||
### P0 — Fuites mémoire via listeners SSE jamais nettoyés
|
||||
|
||||
**Fichiers** : `src/composables/useSSE.ts`, `src/composables/useRenderers.ts`,
|
||||
`src/composables/useMediaServers.ts`
|
||||
|
||||
Les abonnements aux événements SSE sont créés lors du premier appel de chaque composable, mais
|
||||
jamais nettoyés si le composable est réutilisé ou le composant détruit. Dans `useSSE.ts`, la
|
||||
fonction `onRendererEvent()` retourne une fonction de cleanup, mais `useRendererEvents()` ignore
|
||||
ce retour — l'abonnement reste actif indefiniment.
|
||||
|
||||
De plus, dans `imageCache.ts`, un `setInterval` de cleanup s'exécute toutes les 5 minutes sans
|
||||
jamais être annulé si l'app est détruite.
|
||||
|
||||
### P1 — Réactivité Vue incohérente avec `shallowRef` + Maps
|
||||
|
||||
**Fichier** : `src/composables/useRenderers.ts` (L23-37)
|
||||
|
||||
`snapshots` et `loadingIds` sont déclarés en `shallowRef<Map<...>>()`. Vue ne détecte pas les
|
||||
mutations d'objets à l'intérieur d'un `shallowRef`. La solution actuelle — `triggerSnapshotReactivity()`
|
||||
qui crée une nouvelle Map à chaque appel — force une re-render complète de tous les composants
|
||||
qui dépendent de `snapshots`, même si seul un renderer a changé.
|
||||
|
||||
### P2 — Race condition à l'initialisation (main.ts)
|
||||
|
||||
**Fichier** : `src/main.ts`
|
||||
|
||||
Le UIStore est initialisé après le montage de l'app et l'appel à `sse.connect()`. Des événements
|
||||
SSE peuvent arriver avant que `useUIStore()` soit appelé dans les composants, et les notifications
|
||||
correspondantes peuvent être perdues.
|
||||
|
||||
### P3 — SSE singleton sans garantie formelle
|
||||
|
||||
**Fichiers** : `src/composables/useRenderers.ts`, `src/composables/useMediaServers.ts`
|
||||
|
||||
Chaque composable maintient son propre flag `sseInitialized` pour éviter les double-abonnements.
|
||||
Le mécanisme repose sur une convention implicite fragile : si deux composables s'abonnent au même
|
||||
type d'événement SSE dans des contextes différents, les callbacks s'accumulent sans être
|
||||
dédupliqués.
|
||||
|
||||
Dans `useSSE.ts`, `setupConnectionListener()` vérifie `connectionCallbacks.size === 0` mais
|
||||
sans lock — deux appels simultanés peuvent installer deux listeners.
|
||||
|
||||
### P4 — Pas de timeout ni retry sur les requêtes `fetch`
|
||||
|
||||
**Fichier** : `src/services/pmocontrol/api.ts`
|
||||
|
||||
Toutes les requêtes `fetch()` sont émises sans `AbortController`. Si le serveur ne répond pas,
|
||||
la promesse pend indéfiniment, bloquant potentiellement les composants qui attendent le résultat.
|
||||
Il n'y a ni timeout configurable ni retry automatique au niveau du service.
|
||||
|
||||
### P5 — Validation absente des réponses API
|
||||
|
||||
**Fichiers** : `src/services/audioCache.ts` (L76), `src/services/coverCache.ts`,
|
||||
`src/services/playlists.ts`, `src/services/pmocontrol/api.ts`
|
||||
|
||||
Les réponses JSON sont acceptées sans vérification de structure. Une assertion de type comme
|
||||
`metadata as { origin_url?: unknown }` ne protège pas contre un changement d'API côté Rust. Si
|
||||
l'API retourne une structure inattendue, le crash survient au runtime, pas à la compilation.
|
||||
|
||||
### P6 — Type assertions dangereuses dans PMOPlayer
|
||||
|
||||
**Fichier** : `src/services/pmosource.ts` / PMOPlayer (L197-214)
|
||||
|
||||
Les messages de commande sont typés `Record<string, unknown>`, puis les propriétés sont castées
|
||||
directement : `msg.url as string`, `msg.timestamp as number`. Si une propriété est absente ou
|
||||
d'un type différent, TypeScript ne le détecte pas.
|
||||
|
||||
### P7 — `useTabs` : watch multiples sans debounce, flag de restauration non-réinitialisé
|
||||
|
||||
**Fichier** : `src/composables/useTabs.ts` (L44, L350-361)
|
||||
|
||||
Trois `watch()` séparées écrivent dans `localStorage`. Sans debounce commun, si 3 onglets
|
||||
changent d'état simultanément, `localStorage` est écrit 3 fois de suite.
|
||||
|
||||
Le flag `isRestoringFromStorage` (L44) empêche la boucle de sauvegarde pendant la restauration,
|
||||
mais sans timeout : si `restoreFromLocalStorage()` lance une exception non-catchée, le flag reste
|
||||
`true` et toutes les sauvegardes futures sont silencieusement ignorées.
|
||||
|
||||
### P8 — Routes de debug exposées en production, pas de lazy loading
|
||||
|
||||
**Fichier** : `src/router/index.ts`
|
||||
|
||||
Les routes debug (CoversCache, AudioCache, UPnP Explorer, etc.) sont accessibles en production
|
||||
sans contrôle d'accès. Par ailleurs, tous les composants sont importés statiquement, augmentant
|
||||
le bundle initial inutilement — les vues debug notamment ne sont jamais utilisées en prod.
|
||||
|
||||
### P9 — `formatMsToShortTime` est un alias inutile
|
||||
|
||||
**Fichier** : `src/utils/time.ts` (L58-59)
|
||||
|
||||
```typescript
|
||||
// Actuellement
|
||||
export function formatMsToShortTime(ms: number | null): string {
|
||||
return formatMsToTime(ms);
|
||||
}
|
||||
```
|
||||
|
||||
Fonction identique à `formatMsToTime`. Tous les appelants peuvent utiliser directement
|
||||
`formatMsToTime`.
|
||||
|
||||
### P10 — `truncate()` dans `string.ts` peut dépasser `maxLength`
|
||||
|
||||
**Fichier** : `src/utils/string.ts` (L46-48)
|
||||
|
||||
```typescript
|
||||
// Actuellement
|
||||
export function truncate(str: string, maxLength: number, suffix = '…'): string {
|
||||
return str.length > maxLength ? str.slice(0, maxLength - suffix.length) + suffix : str;
|
||||
}
|
||||
```
|
||||
|
||||
Si `suffix.length >= maxLength`, `str.slice(0, maxLength - suffix.length)` retourne une chaîne
|
||||
de longueur négative (comportement silencieux en JS, retourne `''`), et le résultat final est
|
||||
plus long que `maxLength`.
|
||||
|
||||
### P11 — `DEFAULT_COVER_SVG` inline dans coverCache.ts
|
||||
|
||||
**Fichier** : `src/services/coverCache.ts` (L206-226)
|
||||
|
||||
Un SVG inline de ~20 lignes est inclus dans chaque bundle qui importe `coverCache`. Il devrait
|
||||
être un fichier `src/assets/default-cover.svg` importé nativement par Vite (ce qui permet le
|
||||
tree-shaking et le caching HTTP séparé).
|
||||
|
||||
### P12 — `animations` CSS sans `prefers-reduced-motion`
|
||||
|
||||
**Fichiers** : `src/assets/styles/glass-theme.css` (L384-401),
|
||||
`src/assets/styles/pmocontrol.css` (L82)
|
||||
|
||||
Les animations `glassShimmer` (2s infini) et le `pulse` du badge de statut `Transitioning`
|
||||
s'exécutent sans tenir compte de `prefers-reduced-motion: reduce`. Sur certains systèmes ou
|
||||
pour des utilisateurs sensibles au mouvement, ces animations sont gênantes.
|
||||
|
||||
### P13 — CSS dupliqué dans drawers.css
|
||||
|
||||
**Fichier** : `src/assets/styles/drawers.css` (L105-149)
|
||||
|
||||
`drawer-close-btn` et `drawer-back-btn` partagent 90% des styles. Un TODO présent en L167
|
||||
("remplacer par la classe globale .section-title") confirme cette dette. La variable
|
||||
`var(--opacity-disabled)` est utilisée mais non définie dans `variables.css`.
|
||||
|
||||
### P14 — `browseContainer` : clés de cache fragiles et pas de pagination
|
||||
|
||||
**Fichier** : `src/composables/useMediaServers.ts` (L145, L239)
|
||||
|
||||
Les clés de cache sont construites comme `${serverId}/${containerId}`. Si un `containerId`
|
||||
contient un slash (séparateur d'URL), la clé est ambigüe. Par exemple, `server1/a/b` peut
|
||||
correspondre à serverId=`server1`, containerId=`a/b` ou serverId=`server1/a`, containerId=`b`.
|
||||
|
||||
La pagination n'est pas implémentée côté composable : `browseContainer` charge toujours
|
||||
offset=0, limit=50. Pour les containers avec 500+ items, les items au-delà de 50 ne sont
|
||||
jamais accessibles.
|
||||
|
||||
### P15 — Notifications sans limite de taille dans `ui.ts`
|
||||
|
||||
**Fichier** : `src/stores/ui.ts` (L50, L55-57)
|
||||
|
||||
Un bug ou une boucle d'erreur peut générer des centaines de notifications. Le tableau
|
||||
`notifications` n'est pas limité. Chaque notification crée un `setTimeout` individuel, et
|
||||
si le store est détruit avant l'expiration, ces callbacks persistent (ghosts).
|
||||
|
||||
---
|
||||
|
||||
## Plan d'exécution
|
||||
|
||||
Les corrections sont groupées par effort et impact. Les P0–P3 concernent la fiabilité
|
||||
(fuites mémoire, réactivité), les P4–P8 la robustesse et maintenabilité, les P9–P15 la
|
||||
qualité et la dette technique.
|
||||
|
||||
### Étape 1 — Corriger les fuites mémoire SSE (P0)
|
||||
|
||||
Dans `useSSE.ts`, stocker et appeler les fonctions de cleanup retournées par `onRendererEvent` /
|
||||
`onMediaServerEvent` :
|
||||
|
||||
```typescript
|
||||
// useSSE.ts – useRendererEvents()
|
||||
onMounted(() => {
|
||||
const cleanup = onRendererEvent(rendererId(), handler);
|
||||
onUnmounted(cleanup); // ← actuellement ignoré
|
||||
});
|
||||
```
|
||||
|
||||
Dans `imageCache.ts`, exporter une fonction `destroyImageCache()` qui appelle `clearInterval`
|
||||
sur le timer de cleanup, et l'appeler dans le `onUnmounted` de l'app root.
|
||||
|
||||
### Étape 2 — Stabiliser la réactivité des snapshots (P1)
|
||||
|
||||
Remplacer `shallowRef<Map<...>>` + `triggerSnapshotReactivity` par `reactive(new Map<...>)`.
|
||||
Vue 3 rend les Maps réactives nativement. Les composants qui lisent `snapshots.get(id)`
|
||||
seront notifiés uniquement si ce `id` change.
|
||||
|
||||
```typescript
|
||||
// Avant
|
||||
const snapshots = shallowRef<Map<string, FullRendererSnapshot>>(new Map());
|
||||
function triggerSnapshotReactivity() {
|
||||
snapshots.value = new Map(snapshots.value);
|
||||
}
|
||||
|
||||
// Après
|
||||
const snapshots = reactive(new Map<string, FullRendererSnapshot>());
|
||||
// Les modifications directes (snapshots.set/delete) déclenchent la réactivité
|
||||
```
|
||||
|
||||
### Étape 3 — Timeout fetch + AbortController (P4)
|
||||
|
||||
Ajouter un helper dans `api.ts` :
|
||||
|
||||
```typescript
|
||||
function fetchWithTimeout(url: string, options?: RequestInit, timeoutMs = 10_000): Promise<Response> {
|
||||
const controller = new AbortController();
|
||||
const id = setTimeout(() => controller.abort(), timeoutMs);
|
||||
return fetch(url, { ...options, signal: controller.signal })
|
||||
.finally(() => clearTimeout(id));
|
||||
}
|
||||
```
|
||||
|
||||
Utiliser `fetchWithTimeout` pour toutes les requêtes dans le service API.
|
||||
|
||||
### Étape 4 — Corriger `useTabs` watchs et flag de restauration (P7)
|
||||
|
||||
Fusionner les trois `watch()` en un seul `watchEffect` avec un debounce unique (100ms).
|
||||
Encadrer `isRestoringFromStorage` dans un bloc `try/finally` :
|
||||
|
||||
```typescript
|
||||
async function restoreFromLocalStorage() {
|
||||
isRestoringFromStorage = true;
|
||||
try {
|
||||
// ... logique de restauration
|
||||
} catch (e) {
|
||||
console.error('Tab restore failed:', e);
|
||||
} finally {
|
||||
isRestoringFromStorage = false;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### Étape 5 — Limit de notifications et nettoyage timers (P15)
|
||||
|
||||
```typescript
|
||||
const MAX_NOTIFICATIONS = 5;
|
||||
|
||||
function addNotification(notif: Omit<Notification, 'id'>): void {
|
||||
if (notifications.value.length >= MAX_NOTIFICATIONS) {
|
||||
notifications.value.shift(); // supprimer la plus ancienne
|
||||
}
|
||||
const id = nextId++;
|
||||
const timer = setTimeout(() => removeNotification(id), notif.duration ?? 5000);
|
||||
notificationTimers.set(id, timer);
|
||||
notifications.value.push({ ...notif, id });
|
||||
}
|
||||
|
||||
function $dispose() {
|
||||
notificationTimers.forEach(clearTimeout);
|
||||
notificationTimers.clear();
|
||||
}
|
||||
```
|
||||
|
||||
### Étape 6 — Lazy loading des routes et protection debug (P8)
|
||||
|
||||
```typescript
|
||||
// router/index.ts
|
||||
const DebugView = () => import('../views/DebugView.vue');
|
||||
const isDev = import.meta.env.DEV;
|
||||
|
||||
const routes = [
|
||||
// ... routes normales
|
||||
...(isDev ? [{ path: '/debug', component: DebugView }] : []),
|
||||
{ path: '/:pathMatch(.*)*', redirect: '/' }, // wildcard 404
|
||||
];
|
||||
```
|
||||
|
||||
### Étape 7 — Corrections mineures (P9, P10, P11, P12, P13)
|
||||
|
||||
- **P9** : Supprimer `formatMsToShortTime`, remplacer tous les appels par `formatMsToTime`
|
||||
- **P10** : Ajouter un guard dans `truncate` : `if (suffix.length >= maxLength) return str.slice(0, maxLength)`
|
||||
- **P11** : Déplacer le SVG dans `src/assets/default-cover.svg` et l'importer avec `import defaultCover from '../assets/default-cover.svg?raw'`
|
||||
- **P12** : Entourer les animations CSS avec `@media (prefers-reduced-motion: no-preference) { ... }`
|
||||
- **P13** : Factoriser `drawer-close-btn` / `drawer-back-btn` avec une classe `.drawer-icon-btn`. Définir `--opacity-disabled: 0.4` dans `variables.css`
|
||||
|
||||
### Étape 8 — Clés de cache et pagination (P14)
|
||||
|
||||
Encoder les IDs dans les clés de cache :
|
||||
|
||||
```typescript
|
||||
const cacheKey = `${encodeURIComponent(serverId)}:${encodeURIComponent(containerId)}`;
|
||||
```
|
||||
|
||||
Utiliser `:` comme séparateur (absent de l'encoding) pour éviter toute ambigüité.
|
||||
|
||||
Pour la pagination, ajouter une propriété `hasMore: boolean` et `loadMore()` au résultat de
|
||||
`browseContainer`, incrementant offset à chaque appel.
|
||||
|
||||
### Ordre d'exécution
|
||||
|
||||
1. Étape 1 — fuites SSE (P0) — fiabilité critique
|
||||
2. Étape 2 — réactivité Map (P1) — fiabilité
|
||||
3. Étape 3 — timeout fetch (P4) — robustesse réseau
|
||||
4. Étape 4 — useTabs (P7) — fiabilité des onglets
|
||||
5. Étape 5 — notifications (P15) — stabilité UI
|
||||
6. Étape 6 — router (P8) — sécurité + performance bundle
|
||||
7. Étape 7 — corrections mineures (P9–P13)
|
||||
8. Étape 8 — cache keys + pagination (P14)
|
||||
|
||||
## Règle après ces corrections
|
||||
|
||||
**Interdit** : créer un abonnement SSE (`onRendererEvent`, `onMediaServerEvent`) sans stocker
|
||||
et appeler la fonction de cleanup retournée dans `onUnmounted`.
|
||||
|
||||
**Interdit** : utiliser `shallowRef<Map<...>>` avec mutation directe — utiliser `reactive(new Map())`
|
||||
pour les Maps qui doivent déclencher la réactivité Vue sur leurs entrées.
|
||||
|
||||
**Obligatoire** : toute requête `fetch()` dans un service doit utiliser `fetchWithTimeout`
|
||||
avec un AbortController.
|
||||
Reference in New Issue
Block a user