refactor: better cdn failure messaging.#28
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the runtime render error UX by surfacing a more actionable status message when a core runtime CDN import fails, and adds an E2E test to prevent regressions.
Changes:
- Detect core runtime CDN load failures during preview render and set a more actionable status message.
- Add a Playwright test that simulates failed
@knighted/*CDN loads and asserts the new status/error output. - Remove the “CDN failure recovery UX” item from
docs/next-steps.mdand renumber the remaining items.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/modules/render-runtime.js |
Adds error classification for core runtime CDN load failures and sets a more actionable status message. |
playwright/app.spec.ts |
Adds E2E coverage for the new actionable status when core CDN imports fail. |
docs/next-steps.md |
Updates the roadmap by removing the CDN recovery UX item and renumbering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isCdnDependencyLoadFailure = errorMessage.startsWith( | ||
| 'Unable to load core runtime from CDN:', | ||
| ) | ||
|
|
||
| setStatus( | ||
| isCdnDependencyLoadFailure | ||
| ? 'Error loading CDN dependency. Reload to retry.' | ||
| : 'Error', |
There was a problem hiding this comment.
isCdnDependencyLoadFailure is currently true only for core runtime CDN failures. Since this file can also throw Unable to load React runtime from CDN: ... (and other CDN-backed imports exist), the name is misleading and the status UX will remain generic for other CDN dependency failures. Either broaden the detection to cover the other Unable to load ... from CDN: cases, or rename the variable/message to explicitly reference core runtime loading.
| await page.route('**/*', async route => { | ||
| const url = route.request().url() | ||
|
|
||
| if (url.includes('@knighted') || url.includes('%40knighted')) { | ||
| await route.abort() | ||
| return | ||
| } | ||
|
|
||
| await route.continue() | ||
| }) |
There was a problem hiding this comment.
This test routes all requests via page.route('**/*', ...), which is heavier than necessary and can introduce flakiness by accidentally affecting unrelated resources. Consider narrowing the route pattern to only CDN requests for the core runtime packages (e.g., the esm.sh/unpkg/jspm URLs for @knighted/*) so navigation + local assets aren’t intercepted.
| await page.route('**/*', async route => { | |
| const url = route.request().url() | |
| if (url.includes('@knighted') || url.includes('%40knighted')) { | |
| await route.abort() | |
| return | |
| } | |
| await route.continue() | |
| }) | |
| await page.route( | |
| url => url.includes('@knighted') || url.includes('%40knighted'), | |
| async route => { | |
| await route.abort() | |
| }, | |
| ) |
| await expect(page.locator('#status')).toHaveText( | ||
| 'Error loading CDN dependency. Reload to retry.', | ||
| ) | ||
| await expect(page.locator('#preview-host')).toContainText( |
There was a problem hiding this comment.
For consistency with other error-state assertions in this suite (and to avoid any ShadowRoot/text-aggregation quirks), assert against the rendered error element itself (e.g., #preview-host pre) instead of #preview-host when checking for the runtime load failure message.
| await expect(page.locator('#preview-host')).toContainText( | |
| await expect(page.locator('#preview-host pre')).toContainText( |
No description provided.