fix(API): address issues with text API in monorepos #185
Conversation
Deploying patternfly-doc-core with
|
| Latest commit: |
8e9d674
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://19c22c95.patternfly-doc-core.pages.dev |
| Branch Preview URL: | https://fix-text-api-monorepo-issues.patternfly-doc-core.pages.dev |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
patternfly-doc-core | 9fd87fb | Dec 05 2025, 05:49 PM |
wise-king-sullyman
requested review from
dlabaj
and removed request for
dlabaj
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR moves the API index to a generated file placed in the consumer-configured outputDir at build time, adds a prerendered /apiIndex.json route and a runtime fetch utility, updates API routes to fetch that index with error handling, introduces a safe default-tab resolver for API content, and updates CLI/build steps to generate and copy the index. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (9)
.gitignore (1)
37-38: Consider reorganizing the.wrangler/entry for clarity.The addition of
.wrangler/is correct and necessary (Cloudflare Workers CLI artifacts should be ignored). However, the placement after the "Ignore generated files" section without a grouping comment is slightly inconsistent with the rest of the file's organization.For better clarity, consider moving it to the top with other build output directories (near
dist/and.astro/) or adding a brief comment explaining why it's being ignored.Optional refactor:
# build output dist/ +.wrangler/ # generated types .astro/Alternatively, keep it where it is but add a clarifying comment:
coverage/ +# Cloudflare Workers build artifacts .wrangler/src/utils/apiIndex/fetch.ts (1)
1-19: Runtime fetch helper matches newapiIndex.jsonroute; consider tightening typingThe URL construction and error handling look good and align with the new
/apiIndex.jsonroute.To get stronger typing on the parsed payload, you could assert the JSON type:
- return response.json() + const body = (await response.json()) as ApiIndex + return bodyThis keeps the function’s
Promise<ApiIndex>contract explicit instead of relying onanyfromResponse.json().package.json (1)
21-29: CLI-driven deploy and clean script are sensible; watch cross-platformrmRouting
deploythroughnode ./dist/cli/cli.js deployafterbuild:cliis a good way to ensure the CLI is built before deployment.The new
cleanscript:"clean": "rm -rf dist .astro .wrangler"is fine on POSIX shells but will fail on Windows. If you need cross‑platform support, consider switching to a Node-based tool (e.g.,
rimraf) and wiring it in here.README.mdx (1)
36-36: Dev-mode note is helpful; consider naming the build command explicitlyThe note about API endpoints not being available on a clean repo until a build has been done accurately reflects the new build‑time index generation.
Optionally, you could make it a bit clearer by referencing the actual command, e.g., “until
npm run build(or your equivalent build script) has been run.”src/utils/packageUtils.ts (1)
64-65: Explicit'text'fallback for API routes is clear and minimal
The simplification of
getDefaultTabto:return addDemosOrDeprecated(tabBase, filePath)is a no-op behaviorally and improves readability.
The new API-specific helper:
export const getDefaultTabForApi = (filePath?: string): string => getDefaultTab(filePath) || 'text'cleanly defines
'text'as the fallback when package-based tab inference fails (e.g., for local, non–node_modulescontent), which directly addresses the “no tab name can be determined” scenario from the issue.If you expect
'text'to be a long-term, public default, you might eventually want to centralize it as a named constant, but the current implementation is perfectly fine.Also applies to: 67-69
src/pages/apiIndex.json.ts (1)
1-27: Build-timeapiIndex.jsonroute is wired correctly; consider clearer error detailsThe prerendered
/apiIndex.jsonendpoint that callsgetApiIndex()is a good fit with the newfetchApiIndex(url)helper and keeps Cloudflare Worker runtime lean.In the error branch you currently return:
JSON.stringify({ error: 'Failed to load API index', details: error })Serializing an
Errorobject usually yields{}; if you want meaningful diagnostics, you could optionally map it to a string:} catch (error) { const details = error instanceof Error ? error.message : String(error) return new Response( JSON.stringify({ error: 'Failed to load API index', details }), { status: 500, headers: { 'Content-Type': 'application/json' } } ) }(Or omit
detailsentirely if you’d rather not expose internal messages.)src/__tests__/pages/api/__tests__/versions.test.ts (1)
3-66: Fetch-based versions API tests look correct; consider centralizing the fetch mockThe tests correctly adapt to the new
fetchApiIndex(url)behavior by mockingglobal.fetchand passing a realURLintoGET, and they assert status, content type, and body shape appropriately.For readability and to avoid repetition, you could move the
global.fetchsetup andjest.restoreAllMocks()intobeforeEach/afterEach(or a small helper) and optionally add one test that simulates a non‑OKfetchresponse to exercise the 500 error path.src/__tests__/pages/api/__tests__/[version]/[section]/[page]/[tab].test.ts (1)
78-132: Test mock forgetDefaultTabForApidiverges from real fallback behaviorThe mocked
getDefaultTabForApihere falls back to'react'when it can’t infer from the path, whereas the real implementation falls back to'text'. For the current tests (which always use explicitreact/html/react-demostabs), this doesn’t change the outcome, but aligning the mock’s fallback with the real function would make these tests more representative and future‑proof if you later add scenarios that rely on the fallback.src/utils/apiIndex/get.ts (1)
74-78: Consider hoisting the dynamic import.The
createIndexKeydynamic import insidegetPagesandgetTabs(line 91) is executed on every call. If there's no circular dependency concern, moving this to a top-level import would be cleaner:+import { createIndexKey } from '../apiHelpers' // Then in getPages and getTabs: - const { createIndexKey } = await import('../apiHelpers') const key = createIndexKey(version, section)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.gitignore(1 hunks)README.mdx(1 hunks)cli/cli.ts(7 hunks)jest.config.ts(1 hunks)package.json(1 hunks)src/__tests__/pages/api/__tests__/[version].test.ts(2 hunks)src/__tests__/pages/api/__tests__/[version]/[section]/[page]/[tab].test.ts(1 hunks)src/__tests__/pages/api/__tests__/versions.test.ts(1 hunks)src/pages/api/[version].ts(2 hunks)src/pages/api/[version]/[section].ts(2 hunks)src/pages/api/[version]/[section]/[page].ts(2 hunks)src/pages/api/[version]/[section]/[page]/[tab].ts(3 hunks)src/pages/api/openapi.json.ts(1 hunks)src/pages/api/versions.ts(1 hunks)src/pages/apiIndex.json.ts(1 hunks)src/utils/__tests__/packageUtils.test.ts(2 hunks)src/utils/apiIndex/fetch.ts(1 hunks)src/utils/apiIndex/generate.ts(3 hunks)src/utils/apiIndex/get.ts(2 hunks)src/utils/getOutputDir.ts(1 hunks)src/utils/packageUtils.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
src/utils/__tests__/packageUtils.test.ts (1)
src/utils/packageUtils.ts (1)
getDefaultTabForApi(69-69)
src/utils/apiIndex/fetch.ts (1)
src/utils/apiIndex/generate.ts (1)
ApiIndex(37-46)
src/utils/getOutputDir.ts (1)
cli/getConfig.ts (1)
getConfig(22-36)
src/pages/apiIndex.json.ts (2)
src/pages/api/openapi.json.ts (2)
prerender(5-5)GET(7-397)src/pages/api/versions.ts (2)
prerender(5-5)GET(7-17)
src/utils/apiIndex/generate.ts (3)
src/utils/packageUtils.ts (1)
getDefaultTabForApi(69-69)src/utils/apiIndex/index.ts (2)
writeApiIndex(3-3)ApiIndex(5-5)src/utils/getOutputDir.ts (1)
getOutputDir(4-19)
src/pages/api/[version]/[section]/[page].ts (3)
src/pages/api/index.ts (1)
GET(6-172)src/utils/apiIndex/fetch.ts (1)
fetchApiIndex(10-19)src/utils/apiHelpers.ts (2)
createIndexKey(48-50)createJsonResponse(18-27)
src/__tests__/pages/api/__tests__/[version].test.ts (2)
src/pages/api/[version].ts (1)
GET(7-32)src/pages/api/versions.ts (1)
GET(7-17)
src/utils/apiIndex/get.ts (1)
src/utils/getOutputDir.ts (1)
getOutputDir(4-19)
src/pages/api/[version]/[section]/[page]/[tab].ts (1)
src/utils/packageUtils.ts (1)
getDefaultTabForApi(69-69)
src/pages/api/openapi.json.ts (2)
src/utils/apiIndex/fetch.ts (1)
fetchApiIndex(10-19)src/utils/apiHelpers.ts (1)
createJsonResponse(18-27)
src/__tests__/pages/api/__tests__/versions.test.ts (1)
src/pages/api/versions.ts (1)
GET(7-17)
cli/cli.ts (4)
cli/fileExists.ts (1)
fileExists(3-7)cli/getConfig.ts (1)
DocsConfig(15-20)pf-docs.config.mjs (2)
config(1-33)config(1-33)cli/templates/pf-docs.config.mjs (2)
config(1-32)config(1-32)
src/pages/api/versions.ts (3)
src/pages/apiIndex.json.ts (2)
prerender(5-5)GET(7-27)src/utils/apiIndex/fetch.ts (1)
fetchApiIndex(10-19)src/utils/apiHelpers.ts (1)
createJsonResponse(18-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (13)
jest.config.ts (1)
19-23:outputDirJest alias looks correct and ordering is saneThe new moduleNameMapper for
'^outputDir/(.*)$' -> '<rootDir>/dist/$1'is consistent with usingoutputDir/*as a dist alias and is placed before the generic(.+)\\.jsmatcher, so it won’t be shadowed.Once the tsconfig
pathsentry is made relative todist/*, this should keep TypeScript and Jest resolution in sync.src/pages/api/[version]/[section]/[page]/[tab].ts (1)
9-10: Fallback tab handling and tabless-path filtering look robust
Importing and using
getDefaultTabForApifor thedata.tabdefault:tab: data.tab || data.source || getDefaultTabForApi(filePath),ensures that API content always has a tab value, falling back to
'text'when package-based logic doesn’t apply (e.g., local files in a consuming repo).The additional safety pass over
paths:paths.forEach((path) => { if (!path.params.tab) { console.warn(`[API Warning] Tab not found for path: ...`) } }) return paths.filter((path) => !!path.params.tab)guards against any tabless entries that might slip through index generation, preventing build-time crashes while still surfacing a clear warning.
This aligns well with the goal of handling tabless edge cases gracefully without breaking prerendering.
Also applies to: 47-52, 55-55, 120-120
src/pages/api/[version].ts (1)
2-31: Dynamic API index fetch and version-specific error handling look solidThe handler cleanly validates
version, fetches the index withfetchApiIndex(url), differentiates between missing param (400), unknown version (404), and underlying fetch failures (500), and usescreateJsonResponseconsistently. This aligns well with the updated tests and the new index-fetching utility.src/pages/api/versions.ts (1)
3-16:/api/versionsnow correctly delegates tofetchApiIndexwith robust error handlingThe route cleanly switches from static data to
fetchApiIndex(url), returningindex.versionson success and a structured 500 response on failure. This is consistent with the helper utilities and the updated tests mockingglobal.fetch.src/pages/api/[version]/[section]/[page].ts (1)
3-37: Page-level route correctly uses the shared index and distinguishes 400/404/500 pathsThe updated handler validates
version/section/page, fetches the shared index viafetchApiIndex(url), usescreateIndexKeyto look up tabs, and returns clear 404 and 500 JSON responses in failure cases. This matches the generator’s keying strategy and the tests that assert not-found behavior.src/utils/getOutputDir.ts (1)
1-19:getOutputDircleanly validates configuration and surfaces helpful errorsThe new helper correctly reuses
getConfig, distinguishes between “no config file” and “nooutputDirin config”, and returns the configuredoutputDirstring. This is a good central point for monorepo-aware path resolution and will make failures much easier to diagnose.src/utils/__tests__/packageUtils.test.ts (1)
1-261: Comprehensive tests forgetDefaultTabForApiaccurately capture primary and fallback behaviorThe new test suite thoroughly exercises
getDefaultTabForApi, covering standard PatternFly/react-core paths as well as all the fallback cases wheregetDefaultTabreturns an empty string. This matches the implementation’s|| 'text'behavior and should prevent regressions in default tab resolution for the API.src/utils/apiIndex/get.ts (1)
13-26: LGTM - Dynamic output directory resolution with proper validation.The structure validation ensures the index file has the expected format before use, and the error handling provides clear guidance for common failure modes (missing file, invalid JSON).
src/utils/apiIndex/generate.ts (2)
110-113: Good fallback implementation for tab resolution.Using
getDefaultTabForApiensures a'text'fallback when neithertabnorsourceis available, directly addressing the PR objective to prevent build crashes when no tab name can be determined.
145-151: Write failure is logged as warning but doesn't propagate.The
catchblock logs a warning and continues silently. If the API index file fails to write, downstream processes that depend on it will fail with a less clear error. Consider whether this should throw or at least return a failure indicator.} catch (error) { - console.warn('Warning: Could not write API index file:', error) + console.error('Error: Could not write API index file:', error) + throw error }If silent failure is intentional for resilience, a comment explaining why would help future maintainers.
cli/cli.ts (3)
166-173: LGTM - API index copy to docs directory.Copying the generated
apiIndex.jsoninto the docs output directory ensures it's available as a static asset for the runtimefetchApiIndexcalls. This correctly bridges the build-time generation with runtime consumption.
232-241: Start command flow looks correct.The sequence of
updateTsConfigOutputDirPath→updateContent→initializeApiIndex→generateProps→devproperly sets up the environment before starting the dev server.
190-198: No changes needed. Thewrangler.jsoncfile is already configured withpages_build_output_dir: "dist/docs", which matches the output location where the build function places the compiled documentation. Thewrangler pages deploycommand will correctly use this configuration, so an explicit directory argument is unnecessary.
tsconfig.json
Outdated
| "paths": { | ||
| "outputDir/*": [ | ||
| "/Users/ausulliv/repos/patternfly-doc-core/dist/*" | ||
| ] | ||
| } | ||
| }, | ||
| "include": [".astro/types.d.ts", "**/*"], | ||
| "exclude": ["dist"] | ||
| } | ||
| "include": [ | ||
| ".astro/types.d.ts", | ||
| "**/*" | ||
| ], | ||
| "exclude": [ | ||
| "dist" | ||
| ] | ||
| } |
There was a problem hiding this comment.
Avoid committing machine-specific absolute paths in compilerOptions.paths
"outputDir/*" currently maps to "/Users/ausulliv/repos/patternfly-doc-core/dist/*", which will only work on the original author’s machine and will break in CI and for other contributors.
You likely want a repo‑relative alias that matches the Jest moduleNameMapper ('<rootDir>/dist/$1') instead:
- "paths": {
- "outputDir/*": [
- "/Users/ausulliv/repos/patternfly-doc-core/dist/*"
- ]
- }
+ "paths": {
+ "outputDir/*": [
+ "dist/*"
+ ]
+ }This keeps the alias portable and consistent with the runtime/test resolution.
📝 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.
| "paths": { | |
| "outputDir/*": [ | |
| "/Users/ausulliv/repos/patternfly-doc-core/dist/*" | |
| ] | |
| } | |
| }, | |
| "include": [".astro/types.d.ts", "**/*"], | |
| "exclude": ["dist"] | |
| } | |
| "include": [ | |
| ".astro/types.d.ts", | |
| "**/*" | |
| ], | |
| "exclude": [ | |
| "dist" | |
| ] | |
| } | |
| "paths": { | |
| "outputDir/*": [ | |
| "dist/*" | |
| ] | |
| } | |
| }, | |
| "include": [ | |
| ".astro/types.d.ts", | |
| "**/*" | |
| ], | |
| "exclude": [ | |
| "dist" | |
| ] | |
| } |
🤖 Prompt for AI Agents
In tsconfig.json around lines 11–24, the compilerOptions.paths entry uses an
absolute, machine-specific path ("/Users/ausulliv/.../dist/*"); replace that
with a repo-relative path so the alias works for CI and other
contributors—change the mapping to point at the repository's dist folder (e.g.
"./dist/*" or "dist/*") so it matches your Jest moduleNameMapper
('<rootDir>/dist/$1') and is portable across machines.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/cli.ts (2)
115-153: API index population is missing from the build flow.The
buildProjectfunction initializes an API index template (line 133) but never populates it with actual data before copying it to the docs directory (line 146). ThegenerateAndWriteApiIndexfunction exists insrc/utils/apiIndex/generate.tsbut is not called during the build process.The current flow copies only the template file (
cli/templates/apiIndex.json) to the docs directory without generating actual API data. To serve a functional API index as a static asset, add a call togenerateAndWriteApiIndex()betweentransformMDContentToMDX()and thebuild()call, or ensure the build process populates the index during content processing.
155-181: Add validation or documentation for Wrangler configuration before deployment.The
wrangler pages deploycommand at line 171 requireswrangler.tomlto be configured in the consumer's project, but:
- The
setupandinitcommands don't create or configurewrangler.toml- The README doesn't document that Wrangler setup is required
- The code doesn't validate that
wrangler.tomlexists before attempting deploymentEither:
- Add validation to check if
wrangler.tomlexists and provide a helpful error message, or- Document the Wrangler setup requirement in the README with example configuration, or
- Modify the deploy command to accept and pass an explicit output directory to
wrangler pages deploy
🧹 Nitpick comments (6)
src/pages/api/[version]/[section].ts (1)
17-36: Error serialization properly addressed; consider caching for performance.The error handling on line 31 correctly serializes the error object, resolving the issue flagged in the past review comment. The logic flow is solid: fetch → lookup → return 404 if missing → return pages on success → catch errors with proper serialization.
However, fetching the API index on every request could impact performance in high-traffic scenarios. Consider adding a caching layer if this becomes a bottleneck.
Example caching approach using Astro's
Astro.localsor a simple in-memory cache:// At module level let cachedIndex: { data: ApiIndex; timestamp: number } | null = null const CACHE_TTL = 60000 // 1 minute export const GET: APIRoute = async ({ params, url }) => { const { version, section } = params if (!version || !section) { return createJsonResponse( { error: 'Version and section parameters are required' }, 400, ) } try { const now = Date.now() if (!cachedIndex || now - cachedIndex.timestamp > CACHE_TTL) { cachedIndex = { data: await fetchApiIndex(url), timestamp: now } } const index = cachedIndex.data const key = createIndexKey(version, section) const pages = index.pages[key] if (!pages) { return createJsonResponse( { error: `Section '${section}' not found for version '${version}'` }, 404, ) } return createJsonResponse(pages) } catch (error) { const details = error instanceof Error ? error.message : String(error) return createJsonResponse( { error: 'Failed to load API index', details }, 500 ) } }src/__tests__/pages/api/__tests__/[version].test.ts (3)
3-11: Type themockApiIndexfixture to stay aligned with the real index schemaConsider annotating
mockApiIndexwith the shared API index type (e.g.,ApiIndexfrom your apiIndex utilities) so test data stays in lockstep with the runtime shape and any schema drift is caught at compile time.
140-160: Simplify redundant expectations in the final testThe checks
expect(body).toEqual(mockApiIndex.sections.v6) expect(body).toEqual(['components', 'layouts', 'utilities'])are effectively asserting the same thing. You can keep either the typed/fixture-based check or the explicit literal array, but you don’t need both.
14-20: Factor out fetch mocking and usejest.spyOn(global, 'fetch')withbeforeEach/afterEachto ensure proper cleanupEach test directly assigns
global.fetch = jest.fn(...)then callsjest.restoreAllMocks(), but direct property assignments are not restored byrestoreAllMocks()—only mocks created viajest.spyOn()orjest.replaceProperty()are reverted. This pattern repeats across all six tests (lines 14–20, 38–44, 58–64, 78–84, 98–104, 120–126, 141–147).Refactor to use
jest.spyOn(global, 'fetch').mockResolvedValue(...)in a sharedbeforeEach, with a singleafterEach(() => jest.restoreAllMocks()). This both eliminates duplication and ensures the originalfetchis properly restored between tests.cli/cli.ts (2)
15-15: Remove unused imports:readFileandwriteFile.These imports from
fs/promisesare not used anywhere in the file. OnlycopyFileis actually utilized.Apply this diff:
-import { mkdir, copyFile, readFile, writeFile } from 'fs/promises' +import { mkdir, copyFile } from 'fs/promises'
188-188: Consider extending dry-run support to build command.The
--dry-runflag is currently only used by thedeploycommand. Consider whether thebuildcommand should also respect this flag to enable a complete dry-run workflow that validates the entire build and deployment process without side effects.If deploy-only dry-run is intentional, this can be safely ignored.
📜 Review details
Configuration used: CodeRabbit 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 (8)
.gitignore(1 hunks)cli/cli.ts(8 hunks)jest.config.ts(1 hunks)package.json(2 hunks)src/__tests__/pages/api/__tests__/[version].test.ts(2 hunks)src/pages/api/[version]/[section].ts(2 hunks)src/pages/api/openapi.json.ts(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- src/pages/api/openapi.json.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/__tests__/pages/api/__tests__/[version].test.ts (2)
src/pages/api/versions.ts (1)
GET(7-17)src/pages/api/[version].ts (1)
GET(7-32)
cli/cli.ts (2)
cli/fileExists.ts (1)
fileExists(3-7)cli/getConfig.ts (1)
DocsConfig(15-20)
src/pages/api/[version]/[section].ts (4)
src/pages/apiIndex.json.ts (1)
GET(7-27)src/pages/api/[version].ts (1)
GET(7-32)src/utils/apiIndex/fetch.ts (1)
fetchApiIndex(10-19)src/utils/apiHelpers.ts (2)
createIndexKey(48-50)createJsonResponse(18-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (22.x)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (10)
src/pages/api/[version]/[section].ts (3)
1-3: LGTM! Imports are clean and support the runtime fetch pattern.The addition of
fetchApiIndexaligns with the PR objective to move from static import to runtime-fetched API index for monorepo support.
5-5: Correctprerendersetting for runtime fetch.Setting
prerender = falseis appropriate here since the route fetches the API index at request time.
7-15: LGTM! Parameter validation is comprehensive.The handler correctly validates both
versionandsectionparameters and returns a clear error message when they're missing. The addition of theurlparameter enables the runtime fetch pattern.src/__tests__/pages/api/__tests__/[version].test.ts (1)
23-23: URL wiring for the route handler looks correctPassing a concrete
URLinstance that matches the requested path intoGETmirrors Astro’s APIRoute context well and keeps these tests close to real runtime behavior.Also applies to: 47-47, 67-67, 87-87, 107-107, 129-129, 150-150
jest.config.ts (1)
21-21: LGTM: Standard ESM compatibility mapping.This mapping correctly handles TypeScript files imported with
.jsextensions (as required by ESM standards), allowing Jest to resolve them to the actual.tsfiles during testing.package.json (3)
21-21: LGTM: Deploy script now uses programmatic CLI.The updated deploy script correctly invokes the CLI's deploy command, aligning with the broader refactoring to support monorepo workflows.
28-29: LGTM: Clean script addition.The new
cleanscript provides a convenient way to remove build artifacts and the trailing comma maintains JSON formatting consistency.
68-68: Verify caret range compatibility with Astro v5 breaking changes.Changing from an exact version (
5.15.9) to a caret range (^5.15.9) allows automatic minor and patch updates. Given the significant breaking changes in Astro v5 (removed experimental flags, Content Layer migration, changed rendering modes, etc.), minor version updates could introduce incompatibilities.Consider whether a tilde range (
~5.15.9) limiting updates to patches only would be safer, or if the caret range has been tested with expected minor version updates.Based on library documentation for Astro v5 breaking changes.
cli/cli.ts (2)
91-113: LGTM: API index initialization now supports monorepos.This function correctly writes the API index template to the consumer's
outputDirinstead of the package directory, addressing the primary issue described in #184. The conditional verbose logging and early return when the file exists improve the developer experience.
209-246: LGTM: Command handlers correctly updated.All command handlers have been properly updated to pass the
programparameter to refactored functions, maintaining consistency across the CLI interface.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/cli.ts (1)
44-46: Add error handling for critical directory creation.The output directory creation at module initialization lacks error handling. If this fails (e.g., due to permission issues), the entire CLI will crash with an unhandled promise rejection before any commands can execute.
Apply this diff to add error handling:
const absoluteOutputDir = resolve(currentDir, config.outputDir) -await mkdir(absoluteOutputDir, { recursive: true }) +try { + await mkdir(absoluteOutputDir, { recursive: true }) +} catch (e: any) { + console.error(`Failed to create output directory ${absoluteOutputDir}:`, e.message) + process.exit(1) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/cli.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli/cli.ts (2)
cli/fileExists.ts (1)
fileExists(3-7)cli/getConfig.ts (1)
DocsConfig(15-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (22.x)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
cli/cli.ts (3)
91-113: LGTM! API index now correctly targets consumer's output directory.The changes correctly address the monorepo issue by placing the API index in the consumer-configured
outputDirrather than the package directory. The early return optimization and verbose logging are appropriate.
188-188: LGTM! CLI command wiring is consistent and correct.The addition of the
--dry-runoption and the consistent updates to pass theprogramobject to all command handlers ensure proper option handling throughout the CLI. The start command now properly initializes the API index for the dev server.Also applies to: 209-211, 221-221, 246-246
136-141: No action required. The Astro v5 build configuration is correct.The
outDirparameter in thebuild()call is standard Astro API usage and works properly with Astro v5. Theastro.config.mjscontains no conflictingbuild.outDir,build.client, orbuild.serverconfigurations. Static assets and routes resolve correctly with this setup.Likely an incorrect or invalid review comment.
| try { | ||
| // First build the project | ||
| const config = await buildProject() | ||
| if (config) { | ||
| if (verbose) { | ||
| console.log('Build complete, deploying to Cloudflare...') | ||
| } | ||
|
|
||
| // Deploy using Wrangler | ||
| const { execSync } = await import('child_process') | ||
| const outputPath = join(absoluteOutputDir, 'docs') | ||
|
|
||
| execSync(`npx wrangler pages deploy ${outputPath}`, { | ||
| stdio: 'inherit', | ||
| cwd: currentDir, | ||
| }) | ||
|
|
||
| console.log('Successfully deployed to Cloudflare Pages!') | ||
| } | ||
| // Deploy using Wrangler | ||
| const { execSync } = await import('child_process') | ||
|
|
||
| execSync(`wrangler pages deploy`, { | ||
| stdio: 'inherit', | ||
| cwd: currentDir, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for wrangler.toml files and examine the deploy function in detail
echo "=== Finding wrangler.toml files ==="
fd -t f "wrangler.toml"
echo -e "\n=== Checking cli/cli.ts deploy function context ==="
# Read the deploy function in full (lines 155-181 as mentioned in comment)
if [ -f "cli/cli.ts" ]; then
sed -n '155,181p' cli/cli.ts
else
echo "cli/cli.ts not found, searching for it"
fd -t f "cli.ts"
fiRepository: patternfly/patternfly-doc-core
Length of output: 761
🏁 Script executed:
echo "=== Searching for currentDir definition in cli/cli.ts ==="
rg "currentDir" cli/cli.ts -B 2 -A 2
echo -e "\n=== Checking full deploy function and surrounding context ==="
wc -l cli/cli.ts
echo -e "\n=== Search for where deploy() is called ==="
rg "deploy\(" cli/cli.ts -B 3 -A 3
echo -e "\n=== Check if outputDir is used in deploy or related functions ==="
rg "outputDir" cli/cli.ts -B 1 -A 1Repository: patternfly/patternfly-doc-core
Length of output: 1997
🏁 Script executed:
echo "=== Search for README or deployment documentation ==="
fd -t f -i "readme" | head -5
echo -e "\n=== Check for any comments/docs around deploy function ==="
sed -n '145,180p' cli/cli.ts
echo -e "\n=== Search for test files related to deploy ==="
fd -t f "test\|spec" | grep -i deploy
echo -e "\n=== Check if there are any example configs or docs ==="
fd -t f "example\|sample\|template" | head -10
echo -e "\n=== Search for wrangler usage in comments or docs ==="
rg "wrangler" --type md --type txtRepository: patternfly/patternfly-doc-core
Length of output: 1180
Pass the build output directory explicitly to Wrangler.
The deployment command executes wrangler pages deploy without specifying which directory to deploy. Since docs are built to absoluteOutputDir (derived from config.outputDir), but this path is not passed to the deploy function, the deploy command needs to be updated to:
wrangler pages deploy ${absoluteOutputDir}
Or the deploy() function should receive absoluteOutputDir as a parameter and pass it to the wrangler command. Without this, Wrangler may deploy from the wrong directory or fail if the consumer's wrangler.toml is not correctly configured.
🤖 Prompt for AI Agents
In cli/cli.ts around lines 167-174, the wrangler deploy call does not specify
the build output directory; update the execSync invocation to pass the resolved
absoluteOutputDir to Wrangler (e.g., run "wrangler pages deploy
<absoluteOutputDir>") or change the surrounding deploy() function to accept
absoluteOutputDir and forward it to execSync; ensure you use the
already-imported execSync, pass cwd appropriately or quote the path to handle
spaces, and validate absoluteOutputDir is resolved before calling execSync.
There was a problem hiding this comment.
This is intended, we want the deploy dir to come from the wrangler config file.
There was a problem hiding this comment.
@wise-king-sullyman, understood! Using the wrangler.toml configuration for the deployment directory is a valid approach and keeps the deployment configuration centralized. Thank you for clarifying the intent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/cli.ts (1)
110-112: Consider re-throwing or exiting on initialization failure.The catch block logs the error but doesn't propagate it. If the template copy fails during initialization (e.g., missing template file, permission issues), the build will continue without the API index, potentially causing runtime errors later. This differs from the error handling in
buildProject(line 154) which re-throws.} catch (e: any) { console.error('Error copying apiIndex.json template:', e) + throw e }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdx(1 hunks)cli/cli.ts(7 hunks)src/utils/apiIndex/fetch.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/apiIndex/fetch.ts
- README.mdx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (22.x)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
cli/cli.ts (7)
37-37: LGTM!Using
console.logfor this informational message about fallback behavior is appropriate since it's not an error condition.
143-155: LGTM!The error handling for the
apiIndex.jsoncopy operation is properly implemented with try/catch, error logging, and re-throwing to fail the build on error. This addresses the previously identified concern.
167-170: Nice addition of dry-run mode.The
--dry-runflag implementation is clean and allows users to test the deployment flow without actually deploying. This is helpful for CI/CD testing scenarios.
191-193: LGTM!The CLI options are well-defined with appropriate defaults. The
--dry-runflag follows standard CLI conventions.
214-223: LGTM!The
startcommand properly initializes the API index with the program context, ensuring verbose logging works consistently across all commands.
225-227: LGTM!The
buildcommand properly delegates tobuildProjectwith the program context.
250-252: Verify: Deploy command doesn't trigger a build.The
deploycommand only runs the deployment without first building the project. Users must runbuildseparately beforedeploy. If this is intentional for flexibility, consider adding a check or warning if thedocsdirectory doesn't exist to provide clearer feedback.
|
🎉 This PR is included in version 1.15.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #184
Assisted by claude code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.