Sync's UX fixes #1189
Sync's UX fixes #1189
Conversation
WalkthroughThe pull request introduces several modifications across multiple files related to memory synchronization features. Key changes include updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
app/lib/pages/memories/synced_memories_page.dart (1)
Line range hint
17-23: Consider handling loading statesGiven that MemoryProvider now tracks memory fetching state with
isFetchingMemories, consider showing a loading indicator when memories are being fetched. This would provide better feedback to users during data loading.return SingleChildScrollView( child: Column( mainAxisSize: MainAxisSize.min, children: [ + if (memoryProvider.isFetchingMemories) + const Padding( + padding: EdgeInsets.all(16.0), + child: CircularProgressIndicator(color: Colors.white), + ), + if (!memoryProvider.isFetchingMemories) ...[ MemoriesListWidget( memories: memoryProvider.syncedMemoriesPointers .where((e) => e.type == SyncedMemoryType.updatedMemory) .toList(), title: 'Updated Memories', showReprocess: true, ), MemoriesListWidget( memories: memoryProvider.syncedMemoriesPointers .where((e) => e.type == SyncedMemoryType.newMemory) .toList(), title: 'New Memories', showReprocess: false, ), + ], ], ), );app/lib/providers/memory_provider.dart (1)
Line range hint
377-392: Improve error handling in getSyncedMemoriesDataWhile the fetching state management is good, the error handling could be improved:
- Use proper logging instead of print
- Consider propagating the error to allow upstream handling
- Add more specific error handling for different types of failures
Consider this improvement:
Future getSyncedMemoriesData(SyncLocalFilesResponse syncResult) async { List<dynamic> newMemories = syncResult.newMemoryIds; List<dynamic> updatedMemories = syncResult.updatedMemoryIds; setIsFetchingMemories(true); List<Future<ServerMemory?>> newMemoriesFutures = newMemories.map((item) => getMemoryDetails(item)).toList(); List<Future<ServerMemory?>> updatedMemoriesFutures = updatedMemories.map((item) => getMemoryDetails(item)).toList(); var syncedMemories = {'new_memories': [], 'updated_memories': []}; try { final newMemoriesResponses = await Future.wait(newMemoriesFutures); syncedMemories['new_memories'] = newMemoriesResponses; final updatedMemoriesResponses = await Future.wait(updatedMemoriesFutures); syncedMemories['updated_memories'] = updatedMemoriesResponses; addSyncedMemoriesToGroupedMemories(syncedMemories); setIsFetchingMemories(false); - } catch (e) { - print('Error during API calls: $e'); + } catch (e, stackTrace) { + debugPrint('Error during memory sync: $e\n$stackTrace'); + // Consider showing a user-friendly error message + rethrow; // Allow upstream error handling } finally { setIsFetchingMemories(false); } }app/lib/pages/memories/sync_page.dart (1)
341-347: Consider increasing loading indicator sizeThe current 20x20 size might be too small for optimal visibility. Consider increasing it to match common Flutter loading indicator sizes (24x24 or 28x28).
- height: 20, - width: 20, + height: 24, + width: 24,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- app/lib/pages/memories/sync_page.dart (3 hunks)
- app/lib/pages/memories/synced_memories_page.dart (3 hunks)
- app/lib/pages/memories/widgets/synced_memory_list_item.dart (2 hunks)
- app/lib/providers/memory_provider.dart (4 hunks)
🔇 Additional comments (5)
app/lib/pages/memories/synced_memories_page.dart (1)
48-48: LGTM: Type safety improvementThe change from nullable to non-nullable list is safe and improves type safety, as the parameter is required and the parent widget always provides a non-null list.
app/lib/pages/memories/widgets/synced_memory_list_item.dart (2)
8-8: LGTM: Import added for string extension utilityThe import is correctly placed and supports the new string decoding functionality.
103-104: Verify business logic: Reprocessing now available for discarded memoriesThe condition has been expanded to allow reprocessing of discarded memories. Please confirm if this is the intended behavior:
- Previously: Only showed when
showReprocesswas true- Now: Shows when either
showReprocessis true ORmemory.discardedis true#!/bin/bash # Look for related logic around discarded memories and reprocessing rg "discarded.*reprocess|reprocess.*discarded" --type dart # Check for any tests related to this functionality rg -l "discarded.*test|test.*reprocess" --type dartapp/lib/providers/memory_provider.dart (1)
40-40: LGTM: Clean implementation of fetching state managementThe new property and setter method follow Flutter/Dart conventions and properly notify listeners of state changes.
Also applies to: 469-472
app/lib/pages/memories/sync_page.dart (1)
283-303: Verify the safety of allowing navigation during syncWhile improving UX by allowing users to navigate away during sync, we should ensure:
- The sync process continues in the background
- No data inconsistencies occur if the user navigates away
- The sync state is properly managed at the application level
✅ Verification successful
Navigation during sync is safe to allow
The code analysis reveals that sync state is properly managed at the application level:
- The sync process is handled by
WalServiceandWalSyncswhich operate independently of the UI- The sync state (
isSyncing) is managed in multiple layers:
MemoryProviderfor UI state updatesWalServicefor actual sync operations- The sync process continues in the background through:
- Independent service architecture
- Proper state management with listeners
- Robust error handling and progress tracking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if sync state is properly managed at app level # Look for sync state management in providers and services # Check for sync state management in providers rg -l "isSyncing" --type dart # Check for background sync handling rg "sync.*background|background.*sync" --type dartLength of output: 186
Script:
#!/bin/bash # Check implementation of isSyncing in memory provider ast-grep --pattern 'class MemoryProvider $_$ { $$$ bool isSyncing = false; $$$ }' # Check how isSyncing is set/managed rg "isSyncing\s*=\s*" -A 2 --type dart # Check sync implementation in wals service rg "sync" -A 5 app/lib/services/wals.dart # Look for any sync completion handlers rg "onSync(Complete|Finished|Done)" --type dartLength of output: 12907
| if (memories.isEmpty) { | ||
| return const SizedBox(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving empty state feedback
Instead of showing nothing when there are no memories, consider displaying a message to inform users about the empty state. This would improve user experience by making it clear that there are no memories rather than leaving users wondering if something went wrong.
if (memories.isEmpty) {
- return const SizedBox();
+ return Padding(
+ padding: const EdgeInsets.symmetric(vertical: 8.0),
+ child: Text(
+ 'No $title available',
+ style: const TextStyle(
+ color: Colors.white70,
+ fontSize: 16,
+ ),
+ ),
+ );
}Committable suggestion was skipped due to low confidence.
| memory.transcriptSegments.first.text.decodeString, | ||
| style: Theme.of(context).textTheme.titleLarge, | ||
| maxLines: 1, | ||
| ) | ||
| : Text( | ||
| memory.structured.title, | ||
| memory.structured.title.decodeString, | ||
| style: Theme.of(context).textTheme.titleLarge, | ||
| maxLines: 1, |
There was a problem hiding this comment.
💡 Codebase verification
String decoding implementation is consistent but needs standardization
The verification reveals:
decodeStringextension method is properly implemented inapp/lib/widgets/extensions/string.dartusing UTF-8 decoding- Most text display instances use
decodeStringconsistently for memory titles, transcripts, and content - However, there's an inconsistent pattern where
tryDecodeTextis used inai_message.dartthat implements the same functionality
Action items:
- Replace
tryDecodeTextinapp/lib/pages/chat/widgets/ai_message.dartwith the standarddecodeStringextension - Add
decodeStringto these missing instances:app/lib/pages/memory_detail/share.dart:structured.titleusageapp/lib/pages/memory_detail/widgets.dart: One instance ofprovider.memory.structured.titleapp/lib/pages/chat/widgets/ai_message.dart:modifiedDetails.structured.title
🔗 Analysis chain
Verify consistent string decoding across the application
The addition of decodeString suggests there might be encoded characters in the memory content. Let's verify this pattern is consistently applied where needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of memory transcript or title usage that might need decoding
rg -l "transcriptSegments.*text|structured\.title" --type dart
# Check the string extension implementation
rg -l "decodeString" --type dart
Length of output: 114
Script:
#!/bin/bash
# Let's try a more focused search for transcript and title usage
rg "transcriptSegments.*?text|structured\.title" --type dart -A 2
# Search for the decodeString extension method definition
ast-grep --pattern 'extension $_ on String { $$$decodeString$$$ }'
Length of output: 3785
Script:
#!/bin/bash
# Let's find the decodeString implementation since we see it's being used
rg "decodeString" --type dart -B 2 -A 2
# Also check for tryDecodeText since we saw it in the results
rg "tryDecodeText" --type dart -B 2 -A 2
Length of output: 10229
| ? memoryProvider.isFetchingMemories | ||
| ? const Padding( | ||
| padding: EdgeInsets.all(8.0), | ||
| child: Row( | ||
| mainAxisAlignment: MainAxisAlignment.center, | ||
| children: [ | ||
| SizedBox( | ||
| height: 20, | ||
| width: 20, | ||
| child: CircularProgressIndicator( | ||
| color: Colors.white, | ||
| ), | ||
| ), | ||
| SizedBox(width: 12), | ||
| Text( | ||
| 'Finalizing your memories', | ||
| style: TextStyle(color: Colors.white, fontSize: 16), | ||
| textAlign: TextAlign.center, | ||
| ), | ||
| ], | ||
| ), | ||
| ) | ||
| : const Padding( | ||
| padding: EdgeInsets.all(8.0), | ||
| child: Text( | ||
| 'Syncing Memories\nPlease don\'t close the app', | ||
| style: TextStyle(color: Colors.white, fontSize: 16), | ||
| textAlign: TextAlign.center, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract text styles to constants
Consider extracting the text styles to constants for better maintainability and consistency. This applies to both the "Finalizing your memories" and "Syncing Memories" text styles.
// Add to a styles.dart file or similar
class SyncPageStyles {
static const TextStyle statusText = TextStyle(
color: Colors.white,
fontSize: 16,
);
}Then update the usage:
- style: TextStyle(color: Colors.white, fontSize: 16),
+ style: SyncPageStyles.statusText,
| @@ -280,26 +280,27 @@ class _SyncPageState extends State<SyncPage> { | |||
| } | |||
|
|
|||
| return PopScope( | |||
| canPop: !Provider.of<MemoryProvider>(context, listen: false).isSyncing, | |||
| canPop: true, | |||
| onPopInvoked: (didPop) { | |||
| var provider = Provider.of<MemoryProvider>(context, listen: false); | |||
There was a problem hiding this comment.
We allow pop, btw the sync process should be stop if users exited the sync page. We haven't had the background syncs yet 🫣
There was a problem hiding this comment.
The sync state is handled by provider and provider isn't disposed until the user closes the app. So the sync should keep running even if the user is on another screen. That is way changed the text as well (to only mention not to close the app)
There was a problem hiding this comment.
Lmk if I should revert it
There was a problem hiding this comment.
would be great if we could indicate it somewhere that the sync is running, and when the provider closes, we could gratefully stop the sync(e.g notify sd card to stop sync) @mdmohsin7 any idea?
There was a problem hiding this comment.
We can definitely have some sync in progress indicator maybe through a notification without cluttering the existing UI. What do you think? @beastoin
Provider is only disposed when the app is restarted or the app is closed completely
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (4)
app/lib/utils/other/time_utils.dart (1)
1-29: Consider adding unit tests.Given this is a utility function that handles various time ranges and formatting cases, it would be beneficial to have comprehensive unit tests.
Would you like me to generate unit tests covering various scenarios including edge cases?
app/lib/pages/memories/widgets/local_sync.dart (2)
Line range hint
64-71: Consider extracting magic numbers as named constants.The threshold of 120 seconds should be extracted as a named constant at the class level for better maintainability and clarity.
class _LocalSyncWidgetState extends State<LocalSyncWidget> { + static const int _SYNC_THRESHOLD_SECONDS = 120; LocalSyncStatus? _status; Timer? _missSecondsInEstTimer; bool _missSecondsInEstTimerEnabled = false; int _missSeconds = 0; @override void build(BuildContext context) { return Consumer2<MemoryProvider, CaptureProvider>(builder: (context, provider, captureProvider, child) { var previousStatus = _status; - if (provider.missingWalsInSeconds >= 120) { + if (provider.missingWalsInSeconds >= _SYNC_THRESHOLD_SECONDS) {
Line range hint
32-42: Optimize timer management.The timer is created in
initStateand only cancelled indispose. Consider pausing/resuming the timer based on the widget's status to improve performance.@override void initState() { super.initState(); + _startTimerIfNeeded(); + } + void _startTimerIfNeeded() { + _missSecondsInEstTimer?.cancel(); _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) { if (_missSecondsInEstTimerEnabled) { setState(() { _missSeconds++; }); } }); } + void _stopTimer() { + _missSecondsInEstTimer?.cancel(); + _missSecondsInEstTimer = null; + }app/lib/pages/memories/sync_page.dart (1)
289-291: Remove commented out codeThe commented out
_toggleAnimationcalls should be removed as they're no longer used.- // _toggleAnimation(); await memoryProvider.syncWals(); - // _toggleAnimation();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- app/lib/pages/memories/page.dart (0 hunks)
- app/lib/pages/memories/sync_page.dart (3 hunks)
- app/lib/pages/memories/widgets/local_sync.dart (2 hunks)
- app/lib/utils/other/time_utils.dart (1 hunks)
💤 Files with no reviewable changes (1)
- app/lib/pages/memories/page.dart
🔇 Additional comments (3)
app/lib/pages/memories/widgets/local_sync.dart (1)
118-118: Verify type conversion in secondsToHumanReadable call.Converting
_missSecondsto string before passing tosecondsToHumanReadableseems unnecessary and might indicate a type mismatch.#!/bin/bash # Check the function signature in the codebase rg "secondsToHumanReadable" --type dartapp/lib/pages/memories/sync_page.dart (2)
209-222: LGTM: Clean state management implementationThe state management implementation follows Flutter best practices with proper initialization and disposal of the animation controller.
440-467: LGTM: Well-implemented list widgetThe
WalsListWidgetimplementation is clean and efficient:
- Uses
SliverListfor better performance- Properly handles empty states
- Clean separation of grouping logic
app/lib/utils/other/time_utils.dart
Outdated
| if (seconds < 60) { | ||
| return '$seconds secs'; | ||
| } else if (seconds < 3600) { | ||
| var minutes = (seconds / 60).floor(); | ||
| var remainingSeconds = seconds % 60; | ||
| if (remainingSeconds == 0) { | ||
| return '$minutes mins'; | ||
| } else { | ||
| return '$minutes mins $remainingSeconds secs'; | ||
| } |
There was a problem hiding this comment.
Fix singular/plural forms for minutes and seconds.
The current implementation doesn't handle singular forms correctly (e.g., "1 mins" should be "1 min").
Apply this diff to fix the grammar:
if (seconds < 60) {
- return '$seconds secs';
+ return '$seconds ${seconds == 1 ? 'sec' : 'secs'}';
} else if (seconds < 3600) {
var minutes = (seconds / 60).floor();
var remainingSeconds = seconds % 60;
if (remainingSeconds == 0) {
- return '$minutes mins';
+ return '$minutes ${minutes == 1 ? 'min' : 'mins'}';
} else {
- return '$minutes mins $remainingSeconds secs';
+ return '$minutes ${minutes == 1 ? 'min' : 'mins'} $remainingSeconds ${remainingSeconds == 1 ? 'sec' : 'secs'}';
}📝 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.
| if (seconds < 60) { | |
| return '$seconds secs'; | |
| } else if (seconds < 3600) { | |
| var minutes = (seconds / 60).floor(); | |
| var remainingSeconds = seconds % 60; | |
| if (remainingSeconds == 0) { | |
| return '$minutes mins'; | |
| } else { | |
| return '$minutes mins $remainingSeconds secs'; | |
| } | |
| if (seconds < 60) { | |
| return '$seconds ${seconds == 1 ? 'sec' : 'secs'}'; | |
| } else if (seconds < 3600) { | |
| var minutes = (seconds / 60).floor(); | |
| var remainingSeconds = seconds % 60; | |
| if (remainingSeconds == 0) { | |
| return '$minutes ${minutes == 1 ? 'min' : 'mins'}'; | |
| } else { | |
| return '$minutes ${minutes == 1 ? 'min' : 'mins'} $remainingSeconds ${remainingSeconds == 1 ? 'sec' : 'secs'}'; | |
| } |
| @@ -0,0 +1,29 @@ | |||
| String secondsToHumanReadable(int seconds) { | |||
There was a problem hiding this comment.
Add input validation and documentation.
The function should validate its input and include documentation explaining its purpose and usage.
Apply this diff to add validation and documentation:
+/// Converts a duration in seconds to a human-readable string format.
+///
+/// Examples:
+/// - 30 -> "30 secs"
+/// - 90 -> "1 min 30 secs"
+/// - 3600 -> "1 hour"
+/// - 90000 -> "1 day 1 hour"
+///
+/// @param seconds The number of seconds to convert
+/// @throws ArgumentError if seconds is negative
String secondsToHumanReadable(int seconds) {
+ if (seconds < 0) {
+ throw ArgumentError('Seconds cannot be negative');
+ }📝 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.
| String secondsToHumanReadable(int seconds) { | |
| /// Converts a duration in seconds to a human-readable string format. | |
| /// | |
| /// Examples: | |
| /// - 30 -> "30 secs" | |
| /// - 90 -> "1 min 30 secs" | |
| /// - 3600 -> "1 hour" | |
| /// - 90000 -> "1 day 1 hour" | |
| /// | |
| /// @param seconds The number of seconds to convert | |
| /// @throws ArgumentError if seconds is negative | |
| String secondsToHumanReadable(int seconds) { | |
| if (seconds < 0) { | |
| throw ArgumentError('Seconds cannot be negative'); | |
| } |
app/lib/utils/other/time_utils.dart
Outdated
| } else if (seconds < 86400) { | ||
| var hours = (seconds / 3600).floor(); | ||
| var remainingMinutes = (seconds % 3600 / 60).floor(); | ||
| if (remainingMinutes == 0) { | ||
| return '$hours hours'; | ||
| } else { | ||
| return '$hours hours $remainingMinutes mins'; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Fix singular/plural forms for hours.
Similar to the previous issue, hour/hours needs proper handling.
Apply this diff to fix the grammar:
} else if (seconds < 86400) {
var hours = (seconds / 3600).floor();
var remainingMinutes = (seconds % 3600 / 60).floor();
if (remainingMinutes == 0) {
- return '$hours hours';
+ return '$hours ${hours == 1 ? 'hour' : 'hours'}';
} else {
- return '$hours hours $remainingMinutes mins';
+ return '$hours ${hours == 1 ? 'hour' : 'hours'} $remainingMinutes ${remainingMinutes == 1 ? 'min' : 'mins'}';
}📝 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.
| } else if (seconds < 86400) { | |
| var hours = (seconds / 3600).floor(); | |
| var remainingMinutes = (seconds % 3600 / 60).floor(); | |
| if (remainingMinutes == 0) { | |
| return '$hours hours'; | |
| } else { | |
| return '$hours hours $remainingMinutes mins'; | |
| } | |
| } else { | |
| } else if (seconds < 86400) { | |
| var hours = (seconds / 3600).floor(); | |
| var remainingMinutes = (seconds % 3600 / 60).floor(); | |
| if (remainingMinutes == 0) { | |
| return '$hours ${hours == 1 ? 'hour' : 'hours'}'; | |
| } else { | |
| return '$hours ${hours == 1 ? 'hour' : 'hours'} $remainingMinutes ${remainingMinutes == 1 ? 'min' : 'mins'}'; | |
| } | |
| } else { |
app/lib/utils/other/time_utils.dart
Outdated
| var days = (seconds / 86400).floor(); | ||
| var remainingHours = (seconds % 86400 / 3600).floor(); | ||
| if (remainingHours == 0) { | ||
| return '$days days'; | ||
| } else { | ||
| return '$days days $remainingHours hours'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix singular/plural forms for days.
Days also needs proper singular/plural handling.
Apply this diff to fix the grammar:
var days = (seconds / 86400).floor();
var remainingHours = (seconds % 86400 / 3600).floor();
if (remainingHours == 0) {
- return '$days days';
+ return '$days ${days == 1 ? 'day' : 'days'}';
} else {
- return '$days days $remainingHours hours';
+ return '$days ${days == 1 ? 'day' : 'days'} $remainingHours ${remainingHours == 1 ? 'hour' : 'hours'}';
}📝 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.
| var days = (seconds / 86400).floor(); | |
| var remainingHours = (seconds % 86400 / 3600).floor(); | |
| if (remainingHours == 0) { | |
| return '$days days'; | |
| } else { | |
| return '$days days $remainingHours hours'; | |
| } | |
| } | |
| var days = (seconds / 86400).floor(); | |
| var remainingHours = (seconds % 86400 / 3600).floor(); | |
| if (remainingHours == 0) { | |
| return '$days ${days == 1 ? 'day' : 'days'}'; | |
| } else { | |
| return '$days ${days == 1 ? 'day' : 'days'} $remainingHours ${remainingHours == 1 ? 'hour' : 'hours'}'; | |
| } | |
| } |
| import 'package:friend_private/pages/memories/sync_page.dart'; | ||
| import 'package:friend_private/pages/sdcard/sdcard_transfer_progress.dart'; |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused import of sdcard_transfer_progress.dart
The sdcard_transfer_progress.dart import is not being used in local_sync.dart. The secondsToHumanReadable function that's being used in local_sync.dart appears to be defined in multiple places, but based on the import statements, it's likely coming from utils/other/time_utils.dart which should already be available through other imports.
🔗 Analysis chain
Verify usage of sdcard_transfer_progress import.
The newly added import appears to be unused in the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the imported file's components are used
rg "SdcardTransferProgress" app/lib/pages/memories/widgets/local_sync.dart
Length of output: 775
Script:
#!/bin/bash
# Let's check the full content of local_sync.dart to understand the usage
cat app/lib/pages/memories/widgets/local_sync.dart
Length of output: 4316
Script:
#!/bin/bash
# Let's check the content of sdcard_transfer_progress.dart to see what it exports
cat app/lib/pages/sdcard/sdcard_transfer_progress.dart
# Also check where secondsToHumanReadable is defined
rg "secondsToHumanReadable" --type dart
Length of output: 6569
| border: const GradientBoxBorder( | ||
| gradient: LinearGradient(colors: [ | ||
| Color.fromARGB(127, 208, 208, 208), | ||
| Color.fromARGB(127, 188, 99, 121), | ||
| Color.fromARGB(127, 86, 101, 182), | ||
| Color.fromARGB(127, 126, 190, 236) | ||
| ]), | ||
| width: 2, | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicate gradient border decoration
The gradient border decoration is duplicated. Consider extracting it to a reusable constant or widget.
// Add to a constants file or within the class
const _gradientBorderDecoration = GradientBoxBorder(
gradient: LinearGradient(
colors: [
Color.fromARGB(127, 208, 208, 208),
Color.fromARGB(127, 188, 99, 121),
Color.fromARGB(127, 86, 101, 182),
Color.fromARGB(127, 126, 190, 236)
]
),
width: 2,
);Also applies to: 385-393
| onPressed: () async { | ||
| if (context.read<ConnectivityProvider>().isConnected) { | ||
| // _toggleAnimation(); | ||
| await memoryProvider.syncWals(); | ||
| // _toggleAnimation(); | ||
| } else { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Internet connection is required to sync memories'), | ||
| backgroundColor: Colors.red, | ||
| ), | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Add error handling for sync operation
The sync operation should handle potential errors and show appropriate error messages to the user.
onPressed: () async {
if (context.read<ConnectivityProvider>().isConnected) {
- await memoryProvider.syncWals();
+ try {
+ await memoryProvider.syncWals();
+ } catch (e) {
+ if (mounted) {
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(
+ content: Text('Failed to sync memories: ${e.toString()}'),
+ backgroundColor: Colors.red,
+ ),
+ );
+ }
+ }
} else {📝 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.
| onPressed: () async { | |
| if (context.read<ConnectivityProvider>().isConnected) { | |
| // _toggleAnimation(); | |
| await memoryProvider.syncWals(); | |
| // _toggleAnimation(); | |
| } else { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| const SnackBar( | |
| content: Text('Internet connection is required to sync memories'), | |
| backgroundColor: Colors.red, | |
| ), | |
| ); | |
| } | |
| }, | |
| onPressed: () async { | |
| if (context.read<ConnectivityProvider>().isConnected) { | |
| // _toggleAnimation(); | |
| try { | |
| await memoryProvider.syncWals(); | |
| } catch (e) { | |
| if (mounted) { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar( | |
| content: Text('Failed to sync memories: ${e.toString()}'), | |
| backgroundColor: Colors.red, | |
| ), | |
| ); | |
| } | |
| } | |
| // _toggleAnimation(); | |
| } else { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| const SnackBar( | |
| content: Text('Internet connection is required to sync memories'), | |
| backgroundColor: Colors.red, | |
| ), | |
| ); | |
| } | |
| }, |
| bool _handleScrollNotification(ScrollNotification notification) { | ||
| if (notification.depth == 0) { | ||
| if (notification is UserScrollNotification) { | ||
| final UserScrollNotification userScroll = notification; | ||
| switch (userScroll.direction) { | ||
| case ScrollDirection.forward: | ||
| if (userScroll.metrics.maxScrollExtent != userScroll.metrics.minScrollExtent) { | ||
| _hideFabAnimation.forward(); | ||
| } | ||
|
|
||
| var date = keys[idx]; | ||
| List<Wal> wals = groupedWals[date] ?? []; | ||
| return Column( | ||
| mainAxisSize: MainAxisSize.min, | ||
| children: [ | ||
| if (index == 0) const SizedBox(height: 16), | ||
| SyncWalGroupWidget( | ||
| isFirst: index == 0, | ||
| wals: wals, | ||
| date: date, | ||
| ), | ||
| ], | ||
| ); | ||
| }, | ||
| ), | ||
| ); | ||
| break; | ||
| case ScrollDirection.reverse: | ||
| if (userScroll.metrics.maxScrollExtent != userScroll.metrics.minScrollExtent) { | ||
| _hideFabAnimation.reverse(); | ||
| } | ||
| break; | ||
| case ScrollDirection.idle: | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying scroll notification handler
The scroll handling logic can be simplified by flattening conditions and extracting the scroll extent check.
bool _handleScrollNotification(ScrollNotification notification) {
- if (notification.depth == 0) {
- if (notification is UserScrollNotification) {
- final UserScrollNotification userScroll = notification;
- switch (userScroll.direction) {
- case ScrollDirection.forward:
- if (userScroll.metrics.maxScrollExtent != userScroll.metrics.minScrollExtent) {
- _hideFabAnimation.forward();
- }
- break;
- case ScrollDirection.reverse:
- if (userScroll.metrics.maxScrollExtent != userScroll.metrics.minScrollExtent) {
- _hideFabAnimation.reverse();
- }
- break;
- case ScrollDirection.idle:
- break;
- }
- }
- }
+ if (notification.depth != 0 || notification is! UserScrollNotification) {
+ return false;
+ }
+
+ final hasScrollExtent = notification.metrics.maxScrollExtent != notification.metrics.minScrollExtent;
+ if (!hasScrollExtent) {
+ return false;
+ }
+
+ switch (notification.direction) {
+ case ScrollDirection.forward:
+ _hideFabAnimation.forward();
+ case ScrollDirection.reverse:
+ _hideFabAnimation.reverse();
+ case ScrollDirection.idle:
+ break;
+ }
return false;
}📝 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.
| bool _handleScrollNotification(ScrollNotification notification) { | |
| if (notification.depth == 0) { | |
| if (notification is UserScrollNotification) { | |
| final UserScrollNotification userScroll = notification; | |
| switch (userScroll.direction) { | |
| case ScrollDirection.forward: | |
| if (userScroll.metrics.maxScrollExtent != userScroll.metrics.minScrollExtent) { | |
| _hideFabAnimation.forward(); | |
| } | |
| var date = keys[idx]; | |
| List<Wal> wals = groupedWals[date] ?? []; | |
| return Column( | |
| mainAxisSize: MainAxisSize.min, | |
| children: [ | |
| if (index == 0) const SizedBox(height: 16), | |
| SyncWalGroupWidget( | |
| isFirst: index == 0, | |
| wals: wals, | |
| date: date, | |
| ), | |
| ], | |
| ); | |
| }, | |
| ), | |
| ); | |
| break; | |
| case ScrollDirection.reverse: | |
| if (userScroll.metrics.maxScrollExtent != userScroll.metrics.minScrollExtent) { | |
| _hideFabAnimation.reverse(); | |
| } | |
| break; | |
| case ScrollDirection.idle: | |
| break; | |
| } | |
| } | |
| } | |
| return false; | |
| } | |
| bool _handleScrollNotification(ScrollNotification notification) { | |
| if (notification.depth != 0 || notification is! UserScrollNotification) { | |
| return false; | |
| } | |
| final hasScrollExtent = notification.metrics.maxScrollExtent != notification.metrics.minScrollExtent; | |
| if (!hasScrollExtent) { | |
| return false; | |
| } | |
| switch (notification.direction) { | |
| case ScrollDirection.forward: | |
| _hideFabAnimation.forward(); | |
| case ScrollDirection.reverse: | |
| _hideFabAnimation.reverse(); | |
| case ScrollDirection.idle: | |
| break; | |
| } | |
| return false; | |
| } |
| Map<DateTime, List<Wal>> _groupWalsByDate(List<Wal> wals) { | ||
| var groupedWals = <DateTime, List<Wal>>{}; | ||
| for (var wal in wals) { | ||
| var createdAt = DateTime.fromMillisecondsSinceEpoch(wal.timerStart * 1000).toLocal(); | ||
| var date = DateTime(createdAt.year, createdAt.month, createdAt.day, createdAt.hour); | ||
| if (!groupedWals.containsKey(date)) { | ||
| groupedWals[date] = []; | ||
| } | ||
| groupedWals[date]?.add(wal); | ||
| } | ||
| for (final date in groupedWals.keys) { | ||
| groupedWals[date]?.sort((a, b) => b.timerStart.compareTo(a.timerStart)); | ||
| } | ||
| return groupedWals; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving grouping logic to a utility file
The _groupWalsByDate function is a utility function that could be reused across the app. Consider:
- Moving it to a utility file
- Adding documentation to explain the grouping logic
+/// Groups a list of Wal objects by their creation date, using hour-level precision.
+/// The resulting map contains DateTime keys (truncated to hour) and sorted lists of Wals.
+/// The Wals in each group are sorted in descending order by their timer start time.
Map<DateTime, List<Wal>> _groupWalsByDate(List<Wal> wals) {Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (4)
app/lib/providers/memory_provider.dart (3)
40-41: Consider adding documentation for new state properties.
The new state properties look good, but would benefit from documentation explaining their purpose:
multipleSyncs: Tracks concurrent sync operationsisFetchingMemories: Tracks memory fetching state
+ /// Stack to track multiple concurrent sync operations
List<bool> multipleSyncs = [];
+ /// Indicates whether memories are currently being fetched
bool isFetchingMemories = false;375-389: Use debugPrint consistently for logging.
The implementation looks good, but for consistency with line 376, replace print with debugPrint:
- print('Synced memories: ${res.newMemoryIds} ${res.updatedMemoryIds}');
+ debugPrint('Synced memories: ${res.newMemoryIds} ${res.updatedMemoryIds}');Line range hint 399-414: Use debugPrint for error logging.
The fetching state management looks good, but for consistency:
- print('Error during API calls: $e');
+ debugPrint('Error during API calls: $e');app/lib/pages/memories/sync_page.dart (1)
309-324: Enhance the warning message for better user guidance.
The current warning message could be more informative about the sync process and its implications.
- child: const ListTile(
- leading: Icon(
- Icons.warning,
- color: Colors.yellow,
- ),
- title: Text('Please do not close the app while sync is in progress'),
- ),
+ child: const ListTile(
+ leading: Icon(
+ Icons.warning,
+ color: Colors.yellow,
+ ),
+ title: Text('Please keep the app open'),
+ subtitle: Text('Closing the app will interrupt the sync process and may result in incomplete data'),
+ ),📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- app/lib/backend/preferences.dart (1 hunks)
- app/lib/pages/memories/sync_page.dart (4 hunks)
- app/lib/providers/memory_provider.dart (5 hunks)
- app/lib/services/wals.dart (4 hunks)
- app/lib/utils/other/time_utils.dart (1 hunks)
🔇 Additional comments (8)
app/lib/utils/other/time_utils.dart (1)
1-38: Add input validation and documentation.
The previous review comment about adding input validation and documentation is still valid and should be addressed.
app/lib/backend/preferences.dart (1)
126-126: Verify the impact of changing the default sync behavior.
Changing the default value of localSyncEnabled from false to true is a breaking change that affects the default synchronization behavior for new installations or reset preferences. This change needs careful consideration:
- Ensure this aligns with the broader sync-related changes mentioned in the PR
- Verify the impact on the
MemoryProviderand sync functionality - Consider documenting this change in the migration guide if applicable
Let's verify the usage and dependencies:
✅ Verification successful
Based on the search results, I can see that localSyncEnabled is used in several key places:
- In
CaptureProviderto determine if local sync should be enabled during capture - In
DeveloperModeProvideras a toggle setting in developer mode - The default value change affects new installations and reset preferences
Let's verify the impact by checking if there are any conditional behaviors or dependencies on this setting:
Let's check one more thing to understand how this setting affects the actual sync behavior:
Let's check one final thing to understand where this setting is actually used in the sync logic:
Based on the comprehensive analysis of the codebase, I can now provide a final response:
The default value change for localSyncEnabled is safe and intentional
The change from false to true as the default value for localSyncEnabled is safe because:
- The setting is only used in a very specific scenario - it enables local sync for opus codec audio after 15 seconds of device connection
- It's primarily a developer mode setting that can be toggled in the developer settings UI
- The actual sync behavior has proper guards and checks beyond just this flag:
- Requires opus codec
- Device must be connected for >15 seconds
- WAL (Write-Ahead Logging) support must be verified
- Existing installations won't be affected as they already have the preference set
The change aligns with making local sync enabled by default for better data reliability, while still allowing it to be disabled through developer settings if needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of localSyncEnabled
echo "=== Direct usage of localSyncEnabled ==="
rg "localSyncEnabled" -B 2 -A 2
# Search for sync-related functionality that might be affected
echo "=== Related sync functionality ==="
rg "sync|synchroniz(e|ation)" --type dart -B 2 -A 2
Length of output: 245924
Script:
#!/bin/bash
# Search for conditional logic using localSyncEnabled
echo "=== Conditional logic using localSyncEnabled ==="
rg "if.*localSyncEnabled" -B 2 -A 2
# Search for any assignments to localSyncEnabled
echo "=== Assignments to localSyncEnabled ==="
rg "localSyncEnabled\s*=\s*" -B 2 -A 2
Length of output: 2366
Script:
#!/bin/bash
# Search for where localSyncEnabled is used in conditional logic
echo "=== Usage of localSyncEnabled in sync logic ==="
rg -B 5 -A 5 "localSyncEnabled.*\?"
# Search for sync-related methods that might use this flag
echo "=== Sync methods and localSyncEnabled ==="
rg -B 5 -A 5 "sync.*local.*enabled|local.*sync.*enabled"
Length of output: 1056
Script:
#!/bin/bash
# Search for files containing both sync and localSyncEnabled references
echo "=== Files with both sync and localSyncEnabled ==="
rg -l "sync" | xargs rg -l "localSyncEnabled"
# Look for actual usage of localSyncEnabled in capture logic
echo "=== Usage in capture logic ==="
rg -B 10 -A 10 "localSyncEnabled" app/lib/providers/capture_provider.dart
Length of output: 2148
app/lib/providers/memory_provider.dart (2)
Line range hint 360-373: LGTM! Improved sync state management.
The sync state management has been enhanced with proper state transitions using setSyncCompleted.
391-394: LGTM! Clean state management implementation.
The method properly updates the sync completion state and notifies listeners.
app/lib/pages/memories/sync_page.dart (2)
187-200: LGTM: Animation controller setup is correct.
The initialization and disposal of the animation controller follow Flutter best practices.
435-463: LGTM: WalsListWidget implementation is clean and efficient.
The widget properly uses SliverList for efficient list rendering and correctly handles the grouping of Wals.
app/lib/services/wals.dart (2)
34-34: Addition of syncWal method to IWalSync interface
The syncWal method has been added to the IWalSync interface, allowing synchronization of individual Wal instances. This change enhances the flexibility and granularity of the synchronization process.
898-904: Ensure all WalStorage types are properly handled
The syncWal method delegates to _sdcardSync if wal.storage is WalStorage.sdcard and to _phoneSync otherwise. Since WalStorage includes mem, disk, and sdcard, ensure that _phoneSync correctly handles both WalStorage.mem and WalStorage.disk, or consider explicitly handling each storage type to prevent unexpected behaviors.
| } | ||
| return '$minutes mins'; | ||
| } else { | ||
| return '$minutes mins $remainingSeconds secs'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize string concatenation using interpolation.
Consider using string interpolation with conditional expressions for more concise and maintainable code.
Example improvement for line 13:
- return '$minutes mins $remainingSeconds secs';
+ return '$minutes ${minutes == 1 ? 'min' : 'mins'} $remainingSeconds ${remainingSeconds == 1 ? 'sec' : 'secs'}';Also applies to: 24-24, 35-35
| // convert seconds to hh:mm:ss format | ||
| String secondsToHMS(int seconds) { | ||
| var hours = (seconds / 3600).floor(); | ||
| var minutes = (seconds % 3600 / 60).floor(); | ||
| var remainingSeconds = seconds % 60; | ||
| return '$hours:$minutes:$remainingSeconds'; | ||
| } |
There was a problem hiding this comment.
Fix formatting and add validation in secondsToHMS.
The function has several issues:
- Missing input validation
- Numbers aren't zero-padded (e.g., "9:5:0" instead of "09:05:00")
- Comment should be in dartdoc format
Apply this diff to fix the issues:
-// convert seconds to hh:mm:ss format
+/// Converts seconds to HH:MM:SS format.
+///
+/// Example: 3665 seconds becomes "01:01:05"
+///
+/// @param seconds The number of seconds to convert
+/// @throws ArgumentError if seconds is negative
String secondsToHMS(int seconds) {
+ if (seconds < 0) {
+ throw ArgumentError('Seconds cannot be negative');
+ }
var hours = (seconds / 3600).floor();
var minutes = (seconds % 3600 / 60).floor();
var remainingSeconds = seconds % 60;
- return '$hours:$minutes:$remainingSeconds';
+ return '${hours.toString().padLeft(2, '0')}:${minutes.toString().padLeft(2, '0')}:${remainingSeconds.toString().padLeft(2, '0')}';
}📝 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.
| // convert seconds to hh:mm:ss format | |
| String secondsToHMS(int seconds) { | |
| var hours = (seconds / 3600).floor(); | |
| var minutes = (seconds % 3600 / 60).floor(); | |
| var remainingSeconds = seconds % 60; | |
| return '$hours:$minutes:$remainingSeconds'; | |
| } | |
| /// Converts seconds to HH:MM:SS format. | |
| /// | |
| /// Example: 3665 seconds becomes "01:01:05" | |
| /// | |
| /// @param seconds The number of seconds to convert | |
| /// @throws ArgumentError if seconds is negative | |
| String secondsToHMS(int seconds) { | |
| if (seconds < 0) { | |
| throw ArgumentError('Seconds cannot be negative'); | |
| } | |
| var hours = (seconds / 3600).floor(); | |
| var minutes = (seconds % 3600 / 60).floor(); | |
| var remainingSeconds = seconds % 60; | |
| return '${hours.toString().padLeft(2, '0')}:${minutes.toString().padLeft(2, '0')}:${remainingSeconds.toString().padLeft(2, '0')}'; | |
| } |
| void appendMultipleSyncs(bool value) { | ||
| setIsSyncing(true); | ||
| multipleSyncs.add(value); | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| void removeMultipleSyncs() { | ||
| if (multipleSyncs.isNotEmpty) { | ||
| multipleSyncs.removeLast(); | ||
| } else { | ||
| setIsSyncing(false); | ||
| setSyncCompleted(true); | ||
| } | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| void clearMultipleSyncs() { | ||
| multipleSyncs.clear(); | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| void setIsFetchingMemories(bool value) { | ||
| isFetchingMemories = value; | ||
| notifyListeners(); | ||
| } |
There was a problem hiding this comment.
Fix potential race condition in removeMultipleSyncs.
The sync state management methods look good overall, but there's a potential race condition in removeMultipleSyncs. When removing the last sync operation, it doesn't verify if there are other ongoing syncs before setting isSyncing to false.
void removeMultipleSyncs() {
if (multipleSyncs.isNotEmpty) {
multipleSyncs.removeLast();
+ if (multipleSyncs.isEmpty) {
+ setIsSyncing(false);
+ setSyncCompleted(true);
+ }
- } else {
- setIsSyncing(false);
- setSyncCompleted(true);
}
notifyListeners();
}📝 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.
| void appendMultipleSyncs(bool value) { | |
| setIsSyncing(true); | |
| multipleSyncs.add(value); | |
| notifyListeners(); | |
| } | |
| void removeMultipleSyncs() { | |
| if (multipleSyncs.isNotEmpty) { | |
| multipleSyncs.removeLast(); | |
| } else { | |
| setIsSyncing(false); | |
| setSyncCompleted(true); | |
| } | |
| notifyListeners(); | |
| } | |
| void clearMultipleSyncs() { | |
| multipleSyncs.clear(); | |
| notifyListeners(); | |
| } | |
| void setIsFetchingMemories(bool value) { | |
| isFetchingMemories = value; | |
| notifyListeners(); | |
| } | |
| void appendMultipleSyncs(bool value) { | |
| setIsSyncing(true); | |
| multipleSyncs.add(value); | |
| notifyListeners(); | |
| } | |
| void removeMultipleSyncs() { | |
| if (multipleSyncs.isNotEmpty) { | |
| multipleSyncs.removeLast(); | |
| if (multipleSyncs.isEmpty) { | |
| setIsSyncing(false); | |
| setSyncCompleted(true); | |
| } | |
| } | |
| notifyListeners(); | |
| } | |
| void clearMultipleSyncs() { | |
| multipleSyncs.clear(); | |
| notifyListeners(); | |
| } | |
| void setIsFetchingMemories(bool value) { | |
| isFetchingMemories = value; | |
| notifyListeners(); | |
| } |
| double calculateProgress(DateTime? startedAt, int eta) { | ||
| if (startedAt == null) { | ||
| return 0.0; | ||
| } | ||
| if (eta == 0) { | ||
| return 0.01; | ||
| } | ||
| final elapsed = DateTime.now().difference(startedAt).inSeconds; | ||
| final progress = elapsed / eta; | ||
| return progress.clamp(0.0, 1.0); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the progress calculation method.
The progress calculation logic is sound, but could be improved:
- Make the method static since it doesn't use instance state
- Use milliseconds for more precise progress calculation
- double calculateProgress(DateTime? startedAt, int eta) {
+ static double calculateProgress(DateTime? startedAt, int eta) {
if (startedAt == null) {
return 0.0;
}
if (eta == 0) {
return 0.01;
}
- final elapsed = DateTime.now().difference(startedAt).inSeconds;
+ final elapsed = DateTime.now().difference(startedAt).inMilliseconds / 1000.0;
final progress = elapsed / eta;
return progress.clamp(0.0, 1.0);
}📝 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.
| double calculateProgress(DateTime? startedAt, int eta) { | |
| if (startedAt == null) { | |
| return 0.0; | |
| } | |
| if (eta == 0) { | |
| return 0.01; | |
| } | |
| final elapsed = DateTime.now().difference(startedAt).inSeconds; | |
| final progress = elapsed / eta; | |
| return progress.clamp(0.0, 1.0); | |
| } | |
| static double calculateProgress(DateTime? startedAt, int eta) { | |
| if (startedAt == null) { | |
| return 0.0; | |
| } | |
| if (eta == 0) { | |
| return 0.01; | |
| } | |
| final elapsed = DateTime.now().difference(startedAt).inMilliseconds / 1000.0; | |
| final progress = elapsed / eta; | |
| return progress.clamp(0.0, 1.0); | |
| } |
| Map<DateTime, List<Wal>> _groupWalsByDate(List<Wal> wals) { | ||
| var groupedWals = <DateTime, List<Wal>>{}; | ||
| wals.sort((a, b) => b.timerStart.compareTo(a.timerStart)); | ||
| for (var wal in wals) { | ||
| var createdAt = DateTime.fromMillisecondsSinceEpoch(wal.timerStart * 1000).toLocal(); | ||
| var date = DateTime(createdAt.year, createdAt.month, createdAt.day, createdAt.hour); | ||
| if (!groupedWals.containsKey(date)) { | ||
| groupedWals[date] = []; | ||
| } | ||
| groupedWals[date]?.add(wal); | ||
| } | ||
| for (final date in groupedWals.keys) { | ||
| groupedWals[date]?.sort((a, b) => b.timerStart.compareTo(a.timerStart)); | ||
| } | ||
| return groupedWals; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the grouping operation.
The current implementation performs sorting twice: once for the entire list and then for each group. This could be optimized by sorting only once.
Map<DateTime, List<Wal>> _groupWalsByDate(List<Wal> wals) {
var groupedWals = <DateTime, List<Wal>>{};
- wals.sort((a, b) => b.timerStart.compareTo(a.timerStart));
for (var wal in wals) {
var createdAt = DateTime.fromMillisecondsSinceEpoch(wal.timerStart * 1000).toLocal();
var date = DateTime(createdAt.year, createdAt.month, createdAt.day, createdAt.hour);
if (!groupedWals.containsKey(date)) {
groupedWals[date] = [];
}
groupedWals[date]?.add(wal);
}
- for (final date in groupedWals.keys) {
- groupedWals[date]?.sort((a, b) => b.timerStart.compareTo(a.timerStart));
+ // Sort all groups at once using a more efficient approach
+ groupedWals.values.forEach((list) {
+ list.sort((a, b) => b.timerStart.compareTo(a.timerStart));
+ });
}
return groupedWals;
}Committable suggestion was skipped due to low confidence.
| walToSync.syncEtaSeconds = DateTime.now().difference(walToSync.syncStartedAt!).inSeconds * | ||
| (walToSync.storageTotalBytes - wal.storageOffset) ~/ | ||
| (walToSync.storageOffset - storageOffsetStarts); |
There was a problem hiding this comment.
Potential division by zero in ETA calculation
In the calculation of syncEtaSeconds, if walToSync.storageOffset equals storageOffsetStarts, the denominator becomes zero, leading to a division by zero exception.
Please add a check to prevent division by zero:
var offsetProgress = walToSync.storageOffset - storageOffsetStarts;
+ if (offsetProgress > 0) {
walToSync.syncEtaSeconds = DateTime.now().difference(walToSync.syncStartedAt!).inSeconds *
(walToSync.storageTotalBytes - walToSync.storageOffset) ~/
- (walToSync.storageOffset - storageOffsetStarts);
+ offsetProgress;
+ } else {
+ walToSync.syncEtaSeconds = null; // or handle accordingly
+ }📝 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.
| walToSync.syncEtaSeconds = DateTime.now().difference(walToSync.syncStartedAt!).inSeconds * | |
| (walToSync.storageTotalBytes - wal.storageOffset) ~/ | |
| (walToSync.storageOffset - storageOffsetStarts); | |
| var offsetProgress = walToSync.storageOffset - storageOffsetStarts; | |
| if (offsetProgress > 0) { | |
| walToSync.syncEtaSeconds = DateTime.now().difference(walToSync.syncStartedAt!).inSeconds * | |
| (walToSync.storageTotalBytes - walToSync.storageOffset) ~/ | |
| offsetProgress; | |
| } else { | |
| walToSync.syncEtaSeconds = null; // or handle accordingly | |
| } |
|
|
||
| @override | ||
| Future<SyncLocalFilesResponse?> syncWal({required Wal wal, IWalSyncProgressListener? progress}) async { | ||
| var walToSync = _wals.where((w) => w == wal).toList().first; |
There was a problem hiding this comment.
Possible exception when retrieving Wal from list
Using first on a list without checking if it is empty can throw a StateError if wal is not found in _wals. It's safer to check if the list is not empty before accessing first.
Consider modifying the code to handle the case where the wal is not found:
- var walToSync = _wals.where((w) => w == wal).toList().first;
+ var walToSyncList = _wals.where((w) => w == wal).toList();
+ if (walToSyncList.isNotEmpty) {
+ var walToSync = walToSyncList.first;
+ // proceed with synchronization
+ } else {
+ // handle the case when wal is not found
+ return null; // or take appropriate action
+ }Committable suggestion was skipped due to low confidence.
| Future<SyncLocalFilesResponse?> syncWal({required Wal wal, IWalSyncProgressListener? progress}) async { | ||
| await _flush(); | ||
|
|
||
| var walToSync = _wals.where((w) => w == wal).toList().first; |
There was a problem hiding this comment.
Possible exception when retrieving Wal from list
Using first without checking if the list is empty can throw a StateError if the wal is not found in _wals.
Modify the code to handle the case where the wal is not found:
- var walToSync = _wals.where((w) => w == wal).toList().first;
+ var walToSyncList = _wals.where((w) => w == wal).toList();
+ if (walToSyncList.isNotEmpty) {
+ var walToSync = walToSyncList.first;
+ // proceed with synchronization
+ } else {
+ // handle the case when wal is not found
+ return null; // or take appropriate action
+ }Committable suggestion was skipped due to low confidence.
| late File walFile; | ||
| if (wal.filePath == null) { | ||
| debugPrint("file path is not found. wal id ${wal.id}"); | ||
| wal.status = WalStatus.corrupted; | ||
| } | ||
| try { | ||
| File file = File(wal.filePath!); | ||
| if (!file.existsSync()) { | ||
| debugPrint("file ${wal.filePath} is not exists"); | ||
| wal.status = WalStatus.corrupted; | ||
| } else { | ||
| walFile = file; | ||
| wal.isSyncing = true; | ||
| } | ||
| } catch (e) { | ||
| wal.status = WalStatus.corrupted; | ||
| debugPrint(e.toString()); | ||
| } |
There was a problem hiding this comment.
Uninitialized variable walFile may cause runtime errors
The variable walFile is only assigned if the file exists. If the file does not exist, walFile remains uninitialized, which can lead to a runtime error when it's used later.
Consider initializing walFile appropriately or returning early when the file is not found:
var resp = SyncLocalFilesResponse(newMemoryIds: [], updatedMemoryIds: []);
- late File walFile;
+ File? walFile;
if (wal.filePath == null) {
debugPrint("file path is not found. wal id ${wal.id}");
wal.status = WalStatus.corrupted;
+ return resp; // or handle accordingly
}
try {
File file = File(wal.filePath!);
if (!file.existsSync()) {
debugPrint("file ${wal.filePath} is not exists");
wal.status = WalStatus.corrupted;
+ return resp; // or handle accordingly
} else {
walFile = file;
wal.isSyncing = true;
}
} catch (e) {
wal.status = WalStatus.corrupted;
debugPrint(e.toString());
+ return resp; // or handle accordingly
}
+ if (walFile == null) {
+ // Handle the case appropriately
+ return resp; // or take necessary action
+ }📝 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.
| late File walFile; | |
| if (wal.filePath == null) { | |
| debugPrint("file path is not found. wal id ${wal.id}"); | |
| wal.status = WalStatus.corrupted; | |
| } | |
| try { | |
| File file = File(wal.filePath!); | |
| if (!file.existsSync()) { | |
| debugPrint("file ${wal.filePath} is not exists"); | |
| wal.status = WalStatus.corrupted; | |
| } else { | |
| walFile = file; | |
| wal.isSyncing = true; | |
| } | |
| } catch (e) { | |
| wal.status = WalStatus.corrupted; | |
| debugPrint(e.toString()); | |
| } | |
| File? walFile; | |
| if (wal.filePath == null) { | |
| debugPrint("file path is not found. wal id ${wal.id}"); | |
| wal.status = WalStatus.corrupted; | |
| return resp; | |
| } | |
| try { | |
| File file = File(wal.filePath!); | |
| if (!file.existsSync()) { | |
| debugPrint("file ${wal.filePath} is not exists"); | |
| wal.status = WalStatus.corrupted; | |
| return resp; | |
| } else { | |
| walFile = file; | |
| wal.isSyncing = true; | |
| } | |
| } catch (e) { | |
| wal.status = WalStatus.corrupted; | |
| debugPrint(e.toString()); | |
| return resp; | |
| } | |
| if (walFile == null) { | |
| return resp; | |
| } |
|
lgtm 🥳 |
sync_all_test_with_new_states.mp4
single_sync_with_better_states.mov
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
These changes aim to provide a smoother and more intuitive experience while managing memories in the app.