Refs #1745 -- Harden nav/hooks regression guard#1748
Conversation
|
Thanks @BaderEddineBenhirt for the contribution. I'll review run it through the review process. |
|
Thanks @johnrtipton for the quick response. My experience was mostly smooth. The setup with uv and the demo project was straightforward, and the existing Playwright regression script made the issue easy to understand and reproduce locally. The only slightly rough part was understanding which Playwright checks are intended to be blocking versus optional in CI. That is why I kept the broader Playwright job optional and added a dedicated blocking guard only for the nav/hooks regression. Overall, the repo was clear to work with. |
johnrtipton
left a comment
There was a problem hiding this comment.
Code Review (pipeline-run Stage 7) — APPROVE with follow-ups
Solid, well-motivated hardening of the #1745 nav/hooks regression guard. All 14 CI checks green (incl. the new nav-hooks-guard, 2m9s). No 🔴 blockers.
Strengths
- Condition-based waits replace fixed
asyncio.sleep— waits on real signals (__demoWidgetMountCount, thedjustHookMountedDOM marker,location.pathname, mount-count delta) instead of wall-clock. Faster and far less flaky. - Observer installed before
gotoviaadd_init_scriptcorrectly catches the initial hydration churn (#1737) a post-gotoevaluate()could miss; the late-root handling (document observer +DOMContentLoadedre-check) is sound. - Not greenwashing: the script
return 1on any collected failure →sys.exit(exit_code)— a real failing gate (verified). - Made blocking correctly: added to
test-summaryneedsAND the success AND-condition — it genuinely gates, not just echoed.
🟡 Follow-ups (non-blocking — tracked, not gating this merge)
nav-hooks-guardis the only Rust-dependent CI job withoutmaturin develop --release. Every other ext-dependent job runsuv sync+maturin develop. This job relies onuv's.venvcache (key =hashFiles(pyproject.toml, uv.lock)), so a PR that changes onlycrates/*.rs(no Python dep change) would test against a stale cached extension — a blocking render guard that can silently pass on stale code. Recommend adding thematurin develop --releasestep. (Maintainer fast-follow.)- Blocking Playwright job = flake risk to every PR's gate.
playwright-testsis intentionally optional because browser jobs flake; promoting a Playwright job to blocking reintroduces that gate-wide. The condition-based waits mitigate it (good); per repo canon a new runner-only job ideally soakscontinue-on-errorbefore flipping. Green once here, so low risk — flagged for awareness. - ~80% CI duplication with
playwright-tests(server harness). Consider a composite action / reusable workflow to prevent drift.
Minor
- Run step uses
.venv/bin/pythonwhile siblings useuv run(cosmetic). - Confirm new action pins (
checkout@v6,setup-uv@v7,cache@v5,upload-artifact@v7) match versions used elsewhere in this workflow.
Merging; 🟡#1–3 filed as a follow-up. Thanks for the contribution!
APPROVE
Retrospective — PR #1748 (refs #1745)Task: harden the nav/hooks Playwright regression guard (condition-based waits + observer-before-nav) and promote it to a blocking CI job. External contribution by @BaderEddineBenhirt. Quality: 4/5Well-motivated, correctly-implemented hardening; merged green with no 🔴. Held back from 5 only by the blocking-gate-without- What went well
What didn't / to improve
Verified
|
…1752) (#1755) nav-hooks-guard and playwright-tests duplicated ~9 setup steps (Python/uv/cache/ uv sync/maturin build/playwright install/migrate/start server/wait). #1753 had to patch the missing maturin step into nav-hooks-guard precisely because the copy had drifted. Extract the shared harness into .github/actions/djust-playwright-server (composite); both jobs now `uses:` it after checkout, then run their own test step + log upload + server stop. One definition can't drift a step out of one job (parallel-path-drift, #1646). Both jobs keep their identity: playwright-tests stays continue-on-error (optional), nav-hooks-guard stays blocking; test-summary wiring unchanged. Closes #1752. Item 1 (maturin) shipped in #1753; item 2 (blocking-soak) resolved by decision — keep nav-hooks-guard blocking (green across #1748/#1753/#1754); item 3 (this dedup). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@BaderEddineBenhirt thanks for the PR, this has shipped in 1.0.3. |
This PR hardens the nav/hooks regression guard.
Changes:
Tested locally: