Add opt-in video disk cache for ExoPlayer playback#6542
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
WalkthroughThis PR introduces an opt-in on-disk video cache for streamed media. It adds ChangesVideo Disk Cache Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt (1)
73-108: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid process-wide
clearAll()as the signal for deleting this directory.
VideoMediaCache.clearAll()returnstruefor any live cache in the JVM, even one owned by another test. That can skipfileManager.clearVideoCache(context)here and leavevideoCachebehind, making this assertion order-dependent. Make the delete decision based on whether this directory is actually owned, not whether some otherVideoMediaCacheexists.🤖 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/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt` around lines 73 - 108, The cache cleanup test is using the global VideoMediaCache clearAll() state to decide whether this specific directory should be deleted, which makes the assertion dependent on other tests. Update the cleanup logic around chatClient.clearCacheAndTemporaryFiles and fileManager.clearVideoCache(context) so it checks ownership of the current videoCache directory instead of any live cache instance, and ensure the test always verifies deletion of the directory created in this test.
🧹 Nitpick comments (2)
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoCacheDataSourceFactory.kt (1)
40-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider interface delegation for
DataSource.Factory.Static analysis flags this class for interface delegation via
by.cacheKeyForonly touchesdataSpec(no instance state), so it could be extracted to a top-level/private function, lettingdelegatebe delegated directly in the class header.♻️ Proposed refactor
+private fun cacheKeyFor(dataSpec: DataSpec): String = + dataSpec.key ?: dataSpec.uri.buildUpon().clearQuery().build().toString() + `@OptIn`(UnstableApi::class) internal class VideoCacheDataSourceFactory( videoCache: VideoMediaCache, upstreamFactory: DataSource.Factory, -) : DataSource.Factory { - - private val delegate: DataSource.Factory = CacheDataSource.Factory() - .setCache(videoCache.cache) - .setUpstreamDataSourceFactory(upstreamFactory) - .setCacheKeyFactory(::cacheKeyFor) - .setFlags(CacheDataSource.FLAG_IGNORE_CACHE_ON_ERROR) - - override fun createDataSource(): DataSource = delegate.createDataSource() - - /** - * Returns the cache key for [dataSpec]. Strips the URI's query so rotating signature or expiry - * parameters on the same path land on the same cache entry; a caller-supplied [DataSpec.key] is - * honoured when present. - */ - `@OptIn`(UnstableApi::class) - private fun cacheKeyFor(dataSpec: DataSpec): String = - dataSpec.key ?: dataSpec.uri.buildUpon().clearQuery().build().toString() -} +) : DataSource.Factory by CacheDataSource.Factory() + .setCache(videoCache.cache) + .setUpstreamDataSourceFactory(upstreamFactory) + .setCacheKeyFactory(::cacheKeyFor) + .setFlags(CacheDataSource.FLAG_IGNORE_CACHE_ON_ERROR)🤖 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/VideoCacheDataSourceFactory.kt` around lines 40 - 61, The VideoCacheDataSourceFactory class can be simplified by using interface delegation for DataSource.Factory instead of manually forwarding createDataSource through a delegate property. Move cacheKeyFor out of the class as a private top-level helper since it only depends on DataSpec, then delegate DataSource.Factory directly in the class declaration and keep the cache setup logic intact.Source: Linters/SAST tools
stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt (1)
56-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd video cache cleanup to
tearDown().Unlike stream/image/timestamped caches,
clearAllCacheintentionally never touches the video cache directory (perStreamFileManager.clearAllCache). This test creates a video cache dir but relies solely on the in-testclearCacheAndTemporaryFilescall to remove it;tearDown()provides no fallback if that assertion ever fails, so a video cache directory could leak into subsequent test runs.♻️ Proposed defensive cleanup
`@After` fun tearDown() { // Clean up all cache and external storage after each test streamFileManager.clearAllCache(context) streamFileManager.clearExternalStorage(context) + streamFileManager.clearVideoCache(context) }Also applies to: 73-77
🤖 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/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt` around lines 56 - 61, The tearDown cleanup in ChatClientCacheAndTemporaryFilesTest does not remove the video cache directory because StreamFileManager.clearAllCache intentionally skips it, so add explicit video cache cleanup alongside the existing clearAllCache and clearExternalStorage calls. Update the test’s tearDown() to invoke the appropriate StreamFileManager cleanup for the video cache path so it always gets removed even if the in-test clearCacheAndTemporaryFiles assertion fails. Apply the same defensive cleanup to the other referenced test block as well.
🤖 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 137-156: The VideoMediaCache.create(...) path currently constructs
SimpleCache directly, which can throw and abort
ChatClient.Builder.internalBuild() when the cache dir is locked or unusable.
Wrap the SimpleCache initialization in create(...) with failure handling, and on
error fall back to returning no video cache instead of propagating the
exception. Keep the guard localized to VideoMediaCache.create and preserve the
existing reuse behavior in the instances map when cache creation succeeds.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt`:
- Around line 1517-1532: The video cache cleanup in ChatClient should not decide
between clearing live caches and deleting the directory in two separate steps.
Update the cache removal flow around VideoMediaCache.clearAll() so the “clear
live caches or delete on disk” decision is handled atomically under the registry
lock, and remove the fallback delete from ChatClient’s cache-clearing path. Use
the existing VideoMediaCache and fileManager.clearVideoCache(context) symbols to
locate the affected logic and route the fallback through a single synchronized
API that prevents a newly created cache from being deleted mid-flight.
---
Outside diff comments:
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt`:
- Around line 73-108: The cache cleanup test is using the global VideoMediaCache
clearAll() state to decide whether this specific directory should be deleted,
which makes the assertion dependent on other tests. Update the cleanup logic
around chatClient.clearCacheAndTemporaryFiles and
fileManager.clearVideoCache(context) so it checks ownership of the current
videoCache directory instead of any live cache instance, and ensure the test
always verifies deletion of the directory created in this test.
---
Nitpick comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoCacheDataSourceFactory.kt`:
- Around line 40-61: The VideoCacheDataSourceFactory class can be simplified by
using interface delegation for DataSource.Factory instead of manually forwarding
createDataSource through a delegate property. Move cacheKeyFor out of the class
as a private top-level helper since it only depends on DataSpec, then delegate
DataSource.Factory directly in the class declaration and keep the cache setup
logic intact.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt`:
- Around line 56-61: The tearDown cleanup in
ChatClientCacheAndTemporaryFilesTest does not remove the video cache directory
because StreamFileManager.clearAllCache intentionally skips it, so add explicit
video cache cleanup alongside the existing clearAllCache and
clearExternalStorage calls. Update the test’s tearDown() to invoke the
appropriate StreamFileManager cleanup for the video cache path so it always gets
removed even if the in-test clearCacheAndTemporaryFiles assertion fails. Apply
the same defensive cleanup to the other referenced test block as well.
🪄 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: bbed4cb6-da81-49eb-a1d0-4ba2f86e3bd9
📒 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/api/ChatClientConfig.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/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPlayerState.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/attachments/preview/internal/PrepareIfNeededTest.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
| // Clear video cache: in-place via any live cache in the process (keeps the SimpleCache | ||
| // alive so playback continues to work), or by deleting the directory when no live cache | ||
| // owns it. The registry is process-wide, so this covers caches from a prior ChatClient | ||
| // build even if the current client was built without a cacheConfig. | ||
| val videoCacheResult = if (VideoMediaCache.clearAll()) { | ||
| Result.Success(Unit) | ||
| } else { | ||
| fileManager.clearVideoCache(context) | ||
| } | ||
| // Clear all cache directories | ||
| val cacheResult = fileManager.clearAllCache(context) | ||
| // Clear external (temporary) storage files - always run regardless of cache result | ||
| val externalStorageResult = fileManager.clearExternalStorage(context) | ||
| // Return the first failure if any, otherwise success | ||
| when { | ||
| videoCacheResult is Result.Failure -> videoCacheResult |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant symbols before reading slices
echo "== ChatClient outline =="
ast-grep outline stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt --view expanded | sed -n '1,220p'
echo
echo "== VideoMediaCache search =="
rg -n "class VideoMediaCache|fun clearAll|fun create|instances|clearVideoCache|clearAllCache|clearExternalStorage" stream-chat-android-client/src/main/java -S
echo
echo "== FileManager outline if present =="
fd -a "FileManager.kt" stream-chat-android-client/src/main/java || trueRepository: GetStream/stream-chat-android
Length of output: 31263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ChatClient outline =="
ast-grep outline stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt --view expanded | sed -n '1,220p'
echo
echo "== VideoMediaCache search =="
rg -n "class VideoMediaCache|fun clearAll|fun create|instances|clearVideoCache|clearAllCache|clearExternalStorage" stream-chat-android-client/src/main/java -S
echo
echo "== FileManager files =="
fd -a "FileManager.kt" stream-chat-android-client/src/main/java || trueRepository: GetStream/stream-chat-android
Length of output: 31250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant slices with line numbers once the file locations are known.
sed -n '1500,1545p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt
echo
echo "== VideoMediaCache.kt slices =="
sed -n '1,260p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/helpers/VideoMediaCache.ktRepository: GetStream/stream-chat-android
Length of output: 3026
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the exact files and relevant symbols.
git ls-files 'stream-chat-android-client/src/main/java/**' | rg 'ChatClient\.kt|VideoMediaCache\.kt|FileManager\.kt'
echo
for f in \
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt \
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/helpers/VideoMediaCache.kt \
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/helpers/FileManager.kt
do
if [ -f "$f" ]; then
echo "== $f =="
wc -l "$f"
fi
doneRepository: GetStream/stream-chat-android
Length of output: 783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the helper implementation and call sites.
rg -n "class VideoMediaCache|object VideoMediaCache|clearAll\(|create\(|deleteRecursively|clearVideoCache\(" stream-chat-android-client/src/main/java/io/getstream/chat/android/client -SRepository: GetStream/stream-chat-android
Length of output: 7459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant sections of the two helper files.
python3 - <<'PY'
from pathlib import Path
paths = [
Path("stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt"),
Path("stream-chat-android-client/src/main/java/io/getstream/chat/android/client/helpers/VideoMediaCache.kt"),
Path("stream-chat-android-client/src/main/java/io/getstream/chat/android/client/helpers/FileManager.kt"),
]
for p in paths:
if p.exists():
print(f"\n== {p} ==")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if ("clearCacheAndTemporaryFiles" in line or
"clearAll(" in line or
"clearVideoCache" in line or
"deleteRecursively" in line or
"class VideoMediaCache" in line or
"object VideoMediaCache" in line or
"fun create(" in line):
start = max(1, i - 20)
end = min(len(lines), i + 40)
for j in range(start, end + 1):
print(f"{j:5d}: {lines[j-1]}")
print()
PYRepository: GetStream/stream-chat-android
Length of output: 14827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt"
wc -l "$f"
echo
sed -n '1,240p' "$f"Repository: GetStream/stream-chat-android
Length of output: 7626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '360,390p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.ktRepository: GetStream/stream-chat-android
Length of output: 1522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.kt")
lines = p.read_text().splitlines()
for start in [360]:
end = min(len(lines), start+35)
for i in range(start, end+1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: GetStream/stream-chat-android
Length of output: 1954
Close the video-cache clear/delete race.
VideoMediaCache.clearAll() only tells you whether a live cache existed at that moment; if it returns false, the fallback fileManager.clearVideoCache(context) in ChatClient.kt:1521-1524 can run after another thread has created a new VideoMediaCache for the same directory. That deleteRecursively() path can corrupt the live SimpleCache index/lock. Move the fallback delete behind a single atomic VideoMediaCache API that holds the registry lock across the “clear live caches or delete on disk” decision.
🤖 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/ChatClient.kt`
around lines 1517 - 1532, The video cache cleanup in ChatClient should not
decide between clearing live caches and deleting the directory in two separate
steps. Update the cache removal flow around VideoMediaCache.clearAll() so the
“clear live caches or delete on disk” decision is handled atomically under the
registry lock, and remove the fallback delete from ChatClient’s cache-clearing
path. Use the existing VideoMediaCache and fileManager.clearVideoCache(context)
symbols to locate the affected logic and route the fallback through a single
synchronized API that prevents a newly created cache from being deleted
mid-flight.
There was a problem hiding this comment.
@VelikovPetar what's your take on this one? I think CodeRabbit is right that there is a real race here, though it needs an unusual setup: clearing the cache on one client while another client is being built in the same process. Low probability in practice.
The root of it is that clearAll() reports whether any cache is live anywhere in the process, and we use that yes/no to decide whether to delete this one directory. The signal and the target do not match, which is also why the cleanup test ends up coupled to global state.
A small fix would be to make the decision per directory under the registry lock: check whether a live cache owns getVideoCache(context), clear it in place if so, otherwise delete the directory while still holding the lock so a concurrent create() cannot register a new cache for it in between. That would also remove the test's dependence on global state.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,170p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.ktLength of output: 10865
@andremion Agreed — your fix targets the actual mismatch better than my original suggestion. The registry lock already exists inside VideoMediaCache.create/clear/release, so extending it to cover the "resolve directory → clear-in-place-or-delete" decision is a small, contained change and removes the dependency on global registry state.
Concretely, this could be a new internal API on VideoMediaCache.Companion, something like:
/**
* Clears the video cache for [cacheDir] atomically: if a live [VideoMediaCache] owns that
* directory, clears it in place (see [clear]); otherwise deletes the directory on disk via
* [onDeleteDir]. Holding the registry lock across both branches prevents a concurrent [create]
* from registering a new cache for [cacheDir] between the "no live cache" check and the delete.
*/
internal fun clearOrDelete(cacheDir: File, onDeleteDir: (File) -> Result<Unit>): Result<Unit> =
synchronized(instances) {
val key = cacheDir.absolutePath
instances[key]?.let {
it.clear()
Result.Success(Unit)
} ?: onDeleteDir(cacheDir)
}ChatClient.clearCacheAndTemporaryFiles would then call VideoMediaCache.clearOrDelete(fileManager.getVideoCache(context)) { fileManager.deleteVideoCacheDir(it) } (or similar), replacing the current clearAll() boolean + separate clearVideoCache(context) fallback. That also decouples the cleanup test from asserting on global registry state, as you noted.
I'll leave the implementation call to you and @VelikovPetar since it touches StreamFileManager's directory-delete helper too — happy to help wire it up if useful.
🧬 Code Graph Analysis Results
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt
Snippet 1 — clearCacheAndTemporaryFiles(context: Context)
Lines: ~340-400
/**
* Clears all cache and temporary files created by the Stream Chat SDK.
*
* This method removes:
* - All cached files from the default cache directory
* - All cached images from the image cache directory
* - All cached files during the upload/download process
* - All temporary files stored in external storage by the SDK (Photos and videos captured using the SDK)
*
* **Note**: This method does NOT clear database persistence. Use [clearPersistence] to clear
* database data, or call both methods if you need to clear all SDK data.
*
* **Note**: This method does NOT clear downloads made by the SDK to the file system. Those files are
* stored outside of the SDK's control and cannot be removed automatically.
*
* `@param` context The Android [Context] for accessing cache and external storage directories
* `@return` Executable async [Call] which performs the cleanup
*/
`@CheckResult`
public fun clearCacheAndTemporaryFiles(context: Context): Call<Unit> =
CoroutineCall(clientScope) {
logger.d { "[clearCacheAndTemporaryFiles] Clearing all cache and temporary files" }
// Clear video cache: in-place via any live cache in the process (keeps the SimpleCache
// alive so playback continues to work), or by deleting the directory when no live cache
// owns it. The registry is process-wide, so this covers caches from a prior ChatClient
// build even if the current client was built without a cacheConfig.
val videoCacheResult = if (VideoMediaCache.clearAll()) {
Result.Success(Unit)
} else {
fileManager.clearVideoCache(context)
}
// Clear all cache directories
val cacheResult = fileManager.clearAllCache(context)
// Clear external (temporary) storage files - always run regardless of cache result
val externalStorageResult = fileManager.clearExternalStorage(context)
// Return the first failure if any, otherwise success
when {
videoCacheResult is Result.Failure -> videoCacheResult
cacheResult is Result.Failure -> cacheResult
externalStorageResult is Result.Failure -> externalStorageResult
else -> Result.Success(Unit)
}
}Snippet 2 — Builder.internalBuild() creation of VideoMediaCache
Lines: ~760-830
val videoCache = chatClientConfig.cacheConfig.video?.let {
VideoMediaCache.create(appContext, StreamFileManager().getVideoCache(appContext), it)
}
val mediaDataSourceFactory = StreamMediaDataSource.factory(appContext, cdn, videoCache)There was a problem hiding this comment.
Hey @andremion I missed this, checking this first thing next week!
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!
andremion
left a comment
andremion
left a comment
There was a problem hiding this comment.
LGTM, design is solid. Two nits inline, both optional.
- VideoMediaCache.create() now returns VideoMediaCache? and wraps SimpleCache construction in a try-catch; a stale directory lock from a prior crash no longer aborts ChatClient initialisation — video caching is silently disabled instead. - VideoCacheConfig KDoc notes that cache entries are keyed by URL path only (query parameters are stripped). - LRU eviction test comment acknowledges the wall-clock dependency of the Thread.sleep spacing and what to do if the test becomes flaky. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
2 participants
Goal
Resolves: AND-1275
Media attachments currently re-stream from the CDN on every replay, seek, or scroll-back — wasting customer bandwidth and forcing users to wait through buffering on content they have already watched. This PR introduces an opt-in on-disk media cache so previously watched videos (and other ExoPlayer-driven media, including audio) replay from local bytes.
Port of v6 PR #6533 (squash commit
eadde4d) todevelop. This was a manual port, not a clean cherry-pick, because:ChatClient.kthas diverged significantly betweenv6anddevelop.develop(the v6 change toMediaGalleryPreviewScreen.ktmaps toMediaGalleryPlayerState.kthere).Intentional API difference from the v6 PR: the cache is configured via a new
ChatClientConfig.cacheConfigproperty (mirroring the existingmessageLimitConfigpattern) rather than aChatClient.Builder.cacheConfig(...)builder method. The v6 PR's incidentalMediaPreviewActivity.getIntent@JvmOverloadschurn was dropped as unrelated to caching.Implementation
Public API
StreamCacheConfig(video: VideoCacheConfig? = null)— aggregate config; future cache types 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 once exceeded. Rejects non-positive sizes at construction.ChatClientConfig.cacheConfig: StreamCacheConfig— single entry point, off by default (existing apps see no behavior change).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. The cache key strips URL query parameters, so rotating pre-signed signature/query values still resolve to the same cached entry.videoCache:StreamMediaPlayerContent(createPlayer)AttachmentMediaActivity,AttachmentGalleryVideoPageFragmentStreamAudioPlayerpipeline built inChatClient.Builder.build()clearCacheAndTemporaryFiles(context)clears every live cache in the process viaVideoMediaCache.clearAll()(which empties theSimpleCachewhile keeping it alive and its directory lock held), and only falls back to deleting the cache directory when no live cache owns it.Player.prepareIfNeeded(...)skips re-preparing an already-loaded URL, avoiding a second concurrent load that could leave a cached file's head uncached.UI Changes
No UI changes.
Testing
Automated — ran locally, all green:
StreamMediaDataSourceCacheIntegrationTest—{custom CDN, no CDN} × {hit, miss}matrix, rotated-query cache hit, LRU eviction, in-placeclear, andclearAll.VideoMediaCacheTest— directory creation,release()idempotency, per-directory instance registry.VideoCacheConfigTest— default/positive/zero/negativemaxSizeBytesvalidation.StreamFileManagerTest/ChatClientCacheAndTemporaryFilesTest— cache directory lookup and cleanup wiring.PrepareIfNeededTest(compose) — redundant-prepare skipping.Manual smoke
ChatClientConfig(cacheConfig = StreamCacheConfig(video = VideoCacheConfig())).<cacheDir>/stream_video_cache/contains files.clearCacheAndTemporaryFiles(context); cached bytes are removed and playback re-fetches, with the cache staying functional afterwards.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes