Skip to content

fix(ui): guard manual Animator.Update against inactive objects #9434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
eordano wants to merge 1 commit into
base: dev
Choose a base branch
from
+14 −4
Open

fix(ui): guard manual Animator.Update against inactive objects #9434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions Explorer/Assets/DCL/UI/ButtonWithAnimationView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,14 @@ private void OnEnable()
{
Button.onClick.AddListener(OnClick);
ButtonAnimator.enabled = true;
ButtonAnimator.Rebind();
ButtonAnimator.Update(0);

// Animator.Update is only legal on an active-in-hierarchy object; these buttons
// are often enabled while their panel is still hidden during UI bootstrap.
if (ButtonAnimator.gameObject.activeInHierarchy)
{
ButtonAnimator.Rebind();
ButtonAnimator.Update(0);
}
Comment on lines +27 to +33

@decentraland-bot decentraland-bot Jul 19, 2026

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.

[P2] Centralization opportunity: The Rebind() + Update(0) pattern with the activeInHierarchy guard is repeated inline across all 3 files in this PR, and there are 10+ additional unguarded Rebind() + Update(0) call sites in the codebase (e.g. BackpackController, PlacesView, EventsView, SettingsController, CommunitiesBrowserView, NavmapController, CameraReelController, GiftingTabManager, AvatarTabNavigator).

The codebase already has AnimatorExtensions.ResetAnimator() doing exactly Rebind() + Update(0f) — but without the guard. Consider adding a safe variant there so all call sites benefit from the fix and the guard logic isn't duplicated:

// In AnimatorExtensions.cs
public static bool TryResetAnimator(this Animator animator)
{
    if (!animator.gameObject.activeInHierarchy) return false;
    animator.Rebind();
    animator.Update(0f);
    return true;
}

Then this block simplifies to:

Suggested change
// Animator.Update is only legal on an active-in-hierarchy object; these buttons
// are often enabled while their panel is still hidden during UI bootstrap.
if (ButtonAnimator.gameObject.activeInHierarchy)
{
ButtonAnimator.Rebind();
ButtonAnimator.Update(0);
}
ButtonAnimator.TryResetAnimator();

}

private void OnDisable()
Expand Down
4 changes: 3 additions & 1 deletion Explorer/Assets/DCL/UI/SectionSelectorController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ public void SetAnimationState(bool isOn, TabSelectorView selectorToggle)

if (isOn)
selectorToggle.tabAnimator.SetTrigger(UIAnimationHashes.ACTIVE);
else
// Animator.Update is only legal on an active-in-hierarchy object; non-selected
// tabs are frequently snapped to their resting frame while still hidden.
else if (selectorToggle.tabAnimator.gameObject.activeInHierarchy)
{
selectorToggle.tabAnimator.Rebind();
selectorToggle.tabAnimator.Update(0);
Expand Down
4 changes: 3 additions & 1 deletion Explorer/Assets/DCL/UI/TabSelectorView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ private void OnEnable()
{
tabAnimator.enabled = true;

if (tabAnimator != null)
// Animator.Update is only legal on an active-in-hierarchy object; tabs are
// frequently enabled while their panel is still hidden during UI bootstrap.
if (tabAnimator != null && tabAnimator.gameObject.activeInHierarchy)

@decentraland-bot decentraland-bot Jul 19, 2026

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.

[P2] Pre-existing null-check inconsistency (CLAUDE.md §11 "Defensive null-checks against non-null declarations"): tabAnimator is assumed non-null on line 40 (tabAnimator.enabled = true) and line 56 (tabAnimator.enabled = false), but this guard checks tabAnimator != null. Either the field can legitimately be null (in which case lines 40 and 56 also need guards) or it cannot (in which case the null check here is noise).

Since this PR didn't introduce the inconsistency, this is a pre-existing observation — but worth aligning when touching this code. If the field is always assigned via the inspector:

Suggested change
if (tabAnimator != null && tabAnimator.gameObject.activeInHierarchy)
if (tabAnimator.gameObject.activeInHierarchy)

{
tabAnimator.Rebind();
tabAnimator.Update(0);
Expand Down
Loading