feat(commitments): dashboard « Mes engagements » card + shared read (épic PR-3) - #235
Merged
Conversation
…épic PR-3) The cockpit now answers the long-run question next to the monthly one: how much do I still owe in total, and what falls due this month. - New getCommitmentsWithLedger() data helper, used by BOTH /app/commitments and the dashboard card — the two surfaces read the same source and can never disagree on what is owed. The page's own inline query is replaced by it. - EngagementsCard: total still owed, what is due this month (hidden once ticked), and one row per commitment with the instalments left and the end month. A fully settled commitment DISAPPEARS — it must stop weighing on the cockpit. The whole card self-hides when there is nothing to show, so an empty workspace gains no noise. - Sits right after the bills card: bills answer the month, commitments the long run. i18n x5: dashboard.commitments.* + the section heading. 8 card tests (netting, due-this-month, settled disappears, one-off, inactive). Full suite 1572 green, typecheck/lint/build clean.
|
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
status:review-needed
Contributor
Guide du relecteurImplémente la nouvelle carte de tableau de bord « Mes engagements » basée sur un helper de données partagé engagements+ledger, refactore la page des engagements pour utiliser ce helper, et ajoute des tests et le câblage i18n afin de garantir un affichage cohérent et auto-masqué des engagements long terme et des échéances de ce mois-ci. 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 GuideImplements the new "Mes engagements" dashboard card powered by a shared commitments+ledger data helper, refactors the commitments page to use that helper, and adds tests and i18n wiring to ensure consistent, self-hiding display of long-term commitments and this-month dues. 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é 2 problèmes et laissé quelques retours de haut niveau :
- EngagementsCard est défini comme un composant serveur async, mais les tests l'appellent comme une fonction simple ; envisagez de le rendre via du JSX dans un wrapper (ou d'extraire un sous‑composant purement présentational) pour mieux refléter le comportement en runtime et éviter les surprises s'il gagne des hooks plus tard.
- Le mapping
toDomaindeCommitmentRowversCommitmentest maintenant dupliqué entre la carte du tableau de bord et la fonctionnalité des engagements ; envisagez d'extraire un mapper partagé dans la couche domaine ou data pour garder la transformation cohérente.
Prompt pour les agents IA
Veuillez traiter les commentaires de cette revue de code :
## Commentaires généraux
- EngagementsCard est défini comme un composant serveur async, mais les tests l'appellent comme une fonction simple ; envisagez de le rendre via du JSX dans un wrapper (ou d'extraire un sous‑composant purement présentational) pour mieux refléter le comportement en runtime et éviter les surprises s'il gagne des hooks plus tard.
- Le mapping `toDomain` de `CommitmentRow` vers `Commitment` est maintenant dupliqué entre la carte du tableau de bord et la fonctionnalité des engagements ; envisagez d'extraire un mapper partagé dans la couche domaine ou data pour garder la transformation cohérente.
## Commentaires individuels
### Commentaire 1
<location path="src/components/dashboard/__tests__/EngagementsCard.test.tsx" line_range="49-27" />
<code_context>
+ isActive: true,
+};
+
+async function renderCard(input: {
+ commitments: CommitmentRow[];
+ paidKeysByCommitment?: Record<string, string[]>;
+ currentPeriod?: { year: number; month: number };
+}) {
+ return render(
+ await EngagementsCard({
+ commitments: input.commitments,
+ paidKeysByCommitment: input.paidKeysByCommitment ?? {},
+ currentPeriod: input.currentPeriod ?? { year: 2026, month: 3 },
+ locale: 'fr-BE',
+ }),
+ );
+}
+
+describe('<EngagementsCard />', () => {
+ it('renders nothing at all when there is no commitment', async () => {
+ const { container } = await renderCard({ commitments: [] });
+ expect(container).toBeEmptyDOMElement();
+ });
+
+ it('shows the total still owed and links to the page', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Ajoutez un test couvrant plusieurs engagements pour valider l’agrégation des totaux et du montant dû mensuel
Les tests actuels ne couvrent qu’un seul engagement `carLoan`, donc nous ne vérifions jamais que `totalRemaining` et `dueAmount` sont correctement agrégés sur plusieurs engagements. Veuillez ajouter un test avec au moins deux engagements actifs (par exemple tous deux dus pour le mois courant) et vérifier que `engagements-total-remaining` et `engagements-due-this-month` reflètent la somme de toutes les lignes applicables, afin d’éviter les régressions où un seul élément serait compté.
Implémentation suggérée :
```typescript
it('shows the total still owed and links to the page', async () => {
await renderCard({ commitments: [carLoan] });
expect(screen.getByTestId('engagements-total-remaining')).toHaveTextContent(/4[ ]200/);
expect(screen.getByTestId('engagements-card-link')).toHaveAttribute('href', '/app/commitments');
});
it('aggregates totals and due amount across multiple commitments', async () => {
const secondCarLoan: CommitmentRow = {
...carLoan,
key: 'car-2',
name: 'Second car loan',
};
await renderCard({ commitments: [carLoan, secondCarLoan] });
// total remaining should be the sum across both commitments
expect(screen.getByTestId('engagements-total-remaining')).toHaveTextContent(/8[ ]400/);
// due this month should also be aggregated across both commitments
expect(screen.getByTestId('engagements-due-this-month')).toHaveTextContent(/500/);
});
it('nets the ticked instalments out of the total', async () => {
```
Ce patch suppose :
1. `CommitmentRow` est déjà importé dans ce fichier de tests (car il est utilisé dans l’extrait que vous avez fourni).
2. Il existe un élément avec `data-testid="engagements-due-this-month"` qui affiche le montant mensuel dû pour tous les engagements, et pour un seul `carLoan` sa valeur est `250` (donc pour deux prêts identiques, le montant agrégé est `500`).
Si le montant réellement dû pour un seul `carLoan` est différent, ajustez la regex `/500/` en conséquence (deux fois la valeur pour un seul engagement).
Si le `data-testid` utilisé pour le montant dû mensuel diffère, remplacez `'engagements-due-this-month'` par le bon test id.
</issue_to_address>
### Commentaire 2
<location path="src/components/dashboard/__tests__/EngagementsCard.test.tsx" line_range="84-93" />
<code_context>
+ expect(screen.getByTestId('engagements-total-remaining')).toHaveTextContent(/3[ ]700/);
+ });
+
+ it('surfaces what is due this month, and hides it once ticked', async () => {
+ const { unmount } = await renderCard({
+ commitments: [carLoan],
+ currentPeriod: { year: 2026, month: 3 },
+ });
+ expect(screen.getByTestId('engagements-due-this-month')).toHaveTextContent(/250/);
+ unmount();
+ await renderCard({
+ commitments: [carLoan],
+ paidKeysByCommitment: { car: ['2026-3'] },
+ currentPeriod: { year: 2026, month: 3 },
+ });
+ expect(screen.queryByTestId('engagements-due-this-month')).toBeNull();
+ });
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Couvrez le cas où certains engagements sont cochés pour le mois courant et d’autres non
Ce test ne vérifie que le comportement tout ou rien pour un seul engagement sur la période courante. Veuillez ajouter un test où deux engagements partagent le même `currentPeriod`, l’un est dû et non coché et l’autre est déjà coché, afin de confirmer que la bannière apparaît toujours et que `dueAmount` inclut uniquement les engagements non cochés. Cela protégera contre un bug où un paiement coché pour le mois masquerait à tort toute la ligne « Ce mois ».
Implémentation suggérée :
```typescript
it('nets the ticked instalments out of the total', async () => {
await renderCard({
commitments: [carLoan],
paidKeysByCommitment: { car: ['2026-1', '2026-2'] },
});
expect(screen.getByTestId('engagements-total-remaining')).toHaveTextContent(/3[ ]700/);
});
it('includes only unticked commitments in the current-month due amount when some are already paid', async () => {
const secondCommitment = {
...carLoan,
key: 'car-2',
};
await renderCard({
commitments: [carLoan, secondCommitment],
paidKeysByCommitment: { car: ['2026-3'] },
currentPeriod: { year: 2026, month: 3 },
});
const banner = screen.getByTestId('engagements-due-this-month');
expect(banner).toBeInTheDocument();
expect(banner).toHaveTextContent(/250/);
});
import messages from '../../../../messages/fr-BE.json';
```
- Si `carLoan` ne représente pas un engagement de 250 par mois (ou si le montant « dû ce mois‑ci » est différent), ajustez l’assertion `toHaveTextContent(/250/)` au montant correct par période pour un seul engagement.
- Si `CommitmentRow` nécessite des champs uniques supplémentaires au‑delà de `key` pour distinguer les engagements (par exemple un `id`), dupliquez également ces champs dans `secondCommitment` en les surchargant pour éviter les collisions.
</issue_to_address>Sourcery est gratuit pour l’open source - si vous appréciez nos revues, pensez à les partager ✨
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- EngagementsCard is defined as an async server component but the tests call it as a plain function; consider rendering it via JSX in a wrapper (or factoring out a pure presentational subcomponent) to better mirror runtime behavior and avoid surprises if it gains hooks later.
- The
toDomainmapping fromCommitmentRowtoCommitmentis now duplicated between the dashboard card and the commitments feature; consider extracting a shared mapper in the domain or data layer to keep the transformation consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- EngagementsCard is defined as an async server component but the tests call it as a plain function; consider rendering it via JSX in a wrapper (or factoring out a pure presentational subcomponent) to better mirror runtime behavior and avoid surprises if it gains hooks later.
- The `toDomain` mapping from `CommitmentRow` to `Commitment` is now duplicated between the dashboard card and the commitments feature; consider extracting a shared mapper in the domain or data layer to keep the transformation consistent.
## Individual Comments
### Comment 1
<location path="src/components/dashboard/__tests__/EngagementsCard.test.tsx" line_range="49-27" />
<code_context>
+ isActive: true,
+};
+
+async function renderCard(input: {
+ commitments: CommitmentRow[];
+ paidKeysByCommitment?: Record<string, string[]>;
+ currentPeriod?: { year: number; month: number };
+}) {
+ return render(
+ await EngagementsCard({
+ commitments: input.commitments,
+ paidKeysByCommitment: input.paidKeysByCommitment ?? {},
+ currentPeriod: input.currentPeriod ?? { year: 2026, month: 3 },
+ locale: 'fr-BE',
+ }),
+ );
+}
+
+describe('<EngagementsCard />', () => {
+ it('renders nothing at all when there is no commitment', async () => {
+ const { container } = await renderCard({ commitments: [] });
+ expect(container).toBeEmptyDOMElement();
+ });
+
+ it('shows the total still owed and links to the page', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test covering multiple commitments to validate aggregation of totals and monthly due amount
Current tests only cover a single `carLoan` commitment, so we never verify that `totalRemaining` and `dueAmount` are correctly aggregated across multiple commitments. Please add a test with at least two active commitments (e.g. both due in the current month) and assert that `engagements-total-remaining` and `engagements-due-this-month` reflect the sum of all applicable rows, to guard against regressions where only one item is counted.
Suggested implementation:
```typescript
it('shows the total still owed and links to the page', async () => {
await renderCard({ commitments: [carLoan] });
expect(screen.getByTestId('engagements-total-remaining')).toHaveTextContent(/4[ ]200/);
expect(screen.getByTestId('engagements-card-link')).toHaveAttribute('href', '/app/commitments');
});
it('aggregates totals and due amount across multiple commitments', async () => {
const secondCarLoan: CommitmentRow = {
...carLoan,
key: 'car-2',
name: 'Second car loan',
};
await renderCard({ commitments: [carLoan, secondCarLoan] });
// total remaining should be the sum across both commitments
expect(screen.getByTestId('engagements-total-remaining')).toHaveTextContent(/8[ ]400/);
// due this month should also be aggregated across both commitments
expect(screen.getByTestId('engagements-due-this-month')).toHaveTextContent(/500/);
});
it('nets the ticked instalments out of the total', async () => {
```
This patch assumes:
1. `CommitmentRow` is already imported in this test file (as it is used in the snippet you provided).
2. There is an element with `data-testid="engagements-due-this-month"` that shows the monthly due amount for all commitments, and for a single `carLoan` its value is `250` (so for two identical loans the aggregated amount is `500`).
If the actual due amount for a single `carLoan` is different, adjust the `/500/` regex accordingly (twice the single-commitment value).
If the `data-testid` used for the monthly due amount differs, replace `'engagements-due-this-month'` with the correct test id.
</issue_to_address>
### Comment 2
<location path="src/components/dashboard/__tests__/EngagementsCard.test.tsx" line_range="84-93" />
<code_context>
+ expect(screen.getByTestId('engagements-total-remaining')).toHaveTextContent(/3[ ]700/);
+ });
+
+ it('surfaces what is due this month, and hides it once ticked', async () => {
+ const { unmount } = await renderCard({
+ commitments: [carLoan],
+ currentPeriod: { year: 2026, month: 3 },
+ });
+ expect(screen.getByTestId('engagements-due-this-month')).toHaveTextContent(/250/);
+ unmount();
+ await renderCard({
+ commitments: [carLoan],
+ paidKeysByCommitment: { car: ['2026-3'] },
+ currentPeriod: { year: 2026, month: 3 },
+ });
+ expect(screen.queryByTestId('engagements-due-this-month')).toBeNull();
+ });
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Cover the case where some commitments are ticked for the current month and others are not
This only checks the all-or-nothing behavior for a single commitment in the current period. Please add a test where two commitments share the same `currentPeriod`, one is due and unticked and the other is already ticked, to confirm the banner still appears and `dueAmount` includes only the unticked commitment(s). This will protect against a bug where any ticked payment for the month incorrectly hides the entire “Ce mois” line.
Suggested implementation:
```typescript
it('nets the ticked instalments out of the total', async () => {
await renderCard({
commitments: [carLoan],
paidKeysByCommitment: { car: ['2026-1', '2026-2'] },
});
expect(screen.getByTestId('engagements-total-remaining')).toHaveTextContent(/3[ ]700/);
});
it('includes only unticked commitments in the current-month due amount when some are already paid', async () => {
const secondCommitment = {
...carLoan,
key: 'car-2',
};
await renderCard({
commitments: [carLoan, secondCommitment],
paidKeysByCommitment: { car: ['2026-3'] },
currentPeriod: { year: 2026, month: 3 },
});
const banner = screen.getByTestId('engagements-due-this-month');
expect(banner).toBeInTheDocument();
expect(banner).toHaveTextContent(/250/);
});
import messages from '../../../../messages/fr-BE.json';
```
- If `carLoan` does not represent a 250-per-month commitment (or if the “due this month” amount is different), adjust the `toHaveTextContent(/250/)` expectation to the correct per-period amount for a single commitment.
- If `CommitmentRow` requires additional unique fields beyond `key` to distinguish commitments (e.g. an `id`), also spread and override those in `secondCommitment` to avoid clashes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…state (Sourcery #235) Both suggestions were legitimate — the card's aggregation was only exercised with a single commitment. Added: total summed across two commitments with different ledgers (3 950 + 1 200 = 5 150 €); due-this-month counting ONLY the untricked ones when some are already paid (200 €, not 450 €); the figure disappearing when every commitment is ticked while the card itself stays; and a settled commitment dropping out of both the list and the total while a live one remains. 12 card tests.
thierryvm
added a commit
that referenced
this pull request
Jul 20, 2026
… read (Sourcery #235) The row→domain projection was duplicated between the dashboard card and the commitments page — exactly the kind of copy that drifts. Extracted to one definition. Extracting it into the data module naively broke every UI test: importing the mapper pulled → the Supabase client → server-only env parsing into the component bundle. So the pure part (type + mapper) now lives in its own with ZERO Supabase import, and the impure read next door consumes and re-exports it. UI and tests import the pure module. Full suite 1576 green.
… read (Sourcery #235) The row-to-domain projection was duplicated between the dashboard card and the commitments page — exactly the kind of copy that drifts. Extracted to one definition. Extracting it into the data module naively broke every UI test: importing the mapper pulled getCommitmentsWithLedger, hence the Supabase client and its server-only env parsing, into the component bundle. So the pure part (type + mapper) now lives in its own commitment-row.ts with ZERO Supabase import, and the impure read next door consumes and re-exports it. UI and tests import the pure module. Full suite 1576 green.
thierryvm
force-pushed
the
feat/commitments-cockpit
branch
from
July 20, 2026 22:30
65771e3 to
135286a
Compare
Owner
Author
|
Retours globaux traités (135286a) :
Les 2 suggestions inline (agrégation multi-engagements + état mixte) ont été appliquées dans 12b4f14. |
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.
PR-3 — La carte « Mes engagements » sur le dashboard (épic Dettes & échéanciers)
Le cockpit répond maintenant à la question du long terme juste à côté de celle du mois : combien je dois encore au total, et ce qui tombe ce mois-ci.
La carte
Elle se place juste après « Prochaines factures » : les factures répondent au mois, les engagements au long terme.
Cohérence garantie techniquement
Nouveau helper
getCommitmentsWithLedger()utilisé par la page ET la carte — impossible que les deux surfaces affichent un solde différent. La requête inline de la page est remplacée par ce helper (mêmes garanties : RLS,select('*')anti-page-blanche).Tests
8 tests de carte : netting du solde après coches, « ce mois » qui disparaît une fois coché, engagement soldé qui sort, facture ponctuelle sans jargon, inactif ignoré. Suite complète 1572 verte, typecheck/lint/build clean.
Smoke @Thierry
Dashboard → sous « Prochaines factures », la carte apparaît dès que tu as créé un engagement. Coche l'échéance du mois sur
/app/commitments→ reviens au dashboard : le total a baissé et « Ce mois » a disparu.Reste de l'épic (PR-4, séparée volontairement)
L'effort lissé corrigé (une dette qui se termine ne doit plus être comptée à l'infini) touche le cœur du calcul budgétaire — il mérite sa propre PR avec les agents QA financiers, plutôt que d'être glissé ici. Le bouton « convertir une charge » (décision D4) l'accompagnera, puisqu'il change lui aussi ce qui entre dans le calcul.
🤖 Generated with Claude Code
Summary by Sourcery
Ajouter une carte récapitulative des engagements long terme au tableau de bord et centraliser le chargement des données d’engagements afin que le tableau de bord et la page des engagements partagent une vision cohérente de ce qui est dû.
Nouvelles fonctionnalités :
Améliorations :
CommitmentRowtraversant la frontière RSCTests :
Original summary in English
Summary by Sourcery
Add a long-term commitments overview card to the dashboard and centralize commitments data loading so the dashboard and commitments page share a consistent view of what is owed.
New Features:
Enhancements:
Tests: