fix: full screen video resizing and moving message list#40993
fix: full screen video resizing and moving message list#40993MartinSchoeler wants to merge 2 commits into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (2)📚 Learning: 2026-03-27T14:52:56.865ZApplied to files:
📚 Learning: 2026-05-06T12:21:44.083ZApplied to files:
🔇 Additional comments (1)
Walkthrough
ChangesVideo Preview Height Lock
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40993 +/- ##
===========================================
- Coverage 70.13% 70.09% -0.05%
===========================================
Files 3358 3358
Lines 129563 129574 +11
Branches 22405 22444 +39
===========================================
- Hits 90866 90821 -45
- Misses 35386 35441 +55
- Partials 3311 3312 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| ref={mediaRef} | ||
| onLoadedMetadata={(event) => { | ||
| const video = event.currentTarget as HTMLVideoElement; | ||
| setLockedHeight(video.parentElement?.offsetHeight); |
There was a problem hiding this comment.
isn't this too weak? if this structure changes, the locator no longer points to where we are expecting...
maybe add ref to the element we want to track the size and change its height directly through the ref?
| <MessageGenericPreview style={{ maxWidth: 368, width: '100%', minHeight: lockedHeight }}> | ||
| <Box | ||
| is='video' | ||
| minHeight={80} | ||
| controls | ||
| preload='metadata' | ||
| ref={mediaRef} | ||
| onLoadedMetadata={(event) => { | ||
| const video = event.currentTarget as HTMLVideoElement; | ||
| setLockedHeight(video.parentElement?.offsetHeight); | ||
| }} | ||
| > |
There was a problem hiding this comment.
no need for useState this way:
| <MessageGenericPreview style={{ maxWidth: 368, width: '100%', minHeight: lockedHeight }}> | |
| <Box | |
| is='video' | |
| minHeight={80} | |
| controls | |
| preload='metadata' | |
| ref={mediaRef} | |
| onLoadedMetadata={(event) => { | |
| const video = event.currentTarget as HTMLVideoElement; | |
| setLockedHeight(video.parentElement?.offsetHeight); | |
| }} | |
| > | |
| <MessageGenericPreview ref={previewRef} style={{ maxWidth: 368, width: '100%' }}> | |
| <Box | |
| is='video' | |
| minHeight={80} | |
| controls | |
| preload='metadata' | |
| ref={mediaRef} | |
| onLoadedMetadata={() => { | |
| if (previewRef.current) { | |
| previewRef.current.style.minHeight = `${previewRef.current.offsetHeight}px`; | |
| } | |
| }} | |
| > |
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
CORE-2272
Steps to test or reproduce
Further comments
Summary by CodeRabbit