Conversation
- Renamed World Skills screen to World Leaderboards - Added metric switcher (Skills, TrueSkill, EPA) - Implemented fetching of TrueSkill rankings from RoboStem API (/api/rankings/statiq) - Added `getGlobalTrueSkillRankings` to ApiClient with grade level filtering - Added "Coming soon" placeholder for EPA tab - Fixed linter errors in widget argument ordering Co-authored-by: axcdeng <121896294+axcdeng@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @axcdeng, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the World Leaderboards screen by introducing TrueSkill rankings, providing a more comprehensive view of team performance. It includes a new UI control for users to easily switch between different metrics and integrates a new API endpoint to fetch the TrueSkill data. The underlying data fetching and UI rendering logic have been updated to support this new functionality, while maintaining the existing Skills leaderboard features. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively adds TrueSkill leaderboards to the app, renaming the screen to "World Leaderboards" and introducing a metric selector. The implementation is solid, with a new API client method for fetching TrueSkill data and UI updates to support the new functionality. My review includes suggestions to enhance performance by reusing network clients, improve error diagnostics through better logging, and increase code maintainability by refactoring duplicated UI and data-fetching logic.
| final dio = Dio(BaseOptions( | ||
| baseUrl: AppConstants.roboStemBaseUrl, // https://api.robostem-api.org | ||
| headers: { | ||
| 'x-api-key': token, | ||
| 'accept': 'application/json', | ||
| }, | ||
| connectTimeout: const Duration(seconds: 30), | ||
| receiveTimeout: const Duration(seconds: 30), | ||
| )); |
There was a problem hiding this comment.
Creating a new Dio instance within the getGlobalTrueSkillRankings method for every call is inefficient. This can lead to performance issues, such as socket exhaustion under heavy use, and prevents connection reuse. It's better to create a dedicated Dio instance for the RoboStem API at the class level, similar to how _dio is handled for RobotEvents, and reuse it across calls.
| } catch (e) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
The catch block currently swallows the error and returns an empty list without any logging. This can make it very difficult to debug API failures. It's a good practice to log the error, similar to how it's done in the getGlobalSkills method, to aid in diagnostics.
} catch (e) {
if (e is DioException) {
print('RoboStem API Error (TrueSkill): ${e.message} ${e.response?.statusCode}');
} else {
print('Error fetching TrueSkill rankings: $e');
}
return [];
}| children: { | ||
| 'Skills': Padding( | ||
| padding: const EdgeInsets.symmetric( | ||
| horizontal: 20, vertical: 8), | ||
| child: Text('Skills', | ||
| style: TextStyle( | ||
| fontWeight: FontWeight.w600, | ||
| fontSize: 13, | ||
| color: _metric == 'Skills' | ||
| ? Theme.of(context).colorScheme.onPrimary | ||
| : CupertinoColors.secondaryLabel | ||
| .resolveFrom(context))), | ||
| ), | ||
| 'TrueSkill': Padding( | ||
| padding: const EdgeInsets.symmetric( | ||
| horizontal: 20, vertical: 8), | ||
| child: Text('TrueSkill', | ||
| style: TextStyle( | ||
| fontWeight: FontWeight.w600, | ||
| fontSize: 13, | ||
| color: _metric == 'TrueSkill' | ||
| ? Theme.of(context).colorScheme.onPrimary | ||
| : CupertinoColors.secondaryLabel | ||
| .resolveFrom(context))), | ||
| ), | ||
| 'EPA': Padding( | ||
| padding: const EdgeInsets.symmetric( | ||
| horizontal: 20, vertical: 8), | ||
| child: Text('EPA', | ||
| style: TextStyle( | ||
| fontWeight: FontWeight.w600, | ||
| fontSize: 13, | ||
| color: _metric == 'EPA' | ||
| ? Theme.of(context).colorScheme.onPrimary | ||
| : CupertinoColors.secondaryLabel | ||
| .resolveFrom(context))), | ||
| ), | ||
| }, |
There was a problem hiding this comment.
The children map for the metric CupertinoSlidingSegmentedControl contains a lot of repeated code for each segment's Padding and Text widget. This makes the code harder to read and maintain. You can extract this logic into a helper method to build the segment widgets, which will reduce duplication and make future changes easier.
| Widget _buildContent(BuildContext context) { | ||
| if (_isLoading) { | ||
| return const Center(child: CupertinoActivityIndicator()); | ||
| } | ||
|
|
||
| if (_errorMessage.isNotEmpty) { | ||
| return Center(child: Text('Error: $_errorMessage')); | ||
| } | ||
|
|
||
| if (_metric == 'EPA') { | ||
| return const Center(child: Text('Coming soon')); | ||
| } | ||
|
|
||
| if (_metric == 'Skills') { | ||
| final currentList = | ||
| _gradeLevel == 'Middle School' ? _msSkills : _esSkills; | ||
| if (currentList.isEmpty) { | ||
| return const Center(child: Text('No data found')); | ||
| } | ||
| return ListView.builder( | ||
| padding: EdgeInsets.zero, | ||
| itemExtent: 72.0, | ||
| itemCount: currentList.length, | ||
| itemBuilder: (context, index) => | ||
| _buildSkillTile(currentList[index], context), | ||
| ); | ||
| } else if (_metric == 'TrueSkill') { | ||
| final currentList = | ||
| _gradeLevel == 'Middle School' ? _msTrueSkill : _esTrueSkill; | ||
| if (currentList.isEmpty) { | ||
| return const Center(child: Text('No data found')); | ||
| } | ||
| return ListView.builder( | ||
| padding: EdgeInsets.zero, | ||
| itemExtent: 72.0, | ||
| itemCount: currentList.length, | ||
| itemBuilder: (context, index) => | ||
| _buildTrueSkillTile(currentList[index], index + 1, context), | ||
| ); | ||
| } | ||
|
|
||
| return const SizedBox.shrink(); | ||
| } |
There was a problem hiding this comment.
The _buildContent method has significant code duplication for rendering the ListView.builder for 'Skills' and 'TrueSkill'. This can be refactored into a more generic structure to reduce redundancy and improve maintainability. By determining the data list and item builder function upfront, you can have a single ListView.builder implementation.
Widget _buildContent(BuildContext context) {
if (_isLoading) {
return const Center(child: CupertinoActivityIndicator());
}
if (_errorMessage.isNotEmpty) {
return Center(child: Text('Error: $_errorMessage'));
}
if (_metric == 'EPA') {
return const Center(child: Text('Coming soon'));
}
final List<dynamic> currentList;
final Widget Function(BuildContext, int) itemBuilder;
if (_metric == 'Skills') {
currentList = _gradeLevel == 'Middle School' ? _msSkills : _esSkills;
itemBuilder = (context, index) =>
_buildSkillTile(currentList[index] as Map<String, dynamic>, context);
} else if (_metric == 'TrueSkill') {
currentList = _gradeLevel == 'Middle School' ? _msTrueSkill : _esTrueSkill;
itemBuilder = (context, index) =>
_buildTrueSkillTile(currentList[index] as Team, index + 1, context);
} else {
return const SizedBox.shrink();
}
if (currentList.isEmpty) {
return const Center(child: Text('No data found'));
}
return ListView.builder(
padding: EdgeInsets.zero,
itemExtent: 72.0,
itemCount: currentList.length,
itemBuilder: itemBuilder,
);
}The project dependencies (specifically `url_launcher: ^6.3.2`) require Dart SDK ^3.6.0, which corresponds to Flutter 3.27+. The previous CI configuration was pinned to Flutter 3.10.0, causing dependency resolution failures. This commit upgrades the CI to use Flutter 3.27.1 to match the project requirements. Co-authored-by: axcdeng <121896294+axcdeng@users.noreply.github.com>
…oButton The CI analysis failed because `CupertinoButton` does not have a `minimumSize` parameter; it uses `minSize` which takes a `double`. This commit corrects the usage to `minSize: 0`. Co-authored-by: axcdeng <121896294+axcdeng@users.noreply.github.com>
Flutter 3.27+ deprecated `Color.withOpacity` in favor of `Color.withValues(alpha: ...)`. This commit updates `WorldSkillsScreen` to use the new API to resolve CI analysis warnings. Co-authored-by: axcdeng <121896294+axcdeng@users.noreply.github.com>
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request successfully adds the TrueSkill leaderboard feature, enhancing the newly renamed "World Leaderboards" screen. The implementation is mostly solid, but I've identified a couple of critical issues that will likely cause runtime errors, related to an incorrect method call on Color. Additionally, I've provided suggestions to improve error handling in the API client, increase efficiency by caching fetched data, and enhance code readability through refactoring. Addressing these points will improve the robustness and maintainability of the new feature.
| padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 6), | ||
| decoration: BoxDecoration( | ||
| color: primaryColor.withOpacity(0.15), | ||
| color: primaryColor.withValues(alpha: 0.15), |
There was a problem hiding this comment.
The method withValues does not exist on the Color class in Flutter. This will cause a runtime error. You should use withOpacity instead.
color: primaryColor.withOpacity(0.15),| Container( | ||
| padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 6), | ||
| decoration: BoxDecoration( | ||
| color: primaryColor.withValues(alpha: 0.15), |
There was a problem hiding this comment.
The method withValues does not exist on the Color class in Flutter. This will cause a runtime error. You should use withOpacity instead.
color: primaryColor.withOpacity(0.15),| } catch (e) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
This catch block silently swallows exceptions and returns an empty list. This prevents the UI from displaying a meaningful error message to the user in case of a network failure or other API error. The UI layer is prepared to handle exceptions. To improve error feedback, you should log the error and then rethrow it to be handled by the caller.
} catch (e) {
print('Error fetching global TrueSkill rankings: $e');
rethrow;
}| final token = _settings.roboStemApiKey ?? AppConstants.roboStemApiKey; | ||
| final dio = Dio(BaseOptions( | ||
| baseUrl: AppConstants.roboStemBaseUrl, // https://api.robostem-api.org | ||
| headers: { | ||
| 'x-api-key': token, | ||
| 'accept': 'application/json', | ||
| }, | ||
| connectTimeout: const Duration(seconds: 30), | ||
| receiveTimeout: const Duration(seconds: 30), | ||
| )); |
There was a problem hiding this comment.
This block for creating a Dio instance is nearly identical to the one in the existing getGlobalSkills method. To improve maintainability and reduce code duplication, consider extracting this logic into a private helper method, like _getRoboStemClient(), that returns a configured Dio instance for the RoboStem API. You could then call this helper in both getGlobalTrueSkillRankings and getGlobalSkills.
| Widget _buildContent(BuildContext context) { | ||
| if (_isLoading) { | ||
| return const Center(child: CupertinoActivityIndicator()); | ||
| } | ||
|
|
||
| if (_errorMessage.isNotEmpty) { | ||
| return Center(child: Text('Error: $_errorMessage')); | ||
| } | ||
|
|
||
| if (_metric == 'EPA') { | ||
| return const Center(child: Text('Coming soon')); | ||
| } | ||
|
|
||
| if (_metric == 'Skills') { | ||
| final currentList = | ||
| _gradeLevel == 'Middle School' ? _msSkills : _esSkills; | ||
| if (currentList.isEmpty) { | ||
| return const Center(child: Text('No data found')); | ||
| } | ||
| return ListView.builder( | ||
| padding: EdgeInsets.zero, | ||
| itemExtent: 72.0, | ||
| itemCount: currentList.length, | ||
| itemBuilder: (context, index) => | ||
| _buildSkillTile(currentList[index], context), | ||
| ); | ||
| } else if (_metric == 'TrueSkill') { | ||
| final currentList = | ||
| _gradeLevel == 'Middle School' ? _msTrueSkill : _esTrueSkill; | ||
| if (currentList.isEmpty) { | ||
| return const Center(child: Text('No data found')); | ||
| } | ||
| return ListView.builder( | ||
| padding: EdgeInsets.zero, | ||
| itemExtent: 72.0, | ||
| itemCount: currentList.length, | ||
| itemBuilder: (context, index) => | ||
| _buildTrueSkillTile(currentList[index], index + 1, context), | ||
| ); | ||
| } | ||
|
|
||
| return const SizedBox.shrink(); | ||
| } |
There was a problem hiding this comment.
This _buildContent method uses a series of if and else if statements to determine which widget to render. For better readability and maintainability, especially as more metrics might be added, consider refactoring this to use a switch statement on the _metric variable. This should come after handling the initial _isLoading and _errorMessage states.
Widget _buildContent(BuildContext context) {
if (_isLoading) {
return const Center(child: CupertinoActivityIndicator());
}
if (_errorMessage.isNotEmpty) {
return Center(child: Text('Error: $_errorMessage'));
}
switch (_metric) {
case 'Skills':
final currentList =
_gradeLevel == 'Middle School' ? _msSkills : _esSkills;
if (currentList.isEmpty) {
return const Center(child: Text('No data found'));
}
return ListView.builder(
padding: EdgeInsets.zero,
itemExtent: 72.0,
itemCount: currentList.length,
itemBuilder: (context, index) =>
_buildSkillTile(currentList[index], context),
);
case 'TrueSkill':
final currentList =
_gradeLevel == 'Middle School' ? _msTrueSkill : _esTrueSkill;
if (currentList.isEmpty) {
return const Center(child: Text('No data found'));
}
return ListView.builder(
padding: EdgeInsets.zero,
itemExtent: 72.0,
itemCount: currentList.length,
itemBuilder: (context, index) =>
_buildTrueSkillTile(currentList[index], index + 1, context),
);
case 'EPA':
return const Center(child: Text('Coming soon'));
default:
return const SizedBox.shrink();
}
}| borderRadius: BorderRadius.circular(20), | ||
| ), | ||
| child: Text( | ||
| (score as num).toStringAsFixed(2), |
There was a problem hiding this comment.
The score variable is already a double due to the null-coalescing operator ?? 0.0 on line 407. The cast to num is redundant and can be removed for cleaner code.
score.toStringAsFixed(2),- Add TrueSkill and EPA (placeholder) tabs to World Leaderboards. - Update CI to use Flutter 3.27.1 for compatibility with `url_launcher`. - Fix deprecated `withOpacity` usage by replacing with `withValues`. - Fix `CupertinoButton` parameter `minimumSize` -> `minSize`. - Fix `getGlobalTrueSkillRankings` logic to handle variable API response keys. Co-authored-by: axcdeng <121896294+axcdeng@users.noreply.github.com>
1 participant
This PR updates the World Skills screen to include TrueSkill leaderboards alongside the existing Skills rankings. It introduces a new segmented control to switch between metrics and integrates with the RoboStem API to fetch TrueSkill data. The screen title has been updated to "World Leaderboards". EPA metric is added as a placeholder. Code quality checks (flutter analyze) were addressed for the modified files.
PR created automatically by Jules for task 11135300667737851450 started by @axcdeng