feat: add shared api schemas and period helpers - #2230
Conversation
Signed-off-by: anilb <epipav@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Valid date inputs can produce corrupted ranges, and shared enum and error-envelope conventions are not followed.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds reusable TypeBox schemas and UTC period helpers for upcoming Development API endpoints.
Changes:
- Defines shared request and summary schemas.
- Implements previous-period and percentage calculations.
- Adds schema integration and date-math tests.
File summaries
| File | Description |
|---|---|
api/src/schemas/common.ts |
Adds common API schemas. |
api/src/lib/period.ts |
Adds period calculations and summaries. |
api/tests/common-schemas.test.ts |
Tests validation, coercion, and OpenAPI output. |
api/tests/period.test.ts |
Tests date ranges and summary calculations. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: anilb <epipav@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
The date schema conflicts with the documented API contract, and the empty-slug test expects the wrong HTTP status.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
api/src/schemas/common.ts:18
- The public API plan commits all date inputs to UTC ISO-8601 timestamps (
api/docs/arch/PUBLIC_API_PLAN.md:386), but this shared schema rejects that form and accepts only calendar dates. Either accept and normalize the documented timestamp contract, or update the architecture decision and dependent helpers/tests if the contract has intentionally changed.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
|
re copilot's date note on |
gaspergrom
left a comment
There was a problem hiding this comment.
Traced the Luxon-to-native date port by hand against the month-end clamp cases, and the numbers match. Schemas and tests cover the signed percentage and null cases well.
themarolt
left a comment
There was a problem hiding this comment.
lgtm - the luxon port checks out, including the clamp-then-subtract-days order. one question on the repos bound
Summary
Adds the building blocks that every Development endpoint will import, so the fourteen endpoints share one set of request params and one summary shape. This PR adds no routes.
api/src/schemas/common.ts(TypeBox):ProjectSlugParams: any non-empty slug, so an unknown project reaches the handler and returns empty dataDateRangeQuery:reposas repeated keys (a single value becomes a one-item array), andstartDateandendDateasYYYY-MM-DDUTC calendar daysGranularity:daily | weekly | monthly | quarterly | yearly, emitted as a plain OpenAPIenumPeriodSummary:current,previous,percentageChange,changeValue,periodFrom,periodToapi/src/lib/period.ts:getPreviousDates: a port of the helper infrontend/server/data/util.tsthat uses native UTC dates in place of Luxon, and returns the same previous periods, month-end clamping includedcalculatePercentageChangeandtoPeriodSummaryInvalidDateRangeError: a start date after the end date returns 400invalid_request, where Nuxt returned a 500Decisions worth a look
percentageChangeis signed, so a drop reads-50. Nuxt returnedMath.abs(...). It isnullwhenpreviousis 0 andcurrentisn't, where Nuxt left the key out.startDate, so the API's numbers match the widgets.percentageChangeis declared withnullable: truebecause the spec is OpenAPI 3.0.3, which has notype: 'null'.Testing
api/tests/period.test.ts: Luxon parity on 10 fixture ranges (UI presets, month-end clamp, leap day), defaults, two non-UTC host time zones, inverted ranges, and the percentage mathapi/tests/common-schemas.test.ts: the schemas wired into a throwaway Fastify route, covering validation, query coercion, null serialization and the generated OpenAPIgetPreviousDateswith the real Luxon implementation on 523,666 date ranges and found no mismatches.References
main