maximus/Simpl-Resultat

Fork 0

fix(import): persist the import format and restore it faithfully #335

Closed

maximus wants to merge 1 commit from issue-324-persist-import-format into issue-323-migration-v17

maximus commented

2026-08-13 17:17:26 +00:00

Owner

Resolves #324 — link 3 of the import-format stack, stacked on issue-323-migration-v17 (link 2). Review the diff against that branch, not main.

The bug

import_sources carried no amount_mode and no sign_convention until v17, so restoring a configured source re-inferred the mode from mapping.debitAmount !== undefined and wrote signConvention: "negative_expense" outright (useImportWizard.ts:321-323). A credit-card statement configured for positive expenses came back on the default convention at its second import, and parseFilesInternal:514 negated every amount — expenses landed as income, no error shown anywhere. The format is a read value now, not a guessed one.

Two types and a codec, not one composed type

The /review-spec revision on the issue is applied as written: a composed ImportFormat is structurally impossible. ImportSource.has_header is declared boolean, ImportConfigTemplate.has_header is a number, SourceConfig is camelCase on a parsed mapping. So the guarantee does not come from a shared shape — it comes from src/utils/importFormat.ts being the SINGLE conversion point between ImportFormatRow (persisted, snake_case, mapping as JSON, has_header normalized to 0/1) and ImportFormat (domain), and from its completeness test.

That test is enforced on two levels, both mutation-checked:

FORMAT_FIELD_PAIRS is typed Record<keyof ImportFormat, keyof ImportFormatRow> — a field added to the format fails to build until it is listed.
The test compares each codec's real output keys against that table — a field listed but not wired fails the test.

Deleting sign_convention from formatToRow (the exact shape of the original bug) fails 14 tests. Re-introducing the hardcode in the restore fails the static guard.

Validation rather than fallback

formatFromRow raises on a value it cannot map instead of defaulting. The v17 CHECK admits absolute_indicator so the third amount mode ships without another migration, but the app cannot read one: falling through to the single branch would read the wrong column for every row, and anything other than positive_expense would silently mean negative_expense. This is the review's CWE-20 point, scoped to the read path this PR rewrites.

The error carries an i18n key, and the wizard then opens on a fresh configuration — blocking the wizard would have made "reconfigure this source" an action the user could not take. Not a regression either: today's JSON.parse(existing.column_mapping) already throws uncaught on corrupt JSON.

Also here

Write point moved from checkDuplicatesInternal to executeImport, so an import abandoned at the duplicate step leaves no configuration behind. A guard test holds that it is the only write point in the hook.
The mode owns the mapping: switching amount mode prunes the abandoned mode's columns. The column the <select> merely displays is deliberately NOT materialized — #325 turns an unmapped amount column into an explicit row error, and writing a 0 here would make it unreachable. The mode and the pruned mapping land in ONE state update: SourceConfigPanel's handlers each spread the same config prop, so two consecutive calls would see the same stale value and the second would win.
template_id is provenance only — recorded, restored, displayed, never re-read as format. selectedTemplateId is no longer blanked on every source selection. An acceptance test rewrites a template end to end and asserts the linked source reads identically, plus a non-vacuity check that the template really did change.
Both template writers go through the codec too, so a new format field cannot reach one table and miss the other.

Verification

963 vitest (was 924 after link 2), build clean, cargo check clean. No migration, no Rust change.
parseFilesInternal untouched — link 1's static guard on its five pinned expressions still passes, and none of link 1's 8 KNOWN DEFECT blocks moved.
No CHANGELOG or docs/ entry: the plan centralises both in link 10 (#332) to keep this ten-link stack conflict-free.

Handoff to #325 (link 4)

mapRow goes in src/utils/importFormat.ts, next to the codec. It should take ImportFormat — SourceConfig extends ImportFormat now, so the wizard passes its config straight through. When mapRow lands, delete the mapCorpusRow mirror in csvAutoDetect.test.ts and the guard at :246-267 along with it, as link 1's comment instructs.

Generated autonomously by /autopilot run of 2026-08-13

Resolves #324 — link 3 of the import-format stack, **stacked on `issue-323-migration-v17`** (link 2). Review the diff against that branch, not `main`. ## The bug `import_sources` carried no `amount_mode` and no `sign_convention` until v17, so restoring a configured source re-inferred the mode from `mapping.debitAmount !== undefined` and wrote `signConvention: "negative_expense"` outright (`useImportWizard.ts:321-323`). A credit-card statement configured for positive expenses came back on the default convention at its second import, and `parseFilesInternal:514` negated every amount — expenses landed as income, no error shown anywhere. The format is a read value now, not a guessed one. ## Two types and a codec, not one composed type The `/review-spec` revision on the issue is applied as written: a composed `ImportFormat` is structurally impossible. `ImportSource.has_header` is declared boolean, `ImportConfigTemplate.has_header` is a number, `SourceConfig` is camelCase on a parsed mapping. So the guarantee does not come from a shared shape — it comes from `src/utils/importFormat.ts` being the SINGLE conversion point between `ImportFormatRow` (persisted, snake_case, mapping as JSON, `has_header` normalized to 0/1) and `ImportFormat` (domain), and from its completeness test. That test is enforced on two levels, both mutation-checked: - `FORMAT_FIELD_PAIRS` is typed `Record<keyof ImportFormat, keyof ImportFormatRow>` — a field added to the format fails to **build** until it is listed. - The test compares each codec's real output keys against that table — a field listed but not wired fails the **test**. Deleting `sign_convention` from `formatToRow` (the exact shape of the original bug) fails 14 tests. Re-introducing the hardcode in the restore fails the static guard. ## Validation rather than fallback `formatFromRow` raises on a value it cannot map instead of defaulting. The v17 `CHECK` admits `absolute_indicator` so the third amount mode ships without another migration, but the app cannot read one: falling through to the `single` branch would read the wrong column for every row, and anything other than `positive_expense` would silently mean `negative_expense`. This is the review's CWE-20 point, scoped to the read path this PR rewrites. The error carries an i18n key, and the wizard then opens on a fresh configuration — blocking the wizard would have made "reconfigure this source" an action the user could not take. Not a regression either: today's `JSON.parse(existing.column_mapping)` already throws uncaught on corrupt JSON. ## Also here - **Write point moved** from `checkDuplicatesInternal` to `executeImport`, so an import abandoned at the duplicate step leaves no configuration behind. A guard test holds that it is the only write point in the hook. - **The mode owns the mapping**: switching amount mode prunes the abandoned mode's columns. The column the `<select>` merely *displays* is deliberately NOT materialized — #325 turns an unmapped amount column into an explicit row error, and writing a 0 here would make it unreachable. The mode and the pruned mapping land in ONE state update: `SourceConfigPanel`'s handlers each spread the same `config` prop, so two consecutive calls would see the same stale value and the second would win. - **`template_id` is provenance only** — recorded, restored, displayed, never re-read as format. `selectedTemplateId` is no longer blanked on every source selection. An acceptance test rewrites a template end to end and asserts the linked source reads identically, plus a non-vacuity check that the template really did change. - Both template writers go through the codec too, so a new format field cannot reach one table and miss the other. ## Verification - **963 vitest** (was 924 after link 2), **build clean**, **`cargo check` clean**. No migration, no Rust change. - `parseFilesInternal` untouched — link 1's static guard on its five pinned expressions still passes, and none of link 1's 8 `KNOWN DEFECT` blocks moved. - No CHANGELOG or `docs/` entry: the plan centralises both in link 10 (#332) to keep this ten-link stack conflict-free. ## Handoff to #325 (link 4) `mapRow` goes in `src/utils/importFormat.ts`, next to the codec. It should take `ImportFormat` — `SourceConfig extends ImportFormat` now, so the wizard passes its config straight through. When `mapRow` lands, delete the `mapCorpusRow` mirror in `csvAutoDetect.test.ts` and the guard at `:246-267` along with it, as link 1's comment instructs. Generated autonomously by /autopilot run of 2026-08-13

maximus added 1 commit 2026-08-13 17:17:26 +00:00

fix(import): persist the import format and restore it faithfully

PR Check — Frontend / frontend (pull_request) Successful in 1m48s

Details

7a604e0e0d

The root bug of this chantier. `import_sources` carried no `amount_mode` and no
`sign_convention` until v17, so restoring a configured source re-inferred the
mode from `mapping.debitAmount !== undefined` and wrote
`signConvention: "negative_expense"` outright (useImportWizard.ts:321-323). A
credit-card statement configured for positive expenses came back on the default
convention at its second import and `parseFilesInternal` negated every amount:
expenses landed as income, with no error shown anywhere. The format is a read
value now, not a guessed one.

Two types and a codec, not one composed type. The four carriers are
structurally incompatible -- `ImportSource.has_header` is declared boolean,
`ImportConfigTemplate.has_header` is a number, `SourceConfig` is camelCase on a
parsed mapping -- so the guarantee cannot come from a shared shape. It comes
from `src/utils/importFormat.ts` being the single conversion point between
`ImportFormatRow` (persisted: snake_case, mapping as JSON, `has_header`
normalized to 0/1) and `ImportFormat` (domain), and from its completeness test.

That test is enforced on two levels, and both were mutation-checked:
`FORMAT_FIELD_PAIRS` is typed `Record<keyof ImportFormat, keyof
ImportFormatRow>`, so a field added to the format fails to BUILD until it is
listed; the test then compares each codec's real output keys against that table,
so a field listed but not wired fails the TEST. Dropping `sign_convention` from
`formatToRow` -- the shape of the original bug -- fails 14 tests.

`formatFromRow` validates rather than falls back. The v17 CHECK admits
`absolute_indicator` so the third amount mode ships without another migration,
but the app cannot map one: falling through to the `single` branch would read
the wrong column for every row, and anything other than `positive_expense`
would silently mean `negative_expense`. It raises an `ImportFormatError`
carrying an i18n key, and the wizard opens on a fresh configuration so
"reconfigure this source" stays an action the user can actually take.

Also here:

- The config write moves from `checkDuplicatesInternal` to `executeImport`, so
  an import abandoned at the duplicate step leaves no configuration behind. It
  is the only write point in the hook and a guard test holds that.
- Switching amount mode prunes the abandoned mode's columns, so the mode owns
  the mapping rather than the reverse. The column the `<select>` merely displays
  is deliberately not materialized -- #325 turns an unmapped amount column into
  an explicit row error, and writing a 0 here would make it unreachable. The
  mode and the pruned mapping land in ONE state update: the panel's handlers
  each spread the same `config` prop, so two calls would see the same stale
  value.
- `template_id` is recorded and restored as provenance only, never re-read as
  format. `selectedTemplateId` is no longer blanked on every source selection.
  An acceptance test rewrites a template end to end and asserts the linked
  source reads identically, plus a non-vacuity check that the template really
  changed.
- Both template writers go through the codec too, so a new format field cannot
  reach one table and miss the other.

`parseFilesInternal` is untouched: link 1's static guard on its five pinned
expressions still passes. 39 new tests (963 vitest total, was 924), build clean,
`cargo check` clean, no migration.

Resolves #324

maximus added the

autopilot:pending-human

label 2026-08-13 17:17:34 +00:00

maximus commented

2026-08-14 15:31:54 +00:00

Author

Owner

`/pr-review` — Verdict : APPROVE

Le correctif de cause racine tient : la convention de signe et le mode de montant sont désormais lus, plus jamais devinés, et les trois garde-fous annoncés (type, test unitaire, test d'intégration SQL) ont été vérifiés par mutation, pas crus sur parole. Aucun blocage ; quatre points de suivi, dont un qui contraint l'ordre de merge de la pile.

Vérifications faites (et non pas relues)

J'ai matérialisé l'arbre de la branche hors du dépôt (git archive, aucun checkout) et exécuté les mutations :

Mutation	Résultat
Retirer `sign_convention` de `formatToRow`	`tsc` TS2741 + 14 tests rouges — le chiffre exact du corps de la PR
Retirer `sign_convention` de la liste de colonnes SQL de `createSource`	`tsc` passe (le SQL n'est pas typé) mais 6 tests de `import-format-roundtrip.test.ts` rouges
Retirer `sign_convention` de `updateSource`	1 test rouge (« updates the format in place »)
Réintroduire `signConvention: "negative_expense"` dans la restauration	garde statique rouge (« names no format field of its own »)

Le deuxième cas est le plus important : c'est exactement le trou que le type ne peut pas voir, et le FakeDb de import-format-roundtrip.test.ts le ferme parce qu'il interprète réellement la liste de colonnes de l'INSERT. La complétude n'est donc pas décorative.

Autres points contrôlés :

Round-trip = identité réelle. La boucle for (const field of Object.keys(FORMAT_FIELD_PAIRS)) est ce qui porte l'assertion — un toEqual global seul aurait laissé passer un champ tombé à undefined (toEqual ignore les clés undefined). La boucle est présente des deux côtés.
Le throw n'est pas re-transformé en fallback silencieux. formatFromRow lève, selectSource dispatch SET_ERROR, et rien ne réarme l'erreur ensuite : loadHeadersWithConfig avale ses propres erreurs sans toucher state.error, et ni SET_STEP ni SET_PARSED_PREVIEW ne la remettent à null dans le reducer. La bannière atteint donc bien l'écran de configuration, avec un message actionnable dans les deux langues.
Déplacement de l'écriture : checkDuplicatesInternal n'utilisait sourceId nulle part sous le bloc supprimé, et dans executeImport l'écriture précède la boucle createImportedFile — la FK source_id est servie. header_signature et description ne sont pas dans le SET de updateSource, donc préservés (#330 ne sera pas écrasé).
has_header : ImportFormatRowInput élargit à number | boolean et !!row.has_header normalise ; le test « reads the boolean an import_sources row is declared with » couvre l'écart déclaré/runtime des deux tables.
Pas de régression sur les sources existantes : le DEFAULT 'negative_expense' de v17 restitue exactement la valeur que le code écrivait en dur, et le backfill LIKE '%debitAmount%' reproduit l'ancienne inférence. Une source déjà en base ne change pas de comportement au premier rechargement.
Sécurité : SQL entièrement paramétré, les noms de colonnes passés à setColumn sont des littéraux, aucun secret. La frontière de lecture est fail-closed.
i18n : 3 clés import.errors.* présentes en FR et EN, aucune clé dupliquée dans les deux fichiers (vérifié par object_pairs_hook).
Migrations : aucune touchée (link 3 ne contient aucun Rust).
963 vitest verts + tsc --noEmit propre sur l'arbre de la branche, aucun .skip / .only. CI Forgejo verte (run 359).

Les trois critères d'acceptation de #324 sont couverts par des tests qui échouent sans le correctif.

Suggestions non bloquantes

1. clearMappingForMode rend le ?? 0 atteignable en un aller-retour de radio — ne pas merger links 1-3 sans #325

src/utils/importFormat.ts:174-192. Un basculement single → debit_credit → single supprime la clé amount : vérifié, le mapping {date:0, description:1, amount:4} ressort {date:0, description:1}. Le <select> affiche alors mapping.amount ?? 0 (« 0: Date »), et useImportWizard.ts:536 lit la colonne 0. Or parseFrenchAmount ne rejette pas une date : "15/01/2026" → 15, "2026-01-15" → 2026. La ligne passe la validation isNaN et s'importe avec un montant faux, sans erreur.

Avant cette PR la clé amount survivait au basculement, donc c'est un chemin nouvellement atteignable vers la classe de bug que le chantier corrige. Le raisonnement de la PR est juste (matérialiser un 0 rendrait l'erreur de #325 inatteignable) et #325 est le maillon suivant, status:in-progress, avec « supprimer les fallbacks ?? 0 au profit d'une erreur de ligne explicite » dans ses tâches. C'est donc une contrainte d'ordre de merge, pas un défaut de conception : links 1-3 ne doivent pas atteindre main sans #325. L'étape d'aperçu limite la casse entre-temps (les montants aberrants sont visibles avant l'import).

2. Bannière d'erreur périmée en changeant de source — src/hooks/useImportWizard.ts:293

selectSource ne remet jamais error à null en entrée, et goToStep("source-list") (ImportPage.tsx:125) ne le fait pas non plus. Séquence : source A illisible → « Reconfigurez la source avant d'importer » → retour → source B parfaitement valide → la bannière est toujours là et désigne maintenant la mauvaise source. Le défaut préexiste, mais cette PR fait de selectSource un producteur d'erreurs, ce qui le rend visible. Une ligne : dispatch({ type: "SET_ERROR", payload: null }) en tête de selectSource.

3. saveConfigAsTemplate ne s'approprie pas le modèle créé — src/hooks/useImportWizard.ts:939

Appliquer T1, éditer, puis « enregistrer comme nouveau modèle T2 » laisse selectedTemplateId sur T1 ; executeImport inscrit alors T1 en provenance d'une configuration qui vient de T2. Cosmétique tant que template_id n'est jamais relu comme format — c'est bien l'invariant tenu ici — mais la provenance affichée est fausse.

4. Message d'erreur anglais en dur — src/hooks/useImportWizard.ts:928

"Auto-detection failed. Please configure manually." traverse le nouveau t(state.error, { defaultValue: state.error }) et s'affiche tel quel, non traduit. Préexistant, mais la PR touche la ligne d'affichage et vient de créer l'emplacement naturel (import.errors.*) pour le corriger.

## `/pr-review` — Verdict : **APPROVE** Le correctif de cause racine tient : la convention de signe et le mode de montant sont désormais lus, plus jamais devinés, et les trois garde-fous annoncés (type, test unitaire, test d'intégration SQL) ont été **vérifiés par mutation**, pas crus sur parole. Aucun blocage ; quatre points de suivi, dont un qui contraint l'ordre de merge de la pile. --- ### Vérifications faites (et non pas relues) J'ai matérialisé l'arbre de la branche hors du dépôt (`git archive`, aucun checkout) et exécuté les mutations : | Mutation | Résultat | |---|---| | Retirer `sign_convention` de `formatToRow` | `tsc` **TS2741** + **14 tests** rouges — le chiffre exact du corps de la PR | | Retirer `sign_convention` de la **liste de colonnes SQL** de `createSource` | `tsc` **passe** (le SQL n'est pas typé) mais **6 tests** de `import-format-roundtrip.test.ts` rouges | | Retirer `sign_convention` de `updateSource` | 1 test rouge (« updates the format in place ») | | Réintroduire `signConvention: "negative_expense"` dans la restauration | garde statique rouge (« names no format field of its own ») | Le deuxième cas est le plus important : c'est exactement le trou que le type ne peut pas voir, et le `FakeDb` de `import-format-roundtrip.test.ts` le ferme parce qu'il interprète réellement la liste de colonnes de l'`INSERT`. La complétude n'est donc pas décorative. Autres points contrôlés : - **Round-trip = identité réelle.** La boucle `for (const field of Object.keys(FORMAT_FIELD_PAIRS))` est ce qui porte l'assertion — un `toEqual` global seul aurait laissé passer un champ tombé à `undefined` (`toEqual` ignore les clés `undefined`). La boucle est présente des deux côtés. - **Le `throw` n'est pas re-transformé en fallback silencieux.** `formatFromRow` lève, `selectSource` dispatch `SET_ERROR`, et **rien ne réarme l'erreur ensuite** : `loadHeadersWithConfig` avale ses propres erreurs sans toucher `state.error`, et ni `SET_STEP` ni `SET_PARSED_PREVIEW` ne la remettent à `null` dans le reducer. La bannière atteint donc bien l'écran de configuration, avec un message actionnable dans les deux langues. - **Déplacement de l'écriture** : `checkDuplicatesInternal` n'utilisait `sourceId` nulle part sous le bloc supprimé, et dans `executeImport` l'écriture précède la boucle `createImportedFile` — la FK `source_id` est servie. `header_signature` et `description` ne sont pas dans le `SET` de `updateSource`, donc préservés (#330 ne sera pas écrasé). - **`has_header`** : `ImportFormatRowInput` élargit à `number | boolean` et `!!row.has_header` normalise ; le test « reads the boolean an import_sources row is declared with » couvre l'écart déclaré/runtime des deux tables. - **Pas de régression sur les sources existantes** : le `DEFAULT 'negative_expense'` de v17 restitue exactement la valeur que le code écrivait en dur, et le backfill `LIKE '%debitAmount%'` reproduit l'ancienne inférence. Une source déjà en base ne change pas de comportement au premier rechargement. - **Sécurité** : SQL entièrement paramétré, les noms de colonnes passés à `setColumn` sont des littéraux, aucun secret. La frontière de lecture est **fail-closed**. - **i18n** : 3 clés `import.errors.*` présentes en FR **et** EN, aucune clé dupliquée dans les deux fichiers (vérifié par `object_pairs_hook`). - **Migrations** : aucune touchée (link 3 ne contient aucun Rust). - **963 vitest verts + `tsc --noEmit` propre** sur l'arbre de la branche, aucun `.skip` / `.only`. CI Forgejo verte (run 359). Les trois critères d'acceptation de #324 sont couverts par des tests qui échouent sans le correctif. --- ### Suggestions non bloquantes **1. `clearMappingForMode` rend le `?? 0` atteignable en un aller-retour de radio — ne pas merger links 1-3 sans #325** `src/utils/importFormat.ts:174-192`. Un basculement `single` → `debit_credit` → `single` supprime la clé `amount` : vérifié, le mapping `{date:0, description:1, amount:4}` ressort `{date:0, description:1}`. Le `<select>` affiche alors `mapping.amount ?? 0` (« 0: Date »), et `useImportWizard.ts:536` lit la colonne 0. Or `parseFrenchAmount` ne rejette **pas** une date : `"15/01/2026"` → `15`, `"2026-01-15"` → `2026`. La ligne passe la validation `isNaN` et s'importe avec un montant faux, sans erreur. Avant cette PR la clé `amount` survivait au basculement, donc c'est un chemin **nouvellement atteignable** vers la classe de bug que le chantier corrige. Le raisonnement de la PR est juste (matérialiser un `0` rendrait l'erreur de #325 inatteignable) et #325 est le maillon suivant, `status:in-progress`, avec « supprimer les fallbacks `?? 0` au profit d'une erreur de ligne explicite » dans ses tâches. C'est donc une contrainte d'**ordre de merge**, pas un défaut de conception : links 1-3 ne doivent pas atteindre `main` sans #325. L'étape d'aperçu limite la casse entre-temps (les montants aberrants sont visibles avant l'import). **2. Bannière d'erreur périmée en changeant de source** — `src/hooks/useImportWizard.ts:293` `selectSource` ne remet jamais `error` à `null` en entrée, et `goToStep("source-list")` (`ImportPage.tsx:125`) ne le fait pas non plus. Séquence : source A illisible → « Reconfigurez la source avant d'importer » → retour → source B parfaitement valide → la bannière est toujours là et désigne maintenant la mauvaise source. Le défaut préexiste, mais cette PR fait de `selectSource` un producteur d'erreurs, ce qui le rend visible. Une ligne : `dispatch({ type: "SET_ERROR", payload: null })` en tête de `selectSource`. **3. `saveConfigAsTemplate` ne s'approprie pas le modèle créé** — `src/hooks/useImportWizard.ts:939` Appliquer T1, éditer, puis « enregistrer comme nouveau modèle T2 » laisse `selectedTemplateId` sur T1 ; `executeImport` inscrit alors T1 en provenance d'une configuration qui vient de T2. Cosmétique tant que `template_id` n'est jamais relu comme format — c'est bien l'invariant tenu ici — mais la provenance affichée est fausse. **4. Message d'erreur anglais en dur** — `src/hooks/useImportWizard.ts:928` `"Auto-detection failed. Please configure manually."` traverse le nouveau `t(state.error, { defaultValue: state.error })` et s'affiche tel quel, non traduit. Préexistant, mais la PR touche la ligne d'affichage et vient de créer l'emplacement naturel (`import.errors.*`) pour le corriger.

maximus commented

2026-08-14 16:13:57 +00:00

Author

Owner

Mergée dans main en fast-forward avec le reste de la pile (tip 37b832e).

Forgejo ne détecte pas un merge local comme merged — la PR est donc fermée à la main, et l'issue liée s'est fermée automatiquement via son Resolves #N.

Tip cumulé validé avant push : 1181 vitest, 111 tests Rust, build tsc + vite. La CI ne tourne pas sur push main, cette validation locale était donc le seul filet.

Mergée dans `main` en fast-forward avec le reste de la pile (tip `37b832e`). Forgejo ne détecte pas un merge local comme *merged* — la PR est donc fermée à la main, et l'issue liée s'est fermée automatiquement via son `Resolves #N`. Tip cumulé validé avant push : **1181 vitest**, **111 tests Rust**, build tsc + vite. La CI ne tourne pas sur push `main`, cette validation locale était donc le seul filet.

maximus closed this pull request

2026-08-14 16:13:57 +00:00

PR Check — Frontend / frontend (pull_request) Successful in 1m48s

Details