Conversation
- Use wrangler.dev.jsonc for Vite dev/Vitest; keep wrangler.jsonc for build/preview - Gitignore dev/deploy wrangler files; remove tracked wrangler.jsonc (use example) - Default local D1 migrations to wrangler.dev.jsonc when present - Add generate-wrangler-dashboard-config.mjs and production GHA deploy
Contributor
📝 WalkthroughWalkthroughImplements production deployment infrastructure for the dashboard worker by introducing a GitHub Actions workflow that generates deployment-specific Wrangler configurations, applies D1 migrations, and deploys to Cloudflare. Adds configuration-aware utilities for conditional Wrangler config path resolution and supporting scripts for environment variable validation and secret injection during CI builds. Changes
Sequence DiagramsequenceDiagram
actor GitHub as GitHub Actions
participant Workflow as Dashboard Deploy Workflow
participant ConfigGen as Config Generator
participant Wrangler as Wrangler CLI
participant D1 as D1 Database
participant Cloudflare as Cloudflare Workers
GitHub->>Workflow: Trigger on main push
Workflow->>ConfigGen: node scripts/generate-wrangler-dashboard-config.mjs
ConfigGen->>ConfigGen: Read secrets/vars from environment
ConfigGen->>ConfigGen: Validate required values present
ConfigGen->>Workflow: Write wrangler.deploy.json
Workflow->>Wrangler: node scripts/run-d1-migrations.mjs<br/>DB --remote
Wrangler->>D1: Apply migrations with -c wrangler.deploy.json
D1->>Wrangler: Migrations applied
Workflow->>Workflow: Copy wrangler.deploy.json to wrangler.jsonc
Workflow->>Workflow: pnpm run build
Workflow->>Wrangler: pnpm run deploy:ci<br/>(wrangler deploy --config wrangler.deploy.json)
Wrangler->>Cloudflare: Deploy worker
Cloudflare->>GitHub: Deployment complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/dashboard/vite.config.ts (1)
21-36: Consider centralizing Wrangler config discovery to avoid logic drift.The precedence logic at Line 29–34 is now duplicated with
scripts/run-d1-migrations.mjs(Line 29–35). Extracting this into a shared helper would reduce future divergence risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/vite.config.ts` around lines 21 - 36, The precedence logic in resolveCloudflareWranglerConfigPath is duplicated in scripts/run-d1-migrations.mjs; extract this logic into a single shared helper (e.g., export a function like getWranglerConfigPath or resolveWranglerConfigPath from a new util module) and replace the inline implementations in both resolveCloudflareWranglerConfigPath (in vite.config.ts) and the script in run-d1-migrations.mjs with imports that call the shared helper, ensuring the helper accepts (command, mode) and uses the same wranglerDevPath/wranglerMainPath checks so both callers share identical behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/dashboard/vite.config.ts`:
- Around line 21-36: The precedence logic in resolveCloudflareWranglerConfigPath
is duplicated in scripts/run-d1-migrations.mjs; extract this logic into a single
shared helper (e.g., export a function like getWranglerConfigPath or
resolveWranglerConfigPath from a new util module) and replace the inline
implementations in both resolveCloudflareWranglerConfigPath (in vite.config.ts)
and the script in run-d1-migrations.mjs with imports that call the shared
helper, ensuring the helper accepts (command, mode) and uses the same
wranglerDevPath/wranglerMainPath checks so both callers share identical
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 425761e4-1e35-4017-a31a-408bb6576fa4
📒 Files selected for processing (9)
.github/workflows/dashboard-deploy-production.ymlapps/dashboard/.gitignoreapps/dashboard/README.mdapps/dashboard/package.jsonapps/dashboard/vite.config.tsapps/dashboard/wrangler.jsoncapps/dashboard/wrangler.jsonc.examplescripts/generate-wrangler-dashboard-config.mjsscripts/run-d1-migrations.mjs
💤 Files with no reviewable changes (1)
- apps/dashboard/wrangler.jsonc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/resolve-wrangler-config-path.mjs (1)
14-21: Prefer returning the resolved path to avoid cwd-coupling.You check existence using
rootDir, but return a relative filename. Returning the resolved match keeps behavior stable even when caller cwd differs.♻️ Proposed refactor
/** * Same resolution Vite uses for the Cloudflare plugin in dev / Vitest (`serve` + non-production). * * `@param` {{ command: string; mode: string; rootDir: string }} opts - * `@returns` {string | undefined} Relative filename for `wrangler -c`, or undefined for default discovery. + * `@returns` {string | undefined} Resolved config path, or undefined for default discovery. */ export function resolveWranglerConfigPath({ command, mode, rootDir }) { if (command !== "serve" || mode === "production") { return undefined; } const dev = path.join(rootDir, "wrangler.dev.jsonc"); const main = path.join(rootDir, "wrangler.jsonc"); if (existsSync(dev)) { - return "wrangler.dev.jsonc"; + return dev; } if (existsSync(main)) { - return "wrangler.jsonc"; + return main; } return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/resolve-wrangler-config-path.mjs` around lines 14 - 21, The function currently checks for existence with the resolved paths stored in dev and main but returns bare filenames, causing cwd coupling; change the returns to return the resolved path variables (dev or main) instead of "wrangler.dev.jsonc"/"wrangler.jsonc" so callers always get absolute/resolved paths; update the branches that call existsSync(dev) and existsSync(main) to return dev and main respectively and ensure any tests or callers expect a resolved path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/run-d1-migrations.mjs`:
- Around line 29-33: The code calls resolveWranglerConfigPath(...) with rootDir:
process.cwd(), which is fragile because it assumes the script is invoked from
apps/dashboard; change the rootDir passed to resolveWranglerConfigPath to a
deterministic path pointing at the dashboard app (e.g., hardcode or compute a
repo-root-relative path to "apps/dashboard" instead of process.cwd()) and add a
brief inline comment referencing wrangler.dev.jsonc to document the assumption;
update the wranglerConfig assignment (resolveWranglerConfigPath) so it reliably
locates wrangler.dev.jsonc even when the script is run from other working
directories.
---
Nitpick comments:
In `@scripts/resolve-wrangler-config-path.mjs`:
- Around line 14-21: The function currently checks for existence with the
resolved paths stored in dev and main but returns bare filenames, causing cwd
coupling; change the returns to return the resolved path variables (dev or main)
instead of "wrangler.dev.jsonc"/"wrangler.jsonc" so callers always get
absolute/resolved paths; update the branches that call existsSync(dev) and
existsSync(main) to return dev and main respectively and ensure any tests or
callers expect a resolved path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 20710629-1d22-46f2-92a9-cbc8d3ef2f59
📒 Files selected for processing (3)
apps/dashboard/vite.config.tsscripts/resolve-wrangler-config-path.mjsscripts/run-d1-migrations.mjs
Comment on lines
+29
to
+33
| wranglerConfig = resolveWranglerConfigPath({ | ||
| command: "serve", | ||
| mode: "development", | ||
| rootDir: process.cwd(), | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how scripts/run-d1-migrations.mjs is invoked and which working directory is used.
# Expected: invocations either run from apps/dashboard or explicitly set working-directory.
rg -n -C3 'run-d1-migrations\.mjs|working-directory:|WRANGLER_CONFIG|deploy:ci|--local' --iglob '*.yml' --iglob '*.yaml' --iglob 'package.json'Repository: stylessh/diffkit
Length of output: 810
🏁 Script executed:
#!/bin/bash
# Read the full context of the script around the flagged lines
cat -n scripts/run-d1-migrations.mjs | head -50Repository: stylessh/diffkit
Length of output: 1781
🏁 Script executed:
#!/bin/bash
# Search for resolveWranglerConfigPath definition and usage
rg -n 'resolveWranglerConfigPath' --type=javascript --type=typescript -B5 -A5Repository: stylessh/diffkit
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Search for the implementation of resolveWranglerConfigPath
cat -n scripts/resolve-wrangler-config-path.mjs | head -80Repository: stylessh/diffkit
Length of output: 977
🏁 Script executed:
#!/bin/bash
# Search for all invocations of run-d1-migrations.mjs
rg 'run-d1-migrations' -n --no-headingRepository: stylessh/diffkit
Length of output: 513
cwd-coupling in config auto-detection is currently safe but fragile.
Line 32 uses process.cwd() to locate wrangler.dev.jsonc, which assumes the script runs from apps/dashboard. All current invocations (npm scripts in apps/dashboard/package.json and the explicit cd in paseo.json) satisfy this, but if the script is ever run from a different directory, the config won't be found. This isn't silent—Wrangler will fall back to its own discovery—but the assumption should be documented or the rootDir should be hardcoded to apps/dashboard for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/run-d1-migrations.mjs` around lines 29 - 33, The code calls
resolveWranglerConfigPath(...) with rootDir: process.cwd(), which is fragile
because it assumes the script is invoked from apps/dashboard; change the rootDir
passed to resolveWranglerConfigPath to a deterministic path pointing at the
dashboard app (e.g., hardcode or compute a repo-root-relative path to
"apps/dashboard" instead of process.cwd()) and add a brief inline comment
referencing wrangler.dev.jsonc to document the assumption; update the
wranglerConfig assignment (resolveWranglerConfigPath) so it reliably locates
wrangler.dev.jsonc even when the script is run from other working directories.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/run-d1-migrations.mjs (1)
29-33:⚠️ Potential issue | 🟡 MinorMake Wrangler root resolution deterministic (still cwd-coupled).
Line 32 still binds config auto-detection to invocation directory (
process.cwd()), so local auto-pick can fail when the script is run outsideapps/dashboard.💡 Proposed fix
import { spawnSync } from "node:child_process"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; import { resolveWranglerConfigPath } from "./resolve-wrangler-config-path.mts"; import { getSharedWranglerStatePath, isWorktreeCheckout, } from "./shared-worktree-paths.mjs"; const PNPM_SCRIPT_EXT = /\.(c|m)?js$/u; +const scriptDir = path.dirname(fileURLToPath(import.meta.url)); +const dashboardRoot = path.resolve(scriptDir, "../apps/dashboard"); @@ if (!wranglerConfig && mode === "--local") { wranglerConfig = resolveWranglerConfigPath({ command: "serve", mode: "development", - rootDir: process.cwd(), + rootDir: dashboardRoot, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run-d1-migrations.mjs` around lines 29 - 33, The wrangler config root is set using process.cwd() which makes resolution depend on the caller; change the rootDir passed to resolveWranglerConfigPath so it is derived from the script's own location instead of process.cwd() (compute the script directory using fileURLToPath(import.meta.url) and path.dirname, then resolve the desired project subpath from there) and assign that to wranglerConfig; update any imports to include fileURLToPath from 'url' and ensure path.resolve is used when constructing the rootDir for resolveWranglerConfigPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/run-d1-migrations.mjs`:
- Around line 29-33: The wrangler config root is set using process.cwd() which
makes resolution depend on the caller; change the rootDir passed to
resolveWranglerConfigPath so it is derived from the script's own location
instead of process.cwd() (compute the script directory using
fileURLToPath(import.meta.url) and path.dirname, then resolve the desired
project subpath from there) and assign that to wranglerConfig; update any
imports to include fileURLToPath from 'url' and ensure path.resolve is used when
constructing the rootDir for resolveWranglerConfigPath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 220895e4-c7f5-4035-ad1c-e5ae2567967c
📒 Files selected for processing (8)
.github/workflows/dashboard-deploy-production.yml.lintstagedrc.jsonapps/dashboard/package.jsonapps/dashboard/tsconfig.jsonapps/dashboard/vite.config.tsscripts/link-worktree-dev-vars.mjsscripts/resolve-wrangler-config-path.mtsscripts/run-d1-migrations.mjs
✅ Files skipped from review due to trivial changes (2)
- .lintstagedrc.json
- apps/dashboard/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/dashboard/package.json
- .github/workflows/dashboard-deploy-production.yml
- apps/dashboard/vite.config.ts
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
1 participant
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
wrangler.dev.jsoncwhen present (fallback towrangler.jsonc); productionvite build/vite previewstill use the default Wrangler config path.wrangler.jsonc; expandwrangler.jsonc.exampleand README for copying towrangler.dev.jsonc(gitignored).wrangler.dev.jsoncwhenWRANGLER_CONFIGis unset.dashboard-deploy-production.yml(main + path filters) — generatewrangler.deploy.jsonfrom secrets, apply remote D1 migrations,deploy:ci.generate-wrangler-dashboard-config.mjs;deploy:ciin dashboardpackage.json.GitHub setup
Repository secrets (not Environment unless you add
environment:to the job):CLOUDFLARE_API_TOKEN,CLOUDFLARE_ACCOUNT_ID,DASHBOARD_PROD_*bindings. Optional variable:WRANGLER_COMPATIBILITY_DATE.Summary by CodeRabbit
New Features
Documentation
Chores