feat(api): add api endpoint for component props #198
Conversation
WalkthroughAdds two new versioned API endpoints: one lists component names with props and another returns props for a specific component. Both load config to locate Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Astro API Route
participant Config as getConfig (pf-docs.config.mjs)
participant FS as Filesystem (props.json)
Client->>API: GET /api/{version}/{section}/{page}/props
API->>API: validate params (version, section, page)
API->>Config: load config to determine outputDir
Config-->>API: outputDir
API->>FS: read `${outputDir}/props.json`
FS-->>API: props.json content
API->>API: select props by sentenceCased(page)
alt props found
API-->>Client: 200 JSON (props)
else not found
API-->>Client: 404 JSON (not found)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 4
🤖 Fix all issues with AI agents
In `@src/pages/api/`[version]/[section]/[page]/props.ts:
- Around line 28-29: The handler currently assigns const propsData =
props[sentenceCase(page)] and always returns createJsonResponse(propsData);
instead, detect when propsData is undefined/null and return a 404 response for
missing component props. Modify the code around propsData (lookup using props
and sentenceCase(page)) so that if propsData == null you return an HTTP 404 (for
example by calling your JSON response helper with a 404 status or returning a
404 Response) otherwise continue to return createJsonResponse(propsData).
- Line 6: Replace the direct import of sentenceCase from the external
'change-case' package with the project's custom wrapper to ensure
PatternFly-specific casing; update the import to pull sentenceCase from the
local module (the wrapper defined in src/utils/case.ts) and keep all usages of
sentenceCase (the function) unchanged so component name lookups use the
PatternFly-aware casing logic.
In `@src/pages/api/openapi.json.ts`:
- Around line 266-282: The OpenAPI response block under responses '200' uses an
invalid media type key "array"; update the content key to a valid MIME type
("application/json") for the '200' response and keep the existing schema (type:
"array", items: { type: "string" }) and example intact. Locate the responses ->
'200' object in src/pages/api/openapi.json.ts (the schema and example currently
under the "array" key) and replace that key with "application/json" so the
OpenAPI document validates.
- Around line 412-450: The responses.content entry incorrectly uses the media
type key "array" instead of a valid media type; update the responses -> '200' ->
content block (the schema and example under content) to use "application/json"
as the media type key and keep the existing schema, items, properties and
example intact so the OpenAPI response for the component props remains the same
structure but under a valid content type.
🧹 Nitpick comments (4)
src/pages/api/[version]/[section]/names.ts (2)
31-36: Consider returning 404 for missingprops.jsoninstead of 500.Per the PR objectives, the endpoint should "return 404 when props do not exist for the requested component" and "handle the case where props.json has not been generated." Currently, all errors (including
ENOENTfor missing file) return a 500 status. Differentiating between "file not found" (404) and unexpected errors (500) would improve API clarity.Suggested improvement
} catch (error) { + if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { + return createJsonResponse( + { error: 'Component names data not found' }, + 404, + ) + } const details = error instanceof Error ? error.message : String(error) return createJsonResponse( - { error: 'Component names data not found', details }, + { error: 'Failed to load component names', details }, 500, ) }
39-41: Remove trailing blank lines.Minor: there are multiple trailing blank lines at the end of the file.
src/pages/api/[version]/[section]/[page]/props.ts (2)
31-36: Same error handling improvement asnames.ts—differentiate 404 from 500.Consider returning 404 for
ENOENT(missingprops.json) and 500 for unexpected errors, aligning with PR objectives.
39-41: Remove trailing blank lines.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
src/pages/api/[version]/[section]/[page]/props.tssrc/pages/api/[version]/[section]/names.tssrc/pages/api/index.tssrc/pages/api/openapi.json.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/api/[version]/[section]/names.ts (4)
src/pages/api/[version]/[section]/[page]/props.ts (2)
prerender(8-8)GET(10-38)src/pages/api/openapi.json.ts (2)
prerender(15-15)GET(17-824)src/utils/apiHelpers.ts (1)
createJsonResponse(18-27)cli/getConfig.ts (1)
getConfig(23-37)
src/pages/api/[version]/[section]/[page]/props.ts (3)
src/utils/apiHelpers.ts (1)
createJsonResponse(18-27)cli/getConfig.ts (1)
getConfig(23-37)src/utils/case.ts (1)
sentenceCase(12-16)
🔇 Additional comments (5)
src/pages/api/[version]/[section]/names.ts (1)
19-30: Theversionandsectionparameters are validated but not used.The endpoint validates that
versionandsectionare provided, but the implementation reads the sameprops.jsonand returns all component names regardless of these parameters. This creates a semantic inconsistency—the URL path suggests version/section scoping, but the response is global.If intentional (e.g., props.json is already version-specific at build time), consider documenting this behavior. Otherwise, the implementation should filter or route based on these parameters.
src/pages/api/index.ts (2)
101-130: LGTM!The documentation for the new
/namesendpoint is accurate and follows the established pattern in this file.
165-204: LGTM!The documentation for the new
/propsendpoint is well-structured with appropriate examples.src/pages/api/openapi.json.ts (2)
284-298: Spec documents 404 but implementation returns 500 for all errors.The OpenAPI spec shows a 404 response for "Props not found," but the current implementation in
names.tsreturns 500 for all errors. Consider aligning the implementation with the spec by returning 404 whenprops.jsonis missing.
452-466: Same spec/implementation mismatch for 404 response.The 404 response is documented here, but
props.tsreturns 500 for file errors and 200 with undefined for missing components. Align with the earlier suggestion to return proper 404 responses.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/pages/api/`[version]/[section]/[page]/props.ts:
- Around line 10-18: The handler currently validates params in GET ({ params })
but never uses version or section when loading props.json; update the handler to
incorporate version and section into the props lookup path (e.g., build a scoped
path using version and section along with page when reading props.json) so
multi-version/section content is supported, and keep the existing parameter
validation and error response via createJsonResponse; alternatively, if the
routing-only intent is desired, add a clear comment above GET explaining that
version and section are validated for routing but intentionally not used for
props resolution (mirror the same choice in names.ts).
🧹 Nitpick comments (2)
src/pages/api/[version]/[section]/[page]/props.ts (2)
24-26: Prefer async file reading to avoid blocking the event loop.
readFileSyncblocks the Node.js event loop. In an SSR API handler, this can degrade performance under concurrent requests. Use the asyncfs.promises.readFileinstead.♻️ Suggested refactor
-import { readFileSync } from 'node:fs' +import { readFile } from 'node:fs/promises'const propsFilePath = join(outputDir, 'props.json') - const propsDataFile = readFileSync(propsFilePath) - const props = JSON.parse(propsDataFile.toString()) + const propsDataFile = await readFile(propsFilePath, 'utf-8') + const props = JSON.parse(propsDataFile)
39-45: Consider distinguishing "file not found" from other server errors.When
props.jsondoesn't exist, the current code returns a 500 error. Per PR objectives, the endpoint should gracefully handle the absence ofprops.json. A 404 would be more semantically appropriate for missing data, reserving 500 for unexpected failures.♻️ Suggested approach
} catch (error) { + if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { + return createJsonResponse( + { error: 'Props data not available' }, + 404, + ) + } const details = error instanceof Error ? error.message : String(error) return createJsonResponse( { error: 'Props data not found', details }, 500, ) }
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
Couple small things:
| const { version, section } = params | ||
|
|
||
| if (!version || !section) { | ||
| return createJsonResponse( | ||
| { error: 'Version and section parameters are required' }, | ||
| 400, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Looks like we're also not using the version and section here other than just to validate the path
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
Oh also can you add tests for these new routes?
dlabaj
left a comment
dlabaj
left a comment
There was a problem hiding this comment.
Can you add some unit tests?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/pages/api/`[version]/[section]/names.ts:
- Line 9: The exported handler GET is declared with an unused empty
destructuring parameter ({ }), causing a lint error; update the declaration to
remove the empty pattern and use no parameter (declare GET: APIRoute = async ()
=> { ... }) so the function signature matches usage and the linter no longer
complains; ensure the symbol GET is updated accordingly and no other code
expects a parameter.
🧹 Nitpick comments (1)
src/__tests__/pages/api/__tests__/[version]/[section]/[page]/props.test.ts (1)
295-321: Consider strengthening these assertions.The conditional
ifchecks could silently pass if the mock data structure changes. Since the mock data is controlled, you can assert more directly.Suggested improvement
it('handles props with defaultValue field', async () => { const response = await GET({ params: { version: 'v6', section: 'components', page: 'keyboard-handler' }, url: new URL('http://localhost:4321/api/v6/components/keyboard-handler/props'), } as any) const body = await response.json() expect(response.status).toBe(200) - const propWithDefault = body.props.find((p: any) => p.defaultValue !== undefined) - if (propWithDefault) { - expect(propWithDefault).toHaveProperty('defaultValue') - } + const propWithDefault = body.props.find((p: any) => p.defaultValue !== undefined) + expect(propWithDefault).toBeDefined() + expect(propWithDefault.defaultValue).toBe('null') }) it('handles props with required field', async () => { const response = await GET({ params: { version: 'v6', section: 'components', page: 'sample-data-row' }, url: new URL('http://localhost:4321/api/v6/components/sample-data-row/props'), } as any) const body = await response.json() expect(response.status).toBe(200) - const requiredProp = body.props.find((p: any) => p.required === true) - if (requiredProp) { - expect(requiredProp.required).toBe(true) - } + const requiredProp = body.props.find((p: any) => p.required === true) + expect(requiredProp).toBeDefined() + expect(requiredProp.required).toBe(true) })
|
|
||
| export const prerender = false | ||
|
|
||
| export const GET: APIRoute = async ({ }) => { |
There was a problem hiding this comment.
Remove the empty object destructuring pattern.
The empty destructuring { } triggers a lint error and serves no purpose. Since the handler doesn't use any context properties, remove it entirely.
Suggested fix
-export const GET: APIRoute = async ({ }) => {
+export const GET: APIRoute = async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const GET: APIRoute = async ({ }) => { | |
| export const GET: APIRoute = async () => { |
🧰 Tools
🪛 Biome (2.1.2)
[error] 9-9: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🤖 Prompt for AI Agents
In `@src/pages/api/`[version]/[section]/names.ts at line 9, The exported handler
GET is declared with an unused empty destructuring parameter ({ }), causing a
lint error; update the declaration to remove the empty pattern and use no
parameter (declare GET: APIRoute = async () => { ... }) so the function
signature matches usage and the linter no longer complains; ensure the symbol
GET is updated accordingly and no other code expects a parameter.
|
🎉 This PR is included in version 1.18.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Closes #170.
Adds
/api/{version}/{section}/{page}/propsand/api/{version}/{section}/namesendpoints.Based on slack conversation, the props endpoint is currently scoped to directly querying a single component for its props.
Eventually we would like subcomponent props to also be returned when applicable, but we need to generate a mapping of all our component families to properly fetch the data (as string matching based on the queried component name would likely not be a comprehensive list - "ProgressStepper" not returning "ProgressStep" props, or return too much data - "Alert" returning "FormAlert" props). In the interim I've added another endpoint to return all available component names based on our generated props data, so a user/AI could query that initially to understand what components props are available than try to guess at component/subcomponent names.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.