Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
1efd2f7 to
0c746ca
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens rewriteConfiguration so it can safely reorder keys even when it receives nullish or type-mismatched config data (e.g., raw TOML fallback from third-party templates), preventing crashes like config2.map is not a function during app config writes.
Changes:
- Add early null/undefined guards in
rewriteConfiguration, plusArray.isArrayandtypeof === 'object'checks before array/object traversal. - Add unit tests covering null/undefined and type-mismatched values (including a case using the real
buildVersionedAppSchema()). - Add a patch changeset documenting the crash fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/app/src/cli/services/app/write-app-configuration-file.ts | Adds defensive guards in rewriteConfiguration to avoid .map()/Object.entries() on invalid shapes. |
| packages/app/src/cli/services/app/write-app-configuration-file.test.ts | Adds focused tests to ensure rewriteConfiguration doesn’t throw on nullish/type-mismatched config. |
| .changeset/chilly-glasses-fetch.md | Records a patch release note for the crash fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…sions The Zod schema walk in rewriteConfiguration crashes when writeAppConfigurationFile receives unvalidated data from loadOpaqueApp's raw TOML fallback (third-party templates without client_id). Instead of adding defensive guards to the schema walk, stop using it in the write path entirely. - Replace rewriteConfiguration with stripEmptyObjects in writeAppConfigurationFile - Add structuredClone to prevent mutating caller's config object - Add Array.isArray guard in condenseComplianceAndNonComplianceWebhooks - Keep rewriteConfiguration exported for breakdown-extensions.ts (unchanged) - Drop schema parameter from writeAppConfigurationFile signature Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
a304a30 to
59362c7
Compare
TOML section ordering changed with new write path, test expectation needed updating Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfonso-noriega
left a comment
There was a problem hiding this comment.
Minor comment that needs addressing, everything else looks great!
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@shopify/app': patch | |||
There was a problem hiding this comment.
Normally we do not add changesets for non user facing changes, but if it is meant to be trigger a patch, we are only generating changesets on the PR against the stable branch to force the new version generation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Integrate new selectConfigName assertions and default-filename test from main while keeping snapshot-based TOML assertions from this branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Summary
Fixes shop/issues#8553 —
config2.map is not a functionVault issue: https://vault.shopify.io/issues/8435
Stops using
rewriteConfigurationinwriteAppConfigurationFileand replaces it with a simplestripEmptyObjectsutility. The Zod schema walk was the root cause of the crash.rewriteConfigurationis kept exported for its other consumer (breakdown-extensions.ts), which is untouched in this PR.Root Cause
When
loadOpaqueApp(#6751) can't Zod-validate a local TOML config (e.g., a third-party template withoutclient_id), it falls back to raw, unvalidated TOML data. This raw data is deep-merged with the remote config and passed towriteAppConfigurationFile, whererewriteConfigurationwalked the Zod schema assuming config values matched schema types — calling.map()on non-arrays andObject.entries()on non-objects.Triggered by
app dev --reset(or any link flow) on apps using third-party templates.Why remove instead of guard
client_idmakes the app appear "unlinked")rewriteConfiguration's only meaningful behaviors were key reordering (cosmetic) and empty-object stripping — only the latter is neededTomlFile.patch()for surgical edits that preserve comments/formattingChanges
write-app-configuration-file.ts: Stop callingrewriteConfigurationinwriteAppConfigurationFile. AddstripEmptyObjectsutility. AddstructuredCloneto prevent mutating the caller's config (previously masked byrewriteConfigurationcreating fresh objects). AddArray.isArrayguard incondenseComplianceAndNonComplianceWebhooks. KeeprewriteConfigurationexported forbreakdown-extensions.ts.link.ts: RemoveschemafromwriteAppConfigurationFilecall andgetAppVersionedSchemaimport.stripEmptyObjectstests. Add type-mismatched data crash test. Convertlink.test.tsTOML comparisons to snapshots. Update round-trip stability tests to 3-write pattern. Update 2configurationobject expectations to reflect that write-time mutations (webhook condensation, URI relativization) no longer leak back to the caller.Behavior change
Key ordering in the generated TOML now follows JS object insertion order rather than schema declaration order. This may cause a one-time key reorder in
shopify.app.tomlon the nextapp linkorapp config pull. Subsequent writes are stable.Not changed
breakdown-extensions.ts— still usesrewriteConfigurationfor config diffing. Its inputs come from the platform API and Zod-parsed config (not the raw TOML fallback), so it does not have the same crash risk.Test plan
stripEmptyObjectsunit testswriteAppConfigurationFiletests (comments, empty entries, empty arrays, type-mismatched data)shopify app initwith a template lackingclient_id, thenshopify app dev --reset— should not crash🤖 Generated with Claude Code