-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(feedback): add screenshot upload fallback #22201
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.
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import * as Sentry from '@sentry/browser'; | ||
| import { feedbackIntegration } from '@sentry/browser'; | ||
|
|
||
| window.Sentry = Sentry; | ||
|
|
||
| Sentry.init({ | ||
| dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
| integrations: [feedbackIntegration()], | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| <!doctype html> | ||
| <html> | ||
| <head> | ||
| <meta charset="utf-8" /> | ||
| </head> | ||
| <body></body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| import { expect } from '@playwright/test'; | ||
| import { parseEnvelope } from '@sentry/core'; | ||
| import { sentryTest } from '../../../utils/fixtures'; | ||
| import { shouldSkipFeedbackTest } from '../../../utils/helpers'; | ||
|
|
||
| sentryTest('uploads an image after display capture is rejected', async ({ getLocalTestUrl, page }) => { | ||
| if (shouldSkipFeedbackTest()) { | ||
| sentryTest.skip(); | ||
| } | ||
|
|
||
| await page.addInitScript(() => { | ||
| Object.defineProperty(globalThis, 'isSecureContext', { | ||
| configurable: true, | ||
| value: true, | ||
| }); | ||
| Object.defineProperty(navigator, 'mediaDevices', { | ||
| configurable: true, | ||
| value: { | ||
| getDisplayMedia: () => Promise.reject(new DOMException('Blocked', 'NotAllowedError')), | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| const feedbackRequestPromise = page.waitForResponse(response => { | ||
| return response.url().includes('/envelope/'); | ||
| }); | ||
| const url = await getLocalTestUrl({ testDir: __dirname, handleLazyLoadedFeedback: true }); | ||
|
|
||
| await page.goto(url); | ||
| await page.getByText('Report a Bug').click(); | ||
| await page.getByRole('button', { name: 'Add a screenshot' }).click(); | ||
|
|
||
| const fileInput = page.locator('input[type="file"]'); | ||
| await expect(fileInput).toBeVisible(); | ||
| await fileInput.setInputFiles({ | ||
| name: 'bug.png', | ||
| mimeType: 'image/png', | ||
| buffer: Buffer.from('manual screenshot'), | ||
| }); | ||
| await page.locator('[name="name"]').fill('Jane Doe'); | ||
| await page.locator('[name="email"]').fill('janedoe@example.org'); | ||
| await page.locator('[name="message"]').fill('Screenshot capture is blocked'); | ||
| await page.locator('[data-sentry-feedback] .btn--primary').click(); | ||
|
|
||
| const request = (await feedbackRequestPromise).request(); | ||
| const items = parseEnvelope(request.postDataBuffer()!)[1]; | ||
| const attachment = items.find(([header]) => header.type === 'attachment'); | ||
|
|
||
| expect(attachment?.[0]).toEqual({ | ||
| type: 'attachment', | ||
| length: 17, | ||
| filename: 'bug.png', | ||
| content_type: 'image/png', | ||
| }); | ||
| expect(new TextDecoder().decode(attachment?.[1] as Uint8Array)).toBe('manual screenshot'); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,12 @@ | ||
| // oxlint-disable max-lines | ||
| import type { FeedbackInternalOptions, FeedbackModalIntegration } from '@sentry/core'; | ||
| import type { ComponentType, h as hType, VNode } from 'preact'; | ||
| import { h } from 'preact'; // eslint-disable-line @typescript-eslint/no-unused-vars | ||
| import type * as Hooks from 'preact/hooks'; | ||
| import { DOCUMENT, WINDOW } from '../../constants'; | ||
| import IconCloseFactory from './IconClose'; | ||
| import { createScreenshotInputStyles } from './ScreenshotInput.css'; | ||
| import { ScreenshotFallback } from './ScreenshotFallback'; | ||
| import ToolbarFactory from './Toolbar'; | ||
| import { useTakeScreenshotFactory } from './useTakeScreenshot'; | ||
|
|
||
|
|
@@ -29,6 +31,11 @@ interface FactoryParams { | |
| * Needed to set nonce and id values for editor specific styles | ||
| */ | ||
| options: FeedbackInternalOptions; | ||
|
|
||
| onScreenshotStart: () => void; | ||
| onScreenshotCaptured: () => void; | ||
| onFileSelected: (file: File) => void; | ||
| onReset: () => void; | ||
| } | ||
|
|
||
| interface Props { | ||
|
|
@@ -117,6 +124,10 @@ export function ScreenshotEditorFactory({ | |
| outputBuffer, | ||
| dialog, | ||
| options, | ||
| onScreenshotStart, | ||
| onScreenshotCaptured, | ||
| onFileSelected, | ||
| onReset, | ||
| }: FactoryParams): ComponentType<Props> { | ||
| const useTakeScreenshot = useTakeScreenshotFactory({ hooks }); | ||
| const Toolbar = ToolbarFactory({ h }); | ||
|
|
@@ -328,11 +339,19 @@ export function ScreenshotEditorFactory({ | |
| ); | ||
| }; | ||
|
|
||
| return function Wrapper({ onError }: Props): VNode { | ||
| return function Wrapper(_props: Props): VNode { | ||
| const [screenshot, setScreenshot] = hooks.useState<undefined | Screenshot>(); | ||
| const [captureFailed, setCaptureFailed] = hooks.useState(false); | ||
|
|
||
| hooks.useEffect(() => { | ||
| onReset(); | ||
| return onReset; | ||
| }, []); | ||
|
|
||
| useTakeScreenshot({ | ||
| onBeforeScreenshot: hooks.useCallback(() => { | ||
| onScreenshotStart(); | ||
| setCaptureFailed(false); | ||
| dialogStyle.display = 'none'; | ||
| }, []), | ||
| onScreenshot: hooks.useCallback((screenshotVideo: HTMLVideoElement, dpi: number) => { | ||
|
|
@@ -344,6 +363,7 @@ export function ScreenshotEditorFactory({ | |
| ctx.drawImage(screenshotVideo, 0, 0, canvas.width, canvas.height); | ||
|
|
||
| setScreenshot({ canvas, dpi }); | ||
| onScreenshotCaptured(); | ||
| }); | ||
|
|
||
| // The output buffer, we only need to set the width/height on this once, it stays the same forever | ||
|
|
@@ -353,16 +373,20 @@ export function ScreenshotEditorFactory({ | |
| onAfterScreenshot: hooks.useCallback(() => { | ||
| dialogStyle.display = 'block'; | ||
| }, []), | ||
| onError: hooks.useCallback(error => { | ||
| onError: hooks.useCallback(_error => { | ||
| dialogStyle.display = 'block'; | ||
| onError(error); | ||
| setCaptureFailed(true); | ||
| }, []), | ||
| }); | ||
|
|
||
| if (screenshot) { | ||
| return <ScreenshotEditor screenshot={screenshot} />; | ||
| } | ||
|
|
||
| if (captureFailed) { | ||
| return <ScreenshotFallback options={options} onFileSelected={onFileSelected} />; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fallback styles never injectedLow Severity
Additional Locations (2)Reviewed by Cursor Bugbot for commit 5c1d839. Configure here. |
||
| } | ||
|
|
||
| return <div />; | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| import type { FeedbackInternalOptions } from '@sentry/core'; | ||
| import type { VNode } from 'preact'; | ||
| import { h } from 'preact'; // eslint-disable-line @typescript-eslint/no-unused-vars | ||
|
|
||
| interface Props { | ||
| options: FeedbackInternalOptions; | ||
| onFileSelected: (file: File) => void; | ||
| } | ||
|
|
||
| export function ScreenshotFallback({ options, onFileSelected }: Props): VNode { | ||
| return ( | ||
| <label class="screenshot-fallback"> | ||
| {options.addScreenshotButtonLabel} | ||
| <input | ||
| accept="image/*" | ||
| aria-label={options.addScreenshotButtonLabel} | ||
| class="screenshot-fallback__input" | ||
| onChange={event => { | ||
| const file = event.currentTarget.files?.[0]; | ||
| if (file) { | ||
| onFileSelected(file); | ||
| } | ||
| }} | ||
| type="file" | ||
| /> | ||
| </label> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| /** | ||
| * @vitest-environment jsdom | ||
| */ | ||
| import type { FeedbackInternalOptions, FeedbackScreenshotIntegration } from '@sentry/core'; | ||
| import { h, render } from 'preact'; | ||
| import * as hooks from 'preact/hooks'; | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { feedbackScreenshotIntegration } from '../../src/screenshot/integration'; | ||
|
|
||
| const options = { | ||
| addScreenshotButtonLabel: 'Add a screenshot', | ||
| id: 'sentry-feedback', | ||
| removeHighlightText: 'Remove', | ||
| styleNonce: undefined, | ||
| } as FeedbackInternalOptions; | ||
|
|
||
| function createInput(): ReturnType<FeedbackScreenshotIntegration['createInput']> { | ||
| return feedbackScreenshotIntegration().createInput({ | ||
| h, | ||
| hooks, | ||
| dialog: { el: document.createElement('div') } as unknown as Parameters< | ||
| FeedbackScreenshotIntegration['createInput'] | ||
| >[0]['dialog'], | ||
| options, | ||
| }); | ||
| } | ||
|
|
||
| describe('feedback screenshot upload fallback', () => { | ||
| beforeEach(() => { | ||
| document.body.innerHTML = ''; | ||
| Object.defineProperty(window, 'matchMedia', { | ||
| configurable: true, | ||
| value: vi.fn().mockReturnValue({ | ||
| addEventListener: vi.fn(), | ||
| removeEventListener: vi.fn(), | ||
| }), | ||
| }); | ||
| }); | ||
|
|
||
| it('keeps the input visible when display capture is rejected', async () => { | ||
| const onError = vi.fn(); | ||
| Object.defineProperty(navigator, 'mediaDevices', { | ||
| configurable: true, | ||
| value: { getDisplayMedia: vi.fn().mockRejectedValue(new DOMException('Blocked', 'NotAllowedError')) }, | ||
| }); | ||
| const input = createInput(); | ||
| const host = document.createElement('div'); | ||
|
|
||
| render(h(input.input, { onError }), host); | ||
|
|
||
| await vi.waitFor(() => expect(host.querySelector('input[type="file"]')).not.toBeNull()); | ||
| expect(onError).not.toHaveBeenCalled(); | ||
| expect(await input.value()).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('returns the selected image as an attachment', async () => { | ||
| Object.defineProperty(navigator, 'mediaDevices', { | ||
| configurable: true, | ||
| value: { getDisplayMedia: vi.fn().mockRejectedValue(new DOMException('Blocked', 'NotAllowedError')) }, | ||
| }); | ||
| const input = createInput(); | ||
| const host = document.createElement('div'); | ||
|
|
||
| render(h(input.input, { onError: vi.fn() }), host); | ||
| const fileInput = await vi.waitFor(() => { | ||
| const element = host.querySelector<HTMLInputElement>('input[type="file"]'); | ||
| expect(element).not.toBeNull(); | ||
| return element as HTMLInputElement; | ||
| }); | ||
| const file = new File([new Uint8Array([137, 80, 78, 71])], 'bug.png', { type: 'image/png' }); | ||
| Object.defineProperty(fileInput, 'files', { configurable: true, value: [file] }); | ||
| fileInput.dispatchEvent(new Event('change', { bubbles: true })); | ||
|
|
||
| await vi.waitFor(async () => { | ||
| const attachment = await input.value(); | ||
| expect(attachment).toEqual({ | ||
| data: new Uint8Array([137, 80, 78, 71]), | ||
| filename: 'bug.png', | ||
| contentType: 'image/png', | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('clears a selected file when the screenshot input is removed', async () => { | ||
| Object.defineProperty(navigator, 'mediaDevices', { | ||
| configurable: true, | ||
| value: { getDisplayMedia: vi.fn().mockRejectedValue(new DOMException('Blocked', 'NotAllowedError')) }, | ||
| }); | ||
| const input = createInput(); | ||
| const host = document.createElement('div'); | ||
|
|
||
| render(h(input.input, { onError: vi.fn() }), host); | ||
| const fileInput = await vi.waitFor(() => { | ||
| const element = host.querySelector<HTMLInputElement>('input[type="file"]'); | ||
| expect(element).not.toBeNull(); | ||
| return element as HTMLInputElement; | ||
| }); | ||
| const file = new File(['old screenshot'], 'old.png', { type: 'image/png' }); | ||
| Object.defineProperty(fileInput, 'files', { configurable: true, value: [file] }); | ||
| fileInput.dispatchEvent(new Event('change', { bubbles: true })); | ||
| await vi.waitFor(() => expect(input.value()).resolves.toBeDefined()); | ||
|
|
||
| render(null, host); | ||
|
|
||
| await expect(input.value()).resolves.toBeUndefined(); | ||
| }); | ||
| }); |


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flaky envelope wait in test
Medium Severity
This test waits for any
/envelope/response instead of filtering for afeedbackenvelope. With defaultbrowserSessionIntegration, a session can resolve the wait before feedback is submitted, so attachment assertions become flaky. Sibling feedback suites already usegetEnvelopeType(req) === 'feedback'. Flagged because of the Testing Conventions flake rule in the review guidelines.Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 5c1d839. Configure here.