Skip to content

feat(screenshots): Update render logic #93211

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

Conversation

priscilawebdev
Copy link
Member

@priscilawebdev priscilawebdev commented Jun 10, 2025

Contributes to #92154

This PR updates our logic for rendering screenshots, essentially removing the extension checks to match what the backend does.

There is a catch. The frontend also checks the mimytpe of the file, and based on that we can either render something or not render anything.

So the file will be rendered if the name contains "screenshot" and if it is one of the file formats we currently support. A new issue will be created where we will introduce files in newer formats, but that is for a follow-up.

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jun 10, 2025
@priscilawebdev priscilawebdev changed the title Priscila/feat/screenshots/render file if name contains screenshot feat(screenshots): Update render logic Jun 10, 2025
@priscilawebdev priscilawebdev requested review from a team and bruno-garcia June 10, 2025 10:07
Copy link

codecov bot commented Jun 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #93211      +/-   ##
==========================================
- Coverage   85.31%   82.91%   -2.41%     
==========================================
  Files       10256    10255       -1     
  Lines      591639   591622      -17     
  Branches    22990    22986       -4     
==========================================
- Hits       504768   490517   -14251     
- Misses      86380   100465   +14085     
- Partials      491      640     +149     

@priscilawebdev priscilawebdev marked this pull request as ready for review June 10, 2025 10:52

export function WebMViewer(props: WebMViewerProps) {
export function WebMViewer({controls = true, onCanPlay, ...props}: WebMViewerProps) {
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity: Why do we limit this to WebM? Couldn't we try to display any video type?

Copy link
Member Author

Choose a reason for hiding this comment

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

We must rethink these components and refactor them. Agree that we could have something like VideoViewer or something.

@priscilawebdev priscilawebdev merged commit 3096a2b into master Jun 10, 2025
42 checks passed
@priscilawebdev priscilawebdev deleted the priscila/feat/screenshots/render-file-if-name-contains-screenshot branch June 10, 2025 11:05
scttcper added a commit that referenced this pull request Jun 18, 2025
We wanted to open this up the screenshot viewer (in #93211) to anyone uploading a screenshot.png but we've been erroring when the mimetype is not in our short list of allowed image mimetypes attempting to render undefined.

Also fixes a bug where I removed the rrweb viewer in #93359
scttcper added a commit that referenced this pull request Jun 18, 2025
We wanted to open this up the screenshot viewer (in
#93211) to anyone uploading a
screenshot.png but we've been erroring when the mimetype is not in our
short list of allowed image mimetypes and attempting to render
undefined.

Also fixes a bug where I removed the rrweb viewer in
#93359
andrewshie-sentry pushed a commit that referenced this pull request Jun 19, 2025
Contributes to #92154

This PR updates our logic for rendering screenshots, essentially
removing the extension checks to match what the backend
[does](138af3b).
andrewshie-sentry pushed a commit that referenced this pull request Jun 19, 2025
We wanted to open this up the screenshot viewer (in
#93211) to anyone uploading a
screenshot.png but we've been erroring when the mimetype is not in our
short list of allowed image mimetypes and attempting to render
undefined.

Also fixes a bug where I removed the rrweb viewer in
#93359
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants