fix: Analytics list silently dropped deployed scenes - #1492
Conversation
The list was assembled from Redux state the page did not own, through a discovery path where every failure degraded into a missing row rather than an error. Which scenes disappeared varied per load. - The list no longer reads `management.projects`. That slice holds the Manage page's own result set, narrowed by its search box, `publishFilter` and page, so opening Analytics after leaving Manage on `UNPUBLISHED` rendered no world rows at all. `fetchAllDeployedWorlds` fetches the wallet's worlds unfiltered and pages through `total`, replacing the 50-world ceiling. - The thunk fetches its own LAND instead of reading `land.data`, and no longer consults `management.status`. The old `idle`-only guard fell through while AuthProvider's own fetch was still in flight and cached an empty snapshot as `succeeded`, indistinguishable from "you have no Places yet". - A failed world scene lookup returned `[]`, which read as "no scenes" and made that world vanish. It now answers `null`, and the analytics path throws naming the world it could not list. - `fetchLandPublishedScenes` skipped a failed pointer chunk in a bare `catch`, losing up to 100 parcels' scenes with no signal. Chunks now run concurrently, retry once and throw; it also sends `Content-Type: application/json` and allows 30s, since a cold 100-pointer batch outlasts the 5s default. - `fetchWorldScenes` was never paginated, so a world above its undeclared 100-scene page lost the rest. Both endpoints now share one `readAllPages`. - Two scenes could resolve to the same `placeId` and collapse in `metricsByPlaceId`, dropping a row from the render. Places are deduped by location, and the `sceneBase` fallback sorts coordinates numerically rather than as strings, where "10,0" preceded "9,0". - The thunk takes a `condition`, so a remount cannot start a second run that rebuilds the list from staler state and wins the race. The metrics transport is unchanged: it was already fail-loud, and is not a source of the shrink. Also documents: - CLAUDE.md: vitest fake timers leak across describe blocks
CLAUDE.md's "Testing Conventions" section had grown into a grab-bag: six
conventions plus six subsections, four of which were runtime traps (Redux
freeze, composite placeholders, CommsApi, Electron headers) filed under a
testing heading. Anyone reading it for how to write a test read four things
that were not about tests, and `docs/testing-standards.md` covered only E2E.
- `docs/testing-standards.md` now holds how to write a test here and every
testing gotcha. CLAUDE.md keeps the pointer under `## Standards`.
- The leftover subsections move under `## Gotchas`, which is what they are.
- Adds three traps this session hit: a `vi.mock` factory silently replaces the
whole module, so adding an export to a source file breaks a spec that never
named it — and when the missing export is called rather than read at import
time it surfaces as a bare TypeError with nothing pointing at the mock;
`importOriginal<typeof import('./x')>()` fails
`@typescript-eslint/consistent-type-imports`, so the generic needs a
namespace type import; and how to prove an after-the-fact test can fail.
House style allows only JSDoc on public functions and modules; the reasoning that was sitting in `//` comments belongs there or in a name. - `hasEverBeenDeployed` replaces the comment explaining why a null `owner` means the world was never deployed. - `asDispatchedThunk` in the placeAnalytics spec names the promise-plus-unwrap shape that a comment was describing. - The dedupe rationale, the null-vs-empty distinction for `scenes`, the reason `fetchAllDeployedWorlds` throws, and the reason the thunk skips while one is in flight all move into the JSDoc of the function they describe.
Test this pull request on windows-latestDownload the correct version for your architecture: |
Test this pull request on macos-latestDownload the correct version for your architecture:Click here if you don't know which version to downloadFor running this unsigned version of the app, you will need to run the xattr command on it:
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review: fix: Analytics list silently dropped deployed scenes
Overview
Thorough, well-documented fix for a systemic bug where the Analytics page silently dropped deployed scenes by borrowing the Manage page's filtered/paged Redux state. The PR correctly identifies and addresses seven distinct root causes, decouples the analytics data pipeline from the management slice, and adds pagination, retry logic, deduplication, and comprehensive tests.
Git conventions (ADR-6): ✅ Title fix: Analytics list silently dropped deployed scenes and branch fix/analytics-scenes-list both follow the semantic commit format.
CI: ✅ All checks passing — unit tests, E2E, lint, typecheck, macOS/Windows builds.
Security Audit
All six areas the requester asked to verify were reviewed:
| Check | Result |
|---|---|
| Requests injectable? | ✅ No — URLs from app config + hardcoded paths; POST bodies via JSON.stringify; coordinates are typed numbers; wallet address from AuthServerProvider |
| Security flows? | ✅ Auth gated via AuthServerProvider.getAccount() before any network call |
| Security headers spoofable? | ✅ No — only Content-Type: application/json is set |
| useCallbacks/component logic? | ✅ Callbacks have stable deps; dispatch is stable in Redux |
| useEffect loops? | ✅ No — AnalyticsPage depends on [isSignedIn]; AnalyticsDetailPage guards with status === 'idle'; the thunk's condition guard prevents re-entry |
Infinite loop in readAllPages? |
✅ No — dual termination guards: !result returns null, items.length === 0 breaks; while (items.length < total) converges even if total fluctuates |
No security vulnerabilities found.
Findings
[P2] !project.scenes relies on truthiness rather than explicit undefined check
packages/creator-hub/renderer/src/modules/store/management/utils.ts:155
const unreadable = projects.filter(project => !project.scenes).map(project => project.id);The intent is to detect worlds whose scene lookup failed (undefined), not worlds with no scenes ([]). Since ![] is false in JavaScript, this works correctly today. However, using project.scenes === undefined would make the intent explicit and resist future regressions if the type ever gains a null state.
[P2] collectAnalyticsPlaces dedup has last-wins semantics
packages/creator-hub/renderer/src/lib/analyticsLocations.ts:146
return [...new Map(places.map(place => [place.placeId, place])).values()];The Map constructor keeps the last entry for duplicate keys. Since worldPlaces is spread before genesisCity, a land place would overwrite a world place with the same placeId. In practice this can't happen (worlds and Genesis City don't share coordinates), but a brief comment documenting the precedence would clarify intent.
[P2] readAllPages swallows all error types into null
packages/creator-hub/renderer/src/modules/store/management/utils.ts:50
}).catch(() => null);The .catch(() => null) collapses all errors (network, auth, programming bugs in the callback) into null. The callers handle null correctly (throwing in fetchAllDeployedWorlds, returning null in fetchWorldSceneCoords), so this doesn't cause silent data loss. Adding a console.warn before returning null would help debugging transient failures in production.
[P2] createAsyncThunk could use generic type parameters
packages/creator-hub/renderer/src/modules/store/placeAnalytics/slice.ts:34
The _: void annotation and getState() as AppState cast could be replaced with createAsyncThunk<ReturnType, void, { state: AppState }> for idiomatic RTK typing.
Architecture Assessment
- ✅ Decoupling from management slice — correct and necessary. The management slice holds UI-filtered state that analytics must not inherit.
- ✅ Own data fetching — fetching worlds/LAND per visit trades a few extra requests for correctness. The
conditionguard prevents duplicate in-flight fetches. - ✅
readAllPagesabstraction — clean generic with good termination guarantees, used by two call sites. - ✅ Error propagation — fail-closed (throw when any world's scenes are unreadable) is a defensible product decision: a silently short list is worse than an error. The
retry()insidereadAllPagesmitigates most transient failures. - ✅ Test coverage — 305 new lines of tests with explicit failure-mode coverage (timeout, retry, race, dedup, numeric sort). Tests proven to fail against the original code.
Verdict: ✅ APPROVE
No P0 or P1 issues. The four P2 findings are minor clarity improvements, none blocking. The fix is well-reasoned, well-tested, and architecturally sound.
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack
| worlds.map(world => toManagedProject(worldsApi, world, address)), | ||
| ); | ||
|
|
||
| const unreadable = projects.filter(project => !project.scenes).map(project => project.id); |
There was a problem hiding this comment.
[P2] Consider project.scenes === undefined instead of !project.scenes — it makes the intent ("fetch failed" vs "no scenes") explicit and resists future regressions if the type ever gains a null state. Both work today since ![] is false, but the explicit check reads more clearly.
| const answer = await page(items.length); | ||
| if (!answer) throw new Error('Page not available'); | ||
| return answer; | ||
| }).catch(() => null); |
There was a problem hiding this comment.
[P2] This .catch(() => null) collapses all error types into null. Since callers handle null correctly, it works — but adding console.warn('readAllPages: page fetch failed after retries', err) before returning null would help trace transient failures in production.
| return [...worldPlaces(projects, fallbackThumbnail), ...genesisCity]; | ||
| const places = [...worldPlaces(projects, fallbackThumbnail), ...genesisCity]; | ||
|
|
||
| return [...new Map(places.map(place => [place.placeId, place])).values()]; |
There was a problem hiding this comment.
[P2] The Map constructor keeps the last entry for duplicate keys. Since worldPlaces is spread before genesisCity, land entries win on collision. In practice worlds and Genesis City can't share coordinates, but a brief comment documenting the precedence (e.g. // Land entries overwrite world entries on duplicate placeId) would clarify intent.
There was a problem hiding this comment.
Thanks — the mechanics here are right: new Map(...) does keep the last entry, and genesisCity is spread second, so a land entry would win. But the precedence that would document can't be reached.
toLocalId namespaces the two sources:
return world ? `world:${world}${ID_SEPARATOR}${coords}` : `land:${coords}`;A world place is always world:<name>@<x,y> and a Genesis City place is always land:<x,y>, so the two can never produce the same placeId — not merely "in practice", but by construction. A comment naming the winner would describe a branch with no reachable inputs, and imply a collision is possible where the id scheme already rules it out.
The dedupe that can fire is within a single source: two scenes in the same world resolving to the same base coordinate (reachable via the parcels fallback in sceneBase when a deployment's metadata has no base). That case is already stated on the function itself:
Deduplicated by location, because that is what analytics answers by: two scenes sharing a coordinate are one row, and keeping both would key them to the same metrics and lose one anyway.
Worth noting too that this repo's style allows only JSDoc on functions and modules, not inline comments — so reasoning like this belongs on the function, which is where the above already lives.
Leaving as-is. The other two suggestions were both good and are applied in 1617ddb.
|
PR reviewed and approved by QA on both platforms following the PR test instructions. ✅ Build: Test results:
Unrelated errors noted (do not affect verdict): None. Verdict: PASS ✅ Confirms the original non-deterministic places-list bug (same wallet, different scene counts across sessions/platforms) is resolved. Tested both the general cross-platform consistency and the specific Manage-filter-bleed scenario called out in the PR description. 
|
Review feedback on #1492. - `project.scenes === undefined` replaces `!project.scenes`. The two agree today, since `![]` is false, but `undefined` is the sentinel for "we could not ask" while `[]` means "there are none" — saying which one is meant beats relying on a coincidence of falsiness. - `readAllPages` warns with the underlying error before giving up. The catch collapsed every failure into `null`, so the thrown `Failed to fetch worlds` carried no cause; the warning keeps the timeout or status that actually ended it. A third comment asked for a comment documenting which entry wins when `collectAnalyticsPlaces` dedupes. Declined: `toLocalId` namespaces world ids as `world:<name>@<coords>` and Genesis City as `land:<coords>`, so the two sources cannot collide at all, and the dedupe that can happen — two scenes in one world sharing a base — is already in that function's docstring.
The unit tests each mock the boundary the shrink actually crossed, so nothing covered the wiring end to end. This drives the real thunk through the real store with 120 worlds and 80 Genesis City scenes, stubbing only the network and the metrics preload. 200 is the number that matters: `MAX_LOCATIONS_PER_REQUEST` is 100, so the list spans two metrics batches and the positional read has to hold across them. The worlds list also spans more than one page at that size. Asserts the whole chain — every scene present and split 120/80, the worlds list paged rather than truncated, one batch per hundred locations with each answer still matching the coordinates it was asked for, a row per scene from the selector, the first and last rendered, and a failed world lookup ending as an error naming the world instead of a shorter list. Reverting the six source files to their pre-fix state fails all six. Also documents: - docs/testing-standards.md: `vi.mock` factories drop the default export, and `importOriginal<T>()` rejects `export *` barrels; what a page component needs to render (theme, router, stubbed navbar)
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review after 1617ddb1 and a30cec3b
Two new commits since my previous review address two of the four P2 findings:
Addressed
- ✅
readAllPageserror swallowing — now logsconsole.warn('[paged-read] gave up on a page after retries', error)before returningnull, and the inner error message includes the offset (No answer for the page at offset ${items.length}). Matches my suggestion. - ✅
!project.scenestruthiness check — replaced withproject.scenes === undefined, making the intent explicit. Matches my suggestion exactly.
New: at-scale integration test (atScale.spec.tsx)
Excellent addition — 269 lines testing the full analytics pipeline at realistic scale:
- 120 worlds + 80 Genesis City parcels = 200 total scenes
- Verifies pagination fires multiple requests for worlds
- Verifies metrics batching preserves location alignment (each answer's
x,ymatches what was asked) - Verifies fail-closed: one world returning 503 → status
'failed', error names the world, zero places rendered - Verifies the actual React component renders all 200 rows
- Documents testing gotchas (default export mocking,
importOriginalwith barrels, ThemeProvider wrapping, Navbar stubbing) intesting-standards.md
The mock setup is thorough — stubs all three services (worlds list, world scenes, content entities/active) and the signed-fetch module, while keeping the real thunk flow.
Remaining P2s (non-blocking, unchanged)
collectAnalyticsPlacesdedup has last-wins semantics — harmless sinceplaceIdprefixes (world:vsland:) prevent cross-domain collisioncreateAsyncThunkcould use generic type params — style-only
CI
✅ Unit tests, lint, typecheck all passing on a30cec3b. E2E and builds still running.
Verdict: ✅ APPROVE (re-confirmed)
The two addressed findings and the new at-scale test strengthen an already solid PR.
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack
3 participants
Context and Problem Statement
Since the Analytics page shipped (#1447), the list of deployed scenes sometimes
showed everything and sometimes a subset, with no error and no way to tell which
had happened. Which scenes went missing varied between loads.
The list was assembled from Redux state the page did not own, through a
discovery path where every failure degraded into a missing row rather than an
error. Seven distinct paths could shrink it:
management.projectsis the Manage page's result set, narrowed by itssearch box,
publishFilterand page. Leaving Manage onUNPUBLISHEDandopening Analytics rendered no world rows at all; a leftover search term
narrowed it;
PROJECTS_PAGE_LIMITcapped it at 50 worlds.if (management.status === 'idle')guard did not wait for an in-flightload.
AuthProviderdispatchesfetchAllManagedProjectsDatain the samerender pass that flips
isSignedIn, so the thunk regularly fell through, read[], and cached an empty snapshot assucceeded— indistinguishable from"you have no Places yet".
land.datawas read with no readiness check, andfetchLandListisdispatched from exactly one place — inside the thunk skipped by (2). Every
Genesis City scene could vanish while world rows still rendered.
fetchWorldSceneCoordscaught everything and returned[], which reads as"no scenes", so a world whose lookup failed contributed zero rows. One
request per world at
pLimit(6), so a different subset dropped each run.fetchLandPublishedScenesskipped a failed pointer chunk in a barecatch,losing up to 100 parcels' scenes silently.
fetchWorldSceneswas never paginated past its undeclared 100-scene page.placeIds collapsed inmetricsByPlaceId, andgetVisiblePlacesdropped any place missing from it — surfacing as "no results for your search".
The metrics transport was not at fault and is unchanged: it already throws on
a request/response length mismatch and uses
Promise.all, so it fails loud.Solution
Two themes: make the page own its inputs, and make every silent drop loud.
Causes 1–3 looked like three bugs but shared one root — the thunk borrowed Redux
state it did not own. Having it fetch its own worlds and LAND removes all three
along with the status guard and the conditional
await, leaving the slicesmaller than before.
Key changes:
fetchAllDeployedWorldsfetches the wallet's worlds unfiltered (nosearch,no
sort) and pages throughtotal, replacing the borrowed 50-world window.fetchLandListitself and no longer readsmanagement.statusorland.data.fetchWorldSceneCoordsanswersnullon failure vs[]for "none"; theanalytics path throws naming the world it could not list.
fetchLandPublishedScenesruns chunks concurrently, retries once and throws.It also sends
Content-Type: application/jsonand allows 30s — measured, acold 100-pointer batch takes ~1.2s but the inherited default was 5s.
readAllPageshelper paginates both endpoints. Verified against the liveservices:
GET /world/{name}/scenesis a real cursor (stabletotal, no gaps,empty past the end), while
POST /content/entities/activeignoreslimit/offsetentirely — it is pointer resolution, not a listing, so itschunk size is a request-size guard rather than a page size.
sceneBasefallback sorts coordinatesnumerically instead of as strings, where
"10,0"preceded"9,0".condition, so a remount cannot start a second run thatrebuilds from staler state and wins the race.
retryhelper backs the two discovery paths.The second commit is docs only:
docs/testing-standards.mdbecomes the singletesting doc (how to write a test plus every testing gotcha), CLAUDE.md keeps the
pointer, and the runtime traps that were filed under a "Testing" heading move to
## Gotchas.Testing
management/utils.spec.tsfailed 7/8 andland.spec.tsfailed 5/7, includingpromise resolved "[]" instead of rejecting— the silent shrink itself.managementisidle,loading,succeededwith asearchQuery, orsucceededwithpublishFilter: UNPUBLISHED.okand timed-out pointer chunks retried then reported; two scenes sharing a coordinate;sceneBasepicking the numerically lowest parcel.Impact
Creators see every deployed scene on Analytics rather than a varying subset. A
discovery failure now surfaces as an error naming what could not be read,
instead of a short list that looks complete — the more useful answer, and
consistent with how the metrics half already behaved.
Worth watching: Analytics now fetches worlds and LAND on each visit rather than
reusing the Manage page's data. That is more requests for a wallet with many
worlds, bounded by the new in-flight
condition.Screenshots
N/A — no visual change; the same list renders, with the missing rows present.