Skip to content

Commit 1841e0c

Browse files
committed
E2E speedup (Step 6a): data-app-ready signal + Go fixture-parse fix
Replace the static 100 ms cushion in `ensureAppReady`'s focus-attach with a deterministic `data-app-ready` attribute set on `.dual-pane-explorer` at the end of `+page.svelte`'s onMount (after `setupTauriEventListeners()` and a `tick()` flush). Tests now gate on this attribute *before* the click+focus block, so onMount has fully completed by the time keys get dispatched. Also harden the Go check runner's fixture-path parser: it was taking the last line of `npx tsx`'s stdout, which npm's "new version available" notice breaks 100% of the time once it appears. Now scans every line for one starting with `/tmp/cmdr-e2e-`. Strict improvement over the pre-Step-6a state: - Before: 13 failures on a back-to-back validation, cascading ECONNREFUSED after a mid-suite Tauri crash. - After: 4 failures on a clean run, all in the parallel-load-induced keystroke-dispatch family (Cmd+F, F2, F-key dialogs, occasional activeElement timeout). Different tests fail each run — variance, not a deterministic regression. Remaining flakes deferred — see "After Step 6a" in docs/notes/speed-up-e2e-tests.md.
1 parent 140e192 commit 1841e0c

5 files changed

Lines changed: 107 additions & 14 deletions

File tree

apps/desktop/src/lib/file-explorer/pane/DualPaneExplorer.svelte

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2342,6 +2342,7 @@
23422342
tabindex="0"
23432343
role="application"
23442344
aria-label="File explorer"
2345+
data-app-ready="false"
23452346
>
23462347
{#if initialized}
23472348
<!-- eslint-disable-next-line @typescript-eslint/no-confusing-void-expression -- Svelte {@render} syntax -->

apps/desktop/src/routes/(main)/+page.svelte

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<script lang="ts">
2-
import { onMount, onDestroy } from 'svelte'
2+
import { onMount, onDestroy, tick } from 'svelte'
33
import DualPaneExplorer from '$lib/file-explorer/pane/DualPaneExplorer.svelte'
44
import FunctionKeyBar from '$lib/file-explorer/pane/FunctionKeyBar.svelte'
55
import FullDiskAccessPrompt from '$lib/onboarding/FullDiskAccessPrompt.svelte'
@@ -442,6 +442,25 @@
442442
443443
// Set up Tauri event listeners (extracted to reduce complexity)
444444
await setupTauriEventListeners()
445+
446+
// Wait for Svelte to flush any pending DOM updates so DualPaneExplorer
447+
// (which renders when `showApp=true`) is in the DOM before we look for
448+
// it. Without this, the query below would miss the element on first
449+
// mount, and on remounts (e.g. navigating back from /settings) we'd
450+
// race the new render.
451+
await tick()
452+
453+
// Mark the explorer as ready for E2E tests. This is a deterministic
454+
// signal that replaces the previous static 100 ms cushion in
455+
// `ensureAppReady`. By the time we set this, both the document-level
456+
// `keydown` listener and all MCP / dialog listeners are wired up, so
457+
// F-keys dispatched immediately after will reach their handlers.
458+
// The element is absent when showApp=false (FDA prompt path), but
459+
// E2E fixtures always grant FDA, so it will be there in tests.
460+
const explorer = document.querySelector('.dual-pane-explorer')
461+
if (explorer instanceof HTMLElement) {
462+
explorer.dataset.appReady = 'true'
463+
}
445464
})
446465
447466
/**

apps/desktop/test/e2e-playwright/helpers.ts

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,13 @@ export async function ensureAppReady(
192192
)
193193
}
194194

195+
// Wait for the deterministic `data-app-ready` signal set at the end of
196+
// `+page.svelte`'s onMount (after the keydown listener and all MCP / dialog
197+
// listeners are wired). This is the GATE — once it's true, onMount has
198+
// finished and the subsequent click+focus won't race against handler
199+
// attachment or focus theft from late-mounting components.
200+
await tauriPage.waitForFunction("document.querySelector('.dual-pane-explorer')?.dataset.appReady === 'true'", 10000)
201+
195202
// Click on a file entry in the left pane to ensure focus, then focus the
196203
// explorer container so keyboard events reach the handler.
197204
await tauriPage.evaluate(`(function() {
@@ -204,18 +211,13 @@ export async function ensureAppReady(
204211
// Wait until a file entry has the cursor (focus confirmed)
205212
await tauriPage.waitForSelector('.file-pane .file-entry.is-under-cursor', 3000)
206213

207-
// Confirm focus actually landed inside the explorer so keydown handlers
208-
// (both the container-level handler and the document-level shortcut dispatch
209-
// attached in +page.svelte's onMount) reach our F-keys. On back-to-back runs
210-
// the file-entry cursor can render before +page.svelte's onMount finishes
211-
// wiring `document.addEventListener('keydown', ...)`, leading to F5/F6/F8/Delete
212-
// being dropped. Poll for activeElement inside the explorer, then a tiny
213-
// margin to absorb the async listener attach.
214+
// Confirm focus actually landed inside the explorer so the container-level
215+
// keydown handler reaches keys like Tab and ArrowDown. (Document-level F-key
216+
// dispatch doesn't depend on focus, but cursor-driven tests do.)
214217
await tauriPage.waitForFunction(
215218
"document.activeElement && document.activeElement.closest('.dual-pane-explorer') !== null",
216219
3000,
217220
)
218-
await sleep(100)
219221
}
220222

221223
// ── DOM query helpers ────────────────────────────────────────────────────────

docs/notes/speed-up-e2e-tests.md

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -606,3 +606,71 @@ No fork, no symlink trick, no per-cwd hack.
606606
- Three Tauri windows pop up on macOS during the run. Cosmetic but very visible. Not worth chasing a "headless macOS
607607
Tauri" workaround for the checker — the test takes 2-3 minutes total.
608608
- `./scripts/check.sh` (fast) is green after the patch.
609+
610+
## After Step 6a (data-app-ready signal + Go fixture-parse robustness)
611+
612+
Step 6's parallel sharding amplified two pre-existing weaknesses:
613+
614+
1. **`ensureAppReady` focus race.** The static `sleep(100)` cushion after focus-attach in `ensureAppReady` was a margin
615+
to absorb the async `document.addEventListener('keydown', ...)` attach inside `+page.svelte`'s `onMount`. Under
616+
parallel load, 100 ms is occasionally insufficient — F-key dispatches lose the handler.
617+
2. **Go fixture-parse fragility.** `createE2EFixtures` parsed the fixture directory path from the last line of
618+
`npx tsx -e ...`'s stdout. npm's "new version available" notice gets appended after our `console.log` output and
619+
broke the parser, failing 100% of subsequent runs until npm finished its check.
620+
621+
### Fixes applied
622+
623+
**App-side (`+page.svelte`, `DualPaneExplorer.svelte`)**:
624+
625+
- `.dual-pane-explorer` now carries a `data-app-ready` attribute. Initial value: `"false"`.
626+
- At the end of `+page.svelte`'s `onMount`, after `setupTauriEventListeners()` finishes and Svelte has flushed pending
627+
DOM updates (`await tick()`), set `data-app-ready="true"`. This is the deterministic signal that all keydown listeners
628+
and MCP / dialog listeners are wired.
629+
- The element is absent when `showApp=false` (FDA prompt path), but E2E fixtures always grant FDA so the element will be
630+
present in tests.
631+
632+
**Test helpers (`helpers.ts`)**:
633+
634+
- In `ensureAppReady`, added a `waitForFunction("...?.dataset.appReady === 'true'", 10000)` _before_ the click+focus
635+
block. This GATES all subsequent focus/cursor work on onMount having fully completed.
636+
- The static `sleep(100)` after the activeElement check is gone.
637+
- The remaining `waitForFunction` on `activeElement.closest('.dual-pane-explorer')` stays — it confirms the click+focus
638+
landed.
639+
640+
**Go check runner (`scripts/check/checks/desktop-svelte-e2e-playwright.go`)**:
641+
642+
- `createE2EFixtures` now scans every line of the tsx output for one starting with `/tmp/cmdr-e2e-` instead of taking
643+
the last line blindly. npm notices and similar tail-end noise no longer break the parse.
644+
645+
### Result
646+
647+
- Before Step 6a: with the 100 ms cushion, the suite was on a thin margin under parallel sharding load — 13 failures on
648+
a back-to-back validation, with cascading ECONNREFUSED after a mid-suite Tauri crash.
649+
- After Step 6a: **4 failures** on a clean run, all in the same family — a few `waitForSelector`/`waitForFunction`
650+
timeouts on keystroke-driven dialogs (Cmd+F search, F2 rename, F-key transfer) and a couple of `activeElement` checks
651+
in `file-watching` / `indexing` specs. Different tests fail on each run, confirming this is parallel-load-induced
652+
variance, not a deterministic regression.
653+
- Strict improvement over the pre-Step-6a state, but not yet zero-flakes.
654+
655+
### Remaining flakes (deferred)
656+
657+
Under parallel-shard load:
658+
659+
- **Keystroke dispatches occasionally miss their handler** — Cmd+F (search overlay), F2 (rename input), F5/F6/F7/F8
660+
(transfer/delete/mkdir dialogs). After `data-app-ready` is `true`, onMount is fully done, so the keydown listener
661+
exists. Most likely the dispatched `KeyboardEvent` runs before Svelte has reattached the corresponding handler
662+
bindings after a route change (e.g., back from `/settings`), or focus has drifted to an element whose `keydown`
663+
handler stops the dispatch with `preventDefault`. Worth chasing in a follow-up — candidates:
664+
- Have `data-app-ready` also flip back to `"false"` on route change so we wait for the new mount cleanly.
665+
- Replace synthesized `KeyboardEvent`s with `dispatchMenuCommand()` (already a helper) where the test only cares about
666+
the resulting dialog, not the keyboard pathway.
667+
- **`activeElement.closest('.dual-pane-explorer')` occasional timeout** — happens when the click+focus evaluate runs
668+
while a late-mounting child (toast, AI notification) steals focus. Could be fixed by re-issuing the focus after
669+
waitForSelector lands, or by polling `activeElement` over a slightly larger window.
670+
671+
These are deferred — diminishing returns vs. the speedup work. Tracking here for post-Step-6 follow-up.
672+
673+
### Wall-clock
674+
675+
Clean run: **3m 48s** checker total (up from Step 6's 2m 48s on a fully warm cache). The Rust build is cold here because
676+
we've been iterating; with a warm cache it returns to the Step 6 baseline.

scripts/check/checks/desktop-svelte-e2e-playwright.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -353,11 +353,14 @@ func createE2EFixtures(desktopDir string) (string, error) {
353353
return "", fmt.Errorf("failed to create fixtures: %w\n%s", err, indentOutput(output))
354354
}
355355

356-
// The script is `console.log(createFixtures())` so the path is the last line.
357-
lines := strings.Split(strings.TrimSpace(output), "\n")
358-
lastLine := strings.TrimSpace(lines[len(lines)-1])
359-
if strings.HasPrefix(lastLine, "/") {
360-
return lastLine, nil
356+
// The script is `console.log(createFixtures())` so the path is on its own
357+
// line. Scan all lines for one starting with "/" — npm may inject update
358+
// notices after our output.
359+
for line := range strings.SplitSeq(strings.TrimSpace(output), "\n") {
360+
trimmed := strings.TrimSpace(line)
361+
if strings.HasPrefix(trimmed, "/tmp/cmdr-e2e-") {
362+
return trimmed, nil
363+
}
361364
}
362365
return "", fmt.Errorf("could not parse fixture path from output:\n%s", indentOutput(output))
363366
}

0 commit comments

Comments
 (0)