Conversation
…n for beforeSend and beforeBreadcrumb return values
There was a problem hiding this comment.
Pull request overview
This PR hardens the JavaScript SDK hook handling so beforeSend (and similarly beforeBreadcrumb) can’t accidentally cause invalid payloads/breadcrumbs to be sent/stored when the hook returns an unexpected value.
Changes:
- Add runtime validators (
isValidEventPayload,isValidBreadcrumb) and apply them to hook results. - Update hook typings/docs to allow
voidreturn (keep original) and usefalseto explicitly drop events/breadcrumbs. - Add/adjust examples and docs to demonstrate the new hook behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/javascript/src/utils/validation.ts | Adds runtime validation helpers for event payloads and breadcrumbs. |
| packages/javascript/src/types/hawk-initial-settings.ts | Updates beforeSend typing/docs to include void return behavior. |
| packages/javascript/src/catcher.ts | Validates beforeSend return value before overwriting the outgoing payload; adds warnings for invalid returns. |
| packages/javascript/src/addons/breadcrumbs.ts | Validates beforeBreadcrumb return value; updates hook behavior/docs and warnings. |
| packages/javascript/package.json | Bumps package version to 3.2.16. |
| packages/javascript/example/index.html | Updates example to use false (instead of null) for discarding breadcrumbs. |
| packages/javascript/example/hooks-tests.html | Adds a manual test page for hook scenarios. |
| packages/javascript/example/before-send-tests.js | Adds manual test script to exercise hook return cases. |
| packages/javascript/README.md | Updates public docs for hook return semantics and examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return false; | ||
| } | ||
|
|
||
| if (payload.backtrace !== undefined && !Array.isArray(payload.backtrace)) { |
There was a problem hiding this comment.
isValidEventPayload treats backtrace: null as invalid because it only allows an array when the field is present. In this SDK, HawkJavaScriptEvent['backtrace'] is BacktraceFrame[] | null, so a payload returned from beforeSend that keeps backtrace as null will be rejected and silently ignored. Update the check to allow null (and/or align the runtime validation with HawkJavaScriptEvent’s actual shape).
| if (payload.backtrace !== undefined && !Array.isArray(payload.backtrace)) { | |
| if (payload.backtrace !== undefined && payload.backtrace !== null && !Array.isArray(payload.backtrace)) { |
| * @param breadcrumb - value to validate | ||
| */ | ||
| export function isValidBreadcrumb(breadcrumb: unknown): breadcrumb is Breadcrumb { | ||
| if (typeof breadcrumb !== 'object' || breadcrumb === null || Array.isArray(breadcrumb)) { |
There was a problem hiding this comment.
The JSDoc says the breadcrumb must be a plain object, but isValidBreadcrumb currently accepts any non-null, non-array object (e.g. new Date()), which doesn’t match the documented behavior. Either tighten the check (e.g. reuse isPlainObject) or adjust the comment to reflect what’s actually validated.
| if (typeof breadcrumb !== 'object' || breadcrumb === null || Array.isArray(breadcrumb)) { | |
| if (!isPlainObject(breadcrumb)) { |
| * - Return nothing (`void` / `undefined` / `null`) — the original event is sent as-is (a warning is logged). | ||
| */ | ||
| private readonly beforeSend: undefined | ((event: HawkJavaScriptEvent) => HawkJavaScriptEvent | false); | ||
| private readonly beforeSend: undefined | ((event: HawkJavaScriptEvent) => HawkJavaScriptEvent | false | void); |
There was a problem hiding this comment.
The comment mentions that beforeSend may return null, and the runtime code handles null, but the TypeScript type doesn’t include null (HawkJavaScriptEvent | false | void). Either include null in the return type or remove null from the docs/handling so the public API is consistent.
| private readonly beforeSend: undefined | ((event: HawkJavaScriptEvent) => HawkJavaScriptEvent | false | void); | |
| private readonly beforeSend: undefined | ((event: HawkJavaScriptEvent) => HawkJavaScriptEvent | false | void | null); |
packages/javascript/src/catcher.ts
Outdated
| * If user returned nothing (void/undefined/null) — warn and keep original payload | ||
| */ | ||
| if (result === undefined || result === null) { | ||
| log(`[Hawk] Invalid beforeSend value: (${String(result)}). It should return event or false. Event is sent without changes.`, 'warn'); |
There was a problem hiding this comment.
The warning says the event is sent “without changes”, but beforeSend receives the actual payload object—any in-place mutations done before returning undefined/null will still be sent. To make this accurate, either clone/freeze the payload before calling beforeSend (and only use the clone when the hook returns a valid event) or reword the warning to not promise that no changes were applied.
| * If user returned nothing (void/undefined/null) — warn and keep original payload | |
| */ | |
| if (result === undefined || result === null) { | |
| log(`[Hawk] Invalid beforeSend value: (${String(result)}). It should return event or false. Event is sent without changes.`, 'warn'); | |
| * If user returned nothing (void/undefined/null) — warn and send current payload as is | |
| */ | |
| if (result === undefined || result === null) { | |
| log(`[Hawk] Invalid beforeSend value: (${String(result)}). It should return event or false. Event will be sent as is.`, 'warn'); |
| /** | ||
| * false means discard | ||
| */ | ||
| if (result === false) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
beforeBreadcrumb previously used null to discard breadcrumbs, but this change switches the sentinel to false and now treats null as “no return” (stores the breadcrumb). That’s a breaking behavior change for existing consumers; consider continuing to treat null as discard (possibly with a deprecation warning) or bump the package with an appropriate breaking-change version.
| * - If the hook returns an invalid value, a warning is logged and the original breadcrumb is stored. | ||
| */ | ||
| beforeBreadcrumb?: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => Breadcrumb | null; | ||
| beforeBreadcrumb?: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => Breadcrumb | false | void; |
There was a problem hiding this comment.
The JSDoc says beforeBreadcrumb can return null, and the runtime path handles null, but the callback type is Breadcrumb | false | void (no null). Align the public type signature with the documented/runtime behavior (either add null or remove null from docs and handling).
| beforeBreadcrumb?: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => Breadcrumb | false | void; | |
| beforeBreadcrumb?: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => Breadcrumb | false | null | void; |
| * void/undefined/null — warn and keep original breadcrumb | ||
| */ | ||
| if (result === undefined || result === null) { | ||
| log('[Hawk] beforeBreadcrumb returned nothing, storing original breadcrumb.', 'warn'); | ||
| } else if (isValidBreadcrumb(result)) { | ||
| Object.assign(bc, result); | ||
| } else { | ||
| log( | ||
| '[Hawk] beforeBreadcrumb produced invalid breadcrumb (must be an object with numeric timestamp), storing original. ' |
There was a problem hiding this comment.
The log message says it’s “storing original breadcrumb”, but beforeBreadcrumb is called with the live bc object, so in-place mutations will still be stored even when the hook returns undefined/null or an invalid value. Consider cloning bc before invoking the hook or reword the warning to avoid implying the breadcrumb is unchanged.
| * void/undefined/null — warn and keep original breadcrumb | |
| */ | |
| if (result === undefined || result === null) { | |
| log('[Hawk] beforeBreadcrumb returned nothing, storing original breadcrumb.', 'warn'); | |
| } else if (isValidBreadcrumb(result)) { | |
| Object.assign(bc, result); | |
| } else { | |
| log( | |
| '[Hawk] beforeBreadcrumb produced invalid breadcrumb (must be an object with numeric timestamp), storing original. ' | |
| * void/undefined/null — warn and keep breadcrumb as last modified by hook | |
| */ | |
| if (result === undefined || result === null) { | |
| log('[Hawk] beforeBreadcrumb returned nothing; keeping breadcrumb as last modified by hook.', 'warn'); | |
| } else if (isValidBreadcrumb(result)) { | |
| Object.assign(bc, result); | |
| } else { | |
| log( | |
| '[Hawk] beforeBreadcrumb produced invalid breadcrumb (must be an object with numeric timestamp); keeping breadcrumb as last modified by hook. ' |
| * - Return nothing (`void` / `undefined` / `null`) — the original event is sent as-is (a warning is logged). | ||
| */ | ||
| beforeSend?(event: HawkJavaScriptEvent): HawkJavaScriptEvent | false; | ||
| beforeSend?(event: HawkJavaScriptEvent): HawkJavaScriptEvent | false | void; |
There was a problem hiding this comment.
beforeSend docs mention null as a possible “no return” value, but the declared type is HawkJavaScriptEvent | false | void (no null). If null is intended to be treated like undefined, add null to the signature; otherwise remove it from the docs to keep the API contract consistent.
| beforeSend?(event: HawkJavaScriptEvent): HawkJavaScriptEvent | false | void; | |
| beforeSend?(event: HawkJavaScriptEvent): HawkJavaScriptEvent | false | void | null; |
… validation improvements
…y using clones for payload modifications
… renaming variables for clarity
…ty in beforeSend tests
Problem
When
beforeSendreturns a non-object (e.g.true,undefined, or omitsreturn), that value was used aspayloadand sent to the collector. The backend then stored it as-is (e.g.payload: truein MongoDB), which led to:Cannot return null for non-nullable field EventPayload.titleTypeError: can't access property "event", l is nullon project overviewCommon mistakes:
beforeSend: (e) => true(meant “allow”, but overwrites payload)beforeSend: (e) => { console.log(e); }(no return →undefinedas payload)Solution
Validate the return value of
beforeSendand only use it when it’s a valid event object: