feat(commitments): Engagements page — CRUD, remaining balance, progress, ticks (épic PR-2)#234
Merged
Conversation
…rogress, instalment ticks (épic PR-2)
The page that makes the model concrete: for every commitment you see WHAT IS
LEFT to pay and how far along you are, and ticking an instalment moves both.
- /app/commitments (nav: Charges · Engagements · Dépenses). Server page reads
commitments + their payment ledger (RLS-scoped, select('*') so a schema/deploy
window can never blank the page), maps to the pure domain client-side.
- Each row: remaining balance counting down to a ✓ 0 €, a progress bar
(role=progressbar + aria-valuenow), 'x/y échéances de Z · dernière en {mois}'
for schedules or a single dated amount for a one-off, and a 44px tick shown
ONLY when an instalment falls due in the viewed period. Header carries the
total still owed across every commitment.
- 4 Server Actions cloned from the charges contract: uuid guard, workspace
authz (RLS + explicit filter), rate-limit, audit, revalidate. The toggle
reads the instalment amount FROM THE COMMITMENT — never from the client —
and is idempotent (INSERT paid / DELETE unpaid) behind the unique index.
- Add form collapsed by default, instalment fields hidden for a one-off.
i18n x5: app.commitments.* + errors.commitments.* + nav label.
22 new tests (11 UI incl. optimistic tick + balance arithmetic, 11 action incl.
cross-workspace authz). Full suite 1554 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/de la relecteur·riceImplémente la nouvelle page Engagements (/app/commitments) avec des actions serveur CRUD complètes, une UI optimiste pour les coches de paiement, les calculs de solde restant/progression, ainsi que les tests associés, le tout connecté à la navigation et à la journalisation d’audit, en miroir du contrat existant pour les charges. Diagramme de séquence pour l’action serveur de bascule du paiement d’un engagementsequenceDiagram
actor User
participant CommitmentsClient
participant toggleCommitmentPaymentAction
participant authorizedWorkspace
participant Supabase as Supabase_DB
participant logAuditEvent
participant revalidateCommitments
User->>CommitmentsClient: click paid tick
CommitmentsClient->>toggleCommitmentPaymentAction: toggleCommitmentPaymentAction(commitmentId, periodYear, periodMonth)
toggleCommitmentPaymentAction->>authorizedWorkspace: authorizedWorkspace()
authorizedWorkspace-->>toggleCommitmentPaymentAction: { ok: true, userId, workspaceId }
toggleCommitmentPaymentAction->>Supabase: select commitments by id, workspace_id
Supabase-->>toggleCommitmentPaymentAction: commitment(total_amount, installment_amount, installments_total)
toggleCommitmentPaymentAction->>Supabase: select commitment_payments by commitment_id, period_year, period_month
alt existing payment
Supabase-->>toggleCommitmentPaymentAction: existing payment
toggleCommitmentPaymentAction->>Supabase: delete from commitment_payments by id, workspace_id
Supabase-->>toggleCommitmentPaymentAction: delete ok
toggleCommitmentPaymentAction->>logAuditEvent: logAuditEvent(COMMITMENT_PAYMENT_TOGGLED)
toggleCommitmentPaymentAction->>revalidateCommitments: revalidateCommitments()
toggleCommitmentPaymentAction-->>CommitmentsClient: { ok: true, data: { paid: false } }
else no existing payment
Supabase-->>toggleCommitmentPaymentAction: no existing payment
toggleCommitmentPaymentAction->>Supabase: insert into commitment_payments(paid_amount, workspace_id, period_year, period_month)
Supabase-->>toggleCommitmentPaymentAction: insert ok
toggleCommitmentPaymentAction->>logAuditEvent: logAuditEvent(COMMITMENT_PAYMENT_TOGGLED)
toggleCommitmentPaymentAction->>revalidateCommitments: revalidateCommitments()
toggleCommitmentPaymentAction-->>CommitmentsClient: { ok: true, data: { paid: true } }
end
CommitmentsClient-->>User: update optimistic tick and remaining balance
Diagramme de flux pour le chargement et le rendu de la page des engagementsflowchart LR
U[User
/app/commitments] --> P[CommitmentsPage]
P --> WS[getWorkspaceSnapshot]
P --> L[getLocale]
P --> S[Supabase
createClient]
S --> CQ[select * from commitments
where workspace_id]
S --> PQ[select commitment_id,
period_year, period_month
from commitment_payments]
CQ --> PM[map rows to RawCommitment]
PQ --> PK[build paidKeysByCommitment]
PM --> CC[CommitmentsClient]
PK --> CC
WS --> CC
L --> CC
CC --> UI[Commitments list,
remaining balance,
progress bars,
payment ticks]
Changements au niveau des fichiers
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 Engagements (/app/commitments) page with full CRUD server actions, optimistic UI for payment ticks, remaining balance/progress calculations, and associated tests, wired into navigation and audit logging, mirroring the existing charges contract. Sequence diagram for commitment payment toggle server actionsequenceDiagram
actor User
participant CommitmentsClient
participant toggleCommitmentPaymentAction
participant authorizedWorkspace
participant Supabase as Supabase_DB
participant logAuditEvent
participant revalidateCommitments
User->>CommitmentsClient: click paid tick
CommitmentsClient->>toggleCommitmentPaymentAction: toggleCommitmentPaymentAction(commitmentId, periodYear, periodMonth)
toggleCommitmentPaymentAction->>authorizedWorkspace: authorizedWorkspace()
authorizedWorkspace-->>toggleCommitmentPaymentAction: { ok: true, userId, workspaceId }
toggleCommitmentPaymentAction->>Supabase: select commitments by id, workspace_id
Supabase-->>toggleCommitmentPaymentAction: commitment(total_amount, installment_amount, installments_total)
toggleCommitmentPaymentAction->>Supabase: select commitment_payments by commitment_id, period_year, period_month
alt existing payment
Supabase-->>toggleCommitmentPaymentAction: existing payment
toggleCommitmentPaymentAction->>Supabase: delete from commitment_payments by id, workspace_id
Supabase-->>toggleCommitmentPaymentAction: delete ok
toggleCommitmentPaymentAction->>logAuditEvent: logAuditEvent(COMMITMENT_PAYMENT_TOGGLED)
toggleCommitmentPaymentAction->>revalidateCommitments: revalidateCommitments()
toggleCommitmentPaymentAction-->>CommitmentsClient: { ok: true, data: { paid: false } }
else no existing payment
Supabase-->>toggleCommitmentPaymentAction: no existing payment
toggleCommitmentPaymentAction->>Supabase: insert into commitment_payments(paid_amount, workspace_id, period_year, period_month)
Supabase-->>toggleCommitmentPaymentAction: insert ok
toggleCommitmentPaymentAction->>logAuditEvent: logAuditEvent(COMMITMENT_PAYMENT_TOGGLED)
toggleCommitmentPaymentAction->>revalidateCommitments: revalidateCommitments()
toggleCommitmentPaymentAction-->>CommitmentsClient: { ok: true, data: { paid: true } }
end
CommitmentsClient-->>User: update optimistic tick and remaining balance
Flow diagram for loading and rendering the commitments pageflowchart LR
U[User
/app/commitments] --> P[CommitmentsPage]
P --> WS[getWorkspaceSnapshot]
P --> L[getLocale]
P --> S[Supabase
createClient]
S --> CQ[select * from commitments
where workspace_id]
S --> PQ[select commitment_id,
period_year, period_month
from commitment_payments]
CQ --> PM[map rows to RawCommitment]
PQ --> PK[build paidKeysByCommitment]
PM --> CC[CommitmentsClient]
PK --> CC
WS --> CC
L --> CC
CC --> UI[Commitments list,
remaining balance,
progress bars,
payment ticks]
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.
Salut – j’ai trouvé 7 problèmes et laissé quelques retours de haut niveau :
- La logique de coche optimiste dans
CommitmentsClientne rétablit jamais l’état de l’UI quandtoggleCommitmentPaymentActionéchoue, ce qui fait que les utilisateurs peuvent voir un mois marqué comme payé alors qu’il ne l’est pas réellement ; envisage de revenir sur leSetoptimiste en cas d’erreur pour garder l’affichage cohérent avec le backend. - Dans le handler
onCreatedeCommitmentsClient, seultotalAmountest validé avant l’envoi, alors queinstallmentAmountetinstallmentsTotalpeuvent êtreNaNpour les engagements qui ne sont pas en one‑shot ; ajouter une validation basique côté client pour ces champs éviterait des allers‑retours inutiles et des toasts d’erreur déroutants.
Prompt pour les agents IA
Please address the comments from this code review:
## Overall Comments
- The optimistic tick logic in `CommitmentsClient` never rolls back the UI state when `toggleCommitmentPaymentAction` fails, so users can end up seeing a paid month that is actually unpaid; consider reverting the optimistic Set on error to keep the display consistent with the backend.
- In the `onCreate` handler for `CommitmentsClient`, only `totalAmount` is validated before submitting, while `installmentAmount` and `installmentsTotal` can be NaN for non-one-off commitments; adding basic client-side validation for these fields would avoid avoidable round-trips and confusing error toasts.
## Individual Comments
### Comment 1
<location path="src/app/[locale]/app/commitments/CommitmentsClient.tsx" line_range="125-127" />
<code_context>
+ function onTogglePaid(c: RawCommitment) {
+ const entry = `${c.id}|${periodKey(currentPeriod.year, currentPeriod.month)}`;
+ startTransition(async () => {
+ applyOptimisticPaid(entry);
+ try {
+ const result = await toggleCommitmentPaymentAction({
+ commitmentId: c.id,
+ periodYear: currentPeriod.year,
</code_context>
<issue_to_address>
**issue (bug_risk):** Optimistic paid state is never reverted when the toggle action fails, which can desync UI from the backend.
In `onTogglePaid`, the optimistic state is updated before the server call and never reverted if `toggleCommitmentPaymentAction` fails or throws. This leaves the UI showing "paid" when the backend rejected the change (e.g. rate limiting, auth failures). Please revert the optimistic update in both the non-`ok` path and the `catch` block so the UI always reflects the persisted state.
</issue_to_address>
### Comment 2
<location path="src/app/[locale]/app/commitments/CommitmentsClient.tsx" line_range="353-356" />
<code_context>
+ const tickedThisPeriod = paidKeys.has(
+ periodKey(currentPeriod.year, currentPeriod.month),
+ );
+ const progress = Math.round((paid / c.installmentsTotal) * 100);
+ const finished = paid >= c.installmentsTotal;
+
</code_context>
<issue_to_address>
**suggestion:** Progress percentage is not clamped, and assumes installmentsTotal is always > 0.
This relies on `Math.round((paid / c.installmentsTotal) * 100)` without validation. If `installmentsTotal <= 0` (e.g. bad data) you can get NaN, and if `paid > installmentsTotal` you can exceed 100% for `aria-valuenow` and the width style. Consider guarding against non‑positive `installmentsTotal` and clamping the computed percentage to `[0, 100]` before rendering.
```suggestion
const dueThisPeriod = isDueInPeriod(domain, currentPeriod);
const tickedThisPeriod = paidKeys.has(
periodKey(currentPeriod.year, currentPeriod.month),
);
const rawProgress =
c.installmentsTotal > 0 ? (paid / c.installmentsTotal) * 100 : 0;
const progress = Math.min(
100,
Math.max(0, Math.round(rawProgress)),
);
const finished =
c.installmentsTotal > 0 ? paid >= c.installmentsTotal : false;
```
</issue_to_address>
### Comment 3
<location path="src/lib/actions/__tests__/commitments.test.ts" line_range="254" />
<code_context>
+ });
+});
+
+describe('toggleCommitmentPaymentAction', () => {
+ const period = { commitmentId: COMMITMENT_ID, periodYear: 2026, periodMonth: 8 };
+
</code_context>
<issue_to_address>
**suggestion (testing):** Toggle payment tests miss several important error and edge-case branches
The existing tests cover the main happy paths, but several failure branches are untested:
- Rate limit failure (`rateLimit` returning `success: false`) should yield `errors.session.rateLimited` (currently only covered for create).
- DB failures for both deleting an existing tick and inserting a new payment should return `errors.commitments.payments.toggleFailed`.
- `logAuditEvent` isn’t error-handled, but tests should still verify it’s called for both insert and delete to prevent regressions.
Adding 2–3 tests for these cases will validate behaviour under failures and ensure error codes stay aligned with the UI contract.
Suggested implementation:
```typescript
describe('toggleCommitmentPaymentAction', () => {
const period = { commitmentId: COMMITMENT_ID, periodYear: 2026, periodMonth: 8 };
it('returns a rateLimited error when the rate limiter fails', async () => {
// Arrange: rate limit fails before any DB access
(rateLimit as jest.Mock).mockResolvedValue({ success: false });
const result = await toggleCommitmentPaymentAction(period);
expect(result).toEqual({
ok: false,
error: errors.session.rateLimited,
});
// No audit events or DB operations should have happened
expect(auditSpy).not.toHaveBeenCalled();
});
it('returns toggleFailed when deleting an existing payment fails', async () => {
// Arrange: rate limiter allows the operation
(rateLimit as jest.Mock).mockResolvedValue({ success: true });
// First select finds an existing payment for the period
supa.program({
table: 'commitments_payments',
op: 'select',
result: {
data: [
{
commitment_id: COMMITMENT_ID,
period_year: period.periodYear,
period_month: period.periodMonth,
},
],
error: null,
},
});
// Delete of existing payment fails
supa.program({
table: 'commitments_payments',
op: 'delete',
result: {
data: null,
error: { message: 'delete failed' },
},
});
const result = await toggleCommitmentPaymentAction(period);
expect(result).toEqual({
ok: false,
error: errors.commitments.payments.toggleFailed,
});
// Audit event should not be written on failure
expect(auditSpy).not.toHaveBeenCalled();
});
it('returns toggleFailed when inserting a new payment fails', async () => {
// Arrange: rate limiter allows the operation
(rateLimit as jest.Mock).mockResolvedValue({ success: true });
// First select finds no existing payment for the period
supa.program({
table: 'commitments_payments',
op: 'select',
result: {
data: [],
error: null,
},
});
// Insert of new payment fails
supa.program({
table: 'commitments_payments',
op: 'insert',
result: {
data: null,
error: { message: 'insert failed' },
},
});
const result = await toggleCommitmentPaymentAction(period);
expect(result).toEqual({
ok: false,
error: errors.commitments.payments.toggleFailed,
});
// Audit event should not be written on failure
expect(auditSpy).not.toHaveBeenCalled();
});
it('logs an audit event when inserting a new payment succeeds', async () => {
(rateLimit as jest.Mock).mockResolvedValue({ success: true });
// No existing payment
supa.program({
table: 'commitments_payments',
op: 'select',
result: {
data: [],
error: null,
},
});
// Successful insert
supa.program({
table: 'commitments_payments',
op: 'insert',
result: {
data: [
{
commitment_id: COMMITMENT_ID,
period_year: period.periodYear,
period_month: period.periodMonth,
},
],
error: null,
},
});
const result = await toggleCommitmentPaymentAction(period);
expect(result.ok).toBe(true);
expect(auditSpy).toHaveBeenCalledWith('commitment.payment.toggled', {
userId: 'user-1',
workspaceId: 'ws-1',
commitmentId: COMMITMENT_ID,
periodYear: period.periodYear,
periodMonth: period.periodMonth,
action: 'insert',
});
});
it('logs an audit event when deleting an existing payment succeeds', async () => {
(rateLimit as jest.Mock).mockResolvedValue({ success: true });
// Existing payment present
supa.program({
table: 'commitments_payments',
op: 'select',
result: {
data: [
{
commitment_id: COMMITMENT_ID,
period_year: period.periodYear,
period_month: period.periodMonth,
},
],
error: null,
},
});
// Successful delete
supa.program({
table: 'commitments_payments',
op: 'delete',
result: {
data: null,
error: null,
},
});
const result = await toggleCommitmentPaymentAction(period);
expect(result.ok).toBe(true);
expect(auditSpy).toHaveBeenCalledWith('commitment.payment.toggled', {
userId: 'user-1',
workspaceId: 'ws-1',
commitmentId: COMMITMENT_ID,
periodYear: period.periodYear,
periodMonth: period.periodMonth,
action: 'delete',
});
});
```
1. The tests above assume a mocked `rateLimit` function is already imported and jest-mocked in this file (similar to how the create-commitment-payment tests stub rate limiting). If your test suite uses a different helper (e.g. `rateLimitOrThrow`, `rateLimiter`, `limit`), replace `(rateLimit as jest.Mock)` with the correct mock variable used elsewhere in this file.
2. The error objects `errors.session.rateLimited` and `errors.commitments.payments.toggleFailed` are used as suggested in your review comment. If the existing tests assert on `error.code`/`error.message` instead of using the exported constants directly, align these expectations with the existing convention.
3. The audit event name `'commitment.payment.toggled'` and payload shape in the `toHaveBeenCalledWith` assertions are inferred. Update the event name and payload keys to match whatever `toggleCommitmentPaymentAction` actually passes to `logAuditEvent` (you can mirror this from the existing “happy path” toggle tests).
4. The `supa.program` scripting for `commitments_payments` should match the other tests in this file; if you use a different table name or column names for commitment payments, adjust the scripted `data` objects accordingly so the action detects “existing vs. non-existing” payments in the same way as the current tests.
</issue_to_address>
### Comment 4
<location path="src/lib/actions/__tests__/commitments.test.ts" line_range="221-226" />
<code_context>
+ expect(supa.lastInsertPayload()).toMatchObject({ installment_amount: null });
+ });
+
+ it('rejects invalid input before touching the DB', async () => {
+ programMembership();
+ const r = await createCommitmentAction({ ...VALID_INPUT, totalAmount: -1 });
+ expect(r.ok).toBe(false);
+ expect(auditSpy).not.toHaveBeenCalled();
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Validation failure branch should assert `fieldErrors` shape returned by `createCommitmentAction`
This only asserts `ok` is false and no audit event is emitted, but doesn’t verify the `fieldErrors` contract from `commitmentInputSchema`. Since the action returns `fieldErrors: parsed.error.flatten().fieldErrors` and the UI likely relies on this shape, please add an assertion along the lines of:
```ts
expect(r).toMatchObject({
ok: false,
errorCode: 'errors.validation.generic',
fieldErrors: expect.objectContaining({ totalAmount: expect.any(Array) }),
});
```
to lock in the expected validation error shape and prevent regressions in error handling.
```suggestion
it('rejects invalid input before touching the DB', async () => {
programMembership();
const r = await createCommitmentAction({ ...VALID_INPUT, totalAmount: -1 });
expect(r.ok).toBe(false);
expect(r).toMatchObject({
ok: false,
errorCode: 'errors.validation.generic',
fieldErrors: expect.objectContaining({
totalAmount: expect.any(Array),
}),
});
expect(auditSpy).not.toHaveBeenCalled();
});
```
</issue_to_address>
### Comment 5
<location path="src/app/[locale]/app/commitments/__tests__/CommitmentsClient.test.tsx" line_range="140-149" />
<code_context>
+ it('ticks an instalment optimistically and calls the action with the viewed period', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Optimistic tick test does not assert that remaining balance and progress bar update immediately
The header comment says ticking an instalment should immediately update both remaining balance and progress. This test only checks the optimistic aria state and action call. To fully cover the behaviour, also assert the numeric UI before and after the click, e.g. capture `remaining` and `progress`, assert their initial values, trigger the click, then `waitFor` the updated remaining amount and `aria-valuenow` to reflect the optimistic change.
Suggested implementation:
```typescript
it('ticks an instalment optimistically and calls the action with the viewed period', async () => {
let resolveAction!: (v: { ok: true; data: { paid: boolean } }) => void;
toggleMock.mockImplementation(
() =>
new Promise((res) => {
resolveAction = res;
}),
);
renderPage([carLoan], { currentPeriod: { year: 2026, month: 3 } });
const tick = screen.getByTestId('commitment-paid-car');
expect(tick).toHaveAttribute('aria-pressed', 'false');
// Capture initial remaining balance and progress for the car commitment
const remaining = screen.getByTestId('commitment-remaining-car');
const progress = screen.getByTestId('commitment-progress-car');
const initialRemaining = remaining.textContent;
const initialProgress = progress.getAttribute('aria-valuenow');
// Tick the instalment optimistically
await userEvent.click(tick);
// Remaining balance and progress bar should update immediately (optimistic UI),
// before the toggle action promise resolves.
await waitFor(() => {
expect(remaining.textContent).not.toBe(initialRemaining);
expect(progress.getAttribute('aria-valuenow')).not.toBe(initialProgress);
});
```
1. Ensure there are elements in the rendered UI with `data-testid="commitment-remaining-car"` and `data-testid="commitment-progress-car"`, and that the progress element has an `aria-valuenow` attribute. If your existing tests use different test IDs (e.g. `commitment-remaining-car-loan` or a generic pattern), adjust the test IDs in this change to match.
2. Confirm that `userEvent` and `waitFor` are already imported in this test file (typically from `@testing-library/user-event` and `@testing-library/react`). If not, add their imports at the top of the file in line with existing conventions.
3. If your progress UI is accessed differently (for example via `getByRole('progressbar', { name: /car/i })` rather than a `data-testid`), adapt the `progress` query to the pattern already used elsewhere in the test suite.
</issue_to_address>
### Comment 6
<location path="src/app/[locale]/app/commitments/__tests__/CommitmentsClient.test.tsx" line_range="178-187" />
<code_context>
+ it('creates a commitment with the anchor set to the viewed period', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Creation tests should also cover failure scenarios and input validation feedback
This test covers the happy path well. To better exercise the client behaviour, please also add tests for:
- A failed creation (`createCommitmentAction` returns `{ ok: false, errorCode: ... }`): assert `toastErrorMock` is called and form values are preserved.
- The local validation branch in `onCreate` (e.g. `totalAmount < 0` or non-numeric input): assert an error toast is shown and `createCommitmentAction` is **not** called.
These will verify the UI for both server rejection and immediate numeric validation failures.
Suggested implementation:
```typescript
it('creates a commitment with the anchor set to the viewed period', async () => {
createMock.mockResolvedValue({ ok: true });
renderPage([], { currentPeriod: { year: 2026, month: 5 } });
fireEvent.click(screen.getByTestId('commitments-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Arrangement SPF' } });
fireEvent.change(screen.getByLabelText(/Montant restant dû/), { target: { value: '1600' } });
fireEvent.change(screen.getByLabelText(/Montant par échéance/), { target: { value: '200' } });
fireEvent.change(screen.getByLabelText(/Nombre d'échéances/), { target: { value: '8' } });
await act(async () => {
fireEvent.submit(screen.getByRole('button', { name: /^ajouter$/i }).closest('form')!);
});
```
` section so they can be placed after the full test definition in your file.
Here are the code changes:
<file_operations>
<file_operation operation="edit" file_path="src/app/[locale]/app/commitments/__tests__/CommitmentsClient.test.tsx">
<<<<<<< SEARCH
it('creates a commitment with the anchor set to the viewed period', async () => {
createMock.mockResolvedValue({ ok: true });
renderPage([], { currentPeriod: { year: 2026, month: 5 } });
fireEvent.click(screen.getByTestId('commitments-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Arrangement SPF' } });
fireEvent.change(screen.getByLabelText(/Montant restant dû/), { target: { value: '1600' } });
fireEvent.change(screen.getByLabelText(/Montant par échéance/), { target: { value: '200' } });
fireEvent.change(screen.getByLabelText(/Nombre d'échéances/), { target: { value: '8' } });
await act(async () => {
fireEvent.submit(screen.getByRole('button', { name: /^ajouter$/i }).closest('form')!);
});
=======
it('creates a commitment with the anchor set to the viewed period', async () => {
createMock.mockResolvedValue({ ok: true });
renderPage([], { currentPeriod: { year: 2026, month: 5 } });
fireEvent.click(screen.getByTestId('commitments-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Arrangement SPF' } });
fireEvent.change(screen.getByLabelText(/Montant restant dû/), { target: { value: '1600' } });
fireEvent.change(screen.getByLabelText(/Montant par échéance/), { target: { value: '200' } });
fireEvent.change(screen.getByLabelText(/Nombre d'échéances/), { target: { value: '8' } });
await act(async () => {
fireEvent.submit(screen.getByRole('button', { name: /^ajouter$/i }).closest('form')!);
});
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
Please add the following two tests **after** the full body of the existing `"creates a commitment with the anchor set to the viewed period"` test, alongside the other `it(...)` blocks in the same `describe`:
```ts
it('shows an error toast and preserves form values when creation fails on the server', async () => {
createMock.mockResolvedValue({
ok: false,
errorCode: 'errors.commitments.createFailed',
});
renderPage([], { currentPeriod: { year: 2026, month: 5 } });
fireEvent.click(screen.getByTestId('commitments-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Arrangement SPF' } });
fireEvent.change(screen.getByLabelText(/Montant restant dû/), { target: { value: '1600' } });
fireEvent.change(screen.getByLabelText(/Montant par échéance/), { target: { value: '200' } });
fireEvent.change(screen.getByLabelText(/Nombre d'échéances/), { target: { value: '8' } });
await act(async () => {
fireEvent.submit(screen.getByRole('button', { name: /^ajouter$/i }).closest('form')!);
});
await waitFor(() => {
expect(toastErrorMock).toHaveBeenCalled();
});
// Form values should be preserved after a failed creation
expect(screen.getByLabelText('Libellé')).toHaveValue('Arrangement SPF');
expect(screen.getByLabelText(/Montant restant dû/)).toHaveValue('1600');
expect(screen.getByLabelText(/Montant par échéance/)).toHaveValue('200');
expect(screen.getByLabelText(/Nombre d'échéances/)).toHaveValue('8');
});
it('shows a validation error and does not call create when the total amount is invalid', async () => {
createMock.mockResolvedValue({ ok: true }); // should not be reached
renderPage([], { currentPeriod: { year: 2026, month: 5 } });
fireEvent.click(screen.getByTestId('commitments-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Crédit auto' } });
// Trigger local validation: invalid / negative total amount
fireEvent.change(screen.getByLabelText(/Montant restant dû/), { target: { value: '-10' } });
fireEvent.change(screen.getByLabelText(/Montant par échéance/), { target: { value: '200' } });
fireEvent.change(screen.getByLabelText(/Nombre d'échéances/), { target: { value: '8' } });
await act(async () => {
fireEvent.submit(screen.getByRole('button', { name: /^ajouter$/i }).closest('form')!);
});
await waitFor(() => {
expect(toastErrorMock).toHaveBeenCalled();
});
expect(createMock).not.toHaveBeenCalled();
});
```
You may want to adjust:
- The `errorCode` string (`'errors.commitments.createFailed'`) to match the actual error code used by `createCommitmentAction`.
- The specific label texts / regexes if they differ slightly elsewhere in the file.
These tests will cover both the server-side failure branch and the local numeric validation branch in `onCreate`, verifying that the UI shows an error toast, preserves values on server failure, and avoids calling `createCommitmentAction` for immediately invalid input.
</issue_to_address>
### Comment 7
<location path="src/app/[locale]/app/commitments/CommitmentsClient.tsx" line_range="62" />
<code_context>
+ one_off: 'kinds.oneOff',
+} as const;
+
+export function CommitmentsClient({
+ commitments,
+ paidKeysByCommitment,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the optimistic paid ledger logic into a custom hook and the per-item rendering into a `CommitmentRow` component to break this large component into smaller, focused units.
You can reduce complexity without changing behaviour by extracting a couple of focused building blocks.
### 1. Encapsulate the optimistic paid ledger
The `${id}|${key}` encoding + `Set` logic is sprinkled in multiple places (`paidBase`, `applyOptimisticPaid`, `paidKeysOf`, `onTogglePaid`, `tickedThisPeriod`). Moving that into a hook will shrink the main component and centralise the representation.
For example:
```ts
// commitments/hooks/useOptimisticPaidLedger.ts
type Period = { year: number; month: number };
export function useOptimisticPaidLedger(
paidKeysByCommitment: Record<string, string[]>,
) {
const base = useMemo(() => {
const set = new Set<string>();
for (const [id, keys] of Object.entries(paidKeysByCommitment)) {
for (const k of keys) set.add(`${id}|${k}`);
}
return set;
}, [paidKeysByCommitment]);
const [optimisticPaid, applyOptimisticPaid] = useOptimistic(
base,
(current: ReadonlySet<string>, entry: string) => {
const next = new Set(current);
next.has(entry) ? next.delete(entry) : next.add(entry);
return next;
},
);
const periodKeyOf = (p: Period) => `${p.year}-${p.month}`;
const paidKeysOf = (id: string): ReadonlySet<string> => {
const prefix = `${id}|`;
const keys = new Set<string>();
for (const entry of optimisticPaid) {
if (entry.startsWith(prefix)) keys.add(entry.slice(prefix.length));
}
return keys;
};
const togglePaidOptimistic = (id: string, period: Period) => {
applyOptimisticPaid(`${id}|${periodKeyOf(period)}`);
};
return { paidKeysOf, togglePaidOptimistic };
}
```
Then in `CommitmentsClient`:
```ts
const { paidKeysOf, togglePaidOptimistic } = useOptimisticPaidLedger(
paidKeysByCommitment,
);
function onTogglePaid(c: RawCommitment) {
startTransition(async () => {
togglePaidOptimistic(c.id, currentPeriod);
// keep the rest of the action logic unchanged
});
}
```
This removes the string concatenation details from the main component and makes the payment-toggling code easier to scan.
### 2. Extract a `CommitmentRow` component
The `map` body currently computes a view model + renders a dense `<li>`. Extracting that into a row component reduces the size of `CommitmentsClient` and isolates per-row logic.
First, compute the minimal view model in the parent:
```ts
const rows = active.map((c) => {
const domain = toDomain(c);
const paidKeys = paidKeysOf(c.id);
const paid = installmentsPaid(domain, paidKeys);
const remaining = remainingBalance(domain, paidKeys);
const end = endPeriod(domain);
const dueThisPeriod = isDueInPeriod(domain, currentPeriod);
const tickedThisPeriod = paidKeys.has(periodKey(currentPeriod.year, currentPeriod.month));
const progress = Math.round((paid / c.installmentsTotal) * 100);
const finished = paid >= c.installmentsTotal;
return {
raw: c,
paid,
remaining,
end,
dueThisPeriod,
tickedThisPeriod,
progress,
finished,
};
});
```
Then render via a dedicated component:
```tsx
<ul role="list" className="divide-border/60 divide-y" data-testid="commitments-list">
{rows.map((row) => (
<CommitmentRow
key={row.raw.id}
row={row}
locale={locale}
onTogglePaid={() => onTogglePaid(row.raw)}
onDelete={() => onDelete(row.raw.id)}
isPending={isPending}
t={t}
/>
))}
</ul>
```
`CommitmentRow` keeps only row-level concerns:
```tsx
type CommitmentRowProps = {
row: {
raw: RawCommitment;
paid: number;
remaining: number;
end: { year: number; month: number };
dueThisPeriod: boolean;
tickedThisPeriod: boolean;
progress: number;
finished: boolean;
};
locale: Locale;
isPending: boolean;
onTogglePaid(): void;
onDelete(): void;
t: ReturnType<typeof useTranslations<'app.commitments'>>;
};
function CommitmentRow({
row: { raw: c, paid, remaining, end, dueThisPeriod, tickedThisPeriod, progress, finished },
locale,
isPending,
onTogglePaid,
onDelete,
t,
}: CommitmentRowProps) {
return (
<li data-testid={`commitment-row-${c.id}`} className="relative py-4 pr-24">
{/* existing JSX for header, progress bar, summary, buttons … */}
</li>
);
}
```
This keeps all behaviour intact but breaks the 470-line monolith into smaller, single-responsibility units, making the main component mostly orchestration of hooks + child components.
</issue_to_address>Sourcery est gratuit pour l’open source – si nos revues te sont utiles, merci d’en parler ✨
Original comment in English
Hey - I've found 7 issues, and left some high level feedback:
- The optimistic tick logic in
CommitmentsClientnever rolls back the UI state whentoggleCommitmentPaymentActionfails, so users can end up seeing a paid month that is actually unpaid; consider reverting the optimistic Set on error to keep the display consistent with the backend. - In the
onCreatehandler forCommitmentsClient, onlytotalAmountis validated before submitting, whileinstallmentAmountandinstallmentsTotalcan be NaN for non-one-off commitments; adding basic client-side validation for these fields would avoid avoidable round-trips and confusing error toasts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The optimistic tick logic in `CommitmentsClient` never rolls back the UI state when `toggleCommitmentPaymentAction` fails, so users can end up seeing a paid month that is actually unpaid; consider reverting the optimistic Set on error to keep the display consistent with the backend.
- In the `onCreate` handler for `CommitmentsClient`, only `totalAmount` is validated before submitting, while `installmentAmount` and `installmentsTotal` can be NaN for non-one-off commitments; adding basic client-side validation for these fields would avoid avoidable round-trips and confusing error toasts.
## Individual Comments
### Comment 1
<location path="src/app/[locale]/app/commitments/CommitmentsClient.tsx" line_range="125-127" />
<code_context>
+ function onTogglePaid(c: RawCommitment) {
+ const entry = `${c.id}|${periodKey(currentPeriod.year, currentPeriod.month)}`;
+ startTransition(async () => {
+ applyOptimisticPaid(entry);
+ try {
+ const result = await toggleCommitmentPaymentAction({
+ commitmentId: c.id,
+ periodYear: currentPeriod.year,
</code_context>
<issue_to_address>
**issue (bug_risk):** Optimistic paid state is never reverted when the toggle action fails, which can desync UI from the backend.
In `onTogglePaid`, the optimistic state is updated before the server call and never reverted if `toggleCommitmentPaymentAction` fails or throws. This leaves the UI showing "paid" when the backend rejected the change (e.g. rate limiting, auth failures). Please revert the optimistic update in both the non-`ok` path and the `catch` block so the UI always reflects the persisted state.
</issue_to_address>
### Comment 2
<location path="src/app/[locale]/app/commitments/CommitmentsClient.tsx" line_range="353-356" />
<code_context>
+ const tickedThisPeriod = paidKeys.has(
+ periodKey(currentPeriod.year, currentPeriod.month),
+ );
+ const progress = Math.round((paid / c.installmentsTotal) * 100);
+ const finished = paid >= c.installmentsTotal;
+
</code_context>
<issue_to_address>
**suggestion:** Progress percentage is not clamped, and assumes installmentsTotal is always > 0.
This relies on `Math.round((paid / c.installmentsTotal) * 100)` without validation. If `installmentsTotal <= 0` (e.g. bad data) you can get NaN, and if `paid > installmentsTotal` you can exceed 100% for `aria-valuenow` and the width style. Consider guarding against non‑positive `installmentsTotal` and clamping the computed percentage to `[0, 100]` before rendering.
```suggestion
const dueThisPeriod = isDueInPeriod(domain, currentPeriod);
const tickedThisPeriod = paidKeys.has(
periodKey(currentPeriod.year, currentPeriod.month),
);
const rawProgress =
c.installmentsTotal > 0 ? (paid / c.installmentsTotal) * 100 : 0;
const progress = Math.min(
100,
Math.max(0, Math.round(rawProgress)),
);
const finished =
c.installmentsTotal > 0 ? paid >= c.installmentsTotal : false;
```
</issue_to_address>
### Comment 3
<location path="src/lib/actions/__tests__/commitments.test.ts" line_range="254" />
<code_context>
+ });
+});
+
+describe('toggleCommitmentPaymentAction', () => {
+ const period = { commitmentId: COMMITMENT_ID, periodYear: 2026, periodMonth: 8 };
+
</code_context>
<issue_to_address>
**suggestion (testing):** Toggle payment tests miss several important error and edge-case branches
The existing tests cover the main happy paths, but several failure branches are untested:
- Rate limit failure (`rateLimit` returning `success: false`) should yield `errors.session.rateLimited` (currently only covered for create).
- DB failures for both deleting an existing tick and inserting a new payment should return `errors.commitments.payments.toggleFailed`.
- `logAuditEvent` isn’t error-handled, but tests should still verify it’s called for both insert and delete to prevent regressions.
Adding 2–3 tests for these cases will validate behaviour under failures and ensure error codes stay aligned with the UI contract.
Suggested implementation:
```typescript
describe('toggleCommitmentPaymentAction', () => {
const period = { commitmentId: COMMITMENT_ID, periodYear: 2026, periodMonth: 8 };
it('returns a rateLimited error when the rate limiter fails', async () => {
// Arrange: rate limit fails before any DB access
(rateLimit as jest.Mock).mockResolvedValue({ success: false });
const result = await toggleCommitmentPaymentAction(period);
expect(result).toEqual({
ok: false,
error: errors.session.rateLimited,
});
// No audit events or DB operations should have happened
expect(auditSpy).not.toHaveBeenCalled();
});
it('returns toggleFailed when deleting an existing payment fails', async () => {
// Arrange: rate limiter allows the operation
(rateLimit as jest.Mock).mockResolvedValue({ success: true });
// First select finds an existing payment for the period
supa.program({
table: 'commitments_payments',
op: 'select',
result: {
data: [
{
commitment_id: COMMITMENT_ID,
period_year: period.periodYear,
period_month: period.periodMonth,
},
],
error: null,
},
});
// Delete of existing payment fails
supa.program({
table: 'commitments_payments',
op: 'delete',
result: {
data: null,
error: { message: 'delete failed' },
},
});
const result = await toggleCommitmentPaymentAction(period);
expect(result).toEqual({
ok: false,
error: errors.commitments.payments.toggleFailed,
});
// Audit event should not be written on failure
expect(auditSpy).not.toHaveBeenCalled();
});
it('returns toggleFailed when inserting a new payment fails', async () => {
// Arrange: rate limiter allows the operation
(rateLimit as jest.Mock).mockResolvedValue({ success: true });
// First select finds no existing payment for the period
supa.program({
table: 'commitments_payments',
op: 'select',
result: {
data: [],
error: null,
},
});
// Insert of new payment fails
supa.program({
table: 'commitments_payments',
op: 'insert',
result: {
data: null,
error: { message: 'insert failed' },
},
});
const result = await toggleCommitmentPaymentAction(period);
expect(result).toEqual({
ok: false,
error: errors.commitments.payments.toggleFailed,
});
// Audit event should not be written on failure
expect(auditSpy).not.toHaveBeenCalled();
});
it('logs an audit event when inserting a new payment succeeds', async () => {
(rateLimit as jest.Mock).mockResolvedValue({ success: true });
// No existing payment
supa.program({
table: 'commitments_payments',
op: 'select',
result: {
data: [],
error: null,
},
});
// Successful insert
supa.program({
table: 'commitments_payments',
op: 'insert',
result: {
data: [
{
commitment_id: COMMITMENT_ID,
period_year: period.periodYear,
period_month: period.periodMonth,
},
],
error: null,
},
});
const result = await toggleCommitmentPaymentAction(period);
expect(result.ok).toBe(true);
expect(auditSpy).toHaveBeenCalledWith('commitment.payment.toggled', {
userId: 'user-1',
workspaceId: 'ws-1',
commitmentId: COMMITMENT_ID,
periodYear: period.periodYear,
periodMonth: period.periodMonth,
action: 'insert',
});
});
it('logs an audit event when deleting an existing payment succeeds', async () => {
(rateLimit as jest.Mock).mockResolvedValue({ success: true });
// Existing payment present
supa.program({
table: 'commitments_payments',
op: 'select',
result: {
data: [
{
commitment_id: COMMITMENT_ID,
period_year: period.periodYear,
period_month: period.periodMonth,
},
],
error: null,
},
});
// Successful delete
supa.program({
table: 'commitments_payments',
op: 'delete',
result: {
data: null,
error: null,
},
});
const result = await toggleCommitmentPaymentAction(period);
expect(result.ok).toBe(true);
expect(auditSpy).toHaveBeenCalledWith('commitment.payment.toggled', {
userId: 'user-1',
workspaceId: 'ws-1',
commitmentId: COMMITMENT_ID,
periodYear: period.periodYear,
periodMonth: period.periodMonth,
action: 'delete',
});
});
```
1. The tests above assume a mocked `rateLimit` function is already imported and jest-mocked in this file (similar to how the create-commitment-payment tests stub rate limiting). If your test suite uses a different helper (e.g. `rateLimitOrThrow`, `rateLimiter`, `limit`), replace `(rateLimit as jest.Mock)` with the correct mock variable used elsewhere in this file.
2. The error objects `errors.session.rateLimited` and `errors.commitments.payments.toggleFailed` are used as suggested in your review comment. If the existing tests assert on `error.code`/`error.message` instead of using the exported constants directly, align these expectations with the existing convention.
3. The audit event name `'commitment.payment.toggled'` and payload shape in the `toHaveBeenCalledWith` assertions are inferred. Update the event name and payload keys to match whatever `toggleCommitmentPaymentAction` actually passes to `logAuditEvent` (you can mirror this from the existing “happy path” toggle tests).
4. The `supa.program` scripting for `commitments_payments` should match the other tests in this file; if you use a different table name or column names for commitment payments, adjust the scripted `data` objects accordingly so the action detects “existing vs. non-existing” payments in the same way as the current tests.
</issue_to_address>
### Comment 4
<location path="src/lib/actions/__tests__/commitments.test.ts" line_range="221-226" />
<code_context>
+ expect(supa.lastInsertPayload()).toMatchObject({ installment_amount: null });
+ });
+
+ it('rejects invalid input before touching the DB', async () => {
+ programMembership();
+ const r = await createCommitmentAction({ ...VALID_INPUT, totalAmount: -1 });
+ expect(r.ok).toBe(false);
+ expect(auditSpy).not.toHaveBeenCalled();
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Validation failure branch should assert `fieldErrors` shape returned by `createCommitmentAction`
This only asserts `ok` is false and no audit event is emitted, but doesn’t verify the `fieldErrors` contract from `commitmentInputSchema`. Since the action returns `fieldErrors: parsed.error.flatten().fieldErrors` and the UI likely relies on this shape, please add an assertion along the lines of:
```ts
expect(r).toMatchObject({
ok: false,
errorCode: 'errors.validation.generic',
fieldErrors: expect.objectContaining({ totalAmount: expect.any(Array) }),
});
```
to lock in the expected validation error shape and prevent regressions in error handling.
```suggestion
it('rejects invalid input before touching the DB', async () => {
programMembership();
const r = await createCommitmentAction({ ...VALID_INPUT, totalAmount: -1 });
expect(r.ok).toBe(false);
expect(r).toMatchObject({
ok: false,
errorCode: 'errors.validation.generic',
fieldErrors: expect.objectContaining({
totalAmount: expect.any(Array),
}),
});
expect(auditSpy).not.toHaveBeenCalled();
});
```
</issue_to_address>
### Comment 5
<location path="src/app/[locale]/app/commitments/__tests__/CommitmentsClient.test.tsx" line_range="140-149" />
<code_context>
+ it('ticks an instalment optimistically and calls the action with the viewed period', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Optimistic tick test does not assert that remaining balance and progress bar update immediately
The header comment says ticking an instalment should immediately update both remaining balance and progress. This test only checks the optimistic aria state and action call. To fully cover the behaviour, also assert the numeric UI before and after the click, e.g. capture `remaining` and `progress`, assert their initial values, trigger the click, then `waitFor` the updated remaining amount and `aria-valuenow` to reflect the optimistic change.
Suggested implementation:
```typescript
it('ticks an instalment optimistically and calls the action with the viewed period', async () => {
let resolveAction!: (v: { ok: true; data: { paid: boolean } }) => void;
toggleMock.mockImplementation(
() =>
new Promise((res) => {
resolveAction = res;
}),
);
renderPage([carLoan], { currentPeriod: { year: 2026, month: 3 } });
const tick = screen.getByTestId('commitment-paid-car');
expect(tick).toHaveAttribute('aria-pressed', 'false');
// Capture initial remaining balance and progress for the car commitment
const remaining = screen.getByTestId('commitment-remaining-car');
const progress = screen.getByTestId('commitment-progress-car');
const initialRemaining = remaining.textContent;
const initialProgress = progress.getAttribute('aria-valuenow');
// Tick the instalment optimistically
await userEvent.click(tick);
// Remaining balance and progress bar should update immediately (optimistic UI),
// before the toggle action promise resolves.
await waitFor(() => {
expect(remaining.textContent).not.toBe(initialRemaining);
expect(progress.getAttribute('aria-valuenow')).not.toBe(initialProgress);
});
```
1. Ensure there are elements in the rendered UI with `data-testid="commitment-remaining-car"` and `data-testid="commitment-progress-car"`, and that the progress element has an `aria-valuenow` attribute. If your existing tests use different test IDs (e.g. `commitment-remaining-car-loan` or a generic pattern), adjust the test IDs in this change to match.
2. Confirm that `userEvent` and `waitFor` are already imported in this test file (typically from `@testing-library/user-event` and `@testing-library/react`). If not, add their imports at the top of the file in line with existing conventions.
3. If your progress UI is accessed differently (for example via `getByRole('progressbar', { name: /car/i })` rather than a `data-testid`), adapt the `progress` query to the pattern already used elsewhere in the test suite.
</issue_to_address>
### Comment 6
<location path="src/app/[locale]/app/commitments/__tests__/CommitmentsClient.test.tsx" line_range="178-187" />
<code_context>
+ it('creates a commitment with the anchor set to the viewed period', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Creation tests should also cover failure scenarios and input validation feedback
This test covers the happy path well. To better exercise the client behaviour, please also add tests for:
- A failed creation (`createCommitmentAction` returns `{ ok: false, errorCode: ... }`): assert `toastErrorMock` is called and form values are preserved.
- The local validation branch in `onCreate` (e.g. `totalAmount < 0` or non-numeric input): assert an error toast is shown and `createCommitmentAction` is **not** called.
These will verify the UI for both server rejection and immediate numeric validation failures.
Suggested implementation:
```typescript
it('creates a commitment with the anchor set to the viewed period', async () => {
createMock.mockResolvedValue({ ok: true });
renderPage([], { currentPeriod: { year: 2026, month: 5 } });
fireEvent.click(screen.getByTestId('commitments-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Arrangement SPF' } });
fireEvent.change(screen.getByLabelText(/Montant restant dû/), { target: { value: '1600' } });
fireEvent.change(screen.getByLabelText(/Montant par échéance/), { target: { value: '200' } });
fireEvent.change(screen.getByLabelText(/Nombre d'échéances/), { target: { value: '8' } });
await act(async () => {
fireEvent.submit(screen.getByRole('button', { name: /^ajouter$/i }).closest('form')!);
});
```
` section so they can be placed after the full test definition in your file.
Here are the code changes:
<file_operations>
<file_operation operation="edit" file_path="src/app/[locale]/app/commitments/__tests__/CommitmentsClient.test.tsx">
<<<<<<< SEARCH
it('creates a commitment with the anchor set to the viewed period', async () => {
createMock.mockResolvedValue({ ok: true });
renderPage([], { currentPeriod: { year: 2026, month: 5 } });
fireEvent.click(screen.getByTestId('commitments-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Arrangement SPF' } });
fireEvent.change(screen.getByLabelText(/Montant restant dû/), { target: { value: '1600' } });
fireEvent.change(screen.getByLabelText(/Montant par échéance/), { target: { value: '200' } });
fireEvent.change(screen.getByLabelText(/Nombre d'échéances/), { target: { value: '8' } });
await act(async () => {
fireEvent.submit(screen.getByRole('button', { name: /^ajouter$/i }).closest('form')!);
});
=======
it('creates a commitment with the anchor set to the viewed period', async () => {
createMock.mockResolvedValue({ ok: true });
renderPage([], { currentPeriod: { year: 2026, month: 5 } });
fireEvent.click(screen.getByTestId('commitments-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Arrangement SPF' } });
fireEvent.change(screen.getByLabelText(/Montant restant dû/), { target: { value: '1600' } });
fireEvent.change(screen.getByLabelText(/Montant par échéance/), { target: { value: '200' } });
fireEvent.change(screen.getByLabelText(/Nombre d'échéances/), { target: { value: '8' } });
await act(async () => {
fireEvent.submit(screen.getByRole('button', { name: /^ajouter$/i }).closest('form')!);
});
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
Please add the following two tests **after** the full body of the existing `"creates a commitment with the anchor set to the viewed period"` test, alongside the other `it(...)` blocks in the same `describe`:
```ts
it('shows an error toast and preserves form values when creation fails on the server', async () => {
createMock.mockResolvedValue({
ok: false,
errorCode: 'errors.commitments.createFailed',
});
renderPage([], { currentPeriod: { year: 2026, month: 5 } });
fireEvent.click(screen.getByTestId('commitments-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Arrangement SPF' } });
fireEvent.change(screen.getByLabelText(/Montant restant dû/), { target: { value: '1600' } });
fireEvent.change(screen.getByLabelText(/Montant par échéance/), { target: { value: '200' } });
fireEvent.change(screen.getByLabelText(/Nombre d'échéances/), { target: { value: '8' } });
await act(async () => {
fireEvent.submit(screen.getByRole('button', { name: /^ajouter$/i }).closest('form')!);
});
await waitFor(() => {
expect(toastErrorMock).toHaveBeenCalled();
});
// Form values should be preserved after a failed creation
expect(screen.getByLabelText('Libellé')).toHaveValue('Arrangement SPF');
expect(screen.getByLabelText(/Montant restant dû/)).toHaveValue('1600');
expect(screen.getByLabelText(/Montant par échéance/)).toHaveValue('200');
expect(screen.getByLabelText(/Nombre d'échéances/)).toHaveValue('8');
});
it('shows a validation error and does not call create when the total amount is invalid', async () => {
createMock.mockResolvedValue({ ok: true }); // should not be reached
renderPage([], { currentPeriod: { year: 2026, month: 5 } });
fireEvent.click(screen.getByTestId('commitments-add-toggle'));
fireEvent.change(screen.getByLabelText('Libellé'), { target: { value: 'Crédit auto' } });
// Trigger local validation: invalid / negative total amount
fireEvent.change(screen.getByLabelText(/Montant restant dû/), { target: { value: '-10' } });
fireEvent.change(screen.getByLabelText(/Montant par échéance/), { target: { value: '200' } });
fireEvent.change(screen.getByLabelText(/Nombre d'échéances/), { target: { value: '8' } });
await act(async () => {
fireEvent.submit(screen.getByRole('button', { name: /^ajouter$/i }).closest('form')!);
});
await waitFor(() => {
expect(toastErrorMock).toHaveBeenCalled();
});
expect(createMock).not.toHaveBeenCalled();
});
```
You may want to adjust:
- The `errorCode` string (`'errors.commitments.createFailed'`) to match the actual error code used by `createCommitmentAction`.
- The specific label texts / regexes if they differ slightly elsewhere in the file.
These tests will cover both the server-side failure branch and the local numeric validation branch in `onCreate`, verifying that the UI shows an error toast, preserves values on server failure, and avoids calling `createCommitmentAction` for immediately invalid input.
</issue_to_address>
### Comment 7
<location path="src/app/[locale]/app/commitments/CommitmentsClient.tsx" line_range="62" />
<code_context>
+ one_off: 'kinds.oneOff',
+} as const;
+
+export function CommitmentsClient({
+ commitments,
+ paidKeysByCommitment,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the optimistic paid ledger logic into a custom hook and the per-item rendering into a `CommitmentRow` component to break this large component into smaller, focused units.
You can reduce complexity without changing behaviour by extracting a couple of focused building blocks.
### 1. Encapsulate the optimistic paid ledger
The `${id}|${key}` encoding + `Set` logic is sprinkled in multiple places (`paidBase`, `applyOptimisticPaid`, `paidKeysOf`, `onTogglePaid`, `tickedThisPeriod`). Moving that into a hook will shrink the main component and centralise the representation.
For example:
```ts
// commitments/hooks/useOptimisticPaidLedger.ts
type Period = { year: number; month: number };
export function useOptimisticPaidLedger(
paidKeysByCommitment: Record<string, string[]>,
) {
const base = useMemo(() => {
const set = new Set<string>();
for (const [id, keys] of Object.entries(paidKeysByCommitment)) {
for (const k of keys) set.add(`${id}|${k}`);
}
return set;
}, [paidKeysByCommitment]);
const [optimisticPaid, applyOptimisticPaid] = useOptimistic(
base,
(current: ReadonlySet<string>, entry: string) => {
const next = new Set(current);
next.has(entry) ? next.delete(entry) : next.add(entry);
return next;
},
);
const periodKeyOf = (p: Period) => `${p.year}-${p.month}`;
const paidKeysOf = (id: string): ReadonlySet<string> => {
const prefix = `${id}|`;
const keys = new Set<string>();
for (const entry of optimisticPaid) {
if (entry.startsWith(prefix)) keys.add(entry.slice(prefix.length));
}
return keys;
};
const togglePaidOptimistic = (id: string, period: Period) => {
applyOptimisticPaid(`${id}|${periodKeyOf(period)}`);
};
return { paidKeysOf, togglePaidOptimistic };
}
```
Then in `CommitmentsClient`:
```ts
const { paidKeysOf, togglePaidOptimistic } = useOptimisticPaidLedger(
paidKeysByCommitment,
);
function onTogglePaid(c: RawCommitment) {
startTransition(async () => {
togglePaidOptimistic(c.id, currentPeriod);
// keep the rest of the action logic unchanged
});
}
```
This removes the string concatenation details from the main component and makes the payment-toggling code easier to scan.
### 2. Extract a `CommitmentRow` component
The `map` body currently computes a view model + renders a dense `<li>`. Extracting that into a row component reduces the size of `CommitmentsClient` and isolates per-row logic.
First, compute the minimal view model in the parent:
```ts
const rows = active.map((c) => {
const domain = toDomain(c);
const paidKeys = paidKeysOf(c.id);
const paid = installmentsPaid(domain, paidKeys);
const remaining = remainingBalance(domain, paidKeys);
const end = endPeriod(domain);
const dueThisPeriod = isDueInPeriod(domain, currentPeriod);
const tickedThisPeriod = paidKeys.has(periodKey(currentPeriod.year, currentPeriod.month));
const progress = Math.round((paid / c.installmentsTotal) * 100);
const finished = paid >= c.installmentsTotal;
return {
raw: c,
paid,
remaining,
end,
dueThisPeriod,
tickedThisPeriod,
progress,
finished,
};
});
```
Then render via a dedicated component:
```tsx
<ul role="list" className="divide-border/60 divide-y" data-testid="commitments-list">
{rows.map((row) => (
<CommitmentRow
key={row.raw.id}
row={row}
locale={locale}
onTogglePaid={() => onTogglePaid(row.raw)}
onDelete={() => onDelete(row.raw.id)}
isPending={isPending}
t={t}
/>
))}
</ul>
```
`CommitmentRow` keeps only row-level concerns:
```tsx
type CommitmentRowProps = {
row: {
raw: RawCommitment;
paid: number;
remaining: number;
end: { year: number; month: number };
dueThisPeriod: boolean;
tickedThisPeriod: boolean;
progress: number;
finished: boolean;
};
locale: Locale;
isPending: boolean;
onTogglePaid(): void;
onDelete(): void;
t: ReturnType<typeof useTranslations<'app.commitments'>>;
};
function CommitmentRow({
row: { raw: c, paid, remaining, end, dueThisPeriod, tickedThisPeriod, progress, finished },
locale,
isPending,
onTogglePaid,
onDelete,
t,
}: CommitmentRowProps) {
return (
<li data-testid={`commitment-row-${c.id}`} className="relative py-4 pr-24">
{/* existing JSX for header, progress bar, summary, buttons … */}
</li>
);
}
```
This keeps all behaviour intact but breaks the 470-line monolith into smaller, single-responsibility units, making the main component mostly orchestration of hooks + child components.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…er error branches Sourcery #234 review: - The 'missing manual rollback' finding is answered by PROOF, not by code: useOptimistic discards its value when the transition settles and re-derives from the server base — which a rejected toggle left untouched. Two tests now lock it (rejected result AND thrown error): aria-pressed returns to false and the balance goes back to 4 200 €. A hand-rolled revert would double-toggle. - Progress percentage clamped to [0,100] with a zero-guard (unreachable today given the DB CHECK + the schedule-bounded count, but corrupted data must not produce an invalid aria-valuenow). - Coverage added: optimistic tick moves BOTH the balance and the bar mid-flight, creation failure keeps the form open, negative amount blocked client-side, fieldErrors shape on validation failure, DB error on create, non-uuid id, rate-limit, and both INSERT/DELETE failure branches of the toggle. 73 commitment tests. Full suite 1564 green.
Upstream advisory published after the PR opened; the CI 'npm audit --audit-level=high' gate went red on a transitive dev dependency, not on anything in this diff. Lockfile-only patch bump, no new dependency.
thierryvm
added a commit
that referenced
this pull request
Jul 24, 2026
@Thierry: an échéancier (e.g. 11 × 220 €) could only tick the CURRENT month and had no way to edit. Now: - InstallmentStepper « X / N payées » with − / + : + marks the earliest unpaid scheduled instalment, − un-marks the latest paid one, reusing the existing single-period toggle action. Buttons disabled at bounds + while pending, so a double-click can't cancel a payment. useOptimistic reducer switched to explicit {key, paid} intent (idempotent under stale reads) — keeps the #234 rollback. - Edit via the pencil: reuses the form (single formMode source + resetForm), wired to the existing updateCommitmentAction. Guard blocks reducing installmentsTotal below what's already ticked (no silent ledger orphaning). - Row controls moved from cramped absolute corners into a flow flex row. - i18n: +10 keys × 5 locales, −2 dead. plan-reviewer APPROVED. 1616 tests.
thierryvm
added a commit
that referenced
this pull request
Jul 24, 2026
## Motivation (@Thierry) Sur `/app/commitments`, un échéancier (ex. **11 × 220 €**) ne pouvait valider que le **mois courant** (bouton ✓ gated `dueThisPeriod`) et **aucune édition** n'était possible. On doit pouvoir valider les 11 versements + corriger un engagement. ## Ce que fait la PR - **Compteur −/+ « X / N payées »** (`InstallmentStepper`, extrait) : `+` coche la plus ancienne échéance non payée, `−` décoche la plus récente payée — réutilise l'action toggle mono-période existante. Boutons désactivés aux bornes **et pendant une mutation** → un double-clic ne peut pas annuler un paiement (reducer `useOptimistic` passé en **intent explicite** `{key, paid}`, idempotent sur lecture périmée ; contrat rollback #234 préservé). - **Édition** via le crayon : réutilise le formulaire (source unique `formMode` + `resetForm`), branché sur l'`updateCommitmentAction` **déjà existant**. **Garde** : impossible de réduire `installmentsTotal` sous le nombre déjà coché (pas d'orphelin ledger silencieux). - **Layout ligne** : contrôles (stepper + crayon + poubelle) déplacés des coins `absolute` cramés vers une rangée `flex` en flux, mobile-first, cibles 44px (`size-11 md:size-9`). - **i18n** : +10 clés × 5 locales (traductions réelles), −2 obsolètes. ## Process `plan-reviewer` : ✅ APPROVED (6 renforcements intégrés : race double-clic, garde reduction, migration des 6 tests toggle, discipline reset form bi-mode, DoD, hygiène branche). Backend (update/toggle actions) **non modifié** — réutilisé. ## Evidence - `typecheck` ✅ · `lint` 0 erreur · **1616 tests** (+6 stepper, +4 net commitments, 6 tests toggle réécrits sur le stepper dont le contrat rollback) - QA : `dashboard-ux-auditor` + `i18n-auditor` lancés. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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-2 — La page « Engagements » (épic Dettes & échéanciers)
C'est ici que le modèle de PR-1 devient concret :
/app/commitments, accessible depuis le menu (Charges · Engagements · Dépenses).Ce que tu vois pour chaque engagement
Sous le capot
4 Server Actions calquées sur le contrat des charges : garde uuid, authz workspace (RLS + filtre explicite), rate-limit, audit, revalidate. Le toggle lit le montant de l'échéance depuis l'engagement, jamais depuis le client, et reste idempotent (INSERT payé / DELETE non payé derrière l'index unique). La page lit en
select('*')pour qu'une fenêtre de déploiement ne puisse jamais vider l'écran (leçon de l'incident du 18/07).Tests
22 nouveaux : 11 UI (arithmétique du solde, barre à 0/12/100 %, coche optimiste, one-off sans jargon, création avec ancre sur le mois affiché) + 11 action (dont refus d'un engagement d'un autre workspace, montant lu côté serveur, toggle idempotent). Suite complète 1554 verte, typecheck/lint/build clean.
Smoke @Thierry
Note : le bouton « convertir une charge en engagement » (décision D4) arrive en PR-3 avec l'intégration cockpit — ici tu crées tes engagements à la main pour valider le modèle.
🤖 Generated with Claude Code
Summary by Sourcery
Introduire une page Engagements/Commitments dans l’application avec un CRUD complet et le « ticking » des paiements, intégrée à l’authentification de l’espace de travail, à l’audit logging et à la revalidation du tableau de bord.
Nouvelles fonctionnalités :
/app/commitmentsqui liste les engagements actifs avec le solde restant, une barre de progression et des contrôles de validation mensuelle des paiements.Améliorations :
Tests :
Original summary in English
Summary by Sourcery
Introduce an Engagements/Commitments page in the app with full CRUD and payment ticking, wired into workspace auth, audit logging, and dashboard revalidation.
New Features:
Enhancements:
Tests: