From 85daf9015a8e49761a27d3521706dc5742b58f15 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 21 Nov 2024 20:54:31 +0100 Subject: [PATCH 1/4] fix(feedback): Fix non-wrapping form title (#14355) Fixes an issue with long form titles causing the width of the dialog to expand. Instead, apply the css var `--form-width` to the dialog contents so that the title wraps. After: image image Single line: image Closes #14351 --- .../src/modal/components/Dialog.css.ts | 23 +++++++++++-------- .../src/modal/components/DialogHeader.tsx | 2 +- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/packages/feedback/src/modal/components/Dialog.css.ts b/packages/feedback/src/modal/components/Dialog.css.ts index fc0e00580a5d..da12e3d70990 100644 --- a/packages/feedback/src/modal/components/Dialog.css.ts +++ b/packages/feedback/src/modal/components/Dialog.css.ts @@ -60,7 +60,7 @@ const DIALOG = ` gap: 16px; padding: var(--dialog-padding, 24px); max-width: 100%; - width: 100%; + width: var(--form-width, 272px); max-height: 100%; overflow: auto; @@ -71,17 +71,26 @@ const DIALOG = ` transform: translate(0, 0) scale(1); transition: transform 0.2s ease-in-out; } + +@media (max-width: 600px) { + .dialog__content { + width: var(--form-width, 100%); + } +} + `; const DIALOG_HEADER = ` .dialog__header { display: flex; - align-items: center; + gap: 4px; justify-content: space-between; font-weight: var(--dialog-header-weight, 600); margin: 0; } - +.dialog__title { + align-self: center; +} .brand-link { display: inline-flex; } @@ -101,18 +110,12 @@ const FORM = ` .form__right { flex: 0 0 auto; - width: var(--form-width, 272px); display: flex; overflow: auto; flex-direction: column; justify-content: space-between; gap: 20px; -} - -@media (max-width: 600px) { - .form__right { - width: var(--form-width, 100%); - } + width: 100%; } .form__top { diff --git a/packages/feedback/src/modal/components/DialogHeader.tsx b/packages/feedback/src/modal/components/DialogHeader.tsx index 44d29af629f6..217ce6676ee0 100644 --- a/packages/feedback/src/modal/components/DialogHeader.tsx +++ b/packages/feedback/src/modal/components/DialogHeader.tsx @@ -14,7 +14,7 @@ export function DialogHeader({ options }: Props): VNode { return (

- {options.formTitle} + {options.formTitle} {options.showBranding ? ( Date: Thu, 21 Nov 2024 22:06:01 +0100 Subject: [PATCH 2/4] feat(replay): Clear event buffer when full and in buffer mode (#14078) This makes a change so that when the event buffer is full, we will clear it and wait for the next checkout instead of erroring and stopping the replay buffering. This does mean that it's possible an exception occurs in-between the filled buffer and the next checkout, where we will have an empty replay --- .../src/eventBuffer/EventBufferArray.ts | 4 + .../EventBufferCompressionWorker.ts | 4 + .../src/eventBuffer/EventBufferProxy.ts | 14 +++- packages/replay-internal/src/types/replay.ts | 6 ++ packages/replay-internal/src/util/addEvent.ts | 27 +++++-- .../test/integration/eventBuffer.test.ts | 75 +++++++++++++++++++ .../test/integration/stop.test.ts | 38 +++------- 7 files changed, 135 insertions(+), 33 deletions(-) create mode 100644 packages/replay-internal/test/integration/eventBuffer.test.ts diff --git a/packages/replay-internal/src/eventBuffer/EventBufferArray.ts b/packages/replay-internal/src/eventBuffer/EventBufferArray.ts index 2eb760409d9f..9b6f5f602e63 100644 --- a/packages/replay-internal/src/eventBuffer/EventBufferArray.ts +++ b/packages/replay-internal/src/eventBuffer/EventBufferArray.ts @@ -14,12 +14,16 @@ export class EventBufferArray implements EventBuffer { /** @inheritdoc */ public hasCheckout: boolean; + /** @inheritdoc */ + public waitForCheckout: boolean; + private _totalSize: number; public constructor() { this.events = []; this._totalSize = 0; this.hasCheckout = false; + this.waitForCheckout = false; } /** @inheritdoc */ diff --git a/packages/replay-internal/src/eventBuffer/EventBufferCompressionWorker.ts b/packages/replay-internal/src/eventBuffer/EventBufferCompressionWorker.ts index 8ca7f3caccca..13fab7e6717b 100644 --- a/packages/replay-internal/src/eventBuffer/EventBufferCompressionWorker.ts +++ b/packages/replay-internal/src/eventBuffer/EventBufferCompressionWorker.ts @@ -16,6 +16,9 @@ export class EventBufferCompressionWorker implements EventBuffer { /** @inheritdoc */ public hasCheckout: boolean; + /** @inheritdoc */ + public waitForCheckout: boolean; + private _worker: WorkerHandler; private _earliestTimestamp: number | null; private _totalSize; @@ -25,6 +28,7 @@ export class EventBufferCompressionWorker implements EventBuffer { this._earliestTimestamp = null; this._totalSize = 0; this.hasCheckout = false; + this.waitForCheckout = false; } /** @inheritdoc */ diff --git a/packages/replay-internal/src/eventBuffer/EventBufferProxy.ts b/packages/replay-internal/src/eventBuffer/EventBufferProxy.ts index ea81365b66a9..c982567e49c1 100644 --- a/packages/replay-internal/src/eventBuffer/EventBufferProxy.ts +++ b/packages/replay-internal/src/eventBuffer/EventBufferProxy.ts @@ -25,6 +25,11 @@ export class EventBufferProxy implements EventBuffer { this._ensureWorkerIsLoadedPromise = this._ensureWorkerIsLoaded(); } + /** @inheritdoc */ + public get waitForCheckout(): boolean { + return this._used.waitForCheckout; + } + /** @inheritdoc */ public get type(): EventBufferType { return this._used.type; @@ -44,6 +49,12 @@ export class EventBufferProxy implements EventBuffer { this._used.hasCheckout = value; } + /** @inheritdoc */ + // eslint-disable-next-line @typescript-eslint/adjacent-overload-signatures + public set waitForCheckout(value: boolean) { + this._used.waitForCheckout = value; + } + /** @inheritDoc */ public destroy(): void { this._fallback.destroy(); @@ -99,7 +110,7 @@ export class EventBufferProxy implements EventBuffer { /** Switch the used buffer to the compression worker. */ private async _switchToCompressionWorker(): Promise { - const { events, hasCheckout } = this._fallback; + const { events, hasCheckout, waitForCheckout } = this._fallback; const addEventPromises: Promise[] = []; for (const event of events) { @@ -107,6 +118,7 @@ export class EventBufferProxy implements EventBuffer { } this._compression.hasCheckout = hasCheckout; + this._compression.waitForCheckout = waitForCheckout; // We switch over to the new buffer immediately - any further events will be added // after the previously buffered ones diff --git a/packages/replay-internal/src/types/replay.ts b/packages/replay-internal/src/types/replay.ts index 6892c05ee179..4f78a438ecc3 100644 --- a/packages/replay-internal/src/types/replay.ts +++ b/packages/replay-internal/src/types/replay.ts @@ -400,6 +400,12 @@ export interface EventBuffer { */ hasCheckout: boolean; + /** + * If the event buffer needs to wait for a checkout event before it + * starts buffering events. + */ + waitForCheckout: boolean; + /** * Destroy the event buffer. */ diff --git a/packages/replay-internal/src/util/addEvent.ts b/packages/replay-internal/src/util/addEvent.ts index 3a245242e608..5aa420bfff15 100644 --- a/packages/replay-internal/src/util/addEvent.ts +++ b/packages/replay-internal/src/util/addEvent.ts @@ -54,17 +54,22 @@ async function _addEvent( event: RecordingEvent, isCheckout?: boolean, ): Promise { - if (!replay.eventBuffer) { + const { eventBuffer } = replay; + + if (!eventBuffer || (eventBuffer.waitForCheckout && !isCheckout)) { return null; } + const isBufferMode = replay.recordingMode === 'buffer'; + try { - if (isCheckout && replay.recordingMode === 'buffer') { - replay.eventBuffer.clear(); + if (isCheckout && isBufferMode) { + eventBuffer.clear(); } if (isCheckout) { - replay.eventBuffer.hasCheckout = true; + eventBuffer.hasCheckout = true; + eventBuffer.waitForCheckout = false; } const replayOptions = replay.getOptions(); @@ -75,9 +80,19 @@ async function _addEvent( return; } - return await replay.eventBuffer.addEvent(eventAfterPossibleCallback); + return await eventBuffer.addEvent(eventAfterPossibleCallback); } catch (error) { - const reason = error && error instanceof EventBufferSizeExceededError ? 'addEventSizeExceeded' : 'addEvent'; + const isExceeded = error && error instanceof EventBufferSizeExceededError; + const reason = isExceeded ? 'addEventSizeExceeded' : 'addEvent'; + + if (isExceeded && isBufferMode) { + // Clear buffer and wait for next checkout + eventBuffer.clear(); + eventBuffer.waitForCheckout = true; + + return null; + } + replay.handleException(error); await replay.stop({ reason }); diff --git a/packages/replay-internal/test/integration/eventBuffer.test.ts b/packages/replay-internal/test/integration/eventBuffer.test.ts new file mode 100644 index 000000000000..bc796516acf8 --- /dev/null +++ b/packages/replay-internal/test/integration/eventBuffer.test.ts @@ -0,0 +1,75 @@ +/** + * @vitest-environment jsdom + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { WINDOW } from '../../src/constants'; +import type { Replay } from '../../src/integration'; +import type { ReplayContainer } from '../../src/replay'; +import { addEvent } from '../../src/util/addEvent'; + +// mock functions need to be imported first +import { BASE_TIMESTAMP, mockSdk } from '../index'; +import { getTestEventCheckout, getTestEventIncremental } from '../utils/getTestEvent'; +import { useFakeTimers } from '../utils/use-fake-timers'; + +useFakeTimers(); + +describe('Integration | eventBuffer | Event Buffer Max Size', () => { + let replay: ReplayContainer; + let integration: Replay; + const prevLocation = WINDOW.location; + + beforeEach(async () => { + vi.setSystemTime(new Date(BASE_TIMESTAMP)); + + ({ replay, integration } = await mockSdk()); + + await vi.runAllTimersAsync(); + vi.clearAllMocks(); + }); + + afterEach(async () => { + vi.setSystemTime(new Date(BASE_TIMESTAMP)); + integration && (await integration.stop()); + Object.defineProperty(WINDOW, 'location', { + value: prevLocation, + writable: true, + }); + vi.clearAllMocks(); + }); + + it('does not add replay breadcrumb when stopped due to event buffer limit', async () => { + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); + + vi.mock('../../src/constants', async requireActual => ({ + ...(await requireActual()), + REPLAY_MAX_EVENT_BUFFER_SIZE: 500, + })); + + await integration.stop(); + integration.startBuffering(); + + await addEvent(replay, TEST_EVENT); + + expect(replay.eventBuffer?.hasEvents).toBe(true); + expect(replay.eventBuffer?.['hasCheckout']).toBe(true); + + // This should should go over max buffer size + await addEvent(replay, TEST_EVENT); + // buffer should be cleared and wait for next checkout + expect(replay.eventBuffer?.hasEvents).toBe(false); + expect(replay.eventBuffer?.['hasCheckout']).toBe(false); + + await addEvent(replay, TEST_EVENT); + expect(replay.eventBuffer?.hasEvents).toBe(false); + expect(replay.eventBuffer?.['hasCheckout']).toBe(false); + + await addEvent(replay, getTestEventCheckout({ timestamp: Date.now() }), true); + expect(replay.eventBuffer?.hasEvents).toBe(true); + expect(replay.eventBuffer?.['hasCheckout']).toBe(true); + + vi.resetAllMocks(); + }); +}); diff --git a/packages/replay-internal/test/integration/stop.test.ts b/packages/replay-internal/test/integration/stop.test.ts index 6ebca89891b0..5c3aa49b77d8 100644 --- a/packages/replay-internal/test/integration/stop.test.ts +++ b/packages/replay-internal/test/integration/stop.test.ts @@ -3,14 +3,13 @@ */ import type { MockInstance, MockedFunction } from 'vitest'; -import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import * as SentryBrowserUtils from '@sentry-internal/browser-utils'; import { WINDOW } from '../../src/constants'; import type { Replay } from '../../src/integration'; import type { ReplayContainer } from '../../src/replay'; -import { clearSession } from '../../src/session/clearSession'; import { addEvent } from '../../src/util/addEvent'; import { createOptionsEvent } from '../../src/util/handleRecordingEmit'; // mock functions need to be imported first @@ -32,7 +31,7 @@ describe('Integration | stop', () => { let mockAddDomInstrumentationHandler: MockInstance; let mockRunFlush: MockRunFlush; - beforeAll(async () => { + beforeEach(async () => { vi.setSystemTime(new Date(BASE_TIMESTAMP)); mockAddDomInstrumentationHandler = vi.spyOn(SentryBrowserUtils, 'addClickKeypressInstrumentationHandler'); @@ -41,33 +40,18 @@ describe('Integration | stop', () => { // @ts-expect-error private API mockRunFlush = vi.spyOn(replay, '_runFlush'); - vi.runAllTimers(); - }); - - beforeEach(() => { - vi.setSystemTime(new Date(BASE_TIMESTAMP)); - replay.eventBuffer?.destroy(); + await vi.runAllTimersAsync(); vi.clearAllMocks(); }); afterEach(async () => { - vi.runAllTimers(); - await new Promise(process.nextTick); vi.setSystemTime(new Date(BASE_TIMESTAMP)); - sessionStorage.clear(); - clearSession(replay); - replay['_initializeSessionForSampling'](); - replay.setInitialState(); - mockRecord.takeFullSnapshot.mockClear(); - mockAddDomInstrumentationHandler.mockClear(); + integration && (await integration.stop()); Object.defineProperty(WINDOW, 'location', { value: prevLocation, writable: true, }); - }); - - afterAll(() => { - integration && integration.stop(); + vi.clearAllMocks(); }); it('does not upload replay if it was stopped and can resume replays afterwards', async () => { @@ -106,7 +90,7 @@ describe('Integration | stop', () => { vi.advanceTimersByTime(ELAPSED); - const timestamp = +new Date(BASE_TIMESTAMP + ELAPSED + ELAPSED) / 1000; + const timestamp = +new Date(BASE_TIMESTAMP + ELAPSED + ELAPSED + ELAPSED) / 1000; const hiddenBreadcrumb = { type: 5, @@ -123,7 +107,7 @@ describe('Integration | stop', () => { addEvent(replay, TEST_EVENT); WINDOW.dispatchEvent(new Event('blur')); - vi.runAllTimers(); + await vi.runAllTimersAsync(); await new Promise(process.nextTick); expect(replay).toHaveLastSentReplay({ recordingPayloadHeader: { segment_id: 0 }, @@ -131,7 +115,7 @@ describe('Integration | stop', () => { // This event happens when we call `replay.start` { data: { isCheckout: true }, - timestamp: BASE_TIMESTAMP + ELAPSED, + timestamp: BASE_TIMESTAMP + ELAPSED + ELAPSED, type: 2, }, optionsEvent, @@ -142,12 +126,14 @@ describe('Integration | stop', () => { // Session's last activity is last updated when we call `setup()` and *NOT* // when tab is blurred - expect(replay.session?.lastActivity).toBe(BASE_TIMESTAMP + ELAPSED); + expect(replay.session?.lastActivity).toBe(BASE_TIMESTAMP + ELAPSED + ELAPSED); }); it('does not buffer new events after being stopped', async function () { + expect(replay.eventBuffer?.hasEvents).toBe(false); + expect(mockRunFlush).toHaveBeenCalledTimes(0); const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); - addEvent(replay, TEST_EVENT); + addEvent(replay, TEST_EVENT, true); expect(replay.eventBuffer?.hasEvents).toBe(true); expect(mockRunFlush).toHaveBeenCalledTimes(0); From 90f958fe8c5b5cae33e8e68c33c1cfff004b44c8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 22 Nov 2024 09:10:12 +0100 Subject: [PATCH 3/4] ci: Skip optional E2E tests on release branches (#14424) Optional tests should not block releases. There are two groups of optional tests: 1. Canary type tests 2. Tests that send data to sentry IMHO both of these can be skipped on release branches, as failure in either of them should not affect the release. They will (for now) still run on develop, we can revisit this if we want. --- .github/workflows/build.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index cd358158b2ee..d903d4913cd4 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -795,7 +795,8 @@ jobs: # - The build job was successful, not skipped # - AND if the profiling node bindings were either successful or skipped if: | - always() && needs.job_build.result == 'success' && + always() && + needs.job_build.result == 'success' && (needs.job_compile_bindings_profiling_node.result == 'success' || needs.job_compile_bindings_profiling_node.result == 'skipped') needs: [job_get_metadata, job_build, job_compile_bindings_profiling_node] runs-on: ubuntu-20.04-large-js @@ -981,13 +982,16 @@ jobs: directory: dev-packages/e2e-tests token: ${{ secrets.CODECOV_TOKEN }} + # - We skip optional tests on release branches job_optional_e2e_tests: name: E2E ${{ matrix.label || matrix.test-application }} Test # We only run E2E tests for non-fork PRs because the E2E tests require secrets to work and they can't be accessed from forks # We need to add the `always()` check here because the previous step has this as well :( # See: https://github.com/actions/runner/issues/2205 if: - always() && needs.job_e2e_prepare.result == 'success' && + always() && + needs.job_get_metadata.outputs.is_release != 'true' && + needs.job_e2e_prepare.result == 'success' && needs.job_e2e_prepare.outputs.matrix-optional != '{"include":[]}' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) && github.actor != 'dependabot[bot]' From 5841854712f843254ad0c5c3c57f82e630f5bdbb Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 22 Nov 2024 08:24:14 +0000 Subject: [PATCH 4/4] meta(changelog): Update changelog for 8.40.0 --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab6f9fb23635..1c0db74fffcf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,10 +81,12 @@ ### Other Changes - feat(browser): Send additional LCP timing info ([#14372](https://github.com/getsentry/sentry-javascript/pull/14372)) +- feat(replay): Clear event buffer when full and in buffer mode ([#14078](https://github.com/getsentry/sentry-javascript/pull/14078)) - feat(core): Ensure `normalizedRequest` on `sdkProcessingMetadata` is merged ([#14315](https://github.com/getsentry/sentry-javascript/pull/14315)) - feat(core): Hoist everything from `@sentry/utils` into `@sentry/core` ([#14382](https://github.com/getsentry/sentry-javascript/pull/14382)) - fix(core): Do not throw when trying to fill readonly properties ([#14402](https://github.com/getsentry/sentry-javascript/pull/14402)) - fix(feedback): Fix `__self` and `__source` attributes on feedback nodes ([#14356](https://github.com/getsentry/sentry-javascript/pull/14356)) +- fix(feedback): Fix non-wrapping form title ([#14355](https://github.com/getsentry/sentry-javascript/pull/14355)) - fix(nextjs): Update check for not found navigation error ([#14378](https://github.com/getsentry/sentry-javascript/pull/14378)) ## 8.39.0