Skip to content

Commit b4629c6

Browse files
committed
fix(frontend): stop WebKit storage and engine failures from hanging the app
Follow-up to #7314. That PR fixed the IndexedDB blob rejection itself; this one fixes the ways the same failures surfaced as a permanent spinner, and adds the cross-browser CI signal that would have caught them on the pull request instead of six weeks later in a nightly run. Storage no longer hangs: - Blocked IndexedDB opens and deletes now time out with an actionable error instead of never settling, and the app sets `onversionchange` so an open tab yields its connection rather than blocking every other tab forever. - The in-flight open promise is registered before the first await, so concurrent boot-time callers share one connection. Only the first request receives `blocked`; the others were getting no events at all. - Transactions settle on abort. Read-modify-write moves to a single `updateRecord` helper that owns its transaction, guards it once, and resolves on commit - the previous two-promises-over-one-transaction shape left the write with no abort handler, which could hang output persistence silently. - The blob-value refusal is remembered from any write, not just the initial add: WebKit reports it per-operation, so an engine that accepted the add can still refuse the rewrite. WebKit engine gaps: - ReadableStream async iteration, which pdf.js uses for all text extraction. Without it Compare, read-aloud and the text editor were dead on Safari. - requestIdleCallback, installed once at the entry point instead of guarded at each call site. - convertToBlob silently returns PNG for a format it can't encode, so canvas output now probes what the engine really produced and picks the best lossy format it honours. CI: - A small @engine-capability suite runs on Chromium, Firefox and WebKit on every pull request. It asserts the primitives actually work (a counted comparison, a raster thumbnail, a byte round-trip through a reload) rather than that the UI rendered, which is how two total WebKit outages passed. - The cross-browser projects now share the stubbed project's viewport so a layout difference can't read as an engine outage.
1 parent 37a48aa commit b4629c6

26 files changed

Lines changed: 1352 additions & 234 deletions

.github/workflows/e2e-stubbed.yml

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@ name: Playwright E2E (stubbed)
33
# Reusable workflow called from build.yml. Backend-free Playwright suite —
44
# fast, no Spring Boot required. Runs against the `stubbed` project which
55
# mocks API responses in the browser.
6+
#
7+
# Chromium-only for speed, except the `@engine-capability` specs, which run on
8+
# all three engines here - nightly-only let two WebKit outages sit on main.
69
on:
710
workflow_call:
811

@@ -27,13 +30,29 @@ jobs:
2730
cache-dependency-path: frontend/package-lock.json
2831
- name: Install Task
2932
uses: go-task/setup-task@01a4adf9db2d14c1de7a560f09170b6e0df736aa # v2.1.0
30-
- name: Install Playwright (chromium only)
31-
run: task e2e:install -- chromium
33+
- name: Install Playwright (chromium, firefox, webkit)
34+
run: task e2e:install -- chromium firefox webkit
3235
- name: Build frontend (production bundle for vite preview)
3336
env:
3437
VITE_BUILD_FOR_PREVIEW: "1"
3538
run: task frontend:build
39+
- name: Run engine-capability smoke tests (chromium, firefox, webkit)
40+
# First, so "this engine can't read a PDF" lands at the top of the log.
41+
# Own JSON path: the Chromium run below would overwrite it.
42+
env:
43+
PLAYWRIGHT_JSON_OUTPUT_FILE: ${{ github.workspace }}/frontend/playwright-report/capability-results.json
44+
run: task e2e:capabilities
45+
- name: Upload engine-capability Playwright report
46+
# Uploaded here because the Chromium run below writes the same directory.
47+
if: always()
48+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
49+
with:
50+
name: playwright-report-capabilities-${{ github.run_id }}
51+
path: frontend/playwright-report/
52+
retention-days: 7
3653
- name: Run stubbed E2E tests (chromium)
54+
# Runs even if the smoke tests failed, so a PR gets both signals at once.
55+
if: ${{ !cancelled() }}
3756
env:
3857
PLAYWRIGHT_JSON_OUTPUT_FILE: ${{ github.workspace }}/frontend/playwright-report/results.json
3958
run: task e2e:stubbed -- --workers=3
@@ -43,9 +62,10 @@ jobs:
4362
# ::warning:: annotations + a job summary; never fails the job.
4463
if: always()
4564
working-directory: frontend
46-
run: npx tsx editor/scripts/report-flaky-tests.mts "$PLAYWRIGHT_JSON_OUTPUT_FILE"
47-
env:
48-
PLAYWRIGHT_JSON_OUTPUT_FILE: ${{ github.workspace }}/frontend/playwright-report/results.json
65+
run: >-
66+
npx tsx editor/scripts/report-flaky-tests.mts
67+
"${{ github.workspace }}/frontend/playwright-report/results.json"
68+
"${{ github.workspace }}/frontend/playwright-report/capability-results.json"
4969
- name: Upload Playwright report
5070
if: always()
5171
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1

.taskfiles/e2e.yml

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,24 @@ tasks:
1515
cmds:
1616
- npx playwright test --project=stubbed {{.CLI_ARGS}}
1717

18+
capabilities:
19+
desc: "Run the engine-capability smoke tests on Chromium, Firefox, and WebKit"
20+
summary: |
21+
Asserts the PDF engine primitives actually work on each engine - text
22+
extraction, page rasterisation, and an IndexedDB byte round-trip - rather
23+
than that the UI rendered. Small enough to run on every browser on every
24+
pull request, which is the point.
25+
26+
Pass extra Playwright flags via -- :
27+
task e2e:capabilities -- --headed
28+
dir: frontend/editor
29+
deps: [ ':frontend:prepare' ]
30+
cmds:
31+
- >-
32+
npx playwright test
33+
--project=stubbed --project=stubbed-firefox --project=stubbed-webkit
34+
--grep @engine-capability {{.CLI_ARGS}}
35+
1836
live:
1937
desc: "Run live E2E tests"
2038
summary: |

frontend/editor/playwright.config.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,12 @@ import { defineConfig, devices } from "@playwright/test";
1717
*
1818
* @see https://playwright.dev/docs/test-configuration
1919
*/
20+
/** Shared by every stubbed project so a spec sees one layout on all engines. */
21+
const STUBBED_VIEWPORT = { width: 1920, height: 1080 };
22+
2023
const chromiumViewport = {
2124
...devices["Desktop Chrome"],
22-
viewport: { width: 1920, height: 1080 },
25+
viewport: STUBBED_VIEWPORT,
2326
};
2427

2528
export default defineConfig({
@@ -93,16 +96,17 @@ export default defineConfig({
9396
},
9497
},
9598

96-
// Cross-browser coverage for the stubbed suite (opt-in locally)
99+
// Cross-browser coverage for the stubbed suite. Same viewport as `stubbed`,
100+
// or a layout difference here reads as an engine outage.
97101
{
98102
name: "stubbed-firefox",
99103
testDir: "./src/core/tests/stubbed",
100-
use: { ...devices["Desktop Firefox"] },
104+
use: { ...devices["Desktop Firefox"], viewport: STUBBED_VIEWPORT },
101105
},
102106
{
103107
name: "stubbed-webkit",
104108
testDir: "./src/core/tests/stubbed",
105-
use: { ...devices["Desktop Safari"] },
109+
use: { ...devices["Desktop Safari"], viewport: STUBBED_VIEWPORT },
106110
},
107111
],
108112

frontend/editor/src/core/components/shared/FileSelectorPicker.tsx

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -201,13 +201,27 @@ export function FileSelectorPicker({
201201
setSortDir(lsGet(LS_SORT_DIR, "desc", ["asc", "desc"]));
202202
}, [isOpen]);
203203

204-
// Load saved files when the saved tab is active and the picker is open
204+
// Load saved files when the saved tab is active and the picker is open.
205+
// Cancellable: storage reads can be slow or reject, and outlive the popover.
205206
useEffect(() => {
206207
if (activeTab !== "saved" || !isOpen) return;
208+
let cancelled = false;
207209
setSavedLoading(true);
208210
loadRecentFiles()
209-
.then(setSavedStubs)
210-
.finally(() => setSavedLoading(false));
211+
.then((stubs) => {
212+
if (!cancelled) setSavedStubs(stubs);
213+
})
214+
.catch((error: unknown) => {
215+
if (!cancelled) setSavedStubs([]);
216+
console.warn("Failed to load saved files for the picker:", error);
217+
})
218+
.finally(() => {
219+
if (!cancelled) setSavedLoading(false);
220+
});
221+
return () => {
222+
cancelled = true;
223+
setSavedLoading(false);
224+
};
211225
}, [activeTab, isOpen, loadRecentFiles]);
212226

213227
const workbenchIdSet = useMemo(
@@ -541,7 +555,9 @@ export function FileSelectorPicker({
541555
</div>
542556

543557
<ScrollArea h={260} className={styles.list}>
544-
{savedLoading ? (
558+
{/* Workbench renders from memory, so a stuck storage read must not
559+
spin it too. */}
560+
{savedLoading && activeTab === "saved" ? (
545561
<div className={styles.emptyState}>
546562
<Loader size="sm" />
547563
</div>

frontend/editor/src/core/components/shared/FileSidebar.tsx

Lines changed: 38 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -306,32 +306,45 @@ const FileSidebar = forwardRef<HTMLDivElement, FileSidebarProps>(
306306
const storageEnabled = config?.storageEnabled === true && !isAnonymous;
307307

308308
const refreshStubs = useCallback(async () => {
309-
// Leaf files from IDB - same source as the file selection modal.
310-
const stubs = await indexedDB.loadLeafMetadata();
311-
const idbIds = new Set(stubs.map((s) => s.id as string));
312-
313-
// Also include workbench files not yet flushed to IDB.
314-
const pendingStubs = state.files.ids
315-
.map((id) => state.files.byId[id])
316-
.filter(
317-
(stub): stub is NonNullable<typeof stub> =>
318-
!!stub && stub.isLeaf !== false && !idbIds.has(stub.id as string),
319-
);
309+
// `stubsLoaded` gates the spinner, so the `finally` below must set it on
310+
// every path - callers never await this, so a rejection goes nowhere.
311+
let stubs: StirlingFileStub[] = [];
312+
try {
313+
// Leaf files from IDB - same source as the file selection modal.
314+
stubs = await indexedDB.loadLeafMetadata();
315+
} catch (error) {
316+
// Carry on with the in-memory workbench files: an unreadable library
317+
// should cost the user their history, not the file they're working on.
318+
console.error("Failed to read the file library from storage:", error);
319+
}
320320

321-
const allStubs = [...stubs, ...pendingStubs];
322-
// A version swap briefly lists both the old leaf (IDB) and its replacement (workbench); two stubs for one lineage collide on the row key and corrupt React reconciliation, so drop any stub another names as its parent.
323-
const superseded = new Set(
324-
allStubs.map((s) => s.parentFileId as string | undefined),
325-
);
326-
const currentStubs = allStubs.filter(
327-
(s) => !superseded.has(s.id as string),
328-
);
329-
setAllFileStubs(
330-
currentStubs.sort(
331-
(a, b) => (b.lastModified ?? 0) - (a.lastModified ?? 0),
332-
),
333-
);
334-
setStubsLoaded(true);
321+
try {
322+
const idbIds = new Set(stubs.map((s) => s.id as string));
323+
324+
// Also include workbench files not yet flushed to IDB.
325+
const pendingStubs = state.files.ids
326+
.map((id) => state.files.byId[id])
327+
.filter(
328+
(stub): stub is NonNullable<typeof stub> =>
329+
!!stub && stub.isLeaf !== false && !idbIds.has(stub.id as string),
330+
);
331+
332+
const allStubs = [...stubs, ...pendingStubs];
333+
// A version swap briefly lists both the old leaf (IDB) and its replacement (workbench); two stubs for one lineage collide on the row key and corrupt React reconciliation, so drop any stub another names as its parent.
334+
const superseded = new Set(
335+
allStubs.map((s) => s.parentFileId as string | undefined),
336+
);
337+
const currentStubs = allStubs.filter(
338+
(s) => !superseded.has(s.id as string),
339+
);
340+
setAllFileStubs(
341+
currentStubs.sort(
342+
(a, b) => (b.lastModified ?? 0) - (a.lastModified ?? 0),
343+
),
344+
);
345+
} finally {
346+
setStubsLoaded(true);
347+
}
335348
}, [indexedDB, state.files.ids, state.files.byId]);
336349

337350
// Refresh on mount, workbench changes, or external IndexedDB writes —

frontend/editor/src/core/services/fileStorage.blobFallback.test.ts

Lines changed: 54 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,22 +3,17 @@ import "fake-indexeddb/auto";
33
import { expectConsole } from "@app/tests/failOnConsole";
44

55
/**
6-
* Regression test for the WebKit nightly breakage introduced with the
7-
* large-file OOM fix (#7175): `storeStirlingFile` began putting the `File`
8-
* itself into IndexedDB (persisted by reference, so multi-GB uploads never
9-
* materialize in JS memory). WebKit refuses blob values whenever it can't write
10-
* the blob's backing file and rejects the request with `UnknownError: Error
11-
* preparing Blob/File data to be stored in object store`, so on WebKit every
12-
* upload silently failed to persist: files vanished on navigation, Compare
13-
* slots never filled, and the classification backfill had no bytes to read.
6+
* WebKit refuses blob values when it can't write the blob's backing file, so
7+
* every upload silently failed to persist after #7175. Retried as a copy now.
148
*
15-
* The service now retries such a rejection with an ArrayBuffer copy and stops
16-
* offering blobs for the rest of the session.
9+
* fake-indexeddb never returns a Blob from a read, so round-trips can't be
10+
* modelled here - `engine-capabilities.spec.ts` covers those on a real engine.
1711
*/
1812

1913
const nativeAdd = IDBObjectStore.prototype.add;
14+
const nativePut = IDBObjectStore.prototype.put;
2015

21-
/** What each `add` attempt carried in `data` — the blob path or the copy path. */
16+
/** What each `add` attempt carried in `data`: blob path or copy path. */
2217
let attempts: Array<"blob" | "copy"> = [];
2318

2419
/** An IDBRequest that fails asynchronously, the way WebKit rejects blob puts. */
@@ -32,10 +27,7 @@ class FailingRequest extends EventTarget {
3227
}
3328
}
3429

35-
/**
36-
* Record every add attempt, optionally failing the blob-valued ones the way an
37-
* engine without blob storage does.
38-
*/
30+
/** Record every add, optionally failing the blob-valued ones. */
3931
function instrumentAdd(options: { rejectBlobs: boolean }) {
4032
IDBObjectStore.prototype.add = function (
4133
this: IDBObjectStore,
@@ -58,11 +50,8 @@ function instrumentAdd(options: { rejectBlobs: boolean }) {
5850
} as typeof IDBObjectStore.prototype.add;
5951
}
6052

61-
/**
62-
* A fresh service per test: whether the engine accepts blobs is remembered for
63-
* the process lifetime by design, so tests must not inherit that decision from
64-
* each other.
65-
*/
53+
/** A fresh service per test: the blob decision is remembered by design, so
54+
* tests must not inherit it from each other. */
6655
async function freshFileStorage() {
6756
vi.resetModules();
6857
const [{ fileStorage }, { createStirlingFile, createNewStirlingFileStub }] =
@@ -90,6 +79,49 @@ beforeEach(() => {
9079

9180
afterEach(() => {
9281
IDBObjectStore.prototype.add = nativeAdd;
82+
IDBObjectStore.prototype.put = nativePut;
83+
});
84+
85+
/** Abort the transaction the moment a write is issued over it. */
86+
function abortOnPut() {
87+
IDBObjectStore.prototype.put = function (this: IDBObjectStore) {
88+
const request = new FailingRequest(
89+
new DOMException("transaction aborted", "AbortError"),
90+
) as unknown as IDBRequest<IDBValidKey>;
91+
this.transaction.abort();
92+
return request;
93+
} as typeof IDBObjectStore.prototype.put;
94+
}
95+
96+
describe("read-modify-write — a refused rewrite must not hang or vanish", () => {
97+
/** The abort guard used to sit on the read promise, leaving the write with a
98+
* dead reject - and `.catch` can't rescue a promise that never settles. */
99+
test("settles instead of hanging when the write transaction aborts", async () => {
100+
expectConsole.error(/Failed to mark file as processed/);
101+
const { fileStorage, store } = await freshFileStorage();
102+
instrumentAdd({ rejectBlobs: false });
103+
const id = await store("aborts.pdf");
104+
105+
abortOnPut();
106+
107+
// Before the fix this never settled and the test timed out.
108+
await expect(fileStorage.markFileAsProcessed(id)).resolves.toBe(false);
109+
});
110+
111+
/** The copy-and-retry recovery can't be exercised here: it needs a record that
112+
* reads back as a Blob, which fake-indexeddb never returns. */
113+
test("a metadata rewrite still commits, and reports commit not put", async () => {
114+
const { fileStorage, store } = await freshFileStorage();
115+
instrumentAdd({ rejectBlobs: false });
116+
const id = await store("rewrite.pdf");
117+
118+
await expect(fileStorage.markFileAsProcessed(id)).resolves.toBe(true);
119+
// Missing record: `false`, not a throw and not a claim of success.
120+
await expect(
121+
fileStorage.markFileAsProcessed("nope" as never),
122+
).resolves.toBe(false);
123+
expect((await fileStorage.getStirlingFile(id))?.name).toBe("rewrite.pdf");
124+
});
93125
});
94126

95127
describe("storeStirlingFile — blob-value fallback", () => {
@@ -113,8 +145,7 @@ describe("storeStirlingFile — blob-value fallback", () => {
113145
const id = await store("webkit.pdf");
114146

115147
expect(attempts).toEqual(["blob", "copy"]);
116-
// Readable back is what every downstream consumer depends on: rehydration
117-
// after navigation, thumbnails, the classification backfill.
148+
// Readable back is what rehydration, thumbnails and backfill depend on.
118149
expect((await fileStorage.getStirlingFile(id))?.name).toBe("webkit.pdf");
119150
});
120151

@@ -127,8 +158,7 @@ describe("storeStirlingFile — blob-value fallback", () => {
127158
attempts = [];
128159
const id = await store("second.pdf");
129160

130-
// Straight to the copy path — no repeated blob probe, and only the single
131-
// warning expected above.
161+
// Straight to the copy path, and only the one warning expected above.
132162
expect(attempts).toEqual(["copy"]);
133163
expect((await fileStorage.getStirlingFile(id))?.name).toBe("second.pdf");
134164
});

0 commit comments

Comments
 (0)