fix(grafana): show descriptive error when dashboard image fails to load

[nicomiguelino]

User description

Closes #787

Summary

• Return a FetchResult discriminated union from fetchAndRenderDashboard instead of a plain boolean, carrying the HTTP status and error message
• Surface the status/error detail in the thrown error, and note that Screenly Anywhere is not currently supported
• Add unit tests covering the success, HTTP error, and network error paths

---

PR Type

Bug fix, Tests

---

Description

• Return structured Grafana fetch failure details

• Show descriptive dashboard image load errors

• Revoke old blob URLs on refresh

• Cover success and failure test paths

---

Diagram Walkthrough

flowchart LR
  A["Grafana render request"] -- "returns" --> B["FetchResult with status/message"]
  B -- "failure surfaces" --> C["Descriptive error thrown in main.ts"]
  A -- "success sets" --> D["Blob URL on dashboard image"]
  D -- "before replacing" --> E["Previous blob URL revoked"]
  F["Unit and E2E tests"] -- "cover" --> B

Loading

File Walkthrough

	Relevant files
Tests	screenshots.spec.ts Refactor screenshot tests and error states                              edge-apps/grafana/e2e/screenshots.spec.ts Add a display_errors Screenly mock variant Extract shared runScreenshotTest setup helper Reuse OAuth and render route mocking Add screenshot cases for error overlays +81/-33  main.test.ts Expand render fetch unit test coverage                                      edge-apps/grafana/src/main.test.ts Import and exercise fetchAndRenderDashboard Add shared constants and fetch mock helper Restore mocked globals in afterEach Cover success, HTTP failure, network failure, and blob cleanup +148/-59
Bug fix	main.ts Surface detailed dashboard load failures                                  edge-apps/grafana/src/main.ts Use the structured result from fetchAndRenderDashboard Throw a descriptive error including fetch details +5/-3
Error handling	render.ts Return structured fetch results safely                                      edge-apps/grafana/src/render.ts Add the FetchResult discriminated union Return HTTP status and status text on failures Revoke the previous blob URL before replacement Return caught fetch errors as failure messages +20/-6

---

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 51a4008)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 787 - PR Code Verified Compliant requirements: When the Grafana edge app cannot fetch a dashboard photo/screenshot, it should provide a descriptive error instead of a generic failure. The behavior should address screenshot API failure scenarios. Requires further human verification: The descriptive error is actually surfaced in the debug-mode UI exactly as intended during a real screenshot API failure.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ No major issues detected

[Copilot]

Pull request overview

This PR improves error reporting for the Grafana edge-app when the dashboard image fetch/render fails, enabling more descriptive failures to be surfaced to users (and covered by unit tests).

Changes:

• Change fetchAndRenderDashboard to return a FetchResult discriminated union instead of boolean.
• Surface HTTP/network error detail in the error thrown during initial load.
• Add unit tests for success, HTTP failure, and network failure paths.

Reviewed changes

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

File	Description
edge-apps/grafana/src/render.ts	Introduces FetchResult and returns structured failure details from fetchAndRenderDashboard.
edge-apps/grafana/src/main.ts	Uses FetchResult to throw a more descriptive error when initial dashboard image load fails.
edge-apps/grafana/src/main.test.ts	Adds unit tests covering fetchAndRenderDashboard outcomes and refactors existing URL tests.

---

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

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 51a4008
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Always close browser contexts If any step in runScreenshotTest throws, the browser context is never closed and later screenshot tests can leak pages or fail unpredictably. Wrap the body in try/finally so context.close() always runs. edge-apps/grafana/e2e/screenshots.spec.ts [53-81] const screenshotsDir = getScreenshotsDir() const context = await browser.newContext({ viewport: { width, height } }) -const page = await context.newPage() -await setupClockMock(page) -await setupScreenlyJsMock(page, screenlyContent) +try { + const page = await context.newPage() -await page.route('**/oauth/access_token/', async (route) => { - await route.fulfill({ - status: 200, - contentType: 'application/json', - body: JSON.stringify({ - token: 'mock-service-access-token', - metadata: { domain: GRAFANA_DOMAIN }, - }), + await setupClockMock(page) + await setupScreenlyJsMock(page, screenlyContent) + + await page.route('**/oauth/access_token/', async (route) => { + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + token: 'mock-service-access-token', + metadata: { domain: GRAFANA_DOMAIN }, + }), + }) }) -}) -await page.route('**/render/d/**', mockRenderRoute) + await page.route('**/render/d/**', mockRenderRoute) -await page.goto('/') -await page.waitForLoadState('networkidle') + await page.goto('/') + await page.waitForLoadState('networkidle') -await page.screenshot({ - path: path.join(screenshotsDir, filename), - fullPage: false, -}) + await page.screenshot({ + path: path.join(screenshotsDir, filename), + fullPage: false, + }) +} finally { + await context.close() +} -await context.close() - Suggestion importance[1-10]: 7 __ Why: This is a correct and useful test-stability improvement: if any awaited step in runScreenshotTest throws, the manually created context may remain open. Wrapping the body in try/finally makes cleanup reliable without changing the intended test behavior.	Medium
Possible issue	Validate image responses Treating every 2xx response as a valid image can still report success when Grafana returns an HTML or JSON error page with status 200. Validate the response content-type before creating the blob so the caller can surface a real failure message instead of rendering a broken image. edge-apps/grafana/src/render.ts [45-53] +const contentType = response.headers.get('content-type') ?? '' +if (!contentType.startsWith('image/')) { + const message = `Unexpected content type: ${contentType || 'unknown'}` + console.error( + `Failed to fetch dashboard image from ${imageUrl}: ${message}`, + ) + return { success: false, message } +} + const blob = await response.blob() const objectUrl = URL.createObjectURL(blob) if (imgElement.src.startsWith('blob:')) { URL.revokeObjectURL(imgElement.src) } imgElement.setAttribute('src', objectUrl) return { success: true } Suggestion importance[1-10]: 6 __ Why: This is a valid robustness improvement for fetchAndRenderDashboard: a 200 response with non-image content could currently be treated as success and assigned to imgElement. It is not clearly a critical bug in the PR, but checking response.headers would improve failure reporting and avoid rendering invalid content.	Low

---

Previous suggestions

Suggestions up to commit ab95ed3

Category	Suggestion	Impact
Possible issue	Reject non-image responses fetchAndRenderDashboard currently reports success for any 2xx response, even when the proxy returns HTML/JSON error content instead of an image. That leaves main.ts thinking the dashboard loaded successfully while the browser shows a broken image. Validate the response MIME type before returning a successful result. edge-apps/grafana/src/render.ts [45-49] const blob = await response.blob() +const contentType = blob.type || response.headers.get('content-type') || '' + +if (!contentType.startsWith('image/')) { + return { + success: false, + message: `Expected an image response but received ${contentType || 'unknown content type'}`, + } +} + const objectUrl = URL.createObjectURL(blob) imgElement.setAttribute('src', objectUrl) return { success: true } Suggestion importance[1-10]: 7 __ Why: This is a valid robustness improvement because fetchAndRenderDashboard currently treats any successful HTTP response as a rendered dashboard, even if the proxy returns HTML or JSON instead of an image. Checking the response MIME type would make the new FetchResult contract in render.ts more accurate and prevent false positives in main.ts.	Medium
	Wait for image load Setting src does not guarantee that the browser could actually decode and render the image. If the blob is corrupted, this function still returns success and the descriptive error path in main.ts is skipped. Wait for the image load/error event before resolving the FetchResult. edge-apps/grafana/src/render.ts [48-49] -imgElement.setAttribute('src', objectUrl) -return { success: true } +const loadResult = await new Promise<FetchResult>((resolve) => { + imgElement.onload = () => resolve({ success: true }) + imgElement.onerror = () => + resolve({ + success: false, + message: 'The dashboard image could not be rendered by the browser', + }) + imgElement.setAttribute('src', objectUrl) +}) + +return loadResult + Suggestion importance[1-10]: 6 __ Why: This suggestion is technically sound and improves correctness by ensuring fetchAndRenderDashboard only reports success after the browser actually loads the image into imgElement. Its impact is moderate because corrupted image data is a narrower failure mode than HTTP-level errors, but it aligns well with the new structured error handling.	Low

[github-actions]

Persistent review updated to latest commit 51a4008