Conversation
tddang-linagora
requested review from
dab246,
hoangdat and
nqhhdev
as code owners
tddang-linagora
temporarily deployed
to
PR-2883
GitHub Actions
Inactive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces a unified MXC cache system: new cache models and config, Memory and Disk cache tiers (sqflite_ffi mobile; wasm → IndexedDB → memory fallback on web), CacheDatabaseFactory with platform conditional imports and stubs, CacheSchema, DiskCacheTier (SQLite) and DiskCacheTierIndexedDb (web) with eviction/deduplication, MemoryCacheTier, MxcCacheManager with two-tier logic and stats, Http304Validator for conditional revalidation, CacheLifecycleManager and DI registration, MxcImage refactor to use the unified cache (deprecated local cache API), removal of legacy MxcImageCacheManager, web build/prepare script updates, added deps and extensive unit/integration tests. 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@lib/data/cache/database/cache_database_factory.dart`:
- Around line 1-19: The factory currently imports CacheDatabaseWeb and
CacheDatabaseMobile unconditionally causing cross-platform compile issues;
update CacheDatabaseFactory.create (and its imports) to use Dart conditional
imports so only the platform-specific implementation is compiled: replace the
direct imports of CacheDatabaseWeb/CacheDatabaseMobile with a conditional import
that selects CacheDatabaseWeb for web and CacheDatabaseMobile for io (or split
into cache_database_factory_web.dart and cache_database_factory_stub.dart and
use a conditional wrapper import), keeping the CacheDatabaseInterface type and
returning the platform implementation from CacheDatabaseFactory.create.
In `@lib/data/cache/database/cache_database_web.dart`:
- Around line 124-140: The database getter currently calls init() when _database
== null which causes init() to be re-run in IndexedDB mode before the
UnsupportedError is thrown; change the guard to check an _initialized flag
(e.g., _initialized) instead of _database == null, so the getter only calls
init() if !_initialized, then immediately check _activeStorageType ==
CacheStorageType.indexedDb and throw the UnsupportedError (mentioning
DiskCacheTierIndexedDb) without re-triggering init when using IndexedDB; ensure
_initialized is set by init() and used consistently with the _database field.
- Around line 26-33: The init() method currently guards with _database but
IndexedDB mode never sets _database, causing repeated re-initialization; add a
private boolean _initialized on the class (similar to
MxcCacheManager._initialized), change the top guard in init() to if
(_initialized) return;, and set _initialized = true whenever any of the cascade
helpers returns success (i.e., inside the successful branches of
_tryWasmSqlite(), _tryIndexedDb(), and after _useMemoryOnly()); keep _database
assignment behavior unchanged for Wasm/Memory paths but ensure the IndexedDB
path still marks _initialized true even though _database remains null.
- Around line 86-105: The _useMemoryOnly method currently calls
ffi.sqfliteFfiInit() and sets ffi.databaseFactoryFfi which relies on dart:ffi
(invalid on web); replace that FFI-based initialization in _useMemoryOnly with a
web-safe in-memory approach: either use databaseFactoryFfiWeb and open the
database with the ":memory:" path (ensuring you import and reference
databaseFactoryFfiWeb instead of ffi.databaseFactoryFfi) or implement a simple
Map-backed in-memory store used by the cache layer, then set _activeStorageType
= CacheStorageType.memoryOnly and keep the existing log and error handling flow
(remove any rethrow of FFI-specific errors). Ensure you update references inside
_useMemoryOnly (function name) and avoid calling ffi.sqfliteFfiInit or
ffi.databaseFactoryFfi from this web file.
In `@lib/data/cache/mxc_cache_manager.dart`:
- Around line 140-158: clearMemory, clearAll, and evictIfNeeded currently access
late-initialized fields (_memoryTier, _diskTier) without calling
_ensureInitialized(), which can throw LateInitializationError; update each
method (clearMemory, clearAll, evictIfNeeded) to call _ensureInitialized() at
the start (before using _memoryTier/_diskTier) so they raise the descriptive
StateError when not initialized; keep existing behavior otherwise and ensure the
call order is before any accesses to _memoryTier or _diskTier within
init-related flows.
In `@lib/data/cache/tier/memory_cache_tier.dart`:
- Around line 23-37: In put(String key, Uint8List bytes) the oversized-item
check must run before evicting: check if bytes.length > maxSizeBytes and return
immediately (without calling remove, _shouldEvict loop, or modifying
_currentSizeBytes/_cache) to avoid clearing the cache for an item that will be
rejected; then proceed to remove existing entry, run the _shouldEvict/_evictLRU
loop, and finally insert into _cache and update _currentSizeBytes. Ensure
references: put, _shouldEvict, _evictLRU, _cache, _currentSizeBytes,
maxSizeBytes.
In `@lib/main.dart`:
- Around line 45-47: Wrap the await getIt<CacheLifecycleManager>().onAppStart()
call in a try-catch so startup does not crash on cache init errors: catch
Throwable/Exception (and optionally Error), log the full error and stacktrace
using your app logger or crash reporter, do not rethrow, and allow the app to
continue without cache (optionally set a flag on CacheLifecycleManager to mark
cache as disabled); reference the getIt<CacheLifecycleManager>() lookup and the
onAppStart() method when making this change.
In `@lib/utils/cache_lifecycle_manager.dart`:
- Around line 57-70: didChangeAppLifecycleState currently calls
unawaited(_cacheManager.dispose()) on AppLifecycleState.detached which can
double-dispose the underlying MxcCacheManager when dispose() is later invoked;
modify the logic so that either (a) MxcCacheManager exposes a
safeDispose/isDisposed check and didChangeAppLifecycleState calls
_cacheManager.safeDispose() or checks !_cacheManager.isDisposed before calling
dispose(), or (b) wrap the unawaited call in a try-catch and set a flag (e.g.,
_cacheDisposed = true) to prevent a second dispose in the class's dispose()
method; update didChangeAppLifecycleState, dispose, and/or
MxcCacheManager.dispose() accordingly to avoid double-dispose and surface errors
(avoid unhandled futures by awaiting or handling exceptions).
In `@lib/widgets/mxc_image.dart`:
- Line 267: The http.get calls in lib/widgets/mxc_image.dart (the lines creating
"final response = await http.get(httpUri, headers: headers)" and the retry GET
later) lack a timeout and can hang; update both calls to apply a reasonable
timeout (e.g. using .timeout(Duration...)) and catch TimeoutException (and other
network exceptions) so the loading state and retry logic don't block
indefinitely, and ensure the retry code respects the same timeout and error
handling so retries fail-fast and trigger the existing fallback/path for failed
loads.
- Around line 425-428: The error callback in imageErrorWidgetBuilder is mutating
_imageData directly (setting _imageData = null) without calling setState, so the
widget won't rebuild; change the mutation to be wrapped in a setState call
scheduled after the current frame: inside imageErrorWidgetBuilder keep returning
_placeholder(context) immediately, and schedule a
WidgetsBinding.instance.addPostFrameCallback that invokes setState(() =>
_imageData = null) so the state update triggers a rebuild; reference the
existing _imageData field, the imageErrorWidgetBuilder closure, and the
_placeholder(context) helper when implementing this change.
- Around line 133-186: The issue is that _cacheWidth and _cacheHeight (which
access context) are read after awaits in _loadImage and inside _downloadMedia,
risking invalid context; fix by reading and storing local variables (e.g., final
cacheWidth = _cacheWidth; final cacheHeight = _cacheHeight;) immediately at the
start of _loadImage before any await, then use those locals in the later calls
(pass them into _downloadMedia and use them in the _cacheManager.put call) and
update the _downloadMedia signature to accept the cached width/height parameters
so no code reads _cacheWidth/_cacheHeight after an await.
- Around line 280-285: The hardcoded 'image/png' in the _DownloadResult return
corrupts cached MIME types; update the 304/disk-cache code path so it preserves
the original MIME instead of forcing 'image/png': either add a mediaType field
to CacheMetadata and set it when storing responses (ensure _cacheManager.put
receives and persists that mediaType and that cachedMetadata?.mediaType is read
into _DownloadResult.mimeType) or, if modifying CacheMetadata isn't possible,
replace the hardcoded 'image/png' with a safe fallback like
'application/octet-stream' and ensure _cacheManager.put uses that fallback;
adjust references around _DownloadResult, cachedMetadata, and _cacheManager.put
accordingly.
In `@test/data/cache/integration/mxc_cache_integration_test.dart`:
- Around line 100-122: The test ends by creating newManager then disposing it,
but the shared variable cacheManager remains pointing to the old (already
disposed) instance so tearDown will try to dispose/clear it again; fix by
reassigning cacheManager = newManager after await newManager.init() (before the
final awaits/dispose) so subsequent tearDown operates on the active manager, or
alternatively add idempotent guards in MxcCacheManager.dispose() and clearAll()
(check a private `_disposed` flag and no-op if already disposed before calling
`_database.close()` and `_validator.dispose()`) so double-dispose is safe.
In `@test/mixin/message_avatar_mixin_test.dart`:
- Around line 87-89: Tests create a fresh mockCacheManager in setUp but the
existing guard using GetIt.I.isRegistered<MxcCacheManager>() means only the
first mock is ever used; update the test setup to ensure the new mock is
registered each test by either calling GetIt.I.unregister<MxcCacheManager>() (or
unregisterIfRegistered) before registering mockCacheManager in setUp, or enable
reassignment via GetIt.I.allowReassignment = true and then call
GetIt.I.registerSingleton<MxcCacheManager>(mockCacheManager) to replace the
previous instance; reference the setUp, tearDown (if you prefer to cleanup
there), MxcCacheManager, mockCacheManager, GetIt.I.isRegistered and
GetIt.I.registerSingleton symbols when making the change.
🟡 Minor comments (10)
docs/adr/0035-unified-mxc-cache-system.md-145-157 (1)
145-157:⚠️ Potential issue | 🟡 MinorMinor inconsistency in test count.
Line 145 states "93+ passing tests" while line 177 states "86 passing tests." Please reconcile these numbers.
Also applies to: 177-177
test/data/cache/tier/memory_cache_tier_test.dart-99-105 (1)
99-105:⚠️ Potential issue | 🟡 MinorTest should verify existing entries survive an oversized
put.This test only checks that the oversized item isn't cached, but doesn't assert that pre-existing entries are preserved. Adding that assertion would have caught the eviction bug in
put()where all entries are evicted before the size check:Suggested enhancement
test('does not cache item larger than maxSizeBytes', () { + cache.put('small', Uint8List.fromList([1, 2, 3])); final largeBytes = Uint8List.fromList(List.filled(2000, 1)); cache.put('large', largeBytes); expect(cache.get('large'), isNull); - expect(cache.itemCount, equals(0)); + expect(cache.itemCount, equals(1)); + expect(cache.get('small'), isNotNull); // existing entries must survive });lib/data/cache/database/cache_schema.dart-23-36 (1)
23-36:⚠️ Potential issue | 🟡 MinorForeign key enforcement is disabled, but eviction deletion order compensates.
SQLite foreign keys are disabled by default.
PRAGMA foreign_keys = ONis not executed during database initialization incache_database_mobile.dartorcache_database_web.dart, so the foreign key constraint is not actually enforced.However, the eviction logic properly compensates by deleting from
mxc_mappingbeforemedia_cachewithin a transaction (seedisk_cache_tier.dartlines 139–151), preventing orphaned mappings.While this works correctly in practice, relying on application-level deletion order is fragile. Enable FK enforcement with
PRAGMA foreign_keys = ONafter opening the database connection, or addON DELETE CASCADEto the foreign key definition for automatic cleanup and stronger guarantees.test/utils/cache_lifecycle_manager_test.dart-228-233 (1)
228-233:⚠️ Potential issue | 🟡 MinorMisleading test name: "handles errors gracefully" but asserts exception is thrown.
The test name claims graceful error handling, but the assertion
throwsExceptionproves the opposite — the error propagates unhandled. Either:
- The test name should reflect the actual behavior (e.g.,
'propagates init errors'), oronAppStartshould catch and handle the error gracefully (log + continue), and the test should assert no exception is thrown.Which behavior is intended?
test/data/cache/mxc_cache_manager_test.dart-172-191 (1)
172-191:⚠️ Potential issue | 🟡 MinorEviction test is likely a no-op with current config.
MxcCacheConfig.low()hasdiskMaxBytesof 200 MB, but the test only stores 10 × 1000 bytes = 10 KB. Since the cache is far below capacity,evictIfNeeded()won't evict anything, and the assertionlessThanOrEqualTopasses trivially with the size unchanged. This doesn't actually verify eviction behavior.Consider using a tiny config (e.g.,
diskMaxBytes: 5000) so eviction is actually triggered and the size reduction can be asserted withlessThan.Proposed fix
setUp(() async { database = CacheDatabaseMobile(); cacheManager = MxcCacheManager( database: database, - config: MxcCacheConfig.low(), + config: const MxcCacheConfig( + memoryMaxBytes: 25 * 1024 * 1024, + memoryMaxItems: 100, + diskMaxBytes: 5000, // Small enough to trigger eviction in tests + staleThreshold: Duration(hours: 1), + maxAge: Duration(days: 14), + ), );Or create a dedicated test with a tight config just for the eviction scenario.
lib/data/cache/mxc_cache_manager.dart-205-209 (1)
205-209:⚠️ Potential issue | 🟡 MinorThumbnail requests without dimensions silently collide with full-image cache keys.
When
isThumbnail=truebut bothwidthandheightarenull,_buildCacheKeyreturns the base URI — identical to a full-image request. This could cause a thumbnailgetto return the full image (or vice versa) unexpectedly.Consider either appending
?thumb=1even when dimensions are null, or documenting this as intentional behavior.lib/utils/cache_lifecycle_manager.dart-45-49 (1)
45-49:⚠️ Potential issue | 🟡 MinorMisleading comment: "Persist any pending writes" — method actually performs eviction.
evictIfNeeded()runs LRU eviction to trim the disk cache, it does not flush pending writes. The comment should reflect the actual behavior.Proposed fix
/// Called when app goes to background Future<void> onAppBackground() async { - // Persist any pending writes + // Trim disk cache if over limit await _cacheManager.evictIfNeeded(); }lib/data/cache/mxc_cache_manager.dart-184-196 (1)
184-196:⚠️ Potential issue | 🟡 Minor
getStats()also missing_ensureInitialized(), and potential division by zero ondiskUtilization.Same late-field issue as above. Additionally, if
_config.diskMaxBytesis ever 0,diskUtilizationcomputation would produceInfinity/NaN.Proposed fix
Future<MxcCacheStats> getStats() async { + _ensureInitialized(); final memoryStats = _memoryTier.stats; final diskSize = await _diskTier.size(); return MxcCacheStats( memoryItemCount: memoryStats.itemCount, memorySizeBytes: memoryStats.sizeBytes, memoryUtilization: memoryStats.utilizationPercent, diskSizeBytes: diskSize, - diskUtilization: (diskSize / _config.diskMaxBytes) * 100, + diskUtilization: _config.diskMaxBytes > 0 + ? (diskSize / _config.diskMaxBytes) * 100 + : 0, ); }lib/widgets/mxc_image.dart-178-185 (1)
178-185:⚠️ Potential issue | 🟡 MinorComment says "Retry once" but
maxRetrydefaults to 3.Line 181 comment says "Retry once after delay", but the default
maxRetry = 3allows up to 3 retries. Update the comment to match the actual behavior.lib/widgets/mxc_image.dart-403-410 (1)
403-410:⚠️ Potential issue | 🟡 MinorWidget key uses
_imageData.hashCodebut image may load via_filePathalone.When
_filePathis set but_imageDatais null, the key becomesKey('${null.hashCode}')which is alwaysKey('0'). This means theClipRRectwon't get a unique key when a file-path-based image loads, potentially causing stale widget reuse or missing animated transitions.Consider incorporating
_filePathinto the key:- key: Key('${_imageData.hashCode}'), + key: Key('${_imageData.hashCode}_${_filePath.hashCode}'),
🧹 Nitpick comments (18)
scripts/build-web.sh (1)
6-6:flutter pub runis deprecated; preferdart run.
flutter pub runhas been deprecated in favor ofdart run. This applies to both line 6 and line 7.Suggested update
-flutter pub run sqflite_common_ffi_web:setup --dir=web -flutter pub run build_runner build --delete-conflicting-outputs +dart run sqflite_common_ffi_web:setup --dir=web +dart run build_runner build --delete-conflicting-outputslib/data/cache/http_304_validator.dart (1)
100-104:dispose()unconditionally closes a potentially caller-ownedhttp.Client.When an
httpClientis injected via the constructor (e.g., for testing or sharing), calling_httpClient.close()indispose()can break the caller's client. Consider tracking ownership:Proposed fix
class Http304Validator { final http.Client _httpClient; + final bool _ownsClient; final Set<String> _inProgress = {}; final List<_ValidationTask> _queue = []; bool _isProcessing = false; OnCacheUpdated? onCacheUpdated; Http304Validator({http.Client? httpClient}) - : _httpClient = httpClient ?? http.Client(); + : _httpClient = httpClient ?? http.Client(), + _ownsClient = httpClient == null; // ... void dispose() { _queue.clear(); _inProgress.clear(); - _httpClient.close(); + if (_ownsClient) _httpClient.close(); } }pubspec.yaml (1)
213-214: Update dependencies to the latest compatible versions.New versions are available:
sqflite_common_ffi_web: 1.1.1(latest) andidb_shim: 2.8.2+3(latest). While the caret constraints in the current pinning will allow these newer versions, explicitly updating tosqflite_common_ffi_web: ^1.1.1andidb_shim: ^2.8.2+3ensures the latest bug fixes and improvements are captured by default.test/data/cache/database/cache_database_web_test.dart (1)
43-56: Conditional assertions silently pass when the condition isn't met.Tests at Lines 43–48 and 50–56 only assert when the runtime storage type happens to match. In environments where the type differs, these tests pass vacuously. Consider adding a
skipmessage or logging when the condition isn't met so CI results are transparent about what was actually verified.💡 Example with skip logging
test('memory-only storage is not persistent', () async { await database.init(); if (database.storageType == CacheStorageType.memoryOnly) { expect(database.isPersistent, false); + } else { + // ignore: avoid_print + print('Skipped: storageType is ${database.storageType}, not memoryOnly'); } });test/data/cache/http_304_validator_test.dart (2)
252-278: Slow test: 6-second real-time delay in timeout test.This test waits 6 seconds of wall-clock time, which will significantly slow down the test suite in CI. Consider injecting a fake/controllable clock or reducing the simulated delay and timeout values proportionally (e.g., 200ms simulated delay with a 50ms timeout) to keep the test fast while still verifying timeout behavior.
280-297: Post-dispose test lacks meaningful assertions.The test verifies that calling
scheduleRevalidationafterdispose()doesn't throw, but it doesn't assert that the request was actually ignored (e.g., no HTTP call made, no callback invoked). Consider adding aMockClientthat tracks request count and asserting it remains 0 after the post-dispose call.lib/data/cache/database/cache_database_mobile.dart (2)
24-28: Global state mutation on everyinit()call.
sqfliteFfiInit()anddatabaseFactory = databaseFactoryFfimodify process-global state each timeinit()is invoked. While the early return on line 25 guards against re-opening the DB, a secondCacheDatabaseMobileinstance (or test setup) that callsinit()will still execute these global mutations. Consider moving them to a static initializer or a one-time flag.Proposed fix
+ static bool _ffiInitialized = false; + `@override` Future<void> init() async { if (_database != null) return; - ffi.sqfliteFfiInit(); - ffi.databaseFactory = ffi.databaseFactoryFfi; + if (!_ffiInitialized) { + ffi.sqfliteFfiInit(); + ffi.databaseFactory = ffi.databaseFactoryFfi; + _ffiInitialized = true; + }
8-8: Minor: doc comment mentions "Desktop" but desktop is not a supported platform.Based on learnings, only web and mobile targets are supported. Consider updating the comment to just "Mobile".
test/data/cache/mxc_cache_manager_test.dart (2)
44-50: Hardcoded DB path duplicates internal constant fromCacheDatabaseMobile.The path
mxc_cache.dbis hardcoded here but sourced fromCacheDatabaseMobile._dbName. If the constant changes, tearDown silently fails to clean up. Consider usingcustomDbPathin the constructor (likedisk_cache_tier_test.dartdoes) so you control the path explicitly.Proposed fix sketch
+ late String dbPath; + setUp(() async { - database = CacheDatabaseMobile(); + final timestamp = DateTime.now().microsecondsSinceEpoch; + dbPath = '${Directory.systemTemp.path}/mxc_cache_mgr_test_$timestamp.db'; + database = CacheDatabaseMobile(customDbPath: dbPath); cacheManager = MxcCacheManager(tearDown(() async { await cacheManager.clearAll(); await cacheManager.dispose(); try { - final dbFile = File('${Directory.systemTemp.path}/mxc_cache.db'); + final dbFile = File(dbPath); if (await dbFile.exists()) {
54-63: LeakedCacheDatabaseMobileinstance in StateError test.The
uninitializedManagercreates a newCacheDatabaseMobile()that is never disposed. While it's uninitialized so no DB file is created, it's good practice to clean up or use the existingdatabaseinstance.test/widget/message/twake_link_preview_item_test.dart (1)
315-329: Consider extracting duplicate cache-hit mock setup into a shared helper.The
when(mockCacheManager.get(...)).thenAnswer(... CacheResult.hit ...)block is duplicated verbatim in both the "large image" and "small image" tests. A small helper (e.g.,void stubCacheHitForUri(Uri uri)) would reduce duplication and make it easier to update if the mock contract changes.Also applies to: 391-405
test/data/cache/integration/mxc_cache_integration_test.dart (1)
178-203: Type the futures list to get compile-time safety onCacheResultaccess.
futuresisList<Future>(raw), soFuture.wait(futures)returnsList<dynamic>, makingresult.isHiton Line 202 a dynamic dispatch with no compile-time check. Since puts and gets are done in separate phases, you can type the gets list separately.Proposed fix
test('handles concurrent puts and gets', () async { - final futures = <Future>[]; + final putFutures = <Future<void>>[]; // Concurrent puts for (int i = 0; i < 10; i++) { - futures.add( + putFutures.add( cacheManager.put( Uri.parse('mxc://example.com/$i'), Uint8List.fromList([i]), mediaType: 'image/png', ), ); } - await Future.wait(futures); - futures.clear(); + await Future.wait(putFutures); // Concurrent gets + final getFutures = <Future<CacheResult>>[]; for (int i = 0; i < 10; i++) { - futures.add(cacheManager.get(Uri.parse('mxc://example.com/$i'))); + getFutures.add(cacheManager.get(Uri.parse('mxc://example.com/$i'))); } - final results = await Future.wait(futures); + final results = await Future.wait(getFutures); for (final result in results) { expect(result.isHit, isTrue); } });lib/data/cache/tier/disk_cache_tier.dart (3)
113-161:evictLRUloads all entries into memory and performs row-by-row deletes inside a transaction.The current approach fetches all rows (hash + size only, no blobs) to sort by LRU, then deletes one-by-one. This works for moderate cache sizes, but for very large caches the number of rows could cause the transaction to be long-running.
An optimization would be to push the LRU ordering into SQL with a
LIMITor subquery to select only eviction candidates, and batch-delete withWHERE IN (...). This is not urgent but worth considering if cache item counts grow.
163-168:clear()is not transactional — could leave orphan blobs on failure.If the process crashes after deleting mappings but before deleting media cache, orphan blobs will persist. Wrapping both deletes in a transaction would ensure atomicity.
Proposed fix
`@override` Future<void> clear() async { final db = await _cacheDatabase.database; - await db.delete(CacheSchema.tableMxcMapping); - await db.delete(CacheSchema.tableMediaCache); + await db.transaction((txn) async { + await txn.delete(CacheSchema.tableMxcMapping); + await txn.delete(CacheSchema.tableMediaCache); + }); }
17-45:get()performs a write (_markAccessed) inside a read path without a transaction.The mapping lookup (Line 22-27), access count update (Line 34), and blob fetch (Line 36-41) are three separate database operations. While this is acceptable for a cache (stale reads are tolerable), the interleaved write in a read path is worth noting for awareness.
lib/data/cache/tier/disk_cache_tier_indexeddb.dart (2)
52-90:_markAccessedopens a separate write transaction inside a read-onlyget()transaction.While IndexedDB supports concurrent transactions and this works in practice (the read-only transaction auto-completes after its pending requests are fulfilled), calling a write operation mid-read is fragile and could behave differently across
idb_shimimplementations. Consider moving_markAccessedafter the read transaction is fully done, or noting this intentional pattern with a comment.Also, for consistency with
put()(Line 172), consider addingawait txn.completedfor the read-only transaction.
194-247:evictLRUiterates all entries in both stores for eviction — consider using an index.The current implementation loads all media entries (Line 199-205), sorts them in-memory, then iterates all mappings via cursor to find matching hashes (Line 230-236). For the mapping deletion, since there's no index on
contentHashin themxc_mappingstore, every eviction scan is O(n) over all mappings.Adding an index on
contentHashin_storeMxcMappingduringonUpgradeNeededwould allow indexed lookups instead of full-store scans:mappingStore.createIndex('contentHash', 'contentHash');Then use
index('contentHash').openCursor(key: hash)to efficiently find and delete matching mappings.lib/widgets/mxc_image.dart (1)
127-131: In-flight_loadImageis not cancelled on dispose.If
_loadImageis still running when the widget is disposed, it continues executing (network requests, cache writes). While_setLoadResultguardsmounted, the intermediate async work (downloads, cache puts) is wasted. More critically,_cacheWidth/_cacheHeightaccesscontextafter async gaps, which is unsafe after dispose (see next comment).Consider tracking a
_disposedflag or using aCancelableOperationto abort early.
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2883 |
tddang-linagora
temporarily deployed
to
PR-2883
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@lib/data/cache/database/cache_database_factory.dart`:
- Around line 6-7: The conditional import using the `dart.library.html` guard
causes Flutter WASM builds to pull in the native `cache_database_mobile.dart`
(which depends on dart:ffi); update the conditional import so it uses
`dart.library.io` to discriminate native/io platforms and select
`cache_database_mobile.dart` only when `dart.library.io` is available, falling
back to `cache_database_mobile_stub.dart` otherwise; locate the import statement
referencing 'cache_database_mobile.dart' and 'cache_database_mobile_stub.dart'
and replace the `dart.library.html` guard with `dart.library.io` for correct
platform selection.
In `@lib/data/cache/database/cache_database_web.dart`:
- Around line 148-152: The close() implementation should clear the
initialization flag so the DB can be reopened: in CacheDatabaseWeb.close() after
awaiting _database?.close() and setting _database = null, also set _initialized
= false so subsequent calls to init() or the database getter won't short-circuit
returning a closed/null DB; update only the close() method to reset _initialized
while keeping existing nulling/close behavior for _database.
In `@lib/data/cache/mxc_cache_manager.dart`:
- Around line 188-199: getStats() reads late-initialized fields _memoryTier and
_diskTier without ensuring initialization; modify getStats (which returns
MxcCacheStats) to call or await _ensureInitialized() at the start (same pattern
used by clearMemory()/clearAll()) and bail out or return sensible defaults if
initialization fails, then use _memoryTier.stats and await _diskTier.size()
safely to compute the returned MxcCacheStats.diskUtilization and other fields.
In `@lib/data/cache/tier/memory_cache_tier.dart`:
- Line 93: The utilizationPercent getter in MemoryCacheTier currently divides
sizeBytes by maxSizeBytes and can throw when maxSizeBytes == 0; update the
utilizationPercent implementation to defensively handle a zero (or negative)
maxSizeBytes by returning 0.0 (or an appropriate safe value) when maxSizeBytes
<= 0, otherwise compute (sizeBytes / maxSizeBytes) * 100; reference the
utilizationPercent getter and the sizeBytes and maxSizeBytes fields to locate
and modify the code.
🧹 Nitpick comments (6)
lib/data/cache/database/cache_database_mobile.dart (1)
27-47: Concurrentinit()calls can lead to double database opening.In Dart's cooperative async model, two callers of the
databasegetter can both see_database == nullbefore either completesinit(). This results inopenDatabasebeing called twice. Consider caching the initializationFutureor adding aCompleter-based guard:Proposed fix
+ import 'dart:async'; // ... class CacheDatabaseMobile implements CacheDatabaseInterface { // ... ffi.Database? _database; final String? _customDbPath; + Completer<void>? _initCompleter; `@override` Future<void> init() async { - if (_database != null) return; + if (_database != null) return; + if (_initCompleter != null) { + await _initCompleter!.future; + return; + } + _initCompleter = Completer<void>(); ffi.sqfliteFfiInit(); ffi.databaseFactory = ffi.databaseFactoryFfi; // ... rest of init ... + _initCompleter!.complete(); } `@override` Future<void> close() async { await _database?.close(); _database = null; + _initCompleter = null; }lib/data/cache/mxc_cache_manager.dart (2)
209-213: Cache key contains literal"null"when only one dimension is provided.When
thumbnailis true and onlyworhis provided (the other beingnull), the key becomes e.g.mxc://...?w=200&h=null&thumb=1. This could cause cache misses if callers are inconsistent about passing dimensions. Consider omitting null dimensions from the key.Proposed fix
String _buildCacheKey(Uri mxcUri, int? w, int? h, bool thumbnail) { final base = mxcUri.toString(); if (!thumbnail || (w == null && h == null)) return base; - return '$base?w=$w&h=$h&thumb=1'; + final parts = <String>[]; + if (w != null) parts.add('w=$w'); + if (h != null) parts.add('h=$h'); + parts.add('thumb=1'); + return '$base?${parts.join('&')}'; }
140-161: Inconsistent initialization behavior across public methods.
clearMemory()andclearAll()silently no-op when uninitialized, whileevictIfNeeded()throws aStateError. Pick one strategy for consistency — either all throw or all no-op for uninitialized state.test/mixin/message_avatar_mixin_test.dart (1)
49-102: Consider addingtearDownto clean up GetIt registration.Without cleanup, the
MxcCacheManagersingleton leaks into subsequent test files that share the same test runner process. AtearDownortearDownAllthat unregisters it would prevent cross-test pollution.Proposed fix
+ tearDown(() { + if (GetIt.I.isRegistered<MxcCacheManager>()) { + GetIt.I.unregister<MxcCacheManager>(); + } + }); + Future<void> runTest(lib/utils/cache_lifecycle_manager.dart (1)
60-67: Exceptions fromunawaitedfutures are silently lost.Both
unawaited(onAppBackground())andunawaited(dispose())will swallow any exceptions. If eviction or disposal fails, there's no logging or recovery. Consider wrapping with a try-catch that logs:Proposed fix
case AppLifecycleState.paused: case AppLifecycleState.inactive: - unawaited(onAppBackground()); + unawaited(onAppBackground().catchError( + (e, s) => Logs().e('CacheLifecycleManager: background error', e, s), + )); break; case AppLifecycleState.detached: - unawaited(dispose()); + unawaited(dispose().catchError( + (e, s) => Logs().e('CacheLifecycleManager: dispose error', e, s), + )); break;lib/widgets/mxc_image.dart (1)
195-207:_getHttpUrireads_cacheWidth/_cacheHeightindependently from the captured locals.
_getHttpUriat line 146 internally accesses_cacheWidthand_cacheHeight(lines 201–202) for thumbnail dimensions, while_loadImageseparately captures these at lines 144–145. Both happen before anyawaitso they return identical values, but the duplication is fragile — a future refactor moving_getHttpUriafter anawaitwould silently introduce a context-after-async bug. Consider passing the already-captured dimensions into_getHttpUri.Proposed fix
- Uri? _getHttpUri(Uri mxcUri) { + Uri? _getHttpUri(Uri mxcUri, {int? cacheWidth, int? cacheHeight}) { final client = Matrix.of(context).client; return widget.isThumbnail ? mxcUri.getThumbnail( client, - width: _cacheWidth, - height: _cacheHeight, + width: cacheWidth, + height: cacheHeight, animated: widget.animated, method: widget.thumbnailMethod, ) : mxcUri.getDownloadLink(client); }Then at the call site:
- final httpUri = _getHttpUri(mxcUri); + final httpUri = _getHttpUri(mxcUri, cacheWidth: cacheWidth, cacheHeight: cacheHeight);
tddang-linagora
temporarily deployed
to
PR-2883
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/data/cache/mxc_cache_manager.dart`:
- Around line 188-200: The disk utilization calculation in getStats() may divide
by zero when _config.diskMaxBytes is 0; update the getStats() method to guard
the calculation (involving diskSize and _config.diskMaxBytes) by returning 0.0
(or an appropriate sentinel) when _config.diskMaxBytes is zero or null, e.g.,
compute diskUtilization = _config.diskMaxBytes > 0 ? (diskSize /
_config.diskMaxBytes) * 100 : 0.0 before constructing and returning the
MxcCacheStats object.
- Around line 222-226: The _onCacheUpdated callback only writes to the memory
tier (_memoryTier.put) leaving the disk tier stale; to fix, keep the
OnCacheUpdated signature (void Function(String, Uint8List)) but also
fire-and-forget a disk write by calling unawaited(_diskTier.put(...)) inside
_onCacheUpdated so the callback remains synchronous while updating disk in the
background; ensure you pass available validation metadata (etag, lastModified,
dimensions) into _diskTier.put to preserve cache headers, and if those values
aren’t available, write the new bytes with minimal metadata so existing metadata
entries aren’t lost; this change aligns with Http304Validator._validate which
invokes the callback without await.
🧹 Nitpick comments (3)
lib/data/cache/mxc_cache_manager.dart (3)
210-214:_buildCacheKeyembeds literal"null"string for partially-specified thumbnail dimensions.When
isThumbnailistrueand only one ofw/his provided (e.g.,w=100, h=null), the key becomesmxc://...?w=100&h=null&thumb=1. While this is technically consistent, it means a thumbnail request withh=nulland one withhomitted produce different keys if callers are inconsistent. Consider normalizing:Proposed improvement
String _buildCacheKey(Uri mxcUri, int? w, int? h, bool thumbnail) { final base = mxcUri.toString(); if (!thumbnail || (w == null && h == null)) return base; - return '$base?w=$w&h=$h&thumb=1'; + final parts = <String>[]; + if (w != null) parts.add('w=$w'); + if (h != null) parts.add('h=$h'); + parts.add('thumb=1'); + return '$base?${parts.join('&')}'; }
56-104: Verbose debug logging on every cache hit may impact performance in hot paths.Lines 71, 78, and 102 emit
Logs().d(...)on every single cache lookup — including hits from memory. Avatar images are loaded frequently (e.g., scrolling a chat list), so this could generate substantial log noise and string allocations on each frame.Consider demoting these to verbose/trace level or removing them once the feature is stable.
139-152: Consistent guard pattern forclearMemory()/clearAll()vsevictIfNeeded().
clearMemory()andclearAll()gracefully return when not initialized (if (!_initialized) return), butevictIfNeeded()at line 156 calls_ensureInitialized()which throws. Both are maintenance/cleanup operations. Consider aligning these to the same pattern — either all throw or all gracefully no-op.
tddang-linagora
temporarily deployed
to
PR-2883
GitHub Actions
Inactive
tddang-linagora
force-pushed
the
TW-2881/Cache-avatars
branch
from
1a5ac61 to
ce0607f
Compare
tddang-linagora
temporarily deployed
to
PR-2883
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@lib/data/cache/database/cache_database_mobile.dart`:
- Around line 27-55: Concurrent callers can trigger init() twice because they
all observe _database == null; fix by introducing a private
Future<ffi.Database?> _initFuture (or a Completer) to serialize initialization:
in init() check and return _initFuture if set, otherwise assign _initFuture
immediately before calling ffi.openDatabase(...) and await it to set _database;
ensure that on error you clear/reset _initFuture so subsequent attempts can
retry; update the database getter to await/return _initFuture (or call init()
which returns that Future) so concurrent callers share the same initialization
Future rather than opening the DB multiple times.
In `@lib/data/cache/http_304_validator.dart`:
- Around line 100-104: The dispose() method currently always closes the injected
_httpClient; add ownership tracking so only internally-created clients are
closed: introduce a private boolean (e.g. _ownsHttpClient) set to true when you
instantiate a new http.Client in the constructor and set to false when a client
is passed in, then modify dispose() to call _httpClient.close() only if
_ownsHttpClient is true; update the constructor(s) where _httpClient is assigned
to set this flag accordingly.
In `@lib/data/cache/mxc_cache_manager.dart`:
- Around line 213-217: _buildCacheKey currently interpolates null dimensions
producing "w=null" or "h=null"; change it to only include width and height query
params when they are non-null and keep the thumbnail flag when thumbnail==true.
Update the logic in _buildCacheKey (and use Uri.queryParameters or conditional
string appends) so that if w is non-null add "w=<w>", if h is non-null add
"h=<h>", and always add "thumb=1" only when thumbnail is true and at least one
dimension was added; return the base URI unchanged when thumbnail is false or
neither dimension is present.
- Around line 225-237: The background update handler _onCacheUpdated currently
writes bytes to _diskTier.put with a hardcoded mediaType and no validation
metadata, which discards ETag/Last-Modified needed by
Http304Validator._validate; update the OnCacheUpdated typedef (from
http_304_validator.dart) to pass metadata (e.g., etag, lastModified, mediaType)
to callers and change _onCacheUpdated to accept those additional parameters and
forward them into _diskTier.put, or alternatively have _onCacheUpdated read
existing metadata from the disk tier before overwriting and merge/preserve
etag/lastModified/mediaType when calling _diskTier.put so conditional validation
headers are preserved.
In `@lib/data/cache/tier/disk_cache_tier_indexeddb.dart`:
- Around line 229-236: The cursor.delete() Future inside the single-pass
deletion loop isn't awaited, so errors can bypass the surrounding try/catch;
update the loop that iterates mappingStore.openCursor(autoAdvance: true)
(mappingCursorStream) to await cursor.delete() when mapping['contentHash'] is in
hashesToDelete (ensure you call await cursor.delete() instead of
cursor.delete()) so the deletion completes and any exceptions are caught and
handled by the transaction's error handling.
In `@lib/utils/cache_lifecycle_manager.dart`:
- Around line 58-71: didChangeAppLifecycleState currently fire-and-forgets
onAppBackground() and dispose(), which can surface unhandled async errors from
evictIfNeeded() or _cacheManager.dispose(); wrap those async calls with proper
error handling by replacing unawaited(onAppBackground()) and
unawaited(dispose()) with fire-and-forget Futures that catch and log errors
(e.g., onAppBackground().catchError(...) or using .then(...).catchError(...) /
try-catch inside a scheduled Future) so exceptions from onAppBackground,
evictIfNeeded, dispose, or _cacheManager.dispose are handled and do not become
unhandled async errors.
In `@test/data/cache/database/cache_database_factory_test.dart`:
- Around line 4-6: The test imports platform-specific implementations
CacheDatabaseWeb and CacheDatabaseMobile unconditionally causing failures on
non-web VMs; update the test to avoid direct platform imports by either using
conditional imports like the production factory (use the same if
(dart.library.io) pattern to import stubs vs implementations), or mark the test
with `@TestOn`('vm') or `@TestOn`('browser') to restrict execution, or refactor the
test to exercise only the CacheDatabaseFactory interface (e.g., test
CacheDatabaseFactory.create() behavior) without importing
CacheDatabaseWeb/CacheDatabaseMobile directly; apply the change in
cache_database_factory_test.dart referencing the CacheDatabaseWeb and
CacheDatabaseMobile symbols to remove unconditional imports.
In `@test/data/cache/database/cache_database_web_test.dart`:
- Around line 25-29: The test assumes the database getter is always valid but
CacheDatabaseWeb intentionally throws UnsupportedError when storageType ==
CacheStorageType.indexedDb; modify the test around database.init() /
database.database to detect the active storage type (CacheStorageType) and
either skip/assert that an UnsupportedError is thrown for indexedDb or guard the
expect by returning early when storageType == CacheStorageType.indexedDb; update
the test that references database.init(), the database getter, and
CacheDatabaseWeb behavior to handle both paths (valid DB vs unsupported
indexedDb) so the test won't fail in environments where wasm SQLite
initialization falls back to IndexedDB.
In `@test/data/cache/http_304_validator_test.dart`:
- Around line 280-297: Add a disposal guard to make Http304Validator inert after
dispose: introduce a private bool _isDisposed initialized false, set _isDisposed
= true in dispose() after clearing the queue and closing _httpClient, and
early-return from scheduleRevalidation (and any public methods that enqueue
work) when _isDisposed is true so no new tasks are enqueued or _httpClient is
used; optionally check _isDisposed at the start of _validate to avoid using a
closed _httpClient. Use the existing symbols Http304Validator,
scheduleRevalidation, dispose, _httpClient, and _validate when making changes.
In `@test/widget/message/twake_link_preview_item_test.dart`:
- Line 347: The cache-hit widget tests call await
tester.pumpWidget(wrapWithProviders(twakeLinkPreviewItem)) but do not wait for
the MxcImage post-frame async load to complete; update the tests that render
twakeLinkPreviewItem (and the second occurrence around the other test at the
later pumpWidget) to follow the pumpWidget call with await
tester.pumpAndSettle() (or at minimum await tester.pump()) so the
addPostFrameCallback/_loadImage() async work and subsequent setState() have
completed before assertions that may depend on _imageData or loaded state run.
🧹 Nitpick comments (15)
lib/data/cache/database/cache_database_interface.dart (1)
5-7: Nit: doc comment mentions "desktop" which isn't a supported platform.Based on learnings, this project only targets web and mobile. The comment on
sqfliteFfisays "mobile/desktop" — consider changing to just "mobile" for accuracy.- /// Native SQLite on mobile/desktop (sqflite_common_ffi) + /// Native SQLite on mobile (sqflite_common_ffi)lib/data/cache/http_304_validator.dart (2)
22-34: Queue deduplication gap: same URL can be enqueued multiple times.
scheduleRevalidationonly checks_inProgressfor deduplication, but if two calls arrive for the same URL before_processQueuepicks up the first one (i.e., while the queue is idle or between tasks), both get enqueued. The second will execute after the first completes because_inProgressis cleared by then.Consider also checking the queue for pending entries:
Proposed fix
void scheduleRevalidation({ required String mxcUrl, required Uri httpUri, required CacheMetadata metadata, }) { if (_inProgress.contains(mxcUrl)) return; + if (_queue.any((task) => task.mxcUrl == mxcUrl)) return; _queue.add( _ValidationTask(mxcUrl: mxcUrl, httpUri: httpUri, metadata: metadata), ); unawaited(_processQueue()); }
36-59: Consider usingQueueinstead ofListfor O(1) dequeue.
_queue.removeAt(0)on aListis O(n). If many revalidation tasks accumulate, this becomes a hot spot.dart:collection'sQueueprovides O(1)removeFirst().Also, the
asserton line 42 is always true (guarded by thewhilecondition) and can be removed.Proposed fix
+import 'dart:collection'; // ... class Http304Validator { // ... - final List<_ValidationTask> _queue = []; + final Queue<_ValidationTask> _queue = Queue(); // ... Future<void> _processQueue() async { // ... while (_queue.isNotEmpty) { - assert(_queue.isNotEmpty, 'Queue became empty during processing'); - final task = _queue.removeAt(0); + final task = _queue.removeFirst();test/data/cache/http_304_validator_test.dart (1)
252-278: Test uses a real 6-second delay, making it slow.The timeout test at line 272 waits 6 real seconds. Consider using
package:fake_asyncto simulate time advancement and keep the test fast.lib/data/cache/database/cache_database_mobile.dart (1)
11-13: Nit: doc comment mentions "Desktop" — not a supported platform.Same as the interface file; only mobile and web are supported. Consider removing "Desktop" from the class doc.
lib/data/cache/database/cache_database_web.dart (1)
40-52: Side-effect: globaldatabaseFactorymutation could affect other sqflite consumers.Line 42 sets the process-global
ffi.databaseFactory = databaseFactoryFfiWeb. If any other code in the app also uses sqflite with a different factory, this silent override could cause subtle issues. Consider documenting this side-effect or scoping it more narrowly if possible.lib/data/cache/tier/disk_cache_tier.dart (2)
96-109: Orphaned blobs on mapping replacement — potential storage leak.When
put()is called for an existingmxcUrlwith different content,ConflictAlgorithm.replaceupdates the mapping to point to the new content hash. However, the old blob inmedia_cacheis never deleted if no other mapping references it. Over time, these orphaned blobs accumulate and are only reclaimable viaevictLRUorclear().Consider deleting the old blob (if no other mappings reference it) when replacing a mapping:
Sketch
+ // Check for orphaned old blob before upserting mapping + final oldMapping = await txn.query( + CacheSchema.tableMxcMapping, + columns: [CacheSchema.colContentHash], + where: '${CacheSchema.colMxcUrl} = ?', + whereArgs: [mxcUrl], + ); + // Upsert MXC mapping await txn.insert(CacheSchema.tableMxcMapping, { // ... }, conflictAlgorithm: ConflictAlgorithm.replace); + + // Clean up orphaned blob if old hash differs and has no other references + if (oldMapping.isNotEmpty) { + final oldHash = oldMapping.first[CacheSchema.colContentHash] as String; + if (oldHash != contentHash) { + final refCount = await txn.query( + CacheSchema.tableMxcMapping, + where: '${CacheSchema.colContentHash} = ?', + whereArgs: [oldHash], + ); + if (refCount.isEmpty) { + await txn.delete( + CacheSchema.tableMediaCache, + where: '${CacheSchema.colContentHash} = ?', + whereArgs: [oldHash], + ); + } + } + }
163-168:clear()is not transactional — could leave orphaned data on partial failure.If the first
deletesucceeds but the second fails, you end up with orphaned blobs or dangling mappings. Wrapping in a transaction ensures atomicity.Proposed fix
Future<void> clear() async { final db = await _cacheDatabase.database; - await db.delete(CacheSchema.tableMxcMapping); - await db.delete(CacheSchema.tableMediaCache); + await db.transaction((txn) async { + await txn.delete(CacheSchema.tableMxcMapping); + await txn.delete(CacheSchema.tableMediaCache); + }); }test/data/cache/tier/disk_cache_tier_test.dart (1)
25-28: UseCacheSchemaconstants instead of hardcoded table names.Lines 26-27 use hardcoded strings
'mxc_mapping'and'media_cache'for table deletion. If the table names change inCacheSchema, these will silently break. UseCacheSchema.tableMxcMappingandCacheSchema.tableMediaCachefor consistency.Proposed fix
+ import 'package:fluffychat/data/cache/database/cache_schema.dart'; // ... - await (await database.database).delete('mxc_mapping'); - await (await database.database).delete('media_cache'); + await (await database.database).delete(CacheSchema.tableMxcMapping); + await (await database.database).delete(CacheSchema.tableMediaCache);test/utils/cache_lifecycle_manager_test.dart (1)
228-233: Misleading test name — the test proves errors are not handled gracefully.The test asserts
throwsException, confirming thatonAppStartlets the error propagate. The name "handles errors in cache operations gracefully" suggests the opposite. The graceful handling actually happens inmain.dart's try-catch wrapper, not inCacheLifecycleManageritself.Consider renaming to reflect the actual behavior:
Suggested rename
- test('handles errors in cache operations gracefully', () async { + test('onAppStart propagates initialization errors to caller', () async { when(mockCacheManager.init()).thenThrow(Exception('Init failed')); - // Should not throw expect(() => lifecycleManager.onAppStart(), throwsException); });test/data/cache/mxc_cache_manager_test.dart (1)
18-26:setMockMethodCallHandleris deprecated.The
MethodChannel.setMockMethodCallHandlerAPI is deprecated. The recommended replacement isTestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger.setMockMethodCallHandler.test/data/cache/integration/mxc_cache_integration_test.dart (1)
177-204: Concurrent operations test is a good addition.Testing
Future.waitwith concurrent puts followed by concurrent gets validates that the cache handles parallel access correctly. The type ofresultsisList<dynamic>due toFuture.wait(futures)wherefuturesisList<Future>— consider typing the list asList<Future<CacheResult>>for the gets to make the assertions type-safe, but this is minor.lib/widgets/mxc_image.dart (2)
326-331: Default MIME type'image/png'for 200 responses may misrepresent non-PNG content.Lines 314 and 328 both fall back to
'image/png'when thecontent-typeheader is absent. While this only affects cache metadata (not rendering), using a generic fallback like'application/octet-stream'(as done on the 304 path at line 296) would be more consistent and accurate.Suggested fix
return _DownloadResult( bytes: retryResponse.bodyBytes, - mimeType: retryResponse.headers['content-type'] ?? 'image/png', + mimeType: retryResponse.headers['content-type'] ?? 'application/octet-stream', etag: retryResponse.headers['etag'], lastModified: retryResponse.headers['last-modified'], ); } // ... return _DownloadResult( bytes: response.bodyBytes, - mimeType: response.headers['content-type'] ?? 'image/png', + mimeType: response.headers['content-type'] ?? 'application/octet-stream', etag: response.headers['etag'], lastModified: response.headers['last-modified'], );
196-207:_getHttpUrire-reads_cacheWidth/_cacheHeightindependently of the captured locals.At line 146,
_getHttpUri(mxcUri)is called synchronously (before anyawait), so this is safe. However, lines 200-202 read_cacheWidthand_cacheHeightagain rather than using the values captured at lines 144-145. If this method were ever called after anawaitin the future, it would reintroduce the context-after-async issue.Consider passing the pre-captured dimensions as parameters for consistency:
Suggested refactor
- Uri? _getHttpUri(Uri mxcUri) { + Uri? _getHttpUri(Uri mxcUri, {int? cacheWidth, int? cacheHeight}) { final client = Matrix.of(context).client; return widget.isThumbnail ? mxcUri.getThumbnail( client, - width: _cacheWidth, - height: _cacheHeight, + width: cacheWidth, + height: cacheHeight, animated: widget.animated, method: widget.thumbnailMethod, ) : mxcUri.getDownloadLink(client); }Then at line 146:
- final httpUri = _getHttpUri(mxcUri); + final httpUri = _getHttpUri(mxcUri, cacheWidth: cacheWidth, cacheHeight: cacheHeight);lib/data/cache/mxc_cache_manager.dart (1)
140-153: Inconsistent initialization guard:clearMemory/clearAllsilently no-op, butevictIfNeededthrows.Lines 142 and 149 use
if (!_initialized) returnwhile line 157 uses_ensureInitialized()(which throwsStateError). This inconsistency may confuse callers. Consider documenting the rationale or unifying the behavior — either all lifecycle-safe methods no-op, or all throw.
| Future<void> init() async { | ||
| if (_database != null) return; | ||
|
|
||
| ffi.sqfliteFfiInit(); | ||
| ffi.databaseFactory = ffi.databaseFactoryFfi; | ||
|
|
||
| final String dbPath; | ||
| if (_customDbPath != null) { | ||
| dbPath = _customDbPath; | ||
| } else { | ||
| final dir = await getApplicationDocumentsDirectory(); | ||
| dbPath = join(dir.path, _dbName); | ||
| } | ||
|
|
||
| _database = await ffi.openDatabase( | ||
| dbPath, | ||
| version: _dbVersion, | ||
| onCreate: _onCreate, | ||
| onUpgrade: _onUpgrade, | ||
| ); | ||
| } | ||
|
|
||
| @override | ||
| Future<ffi.Database> get database async { | ||
| if (_database == null) { | ||
| await init(); | ||
| } | ||
| return _database!; | ||
| } |
There was a problem hiding this comment.
Race condition: concurrent calls to init() or the database getter can open the database twice.
If two callers access database concurrently, both see _database == null, both enter init(), and both call ffi.openDatabase(...). The first handle is leaked (never closed), and global state may be corrupted.
Guard initialization with a stored Future (or a Completer):
Proposed fix
class CacheDatabaseMobile implements CacheDatabaseInterface {
static const String _dbName = 'mxc_cache.db';
static const int _dbVersion = 1;
ffi.Database? _database;
+ Future<void>? _initFuture;
final String? _customDbPath;
CacheDatabaseMobile({String? customDbPath}) : _customDbPath = customDbPath;
`@override`
Future<void> init() async {
- if (_database != null) return;
+ if (_database != null) return;
+ _initFuture ??= _doInit();
+ await _initFuture;
+ }
+ Future<void> _doInit() async {
ffi.sqfliteFfiInit();
ffi.databaseFactory = ffi.databaseFactoryFfi;
final String dbPath;
if (_customDbPath != null) {
dbPath = _customDbPath;
} else {
final dir = await getApplicationDocumentsDirectory();
dbPath = join(dir.path, _dbName);
}
_database = await ffi.openDatabase(
dbPath,
version: _dbVersion,
onCreate: _onCreate,
onUpgrade: _onUpgrade,
);
}
`@override`
Future<ffi.Database> get database async {
if (_database == null) {
await init();
}
return _database!;
}📝 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.
| Future<void> init() async { | |
| if (_database != null) return; | |
| ffi.sqfliteFfiInit(); | |
| ffi.databaseFactory = ffi.databaseFactoryFfi; | |
| final String dbPath; | |
| if (_customDbPath != null) { | |
| dbPath = _customDbPath; | |
| } else { | |
| final dir = await getApplicationDocumentsDirectory(); | |
| dbPath = join(dir.path, _dbName); | |
| } | |
| _database = await ffi.openDatabase( | |
| dbPath, | |
| version: _dbVersion, | |
| onCreate: _onCreate, | |
| onUpgrade: _onUpgrade, | |
| ); | |
| } | |
| @override | |
| Future<ffi.Database> get database async { | |
| if (_database == null) { | |
| await init(); | |
| } | |
| return _database!; | |
| } | |
| `@override` | |
| Future<void> init() async { | |
| if (_database != null) return; | |
| _initFuture ??= _doInit(); | |
| await _initFuture; | |
| } | |
| Future<void> _doInit() async { | |
| ffi.sqfliteFfiInit(); | |
| ffi.databaseFactory = ffi.databaseFactoryFfi; | |
| final String dbPath; | |
| if (_customDbPath != null) { | |
| dbPath = _customDbPath; | |
| } else { | |
| final dir = await getApplicationDocumentsDirectory(); | |
| dbPath = join(dir.path, _dbName); | |
| } | |
| _database = await ffi.openDatabase( | |
| dbPath, | |
| version: _dbVersion, | |
| onCreate: _onCreate, | |
| onUpgrade: _onUpgrade, | |
| ); | |
| } | |
| `@override` | |
| Future<ffi.Database> get database async { | |
| if (_database == null) { | |
| await init(); | |
| } | |
| return _database!; | |
| } |
🤖 Prompt for AI Agents
In `@lib/data/cache/database/cache_database_mobile.dart` around lines 27 - 55,
Concurrent callers can trigger init() twice because they all observe _database
== null; fix by introducing a private Future<ffi.Database?> _initFuture (or a
Completer) to serialize initialization: in init() check and return _initFuture
if set, otherwise assign _initFuture immediately before calling
ffi.openDatabase(...) and await it to set _database; ensure that on error you
clear/reset _initFuture so subsequent attempts can retry; update the database
getter to await/return _initFuture (or call init() which returns that Future) so
concurrent callers share the same initialization Future rather than opening the
DB multiple times.
| void dispose() { | ||
| _queue.clear(); | ||
| _inProgress.clear(); | ||
| _httpClient.close(); | ||
| } |
There was a problem hiding this comment.
dispose() unconditionally closes an externally-provided httpClient.
When a caller injects their own http.Client via the constructor, dispose() will close it, potentially breaking other consumers of that client. Track ownership and only close the client if it was internally created.
Proposed fix
class Http304Validator {
final http.Client _httpClient;
+ final bool _ownsHttpClient;
final Set<String> _inProgress = {};
final List<_ValidationTask> _queue = [];
bool _isProcessing = false;
OnCacheUpdated? onCacheUpdated;
Http304Validator({http.Client? httpClient})
- : _httpClient = httpClient ?? http.Client();
+ : _httpClient = httpClient ?? http.Client(),
+ _ownsHttpClient = httpClient == null;
// ...
void dispose() {
_queue.clear();
_inProgress.clear();
- _httpClient.close();
+ if (_ownsHttpClient) {
+ _httpClient.close();
+ }
}
}🤖 Prompt for AI Agents
In `@lib/data/cache/http_304_validator.dart` around lines 100 - 104, The dispose()
method currently always closes the injected _httpClient; add ownership tracking
so only internally-created clients are closed: introduce a private boolean (e.g.
_ownsHttpClient) set to true when you instantiate a new http.Client in the
constructor and set to false when a client is passed in, then modify dispose()
to call _httpClient.close() only if _ownsHttpClient is true; update the
constructor(s) where _httpClient is assigned to set this flag accordingly.
| String _buildCacheKey(Uri mxcUri, int? w, int? h, bool thumbnail) { | ||
| final base = mxcUri.toString(); | ||
| if (!thumbnail || (w == null && h == null)) return base; | ||
| return '$base?w=$w&h=$h&thumb=1'; | ||
| } |
There was a problem hiding this comment.
_buildCacheKey embeds literal "null" when only one dimension is provided.
When isThumbnail is true but only one of width/height is non-null, Dart's string interpolation produces keys like mxc://...?w=null&h=100&thumb=1. This could cause cache misses or collisions depending on caller patterns.
Proposed fix
String _buildCacheKey(Uri mxcUri, int? w, int? h, bool thumbnail) {
final base = mxcUri.toString();
if (!thumbnail || (w == null && h == null)) return base;
- return '$base?w=$w&h=$h&thumb=1';
+ final parts = <String>[];
+ if (w != null) parts.add('w=$w');
+ if (h != null) parts.add('h=$h');
+ parts.add('thumb=1');
+ return '$base?${parts.join('&')}';
}🤖 Prompt for AI Agents
In `@lib/data/cache/mxc_cache_manager.dart` around lines 213 - 217, _buildCacheKey
currently interpolates null dimensions producing "w=null" or "h=null"; change it
to only include width and height query params when they are non-null and keep
the thumbnail flag when thumbnail==true. Update the logic in _buildCacheKey (and
use Uri.queryParameters or conditional string appends) so that if w is non-null
add "w=<w>", if h is non-null add "h=<h>", and always add "thumb=1" only when
thumbnail is true and at least one dimension was added; return the base URI
unchanged when thumbnail is false or neither dimension is present.
| void _onCacheUpdated(String mxcUrl, Uint8List newBytes) { | ||
| // Update memory cache with fresh content | ||
| _memoryTier.put(mxcUrl, newBytes); | ||
| // Also update disk tier using fire-and-forget | ||
| unawaited( | ||
| _diskTier.put( | ||
| mxcUrl: mxcUrl, | ||
| bytes: newBytes, | ||
| mediaType: 'application/octet-stream', | ||
| ), | ||
| ); | ||
| Logs().d('MxcCacheManager: updated $mxcUrl from background revalidation'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "http_304_validator.dart" --exec cat -n {} \;Repository: linagora/twake-on-matrix
Length of output: 4100
🏁 Script executed:
cat -n lib/data/cache/mxc_cache_manager.dart | head -300Repository: linagora/twake-on-matrix
Length of output: 9196
🏁 Script executed:
fd -t f "cache_metadata.dart" --exec cat -n {} \;Repository: linagora/twake-on-matrix
Length of output: 1429
🏁 Script executed:
fd -t f "cache_tier_interface.dart" --exec cat -n {} \;Repository: linagora/twake-on-matrix
Length of output: 1315
Stale-while-revalidate chain breaks due to missing response metadata in background updates.
_onCacheUpdated writes to disk with hardcoded 'application/octet-stream' media type and no etag or lastModified. When this overwrites the existing disk entry, the next revalidation cycle loses the validation headers needed for If-None-Match and If-Modified-Since conditional requests, forcing full content refetches.
The root cause: Http304Validator._validate() receives the complete HTTP 200 response with headers but the OnCacheUpdated typedef (line 8 of http_304_validator.dart) only passes mxcUrl and newBytes, discarding headers entirely. _onCacheUpdated then has no way to preserve the response metadata.
Recommended fixes:
- Expand
OnCacheUpdatedtypedef to pass metadata:typedef OnCacheUpdated = void Function(String mxcUrl, Uint8List newBytes, String? etag, String? lastModified, String? mediaType); - Or read existing metadata before disk write to preserve what can be recovered.
🤖 Prompt for AI Agents
In `@lib/data/cache/mxc_cache_manager.dart` around lines 225 - 237, The background
update handler _onCacheUpdated currently writes bytes to _diskTier.put with a
hardcoded mediaType and no validation metadata, which discards
ETag/Last-Modified needed by Http304Validator._validate; update the
OnCacheUpdated typedef (from http_304_validator.dart) to pass metadata (e.g.,
etag, lastModified, mediaType) to callers and change _onCacheUpdated to accept
those additional parameters and forward them into _diskTier.put, or
alternatively have _onCacheUpdated read existing metadata from the disk tier
before overwriting and merge/preserve etag/lastModified/mediaType when calling
_diskTier.put so conditional validation headers are preserved.
| // Single pass deletion of mappings | ||
| final mappingCursorStream = mappingStore.openCursor(autoAdvance: true); | ||
| await for (final cursor in mappingCursorStream) { | ||
| final mapping = cursor.value as Map; | ||
| if (hashesToDelete.contains(mapping['contentHash'])) { | ||
| cursor.delete(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
idb_shim CursorWithValue delete method return type Dart
💡 Result:
In Dart (and thus in idb_shim, which follows dart:indexed_db), CursorWithValue.delete() has return type:
Future(i.e. effectivelyFuture<dynamic>/Future<void>in usage) (api.dart.dev)
It completes when the delete request finishes; IndexedDB’s underlying cursor delete() operation conceptually returns “undefined” on success. (developer.mozilla.org)
Citations:
- 1: https://api.dart.dev/dart-indexed_db/Cursor/delete.html
- 2: https://developer.mozilla.org/en-US/docs/Web/API/IDBCursor/delete?utm_source=openai
cursor.delete() is not awaited — add await for proper error handling.
On line 234, cursor.delete() returns a Future that completes when the delete operation finishes. Not awaiting it means exceptions won't be caught by the surrounding try/catch block and could surface unexpectedly on txn.completed. IndexedDB queues the operation within the transaction, but awaiting ensures errors are properly handled.
Proposed fix
final mappingCursorStream = mappingStore.openCursor(autoAdvance: true);
await for (final cursor in mappingCursorStream) {
final mapping = cursor.value as Map;
if (hashesToDelete.contains(mapping['contentHash'])) {
- cursor.delete();
+ await cursor.delete();
}
}📝 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.
| // Single pass deletion of mappings | |
| final mappingCursorStream = mappingStore.openCursor(autoAdvance: true); | |
| await for (final cursor in mappingCursorStream) { | |
| final mapping = cursor.value as Map; | |
| if (hashesToDelete.contains(mapping['contentHash'])) { | |
| cursor.delete(); | |
| } | |
| } | |
| // Single pass deletion of mappings | |
| final mappingCursorStream = mappingStore.openCursor(autoAdvance: true); | |
| await for (final cursor in mappingCursorStream) { | |
| final mapping = cursor.value as Map; | |
| if (hashesToDelete.contains(mapping['contentHash'])) { | |
| await cursor.delete(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@lib/data/cache/tier/disk_cache_tier_indexeddb.dart` around lines 229 - 236,
The cursor.delete() Future inside the single-pass deletion loop isn't awaited,
so errors can bypass the surrounding try/catch; update the loop that iterates
mappingStore.openCursor(autoAdvance: true) (mappingCursorStream) to await
cursor.delete() when mapping['contentHash'] is in hashesToDelete (ensure you
call await cursor.delete() instead of cursor.delete()) so the deletion completes
and any exceptions are caught and handled by the transaction's error handling.
| @override | ||
| void didChangeAppLifecycleState(AppLifecycleState state) { | ||
| switch (state) { | ||
| case AppLifecycleState.paused: | ||
| case AppLifecycleState.inactive: | ||
| unawaited(onAppBackground()); | ||
| break; | ||
| case AppLifecycleState.detached: | ||
| unawaited(dispose()); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Unhandled errors from unawaited calls in lifecycle callbacks.
onAppBackground() (line 63) and dispose() (line 66) are fire-and-forget. If evictIfNeeded() or _cacheManager.dispose() throws, these become unhandled async errors. Since didChangeAppLifecycleState is a synchronous override, consider wrapping the async work with error handling:
Suggested fix
`@override`
void didChangeAppLifecycleState(AppLifecycleState state) {
switch (state) {
case AppLifecycleState.paused:
case AppLifecycleState.inactive:
- unawaited(onAppBackground());
+ unawaited(onAppBackground().catchError((e, s) {
+ Logs().e('CacheLifecycleManager: background error', e, s);
+ }));
break;
case AppLifecycleState.detached:
- unawaited(dispose());
+ unawaited(dispose().catchError((e, s) {
+ Logs().e('CacheLifecycleManager: dispose error', e, s);
+ }));
break;
default:
break;
}
}📝 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.
| @override | |
| void didChangeAppLifecycleState(AppLifecycleState state) { | |
| switch (state) { | |
| case AppLifecycleState.paused: | |
| case AppLifecycleState.inactive: | |
| unawaited(onAppBackground()); | |
| break; | |
| case AppLifecycleState.detached: | |
| unawaited(dispose()); | |
| break; | |
| default: | |
| break; | |
| } | |
| } | |
| `@override` | |
| void didChangeAppLifecycleState(AppLifecycleState state) { | |
| switch (state) { | |
| case AppLifecycleState.paused: | |
| case AppLifecycleState.inactive: | |
| unawaited(onAppBackground().catchError((e, s) { | |
| Logs().e('CacheLifecycleManager: background error', e, s); | |
| })); | |
| break; | |
| case AppLifecycleState.detached: | |
| unawaited(dispose().catchError((e, s) { | |
| Logs().e('CacheLifecycleManager: dispose error', e, s); | |
| })); | |
| break; | |
| default: | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@lib/utils/cache_lifecycle_manager.dart` around lines 58 - 71,
didChangeAppLifecycleState currently fire-and-forgets onAppBackground() and
dispose(), which can surface unhandled async errors from evictIfNeeded() or
_cacheManager.dispose(); wrap those async calls with proper error handling by
replacing unawaited(onAppBackground()) and unawaited(dispose()) with
fire-and-forget Futures that catch and log errors (e.g.,
onAppBackground().catchError(...) or using .then(...).catchError(...) /
try-catch inside a scheduled Future) so exceptions from onAppBackground,
evictIfNeeded, dispose, or _cacheManager.dispose are handled and do not become
unhandled async errors.
| import 'package:fluffychat/data/cache/database/cache_database_factory.dart'; | ||
| import 'package:fluffychat/data/cache/database/cache_database_mobile.dart'; | ||
| import 'package:fluffychat/data/cache/database/cache_database_web.dart'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine the test file
echo "=== Test file: cache_database_factory_test.dart ==="
cat -n test/data/cache/database/cache_database_factory_test.dart
echo ""
echo "=== cache_database_web.dart (first 30 lines) ==="
head -30 lib/data/cache/database/cache_database_web.dart
echo ""
echo "=== cache_database_mobile.dart (first 30 lines) ==="
head -30 lib/data/cache/database/cache_database_mobile.dart
echo ""
echo "=== cache_database_factory.dart ==="
cat lib/data/cache/database/cache_database_factory.dartRepository: linagora/twake-on-matrix
Length of output: 4399
🏁 Script executed:
#!/bin/bash
# Check for stub files and see how the test should be properly written
echo "=== Looking for stub files ==="
find lib/data/cache/database -name "*stub*" -type f
echo ""
echo "=== Check pubspec.yaml for dependency info ==="
rg "idb_shim|sqflite_common_ffi_web|path_provider" pubspec.yaml -A 2 -B 2
echo ""
echo "=== Check for `@TestOn` annotations in test file ==="
head -10 test/data/cache/database/cache_database_factory_test.dart
echo ""
echo "=== Check if there are other similar test files for reference ==="
find test -name "*factory_test*" -type fRepository: linagora/twake-on-matrix
Length of output: 1367
🏁 Script executed:
#!/bin/bash
# Verify what idb_shim/idb_browser.dart actually contains to confirm web-only nature
echo "=== Check idb_shim dependency in pubspec.yaml ==="
rg "idb_shim" pubspec.yaml -B 1 -A 1
# Check if project runs tests successfully to see if this is actually an issue
echo ""
echo "=== Look for test configuration or CI setup ==="
find . -name "*.yml" -o -name "*.yaml" | grep -i "test\|ci\|github" | head -5
</parameter>Repository: linagora/twake-on-matrix
Length of output: 713
🏁 Script executed:
#!/bin/bash
# Check the build workflow to understand how tests are run
echo "=== .github/workflows/build.yaml ==="
cat .github/workflows/build.yaml | head -100Repository: linagora/twake-on-matrix
Length of output: 2886
🏁 Script executed:
#!/bin/bash
# Final verification: Check the actual nature of idb_shim/idb_browser import
# by looking at what idb_shim exports and where it's available
echo "=== Verify idb_shim is web-only by checking its pub.dev info ==="
# Search for documentation about idb_shim platform availability
rg -i "platforms|web.?only" lib/data/cache/database/cache_database_web.dart -B 5 -A 5Repository: linagora/twake-on-matrix
Length of output: 132
Remove unconditional imports of platform-specific implementations from the test file.
This test imports both CacheDatabaseWeb (which depends on idb_shim/idb_browser.dart, a web-only package requiring dart:html) and CacheDatabaseMobile (which imports platform-specific packages like path_provider). When running flutter test on non-web platforms, the cache_database_web.dart import will fail because browser-only dependencies are unavailable on the VM.
The factory already demonstrates the correct pattern using conditional imports with if (dart.library.io) and stub files. The test should either:
- Use conditional imports like the factory (import stubs on web, real implementations on VM)
- Add
@TestOn('vm')or@TestOn('browser')to separate platform-specific tests - Test only the factory interface without importing concrete implementations
🤖 Prompt for AI Agents
In `@test/data/cache/database/cache_database_factory_test.dart` around lines 4 -
6, The test imports platform-specific implementations CacheDatabaseWeb and
CacheDatabaseMobile unconditionally causing failures on non-web VMs; update the
test to avoid direct platform imports by either using conditional imports like
the production factory (use the same if (dart.library.io) pattern to import
stubs vs implementations), or mark the test with `@TestOn`('vm') or
`@TestOn`('browser') to restrict execution, or refactor the test to exercise only
the CacheDatabaseFactory interface (e.g., test CacheDatabaseFactory.create()
behavior) without importing CacheDatabaseWeb/CacheDatabaseMobile directly; apply
the change in cache_database_factory_test.dart referencing the CacheDatabaseWeb
and CacheDatabaseMobile symbols to remove unconditional imports.
| test('database getter returns valid database', () async { | ||
| await database.init(); | ||
| final db = await database.database; | ||
| expect(db, isNotNull); | ||
| }); |
There was a problem hiding this comment.
Test will fail when IndexedDB is the active storage backend.
The database getter throws UnsupportedError when storageType == CacheStorageType.indexedDb (by design in CacheDatabaseWeb). If the test environment's wasm SQLite initialization fails and IndexedDB is selected, this test will throw unexpectedly.
Guard the assertion or skip based on storage type:
Proposed fix
test('database getter returns valid database', () async {
await database.init();
+ if (database.storageType == CacheStorageType.indexedDb) {
+ expect(
+ () async => await database.database,
+ throwsUnsupportedError,
+ );
+ return;
+ }
final db = await database.database;
expect(db, isNotNull);
});📝 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.
| test('database getter returns valid database', () async { | |
| await database.init(); | |
| final db = await database.database; | |
| expect(db, isNotNull); | |
| }); | |
| test('database getter returns valid database', () async { | |
| await database.init(); | |
| if (database.storageType == CacheStorageType.indexedDb) { | |
| expect( | |
| () async => await database.database, | |
| throwsUnsupportedError, | |
| ); | |
| return; | |
| } | |
| final db = await database.database; | |
| expect(db, isNotNull); | |
| }); |
🤖 Prompt for AI Agents
In `@test/data/cache/database/cache_database_web_test.dart` around lines 25 - 29,
The test assumes the database getter is always valid but CacheDatabaseWeb
intentionally throws UnsupportedError when storageType ==
CacheStorageType.indexedDb; modify the test around database.init() /
database.database to detect the active storage type (CacheStorageType) and
either skip/assert that an UnsupportedError is thrown for indexedDb or guard the
expect by returning early when storageType == CacheStorageType.indexedDb; update
the test that references database.init(), the database getter, and
CacheDatabaseWeb behavior to handle both paths (valid DB vs unsupported
indexedDb) so the test won't fail in environments where wasm SQLite
initialization falls back to IndexedDB.
| test('dispose clears queue and in-progress tasks', () { | ||
| final validator = Http304Validator(); | ||
|
|
||
| validator.scheduleRevalidation( | ||
| mxcUrl: 'mxc://example.com/test', | ||
| httpUri: Uri.parse('https://example.com/media/test'), | ||
| metadata: CacheMetadata(etag: '"abc"', createdAt: DateTime.now()), | ||
| ); | ||
|
|
||
| validator.dispose(); | ||
|
|
||
| // After dispose, scheduling should not cause any issues | ||
| validator.scheduleRevalidation( | ||
| mxcUrl: 'mxc://example.com/test2', | ||
| httpUri: Uri.parse('https://example.com/media/test2'), | ||
| metadata: CacheMetadata(etag: '"def"', createdAt: DateTime.now()), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Test validates use-after-dispose, but this actually exercises undefined behavior.
After dispose() at line 289, _httpClient is already closed. The subsequent scheduleRevalidation at line 292 will enqueue a task and attempt to use the closed client, which may throw a ClientException. The test passes only because the error is silently swallowed in _validate's catch block — it doesn't assert any meaningful post-dispose invariant.
If the intent is to verify that post-dispose calls don't crash, add an explicit comment and verify no callbacks fire. If the intent is that dispose should make the validator inert, consider guarding scheduleRevalidation with an _isDisposed flag.
Suggested improvement: guard against post-dispose usage
In Http304Validator:
+ bool _isDisposed = false;
+
void dispose() {
+ _isDisposed = true;
_queue.clear();
_inProgress.clear();
- _httpClient.close();
+ if (_ownsHttpClient) {
+ _httpClient.close();
+ }
}
void scheduleRevalidation({...}) {
+ if (_isDisposed) return;
if (_inProgress.contains(mxcUrl)) return;
// ...
}Then in the test:
test('dispose clears queue and in-progress tasks', () {
// ...
validator.dispose();
- // After dispose, scheduling should not cause any issues
+ // After dispose, scheduling should be silently ignored
validator.scheduleRevalidation(
mxcUrl: 'mxc://example.com/test2',
httpUri: Uri.parse('https://example.com/media/test2'),
metadata: CacheMetadata(etag: '"def"', createdAt: DateTime.now()),
);
+ // No crash, no processing — validator is inert
});🤖 Prompt for AI Agents
In `@test/data/cache/http_304_validator_test.dart` around lines 280 - 297, Add a
disposal guard to make Http304Validator inert after dispose: introduce a private
bool _isDisposed initialized false, set _isDisposed = true in dispose() after
clearing the queue and closing _httpClient, and early-return from
scheduleRevalidation (and any public methods that enqueue work) when _isDisposed
is true so no new tasks are enqueued or _httpClient is used; optionally check
_isDisposed at the start of _validate to avoid using a closed _httpClient. Use
the existing symbols Http304Validator, scheduleRevalidation, dispose,
_httpClient, and _validate when making changes.
| await tester.pumpWidget( | ||
| MaterialApp(home: Scaffold(body: twakeLinkPreviewItem)), | ||
| ); | ||
| await tester.pumpWidget(wrapWithProviders(twakeLinkPreviewItem)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.dart" | xargs grep -l "class MxcImage" | head -5Repository: linagora/twake-on-matrix
Length of output: 96
🏁 Script executed:
cat -n test/widget/message/twake_link_preview_item_test.dart | sed -n '310,430p'Repository: linagora/twake-on-matrix
Length of output: 4860
🏁 Script executed:
ast-grep --pattern $'class MxcImage extends $_'Repository: linagora/twake-on-matrix
Length of output: 50
🏁 Script executed:
cat -n ./lib/widgets/mxc_image.dart | head -150Repository: linagora/twake-on-matrix
Length of output: 5949
🏁 Script executed:
wc -l ./lib/widgets/mxc_image.dartRepository: linagora/twake-on-matrix
Length of output: 100
🏁 Script executed:
cat -n ./lib/widgets/mxc_image.dart | sed -n '120,250p'Repository: linagora/twake-on-matrix
Length of output: 4605
🏁 Script executed:
cat -n ./lib/widgets/mxc_image.dart | sed -n '260,350p'Repository: linagora/twake-on-matrix
Length of output: 3604
🏁 Script executed:
cat -n test/widget/message/twake_link_preview_item_test.dart | sed -n '373,384p'Repository: linagora/twake-on-matrix
Length of output: 492
🏁 Script executed:
cat -n test/widget/message/twake_link_preview_item_test.dart | sed -n '427,430p'Repository: linagora/twake-on-matrix
Length of output: 216
🏁 Script executed:
# Check if there are any assertions that depend on the loaded image state
cat -n test/widget/message/twake_link_preview_item_test.dart | sed -n '310,384p' | grep -A 5 "pumpWidget"Repository: linagora/twake-on-matrix
Length of output: 333
🏁 Script executed:
# Verify the complete test assertions after pumpWidget for both tests
cat -n test/widget/message/twake_link_preview_item_test.dart | sed -n '345,384p'Repository: linagora/twake-on-matrix
Length of output: 1518
Add pumpAndSettle() after pumpWidget() in cache-hit tests for proper async handling.
MxcImage schedules image loading via addPostFrameCallback(() => _loadImage()) in initState(), which fires an async operation that awaits the cache manager and later calls setState(). Without await tester.pumpAndSettle() (or at minimum await tester.pump()), the widget tree will not reflect the completed async operation before assertions run. While the current assertions check only widget presence and constructor properties (which pass immediately), this pattern is fragile—any future assertions depending on the loaded state or changes to MxcImage.build() that reference _imageData would fail without settling. Apply the same fix to line 423.
🤖 Prompt for AI Agents
In `@test/widget/message/twake_link_preview_item_test.dart` at line 347, The
cache-hit widget tests call await
tester.pumpWidget(wrapWithProviders(twakeLinkPreviewItem)) but do not wait for
the MxcImage post-frame async load to complete; update the tests that render
twakeLinkPreviewItem (and the second occurrence around the other test at the
later pumpWidget) to follow the pumpWidget call with await
tester.pumpAndSettle() (or at minimum await tester.pump()) so the
addPostFrameCallback/_loadImage() async work and subsequent setState() have
completed before assertions that may depend on _imageData or loaded state run.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/widgets/identity_provider_icon.dart`:
- Line 17: The code uses Uri.tryParse(identityProvider.icon ?? '') which returns
a non-null empty Uri for an empty string; change the logic so empty or
whitespace icon strings yield null instead of parsing: check
identityProvider.icon for null/empty (e.g., if
identityProvider.icon?.trim().isEmpty ?? true then set iconUri = null) otherwise
call Uri.tryParse(identityProvider.icon) and pass that result to MxcImage;
reference variables: identityProvider.icon, iconUri, and the MxcImage
constructor.
| }, | ||
| ); | ||
| }, | ||
| final iconUri = Uri.tryParse(identityProvider.icon ?? ''); |
There was a problem hiding this comment.
Uri.tryParse('') returns a non-null empty Uri, not null
Uri.tryParse wraps Uri.parse in a try-catch and only returns null on a FormatException. An empty string is a valid relative URI reference, so Uri.tryParse('') succeeds and returns Uri(). This means when identityProvider.icon is null, the ?? '' fallback causes iconUri to be Uri() (non-null, empty) rather than null. As a result, MxcImage receives a non-null but empty URI instead of null, which likely skips the placeholder path and may cause MxcImage to attempt loading a broken/empty URI.
🐛 Proposed fix
- final iconUri = Uri.tryParse(identityProvider.icon ?? '');
+ final iconUri = identityProvider.icon != null
+ ? Uri.tryParse(identityProvider.icon!)
+ : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/widgets/identity_provider_icon.dart` at line 17, The code uses
Uri.tryParse(identityProvider.icon ?? '') which returns a non-null empty Uri for
an empty string; change the logic so empty or whitespace icon strings yield null
instead of parsing: check identityProvider.icon for null/empty (e.g., if
identityProvider.icon?.trim().isEmpty ?? true then set iconUri = null) otherwise
call Uri.tryParse(identityProvider.icon) and pass that result to MxcImage;
reference variables: identityProvider.icon, iconUri, and the MxcImage
constructor.
chibenwa
left a comment
chibenwa
left a comment
There was a problem hiding this comment.
Simple question here: caching is needed because of mobile, right?
We agree that on web we could rely on default browser caching?
Also the 56+ 4000+ lines of code PR triggers me: are we trying to reinvent the wheel?
I was suggested to use cached_network_image what's wrong with it?
Not necessarily, and not explicitly mentioned either, so why not let the web benefit from it as well?
Do we have any specific documentation on this agreement?
Yes the PR is big. I can break it down to smaller, more manageable PRs.
No cache deduplication capability: If you send/load the same image 100 times, you'll have 100 same images in your cache. This also apply if e.g. |
Web native way would be to use |
Do we really need dedup? Are we having multiple cache key with the same content? Is it a big enough problem to disregard this cache alltogether?
That's cache - is it an issue? |
|
If you don't want all these, I will open another PR with much smaller changes using |
The question is not "if I want" The question is "What's better" - I cannot decide in the name of the team. I just wishes that the prblem is not overengineered, and that it uses idiomatic patterns. I can see numerous things here:
I just wishes that there is a design discussion on this, with documented alternatives. Also ideally ADRs should be in a separated PR so that they can be independently discussed. |
|
Hello,
I think this PR is a bit over-engineered for the performance problem |
3 participants
Ticket
Resolved
cache-web.mov
Screen.Recording.2026-02-13.at.2.01.11.PM.mov
Summary by CodeRabbit