fix/rpu-12-digit

needs attentionviewing older commit

c6ce7d8 · incrementalPR #246reviewed 2026-07-05 21:35 UTC0H · 3M · 6L · 3I

The branch

Purpose

Hotfix for the CFE RPU 12-digit validation gate — an incomplete RPU reached the create form, failed server-side with an opaque ZodError, and froze the form in Creando with a blank toast.

Goal

Enforce the 12-digit RPU rule at every entry point (drawer lookup gate, inline create form), surface specific error messages from all server error shapes, and prevent the submit lock from freezing on error.

Sub-goals

SG-1: Single source of truth for the 12-digit rule (rpu-errors.ts) shared by both components
SG-2: Drawer lookup gate fires only at exactly 12 digits — incomplete RPU shows Faltan N digitos
SG-3: InlineContractForm client-side isValidRpu guard (PDF extraction defense-in-depth)
SG-4: extractServerErrorMessage unwraps ts-rest 400 ZodError, JSend fail, JSend error into human toast
SG-5: Submit lock released on error path (no more frozen Creando)
SG-6: Toast message length clamped to prevent UI overflow from server-controlled strings

The changes (whole branch)

What: This incremental commit (c6ce7d8e) over d8cbd917 adds: (1) a comment update in AddContractDrawer clarifying the defense-in-depth rationale for the digits useMemo; (2) MAX_SERVER_MESSAGE_LENGTH constant + clampMessage function applied to all three extraction branches in extractServerErrorMessage; (3) two new tests for the clamping behavior; (4) one new drawer test for >12-digit input clamping.
Why: Previous review identified that extractServerErrorMessage returned unbounded server-controlled strings. This commit adds the clamping defense and tests it.
Areas: apps/platform/src/app/[locale]/(dashboard)/bills/_components+150−12apps/platform/src/app/[locale]/(dashboard)/bills/_lib+206−0apps/platform/src/messages+6−2
Blast: 6 files changed across the bills feature slice (+524/-26 cumulative vs main). No domain layer, no API contracts, no DB schema touched. Blast radius confined to AddContractDrawer + InlineContractForm UI components and their shared _lib utilities.

comment-hygiene: file JSDoc refs branch name (will rot) test-coverage: clamping boundary + multi-branch coverage gaps

➖ci· No CI check data available for this branch➖coderabbit· No .coderabbit.yaml in repo

Findings · 12

correctness2

low

clampMessage semantics: MAX_SERVER_MESSAGE_LENGTH is the total output cap, not the content length

apps/platform/src/app/[locale]/(dashboard)/bills/_lib/rpu-errors.ts:35

slice(0, MAX-1) + '…' yields exactly MAX chars total. The test asserts out.length === MAX_SERVER_MESSAGE_LENGTH which passes — behavior is consistent — but if the intent were 200 content chars + ellipsis (total 201), the slice boundary would be wrong. The constant name implies total length; the implementation matches. No bug, but the arithmetic deserves a comment.

low

Test .toBeTruthy() on getByTestId result is redundant

apps/platform/src/app/[locale]/(dashboard)/bills/_components/__tests__/AddContractDrawer.rpuGate.test.tsx:119

getByTestId throws if the element is absent, making .toBeTruthy() a no-op assertion. The real guard is the subsequent textContent check. Replace with a direct textContent assertion as the prior test does.

security1

info

Server-controlled toast string — XSS risk is absent

apps/platform/src/app/[locale]/(dashboard)/bills/_lib/rpu-errors.ts:53

extractServerErrorMessage surfaces server-controlled strings into a toast rendered as a React text node (not dangerouslySetInnerHTML), so HTML/script injection cannot execute. The 200-char clamp is adequate defense-in-depth against UI-flooding. No actionable finding.

conventions3

medium

File-level JSDoc references current branch/fix name

apps/platform/src/app/[locale]/(dashboard)/bills/_lib/rpu-errors.ts:2

The @file block header includes '(hotfix fix/rpu-12-digit)'. Per comment discipline, task/fix/branch references belong in the PR description, not source comments — they rot immediately once the branch ships. Drop the parenthetical; the rest of the docblock stands on its own.

low

shouldLookup comment describes old bug rather than current invariant

apps/platform/src/app/[locale]/(dashboard)/bills/_components/AddContractDrawer.tsx:154

The comment reads: an 11-digit RPU used to slip through as 'available', then the create failed server-side. This is fix-history describing what broke, not why the current rule exists. Future readers only need the invariant. Fix-history belongs in the commit message.

low

onChange comment mixes WHAT with WHY

apps/platform/src/app/[locale]/(dashboard)/bills/_components/AddContractDrawer.tsx:422

First sentence restates what normalizeRpuInput's own JSDoc already says. The second sentence ('No maxLength — the browser would apply it BEFORE onChange') is a genuine non-obvious WHY and should stay. Trim the first sentence.

tests4

medium

clampMessage boundary (exactly 200 chars) not tested

apps/platform/src/app/[locale]/(dashboard)/bills/_lib/__tests__/rpu-errors.test.ts:106

The new clamping test uses length 500; the unclamped test uses 18 chars. Neither covers the exact boundary: a 200-char message should pass through unchanged (the guard is > 200, not >= 200), and a 201-char message is the first to trigger the clamp. The off-by-one risk in slice(0, MAX-1) + '...' is untested at the boundary.

medium

clampMessage only tested through body.message branch — data.message and ZodError branches uncovered

apps/platform/src/app/[locale]/(dashboard)/bills/_lib/__tests__/rpu-errors.test.ts:106

clampMessage is called in all three extraction branches of extractServerErrorMessage. The new test only exercises the top-level body.message branch. A regression removing clampMessage from the body.data.message or bodyErrors ZodError branches would go undetected.

low

>12-digit drawer test uses 14 digits; 13 (minimal over-long) would be more precise

apps/platform/src/app/[locale]/(dashboard)/bills/_components/__tests__/AddContractDrawer.rpuGate.test.tsx:111

Testing 13 digits (one above the cap) is the minimal case that confirms the slice boundary. 14 digits confirms the happy path but is further from the edge. Low priority since rpu-errors.test.ts already covers the normalizer boundary directly.

info

No test for clamp when body.data.message is over-long

apps/platform/src/app/[locale]/(dashboard)/bills/_lib/__tests__/rpu-errors.test.ts:85

The existing JSend fail test uses a short string. An over-long data.message test would confirm clampMessage is wired in that branch. Informational since all branches share the same helper.

improvement2

low

slice offset-by-one is non-obvious without inline comment

apps/platform/src/app/[locale]/(dashboard)/bills/_lib/rpu-errors.ts:37

slice(0, MAX_SERVER_MESSAGE_LENGTH - 1) reserves one code-unit for the ellipsis so the total is exactly MAX chars. The -1 looks like an accidental off-by-one to the next reader. A brief inline comment (e.g. -1 + ellipsis = MAX chars total) makes the invariant self-documenting.

info

clampMessage naming could be more domain-specific

apps/platform/src/app/[locale]/(dashboard)/bills/_lib/rpu-errors.ts:35

clampMessage is private and domain-specific. clampServerMessage would signal intent more clearly. Minor naming nit.

History · 6 commits

427115eneeds attentionincremental0H · 1M · 3L2026-07-05 21:43
c6ce7d8needs attentionincremental0H · 3M · 6L2026-07-05 21:35current
d8cbd91needs attentionincremental0H · 0M · 3L2026-07-05 21:25
20aa256safeincremental0H · 0M · 2L2026-07-05 21:14
77a5d70needs attentionincremental0H · 2M · 9L2026-07-05 21:09
586531aneeds attentionfull0H · 5M · 10L2026-07-05 20:51