fix: voice chat tip frequency - #9742
Conversation
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings count reduced: 13156 => 13154 Warnings/errors in files changed by this PR (11)All Unity tests passed ✅
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — fix: voice chat tip frequency
STEP 2 — Root-cause check: PASS ✅
The problem is that the Nearby Voice Chat intro tip shows on the very first session and reappears every launch until dismissed. The root cause was the original NearbyVoiceTipFlow class, which had no scheduling logic — it simply showed on every launch gated only by a single NEARBY_VOICE_TIP_DISMISSED pref.
The diff fixes the actual cause by replacing the inline flow with:
- A
NearbyVoiceTipSchedulestruct with configurable frequency and max-display cap - A
NearbyVoiceTipControllerthat manages schedule checking, panel-covering awareness, voice-use detection, and retirement - A feature flag kill-switch (
alfa-nearby-voice-chat-tip)
STEP 3 — Design & integration: PASS ✅
New long-lived units introduced:
NearbyVoiceTipController— manages the tip view lifecycle (schedule, show/hide, retire)NearbyVoiceTipSchedule— pure value-type (readonly struct) for schedule logic, no lifecycle
Owner search for NearbyVoiceTipController:
- The tip view (
NearbyVoiceTipView) is constructed in the sidebar and injected throughVoiceChatPlugin. - The lifecycle owner is
VoiceChatPlugin, which creates the controller and adds it topluginScopefor disposal — matching the pattern used byNearbyVoiceChatButtonController(line 253) andNearbyVoiceWidgetController(line 256). - The old tip logic was an inline static method
RunNearbyVoiceTipAsync+ nestedNearbyVoiceTipFlowwithin VoiceChatPlugin. - Extraction is justified: the controller holds its own subscription state (event bus for MVC view open/close, voice state subscription, CTS management, prefs tracking). Inlining this into the plugin would bloat a class that already creates ~15 objects. The pattern is consistent with existing nearby-voice controllers in the same namespace.
Teardown / consumption trace — all subscriptions have matching teardowns:
stateSubscription→Dispose()line 63 ✅scope(event bus subscriptions) →Dispose()line 64 ✅cts(CancellationTokenSource) →SafeCancelAndDispose()inDispose()(line 61) andRetire()(line 143) ✅- The controller itself → added to
pluginScopein VoiceChatPlugin (line 263) ✅
STEP 4 — Member audit: PASS ✅
NearbyVoiceTipSchedule.ShowEverySessions(public readonly field) — consumed byShouldShowinternally and by the clamping assertion inClampANonPositivePeriodSoTheTipIsNotDueEveryLaunchtest. Legitimate public exposure for testability.NearbyVoiceTipSchedule.MaxTimesShown(public readonly field) — symmetric withShowEverySessions; struct fields are value-copied so exposure is harmless.NearbyVoiceTipSchedule.ShouldShow(...)— core logic, 11 test cases covering thresholds, caps, returning users, voice-use suppression, custom frequencies, disabled schedule, and degenerate input.NearbyVoiceTipSchedule.FromFeatureFlags(...)— factory method, single consumer (VoiceChatPlugin line 260). Standard factory pattern.NearbyVoiceTipSchedule.Disabled— used in VoiceChatPlugin (line 261) and tested inNeverShowWhenDisabled. Two consumers.
No single-use accessor anti-patterns. No absent ≠ false/null conflation.
STEP 5 — Line-level review
See inline comments below.
STEP 6 — Complexity assessment
COMPLEX — introduces new async flow with CTS management, event bus subscriptions, feature flag integration, and plugin wiring changes. Touches 7+ files with ~450 lines of changes.
STEP 7 — QA assessment
QA_REQUIRED: YES — modifies runtime UI code that controls when and how often the voice chat tip is shown to users.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
Security review
No security issues found. No hardcoded secrets, no auth/authz changes, no sensitive data exposure. Feature flag payload uses nullable types with defaults and input validation (Math.Max).
Consumer impact
No public API surface is modified. All new types are internal to the VoiceChat subsystem. New FeatureId.NearbyVoiceChatTip enum value is additive. No breaking changes.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Introduces new controller with async/CTS management, event bus subscriptions, feature flag integration, and plugin wiring changes across the VoiceChat subsystem.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
This comment has been minimized.
This comment has been minimized.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: fix: voice chat tip frequency
STEP 2 — Root-cause check: ✅ PASS
The original problem was that the Nearby Voice Chat intro tip appeared on every launch until explicitly dismissed — hitting brand-new users on their very first session. The diff addresses the root cause by introducing proper scheduling logic (frequency interval, display cap, user-discovery detection) rather than working around a symptom.
STEP 3 — Design & integration: ✅ PASS
Owner search for NearbyVoiceTipController:
| Question | Answer |
|---|---|
| What does it manage? | NearbyVoiceTipView (intro tip UI), display scheduling, and nearby-voice-used tracking |
| Who already owns creation/disposal? | VoiceChatPlugin — creates all nearby voice chat controllers in InitializeAsync() and disposes via pluginScope |
| Files checked | VoiceChatPlugin.cs, NearbyVoiceChatButtonController, NearbyVoiceWidgetController, NearbyVoiceChatSuppressor, VoiceChatPanelPresenter |
| Could this live in the existing owner? | It does — the controller is instantiated inside VoiceChatPlugin.InitializeAsync() and added to pluginScope (line 263), exactly like the other 17 pluginScope.Add() calls in the same file |
The old code was a static nested class NearbyVoiceTipFlow with a detached async and a separate CancellationTokenSource field on the plugin. The new design moves this into a self-contained IDisposable controller — cleaner ownership, same lifecycle home.
Schedule/Controller separation is justified:
NearbyVoiceTipScheduleis a purereadonly structwith no side effects — independently testable (13 test cases)NearbyVoiceTipControllermanages 3 disposal chains (CTS, event scope, state subscription) and interactive state- Neither is a bridge/wrapper; each has distinct responsibility
Teardown / consumption trace — all resources matched:
| Resource | Opened | Closed | Line |
|---|---|---|---|
CancellationTokenSource |
Constructor (L56) | Retire() (L158) + Dispose() (L62) via SafeCancelAndDispose() |
✅ |
EventSubscriptionScope (MVCViewOpen/Close) |
Constructor (L53–54) | Dispose() → scope.Dispose() (L65) |
✅ |
DisposableSubscription<NearbyVoiceChatState> |
Constructor (L47) | Dispose() → stateSubscription.Dispose() (L64) |
✅ |
The stateSubscription intentionally outlives the tip display — it tracks nearby-voice-used discovery even when the tip is not scheduled, so enabling the flag later doesn't resurface it for users who already found the feature on their own. This is correct.
STEP 4 — Member audit: ✅ PASS
| Member | Consumers | Verdict |
|---|---|---|
ShowEverySessions |
ShouldShow(), test ClampANonPositivePeriodSoTheTipIsNotDueEveryLaunch |
Public readonly field on readonly struct — legitimate |
MaxTimesShown |
ShouldShow() |
Same — read by core logic |
ShouldShow() |
NearbyVoiceTipController.IsDue(), 13 test cases |
Pure function, well-tested |
FromFeatureFlags() |
VoiceChatPlugin |
Factory appropriately located on the struct |
Disabled |
VoiceChatPlugin, test |
Sentinel value (maxTimesShown=0) — correct |
No single-use intermediates that should be merged. No absent-means-false predicates.
STEP 5 — Line-level review: ✅ No blocking issues
Checked and verified:
- Naming: PascalCase for types/methods/properties, camelCase for locals/params — consistent ✅
- Async (§9):
ShowWhenReadyAsyncisasync UniTaskVoidwith.Forget(), CTS-lifecycled, catchesOperationCanceledException— follows existing pattern (NearbyMuteService.LoadAsync(ct).Forget()in same file) ✅ - CTS management:
SafeCancelAndDispose()extension handles null correctly (null-conditional?.Cancel()/?.Dispose()) ✅ - Event pattern:
MVCViewOpenEvent/MVCViewClosedEventviaChatSharedAreaEventBus+EventSubscriptionScope— matchesVoiceChatPanelPresenter✅ - DTO (§11):
ConfigDtostruct with// ReSharper disable InconsistentNaming, nullableint?for optional fields — correct ✅ - Input validation:
Math.Max(1, showEverySessions)prevents zero/negative period;Math.Max(0, maxTimesShown)prevents negative cap ✅ - Feature flag: Kill-switch pattern (off until explicitly enabled) with
FeatureId.NearbyVoiceChatTip = 74— correct next enum value after 73 ✅ - Unused import cleanup: Removed
LiveKit.Rooms.Streaming.AudioandUtility.Multithreading— good hygiene ✅ - Nullability:
CancellationTokenSource?properly annotated; no unjustified null-forgiving operators ✅ coveringViewscounter:Math.Max(0, coveringViews - 1)prevents underflow from mismatched open/close events ✅- Test coverage: 13 test cases covering thresholds, caps, returning users, custom frequencies, disabled schedule, and degenerate input — thorough for the pure logic ✅
- Documentation:
docs/feature-flags.mdupdate is clear and complete ✅ - Unrelated .meta files:
EventsStateServiceLookupPerformanceTest.cs.metaandPlacesStateServiceLookupPerformanceTest.cs.metaare missing-meta fixes — harmless ✅
STEP 6 — Complexity
COMPLEX — introduces new controller + schedule struct with async lifecycle, feature flag integration, event-driven UI state management, and unit tests across 14 files.
STEP 7 — QA assessment
QA_REQUIRED: YES — changes user-facing behavior (when and how often the voice chat intro tip appears), modifies runtime code.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
Security review
No security issues found. The code reads server-side feature flag configuration via the existing TryGetJsonPayload() API with type-safe deserialization and default fallbacks. PlayerPrefs stores only non-sensitive display metrics (count, launch number, boolean flags). No secrets, injection vectors, or auth concerns.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: New controller + schedule struct with async lifecycle, feature flag integration, event-driven UI state, and tests across 14 files
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
This comment has been minimized.
This comment has been minimized.
# Conflicts: # Explorer/Assets/DCL/Tests/PlayMode/PerformanceTests/EventsStateServiceLookupPerformanceTest.cs.meta # Explorer/Assets/DCL/Tests/PlayMode/PerformanceTests/PlacesStateServiceLookupPerformanceTest.cs.meta
This comment has been minimized.
This comment has been minimized.
|
PR #9742, run #31815319012 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
✅ Approve by QATested on Windows and Mac. Existing account
New account
No blockers on either platform. pass-9742.mp4 |
✅ Approve by QATested on Windows and Mac. Existing account
New account
No blockers on either platform. 9742-evi.mp4 |
6 participants
What does this PR change?
Changes when the Nearby Voice Chat intro tip shows up. Closes #9708 and #9696, supersedes #9698.
Today it hits brand new users on their very first session, and comes back every launch until they click one of its buttons. Now it waits a few sessions, shows at most twice, skips anyone who already uses Nearby Voice Chat, and hides when you open another panel instead of sitting on top of it.
It also gets its own feature flag,
alfa-nearby-voice-chat-tip, so it can be turned off remotely. How often it shows and how many times are set in the flag'sconfigvariant:{ "showEverySessions": 5, "maxTimesShown": 2 }With those values a new user sees it on session 5 and again on session 10.
Test Instructions
Test steps for existing account
Test Steps for new account
Steps (fresh account):
metaforge account create --clear metaforge explorer run XXXX # ← replace with this PR number~/Library/Application Support/Decentraland/Explorer/userdata_{n}.jsonQuality Checklist
docs/feature-flags.md)