From dcbea1c0eed0d5c5adce9f188253788285f00370 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Mon, 5 Oct 2026 09:26:55 -0400 Subject: [PATCH 1/6] test: Check on Windows that busy indicators show while outputs render A Playwright test records when the browser receives each output's "recalculating" and "recalculated" messages, and fails when they arrive together (#1381). A one-off workflow runs it on windows-latest against main, main without the #1388 yields (the negative control), this branch, and this branch without its remaining yield. --- .../windows-busy-indicator-check.yaml | 91 +++++++++++++++++++ .../bugs/1381-busy-indicator-progress/app.py | 38 ++++++++ .../test_busy_indicator_progress.py | 70 ++++++++++++++ 3 files changed, 199 insertions(+) create mode 100644 .github/workflows/windows-busy-indicator-check.yaml create mode 100644 tests/playwright/shiny/bugs/1381-busy-indicator-progress/app.py create mode 100644 tests/playwright/shiny/bugs/1381-busy-indicator-progress/test_busy_indicator_progress.py diff --git a/.github/workflows/windows-busy-indicator-check.yaml b/.github/workflows/windows-busy-indicator-check.yaml new file mode 100644 index 000000000..d54a8c455 --- /dev/null +++ b/.github/workflows/windows-busy-indicator-check.yaml @@ -0,0 +1,91 @@ +# One-off check for #1381 (busy indicators that do not show on Windows) against the +# concurrency changes in #2508. Each variant runs the busy-indicator progress test +# five times on Windows: +# +# - main: the released behavior, with both #1388 yields. +# - main-without-yields: main with both yields removed. This is the negative +# control: the test must fail here, or the runner does not reproduce #1381. +# - this-branch: the #2508 stack, which keeps only the yield after each effect run. +# - this-branch-without-yield: the #2508 stack with that yield removed too. +name: Windows busy indicator check (#1381) + +on: + push: + branches: ["jat255/2508-windows-busy-indicator-check"] + workflow_dispatch: + +jobs: + busy-indicator: + name: ${{ matrix.target }} + runs-on: windows-latest + timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + target: + - main + - main-without-yields + - this-branch + - this-branch-without-yield + steps: + - name: Check out the code under test + uses: actions/checkout@v6 + with: + ref: ${{ startsWith(matrix.target, 'main') && 'main' || github.sha }} + fetch-depth: 0 + + - name: Check out the test + uses: actions/checkout@v6 + with: + path: .harness + + - name: Remove the #1381 yields + if: endsWith(matrix.target, 'yields') || endsWith(matrix.target, 'yield') + shell: python + run: | + import pathlib + import re + import sys + + files = ["shiny/reactive/_reactives.py"] + if "${{ matrix.target }}".startswith("main"): + files.append("shiny/session/_session.py") + for name in files: + path = pathlib.Path(name) + text, count = re.subn( + r"(# https://github\.com/posit-dev/py-shiny/issues/1381\n)" + r"[ \t]*await asyncio\.sleep\(0\)\n", + r"\1", + path.read_text(), + ) + if count != 1: + sys.exit(f"{name}: expected 1 yield, found {count}") + path.write_text(text) + print(f"{name}: removed the yield") + + - name: Setup py-shiny + uses: ./.github/py-shiny/setup + with: + python-version: "3.12" + + - name: Add the test + shell: bash + run: | + cp -r .harness/tests/playwright/shiny/bugs/1381-busy-indicator-progress \ + tests/playwright/shiny/bugs/ + + - name: Install chromium + shell: bash + run: playwright install chromium + + - name: Run the test five times + shell: bash + run: | + status=0 + for i in 1 2 3 4 5; do + echo "::group::Run $i" + pytest -p no:xdist -o addopts="" -s --browser chromium \ + tests/playwright/shiny/bugs/1381-busy-indicator-progress || status=1 + echo "::endgroup::" + done + exit $status diff --git a/tests/playwright/shiny/bugs/1381-busy-indicator-progress/app.py b/tests/playwright/shiny/bugs/1381-busy-indicator-progress/app.py new file mode 100644 index 000000000..53ed62ec7 --- /dev/null +++ b/tests/playwright/shiny/bugs/1381-busy-indicator-progress/app.py @@ -0,0 +1,38 @@ +import time + +from shiny import App, Inputs, render, ui + +app_ui = ui.page_fluid( + ui.input_action_button("rerender", "Re-render"), + ui.output_text("out1"), + ui.output_text("out2"), + ui.output_text("out3"), + ui.output_text("out4"), +) + + +def server(input: Inputs): + def slow_value() -> str: + n = input.rerender() + # Block the event loop, as the plots in #1381 did. + time.sleep(0.5) + return str(n) + + @render.text + def out1(): + return slow_value() + + @render.text + def out2(): + return slow_value() + + @render.text + def out3(): + return slow_value() + + @render.text + def out4(): + return slow_value() + + +app = App(app_ui, server) diff --git a/tests/playwright/shiny/bugs/1381-busy-indicator-progress/test_busy_indicator_progress.py b/tests/playwright/shiny/bugs/1381-busy-indicator-progress/test_busy_indicator_progress.py new file mode 100644 index 000000000..26e9548e7 --- /dev/null +++ b/tests/playwright/shiny/bugs/1381-busy-indicator-progress/test_busy_indicator_progress.py @@ -0,0 +1,70 @@ +""" +Each output's "recalculating" message must reach the browser while the output +renders, so that its busy indicator shows. In #1381, on Windows, the messages for +most outputs arrived together with the finished values. +""" + +import json +from typing import cast + +from playwright.sync_api import Page + +from shiny.playwright import controller +from shiny.run import ShinyAppProc + +OUTPUTS = ["out1", "out2", "out3", "out4"] +# Each output blocks the event loop for 0.5 s while it renders (see app.py). +MIN_GAP_MS = 250 + +# Records when the browser receives each websocket message. +RECORD_MESSAGES = """ +window.__shinyMessages = []; +window.WebSocket = class extends window.WebSocket { + constructor(...args) { + super(...args); + this.addEventListener("message", (e) => { + window.__shinyMessages.push([performance.now(), e.data]); + }); + } +}; +""" + + +def status_times(page: Page) -> dict[tuple[str, str], float]: + """The first time that each (output, status) message was received, in ms.""" + times: dict[tuple[str, str], float] = {} + messages: list[tuple[float, object]] = page.evaluate("window.__shinyMessages") + for t, data in messages: + if not isinstance(data, str): + continue + try: + msg: object = json.loads(data) + except ValueError: + continue + if not isinstance(msg, dict): + continue + recalc = cast("dict[str, object]", msg).get("recalculating") + if isinstance(recalc, dict): + status = cast("dict[str, str]", recalc) + times.setdefault((status["name"], status["status"]), t) + return times + + +def test_recalculating_messages_arrive_while_outputs_render( + page: Page, local_app: ShinyAppProc +) -> None: + page.add_init_script(RECORD_MESSAGES) + page.goto(local_app.url) + controller.OutputText(page, "out4").expect_value("0", timeout=10_000) + + page.evaluate("window.__shinyMessages = []") + controller.InputActionButton(page, "rerender").click() + controller.OutputText(page, "out4").expect_value("1", timeout=10_000) + + times = status_times(page) + gaps = { + name: times[(name, "recalculated")] - times[(name, "recalculating")] + for name in OUTPUTS + } + print("ms between recalculating and recalculated:", gaps) + assert all(gap >= MIN_GAP_MS for gap in gaps.values()), gaps From e515c51f4b07907cfc4f32b7e3aafa283f2e9b57 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Mon, 5 Oct 2026 09:34:11 -0400 Subject: [PATCH 2/6] fix(session): Send each output's progress message before it renders On Windows, a websocket write made while an earlier write is still in flight goes out only when the event loop runs again. A synchronous renderer blocks the loop, so the output's "recalculating" message reached the client together with its value and the busy indicator never showed (#1381). A yield after the message lets it go out first. --- .../windows-busy-indicator-check.yaml | 52 ++++++++----------- shiny/session/_session.py | 6 +++ 2 files changed, 28 insertions(+), 30 deletions(-) diff --git a/.github/workflows/windows-busy-indicator-check.yaml b/.github/workflows/windows-busy-indicator-check.yaml index d54a8c455..00ec6150f 100644 --- a/.github/workflows/windows-busy-indicator-check.yaml +++ b/.github/workflows/windows-busy-indicator-check.yaml @@ -1,12 +1,9 @@ -# One-off check for #1381 (busy indicators that do not show on Windows) against the -# concurrency changes in #2508. Each variant runs the busy-indicator progress test -# five times on Windows: +# One-off check for #1381 (busy indicators that do not show on Windows). Each +# variant runs the busy-indicator progress test ten times on Windows: # -# - main: the released behavior, with both #1388 yields. -# - main-without-yields: main with both yields removed. This is the negative -# control: the test must fail here, or the runner does not reproduce #1381. -# - this-branch: the #2508 stack, which keeps only the yield after each effect run. -# - this-branch-without-yield: the #2508 stack with that yield removed too. +# - main: the released behavior. +# - this-branch: a yield after each output's "recalculating" message. +# - this-branch-no-yield: the same without that yield. The test must fail here. name: Windows busy indicator check (#1381) on: @@ -24,14 +21,13 @@ jobs: matrix: target: - main - - main-without-yields - this-branch - - this-branch-without-yield + - this-branch-no-yield steps: - name: Check out the code under test uses: actions/checkout@v6 with: - ref: ${{ startsWith(matrix.target, 'main') && 'main' || github.sha }} + ref: ${{ matrix.target == 'main' && 'main' || github.sha }} fetch-depth: 0 - name: Check out the test @@ -39,29 +35,25 @@ jobs: with: path: .harness - - name: Remove the #1381 yields - if: endsWith(matrix.target, 'yields') || endsWith(matrix.target, 'yield') + - name: Remove the yield after the "recalculating" message + if: matrix.target == 'this-branch-no-yield' shell: python run: | import pathlib import re import sys - files = ["shiny/reactive/_reactives.py"] - if "${{ matrix.target }}".startswith("main"): - files.append("shiny/session/_session.py") - for name in files: - path = pathlib.Path(name) - text, count = re.subn( - r"(# https://github\.com/posit-dev/py-shiny/issues/1381\n)" - r"[ \t]*await asyncio\.sleep\(0\)\n", - r"\1", - path.read_text(), - ) - if count != 1: - sys.exit(f"{name}: expected 1 yield, found {count}") - path.write_text(text) - print(f"{name}: removed the yield") + path = pathlib.Path("shiny/session/_session.py") + text, count = re.subn( + r"(# https://github\.com/posit-dev/py-shiny/issues/1381\n)" + r"[ \t]*await asyncio\.sleep\(0\)\n", + r"\1", + path.read_text(), + ) + if count != 1: + sys.exit(f"{path}: expected 1 yield, found {count}") + path.write_text(text) + print(f"{path}: removed the yield") - name: Setup py-shiny uses: ./.github/py-shiny/setup @@ -78,11 +70,11 @@ jobs: shell: bash run: playwright install chromium - - name: Run the test five times + - name: Run the test ten times shell: bash run: | status=0 - for i in 1 2 3 4 5; do + for i in $(seq 10); do echo "::group::Run $i" pytest -p no:xdist -o addopts="" -s --browser chromium \ tests/playwright/shiny/bugs/1381-busy-indicator-progress || status=1 diff --git a/shiny/session/_session.py b/shiny/session/_session.py index 24be5b102..e2ad5b876 100644 --- a/shiny/session/_session.py +++ b/shiny/session/_session.py @@ -2952,6 +2952,12 @@ async def output_obs(): await session._send_message( {"recalculating": {"name": output_name, "status": "recalculating"}} ) + # Let the message go out before a synchronous renderer blocks the event + # loop, so that the output's busy indicator shows. On Windows, a write + # made while an earlier write is still in flight goes out only when the + # loop runs again. + # https://github.com/posit-dev/py-shiny/issues/1381 + await asyncio.sleep(0) try: async with shiny_otel_span( From 7bac46e7fcb56831c24b94298698f71b164fd069 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Mon, 5 Oct 2026 09:42:32 -0400 Subject: [PATCH 3/6] docs: Cite #1381 in the changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f85aa447f..39c1d7ee1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Bug fixes +* On Windows, an output's busy indicator now shows while the output renders. Before, when a synchronous render function blocked the event loop, the message that turns on the indicator reached the browser together with the output's new value, so most outputs never showed it. (#1381) + * Setting a reactive value from a download handler (`@render.download_button`) now updates the outputs and effects that read it right away, including while a streamed download is still sending. The session also keeps handling input during a streamed download. Before, they updated only after the next message from the client. (#1785) * `near_points(add_dist=True)` now adds the `dist_` column its documentation describes, instead of a column named `dist`. Shiny for R names it `dist_` as well, and the trailing underscore is what keeps it from colliding with a `dist` column of the caller's own data. Code reading `df["dist"]` from the result must read `df["dist_"]`. (#2510) From a8272c2b098ed201939d6703f279785a5bae4be1 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Mon, 5 Oct 2026 09:49:49 -0400 Subject: [PATCH 4/6] ci: Run Windows-specific end-to-end tests on Windows The other Playwright jobs run on Ubuntu, where the #1381 busy-indicator test passes with or without its fix. A Windows job runs it where it can fail, and the PR gate requires it. This replaces the one-off check workflow. --- .github/workflows/pytest.yaml | 21 +++++ .../windows-busy-indicator-check.yaml | 83 ------------------- 2 files changed, 21 insertions(+), 83 deletions(-) delete mode 100644 .github/workflows/windows-busy-indicator-check.yaml diff --git a/.github/workflows/pytest.yaml b/.github/workflows/pytest.yaml index 0dba68130..2606da8a4 100644 --- a/.github/workflows/pytest.yaml +++ b/.github/workflows/pytest.yaml @@ -252,6 +252,26 @@ jobs: docker inspect --format '{{json .State}}' playwright || true docker logs playwright || true + # End-to-end tests for behavior that differs on Windows. The other Playwright + # jobs run on Ubuntu, where these tests pass either way. For example, on + # Windows a websocket write made while an earlier write is still in flight goes + # out only when the event loop runs again (#1381). + playwright-windows: + if: github.event_name != 'release' + runs-on: windows-latest + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 + - name: Setup py-shiny + uses: ./.github/py-shiny/setup + + - name: Run Windows-specific End-to-End tests + timeout-minutes: 10 + shell: bash + run: | + make playwright-shiny SUB_FILE="bugs/1381-busy-indicator-progress" PLAYWRIGHT_BROWSERS=chromium PYTEST_BROWSERS="--browser chromium" + playwright-examples: if: github.event_name != 'release' runs-on: ubuntu-latest @@ -467,6 +487,7 @@ jobs: - pyright - oldest-deps - playwright-shiny + - playwright-windows - playwright-examples - playwright-ai - playwright-deploys-precheck diff --git a/.github/workflows/windows-busy-indicator-check.yaml b/.github/workflows/windows-busy-indicator-check.yaml deleted file mode 100644 index 00ec6150f..000000000 --- a/.github/workflows/windows-busy-indicator-check.yaml +++ /dev/null @@ -1,83 +0,0 @@ -# One-off check for #1381 (busy indicators that do not show on Windows). Each -# variant runs the busy-indicator progress test ten times on Windows: -# -# - main: the released behavior. -# - this-branch: a yield after each output's "recalculating" message. -# - this-branch-no-yield: the same without that yield. The test must fail here. -name: Windows busy indicator check (#1381) - -on: - push: - branches: ["jat255/2508-windows-busy-indicator-check"] - workflow_dispatch: - -jobs: - busy-indicator: - name: ${{ matrix.target }} - runs-on: windows-latest - timeout-minutes: 30 - strategy: - fail-fast: false - matrix: - target: - - main - - this-branch - - this-branch-no-yield - steps: - - name: Check out the code under test - uses: actions/checkout@v6 - with: - ref: ${{ matrix.target == 'main' && 'main' || github.sha }} - fetch-depth: 0 - - - name: Check out the test - uses: actions/checkout@v6 - with: - path: .harness - - - name: Remove the yield after the "recalculating" message - if: matrix.target == 'this-branch-no-yield' - shell: python - run: | - import pathlib - import re - import sys - - path = pathlib.Path("shiny/session/_session.py") - text, count = re.subn( - r"(# https://github\.com/posit-dev/py-shiny/issues/1381\n)" - r"[ \t]*await asyncio\.sleep\(0\)\n", - r"\1", - path.read_text(), - ) - if count != 1: - sys.exit(f"{path}: expected 1 yield, found {count}") - path.write_text(text) - print(f"{path}: removed the yield") - - - name: Setup py-shiny - uses: ./.github/py-shiny/setup - with: - python-version: "3.12" - - - name: Add the test - shell: bash - run: | - cp -r .harness/tests/playwright/shiny/bugs/1381-busy-indicator-progress \ - tests/playwright/shiny/bugs/ - - - name: Install chromium - shell: bash - run: playwright install chromium - - - name: Run the test ten times - shell: bash - run: | - status=0 - for i in $(seq 10); do - echo "::group::Run $i" - pytest -p no:xdist -o addopts="" -s --browser chromium \ - tests/playwright/shiny/bugs/1381-busy-indicator-progress || status=1 - echo "::endgroup::" - done - exit $status From a6f654b2c358ca501aabd43d5f895e74267d9eff Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Mon, 5 Oct 2026 09:58:38 -0400 Subject: [PATCH 5/6] test: Allow the first render to run after session_start ends An output now yields before it renders, so the first render of span_summary can run after the session_start span has closed. The test still checks that the span closes. --- tests/playwright/shiny/otel/express-session-start/app.py | 8 ++++---- .../express-session-start/test_express_session_start.py | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/playwright/shiny/otel/express-session-start/app.py b/tests/playwright/shiny/otel/express-session-start/app.py index e05274cda..c4cf3a734 100644 --- a/tests/playwright/shiny/otel/express-session-start/app.py +++ b/tests/playwright/shiny/otel/express-session-start/app.py @@ -5,10 +5,10 @@ re-executed for every new session, but it only reads from the exporter — it never replaces the TracerProvider. -On initial page load the span_summary output shows 0 session_start spans -because session_start hasn't ended yet (the initial flush runs inside it). -After clicking "Show Session Spans" the re-render fires outside session_start, -so the now-closed span is visible in the exporter. +The initial flush runs inside session_start, but an output yields before it +renders, so on initial page load span_summary can show 0 or 1 session_start +spans. After clicking "Show Session Spans" the re-render fires outside +session_start, so the now-closed span is visible in the exporter. """ import json diff --git a/tests/playwright/shiny/otel/express-session-start/test_express_session_start.py b/tests/playwright/shiny/otel/express-session-start/test_express_session_start.py index 1131096db..2a8c803d1 100644 --- a/tests/playwright/shiny/otel/express-session-start/test_express_session_start.py +++ b/tests/playwright/shiny/otel/express-session-start/test_express_session_start.py @@ -27,9 +27,9 @@ def test_session_start_span_closes(page: Page, local_app: ShinyAppProc) -> None: show_spans_btn = InputActionButton(page, "show_spans") output = OutputCode(page, "span_summary") - # Before clicking, the initial render fires inside session_start's - # reactive_flush (before the span ends), so the count must be 0. - expect(output.loc).to_contain_text('"session_start_count": 0,') + # The initial render can run before or after session_start ends (an output + # yields before it renders), so only wait for it here. + expect(output.loc).to_contain_text('"session_start_count":') # Click after page load so the re-render fires *outside* session_start. show_spans_btn.click() From b91ae416ad3cc4d606ebab63bd417e07112b300a Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Mon, 5 Oct 2026 10:04:30 -0400 Subject: [PATCH 6/6] ci: Skip --with-deps in the Windows end-to-end job On Windows, `playwright install --with-deps` only adds the Media Foundation feature, which takes about 3 minutes and which headless Chromium doesn't need. A new PLAYWRIGHT_INSTALL_ARGS Makefile variable (default `--with-deps`) lets the job install only Chromium's headless shell. --- .github/workflows/pytest.yaml | 4 +++- Makefile | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/pytest.yaml b/.github/workflows/pytest.yaml index 2606da8a4..66282fce4 100644 --- a/.github/workflows/pytest.yaml +++ b/.github/workflows/pytest.yaml @@ -270,7 +270,9 @@ jobs: timeout-minutes: 10 shell: bash run: | - make playwright-shiny SUB_FILE="bugs/1381-busy-indicator-progress" PLAYWRIGHT_BROWSERS=chromium PYTEST_BROWSERS="--browser chromium" + # No `--with-deps`: on Windows it only adds the Media Foundation feature, + # which headless Chromium doesn't need, and that takes about 3 minutes. + make playwright-shiny SUB_FILE="bugs/1381-busy-indicator-progress" PLAYWRIGHT_BROWSERS=chromium PLAYWRIGHT_INSTALL_ARGS=--only-shell PYTEST_BROWSERS="--browser chromium" playwright-examples: if: github.event_name != 'release' diff --git a/Makefile b/Makefile index d94ab70d5..391feb1dd 100644 --- a/Makefile +++ b/Makefile @@ -189,6 +189,8 @@ clean-js: FORCE SUB_FILE:= PYTEST_BROWSERS:= --browser webkit --browser firefox --browser chromium PYTEST_DEPLOYS_BROWSERS:= --browser chromium +# Arguments for `playwright install`, besides the browsers. +PLAYWRIGHT_INSTALL_ARGS:= --with-deps # Per-test timeout (seconds) so a single hung test fails fast with a full # thread-stack dump instead of silently consuming the whole CI job. PLAYWRIGHT_TEST_TIMEOUT:= 120 @@ -236,7 +238,7 @@ install-playwright: FORCE @if [ -n "$$PW_TEST_CONNECT_WS_ENDPOINT" ]; then \ echo "Using remote Playwright server at $$PW_TEST_CONNECT_WS_ENDPOINT"; \ else \ - playwright install --with-deps $(PLAYWRIGHT_BROWSERS); \ + playwright install $(PLAYWRIGHT_INSTALL_ARGS) $(PLAYWRIGHT_BROWSERS); \ fi install-rsconnect: FORCE