Skip to content

feat(feedback): New feedback integration with screenshots #10590

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

Merged
merged 62 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
7a22a83
working feedback screenshot package with preact
c298lee Feb 9, 2024
4877b4f
working feedback screenshot package with preact
c298lee Feb 9, 2024
ef7d7e8
update rollup
c298lee Feb 9, 2024
0395331
allow passing options to sucrase + pass jsxPragma
billyvg Feb 10, 2024
364e187
proper ts-expect-error
billyvg Feb 10, 2024
510f37c
fix for preact
billyvg Feb 10, 2024
513ab3e
change to tsx and fix for preact
billyvg Feb 10, 2024
7d9b800
screenshot button
c298lee Feb 12, 2024
8dbd500
screenshot button and widget placeholders
c298lee Feb 12, 2024
4a007a1
Make feedback depend on feedback-screenshot for now, until we have ay…
ryan953 Feb 13, 2024
0de030f
Convert screenshotButton to a functional component, and add types to …
ryan953 Feb 14, 2024
8ea52d3
Update docblock for feedbackScreenshotIntegration
ryan953 Feb 14, 2024
d30f236
take screenshot
c298lee Feb 15, 2024
f9b6f31
take screenshot
c298lee Feb 15, 2024
354bb52
move screenshot into form
c298lee Feb 16, 2024
22efe6e
working screenshot attachment
c298lee Feb 22, 2024
22dad05
Merge branch 'develop' into cl/screenshot-integration
c298lee Feb 22, 2024
ea7b909
clean up attachment code
c298lee Feb 22, 2024
1b216b9
remove alias plugin
c298lee Feb 22, 2024
b03f61a
remove dialog when screenshotting
c298lee Feb 22, 2024
30e9c40
wip: cropping box follows mouse, need to remove screenshot editor & h…
c298lee Feb 29, 2024
a1771bd
cropping works for 1 cornergit add .
c298lee Feb 29, 2024
ea96f76
Ryan953/feedback async (#10683)
ryan953 Feb 29, 2024
4447057
4 corners working! need to fix resetting image
c298lee Feb 29, 2024
5122a60
fix cropping & button styling
c298lee Mar 1, 2024
a38d9c5
disable screenshots in mobile browsers
ryan953 Mar 1, 2024
752039a
Merge branch 'develop' into cl/screenshot-integration
c298lee Mar 1, 2024
1c94e75
update import path
ryan953 Mar 1, 2024
4312d30
bump versions of new packages to match latest release
ryan953 Mar 1, 2024
c4124b7
Merge branch 'develop' into cl/screenshot-integration
c298lee Mar 4, 2024
e19a876
cropping works with feedback 2! need to clean up & refactor
c298lee Mar 6, 2024
2649607
clean up
c298lee Mar 7, 2024
7d39df4
revert feedback form
c298lee Mar 7, 2024
e148ef5
Merge branch 'develop' into cl/screenshot-integration
c298lee Mar 7, 2024
8a08bbd
some styling
c298lee Mar 7, 2024
359a379
rename to imageBuffer
ryan953 Mar 7, 2024
cfdb29e
add return types to all the callbacks
ryan953 Mar 7, 2024
61c9a5a
wip - managing canvas drawing
ryan953 Mar 7, 2024
3a60a19
bringing cropping handles inside the modal
ryan953 Mar 7, 2024
1bcd4ae
follow rule of hooks in Form, and render the gray background always
ryan953 Mar 7, 2024
f6355cb
start the selection size as large as possible, reset it whenever wind…
ryan953 Mar 7, 2024
2786d6a
bump versions of new packages to match latest release
ryan953 Mar 8, 2024
824c7b6
everything functioning including resizing & tall images :D
c298lee Mar 9, 2024
b0d2ad5
remove feedback-screenshot
c298lee Mar 11, 2024
1f89d8a
Merge branch 'develop' into cl/screenshot-integration
c298lee Mar 11, 2024
ce0be69
don't allow crop stuff to go outside of the image
c298lee Mar 12, 2024
d8b6242
Merge branch 'develop' into cl/screenshot-integration
c298lee Mar 12, 2024
c106fd6
rename feedback2 to feedback
ryan953 Mar 12, 2024
c73ab4b
crop buttons don't move past the other crop buttons
c298lee Mar 12, 2024
84ed105
disable lint & move styling
c298lee Mar 12, 2024
40cc76a
ignore file check and fix name
c298lee Mar 12, 2024
24b3956
remove extra eslint rules, re-arrange package.json to keep diff tidy
ryan953 Mar 13, 2024
50fad97
rm extra css comment, and rm unneeded line-move
ryan953 Mar 13, 2024
75a37f8
fix indent
ryan953 Mar 13, 2024
fc8b8c7
fix indent
ryan953 Mar 13, 2024
d6f70fe
Merge branch 'develop' into cl/screenshot-integration
ryan953 Mar 13, 2024
1621e61
convert indent to spaces
ryan953 Mar 13, 2024
b5ed596
add specific error message when response.statusCode === 0
ryan953 Mar 13, 2024
b33d2df
keep the original test for sendFeedback()
ryan953 Mar 13, 2024
b2016cf
drop preact-compat, we use sucrase and set `jsxPragma: h,`
ryan953 Mar 13, 2024
c0ec628
Merge branch 'develop' into cl/screenshot-integration
c298lee Mar 13, 2024
4591603
Try aliasing preact typescript declaration to fix ts errors (#11088)
AbhiPrasad Mar 14, 2024
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
16 changes: 15 additions & 1 deletion .size-limit.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,26 @@ module.exports = [
limit: '35 KB',
},
{
name: '@sentry/browser (incl. Feedback) - Webpack (gzipped)',
name: '@sentry/browser (incl. feedbackIntegration) - Webpack (gzipped)',
path: 'packages/browser/build/npm/esm/index.js',
import: '{ init, feedbackIntegration }',
gzip: true,
limit: '50 KB',
},
{
name: '@sentry/browser (incl. feedbackModalIntegration) - Webpack (gzipped)',
path: 'packages/browser/build/npm/esm/index.js',
import: '{ init, feedbackIntegration, feedbackModalIntegration }',
gzip: true,
limit: '50 KB',
},
{
name: '@sentry/browser (incl. feedbackScreenshotIntegration) - Webpack (gzipped)',
path: 'packages/browser/build/npm/esm/index.js',
import: '{ init, feedbackIntegration, feedbackModalIntegration, feedbackScreenshotIntegration }',
gzip: true,
limit: '50 KB',
},
{
name: '@sentry/browser (incl. sendFeedback) - Webpack (gzipped)',
path: 'packages/browser/build/npm/esm/index.js',
Expand Down
31 changes: 31 additions & 0 deletions packages/core/src/envelope.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import type {
Attachment,
AttachmentItem,
DsnComponents,
Event,
EventEnvelope,
Expand All @@ -11,6 +13,7 @@ import type {
SessionItem,
} from '@sentry/types';
import {
createAttachmentEnvelopeItem,
createEnvelope,
createEventEnvelopeHeaders,
dsnToString,
Expand Down Expand Up @@ -86,3 +89,31 @@ export function createEventEnvelope(
const eventItem: EventItem = [{ type: eventType }, event];
return createEnvelope<EventEnvelope>(envelopeHeaders, [eventItem]);
}

/**
* Create an Envelope from an event.
*/
export function createAttachmentEnvelope(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to send the attachment in a seperate envelope

Copy link
Member

Choose a reason for hiding this comment

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

hmm, this is a bit weird IMHO. We are not really creating an attachment envelope, aren't we? We are just sending a regular (?) event and adding attachments to it...? Or am I misunderstanding this? What is event in this case?

Could we not just use client.sendEvent() here - that takes a hint which will add any attachments from it. So we could do e.g.:

client.sendEvent(feedbackEvent, { attachments });

This would also take care of all the metadata etc.

Copy link
Member

Choose a reason for hiding this comment

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

afaik there's something in the backend that's preventing this from working properly, because the old user-feedbacks had their own pipeline/queue or something.

@andrewshie-sentry is working on creating a new queue that'll fix the issue. maybe @JoshFerge can also explain in the meantime. It's really causing a bunch of extra code in here to manage the attachment sending :(

Copy link
Member

Choose a reason for hiding this comment

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

yeah, but we can still send it twice? we are already doing this, should have the same outcome I believe?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, we need to separate out some things in relay / our own consumer long term for this to work well.

event: Event,
attachments: Attachment[],
dsn?: DsnComponents,
metadata?: SdkMetadata,
tunnel?: string,
): EventEnvelope {
const sdkInfo = getSdkMetadataForEnvelopeHeader(metadata);
enhanceEventWithSdkInfo(event, metadata && metadata.sdk);

const envelopeHeaders = createEventEnvelopeHeaders(event, sdkInfo, tunnel, dsn);

// Prevent this data (which, if it exists, was used in earlier steps in the processing pipeline) from being sent to
// sentry. (Note: Our use of this property comes and goes with whatever we might be debugging, whatever hacks we may
// have temporarily added, etc. Even if we don't happen to be using it at some point in the future, let's not get rid
// of this `delete`, lest we miss putting it back in the next time the property is in use.)
delete event.sdkProcessingMetadata;

const attachmentItems: AttachmentItem[] = [];
for (const attachment of attachments || []) {
attachmentItems.push(createAttachmentEnvelopeItem(attachment));
}
return createEnvelope<EventEnvelope>(envelopeHeaders, attachmentItems);
}
2 changes: 1 addition & 1 deletion packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export type { IntegrationIndex } from './integration';

export * from './tracing';
export * from './semanticAttributes';
export { createEventEnvelope, createSessionEnvelope } from './envelope';
export { createEventEnvelope, createSessionEnvelope, createAttachmentEnvelope } from './envelope';
export {
captureCheckIn,
withMonitor,
Expand Down
20 changes: 0 additions & 20 deletions packages/feedback/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,6 @@ module.exports = {
parserOptions: {
project: ['tsconfig.test.json'],
},
rules: {
'no-console': 'off',
},
},
{
files: ['test/**/*.ts'],

rules: {
// most of these errors come from `new Promise(process.nextTick)`
'@typescript-eslint/unbound-method': 'off',
// TODO: decide if we want to enable this again after the migration
// We can take the freedom to be a bit more lenient with tests
'@typescript-eslint/no-floating-promises': 'off',
},
},
{
files: ['src/types/deprecated.ts'],
rules: {
'@typescript-eslint/naming-convention': 'off',
},
},
],
};
5 changes: 3 additions & 2 deletions packages/feedback/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@
"dependencies": {
"@sentry/core": "8.0.0-alpha.2",
"@sentry/types": "8.0.0-alpha.2",
"@sentry/utils": "8.0.0-alpha.2"
"@sentry/utils": "8.0.0-alpha.2",
"preact": "^10.19.4"
},
"scripts": {
"build": "run-p build:transpile build:types build:bundle",
Expand All @@ -53,7 +54,7 @@
"build:dev": "run-p build:transpile build:types",
"build:types": "run-s build:types:core build:types:downlevel",
"build:types:core": "tsc -p tsconfig.types.json",
"build:types:downlevel": "yarn downlevel-dts build/npm/types build/npm/types-ts3.8 --to ts3.8",
"build:types:downlevel": "yarn downlevel-dts build/npm/types build/npm/types-ts3.8 --to ts3.8 && yarn node ./scripts/shim-preact-export.js",
"build:watch": "run-p build:transpile:watch build:bundle:watch build:types:watch",
"build:dev:watch": "run-p build:transpile:watch build:types:watch",
"build:transpile:watch": "yarn build:transpile --watch",
Expand Down
23 changes: 13 additions & 10 deletions packages/feedback/rollup.bundle.config.mjs
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { makeBaseBundleConfig, makeBundleConfigVariants } from '@sentry-internal/rollup-utils';

const baseBundleConfig = makeBaseBundleConfig({
bundleType: 'addon',
entrypoints: ['src/index.ts'],
licenseTitle: '@sentry-internal/feedback',
outputFileBase: () => 'bundles/feedback',
});

const builds = makeBundleConfigVariants(baseBundleConfig);

export default builds;
export default makeBundleConfigVariants(
makeBaseBundleConfig({
bundleType: 'addon',
entrypoints: ['src/index.ts'],
jsVersion: 'es6',
licenseTitle: '@sentry-internal/feedback',
outputFileBase: () => 'bundles/feedback',
sucrase: {
jsxPragma: 'h',
jsxFragmentPragma: 'Fragment',
},
}),
);
4 changes: 4 additions & 0 deletions packages/feedback/rollup.npm.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,9 @@ export default makeNPMConfigVariants(
preserveModules: false,
},
},
sucrase: {
jsxPragma: 'h',
jsxFragmentPragma: 'Fragment',
},
}),
);
75 changes: 75 additions & 0 deletions packages/feedback/scripts/shim-preact-export.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// preact does not support more modern TypeScript versions, which breaks our users that depend on older
// TypeScript versions. To fix this, we shim the types from preact to be any and remove the dependency on preact
// for types directly. This script is meant to be run after the build/npm/types-ts3.8 directory is created.

// Path: build/npm/types-ts3.8/global.d.ts

const fs = require('fs');
const path = require('path');

/**
* This regex looks for preact imports we can replace and shim out.
*
* Example:
* import { ComponentChildren, VNode } from 'preact';
*/
const preactImportRegex = /import\s*{\s*([\w\s,]+)\s*}\s*from\s*'preact'\s*;?/;

function walk(dir) {
const files = fs.readdirSync(dir);
files.forEach(file => {
const filePath = path.join(dir, file);
const stat = fs.lstatSync(filePath);
if (stat.isDirectory()) {
walk(filePath);
} else {
if (filePath.endsWith('.d.ts')) {
const content = fs.readFileSync(filePath, 'utf8');

Check failure

Code scanning / CodeQL

Potential file system race condition

The file may have changed since it [was checked](1).
const capture = preactImportRegex.exec(content);
if (capture) {
const groups = capture[1].split(',').map(s => s.trim());

// This generates a shim snippet to replace the type imports from preact
// It generates a snippet based on the capture groups of preactImportRegex.
//
// Example:
//
// import type { ComponentChildren, VNode } from 'preact';
// becomes
// type ComponentChildren: any;
// type VNode: any;
const snippet = groups.reduce((acc, curr) => {
const searchableValue = curr.includes(' as ') ? curr.split(' as ')[1] : curr;

// look to see if imported as value, then we have to use declare const
if (content.includes(`typeof ${searchableValue}`)) {
return `${acc}declare const ${searchableValue}: any;\n`;
}

// look to see if generic type like Foo<T>
if (content.includes(`${searchableValue}<`)) {
return `${acc}type ${searchableValue}<T> = any;\n`;
}

// otherwise we can just leave as type
return `${acc}type ${searchableValue} = any;\n`;
}, '');

// we then can remove the import from preact
const newContent = content.replace(preactImportRegex, '// replaced import from preact');

// and write the new content to the file
fs.writeFileSync(filePath, snippet + newContent, 'utf8');

Check failure

Code scanning / CodeQL

Potential file system race condition

The file may have changed since it [was checked](1).
}
}
}
});
}

function run() {
// recurse through build/npm/types-ts3.8 directory
const dir = path.join('build', 'npm', 'types-ts3.8');
walk(dir);
}

run();
27 changes: 27 additions & 0 deletions packages/feedback/src/constants/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { GLOBAL_OBJ } from '@sentry/utils';

export { DEFAULT_THEME } from './theme';

// exporting a separate copy of `WINDOW` rather than exporting the one from `@sentry/browser`
// prevents the browser package from being bundled in the CDN bundle, and avoids a
// circular dependency between the browser and feedback packages
export const WINDOW = GLOBAL_OBJ as typeof GLOBAL_OBJ & Window;
export const DOCUMENT = WINDOW.document;
export const NAVIGATOR = WINDOW.navigator;

export const ACTOR_LABEL = 'Report a Bug';
export const CANCEL_BUTTON_LABEL = 'Cancel';
export const SUBMIT_BUTTON_LABEL = 'Send Bug Report';
export const FORM_TITLE = 'Report a Bug';
export const EMAIL_PLACEHOLDER = '[email protected]';
export const EMAIL_LABEL = 'Email';
export const MESSAGE_PLACEHOLDER = "What's the bug? What did you expect?";
export const MESSAGE_LABEL = 'Description';
export const NAME_PLACEHOLDER = 'Your Name';
export const NAME_LABEL = 'Name';
export const SUCCESS_MESSAGE_TEXT = 'Thank you for your report!';

export const FEEDBACK_WIDGET_SOURCE = 'widget';
export const FEEDBACK_API_SOURCE = 'api';

export const SUCCESS_MESSAGE_TIMEOUT = 5000;
Original file line number Diff line number Diff line change
@@ -1,14 +1,8 @@
import { GLOBAL_OBJ } from '@sentry/utils';

// exporting a separate copy of `WINDOW` rather than exporting the one from `@sentry/browser`
// prevents the browser package from being bundled in the CDN bundle, and avoids a
// circular dependency between the browser and feedback packages
export const WINDOW = GLOBAL_OBJ as typeof GLOBAL_OBJ & Window;

const LIGHT_BACKGROUND = '#ffffff';
const INHERIT = 'inherit';
const SUBMIT_COLOR = 'rgba(108, 95, 199, 1)';
const LIGHT_THEME = {

export const LIGHT_THEME = {
fontFamily: "system-ui, 'Helvetica Neue', Arial, sans-serif",
fontSize: '14px',

Expand Down Expand Up @@ -59,18 +53,3 @@ export const DEFAULT_THEME = {
error: '#f55459',
},
};

export const ACTOR_LABEL = 'Report a Bug';
export const CANCEL_BUTTON_LABEL = 'Cancel';
export const SUBMIT_BUTTON_LABEL = 'Send Bug Report';
export const FORM_TITLE = 'Report a Bug';
export const EMAIL_PLACEHOLDER = '[email protected]';
export const EMAIL_LABEL = 'Email';
export const MESSAGE_PLACEHOLDER = "What's the bug? What did you expect?";
export const MESSAGE_LABEL = 'Description';
export const NAME_PLACEHOLDER = 'Your Name';
export const NAME_LABEL = 'Name';
export const SUCCESS_MESSAGE_TEXT = 'Thank you for your report!';

export const FEEDBACK_WIDGET_SOURCE = 'widget';
export const FEEDBACK_API_SOURCE = 'api';
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,50 @@ import { resolvedSyncPromise } from '@sentry/utils';

export interface TestClientOptions extends ClientOptions, BrowserClientReplayOptions {}

/**
*
*/
export class TestClient extends BaseClient<TestClientOptions> {
public constructor(options: TestClientOptions) {
super(options);
}

/**
*
*/
public eventFromException(exception: any): PromiseLike<Event> {
return resolvedSyncPromise({
exception: {
values: [
{
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
type: exception.name,
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
value: exception.message,
/* eslint-enable @typescript-eslint/no-unsafe-member-access */
},
],
},
});
}

/**
*
*/
public eventFromMessage(message: string, level: SeverityLevel = 'info'): PromiseLike<Event> {
return resolvedSyncPromise({ message, level });
}
}

/**
*
*/
export function init(options: TestClientOptions): void {
initAndBind(TestClient, options);
}

/**
*
*/
export function getDefaultClientOptions(options: Partial<ClientOptions> = {}): ClientOptions {
return {
integrations: [],
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,26 @@
import { DOCUMENT } from '../../constants';

/**
* Creates <style> element for widget actor (button that opens the dialog)
*/
export function createActorStyles(d: Document): HTMLStyleElement {
const style = d.createElement('style');
export function createActorStyles(): HTMLStyleElement {
const style = DOCUMENT.createElement('style');
style.textContent = `
.widget__actor {
position: fixed;
left: var(--left);
right: var(--right);
bottom: var(--bottom);
top: var(--top);
z-index: var(--z-index);

line-height: 25px;

display: flex;
align-items: center;
gap: 8px;


border-radius: var(--border-radius);
cursor: pointer;
font-size: 14px;
Expand Down
Loading