feat(feedback): New feedback integration with screenshots by c298lee · Pull Request #10590 · getsentry/sentry-javascript

c298lee · 2024-02-09T17:42:57Z

Replace the current screenshot package with a new package that has screenshots and uses Preact for better readability. It will also allow for lazy loading in the future.

Relates to: getsentry/sentry#63749

…nc loading

…components

github-actions · 2024-02-14T00:16:24Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped)	84.32 KB (+9.27% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	68.43 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped)	72.34 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped)	61.97 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	32.65 KB (0%)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped)	32.65 KB (0%)
@sentry/browser (incl. Feedback) - Webpack (gzipped)	37.88 KB (+22.93% 🔺)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped)	37.87 KB (+22.91% 🔺)
@sentry/browser - Webpack (gzipped)	22.09 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped)	82.52 KB (+9.19% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	67.26 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	33.09 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped)	24.58 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	210.74 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	99.46 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	73.64 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	36.16 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	68.71 KB (0%)
@sentry/react - Webpack (gzipped)	22.12 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	85.18 KB (0%)
@sentry/nextjs Client - Webpack (gzipped)	49.52 KB (0%)
@sentry-internal/feedback - Webpack (gzipped)	24.15 KB (+41.78% 🔺)

This allows you to configure Sucrase for rollup bundles. Needed for our work on the Feedback screenshotting feature as we want to use Preact + JSX in the project. (ref: #10590)

…elp once submitting works

This is building off a version of #10590, specifically working towards async loading of the Dialog component, and rewriting it in preact in order to host and interact with the screenshot feature in an easier way. Setup in my test app is (gives screenshots): ``` import { feedback2Integration, feedback2ModalIntegration, feedback2ScreenshotIntegration } from "@sentry-internal/feedback2"; integrations: [ feedback2Integration({ colorScheme: 'light', isNameRequired: false, isEmailRequired: false, }), feedback2ModalIntegration(), feedback2ScreenshotIntegration(), ] ```

billyvg · 2024-03-12T16:22:36Z

packages/feedback/src/core/createMainStyles.ts

 :host {
  --bottom: 1rem;
-  --right: 1rem;
+  --right: 1rem; /* this is the font-size of the page, not the shadowroot */


Is this comment in the right place?

I think it is, sorta... we don't need this comment, it's more like a TODO or 'dont forget'

The rem unit is relative to the page, so it could be set to something other than 16px and thus our modal spacing from the edge would be different as well.

let's remove this or make it a JS comment, otherwise it may just add unnecessary bytes I guess?

billyvg · 2024-03-12T16:22:44Z

packages/feedback/src/core/createMainStyles.ts

-  position: fixed;
-  left: var(--left);
-  right: var(--right);
-  bottom: var(--bottom);
-  top: var(--top);
-  z-index: var(--z-index);


Are these not used?

this has been moved into Actor.css

packages/feedback/src/modal/components/DialogContainer.tsx

mydea · 2024-03-13T08:58:09Z

packages/feedback/.eslintrc.js

+      files: ['src/**/*.ts', 'src/**/*.tsx'],
      rules: {
-        'no-console': 'off',
+        '@sentry-internal/sdk/no-unsupported-es6-methods': 'off',


you can remove this, we have since removed this rule :)

mydea · 2024-03-13T08:58:27Z

packages/feedback/.eslintrc.js

-      files: ['src/types/deprecated.ts'],
      rules: {
-        '@typescript-eslint/naming-convention': 'off',
+        'no-console': 'off',


hmm, do we really want to disable this? IMHO we should leave this on to avoid debugging stuff leaking in.

we don't even leverage that this is off. i'm removing it.

mydea · 2024-03-13T09:06:41Z

packages/feedback/src/core/sendFeedback.ts


-    client.emit('beforeSendFeedback', feedbackEvent, { includeReplay: Boolean(includeReplay) });
+    try {
+      const response = await transport.send(


So instead of this, I think we can/should just use client.sendEvent(feedbackEvent). This will also take care of getsentry/sentry#66214 (comment), I believe. Or is there anything that is preventing this?

replacing this call seems to work fine, but we've got no response now, so the error messages are not going to come from this file.

mydea · 2024-03-13T09:07:08Z

packages/feedback/src/core/sendFeedback.ts

+
+      if (attachments && attachments.length) {
+        // TODO: https://docs.sentry.io/platforms/javascript/enriching-events/attachments/
+        await transport.send(


See comment above, instead we can do client.sendEvent(feedbackEvent, { attachments }) to send another event with the attachments.

with either the transport.send or the client.sendEvent i'm not seeing a screenshot coming into sentry today. :(

yea sendEvent currently doesn't work with attachments due to our backend. If there's an attachment and a feedback within the same envelope, the screenshot doesn't come in. We have to send them in seperate envelopes, but once the backend is fixed we will be using sendEvent client.sendEvent(feedbackEvent, { attachments })

mydea · 2024-03-13T09:07:27Z

packages/feedback/src/core/sendFeedback.ts

+      }

-    const envelope = createEventEnvelope(feedbackEvent, dsn, client.getOptions()._metadata, client.getOptions().tunnel);
+      // TODO (v8): we can remove this guard once transport.send's type signature doesn't include void anymore


This should already be done, so you can remove this check!

mydea · 2024-03-13T09:08:34Z

packages/feedback/src/core/sendFeedback.ts

-    let response: TransportMakeRequestResponse;
+      // Require valid status codes, otherwise can assume feedback was not sent successfully
+      if (typeof response.statusCode === 'number' && (response.statusCode < 200 || response.statusCode >= 300)) {
+        throw new Error('Unable to send Feedback. Invalid response from server.');


While we are at it, or in a follow up, we could check for statusCode === 0 and show a message like:

Unable to send Feedback. This is because of network issues, or because you are using an ad-blocker.

Or something along these lines...!

ryan953 · 2024-03-13T13:58:27Z

packages/feedback/src/screenshot/components/ScreenshotEditor.tsx

@@ -0,0 +1,330 @@
+// eslint-disable max-lines


This file is currently a bit long, but we will be iterating and will tidy up as we go.

AbhiPrasad · 2024-03-13T22:31:38Z

try 1 at fixing ts3.8 integration test failure: #11088

Right now the feedback PR fails ts3.8 e2e tests. This is because of TS issues, preact types do not work well with older TS types: ``` > @sentry-internal/ts3.8-test@ type-check /home/runner/work/sentry-javascript/sentry-javascript/dev-packages/e2e-tests/test-applications/generic-ts3.8 > tsc --project tsconfig.json ##[debug]Dropping file value '/home/runner/work/sentry-javascript/sentry-javascript/node_modules/.pnpm/registry.npmjs.org+preact@10.19.6/node_modules/preact/src/jsx.d.ts'. Path does not exist Error: node_modules/.pnpm/registry.npmjs.org+preact@10.19.6/node_modules/preact/src/jsx.d.ts(1462,3): error TS2304: Cannot find name 'SubmitEvent'. ##[debug]Dropping file value '/home/runner/work/sentry-javascript/sentry-javascript/node_modules/.pnpm/registry.npmjs.org+preact@10.19.6/node_modules/preact/src/jsx.d.ts'. Path does not exist Error: node_modules/.pnpm/registry.npmjs.org+preact@10.19.6/node_modules/preact/src/jsx.d.ts(1479,25): error TS2304: Cannot find name 'PictureInPictureEvent'. ##[debug]Dropping file value '/home/runner/work/sentry-javascript/sentry-javascript/node_modules/.pnpm/registry.npmjs.org+preact@10.19.6/node_modules/preact/src/jsx.d.ts'. Path does not exist Error: node_modules/.pnpm/registry.npmjs.org+preact@10.19.6/node_modules/preact/src/jsx.d.ts(2549,65): error TS2304: Cannot find name 'MathMLElement'. ##[debug]Dropping file value '/home/runner/work/sentry-javascript/sentry-javascript/node_modules/.pnpm/registry.npmjs.org+preact@10.19.6/node_modules/preact/src/jsx.d.ts'. Path does not exist Error: node_modules/.pnpm/registry.npmjs.org+preact@10.19.6/node_modules/preact/src/jsx.d.ts(2565,49): error TS2304: Cannot find name 'MathMLElement'. ##[debug]Dropping file value '/home/runner/work/sentry-javascript/sentry-javascript/node_modules/.pnpm/registry.npmjs.org+preact@10.19.6/node_modules/preact/src/jsx.d.ts'. Path does not exist Error: node_modules/.pnpm/registry.npmjs.org+preact@10.19.6/node_modules/preact/src/jsx.d.ts(2571,52): error TS2304: Cannot find name 'MathMLElement'. ``` Essentially these types don't work on older versions and because the feedback package is depended on by the browser package, we will break a certain set of our users. We have to fix this before we can ship because of the browser package dependency. To get around this, we do a workaround with `packages/feedback/scripts/shim-preact-export.js`. Essentially we shim the preact type in our exports, which should change what the declaration files themselves point to. So something like the following: ```ts import type { ComponentChildren, VNode } from 'preact'; ``` turns into ```ts // no preact to be seen! type ComponentChildren: any; type VNode: any; ```

packages/feedback/scripts/shim-preact-export.js

+      walk(filePath);
+    } else {
+      if (filePath.endsWith('.d.ts')) {
+        const content = fs.readFileSync(filePath, 'utf8');


packages/feedback/scripts/shim-preact-export.js

+          const newContent = content.replace(preactImportRegex, '// replaced import from preact');
+
+          // and write the new content to the file
+          fs.writeFileSync(filePath, snippet + newContent, 'utf8');


AbhiPrasad · 2024-03-14T15:10:46Z

Merging this in to add to the alpha release!

c298lee and others added 11 commits February 9, 2024 12:40

working feedback screenshot package with preact

7a22a83

working feedback screenshot package with preact

4877b4f

update rollup

ef7d7e8

allow passing options to sucrase + pass jsxPragma

0395331

proper ts-expect-error

364e187

fix for preact

510f37c

change to tsx and fix for preact

513ab3e

screenshot button

7d9b800

screenshot button and widget placeholders

8dbd500

Make feedback depend on feedback-screenshot for now, until we have ay…

4a007a1

…nc loading

Convert screenshotButton to a functional component, and add types to …

0de030f

…components

ryan953 force-pushed the cl/screenshot-integration branch from 72d3b27 to 0de030f Compare February 14, 2024 00:06

ryan953 and others added 3 commits February 13, 2024 16:17

Update docblock for feedbackScreenshotIntegration

8ea52d3

take screenshot

d30f236

take screenshot

f9b6f31

ryan953 mentioned this pull request Feb 15, 2024

Ryan953/feedback async #10683

Merged

move screenshot into form

354bb52

billyvg mentioned this pull request Feb 20, 2024

wip: screenshots in feedback #9562

Closed

billyvg mentioned this pull request Feb 20, 2024

feat(build): Allow passing Sucrase options for rollup #10747

Merged

c298lee and others added 9 commits February 22, 2024 00:53

working screenshot attachment

22efe6e

Merge branch 'develop' into cl/screenshot-integration

22dad05

clean up attachment code

ea7b909

remove alias plugin

1b216b9

remove dialog when screenshotting

b03f61a

wip: cropping box follows mouse, need to remove screenshot editor & h…

30e9c40

…elp once submitting works

cropping works for 1 cornergit add .

a1771bd

4 corners working! need to fix resetting image

4447057

rename feedback2 to feedback

c106fd6

billyvg reviewed Mar 12, 2024

View reviewed changes

c298lee added 3 commits March 12, 2024 14:07

crop buttons don't move past the other crop buttons

c73ab4b

disable lint & move styling

84ed105

ignore file check and fix name

40cc76a

mydea reviewed Mar 13, 2024

View reviewed changes

ryan953 added 3 commits March 13, 2024 06:49

remove extra eslint rules, re-arrange package.json to keep diff tidy

24b3956

rm extra css comment, and rm unneeded line-move

50fad97

fix indent

75a37f8

ryan953 reviewed Mar 13, 2024

View reviewed changes

ryan953 added 4 commits March 13, 2024 07:01

fix indent

fc8b8c7

Merge branch 'develop' into cl/screenshot-integration

d6f70fe

convert indent to spaces

1621e61

add specific error message when response.statusCode === 0

b5ed596

mydea approved these changes Mar 13, 2024

View reviewed changes

ryan953 and others added 3 commits March 13, 2024 09:17

keep the original test for sendFeedback()

b33d2df

drop preact-compat, we use sucrase and set jsxPragma: h,

b2016cf

Merge branch 'develop' into cl/screenshot-integration

c0ec628

AbhiPrasad mentioned this pull request Mar 13, 2024

Try aliasing preact typescript declaration to fix ts errors #11088

Merged

github-advanced-security bot found potential problems Mar 14, 2024

View reviewed changes

AbhiPrasad merged commit df5f44e into develop Mar 14, 2024

AbhiPrasad deleted the cl/screenshot-integration branch March 14, 2024 15:10

Uh oh!

Conversation

c298lee commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad commented Mar 13, 2024

Uh oh!

Check failure

Check failure

AbhiPrasad commented Mar 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

c298lee commented Feb 9, 2024 •

edited

Loading

github-actions bot commented Feb 14, 2024 •

edited

Loading