diff --git a/.changeset/jolly-coins-search.md b/.changeset/jolly-coins-search.md new file mode 100644 index 00000000000..c017ea4e3c4 --- /dev/null +++ b/.changeset/jolly-coins-search.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme': patch +--- + +Fix theme dev preview theme resolution diff --git a/packages/theme/src/cli/utilities/theme-environment/html.test.ts b/packages/theme/src/cli/utilities/theme-environment/html.test.ts index 88d59fd0ee8..c728b5ecb27 100644 --- a/packages/theme/src/cli/utilities/theme-environment/html.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/html.test.ts @@ -78,6 +78,29 @@ describe('getHtmlHandler', async () => { expect(ctx.lastRequestedPath).toStrictEqual('/previous-page') }) + test('renders storefront requests with the development theme id', async () => { + const handler = getHtmlHandler(theme, ctx) + const event = createH3Event('GET', '/products/test?color=blue', {'sec-fetch-mode': 'navigate'}) + + vi.mocked(render).mockResolvedValueOnce( + new Response('', { + status: 200, + headers: {'x-request-id': 'test-request-id'}, + }), + ) + + await handler(event) + + expect(render).toHaveBeenCalledWith( + ctx.session, + expect.objectContaining({ + path: '/products/test', + query: [['color', 'blue']], + themeId: '123', + }), + ) + }) + test('the development server session recovers when a theme id mismatch occurs', async () => { // Given const handler = getHtmlHandler(theme, ctx) @@ -108,6 +131,29 @@ describe('getHtmlHandler', async () => { expect(ctx.session.refresh).toHaveBeenCalled() }) + test('the mismatch retry redirect drops a stale preview_theme_id from the browser', async () => { + const handler = getHtmlHandler(theme, ctx) + const event = createH3Event('GET', '/?preview_theme_id=999&color=blue') + + vi.mocked(render).mockResolvedValueOnce( + new Response( + ``, + { + status: 200, + headers: {'x-request-id': 'test-request-id'}, + }, + ), + ) + + const response = await handler(event) + + expect(response.status).toBe(302) + expect(response.headers.get('Location')).toBe('/?color=blue') + }) + test('the development server aborts when max theme id mismatch retries is reached', async () => { // Given const handler = getHtmlHandler(theme, ctx) diff --git a/packages/theme/src/cli/utilities/theme-environment/html.ts b/packages/theme/src/cli/utilities/theme-environment/html.ts index 909b6ba3d1c..dc493cf2e95 100644 --- a/packages/theme/src/cli/utilities/theme-environment/html.ts +++ b/packages/theme/src/cli/utilities/theme-environment/html.ts @@ -101,10 +101,14 @@ export function getHtmlHandler(theme: Theme, ctx: DevServerContext): EventHandle /** * Filtering out __sfr params to avoid infinite redirects, when in - * development mode. + * development mode. Also drop preview_theme_id so a stale value + * carried over from the browser cannot pin retries to the wrong + * theme; the dev server will re-inject the correct id. */ const searchParams = new URLSearchParams(browserSearch) - const filteredParams = new URLSearchParams([...searchParams].filter(([key]) => !key.startsWith('__sfr'))) + const filteredParams = new URLSearchParams( + [...searchParams].filter(([key]) => !key.startsWith('__sfr') && key !== 'preview_theme_id'), + ) const location = browserPathname + (filteredParams.toString() ? `?${filteredParams}` : '') return new Response(null, { diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts index 9279b397814..45b13f553c0 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts @@ -269,6 +269,20 @@ describe('dev proxy', () => { expect(result.headers.get('etag')).toBe('"abc123"') expect(result.body).toBeNull() }) + + test('patches redirect locations and removes internal rendering params', async () => { + const redirectResponse = new Response(null, { + status: 302, + headers: { + Location: 'https://my-store.myshopify.com/products/test?_fd=0&pb=0&preview_theme_id=123&variant=1', + }, + }) + + const result = await patchRenderingResponse(ctx, redirectResponse) + + expect(result.status).toBe(302) + expect(result.headers.get('Location')).toBe('/products/test?variant=1') + }) }) describe('getProxyStorefrontHeaders', () => { diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.ts index debfc8e6d94..d5a8203f72f 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.ts @@ -183,13 +183,13 @@ export async function patchRenderingResponse( rawResponse: Response, patchCallback?: (html: string) => string | undefined, ): Promise { - // 3xx responses should be returned + const response = patchProxiedResponseHeaders(ctx, rawResponse) + + // 3xx responses should be returned after header patching. if (rawResponse.status >= 300 && rawResponse.status < 400) { - return rawResponse + return response } - const response = patchProxiedResponseHeaders(ctx, rawResponse) - // Only set HTML content-type for actual HTML responses, preserve JSON content-type: const originalContentType = rawResponse.headers.get('content-type') const isJsonResponse = originalContentType?.includes('application/json') @@ -255,6 +255,7 @@ function patchProxiedResponseHeaders(ctx: DevServerContext, rawResponse: Respons if (!CHECKOUT_PATTERN.test(url.pathname)) { url.searchParams.delete('_fd') url.searchParams.delete('pb') + url.searchParams.delete('preview_theme_id') response.headers.set('Location', url.href.replace(url.origin, '')) } } diff --git a/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.test.ts b/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.test.ts index 87e32234bea..fdd3ae74087 100644 --- a/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.test.ts @@ -46,7 +46,7 @@ describe('render', () => { expect(response.headers.get('Content-Type')).toEqual('application/json') expect(response.headers.get('something')).toEqual('else') expect(fetch).toHaveBeenCalledWith( - 'https://store.myshopify.com/products/1?_fd=0&pb=0', + 'https://store.myshopify.com/products/1?_fd=0&pb=0&preview_theme_id=123', expect.objectContaining({ method: 'GET', headers: expect.objectContaining({ @@ -104,7 +104,7 @@ describe('render', () => { expect(response.headers.get('Content-Type')).toEqual('application/json') expect(response.headers.get('something')).toEqual('else') expect(fetch).toHaveBeenCalledWith( - 'https://theme-kit-access.shopifyapps.com/cli/sfr/products/1?_fd=0&pb=0', + 'https://theme-kit-access.shopifyapps.com/cli/sfr/products/1?_fd=0&pb=0&preview_theme_id=123', expect.objectContaining({ method: 'GET', headers: expect.objectContaining({ @@ -116,7 +116,7 @@ describe('render', () => { }), ) expect(fetch).toHaveBeenCalledWith( - 'https://theme-kit-access.shopifyapps.com/cli/sfr/products/1?_fd=0&pb=0', + 'https://theme-kit-access.shopifyapps.com/cli/sfr/products/1?_fd=0&pb=0&preview_theme_id=123', expect.objectContaining({ method: 'GET', headers: expect.not.objectContaining({ @@ -139,7 +139,7 @@ describe('render', () => { // Then expect(response.status).toEqual(200) expect(fetch).toHaveBeenCalledWith( - 'https://store.myshopify.com/products/1?_fd=0&pb=0§ion_id=sections--1__announcement-bar', + 'https://store.myshopify.com/products/1?_fd=0&pb=0&preview_theme_id=123§ion_id=sections--1__announcement-bar', expect.objectContaining({ method: 'GET', headers: expect.objectContaining({ @@ -165,7 +165,7 @@ describe('render', () => { // Then expect(response.status).toEqual(200) expect(fetch).toHaveBeenCalledWith( - 'https://store.myshopify.com/products/1?_fd=0&pb=0&app_block_id=00001111222233334444', + 'https://store.myshopify.com/products/1?_fd=0&pb=0&preview_theme_id=123&app_block_id=00001111222233334444', expect.objectContaining({ method: 'GET', headers: expect.objectContaining({ @@ -192,7 +192,7 @@ describe('render', () => { // Then expect(response.status).toEqual(200) expect(fetch).toHaveBeenCalledWith( - 'https://store.myshopify.com/products/1?_fd=0&pb=0§ion_id=sections--1__announcement-bar', + 'https://store.myshopify.com/products/1?_fd=0&pb=0&preview_theme_id=123§ion_id=sections--1__announcement-bar', expect.objectContaining({ method: 'GET', headers: expect.objectContaining({ @@ -221,7 +221,36 @@ describe('render', () => { // Then expect(response.status).toEqual(200) expect(fetch).toHaveBeenCalledWith( - 'https://store.myshopify.com/products/1?_fd=0&pb=0&value=A&value=B', + 'https://store.myshopify.com/products/1?_fd=0&pb=0&value=A&value=B&preview_theme_id=123', + expect.objectContaining({ + method: 'GET', + headers: expect.objectContaining({ + Authorization: 'Bearer token_111222333', + Cookie: 'theme_cookie=abc; storefront_digest=00001111222233334444; _shopify_essential=:00112233445566778899:', + 'Content-Length': '100', + 'X-Special-Header': '200', + }), + }), + ) + }) + + test('preserves preview_theme_id from query parameters', async () => { + // Given + vi.mocked(fetch).mockResolvedValue(new Response()) + + // When + const response = await render(session, { + ...context, + query: [ + ['preview_theme_id', '456'], + ['value', 'A'], + ], + }) + + // Then + expect(response.status).toEqual(200) + expect(fetch).toHaveBeenCalledWith( + 'https://store.myshopify.com/products/1?_fd=0&pb=0&preview_theme_id=456&value=A', expect.objectContaining({ method: 'GET', headers: expect.objectContaining({ diff --git a/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.ts b/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.ts index 4a61e423558..6c2a969b1a0 100644 --- a/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.ts +++ b/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.ts @@ -114,7 +114,10 @@ export function buildCookies(session: DevServerSession, ctx: Pick