feature: introduce /share route for web widget #853
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Files:
🧬 Code graph analysis (1)routers/root.py (1)
🪛 Ruff (0.14.8)routers/root.py548-548: Unused function argument: (ARG001) 565-565: Within an (B904) ⏰ 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). (1)
🔇 Additional comments (3)
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: 1
🧹 Nitpick comments (1)
routers/root.py (1)
533-585: Tighten chat share route typing and conversation lookupThe new
/share/{conversation_id}/variant andconversationDatawiring look good overall, but a few small tweaks will improve robustness and keep linters happy:
- Use explicit optionals for type hints (RUF013)
Align with the rest of the file (e.g.,example_id: str | None) and Ruff’s guidance by making these parameters explicit optionals:-def chat_route( - request: Request, - integration_id: str = None, - integration_name: str = None, - conversation_id: str = None, -): +def chat_route( + request: Request, + integration_id: str | None = None, + integration_name: str | None = None, + conversation_id: str | None = None, +):
integration_nameremains unused; if Ruff’sARG001is enforced, you may want to either prefix it with_or add a short comment to clarify it’s only there for URL aesthetics.
- Make conversation/bot association explicit (hardening)
To avoid accidentally showing a conversation under the wrong bot if a mismatchedconversation_idis passed, you can assert thebot_integrationrelationship and reuse your existing redirect behavior:- try: - conversation: Conversation = Conversation.objects.get( - id=api_hashids.decode(conversation_id)[0], - ) - mesasges = db_msgs_to_api_json(conversation.last_n_msgs()) + try: + conversation: Conversation = Conversation.objects.get( + id=api_hashids.decode(conversation_id)[0], + ) + if conversation.bot_integration_id != bi.id: + raise Conversation.DoesNotExist + + messages = db_msgs_to_api_json(conversation.last_n_msgs()) conversationData = dict( id=conversation_id, bot_id=integration_id, timestamp=conversation.created_at.isoformat(), user_id=conversation.web_user_id, - messages=mesasges, + messages=messages, ) except (IndexError, Conversation.DoesNotExist): - # remove /share/conversation_id from the url and redirect to the root url - redirect_url = furl( - request.url.path.replace(f"/share/{conversation_id}", "/") - ) - return RedirectResponse(str(redirect_url), status_code=302) + # remove /share/{conversation_id} from the URL and redirect to the root URL + base_path = request.url.path.split("/share/", 1)[0].rstrip("/") + "/" + redirect_url = furl(base_path) + return RedirectResponse(str(redirect_url), status_code=302)This also:
- Fixes the
mesasgestypo while keeping behavior the same.- Avoids potential
//in the redirect path caused by the currentreplace.Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bots/models/convo_msg.py(2 hunks)routers/root.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
routers/root.pybots/models/convo_msg.py
🧬 Code graph analysis (1)
routers/root.py (1)
bots/models/convo_msg.py (4)
Conversation(124-336)db_msgs_to_api_json(598-685)last_n_msgs(330-331)last_n_msgs(456-462)
🪛 Ruff (0.14.8)
routers/root.py
537-537: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
538-538: Unused function argument: integration_name
(ARG001)
538-538: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
539-539: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
⏰ 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). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (2)
bots/models/convo_msg.py (1)
12-12: Import ofQfor attachment filtering looks correctThe added
Qimport is required for theinput_documentsexclusion logic indb_msgs_to_api_jsonand is consistent with the rest of the module.routers/root.py (1)
10-13: Direct import ofConversationanddb_msgs_to_api_jsonis appropriatePulling
Conversationand the new helper directly frombots.models.convo_msgkeeps this route’s dependencies explicit and avoids bloating the genericbots.modelsimport.
bots/models/convo_msg.py
Outdated
| def db_msgs_to_api_json(msgs: list["Message"]) -> list[dict]: | ||
| from daras_ai_v2.bots import parse_bot_html | ||
| from routers.bots_api import MSG_ID_PREFIX | ||
|
|
||
| api_messages = [None] * len(msgs) | ||
| for i, msg in enumerate(msgs): | ||
| msg: Message | ||
| if msg.role == CHATML_ROLE_USER: | ||
| input_images = ( | ||
| list( | ||
| MessageAttachment.objects.filter( | ||
| message=msg, metadata__mime_type__startswith="image/" | ||
| ).values_list("url", flat=True) | ||
| ) | ||
| or [] | ||
| ) | ||
|
|
||
| input_audio = ( | ||
| list( | ||
| MessageAttachment.objects.filter( | ||
| message=msg, metadata__mime_type__startswith="audio/" | ||
| ).values_list("url", flat=True) | ||
| ) | ||
| or "" | ||
| ) | ||
|
|
||
| # any document type other than audio/image | ||
| input_documents = ( | ||
| list( | ||
| MessageAttachment.objects.filter(message=msg) | ||
| .exclude( | ||
| Q(metadata__mime_type__startswith="image/") | ||
| | Q(metadata__mime_type__startswith="audio/") | ||
| ) | ||
| .values_list("url", flat=True) | ||
| ) | ||
| or [] | ||
| ) | ||
| api_messages[i] = { | ||
| "role": msg.role, | ||
| "input_prompt": msg.display_content, | ||
| "input_images": input_images, | ||
| "input_audio": input_audio, | ||
| "input_documents": input_documents, | ||
| "created_at": msg.created_at.isoformat(), | ||
| } | ||
| elif msg.role == CHATML_ROLE_ASSISTANT: | ||
| output_images = ( | ||
| list( | ||
| MessageAttachment.objects.filter( | ||
| message=msg, metadata__mime_type__startswith="image/" | ||
| ).values_list("url", flat=True) | ||
| ) | ||
| or [] | ||
| ) | ||
| output_audio = ( | ||
| list( | ||
| MessageAttachment.objects.filter( | ||
| message=msg, metadata__mime_type__startswith="audio/" | ||
| ).values_list("url", flat=True) | ||
| ) | ||
| or [] | ||
| ) | ||
| output_documents = ( | ||
| list( | ||
| MessageAttachment.objects.filter( | ||
| message=msg, metadata__mime_type__startswith="application/pdf" | ||
| ).values_list("url", flat=True) | ||
| ) | ||
| or [] | ||
| ) | ||
| buttons, text, _ = parse_bot_html(msg.display_content) | ||
| api_messages[i] = { | ||
| "role": msg.role, | ||
| "created_at": msg.created_at.isoformat(), | ||
| "status": "completed", | ||
| "type": "final_response", | ||
| "raw_output_text": [msg.content], | ||
| "output_text": [text], | ||
| "buttons": buttons, | ||
| "output_images": output_images, | ||
| "output_audio": output_audio, | ||
| "output_documents": output_documents, | ||
| "web_url": msg.saved_run.get_app_url(), | ||
| "user_message_id": msg.platform_msg_id, | ||
| "bot_message_id": msg.platform_msg_id.strip(MSG_ID_PREFIX), | ||
| } | ||
| return api_messages |
There was a problem hiding this comment.
Fix None-handling and ID stripping in db_msgs_to_api_json
There are a few correctness issues here that can lead to crashes or inconsistent data:
msg.platform_msg_idis nullable, butmsg.platform_msg_id.strip(MSG_ID_PREFIX)will raise if it isNone, and.strip()removes characters, not a prefix substring, so IDs can be mangled.msg.saved_runis nullable, butmsg.saved_run.get_app_url()is called unconditionally and can raise.input_audiofalls back to"", while all other attachment fields are lists (andoutput_audiouses[]), which makes the type ofinput_audioinconsistent.
Suggested minimal fix:
def db_msgs_to_api_json(msgs: list["Message"]) -> list[dict]:
@@
- input_audio = (
- list(
- MessageAttachment.objects.filter(
- message=msg, metadata__mime_type__startswith="audio/"
- ).values_list("url", flat=True)
- )
- or ""
- )
+ input_audio = (
+ list(
+ MessageAttachment.objects.filter(
+ message=msg, metadata__mime_type__startswith="audio/"
+ ).values_list("url", flat=True)
+ )
+ or []
+ )
@@
- api_messages[i] = {
+ api_messages[i] = {
"role": msg.role,
"created_at": msg.created_at.isoformat(),
"status": "completed",
"type": "final_response",
"raw_output_text": [msg.content],
"output_text": [text],
"buttons": buttons,
"output_images": output_images,
"output_audio": output_audio,
"output_documents": output_documents,
- "web_url": msg.saved_run.get_app_url(),
- "user_message_id": msg.platform_msg_id,
- "bot_message_id": msg.platform_msg_id.strip(MSG_ID_PREFIX),
+ "web_url": msg.saved_run.get_app_url() if msg.saved_run else None,
+ "user_message_id": msg.platform_msg_id,
+ "bot_message_id": (
+ msg.platform_msg_id
+ and msg.platform_msg_id.removeprefix(MSG_ID_PREFIX)
+ ),
}Additionally (non-blocking but recommended), you could reuse msg.attachments instead of repeated MessageAttachment.objects.filter(...) calls to benefit from any existing prefetch_related("attachments") and cut down on per-message queries.
🤖 Prompt for AI Agents
In bots/models/convo_msg.py around lines 598-685, fix three issues: ensure
attachment fields are consistently lists by changing input_audio's fallback to
[] (matching other fields); guard against None for msg.platform_msg_id before
deriving bot_message_id (only strip the MSG_ID_PREFIX if platform_msg_id is
truthy and use a proper prefix-removal, e.g. if
platform_msg_id.startswith(MSG_ID_PREFIX) then remove that slice, otherwise
leave as-is); and guard msg.saved_run before calling get_app_url() (use a
conditional to call get_app_url() only when saved_run is not None and provide a
safe default like None or empty string otherwise). Optionally, replace repeated
MessageAttachment.objects.filter(...) calls with using msg.attachments
(prefetch_related) to reduce queries.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
routers/root.py (2)
533-540: Tightenchat_routetype hints and address Ruff warnings (RUF013, ARG001)To align with existing style in this file and satisfy Ruff:
- Avoid implicit
Optionalby using explicit unions.- Treat path parameters as required (no
= None) except forconversation_id, which is only present on the/share/...variant.integration_nameis unused but required for the path parameter; if you want to silenceARG001you can either (a) start using it (e.g., in SEO/meta), or (b) add a# noqa: ARG001comment.A concise refactor that should work with FastAPI and Ruff:
-@app.get("/chat/{integration_name}-{integration_id}/") -@app.get("/chat/{integration_name}-{integration_id}/share/{conversation_id}/") -def chat_route( - request: Request, - integration_id: str = None, - integration_name: str = None, - conversation_id: str = None, -): +@app.get("/chat/{integration_name}-{integration_id}/") +@app.get("/chat/{integration_name}-{integration_id}/share/{conversation_id}/") +def chat_route( + request: Request, + integration_id: str, + integration_name: str, + conversation_id: str | None = None, +):If you’d like to keep
conversation_idstrictly required on the/share/route, you can also drop the| Noneand default, but the above matches your current logic (if conversation_id:).
574-581: Embedding conversationData into the web widget config looks consistentPassing
conversationDatathroughget_web_widget_embed_codealongsideenableShareConversation=Truecleanly wires the new server-side behavior into the widget without affecting existing calls whenconversation_idis absent (the value will just beNone).If you’d prefer to omit the key entirely when there’s no conversation, you could conditionally build the config dict, but that’s purely stylistic:
- "embed_code": get_web_widget_embed_code( - bi, - config=dict( - mode="fullscreen", - enableShareConversation=True, - conversationData=conversationData, - ), - ), + "embed_code": get_web_widget_embed_code( + bi, + config={ + "mode": "fullscreen", + "enableShareConversation": True, + **( + {"conversationData": conversationData} + if conversationData is not None + else {} + ), + }, + ),Not required, just an option if you want a slightly cleaner JSON shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
routers/root.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
routers/root.py
🪛 Ruff (0.14.8)
routers/root.py
537-537: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
538-538: Unused function argument: integration_name
(ARG001)
538-538: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
539-539: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
⏰ 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). (2)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (1)
routers/root.py (1)
10-13: Importing Conversation and db_msgs_to_api_json looks correctThe new imports are minimal, scoped, and used below in
chat_routewithout introducing any circular or unused-import issues.
routers/root.py
Outdated
| conversationData = None | ||
| if conversation_id: | ||
| try: | ||
| conversation: Conversation = Conversation.objects.get( | ||
| id=api_hashids.decode(conversation_id)[0], | ||
| ) | ||
| mesasges = db_msgs_to_api_json(conversation.last_n_msgs()) | ||
| conversationData = dict( | ||
| id=conversation_id, | ||
| bot_id=integration_id, | ||
| timestamp=conversation.created_at.isoformat(), | ||
| user_id=conversation.web_user_id, | ||
| messages=mesasges, | ||
| ) | ||
| except (IndexError, Conversation.DoesNotExist): | ||
| # remove /share/conversation_id from the url and redirect to the root url | ||
| redirect_url = furl( | ||
| request.url.path.replace(f"/share/{conversation_id}", "/") | ||
| ) | ||
| return RedirectResponse(str(redirect_url), status_code=302) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the routers/root.py file and check its content
fd -type f -name "root.py" | grep -E "routers/root\.py"Repository: GooeyAI/gooey-server
Length of output: 234
🏁 Script executed:
# Get the file and check its size first
wc -l routers/root.pyRepository: GooeyAI/gooey-server
Length of output: 83
🏁 Script executed:
# Read lines 549-568 to see the actual code
sed -n '549,568p' routers/root.pyRepository: GooeyAI/gooey-server
Length of output: 949
🏁 Script executed:
# Also read some context around it (540-575) to understand the function better
sed -n '540,575p' routers/root.pyRepository: GooeyAI/gooey-server
Length of output: 1475
🏁 Script executed:
# Get the full function to understand context
sed -n '530,575p' routers/root.pyRepository: GooeyAI/gooey-server
Length of output: 1754
🏁 Script executed:
# Check the function signature and decorators
sed -n '520,575p' routers/root.pyRepository: GooeyAI/gooey-server
Length of output: 2028
🏁 Script executed:
# Look for how conversation_id is used elsewhere and what web_user_id represents
rg "web_user_id" routers/root.py -B 2 -A 2Repository: GooeyAI/gooey-server
Length of output: 264
🏁 Script executed:
# Check what db_msgs_to_api_json returns to understand data structure
rg "def db_msgs_to_api_json" -A 10Repository: GooeyAI/gooey-server
Length of output: 736
🏁 Script executed:
# Check the Conversation model to understand web_user_id
rg "class Conversation" -A 20 | head -40Repository: GooeyAI/gooey-server
Length of output: 1985
🏁 Script executed:
# Check if web_user_id is mentioned in security/privacy contexts
rg "web_user_id" -B 3 -A 3Repository: GooeyAI/gooey-server
Length of output: 5388
🏁 Script executed:
# Look at the structure of the route - check trailing slash handling
sed -n '532,535p' routers/root.pyRepository: GooeyAI/gooey-server
Length of output: 214
Fix redirect path, variable typo, and review privacy exposure in shared conversation endpoint
Three issues need addressing:
-
Redirect path double slash: The path replacement
request.url.path.replace(f"/share/{conversation_id}", "/")will produce a double slash. For a URL like/chat/foo-123/share/abc/, this creates/chat/foo-123//instead of the intended/chat/foo-123/. Update the replacement pattern tof"/share/{conversation_id}/"to account for the trailing slash. -
Typo in variable name: Line 554 has
mesasgesinstead ofmessages, and line 560 references this typo. Rename tomessages. -
Privacy exposure: The
web_user_id(internal user tracking identifier) is exposed in the public share endpoint at/chat/{integration_name}-{integration_id}/share/{conversation_id}/. Anyone with the share link can see this identifier, which reveals who initiated the conversation. Consider whether this user identifier should be included or omitted from the shared conversation data.
Apply the suggested changes:
- Fix redirect path:
replace(f"/share/{conversation_id}/", "/") - Rename
mesasges→messageson lines 554 and 560 - Evaluate whether
user_id=conversation.web_user_idshould remain in the public response
🤖 Prompt for AI Agents
In routers/root.py around lines 549-568, fix three issues: change the path
replacement to remove the extra slash by using
replace(f"/share/{conversation_id}/", "/") when building redirect_url; rename
the misspelled local variable `mesasges` to `messages` wherever used (assignment
and when building conversationData); and remove or omit the
`user_id=conversation.web_user_id` field from the public `conversationData`
payload (or replace it with a non-identifying placeholder) so the internal
web_user_id is not exposed in the shared response.
anish-work
force-pushed
the
feature/share-web-widget-conversations
branch
from
419b7bd to
7ccdfed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
routers/root.py (1)
549-568: Fix redirect path, variable typo, and avoid exposingweb_user_idin shared payloadThree issues here (same as the earlier review, still present in this revision):
Redirect can generate a double slash
request.url.path.replace(f"/share/{conversation_id}", "/")leaves the trailing/, so/chat/foo-123/share/abc/becomes/chat/foo-123//. Replace the full"/share/{conversation_id}/"segment instead.Typo in
mesasgesvariable
Local name is misspelled, and the same typo is used in theconversationDatadict. This is error‑prone and hurts readability.Privacy:
web_user_idis exposed in a public share endpoint
The share URL is intended to be public; includinguser_id=conversation.web_user_idleaks an internal tracking identifier about who initiated the conversation. Unless product explicitly requires this, it’s safer to omit it (or replace with a non-identifying label) fromconversationData.Suggested patch:
- conversationData = None - if conversation_id: - try: - conversation: Conversation = Conversation.objects.get( - id=api_hashids.decode(conversation_id)[0], - ) - mesasges = db_msgs_to_api_json(conversation.last_n_msgs()) - conversationData = dict( - id=conversation_id, - bot_id=integration_id, - timestamp=conversation.created_at.isoformat(), - user_id=conversation.web_user_id, - messages=mesasges, - ) - except (IndexError, Conversation.DoesNotExist): - # remove /share/conversation_id from the url and redirect to the root url - redirect_url = furl( - request.url.path.replace(f"/share/{conversation_id}", "/") - ) - return RedirectResponse(str(redirect_url), status_code=302) + conversationData = None + if conversation_id: + try: + conversation: Conversation = Conversation.objects.get( + id=api_hashids.decode(conversation_id)[0], + ) + messages = db_msgs_to_api_json(conversation.last_n_msgs()) + conversationData = dict( + id=conversation_id, + bot_id=integration_id, + timestamp=conversation.created_at.isoformat(), + # user_id intentionally omitted to avoid exposing internal identifiers + messages=messages, + ) + except (IndexError, Conversation.DoesNotExist): + # remove /share/conversation_id/ from the url and redirect to the root url + redirect_url = furl( + request.url.path.replace(f"/share/{conversation_id}/", "/") + ) + return RedirectResponse(str(redirect_url), status_code=302)Additionally (optional but recommended), consider asserting that the loaded
conversationactually belongs tobibefore buildingconversationData, and treat mismatches as not found.
🧹 Nitpick comments (1)
routers/root.py (1)
533-539: Tighten type hints and handle Ruff’s ARG001 warning forintegration_nameFunctionally this signature works, but there are a couple of small cleanups you can make:
- To satisfy RUF013, consider making the optional parameters explicit, e.g.:
def chat_route( request: Request, integration_id: str | None = None, integration_name: str | None = None, # required by the path, but may stay unused conversation_id: str | None = None, ):integration_nameis required for the FastAPI path, but unused in the body, which trips ARG001. Easiest fix is to keep the name (for routing) and add a# noqa: ARG001comment, or explicitly reference it in logging/metrics if useful.These are non-blocking, but will keep static analysis clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bots/models/convo_msg.py(2 hunks)routers/root.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bots/models/convo_msg.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
routers/root.py
🧬 Code graph analysis (1)
routers/root.py (2)
bots/models/convo_msg.py (4)
Conversation(124-336)db_msgs_to_api_json(600-687)last_n_msgs(330-331)last_n_msgs(456-462)daras_ai_v2/bot_integration_widgets.py (1)
get_web_widget_embed_code(364-385)
🪛 Ruff (0.14.8)
routers/root.py
537-537: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
538-538: Unused function argument: integration_name
(ARG001)
538-538: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
539-539: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
⏰ 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). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (2)
routers/root.py (2)
10-13: Import ofConversationanddb_msgs_to_api_jsonis appropriate and scoped correctlyBoth symbols are used only inside
chat_routefor the share flow; importing them here keeps dependencies local and matches their use below. No issues from a correctness or style perspective.
574-581: EmbeddingconversationDatainto widget config looks correctPassing
enableShareConversation=Trueand the (possiblyNone)conversationDataintoget_web_widget_embed_codekeeps the existing fullscreen behavior and cleanly extends it for shared conversations. The data remains JSON‑serializable and defaults tonullwhen no conversation is provided, which the frontend can handle.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| "created_at": msg.created_at.isoformat(), | ||
| } | ||
| elif msg.role == CHATML_ROLE_ASSISTANT: | ||
| references = msg.saved_run.state.get("references") |
There was a problem hiding this comment.
Bug: Null pointer when saved_run is None
The code accesses msg.saved_run.state.get("references") without checking if saved_run is None. The Message.saved_run field is defined with null=True, default=None, so assistant messages may have no associated saved run, which would cause an AttributeError when accessing .state.
| "output_audio": audio, | ||
| "web_url": msg.saved_run.get_app_url(), | ||
| "user_message_id": msg.platform_msg_id, | ||
| "bot_message_id": msg.platform_msg_id.strip(MSG_ID_PREFIX), |
There was a problem hiding this comment.
Bug: Null pointer when platform_msg_id is None
The code calls msg.platform_msg_id.strip(MSG_ID_PREFIX) without checking if platform_msg_id is None. The Message.platform_msg_id field is defined with null=True, default=None, so this will raise an AttributeError when the field is None.
| try: | ||
| conversation: Conversation = Conversation.objects.get( | ||
| id=api_hashids.decode(conversation_id)[0], | ||
| ) |
There was a problem hiding this comment.
Bug: Conversation access lacks bot integration authorization check
The conversation is fetched only by its ID without verifying it belongs to the bi (bot integration) specified in the URL. Since Conversation has a bot_integration foreign key, a malicious user could craft a share URL with one integration's ID but another conversation's ID, potentially exposing private conversation data from a different integration. The query needs to filter by bot_integration=bi as well.
| "output_audio": audio, | ||
| "web_url": msg.saved_run.get_app_url(), | ||
| "user_message_id": msg.platform_msg_id, | ||
| "bot_message_id": msg.platform_msg_id.strip(MSG_ID_PREFIX), |
There was a problem hiding this comment.
Bug: Wrong string method strips characters instead of prefix
The code uses str.strip(MSG_ID_PREFIX) but strip() removes all characters in the argument from both ends of the string, not a prefix. With MSG_ID_PREFIX = "web-", this removes any 'w', 'e', 'b', or '-' characters from both ends rather than just the "web-" prefix. Other places in the codebase correctly use .removeprefix() or .lstrip() for this purpose.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
bots/models/convo_msg.py (1)
600-650: Critical issues from previous review remain unfixed.The same critical None-handling and ID-stripping issues flagged in the previous review are still present:
- Line 634:
msg.saved_run.state.get("references")will raiseAttributeErrorifmsg.saved_runisNone(nullable field).- Line 646:
msg.saved_run.get_app_url()will raiseAttributeErrorifmsg.saved_runisNone.- Line 648:
msg.platform_msg_id.strip(MSG_ID_PREFIX)will raiseAttributeErrorifmsg.platform_msg_idisNone, and.strip()removes characters not prefixes, potentially mangling IDs.- Line 616:
audio = audios and audios[0]producesNoneorFalsewhen empty, inconsistent with other attachment fields that are lists.Apply this diff to fix the issues:
def db_msgs_to_api_json(msgs: list["Message"]) -> typing.Iterator[dict]: from daras_ai_v2.bots import parse_bot_html from routers.bots_api import MSG_ID_PREFIX for msg in msgs: msg: Message images = list( msg.attachments.filter( metadata__mime_type__startswith="image/" ).values_list("url", flat=True) ) audios = list( msg.attachments.filter( metadata__mime_type__startswith="audio/" ).values_list("url", flat=True) ) - audio = audios and audios[0] + audio = audios[0] if audios else None if msg.role == CHATML_ROLE_USER: # any document type other than audio/image documents = list( msg.attachments.exclude( Q(metadata__mime_type__startswith="image/") | Q(metadata__mime_type__startswith="audio/") ).values_list("url", flat=True) ) yield { "role": msg.role, "input_prompt": msg.display_content, "input_images": images, "input_audio": audio, "input_documents": documents, "created_at": msg.created_at.isoformat(), } elif msg.role == CHATML_ROLE_ASSISTANT: - references = msg.saved_run.state.get("references") + references = msg.saved_run.state.get("references") if msg.saved_run else None buttons, text, _ = parse_bot_html(msg.display_content) yield { "role": msg.role, "created_at": msg.created_at.isoformat(), "status": "completed", "type": "final_response", "raw_output_text": [msg.content], "output_text": [text], "buttons": buttons, "output_images": images, "output_audio": audio, - "web_url": msg.saved_run.get_app_url(), + "web_url": msg.saved_run.get_app_url() if msg.saved_run else None, "user_message_id": msg.platform_msg_id, - "bot_message_id": msg.platform_msg_id.strip(MSG_ID_PREFIX), + "bot_message_id": ( + msg.platform_msg_id.removeprefix(MSG_ID_PREFIX) + if msg.platform_msg_id + else None + ), "references": references, }routers/root.py (1)
549-565: Fix typo, add authorization check, and address privacy/exception handling.Multiple issues need attention:
Line 556 (typo from previous review): Variable misspelled as
mesasgesinstead ofmessages(also line 562).Critical security issue: No validation that the conversation belongs to this bot integration. Anyone with a
conversation_idcan view conversations from ANY bot by accessing them through a different bot's share URL. Add authorization check:if conversation.bot_integration_id != bi.id: raise HTTPException(status_code=404).Line 561 (privacy from previous review):
web_user_idis an internal tracking identifier exposed in public share URLs. Consider whether this should be included or replaced with a non-identifying placeholder.Line 555 (static analysis): Exception handling should use
raise ... from errorraise ... from Nonefor proper exception chaining.Apply this diff:
if conversation_id: try: conversation: Conversation = Conversation.objects.get( id=api_hashids.decode(conversation_id)[0], ) - except (IndexError, Conversation.DoesNotExist): + except (IndexError, Conversation.DoesNotExist) as e: - raise HTTPException(status_code=404) + raise HTTPException(status_code=404) from e + + # Validate conversation belongs to this bot integration + if conversation.bot_integration_id != bi.id: + raise HTTPException(status_code=404) + - mesasges = list(db_msgs_to_api_json(conversation.last_n_msgs())) + messages = list(db_msgs_to_api_json(conversation.last_n_msgs())) conversation_data = dict( id=conversation_id, bot_id=integration_id, timestamp=conversation.created_at.isoformat(), - user_id=conversation.web_user_id, + user_id=conversation.web_user_id, # TODO: Consider privacy implications - messages=mesasges, + messages=messages, ) else: conversation_data = None
🧹 Nitpick comments (1)
routers/root.py (1)
535-540: Unused parameter may be intentional for URL routing.Static analysis flags
integration_nameas unused in the function body. While the parameter isn't used in the logic, it's likely required in the URL path for human-readable routes (e.g.,/chat/my-bot-abc123/vs/chat/abc123/). If this is intentional, consider suppressing the warning or adding a comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bots/models/convo_msg.py(2 hunks)routers/root.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
bots/models/convo_msg.pyrouters/root.py
🧬 Code graph analysis (2)
bots/models/convo_msg.py (2)
daras_ai_v2/bots.py (1)
parse_bot_html(339-368)bots/models/saved_run.py (1)
get_app_url(172-178)
routers/root.py (1)
bots/models/convo_msg.py (4)
Conversation(124-336)db_msgs_to_api_json(600-650)last_n_msgs(330-331)last_n_msgs(456-462)
🪛 Ruff (0.14.8)
routers/root.py
538-538: Unused function argument: integration_name
(ARG001)
555-555: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ 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: Cursor Bugbot
- GitHub Check: Analyze (python)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (4)
bots/models/convo_msg.py (1)
12-12: LGTM: Import addition supports new functionality.The
Qimport is correctly used in the newdb_msgs_to_api_jsonfunction for building complex query filters.routers/root.py (3)
27-30: LGTM: Imports support new conversation-sharing feature.The
Conversationmodel anddb_msgs_to_api_jsonfunction are correctly imported for the new share route functionality.
534-534: LGTM: Share route pattern is consistent.The new route path
/chat/{integration_name}-{integration_id}/share/{conversation_id}/follows existing URL conventions and enables conversation sharing.
572-579: LGTM: Widget configuration correctly enables sharing.The
get_web_widget_embed_codecall properly passesconversationDatawhen present, enabling the share conversation feature in fullscreen mode.
- make new method db_msgs_to_api_json - fetch attachments and append according to message role - send as conversationData in web widget config - redirect to base /chat url if no ceoonverstion found
anish-work
force-pushed
the
feature/share-web-widget-conversations
branch
from
5e7fab8 to
e2411ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
bots/models/convo_msg.py (1)
12-12: Fix None-handling, ID prefix stripping, and audio typing indb_msgs_to_api_jsonThe new serializer is close, but there are some correctness and robustness issues:
saved_runis nullable, but you access it unconditionally:
- Line 634:
references = msg.saved_run.state.get("references")- Line 646:
web_url = msg.saved_run.get_app_url()If an assistant
Messagehassaved_run=None, both lines will raiseAttributeError. Please guard:@@ - elif msg.role == CHATML_ROLE_ASSISTANT: - references = msg.saved_run.state.get("references") + elif msg.role == CHATML_ROLE_ASSISTANT: + saved_run = msg.saved_run + references = saved_run.state.get("references") if saved_run else None @@ - "output_audio": audio, - "web_url": msg.saved_run.get_app_url(), - "user_message_id": msg.platform_msg_id, - "bot_message_id": msg.platform_msg_id.strip(MSG_ID_PREFIX), + "output_audio": audio, + "web_url": saved_run.get_app_url() if saved_run else None, + "user_message_id": msg.platform_msg_id, + "bot_message_id": ( + msg.platform_msg_id + and msg.platform_msg_id.removeprefix(MSG_ID_PREFIX) + ),
platform_msg_idis nullable and.strip()is the wrong operation:
- If
msg.platform_msg_idisNone,msg.platform_msg_id.strip(...)(line 648) raises.strip(MSG_ID_PREFIX)removes any of the characters from both ends, not the literal"web-"prefix, so IDs can be silently mangled.Switch to guarded
removeprefixas inMessageQuerySet.to_json(see lines 433–436), as shown above.
- Audio handling is subtly type-unstable:
audio = audios and audios[0](line 616) yields:
- a URL string when there is audio,
- but
[]when there is none.- This means
input_audio/output_audiocan be eitherstrorlist, unlike the other attachment fields.Consider making the fallback explicit and stable, e.g.:
- audios = list( + audios = list( msg.attachments.filter( metadata__mime_type__startswith="audio/" ).values_list("url", flat=True) ) - audio = audios and audios[0] + audio = audios[0] if audios else Noneor, if the API expects lists for audio as well, use
audiosconsistently for both input and output.
- Optional perf improvement:
last_n_msgs()already usesprefetch_related("attachments"); callingmsg.attachments.filter(...)/.exclude(...)for each message will still hit the DB. For better performance, you could use the prefetchedmsg.attachments.all()and filter in Python, especially if many messages are serialized at once.To confirm the
stripvsremoveprefixbehavior and theaudios and audios[0]edge case, you can run:# strip vs removeprefix s = "web-abc" print(s.strip("web-")) # removes any of 'w','e','b','-' from both ends print(s.removeprefix("web-")) # removes literal prefix only # audio expression audios = [] print(audios and audios[0]) # [] audios = ["url"] print(audios and audios[0]) # "url"Also applies to: 600-651
routers/root.py (2)
559-565: Scope shared conversation lookup to the current bot integrationRight now the shared
Conversationis fetched only by its ID:conversation: Conversation = Conversation.objects.get( id=api_hashids.decode(conversation_id)[0], )This does not verify that the conversation belongs to the
bi(bot integration) resolved earlier, so a crafted URL could mix an integration’s slug/ID with an unrelated conversation’s ID and potentially expose another integration’s data.Filter by
bot_integrationas well:- try: - conversation: Conversation = Conversation.objects.get( - id=api_hashids.decode(conversation_id)[0], - ) + try: + conversation: Conversation = Conversation.objects.get( + id=api_hashids.decode(conversation_id)[0], + bot_integration=bi, + )This keeps shared conversations confined to their owning integration.
You can verify there are no other unconstrained
Conversation.objects.get(id=...)lookups tied to integrations with:#!/bin/bash rg -n "Conversation\.objects\.get\(" -C2
567-572: Avoid exposingweb_user_idin publicconversationDataThe share route is publicly accessible, but
conversation_dataincludes:conversation_data = dict( id=conversation_id, bot_id=integration_id, timestamp=conversation.created_at.isoformat(), user_id=conversation.web_user_id, messages=mesasges, )
web_user_idis an internal, per-user identifier onConversationand may tie the shared transcript back to a specific user account. For a public share link this is likely unnecessary and a privacy risk.Consider either:
- Omitting the field entirely, or
- Replacing it with a non-identifying placeholder (e.g., a random or hashed share ID), only if the frontend truly needs a stable identifier.
Example minimal change:
- timestamp=conversation.created_at.isoformat(), - user_id=conversation.web_user_id, - messages=mesasges, + timestamp=conversation.created_at.isoformat(), + messages=mesasges,If you’re unsure whether
user_idis required on the frontend, search for its usage:#!/bin/bash rg -n "\"user_id\"" -S
🧹 Nitpick comments (1)
routers/root.py (1)
545-549: Fix typo inmesasgesand address unused parameterThree smaller improvements worth making:
- Typo in variable name (line 566):
- mesasges = list(db_msgs_to_api_json(conversation.last_n_msgs())) + messages = list(db_msgs_to_api_json(conversation.last_n_msgs()))Also update the reference on line 571 from
messages=mesasges,tomessages=messages,.
- Unused
integration_nameparameter (line 545):The
integration_nameparameter is accepted but never used. To satisfy linters, either use it or rename to_integration_name:- integration_name: str | None = None, + _integration_name: str | None = None,
- Optional: explicit exception chaining (lines 555 and 567):
Add
from Noneto distinguish intentional exception suppression from implicit chaining:- except (IndexError, BotIntegration.DoesNotExist): - raise HTTPException(status_code=404) + except (IndexError, BotIntegration.DoesNotExist): + raise HTTPException(status_code=404) from NoneApply the same pattern to the
Conversation.DoesNotExisthandler.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bots/models/convo_msg.py(2 hunks)routers/root.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
bots/models/convo_msg.pyrouters/root.py
🧬 Code graph analysis (2)
bots/models/convo_msg.py (2)
daras_ai_v2/bots.py (1)
parse_bot_html(339-368)bots/models/saved_run.py (1)
get_app_url(172-178)
routers/root.py (2)
bots/models/convo_msg.py (4)
Conversation(124-336)db_msgs_to_api_json(600-650)last_n_msgs(330-331)last_n_msgs(456-462)daras_ai_v2/bot_integration_widgets.py (1)
get_web_widget_embed_code(364-385)
🪛 Ruff (0.14.8)
routers/root.py
548-548: Unused function argument: integration_name
(ARG001)
565-565: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ 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). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (1)
routers/root.py (1)
28-31: New imports frombots.models.convo_msglook goodImporting
Conversationanddb_msgs_to_api_jsonhere is appropriate and keeps the share-route logic local toroot.py. No issues from a dependency or import-time perspective.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
routers/root.py (1)
559-573: Critical: Add bot_integration filter to prevent IDOR vulnerability; fix typo.The conversation lookup fetches by ID only without verifying it belongs to the specified bot integration. An attacker could access conversations from other integrations by using a valid
conversation_idfrom a differentintegration_id.Additionally,
mesasgesshould bemessages.Apply this diff to fix both issues:
if conversation_id: try: conversation: Conversation = Conversation.objects.get( id=api_hashids.decode(conversation_id)[0], + bot_integration=bi, ) except (IndexError, Conversation.DoesNotExist): raise HTTPException(status_code=404) - mesasges = list(db_msgs_to_api_json(conversation.last_n_msgs())) + messages = list(db_msgs_to_api_json(conversation.last_n_msgs())) conversation_data = dict( id=conversation_id, bot_id=integration_id, timestamp=conversation.created_at.isoformat(), user_id=conversation.web_user_id, - messages=mesasges, + messages=messages, )
🧹 Nitpick comments (1)
routers/root.py (1)
564-565: Useraise ... from Nonefor proper exception chaining.Per static analysis hint, distinguish the 404 from an error in exception handling.
except (IndexError, Conversation.DoesNotExist): - raise HTTPException(status_code=404) + raise HTTPException(status_code=404) from None
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bots/models/convo_msg.py(2 hunks)daras_ai_v2/bot_integration_widgets.py(2 hunks)routers/root.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bots/models/convo_msg.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
daras_ai_v2/bot_integration_widgets.pyrouters/root.py
🧬 Code graph analysis (1)
routers/root.py (2)
bots/models/convo_msg.py (3)
Conversation(124-336)last_n_msgs(330-331)last_n_msgs(456-462)daras_ai_v2/bot_integration_widgets.py (1)
get_web_widget_embed_code(364-385)
🪛 Ruff (0.14.8)
routers/root.py
548-548: Unused function argument: integration_name
(ARG001)
565-565: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ 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). (2)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (5)
daras_ai_v2/bot_integration_widgets.py (1)
454-476: LGTM!The
enableShareConversationfeature flag and UI checkbox follow the established pattern for other widget configuration options. The default ofFalseis a safe choice for a new sharing feature.routers/root.py (4)
28-31: LGTM!The new imports are correctly scoped for the share route functionality.
543-550: LGTM!The route pattern and signature updates are appropriate. The
integration_nameparameter, while flagged as unused by static analysis, serves a valid purpose in the URL structure for readability and is consistent with the existing pattern on line 543.
571-571: Verify: Shouldweb_user_idbe exposed in shared conversations?The
web_user_idis an internal user tracking identifier. Including it in the public share response reveals who initiated the conversation. Consider whether this aligns with privacy expectations for shared conversations, or if it should be omitted/anonymized.
582-588: LGTM!The embed code generation correctly passes
mode="fullscreen"and includesconversationDataonly when a conversation is being shared. The fallback toNonepreserves existing behavior.
Q/A checklist
How to check import time?
You can visualize this using tuna:
To measure import time for a specific library:
To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:
Legal Boilerplate
Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.
Note
Adds a shareable chat route that loads a conversation and prepopulates the web widget with serialized messages (including media, buttons, and references).
GET /chat/{integration_name}-{integration_id}/share/{conversation_id}/to load a shared conversation.enableShareConversationandconversationDatato web widget viaget_web_widget_embed_codeinrouters/root.py.db_msgs_to_api_jsoninbots/models/convo_msg.pyto serializeMessages into API JSON withinput_images/audio/documentsfor users andoutput_text/images/audio,buttons,references, IDs for assistants.Conversationanddb_msgs_to_api_jsoninrouters/root.py; optional typing for route params; addQimport for attachment filtering.Written by Cursor Bugbot for commit 5e7fab8. This will update automatically on new commits. Configure here.