feat: bug report platform and multiple screenshots - #10064
Open
alejandro-jimenez-dcl wants to merge 4 commits into
Open
feat: bug report platform and multiple screenshots#10064alejandro-jimenez-dcl wants to merge 4 commits into
alejandro-jimenez-dcl wants to merge 4 commits into
Conversation
The submit button's color tint only affects its background, so the SUBMIT text stayed fully white while the form was incomplete. The view now toggles the label alpha together with the button's interactable state.
… ticket The Intercom proxy now takes a "Platform" list code (0 = Desktop; Explorer has no mobile build) and an "evidence" array of up to three images instead of a single object. Each image stays under the 3 MB per-image cap and the set shares a 3 MB budget so the base64 body stays below the proxy's 4.5 MB limit; images that would trip a cap are dropped from the ticket with a warning, in attachment order. The form keeps a list of attached screenshots rendered through BugReportScreenshotSlot entries on the view; the attach button hides once the slots are full. Sentry still receives the first image only, since a feedback envelope carries a single attachment. Requires the proxy branch feat/intercom-platform-and-multiple-screenshots to be deployed first: the current proxy rejects the unknown "Platform" attribute.
The proxy takes 1 for Desktop and 2 for Mobile and rejects 0, the value an unassigned C# enum serializes to, so it never files as Desktop by accident. The client enum was numbered 0/1 and every ticket would have been a 400.
Contributor
🚦 CI StatusWindows and Mac built successfully in Unity Cloud.
Warnings not reduced: 12025 => 12027 — remove at least 3 warnings to merge. No warnings in files changed by this PR — showing general ones you can remove to unblock (50 of 12027)Lint run · took 21m 30s All Unity tests passed ✅
Tests time sums the test cases; Job time is the job's wall clock including checkout, licensing and asset import. Slowest tests
Full report: run summary · results + editor logs: editmode · playmode 🏁 Bare-metal benchmark finished — run #34823074175. Full reportPR #10064, run #34823074175 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Apple M1
Intel Core i5
On demand — comment |
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
Review — PR #10064 feat: bug report platform and multiple screenshots
STEP 2 — Root-cause check: PASS
Feature enhancement: evolves the bug-reporting pipeline from a single screenshot to multiple screenshots and adds a Platform field to Intercom tickets. The changes address a genuine feature need; no symptom-masking.
STEP 3 — Design & integration: PASS
No new long-lived units are introduced. EvidenceImage, IntercomTicketPlatform, and BugReportScreenshotSlot are stateless data carriers.
EvidenceImagevs the pre-existingBugReportImagelooks like near-duplication (both wrapBytes/ContentType), but is justified layering:BugReportImagecarries aTexture2Dtied to the UI lifecycle, whileEvidenceImageis a pure wire-payload type with no Unity display dependency. KeepingTexture2Dout ofBugReportService/IntercomTicketPayloadis the right call.IntercomTicketPlatformis a minimal addition — the proxy requires a non-zero enum code per its doc comment. Starting at 1 is intentional defensive design.BugReportScreenshotSlotis a standard Unity inspector-wiring struct (not overridingreadonly struct—[Serializable]requires mutability for Unity serialization).
TEARDOWN / CONSUMPTION TRACE:
| Opener | Mirror | Location |
|---|---|---|
onClick.AddListener(() => OnRemoveScreenshotClicked(slotIndex)) |
Dies with the view (wired once in OnViewInstantiated) |
BugReportController.cs:119–123 |
attachedImages.Add(picked.Value) |
ClearAttachedImages() → Object.Destroy(preview) + Clear() |
Called from Dispose(), OnBeforeViewShow(), OnViewClose() |
operationsCts |
SafeRestart() on show/close, SafeCancelAndDispose() in Dispose() |
BugReportController.cs:73–74 |
submissionsCts |
SafeCancelAndDispose() in Dispose() |
BugReportController.cs:75 |
EvidenceImage bytes |
base64-encoded in BuildCreateTicketJson, sent to proxy |
IntercomTicketPayload.cs:103 |
No leaks or unconsumed buffers found. ✓
Texture lifecycle across async boundary: Verified that BugReportImage.Bytes is the raw file data captured independently of Preview in OsFileBrowserBugReportImageProvider.PickAsync. When ClearAttachedImages() destroys preview textures while SubmitDraftAsync is in flight, the EvidenceImage[] conversion (which only reads .Bytes/.ContentType) is unaffected. The BugReportDraft's ToArray() snapshot correctly decouples the submission from the controller's mutable list. ✓
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
maxScreenshots (private property) |
2 (AttachScreenshotAsync, RefreshScreenshots) |
Correctly derives limit from both proxy cap and prefab slot count. ✓ |
SetSubmitInteractable(bool) |
1 (RefreshSubmitInteractable) |
Encapsulates two visual updates (button interactable + label alpha) that must stay in sync. Legitimate view method. ✓ |
SetScreenshots(...) |
1 (RefreshScreenshots) |
Legitimate view method handling all slots. ✓ |
SelectEvidenceImages(...) |
1 production + 5 tests | Public static with comprehensive coverage. ✓ |
STEP 5 — Line-level review
No P0 or P1 issues found.
One P2 noted inline (see comment on MAX_EVIDENCE_TOTAL_BYTES).
Slot closure wiring (BugReportController.cs:119–123): for loop with a local int slotIndex copy, wired once in OnViewInstantiated (not per-show). Correct — no closure-over-loop-variable bug, no pooled-rebind anti-pattern. ✓
Evidence format change (single object → array): The payload's evidence key changes from a single {content_type, data} object to an array of them. Tests in IntercomTicketPayloadShould verify both single-image and multi-image serialization, including the array-of-one case. ✓
Validation layers are defense-in-depth:
OsFileBrowserBugReportImageProvidercaps at 10 MB and validates viaTexture2D.LoadImageBugReportControllerlimits tomaxScreenshots(min of proxy cap and prefab slots)BugReportService.SelectEvidenceImagesenforces per-image (3 MB), count (3), and total budget (3 MB) caps
Test coverage is comprehensive: 5 new/updated tests for SelectEvidenceImages edge cases (null, empty, zero-length, oversized, count cap, total budget overflow), 2 platform-code tests, updated controller and payload tests. ✓
STEP 6 — Complexity: COMPLEX
Changes span the bug-reporting pipeline across service, controller, view, payload, and prefab layers, plus the API contract with the Intercom proxy.
STEP 7 — QA: YES
Modifies runtime UI code in the bug report form (screenshot attachment, submit button, evidence serialization). User-facing.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
Security review
- Credentials: Clean. No secrets in diff. The proxy holds the workspace token.
- Input validation: Well-layered (see Step 5 validation analysis). No crash paths — loops are bounds-checked, no unguarded indexing.
- JSON construction: Built via
Dictionary<string,object>+JsonConvert.SerializeObject(Newtonsoft). No string concatenation, no injection vector. - Resource limits: Total upload budget capped at 3 MB before base64. Max 3 images enforced in both UI and service layers.
- Log exposure:
ReportHub.LogWarningcalls log only image index and byte thresholds — no image bytes, PII, or tokens. - Dependencies: No new packages added.
- Prefab: No script/plugin/binary additions; only UI layout changes for screenshot slots.
DEPENDENCY_REVIEW: PASS
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Changes the bug-reporting pipeline across service, controller, view, and payload layers plus the API contract with the Intercom proxy
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
Comment on lines
+60
to
+65
|
|
||
| /// <summary> | ||
| /// Bound for all the images together. The proxy caps the request body at 4.5 MB and base64 inflates | ||
| /// the bytes by a third, so the images share the budget that one image alone may fill. | ||
| /// </summary> | ||
| public const int MAX_EVIDENCE_TOTAL_BYTES = MAX_EVIDENCE_BYTES; |
Contributor
There was a problem hiding this comment.
[P2] The total budget constant aliases the per-image constant (MAX_EVIDENCE_TOTAL_BYTES = MAX_EVIDENCE_BYTES). While the comment explains the derivation (4.5 MB proxy body cap ÷ base64 inflation ≈ 3 MB raw budget), the identical value leaves a reader wondering whether the equality is coincidence or intentional coupling. Consider either inlining a reference to MAX_EVIDENCE_BYTES directly in the running-total check with a comment explaining the shared cap, or adding a note here that the equality is intentional and when each would diverge.
Suggested change
| /// <summary> | |
| /// Bound for all the images together. The proxy caps the request body at 4.5 MB and base64 inflates | |
| /// the bytes by a third, so the images share the budget that one image alone may fill. | |
| /// </summary> | |
| public const int MAX_EVIDENCE_TOTAL_BYTES = MAX_EVIDENCE_BYTES; | |
| /// <summary> | |
| /// Bound for all the images together. Currently equals MAX_EVIDENCE_BYTES because the proxy | |
| /// caps the request body at 4.5 MB and base64 inflates by a third — so the raw budget for all | |
| /// images together is the same 3 MB that one image alone may fill. | |
| /// </summary> | |
| public const int MAX_EVIDENCE_TOTAL_BYTES = MAX_EVIDENCE_BYTES; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
What does this PR change?
Requires the proxy change to be merged and deployed first
https://dcl.tools/ops/ops-lambdas/-/merge_requests/423 teaches the Intercom proxy the
Platformattribute and the new evidence format. Against the proxy currently deployed, every ticket from this branch fails with a validation error becausePlatformis an unknown attribute. Merge order: proxy first, then this PR.Test Instructions
Prerequisites
Test Steps
Report with a screenshot
Report without a screenshot
Oversized screenshot
Submit button state
Additional Testing Notes
Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does. Especially useful for first-time contributors.