TypeBox => Zod - #812
Conversation
ref https://linear.app/ghost/issue/PLA-280/optimize-traffic-analytics-docker-buildself-hosting-execution - replace Type.Transform with Type.Any(), add resolveEventId function to standardize event id resolution - fix handling of non-canonical timestamps - update tests
ref https://linear.app/ghost/issue/PLA-280/optimize-traffic-analytics-docker-buildself-hosting-execution - replace typebox with zod => ajv json schema pipeline - zod + ajv has relatively similar performance characteristics to TypeBox (esp. when the event_id fix is included) - add separate parser for pubsub
WalkthroughMigrated page-hit schemas from TypeBox to Zod with inferred types and explicit event-ID resolution. Added Zod-to-AJV compilation, separate AJV validator configurations, Fastify validator and serializer compilers, and Zod type-provider mappings. Updated routes and batch processing to use the new validation path, and expanded unit and integration coverage for schema parity, timestamps, UUIDs, and validation errors. Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/schemas/v1/page-hit-processed.ts (1)
146-190: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSettle
event_idbefore buildingPageHitProcessed.The raw payload accepts an empty
event_idstring, buttransformPageHitRawToProcessedonly substitutes a generated UUID fornull/undefined. An empty-string value would reach the processed payload as invalidz.guid()input, while the request-to-publication path already settles it withresolveEventId. UseresolveEventId(pageHitRaw.payload.event_id)here too.🤖 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 `@src/schemas/v1/page-hit-processed.ts` around lines 146 - 190, The transformPageHitRawToProcessed function must resolve empty event IDs before constructing the processed payload. Replace the current nullish-only event_id fallback with resolveEventId(pageHitRaw.payload.event_id), preserving generated UUID behavior for missing or empty values and ensuring the processed payload receives a valid ID.
🧹 Nitpick comments (3)
test/unit/schemas/v1/page-hit-request.test.ts (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew
node:assert/strictimport mixes assertion styles with the rest of the suite.The rest of the file (and the sibling schema test files) uses vitest's
expect(...).toBe(...).assert.ok(...)here is functionally fine but introduces a second assertion library for no apparent reason.✏️ Suggested consistency fix
-import assert from 'node:assert/strict'; +- it(`should validate with ${label} as the event ID`, () => { - assert.ok(EventIdSchema.safeParse(value).success, `Event ID can be ${label}`); - }); + it(`should validate with ${label} as the event ID`, () => { + expect(EventIdSchema.safeParse(value).success).toBe(true); + });Also applies to: 27-31
🤖 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 `@test/unit/schemas/v1/page-hit-request.test.ts` at line 11, Remove the node:assert/strict import from the page-hit request test and replace its assert.ok usage with Vitest’s expect(...).toBe(...) style, matching the existing assertions in the file and sibling schema tests.Source: Coding guidelines
src/schemas/v1/page-hit-request.ts (1)
6-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCommon primitive schemas (
StringSchema,NonEmptyStringSchema,UUIDSchema,ISO8601DateTimeSchema) are copy-pasted verbatim across the three v1 schema files, and have already drifted once (request.ts'sNonEmptyStringSchemaadds a whitespace-rejecting regex the other two lack). A shared module would prevent future divergence (e.g. if the ISO precision or guid pattern needs to change).
src/schemas/v1/page-hit-request.ts#L6-L13: extract to a shared module; keep the extra.regex()constraint local if it's intentionally request-only.src/schemas/v1/page-hit-raw.ts#L4-L8: import the shared primitives instead of redefining them.src/schemas/v1/page-hit-processed.ts#L10-L13: import the shared primitives instead of redefining them.🤖 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 `@src/schemas/v1/page-hit-request.ts` around lines 6 - 13, Extract the shared StringSchema, UUIDSchema, and ISO8601DateTimeSchema primitives from src/schemas/v1/page-hit-request.ts, along with the base NonEmptyStringSchema, into a shared module; keep page-hit-request.ts’s whitespace-rejecting regex as a local request-specific constraint. Update src/schemas/v1/page-hit-raw.ts and src/schemas/v1/page-hit-processed.ts to import the shared primitives instead of redefining them, preserving their existing schema behavior.test/unit/schemas/validation.test.ts (1)
71-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo AJV-parity case for
EventIdSchema = z.any().
z.any()looks like a new schema construct for this codebase (accept-anything, resolved separately byresolveEventId). Consider adding a case (e.g.payload.event_id: 123or{}) to the cases array to confirm ajv's projection ofz.any()doesn't reject non-string values, matching Zod's behavior.🤖 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 `@test/unit/schemas/validation.test.ts` around lines 71 - 98, Add an AJV-parity case to the validation cases array for PageHitRequestBodySchema where payload.event_id is a non-string value such as a number or object. Keep the case valid and verify that AJV accepts it consistently with EventIdSchema’s z.any() behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/schemas/v1/page-hit-processed.ts`:
- Around line 146-190: The transformPageHitRawToProcessed function must resolve
empty event IDs before constructing the processed payload. Replace the current
nullish-only event_id fallback with resolveEventId(pageHitRaw.payload.event_id),
preserving generated UUID behavior for missing or empty values and ensuring the
processed payload receives a valid ID.
---
Nitpick comments:
In `@src/schemas/v1/page-hit-request.ts`:
- Around line 6-13: Extract the shared StringSchema, UUIDSchema, and
ISO8601DateTimeSchema primitives from src/schemas/v1/page-hit-request.ts, along
with the base NonEmptyStringSchema, into a shared module; keep
page-hit-request.ts’s whitespace-rejecting regex as a local request-specific
constraint. Update src/schemas/v1/page-hit-raw.ts and
src/schemas/v1/page-hit-processed.ts to import the shared primitives instead of
redefining them, preserving their existing schema behavior.
In `@test/unit/schemas/v1/page-hit-request.test.ts`:
- Line 11: Remove the node:assert/strict import from the page-hit request test
and replace its assert.ok usage with Vitest’s expect(...).toBe(...) style,
matching the existing assertions in the file and sibling schema tests.
In `@test/unit/schemas/validation.test.ts`:
- Around line 71-98: Add an AJV-parity case to the validation cases array for
PageHitRequestBodySchema where payload.event_id is a non-string value such as a
number or object. Keep the case valid and verify that AJV accepts it
consistently with EventIdSchema’s z.any() behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db684646-4d0a-4aa5-a178-32fed5fdee66
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
AGENTS.mdpackage.jsonsrc/app.tssrc/handlers/page-hit-handlers.tssrc/schemas/format-registry.tssrc/schemas/index.tssrc/schemas/v1/page-hit-processed.tssrc/schemas/v1/page-hit-raw.tssrc/schemas/v1/page-hit-request.tssrc/schemas/validation.tssrc/services/batch-worker/BatchWorker.tssrc/transformations/page-hit-transformations.tssrc/types/tryghost-validator.d.tstest/integration/app.test.tstest/integration/validation-error-logging.test.tstest/unit/schemas/v1/page-hit-processed.test.tstest/unit/schemas/v1/page-hit-raw.test.tstest/unit/schemas/v1/page-hit-request.test.tstest/unit/schemas/validation.test.ts
💤 Files with no reviewable changes (2)
- src/types/tryghost-validator.d.ts
- src/schemas/format-registry.ts
1 participant
This PR converts the existing validation logic via TypeBox to Zod-based schemas. Normally, Zod schemas would negatively impact performance (ajv + TypeBox both use json-schema based compilation/validation under the hood), but with this approach we get comparable performance by converting the Zod schemas to JSON schemas at runtime, then compiling them with ajv to get maximum throughput.
Additionally, this PR makes a slight tweak to event_id processing. Largely the behavior is the same, but removing the need for transforming the event_id payload during validation (instead using a relaxed
anyvalue for validation and then a manual check after the fact.