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

2026-05-03 23:42:14 +00:00 · 2026-05-03 23:42:14 +00:00 · d2e65ae1ea
commit d2e65ae1ea
parent 4e7ba6b460 4095aec453
8 changed files with 160 additions and 99 deletions
--- a/CHANGELOG.fr.md
+++ b/CHANGELOG.fr.md
@ -19,6 +19,7 @@
 - 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 `</style>` non échappé dans le stringifier CSS). Transitive via vite, build-time uniquement — aucun impact runtime sur le binaire Tauri livré (#180).
 - Contournement du sélecteur de date WebKitGTK étendu aux 7 autres champs `<input type="date">` natifs répartis sur 4 composants (barre de filtres Transactions, formulaire Ajustements, modal de Liaison de transferts, sélecteur de période). Chaque handler onChange appelle désormais `e.currentTarget.blur()` pour fermer le popup natif sur Linux Tauri WebView — sans effet sur Windows WebView2 / macOS WKWebView. Même approche que #177 (#188).
+- 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

--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -19,6 +19,7 @@
 - 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 `</style>` in CSS stringifier). Transitive via vite, build-time only — no runtime impact on the shipped Tauri binary (#180).
 - WebKitGTK date picker workaround extended to the remaining 7 native `<input type="date">` fields across 4 components (Transactions filter bar, Adjustments form, Link Transfers modal, Period selector). Each onChange handler now calls `e.currentTarget.blur()` to dismiss the native popup on Linux Tauri WebView — no-op on Windows WebView2 / macOS WKWebView. Same approach as #177 (#188).
+- 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

--- 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
--- 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
--- 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}
        />
        <Step
          number={2}
@ -92,7 +90,6 @@ export default function BalanceOnboardingCard({
          ctaLabel={t("balance.onboarding.step2.cta")}
          ctaHref="/balance/snapshot"
          disabledHint={t("balance.onboarding.step2.disabledHint")}
-          t={t}
        />
      </ol>
    </div>
@ -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";
--- 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", () => {
@ -85,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
@ -103,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

--- 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;
@ -174,96 +173,104 @@ export default function BalancePage() {
        </div>
      )}

-      <div className="space-y-6">
-        {(() => {
-          // 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 (
+            <div className="space-y-6">
              <BalanceOnboardingCard
                accountsCount={accountsCount}
                snapshotsCount={hasAnySnapshot ? 1 : 0}
              />
-            );
-          }
-          return <BalanceOverviewCard totals={state.evolutionTotals} />;
-        })()}
+            </div>
+          );
+        }

-        <div className="flex flex-col sm:flex-row sm:items-center sm:justify-between gap-3">
-          {/* Period selector */}
-          <div
-            role="group"
-            aria-label={t("balance.period.legend")}
-            className="inline-flex rounded-lg border border-[var(--border)] overflow-hidden"
-          >
-            {PERIOD_OPTIONS.map((p) => (
-              <button
-                key={p}
-                type="button"
-                onClick={() => setPeriod(p)}
-                className={`px-3 py-1.5 text-sm font-medium ${
-                  state.period === p
-                    ? "bg-[var(--primary)] text-white"
-                    : "bg-[var(--card)] text-[var(--foreground)] hover:bg-[var(--muted)]/40"
-                }`}
-                aria-pressed={state.period === p}
+        return (
+          <div className="space-y-6">
+            <BalanceOverviewCard totals={state.evolutionTotals} />
+
+            <div className="flex flex-col sm:flex-row sm:items-center sm:justify-between gap-3">
+              {/* Period selector */}
+              <div
+                role="group"
+                aria-label={t("balance.period.legend")}
+                className="inline-flex rounded-lg border border-[var(--border)] overflow-hidden"
              >
-                {t(`balance.period.${p}`)}
-              </button>
-            ))}
-          </div>
+                {PERIOD_OPTIONS.map((p) => (
+                  <button
+                    key={p}
+                    type="button"
+                    onClick={() => setPeriod(p)}
+                    className={`px-3 py-1.5 text-sm font-medium ${
+                      state.period === p
+                        ? "bg-[var(--primary)] text-white"
+                        : "bg-[var(--card)] text-[var(--foreground)] hover:bg-[var(--muted)]/40"
+                    }`}
+                    aria-pressed={state.period === p}
+                  >
+                    {t(`balance.period.${p}`)}
+                  </button>
+                ))}
+              </div>

-          {/* Chart mode toggle */}
-          <div
-            role="group"
-            aria-label={t("balance.chart.modeLegend")}
-            className="inline-flex rounded-lg border border-[var(--border)] overflow-hidden"
-          >
-            {(["line", "stacked"] as BalanceChartMode[]).map((mode) => (
-              <button
-                key={mode}
-                type="button"
-                onClick={() => setChartMode(mode)}
-                className={`px-3 py-1.5 text-sm font-medium ${
-                  state.chartMode === mode
-                    ? "bg-[var(--primary)] text-white"
-                    : "bg-[var(--card)] text-[var(--foreground)] hover:bg-[var(--muted)]/40"
-                }`}
-                aria-pressed={state.chartMode === mode}
+              {/* Chart mode toggle */}
+              <div
+                role="group"
+                aria-label={t("balance.chart.modeLegend")}
+                className="inline-flex rounded-lg border border-[var(--border)] overflow-hidden"
              >
-                {t(`balance.chart.mode.${mode}`)}
-              </button>
-            ))}
+                {(["line", "stacked"] as BalanceChartMode[]).map((mode) => (
+                  <button
+                    key={mode}
+                    type="button"
+                    onClick={() => setChartMode(mode)}
+                    className={`px-3 py-1.5 text-sm font-medium ${
+                      state.chartMode === mode
+                        ? "bg-[var(--primary)] text-white"
+                        : "bg-[var(--card)] text-[var(--foreground)] hover:bg-[var(--muted)]/40"
+                    }`}
+                    aria-pressed={state.chartMode === mode}
+                  >
+                    {t(`balance.chart.mode.${mode}`)}
+                  </button>
+                ))}
+              </div>
+            </div>
+
+            <BalanceEvolutionChart
+              mode={state.chartMode}
+              totals={state.evolutionTotals}
+              byCategory={state.evolutionByCategory}
+              categoryLabels={categoryLabels}
+              transferMarkers={allTransferMarkers}
+            />
+
+            <div>
+              <h2 className="text-lg font-semibold mb-3">
+                {t("balance.overview.accountsTitle")}
+              </h2>
+              <BalanceAccountsTable
+                accounts={state.accountsLatest}
+                periodAnchor={state.accountsPeriodAnchor}
+                sinceCreationDate={earliestSnapshotDate}
+                onArchiveAccount={(acc) => handleArchiveAccount(acc.account_id)}
+                onLinkTransfers={(acc) => setLinkTarget(acc)}
+              />
+            </div>
          </div>
-        </div>
-
-        <BalanceEvolutionChart
-          mode={state.chartMode}
-          totals={state.evolutionTotals}
-          byCategory={state.evolutionByCategory}
-          categoryLabels={categoryLabels}
-          transferMarkers={allTransferMarkers}
-        />
-
-        <div>
-          <h2 className="text-lg font-semibold mb-3">
-            {t("balance.overview.accountsTitle")}
-          </h2>
-          <BalanceAccountsTable
-            accounts={state.accountsLatest}
-            periodAnchor={state.accountsPeriodAnchor}
-            sinceCreationDate={earliestSnapshotDate}
-            onArchiveAccount={(acc) => handleArchiveAccount(acc.account_id)}
-            onLinkTransfers={(acc) => setLinkTarget(acc)}
-          />
-        </div>
-      </div>
+        );
+      })()}

      <StarterAccountsModal
        isOpen={showStarterModal}
--- a/src/services/balance.service.ts
+++ b/src/services/balance.service.ts
@ -487,7 +487,8 @@ export async function getStarterCollisions(): Promise<Set<string>> {
    `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<string>();
  for (const starter of STARTER_ACCOUNTS) {
@ -508,9 +509,11 @@ export async function getStarterCollisions(): Promise<Set<string>> {
 * 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[]
@ -537,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)`,