chore: promote src/nimbus to src#32249
Merged
Merged
Conversation
Contributor
|
This pull request requires reviews from CODEOWNERS as it changes files that match the following patterns:
|
Contributor
|
Preview URL: https://edbd082b.preview.developers.cloudflare.com |
mvvmm
marked this pull request as ready for review
July 22, 2026 17:37
mvvmm
requested review from
a team,
kodster28,
ravindra-cloudflare and
roerohan
as code owners
July 22, 2026 17:37
Contributor
Review
👉 Fix in your agent 👈Fix the following review findings in PR #32249 (https://github.com/cloudflare/cloudflare-docs/pull/32249).
Before making changes, review each finding and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g. the fix requires disproportionate effort for minimal benefit,
or the finding is factually incorrect), explain why
- If you need clarification before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
After triaging, follow this order:
1. Post a comment on this PR for any findings you are skipping, with the finding ID and your reasoning.
2. Then commit the fixes for the legitimate findings.
The comment must come before the commit — the bot reads PR comments when a new
push triggers a review, so skip comments posted after the push will be missed.
---
## Code Review
### Warnings (8)
#### CR-7ba85bcce2bf · Unchecked external input
- **File:** `src/content/collections/mcp-servers.ts` line 17
- **Issue:** The parser type-asserts `JSON.parse(fileContent) as { servers: McpServer[] }` and then iterates `data.servers` and calls `slug(server.name)` without verifying the manifest shape or that each entry has a usable `name`. A malformed middlecache response will produce a cryptic TypeError instead of a clear validation failure.
- **Fix:** Validate the parsed JSON before using it: check that `data.servers` is an array and that each server has a string `name`, or run the data through the Zod schema inside the parser so failures have actionable messages.
#### CR-47f44786835b · Unchecked external input
- **File:** `src/content/collections/skills-manifest.ts` line 18
- **Issue:** The parser casts middlecache JSON to `{ skills: SkillManifestEntry[] }` without validating that `skills` exists, so a malformed response or missing key throws a generic `TypeError: data.skills is not iterable`.
- **Fix:** Validate the parsed JSON shape (e.g. with Zod) or check `Array.isArray(data.skills)` before iterating.
#### CR-64aae7ca96b1 · Duplicate IDs silently overwritten
- **File:** `src/content/collections/skills-manifest.ts` line 25
- **Issue:** Building the lookup as `lookup[skill.name]` overwrites any earlier entry with the same skill name, silently dropping data if the manifest ever contains duplicates.
- **Fix:** Detect duplicate skill names and throw a descriptive error, similar to the duplicate-slug guard in `mcp-servers.ts`.
#### CR-6b335f745e5e · Unsafe type assertion on unknown field
- **File:** `src/components/cf/ResourcesBySelector.astro` line 69
- **Issue:** data.pcx_content_type is typed as unknown (passthrough), but it is cast directly to string | undefined. If the value is null or a non-string, the filter behaves incorrectly: null yields type null instead of falling back to collection, and non-strings silently fail types.includes.
- **Fix:** Narrow the value with typeof data.pcx_content_type === "string" before using it, and fall back to collection for null or non-string values.
#### CR-e44bfb83d9ee · Missing null guard before object access
- **File:** `src/components/cf/ResourcesBySelector.astro` line 76
- **Issue:** typeof v === "object" is true for null, so a null product item causes v.id to throw Cannot read properties of null (reading 'id').
- **Fix:** Check v !== null before treating v as an object, e.g., typeof v === "object" && v !== null ? v.id : v.
#### CR-59097b881c30 · Sort order mismatches displayed label
- **File:** `src/components/changelog/Header.astro` line 43
- **Issue:** The product sort on line 43 falls back to `""` when `entry.title` is missing, but line 119 displays `product.id` as the fallback. A product with no title would sort at the top of the list while displaying its ID, so the list order would no longer match the alphabetical order of the visible labels.
- **Fix:** Sort by the same displayed value, e.g. `(a.data.entry?.title ?? a.id).localeCompare(b.data.entry?.title ?? b.id)`, so the combobox order matches the rendered labels.
#### CR-447208b47e3d · In-place mutation of collection data
- **File:** `src/pages/pages/platform/language-support-and-tools.json.ts` line 9
- **Issue:** The code assigns `x.data.enable_date = new Date(...)` directly on the object returned by `getCollection`, mutating the collection entry in place. Depending on Astro's collection caching, this side effect can leak transformed data across requests or callers.
- **Fix:** Build a new object instead of mutating `x.data`, for example `return { ...x.data, enable_date: new Date(...).toISOString(), status: x.data.status ?? null };`.
#### CR-c60a5d4cbca7 · Unsafe cast of unknown value
- **File:** `src/pages/pages/platform/language-support-and-tools.json.ts` line 10
- **Issue:** `x.data.enable_date` is typed as `unknown` and cast to `string | number | Date` without runtime validation. If the YAML value is missing, null, a malformed string, or another type, `new Date(...).toISOString()` will throw a `RangeError` at runtime.
- **Fix:** Validate the runtime value before constructing the Date, handle missing/invalid dates explicitly, or narrow the type with a type guard.
Code ReviewThis code review is in beta and may not always be helpful — use your judgment. Warnings (8)
ConventionsNo convention issues found. Style Guide ReviewNo style-guide issues found. CommandsOnly codeowners can run commands. Post a comment with the command to trigger it.
|
kodster28
approved these changes
Jul 22, 2026
mvvmm
force-pushed
the
chore/promote-nimbus-to-src
branch
from
July 22, 2026 18:17
47e1d14 to
98efed9
Compare
Nimbus is the only build target now — the srcDir/cacheDir split that
existed to run Starlight and Nimbus side by side no longer serves a
purpose. This merges src/nimbus/* into src/, matching Astro's default
srcDir and eliminating the cross-graph aliasing that split required.
- Merge src/nimbus/{components,scripts,util,plugins,layouts,pages,
schemas,styles,lib,content.config.ts,mdx-components.ts,components.ts,
astro-config.ts} into src/ (git mv, no name collisions)
- astro.config.ts: drop the srcDir/cacheDir override (Astro defaults)
- src/astro-config.ts: delete the ~120-line aliasResolver + resolveId
fallback Vite plugin entirely — with one tree, ~ and @ resolve to
src/* via the standard tsconfig paths, no override needed
- tsconfig.json: single config (~/* and @/* -> src/*), no more
src/nimbus exclude or separate src/nimbus/tsconfig.json
- Fix 6 relative imports whose depth changed with the move
(PageHead.astro, llms.txt.ts x2, 404.astro, docsearch-config.ts,
webmcp.ts, plugins/docsearch/index.ts)
- Fix 5 broken src/components.ts barrel paths (../components/AiSearch*
-> ./components/AiSearch*)
Removing the srcDir override also removed the root tsconfig's
'exclude: ["src/nimbus"]', which had been silently hiding the real
app from 'astro check' since the migration (check was only exercising
~40 leftover top-level files, not the ~400 files under src/nimbus).
Fixing the app for real type-checking surfaced two categories of
pre-existing issues, both fixed here:
- Astro 7 / @cloudflare/nimbus-docs 0.2.2->0.6.1 API changes that
landed in #32240 without anyone being able to see them break:
'incrementalBuilds' and 'partialResolver' were removed from
NimbusIntegrationOptions (partialResolver was already dead — Render
.astro resolves partials itself via getEntry); shiki's CodeLanguage
narrowed (lang="curl" -> lang="bash", a real highlighting
improvement, not just a cast); GetRSSItemsOptions dropped an
already-inert 'markdown' flag; [...slug].astro used two content
frontmatter fields ('canonical', 'styleGuide') never declared in
content.config.ts's schemaFields.
- ~50 pre-existing null-safety gaps on the permissive 'directory'
collection schema (data.entry / data.name are .optional()), mostly
'!' assertions or '?.' at existing, already-safe access sites (e.g.
mobile-sidebar.client.ts's hoisted function declarations couldn't
see the outer null-guard's narrowing).
Also found and fixed two independent bugs while unblocking 'check':
- SkillsList.astro called getCollection("cloudflare-skills-manifest"),
which doesn't exist — the declared collection is "skills". Fixed to
the correct name.
- McpServerList.astro calls getCollection("cloudflare-mcps-manifest"),
which also doesn't exist and has no declared collection anywhere.
Astro's getCollection warns + returns [] for unknown collections
rather than throwing, so this has been silently rendering an empty
MCP server list on 6 live agent-setup pages. Left a
'@ts-expect-error' + FIXME rather than guessing at a fix — this
needs a real collection/loader, which is out of scope here.
Verified locally: pnpm run check/lint/format:check/test all clean,
full 'pnpm run build' succeeds (8707 pages), test:postbuild passes.
McpServerList.astro called getCollection("cloudflare-mcps-manifest"),
which never existed as a declared collection anywhere — it silently
rendered an empty list on all 6 agent-setup pages (getCollection warns
+ returns [] for unknown collections rather than throwing).
There's already a reusable pattern for exactly this (middlecacheLoader
in src/util/custom-loaders.ts, used by product-availability and
granular-control-applications) — I'd missed it when I first found this
gap. The real middlecache key is v1/cloudflare-mcps/mcps-manifest.json:
{ name, description, servers: [{ name, description, url }, ...] },
matching the component's existing usage exactly.
Adds a mcp-servers collection (src/content/collections/mcp-servers.ts)
using middlecacheLoader with a parser that flattens the servers array
into an id-keyed lookup (slugified name), same shape as the existing
product-availability/granular-control-applications loaders. Updates
McpServerList.astro to read from it and removes the @ts-expect-error/
FIXME workaround.
Verified: pnpm run check clean, full build succeeds, and the rendered
agent-setup pages now show the real server list (previously empty).
… skills collection
My earlier fix for SkillsList.astro (which called
getCollection("cloudflare-skills-manifest") — a collection that never
existed) repointed it at the existing "skills" collection instead.
That was the wrong fix: "skills" is populated by astro-skills's
skillsLoader reading the full SKILL.md bundle tarball, which exists to
serve the /.well-known/skills/ discovery routes — a different purpose
with a heavier loading mechanism than a simple name+description list.
bin/fetch-skills.ts already downloads a lightweight, purpose-built
manifest for exactly this display case
(v1/cloudflare-skills/skills-manifest.json: { skills: [{ name,
description, files }] }) to .tmp/middlecache/, but nothing ever reads
it afterward — same class of bug as the MCP servers manifest.
Adds a skills-manifest collection (mirroring mcp-servers.ts) using the
same middlecacheLoader pattern, fetching the manifest directly rather
than reusing the local skills/ bundle. Points SkillsList.astro at it.
Verified against a fresh (non-stale-cached) middlecache fetch: the
build now renders all 11 skills, not the 8 that happened to be present
in the local skills/ bundle when I ran the original (wrong) fix.
Fixes 10 findings from PR review, all pre-existing null-safety gaps
introduced when adding !/?? handling for the permissive directory
schema in the earlier check-fixing commit:
- mcp-servers.ts: detect duplicate slugified server names and throw
instead of silently overwriting a colliding entry (CR-9f8cfe630555)
- Header.astro: entry!.title -> entry?.title ?? product.id, matching
the fallback already used two lines away for the sort comparator
(CR-b89feaef75fa)
- Directory.astro: name!/entry! at 5 sites (sort comparator,
description fallback, data-name attr, href, icon-letter fallback)
replaced with ?? "" / ?. plus a real fallback href (/${product.id}/)
instead of asserting non-null on fields the schema declares optional
(CR-dd0c61f74f87, CR-7e2e21a47e40, CR-c4d911cf54f4, CR-fd550522934f,
CR-3fbf0fffe23d)
- PagesBuildEnvironment.astro: filter out any version missing
build_environment (with a type predicate so the narrowing actually
holds) rather than asserting it's always present (CR-5def7f09bcad,
CR-b6e4e5021685)
- AvailableChangelogFeeds.astro: entry!.url/.title -> ?. plus fallback
to the entry's own id/path — the upstream filter only guarantees
entry.data.entry itself is defined, not its url/title fields
(CR-7fd7872dd3a9)
All 10 findings were agreed with — see PR discussion for the full
triage. Verified: pnpm run check/lint/format:check/test all clean,
full build succeeds (8707 pages, no collision-check throw), spot
checked rendered output for /directory/, /changelog/, and
/fundamentals/new-features/available-rss-feeds/ (uses
AvailableChangelogFeeds) all render correctly, test:postbuild passes.
mvvmm
force-pushed
the
chore/promote-nimbus-to-src
branch
from
July 22, 2026 18:33
98efed9 to
edbd082
Compare
1 task
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
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.
Summary
Promotes
src/nimbus→src, the structural follow-up flagged in #32229. Nimbus has been the only build target since #32181/#32203, but the app still lived atsrc/nimbuswithsrcDirpointed at it, while content/assets/util/some components stayed "shared in place" at the repo-rootsrc/. That split required a ~120-line custom Vite alias-resolver plugin to bridge the two trees. With one tree, that plugin is gone entirely.Structural changes
git mveverysrc/nimbus/*subtree intosrc/(components, layouts, pages, schemas, plugins, scripts, styles, lib,content.config.ts,mdx-components.ts,components.ts,astro-config.ts) — no name collisions with what already lived atsrc/astro.config.ts: drop thesrcDir/cacheDiroverride (back to Astro defaults forsrcDir)src/astro-config.ts: delete thealiasResolver+resolveIdfallback Vite plugin — with a single tree,~and@resolve tosrc/*via the standard tsconfigpaths, no override neededtsconfig.json(~/*and@/*→src/*); removed the separatesrc/nimbus/tsconfig.jsonand the root config'sexclude: ["src/nimbus"]../components/AiSearch*paths in thecomponents.tsbarrel that needed to become./components/AiSearch*Type-check fallout (the interesting part)
Removing the
src/nimbusexclude meantpnpm run checktype-checked the real app for the first time since the migration — previously it was silently scanning ~40 leftover top-level files, not the ~400 undersrc/nimbus. That surfaced 82 pre-existing errors, all fixed here:@cloudflare/nimbus-docs0.2.2→0.6.1 breakage from chore: bump to astro 7 #32240 that nobody could see break:incrementalBuildsandpartialResolverwere removed fromNimbusIntegrationOptions(partialResolverwas already dead code —Render.astroresolves partials itself viagetEntry); shiki'sCodeLanguagenarrowed (lang="curl"→lang="bash", a real highlighting improvement, not just a type cast);GetRSSItemsOptionsdropped an already-inertmarkdownflag;[...slug].astroread two frontmatter fields (canonical,styleGuide) that were never declared incontent.config.ts'sschemaFields.directorycollection schema (data.entry/data.nameare.optional()) — mostly!/?.at access sites that were already safe in practice, plus one real TS narrowing gap inmobile-sidebar.client.ts(hoistedfunctiondeclarations can't see an outer early-return guard's narrowing; fixed with a re-boundconst).Two independent bugs found and fixed along the way
Both
SkillsList.astroandMcpServerList.astrocalledgetCollection(...)with a name that was never declared anywhere.getCollectionwarns + returns[]for unknown collections rather than throwing, so both have been silently rendering empty on all 6 liveagent-setuppages.McpServerList.astrocalledgetCollection("cloudflare-mcps-manifest"). Added amcp-serverscollection (src/content/collections/mcp-servers.ts) using the existingmiddlecacheLoaderpattern (same asproduct-availability/granular-control-applications) against the real middlecache keyv1/cloudflare-mcps/mcps-manifest.json, and pointed the component at it.SkillsList.astrocalledgetCollection("cloudflare-skills-manifest"). My first attempt at this repointed it at the existingskillscollection instead — wrong fix, caught in review:skillsis populated byastro-skills's loader reading the full SKILL.md bundle tarball, which exists for the/.well-known/skills/discovery routes, a different purpose with a heavier loading mechanism.bin/fetch-skills.tsalready downloads a lightweight, purpose-built manifest for exactly this display case (v1/cloudflare-skills/skills-manifest.json) to.tmp/middlecache/, but nothing read it afterward — same bug class as the MCP one. Added a dedicatedskills-manifestcollection mirroringmcp-servers.ts, fetching that manifest directly instead.Verification
pnpm run check/lint/format:check/testall cleanpnpm run buildsucceeds locally (8707 pages)pnpm run test:postbuildpassesdist/agent-setup/claude-code/index.htmlconfirms both the MCP server list (15 servers) and skills list (all 11 skills) now show real contentDocumentation checklist