pmomusic/Blackboard/Todo/Frontend_Review.md

** Ce travail devra être réalisé en suivant scrupuleusement les consignes listées dans le fichier @Rules_optimal.md **

Vue d'ensemble

Le frontend de PMOMusic est une application Vue 3 + TypeScript avec Pinia, organisée autour de composables réactifs, d'un client SSE centralisé et d'un cache API à plusieurs niveaux. L'architecture générale est solide : séparation claire composables/services/vues, typage strict (strict: true dans tsconfig.app.json), reconnexion SSE avec backoff exponentiel.

Cette revue documente les bugs avérés, les fragilités de conception et les axes d'amélioration relevés lors d'une lecture complète des fichiers pmoapp/webapp/src/.

---

Bugs

1. apiCache.ts:130-133 — Mutation globale du TTL non réentrante

Problème : la méthode fetch() accepte un ttl optionnel par appel. Pour l'appliquer, elle modifie this.options.ttl globalement avant d'appeler this.set(), puis le restaure :

// apiCache.ts:130-133
if (ttl) {
  const originalTtl = this.options.ttl;
  this.options.ttl = ttl;          // (A) modification globale
  this.set(endpoint, data, params);
  this.options.ttl = originalTtl;  // (B) restauration
}

Cause : fetcher() est await-é (ligne 128) avant ce bloc. Pendant cet await, d'autres microtasks peuvent s'intercaler et appeler isFresh() ou set(), qui lisent this.options.ttl. Si deux appels fetch() avec des ttl différents sont en vol simultanément, la restauration de (B) peut effacer la valeur posée par le second appel concurrent, ou (A) peut lire un TTL modifié par un autre appel.

Solution : passer le ttl directement à set() comme paramètre, sans modifier l'état partagé :

// Dans set() : ajouter un paramètre ttl optionnel
set<T>(endpoint: string, data: T, params?: ..., etag?: string, ttl?: number): void {
  const key = this.makeKey(endpoint, params);
  this.cache.set(key, {
    data,
    timestamp: Date.now(),
    ttl: ttl ?? this.options.ttl,  // TTL par entrée, pas global
    etag,
  });
  this.notifySubscribers(key, data);
}

// Dans isFresh() : lire le ttl de l'entrée
private isFresh(key: string): boolean {
  const entry = this.cache.get(key);
  if (!entry) return false;
  return Date.now() - entry.timestamp < (entry.ttl ?? this.options.ttl);
}

// Dans fetch() : supprimer le bloc de mutation globale
this.set(endpoint, data, params, undefined, ttl);

---

2. useRenderers.ts:151 + 222 — Réassignation post-switch écrase le nouvel objet

Problème : le handler onRendererEvent termine par une ligne inconditionnelle :

// useRenderers.ts:222
snapshotState.snapshots.set(rendererId, snapshot);

Cette ligne s'exécute pour tous les types d'événements après le switch, y compris pour position_changed et metadata_changed qui ont déjà créé et stocké un nouvel objet dans le Map à l'intérieur du switch :

// position_changed — ligne 151 : stocke newSnapshot
snapshotState.snapshots.set(rendererId, newSnapshot);
break;
// → puis ligne 222 écrase avec snapshot (proxy d'origine)

// metadata_changed — ligne 176 : stocke un spread
snapshotState.snapshots.set(rendererId, { ...snapshot, state: { ...snapshot.state } });
break;
// → puis ligne 222 écrase avec snapshot (proxy d'origine)

Cause : break sort du switch mais pas de la fonction. La ligne 222 est atteinte dans tous les cas.

Conséquence : les objets créés pour forcer la détection de changement par Vue sont immédiatement écrasés. Le mécanisme de réactivité fonctionne malgré tout (le proxy muté est re-stocké), mais la logique est trompeuse et fragile : si Vue venait à optimiser la détection d'identité des objets réactifs, cette redondance deviendrait un bug visible.

Solution : supprimer la ligne 222 et s'assurer que chaque branche du switch stocke explicitement son résultat dans la Map. Les branches volume_changed, mute_changed et binding_changed qui mutent directement snapshot doivent aussi créer un nouvel objet :

case "volume_changed":
  snapshotState.snapshots.set(rendererId, {
    ...snapshot,
    state: { ...snapshot.state, volume: event.volume },
  });
  break;

case "mute_changed":
  snapshotState.snapshots.set(rendererId, {
    ...snapshot,
    state: { ...snapshot.state, mute: event.mute },
  });
  break;
// idem pour binding_changed, stream_state_changed

Supprimer la ligne 222. Chaque case devient responsable de son stockage, ce qui élimine aussi le besoin de toRaw().

---

3. useRenderers.ts:122 — as any sur transport_state

Problème :

// useRenderers.ts:122
snapshot.state.transport_state = event.state as any;

Cause : event.state est typé string (type SSE générique), alors que transport_state est une union littérale ("PLAYING" | "PAUSED" | "STOPPED" | ...). Le cast as any contourne la vérification de type.

Conséquence : si le backend envoie une valeur non prévue (ex. "TRANSITIONING"), elle sera stockée sans validation. Les composants qui comparent transport_state === "PLAYING" ne matcheront pas et l'UI restera muette.

Solution : définir un guard de type ou une assertion dans types.ts :

// services/pmocontrol/types.ts
export type TransportState = "PLAYING" | "PAUSED" | "STOPPED" | "NO_MEDIA" | "TRANSITIONING";

export function isTransportState(s: string): s is TransportState {
  return ["PLAYING", "PAUSED", "STOPPED", "NO_MEDIA", "TRANSITIONING"].includes(s);
}

// useRenderers.ts — case state_changed
case "state_changed":
  if (isTransportState(event.state)) {
    snapshot.state.transport_state = event.state;
  } else {
    console.warn(`[useRenderers] transport_state inconnu: ${event.state}`);
  }
  break;

---

4. apiCache.ts:181-200 — invalidate() supprime silencieusement les subscriptions actives

Problème :

// apiCache.ts:197-200
keysToDelete.forEach(key => {
  this.cache.delete(key);
  this.subscriptions.delete(key);  // ← subscriptions perdues sans notification
});

Cause : lors d'une invalidation (ex. après un SSE event), les callbacks enregistrés via subscribe() sont supprimés de la Map. Les composants ne sont pas notifiés de la suppression et ne reçoivent plus les futures mises à jour même après un refetch.

Conséquence : un composant qui a appelé apiCache.subscribe(...) et qui survit à une invalidation devient « sourd » sans le savoir.

Solution : conserver les subscriptions lors d'une invalidation — seule la donnée en cache est périmée, pas les abonnés :

invalidate(pattern: string): void {
  const keysToDelete: string[] = [];
  // ... construction de keysToDelete inchangée ...
  keysToDelete.forEach(key => {
    this.cache.delete(key);
    // NE PAS supprimer this.subscriptions.get(key)
    // Les abonnés seront notifiés lors du prochain set()
  });
}

Si l'on veut notifier les abonnés d'une invalidation (pour qu'ils affichent un état de chargement), ajouter un callback optionnel onInvalidate dans l'interface de subscription.

---

Fragilités de conception

5. useRenderers.ts:7,143-151 — toRaw() comme contournement de réactivité Vue

Problème : le code utilise toRaw() pour extraire l'objet brut d'un proxy Vue avant de faire un spread, afin que la copie ne contienne pas de getters réactifs qui pointent vers l'objet original :

// useRenderers.ts:143-151
const rawState = toRaw(snapshot.state);
const newState = { ...rawState };
const newSnapshot = { ...snapshot, state: newState };
snapshotState.snapshots.set(rendererId, newSnapshot);

Cause : les Maps imbriquées dans un objet reactive() ont un comportement de réactivité peu prévisible dans Vue 3. Vue ne détecte pas les mutations d'éléments d'une Map réactive si la référence de la Map elle-même ne change pas.

Solution recommandée : remplacer reactive(new Map()) par shallowRef(new Map()) pour les Maps qui contiennent des données complexes. La réactivité se déclenche en remplaçant la Map entière (ou en forçant un triggerRef) :

// Au lieu de :
const snapshotState = reactive<RendererSnapshotState>({ snapshots: reactive(new Map()), ... });

// Utiliser :
const snapshots = shallowRef(new Map<string, FullRendererSnapshot>());

// Pour déclencher la réactivité après mutation :
snapshots.value = new Map(snapshots.value);  // ou triggerRef(snapshots)

Cela rend la propagation de réactivité explicite et élimine le besoin de toRaw().

---

6. useRenderers.ts:552-570 — Timer de debounce non nettoyé dans useRenderer()

Problème : useRenderer() crée un timer de debounce local qui n'est jamais nettoyé si le composant parent est démonté :

// useRenderers.ts:553-566
let refreshDebounceTimer: ReturnType<typeof setTimeout> | null = null;
const REFRESH_DEBOUNCE_MS = 500;

async function refresh(force = true) {
  if (refreshDebounceTimer !== null) return;
  refreshDebounceTimer = setTimeout(() => {
    refreshDebounceTimer = null;
  }, REFRESH_DEBOUNCE_MS);
  await Promise.all([...]);
}

Cause : pas d'appel à clearTimeout dans un onUnmounted. Si le composant est démonté pendant les 500 ms du debounce, le timer continue de s'exécuter.

Conséquence : fuite mémoire potentielle ; dans des cas extrêmes (navigation rapide), le callback peut tenter de déclencher un fetch sur un composant déjà démonté.

Solution :

import { onUnmounted } from 'vue';

// Dans useRenderer() :
onUnmounted(() => {
  if (refreshDebounceTimer !== null) {
    clearTimeout(refreshDebounceTimer);
    refreshDebounceTimer = null;
  }
});

---

7. useRenderers.ts:45-46 — Singleton SSE initialisé par flag de module non réinitialisable

Problème :

// useRenderers.ts:45-46
let sseInitialized = false;
function ensureSSEInitialized() {
  if (sseInitialized) return;
  // ...
  sseInitialized = true;
}

Cause : ce flag de module est persistant pour toute la durée de vie de la page. Si la connexion SSE est perdue puis rétablie avec un nouvel objet PMOControlSSE, le handler onRendererEvent précédent peut ne plus être actif, mais sseInitialized empêche sa re-enregistration.

Conséquence : après une déconnexion et reconnexion SSE, les événements renderer peuvent ne plus être reçus par useRenderers jusqu'à un rechargement de page.

Solution : exposer une fonction resetSSE() qui remet sseInitialized = false et la connecter à l'événement de reconnexion du service SSE. Alternativement, utiliser le pattern provide/inject ou un store Pinia pour gérer le cycle de vie SSE explicitement, en lieu et place du flag de module.

---

Qualité du code

8. useTabs.ts — Deep watch déclenchant une sérialisation localStorage à chaque mutation

Problème : le watch qui persiste l'état des onglets utilise { deep: true } sur un tableau qui peut contenir jusqu'à 12 entrées avec des métadonnées :

// useTabs.ts:349-355
watch(
  () => [state.tabs, state.activeTabId, state.tabHistory],
  () => { saveToLocalStorage(); },
  { deep: true },
);

Cause : { deep: true } traverse récursivement toutes les propriétés observées. saveToLocalStorage() appelle JSON.stringify sur l'ensemble des tabs à chaque mutation, même mineure (ex. changement de activeTabId).

Solution : surveiller les propriétés individuellement et sérialiser uniquement ce qui change, ou utiliser un computed pour construire la clé de changement :

// Watch séparés, sans deep
watch(() => state.activeTabId, saveToLocalStorage);
watch(() => state.tabHistory.length, saveToLocalStorage);
watch(
  () => state.tabs.map(t => t.id + t.type + (t.metadata?.rendererId ?? '')).join('|'),
  saveToLocalStorage,
);

---

9. UnifiedControlView.vue:40-49 — Swipe : clientX final au lieu de la position initiale

Problème :

// UnifiedControlView.vue:40-49
useSwipe(viewRef, {
  threshold: 50,
  onSwipeEnd(_e: TouchEvent, swipeDirection: string) {
    if (swipeDirection === "right" && !drawerOpen.value) {
      const touch = _e.changedTouches[0];
      if (touch && touch.clientX < 50) {  // ← position finale du doigt
        drawerOpen.value = true;
      }
    }
  },
});

Cause : onSwipeEnd reçoit l'événement touchend. Dans changedTouches, clientX est la position finale du doigt (après le swipe), pas la position initiale. Un swipe commençant à x=30 et terminant à x=150 a clientX=150 dans touchend — la condition < 50 ne sera jamais vraie pour un swipe horizontal significatif.

Conséquence : le geste de swipe depuis le bord gauche ne fonctionne probablement pas sur les appareils tactiles.

Solution : capturer la position initiale dans onSwipeStart :

const swipeStartX = ref(0);

useSwipe(viewRef, {
  threshold: 50,
  onSwipeStart(e: TouchEvent) {
    swipeStartX.value = e.touches[0]?.clientX ?? 0;
  },
  onSwipeEnd(_e: TouchEvent, swipeDirection: string) {
    if (swipeDirection === "right" && !drawerOpen.value && swipeStartX.value < 50) {
      drawerOpen.value = true;
    }
  },
});

---

10. api.ts:50 — Réponse JSON non validée avant le cast TypeScript

Problème :

// api.ts:50
return response.json();  // retour typé T par inférence, sans validation

Cause : response.json() retourne Promise<any>. TypeScript accepte le retour car la méthode request<T> promet Promise<T>, mais aucune validation de structure n'est effectuée.

Conséquence : si le backend renvoie un schéma légèrement différent (champ renommé, type changé), le bug se manifestera loin du point d'appel avec un message cryptique. En développement avec plusieurs instances en parallèle (udn_prefix différent), une requête dirigée vers la mauvaise instance peut retourner un format inattendu.

Solution pragmatique : ajouter une validation légère avec un type guard pour les réponses critiques, ou au minimum loguer la réponse brute en mode développement :

private async request<T>(path: string, options: RequestInit = {}): Promise<T> {
  // ...
  const data = await response.json();
  if (import.meta.env.DEV && data == null) {
    console.warn(`[PMOControlAPI] Réponse vide pour ${path}`);
  }
  return data as T;
}

Pour les endpoints critiques (getRendererFullSnapshot, getRenderers), envisager un schéma de validation Zod ou une assertion runtime minimale.

---

Accessibilité et feedback utilisateur

11. Absence de labels ARIA sur les contrôles transport

Les composants TransportControls.vue et VolumeControl.vue contiennent des boutons iconiques (play, pause, stop, volume) sans attributs aria-label. Les lecteurs d'écran ne peuvent pas identifier la fonction de ces contrôles.

Correction minimale :

<!-- TransportControls.vue -->
<button @click="play" aria-label="Lecture">
  <PlayIcon />
</button>
<button @click="pause" aria-label="Pause">
  <PauseIcon />
</button>

---

12. Erreurs réseau silencieuses sans feedback utilisateur

Plusieurs appels critiques sont lancés en fire-and-forget sans propagation vers l'UI :

// useRenderers.ts:86
void fetchRenderers(true);   // erreur loggée en console uniquement

// useRenderers.ts:89
void fetchRendererSnapshot(rendererId, { force: true });  // idem

Le store ui.ts dispose d'un système de notifications toast (addNotification). Les erreurs de réseau devraient y être propagées pour informer l'utilisateur :

import { useUIStore } from '@/stores/ui';

const uiStore = useUIStore();

// Dans le handler SSE :
try {
  await fetchRenderers(true);
} catch {
  uiStore.addNotification({
    message: 'Impossible de rafraîchir la liste des renderers',
    type: 'error',
  });
}

---

Points forts à conserver

Ces patterns sont bien conçus et ne doivent pas être modifiés dans les corrections ci-dessus :

• Déduplication des requêtes en vol (apiCache.ts:100-112) : évite les appels réseau redondants quand plusieurs composants demandent la même ressource simultanément.
• Watch sélectif sur les IDs (UnifiedControlView.vue:139-150) : calcule une clé synthétique ids.join(',') au lieu d'un deep watch sur le tableau de renderers.
• Backoff exponentiel SSE (sse.ts) : reconnexion progressive 1s→2s→4s→8s→16s→30s avec cap. Implémentation robuste.
• Fetch batch contrôlé (useRenderers.ts:351-383) : fetchBatchSnapshots() avec concurrence limitée (défaut 3) et délai inter-batches. Évite de saturer le réseau au démarrage.
• filterRenderers par UDN (UnifiedControlView.vue:107-120) : filtre les WebRenderers étrangers en comparant le UDN normalisé. Logique correcte avec gestion du préfixe uuid:.

---

État d'avancement — corrections appliquées (commit 2026-04-06)

Les 12 points de la revue ont été traités. Le tableau ci-dessous récapitule ce qui a été fait et ce qui reste à finir.

#	Problème	État
1	apiCache — TTL mutation globale	✅ Corrigé (ttl par entrée dans CacheEntry, isFresh() lit entry.ttl)
2	useRenderers — double réassignation post-switch	✅ Corrigé (chaque case responsable, ligne 222 supprimée)
3	useRenderers — as any sur transport_state	✅ Corrigé (isTransportState guard dans types.ts)
4	apiCache — invalidate() détruisait les subscriptions	✅ Corrigé (this.subscriptions.delete supprimé)
5	useRenderers — toRaw() / reactive(Map) fragile	✅ Migré vers shallowRef + helpers triggerSnapshotReactivity() / triggerLoadingReactivity()
6	useRenderer() — timer debounce non nettoyé	✅ Corrigé (onUnmounted + clearTimeout)
7	Singleton SSE non réinitialisable	✅ Corrigé (resetSSE() exposé dans le retour de useRenderers())
8	useTabs — deep watch coûteux	✅ Corrigé (watches séparés sans deep: true)
9	Swipe — clientX final au lieu d'initial	✅ Corrigé (swipeStartX capturé dans onSwipeStart)
10	api.ts — JSON non validé	✅ Corrigé (log dev-mode pour réponse nulle)
11	Absence de labels ARIA	✅ Corrigé (4 boutons transport + bouton mute + slider volume)
12	Erreurs réseau silencieuses	✅ Corrigé (uiStore.notifyError() dans fetchRenderers et fetchRendererSnapshot)

---

Tâches restantes — ✅ Toutes corrigées (commit 2026-04-06)

#	Problème résiduel	État
A	state_changed — mutation directe sans trigger réactivité	✅ Corrigé (spread + triggerSnapshotReactivity())
B	queue_refreshing/queue_updated — queueRefreshingIds sans trigger	✅ Corrigé (triggerQueueReactivity() ajoutée et appelée)
C	toRaw import obsolète, position_changed simplifié	✅ Corrigé (toRaw supprimé de l'import, spread direct)

---

A. useRenderers.ts — state_changed : mutation directe sans déclenchement de réactivité

Problème : avec la migration vers shallowRef, les objets dans la Map ne sont plus des proxies Vue. La mutation directe de snapshot.state.transport_state ne déclenche aucune réactivité — les composants ne se mettront pas à jour quand l'état de transport change :

// useRenderers.ts — case state_changed (code actuel)
case "state_changed":
  if (isTransportState(event.state)) {
    snapshot.state.transport_state = event.state;  // ← mutation directe, pas de trigger
  }
  break;

Solution : créer un nouvel objet, comme pour volume_changed et mute_changed :

case "state_changed":
  if (isTransportState(event.state)) {
    snapshots.value.set(rendererId, {
      ...snapshot,
      state: { ...snapshot.state, transport_state: event.state },
    });
  } else {
    console.warn(`[useRenderers] transport_state inconnu: ${event.state}`);
  }
  break;

---

B. useRenderers.ts — queue_refreshing / queue_updated : mutations de queueRefreshingIds sans trigger

Problème : queueRefreshingIds est un shallowRef<Set>. Les appels .add() et .delete() sur .value ne déclenchent pas la réactivité de shallowRef :

// useRenderers.ts — case queue_refreshing (code actuel)
case "queue_refreshing":
  queueRefreshingIds.value.add(rendererId);   // ← pas de trigger
  break;

case "queue_updated":
  queueRefreshingIds.value.delete(rendererId); // ← pas de trigger
  break;

Le composable isQueueRefreshing(id) retourne queueRefreshingIds.value.has(id). Sans trigger, les templates qui dépendent de cette valeur ne se recalculeront pas.

Solution : ajouter une fonction triggerQueueReactivity() analogue à triggerLoadingReactivity() et l'appeler après chaque mutation :

function triggerQueueReactivity() {
  queueRefreshingIds.value = new Set(queueRefreshingIds.value);
}

// Dans le switch :
case "queue_refreshing":
  queueRefreshingIds.value.add(rendererId);
  triggerQueueReactivity();
  break;

case "queue_updated":
  snapshot.state.queue_len = event.queue_length;
  queueRefreshingIds.value.delete(rendererId);
  triggerQueueReactivity();
  void fetchRendererSnapshot(rendererId, { force: true });
  break;

---

C. useRenderers.ts — import toRaw et commentaire obsolètes

Problème : avec shallowRef, les objets stockés dans snapshots.value sont de simples objets JavaScript (jamais des proxies Vue). L'appel toRaw(snapshot.state) dans position_changed est devenu un no-op, et le commentaire qui le justifie est trompeur :

// useRenderers.ts:160-163 (commentaire et import obsolètes)
// IMPORTANT: Utiliser toRaw() pour obtenir l'objet brut non-réactif avant de copier
// sinon Vue copie les getters réactifs qui continuent à pointer vers l'objet d'origine
const rawState = toRaw(snapshot.state);
const newState = { ...rawState };

Solution : supprimer le toRaw(), simplifier en spread direct, retirer toRaw de l'import ligne 7 :

// Remplacer :
import { ref, shallowRef, computed, toRaw, type Ref, onUnmounted } from "vue";

// Par :
import { ref, shallowRef, computed, type Ref, onUnmounted } from "vue";

// Dans position_changed :
const newSnapshot = {
  ...snapshot,
  state: {
    ...snapshot.state,
    position_ms: positionMs ?? 0,
    duration_ms: durationMs,
  },
};
snapshots.value.set(rendererId, newSnapshot);

Cette simplification rend aussi le cas position_changed cohérent avec les autres cases (volume_changed, mute_changed, etc.) qui construisent directement l'objet final sans passer par une variable intermédiaire.