Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the vote counting logic in the updateThousandVotesIncremental method of RankReportHistoryBuilder. The changes fix how the system maintains a sliding window of the most recent 1000 votes by correctly calculating how many old votes to remove when new votes are added.
Changes:
- Improved calculation of votes to remove by introducing
$countToRemovebased on total vote count instead of simply matching new vote count - Enhanced handling of
minIdinitialization for first-time setup (whenminId == 0) - Updated comments for better code clarity and documentation in Chinese
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| // [情況 B] 總數還沒滿 1000，不需要移除舊資料 | ||
| // 但如果是第一次建立 ($minId == 0)，需要設定 minId | ||
| elseif ($minId == 0 && $newCount > 0) { |
There was a problem hiding this comment.
Inconsistent comparison operators for checking minId. Line 353 uses strict comparison ($minId > 0) while line 379 uses loose equality ($minId == 0). For consistency and to avoid potential type coercion issues, use the same comparison style throughout. Consider using strict equality (===) for the zero check on line 379.
| elseif ($minId == 0 && $newCount > 0) { | |
| elseif ($minId === 0 && $newCount > 0) { |
| elseif ($minId == 0 && $newCount > 0) { | ||
| // 因為 $newVotes 是 orderByDesc，所以最後一筆是 ID 最小的 | ||
| $newMinId = $newVotes->last()->id; | ||
| } |
There was a problem hiding this comment.
The comment states this handles the first time case when $minId == 0, but there's a potential edge case. If the cached summary has min_id set to 0 but has non-zero win/lose counts, this could indicate data corruption or an invalid state. The code should either validate this scenario doesn't happen, or handle it more explicitly.
Consider adding a check: if $minId == 0 but $currentTotal > 0, this indicates an inconsistent state that should either be logged or trigger a rebuild from scratch.
1 participant
…toryBuilder