Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove group option conditionally #138

Merged
merged 4 commits into from
Nov 19, 2024

Conversation

cp-sidhdhi-p
Copy link
Collaborator

@cp-sidhdhi-p cp-sidhdhi-p commented Nov 18, 2024

Changes:

  • Tournament start date validation to not let it be selected greater than end date.
  • Remove Group Option Conditionally.
    • do not let add quarterfinal or semifinal in case of double out.
    • do not let add more than two rounds in double out etc.
  • Fix error on banner image change and image not updating in Home screen.
  • fix date selection in profile screen
  • resolve overflow error in tournament detail overview
  • fix stats matchId exceeds in where in query

dateValidation
Simulator Screen Recording - iPhone 16 Pro - 2024-11-18 at 10 45 44

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced tournament creation with date selection functionality, including improved error handling for date conflicts.
    • Added optional startDate and endDate parameters to the date picker for better user experience.
    • Introduced a new entry for "Match tied" in localization for clearer messaging.
  • Improvements

    • Improved image loading and state management in tournament-related widgets, allowing for better handling of null image URLs.
    • Streamlined match selection logic for clearer UI component structure.
    • Enhanced responsiveness and layout of tournament detail views.
    • Improved date selection logic in the user profile to restrict dates within a specified range.
  • Bug Fixes

    • Adjusted date validation logic to ensure consistent start and end dates during tournament creation.
  • Refactor

    • Simplified match group option handling and button visibility logic for better maintainability.

Copy link

coderabbitai bot commented Nov 18, 2024

Walkthrough

The changes in this pull request involve several modifications across various components related to tournament management. Key updates include making the imageProvider variable nullable in the TournamentItem and FlexibleSpace widgets, enhancing date selection functionality in the AddTournamentScreen, and refining date management in the AddTournamentViewNotifier. Additionally, new methods for handling match group options were introduced in MatchSelectionViewNotifier and related components, improving the clarity and maintainability of the code.

Changes

File Path Change Summary
khelo/lib/ui/flow/home/components/tournament_item.dart Changed imageProvider from non-nullable to nullable. Introduced _initializeImageProvider method for image loading logic. Added didUpdateWidget to handle updates to tournament prop. Updated palette generation logic.
khelo/lib/ui/flow/tournament/add/add_tournament_screen.dart Updated _dateScheduleView method to include startDate parameter for date picker initialization. Enhanced _observeDateError to show error snackbar for date selection issues.
khelo/lib/ui/flow/tournament/add/add_tournament_view_model.dart Modified onStartDate and onEndDate methods to ensure logical consistency between start and end dates during tournament creation.
khelo/lib/ui/flow/tournament/detail/components/flexible_space.dart Changed imageProvider from non-nullable to nullable. Encapsulated initialization logic in _initializeImageProvider, updating palette generation based on image URL.
khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart Reformatted scheduleMatchesByType method for improved readability without changing logic or functionality.
khelo/lib/ui/flow/tournament/match_selection/match_selection_screen.dart Refactored match group option handling into notifier.getMatchGroupOption(). Introduced showAddButton variable for conditional rendering of the add button.
khelo/lib/ui/flow/tournament/match_selection/match_selection_view_model.dart Added getMatchGroupOption method to generate available match group options based on tournament state.
style/lib/pickers/date_and_time_picker.dart Introduced optional startDate and endDate parameters in selectDate function. Modified logic for determining firstDate based on the new parameter.
data/lib/service/ball_score/ball_score_service.dart Improved getBallScoresByMatchIds method with batching for Firestore queries. Updated import statements for better organization.
khelo/assets/locales/app_en.arb Added new localization entry for "Match tied" with key "score_board_match_tied" and value "Match tied".
khelo/lib/components/won_by_message_text.dart Updated WonByMessageText to conditionally display tied match text based on isTournament flag.
khelo/lib/ui/flow/settings/edit_profile/edit_profile_screen.dart Enhanced date selection in _userPersonalDetailsView to restrict date range between January 1, 1950, and current date.
khelo/lib/ui/flow/tournament/detail/components/filter_tab_view.dart Wrapped Text widget in Expanded for better layout behavior.
khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart Restructured widget layouts using Expanded and Visibility for improved responsiveness and alignment in the UI.

Possibly related PRs

🐇 In the land of tournaments, changes abound,
With images and dates, new logic is found.
A nullable provider, a picker to choose,
Match groups now clearer, no options to lose!
So hop along, friends, let’s celebrate this feat,
For tournaments now dance to a more rhythmic beat! 🎉


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ccc2aba and 01f0c4c.

📒 Files selected for processing (1)
  • khelo/lib/components/won_by_message_text.dart (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • khelo/lib/components/won_by_message_text.dart

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (10)
style/lib/pickers/date_and_time_picker.dart (1)

9-9: LGTM! Consider adding parameter documentation.

The addition of the nullable startDate parameter is well-placed and aligns with the date validation requirements.

Add documentation to clarify the parameter's purpose:

Future<void> selectDate(
  BuildContext context, {
  String? helpText,
+ /// The minimum selectable date. If null, defaults to either initialDate or current date
  DateTime? startDate,
  required DateTime initialDate,
  required Function(DateTime) onDateSelected,
})
khelo/lib/ui/flow/tournament/detail/components/flexible_space.dart (1)

47-57: Add error handling and ensure consistent state management

While the implementation is generally good, consider these improvements:

  1. Add error handling for palette generation
  2. Ensure consistent mounted checks for all setState calls

Consider this improved implementation:

 void _initializeImageProvider(String? imageUrl) {
   if (imageUrl != null) {
     imageProvider = CachedNetworkImageProvider(imageUrl);
-    imageProvider!.createPaletteGenerator().then((generatedPalette) {
-      if (mounted) {
-        setState(() => palette = generatedPalette);
-      }
+    imageProvider!.createPaletteGenerator().then((generatedPalette) {
+      if (mounted) {
+        setState(() => palette = generatedPalette);
+      }
+    }).catchError((error) {
+      // Handle error gracefully, maybe set a default palette
+      if (mounted) {
+        setState(() => palette = null);
+      }
     });
   } else {
-    setState(() => imageProvider = null);
+    if (mounted) {
+      setState(() {
+        imageProvider = null;
+        palette = null;
+      });
+    }
   }
 }
khelo/lib/ui/flow/home/components/tournament_item.dart (1)

117-122: Simplify null checks and improve safety

The current implementation has redundant null checks and a forced unwrap that could be simplified.

Consider this more concise and safer approach:

-        image: bannerUrl == null || imageProvider == null
+        image: imageProvider == null
             ? null
             : DecorationImage(
-                image: imageProvider!,
+                image: imageProvider,
                 fit: BoxFit.fill,
               ),

The imageProvider null check alone is sufficient since it can only be non-null when bannerUrl is also non-null, based on the initialization logic.

khelo/lib/ui/flow/tournament/match_selection/match_selection_view_model.dart (2)

156-164: Simplify quarterfinal removal logic

The current implementation has redundant checks and could be simplified:

  1. The outer condition partially overlaps with inner conditions
  2. Tournament type checks are duplicated

Consider this simplified logic:

-   if (tournamentType == TournamentType.doubleOut ||
-       state.matches.containsKey(MatchGroup.quarterfinal)) {
-     if (tournamentType == TournamentType.doubleOut ||
-         tournamentType == TournamentType.miniRobin ||
-         tournamentType == TournamentType.boxLeague ||
-         tournamentType == TournamentType.knockOut) {
-       options.remove(MatchGroup.quarterfinal);
-     }
-   }
+   final shouldRemoveQuarterfinal = tournament.type == TournamentType.doubleOut ||
+                                   tournament.type == TournamentType.miniRobin ||
+                                   tournament.type == TournamentType.boxLeague ||
+                                   tournament.type == TournamentType.knockOut ||
+                                   state.matches.containsKey(MatchGroup.quarterfinal);
+   if (shouldRemoveQuarterfinal) {
+     options.remove(MatchGroup.quarterfinal);
+   }

145-183: Document business rules and improve structure

While the implementation correctly handles the match group restrictions, consider these improvements:

  1. Add documentation explaining the business rules for each tournament type
  2. Consider using a more maintainable structure for validation rules

Consider this approach:

+ /// Rules for match group availability by tournament type:
+ /// - Double Out: Maximum 2 rounds, no quarterfinals/semifinals
+ /// - Mini Robin: No rounds after first, no quarterfinals/semifinals
+ /// - Box League: No quarterfinals/semifinals
+ /// - Knock Out: No quarterfinals/semifinals
  List<MatchGroup> getMatchGroupOption() {
+   final validationRules = {
+     TournamentType.doubleOut: {
+       MatchGroup.round: (matches) => 
+         (matches[MatchGroup.round]?.entries.length ?? 0) >= 2,
+       MatchGroup.quarterfinal: (_) => true,
+       MatchGroup.semifinal: (_) => true,
+     },
+     // Add rules for other tournament types
+   };
    // Implementation using validation rules
  }
khelo/lib/ui/flow/tournament/add/add_tournament_view_model.dart (3)

115-120: LGTM! Consider extracting the duration constant.

The date validation logic is correct and aligns with the requirements. However, consider extracting the one-day duration as a named constant for better maintainability.

+  static const Duration MIN_TOURNAMENT_DURATION = Duration(days: 1);
   
   void onStartDate(DateTime startDate) {
     DateTime endDate = state.endDate;

     if (startDate.isAfter(endDate)) {
-      endDate = startDate.add(Duration(days: 1));
+      endDate = startDate.add(MIN_TOURNAMENT_DURATION);
     }
     state = state.copyWith(startDate: startDate, endDate: endDate);
     onChange();
   }

125-134: Improve readability of date validation logic.

While the logic is correct, the nested conditions could be refactored for better readability. Also, the Duration instantiation is repeated.

+  static const Duration MIN_TOURNAMENT_DURATION = Duration(days: 1);
   
   void onEndDate(DateTime endDate) {
     DateTime startDate = state.startDate;
 
     if (endDate.isBefore(startDate)) {
-      startDate = endDate.subtract(Duration(days: 1));
-      if (_editedTournament != null &&
-          startDate.isBefore(_editedTournament!.start_date)) {
-        startDate = _editedTournament!.start_date;
-      }
+      startDate = endDate.subtract(MIN_TOURNAMENT_DURATION);
+      
+      // For edited tournaments, ensure start date isn't before original
+      if (_editedTournament?.start_date != null && 
+          startDate.isBefore(_editedTournament!.start_date)) {
+         startDate = _editedTournament!.start_date;
+      }
     }
     state = state.copyWith(endDate: endDate, startDate: startDate);
     onChange();
   }

115-134: Consider consolidating date validation logic.

Currently, date validation occurs in two places:

  1. During date selection (onStartDate/onEndDate)
  2. During tournament creation (addTournament)

Consider extracting the date validation logic into a separate method to maintain a single source of truth and improve maintainability.

+  bool _isValidDateRange(DateTime startDate, DateTime endDate) {
+    return !endDate.isBefore(startDate);
+  }
+
   void onStartDate(DateTime startDate) {
     DateTime endDate = state.endDate;
-    if (startDate.isAfter(endDate)) {
+    if (!_isValidDateRange(startDate, endDate)) {
       endDate = startDate.add(Duration(days: 1));
     }
     // ...
   }

   void onEndDate(DateTime endDate) {
     DateTime startDate = state.startDate;
-    if (endDate.isBefore(startDate)) {
+    if (!_isValidDateRange(startDate, endDate)) {
       startDate = endDate.subtract(Duration(days: 1));
       // ...
     }
     // ...
   }

   void addTournament() async {
     // ...
-    if (state.endDate.isBefore(state.startDate)) {
+    if (!_isValidDateRange(state.startDate, state.endDate)) {
       state = state.copyWith(showDateError: true, loading: false);
       return;
     }
     // ...
   }
khelo/lib/ui/flow/tournament/match_selection/match_selection_screen.dart (2)

Line range hint 59-71: LGTM! Consider adding error handling for empty options.

Good refactoring to move the match group option logic to the view model. This improves separation of concerns.

Consider handling the case when no options are available:

  final option = notifier.getMatchGroupOption();
+ if (option.isEmpty) {
+   context.showErrorSnackBar(context.l10n.no_available_groups_message);
+   return;
+ }
  showActionBottomSheet(

152-156: Extract magic numbers into named constants.

The logic for controlling add button visibility is clear, but the magic numbers should be constants for better maintainability.

Consider extracting the numbers into named constants:

+ const int MAX_QUARTERFINAL_MATCHES = 4;
+ const int MAX_SEMIFINAL_MATCHES = 2;
  final showAddButton =
-     (group == MatchGroup.quarterfinal && matches.length < 4) ||
-     (group == MatchGroup.semifinal && matches.length < 2) ||
+     (group == MatchGroup.quarterfinal && matches.length < MAX_QUARTERFINAL_MATCHES) ||
+     (group == MatchGroup.semifinal && matches.length < MAX_SEMIFINAL_MATCHES) ||
      (group == MatchGroup.finals && matches.isEmpty) ||
      group == MatchGroup.round;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 58721e7 and b75943a.

📒 Files selected for processing (8)
  • khelo/lib/ui/flow/home/components/tournament_item.dart (2 hunks)
  • khelo/lib/ui/flow/tournament/add/add_tournament_screen.dart (2 hunks)
  • khelo/lib/ui/flow/tournament/add/add_tournament_view_model.dart (1 hunks)
  • khelo/lib/ui/flow/tournament/detail/components/flexible_space.dart (1 hunks)
  • khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart (1 hunks)
  • khelo/lib/ui/flow/tournament/match_selection/match_selection_screen.dart (3 hunks)
  • khelo/lib/ui/flow/tournament/match_selection/match_selection_view_model.dart (1 hunks)
  • style/lib/pickers/date_and_time_picker.dart (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart
🔇 Additional comments (11)
style/lib/pickers/date_and_time_picker.dart (1)

19-19: Verify date constraint handling

The logic for firstDate looks correct, but let's ensure it properly handles all edge cases:

  1. When startDate is after initialDate
  2. When startDate is after now
  3. When startDate is after lastDate

Consider adding validation to ensure date constraints are properly enforced:

-    firstDate: startDate ?? (initialDate.isBefore(now) ? initialDate : now),
+    firstDate: [startDate, initialDate.isBefore(now) ? initialDate : now]
+        .whereType<DateTime>()
+        .reduce((a, b) => a.isBefore(b) ? a : b),

This ensures we always use the earliest valid date as firstDate.

khelo/lib/ui/flow/tournament/detail/components/flexible_space.dart (2)

28-28: LGTM: Nullable imageProvider improves error handling

The change to make imageProvider nullable is appropriate as it better handles cases where tournaments don't have banner images.


35-45: LGTM: Proper lifecycle management

The lifecycle methods correctly handle image provider initialization and updates. The comparison in didUpdateWidget ensures the image provider is only reinitialized when the banner URL actually changes.

Let's verify that banner URL changes are properly propagated:

✅ Verification successful

LGTM: Banner URL changes are properly handled

The banner URL is correctly managed across the codebase:

  • It's properly defined as a nullable String in the data model
  • Changes are handled consistently in both FlexibleSpace and TournamentItem widgets using the same pattern
  • The URL is only used for display purposes and proper null checks are in place
  • The didUpdateWidget implementation correctly reinitializes the image provider when the URL changes
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for places where banner_img_url is modified to ensure proper state updates
rg -l "banner_img_url.*=" --type dart

Length of output: 215


Script:

#!/bin/bash
# Let's examine the actual usage context of banner_img_url in these files
rg "banner_img_url" --type dart -C 3

Length of output: 17971

khelo/lib/ui/flow/home/components/tournament_item.dart (3)

34-34: LGTM! Improved null safety handling

Making imageProvider nullable is a good change that prevents potential runtime errors when banner images are not available.


59-65: LGTM! Well-implemented widget lifecycle handling

The didUpdateWidget implementation correctly handles banner URL changes and follows widget lifecycle best practices.


Line range hint 34-122: Verify banner image update fixes across the app

Since this PR aims to fix banner image update issues, let's verify the implementation across related components.

✅ Verification successful

Let me gather more context about the banner image handling in the related files to verify if similar patterns need fixing.


Based on the analysis of the code, I can now provide a final response as the verification is complete.

Banner image update implementation is correctly handled across components

The banner image update implementation in tournament_item.dart follows the same pattern as other components that handle banner images:

  • FlexibleSpace component has identical image update logic with didUpdateWidget and _initializeImageProvider
  • AddTournamentScreen handles banner updates through state management
  • The implementation is used consistently in both home screen and view all screen
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar image provider patterns that might need the same fixes
ast-grep --pattern 'ImageProvider$_'

# Look for related banner image handling
rg -l "banner.*image|image.*banner" --type dart

# Check for potential uses of TournamentItem
rg "TournamentItem\(" --type dart

Length of output: 769


Script:

#!/bin/bash
# Check banner image handling in flexible_space.dart
cat "khelo/lib/ui/flow/tournament/detail/components/flexible_space.dart"

# Check banner image handling in add_tournament_screen.dart
cat "khelo/lib/ui/flow/tournament/add/add_tournament_screen.dart"

# Check how TournamentItem is used in home screens
cat "khelo/lib/ui/flow/home/home_screen.dart"
cat "khelo/lib/ui/flow/home/view_all/home_view_all_screen.dart"

Length of output: 34541

khelo/lib/ui/flow/tournament/match_selection/match_selection_view_model.dart (1)

178-183: Verify finals handling requirements for different tournament types

The current implementation only removes finals if they already exist. Please verify:

  1. Should finals be allowed in double out format?
  2. Are there any tournament type specific restrictions for finals similar to quarterfinals and semifinals?

Let's check the codebase for any existing finals-related restrictions:

✅ Verification successful

Based on the code analysis, I can now provide a definitive response regarding the finals handling requirements:

Finals handling is correctly implemented for all tournament types

The code shows that finals handling is consistent with the tournament type requirements:

  1. For double out format:

    • The code in match_selection_view_model.dart shows that finals are handled independently of tournament type, including double out format
    • This is correct because finals are a valid match group for all tournament types including double out
  2. Tournament type specific restrictions:

    • Unlike quarterfinals and semifinals which have specific restrictions for tournament types (as seen in lines 158-177 of match_selection_view_model.dart)
    • Finals are intentionally not restricted by tournament type since they represent the ultimate match(es) in any tournament format
    • The only restriction is preventing duplicate finals groups (line 178-183)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for finals-related logic in tournament types
rg -A 5 "MatchGroup.finals.*TournamentType" 

# Search for any finals validation logic
ast-grep --pattern 'if ($_ == MatchGroup.finals) { $$$ }'

Length of output: 105


Script:

#!/bin/bash
# Search for finals-related logic in a broader context
rg -A 5 "MatchGroup.finals"

# Search for tournament type validations
rg -A 10 "TournamentType.*match.*group"

# Search for double out format related code
rg "double.*out.*format" -A 5

Length of output: 5662


Script:

#!/bin/bash
# Search for match group enum definition
rg -A 10 "enum MatchGroup"

# Search for tournament type and format related validations
rg -A 10 "TournamentType|TournamentFormat"

# Search for match selection related files
fd "match.*selection.*\.dart$" --exec cat {}

Length of output: 76854

khelo/lib/ui/flow/tournament/match_selection/match_selection_screen.dart (2)

174-181: LGTM! Clean implementation of the add match button.

The conditional rendering and navigation implementation is clean and passes all necessary parameters correctly.


59-59: Verify double out format restrictions in view model.

The PR objectives mention preventing quarterfinal/semifinal rounds in "double out" format, but this constraint should be implemented in getMatchGroupOption().

Let's verify the implementation:

✅ Verification successful

Let me generate additional scripts to verify the implementation of getMatchGroupOption() which wasn't found in the previous search:


Double out format restrictions are correctly implemented in view model

The implementation in getMatchGroupOption() properly handles the double out format restrictions by:

  • Removing quarterfinal and semifinal options when tournament type is double out
  • Limiting rounds to maximum of 2 for double out format (prevents additional rounds when one already exists)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if getMatchGroupOption properly handles double out format restrictions

# Search for getMatchGroupOption implementation
ast-grep --pattern 'getMatchGroupOption() {
  $$$
}'

# Search for double out format related code
rg -i "double.?out" -A 5

Length of output: 7768


Script:

#!/bin/bash
# Search for getMatchGroupOption implementation in match_selection_view_model.dart
cat khelo/lib/ui/flow/tournament/match_selection/match_selection_view_model.dart

Length of output: 6756

khelo/lib/ui/flow/tournament/add/add_tournament_screen.dart (2)

Line range hint 52-60: LGTM: Well-implemented error handling

The error handling for date validation is well implemented with:

  • Proper error observation pattern
  • User-friendly error message using localization
  • Clean integration with the UI

Line range hint 19-19: Verify date picker component compatibility

Please ensure that the selectDate function in style/lib/pickers/date_and_time_picker.dart supports the new startDate parameter.

✅ Verification successful

Date picker component supports startDate parameter

The selectDate function in style/lib/pickers/date_and_time_picker.dart already includes support for the startDate parameter in its signature:

Future<void> selectDate(
  BuildContext context, {
  String? helpText,
  DateTime? startDate,  // Parameter exists
  required DateTime initialDate,
  required Function(DateTime) onDateSelected,
})
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the date picker component supports the startDate parameter

# Search for the selectDate function definition
rg -A 10 "selectDate.*\(" "style/lib/pickers/date_and_time_picker.dart"

Length of output: 326

@cp-sidhdhi-p cp-sidhdhi-p marked this pull request as draft November 18, 2024 06:17
@canopas canopas deleted a comment from coderabbitai bot Nov 18, 2024
@canopas canopas deleted a comment from coderabbitai bot Nov 18, 2024
@canopas canopas deleted a comment from coderabbitai bot Nov 18, 2024
@canopas canopas deleted a comment from coderabbitai bot Nov 18, 2024
@cp-sidhdhi-p cp-sidhdhi-p marked this pull request as ready for review November 18, 2024 10:19
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (5)
data/lib/service/ball_score/ball_score_service.dart (1)

110-118: Good implementation of batching, but needs documentation and constants.

The chunking implementation effectively solves the Firestore query limitation issue. However, a few improvements would make this more maintainable:

Consider these enhancements:

  1. Add a constant for the chunk size:
+ /// Maximum number of items allowed in a Firestore whereIn clause
+ static const int _FIRESTORE_WHERE_IN_LIMIT = 30;

  Future<List<BallScoreModel>> getBallScoresByMatchIds(
    List<String> matchIds,
  ) async {
    try {
      if (matchIds.isEmpty) return [];
      final List<BallScoreModel> data = [];
-     for (var ids in matchIds.chunked(30)) {
+     for (var ids in matchIds.chunked(_FIRESTORE_WHERE_IN_LIMIT)) {
  1. Add documentation explaining the batching behavior:
+ /// Retrieves ball scores for the given match IDs.
+ /// 
+ /// The method processes match IDs in batches to comply with Firestore's 
+ /// limitation on the number of items in a whereIn clause.
+ /// Returns an empty list if no match IDs are provided.
  Future<List<BallScoreModel>> getBallScoresByMatchIds(

For future enhancement: Consider implementing pagination if this method needs to handle very large datasets, as loading all scores at once could impact performance.

khelo/lib/ui/flow/tournament/match_selection/match_selection_view_model.dart (2)

145-154: Improve readability and add documentation.

The method could benefit from the following improvements:

  1. Add documentation to explain the purpose and return value
  2. Simplify the nested condition for doubleOut tournament type
  3. Use null-safe operator for better readability
+  /// Returns a list of available match group options based on tournament type and existing matches.
+  /// Filters out invalid options according to tournament rules.
   List<MatchGroup> getMatchGroupOption() {
     final options = MatchGroup.values.toList();
     final tournamentType = state.tournament?.type;
-    if (state.matches.containsKey(MatchGroup.round)) {
-      if (tournamentType == TournamentType.miniRobin ||
-          (tournamentType == TournamentType.doubleOut &&
-              (state.matches[MatchGroup.round]?.entries.length ?? 0) > 1)) {
-        options.remove(MatchGroup.round);
-      }
+    
+    final shouldRemoveRound = state.matches.containsKey(MatchGroup.round) &&
+        (tournamentType == TournamentType.miniRobin ||
+         (tournamentType == TournamentType.doubleOut &&
+          state.matches[MatchGroup.round]?.entries.length > 1));
+          
+    if (shouldRemoveRound) {
+      options.remove(MatchGroup.round);
     }

172-174: Improve consistency in group handling.

The finals group handling could be made consistent with other group validations by including it in the validation function.

-    if (state.matches.containsKey(MatchGroup.finals)) {
-      options.remove(MatchGroup.finals);
-    }
+    removeGroupForRestrictedTypes(MatchGroup.finals);
khelo/lib/ui/flow/settings/edit_profile/edit_profile_screen.dart (1)

181-182: LGTM! Consider extracting date constants.

The date range validation is a good addition that prevents invalid birth date selections. However, the hard-coded years could be moved to named constants for better maintainability.

Consider extracting the date values to named constants at the class level:

class EditProfileScreen extends ConsumerWidget {
  final bool isToCreateAccount;
+ static final DateTime _minBirthDate = DateTime(1950);

  const EditProfileScreen({super.key, required this.isToCreateAccount});

  final double profileViewHeight = 128;

Then update the selectDate call:

-              startDate: DateTime(1950),
+              startDate: _minBirthDate,
               endDate: DateTime.now(),
khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart (1)

130-161: Consider adding overflow handling for team initials

While the layout improvements and text scaling prevention are good, consider adding overflow handling for the team initials text to prevent potential layout issues with very long team names.

 Text(
   initials,
   style: AppTextStyle.subtitle1.copyWith(
     color: context.colorScheme.textPrimary,
   ),
+  overflow: TextOverflow.ellipsis,
 ),
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b75943a and ccc2aba.

📒 Files selected for processing (8)
  • data/lib/service/ball_score/ball_score_service.dart (2 hunks)
  • khelo/assets/locales/app_en.arb (1 hunks)
  • khelo/lib/components/won_by_message_text.dart (1 hunks)
  • khelo/lib/ui/flow/settings/edit_profile/edit_profile_screen.dart (1 hunks)
  • khelo/lib/ui/flow/tournament/detail/components/filter_tab_view.dart (1 hunks)
  • khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart (5 hunks)
  • khelo/lib/ui/flow/tournament/match_selection/match_selection_view_model.dart (1 hunks)
  • style/lib/pickers/date_and_time_picker.dart (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • khelo/assets/locales/app_en.arb
🔇 Additional comments (9)
khelo/lib/ui/flow/tournament/detail/components/filter_tab_view.dart (1)

25-30: LGTM! Good fix for potential text overflow issues.

Wrapping the title text in an Expanded widget is the correct solution here. It ensures that:

  1. The title text can utilize available space without causing overflow
  2. The filter button remains properly aligned and visible
style/lib/pickers/date_and_time_picker.dart (1)

9-10: LGTM! Parameter additions align with date validation requirements.

The new nullable parameters startDate and endDate provide the necessary flexibility for implementing date range validation.

khelo/lib/components/won_by_message_text.dart (2)

27-30: LGTM! Clear implementation of conditional tie message.

The conditional rendering based on tournament context is well-implemented and maintains consistent styling.


27-30: Verify localization keys exist.

Let's ensure both localization keys are properly defined.

✅ Verification successful

Both localization keys are properly defined

Both score_board_match_tied and score_board_match_tied_text keys are present in khelo/assets/locales/app_en.arb with appropriate translations:

  • score_board_match_tied: "Match tied"
  • score_board_match_tied_text: "The match has been tied"
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if both localization keys exist in the English locale file
# Expected: Both keys should be present in the arb file

# Search for both localization keys in the English locale file
echo "Checking for localization keys in English locale file..."
rg '"score_board_match_tied":' assets/locales/app_en.arb -A 1
rg '"score_board_match_tied_text":' assets/locales/app_en.arb -A 1

Length of output: 387


Script:

#!/bin/bash
# Let's first find the localization files in the repository
fd -e arb

# Then search for the localization keys in all arb files
echo "Searching for localization keys in all arb files..."
rg '"score_board_match_tied":' -A 1
rg '"score_board_match_tied_text":' -A 1

Length of output: 571

khelo/lib/ui/flow/tournament/match_selection/match_selection_view_model.dart (1)

145-177: Verify implementation matches PR requirements.

The implementation prevents adding quarterfinal/semifinal rounds in double out format as required. However, let's verify that this behavior is consistent across the codebase.

✅ Verification successful

Implementation correctly prevents quarterfinal/semifinal rounds in double out format

The implementation in match_selection_view_model.dart is consistent with the PR objective and works in conjunction with other files:

  • The view model's handleDoubleOutValidation method correctly removes quarterfinal/semifinal options for double out tournaments
  • The match scheduler (match_scheduler.dart) handles the group creation logic appropriately
  • The UI (match_selection_screen.dart) respects these restrictions when showing add buttons
  • No conflicting implementations were found in the codebase

The absence of test files suggests adding tests would be beneficial, but that's outside the scope of verifying the current implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that quarterfinal/semifinal rounds are properly restricted in double out tournaments

# Search for any other places where MatchGroup options are modified
rg -l "MatchGroup\.(quarterfinal|semifinal)"

# Search for tests verifying this behavior
rg -l "test.*double.*out.*quarterfinal" --type-add 'test:*{test,spec}*' --type test

Length of output: 435


Script:

#!/bin/bash
# Let's examine the implementation details in the related files

# Check match_selection_screen.dart for any MatchGroup handling
rg "MatchGroup\.(quarterfinal|semifinal)" khelo/lib/ui/flow/tournament/match_selection/match_selection_screen.dart -B 2 -A 2

# Check match_scheduler.dart for any MatchGroup handling
rg "MatchGroup\.(quarterfinal|semifinal)" khelo/lib/ui/flow/tournament/match_selection/match_scheduler.dart -B 2 -A 2

# Check enum_extensions.dart for any relevant MatchGroup definitions/handling
rg "MatchGroup\.(quarterfinal|semifinal)" khelo/lib/domain/extensions/enum_extensions.dart -B 2 -A 2

# Look for any test files related to match selection or tournament types
fd "test.*match.*selection" --type f
fd "test.*tournament.*type" --type f

Length of output: 2197

khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart (4)

85-110: LGTM! Layout improvements for match cell view

The changes improve the layout stability by:

  • Properly containing WonByMessageText within available space using Expanded
  • Adding center alignment for match time display

205-264: LGTM! Well-structured key stats cell implementation

The implementation shows good attention to detail with:

  • Proper text scaling prevention
  • Well-organized nested layout
  • Appropriate overflow handling for text elements

385-391: LGTM! Improved info cell layout stability

Good improvements with:

  • Proper space allocation using Expanded
  • Text scaling prevention for consistent layout

416-424: LGTM! Better header layout implementation

Good improvements:

  • Using Expanded instead of Spacer for better space allocation
  • Using Visibility for conditional rendering of the view all button

@canopas canopas deleted a comment from coderabbitai bot Nov 18, 2024
@canopas canopas deleted a comment from coderabbitai bot Nov 18, 2024
@cp-sidhdhi-p cp-sidhdhi-p merged commit 29feef1 into main Nov 19, 2024
2 checks passed
@cp-sidhdhi-p cp-sidhdhi-p deleted the sidhdhi/limit-creating-match-group branch November 19, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants