Feat/session reconnection #182
Conversation
WalkthroughSession-scoped Socket.IO rooms added and used for broadcasting; ChatNamespace gains connect/join/disconnect handlers and requires/joins a session_id for chat events. Message emitters now attempt real-time emission before persisting messages. Session exposes a new public update(**kwargs) -> bool to persist session attributes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant NS as ChatNamespace (Socket.IO)
participant H as ChatHandler
participant S as Session
participant IO as Socket.IO Server
participant DB as DB
Note over C,NS: Client sends chat with session_id
C->>NS: on_chat({ session_id, text, ... })
NS->>IO: join_room(session_id)
NS->>H: handle(message)
H->>S: publish/update(message)
S->>IO: emit to room=session_id (namespace "/chat")
S->>DB: add_or_update_msg_to_conv(...)
IO-->>C: room-scoped message/event delivered
alt Missing session_id
NS-->>C: emit error "session_id required"
end
sequenceDiagram
participant C as Client
participant NS as ChatNamespace
participant IO as Socket.IO Server
C->>NS: on_join_session({ session_id })
NS->>IO: join_room(session_id)
NS-->>C: emit joined confirmation (room=session_id)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/director/core/session.py (1)
403-411: emit_event should target the session room to align with session-scoped deliveryCurrently emits to the namespace only; clients on reconnection may miss events. Pass room=self.session_id.
Apply this diff:
- def emit_event(self, event: BaseEvent, namespace="/chat"): + def emit_event(self, event: BaseEvent, namespace="/chat"): """Emits a structured WebSocket event to notify all clients about updates.""" event_payload = event.model_dump() try: - emit("event", event_payload, namespace=namespace) - except Exception: - pass + emit("event", event_payload, room=self.session_id, namespace=namespace) + except RuntimeError as e: + logger.warning( + "Error emitting event to session %s: %s", + self.session_id, + e, + exc_info=True, + )
🧹 Nitpick comments (2)
backend/director/entrypoint/api/socket_io.py (2)
11-14: Replace prints with app.logger and remove extraneous f prefix (F541)Use Flask’s logger for consistency and to satisfy Ruff.
Apply this diff:
- def on_connect(self): - """Handle client connection""" - print(f"Client connected to chat namespace") + def on_connect(self): + """Handle client connection""" + app.logger.info("Client connected to chat namespace")
38-40: Replace prints with app.logger and remove extraneous f prefix (F541)Use Flask logger.
Apply this diff:
- def on_disconnect(self): - """Handle client disconnection""" - print(f"Client disconnected from chat namespace") + def on_disconnect(self): + """Handle client disconnection""" + app.logger.info("Client disconnected from chat namespace")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/director/core/session.py(2 hunks)backend/director/entrypoint/api/socket_io.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/director/entrypoint/api/socket_io.py (2)
backend/director/db/__init__.py (1)
load_db(12-19)backend/director/handler.py (2)
ChatHandler(43-124)chat(100-124)
backend/director/core/session.py (2)
backend/director/db/sqlite/db.py (1)
add_or_update_msg_to_conv(101-152)backend/director/db/base.py (1)
add_or_update_msg_to_conv(25-27)
🪛 Ruff (0.13.1)
backend/director/entrypoint/api/socket_io.py
13-13: f-string without any placeholders
Remove extraneous f prefix
(F541)
40-40: f-string without any placeholders
Remove extraneous f prefix
(F541)
backend/director/core/session.py
231-231: Do not catch blind exception: Exception
(BLE001)
232-232: Use explicit conversion flag
Replace with conversion flag
(RUF010)
244-244: Do not catch blind exception: Exception
(BLE001)
245-245: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (4)
backend/director/core/session.py (3)
222-226: Docstring update reads wellStatus updates now clearly mention session-room broadcast. No issues.
235-237: Docstring OK; DB-after-emit sequencing is acceptableSequence keeps persistence even if an emit fails. Fine as-is.
399-402: Confirm return type contract for update()Method is annotated to return bool. Ensure db.update_session(...) indeed returns bool; otherwise adjust the annotation/docstring.
backend/director/entrypoint/api/socket_io.py (1)
4-4: Import changes look goodUsing Namespace, join_room, emit is appropriate for room-scoped messaging.
| """Push real-time update to session room.""" | ||
| try: | ||
| self._publish() | ||
| except Exception as e: | ||
| print(f"Error in emitting message: {str(e)}") | ||
| print(f"Error in emitting update to session {self.session_id}: {str(e)}") | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Don’t swallow exceptions; replace print with proper logging and explicit conversion flag
Use logger.exception (or at least logger.warning with exc_info). Also prefer {e!s} over str(e) to satisfy RUF010. Avoid broad Exception if possible.
Apply this diff:
- def push_update(self):
- """Push real-time update to session room."""
- try:
- self._publish()
- except Exception as e:
- print(f"Error in emitting update to session {self.session_id}: {str(e)}")
+ def push_update(self):
+ """Push real-time update to session room."""
+ try:
+ self._publish()
+ except RuntimeError as e:
+ logger.warning("Emit failed outside request context for session %s: %s", self.session_id, e, exc_info=True)Add the following once at the top of the file to support logging (outside the changed hunk):
import logging
logger = logging.getLogger(__name__)🧰 Tools
🪛 Ruff (0.13.1)
231-231: Do not catch blind exception: Exception
(BLE001)
232-232: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In backend/director/core/session.py around lines 228 to 233, the except block
currently prints errors and swallows exceptions; add a module-level logger by
importing logging and creating logger = logging.getLogger(__name__) at the top
of the file, then replace the print with a logger.exception call (or
logger.warning(..., exc_info=True)) and format the message using {e!s} instead
of str(e) to satisfy RUF010; also avoid catching blanket Exception if you
can—catch the specific exceptions that _publish may raise (or re-raise after
logging) so errors aren’t silently swallowed.
| emit("chat", self.model_dump(), | ||
| room=self.session_id, | ||
| namespace="/chat") | ||
| print(f"Emitted message to session room: {self.session_id}") | ||
| except Exception as e: | ||
| print(f"Error in emitting message: {str(e)}") | ||
| self.db.add_or_update_msg_to_conv(**self.model_dump()) | ||
| print(f"Error emitting to session {self.session_id}: {str(e)}") | ||
|
|
||
| self.db.add_or_update_msg_to_conv(**self.model_dump()) |
There was a problem hiding this comment.
🧹 Nitpick
Room-scoped emit is correct; switch prints to logging and avoid blind except
Good: emit(..., room=self.session_id, namespace="/chat"). Replace print with logger and avoid catching broad Exception. Also use explicit f-string conversion flags per RUF010.
Apply this diff:
- try:
- emit("chat", self.model_dump(),
- room=self.session_id,
- namespace="/chat")
- print(f"Emitted message to session room: {self.session_id}")
- except Exception as e:
- print(f"Error emitting to session {self.session_id}: {str(e)}")
+ try:
+ emit(
+ "chat",
+ self.model_dump(),
+ room=self.session_id,
+ namespace="/chat",
+ )
+ logger.info("Emitted message to session room: %s", self.session_id)
+ except RuntimeError as e:
+ logger.warning(
+ "Error emitting to session %s: %s",
+ self.session_id,
+ e,
+ exc_info=True,
+ )📝 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.
| emit("chat", self.model_dump(), | |
| room=self.session_id, | |
| namespace="/chat") | |
| print(f"Emitted message to session room: {self.session_id}") | |
| except Exception as e: | |
| print(f"Error in emitting message: {str(e)}") | |
| self.db.add_or_update_msg_to_conv(**self.model_dump()) | |
| print(f"Error emitting to session {self.session_id}: {str(e)}") | |
| self.db.add_or_update_msg_to_conv(**self.model_dump()) | |
| try: | |
| emit( | |
| "chat", | |
| self.model_dump(), | |
| room=self.session_id, | |
| namespace="/chat", | |
| ) | |
| logger.info("Emitted message to session room: %s", self.session_id) | |
| except RuntimeError as e: | |
| logger.warning( | |
| "Error emitting to session %s: %s", | |
| self.session_id, | |
| e, | |
| exc_info=True, | |
| ) | |
| self.db.add_or_update_msg_to_conv(**self.model_dump()) |
🧰 Tools
🪛 Ruff (0.13.1)
244-244: Do not catch blind exception: Exception
(BLE001)
245-245: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In backend/director/core/session.py around lines 240-247, replace the two
print() calls with the module logger (e.g., logger.info or logger.debug for the
success message, and logger.error or logger.exception for failures), and stop
using a blind "except Exception"; instead catch the specific
socketio/emit-related exceptions you expect (or at minimum re-raise after
logging). When logging the exception include the exception object with an
explicit conversion flag (e.g., {e!r}) or use logger.exception(...,
exc_info=True) to record the traceback, and ensure
db.add_or_update_msg_to_conv(**self.model_dump()) still runs in the appropriate
place (either after successful emit or in a finally if it must always run).
| """Handle chat messages and auto-join session room""" | ||
| session_id = message.get('session_id') | ||
| if not session_id: | ||
| emit('error', {'message': 'session_id is required'}) | ||
| return | ||
|
|
||
| join_room(session_id) | ||
| print(f"Client joined session room {session_id}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Auto-join is correct; prefer logging over print and consider event name for errors
- Good: requiring session_id and joining the room.
- Swap print for app.logger.
- Verify that emitting 'error' as an event name matches client handlers and isn’t conflicting with any reserved handling; consider a namespaced event like 'chat_error' if not already standardized.
Apply this diff for logging:
- if not session_id:
- emit('error', {'message': 'session_id is required'})
+ if not session_id:
+ emit('error', {'message': 'session_id is required'})
return
join_room(session_id)
- print(f"Client joined session room {session_id}")
+ app.logger.info("Client joined session room %s", session_id)Also confirm client expectations:
🌐 Web query:
Is 'error' a reserved or special event name in Socket.IO client handlers that could interfere with custom error messaging? Should we prefer a custom event name like 'chat_error'?
💡 Result:
Short answer: Yes — "error" is a built‑in Socket.IO event (fired on connection/transport errors) and is treated as a special/reserved event name, so using it for your own application-level error messages can conflict or be confusing. Use a custom name such as "chat_error" (or any app‑specific name) for application error payloads. [1][2][3]
Sources:
- Official Socket.IO docs — client/server event list (shows "error" as an event). [1][2]
- Community guidance (StackOverflow) recommending app‑specific event names instead of reusing "error". [3]
Replace print with logger and stop emitting reserved 'error' event — use an app-specific event (e.g. 'chat_error')
- In backend/director/entrypoint/api/socket_io.py replace print(...) with app.logger.info("Client joined session room %s", session_id).
- Do not emit('error', ...); "error" is a built-in Socket.IO event. Emit a custom event (e.g. emit('chat_error', {'message': 'session_id is required'})) and update client handlers to listen for that name.
- Verify no other server/client code expects application-level messages on the generic "error" event and update those callers/handlers accordingly.
🤖 Prompt for AI Agents
In backend/director/entrypoint/api/socket_io.py around lines 16 to 24, replace
the print statement with app.logger.info("Client joined session room %s",
session_id) and stop emitting the Socket.IO reserved "error" event; instead emit
a custom application event such as emit('chat_error', {'message': 'session_id is
required'}) when session_id is missing. Update any client/server handlers that
expect application-level errors on "error" to listen for the new event name, and
scan the codebase to ensure no other components rely on application messages
using the generic "error" event before deploying.
| def on_join_session(self, data): | ||
| """Explicitly join a session room (for reconnection)""" | ||
| session_id = data.get('session_id') | ||
| if session_id: | ||
| join_room(session_id) | ||
| emit('session_joined', {'session_id': session_id}) | ||
| print(f"Client explicitly joined session room {session_id}") | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Add else-branch for missing session_id and replace print with logging
If session_id is missing, emit an error back to the client. Use app.logger instead of print.
Apply this diff:
def on_join_session(self, data):
"""Explicitly join a session room (for reconnection)"""
session_id = data.get('session_id')
- if session_id:
- join_room(session_id)
- emit('session_joined', {'session_id': session_id})
- print(f"Client explicitly joined session room {session_id}")
+ if session_id:
+ join_room(session_id)
+ emit('session_joined', {'session_id': session_id})
+ app.logger.info("Client explicitly joined session room %s", session_id)
+ else:
+ emit('error', {'message': 'session_id is required to join a session'})📝 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 on_join_session(self, data): | |
| """Explicitly join a session room (for reconnection)""" | |
| session_id = data.get('session_id') | |
| if session_id: | |
| join_room(session_id) | |
| emit('session_joined', {'session_id': session_id}) | |
| print(f"Client explicitly joined session room {session_id}") | |
| def on_join_session(self, data): | |
| """Explicitly join a session room (for reconnection)""" | |
| session_id = data.get('session_id') | |
| if session_id: | |
| join_room(session_id) | |
| emit('session_joined', {'session_id': session_id}) | |
| app.logger.info("Client explicitly joined session room %s", session_id) | |
| else: | |
| emit('error', {'message': 'session_id is required to join a session'}) |
🤖 Prompt for AI Agents
In backend/director/entrypoint/api/socket_io.py around lines 30 to 37, the
on_join_session handler lacks an else-branch to handle a missing session_id and
uses print for logging; modify the function to emit an error event back to the
client (e.g., emit('error', {'message': 'session_id required'})) when session_id
is falsy and replace the print statement with app.logger (use app.logger.info or
app.logger.error as appropriate) to log the join action and error; ensure you
import or reference the Flask app context correctly if needed so app.logger is
available.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/director/core/session.py (1)
211-219: Replace prints and blind except with logging; decide on emit vs. persist ordering
- Avoid print and broad Exception; use module logger and catch RuntimeError from Flask‑SocketIO when outside request context. Also fixes RUF010.
- Input vs. Output ordering is inconsistent: InputMessage persists before emitting; OutputMessage emits before persisting. Pick one policy (persist‑then‑emit for durability, or emit‑then‑persist for lower latency) and apply consistently.
Apply within this hunk:
- self.db.add_or_update_msg_to_conv(**self.model_dump(exclude={"db"})) - try: - emit("chat", self.model_dump(exclude={"db"}), - room=self.session_id, - namespace="/chat") - print(f"Broadcasted input message to session room: {self.session_id}") - except Exception as e: - print(f"Error broadcasting input message to session {self.session_id}: {str(e)}") + self.db.add_or_update_msg_to_conv(**self.model_dump(exclude={"db"})) + try: + emit( + "chat", + self.model_dump(exclude={"db"}), + room=self.session_id, + namespace="/chat", + ) + logger.info("Broadcasted input message to session room: %s", self.session_id) + except RuntimeError as e: + logger.warning( + "Error broadcasting input message to session %s: %s", + self.session_id, + e, + exc_info=True, + )Add once at the top of the file:
import logging logger = logging.getLogger(__name__)Would you like me to align both Input and Output flows to the same ordering? If yes, which policy do you prefer (persist‑then‑emit vs emit‑then‑persist)?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/director/core/session.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/director/core/session.py (2)
backend/director/db/sqlite/db.py (1)
add_or_update_msg_to_conv(101-152)backend/director/db/base.py (1)
add_or_update_msg_to_conv(25-27)
🪛 Ruff (0.13.1)
backend/director/core/session.py
218-218: Do not catch blind exception: Exception
(BLE001)
219-219: Use explicit conversion flag
Replace with conversion flag
(RUF010)
238-238: Do not catch blind exception: Exception
(BLE001)
239-239: Use explicit conversion flag
Replace with conversion flag
(RUF010)
251-251: Do not catch blind exception: Exception
(BLE001)
252-252: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (5)
backend/director/core/session.py (5)
229-233: LGTM on the clarificationDocstring accurately reflects the broadcast behavior of update_status.
234-240: Don’t swallow exceptions; replace print with logger and narrow the exceptUse logger and catch RuntimeError; keep traceback.
def push_update(self): - """Push real-time update to session room.""" - try: - self._publish() - except Exception as e: - print(f"Error in emitting update to session {self.session_id}: {str(e)}") + """Push real-time update to session room.""" + try: + self._publish() + except RuntimeError as e: + logger.warning( + "Emit failed outside request context for session %s: %s", + self.session_id, + e, + exc_info=True, + )
241-244: LGTM on publish docstringMatches the method’s behavior.
245-254: Use logger instead of prints; avoid blind except; ensure DB persist in finally
- Replace prints with logger and catch RuntimeError to satisfy BLE001/RUF010.
- Wrap DB persistence in finally to guarantee execution even if emit raises.
- def _publish(self): - try: - emit("chat", self.model_dump(), - room=self.session_id, - namespace="/chat") - print(f"Emitted message to session room: {self.session_id}") - except Exception as e: - print(f"Error emitting to session {self.session_id}: {str(e)}") - - self.db.add_or_update_msg_to_conv(**self.model_dump()) + def _publish(self): + payload = self.model_dump() + try: + emit( + "chat", + payload, + room=self.session_id, + namespace="/chat", + ) + logger.info("Emitted message to session room: %s", self.session_id) + except RuntimeError as e: + logger.warning( + "Error emitting to session %s: %s", + self.session_id, + e, + exc_info=True, + ) + finally: + try: + self.db.add_or_update_msg_to_conv(**payload) + except Exception: + logger.exception( + "Failed to persist message %s for session %s", + self.msg_id, + self.session_id, + )Note: keep the logger import and instance at module scope as suggested earlier.
406-409: Confirm BaseDB declares update_session or narrow the db typebackend/director/core/session.py (lines 406–409) calls self.db.update_session — if BaseDB doesn't declare update_session this will raise AttributeError at runtime and breaks static typing.
- Either add update_session(...) to the BaseDB ABC/Protocol with the correct signature; or
- Narrow the type of self.db (or the field/parameter) to a concrete class or Protocol that exposes update_session.
1 participant
Current Scenario
request.sidProblem
If the user reloads the page, leaves the page or checks the same session on a different device they will not be able to see live updates
Proposed Solution
session_idwe preserve in oursessionstableroomwhich corresponds to thesession_idSummary by CodeRabbit
New Features
Refactor