Skip to content

fix: stop mutating the shared user agent cache and serve intel macs the universal launcher - #462

Open
braianj wants to merge 2 commits into
masterfrom
fix/download-button-os-override-without-mutation
Open

fix: stop mutating the shared user agent cache and serve intel macs the universal launcher#462
braianj wants to merge 2 commits into
masterfrom
fix/download-button-os-override-without-mutation

Conversation

@braianj

@braianj braianj commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Two independent defects in the download path, found while auditing where the download funnel loses people.

1. ?os= mutated a cache shared by the whole session

DownloadButton called setUserAgentArchitectureDefaultByOs(userAgentData, os) inside a useEffect. That function edits its argument in place, and the argument is @dcl/hooks' module-level _cachedData singleton, handed to every consumer in the session. Consequences:

  • The mutation does not change the object reference, so the useMemo keyed on it never recomputes: the rendered icon and href stayed on the old platform while handleClick, which re-derives from the mutated object, delivered the new one. The button could show the Apple icon and download the .exe.
  • The override leaked to every other component reading the hook, for the rest of the session, with no path back short of a full page load (the effect only runs when os is present).

Also, ?os=macOS never actually delivered a macOS build: the function set cpu.architecture = 'unknown', and no published artifact has an unknown key, so createDownloadOption returned null and the button silently degraded to reopening the download page.

Replaced with a pure resolveUserAgentDataForOs, which returns a new object (the same reference when there is nothing to override, so it stays memo-safe) and resolves macOS to arm64. The query param keeps overriding detection: that is the feature behind the "Also available on…" links and it is intentional. setUserAgentArchitectureDefaultByOs is kept and marked @deprecated rather than deleted, in case other consumers import it.

2. Intel Macs were served the legacy "Decentraland Outdated" build

cdnReleases.ts mapped macOS amd64 to Decentraland%20Outdated-mac-x64.dmg (the old Electron launcher) while arm64 got the current one.

The current launcher dmg is a universal binary, verified against the published artifact on 2026-08-03:

$ lipo -archs .../Decentraland.app/Contents/MacOS/dcl_launcher
x86_64 arm64

So Intel Macs run it natively and there is no reason for a separate legacy target. This matches what we measured: Intel Macs were a 100% dead end — 9 anons in July 2026, none of whom ever reached the world. Both architectures now point at one constant so they cannot drift apart again, and on the identity-bound source both use the gateway URL, so Intel downloads keep the baked attribution id instead of falling back to a plain CDN link.

LAUNCHER_LEGACY_BASE_URL is removed (no remaining references).

Incidental

cdnReleases.ts imported OperativeSystem from the DownloadButton barrel, which pulled the component and React back into this module's graph — a cycle (DownloadButton imports cdnReleases) and the reason the module could not be unit tested. Now imports from DownloadButton.types directly, matching what userAgent.ts already did.

Verification

Tests are written as pure-function specs to fit this repo's testEnvironment: 'node' jest setup (no RTL): 13 new tests across userAgent.spec.ts and cdnReleases.spec.ts, including a guard that neither macOS architecture may ever point at an "Outdated" URL again.

  • npx jest — 6 suites, 50 tests green
  • npx tsc --noEmit clean
  • npm run lint and prettier clean

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying ui2 with  Cloudflare Pages  Cloudflare Pages

Latest commit: cc30547
Status: ✅  Deploy successful!
Preview URL: https://60fa1d41.ui2-423.pages.dev
Branch Preview URL: https://fix-download-button-os-overr.ui2-423.pages.dev

View logs

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: fix: stop mutating the shared user agent cache and serve intel macs the universal launcher

Verdict: Approve ✅

Excellent fix for two real bugs with clear data-backed impact. The singleton mutation fix is correct and well-guarded, and the Intel Mac universal binary change is straightforward.


Bug 1: Singleton mutation — Fixed correctly

The old setUserAgentArchitectureDefaultByOs mutated the @dcl/hooks module-level _cachedData singleton in place via a useEffect. This leaked the ?os= override to every consumer in the session, with no way to undo it. The new resolveUserAgentDataForOs:

  • Returns a new object via shallow spread ({ ...userAgent, os: { ...userAgent.os, name: os }, cpu: { ...userAgent.cpu, architecture: ... } }) — never mutates the input
  • Returns the same reference when no override is needed (!isSupportedOperativeSystem(os) or userAgent.os.name === os) — safe as a useMemo dependency
  • Gates unsupported OS values via isSupportedOperativeSystem (strict enum match, case-sensitive) — no injection risk from arbitrary ?os= values
  • Correctly maps macOS → arm64 instead of the old unknown that matched no artifact

Bug 2: Intel Mac CDN — Fixed correctly

Both macOS architectures now point to the same LAUNCHER_MACOS_DMG / AUTO_SIGNING_MACOS_DMG constants. The published .dmg is a universal binary (lipo -archsx86_64 arm64), so Intel Macs run it natively. The LAUNCHER_LEGACY_BASE_URL is removed with no remaining references.

[P2] Minor notes (non-blocking)

  1. Deprecated function is justified: setUserAgentArchitectureDefaultByOs is re-exported via src/modules/index.ts, so external npm consumers of @dcl/ui2 could import it. Keeping it with @deprecated rather than deleting is the correct approach for a published package.

  2. cdnReleases.spec.ts only tests macOS links: Adding a basic Windows smoke test and identity substitution test for Windows would round out coverage. Not blocking since the Windows config is unchanged.

  3. ?os=windows (lowercase) silently does nothing: The isSupportedOperativeSystem guard is case-sensitive against the enum values (Windows, macOS). This is intentional — the ?os= values come from the "Also available on…" links which use the enum directly. No action needed.

  4. Import cycle fix is a good bonus: cdnReleases.ts now imports from DownloadButton.types instead of the barrel, breaking a cycle that pulled React into the module graph.

Security

No concerns. The os query param is guarded by isSupportedOperativeSystem (only macOS or Windows pass). The value is never used in string interpolation or DOM rendering — it only selects a pre-defined CDN link.


Reviewed by Jarvis 🤖 · Requested by Braian Mellor (<@U03ACDWNHR8>) via Slack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants