Title: Fix a shuffle bug on desktop [fix]#9
Conversation
Details: Fix a shuffle bug on desktop Type: Bugfix. Test: Local Passed. Related Issue: N/A.
📝 WalkthroughWalkthrough此PR新增移动端Web登录（应用内浏览器）实现与UI，增加相关本地化字符串、常量与平台插件，并调整播放器队列循环与重排逻辑以支持桌面/移动差异与重新洗牌功能。 Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant LoginDialog as 登录对话框
participant WebLoginPage as Web登录页面
participant InAppWebView as 应用内浏览器
participant AuthServer as 认证服务器
participant CookiePoller as Cookie轮询器
participant AuthProvider as 认证提供者
participant CookieJar as Cookie存储
User->>LoginDialog: 选择 Web 登录
LoginDialog->>WebLoginPage: 打开 Web 登录页面
WebLoginPage->>InAppWebView: 加载 urlWebLogin
InAppWebView->>AuthServer: 提交认证请求
AuthServer-->>InAppWebView: 返回/重定向（可能到 success URL）
InAppWebView-->>WebLoginPage: 检测到重定向
WebLoginPage->>CookiePoller: 启动轮询（跨域）
CookiePoller->>AuthServer: 请求各域 Cookie
AuthServer-->>CookiePoller: 返回 Cookie 集合
CookiePoller->>AuthProvider: 验证并合并关键 Cookie
AuthProvider->>CookieJar: 保存 Cookie 到多个 Jar
CookieJar-->>AuthProvider: 保存完成
AuthProvider-->>WebLoginPage: 登录成功回调
WebLoginPage-->>LoginDialog: 关闭并返回成功
LoginDialog-->>User: 登录完成
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @UtopiaXC, 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 primarily resolves a shuffle playback issue on desktop platforms, ensuring a more consistent and reliable audio experience. Additionally, it introduces a new web login method specifically for mobile users, offering an alternative and potentially more convenient authentication flow by utilizing an embedded web browser to capture necessary login cookies. These changes enhance both the stability and user experience of the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 primarily fixes a shuffle bug on desktop by implementing a custom looping mechanism and re-shuffling the playlist when shuffle mode is activated. It also introduces a new web-based login feature for mobile platforms using an InAppWebView. A high-severity security issue has been identified: sensitive session cookies are logged to the application's debug log, which are persisted to disk and can be exported, creating a risk of session hijacking. Additionally, there is a high-severity suggestion to improve cookie handling robustness in the new web login feature and a medium-severity comment on a potential build configuration inconsistency. The shuffle bug fix and other changes appear solid and secure.
| for (var cookie in allCookies) { | ||
| final displayValue = cookie.value.length > 10 | ||
| ? '${cookie.value.substring(0, 10)}...' | ||
| : cookie.value; | ||
| Log.d(_tag, 'Cookie: ${cookie.name}=$displayValue'); | ||
| } |
There was a problem hiding this comment.
Sensitive session cookies are being logged to the debug log. Even though the values are truncated to 10 characters, this still leaks partial session data (like SESSDATA) and potentially full User IDs (DedeUserID). Since the application includes an 'Export Logs' feature in the settings, users might accidentally share their session credentials when providing logs for support or bug reporting.
Recommendation: Remove these log statements or ensure that sensitive cookie values are never logged. If logging is necessary for debugging, only log the cookie names.
// Remove or comment out the following loop to avoid logging sensitive cookies
/*
Log.d(_tag, 'Found ${allCookies.length} cookies');
for (var cookie in allCookies) {
final displayValue = cookie.value.length > 10
? '${cookie.value.substring(0, 10)}...'
: cookie.value;
Log.d(_tag, 'Cookie: ${cookie.name}=$displayValue');
}
*/| final Map<String, io.Cookie> cookieMap = {}; | ||
| for (var cookie in siteCookies) { | ||
| cookieMap[cookie.name] = io.Cookie(cookie.name, cookie.value); | ||
| } | ||
| for (var cookie in passportCookies) { | ||
| cookieMap[cookie.name] = io.Cookie(cookie.name, cookie.value); | ||
| } |
There was a problem hiding this comment.
The current implementation of merging cookies is lossy. It only preserves the name and value of the cookies, discarding other important attributes like domain, path, expires, and httpOnly. This can lead to incorrect cookie handling and potential authentication issues. It's better to preserve all relevant attributes when converting flutter_inappwebview's Cookie to dart:io's Cookie.
final Map<String, io.Cookie> cookieMap = {};
for (final c in [...siteCookies, ...passportCookies]) {
cookieMap[c.name] = io.Cookie(c.name, c.value)
..domain = c.domain
..path = c.path
..expires = c.expiresDate != null
? DateTime.fromMillisecondsSinceEpoch(c.expiresDate!)
: null
..httpOnly = c.isHttpOnly ?? false
..secure = c.isSecure ?? false;
}| flutter_inappwebview: | ||
| windows: | ||
| enabled: false | ||
| macos: | ||
| enabled: false |
There was a problem hiding this comment.
There's an inconsistency in the project configuration. You've disabled flutter_inappwebview for Windows and macOS here, but the generated plugin registration files (macos/Flutter/GeneratedPluginRegistrant.swift, windows/flutter/generated_plugin_registrant.cc) still include registration code for it. This might cause the plugin to be unnecessarily bundled in desktop builds, increasing their size. You might want to verify your build process, perhaps by running flutter clean followed by flutter pub get, to ensure the generated files reflect this configuration.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/audio/audio_player_service.dart (1)
486-509:⚠️ Potential issue | 🔴 Critical桌面端手动切歌 (
playNext) 在队列末尾不会循环——回归 bug。
_handleDesktopAutoNext（Line 108）已正确使用_shouldLoopQueue来处理自动播放下一首时的队列循环。但playNext()在桌面端（Line 494）仍然检查_player.loopMode == LoopMode.all。由于
player_provider.dart中对桌面端的 loop/shuffle 模式设置了LoopMode.off（而非LoopMode.all），当用户在队列最后一首歌时手动点击"下一首"，playNext不会回绕到第一首。而hasNext（在player_provider.dartLine 773）对 loop/shuffle 模式返回true，UI 上"下一首"按钮仍然可点击，但点击后无效果。建议：在 playNext 中也使用 _shouldLoopQueue
} else { - if (_player.loopMode == LoopMode.all) { + if (_shouldLoopQueue) { _currentIndex = 0; _indexController.add(0); await _playSingleDesktop(0); } }
🤖 Fix all issues with AI agents
In `@lib/widgets/login/web_login_page.dart`:
- Around line 87-93: The code currently logs cookie values (even truncated) in
the loop over allCookies; change the logging in the widget/page
(web_login_page.dart) to only log the cookie name and never output cookie.value
— update the loop that iterates over allCookies (variables: allCookies, cookie)
and the Log.d calls (using _tag) so they emit only something like the
cookie.name and count, removing any reference to cookie.value to avoid leaking
sensitive credentials like SESSDATA or DedeUserID.
- Around line 55-66: The Timer.periodic callback can reenter because it doesn't
await async work; modify _startPollingForCookies to prevent concurrent
_extractAndSaveCookies calls by adding a reentrancy guard (e.g., a private bool
like _isPolling) and check it at the start of the periodic callback, setting
_isPolling = true before awaiting _extractAndSaveCookies and clearing it in a
finally block; also keep the existing checks on _isLoginSuccess and attempts and
ensure _pollTimer is cancelled when _isLoginSuccess becomes true so cookies
aren't saved twice (references: _startPollingForCookies, _pollTimer,
Timer.periodic, _extractAndSaveCookies, _isLoginSuccess).
- Around line 118-128: Move the _isLoginSuccess assignment so it only flips to
true after all persistence and login steps complete: keep cancelling _pollTimer
as needed, but perform the Request()/await request.cookieJar and all await
cookieJar.saveFromResponse(Uri.parse(...), allCookies) calls and the
authProvider.login(...) call first, then set _isLoginSuccess = true; also ensure
any catch/finally resets _isLoginSuccess to false on failure so a retry is
possible. Reference: _isLoginSuccess, _pollTimer, Request,
cookieJar.saveFromResponse, authProvider.login.
In `@pubspec.yaml`:
- Around line 136-141: The pubspec disables flutter_inappwebview for
Windows/macOS but lib/widgets/login/web_login_page.dart still instantiates
InAppWebView (around line 177), which will crash on those platforms; either
re-enable the plugin for those platforms in pubspec.yaml or guard the usage in
WebLoginPage (or the widget/function that creates InAppWebView) with a platform
check (Platform.isWindows/Platform.isMacOS or kIsWeb/Theme.of(context).platform)
and provide an alternative web-login flow or fallback widget for unsupported
platforms; update the constructor/initialization in WebLoginPage to only create
InAppWebView when the platform is supported, and document the chosen approach in
pubspec or the widget code comments.
🧹 Nitpick comments (4)
pubspec.yaml (1)
56-57: PR 描述为修复 shuffle bug，但包含了大量 Web 登录相关的新依赖和功能代码。
webview_flutter和flutter_inappwebview是用于 Web 登录功能的新依赖，与 PR 标题描述的 "Fix a shuffle bug on desktop" 不符。建议将 Web 登录功能拆分到单独的 PR 中，以便于代码审查和问题追踪。lib/connection/utils/constants.dart (1)
5-8: User-Agent 字符串中的版本号较旧，可能影响 Web 登录兼容性。
userAgentIOS使用 iOS 16，userAgentAndroid使用 Android 13 / Chrome 116（2023 年 8 月发布）。部分网站可能会对过旧的 User-Agent 进行风控限制。建议定期更新这些字符串，或考虑动态获取设备的实际 User-Agent。lib/widgets/login/login_dialog.dart (1)
268-302:_buildWebLoginView中Navigator.push返回后缺少对context挂载状态的二次确认。Line 284 的
await Navigator.of(context).push<bool>(...)之后，Line 289 虽然检查了mounted，但在await和mounted检查之间，context可能已经失效。当前的实现已经用了mounted检查，但建议使用更明确的模式：建议的改进
onPressed: () async { final result = await Navigator.of(context).push<bool>( MaterialPageRoute( builder: (context) => const WebLoginPage(), ), ); - if (result == true && mounted) { + if (!mounted) return; + if (result == true) { Navigator.of(context).pop(); } },lib/providers/player_provider.dart (1)
527-535: 1 秒的保护超时机制可用但略显脆弱。
finally块中的 1000ms 延迟作为安全网防止_isTogglingPlayMode永久卡住是可以接受的。但在极端情况下（如设备性能不足或 just_audio 事件延迟），这个窗口可能不够长或过长。建议考虑将超时时长提取为常量，方便后续调优：
建议
+ static const Duration _toggleProtectionTimeout = Duration(milliseconds: 1000); + // ...in togglePlayMode: } finally { - await Future.delayed(const Duration(milliseconds: 1000)); + await Future.delayed(_toggleProtectionTimeout);
| void _startPollingForCookies() { | ||
| _pollTimer?.cancel(); | ||
| int attempts = 0; | ||
| _pollTimer = Timer.periodic(const Duration(milliseconds: 500), (timer) async { | ||
| attempts++; | ||
| if (attempts > 10 || _isLoginSuccess) { | ||
| timer.cancel(); | ||
| return; | ||
| } | ||
| await _extractAndSaveCookies(showError: false); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Timer.periodic 的异步回调可能导致并发重入问题。
Timer.periodic 不会等待异步回调完成。如果 _extractAndSaveCookies 执行时间超过 500ms，多个调用可能会重叠执行。_isLoginSuccess 检查在 Line 69 和赋值在 Line 120 之间存在竞态窗口，可能导致 cookie 被重复保存。
建议的改进：增加并发保护
void _startPollingForCookies() {
_pollTimer?.cancel();
int attempts = 0;
+ bool isPolling = false;
_pollTimer = Timer.periodic(const Duration(milliseconds: 500), (timer) async {
attempts++;
if (attempts > 10 || _isLoginSuccess) {
timer.cancel();
return;
}
+ if (isPolling) return;
+ isPolling = true;
- await _extractAndSaveCookies(showError: false);
+ try {
+ await _extractAndSaveCookies(showError: false);
+ } finally {
+ isPolling = false;
+ }
});
}🤖 Prompt for AI Agents
In `@lib/widgets/login/web_login_page.dart` around lines 55 - 66, The
Timer.periodic callback can reenter because it doesn't await async work; modify
_startPollingForCookies to prevent concurrent _extractAndSaveCookies calls by
adding a reentrancy guard (e.g., a private bool like _isPolling) and check it at
the start of the periodic callback, setting _isPolling = true before awaiting
_extractAndSaveCookies and clearing it in a finally block; also keep the
existing checks on _isLoginSuccess and attempts and ensure _pollTimer is
cancelled when _isLoginSuccess becomes true so cookies aren't saved twice
(references: _startPollingForCookies, _pollTimer, Timer.periodic,
_extractAndSaveCookies, _isLoginSuccess).
| Log.i(_tag, 'Found essential cookies, saving...'); | ||
|
|
||
| _isLoginSuccess = true; | ||
| _pollTimer?.cancel(); | ||
|
|
||
| final request = Request(); | ||
| final cookieJar = await request.cookieJar; | ||
|
|
||
| await cookieJar.saveFromResponse(Uri.parse(Api.urlBase), allCookies); | ||
| await cookieJar.saveFromResponse(Uri.parse(Api.urlLoginBase), allCookies); | ||
| await cookieJar.saveFromResponse(Uri.parse(Api.urlSiteBase), allCookies); |
There was a problem hiding this comment.
_isLoginSuccess 在 cookie 持久化完成前就被设为 true，若后续操作失败将无法重试。
Line 120 设置 _isLoginSuccess = true 后，如果 Lines 126-128 的 cookieJar.saveFromResponse 或 Line 134 的 authProvider.login 抛出异常，虽然会被 catch 捕获，但 _isLoginSuccess 仍为 true，用户无法再次触发登录流程。
建议：将成功标志移到所有操作完成之后
Log.i(_tag, 'Found essential cookies, saving...');
- _isLoginSuccess = true;
_pollTimer?.cancel();
final request = Request();
final cookieJar = await request.cookieJar;
await cookieJar.saveFromResponse(Uri.parse(Api.urlBase), allCookies);
await cookieJar.saveFromResponse(Uri.parse(Api.urlLoginBase), allCookies);
await cookieJar.saveFromResponse(Uri.parse(Api.urlSiteBase), allCookies);
Log.i(_tag, 'Cookies saved successfully');
+ _isLoginSuccess = true;
+
if (mounted) {🤖 Prompt for AI Agents
In `@lib/widgets/login/web_login_page.dart` around lines 118 - 128, Move the
_isLoginSuccess assignment so it only flips to true after all persistence and
login steps complete: keep cancelling _pollTimer as needed, but perform the
Request()/await request.cookieJar and all await
cookieJar.saveFromResponse(Uri.parse(...), allCookies) calls and the
authProvider.login(...) call first, then set _isLoginSuccess = true; also ensure
any catch/finally resets _isLoginSuccess to false on failure so a retry is
possible. Reference: _isLoginSuccess, _pollTimer, Request,
cookieJar.saveFromResponse, authProvider.login.
| flutter_inappwebview: | ||
| windows: | ||
| enabled: false | ||
| macos: | ||
| enabled: false | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check pubspec.yaml content around lines 136-141
echo "=== Checking pubspec.yaml at lines 136-141 ==="
sed -n '136,141p' pubspec.yaml
echo ""
echo "=== Full flutter_inappwebview config in pubspec.yaml ==="
rg -A 5 "flutter_inappwebview:" pubspec.yaml
echo ""
echo "=== Check windows/flutter/generated_plugins.cmake ==="
cat -n windows/flutter/generated_plugins.cmake
echo ""
echo "=== Check windows/flutter/generated_plugin_registrant.cc ==="
cat -n windows/flutter/generated_plugin_registrant.cc
echo ""
echo "=== Search for InAppWebView usage in dart files ==="
fd -e dart . lib --exec rg -l "InAppWebView|inappwebview" {} \;Repository: UtopiaXC/UtopiaMusic
Length of output: 3253
🏁 Script executed:
# Check web_login_page.dart for InAppWebView usage and platform conditions
echo "=== web_login_page.dart content ==="
cat -n lib/widgets/login/web_login_page.dart
echo ""
echo "=== Check for platform-specific code in web_login_page.dart ==="
rg -n "Platform|isWeb|kIsWeb|WebView" lib/widgets/login/web_login_page.dartRepository: UtopiaXC/UtopiaMusic
Length of output: 8432
InAppWebView 在 Windows/macOS 被禁用的平台上被无条件使用
pubspec.yaml 禁用了 flutter_inappwebview 在 Windows 和 macOS 上的支持，但 lib/widgets/login/web_login_page.dart 中 InAppWebView（第 177 行）的使用没有任何平台检查。这将导致应用在 Windows 或 macOS 上崩溃。
请确认：
- 是否需要在所有平台启用该插件，还是
- 需要为 Windows/macOS 添加平台检查，使用替代的 web 登录方法
如果选择启用该插件，则需要在 pubspec.yaml 中移除或修改 Windows/macOS 的禁用配置。
🤖 Prompt for AI Agents
In `@pubspec.yaml` around lines 136 - 141, The pubspec disables
flutter_inappwebview for Windows/macOS but lib/widgets/login/web_login_page.dart
still instantiates InAppWebView (around line 177), which will crash on those
platforms; either re-enable the plugin for those platforms in pubspec.yaml or
guard the usage in WebLoginPage (or the widget/function that creates
InAppWebView) with a platform check (Platform.isWindows/Platform.isMacOS or
kIsWeb/Theme.of(context).platform) and provide an alternative web-login flow or
fallback widget for unsupported platforms; update the constructor/initialization
in WebLoginPage to only create InAppWebView when the platform is supported, and
document the chosen approach in pubspec or the widget code comments.
Details: Addressing the potential risk of cookie leakage Type: Bugfix. Test: Local Passed. Related Issue: N/A.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new web-based login method for mobile platforms, leveraging flutter_inappwebview to display the Bilibili login page and extract authentication cookies. Key changes include adding new API constants for web login URLs, platform-specific user agents, and extensive internationalization strings for the new login flow. The LoginDialog was updated to include a 'Web Login' tab for mobile users, which navigates to a new WebLoginPage responsible for handling the in-app browser and cookie extraction. Additionally, the audio player's shuffle functionality was enhanced by implementing a reshufflePlaylist method in the database service, ensuring a new shuffle order when the play mode changes. Loop mode control was also refined, with a new _shouldLoopQueue flag to manage queue looping behavior, especially for desktop platforms. Review comments consistently pointed out the duplication of platform detection logic across player_provider.dart, login_dialog.dart, and web_login_page.dart, recommending consolidation into a centralized PlatformInfo utility class for improved maintainability.
| Log.v(_tag, "_setPlayerLoopMode"); | ||
| await player.setShuffleModeEnabled(false); | ||
|
|
||
| final isDesktop = !kIsWeb && (Platform.isWindows || Platform.isLinux || Platform.isMacOS); |
There was a problem hiding this comment.
This platform detection logic is duplicated in several places (player_provider.dart, audio_player_service.dart, login_dialog.dart). To improve maintainability and avoid code duplication, consider creating a centralized platform utility class.
For example, you could create a file lib/utils/platform_info.dart:
import 'dart:io';
import 'package:flutter/foundation.dart' show kIsWeb;
class PlatformInfo {
static bool get isDesktop => !kIsWeb && (Platform.isWindows || Platform.isLinux || Platform.isMacOS);
static bool get isMobile => !kIsWeb && (Platform.isIOS || Platform.isAndroid);
// ... other platform checks
}Then you can use PlatformInfo.isDesktop here and in other files, which makes the code cleaner and easier to manage.
| bool _isSuccess = false; | ||
| Timer? _pollTimer; | ||
| final LoginApi _loginApi = LoginApi(); | ||
| bool get _isMobile => !kIsWeb && (Platform.isIOS || Platform.isAndroid); |
There was a problem hiding this comment.
This platform detection logic is also present in other files. To improve code consistency and maintainability, it would be beneficial to consolidate these checks into a single utility class, as suggested in other comments.
| String _getUserAgent() { | ||
| if (io.Platform.isIOS) { | ||
| return HttpConstants.userAgentIOS; | ||
| } else if (io.Platform.isAndroid) { | ||
| return HttpConstants.userAgentAndroid; | ||
| } | ||
| return HttpConstants.userAgent; | ||
| } |
There was a problem hiding this comment.
Similar to other parts of the codebase, this platform-specific logic could be centralized into a utility class (e.g., PlatformInfo) to avoid duplication and improve maintainability. This would also make the code more consistent across the app.
Using the suggested PlatformInfo class, this could look like:
String _getUserAgent() {
if (PlatformInfo.isIOS) {
return HttpConstants.userAgentIOS;
} else if (PlatformInfo.isAndroid) {
return HttpConstants.userAgentAndroid;
}
return HttpConstants.userAgent;
}
1 participant
Details: Fix a shuffle bug on desktop
Type: Bugfix.
Test: Local Passed.
Related Issue: #7 #8
Closes #7 #8
Summary by CodeRabbit
发布说明
新功能
多语言支持
改进