Implement resumable upload for google drive #117
Conversation
|
Warning Rate limit exceeded@cp-pratik-k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces enhancements to the media upload functionality, focusing on adding pause and resume capabilities for upload processes. New callback functions are implemented to manage upload states, and the user interface is refined to provide clearer controls for users. Additionally, the changes include the introduction of upload session management for cloud services like Google Drive and Dropbox, along with updates to data models and repositories to support these features. Changes
Possibly related PRs
Poem
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: 8
🔭 Outside diff range comments (1)
data/lib/repositories/media_process_repository.dart (1)
Line range hint
721-723: Fix incorrect method call for updating upload process statusIn the catch block of
_uploadInDropbox, the methodupdateDownloadProcessStatusis incorrectly called instead ofupdateUploadProcessStatus. This needs to be corrected to update the upload queue appropriately upon failure.Apply this diff to correct the method call:
await updateDownloadProcessStatus( + await updateUploadProcessStatus( status: MediaQueueProcessStatus.failed, id: process.id, );
🧹 Nitpick comments (6)
data/lib/models/media_process/media_process.freezed.dart (1)
731-731: Consider adding documentation for the upload_session_id field.While the toString implementation is correct, it would be helpful to add documentation explaining the purpose and lifecycle of the upload_session_id field in the original class definition.
data/lib/repositories/media_process_repository.dart (1)
454-468: Prevent multiple resumptions of the same upload processIn
resumeUploadProcess, ensure that an upload process is not started multiple times ifresumeUploadProcessis called repeatedly.Add a check to see if the process is already running before initiating the upload.
if (process.status.isUploading) { // The upload is already in progress. return; }data/lib/domain/config.dart (1)
22-28: LGTM! The configuration values are well-chosen.
uploadRequestByteSizeof 256KB (262144 bytes) is a good balance between network efficiency and memory usage for resumable uploads.processProgressUpdateDurationof 300ms provides smooth progress updates without overwhelming the UI.Consider making these values configurable through environment variables or build configuration for different environments (development, production) if needed.
app/lib/ui/flow/media_transfer/media_transfer_screen.dart (1)
125-130: Add error handling for pause/resume actionsWhile the implementation of pause/resume callbacks is correct, consider adding error handling to gracefully handle failures during these operations.
onResumeTap: () { + try { notifier.onResumeUploadProcess(uploadProcesses[index].id); + } catch (e) { + context.showErrorSnackBar(message: 'Failed to resume upload: $e'); + } }, onPausedTap: () { + try { notifier.onPauseUploadProcess(uploadProcesses[index].id); + } catch (e) { + context.showErrorSnackBar(message: 'Failed to pause upload: $e'); + } },data/lib/apis/google_drive/google_drive_endpoint.dart (1)
141-144: Add validation for required headersConsider adding validation to ensure that required headers (
Content-Type,Content-Length,Content-Range) are not null or empty before making the request.@override Map<String, dynamic> get headers => { + if (content.type.isEmpty) throw ArgumentError('Content-Type cannot be empty'), + if (content.length == null) throw ArgumentError('Content-Length is required'), + if (content.range == null) throw ArgumentError('Content-Range is required for resumable uploads'), 'Content-Type': content.type, 'Content-Length': content.length.toString(), 'Content-Range': content.range, };app/lib/ui/flow/media_transfer/components/transfer_item.dart (1)
141-142: Consider using localization for the pause message.Other status messages in this method use localization (
context.l10n), but the pause message is hardcoded. Consider using localization for consistency.- return "Upload paused"; + return context.l10n.upload_status_paused;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/lib/ui/flow/media_transfer/components/transfer_item.dart(4 hunks)app/lib/ui/flow/media_transfer/media_transfer_screen.dart(1 hunks)app/lib/ui/flow/media_transfer/media_transfer_view_model.dart(1 hunks)data/.flutter-plugins-dependencies(1 hunks)data/lib/apis/dropbox/dropbox_content_endpoints.dart(1 hunks)data/lib/apis/google_drive/google_drive_endpoint.dart(3 hunks)data/lib/domain/config.dart(1 hunks)data/lib/models/media_content/media_content.dart(1 hunks)data/lib/models/media_content/media_content.freezed.dart(10 hunks)data/lib/models/media_process/media_process.dart(3 hunks)data/lib/models/media_process/media_process.freezed.dart(14 hunks)data/lib/models/media_process/media_process.g.dart(3 hunks)data/lib/repositories/media_process_repository.dart(12 hunks)data/lib/services/dropbox_services.dart(5 hunks)data/lib/services/google_drive_service.dart(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- data/.flutter-plugins-dependencies
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (21)
data/lib/models/media_process/media_process.freezed.dart (2)
417-417: LGTM! Theupload_session_idfield is properly integrated.The new optional
String?field is correctly implemented across all necessary locations in the Freezed-generated code, including:
- Field declarations
- Constructor parameters
- copyWith method
- toString, equality, and hashCode implementations
Also applies to: 450-450, 509-512, 682-682, 705-706, 731-731, 746-747, 771-771, 804-804, 827-828
417-417: Verify session ID persistence across app restarts.Since
upload_session_idis used for resumable uploads, ensure that it's properly persisted and can survive app restarts to maintain upload progress.Also applies to: 705-706
✅ Verification successful
Upload session ID is properly persisted for resumable uploads
The implementation uses SQLite database storage which ensures upload_session_id persists across app restarts. The session ID is stored in a dedicated column in the upload_queue_table and is properly managed through the updateUploadSessionId method. This allows for reliable resumption of uploads from their last position.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if upload_session_id is being persisted in any storage implementations rg -A 5 "upload_session_id.*persist|persist.*upload_session_id" # Check if there are any database schema or migration files fd -e sql -e dart ".*schema.*|.*migration.*"Length of output: 112
Script:
#!/bin/bash # Find the original MediaProcess class file fd -e dart "media_process" --exclude "*.freezed.dart" --exclude "*.g.dart" # Search for all occurrences of upload_session_id rg -l "upload_session_id" # Look for storage/persistence related files fd -e dart "storage|repository|dao|database"Length of output: 509
Script:
#!/bin/bash # Check the repository implementation cat data/lib/repositories/media_process_repository.dart # Look for any persistence-related code in the model cat data/lib/models/media_process/media_process.dartLength of output: 35603
data/lib/apis/dropbox/dropbox_content_endpoints.dart (3)
164-199: Well-structured implementation ofDropboxStartUploadEndpointThe
DropboxStartUploadEndpointclass is well-implemented, properly setting up the initial upload session with correct headers and data handling.
200-245: Effective implementation ofDropboxAppendUploadEndpointThe
DropboxAppendUploadEndpointclass efficiently handles appending data to an ongoing upload session, with appropriate management of headers and content.
246-312: Comprehensive implementation ofDropboxFinishUploadEndpointApart from the minor issue with the endpoint path, the
DropboxFinishUploadEndpointclass is thoroughly implemented, correctly finalizing the upload session with proper handling of commit details and cursor information.data/lib/services/google_drive_service.dart (1)
220-220: Correct property name for content typeEnsure consistency in property naming. The
AppMediaContentclass usestypeas the property, but if any part of the code still referencescontentType, it should be updated totypeto prevent errors.Run the following script to verify all usages of
contentType:✅ Verification successful
Property naming is correct and consistent ✅
The codebase maintains proper separation between HTTP-standard
contentTypein the network layer andtypein application models. The mapping between these conventions is handled correctly inmedia_content.dart. No changes needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all references to 'contentType' in the codebase. # Search for 'contentType' in Dart files rg --glob '*.dart' 'contentType'Length of output: 702
data/lib/models/media_content/media_content.dart (2)
11-12: LGTM! The changes support resumable uploads.The addition of the
rangeproperty and renaming ofcontentTypetotypealign well with implementing resumable uploads. Therangeproperty will be useful for specifying byte ranges during resumable uploads.
18-18: Verify type compatibility with Google Drive API.The factory method correctly maps Google Drive's
contentTypeto the newtypeproperty.Let's verify the type compatibility:
✅ Verification successful
Type compatibility verified successfully
The
contentTypefrom Google Drive API is correctly mapped to thetypeproperty, as both represent MIME types and are used consistently in Content-Type headers across the API implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other usages of media.contentType to ensure consistency rg "media\.contentType" --type dartLength of output: 119
Script:
#!/bin/bash # Find MediaContent class definition and type property ast-grep --pattern 'class MediaContent { $$$ }' # Search for type-related definitions rg "(?i)content.*type" --type dart -A 3 # Look for Google Drive API related files fd "drive.*\.dart$"Length of output: 4871
data/lib/models/media_process/media_process.dart (3)
20-20: LGTM! The paused status supports resumable uploads.The addition of the
pausedstatus is essential for implementing resumable uploads.
37-37: LGTM! The isPaused getter is well-implemented.The getter provides a clean way to check for paused status.
125-125: Verify session ID persistence.The
upload_session_idis crucial for resumable uploads. Ensure it's properly persisted and recovered after app restarts.Let's check the persistence implementation:
✅ Verification successful
Upload session ID persistence implementation verified.
The
upload_session_idis properly persisted in the SQLite database and recovered through JSON serialization, ensuring it survives app restarts for resumable uploads.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for upload_session_id usage in database operations rg "upload_session_id" --type dart -A 5 # Search for session-related operations ast-grep --pattern 'class $_ { $$$ upload_session_id $$$ }'Length of output: 12168
data/lib/models/media_process/media_process.g.dart (2)
56-56: LGTM: Status enum updated for pause functionalityThe addition of
MediaQueueProcessStatus.pausedstatus is correctly implemented and aligns with the resumable upload feature.
67-67: LGTM: Upload session tracking implementedThe
upload_session_idfield is properly integrated into both JSON serialization and deserialization methods, enabling proper tracking of resumable upload sessions.Also applies to: 91-91
data/lib/models/media_content/media_content.freezed.dart (1)
21-22: LGTM: Media content model enhanced for resumable uploadsThe model has been correctly updated to support resumable uploads:
- Replaced
contentTypewith a more specifictypefield- Added optional
rangefield for tracking upload progress- All related methods are properly updated
Also applies to: 147-149
data/lib/apis/google_drive/google_drive_endpoint.dart (2)
87-115: LGTM: Start upload endpoint implementationThe
GoogleDriveStartUploadEndpointcorrectly implements the resumable upload initiation as per Google Drive API specifications.
117-160: Verify Content-Range header formatThe
GoogleDriveAppendUploadEndpointimplementation looks good, but ensure that theContent-Rangeheader follows the format specified by Google Drive API:
- For first chunk:
bytes 0-524287/2000000- For final chunk:
bytes 524288-2000000/2000000✅ Verification successful
Content-Range header format is correctly implemented
The Content-Range header follows the required Google Drive API format. The implementation in
google_drive_service.dartcorrectly generates the range asbytes start-end/totalwith proper zero-based byte ranges.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Content-Range header format matches Google Drive API specs ast-grep --pattern 'content.range = "bytes $_-$_/$_"'Length of output: 55
Script:
#!/bin/bash # Find AppMediaContent class definition and implementation ast-grep --pattern 'class AppMediaContent { $$$ }' # Also search for any mentions of range property or method rg -A 5 'range' --type dartLength of output: 9948
app/lib/ui/flow/media_transfer/components/transfer_item.dart (3)
14-15: LGTM! Well-structured implementation of pause/resume callbacks.The addition of
onPausedTapandonResumeTapcallbacks provides the necessary control for implementing resumable uploads.Also applies to: 22-23
93-101: LGTM! Clean implementation of the pause button.The pause button is appropriately shown only when the upload is running, using platform-specific icons.
84-92: LGTM! Well-designed state-specific resume buttons.The implementation thoughtfully uses different icons based on the upload state:
- Play icon for paused uploads
- Refresh icon for terminated/failed uploads
This provides clear visual feedback to users about the available actions.Also applies to: 113-121
data/lib/services/dropbox_services.dart (2)
295-295: LGTM! Consistent renaming of content type property.The change from
contentTypetotypeis applied consistently across allAppMediaContentinstantiations.Also applies to: 348-349, 389-390, 409-410, 539-539, 564-564, 598-598
328-361: LGTM! Robust implementation of upload session initialization.The implementation includes:
- File existence validation
- Proper error handling
- Clear session ID return for subsequent operations
| HttpMethod get method => HttpMethod.post; | ||
|
|
||
| @override | ||
| String get path => 'files/upload_session/finish'; |
There was a problem hiding this comment.
Fix missing leading slash in the endpoint path
In DropboxFinishUploadEndpoint, the path property is missing a leading slash. This could lead to incorrect API endpoint resolution. It should be '/files/upload_session/finish' to ensure the correct endpoint is called.
Apply this diff to correct the path:
- String get path => 'files/upload_session/finish';
+ String get path => '/files/upload_session/finish';📝 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 get path => 'files/upload_session/finish'; | |
| String get path => '/files/upload_session/finish'; |
| return Uri.parse(res.headers.value('location')!) | ||
| .queryParameters['upload_id']!; |
There was a problem hiding this comment.
Add null safety checks when parsing headers
When accessing the 'Location' header and parsing the URI, there is a possibility of encountering null values, which can lead to runtime exceptions. Implement null checks to handle cases where the header might be missing or improperly formatted.
Apply this diff to enhance null safety:
- return Uri.parse(res.headers.value('location')!)
- .queryParameters['upload_id']!;
+ final locationHeader = res.headers.value('location');
+ if (locationHeader == null) {
+ throw SomethingWentWrongError(
+ message: 'Missing Location header in response',
+ statusCode: res.statusCode,
+ );
+ }
+ final uploadId = Uri.parse(locationHeader).queryParameters['upload_id'];
+ if (uploadId == null) {
+ throw SomethingWentWrongError(
+ message: 'Missing upload_id in Location header',
+ statusCode: res.statusCode,
+ );
+ }
+ return uploadId;Committable suggestion skipped: line range outside the PR's diff.
| await _client.req(GoogleDriveStartUploadEndpoint(request: file)); | ||
|
|
||
| if (res.statusCode == 200) { | ||
| return Uri.parse(res.headers.value('location')!) | ||
| .queryParameters['upload_id']!; |
There was a problem hiding this comment.
Handle the upload session URI correctly
In the startUploadSession method, the 'Location' header contains the upload session URI required for subsequent upload requests. Instead of attempting to extract an upload_id from the query parameters, the full session URI should be stored and used.
Apply this diff to correct the code:
if (res.statusCode == 200) {
- return Uri.parse(res.headers.value('location')!)
- .queryParameters['upload_id']!;
+ final sessionUri = res.headers.value('location');
+ if (sessionUri == null) {
+ throw SomethingWentWrongError(
+ message: 'Missing Location header in response',
+ statusCode: res.statusCode,
+ );
+ }
+ return sessionUri;
}Also, update the appendUploadSession method to accept uploadSessionUri instead of uploadId, and use it directly to upload the chunks.
Committable suggestion skipped: line range outside the PR's diff.
| while (chunk < total) { | ||
| process = | ||
| _uploadQueue.firstWhere((element) => element.id == process.id); | ||
|
|
||
| if (process.status.isTerminated) { | ||
| showNotification('Upload to Google Drive Cancelled'); | ||
| break; | ||
| } else if (process.status.isPaused) { | ||
| showNotification('Upload to Google Drive Paused'); | ||
| break; | ||
| } | ||
|
|
||
| final endBytes = (chunk + ApiConfigs.uploadRequestByteSize) > total | ||
| ? total | ||
| : chunk + ApiConfigs.uploadRequestByteSize; | ||
|
|
||
| final res = await _googleDriveService.appendUploadSession( | ||
| startByte: chunk, | ||
| endByte: endBytes, | ||
| uploadId: uploadSessionId, | ||
| path: process.path, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement cancellation support during upload
In the _uploadInGoogleDrive method, if the upload process is paused or terminated, the ongoing network request to appendUploadSession should be canceled to prevent unnecessary data transfer and potential data corruption.
Consider passing a CancelToken to appendUploadSession and invoking cancelToken.cancel() when the process status is paused or terminated.
Apply this diff to pass the CancelToken:
final res = await _googleDriveService.appendUploadSession(
startByte: chunk,
endByte: endBytes,
uploadId: uploadSessionId,
path: process.path,
+ cancelToken: cancelToken,
);Ensure you create a CancelToken before the loop and cancel it when needed:
+ final cancelToken = CancelToken();And modify the condition to cancel the request:
if (process.status.isTerminated || process.status.isPaused) {
+ cancelToken.cancel();
showNotification('Upload to Google Drive Cancelled');
break;
}| Future<void> pauseUploadProcess(String id) async { | ||
| await database.rawUpdate( | ||
| "UPDATE ${LocalDatabaseConstants.uploadQueueTable} SET status = ? WHERE id = ? AND (status = ? OR status = ?)", | ||
| [ | ||
| MediaQueueProcessStatus.paused.value, | ||
| id, | ||
| MediaQueueProcessStatus.uploading.value, | ||
| MediaQueueProcessStatus.waiting.value, | ||
| ], | ||
| ); | ||
| await updateQueue(database); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure pausing the upload process cancels ongoing requests
The pauseUploadProcess method updates the status in the database but does not cancel any ongoing network requests. This might lead to the upload continuing despite being marked as paused.
Consider implementing cancellation logic by tracking CancelTokens for each upload process and invoking cancelToken.cancel() when pausing.
| showNotification('Failed to upload to Google Drive'); | ||
|
|
||
| await updateDownloadProcessStatus( |
There was a problem hiding this comment.
Fix incorrect method call for updating upload process status
In the catch block of _uploadInGoogleDrive, the method updateDownloadProcessStatus is incorrectly called instead of updateUploadProcessStatus. This would result in incorrect status updates for the upload queue.
Apply this diff to correct the method call:
showNotification('Failed to upload to Google Drive');
- await updateDownloadProcessStatus(
+ await updateUploadProcessStatus(
status: MediaQueueProcessStatus.failed,
id: process.id,
);📝 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.
| showNotification('Failed to upload to Google Drive'); | |
| await updateDownloadProcessStatus( | |
| showNotification('Failed to upload to Google Drive'); | |
| await updateUploadProcessStatus( |
| void onPauseUploadProcess(String id) { | ||
| _mediaProcessRepo.pauseUploadProcess(id); | ||
| } | ||
|
|
||
| void onResumeUploadProcess(String id) { | ||
| _mediaProcessRepo.resumeUploadProcess(id); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for pause/resume operations.
While the implementation is correct, it lacks error handling. Consider catching and handling potential errors that might occur during pause/resume operations.
Here's a suggested implementation:
void onPauseUploadProcess(String id) {
- _mediaProcessRepo.pauseUploadProcess(id);
+ try {
+ _mediaProcessRepo.pauseUploadProcess(id);
+ } catch (e) {
+ state = state.copyWith(error: e);
+ }
}
void onResumeUploadProcess(String id) {
- _mediaProcessRepo.resumeUploadProcess(id);
+ try {
+ _mediaProcessRepo.resumeUploadProcess(id);
+ } catch (e) {
+ state = state.copyWith(error: e);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| Future<AppMedia> appendUploadSession({ | ||
| required String path, | ||
| required int offset, | ||
| required String sessionId, | ||
| required String localRefId, | ||
| CancelToken? cancelToken, | ||
| void Function(int sent, int total)? onProgress, | ||
| }) async { | ||
| final localFile = File(path); | ||
|
|
||
| if (localFile.existsSync() == false) { | ||
| throw SomethingWentWrongError( | ||
| statusCode: 404, | ||
| message: 'File not found', | ||
| ); | ||
| } | ||
|
|
||
| final res = await _dropboxAuthenticatedDio.req( | ||
| DropboxAppendUploadEndpoint( | ||
| offset: offset, | ||
| sessionId: sessionId, | ||
| cancellationToken: cancelToken, | ||
| onProgress: onProgress, | ||
| content: AppMediaContent( | ||
| stream: localFile.openRead(), | ||
| length: localFile.lengthSync(), | ||
| type: 'application/octet-stream', | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| if (res.statusCode == 200) { | ||
| final res = await _dropboxAuthenticatedDio.req( | ||
| DropboxFinishUploadEndpoint( | ||
| filePath: | ||
| "/${ProviderConstants.backupFolderName}/${localFile.path.split('/').last}", | ||
| appPropertyTemplateId: | ||
| _dropboxFileIdAppPropertyTemplateIdController.state!, | ||
| localRefId: localRefId, | ||
| offset: offset, | ||
| sessionId: sessionId, | ||
| cancellationToken: cancelToken, | ||
| onProgress: onProgress, | ||
| content: AppMediaContent( | ||
| stream: localFile.openRead(), | ||
| length: localFile.lengthSync(), | ||
| type: 'application/octet-stream', | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| if (res.statusCode == 200) { | ||
| return AppMedia.fromDropboxJson(json: res.data); | ||
| } | ||
|
|
||
| throw SomethingWentWrongError( | ||
| statusCode: res.statusCode, | ||
| message: res.statusMessage, | ||
| ); | ||
| } | ||
|
|
||
| throw SomethingWentWrongError( | ||
| statusCode: res.statusCode, | ||
| message: res.statusMessage, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider splitting append and finish operations.
The method currently handles both append and finish operations, which violates the Single Responsibility Principle. Consider splitting into separate methods for better clarity and maintainability:
appendUploadSession: Only handles appending datafinishUploadSession: Handles completing the upload
Example refactor:
Future<void> appendUploadSession({
required String path,
required int offset,
required String sessionId,
- required String localRefId,
CancelToken? cancelToken,
void Function(int sent, int total)? onProgress,
}) async {
// ... file check ...
final res = await _dropboxAuthenticatedDio.req(
DropboxAppendUploadEndpoint(/* ... */)
);
if (res.statusCode != 200) {
throw SomethingWentWrongError(/* ... */);
}
}
+Future<AppMedia> finishUploadSession({
+ required String path,
+ required int offset,
+ required String sessionId,
+ required String localRefId,
+ CancelToken? cancelToken,
+ void Function(int sent, int total)? onProgress,
+}) async {
+ // ... finish upload logic ...
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
data/lib/services/google_drive_service.dart (1)
256-258:⚠️ Potential issueAdd null safety checks when parsing headers
The code is vulnerable to null pointer exceptions when accessing the 'location' header and parsing the URI.
Apply this diff to enhance null safety:
- return Uri.parse(res.headers.value('location')!) - .queryParameters['upload_id']!; + final locationHeader = res.headers.value('location'); + if (locationHeader == null) { + throw SomethingWentWrongError( + message: 'Missing Location header in response', + statusCode: res.statusCode, + ); + } + final uploadId = Uri.parse(locationHeader).queryParameters['upload_id']; + if (uploadId == null) { + throw SomethingWentWrongError( + message: 'Missing upload_id in Location header', + statusCode: res.statusCode, + ); + } + return uploadId;data/lib/apis/dropbox/dropbox_content_endpoints.dart (1)
281-281:⚠️ Potential issueFix missing leading slash in the endpoint path
The path property is missing a leading slash. This could lead to incorrect API endpoint resolution.
Apply this diff to correct the path:
- String get path => '/files/upload_session/finish'; + String get path => '/files/upload_session/finish';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
data/lib/apis/dropbox/dropbox_content_endpoints.dart(2 hunks)data/lib/repositories/media_process_repository.dart(21 hunks)data/lib/services/dropbox_services.dart(5 hunks)data/lib/services/google_drive_service.dart(7 hunks)
🧰 Additional context used
🪛 GitHub Actions: Analyze
data/lib/repositories/media_process_repository.dart
[info] 705-705: Missing a required trailing comma. Try adding a trailing comma.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (13)
data/lib/services/google_drive_service.dart (3)
95-95: LGTM! Good separation of concernsThe query modification correctly excludes album files from media listing, maintaining a clean separation between media and album management.
220-220: LGTM! Consistent parameter namingThe parameter rename from
contentTypetotypeis applied consistently across all methods, improving code consistency.Also applies to: 459-459, 485-485, 562-562, 636-636
267-315: LGTM! Robust implementation of appendUploadSessionThe implementation includes:
- Proper file existence validation
- Correct byte range handling
- Comprehensive error handling including resume status (308)
data/lib/apis/dropbox/dropbox_content_endpoints.dart (3)
150-150: LGTM! Property name change fromcontentTypetotypeThe change aligns with the updated
AppMediaContentmodel.
164-197: LGTM! Well-structured upload session initialization endpointThe implementation follows the endpoint pattern consistently and includes all necessary configurations for starting an upload session.
199-242: LGTM! Well-implemented append operation endpointThe implementation correctly handles session continuation with proper offset tracking and content streaming.
data/lib/services/dropbox_services.dart (4)
295-295: LGTM! Consistent property name updatesThe changes from
contentTypetotypeare applied consistently across all occurrences.Also applies to: 561-561, 586-586, 620-620
328-364: LGTM! Well-implemented upload session initializationThe method includes proper error handling and file existence validation.
366-403: LGTM! Well-structured append operationThe method follows consistent error handling patterns and properly manages upload session continuation.
405-450: LGTM! Well-implemented session finalizationThe method properly handles session completion with appropriate error handling.
data/lib/repositories/media_process_repository.dart (3)
15-15: LGTM! Consistent integration of UniqueIdGeneratorThe UniqueIdGenerator is properly injected and used consistently across the repository.
Also applies to: 31-31, 81-81, 246-246, 298-298, 388-388
141-141: LGTM! Database schema update for session managementThe upload_session_id column is properly added to support resumable uploads.
463-490: LGTM! Well-implemented pause/resume functionalityThe implementation properly handles upload process state transitions between paused and resumed states.
| // If the upload session id is null and the chunk is not equal to 0 then start the upload session from the beginning | ||
| if (chunk != 0) { | ||
| await updateUploadProcessProgress( | ||
| id: process.id, chunk: 0, total: total); |
There was a problem hiding this comment.
Add missing trailing comma
A trailing comma is required for consistent formatting.
Apply this diff to add the trailing comma:
- id: process.id, chunk: 0, total: total);
+ id: process.id, chunk: 0, total: total,);📝 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.
| id: process.id, chunk: 0, total: total); | |
| id: process.id, chunk: 0, total: total,); |
🧰 Tools
🪛 GitHub Actions: Analyze
[info] 705-705: Missing a required trailing comma. Try adding a trailing comma.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
data/lib/repositories/media_process_repository.dart (3)
463-473: 🛠️ Refactor suggestionImplement request cancellation in pauseUploadProcess.
The
pauseUploadProcessmethod only updates the status but doesn't cancel ongoing upload requests. This could lead to unnecessary network usage.
626-628:⚠️ Potential issueFix incorrect method call in error handling.
The error handling block incorrectly calls
updateDownloadProcessStatusinstead ofupdateUploadProcessStatus.
705-705:⚠️ Potential issueAdd missing trailing comma.
A trailing comma is required for consistent formatting.
🧰 Tools
🪛 GitHub Actions: Analyze
[info] 705-705: Missing a required trailing comma. Try adding a trailing comma.
🧹 Nitpick comments (3)
data/lib/repositories/media_process_repository.dart (3)
15-15: LGTM! Consider adding documentation for the new dependency.The addition of
UniqueIdGeneratoras a dependency is a good practice for better testability and maintainability.Consider adding a documentation comment explaining the purpose of
_uniqueIdGenerator:/// Generator for creating unique identifiers used in media processes final UniqueIdGenerator _uniqueIdGenerator;Also applies to: 81-81, 109-109
141-141: Consider adding an index for the upload_session_id column.Since
upload_session_idwill be frequently queried during upload operations, adding an index could improve query performance.CREATE INDEX idx_upload_session_id ON ${LocalDatabaseConstants.uploadQueueTable} (upload_session_id);
571-624: Extract common upload functionality into a shared method.The upload implementations for Google Drive and Dropbox share similar logic for chunking, progress tracking, and notifications. Consider extracting these into a shared method to reduce code duplication.
Future<void> _handleChunkedUpload({ required UploadMediaProcess process, required Future<void> Function(int startByte, int endByte) uploadChunk, required Future<void> Function(String message) showNotification, required AppMedia? response, }) async { int chunk = process.chunk; final total = await File(process.path).length(); while (chunk < total) { if (process.status.isTerminated || process.status.isPaused) { showNotification(process.status.isTerminated ? 'Upload Cancelled' : 'Upload Paused'); break; } final end = (chunk + ApiConfigs.uploadRequestByteSize).clamp(0, total); await uploadChunk(chunk, end); chunk = end; await updateUploadProcessProgress( id: process.id, chunk: chunk, total: total, ); await showNotification( '${chunk.formatBytes} / ${total.formatBytes} - ${(chunk / total * 100).round()}%', ); } if (chunk >= total) { await updateUploadProcessStatus( status: MediaQueueProcessStatus.completed, response: response, id: process.id, ); await showNotification('Upload completed successfully'); await clearUploadProcessResponse(id: process.id); } }Also applies to: 664-749
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
data/lib/repositories/media_process_repository.dart(21 hunks)
🧰 Additional context used
🪛 GitHub Actions: Analyze
data/lib/repositories/media_process_repository.dart
[info] 705-705: Missing a required trailing comma. Try adding a trailing comma.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
data/lib/repositories/media_process_repository.dart (2)
111-111: Consider adding error handling for database initialization.The database initialization is called directly in the constructor without any error handling. If the initialization fails, it could leave the repository in an invalid state.
Consider wrapping the initialization in a try-catch block and handling potential database errors:
- _initializeLocalDatabase(); + try { + await _initializeLocalDatabase(); + } catch (e) { + debugPrint('Failed to initialize database: $e'); + // Consider throwing a custom exception or handling the error appropriately + }
557-624: Add cleanup for abandoned upload sessions.When an upload is terminated or fails, the upload session is not cleaned up on Google Drive's side. This could lead to orphaned upload sessions.
Consider adding cleanup logic:
if (process.status.isTerminated) { + try { + await _googleDriveService.deleteUploadSession(uploadSessionId); + } catch (e) { + debugPrint('Failed to cleanup upload session: $e'); + } showNotification('Upload to Google Drive Cancelled'); break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
data/lib/repositories/media_process_repository.dart(21 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
data/lib/repositories/media_process_repository.dart (2)
591-596: Implement cancellation support during upload.The
appendUploadSessioncall should support cancellation to prevent unnecessary data transfer when the upload is paused or terminated.
Line range hint
124-164: Consider implementing database migration strategy.The database schema has been updated with a new column
upload_session_id, but there's no migration strategy for existing installations. This could cause issues for users upgrading from a previous version.Consider implementing a version-based migration strategy. Also, verify if there are any existing installations that need migration:
| // If the upload session id is null and the chunk is not equal to 0 then start the upload session from the beginning | ||
| if (chunk != 0) { | ||
| await updateUploadProcessProgress( | ||
| id: process.id, | ||
| chunk: chunk, | ||
| chunk: 0, | ||
| total: total, | ||
| ); | ||
| process = _uploadQueue.firstWhere((e) => e.id == process.id); | ||
| _uploadInDropbox(process); | ||
| return; |
There was a problem hiding this comment.
Fix potential infinite loop in Dropbox upload session restart.
The current implementation could lead to an infinite loop if the upload session fails to start repeatedly. There's no maximum retry limit or backoff strategy.
Consider adding a retry limit and exponential backoff:
if (chunk != 0) {
+ static const maxRetries = 3;
+ int retryCount = 0;
+ while (retryCount < maxRetries) {
await updateUploadProcessProgress(
id: process.id,
chunk: 0,
total: total,
);
process = _uploadQueue.firstWhere((e) => e.id == process.id);
- _uploadInDropbox(process);
- return;
+ try {
+ await _uploadInDropbox(process);
+ return;
+ } catch (e) {
+ retryCount++;
+ if (retryCount >= maxRetries) throw e;
+ await Future.delayed(Duration(seconds: math.pow(2, retryCount).toInt()));
+ }
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| Future<void> resumeUploadProcess(String id) async { | ||
| final process = _uploadQueue.firstWhere( | ||
| (element) => element.id == id, | ||
| ); | ||
| if (process.provider == MediaProvider.googleDrive) { | ||
| _uploadInGoogleDrive(process); | ||
| } else if (process.provider == MediaProvider.dropbox) { | ||
| _uploadInDropbox(process); | ||
| } else { | ||
| updateUploadProcessStatus( | ||
| status: MediaQueueProcessStatus.failed, | ||
| id: process.id, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential race condition in resumeUploadProcess.
The method retrieves the process and then starts the upload without checking if the status is still valid. Another operation could change the status between these steps.
Consider adding a status check and using a transaction:
Future<void> resumeUploadProcess(String id) async {
+ bool updated = false;
+ await database.transaction((txn) async {
+ final result = await txn.rawUpdate(
+ "UPDATE ${LocalDatabaseConstants.uploadQueueTable} SET status = ? WHERE id = ? AND status = ?",
+ [MediaQueueProcessStatus.waiting.value, id, MediaQueueProcessStatus.paused.value],
+ );
+ updated = result > 0;
+ });
+
+ if (!updated) return;
+
final process = _uploadQueue.firstWhere(
(element) => element.id == id,
);Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
These updates provide more robust and flexible media transfer capabilities, giving users greater control over their upload and download processes.