Conversation
Greptile SummaryThis PR replaces the LCM-based viewer interaction with a WebSocket server ( The mechanical changes are solid — subscriptions are properly cleaned up via Confidence Score: 3/5Do not merge yet — several P1 issues from prior rounds remain open (port collision, _server_ready restart bug, silent bind failure, misleading foxglove hints); PR is also self-described as not-ready-for-review pending the dimos-viewer companion PR. Multiple open P1 findings from previous review rounds are still present in the code: RERUN_WEB_PORT/gRPC port collision at 9877, _server_ready threading.Event never cleared between start/stop cycles, start() not surfacing bind failures, and misleading --connect hints in foxglove mode. The core WebSocket server design and MovementManager logic are sound, but those unresolved issues pull the score well below the P1 ceiling of 4. dimos/visualization/rerun/websocket_server.py and dimos/visualization/rerun/constants.py need the most attention (port mismatch, restart bug, silent bind failure). Important Files Changed
Reviews (14): Last reviewed commit: "Merge branch 'dev' into jeff/fix/rconnec..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Sorry I forgot to port the project.toml change from rosnav8 back to this branch. Ready now though |
| module = RerunWebSocketServer(port=_E2E_PORT) | ||
| module.start() | ||
| wait_for_server(_E2E_PORT) | ||
| yield module # type: ignore[misc] |
There was a problem hiding this comment.
No need for ignore, we don't type check tests.
| shutil.which("dimos-viewer") is None | ||
| or "--connect" | ||
| not in subprocess.run(["dimos-viewer", "--help"], capture_output=True, text=True).stdout, | ||
| reason="dimos-viewer binary not installed or does not support --connect", |
There was a problem hiding this comment.
Why skip? If dimos-viewer is not present or doesn't support --connect that sounds to me like we should know that. I.e., the test should fail.
| yield publisher # type: ignore[misc] | ||
|
|
||
|
|
||
| # ── Tests ──────────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
| # ── Tests ──────────────────────────────────────────────────────────────── |
|
|
||
| def test_invalid_json_does_not_crash(server: RerunWebSocketServer) -> None: | ||
| """Malformed JSON is silently dropped; server stays alive for the next message.""" | ||
| import websockets.asyncio.client as ws_client |
There was a problem hiding this comment.
Please move to the top.
| manager._on_teleop(_twist(lx=0.3)) | ||
|
|
||
| # Nav is suppressed | ||
| manager.cmd_vel.publish.reset_mock() # type: ignore[union-attr] |
There was a problem hiding this comment.
None of these type ignores are needed. We don't type check tests.
| import time | ||
| from typing import Any | ||
|
|
||
| from dimos_lcm.std_msgs import Bool # type: ignore[import-untyped] |
There was a problem hiding this comment.
Why is this ignore needed? I can see the import in other files without any ignore.
# Conflicts: # dimos/robot/unitree/go2/blueprints/smart/unitree_go2.py # dimos/visualization/rerun/bridge.py
The section-marker test walks REPO_ROOT and was catching personal overlay scripts that live outside the main project tree.
The toolz pipe() function returns Any, which triggers mypy's no-any-return when used in a function with a declared return type.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Address Paul's review nit: inline imports of websockets.asyncio.client moved to module-level imports in test_websocket_server.py and test_viewer_ws_e2e.py.
- Move all inline `import rerun` to top of bridge.py (rerun already loaded via other top-level imports) - Convert wait_for_server from conftest import to pytest fixture - Move websockets import to top of conftest.py - Change RERUN_WEB_PORT from 9090 to 9877 (9090 conflicts with VPN/TOR)
Ruff requires `import X` after `from X import Y` within the same import group. Fixes pre-commit failure.
Revert the .ignore.enhance entry in test_no_sections.py and replace with .hidden. Add .hidden/ to .gitignore for personal/overlay dirs.
3 participants
NOTE: this will become ready-for-review once this dimos-viewer PR is merged
Problem(s)
--connectdoesn't work for remote connections.cmd_velwhich ends up making stream-renaming a pain since everything else must be renamed to know if the cmd_vel is coming from the viewer or from a planner/module.Solution
Use websockets instead of LCM for the viewer.
vis_moduleto de-dup the visualization logicBreaking Changes
None
How to Test
You'll need the
jeff/fix/connectbranch ofdimos-viewercompiled and python installed into dimos:Alternatively
Click in the 3D viewport and use WASD keys — should see
[CLICK]and[TWIST]in terminal 1.Contributor License Agreement