Skip to content

feat: [Re-Discover] Polishing#7155

Merged
sandrade-dcl merged 65 commits intodevfrom
feat/re-discover-polishing
Feb 24, 2026
Merged

feat: [Re-Discover] Polishing#7155
sandrade-dcl merged 65 commits intodevfrom
feat/re-discover-polishing

Conversation

@sandrade-dcl
Copy link
Contributor

@sandrade-dcl sandrade-dcl commented Feb 18, 2026

Pull Request Description

Fix #6830

What does this PR change?

This PR is part of the "Re-Discover" shape's implementation.
If fixes all the missing features and polishing that we had listed here.

Test Steps

  1. Execute the build using these app args: --debug --discover.
  2. Check that everything works as expected.

Code Review Reference

Please review our Code Review Standards before submitting.

@sandrade-dcl sandrade-dcl self-assigned this Feb 18, 2026
@sandrade-dcl sandrade-dcl added the shape-up It has been shaped and is awaiting the cycle. label Feb 18, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Feb 18, 2026

badge

Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below.

Name Link
Commit 071b3cf
Logs https://github.com/decentraland/unity-explorer/actions/runs/22372278110
Download Windows https://github.com/decentraland/unity-explorer/suites/58447035843/artifacts/5644378086
Download Windows S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/feat/re-discover-polishing/pr-19083-071b3cf/Decentraland_windows64.zip
Download Mac https://github.com/decentraland/unity-explorer/suites/58447035843/artifacts/5644221562
Download Mac S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/feat/re-discover-polishing/pr-19083-071b3cf/Decentraland_macos.zip
Built on 2026-02-24T22:58:09Z

Artifact size check

Target Size (MB / MiB) Cap (MB) Δ MB vs release Δ % vs release Result
windows64 317.8 / 303.1 320 0 -0.06% ✅ OK
macos 408.1 / 389.2 410 0 0.02% ✅ OK

Comparing against: v0.126.0-alpha

Base automatically changed from feat/live-events-counter to dev February 18, 2026 21:07
@balducciv
Copy link

balducciv commented Feb 20, 2026

PR reviewed and approved by QA on both platforms following instructions. ✅
Smoke test for this PR was performed on Windows and Mac to verify that the normal flow is working as expected ✅

  • BUG
    The "Featured" mark in the place card is based on the "featured" category of the place and not on the highlighted property.

  • BUG
    The "EXPLORE PLACES" button when an empty search doesn't work (context)

  • Spinner
    While loading the next batch of cards, show a bigger space and spinner at the bottom. You can find the Figma reference here.

  • Show tooltip wheen hover on the LIVE tag -> This is not working due to an issue with the BE endpoint
    Show the event's name.

  • Show tooltip when hover on a connected friend thumbnail (mini-card and modal)
    Show the friend's name.

  • Support friends connected in "Last Visited" section
    Integrate this new endpoint.

  • In the Place Detail Modal, if we click on the creator thumbnail the passport will open
    Note: There are some creator thumbnails that, clicking on them, the passport doesn’t open: This is because there are some places that come from the back-end without the owner id information. In these cases, as we cannot have the wallet id available, we cannot open it. As you can also see, these cases usually coincide with the places where we are not loading a thumbnail for its creator (it’s due to the same thing). It’s a expected behavior

  • Show wallet address in the detail modal when we don't receive the contact name
    Comment in Figma: HERE

  • Explore section
    Remove the category title, since it is redundant.

  • Update filter name: from Recommender Only to Compatible Only

  • Integrate animations of Places and Events buttons in both: Sidebar + Explore menu

  • Events show correct number of current live events

The testing of this PR has been discussed here

My Places section:
the card shows an old thumbnail (world was re-deployed using a different scene): This is a BE issue that is being fixed soon.
The number of live events show as 2, but only 1 event shows on the events tab -> fixed on the latest commit

PR-7155.windows.mp4
24.02.2026_14.32.23_REC.7155.mp4

Copy link
Contributor

@fcolarich fcolarich left a comment

Choose a reason for hiding this comment

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

Great work!! 💪

@claude
Copy link

claude bot commented Feb 24, 2026

Claude encountered an error —— View job


Code Review in Progress

  • Gather PR diff and changed files
  • Review C# source files for bugs, anti-patterns, and code quality issues
  • Check for ECS violations per CLAUDE.md standards
  • Post inline comments for specific issues
  • Post final summary

@claude
Copy link

claude bot commented Feb 24, 2026

Claude finished @balducciv's task in 3m 23s —— View job


PR Review

  • Gather PR diff and changed files
  • Review C# source files for code quality issues
  • Check for bugs, performance issues, and anti-patterns
  • Post inline comments for specific issues
  • Post summary of findings

Summary

Found 5 issues requiring fixes: 3 bugs (loading state stuck, potential IndexOutOfRangeException, potential FormatException) and 2 code quality issues (dead code, O(n²) algorithm).


Issues Found

1. Loading state never cleaned up on early returns — PlacesResultsController.cs

Location: PlacesResultsController.cs:254-256, 310-311, 313-317

Problem: LoadPlacesAsync sets isPlacesGridLoadingItems = true and shows a spinner at the start, but three early-return paths never clean them up:

// Path 1: null profile
Profile? ownProfile = await selfProfile.ProfileAsync(ct);
if (ownProfile == null)
    return;  // isPlacesGridLoadingItems stays true, spinner stays visible

