Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
frontend/projects/webapp/src/app/feature/budget/budget-list/budget-list-store.ts
Outdated
Show resolved
Hide resolved
frontend/projects/webapp/src/app/feature/budget-templates/details/template-detail.ts
Outdated
Show resolved
Hide resolved
frontend/projects/webapp/src/app/core/budget-template/budget-templates-api.ts
Show resolved
Hide resolved
|
Review PR 373 - ziflux SWR migration. 0 bloquant - 2 importants - 1 suggestion. Verdict: CHANGES REQUESTED. Details dans les commentaires inline. |
frontend/projects/webapp/src/app/feature/budget/budget-list/budget-list-store.ts
Outdated
Show resolved
Hide resolved
frontend/projects/webapp/src/app/feature/current-month/services/dashboard-store.ts
Outdated
Show resolved
Hide resolved
|
Review PR 373 Résumé : 0 bloquant(s) | 1 important(s) | 2 suggestion(s) Important : budget-list-store.ts:L92 — Régression UX sur selectedYear La nouvelle computation ne vérifie plus si l'année retenue existe dans plannedYears. Au premier chargement (pas de previous), selectedYear() retourne new Date().getFullYear() même si aucun budget n'existe pour l'année courante. Le composant s'en sort via Math.max(0, findIndex(...)) = 0, mais le signal diverge de l'onglet affiché jusqu'à la prochaine interaction utilisateur. (confiance: 90%) Suggestion 1 : dashboard-store.ts:L342 return data! masque un null derrière un cast TypeScript sans bénéfice runtime. L'ancienne forme return data était plus honnête avec le compilateur. (confiance: 85%) Suggestion 2 : budget-templates-api.ts getDetail() fait 2 requêtes parallèles en cache miss (getById + getTemplateTransactions). Pattern hérité, désormais visible en core. Pas bloquant. (confiance: 82%) Points positifs Suppression de 1900 lignes de code maison (DataCache, BudgetInvalidationService, TemplateTotalsCalculator). Migration cachedResource / cachedMutation cohérente dans tous les stores. BudgetTemplatesApi déplacé correctement en core (partagé entre plusieurs features, providedIn root). deleteTemplate avec cachedMutation et rollback optimiste bien implémenté. Nouveau calculateTotalExpenseOnly dans shared/budget-formulas.ts réutilisable iOS et Angular. Internationalisation des chaînes hardcodées dans budget-list-page.ts. lines.asReadonly() dans TemplateLineStore : bonne encapsulation. Verdict : APPROVE — La régression selectedYear est mineure (pas de crash, auto-récupération) mais mérite un correctif simple. |
frontend/projects/webapp/src/app/core/budget-template/budget-templates-api.ts
Outdated
Show resolved
Hide resolved
frontend/projects/webapp/src/app/feature/current-month/services/dashboard-store.ts
Show resolved
Hide resolved
frontend/projects/webapp/src/app/feature/budget/budget-list/budget-list-store.ts
Show resolved
Hide resolved
|
test review comment |
|
Review PR 373 Resume : 0 bloquant - 1 important - 2 suggestions IMPORTANT budget-templates-api.ts L61-69 - createFromOnboarding n invalide pas le cache (confiance 95 pct) Toutes les autres mutations de cette PR ajoutent .pipe(tap invalidate templates) : create, update, updateTemplateLines, bulkOperationsTemplateLines, delete. La methode createFromOnboarding a ete oubliee. Apres onboarding, le cache de la liste des templates reste perime jusqu a 5 min et le template fraichement cree peut ne pas apparaitre dans l UI. SUGGESTIONS
POINTS POSITIFS Migration ngx-ziflux coherente sur tous les stores. Verdict : CHANGES REQUESTED (1 important a corriger) |
frontend/projects/webapp/src/app/feature/budget-templates/details/template-detail.ts
Outdated
Show resolved
Hide resolved
frontend/projects/webapp/src/app/core/budget-template/budget-templates-api.ts
Outdated
Show resolved
Hide resolved
frontend/projects/webapp/src/app/feature/budget-templates/services/budget-templates-store.ts
Outdated
Show resolved
Hide resolved
.../projects/webapp/src/app/feature/budget-templates/details/services/template-details-store.ts
Show resolved
Hide resolved
Review PR #373 — refactor(cache): complete ziflux SWR migrationRésumé : 0 bloquant(s) — 2 important(s) — 2 suggestion(s) Importants
Suggestions
Points positifs
Verdict : CHANGES REQUESTED — les 2 points importants méritent correction avant merge. |
frontend/projects/webapp/src/app/feature/budget-templates/details/template-detail.ts
Outdated
Show resolved
Hide resolved
neogenz
force-pushed
the
maximedesogus/pul-93-integrer-la-librairie-ziflux-pour-le-cache-swr-des-resources
branch
from
c0de7a0 to
c67324a
Compare
| expect(mockBudgetInvalidation.invalidate).toHaveBeenCalledOnce(); | ||
| expect(mockUserSettingsApi.reload).toHaveBeenCalledOnce(); | ||
| expect(mockBudgetApi.cache.invalidate).toHaveBeenCalledOnce(); | ||
| expect(mockUserSettingsStore.reload).toHaveBeenCalledOnce(); |
There was a problem hiding this comment.
[SUGGESTION] Assertion manquante sur mockBudgetTemplatesApi.cache.invalidate (confiance: 95%)
L'implémentation appelle this.#budgetTemplatesApi.cache.invalidate(['templates']) lors de chaque soft recovery, mais aucun des 4 scénarios "should trigger soft recovery" n'asserte ce comportement. Une régression (suppression de l'invalidation templates) passerait silencieusement.
| expect(mockUserSettingsStore.reload).toHaveBeenCalledOnce(); | |
| expect(mockBudgetApi.cache.invalidate).toHaveBeenCalledOnce(); | |
| expect(mockBudgetTemplatesApi.cache.invalidate).toHaveBeenCalledOnce(); | |
| expect(mockUserSettingsStore.reload).toHaveBeenCalledOnce(); |
| createBudget$: vi.fn().mockReturnValue( | ||
| of({ | ||
| budget: { id: 'new-budget', month: 3, year: 2026 }, | ||
| message: 'Budget créé', |
There was a problem hiding this comment.
[SUGGESTION] Propriété message obsolète dans le mock (confiance: 90%)
CreateBudgetApiResponse.message a été supprimée de l'interface dans cette PR, mais le mock conserve message: 'Budget créé'. Ce n'est pas un bug (la propriété supplémentaire est ignorée), mais cela trompe les lecteurs futurs qui pourraient croire que la réponse inclut toujours un message.
| message: 'Budget créé', | |
| of({ | |
| budget: { id: 'new-budget', month: 3, year: 2026 }, | |
| }), |
| @@ -93,7 +81,7 @@ export class PreloadService { | |||
| // Transform to BudgetDetailsViewModel so the cache entry matches | |||
There was a problem hiding this comment.
[SUGGESTION] Le préchargement de ['budget', 'details', ...] ne bénéficie plus au DashboardStore (confiance: 85%)
Dans l'ancien code, getDashboardData$ faisait une lecture en cascade : il vérifiait ['budget', 'list'] puis ['budget', 'details', budgetId] avant de faire des appels HTTP. Désormais, getDashboardData$ est pure (0 lecture de cache) et DashboardStore cache le résultat final sous ['budget', 'dashboard', month, year].
Résultat : le premier chargement du dashboard génère toujours 2 appels HTTP (/budgets + /budgets/:id/details) même si le PreloadService vient de les pré-charger. Le prefetch de ['budget', 'details', budgetId] ne sert plus qu'au BudgetDetailsStore.
Pour restaurer le bénéfice du préchargement pour le dashboard, le PreloadService devrait aussi prefetch ['budget', 'dashboard', paddedMonth, year] via getDashboardData$(month, year).
| throw error; | ||
| } | ||
| }, | ||
| readonly budgetTemplates = cachedResource({ |
There was a problem hiding this comment.
[Important] budgetTemplates expose un CachedResourceRef mutable directement en propriété publique (confiance: 92%)
CachedResourceRef expose des méthodes mutables (.set(), .update(), .reload()) que n'importe quel composant peut appeler. template-list-page.ts y accède directement via store.budgetTemplates.status(), store.budgetTemplates.value(), etc. — ce qui viole l'encapsulation du store pattern défini dans angular-store-pattern.md.
Le pattern établi dans le projet (cf. BudgetListStore.budgets, BudgetDetailsStore.#budgetDetailsResource) est de passer le resource en privé (#) et d'exposer uniquement des computed() en lecture seule :
| readonly budgetTemplates = cachedResource({ | |
| readonly #budgetTemplates = cachedResource({ |
Puis exposer des selectors :
readonly templates = computed(() => this.#budgetTemplates.value() ?? []);
readonly isLoading = this.#budgetTemplates.isInitialLoading;
readonly error = this.#budgetTemplates.error;
readonly status = computed(() => this.#budgetTemplates.status());
Review PR 373 - Migration ngx-zifluxResume : 0 bloquant - 0 important - 3 suggestions Suggestions
Points positifs
Verdict : APPROVE |
| this.#api.cache.clear(); | ||
| } | ||
|
|
||
| async #loadSettings(): Promise<UserSettings> { |
There was a problem hiding this comment.
[Important] Le catch silencieux neutralise le signal error et masque les échecs réseau aux utilisateurs (confiance: 88%)
Le try/catch intercepte toutes les erreurs et retourne { payDayOfMonth: null } — ce qui fait que la resource voit une résolution réussie. En conséquence :
this.#settingsResource.error()ne sera jamais populé, même lors d'un échec 500 ou réseau.- La propriété publique
readonly error = this.#settingsResource.errorest du dead code — aucun consommateur ne verra jamais d'erreur. - La
settings-page.tsne peut pas afficher d'état d'erreur pour les settings.
Si l'intention est une dégradation silencieuse (soft failure acceptable), le signal error doit être supprimé et la property settings doit gérer null explicitement. Si une erreur doit être remontée, il faut laisser rethrow et laisser la resource gérer l'état d'erreur :
| async #loadSettings(): Promise<UserSettings> { | |
| async #loadSettings(): Promise<UserSettings> { | |
| const response = await firstValueFrom(this.#api.getSettings$()); | |
| return response.data; | |
| } |
|
|
||
| // Sort to put default template first | ||
| return [...viewModels].sort((a, b) => { | ||
| return viewModels.toSorted((a, b) => { |
There was a problem hiding this comment.
[Suggestion] La logique de tri duplique celle déjà présente dans TemplateStore.sortedTemplates (confiance: 83%)
TemplateStore.sortedTemplates (lignes 57-64 du store) trie déjà par isDefault desc puis createdAt desc. Ici, templateViewModels retrie par isDefault uniquement, ce qui peut diverger si le tri du store évolue. Il suffirait de construire les view models à partir de this.templateStore.sortedTemplates() plutôt que de templates() pour éliminer la duplication.
|
Review PR 373 - Resume: 0 bloquant - 2 importants - 1 suggestion. Voir commentaires inline pour details. Verdict: CHANGES REQUESTED |
Review complete - DetailsVerdict : CHANGES REQUESTED 0 bloquant - 2 importants - 1 suggestion Importants :
Suggestion :
Points positifs : suppression de ~1900 lignes custom (DataCache, BudgetInvalidationService, TemplateTotalsCalculator), cachedMutation pattern uniforme dans BudgetDetailsStore, hasBudgetGuard simplifie, PreloadService reduit grace a cache.prefetch(). |
…expose errors
Remove error-swallowing try/catch that prevented error() signal from working. Replace
private #loadSettings() method with direct Observable loader using map() operator,
matching the pattern used by other 6 stores (BudgetListStore, TemplateStore, etc).
This restores proper SWR lifecycle where errors propagate to cachedResource.error()
while stale data is preserved in value() — the fallback { payDayOfMonth: null } was
redundant and masked failures.
Add UserSettingsStore spec with error propagation tests to validate the fix.
| error instanceof Error | ||
| ? error.message | ||
| : 'Une erreur est survenue lors de la sauvegarde'; | ||
| : this.#transloco.translate('template.saveError'); |
There was a problem hiding this comment.
[IMPORTANT] catch mort — mutate() ne rejette jamais (confiance: 85%)
Selon la doc ziflux intégrée dans cette PR : mutate() never rejects — les erreurs vont dans le signal error. Ce bloc catch ne peut donc jamais s'exécuter. La logique est déjà correctement gérée par le if (!response) aux lignes 154-161 via this.#bulkSaveMutation.error().
Le catch crée une fausse impression de protection et duplique le code d'erreur inutilement.
Fix : Supprimer le try/catch et conserver un try/finally nu pour le reset de isLoading :
async saveChanges(...): Promise<SaveResult> {
// ...
this.#isLoading.set(true);
this.#error.set(null);
try {
const response = await this.#bulkSaveMutation.mutate({ templateId, operations });
if (!response) { /* handle via mutation.error() */ }
// ...
} finally {
this.#isLoading.set(false);
}
}
Review PR #373Résumé : 0 bloquant(s) · 1 important · 0 suggestion Importants
Points positifs
Verdict : CHANGES REQUESTED (1 important à corriger avant merge) |
…save Merge #convertToExistingLine and #syncWithServerData into single #toCleanLine. Replace mutable index counter with queue, use Map for O(1) update lookups. Fix early return to include updatedLines/deletedIds/propagation.
| // Slow path: cache miss - fetch from API | ||
| // Router automatically shows loading indicator during async operation | ||
| try { | ||
| const hasBudget = await firstValueFrom(budgetApi.checkBudgetExists$()); |
There was a problem hiding this comment.
[Suggestion] Résultat du fallback API non mis en cache (confiance: 85%)
Quand getCachedBudgetExists() retourne null (cache vide) et que checkBudgetExists$() est appelé, le résultat n'est pas écrit dans cache. Si le guard est activé plusieurs fois avant que le PreloadService ait rempli ['budget', 'list'], chaque activation déclenchera un appel HTTP.
En pratique, le PreloadService remplit le cache au login, donc ce cas est rare. Si l'on veut l'éliminer complètement :
| const hasBudget = await firstValueFrom(budgetApi.checkBudgetExists$()); | |
| try { | |
| const hasBudget = await firstValueFrom(budgetApi.checkBudgetExists$()); | |
| return hasBudget ? true : redirectToCompleteProfile(); |
| await this.#api.deleteAccount(); | ||
| } | ||
|
|
||
| reset(): void { |
There was a problem hiding this comment.
[Suggestion] reset() vide le cache de UserSettingsApi mais pas l'état local du resource (confiance: 82%)
reset() appelle this.#api.cache.clear(), ce qui vide le DataCache de UserSettingsApi. Cependant, #settingsResource (un cachedResource) peut conserver son value() en mémoire jusqu'au prochain rechargement. Sur un logout, l'utilisateur précédent pourrait voir brièvement les données du précédent session si le composant est réutilisé.
Vérifier si cachedResource expose un reload() ou un set(null) pour vider l'état du resource en même temps que le cache.
|
|
||
| store = TestBed.inject(UserSettingsStore); | ||
| store.reload(); | ||
|
|
There was a problem hiding this comment.
[Important] Tests utilisant setTimeout(resolve, 100) au lieu de vi.waitFor ou TestBed.flushEffects() (confiance: 88%)
Les tests de UserSettingsStore utilisent await new Promise((resolve) => setTimeout(resolve, 100)) pour attendre le chargement asynchrone. Cela rend les tests fragiles (dépendent du timing réel) et lents. Le pattern établi dans le projet est d'utiliser await vi.waitFor(() => expect(...)) ou TestBed.flushEffects() suivi de vi.waitFor.
Ce pattern setTimeout(100) est utilisé aux lignes 74, 86, 97, 103 de ce fichier et dans template-details-store.spec.ts.
Review PR #373Résumé : 0 bloquant — 1 important — 2 suggestions BloquantsAucun Importants
Suggestions
Points positifs
Verdict : APPROVE — la PR est mergeable. Le point important sur les |
- Replace manual #isLoading signal with #bulkSaveMutation.isPending (TemplateLineStore) - Use .asReadonly() instead of computed(() => signal()) wrappers - Derive netBalance from totals instead of recomputing (TemplateDetailsStore) - Remove unused transactions alias, update consumers - Wrap deleteTemplate mutation in method, expose deleteTemplateError signal (BudgetTemplatesStore) - Fix selectedTemplate to derive from resource instead of stale copy - Simplify toggleCheck guard in BudgetDetailsStore - Make isSettingsLoading private (DashboardStore) - Standardize error sentinel from '' to null (CompleteProfileStore) - Remove dead loadSingleTemplateTotals method (TemplateStore) - Update test mocks to match new APIs All 1590 tests pass. Adversarial review: 0 findings.
| expect(mockBudgetInvalidation.invalidate).toHaveBeenCalledOnce(); | ||
| expect(mockUserSettingsApi.reload).toHaveBeenCalledOnce(); | ||
| expect(mockBudgetApi.cache.invalidate).toHaveBeenCalledOnce(); | ||
| expect(mockUserSettingsStore.reload).toHaveBeenCalledOnce(); |
There was a problem hiding this comment.
[IMPORTANT] Assertion manquante : budgetTemplatesApi.cache.invalidate non testée (confiance: 97%)
Le service appelle bien this.#budgetTemplatesApi.cache.invalidate(['templates']) en production (page-lifecycle-recovery.service.ts:193), mais aucun test n'assert cet appel. Si cette ligne est accidentellement supprimée, les templates restent stale indéfiniment après un retour sur l'onglet — sans que les tests échouent.
| expect(mockUserSettingsStore.reload).toHaveBeenCalledOnce(); | |
| expect(mockBudgetApi.cache.invalidate).toHaveBeenCalledOnce(); | |
| expect(mockBudgetTemplatesApi.cache.invalidate).toHaveBeenCalledOnce(); | |
| expect(mockUserSettingsStore.reload).toHaveBeenCalledOnce(); |
| this.#budgetApi.cache.invalidate(['budget']); | ||
| this.#budgetApi.cache.set(['budget', 'list'], [budget]); |
There was a problem hiding this comment.
[SUGGESTION] Manipulation directe du cache — incohérence avec le pattern cachedMutation (confiance: 85%)
Le reste de la codebase utilise cachedMutation avec invalidateKeys pour orchestrer invalidation + mise à jour du cache. Ici, on manipule cache directement depuis un service de core.
L'ordre actuel est correct (invalidate → set : les entrées stale sont immédiatement écrasées par la donnée fraîche), mais le pattern diffère du reste. Si la logique évolue (ex: race condition entre l'appel set et un cachedResource déjà en refetch), le comportement devient imprévisible.
Alternative : si ProfileSetupService avait accès à un store (ou si BudgetListStore exposait une méthode addBudget), on pourrait déléguer la mise à jour du cache optimiste à cachedMutation. À défaut, un commentaire expliquant pourquoi ce pattern direct est utilisé ici serait utile.
| "totalIncome": "Revenus total :", | ||
| "totalExpenses": "Dépenses total :", |
There was a problem hiding this comment.
[SUGGESTION] Accord grammatical incorrect en français (confiance: 95%)
"Revenus total :" → "total" ne s'accorde pas avec "revenus" (masculin pluriel). "Dépenses total :" → idem, "total" ne s'accorde pas avec "dépenses" (féminin pluriel).
| "totalIncome": "Revenus total :", | |
| "totalExpenses": "Dépenses total :", | |
| "totalIncome": "Total des revenus :", | |
| "totalExpenses": "Total des dépenses :", |
1 participant
Summary
• Completes ziflux SWR cache integration across budget and template stores
• Removes custom cache infrastructure (DataCache, budget-invalidation service)
• Simplifies budget-details store with ziflux signal management
• Refactors template stores to use ziflux cachedResource patterns
• Consolidates budget calculations into shared BudgetFormulas utility
Changes
Type
refactor