diff --git a/CHANGELOG.md b/CHANGELOG.md index fa7a824..9383276 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Fixed the spinner getting stuck after a WebSocket reconnect: spinner messages are now persisted (under a stable `obj_id="spinner"`) like other viewer state, so a reconnecting client sees the current spinner state instead of missing a "stop" it never received. + ### Removed diff --git a/src/compas_threejs/viewer/outbox.py b/src/compas_threejs/viewer/outbox.py index 4dd5362..dd7aaa3 100644 --- a/src/compas_threejs/viewer/outbox.py +++ b/src/compas_threejs/viewer/outbox.py @@ -52,7 +52,15 @@ def send_dict(self, message: dict, *, workspace_id: str = "main", remove_key=Non binary_data = compas_pb.pb_dump_bts(message) dispatch = message.get("dispatch", "") is_remove = dispatch == "handle_geometry" and message.get("type") == "remove" - persist = dispatch not in ("ui", "spinner") and not is_remove + # "ui" messages are one-off toasts - a client reconnecting later shouldn't see a + # stale one replayed. "spinner" is the opposite: it's a *state* (loading or not), + # same idea as camera position/background color above - if a client's connection + # drops mid-load and reconnects, it needs to see the CURRENT spinner state (still + # loading, or long since finished), not silently miss the "stop" it never received + # while disconnected and get stuck showing a spinner forever. See Workspace. + # start_spinner/stop_spinner, which pass the stable obj_id="spinner" that makes + # each call overwrite the previous one instead of piling up as separate entries. + persist = dispatch != "ui" and not is_remove self.send_bytes(binary_data, obj_id or "", persist=persist, workspace_id=workspace_id, remove_key=remove_key) def forget(self, key, *, workspace_id: str = "main"): diff --git a/src/compas_threejs/viewer/workspace.py b/src/compas_threejs/viewer/workspace.py index a4199ac..d3708d8 100644 --- a/src/compas_threejs/viewer/workspace.py +++ b/src/compas_threejs/viewer/workspace.py @@ -593,9 +593,16 @@ def start_spinner(self, message: Optional[str] = None): message : str, optional A short description to display alongside the spinner, e.g. "Loading model...". """ + # obj_id="spinner" - a fixed, stable key (see Outbox.send_dict's own docstring) - so + # this overwrites the *same* persisted slot every call rather than each start/stop + # piling up as its own separate history entry. Combined with "spinner" no longer + # being excluded from persistence (see Outbox.send_dict), a client that reconnects + # mid-load (or right after) always replays whatever the CURRENT spinner state + # actually is, instead of only ever seeing whichever call happened to reach it live. self.app.outbox.send_dict( {"dispatch": "spinner", "visible": True, "message": message}, workspace_id=self.workspace_id, + obj_id="spinner", ) def stop_spinner(self): @@ -603,6 +610,7 @@ def stop_spinner(self): self.app.outbox.send_dict( {"dispatch": "spinner", "visible": False}, workspace_id=self.workspace_id, + obj_id="spinner", ) def open_in_browser(self): diff --git a/tests/test_compas_pb_compatibility.py b/tests/test_compas_pb_compatibility.py index 8685f8f..ac8d87c 100644 --- a/tests/test_compas_pb_compatibility.py +++ b/tests/test_compas_pb_compatibility.py @@ -54,7 +54,10 @@ def test_outbox_command_roundtrip(self): binary_data, object_id, persist, workspace_id, remove_key, broadcast = outbox._queue[0] self.assertEqual(compas_pb.pb_load_bts(binary_data), command) self.assertEqual(object_id, "") - self.assertFalse(persist) + # "spinner" messages are persisted (see test_spinner_persistence.py) - unlike a + # one-off "ui" toast, spinner visibility is a *state*, so a reconnecting client + # must see its current value rather than never finding out it was ever hidden. + self.assertTrue(persist) self.assertEqual(workspace_id, "main") self.assertIsNone(remove_key) self.assertTrue(broadcast) diff --git a/tests/test_spinner_persistence.py b/tests/test_spinner_persistence.py new file mode 100644 index 0000000..fcbab8e --- /dev/null +++ b/tests/test_spinner_persistence.py @@ -0,0 +1,61 @@ +import asyncio +import unittest + +from compas_threejs.viewer.app import App +from compas_threejs.viewer.server import AppServer + + +class SpinnerPersistenceTest(unittest.TestCase): + """ + A client whose WebSocket connection drops mid-load and reconnects must see the + CURRENT spinner state on connect (loading or not), not silently miss whichever + start/stop call happened to be live at the exact moment it was disconnected and + get stuck showing a spinner forever - the bug this fix addresses. Two pieces: + Outbox.send_dict must actually persist "spinner" messages (it used to exclude them, + same as one-off "ui" toasts, which is right for a toast but wrong for a state), and + start_spinner/stop_spinner must use a stable obj_id so each call overwrites the + same persisted slot rather than piling up as separate history entries a reconnect + could replay out of order. + """ + + def test_send_dict_persists_spinner_but_not_ui_messages(self): + # No server running - Outbox.send_bytes queues rather than broadcasting (see + # test_object_lifecycle_dispatch.py's own note on this), which lets the queued + # (obj_id, persist) pair be inspected directly without a live event loop. + app = App() + + app.outbox.send_dict({"dispatch": "spinner", "visible": True}, workspace_id="test", obj_id="spinner") + app.outbox.send_dict({"dispatch": "ui", "text": "hello"}, workspace_id="test") + + persist_by_obj_id = {entry[1]: entry[2] for entry in app.outbox._queue} + self.assertTrue(persist_by_obj_id["spinner"]) + self.assertFalse(persist_by_obj_id[""]) # the "ui" message's obj_id defaults to "" + + def test_workspace_start_and_stop_spinner_use_the_same_stable_obj_id(self): + app = App() + + app.main.start_spinner("Loading...") + app.main.stop_spinner() + + obj_ids = [entry[1] for entry in app.outbox._queue] + self.assertEqual(obj_ids, ["spinner", "spinner"]) + + def test_broadcast_overwrites_the_persisted_spinner_entry_instead_of_piling_up(self): + # Exercises AppServer.broadcast directly - persistence bookkeeping only happens + # there (Outbox.send_bytes just schedules a call to it onto the event loop), and + # it runs fine with no connected clients (persist bookkeeping happens before the + # "any clients?" check), so no live server/WebSocket is needed here either. + server = AppServer() + + asyncio.run(server.broadcast(b"start", obj_id="spinner", persist=True, workspace_id="test")) + self.assertEqual(dict(server.workspace_states["test"]), {"spinner": b"start"}) + + asyncio.run(server.broadcast(b"stop", obj_id="spinner", persist=True, workspace_id="test")) + + # Still exactly one persisted entry, holding the LATEST state - not two entries + # (one stale) that a reconnecting client could replay in the wrong order. + self.assertEqual(dict(server.workspace_states["test"]), {"spinner": b"stop"}) + + +if __name__ == "__main__": + unittest.main()