Skip to content

Revue de code globale — fin Phase 2 (2026-05-06)

Scope : audit du nouveau code Phase 2 livré depuis le précédent audit du 2026-05-02 — modules backend analyst/, earnings/, config/ (déjà partiellement audité), news/, refonte sector via FinnhubSectorClassifier, ajout instrumentType end-to-end, sidebar config et 4 routers @Primary ; côté frontend la sidenav outils chart, les sous-blocs Fondamentaux (analyst + earnings), l'accordéon news, le chart analyse v1+v2+v3 et l'overlay benchmark v2. Modules gelés Phase 0 hors scope, modules portfolio/ / watchlist/ audités uniquement sur leurs interactions Phase 2 (validation autocomplete, instrumentType).

Méthode : revue par agent automatisé, read-only sur le checkout courant, briefée sur les conventions ground truth (CLAUDE.md, architecture.md, ddd.md) et sur les findings que l'audit 2026-05-02 avait remontés. Findings classés par sévérité avec référence fichier:ligne. Limité aux risques objectifs et aux violations de conventions stated.

État du commit au moment de la revue : working tree de la branche feat, dernier commit 31997d2 feat: recommendation analyst. Phase 2 marquée close dans backlog.md / fonctionnalites.md avec news inline minimaliste comme dernier livrable. 6 caches Caffeine, 7 clés runtime, 11 repositories frontend, 4 routers @Primary. Tests : 41 fichiers backend, 18 specs frontend.

Résumé exécutif

App globalement saine et la Phase 2 a tenu le pattern hexagonal proprement : ports/adapters appliqués sans concession à analyst/ et earnings/, fail-soft documenté sur les endpoints Finnhub paid-tier, mappers purs séparés des HTTP clients, mocks avec symboles réservés cohérents (UNKNOWN / RATELIMIT / NOTARGET / NOCALENDAR). Le swap Twelve Data → Finnhub côté sector est bien couvert par le test RoutingSectorClassifierTest qui pin la décision routing.

Principaux risques identifiés : (1) le FinnhubAnalystClient n'a pas de test MockWebServer alors que tous ses voisins (news, earnings, sector, twelvedata) en ont un — c'est une couture observable manquante ; (2) la page /settings/configuration n'expose ni analyst.provider ni earnings.provider alors que le backend les surface dans ConfigKeys.KNOWN_KEYS et que la doc affirme que les 4 toggles y sont éditables — drift contrat silencieux ; (3) RoutingSectorClassifier route twelvedata → Finnhub ce qui veut dire qu'un user qui aurait rempli sa Twelve Data API key mais pas sa Finnhub se verra refuser le sector benchmark sans message évident — la validation côté Settings ne couvre pas ce cas asymétrique. Plusieurs petits items de cohérence sur la stratégie de cache (key prefix présent sur MARKET_CHART_CACHE mais absent ailleurs, contradiction avec la doc).

Forces : i18n FR / EN strictement parallèles (334 keys en miroir), tests-as-documentation très bien tenus sur les mappers Finnhub avec docstring + test names en phrases complètes, pas de wildcard imports neufs, inject() partout côté front, signal-based 100 %, no BehaviorSubject. La discipline RestController/Service/Client reste claire à travers tous les nouveaux modules.

Critique

(blocker ou risque sévère — corruption data, sécurité, contrat cassé)

1. La page /settings/configuration cache deux clés runtime live

  • frontend/src/app/features/settings/configuration/configuration.ts:14-19 ne déclare que TWELVE_DATA_KEY, FINNHUB_KEY, CACHE_TTL_KEY, MARKET_PROVIDER_KEY, NEWS_PROVIDER_KEY. Pas de ANALYST_PROVIDER_KEY ni EARNINGS_PROVIDER_KEY.
  • backend/.../config/application/ConfigKeys.kt:14-51 exporte les 7 clés et GET /api/config les retourne toutes.
  • docs/technique/architecture.md:191-200 et CLAUDE.md affirment que les toggles analyst.provider / earnings.provider sont éditables depuis cette page.

L'utilisateur pour switcher analyst/earnings provider doit éditer la BDD à la main ou redémarrer avec une YAML changée. C'est exactement ce que la Phase 2 promettait de supprimer.

→ Soit afficher les deux toggles supplémentaires dans configuration.html (avec les keys existantes, le code est paramétrable), soit retirer ces clés de ConfigKeys.KNOWN_KEYS et documenter qu'elles restent YAML-only.

Important

(à traiter en priorité, dans la semaine)

2. FinnhubAnalystClient n'a pas de test MockWebServer

  • backend/src/test/.../analyst/infrastructure/analyst/ contient FinnhubAnalystMappersTest, MockAnalystClientTest, RoutingAnalystClientTest mais aucun test sur le client HTTP lui-même.
  • À comparer avec FinnhubClientTest (news), FinnhubEarningsClientTest, FinnhubSectorClassifierTest, TwelveDataClientTest : tous ont un MockWebServer + mappings d'erreurs (401/403/429/5xx/network), test de fail-soft sur le second endpoint, blank-key short-circuit.

Le code dans FinnhubAnalystClient.kt:74-92 (mapping des exceptions HTTP) est dupliqué de FinnhubClient.kt mais n'est exercé par aucun test — le jour où on factorise on perdra silencieusement le bon comportement de fail-soft sur /price-target.

→ Ajouter FinnhubAnalystClientTest calqué sur FinnhubEarningsClientTest (deux endpoints, fail-soft sur le second, suite d'erreurs HTTP, blank-key).

3. Cache de MARKET_CHART_CACHE partiel : seul TwelveDataClient cache, le mock pas

  • backend/.../market/infrastructure/market/TwelveDataClient.kt:55 : @Cacheable(MARKET_CHART_CACHE, key = "'twelvedata|' + …").
  • backend/.../market/infrastructure/market/MockMarketChartClient.kt : pas d'@Cacheable.
  • RoutingMarketChartClient n'a pas non plus d'@Cacheable (dispatcher simple).

Conséquence : en mode mock, chaque dossier rechargé regénère 260 bars synthétiques + une random walk. Pas dramatique en charge dev mais asymétrique, et l'architecture.md affirme « Cache key préfixée par adapter (twelvedata|, mock|) » — la moitié mock| n'existe pas. Plus important : les services news/, analyst/, earnings/, sector cachent au niveau service sans préfixe provider, donc un toggle mock → finnhub continue de servir le mock pour 15 min (cf. RoutingNewsClient docstring : « We deliberately don't include the provider in the key »). C'est intentionnel mais incohérent avec la stratégie de chart cache et la doc induit en erreur.

→ Soit documenter explicitement les deux stratégies (chart = key prefix, autres = service cache sans prefix avec accept-stale-window-on-switch) dans architecture.md et ddd.md. Soit homogénéiser. Pas de patch code, c'est une décision de cohérence.

4. RoutingSectorClassifier route silencieusement vers Finnhub sans dépendance déclarée sur la clé Finnhub

  • RoutingSectorClassifier.kt:39 : PROVIDER_TWELVEDATA -> finnhub.classify(symbol).
  • Si l'utilisateur a market.provider=twelvedata + market.twelvedata.api-key rempli mais market.finnhub.api-key vide, il reçoit MarketUnavailableException("Finnhub API key is missing") → 503 sur le toggle Sector benchmark, alors que tous les autres tools (chart, search) marchent.
  • ConfigTestClient.testTwelveData valide la clé Twelve Data isolément et ne probe pas Finnhub. La page Settings ne signale pas l'asymétrie. Ce n'est pas un bug code mais une couture utilisateur invisible.

→ Soit afficher un avertissement dans la card Twelve Data (« Finnhub aussi requis pour le Sector benchmark »), soit rendre le Sector toggle Finnhub-aware dans benchmarkChoicesForCurrentTicker (filtrer si market.finnhub.api-key non set).

5. AppConfigService.set publie ConfigChangedEvent à l'intérieur de la transaction

  • AppConfigService.kt:74-88 est @Transactional, et eventPublisher.publishEvent(ConfigChangedEvent(...)) est appelé en dernière ligne avant le commit.
  • CacheTtlListener.onConfigChanged appelle cacheManager.setCaffeine(...) qui invalide les caches avant que la nouvelle valeur soit visible aux autres threads. Si la transaction rollback (peu probable mais pas exclu sur lock conflict), le cache aurait déjà été flush.

→ Préférer @TransactionalEventListener(phase = AFTER_COMMIT) sur CacheTtlListener.onConfigChanged, ou publish l'event après le @Transactional (méthode séparée).

6. WatchlistService.add fail-open silencieux sur 503 — masque les outages

  • WatchlistService.kt:66-76 : si SymbolSearchService.validate lève MarketUnavailableException, on log warn puis return true → save passe.
  • Conséquence : un user clique « Add MSFT » pendant un outage Twelve Data → entrée sauvée sans validation → next dossier load échoue avec 503. Le rationale (l'autocomplete a déjà confirmé) est documenté mais (a) n'est pas testé, (b) ne distingue pas un user qui pousse une string sans avoir utilisé l'autocomplete (curl direct).

→ Acceptable choix produit, mais ajouter un test dédié dans WatchlistServiceTest qui pin ce comportement et idéalement remonter un flag unverified côté DTO front.

7. analystRepository.getForSymbol 404 path : pas de test pour le 503 vs 404 distinguishable côté front

  • frontend/src/app/features/ticker/ticker.ts distingue analystNotCovered() (404) de analystError() (503).
  • ticker.spec.ts est très large (1964 lignes) mais l'agent reviewer n'a pas confirmé qu'un test pin spécifiquement la branche « status: 404 → notCovered = true ». Pareil pour earnings.
  • Le pattern est identique à news mais news est plus vieux.

→ Vérifier dans ticker.spec.ts qu'il y a au moins un test pour chaque triplet (loading / 404 / 503) sur analyst et earnings. Si manquant, c'est ajouter 6 specs.

Modérée

(à filer en dette technique, à traiter quand l'occasion se présente)

8. MockMarketChartClient.MOCK_ETF_SYMBOLS ne couvre pas tous les ETFs SPDR mappés

  • MockMarketChartClient.kt:180-201 liste 17 symboles ETF.
  • Les 11 SPDR sectors + 6 broad markets sont là, mais MockSectorClassifier peut router une stock vers un sector ETF quelconque ; un user qui clique « Sector » sur un ticker mock obtient Technology (XLK) puis le call de bars sur XLK retourne un type ETF — OK.
  • Mais le commentaire Phase 2 (suite 5 du CHANGELOG) parle de masquer Sector pour les ETF. Sur un ETF qui n'est PAS dans la liste hardcodée (par exemple un ARKK fictif), il sera tagué STOCK → toggle Sector visible → 404 inline.

→ Acceptable v1, à documenter comme « limite mock » ou élargir le set.

9. EarningsClient.fetch n'a pas de path "no upcoming announcement" testé en prod

  • FinnhubEarningsMappers.toEarningsSnapshot:34 : if (reports.isEmpty() && nextEntry == null) throw NoSuchElementException.
  • Le cas réaliste « entreprise vient de publier ses résultats, pas de prochaine date dans les 90 jours » (reports non vide + nextEntry == null) ne throw pas — c'est correct.
  • Mais le test FinnhubEarningsMappersTest ne couvre que les paths happy / sorted defensively / empty-and-empty-throw. Pas de test pour le path « reports OK + calendar empty » qui est le cas le plus commun en pratique entre deux trimestres.

→ Ajouter un test dédié.

10. FinnhubPriceTarget.toDomainOrNull ignore le cas targetMean == 0 mais targetHigh != 0

  • FinnhubAnalystMappers.kt:74-80 : isEmpty exige les 4 fields à 0.
  • Si Finnhub retourne {high: 200, low: 0, mean: 0, median: 0} (cas dégradé partiel), on construit un PriceTarget avec mean=0 que la UI affiche 0.00 $.

→ Considérer un check plus strict : mean == 0 && median == 0 → null.

11. RoutingSectorClassifier else mismatchant l'ENUM — double validation

  • AppConfigService.validate rejette toute valeur hors ENUM_KEYS[market.provider] (mock | twelvedata).
  • RoutingSectorClassifier.classify:40 : else -> throw IllegalArgumentException("Unknown market provider: '$provider'"). Cette branche ne peut être atteinte qu'en cas d'incohérence (ligne BDD écrite hors validate, ou refacto).

→ Acceptable défensif, mais à harmoniser avec RoutingMarketChartClient / RoutingNewsClient / RoutingAnalystClient / RoutingEarningsClient qui ont tous le même pattern. C'est consistant — pas un finding seul.

12. MockEarningsClient.previousQuarterEnd peut être off-by-one en début de mois

  • MockEarningsClient.kt:128-140 : si reference (today) est un 1er Mars, le quarterEndMonth calculé est mars (((3-1)/3)*3+3 = 3), quarterEnd = 31 mars, reference < quarterEnd → fallback à quarterEnd.minusMonths(3) = 31 décembre.
  • Le résultat est correct mais le code est tortueux. Une simple table month → quarter end serait plus lisible.

→ Refacto cosmétique, à filer en dette.

13. La page Configuration n'a pas de test pour le 7e/8e/... key futur

  • configuration.spec.ts ne semble pas tester la résilience face à l'apparition d'une nouvelle key dans la response — le composant filtre par find(e => e.key === KEY_X) donc les nouvelles keys sont juste ignorées (bonne backward compat) mais on perd la visibilité si une nouvelle key apparaît back sans support front (cf. finding #1).

→ Test « given 8 keys returned by backend, only the 5 wired in the component render » serait un canary utile.

Mineure

(nits, polish, cohérence)

14. FinnhubModels.kt:23 — id: Long mappé en String plus tard, faisable directement

  • FinnhubNewsItem.id: Long puis id.toString() dans FinnhubMappers.toDomain. Au lieu, déclarer id: String avec un converter Jackson (@JsonDeserialize(using = LongAsStringDeserializer)) éviterait le cast intermédiaire. Cosmétique.

15. FinnhubAnalystModels.kt:42-46 — java.math.BigDecimal qualifié au lieu d'import

  • Les autres Finnhub*Models importent BigDecimal. Cohérence.

16. MockNewsClient.kt:44 — magic number rng.nextInt(10) == 0

  • Le « 10 % quiet symbols » est documenté en commentaire mais n'a pas de constante. Mineur.

17. ticker.scss 1673 lignes, 5 occurrences ::ng-deep

  • Documenté comme dette ouverte, à pas étendre. RAS.

18. analyst.repository.ts JSDoc dit « Up to 6 monthly snapshots » — la valeur exacte est en backend HISTORY_DEPTH = 6

  • Pas faux mais fragile : si on bumpe la const back à 12, la JSDoc front ment. Référence croisée silencieuse.

19. ticker.spec.ts à 1964 lignes

  • Le brief réviseur demandait de checker. La taille est explicable (toutes les permutations chart/zoom/overlay/annotation/measure/news/analyst/earnings/watchlist/narrative). À envisager un split par concern (ticker.chart.spec.ts, ticker.fundamentals.spec.ts, etc.) pour la prochaine session, pas urgent.

20. FinnhubEarningsClient.requireApiKey dupliqué x4 (news, analyst, earnings, sector)

  • Le code if (apiKey.isBlank()) throw MarketUnavailableException("Finnhub API key is missing — set market.finnhub.api-key (env FINNHUB_API_KEY)") est dans 4 fichiers. Une internal fun requireFinnhubApiKey(key: String) dans un util shared pour le finnhub package éviterait la dérive du message.

Documentation

(drift doc → code, manques d'explication, etc.)

21. architecture.md:200 affirme « Cache key préfixée par adapter ⇒ pas de collision »

  • Vrai uniquement pour MARKET_CHART_CACHE côté Twelve Data. Pas vrai pour les 4 autres caches (news-by-symbol, analyst-recommendations, earnings, sector-by-symbol) qui sont au niveau service sans préfixe — cf. finding #3.
  • À reformuler : « Le chart cache préfixe par adapter ; les autres caches vivent au-dessus du dispatcher et acceptent une staleness post-switch (15 min max). »

22. architecture.md / developper.md / providers.md annoncent analyst.provider et earnings.provider éditables depuis /settings/configuration

  • Pas vrai aujourd'hui — finding #1. Soit on patch l'UI, soit on patch tous ces docs en cascade.

23. RoutingSectorClassifier rationale dispersée

  • L'asymétrie « market.provider=twelvedata → Finnhub côté sector » est documentée dans architecture.md, RoutingSectorClassifier.kt Kotlin doc, le test, et le CHANGELOG. Beaucoup de prose, mais le fait qu'une key Finnhub vide bloque le Sector benchmark même quand market.provider=twelvedata n'est documenté nulle part (finding #4).

24. news.provider, analyst.provider, earnings.provider se partagent la même clé Finnhub mais le swap est asymétrique

  • Si l'utilisateur met news.provider=mock, analyst.provider=finnhub, earnings.provider=finnhub, sa clé Finnhub est consommée par 2 modules et son quota se split. Pas documenté côté developper.md qui présente les toggles indépendamment.

Récapitulatif

Niveau Count Exemples
Critique 1 Page Settings n'expose pas analyst.provider / earnings.provider (#1)
Important 6 FinnhubAnalystClient pas de MockWebServer test (#2), cache strategy incohérente (#3), Sector silencieux dépend de Finnhub key (#4), event publish in-tx (#5), watchlist fail-open silencieux (#6), front 404/503 paths à pin (#7)
Modérée 6 mock ETF set partiel (#8), earnings empty-cal path pas testé (#9), priceTarget partial-zero (#10), routing else-branch (#11), previousQuarterEnd code tortueux (#12), config UI new-key resilience (#13)
Mineure 7 Magic numbers, JSDoc cross-ref fragile, ticker.spec.ts size, BigDecimal qualifié, requireApiKey x4, etc.
Documentation 4 Cache prefix claim faux (#21), 4 toggles éditables claim faux (#22), Sector→Finnhub key dependency non doc (#23), Finnhub quota split non expliqué (#24)

Reco de priorisation (1 demi-journée chacune) :

  1. Patcher la page Configuration pour exposer analyst.provider + earnings.provider (15 min — le code paramétrique est déjà là, juste 2 constantes + 2 sections HTML).
  2. Ajouter FinnhubAnalystClientTest MockWebServer (45 min — calque FinnhubEarningsClientTest).
  3. Trancher la stratégie de cache (homogénéiser ou documenter explicitement les deux modèles dans architecture.md) (30 min doc, 1-2 h si on veut homogénéiser).
  4. Décider du UX sur le finding #4 (Sector / Finnhub key) — soit avertissement Settings, soit filtrage côté benchmarkChoicesForCurrentTicker.
  5. @TransactionalEventListener(AFTER_COMMIT) sur le CacheTtlListener (10 min).
  6. Le reste en cleanup commit dédié.