fix: QR code URL generation and WebSocket robustness #1527
Conversation
- Fix: Ensure defaults to if not explicitly set, fixing incorrect QR codes (localhost). - Fix: Determine dynamically from environment variables in production. - Refactor: Add in to prevent WebSocket consumer crashes when the video server is unconfigured (common in dev/partial deployments).
Reviewer's GuideAdds defensive handling around BigBlueButton (BBB) interactions to prevent WebSocket crashes when BBB is not configured, and updates settings so QR codes use the configured site URL as the default short URL instead of always localhost. Sequence diagram for BBB WebSocket error handling and URL generationsequenceDiagram
actor WebClient
participant WebSocketConsumer
participant BBBModule
participant BBBService
WebClient->>WebSocketConsumer: send room_url request
WebSocketConsumer->>BBBModule: room_url(body)
BBBModule->>BBBService: get_join_url_for_room(room, user, moderator)
BBBService-->>BBBModule: IntegrityError
BBBModule->>WebSocketConsumer: raise ConsumerException bbb.failed
WebSocketConsumer-->>WebClient: error response bbb.failed
WebClient->>WebSocketConsumer: send call_url request
WebSocketConsumer->>BBBModule: call_url(body)
BBBModule->>BBBService: get_join_url_for_call_id(call_id, user)
BBBService-->>BBBModule: IntegrityError
BBBModule->>WebSocketConsumer: raise ConsumerException bbb.failed
WebSocketConsumer-->>WebClient: error response bbb.failed
WebClient->>WebSocketConsumer: send recordings request
WebSocketConsumer->>BBBModule: recordings(body)
BBBModule->>BBBService: get_recordings_for_room(room)
BBBService-->>BBBModule: IntegrityError
BBBModule-->>WebSocketConsumer: recordings = []
WebSocketConsumer-->>WebClient: success response results = []
Class diagram for updated BaseSettings and short_url defaultingclassDiagram
class BaseSettings {
HttpUrl site_url
HttpUrl short_url
str talk_hostname
str sentry_dsn
str instance_name
str zoom_key
str zoom_secret
str control_secret
str statsd_host
int statsd_port
str statsd_prefix
BaseSettings default_short_url()
}
class _BaseSettings {
}
BaseSettings --|> _BaseSettings
class default_short_url {
+BaseSettings __call__(BaseSettings self)
}
BaseSettings ..> default_short_url
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new IntegrityError handling in the BBB handlers is very broad; consider catching a BBB-specific exception or at least logging the IntegrityError so genuine data issues in production don’t get silently converted into generic
bbb.failedresponses. - In the
recordingshandler, swallowing IntegrityError and returning an empty list changes the semantics compared to the other handlers that raisebbb.failed; it might be clearer to surface a controlled error instead of silently returning no recordings. - There appears to be an indentation issue in the
except IntegrityError:block forrecordings(recordings = []has an extra leading space) which could cause linting or readability problems; align this line with the rest of the block.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new IntegrityError handling in the BBB handlers is very broad; consider catching a BBB-specific exception or at least logging the IntegrityError so genuine data issues in production don’t get silently converted into generic `bbb.failed` responses.
- In the `recordings` handler, swallowing IntegrityError and returning an empty list changes the semantics compared to the other handlers that raise `bbb.failed`; it might be clearer to surface a controlled error instead of silently returning no recordings.
- There appears to be an indentation issue in the `except IntegrityError:` block for `recordings` (`recordings = []` has an extra leading space) which could cause linting or readability problems; align this line with the rest of the block.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR addresses two distinct issues: fixing QR code URL generation to use the configured site domain instead of localhost, and improving WebSocket stability when BigBlueButton servers are not configured.
Key Changes:
- Modified settings to make
short_urloptional and default tosite_urlvia a model validator - Added exception handling in BBB WebSocket handlers to prevent crashes when no BBB server is configured
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/eventyay/config/next_settings.py | Changed short_url to optional (None allowed) and added a model validator that defaults it to site_url when not explicitly set |
| app/eventyay/features/live/modules/bbb.py | Wrapped BBB service calls in try-except blocks to catch IntegrityError and gracefully handle missing BBB server configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.room, | ||
| ) | ||
| except IntegrityError: | ||
| recordings = [] |
There was a problem hiding this comment.
There is an extra space before 'recordings' causing incorrect indentation. This line should align with the 'except' block above it.
| recordings = [] | |
| recordings = [] |
| except IntegrityError: | ||
| # This happens in dev environment when no BBB server is configured | ||
| raise ConsumerException("bbb.failed") |
There was a problem hiding this comment.
Catching IntegrityError here is too broad and may mask other database integrity issues. The actual problem occurs when choose_server() returns None (no BBB servers configured), which causes the BBBCall.server foreign key constraint to fail. Consider catching a more specific exception or checking if server is None in the get_create_params_for_room function instead.
| try: | ||
| url = await service.get_join_url_for_call_id( | ||
| body.get("call"), | ||
| self.consumer.user, | ||
| ) | ||
| except IntegrityError: | ||
| raise ConsumerException("bbb.failed") |
There was a problem hiding this comment.
Catching IntegrityError here is too broad and may mask other database integrity issues. The actual problem occurs when choose_server() returns None (no BBB servers configured), which causes the BBBCall.server foreign key constraint to fail. Consider catching a more specific exception or checking if server is None in the get_create_params_for_call_id function instead.
| try: | |
| url = await service.get_join_url_for_call_id( | |
| body.get("call"), | |
| self.consumer.user, | |
| ) | |
| except IntegrityError: | |
| raise ConsumerException("bbb.failed") | |
| url = await service.get_join_url_for_call_id( | |
| body.get("call"), | |
| self.consumer.user, | |
| ) |
| try: | ||
| recordings = await service.get_recordings_for_room( | ||
| self.room, | ||
| ) | ||
| except IntegrityError: | ||
| recordings = [] |
There was a problem hiding this comment.
The recordings method is unlikely to raise IntegrityError since it doesn't create or update any BBBCall records - it only queries for recordings. The get_recordings_for_room method already has comprehensive exception handling that returns an empty list on errors. This try-except block appears unnecessary and may give a false sense of error handling.
| try: | |
| recordings = await service.get_recordings_for_room( | |
| self.room, | |
| ) | |
| except IntegrityError: | |
| recordings = [] | |
| recordings = await service.get_recordings_for_room( | |
| self.room, | |
| ) |
app/eventyay/config/next_settings.py
Outdated
| @model_validator(mode='after') | ||
| def default_short_url(self) -> 'BaseSettings': | ||
| if self.short_url is None: | ||
| self.short_url = self.site_url | ||
| return self |
There was a problem hiding this comment.
The model_validator is placed in the middle of field definitions, between control_secret and statsd_host. For better code organization and readability, consider moving all validators to the end of the class after all field definitions, or grouping them with related fields.
|
@ArnavBallinCode hey any changes needed? |
hongquan
left a comment
hongquan
left a comment
There was a problem hiding this comment.
Please address the Copilot review.
I don't want the short_url to accept None. If you find that the cause of this bug is that people forgot to set short_url, then just provide the default value in the *.toml files.
…e error handling by removing IntegrityError catches.
|
@hongquan I've ensured short_url no longer accepts None. I removed the validator and instead explicitly added short_url to both eventyay.development.toml and eventyay.production.toml with appropriate default values. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
app/eventyay/config/next_settings.py:16
- Import of 'model_validator' is not used.
from pydantic import Field, HttpUrl, model_validator
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from kombu import Queue | ||
| from pycountry import currencies | ||
| from pydantic import Field, HttpUrl | ||
| from pydantic import Field, HttpUrl, model_validator |
There was a problem hiding this comment.
The model_validator import is not used anywhere in this file. According to the PR description, a model validator should be implemented to automatically default short_url to site_url when not explicitly configured, but no such validator has been added. Either implement the validator or remove this unused import.
| from pydantic import Field, HttpUrl, model_validator | |
| from pydantic import Field, HttpUrl |
|
@mariobehling any changes needed? or it'll be closed? |
3 participants
I fixed an issue where the QR code for rooms was always showing localhost instead of the actual site URL. I updated the settings logic so it now correctly uses the configured domain (like test.eventyay.com) if a short URL isn't explicitly set.
Also, I found that if the BigBlueButton server isn't set up (which happens often in local dev), the whole WebSocket connection would crash. This side effect was causing other features, like the QR code generation, to fail or load infinitely. I added a check to handle this error gracefully so the app stays stable even without a video server.
Result:
QR codes now point to the correct domain instead of localhost.
The app no longer crashes/freezes when checking for the video server.
Screenshots:
Before:
After:
#1526
Summary by Sourcery
Handle missing BigBlueButton configuration more gracefully and align QR code short URL behavior with the primary site URL configuration.
Bug Fixes:
Enhancements: