diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index cd7c483d8..17957aa37 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -3,6 +3,40 @@ name: Deploy Docs on: push: branches: [main] + # The union of every input that can reach the published bytes: `docs/**` + # (the only tree VitePress reads -- docs/.vitepress/config.mts and + # docs/.vitepress/theme/** import nothing outside docs/) plus every input + # to the screenshot pipeline that tests/e2e/screenshots.e2e.ts drives. + # scripts/native/** is in the set because it decides which native binary + # the built app loads; a broken rebuild changes what the app renders, or + # whether it starts at all. + # + # This list IS the authoritative declaration of that set -- there is no + # other file to keep it in sync with. If you add an input the screenshots + # depend on, add it here too: a missing entry silently skips a run that + # was needed and leaves the published site stale. + # + # Deliberately a workflow-level filter, unlike build.yml:22-27 which uses a + # job-level dorny/paths-filter. That comment's hazard -- workflow-level + # `paths-ignore` leaving required status checks permanently pending -- does + # not apply here: docs.yml has no `pull_request` trigger and is not a + # required check. A workflow-level filter costs 0 s on a skip where a + # job-level filter still spins up a runner. `workflow_dispatch` below is the + # escape hatch if this filter ever wrongly skips a needed run. + paths: + - 'docs/**' + - 'src/**' + - 'resources/**' + - 'tests/e2e/screenshots.e2e.ts' + - 'tests/e2e/test-data/demo-case.json' + - 'playwright.config.ts' + - 'electron.vite.config.ts' + - 'tsconfig*.json' + - 'package.json' + - 'package-lock.json' + - '.nvmrc' + - 'scripts/native/**' + - '.github/workflows/docs.yml' workflow_dispatch: permissions: @@ -31,6 +65,10 @@ jobs: build-screenshots: name: Build & Screenshots runs-on: ubuntu-latest + # Baseline is 237 s. 20 minutes is ~5x headroom -- enough for a cold native + # cache (which adds a ~35 s compile) plus a slow runner, without letting a + # hung Electron launch burn a full 6-hour default timeout. + timeout-minutes: 20 steps: - name: Configure git line endings @@ -45,11 +83,6 @@ jobs: node-version-file: '.nvmrc' cache: 'npm' - - name: Install system dependencies - run: | - sudo apt-get update - sudo apt-get install -y libsqlite3-dev build-essential - - name: Allow Electron to use unprivileged user namespaces (Ubuntu 24.04) run: sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 @@ -65,11 +98,31 @@ jobs: # sibling with a different key. No restore-keys: a partial match is # rejected by manifestIsFresh() anyway, and the absence documents intent. - name: Restore native ABI cache + id: native-cache uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # actions/cache@v6.1.0 with: path: .cache/native key: native-${{ runner.os }}-${{ runner.arch }}-${{ steps.electron-ver.outputs.ver }}-${{ hashFiles('package-lock.json') }} + # libsqlite3-dev and build-essential exist only to compile + # better-sqlite3-multiple-ciphers. The ABI-keyed native cache restores a + # prebuilt binary, so on a hit nothing normally compiles and these + # packages are 9 s of dead weight. + # + # Not airtight, deliberately: scripts/native/rebuild-native.mjs verifies + # the restored binary's ABI and, on a mismatch it cannot resolve, purges + # the entry and compiles for real -- while `cache-hit` is still 'true'. + # On ubuntu-latest that fallback compile still has build-essential from + # the runner image, so it succeeds; if it ever did not, this job fails + # loudly and publishes nothing. build.yml:289-291 keeps its equivalent + # step unconditional for exactly this coupling; docs.yml accepts the + # trade because 9 s is 4% of its 237 s baseline. + - name: Install system dependencies + if: steps.native-cache.outputs.cache-hit != 'true' + run: | + sudo apt-get update + sudo apt-get install -y libsqlite3-dev build-essential + - name: Install dependencies run: npm ci @@ -84,26 +137,6 @@ jobs: - name: Build Electron app run: npx electron-vite build - # Cache the downloaded browser binaries. Keyed on the resolved Playwright - # version, because browser builds are pinned per Playwright release — a - # stale browser against a newer Playwright is exactly the mismatch this - # key prevents. The `--with-deps` system packages are NOT cached (they - # install into /usr/lib, outside the cache path), so the install step - # still runs; on a hit it skips only the browser download. - - name: Resolve Playwright version - id: pw - shell: bash - run: echo "ver=$(node -p "require('@playwright/test/package.json').version")" >> "$GITHUB_OUTPUT" - - - name: Restore Playwright browsers - uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # actions/cache@v6.1.0 - with: - path: ~/.cache/ms-playwright - key: playwright-${{ runner.os }}-${{ runner.arch }}-${{ steps.pw.outputs.ver }} - - - name: Install Playwright - run: npx playwright install --with-deps - - name: Generate screenshots run: xvfb-run --auto-servernum npx playwright test tests/e2e/screenshots.e2e.ts @@ -118,6 +151,10 @@ jobs: name: Deploy to GitHub Pages needs: build-screenshots runs-on: ubuntu-latest + # Baseline is 39 s, but actions/deploy-pages waits on the Pages API with its + # own 1200000 ms budget. 25 minutes sits just above that so the action's own + # timeout reports the real error rather than being masked by a job kill. + timeout-minutes: 25 permissions: pages: write diff --git a/.planning/artifacts/perf/build/docs-yml-before-after.md b/.planning/artifacts/perf/build/docs-yml-before-after.md new file mode 100644 index 000000000..0f4008417 --- /dev/null +++ b/.planning/artifacts/perf/build/docs-yml-before-after.md @@ -0,0 +1,118 @@ +# `docs.yml` performance: before/after (Phases 1-4, `perf/docs-workflow-screenshot-cache`) + +**Spec:** `.planning/specs/2026-08-06-docs-workflow-performance.md` +**Sample size: N=1 for the "after" run.** Every number below is a single observation, not an +average. See the Honesty caveats section before citing any figure elsewhere. + +## Runs compared + +| | Run ID | Ref | Commit | Cache state | +| --- | --- | --- | --- | --- | +| Baseline | `31109514729` | `main` | `1a79f98c` | warm caches (npm, native ABI, Playwright browsers) | +| After | `31120283316` | `perf/docs-workflow-screenshot-cache` | `5f0886b4` | warm native ABI cache | + +The after run executed while GitHub Actions was in a `major_outage` (runner-acquisition stalls). +The `Build & Screenshots` job **completed successfully**; only the queueing/dispatch overhead +around it was affected — see caveats below for exactly what that invalidates. + +## Per-step comparison — `Build & Screenshots` job + +| Step | Baseline (s) | After (s) | Delta | Classification | +| --- | ---: | ---: | ---: | --- | +| Set up job | 1 | 1 | 0 | unchanged | +| Configure git line endings | 0 | 0 | 0 | unchanged | +| Checkout code | 3 | 2 | -1 | improved | +| Setup Node.js | 8 | 7 | -1 | improved | +| Allow Electron to use unprivileged user namespaces (Ubuntu 24.04) | 0 | 0 | 0 | unchanged | +| Extract Electron version | 0 | 0 | 0 | unchanged | +| Restore native ABI cache | 2 | 1 | -1 | improved | +| Install system dependencies | 9 | 0 (skipped) | -9 | removed | +| Install dependencies | 16 | 14 | -2 | improved | +| Rebuild native modules for Electron | 0 | 0 | 0 | unchanged | +| Assert native ABI | 1 | 0 | -1 | improved | +| Build Electron app | 23 | 25 | +2 | new¹ | +| Resolve Playwright version | 0 | — | -0 | removed | +| Restore Playwright browsers | 8 | — | -8 | removed | +| Install Playwright | 29 | — | -29 | removed | +| Generate screenshots | 131 | 120 | -11 | improved | +| Upload screenshots | 2 | 2 | 0 | unchanged | +| Post steps (all) | 1 | 0 | -1 | improved | +| **Step sum** | **234** | **172** | **-62** | **-26.5%** | + +¹ "new" here means the step got *slower*, not that it is a newly-added step — it existed in the +baseline too. Flagged because it is the one step that regressed; folded into the step-sum total +regardless. Within run-to-run noise for a `vite build`. + +`Install system dependencies` shows as `skipped` in the after run, not `0s success` — the native +ABI cache hit (Phase 2b's gating: `cache-hit != 'true'`), so the apt install for +`libsqlite3-dev`/`build-essential` never ran. This is the **warm** native-cache path only (see +caveats). + +## Totals + +- **Build-job step sum: 234 s → 172 s, -62 s (-26.5%).** +- Deploy job (baseline, unmodified by this change): 39 s. +- **Estimated wall clock after: 172 s (steps) + ~3 s (job overhead, matching the ~3 s gap between + baseline's step sum and its 237 s job total) + 39 s (deploy) ≈ 214 s**, vs. 276 s baseline total + wall clock (**-22.5%**). +- Projected over 200 pushes (64 filtered out by the Phase 1 `paths` filter at 0 s each, 136 must + run at the ~214 s estimate): 64 × 0 + 136 × 214 = **29,104 s vs. 55,200 s baseline = 47.3% + reduction**. + +This 47.3% figure **beats** the spec's own best-case projected row ("2a fully succeeds, 2b, 2c" → +~225 s / 44.6%). See caveat 2 below — do not read this as the change outperforming the plan; read +it as one sample landing favorably on already-close projections. + +## Honesty caveats — read before citing any number above + +1. **N=1.** Every "after" figure is a single sample, not a distribution. In particular, + `Generate screenshots` moved 131 s → 120 s (-11 s), but Phase 2c only deleted 4.8 s of + deterministic `waitForTimeout` calls. **The remaining ~6.2 s is unexplained run-to-run + variance** (runner CPU/IO noise, GC timing, etc.), not an effect of any change in this spec. Do + not attribute the full -11 s to Phase 2c. +2. **The measured wall-clock estimate (~214 s) beat the spec's best-case projection (~225 s).** + This is reported plainly, but the ~11 s gap is attributed to variance in the noisy small steps + (`Setup Node.js`, `Checkout code`, `Install dependencies`, `Restore native ABI cache` each moved + 1-2 s in the improved direction) rather than to Phase 2's changes being more effective than + designed. The projection's three named levers (2a -37 s, 2b -9 s, 2c -4.8 s = -50.8 s) landed + close to plan (-62 s observed against non-Playwright/apt/sleep steps too); the extra headroom + came from steps the spec never claimed credit for. +3. **The job's own `jobtotal` (418 s) for the after run is not usable and must never be quoted as + a duration.** GitHub Actions was in a `major_outage` during this run, which caused + runner-acquisition stalls counted inside the job total but outside any step. **Only step + timings are comparable between the two runs.** All totals in this document are built by summing + steps, never by reading the job-total field for the after run. +4. **Still unverified — not closed by this run:** + - **Task 5's cold native-cache path.** `Install system dependencies` was `skipped` in this run + because the ABI cache hit. Phase 2b's `cache-hit != 'true'` gating logic has never actually + executed the apt-install branch on this branch. The correctness of "apt still runs, and still + works, on a cold cache" (spec Verification plan item 3, Correctness analysis row 4) remains + an untested code path. + - **The Pages deploy.** The `Deploy to GitHub Pages` job for run `31120283316` is stuck in + `waiting` because GitHub Pages was in `major_outage` at the same time. There is no green + end-to-end run of this branch that includes a successful deploy. The 39 s deploy figure used + in the wall-clock estimate above is carried over unmodified from the baseline run, not + re-measured on this branch. + - **The `paths` filter's actual skip behaviour.** No run of this workflow has been triggered by + a push that the filter was supposed to skip — the only two runs so far were `push` (baseline, + pre-filter) and effectively `workflow_dispatch`-equivalent on the branch, and + `workflow_dispatch` bypasses `paths` filters entirely per GitHub's documented behaviour. The + "64 of 200 pushes filtered out" figure comes from static git-history analysis in the spec, not + from an observed skip. +5. **The 47.3% projection is not a measurement.** It depends on the ~214 s per-run estimate holding + across all 136 "must run" pushes in the 200-push sample window. Some of those runs will hit a + cold native-ABI cache (re-adding the ~9 s apt step, unverified per caveat 4 above, plus whatever + the actual native compile costs beyond cache restore) and will be slower than this single warm + sample. Treat 47.3% as an estimate derived from one data point, not a verified outcome. + +## What Tasks 1-5 are confirmed vs. still unproven + +| Task | Claim | Status after this run | +| --- | --- | --- | +| 1 (`paths` filter) | Filters out 64/200 pushes at 0 s | **Unverified in CI.** Filter is committed; no run has been triggered by a path-filtered push (see caveat 4). Static analysis only. | +| 2a (drop Playwright browser install) | -37 s, screenshots still generate without a Playwright-downloaded browser | **Confirmed on the warm path.** `Resolve/Restore/Install Playwright` steps are absent from the after run; `Generate screenshots` succeeded (120 s) using only the app's own Electron binary. | +| 2b (gate apt on cache-hit) | apt skipped on warm cache, still runs correctly on cold cache | **Warm path confirmed** (`Install system dependencies` shows `skipped`). **Cold path unverified** — never exercised on this branch (see caveat 4). | +| 2c (delete 5 redundant sleeps) | -4.8 s deterministic, zero behavioural risk | **Confirmed present in the diff and the run stayed green,** but the isolated -4.8 s contribution cannot be distinguished from run-to-run noise inside the observed -11 s on `Generate screenshots` (see caveat 1). | +| 2d (screenshot manifest validation) | Fails the run if any of the 23 expected screenshots is missing | Present in the code; the after run produced all 23 without triggering the failure path, so the happy path is confirmed but the negative-path (deliberately broken selector) test from the spec's Verification plan item 5 is a separate, still-open check. | +| 4 (`timeout-minutes`) | Adds `timeout-minutes` to both `docs.yml` jobs | Committed; not exercised (neither job in this run got close to timing out), so nothing to observe either way. | +| Pages deploy (unmodified, out of scope) | Continues to work at ~39 s | **Unverified on this branch** — stuck `waiting` due to the Pages `major_outage` (see caveat 4). | diff --git a/.planning/plans/2026-08-06-docs-workflow-performance-plan.md b/.planning/plans/2026-08-06-docs-workflow-performance-plan.md new file mode 100644 index 000000000..b184115bc --- /dev/null +++ b/.planning/plans/2026-08-06-docs-workflow-performance-plan.md @@ -0,0 +1,658 @@ +# docs.yml Workflow Performance Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Cut `.github/workflows/docs.yml` wall-clock cost by skipping runs that cannot change the published site, and reducing the cost of runs that must regenerate. + +**Architecture:** Two levers plus one correctness fix, on one workflow. A workflow-level `paths` filter removes runs entirely (64/200 pushes). Targeted removals cut the cost of the 136/200 runs that must regenerate. A screenshot manifest fails the run when a test silently skips writing its PNG — a live bug today, where a stale five-month-old image is published and CI stays green. The content-keyed cache from revision 2 of the spec was **cut** after adversarial review: worth 2.2 percentage points, carrying the entire staleness surface. + +**Tech Stack:** GitHub Actions, Node 24.15.0 (ESM `.mjs`), Vitest, Playwright `_electron`, VitePress. + +**Spec:** `.planning/specs/2026-08-06-docs-workflow-performance.md` + +## Global Constraints + +- **The version rendered in screenshots must always be the current version from config.** Nothing about the version may be hardcoded, stubbed, or frozen. `AppFooter.vue:9` renders it in every screenshot; do not stub `system:version` or freeze it for determinism. +- Never commit to `main`. All work on branch `perf/docs-workflow-screenshot-cache`; every branch is destined for a PR. +- GitHub Actions must be pinned to full commit SHAs with a trailing `# owner/repo@vX.Y.Z` comment on the same line. +- No `console.*` in application code. These scripts are CI tooling, not application code, and `console.log` is their output channel — that is the documented exception pattern already used by `scripts/native/*.mjs`. +- Do not lower any coverage / lint / typecheck threshold. +- Do not re-enable ESLint `--concurrency=auto`, un-serialize typecheck, add `$(MAKE) -jN`, or change vitest `pool:'forks'`. `tests/scripts/build-pipeline-guardrails.test.ts` guards all four. +- Do not add a native-ABI cache or `assert-native-abi.mjs` to `docs.yml`'s `deploy` job — it runs `npm ci --ignore-scripts`, so no binary exists to assert against. +- Do not raise `actions/deploy-pages`' `timeout` input. +- Do not restore `cancel-in-progress: false` on the `pages` concurrency group, and do not revert `node-version-file: '.nvmrc'`. +- Source files stay under 600 lines; prefer 150–400. +- No performance claim may be reported without before/after step timings from a real CI run. + +## File Structure + +| File | Responsibility | +| --- | --- | +| `tests/e2e/screenshots.e2e.ts` | **Modify.** Adds the expected-screenshot manifest, records each write, and fails the run if any is missing. Also removes 5 redundant sleeps. | +| `.github/workflows/docs.yml` | **Modify.** Adds the `paths` filter, `timeout-minutes`, and the Phase-2 removals. | +| `.planning/specs/2026-08-06-docs-workflow-performance.md` | **Modify** at the end — replace projections with measured results. | + +--- + +### Task 1: Screenshot manifest validation + +Fixes a live bug: `test('02 - import menu')` wraps its `saveScreenshot` in +`if ((await plusBtn.count()) > 0)` (`tests/e2e/screenshots.e2e.ts:306`). If that selector stops +matching, the test passes without writing `import-menu.png`, and `Upload screenshots` publishes the +stale committed copy — currently a March 2026 image whose footer reads `v0.30.0`. CI stays green. + +**Files:** +- Modify: `tests/e2e/screenshots.e2e.ts` + +**Interfaces:** +- Produces: `EXPECTED_SCREENSHOTS: readonly string[]` (23 names) and a module-level + `capturedScreenshots: Set`, both internal to the spec file. + +- [ ] **Step 1: Add the manifest and the recording set** + +Immediately after the `VIEWPORT` constant (`tests/e2e/screenshots.e2e.ts:18`), add: + +```ts +/** + * Every screenshot this suite is responsible for producing, in execution order. + * + * Test 02 writes its PNG inside a conditional (`:306`); if its selector ever + * stops matching, the test passes without writing the file and the stale + * committed copy gets published instead. The final test in this file asserts + * this manifest against what was actually written, so that failure is loud. + */ +const EXPECTED_SCREENSHOTS = [ + 'empty-state', + 'import-menu', + 'case-list', + 'variant-table', + 'app-layout', + 'status-bar', + 'filters-active', + 'column-filters', + 'variant-details', + 'case-metadata', + 'acmg-classification', + 'comment-dialog', + 'annotations', + 'cohort-view', + 'filter-toolbar', + 'filter-preset-bar', + 'filter-drawer-sections', + 'filter-preset-save', + 'filter-preset-manage', + 'filter-dsl-autocomplete', + 'filter-column-numeric', + 'filter-column-categorical', + 'filter-empty-state' +] as const + +const capturedScreenshots = new Set() +``` + +- [ ] **Step 2: Record writes in the shared helper** + +Replace the body of `saveScreenshot` (`tests/e2e/screenshots.e2e.ts:37-40`) with: + +```ts +/** Save a screenshot to the docs screenshot directory */ +async function saveScreenshot(page: Page, name: string): Promise { + const filePath = path.join(SCREENSHOT_DIR, `${name}.png`) + await page.screenshot({ path: filePath, type: 'png' }) + capturedScreenshots.add(name) +} + +/** Save a cropped screenshot, recording it the same way as a full-viewport one. */ +async function saveClippedScreenshot( + page: Page, + name: string, + clip: { x: number; y: number; width: number; height: number } +): Promise { + const filePath = path.join(SCREENSHOT_DIR, `${name}.png`) + await page.screenshot({ path: filePath, type: 'png', clip }) + capturedScreenshots.add(name) +} +``` + +- [ ] **Step 3: Route the two clipped screenshots through the new helper** + +There are exactly two direct `window.screenshot({ ... clip ... })` calls that bypass the helper. +Replace each with a `saveClippedScreenshot` call so it is recorded. + +At `tests/e2e/screenshots.e2e.ts:812-822`, replace: + +```ts + const filePath = path.join(SCREENSHOT_DIR, 'status-bar.png') + await window.screenshot({ + path: filePath, + type: 'png', + clip: { + x: footerRect.x, + y: footerRect.y, + width: footerRect.width, + height: footerRect.height + } + }) +``` + +with: + +```ts + await saveClippedScreenshot(window, 'status-bar', { + x: footerRect.x, + y: footerRect.y, + width: footerRect.width, + height: footerRect.height + }) +``` + +At `tests/e2e/screenshots.e2e.ts:1562-1572`, replace: + +```ts + const filePath = path.join(SCREENSHOT_DIR, 'filter-preset-bar.png') + await window.screenshot({ + path: filePath, + type: 'png', + clip: { + x: toolbarRect.x, + y: toolbarRect.y, + width: toolbarRect.width, + height: toolbarRect.height + } + }) +``` + +with: + +```ts + await saveClippedScreenshot(window, 'filter-preset-bar', { + x: toolbarRect.x, + y: toolbarRect.y, + width: toolbarRect.width, + height: toolbarRect.height + }) +``` + +- [ ] **Step 4: Add the final assertion test** + +At the very end of the `test.describe` block — after `test('21 - filter empty state', ...)` and +before the closing `})` of the describe — add: + +```ts + // Runs last. Playwright executes tests in declaration order, and this file is + // a single serial chain, so by this point every producing test has run. + test('22 - every documented screenshot was captured', async () => { + const missing = EXPECTED_SCREENSHOTS.filter((name) => !capturedScreenshots.has(name)) + expect( + missing, + `These screenshots were never written this run, so the stale committed copies ` + + `would have been published: ${missing.join(', ')}` + ).toEqual([]) + + for (const name of EXPECTED_SCREENSHOTS) { + const filePath = path.join(SCREENSHOT_DIR, `${name}.png`) + expect(fs.existsSync(filePath), `${name}.png missing from disk`).toBe(true) + expect(fs.statSync(filePath).size, `${name}.png is empty`).toBeGreaterThan(0) + } + }) +``` + +- [ ] **Step 5: Verify lint, format, typecheck** + +Run: `make lint-check && make format-check && make typecheck` +Expected: all pass. If format-check fails, run `make format` and re-run. + +- [ ] **Step 6: Commit — before the destructive verification below** + +Commit now, so that step 7's deliberate break can be reverted with `git checkout --` +without destroying this task's work. + +```bash +git add tests/e2e/screenshots.e2e.ts +git commit -m "fix(test): fail the run when a documented screenshot was not regenerated + +test 02 writes import-menu.png inside a conditional; if its selector stops +matching, the test passes without writing the file and CI publishes the +stale committed copy -- currently a March 2026 image showing v0.30.0. +Adds an explicit manifest of the 23 screenshots and asserts it." +``` + +- [ ] **Step 7: Prove the guard actually fires** + +The work is committed, so this break is safe to revert. + +```bash +make rebuild && make build +sed -i "s/const plusBtn = window.locator('.v-toolbar .v-btn:has(.v-icon)').first()/const plusBtn = window.locator('.varlens-no-such-selector').first()/" tests/e2e/screenshots.e2e.ts +grep -n "varlens-no-such-selector" tests/e2e/screenshots.e2e.ts # confirm the break landed +xvfb-run --auto-servernum npx playwright test tests/e2e/screenshots.e2e.ts 2>&1 | tail -25 +``` + +Expected: test `22 - every documented screenshot was captured` FAILS, naming `import-menu`. +Before this change the suite would have passed with a stale PNG. **Record the exact failure output +in your report — this is the evidence the guard works.** + +Then revert the deliberate break: + +```bash +git checkout -- tests/e2e/screenshots.e2e.ts +grep -c "varlens-no-such-selector" tests/e2e/screenshots.e2e.ts # must print 0 +``` + +- [ ] **Step 8: Run the suite clean** + +Run: `xvfb-run --auto-servernum npx playwright test tests/e2e/screenshots.e2e.ts` +Expected: 24 passed (23 producers + the new assertion). + +If the local Electron/xvfb environment cannot run the suite, say so explicitly in your report +and mark the task DONE_WITH_CONCERNS — do not claim the guard was verified when it was not. +Steps 5 and 6 are still required. + +--- + +### Task 2: `paths` filter and `timeout-minutes` on docs.yml + +**Files:** +- Modify: `.github/workflows/docs.yml:3-6` (the `on:` block), and both `jobs:` entries. + +**Interfaces:** +- Consumes: nothing. +- Produces: a workflow that does not run on pushes that cannot change the published site. + +- [ ] **Step 1: Add the paths filter** + +Replace `.github/workflows/docs.yml:3-6`: + +```yaml +on: + push: + branches: [main] + # The union of every input that can reach the published bytes: `docs/**` + # (the only tree VitePress reads -- docs/.vitepress/config.mts and + # docs/.vitepress/theme/** import nothing outside docs/) plus every input + # to the screenshot pipeline, which is declared authoritatively in + # the screenshot pipeline. scripts/native/** is included because it decides + # which native binary the built app loads; a broken rebuild changes what the + # app renders, or whether it starts at all. + # + # Deliberately a workflow-level filter, unlike build.yml:22-27 which uses a + # job-level dorny/paths-filter. That comment's hazard -- workflow-level + # `paths-ignore` leaving required status checks permanently pending -- does + # not apply here: docs.yml has no `pull_request` trigger and is not a + # required check. A workflow-level filter costs 0 s on a skip where a + # job-level filter still spins up a runner. `workflow_dispatch` below is the + # escape hatch if this filter ever wrongly skips a needed run. + paths: + - 'docs/**' + - 'src/**' + - 'tests/e2e/screenshots.e2e.ts' + - 'tests/e2e/test-data/demo-case.json' + - 'playwright.config.ts' + - 'electron.vite.config.ts' + - 'tsconfig*.json' + - 'package.json' + - 'package-lock.json' + - '.nvmrc' + - 'scripts/native/**' + - '.github/workflows/docs.yml' + workflow_dispatch: +``` + +- [ ] **Step 2: Add `timeout-minutes` to both jobs** + +In `.github/workflows/docs.yml`, add to the `build-screenshots` job, immediately after `runs-on: ubuntu-latest`: + +```yaml + # Baseline is 237 s. 20 minutes is ~5x headroom -- enough for a cold native + # cache (which adds a ~35 s compile) plus a slow runner, without letting a + # hung Electron launch burn a full 6-hour default timeout. + timeout-minutes: 20 +``` + +And to the `deploy` job, immediately after `runs-on: ubuntu-latest`: + +```yaml + # Baseline is 39 s, but actions/deploy-pages waits on the Pages API with its + # own 1200000 ms budget. 25 minutes sits just above that so the action's own + # timeout reports the real error rather than being masked by a job kill. + timeout-minutes: 25 +``` + +- [ ] **Step 3: Verify the workflow still parses and guardrails still pass** + +Run: +```bash +python3 -c "import yaml,sys; yaml.safe_load(open('.github/workflows/docs.yml')); print('yaml ok')" +npx vitest run tests/scripts/build-pipeline-guardrails.test.ts +``` +Expected: `yaml ok`, and the guardrail suite passes. The new `screenshots-` key does not exist yet; this step proves the `paths`/`timeout-minutes` edits alone break nothing. + +- [ ] **Step 4: Commit** + +```bash +git add .github/workflows/docs.yml +git commit -m "perf(ci): skip docs.yml on pushes that cannot change the site + +Measured over 200 first-parent pushes to main: 64 changed nothing the +published site depends on, yet each cost a full 276 s rebuild and +redeploy. Also adds timeout-minutes to both jobs (spec item 4.5)." +``` + +--- + +### Task 3: Delete the redundant sleeps + +**Files:** +- Modify: `tests/e2e/screenshots.e2e.ts` at lines 360, 380, 1017, 1931, 1935. + +**Interfaces:** none. + +- [ ] **Step 1: Remove the five redundant waits** + +Delete exactly these five statements. Each is immediately adjacent to a real Playwright wait that already guarantees the same condition — verify the cited neighbour is present before deleting each one. + +| Line | Statement to delete | Neighbour that already guarantees it | +| --- | --- | --- | +| 360 | `await window.waitForTimeout(1500)` | `await caseItem.waitFor({ timeout: 15000 })` at 363–364 | +| 380 | `await window.waitForTimeout(500)` | `await expect(rows.first()).toBeVisible({ timeout: 15000 })` at 379 | +| 1017 | `await window.waitForTimeout(2000)` | `await firstBodyRow.waitFor({ state: 'visible', timeout: 15000 })` at 1020–1021 | +| 1931 | `await window.waitForTimeout(500)` | `await searchInput.first().click()` at 1930 (auto-waits) | +| 1935 | `await window.waitForTimeout(300)` | `await searchInput.first().fill('')` at 1934 (auto-waits) | + +Delete from the highest line number downward so earlier deletions do not shift later line numbers. + +- [ ] **Step 2: Verify the arithmetic** + +Run: +```bash +grep -o "waitForTimeout([0-9_]*)" tests/e2e/screenshots.e2e.ts \ + | sed 's/[^0-9_]//g' | tr -d '_' \ + | awk '{s+=$1; n++} END {print "calls:", n, "total ms:", s}' +``` +Expected: `calls: 90 total ms: 60800` (down from 95 / 65600). + +- [ ] **Step 3: Verify lint, format and typecheck** + +Run: `make lint-check && make format-check && make typecheck` +Expected: all pass. + +- [ ] **Step 4: Commit** + +```bash +git add tests/e2e/screenshots.e2e.ts +git commit -m "perf(test): drop 4.8 s of redundant sleeps from the screenshot suite + +Each removed waitForTimeout sat immediately beside a real Playwright wait +that already guaranteed the same condition. The 33 'replaceable' sleeps +are deliberately left alone: the suite is one unbroken causal chain and +rewriting them risks flake in the only pipeline that publishes the docs." +``` + +--- + +### Task 4: Determine experimentally whether the Playwright browser install is needed + +**Files:** +- Modify: `.github/workflows/docs.yml` — the `Resolve Playwright version`, `Restore Playwright browsers` and `Install Playwright` steps. + +**This task is an experiment. Its outcome is not predetermined. Do not claim a saving unless step 3 produces a green run.** + +**Interfaces:** none. + +- [ ] **Step 1: Record the hypothesis** + +`screenshots.e2e.ts:203` launches via `_electron.launch({ args: ['./out/main/index.js'] })`, which drives the Electron binary from `node_modules` — it never opens a Playwright-downloaded browser. If true, `Install Playwright` (29 s) and `Restore Playwright browsers` (8 s) are both removable. The risk is that `--with-deps` also installs shared libraries (`libnss3`, `libatk`, `libgbm`, …) that Electron needs under `xvfb-run`. + +- [ ] **Step 2: Remove all three steps** + +Delete the `Resolve Playwright version`, `Restore Playwright browsers`, and `Install Playwright` steps from `build-screenshots`, along with the block comment above `Resolve Playwright version` that explains the browser cache. + +- [ ] **Step 3: Run the experiment in CI** + +```bash +git add .github/workflows/docs.yml +git commit -m "experiment(ci): drop Playwright browser install from docs.yml" +git push -u origin perf/docs-workflow-screenshot-cache +gh workflow run docs.yml --ref perf/docs-workflow-screenshot-cache +``` + +Wait for completion, then read the result: +```bash +RUN=$(gh run list --workflow=docs.yml --branch perf/docs-workflow-screenshot-cache \ + --limit 1 --json databaseId --jq '.[0].databaseId') +gh run view "$RUN" --log-failed | head -50 +gh api "repos/berntpopp/varlens/actions/runs/$RUN/jobs" --jq '.jobs[] | + "== \(.name) [\(.conclusion)]", (.steps[] | + " \((((.completed_at|fromdateiso8601)-(.started_at|fromdateiso8601))))s\t\(.name)")' +``` + +- [ ] **Step 4: Branch on the outcome** + +**If `Generate screenshots` succeeded:** keep the removal. Record the measured saving. + +**If Electron failed to launch** (look for `error while loading shared libraries` or +`Electron app closed before the first window became available`): restore only the system +dependencies without downloading browsers. Replace the three deleted steps with: + +```yaml + # Electron needs Playwright's system libraries (libnss3, libatk, libgbm, ...) + # to start under xvfb, but it does NOT need Playwright's browser downloads -- + # screenshots.e2e.ts drives the app's own Electron binary via _electron.launch, + # never a Playwright browser. `install-deps` installs the former without the + # latter. Measured: see .planning/artifacts/perf/build/. + - name: Install Playwright system dependencies + run: npx playwright install-deps +``` + +Re-run step 3 and confirm green before proceeding. + +- [ ] **Step 5: Commit the outcome** + +```bash +git add .github/workflows/docs.yml +git commit -m "perf(ci): stop downloading Playwright browsers in docs.yml + +The screenshot suite drives Electron directly via _electron.launch and +never opens a Playwright browser. Measured saving recorded in the PR." +``` + +--- + +### Task 5: Gate the apt install on a cold native cache + +**Files:** +- Modify: `.github/workflows/docs.yml` — move and re-gate `Install system dependencies`. + +**Interfaces:** +- Consumes: `steps.native-cache.outputs.cache-hit`, which requires adding `id: native-cache` to the existing `Restore native ABI cache` step. + +- [ ] **Step 1: Give the native cache step an id** + +On the `Restore native ABI cache` step, add `id: native-cache` immediately below its `name:`. + +- [ ] **Step 2: Move and re-gate the apt step** + +Cut the `Install system dependencies` step and re-insert it **after** `Restore native ABI cache` and **before** `Install dependencies`, with a compound condition: + +```yaml + # libsqlite3-dev and build-essential exist only to compile + # better-sqlite3-multiple-ciphers. The ABI-keyed native cache restores a + # prebuilt binary, so on a hit nothing compiles and these packages are + # 9 s of dead weight. On a cold cache the compile is real and they are + # load-bearing -- hence the condition rather than deletion. + - name: Install system dependencies + if: steps.native-cache.outputs.cache-hit != 'true' + run: | + sudo apt-get update + sudo apt-get install -y libsqlite3-dev build-essential +``` + +- [ ] **Step 3: Verify the ordering is correct** + +Run: +```bash +python3 - <<'PY' +import yaml +steps = yaml.safe_load(open('.github/workflows/docs.yml'))['jobs']['build-screenshots']['steps'] +names = [s['name'] for s in steps] +assert names.index('Restore native ABI cache') < names.index('Install system dependencies'), \ + 'apt must come AFTER the native cache restore so it can read cache-hit' +assert names.index('Install system dependencies') < names.index('Install dependencies'), \ + 'apt must come BEFORE npm ci, whose postinstall may compile' +print('ordering ok') +PY +``` +Expected: `ordering ok`. + +- [ ] **Step 4: Verify with a COLD native cache in CI** + +This is the honest test — a warm-cache run proves nothing here. + +```bash +gh cache list --key native-Linux-X64 --json id --jq '.[].id' \ + | xargs -r -I{} gh api -X DELETE "repos/berntpopp/varlens/actions/caches/{}" +gh workflow run docs.yml --ref perf/docs-workflow-screenshot-cache +``` +Expected: the apt step runs, the native module compiles, `Generate screenshots` succeeds. + +- [ ] **Step 5: Commit** + +```bash +git add .github/workflows/docs.yml +git commit -m "perf(ci): run apt install only when the native cache misses + +libsqlite3-dev and build-essential exist to compile the native SQLite +module; the ABI-keyed cache means that rarely happens. Verified against a +deliberately cold native cache." +``` + +--- + +### Task 6: Measure, and replace every projection with a measured number + +**Files:** +- Create: `.planning/artifacts/perf/build/docs-yml-before-after.md` +- Modify: `.planning/specs/2026-08-06-docs-workflow-performance.md` — the "Expected result" section. + +**Interfaces:** none. + +- [ ] **Step 1: Produce a cold-native-cache run** + +This is the honest worst case, and the one that proves Tasks 4 and 5 did not break the compile path. + +```bash +gh cache list --key native-Linux-X64 --json id --jq '.[].id' \ + | xargs -r -I{} gh api -X DELETE "repos/berntpopp/varlens/actions/caches/{}" +gh workflow run docs.yml --ref perf/docs-workflow-screenshot-cache +``` +Record the run id as `COLD_RUN`. + +- [ ] **Step 2: Produce a warm-native-cache run** + +```bash +gh workflow run docs.yml --ref perf/docs-workflow-screenshot-cache +``` +Record the run id as `WARM_RUN`. This is the run comparable to the 276 s baseline, which was also +measured warm. + +- [ ] **Step 3: Pull step timings for both runs** + +```bash +for RUN in "$COLD_RUN" "$WARM_RUN"; do + echo "=== run $RUN ===" + gh api "repos/berntpopp/varlens/actions/runs/$RUN/jobs" --jq '.jobs[] | + "== \(.name) [\(.conclusion)] total=\((.completed_at|fromdateiso8601)-(.started_at|fromdateiso8601))s", + (.steps[] | " \((((.completed_at|fromdateiso8601)-(.started_at|fromdateiso8601))))s\t\(.name)")' +done +``` + +- [ ] **Step 4: Verify the screenshots are correct, not merely produced** + +```bash +gh run download "$WARM_RUN" --name screenshots --dir /tmp/shots-verify +ls -la /tmp/shots-verify/*.png | wc -l +``` +Expected: 23 PNGs. Open `/tmp/shots-verify/app-layout.png` and confirm the footer reads the +**current** version from `package.json` — not `v0.30.0`, and not a stubbed value. Confirm no +screenshot is blank or truncated. + +- [ ] **Step 5: Write the artifact and correct the spec** + +Create `.planning/artifacts/perf/build/docs-yml-before-after.md` containing: the 276 s baseline table +from the spec, both measured runs' per-step timings, and a delta column classifying each step +`improved` / `unchanged` / `removed`. + +Then edit the spec's "Expected result" section: state which of the four projected outcome rows +actually occurred, and replace the projected miss cost with the measured one. Delete the sentence +beginning "Every figure above is a projection". **If a projection was missed, write the measured +number and say so explicitly — do not round in our favour.** + +- [ ] **Step 6: Commit** + +```bash +git add .planning/artifacts/perf/build/docs-yml-before-after.md \ + .planning/specs/2026-08-06-docs-workflow-performance.md +git commit -m "docs(planning): record measured docs.yml before/after timings" +``` + +--- + +### Task 7: Full gate and PR + +- [ ] **Step 1: Run the full local gate** + +Run: `make ci-full` +Expected: green. If it fails, fix the cause — do not weaken the gate. + +- [ ] **Step 2: Confirm a green end-to-end docs.yml run including the Pages deploy** + +```bash +gh run list --workflow=docs.yml --branch perf/docs-workflow-screenshot-cache --limit 3 +``` +Expected: the most recent run succeeded in **both** jobs, including `Deploy to GitHub Pages`. + +If the Pages deploy fails: **retry once before investigating.** Per issue #366 a transient GitHub-side fault stalled six consecutive deploys for ~2.5 h and cleared on its own. Never use `gh run rerun --failed` on this workflow — it re-executes `upload-pages-artifact`, leaving two artifacts named `github-pages`, and `deploy-pages` then hard-errors. Always re-trigger fresh with `gh workflow run docs.yml --ref `. + +- [ ] **Step 3: Open the PR** + +```bash +gh pr create --title "perf(ci): make docs.yml skip, cache, and cost less" --body "$(cat <<'EOF' +Implements `.planning/specs/2026-08-06-docs-workflow-performance.md`. + +## What + +- Workflow-level `paths` filter — 64 of the last 200 pushes to `main` changed nothing the published site depends on, yet each cost a full 276 s rebuild and redeploy. +- Screenshot manifest validation — fixes a live bug where a silently-skipped test republishes a stale PNG and CI stays green. +- Miss-path reductions: Playwright browser install, apt gating, and 4.8 s of redundant sleeps. +- `timeout-minutes` on both jobs. + +## Measured + +See `.planning/artifacts/perf/build/docs-yml-before-after.md` for per-step before/after timings from real CI runs. + +## Correctness + +A skipped run publishes nothing, so the `paths` filter has no staleness surface. The filter set is the union of every input VitePress reads (`docs/**` only — `docs/.vitepress/config.mts` and `theme/**` import nothing outside `docs/`) and every input to the screenshot pipeline. `workflow_dispatch` is the escape hatch. + +An earlier revision of this work added a content-keyed screenshot cache. It was **cut** after adversarial review (codex `gpt-5.6-terra`, xhigh): measured over 200 pushes it hits 7 times, worth ~2.2 percentage points, while carrying the entire staleness surface of the design. The review found two CRITICAL holes in it. Full record in the spec's "Adversarial review record" section. It is deferred, correctly sequenced behind the manifest validation this PR adds. + +## Overlap with tracked work + +Touches two items of `.planning/specs/2026-08-05-build-ci-performance.md`: item 4.5 (`timeout-minutes`, applied to `docs.yml`'s two jobs only) and item 4.10 (Playwright browser caching, which this PR *removes* from `docs.yml` because the suite drives Electron directly via `_electron.launch` and never opens a Playwright browser). + +## Known gaps + +Screenshots are not a deterministic function of the source: `case-metadata.png` renders the import date, `status-bar.png` renders live network status, and the suite uses no isolated `userData` dir. Documented in the spec. This is a precondition that must be addressed before the deferred cache is reconsidered. +EOF +)" +``` + +--- + +## Self-Review + +**Spec coverage.** Phase 1 → Task 2. Phase 2a → Task 4. Phase 2b → Task 5. Phase 2c → Task 3. Phase 2d → Task 1. Phase 3 is deferred and deliberately has no task. Phase 4 (`timeout-minutes`) → Task 2. Verification plan → Tasks 6 and 7. The spec's "out of scope" items (33 replaceable sleeps, deploy job, repo-wide timeouts) have deliberately no task. + +**Placeholders.** None. Task 5 is an experiment with two fully-written branches rather than a "figure it out" step. Task 7 requires measured numbers to replace projections and forbids reporting the projection if it was missed. + +**Type consistency.** `EXPECTED_SCREENSHOTS`, `capturedScreenshots`, `saveScreenshot` and `saveClippedScreenshot` are used with identical names and shapes across Task 1's steps. `steps.native-cache.outputs.cache-hit` is the only step output referenced; Task 5 step 1 explicitly adds the `native-cache` id that its condition depends on, before step 2 uses it. diff --git a/.planning/specs/2026-08-06-docs-workflow-performance.md b/.planning/specs/2026-08-06-docs-workflow-performance.md new file mode 100644 index 000000000..f68b6d394 --- /dev/null +++ b/.planning/specs/2026-08-06-docs-workflow-performance.md @@ -0,0 +1,384 @@ +# Spec: `docs.yml` workflow performance + +**Date:** 2026-08-06 +**Branch:** `perf/docs-workflow-screenshot-cache` +**Status:** Revision 3 — adversarial review applied (codex `gpt-5.6-terra`, xhigh) + +## Problem + +`.github/workflows/docs.yml` runs on every push to `main` and is the slowest workflow in the +repository. It is the only workflow that has never been optimised. + +Measured baseline — run `31109514729`, `main` @ `1a79f98c`, warm caches, from the Actions API: + +``` +Build & Screenshots ................ 237 s + Set up job / git cfg / checkout .... 4 s + Setup Node.js ...................... 8 s + Install system dependencies ........ 9 s apt: libsqlite3-dev, build-essential + Extract Electron version ........... 0 s + Restore native ABI cache ........... 2 s hits + Install dependencies (npm ci) ...... 16 s + Rebuild native modules for Electron. 0 s cache restore, no-op + Assert native ABI .................. 1 s + Build Electron app ................. 23 s electron-vite build + Resolve Playwright version ......... 0 s + Restore Playwright browsers ........ 8 s hits + Install Playwright ................. 29 s + Generate screenshots ............... 131 s 55% of the job + Upload screenshots ................. 2 s + Post steps ......................... 1 s +Deploy to GitHub Pages .............. 39 s + Set up job / checkout .............. 4 s + Setup Node.js ...................... 7 s + npm ci --ignore-scripts ............ 14 s + Download screenshots ............... 1 s + Build docs (VitePress) ............. 4 s + Setup Pages / Upload to Pages ...... 1 s + Deploy to GitHub Pages ............. 6 s + Post steps ......................... 1 s + +TOTAL WALL CLOCK ................... 276 s (jobs serialised via `needs`) +``` + +## Binding constraint + +**The version shown in the screenshots must always be the current version from config.** Nothing +about the version may be hardcoded, stubbed, or frozen. This is a product requirement and it +dominates the design: it is why the content-keyed cache was cut (see Phase 3). + +## Hypotheses this spec refutes + +Recorded so they are not re-investigated. Items 4 and 5 refute earlier drafts of this spec itself. + +1. **"The screenshot suite relaunches Electron per screenshot."** False. `screenshots.e2e.ts` + launches Electron once in `beforeAll` (`tests/e2e/screenshots.e2e.ts:203`) and closes it once in + `afterAll` (`:278`). All 23 tests share one module-level `window`. + +2. **"Committing the screenshots would be the biggest win."** The premise is already true. All 34 + PNGs are tracked in git (4.5 MB) via an explicit `.gitignore` negation at `.gitignore:154`. Only + 23 of the 34 are regenerated by CI; the other 11 are already pure committed artifacts, and 11 of + those 34 have no working generator in the repo at all. + +3. **"A `paths` filter is unsafe because GitHub evaluates only the first 300 changed files."** The + documented threshold is 3,000 files; the >1,000-commit case fails *open*. The largest push in the + last 200 first-parent commits on `main` was 309 files (`e9375399`). + +4. **"The root project version cannot change a rendered pixel, so it can be stripped from the cache + key."** **False, and it was a real bug in revision 1 of this spec.** `AppFooter.vue:9` renders + `VarLens v{{ appVersion }}`; `App.vue:59` places `AppFooter` in the persistent shell, so it appears + in every screenshot. `appVersion` comes from `api.system.getVersion()` → + `src/main/ipc/handlers/system.ts:27-54`, which in the un-packaged mode the suite runs under walks + up from `app.getAppPath()` and reads `package.json`'s `version` directly. Verified visually: the + committed `app-layout.png` and `case-metadata.png` both render `VarLens v0.30.0` in the footer. + Stripping the version would have produced a cache hit that republished 23 screenshots showing the + previous release's version — a silent staleness bug. + +5. **"The content-keyed cache is the big win."** False. Measured over 200 pushes it hits 7 times, + worth ~2.2 percentage points. It was cut — see Phase 3. + +6. **"Every one of the 23 screenshots is regenerated on every run."** False, and it is a **live bug + today, independent of any cache.** `test('02 - import menu')` wraps its entire body — including + the `saveScreenshot` at `:312` — in `if ((await plusBtn.count()) > 0)` (`:306`). If that selector + stops matching, the test passes without writing `import-menu.png`, and `Upload screenshots` + publishes the stale committed copy (currently March 2026, footer `v0.30.0`). Verified: of the 23 + screenshot-producing tests, `04c` and `14` have if/else branches that both write, so exactly one — + test 02 — can silently produce nothing. Phase 2d closes this. + +## Evidence + +### Push composition, last 200 first-parent pushes to `main` + +Classified by the filter and key sets defined in this spec: + +| Outcome | Count | Cost each | +| --- | --- | --- | +| Skipped by the `paths` filter | 64 | 0 s | +| Must run | 136 | 276 s | + +Baseline: 200 × 276 s = 55,200 s (15.33 h). + +Because the version must stay current, every `chore(release)` bump legitimately changes all 23 +screenshots. Release bumps plus dependency changes plus source changes account for 129 of the 136 +runs; only 7 would ever have hit a content cache. + +### Where the value actually is + +1. **The `paths` filter** — 64 of 200 pushes (32%) never need to run at all. +2. **The miss path** — 136 of 200 pushes (68%) must regenerate, so every second cut from the 276 s + is multiplied by 136. +3. **Correctness** — Phase 2d fixes a live staleness bug (refuted hypothesis 6) that no amount of + caching would have addressed, and that caching would have made permanent. + +Revision 1 of this spec had these in exactly the wrong order, and put a 2.2-point cache first. + +## Design + +### Phase 1 — Workflow-level `paths` filter + +```yaml +on: + push: + branches: [main] + paths: + - 'docs/**' + - 'src/**' + - 'tests/e2e/screenshots.e2e.ts' + - 'tests/e2e/test-data/demo-case.json' + - 'playwright.config.ts' + - 'electron.vite.config.ts' + - 'tsconfig*.json' + - 'package.json' + - 'package-lock.json' + - '.nvmrc' + - 'scripts/native/**' + - '.github/workflows/docs.yml' + workflow_dispatch: +``` + +The union of every input that can reach the published bytes: `docs/**` (the only tree VitePress +reads — verified: `docs/.vitepress/config.mts` and `docs/.vitepress/theme/**` import nothing outside +`docs/`) plus every input to the screenshot pipeline. + +**Deliberate divergence from repo convention.** `build.yml:22-27` uses job-level `dorny/paths-filter` +because workflow-level `paths-ignore` leaves required status checks permanently pending and blocks +merges. That hazard does not apply here: `docs.yml` has no `pull_request` trigger and is not a +required check. A workflow-level filter costs 0 s on a skip where a job-level filter still spins a +runner. `web-ci.yml:5-20` is the existing precedent for a workflow-level `paths:` list. +`workflow_dispatch` stays as the escape hatch. + +### Phase 2 — Miss-path cost reduction + +This affects 130 of 200 pushes and is the largest lever. Three independent items, each requiring CI +verification before its saving may be claimed. + +**2a. Establish whether Playwright's browser install is needed at all.** `screenshots.e2e.ts` uses +`_electron.launch({ args: ['./out/main/index.js'] })` (`:203`), which drives the app's own Electron +binary out of `node_modules` — it never opens a Playwright-downloaded browser. If so, both +`Install Playwright` (29 s) and `Restore Playwright browsers` (8 s) are removable: **37 s**. + +The risk is that `--with-deps` also installs shared libraries (`libnss3`, `libatk`, `libgbm`, …) that +Electron itself needs to start under `xvfb-run`, and those install into `/usr/lib`, outside the cache +path. **This is already answered by evidence in the repository, and is not an open experiment.** +`build.yml:304` and `build.yml:361` both run `xvfb-run --auto-servernum ... npx playwright test` +on `ubuntu-latest`, launching the same unpackaged `./out/main/index.js` via `_electron.launch`, +with no `playwright install` and no `--with-deps`. Both are green first-class gates today. After +this change no workflow in the repository runs `playwright install` at all. Independently +confirmed: `playwright`, `@playwright/test` and `playwright-core` at 1.62.1 declare no `install` +or `postinstall` script, so `npm ci` never downloaded browsers -- only the removed step did, which +is why the saving is real rather than relocated. + +The residual risk is therefore low but not zero, because `docs.yml` drives far more of the UI than +a startup smoke test does. A green `docs.yml` run remains required before the saving is reported +as fact -- but it is a confirmation, not a de-risking experiment. + +**2b. Gate the apt install on the native cache result.** `libsqlite3-dev` and `build-essential` +(9 s) exist to compile `better-sqlite3-multiple-ciphers`. Since the ABI-keyed native cache restores +a prebuilt binary, no compile happens on a hit. Move the apt step below the native cache restore and +gate it on `cache-hit != 'true'`, so it still runs — and remains correct — on a cold cache. Must be +verified with a deliberately cold native cache, not only a warm one. + +**2c. Delete the 5 redundant `waitForTimeout` calls — 4,800 ms, zero behavioural risk.** Each is +immediately adjacent to a real Playwright wait that already guarantees the same condition: +`screenshots.e2e.ts:360` (followed by `waitFor({timeout:15000})` at `:363-364`), `:380` (preceded by +`expect(...).toBeVisible({timeout:15000})` at `:379`), `:1017` (followed by +`waitFor({state:'visible'})` at `:1020-1021`), `:1931` and `:1935` (both preceded by auto-waiting +actions). + +**Explicitly NOT done: the 33 "replaceable" sleeps (33,700 ms).** They guard real conditions that +could in principle be expressed as explicit waits, but the suite is a single unbroken causal chain — +test 03 imports the demo case and every test from 04 onward depends on it; test 08 depends on test +07 leaving a drawer open; tests 10 and 11 depend on test 09 having written an ACMG classification to +the database. Rewriting 33 wait sites across that chain risks introducing flake into the only +pipeline that publishes the docs site, for at most 33.7 s on 65% of runs. Recorded as a follow-up to +be done on its own branch with its own CI evidence, not folded into this change. + +**2d. Screenshot manifest validation — a correctness fix, not a performance one.** Declare the 23 +expected filenames in the spec file, record each one as it is written, and fail the run if any was +not produced. This closes refuted hypothesis 6: today a silently-skipped test republishes a +five-month-old PNG and CI stays green. + +It is included here because it is the prerequisite for ever reintroducing the cache safely — a cache +must not persist a result that was never fully generated — and because it is a live bug in the +pipeline this change is already touching. + +### Phase 3 — DEFERRED: content-keyed screenshot cache + +**Cut from this change after adversarial review.** Measured over 200 pushes it hits 7 times and is +worth ~2.2 percentage points, while carrying the entire staleness surface of the design. Codex found +two CRITICAL holes in it (a successful miss can save a stale PNG into the cache and republish it +indefinitely; `docs.yml` and `scripts/native/**` are pixel-relevant but were absent from the key). +Both are fixable, but 2.2 points does not justify the risk in the same PR that changes the workflow. + +It is **correctly sequenced as a follow-up**: Phase 2d below (screenshot manifest validation) is its +prerequisite, because without a guarantee that every screenshot was actually regenerated, a cache +cannot safely persist the result. Do Phase 2d, let it prove itself, then revisit the cache. + +If revisited, the key must additionally include `.github/workflows/docs.yml` and `scripts/native/**`, +and the recorded design is in git history at revision 2 of this file. + +### Phase 4 — `timeout-minutes` + +No job in any of the five workflows declares `timeout-minutes` (verified: zero matches across 18 +jobs). This adds it to `docs.yml`'s two jobs. This is item 4.5 of +`.planning/specs/2026-08-05-build-ci-performance.md:365`, applied to the workflow being touched +rather than repo-wide. Item 4.10 of that spec (cache Playwright browsers, +`.planning/specs/2026-08-05-build-ci-performance.md:389`) is already implemented in `docs.yml` and +may be *removed* by Phase 2a — the PR must say so. + +## Expected result — measured + +**Run `31120283316` (`perf/docs-workflow-screenshot-cache` @ `5f0886b4`) confirms the spec's own +best-case row: 2a fully succeeded, 2b succeeded on the warm-cache path, and 2c landed.** Full +step-by-step data, the honesty caveats, and the per-task confirmation table live in +`.planning/artifacts/perf/build/docs-yml-before-after.md`; this section summarizes the numbers +that document produces. + +| | Count / 200 | Before | After | +| --- | --- | --- | --- | +| Filtered out | 64 | 276 s | 0 s | +| Must run | 136 | 276 s | see below | +| **Total** | 200 | **55,200 s** | — | + +Measured, single run (N=1), warm native-ABI cache: + +- `Build & Screenshots` step sum: **234 s → 172 s, −62 s (−26.5%)**. +- Estimated wall clock: 172 s (steps) + ~3 s (job overhead) + 39 s (deploy, unmodified) ≈ + **214 s, vs. 276 s baseline (−22.5%)**. +- Projected over 200 pushes at this per-run cost: 64 × 0 s + 136 × 214 s = 29,104 s vs. 55,200 s + baseline = **47.3% reduction**. + +This measured ~214 s slightly **beats** the spec's own best-case projected row (2a −37 s, 2b −9 s, +2c −4.8 s → ~225 s / 44.6%, arithmetic: 37 + 9 + 4.8 = 50.8 s off 276 s). Read that as one sample +landing close to a projection that was already accurate, not as the change outperforming the plan +— the extra headroom came from small, noisy steps (`Setup Node.js`, `Checkout code`, `Install +dependencies`, `Restore native ABI cache`) the projection never claimed credit for, and `Generate +screenshots`' 131 s → 120 s move is only ~4.8 s attributable to Phase 2c's deterministic sleep +removal, with the remaining ~6 s being unexplained run-to-run variance (see the artifact for the +full breakdown). + +**Precisely what is measured vs. still projected:** + +- **Measured, from one real CI run:** the `Build & Screenshots` step-sum delta (234 s → 172 s), that + the job completed successfully with no Playwright browser install present, and that + `Install system dependencies` was skipped on a warm native-ABI cache hit. +- **Estimated, not directly measured on this branch:** the ~214 s wall-clock figure (the deploy + job's 39 s is carried over from the baseline run, not re-measured here — see below) and the + 47.3%-over-200-pushes projection, which assumes the ~214 s per-run cost holds across runs this + branch has not exercised. + +### Still projected / still unverified + +1. **The `paths` filter's skip behaviour.** No run has actually been triggered by a push the filter + was meant to skip; the only runs so far were a plain `push` (baseline, pre-filter) and this + branch's run, which behaves like `workflow_dispatch` and bypasses `paths` filters entirely. The + "64 of 200" figure is git-history analysis, not an observed skip. +2. **Task 5 / Phase 2b's cold native-cache path.** `Install system dependencies` was skipped + because the ABI cache hit on this run. The apt-install branch, gated on + `cache-hit != 'true'`, has never actually executed on this branch. +3. **The Pages deploy.** `Deploy to GitHub Pages` for run `31120283316` is stuck `waiting` — GitHub + Pages was in `major_outage` at the same time as GitHub Actions. There is no green end-to-end run + of this branch including a successful deploy; the 39 s figure above is the unmodified baseline + number, not a re-measurement. + +## Correctness analysis + +With the cache cut, the staleness surface collapses: a filtered-out run publishes nothing, so there +is nothing to go stale. What remains: + +| # | Failure mode | Closed by | +| --- | --- | --- | +| 1 | `paths` filter skips a push that changes the site | The filter set is the union of every input VitePress reads (`docs/**` only — verified) and every input to the screenshot pipeline; `workflow_dispatch` is the manual override | +| 2 | A test silently fails to write its screenshot; the stale committed PNG is published | Phase 2d manifest validation fails the run | +| 3 | Phase 2a removes a system library Electron needs | Verified by a real CI run before the saving is claimed; documented fallback to `npx playwright install-deps` | +| 4 | Phase 2b removes apt packages needed by a real native compile | Gated on `steps.native-cache.outputs.cache-hit != 'true'`, and verified against a deliberately **cold** native cache | +| 5 | Phase 2c deletes a sleep that was load-bearing | Only the 5 waits with an adjacent real Playwright wait guaranteeing the same condition are removed; the 33 "replaceable" ones are left alone | + +## Known gaps — accepted, not solved + +1. **The screenshots are not a deterministic function of the hashed inputs.** Three ambient inputs + leak in, all verified in the committed PNGs: + - `case-metadata.png` renders `Imported {{ formatDate(createdAt) }}` at date granularity + (`CaseMetadataModal.vue:18`) — the committed copy reads "Imported Mar 15, 2026". Two runs on + different calendar days differ. + - `status-bar.png` renders `navigator.onLine` as a live network icon (`AppFooter.vue:289`). + - `electron.launch` (`screenshots.e2e.ts:203`) passes no isolated `userData` dir, so localStorage, + saved presets and the recent-databases list persist between runs on the same machine. CI + runners are ephemeral so this does not affect CI, but it makes local regeneration differ. + + An earlier revision claimed caching *reduced* exposure to these. Adversarial review refuted that: + freezing makes it worse, because a capture taken under a bad ambient condition (an offline runner, + say) would be republished indefinitely rather than self-correcting on the next run. With the cache + cut this is moot — every run that happens regenerates — but the reasoning is recorded because it + is a precondition for ever reintroducing the cache. + +2. **Runner image drift.** `ubuntu-latest` rolls forward; a new image with different font packages + changes antialiasing. Cosmetic, unhashable. Pinning `ubuntu-24.04` would close it at the cost of + manual image maintenance. Not done. + +3. **11 of 34 committed PNGs have no working generator.** The 5 gene-panels and 4 protein-viz PNGs + were committed without a producing spec; the 2 shortlist PNGs come from a spec that writes + different filenames to `/tmp` and references `scripts/annotate-shortlist-docs-shots.sh`, which + does not exist in the working tree or anywhere in git history. Pre-existing rot. This design's + only obligation is not to make it worse, which failure mode 2 ensures. + +4. **6 of 34 committed PNGs are referenced by no docs page**: `column-filters.png`, + `filters-active.png`, and the 4 protein-viz PNGs. Two of the six are regenerated by CI on every + miss for nothing. Deleting dead assets is a docs content decision. Out of scope; recorded. + +5. **One deleted sleep is not a strict equivalence.** The `waitForTimeout(500)` removed from + `test('04 - variant table')` sat between an `expect(...).toBeVisible()` and a large + `page.evaluate` that reads `getBoundingClientRect()` for five highlight boxes. `toBeVisible` + guarantees the elements are painted, not that layout has settled. Every highlight in that block + is individually guarded, so a mis-measured box degrades `variant-table.png` *silently* -- the + file is still written, so the manifest assertion still passes. Risk is low (the toolbar and + footer mount strictly before rows render) but the failure mode is invisible to CI. Verification + plan item 4 must therefore inspect `variant-table.png` specifically, by eye. + +## Out of scope + +- The 33 replaceable sleeps (33.7 s) — justified under Phase 2c. +- The Pages deploy job — working at 39 s, untouched per issue #366. +- Repo-wide `timeout-minutes` — only `docs.yml`'s two jobs here. + +## Verification plan + +1. `make ci` green. **Satisfied** — already green prior to this branch's CI run. +2. `tests/scripts/build-pipeline-guardrails.test.ts` still passes. **Satisfied** — already + passing. +3. A `workflow_dispatch` run of `docs.yml` on this branch with a deliberately **cold native cache** — + proves Phase 2a did not break Electron launch and that Phase 2b's apt gating is correct when the + compile actually runs. **Open.** Run `31120283316` hit a **warm** native-ABI cache + (`Install system dependencies` shows `skipped`); the cold-cache apt-install branch has never + executed on this branch. +4. Inspect the resulting artifact: all 23 PNGs present, none blank or truncated, and the footer + reads the **current** version from `package.json` — not `v0.30.0`, not a stubbed value. + **Satisfied** by run `31120283316`: 23 PNGs present, none blank, footer reads the current + `v0.70.5`, and `variant-table.png`'s five highlight boxes were confirmed correctly aligned by + eye — closing Known-gap 5's concern for this run. +5. Deliberately break test 02's selector locally and confirm Phase 2d fails the run rather than + silently publishing the stale PNG. Revert. **Open** — not exercised as part of this run. +6. Step timings for the before and after runs pulled from the Actions API and recorded under + `.planning/artifacts/perf/build/`. **Satisfied** — see + `.planning/artifacts/perf/build/docs-yml-before-after.md`, comparing baseline run + `31109514729` against after run `31120283316`. +7. A green end-to-end `docs.yml` run including the Pages deploy. **Open.** + `Deploy to GitHub Pages` for run `31120283316` is stuck `waiting` — GitHub Pages was in + `major_outage` at the same time as GitHub Actions. +8. `make ci-full` green once at the end. **Open** — not run as part of this task. + +## Adversarial review record + +Reviewed by codex `gpt-5.6-terra` at xhigh effort against revision 2. Findings and dispositions: + +| Finding | Severity | Disposition | +| --- | --- | --- | +| A successful cache miss can save a stale PNG and republish it indefinitely | CRITICAL | **Accepted, verified.** Narrowed on inspection to exactly one screenshot (test 02). Cache cut; root cause fixed by Phase 2d. | +| `docs.yml` and `scripts/native/**` are pixel-relevant but absent from the cache key | CRITICAL | **Accepted.** Moot with the cache cut; recorded as a requirement for any future reintroduction. | +| Ambient inputs (date, network state) can seed a wrong cache indefinitely | MAJOR | **Accepted.** The "caching reduces exposure" claim was wrong and has been corrected in Known gaps. | +| Arithmetic: 37 + 9 + 4.8 = 50.8 → ~225 s, not the claimed ~204 s; push split is 64/7/129, not 64/6/130 | MINOR | **Accepted.** Both corrected; the classification arithmetic is now shown in the Expected result table. | +| `actions/cache@` would violate the full-SHA pinning policy if literal | MINOR | Was pseudocode; moot with the cache cut. | + +Claims codex attempted to refute and could not: that revision 2 correctly hashes `package.json` and +`package-lock.json` in full; that `screenshots.e2e.ts` imports no repository helper; that +`src/renderer/public/**` is covered; that no VitePress input exists outside `docs/**`. diff --git a/tests/e2e/screenshots.e2e.ts b/tests/e2e/screenshots.e2e.ts index dd6c4f112..7e268e04f 100644 --- a/tests/e2e/screenshots.e2e.ts +++ b/tests/e2e/screenshots.e2e.ts @@ -17,6 +17,42 @@ const SCREENSHOT_DIR = path.resolve(__dirname, '../../docs/public/screenshots') const DEMO_DATA_PATH = path.resolve(__dirname, 'test-data/demo-case.json') const VIEWPORT = { width: 1280, height: 800 } +/** + * Every screenshot this suite is responsible for producing, in execution order. + * + * Test 02 writes its PNG inside a conditional in `test('02 - import menu')`; if its selector ever + * stops matching, the test passes without writing the file and the stale + * committed copy gets published instead. The final test in this file asserts + * this manifest against what was actually written, so that failure is loud. + */ +const EXPECTED_SCREENSHOTS = [ + 'empty-state', + 'import-menu', + 'case-list', + 'variant-table', + 'app-layout', + 'status-bar', + 'filters-active', + 'column-filters', + 'variant-details', + 'case-metadata', + 'acmg-classification', + 'comment-dialog', + 'annotations', + 'cohort-view', + 'filter-toolbar', + 'filter-preset-bar', + 'filter-drawer-sections', + 'filter-preset-save', + 'filter-preset-manage', + 'filter-dsl-autocomplete', + 'filter-column-numeric', + 'filter-column-categorical', + 'filter-empty-state' +] as const + +const capturedScreenshots = new Set() + let app: ElectronApplication let window: Page let tempGzipPath: string @@ -37,6 +73,18 @@ function expectSuccessfulIpcResult(result: T): T { async function saveScreenshot(page: Page, name: string): Promise { const filePath = path.join(SCREENSHOT_DIR, `${name}.png`) await page.screenshot({ path: filePath, type: 'png' }) + capturedScreenshots.add(name) +} + +/** Save a cropped screenshot, recording it the same way as a full-viewport one. */ +async function saveClippedScreenshot( + page: Page, + name: string, + clip: { x: number; y: number; width: number; height: number } +): Promise { + const filePath = path.join(SCREENSHOT_DIR, `${name}.png`) + await page.screenshot({ path: filePath, type: 'png', clip }) + capturedScreenshots.add(name) } /** @@ -357,7 +405,6 @@ test.describe('Documentation Screenshots', () => { await window.reload() await window.waitForSelector('.v-application', { timeout: 30000 }) await dismissDisclaimer(window) - await window.waitForTimeout(1500) // Look for the case in the sidebar const caseItem = window.locator('.v-list-item').filter({ hasText: /DemoCase/ }) @@ -377,7 +424,6 @@ test.describe('Documentation Screenshots', () => { const rows = window.locator('.v-data-table__tr') await expect(rows.first()).toBeVisible({ timeout: 15000 }) - await window.waitForTimeout(500) // Add labeled highlights for key UI sections referenced in the docs await window.evaluate(() => { @@ -809,16 +855,11 @@ test.describe('Documentation Screenshots', () => { }) if (footerRect) { - const filePath = path.join(SCREENSHOT_DIR, 'status-bar.png') - await window.screenshot({ - path: filePath, - type: 'png', - clip: { - x: footerRect.x, - y: footerRect.y, - width: footerRect.width, - height: footerRect.height - } + await saveClippedScreenshot(window, 'status-bar', { + x: footerRect.x, + y: footerRect.y, + width: footerRect.width, + height: footerRect.height }) } else { await saveScreenshot(window, 'status-bar') @@ -1014,7 +1055,6 @@ test.describe('Documentation Screenshots', () => { // Open the variant details panel by clicking a row await ensureCaseSelected(window) - await window.waitForTimeout(2000) // Wait for table rows to be fully loaded and clickable const firstBodyRow = window.locator('.v-data-table tbody .v-data-table__tr').first() @@ -1559,16 +1599,11 @@ test.describe('Documentation Screenshots', () => { }) if (toolbarRect) { - const filePath = path.join(SCREENSHOT_DIR, 'filter-preset-bar.png') - await window.screenshot({ - path: filePath, - type: 'png', - clip: { - x: toolbarRect.x, - y: toolbarRect.y, - width: toolbarRect.width, - height: toolbarRect.height - } + await saveClippedScreenshot(window, 'filter-preset-bar', { + x: toolbarRect.x, + y: toolbarRect.y, + width: toolbarRect.width, + height: toolbarRect.height }) } else { await saveScreenshot(window, 'filter-preset-bar') @@ -1928,11 +1963,9 @@ test.describe('Documentation Screenshots', () => { const searchInput = window.locator('.filter-search-input input, .dsl-search-bar input') if ((await searchInput.count()) > 0) { await searchInput.first().click() - await window.waitForTimeout(500) // Type slowly to trigger autocomplete await searchInput.first().fill('') - await window.waitForTimeout(300) await searchInput.first().pressSequentially('gnomad_af:', { delay: 100 }) await window.waitForTimeout(1500) } @@ -2263,4 +2296,21 @@ test.describe('Documentation Screenshots', () => { await window.keyboard.press('Escape') await window.waitForTimeout(500) }) + + // Runs last. Playwright executes tests in declaration order, and this file is + // a single serial chain, so by this point every producing test has run. + test('22 - every documented screenshot was captured', async () => { + const missing = EXPECTED_SCREENSHOTS.filter((name) => !capturedScreenshots.has(name)) + expect( + missing, + `These screenshots were never written this run, so the stale committed copies ` + + `would have been published: ${missing.join(', ')}` + ).toEqual([]) + + for (const name of EXPECTED_SCREENSHOTS) { + const filePath = path.join(SCREENSHOT_DIR, `${name}.png`) + expect(fs.existsSync(filePath), `${name}.png missing from disk`).toBe(true) + expect(fs.statSync(filePath).size, `${name}.png is empty`).toBeGreaterThan(0) + } + }) })