Skip to content

Conversation

@tustanivsky
Copy link
Collaborator

@tustanivsky tustanivsky commented Oct 20, 2025

📜 Description

Updated SentryFeedback to accept [Attachment] objects instead of raw [Data] arrays, enabling proper support for
multiple attachments with full metadata (filename, content type, etc.).

This also introduces some breaking changes to public API:

  • init method signature
  • dataDictionary() method now returns attachments as [[String: Any]] with metadata instead of [Data] affecting consumers of the onSubmitSuccess callback

This issue originally came up in Unreal SDK while adding attachment support for user feedback:

💡 Motivation and Context

The previous implementation had several limitations:

  1. Hardcoded metadata: attachmentsForEnvelope() assumed all attachments were screenshots and hardcoded filename: "screenshot.png" and contentType: "application/png"
  2. Single attachment limitation: Only the first attachment was processed despite accepting an array
  3. Lost metadata: Converting Attachment → Data → Attachment discarded important information like custom filenames and content types
  4. Backend supports multiple: The Sentry backend can successfully process and display multiple feedback attachments, but the SDK wasn't taking advantage of this

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

  • I added tests to verify the 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.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

@codecov
Copy link

codecov bot commented Oct 27, 2025

Codecov Report

❌ Patch coverage is 13.33333% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.105%. Comparing base (68d6f41) to head (f2f059c).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ift/Integrations/UserFeedback/SentryFeedback.swift 16.666% 10 Missing ⚠️
...UserFeedback/SentryUserFeedbackFormViewModel.swift 0.000% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #6459       +/-   ##
=============================================
- Coverage   85.145%   85.105%   -0.041%     
=============================================
  Files          453       453               
  Lines        27689     27695        +6     
  Branches     12111     12118        +7     
=============================================
- Hits         23576     23570        -6     
- Misses        4069      4082       +13     
+ Partials        44        43        -1     
Files with missing lines Coverage Δ
...UserFeedback/SentryUserFeedbackFormViewModel.swift 84.384% <0.000%> (ø)
...ift/Integrations/UserFeedback/SentryFeedback.swift 59.183% <16.666%> (-15.235%) ⬇️

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68d6f41...f2f059c. Read the comment docs.

@itaybre itaybre closed this Oct 27, 2025
@itaybre itaybre reopened this Oct 27, 2025
@itaybre
Copy link
Contributor

itaybre commented Oct 27, 2025

(Closed and reopened this to retrigger workflows)

@tustanivsky
Copy link
Collaborator Author

(Closed and reopened this to retrigger workflows)

@itaybre Apparently running workflows require manual approval

@tustanivsky tustanivsky marked this pull request as ready for review November 10, 2025 09:29
Copy link
Member

@philprime philprime left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@philprime
Copy link
Member

Looks like you need to fix the Changelog due to the alpha release of v9

@tustanivsky
Copy link
Collaborator Author

tustanivsky commented Nov 10, 2025

Looks like you need to fix the Changelog due to the alpha release of v9

Right, moved the new changelog entry to Unreleased section (d5d2e20).

@tustanivsky
Copy link
Collaborator Author

@philprime I see that some CI checks are failing due to the missing ready-to-merge label - not sure if it needs to be added manually or is handled by some automation.

@philprime philprime added the ready-to-merge Use this label to trigger all PR workflows label Nov 10, 2025
@philprime
Copy link
Member

I'll try to push a commit to this PR so hopefully it uses my permissions and exposes the GitHub secrets

@philprime
Copy link
Member

We probably have to reopen this PR as soon as you repo write access

@tustanivsky
Copy link
Collaborator Author

We probably have to reopen this PR as soon as you repo write access

Right, I’ll re-create it once I receive the invite (still waiting for it)

@tustanivsky
Copy link
Collaborator Author

Superseded by #6752

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Use this label to trigger all PR workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants