Repository navigation
feat: Add explanatory loader text is during first load and handle potential loading failure modes - #240
Conversation
jat255
changed the title
|
Local Claude review found a few issues worth fixing, and a couple "low" findings I think weren't worth the effort: Fixed
Not addressed — two minor points in the R engine code: a comment describing a failed package install as non-fatal when it actually stops startup (the comment is wrong, the behaviour is unchanged from |
jat255
changed the title
cpsievert
left a comment
cpsievert
left a comment
There was a problem hiding this comment.
Here's a few things I noticed from just a quick sniff test. Also, I think the scope is large enough here that I would feel more comfortable shipping this if we had decent test coverage. Is that something you've looked into already?
We can (and I think we should) add some unit test coverage. I've been running a suite of tests locally (not included in the PR) for this set of changes in particular, but since the repo wasn't already setup for running unit tests, I didn't want to blow up the scope as part of this PR. Happy to do that either here or in a separate PR (probably more appropriate). |
|
Would it make sense to add to the |
|
@cpsievert after talking with @schloerke and @karangattu, I'm going to make a separate PR to setup the unit test infrastructure with some baselines so this PR will have something to run against once getting rebased. I'll set it back to draft for now. |
`playwright/` held four specs that nothing ran: `make test` called an npm script that does not exist, and two of the four shelled out to `shinylive export` from the Python shinylive package -- which depends on this repository, the circular dependency that kept the playwright job in build.yml commented out for years. All four are now pytest tests in `tests/`, alongside the example suite, and they run in CI. Groundwork, which #240 also needs: - `fail_on_example_errors` becomes `fail_on_page_errors`, and takes an opt-out: a test that provokes an error on purpose marks itself `allow_page_errors` and asserts on the failure instead. - Every test now carries an `examples` or a `site` marker. The example targets select on `examples`, so the site tests are not swept into their engine x shard matrix, and `make site-test` runs them in one job. The export fixture, `tests/export_app.py`: an export is `app.json`, `export_template/index.html` with its variables filled in, `edit/index.html` as-is, and the shinylive bundle -- all of which `make all` already produces under `build/`. Nothing here needs `pip install shinylive` any more. The migrated tests: - `test_site_editor.py` and `test_site_url_loading.py` from examples-viewer.spec.ts and load-from-url.spec.ts, against the `_shinylive/` build the session fixture already serves. - `test_site_export.py` from shiny-static.spec.ts and editor-cell.spec.ts, against the export fixture. The app mode comes from the `?_shinylive-mode=` query param, replacing a `sed` in playwright.config.ts that had not matched anything in `export_template/index.html` in a long time. Two fixes the migration turned up: load-from-url's "app view can show header bar" test navigated to `/editor/`, so it never tested the app view; and Mod-Enter in the editor is Cmd on a Mac, which the old spec worked around by pressing both modifiers. Removals: `playwright.config.ts`, the four specs and their helpers, the `cypress:open` script (cypress is not a dependency), the `@playwright/test` devDependency, the broken `test` target, and the commented-out playwright job in build.yml with its now-obsolete rationale.
`playwright/` held four specs that nothing ran: `make test` called an npm script that does not exist, and two of the four shelled out to `shinylive export` from the Python shinylive package -- which depends on this repository, the circular dependency that kept the playwright job in build.yml commented out for years. All four are now pytest tests in `tests/`, alongside the example suite, and they run in CI. Groundwork, which #240 also needs: - `fail_on_example_errors` becomes `fail_on_page_errors`, and takes an opt-out: a test that provokes an error on purpose marks itself `allow_page_errors` and asserts on the failure instead. - Every test now carries an `examples` or a `site` marker. The example targets select on `examples`, so the site tests are not swept into their engine x shard matrix, and `make site-test` runs them in one job. The export fixture, `tests/export_app.py`: an export is `app.json`, `export_template/index.html` with its variables filled in, `edit/index.html` as-is, and the shinylive bundle -- all of which `make all` already produces under `build/`. Nothing here needs `pip install shinylive` any more. The migrated tests: - `test_site_editor.py` and `test_site_url_loading.py` from examples-viewer.spec.ts and load-from-url.spec.ts, against the `_shinylive/` build the session fixture already serves. - `test_site_export.py` from shiny-static.spec.ts and editor-cell.spec.ts, against the export fixture. The app mode comes from the `?_shinylive-mode=` query param, replacing a `sed` in playwright.config.ts that had not matched anything in `export_template/index.html` in a long time. Two fixes the migration turned up: load-from-url's "app view can show header bar" test navigated to `/editor/`, so it never tested the app view; and Mod-Enter in the editor is Cmd on a Mac, which the old spec worked around by pressing both modifiers. Removals: `playwright.config.ts`, the four specs and their helpers, the `cypress:open` script (cypress is not a dependency), the `@playwright/test` devDependency, the broken `test` target, and the commented-out playwright job in build.yml with its now-obsolete rationale.
* test: Migrate the orphaned playwright specs to pytest (#243) `playwright/` held four specs that nothing ran: `make test` called an npm script that does not exist, and two of the four shelled out to `shinylive export` from the Python shinylive package -- which depends on this repository, the circular dependency that kept the playwright job in build.yml commented out for years. All four are now pytest tests in `tests/`, alongside the example suite, and they run in CI. Groundwork, which #240 also needs: - `fail_on_example_errors` becomes `fail_on_page_errors`, and takes an opt-out: a test that provokes an error on purpose marks itself `allow_page_errors` and asserts on the failure instead. - Every test now carries an `examples` or a `site` marker. The example targets select on `examples`, so the site tests are not swept into their engine x shard matrix, and `make site-test` runs them in one job. The export fixture, `tests/export_app.py`: an export is `app.json`, `export_template/index.html` with its variables filled in, `edit/index.html` as-is, and the shinylive bundle -- all of which `make all` already produces under `build/`. Nothing here needs `pip install shinylive` any more. The migrated tests: - `test_site_editor.py` and `test_site_url_loading.py` from examples-viewer.spec.ts and load-from-url.spec.ts, against the `_shinylive/` build the session fixture already serves. - `test_site_export.py` from shiny-static.spec.ts and editor-cell.spec.ts, against the export fixture. The app mode comes from the `?_shinylive-mode=` query param, replacing a `sed` in playwright.config.ts that had not matched anything in `export_template/index.html` in a long time. Two fixes the migration turned up: load-from-url's "app view can show header bar" test navigated to `/editor/`, so it never tested the app view; and Mod-Enter in the editor is Cmd on a Mac, which the old spec worked around by pressing both modifiers. Removals: `playwright.config.ts`, the four specs and their helpers, the `cypress:open` script (cypress is not a dependency), the `@playwright/test` devDependency, the broken `test` target, and the commented-out playwright job in build.yml with its now-obsolete rationale. * test: harden the site-test groundwork from the review Four follow-ups to the playwright migration, none of them changing what the migrated tests assert. The console-error gate would have failed on CI. build.yml is the only workflow that sets GOOGLE_TAG_MANAGER_ID, so `make all` there templates a Google Tag Manager loader into the site's pages (scripts/build.ts) -- and the site tests are the only suite that runs against that build. A tag manager that fails to load reaches the console as an error, exactly the way the missing favicon the gate already forgives does, so the suite that now gates the deploy would have depended on an outside host being reachable. `block_analytics` aborts those requests, which also keeps the async loader from holding up the page's load event, and the gate forgives the abort. A test with neither the `examples` nor the `site` marker runs in no CI job. `pytest.ini` says every test carries one, but nothing made that true; collection now refuses a test that has neither rather than let it silently never run. `--shard` was dealing out the whole of `tests/`, not the selected engine. A conftest's `pytest_collection_modifyitems` runs before the one in `_pytest.mark` -- which is what lets `-m py` see the engine markers added there -- so the sharding at the end of it was splitting all 91 tests and only then dropping the other engine's. The shards come out the same size today by coincidence; moving the sharding into a `trylast` plugin makes that hold whatever else lands in tests/. Also: `make help` never listed `test-deps`, because the `##` line has to sit directly above the target, and README now tells people to run it. * docs: Reconcile the README with the jest harness The rebase left both PRs' test sections side by side, and the surviving one still pointed at `make examples-test-deps`, the `playwright/` specs and the `make test` target, none of which exist after this branch. One section now, covering unit tests, examples and site tests, with the split by marker described where the app tests are introduced. * docs: Cut the history lesson from the export comments Four places retold how the playwright job came to be commented out. The comments only need to say why the tests build their own export: depending on the Python shinylive package would be circular. One of them had also gone stale, since this branch removes that commented-out job. * chore: Give the test targets a common `test-` prefix Review asked for prefix matching, so `make test-<TAB>` completes the whole family rather than three separate stems: test-unit, test-unit-coverage, test-deps, test-examples-smoke, test-examples-intent, test-site. The workflow job ids and artifact names follow their targets. Renaming a job id renames its status check, which is safe here -- main has no branch protection, no rulesets, and no required checks -- and nothing downloads the artifacts. deploy_gh_pages goes with them, as the last underscore left in build.yml. type-check and examples-check-index keep their names: they are checks rather than test suites, and type-check mirrors the npm script it runs. Also adds type-check and the two unit-test targets to the README's `make help` listing, which had not been updated for them.
The app-loading overlay showed a bare animation with no text. On a first visit the browser is downloading a multi-megabyte Pyodide or webR runtime plus packages, which can take tens of seconds, and nothing said so — a slow load was indistinguishable from a broken app. Under the animation, the overlay now names the current stage: "Downloading Python…", "Starting Python…", then "Loading packages and starting app…" (or the R equivalents). The text appears only after a delay, so a fast load looks exactly as it did before. A small per-engine store carries the stage from the engine init functions to the overlay. It is module-level because initialization begins before any component mounts, and per-engine because a page can mix Python and R blocks and run both at once. Also: if the engine fails to load, the overlay now shows an error instead of animating forever.
When loadPyodide() itself threw, the catch block tested `e instanceof pyodide.ffi.PythonError` while `pyodide` was still undefined. That raised a second error from inside the catch, so the reply was never posted and the caller's promise never settled — leaving the loading overlay up indefinitely. Guard the check so the original error is reported.
…nothing Four failure paths were either silent or unbounded. A missing engine wasm hung the loader forever. Neither loadPyodide() nor webR's init() settles in that case — not resolved, not rejected — and neither leaves an error the page can catch. engine-load-guard.ts now checks that the core wasm can be fetched before the engine starts, reporting a 404 or an unreachable host in about 0.2s and naming the URL. Reading only the status line costs roughly one round trip rather than the tens of seconds the file itself takes. An asset that downloads but cannot be instantiated, such as a truncated one, still spins with no error screen; that gap is left open deliberately. WebWorkerPyodideProxy.init() discarded the worker's error reply, so a Pyodide that never started looked like one that did. A 404 on pyodide-lock.json now surfaces in under a second rather than being swallowed. R app startup errors never reached JavaScript. .start_app is evaluated with captureConditions = FALSE, so a syntax error left the viewer displaying an app that had not started: a blank frame, the error only in the terminal, and nothing at all in a viewer-only embed. It now returns the message instead, and the caller uses evalRString so webR marshals the string before the shelter holding it is purged. Package installation stays non-fatal, because renv::dependencies() also reports packages that are never used and those apps run fine today. Errors offered no advice. The alert now leads with a hard refresh and clearing cached data, ahead of the traceback. There is deliberately no reload button: a reload triggered from JS cannot bypass the cache, so it would do the one thing that has already failed. The alert is also laid out in one fixed-width column, so its position no longer depends on how long the traceback happens to be.
shiny's sourceUTF8 catches the parse error with try() and then raises a bare
"Error sourcing <file>", discarding the line, the offending source line and the
caret. In the full editor that detail still reached the terminal, but a
viewer-only embed has no terminal, so a typo gave the reader nothing but a file
path.
.start_app now parses the app's .R files before shiny sees them, so the real
parse error travels the return-status path added in the previous commit:
/home/web_user/app_.../app.R:7:1: unexpected symbol
6:
7: server
^
It runs before the VFS mount and the package installs, so a typo fails in about
two seconds rather than after work the app cannot use.
Non-recursive on purpose. Recursing would also cover shiny's auto-sourced R/
directory, but it would risk failing an app over a stray .R file that shiny never
sources; a syntax error below the top level still falls back to the old message,
which is no worse than before.
Runtime errors during sourcing were never affected: parse() succeeds for those,
so their own message already propagated.
medium finding from PR Claude code review setupPythonEnv() runs once loadPyodide() has resolved, so an error from it reached the worker's catch with pyodide defined but pyUtils still undefined. Formatting the message via pyUtils threw a TypeError out of the handler, so the reply was never posted and postMessageAsync — which has no reject path and no timeout — waited forever. That is the hang 7ad6bd8 removed, one step later in init. Guard on pyUtils as well, and reset pyodideStatus when init throws so a later init starts over rather than reporting success with a pyodide that was never built.
Code review flagged that the reachability check treats any HTTP 200 as a usable engine. A response that arrives but cannot be instantiated is handed to loadPyodide() / webR.init(), neither of which settles, so the loader spins with no error and no way out — the failure the check exists to prevent. A captive portal's sign-in page, a single-page app's index.html fallback and a truncated file all reach it as 200. Read the first four bytes and require the wasm magic number. The transfer is cancelled as soon as they arrive, so this still costs a round trip rather than the 10-18 MB file. Anything unreadable — no body, no getReader, a failure mid-read — returns no opinion rather than reporting a load failure, since this runs on the path that decides whether loading may proceed and must not become a new way for it to stop. A valid module that is the wrong build, such as a stale cached one, has the right magic number and still hangs. Closing that needs a load timeout and is left open.
Code review on the loader-status work flagged that RecoveryHint rendered for app-level errors as well as engine errors: a typo in the user's own app produced a prominent panel telling them to hard-refresh and clear their cached site data. The engine had already loaded in that case, so the cache is not the suspect, and the advice points away from the traceback sitting right below it. Pull the failure screen out into ViewerError, which takes the failure class as a prop, and gate the hint on the engine case. The headline was already conditional; the hint just wasn't.
Code review flagged that the try/catch around engine init also spanned initShiny, terminalInterface.clear() and the showStartBanner print, so a throw from any of them was recorded as stage "failed". That state is terminal in the store and outranks every other viewer state, so a cosmetic step failing on an otherwise healthy engine permanently replaced a working app with "Error loading Python!". Latent only because showStartBanner is hardcoded false. Split on readiness rather than on the init call alone. initShiny and initRShiny stay inside the catch — initRShiny runs library(shiny), and an engine that cannot load shiny is genuinely unusable, so moving it out would trade a wrong error screen for a silent hang when the rejection is swallowed by the caller. The terminal clear and the banner print move out, into a catch that only logs. Verified with a browser against both structures, forcing showStartBanner on and making the banner raise: before, the error screen; after, the app runs with the error logged.
_files() and sabotage() fell through to WORKING_APP / a no-op for any mode string that didn't match a known case, so a typo in a later positive-assertion test would exercise nothing and pass for the wrong reason. Guard both against MODES and add a test pinning the failure.
Adds test_slow_load_announces_its_stages, parametrized over engine, asserting the loader announces Downloading -> Starting -> ready in order and that the text stays hidden for the first ~3s (STATUS_DELAY_MS). A first pass followed the plan's illustrative sequential expect() calls and failed on every run: sabotage()'s route delay blocks this whole process's Playwright connection while it sleeps, the engine hits that sabotaged route twice before it's up, and a fast local engine can finish booting inside that blocked window -- so by the time a live expect() got a chance to poll, the text it wanted was already gone for good. Switched to a MutationObserver installed via page.add_init_script() before either delay can fire, which timestamps stage changes in the browser (immune to how blocked this process gets) for the test to read back once the load completes.
The pre-3s check compared two recorded timestamps (elapsed time between the loader mounting and the first stage text appearing), which is a wall-clock duration assertion -- exactly what the spec for this test forbids, and it would go red on a slow CI runner for reasons unrelated to the behaviour under test. Replaced with an absence check: the loader's first "mounted" log entry must show no stage text yet, which states what STATUS_DELAY_MS actually guarantees without comparing timestamps, and still can't pass vacuously because next() raises on an empty log.
Adds an embed_page fixture (no built page ships a .shinylive-python/ .shinylive-r block, so none was reusable) that writes a minimal page next to _shinylive/<engine>/shinylive-sw.js, and two representative failure modes (app-syntax, engine-load) across both engines -- the failure detection is identical to the full-page layout, so only the error screen's presentation in a small block is worth re-checking.
…disk writes The app-syntax assertions were engine-agnostic (headline, no recovery hint, non-empty log), so they could not have caught a .shinylive-r block silently booting Pyodide -- the R fixture's source also fails to compile as Python. Assert the engine-specific filename the error log actually names instead (app.R vs app.py), verified by simulating the mixed-engine case directly. Also replace the embed_page fixture's write into _shinylive/<engine>/ (live build output, non-cleaning overlay copy) with page.route() fulfilling a virtual URL under the same origin, so a crash mid-test can no longer leave a stray file to ride into a later build or deploy.
scripts/loader-demo.ts was a 1073-line local-only script for watching and recording the loader's stages and failure modes by hand; tasks 4-10 replaced its scenarios with 19 automated tests in tests/test_loader_status.py. Delete the script and its .loader-demo/ scratch directory, and document the Playwright-native equivalents (--headed/--slowmo, --video, --tracing) in tests/README.md so the capability isn't lost with the script.
Seven comment/one-token fixes from the final whole-branch review: treat an empty R error message as absent in Viewer's message fallback, rename the app-syntax fixture's second file so .start_app's parse guard actually iterates over it, replace dead scripts/loader-demo.ts citations with the real requirements they were standing in for, drop a false claim about R's own error printing, correct a docstring that overclaimed no browser is used, rewrite a test rationale comment to match what the store actually guarantees, and fence three bare command blocks in tests/README.md.
da9365a to
1016582
Compare
|
@cpsievert this should be ready for re-review. The PR is now covered using the infrastructure setup in #246 and #247. Added 37 jest unit tests covering the pure logic: Coverage output (100%)
Added 19 pytest + Playwright tests based off how I was manually testing using the demo script ( One thing to flag: The new tests don't gate the deploy. They run in their own workflow, For reference, |
They cost about as much as every other site test together, and they have no CI history yet, so they run alongside the build job rather than inside it. That means they do not gate the deploy; the comment on the job records what would have to change to make them. make test-site and make test-loader are now halves of a pair, splitting on the loader marker.
test_r_unresolvable_library_still_runs opts out of fail_on_page_errors and collects console errors itself, but reimplemented the collection without the fixture's forgiveness, so the aborted tag-manager requests failed it. They only appear where GOOGLE_TAG_MANAGER_ID is set, which build.yml does and a local run does not, so this was invisible until CI ran it. The predicate now lives in shinylive_app.py and both callers share it.
Same reasoning test-apps.yml gives for the example tests: slow browser tests should not sit in front of a deploy. The other half of the site marker stays in build.yml, where a failure still stops the deploy. Also gets the tests a cancel-in-progress concurrency group, which build.yml cannot have without risking a cancelled deploy.
The status-list change added four defensive branches that neither suite could reach: jest could not, because the function was unexported inside a component file, and pytest could not, because .start_app always returns a well-formed list. They are the branches a webR upgrade would break -- Viewer.tsx reads status through the same function, so a shape change makes a successful app start report itself as a failure. Moving it to its own module is enough to reach them. No behaviour change; the R browser tests are the regression check.
Trims rationale that restated the code, or that justified a decision by contrasting it with an alternative the reader cannot see -- the CSS centring notes, the workflow headers, and .start_app's return shape among them. Corrects two comments that had gone stale rather than merely long: load-status.test.ts cited a "Fix 1" that exists in no document, and .start_app's described a "spurious ready transition" that the current code cannot produce. Also adds a one-line purpose comment to useLoadStatus.ts, moves Callable, Iterator and Mapping to collections.abc in the two test modules, and formats r-status.test.ts with prettier. No behaviour change.
2 participants
Problem
On a first visit to a site using shinylive, the app-loading overlay shows the pulsing hex animation with no text. Behind the scenes, a multi-megabyte Pyodide or webR engine is downloaded plus package installs, which can take tens of seconds (or longer on slow connections), so a slow first load doesn't give any feedback about if something useful is happening during that time. Generally speaking, I wanted to address "could the shinylive loader show a bit more help text on first load?"
Related: #192 (long loading times) describes the underlying slowness, which this does not attempt to fix.
What changed
Two things, primarily:
1. The overlay says what it is doing. Under the existing hex animation:
Downloading Python…→Starting Python…→Loading packages and starting app…Downloading R…,Starting R…)The text only appears after a 3 second delay, so a fast load (from cache) looks essentially as it did before.
A store implemented per engine in
src/load-status.tskeeps track of the various loading stages.2. Failures produce an error screen instead of just endlessly pulsing.
The following conditions were tested:
pyodide-lock.json404)Error starting app!, naming the line and showing the offending source lineSpecifically, these limitations came from:
loadPyodide()nor webR'sinit()settles when the core wasm app cannot be instantiated; its promise is neither resolved nor rejected, so there were no error signals the page can catch.src/engine-load-guard.tschecks that at least the wasm file is fetchable before the engine starts, and fails early if not.WebWorkerPyodideProxy.init()discarded the worker's error reply, so a Pyodide that never started would appear successful..start_app(inuseWebR.tsx) is evaluated withcaptureConditions = FALSE: with this setting an R error makesevalRresolve rather than reject, so a raised error would go to the terminal and never reach JavaScript..start_apptherefore reports its outcome by returning a value. It returnslist(status = "ok"), orlist(status = "error", message, class, call)on failure. An earlier revision of this PR returned""for success and the message for failure, which made a condition carrying an empty message indistinguishable from success; the status list removes that.Viewer.tsxreads it withevalR+toJs()through the shelter it already opens and purges, rather thanevalRString. The condition'sclassnow reaches the console for diagnosis, andcallis reported only when it names the author's own code — shiny wraps every app body in..stacktraceon..(), so a top-level failure's raw call is the whole app source, and that wrapper is filtered out. FlippingcaptureConditionstoTRUEwould also have fixed the original problem, but it changes error handling for every R call rather than just app startup. Package installation stays non-fatal on purpose — see the R note under Testing.sourceUTF8catches the parse error and raises onlyError sourcing <file>, discarding the detail..start_appnow quickly parses the app's top-level.Rfiles before shiny sees them, so the real parse error including line, column, offending line and caret, reaches the viewer. It runs before the VFS mount and package installs, so a typo fails in a couple of seconds instead of after work the app cannot use.One known failure-mode limitation
An engine asset that downloads but cannot be instantiated, for example a truncated file, or an HTML error page served with 200, will still cause the loading hexes to spin with no error screen. Closing it needs either byte-level validation or a timeout. I had a timeout implemented, but removed it because it was a messy catch for a rare problem and definitely overengineered
Architecture
The
load-status.tsobservable store controls the "status" of the engine and app load.App.tsxwrites into the store viainitPyodideorinitWebR, and thenViewer.tsxresponds to changes in the store to control what's shown to the user:flowchart TB A["<b>App.tsx</b><br/>starts engine init, once per page"] B["<b>initPyodide / initWebR</b><br/>guard the asset, then boot the engine"] S["<b>load-status.ts</b><br/>one store per engine"] H["<b>useLoadStatus.ts</b><br/>useSyncExternalStore"] V["<b>Viewer.tsx</b>"] L["LoadingStatus.tsx<br/>stage text, after 3s"] E["ViewerError<br/>failure screen"] A --> B B -- "set(stage)" --> S S -- "subscribe" --> H H --> V V -- "loading" --> L V -- "failed" --> E classDef store fill:#fde8c8,stroke:#b8860b class S storeStages are linear and one-way:
engine-download→engine-start→ready, orfailed, which is terminal and causes an error to be displayed in the UI.How a failure reaches the screen
Failures were previously just silently discarded and the loading icon would endlessly pulse. Now, error sources fan into two distinct types of errors:
flowchart LR G["<b>guard</b><br/>wasm 404, host unreachable,<br/>not a wasm module"] W["<b>worker</b><br/>pyodide init error reply<br/><i>pyodide-proxy.ts:388</i>"] P["<b>engine init</b><br/>load_python_pre / load_r_pre,<br/>initShiny / initRShiny throws"] PA["<b>Python app</b><br/>_start_app throws<br/><i>Viewer.tsx:381</i>"] RA["<b>R app</b><br/>.start_app returns a status list<br/><i>Viewer.tsx:331</i>"] C["<b>cosmetic</b><br/>terminal clear, start banner"] F["stage = <b>failed</b><br/><i>terminal</i>"] X["appRunningState<br/>= <b>errored</b>"] LOG(["console.error only<br/><i>app keeps running</i>"]) E1["<b>ViewerError kind='engine'</b><br/><i>+ recovery hint</i>"] E2["<b>ViewerError kind='app'</b><br/><i>traceback only</i>"] G --> F W --> F P --> F PA --> X RA --> X C --> LOG F --> E1 X --> E2 classDef prior fill:#f2f2f2,stroke:#aaa,color:#666 classDef dead fill:#eef6ee,stroke:#8ab88a,color:#456 class PA prior class LOG deadThe python app failure was the only one that already reached the screen before this PR. Everything else was silent.
The engine/app split determines the advice that gets shown to the user. The hard-refresh / clear-cache hint appears only on the engine side. On the app side the engine already loaded fine, so the cache isn't the suspect and the hint would point the reader away from the traceback sitting right below it (commit
6f99001).Example Recordings
The following screen recordings demonstrate what this looks like in a few different scenarios, both with throttling to simulate a slow connection, and on a fast connection. There are both R and Python examples here, in an "embed"/Quarto type display, as well as the full screen height version like on https://shinylive.io/py/examples/
python-full_example-throttled.mp4
r-full_example-throttled.mp4
python-full_example-nonthrottled.mp4
r-full_example-nonthrottled.mp4
python-quarto_embed-throttled.mp4
r-quarto_embed-throttled.mp4
python-quarto_embed-nonthrottled.mp4
r-quarto_embed-nonthrottled.mp4
Testing
Both test layers this PR needed landed while it was open: #246 added the jest unit harness and a
tsc --noEmitCI gate, and #247 moved browser testing to pytest + Playwright undertests/. This PR uses both, so the manual verification below is now automated.Unit tests — 37, jest.
src/load-status.test.tscovers the status store: subscription semantics thatuseSyncExternalStoredepends on, per-engine isolation, andfailedbeing terminal.src/engine-load-guard.test.tscovers the reachability and wasm-magic-number checks against a stubbedfetch, including a magic number split across chunks, a body that cannot be cancelled, and one that throws mid-read. Coverage over the modules these import is 100% on all four metrics.Browser tests — 19, pytest + Playwright,
-m loader.tests/test_loader_status.pydrives every mode in the table below end to end against the built site, for both engines, in both the full-page and embedded layouts. App source travels in the URL hash, so app-level failures need no interception; engine-level ones usepage.route(). Two choices worth a reviewer's eye: the staged-loading test records transitions with an in-pageMutationObserverrather than polling assertions, because a delay injected in a sync route handler blocks Playwright's own connection thread; and the delay is a--loader-delayoption rather than CDP throttling, so assertions are on the observed order of stages and never on elapsed time.scripts/loader-demo.ts, the local script behind the recordings above, is deleted.tests/README.mddocuments the equivalent recipes —--headed --slowmo,--video on,--tracing onwithplaywright show-trace, and turning--loader-delayup to watch the stages by hand.The table below was the original manual verification; every row now has a test.
Error starting app!+ tracebackError starting app!+app.R:7:1: unexpected symbolwith the offending line and caret (new — previously silent in an embed)Error starting app!+ micropip'sValueErrorOn the R dependency case. R has no
requirements.txt; dependencies are inferred from the source byrenv::dependencies(), and a missing one is tolerated.library()is shimmed by webR (environmentNamereportswebr) and returns normally instead of erroring, so the app really does start. That appears to be upstream's intended behaviour, which may also be why.start_appdoes not treat a failed install as fatal —renv::dependencies()reports packages that are never used (alibrary()call insideif (FALSE)is reported, and that app runs fine), so failing on it could break working apps.CI placement. The loader tests carry both the
siteandloadermarkers, and the Make targets split on that:make test-siteis-m "site and not loader",make test-loaderis-m loader. Between them they cover everysitetest, so running one is not running them all.make test-sitestays where it was, inbuild.yml's build job, so a failure in the editor, URL-loading or static-export tests still stops the deploy that follows. The loader tests move to a newtest-loader.yml, for the same reasontest-apps.ymlexists: they are slow — nearly every one boots a real Pyodide or webR instance, so the suite costs about as much as all the other site tests together — and they should not sit in front of an S3 deploy. Being a separate workflow also gets them acancel-in-progressconcurrency group, whichbuild.ymlcannot have without risking a cancelled deploy.They therefore do not gate anything yet, which is deliberate and worth revisiting once they have some green history. CI has already earned that caution: the first run of this branch failed on an intermittent console-error assertion that passes locally and passed on the next run, which is exactly what gating would have turned into a blocked deploy. That bug is fixed — the test was collecting console errors itself after opting out of
fail_on_page_errors, but had reimplemented the collection without the fixture's forgiveness for the tag-manager requestsblock_analyticsaborts, which only exist whereGOOGLE_TAG_MANAGER_IDis set. The predicate now lives in one place and both callers share it.Moving the gate later means either folding the marker back into
make test-site, or triggering the deploy fromtest-loader.yml's success viaworkflow_run.Verification.
make type-checkclean;npm run test:unit328/328;npm run test:unit:coverage100% on all four metrics;venv/bin/pytest tests -m site34/34.