chore(balance): suggestions post-merge des reviews du 2026-05-01 (#175 #176 #178 #179) #187

New issue

Closed

opened 2026-05-02 19:29:16 +00:00 by maximus · 0 comments

maximus commented

2026-05-02 19:29:16 +00:00

Owner

Suivi post-merge — milestone overnight-2026-05-01-bilan-anomalies-174

Améliorations identifiées par les 4 reviews adversariales (/pr-review) sur les PRs #182, #183, #184, #185 — toutes APPROVE, 12 suggestions non bloquantes capturées ici pour traitement ultérieur.

Statut post-/analyze (2026-05-03)

ID	Sujet	Statut	Localisation actuelle	Complexite
S1	Modal StarterAccounts sur nouveaux profils	A faire (pre-seed)	`consolidated_schema.sql` (table `user_preferences`)	Trivial
S2	Empty states empiles sur `/balance`	A faire	`src/pages/BalancePage.tsx:177-266`	Petit
S3	Collision starter accounts in-txn	A faire (re-check)	`src/services/balance.service.ts:515-560` (`proposeStarterAccounts`)	Petit
S4	`archived_at` non filtre dans collisions	A faire	`src/services/balance.service.ts:482-503` (`getStarterCollisions`)	Trivial
S5	`<Step>` recoit `t` en prop	A faire	`src/components/balance/BalanceOnboardingCard.tsx:115-127`	Trivial
S6	Factorisation `collision_tooltip`	DEJA DONE	`StarterAccountsModal.tsx:151,169` (1 seul `t()` reutilise)	N/A
S7	Doctest cassant `return_calculator.rs`	A faire	`src-tauri/src/commands/return_calculator.rs:12-13`	Trivial
S8	Tests vitest assertant sur SQL strings	DEFERRED	Backlog technique, non urgent	Moyen
S9	`kindByAccountId` commentaire mort	OBSOLETE	Fonction non trouvee dans le codebase actuel	N/A

A faire dans cette PR : S1, S2, S3, S4, S5, S7 (6 suggestions, lot unique).
Hors scope : S6 (deja fait), S8 (backlog), S9 (obsolete).

Decisions retenues

S1 — Pre-seed balance_starter_proposed dans consolidated_schema.sql (solution data-level, nouveaux profils ne verront jamais le modal)
S3 — Re-check dans la transaction (SELECT EXISTS WHERE name=? AND balance_category_id=? avant chaque INSERT, dans le meme BEGIN). Pas de migration SQL.
Decoupage — 1 PR multi-commit chore(balance): post-merge cleanup #182-#185 avec ~6 commits atomiques.

Plan de commits

fix(balance): exclude archived accounts from starter collisions (S4) — ajoute archived_at IS NULL dans getStarterCollisions
fix(balance): re-check collisions in-transaction in proposeStarterAccounts (S3) — defense in-depth in-txn
fix(balance): pre-seed balance_starter_proposed in schema for new profiles (S1) — migration v12 + update consolidated_schema.sql
fix(balance): hide period/chart/table when no accounts on /balance (S2) — wrapper sous le meme guard que BalanceOnboardingCard
refactor(balance): use useTranslation directly in BalanceOnboardingCard.Step (S5) — alignement pattern
fix(rust): convert formula doctest fence to text in return_calculator (S7) — \``text` au lieu de bloc Rust

Chaque commit reference l'ID de suggestion (S1-S7) pour tracabilite.

Surface de test

Tests vitest existants : balance.service.test.ts (couvre getStarterCollisions + proposeStarterAccounts), BalanceOnboardingCard.test.tsx, useBalanceOverview.test.ts
Tests a ajouter :
- S4 : test que getStarterCollisions ignore les comptes archives
- S3 : test que proposeStarterAccounts ne cree pas de doublon meme si appele deux fois en parallele (mock conflict)
S2, S5 : pas de test (UI restructuring trivial)
S7 : cargo test --doc doit passer apres le fix

Criteres d'acceptation

getStarterCollisions filtre archived_at IS NULL
proposeStarterAccounts re-verifie chaque collision avant INSERT (in-txn)
Migration v12 pre-seede balance_starter_proposed pour nouveaux profils + consolidated_schema.sql aligne
/balance ne rend ni period selector ni chart ni accounts table quand accountsCount === 0
BalanceOnboardingCard.Step utilise useTranslation() directement
cargo test --doc --manifest-path src-tauri/Cargo.toml passe (doctest formula bloc en text)
CHANGELOG (FR + EN) sous ### Fixed / ### Corrigé (1 entree resumant les 6 fixes)
npm test + npm run build + cargo check verts

Complexite estimee

Petit — 6 changements coherents thematiquement, ~30 min effectif + CI.

Suggestions actionnables originales (priorité haute → basse)

UX — friction sur nouveaux profils (PR #185 suggestion 1)

Le modal StarterAccountsModal s'ouvre sur les nouveaux profils même quand les 4 starters sont déjà seedés (collisions complètes). Le commentaire BalancePage.tsx:597-600 est trompeur. Fix proposé : pré-seeder user_preferences.balance_starter_proposed dans consolidated_schema.sql (avec accepted: [<les 4 starter ids>] et shown_at: <date du seed>) OU auto-fermer le modal sur mount si toutes les options sont en collision.

UX — empty state empilés (PR #184 suggestion 1)

Quand accountsCount === 0, le sélecteur de période + le chart + l'accounts table se rendent quand même sous la carte d'onboarding, créant 3 messages d'état vide consécutifs. Wrapper l'ensemble du bloc dans le même guard que BalanceOnboardingCard simplifierait l'écran.

Defense en profondeur — collision starter accounts (PR #185 suggestion 2)

proposeStarterAccounts ne re-vérifie pas les collisions à l'intérieur de la transaction. Pas de contrainte UNIQUE sur (name, balance_category_id) non plus. Ajouter un re-check dans la BEGIN ou poser la contrainte UNIQUE.

Bug latent — comptes archivés ignorés (PR #185 suggestion 3)

getStarterCollisions n'exclut pas les comptes avec archived_at IS NOT NULL. Un compte archivé avec le même nom bloque la re-création même si l'utilisateur a archivé volontairement. Soit filtrer archived_at IS NULL, soit documenter le comportement.

Cosmétique — pattern divergent (PR #184 suggestion 3)

Le sous-composant <Step> reçoit t: TFunction en prop alors que les autres composants du repo appellent useTranslation() directement. Aligner sur le pattern majoritaire.

Cosmétique — code dupliqué (PR #185 suggestion 4)

t("balance.starters.collision_tooltip") peut être factorisé en variable locale ou dans une fonction utility si réutilisé.

Doctest pre-existing hors scope (note worker #176)

cargo test échoue sur un doctest dans src-tauri/src/commands/return_calculator.rs:12-13 (formule pseudo-code dans un /// rustdoc essaie de compiler). Pre-existing depuis le commit 531624b. Solution : convertir le bloc en text ou ignore rustdoc fence.

Tests — asserts SQL string-based (PR #182 suggestions)

Plusieurs tests vitest assertent sur des fragments de chaîne SQL (expect(sql).toMatch(/ROW_NUMBER/)). Pattern fragile mais cohérent avec le reste du fichier. Migration progressive vers des asserts de comportement (résultat de la query) plutôt que de syntaxe quand opportunité se présente.

Doc — commentaires (PR #183 suggestions)

Commentaire mort sur kindByAccountId à nettoyer
Doc du ROLLBACK swallow (try/catch défensif autour du ROLLBACK lui-même) à expliciter

Source

Daily report : reports/DAILY-REPORT-2026-05-01.md
Decisions log : reports/autopilot-2026-05-01/decisions-log.md
Reviews : commentaires #8701 (PR #182), #8703 (PR #183), #8707 (PR #184), #8709 (PR #185)

# Suivi post-merge — milestone overnight-2026-05-01-bilan-anomalies-174 Améliorations identifiées par les 4 reviews adversariales (`/pr-review`) sur les PRs #182, #183, #184, #185 — toutes APPROVE, 12 suggestions non bloquantes capturées ici pour traitement ultérieur. --- ## Statut post-/analyze (2026-05-03) | ID | Sujet | Statut | Localisation actuelle | Complexite | |---|---|---|---|---| | **S1** | Modal StarterAccounts sur nouveaux profils | A faire (pre-seed) | `consolidated_schema.sql` (table `user_preferences`) | Trivial | | **S2** | Empty states empiles sur `/balance` | A faire | `src/pages/BalancePage.tsx:177-266` | Petit | | **S3** | Collision starter accounts in-txn | A faire (re-check) | `src/services/balance.service.ts:515-560` (`proposeStarterAccounts`) | Petit | | **S4** | `archived_at` non filtre dans collisions | A faire | `src/services/balance.service.ts:482-503` (`getStarterCollisions`) | Trivial | | **S5** | `<Step>` recoit `t` en prop | A faire | `src/components/balance/BalanceOnboardingCard.tsx:115-127` | Trivial | | **S6** | Factorisation `collision_tooltip` | DEJA DONE | `StarterAccountsModal.tsx:151,169` (1 seul `t()` reutilise) | N/A | | **S7** | Doctest cassant `return_calculator.rs` | A faire | `src-tauri/src/commands/return_calculator.rs:12-13` | Trivial | | **S8** | Tests vitest assertant sur SQL strings | DEFERRED | Backlog technique, non urgent | Moyen | | **S9** | `kindByAccountId` commentaire mort | OBSOLETE | Fonction non trouvee dans le codebase actuel | N/A | **A faire dans cette PR** : S1, S2, S3, S4, S5, S7 (6 suggestions, lot unique). **Hors scope** : S6 (deja fait), S8 (backlog), S9 (obsolete). --- ## Decisions retenues - **S1** — **Pre-seed** `balance_starter_proposed` dans `consolidated_schema.sql` (solution data-level, nouveaux profils ne verront jamais le modal) - **S3** — **Re-check dans la transaction** (`SELECT EXISTS WHERE name=? AND balance_category_id=?` avant chaque INSERT, dans le meme BEGIN). Pas de migration SQL. - **Decoupage** — **1 PR multi-commit** `chore(balance): post-merge cleanup #182-#185` avec ~6 commits atomiques. ## Plan de commits 1. `fix(balance): exclude archived accounts from starter collisions (S4)` — ajoute `archived_at IS NULL` dans `getStarterCollisions` 2. `fix(balance): re-check collisions in-transaction in proposeStarterAccounts (S3)` — defense in-depth in-txn 3. `fix(balance): pre-seed balance_starter_proposed in schema for new profiles (S1)` — migration v12 + update consolidated_schema.sql 4. `fix(balance): hide period/chart/table when no accounts on /balance (S2)` — wrapper sous le meme guard que `BalanceOnboardingCard` 5. `refactor(balance): use useTranslation directly in BalanceOnboardingCard.Step (S5)` — alignement pattern 6. `fix(rust): convert formula doctest fence to text in return_calculator (S7)` — `\`\`\`text` au lieu de bloc Rust Chaque commit reference l'ID de suggestion (S1-S7) pour tracabilite. ## Surface de test - Tests vitest existants : `balance.service.test.ts` (couvre `getStarterCollisions` + `proposeStarterAccounts`), `BalanceOnboardingCard.test.tsx`, `useBalanceOverview.test.ts` - Tests a ajouter : - S4 : test que `getStarterCollisions` ignore les comptes archives - S3 : test que `proposeStarterAccounts` ne cree pas de doublon meme si appele deux fois en parallele (mock conflict) - S2, S5 : pas de test (UI restructuring trivial) - S7 : `cargo test --doc` doit passer apres le fix ## Criteres d'acceptation - [ ] `getStarterCollisions` filtre `archived_at IS NULL` - [ ] `proposeStarterAccounts` re-verifie chaque collision avant INSERT (in-txn) - [ ] Migration v12 pre-seede `balance_starter_proposed` pour nouveaux profils + `consolidated_schema.sql` aligne - [ ] `/balance` ne rend ni period selector ni chart ni accounts table quand `accountsCount === 0` - [ ] `BalanceOnboardingCard.Step` utilise `useTranslation()` directement - [ ] `cargo test --doc --manifest-path src-tauri/Cargo.toml` passe (doctest formula bloc en `text`) - [ ] CHANGELOG (FR + EN) sous `### Fixed` / `### Corrigé` (1 entree resumant les 6 fixes) - [ ] `npm test` + `npm run build` + `cargo check` verts ## Complexite estimee **Petit** — 6 changements coherents thematiquement, ~30 min effectif + CI. --- ## Suggestions actionnables originales (priorité haute → basse) ### UX — friction sur nouveaux profils (PR #185 suggestion 1) Le modal `StarterAccountsModal` s'ouvre sur les nouveaux profils même quand les 4 starters sont déjà seedés (collisions complètes). Le commentaire `BalancePage.tsx:597-600` est trompeur. Fix proposé : pré-seeder `user_preferences.balance_starter_proposed` dans `consolidated_schema.sql` (avec `accepted: [<les 4 starter ids>]` et `shown_at: <date du seed>`) OU auto-fermer le modal sur mount si toutes les options sont en collision. ### UX — empty state empilés (PR #184 suggestion 1) Quand `accountsCount === 0`, le sélecteur de période + le chart + l'accounts table se rendent quand même sous la carte d'onboarding, créant 3 messages d'état vide consécutifs. Wrapper l'ensemble du bloc dans le même guard que `BalanceOnboardingCard` simplifierait l'écran. ### Defense en profondeur — collision starter accounts (PR #185 suggestion 2) `proposeStarterAccounts` ne re-vérifie pas les collisions à l'intérieur de la transaction. Pas de contrainte UNIQUE sur `(name, balance_category_id)` non plus. Ajouter un re-check dans la BEGIN ou poser la contrainte UNIQUE. ### Bug latent — comptes archivés ignorés (PR #185 suggestion 3) `getStarterCollisions` n'exclut pas les comptes avec `archived_at IS NOT NULL`. Un compte archivé avec le même nom bloque la re-création même si l'utilisateur a archivé volontairement. Soit filtrer `archived_at IS NULL`, soit documenter le comportement. ### Cosmétique — pattern divergent (PR #184 suggestion 3) Le sous-composant `<Step>` reçoit `t: TFunction` en prop alors que les autres composants du repo appellent `useTranslation()` directement. Aligner sur le pattern majoritaire. ### Cosmétique — code dupliqué (PR #185 suggestion 4) `t("balance.starters.collision_tooltip")` peut être factorisé en variable locale ou dans une fonction utility si réutilisé. ### Doctest pre-existing hors scope (note worker #176) `cargo test` échoue sur un doctest dans `src-tauri/src/commands/return_calculator.rs:12-13` (formule pseudo-code dans un `///` rustdoc essaie de compiler). Pre-existing depuis le commit 531624b. Solution : convertir le bloc en `text` ou `ignore` rustdoc fence. ### Tests — asserts SQL string-based (PR #182 suggestions) Plusieurs tests vitest assertent sur des fragments de chaîne SQL (`expect(sql).toMatch(/ROW_NUMBER/)`). Pattern fragile mais cohérent avec le reste du fichier. Migration progressive vers des asserts de comportement (résultat de la query) plutôt que de syntaxe quand opportunité se présente. ### Doc — commentaires (PR #183 suggestions) - Commentaire mort sur `kindByAccountId` à nettoyer - Doc du ROLLBACK swallow (try/catch défensif autour du ROLLBACK lui-même) à expliciter ## Source - Daily report : `reports/DAILY-REPORT-2026-05-01.md` - Decisions log : `reports/autopilot-2026-05-01/decisions-log.md` - Reviews : commentaires #8701 (PR #182), #8703 (PR #183), #8707 (PR #184), #8709 (PR #185)