Conversation
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
sandrade-dcl
changed the title
…for the reporting confirmation dialog
|
Claude finished @sandrade-dcl's task in 5m 5s —— View job PR Review — feat: "Report User" feature
SummaryThe implementation is well-structured overall. The feature flag gating, URL formatting, and confirmation dialog flow are clean. Two issues require attention: Issues Found1. Missing exception handling — The See inline comment for the suggested fix. Fix this → 2. Empty The See inline comment for the suggested fix. Fix this → REVIEW_RESULT: FAIL ❌ |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Explorer/Assets/DCL/ApplicationsGuards/ApplicationBlocklistGuard/BannedNotificationHandler.cs
Show resolved
Hide resolved
Explorer/Assets/DCL/ApplicationsGuards/ApplicationBlocklistGuard/BlockedScreenController.cs
Show resolved
Hide resolved
…rd/BannedNotificationHandler.cs Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Signed-off-by: Santi Andrade <sandrade@decentraland.org>
|
@claude re-review |
|
Claude finished @sandrade-dcl's task in 1m 55s —— View job PR Re-Review — feat: "Report User" feature
Previously Raised Issues — Status
New Blocking Issue FoundCompilation error: dangling block in The try-catch fix was applied inside
This will not compile. The fix is to delete lines 75–85 (the dangling block). See inline comment for the exact change. Fix this → REVIEW_RESULT: FAIL ❌ |
Explorer/Assets/DCL/ApplicationsGuards/ApplicationBlocklistGuard/BannedNotificationHandler.cs
Outdated
Show resolved
Hide resolved
|
@claude all suggested changes applied! |
|
@claude re-review |
|
Claude finished @sandrade-dcl's task in 2m 39s —— View job PR Re-Review — feat: "Report User" feature
Previously Raised Issues — Status
New Blocking Issue FoundMissing exception handling in all Every new Affected locations (all the same fix — wrap the body in try/catch):
See the inline comment on REVIEW_RESULT: FAIL ❌ |
|
@claude re-review |
|
Claude finished @sandrade-dcl's task in 1m 3s —— View job PR Re-Review — feat: "Report User" feature
Previously Raised Issues — Status
Remaining Issue
The fix was applied to all four other See inline comment for the exact fix. Fix this → REVIEW_RESULT: FAIL ❌ |
Explorer/Assets/DCL/Communities/CommunitiesCard/Members/MembersListController.cs
Show resolved
Hide resolved
|
@claude re-review |
|
Claude finished @sandrade-dcl's task in 32s —— View job PR Re-Review — feat: "Report User" feature
Previously Raised Issues — Status
All previously raised issues have been addressed. No new blocking issues found. REVIEW_RESULT: PASS ✅ |
* Replace bans url * Replace ReportUserForm url --------- Co-authored-by: Santi Andrade <sandrade@decentraland.org>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review: Report User Feature
Overall
This is a substantial feature adding user reporting, ban notifications, and an enhanced blocked screen across multiple surfaces (passport, friends list, communities, context menus). The feature is properly gated behind the REPORT_USER feature flag with a --report-user app arg override. The overall architecture is sound, but there are several issues worth addressing.
🔴 Issues
1. Duplicated report-user logic (DRY violation)
The "show confirmation dialog → fetch self profile → open web form" pattern is copy-pasted nearly identically across 5+ controllers:
PassportController.ReportUserClickedGenericUserProfileContextMenuController.OnReportUserClickedMembersListController.ReportUserClickedAsyncRequestsSectionController.ReportUserClickedCommunitiesBrowserView.OnReportUser+CommunitiesBrowserController.OpenReportUserForm
Each one creates its own CancellationTokenSource, calls ReportUserConfirmationDialog.ShowAsync, fetches self profile, and formats the URL. This should be extracted into a shared helper (e.g. a static method on ReportUserConfirmationDialog or a dedicated ReportUserService) that takes IConfirmationDialogOpener, ISelfProfile, IWebBrowser, IDecentralandUrlsSource, a reported user ID, and a CancellationToken.
2. BannedNotificationHandler doesn't unsubscribe from notification bus
Dispose() only calls cts.SafeCancelAndDispose(), but never unsubscribes from NotificationsBusController.Instance.SubscribeToNotificationTypeClick. If the notification bus holds a strong reference to the handler's callbacks, this is a memory leak.
3. MembersListView.contextMenuSettings changed from private to public
The [field: SerializeField] property was changed from private to public just so MembersListController.ReportUserClickedAsync can access view.contextMenuSettings.ReportSprite. This breaks encapsulation. Consider exposing only the needed sprite via a read-only property instead.
🟡 Suggestions
4. Hardcoded color parsed at runtime in multiple places
ColorUtility.TryParseHtmlString("#FF2D55", out Color redColor) is called in at least 5 different spots (PassportController, GenericUserProfileContextMenuController, MembersListView, CommunityRequestsReceivedGroupView, RequestsSectionController). Consider defining a shared static readonly Color constant to avoid redundant parsing and ensure consistency.
5. Default color check in ButtonWithDelegateContextMenuControlSettings
Using textColor == default(Color) to detect whether a color was provided is fragile — default(Color) is (0,0,0,0) (transparent black), which could be a valid intentional value. A nullable Color? parameter would be safer, though this is a minor concern for current usage.
6. AnimatePersistentNotificationCanvasGroupAsync — DOFade in finally without cancellation token
In the finally block, notificationCanvasGroup.DOFade(0, ANIMATION_DURATION).ToUniTask() runs without a cancellation token. If the notification system is being torn down, this tween could run against a destroyed object. Consider guarding this or passing a fallback token.
7. Inconsistent confirmation dialog ownership
Most surfaces handle the confirmation dialog in the controller layer, but CommunitiesBrowserView.OnReportUser handles it in the view layer and then invokes ReportUserRequested to the controller for the URL open. This architectural inconsistency makes the flow harder to follow. (Would be naturally solved by extracting the shared helper from point #1.)
✅ What looks good
- Feature flag integration is clean —
FeatureId.REPORT_USERwith both FF and app arg support. ModerationDataProvidercleanly encapsulates the new API call using signed fetch.BlockedScreenController.FormatRemainingBanTimehandles edge cases well (expired bans, hour/day formatting).- Proper CTS lifecycle management throughout (
SafeRestart,SafeCancelAndDisposeon close/dispose). GetBanStatusDatagracefully falls back toisBanned = falseon API failure during startup.- Notification types (BAN_WARNING, BANNED, BAN_LIFTED) are well-structured with persistent display behavior.
⚠️ CI Status
- Test (playmode): ❌ FAILURE
- Test (editmode): ❌ CANCELLED
- Build: 🔄 IN PROGRESS
The playmode test failure needs investigation before merge.
📝 Missing Tests
No tests were added for the new code. At minimum, unit tests for FormatRemainingBanTime, ModerationDataProvider, and ReportUserConfirmationDialog would be valuable.
Requested by Santiago Andrade Lara via Slack
3 participants
Pull Request Description
Fix #7299
Fix #7300
Fix #7479
What does this PR change?
It implements the Report User feature in the Explorer side:
Ban Warningnotification: The player will receive a notification informing about a possible banning if he continues violating the rules. Clicking on it, the user will be redirected to the support webpage.Bannednotification: The player will receive a notification informing about he was banned. Clicking on it, the "Blocked Screen" modal will be opened.Test Steps
Report an user
--debug --report-user --dclenv zone.When your user is flagged
--debug --report-user --dclenv zone.When your user is banned
--debug --report-user --dclenv zone.Check that the feature flag is working properly
--report-user.Code Review Reference
Please review our Code Review Standards before submitting.