Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
score-ios/ViewModels/GamesViewModel.swift (1)
322-324: Add a deterministic tie-break for equal upcoming dates.If two sports share the same next game date, current ordering can vary between refreshes. Add a secondary sort key for stable UI ordering.
Proposed refinement
let sortedWithGames = withGames.sorted { s1, s2 in - (nextGameBySport[s1] ?? Date.distantFuture) < (nextGameBySport[s2] ?? Date.distantFuture) + let d1 = nextGameBySport[s1] ?? Date.distantFuture + let d2 = nextGameBySport[s2] ?? Date.distantFuture + return d1 == d2 ? s1.rawValue < s2.rawValue : d1 < d2 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@score-ios/ViewModels/GamesViewModel.swift` around lines 322 - 324, The sort for sortedWithGames is non-deterministic when nextGameBySport dates are equal; modify the comparator used where sortedWithGames is created (the sorted { s1, s2 in ... } closure that references nextGameBySport and withGames) to add a stable secondary key—for example compare s1 and s2 by a deterministic property such as sport identifier or name (e.g., sport.id or sport.name) when the two Date values are equal—so ties on (nextGameBySport[s1] ?? Date.distantFuture) result in a consistent ordering across refreshes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@score-ios/ViewModels/GamesViewModel.swift`:
- Around line 304-327: orderSportsByUpcoming currently ignores the passed-in
sports array and uses Sport.allCases; change it to use the function parameter
instead: compute allOtherSports from the provided sports (e.g., filter out .All
from the sports parameter) so you only sort the caller-requested items, and only
include .All in the returned array if the input contained it. Update the
downstream logic that builds withGames, withoutGames, nextGameBySport,
sortedWithGames, and sortedWithoutGames to operate on that filtered sports list
so the result respects the input order/selection.
---
Nitpick comments:
In `@score-ios/ViewModels/GamesViewModel.swift`:
- Around line 322-324: The sort for sortedWithGames is non-deterministic when
nextGameBySport dates are equal; modify the comparator used where
sortedWithGames is created (the sorted { s1, s2 in ... } closure that references
nextGameBySport and withGames) to add a stable secondary key—for example compare
s1 and s2 by a deterministic property such as sport identifier or name (e.g.,
sport.id or sport.name) when the two Date values are equal—so ties on
(nextGameBySport[s1] ?? Date.distantFuture) result in a consistent ordering
across refreshes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24bf2687-698f-4b20-b81f-838efe857810
📒 Files selected for processing (2)
score-ios/ViewModels/GamesViewModel.swiftscore-ios/Views/ListViews/SportSelectorView.swift
Comment on lines
+304
to
+327
| func orderSportsByUpcoming(sports: [Sport]) -> [Sport] { | ||
| let allOtherSports = Sport.allCases.filter { $0 != .All } | ||
| let (withGames, withoutGames) = allOtherSports.reduce(into: ([Sport](), [Sport]())) { acc, sport in | ||
| if allUpcomingGames.contains(where: { $0.sport == sport }) { | ||
| acc.0.append(sport) | ||
| } else { | ||
| acc.1.append(sport) | ||
| } | ||
| } | ||
| let nextGameBySport: [Sport: Date] = Dictionary(uniqueKeysWithValues: | ||
| withGames.map { sport in | ||
| let nextDate = allUpcomingGames | ||
| .filter { $0.sport == sport } | ||
| .map { $0.date } | ||
| .min() ?? Date.distantFuture | ||
| return (sport, nextDate) | ||
| } | ||
| ) | ||
| let sortedWithGames = withGames.sorted { s1, s2 in | ||
| (nextGameBySport[s1] ?? Date.distantFuture) < (nextGameBySport[s2] ?? Date.distantFuture) | ||
| } | ||
| let sortedWithoutGames = withoutGames.sorted { $0.rawValue < $1.rawValue } | ||
| return [.All] + sortedWithGames + sortedWithoutGames | ||
| } |
There was a problem hiding this comment.
Use the sports argument; it is currently ignored.
The function signature accepts sports, but Line 305 derives from Sport.allCases. This can return items the caller did not request (and always forces .All in the result).
Proposed fix
func orderSportsByUpcoming(sports: [Sport]) -> [Sport] {
- let allOtherSports = Sport.allCases.filter { $0 != .All }
+ let allOtherSports = sports.filter { $0 != .All }
@@
- return [.All] + sortedWithGames + sortedWithoutGames
+ return (sports.contains(.All) ? [.All] : []) + sortedWithGames + sortedWithoutGames
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@score-ios/ViewModels/GamesViewModel.swift` around lines 304 - 327,
orderSportsByUpcoming currently ignores the passed-in sports array and uses
Sport.allCases; change it to use the function parameter instead: compute
allOtherSports from the provided sports (e.g., filter out .All from the sports
parameter) so you only sort the caller-requested items, and only include .All in
the returned array if the input contained it. Update the downstream logic that
builds withGames, withoutGames, nextGameBySport, sortedWithGames, and
sortedWithoutGames to operate on that filtered sports list so the result
respects the input order/selection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit