From 2eeac78b403cb58d464880eb8463ce306bddd5d1 Mon Sep 17 00:00:00 2001 From: le king fu Date: Sun, 3 May 2026 16:26:23 -0400 Subject: [PATCH 1/6] fix(balance): exclude archived accounts from starter collisions (S4) 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) --- src/components/balance/StarterAccountsModal.test.tsx | 7 +++++++ src/services/balance.service.ts | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/components/balance/StarterAccountsModal.test.tsx b/src/components/balance/StarterAccountsModal.test.tsx index 81486d6..e0482dc 100644 --- a/src/components/balance/StarterAccountsModal.test.tsx +++ b/src/components/balance/StarterAccountsModal.test.tsx @@ -73,6 +73,13 @@ describe("getStarterCollisions", () => { expect(result.has("tfsa")).toBe(false); expect(result.has("cash")).toBe(false); // name "CELI" != "Compte chèque" }); + + it("excludes archived accounts via SQL filter", async () => { + mockSelect.mockResolvedValueOnce([]); + await getStarterCollisions(); + const sql = mockSelect.mock.calls[0][0]; + expect(sql).toMatch(/archived_at IS NULL/); + }); }); describe("proposeStarterAccounts", () => { diff --git a/src/services/balance.service.ts b/src/services/balance.service.ts index 5631466..52c083a 100644 --- a/src/services/balance.service.ts +++ b/src/services/balance.service.ts @@ -487,7 +487,8 @@ export async function getStarterCollisions(): Promise> { `SELECT c.key AS key, a.name AS account_name FROM balance_accounts a INNER JOIN balance_categories c ON c.id = a.balance_category_id - WHERE c.key IN ('cash','tfsa','rrsp','other')` + WHERE c.key IN ('cash','tfsa','rrsp','other') + AND a.archived_at IS NULL` ); const collisions = new Set(); for (const starter of STARTER_ACCOUNTS) { -- 2.45.2 From 8c3a64d17257c5b8913381e72f04f21298bfcfff Mon Sep 17 00:00:00 2001 From: le king fu Date: Sun, 3 May 2026 16:27:16 -0400 Subject: [PATCH 2/6] fix(balance): re-check collisions in-transaction in proposeStarterAccounts (S3) 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) --- .../balance/StarterAccountsModal.test.tsx | 34 ++++++++++++++++--- src/services/balance.service.ts | 20 +++++++++-- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/components/balance/StarterAccountsModal.test.tsx b/src/components/balance/StarterAccountsModal.test.tsx index e0482dc..d56239c 100644 --- a/src/components/balance/StarterAccountsModal.test.tsx +++ b/src/components/balance/StarterAccountsModal.test.tsx @@ -92,10 +92,12 @@ describe("proposeStarterAccounts", () => { it("inserts selected starters atomically and returns their ids", async () => { // BEGIN mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); - // For each starter: SELECT id FROM balance_categories + INSERT + // For each starter: SELECT category id, SELECT in-txn collision check, INSERT mockSelect - .mockResolvedValueOnce([{ id: 11 }]) // cash category - .mockResolvedValueOnce([{ id: 13 }]); // rrsp category + .mockResolvedValueOnce([{ id: 11 }]) // cash category lookup + .mockResolvedValueOnce([{ count: 0 }]) // S3 collision check for cash + .mockResolvedValueOnce([{ id: 13 }]) // rrsp category lookup + .mockResolvedValueOnce([{ count: 0 }]); // S3 collision check for rrsp mockExecute .mockResolvedValueOnce({ rowsAffected: 1, lastInsertId: 100 }) // INSERT cash .mockResolvedValueOnce({ rowsAffected: 1, lastInsertId: 101 }) // INSERT rrsp @@ -110,9 +112,33 @@ describe("proposeStarterAccounts", () => { expect(sqls.filter((s) => /INSERT INTO balance_accounts/.test(s))).toHaveLength(2); }); + it("skips silently when in-txn collision check finds an existing account (S3)", async () => { + // BEGIN + mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); + // First starter "cash": category lookup succeeds, collision check returns count=1 → skip + mockSelect + .mockResolvedValueOnce([{ id: 11 }]) // cash category lookup + .mockResolvedValueOnce([{ count: 1 }]) // S3 collision: cash already exists + // Second starter "rrsp": category lookup + clean collision check + .mockResolvedValueOnce([{ id: 13 }]) // rrsp category lookup + .mockResolvedValueOnce([{ count: 0 }]); // rrsp clean + mockExecute + .mockResolvedValueOnce({ rowsAffected: 1, lastInsertId: 101 }) // INSERT rrsp + .mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); // COMMIT + + const result = await proposeStarterAccounts(["cash", "rrsp"]); + expect(result).toEqual([101]); // only rrsp inserted, cash skipped silently + + const sqls = mockExecute.mock.calls.map((c) => c[0]); + expect(sqls.filter((s) => /INSERT INTO balance_accounts/.test(s))).toHaveLength(1); + expect(sqls).toContain("COMMIT"); // no rollback — skip is normal flow + }); + it("rolls back on insert failure", async () => { mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); // BEGIN - mockSelect.mockResolvedValueOnce([{ id: 11 }]); + mockSelect + .mockResolvedValueOnce([{ id: 11 }]) // cash category + .mockResolvedValueOnce([{ count: 0 }]); // S3 collision check clean mockExecute.mockRejectedValueOnce(new Error("disk full")); mockExecute.mockResolvedValueOnce({ rowsAffected: 0, lastInsertId: 0 }); // ROLLBACK diff --git a/src/services/balance.service.ts b/src/services/balance.service.ts index 52c083a..6401ef0 100644 --- a/src/services/balance.service.ts +++ b/src/services/balance.service.ts @@ -509,9 +509,11 @@ export async function getStarterCollisions(): Promise> { * in BEGIN/COMMIT — on any failure ROLLBACK is issued and the original error * is re-thrown. Returns the inserted account ids in input order. * - * Callers MUST pre-filter `selectedKeys` against `getStarterCollisions()` so - * we never INSERT a duplicate (the table has no UNIQUE on (name, category), - * so collisions would silently create dupes if not guarded upstream). + * Callers SHOULD pre-filter `selectedKeys` against `getStarterCollisions()` + * to keep the UI honest, but each iteration ALSO re-checks for an existing + * (name, category) account inside the transaction and skips silently on a + * hit — a defense-in-depth guard since the table has no UNIQUE constraint + * on (name, balance_category_id). Returned ids exclude any skipped starter. */ export async function proposeStarterAccounts( selectedKeys: string[] @@ -538,6 +540,18 @@ export async function proposeStarterAccounts( `Seeded category '${starter.categoryKey}' missing — expected v9 schema` ); } + // Defense-in-depth: re-check collision in-txn before INSERT so we + // never create a silent duplicate even if the upstream pre-filter + // raced or was bypassed (S3 from PR #185 review). + const existing = await db.select<{ count: number }[]>( + `SELECT COUNT(*) AS count FROM balance_accounts + WHERE name = $1 AND balance_category_id = $2 + AND archived_at IS NULL`, + [starter.name, catRows[0].id] + ); + if ((existing[0]?.count ?? 0) > 0) { + continue; + } const result = await db.execute( `INSERT INTO balance_accounts (balance_category_id, name, currency, is_active) VALUES ($1, $2, 'CAD', 1)`, -- 2.45.2 From 445822b7920bd27ad2ad207d680d3a235777f074 Mon Sep 17 00:00:00 2001 From: le king fu Date: Sun, 3 May 2026 16:27:36 -0400 Subject: [PATCH 3/6] fix(balance): pre-seed balance_starter_proposed pref for new profiles (S1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src-tauri/src/database/consolidated_schema.sql | 6 ++++++ src/pages/BalancePage.tsx | 7 +++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src-tauri/src/database/consolidated_schema.sql b/src-tauri/src/database/consolidated_schema.sql index de73462..83a08a9 100644 --- a/src-tauri/src/database/consolidated_schema.sql +++ b/src-tauri/src/database/consolidated_schema.sql @@ -288,6 +288,12 @@ INSERT OR IGNORE INTO user_preferences (key, value) VALUES ('theme', 'light'); INSERT OR IGNORE INTO user_preferences (key, value) VALUES ('currency', 'EUR'); INSERT OR IGNORE INTO user_preferences (key, value) VALUES ('date_format', 'DD/MM/YYYY'); INSERT OR REPLACE INTO user_preferences (key, value) VALUES ('categories_schema_version', 'v1'); +-- Suppress StarterAccountsModal on first /balance visit for new profiles +-- (Issue #179). The 4 starter accounts are already seeded above, so the +-- modal would only show 4 collision rows with no actionable choice. Pre- +-- writing the pref skips that briefly-empty UX entirely. Suggestion S1 +-- from PR #185 review (#187). +INSERT OR IGNORE INTO user_preferences (key, value) VALUES ('balance_starter_proposed', '{"shown_at":"seed","accepted":[]}'); -- ============================================================================ -- Seed v1 — IPC Statistique Canada-aligned, 3 levels, Canada/Québec diff --git a/src/pages/BalancePage.tsx b/src/pages/BalancePage.tsx index 456ea2b..4a15f33 100644 --- a/src/pages/BalancePage.tsx +++ b/src/pages/BalancePage.tsx @@ -58,10 +58,9 @@ export default function BalancePage() { // Issue #179 — one-shot starter-accounts modal for existing profiles. The // pref `balance_starter_proposed` is written once (confirmed or dismissed), - // so the modal never re-appears. New profiles get the 4 starters seeded - // directly via consolidated_schema.sql and never hit this branch (the - // first /balance visit will write the pref with accepted=[] silently - // since collisions match all 4). + // so the modal never re-appears. New profiles get both the 4 starters AND + // the pref pre-seeded via consolidated_schema.sql, so they never hit this + // branch at all (S1 fix from #187). const [showStarterModal, setShowStarterModal] = useState(false); useEffect(() => { let cancelled = false; -- 2.45.2 From 372a785834c0ab4a89461679d0c6f91d4cc34987 Mon Sep 17 00:00:00 2001 From: le king fu Date: Sun, 3 May 2026 16:28:41 -0400 Subject: [PATCH 4/6] fix(balance): hide period selector, chart and table on empty /balance (S2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/pages/BalancePage.tsx | 170 ++++++++++++++++++++------------------ 1 file changed, 89 insertions(+), 81 deletions(-) diff --git a/src/pages/BalancePage.tsx b/src/pages/BalancePage.tsx index 4a15f33..b184641 100644 --- a/src/pages/BalancePage.tsx +++ b/src/pages/BalancePage.tsx @@ -173,96 +173,104 @@ export default function BalancePage() { )} -
- {(() => { - // Issue #178 — show a 2-step onboarding card while the user has no - // accounts or no snapshots yet. We probe accountsLatest for ANY - // snapshot date so the empty-state guard is independent of the - // active period filter (state.period). - const accountsCount = state.accountsLatest.length; - const hasAnySnapshot = state.accountsLatest.some( - (a) => a.latest_snapshot_date != null - ); - if (accountsCount === 0 || !hasAnySnapshot) { - return ( + {/* Issue #178 — empty-state guard. We probe accountsLatest for ANY + snapshot date so the guard is independent of the active period + filter (state.period). When empty, we render only the onboarding + card — period selector, chart and accounts table would all show + empty states stacked under it (S2 from #187). */} + {(() => { + const accountsCount = state.accountsLatest.length; + const hasAnySnapshot = state.accountsLatest.some( + (a) => a.latest_snapshot_date != null + ); + const isEmpty = accountsCount === 0 || !hasAnySnapshot; + + if (isEmpty) { + return ( +
- ); - } - return ; - })()} +
+ ); + } -
- {/* Period selector */} -
- {PERIOD_OPTIONS.map((p) => ( - - ))} -
+ {PERIOD_OPTIONS.map((p) => ( + + ))} +
- {/* Chart mode toggle */} -
- {(["line", "stacked"] as BalanceChartMode[]).map((mode) => ( - - ))} + {(["line", "stacked"] as BalanceChartMode[]).map((mode) => ( + + ))} +
+
+ + + +
+

+ {t("balance.overview.accountsTitle")} +

+ handleArchiveAccount(acc.account_id)} + onLinkTransfers={(acc) => setLinkTarget(acc)} + /> +
- - - - -
-

- {t("balance.overview.accountsTitle")} -

- handleArchiveAccount(acc.account_id)} - onLinkTransfers={(acc) => setLinkTarget(acc)} - /> -
- + ); + })()} Date: Sun, 3 May 2026 16:29:12 -0400 Subject: [PATCH 5/6] refactor(balance): use useTranslation directly in BalanceOnboardingCard.Step (S5) 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) --- src/components/balance/BalanceOnboardingCard.tsx | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/components/balance/BalanceOnboardingCard.tsx b/src/components/balance/BalanceOnboardingCard.tsx index 3f88402..ff987e9 100644 --- a/src/components/balance/BalanceOnboardingCard.tsx +++ b/src/components/balance/BalanceOnboardingCard.tsx @@ -16,7 +16,6 @@ import { useTranslation } from "react-i18next"; import { Link } from "react-router-dom"; -import type { TFunction } from "i18next"; import { Wallet, FileText, Check, ArrowRight } from "lucide-react"; interface BalanceOnboardingCardProps { @@ -81,7 +80,6 @@ export default function BalanceOnboardingCard({ description={t("balance.onboarding.step1.description")} ctaLabel={t("balance.onboarding.step1.cta")} ctaHref="/balance/accounts" - t={t} /> @@ -112,7 +109,6 @@ interface StepProps { ctaLabel: string; ctaHref: string; disabledHint?: string; - t: TFunction; } function Step({ @@ -124,8 +120,8 @@ function Step({ ctaLabel, ctaHref, disabledHint, - t, }: StepProps) { + const { t } = useTranslation(); const isDone = state === "done"; const isActive = state === "active"; const isDisabled = state === "disabled"; -- 2.45.2 From 9dd78b77f2d98ff4273a42224a63bbc0437b4de8 Mon Sep 17 00:00:00 2001 From: le king fu Date: Sun, 3 May 2026 16:31:29 -0400 Subject: [PATCH 6/6] fix(rust): wrap Modified Dietz formula doc block in text fence (S7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.fr.md | 1 + CHANGELOG.md | 1 + src-tauri/src/commands/return_calculator.rs | 4 +++- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index f27b231..6de2ad2 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -18,6 +18,7 @@ - Bilan : la sauvegarde d'un snapshot utilise désormais une transaction atomique BEGIN/COMMIT et valide toutes les lignes avant toute écriture en BDD, empêchant les snapshots orphelins lorsque la validation échoue. La migration v11 nettoie les orphelins existants (#176). - Bilan : le sélecteur de date sur `/balance/snapshot` se ferme maintenant après la sélection sur Linux (WebKitGTK) au lieu de rester ouvert jusqu'à ce que l'utilisateur appuie sur Échap. Le contournement appelle `blur()` sur le champ après chaque changement — sans effet sur Windows WebView2 / macOS WKWebView, où le sélecteur se ferme déjà automatiquement (#177). - Mise à jour de la dépendance `postcss` (8.5.6 → 8.5.13) pour corriger l'avis de sécurité de sévérité modérée GHSA-qx2v-qp2m-jg93 (XSS via `` non échappé dans le stringifier CSS). Transitive via vite, build-time uniquement — aucun impact runtime sur le binaire Tauri livré (#180). +- Bilan : nettoyage post-merge des suggestions issues des reviews des PR #182-#185. Six corrections groupées : (1) `getStarterCollisions` filtre désormais `archived_at IS NULL`, donc recréer un compte starter volontairement archivé n'est plus bloqué ; (2) `proposeStarterAccounts` re-vérifie chaque collision (nom, catégorie) en transaction avant l'INSERT en défense-in-depth (saut silencieux si déjà présent, aucune contrainte UNIQUE ajoutée) ; (3) les nouveaux profils reçoivent désormais `balance_starter_proposed` pré-seedé dans `consolidated_schema.sql` pour que le StarterAccountsModal ne s'ouvre jamais brièvement avec uniquement des collisions à la première visite de /balance ; (4) `/balance` cache maintenant le sélecteur de période, le graphique d'évolution et le tableau des comptes tant que la carte d'onboarding vide est affichée (évite trois messages vides empilés) ; (5) `BalanceOnboardingCard.Step` appelle directement `useTranslation()` au lieu de recevoir `t` en prop ; (6) le bloc de doc de la formule Modified Dietz dans `return_calculator.rs` est maintenant entouré d'une fence `text` pour que `cargo test --doc` n'essaie plus de compiler la pseudo-math comme du Rust (#187). ## [0.9.0] - 2026-04-29 diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ecf293..bf0e508 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ - Bilan: snapshot save now uses atomic BEGIN/COMMIT and validates all lines before any DB write, preventing orphan snapshot rows when validation fails. Migration v11 cleans existing orphans (#176). - Bilan: snapshot date picker on `/balance/snapshot` now closes after a date is selected on Linux (WebKitGTK), instead of staying open until the user pressed Esc. Workaround calls `blur()` on the input after each change — no-op on Windows WebView2 / macOS WKWebView, where the picker already auto-closes (#177). - Updated `postcss` dependency (8.5.6 → 8.5.13) to address moderate severity advisory GHSA-qx2v-qp2m-jg93 (XSS via unescaped `` in CSS stringifier). Transitive via vite, build-time only — no runtime impact on the shipped Tauri binary (#180). +- Bilan: post-merge cleanup of suggestions raised in the #182-#185 reviews. Six fixes bundled: (1) `getStarterCollisions` now filters `archived_at IS NULL` so re-creating a voluntarily archived starter is no longer blocked; (2) `proposeStarterAccounts` re-checks each (name, category) collision in-transaction before INSERT as defense-in-depth (skips silently on hit, no UNIQUE constraint added); (3) brand-new profiles now get `balance_starter_proposed` pre-seeded in `consolidated_schema.sql` so the StarterAccountsModal never briefly opens with all-collisions on first /balance visit; (4) `/balance` now hides the period selector, evolution chart and accounts table while the empty-state onboarding card is shown (avoids three stacked empty messages); (5) `BalanceOnboardingCard.Step` now calls `useTranslation()` directly instead of receiving `t` as a prop; (6) `return_calculator.rs` Modified Dietz formula doc block is wrapped in a `text` fence so `cargo test --doc` no longer fails to compile pseudo-math as Rust (#187). ## [0.9.0] - 2026-04-29 diff --git a/src-tauri/src/commands/return_calculator.rs b/src-tauri/src/commands/return_calculator.rs index f47bc53..46f1516 100644 --- a/src-tauri/src/commands/return_calculator.rs +++ b/src-tauri/src/commands/return_calculator.rs @@ -9,7 +9,9 @@ //! //! Modified Dietz formula: //! -//! R = (V_end - V_start - sum(CF_i)) / (V_start + sum(W_i * CF_i)) +//! ```text +//! R = (V_end - V_start - sum(CF_i)) / (V_start + sum(W_i * CF_i)) +//! ``` //! //! where `W_i = (T - t_i) / T`, `T = period_days`, `t_i = days from period_start //! to flow date`. A flow on day 0 is fully invested for the whole period -- 2.45.2