Skip to content
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

(2.4) Feedback form attachments #4338

Open
wants to merge 115 commits into
base: feedback-ui
Choose a base branch
from

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Dec 3, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

Based on #4328

📜 Description

This PR adds attachment handling in the SDK capture feedback form leaving the image picking to be implemented on the client:

Note that string attachments are converted to UInt8Array before sending to the server.

Android iOS

💡 Motivation and Context

Fixes #4337

💚 How did you test it?

CI, Manual testing (example)

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

#skip-changelog

As discussed we will use the no props configuration in the changelog.

antonis and others added 8 commits December 18, 2024 17:30
* Adds sentry branding logo as a base64 encoded png

---------

Co-authored-by: LucasZF <[email protected]>
…959-captureFeedback-attachements

# Conflicts:
#	packages/core/src/js/feedback/FeedbackForm.tsx
#	packages/core/src/js/feedback/FeedbackForm.types.ts
#	packages/core/test/feedback/FeedbackForm.test.tsx
Base automatically changed from antonis/3859-newCaptureFeedbackAPI-Form to feedback-ui January 10, 2025 10:32
…ents

# Conflicts:
#	CHANGELOG.md
#	packages/core/src/js/feedback/FeedbackForm.styles.ts
#	packages/core/src/js/feedback/FeedbackForm.tsx
#	packages/core/src/js/feedback/FeedbackForm.types.ts
#	packages/core/src/js/feedback/defaults.ts
#	packages/core/src/js/index.ts
#	packages/core/test/feedback/FeedbackForm.test.tsx
#	samples/react-native/src/App.tsx
#	samples/react-native/src/Screens/ErrorsScreen.tsx
Comment on lines 1 to 29
/* eslint-disable no-bitwise */
export const base64ToUint8Array = (base64?: string): Uint8Array | undefined => {
if (!base64) return undefined;

const cleanedBase64 = base64.replace(/^data:.*;base64,/, ''); // Remove any prefix before the base64 string
const chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/';
const bytes: number[] = [];

let buffer = 0;
let bits = 0;

for (const char of cleanedBase64) {
if (char === '=') break;

const value = chars.indexOf(char); // Validate each character
if (value === -1) return undefined;

buffer = (buffer << 6) | value; // Shift 6 bits to the left and add the value
bits += 6;

if (bits >= 8) {
// Add a byte when we have 8 or more bits
bits -= 8;
bytes.push((buffer >> bits) & 0xff);
}
}

return new Uint8Array(bytes);
};
Copy link
Member

Choose a reason for hiding this comment

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

This is quite resource intensive, we do some of these conversion in the JS already, but it should be avoided as much as possible.


I this case I would leave that up to the users even thought it makes it a bit harder to use the API (depending on how they get the screenshot)

But it enables user to use conversion lib that are implemented in cpp with much better performance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that this is indeed resource intensive 😓 @krystofwoldrich. I ended up with this approach mainly because I was only able to successfully upload a Uint8Array with captureFeedback 🤔

I this case I would leave that up to the users even thought it makes it a bit harder to use the API (depending on how they get the screenshot)

I updated the implementation to only allow Uint8Array data and leave this to the user with 0a4b12d

@krystofwoldrich
Copy link
Member

The impl looks good, but I would scale the scope down to only allow bytes array for now and remove the conversion. We can extend it later.

@krystofwoldrich
Copy link
Member

@antonis Have you explored the option of taking screenshot of the app and attaching it?

@antonis
Copy link
Collaborator Author

antonis commented Jan 10, 2025

@antonis Have you explored the option of taking screenshot of the app and attaching it?

I haven't explored this option on the SDK side. My assumption was that the developer may choose to implement this. E.g to capture a screenshot with react-native-view-shot instead of selecting an image with react-native-image-picker as the sample implementation.
Looking at it now I see that we already have captureScreenshot (that convenietly returns Uint8Array) functionality exposed by the native SDKs. I'm not sure if this would be useful though since it will be a screenshot of the feedback form unless we use a screenshot taken before 🤔 Maybe I can explore this on this or a separate PR.

@krystofwoldrich
Copy link
Member

I haven't explored this option on the SDK side. My assumption was that the developer may choose to implement this. E.g to capture a screenshot with react-native-view-shot instead of selecting an image with react-native-image-picker as the sample implementation.

Got it. This is nice and flexible, but provides less out of the box value.

Looking at it now I see that we already have captureScreenshot (that convenietly returns Uint8Array) functionality exposed by the native SDKs. I'm not sure if this would be useful though since it will be a screenshot of the feedback form unless we use a screenshot taken before 🤔 Maybe I can explore this on this or a separate PR.

Yes, exactly we already have that function, but as you said taking the screenshot of the form is not useful. But especially when combined with the auto inject, since we will be responsible for showing and hiding the form we can, hide the form take the screenshot and then show it again.

Note: We might anyway also need to create a base64 representation of the screenshot to be able to show a preview using RN Image tag. Or create a tmp file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add attachment/screenshot handling in the feedback form UI
3 participants