maximus/Simpl-Resultat
1
0
Fork 0
Code Issues 5 Pull requests Projects Releases 23 Packages Wiki Activity Actions

chore(balance): post-merge cleanup of #182-#185 reviews (#187) #195

Merged
maximus merged 7 commits from issue-187-balance-cleanup-post-184-185 into main 2026-05-03 23:42:15 +00:00
Conversation 1 Commits 7 Files changed 8 +160 -99
maximus commented 2026-05-03 20:32:02 +00:00
Owner
Copy link

Fixes #187

Summary

Bundles 6 atomic commits addressing 6 of the 9 suggestions raised during the adversarial reviews of PRs #182, #183, #184 and #185. The 3 remaining suggestions are out of scope for this PR:

  • S6 (factorisation collision_tooltip) — already done in StarterAccountsModal.tsx, no action needed.
  • S8 (vitest assertions on raw SQL strings) — deferred to backlog, opportunistic refactor.
  • S9 (kindByAccountId dead comment) — function not found in the current codebase, likely cleaned up earlier.

Commits

# Suggestion What
1 S4 getStarterCollisions filters archived_at IS NULL so re-creating a voluntarily archived starter is no longer blocked.
2 S3 proposeStarterAccounts re-checks each (name, balance_category_id) collision in-transaction before INSERT (defense-in-depth). Skips silently on hit, no UNIQUE constraint added.
3 S1 consolidated_schema.sql pre-seeds balance_starter_proposed so brand-new profiles never see the briefly-empty StarterAccountsModal.
4 S2 /balance hides period selector, evolution chart and accounts table while the empty-state onboarding card is shown — avoids three stacked empty messages under the card.
5 S5 BalanceOnboardingCard.Step uses useTranslation() directly instead of receiving t as a prop — aligns with the project's majority pattern.
6 S7 return_calculator.rs Modified Dietz formula doc block wrapped in a text fence so cargo test --doc no longer fails to compile pseudo-math as Rust.

Tests

  • Updated 2 existing tests in StarterAccountsModal.test.tsx (atomic insert + rollback) to mock the new in-txn collision SELECT call sequence (S3).
  • Added 1 test in StarterAccountsModal.test.tsx: getStarterCollisions excludes archived accounts via SQL filter (S4).
  • Added 1 test in StarterAccountsModal.test.tsx: proposeStarterAccounts skips silently when in-txn collision check finds an existing account (S3).
  • No test added for S2 (UI restructure with no logic change).
  • No test added for S5 (refactor, type system covers correctness).
  • No test added for S7 — cargo test --doc itself is the test (now passes with 0 doctests instead of 9 compile errors).

Verification

  • npm test (vitest) — 227 test files, 3722 tests passed (3720 + 2 new)
  • npm run build (tsc + vite) — built in 6.60s
  • cargo check --manifest-path src-tauri/Cargo.toml --all-targets — clean
  • cargo test --doc --manifest-path src-tauri/Cargo.toml — 0 passed; 0 failed; 0 ignored (was 9 errors before)

Decisions retained from /analyze

  • S1: pre-seed pref in consolidated_schema.sql (data-level invariant). Existing profiles dismiss once on first /balance visit (current behaviour, no migration needed).
  • S3: in-transaction re-check (no migration v12 with UNIQUE constraint). Cheaper, no rollback risk on existing prod databases.
  • Scope: 1 PR multi-commit instead of 5 separate PRs (cohesion thematique sur le post-merge balance).

Test plan (manual smoke)

  • Create new profile → /balance should land on onboarding card without flashing the StarterAccountsModal (S1)
  • On a profile with no accounts → only the onboarding card shows, no period selector / chart / table below (S2)
  • Archive a starter account, then re-open StarterAccountsModal → the row is no longer disabled (S4)
  • Build + tests verts in CI
Fixes #187 ## Summary Bundles 6 atomic commits addressing 6 of the 9 suggestions raised during the adversarial reviews of PRs #182, #183, #184 and #185. The 3 remaining suggestions are out of scope for this PR: - **S6** (factorisation `collision_tooltip`) — already done in `StarterAccountsModal.tsx`, no action needed. - **S8** (vitest assertions on raw SQL strings) — deferred to backlog, opportunistic refactor. - **S9** (`kindByAccountId` dead comment) — function not found in the current codebase, likely cleaned up earlier. ## Commits | # | Suggestion | What | |---|---|---| | 1 | **S4** | `getStarterCollisions` filters `archived_at IS NULL` so re-creating a voluntarily archived starter is no longer blocked. | | 2 | **S3** | `proposeStarterAccounts` re-checks each `(name, balance_category_id)` collision in-transaction before INSERT (defense-in-depth). Skips silently on hit, no UNIQUE constraint added. | | 3 | **S1** | `consolidated_schema.sql` pre-seeds `balance_starter_proposed` so brand-new profiles never see the briefly-empty StarterAccountsModal. | | 4 | **S2** | `/balance` hides period selector, evolution chart and accounts table while the empty-state onboarding card is shown — avoids three stacked empty messages under the card. | | 5 | **S5** | `BalanceOnboardingCard.Step` uses `useTranslation()` directly instead of receiving `t` as a prop — aligns with the project's majority pattern. | | 6 | **S7** | `return_calculator.rs` Modified Dietz formula doc block wrapped in a `text` fence so `cargo test --doc` no longer fails to compile pseudo-math as Rust. | ## Tests - **Updated** 2 existing tests in `StarterAccountsModal.test.tsx` (atomic insert + rollback) to mock the new in-txn collision SELECT call sequence (S3). - **Added** 1 test in `StarterAccountsModal.test.tsx`: `getStarterCollisions` excludes archived accounts via SQL filter (S4). - **Added** 1 test in `StarterAccountsModal.test.tsx`: `proposeStarterAccounts` skips silently when in-txn collision check finds an existing account (S3). - No test added for S2 (UI restructure with no logic change). - No test added for S5 (refactor, type system covers correctness). - No test added for S7 — `cargo test --doc` itself is the test (now passes with 0 doctests instead of 9 compile errors). ## Verification - `npm test` (vitest) — 227 test files, **3722 tests passed** (3720 + 2 new) - `npm run build` (tsc + vite) — `built in 6.60s` - `cargo check --manifest-path src-tauri/Cargo.toml --all-targets` — clean - `cargo test --doc --manifest-path src-tauri/Cargo.toml` — `0 passed; 0 failed; 0 ignored` (was 9 errors before) ## Decisions retained from /analyze - **S1**: pre-seed pref in `consolidated_schema.sql` (data-level invariant). Existing profiles dismiss once on first /balance visit (current behaviour, no migration needed). - **S3**: in-transaction re-check (no migration v12 with `UNIQUE` constraint). Cheaper, no rollback risk on existing prod databases. - **Scope**: 1 PR multi-commit instead of 5 separate PRs (cohesion thematique sur le post-merge balance). ## Test plan (manual smoke) - [ ] Create new profile → /balance should land on onboarding card without flashing the StarterAccountsModal (S1) - [ ] On a profile with no accounts → only the onboarding card shows, no period selector / chart / table below (S2) - [ ] Archive a starter account, then re-open StarterAccountsModal → the row is no longer disabled (S4) - [ ] Build + tests verts in CI
maximus added 6 commits 2026-05-03 20:32:03 +00:00 
getStarterCollisions now filters `archived_at IS NULL` so a starter
account the user voluntarily archived no longer blocks re-creation
through the StarterAccountsModal. Matches the rest-of-codebase
convention (active = is_active=1 AND archived_at IS NULL).

Suggestion S4 from PR #185 review (#187).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds defense-in-depth: each iteration runs a SELECT COUNT(*) WHERE name=?
AND balance_category_id=? AND archived_at IS NULL inside the BEGIN/COMMIT
block, immediately before its INSERT. On a hit, the iteration skips the
INSERT silently and the returned ids array excludes the skipped starter.

Rationale: balance_accounts has no UNIQUE constraint on (name, category)
and the upstream pre-filter (getStarterCollisions) is best-effort. If a
race or a bypass slips a duplicate through, the in-txn check catches it
without surfacing a confusing error to the user.

Existing two tests in StarterAccountsModal.test.tsx updated to mock the
new SELECT call sequence; new test "skips silently when in-txn collision
check finds an existing account" added.

Suggestion S3 from PR #185 review (#187).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Before this commit, a brand-new profile briefly showed the
StarterAccountsModal even though the 4 starter accounts were already
seeded — the modal rendered 4 collision rows with no actionable choice
before being dismissed. Pre-seeding the pref in consolidated_schema.sql
suppresses the modal on first /balance visit for new profiles entirely.

Existing profiles already running the app are unaffected: they handle
the modal once on their first /balance visit (the pref-write happens on
dismiss). No migration is needed for them.

Suggestion S1 from PR #185 review (#187).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Before this commit, /balance rendered the BalanceOnboardingCard plus the
period selector + evolution chart + accounts table whenever the user had
no accounts or no snapshot. The lower three components surfaced their
own empty states, producing 3 stacked "no data" messages under the
onboarding card.

Lifts the (accountsCount, hasAnySnapshot) computation out of the inline
IIFE and uses a single isEmpty branch: empty profiles see only the
BalanceOnboardingCard; populated profiles see the full overview.

No logic change — only JSX restructuring. Tests covering useBalanceOverview
and BalanceOnboardingCard remain green (61 tests passing).

Suggestion S2 from PR #184 review (#187).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The internal Step component received `t: TFunction` as a prop while every
other component in the codebase calls useTranslation() directly at the
top of the function. Aligns with the majority pattern.

Suggestion S5 from PR #184 review (#187).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fix(rust): wrap Modified Dietz formula doc block in text fence (S7)
All checks were successful
PR Check / rust (pull_request) Successful in 23m2s
Details
PR Check / frontend (pull_request) Successful in 2m42s
Details
9dd78b77f2 
Before this commit, `cargo test --doc --manifest-path src-tauri/Cargo.toml`
failed: the indented formula at return_calculator.rs:12-13 was parsed by
rustdoc as a Rust code block and the pseudo-math (`R = ... sum(CF_i)`)
did not compile. Pre-existing since commit 531624b.

Wrapping the formula in an explicit `\`\`\`text` fence tells rustdoc to
render but not compile-test the block. `cargo test --doc` now passes
(0 doctests, no failures).

Also adds the consolidated #187 entry to CHANGELOG.md and CHANGELOG.fr.md
under Fixed/Corrigé summarizing all six fixes (S1, S2, S3, S4, S5, S7) —
S6 already factorized, S8 deferred to backlog, S9 obsolete.

Suggestion S7 from worker note on #176 (#187).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
maximus commented 2026-05-03 20:35:07 +00:00
Author
Owner
Copy link

Review — APPROVE

Solid post-merge cleanup bundling 6 atomic, well-scoped fixes (S1-S5, S7) from the adversarial reviews of #182-#185. Each commit references its suggestion ID for traceability, the changelog entries are dual-language, the SQL change is additive (consolidated_schema.sql only, no existing migration touched), and tests cover both new behaviours (S3 collision skip + S4 archived filter). The defense-in-depth design on S3 (callers pre-filter, in-txn re-check skips silently on hit) and the data-level S1 solution (no migration v12 needed because the bug is benign on existing profiles — once dismissed, never re-shows) are both well-justified.

Checks pass:

  • No secrets, no SQL injection (parameterized queries throughout)
  • i18n respected — no hardcoded user-facing strings introduced
  • consolidated_schema.sql change is additive only, no existing migration modified (compliant with the SQL migration rule)
  • Rust doc-fence change is functionally inert outside cargo test --doc
  • Tests added for the two behaviour changes (S3, S4); S2/S5 are pure refactors and S7 is a doctest fix verified by the doctest runner itself

Suggestions (non-blocking)

  1. BalanceOnboardingCard.tsx:124 — calling useTranslation() inside the Step sub-component is fine and aligns with the project pattern, but creates one extra hook subscription per step (2 today, marginal). If the file ever expands to many Step instances, hoisting t back to the parent could be revisited — current scale doesn't warrant action.

  2. consolidated_schema.sql:296 — the seeded value uses the literal string "seed" for shown_at instead of an ISO timestamp. If a future reader parses shown_at (analytics, migration logic, anywhere new Date(...)), it will silently produce Invalid Date. Today nothing reads it; flagging for if/when it becomes consumed.

  3. Test count claim — PR body reports 3722 tests passed (3720 + 2 new) but the diff adds 2 tests on top of the 1 collision-check assertion that touches existing test scaffolding. Numbers look consistent; just flagging that the suite green can't be re-verified from the diff alone.

