Merged
Conversation
tddang-linagora
requested review from
9clg6,
dab246,
hoangdat and
nqhhdev
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a mobile-only "Data and storage" settings screen and related navigation, localization entries, and assets. Introduces storage scanning and calculation utilities, a storage-scanner service using isolates, GetIt registration for the scanner, UI view/controller/widgets for presenting storage usage and clearing cache, a StorageCategory enum with classification logic, unit tests for calculators and scanner mapping, and a new dependency to query device storage. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2917 |
Member
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
lib/pages/settings_dashboard/settings/settings.dart (1)
264-265: UseAppRoutePaths.dataAndStorageFullhere instead of another string literal.This duplicates the new constant from
lib/config/go_routes/app_route_paths.dart:7-8, so the navigation target can drift from the router definition.♻️ Suggested cleanup
+import 'package:fluffychat/config/go_routes/app_route_paths.dart'; + - final result = await context.push('/rooms/dataAndStorage'); + final result = await context.push(AppRoutePaths.dataAndStorageFull);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings/settings.dart` around lines 264 - 265, Replace the hard-coded route string used when handling SettingEnum.dataAndStorage with the route constant AppRoutePaths.dataAndStorageFull: in the switch branch that currently calls context.push('/rooms/dataAndStorage') (the SettingEnum.dataAndStorage case), change the push target to use AppRoutePaths.dataAndStorageFull so navigation uses the centralized route constant defined in app_route_paths.dart and cannot drift from the router definition.lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_view.dart (1)
85-113: Consider using design system colors instead of hardcoded hex values.The view uses
LinagoraSysColorsandLinagoraRefColorsthroughout, but falls back to hardcoded colors for the progress indicator (lines 92-93) and label text (lines 103, 108). Using the design system consistently would improve maintainability and theme support.Example using design system colors
ClipRRect( borderRadius: BorderRadius.circular(999), child: LinearProgressIndicator( value: controller.isScanning ? null : controller.storageUsageRatio, minHeight: 8, - backgroundColor: const Color(0xFFEEEEEE), - color: const Color(0xFF006DE2), + backgroundColor: sysColors.surfaceVariant, + color: sysColors.primary, ), ), const SizedBox(height: 8), Row( mainAxisAlignment: MainAxisAlignment.spaceBetween, children: [ Text( l10n.twake, style: textTheme.labelSmall?.copyWith( - color: const Color(0xFF636363), + color: refColors.tertiary[30], ), ), Text( l10n.available, style: textTheme.labelSmall?.copyWith( - color: const Color(0xFF636363), + color: refColors.tertiary[30], ), ), ], ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_view.dart` around lines 85 - 113, The hardcoded colors used in the progress bar and label texts should be replaced with the design system colors; update the LinearProgressIndicator (inside the ClipRRect) to use the appropriate LinagoraSysColors/LinagoraRefColors token(s) instead of Color(0xFFEEEEEE) and Color(0xFF006DE2), and replace the label text color usages (the two text widgets using textTheme.labelSmall) to use the design system color token instead of Color(0xFF636363); locate these in the ClipRRect -> LinearProgressIndicator and the two Text widgets (l10n.twake and l10n.available) and swap the hardcoded Color(...) instances for the corresponding LinagoraSysColors or LinagoraRefColors constants so theming/maintainability is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/config/go_routes/go_router.dart`:
- Line 48: The router currently registers the '/rooms/dataAndStorage' route
unconditionally which will crash on web because SettingsDataAndStorage imports
dart:io and package:storage_space; update the GoRouter registration code that
creates the redirect/route for '/rooms/dataAndStorage' to only register (or
return the mobile-specific redirect) when PlatformInfos.isMobile is true (use
the same guard as in settings.dart); if PlatformInfos.isMobile is false, omit
the route or return a safe fallback redirect so web does not resolve the page.
In
`@lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_scanner.dart`:
- Around line 16-26: The current try/catch around the whole directory scan
swallows directory-level errors and returns a partial result; instead, let
directory-level failures surface and only catch per-entity errors inside the
loop: remove or rethrow the outer catch so errors from
Directory(dirPath).list(...) bubble up to the caller (scanDirectory), and wrap
the body that handles each entity (the is! File check,
StorageCategory.fromFile(entity.path) and entity.lengthSync()) in a small
try/catch that on error skips that file (continue) and optionally records/logs
the failure; keep updates to the result map (result[category.name] = ...) only
when the per-file processing succeeds.
- Around line 35-40: The code currently awaits events.first twice (creating a
race where the second message can be lost); replace the dual await on the
broadcast stream with a StreamQueue to keep a single listener alive across both
isolate messages: construct a StreamQueue from receivePort.asBroadcastStream()
(e.g. final queue = StreamQueue(events)), then await queue.next for the SendPort
(assign to isolateSendPort) and await queue.next again for the Map (convert to
Map<String,int>), and finally cancel the queue and close the ReceivePort; update
usages around receivePort/events/isolateSendPort to use this queue-based
sequencing so no messages are dropped.
In
`@lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage.dart`:
- Around line 76-87: The clear-cache routine currently deletes the whole
temporary directory and can remove files in use by the preview flow; update
_clearCacheDir to avoid disrupting active previews by either (A) prompting the
user with a confirmation/warning before clearing (add a UI confirmation call
from the settings flow that explains active previews may be affected), or (B)
skipping currently-used preview files by integrating a usage check from the
preview logic: expose a method like isPreviewFileInUse(String path) from the
preview mixin/manager (refer to handle_download_and_preview_file_mixin and its
tempDirPath usage) and during _clearCacheDir iteration only delete files for
which isPreviewFileInUse returns false (log/skipped files). Also ensure
_getCacheCategories and any cache-scan logic account for skipped files so sizes
remain accurate when files are excluded. Implement one of these fixes and add a
short user-facing note in the settings UI if you choose the skip behavior.
- Around line 108-127: The _clearCacheDir method shows
l10n.cacheClearedSuccessfully even when deletion fails because exceptions are
only logged; change the flow to track success and only show the snack on
success: inside _clearCacheDir (function name) introduce a local bool (e.g.,
success = true) set to false in the catch block where Logs().e(...) is called,
or rethrow/return early on failure, and move the TwakeSnackBar.show(...) call so
it only runs when success is true and context.mounted; ensure setState still
runs as needed only after a successful clear or adjust state updates to reflect
failure appropriately.
---
Nitpick comments:
In
`@lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_view.dart`:
- Around line 85-113: The hardcoded colors used in the progress bar and label
texts should be replaced with the design system colors; update the
LinearProgressIndicator (inside the ClipRRect) to use the appropriate
LinagoraSysColors/LinagoraRefColors token(s) instead of Color(0xFFEEEEEE) and
Color(0xFF006DE2), and replace the label text color usages (the two text widgets
using textTheme.labelSmall) to use the design system color token instead of
Color(0xFF636363); locate these in the ClipRRect -> LinearProgressIndicator and
the two Text widgets (l10n.twake and l10n.available) and swap the hardcoded
Color(...) instances for the corresponding LinagoraSysColors or
LinagoraRefColors constants so theming/maintainability is preserved.
In `@lib/pages/settings_dashboard/settings/settings.dart`:
- Around line 264-265: Replace the hard-coded route string used when handling
SettingEnum.dataAndStorage with the route constant
AppRoutePaths.dataAndStorageFull: in the switch branch that currently calls
context.push('/rooms/dataAndStorage') (the SettingEnum.dataAndStorage case),
change the push target to use AppRoutePaths.dataAndStorageFull so navigation
uses the centralized route constant defined in app_route_paths.dart and cannot
drift from the router definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 174cd193-cc80-4f03-964d-8aa636841ec6
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
assets/l10n/intl_en.arblib/config/go_routes/app_route_paths.dartlib/config/go_routes/go_router.dartlib/domain/model/preview_file/supported_preview_file_types.dartlib/pages/settings_dashboard/settings/settings.dartlib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage.dartlib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_calculator.dartlib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_constants.dartlib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_scanner.dartlib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_view.dartlib/pages/settings_dashboard/settings_style/settings_style_twake_view.dartlib/presentation/enum/settings/settings_enum.dartpubspec.yamltest/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_calculator_test.darttest/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_scanner_test.dart
lib/pages/settings_dashboard/settings_data_and_storage/isolate_storage_scanner_service.dart
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_scanner.dart
Outdated
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage.dart
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage.dart
Show resolved
Hide resolved
Member
|
@tddang-linagora we should to calculate the size of files in |
hoangdat
reviewed
Mar 17, 2026
Comment on lines
+16
to
+27
| static String formatBytes(double bytes) { | ||
| if (bytes >= 1024 * 1024 * 1024) { | ||
| return '${(bytes / (1024 * 1024 * 1024)).toStringAsFixed(1)} GB'; | ||
| } | ||
| if (bytes >= 1024 * 1024) { | ||
| return '${(bytes / (1024 * 1024)).toStringAsFixed(0)} MB'; | ||
| } | ||
| if (bytes >= 1024) { | ||
| return '${(bytes / 1024).toStringAsFixed(0)} KB'; | ||
| } | ||
| return '${bytes.toStringAsFixed(0)} B'; | ||
| } |
Member
There was a problem hiding this comment.
we dont have any 3rd party lib for this?
Collaborator
Author
There was a problem hiding this comment.
To get the right format "number{space}unit"? No.
Collaborator
There was a problem hiding this comment.
This should be in a seperated Util class i think
And comments to make it understandable by people who don't understand how it works
lib/pages/settings_dashboard/settings_data_and_storage/isolate_storage_scanner_service.dart
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage.dart
Show resolved
Hide resolved
Collaborator
Author
We don't use |
hoangdat
reviewed
Mar 19, 2026
lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage.dart
Outdated
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage.dart
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_view.dart
Outdated
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_view.dart
Outdated
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_view.dart
Outdated
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage_view.dart
Outdated
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_data_and_storage/settings_data_and_storage.dart
Outdated
Show resolved
Hide resolved
Member
|
@tddang-linagora please rebase |
nqhhdev
previously approved these changes
Mar 24, 2026
tddang-linagora
force-pushed
the
TW-2912/Mobile-clear-cache
branch
from
02aa8d7 to
7023ad1
Compare
hoangdat
approved these changes
Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
Resolved
Mobile
storage-mobile.mp4
Responsive
storage-responsive.mov
Summary by CodeRabbit
New Features
Bug Fixes
Tests