refactor(ack-pay): extract timestamp schemas#104
Conversation
WalkthroughExtracts timestamp validation/normalization into module-local ChangesTimestamp schema consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ack-pay/src/schemas/valibot.ts`:
- Around line 7-10: timestampSchema currently transforms a string/date by
calling new Date(input).toISOString(), which throws RangeError for invalid date
strings; change the schema to first validate that the input yields a valid Date
before transforming: within the v.pipe/v.union flow (timestampSchema) add a
refinement or pre-check (e.g., v.string().refine or a custom step) that parses
the input into a Date and checks !isNaN(date.getTime()), and only then call the
transform to toISOString(); ensure invalid inputs cause a validation error (via
refine failure) instead of allowing transform to throw.
In `@packages/ack-pay/src/schemas/zod/v3.ts`:
- Around line 7-9: The current timestampSchema uses .transform((val) => new
Date(val).toISOString()) which can throw RangeError for invalid strings; update
timestampSchema (the z.union of z.date() and z.string()) to first validate
inputs with a .refine() or .superRefine() that checks date validity (e.g., for
strings use Date.parse or new Date(val).getTime() and ensure it is not NaN, and
for Date objects ensure !isNaN(val.getTime())), and only then apply the
transform toISOString(), so the schema returns Zod validation errors instead of
throwing during the transform.
In `@packages/ack-pay/src/schemas/zod/v4.ts`:
- Around line 7-9: timestampSchema currently uses .transform((val) => new
Date(val).toISOString()) which can throw a RangeError for invalid date strings;
update the schema to validate the input before transforming by adding a
refinement (e.g., on the union of z.date() | z.string()) that checks the value
can be parsed to a valid Date (using Date.parse or isNaN(new
Date(...).getTime()) logic) and return a validation error if invalid, then
perform the .transform toISOString() only after the refinement succeeds; refer
to the timestampSchema, the .transform callback, and the z.union([z.date(),
z.string()]) as the locations to add the .refine()/.superRefine() check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04a95d74-16ec-4e29-8b25-b13fb9551f5d
📒 Files selected for processing (3)
packages/ack-pay/src/schemas/valibot.tspackages/ack-pay/src/schemas/zod/v3.tspackages/ack-pay/src/schemas/zod/v4.ts
|
Addressed the timestamp validation feedback in 224c426. Changes:
Verification:
|
venables
left a comment
venables
left a comment
There was a problem hiding this comment.
Thanks for tightening up the expiresAt handling — extracting the shared timestampSchema and rejecting invalid dates instead of letting new Date("…").toISOString() throw a RangeError is a solid fix.
Requesting one change before merge, plus an optional cleanup:
- (please address) The new
schemas.test.tsonly covers the rejection path. Add a happy-path test that asserts valid inputs are accepted and normalized to an ISO string across all three schemas — that transform is the load-bearing behavior and it's only covered indirectly today. (Inline comment below.) - (optional) Valibot has a built-in
isoTimestampaction that can replace the hand-rolled NaN check and also remove the doublenew Date()construction. There's a behavioral caveat worth a conscious decision — details inline.
A panel of independent reviewers (claude-opus-4.6, qwen3.6-plus) otherwise found no correctness issues; claude empirically verified accept/reject parity across valibot / zod v3 / zod v4 for NaN, empty string, Date objects, negatives, fractionals, numeric strings, and null.
| describe("paymentRequestSchema", () => { | ||
| it("reports invalid expiresAt strings as schema errors", () => { | ||
| const input = { | ||
| ...paymentRequest, | ||
| expiresAt: "invalid-date", | ||
| } | ||
|
|
||
| expect(v.safeParse(valibotPaymentRequestSchema, input).success).toBe(false) | ||
| expect(zodV3PaymentRequestSchema.safeParse(input).success).toBe(false) | ||
| expect(zodV4PaymentRequestSchema.safeParse(input).success).toBe(false) | ||
| }) |
There was a problem hiding this comment.
This only exercises the rejection path. Please add a happy-path test asserting that valid inputs are accepted and normalized to an ISO string across all three schemas — the .toISOString() transform is the load-bearing behavior here and is currently only covered indirectly (and only for valibot) by create-signed-payment-request.test.ts. A direct parity test guards against the three implementations drifting.
Suggested cases (a Date object and an equivalent string, both normalizing to the same ISO output):
it("normalizes valid expiresAt inputs to an ISO string", () => {
const expected = "2024-12-31T23:59:59.000Z"
for (const expiresAt of [
new Date("2024-12-31T23:59:59Z"),
"2024-12-31T23:59:59Z",
]) {
const input = { ...paymentRequest, expiresAt }
const valibot = v.safeParse(valibotPaymentRequestSchema, input)
expect(valibot.success && valibot.output.expiresAt).toBe(expected)
const v3 = zodV3PaymentRequestSchema.safeParse(input)
expect(v3.success && v3.data.expiresAt).toBe(expected)
const v4 = zodV4PaymentRequestSchema.safeParse(input)
expect(v4.success && v4.data.expiresAt).toBe(expected)
}
})| const timestampSchema = v.pipe( | ||
| v.union([v.date(), v.string()]), | ||
| v.check((input) => !Number.isNaN(new Date(input).getTime()), "Invalid date"), | ||
| v.transform((input) => new Date(input).toISOString()), | ||
| ) |
There was a problem hiding this comment.
Optional: Valibot ships an isoTimestamp action that can replace the hand-rolled NaN check here. The canonical pattern from valibot's own docs is v.union([v.date(), v.pipe(v.string(), v.isoTimestamp())]):
const timestampSchema = v.pipe(
v.union([v.date(), v.pipe(v.string(), v.isoTimestamp())]),
v.transform((input) => new Date(input).toISOString()),
)Two upsides: it drops the custom check, and it removes the redundant double new Date(input) construction (currently built once in the check and again in the transform).
Behavioral caveat to decide on first: isoTimestamp only accepts strings that are already in ISO 8601 form, so loosely-formatted-but-parseable strings (e.g. "2024/12/31", "December 31 2024") that the current new Date()-based check accepts would now be rejected. For an expiresAt field that's arguably more correct, but it is a tightening of the accepted input set. If you adopt it, mirror the change on the zod side (z.string().datetime() for v3, z.iso.datetime() for v4) so the three schemas stay in parity — otherwise the permissive new Date() version you have now is fine to keep.
Add a happy-path parity test feeding a Date object and the equivalent ISO string, asserting all three schemas (valibot, zod v3, zod v4) accept them and normalize to the same ISO string. The .toISOString() transform was only covered indirectly via create-signed-payment-request.test.ts; this guards the three implementations against drift. Signed-off-by: EfeDurmaz16 <efebarandurmaz05@gmail.com>
|
Thanks @venables. Happy-path test (addressed): added a direct parity test feeding both a isoTimestamp (kept current): I went with the permissive Verification:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ack-pay/src/schemas.test.ts`:
- Line 33: Rename the test case declaration currently written as it("normalizes
valid expiresAt inputs to an ISO string", ...) to use the required assertive
prefix, e.g. change the description to start with "returns" such as it("returns
an ISO string for valid expiresAt inputs", ...) so the test name follows the
mandated pattern while keeping the same test body and intent (locate and update
the it(...) call in the test file).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69345668-a7ce-451a-a4e1-84f49546f71c
📒 Files selected for processing (1)
packages/ack-pay/src/schemas.test.ts
| expect(zodV4PaymentRequestSchema.safeParse(input).success).toBe(false) | ||
| }) | ||
|
|
||
| it("normalizes valid expiresAt inputs to an ISO string", () => { |
There was a problem hiding this comment.
Rename the test to follow the required assertive pattern.
Use one of the mandated prefixes (e.g., it("returns ...")) instead of it("normalizes ...") for this case.
As per coding guidelines, test names in **/*.test.ts must use patterns: it("creates..."), it("throws..."), it("requires..."), or it("returns...").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ack-pay/src/schemas.test.ts` at line 33, Rename the test case
declaration currently written as it("normalizes valid expiresAt inputs to an ISO
string", ...) to use the required assertive prefix, e.g. change the description
to start with "returns" such as it("returns an ISO string for valid expiresAt
inputs", ...) so the test name follows the mandated pattern while keeping the
same test body and intent (locate and update the it(...) call in the test file).
2 participants
Summary
timestampSchemahelpersContext
Split out from the review feedback on #96, where the timestamp schema extraction was identified as useful independently of the approval model proposal.
Verification
corepack pnpm install --frozen-lockfilecorepack pnpm --filter @agentcommercekit/ack-pay... buildcorepack pnpm --filter @agentcommercekit/ack-pay test -- --runcorepack pnpm --filter @agentcommercekit/ack-pay check:typescorepack pnpm exec oxfmt --check packages/ack-pay/src/schemas/valibot.ts packages/ack-pay/src/schemas/zod/v3.ts packages/ack-pay/src/schemas/zod/v4.tsgit diff --checkNotes
distoutputs. Building the ACK-Pay dependency graph fixed that, and the rerun passed.AI Usage Disclosure
This contribution was AI-assisted. Initial work used Codex CLI and the Codex app; the follow-up happy-path test was added with Claude Opus. AI assistance was used for repository/docs navigation, understanding ACK/Catena context, identifying relevant issues or contribution areas, and assisting with small edits/validation. I reviewed the final diff and take responsibility for the submitted changes.
Summary by CodeRabbit
Refactor
Bug Fix
Tests