Conversation
📝 WalkthroughWalkthroughThis PR introduces a comprehensive sound settings feature enabling users to configure notification sounds for pomodoro timer events (micro-break start/complete, focus complete, break complete) via built-in assets, system ringtones, or custom imported audio files. The change includes a new Dart/Flutter sound settings UI, native Android ringtone picker integration, permission management interface, and refactored timer event sound playback. The release also bundles 17 new agent skill documentation files covering Dart and Flutter development best practices, alongside build system updates and dependency version bumps. ChangesConfigurable Sound Settings and Permission Management
🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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 introduces a comprehensive system ringtone selection and custom sound management feature for a Flutter application, including native Android implementation via a Kotlin RingtoneChannel, a new sound settings UI, and dedicated services for permissions and file storage. However, several issues were identified in the review: critical compilation errors exist in backup_restore_db_service.dart and sound_settings/controller.dart because FilePicker.pickFiles is called instead of FilePicker.platform.pickFiles. Additionally, requesting the highly restricted REQUEST_IGNORE_BATTERY_OPTIMIZATIONS permission in AndroidManifest.xml poses a high risk of Google Play Store rejection. Finally, the backup settings map should be updated to include the display names of custom sounds to prevent data loss upon restoration.
|
|
||
| // 选择备份文件 | ||
| final result = await FilePicker.platform.pickFiles( | ||
| final result = await FilePicker.pickFiles( |
There was a problem hiding this comment.
The static method pickFiles does not exist on the FilePicker class. In the file_picker package, you must use FilePicker.platform.pickFiles to select files. Changing this will cause a compilation error.
Suggested change
| final result = await FilePicker.pickFiles( | |
| final result = await FilePicker.platform.pickFiles( |
| return; | ||
| } | ||
|
|
||
| final result = await FilePicker.pickFiles( |
There was a problem hiding this comment.
The static method pickFiles does not exist on the FilePicker class. Use FilePicker.platform.pickFiles instead to avoid compilation errors.
Suggested change
| final result = await FilePicker.pickFiles( | |
| final result = await FilePicker.platform.pickFiles( |
| <uses-permission android:name="android.permission.READ_MEDIA_AUDIO" /> | ||
|
|
||
| <!-- 电池优化白名单 --> | ||
| <uses-permission android:name="android.permission.REQUEST_IGNORE_BATTERY_OPTIMIZATIONS" /> |
There was a problem hiding this comment.
The REQUEST_IGNORE_BATTERY_OPTIMIZATIONS permission is highly restricted by Google Play Policy. Unless your app falls under specific exempted categories (like alarm clocks or active navigation apps), requesting this permission can lead to immediate app rejection during Google Play Store review. Consider using alternative background execution strategies (like WorkManager or Foreground Services) if you plan to distribute on Google Play.
Comment on lines
+195
to
+199
| // 加入提示音事件的 type/value 键 | ||
| for (final eventId in StorageKeys.soundEventIds) { | ||
| settingsMap[StorageKeys.soundTypeKey(eventId)] = 'string'; | ||
| settingsMap[StorageKeys.soundValueKey(eventId)] = 'string'; | ||
| } |
There was a problem hiding this comment.
The display names of the custom sounds/ringtones (StorageKeys.soundDisplayNameKey(eventId)) are not included in the backup settings map. After restoring a backup, the settings UI will lose the original filenames or ringtone titles and fall back to generic names like '自定义' or '系统铃声'. Consider backing up the display name keys as well.
Suggested change
| // 加入提示音事件的 type/value 键 | |
| for (final eventId in StorageKeys.soundEventIds) { | |
| settingsMap[StorageKeys.soundTypeKey(eventId)] = 'string'; | |
| settingsMap[StorageKeys.soundValueKey(eventId)] = 'string'; | |
| } | |
| // 加入提示音事件的 type/value/displayName 键 | |
| for (final eventId in StorageKeys.soundEventIds) { | |
| settingsMap[StorageKeys.soundTypeKey(eventId)] = 'string'; | |
| settingsMap[StorageKeys.soundValueKey(eventId)] = 'string'; | |
| settingsMap[StorageKeys.soundDisplayNameKey(eventId)] = 'string'; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/page/home/controller.dart (1)
223-236:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPause should stop system ringtone playback too.
_playAudio()can route throughRingtonePickerService, but_pauseTimer()only stops_audioPlayer. If the active sound is a system ringtone, pausing the timer won't actually silence it. A shared "stop all playback" helper would also help cover reset/close paths consistently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/page/home/controller.dart` around lines 223 - 236, The _pauseTimer function only stops the local _audioPlayer but not system ringtones routed via RingtonePickerService; add a shared helper (e.g., stopAllPlayback) that stops _audioPlayer and calls RingtonePickerService.stop() (or the service's equivalent) and use that helper from _pauseTimer (and from reset/close handlers). Update references: implement stopAllPlayback in the same controller (or a shared service) and replace the direct await _audioPlayer.stop() call in _pauseTimer with a call to stopAllPlayback so system ringtone playback is silenced as well.lib/page/setting/controller.dart (1)
325-356:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequest storage permission only after restore is confirmed.
Lines 325-336 run before the confirmation dialog, so tapping “restore” can trigger the OS permission prompt even when the user cancels on Line 356. Move the permission request below the
confirmed == truecheck so we only ask for access when the user has actually chosen to proceed.Suggested change
Future<void> performRestore() async { if (state.isRestoreInProgress.value) return; - - // 申请存储权限 - final perm = Get.find<PermissionService>(); - if (!await perm.requestStorage()) { - Get.snackbar( - '权限不足', - '恢复需要存储权限, 请在「权限管理→存储权限」中授予后再试', - snackPosition: SnackPosition.TOP, - barBlur: 100, - duration: const Duration(seconds: 3), - ); - return; - } // 先显示确认对话框 final confirmed = await Get.dialog<bool>( AlertDialog( title: const Text('确认恢复'), @@ ); if (confirmed != true) return; + + // 申请存储权限 + final perm = Get.find<PermissionService>(); + if (!await perm.requestStorage()) { + Get.snackbar( + '权限不足', + '恢复需要存储权限, 请在「权限管理→存储权限」中授予后再试', + snackPosition: SnackPosition.TOP, + barBlur: 100, + duration: const Duration(seconds: 3), + ); + return; + } state.isRestoreInProgress.value = true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/page/setting/controller.dart` around lines 325 - 356, The permission request currently runs before the confirmation dialog (perm.requestStorage() before Get.dialog), causing the OS storage prompt even if the user cancels; move the storage permission check and the Get.snackbar fallback to after the confirmed check (i.e., only call await perm.requestStorage() and show the snackbar if confirmed == true) so permission is requested only when the user explicitly confirms the restore (references: perm.requestStorage(), confirmed, Get.dialog, Get.snackbar)..github/workflows/dev.yml (1)
65-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin
appleboy/telegram-actionto a commit SHA (remove@master) in.github/workflows/dev.yml.
.github/workflows/dev.ymlusesappleboy/telegram-action@masterfor the build notification and each APK upload step (currently 4 occurrences).@masteris mutable, so CI is non-reproducible and vulnerable to upstream changes—pin eachuses:to a full commit SHA.🔒 Suggested hardening
- uses: appleboy/telegram-action@master + uses: appleboy/telegram-action@<full-commit-sha>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/dev.yml around lines 65 - 84, Replace the four occurrences of the mutable reference "appleboy/telegram-action@master" in the workflow steps that send APKs with the repository pinned to a specific commit SHA; locate each "uses: appleboy/telegram-action@master" entry (the steps named like "Send armeabi-v7a APK", "Send arm64-v8a APK", "Send x86_64 APK", etc.) and update the value to "appleboy/telegram-action@<full-commit-sha>" where <full-commit-sha> is the exact commit hash you want to pin; after updating all occurrences, run/validate the workflow lint or a workflow dry-run to ensure the pinned action syntax is correct and the APK upload steps still execute.
🧹 Nitpick comments (3)
pubspec.yaml (1)
43-43: ⚡ Quick winAvoid taking a prerelease
file_pickerinto the main app unless you need a beta-only fix.
file_pickeris still on the12.0.0-beta.5prerelease track on pub.dev. Unless this PR depends on a beta-only API/bugfix, prefer a stable release here so dependency resolution and release builds are less volatile. (pub.dev)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pubspec.yaml` at line 43, The pubspec entry currently pins a prerelease package "file_picker" at 12.0.0-beta.5; replace that prerelease version with a stable release (e.g., the latest non-beta version) in pubspec.yaml so the app depends on a stable file_picker release instead of a beta; update the "file_picker" line to reference the chosen stable version and run pub get to verify dependency resolution..github/workflows/release.yml (2)
49-54: ⚡ Quick winConsider adding verification that APKs exist before copying.
The step assumes the APK files exist at the expected paths. While
cpwill fail if they don't exist, adding explicit verification improves clarity and debugging.🔍 Proposed improvement with file existence checks
- name: Rename APKs run: | mkdir -p dist + # Verify APKs were built successfully + test -f build/app/outputs/flutter-apk/app-arm64-v8a-release.apk || exit 1 + test -f build/app/outputs/flutter-apk/app-x86_64-release.apk || exit 1 cp build/app/outputs/flutter-apk/app-arm64-v8a-release.apk dist/time_machine-arm64-v8a.apk - cp build/app/outputs/flutter-apk/app-x86_64-release.apk dist/time_machine-x86_64.apk + cp build/app/outputs/flutter-apk/app-x86_64-release.apk dist/time_machine-x86_64.apk🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 49 - 54, Update the "Rename APKs" workflow step to explicitly verify each expected APK file exists before attempting to copy: check the two source paths used in the step (build/app/outputs/flutter-apk/app-arm64-v8a-release.apk and build/app/outputs/flutter-apk/app-x86_64-release.apk), emit a clear error via the workflow and exit non‑zero if any file is missing, then perform the mkdir -p dist and cp commands only after the checks pass so failures are obvious and easier to debug.
82-87: ⚡ Quick winAdd error handling to fail fast if any upload fails.
The for loop continues even if an upload fails. Consider adding
set -eor explicit error checking to ensure both APKs are uploaded successfully.💪 Proposed improvement with error handling
run: | + set -e # Exit on first error for f in time_machine-arm64-v8a.apk time_machine-x86_64.apk; do aws s3 cp "dist/$f" \ "s3://${{ secrets.R2_BUCKET }}/time_machine/$f" \ --endpoint-url "${{ secrets.R2_ENDPOINT }}" done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 82 - 87, The current run step's for loop (for f in time_machine-arm64-v8a.apk time_machine-x86_64.apk; do ... aws s3 cp ...) continues on error; update the run block to fail-fast by enabling strict shell error handling (e.g., add set -euo pipefail at the start of the run script) or explicitly check the exit status of each aws s3 cp and exit non-zero on failure so the workflow stops if any upload fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/dart-collect-coverage/SKILL.md:
- Line 3: Fix the typo in the SKILL.md description field by changing "packge" to
"package" so the line reads "description: Collect coverage using the coverage
package and create an LCOV report"; update the description entry in SKILL.md
(the frontmatter/description string) accordingly.
In @.agents/skills/dart-use-pattern-matching/SKILL.md:
- Around line 138-146: The guard-clause switch uses a non-existent `size` field;
align the patterns with the earlier class fields by replacing the logical-or
case with separate cases: use `Square(length: var l) when l > 0` and
`Circle(radius: var r) when r > 0` to detect valid symmetric shapes, then add
corresponding non-positive/empty cases like `Square()` and `Circle()` for
invalid shapes; update any references to the captured variable names (`l`, `r`)
in the print messages accordingly.
In `@android/app/src/main/kotlin/top/merack/time_machine/RingtoneChannel.kt`:
- Around line 41-42: In dispose(), ensure any in-flight picker result is
resolved before clearing handlers: if pendingResult != null, call
pendingResult.error("CANCELLED", "Activity disposed while picker open", null)
(or pendingResult.success(null) if you prefer treating it as a cancel), then set
pendingResult = null; do this before unregistering the MethodChannel/clearing
handlers and also make sure to release/stop currentPlayer
(currentPlayer?.release(); currentPlayer = null) to fully clean up resources.
In `@lib/config/storage_keys.dart`:
- Around line 87-93: The display label for StorageKeys.soundEventBreakComplete
in the eventDisplayNames map currently only mentions big/long breaks, but that
key is also used for TimerStatus.shortBreak; update the map entry for
soundEventBreakComplete in eventDisplayNames so the value explicitly includes
short breaks (e.g., include “短休息” alongside the existing “大休息/长休息结束”) to avoid
misleading the settings UI.
In `@lib/database/backup_restore_db_service.dart`:
- Around line 195-199: The current backup code only saves
StorageKeys.soundTypeKey(eventId) and soundValueKey(eventId) but omits
StorageKeys.soundDisplayNameKey(...) and any custom sound files, so restores
produce broken file paths; update the BackupRestoreDbService backup routine to
also include soundDisplayNameKey for each event and, for entries where
soundTypeKey indicates a custom file, bundle the referenced file data (read
bytes from the path in soundValueKey) into the backup payload (or record a flag
to re-attach the file). In the restore routine, if restoring a custom sound
entry, attempt to recreate the file in app-private storage from the bundled
bytes and update soundValueKey to the new path; if the file is missing/corrupted
or you choose not to include files, instead normalize that event to a safe
default sound (write default type/value/displayName) so you never restore broken
paths. Ensure you use StorageKeys.soundEventIds, StorageKeys.soundTypeKey(...),
StorageKeys.soundValueKey(...), and StorageKeys.soundDisplayNameKey(...) when
implementing both backup and restore steps.
In `@lib/page/sound_settings/controller.dart`:
- Around line 99-106: When switching an event away from a custom/imported file
(e.g., in selectBuiltin), detect whether the prior sound type for that event was
the custom/imported type, obtain the old path from
StorageKeys.soundValueKey(eventId), delete the underlying file/resource for that
path, and clear any persistent storage entries for the file (removeValue or
equivalent) before writing the new builtin values and refreshing state; apply
the same cleanup logic to the other handlers that switch types (the methods
covering the 109-124 and 175-184 ranges) so switching to system/builtin/none
always removes the previous custom file and its storage reference.
In `@lib/service/custom_sound_storage_service.dart`:
- Around line 51-57: The code currently calls _deleteExistingForEvent(eventId)
before copying the new file, which can leave MMKV pointing to a missing file if
the copy fails; instead copy the sourcePath to a temporary file inside soundsDir
(e.g. p.join(dir.path, '${eventId}.tmp${ext}') using src.copy), then if the copy
succeeds call _deleteExistingForEvent(eventId) and atomically rename/move the
temp file to the final name (p.join(dir.path, '$eventId$ext')) or replace via
tempFile.renameSync/await, ensuring errors during copy do not remove the
existing file referenced by MMKV; update code around soundsDir, src.copy, dest
and _deleteExistingForEvent to implement this safe replace pattern.
In `@lib/service/permission_service.dart`:
- Around line 79-88: Change requestAudioRead() in permission_service.dart to
return the raw PermissionStatus instead of a bool: keep the Platform.isAndroid
and _androidSdkInt >= 33 branch logic but return the PermissionStatus from
Permission.audio.request() or Permission.storage.request() directly (do not map
to status.isGranted). Then update callers (notably selectCustom in
sound_settings/controller.dart where _permissionService.requestAudioRead() is
awaited) to handle the PermissionStatus: check status.isGranted to proceed, and
if status.isPermanentlyDenied show the snackbar hint and call openSettings()
(matching the UX in permission_settings.dart); also update any other call sites
that assumed a bool to use status.isGranted/isPermanentlyDenied accordingly.
---
Outside diff comments:
In @.github/workflows/dev.yml:
- Around line 65-84: Replace the four occurrences of the mutable reference
"appleboy/telegram-action@master" in the workflow steps that send APKs with the
repository pinned to a specific commit SHA; locate each "uses:
appleboy/telegram-action@master" entry (the steps named like "Send armeabi-v7a
APK", "Send arm64-v8a APK", "Send x86_64 APK", etc.) and update the value to
"appleboy/telegram-action@<full-commit-sha>" where <full-commit-sha> is the
exact commit hash you want to pin; after updating all occurrences, run/validate
the workflow lint or a workflow dry-run to ensure the pinned action syntax is
correct and the APK upload steps still execute.
In `@lib/page/home/controller.dart`:
- Around line 223-236: The _pauseTimer function only stops the local
_audioPlayer but not system ringtones routed via RingtonePickerService; add a
shared helper (e.g., stopAllPlayback) that stops _audioPlayer and calls
RingtonePickerService.stop() (or the service's equivalent) and use that helper
from _pauseTimer (and from reset/close handlers). Update references: implement
stopAllPlayback in the same controller (or a shared service) and replace the
direct await _audioPlayer.stop() call in _pauseTimer with a call to
stopAllPlayback so system ringtone playback is silenced as well.
In `@lib/page/setting/controller.dart`:
- Around line 325-356: The permission request currently runs before the
confirmation dialog (perm.requestStorage() before Get.dialog), causing the OS
storage prompt even if the user cancels; move the storage permission check and
the Get.snackbar fallback to after the confirmed check (i.e., only call await
perm.requestStorage() and show the snackbar if confirmed == true) so permission
is requested only when the user explicitly confirms the restore (references:
perm.requestStorage(), confirmed, Get.dialog, Get.snackbar).
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 49-54: Update the "Rename APKs" workflow step to explicitly verify
each expected APK file exists before attempting to copy: check the two source
paths used in the step (build/app/outputs/flutter-apk/app-arm64-v8a-release.apk
and build/app/outputs/flutter-apk/app-x86_64-release.apk), emit a clear error
via the workflow and exit non‑zero if any file is missing, then perform the
mkdir -p dist and cp commands only after the checks pass so failures are obvious
and easier to debug.
- Around line 82-87: The current run step's for loop (for f in
time_machine-arm64-v8a.apk time_machine-x86_64.apk; do ... aws s3 cp ...)
continues on error; update the run block to fail-fast by enabling strict shell
error handling (e.g., add set -euo pipefail at the start of the run script) or
explicitly check the exit status of each aws s3 cp and exit non-zero on failure
so the workflow stops if any upload fails.
In `@pubspec.yaml`:
- Line 43: The pubspec entry currently pins a prerelease package "file_picker"
at 12.0.0-beta.5; replace that prerelease version with a stable release (e.g.,
the latest non-beta version) in pubspec.yaml so the app depends on a stable
file_picker release instead of a beta; update the "file_picker" line to
reference the chosen stable version and run pub get to verify dependency
resolution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74a7d43e-0053-4ce5-ad2c-0faadc27cc8f
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (55)
.agents/skills/dart-add-unit-test/SKILL.md.agents/skills/dart-build-cli-app/SKILL.md.agents/skills/dart-collect-coverage/SKILL.md.agents/skills/dart-fix-runtime-errors/SKILL.md.agents/skills/dart-generate-test-mocks/SKILL.md.agents/skills/dart-migrate-to-checks-package/SKILL.md.agents/skills/dart-resolve-package-conflicts/SKILL.md.agents/skills/dart-run-static-analysis/SKILL.md.agents/skills/dart-use-pattern-matching/SKILL.md.agents/skills/flutter-add-integration-test/SKILL.md.agents/skills/flutter-add-widget-preview/SKILL.md.agents/skills/flutter-add-widget-test/SKILL.md.agents/skills/flutter-apply-architecture-best-practices/SKILL.md.agents/skills/flutter-build-responsive-layout/SKILL.md.agents/skills/flutter-fix-layout-issues/SKILL.md.agents/skills/flutter-implement-json-serialization/SKILL.md.agents/skills/flutter-setup-declarative-routing/SKILL.md.agents/skills/flutter-setup-localization/SKILL.md.agents/skills/flutter-use-http-package/SKILL.md.github/workflows/dev.yml.github/workflows/release.yml.gitignoreandroid/app/build.gradle.ktsandroid/app/src/main/AndroidManifest.xmlandroid/app/src/main/kotlin/top/merack/time_machine/MainActivity.ktandroid/app/src/main/kotlin/top/merack/time_machine/RingtoneChannel.ktandroid/build.gradle.ktsandroid/gradle.propertiesandroid/gradle/wrapper/gradle-wrapper.propertiesandroid/settings.gradle.ktslib/config/storage_keys.dartlib/database/backup_restore_db_service.dartlib/database/database_helper.dartlib/main.dartlib/page/home/controller.dartlib/page/setting/controller.dartlib/page/setting/view.dartlib/page/setting/widgets/permission_settings.dartlib/page/setting/widgets/pomodoro_settings.dartlib/page/setting/widgets/setting_tile.dartlib/page/setting/widgets/widgets.dartlib/page/sound_settings/controller.dartlib/page/sound_settings/state.dartlib/page/sound_settings/view.dartlib/page/sound_settings/widgets/sound_picker_dialog.dartlib/route/route_name.dartlib/route/route_page.dartlib/service/custom_sound_storage_service.dartlib/service/permission_service.dartlib/service/ringtone_picker_service.dartlinux/flutter/generated_plugins.cmakepubspec.yamlskills-lock.jsonwindows/flutter/generated_plugin_registrant.ccwindows/flutter/generated_plugins.cmake
💤 Files with no reviewable changes (2)
- lib/page/setting/widgets/pomodoro_settings.dart
- lib/database/database_helper.dart
| @@ -0,0 +1,141 @@ | |||
| --- | |||
| name: dart-collect-coverage | |||
| description: Collect coverage using the coverage packge and create an LCOV report | |||
There was a problem hiding this comment.
Fix typo in description.
The word "packge" should be "package".
📝 Proposed fix
-description: Collect coverage using the coverage packge and create an LCOV report
+description: Collect coverage using the coverage package and create an LCOV report📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| description: Collect coverage using the coverage packge and create an LCOV report | |
| description: Collect coverage using the coverage package and create an LCOV report |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/dart-collect-coverage/SKILL.md at line 3, Fix the typo in the
SKILL.md description field by changing "packge" to "package" so the line reads
"description: Collect coverage using the coverage package and create an LCOV
report"; update the description entry in SKILL.md (the frontmatter/description
string) accordingly.
Comment on lines
+138
to
+146
| switch (shape) { | ||
| case Square(size: var s) || Circle(size: var s) when s > 0: | ||
| print('Valid symmetric shape with size $s'); | ||
| case Square() || Circle(): | ||
| print('Invalid or empty shape'); | ||
| default: | ||
| print('Unknown shape'); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Fix inconsistent field names in the guard clause example.
The example references Square(size: var s) || Circle(size: var s) but the Shape classes defined earlier (lines 102-118) have Square(length:) and Circle(radius:), not a size field. This code will not compile.
🐛 Proposed fix to align with the earlier class definitions
-switch (shape) {
- case Square(size: var s) || Circle(size: var s) when s > 0:
- print('Valid symmetric shape with size $s');
- case Square() || Circle():
- print('Invalid or empty shape');
- default:
- print('Unknown shape');
-}
+switch (shape) {
+ case Square(length: var s) when s > 0:
+ print('Valid square with size $s');
+ case Circle(radius: var s) when s > 0:
+ print('Valid circle with size $s');
+ case Square() || Circle():
+ print('Invalid or empty shape');
+ default:
+ print('Unknown shape');
+}Note: The logical-or pattern cannot be used here because Square and Circle have differently named fields (length vs radius). The corrected version uses separate cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/dart-use-pattern-matching/SKILL.md around lines 138 - 146,
The guard-clause switch uses a non-existent `size` field; align the patterns
with the earlier class fields by replacing the logical-or case with separate
cases: use `Square(length: var l) when l > 0` and `Circle(radius: var r) when r
> 0` to detect valid symmetric shapes, then add corresponding non-positive/empty
cases like `Square()` and `Circle()` for invalid shapes; update any references
to the captured variable names (`l`, `r`) in the print messages accordingly.
Comment on lines
+41
to
+42
| private var pendingResult: MethodChannel.Result? = null | ||
| private var currentPlayer: MediaPlayer? = null |
There was a problem hiding this comment.
Complete the in-flight picker result during disposal.
If the activity is destroyed while the system picker is open, pendingResult is abandoned here without success/error. That leaves the Dart pickSystemRingtone() future hanging forever after the activity recreation path. Resolve and clear pendingResult in dispose() before unregistering the channel.
Suggested fix
fun dispose() {
+ pendingResult?.error(
+ "ACTIVITY_DESTROYED",
+ "Ringtone picker was interrupted",
+ null
+ )
+ pendingResult = null
stopCurrent(notifyCompleted = false)
channel.setMethodCallHandler(null)
}Also applies to: 296-299
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@android/app/src/main/kotlin/top/merack/time_machine/RingtoneChannel.kt`
around lines 41 - 42, In dispose(), ensure any in-flight picker result is
resolved before clearing handlers: if pendingResult != null, call
pendingResult.error("CANCELLED", "Activity disposed while picker open", null)
(or pendingResult.success(null) if you prefer treating it as a cancel), then set
pendingResult = null; do this before unregistering the MethodChannel/clearing
handlers and also make sure to release/stop currentPlayer
(currentPlayer?.release(); currentPlayer = null) to fully clean up resources.
Comment on lines
+87
to
+93
| // 事件展示名 | ||
| static const Map<String, String> eventDisplayNames = { | ||
| soundEventMicroBreakStart: '微休息开始', | ||
| soundEventMicroBreakComplete: '微休息结束', | ||
| soundEventFocusComplete: '专注完成', | ||
| soundEventBreakComplete: '大休息/长休息结束', | ||
| }; |
There was a problem hiding this comment.
Include short breaks in this event label.
StorageKeys.soundEventBreakComplete is also used for TimerStatus.shortBreak in lib/page/home/controller.dart, but the display name here only mentions big/long breaks. The settings UI will mislead users about which phase this sound controls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/config/storage_keys.dart` around lines 87 - 93, The display label for
StorageKeys.soundEventBreakComplete in the eventDisplayNames map currently only
mentions big/long breaks, but that key is also used for TimerStatus.shortBreak;
update the map entry for soundEventBreakComplete in eventDisplayNames so the
value explicitly includes short breaks (e.g., include “短休息” alongside the
existing “大休息/长休息结束”) to avoid misleading the settings UI.
Comment on lines
+195
to
+199
| // 加入提示音事件的 type/value 键 | ||
| for (final eventId in StorageKeys.soundEventIds) { | ||
| settingsMap[StorageKeys.soundTypeKey(eventId)] = 'string'; | ||
| settingsMap[StorageKeys.soundValueKey(eventId)] = 'string'; | ||
| } |
There was a problem hiding this comment.
The backup format still doesn't preserve usable custom sound selections.
This only stores the MMKV type/value keys, but custom sounds live as files under app-private storage. After restore on a fresh install or another device, those restored custom entries will point to files that were never backed up. soundDisplayNameKey(...) is also omitted, so even surviving system/custom entries lose their titles.
Please either include the custom sound files in the backup/restore flow, or normalize restored custom selections back to defaults instead of restoring broken paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/database/backup_restore_db_service.dart` around lines 195 - 199, The
current backup code only saves StorageKeys.soundTypeKey(eventId) and
soundValueKey(eventId) but omits StorageKeys.soundDisplayNameKey(...) and any
custom sound files, so restores produce broken file paths; update the
BackupRestoreDbService backup routine to also include soundDisplayNameKey for
each event and, for entries where soundTypeKey indicates a custom file, bundle
the referenced file data (read bytes from the path in soundValueKey) into the
backup payload (or record a flag to re-attach the file). In the restore routine,
if restoring a custom sound entry, attempt to recreate the file in app-private
storage from the bundled bytes and update soundValueKey to the new path; if the
file is missing/corrupted or you choose not to include files, instead normalize
that event to a safe default sound (write default type/value/displayName) so you
never restore broken paths. Ensure you use StorageKeys.soundEventIds,
StorageKeys.soundTypeKey(...), StorageKeys.soundValueKey(...), and
StorageKeys.soundDisplayNameKey(...) when implementing both backup and restore
steps.
Comment on lines
+99
to
+106
| Future<void> selectBuiltin(String eventId, String assetPath) async { | ||
| await stopAllPreview(); | ||
| _storage.encodeString(StorageKeys.soundTypeKey(eventId), StorageKeys.soundTypeBuiltin); | ||
| _storage.encodeString(StorageKeys.soundValueKey(eventId), assetPath); | ||
| _storage.removeValue(StorageKeys.soundDisplayNameKey(eventId)); | ||
| state.configs[eventId] = _readEvent(eventId); | ||
| state.configs.refresh(); | ||
| } |
There was a problem hiding this comment.
Clean up the old custom file when switching away from it.
These paths update MMKV out of custom, but they never remove the previously imported file for that event. Right now those files only get reclaimed on the next cleanupOrphans() run, so a user can accumulate stale files during one session.
Also applies to: 109-124, 175-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/page/sound_settings/controller.dart` around lines 99 - 106, When
switching an event away from a custom/imported file (e.g., in selectBuiltin),
detect whether the prior sound type for that event was the custom/imported type,
obtain the old path from StorageKeys.soundValueKey(eventId), delete the
underlying file/resource for that path, and clear any persistent storage entries
for the file (removeValue or equivalent) before writing the new builtin values
and refreshing state; apply the same cleanup logic to the other handlers that
switch types (the methods covering the 109-124 and 175-184 ranges) so switching
to system/builtin/none always removes the previous custom file and its storage
reference.
Comment on lines
+51
to
+57
| // 删掉该 eventId 的旧自定义音(如果有) | ||
| await _deleteExistingForEvent(eventId); | ||
|
|
||
| final ext = p.extension(sourcePath); // 含点符号 | ||
| final dir = await soundsDir; | ||
| final dest = File(p.join(dir.path, '$eventId$ext')); | ||
| await src.copy(dest.path); |
There was a problem hiding this comment.
Copy first, then replace the old custom sound.
Deleting the current file before the new copy succeeds can leave the existing MMKV entry pointing at a missing file when the import fails. This turns a transient I/O failure into data loss for the current selection.
Suggested fix
- // 删掉该 eventId 的旧自定义音(如果有)
- await _deleteExistingForEvent(eventId);
-
final ext = p.extension(sourcePath); // 含点符号
final dir = await soundsDir;
- final dest = File(p.join(dir.path, '$eventId$ext'));
- await src.copy(dest.path);
+ final temp = File(p.join(dir.path, '$eventId.importing$ext'));
+ await src.copy(temp.path);
+
+ // 新文件落盘成功后再替换旧文件
+ await _deleteExistingForEvent(eventId);
+ final dest = File(p.join(dir.path, '$eventId$ext'));
+ await temp.rename(dest.path);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 删掉该 eventId 的旧自定义音(如果有) | |
| await _deleteExistingForEvent(eventId); | |
| final ext = p.extension(sourcePath); // 含点符号 | |
| final dir = await soundsDir; | |
| final dest = File(p.join(dir.path, '$eventId$ext')); | |
| await src.copy(dest.path); | |
| final ext = p.extension(sourcePath); // 含点符号 | |
| final dir = await soundsDir; | |
| final temp = File(p.join(dir.path, '$eventId.importing$ext')); | |
| await src.copy(temp.path); | |
| // 新文件落盘成功后再替换旧文件 | |
| await _deleteExistingForEvent(eventId); | |
| final dest = File(p.join(dir.path, '$eventId$ext')); | |
| await temp.rename(dest.path); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/service/custom_sound_storage_service.dart` around lines 51 - 57, The code
currently calls _deleteExistingForEvent(eventId) before copying the new file,
which can leave MMKV pointing to a missing file if the copy fails; instead copy
the sourcePath to a temporary file inside soundsDir (e.g. p.join(dir.path,
'${eventId}.tmp${ext}') using src.copy), then if the copy succeeds call
_deleteExistingForEvent(eventId) and atomically rename/move the temp file to the
final name (p.join(dir.path, '$eventId$ext')) or replace via
tempFile.renameSync/await, ensuring errors during copy do not remove the
existing file referenced by MMKV; update code around soundsDir, src.copy, dest
and _deleteExistingForEvent to implement this safe replace pattern.
Comment on lines
+79
to
+88
| /// 读取音频文件需要的权限(Android 13+ READ_MEDIA_AUDIO) | ||
| Future<bool> requestAudioRead() async { | ||
| if (!Platform.isAndroid) return true; | ||
| if (_androidSdkInt >= 33) { | ||
| final status = await Permission.audio.request(); | ||
| return status.isGranted; | ||
| } | ||
| final status = await Permission.storage.request(); | ||
| return status.isGranted; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate requestAudioRead() and its callers
rg -n "requestAudioRead\(" -S .
# Show the implementation around lib/service/permission_service.dart:79-88
sed -n '60,120p' lib/service/permission_service.dart
# Inspect controller usage around lib/page/sound_settings/controller.dart:126-142
sed -n '90,190p' lib/page/sound_settings/controller.dart
# Check for openSettings() / permanentlyDenied handling in the controller
rg -n "openSettings|permanentlyDenied|PermissionStatus" lib/page/sound_settings/controller.dartRepository: Merack/time_machine
Length of output: 4961
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "openSettings\(" lib/page/sound_settings/controller.dart
rg -n "describeStatus|permanentlyDenied|PermissionStatus" lib/page/sound_settings/controller.dartRepository: Merack/time_machine
Length of output: 45
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all callers of requestAudioRead/requestStorage/requestBatteryWhitelist
rg -n "requestAudioRead\(|requestStorage\(|requestBatteryWhitelist\(" lib
# Find openSettings usage across repo
rg -n "openSettings\(" lib
# Find any PermissionStatus branching for other permissions
rg -n "permanentlyDenied|isPermanentlyDenied|openAppSettings|openAppSettings\(" lib
# Show permission-related sections near controller permission calls
sed -n '110,170p' lib/page/sound_settings/controller.dart
sed -n '1,160p' lib/service/permission_service.dartRepository: Merack/time_machine
Length of output: 7039
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show selectCustom block around requestAudioRead usage
sed -n '110,160p' lib/page/sound_settings/controller.dart
# Confirm only call sites of requestAudioRead
rg -n "requestAudioRead\(" libRepository: Merack/time_machine
Length of output: 1945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '220,360p' lib/page/setting/controller.dart
sed -n '1,160p' lib/page/setting/widgets/permission_settings.dartRepository: Merack/time_machine
Length of output: 8342
Return PermissionStatus (and handle permanentlyDenied) for audio permission requests.
requestAudioRead() returns only status.isGranted, so denied and permanentlyDenied are both collapsed to false. In selectCustom, the only failure path is showing the retry snackbar—there’s no way to route permanently denied users to openSettings(), unlike the existing permanently-denied handling for notification/storage/battery in lib/page/setting/widgets/permission_settings.dart.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/service/permission_service.dart` around lines 79 - 88, Change
requestAudioRead() in permission_service.dart to return the raw PermissionStatus
instead of a bool: keep the Platform.isAndroid and _androidSdkInt >= 33 branch
logic but return the PermissionStatus from Permission.audio.request() or
Permission.storage.request() directly (do not map to status.isGranted). Then
update callers (notably selectCustom in sound_settings/controller.dart where
_permissionService.requestAudioRead() is awaited) to handle the
PermissionStatus: check status.isGranted to proceed, and if
status.isPermanentlyDenied show the snackbar hint and call openSettings()
(matching the UX in permission_settings.dart); also update any other call sites
that assumed a bool to use status.isGranted/isPermanentlyDenied accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements