Add player identity clearing

[tudddorrr]

No description provided.

[claude]

🔍 Code Review

🛠️ Code Quality & Best Practices

Method signature concerns:

• ClearIdentity() in PlayersAPI.cs:167 is marked async void which should be avoided. Consider making it async Task to enable proper error handling and awaiting by callers.

Visibility changes:

• Changing queue and eventsToFlush from private to internal (EventsAPI.cs:13,16) breaks encapsulation. Consider adding public methods for testing access instead.

---

🐛 Potential Issues

Exception handling:

• ClearIdentity() calls Talo.IdentityCheck() and await Talo.PlayerAuth.SessionManager.ClearSession() without try-catch blocks. If these throw exceptions, the method will terminate early and skip clearing queues/continuity requests, leaving the system in an inconsistent state.

State consistency:

• If SessionManager.ClearSession() fails, the offline alias is already deleted but events/continuity queues remain populated, creating inconsistent state.

---

⚡ Performance Considerations

File I/O operations:

• DeleteOfflineAlias() performs file operations on the main thread. Consider moving to background thread if this becomes a bottleneck.

---

🔒 Security Concerns

File deletion robustness:

• The DeleteOfflineAlias() method only logs warnings on failure. Consider whether partial cleanup poses security risks or if stronger guarantees are needed.

---

⬅️ Backwards Compatibility

API changes:

• Adding internal visibility to previously private fields could break existing code that relied on reflection access patterns.
• New OnIdentityCleared event maintains backwards compatibility as it's additive.

Test methodology:

• The test uses yield return with Talo.Events.Track() which returns a Task, but coroutines expect IEnumerator. This may not work as expected and could mask timing issues in the clear operation.

[claude]

🔍 Code Review: Player Identity Clearing Feature

🚨 Security Concerns

File system race conditions in DeleteOfflineAlias() (Assets/Talo Game Services/Talo/Runtime/APIs/PlayersAPI.cs:152)

• No synchronization when checking File.Exists() and calling File.Delete()
• Another thread could delete the file between the existence check and deletion attempt
• Consider using a try-catch around File.Delete() without the existence check

---

⚠️ Potential Bugs & Issues

Inconsistent async/await pattern in ClearIdentity() (Assets/Talo Game Services/Talo/Runtime/APIs/PlayersAPI.cs:167)

• Method is marked async but calls synchronous methods (DeleteOfflineAlias, Talo.Events.ClearQueue, Talo.Continuity.ClearRequests)
• Only SessionManager.ClearSession() is awaited
• Consider making other operations async if they perform I/O, or remove async if not needed

Missing null safety checks

• ClearIdentity() calls Talo.IdentityCheck() but doesn't handle potential exceptions
• No validation that required components exist before clearing state

Test reliability issues (Assets/Talo Game Services/Talo/Tests/PlayersAPI/ClearIdentityTest.cs:41)

• Test directly accesses Talo.Events.queue which is now internal - this creates tight coupling
• Consider testing through public APIs or adding proper test accessors

---

🏗️ Code Quality

Visibility changes break encapsulation (Assets/Talo Game Services/Talo/Runtime/APIs/EventsAPI.cs:13,16)

• Changed queue and eventsToFlush from private to internal solely for testing
• This exposes internal implementation details
• Consider adding a public method to check queue state or using reflection in tests

Exception swallowing in ClearIdentity()

• Two separate try-catch blocks that only log warnings but continue execution
• Could mask important failures that should be propagated to the caller
• Consider letting critical errors bubble up or providing more structured error handling

---

🚀 Performance Considerations

Unnecessary Update() modification (Assets/Talo Game Services/Talo/Runtime/APIs/PlayersAPI.cs:85)

• Adding Talo.IdentityCheck() to every Update() call introduces overhead
• This check will run on every player update, not just when identity might be cleared
• Consider moving this check to methods that actually modify identity state

File I/O on main thread

• DeleteOfflineAlias() performs synchronous file operations
• Could cause frame drops in Unity if called during gameplay
• Consider making file operations async

---

🔄 Backwards Compatibility

New public event without versioning

• OnIdentityCleared event added to public API without version considerations
• Existing code that relies on identity state might not handle the new clearing behavior
• Consider documenting migration path for existing integrations

State clearing order

• No guarantee that external systems depending on player identity are notified before state is cleared
• Event fires after all clearing operations complete
• Consider firing event before clearing or providing pre/post events