Skip to content

fix(replay): Only call scope.getLastBreadcrumb if available #6969

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 30, 2023
Merged

fix(replay): Only call scope.getLastBreadcrumb if available #6969

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6a58e3
fix(replay): Only use `scope.getLastBreadcrumb` if
Lms24 Jan 30, 2023
2690545
rewrite guard and add comment
Lms24 Jan 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/replay/src/coreHandlers/handleScope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ export const handleScopeListener: (replay: ReplayContainer) => (scope: Scope) =>
* An event handler to handle scope changes.
*/
export function handleScope(scope: Scope): Breadcrumb | null {
if (typeof scope.getLastBreadcrumb !== 'function') {
Copy link

@m-nathani m-nathani Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍🏼

I wish i would have created this PR before 🤦🏼 🥼

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this methods signature, scope is always defined; so no need to guard against it being undefined ;)
Furthermore, this is where we call the function that ends up calling handleScope. As you can see, it is guarded by the scope being defined there:

sentry-javascript/packages/replay/src/util/addGlobalListeners.ts

Line 19 in a6a58e3

scope.addScopeListener(handleScopeListener(replay));

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome sauce... 🆒 . The guarded scope looks good ⚡

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: I would just do if (scope.getLastBreadcrumb), saves some bytes and should be good enough here!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2690545

return null;
}

const newBreadcrumb = scope.getLastBreadcrumb();

// Listener can be called when breadcrumbs have not changed, so we store the
Expand Down
14 changes: 13 additions & 1 deletion packages/replay/test/unit/coreHandlers/handleScope.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ import type { Breadcrumb, Scope } from '@sentry/types';
import * as HandleScope from '../../../src/coreHandlers/handleScope';

describe('Unit | coreHandlers | handleScope', () => {
const mockHandleScope = jest.spyOn(HandleScope, 'handleScope');
let mockHandleScope: jest.SpyInstance;

beforeEach(() => {
mockHandleScope = jest.spyOn(HandleScope, 'handleScope');
mockHandleScope.mockClear();
});

it('returns a breadcrumb only if last breadcrumb has changed', function () {
const scope = {
Expand Down Expand Up @@ -47,4 +52,11 @@ describe('Unit | coreHandlers | handleScope', () => {
expect(mockHandleScope).toHaveBeenCalledTimes(1);
expect(mockHandleScope).toHaveReturnedWith(expect.objectContaining({ message: 'f00', category: 'console' }));
});

it('returns null if the method does not exist on the scope', () => {
const scope = {} as unknown as Scope;
HandleScope.handleScope(scope);
expect(mockHandleScope).toHaveBeenCalledTimes(1);
expect(mockHandleScope).toHaveReturnedWith(null);
});
});