Resolves offline sound playback by caching assets locally#50130
Resolves offline sound playback by caching assets locally#50130yuwenmemon merged 14 commits intoExpensify:mainfrom
Conversation
|
@rayane-djouah Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
wildan-m
changed the title
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid_native.mp4Android: mWeb Chromeandroid_mweb_chrome.mp4iOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-18.at.22.59.17.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-18.at.22.47.53.mp4MacOS: Chrome / SafariScreen.Recording.2024-10-18.at.9.03.44.PM.movMacOS: DesktopScreen.Recording.2024-10-18.at.9.07.20.PM.mov |
rayane-d
left a comment
rayane-d
left a comment
There was a problem hiding this comment.
Looks good! left some comments
| // Cache each sound file if it's not already cached. | ||
| const cachePromises = soundFiles.map((soundFile) => { | ||
| return cache.match(soundFile).then((response) => { | ||
| if (response) { | ||
| return; | ||
| } | ||
| return cache.add(soundFile); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Potential Issue: Stale Cache Entries
Clients may continue using outdated cached sound files if the server updates them. To ensure users always have the latest sounds, we should refresh the cache on every app load.
There was a problem hiding this comment.
@rayane-djouah Not sure about this suggestion, but is it better to use in-memory cache if we want to update the sound assets on each page load?
Replacing the cache during the page load would result in fetching assets from the remote URL every time we load.
I think server will rarely update sound assets. To optimize, how about clearing sound assets during logout and resetting the app on the troubleshooting page?
src/libs/Sound/index.ts
function clearSoundAssetsCache() {
// Exit early if the Cache API is not available in the current browser.
if (!('caches' in window)) {
return;
}
caches.delete('sound-assets').then((success) => {
if (success) {
console.log('Sound assets cache cleared.');
} else {
console.log('Failed to clear sound assets cache.');
}
}).catch((error) => {
console.error('Error clearing sound assets cache:', error);
});
}And execute that function in:
src/libs/actions/App.ts
function clearOnyxAndResetApp(shouldNavigateToHomepage?: boolean) {
......
clearSoundAssetsCache();
.....And for clear action in Troubleshoot page:
src/libs/actions/App.ts
function clearOnyxAndResetApp(shouldNavigateToHomepage?: boolean) {
.........
clearSoundAssetsCache();
}Kapture.2024-10-04.at.08.01.29.mp4
There was a problem hiding this comment.
@wildan-m, I agree that clearing the cache upon logout and during troubleshooting is a smart approach to handle updates to sound assets.
To complement this, we could consider a versioned cache strategy. This involves appending a version number to the cache name. We can update the cache version whenever we change or add a sound.
// Update this version string when new sound files are added or existing ones are changed to invalidate old caches and prompt caching of the new assets.
const CURRENT_CACHE_VERSION = 'sound-assets-v1';
caches.open(CURRENT_CACHE_VERSION).then(...)There was a problem hiding this comment.
@rayane-djouah how would we determine version? usually server will send the version to invalidate the cache. Is there any API for that purpose?
There was a problem hiding this comment.
Another common approach is version the file name sound-v1.mp3, since the name already stored as key to put the cache I think we don't need any other change to version the cache in FE
There was a problem hiding this comment.
The updated main has been merged and clearSoundAssetsCache has been implemented
…x/47148-fix-sound-offline
Co-authored-by: rayane-djouah <77965000+rayane-djouah@users.noreply.github.com>
Co-authored-by: rayane-djouah <77965000+rayane-djouah@users.noreply.github.com>
…x/47148-fix-sound-offline
| }); | ||
| }); | ||
| }); | ||
| clearSoundAssetsCache(); |
There was a problem hiding this comment.
Will not it crash mobile app? It looks like clearSoundAssetsCache is implemented only on web and such function doesn't exist on mobile? Correct me if I'm wrong 🙏
Also, why do we need to clear a cache on logout? In case if we want to replace sound we can just change name and we'll simply download new version of the file? Don't you think that clearSoundAssetsCache is kind of over engineering?
There was a problem hiding this comment.
Will not it crash mobile app?
I think it won't crash, there is check if (!('caches' in window)) { and also try catch, but let me know if we need to extract the function and create platform specific code
function clearSoundAssetsCache() {
// Exit early if the Cache API is not available in the current browser.
if (!('caches' in window)) {
return;
}
caches
.delete('sound-assets')
.then((success) => {
if (success) {
return;
}
Log.alert('[sound] Failed to clear sound assets cache.');
})
.catch((error) => {
Log.alert('[sound] Error clearing sound assets cache:', {message: (error as Error).message});
});Also, why do we need to clear a cache on logout?
The assets might be replaced without changing the name.
we can just change name and we'll simply download new version of the file?
replacing the name might need code change to the assets references and we might need to rebuild the app each time we change the name
Don't you think that clearSoundAssetsCache is kind of over engineering?
I don't think so, the code is quiet simple and it's also make sense to make it as part of app reset
There was a problem hiding this comment.
I think it won't crash, there is check if (!('caches' in window)) {
I think clearSoundAssetsCache will be undefined and you will receive undefined is not a function. Did you test native code in this PR?
The assets might be replaced without changing the name.
At the moment only developer can update asset. So if they update it without changing a name it'll be kind of developer fault...
replacing the name might need code change to the assets references and we might need to rebuild the app each time we change the name
We can replace only name of SOUNDS, i. e.:
const SOUNDS = {
DONE: 'done-v1',
SUCCESS: 'success',
ATTENTION: 'attention',
RECEIVE: 'receive',
} as const;And other code will be untouched 🤷♂️
I don't think so, the code is quiet simple and it's also make sense to make it as part of app reset
Oki doki, just wanted to be sure, that alternative solution also has been considered 👍
There was a problem hiding this comment.
I think clearSoundAssetsCache will be undefined and you will receive undefined is not a function.
@kirillzyusko you are right, I've updated the native implementation to empty function. thank you!
|
Testing the PR today |
|
@wildan-m |
|
@rayane-djouah I think that's expected, it should be reloaded whole online for at least once to fill up the cache again. Or should we directly call |
|
@wildan-m I think it's fine to keep the current behaviour 👍 |
|
@yuwenmemon all yours! |
|
@yuwenmemon looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
False positive, check the latest build -tests were passing. |
|
🚀 Deployed to staging by https://github.com/yuwenmemon in version: 9.0.52-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.52-5 🚀
|
5 participants
Details
This PR resolves offline sound playback by caching assets locally for web and desktop environments.
Fixed Issues
$ #47148
PROPOSAL: #47148 (comment) (Alternative 4)
Tests
Offline tests
Same as test
QA Steps
Same as test
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Kapture.2024-10-03.at.13.22.01.mp4
Android: mWeb Chrome
Kapture.2024-10-03.at.13.29.35.mp4
iOS: Native
Kapture.2024-10-03.at.11.30.24.mp4
iOS: mWeb Safari
Kapture.2024-10-03.at.11.37.01.mp4
MacOS: Chrome / Safari
Kapture.2024-10-03.at.11.40.45.mp4
MacOS: Desktop
Kapture.2024-10-03.at.13.25.40.mp4