Skip to content

fix: generate png thumbnail previews for svg files - #31883

Merged
kodiakhq[bot] merged 9 commits into
developfrom
fix/svg-image-preview-issue
Mar 15, 2024
Merged

fix: generate png thumbnail previews for svg files#31883
kodiakhq[bot] merged 9 commits into
developfrom
fix/svg-image-preview-issue

Conversation

@abhinavkrin

@abhinavkrin abhinavkrin commented Mar 4, 2024

Copy link
Copy Markdown
Member

Proposed changes (including videos or screenshots)

Fixed an issue where SVG thumbnails were incorrectly handled as PNGs, causing preview errors. Now, the system correctly identifies and presents the generated PNG thumbnails, allowing for proper SVG image previews.

Issue(s)

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

  1. Go to File Upload > Blocked Media Types, leave the field blank.
  2. Upload any .svg image file.
  3. The SVG image should now preview correctly without reverting to a retry screen.

Further comments

SUP-495

Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
@changeset-bot

changeset-bot Bot commented Mar 4, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fec0686

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 31 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/models Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov

codecov Bot commented Mar 4, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.50%. Comparing base (44721d7) to head (fec0686).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop   #31883   +/-   ##
========================================
  Coverage    54.50%   54.50%           
========================================
  Files         2294     2294           
  Lines        50562    50562           
  Branches     10325    10325           
========================================
  Hits         27561    27561           
  Misses       20501    20501           
  Partials      2500     2500           
Flag Coverage Δ
e2e 53.50% <ø> (ø)
e2e-api 40.15% <ø> (ø)
unit 75.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@abhinavkrin
abhinavkrin marked this pull request as ready for review March 4, 2024 17:10
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
@scuciatto scuciatto added this to the 7.0 milestone Mar 7, 2024

@MarcosSpessatto MarcosSpessatto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could add a test case to cover this. We can get the message and see if there is one of the files with the desired extension.

Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
@abhinavkrin
abhinavkrin requested a review from a team as a code owner March 12, 2024 12:00
Comment thread apps/meteor/app/file-upload/server/lib/FileUpload.ts Outdated
Comment thread apps/meteor/app/file-upload/server/lib/FileUpload.ts Outdated
@debdutdeb debdutdeb changed the title fix: Resolved SVG Image Preview Issue fix: generate png thumbnail previews for svg files Mar 12, 2024
@debdutdeb

Copy link
Copy Markdown
Member

Also, I'm seeing a lot of ||s ..not all just from this pr, but at what point we should just deny the process instead of adding fallback?

for example, what are the chances of a file getting uploaded without a filename, shouldn't we fail the action if there is no name? Then wouldn't have to fallback onto an empty string there.

Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
@abhinavkrin

Copy link
Copy Markdown
Member Author

Also, I'm seeing a lot of ||s ..not all just from this pr, but at what point we should just deny the process instead of adding fallback?

for example, what are the chances of a file getting uploaded without a filename, shouldn't we fail the action if there is no name? Then wouldn't have to fallback onto an empty string there.

It seems we cannot upload a file without a name. So, I can remove the fallback

Comment thread apps/meteor/app/file-upload/server/lib/FileUpload.ts Outdated
Comment thread apps/meteor/app/file-upload/server/lib/FileUpload.ts Outdated
Comment thread apps/meteor/app/file-upload/server/methods/sendFileMessage.ts Outdated
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Comment thread apps/meteor/tests/end-to-end/api/09-rooms.js
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
@scuciatto scuciatto removed this from the 7.0 milestone Mar 14, 2024

@debdutdeb debdutdeb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

code lgtm. didn't test it. 👍

@dionisio-bot dionisio-bot Bot added the stat: ready to merge PR tested and approved waiting for merge label Mar 15, 2024
@dionisio-bot dionisio-bot Bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Mar 15, 2024
@kodiakhq
kodiakhq Bot merged commit 38a2036 into develop Mar 15, 2024
@kodiakhq
kodiakhq Bot deleted the fix/svg-image-preview-issue branch March 15, 2024 19:46
@scuciatto scuciatto added this to the 6.7 milestone Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants