Implement tournament overview tab #121
Conversation
WalkthroughThe pull request introduces significant enhancements to the tournament-related data structures within the application. Key modifications include the addition of the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (4)
style/lib/extensions/date_extensions.dart (1)
1-1: Verify the necessity of the Material import.The import statement
import 'package:flutter/material.dart';is present, but it's not immediately clear if it's necessary for this file. TheDateUtilsclass is part of the material library, so this import is required for theisSameDaymethod. However, for better code organization, consider importing only the specific parts needed.You could replace the current import with a more specific one:
import 'package:flutter/src/material/date.dart' show DateUtils;This would make the dependencies of this file more explicit and potentially improve compile times.
khelo/lib/domain/formatter/date_formatter.dart (1)
46-62: LGTM: Well-implemented relative time formatting.The
relativeTimemethod provides a user-friendly representation of dates. It correctly handles cases for today, tomorrow, and other dates within the same year or different years.Consider adding localization support for the strings "Today" and "Tomorrow" to make the method more internationalization-friendly. You could use the
contextparameter to access localized strings:day = context.l10n.today; // and day = context.l10n.tomorrow;data/lib/service/ball_score/ball_score_service.dart (1)
186-205: LGTM! Consider minor improvements.The implementation of
getPlayerTotalRunsis well-structured and aligns with the class's error handling pattern. Good job on using theFilter.andfor efficient querying and the fold operation for summing runs.Consider the following minor improvements:
Add a comment explaining the purpose of the
runsScored > 0check in the fold operation. For example:// Only sum positive run values to exclude any potential negative adjustments return runsScored > 0 ? total + runsScored : total;Add input validation for
matchIdandplayerIdat the beginning of the method:if (matchId.isEmpty || playerId.isEmpty) { throw ArgumentError('matchId and playerId must not be empty'); }These changes will improve code readability and robustness.
khelo/lib/ui/flow/tournament/detail/tournament_detail_screen.dart (1)
114-117: LGTM: Addition of TournamentDetailOverviewTabThe changes here effectively implement the new overview tab, which aligns with the PR objective. The
constkeyword removal is correct as the list now contains a non-constant widget.Suggestion for improvement:
Consider using a list comprehension or a separate method to generate the tab widgets. This approach would make it easier to add or modify tabs in the future.For example:
children: [ for (final tab in _tabs) tab.buildWidget(tournament: state.tournament!), ],Where
_tabscould be a list of custom objects that know how to build their respective widgets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
- data/lib/api/tournament/tournament_model.dart (3 hunks)
- data/lib/api/tournament/tournament_model.freezed.dart (14 hunks)
- data/lib/api/tournament/tournament_model.g.dart (1 hunks)
- data/lib/service/ball_score/ball_score_service.dart (1 hunks)
- data/lib/service/match/match_service.dart (1 hunks)
- data/lib/service/tournament/tournament_service.dart (5 hunks)
- khelo/assets/locales/app_en.arb (2 hunks)
- khelo/lib/components/won_by_message_text.dart (2 hunks)
- khelo/lib/domain/formatter/date_formatter.dart (2 hunks)
- khelo/lib/ui/flow/home/home_screen.dart (1 hunks)
- khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart (1 hunks)
- khelo/lib/ui/flow/tournament/detail/tournament_detail_screen.dart (3 hunks)
- khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (1 hunks)
- khelo/lib/ui/flow/tournament/tournament_list_screen.dart (1 hunks)
- style/lib/extensions/date_extensions.dart (1 hunks)
🧰 Additional context used
🔇 Additional comments (32)
style/lib/extensions/date_extensions.dart (1)
6-6: New methodisSameDaylooks good.The new
isSameDaymethod is a valuable addition to theDateTimeExtensions. It provides a convenient way to compare dates while ignoring the time component. The use ofDateUtils.isSameDayfrom the Flutter framework ensures consistency with Flutter's date handling.A few observations:
- The method correctly handles null input by using the nullable
DateTime?type.- It uses
thisto refer to the current DateTime instance, which is correct in this context.One minor suggestion for improvement:
Consider adding a brief documentation comment to explain the method's purpose and behavior, especially regarding null handling. For example:/// Returns true if this date is the same day as the given [date]. /// If [date] is null, returns false. bool isSameDay(DateTime? date) => DateUtils.isSameDay(this, date);This would enhance the API's clarity for other developers using this extension.
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (2)
Line range hint
54-58: Excellent addition of thedisposemethod!The implementation of the
disposemethod is a great improvement to the class. It properly cancels the_tournamentSubscriptionand callssuper.dispose(), which helps prevent memory leaks and ensures proper cleanup of resources. This is a best practice when working with subscriptions in Dart and Flutter.
Line range hint
1-73: Overall, this is a well-implemented and focused change.The addition of the
disposemethod to theTournamentDetailStateViewNotifierclass is a targeted improvement that enhances resource management without affecting the existing functionality. This change follows best practices for handling subscriptions in Dart and Flutter applications.khelo/lib/domain/formatter/date_formatter.dart (1)
4-4: LGTM: New import added for date extensions.The addition of
style/extensions/date_extensions.dartimport is appropriate, likely providing additional date-related utilities for the new functionality.data/lib/api/tournament/tournament_model.g.dart (3)
Line range hint
1-113: Caution: Generated file modificationsThis file is generated code, as indicated by the comment at the top:
// GENERATED CODE - DO NOT MODIFY BY HANDWhile the additions for
PlayerKeyStatImplseem to be correctly integrated, it's important to note that manually editing generated files is generally discouraged. Instead, modifications should be made to the source files that generate this code.To maintain consistency and prevent unintended side effects:
- Ensure that the
PlayerKeyStatclass is properly defined in the sourcetournament_model.dartfile.- Use appropriate annotations (like
@JsonSerializable) in the source file to generate the desired serialization methods.- Re-run the code generation tool (likely
build_runner) to update this file automatically.This approach will ensure that the generated code remains consistent with the source definitions and reduces the risk of introducing errors through manual edits.
To confirm the existence of the source file and its annotations, run:
rg "@JsonSerializable" data/lib/api/tournament/tournament_model.dartThis will help verify that the
PlayerKeyStatclass is properly annotated in the source file.
108-113: LGTM with a minor suggestion for robustnessThe
_$$PlayerKeyStatImplToJsonmethod looks good and follows the standard pattern for JSON serialization in Dart.For added robustness, consider handling potential null values, even if they're not expected:
Map<String, dynamic> _$$PlayerKeyStatImplToJson(_$PlayerKeyStatImpl instance) => <String, dynamic>{ 'teamName': instance.teamName ?? '', 'player': instance.player?.toJson() ?? {}, 'runs': instance.runs ?? 0, };This change ensures that the serialization won't throw errors even if some fields are unexpectedly null.
To ensure that the
UserModelclass has the necessarytoJsonmethod, run the following command:rg "class UserModel" -A 20This will help verify the existence of the
toJsonmethod in theUserModelclass.
101-106:⚠️ Potential issueConsider adding null checks and handling potential data loss
The
_$$PlayerKeyStatImplFromJsonmethod looks good overall, but there are a couple of points to consider:
The method doesn't perform null checks on the required fields ('teamName', 'player', 'runs'). This could lead to runtime errors if the JSON is malformed or missing these fields.
The 'runs' field is converted from
numtointusingtoInt(). This might cause data loss if the original value is a floating-point number.Consider the following improvements:
- Add null checks for required fields:
_$PlayerKeyStatImpl _$$PlayerKeyStatImplFromJson(Map<String, dynamic> json) => _$PlayerKeyStatImpl( teamName: json['teamName'] as String? ?? '', player: json['player'] != null ? UserModel.fromJson(json['player'] as Map<String, dynamic>) : UserModel.empty(), // Assuming there's an empty constructor or a default user runs: json['runs'] != null ? (json['runs'] as num).toInt() : 0, );
- If fractional runs are meaningful in your application, consider keeping 'runs' as a
numinstead of converting toint, or useround()instead oftoInt()to handle floating-point values more accurately.To ensure that the
UserModelclass exists and has the necessaryfromJsonmethod, run the following command:This will help verify the existence and structure of the
UserModelclass.khelo/lib/ui/flow/tournament/tournament_list_screen.dart (1)
206-211:⚠️ Potential issueCaution: Potential null safety issue in date formatting
The removal of the null check for
tournament.end_dateand the use of the null assertion operator (!) could lead to runtime errors ifend_dateis ever null. While this change simplifies the code, it assumes thatend_dateis always non-null, which may not be guaranteed.Consider using a null-safe approach to maintain robustness:
Text.rich(TextSpan( text: DateFormatter.formatDateRange( context, startDate: tournament.start_date, endDate: tournament.end_date ?? tournament.start_date, formatType: DateFormatType.dayMonth, ), // ... rest of the code ))This approach uses the null-coalescing operator (
??) to provide a fallback value ifend_dateis null, preventing potential runtime errors while maintaining the simplified structure.To ensure this change is safe, we should verify that
end_dateis always non-null in theTournamentModel. Run the following script to check for any nullableend_datedeclarations or usages:If these searches return results, it indicates that
end_datecan be null, and we should maintain null-safety in this method.✅ Verification successful
Null safety for
end_dateconfirmedThe
end_datefield inTournamentModelis non-nullable. The removal of the null check and the use of the null assertion operator (!) inDateFormatter.formatDateRangeare safe and won't lead to runtime errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TournamentModel definition and check end_date nullability ast-grep --lang dart --pattern 'class TournamentModel { $$$ DateTime? end_date; $$$ }' # Search for any nullable usages of end_date in the codebase rg --type dart 'tournament\.end_date\?'Length of output: 136
khelo/lib/ui/flow/tournament/detail/tournament_detail_screen.dart (3)
11-11: LGTM: New import for TournamentDetailOverviewTabThe addition of this import aligns with the PR objective of implementing a tournament overview tab. It's a good practice to keep related components in separate files for better code organization.
100-100: Please clarify the reason for increasing the header sizeThe size of the
SliverPersistentHeaderhas been increased from 60 to 70. While this change might be necessary to accommodate the new tab or improve the layout, it's not immediately clear why this adjustment was made. Could you please provide some context for this change?To ensure this change doesn't negatively impact the UI, please verify:
- The tab selection area is fully visible and not cut off.
- The increased size doesn't push important content too far down the screen.
- The change is consistent across different device sizes.
Line range hint
124-137: Address incomplete tab implementationWhile the overview tab has been successfully added, I noticed that the UI still shows options for other tabs (Teams, Matches, Points Table, Stats) that haven't been implemented yet. This could lead to a confusing user experience if users can select these tabs but see no content.
Suggestions:
- If these tabs are planned for future implementation, consider adding placeholder widgets or "Coming Soon" messages for now.
- Alternatively, if these tabs are out of scope for this PR, consider removing them from the UI temporarily.
- Update the
_tabSelectionmethod to only show the implemented tabs.Please clarify the intended approach for handling these additional tabs.
To ensure a good user experience, please verify:
- The behavior when selecting unimplemented tabs.
- The visual consistency across all tab states (selected, unselected, implemented, unimplemented).
khelo/assets/locales/app_en.arb (8)
62-62: LGTM: Improved reusability of "View all" stringThe change from "home_screen_view_all_btn" to "common_view_all" improves the reusability of this string across the app. This aligns well with localization best practices.
186-186: LGTM: Clear and consistent tournament info titleThe new entry "tournament_detail_overview_info_title" is well-named and follows the established convention for tournament detail strings. The value is properly capitalized and consistent with other title entries.
187-187: LGTM: Concise and clear tournament type titleThe new entry "tournament_detail_overview_type_title" is appropriately named and follows the established convention. The value "Type" is concise and clear, maintaining consistency with other title entries.
188-188: LGTM: Clear tournament duration titleThe new entry "tournament_detail_overview_duration_title" is well-named and consistent with the established convention. The value "Duration" is clear and appropriate for displaying tournament length information.
190-190: LGTM: Clear and consistent key stats titleThe new entry "tournament_detail_overview_key_stats_title" is well-named and follows the established convention. The value "Key Stats" is clear, properly capitalized, and consistent with other title entries.
191-191: LGTM: Clear and consistent most runs titleThe new entry "tournament_detail_overview_key_stats_most_runs_title" is appropriately named and follows the established convention. The value "Most Runs" is clear, properly capitalized, and consistent with other title entries.
192-192: LGTM: Clear and consistent featured matches titleThe new entry "tournament_detail_overview_featured_matches_title" is well-named and follows the established convention. The value "Featured Matches" is clear, properly capitalized, and consistent with other title entries.
Line range hint
62-193: Overall: Good additions for tournament overview, with one minor issueThe new entries for the tournament overview feature are generally well-structured and consistent. They follow the established naming conventions and provide clear, properly capitalized values. The change from a specific "home_screen_view_all_btn" to a more generic "common_view_all" improves reusability.
However, there is one grammatical issue in the "teams squads" entry that should be addressed.
To ensure these new localization strings are being used correctly in the implementation, please run the following script:
This script will help verify that all new localization keys are being used in the codebase and that the old "home_screen_view_all_btn" key has been fully replaced.
khelo/lib/components/won_by_message_text.dart (1)
12-18: LGTM: Addition of 'isTournament' PropertyThe addition of the
isTournamentboolean property with a default value offalseallows the widget to conditionally render content based on the tournament status while maintaining backward compatibility.data/lib/api/tournament/tournament_model.dart (3)
37-39: Confirm exclusion ofkeyStatsfrom JSON serializationThe
keyStatsfield is annotated with@JsonKey(includeFromJson: false, includeToJson: false), which means it will not be included during JSON serialization or deserialization. IfkeyStatsneeds to be stored in or retrieved from Firestore or any JSON operations, consider removing these annotations. Ensure that this exclusion is intentional and aligns with your data handling requirements.
58-59: Verify inclusion ofuserfield in JSON serialization is intendedThe
@JsonKey(includeToJson: false, includeFromJson: false)annotation has been removed from theuserfield inTournamentMember. This change means theuserfield will now be included during JSON serialization and deserialization. Confirm that this inclusion is intentional and that it won't introduce issues such as data redundancy or circular references in your JSON data.
93-101:PlayerKeyStatclass is correctly definedThe new
PlayerKeyStatclass is properly defined with the necessary fields and annotations. The use of@freezedand@JsonSerializable(explicitToJson: true)ensures immutable data models and correct JSON serialization.data/lib/service/tournament/tournament_service.dart (3)
19-19: BallScoreService provider correctly injectedThe
BallScoreServiceis properly injected into theTournamentServiceprovider, ensuring that the service is available for use within theTournamentService.
28-28: BallScoreService dependency added to TournamentServiceAdding
_ballScoreServiceas a final field and including it in the constructor parameters aligns with the existing dependency injection pattern. This ensures thatBallScoreServiceis accessible throughout theTournamentService.Also applies to: 35-35
100-102: Key statistics integrated into tournament dataThe inclusion of
keyStatsin the tournament object by callinggetKeyStats(matches)enhances the tournament data with valuable player statistics. This is correctly implemented and will improve the functionality of thestreamTournamentByIdmethod.data/lib/api/tournament/tournament_model.freezed.dart (6)
321-329: Initialization of '_keyStats' field is correctThe private field
_keyStatsis properly initialized in the constructor, and the corresponding public getterkeyStatsprovides read-only access. This ensures that theTournamentModelremains immutable.
400-408: Ensured immutability of 'keyStats' listWrapping
_keyStatswithEqualUnmodifiableListViewin the getter ensures that the returned list cannot be modified, preserving the immutability of theTournamentModelinstance.
77-79: Update 'copyWith' method usage to include 'keyStats'The
copyWithmethod now includes thekeyStatsparameter. Ensure that all calls tocopyWithonTournamentModelare updated to handlekeyStatsappropriately to prevent any unexpected behavior.You can verify all usages of
copyWithonTournamentModelin the codebase with the following script:#!/bin/bash # Description: Find all usages of 'TournamentModel.copyWith' and check for 'keyStats' parameter. # Test: Search for 'copyWith' method calls on 'TournamentModel'. rg --type dart 'TournamentModel' -A 5 | rg '\.copyWith\(' -C 3
170-173: Ensure correct null handling for 'keyStats' in 'copyWith'In the
copyWithimplementation,keyStatsis nullable. Verify that this nullability aligns with howkeyStatsis used elsewhere and that it doesn't introduce any null-safety issues.To check for potential nullability issues, run:
#!/bin/bash # Description: Search for usages of 'keyStats' where null handling is critical. # Test: Find places where 'keyStats' is accessed and ensure null checks are in place. rg --type dart 'keyStats' -A 5 | rg -v '@JsonKey' -B 2
764-944:⚠️ Potential issueAdd JSON serialization for 'PlayerKeyStat' class
The
PlayerKeyStatclass is defined with propertiesteamName,player, andruns. However, it lacks JSON serialization annotations. If instances ofPlayerKeyStatneed to be serialized or deserialized, consider adding@JsonSerializableand implementingtoJsonandfromJsonmethods.To verify if
PlayerKeyStathas the necessary JSON serialization methods, run:#!/bin/bash # Description: Check for JSON serialization annotations and methods in 'PlayerKeyStat'. # Test: Search for '@JsonSerializable' annotation on 'PlayerKeyStat' class. rg --type dart '@JsonSerializable' -A 3 | rg 'class PlayerKeyStat' # Check for 'toJson' and 'fromJson' methods in 'PlayerKeyStat'. rg --type dart 'class PlayerKeyStat' -A 20 | rg 'toJson|fromJson'
42-43:⚠️ Potential issueEnsure 'keyStats' field is serialized if needed
The
keyStatsfield is annotated with@JsonKey(includeFromJson: false, includeToJson: false), meaning it will not be included during JSON serialization or deserialization. IfkeyStatsneeds to be persisted or transmitted (e.g., via APIs), consider removing these annotations to include it in JSON operations.To verify where
keyStatsis being used and ensure it aligns with your serialization logic, run the following script:
| Future<List<PlayerKeyStat>> getKeyStats(List<MatchModel> matches) async { | ||
| final Map<String, PlayerKeyStat> playerStatsMap = {}; | ||
|
|
||
| for (var match in matches) { | ||
| for (var team in match.teams) { | ||
| for (var player in team.squad) { | ||
| final totalRuns = | ||
| await _ballScoreService.getPlayerTotalRuns(match.id, player.id); | ||
|
|
||
| if (totalRuns > 0) { | ||
| if (playerStatsMap.containsKey(player.id)) { | ||
| playerStatsMap[player.id] = playerStatsMap[player.id]!.copyWith( | ||
| runs: playerStatsMap[player.id]!.runs + totalRuns, | ||
| ); | ||
| } else { | ||
| playerStatsMap[player.id] = PlayerKeyStat( | ||
| teamName: team.team.name, | ||
| player: player.player, | ||
| runs: totalRuns, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return playerStatsMap.values.toList() | ||
| ..sort((a, b) => b.runs.compareTo(a.runs)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize asynchronous operations in getKeyStats method
The getKeyStats method performs await calls inside nested loops, causing the asynchronous operations to execute sequentially. This can lead to performance bottlenecks, especially with a large number of matches, teams, and players. Consider refactoring the code to execute these asynchronous calls concurrently.
Here is a suggested refactor to improve performance:
Future<List<PlayerKeyStat>> getKeyStats(List<MatchModel> matches) async {
final Map<String, PlayerKeyStat> playerStatsMap = {};
final List<Future<void>> futures = [];
for (var match in matches) {
for (var team in match.teams) {
for (var player in team.squad) {
futures.add(() async {
final totalRuns = await _ballScoreService.getPlayerTotalRuns(match.id, player.id);
if (totalRuns > 0) {
if (playerStatsMap.containsKey(player.id)) {
playerStatsMap[player.id] = playerStatsMap[player.id]!.copyWith(
runs: playerStatsMap[player.id]!.runs + totalRuns,
);
} else {
playerStatsMap[player.id] = PlayerKeyStat(
teamName: team.team.name,
player: player.player,
runs: totalRuns,
);
}
}
}());
}
}
}
// Await all asynchronous operations concurrently
await Future.wait(futures);
return playerStatsMap.values.toList()
..sort((a, b) => b.runs.compareTo(a.runs));
}This refactor collects all the asynchronous operations into a list of futures and uses Future.wait to execute them concurrently, significantly improving performance when processing large datasets.
| @@ -111,4 +118,34 @@ class TournamentService { | |||
| } | |||
| }).handleError((error, stack) => throw AppError.fromError(error, stack)); | |||
| } | |||
|
|
|||
| Future<List<PlayerKeyStat>> getKeyStats(List<MatchModel> matches) async { | |||
There was a problem hiding this comment.
I think this KeyStats has been mistaken by you, it's not about showing highest run done by each player, participated in tournament but we'll show highest run done in the tournament, similarly other stats like, highest wickets taken by a player, let's look into this area more.
khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart
Show resolved
Hide resolved
overview_tab.webm
Summary by CodeRabbit
Release Notes
New Features
PlayerKeyStatclass for tracking player statistics in tournaments.getPlayerTotalRunsmethod to calculate total runs for players in matches.TournamentDetailOverviewTabwidget.Improvements
Localization
UI Changes
WonByMessageTextcomponent based on tournament context.