Skip to content

Conversation

@tudddorrr
Copy link
Contributor

No description provided.

@tudddorrr tudddorrr added the fix This pull request contains a fix label Dec 6, 2025
@claude
Copy link

claude bot commented Dec 6, 2025

PR Review: Preserve entry positions in cache

After reviewing the changes, I found one issue that should be addressed:


🟡 Code Quality

Position mutation of cached objects

The bump logic directly mutates the position field of cached LeaderboardEntry objects (LeaderboardEntriesManager.cs:40):

foreach (var e in entries)
{
    if (e.id != entry.id && e.position >= entry.position)
    {
        e.position += 1;  // Mutates cached object
    }
}

If external code holds references to these entries (via GetCachedEntries() or GetCachedEntriesForCurrentPlayer()), their positions will change unexpectedly when AddEntry is called. This creates potential confusion where a reference that was position=1 suddenly becomes position=2 without the holder knowing.

Suggested fix: Document this behavior in the method XML comments, or consider if entries should be immutable with a new instance created when positions change. Given the caching architecture, documentation is likely sufficient:

/// <summary>
/// Note: When bumpPositions=true, this method mutates the position field of existing cached entries.
/// </summary>
public void UpsertEntry(string internalName, LeaderboardEntry entry, bool bumpPositions = false)

All other aspects look good - the implementation correctly handles the insert/update distinction, tests cover the new behavior thoroughly, and the change aligns with the PR's stated goal.

@tudddorrr tudddorrr force-pushed the preserve-entry-positions branch from d69bb0f to 132a47c Compare December 6, 2025 22:14
@tudddorrr tudddorrr force-pushed the preserve-entry-positions branch from 132a47c to e104d96 Compare December 6, 2025 22:16
@tudddorrr tudddorrr merged commit acc5f0c into develop Dec 6, 2025
2 checks passed
@tudddorrr tudddorrr deleted the preserve-entry-positions branch December 6, 2025 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix This pull request contains a fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants