fix: non-deterministic event ordering in Genesis Plaza map panel - #9599
fix: non-deterministic event ordering in Genesis Plaza map panel#9599eordano wants to merge 4 commits into
Conversation
PlaceInfoPanelController's events list bound pooled EventElementView rows without ever calling SetAsLastSibling(), so the LIFO object pool left rows in whatever sibling slot they last occupied — the on-screen order permuted on every reopen instead of following the fetched data. The fetch was also never sorted, so events appeared in raw server order (next_start_at) while the panel displays start_at. Sort events (live first, then by NextStartAtProcessed) before binding, and call transform.SetAsLastSibling() after every pool Get() in both the population loop and the loading-skeleton path, mirroring the existing idiom in SearchResultPanelController. Fixes #9529 Includes a regression test that fails without this fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Build skipped — no changes detected under |
decentraland-bot
added
the
ext-contribution
|
Slack notification sent to #explorer-ext-contributions for external review. |
|
Lint did not finish ( |
|
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: fix: non-deterministic event ordering in Genesis Plaza map panel
STEP 2 — Root-cause check: PASS ✅
The PR correctly identifies two independent defects:
- Pool reuse ordering —
ObjectPool<EventElementView>is LIFO; released rows keep their previous sibling index, so a reopen permutes on-screen order relative to the data. - Missing client-side sort — the panel displayed events in whatever order the API returned them, showing
start_at(series start) while the backend orders bynext_start_at.
The fix addresses both root causes directly: it sorts before binding and repositions after every pool retrieval. This is not a symptom-level patch.
STEP 3 — Design & integration: PASS ✅
No new long-lived units introduced. All changes are localized within the existing FetchAndShowEventsOfThePlace() method of PlaceInfoPanelController, which already owns the event display lifecycle. No new subscriptions, connections, or lifecycle management added.
The SetAsLastSibling() idiom mirrors existing usage in SearchResultPanelController.cs (confirmed — placeElementView.transform.SetAsLastSibling() after pool retrieval). Consistent with codebase patterns.
Teardown trace: No new subscriptions or connections added. The existing ClearEventElements() teardown path (which releases pool elements and removes listeners) is unchanged and already covers the new code path — SetAsLastSibling() is a fire-and-forget Transform operation with no corresponding unsubscribe.
STEP 4 — Member audit: PASS ✅
No public properties or accessors added or changed. All modifications are to private method bodies.
STEP 5 — Line-level review
One P2 finding — see inline comment on the sort comparator.
Allocation note: new List<EventDTO>(events) allocates a copy of the events list. This is acceptable here — it runs in a user-triggered async path (clicking the Events tab), not in a per-frame system Update(). The sort delegate captures no external variables, so the compiler caches it as a static delegate (no allocation). EventDTO is a struct, so the list copy is by-value — fine for the typical single-digit event count per place.
Test observations (non-blocking):
- The test uses
FormatterServices.GetUninitializedObjectto bypass the heavy constructor — precedented in the repo (NearbyAudioPerformanceManualTest.cs). - The test covers the loading-skeleton
SetAsLastSiblingfix. It does not additionally drive the sort +SetAsLastSiblingin the main fetch-result loop — the PR explicitly acknowledges this gap and explains the rationale. Acceptable for a regression test scoped to the pool-reuse bug. - Reflection into private members (
FetchAndShowEventsOfThePlace,eventElements) is fragile but pragmatic for this use case.
Security review: No security issues found. This is a client-side UI sorting fix with no input handling, authentication, authorization, or data exposure changes.
STEP 6 — Complexity
SIMPLE — Touches 1 production file (~10 lines of meaningful change) + 1 test file. No ECS systems, async pattern changes, or cross-assembly modifications.
STEP 7 — QA assessment
YES — Changes affect what the user sees (event ordering in the map panel Events tab). Modifies runtime code under Explorer/.
STEP 8 — Non-blocking warnings
None. Main.unity is not in the changed files.
CI Status
Several checks are still pending (Test (editmode), Test (playmode), Prebuild). The Lint and watchdog failures appear to be infrastructure-related (Docker disk space cancellation), not caused by this PR's changes. enforce-approvals is waiting on QA/DEV approval (expected). semantic / title-matches-convention passes — the PR title follows fix: convention.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Localized UI fix in a single controller method — sorts events and repositions pooled views after retrieval, no ECS/async/system changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown via Slack
| foreach (EventDTO @event in events) | ||
| // Deterministic order: live events first, then by next occurrence (the field the backend sorts by). | ||
| List<EventDTO> sortedEvents = new (events); | ||
| sortedEvents.Sort((a, b) => a.live != b.live ? (a.live ? -1 : 1) : a.NextStartAtProcessed.CompareTo(b.NextStartAtProcessed)); |
There was a problem hiding this comment.
[P2] Sort stability: List<T>.Sort is not stable in .NET — events with identical live status and NextStartAtProcessed values can still appear in non-deterministic order across reopens, which doesn't fully deliver on the "deterministic ordering" promise. Adding a tiebreaker by id (a unique string) makes the sort fully deterministic.
| sortedEvents.Sort((a, b) => a.live != b.live ? (a.live ? -1 : 1) : a.NextStartAtProcessed.CompareTo(b.NextStartAtProcessed)); | |
| sortedEvents.Sort((a, b) => | |
| { | |
| if (a.live != b.live) return a.live ? -1 : 1; | |
| int cmp = a.NextStartAtProcessed.CompareTo(b.NextStartAtProcessed); | |
| return cmp != 0 ? cmp : string.Compare(a.id, b.id, StringComparison.Ordinal); | |
| }); |
mikhail-dcl
left a comment
mikhail-dcl
left a comment
There was a problem hiding this comment.
It would be better to move sorting to backend so it's unified across all consumers
|
|
||
| foreach (EventDTO @event in events) | ||
| // Deterministic order: live events first, then by next occurrence (the field the backend sorts by). | ||
| List<EventDTO> sortedEvents = new (events); |
There was a problem hiding this comment.
Avoid allocation of a new collection
| foreach (EventDTO @event in events) | ||
| // Deterministic order: live events first, then by next occurrence (the field the backend sorts by). | ||
| List<EventDTO> sortedEvents = new (events); | ||
| sortedEvents.Sort((a, b) => a.live != b.live ? (a.live ? -1 : 1) : a.NextStartAtProcessed.CompareTo(b.NextStartAtProcessed)); |
There was a problem hiding this comment.
Sort is unstable as well
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings count reduced: 13156 => 13137 Warnings/errors in files changed by this PR (15)All Unity tests passed ✅
|
Address review feedback on the ordering fix: - GetEventsByParcelAsync(parcels) and FetchEventListAsync now return the freshly deserialized EventDTO[] directly, so the Events tab orders it with Array.Sort on the array it already owns - no copy of the response list. - The order lives in EventDisplayOrderComparer (live first, then soonest next occurrence, then id). The id tiebreaker makes the order total, so the unstable Array.Sort cannot reshuffle equal keys between reopens. EventDisplayOrderComparerShould pins the contract, including identical-key ties under every input rotation. - Lint cleanup in touched files: EventDTO.cs gets the documented wire-format DTO naming suppression plus a schema link, and a stray double semicolon in HttpEventsApiService is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t comments nickkhalow's review blocked on the diff-ratchet lint BLOCK for `place!.Positions` at PlaceInfoPanelController.cs:439: the diff retyped the fetch's return value but re-touched a pre-existing null-forgiving dereference instead of guarding it. Root cause is that FetchEventsAndShowThemAsync never establishes that `place` is non-null before using it, unlike sibling methods in this file (SetOriginParcel, StartNavigation) which already guard with `if (place == null) return;`. Add the same guard and capture the narrowed value into a local before the SetAsLoadingState() call runs (a field's narrowed null-state does not survive a method call), so the fetch call site no longer needs `!`. The file's other six pre-existing `place!` occurrences are left untouched as out of scope for this PR. Also trims PlaceInfoPanelControllerEventOrderingShould.cs's three comment blocks (class doc + two SetUp asides) down to this repo's established 1-3 line regression-comment convention (JumpButtonShould.cs, LiveKitChatMessagesBusShould.cs, DebugWebRequestInfoShould.cs); the narrative content they trimmed was independently verified accurate but belongs in the PR description, not the diff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018c638dR1vPysCMbYt2qQg5
|
PR #9599, run #32035092962 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
|
3 participants
Symptom
Opening the map, clicking Genesis Plaza, and switching to the Events tab shows the place's events in non-chronological order, and the order is different every time the tab is reopened (100% repro).
Root cause
Two independent defects combine. First,
PlaceInfoPanelControllertakes rows from a pooledEventElementViewobject pool without ever callingSetAsLastSibling(); the pool is LIFO, so a reused row keeps whatever sibling slot it occupied on a previous fill, and the loading-skeletonGet/release cycle on every reopen produces a fresh permutation of the data-to-row mapping each time. Second, the panel never sorts the fetched events itself and displaysstart_at(the recurring event's original series start) while the backend orders bynext_start_at, so even a stable response looks chronologically scrambled.Fix
HttpEventsApiService.GetEventsByParcelAsync(parcels)andFetchEventListAsyncnow return the freshly deserializedEventDTO[]directly, andPlaceInfoPanelControllerorders it in place withArray.Sorton the array it already owns — no copy of the response list. The order is total, defined byEventDisplayOrderComparer(live events first, then soonestNextStartAtProcessed, thenid): the id tiebreaker means the unstableArray.Sortcannot reshuffle equal keys between reopens.element.transform.SetAsLastSibling()is called immediately after everyeventElementPool.Get()— both in the population loop and in the loading-skeleton path — mirroring the identical idiom already used bySearchResultPanelController.Tests
EventDisplayOrderComparerShouldpins the order contract (live-first, chronological, id tie-break, and same output for every input permutation — exactly the unstable-sort case), alongside the original pooled-row regression test, which fails at the base pin without the fix and passes with it (validated on v16, Windows Unity 6000.4 EditMode).Also in touched files (warning ratchet):
EventDTO.csnow carries the documented wire-format DTO naming suppression + schema link, and a stray;;inHttpEventsApiServiceis gone (−37 lint warnings).Fixes #9529
🤖 Generated with Claude Code