Nothing here blocks merge. Good polish work.

## Review — APPROVE Solid post-merge cleanup bundling 6 atomic, well-scoped fixes (S1-S5, S7) from the adversarial reviews of #182-#185. Each commit references its suggestion ID for traceability, the changelog entries are dual-language, the SQL change is additive (`consolidated_schema.sql` only, no existing migration touched), and tests cover both new behaviours (S3 collision skip + S4 archived filter). The defense-in-depth design on S3 (callers pre-filter, in-txn re-check skips silently on hit) and the data-level S1 solution (no migration v12 needed because the bug is benign on existing profiles — once dismissed, never re-shows) are both well-justified. Checks pass: - No secrets, no SQL injection (parameterized queries throughout) - i18n respected — no hardcoded user-facing strings introduced - `consolidated_schema.sql` change is additive only, no existing migration modified (compliant with the SQL migration rule) - Rust doc-fence change is functionally inert outside `cargo test --doc` - Tests added for the two behaviour changes (S3, S4); S2/S5 are pure refactors and S7 is a doctest fix verified by the doctest runner itself ### Suggestions (non-blocking) 1. **`BalanceOnboardingCard.tsx:124`** — calling `useTranslation()` inside the `Step` sub-component is fine and aligns with the project pattern, but creates one extra hook subscription per step (2 today, marginal). If the file ever expands to many `Step` instances, hoisting `t` back to the parent could be revisited — current scale doesn't warrant action. 2. **`consolidated_schema.sql:296`** — the seeded value uses the literal string `"seed"` for `shown_at` instead of an ISO timestamp. If a future reader parses `shown_at` (analytics, migration logic, anywhere `new Date(...)`), it will silently produce `Invalid Date`. Today nothing reads it; flagging for if/when it becomes consumed. 3. **Test count claim** — PR body reports `3722 tests passed (3720 + 2 new)` but the diff adds 2 tests on top of the 1 collision-check assertion that touches existing test scaffolding. Numbers look consistent; just flagging that the suite green can't be re-verified from the diff alone. Nothing here blocks merge. Good polish work.
maximus added 1 commit 2026-05-03 23:41:58 +00:00
Merge remote-tracking branch 'origin/main' into issue-187-balance-cleanup-post-184-185
All checks were successful
PR Check / rust (pull_request) Successful in 22m28s
Details
PR Check / frontend (pull_request) Successful in 2m33s
Details
4095aec453 
# Conflicts:
#	CHANGELOG.fr.md
#	CHANGELOG.md
maximus merged commit d2e65ae1ea into main 2026-05-03 23:42:15 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
autopilot:pending-human

PR produced by /autopilot — awaits human review/merge

  
source:analyste

Créée par le Super-Analyste

  
source:defenseur

Créée par l Escouade Défenseur

  
source:human

Créée par un humain

  
source:medic

Créée par le Medic (auto-détection)

  
status:approved

PR approuvée, prête à merger

  
status:blocked

Bloquée (question, dépendance, clarification)

  
status:in-progress

Un agent (Medic/Dev) travaille dessus

  
status:needs-clarification

Issue blocked by ambiguity — Max needs to clarify before re-running autopilot

  
status:needs-fix

Reviewer demande des corrections

  
status:ready

Analysée et documentée, prête à prendre

  
status:review

PR créée, en attente du Reviewer

  
status:triage

Nouvelle issue, pas encore analysée

  
type:bug

Correction de bug → Medic

  
type:feature

Nouvelle fonctionnalité → Dev

  
type:infra

Config, CI/CD, déploiement → Dev

  
type:refactor

Refactoring / amélioration technique → Dev

  
type:schema

Modèle de données / migrations → Dev

  
type:security

Issue de sécurité → Medic ou Dev

No labels
autopilot:pending-human
source:analyste
source:defenseur
source:human
source:medic
status:approved
status:blocked
status:in-progress
status:needs-clarification
status:needs-fix
status:ready
status:review
status:triage
type:bug
type:feature
type:infra
type:refactor
type:schema
type:security
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: maximus/Simpl-Resultat#195
No description provided.
Delete branch "issue-187-balance-cleanup-post-184-185"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?