Skip to content

Correctly calculate global value after stat reset#878

Merged
tudddorrr merged 1 commit intodevelopfrom
reset-stat-calculate-global-value
Mar 5, 2026
Merged

Correctly calculate global value after stat reset#878
tudddorrr merged 1 commit intodevelopfrom
reset-stat-calculate-global-value

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr added the fix This pull request contains a fix label Mar 5, 2026
@tudddorrr tudddorrr changed the title correctly calculate global value after stat reset Correctly calculate global value after stat reset Mar 5, 2026
@claude
Copy link

claude bot commented Mar 5, 2026

PR Review: Correctly calculate global value after stat reset

The fix is logically sound and test coverage is good. A couple of things worth addressing:


🔵 Minor — Confusing parameter interaction in recalculateGlobalValue

The devOnly/includeDevData combination is non-obvious. When mode === "live", the caller passes includeDevData: true not because it wants all data included, but purely to avoid triggering the devBuild: false path — while devOnly: true then overrides to filter only dev builds. The two parameters interact in a way that requires reading the implementation to understand the caller intent.

A single filter parameter would be clearer:

// game-stat.ts
async recalculateGlobalValue({ em, filter }: { em: EntityManager; filter: 'all' | 'live' | 'dev' }) {
  const qb = em.qb(PlayerGameStat, 'pgs').select(raw('SUM(pgs.value) as total')).where({ stat: this.id })
  if (filter \!== 'all') {
    qb.innerJoin('pgs.player', 'p').andWhere({ 'p.devBuild': filter === 'dev' })
  }
  ...
}

// reset.ts
await stat.recalculateGlobalValue({ em: trx, filter: mode })

This maps directly to the mode values and removes the dual-parameter ambiguity.


🔵 Minor — Redis set not error-isolated unlike the cache-clear calls below it

// reset.ts (after transaction)
await em.refresh(stat)
await ctx.redis.set(GameStat.getGlobalValueCacheKey(stat.id), stat.globalValue)  // ← unguarded

await Promise.allSettled([   // ← these are safely fire-and-forget
  deferClearResponseCache(...),
  deferClearResponseCache(...),
])

If ctx.redis.set throws, the client receives a 500 even though the DB update committed successfully. The deferClearResponseCache calls are correctly wrapped in Promise.allSettled to avoid this. The Redis cache update should be similarly protected, or the error should be caught and logged rather than surfaced to the caller.


No issues with the core recalculation logic — the query filtering for each mode is correct and the new tests validate the right behaviour.

@tudddorrr tudddorrr force-pushed the reset-stat-calculate-global-value branch from 0bc99ca to f9bcbcd Compare March 5, 2026 00:11
@tudddorrr tudddorrr force-pushed the reset-stat-calculate-global-value branch from f9bcbcd to a285d06 Compare March 5, 2026 00:16
@tudddorrr tudddorrr merged commit aa97c86 into develop Mar 5, 2026
8 checks passed
@tudddorrr tudddorrr deleted the reset-stat-calculate-global-value branch March 5, 2026 00:19
@codecov
Copy link

codecov bot commented Mar 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.07%. Comparing base (5c1c9c5) to head (a285d06).
⚠️ Report is 5 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #878      +/-   ##
===========================================
+ Coverage    97.03%   97.07%   +0.03%     
===========================================
  Files          390      390              
  Lines         6179     6184       +5     
  Branches       792      794       +2     
===========================================
+ Hits          5996     6003       +7     
  Misses          94       94              
+ Partials        89       87       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant