-
Notifications
You must be signed in to change notification settings - Fork 195
Feat/session reconnection #182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -220,28 +220,31 @@ class OutputMessage(BaseMessage): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status: MsgStatus = MsgStatus.progress | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def update_status(self, status: MsgStatus): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Update the status of the message and publish the message to the socket. for loading state.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Update the status and broadcast to session room.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.status = status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._publish() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def push_update(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Publish the message to the socket.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """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)}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def publish(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Store the message in the database. for conversation history and publish the message to the socket.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Store in database and broadcast final result to session room.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._publish() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _publish(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| emit("chat", self.model_dump(), namespace="/chat") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+247
to
+254
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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
Suggested change
🧰 Tools🪛 Ruff (0.13.1)244-244: Do not catch blind exception: (BLE001) 245-245: Use explicit conversion flag Replace with conversion flag (RUF010) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def format_user_message(message: dict) -> dict: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| message_content = message.get("content") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -393,6 +396,10 @@ def delete(self): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Delete the session from the database.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return self.db.delete_session(self.session_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def update(self, **kwargs) -> bool: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Update the session in the database.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return self.db.update_session(self.session_id, **kwargs) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def emit_event(self, event: BaseEvent, namespace="/chat"): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Emits a structured WebSocket event to notify all clients about updates.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,18 +1,40 @@ | ||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| from flask import current_app as app | ||||||||||||||||||||||||||||||||||||
| from flask_socketio import Namespace | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| from flask_socketio import Namespace, join_room, emit | ||||||||||||||||||||||||||||||||||||
| from director.db import load_db | ||||||||||||||||||||||||||||||||||||
| from director.handler import ChatHandler | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| class ChatNamespace(Namespace): | ||||||||||||||||||||||||||||||||||||
| """Chat namespace for socket.io""" | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| """Chat namespace for socket.io with session-based rooms""" | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| def on_connect(self): | ||||||||||||||||||||||||||||||||||||
| """Handle client connection""" | ||||||||||||||||||||||||||||||||||||
| print(f"Client connected to chat namespace") | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| def on_chat(self, message): | ||||||||||||||||||||||||||||||||||||
| """Handle chat messages""" | ||||||||||||||||||||||||||||||||||||
| """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}") | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
Comment on lines
+16
to
+24
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion 🧩 Analysis chainAuto-join is correct; prefer logging over print and consider event name for errors
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: 💡 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:
Replace print with logger and stop emitting reserved 'error' event — use an app-specific event (e.g. 'chat_error')
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| chat_handler = ChatHandler( | ||||||||||||||||||||||||||||||||||||
| db=load_db(os.getenv("SERVER_DB_TYPE", app.config["DB_TYPE"])) | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| chat_handler.chat(message) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| 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}") | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
Comment on lines
+30
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| def on_disconnect(self): | ||||||||||||||||||||||||||||||||||||
| """Handle client disconnection""" | ||||||||||||||||||||||||||||||||||||
| print(f"Client disconnected from chat namespace") | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 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:
Add the following once at the top of the file to support logging (outside the changed hunk):
🧰 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