[Themes] Add error overlay for theme upload failures #5208

jamesmengo · 2025-01-15T22:08:15Z

WHY are these changes introduced?

Provide a visual indicator in the rendered development host server when asset upload errors occur. This can make it easier to spot / identify when you live theme may differ from what theme dev is serving.

In the theme dev proxy server, we directly serve asset values to SFR while waiting for upload of the file. This means that may still render content that is actually not being saved to the remote theme - this can be confusing, and currently the only indicator of this is in the terminal)

WHAT is this pull request doing?

Adds an error overlay system that displays theme development errors in the browser. This PR only renders NEW errors you introduce while the theme dev server is running.

The following PR in this stack will include the initial theme upload results in the overlay.

I want this PR to focus on the overlay itself / how we are storing the data.

🎥 Video uploaded on Graphite:

How to test your changes?

Start theme development server
Introduce an error in a theme file (e.g., invalid Liquid syntax)
Save the file, wait for the terminal to report a failure, and refresh your localhost

Measuring impact

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes

jamesmengo · 2025-01-15T22:08:40Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions · 2025-01-15T22:12:26Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	75.16% (-0.01% 🔻)	8893/11832
🟡	Branches	70.28% (-0.02% 🔻)	4342/6178
🟡	Functions	75.06%	2326/3099
🟡	Lines	75.71% (-0.01% 🔻)	8408/11105

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / ConcurrentOutput.tsx	98.36% (-1.64% 🔻)	88% (-4% 🔻)	100%	98.33% (-1.67% 🔻)

Test suite run success

2007 tests passing in 906 suites.

Report generated by 🧪jest coverage report action from c720fc6

github-actions · 2025-01-15T22:55:59Z

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

jamesmengo · 2025-01-15T23:04:26Z

packages/theme/src/cli/utilities/theme-fs.ts

+        } else {
+          const errors = result?.errors?.asset ?? ['Response was not successful.']
+          uploadErrors.set(fileKey, errors)
+          throw new Error(errors.length === 1 ? errors[0] : errors.join('\n'))


we want to keep throwing the error here so that we output to the terminal.

jamesmengo · 2025-01-15T23:57:33Z

packages/cli-kit/src/public/node/themes/types.ts

+  /**
+   * Stores upload errors returned when uploading files via the Asset API
+   */
+  uploadErrors: Map<Key, string[]>


I considered storing this inside of the ThemeAsset type

I chose to store it here because

This type is simple, easy to update, and communicates that this is a temporary store of errors rather than an actual property of the theme asset

- It is likely that we will update this and check if this is empty frequently. We want these operations to be fast and straightforward

Checking whether uploadErrors are present is be O(1) since we don't need to read all theme assets

We COULD do a two-pronged approach where we store a list of files with errors in a Set, and store the errors themselves on the ThemeAsset. I tried this and it felt clunky.

Updates should be fast and easy

If we store in ThemeAsset, we'd need to get the asset, check if it exists, and update the error key

graygilmore

Conceptually I love this. Let's see if we can put a little polish on it!

graygilmore · 2025-01-15T23:51:36Z

packages/theme/src/cli/utilities/theme-fs.ts

+        } else {
+          const errors = result?.errors?.asset ?? ['Response was not successful.']
+          uploadErrors.set(fileKey, errors)
+          throw new Error(errors.length === 1 ? errors[0] : errors.join('\n'))


Suggested change

throw new Error(errors.length === 1 ? errors[0] : errors.join('\n'))

throw new Error(errors.join('\n'))

> ['foo'].join(',') 'foo'

graygilmore · 2025-01-16T00:14:43Z

packages/theme/src/cli/utilities/theme-environment/hot-reload/error-overlay.ts

+    .join('')
+
+  return `
+      <div id="section-error-overlay" style="${OVERLAY_STYLES.container}">


Would prefer to use something like the dialog element so that we don't need to manually roll a bunch of accessibility features: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

graygilmore · 2025-01-16T00:15:44Z

packages/theme/src/cli/utilities/theme-environment/hot-reload/error-overlay.ts

+}
+
+export function injectErrorIntoHtml(html: string, errors: Map<string, string[]>): string {
+  return html + getErrorOverlay(errors)


If I had an error and then another error would this end up rendering multiple overlays?

graygilmore · 2025-01-16T00:17:21Z

packages/theme/src/cli/utilities/theme-environment/html.test.ts

+
+    // Then
+    const overlay = getErrorOverlay(errors)
+    expect(result).toBe(html + overlay)


🤔 would this imply that our overlay is rendering after the closing </html> tag? If so let's make sure we're rendering inside of the document.

graygilmore · 2025-01-16T00:17:50Z

packages/theme/src/cli/utilities/theme-environment/hot-reload/error-overlay.ts

@@ -0,0 +1,74 @@
+const OVERLAY_STYLES = {


I have a few opinions on the styling here but let's settle on implementation before tweaking a bunch of this!

@graygilmore let's pair on this tommorow! Some of your questions will be resolved in the stack above this.

This PR is definitely the place to talk about styling / the overlay itself, so thanks for raising that!

github-actions · 2025-01-16T18:30:44Z

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

Some seemingly private modules might be re-exported through public modules.
If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/private/node/constants.d.ts

@@ -31,6 +31,7 @@ export declare const environmentVariables: {
     otelURL: string;
     themeKitAccessDomain: string;
     json: string;
+    useAppManagement: string;
 };
 export declare const defaultThemeKitAccessDomain = "theme-kit-access.shopifyapps.com";
 export declare const systemEnvironmentVariables: {

packages/cli-kit/dist/private/node/api/headers.d.ts

@@ -1,6 +1,6 @@
 import { ExtendableError } from '../../../public/node/error.js';
 import https from 'https';
-declare class RequestClientError extends ExtendableError {
+export declare class RequestClientError extends ExtendableError {
     statusCode: number;
     constructor(message: string, statusCode: number);
 }
@@ -25,5 +25,4 @@ export declare function buildHeaders(token?: string): {
  * if the service is running in a Spin environment, the attribute "rejectUnauthorized" is
  * set to false
  */
-export declare function httpsAgent(): Promise<https.Agent>;
-export {};
\ No newline at end of file
+export declare function httpsAgent(): Promise<https.Agent>;
\ No newline at end of file

packages/cli-kit/dist/private/node/session/scopes.d.ts

@@ -5,7 +5,7 @@ import { API } from '../api.js';
  * @param extraScopes - custom user-defined scopes
  * @returns Array of scopes
  */
-export declare function allDefaultScopes(extraScopes?: string[]): string[];
+export declare function allDefaultScopes(extraScopes?: string[], systemEnvironment?: NodeJS.ProcessEnv): string[];
 /**
  * Generate a flat array with the default scopes for the given API plus
  * any custom scope defined by the user
@@ -13,4 +13,4 @@ export declare function allDefaultScopes(extraScopes?: string[]): string[];
  * @param extraScopes - custom user-defined scopes
  * @returns Array of scopes
  */
-export declare function apiScopes(api: API, extraScopes?: string[]): string[];
\ No newline at end of file
+export declare function apiScopes(api: API, extraScopes?: string[], systemEnvironment?: NodeJS.ProcessEnv): string[];
\ No newline at end of file

packages/cli-kit/dist/public/node/context/local.d.ts

@@ -26,12 +26,12 @@ export declare function isDevelopment(env?: NodeJS.ProcessEnv): boolean;
  */
 export declare function isVerbose(env?: NodeJS.ProcessEnv): boolean;
 /**
- * It returns true if the App Management API is disabled.
- * This should only be relevant when using a Partners token.
+ * It returns true if the App Management API is available.
  *
- * @returns True if the App Management API is disabled.
+ * @param env - The environment variables from the environment of the current process.
+ * @returns True if the App Management API is available.
  */
-export declare function isAppManagementDisabled(): boolean;
+export declare function isAppManagementEnabled(env?: NodeJS.ProcessEnv): boolean;
 /**
  * Returns true if the environment in which the CLI is running is either
  * a local environment (where dev is present) or a cloud environment (spin).

packages/cli-kit/dist/public/node/themes/types.d.ts

@@ -92,6 +92,10 @@ export interface ThemeFileSystem extends VirtualFileSystem {
     applyIgnoreFilters: <T extends {
         key: string;
     }>(files: T[]) => T[];
+    /**
+     * Stores upload errors returned when uploading files via the Asset API
+     */
+    uploadErrors: Map<Key, string[]>;
 }
 /**
  * Represents a theme on the file system.

jamesmengo force-pushed the jm/error_overlay branch 2 times, most recently from 296bee3 to 5c60ebd Compare January 15, 2025 22:53

jamesmengo changed the base branch from main to jm/01-15-unify_asset_upload_error_reporting January 15, 2025 22:54

jamesmengo mentioned this pull request Jan 15, 2025

[Themes] theme dev - unify asset upload error reporting #5209

Open

5 tasks

jamesmengo marked this pull request as ready for review January 15, 2025 22:55

jamesmengo requested review from a team as code owners January 15, 2025 22:55

jamesmengo self-assigned this Jan 15, 2025

jamesmengo added the Area: @shopify/theme @shopify/theme package issues label Jan 15, 2025

jamesmengo added the Type: Enhancement New feature or request label Jan 15, 2025

jamesmengo force-pushed the jm/error_overlay branch from 5c60ebd to d659886 Compare January 15, 2025 23:02

jamesmengo commented Jan 15, 2025

View reviewed changes

jamesmengo force-pushed the jm/error_overlay branch from d659886 to 4ba80de Compare January 15, 2025 23:05

jamesmengo mentioned this pull request Jan 15, 2025

[Themes] Theme Dev - Render asset upload errors coming from initial theme upload #5210

Draft

3 tasks

jamesmengo commented Jan 15, 2025

View reviewed changes

graygilmore reviewed Jan 16, 2025

View reviewed changes

jamesmengo mentioned this pull request Jan 16, 2025

[Themes] Trigger hot reload to update/render error overlay on asset upload error / fix #5211

Draft

5 tasks

jamesmengo marked this pull request as draft January 16, 2025 18:52

jamesmengo added 4 commits January 16, 2025 12:04

Add uploadErrors property to theme file system

aca55f8

Save asset upload failures during theme dev to uploadErrors map

2b10d6b

Render error overlay when themeFileSystem has upload errors

50a605e

PIVOT - Use existing errorPage to render error overlay

97f475a

jamesmengo force-pushed the jm/error_overlay branch from c720fc6 to 97f475a Compare January 16, 2025 20:05

jamesmengo force-pushed the jm/01-15-unify_asset_upload_error_reporting branch from 454ca51 to 0971b54 Compare January 16, 2025 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Themes] Add error overlay for theme upload failures #5208

[Themes] Add error overlay for theme upload failures #5208

jamesmengo commented Jan 15, 2025 •

edited

Loading

jamesmengo commented Jan 15, 2025 •

edited

Loading

github-actions bot commented Jan 15, 2025 •

edited

Loading

github-actions bot commented Jan 15, 2025

jamesmengo Jan 15, 2025

jamesmengo Jan 15, 2025

graygilmore left a comment

graygilmore Jan 15, 2025

graygilmore Jan 16, 2025

graygilmore Jan 16, 2025

graygilmore Jan 16, 2025

graygilmore Jan 16, 2025

jamesmengo Jan 16, 2025

github-actions bot commented Jan 16, 2025

	throw new Error(errors.length === 1 ? errors[0] : errors.join('\n'))
	throw new Error(errors.join('\n'))

[Themes] Add error overlay for theme upload failures #5208

Are you sure you want to change the base?

[Themes] Add error overlay for theme upload failures #5208

Conversation

jamesmengo commented Jan 15, 2025 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Measuring impact

Checklist

jamesmengo commented Jan 15, 2025 • edited Loading

github-actions bot commented Jan 15, 2025 • edited Loading

Coverage report

Test suite run success

github-actions bot commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graygilmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 16, 2025

Differences in type declarations

New type declarations

Existing type declarations

jamesmengo commented Jan 15, 2025 •

edited

Loading

jamesmengo commented Jan 15, 2025 •

edited

Loading

github-actions bot commented Jan 15, 2025 •

edited

Loading