fix: openExplorerUi verdict, request_id correlation and stopEmote's success value - #10096
Merged
Merged
Conversation
Brings in request_id on the explorer-ui request and on its result component, plus the item-purchase panel value. The generated ExplorerItemPurchaseResult binding lands unused on purpose: neither the panel nor its result component is implemented.
The verdict was decided on the scene's JS thread and returned before the work that shows the panel had run. Two calls in quick succession both got OPENED although one panel opened, and a user opening the panel inside that window left the scene holding an OPENED that emitted no events at all — an await on the panel's life cycle never finished. The restricted action is now asynchronous: it hops to the main thread, decides there, answers, and shows. The slot a request has handed to MVC counts as taken, because MVC reports a panel as showing only once the view's life cycle has begun, which is later than the call to ShowAsync. The cheap gates stay synchronous, so refusing a misbehaving scene still costs no frame. There is no per-call cancellation token on purpose: a second call is a legitimate request owed WasAlreadyOpen, and the house pattern of restarting a shared token would cancel the first one instead.
…n tick A scene could not tell which of its own calls a panel lifecycle event belonged to. The id was dropped at the JS module boundary, which forwarded only the panel value, and the echo field on the result component was never written, so two waits live on the same panel settled on one close. The id now travels from the request across the bridge, through the restricted actions API and the explorer UI action, into the event queue and out onto every event the request produces, on the close as well as the open. A request that carries no id arrives as 0, the value the protocol reserves for an uncorrelated one. The tick is captured where the event happens instead of where the queue is drained, which is a tick or more later and is not what the field is documented to mean. With the tick riding on the event the writer no longer reads the scene state at all. EU_ITEM_PURCHASE stays unimplemented and keeps answering REJECTED_FEATURE_DISABLED; the default branch of the ui mapping now says that this is deliberate.
The JS module handed the bridge call's promise straight back as `success`, so every scene read a truthy object no matter what happened. Awaiting it alone would have resolved to undefined, since nothing along the chain produced a value: TryStopEmote returned void and quietly did nothing when the scene was not the current one. That refusal is the one outcome a scene can act on, so the bool is carried from the API through the wrapper to the module, and `success` is now the boolean the protocol's SuccessResponse promises.
Contributor
🚦 CI StatusWindows and Mac built successfully in Unity Cloud.
Warnings count reduced: 11875 => 11874 Lint run · full InspectCode report · took 27m 49s 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 #35742327217. Full reportPR #10096, run #35742327217 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 |
A comment-only pass over the openExplorerUi fixes. Every surviving fact now lives in exactly one place: the meaning of requestId 0 and of the event tick stay on ExplorerUiEvent, the showPending window stays at the check a cleanup would "simplify" away, and the reason no SafeRestart token is taken stays next to the call that breaks without it. The rest was narration, the same fact echoed across layers, or tests restating their own names. No logic, naming or formatting changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
popuz
marked this pull request as ready for review
September 15, 2026 09:03
decentraland-bot
self-requested a review
September 15, 2026 09:03
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
PR Review — fix: openExplorerUi verdict, request_id correlation and stopEmote's success value
STEP 1 — Context & Scope
Inspected at base 06de13e1d490 / head 971a07d94f1f. Changed files span: ExplorerUiActions.cs, IExplorerUiActions.cs, RestrictedActionsAPIImplementation.cs, RestrictedActionsAPIWrapper.cs, IRestrictedActionsAPI.cs, RestrictedActions.js, ExplorerUiEvent.cs, WriteExplorerUiEventsSystem.cs, ECSWorldFactory.cs, SceneInstanceDeps.cs, three test files, five protocol .gen.cs files plus one .meta, and scripts/package.json + lockfile. Protocol gen files are auto-generated protobuf output.
Read surrounding files at the PR head: ExplorerUiActions.cs, WriteExplorerUiEventsSystem.cs, ExplorerUiEvent.cs, IExplorerUiActions.cs, IRestrictedActionsAPI.cs, RestrictedActionsAPIWrapper.cs, RestrictedActions.js. Verified assembly, lifecycle, and threading context.
STEP 2 — Root-cause check: PASS
Three defects, each addressed at its root:
-
Race condition (verdict decided before state existed):
OpenSectiondecided synchronously on the JS thread, before the main-thread hop, so two rapid calls both gotOPENED. The fix moves the decision to afterSwitchToMainThreadand adds ashowPendingflag that covers the window between accepting the request and MVC reporting the panel as showing. This fixes the cause — not a workaround. -
request_iddropped: The JS bridge (RestrictedActions.js) forwarded onlymessage.ui. The id now crosses the bridge as auint, echoed on bothUiOpenedandUiClosedevents. Root cause addressed. -
stopEmotereturning promise as boolean: The JS shim returned the bridge call's promise object withoutawait, sosuccesswas always a truthy object. Addingawaitand returningboolfromTryStopEmotefixes the cause.
STEP 3 — Design & integration: PASS
MANDATORY OWNER SEARCH: No new long-lived systems, managers, or controllers are introduced. The showPending flag is a new field on the existing ExplorerUiActions class (created per scene in SceneInstanceDeps.cs). The scene owns its lifecycle; teardown cancels via disposeCts.Token. No new lifecycle owners needed.
TEARDOWN / CONSUMPTION TRACE:
showPending— set totrueinOpenSectionAsync, cleared inShowUntilClosedAsync'sfinallyblock (guaranteed even on cancellation/exception). No leak path.ShowUntilClosedAsync.Forget()— intentionally detached becauseShowAsyncresolves when the panel closes, which is seconds/minutes later. The caller must returnOpenedimmediately. Exceptions are caught:OperationCanceledExceptionis swallowed, others logged viaReportHub.Openedis enqueued before the detached call;Closedis in thefinally, so everyOpenedis paired with aClosedregardless of outcome. This matches CLAUDE.md §9 — detached async for work that outlives the caller's scope, with proper exception handling.ISceneStateProviderremoved fromWriteExplorerUiEventsSystem— the tick now comes from the event itself, eliminating the drain-tick inaccuracy. Both the CRDTAPPENDtimestamp and the payloadTimestampuse the event's tick, consistent withTriggerAreaHandlerSystem.
Threading: OpenSectionAsync hops to the main thread via UniTask.SwitchToMainThread(ct) before accessing showPending or mvcManager. ShowUntilClosedAsync runs entirely on the main thread. Queue.Enqueue is called only from the main thread. No thread-safety concern.
Deliberate absence of per-call CancellationTokenSource: Correct — SafeRestart would cancel the first of two overlapping calls, which is exactly the scenario being fixed. Scene teardown remains the only cancellation.
STEP 4 — Member audit: PASS
| Member | Consumers | Notes |
|---|---|---|
IExplorerUiActions.OpenSectionAsync |
RestrictedActionsAPIImplementation.TryOpenExplorerUiAsync (1) |
Replaces OpenSection; interface exists for test substitution — legitimate |
IRestrictedActionsAPI.TryOpenExplorerUiAsync |
RestrictedActionsAPIWrapper.OpenExplorerUi (1) |
Same pattern as other async methods on the interface |
IRestrictedActionsAPI.TryStopEmote (now bool) |
RestrictedActionsAPIWrapper.StopEmote (1) |
Return type change only; interface exists for test seam |
ExplorerUiEvent.RequestId, .Tick |
ExplorerUiActions.Enqueue, WriteExplorerUiEventsSystem.Update, tests |
Consumed correctly |
No single-use extraction or derived predicates that re-validate guaranteed state.
STEP 5 — Line-level review
No P0 or P1 issues found. Two P2 observations:
[P2] ExplorerUiEvent struct does not override Equals/GetHashCode — relies on ValueType.Equals (reflection-based). Works correctly for all-value-type fields but a manual override would be faster and more explicit. See inline suggestion.
[P2] EU_ITEM_PURCHASE not in JS enum — intentional (disabled feature); TryMapExplorerUi's default branch returns RejectedFeatureDisabled with a clarifying comment. No action needed.
STEP 6 — Complexity
COMPLEX — touches the scene runtime CRDT bridge, JS module system, shared interfaces, async patterns (new UniTask flow with showPending flag), and protocol bindings across 4+ files.
STEP 7 — QA assessment
YES — runtime code changes affecting scene-to-explorer panel interactions, emote stop behavior, and the JS/C# bridge.
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
Security review
Credentials: No secrets committed.
Input validation: requestId is a uint from the scene, guarded with != undefined defaulting to 0. ui validated by TryMapExplorerUi. No injection surface.
Authorization: All existing gates preserved (IsCurrent, gesture check, FeaturesRegistry, Communities allowlist).
Supply chain:
| Dependency | Version | Publisher | Scope | Risk |
|---|---|---|---|---|
@dcl/protocol |
1.0.0-34848312390.commit-ca008c2 |
@dcl |
build (codegen) | Low — exact commit pin, first-party |
ExplorerItemPurchaseResult.gen.cs |
new | protobuf codegen | runtime (unused) | Low — auto-generated, RejectedFeatureDisabled for its enum |
Pin changed from caret range to exact version — improvement. No new native plugins, assemblies, or execution hooks.
DEPENDENCY_REVIEW: PASS
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the scene runtime CRDT bridge, JS module interop, async patterns with new showPending flag, and protocol bindings across multiple assemblies.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
Contributor
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
PR Review — fix: openExplorerUi verdict, request_id correlation and stopEmote's success value
Inspected at head 3dcbac41 (base dev). The diff since the prior review (971a07d) is merge-only — no substantive code changes.
STEP 2 — Root-cause check: PASS
Three defects, each fixed at the root:
- Verdict race —
OpenSectiondecided synchronously on the JS thread before the main-thread hop, so two rapid calls both sawIsShowing == falseand both gotOPENED. The fix moves the decision afterSwitchToMainThreadand adds ashowPendingflag that covers the window betweenShowAsyncbeing called and MVC reporting the view as showing. Root cause addressed. request_iddropped — the JS bridge forwarded onlymessage.ui; the id now travels as auintthrough the bridge, restricted-actions API, event queue, and onto both the open and close events. Root cause addressed.stopEmotepromise-as-boolean — the JS shim returned the bridge promise object (always truthy) withoutawait.TryStopEmotenow returnsbool, the shim awaits it, andsuccessis the actual boolean the protocol promises. Root cause addressed.
STEP 3 — Design & integration: PASS
Owner search: No new long-lived units introduced. showPending lives on the existing ExplorerUiActions (created per scene in SceneInstanceDeps), which is the natural owner of the panel-show lifecycle for that scene.
Teardown trace:
showPending→ set inOpenSectionAsync, cleared inShowUntilClosedAsync'sfinally(guaranteed on success, cancellation, and exception). No leak path.ShowUntilClosedAsync.Forget()— intentionally detached becauseShowAsyncresolves only when the panel closes. Exceptions caught:OperationCanceledExceptionswallowed (sanctioned pattern), others logged viaReportHub.Openedenqueued before the detached call;Closedinfinally— every open is paired with a close.ISceneStateProviderremoved fromWriteExplorerUiEventsSystem— tick now travels on the event itself, eliminating drain-tick inaccuracy. Design improvement.
Threading: All reads/writes of showPending and all Queue.Enqueue calls occur after SwitchToMainThread. ShowUntilClosedAsync also runs on the main thread. No concurrent access.
No per-call CTS, intentionally: SafeRestart would cancel the first of two overlapping calls — the exact scenario being fixed. Scene teardown via disposeCts.Token remains the only cancellation vector. Correct.
STEP 4 — Member audit: PASS
No single-use accessors or derived predicates that re-validate guaranteed state. Enqueue helper has two call sites (open + close) and centralizes all four event fields — earns its keep.
STEP 5 — Findings
See inline comment below. One P2 carried forward from the prior review.
Checked and clean
| Category | Status |
|---|---|
| R1 hot-path alloc | ✅ Static lambda in WriteExplorerUiEventsSystem; ExplorerUiEvent is a stack-allocated struct |
| R2 LINQ | ✅ None |
| R4 ECS discipline | ✅ WriteExplorerUiEventsSystem lost a dependency it didn't need; no new per-frame work |
| R5 structural changes | ✅ No entity structural changes |
| R6 acquire/release | ✅ Teardown traced above |
| R7 nullability | ✅ No new nullable lies or forgiving operators |
| R8 root cause | ✅ All three fixes address the actual defect |
| R9 logging | ✅ ReportHub.LogException with ReportCategory.RESTRICTED_ACTIONS |
| R10 catch scope | ✅ Inner try/finally + outer specific catches |
| R11 cancellation | ✅ Caller's CT threaded through SwitchToMainThread; no per-call CTS by design |
| R12 abstraction | ✅ No new single-impl interfaces |
| R13 reuse | ✅ Enqueue centralizes; TryMapExplorerUi reused |
| R14 contract types | ✅ ExplorerUiEvent is a readonly struct with value-type fields |
| R15 dead weight | ✅ Removed stale ISceneStateProvider dep from writer |
| R16–R17 naming | ✅ OpenSectionAsync, ShowUntilClosedAsync, Enqueue — clear responsibility |
| R18 magic numbers | ✅ 0 for absent request-id is the protocol's documented default |
| R19 file hygiene | ✅ |
| R20 idioms | ✅ readonly on all immutable fields |
| R21 MVC | ✅ Panel lifecycle unchanged |
| R22 tests | ✅ 5 new regression tests covering each bug; test scene linked |
| R23 comments | ✅ Comments explain the code's own guarantees, no narration |
| R24 scope | ✅ All files relate to the three stated fixes + protocol repin |
Merge gates
- R25 — Outstanding comments: 1 prior inline comment on
ExplorerUiEvent.cs(P2: missingEquals/GetHashCodeoverride) is still unresolved. Refreshed below with an updated suggestion. - R26 — QA: Awaiting QA. PR includes detailed test instructions and links sdk7-test-scenes #98.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies async/UniTask patterns in the restricted-actions pipeline, the JS–C# bridge, ECS event writing, and cross-assembly interfaces.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Vitaly Popuzin (<@U03V3D7E0NL>) via Slack
DafGreco
approved these changes
Sep 16, 2026
DafGreco
left a comment
DafGreco
left a comment
There was a problem hiding this comment.
✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
- [ ✔️] Backpack and wearables in world
- [ ✔️] Emotes in world and in backpack
- [ ✔️] Teleport with map/coordinates/Jump In
- [ ✔️] Chat and multiplayer
- [ ✔️] Profile card
- [ ✔️] Camera
- [ ✔️] Skybox
- [ ✔️] Settings
Evidence:
20260916-1251-47.3336689.mp4
pravusjif
approved these changes
Sep 22, 2026
pravusjif
left a comment
pravusjif
left a comment
Member
There was a problem hiding this comment.
LGTM! the stopEmote change is unrelated but I guess it helps with lowering warnings.
Discard the UniTask returned by the NSubstitute Received/DidNotReceive assertions inside async tests (CS4014), drop the redundant DCL qualifier in RestrictedActionsAPIWrapper, and give ExplorerUiEvent an explicit IEquatable implementation instead of the reflection-based ValueType one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
popuz
added a commit
to decentraland/sdk7-test-scenes
that referenced
this pull request
Sep 22, 2026
Adds `scenes/80,-6-explorer-ui-events`, a 2×2 scene covering the `openExplorerUi` restricted action and the `ExplorerUiEventsResult` lifecycle stream the explorer writes back to the scene. It exists to make four things checkable that were previously either broken or unobservable: 1. **The open verdict.** Two calls back to back both used to be granted `OPENED`, and the explore panel was pushed onto the windows stack twice. 2. **`request_id` correlation.** The id was dropped at the JS module boundary and never written to the result, so a scene with two sessions on the same panel could not tell which one a close belonged to. 3. **The per-event tick.** Events were stamped with the tick the queue happened to be *drained* on, which flattened distinct events onto one value. 4. **`stopEmote`'s result.** The `success` field held an unresolved promise — always truthy — instead of a boolean. The explorer-side fixes are in decentraland/unity-explorer#10096. Against a build without them, scenarios 4, 7 and 8 in the scene README fail and every request id in the HUD reads `0`, which is exactly what makes this scene useful as a regression check. ## What the scene shows Two HUD feeds report side by side, so nothing needs a log tail (everything is logged too, under the `[openExplorerUi]`, `[explorerUiWait]`, `[explorerUiEvents]` and `[stopEmote]` prefixes): - **Right — "Call results":** what each call was *told*. Green `OPENED`, yellow `WAS_ALREADY_OPEN`, red rejections. - **Left — "ExplorerUiEventsResult":** what actually *happened* to the panels — one line per lifecycle event with the panel, the kind, the tick and the request id. The two are deliberately not merged: a verdict says a request was accepted, an event says a panel really appeared, and a `CLOSED` has no verdict counterpart at all — it arrives unprompted, including when the player presses Esc. Four rows of cubes running north from the spawn: | Row | What it covers | | --- | --- | | Panels | One cube per `ExplorerUi` value — the happy path, no `requestId`, so both events report `req 0` | | Gates | Calls the explorer must refuse: expired gesture window, a second call on a busy panel (ids 101/102), an unknown enum value | | Wait helper | `openExplorerUiAndWait` across its whole outcome surface: `closed`, `matched`, `timedOut`, `notOpened` | | stopEmote | Play an emote, stop it, print `typeof success` | Both consumption styles run side by side on purpose: the raw drain over the grow-only value set and the SDK helper. Every event the helper consumes must still appear in the left-hand HUD. The full scenario list with expected results is in the scene's [README](scenes/80,-6-explorer-ui-events/README.md). ## Testing The scene deploys to `sdk7testscenes.dcl.eth` on SEPOLIA automatically from this PR — see the bot comment below for the deep link. **This needs a custom explorer build.** The fixes are not in any released client, so on a stock build the DOUBLE CALL cube reports two `OPENED`s, the ids all read `0`, and STOP EMOTE prints `typeof=object`. Run decentraland/unity-explorer#10096 to see the expected behaviour: ```bash metaforge explorer run 10096 ``` then connect with `--dclenv zone --realm sdk7testscenes.dcl.eth --position 80,-6`. **Closing a panel is done with Esc** — there is no in-world button for it, and the scene never closes a panel itself. That is what makes the `CLOSED` events worth watching. ## Before merging The scene is pinned to a **branch build** of `@dcl/sdk`, `@dcl/js-runtime` and `@dcl/sdk-commands` (`7.28.1-34832208486.commit-2bfb1a8`), because `openExplorerUiAndWait`, the `requestId` field and the `ExplorerUiEventsResult` component are not in a published release yet. The pin must move to a published version once decentraland/js-sdk-toolchain#1543 lands. Merge order: js-sdk-toolchain#1543 → unity-explorer#10096 → repin here → merge this. Parcels `80,-6`, `81,-6`, `80,-5`, `81,-5` are free; `npm run check-parcels` reports no collisions. The scene sits directly south of `80,-4-restricted-actions`, which covers the rest of that API.
popuz
added a commit
to decentraland/js-sdk-toolchain
that referenced
this pull request
Sep 23, 2026
Follow-up to #1511, as announced there. `openExplorerUiAndWait` opens an explorer panel and resolves once the session that call started ends. This is a rewrite of what this PR originally proposed (`openExplorerUiAndWaitClose`). decentraland/protocol#464 added a second result component rather than new fields, which showed that this surface grows by **adding whole components** — the first version could not absorb that, so the helper was redesigned around correlation instead of guessing. The design notes live in the commit messages of `ae7413b` and `6c60912`. ## What changes - New `@dcl/sdk/explorer-ui` subpath module (file-layout entry, same mechanism as `platform/`), exporting `openExplorerUiAndWait` plus re-exports of `ExplorerUi` / `OpenExplorerUiResult`, so one import covers the whole flow. Like `platform/`, it is not part of the playground-assets rollup, so none of the helper's own symbols reach `api.md`. - **Sessions are correlated, not guessed.** Every call mints a `request_id` (a counter starting at 1; the wire's `0` means uncorrelated) and the explorer echoes it on the events that call produced. Listeners are armed *before* the RPC, so an event that lands while it is in flight is already observed. That deletes the anchor, the per-`ui` FIFO, the consumption high-water mark, the pre-RPC timestamp snapshot and the replay loop of the first version. - **Graceful on an explorer that does not echo yet.** An uncorrelated lifecycle event falls back to matching on `ui`, which is unambiguous while panels cannot coexist (a second call for the same panel is answered `WAS_ALREADY_OPEN`). Where that fallback cannot be trusted — a panel that can coexist with another, or a channel with no `ui` field — the call rejects with an explicit message instead of attributing the event to the wrong session. - **Channels are the extension point.** `channel(name, component)` wraps any grow-only result component, so a component added later needs no SDK release. `ExplorerUiEvents` and `ItemPurchase` ship ready-made. `collect` names the channels whose events are gathered, each tagged with its channel name; `until` (a channel, or `variant(channel, 'case')`) stops the wait early. - **The panel closing is the terminal**, and it wins over `until`. `WaitOutcome` is `closed` | `matched` | `notOpened` | `timedOut` — the outcome is the outer axis, so `notOpened`/`timedOut` are written once and never change as channels are added. `$case` says *why the wait stopped*; `events` always holds everything collected, the stopping event included. - **Non-`OPENED` verdicts resolve immediately** as `notOpened`: event delivery is owner-only, so a scene that did not open the panel never receives its `closed` — waiting would hang deterministically. - **`timeoutMs` (no default) resolves with `$case: 'timedOut'`.** Implemented as a lazily added engine system (the scene runtime has no timers) that removes itself once no timed session remains. It stays optional because panels legitimately stay open for minutes; what omitting it costs is documented on the option. - The promise never rejects on lifecycle grounds — only when an event cannot be attributed to any call, or when the RPC itself fails. - **`ExplorerItemPurchaseResult` (1221) is declared grow-only** in `generateIndex.ts`. Without that allowlist entry it silently generates as LWW and same-tick events overwrite each other. - **`code-to-composite` mock completed.** Its auto-mock returned an async function for any unknown property, which turned `OpenExplorerUiResult` into a function whose members are `undefined` — a scene using the helper hung forever while the command reported success. The mock now carries the real enum and answers `REJECTED_FEATURE_DISABLED`, matching the file's refuse-gracefully convention. Also fills in `copyToClipboard` and `stopEmote`. ## Protocol bump `@dcl/protocol` moves to `1.0.0-34643020503.commit-6402953`, which carries decentraland/protocol#464. That is what adds the 69 lines to `api.md` — the new `PBExplorerItemPurchaseResult` message, the `EU_ITEM_PURCHASE` enum member and the `core::ExplorerItemPurchaseResult` entry in `componentDefinitionByName`. The regenerated `.crdt` snapshots in the diff come from the same bump. ## Verification - `test/sdk/explorer-ui/openExplorerUiAndWait.spec.ts` — 24 tests, all green. Events are driven through the real incoming CRDT path (a transport feeding `APPEND_VALUE`), never `addValue`: the outgoing path hands `onChange` the whole set, which a scene never sees for a renderer-owned component. Tests read the minted id back from the mocked RPC rather than guessing it, and use a synthetic grow-only component to cover channel behaviour independently of 1221. - Covered: id minting and RPC forwarding; foreign-id events ignored; two concurrent sessions with interleaved events; the uncorrelated `ui` fallback and both rejection paths; every non-`OPENED` verdict; the collected chain including its terminator; `until` by channel and by variant, including on a channel that is not collected; close beating `until`; `timedOut` with partial events and system self-removal; close beating a timeout that expires in the same tick; `timeoutMs: 0`; RPC failure; a re-wrapped channel not double-collecting; the name-conflict throw; events arriving while the RPC is in flight; the `undefined` delivered on `DELETE_ENTITY`. ## Trying it end to end The helper needs an explorer that echoes `request_id`, which is decentraland/unity-explorer#10096. Against a client without it, the helper still works through the uncorrelated `ui` fallback, and only `EU_ITEM_PURCHASE` rejects. decentraland/sdk7-test-scenes#98 exercises it live, next to a hand-rolled drain of the same component so both consumption styles stay covered. It is deployed, so no local build is needed: [jump in](http://decentraland.zone/jump/?dclenv=zone&realm=sdk7testscenes.dcl.eth&position=80,-6) and use the **Wait helper** row — the five cubes cover close, a collected chain, an early `until` match, a timeout, and a second call answered `WAS_ALREADY_OPEN`. Each outcome is printed to the scene's own HUD on the **right**; the **left** HUD shows the raw events underneath, and every event the helper consumes must still appear there.
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
4 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?
Three defects in
openExplorerUiand the event stream it writes back to scenes, found in a manual pass. One commit each, so any can be dropped.1. Two calls back to back both got
OPENEDand the panel was pushed onto the windows stacktwice. The verdict is now decided after the thread hop, behind a private
showPendingflag —MVCManager.IsShowingreadscontroller.State, which flips only afterpopupCloser.HideAsync, soMVC cannot answer for a panel that is still landing. The same commit fixes the case where the user
had opened the panel first: the continuation bailed before enqueuing, so the scene was told
OPENEDand then received no events at all. No per-call CTS on purpose —SafeRestartwould cancelthe first of two overlapping calls, which is the scenario being fixed.
2.
request_idwas dropped at the JS boundary and never written to the result, so a scene withtwo sessions on one panel could not tell which close was whose. It now crosses as a
uintand isechoed on both events of the call that actually got the panel; an absent id collapses onto
0atthe shim. Separately,
timestampwas the tick the queue was drained on — it is now read atenqueue, and
WriteExplorerUiEventsSystemno longer takesISceneStateProviderat all.3.⚠️ Behaviour
stopEmotereturned a promise in the booleansuccessfield. Anawaitalone would resolveto
undefined, soboolis threaded throughTryStopEmote→ wrapper → shim.change:
successwas an unresolved promise, i.e. always truthy; it is nowfalsewhen the sceneis not the current one and the stop was refused.
ExplorerItemPurchaseResult/ component 1222 arrive unused on purpose. The protocol repin pullsthem in, IAP is a separate effort, and
EU_ITEM_PURCHASEanswersREJECTED_FEATURE_DISABLED.Repinning to
@dcl/protocol@nextis not an option —experimentalcarriesAvatarEmoteMaskand thecomms social-emote fields that
mainlacks, and dropping them breaks the emote system.Related: decentraland/protocol#464 · decentraland/protocol#486 · decentraland/js-sdk-toolchain#1543
Test Instructions
Steps (standard run):
Point the build at the test scene — it is already deployed, nothing to build or install. Either
launch with these app params
(how):
…or, if the build already runs on zone, type
/goto sdk7testscenes.dcl.eth/80,-6in chat.Expected result: you land in front of four rows of labelled cubes, with two report panels drawn
on screen by the scene. Hovering a cube says what it does.
Where the results appear
The scene draws two separate report panels, and every step below says which one to read.
🟩
OPENEDgranted · 🟨WAS_ALREADY_OPENrefused because a panel is already up · 🟥 rejected.MAP OPENED (tick 1953, req 101). The open and close of one panel share the samereq, and theclose shows a bigger
tick.Both are in-scene UI, not Explorer UI — they sit on top of whatever panel you open, so you can read
them without closing it.
Panels are closed with Esc — there is no button for it anywhere.
Test Steps
ON-START(MAP) REJECTED_NO_USER_GESTURE (5)OPENED· LEFT:OPENED, thenCLOSEDafter Esc, samereq, biggertickOPENEDand 🟨WAS_ALREADY_OPEN· LEFT:req 101on both linesreqnumbers than the firstREJECTED_NO_USER_GESTURE (5)REJECTED_FEATURE_DISABLED (4), instantlyCLOSED (0 collected)CLOSED [opened+closed]MATCHEDwhile the panel is still openTIMEDOUTafter ~3 s, panel stays upNOT_OPENED WAS_ALREADY_OPEN· one panel onlysuccess=true typeof=boolean/goto sdk7testscenes.dcl.eth/80,-4and use that sceneAdditional Testing Notes
each other by design.
appear there.
slowness.
Quality Checklist
CrdtEcsBridge.RestrictedActions.Tests(from 57); each fix has a regression that fails without it
sdk7testscenes.dcl.eth80,-6