feat(api): harden developer gear API and add catalog snapshot#367
Conversation
…o break request loop A server-side redirect() inside the @edit/(.)edit parallel-route interception slot caused a runaway request loop: Next.js re-fetches the slot on every prefetch/soft-nav, each fetch triggers fetchPendingEditId + redirect, and the cycle repeats at ~100+ req/s from a single browser tab. Changes: - New EditAlreadyPendingRedirect client component that uses router.replace() + toast instead of server redirect() - Updated (.)edit/page.tsx and edit/page.tsx to render the client component when a pending edit exists - New EditAlreadyPendingToast on the gear page to show feedback and clean up the editAlreadyPending search param (mirrors EditAppliedToast pattern)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…loop fix: replace server redirect in (.)edit with client-side navigation to break request loop
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesThe PR adds a cached, ETag-aware Developer API catalog, enriches public gear taxonomy responses, replaces implicit serialization with explicit allowlists, and updates API documentation and localizations. It also changes pending gear-edit handling from server redirects to client navigation with an informational toast. Developer API catalog and gear contracts
Pending gear edit navigation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/server/admin/gear/service.ts (1)
364-365: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider making catalog cache invalidation conditional on thumbnail changes.
setGearThumbnailServiceupdates boththumbnailUrlandogImageUrl. However, the developer API catalog only exposes thethumbnailUrl(as seen inCATALOG_CORE_FIELDS). If this function is called only to backfill or update theogImageUrlwhile thethumbnailUrlremains unchanged, invalidating the catalog cache is unnecessary.Depending on how this function is used (and potentially what the
tests/unit/admin-gear-thumbnail-service.test.tsexpects), consider tracking if the thumbnail actually changed before invalidating:♻️ Proposed conditional invalidation
- invalidateDeveloperApiCatalogCache(); + if (thumbnailUrl !== currentGear.thumbnailUrl) { + invalidateDeveloperApiCatalogCache(); + }🤖 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/server/admin/gear/service.ts` around lines 364 - 365, Update setGearThumbnailService to track whether thumbnailUrl changes independently from ogImageUrl, and call invalidateDeveloperApiCatalogCache only when the thumbnail value actually changes. Preserve both field updates while avoiding invalidation for ogImageUrl-only backfills.tests/unit/developer-api-service.test.ts (1)
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the cache instead of replacing it with the raw resolver.
The current mock invokes
fetchCatalogon both calls, so this test only verifies deterministic hashing. Memoize the resolver and assert the catalog is fetched once.Proposed test update
vi.mock("next/cache", () => ({ - unstable_cache: (resolver: () => Promise<unknown>) => resolver, + unstable_cache: (resolver: () => Promise<unknown>) => { + let cached: Promise<unknown> | undefined; + return () => (cached ??= resolver()); + }, })); const first = await getDeveloperCatalogSnapshot(); const second = await getDeveloperCatalogSnapshot(); +expect(mocks.fetchCatalog).toHaveBeenCalledTimes(1);As per coding guidelines, “Include or update automated tests covering the primary path and at least one meaningful edge case when introducing or modifying behavior.”
Also applies to: 128-162
🤖 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 `@tests/unit/developer-api-service.test.ts` around lines 29 - 31, Update the next/cache mock around unstable_cache so it memoizes each resolver’s result rather than returning the resolver directly. Adjust the relevant developer API service test to invoke the cached fetch path twice and assert the catalog-fetch operation runs only once, while preserving the existing deterministic-result assertions.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.
Inline comments:
In `@messages/de.json`:
- Around line 1896-1901: Translate the catalogTitle, catalogDescription,
catalogConditionalRequest, gearTitle, gearDescription, and gearTaxonomyNote
strings in messages/de.json lines 1896-1901 into German; make the corresponding
translations in messages/es.json lines 1896-1901 for Spanish, messages/fr.json
lines 1896-1901 for French, and messages/it.json lines 1896-1901 for Italian,
preserving the existing keys and JSON structure.
In `@src/app/`[locale]/(pages)/gear/_components/edit-already-pending-redirect.tsx:
- Around line 17-33: Update EditAlreadyPendingRedirect to use usePathname and
derive the destination by removing the current pathname’s trailing /edit
segment, then append the existing editAlreadyPending and pendingId query
parameters. Replace the hardcoded /gear/${slug} route while preserving scroll:
false and include the derived pathname in the effect dependencies.
In `@src/app/`[locale]/(pages)/gear/_components/edit-already-pending-toast.tsx:
- Around line 18-24: Update the useEffect toast.info call in the
edit-already-pending component to use translation keys for both user-facing
strings, adding matching keys and translations to every locale file under
messages/. Add a stable id to the toast options so Sonner deduplicates repeated
invocations while preserving the existing trigger condition.
In `@src/server/developer-api/cache.ts`:
- Around line 6-9: Remove the second options argument from revalidateTag in
invalidateDeveloperApiCatalogCache, leaving only
DEVELOPER_API_CATALOG_CACHE_TAG. In tests/unit/developer-api-cache.test.ts lines
10-16, update the assertion to expect revalidateTag to be called with
"developer-api-catalog" as its sole argument.
In `@tests/unit/admin-gear-thumbnail-service.test.ts`:
- Line 186: Update setGearThumbnailService to make
invalidateDeveloperApiCatalogCache conditional so the OG image backfill path
does not invalidate the catalog cache, matching the expectation in the admin
gear thumbnail test. Preserve invalidation for flows that require catalog
refresh, and keep the test assertion aligned with the resulting behavior.
---
Nitpick comments:
In `@src/server/admin/gear/service.ts`:
- Around line 364-365: Update setGearThumbnailService to track whether
thumbnailUrl changes independently from ogImageUrl, and call
invalidateDeveloperApiCatalogCache only when the thumbnail value actually
changes. Preserve both field updates while avoiding invalidation for
ogImageUrl-only backfills.
In `@tests/unit/developer-api-service.test.ts`:
- Around line 29-31: Update the next/cache mock around unstable_cache so it
memoizes each resolver’s result rather than returning the resolver directly.
Adjust the relevant developer API service test to invoke the cached fetch path
twice and assert the catalog-fetch operation runs only once, while preserving
the existing deterministic-result assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d99992c6-9fbb-4bd2-9bff-7080e7cc8702
📒 Files selected for processing (32)
docs/developer-api.mdmessages/de.jsonmessages/en.jsonmessages/es.jsonmessages/fr.jsonmessages/it.jsonmessages/ja.jsonmessages/ms.jsonmessages/zh.jsonsrc/app/[locale]/(pages)/developer/docs/page.tsxsrc/app/[locale]/(pages)/gear/[slug]/@edit/(.)edit/page.tsxsrc/app/[locale]/(pages)/gear/[slug]/edit/page.tsxsrc/app/[locale]/(pages)/gear/[slug]/page.tsxsrc/app/[locale]/(pages)/gear/_components/edit-already-pending-redirect.tsxsrc/app/[locale]/(pages)/gear/_components/edit-already-pending-toast.tsxsrc/app/api/v1/catalog/route.tssrc/server/admin/gear/service.tssrc/server/admin/proposals/service.tssrc/server/developer-api/cache.tssrc/server/developer-api/constants.tssrc/server/developer-api/data.tssrc/server/developer-api/http.tssrc/server/developer-api/serializers.tssrc/server/developer-api/service.tssrc/server/developer-api/specs.tstests/unit/admin-gear-thumbnail-service.test.tstests/unit/developer-api-cache.test.tstests/unit/developer-api-catalog-route.test.tstests/unit/developer-api-http.test.tstests/unit/developer-api-schemas-serializers.test.tstests/unit/developer-api-service.test.tstests/unit/developer-docs-page.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/{app,src}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Replace any new hardcoded user-facing strings with translation keys for all locales
Files:
src/app/[locale]/(pages)/gear/[slug]/@edit/(.)edit/page.tsxsrc/app/[locale]/(pages)/gear/_components/edit-already-pending-toast.tsxsrc/server/developer-api/cache.tssrc/app/[locale]/(pages)/gear/[slug]/page.tsxsrc/app/api/v1/catalog/route.tssrc/server/developer-api/constants.tssrc/app/[locale]/(pages)/gear/[slug]/edit/page.tsxsrc/app/[locale]/(pages)/gear/_components/edit-already-pending-redirect.tsxsrc/server/developer-api/specs.tssrc/app/[locale]/(pages)/developer/docs/page.tsxsrc/server/admin/proposals/service.tssrc/server/developer-api/http.tssrc/server/developer-api/service.tssrc/server/admin/gear/service.tssrc/server/developer-api/serializers.tssrc/server/developer-api/data.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Include or update automated tests (vitest and/or playwright) covering the primary path and at least one meaningful edge case when introducing or modifying behavior
Files:
tests/unit/developer-api-cache.test.tstests/unit/developer-api-http.test.tstests/unit/developer-api-catalog-route.test.tstests/unit/developer-api-service.test.tstests/unit/developer-docs-page.test.tstests/unit/admin-gear-thumbnail-service.test.tstests/unit/developer-api-schemas-serializers.test.ts
messages/**/*.json
📄 CodeRabbit inference engine (AGENTS.md)
Maintain translation key parity: if a key is added, removed, or renamed in
messages/en.json, apply the same key change across all locale files inmessages/directory
Files:
messages/en.jsonmessages/de.jsonmessages/it.jsonmessages/ms.jsonmessages/es.jsonmessages/ja.jsonmessages/zh.jsonmessages/fr.json
🪛 Betterleaks (1.6.1)
src/app/[locale]/(pages)/developer/docs/page.tsx
[high] 203-204: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 236-237: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
docs/developer-api.md
[high] 62-62: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🔇 Additional comments (20)
docs/developer-api.md (1)
36-70: LGTM!Also applies to: 71-77
src/app/[locale]/(pages)/developer/docs/page.tsx (1)
189-245: LGTM!Also applies to: 270-272, 287-303
tests/unit/developer-docs-page.test.ts (1)
66-86: LGTM!Also applies to: 87-89
messages/en.json (1)
1896-1901: LGTM!src/server/developer-api/constants.ts (1)
7-16: LGTM!tests/unit/developer-api-catalog-route.test.ts (1)
1-78: LGTM!tests/unit/developer-api-http.test.ts (1)
67-88: LGTM!src/server/admin/proposals/service.ts (1)
42-63: LGTM!tests/unit/developer-api-cache.test.ts (1)
10-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign test with the correct
revalidateTagsignature.As flagged in
cache.ts, the Next.jsrevalidateTagAPI accepts only a single argument (tag: string). Asserting that it is called with a second{ expire: 0 }argument expects invalid usage that will fail typechecking once restored.💚 Proposed fix
- expect(revalidateTag).toHaveBeenCalledWith("developer-api-catalog", { - expire: 0, - }); + expect(revalidateTag).toHaveBeenCalledWith("developer-api-catalog");> Likely an incorrect or invalid review comment.src/server/developer-api/data.ts (1)
3-106: LGTM!src/server/developer-api/service.ts (1)
4-130: LGTM!Also applies to: 450-523
src/server/developer-api/specs.ts (1)
9-9: LGTM!Also applies to: 56-56
tests/unit/developer-api-service.test.ts (1)
1-28: LGTM!Also applies to: 33-126
src/server/developer-api/serializers.ts (1)
7-25: LGTM!Also applies to: 35-359, 379-379, 435-474
tests/unit/developer-api-schemas-serializers.test.ts (1)
4-7: LGTM!Also applies to: 47-100, 141-450
src/server/developer-api/http.ts (1)
18-32: LGTM!Also applies to: 109-118
src/app/api/v1/catalog/route.ts (1)
1-27: LGTM!src/app/[locale]/(pages)/gear/[slug]/@edit/(.)edit/page.tsx (1)
3-3: LGTM!Also applies to: 36-44
src/app/[locale]/(pages)/gear/[slug]/edit/page.tsx (1)
4-4: LGTM!Also applies to: 53-61
src/app/[locale]/(pages)/gear/[slug]/page.tsx (1)
63-63: LGTM!Also applies to: 260-260
1 participant
Summary
GET /api/v1/catalogwith deterministic versioning, ETags, conditional304responses, and catalog-specific invalidationValidation
200catalog response followed by conditional304npm run typecheckremains blocked by the existing unrelated error intests/unit/spec-registry-i18n.test.ts:257.