Podonnell/preview onboarding - #15598
Conversation
🦋 Changeset detectedLatest commit: 8347afb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
There was a problem hiding this comment.
Devin Review found 4 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| const baseConversion: ProposedPreviewsConfig = baseConfig | ||
| ? convertPreviewBaseToPreviewsConfig(baseConfig) | ||
| : { config: {}, omittedBindings: [] }; |
There was a problem hiding this comment.
🟡 Production bindings disappear during onboarding
When Preview Base contains any setting, baseConversion.config replaces every generated production-binding placeholder. A production binding absent from Preview Base is deployed missing after onboarding.
Learn more
Production conversion creates placeholder entries for supported production bindings so users must choose Preview-safe replacements. Preview Base conversion contains only remotely configured Preview defaults. Once Preview Base has any configured value, the current flow selects its conversion alone and never merges or checks the production proposal. The later deployment reads only the newly written previews block through extractConfigBindings, so omitted binding names are absent at runtime.
Example: Production defines KV binding SESSIONS, while Preview Base only enables observability. Onboarding writes previews.observability, deploys without SESSIONS, and the Worker fails when it accesses env.SESSIONS. Wrangler must instead require a Preview-safe KV ID.
Recommended fix: Compare production binding names against both converted and omitted Preview Base bindings. Merge non-conflicting production placeholders into the proposed config, or stop with a complete snippet that includes every production binding absent from Preview Base.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const manualBindings = productionConversion.omittedBindings.filter( | ||
| (productionBinding) => | ||
| !baseConversion.omittedBindings.some( | ||
| (baseBinding) => | ||
| baseBinding.name === productionBinding.name && | ||
| baseBinding.type === productionBinding.type | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🟡 Safe Preview Base replacements are rejected
When Preview Base replaces an unsupported production binding, manualBindings still marks it unresolved. Matching only omitted base bindings makes onboarding abort despite a deployable replacement.
Learn more
productionConversion.omittedBindings lists production bindings that cannot safely become placeholder configuration. A binding with the same name in baseConversion.config is already a Preview-safe replacement, but converted base bindings are not retained as references. The filter removes an item only when Preview Base also omitted the same name and type, which is the opposite case from a converted replacement.
Example: Production has a secret_text binding named TOKEN, and Preview Base has a plain_text binding named TOKEN. Preview Base converts TOKEN into previews.vars, but manualBindings retains the production secret and aborts onboarding. The Preview Base value could have been deployed safely.
Recommended fix: Have conversion return references for successfully converted bindings as well as omitted bindings. Treat a production omission as resolved when Preview Base supplies any compatible converted binding with that name, while preserving the existing behavior for remote-only omitted base bindings.
Was this helpful? React with 👍 or 👎 to provide feedback.
| case "ai": | ||
| if (binding.staging !== undefined) { | ||
| return null; |
There was a problem hiding this comment.
🟡 Disabled AI staging blocks onboarding
With staging: false, convertBinding treats the AI binding as unsupported because the property is defined. Preview onboarding then aborts and requests manual configuration for a normal AI binding.
| case "ai": | |
| if (binding.staging !== undefined) { | |
| return null; | |
| case "ai": | |
| if (binding.staging === true) { | |
| return null; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const format = configFormat(config.userConfigPath); | ||
| const canWriteConfig = | ||
| config.userConfigPath !== undefined && | ||
| config.userConfigPath === config.configPath && | ||
| (format === "toml" || JSON_CONFIG_FORMATS.includes(format)); |
There was a problem hiding this comment.
🟡 Commented TOML onboarding crashes
For a commented TOML file, canWriteConfig allows automatic onboarding even though the patcher rejects comments. Accepting the prompt throws PatchConfigError instead of adding or presenting the configuration.
Learn more
Automatic onboarding calls experimental_patchConfig after the user accepts. That patcher refuses any TOML source containing # because TOML round-tripping loses comments. The writability gate checks only the file format, so a common commented wrangler.toml reaches this known failure after prompting.
Example: A project has wrangler.toml containing # Worker settings and no previews table. Preview Base has one variable. Wrangler prompts to add it, the user accepts, and the command fails without deploying or showing the copyable fallback snippet.
Recommended fix: Detect commented TOML before offering automatic writeback and use the existing manual-snippet error path. Alternatively, catch PatchConfigError around writePreviewsConfig and return a UserError containing the generated snippet.
Was this helpful? React with 👍 or 👎 to provide feedback.
Fixes #[insert GH or internal issue link(s)].
Describe your change...
A picture of a cute animal (not mandatory, but encouraged)