Skip to content

Simplify locking: @check_lock backend + frontend simplification#846

Merged
PythonFZ merged 6 commits intomainfrom
feature/frontend-lock-simplification
Jan 15, 2026
Merged

Simplify locking: @check_lock backend + frontend simplification#846
PythonFZ merged 6 commits intomainfrom
feature/frontend-lock-simplification

Conversation

@PythonFZ
Copy link
Copy Markdown
Member

@PythonFZ PythonFZ commented Jan 15, 2026

TODO

  • manual testing

Backend: Replace @requires_lock with @check_lock decorator

  • Operations proceed unless blocked by another session's lock
  • FIFO ordering handles concurrency when no lock exists
  • Fine-grained locks: geometry:{key}, step, bookmarks, global
  • Remove deprecated @requires_lock decorator

Frontend: Remove automatic lock acquisition

  • Simplify createGeometry/deleteGeometry - remove withAutoLock
  • Remove withAutoLock function from client.ts
  • Simplify useStepControl - remove auto-lock acquisition
  • Delete useLockManager.ts hook (no longer needed)
  • Remove lockToken parameter from geometry persistence
  • Add 423 error handling with snackbar notifications
  • Fix GeometryListResponse type to include types property

Bug fix:

  • Fix AttributeError in socket_manager.py bookmark exception handling

Tests:

  • Add test_check_lock_decorator.py for new behavior
  • Update test_step_endpoints.py for @check_lock semantics
  • Update test_partial_frame_update.py
  • Remove test_requires_lock_decorator.py

Summary by CodeRabbit

  • Refactor

    • Client now relies on server-side lock checks; local lock tokens and automatic client-side lock management removed.
    • Lock semantics changed to allow operations to proceed when no lock exists (FIFO); conflicting sessions are blocked.
  • User Experience

    • Lock conflicts now produce clearer warnings (423 responses); UI shows snackbars when lock is lost.
  • Tests

    • Extensive test suite added/updated to validate new lock-checking and concurrency behaviors.

✏️ Tip: You can customize this high-level summary in your review settings.

Backend: Replace @requires_lock with @check_lock decorator
- Operations proceed unless blocked by another session's lock
- FIFO ordering handles concurrency when no lock exists
- Fine-grained locks: geometry:{key}, step, bookmarks, global
- Remove deprecated @requires_lock decorator

Frontend: Remove automatic lock acquisition
- Simplify createGeometry/deleteGeometry - remove withAutoLock
- Remove withAutoLock function from client.ts
- Simplify useStepControl - remove auto-lock acquisition
- Delete useLockManager.ts hook (no longer needed)
- Remove lockToken parameter from geometry persistence
- Add 423 error handling with snackbar notifications
- Fix GeometryListResponse type to include types property

Bug fix:
- Fix AttributeError in socket_manager.py bookmark exception handling

Tests:
- Add test_check_lock_decorator.py for new behavior
- Update test_step_endpoints.py for @check_lock semantics
- Update test_partial_frame_update.py
- Remove test_requires_lock_decorator.py

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jan 15, 2026

📝 Walkthrough

Walkthrough

Client-side lock token management was removed and server-side lock checks (via a new check_lock) were introduced. Client calls no longer pass lock tokens; backend decorators and lock acquisition logic were updated for per-key and global lock checks; tests were rewritten to exercise the new semantics.

Changes

Cohort / File(s) Change Summary
Frontend: Lock removal & step control
app/src/hooks/useLockManager.ts, app/src/hooks/useStepControl.ts
Deleted useLockManager hook; removed client lock acquire/release/heartbeat logic; setStep now returns void and no longer uses lock tokens; 423 error handling shows snackbar.
Frontend: Geometry/Curve calls
app/src/components/three/Curve.tsx, app/src/hooks/useGeometryCameraSync.ts, app/src/hooks/useGeometryPersistence.ts, app/src/store.tsx
Removed retrieval/passing of lock tokens to createGeometry calls; updated effects/deps/comments to rely on server @check_lock.
Frontend: API client & socket integration
app/src/myapi/client.ts, app/src/hooks/useSocketManager.ts, app/src/hooks/*
Removed withAutoLock; simplified createGeometry/deleteGeometry signatures (no lockToken); added 423 handling with snackbar; added getLockStatus usage in room join secondary fetch to populate trajectory:meta lock status.
Backend: Lock utilities & decorator
src/zndraw/app/route_utils.py
Replaced requires_lock with check_lock supporting dynamic targets (LockTargetType) and separated global vs target-specific checks (return 423 on conflict).
Backend: Route decorator updates
src/zndraw/app/bookmark_routes.py, src/zndraw/app/frame_routes.py, src/zndraw/app/room_routes.py, src/zndraw/app/geometry_routes.py
Swapped @requires_lock@check_lock across many endpoints; added helpers _get_geometry_key/_get_figure_key for per-key locking; minor docstring and sid-decoding cleanup.
Backend: Lock acquisition & disconnect events
src/zndraw/app/lock_routes.py, src/zndraw/app/events.py
acquire_lock now blocks per-target lock acquisition when a global lock exists; idempotent acquire refreshes session_locks TTL; disconnect cleanup emits lock-release events for orphaned locks.
Zndraw: upload locking refactor
src/zndraw/zndraw.py, src/zndraw/app/tasks.py
extend acquires outer distributed lock and delegates to new _extend; batch processing wrapped by vis.get_lock for thread-safe operations.
Socket manager tweak
src/zndraw/socket_manager.py
Safer error handling when checking response.status_code during bookmark invalidation (use getattr).
Tests: lock/decorator changes
tests/test_check_lock_decorator.py, tests/test_requires_lock_decorator.py, tests/test_partial_frame_update.py, tests/test_step_endpoints.py
Added comprehensive @check_lock test suite; removed legacy @requires_lock tests; updated many endpoint tests to reflect FIFO/no-lock-pass semantics and added timeouts.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant API as Server/API
    participant LockStore as Redis
    participant DB

    Client->>API: POST /rooms/:room/geometries (no lock token)
    API->>LockStore: check global lock for room
    alt global lock held by other session
        LockStore-->>API: lock held (other)
        API-->>Client: 423 Locked (with holder info)
    else no conflicting global lock
        API->>LockStore: resolve target (e.g., geometry key) -> check target lock
        alt target lock held by other session
            LockStore-->>API: lock held (other)
            API-->>Client: 423 Locked
        else no conflicting target lock
            API->>DB: persist geometry data
            DB-->>API: 200 OK
            API-->>Client: 200/201 Created
            API->>Client: emit lock update via socket (optional)
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐰
I hopped in code with eager paws,
Dropped tokens, trusted server laws.
Locks now checked where they belong,
FIFO hums a steadier song.
A carrot-cheer for sync so strong! 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title directly summarizes the main changes: replacing @requires_lock with @check_lock decorator and simplifying locking on both backend and frontend, which aligns with the substantial refactoring across the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 91.25% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/zndraw/app/route_utils.py`:
- Around line 211-216: The dynamic target resolution block (when target is
callable and sets actual_target = target(request, kwargs)) lacks error handling;
wrap the callable invocation in a try/except that catches AttributeError,
TypeError (and optionally ValueError) raised during inspection of request.json
and on error return an HTTP 400 response with a clear message (e.g., "Invalid or
missing request body for dynamic target: <error>") so callers get a client error
instead of a server crash; update the logic around target, actual_target,
request and kwargs to perform this guarded call and surface the error message in
the 400 response.
🧹 Nitpick comments (6)
tests/test_partial_frame_update.py (2)

162-193: Consider adding a timeout to the requests call.

The test correctly validates FIFO behavior when no lock exists. However, static analysis flagged the missing timeout parameter on the requests.patch call, which could cause tests to hang indefinitely if the server becomes unresponsive.

💡 Add timeout parameter
     response = requests.patch(
         f"{server}/api/rooms/{room}/frames/0/partial",
         data=update_data,
         headers={
             **auth_headers,
             "X-Session-ID": session_id,
             "Content-Type": "application/msgpack",
         },
+        timeout=30,
     )

342-377: Good test coverage for lock blocking behavior.

The test properly validates that a second session is blocked (HTTP 423) when another session holds the lock. The assertion on line 377 defensively checks for either "locked" or the locking user's name in the error message.

One minor note: lines 352 and 366 are flagged by static analysis for missing timeout parameters on requests calls.

💡 Add timeout parameters for test reliability
     response = requests.post(
         f"{server}/api/rooms/{room}/locks/trajectory:meta/acquire",
         json={"msg": "user1 lock"},
         headers=conn1.headers,
+        timeout=30,
     )
     assert response.status_code == 200

     # Session 2 joins (different user)
     conn2 = connect_room(room, user="user2")

     # Session 2 tries to update - should be blocked
     new_positions = np.array([[0, 0, 0]], dtype=np.float64)
     update_data = _encode_numpy_for_msgpack({"arrays.positions": new_positions})

     response = requests.patch(
         f"{server}/api/rooms/{room}/frames/0/partial",
         data=update_data,
         headers={
             **conn2.headers,
             "Content-Type": "application/msgpack",
         },
+        timeout=30,
     )
app/src/myapi/client.ts (1)

227-273: Consider extracting duplicated 423 error handling.

Both createGeometry and deleteGeometry have identical error handling for HTTP 423. This could be extracted into a helper or handled via an axios response interceptor for consistency.

💡 Optional: Extract 423 handling to reduce duplication
// Helper for geometry operations with lock error handling
async function withLockErrorHandling<T>(
  operation: () => Promise<T>,
  errorMessage: string,
): Promise<T> {
  try {
    return await operation();
  } catch (error: any) {
    if (error.response?.status === 423) {
      useAppStore.getState().showSnackbar(errorMessage, "warning");
    }
    throw error;
  }
}

// Usage in createGeometry:
export const createGeometry = async (
  roomId: string,
  key: string,
  geometryType: string,
  geometryData: Record<string, any>,
): Promise<{ status: string }> => {
  return withLockErrorHandling(
    async () => {
      const { data } = await apiClient.post(`/api/rooms/${roomId}/geometries`, {
        key,
        type: geometryType,
        data: geometryData,
      });
      return data;
    },
    "Cannot create geometry - resource is locked by another user",
  );
};
tests/test_step_endpoints.py (1)

67-71: Consider adding timeout to requests calls.

Static analysis flagged this and other requests calls (lines 231, 283, 340) as missing timeouts. While this is acceptable for test code running against a local server, adding a timeout (e.g., timeout=10) would prevent tests from hanging indefinitely if the server becomes unresponsive.

Example fix
     response = requests.put(
         f"{server}/api/rooms/{room}/step",
         json={"step": 42},
         headers={**auth_headers, "X-Session-ID": session_id},
+        timeout=10,
     )
tests/test_check_lock_decorator.py (1)

13-32: Unused fixture parameter.

The get_jwt_auth_headers parameter is unused since connect_room handles authentication internally. Consider removing it to avoid confusion.

Suggested fix
 `@pytest.fixture`
-def room_with_lock(server, get_jwt_auth_headers, connect_room):
+def room_with_lock(server, connect_room):
     """Create a room, join it, acquire a lock, and return connection info."""
src/zndraw/app/room_routes.py (1)

833-850: Minor: Unicode character in docstring.

Line 840 uses × (multiplication sign) instead of x. While functionally fine, it may cause issues with some text editors or documentation generators.

Suggested fix
-    - Continuous updates with lock: acquire lock → PUT step × N → release lock
+    - Continuous updates with lock: acquire lock → PUT step x N → release lock

Note on lock semantics: The update_step endpoint intentionally omits forbid=["room:locked"], allowing step changes even when a room is permanently locked. This makes sense since navigating frames is a read-like operation that shouldn't be blocked by room-level locks.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 543900a and 5dcf5f0.

📒 Files selected for processing (18)
  • app/src/components/three/Curve.tsx
  • app/src/hooks/useGeometryCameraSync.ts
  • app/src/hooks/useGeometryPersistence.ts
  • app/src/hooks/useLockManager.ts
  • app/src/hooks/useStepControl.ts
  • app/src/myapi/client.ts
  • app/src/store.tsx
  • src/zndraw/app/bookmark_routes.py
  • src/zndraw/app/frame_routes.py
  • src/zndraw/app/geometry_routes.py
  • src/zndraw/app/lock_routes.py
  • src/zndraw/app/room_routes.py
  • src/zndraw/app/route_utils.py
  • src/zndraw/socket_manager.py
  • tests/test_check_lock_decorator.py
  • tests/test_partial_frame_update.py
  • tests/test_requires_lock_decorator.py
  • tests/test_step_endpoints.py
💤 Files with no reviewable changes (2)
  • app/src/hooks/useLockManager.ts
  • tests/test_requires_lock_decorator.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: You MUST NEVER @pytest.mark.xfail or similar - all tests must pass!
All default values (e.g., camera, particles) must be defined exclusively within the Pydantic model. Do not scatter fallback logic throughout the codebase.
Do not perform null checks combined with hardcoded literals for default values. Rely entirely on the schema to populate default values during initialization.
You can not use LUA scripts with Redis!
If sensible, implement collections.abc interfaces for your classes, such as MutableMapping or MutableSequence.
Use numpy style docstrings in Python code.
Docstrings must be concise and to the point.
Use type hints wherever possible in Python. Use list[int|float] | None instead of t.Optional[t.List[int|float]]!
Imports should always be at the top of the file in Python.

Files:

  • src/zndraw/socket_manager.py
  • src/zndraw/app/lock_routes.py
  • src/zndraw/app/frame_routes.py
  • tests/test_check_lock_decorator.py
  • tests/test_partial_frame_update.py
  • src/zndraw/app/room_routes.py
  • tests/test_step_endpoints.py
  • src/zndraw/app/route_utils.py
  • src/zndraw/app/geometry_routes.py
  • src/zndraw/app/bookmark_routes.py
**/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

When designing new tests, read the old tests first to understand the existing patterns. Use pytest.mark.parametrize to avoid code duplication. 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_check_lock_decorator.py
  • tests/test_partial_frame_update.py
  • tests/test_step_endpoints.py
🧬 Code graph analysis (13)
app/src/hooks/useStepControl.ts (1)
app/src/myapi/client.ts (1)
  • updateStep (1312-1318)
app/src/hooks/useGeometryCameraSync.ts (2)
app/src/myapi/client.ts (1)
  • createGeometry (227-251)
app/src/components/CameraManager.js (1)
  • attachedCameraKey (17-17)
app/src/components/three/Curve.tsx (3)
app/src/myapi/client.ts (1)
  • createGeometry (227-251)
src/zndraw/app/redis_keys.py (1)
  • geometries (308-310)
src/zndraw/zndraw.py (1)
  • geometries (573-574)
app/src/store.tsx (1)
app/src/myapi/client.ts (1)
  • createGeometry (227-251)
src/zndraw/app/lock_routes.py (2)
src/zndraw/app/route_utils.py (1)
  • get_lock_key (27-42)
src/zndraw/app/events.py (1)
  • get_lock_key (30-33)
src/zndraw/app/frame_routes.py (1)
src/zndraw/app/route_utils.py (1)
  • check_lock (68-253)
tests/test_check_lock_decorator.py (3)
tests/conftest.py (4)
  • get_jwt_auth_headers (263-265)
  • connect_room (281-306)
  • create_and_join_room (269-271)
  • s22 (475-477)
src/zndraw/app/redis_keys.py (1)
  • session_id (617-619)
tests/test_room_lock_enforcement.py (1)
  • auth_headers (44-46)
tests/test_partial_frame_update.py (1)
tests/conftest.py (2)
  • server (431-437)
  • connect_room (281-306)
src/zndraw/app/room_routes.py (3)
src/zndraw/app/route_utils.py (1)
  • check_lock (68-253)
src/zndraw/app/redis_keys.py (1)
  • session_id (617-619)
src/zndraw/api_manager.py (1)
  • update_step (1609-1656)
tests/test_step_endpoints.py (4)
tests/test_room_lock_enforcement.py (1)
  • auth_headers (44-46)
tests/conftest.py (4)
  • get_jwt_auth_headers (263-265)
  • server (431-437)
  • create_and_join_room (269-271)
  • connect_room (281-306)
misc/conftest.py (1)
  • server (41-69)
src/zndraw/app/redis_keys.py (1)
  • session_id (617-619)
src/zndraw/app/route_utils.py (3)
src/zndraw/storage/asebytes_backend.py (1)
  • get (46-103)
src/zndraw/app/redis_keys.py (1)
  • lock (270-283)
src/zndraw/auth.py (2)
  • get_current_user (135-153)
  • AuthError (20-34)
src/zndraw/app/geometry_routes.py (1)
src/zndraw/app/route_utils.py (1)
  • check_lock (68-253)
src/zndraw/app/bookmark_routes.py (1)
src/zndraw/app/route_utils.py (1)
  • check_lock (68-253)
🪛 Ruff (0.14.11)
tests/test_check_lock_decorator.py

14-14: Unused function argument: get_jwt_auth_headers

(ARG001)


22-22: Probable use of requests call without timeout

(S113)


46-46: Probable use of requests call without timeout

(S113)


68-68: Probable use of requests call without timeout

(S113)


93-93: Probable use of requests call without timeout

(S113)


108-108: Probable use of requests call without timeout

(S113)


126-126: Probable use of requests call without timeout

(S113)


142-142: Probable use of requests call without timeout

(S113)


173-173: Probable use of requests call without timeout

(S113)


188-188: Probable use of requests call without timeout

(S113)


206-206: Probable use of requests call without timeout

(S113)


214-214: Probable use of requests call without timeout

(S113)


234-234: Probable use of requests call without timeout

(S113)


246-246: Probable use of requests call without timeout

(S113)


254-254: Probable use of requests call without timeout

(S113)


277-277: Probable use of requests call without timeout

(S113)


290-290: Probable use of requests call without timeout

(S113)


309-309: Probable use of requests call without timeout

(S113)


321-321: Probable use of requests call without timeout

(S113)


333-333: Probable use of requests call without timeout

(S113)


353-353: Probable use of requests call without timeout

(S113)


374-374: Probable use of requests call without timeout

(S113)


394-394: Probable use of requests call without timeout

(S113)


406-406: Probable use of requests call without timeout

(S113)


430-430: Probable use of requests call without timeout

(S113)


438-438: Probable use of requests call without timeout

(S113)


458-458: Probable use of requests call without timeout

(S113)


469-469: Probable use of requests call without timeout

(S113)


481-481: Probable use of requests call without timeout

(S113)

tests/test_partial_frame_update.py

181-181: Probable use of requests call without timeout

(S113)


352-352: Probable use of requests call without timeout

(S113)


366-366: Probable use of requests call without timeout

(S113)

src/zndraw/app/room_routes.py

636-636: Unused function argument: session_id

(ARG001)


636-636: Unused function argument: user_id

(ARG001)


835-835: Unused function argument: user_id

(ARG001)


840-840: Docstring contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?

(RUF002)

tests/test_step_endpoints.py

67-67: Probable use of requests call without timeout

(S113)


231-231: Probable use of requests call without timeout

(S113)


283-283: Probable use of requests call without timeout

(S113)


340-340: Probable use of requests call without timeout

(S113)

src/zndraw/app/geometry_routes.py

46-46: Unused function argument: kwargs

(ARG001)


54-54: Unused function argument: session_id

(ARG001)


54-54: Unused function argument: user_id

(ARG001)


168-168: Unused lambda argument: req

(ARG005)


169-169: Unused function argument: session_id

(ARG001)


169-169: Unused function argument: user_id

(ARG001)


262-262: Unused function argument: kwargs

(ARG001)


310-310: Unused lambda argument: req

(ARG005)

src/zndraw/app/bookmark_routes.py

63-63: Unused function argument: session_id

(ARG001)


63-63: Unused function argument: user_id

(ARG001)


110-110: Unused function argument: session_id

(ARG001)


110-110: Unused function argument: user_id

(ARG001)

⏰ 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.13, ubuntu-latest)
  • GitHub Check: pytest (3.11, ubuntu-latest)
  • GitHub Check: pytest (3.12, ubuntu-latest)
🔇 Additional comments (42)
tests/test_partial_frame_update.py (2)

45-60: LGTM! New fixture properly supports FIFO behavior testing.

The room_with_frames_no_lock fixture correctly creates a room without acquiring a lock, enabling tests to validate that operations proceed under FIFO ordering when no lock exists. This aligns well with the @check_lock semantics.


514-533: LGTM! Updated test aligns with FIFO semantics.

The comment update at line 518 correctly reflects that no lock is needed for the empty room test under the new @check_lock behavior.

app/src/hooks/useStepControl.ts (4)

38-64: LGTM! Clean throttled update implementation with proper error handling.

The throttled server update correctly:

  • Guards against missing roomId
  • Handles HTTP 423 (locked) with a user-friendly snackbar
  • Uses leading + trailing throttle for responsive UX while ensuring final value syncs
  • Logs non-423 errors for debugging

The dependency array correctly includes only roomId.


67-81: LGTM! Simplified setStep implementation.

The function properly:

  1. Updates local state immediately for responsive UI
  2. Delegates to throttled server update without client-side lock management
  3. Server handles locking via @check_lock

The dependency array correctly includes all referenced values.


84-104: LGTM! Lock event handling correctly determines remoteLocked state.

The logic properly ignores events from the current session and sets remoteLocked based on whether another client has acquired or released the lock.


7-10: API signature change does not break existing callers.

The return type changed from Promise<void> to void. All call sites invoke setStep directly without awaiting, so this change is safe and does not break any existing code.

app/src/myapi/client.ts (2)

97-98: LGTM! Comment correctly updated to reflect new decorator.

The comment now accurately documents that @check_lock is used for frame uploads and geometry operations.


202-208: LGTM! Type extension is additive and non-breaking.

The optional types field with schemas and defaults properly extends GeometryListResponse without breaking existing consumers.

app/src/hooks/useGeometryPersistence.ts (1)

44-58: LGTM! Lock token removal aligns with server-side enforcement.

The createGeometry call now correctly relies on server-side @check_lock validation. The existing error handling will properly catch and log any 423 (locked) errors that propagate from the API client.

app/src/hooks/useGeometryCameraSync.ts (1)

93-104: LGTM! Consistent with other geometry persistence changes.

The lock token removal mirrors the change in useGeometryPersistence.ts. The error handling correctly:

  1. Logs the error for debugging
  2. Clears lastSentRef so subsequent updates can proceed

The 423 error handling in createGeometry will show the appropriate snackbar before this catch block logs the error.

app/src/store.tsx (1)

975-978: LGTM! Lock token removal aligns with server-side @check_lock.

The createGeometry call correctly omits the lock token. The server's @check_lock decorator will allow this operation because:

  1. The lock for trajectory:meta was already acquired on line 943-949
  2. HTTP 423 errors are handled in client.ts with a snackbar notification
app/src/components/three/Curve.tsx (1)

766-771: LGTM! Persistence correctly relies on server-side lock checking.

The dependency array is correct—roomId, geometryKey, and geometries are all the values used in the callback. The createGeometry call will proceed with FIFO ordering when no lock exists, and HTTP 423 errors are handled in the client with user-facing snackbar notifications.

tests/test_step_endpoints.py (3)

1-5: Clear module docstring documenting the new lock semantics.

The docstring accurately explains the @check_lock behavior: operations proceed with FIFO ordering when no lock exists, and blocking only occurs when another session holds the lock.


55-77: Test correctly validates FIFO behavior when no lock exists.

The test properly verifies that PUT /step succeeds with a 200 response when no lock is held, aligning with the new @check_lock semantics.


266-291: Test correctly validates blocking by another session's lock.

The test properly verifies that a different session cannot update step when another session holds the lock, returning HTTP 423.

One minor observation: the assertion on line 291 uses an or condition which is flexible but could mask unexpected error messages. Consider a more specific assertion if the error format is stable.

src/zndraw/socket_manager.py (1)

231-239: Good bug fix for AttributeError in exception handling.

The use of getattr(e, "response", None) with a subsequent None check correctly handles cases where the exception may not have a response attribute (e.g., network errors vs HTTP errors). This prevents the AttributeError mentioned in the PR objectives.

src/zndraw/app/lock_routes.py (1)

79-102: Global lock pre-check correctly blocks non-global lock acquisition.

The logic properly checks for a global lock before acquiring any target-specific lock, returning HTTP 423 when another session holds the global lock. The same-session check (line 86) correctly allows a session to acquire additional locks while holding the global lock.

One consideration: when JSONDecodeError occurs (line 100-101), the code logs a warning but proceeds with lock acquisition. If global lock data is corrupted, this could inadvertently allow an operation that should be blocked. However, this is a defensive choice—blocking on corrupted data could cause broader issues.

src/zndraw/app/bookmark_routes.py (3)

14-14: LGTM!

Import correctly updated to use the new check_lock decorator.


62-66: LGTM!

The decorator change from @requires_lock to @check_lock(target="bookmarks", forbid=["room:locked"]) correctly implements the new locking semantics. The session_id and user_id parameters are intentionally injected by the decorator for potential use in logging or future enhancements—the static analysis warning about unused arguments is a false positive for this decorator pattern.


109-113: LGTM!

Consistent application of @check_lock decorator with the same target and forbid parameters as set_bookmark. The symmetry between set and delete operations is appropriate.

src/zndraw/app/geometry_routes.py (6)

17-17: LGTM!

Import correctly updated to use the new check_lock decorator.


46-49: Consider the fallback behavior for missing keys.

When data.get('key') returns None, the lock target becomes "geometry:unknown". This could cause unintended lock contention if multiple requests with missing keys compete for the same "geometry:unknown" lock. However, since the route handler at line 77 validates that key is required and returns 400 if missing, this fallback should rarely be reached in practice.

The kwargs parameter is unused because the key comes from the request body, not URL parameters—this is expected for POST operations.


52-58: LGTM!

Per-geometry locking with dynamic target extraction is a good design choice, enabling fine-grained concurrency control.


168-173: LGTM!

For DELETE operations, extracting the key from kw['key'] (URL path parameter) is correct. The req parameter is unused because the key isn't in the request body—this is expected.


262-269: LGTM!

Consistent pattern with geometry helpers. Same note applies regarding the "figure:unknown" fallback when key is missing.


310-311: LGTM!

Consistent with delete_geometry pattern—key extracted from URL path parameter.

tests/test_check_lock_decorator.py (5)

1-11: LGTM!

Clear and concise module docstring explaining the behavioral difference from @requires_lock.


35-109: LGTM!

Good coverage of authentication and session validation edge cases. The tests correctly verify:

  • Missing X-Session-ID returns 400
  • Invalid session ID returns 401
  • Operations proceed when no lock exists (FIFO behavior)

161-196: LGTM!

This test correctly validates the core @check_lock behavior: blocking when another session holds the lock. Good use of connect_room fixture to maintain socket connections.


387-489: LGTM!

Excellent coverage of global lock and per-geometry lock semantics:

  • Global lock blocks all operations by other sessions
  • Global lock holder can still operate
  • Per-geometry locks only block that specific geometry key

284-287: This concern is not applicable—hardcoded localhost:6379 is the correct configuration.

CI explicitly configures Redis as a service with port 6379 exposed (see .github/workflows/test.yaml). This is the standard pattern used throughout the test suite, including in the redis_client() fixture in conftest.py, which uses identical values. The hardcoded connection is intentional and will not fail in CI.

Likely an incorrect or invalid review comment.

src/zndraw/app/frame_routes.py (5)

17-29: LGTM!

Import correctly updated. The other imports from route_utils remain unchanged.


333-336: LGTM!

Correct application of @check_lock for frame deletion. The trajectory:meta target ensures frame mutations are coordinated.


469-471: LGTM!

Consistent decorator application for frame append/extend/replace/insert operations.


722-724: LGTM!

Bulk replace correctly protected with the same lock target.


907-911: LGTM!

Partial frame updates correctly protected. All frame mutation endpoints are now consistently using @check_lock(target="trajectory:meta", forbid=["room:locked"]).

src/zndraw/app/room_routes.py (3)

24-29: LGTM!

Import correctly updated to use the new check_lock decorator.


634-643: LGTM!

Correct decorator application for frame index renormalization. The trajectory:meta target and room:locked forbid are appropriate for this operation.


906-918: LGTM!

Good use of session_id to look up the socket ID for skip_sid, preventing the sender from receiving their own update. This is the correct pattern for real-time collaborative updates.

src/zndraw/app/route_utils.py (3)

10-10: LGTM!

The import and type alias are well-structured. The LockTargetType union type clearly expresses the three valid target options (static string, dynamic callable, or None).

Also applies to: 65-65


68-131: LGTM!

The decorator signature and docstring are well-designed. The numpy-style docstring clearly documents the execution order, parameter options, and provides good examples covering static targets, dynamic callables, and forbid-only usage.


190-249: Lock checking logic is well-structured.

The implementation correctly:

  • Checks global lock before target-specific lock (proper precedence)
  • Compares sessionId to allow same-session operations
  • Returns consistent 423 responses with holder information for client-side notifications
  • Properly injects validated session_id and user_id into kwargs

The graceful handling of JSONDecodeError (log and continue) is appropriate—corrupted lock data shouldn't permanently block operations.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread src/zndraw/app/route_utils.py
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 15, 2026

Codecov Report

❌ Patch coverage is 84.53947% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.02%. Comparing base (a1b362b) to head (7f91416).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/zndraw/app/route_utils.py 50.00% 16 Missing ⚠️
src/zndraw/app/lock_routes.py 0.00% 14 Missing ⚠️
src/zndraw/app/events.py 0.00% 10 Missing ⚠️
src/zndraw/zndraw.py 68.18% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #846      +/-   ##
==========================================
- Coverage   80.08%   80.02%   -0.06%     
==========================================
  Files         165      165              
  Lines       19985    20084      +99     
==========================================
+ Hits        16004    16073      +69     
- Misses       3981     4011      +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@PythonFZ PythonFZ merged commit e66be9d into main Jan 15, 2026
6 checks passed
@PythonFZ PythonFZ deleted the feature/frontend-lock-simplification branch January 15, 2026 20:24
@coderabbitai coderabbitai Bot mentioned this pull request Mar 20, 2026
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants