Manage video reducer from composer reducer #5573
Merged
Conversation
This file contains 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
gaearon
commented
Oct 2, 2024
| if (prevMedia?.type === 'video') { | ||
| nextMedia = { | ||
| ...prevMedia, | ||
| video: videoReducer(prevMedia.video, videoAction), |
|
This is slightly broken. In particular, we can't remove video yet because there's no action that results in video embed being removed.
This aligns the video state lifetime with the embed lifetime. Video can now be properly added and removed.
arcalinea
temporarily deployed
to
embed-4 - social-app PR #5573
Render
Destroyed
haileyok
approved these changes
Oct 3, 2024
estrattonbailey
added a commit
that referenced
this pull request
Oct 3, 2024
estrattonbailey
added a commit
that referenced
this pull request
Oct 3, 2024
* origin/main: 🪵📌 (#5594) Rename some files and variables (#5587) Manage video reducer from composer reducer (#5573) Refactor video uploads (#5570) Delete messages NUX (#5574) move files around (#5576) Make reply prompt more subtle on desktop (#5569) Ignore built files (#5572) import order (#5571) Add suspense boundary in onboarding (#5556) Tweak verify email dialog (#5555) Add intent/verify-email to go server (#5554)
tkusano
pushed a commit
to tkusano/social-app
that referenced
this pull request
Oct 4, 2024
* Move video state into composer state * Represent video as embed This is slightly broken. In particular, we can't remove video yet because there's no action that results in video embed being removed. * Properly represent video as embed This aligns the video state lifetime with the embed lifetime. Video can now be properly added and removed. * Disable Add Video when we have images * Ignore empty image pick
tkusano
added a commit
to tkusano/social-app
that referenced
this pull request
Oct 4, 2024
…)" This reverts commit 59589e3.
estrattonbailey
added a commit
that referenced
this pull request
Oct 7, 2024
* Update translation * Fixed * Update translation * Update translation * Update translation * Update translation * Suggested fix by Hima-Zinn * Update translation * Update translation * Update translation * Translate "pinned" * Update translation * Refactor video uploads (#5570) * Remove unused video field * Stop exposing video dispatch * Move cancellation out of the reducer * Make useUploadStatusQuery controlled by jobId * Rename SetStatus to SetProcessing This action only has one callsite and it's always passing "processing". * Move jobId into video reducer state * Make cancellation scoped * Inline useCompressVideoMutation * Move processVideo down the file * Extract getErrorMessage * useServiceAuthToken -> getServiceAuthToken * useVideoAgent -> createVideoAgent * useVideoUploadLimits -> getVideoUploadLimits * useUploadVideoMutation -> uploadVideo * Use async/await in processVideo * Inline onVideoCompressed into processVideo * Use async/await for uploadVideo * Factor out error messages * Guard dispatch with signal This lets us remove the scattered signal checks around dispatch. * Move job polling out of RQ * Handle poll failures * Remove unnecessary guards * Slightly more accurate condition * Move initVideoUri handling out of the hook * Remove dead argument It wasn't being used before either. * Remove unused detailed status This isn't being used because we're only respecting that state variable when isProcessing=true, but isProcessing is always false during video upload. If we want to re-add this later, it should really just be derived from the reducer state. * Harden the video reducer * Tie all spawned work to a signal * Preserve asset/media for nicer error state * Rename actions to match states * Inline useUploadVideo This abstraction is getting in the way of some future work. * Move MIME check to the only place that handles it * Manage video reducer from composer reducer (#5573) * Move video state into composer state * Represent video as embed This is slightly broken. In particular, we can't remove video yet because there's no action that results in video embed being removed. * Properly represent video as embed This aligns the video state lifetime with the embed lifetime. Video can now be properly added and removed. * Disable Add Video when we have images * Ignore empty image pick * Rename some files and variables (#5587) * Move composer reducers together * videoUploadState -> videoState * Inline videoDispatch * 🪵📌 (#5594) * [🐴] Reduce amount that message sent date is shown (#4228) * Fix profile header buttons (#5558) * Fix profile header buttons * Adjust labeler buttons too * Fix load state jumps * Small tweak for web * Remove log * Update * Revert "Update" This reverts commit b073c04. * Revert "Fix profile header buttons (#5558)" This reverts commit c00c5fa. * Revert "[🐴] Reduce amount that message sent date is shown (#4228)" This reverts commit af8e342. * Revert "🪵📌 (#5594)" This reverts commit 3ab5190. * Revert "Rename some files and variables (#5587)" This reverts commit 475708e. * Revert "Manage video reducer from composer reducer (#5573)" This reverts commit 59589e3. * Revert "Refactor video uploads (#5570)" This reverts commit f74f9bc. * Update translation * Update translation * Delete unused entries * Restored the trailing dot by #5622 --------- Co-authored-by: dan <[email protected]> Co-authored-by: Samuel Newman <[email protected]> Co-authored-by: Eric Bailey <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In #5547, I introduced a reducer for composer state management. Over time, we want to move the rest of meaningful composer state there so that we can later lift it up and make it work for multiple threaded posts. In #5547 I started with managing the image state there. In this PR, I'm extending this approach to also handle videos.
Integrating videos into the reducer was a bit tricky because they were managed by a custom hook with a combination of React Query and a separate reducer. So there was no easy way to let that state be controlled. To fix this, I've refactored video state management in #5570 to just be a plain reducer. Why? Because plain reducers compose.
In this PR, the way the video reducer is integrated has changed. Previously, it was given to
useReducer. Now, the videos reducer is called/composed from the composer reducer. Note how it's not always "there": its state gets created when you add the video embed, and gets destroyed when you remove the video embed. (To do this, I've made theidlestate represent the lack of a video, and excluded it from the state union.) It always starts its life in thecompressingstate now.Note how the video actions are now "packed" inside the composer actions. That's composition again.
Test Plan
Add/remove images. Add/remove video. Should work like before.
One consequence of better modeling is that now we can't add a video while we have images. (Previously the composer would let you do that but would silently omit images on post.) Since the action being (correctly) ignored is confusing, I've added greying out of the "add video" button when images are selected.