Skip to content

Watch admin static_root and serve assets via dev server WebSocket#7152

Closed
melissaluu wants to merge 31 commits into
mainfrom
ml-add-staticRoot-ext-websocket
Closed

Watch admin static_root and serve assets via dev server WebSocket#7152
melissaluu wants to merge 31 commits into
mainfrom
ml-add-staticRoot-ext-websocket

Conversation

@melissaluu

@melissaluu melissaluu commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds end-to-end support for watching admin extension static_root directories during shopify app dev. When files change in the configured static_root directory, the dev server detects the change, updates asset timestamps in the payload store, and broadcasts the update over WebSocket so connected clients can react.

This PR does not include exposing CSP content for the app.

What changed

File watching pipeline — Extended the existing FileWatcherAppEventWatcher → dev server pipeline with a new app_asset_updated event type. The FileWatcher resolves static_root from the admin extension config, watches that directory, and emits app_asset_updated events on file changes. A new AppAssetUpdatedHandler propagates these as AppEvents with an appAssetsUpdated flag.

dist/ ignore exemption — Replaced the static **/dist/** chokidar ignore pattern with a function that allows dist/ paths through when they fall under a watched app asset directory (since static_root may point to a dist/ folder).

Payload store — Added app.assets to the WebSocket payload model: a Record<string, { url, lastUpdated }>. New methods updateAppAssetTimestamp() and updateAppAssets() manage timestamps and emit updates to WebSocket clients.

HTTP serving — Added a parameterized route at /extensions/assets/:assetKey/:filePath that serves files from the resolved asset directory.

Admin spec — Set experience: 'configuration' so the admin extension loads from shopify.app.toml. Added a proper zod schema ({ admin: { static_root? } }) so the extension is only instantiated when [admin] is present in the config. Also added optional schema support to createContractBasedModuleSpecification to enable this.

Test plan

  • loader.test.ts — 129 tests pass, including the 3 that were failing due to the admin spec change
  • extension.test.tsresolveAppAssets() returns correct map; appAssets passed through to HTTP server; getAppAssets callback works
  • store.test.tsupdateAppAssetTimestamp updates timestamp and emits; updateAppAssets rebuilds or removes assets; raw payload populates app.assets when configured
  • middlewares.test.tsgetAppAssetsMiddleware serves files for valid keys, returns 404 for unknown keys
  • file-watcher.test.ts — Updated chokidar ignored pattern assertion for the new dist/ filter function
  • app-event-watcher.test.ts — 19 tests pass

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings April 1, 2026 23:02
@melissaluu melissaluu requested a review from a team as a code owner April 1, 2026 23:02
@github-actions

github-actions Bot commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

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.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support to the UI extensions dev server for app-level “asset directories” (starting with admin extension static_root), including HTTP serving and WebSocket payload updates so clients can invalidate/reload assets when files change.

Changes:

  • Extend dev server WebSocket payload/types with optional app.assets (keyed map of {url, lastUpdated}).
  • Serve app-level assets from /extensions/assets/:assetKey/**:filePath when configured.
  • Watch configured asset directories and broadcast lastUpdated timestamp bumps on changes; re-resolve assets on app reload.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
packages/ui-extensions-server-kit/src/types.ts Adds App.assets to the shared server-kit type surface.
packages/app/src/cli/services/dev/processes/previewable-extension.ts Resolves and passes appAssets into the dev server process options.
packages/app/src/cli/services/dev/extension/server/middlewares.ts Introduces getAppAssetsMiddleware for serving app-level assets.
packages/app/src/cli/services/dev/extension/server/middlewares.test.ts Adds middleware tests for serving app assets + unknown key 404.
packages/app/src/cli/services/dev/extension/server.ts Wires the new assets route into the HTTP server when appAssets is present.
packages/app/src/cli/services/dev/extension/payload/store.ts Adds appAssets to payload initialization and new store update APIs.
packages/app/src/cli/services/dev/extension/payload/store.test.ts Adds unit tests for app asset timestamp updates + payload initialization behavior.
packages/app/src/cli/services/dev/extension/payload/models.ts Adds app.assets to the server-side payload model interface.
packages/app/src/cli/services/dev/extension.ts Implements resolveAppAssets, chokidar watchers, and app reload plumbing.
packages/app/src/cli/services/dev/extension.test.ts Adds tests for resolveAppAssets and passing appAssets through to HTTP server setup.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/app/src/cli/services/dev/extension/server/middlewares.ts Outdated
Comment thread packages/app/src/cli/services/dev/extension.ts Outdated
Comment thread packages/app/src/cli/services/dev/extension.ts
Comment thread packages/app/src/cli/services/dev/extension.ts Outdated
Comment thread packages/app/src/cli/services/dev/extension/payload/store.ts
Comment thread packages/app/src/cli/services/dev/extension/server/middlewares.ts
melissaluu and others added 5 commits April 2, 2026 14:55
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… leak

- Changed appAssets from a captured reference to a getAppAssets() callback
  in setupHTTPServer and getAppAssetsMiddleware, so the middleware always
  reads fresh asset config after app reloads
- Replaced unbounded debounceTimers array with a Map<string, setTimeout>
  keyed by assetKey, so each key has at most one timer entry

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…and fix upload assets on change

Co-authored-by: Trish Ta <trish.ta@shopify.com>
@melissaluu melissaluu force-pushed the ml-add-staticRoot-ext-websocket branch from 53e7bea to 5ce5d81 Compare April 2, 2026 18:56
@melissaluu melissaluu requested a review from isaacroldan April 6, 2026 23:57
Comment on lines 13 to +17
const adminSpecificationSpec = createContractBasedModuleSpecification({
identifier: 'admin',
uidStrategy: 'single',
experience: 'configuration',
schema: AdminSchema as unknown as ZodSchemaType<BaseConfigType>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are providing a schema here, then you shouldn't use createContractbasedModuleSpecification, because it's not using contracts anymore.
Use the generic createExtensionSpecification

return createExtensionSpecification({
identifier: spec.identifier,
schema: zod.any({}) as unknown as ZodSchemaType<TConfiguration>,
schema: spec.schema ?? (zod.any({}) as unknown as ZodSchemaType<TConfiguration>),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by definition, contract based modules can't provide a local schema.

@isaacroldan isaacroldan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's refactor and treat it as is identifier === 'admin' is a forbidden pattern. The file-watcher shouldn't know about specific extensions in any case.

Copy link
Copy Markdown
Contributor

Why do we need to treat app assets as a different event/output/property instead of just the output of another extension? This PR introduces the concept of App Assets everywhere, is it necessary? are App Assets just the output of an admin extension? can they come from a different extension? Seems like we are making the output of this specific extension a first-class element of the app, instead of treating it like any other extension.

lizkenyon and others added 12 commits April 7, 2026 11:16
Partner-facing summary of changes from Version Packages PR #6978

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…-plugin-cli

Neither package is directly used: @babel/core had no consumer (the only
babel reference is a rule set to 'off'), and eslint-plugin-jsx-a11y is
already pulled in transitively via @shopify/eslint-plugin.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The only usage was SemanticResourceAttributes.SERVICE_NAME (the string
literal 'service.name') in DefaultMeterProvider.ts. Inline the value
and drop the package dependency.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The commondir package is a tiny, unmaintained (~2014) utility. Inline
its logic as an exported commonParentDirectory() function in path.ts,
removing both the runtime dep and @types/commondir. Added parity tests
covering the original package's test cases.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace get-port-please with Node's built-in net.createServer for both
random port allocation (bind to port 0) and port availability checks.
This removes an unmaintained dependency and fixes the HOST env var bug
that required a hardcoded workaround.

Servers call unref() to prevent dangling handles from keeping the
process alive, matching get-port-please's behavior. Tests cover
integration behavior (real sockets, bindability verification) and
retry/error paths (mocked net).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Routes component used a catch-all route (path="*") that always
rendered <Extensions />. No actual routing was happening. Render the
component directly in App.tsx and remove the Routes wrapper, its
directory, and the react-router-dom dependency.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The only usage was a single diagnostic log line in no-inline-graphql.
Replace with a conditional console.error gated on process.env.DEBUG.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace tempy with Node's built-in fs.mkdtemp/mkdtempSync for temp
directory creation and cleanup. Add a small temp-dir.ts module that
captures the real temp directory path at module load time using async
realpath (via top-level await), which resolves both macOS symlinks
(/var -> /private/var) and Windows 8.3 short names (RUNNER~1 ->
runneradmin). Cleanup uses maxRetries: 2 for Windows robustness.
All matching tempy's behavior.

Fix three test files that used bare vi.mock('os') to use partial mocks
instead, preserving real os.tmpdir() while still allowing specific
function mocking (platform, hostname, EOL).

Also removes unused tempy dep from e2e and workspace packages.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move the jsdoc plugin dependency and registration from eslint-plugin-cli
to the root eslint.config.js where the jsdoc rules are actually
configured. The single jsdoc/require-returns-description rule that was
in eslint-plugin-cli's config.js is redundant with the comprehensive
jsdoc rule set in the root config, so it's removed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ryancbahan and others added 12 commits April 7, 2026 11:16
The single call site in content-tokens.ts is replaced with a direct
check of supportsHyperlinks.stdout and ansiEscapes.link(), both of
which are already dependencies used by the Link.tsx component.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…on factories

Ensures clientSteps is properly forwarded in createContractBasedModuleSpecification,
createExtensionSpecification, and createConfigExtensionSpecification.

Prevents regression of the bug fixed in #7141 where clientSteps was accidentally
dropped from createContractBasedModuleSpecification during a conflict resolution,
breaking channel_config extension builds.
Playwright traces capture page.fill() values verbatim, and traces are
uploaded as publicly downloadable CI artifacts. This stops tracing before
entering credentials and restarts it after login completes, navigating to
about:blank before restart so the first snapshot doesn't capture residual
form state. Also removes page HTML dump from login error messages since
filled input values could appear in the DOM.

Ref: shopify/bugbounty#3638393

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous approach (context.tracing.stop()) did not work — Playwright's
test runner instruments API calls at a level above context.tracing, so
fill() values are logged in traces regardless. Using evaluate() to set
input values via the DOM bypasses the Playwright action log entirely, so
credentials never appear in trace files or HTML reports.

Ref: shopify/bugbounty#3638393

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix prettier formatting for error string concatenation
- Avoid HTMLInputElement (not in tsconfig types: ["node"]) by casting
  through unknown

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Drop explicit Element type annotation (not in node-only tsconfig)
- Inline string concatenation to fix no-useless-concat

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Navigate to about:blank in the catch block before rethrowing so that
failure artifacts (screenshots, trace snapshots) do not capture the
login form with credentials still populated.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vividviolet

Copy link
Copy Markdown
Member

This PR introduces the concept of App Assets everywhere, is it necessary? are App Assets just the output of an admin extension? can they come from a different extension?

@isaacroldan I'm not sure what's the best way to handle this would be but this is different than other extensions because the settings comes from the shopify.app.toml. The way we fetch the data too will be from the app itself rather than an extension so it would be nice to keep the assets on the app's payload if possible.

@melissaluu melissaluu requested a review from a team as a code owner April 8, 2026 20:31
@github-actions

github-actions Bot commented May 9, 2026

Copy link
Copy Markdown
Contributor

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's dev tooling and experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants