-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: nullable exception handlers #5703
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: nullable exception handlers #5703
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR introduces nullable exception handlers for both frontend and backend error handling in the Reflex framework. The changes allow users to explicitly set exception handlers to None
to disable custom exception handling, which is particularly useful for Kubernetes liveness and readiness probes where standard HTTP error responses are preferred over custom error handling logic.
The implementation modifies two core files:
In reflex/app.py
:
- Updates type annotations for
frontend_exception_handler
andbackend_exception_handler
to include| None
- Modifies validation logic to skip validation when handlers are
None
- Adds a null check before executing the backend exception handler
In reflex/state.py
:
- Adds null checks in the
_process_event
method before callingbackend_exception_handler
- Adds null checks in the
handle_frontend_exception
method before callingfrontend_exception_handler
The change maintains full backward compatibility since the default handlers are still provided, but adds flexibility for cloud-native deployments where infrastructure monitoring systems need predictable error responses. This fits into the broader Reflex architecture by extending the existing exception handling system without breaking existing functionality, allowing apps to have optional exception handlers that can be conditionally disabled based on deployment context.
Confidence score: 4/5
- This PR is safe to merge with minimal risk as it maintains backward compatibility while adding useful functionality
- Score reflects well-structured null safety checks and non-breaking changes to existing exception handling
- Pay close attention to the validation logic in
reflex/app.py
to ensure the skip condition works correctly
2 files reviewed, no comments
CodSpeed Performance ReportMerging #5703 will not alter performanceComparing Summary
|
@masenf @benedikt-bartscher ready for review |
reflex/state.py
Outdated
event_specs = ( | ||
prerequisites.get_and_validate_app().app.backend_exception_handler(ex) | ||
app.backend_exception_handler(ex) | ||
if app.backend_exception_handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be a tad better to check with is not None
just in case a Var might come here, otherwise looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adhami3310 thx, just fixed it
the only issue is that this doesn't disable the frontend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only issue is that this doesn't disable the frontend
onError
from triggering, it just that when it goes to the backend, it does nothing
That was the original intention/idea in #3776. Can you adjust it @maximvlah?
onError would still trigger, just the backend will do the equiv of lambda _: None
, so nothing would meaningfully change for that
it's quite difficult to pipe the logic through the default error handler :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fine to me, are we wanting to disable the entire frontend exception handling mechanism if the handler is None? because that's definitely more work
i think to truly resolve the associated ticket, we do want to actually disable onError |
Closes #3776