Conversation
📝 WalkthroughWalkthroughThis pull request reworks room/session handling to session-scoped frontend sessions and socket-driven joins, migrates settings to per-session endpoints, introduces session cameras as protected geometries, separates geometry defaults from schemas, and adds two-way camera sync and session management APIs on both frontend and Python client sides. Changes
Sequence Diagram(s)sequenceDiagram
actor Client as Frontend
participant SocketClient as Socket.IO Client
participant SocketServer as Socket Server
participant REST as REST API
participant Redis as Redis
Client->>SocketClient: connect (JWT)
SocketClient->>SocketServer: on:connect (auth:{token})
SocketServer->>Redis: validate token / user
SocketServer->>SocketClient: connected ack
Client->>SocketClient: call room:join {roomId, clientType}
SocketClient->>SocketServer: room:join
SocketServer->>Redis: check room exists
alt room not found
SocketServer->>REST: POST /api/rooms (create)
REST-->>Redis: create room entries
end
SocketServer->>Redis: generate sessionId, create session camera (cam:session:{id})
SocketServer->>REST: fetch room geometries/metadata
REST->>SocketServer: return geometries, schemas, defaults
SocketServer->>SocketClient: room:join response {status, sessionId, cameraKey, roomData}
Client->>Client: render Canvas using sessionCameraData
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #834 +/- ##
==========================================
+ Coverage 79.33% 79.92% +0.58%
==========================================
Files 156 161 +5
Lines 19075 19745 +670
==========================================
+ Hits 15133 15781 +648
- Misses 3942 3964 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Fix tests failing due to socket disconnect triggering session cleanup before REST endpoints are called - Use `connect_room` fixture instead of `create_and_join_room` to keep socket connections alive during tests - Refactor extension registration to occur after room:join creates session - Add `_initial_connect_done` flag to handle auto-reconnect scenarios - Update tests for session cameras now stored as geometries - Use RoomKeys class instead of hardcoded Redis key strings - Fix test isolation issues with unique room names Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| } else { | ||
| // Fallback: try to get single point from curve geometry | ||
| const curveGeometry = geometries[curveKey]; | ||
| if ( | ||
| curveGeometry?.type === "Curve" && | ||
| curveGeometry.data?.position?.[0] | ||
| ) { | ||
| targetPosition = curveGeometry.data.position[0]; | ||
| } |
There was a problem hiding this comment.
what does this fallback mean? Can this happen?
| # 1. Create sessionId | ||
| session_id = str(uuid.uuid4()) | ||
|
|
There was a problem hiding this comment.
add a small check, if the session_id is available
| progress_id: json.loads(progress_json) | ||
| for progress_id, progress_json in progress_data.items() | ||
| } | ||
| emit("progress:initial", {"progressTrackers": progress_trackers}, to=sid) |
There was a problem hiding this comment.
investigate what this is! the emit is only used here, but used inside the frontend! Review if needed!
- Remove duplicate settings_routes.py, unify to session_routes.py
- Change endpoint from header-based to URL-based:
/api/rooms/{room_id}/sessions/{session_id}/settings
- Update frontend to pass sessionId in URL path
- Fix frontend invalidation to use sessionId instead of userName
- FrontendSession.settings now returns RoomConfig with auto-save callbacks
- Fix callback type: Callable[[dict], None] to match actual usage
- Remove deprecated RoomKeys.settings(username) method
- Remove no-op _on_invalidate handler
- Add comprehensive Python API tests for session settings
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Session cameras are now stored as regular geometries (key: cam:session:{session_id})
instead of using a separate Redis hash and dedicated API endpoints. This simplifies
the architecture by reusing the existing geometry infrastructure for syncing.
Removes: useCameraSync hook, session camera REST endpoints, CAMERA_STATE_UPDATE/CAMERA_CONTROL
socket events, session_cameras Redis key, and Camera section from RoomConfig.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR implements a major refactoring that moves camera from room-level settings to geometry-based management with per-session state. The main changes include:
Changes:
- Camera geometry now supports direct coordinates or CurveAttachment (previously required curves)
- Settings changed from user-scoped to session-scoped (per browser window/tab)
- Room join flow migrated from REST endpoint to socket-based with automatic session camera creation for frontend clients
Reviewed changes
Copilot reviewed 76 out of 77 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/zndraw/geometries/camera.py | Refactored Camera to support direct coordinates or CurveAttachment, added protected flag and rendering settings |
| src/zndraw/session_manager.py | New session management API for accessing frontend browser sessions |
| src/zndraw/app/events.py | Room join migrated to socket event with automatic session camera creation |
| src/zndraw/settings.py | Removed Camera from RoomConfig, changed settings to session-scoped |
| src/zndraw/socket_manager.py | Socket connection now handles room join and initial data loading |
| tests/conftest.py | Added connect_room fixture for persistent socket connections in tests |
| tests/test_vis_settings.py | Updated settings tests for session-scoped settings without camera |
| tests/test_session_*.py | New test files for session management and camera API |
| tests/test_camera_model.py | New unit tests for Camera Pydantic model validation |
| app/src/* | Frontend updated to match new socket-based join flow and camera handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { useAppStore } from "../../store"; | ||
| import { | ||
| isCurveAttachment, | ||
| CurveAttachment, |
There was a problem hiding this comment.
Unused import CurveAttachment.
| CurveAttachment, |
| data=value.model_dump(), key=key, geometry_type=geometry_type | ||
| ) | ||
|
|
||
| def __delitem__(self, key: str) -> None: |
There was a problem hiding this comment.
This method raises PermissionError - should raise a LookupError (KeyError or IndexError) instead.
| import uuid | ||
| from contextlib import contextmanager | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
Import of 'pytest' is not used.
| return f"FrontendSession({self.session_id!r})" | ||
|
|
||
|
|
||
| class FrontendSessions(Mapping): |
There was a problem hiding this comment.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
app/src/hooks/useSettings.ts (1)
42-73: Fix stalesessionIdclosure bug in cache update callback.In
onSuccess, the queryKey is built from the closure-capturedsessionId(line 59), which may have changed while the mutation was inflight. IfsessionIdchanges during a reconnect or new session, the cache entry at the old session key won't be updated—instead, the callback will attempt to update a different cache entry with the current (changed)sessionId.Carry
sessionIdthrough the mutation variables to ensureonSuccessupdates the correct cache entry:Proposed fix
export const useUpdateSettings = () => { const queryClient = useQueryClient(); const sessionId = useAppStore((state) => state.sessionId); return useMutation({ mutationFn: async (variables: { roomId: string; data: Record<string, any>; + sessionId: string; }) => { - if (!sessionId) throw new Error("No session ID"); - const { roomId, data } = variables; - return await updateSettings(roomId, sessionId, data); + const { roomId, data, sessionId } = variables; + if (!sessionId) throw new Error("No session ID"); + return await updateSettings(roomId, sessionId, data); }, onSuccess: (_, variables) => { - const { roomId, data: submittedData } = variables; + const { roomId, data: submittedData, sessionId } = variables; // Optimistically update the cache with the submitted data const queryKey = ["settings", roomId, sessionId];src/zndraw/settings.py (1)
13-28: Avoid mutatinginfo.data; copy before constructing callback payload and tighten types.In the validator,
new_data = info.datacreates a reference, not a copy. The assignmentnew_data[info.field_name] = vmutates Pydantic's internal state dict. Usenew_data = dict(info.data)to safely copy before mutation. Also, tighten the callback type hint fromt.Callable[[dict], None]tot.Callable[[dict[str, t.Any]], None]for clarity, and avoid shadowing the method parametervin the dict comprehension.Proposed fix
class SettingsBase(BaseModel): model_config = ConfigDict(validate_assignment=True) - callback: SkipJsonSchema[t.Callable[[dict], None] | None] = Field( + callback: SkipJsonSchema[t.Callable[[dict[str, t.Any]], None] | None] = Field( default=None, exclude=True, # 🚀 excludes from model_dump and schema repr=False, ) @@ def trigger_callback_on_change(cls, v: t.Any, info: ValidationInfo) -> t.Any: if "callback" in info.data and callable(info.data["callback"]): - new_data = info.data + new_data = dict(info.data) new_data[info.field_name] = v info.data["callback"]( - {k: v for k, v in new_data.items() if k != "callback"} + {k: value for k, value in new_data.items() if k != "callback"} ) return vapp/src/utils/jsonforms.ts (1)
119-136: Align the pattern with the established custom type validation approach.The new dynamic-geometries injection pattern (lines 119-136) is permissive compared to the preceding dynamic-atom-props pattern (lines 109-117). The earlier pattern enforces
x-custom-type === "dynamic-enum"alongside the feature flag, whereas this one omits the custom type check entirely.The comment claims support for both
x-custom-type="dynamic-enum"and"position-attachment", but the implementation doesn't validate either. This creates inconsistency: geometry keys will be injected into any schema object withx-features: ["dynamic-geometries"], regardless of its custom type, which violates the validation principle established above.Either add the
x-custom-typechecks to match the established pattern or document strict usage guidelines to prevent unintended enum injection.src/zndraw/geometries/curve.py (1)
126-139:get_interpolated_points()will crash ifpositionis a string key; add a guard
Curve.positionexplicitly allows string values for dynamic data keys viaPositionProp, butget_interpolated_points()assumes numeric data when callingnp.array(self.position). The method is used at runtime inTranslatemodifier (modifiers.py:163), where it will fail if the curve's position is set to a dynamic data key. Add a guard to check if position is a string and either resolve it or raise a clear error.docs/source/python-api.rst (1)
260-582: Critical API mismatches: CurveAttachment and vis.sessions do not exist in the released ZnDraw APIVerification found that the documented APIs differ significantly from the actual released implementation:
CurveAttachment does not exist. The Camera class does not accept
CurveAttachmentobjects. The correct API uses:
position_curve_key="geometry_key"(instead ofposition=CurveAttachment(...))target_curve_key="geometry_key"(instead oftarget=CurveAttachment(...))position_progress=0.5andtarget_progress=0.5(instead ofprogress=0.5within CurveAttachment)
vis.sessionsdoes not exist in the ZnDraw Python API. The vis object provides geometries, figures, chat, and other features, but no sessions attribute. The entire "Browser Sessions" section must be removed or replaced with the actual available APIs.Import path is incorrect.
from zndraw.transformations import CurveAttachmentwill fail; CurveAttachment is not exported anywhere in ZnDraw.Update all Camera animation examples and remove the "Browser Sessions" section entirely.
tests/test_vis_room.py (1)
24-28: Add explicittimeout=torequests.postin tests (Ruff S113).
Avoids indefinite hangs in CI if the server stalls.Proposed fix
create_response = requests.post( f"{server}/api/rooms", json={"roomId": room}, headers=headers, + timeout=10, ) ... create_response = requests.post( f"{server}/api/rooms", json={"roomId": room}, headers=headers, + timeout=10, ) ... response = requests.post( f"{server}/api/rooms", json={"roomId": room}, headers=headers, + timeout=10, )Also applies to: 97-101, 172-176
app/src/components/three/Camera.tsx (1)
65-128: Guardprogresswith?? 0before usinggetPointAt.
Right nowprogresscan becomeundefinedat runtime (even if TS says number), which can produce NaNs.Proposed fix
const positionProgress = isCurveAttachment(data.position) - ? data.position.progress + ? (data.position.progress ?? 0) : 0; ... const targetProgress = isCurveAttachment(data.target) - ? data.target.progress + ? (data.target.progress ?? 0) : 0;app/src/components/Canvas.tsx (1)
212-264: Use existing camera utilities to safely handle potential CurveAttachment in session camera position.The
sessionCameraData.positionhas typePositionType(which is[number, number, number] | CurveAttachment), but line 237 unsafely casts it to a tuple. If position becomes aCurveAttachmentobject, passing it to Three.jsCamera.positionwill fail. Use the existingresolvePosition()utility fromcameraUtils.tsto handle both cases:+ import { resolvePosition } from "../utils/cameraUtils"; + import { useAppStore } from "../store"; - const cameraPosition = sessionCameraData.position as [number, number, number]; + const { geometries } = useAppStore(); + const cameraPosition = resolvePosition(sessionCameraData.position, geometries);Alternatively, check with
isCurveAttachment()and provide a fallback as suggested in the original review.
🤖 Fix all issues with AI agents
In @app/src/components/CameraManager.js:
- Around line 20-58: The comparison for isViewingSessionCamera can be true when
both attachedCameraKey and sessionCameraKey are null; update the check so it
only considers them equal when a session exists (e.g., ensure sessionCameraKey
or sessionId is non-null/defined) before treating it as the session camera.
Specifically, change the isViewingSessionCamera logic used in CameraManager
(currently: attachedCameraKey === sessionCameraKey) to also require a non-null
sessionCameraKey (or sessionId), so position/target syncing only runs for an
actual session camera.
In @app/src/components/Canvas.tsx:
- Around line 182-183: The ref declaration const orbitControlsRef =
useRef<OrbitControlsImpl>(null); will fail under strictNullChecks—change the ref
type to allow null (e.g., useRef<OrbitControlsImpl | null>(null)) or use a
non-null assertion where appropriate, and update any code that dereferences
orbitControlsRef to guard for null (check orbitControlsRef.current before
calling methods) so OrbitControlsImpl usage is type-safe.
In @app/src/components/jsonforms-renderers/PositionAttachmentRenderer.tsx:
- Around line 67-78: The PositionAttachmentRenderer currently ignores JSONForms’
enabled state and can pass a Select value that isn’t in curveOptions causing MUI
warnings; update PositionAttachmentRenderer to read the enabled prop and pass
disabled={!enabled} to inputs, and when rendering the curve Select ensure the
value is only set if curveKey exists in the curveOptions list (e.g., check
curveOptions.includes(curveKey) ? curveKey : '') to avoid out-of-range values;
similarly guard any setters (handleChange) to normalize missing/invalid curveKey
to '' or undefined before calling, and apply the same enabled/value guard to the
other instances mentioned (lines ~235-310) where curveOptions/curveKey are used.
- Around line 67-205: The current handleCoordChange immediately emits parsed
numbers causing re-renders that overwrite in-progress input; change it to only
update local coords state (no handleChange), implement a commitCoords() that
parses coords and calls handleChange(path, [x,y,z]) and call commitCoords from
onBlur for each coordinate (or debounce it), and add a local editing flag or
focusedIndex state so the useEffect that syncs external data into coords skips
overwriting while editing; update handleModeChange to call commitCoords before
switching modes to ensure latest typed values are saved.
In @app/src/components/RegisterDialog.tsx:
- Around line 99-100: The JSX renders getUsernameFromToken() which can return
string | null; update the rendering in RegisterDialog (the line showing "Current
temporary username: <strong>{getUsernameFromToken()}</strong>") to guard against
null by using a fallback (e.g., getUsernameFromToken() ?? "—" or conditional
rendering like {getUsernameFromToken() ?
<strong>{getUsernameFromToken()}</strong> : <strong>unknown</strong>}) so the UI
never displays "null" when the token is missing or invalid.
In @app/src/components/UserProfileDialog.tsx:
- Line 91: getUsernameFromToken() can return null but the JSX in
UserProfileDialog currently renders it directly; update the rendering to handle
null by providing a fallback (e.g., "Guest") so the UI never shows "null" —
replace the direct usage of getUsernameFromToken() in the Username display with
a null-coalescing/fallback expression (or conditional) that renders the returned
string when present and "Guest" when null.
In @src/zndraw/app/redis_keys.py:
- Around line 304-306: RoomKeys.users() is a static room key but is missing from
RoomKeys.all_static_keys(), and its docstring still references
settings(username) which is now session-scoped; update
RoomKeys.all_static_keys() to include the "room:{self.room_id}:users" key
(reference RoomKeys.users) so bulk deletion/cleanup removes room membership, and
adjust the users() docstring to remove or correct the outdated mention of
settings(username) (or replace with a note that settings are session-scoped) to
reflect current behavior.
In @src/zndraw/socket_manager.py:
- Around line 36-88: Update the connect() method to add timeouts, null-safety
and cleanup: pass wait_timeout=10 to self.sio.connect(...), wrap the room join
call (self.sio.call("room:join", ...)) in a try/except with a per-call timeout
and on any exception or non-ok response call self.sio.disconnect() and re-raise
or return; only set self.zndraw.api.session_id, self.zndraw._len, _step,
_frame_selection and _bookmarks after a successful join; coerce nullable
room_data fields with "or" (e.g., room_data.get("frameCount") or 0,
room_data.get("currentStep") or 0, room_data.get("frameSelection") or [],
room_data.get("bookmarks") or {}); ensure _register_extensions_after_join() and
self._initial_connect_done = True run only after all successful initialization,
and clear any partial state (reset session_id and relevant _* attrs) on failure.
In @tests/conftest.py:
- Around line 146-154: The requests.post call that creates rooms (the
create_response = requests.post(...) in tests/conftest.py) is missing a timeout
and can hang tests; add a timeout argument (e.g. timeout=5 or
timeout=REQUEST_TIMEOUT) to that requests.post invocation, and preferably define
a module-level REQUEST_TIMEOUT constant and use it here so the timeout is
configurable and consistent across other test HTTP calls.
- Around line 206-215: The requests.post call that creates a room (creating
create_response in tests/conftest.py) lacks a timeout which can hang tests; add
a timeout parameter (e.g., timeout=5 or timeout=TEST_TIMEOUT if a test timeout
constant exists) to the requests.post invocation that posts to
f"{server}/api/rooms" so the call fails fast and the subsequent error handling
(sio.disconnect() and RuntimeError) runs reliably.
In @tests/test_vis_bookmarks.py:
- Around line 182-231: The tests test_vis_bookmarks_sync_to_second_client,
test_vis_bookmarks_sync_bidirectional, and test_vis_bookmarks_overwrite_syncs
currently use time.sleep(0.1) which is flaky; replace those sleeps with a small
polling helper (e.g., wait_for_bookmarks(vis, expected, timeout=2,
poll_interval=0.05)) that repeatedly checks dict(vis.bookmarks) for the expected
mapping and returns once matched or raises/asserts on timeout; call this helper
after each change from ZnDraw instances (vis1/vis2) to wait deterministically
for synchronization instead of sleeping.
In @tests/test_vis_geometries.py:
- Around line 149-155: The requests.get call in the test_vis_geometries.py
snippet lacks a timeout which can cause the test to hang; modify the
requests.get(...) invocation that queries
f"{server}/api/rooms/{room}/geometries" (the call using variables server, room,
headers) to include a sensible timeout argument (e.g., a few seconds) so the
test fails fast on unresponsive servers and retains the subsequent JSON parsing
and remaining_keys/assert logic unchanged.
In @tests/test_vis_room.py:
- Around line 37-48: Tests use socketio.Client().connect(...) and .call(...)
without explicit timeouts; add explicit timeouts to avoid hanging. Update each
use of sio.connect(...) to pass wait_timeout=10 (e.g., sio.connect(server,
auth={"token": jwt_token}, wait=True, wait_timeout=10)) and each sio.call(...)
to pass timeout=10 (e.g., sio.call("room:join", {...}, timeout=10)); apply this
change to all three occurrences in the test file where connect and call are
used.
🧹 Nitpick comments (32)
tests/test_workers.py (1)
567-567: LGTM! Improved test isolation with unique room identifier.The change from
"testroom"to"test-delete-job-room"improves test isolation, which is especially valuable given the PR's session-based architecture changes where room-level state is more critical.Optional: Consider extending this pattern to other tests
Several other tests in this file still use the generic
"testroom"identifier (lines 45, 156, 412, 477, 642, 778, 822, 837, 863, 895, 902). For better test isolation and to prevent potential race conditions when tests run in parallel, consider using unique, descriptive room identifiers for each test function, e.g.:
test_register_extensions→"test-register-ext-room"test_run_client_extensions→"test-run-client-ext-room"- etc.
This is not critical for this PR but would further improve test reliability.
src/zndraw/geometries/cell.py (1)
31-37: Good schema-driven constraint + UI hint; consider usingle=10.0for float consistency.
le=10will validate fine, but keeping float bounds (10.0) avoids type-mismatch weirdness in schema/tooling that expects floats.src/zndraw/app/utility_routes.py (1)
70-87: Harden defaults endpoint: JSON-safe dump + cache + clearer failure mode.
Recommendinstance.model_dump(mode="json")(ormodel_dump_json) to avoid accidentally returning non-serializable values, plus caching the computed defaults since they’re static. Also consider handling per-geometry instantiation failures with a clear error payload (which geometry failed).Proposed fix (JSON-safe dump + simple caching)
@utility.route("/api/schema/geometries/defaults") def get_geometry_defaults(): @@ - defaults = {} - for name, model in geometry_classes.items(): - instance = model() - defaults[name] = instance.model_dump() - return {"defaults": defaults}, 200 + cached = current_app.extensions.get("_geometry_defaults_cache") + if cached is not None: + return {"defaults": cached}, 200 + + defaults: dict[str, dict] = {} + for name, model in geometry_classes.items(): + try: + instance = model() + defaults[name] = instance.model_dump(mode="json") + except Exception as e: + log.exception("Failed to compute defaults for geometry '%s'", name) + return {"error": str(e), "geometry": name, "type": "DefaultsError"}, 500 + + current_app.extensions["_geometry_defaults_cache"] = defaults + return {"defaults": defaults}, 200tests/test_auth_integration.py (1)
112-145: Consider removing redundant imports.The
SocketIOConnectionErroris already imported at the top of the file (line 5), so the local imports on lines 115, 127, and 139 are unnecessary.Proposed fix
def test_join_room_without_jwt_fails(server): """Test that socket connection without JWT token fails.""" import socketio - from socketio.exceptions import ConnectionError as SocketIOConnectionError sio = socketio.Client() # Try to connect without auth token with pytest.raises(SocketIOConnectionError): sio.connect(server, wait=True) def test_join_room_with_invalid_jwt_fails(server): """Test that socket connection with invalid JWT token fails.""" import socketio - from socketio.exceptions import ConnectionError as SocketIOConnectionError sio = socketio.Client() # Try to connect with invalid token with pytest.raises(SocketIOConnectionError): sio.connect(server, auth={"token": "invalid-token-here"}, wait=True) def test_join_room_with_malformed_auth_header_fails(server): """Test that socket connection without proper token fails.""" import socketio - from socketio.exceptions import ConnectionError as SocketIOConnectionError sio = socketio.Client() # Try to connect with empty auth (no token key) with pytest.raises(SocketIOConnectionError): sio.connect(server, auth={}, wait=True)tests/test_requires_lock_decorator.py (1)
177-181: Consider adding timeout to requests calls in tests.While this is test code, adding timeouts prevents tests from hanging indefinitely if the server becomes unresponsive. The static analysis tool flagged this.
Optional: Add timeout parameter
response = requests.post( f"{server}/api/rooms/{room}/locks/trajectory:meta/acquire", json={"msg": "session 1 lock"}, headers=conn1.headers, + timeout=10, )src/zndraw/app/events.py (1)
799-803: Consider: Comment numbering gap suggests removed steps.Steps are numbered 1, 2, 3, 4, 5, 7, 9, 10 - steps 6 and 8 appear to have been removed. Consider renumbering for clarity, or this may indicate incomplete cleanup from refactoring.
app/src/components/CanvasLoadingState.tsx (1)
1-25: Consider extracting the magic number 64px.The component is clean and functional. However, the hardcoded 64px in
calc(100vh - 64px)(Line 15) appears to represent a header height. If this value is used elsewhere or might change, consider extracting it to a theme constant or shared configuration.♻️ Optional refactor to use a theme constant
If the app theme defines a header height:
<Box sx={{ width: "100%", - height: "calc(100vh - 64px)", + height: `calc(100vh - ${theme.mixins.toolbar.minHeight}px)`, display: "flex", alignItems: "center", justifyContent: "center", bgcolor: theme.palette.background.default, }} >Alternatively, define a constant if not in theme:
const HEADER_HEIGHT = 64; // Then use: height: `calc(100vh - ${HEADER_HEIGHT}px)`,tests/test_protected_geometry.py (1)
9-79: Consider parametrizing deletion tests to reduce duplication.While the current test structure is clear and passes review, you could optionally reduce duplication by parametrizing the deletion tests (lines 9-44) to cover protected/unprotected cases more concisely. However, the current explicit structure is also acceptable and may be clearer for documentation purposes.
♻️ Optional parametrization example
@pytest.mark.parametrize("protected,should_raise", [ (True, True), (False, False), ]) def test_geometry_deletion_by_protection_status(server, protected, should_raise): """Test deletion based on protected flag.""" vis = ZnDraw(url=server, room=f"room-delete-{protected}", user="tester") vis.geometries["test_cam"] = Camera(protected=protected) if should_raise: with pytest.raises(PermissionError, match="Cannot delete protected geometry"): del vis.geometries["test_cam"] else: del vis.geometries["test_cam"] assert "test_cam" not in vis.geometriessrc/zndraw/app/redis_keys.py (1)
328-344:session_settings(session_id)key is fine; ensuresession_idis server-controlled/validated. Ifsession_idcan be influenced by clients, it’s worth enforcing a safe format (e.g., UUID) at the boundary to avoid key-collision/spoofing surprises.app/src/socket.ts (1)
7-14: Avoid sendingtoken: nullin Socket.IO auth payload (prefer omission). IfgetToken()returnsnull, emitting{ token: null }may be interpreted differently than “no token” on the server.Proposed change
export const socket = io(URL, { autoConnect: false, auth: (cb) => { // Send JWT token in auth on connect/reconnect // SessionId is created in room:join handler, not at connect time const token = getToken(); - cb({ token }); + cb(token ? { token } : {}); }, });src/zndraw/transformations.py (1)
14-44: Add type hints tois_curve_attachment(and optionally acceptCurveAttachmentinstances). This aligns with the repo’s Python guidelines and makes call sites clearer.Proposed change
-from typing import Literal +from typing import Any, Literal from pydantic import BaseModel, Field @@ -def is_curve_attachment(value) -> bool: +def is_curve_attachment(value: Any) -> bool: """Check if value is a CurveAttachment dict. @@ - return isinstance(value, dict) and value.get("type") == "curve_attachment" + if isinstance(value, CurveAttachment): + return True + return isinstance(value, dict) and value.get("type") == "curve_attachment"Also applies to: 46-60
app/src/components/CanvasErrorState.tsx (1)
17-27: Consider making the header height offset configurable.The hardcoded
64pxoffset incalc(100vh - 64px)assumes a fixed header height. If the header height changes (e.g., responsive design, theme variations), this component will misalign.💡 Suggested approach
Consider extracting the header height to a theme constant or computing it dynamically:
// Option 1: Use theme constant const headerHeight = theme.spacing(8); // or theme.mixins.toolbar.minHeight <Box sx={{ height: `calc(100vh - ${headerHeight}px)`, // ... rest of styles }} >tests/test_camera_model.py (1)
85-119: Consider using parametrize to reduce test duplication.Several validation tests follow similar patterns and could benefit from parametrization to improve maintainability:
test_camera_validation_near_positiveandtest_camera_validation_far_positive(lines 85-94) are nearly identicaltest_camera_validation_zoom_positive(lines 113-119) tests two invalid values♻️ Refactor using parametrize
-def test_camera_validation_near_positive(): - """Camera near must be positive.""" - with pytest.raises(ValidationError): - Camera(near=-1.0) - - -def test_camera_validation_far_positive(): - """Camera far must be positive.""" - with pytest.raises(ValidationError): - Camera(far=-1.0) +@pytest.mark.parametrize( + "field,value", + [ + ("near", -1.0), + ("far", -1.0), + ], +) +def test_camera_validation_positive_values(field, value): + """Camera near and far must be positive.""" + with pytest.raises(ValidationError): + Camera(**{field: value})Similarly for zoom validation:
-def test_camera_validation_zoom_positive(): - """Camera zoom must be positive.""" - with pytest.raises(ValidationError): - Camera(zoom=0.0) - - with pytest.raises(ValidationError): - Camera(zoom=-1.0) +@pytest.mark.parametrize("zoom_value", [0.0, -1.0]) +def test_camera_validation_zoom_positive(zoom_value): + """Camera zoom must be positive.""" + with pytest.raises(ValidationError): + Camera(zoom=zoom_value)app/src/components/CameraManager.js (1)
27-67: Consider stricter value checks for camera numeric fields (defensive)If
sessionCameraDatacan ever containnull/undefined(e.g., partial updates), assigningnear/far/fovdirectly can put the camera in a bad state. A quicktypeof === "number" && Number.isFinite(...)guard would make this more resilient.tests/test_vis_settings.py (3)
1-1: Movepytestimport to top of file.Line 367 imports
pytestinside a function body. Per coding guidelines, imports should be at the top of Python files.Proposed fix
import uuid +import pytest import requestsThen remove the import from line 367.
304-304: Prefernext(iter(...))over single element slice.As flagged by Ruff (RUF015), use
next(iter(vis.sessions.values()))instead oflist(vis.sessions.values())[0]for clearer intent and efficiency.Proposed fix (apply to lines 304, 331, 352, 377)
- session = list(vis.sessions.values())[0] + session = next(iter(vis.sessions.values()))
380-382: Use a more specific exception type instead of bareException.Catching
Exceptionis too broad (Ruff B017). Use the specific Pydantic validation exception for clarity.Proposed fix
+from pydantic import ValidationError + # Pydantic should validate - key_light has ge=0.0, le=3.0 - with pytest.raises(Exception): + with pytest.raises(ValidationError): settings.studio_lighting.key_light = 10.0 # Out of rangesrc/zndraw/app/session_routes.py (2)
53-81: Consider validating session existence before returning settings.The endpoint returns settings for any
session_idwithout verifying it exists inkeys.frontend_sessions(). This could return default settings for non-existent sessions.Proposed fix
@session_bp.route( "/api/rooms/<string:room_id>/sessions/<string:session_id>/settings", methods=["GET"], ) @require_auth def get_session_settings(room_id: str, session_id: str): + r = current_app.extensions["redis"] + keys = RoomKeys(room_id) + + # Verify session exists + if not r.sismember(keys.frontend_sessions(), session_id): + return {"error": "Session not found"}, 404 + settings_service = current_app.extensions["settings_service"]
84-137: Consider validating session existence before updating settings.Similar to the GET endpoint, the PUT endpoint should verify the session exists before allowing updates to prevent orphaned settings data.
app/src/hooks/useGeometryCameraSync.ts (1)
87-92: Consider potential race condition with the reset timeout.The 50ms timeout to reset
isUpdatingGeometryRefcould cause issues if the geometry update propagates back faster than expected, or if rapid successive updates occur. Consider using the response fromcreateGeometryto coordinate the reset instead of a fixed timeout.app/src/hooks/useCameraControls.ts (1)
63-65: Consider adding null check forcamera.data.The code accesses
camera.data.positionandcamera.data.targetwithout verifyingcamera.dataexists first.Proposed fix
+ const cameraData = camera.data || {}; // Check if position/target are CurveAttachment (locked) or XYZ (editable) - const positionLocked = isCurveAttachment(camera.data.position); - const targetLocked = isCurveAttachment(camera.data.target); + const positionLocked = isCurveAttachment(cameraData.position); + const targetLocked = isCurveAttachment(cameraData.target);tests/test_session_camera_api.py (1)
322-334: Consider increasing sleep or using polling for disconnect test.The fixed 0.2s sleep may be flaky under load. Consider polling with a timeout for more reliable behavior.
Proposed fix using polling
# Disconnect sio.disconnect() - # Give server time to process disconnect - time.sleep(0.2) + # Poll for camera deletion with timeout + for _ in range(10): + response = requests.get( + f"{server}/api/rooms/{room_id}/geometries/{camera_key}", + headers=headers, + timeout=10, + ) + if response.status_code == 404: + break + time.sleep(0.1) # Verify camera is deleted response = requests.get(tests/test_lazy_room_loading.py (2)
144-165: Auth test may be brittle if server returns 403 instead of 401.
If the backend distinguishes unauthenticated vs unauthorized, consider allowing{401,403}(or assert on error schema) to reduce false failures.
238-279: Parallel endpoint test can become flaky without a small retry/backoff.
Threaded fan-out is useful, but CI noise can happen from transient connect/race issues; a short retry (or asserting “not 5xx”) can make it more stable.tests/test_vis_room.py (1)
252-272: Avoid fixedsleep(0.1); poll until expected geometry count (flake risk).
Disconnect cleanup timing can vary; a short loop with a deadline is usually more stable than a single sleep.src/zndraw/socket_manager.py (1)
100-135: Reconnect path should refresh cached room state (not justsession_id).
After re-join,frameCount/currentStep/bookmarks/geometriesmay have changed while offline; consider re-initializing those fields similarly toconnect().src/zndraw/zndraw.py (1)
905-922: Bookmark cache invalidation may be overly broad on single-frame replace.
If_bookmarksis only a local cache, you can likely dopop(frame_id, None)instead of clearing everything.tests/test_vis_geometries.py (1)
346-368: Avoid catching blindExceptioninpytest.raises.Using
pytest.raises(Exception)is too broad and may mask unexpected errors. Use the specific Pydantic validation error type for clearer test intent.♻️ Proposed fix
+from pydantic import ValidationError + def test_vis_camera_validation(server): """Test camera validation for invalid parameters.""" vis = ZnDraw(url=server, room="room-camera-validation", user="tester") # Test invalid FOV (must be between 0 and 180) - with pytest.raises(Exception): # Pydantic validation error + with pytest.raises(ValidationError): Camera( position=(0.0, 0.0, 10.0), target=(0.0, 0.0, 0.0), fov=200.0, # Invalid: > 180 ) # Test invalid far plane (must be > near) - with pytest.raises(Exception): # Pydantic validation error + with pytest.raises(ValidationError): Camera( position=(0.0, 0.0, 10.0), target=(0.0, 0.0, 0.0), near=10.0, far=5.0, # Invalid: < near ) # Test invalid up vector (cannot be zero) - with pytest.raises(Exception): # Pydantic validation error + with pytest.raises(ValidationError): Camera( position=(0.0, 0.0, 10.0), target=(0.0, 0.0, 0.0), up=(0.0, 0.0, 0.0), # Invalid: zero vector )app/src/hooks/useSocketManager.ts (1)
203-228: Consider handling room creation errors more explicitly.When
createRoomfails with a non-409 error, the function logs and returns without setting an error state, leaving the user with no feedback about what went wrong.♻️ Proposed fix
try { await createRoom({ roomId, copyFrom: template ?? undefined, }); } catch (error: any) { // 409 = room already exists = success (race condition) if (error.response?.status !== 409) { console.error("Failed to create room:", error); + setInitializationError({ + message: "Failed to create room", + details: error.message || "Could not create the room", + }); return; } }src/zndraw/session_manager.py (2)
1-18: Add TYPE_CHECKING import for theZnDrawforward reference.The static analysis tool flags
ZnDrawas undefined because the string-quoted forward reference isn't validated at runtime but needs the import for type checkers. UsingTYPE_CHECKINGavoids circular imports while satisfying type checkers.♻️ Proposed fix
"""Session management for frontend browser windows. ... """ +from __future__ import annotations + from collections.abc import Mapping +from typing import TYPE_CHECKING from zndraw.geometries import Camera from zndraw.settings import RoomConfig +if TYPE_CHECKING: + from zndraw import ZnDraw +With
from __future__ import annotations, you can also remove the quotes around"ZnDraw"in the type hints.
199-202: Consider caching session IDs to avoid duplicate API calls.
__getitem__calls_get_frontend_session_ids()for validation, but callers iterating viavalues()oritems()will already have fetched the list. This results in an extra API call per access. For simple cases this is fine, but if performance becomes a concern, consider short-lived caching or restructuring.src/zndraw/geometries/camera.py (1)
174-205: Clarify the distinction betweenhelper_colorandcolorfields.Both
helper_colorandcolorhave identical defaults (#00ff00) and similar descriptions related to the camera helper visualization. The docstrings suggestcoloris "Camera helper color (visualization only)" which overlaps withhelper_color's purpose. Consider:
- Removing one if they're truly redundant
- Documenting why both exist if they serve different purposes (e.g.,
colorfor API consistency with other geometries)
| You MUST NEVER `@pytest.mark.xfail` or similar - all tests must pass! | ||
| Always review, if the new code will be a workaround or a proper solution before implementing it. | ||
| DO NOT IMPLEMENT WORKAROUNDS! | ||
| Remove deprecated code instead of just marking it as deprecated! | ||
|
|
||
| ### Handling Default Values | ||
|
|
||
| **Principle: Single Source of Truth** | ||
| All default values (e.g., `camera`, `particles`) must be defined exclusively within the **Pydantic model**. Do not scatter fallback logic throughout the codebase. | ||
|
|
||
| **Anti-Pattern (Hardcoded Fallbacks)** | ||
| Do not perform null checks combined with hardcoded literals. | ||
|
|
||
| ```js | ||
| camera.near = sessionCameraData.near ?? 0.1; | ||
| ``` | ||
|
|
||
| **Best Practice (Schema-Driven)** | ||
| Rely entirely on the schema to populate default values during initialization. Ensure the data model is fully validated before usage. | ||
|
|
||
| ```js | ||
| camera.near = sessionCameraData.near; | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Tighten wording around pytest rule to remove ambiguity.
Consider: “You MUST NEVER use pytest.mark.xfail (or similar) — all tests must pass.” This reads cleaner and avoids interpreting @pytest.mark.xfail as a verb. Based on learnings, this rule is important for **/*test*.py.
| // Derive session camera key | ||
| const sessionCameraKey = sessionId ? `cam:session:${sessionId}` : null; | ||
|
|
||
| // Track if this is the initial mount to avoid position sync on first render | ||
| // (let the Canvas's initial camera position take precedence) | ||
| const isInitialMount = useRef(true); | ||
|
|
||
| useEffect(() => { | ||
| if (settings) { | ||
| // Update properties that can be changed on the fly | ||
| camera.near = settings.near_plane; | ||
| camera.far = settings.far_plane; | ||
|
|
||
| // You could also update other properties like fov for a perspective camera | ||
| // if (camera.isPerspectiveCamera) { | ||
| // camera.fov = settings.fov; | ||
| // } | ||
|
|
||
| // CRITICAL: This tells Three.js to re-calculate the camera's projection matrix | ||
| // with the new values. Without this, you won't see any change. | ||
| camera.updateProjectionMatrix(); | ||
| if (!sessionCameraData) return; | ||
|
|
||
| camera.near = sessionCameraData.near; | ||
| camera.far = sessionCameraData.far; | ||
|
|
||
| if (camera.isPerspectiveCamera && sessionCameraData.fov) { | ||
| camera.fov = sessionCameraData.fov; | ||
| } | ||
| }, [settings, camera]); // Re-run this effect when settings or the camera object changes | ||
|
|
||
| return null; // This component doesn't render anything itself | ||
| // CRITICAL: Re-calculate projection matrix with new values | ||
| camera.updateProjectionMatrix(); | ||
|
|
||
| // Only sync position/target when viewing through the session camera | ||
| // (not when attached to another camera) and not on initial mount | ||
| const isViewingSessionCamera = attachedCameraKey === sessionCameraKey; | ||
|
|
||
| if (isViewingSessionCamera && !isInitialMount.current) { | ||
| const position = sessionCameraData.position; | ||
| const target = sessionCameraData.target; | ||
|
|
||
| // Sync position if it's a direct coordinate array | ||
| if (Array.isArray(position) && position.length === 3) { | ||
| camera.position.set(position[0], position[1], position[2]); | ||
| } | ||
|
|
||
| // Sync target if controls exist and target is a direct coordinate array | ||
| if (controls?.target && Array.isArray(target) && target.length === 3) { | ||
| controls.target.set(target[0], target[1], target[2]); | ||
| controls.update(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: attachedCameraKey === sessionCameraKey is true when both are null
When sessionId isn’t set yet, sessionCameraKey is null. If attachedCameraKey is also null (store default), isViewingSessionCamera becomes true, which can trigger unintended position/target syncing once isInitialMount.current flips.
Proposed fix
- const isViewingSessionCamera = attachedCameraKey === sessionCameraKey;
+ const isViewingSessionCamera =
+ sessionCameraKey !== null && attachedCameraKey === sessionCameraKey;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Derive session camera key | |
| const sessionCameraKey = sessionId ? `cam:session:${sessionId}` : null; | |
| // Track if this is the initial mount to avoid position sync on first render | |
| // (let the Canvas's initial camera position take precedence) | |
| const isInitialMount = useRef(true); | |
| useEffect(() => { | |
| if (settings) { | |
| // Update properties that can be changed on the fly | |
| camera.near = settings.near_plane; | |
| camera.far = settings.far_plane; | |
| // You could also update other properties like fov for a perspective camera | |
| // if (camera.isPerspectiveCamera) { | |
| // camera.fov = settings.fov; | |
| // } | |
| // CRITICAL: This tells Three.js to re-calculate the camera's projection matrix | |
| // with the new values. Without this, you won't see any change. | |
| camera.updateProjectionMatrix(); | |
| if (!sessionCameraData) return; | |
| camera.near = sessionCameraData.near; | |
| camera.far = sessionCameraData.far; | |
| if (camera.isPerspectiveCamera && sessionCameraData.fov) { | |
| camera.fov = sessionCameraData.fov; | |
| } | |
| }, [settings, camera]); // Re-run this effect when settings or the camera object changes | |
| return null; // This component doesn't render anything itself | |
| // CRITICAL: Re-calculate projection matrix with new values | |
| camera.updateProjectionMatrix(); | |
| // Only sync position/target when viewing through the session camera | |
| // (not when attached to another camera) and not on initial mount | |
| const isViewingSessionCamera = attachedCameraKey === sessionCameraKey; | |
| if (isViewingSessionCamera && !isInitialMount.current) { | |
| const position = sessionCameraData.position; | |
| const target = sessionCameraData.target; | |
| // Sync position if it's a direct coordinate array | |
| if (Array.isArray(position) && position.length === 3) { | |
| camera.position.set(position[0], position[1], position[2]); | |
| } | |
| // Sync target if controls exist and target is a direct coordinate array | |
| if (controls?.target && Array.isArray(target) && target.length === 3) { | |
| controls.target.set(target[0], target[1], target[2]); | |
| controls.update(); | |
| } | |
| } | |
| // Derive session camera key | |
| const sessionCameraKey = sessionId ? `cam:session:${sessionId}` : null; | |
| // Track if this is the initial mount to avoid position sync on first render | |
| // (let the Canvas's initial camera position take precedence) | |
| const isInitialMount = useRef(true); | |
| useEffect(() => { | |
| if (!sessionCameraData) return; | |
| camera.near = sessionCameraData.near; | |
| camera.far = sessionCameraData.far; | |
| if (camera.isPerspectiveCamera && sessionCameraData.fov) { | |
| camera.fov = sessionCameraData.fov; | |
| } | |
| // CRITICAL: Re-calculate projection matrix with new values | |
| camera.updateProjectionMatrix(); | |
| // Only sync position/target when viewing through the session camera | |
| // (not when attached to another camera) and not on initial mount | |
| const isViewingSessionCamera = | |
| sessionCameraKey !== null && attachedCameraKey === sessionCameraKey; | |
| if (isViewingSessionCamera && !isInitialMount.current) { | |
| const position = sessionCameraData.position; | |
| const target = sessionCameraData.target; | |
| // Sync position if it's a direct coordinate array | |
| if (Array.isArray(position) && position.length === 3) { | |
| camera.position.set(position[0], position[1], position[2]); | |
| } | |
| // Sync target if controls exist and target is a direct coordinate array | |
| if (controls?.target && Array.isArray(target) && target.length === 3) { | |
| controls.target.set(target[0], target[1], target[2]); | |
| controls.update(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @app/src/components/CameraManager.js around lines 20 - 58, The comparison for
isViewingSessionCamera can be true when both attachedCameraKey and
sessionCameraKey are null; update the check so it only considers them equal when
a session exists (e.g., ensure sessionCameraKey or sessionId is
non-null/defined) before treating it as the session camera. Specifically, change
the isViewingSessionCamera logic used in CameraManager (currently:
attachedCameraKey === sessionCameraKey) to also require a non-null
sessionCameraKey (or sessionId), so position/target syncing only runs for an
actual session camera.
| const orbitControlsRef = useRef<OrbitControlsImpl>(null); | ||
|
|
There was a problem hiding this comment.
useRef<OrbitControlsImpl>(null) is likely a strict-null TS error.
If strictNullChecks is on, this won’t typecheck.
Proposed fix
- const orbitControlsRef = useRef<OrbitControlsImpl>(null);
+ const orbitControlsRef = useRef<OrbitControlsImpl | null>(null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const orbitControlsRef = useRef<OrbitControlsImpl>(null); | |
| const orbitControlsRef = useRef<OrbitControlsImpl | null>(null); |
🤖 Prompt for AI Agents
In @app/src/components/Canvas.tsx around lines 182 - 183, The ref declaration
const orbitControlsRef = useRef<OrbitControlsImpl>(null); will fail under
strictNullChecks—change the ref type to allow null (e.g.,
useRef<OrbitControlsImpl | null>(null)) or use a non-null assertion where
appropriate, and update any code that dereferences orbitControlsRef to guard for
null (check orbitControlsRef.current before calling methods) so
OrbitControlsImpl usage is type-safe.
| def test_vis_bookmarks_sync_to_second_client(server, s22): | ||
| """Test that bookmarks set by one client are visible to another.""" | ||
| vis1 = ZnDraw(url=server, room="testroom", user="user1") | ||
| vis2 = ZnDraw(url=server, room="testroom", user="user2") | ||
|
|
||
| vis1.extend(s22) | ||
| vis1.bookmarks[1] = "First Frame" | ||
| vis1.bookmarks[5] = "Middle Frame" | ||
| vis1.bookmarks[9] = "Last Frame" | ||
|
|
||
| assert len(vis2) == len(s22) | ||
| assert len(vis1) == len(s22) | ||
| time.sleep(0.1) | ||
|
|
||
| assert dict(vis2.bookmarks) == {1: "First Frame", 5: "Middle Frame"} | ||
|
|
||
|
|
||
| def test_vis_bookmarks_sync_bidirectional(server, s22): | ||
| """Test that bookmarks sync bidirectionally between clients.""" | ||
| vis1 = ZnDraw(url=server, room="testroom", user="user1") | ||
| vis2 = ZnDraw(url=server, room="testroom", user="user2") | ||
|
|
||
| # Wait a moment for socket events to propagate | ||
| import time | ||
| vis1.extend(s22) | ||
| vis1.bookmarks[1] = "From User1" | ||
|
|
||
| time.sleep(0.1) | ||
|
|
||
| # Both clients should see the same bookmarks (room-wide) | ||
| assert ( | ||
| dict(vis2.bookmarks) | ||
| == dict(vis1.bookmarks) | ||
| == { | ||
| 1: "First Frame", | ||
| 5: "Middle Frame", | ||
| 9: "Last Frame", | ||
| } | ||
| ) | ||
|
|
||
| # vis2 adds new bookmarks and updates one | ||
| vis2.bookmarks[2] = "Second Frame" | ||
| vis2.bookmarks[6] = "Another Middle" | ||
| vis2.bookmarks[9] = "Updated Last Frame" | ||
|
|
||
| # Wait for propagation | ||
| vis2.bookmarks[2] = "From User2" | ||
|
|
||
| time.sleep(0.1) | ||
|
|
||
| # Bookmarks are room-wide, so both clients see ALL bookmarks | ||
| expected_bookmarks = { | ||
| 1: "First Frame", | ||
| 2: "Second Frame", | ||
| 5: "Middle Frame", | ||
| 6: "Another Middle", | ||
| 9: "Updated Last Frame", | ||
| } | ||
| assert dict(vis2.bookmarks) == expected_bookmarks | ||
| assert dict(vis1.bookmarks) == expected_bookmarks | ||
| expected = {1: "From User1", 2: "From User2"} | ||
| assert dict(vis1.bookmarks) == expected | ||
| assert dict(vis2.bookmarks) == expected | ||
|
|
||
|
|
||
| def test_vis_bookmarks_overwrite_syncs(server, s22): | ||
| """Test that overwriting a bookmark syncs to other clients.""" | ||
| vis1 = ZnDraw(url=server, room="testroom", user="user1") | ||
| vis2 = ZnDraw(url=server, room="testroom", user="user2") | ||
|
|
||
| def test_vis_bookmarks_update(server, s22): | ||
| """Test the update() method for batch bookmark updates.""" | ||
| vis1.extend(s22) | ||
| vis1.bookmarks[1] = "Original" | ||
|
|
||
| time.sleep(0.1) | ||
|
|
||
| vis2.bookmarks[1] = "Updated" | ||
|
|
||
| time.sleep(0.1) | ||
|
|
||
| assert vis1.bookmarks[1] == "Updated" | ||
| assert vis2.bookmarks[1] == "Updated" | ||
|
|
There was a problem hiding this comment.
Avoid time.sleep() in sync tests (flaky); poll until condition or timeout
The multi-client sync tests rely on time.sleep(0.1). This will be flaky under load/CI variance. Prefer a small polling helper that waits until dict(vis.bookmarks) matches expected (or times out with a useful assertion).
Proposed refactor
import time
import pytest
from zndraw.zndraw import ZnDraw
+# Small polling helper for eventual consistency over sockets
+def _wait_until(predicate, timeout_s: float = 2.0, interval_s: float = 0.02) -> None:
+ deadline = time.time() + timeout_s
+ last_exc: Exception | None = None
+ while time.time() < deadline:
+ try:
+ if predicate():
+ return
+ except Exception as exc: # noqa: BLE001 - keep last exception for debugging
+ last_exc = exc
+ time.sleep(interval_s)
+ if last_exc:
+ raise last_exc
+ raise AssertionError("Condition not met before timeout")
+
...
def test_vis_bookmarks_sync_to_second_client(server, s22):
"""Test that bookmarks set by one client are visible to another."""
vis1 = ZnDraw(url=server, room="testroom", user="user1")
vis2 = ZnDraw(url=server, room="testroom", user="user2")
@@
vis1.bookmarks[1] = "First Frame"
vis1.bookmarks[5] = "Middle Frame"
-
- time.sleep(0.1)
-
- assert dict(vis2.bookmarks) == {1: "First Frame", 5: "Middle Frame"}
+ expected = {1: "First Frame", 5: "Middle Frame"}
+ _wait_until(lambda: dict(vis2.bookmarks) == expected)
+ assert dict(vis2.bookmarks) == expected🤖 Prompt for AI Agents
In @tests/test_vis_bookmarks.py around lines 182 - 231, The tests
test_vis_bookmarks_sync_to_second_client, test_vis_bookmarks_sync_bidirectional,
and test_vis_bookmarks_overwrite_syncs currently use time.sleep(0.1) which is
flaky; replace those sleeps with a small polling helper (e.g.,
wait_for_bookmarks(vis, expected, timeout=2, poll_interval=0.05)) that
repeatedly checks dict(vis.bookmarks) for the expected mapping and returns once
matched or raises/asserts on timeout; call this helper after each change from
ZnDraw instances (vis1/vis2) to wait deterministically for synchronization
instead of sleeping.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
tests/test_vis_geometries.py (1)
358-380: Use specific exception type instead of blindException.The tests correctly validate camera parameters, but catching bare
Exceptionis too broad. Pydantic raisesValidationErrorfor these cases.♻️ Proposed refactor
+from pydantic import ValidationError + def test_vis_camera_validation(server): """Test camera validation for invalid parameters.""" vis = ZnDraw(url=server, room="room-camera-validation", user="tester") # Test invalid FOV (must be between 0 and 180) - with pytest.raises(Exception): # Pydantic validation error + with pytest.raises(ValidationError): Camera( position=(0.0, 0.0, 10.0), target=(0.0, 0.0, 0.0), fov=200.0, # Invalid: > 180 ) # Test invalid far plane (must be > near) - with pytest.raises(Exception): # Pydantic validation error + with pytest.raises(ValidationError): Camera( position=(0.0, 0.0, 10.0), target=(0.0, 0.0, 0.0), near=10.0, far=5.0, # Invalid: < near ) # Test invalid up vector (cannot be zero) - with pytest.raises(Exception): # Pydantic validation error + with pytest.raises(ValidationError): Camera( position=(0.0, 0.0, 10.0), target=(0.0, 0.0, 0.0), up=(0.0, 0.0, 0.0), # Invalid: zero vector )tests/test_vis_room.py (1)
265-285: Consider replacing sleep with polling for more reliable tests.The test uses
sio.sleep(0.1)to wait for camera deletion, which could be flaky in CI environments with variable timing.♻️ Consider using polling instead of sleep
c2.sio.disconnect() - c2.sio.sleep(0.1) + # Poll for camera deletion with timeout + import time + timeout = 2.0 + start = time.time() + while time.time() - start < timeout: + if len([x for x in vis.geometries if x.startswith("cam:")]) == 1: + break + time.sleep(0.05) assert len([x for x in vis.geometries if x.startswith("cam:")]) == 1 c1.sio.disconnect() - c1.sio.sleep(0.1) + start = time.time() + while time.time() - start < timeout: + if len([x for x in vis.geometries if x.startswith("cam:")]) == 0: + break + time.sleep(0.05) assert len([x for x in vis.geometries if x.startswith("cam:")]) == 0This approach is more resilient to timing variations while still failing fast when the expected condition is met.
tests/test_session_camera_api.py (1)
15-37: Add return type hint to context manager.The
_join_room_sessioncontext manager is missing a return type annotation. Based on coding guidelines, type hints should be used wherever possible in Python code.♻️ Add type hint
+import typing as t + @contextmanager -def _join_room_session(server: str, room_id: str, headers: dict): +def _join_room_session(server: str, room_id: str, headers: dict) -> t.Generator[tuple[str, sio_lib.Client, dict], None, None]: """Context manager that joins a room and keeps socket connected.tests/conftest.py (2)
119-167: Add return type hint.The function is missing a return type annotation. Based on coding guidelines, use type hints wherever possible.
♻️ Add type hint
def _create_and_join_room(server: str, room: str, auth_headers: dict) -> str:
169-176: Add return type hint.The function is missing a return type annotation.
♻️ Add type hint
def _join_room_and_get_headers( server: str, room_id: str, user: str = "test-user" ) -> dict:src/zndraw/app/events.py (3)
74-102: Add type hint for Redis parameter.The
rparameter is missing a type annotation. Based on coding guidelines, use type hints wherever possible.♻️ Add type hint
+import redis + -def create_session_camera(r, room: str, session_id: str) -> None: +def create_session_camera(r: redis.Redis, room: str, session_id: str) -> None:
104-120: Add type hint for Redis parameter.The
rparameter is missing a type annotation.♻️ Add type hint
+import redis + -def delete_session_camera(r, room: str, session_id: str) -> None: +def delete_session_camera(r: redis.Redis, room: str, session_id: str) -> None:
122-197: Add type hint for Redis parameter.The
rparameter is missing a type annotation.♻️ Add type hint
+import redis + -def get_room_metadata(r, room_id: str) -> dict: +def get_room_metadata(r: redis.Redis, room_id: str) -> dict:app/src/myapi/client.ts (2)
245-253: Extract magic string prefix as a named constant.The hardcoded string
"cam:session:"couples the client to a specific naming convention. If this prefix changes or if other prefixes need similar treatment, this logic becomes harder to maintain.♻️ Refactor to use a named constant
Add a constant at the top of the file (after imports):
+// Session-scoped camera geometries that don't require trajectory:meta lock +const SESSION_CAMERA_PREFIX = "cam:session:"; + /** * Decodes frame data from the backend msgpack format.Then update the check:
- // Session cameras (cam:session:*) don't need lock acquisition - they're per-user viewport state + // Session cameras don't need lock acquisition - they're per-user viewport state // Backend only checks forbid=["room:locked"], not trajectory:meta lock - if (key.startsWith("cam:session:")) { + if (key.startsWith(SESSION_CAMERA_PREFIX)) { const { data } = await apiClient.post( `/api/rooms/${roomId}/geometries`, requestBody, ); return data; }
1428-1433: Consider adding JSDoc to explain what a session represents.The
SessionInfointerface is minimal and could benefit from documentation explaining what a frontend session represents in the context of ZnDraw, especially since this is part of a new session-scoped architecture.📝 Add JSDoc documentation
/** - * Session information returned from list endpoint. + * Represents a frontend browser session in a room. + * Each browser tab/window gets its own session with independent camera state. + * Sessions are used to scope settings and per-user viewport geometries. */ export interface SessionInfo { + /** Unique session identifier for this browser tab/window */ session_id: string; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
app/src/components/Canvas.tsxapp/src/components/geometry/GeometryForm.tsxapp/src/components/geometry/GeometryGrid.tsxapp/src/components/three/Arrow.tsxapp/src/components/three/Bonds.tsxapp/src/components/three/Box.tsxapp/src/components/three/Camera.tsxapp/src/components/three/Cell.tsxapp/src/components/three/Curve.tsxapp/src/components/three/Floor.tsxapp/src/components/three/Particles.tsxapp/src/components/three/Plane.tsxapp/src/components/three/Shape.tsxapp/src/hooks/useSocketManager.tsapp/src/myapi/client.tsapp/src/store.tsxapp/src/utils/geometryDefaults.tssrc/zndraw/app/events.pytests/conftest.pytests/test_session_camera_api.pytests/test_vis_geometries.pytests/test_vis_room.py
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/components/geometry/GeometryGrid.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*test*.py: MUST NEVER use@pytest.mark.xfailor similar; all tests must pass
Usepytest.mark.parametrizeto avoid code duplication in tests
Tests should be very specific and test only one thing; avoid complex test setups
Each test must be a function, not a method of a class
Files:
tests/test_vis_room.pytests/test_session_camera_api.pytests/conftest.pytests/test_vis_geometries.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings that are concise and to the point
Use type hints wherever possible in Python code; useimport typing as tif necessary, but preferlist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Always place imports at the top of Python files
Files:
tests/test_vis_room.pysrc/zndraw/app/events.pytests/test_session_camera_api.pytests/conftest.pytests/test_vis_geometries.py
🧠 Learnings (2)
📚 Learning: 2026-01-08T16:00:51.886Z
Learnt from: CR
Repo: zincware/ZnDraw PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T16:00:51.886Z
Learning: Applies to **/*.py : Use numpy style docstrings that are concise and to the point
Applied to files:
src/zndraw/app/events.py
📚 Learning: 2026-01-08T16:00:51.886Z
Learnt from: CR
Repo: zincware/ZnDraw PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T16:00:51.886Z
Learning: Applies to **/*test*.py : MUST NEVER use `pytest.mark.xfail` or similar; all tests must pass
Applied to files:
tests/test_session_camera_api.py
🧬 Code graph analysis (9)
app/src/components/three/Bonds.tsx (2)
app/src/store.tsx (1)
useAppStore(275-1234)app/src/utils/geometryDefaults.ts (1)
getGeometryWithDefaults(29-49)
app/src/components/three/Box.tsx (1)
app/src/utils/geometryDefaults.ts (1)
getGeometryWithDefaults(29-49)
app/src/components/three/Plane.tsx (2)
app/src/store.tsx (1)
useAppStore(275-1234)app/src/utils/geometryDefaults.ts (1)
getGeometryWithDefaults(29-49)
app/src/components/three/Camera.tsx (1)
app/src/utils/cameraUtils.ts (2)
PositionType(21-21)isCurveAttachment(29-36)
tests/test_vis_room.py (2)
tests/conftest.py (3)
server(419-425)get_jwt_auth_headers(251-253)connect_room(269-294)src/zndraw/socket_manager.py (2)
connect(36-87)disconnect(89-94)
app/src/components/three/Cell.tsx (2)
app/src/store.tsx (1)
useAppStore(275-1234)app/src/utils/geometryDefaults.ts (1)
getGeometryWithDefaults(29-49)
tests/test_session_camera_api.py (5)
tests/conftest.py (2)
server(419-425)get_jwt_auth_headers(251-253)src/zndraw/zndraw.py (8)
connect(862-864)wait(870-881)disconnect(866-868)get(213-226)get(935-935)get(937-939)get(941-943)get(972-1065)src/zndraw/socket_manager.py (2)
connect(36-87)disconnect(89-94)src/zndraw/app/redis_keys.py (1)
session_id(600-602)src/zndraw/session_manager.py (2)
camera_key(54-56)get(212-234)
tests/test_vis_geometries.py (2)
src/zndraw/transformations.py (1)
CurveAttachment(14-43)src/zndraw/geometries/camera.py (2)
Camera(27-223)CameraType(16-20)
app/src/myapi/client.ts (2)
app/src/components/CameraManager.js (1)
sessionId(18-18)app/src/hooks/useSettings.ts (1)
SettingsResponse(16-16)
🪛 Ruff (0.14.10)
tests/conftest.py
12-12: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
105-105: Avoid specifying long messages outside the exception class
(TRY003)
155-157: Avoid specifying long messages outside the exception class
(TRY003)
162-162: Avoid specifying long messages outside the exception class
(TRY003)
217-219: Avoid specifying long messages outside the exception class
(TRY003)
225-225: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_vis_geometries.py
366-366: Do not assert blind exception: Exception
(B017)
375-375: Do not assert blind exception: Exception
(B017)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (3.12, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
- GitHub Check: pytest (3.13, ubuntu-latest)
🔇 Additional comments (35)
app/src/store.tsx (2)
40-46: LGTM! Clean error interface for initialization failures.The
InitializationErrorinterface provides a clear structure for tracking room initialization errors with required message and optional details.
69-70: LGTM! Good separation of schemas and defaults.The distinction between
geometrySchemas(for form generation) andgeometryDefaults(for default values) is clear and aligns with single responsibility principle.tests/test_vis_geometries.py (2)
159-167: LGTM! Test correctly handles session-based camera leftovers.The test properly accounts for session-scoped cameras (
cam:session:prefix) that may exist for frontend clients, while verifying all shared geometries were deleted. The comment clarifies this behavior.
607-634: LGTM! Comprehensive test coverage for reserved prefix validation.The tests correctly verify that:
cam:session:prefixed keys are rejected with appropriate error- Similar but non-reserved patterns (underscores, different ordering) are allowed
- Valid geometries are successfully created
app/src/hooks/useSocketManager.ts (4)
70-71: LGTM! Good guard against concurrent auth recovery.The
isRecoveringAuthRefguard prevents race conditions when multipleconnect_errorevents fire before the first auth recovery completes.
136-198: LGTM! Robust room join flow with proper error handling.The join response handler correctly:
- Stores sessionId for session-scoped operations
- Sets initialization error on join failure
- Fetches geometries via REST and handles errors gracefully
- Only marks as connected after geometry data loads successfully
The early return on geometry fetch failure prevents rendering an incomplete scene, with the error state allowing the UI to display an appropriate message.
646-720: LGTM! Well-designed auth recovery with exponential backoff.The authentication recovery logic correctly:
- Guards against concurrent recovery attempts
- Implements exponential backoff (1s, 2s, 4s) across max 3 retries
- Clears stale credentials before re-login
- Updates username from new token
- Resets retry count on successful login
- Uses
finallyblock to ensure guard is releasedThe error handling is defensive and prevents auth issues (e.g., Redis flush) from permanently breaking the connection.
774-783: LGTM! Proper sessionId cleanup prevents premature clearing.The cleanup logic correctly distinguishes between:
- Temporary reconnects (username change, etc.) → keeps sessionId
- Actually leaving the room (navigation away) → clears sessionId
This prevents session state loss during transient disconnects.
app/src/components/three/Cell.tsx (1)
25-32: LGTM! Consistent use of geometryDefaults for default values.The change correctly uses
geometryDefaults(instead ofgeometrySchemas) as the source of default values when merging with runtime data. This aligns with the separation of JSON schemas (for form generation) from default values (for data merging).app/src/components/three/Floor.tsx (1)
30-40: LGTM! Consistent use of geometryDefaults across components.The change mirrors the pattern in Cell.tsx, correctly using
geometryDefaultsas the source for default values. This maintains consistency across all geometry components that usegetGeometryWithDefaults.app/src/components/three/Shape.tsx (1)
93-99: LGTM! Consistent defaults migration.The switch from
geometrySchemastogeometryDefaultscorrectly aligns with the PR's refactoring to separate geometry defaults from schemas as a single source of truth.app/src/components/three/Arrow.tsx (1)
105-111: LGTM! Defaults migration applied correctly.The refactor from
geometrySchemastogeometryDefaultsis consistent with the project-wide migration to separate defaults handling.app/src/components/three/Box.tsx (1)
69-75: LGTM! Defaults source updated correctly.The migration to
geometryDefaultsis consistent with the broader refactoring across geometry components.app/src/components/three/Curve.tsx (1)
62-68: LGTM! Memoization dependency correctly updated.The defaults migration is applied correctly, and the dependency array update on line 68 ensures the memoized
fullDatarecomputes whengeometryDefaultschanges.app/src/components/three/Bonds.tsx (1)
147-153: LGTM! Defaults migration complete.The switch to
geometryDefaultsis consistent with the refactoring applied across all geometry components in this PR.app/src/components/three/Plane.tsx (1)
71-77: LGTM! Clean refactor to use geometry defaults.The change from
geometrySchemastogeometryDefaultsis consistent with the PR's architectural shift to treat Pydantic defaults as the single source of truth. The integration is straightforward and correct.app/src/components/three/Particles.tsx (1)
75-113: LGTM! Consistent with the defaults-driven architecture.The changes align with the broader refactor to use
geometryDefaultsinstead ofgeometrySchemas. The implementation is consistent with other geometry components.app/src/utils/geometryDefaults.ts (1)
29-48: LGTM! Well-documented refactor to use defaults directly.The function signature change from
schemastodefaultsis clear and well-documented. The implementation correctly:
- Falls back to partial data when defaults aren't loaded
- Deep merges defaults with incoming data using lodash
- Maintains type safety with appropriate type assertions
app/src/components/geometry/GeometryForm.tsx (3)
42-42: LGTM! Selector added for geometry defaults.The addition of the
geometryDefaultsselector is necessary for the refactor and properly integrated throughout the component.
156-183: LGTM! Proper integration of geometry defaults in create flow.The
lockAndCreateGeometryfunction correctly:
- Checks for
geometryDefaultsavailability before use- Passes defaults to
getGeometryWithDefaults- Updates the dependency array to include
geometryDefaults
198-201: LGTM! Type change handler correctly uses defaults.The type selection handler properly applies defaults when a geometry type is selected in create mode.
tests/test_vis_room.py (3)
9-50: LGTM! Comprehensive test for REST creation and socket joining.The test properly validates the new socket-based room join flow after REST creation. The assertions cover all essential response fields.
72-92: LGTM! Good error path coverage.The test correctly validates that joining a non-existent room via socket returns a 404 error with appropriate messaging.
94-139: LGTM! Multi-user socket join scenario well tested.The test validates that multiple users can join the same room via socket and receive unique session IDs.
tests/test_session_camera_api.py (1)
44-358: LGTM! Well-structured integration tests.The test suite provides comprehensive coverage of session camera lifecycle:
- Creation on join
- Updates via geometry endpoints
- Validation of invalid data
- Isolation between sessions
- Cleanup on disconnect
All tests follow best practices: they're functions (not class methods), avoid
pytest.mark.xfail, and test one thing each.app/src/components/three/Camera.tsx (1)
1-287: LGTM! Clean refactoring to PositionType.The refactoring from separate curve-key fields to the unified
PositionType(either direct coordinates orCurveAttachment) improves the API design and reduces complexity. TheresolvePositionToVectorhelper cleanly handles both cases, and the useEffect dependencies are properly declared.tests/conftest.py (2)
178-235: Function implementation looks good.The
_create_room_connectionfunction properly creates a persistent room connection with automatic cleanup support. The return type is already declared in the signature.
268-295: Well-designed fixture with automatic cleanup.The
connect_roomfactory fixture provides a clean API for tests that need persistent connections, with proper cleanup guaranteed by the fixture teardown.app/src/components/Canvas.tsx (3)
43-146: Well-encapsulated camera sync logic.The
CameraSyncIntegrationcomponent cleanly handles bidirectional sync between OrbitControls and geometry cameras. The separation into its own component is good design, and theisUpdatingGeometryref prevents sync loops.
169-184: Good UX: auto-attach to session camera.Automatically attaching to the session camera when it becomes available provides a smooth initialization experience for users.
208-217: Proper initialization gating.The guards ensure the Canvas doesn't render until all required data is available (
isConnected,sessionId,settingsResponse,sessionCameraData), preventing race conditions and undefined state.src/zndraw/app/events.py (2)
717-832: Well-designed session-based room joining.The new
room:joinflow properly establishes session state, creates bidirectional mappings, and returns all necessary initialization data. The separation of small metadata (returned in socket response) from large geometries (fetched via REST) is a good performance optimization.The session camera creation for frontend clients with
helper_visible=Falseandprotected=Trueis appropriate for preventing clutter and accidental deletion.
297-310: Good session cleanup on disconnect.The cleanup logic properly removes frontend session data, including the session camera geometry and session settings, preventing resource leaks.
app/src/myapi/client.ts (2)
577-585: All callers ofgetSettingshave been correctly updated.The function's breaking change (addition of the
sessionIdparameter) has been properly handled. The only caller in the codebase (useSettings.tsline 29) correctly passes bothroomIdandsessionId. Error handling is also in place with a guard that throws ifsessionIdis missing.
587-597: All callers ofupdateSettingshave been properly updated with thesessionIdparameter.The function signature change is a breaking change, but the single call site in
useSettings.tscorrectly passessessionIdextracted from the app store. The hook properly handles the new signature and consumers likeSettingsPaneluse the hook correctly, providing onlyroomIdanddatato the mutation while the hook injects thesessionIdfrom the store before calling the API.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @app/src/components/Canvas.tsx:
- Around line 2-17: Import the proper React ref type and make the ref nullable:
replace usages of React.RefObject with an imported RefObject (e.g., import {
RefObject } from "react") or add an import for React namespace, and update the
OrbitControls ref declaration from useRef<OrbitControlsImpl>(null) to
useRef<OrbitControlsImpl | null>(null) (the variable referenced is the
OrbitControls ref, e.g., controlsRef) so the type matches the null initializer
and satisfies strictNullChecks.
In @app/src/components/three/Camera.tsx:
- Around line 40-77: The resolveInitialPosition function currently throws when a
CurveAttachment references missing geometry, which causes error boundaries to
trip; modify resolveInitialPosition (used by the computedPosition/computedTarget
initializers) to avoid throwing: when isCurveAttachment(positionData) and
geometries[positionData.geometry_key] is missing or incomplete, return a
sensible fallback THREE.Vector3 (e.g., new THREE.Vector3(0,0,0) or a provided
default) and optionally console.warn instead of console.error, allowing
downstream effects to update the real position once the geometry loads.
In @tests/test_camera_model.py:
- Around line 97-111: The test docstring for test_camera_validation_fov_range is
incorrect: it states the fov is between 0 and 180 (exclusive) while the Camera
model enforces 1–179; update the docstring to accurately describe the constraint
(e.g., "Camera fov must be between 1 and 179 (inclusive)") so it matches the
assertions and the Camera model's validation.
In @tests/test_protected_geometry.py:
- Around line 65-85: Replace the explicit raise AssertionError in
test_protected_flag_multi_client_sync with pytest.fail(...) passing the same
formatted message; update the test to import pytest at top if it isn't already
so pytest.fail is available; keep the existing timeout/poll logic and message
content but call pytest.fail(f"Geometry 'protected_cam' not synced to client 2
within {max_wait}s") instead of raising AssertionError.
In @tests/test_session_camera_api.py:
- Around line 180-212: The test test_session_camera_updates_are_independent
currently posts camera updates but doesn’t assert the POST responses, so
failures (4xx/5xx) are only detected later; capture the responses from the two
requests.post calls (the calls that use server, conn1.headers and conn2.headers
with keys camera_key_1 and camera_key_2) and assert each response has a
successful status (e.g., response.status_code == 200 or call
response.raise_for_status()) immediately after each POST to fail fast and
provide clearer diagnostics.
- Around line 251-270: The polling loop in the test (the while elapsed <
max_wait block that checks GET /api/rooms/{room_id}/geometries/{camera_key})
currently uses raise AssertionError(...) in the else branch; replace that with
pytest.fail("Session camera not deleted within {max_wait}s after disconnect") to
satisfy Ruff TRY003, and ensure pytest is imported at the top of
tests/test_session_camera_api.py if not already present.
🧹 Nitpick comments (6)
tests/test_camera_model.py (3)
14-31: Good coverage of defaults + basic overrides; consider assertingprotectedtoo.
These two tests are clean and readable; sinceCameranow hasprotected, it’s worth asserting the default (False) here as well (even though you cover it elsewhere).Also applies to: 33-58
60-120: Preferpytest.mark.parametrizefor the validation matrix (less duplication, clearer intent).
A lot of these tests are the same shape (“given bad value → raises ValidationError”). Parametrizing will reduce maintenance cost.Proposed refactor (example)
+import pytest from pydantic import ValidationError @@ -def test_camera_validation_near_positive(): - """Camera near must be positive.""" - with pytest.raises(ValidationError): - Camera(near=-1.0) - - -def test_camera_validation_far_positive(): - """Camera far must be positive.""" - with pytest.raises(ValidationError): - Camera(far=-1.0) +@pytest.mark.parametrize( + "kwargs", + [ + {"near": -1.0}, + {"far": -1.0}, + {"zoom": 0.0}, + {"zoom": -1.0}, + {"fov": 0.0}, + {"fov": 180.0}, + {"up": (0.0, 0.0, 0.0)}, + {"near": 10.0, "far": 5.0}, + {"near": 10.0, "far": 10.0}, + ], +) +def test_camera_invalid_values_rejected(kwargs): + with pytest.raises(ValidationError): + Camera(**kwargs)
158-184: Serialization/deserialization tests look solid; add one “JSON list → tuple” case.
You’re mostly using tuples inmodel_validate; adding a single test that passes lists (the common JSON shape) would catch regressions in coercion behavior.Also applies to: 186-202, 240-289
tests/test_protected_geometry.py (1)
23-32: When “unprotecting”, prefermodel_copy(update=...)to avoid resetting other camera fields.
ReconstructingCamera(...)with only position/target will implicitly reset fields likefov/near/far/...to defaults.Proposed refactor
- cam = vis.geometries["protected_cam"] - vis.geometries["protected_cam"] = Camera( - position=cam.position, - target=cam.target, - protected=False, - ) + cam = vis.geometries["protected_cam"] + vis.geometries["protected_cam"] = cam.model_copy(update={"protected": False})app/src/components/Canvas.tsx (1)
73-125: Potential geometry↔controls feedback loop:controls.update()may emitchange.
controls.update()inside the “geometry → controls target sync” effect can trigger thechangelistener, which callssyncToGeometry(). IfsyncToGeometry()doesn’t internally guard against “remote-driven” updates, you can get extra writes/chatter.Also applies to: 127-140
app/src/components/three/Camera.tsx (1)
100-147:resolvePositionToVectorpath is good; consider validating CurveAttachment shape explicitly.
Right now, any non-array will go down the CurveAttachment path and rely oncurveKeybeing non-null. A quickisCurveAttachment(positionData)check before logging “missing geometry_key” would make diagnostics more accurate.Also applies to: 148-188
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/components/Canvas.tsxapp/src/components/three/Camera.tsxapp/src/hooks/useGeometryCameraSync.tsapp/src/utils/cameraUtils.tstests/test_camera_model.pytests/test_protected_geometry.pytests/test_session_camera_api.py
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/hooks/useGeometryCameraSync.ts
- app/src/utils/cameraUtils.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*test*.py: MUST NEVER use@pytest.mark.xfailor similar; all tests must pass
Usepytest.mark.parametrizeto avoid code duplication in tests
Tests should be very specific and test only one thing; avoid complex test setups
Each test must be a function, not a method of a class
Files:
tests/test_camera_model.pytests/test_session_camera_api.pytests/test_protected_geometry.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings that are concise and to the point
Use type hints wherever possible in Python code; useimport typing as tif necessary, but preferlist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Always place imports at the top of Python files
Files:
tests/test_camera_model.pytests/test_session_camera_api.pytests/test_protected_geometry.py
🧠 Learnings (1)
📚 Learning: 2026-01-08T16:00:51.886Z
Learnt from: CR
Repo: zincware/ZnDraw PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T16:00:51.886Z
Learning: Applies to **/*test*.py : MUST NEVER use `pytest.mark.xfail` or similar; all tests must pass
Applied to files:
tests/test_session_camera_api.py
🧬 Code graph analysis (3)
tests/test_session_camera_api.py (3)
tests/conftest.py (2)
server(419-425)connect_room(269-294)src/zndraw/session_manager.py (2)
camera_key(54-56)get(212-234)src/zndraw/socket_manager.py (1)
disconnect(89-94)
app/src/components/Canvas.tsx (4)
app/src/hooks/useCameraControls.ts (2)
ControlsState(4-13)useCameraControls(22-113)app/src/hooks/useGeometryCameraSync.ts (1)
useGeometryCameraSync(40-188)app/src/components/CanvasErrorState.tsx (1)
CanvasErrorState(13-46)app/src/components/CanvasLoadingState.tsx (1)
CanvasLoadingState(8-25)
tests/test_protected_geometry.py (2)
src/zndraw/zndraw.py (2)
ZnDraw(382-1791)geometries(573-574)src/zndraw/geometries/camera.py (1)
Camera(27-223)
🪛 Ruff (0.14.10)
tests/test_session_camera_api.py
266-268: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_protected_geometry.py
82-84: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (3.11, ubuntu-latest)
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.12, ubuntu-latest)
🔇 Additional comments (3)
tests/test_protected_geometry.py (1)
9-17: Core protected/unprotected flows are covered well.
Nice to see both “cannot delete” and “can delete after unprotect” plus default behavior.Also applies to: 19-36, 38-45, 47-54, 56-63
tests/test_session_camera_api.py (1)
13-16: Helper for session camera key is good (single source of truth).tests/test_camera_model.py (1)
204-219: No action needed—exception type is not version-dependent.In Pydantic v2, assigning to a model with
ConfigDict(frozen=True)consistently raisesValidationError(error type "frozen_instance"). This is stable across v2.x releases. The test correctly catchesValidationErroras written.(Note:
TypeErroris raised only by validators when they throw it—not by the frozen model mechanism itself.)
| import { useEffect, useRef } from "react"; | ||
| import { Canvas, useThree } from "@react-three/fiber"; | ||
| import { OrbitControls } from "@react-three/drei"; | ||
| import type { OrbitControls as OrbitControlsImpl } from "three-stdlib"; | ||
| import { useAppStore, getActiveCurves, selectPreferredCurve } from "../store"; | ||
| import { useSettings } from "../hooks/useSettings"; | ||
| import { useTheme } from "@mui/material/styles"; | ||
| import { Snackbar, Alert } from "@mui/material"; | ||
| import { CanvasLoadingState } from "./CanvasLoadingState"; | ||
| import { CanvasErrorState } from "./CanvasErrorState"; | ||
| import { | ||
| Snackbar, | ||
| Alert, | ||
| Box as MuiBox, | ||
| CircularProgress, | ||
| } from "@mui/material"; | ||
| import { useCameraControls } from "../hooks/useCameraControls"; | ||
| useCameraControls, | ||
| type ControlsState, | ||
| } from "../hooks/useCameraControls"; | ||
| import { useGeometryCameraSync } from "../hooks/useGeometryCameraSync"; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the Canvas.tsx file
fd "Canvas.tsx" --type fRepository: zincware/ZnDraw
Length of output: 174
🏁 Script executed:
# Check the TypeScript configuration
find . -name "tsconfig.json" -o -name "tsconfig*.json" | head -5Repository: zincware/ZnDraw
Length of output: 78
🏁 Script executed:
# Read the Canvas.tsx file to see the actual imports and code
cat -n "app/src/components/Canvas.tsx" | head -80Repository: zincware/ZnDraw
Length of output: 3892
🏁 Script executed:
# Continue reading the Canvas.tsx file, focusing on lines around 54-60 and 170-171
sed -n '54,100p' "app/src/components/Canvas.tsx" | cat -nRepository: zincware/ZnDraw
Length of output: 1883
🏁 Script executed:
# Also check lines 170-180 for the other instances mentioned
sed -n '165,180p' "app/src/components/Canvas.tsx" | cat -nRepository: zincware/ZnDraw
Length of output: 596
🏁 Script executed:
# Read the tsconfig.json to check strictNullChecks and jsx settings
cat -n "app/tsconfig.json"Repository: zincware/ZnDraw
Length of output: 532
🏁 Script executed:
# Search for all useRef declarations in the file to see the pattern
rg "useRef" "app/src/components/Canvas.tsx" -n -A 1Repository: zincware/ZnDraw
Length of output: 231
Fix TypeScript types: React.RefObject without namespace import + useRef nullability mismatch.
The code uses React.RefObject on line 58 without importing the React namespace, and line 170 declares useRef<OrbitControlsImpl>(null) where the type parameter doesn't match the null initializer. Under the project's "strict": true TypeScript config with strictNullChecks, both cause compilation errors.
Fix by either importing React namespace or importing RefObject directly from "react", and add | null to the useRef type parameter:
Proposed fix
-import { useEffect, useRef } from "react";
+import { useEffect, useRef, type RefObject } from "react";
@@
function CameraSyncIntegration({
controlsRef,
controlsState,
}: {
- controlsRef: React.RefObject<OrbitControlsImpl | null>;
+ controlsRef: RefObject<OrbitControlsImpl | null>;
controlsState: ControlsState;
}) {
@@
- const orbitControlsRef = useRef<OrbitControlsImpl>(null);
+ const orbitControlsRef = useRef<OrbitControlsImpl | null>(null);Also applies to: line 170
🤖 Prompt for AI Agents
In @app/src/components/Canvas.tsx around lines 2 - 17, Import the proper React
ref type and make the ref nullable: replace usages of React.RefObject with an
imported RefObject (e.g., import { RefObject } from "react") or add an import
for React namespace, and update the OrbitControls ref declaration from
useRef<OrbitControlsImpl>(null) to useRef<OrbitControlsImpl | null>(null) (the
variable referenced is the OrbitControls ref, e.g., controlsRef) so the type
matches the null initializer and satisfies strictNullChecks.
| // Get session camera geometry data | ||
| const sessionCamera = sessionCameraKey ? geometries[sessionCameraKey] : null; | ||
| const sessionCameraData = sessionCamera?.data; | ||
|
|
||
| // Show error state if initialization failed | ||
| if (initializationError) { | ||
| return <CanvasErrorState error={initializationError} />; | ||
| } | ||
|
|
||
| // Return early with loading state until fully connected and data is ready | ||
| // Gate on: 1) isConnected (socket connected), 2) sessionId (room joined), | ||
| // 3) settingsResponse (settings loaded), 4) sessionCameraData (camera geometry loaded) | ||
| if (!isConnected || !sessionId || !settingsResponse || !sessionCameraData) { | ||
| return <CanvasLoadingState />; | ||
| } | ||
|
|
||
| // Backend always returns defaults, so these are guaranteed to exist | ||
| const studioLightingSettings = settingsResponse.data.studio_lighting; | ||
| const cameraSettings = settingsResponse.data.camera; | ||
| const pathtracingSettings = settingsResponse.data.pathtracing; | ||
| const pathtracingEnabled = pathtracingSettings.enabled === true; | ||
|
|
||
| const cameraPosition = sessionCameraData.position as [number, number, number]; | ||
| const cameraFov = sessionCameraData.fov; | ||
| const cameraType = sessionCameraData.camera_type; | ||
| const preserveDrawingBuffer = sessionCameraData.preserve_drawing_buffer; | ||
| const showCrosshair = sessionCameraData.show_crosshair; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate Canvas.tsx
git ls-files | grep -i canvas.tsxRepository: zincware/ZnDraw
Length of output: 131
🏁 Script executed:
# Search for PositionType definition
rg "type PositionType|interface PositionType" -A 3Repository: zincware/ZnDraw
Length of output: 292
🏁 Script executed:
# Search for CurveAttachment definition
rg "type CurveAttachment|interface CurveAttachment|class CurveAttachment" -A 5Repository: zincware/ZnDraw
Length of output: 770
🏁 Script executed:
# Examine the Canvas.tsx file around the flagged lines
git ls-files | grep -i canvas.tsx | head -1 | xargs wc -lRepository: zincware/ZnDraw
Length of output: 92
🏁 Script executed:
# Read Canvas.tsx to see the actual code around lines 200-226 and 239-252
cat -n app/src/components/Canvas.tsx | sed -n '195,260p'Repository: zincware/ZnDraw
Length of output: 3120
🏁 Script executed:
# Check if there's a helper to check for CurveAttachment
rg "isCurveAttachment|is_curve_attachment" -B 2 -A 5Repository: zincware/ZnDraw
Length of output: 9801
🏁 Script executed:
# Look for how sessionCameraData is populated and its type
rg "sessionCameraData|sessionCamera" app/src/components/Canvas.tsx -B 3 -A 2Repository: zincware/ZnDraw
Length of output: 1835
🏁 Script executed:
# Search for where session camera is created/initialized
rg "cam:session" -B 3 -A 5Repository: zincware/ZnDraw
Length of output: 10994
🏁 Script executed:
# Check CameraManager component to see how it handles sessionCameraData
rg "CameraManager" app/src/components -A 10 | head -50Repository: zincware/ZnDraw
Length of output: 3792
🏁 Script executed:
# Look at the CameraManager component file
fd "CameraManager" app/src/components -t fRepository: zincware/ZnDraw
Length of output: 94
🏁 Script executed:
# Read CameraManager.js to see how it handles sessionCameraData
cat -n app/src/components/CameraManager.jsRepository: zincware/ZnDraw
Length of output: 2848
🏁 Script executed:
# Check how session cameras are created/initialized on backend
rg "create_session_camera" -A 20 src/zndraw/app/events.pyRepository: zincware/ZnDraw
Length of output: 1688
🏁 Script executed:
# Look for Camera geometry schema/defaults
rg "class Camera" -A 15 src/zndraw/geometries.py | head -40Repository: zincware/ZnDraw
Length of output: 123
🏁 Script executed:
# Find the geometries file location
fd geometries -t f -e py src/Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
# Search for Camera class definition
rg "class Camera" -A 20 src/Repository: zincware/ZnDraw
Length of output: 2376
🏁 Script executed:
# Get full Camera class definition including defaults
rg "class Camera" -A 50 src/zndraw/geometries/camera.pyRepository: zincware/ZnDraw
Length of output: 2281
🏁 Script executed:
# Check if there's any way CurveAttachment could be assigned to session camera position/target
rg "session.*position|position.*session" -i src/ app/Repository: zincware/ZnDraw
Length of output: 716
🏁 Script executed:
# Check if there are other position/target assignments in Canvas.tsx lines 239-252
cat -n app/src/components/Canvas.tsx | sed -n '239,252p'Repository: zincware/ZnDraw
Length of output: 578
🏁 Script executed:
# Look for all position/target assignments in Canvas.tsx
rg "position|target" app/src/components/Canvas.tsx -n | grep -i "const\|let\|var"Repository: zincware/ZnDraw
Length of output: 347
🏁 Script executed:
# Get the full Camera class defaults
rg "class Camera" -A 100 src/zndraw/geometries/camera.py | head -80Repository: zincware/ZnDraw
Length of output: 2893
🏁 Script executed:
# Check if there are field defaults
rg "position.*=" src/zndraw/geometries/camera.pyRepository: zincware/ZnDraw
Length of output: 350
🏁 Script executed:
# Get the full field definitions for Camera class
rg "position: PositionType" -A 2 src/zndraw/geometries/camera.pyRepository: zincware/ZnDraw
Length of output: 207
🏁 Script executed:
# Check the complete Camera class fields
cat -n src/zndraw/geometries/camera.py | sed -n '1,150p'Repository: zincware/ZnDraw
Length of output: 6077
Add defensive check before casting sessionCameraData.position to match pattern used in CameraManager.
While session cameras are initialized with coordinates (0.0, 5.0, 10.0) and marked protected=True, the type system allows PositionType (coords OR CurveAttachment). For consistency with CameraManager.js (which checks Array.isArray(position)), add the same guard before passing to R3F's camera.position.
Defensive guard (prevents crashes and improves consistency)
- const cameraPosition = sessionCameraData.position as [number, number, number];
+ const cameraPosition = Array.isArray(sessionCameraData.position)
+ ? (sessionCameraData.position as [number, number, number])
+ : ([0, 5, 10] as [number, number, number]); // fallback if CurveAttachmentAlso applies to: 239-252
| /** | ||
| * Resolve initial position from data. | ||
| * For arrays: use directly. For CurveAttachment: try to resolve from geometry data. | ||
| * Backend always provides valid data, so fallback cases should not occur. | ||
| */ | ||
| const resolveInitialPosition = ( | ||
| positionData: PositionType, | ||
| ): THREE.Vector3 => { | ||
| if (Array.isArray(positionData)) { | ||
| return new THREE.Vector3( | ||
| positionData[0], | ||
| positionData[1], | ||
| positionData[2], | ||
| ); | ||
| } | ||
| // CurveAttachment - resolve from geometry data | ||
| if (isCurveAttachment(positionData)) { | ||
| const curveGeometry = geometries[positionData.geometry_key]; | ||
| if ( | ||
| curveGeometry?.type === "Curve" && | ||
| curveGeometry.data?.position?.[0] | ||
| ) { | ||
| const [x, y, z] = curveGeometry.data.position[0]; | ||
| return new THREE.Vector3(x, y, z); | ||
| } | ||
| } | ||
| // Backend should always provide valid data - this indicates a bug | ||
| console.error("Camera: received invalid position data from backend"); | ||
| throw new Error("Invalid camera position data"); | ||
| }; | ||
|
|
||
| const [computedPosition, setComputedPosition] = useState<THREE.Vector3>(() => | ||
| resolveInitialPosition(data.position), | ||
| ); | ||
| const [computedTarget, setComputedTarget] = useState<THREE.Vector3>( | ||
| new THREE.Vector3(0, 0, 0), | ||
| const [computedTarget, setComputedTarget] = useState<THREE.Vector3>(() => | ||
| resolveInitialPosition(data.target), | ||
| ); | ||
|
|
There was a problem hiding this comment.
Avoid throwing on initial CurveAttachment resolution; it’s a common timing case.
If the camera geometry arrives before the referenced curve geometry, resolveInitialPosition() will throw and trip the error boundary even though the curve would resolve shortly after.
Proposed fix (fallback initial vector; let effects resolve later)
const resolveInitialPosition = (
positionData: PositionType,
): THREE.Vector3 => {
if (Array.isArray(positionData)) {
return new THREE.Vector3(
positionData[0],
positionData[1],
positionData[2],
);
}
// CurveAttachment - resolve from geometry data
if (isCurveAttachment(positionData)) {
const curveGeometry = geometries[positionData.geometry_key];
if (
curveGeometry?.type === "Curve" &&
curveGeometry.data?.position?.[0]
) {
const [x, y, z] = curveGeometry.data.position[0];
return new THREE.Vector3(x, y, z);
}
}
- // Backend should always provide valid data - this indicates a bug
- console.error("Camera: received invalid position data from backend");
- throw new Error("Invalid camera position data");
+ // Likely timing: referenced curve not loaded yet. Effects will resolve later.
+ console.warn(
+ `Camera ${geometryKey}: initial position/target not resolvable yet; using fallback`,
+ );
+ return new THREE.Vector3(0, 0, 0);
};🤖 Prompt for AI Agents
In @app/src/components/three/Camera.tsx around lines 40 - 77, The
resolveInitialPosition function currently throws when a CurveAttachment
references missing geometry, which causes error boundaries to trip; modify
resolveInitialPosition (used by the computedPosition/computedTarget
initializers) to avoid throwing: when isCurveAttachment(positionData) and
geometries[positionData.geometry_key] is missing or incomplete, return a
sensible fallback THREE.Vector3 (e.g., new THREE.Vector3(0,0,0) or a provided
default) and optionally console.warn instead of console.error, allowing
downstream effects to update the real position once the geometry loads.
| def test_camera_validation_fov_range(): | ||
| """Camera fov must be between 0 and 180 (exclusive).""" | ||
| with pytest.raises(ValidationError): | ||
| Camera(fov=0.0) | ||
|
|
||
| with pytest.raises(ValidationError): | ||
| Camera(fov=180.0) | ||
|
|
||
| # Valid fov values | ||
| cam1 = Camera(fov=1.0) | ||
| assert cam1.fov == 1.0 | ||
|
|
||
| cam2 = Camera(fov=179.0) | ||
| assert cam2.fov == 179.0 | ||
|
|
There was a problem hiding this comment.
Docstring mismatches the actual constraints (model is 1–179, not (0,180)).
The tests align with the model (1 and 179), but the docstring says “between 0 and 180 (exclusive)”.
🤖 Prompt for AI Agents
In @tests/test_camera_model.py around lines 97 - 111, The test docstring for
test_camera_validation_fov_range is incorrect: it states the fov is between 0
and 180 (exclusive) while the Camera model enforces 1–179; update the docstring
to accurately describe the constraint (e.g., "Camera fov must be between 1 and
179 (inclusive)") so it matches the assertions and the Camera model's
validation.
| def test_protected_flag_multi_client_sync(server): | ||
| """Protected flag is synced correctly between clients.""" | ||
| vis1 = ZnDraw(url=server, room="room-protected-sync", user="tester1") | ||
| vis2 = ZnDraw(url=server, room="room-protected-sync", user="tester2") | ||
|
|
||
| vis1.geometries["protected_cam"] = Camera(protected=True) | ||
|
|
||
| # Poll until geometry is synced to client 2 (max 5 seconds) | ||
| max_wait = 5.0 | ||
| poll_interval = 0.1 | ||
| elapsed = 0.0 | ||
| while elapsed < max_wait: | ||
| if "protected_cam" in vis2.geometries: | ||
| break | ||
| vis2.socket.sio.sleep(poll_interval) | ||
| elapsed += poll_interval | ||
| else: | ||
| raise AssertionError( | ||
| f"Geometry 'protected_cam' not synced to client 2 within {max_wait}s" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Use pytest.fail(...) instead of raising a long AssertionError (Ruff TRY003).
This keeps the same failure semantics but satisfies the linter.
Proposed change
else:
- raise AssertionError(
- f"Geometry 'protected_cam' not synced to client 2 within {max_wait}s"
- )
+ pytest.fail(
+ f"Geometry 'protected_cam' not synced to client 2 within {max_wait}s"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_protected_flag_multi_client_sync(server): | |
| """Protected flag is synced correctly between clients.""" | |
| vis1 = ZnDraw(url=server, room="room-protected-sync", user="tester1") | |
| vis2 = ZnDraw(url=server, room="room-protected-sync", user="tester2") | |
| vis1.geometries["protected_cam"] = Camera(protected=True) | |
| # Poll until geometry is synced to client 2 (max 5 seconds) | |
| max_wait = 5.0 | |
| poll_interval = 0.1 | |
| elapsed = 0.0 | |
| while elapsed < max_wait: | |
| if "protected_cam" in vis2.geometries: | |
| break | |
| vis2.socket.sio.sleep(poll_interval) | |
| elapsed += poll_interval | |
| else: | |
| raise AssertionError( | |
| f"Geometry 'protected_cam' not synced to client 2 within {max_wait}s" | |
| ) | |
| def test_protected_flag_multi_client_sync(server): | |
| """Protected flag is synced correctly between clients.""" | |
| vis1 = ZnDraw(url=server, room="room-protected-sync", user="tester1") | |
| vis2 = ZnDraw(url=server, room="room-protected-sync", user="tester2") | |
| vis1.geometries["protected_cam"] = Camera(protected=True) | |
| # Poll until geometry is synced to client 2 (max 5 seconds) | |
| max_wait = 5.0 | |
| poll_interval = 0.1 | |
| elapsed = 0.0 | |
| while elapsed < max_wait: | |
| if "protected_cam" in vis2.geometries: | |
| break | |
| vis2.socket.sio.sleep(poll_interval) | |
| elapsed += poll_interval | |
| else: | |
| pytest.fail( | |
| f"Geometry 'protected_cam' not synced to client 2 within {max_wait}s" | |
| ) |
🧰 Tools
🪛 Ruff (0.14.10)
82-84: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @tests/test_protected_geometry.py around lines 65 - 85, Replace the explicit
raise AssertionError in test_protected_flag_multi_client_sync with
pytest.fail(...) passing the same formatted message; update the test to import
pytest at top if it isn't already so pytest.fail is available; keep the existing
timeout/poll logic and message content but call pytest.fail(f"Geometry
'protected_cam' not synced to client 2 within {max_wait}s") instead of raising
AssertionError.
| def test_session_camera_updates_are_independent(server, connect_room): | ||
| """Updates to one session's camera don't affect another session's camera.""" | ||
| room_id = "test-camera-independence" | ||
| conn1 = connect_room(room_id, user="user1") | ||
| conn2 = connect_room(room_id, user="user2") | ||
|
|
||
| camera_key_1 = _camera_key(conn1.session_id) | ||
| camera_key_2 = _camera_key(conn2.session_id) | ||
|
|
||
| # Update first camera | ||
| requests.post( | ||
| f"{server}/api/rooms/{room_id}/geometries", | ||
| headers=conn1.headers, | ||
| json={ | ||
| "key": camera_key_1, | ||
| "type": "Camera", | ||
| "data": {"fov": 60.0, "position": [1.0, 1.0, 1.0]}, | ||
| }, | ||
| timeout=10, | ||
| ) | ||
|
|
||
| # Update second camera with different values | ||
| requests.post( | ||
| f"{server}/api/rooms/{room_id}/geometries", | ||
| headers=conn2.headers, | ||
| json={ | ||
| "key": camera_key_2, | ||
| "type": "Camera", | ||
| "data": {"fov": 90.0, "position": [2.0, 2.0, 2.0]}, | ||
| }, | ||
| timeout=10, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Assert POST results in the “independence” test for clearer failures.
Right now, a 4xx/5xx will only be caught indirectly later.
Proposed change
- requests.post(
+ r1 = requests.post(
f"{server}/api/rooms/{room_id}/geometries",
headers=conn1.headers,
json={
@@
timeout=10,
)
+ assert r1.status_code == 200
- requests.post(
+ r2 = requests.post(
f"{server}/api/rooms/{room_id}/geometries",
headers=conn2.headers,
json={
@@
timeout=10,
)
+ assert r2.status_code == 200📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_session_camera_updates_are_independent(server, connect_room): | |
| """Updates to one session's camera don't affect another session's camera.""" | |
| room_id = "test-camera-independence" | |
| conn1 = connect_room(room_id, user="user1") | |
| conn2 = connect_room(room_id, user="user2") | |
| camera_key_1 = _camera_key(conn1.session_id) | |
| camera_key_2 = _camera_key(conn2.session_id) | |
| # Update first camera | |
| requests.post( | |
| f"{server}/api/rooms/{room_id}/geometries", | |
| headers=conn1.headers, | |
| json={ | |
| "key": camera_key_1, | |
| "type": "Camera", | |
| "data": {"fov": 60.0, "position": [1.0, 1.0, 1.0]}, | |
| }, | |
| timeout=10, | |
| ) | |
| # Update second camera with different values | |
| requests.post( | |
| f"{server}/api/rooms/{room_id}/geometries", | |
| headers=conn2.headers, | |
| json={ | |
| "key": camera_key_2, | |
| "type": "Camera", | |
| "data": {"fov": 90.0, "position": [2.0, 2.0, 2.0]}, | |
| }, | |
| timeout=10, | |
| ) | |
| def test_session_camera_updates_are_independent(server, connect_room): | |
| """Updates to one session's camera don't affect another session's camera.""" | |
| room_id = "test-camera-independence" | |
| conn1 = connect_room(room_id, user="user1") | |
| conn2 = connect_room(room_id, user="user2") | |
| camera_key_1 = _camera_key(conn1.session_id) | |
| camera_key_2 = _camera_key(conn2.session_id) | |
| # Update first camera | |
| r1 = requests.post( | |
| f"{server}/api/rooms/{room_id}/geometries", | |
| headers=conn1.headers, | |
| json={ | |
| "key": camera_key_1, | |
| "type": "Camera", | |
| "data": {"fov": 60.0, "position": [1.0, 1.0, 1.0]}, | |
| }, | |
| timeout=10, | |
| ) | |
| assert r1.status_code == 200 | |
| # Update second camera with different values | |
| r2 = requests.post( | |
| f"{server}/api/rooms/{room_id}/geometries", | |
| headers=conn2.headers, | |
| json={ | |
| "key": camera_key_2, | |
| "type": "Camera", | |
| "data": {"fov": 90.0, "position": [2.0, 2.0, 2.0]}, | |
| }, | |
| timeout=10, | |
| ) | |
| assert r2.status_code == 200 |
🤖 Prompt for AI Agents
In @tests/test_session_camera_api.py around lines 180 - 212, The test
test_session_camera_updates_are_independent currently posts camera updates but
doesn’t assert the POST responses, so failures (4xx/5xx) are only detected
later; capture the responses from the two requests.post calls (the calls that
use server, conn1.headers and conn2.headers with keys camera_key_1 and
camera_key_2) and assert each response has a successful status (e.g.,
response.status_code == 200 or call response.raise_for_status()) immediately
after each POST to fail fast and provide clearer diagnostics.
| # Poll until camera is deleted (max 5 seconds) | ||
| max_wait = 5.0 | ||
| poll_interval = 0.1 | ||
| elapsed = 0.0 | ||
| while elapsed < max_wait: | ||
| response = requests.get( | ||
| f"{server}/api/rooms/{room_id}/geometries/{camera_key}", | ||
| headers=conn.headers, | ||
| timeout=10, | ||
| ) | ||
| if response.status_code == 404: | ||
| break | ||
| time.sleep(poll_interval) | ||
| elapsed += poll_interval | ||
| else: | ||
| raise AssertionError( | ||
| f"Session camera not deleted within {max_wait}s after disconnect" | ||
| ) | ||
|
|
||
| assert response.status_code == 404 |
There was a problem hiding this comment.
Replace raise AssertionError(...) with pytest.fail(...) (Ruff TRY003).
Same rationale as the other polling test.
Proposed change
else:
- raise AssertionError(
- f"Session camera not deleted within {max_wait}s after disconnect"
- )
+ import pytest
+ pytest.fail(
+ f"Session camera not deleted within {max_wait}s after disconnect"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Poll until camera is deleted (max 5 seconds) | |
| max_wait = 5.0 | |
| poll_interval = 0.1 | |
| elapsed = 0.0 | |
| while elapsed < max_wait: | |
| response = requests.get( | |
| f"{server}/api/rooms/{room_id}/geometries/{camera_key}", | |
| headers=conn.headers, | |
| timeout=10, | |
| ) | |
| if response.status_code == 404: | |
| break | |
| time.sleep(poll_interval) | |
| elapsed += poll_interval | |
| else: | |
| raise AssertionError( | |
| f"Session camera not deleted within {max_wait}s after disconnect" | |
| ) | |
| assert response.status_code == 404 | |
| # Poll until camera is deleted (max 5 seconds) | |
| max_wait = 5.0 | |
| poll_interval = 0.1 | |
| elapsed = 0.0 | |
| while elapsed < max_wait: | |
| response = requests.get( | |
| f"{server}/api/rooms/{room_id}/geometries/{camera_key}", | |
| headers=conn.headers, | |
| timeout=10, | |
| ) | |
| if response.status_code == 404: | |
| break | |
| time.sleep(poll_interval) | |
| elapsed += poll_interval | |
| else: | |
| pytest.fail( | |
| f"Session camera not deleted within {max_wait}s after disconnect" | |
| ) | |
| assert response.status_code == 404 |
🧰 Tools
🪛 Ruff (0.14.10)
266-268: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @tests/test_session_camera_api.py around lines 251 - 270, The polling loop in
the test (the while elapsed < max_wait block that checks GET
/api/rooms/{room_id}/geometries/{camera_key}) currently uses raise
AssertionError(...) in the else branch; replace that with pytest.fail("Session
camera not deleted within {max_wait}s after disconnect") to satisfy Ruff TRY003,
and ensure pytest is imported at the top of tests/test_session_camera_api.py if
not already present.
Summary by CodeRabbit
New Features
Changes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.