// Path 2: cancellation
if (ct.IsCancellationRequested)
    return;  // same

// Path 3: API failure
if (!placesResult.Success)
{
    NotificationsBusController.Instance.AddNotification(...);
    return;  // same
}

The cleanup block at lines 352–357 is only reached on success. The UI gets stuck in a loading state until the panel is closed and reopened.

Fix: Add cleanup before each early return, or use a try/finally block to guarantee cleanup. Fix this →


2. Potential IndexOutOfRangeException on short owner address — PlaceDetailPanelView.cs:178

Location: PlaceDetailPanelView.cs:178

Problem:

creatorWalletText.text = !string.IsNullOrEmpty(placeInfo.owner)
    ? $"{placeInfo.owner[..5]}...{placeInfo.owner[^5..]}"
    : "Unknown";

[..5] and [^5..] throw ArgumentOutOfRangeException if placeInfo.owner is non-empty but has fewer than 5 characters. Ethereum addresses are 42 chars in practice, but the API response has no contract guaranteeing this length, making this fragile.

Fix:

creatorWalletText.text = !string.IsNullOrEmpty(placeInfo.owner) && placeInfo.owner.Length >= 10
    ? $"{placeInfo.owner[..5]}...{placeInfo.owner[^5..]}"
    : placeInfo.owner ?? "Unknown";

Fix this →


3. DateTimeOffset.Parse can throw FormatExceptionPlaceDetailPanelView.cs:192

Location: PlaceDetailPanelView.cs:192

Problem:

updatedDateText.text = !string.IsNullOrEmpty(placeInfo.updated_at)
    ? DateTimeOffset.Parse(placeInfo.updated_at).ToString("dd/MM/yyyy")
    : "-";

DateTimeOffset.Parse throws FormatException if updated_at contains a malformed string from the API. This would crash the calling code and leave the panel in a bad state.

Fix: Use TryParse:

updatedDateText.text = DateTimeOffset.TryParse(placeInfo.updated_at, out var date)
    ? date.ToString("dd/MM/yyyy")
    : "-";

Fix this →


4. Dead code in PlacesAPIClient.cs — unreachable null check

Location: PlacesAPIClient.cs:340-341 and 376-377

Problem: Both GetPlacesByIdsAsync and GetDestinationsByIdsAsync have a dead code block that can never execute:

if (placeIdsList.Count == 0)
    return new PlacesData.PlacesAPIResponse() { ... };  // early return on line 323

// ... build jsonBody ...

if (placeIdsList.Count == 0)  // unreachable — already returned above
    jsonBody.Clear();

The second Count == 0 check at lines 340 and 376 is never reached.

Fix: Remove the dead code blocks on lines 340–341 and 376–377. Fix this →


5. O(n²) sort in GetRecentlyVisitedPlacesSortedPlacesResultsController.cs:360

Location: PlacesResultsController.cs:367-377

Problem: Nested loops to sort recently visited places by the original ID order:

foreach (string placeId in sortedPlacesIds)
{
    foreach (PlacesData.PlaceInfo placeInfo in placesResult.Value.Data)  // O(n*m)
    {
        if (placeInfo.id != placeId) continue;
        sortedPlacesResponse.data.Add(placeInfo);
        break;
    }
}

This is O(n×m) where n = sorted IDs count and m = returned places count. While the page size is capped at 20, if recently visited history grows this degrades unnecessarily.

Fix: Build a Dictionary<string, PlaceInfo> from the API response first (O(n+m)):

var placeById = placesResult.Value.Data.ToDictionary(p => p.id);
foreach (string placeId in sortedPlacesIds)
    if (placeById.TryGetValue(placeId, out var placeInfo))
        sortedPlacesResponse.data.Add(placeInfo);

Fix this →

@sandrade-dcl
Copy link
Contributor Author

@claude re-review, I applied all your suggested changes except the point 1 because, although you assume that's an issue, it is the expected behavior that I want 👍

Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Re-review after the 'Apply feedback from Claude' commit (3208283).

Issues 2-5 — Addressed

The author confirmed applying the fixes for:

  • Bug 2 (PlaceDetailPanelView.cs:178) — IndexOutOfRangeException guard on short owner strings
  • Bug 3 (PlaceDetailPanelView.cs:192) — DateTimeOffset.TryParse instead of Parse
  • Code Quality 4 (PlacesAPIClient.cs) — Dead unreachable placeIdsList.Count == 0 blocks removed
  • Code Quality 5 (PlacesResultsController.cs) — O(n2) nested loop replaced with Dictionary lookup

Issue 1 — Loading state on early returns (intentional)

The author explained that keeping the spinner active on the three early-return paths in LoadPlacesAsync is intentional. For the cancellation path this is reasonable (a new load follows immediately). For the null-profile and API-failure paths the spinner remains visible with no in-panel feedback, but the QA reviewer verified the behaviour on both platforms and approved. Accepted as a deliberate design decision.

Approving.

@sandrade-dcl sandrade-dcl enabled auto-merge (squash) February 24, 2026 22:24
@sandrade-dcl sandrade-dcl merged commit bf1fcbf into dev Feb 24, 2026
10 checks passed
@sandrade-dcl sandrade-dcl deleted the feat/re-discover-polishing branch February 24, 2026 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

shape-up It has been shaped and is awaiting the cycle.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Re-Discover / Places Section] Polishing

4 participants