chore(stage6): wrap-up — Playwright per-US specs + facets unit test + typo-recall eval#18
Conversation
…ypo-recall eval Closes out the remaining Stage 6 todos that were deferred to "Phase 11 polish" pending a live preview / built data package. Now that the data package is local and production is up, they're all genuinely runnable. Tests added: - `site/src/tests/e2e/search.spec.ts` (T062) — FR-007 narrowing, FR-008 typo recovery on ≥ 7-char words, PR#17 operator grammar (`"phrase"` ≤ bare-AND set, `-word` subtracts, `OR` unions), and the ✨ semantic-only badge tooltip contract. - `site/src/tests/e2e/facets.spec.ts` (T068) — FR-005/FR-013: facet click narrows results, sibling-facet counts recompute against the narrowed set, `Clear` restores the corpus-wide count. - `site/src/tests/e2e/cart.spec.ts` (T071) — FR-006/SC-009: card-icon add + reload (localStorage persistence), `Clear` empties, `cart-email` produces a `mailto:` URL with the poster_id in the body. - `site/src/tests/e2e/tour.spec.ts` (T081) — US6 acceptance: CTA banner on first visit, "Start tour" opens `.shepherd-element`, dismissal hides the CTA while the header `Tour` button remains. - `site/src/tests/unit/facets.test.ts` (T064) — FR-013 nuance: a facet's own selections don't zero its sibling options' counts. 4 cases against a 4-abstract fixture. 75/75 unit tests pass. Eval script: - `scripts/eval_typo_recall.py` (T058a, SC-010) — ports the lexical- search core to Python and replays it against the data-package shards. Generates full-title probes (capped at 8 tokens) with one recoverable single-typo on a ≥ 4-char content word. Recall against the live shards: **0.9685 over 762 probes** (≥ 0.90 target). The threshold scheme (<4 → exact, 4–6 → DL ≤ 1, ≥ 7 → DL ≤ 2) confirmed sound. Exits 0 on pass / 3 on fail (so the script is usable as a CI gate later). Bookkeeping (T099 + T100 + several already-shipped tasks reconciled in `specs/008-ui-rewrite/tasks.md`): - T026 (US8 draft-PR verification) — DONE via PRs #9–#17 Deployments- box surface. - T090 (Lighthouse-CI) — DONE in PR #15. - T091 (a11y axe-core audit) — DONE in PRs #12 + #14. - T096 (SC sweep) — DONE; SC-002 / SC-003 / SC-004 / SC-005 / SC-011 measured against production via `sc-sweep.spec.ts`. - T097 (FR-021 / FR-022 acceptance) — DONE in spirit across PRs #9–#17. - T099 (reconciliation pass) — DONE in this commit. - T100 — marked OBSOLETE (Stage 6 shipped across 10+ PRs, not as one final consolidating PR). After this PR lands, the only Stage 6 work that remains is the operator-syntax UX shipped in PR #17 (post-spec enhancement) — the spec's 105 numbered tasks are complete. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of tests and evaluation scripts for the UI rewrite. Key additions include a Python-based typo-recall evaluation script, Playwright E2E tests for search, facets, cart, and the guided tour, and Vitest unit tests for facet logic. The task list in tasks.md has also been updated to reflect the completion of these items. Reviewers identified several improvement opportunities: ensuring determinism in the typo-recall script by avoiding Python's non-deterministic hash(), adopting 'web-first' Playwright assertions instead of manual counts and brittle timeouts, and utilizing native Vitest Map comparisons for cleaner test code.
| """ | ||
| if len(word) < 4 or not word.isalpha(): | ||
| return [] | ||
| rng = random.Random(hash(word) & 0xFFFFFFFF) |
There was a problem hiding this comment.
The use of hash() in Python 3 is non-deterministic across different process executions because of hash randomization. This contradicts the docstring's claim that the evaluation is "deterministic across runs". To ensure reproducibility, consider using a stable hash (like zlib.adler32) or passing the rng instance from the main function to seed the local random.Random instance.
| await page.getByTestId('toggle-cart').click(); | ||
| await expect(page.getByTestId('cart-drawer')).toBeVisible(); | ||
| const items = page.getByTestId('cart-item'); | ||
| expect(await items.count()).toBeGreaterThanOrEqual(1); |
There was a problem hiding this comment.
Using locator.count() in a standard expect assertion is not "web-first" and will not retry if the UI hasn't finished updating. This can lead to flaky tests in environments with varying performance.
| expect(await items.count()).toBeGreaterThanOrEqual(1); | |
| await expect(items.first()).toBeVisible(); |
| // The lexical pipeline is synchronous; semantic is async but only | ||
| // changes the ✨ badge / rank, not the result-count. Wait briefly for | ||
| // the count to stabilise. | ||
| await page.waitForTimeout(250); |
There was a problem hiding this comment.
Hardcoded timeouts like waitForTimeout are brittle and can lead to flakiness or unnecessarily slow tests. It is recommended to wait for a specific UI state (e.g., the result count updating or a loading indicator disappearing) using Playwright's web-first assertions or waitForFunction.
| ) | ||
| .catch(() => null); | ||
| await typeQueryAndSettle(page, '"critical brain hypothesis"'); | ||
| await page.waitForTimeout(800); // give semantic neighbour pass time to settle |
There was a problem hiding this comment.
This 800ms timeout is quite long and brittle. Since the semantic search status is tracked in a store (as mentioned in the task list), it would be more robust to wait for the search to transition from a 'loading' to a 'ready' state, or for the specific semantic-score elements to appear if they are expected.
| // recomputeFacets with the empty map === recomputeFacets with no filters. | ||
| const a = recomputeFacets(fixture, empty, null); | ||
| const b = recomputeFacets(fixture, NO_FILTERS, null); | ||
| expect(JSON.stringify([...a])).toBe(JSON.stringify([...b])); |
There was a problem hiding this comment.
Vitest's toEqual assertion natively supports comparing Map objects. Stringifying the maps is less efficient and provides less helpful diffs on failure.
| expect(JSON.stringify([...a])).toBe(JSON.stringify([...b])); | |
| expect(a).toEqual(b); |
…erts Addresses 5 medium-priority gemini-code-assist comments on PR #18. scripts/eval_typo_recall.py — `hash(word) & 0xFFFFFFFF` was non- deterministic across Python processes (`PYTHONHASHSEED` randomisation), contradicting the docstring's "deterministic across runs" promise. Switched to `zlib.adler32(word.encode('utf-8'))` — a stable 32-bit hash that's plenty for seeding a per-word PRNG. Eval still passes: **recall@10 = 0.9894 over 378 probes** (was 0.9685 with the old randomised seed, well above the 0.90 target either way). site/src/tests/unit/facets.test.ts — replaced `JSON.stringify(...)` Map equality with Vitest's native `expect(...).toEqual(...)`. Walks the Map structurally and gives a useful diff on failure. site/src/tests/e2e/cart.spec.ts — converted `expect(await count())` one-shots to web-first `await expect(locator.first()).toBeVisible()` and `await expect(locator).toHaveCount(0)`. Dropped every `waitForTimeout` in this spec; Playwright auto-retries. site/src/tests/e2e/search.spec.ts — replaced the three `waitForTimeout` calls (250 ms between fills, 100 ms after clearing, 800 ms after a semantic query) with `expect.poll` based two-tick stability checks. The semantic-settle wait now has a 2 s budget and fails fast if the worker hangs, rather than always burning the hard-coded 800 ms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed all 5 gemini-code-assist comments in 6fc3b3f:
|
1 participant
Summary
Closes out the remaining Stage 6 todos that had been deferred to "Phase 11 polish" pending a live preview / built data package. Now that the data package is local and production is deployed, all of them are genuinely runnable.
Test plan
After this lands
Stage 6's 105 numbered tasks are complete. The only Stage-6-area work remaining is the operator-syntax UX shipped in PR #17 (post-spec enhancement).
🤖 Generated with Claude Code