Skip to content

Conversation

@tudddorrr
Copy link
Contributor

Fixes #170

@claude
Copy link

claude bot commented Dec 4, 2025

Code Review - PR #171

🟡 Potential Bugs

Position collision when bumping sparse cache (leaderboard_entries_manager.gd:44-47)

The position bumping logic can create duplicate positions when the cache contains non-contiguous entries:

# Current code
if bump_positions:
    for e in named_entries:
        if e.id != entry.id and e.position >= entry.position:
            e.position += 1

Scenario: Cache has entries at positions [1, 2, 3, 50, 51]. User adds entry that gets position 49 from server.

Result:

  • Entry at position 50 bumps to 51 (collision with existing entry!)
  • Entry at position 51 bumps to 52
  • Cache now has two entries at position 51

Root cause: The logic assumes contiguous positions but caches can be sparse (e.g., fetching only top 10 from a leaderboard with 100 entries).

Suggested fix: Only bump if the entry would actually be displaced by the insertion:

if bump_positions:
    for e in named_entries:
        if e.id != entry.id and e.position >= entry.position:
            # Only bump entries that are actually below the new entry in the sorted array
            var new_entry_index := named_entries.find(entry)
            var e_index := named_entries.find(e)
            if e_index > new_entry_index:
                e.position += 1

Alternatively, consider whether position bumping is necessary at all—fetching fresh data via get_entries() after add_entry() would ensure accurate positions from the server.


✅ Code Quality

The type safety improvements (:= operator, explicit int type) correctly follow the GDScript standards in CLAUDE.md.


Overall: The core idea of preserving API positions is sound, but the position bumping optimization has an edge case bug with sparse caches.

@tudddorrr tudddorrr force-pushed the preserve-leaderboard-positions branch from d890be7 to f8df49d Compare December 4, 2025 07:28
@tudddorrr tudddorrr merged commit bc6c353 into develop Dec 4, 2025
5 checks passed
@tudddorrr tudddorrr deleted the preserve-leaderboard-positions branch December 4, 2025 07:35
@tudddorrr tudddorrr added the fix This pull request contains a fix label Dec 4, 2025
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.

Position of current user for leaderboards

2 participants