feat: rename and share session handling #181
Conversation
WalkthroughAdds public session sharing: core Session gains Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Browser (Share URL)
participant Router as Frontend Router
participant ShareView as ShareView.vue
participant Hook as publicUseVideoDBAgent
participant API as /api
participant DB as DB (Postgres/SQLite)
User->>Router: Navigate /share/:sessionId
Router->>ShareView: Mount component
ShareView->>Hook: init(sessionId)
Hook->>API: GET /sessions/public/:sessionId
API->>DB: get_public_session(sessionId)
DB-->>API: session (if is_public=true)
API->>DB: get_conversations(sessionId)
DB-->>API: conversations[]
API-->>Hook: { session, conversations }
Hook-->>ShareView: reactive data
ShareView-->>User: Render public chat
note right of API: Only sessions with is_public=true are returned
sequenceDiagram
autonumber
actor Owner as Authenticated Client
participant API as /api
participant Handler as SessionHandler
participant Core as Session (core)
participant DB as DB
Owner->>API: PUT /sessions/:id/rename { name }
API->>Handler: rename_session(id, name)
Handler->>Core: Session(id).update(name=name)
Core->>DB: update_session(id, name=name, updated_at=now)
DB-->>Core: bool
Core-->>Handler: result
Handler-->>API: payload
API-->>Owner: 200/500
sequenceDiagram
autonumber
actor Client as Websocket Client
participant Socket as ChatNamespace
participant Room as Socket Room (session_id)
participant Core as Session (publisher)
Client->>Socket: connect
Socket->>Client: connected ack
Client->>Socket: join_session { session_id }
Socket->>Room: join room session_id
Socket->>Client: event session_joined
Core->>Socket: publish OutputMessage (room=session_id)
Socket->>Room: emit output_message (room=session_id)
Room-->>Client: receives output_message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 8
🧹 Nitpick comments (12)
backend/director/core/session.py (1)
396-399: Add return type for clarityAnnotate the return to match DB contract.
- def update(self, **kwargs): + def update(self, **kwargs) -> bool: """Update the session in the database.""" return self.db.update_session(self.session_id, **kwargs)frontend/src/router/index.js (1)
3-3: Prefer lazy‑loading ShareView to reduce initial bundle sizeLoad the share view on demand.
-import ShareView from "../views/ShareView.vue"; @@ - { - path: "/share/:sessionId", - name: "Share", - component: ShareView, - }, + { + path: "/share/:sessionId", + name: "Share", + component: () => import("../views/ShareView.vue"), + },Also applies to: 11-15
frontend/src/views/ShareView.vue (2)
6-11: Make sessionId reactive and harden config defaultsEnsure the view reacts to route changes; avoid passing a stale ID to the hook; add a safe BACKEND_URL fallback.
-import { useRoute } from "vue-router"; +import { useRoute } from "vue-router"; +import { ref, watch } from "vue"; @@ -const route = useRoute(); -const sessionId = route.params.sessionId; -const BACKEND_URL = import.meta.env.VITE_APP_BACKEND_URL; +const route = useRoute(); +const sessionId = ref(route.params.sessionId); +watch(() => route.params.sessionId, (v) => (sessionId.value = v)); +const BACKEND_URL = import.meta.env.VITE_APP_BACKEND_URL ?? window.location.origin; import { publicUseVideoDBAgent } from "../hooks/shareViewHandler"; -const publicHook = (config) => publicUseVideoDBAgent(config, sessionId); +const publicHook = (config) => publicUseVideoDBAgent(config, sessionId.value);
18-22: Don’t ship debug logging in productionDrive debug from env to avoid verbose logs in prod.
- debug: true, - sessionId: sessionId, + debug: import.meta.env.DEV === true, + sessionId: sessionId,Confirm ChatInterface accepts a reactive prop value for sessionId in object literals; if not, pass sessionId.value explicitly.
backend/director/entrypoint/api/routes.py (1)
126-146: Public session payload may leak internals; consider shaping/sanitizingYou’re returning full conversation rows (agents/actions/content/metadata). For public sharing, consider a whitelisted subset to prevent accidental leakage of internal agent names, tool params, or metadata.
- Build a response DTO that includes only: session_id, name, created_at/updated_at, and sanitized conversation messages (e.g., content only).
- Optionally paginate conversations to reduce payload size.
backend/director/db/sqlite/db.py (4)
37-39: Use Optional[str] for name to satisfy typing (PEP 484).Change the annotation to Optional[str] and import Optional.
- name: str = None, + name: Optional[str] = None,Add import (outside this hunk):
from typing import List, Optional
157-161: Restore deterministic ordering for conversations.Postgres orders by created_at ASC; SQLite no longer does. This breaks message order consistency.
- self.cursor.execute( - "SELECT * FROM conversations WHERE session_id = ?", (session_id,) - ) + self.cursor.execute( + "SELECT * FROM conversations WHERE session_id = ? ORDER BY created_at ASC", + (session_id,), + )
318-326: SQLite boolean check: prefer = 1 over = TRUE.SQLite stores booleans as 0/1; comparing to TRUE can be dialect‑dependent. Use 1 for reliability.
- WHERE session_id = ? AND is_public = TRUE + WHERE session_id = ? AND is_public = 1
340-343: Trim redundant exception interpolation.logger.exception already captures the exception; avoid including {e}.
- except Exception as e: - logger.exception(f"Error getting public session: {e}") + except Exception: + logger.exception("Error getting public session") return {}backend/director/db/postgres/db.py (2)
40-42: Use Optional[str] for name to satisfy typing (PEP 484).Update annotation and import Optional.
- name: str = None, + name: Optional[str] = None,Add import (outside this hunk):
from typing import List, Optional
264-289: Normalize metadata shape across backends.SQLite returns metadata as dict (via json.loads), Postgres returns raw DB value. For parity, decode string JSON.
- "metadata": row["metadata"] if row["metadata"] else {}, + "metadata": ( + json.loads(row["metadata"]) + if (row.get("metadata") and isinstance(row["metadata"], str)) + else (row["metadata"] or {}) + ),frontend/src/hooks/shareViewHandler.js (1)
42-71: Handle non‑200 responses and parsing errors when fetching the public session.Avoid blindly calling res.json(); check res.ok and guard against non‑JSON bodies.
- fetch(`${httpUrl}/session/public/${sessionId}`) - .then((res) => res.json()) - .then((res) => { + fetch(`${httpUrl}/session/public/${sessionId}`) + .then(async (res) => { + if (!res.ok) { + const text = await res.text().catch(() => ""); + throw new Error(`GET /session/public/${sessionId} failed (${res.status}): ${text}`); + } + return res.json(); + }) + .then((res) => { console.log("debug :videodb-chat res", res); if (res) { session.videoId = res.video_id || null; session.collectionId = res.collection_id || session.collectionId || null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/director/core/session.py(1 hunks)backend/director/db/base.py(1 hunks)backend/director/db/postgres/db.py(3 hunks)backend/director/db/postgres/initialize.py(2 hunks)backend/director/db/sqlite/db.py(5 hunks)backend/director/db/sqlite/initialize.py(2 hunks)backend/director/entrypoint/api/routes.py(1 hunks)backend/director/handler.py(1 hunks)frontend/src/hooks/shareViewHandler.js(1 hunks)frontend/src/router/index.js(1 hunks)frontend/src/views/ShareView.vue(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
backend/director/core/session.py (3)
backend/director/db/base.py (1)
update_session(47-49)backend/director/db/postgres/db.py (1)
update_session(215-245)backend/director/db/sqlite/db.py (1)
update_session(264-299)
backend/director/handler.py (2)
backend/director/entrypoint/api/routes.py (1)
rename_session(66-90)backend/director/core/session.py (2)
Session(310-408)update(396-398)
backend/director/db/base.py (4)
backend/director/db/postgres/db.py (4)
update_session(215-245)delete_session(200-213)make_session_public(247-262)get_public_session(264-289)backend/director/db/sqlite/db.py (4)
update_session(264-299)delete_session(246-262)make_session_public(301-316)get_public_session(318-343)backend/director/handler.py (1)
delete_session(139-141)backend/director/entrypoint/api/routes.py (2)
make_session_public(93-123)get_public_session(127-145)
backend/director/entrypoint/api/routes.py (6)
backend/director/handler.py (3)
rename_session(143-150)SessionHandler(127-150)get_session(135-137)backend/director/core/session.py (1)
get(381-386)backend/director/db/__init__.py (1)
load_db(12-19)backend/director/db/base.py (4)
get_session(15-17)make_session_public(57-59)get_public_session(62-64)get_conversations(30-32)backend/director/db/postgres/db.py (4)
get_session(67-75)make_session_public(247-262)get_public_session(264-289)get_conversations(134-145)backend/director/db/sqlite/db.py (4)
get_session(72-89)make_session_public(301-316)get_public_session(318-343)get_conversations(157-171)
backend/director/db/sqlite/db.py (4)
backend/director/db/sqlite/initialize.py (1)
initialize_sqlite(49-69)backend/director/db/base.py (3)
update_session(47-49)make_session_public(57-59)get_public_session(62-64)backend/director/db/postgres/db.py (3)
update_session(215-245)make_session_public(247-262)get_public_session(264-289)backend/director/entrypoint/api/routes.py (2)
make_session_public(93-123)get_public_session(127-145)
backend/director/db/postgres/db.py (4)
backend/director/db/postgres/initialize.py (1)
initialize_postgres(51-83)backend/director/db/base.py (4)
create_session(8-12)update_session(47-49)make_session_public(57-59)get_public_session(62-64)backend/director/db/sqlite/db.py (4)
create_session(32-70)update_session(264-299)make_session_public(301-316)get_public_session(318-343)backend/director/entrypoint/api/routes.py (2)
make_session_public(93-123)get_public_session(127-145)
🪛 Ruff (0.13.1)
backend/director/db/sqlite/db.py
37-37: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
287-291: Possible SQL injection vector through string-based query construction
(S608)
295-295: Consider moving this statement to an else block
(TRY300)
297-297: Do not catch blind exception: Exception
(BLE001)
298-298: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
313-313: Consider moving this statement to an else block
(TRY300)
315-315: Redundant exception object included in logging.exception call
(TRY401)
340-340: Consider moving this statement to an else block
(TRY300)
342-342: Redundant exception object included in logging.exception call
(TRY401)
backend/director/db/postgres/db.py
40-40: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
233-237: Possible SQL injection vector through string-based query construction
(S608)
241-241: Consider moving this statement to an else block
(TRY300)
243-243: Do not catch blind exception: Exception
(BLE001)
244-244: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
259-259: Consider moving this statement to an else block
(TRY300)
261-261: Redundant exception object included in logging.exception call
(TRY401)
286-286: Consider moving this statement to an else block
(TRY300)
288-288: Redundant exception object included in logging.exception call
(TRY401)
🔇 Additional comments (2)
backend/director/handler.py (1)
143-151: LGTM: thin wrapper around Session.updateStraightforward rename flow with appropriate success/error payloads.
backend/director/db/sqlite/initialize.py (1)
59-67: SQLite migration logic looks solidColumn existence check via PRAGMA and conditional ALTERs are correct; commit covers both changes.
| @session_bp.route("/<session_id>/rename", methods=["PUT"]) | ||
| def rename_session(session_id): | ||
| """ | ||
| Rename a session by updating its metadata | ||
| """ | ||
| if not session_id: | ||
| return {"message": "Please provide session_id."}, 400 | ||
|
|
||
| data = request.get_json() | ||
| if not data or not data.get("name"): | ||
| return {"message": "Session name is required."}, 400 | ||
|
|
||
| new_name = data["name"] | ||
| if not new_name.strip(): | ||
| return {"message": "Session name cannot be empty."}, 400 | ||
|
|
||
| session_handler = SessionHandler( | ||
| db=load_db(os.getenv("SERVER_DB_TYPE", app.config["DB_TYPE"])) | ||
| ) | ||
|
|
||
| session = session_handler.get_session(session_id) | ||
| if not session: | ||
| return {"message": "Session not found."}, 404 | ||
|
|
||
| result = session_handler.rename_session(session_id, new_name) | ||
| return result | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Authorization missing for rename endpoint
This write operation should be authenticated/authorized (ownership check), but only existence is verified. Publicly callable rename risks unauthorized changes.
- If you already gate these blueprints behind auth middleware, ignore.
- Otherwise, gate with your existing auth decorator or add a simple token check until full auth lands.
Would you like a patch wiring a @requires_auth decorator here?
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show file: backend/director/entrypoint/api/routes.py ==="
sed -n '1,240p' backend/director/entrypoint/api/routes.py || true
echo
echo "=== search for auth decorator names ==="
rg -n --hidden --no-ignore 'requires_auth|auth_required|login_required|@requires_auth|@auth_required|@login_required' || true
echo
echo "=== search for Authorization header / token usage ==="
rg -n --hidden --no-ignore 'Authorization|HTTP_AUTHORIZATION|AUTH_TOKEN|Bearer |api_key|API_KEY' || true
echo
echo "=== search for blueprint definitions/registrations and session_bp usage ==="
rg -n --hidden --no-ignore 'Blueprint\(|register_blueprint|session_bp|session_bp.route|bp.route' || true
echo
echo "=== search for before_request hooks that might enforce auth ==="
rg -n --hidden --no-ignore 'before_request|before_app_request|app.before_request|@app.before_request' || trueLength of output: 19931
Require auth + ownership check on session rename (backend/director/entrypoint/api/routes.py::rename_session)
No auth decorator or before_request hook found in the repo; rename_session updates session data without verifying caller or ownership. Gate the route with your auth decorator (or token check) and enforce that the requester owns the session before calling session_handler.rename_session — also audit other session endpoints (GET/DELETE, /<session_id>/public) for the same missing checks.
🤖 Prompt for AI Agents
In backend/director/entrypoint/api/routes.py around lines 65 to 91, the
rename_session route lacks authentication and ownership verification before
mutating session data; update the route to require the project's auth mechanism
(apply the auth decorator or explicit token/session check) and, after
authenticating, verify the authenticated user owns the session returned by
session_handler.get_session(session_id) (return 403 if not); only call
session_handler.rename_session after ownership is confirmed; additionally, audit
sibling session endpoints (GET/DELETE and /<session_id>/public) and add
identical auth + ownership checks to them.
| @session_bp.route("/<session_id>/public", methods=["PUT"]) | ||
| def make_session_public(session_id): | ||
| """ | ||
| Make a session public or private | ||
| """ | ||
| if not session_id: | ||
| return {"message": "Please provide session_id."}, 400 | ||
|
|
||
| data = request.get_json() | ||
| if not data or "is_public" not in data: | ||
| return {"message": "is_public field is required."}, 400 | ||
|
|
||
| is_public = data["is_public"] | ||
| if not isinstance(is_public, bool): | ||
| return {"message": "is_public must be a boolean value."}, 400 | ||
|
|
||
| db = load_db(os.getenv("SERVER_DB_TYPE", app.config["DB_TYPE"])) | ||
|
|
||
| # Check if session exists and belongs to user | ||
| session_handler = SessionHandler(db=db) | ||
| session = session_handler.get_session(session_id) | ||
| if not session: | ||
| return {"message": "Session not found."}, 404 | ||
|
|
||
| # Update the session's public status | ||
| success = db.make_session_public(session_id, is_public) | ||
|
|
||
| if success: | ||
| status = "public" if is_public else "private" | ||
| return {"message": f"Session successfully made {status}."}, 200 | ||
| else: | ||
| return {"message": "Failed to update session visibility."}, 500 | ||
|
|
There was a problem hiding this comment.
Authorization missing for make_public endpoint
Toggling visibility must be restricted to the session owner. Current logic only checks existence, enabling takeover by ID guessing.
- Require auth and ownership.
- Consider rate limiting this endpoint.
- Log visibility changes (session_id, actor, timestamp) for audit.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
backend/director/db/base.py (1)
51-56: Tighten return type to list[str]Clarify element type in the tuple for stronger typing.
Apply this diff:
- def delete_session(self, session_id: str) -> tuple[bool, list]: + def delete_session(self, session_id: str) -> tuple[bool, list[str]]:backend/director/db/sqlite/db.py (4)
37-37: Use Optional[str] for nullable namePEP 484: avoid implicit Optional.
Apply this diff:
- name: str = None, + name: Optional[str] = None,Also update imports at top:
from typing import List, Optional
246-263: Docstring return now outdatedMethod returns (success, failed_components); docstring still says bool.
Apply this diff:
- :return: True if the session was deleted, False otherwise. + :return: (success, failed_components)
331-333: SQLite boolean: prefer 1 over TRUE for portabilitySQLite treats TRUE as 1, but using 1 is clearer and avoids dialect confusion.
Apply this diff:
- WHERE session_id = ? AND is_public = TRUE + WHERE session_id = ? AND is_public = 1
348-350: Simplify exception logging
logging.exceptionalready captures the exception; no need to formate.Apply this diff:
- except Exception as e: - logger.exception(f"Error getting public session: {e}") + except Exception: + logger.exception("Error getting public session")backend/director/db/postgres/db.py (4)
40-40: Use Optional[str] for nullable nameAvoid implicit Optional.
Apply this diff:
- name: str = None, + name: Optional[str] = None,Also update imports:
from typing import List, Optional
215-254: Hardened update_session: LGTM; consider centralizing allowed_fieldsImplementation mirrors SQLite; consider sharing the allowed field set to avoid drift.
267-269: Simplify exception loggingDrop
as eand f-string; let logger capture the exception.Apply this diff:
- except Exception as e: - logger.exception(f"Error making session public/private: {e}") + except Exception: + logger.exception("Error making session public/private")
294-296: Simplify exception loggingSame as above; remove redundant
e.Apply this diff:
- except Exception as e: - logger.exception(f"Error getting public session: {e}") + except Exception: + logger.exception("Error getting public session")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/director/core/session.py(1 hunks)backend/director/db/base.py(1 hunks)backend/director/db/postgres/db.py(4 hunks)backend/director/db/postgres/initialize.py(2 hunks)backend/director/db/sqlite/db.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/director/core/session.py
- backend/director/db/postgres/initialize.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/director/db/sqlite/db.py (5)
backend/director/db/sqlite/initialize.py (1)
initialize_sqlite(49-69)backend/director/db/base.py (4)
delete_session(52-56)update_session(47-49)make_session_public(59-61)get_public_session(64-66)backend/director/db/postgres/db.py (4)
delete_session(200-213)update_session(215-253)make_session_public(255-269)get_public_session(271-296)backend/director/handler.py (1)
delete_session(139-141)backend/director/entrypoint/api/routes.py (2)
make_session_public(93-123)get_public_session(127-145)
backend/director/db/base.py (4)
backend/director/db/postgres/db.py (4)
update_session(215-253)delete_session(200-213)make_session_public(255-269)get_public_session(271-296)backend/director/db/sqlite/db.py (4)
update_session(264-307)delete_session(246-262)make_session_public(309-323)get_public_session(325-350)backend/director/handler.py (1)
delete_session(139-141)backend/director/entrypoint/api/routes.py (2)
make_session_public(93-123)get_public_session(127-145)
backend/director/db/postgres/db.py (4)
backend/director/db/postgres/initialize.py (1)
initialize_postgres(51-82)backend/director/db/base.py (5)
create_session(8-12)delete_session(52-56)update_session(47-49)make_session_public(59-61)get_public_session(64-66)backend/director/db/sqlite/db.py (5)
create_session(32-70)delete_session(246-262)update_session(264-307)make_session_public(309-323)get_public_session(325-350)backend/director/entrypoint/api/routes.py (2)
make_session_public(93-123)get_public_session(127-145)
🪛 Ruff (0.13.1)
backend/director/db/sqlite/db.py
37-37: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
295-299: Possible SQL injection vector through string-based query construction
(S608)
303-303: Consider moving this statement to an else block
(TRY300)
320-320: Consider moving this statement to an else block
(TRY300)
347-347: Consider moving this statement to an else block
(TRY300)
349-349: Redundant exception object included in logging.exception call
(TRY401)
backend/director/db/postgres/db.py
40-40: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
241-245: Possible SQL injection vector through string-based query construction
(S608)
249-249: Consider moving this statement to an else block
(TRY300)
266-266: Consider moving this statement to an else block
(TRY300)
268-268: Redundant exception object included in logging.exception call
(TRY401)
293-293: Consider moving this statement to an else block
(TRY300)
295-295: Redundant exception object included in logging.exception call
(TRY401)
🔇 Additional comments (11)
backend/director/db/base.py (3)
46-49: Interface addition looks goodAbstract
update_sessionwith kwargs and bool return is appropriate and matches impls.
58-62: Public visibility toggle: LGTMSignature and docstring align with usage.
63-67: Public fetch API: LGTMBoth PostgresDB and SQLiteDB implement update_session, make_session_public, get_public_session, and delete_session (backend/director/db/postgres/db.py, backend/director/db/sqlite/db.py).
backend/director/db/sqlite/db.py (5)
26-26: Early initialization: LGTMCalling
initialize_sqlitein__init__is fine and idempotent per initializer.
57-59: Include name in INSERT: LGTMColumn order and bound params line up; metadata JSON-encoded.
Also applies to: 64-64
159-160: Query param binding nit: LGTMInline tuple is fine and clearer.
264-308: Hardened update_session: LGTM
- Whitelisted fields mitigate S608.
- JSON-encoding metadata and rowcount check are correct.
309-324: Visibility toggle: LGTMConsistent seconds timestamps and success check.
backend/director/db/postgres/db.py (3)
34-34: Init DB on startup: LGTM
initialize_postgres()on construct matches SQLite path and is safe.
51-53: INSERT with name: LGTMOrdering and JSON encoding are correct.
Also applies to: 59-59
200-214: Tuple return delete_session: LGTMContract matches BaseDB and provides useful diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
backend/director/entrypoint/api/socket_io.py (4)
18-20: Avoid generic 'error' event; use a namespaced event name.Prevents collisions with framework error handling.
Apply this diff:
- emit('error', {'message': 'session_id is required'}) + emit('chat_error', {'message': 'session_id is required'})
16-24: Re-evaluate joining the room on every message.Joining on each
on_chatcall is redundant. Prefer joining once (on connect or via expliciton_join_session) to cut per-message overhead. If you keep auto-join, leave a comment clarifying intent.
25-27: Avoid reloading the DB per message.Initialize and cache the DB instance once (e.g., in
on_connector__init__), reuse it across events.Example (outside diff scope):
class ChatNamespace(Namespace): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) # Delay init to connection if app.config isn't ready here self._db = None def on_connect(self): self._db = load_db(os.getenv("SERVER_DB_TYPE", app.config["DB_TYPE"])) print("Client connected to chat namespace") def on_chat(self, message): chat_handler = ChatHandler(db=self._db) chat_handler.chat(message)
13-13: Fix Ruff F541: f-string without placeholders.Apply these diffs:
- print(f"Client connected to chat namespace") + print("Client connected to chat namespace")- print(f"Client disconnected from chat namespace") + print("Client disconnected from chat namespace")Also applies to: 40-40
backend/director/core/session.py (2)
229-233: Replace prints with logger; avoid blind exceptions and satisfy Ruff (RUF010).Use structured logging and
!sfor explicit conversion.Apply these diffs:
- except Exception as e: - print(f"Error in emitting update to session {self.session_id}: {str(e)}") + except Exception as e: + print(f"Error in emitting update to session {self.session_id}: {e!s}")- except Exception as e: - print(f"Error emitting to session {self.session_id}: {str(e)}") + except Exception as e: + print(f"Error emitting to session {self.session_id}: {e!s}")Optionally switch to logger (outside diff scope):
import logging logger = logging.getLogger(__name__) # ... then use logger.exception(...) / logger.info(...)Also applies to: 245-245
247-247: Be explicit about excludingdbfrom dumps.Pydantic’s
exclude=Trueshould suffice, but making it explicit reduces surprises.Apply this diff:
- self.db.add_or_update_msg_to_conv(**self.model_dump()) + self.db.add_or_update_msg_to_conv(**self.model_dump(exclude={"db"}))
📜 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/core/session.py (3)
backend/director/db/base.py (2)
add_or_update_msg_to_conv(25-27)update_session(47-49)backend/director/db/postgres/db.py (2)
add_or_update_msg_to_conv(82-132)update_session(215-253)backend/director/db/sqlite/db.py (2)
add_or_update_msg_to_conv(104-155)update_session(264-307)
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)
🪛 Ruff (0.13.1)
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)
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)
🔇 Additional comments (1)
backend/director/core/session.py (1)
399-402: Session.update passthrough looks good.Straightforward delegation to DB layer; consistent with new DB surface.
Please confirm
BaseDB.update_sessionenforces allowed fields consistently across backends (sqlite/postgres).
| 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.
Persist before emitting to prevent “phantom” messages.
Emit currently happens before DB write, so a failed write can leave clients seeing messages that aren’t in history. Persist first, then emit.
Apply this diff:
- 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(exclude={"db"})
+ # Persist first to keep history consistent with real-time updates
+ try:
+ self.db.add_or_update_msg_to_conv(**payload)
+ except Exception as e:
+ print(f"Error persisting message for session {self.session_id}: {e!s}")
+ # Then emit to the session room
+ try:
+ emit("chat", payload, 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}: {e!s}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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()) | |
| def _publish(self): | |
| payload = self.model_dump(exclude={"db"}) | |
| # Persist first to keep history consistent with real-time updates | |
| try: | |
| self.db.add_or_update_msg_to_conv(**payload) | |
| except Exception as e: | |
| print(f"Error persisting message for session {self.session_id}: {e!s}") | |
| # Then emit to the session room | |
| try: | |
| emit("chat", payload, 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}: {e!s}") |
🧰 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 to 248, the code emits the
chat message before persisting it, which can create “phantom” messages if the DB
write fails; move the DB write
(self.db.add_or_update_msg_to_conv(**self.model_dump())) to occur before emit,
check for exceptions on the DB operation and only proceed to emit if persistence
succeeds, and in the DB-exception path log/raise the error (and avoid emitting)
so clients only see messages that are actually stored.
| 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.
🧩 Analysis chain
Authorize room joins: require session to be public before joining.
Anyone can join any session room by ID. Gate on_join_session with an is_public check to avoid leaking private sessions.
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}")
+ """Explicitly join a session room (requires public session)"""
+ session_id = data.get('session_id')
+ if not session_id:
+ emit('chat_error', {'message': 'session_id is required'})
+ return
+ db = load_db(os.getenv("SERVER_DB_TYPE", app.config["DB_TYPE"]))
+ # Allow joining only if the session is explicitly public
+ is_public = False
+ get_public = getattr(db, "get_public_session", None)
+ if callable(get_public):
+ is_public = bool(db.get_public_session(session_id))
+ else:
+ sess = db.get_session(session_id)
+ is_public = bool(sess and sess.get("is_public"))
+ if not is_public:
+ emit('chat_error', {'message': 'Session is not public'})
+ return
+ join_room(session_id)
+ emit('session_joined', {'session_id': session_id})
+ print(f"Client explicitly joined session room {session_id}")🏁 Script executed:
#!/bin/bash
# Verify DB exposes a public-session lookup
rg -nP 'def\s+get_public_session\s*\(' -C2Length of output: 1384
🏁 Script executed:
#!/bin/bash
sed -n '1,240p' backend/director/entrypoint/api/socket_io.pyLength of output: 1394
Authorize room joins: require session to be public before joining.
on_join_session currently allows arbitrary joins by session_id; gate it with the DB's get_public_session/get_session lookup (implementations exist in backend/director/db/*) and return an error without calling join_room if the session is not public. Apply the same check to on_chat (it currently auto-joins).
Location: backend/director/entrypoint/api/socket_io.py — ChatNamespace.on_join_session (and on_chat)
🤖 Prompt for AI Agents
In backend/director/entrypoint/api/socket_io.py around lines 30-37
(ChatNamespace.on_join_session) and the on_chat handler, the code allows joining
rooms by session_id without authorization; fetch the session from the DB using
the existing backend/director/db/* utilities (get_public_session or
get_session), verify the session exists and is public, and only then call
join_room and emit session_joined; if the session is missing or not public, do
NOT call join_room and instead emit a clear error event (e.g.,
emit('session_join_error', { 'session_id': id, 'error': 'session not public' }))
and return; apply the exact same DB lookup+public-check flow to on_chat before
any automatic join to prevent unauthorized room joins.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/director/entrypoint/api/routes.py (2)
73-99: Add auth + ownership checks before renaming sessionsThis endpoint still performs a write with no authentication or ownership verification, so anyone knowing a session_id can rename it. Gate the route with your existing auth middleware/decorator and refuse the update unless the authenticated user owns the session (return 403 otherwise).
100-132: Prevent unauthenticated toggling of session visibilityLikewise,
/publicflipsis_publicwithout any auth or ownership guard. An attacker can guess IDs and expose or hide sessions. Protect the route with the project’s auth mechanism and verify the caller owns the target session before updating visibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/director/constants.py(1 hunks)backend/director/entrypoint/api/routes.py(2 hunks)backend/director/handler.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/director/handler.py (6)
backend/director/llm/__init__.py (1)
get_default_llm(11-27)backend/director/llm/base.py (2)
LLMResponseStatus(8-10)chat_completions(68-70)backend/director/core/session.py (9)
ContextMessage(270-310)RoleTypes(13-19)Session(313-411)InputMessage(204-212)MsgStatus(22-30)update(399-401)create(357-359)get(384-389)to_llm_msg(284-305)backend/director/db/sqlite/db.py (1)
create_session(32-70)backend/director/db/postgres/db.py (1)
create_session(35-65)backend/director/llm/openai.py (1)
chat_completions(141-190)
backend/director/entrypoint/api/routes.py (6)
backend/director/handler.py (4)
create_session(158-186)rename_session(149-156)SessionHandler(133-186)get_session(141-143)backend/director/db/base.py (5)
create_session(8-12)get_session(15-17)make_session_public(59-61)get_public_session(64-66)get_conversations(30-32)backend/director/db/sqlite/db.py (5)
create_session(32-70)get_session(72-89)make_session_public(309-323)get_public_session(325-350)get_conversations(157-171)backend/director/db/postgres/db.py (5)
create_session(35-65)get_session(67-75)make_session_public(255-269)get_public_session(271-296)get_conversations(134-145)backend/director/core/session.py (1)
get(384-389)backend/director/db/__init__.py (1)
load_db(12-19)
| elif request.method == "POST": | ||
| data = request.get_json() | ||
|
|
||
| session = session_handler.create_session( | ||
| data.get("message") | ||
| ) | ||
|
|
||
| return session, 200 |
There was a problem hiding this comment.
Validate POST payload before delegating to create_session
Right now we call session_handler.create_session(data.get("message")) without confirming data exists or that "message" holds a dict. If the client forgets "message" (or sends null), the handler ends up doing Session(db=self.db, **None) and the request bombs with a 500. Add payload validation here and return a 4xx when the body is missing/ill-formed so we don’t crash on bad input.
🤖 Prompt for AI Agents
In backend/director/entrypoint/api/routes.py around lines 63 to 70, the POST
handler currently calls session_handler.create_session(data.get("message"))
without validating the request body; add checks to ensure request.get_json()
returned a non-null object, that it contains a "message" key, and that
data["message"] is a dict (or at least truthy and of expected type) before
calling create_session; if validation fails return a 4xx (400) response with a
clear error message (e.g., "missing or invalid 'message' payload") instead of
delegating to create_session so the service does not raise a 500 on bad input.
| session = Session(db=self.db, **message) | ||
| session.create() | ||
|
|
||
| session_dict = session.get() | ||
| if not session_dict["name"]: | ||
| llm = get_default_llm() | ||
| context_messages = [ | ||
| ContextMessage( | ||
| role=RoleTypes.system, | ||
| content=CHAT_NAMING_SYSTEM_PROMPT |
There was a problem hiding this comment.
Guard against missing/invalid message payload before unpacking
Line 162 builds a Session via Session(db=self.db, **message), but the API passes whatever sits under "message" in the request body. If the client omits that field or sends something other than a dict, this unpacking raises immediately (e.g., TypeError: 'NoneType' object is not iterable), returning a 500 instead of a clean 400. Please validate the payload (ensure it exists and is a mapping) before unpacking, and fail fast with a 4xx response if it’s absent.
🤖 Prompt for AI Agents
In backend/director/handler.py around lines 159 to 168, the code unpacks message
into Session with Session(db=self.db, **message) without validating message; add
a guard just before that line to ensure message is present and is a mapping
(e.g., isinstance(message, dict) or collections.abc.Mapping). If the check
fails, return an HTTP 400 response (or raise a framework-specific BadRequest)
with a clear error message about the missing/invalid "message" payload;
otherwise proceed to unpack safely. Ensure any unit tests or callers expecting
4xx behavior are updated accordingly.
1 participant
Features added
namecolumn tosessionstable for renaming sessionsis_publiccolumn tosessionstable for allowing session sharingvideodb-chatPR: feat: rename, share logic and popper menu videodb-chat#102Summary by CodeRabbit
New Features
API
Frontend
Chores
Bug Fixes