Add option to display error tracebacks inline and in modal#8376
Add option to display error tracebacks inline and in modal#8376wlach wants to merge 2 commits intomarimo-team:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
akshayka
left a comment
There was a problem hiding this comment.
A couple comments on the API
| help="Custom asset URL for loading static resources. Can include {version} placeholder.", | ||
| ) | ||
| @click.option( | ||
| "--show-error-tracebacks/--no-show-error-tracebacks", |
There was a problem hiding this comment.
| "--show-error-tracebacks/--no-show-error-tracebacks", | |
| "--show-tracebacks/--no-show-tracebacks", |
| - `show_error_tracebacks`: if `True`, show detailed error tracebacks in run mode. | ||
| When enabled, exceptions will display a clickable toast that opens a modal with the full traceback. | ||
| The default is `True`. |
There was a problem hiding this comment.
Does this need to be in config? This isn't really a runtime configuration in my mind. The console redirection is also not in config
manzt
left a comment
There was a problem hiding this comment.
Thanks for pushing this forward.
I'm convinced separating console redirection from tracebacks is the right call. They serve different purposes (as you mentioned), so keeping them independent makes sense.
+1 to removing from config. redirect_console_to_browser is CLI/ASGI-only and this should follow the same pattern. Keeping it out of config also gives us room to easily evolve the API later (if ever):
--show-tracebacks # bare flag, sensible default
--show-tracebacks=toast
--show-tracebacks=inlineNot needed in this PR, just flagging that staying out of config preserves that optionality.
| const exceptionErrors = cell.output.data.filter( | ||
| (error: any) => error.type === "exception" || error.type === "internal", | ||
| ); |
There was a problem hiding this comment.
I believe we can remove the explicit any with type narrowing:
| const exceptionErrors = cell.output.data.filter( | |
| (error: any) => error.type === "exception" || error.type === "internal", | |
| ); | |
| const exceptionErrors = cell.output.data.filter( | |
| (error) => | |
| "type" in error && | |
| (error.type === "exception" || error.type === "internal"), | |
| ); |
|
|
||
| // Find first error with a traceback | ||
| const errorWithTraceback = exceptionErrors.find( | ||
| (error: any) => error.traceback, |
There was a problem hiding this comment.
I think with changes above this can be removed (type is inferred).
| const errorWithTraceback = exceptionErrors.find( | ||
| (error: any) => error.traceback, | ||
| ); |
There was a problem hiding this comment.
I think with changes above this can be removed (type is inferred).
| const errorWithTraceback = exceptionErrors.find( | |
| (error: any) => error.traceback, | |
| ); | |
| const errorWithTraceback = exceptionErrors.find( | |
| (error) => error.traceback, | |
| ); |
|
Thanks all! Very reasonable requests for changes, I'll try to find some time to polish this up next week. |
📝 Summary
Closes #7984
🔍 Description of Changes
Add an optional mode (disabled by default) to display error tracebacks both inline and in a modal when in run (app) mode. This makes it easier for users to report errors in internal environments.
📋 Checklist