Repository navigation
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Lash-L
left a comment
Lash-L
left a comment
There was a problem hiding this comment.
Automated code review by Gemini 4 Argon in Antigravity using our /review (/code-review) skill (.agents/skills/code-review/SKILL.md / AGENTS.md).
Summary
Clean, well-scoped V1 trait implementation that keeps WebRTC session state out of the trait while redacting sensitive camera signaling payloads from debug logs.
- Layer 1 (Architecture & Boundaries):
CameraTraitcleanly accepts only its injectedV1RpcChannel, delegates payload encoding/decoding toroborock.protocols.camera_signaling, and satisfiestest_trait_boundary_conformance.py. - Layer 2 (Data Models & Wire Mapping): Command methods,
CameraTurnServer,CameraSdpDescription, andCameraIceCandidatewire shapes match the V1 camera signaling contract, with two optional command parameter notes inline (stop_camera_previewandstart_voice_chat). - Layer 3 (Code Style & Conventions): Modern typing (
X | None), docstrings, and exception chaining (from Noneon TURN parse failures to avoid leaking raw credentials) are consistent with repo standards. - Layer 4 (Error Handling):
_send_ackstrictly validates{}/["ok"]and avoids local/cloud fallback retries on rejected PIN checks. - Layer 5 (Testing Standards): Thorough unit tests in
tests/devices/traits/v1/test_camera.pyand log-redaction tests intests/devices/rpc/test_v1_channel.py; one small test addition noted inline to cover the uninitializedrpc_channelguard.
| """Return the channel supplied by the device.""" | ||
| if self._rpc_channel is None: | ||
| raise RoborockException("Camera trait has no RPC channel") | ||
| return self._rpc_channel |
There was a problem hiding this comment.
Layer 5 (Testing / Patch Coverage): This Camera trait has no RPC channel guard is the only uncovered line in the PR diff. Adding a unit test in tests/devices/traits/v1/test_camera.py that calls a command on a bare CameraTrait() instance before _rpc_channel is attached will exercise this branch and bring patch coverage to 100%.
| async def stop_preview(self) -> None: | ||
| """Stop the active preview; the command is not scoped to a client ID.""" | ||
| await self._send_ack(RoborockCommand.STOP_CAMERA_PREVIEW) | ||
|
|
There was a problem hiding this comment.
On the wire, stop_camera_preview accepts {"client_id": client_id} matching the client_id passed to start_camera_preview. Consider accepting an optional client_id: str | None = None here (sending {"client_id": client_id} when provided, or None when omitted) so callers can scope teardown to their session ID.
| async def start_voice_chat(self) -> None: | ||
| """Enable the robot's microphone for the active WebRTC session.""" | ||
| await self._send_ack(RoborockCommand.START_VOICE_CHAT) | ||
|
|
There was a problem hiding this comment.
On the wire, start_voice_chat accepts {"play": True, "record": True} (and stop_voice_chat sends {}). Consider accepting optional play: bool | None = None and record: bool | None = None parameters (or forwarding them when specified) so callers can explicitly set the playback and recording flags.
Expose camera preview control and WebRTC signaling through
device.v1_properties.camera, using the existing MQTT RPC channel. Cloud connectivity is required. A single channel strategy avoids repeating a failed or timed-out PIN attempt through local/cloud fallback. This is the next camera increment after #980 and #981 and follows the integration approach discussed in #764.The trait is created during feature discovery only when
is_video_monitor_supportedis set; the device manager already restricts V1 traits to V1 vacuum products. Public methods hash PIN/pattern digits, select video quality, fetch typed TURN credentials, exchange typed SDP/ICE data and control voice chat. Bareokresponses normalized by the existing decoder and list acknowledgments are accepted; rejected commands raise without echoing response secrets.Exclude camera PIN hashes and SDP/ICE request data from RPC debug logs, and replace decrypted response/callback payload logs with metadata. Regression tests exercise actual channel requests, successful responses and malformed response logging without changing the wire payloads.
This adds no dependencies or media connection. The caller owns session ordering, bounded polling, ICE deduplication and teardown; each SDP/ICE read makes one request. Preview stop is not scoped to a client ID; the docs cover successful starts and ambiguous start timeouts. The following streaming/session layer can build on this interface. TURN parsing failures redact payload details and suppress parser exception text in displayed tracebacks.
Validation: