Skip to content

fix: lint surfaced cleanups - #9731

Open
eordano wants to merge 3 commits into
devfrom
fix/lint-surfaced-cleanups
Open

fix: lint surfaced cleanups#9731
eordano wants to merge 3 commits into
devfrom
fix/lint-surfaced-cleanups

Conversation

@eordano

@eordano eordano commented Aug 13, 2026

Copy link
Copy Markdown
Member

All mechanical:

  • CharacterPreviewController.UpdateAvatarAsync held 'ref avatarShape'
    across structural changes (globalWorld.Create, plus the entity
    creation/destruction hidden inside Promise.Create and ForgetLoading) -
    writes through a relocated ref are silently lost. All structural work now
    completes into locals before the ref is taken. Verified on Unity
    6000.4.0f1 (GREEN, 219 assemblies).
  • DCLPlayerPrefs: shutdown warning + cleanup-timing logs migrated to
    ReportHub (the TODO claiming it was unavailable is stale - Utility
    references Diagnostics); the editor-only MenuItem log keeps Debug.Log.
  • SituationalReactionPresenter: Camera.main replaced with injected
    IExposedCameraData.CinemachineBrain?.OutputCamera, threaded
    ChatContainer -> ChatPlugin -> ChatPanelPresenter.
  • All 21 banned '// ReSharper disable CheckNamespace' comments deleted.
  • 37 dead csc.rsp files next to asmrefs deleted - Unity only honors rsp
    beside an asmdef (verified against the Bee build graph: zero references);
    orphaned by the assembly consolidation (refactor: consolidate assembly definitions #8961).
  • NRT enabled on the four uncovered assemblies: MarketplaceCredits.API,
    Character, Quality.RenderFeatures, DCL.Editor (~154 new nullable
    warnings; 'no-warning-ratchet' label applies if the ReSharper count
    rises).

…ad config

Violations surfaced by the new deterministic lint layer and DCL.Analyzers
(companion PRs), all mechanical:

- CharacterPreviewController.UpdateAvatarAsync held 'ref avatarShape'
  across structural changes (globalWorld.Create, plus the entity
  creation/destruction hidden inside Promise.Create and ForgetLoading) -
  writes through a relocated ref are silently lost. All structural work now
  completes into locals before the ref is taken. Verified on Unity
  6000.4.0f1 (GREEN, 219 assemblies).
- DCLPlayerPrefs: shutdown warning + cleanup-timing logs migrated to
  ReportHub (the TODO claiming it was unavailable is stale - Utility
  references Diagnostics); the editor-only MenuItem log keeps Debug.Log.
- SituationalReactionPresenter: Camera.main replaced with injected
  IExposedCameraData.CinemachineBrain?.OutputCamera, threaded
  ChatContainer -> ChatPlugin -> ChatPanelPresenter.
- All 21 banned '// ReSharper disable CheckNamespace' comments deleted.
- 37 dead csc.rsp files next to asmrefs deleted - Unity only honors rsp
  beside an asmdef (verified against the Bee build graph: zero references);
  orphaned by the assembly consolidation (#8961).
- NRT enabled on the four uncovered assemblies: MarketplaceCredits.API,
  Character, Quality.RenderFeatures, DCL.Editor (~154 new nullable
  warnings; 'no-warning-ratchet' label applies if the ReSharper count
  rises).
- Utility/csc.rsp: -nullable:enable -> -nullable:annotations + -nowarn:0168, so
  Utility.dll emits zero compiler messages and Unity's assembly-updater never
  reads its .mvfrm -> fixes the macOS Cloud Build incremental .mvfrm crash.
- BoneHider.cs: fully-qualify UnityEngine.FindObjectsInactive.Exclude (CS0103).
- Terrain + noise stack: propagate [Obsolete] to the legacy callers instead of
  -nowarn, zeroing CS0618/CS0612 at the root cause (all-or-nothing across the
  INoiseDataFactory/NoiseDataBase/NoiseData/VariantNoiseData/CompositeNoiseData
  GetGenerator overrides, CompositeNoiseGenerator, TerrainDetailSetter,
  TerrainModel.ChunkContains/TryOverlap, and the noise editors).
@eordano
eordano requested review from a team as code owners August 13, 2026 13:42
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

🚦 CI Status

Build

New build in progress, come back later!

Lint

Lint did not finish (failure) — the warning ratchet could not be evaluated. See logs.

Tests

⚠️ EditMode produced no results — the run likely crashed or timed out before finishing. Check the Unity Test / Test (editmode) job.

⚠️ PlayMode produced no results — the run likely crashed or timed out before finishing. Check the Unity Test / Test (playmode) job.

TESTS SUITE Result Passed Failed Skipped
EditMode ⚠️ No results
PlayMode ⚠️ No results

@decentraland-bot decentraland-bot added the ext-contribution Identifies a contribution which was not initiated by a Unity Developer label Aug 13, 2026
@github-actions
github-actions Bot requested a review from DafGreco August 13, 2026 13:42
@github-actions

Copy link
Copy Markdown
Contributor

Slack notification sent to #explorer-ext-contributions for external review.
To re-send, delete this comment and re-add the ext-contribution label.

@eordano eordano added performance force-build Used to trigger a build on draft PR labels Aug 13, 2026

@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.

PR Review — fix: lint surfaced cleanups

STEP 2 — Root-cause check

PASS. Every change addresses a cause, not a symptom:

  • CharacterPreviewController.UpdateAvatarAsync held ref AvatarShapeComponent across structural changes (ForgetLoadingworld.Destroy, Promise.Create → entity creation, globalWorld.Create). The fix correctly sequences all structural work into locals before taking the ref (CLAUDE.md §5 / DCLA001). Verified that World.Get<T>() returns ref T, so the transient chained access in the ForgetLoading call mutates in-place without a value-copy issue.
  • Camera.main in SituationalReactionPresenter replaced with injected IExposedCameraData — eliminates per-frame FindGameObjectWithTag and aligns with CLAUDE.md §11 (camera is an ECS singleton; reach for injection, not static accessors).
  • Debug.Log/LogWarning in DCLPlayerPrefs migrated to ReportHub — the stale TODO claiming ReportHub was unavailable is no longer true. The editor-only MenuItem Debug.Log correctly stays with a // lint-ignore comment (ReportHub is not initialized outside play mode).
  • Dead csc.rsp files beside .asmref files deleted (Unity only honors rsp beside an .asmdef).
  • Banned // ReSharper disable once CheckNamespace comments deleted per docs/code-style-guidelines.md § Namespaces.

STEP 3 — Design & integration

PASS. No new long-lived units introduced. The IExposedCameraData dependency is threaded through the existing composition chain: ChatContainer.CreatePluginChatPlugin constructor → ChatPanelPresenter constructor → SituationalReactionPresenter constructor. The source is staticContainer.ExposedGlobalDataContainer.ExposedCameraData, the established injection point for camera data. This follows the existing DI pattern.

Teardown trace: SituationalReactionPresenter.Dispose() unsubscribes RenderPipelineManager.beginCameraRendering -= OnBeginCameraRendering and calls cts.SafeCancelAndDispose(). The new exposedCameraData field is read-only and does not subscribe to anything — no new teardown needed. ✓

STEP 4 — Member audit

No new public properties or accessors added. The exposedCameraData field in SituationalReactionPresenter is private readonly, consumed in one place (UpdateLoopAsync line 68). This is a standard injected dependency, not a derived predicate — no merge/inline concern.

STEP 5 — Line-level review

See inline comment on Utility/csc.rsp.

STEP 6 — Complexity

COMPLEX — touches ECS structural-change ordering across archetypes, DI wiring through 4 layers, nullable reference type configuration across multiple assemblies, and assembly-level compiler settings.

STEP 7 — QA assessment

QA_REQUIRED: YESCharacterPreviewController changes affect avatar preview rendering, and SituationalReactionPresenter changes affect chat reactions camera positioning. Both are runtime user-facing paths.

STEP 8 — Non-blocking warnings

No warnings. Main scene not modified.

STEP 9 — Verdict

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches ECS ref-safety ordering, DI wiring through ChatContainer→ChatPlugin→ChatPanelPresenter→SituationalReactionPresenter, and NRT/csc.rsp configuration across multiple assemblies.
QA_REQUIRED: YES

Security review

No security issues found. No secrets, injection vectors, auth changes, or sensitive data exposure.


Reviewed by Jarvis 🤖 · Requested by eordano via Slack

@@ -1 +1,2 @@
-nullable:enable No newline at end of file
-nullable:annotations
-nowarn:0168 No newline at end of file

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] Nullable safety downgrade. This file changed from -nullable:enable (full NRT: annotations + warnings) to -nullable:annotations (annotations only, warnings silenced). This means CS8600–CS8609 null-safety diagnostics are no longer emitted for the Utility assembly — null dereferences and null-to-non-nullable assignments will compile without any warning. Additionally, -nowarn:0168 suppresses "variable declared but never used" (CS0168).

The PR description mentions NRT being enabled on four uncovered assemblies, but this change goes in the opposite direction for an assembly that already had full NRT. If the intent is to keep -nowarn:0168 only, consider:

Suggested change
-nowarn:0168
-nullable:enable
-nowarn:0168

If the downgrade to -nullable:annotations is intentional (e.g., to manage the ~154 new warnings mentioned in the description), please document the rationale — otherwise this silently regresses compile-time null safety for the entire Utility assembly.

@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.

STEP 1 — Context & Scope

PR bundles six independent mechanical cleanups: (a) CharacterPreviewController ECS ref-invalidation fix, (b) SituationalReactionPresenter Camera.mainIExposedCameraData injection, (c) DCLPlayerPrefs Debug.LogReportHub migration, (d) 37 dead csc.rsp file deletions + 4 NRT-enabling additions, (e) 21 banned // ReSharper disable CheckNamespace comment removals, (f) Landscape World-Terrain [Obsolete] annotations. Also updates BoneHider.cs deprecated FindObjectsOfTypeFindObjectsByType.

Changed files read in full context: CharacterPreviewController.cs, SituationalReactionPresenter.cs, ChatPanelPresenter.cs, ChatPlugin.cs, ChatContainer.cs, DCLPlayerPrefs.cs, TerrainModel.cs, LandscapeAudioCullingSystem.cs, BoneHider.cs.


STEP 2 — Root-cause check: PASS

Each change fixes the actual cause:

  1. CharacterPreviewController — The original code held ref AvatarShapeComponent across structural changes (ForgetLoading, Promise.Create, globalWorld.Create), violating CLAUDE.md §5: "NEVER perform structural changes after obtaining a ref." The fix correctly reorders: complete all structural work into locals, then take the ref and write fields. This fixes the root cause (ref invalidation), not a symptom.

  2. Camera.main replacementCamera.main is a per-call FindObjectOfType in hot code. The fix injects IExposedCameraData (the canonical camera source per CLAUDE.md §8/§11), fixing the cause of the anti-pattern.

  3. Debug.Log → ReportHub — Direct fix of a CLAUDE.md convention violation.

  4. csc.rsp cleanup — The orphaned files were verified against the Bee build graph as dead. New NRT files target assemblies that actually need them.

  5. ReSharper disable removal — Banned per docs/code-style-guidelines.md § Namespaces.

  6. [Obsolete] annotations — Properly marks deprecated World-Terrain infrastructure with a descriptive OBSOLESCENCE_MESSAGE.


STEP 3 — Design & integration: PASS

No new long-lived units introduced. All changes modify existing classes or delete dead files.

Camera dependency threading (ChatContainerChatPluginChatPanelPresenterSituationalReactionPresenter):

  • ChatContainer.CreatePlugin() passes staticContainer.ExposedGlobalDataContainer.ExposedCameraData — the canonical source, already available at this scope.
  • ChatPlugin stores it and passes to ChatPanelPresenter constructor.
  • ChatPanelPresenter passes to SituationalReactionPresenter constructor.
  • Correct top-down DI pattern per CLAUDE.md.

CharacterPreviewController reordering: No new units — same operations in a safe order. The one-line globalWorld.Get<AvatarShapeComponent>(entity).WearablePromise.ForgetLoading(globalWorld) uses a temporary ref discarded at the semicolon; the ref is never used post-structural-change. Since WearablePromise is entirely replaced on the next ref-access, any self-mutation lost during ForgetLoading does not affect correctness.

Teardown/consumption trace for SituationalReactionPresenter:

  • RenderPipelineManager.beginCameraRendering += OnBeginCameraRendering (line 56) → -= OnBeginCameraRendering in Dispose() (line 61). ✅
  • new CancellationTokenSource() (line 27) → cts.SafeCancelAndDispose() in Dispose() (line 62). ✅
  • exposedCameraData is read-only (no subscription). ✅

STEP 4 — Member audit: PASS

No new public properties or accessors. Only constructor parameters (IExposedCameraData) added to existing classes and a private field exposedCameraData in SituationalReactionPresenter. The field is consumed in UpdateLoopAsync (line 69) — single consumer, but it's a stored dependency (not a derived predicate), so this is legitimate encapsulation.


STEP 5 — Line-level review

See inline comments below. Two P2 findings (non-blocking).

Also noted (not findings):

  • cachedMainCamera field naming is now slightly misleading since it's no longer populated from Camera.main. Consider renaming to cachedOutputCamera in a follow-up — not blocking since the old code had the same name and the semantic difference is minor.
  • BoneHider.cs: FindObjectsByType<T>(FindObjectsInactive.Exclude) uses the unsorted overload, which is correct — sort order is irrelevant for visibility toggling.
  • -nullable:annotations (Utility): Downgrade from enable to annotations means NRT analysis warnings are no longer emitted, but annotations are still tracked for consumers. Reasonable stance for a utility assembly with many pre-existing issues.
  • Editor landscape types (CompositeNoiseDataEditor, NoiseDataEditor, etc.) use bare [System.Obsolete] without OBSOLESCENCE_MESSAGE — acceptable since these are CustomEditor wrappers that Unity invokes automatically; the attribute just suppresses compiler warnings about referencing obsolete types.

STEP 6 — Complexity: COMPLEX

Touches ECS structural change ordering (CharacterPreviewController ref safety), camera dependency injection chain across 4 classes, assembly-level NRT configuration (csc.rsp), and landscape obsolescence marking. Over 100 files changed, even though most are mechanical deletions.


STEP 7 — QA assessment: YES

Runtime code is modified: avatar preview loading (CharacterPreviewController), chat reactions camera access (SituationalReactionPresenter), player prefs logging (DCLPlayerPrefs). All affect user-facing behavior in the Unity player.


STEP 8 — Non-blocking warnings

No Main.unity scene file modified. No warnings to emit.


STEP 9 — Verdict

CI Status: Edit-mode tests FAIL (Docker exit code 1 / "No test report files found" — CI infrastructure issue, not code-related). Play-mode tests CANCELLED. Lint PENDING. The failures produce no test artifacts, suggesting the test runner failed to start rather than code changes causing test failures.

Security review: No security issues found. No secrets, credentials, injection vectors, auth changes, or unsafe input handling introduced.

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches ECS structural-change ordering, camera DI chain across four classes, assembly-level NRT/warning configuration, and landscape obsolescence marking
QA_REQUIRED: YES


Reviewed by Jarvis 🤖 · Requested by unknown via Slack

@@ -1 +1,2 @@
-nullable:enable No newline at end of file
-nullable:annotations
-nowarn:0168 No newline at end of file

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] -nowarn:0168 suppresses CS0168 ("variable declared but never used") assembly-wide. This warning catches dead variables and missed error handling (e.g. catch (Exception ex) where ex is silently swallowed). Blanket suppression can mask future issues.

Consider targeted #pragma warning disable 0168 at specific catch sites instead, or file a follow-up issue to fix the underlying unused variables.

Suggested change
-nowarn:0168
-nowarn:0168

(If the count of pre-existing CS0168 warnings is large and this is a deliberate trade-off, please note that in the PR body or as a code comment for future maintainers.)

public void ApplyDetailLayer(TerrainData terrainData, int i, int[,] detailLayer) { }
}

[Obsolete]

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] Inconsistent [Obsolete] usage — other landscape types in this PR use TerrainModel.OBSOLESCENCE_MESSAGE for discoverability (e.g. NoiseData.GetGenerator, INoiseDataFactory.GetGenerator). CPUTerrainDetailSetter is a runtime type and should follow the same pattern.

Suggested change
[Obsolete]
[Obsolete(TerrainModel.OBSOLESCENCE_MESSAGE)]

@eordano eordano removed performance ext-contribution Identifies a contribution which was not initiated by a Unity Developer labels Aug 13, 2026
…ype overload

Both BoneHider menu items passed FindObjectsInactive.Exclude as the only
argument, leaving the sort mode to whichever overload the compiler resolves -
the shape the review read as a compile break. They only flip a flag on every
result, so instance-ID ordering is work nobody consumes, and the repo's other
call site (NearbyAudioSourceFactoryShould.cs:46) already spells the pair out.
Naming FindObjectsSortMode.None explicitly restores the pre-change
FindObjectsOfType<T>() semantics (inactive excluded, unsorted) and leaves no
overload ambiguity to resolve.

Utility/AssemblyInfo.cs now carries the constraint behind that assembly's
csc.rsp: Utility must compile with zero compiler diagnostics, because any
message emitted for Utility.dll makes Unity's assembly updater read
Utility.mvfrm and crashes the incremental macOS Cloud Build. That is the reason
for -nullable:annotations + -nowarn:0168; a .rsp file has nowhere to hold it,
so without this the flags read as an arbitrary downgrade and get "fixed" back.

FIXNOTES.md records the two review items that take no code change: the
CheckNamespace ban behind the 21 removed suppressions already landed in
05db1c0 (#9396), an ancestor of this PR's base, so merge ordering is
satisfied and no .editorconfig override is wanted (§ Namespaces prescribes
leaving the inspection visible); and the fresh green Editor-assembly build owed
to this branch's head cannot be produced outside the Unity Cloud lanes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018c638dR1vPysCMbYt2qQg5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

force-build Used to trigger a build on draft PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants