Skip to content

refactor: better cdn failure messaging. #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
knightedcodemonkey wants to merge 1 commit into from
Closed

refactor: better cdn failure messaging. #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions docs/next-steps.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,15 @@ Focused follow-up work for `@knighted/develop`.
- Explore authoring and running component-focused tests in-browser (for example, a Vitest-compatible flow) using CDN-delivered tooling.
- Define a lightweight test UX that supports writing tests, running them on demand, and displaying results in-app.

2. **CDN failure recovery UX**
- Detect transient CDN/module loading failures and surface a clear recovery action in-app.
- Add a user-triggered retry path (for example, Reload page / Force reload) when runtime bootstrap imports fail.
- Consider an optional automatic one-time retry before showing recovery controls, while avoiding infinite reload loops.

3. **Deterministic E2E lane in CI**
2. **Deterministic E2E lane in CI**
- Add an integration-style E2E path that uses locally served/pinned copies of CDN runtime dependencies for test execution, while keeping production runtime behavior unchanged.
- Keep the current true CDN-backed E2E path as a separate smoke check, but make the deterministic lane the required gate for pull requests.
- Run this deterministic E2E suite on **every pull request** in CI.
- Ensure the deterministic lane still exercises the same user-facing flows (render, typecheck, lint, diagnostics drawer/button states), only swapping the source of runtime artifacts.
- Suggested implementation prompt:
- "Add a deterministic E2E execution mode for `@knighted/develop` that serves pinned runtime artifacts locally (instead of live CDN fetches) and wire it into CI as a required check on every PR. Keep a separate lightweight CDN-smoke E2E check for real-network coverage. Validate with `npm run lint`, deterministic Playwright PR checks, and one CDN-smoke Playwright run."

4. **Issue #18 continuation (resume from Phase 3)**
3. **Issue #18 continuation (resume from Phase 3)**
- Current rollout status:
- Phase 0 complete: feature flag + scaffolding.
- Phase 1 complete: BYOT token flow, localStorage persistence, writable repo discovery/filtering.
Expand Down
22 changes: 22 additions & 0 deletions playwright/app.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,28 @@ test('BYOT controls stay hidden when feature flag is disabled', async ({ page })
await expect(page.locator('#ai-chat-drawer')).toBeHidden()
})

test('shows actionable status when core CDN dependency loading fails', async ({
page,
}) => {
await page.route('**/*', async route => {
const url = route.request().url()

if (url.includes('@knighted') || url.includes('%40knighted')) {
await route.abort()
return
}

await route.continue()
})
Comment on lines +195 to +204
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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()
},
)

Copilot uses AI. Check for mistakes.
await waitForAppReady(page)
await expect(page.locator('#status')).toHaveText(
'Error loading CDN dependency. Reload to retry.',
)
await expect(page.locator('#preview-host')).toContainText(
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
await expect(page.locator('#preview-host')).toContainText(
await expect(page.locator('#preview-host pre')).toContainText(

Copilot uses AI. Check for mistakes.
'Unable to load core runtime from CDN:',
)
})

test('BYOT controls render when feature flag is enabled by query param', async ({
page,
}) => {
Expand Down
14 changes: 12 additions & 2 deletions src/modules/render-runtime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,11 +815,21 @@ export const createRenderRuntimeController = ({
setStatus('Rendered', 'neutral')
setRenderedStatus()
} catch (error) {
setStatus('Error', 'error')
const errorMessage = error instanceof Error ? error.message : String(error)
const isCdnDependencyLoadFailure = errorMessage.startsWith(
'Unable to load core runtime from CDN:',
)

setStatus(
isCdnDependencyLoadFailure
? 'Error loading CDN dependency. Reload to retry.'
: 'Error',
Comment on lines +819 to +826
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
'error',
)
const target = getRenderTarget()
clearTarget(target)
const message = document.createElement('pre')
message.textContent = error instanceof Error ? error.message : String(error)
message.textContent = errorMessage
message.style.color = '#ff9aa2'
target.append(message)
} finally {
Expand Down
Loading