fix(ci): notebook sync readiness signal, lightweight fixture deps#714
fix(ci): notebook sync readiness signal, lightweight fixture deps#714
Conversation
The smoke spec queries for code cells directly without using executeFirstCell from helpers, so it missed the waitForCodeCells fix from PR #709. Same root cause — kernel idle arrives before cells materialize from Automerge sync.
NotebookView now exposes data-notebook-synced and data-cell-count attributes on its container. data-notebook-synced is true when isLoading is false and cells have rendered — the canonical signal that Automerge sync is complete. E2E tests use waitForNotebookSynced() instead of polling for cell DOM elements with arbitrary timeouts. This replaces waitForCodeCells in executeFirstCell and the smoke test.
pandas, numpy, scipy, matplotlib, seaborn → httpx, markupsafe. These fixtures exist to test environment detection and kernel launch, not data science imports. Heavy deps add 30+ seconds of install time on CI cold runners, causing timeout flakes in integration tests.
rgbkrk
changed the title
There was a problem hiding this comment.
Pull request overview
Improves CI reliability by adding a deterministic “notebook is ready” signal for E2E tests and by slimming down Python fixture dependencies to reduce install time and timeout flakiness.
Changes:
- Exposes a
data-notebook-syncedattribute onNotebookViewand adds awaitForNotebookSynced()E2E helper to wait on that signal. - Updates E2E specs to use the new wait helper and switches dependency-import assertions from heavy DS libs to lightweight packages.
- Replaces heavy fixture dependencies (numpy/pandas/matplotlib/etc.) with
httpx/markupsafeacross fixture manifests and related tests.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/notebook/src/components/NotebookView.tsx | Adds DOM data attributes used as an E2E readiness signal. |
| e2e/helpers.js | Introduces waitForNotebookSynced() and updates executeFirstCell() to use it. |
| e2e/specs/smoke.spec.js | Uses the new notebook-synced wait before interacting with cells. |
| e2e/specs/uv-pyproject.spec.js | Updates the pyproject dependency import check to httpx. |
| e2e/specs/untitled-pyproject.spec.js | Updates the pyproject dependency import check to httpx. |
| e2e/specs/conda-inline.spec.js | Updates the conda-inline dependency import check to markupsafe. |
| python/runtimed/tests/test_daemon_integration.py | Updates pyproject auto-detection integration assertion to import httpx. |
| crates/notebook/src/pyproject.rs | Updates fixture-based parsing assertions to match new fixture deps. |
| crates/runtimed/src/notebook_sync_client.rs | Adds module-level docs describing confirm_sync usage. |
| crates/notebook/fixtures/sample-project/pyproject.toml | Replaces DS deps with lightweight deps (httpx, markupsafe). |
| crates/notebook/fixtures/conda-project/environment.yml | Replaces DS deps with markupsafe. |
| crates/notebook/fixtures/audit-test/pyproject-project/pyproject.toml | Replaces DS deps with httpx. |
| crates/notebook/fixtures/audit-test/pixi-project/pixi.toml | Replaces DS deps with markupsafe. |
| crates/notebook/fixtures/audit-test/conda-env-project/environment.yaml | Replaces DS deps with markupsafe. |
| crates/notebook/fixtures/audit-test/3-conda-inline.ipynb | Updates inline conda deps metadata to markupsafe. |
| crates/notebook/fixtures/audit-test/4-both-deps.ipynb | Updates inline conda deps metadata to markupsafe. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| ref={containerRef} | ||
| className="flex-1 overflow-y-auto overflow-x-clip overscroll-x-contain py-4 pl-8 pr-4" | ||
| style={{ contain: "paint" }} | ||
| data-notebook-synced={!isLoading && cells.length > 0} |
There was a problem hiding this comment.
data-notebook-synced is computed as !isLoading && cells.length > 0, which means it will never become true for a legitimately empty notebook once loading finishes (cells rendered = empty state). This conflates “sync/loading complete” with “has at least one cell” and can block any E2E wait that wants to proceed on an empty-but-loaded notebook (e.g., before clicking “Add code cell”). Consider separating these concerns (e.g., a data-notebook-loaded={!isLoading} readiness signal plus using data-cell-count when a test specifically needs cells), or adjust the attribute naming/logic so an empty notebook can still be considered ready.
| data-notebook-synced={!isLoading && cells.length > 0} | |
| data-notebook-synced={!isLoading} |
| await typeSlowly("import markupsafe; print(markupsafe.__version__)"); | ||
| await browser.keys(["Shift", "Enter"]); |
There was a problem hiding this comment.
This test now imports markupsafe; the nearby comment that gives an example version string still references a numpy-style version ("1.26.4"). Please update that example to a MarkupSafe-appropriate version to avoid confusion when debugging CI failures.
Three changes to make CI reliable:
data-notebook-syncedattribute — NotebookView exposes a data attribute when Automerge sync is complete and cells are rendered. E2E tests wait for this signal instead of polling for cell DOM elements with arbitrary timeouts. Fixes the flaky "element not existing after 5000ms" failures where kernel idle arrived before cells materialized.waitForNotebookSynced()helper — ReplaceswaitForCodeCellsinexecuteFirstCelland the smoke test. One canonical wait function backed by a real readiness signal.Lightweight fixture deps — Replaced pandas, numpy, scipy, matplotlib, seaborn with httpx and markupsafe across all fixture project files. These fixtures test environment detection and kernel launch, not data science imports. Heavy deps added 30+ seconds of install time on CI, causing timeout flakes in the pyproject integration test.
PR submitted by @rgbkrk's agent Quill, via Zed