Normalize paths in bundle-ui-step same-path guard#7388
Merged
Conversation
The same-path guard at bundle-ui-step.ts compared `localOutputDir` and `bundleOutputDir` as raw strings. In practice these two values are computed along different code paths (`dirname(buildUIExtension())` vs `dirname(extension.outputPath)` or `joinPath(..., bundleFolder)`) and can resolve to the same filesystem directory while differing as strings — e.g. when one path has a `.` segment, a trailing slash, or otherwise non-canonical shape. When that happens, the guard slips through and fs-extra rejects the copy with "Source and destination must not be the same". Normalize both sides via `resolvePath` before comparing so the guard catches any string variant that maps to the same directory. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an intermittent shopify app build failure for UI extensions where the “same-path” copy guard in the bundle UI build step could miss equivalent paths that differ only by non-canonical string shape (e.g., . segments), causing fs-extra to throw “Source and destination must not be the same.”
Changes:
- Normalize
localOutputDirandbundleOutputDirviaresolvePathbefore comparing, preventing same-path copy attempts. - Add a changeset documenting the patch release and the failure mode addressed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/app/src/cli/services/build/steps/bundle-ui-step.ts | Normalizes both directories before the same-path guard comparison to avoid same-path copy errors. |
| .changeset/fix-bundle-ui-step-path-normalization.md | Declares a patch release for @shopify/app and documents the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
69fa7e0 to
d5b9fab
Compare
Extends the same-path guard coverage with a case where localOutputDir and bundleOutputDir are string-distinct but resolve to the same directory (`/test/extension/dist` vs `/test/./extension/dist`). This is the actual shape that triggered the original fs-extra "Source and destination must not be the same" failure in the field — the existing identical-strings test did not catch it because both the old and new guard handle the trivial case. Verified that this test fails against the pre-fix guard (`copyFile` is called with distinct strings) and passes once the guard normalizes both sides with `resolvePath`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d5b9fab to
35deef9
Compare
gonzaloriestra
approved these changes
Apr 24, 2026
isaacroldan
approved these changes
Apr 24, 2026
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.
Resolves https://github.com/shop/issues-retail/issues/28335
Companion ui-extension PR
Summary
shopify app buildintermittently fails on UI extensions with:The same-path guard in bundle-ui-step.ts compared
localOutputDirandbundleOutputDiras raw strings. These are computed along different code paths:localOutputDir = dirname(buildUIExtension(...))— where esbuild writesbundleOutputDir = dirname(extension.outputPath)orjoinPath(dirname(extension.outputPath), bundleFolder)— copy destinationWhen both resolve to the same directory on disk but differ as strings (a
.segment, a trailing slash, a path that went through a differentjoinPathpath and came out non-canonical), the!==check slips through and fs-extra rejects the copy.Fix
Normalize both sides with
resolvePathbefore comparing:Two-line change. Same code path that the existing test at bundle-ui-step.test.ts:38-47 was already asserting skips the copy — the guard just wasn't robust to non-canonical path shapes.
Scope
copyFileis only invoked in the "different directories" case (unchanged).resolvePathis idempotent on canonical paths.realpathSync).How to reproduce on main
Any local setup where
extension.outputPathandbuildUIExtension's return value compute to the same directory via different string shapes will hit this. It surfaced for me while runningpnpm shopify app build --path <sibling-app>against a test extension that links@shopify/ui-extensionsvia apnpm.overrideslink:path — the esbuild output path came back in a different normalization from the extension output path, and the existing guard missed it.Test plan
bundle-ui-step.test.tsstill passes (unit test mocksfs.copyFile; the behavior asserted — "skips the copy when local and bundle output directories are identical" — is preserved and now more robust).shopify app buildon an affected extension completes end-to-end (previously failed at the bundle step).resolvePath-equal would now skip the copy. Confirm no intended code path relied on that (I could not find one — the branch is explicitly guarding against fs-extra's same-path rejection, and every caller I traced expects a no-op when the dirs collapse).Rollback
Revert the two-line change. No migration required.