Repository navigation
Conversation
T4rk1n
left a comment
T4rk1n
left a comment
There was a problem hiding this comment.
The fix in _callback.py is right and is all that's needed: returning the sideUpdate payload as a 200 when PreventUpdate is raised gets picked up by the renderer's existing OK path. I ran arb008 against the code before this PR and it fails, and it passes with only the Python change (no renderer rebuild).
The ws.py and callbacks.ts changes never run, though, and test_ws037 passes without the fix. Can you drop those and keep the PR to the _callback.py change plus arb008? Not blocking on correctness, but I'd rather not merge code paths that can't run.
| }); | ||
| } else if (status === STATUS.PREVENT_UPDATE) { | ||
| const contentType = res.headers.get('Content-Type') || ''; | ||
| const hasJsonBody = |
There was a problem hiding this comment.
This branch can't receive a body. PreventUpdate with side updates now comes back as a 200 from _callback.py, and the plain PreventUpdate case is a 204, which has no body by spec (Flask's handler returns "", 204). Can you remove the 204 body parsing and the applySideEffects helper? The OK path already dispatches data.sideUpdate.
| return {"status": "ok", "data": json.loads(response_data)} | ||
|
|
||
| except PreventUpdate: | ||
| side_update = dict(getattr(cb_ctx, "updated_props", {}) or {}) |
There was a problem hiding this comment.
updated_props is always empty here. Over WebSocket, set_props sends straight to the socket instead of writing to updated_props. And if it weren't empty, the async_add_context wrapper would already have caught PreventUpdate and returned the side update before reaching this handler. Can you revert the ws.py changes (same for line 782 and the _prepare_ws_partial return type)?
| assert dash_duo.get_logs() == [] | ||
|
|
||
|
|
||
| def test_ws037_set_props_survives_prevent_update(dash_duo): |
There was a problem hiding this comment.
This test passes on dev without the fix, because WS set_props already streams before PreventUpdate is raised. It doesn't cover anything the PR changes. Can you drop it, or keep it under a name that says it's a regression guard for existing behavior?
| }; | ||
|
|
||
| if (response.status === 'prevent_update') { | ||
| applySideEffects(response.data as CallbackResponseData | undefined); |
There was a problem hiding this comment.
Same as above: response.data is never set on a prevent_update WS response once the ws.py change goes, so this can come out too.
|
I dropped all the websocket and callback.ts adjustments. |
|
T4rk1n
left a comment
T4rk1n
left a comment
There was a problem hiding this comment.
The fix for regular sync and async callbacks is right: the renderer already applies sideUpdate from a 200 response, so returning it in place of the 204 is enough. Streaming callbacks still drop the updates, though (inline). Since the goal is for set_props to always apply under PreventUpdate, I'd like that fixed in this PR. Can you also add an async def version of test_arb008? The async handler changed but only the sync one is tested. CHANGELOG notes inline.
| ) | ||
| except PreventUpdate: | ||
| if _set_side_update(callback_ctx, response): | ||
| return to_json(response) |
There was a problem hiding this comment.
Streaming callbacks need the same fix. In _astream_frames, if the generator calls set_props("side", ...) and then raises PreventUpdate, it hits except (StopAsyncIteration, PreventUpdate): break, and whatever is still in callback_ctx.updated_props never gets sent. Same thing if the generator calls set_props after its last yield and then returns. Can you send any leftover updated_props as a sideUpdate frame before {"done": True}, and add a streaming test for it?
| This project adheres to [Semantic Versioning](https://semver.org/). | ||
|
|
||
| ## [UNRELEASED] | ||
| ## [unreleased] |
There was a problem hiding this comment.
Can you revert this to ## [UNRELEASED]? It isn't related to the fix.
| ### Fixed | ||
| - [#4136](https://github.com/plotly/dash/pull/4136) MCP tools with a generic type annotation such as `list[str]` no longer crash on Python 3.10. | ||
| - [#4114](https://github.com/plotly/dash/pull/4114) Fix the `get_dash_component` MCP tool failing when a component's value contains Dash components, Plotly figures, or numpy arrays. Fixes [#4113](https://github.com/plotly/dash/issues/4113). | ||
| - [#4128](https://github.com/plotly/dash/pull/4128) Fix `set_props` side updates being dropped when a callback raises `PreventUpdate`. Callback responses now preserve and apply `sideUpdate` payloads across HTTP and WebSocket paths, while keeping the no-payload `PreventUpdate` behavior unchanged. |
There was a problem hiding this comment.
This says the fix covers both HTTP and WebSocket, but the diff only changes _callback.py. Can you cut it to something like "Fix set_props updates being dropped when a callback raises PreventUpdate. Fixes #2958."?
This makes it so that Side Updates (
set_props) is always used even whenPreventUpdatewas triggered, this helps make the process more inline with expected behaviour.fixes #2958