fix(api): resolve issue causing the props API to fail when deployed#223
Open
wise-king-sullyman wants to merge 3 commits intomainfrom
Open
fix(api): resolve issue causing the props API to fail when deployed#223wise-king-sullyman wants to merge 3 commits intomainfrom
wise-king-sullyman wants to merge 3 commits intomainfrom
Conversation
Contributor
WalkthroughMigrates props data loading from runtime Node.js filesystem reads to HTTP-based fetching. Adds a prerendered Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant API as Props API Route
participant FetchUtil as fetchProps(url)
participant Asset as /props.json (Server Asset)
rect rgba(200,200,255,0.5)
Client->>API: GET /api/.../props (with request URL)
end
rect rgba(200,255,200,0.5)
API->>FetchUtil: fetchProps(request.url)
FetchUtil->>Asset: HTTP GET /props.json (origin)
Asset-->>FetchUtil: 200 + JSON (or error)
end
rect rgba(255,200,200,0.5)
FetchUtil-->>API: props JSON (or throws)
API-->>Client: 200 JSON (or 4xx/5xx with error details)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Deploying patternfly-doc-core with
|
| Latest commit: |
d1589af
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cb9adc09.patternfly-doc-core.pages.dev |
| Branch Preview URL: | https://fix-props-api-when-deployed.patternfly-doc-core.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/props.ts (1)
5-13:⚠️ Potential issue | 🟠 MajorValidate
componentsand normalize fetch failures on/props.This handler now has the same runtime dependency on
/props.jsonas the versioned API route, but it does not catchfetchProps()failures. A missing asset or failed subrequest will bubble into Astro’s default 500 response, and requests withoutcomponentsstill do the fetch and return200with an empty body.Proposed fix
export async function GET({ request }: { request: Request }) { const url = new URL(request.url) - const props = await fetchProps(url) - const components = url.searchParams.get('components') - const componentsArray = components?.split(',') - const propsData = componentsArray?.map((component) => props[component]) - - return new Response(JSON.stringify(propsData)) + if (!components) { + return Response.json( + { error: 'components query parameter is required' }, + { status: 400 }, + ) + } + + try { + const props = await fetchProps(url) + const propsData = components + .split(',') + .map((component) => props[component.trim()]) + + return Response.json(propsData) + } catch (error) { + const details = error instanceof Error ? error.message : String(error) + return Response.json( + { error: 'Failed to load props data', details }, + { status: 500 }, + ) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/props.ts` around lines 5 - 13, Validate the incoming components query and guard the fetch: in GET, read url.searchParams.get('components') and if it's missing or empty return an early Response with a non-2xx status (e.g. 400) and a JSON error body instead of calling fetchProps; only call fetchProps when components is present. Wrap the await fetchProps(url) call in try/catch to normalize subrequest failures—on error return a Response with a non-200 status (e.g. 502) and a JSON error message. After a successful fetch, split components into componentsArray and map to propsData using props[component] but ensure missing keys produce null (not undefined) and always return a JSON array body. Reference: GET, fetchProps, components, componentsArray, propsData.
🧹 Nitpick comments (1)
src/utils/propsData/fetch.ts (1)
8-16: Add direct coverage for this helper.The route suites now mock
fetchProps(), so the new URL construction andresponse.okhandling introduced here are untested. A small unit test around this helper would make the Cloudflare fix much harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/propsData/fetch.ts` around lines 8 - 16, Add a unit test for fetchProps that exercises the new URL construction and response.ok handling: call fetchProps(new URL('https://example.com/some/path')) and assert the helper requested new URL('/props.json', url.origin) (i.e. https://example.com/props.json) by mocking global fetch and verifying the requested URL, verify that when the mocked response.ok is true it returns the parsed JSON, and when response.ok is false the function (fetchProps) throws an Error with the status and statusText; use the fetchProps symbol and inspect the propsUrl request in the mocked fetch to ensure full coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/props.json.ts`:
- Around line 21-30: The catch in src/pages/props.json.ts currently returns a
static 500 Response (and tries to include details: error which stringifies to
{}), so change it to fail the build instead: in the catch block of the code that
reads props data (the try/catch surrounding the props file read), rethrow the
original error (or throw a new Error with the original error.message) instead of
returning new Response, so the build/prerender process aborts and you don't ship
a broken /props.json; do not attempt to JSON.stringify the Error object for the
response.
---
Outside diff comments:
In `@src/pages/props.ts`:
- Around line 5-13: Validate the incoming components query and guard the fetch:
in GET, read url.searchParams.get('components') and if it's missing or empty
return an early Response with a non-2xx status (e.g. 400) and a JSON error body
instead of calling fetchProps; only call fetchProps when components is present.
Wrap the await fetchProps(url) call in try/catch to normalize subrequest
failures—on error return a Response with a non-200 status (e.g. 502) and a JSON
error message. After a successful fetch, split components into componentsArray
and map to propsData using props[component] but ensure missing keys produce null
(not undefined) and always return a JSON array body. Reference: GET, fetchProps,
components, componentsArray, propsData.
---
Nitpick comments:
In `@src/utils/propsData/fetch.ts`:
- Around line 8-16: Add a unit test for fetchProps that exercises the new URL
construction and response.ok handling: call fetchProps(new
URL('https://example.com/some/path')) and assert the helper requested new
URL('/props.json', url.origin) (i.e. https://example.com/props.json) by mocking
global fetch and verifying the requested URL, verify that when the mocked
response.ok is true it returns the parsed JSON, and when response.ok is false
the function (fetchProps) throws an Error with the status and statusText; use
the fetchProps symbol and inspect the propsUrl request in the mocked fetch to
ensure full coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f668236-e95b-4746-b155-849f781e1bdc
📒 Files selected for processing (7)
src/__tests__/pages/api/__tests__/[version]/[section]/[page]/props.test.tssrc/__tests__/pages/api/__tests__/[version]/[section]/names.test.tssrc/pages/api/[version]/[section]/[page]/props.tssrc/pages/api/[version]/[section]/names.tssrc/pages/props.json.tssrc/pages/props.tssrc/utils/propsData/fetch.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pages/props.ts (1)
19-23: Non-existent component names result innullvalues in the response.When a requested component name doesn't exist in
props,props[component.trim()]returnsundefined, which serializes tonullin JSON. The consumer receives an array potentially containingnullentries with no indication of which components were not found.If this is intentional behavior, consider documenting it. Otherwise, consider filtering out undefined values or returning structured data that indicates which components were found vs. missing.
♻️ Option: Filter undefined values
const propsData = components .split(',') .map((component) => props[component.trim()]) + .filter((data) => data !== undefined) return createJsonResponse(propsData)♻️ Option: Return structured response with found/missing info
- const propsData = components - .split(',') - .map((component) => props[component.trim()]) - - return createJsonResponse(propsData) + const componentList = components.split(',').map((c) => c.trim()) + const propsData = componentList.reduce( + (acc, component) => { + const data = props[component] + if (data !== undefined) { + acc[component] = data + } + return acc + }, + {} as Record<string, any>, + ) + + return createJsonResponse(propsData)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/props.ts` around lines 19 - 23, The current mapping in propsData uses props[component.trim()] which yields undefined for missing component names and becomes null in the JSON; update the logic in the components.split(...) -> propsData creation so you return a structured result per requested name (e.g., { name, found: boolean, value?: any }) instead of raw values, by iterating over components, trimming each into a variable, looking up props[name], and pushing an object that sets found = true and value = props[name] when present or found = false when missing; then pass that structured array into createJsonResponse so consumers can see which components were found vs missing (refer to symbols: components, props, propsData, createJsonResponse).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pages/props.ts`:
- Around line 19-23: The current mapping in propsData uses
props[component.trim()] which yields undefined for missing component names and
becomes null in the JSON; update the logic in the components.split(...) ->
propsData creation so you return a structured result per requested name (e.g., {
name, found: boolean, value?: any }) instead of raw values, by iterating over
components, trimming each into a variable, looking up props[name], and pushing
an object that sets found = true and value = props[name] when present or found =
false when missing; then pass that structured array into createJsonResponse so
consumers can see which components were found vs missing (refer to symbols:
components, props, propsData, createJsonResponse).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 365626a2-6ec5-46f0-a19e-e48750f4921b
📒 Files selected for processing (2)
src/pages/props.json.tssrc/pages/props.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/props.json.ts
kmcfaul
requested changes
Mar 12, 2026
Collaborator
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
Routes work on the preview! 🎉 (though with one caveat due to a pre-existing bug in the comment below)
I also noticed that there might be an issue with missing entries in the props data (DataList is missing, but its subcomponents are there). I've looked in the full props.json that gets created locally and it's missing there too so this is probably a separate problem with how we're doing the fetching.
| const props = JSON.parse(propsDataFile.toString()) | ||
|
|
||
| const props = await fetchProps(url) | ||
| const propsData = props[sentenceCase(removeSubsection(page))] |
Collaborator
There was a problem hiding this comment.
sentenceCase needs to be removed here.
I'm not remembering my thought process as to why I included it initially (maybe I thought to manually format the props.json), but this is causing lookups on subcomponents or really anything with multiple words in camel/pascal case or hyphens to fail because it's inserting spaces.
This will mean that the route is case sensitive unless we add some more logic to make the data format consistent, such as formatting the props data to use lowercase without any spaces/hyphens and manipulating the url param into that format. But I think at least for now we can just remove it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #214
Assisted by Claude Code
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests