Skip to content

Commit 1e43a6f

Browse files
BYKBYK
authored
test(e2e): fix flaky attachments UI tests with web-first wait (#1327)
## Summary Fixes the flaky **E2E UI Tests** failure on `attachments.e2e.test.ts` (the "should display attachments from fixture directory" test timed out with an empty body in PR #1326's CI run, passing only on re-run). ## Root cause Every test in the file asserted `body.textContent() !== ""` by reading `textContent()` **once**, right after the `<body>` element attached. In an SPA the body shell exists before React's first paint, so this races the initial render with **no auto-retry**. The passing tests incidentally got settle time (the `sendTestEnvelope` fixture sleeps 500ms, or `waitForTimeout(1000)`); the failing test sends no envelope, so it had no slack and flaked under CI load. ## Fix - Add a web-first `waitForAppReady(page)` helper in `fixtures.ts` that asserts the navigation sidebar (`nav[aria-label="Navigation"]`) is visible. The sidebar (`TelemetrySidebar`) renders **unconditionally** once the app mounts, independent of telemetry data, so Playwright's auto-retrying `toBeVisible()` eliminates the render race. - Replace the fragile one-shot `body.textContent()` checks throughout `attachments.e2e.test.ts` with `waitForAppReady(page)`. This is also a **stronger** assertion: if the UI crashed into the `ErrorBoundary` fallback (which renders no nav), these tests now correctly fail instead of passing on non-empty body text. ## Verification - Lint (biome) + TypeScript check pass on the changed files. - ⚠️ I could not run the Playwright UI suite locally — this environment couldn't download the chromium browser (CDN download stalled/retried repeatedly). CI has browsers preinstalled and is the exact environment where the flake occurred; I'll confirm stability by running the E2E UI job here (including re-runs).
1 parent a73242b commit 1e43a6f

3 files changed

Lines changed: 57 additions & 48 deletions

File tree

‎.lore.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,12 @@
1010
### Gotcha
1111

1212
<!-- lore:019ed0b6-c2c6-7197-9cec-2026914ec51d -->
13-
* **esbuild override cap in getsentry/spotlight — resolved at 0.28.1**: The pnpm override \`"esbuild": ">=0.25.0 <0.28.0"\` was a deliberate cap to avoid esbuild#4436 (erroring on destructuring for old targets). Trap: bumping to \`>=0.28.1\` looks risky because 0.28.x retained that behavior. Fix: the regression did NOT reappear in practice — website build succeeded without adding \`target: "es2020"\`. Override is now \`"esbuild": ">=0.28.1"\`, resolving Dependabot alerts #279 and #280.
13+
* **esbuild override cap in getsentry/spotlight — resolved at 0.28.1**: The pnpm override \`"esbuild": ">=0.25.0 <0.28.0"\` was a deliberate cap to avoid esbuild#4436 (erroring on destructuring for old targets). Trap: bumping to \`>=0.28.1\` looks risky because 0.28.x retained that behavior. Fix: the regression did NOT reappear in practice — website build succeeded without adding \`target: "es2020"\`. Override is now \`"esbuild": ">=0.28.1"\`, resolving Dependabot alerts #279 and #280. These alerts were merged via PR #1323 on 2026-06-16 and will auto-close without further action.
1414

1515
<!-- lore:019e2b7e-99e1-7402-8c4f-9699919d7e69 -->
1616
* **plist override breaks electron-builder osx-sign**: Forcing \`plist>=3.1.1\` via pnpm overrides bumps it to v5.x, which breaks \`@electron/osx-sign@1.0.5\` (used by \`electron-builder@24.13.3\`) due to incompatible CJS \`require()\` and new \`exports\` map. Fix: remove the \`plist\` override and instead override \`@xmldom/xmldom\` directly to \`>=0.8.13\` (first patched 0.8.x version). This keeps \`plist@3.1.0\` for osx-sign compatibility while eliminating the \`@xmldom/xmldom\` vulnerability.
1717

1818
### Pattern
1919

2020
<!-- lore:019ed0b6-c2ce-7193-9376-5e711e5d5441 -->
21-
* **Security dep-bump workflow in getsentry/spotlight**: Pattern for resolving Dependabot alerts: (1) fetch alerts via \`gh api /repos/{owner}/{repo}/dependabot/alerts\`; (2) plan fix in \`.opencode/plans/\`; (3) bump pnpm override in root \`package.json\`; (4) run \`pnpm install\`, verify lockfile, run full \`pnpm build\` + \`vitest run\`; (5) create branch \`security/deps-\<pkg>-\<version>\` off main, commit, push, open PR. Untracked \`.opencode/\` and \`packages/website/content.config.ts\` are intentionally excluded from security commits.
21+
* **Security dep-bump workflow in getsentry/spotlight**: Pattern for resolving Dependabot alerts in getsentry/spotlight: (1) fetch alerts via \`gh api /repos/{owner}/{repo}/dependabot/alerts\`; (2) plan fix in \`.opencode/plans/\`; (3) bump pnpm overrides in root \`package.json\` AND bump direct deps in affected \`packages/\*/package.json\` where needed (e.g. astro direct dep); (4) run \`pnpm install\`, verify lockfile, run full \`pnpm build\` + \`vitest run\`; (5) create branch \`security/deps-\<descriptor>\` off main, commit, push, open PR. Vite major-version overrides must be bounded (e.g. \`<8\`) to prevent accidental major jumps. Untracked \`.opencode/\` and \`packages/website/content.config.ts\` are intentionally excluded from security commits. E2E UI test flakes are known — rerun before investigating.

‎packages/spotlight/tests/e2e/ui/attachments.e2e.test.ts‎

Lines changed: 41 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
1-
import { expect, test } from "./fixtures";
1+
import { test, waitForAppReady } from "./fixtures";
22

33
test.describe("Attachments Display UI Tests", () => {
44
test("should display envelope with screenshot", async ({ page, sidecar, sendTestEnvelope }) => {
55
await page.goto(sidecar.baseURL);
6+
await waitForAppReady(page);
7+
68
// Send envelope with screenshot (binary)
79
await sendTestEnvelope("envelope_with_screenshot.bin");
810

@@ -14,102 +16,98 @@ test.describe("Attachments Display UI Tests", () => {
1416
.click()
1517
.catch(() => {});
1618

17-
// Wait and verify some content is displayed
18-
const pageContent = page.locator("body");
19-
const text = await pageContent.textContent();
20-
expect(text).not.toBe("");
19+
// The app should still be rendered after interacting with it
20+
await waitForAppReady(page);
2121
});
2222

2323
test("should handle Flutter replay binary", async ({ page, sidecar, sendTestEnvelope }) => {
2424
await page.goto(sidecar.baseURL);
25+
await waitForAppReady(page);
26+
2527
// Send Flutter replay binary
2628
await sendTestEnvelope("envelope_flutter_replay.bin");
2729

28-
// Verify content is displayed
29-
const pageContent = page.locator("body");
30-
const text = await pageContent.textContent();
31-
expect(text).not.toBe("");
30+
// The UI should not crash on this payload
31+
await waitForAppReady(page);
3232
});
3333

3434
test("should handle browser JS profile", async ({ page, sidecar, sendTestEnvelope }) => {
3535
await page.goto(sidecar.baseURL);
36+
await waitForAppReady(page);
37+
3638
// Send browser JS profile binary
3739
await sendTestEnvelope("enveplope_browser_js_profile.bin");
3840

39-
// Verify content is displayed
40-
const pageContent = page.locator("body");
41-
const text = await pageContent.textContent();
42-
expect(text).not.toBe("");
41+
// The UI should not crash on this payload
42+
await waitForAppReady(page);
4343
});
4444

4545
test("should handle generic binary envelope", async ({ page, sidecar, sendTestEnvelope }) => {
4646
await page.goto(sidecar.baseURL);
47+
await waitForAppReady(page);
48+
4749
// Send generic binary envelope
4850
await sendTestEnvelope("envelope_binary.bin");
4951

50-
// Verify the UI doesn't crash and displays something
51-
const pageContent = page.locator("body");
52-
const text = await pageContent.textContent();
53-
expect(text).not.toBe("");
52+
// Verify the UI doesn't crash and is still rendered
53+
await waitForAppReady(page);
5454
});
5555

5656
test("should display attachments from fixture directory", async ({ page, sidecar }) => {
5757
await page.goto(sidecar.baseURL);
58-
// The test verifies that the UI can handle attachment data
59-
// Wait for page to be ready
60-
const pageContent = page.locator("body");
61-
await pageContent.waitFor({ timeout: 5000 });
6258

63-
const text = await pageContent.textContent();
64-
expect(text).not.toBe("");
59+
// The test verifies that the UI renders without any telemetry present
60+
await waitForAppReady(page);
6561
});
6662

6763
test("should handle empty payload", async ({ page, sidecar, sendTestEnvelope }) => {
6864
await page.goto(sidecar.baseURL);
65+
await waitForAppReady(page);
66+
6967
// Send empty payload envelope
7068
await sendTestEnvelope("envelope_empty_payload.txt");
7169

7270
// Should not crash
73-
const pageContent = page.locator("body");
74-
const text = await pageContent.textContent();
75-
expect(text).not.toBe("");
71+
await waitForAppReady(page);
7672
});
7773

7874
test("should handle empty envelope", async ({ page, sidecar, sendTestEnvelope }) => {
7975
await page.goto(sidecar.baseURL);
76+
await waitForAppReady(page);
77+
8078
// Send empty envelope
8179
await sendTestEnvelope("envelope_empty.txt");
8280

8381
// Should not crash
84-
const pageContent = page.locator("body");
85-
const text = await pageContent.textContent();
86-
expect(text).not.toBe("");
82+
await waitForAppReady(page);
8783
});
8884

8985
test("should display image attachments if present", async ({ page, sidecar, sendTestEnvelope }) => {
9086
await page.goto(sidecar.baseURL);
87+
await waitForAppReady(page);
88+
9189
// Send envelope with screenshot
9290
await sendTestEnvelope("envelope_with_screenshot.bin");
9391

94-
// Look for image elements
92+
// Look for image elements (best-effort, the payload may not surface one)
9593
const _hasImage = await page
9694
.locator('img, [role="img"]')
9795
.first()
9896
.isVisible()
9997
.catch(() => false);
10098

101-
// At minimum, page should have content
102-
const pageContent = page.locator("body");
103-
const text = await pageContent.textContent();
104-
expect(text).not.toBe("");
99+
// At minimum, the app should still be rendered
100+
await waitForAppReady(page);
105101
});
106102

107103
test("should provide download capability for attachments", async ({ page, sidecar, sendTestEnvelope }) => {
108104
await page.goto(sidecar.baseURL);
105+
await waitForAppReady(page);
106+
109107
// Send envelope with attachment
110108
await sendTestEnvelope("envelope_with_screenshot.bin");
111109

112-
// Look for download buttons or links
110+
// Look for download buttons or links (best-effort)
113111
const _hasDownloadLink = await Promise.race([
114112
page
115113
.locator('a[download], button[aria-label*="download"], [title*="download"]')
@@ -120,32 +118,30 @@ test.describe("Attachments Display UI Tests", () => {
120118
new Promise<boolean>(resolve => setTimeout(() => resolve(false), 2000)),
121119
]);
122120

123-
// At minimum, page should render
124-
const pageContent = page.locator("body");
125-
await pageContent.waitFor({ timeout: 5000 });
126-
expect(await pageContent.isVisible()).toBe(true);
121+
// At minimum, the app should still be rendered
122+
await waitForAppReady(page);
127123
});
128124

129125
test("should handle various attachment content types", async ({ page, sidecar, sendTestEnvelope }) => {
130126
await page.goto(sidecar.baseURL);
127+
await waitForAppReady(page);
128+
131129
// Send multiple envelopes with different content
132130
await sendTestEnvelope("envelope_javascript.txt");
133131
await sendTestEnvelope("envelope_python.txt");
134132

135133
// Should handle various types without crashing
136-
const pageContent = page.locator("body");
137-
const text = await pageContent.textContent();
138-
expect(text).not.toBe("");
134+
await waitForAppReady(page);
139135
});
140136

141137
test("should handle envelope with no length and EOF", async ({ page, sidecar, sendTestEnvelope }) => {
142138
await page.goto(sidecar.baseURL);
139+
await waitForAppReady(page);
140+
143141
// Send envelope with no length and EOF
144142
await sendTestEnvelope("envelope_no_len_w_eof.txt");
145143

146144
// Should not crash
147-
const pageContent = page.locator("body");
148-
const text = await pageContent.textContent();
149-
expect(text).not.toBe("");
145+
await waitForAppReady(page);
150146
});
151147
});

‎packages/spotlight/tests/e2e/ui/fixtures.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import fs from "node:fs/promises";
33
import http from "node:http";
44
import path from "node:path";
55
import { fileURLToPath } from "node:url";
6-
import { type Page, test as base } from "@playwright/test";
6+
import { type Page, test as base, expect } from "@playwright/test";
77
import { findFreePort, getFixturePath, killProcess, spawnProcess } from "../shared/utils";
88

99
const __dirname = path.dirname(fileURLToPath(import.meta.url));
@@ -120,6 +120,19 @@ async function sendEnvelopeToSidecar(
120120
});
121121
}
122122

123+
/**
124+
* Wait for the Spotlight UI to finish its initial render.
125+
*
126+
* The navigation sidebar (`nav[aria-label="Navigation"]`) is rendered
127+
* unconditionally as soon as the React app mounts, regardless of whether any
128+
* telemetry has been received. Asserting on it with a web-first (auto-retrying)
129+
* assertion is far more reliable than reading `body.textContent()` once, which
130+
* races React's first paint and previously made these tests flaky.
131+
*/
132+
export async function waitForAppReady(page: Page, timeout = 15000): Promise<void> {
133+
await expect(page.locator('nav[aria-label="Navigation"]')).toBeVisible({ timeout });
134+
}
135+
123136
/**
124137
* Wait for an event to appear in the UI
125138
*/

0 commit comments

Comments
 (0)