Skip to content

Conversation

@tudddorrr
Copy link
Contributor

No description provided.

@tudddorrr tudddorrr added the enhancement New feature or request label Jan 3, 2026
@claude
Copy link

claude bot commented Jan 3, 2026

Code Review: Player Relationships API


🔴 Critical Issues

Memory leak - Event handler never unsubscribed (PlayerRelationshipsAPI.cs:96)

The socket event handler is registered in the constructor but never cleaned up:

public PlayerRelationshipsAPI() : base("v1/players/relationships")
{
    Talo.Socket.OnMessageReceived += HandleSocketMessage;
}

This means every time a PlayerRelationshipsAPI instance is created, a new handler is added to Talo.Socket.OnMessageReceived, but these handlers are never removed. If the API is instantiated multiple times (unlikely given the singleton pattern in Talo.cs, but possible in tests or edge cases), this causes a memory leak and duplicate event firing.

Fix: Add cleanup, similar to how other resources are managed. Consider implementing IDisposable or adding an internal cleanup method called by TaloManager.


🔵 Minor Issues

Code duplication in query string builders (PlayerRelationshipsAPI.cs:30-51, 62-83)

GetSubscribersOptions.ToQueryString() and GetSubscriptionsOptions.ToQueryString() are identical implementations. This violates DRY and makes maintenance harder.

Fix: Extract to a shared base class or helper method:

public abstract class BaseSubscriptionOptions
{
    public int page = 0;
    public ConfirmedFilter confirmed = ConfirmedFilter.Any;
    public int aliasId = -1;
    public RelationshipTypeFilter relationshipType = RelationshipTypeFilter.Any;
    
    public string ToQueryString() { /* existing implementation */ }
}

public class GetSubscribersOptions : BaseSubscriptionOptions { }
public class GetSubscriptionsOptions : BaseSubscriptionOptions { }

Other Categories

Performance considerations: No issues found

Security concerns: No issues found

Backwards compatibility: No issues found (new feature)

@tudddorrr tudddorrr force-pushed the player-relationships branch from d5af2e1 to fad0021 Compare January 3, 2026 20:18
@tudddorrr tudddorrr merged commit 35bad87 into develop Jan 3, 2026
2 checks passed
@tudddorrr tudddorrr deleted the player-relationships branch January 3, 2026 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants