ci/docs: deploy documentation site on Cloudflare with PR previews - #225
Conversation
e413c1b to
ef21046
Compare
Host the static documentation site on Cloudflare Workers and wire GitHub Deployments so each pull request can publish an isolated preview URL. Reviewers can open the rendered site for a change instead of inferring layout and navigation from diffs alone. This removes the GitHub Pages mindmap workflow in favor of site build and deploy workflows, adds site/wrangler.toml, operator guidance in docs/site-deployment.md, and design/spec notes under docs/superpowers/.
ef21046 to
5b24708
Compare
waynesun09
left a comment
There was a problem hiding this comment.
Overall the two-workflow pattern is well-designed — fork safety, secret isolation, and artifact flow are solid. A few items to address before merging:
Must fix:
--assets publicCLI flag onversions uploadmay overridewrangler.tomlasset config (includingnot_found_handling), creating divergence between preview and production- Verify
deployment-urloutput fromwrangler-actionforversions upload --preview-aliasreturns the alias URL, not the internal versioned URL
Should fix:
- URL parsing fallback matches
workers.devas substring — tighten to\.workers\.dev - Preview alias uses
workflow_run.idinstead of PR number — creates stale alias sprawl per_page: 100on comment listing may miss marker comment on busy PRs- Fork PR number resolution silently skips on ambiguous match — use
core.warning - Plan doc references
site-github-pages.ymlbut the deleted file ismindmap.yml
| fi | ||
| if [ -z "$url" ]; then | ||
| comb="${OUT_PROD:-}${ERR_PROD:-}${OUT_PR:-}${ERR_PR:-}" | ||
| url=$(printf '%s' "$comb" | grep -oE 'https://[a-zA-Z0-9._/?#&=%_-]+' | grep 'workers.dev' | head -1 || true) |
There was a problem hiding this comment.
Security — URL parsing matches workers.dev as substring
grep 'workers.dev' would also match deceptive domains like evil-workers.dev.attacker.com. This URL ends up as environment_url in a GitHub Deployment and rendered as a clickable link in the PR comment.
Suggest tightening to:
grep -E '\.workers\.dev(/|$)'| command: >- | ||
| versions upload | ||
| --name="${{ vars.CLOUDFLARE_PROJECT_NAME }}" | ||
| --assets public | ||
| --preview-alias pr-${{ github.event.workflow_run.id }} |
There was a problem hiding this comment.
Inconsistent asset config between preview and production paths
The preview command passes --assets public explicitly here, but the production deploy (line 48) does NOT pass --assets and relies on wrangler.toml's [assets] directory = "./public". Two concerns:
- The CLI
--assetsflag may override the entire asset config block fromwrangler.toml, meaningnot_found_handling = "single-page-application"might not apply to previews. - Behavior divergence between preview and production — what reviewers see in preview may not match what ships to production.
Consider removing --assets public and letting wrangler.toml govern both paths consistently, or verify that the CLI flag preserves the [assets] table settings.
| versions upload | ||
| --name="${{ vars.CLOUDFLARE_PROJECT_NAME }}" | ||
| --assets public | ||
| --preview-alias pr-${{ github.event.workflow_run.id }} |
There was a problem hiding this comment.
Preview alias uses workflow_run.id instead of PR number
pr-${{ github.event.workflow_run.id }} produces aliases like pr-12345678901 which are not human-readable and create a new alias on every re-run (stale alias sprawl). Using the resolved PR number instead (e.g., pr-225) would produce stable, predictable preview URLs.
This would require resolving the PR number before the wrangler step — a structural change, but worth considering since the PR number resolution logic already exists later in the workflow.
| if: >- | ||
| (steps.cf-prod.outcome == 'success' || steps.cf-preview.outcome == 'success') | ||
| env: | ||
| URL_PROD: ${{ steps.cf-prod.outputs.deployment-url }} |
There was a problem hiding this comment.
Verify deployment-url output for versions upload
wrangler-action extracts deployment-url from wrangler's structured output. For wrangler deploy this reliably returns the production URL, but for versions upload --preview-alias it's unclear whether the output contains the alias URL (e.g., pr-XXX.<worker>.workers.dev) or the internal versioned URL. If it returns the versioned URL, the preview comment will show a less stable link. The grep fallback at line 84 helps but depends on wrangler output format. Worth validating with a test run.
| owner, | ||
| repo, | ||
| issue_number: prNumber, | ||
| per_page: 100, |
There was a problem hiding this comment.
per_page: 100 may miss existing marker comment
On heavily-discussed PRs with 100+ comments (e.g., bot activity from multiple tools), the marker comment could be on a later page, causing a duplicate comment to be created. Consider paginating or documenting this as a known limitation.
| let prNumber = run.pull_requests?.[0]?.number; | ||
| if (!prNumber) { | ||
| const head = `${run.head_repository.owner.login}:${run.head_branch}`; | ||
| const { data: prs } = await github.rest.pulls.list({ | ||
| owner, | ||
| repo, | ||
| state: 'open', | ||
| head, | ||
| per_page: 100, | ||
| }); | ||
| if (prs.length !== 1) { | ||
| core.info(`Skipping PR comment: expected 1 open PR for head=${head}, found ${prs.length}`); | ||
| return; | ||
| } | ||
| prNumber = prs[0].number; |
There was a problem hiding this comment.
PR number resolution unreliable for fork PRs
workflow_run.pull_requests is documented as empty when head and base repos differ (fork PRs). The pulls.list fallback with head=owner:branch is generally correct, but silently skips the comment if the fork owner has multiple open PRs from the same branch name. Consider logging a warning (core.warning) instead of core.info so this is more visible in the Actions log.
| | `.github/workflows/site-deploy.yml` | Checkout + artifact → `site/public/`, `wrangler deploy` / `versions upload`, GitHub Deployment + PR comment | | ||
| | `site/wrangler.toml` | Worker name placeholder, `assets.directory = public`, SPA `not_found_handling`, `preview_urls` | | ||
| | `site/public/.gitkeep` | Keeps `public/` in git; CI overwrites with artifact contents | | ||
| | `.github/workflows/site-github-pages.yml` | **Removed** (replaced by `site-build.yml` / `site-deploy.yml`) | |
There was a problem hiding this comment.
Wrong filename — references site-github-pages.yml but the actual deleted file is mindmap.yml. This appears in the file map and again in Task 3 (lines 118, 122, 127).
- Resolve PR number early for stable --preview-alias pr-<n>; fall back to workflow_run.id with a warning when multiple open PRs share the same head. - Drop --assets public from versions upload so wrangler.toml governs SPA not_found_handling for preview and production. - Tighten workers.dev URL extraction to avoid deceptive hostnames. - Paginate PR comment search; gate deployment script on meta success; warn when preview URL omits the alias token (versioned URL hint). - Fix plan/spec/runbook: removed workflow was mindmap.yml; document alias behavior. Made-with: Cursor
|
Pushed Must fix
Should fix
CI |
waynesun09
left a comment
There was a problem hiding this comment.
All 7 items from my previous review are resolved in 7a395b7. Verified each fix against the current diff:
--assets publicdropped — wrangler.toml governs both paths consistently- Preview URL validation step added (warns if alias token missing, doesn't block)
- URL grep anchored to
\.workers\.dev(/|$) - PR number resolved before wrangler step — stable
pr-<number>aliases, fallback toworkflow_run.idonly on ambiguous head withcore.warning - Comment listing paginates (20×100)
- Ambiguous/missing PR uses
core.warningthroughout - Plan/spec/runbook corrected to reference
mindmap.yml
Clean restructuring, no new issues.
Summary
Moves documentation site hosting to Cloudflare Workers with GitHub Deployments so each open pull request can publish its own preview URL. Reviewers can validate rendered docs, links, and layout in a browser instead of relying only on Markdown and workflow diffs.
What changed
.github/workflows/mindmap.ymlwithsite-build.ymlandsite-deploy.yml.site/wrangler.tomlandsite/public/for static assets.docs/site-deployment.md(operator runbook) and design/spec material underdocs/superpowers/.Ran
make lintlocally before pushing, as recommended in CONTRIBUTING.md.