fix: close Video Library drawer after Select (NES-1568)#9043
Open
fix: close Video Library drawer after Select (NES-1568)#9043
Conversation
When a user edits an existing video block and changes the audio language, clicking Select correctly saved the language but left the parent Video Library drawer open underneath — exposing the browse tabs and suggesting they still had to pick a different video. The onSelect handler was only closing the inner VideoDetails drawer via setOpenVideoDetails(false); it never invoked the outer onClose supplied by the parent (Source.tsx, BackgroundMediaVideo.tsx). A confirmed Select is a terminal action — both drawers should close. Regression-guarded: the Change Video button still keeps the outer drawer open so users can pick a different video.
The previous commit unconditionally called the parent drawer's onClose after a video block update. That broke the background MUX upload completion contract: AddByFile explicitly passes shouldCloseDrawer=false through VideoFromMux → VideoLibrary.onSelect so an upload finishing in the background cannot close a drawer the user is now using on a different block. Gate the new onClose?.() call on shouldCloseDrawer, mirroring the pattern already in handleVideoDetailsClose. This restores the background-completion contract and aligns the parameter name with real behaviour. Tests: - Tighten onClose assertions from toHaveBeenCalled to toHaveBeenCalledTimes(1). - Add regression guards (expect(onClose).not.toHaveBeenCalled()) to the YouTube and MUX Change-Video tests so every source type is protected against a future unconditional-close regression. - Add a new test that stubs VideoFromMux to fire onSelect(block, false) and asserts onClose is not called — covering the background upload completion path end-to-end. Plan updated (Requirements Trace R3, Key Technical Decisions, and per-unit Approach) to reflect the corrected call chain and gate.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThe changes fix a UX bug where the outer Video Library drawer remained open after confirming a video-block update by adding conditional drawer-closing logic controlled by a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
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 |
|
View your CI Pipeline Execution ↗ for commit ef1384f
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
`@docs/plans/2026-04-20-001-fix-video-library-stays-open-after-language-select-plan.md`:
- Line 158: The plan text incorrectly states that background MUX upload
completion does not trigger VideoLibrary.onSelect and instead goes through
VideoOptions.handleChange; update the risk row to reflect the actual
implementation and tests: background upload completions do flow through
VideoLibrary.onSelect with shouldCloseDrawer=false (which is why the gate
exists) and the regression test at the spec (lines 1090–1129) verifies this
behavior; ensure the row references VideoLibrary.onSelect,
VideoOptions.handleChange only as the alternate path, and mention
shouldCloseDrawer=false and the gate as the rationale so the plan and
implementation are consistent.
🪄 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: 4f5d9e49-15f1-4f29-99a9-54d74cbc229a
📒 Files selected for processing (3)
apps/journeys-admin/src/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoLibrary.spec.tsxapps/journeys-admin/src/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoLibrary.tsxdocs/plans/2026-04-20-001-fix-video-library-stays-open-after-language-select-plan.md
| |------|------------| | ||
| | A rare caller might rely on the outer drawer staying open after Select (e.g. a preview-multiple-videos flow). | None found in the codebase after reviewing `VideoLibrary` consumers (`Source`, `BackgroundMediaVideo`, `VideoOptions`). Test coverage will catch any hidden coupling. | | ||
| | Snapshot or storybook tests asserting the pre-bug DOM state could break. | Run `VideoLibrary.stories.tsx` and `VideoLibrary.spec.tsx` after the change and update any snapshots that reflect the new (correct) behaviour, with a note in the PR. | | ||
| | Background MUX upload completion inadvertently triggers `VideoLibrary.onSelect`. | Confirmed in Phase 1 research it does not — it goes through `VideoOptions.handleChange` directly. A test asserts upload-completion paths don't invoke `onClose`. | |
Contributor
There was a problem hiding this comment.
Minor inconsistency between the plan and the implemented fix.
Line 158's risk row states background MUX upload completion "does not" trigger VideoLibrary.onSelect and "goes through VideoOptions.handleChange directly" — but the rest of the plan (R3 on line 31, the resolved Q on line 96, and the Key Decision on line 86) correctly documents that completions do flow through VideoLibrary.onSelect with shouldCloseDrawer=false, which is exactly why the gate exists. Worth tightening this row so the rationale stays consistent with the actual implementation and the new regression test at lines 1090–1129 of the spec.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@docs/plans/2026-04-20-001-fix-video-library-stays-open-after-language-select-plan.md`
at line 158, The plan text incorrectly states that background MUX upload
completion does not trigger VideoLibrary.onSelect and instead goes through
VideoOptions.handleChange; update the risk row to reflect the actual
implementation and tests: background upload completions do flow through
VideoLibrary.onSelect with shouldCloseDrawer=false (which is why the gate
exists) and the regression test at the spec (lines 1090–1129) verifies this
behavior; ensure the row references VideoLibrary.onSelect,
VideoOptions.handleChange only as the alternate path, and mention
shouldCloseDrawer=false and the gate as the rationale so the plan and
implementation are consistent.
Contributor
|
The latest updates on your projects.
|
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
1 participant
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.
Summary
When a user edits an existing video block in Journey Builder and clicks Select to confirm a language (or video) change, the parent Video Library drawer stays open underneath — exposing the library browse tabs and making it look like the UI is asking them to pick a different video. This PR closes both drawers on a confirmed Select while preserving the background MUX upload completion contract (uploads finishing in the background must not yank a drawer the user has moved on from).
Reported by Lucinda (JFP) while preparing 34 World Cup journey translations. Confirmed by Siyang in #nextsteps-bugs on 2026-04-17 NZ time. Related UX overhaul tracked in QA-221.
Changes
apps/journeys-admin/src/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoLibrary.tsx— after closing the innerVideoDetailsdrawer inonSelect, also callonClose?.()on the parent, gated on the existingshouldCloseDrawerparameter. Mirrors the pattern already inhandleVideoDetailsClose(if (closeParent === true) onClose?.()).apps/journeys-admin/src/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoLibrary.spec.tsx— new and extended tests:shouldCloseDrawer=false) does NOT close the outer drawer;toHaveBeenCalled()totoHaveBeenCalledTimes(1)to guard against double-fire regressions.docs/plans/2026-04-20-001-fix-video-library-stays-open-after-language-select-plan.md— plan doc.Why the gate matters
AddByFile.onChange(videoId, false)→VideoFromMux.handleChange→VideoLibrary.onSelect(block, false)is the background-upload completion path. It deliberately passesshouldCloseDrawer=falseso a completion firing after the user has navigated to a different block cannot close the drawer they are now using. Gating the newonClose?.()on that existing parameter restores the contract and happens to realign the parameter name with real behaviour.Test plan
npx jest --config apps/journeys-admin/jest.config.ts --no-coverage 'apps/journeys-admin/src/components/Editor/Slider/Settings/Drawer/VideoLibrary/'— 15 suites, 102 pass (1 pre-existing skip)npx jeston the three direct consumers (Source,VideoOptions,BackgroundMediaVideo) — 3 suites, 15 passLinks
🤖 Generated with Claude Code
Summary by CodeRabbit