Add opt-in video disk cache for ExoPlayer playback#6533
Open
VelikovPetar wants to merge 6 commits into
Open
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
SDK Size Comparison 📏
|
Pre-signed URLs with rotating Expires/Signature query parameters would otherwise produce a fresh cache key on every open and defeat the cache. The CacheKeyFactory now strips the query, so the same path resolves to the same entry regardless of signature rotation. The full DataSpec still flows to the upstream on a miss, so a custom CDN sees the original URL with its query intact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.kt (1)
86-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider defaulting
useVideoCachetofalse.The default is
true, so any caller that omits the argument routes playback through the video cache — including non-video (e.g., audio) content, which contradicts the "video only" intent documented for this parameter. Current in-cohort callers pass it explicitly, so this is only a guard against future misuse; a conservativefalsedefault would make the opt-in semantics safer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.kt` at line 86, The default value for useVideoCache in StreamMediaPlayerContent is too permissive; update it to false so cache usage remains an explicit opt-in and doesn’t affect callers that omit the argument. Adjust the default on the StreamMediaPlayerContent parameter, keeping the existing explicit call sites unchanged, and ensure the parameter’s intent remains “video only” in its usage.stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.kt (1)
19-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd thread/state notes to the new public KDoc.
These are new public client APIs, but the KDoc still omits the thread-safety/state expectations required for this module's public surface. A short note that the configs are immutable value objects and that caching stays disabled unless
videois non-null would close that gap. As per coding guidelines "Document public APIs with KDoc, including thread expectations and state notes".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.kt` around lines 19 - 45, The new public KDoc on StreamCacheConfig and VideoCacheConfig needs thread/state expectations added. Update the docs for StreamCacheConfig and its video property to note that these are immutable value objects and that video caching remains disabled unless video is non-null. Also add a brief state note to VideoCacheConfig in the same style so the public API documentation matches the module guidelines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt`:
- Around line 69-80: The `VideoMediaCache.release()` method removes `dirPath`
from `instances` before teardown finishes, which can let `create()` reuse a slot
while `cache.release()` or `databaseProvider.close()` is still failing. Move the
`instances.remove(dirPath)` logic so it only happens after both teardown calls
succeed, and keep the registry entry intact if either close/release throws. Use
`release`, `create`, `instances`, `cache.release()`, and
`databaseProvider.close()` to locate the change.
- Around line 102-119: Normalize the cache directory before deriving the
singleton key in VideoMediaCache.create so alias paths for the same on-disk
directory map to one instance. Update the key generation around
cacheDir.absolutePath to use a canonical/resolved form of the directory, and
keep the existing synchronized(instances) reuse check and SimpleCache
construction behavior unchanged so Media3 still gets only one cache per actual
directory.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.kt`:
- Around line 43-49: Reject invalid cache sizes at the API boundary by
validating VideoCacheConfig.maxSizeBytes before it reaches VideoMediaCache and
LeastRecentlyUsedCacheEvictor. Add a fail-fast check in VideoCacheConfig (or its
constructor/init path) so 0 and negative values are rejected with a clear error,
while keeping the existing default constant unchanged. Use the VideoCacheConfig
and VideoMediaCache symbols to locate the validation point and ensure all
callers get the same safeguard.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cdn/internal/StreamMediaDataSource.kt`:
- Around line 46-48: Update the KDoc on StreamMediaDataSource to match the
actual cache-key behavior in VideoCacheDataSourceFactory: the cache is not keyed
by the raw dataSpec.uri, but by a query-stripped URI so differently signed or
rotating URLs still resolve to the same cached entry. Keep the description
aligned with the wrapping behavior in StreamMediaDataSource and any helper that
normalizes the cache key, so readers are not misled about distinct cache entries
for query changes.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.kt`:
- Around line 441-455: The clearVideoCache() helper deletes the video cache
directory while VideoMediaCache may still hold a process-wide SimpleCache on
that path. Update clearAllCache() to tear down/release the shared video cache
first, then call clearVideoCache(context) so the cache lock and index are closed
before deleteRecursively() runs. Use the existing VideoMediaCache teardown path
and keep the clearVideoCache() deletion logic unchanged after the cache is
released.
---
Nitpick comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.kt`:
- Around line 19-45: The new public KDoc on StreamCacheConfig and
VideoCacheConfig needs thread/state expectations added. Update the docs for
StreamCacheConfig and its video property to note that these are immutable value
objects and that video caching remains disabled unless video is non-null. Also
add a brief state note to VideoCacheConfig in the same style so the public API
documentation matches the module guidelines.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.kt`:
- Line 86: The default value for useVideoCache in StreamMediaPlayerContent is
too permissive; update it to false so cache usage remains an explicit opt-in and
doesn’t affect callers that omit the argument. Adjust the default on the
StreamMediaPlayerContent parameter, keeping the existing explicit call sites
unchanged, and ensure the parameter’s intent remains “video only” in its usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0dd8e7c8-1426-48be-a800-3705667274a5
📒 Files selected for processing (21)
stream-chat-android-client-test/src/main/java/io/getstream/chat/android/client/test/MockedChatClientTest.ktstream-chat-android-client/api/stream-chat-android-client.apistream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoCacheDataSourceFactory.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cdn/internal/CDNDataSourceFactory.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cdn/internal/StreamMediaDataSource.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/StreamMediaDataSourceCacheIntegrationTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/VideoMediaCacheTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/cdn/internal/CDNDataSourceTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/file/StreamFileManagerTest.ktstream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaPreviewActivity.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/handler/MediaAttachmentPreviewHandler.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.ktstream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/feature/gallery/AttachmentMediaActivity.ktstream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/feature/gallery/internal/AttachmentGalleryVideoPageFragment.kt
Comment on lines
+441
to
+455
| @Suppress("TooGenericExceptionCaught") | ||
| private fun clearVideoCache(context: Context): Result<Unit> { | ||
| return try { | ||
| val directory = getVideoCache(context) | ||
| if (!directory.exists()) { | ||
| Result.Success(Unit) | ||
| } else if (directory.deleteRecursively()) { | ||
| Result.Success(Unit) | ||
| } else { | ||
| Result.Failure(Error.GenericError("Could not clear video cache directory.")) | ||
| } | ||
| } catch (e: Exception) { | ||
| Result.Failure(Error.ThrowableError("Could not clear video cache directory.", e)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect VideoMediaCache for release/clear handling and any teardown hooked into cache clearing.
fd -t f 'VideoMediaCache.kt' --exec cat -n {}
rg -nP '\b(release|delete|clearAllCache|clearVideoCache)\s*\(' stream-chat-android-client/src/main/java/io/getstream/chat/android/clientRepository: GetStream/stream-chat-android
Length of output: 9295
🏁 Script executed:
#!/bin/bash
sed -n '230,470p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.kt
printf '\n---\n'
sed -n '1,140p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt
printf '\n---\n'
sed -n '1510,1545p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.ktRepository: GetStream/stream-chat-android
Length of output: 17925
Release the shared video cache before deleting its directory. clearAllCache deletes the video cache folder while VideoMediaCache keeps a process-wide SimpleCache open on that same path. Call the cache teardown path first so the lock/index are released before deleteRecursively(), or later video cache writes/playback can fail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.kt`
around lines 441 - 455, The clearVideoCache() helper deletes the video cache
directory while VideoMediaCache may still hold a process-wide SimpleCache on
that path. Update clearAllCache() to tear down/release the shared video cache
first, then call clearVideoCache(context) so the cache lock and index are closed
before deleteRecursively() runs. Use the existing VideoMediaCache teardown path
and keep the clearVideoCache() deletion logic unchanged after the cache is
released.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Outline: StreamFileManager =="
ast-grep outline stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.kt --view expanded || true
echo
echo "== Outline: VideoMediaCache =="
ast-grep outline stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt --view expanded || true
echo
echo "== StreamFileManager relevant slice =="
sed -n '230,280p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.kt
echo
sed -n '430,500p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.kt
echo
echo "== VideoMediaCache file =="
cat -n stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt
echo
echo "== Search for release/clear usage =="
rg -n -C3 --type=kt '\b(VideoMediaCache|release|clearVideoCache|clearAllCache)\b' \
stream-chat-android-client/src/main/java/io/getstream/chat/android/clientLength of output: 14414
@gpunto Yes — I think the concern is valid.
What I verified:
- In
StreamFileManager.kt,clearAllCache()callsclearVideoCache(context)directly, andclearVideoCache()just doesdeleteRecursively()on the video cache directory. - In
VideoMediaCache.kt, the cache is process-wide and backed by aSimpleCache, with instances kept in a singleton map by directory path. - That file also has a
release()method that explicitly releasesSimpleCache, closes the database provider, and removes the instance from the registry.
So deleting the directory without releasing that shared cache first is risky: the in-memory singleton can still believe it owns that path, and Media3’s per-directory lock/index lifecycle is exactly the thing release() is meant to tear down.
The only nuance is that the current teardown method is marked @VisibleForTesting and its KDoc says production code “does not need to call this”, so the issue is real, but the final fix may be better as a small production-safe reset API rather than wiring the current test-oriented method directly from StreamFileManager.
🧠 Learnings used
Learnt from: VelikovPetar
Repo: GetStream/stream-chat-android PR: 6152
File: stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelEventHandlerImpl.kt:194-212
Timestamp: 2026-02-17T13:56:11.136Z
Learning: In Kotlin code handling channel events, specifically MessageUpdatedEvent, prefer using server-provided data for replyTo rather than enriching it from local state as done in ChannelEventHandlerImpl. This should be treated as the source of truth. Ensure code comments document this design decision and that tests validate server data is used for replyTo. If there are any places that historically enriched replyTo from local state (e.g., ChannelEventHandlerLegacyImpl), consider aligning those implementations or adding a clear rationale in design docs to avoid future regressions.
Learnt from: VelikovPetar
Repo: GetStream/stream-chat-android PR: 6295
File: stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cdn/internal/StreamMediaDataSource.kt:27-49
Timestamp: 2026-03-27T08:18:10.611Z
Learning: In GetStream/stream-chat-android (stream-chat-android-client), the usual expectation to document thread-safety and state/lifecycle for public APIs does not apply to public objects/factory methods annotated with `InternalStreamChatApi` (e.g., `StreamMediaDataSource.factory` in `.../cdn/internal/StreamMediaDataSource.kt`). For these `InternalStreamChatApi`-annotated utilities, maintainers consider that level of thread/lifecycle KDoc detail unnecessary; review/flag KDoc thread/state expectations primarily for non-`InternalStreamChatApi` public APIs.
Learnt from: andremion
Repo: GetStream/stream-chat-android PR: 6301
File: stream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/feature/channel/info/AddMembersViewAction.kt:19-25
Timestamp: 2026-03-30T09:19:45.796Z
Learning: In GetStream/stream-chat-android SDK modules that set a module-level Kotlin compiler argument (via build.gradle.kts) like `-opt-in=io.getstream.chat.android.core.internal.InternalStreamChatApi`, do not require per-file opt-in annotations. Specifically, don’t flag missing `file:OptIn(InternalStreamChatApi::class)` in `.kt` files within those modules when using `InternalStreamChatApi` types, since the opt-in is already applied at the module build level. Only flag missing per-file opt-in in modules that do NOT configure this compiler arg.
Learnt from: gpunto
Repo: GetStream/stream-chat-android PR: 6328
File: stream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/feature/messages/list/MessagePositionHandler.kt:19-28
Timestamp: 2026-04-02T14:29:26.633Z
Learning: In Kotlin, if a `private const val` is declared inside a `companion object`, it is directly accessible within the same `companion object` body (where it’s declared) without any import. During review, don’t claim an import is required or that an import would cause a compile error for usage inside that same companion object in the same file; only flag missing/incorrect imports when code is referencing the constant from outside the companion object.
Learnt from: gpunto
Repo: GetStream/stream-chat-android PR: 6468
File: stream-chat-android-ui-components/api/stream-chat-android-ui-components.api:3064-3069
Timestamp: 2026-05-27T15:59:48.528Z
Learning: In this repository, do not flag public Kotlin `sealed` interfaces (e.g., `public sealed interface ...`) for source-compatibility concerns specifically about adding new abstract members. Because Kotlin `sealed` types cannot be implemented outside the permitted scope (so external consumers cannot add new implementors), adding abstract members should not break external compilation on the basis of missing implementations. Note: the project does not generally guarantee binary compatibility, so this guidance is only for the source-compatibility check.
Learnt from: VelikovPetar
Repo: GetStream/stream-chat-android PR: 6472
File: stream-chat-android-client/src/main/java/io/getstream/chat/android/client/api/ChatClientConfig.kt:133-136
Timestamp: 2026-06-02T08:42:28.269Z
Learning: For this GetStream/stream-chat-android repository, do not treat Kotlin JVM binary compatibility of SDK public API data classes as a long-term compatibility contract across releases. In code reviews, it is acceptable to add new constructor parameters (e.g., `val`/`var` fields) with default values to existing public `data class` types without adding additive/secondary-constructor workarounds. Therefore, reviewers should not raise binary-compatibility breakage concerns for this repository when such changes are made.
Contributor
Author
There was a problem hiding this comment.
I believe that we shouldn't fully release the VideoCache - if we release it, it means that our VideoMediaCache instance will reference an outdated SimpleCache. I think we should instead just clear the cache dir via the cache.removeResource(key) method, and not directly via deleteRecursively. Currently running some tests related to this.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Contributor
Author
There was a problem hiding this comment.
Yes, valid concern — fixed in 372ab0f. Added VideoMediaCache.clear() which iterates cache.keys and calls SimpleCache.removeResource(key) for each entry; this empties the cache in place without touching the directory Media3's live SimpleCache still owns (deleting the directory out from under it would corrupt the on-disk index and lock).
Wiring:
StreamFileManager.clearAllCacheno longer touches the video cache directory (the invariant is guarded by a new test).ChatClient.clearCacheAndTemporaryFilesroutes video-cache clearing throughvideoCache?.clear()when a live cache is present, falling back tofileManager.clearVideoCache(context)when no cache is configured (so leftover files from a prior opt-in still get cleaned).
This keeps subsequent playback working immediately after the clear (verified end-to-end via the new clear removes cached bytes from disk and keeps the cache functional test).
gpunto
reviewed
Jun 30, 2026
gpunto
left a comment
gpunto
left a comment
Contributor
There was a problem hiding this comment.
Looks good, just a couple of doubts and a nitpick
Comment on lines
+441
to
+455
| @Suppress("TooGenericExceptionCaught") | ||
| private fun clearVideoCache(context: Context): Result<Unit> { | ||
| return try { | ||
| val directory = getVideoCache(context) | ||
| if (!directory.exists()) { | ||
| Result.Success(Unit) | ||
| } else if (directory.deleteRecursively()) { | ||
| Result.Success(Unit) | ||
| } else { | ||
| Result.Failure(Error.GenericError("Could not clear video cache directory.")) | ||
| } | ||
| } catch (e: Exception) { | ||
| Result.Failure(Error.ThrowableError("Could not clear video cache directory.", e)) | ||
| } | ||
| } |
- Cache all ExoPlayer-driven playback (video, audio, voice recordings) through the same VideoCacheConfig; remove the now-dead isVideo gating and mimeType/type intent extras on MediaPreviewActivity. - Clear the video cache in place via VideoMediaCache.clear() so the live SimpleCache stays alive after ChatClient.clearCacheAndTemporaryFiles; StreamFileManager.clearAllCache no longer deletes the video directory out from under the live cache. - Reject non-positive VideoCacheConfig.maxSizeBytes at the API boundary. - Fix stale KDoc on StreamMediaDataSource to reflect the query-stripped cache key. - Stabilise the flaky LRU eviction test by spacing cache writes across distinct millisecond timestamps.
|
WalkthroughThis PR adds a configurable on-disk video cache to the Stream Chat Android SDK. New ChangesOn-disk Video Cache Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Builder as ChatClient.Builder
participant VMC as VideoMediaCache.create()
participant SC as SimpleCache (LRU)
participant VCDSF as VideoCacheDataSourceFactory
participant CDN as CDNDataSourceFactory
participant Upstream as HttpDataSource
Builder->>VMC: create(appContext, cacheDir, VideoCacheConfig)
VMC-->>Builder: VideoMediaCache (singleton per dir)
Builder->>VCDSF: wrap(videoCache, upstreamFactory)
VCDSF->>CDN: createDataSource() on miss
CDN->>Upstream: open(signed URL)
Upstream-->>SC: write bytes
SC-->>VCDSF: serve bytes on hit
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt (2)
90-105: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRegistry entry removed before teardown succeeds (still unresolved from prior review).
instances.remove(dirPath)at line 93 runs beforecache.release()/databaseProvider.close(). If either throws (caught and only logged), a subsequentcreate()for the same directory will construct a newSimpleCachewhile the old one may still hold Media3's directory lock, causing the new construction to fail.🔒 Proposed fix
internal fun release() { synchronized(instances) { - instances.remove(dirPath) + var released = true try { cache.release() } catch (`@Suppress`("TooGenericExceptionCaught") e: Exception) { logger.e(e) { "[release] failed to release SimpleCache" } + released = false } try { databaseProvider.close() } catch (`@Suppress`("TooGenericExceptionCaught") e: Exception) { logger.e(e) { "[release] failed to close StandaloneDatabaseProvider" } + released = false } + if (released) { + instances.remove(dirPath) + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt` around lines 90 - 105, The release flow in VideoMediaCache.release removes the registry entry before teardown finishes, so a failed cache.release() or databaseProvider.close() can leave the old Media3 lock active while create() builds a new instance. Move instances.remove(dirPath) to after both teardown calls succeed, or otherwise only unregister once release and close have completed without error, using VideoMediaCache.release, create(), and the instances map as the key symbols to update.
123-142: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSingleton key still uses raw
absolutePath(still unresolved from prior review).
cacheDir.absolutePathdoesn't normalize aliases like./video-cachevsvideo-cacheor symlinked equivalents, so the "oneSimpleCacheper directory" guard can still be bypassed for the same physical directory.Proposed fix
public fun create(appContext: Context, cacheDir: File, config: VideoCacheConfig): VideoMediaCache = synchronized(instances) { cacheDir.mkdirs() - val key = cacheDir.absolutePath + val normalizedDir = cacheDir.canonicalFile + val key = normalizedDir.path instances[key]?.let { existing -> ... } val dbProvider = StandaloneDatabaseProvider(appContext) val simpleCache = SimpleCache( - cacheDir, + normalizedDir, LeastRecentlyUsedCacheEvictor(config.maxSizeBytes), dbProvider, ) VideoMediaCache(simpleCache, dbProvider, key).also { instances[key] = it } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt` around lines 123 - 142, The singleton lookup in VideoMediaCache.create still uses cacheDir.absolutePath, so equivalent directories can bypass the reuse guard. Update the key generation to use a normalized canonical/real path for the cache directory before checking instances and storing the cache, and keep the reuse logic in create synchronized on that normalized key.
🧹 Nitpick comments (2)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaPreviewActivity.kt (1)
187-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReconsider adding
@JvmOverloadshere.Based on learnings, this module intentionally avoids
@JvmOverloadsfor public API compatibility and relies on Kotlin default arguments instead. Please confirm there's a specific Java-interop need driving this addition; otherwise consider dropping@JvmOverloadsto stay consistent with the established convention.♻️ Suggested diff
- `@JvmOverloads` public fun getIntent( context: Context, url: String, title: String? = null, ): Intent {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaPreviewActivity.kt` around lines 187 - 192, The public MediaPreviewActivity.getIntent API should not introduce `@JvmOverloads` unless there is a concrete Java-interop requirement; this module prefers Kotlin default arguments for public compatibility. Remove `@JvmOverloads` from getIntent unless you can justify the need, and keep the signature aligned with the established convention used in this compose module.Source: Learnings
stream-chat-android-compose/api/stream-chat-android-compose.api (1)
979-981: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew
@JvmOverloads-generated overload conflicts with an established module convention.This entry is generated correctly, but it's a direct byproduct of adding
@JvmOverloadstoMediaPreviewActivity.Companion.getIntent(see that file). Based on learnings, in this module Kotlin default arguments should be relied on instead of introducing@JvmOverloads-generated overloads for public API compatibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-compose/api/stream-chat-android-compose.api` around lines 979 - 981, Remove the public overloads introduced by `@JvmOverloads` on MediaPreviewActivity.Companion.getIntent and rely on Kotlin default arguments instead. Update the Companion.getIntent API so it exposes only the intended single Kotlin entry point with defaulted parameters, and regenerate the .api metadata to drop the extra getIntent overload and getIntent$default symbol.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt`:
- Around line 90-105: The release flow in VideoMediaCache.release removes the
registry entry before teardown finishes, so a failed cache.release() or
databaseProvider.close() can leave the old Media3 lock active while create()
builds a new instance. Move instances.remove(dirPath) to after both teardown
calls succeed, or otherwise only unregister once release and close have
completed without error, using VideoMediaCache.release, create(), and the
instances map as the key symbols to update.
- Around line 123-142: The singleton lookup in VideoMediaCache.create still uses
cacheDir.absolutePath, so equivalent directories can bypass the reuse guard.
Update the key generation to use a normalized canonical/real path for the cache
directory before checking instances and storing the cache, and keep the reuse
logic in create synchronized on that normalized key.
---
Nitpick comments:
In `@stream-chat-android-compose/api/stream-chat-android-compose.api`:
- Around line 979-981: Remove the public overloads introduced by `@JvmOverloads`
on MediaPreviewActivity.Companion.getIntent and rely on Kotlin default arguments
instead. Update the Companion.getIntent API so it exposes only the intended
single Kotlin entry point with defaulted parameters, and regenerate the .api
metadata to drop the extra getIntent overload and getIntent$default symbol.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaPreviewActivity.kt`:
- Around line 187-192: The public MediaPreviewActivity.getIntent API should not
introduce `@JvmOverloads` unless there is a concrete Java-interop requirement;
this module prefers Kotlin default arguments for public compatibility. Remove
`@JvmOverloads` from getIntent unless you can justify the need, and keep the
signature aligned with the established convention used in this compose module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 770c5842-5cdd-4646-91a5-13354388d42a
📒 Files selected for processing (20)
stream-chat-android-client-test/src/main/java/io/getstream/chat/android/client/test/MockedChatClientTest.ktstream-chat-android-client/api/stream-chat-android-client.apistream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoCacheDataSourceFactory.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cdn/internal/CDNDataSourceFactory.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cdn/internal/StreamMediaDataSource.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/VideoCacheConfigTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/StreamMediaDataSourceCacheIntegrationTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/VideoMediaCacheTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/cdn/internal/CDNDataSourceTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/file/StreamFileManagerTest.ktstream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaPreviewActivity.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.ktstream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/feature/gallery/AttachmentMediaActivity.ktstream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/feature/gallery/internal/AttachmentGalleryVideoPageFragment.kt
gpunto
approved these changes
Jul 1, 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
2 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.
Goal
Video attachments currently re-stream from the CDN on every replay, seek, or scroll-back. This wastes customer bandwidth and forces users to wait through buffering on content they have already watched. This PR introduces an opt-in on-disk video cache so previously watched videos replay from local bytes.
Implementation
Public API
A new
io.getstream.chat.android.client.cachepackage exposes the opt-in surface:StreamCacheConfig(video: VideoCacheConfig? = null)— aggregate config; future cache types (e.g. images) can be added as additional nullable properties without breaking the call site.VideoCacheConfig(maxSizeBytes: Long)— soft cap on cache size; defaults to 150 MB. LRU eviction kicks in once exceeded.ChatClient.Builder.cacheConfig(StreamCacheConfig)— single entry point. Default is off; existing apps see no behavior change.Usage:
Internals
VideoMediaCacheowns a Media3SimpleCache+StandaloneDatabaseProvider. A process-wide registry keyed by the cache directory's absolute path prevents the "twoSimpleCacheinstances against the same directory" crash if a customer rebuildsChatClientin the same process.VideoCacheDataSourceFactorywraps Media3'sCacheDataSource.Factoryand composes outside any custom CDN — on a hit the bytes are served from disk and the customer'sCDN.fileRequestis not consulted; on a miss the CDN signs the URL just-in-time.MediaPreviewActivity→StreamMediaPlayerContent,MediaGalleryPagerAttachmentMediaActivity,AttachmentGalleryVideoPageFragmenttype == AttachmentType.VIDEOor mime starts withvideo/) is routed through the cache. Audio attachments and other media bypass it entirely so audio bytes don't occupy disk space reserved for video playback.StreamFileManager.getVideoCache(context)and is included inclearCacheAndTemporaryFiles(context).UI Changes
No UI changes.
Testing
Automated
StreamMediaDataSourceCacheIntegrationTest— four-cell matrix{custom CDN, no CDN} × {hit, miss}plus an LRU eviction case that writes three videos into a 2-video-capacity cache and verifies the least-recently-used entry is the one dropped.VideoMediaCacheTest— directory creation,release()idempotency, per-directory instance registry.StreamFileManagerTest/ChatClientCacheAndTemporaryFilesTest— cache directory lookup and cleanup wiring.Manual smoke
ChatClient.Builder.cacheConfig(StreamCacheConfig(video = VideoCacheConfig()))in the sample app.<cacheDir>/stream_video_cache/contains files.Summary by CodeRabbit
New Features
Bug Fixes