feat(charges): coherence pass — persistent paid state, cadence units, list-first (THI-329) - #226
Merged
Merged
Conversation
… list-first layout (THI-329) Full-page coherence review with @Thierry (scope validated 2026-07-18): 1. Group payment chip is now PERSISTENT tri-state: 'reste X €' counts down, then flips to a 'tout payé' check instead of silently disappearing — the static subtotal could be misread as an amount still owed. 2. Group subtotals get their cadence unit (/mois, /trimestre, /an): the monthly subtotal is per month but the annual one per YEAR — same label, different time units, unlabeled until now. 3. All-paid banner success state (brand tint + check + 'Tout est payé ce mois') — the month's micro-reward; hint hidden once everything is ticked. 4. Card title says '19 charges · 16 dues ce mois' — explains the 19 vs 16/16 gap. 5. Rows sorted by resolved due date ascending (stable: ticking never reorders). 6. List-first layout: the add form (rare action) is collapsed behind a header toggle (aria-expanded); it opens above the list and closes on successful add. UI-only (light lane): no domain/action/migration change. E2E testids untouched. i18n: app.charges.{groupAllPaid,allPaidTitle,dueCount,subtotalUnit.*} x5 locales. 39 ChargesClient tests green.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
area:charges
Contributor
Guide du relecteurRefactorise le tableau de bord des prélèvements pour rendre l’état « payé » persistant et plus clair, ajoute des unités de cadence aux sous-totaux de groupe, réorganise la mise en page pour que la liste soit affichée en premier avec un formulaire d’ajout repliable, et fiabilise le comportement avec de nouveaux tests et des chaînes i18n pour toutes les locales. Diagramme de séquence pour le comportement du formulaire d’ajout de prélèvement avec basculesequenceDiagram
actor User
participant ChargesClient
User->>ChargesClient: click charges-add-toggle
ChargesClient->>ChargesClient: setShowAddForm(true)
User->>ChargesClient: submit onCreate
ChargesClient->>ChargesClient: setLabel("")
ChargesClient->>ChargesClient: setAmount("")
ChargesClient->>ChargesClient: setShowAddForm(false)
Modifications par fichier
Conseils et commandesInteragir avec Sourcery
Personnaliser votre expérienceAccédez à votre dashboard pour :
Obtenir de l’aide
Original review guide in EnglishReviewer's GuideRefactors the charges dashboard to make the “paid” state persistent and clearer, adds cadence units to group subtotals, reorders the layout to be list-first with a toggled add form, and hardens behavior with new tests and i18n strings across locales. Sequence diagram for the toggled add-charge form behaviorsequenceDiagram
actor User
participant ChargesClient
User->>ChargesClient: click charges-add-toggle
ChargesClient->>ChargesClient: setShowAddForm(true)
User->>ChargesClient: submit onCreate
ChargesClient->>ChargesClient: setLabel("")
ChargesClient->>ChargesClient: setAmount("")
ChargesClient->>ChargesClient: setShowAddForm(false)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - j'ai trouvé 5 problèmes
Prompt pour les agents IA
Veuillez traiter les commentaires de cette revue de code :
## Commentaires individuels
### Commentaire 1
<location path="src/app/[locale]/app/charges/__tests__/ChargesClient.test.tsx" line_range="187-190" />
<code_context>
it('preserves the add-form CRUD scaffolding (out-of-scope guard against accidental refactor)', () => {
renderCharges([]);
+ // Coherence pass 2026-07: the form is collapsed by default (the list owns
+ // the first screen) and opens through the header toggle.
+ expect(screen.queryByLabelText('Libellé')).toBeNull();
+ fireEvent.click(screen.getByTestId('charges-add-toggle'));
// The 5 form fields + submit button stay reachable by their labels and roles.
expect(screen.getByLabelText('Libellé')).toBeInTheDocument();
</code_context>
<issue_to_address>
**suggestion (testing):** Ajoutez un test vérifiant que le formulaire d’ajout se replie automatiquement après une création réussie
Étant donné que l’implémentation replie également automatiquement le formulaire après un `createChargeAction` réussi, merci d’ajouter un test qui :
- ouvre le formulaire via `charges-add-toggle`,
- remplit et soumet le formulaire, attend le succès optimiste,
- puis vérifie que le formulaire est retiré du DOM et que `aria-expanded` sur le toggle est à `false`.
Cela permettra de se protéger contre les régressions où le formulaire reste ouvert après un ajout réussi.
Implémentation suggérée :
```typescript
it('preserves the add-form CRUD scaffolding (out-of-scope guard against accidental refactor)', () => {
renderCharges([]);
// Coherence pass 2026-07: the form is collapsed by default (the list owns
// the first screen) and opens through the header toggle.
expect(screen.queryByLabelText('Libellé')).toBeNull();
fireEvent.click(screen.getByTestId('charges-add-toggle'));
// The 5 form fields + submit button stay reachable by their labels and roles.
expect(screen.getByLabelText('Libellé')).toBeInTheDocument();
expect(screen.getByLabelText(/Montant/)).toBeInTheDocument();
});
it('passes paymentDay + computed paymentMonths to createChargeAction', async () => {
createChargeMock.mockResolvedValue({ ok: true });
renderCharges([]);
fireEvent.click(screen.getByTestId('charges-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Assurance' } });
fireEvent.change(screen.getByLabelText(/Montant/), { target: { value: '120' } });
fireEvent.change(screen.getByLabelText(/jour du mois/i), { target: { value: '15' } });
// Submit the form (kept minimal here; other fields may be populated elsewhere in the suite).
fireEvent.click(screen.getByRole('button', { name: /ajouter/i }));
// The optimistic result updates the summary.
await waitFor(() => {
expect(screen.getByTestId('charges-paid-summary')).toHaveTextContent('1/1');
});
});
it('auto-collapses the add form after a successful createChargeAction', async () => {
createChargeMock.mockResolvedValue({ ok: true });
renderCharges([]);
// Initially collapsed.
const toggle = screen.getByTestId('charges-add-toggle');
expect(toggle).toHaveAttribute('aria-expanded', 'false');
expect(screen.queryByLabelText('Libellé')).toBeNull();
// Open the form.
fireEvent.click(toggle);
expect(toggle).toHaveAttribute('aria-expanded', 'true');
expect(screen.getByLabelText('Libellé')).toBeInTheDocument();
// Fill in the minimal required fields.
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Assurance' } });
fireEvent.change(screen.getByLabelText(/Montant/), { target: { value: '120' } });
fireEvent.change(screen.getByLabelText(/jour du mois/i), { target: { value: '15' } });
// Submit the form.
fireEvent.click(screen.getByRole('button', { name: /ajouter/i }));
// Wait for the optimistic success to be reflected in the UI and for the form to auto-collapse.
await waitFor(() => {
// The newly added charge is reflected in the summary.
expect(screen.getByTestId('charges-paid-summary')).toHaveTextContent('1/1');
// The form is removed from the DOM.
expect(screen.queryByLabelText('Libellé')).toBeNull();
// The toggle reflects the collapsed state.
expect(toggle).toHaveAttribute('aria-expanded', 'false');
});
});
```
- Assurez-vous que `waitFor` est importé depuis vos utilitaires de test (par exemple `import { render, screen, waitFor, fireEvent } from '@testing-library/react';`) s’il n’est pas déjà utilisé ailleurs dans le fichier.
- Si le bouton de soumission a un autre nom accessible que `/ajouter/i`, adaptez le sélecteur `getByRole('button', { name: /ajouter/i })` pour correspondre au libellé réel utilisé dans le composant.
- Si des champs supplémentaires sont requis pour une soumission réussie (par ex. fréquence, sélection des mois), reflétez exactement l’ensemble des `fireEvent.change` ou clics déjà utilisés dans le test existant « passes paymentDay + computed paymentMonths » ou dans d’autres tests de création, afin que le chemin de succès optimiste soit déclenché de manière fiable.
</issue_to_address>
### Commentaire 2
<location path="src/app/[locale]/app/charges/__tests__/ChargesClient.test.tsx" line_range="229-234" />
<code_context>
it('passes paymentDay + computed paymentMonths to createChargeAction', async () => {
createChargeMock.mockResolvedValue({ ok: true });
renderCharges([]);
+ fireEvent.click(screen.getByTestId('charges-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Assurance' } });
fireEvent.change(screen.getByLabelText(/Montant/), { target: { value: '120' } });
fireEvent.change(screen.getByLabelText(/jour du mois/i), { target: { value: '15' } });
</code_context>
<issue_to_address>
**suggestion (testing):** Vérifier également que `aria-expanded` sur le toggle du formulaire d’ajout reflète l’état replié/ouvert
Comme ce test valide déjà le flux CRUD, ajoutez également une vérification du comportement d’accessibilité du toggle : avant le clic, `getByTestId('charges-add-toggle')` devrait avoir `aria-expanded="false"` et le formulaire ne devrait pas être dans le DOM ; après le clic, `aria-expanded="true"` et le formulaire devrait être visible. Cela permettra de détecter les régressions sur `aria-expanded`/`aria-controls` via le test.
Implémentation suggérée :
```typescript
it('preserves the add-form CRUD scaffolding (out-of-scope guard against accidental refactor)', () => {
renderCharges([]);
// Coherence pass 2026-07: the form is collapsed by default (the list owns
// the first screen) and opens through the header toggle.
const toggle = screen.getByTestId('charges-add-toggle');
expect(toggle).toHaveAttribute('aria-expanded', 'false');
expect(screen.queryByLabelText('Libellé')).toBeNull();
fireEvent.click(toggle);
// The toggle now reflects the open state and the form appears in the DOM.
expect(toggle).toHaveAttribute('aria-expanded', 'true');
// The 5 form fields + submit button stay reachable by their labels and roles.
expect(screen.getByLabelText('Libellé')).toBeInTheDocument();
expect(screen.getByLabelText(/Montant/)).toBeInTheDocument();
});
it('passes paymentDay + computed paymentMonths to createChargeAction', async () => {
createChargeMock.mockResolvedValue({ ok: true });
renderCharges([]);
const toggle = screen.getByTestId('charges-add-toggle');
expect(toggle).toHaveAttribute('aria-expanded', 'false');
expect(screen.queryByLabelText('Libellé')).toBeNull();
fireEvent.click(toggle);
expect(toggle).toHaveAttribute('aria-expanded', 'true');
expect(screen.getByLabelText('Libellé')).toBeInTheDocument();
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Assurance' } });
fireEvent.change(screen.getByLabelText(/Montant/), { target: { value: '120' } });
fireEvent.change(screen.getByLabelText(/jour du mois/i), { target: { value: '15' } });
expect(screen.getByTestId('charges-paid-summary')).toHaveTextContent('1/1');
});
```
Si le formulaire d’ajout est rendu de manière conditionnelle via `aria-controls` pointant vers le conteneur du formulaire, vous pouvez éventuellement renforcer encore les assertions en vérifiant que l’élément dont l’`id` est référencé par `aria-controls` est absent/présent avant/après le clic :
1. Lire `const controlsId = toggle.getAttribute('aria-controls');`
2. Vérifier `expect(screen.queryByTestId(controlsId!)).toBeNull()` (ou `queryByRole`/`queryByLabelText`) avant le clic.
3. Vérifier `expect(screen.getByTestId(controlsId!)).toBeInTheDocument()` après le clic.
Adaptez le sélecteur (`getByTestId`, `getByRole`, etc.) pour qu’il corresponde à la façon dont le conteneur du formulaire est identifié dans l’implémentation.
</issue_to_address>
### Commentaire 3
<location path="src/app/[locale]/app/charges/__tests__/ChargesClient.test.tsx" line_range="491-500" />
<code_context>
+ it('sorts rows by resolved due date ascending within a group (stable under ticking)', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Renforcer le test de tri pour prouver que l’ordre reste stable lors du pointage d’une facture
Ce test ne vérifie que l’ordre ascendant initial, alors que sa description promet aussi la stabilité lors du pointage. Pour couvrir cela, veuillez également (dans ce test ou un nouveau) capturer les IDs des éléments de liste, pointer l’un des éléments via son testid, puis relire la liste et vérifier que l’ordre des IDs reste inchangé. Cela validera le comportement « aucun réordonnancement sous le doigt de l’utilisateur ».
Implémentation suggérée :
```typescript
it('sorts rows by resolved due date ascending within a group (stable under ticking)', async () => {
const late = { ...monthlyCharge, id: 'late', label: 'Late bill', paymentDay: 20 };
const early = { ...monthlyCharge, id: 'early', label: 'Early bill', paymentDay: 3 };
renderCharges([late, early], { currentPeriod: { year: 2026, month: 1 } });
const group = within(screen.getByTestId('charges-group-monthly'));
const listItemsBefore = group.getAllByRole('listitem');
const idsBefore = listItemsBefore.map((el) => el.getAttribute('data-testid'));
expect(idsBefore).toEqual(['charges-row-early', 'charges-row-late']);
// Tick one of the items and ensure the order stays the same
// (no reordering under the user's finger)
const firstRowCheckbox = within(listItemsBefore[0]).getByRole('checkbox');
await user.click(firstRowCheckbox);
const listItemsAfter = group.getAllByRole('listitem');
const idsAfter = listItemsAfter.map((el) => el.getAttribute('data-testid'));
expect(idsAfter).toEqual(idsBefore);
});
```
Ce changement suppose :
1. Qu’une instance `user` provenant de `@testing-library/user-event` est déjà disponible dans ce fichier de test (par ex. via `const user = userEvent.setup()` dans un `beforeEach` ou au niveau supérieur).
2. Que chaque ligne de la liste contient une case à cocher accessible via `getByRole('checkbox')` à partir de l’élément de ligne.
Si l’une de ces hypothèses est fausse, vous devrez :
- Vous assurer que `user` est défini, généralement en important `userEvent` et en créant une instance partagée.
- Adapter `getByRole('checkbox')` pour qu’il corresponde à votre contrôle de pointage réel, par exemple en utilisant un `data-testid` spécifique dans la ligne ou un autre rôle/sélecteur.
</issue_to_address>
### Commentaire 4
<location path="src/app/[locale]/app/charges/__tests__/ChargesClient.test.tsx" line_range="501-506" />
<code_context>
+ expect(ids).toEqual(['charges-row-early', 'charges-row-late']);
+ });
+
+ it('labels each group subtotal with its cadence unit', () => {
+ renderCharges(sampleCharges, {
+ subtotals: { monthly: 1200, quarterly: 0, semiannual: 0, annual: 300 },
+ });
+ expect(screen.getByTestId('charges-group-subtotal-monthly')).toHaveTextContent('/mois');
+ expect(screen.getByTestId('charges-group-subtotal-annual')).toHaveTextContent('/an');
});
</code_context>
<issue_to_address>
**suggestion (testing):** Couvrir toutes les unités de cadence et le nouveau badge de nombre d’échéances dans les tests de header
Le test actuel couvre déjà le mensuel et l’annuel, mais deux petits ajouts réduiraient encore le risque de régression :
1. Étendre ou paramétrer ce test pour vérifier aussi les libellés trimestriel et semestriel, afin que les quatre cadences soient validées et que tout problème de clé de traduction soit détecté.
2. Ajouter un test dédié pour le badge `dueCount` du titre d’en-tête, en vérifiant à la fois le libellé « N au total / M dues ce mois-ci » et le fait que le badge est omis lorsque `dueThisMonth.length === 0`.
Cela reliera directement l’explication numérique et les suffixes d’unité aux tests.
Implémentation suggérée :
```typescript
it('sorts rows by resolved due date ascending within a group (stable under ticking)', () => {
const late = { ...monthlyCharge, id: 'late', label: 'Late bill', paymentDay: 20 };
const early = { ...monthlyCharge, id: 'early', label: 'Early bill', paymentDay: 3 };
renderCharges([late, early], { currentPeriod: { year: 2026, month: 1 } });
const ids = within(screen.getByTestId('charges-group-monthly'))
.getAllByRole('listitem')
.map((el) => el.getAttribute('data-testid'));
expect(ids).toEqual(['charges-row-early', 'charges-row-late']);
});
it.each([
['monthly', '/mois'],
['quarterly', '/trimestre'],
['semiannual', '/semestre'],
['annual', '/an'],
] as const)(
'labels %s group subtotal with its cadence unit',
(cadence, expectedSuffix) => {
renderCharges(sampleCharges, {
subtotals: { monthly: 1200, quarterly: 800, semiannual: 600, annual: 300 },
});
expect(
screen.getByTestId(`charges-group-subtotal-${cadence}`),
).toHaveTextContent(expectedSuffix);
},
);
it('shows the header due-count badge with total and current-month counts, and hides it when none are due', () => {
// Scenario 1: some charges are due this month
renderCharges(sampleCharges, {
currentPeriod: { year: 2026, month: 1 },
});
const dueCountBadge = screen.getByTestId('charges-header-due-count');
// Example expected wording: "N au total / M dues ce mois-ci"
// The exact numbers depend on sampleCharges and the header implementation.
expect(dueCountBadge.textContent).toMatch(/au total/);
expect(dueCountBadge.textContent).toMatch(/dues? ce mois[- ]ci/);
// Scenario 2: no charges due this month should omit the badge entirely
renderCharges(sampleCharges, {
currentPeriod: { year: 2026, month: 12 },
// Ensure the helper treats this as "no dues this month"; see additional_changes.
});
expect(screen.queryByTestId('charges-header-due-count')).toBeNull();
});
it('preserves the add-form CRUD scaffolding (out-of-scope guard against accidental refactor)', () => {
```
1. Alignez les attentes sur les libellés de cadence avec vos clés i18n réelles :
- Si trimestriel et semestriel utilisent des suffixes différents de `/trimestre` et `/semestre`, mettez à jour les valeurs `expectedSuffix` en conséquence.
- Assurez-vous que les nœuds DOM correspondants ont bien `data-testid="charges-group-subtotal-quarterly"` et `data-testid="charges-group-subtotal-semiannual"` comme utilisé dans le test.
2. Le helper `renderCharges` ne montre actuellement qu’une partie de son API dans l’extrait ; assurez-vous qu’il :
- Accepte l’objet `subtotals` avec les quatre clés de cadence (`monthly`, `quarterly`, `semiannual`, `annual`) et rend les quatre éléments de sous-total avec les `data-testid="charges-group-subtotal-*"`.
3. Pour le test du badge de nombre d’échéances dans le header :
- Confirmez que le header rend un badge avec `data-testid="charges-header-due-count"` lorsqu’au moins une charge est due dans la période courante, et qu’il inclut à la fois le nombre total et le nombre « dues ce mois-ci » dans son texte. Si votre libellé réel diffère (par ex. anglais vs français), adaptez les deux attentes `toMatch` au texte réel, ou remplacez-les par un seul `toHaveTextContent` comparant la chaîne exacte.
- Assurez-vous que `renderCharges` rend un arbre frais lors du deuxième appel dans le test (s’il effectue du caching interne, vous devrez peut-être mettre le deuxième scénario dans un test séparé ou appeler `cleanup()` entre les deux).
- Adaptez le deuxième scénario de sorte que, pour la `currentPeriod` donnée, aucune charge ne soit considérée comme « due ce mois-ci ». Selon votre implémentation, cela peut nécessiter soit :
* De modifier `currentPeriod` vers un mois où `sampleCharges` n’a aucun élément dû, soit
* D’ajouter un override explicite (par ex. `dueThisMonth: []`) si `renderCharges` le permet.
</issue_to_address>
### Commentaire 5
<location path="src/app/[locale]/app/charges/ChargesClient.tsx" line_range="125" />
<code_context>
+ // Rows are sorted by resolved due date ascending — a STABLE order (ticking a
+ // bill never reorders rows under the user's finger, unlike unpaid-first).
+ // The resolver is paid-independent for the date, so `isPaid: false` is fine.
+ const groups = useMemo(() => {
+ const dueIsoOf = (c: RawCharge): string =>
+ currentPeriodDueDate(
</code_context>
<issue_to_address>
**issue (complexity):** Envisagez d’extraire la logique de tri et d’état de paiement dans des helpers partagés et de simplifier les `classNames`/configs pour garder le composant ChargesClient plus déclaratif et plus facile à lire.
Vous pouvez conserver tout le nouveau comportement tout en rendant le composant plus lisible en extrayant quelques helpers purs et en aplatissant une partie du JSX.
### 1. Extraire le calcul de `groups`
Le `useMemo` définit actuellement `dueIsoOf`, décore les lignes pour le tri, puis les décore. Déplacez cela dans un helper de haut niveau pour que le code de rendu se lise comme « group + sort » plutôt que « implémenter le tri » :
```ts
function sortChargesByDueDate(
charges: RawCharge[],
currentPeriod: CurrentPeriod,
todayIso: string,
): RawCharge[] {
const dueIsoOf = (c: RawCharge): string =>
currentPeriodDueDate(
{ isActive: c.isActive, paymentMonths: c.paymentMonths, paymentDay: c.paymentDay },
currentPeriod,
todayIso,
false,
)?.dueDateIso ?? '9999-12-31';
return [...charges]
.map((c) => ({ c, dueIso: dueIsoOf(c) }))
.sort((a, b) => (a.dueIso < b.dueIso ? -1 : a.dueIso > b.dueIso ? 1 : 0))
.map(({ c }) => c);
}
```
Puis le hook devient :
```ts
const groups = useMemo(
() =>
FREQUENCIES.map((freq) => ({
freq,
rows: sortChargesByDueDate(
charges.filter((c) => c.frequency === freq),
currentPeriod,
todayIso,
),
})).filter((group) => group.rows.length > 0),
[charges, currentPeriod, todayIso],
);
```
On conserve le même comportement d’ordre stable tout en rendant « qu’est-ce qu’un groupe ? » évident au premier coup d’œil.
### 2. Extraire l’état de paiement par groupe
Vous répétez la logique `isActive`/`paymentMonths.includes` inline. La centraliser réduira la duplication et gardera l’arbre de rendu déclaratif :
```ts
type GroupPaymentState = {
remaining: number;
allPaid: boolean;
};
function computeGroupPaymentState(
rows: RawCharge[],
currentPeriod: CurrentPeriod,
optimisticPaid: ReadonlySet<string>,
): GroupPaymentState {
const groupDue = rows.filter(
(c) => c.isActive && c.paymentMonths.includes(currentPeriod.month),
);
const remaining = groupDue
.filter((c) => !optimisticPaid.has(c.id))
.reduce((sum, c) => sum + c.amount, 0);
const allPaid = groupDue.length > 0 && groupDue.every((c) => optimisticPaid.has(c.id));
return { remaining, allPaid };
}
```
Utilisation dans le `map` :
```ts
{groups.map(({ freq, rows }) => {
const headingId = `charges-group-${freq}-heading`;
const { remaining: groupRemaining, allPaid: groupAllPaid } = computeGroupPaymentState(
rows,
currentPeriod,
optimisticPaid,
);
// ... JSX unchanged, using groupRemaining, groupAllPaid ...
})}
```
Même logique, mais le composant n’a plus besoin de connaître les règles exactes de paiement à plusieurs endroits.
### 3. Aplatir les conditions sur les `className` du résumé
Si vous disposez d’un helper `cn`/`clsx`, vous pouvez éviter les longues chaînes interpolées et rendre l’état « succès » vs état par défaut plus clair :
```ts
import { cn } from '@/lib/cn';
<div
data-testid="charges-paid-summary"
className={cn(
'mb-4 flex items-center justify-between gap-3 rounded-lg px-4 py-3',
allPaidThisMonth ? 'bg-surface-muted' : 'bg-brand-600/10',
)}
>
<div className="min-w-0" aria-live="polite" role="status" aria-atomic="true">
<p
className={cn(
'flex items-center gap-1.5 text-xs font-medium',
allPaidThisMonth ? 'text-brand-text' : 'text-muted-foreground',
)}
>
{allPaidThisMonth && <Check aria-hidden className="h-3.5 w-3.5" strokeWidth={3} />}
{allPaidThisMonth ? t('allPaidTitle') : t('remainingLabel')}
</p>
<p
data-testid="charges-remaining-amount"
className={cn(
'text-xl font-bold tabular-nums',
allPaidThisMonth ? 'text-brand-text' : 'text-foreground',
)}
>
{formatCurrency(remainingThisMonth, locale)}
</p>
</div>
{/* ... */}
</div>
```
Le comportement reste identique ; le JSX est plus facile à parcourir.
### 4. Déplacer `SUBTOTAL_UNIT_KEY` dans une config partagée
Pour garder ce composant centré sur le comportement, vous pouvez colocaliser la correspondance fréquence → clé de traduction avec les autres configurations de fréquence :
```ts
// chargesConfig.ts
export const FREQUENCIES = ['monthly', 'quarterly', 'semiannual', 'annual'] as const;
export type Frequency = (typeof FREQUENCIES)[number];
export const SUBTOTAL_UNIT_KEY: Record<Frequency, string> = {
monthly: 'subtotalUnit.monthly',
quarterly: 'subtotalUnit.quarterly',
semiannual: 'subtotalUnit.semiannual',
annual: 'subtotalUnit.annual',
};
```
Puis dans le composant :
```ts
import { FREQUENCIES, SUBTOTAL_UNIT_KEY, type Frequency } from './chargesConfig';
// ...
{t(SUBTOTAL_UNIT_KEY[freq])}
```
Aucun changement de comportement, mais le composant se débarrasse du bruit de configuration et ses responsabilités deviennent plus claires.
</issue_to_address>Sourcery est gratuit pour l’open source – si vous aimez nos revues, merci d’envisager de les partager ✨
Original comment in English
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/app/[locale]/app/charges/__tests__/ChargesClient.test.tsx" line_range="187-190" />
<code_context>
it('preserves the add-form CRUD scaffolding (out-of-scope guard against accidental refactor)', () => {
renderCharges([]);
+ // Coherence pass 2026-07: the form is collapsed by default (the list owns
+ // the first screen) and opens through the header toggle.
+ expect(screen.queryByLabelText('Libellé')).toBeNull();
+ fireEvent.click(screen.getByTestId('charges-add-toggle'));
// The 5 form fields + submit button stay reachable by their labels and roles.
expect(screen.getByLabelText('Libellé')).toBeInTheDocument();
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that the add form auto-collapses after a successful creation
Since the implementation also auto-collapses the form after a successful `createChargeAction`, please add a test that:
- opens the form via `charges-add-toggle`,
- fills and submits the form, waits for the optimistic success,
- then asserts the form is removed from the DOM and `aria-expanded` on the toggle is `false`.
This will guard against regressions where the form stays open after a successful add.
Suggested implementation:
```typescript
it('preserves the add-form CRUD scaffolding (out-of-scope guard against accidental refactor)', () => {
renderCharges([]);
// Coherence pass 2026-07: the form is collapsed by default (the list owns
// the first screen) and opens through the header toggle.
expect(screen.queryByLabelText('Libellé')).toBeNull();
fireEvent.click(screen.getByTestId('charges-add-toggle'));
// The 5 form fields + submit button stay reachable by their labels and roles.
expect(screen.getByLabelText('Libellé')).toBeInTheDocument();
expect(screen.getByLabelText(/Montant/)).toBeInTheDocument();
});
it('passes paymentDay + computed paymentMonths to createChargeAction', async () => {
createChargeMock.mockResolvedValue({ ok: true });
renderCharges([]);
fireEvent.click(screen.getByTestId('charges-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Assurance' } });
fireEvent.change(screen.getByLabelText(/Montant/), { target: { value: '120' } });
fireEvent.change(screen.getByLabelText(/jour du mois/i), { target: { value: '15' } });
// Submit the form (kept minimal here; other fields may be populated elsewhere in the suite).
fireEvent.click(screen.getByRole('button', { name: /ajouter/i }));
// The optimistic result updates the summary.
await waitFor(() => {
expect(screen.getByTestId('charges-paid-summary')).toHaveTextContent('1/1');
});
});
it('auto-collapses the add form after a successful createChargeAction', async () => {
createChargeMock.mockResolvedValue({ ok: true });
renderCharges([]);
// Initially collapsed.
const toggle = screen.getByTestId('charges-add-toggle');
expect(toggle).toHaveAttribute('aria-expanded', 'false');
expect(screen.queryByLabelText('Libellé')).toBeNull();
// Open the form.
fireEvent.click(toggle);
expect(toggle).toHaveAttribute('aria-expanded', 'true');
expect(screen.getByLabelText('Libellé')).toBeInTheDocument();
// Fill in the minimal required fields.
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Assurance' } });
fireEvent.change(screen.getByLabelText(/Montant/), { target: { value: '120' } });
fireEvent.change(screen.getByLabelText(/jour du mois/i), { target: { value: '15' } });
// Submit the form.
fireEvent.click(screen.getByRole('button', { name: /ajouter/i }));
// Wait for the optimistic success to be reflected in the UI and for the form to auto-collapse.
await waitFor(() => {
// The newly added charge is reflected in the summary.
expect(screen.getByTestId('charges-paid-summary')).toHaveTextContent('1/1');
// The form is removed from the DOM.
expect(screen.queryByLabelText('Libellé')).toBeNull();
// The toggle reflects the collapsed state.
expect(toggle).toHaveAttribute('aria-expanded', 'false');
});
});
```
- Ensure `waitFor` is imported from your test utils (e.g. `import { render, screen, waitFor, fireEvent } from '@testing-library/react';`) if it is not already used elsewhere in the file.
- If the submit button has a different accessible name than `/ajouter/i`, adjust the `getByRole('button', { name: /ajouter/i })` selector to match the actual label used in the component.
- If some additional fields are required for a successful submission (e.g. frequency, months selection), mirror the exact set of `fireEvent.change` or clicks already used in the existing “passes paymentDay + computed paymentMonths” or other create tests so that the optimistic success path is triggered reliably.
</issue_to_address>
### Comment 2
<location path="src/app/[locale]/app/charges/__tests__/ChargesClient.test.tsx" line_range="229-234" />
<code_context>
it('passes paymentDay + computed paymentMonths to createChargeAction', async () => {
createChargeMock.mockResolvedValue({ ok: true });
renderCharges([]);
+ fireEvent.click(screen.getByTestId('charges-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Assurance' } });
fireEvent.change(screen.getByLabelText(/Montant/), { target: { value: '120' } });
fireEvent.change(screen.getByLabelText(/jour du mois/i), { target: { value: '15' } });
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert `aria-expanded` on the add-form toggle reflects the collapsed/open state
Since this test already validates the CRUD flow, also assert the accessibility behavior of the toggle: before the click, `getByTestId('charges-add-toggle')` should have `aria-expanded="false"` and the form should not be in the DOM; after the click, `aria-expanded="true"` and the form should be visible. This will ensure regressions in `aria-expanded`/`aria-controls` are caught by the test.
Suggested implementation:
```typescript
it('preserves the add-form CRUD scaffolding (out-of-scope guard against accidental refactor)', () => {
renderCharges([]);
// Coherence pass 2026-07: the form is collapsed by default (the list owns
// the first screen) and opens through the header toggle.
const toggle = screen.getByTestId('charges-add-toggle');
expect(toggle).toHaveAttribute('aria-expanded', 'false');
expect(screen.queryByLabelText('Libellé')).toBeNull();
fireEvent.click(toggle);
// The toggle now reflects the open state and the form appears in the DOM.
expect(toggle).toHaveAttribute('aria-expanded', 'true');
// The 5 form fields + submit button stay reachable by their labels and roles.
expect(screen.getByLabelText('Libellé')).toBeInTheDocument();
expect(screen.getByLabelText(/Montant/)).toBeInTheDocument();
});
it('passes paymentDay + computed paymentMonths to createChargeAction', async () => {
createChargeMock.mockResolvedValue({ ok: true });
renderCharges([]);
const toggle = screen.getByTestId('charges-add-toggle');
expect(toggle).toHaveAttribute('aria-expanded', 'false');
expect(screen.queryByLabelText('Libellé')).toBeNull();
fireEvent.click(toggle);
expect(toggle).toHaveAttribute('aria-expanded', 'true');
expect(screen.getByLabelText('Libellé')).toBeInTheDocument();
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Assurance' } });
fireEvent.change(screen.getByLabelText(/Montant/), { target: { value: '120' } });
fireEvent.change(screen.getByLabelText(/jour du mois/i), { target: { value: '15' } });
expect(screen.getByTestId('charges-paid-summary')).toHaveTextContent('1/1');
});
```
If the add-form is conditionally rendered via `aria-controls` pointing to the form container, you may optionally strengthen the assertions further by checking that the element with `id` referenced by `aria-controls` is absent/present before/after the click:
1. Read `const controlsId = toggle.getAttribute('aria-controls');`
2. Assert `expect(screen.queryByTestId(controlsId!)).toBeNull()` (or `queryByRole`/`queryByLabelText`) before click.
3. Assert `expect(screen.getByTestId(controlsId!)).toBeInTheDocument()` after click.
Adjust the selector (`getByTestId`, `getByRole`, etc.) to match how the form container is identified in the implementation.
</issue_to_address>
### Comment 3
<location path="src/app/[locale]/app/charges/__tests__/ChargesClient.test.tsx" line_range="491-500" />
<code_context>
+ it('sorts rows by resolved due date ascending within a group (stable under ticking)', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the sort test to prove the order is stable when ticking a bill
This test only checks the initial ascending order, but the description also promises stability when ticking. To cover that, please also (in this test or a new one) capture the list item IDs, tick one of the items via its testid, then re-read the list and assert the ID order is unchanged. That will validate the “no reordering under the user’s finger” behavior.
Suggested implementation:
```typescript
it('sorts rows by resolved due date ascending within a group (stable under ticking)', async () => {
const late = { ...monthlyCharge, id: 'late', label: 'Late bill', paymentDay: 20 };
const early = { ...monthlyCharge, id: 'early', label: 'Early bill', paymentDay: 3 };
renderCharges([late, early], { currentPeriod: { year: 2026, month: 1 } });
const group = within(screen.getByTestId('charges-group-monthly'));
const listItemsBefore = group.getAllByRole('listitem');
const idsBefore = listItemsBefore.map((el) => el.getAttribute('data-testid'));
expect(idsBefore).toEqual(['charges-row-early', 'charges-row-late']);
// Tick one of the items and ensure the order stays the same
// (no reordering under the user's finger)
const firstRowCheckbox = within(listItemsBefore[0]).getByRole('checkbox');
await user.click(firstRowCheckbox);
const listItemsAfter = group.getAllByRole('listitem');
const idsAfter = listItemsAfter.map((el) => el.getAttribute('data-testid'));
expect(idsAfter).toEqual(idsBefore);
});
```
This change assumes:
1. There is a `user` instance from `@testing-library/user-event` already available in this test file (e.g. via `const user = userEvent.setup()` in a `beforeEach` or at the top level).
2. Each list item row contains a checkbox reachable via `getByRole('checkbox')` from within the row element.
If either assumption is false, you will need to:
- Ensure `user` is defined, typically by importing `userEvent` and creating a shared instance.
- Adjust `getByRole('checkbox')` to match your actual ticking control, e.g. by using a specific `data-testid` inside the row or a different role/selector.
</issue_to_address>
### Comment 4
<location path="src/app/[locale]/app/charges/__tests__/ChargesClient.test.tsx" line_range="501-506" />
<code_context>
+ expect(ids).toEqual(['charges-row-early', 'charges-row-late']);
+ });
+
+ it('labels each group subtotal with its cadence unit', () => {
+ renderCharges(sampleCharges, {
+ subtotals: { monthly: 1200, quarterly: 0, semiannual: 0, annual: 300 },
+ });
+ expect(screen.getByTestId('charges-group-subtotal-monthly')).toHaveTextContent('/mois');
+ expect(screen.getByTestId('charges-group-subtotal-annual')).toHaveTextContent('/an');
});
</code_context>
<issue_to_address>
**suggestion (testing):** Cover all cadence units and the new due-count badge in header tests
The current test already covers monthly and annual, but two small additions would make regressions less likely:
1. Extend or parameterize this test to also assert quarterly and semiannual labels, so all four cadences are verified and any translation-key mismatch is caught.
2. Add a dedicated test for the header title’s `dueCount` badge, verifying both the “N total / M due this month” wording and that the badge is omitted when `dueThisMonth.length === 0`.
This will tie the numeric explanation and unit suffixes directly to tests.
Suggested implementation:
```typescript
it('sorts rows by resolved due date ascending within a group (stable under ticking)', () => {
const late = { ...monthlyCharge, id: 'late', label: 'Late bill', paymentDay: 20 };
const early = { ...monthlyCharge, id: 'early', label: 'Early bill', paymentDay: 3 };
renderCharges([late, early], { currentPeriod: { year: 2026, month: 1 } });
const ids = within(screen.getByTestId('charges-group-monthly'))
.getAllByRole('listitem')
.map((el) => el.getAttribute('data-testid'));
expect(ids).toEqual(['charges-row-early', 'charges-row-late']);
});
it.each([
['monthly', '/mois'],
['quarterly', '/trimestre'],
['semiannual', '/semestre'],
['annual', '/an'],
] as const)(
'labels %s group subtotal with its cadence unit',
(cadence, expectedSuffix) => {
renderCharges(sampleCharges, {
subtotals: { monthly: 1200, quarterly: 800, semiannual: 600, annual: 300 },
});
expect(
screen.getByTestId(`charges-group-subtotal-${cadence}`),
).toHaveTextContent(expectedSuffix);
},
);
it('shows the header due-count badge with total and current-month counts, and hides it when none are due', () => {
// Scenario 1: some charges are due this month
renderCharges(sampleCharges, {
currentPeriod: { year: 2026, month: 1 },
});
const dueCountBadge = screen.getByTestId('charges-header-due-count');
// Example expected wording: "N au total / M dues ce mois-ci"
// The exact numbers depend on sampleCharges and the header implementation.
expect(dueCountBadge.textContent).toMatch(/au total/);
expect(dueCountBadge.textContent).toMatch(/dues? ce mois[- ]ci/);
// Scenario 2: no charges due this month should omit the badge entirely
renderCharges(sampleCharges, {
currentPeriod: { year: 2026, month: 12 },
// Ensure the helper treats this as "no dues this month"; see additional_changes.
});
expect(screen.queryByTestId('charges-header-due-count')).toBeNull();
});
it('preserves the add-form CRUD scaffolding (out-of-scope guard against accidental refactor)', () => {
```
1. Align the cadence-label expectations with your actual i18n keys:
- If quarterly and semiannual use different suffixes than `/trimestre` and `/semestre`, update the `expectedSuffix` values accordingly.
- Ensure the corresponding DOM nodes have `data-testid="charges-group-subtotal-quarterly"` and `data-testid="charges-group-subtotal-semiannual"` as used in the test.
2. The `renderCharges` helper currently only shows part of its API in the snippet; make sure it:
- Accepts the `subtotals` object with all four cadence keys (`monthly`, `quarterly`, `semiannual`, `annual`) and renders the four subtotal elements with the `charges-group-subtotal-*` test IDs.
3. For the header due-count badge test:
- Confirm that the header renders a badge with `data-testid="charges-header-due-count"` when at least one charge is due in the current period, and that it includes both the total count and the “due this month” count in its text. If your actual wording differs (e.g. English vs French), adjust the two `toMatch` expectations to the real copy, or replace them with a single `toHaveTextContent` comparing the precise string.
- Ensure that `renderCharges` re-renders a fresh tree on the second call in the test (if it does internal caching you might need to wrap the second scenario in a separate test or call `cleanup()` between them).
- Adapt the second scenario so that, under the given `currentPeriod`, no charges qualify as "due this month". Depending on your implementation, this might require either:
* Changing the `currentPeriod` to a month where `sampleCharges` has no due items, or
* Adding an explicit override (e.g. `dueThisMonth: []`) if `renderCharges` supports that.
</issue_to_address>
### Comment 5
<location path="src/app/[locale]/app/charges/ChargesClient.tsx" line_range="125" />
<code_context>
+ // Rows are sorted by resolved due date ascending — a STABLE order (ticking a
+ // bill never reorders rows under the user's finger, unlike unpaid-first).
+ // The resolver is paid-independent for the date, so `isPaid: false` is fine.
+ const groups = useMemo(() => {
+ const dueIsoOf = (c: RawCharge): string =>
+ currentPeriodDueDate(
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the sorting and payment-state logic into shared helpers and simplifying classNames/config to keep the ChargesClient component more declarative and easier to scan.
You can keep all the new behavior while making the component easier to follow by extracting a couple of pure helpers and flattening some JSX.
### 1. Extract `groups` computation
The `useMemo` currently defines `dueIsoOf`, decorates rows to sort, then undecorates. Pull that into a top-level helper so the render code reads as “group + sort”, not “implement sorting”:
```ts
function sortChargesByDueDate(
charges: RawCharge[],
currentPeriod: CurrentPeriod,
todayIso: string,
): RawCharge[] {
const dueIsoOf = (c: RawCharge): string =>
currentPeriodDueDate(
{ isActive: c.isActive, paymentMonths: c.paymentMonths, paymentDay: c.paymentDay },
currentPeriod,
todayIso,
false,
)?.dueDateIso ?? '9999-12-31';
return [...charges]
.map((c) => ({ c, dueIso: dueIsoOf(c) }))
.sort((a, b) => (a.dueIso < b.dueIso ? -1 : a.dueIso > b.dueIso ? 1 : 0))
.map(({ c }) => c);
}
```
Then the hook becomes:
```ts
const groups = useMemo(
() =>
FREQUENCIES.map((freq) => ({
freq,
rows: sortChargesByDueDate(
charges.filter((c) => c.frequency === freq),
currentPeriod,
todayIso,
),
})).filter((group) => group.rows.length > 0),
[charges, currentPeriod, todayIso],
);
```
This keeps the same stable ordering behavior but makes “what is a group?” obvious at a glance.
### 2. Extract per‑group payment state
You’re repeating the `isActive`/`paymentMonths.includes` logic inline. Centralizing it will reduce duplication and keep the render tree declarative:
```ts
type GroupPaymentState = {
remaining: number;
allPaid: boolean;
};
function computeGroupPaymentState(
rows: RawCharge[],
currentPeriod: CurrentPeriod,
optimisticPaid: ReadonlySet<string>,
): GroupPaymentState {
const groupDue = rows.filter(
(c) => c.isActive && c.paymentMonths.includes(currentPeriod.month),
);
const remaining = groupDue
.filter((c) => !optimisticPaid.has(c.id))
.reduce((sum, c) => sum + c.amount, 0);
const allPaid = groupDue.length > 0 && groupDue.every((c) => optimisticPaid.has(c.id));
return { remaining, allPaid };
}
```
Usage in the map:
```ts
{groups.map(({ freq, rows }) => {
const headingId = `charges-group-${freq}-heading`;
const { remaining: groupRemaining, allPaid: groupAllPaid } = computeGroupPaymentState(
rows,
currentPeriod,
optimisticPaid,
);
// ... JSX unchanged, using groupRemaining, groupAllPaid ...
})}
```
Same logic, but the component no longer needs to know the exact payment rules in multiple places.
### 3. Flatten the summary className conditionals
If you have a `cn`/`clsx` helper, you can avoid long template literals and make the success state vs default state clearer:
```ts
import { cn } from '@/lib/cn';
<div
data-testid="charges-paid-summary"
className={cn(
'mb-4 flex items-center justify-between gap-3 rounded-lg px-4 py-3',
allPaidThisMonth ? 'bg-surface-muted' : 'bg-brand-600/10',
)}
>
<div className="min-w-0" aria-live="polite" role="status" aria-atomic="true">
<p
className={cn(
'flex items-center gap-1.5 text-xs font-medium',
allPaidThisMonth ? 'text-brand-text' : 'text-muted-foreground',
)}
>
{allPaidThisMonth && <Check aria-hidden className="h-3.5 w-3.5" strokeWidth={3} />}
{allPaidThisMonth ? t('allPaidTitle') : t('remainingLabel')}
</p>
<p
data-testid="charges-remaining-amount"
className={cn(
'text-xl font-bold tabular-nums',
allPaidThisMonth ? 'text-brand-text' : 'text-foreground',
)}
>
{formatCurrency(remainingThisMonth, locale)}
</p>
</div>
{/* ... */}
</div>
```
Behavior stays identical; JSX is easier to scan.
### 4. Move `SUBTOTAL_UNIT_KEY` to shared config
To keep this component focused on behavior, you can colocate the frequency → translation key mapping with the other frequency configuration:
```ts
// chargesConfig.ts
export const FREQUENCIES = ['monthly', 'quarterly', 'semiannual', 'annual'] as const;
export type Frequency = (typeof FREQUENCIES)[number];
export const SUBTOTAL_UNIT_KEY: Record<Frequency, string> = {
monthly: 'subtotalUnit.monthly',
quarterly: 'subtotalUnit.quarterly',
semiannual: 'subtotalUnit.semiannual',
annual: 'subtotalUnit.annual',
};
```
Then in the component:
```ts
import { FREQUENCIES, SUBTOTAL_UNIT_KEY, type Frequency } from './chargesConfig';
// ...
{t(SUBTOTAL_UNIT_KEY[freq])}
```
No behavior change, but the component sheds configuration noise and the responsibilities are clearer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…l cadence units (Sourcery) Four testing suggestions from the #226 review applied: successful creation collapses the form; the header toggle reflects state via aria-expanded; ticking a bill never reorders rows; every cadence unit + the due-this-month title count are asserted. 42 ChargesClient tests green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Passe de cohérence /app/charges (THI-329, analyse validée @Thierry 2026-07-18)
Suite du retour « le Sous-total affiche 1 804,21 € alors que tout est payé ». Analyse complète de la logique de la page → 6 fixes, scope validé via AskUserQuestion (« Cohérence + structure »).
Les 6 changements
aria-expanded), s'ouvre au-dessus de la liste, se referme après un ajout réussi.Risque / QA
UI-only, voie LÉGÈRE (risk tiering acté) : zéro domaine/action/migration. Testids E2E tous conservés (vérifié : les specs
e2e/charges/n'interagissent pas avec le formulaire). i18n ×5 locales. 39 tests ChargesClient verts + typecheck.Smoke @Thierry (desktop + mobile + dark)
🤖 Generated with Claude Code
Summary by Sourcery
Affiner l’UX du tableau de bord des charges pour mettre l’accent sur le flux de paiement, améliorer la clarté des sous-totaux et des unités de cadence, et rendre les états payés persistants et sans ambiguïté.
Améliorations :
Tests :
Original summary in English
Summary by Sourcery
Refine the charges dashboard UX to emphasize the payment workflow, improve clarity of subtotals and cadence units, and make paid states persistent and unambiguous.
Enhancements:
Tests: