Fix theme profile for URLs with a query string and report redirects

Split the --url into a path and a query before rendering. The renderer appends its own query, so a --url with a query string produced a malformed URL that dropped _fd=0, and stores with a custom primary domain answered with a 301.

Raise an AbortError with the redirect destination as the next step when the profiled page redirects, with a storefront password hint for /password. Raise an AbortError for other non-200 responses too, so they are reported as expected errors instead of crashes.

Author: MillanWangShopify

.changeset/theme-profile-redirect.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme': patch
+---
+
+Fix `theme profile` failing with "Bad response: 301" when `--url` includes a query string, and show an actionable error when the page redirects.

packages/theme/src/cli/services/profile.test.ts

@@ -112,20 +112,127 @@ describe('profile', () => {
     await expect(result).rejects.toThrow('Network error')
   })
 
-  test('throws error when response status is not 200', async () => {
+  test('passes the query string of the url separately from the path', async () => {
+    // When
+    await profile(mockAdminSession, themeId, '/search?q=kw&page=2#results')
+
+    // Then
+    expect(render).toHaveBeenCalledWith(
+      expect.anything(),
+      expect.objectContaining({
+        path: '/search',
+        query: [
+          ['q', 'kw'],
+          ['page', '2'],
+        ],
+      }),
+    )
+  })
+
+  test('does not let the url override the storefront renderer query params', async () => {
+    // When
+    await profile(mockAdminSession, themeId, '/collections/all?_fd=1&pb=1&page=2')
+
+    // Then
+    expect(render).toHaveBeenCalledWith(
+      expect.anything(),
+      expect.objectContaining({path: '/collections/all', query: [['page', '2']]}),
+    )
+  })
+
+  test('uses only the path and query of an absolute url', async () => {
+    // When
+    await profile(mockAdminSession, themeId, 'https://custom-domain.com/products/test?variant=1')
+
+    // Then
+    expect(render).toHaveBeenCalledWith(
+      expect.anything(),
+      expect.objectContaining({path: '/products/test', query: [['variant', '1']]}),
+    )
+  })
+
+  test('suggests the redirect destination when the page redirects', async () => {
+    // Given
+    vi.mocked(render).mockResolvedValue(
+      new Response(null, {
+        status: 301,
+        headers: {Location: 'https://custom-domain.com/de/pages/x?_fd=0&pb=0&view=full'},
+      }),
+    )
+
+    // When
+    const result = profile(mockAdminSession, themeId, urlPath)
+
+    // Then
+    await expect(result).rejects.toThrow(
+      new AbortError(
+        `The page ${urlPath} redirects (301) instead of rendering, so it can't be profiled.`,
+        'It redirects to /de/pages/x?view=full. Run the command again with --url "/de/pages/x?view=full".',
+      ),
+    )
+  })
+
+  test('suggests the storefront password when the page redirects to the password page', async () => {
+    // Given
+    vi.mocked(render).mockResolvedValue(new Response(null, {status: 302, headers: {Location: '/password'}}))
+
+    // When
+    const result = profile(mockAdminSession, themeId, '/')
+
+    // Then
+    await expect(result).rejects.toThrow(
+      new AbortError(
+        "The page / redirects (302) instead of rendering, so it can't be profiled.",
+        "It redirects to the storefront password page, so the storefront password wasn't accepted. Run the command again with the correct --store-password.",
+      ),
+    )
+  })
+
+  test('reports a redirect without a location as an expected error', async () => {
+    // Given
+    vi.mocked(render).mockResolvedValue(new Response(null, {status: 301}))
+
+    // When
+    const result = profile(mockAdminSession, themeId, urlPath)
+
+    // Then
+    await expect(result).rejects.toThrow(
+      new AbortError(`The page ${urlPath} redirects (301) instead of rendering, so it can't be profiled.`),
+    )
+  })
+
+  test('reports a non-200 response as an expected error', async () => {
     // Given
     vi.mocked(render).mockResolvedValue(
       new Response(JSON.stringify({error: 'Some error message'}), {
         status: 404,
-        headers: new Headers({'content-type': 'application/json'}),
+        headers: new Headers({'content-type': 'application/json', 'x-request-id': 'request-id'}),
       }),
     )
 
     // When
     const result = profile(mockAdminSession, themeId, urlPath)
 
     // Then
-    await expect(result).rejects.toThrow('Bad response: 404: {"error":"Some error message"}')
+    await expect(result).rejects.toThrow(
+      new AbortError(
+        `Failed to profile ${urlPath}. The storefront responded with 404 (x-request-id: request-id): {"error":"Some error message"}`,
+      ),
+    )
+  })
+
+  test('does not include a long non-JSON response body in the error', async () => {
+    // Given
+    const htmlPage = `<html><body>${'Server error. '.repeat(20)}</body></html>`
+    vi.mocked(render).mockResolvedValue(new Response(htmlPage, {status: 500}))
+
+    // When
+    const result = profile(mockAdminSession, themeId, urlPath)
+
+    // Then
+    await expect(result).rejects.toThrow(
+      new AbortError(`Failed to profile ${urlPath}. The storefront responded with 500`),
+    )
   })
 
   test('throws error when a password is used', async () => {

packages/theme/src/cli/services/profile.ts

@@ -6,6 +6,12 @@ import {isStorefrontPasswordProtected} from '../utilities/theme-environment/stor
 import {AdminSession} from '@shopify/cli-kit/node/session'
 import {AbortError} from '@shopify/cli-kit/node/error'
 
+/** Query parameters that the storefront renderer always sets itself. */
+const RENDERER_QUERY_PARAMS = ['_fd', 'pb']
+
+/** Longest non-JSON response body included in an error message, so full HTML pages are never shown. */
+const MAX_ERROR_BODY_LENGTH = 200
+
 export async function profile(
   adminSession: AdminSession,
   themeId: string,
@@ -25,22 +31,64 @@ export async function profile(
   }
 
   const session = await fetchDevServerSession(themeId, adminSession, themeAccessPassword, storePassword)
+  const target = storefrontTarget(url)
   const response = await render(session, {
     method: 'GET',
-    path: url,
-    query: [],
+    path: target.pathname,
+    query: [...target.searchParams],
     themeId,
     headers: {
       Accept: 'application/vnd.speedscope+json',
     },
   })
 
+  if (response.status >= 300 && response.status < 400) {
+    throw new AbortError(
+      `The page ${url} redirects (${response.status}) instead of rendering, so it can't be profiled.`,
+      redirectTryMessage(response.headers.get('Location')),
+    )
+  }
+
   if (response.status !== 200) {
-    const body = await response.text()
-    throw new Error(`Bad response: ${response.status}: ${body}`)
+    const details = await responseErrorDetails(response)
+    throw new AbortError(`Failed to profile ${url}. The storefront responded with ${response.status}${details}`)
   }
 
   const profileJson = await response.text()
 
   return {result: themeProfileResultSchema.parse(JSON.parse(profileJson)), source: profileJson}
 }
+
+/**
+ * Parses a `--url` or `Location` value into a store-relative path and query.
+ * The renderer adds its own query, so the query has to be passed separately from the path.
+ */
+function storefrontTarget(url: string) {
+  const target = new URL(url, 'https://storefront.invalid')
+  RENDERER_QUERY_PARAMS.forEach((param) => target.searchParams.delete(param))
+  return target
+}
+
+function redirectTryMessage(location: string | null) {
+  if (!location) return null
+
+  const redirectTarget = storefrontTarget(location)
+  if (redirectTarget.pathname === '/password') {
+    return "It redirects to the storefront password page, so the storefront password wasn't accepted. Run the command again with the correct --store-password."
+  }
+
+  const suggestedUrl = `${redirectTarget.pathname}${redirectTarget.search}`
+  return `It redirects to ${suggestedUrl}. Run the command again with --url "${suggestedUrl}".`
+}
+
+async function responseErrorDetails(response: Response) {
+  const requestId = response.headers.get('x-request-id')
+  const requestIdDetail = requestId ? ` (x-request-id: ${requestId})` : ''
+
+  const body = (await response.text()).trim()
+  const isJsonBody = response.headers.get('Content-Type')?.includes('application/json') ?? false
+  const showBody = body.length > 0 && (isJsonBody || body.length <= MAX_ERROR_BODY_LENGTH)
+  const bodyDetail = showBody ? `: ${body}` : ''
+
+  return `${requestIdDetail}${bodyDetail}`
+}