feat(commitments): foundations — schema, Zod contracts, pure schedule domain (épic PR-1) - #233
Merged
Merged
Conversation
… domain (épic Dettes & échéanciers PR-1) Ankora only modelled INFINITE recurring charges. Three real needs did not fit (@Thierry): a debt with a remaining balance (car loan), a FINITE instalment plan (SPF arrangement), a one-off future bill. One object covers all three. - Migration 20260719000001: commitments + commitment_payments. RLS/policies/ indexes/trigger cloned 1:1 from charges/charge_payments. Two CHECKs encode the invariants (a one_off is exactly one instalment; multi-instalment kinds carry an instalment amount). ADDITIVE — no existing table touched. - Pure domain (src/lib/domain/commitments): installmentPeriods, endPeriod, isDueInPeriod, remainingBalance, installmentsPaid, isFinished. Everything is DERIVED from anchor + cadence + count — the end date and the balance are never stored, so they cannot drift. Balance is clamped to [0, total]: the last instalment absorbs rounding (3 x 33.33 on 100 lands on exactly 0) and a stray off-schedule ledger tick can never over-count progress. - Zod contracts mirroring the DB CHECKs. The update variant derives from an unrefined base (Zod v4 forbids .partial() on a refined schema) and only fires cross-field rules when both sides are present in the patch. - supabase/types.ts regenerated from prod (CLI), replacing the hand-edited is_watched append from #227. Migration APPLIED to prod (supabase db push, ledger verified) before merge — the code ships behind no UI, so prod is unaffected either way. Locked decisions in docs/plans/epic-dettes-echeanciers-spec.md. 22 new tests; full suite 1513 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
status:review-needed
Contributor
Guide du relecteurPrésente le modèle de données fondamental, la logique métier pure et la couche de validation pour les « commitments » (dettes, plans de paiements échelonnés finis et factures ponctuelles futures), incluant de nouvelles tables Supabase et des types régénérés, sans aucun changement d’interface ou de comportement dans l’application existante. Diagramme de relations d’entités pour le modèle de données des commitmentserDiagram
commitments {
uuid id
text kind
numeric total_amount
numeric installment_amount
smallint installments_total
smallint start_year
smallint start_month
smallint payment_day
text frequency
boolean is_active
uuid workspace_id
uuid created_by
uuid category_id
}
commitment_payments {
uuid id
uuid commitment_id
uuid workspace_id
smallint period_year
smallint period_month
numeric paid_amount
timestamptz paid_at
uuid created_by
}
users {
uuid id
}
workspaces {
uuid id
}
categories {
uuid id
}
commitments ||--o{ commitment_payments : "has payments"
workspaces ||--o{ commitments : "workspace_commitments"
users ||--o{ commitments : "created_commitments"
categories ||--o{ commitments : "categorized_commitments"
workspaces ||--o{ commitment_payments : "workspace_commitment_payments"
users ||--o{ commitment_payments : "created_commitment_payments"
Modifications au niveau des fichiers
Conseils et commandesInteragir avec Sourcery
Personnaliser votre expérienceAccédez à votre tableau de bord pour :
Obtenir de l’aide
Original review guide in EnglishReviewer's GuideIntroduces the foundational data model, pure domain logic, and validation layer for "commitments" (debts, finite installment plans, and one-off future bills), including new Supabase tables and regenerated types, without any UI or behavioral changes to the existing app. Entity relationship diagram for commitments data modelerDiagram
commitments {
uuid id
text kind
numeric total_amount
numeric installment_amount
smallint installments_total
smallint start_year
smallint start_month
smallint payment_day
text frequency
boolean is_active
uuid workspace_id
uuid created_by
uuid category_id
}
commitment_payments {
uuid id
uuid commitment_id
uuid workspace_id
smallint period_year
smallint period_month
numeric paid_amount
timestamptz paid_at
uuid created_by
}
users {
uuid id
}
workspaces {
uuid id
}
categories {
uuid id
}
commitments ||--o{ commitment_payments : "has payments"
workspaces ||--o{ commitments : "workspace_commitments"
users ||--o{ commitments : "created_commitments"
categories ||--o{ commitments : "categorized_commitments"
workspaces ||--o{ commitment_payments : "workspace_commitment_payments"
users ||--o{ commitment_payments : "created_commitment_payments"
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é 8 problèmes et laissé quelques retours de haut niveau :
- Dans
commitmentUpdateSchema, tu ne réappliques que la règle « one_off » (un seul versement) ; pense aussi à refléter la condition CHECKinstallment_amount_requiredpour les patchs oùinstallmentsTotaletinstallmentAmountsont tous les deux présents, afin que le schéma côté client n’accepte pas des mises à jour que la base refusera. isDueInPeriodrecalcule actuellement tout le tableauinstallmentPeriodsà chaque appel ; si ça finit dans un chemin chaud (par ex. calculs de budget par ligne), tu pourrais le remplacer par un contrôle arithmétique direct basé surstartYear/startMonth,installmentsTotaletfrequencypour éviter des allocations inutiles.
Prompt pour agents IA
Please address the comments from this code review:
## Overall Comments
- In `commitmentUpdateSchema` you only reapply the `one_off` single-installment rule; consider also mirroring the `installment_amount_required` CHECK condition for patches where both `installmentsTotal` and `installmentAmount` are present, so the client-side schema doesn’t accept updates the DB will reject.
- `isDueInPeriod` currently recomputes the full `installmentPeriods` array on every call; if this ends up in any hot path (e.g. per-row budget calculations), you might want to replace it with a direct arithmetic check against `startYear/startMonth`, `installmentsTotal` and `frequency` to avoid unnecessary allocations.
## Individual Comments
### Comment 1
<location path="src/lib/schemas/commitment.ts" line_range="18-20" />
<code_context>
+ error: 'commitment.frequency.invalid',
+});
+
+const amountSchema = (key: string) =>
+ z
+ .number({ error: key })
+ .finite({ message: key })
+ .min(0, { message: `${key}.negative` })
</code_context>
<issue_to_address>
**issue (bug_risk):** The Zod `number` factory option key `error` is likely ignored; use `invalid_type_error` / `required_error` instead.
As written, `z.number({ error: key })` will ignore `error` and use default messages, so your i18n key won’t be used. Use the specific options instead, e.g. `z.number({ invalid_type_error: key, required_error: key })`, depending on which cases you want to customize.
</issue_to_address>
### Comment 2
<location path="src/lib/schemas/commitment.ts" line_range="38" />
<code_context>
+ .max(120, { message: 'commitment.label.tooLong' }),
+ kind: commitmentKindSchema,
+ /** Amount still engaged (locked decision D3: the REMAINING balance). */
+ totalAmount: amountSchema('commitment.totalAmount.invalid'),
+ /** One instalment; omitted for a one-off (the total is due at once). */
+ installmentAmount: amountSchema('commitment.installmentAmount.invalid').optional(),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `totalAmount` schema upper bound does not match the SQL constraints despite the comment claiming parity.
The DB `total_amount` column only has `CHECK (total_amount >= 0)` with no max, while `amountSchema` applies `.max(10_000_000, ...)`. So values between 10M and the DB numeric limit are allowed in SQL but rejected by the schema, conflicting with the claim that bounds mirror the SQL CHECKs. Please either add a matching DB upper bound, relax/remove the Zod max, and/or update the comment so it no longer implies full parity.
Suggested implementation:
```typescript
/**
* Amount still engaged (locked decision D3: the REMAINING balance).
*
* Note: the lower bound mirrors the DB `CHECK (total_amount >= 0)`.
* The upper bound comes from `amountSchema` and is a stricter frontend-only constraint,
* not enforced by the database.
*/
```
If there are other comments in this file (or nearby schema modules) that state or imply that *all* Zod bounds mirror the SQL `CHECK` constraints, they should be updated similarly to clarify that only the non-negativity of `totalAmount` matches the DB constraint, while the upper bound is frontend-only.
</issue_to_address>
### Comment 3
<location path="src/lib/schemas/commitment.ts" line_range="90-92" />
<code_context>
+ * are present in the patch — a lone `label` edit must never be rejected for a
+ * rule about fields it does not touch. The DB CHECKs remain the backstop.
+ */
+export const commitmentUpdateSchema = commitmentBaseSchema
+ .partial()
+ .refine(
+ (v) => v.kind !== 'one_off' || v.installmentsTotal === undefined || v.installmentsTotal === 1,
+ {
</code_context>
<issue_to_address>
**issue (bug_risk):** `commitmentUpdateSchema` does not re-apply the `installmentAmount` cross-field requirement, diverging from the create rules and DB CHECK.
Creates enforce `installmentsTotal === 1 || installmentAmount !== undefined`, matching the `commitments_installment_amount_required` CHECK, but the update schema only enforces the one-off/single-installment rule. That means a patch with `installmentsTotal > 1` and no `installmentAmount` will pass Zod and then fail at the DB layer. If you want “same invariants as create, but only when the fields are being patched”, you could use a `superRefine` that validates the `installmentsTotal`/`installmentAmount` relationship when either field is present in the patch object.
</issue_to_address>
### Comment 4
<location path="src/lib/domain/commitments/__tests__/schedule.test.ts" line_range="31" />
<code_context>
+const paidSet = (periods: Array<[number, number]>): ReadonlySet<string> =>
+ new Set(periods.map(([y, m]) => `${y}-${m}`));
+
+describe('installmentPeriods', () => {
+ it('lists every monthly instalment from the anchor', () => {
+ const periods = installmentPeriods(
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for `semiannual` and `annual` frequencies in `installmentPeriods` and `isDueInPeriod`
Right now the tests only cover `monthly`, `quarterly`, and `one_off`, but the domain also supports `semiannual` and `annual`. Please add tests that:
- assert the generated periods for `semiannual` and `annual` (including year roll-over), and
- verify `isDueInPeriod` only returns true for the correctly aligned months.
This will guard against regressions in the `MONTHS_BETWEEN` mapping and the month/year arithmetic for longer cadences.
Suggested implementation:
```typescript
describe('installmentPeriods – longer cadences', () => {
it('lists semiannual installments including year roll-over', () => {
const periods = installmentPeriods(
commitment({
frequency: 'semiannual',
installmentsTotal: 4,
startYear: 2026,
startMonth: 7,
}),
);
expect(periods).toEqual([
{ year: 2026, month: 7 },
{ year: 2027, month: 1 },
{ year: 2027, month: 7 },
{ year: 2028, month: 1 },
]);
});
it('lists annual installments including multi-year span', () => {
const periods = installmentPeriods(
commitment({
frequency: 'annual',
installmentsTotal: 3,
startYear: 2025,
startMonth: 11,
}),
);
expect(periods).toEqual([
{ year: 2025, month: 11 },
{ year: 2026, month: 11 },
{ year: 2027, month: 11 },
]);
});
});
const paidSet = (periods: Array<[number, number]>): ReadonlySet<string> =>
new Set(periods.map(([y, m]) => `${y}-${m}`));
describe('installmentPeriods', () => {
```
To fully implement your comment, you should also extend the `isDueInPeriod` tests (not shown in the snippet). Following the existing style in that file:
1. In the `describe('isDueInPeriod', ...)` block (or create one if it does not exist yet), add tests along these lines:
- For **semiannual**:
- A commitment with `frequency: 'semiannual'`, e.g. `startYear: 2026, startMonth: 2`.
- Assert that `isDueInPeriod` returns `true` only for months spaced by 6 (e.g. 2026‑02, 2026‑08, 2027‑02, …) and `false` for misaligned months (e.g. 2026‑03, 2026‑07, etc.).
- For **annual**:
- A commitment with `frequency: 'annual'`, e.g. `startYear: 2025, startMonth: 11`.
- Assert that `isDueInPeriod` returns `true` for 2025‑11, 2026‑11, 2027‑11 and `false` for other months/years (e.g. 2026‑10, 2026‑12).
2. Prefer using the existing `paidSet` helper if there is a pattern of computing all due periods and comparing sets, e.g.:
- Iterate over a range of `[year, month]` pairs, call `isDueInPeriod` for each, collect the `(year, month)` where it is `true`, and compare to an expected set using `paidSet`.
3. Keep naming consistent with existing tests, e.g. `"returns true for correctly aligned semiannual periods"` and `"returns false for misaligned semiannual periods"`, and likewise for annual.
</issue_to_address>
### Comment 5
<location path="src/lib/domain/commitments/__tests__/schedule.test.ts" line_range="62" />
<code_context>
+ });
+});
+
+describe('endPeriod', () => {
+ it('derives the LAST instalment period (never stored)', () => {
+ expect(
</code_context>
<issue_to_address>
**suggestion (testing):** Exercise `endPeriod` for non-monthly commitments and a longer schedule
Since `endPeriod` depends on `installmentPeriods`, consider adding coverage for a non-monthly plan (e.g. `quarterly` or `semiannual`) to verify the final period matches the expected cadence, and another test where `installmentsTotal` spans multiple years to validate long-running “SPF-like” schedules from the spec.
Suggested implementation:
```typescript
describe('endPeriod', () => {
it('derives the LAST instalment period (never stored)', () => {
expect(
endPeriod(commitment({ installmentsTotal: 3, startYear: 2026, startMonth: 11 })),
).toEqual({ year: 2027, month: 1 });
});
it('equals the anchor for a one-off', () => {
expect(endPeriod(commitment({ kind: 'one_off', installmentsTotal: 1 }))).toEqual({
year: 2026,
month: 8,
});
});
it('respects non-monthly cadence for the final period', () => {
expect(
endPeriod(
commitment({
installmentsTotal: 8,
startYear: 2026,
startMonth: 2,
cadence: 'quarterly',
}),
),
).toEqual({ year: 2027, month: 11 });
});
it('handles long-running schedules spanning multiple years', () => {
expect(
endPeriod(
commitment({
installmentsTotal: 60,
startYear: 2024,
startMonth: 6,
}),
),
).toEqual({ year: 2029, month: 5 });
});
```
1. Ensure the `commitment` helper supports a `cadence` (or similar) property and that `'quarterly'` is the correct value for a 3‑month interval. If the existing API uses a different key (e.g. `frequency`, `interval`, `periodicity`) or different enum/string values, update the property name and value in the new `cadence: 'quarterly'` usage accordingly.
2. Confirm that the default cadence used by `commitment(...)` when `cadence` is omitted is monthly; if not, explicitly set the monthly cadence in the "long-running schedules spanning multiple years" test to align with how SPF-like schedules are defined in your spec.
3. If `endPeriod` or `installmentPeriods` currently only support monthly cadences, you will need to extend their implementations to handle quarterly (or other non-monthly) cadences so that these new tests pass.
</issue_to_address>
### Comment 6
<location path="src/lib/domain/commitments/__tests__/schedule.test.ts" line_range="120" />
<code_context>
+ });
+});
+
+describe('remainingBalance / installmentsPaid / isFinished', () => {
+ const c = commitment({ installmentsTotal: 3, startYear: 2026, startMonth: 11, totalAmount: 750 });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider an explicit unit test for `periodKey` and for stray ticks before the schedule start
The current tests cover rounding, extra ticks after the schedule, and the link between `remainingBalance`, `installmentsPaid`, and `isFinished`. To tighten this:
- Add a focused test for `periodKey` to lock in the `year-month` ledger key format.
- Add a case where the paid set includes a period before `startYear/startMonth`, asserting it’s ignored by `installmentsPaid` (mirroring the off-schedule future tick behavior).
These will fully specify what counts as a scheduled tick from the ledger’s perspective.
Suggested implementation:
```typescript
});
});
describe('periodKey', () => {
it('formats as year-month with zero-padded month', () => {
expect(periodKey({ year: 2026, month: 1 })).toBe('2026-01');
expect(periodKey({ year: 2026, month: 11 })).toBe('2026-11');
});
});
describe('installmentsPaid with stray pre-start ticks', () => {
const c: Commitment = {
kind: 'installments',
startYear: 2026,
startMonth: 3,
installmentsTotal: 3,
totalAmount: 750,
installmentAmount: null,
};
it('ignores ledger ticks before the schedule start period', () => {
const paid = new Set<string>([
periodKey({ year: 2026, month: 1 }), // before start
periodKey({ year: 2026, month: 2 }), // before start
periodKey({ year: 2026, month: 3 }),
periodKey({ year: 2026, month: 4 }),
]);
expect(installmentsPaid(c, paid)).toBe(2);
});
});
import {
```
```typescript
installmentAmountOf,
remainingBalance,
installmentsPaid,
isFinished,
periodKey,
type Commitment,
} from '../schedule';
```
</issue_to_address>
### Comment 7
<location path="src/lib/schemas/__tests__/commitment.test.ts" line_range="3" />
<code_context>
+import { describe, it, expect } from 'vitest';
+
+import { commitmentInputSchema } from '../commitment';
+
+const validDebt = {
</code_context>
<issue_to_address>
**issue (testing):** Add tests for `commitmentUpdateSchema`, especially the partial cross-field rules
The update spec says cross-field rules only re-apply when both fields are present, but there are no tests for `commitmentUpdateSchema`. Please import `commitmentUpdateSchema` here and add tests that verify:
- patches changing only `label` or `notes` succeed regardless of other fields,
- patches with `kind: 'one_off'` and `installmentsTotal > 1` are rejected,
- patches where only one of `kind` or `installmentsTotal` is provided behave correctly.
This helps ensure partial updates aren’t blocked by constraints on untouched fields and that SQL CHECKs and Zod stay consistent for updates as well as creates.
</issue_to_address>
### Comment 8
<location path="src/lib/schemas/__tests__/commitment.test.ts" line_range="50-55" />
<code_context>
+ expect(r.success).toBe(false);
+ });
+
+ it('rejects a negative balance and an out-of-range month', () => {
+ expect(commitmentInputSchema.safeParse({ ...validDebt, totalAmount: -1 }).success).toBe(false);
+ expect(commitmentInputSchema.safeParse({ ...validDebt, startMonth: 13 }).success).toBe(false);
+ });
+
+ it('rejects an empty label and more than 600 instalments', () => {
+ expect(commitmentInputSchema.safeParse({ ...validDebt, label: ' ' }).success).toBe(false);
+ expect(commitmentInputSchema.safeParse({ ...validDebt, installmentsTotal: 601 }).success).toBe(
</code_context>
<issue_to_address>
**suggestion (testing):** Broaden schema tests to cover boundary values and key invariants (frequency, paymentDay, categoryId, max amount)
Existing tests already cover several key constraints. To better mirror the SQL CHECKs, consider adding targeted cases for:
- `paymentDay` at 1/31 (accepted) and 0/32 (rejected)
- `installmentsTotal` at 1 and 600 (accepted) and 0 (rejected)
- an invalid `frequency` value (e.g. `'weekly'`) to verify enum validation
- a non-UUID `categoryId` to verify UUID validation
Optionally, also test that a `totalAmount` just above `10_000_000` is rejected to confirm the upper bound.
</issue_to_address>Sourcery est gratuit pour l’open source - si vous aimez nos revues, pensez à les partager ✨
Original comment in English
Hey - I've found 8 issues, and left some high level feedback:
- In
commitmentUpdateSchemayou only reapply theone_offsingle-installment rule; consider also mirroring theinstallment_amount_requiredCHECK condition for patches where bothinstallmentsTotalandinstallmentAmountare present, so the client-side schema doesn’t accept updates the DB will reject. isDueInPeriodcurrently recomputes the fullinstallmentPeriodsarray on every call; if this ends up in any hot path (e.g. per-row budget calculations), you might want to replace it with a direct arithmetic check againststartYear/startMonth,installmentsTotalandfrequencyto avoid unnecessary allocations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `commitmentUpdateSchema` you only reapply the `one_off` single-installment rule; consider also mirroring the `installment_amount_required` CHECK condition for patches where both `installmentsTotal` and `installmentAmount` are present, so the client-side schema doesn’t accept updates the DB will reject.
- `isDueInPeriod` currently recomputes the full `installmentPeriods` array on every call; if this ends up in any hot path (e.g. per-row budget calculations), you might want to replace it with a direct arithmetic check against `startYear/startMonth`, `installmentsTotal` and `frequency` to avoid unnecessary allocations.
## Individual Comments
### Comment 1
<location path="src/lib/schemas/commitment.ts" line_range="18-20" />
<code_context>
+ error: 'commitment.frequency.invalid',
+});
+
+const amountSchema = (key: string) =>
+ z
+ .number({ error: key })
+ .finite({ message: key })
+ .min(0, { message: `${key}.negative` })
</code_context>
<issue_to_address>
**issue (bug_risk):** The Zod `number` factory option key `error` is likely ignored; use `invalid_type_error` / `required_error` instead.
As written, `z.number({ error: key })` will ignore `error` and use default messages, so your i18n key won’t be used. Use the specific options instead, e.g. `z.number({ invalid_type_error: key, required_error: key })`, depending on which cases you want to customize.
</issue_to_address>
### Comment 2
<location path="src/lib/schemas/commitment.ts" line_range="38" />
<code_context>
+ .max(120, { message: 'commitment.label.tooLong' }),
+ kind: commitmentKindSchema,
+ /** Amount still engaged (locked decision D3: the REMAINING balance). */
+ totalAmount: amountSchema('commitment.totalAmount.invalid'),
+ /** One instalment; omitted for a one-off (the total is due at once). */
+ installmentAmount: amountSchema('commitment.installmentAmount.invalid').optional(),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `totalAmount` schema upper bound does not match the SQL constraints despite the comment claiming parity.
The DB `total_amount` column only has `CHECK (total_amount >= 0)` with no max, while `amountSchema` applies `.max(10_000_000, ...)`. So values between 10M and the DB numeric limit are allowed in SQL but rejected by the schema, conflicting with the claim that bounds mirror the SQL CHECKs. Please either add a matching DB upper bound, relax/remove the Zod max, and/or update the comment so it no longer implies full parity.
Suggested implementation:
```typescript
/**
* Amount still engaged (locked decision D3: the REMAINING balance).
*
* Note: the lower bound mirrors the DB `CHECK (total_amount >= 0)`.
* The upper bound comes from `amountSchema` and is a stricter frontend-only constraint,
* not enforced by the database.
*/
```
If there are other comments in this file (or nearby schema modules) that state or imply that *all* Zod bounds mirror the SQL `CHECK` constraints, they should be updated similarly to clarify that only the non-negativity of `totalAmount` matches the DB constraint, while the upper bound is frontend-only.
</issue_to_address>
### Comment 3
<location path="src/lib/schemas/commitment.ts" line_range="90-92" />
<code_context>
+ * are present in the patch — a lone `label` edit must never be rejected for a
+ * rule about fields it does not touch. The DB CHECKs remain the backstop.
+ */
+export const commitmentUpdateSchema = commitmentBaseSchema
+ .partial()
+ .refine(
+ (v) => v.kind !== 'one_off' || v.installmentsTotal === undefined || v.installmentsTotal === 1,
+ {
</code_context>
<issue_to_address>
**issue (bug_risk):** `commitmentUpdateSchema` does not re-apply the `installmentAmount` cross-field requirement, diverging from the create rules and DB CHECK.
Creates enforce `installmentsTotal === 1 || installmentAmount !== undefined`, matching the `commitments_installment_amount_required` CHECK, but the update schema only enforces the one-off/single-installment rule. That means a patch with `installmentsTotal > 1` and no `installmentAmount` will pass Zod and then fail at the DB layer. If you want “same invariants as create, but only when the fields are being patched”, you could use a `superRefine` that validates the `installmentsTotal`/`installmentAmount` relationship when either field is present in the patch object.
</issue_to_address>
### Comment 4
<location path="src/lib/domain/commitments/__tests__/schedule.test.ts" line_range="31" />
<code_context>
+const paidSet = (periods: Array<[number, number]>): ReadonlySet<string> =>
+ new Set(periods.map(([y, m]) => `${y}-${m}`));
+
+describe('installmentPeriods', () => {
+ it('lists every monthly instalment from the anchor', () => {
+ const periods = installmentPeriods(
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for `semiannual` and `annual` frequencies in `installmentPeriods` and `isDueInPeriod`
Right now the tests only cover `monthly`, `quarterly`, and `one_off`, but the domain also supports `semiannual` and `annual`. Please add tests that:
- assert the generated periods for `semiannual` and `annual` (including year roll-over), and
- verify `isDueInPeriod` only returns true for the correctly aligned months.
This will guard against regressions in the `MONTHS_BETWEEN` mapping and the month/year arithmetic for longer cadences.
Suggested implementation:
```typescript
describe('installmentPeriods – longer cadences', () => {
it('lists semiannual installments including year roll-over', () => {
const periods = installmentPeriods(
commitment({
frequency: 'semiannual',
installmentsTotal: 4,
startYear: 2026,
startMonth: 7,
}),
);
expect(periods).toEqual([
{ year: 2026, month: 7 },
{ year: 2027, month: 1 },
{ year: 2027, month: 7 },
{ year: 2028, month: 1 },
]);
});
it('lists annual installments including multi-year span', () => {
const periods = installmentPeriods(
commitment({
frequency: 'annual',
installmentsTotal: 3,
startYear: 2025,
startMonth: 11,
}),
);
expect(periods).toEqual([
{ year: 2025, month: 11 },
{ year: 2026, month: 11 },
{ year: 2027, month: 11 },
]);
});
});
const paidSet = (periods: Array<[number, number]>): ReadonlySet<string> =>
new Set(periods.map(([y, m]) => `${y}-${m}`));
describe('installmentPeriods', () => {
```
To fully implement your comment, you should also extend the `isDueInPeriod` tests (not shown in the snippet). Following the existing style in that file:
1. In the `describe('isDueInPeriod', ...)` block (or create one if it does not exist yet), add tests along these lines:
- For **semiannual**:
- A commitment with `frequency: 'semiannual'`, e.g. `startYear: 2026, startMonth: 2`.
- Assert that `isDueInPeriod` returns `true` only for months spaced by 6 (e.g. 2026‑02, 2026‑08, 2027‑02, …) and `false` for misaligned months (e.g. 2026‑03, 2026‑07, etc.).
- For **annual**:
- A commitment with `frequency: 'annual'`, e.g. `startYear: 2025, startMonth: 11`.
- Assert that `isDueInPeriod` returns `true` for 2025‑11, 2026‑11, 2027‑11 and `false` for other months/years (e.g. 2026‑10, 2026‑12).
2. Prefer using the existing `paidSet` helper if there is a pattern of computing all due periods and comparing sets, e.g.:
- Iterate over a range of `[year, month]` pairs, call `isDueInPeriod` for each, collect the `(year, month)` where it is `true`, and compare to an expected set using `paidSet`.
3. Keep naming consistent with existing tests, e.g. `"returns true for correctly aligned semiannual periods"` and `"returns false for misaligned semiannual periods"`, and likewise for annual.
</issue_to_address>
### Comment 5
<location path="src/lib/domain/commitments/__tests__/schedule.test.ts" line_range="62" />
<code_context>
+ });
+});
+
+describe('endPeriod', () => {
+ it('derives the LAST instalment period (never stored)', () => {
+ expect(
</code_context>
<issue_to_address>
**suggestion (testing):** Exercise `endPeriod` for non-monthly commitments and a longer schedule
Since `endPeriod` depends on `installmentPeriods`, consider adding coverage for a non-monthly plan (e.g. `quarterly` or `semiannual`) to verify the final period matches the expected cadence, and another test where `installmentsTotal` spans multiple years to validate long-running “SPF-like” schedules from the spec.
Suggested implementation:
```typescript
describe('endPeriod', () => {
it('derives the LAST instalment period (never stored)', () => {
expect(
endPeriod(commitment({ installmentsTotal: 3, startYear: 2026, startMonth: 11 })),
).toEqual({ year: 2027, month: 1 });
});
it('equals the anchor for a one-off', () => {
expect(endPeriod(commitment({ kind: 'one_off', installmentsTotal: 1 }))).toEqual({
year: 2026,
month: 8,
});
});
it('respects non-monthly cadence for the final period', () => {
expect(
endPeriod(
commitment({
installmentsTotal: 8,
startYear: 2026,
startMonth: 2,
cadence: 'quarterly',
}),
),
).toEqual({ year: 2027, month: 11 });
});
it('handles long-running schedules spanning multiple years', () => {
expect(
endPeriod(
commitment({
installmentsTotal: 60,
startYear: 2024,
startMonth: 6,
}),
),
).toEqual({ year: 2029, month: 5 });
});
```
1. Ensure the `commitment` helper supports a `cadence` (or similar) property and that `'quarterly'` is the correct value for a 3‑month interval. If the existing API uses a different key (e.g. `frequency`, `interval`, `periodicity`) or different enum/string values, update the property name and value in the new `cadence: 'quarterly'` usage accordingly.
2. Confirm that the default cadence used by `commitment(...)` when `cadence` is omitted is monthly; if not, explicitly set the monthly cadence in the "long-running schedules spanning multiple years" test to align with how SPF-like schedules are defined in your spec.
3. If `endPeriod` or `installmentPeriods` currently only support monthly cadences, you will need to extend their implementations to handle quarterly (or other non-monthly) cadences so that these new tests pass.
</issue_to_address>
### Comment 6
<location path="src/lib/domain/commitments/__tests__/schedule.test.ts" line_range="120" />
<code_context>
+ });
+});
+
+describe('remainingBalance / installmentsPaid / isFinished', () => {
+ const c = commitment({ installmentsTotal: 3, startYear: 2026, startMonth: 11, totalAmount: 750 });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider an explicit unit test for `periodKey` and for stray ticks before the schedule start
The current tests cover rounding, extra ticks after the schedule, and the link between `remainingBalance`, `installmentsPaid`, and `isFinished`. To tighten this:
- Add a focused test for `periodKey` to lock in the `year-month` ledger key format.
- Add a case where the paid set includes a period before `startYear/startMonth`, asserting it’s ignored by `installmentsPaid` (mirroring the off-schedule future tick behavior).
These will fully specify what counts as a scheduled tick from the ledger’s perspective.
Suggested implementation:
```typescript
});
});
describe('periodKey', () => {
it('formats as year-month with zero-padded month', () => {
expect(periodKey({ year: 2026, month: 1 })).toBe('2026-01');
expect(periodKey({ year: 2026, month: 11 })).toBe('2026-11');
});
});
describe('installmentsPaid with stray pre-start ticks', () => {
const c: Commitment = {
kind: 'installments',
startYear: 2026,
startMonth: 3,
installmentsTotal: 3,
totalAmount: 750,
installmentAmount: null,
};
it('ignores ledger ticks before the schedule start period', () => {
const paid = new Set<string>([
periodKey({ year: 2026, month: 1 }), // before start
periodKey({ year: 2026, month: 2 }), // before start
periodKey({ year: 2026, month: 3 }),
periodKey({ year: 2026, month: 4 }),
]);
expect(installmentsPaid(c, paid)).toBe(2);
});
});
import {
```
```typescript
installmentAmountOf,
remainingBalance,
installmentsPaid,
isFinished,
periodKey,
type Commitment,
} from '../schedule';
```
</issue_to_address>
### Comment 7
<location path="src/lib/schemas/__tests__/commitment.test.ts" line_range="3" />
<code_context>
+import { describe, it, expect } from 'vitest';
+
+import { commitmentInputSchema } from '../commitment';
+
+const validDebt = {
</code_context>
<issue_to_address>
**issue (testing):** Add tests for `commitmentUpdateSchema`, especially the partial cross-field rules
The update spec says cross-field rules only re-apply when both fields are present, but there are no tests for `commitmentUpdateSchema`. Please import `commitmentUpdateSchema` here and add tests that verify:
- patches changing only `label` or `notes` succeed regardless of other fields,
- patches with `kind: 'one_off'` and `installmentsTotal > 1` are rejected,
- patches where only one of `kind` or `installmentsTotal` is provided behave correctly.
This helps ensure partial updates aren’t blocked by constraints on untouched fields and that SQL CHECKs and Zod stay consistent for updates as well as creates.
</issue_to_address>
### Comment 8
<location path="src/lib/schemas/__tests__/commitment.test.ts" line_range="50-55" />
<code_context>
+ expect(r.success).toBe(false);
+ });
+
+ it('rejects a negative balance and an out-of-range month', () => {
+ expect(commitmentInputSchema.safeParse({ ...validDebt, totalAmount: -1 }).success).toBe(false);
+ expect(commitmentInputSchema.safeParse({ ...validDebt, startMonth: 13 }).success).toBe(false);
+ });
+
+ it('rejects an empty label and more than 600 instalments', () => {
+ expect(commitmentInputSchema.safeParse({ ...validDebt, label: ' ' }).success).toBe(false);
+ expect(commitmentInputSchema.safeParse({ ...validDebt, installmentsTotal: 601 }).success).toBe(
</code_context>
<issue_to_address>
**suggestion (testing):** Broaden schema tests to cover boundary values and key invariants (frequency, paymentDay, categoryId, max amount)
Existing tests already cover several key constraints. To better mirror the SQL CHECKs, consider adding targeted cases for:
- `paymentDay` at 1/31 (accepted) and 0/32 (rejected)
- `installmentsTotal` at 1 and 600 (accepted) and 0 (rejected)
- an invalid `frequency` value (e.g. `'weekly'`) to verify enum validation
- a non-UUID `categoryId` to verify UUID validation
Optionally, also test that a `totalAmount` just above `10_000_000` is rejected to confirm the upper bound.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
… coverage Sourcery #233 review applied: - commitmentUpdateSchema now mirrors commitments_installment_amount_required for the patch case (superRefine): raising the count above 1 in the SAME patch that clears the amount is rejected client-side instead of failing at the DB. A lone count bump still passes — the stored row keeps its amount. - isDueInPeriod switched to direct arithmetic (offset % step, no array allocation): it runs per row in the cockpit budget pass. Equivalence with installmentPeriods() is locked by a test scanning 4 cadences x 96 months. - Header comment no longer claims full Zod<->SQL parity: every DB rule is enforced here, but the 10M ceilings are UI-side guards with no SQL counterpart. - Coverage: semiannual/annual periods + alignment, endPeriod on a non-monthly and a 60-instalment schedule, periodKey format, a stray PRE-start ledger tick, commitmentUpdateSchema partial rules, and boundary values (paymentDay 1/31 vs 0/32, instalments 1/600 vs 0, unknown frequency, non-UUID category, amount ceiling). Declined with proof: z.number({ error }) IS the Zod v4 API (repo runs 4.3.6, charge.ts:15 uses it) — the suggested invalid_type_error is v3 and would silently drop the i18n key. Verified by executing the schema. 41 commitment tests; full suite 1532 green.
Owner
Author
|
Retours globaux traités (3b9ed47) :
Le seul point décliné est le |
thierryvm
added a commit
that referenced
this pull request
Jul 22, 2026
… finite commitments Pure domain (ADR-021 Task 1). An active multi-instalment commitment contributes installmentAmount / cycleMonths while its window is open and it is not settled; single payments and one-offs contribute nothing. Ordinal-arithmetic window avoids the per-row schedule allocation (Sourcery #233). 13 tests.
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-1 — Fondations « Engagements » (dettes, échéanciers, factures futures)
Première brique de l'épic issu de tes retours sur ton Coda : « pas les factures futures, les dettes liées à des crédits, ou un remboursement différé des impôts suite à un arrangement avec le SPF ».
Cette PR ne change RIEN de visible — schéma + domaine + contrats uniquement. L'UI arrive en PR-2.
Le modèle : un seul objet pour tes 3 besoins
debt(crédit voiture)installment_plan(SPF)one_off(facture future)Tout est dérivé (date de fin, solde restant, progression) — jamais stocké, donc jamais faux. Les paiements réutilisent exactement le mécanisme des factures : cocher une échéance = le geste que tu connais déjà.
Contenu
commitments+commitment_payments: RLS, policies, index et trigger clonés 1:1 surcharges/charge_payments. Deux CHECKs encodent les invariants. Additive, aucune table existante touchée.installmentPeriods,endPeriod,isDueInPeriod,remainingBalance,installmentsPaid,isFinished. Le solde est borné à [0, total] : la dernière échéance absorbe l'arrondi (3 × 33,33 € sur 100 € tombe pile sur 0) et une coche parasite hors échéancier ne peut pas fausser la progression.supabase/types.tsrégénéré depuis la prod via le CLI — remplace l'édition manuelle de feat(charges): watched marker + dashboard bills card rework (THI-329 PR-C) #227.Décisions verrouillées avec toi (spec :
docs/plans/epic-dettes-echeanciers-spec.md)Nom « Engagements » · saisie « solde restant + échéances restantes » · bouton « convertir » depuis une charge (PR-2) · facture ponctuelle incluse.
Migration prod
Déjà appliquée (
supabase db push, ledger vérifié). Comme aucune UI ne la consomme, la prod est inchangée dans les deux sens.Tests
22 nouveaux (16 domaine + 6 schéma), suite complète 1513 verte, typecheck/lint/build clean.
🤖 Generated with Claude Code
Summary by Sourcery
Introduire un modèle de données fondamental et une logique métier pour des engagements financiers finis (dettes, plans de paiement échelonné et factures ponctuelles futures) sans aucun changement d’interface utilisateur.
Nouvelles fonctionnalités :
commitmentsetcommitment_payments, ainsi que les types Supabase associés et les politiques RLS, afin de modéliser les engagements financiers finis et leurs paiements.Améliorations :
Tests :
Original summary in English
Summary by Sourcery
Introduce a foundational data model and domain logic for finite financial commitments (debts, installment plans, and one-off future bills) without any UI changes.
New Features:
commitmentsandcommitment_paymentstables, plus related Supabase types and RLS policies, to model finite financial engagements and their payments.Enhancements:
Tests: