refactor: migrate frame_selection and chat from Socket.IO to REST#835
refactor: migrate frame_selection and chat from Socket.IO to REST#835
Conversation
📝 WalkthroughWalkthroughMoves frame-selection and chat handling from Socket.IO to REST: adds server REST endpoints and broadcasts, client API methods (Python/TS), removes socket handlers, and updates frontend store/hooks/components and tests to use HTTP calls with real-time room broadcasts. Changes
Sequence DiagramsequenceDiagram
participant Frontend as Client (frontend / zndraw)
participant APIClient as API client (JS/Python)
participant Server as REST API
participant Redis
participant Broadcaster as Socket.IO Broadcast
Frontend->>APIClient: createChatMessage / editChatMessage / updateFrameSelection
APIClient->>Server: POST / PATCH / PUT /api/rooms/{room}/... (auth headers + body)
Server->>Redis: validate and persist chat message or frame_selection
Server->>Broadcaster: emit room event (skip caller)
Broadcaster-->>Frontend: realtime update delivered to room participants
Server-->>APIClient: JSON response (success + resource)
APIClient-->>Frontend: return parsed JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/test_*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)tests/test_vis_settings.py (1)
⏰ 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)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/zndraw/app/screenshot_chat_routes.py (2)
22-31: Extract_get_caller_sidto a shared utility module.This helper function is duplicated in
src/zndraw/app/geometry_routes.py(lines 20-29). Consider extracting it to a shared module (e.g.,route_utils.py) to follow DRY principles.♻️ Suggested refactor
Create a shared helper in
route_utils.py:# In src/zndraw/app/route_utils.py def get_caller_sid() -> str | None: """Get socket sid of the caller from their session ID header. Used to exclude the caller from socket broadcasts (skip_sid). """ from flask import current_app, request from .redis_keys import SessionKeys session_id = request.headers.get("X-Session-ID") if not session_id: return None r = current_app.extensions["redis"] return r.get(SessionKeys.session_to_sid(session_id))Then import and use it in both route files.
149-180: Redundant auth error handling after@require_authdecorator.The
@require_authdecorator already enforces authentication and returns an error response if the user is not authenticated. Thetry/except AuthErrorblock on lines 165-168 is redundant becauseget_current_user()will only raiseAuthErrorif authentication fails, which is already handled by the decorator.♻️ Suggested simplification
- try: - user_name = get_current_user() - except AuthError as e: - return {"error": e.message}, e.status_code + user_name = get_current_user()src/zndraw/app/geometry_routes.py (1)
383-414: Consider usingSocketEventsconstant for broadcast event name.Line 408 uses a literal string
"frame_selection:update"for the broadcast event. For consistency with the chat endpoints that useSocketEvents.CHAT_MESSAGE_NEWandSocketEvents.CHAT_MESSAGE_UPDATED, consider adding a corresponding constant toSocketEventsclass.Otherwise, the endpoint implementation is correct:
- Proper validation of indices as non-negative integers
- Redis storage using
RoomKeys.frame_selection()- Broadcasts to room while skipping the caller to prevent UI flicker
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/zndraw/api_manager.pysrc/zndraw/app/events.pysrc/zndraw/app/geometry_routes.pysrc/zndraw/app/screenshot_chat_routes.pysrc/zndraw/zndraw.pytests/test_chat.py
💤 Files with no reviewable changes (1)
- src/zndraw/app/events.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: You MUST NEVER@pytest.mark.xfailor 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. Uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]!
Imports should always be at the top of the file in Python.
Files:
tests/test_chat.pysrc/zndraw/api_manager.pysrc/zndraw/app/screenshot_chat_routes.pysrc/zndraw/zndraw.pysrc/zndraw/app/geometry_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.parametrizeto 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_chat.py
🧬 Code graph analysis (5)
tests/test_chat.py (2)
src/zndraw/zndraw.py (1)
edit_message(1722-1724)tests/conftest.py (1)
server(419-425)
src/zndraw/api_manager.py (2)
src/zndraw/app/geometry_routes.py (1)
update_frame_selection(385-414)src/zndraw/app/screenshot_chat_routes.py (2)
create_chat_message(151-180)edit_chat_message(187-224)
src/zndraw/app/screenshot_chat_routes.py (6)
src/zndraw/auth.py (3)
AuthError(20-34)get_current_user(135-153)require_auth(180-200)tests/conftest.py (1)
server(419-425)src/zndraw/app/chat_utils.py (3)
create_message(8-54)get_message(57-74)update_message(77-106)src/zndraw/app/constants.py (1)
SocketEvents(4-25)src/zndraw/app/redis_keys.py (4)
RoomKeys(242-533)SessionKeys(587-652)session_id(600-602)session_to_sid(623-636)src/zndraw/app/geometry_routes.py (1)
_get_caller_sid(20-29)
src/zndraw/zndraw.py (3)
src/zndraw/api_manager.py (3)
update_frame_selection(1501-1525)create_chat_message(1531-1555)edit_chat_message(1557-1583)src/zndraw/app/geometry_routes.py (1)
update_frame_selection(385-414)src/zndraw/app/screenshot_chat_routes.py (2)
create_chat_message(151-180)edit_chat_message(187-224)
src/zndraw/app/geometry_routes.py (2)
src/zndraw/app/redis_keys.py (6)
RoomKeys(242-533)SessionKeys(587-652)session_id(600-602)session_to_sid(623-636)geometries(308-310)frame_selection(392-405)src/zndraw/app/screenshot_chat_routes.py (1)
_get_caller_sid(22-31)
⏰ 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.12, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
🔇 Additional comments (10)
tests/test_chat.py (2)
3-3: LGTM!The
pytestimport is correctly placed at the top of the file with other imports.
80-84: LGTM!The test correctly validates that unauthorized message edits raise
requests.HTTPErrorwith a 403 status code, properly reflecting the REST API migration.src/zndraw/app/screenshot_chat_routes.py (2)
11-17: LGTM!Imports are properly organized at the top of the file and all are utilized by the new chat message endpoints.
183-224: LGTM!The endpoint correctly validates message ownership before allowing edits, returns appropriate HTTP status codes (404/403), and broadcasts updates to the room. The same optional simplification for the
AuthErrortry/except block applies here as increate_chat_message.src/zndraw/api_manager.py (3)
1501-1525: LGTM!The method follows the established patterns in
APIManager:
- Proper numpy-style docstring with parameters, returns, and raises sections
- Consistent use of
_get_headers(), timeout, andraise_for_status()- Type hints match the server endpoint expectations
1531-1555: LGTM!Consistent implementation following the established API method patterns. The docstring properly documents the return type and potential exceptions.
1557-1583: LGTM!The method correctly uses PATCH for editing, and the docstring properly documents both 404 and 403 error cases that the tests verify.
src/zndraw/zndraw.py (3)
802-803: LGTM!The implementation correctly calls the API first, and only updates the internal state after a successful API response. This ensures consistency between client state and server state.
1718-1720: LGTM!Clean delegation to the API layer. The return type simplification from
dict | Nonetodictis appropriate since failures now raiseHTTPErrorinstead of returningNone.
1722-1724: LGTM!Consistent with the
logmethod migration. The testtest_chat_message_edit_unauthorizedconfirms that 403 errors correctly raiseHTTPError.
- Add PUT /api/rooms/{room}/frame-selection endpoint
- Add POST /api/rooms/{room}/chat/messages endpoint
- Add PATCH /api/rooms/{room}/chat/messages/{id} endpoint
- Add APIManager methods for frame selection and chat
- Update ZnDraw client to use REST instead of Socket.IO
- Remove Socket.IO event handlers (frame_selection:set, chat:message:create, chat:message:edit)
- Add skip_sid to broadcasts to avoid UI flicker for the caller
- Update test to expect HTTPError on 403 instead of response dict
This is part of the migration away from eventlet towards async.
Socket.IO is now used only for broadcasts, not for request/response patterns.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #835 +/- ##
==========================================
+ Coverage 79.93% 79.96% +0.02%
==========================================
Files 161 162 +1
Lines 19745 19769 +24
==========================================
+ Hits 15783 15808 +25
+ Misses 3962 3961 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PythonFZ
force-pushed
the
feat/rest-frame-selection-chat
branch
from
6d47459 to
0754fcd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/zndraw/app/geometry_routes.py:
- Around line 370-401: The update_frame_selection route calls request.get_json()
without a fallback, so set data = request.get_json() or {} to avoid
AttributeError when the body is missing/malformed and validate indices against
that dict; also update the socketio.emit call in update_frame_selection to pass
the caller's sid as skip_sid (retrieve the session/sid the same way other routes
do and forward it into socketio.emit) so the requester doesn't receive the
broadcast and cause UI flicker.
In @src/zndraw/app/screenshot_chat_routes.py:
- Around line 157-161: The socketio.emit call that broadcasts CHAT_MESSAGE_NEW
(SocketEvents.CHAT_MESSAGE_NEW) to the room (to=f"room:{room_id}") should
exclude the originating caller to prevent UI flicker; update the socketio.emit
invocation to pass skip_sid=<caller_sid> (e.g., skip_sid=request.sid when in a
socket context) or extract the caller's sid from the incoming HTTP
header/session and pass that value as skip_sid. Locate the emit in
screenshot_chat_routes.py (the socketio.emit(..., message, to=f"room:{room_id}")
call), obtain the caller session id appropriately for your Flask-SocketIO REST
vs socket context, and add skip_sid=caller_sid to the emit parameters.
- Around line 197-201: The broadcast currently sends the CHAT_MESSAGE_UPDATED
event to the room including the caller, causing duplicate UI updates; update the
socketio.emit call that sends SocketEvents.CHAT_MESSAGE_UPDATED with payload
updated to include skip_sid=current_socket_id (the caller's socket id) so the
emitter skips the originator and avoids flicker—use the existing room identifier
(to=f"room:{room_id}") and pass skip_sid alongside it.
🧹 Nitpick comments (1)
app/src/store.tsx (1)
412-423: Consider adding rollback on API failure for consistency.The optimistic update pattern is good, but unlike
addBookmarkanddeleteBookmarkmethods in this file (lines 774-828),setFrameSelectiondoesn't rollback the local state if the API call fails. This could lead to UI showing a frame selection that doesn't match the server state.♻️ Suggested rollback pattern
setFrameSelection: (frame_selection) => { const roomId = get().roomId; if (!roomId) return; + // Store old value for rollback + const oldFrameSelection = get().frame_selection; + // Optimistic update set({ frame_selection: frame_selection }); // Update via REST API updateFrameSelectionAPI(roomId, frame_selection || []).catch((error) => { console.error("Failed to update frame selection:", error); + // Rollback on error + set({ frame_selection: oldFrameSelection }); }); },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/src/hooks/useChat.tsapp/src/myapi/client.tsapp/src/store.tsxsrc/zndraw/api_manager.pysrc/zndraw/app/constants.pysrc/zndraw/app/events.pysrc/zndraw/app/geometry_routes.pysrc/zndraw/app/screenshot_chat_routes.pysrc/zndraw/zndraw.pytests/test_chat.py
💤 Files with no reviewable changes (1)
- src/zndraw/app/events.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_chat.py
- src/zndraw/api_manager.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: You MUST NEVER@pytest.mark.xfailor 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. Uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]!
Imports should always be at the top of the file in Python.
Files:
src/zndraw/app/constants.pysrc/zndraw/app/screenshot_chat_routes.pysrc/zndraw/app/geometry_routes.pysrc/zndraw/zndraw.py
🧬 Code graph analysis (3)
app/src/hooks/useChat.ts (1)
app/src/myapi/client.ts (2)
createChatMessage(793-801)editChatMessage(808-818)
src/zndraw/app/screenshot_chat_routes.py (3)
src/zndraw/app/chat_utils.py (3)
create_message(8-54)get_message(57-74)update_message(77-106)src/zndraw/app/constants.py (1)
SocketEvents(4-26)src/zndraw/app/redis_keys.py (1)
RoomKeys(242-533)
app/src/store.tsx (2)
src/zndraw/zndraw.py (2)
frame_selection(791-793)frame_selection(796-803)src/zndraw/app/redis_keys.py (1)
frame_selection(392-405)
⏰ 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.12, ubuntu-latest)
- GitHub Check: pytest (3.13, ubuntu-latest)
🔇 Additional comments (8)
src/zndraw/zndraw.py (2)
796-803: LGTM! Clean migration to REST API.The frame selection setter correctly validates input, calls the API, and updates internal state. The change from Socket.IO to REST is well-implemented.
1718-1724: LGTM! Chat methods successfully migrated to REST API.The chat message creation and editing methods now correctly delegate to the API manager, maintaining the same interface while using REST instead of Socket.IO.
src/zndraw/app/constants.py (1)
26-26: LGTM! New socket event constant added.The
FRAME_SELECTION_UPDATEconstant follows the existing naming convention and supports the new REST-based frame selection flow.app/src/hooks/useChat.ts (2)
34-42: LGTM! Clean migration to REST API for chat messages.The
useSendMessagehook correctly calls the newcreateChatMessageAPI and returns the message. The comment explaining why query invalidation isn't needed (socket events handle cache updates) is helpful.
44-59: LGTM! Message editing successfully migrated to REST API.The
useEditMessagehook properly delegates to the neweditChatMessageAPI client method, maintaining the same mutation pattern while using REST instead of Socket.IO.app/src/myapi/client.ts (3)
481-489: LGTM!The
updateFrameSelectionmethod is correctly implemented with appropriate HTTP method and typing.
788-801: LGTM!The chat message creation interface and method are properly typed and correctly implemented.
803-818: LGTM!The chat message editing interface and method are properly typed with the correct HTTP method (PATCH) for partial updates.
| @geometries.route("/api/rooms/<string:room_id>/frame-selection", methods=["PUT"]) | ||
| @require_auth | ||
| def update_frame_selection(room_id: str): | ||
| """Update frame selection for the room. | ||
|
|
||
| Body: {"indices": [0, 1, 5, 10]} | ||
|
|
||
| Broadcasts frame_selection:update to other clients. | ||
| """ | ||
| r = current_app.extensions["redis"] | ||
| keys = RoomKeys(room_id) | ||
| data = request.get_json() | ||
|
|
||
| indices = data.get("indices", []) | ||
| if not isinstance(indices, list): | ||
| return {"error": "indices must be a list"}, 400 | ||
|
|
||
| if not all(isinstance(idx, int) and idx >= 0 for idx in indices): | ||
| return {"error": "All indices must be non-negative integers"}, 400 | ||
|
|
||
| # Store frame selection | ||
| r.set(keys.frame_selection(), json.dumps(indices)) | ||
|
|
||
| # Broadcast to room - update appearing confirms it was saved | ||
| socketio.emit( | ||
| SocketEvents.FRAME_SELECTION_UPDATE, | ||
| {"indices": indices}, | ||
| to=f"room:{room_id}", | ||
| ) | ||
|
|
||
| return {"success": True}, 200 | ||
|
|
There was a problem hiding this comment.
Fix potential AttributeError on malformed request body.
Line 381 uses request.get_json() without a default value. If the request body is missing or malformed JSON, data will be None, causing line 383's data.get("indices", []) to raise an AttributeError.
Other routes in this file (lines 67, 244, 428) use the safer pattern: request.get_json() or {}.
🐛 Proposed fix
r = current_app.extensions["redis"]
keys = RoomKeys(room_id)
- data = request.get_json()
+ data = request.get_json() or {}
indices = data.get("indices", [])Consider adding skip_sid parameter to socketio.emit.
According to the PR objectives, skip_sid should be added to broadcasts to "avoid UI flicker for the caller." The socketio.emit call on lines 394-398 doesn't include a skip_sid parameter. This means the client that made the request will receive the broadcast event, potentially causing UI flicker if they've already optimistically updated their state.
Note: This would require passing the session ID through to this function, similar to how other routes handle it. You may need to add authentication/session handling to this endpoint.
🤖 Prompt for AI Agents
In @src/zndraw/app/geometry_routes.py around lines 370 - 401, The
update_frame_selection route calls request.get_json() without a fallback, so set
data = request.get_json() or {} to avoid AttributeError when the body is
missing/malformed and validate indices against that dict; also update the
socketio.emit call in update_frame_selection to pass the caller's sid as
skip_sid (retrieve the session/sid the same way other routes do and forward it
into socketio.emit) so the requester doesn't receive the broadcast and cause UI
flicker.
| socketio.emit( | ||
| SocketEvents.CHAT_MESSAGE_NEW, | ||
| message, | ||
| to=f"room:{room_id}", | ||
| ) |
There was a problem hiding this comment.
Add skip_sid to broadcast to prevent UI flicker for the caller.
Per the PR objectives, broadcasts should skip the caller's socket ID to avoid UI flicker. The broadcast should exclude the session that initiated the request so the caller doesn't receive a duplicate update via Socket.IO.
🔧 Suggested fix
You'll need to obtain the session ID from the request context. In Flask-SocketIO, you can access it via request.sid if called from a socket context, or you may need to pass the session ID via header and look it up in the room's sessions.
+ # Get session ID to skip in broadcast (avoid UI flicker for caller)
+ # Session ID should be available from request headers or socket context
+ session_id = request.headers.get("X-Session-ID")
+
# Broadcast to room - message appearing confirms it was saved
socketio.emit(
SocketEvents.CHAT_MESSAGE_NEW,
message,
to=f"room:{room_id}",
+ skip_sid=session_id,
)Note: Verify the correct way to obtain the session/socket ID in your Flask-SocketIO setup for REST endpoints.
🤖 Prompt for AI Agents
In @src/zndraw/app/screenshot_chat_routes.py around lines 157 - 161, The
socketio.emit call that broadcasts CHAT_MESSAGE_NEW
(SocketEvents.CHAT_MESSAGE_NEW) to the room (to=f"room:{room_id}") should
exclude the originating caller to prevent UI flicker; update the socketio.emit
invocation to pass skip_sid=<caller_sid> (e.g., skip_sid=request.sid when in a
socket context) or extract the caller's sid from the incoming HTTP
header/session and pass that value as skip_sid. Locate the emit in
screenshot_chat_routes.py (the socketio.emit(..., message, to=f"room:{room_id}")
call), obtain the caller session id appropriately for your Flask-SocketIO REST
vs socket context, and add skip_sid=caller_sid to the emit parameters.
| socketio.emit( | ||
| SocketEvents.CHAT_MESSAGE_UPDATED, | ||
| updated, | ||
| to=f"room:{room_id}", | ||
| ) |
There was a problem hiding this comment.
Add skip_sid to broadcast to prevent UI flicker for the caller.
Same issue as in the create endpoint—the broadcast should skip the caller's socket ID to avoid duplicate updates on the client that initiated the edit.
🔧 Suggested fix
+ # Get session ID to skip in broadcast (avoid UI flicker for caller)
+ session_id = request.headers.get("X-Session-ID")
+
# Broadcast to room - message appearing confirms it was saved
socketio.emit(
SocketEvents.CHAT_MESSAGE_UPDATED,
updated,
to=f"room:{room_id}",
+ skip_sid=session_id,
)🤖 Prompt for AI Agents
In @src/zndraw/app/screenshot_chat_routes.py around lines 197 - 201, The
broadcast currently sends the CHAT_MESSAGE_UPDATED event to the room including
the caller, causing duplicate UI updates; update the socketio.emit call that
sends SocketEvents.CHAT_MESSAGE_UPDATED with payload updated to include
skip_sid=current_socket_id (the caller's socket id) so the emitter skips the
originator and avoids flicker—use the existing room identifier
(to=f"room:{room_id}") and pass skip_sid alongside it.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/FigureWindow.tsx (1)
452-457: Fix conditional hook execution (Rules of Hooks violation) caused by early return.The guard
if (!windowInstance) return null;at lines 455-456 executes before multiple hooks (useMemo,useCallback,useEffect) are declared. On renders wherewindowInstanceis falsy, zero hooks are called; when truthy, all hooks execute. This violates React's Rules of Hooks, which require the same number and order of hooks on every render.Move the early return to after all hook declarations but before the rendering JSX (around where the
<WindowTemplate>component is returned). This ensures all hooks are always called in the same order regardless of thewindowInstancevalue.
🤖 Fix all issues with AI agents
In @app/src/store.tsx:
- Around line 413-429: updateFrameSelection currently does an optimistic set but
silently keeps the change on API failure; capture the previous selection before
calling set({ frame_selection }) then call updateFrameSelectionAPI(roomId,
frame_selection || []) and in the .catch examine error.response?.status (not
error.status); on any error invoke the app's user-feedback/snackbar mechanism to
show a failure message, and if error.response?.status === 403 revert by calling
set({ frame_selection: previousSelection }) to roll back the optimistic update.
Use the existing symbols updateFrameSelection, updateFrameSelectionAPI,
get().roomId and set() to locate and implement these changes.
🧹 Nitpick comments (2)
app/src/components/FigureWindow.tsx (2)
568-633: Dedupe selected frame indices before sending to REST-backed action.
With REST in the loop, avoid sending duplicates (Plotly selections can include repeated points).Proposed change
// Apply frame selection - replace with newly selected frames if (selectedFrames.length > 0) { - updateFrameSelection(selectedFrames); + updateFrameSelection(Array.from(new Set(selectedFrames))); }
635-638: Avoid redundant “clear selection” calls (optional).
Ifframe_selectionis already empty/null, this can generate no-op REST calls. Consider guarding (or accepting as-is if cheap).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/components/FigureWindow.tsxapp/src/store.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/components/FigureWindow.tsx (1)
app/src/myapi/client.ts (1)
updateFrameSelection(481-489)
🪛 Biome (2.1.2)
app/src/components/FigureWindow.tsx
[error] 635-635: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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)
app/src/components/FigureWindow.tsx (1)
446-448: Store action rename usage looks consistent (updateFrameSelection).
Good alignment with the new optimistic+REST-backed store action.app/src/store.tsx (2)
4-16: Import aliasing is clear; verify thrown error shape fromupdateFrameSelectionAPI.
Store-side handling likely depends on whether the client throws anHTTPError(as tests now expect for 403).
147-149: Public store API split (setFrameSelectionvsupdateFrameSelection) is correctly implemented.Socket-driven updates in
useSocketManager.tsproperly usesetFrameSelection(at lines 159 and 280), while user actions inFigureWindow.tsxproperly useupdateFrameSelection(at lines 624 and 636). The semantic split is enforced across call sites.
| // For socket broadcasts - local state only, no API call | ||
| setFrameSelection: (frame_selection) => | ||
| set({ frame_selection: frame_selection }), | ||
|
|
||
| // For user-initiated changes - local state + API call | ||
| updateFrameSelection: (frame_selection) => { | ||
| const roomId = get().roomId; | ||
| if (!roomId) return; | ||
|
|
||
| // Optimistic update | ||
| set({ frame_selection: frame_selection }); | ||
|
|
||
| // Update via REST API | ||
| updateFrameSelectionAPI(roomId, frame_selection || []).catch((error) => { | ||
| console.error("Failed to update frame selection:", error); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get context around the mentioned lines in store.tsx
fd -t f "store.tsx" --path "*/src/*" | head -5Repository: zincware/ZnDraw
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Search for the store.tsx file
find . -name "store.tsx" -type fRepository: zincware/ZnDraw
Length of output: 78
🏁 Script executed:
#!/bin/bash
# Examine the store.tsx file around lines 413-429
if [ -f "app/src/store.tsx" ]; then
wc -l app/src/store.tsx
fiRepository: zincware/ZnDraw
Length of output: 81
🏁 Script executed:
#!/bin/bash
# Look for updateFrameSelectionAPI definition
rg "updateFrameSelectionAPI" --type ts --type tsx -B 3 -A 10Repository: zincware/ZnDraw
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Check if showSnackbar exists in the store
rg "showSnackbar" --type ts --type tsx -B 2 -A 2Repository: zincware/ZnDraw
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Read lines 413-429 with context from store.tsx
sed -n '400,450p' app/src/store.tsxRepository: zincware/ZnDraw
Length of output: 1727
🏁 Script executed:
#!/bin/bash
# Search for updateFrameSelectionAPI
rg "updateFrameSelectionAPI" -t js -B 3 -A 15Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Search for showSnackbar
rg "showSnackbar" -t js -B 2 -A 5Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look for other API error handling in store
rg "catch.*error" app/src/store.tsx -B 2 -A 5Repository: zincware/ZnDraw
Length of output: 3226
🏁 Script executed:
#!/bin/bash
# Check if error.status is used anywhere in error handling
rg "error.*status|error\.status" -t js -B 2 -A 2Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look at updateFrameSelectionAPI and similar API functions
rg "API\(" -t js -B 2 -A 8 | head -80Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/antml:thinking
Need to search for the API implementations to see how errors are structured
</thinking>
bash
# Find API files
find . -name "*api*" -type f | grep -E "\.(ts|tsx|js)$"Repository: zincware/ZnDraw
Length of output: 206
🏁 Script executed:
#!/bin/bash
# Find API files
find . -name "*api*" -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" \)Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Search for updateFrameSelectionAPI definition
rg "updateFrameSelectionAPI\s*=" -A 20Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look for error status handling in API calls
rg "status\s*===" -A 3 -B 3Repository: zincware/ZnDraw
Length of output: 4028
🏁 Script executed:
#!/bin/bash
# Check the store.tsx file for showSnackbar method definition
rg "showSnackbar\s*:" app/src/store.tsx -B 2 -A 10Repository: zincware/ZnDraw
Length of output: 1056
🏁 Script executed:
#!/bin/bash
# Find API implementation files
find . -path ./node_modules -prune -o -name "*.ts" -o -name "*.tsx" | grep -i api | head -20Repository: zincware/ZnDraw
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Search for where updateFrameSelectionAPI is imported from
rg "import.*updateFrameSelectionAPI|from.*updateFrameSelectionAPI" -A 2 -B 2Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look for API client implementation
find . -path ./node_modules -prune -o -type f -name "*.ts" -print | xargs grep -l "updateFrameSelectionAPI" 2>/dev/nullRepository: zincware/ZnDraw
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Examine the client.ts file for updateFrameSelectionAPI
rg "updateFrameSelectionAPI" app/src/myapi/client.ts -B 5 -A 15Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look at the imports and error handling in client.ts
head -100 app/src/myapi/client.tsRepository: zincware/ZnDraw
Length of output: 3100
🏁 Script executed:
#!/bin/bash
# Check how errors are structured from API calls
grep -A 10 "catch.*error" app/src/myapi/client.ts | head -40Repository: zincware/ZnDraw
Length of output: 828
🏁 Script executed:
#!/bin/bash
# Search for updateFrameSelectionAPI in the entire codebase
rg "updateFrameSelectionAPI" --type js -B 2 -A 20Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/an/bash
# Search in all JS/TS files for the function definition
find . -name "*.ts" -o -name "*.tsx" | xargs grep -l "updateFrameSelectionAPI" 2>/dev/nullRepository: zincware/ZnDraw
Length of output: 78
🏁 Script executed:
#!/bin/bash
# Look at more of client.ts to find updateFrameSelectionAPI
wc -l app/src/myapi/client.ts && echo "---" && tail -400 app/src/myapi/client.ts | head -200Repository: zincware/ZnDraw
Length of output: 5199
🏁 Script executed:
#!/bin/bash
# Search for the exact line where updateFrameSelectionAPI is exported/defined
rg "export.*updateFrameSelectionAPI|updateFrameSelectionAPI\s*=" app/src/myapi/client.ts -B 3 -A 15Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Get the middle section of client.ts that likely contains updateFrameSelectionAPI
sed -n '700,900p' app/src/myapi/client.tsRepository: zincware/ZnDraw
Length of output: 4756
🏁 Script executed:
#!/bin/bash
# Look for selection-related API functions in client.ts
rg "Selection|selection" app/src/myapi/client.ts -B 2 -A 10Repository: zincware/ZnDraw
Length of output: 3628
🏁 Script executed:
#!/bin/bash
# Check what is imported at the top of store.tsx
head -150 app/src/store.tsx | grep -E "import|from"Repository: zincware/ZnDraw
Length of output: 405
🏁 Script executed:
#!/bin/bash
# Search for where updateFrameSelectionAPI comes from in store.tsx
rg "updateFrameSelectionAPI" app/src/store.tsx -B 30 | head -80Repository: zincware/ZnDraw
Length of output: 1284
🏁 Script executed:
#!/bin/bash
# Look for test files related to frame selection
find . -name "*.test.*" -o -name "*.spec.*" | xargs grep -l "frameSelection\|frame.selection" 2>/dev/null | head -5Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check if there are any 403 status codes mentioned in tests
rg "403" --type js -B 3 -A 3 | grep -A 5 -B 5 "frame\|selection"Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look at other API error handling patterns in store.tsx to see if rollbacks are common
sed -n '200,600p' app/src/store.tsx | rg "catch.*error" -A 10Repository: zincware/ZnDraw
Length of output: 1713
🏁 Script executed:
#!/bin/bash
# Verify the axios error structure by looking at how errors are checked in the codebase
rg "error\?.response\?.status|error\.response\.status" --type js -B 3 -A 3 | head -60Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look at the setBookmarkAPI and deleteBookmarkAPI to see rollback pattern
rg "setBookmarkAPI|deleteBookmarkAPI" app/src/store.tsx -A 25 | head -100Repository: zincware/ZnDraw
Length of output: 2363
🏁 Script executed:
#!/bin/bash
# Check if similar methods have error handling patterns we should follow
sed -n '600,900p' app/src/store.tsx | grep -A 15 "catch.*error"Repository: zincware/ZnDraw
Length of output: 2466
🏁 Script executed:
#!/bin/bash
# Check if error.status (direct property) vs error.response.status is used in axios error handling
rg "error\.\w+\s*===" app/src/ -B 2 -A 2 | head -40Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Verify axios error structure in existing code
rg "catch.*error.*\{" app/src/hooks/ -A 15 | head -60Repository: zincware/ZnDraw
Length of output: 3485
🏁 Script executed:
#!/bin/bash
# Look at a complete bookmark method with rollback to compare
sed -n '330,380p' app/src/store.tsxRepository: zincware/ZnDraw
Length of output: 1861
🏁 Script executed:
#!/bin/bash
# Double-check the axios error structure used throughout the codebase
rg "error\.response\?.status" app/src/ -B 2 -A 2 | head -40Repository: zincware/ZnDraw
Length of output: 1163
🏁 Script executed:
#!/bin/bash
# Look for any usage of error.status (without response)
rg "error\?.status\s*===" app/src/ -B 2 -A 2Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Verify the complete updateFrameSelection implementation one more time
sed -n '415,430p' app/src/store.tsxRepository: zincware/ZnDraw
Length of output: 543
Add error feedback and rollback handling for frame selection updates.
The current implementation silently fails when the API rejects (e.g., 403), keeping optimistic state even though the server rejected the change. Additionally, there's no user feedback on failure.
Consider: (1) always apply the local update to keep UI responsive, (2) show a snackbar on failure, (3) optionally rollback on permission errors (403). Note: use error.response?.status (not error.status) to check axios error codes, as shown elsewhere in the codebase.
Proposed change
// For user-initiated changes - local state + API call
updateFrameSelection: (frame_selection) => {
- const roomId = get().roomId;
- if (!roomId) return;
-
- // Optimistic update
- set({ frame_selection: frame_selection });
+ const prev = get().frame_selection;
+ const roomId = get().roomId;
+
+ // Always update locally (keeps UI responsive even in edge init states)
+ set({ frame_selection });
+
+ // If we can't persist yet, stop here
+ if (!roomId) return;
// Update via REST API
updateFrameSelectionAPI(roomId, frame_selection || []).catch((error) => {
console.error("Failed to update frame selection:", error);
+ // Check error.response?.status (axios error structure used throughout codebase)
+ if ((error as any)?.response?.status === 403) {
+ set({ frame_selection: prev });
+ }
+ get().showSnackbar("Failed to update frame selection", "error");
});
},🤖 Prompt for AI Agents
In @app/src/store.tsx around lines 413 - 429, updateFrameSelection currently
does an optimistic set but silently keeps the change on API failure; capture the
previous selection before calling set({ frame_selection }) then call
updateFrameSelectionAPI(roomId, frame_selection || []) and in the .catch examine
error.response?.status (not error.status); on any error invoke the app's
user-feedback/snackbar mechanism to show a failure message, and if
error.response?.status === 403 revert by calling set({ frame_selection:
previousSelection }) to roll back the optimistic update. Use the existing
symbols updateFrameSelection, updateFrameSelectionAPI, get().roomId and set() to
locate and implement these changes.
- Add null safety for request.get_json() in geometry_routes.py - Fix Rules of Hooks violation in FigureWindow.tsx (move early return after hooks) - Dedupe selected frame indices before sending to REST API - Guard redundant clear selection calls when already empty Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
Socket.IO is now used only for broadcasts, not for request/response patterns.
Summary by CodeRabbit
New Features
Improvements
Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.