feat(core): Add safeCallback helper for isolating user-provided callbacks - #23760
feat(core): Add safeCallback helper for isolating user-provided callbacks#23760msonnb wants to merge 1 commit into
safeCallback helper for isolating user-provided callbacks#23760Conversation
size-limit report 📦
|
|
bugbot run |
msonnb
changed the title
safeCallback helper for isolating user-provided callbackssafeCallback helper for isolating user-provided callbacks
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit bb9a102. Configure here.
| return modifiedSpan; | ||
| } | ||
|
|
||
| if (!hasShownSpanDropWarning) { |
There was a problem hiding this comment.
unrelated to this PR, but we can possibly remove this warning (safe some bytes), this has been this way for some time 🤔 or at least make it a debug.warn gated by the debug flag so you can shake it out?
There was a problem hiding this comment.
yeah i think we can remove it tbh. has been like this since v9 and the type doesn't allow it anyway.
…backs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bb9a102 to
9792842
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9792842. Configure here.
|
|
||
| function recover<T>(message: string, error: unknown, fallback: (error: unknown) => T): T { | ||
| DEBUG_BUILD && debug.error(message, error); | ||
| return fallback(error); |
There was a problem hiding this comment.
Debug strings skip tree-shaking gate
Low Severity
Flagged this because the review rules require DEBUG_BUILD && debug.error(...) so message text can be tree-shaken from production browser bundles. safeCallback takes message as a live argument, so the beforeSendSpan error string is no longer behind that gate and now ships in production.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 9792842. Configure here.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Had one follow-up suggestion for better tree-shaking but otherwise looks good!
| } | ||
|
|
||
| function recover<T>(message: string, error: unknown, fallback: (error: unknown) => T): T { | ||
| DEBUG_BUILD && debug.error(message, error); |
There was a problem hiding this comment.
I think the flag is correct here but the bot review is right: We no longer tree-shake out the actual string passed to this function if DEBUG_BUILD is false. We can still add a ternary to the call-sitest to enable tree shaking, something like
safeCallback(DEBUG_BUILD ? 'full warning/error message' : '', () => {}, () => {})3 participants
Adds
safeCallback(message, fn, fallback)helper which runs a user-provided callback, and on a sync throw or async rejection logsmessageviadebug.errorand returnsfallback(error)instead of propagating.applyBeforeSendSpanCallbackand the undicisafeExecuteare refactored onto the helper so there is a single implementation.First step of #23755, next PR will wrap the rest of the user-defined callbacks.