[Feature] Render error page when `theme dev` encounters asset upload error #5221

jamesmengo · 2025-01-16T20:21:02Z

WHY are these changes introduced?

Fixes https://github.com/Shopify/develop-advanced-edits/issues/425

Builds on top of: #5220

Builds on top of

When using the theme dev command, some users were seeing discrepancies between the localhost preview @ 127.0.01 and the remote theme via the Online Store Editor.

This scenario occurs when users are editing their assets and create an invalid change. This change prevents the file from getting uploaded to their remote theme. While this file is not saved to the remote theme, it is still being passed directly to SFR in our localhost preview (we do this to enable a snappy preview experience).

Error Page Design

WHAT is this pull request doing?

Video

This change will render an error page over the when an asset upload error is encountered (this depends on when the server responds).

These errors are asynchronous sine they come from the result of an upload operation, and can:

exist before running theme dev
be added while the theme dev server is running

Limitations:

This will only work for errors that cause the asset update operation to fail. Depending on the code itself, certain changes may actually not prevent you from saving entirely, and would not be caught here.
The validation code lives in core

How to test your changes?

Run shopify-dev theme dev in a valid theme
Test errors added while server is active: Create errors across multiple files. Note the limitations above.
Note that your page should refresh and update as you create more errors
Resolve the errors.
Confirm that the errors are removed as you fix the files
Test pre-existing errors: Do step 2, then 1

Errors added while server is active

Demo.-.asset.upload.errors.whil.server.is.active.mp4

Pre-Existing Errors

iTerm2.-.jamesmeng@Jamess-MacBook-Pro-2_._src_github.com_Shopify_jm-theme.mp4

Post-release steps

Areas for future extension -> we could grab the line number and render the lines before / after, similar to how vite handles the error page

Measuring impact

How do we know this change was effective? Please choose one:

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
Existing analytics will cater for this addition
PR includes analytics changes to measure impact

Checklist

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

jamesmengo · 2025-01-16T20:21:27Z

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

github-actions · 2025-01-16T20:25:10Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	75.57% (-0.03% 🔻)	9048/11973
🟡	Branches	70.81% (-0.03% 🔻)	4411/6229
🟡	Functions	75.36% (-0.04% 🔻)	2373/3149
🟡	Lines	76.08% (-0.02% 🔻)	8544/11230

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / app-event-watcher.ts	95.18% (-1.2% 🔻)	86.49% (-2.7% 🔻)	95.45%	100%
🟡	`...` / local.ts	62.96% (-6.13% 🔻)	52% (-4.6% 🔻)	47.83% (-6.34% 🔻)	62.96% (-6.13% 🔻)

Test suite run success

2040 tests passing in 911 suites.

Report generated by 🧪jest coverage report action from 599cdd2

github-actions · 2025-01-16T23:34:32Z

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/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.

karreiro

Thanks a lot for this PR, @jamesmengo! Excellent stuff 🔥 It's great how this raises awareness on Liquid errors :)

I've noticed only one minor issue in the grouping of errors (as you may notice /sections/collage.liquid is not bold):

Here's the content of my files with errors (I'm using dawn on main):

Thanks again for this PR! Looking forward to seeing this feature with developers :)

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

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

edmund-teh · 2025-01-24T18:26:24Z

Great work @jamesmengo! Love this project and the improvements you're making with this error page.

Sorry— one small pedantic change tho... can we change the icon to AlertCircleIcon (https://polaris.shopify.com/icons?icon=AlertCircleIcon)? You're currently using AlertDiamondIcon, which is inconsistent with what we're doing in the code editor:

I realize that's what we're using with the default banner-critical: https://polaris.shopify.com/components/feedback-indicators/banner?example=banner-critical

But I'd like to keep things consistent— thank you!

frandiox

Looking great 🔥

frandiox · 2025-01-28T04:29:47Z

packages/theme/src/cli/commands/theme/dev.ts

+    'error-overlay': Flags.string({
+      description: `Controls the visibility of the error overlay when an theme asset upload fails:
+- silent Prevents the error overlay from appearing.
+- default Displays the error overlay.
+      `,
+      options: ['silent', 'default'],


Instead of passing values, should we pass allowNo: true so that devs can use the flag as --no-error-overlay to disable the behavior? Or do we expect to have more options in the future?

yeah, the idea is to leave room for expansion moving forward, though there's no clear timeline for that so perhaps something we could call into question

frandiox · 2025-01-28T04:35:59Z

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

        assertThemeId(response, html, String(theme.id))

+        if (ctx.options.errorOverlay !== 'silent' && ctx.localThemeFileSystem.uploadErrors.size > 0) {
+          html = getErrorPage({


I was going to say that maybe we should set the status here to 500. However, what we are showing is the summary of the upload errors, not the rendering errors... so perhaps we should not change the status? :confused_parrot:
Ideally we'd just inject a modal overlay on top of the rendered page so that it makes sense to use status 200 and still show errors. But that doesn't need to be done now of course.
Thoughts? cc @karreiro

-- Thinking more about this, the observed result from the user is an error page so I think we should probably return status 500 regardless of the nature of the error until we move this to a modal 🤔

Do you think we should try to expand the accepted types of the catch block below and use that?

Actually, what if you do this check before calling render and just return the error without reaching SFR? Since it's not a real overlay yet, we don't need the actual storefront document, right?

Personally, I kind of like that it blocks the entire page, as we set a bit more deterministic and side-effect-free behavior (with less dependencies with the content of the theme).

…ay rendering

…call to SFR

frandiox

Great job, thanks!

karreiro

Thank you, @jamesmengo! Great job indeed ✨

I can no longer reproduce the issue I noticed before :) 🚀

jamesmengo · 2025-02-07T17:43:32Z

Thank you, @jamesmengo! Great job indeed ✨

I can no longer reproduce the issue I noticed before :) 🚀

@karreiro sorry, should have followed up.
I fixed this by escaping HTML: https://github.com/Shopify/cli/pull/5221/files#diff-c2f322ee42cf634d8c54bb93b59f052f5618ea664f5440ff5bc213bcbecf2d19R8-R15

Special characters in the error message were causing the issue you saw last time!

jamesmengo mentioned this pull request Jan 16, 2025

[Themes] Store and track theme asset upload errors during theme dev #5220

Merged

3 tasks

jamesmengo force-pushed the jm/store_asset_upload_failures branch from 550b4c6 to 6edd52f Compare January 16, 2025 20:25

jamesmengo force-pushed the jm/render_error_page branch from 7ef0845 to 7152c71 Compare January 16, 2025 20:27

jamesmengo force-pushed the jm/store_asset_upload_failures branch from 6edd52f to dd17d8a Compare January 16, 2025 23:31

jamesmengo force-pushed the jm/render_error_page branch from 7152c71 to d7665cd Compare January 16, 2025 23:32

jamesmengo force-pushed the jm/store_asset_upload_failures branch from dd17d8a to 7e34195 Compare January 16, 2025 23:58

Base automatically changed from jm/store_asset_upload_failures to main January 17, 2025 00:58

jamesmengo force-pushed the jm/render_error_page branch 2 times, most recently from 8f1e4e3 to c7c56e8 Compare January 23, 2025 04:15

jamesmengo changed the title ~~Render error page on theme asset upload error~~ [Feature] Render error page when theme dev encounters asset upload error Jan 24, 2025

jamesmengo marked this pull request as ready for review January 24, 2025 01:01

jamesmengo requested review from a team as code owners January 24, 2025 01:01

jamesmengo assigned jamesmengo and edmund-teh Jan 24, 2025

This comment has been minimized.

Sign in to view

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

jamesmengo requested review from frandiox, graygilmore and karreiro January 24, 2025 01:04

karreiro reviewed Jan 24, 2025

View reviewed changes

packages/theme/src/cli/utilities/theme-environment/html.ts Outdated Show resolved Hide resolved

packages/theme/src/cli/utilities/theme-environment/hot-reload/error-page.ts Show resolved Hide resolved

jamesmengo force-pushed the jm/render_error_page branch 3 times, most recently from c114df1 to 325191f Compare January 28, 2025 03:02

jamesmengo marked this pull request as draft January 28, 2025 03:04

jamesmengo force-pushed the jm/render_error_page branch from 0d5959a to b3049a4 Compare January 28, 2025 03:23

jamesmengo marked this pull request as ready for review January 28, 2025 03:30

jamesmengo requested a review from karreiro January 28, 2025 03:35

frandiox reviewed Jan 28, 2025

View reviewed changes

jamesmengo added 5 commits February 5, 2025 18:08

Render error page on theme asset upload error

45d29fc

Trigger full reload when error overlay needs to be updated

b2f0ae1

Add 'error-overlay' flag to theme dev command to toggle error overl…

f26ff7f

…ay rendering

Bugfix - Escape HTML in error messages

1761d5e

Changeset + docs

fca9c19

jamesmengo force-pushed the jm/render_error_page branch from b3049a4 to 7cc806f Compare February 6, 2025 02:26

jamesmengo requested a review from frandiox February 6, 2025 02:26

Return 500 status when asset upload error page is rendered

e771c6b

jamesmengo force-pushed the jm/render_error_page branch from 7cc806f to e771c6b Compare February 6, 2025 02:28

Refactor: Move asset upload error page rendering prior to the render …

599cdd2

…call to SFR

frandiox approved these changes Feb 7, 2025

View reviewed changes

karreiro approved these changes Feb 7, 2025

View reviewed changes

jamesmengo added this pull request to the merge queue Feb 7, 2025

Merged via the queue into main with commit dd374c4 Feb 7, 2025
4 checks passed

jamesmengo deleted the jm/render_error_page branch February 7, 2025 17:39

shopify-shipit bot temporarily deployed to nightly February 8, 2025 13:48 Inactive

jamesmengo mentioned this pull request Feb 19, 2025

[Feature]: Shopify CLI to make an alert sound when there is an error #5430

Closed

[Feature] Render error page when theme dev encounters asset upload error #5221

[Feature] Render error page when theme dev encounters asset upload error #5221

Uh oh!

Conversation

jamesmengo commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Post-release steps

Measuring impact

Checklist

Uh oh!

jamesmengo commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Test suite run success

Uh oh!

github-actions bot commented Jan 16, 2025

Differences in type declarations

New type declarations

Existing type declarations

Uh oh!

This comment has been minimized.

karreiro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edmund-teh commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frandiox left a comment

Choose a reason for hiding this comment

Uh oh!

frandiox Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

jamesmengo Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

frandiox Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

jamesmengo Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

frandiox Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

karreiro Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frandiox left a comment

Choose a reason for hiding this comment

Uh oh!

karreiro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesmengo commented Feb 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[Feature] Render error page when `theme dev` encounters asset upload error #5221

[Feature] Render error page when `theme dev` encounters asset upload error #5221

jamesmengo commented Jan 16, 2025 •

edited

Loading

jamesmengo commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

edmund-teh commented Jan 24, 2025 •

edited

Loading

karreiro Feb 7, 2025 •

edited

Loading