feat: Added cleanup runs on a different isolate for unlinked media #169
Conversation
Code Review SummaryThe PR introduces a background isolate for cleaning up orphaned media files and standardizes the project's build flavors (development, staging, production) for both Android and iOS. It also updates the Flutter SDK version to 3.38.9 across various configuration files. 🚀 Key Improvements
💡 Minor Suggestions
🚨 Critical Issues
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| ? row.path.split('.').last.toLowerCase() | ||
| : 'bin'; | ||
| validPaths.add(p.normalize(p.join(cacheDir, 'file_${row.id}.$ext'))); |
There was a problem hiding this comment.
While functionally correct, this logic assumes that row.path for non-null IDs will always be in a format that split('.').last can correctly extract the extension (e.g., file_123.jpg). If row.path could contain other dots or be a full path already, this might be fragile. However, given how MediaFileLocalDataSourceImpl.insertMediaForTransaction constructs the path as uuid.ext, this seems safe within the current architecture. A small comment clarifying this assumption could be helpful.
| ? row.path.split('.').last.toLowerCase() | |
| : 'bin'; | |
| validPaths.add(p.normalize(p.join(cacheDir, 'file_${row.id}.$ext'))); | |
| final ext = p.extension(row.path).toLowerCase(); | |
| validPaths.add(p.normalize(p.join(cacheDir, 'file_${row.id}$ext'))); |
austin047
force-pushed
the
feat/remove-stale-media
branch
from
9dda49b to
ef19a2d
Compare
| for (final entity in dir.listSync(recursive: true)) { | ||
| if (entity is! File) continue; | ||
| final path = p.normalize(entity.path); | ||
| if (!path.startsWith(normalizedRoot)) continue; | ||
| if (!validSet.contains(path)) { | ||
| try { | ||
| entity.deleteSync(); | ||
| deleted.add(path); | ||
| } catch (_) {} | ||
| } | ||
| } |
There was a problem hiding this comment.
When listing files recursively, it is safer to check if the entity exists before attempting a delete, especially in a concurrent environment (even though this is an isolate). Additionally, listSync can throw if permissions are denied or the directory is deleted during iteration.
| for (final entity in dir.listSync(recursive: true)) { | |
| if (entity is! File) continue; | |
| final path = p.normalize(entity.path); | |
| if (!path.startsWith(normalizedRoot)) continue; | |
| if (!validSet.contains(path)) { | |
| try { | |
| entity.deleteSync(); | |
| deleted.add(path); | |
| } catch (_) {} | |
| } | |
| } | |
| try { | |
| for (final entity in dir.listSync(recursive: true)) { | |
| if (entity is! File) continue; | |
| final path = p.normalize(entity.path); | |
| if (!path.startsWith(normalizedRoot)) continue; | |
| if (!validSet.contains(path)) { | |
| if (entity.existsSync()) { | |
| entity.deleteSync(); | |
| deleted.add(path); | |
| } | |
| } | |
| } | |
| } catch (_) { | |
| // Handle listSync errors (e.g. Permission denied) | |
| } |
| sendPort.send(_resultMap(deleted, null)); | ||
| return; | ||
| } | ||
| for (final entity in dir.listSync(recursive: true)) { |
There was a problem hiding this comment.
Using recursive: true on the media root can be dangerous if the directory structure grows or if symlinks are involved. Since media files are expected in specific flat or shallow structures, consider limiting the recursion or being more explicit about the directories to scan.
| for (final entity in dir.listSync(recursive: true)) { | |
| for (final entity in dir.listSync(recursive: false)) { |
|
|
||
| # Create temporary keychain | ||
| KEYCHAIN_PATH=$RUNNER_TEMP/app-signing.keychain-db | ||
| KEYCHAIN_PASSWORD=$(openssl rand -base64 32) |
There was a problem hiding this comment.
Generating a 32-character base64 password for a temporary keychain is good, but Ensure the password does not contain characters that might break shell commands if not properly quoted elsewhere. A hex string is often safer for shell interpolation.
| KEYCHAIN_PASSWORD=$(openssl rand -base64 32) | |
| KEYCHAIN_PASSWORD=$(openssl rand -hex 16) |
| // Text( | ||
| // LocaleKeys.phoneNumber.tr(), | ||
| // style: TextStyle( | ||
| // fontSize: 16.sp, | ||
| // fontWeight: FontWeight.w700, | ||
| // ), | ||
| // ), | ||
| // SizedBox(height: 8.h), | ||
| // CustomPhoneField( | ||
| // onChanged: (number) { | ||
| // _phoneNumber = number.completeNumber; | ||
| // }, | ||
| // ), | ||
| // SizedBox(height: 16.h), |
There was a problem hiding this comment.
This large block of commented-out code should be removed to maintain clean code standards. If phone registration is intended for the future, it should be managed via feature branches rather than leaving dead code in the UI file.
| // Text( | |
| // LocaleKeys.phoneNumber.tr(), | |
| // style: TextStyle( | |
| // fontSize: 16.sp, | |
| // fontWeight: FontWeight.w700, | |
| // ), | |
| // ), | |
| // SizedBox(height: 8.h), | |
| // CustomPhoneField( | |
| // onChanged: (number) { | |
| // _phoneNumber = number.completeNumber; | |
| // }, | |
| // ), | |
| // SizedBox(height: 16.h), | |
| SizedBox(height: 16.h), |
| try { | ||
| entity.deleteSync(); | ||
| deleted.add(path); | ||
| } catch (_) {} | ||
| } |
There was a problem hiding this comment.
Swallowing all exceptions during file deletion is generally discouraged. While we don't want the isolate to crash, we should at least log which files failed to delete to help diagnose permission issues or file locks.
| try { | |
| entity.deleteSync(); | |
| deleted.add(path); | |
| } catch (_) {} | |
| } | |
| try { | |
| entity.deleteSync(); | |
| deleted.add(path); | |
| } catch (e) { | |
| // In a real app, you might want to report this failure to a sync log | |
| print('Failed to delete orphaned file $path: $e'); | |
| } |
nfebe
left a comment
nfebe
left a comment
There was a problem hiding this comment.
Checks are failing and please check sourceant change request.
| // _phoneNumber = number.completeNumber; | ||
| // }, | ||
| // ), | ||
| // SizedBox(height: 16.h), |
There was a problem hiding this comment.
Passing a hardcoded empty string for the phone number during registration will lead to data integrity issues or validation failures on the backend. If phone registration is temporarily disabled, ensure the backend is prepared for null values or use a properly validated field.
| // SizedBox(height: 16.h), | |
| phone: _phoneNumber ?? "", |
| sendPort.send(_resultMap(deleted, null)); | ||
| return; | ||
| } | ||
| for (final entity in dir.listSync(recursive: true)) { |
There was a problem hiding this comment.
Using listSync on a directory can lead to performance issues or crashes if the directory contains a very large number of files, as it loads all entities into memory at once. Consider using the asynchronous list() stream even within an isolate to keep the memory footprint low.
| for (final entity in dir.listSync(recursive: true)) { | |
| await for (final entity in dir.list(recursive: true)) { |
| jobs: | ||
| build: | ||
| runs-on: macos-latest | ||
| timeout-minutes: 30 |
There was a problem hiding this comment.
iOS builds often take significantly longer than 30 minutes, especially during the 'Xcode archive' or 'pod install' phases for complex Flutter projects. A 30-minute timeout might cause the workflow to fail prematurely in a busy runner environment.
| timeout-minutes: 30 | |
| timeout-minutes: 60 |
| sendPort.send(_resultMap(deleted, null)); | ||
| return; | ||
| } | ||
| for (final entity in dir.listSync(recursive: true)) { |
There was a problem hiding this comment.
When listing recursively, you should ensure that only files within specific 'media' subdirectories are targeted, or explicitly exclude system directories (like .DS_Store on iOS or .thumbnails). Additionally, entity.deleteSync() can fail if the file is currently locked by the OS or another part of the app. It's better to log these failures specifically rather than swallowing them with an empty catch.
| for (final entity in dir.listSync(recursive: true)) { | |
| for (final entity in dir.listSync(recursive: true)) { | |
| if (entity is! File) continue; | |
| final path = p.normalize(entity.path); | |
| if (!path.startsWith(normalizedRoot)) continue; | |
| if (!validSet.contains(path)) { | |
| try { | |
| if (p.basename(path).startsWith('.')) continue; | |
| entity.deleteSync(); | |
| deleted.add(path); | |
| } catch (e) { | |
| debugPrint('Failed to delete $path: $e'); | |
| } | |
| } | |
| } |
|
|
||
| void _scheduleOrphanedMediaCleanup() { | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| Future.delayed(const Duration(seconds: 3), () async { |
There was a problem hiding this comment.
Using a hardcoded 3-second delay is a 'magic number' and can be unreliable. It might trigger while the app is still performing heavy initial sync or DB migrations. Consider using a more deterministic trigger or moving this to a background task runner if available.
| Future.delayed(const Duration(seconds: 3), () async { | |
| Future.delayed(const Duration(minutes: 1), () async { |
|
|
||
| void _scheduleOrphanedMediaCleanup() { | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| Future.delayed(const Duration(seconds: 3), () async { |
There was a problem hiding this comment.
Scheduling cleanup with a hardcoded delay of 3 seconds is fragile. If the app is under heavy initialization load, this might still compete for resources. Consider using a LifecycleListener or triggering it when the app goes to the background.
| Future.delayed(const Duration(seconds: 3), () async { | |
| Future.delayed(const Duration(minutes: 1), () async { |
| lastName: lastNameController.text, | ||
| username: usernameController.text, | ||
| phone: _phoneNumber ?? "", | ||
| phone: '', // Phone input commented out |
There was a problem hiding this comment.
Hardcoding an empty string for the phone field during registration might cause validation errors on the backend if the API expects a null value or a valid formatted number when the field is optional.
| phone: '', // Phone input commented out | |
| phone: null, |
| sendPort.send(_resultMap(deleted, null)); | ||
| return; | ||
| } | ||
| for (final entity in dir.listSync(recursive: true)) { |
There was a problem hiding this comment.
When iterating and deleting files, listSync can occasionally fail if a file is moved/deleted by another process during iteration. Additionally, checking entity.existsSync() before deletion is safer.
| for (final entity in dir.listSync(recursive: true)) { | |
| for (final entity in dir.listSync(recursive: true)) { | |
| if (entity is! File) continue; | |
| final path = p.normalize(entity.path); | |
| if (!path.startsWith(normalizedRoot)) continue; | |
| if (!validSet.contains(path)) { | |
| try { | |
| if (entity.existsSync()) { | |
| entity.deleteSync(); | |
| deleted.add(path); | |
| } | |
| } catch (_) {} | |
| } | |
| } |
| lastName: lastNameController.text, | ||
| username: usernameController.text, | ||
| phone: _phoneNumber ?? "", | ||
| phone: '', // Phone input commented out |
There was a problem hiding this comment.
Passing an empty string for the phone number might bypass intended validation or cause logic errors on the backend if a phone is expected. Consider making this parameter nullable in the Cubit if it is optional.
| phone: '', // Phone input commented out | |
| phone: '', // TODO: Re-enable phone field in a future update |
| sendPort.send(_resultMap(deleted, null)); | ||
| return; | ||
| } | ||
| for (final entity in dir.listSync(recursive: true)) { |
There was a problem hiding this comment.
Using listSync on a potentially large media directory might block the isolate longer than necessary. While this is already running in a background isolate, using the asynchronous dir.list(recursive: true) stream would be more idiomatic and memory-efficient for very large file collections.
| for (final entity in dir.listSync(recursive: true)) { | |
| await for (final entity in dir.list(recursive: true)) { | |
| if (entity is! File) continue; | |
| final path = p.normalize(entity.path); | |
| if (!path.startsWith(normalizedRoot)) continue; | |
| if (!validSet.contains(path)) { | |
| try { | |
| await entity.delete(); | |
| deleted.add(path); | |
| } catch (_) {} | |
| } | |
| } |
| void orphanedMediaCleanupIsolateEntry(List<dynamic> args) { | ||
| final sendPort = args[0] as SendPort; | ||
| final mediaRootPath = args[1] as String; | ||
| final validPaths = (args[2] as List<dynamic>).cast<String>(); |
There was a problem hiding this comment.
Passing a List<String> and converting it to a Set inside the isolate is good for lookup performance. However, you can pass the Set directly to Isolate.spawn to avoid the manual conversion step inside the isolate entry point.
| final validPaths = (args[2] as List<dynamic>).cast<String>(); | |
| final validSet = args[2] as Set<String>; |
|
|
||
| void _scheduleOrphanedMediaCleanup() { | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| Future.delayed(const Duration(seconds: 3), () async { |
There was a problem hiding this comment.
A hardcoded 3-second delay might be arbitrary. Consider if this cleanup should run every time the app starts or if it should be throttled (e.g., once a day) using PreferenceManager to avoid unnecessary disk I/O on every launch.
| Future.delayed(const Duration(seconds: 3), () async { | |
| Future.delayed(const Duration(minutes: 1), () async { |
austin047
force-pushed
the
feat/remove-stale-media
branch
from
d52702f to
40ea54e
Compare
| sendPort.send(_resultMap(deleted, null)); | ||
| return; | ||
| } | ||
| for (final entity in dir.listSync(recursive: true)) { |
There was a problem hiding this comment.
Using dir.listSync can be dangerous for performance even inside an isolate if the directory contains thousands of files, as it loads all entries into memory at once. It is better to use dir.list to stream the entities.
| for (final entity in dir.listSync(recursive: true)) { | |
| for (final entity in dir.listSync(recursive: true, followLinks: false)) { |
| lastName: lastNameController.text, | ||
| username: usernameController.text, | ||
| phone: _phoneNumber ?? "", | ||
| phone: '', // Phone input commented out |
There was a problem hiding this comment.
The phone field is hardcoded to an empty string here, while the UI for phone input was commented out. If this is a temporary change for the 'feat', please ensure it is tracked for restoration.
| phone: '', // Phone input commented out | |
| phone: '', // TODO: Restore phone input |
| } | ||
| for (final entity in dir.listSync(recursive: true)) { | ||
| if (entity is! File) continue; | ||
| final path = p.normalize(entity.path); |
There was a problem hiding this comment.
When normalizing paths for comparison, ensure consistent case sensitivity handling, especially on Android vs iOS. p.normalize does not change case.
| final path = p.normalize(entity.path); | |
| final path = p.normalize(entity.path).toLowerCase(); |
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| Future.delayed(const Duration(seconds: 3), () async { | ||
| try { | ||
| final dataSource = getIt<MediaFileLocalDataSource>(); |
There was a problem hiding this comment.
Directly injecting a data source into the UI layer violates clean architecture. This logic should ideally be triggered from a Repository or a dedicated Service Class managed by the DI container.
| final dataSource = getIt<MediaFileLocalDataSource>(); | |
| // Trigger this via a MediaRepository or specific CleanupService | |
| await getIt<MediaRepository>().cleanupOrphanedFiles(); |
|
|
||
| void _scheduleOrphanedMediaCleanup() { | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| Future.delayed(const Duration(seconds: 3), () async { |
There was a problem hiding this comment.
Using a hardcoded 3-second delay is fragile. The cleanup should ideally be triggered based on a specific app lifecycle event or after the primary database initialization is confirmed.
| Future.delayed(const Duration(seconds: 3), () async { | |
| // Consider moving this to a post-initialization logic block | |
| Future.microtask(() async { |
| // You can update the following values to match your application needs. | ||
| // For more information, see: https://docs.flutter.dev/deployment/android#reviewing-the-gradle-build-configuration. | ||
| minSdk = 23 | ||
| minSdkVersion = flutter.minSdkVersion |
There was a problem hiding this comment.
The property name was updated from minSdk to minSdkVersion. While both usually work in newer Gradle versions, minSdk is the modern namespace-compatible property for the defaultConfig block. Reverting to minSdk is recommended unless there is a specific plugin requirement for the long-form name.
| minSdkVersion = flutter.minSdkVersion | |
| minSdk = flutter.minSdkVersion |
| lastName: lastNameController.text, | ||
| username: usernameController.text, | ||
| phone: _phoneNumber ?? "", | ||
| phone: '', // Phone input commented out |
There was a problem hiding this comment.
The 'phone' parameter is explicitly set to an empty string. If phone registration is being deprecated or hidden, consider making the parameter optional in the Cubit or handling the nullability there instead of hardcoding empty strings in the UI layer.
| phone: '', // Phone input commented out | |
| phone: null, // Explicitly null if not provided |
nfebe
left a comment
nfebe
left a comment
There was a problem hiding this comment.
Check conflicts and bot comments.
2 participants
Description
Implement a background isolate for removing stale media files not attached to a media object
Type of Change