feat: if video cannot play open in a new tab #1554
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContentModal
participant VideoElement
User->>ContentModal: Open modal with video
ContentModal->>ContentModal: Reset error, set isLoading=true
ContentModal->>VideoElement: Render video
VideoElement-->>ContentModal: onLoadStart
VideoElement-->>ContentModal: onLoadedData (success)
ContentModal->>ContentModal: Set isLoading=false
alt Video load error
VideoElement-->>ContentModal: onError
ContentModal->>ContentModal: Set videoError=true, isLoading=false
ContentModal->>User: Show error message with actions
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
web/src/components/table/task-logs/modals/ContentModal.jsx (5)
35-41: Reset loading/error when the video URL changes.Include modalContent in deps to properly reset state if the source changes while open.
- }, [isModalOpen, isVideo]); + }, [isModalOpen, isVideo, modalContent]);
117-126: Force video remount on URL change to avoid stale state.Add a stable key; also consider playsInline for mobile autoplay behavior.
- <video + <video + key={modalContent} src={modalContent} controls style={{ width: '100%' }} autoPlay + playsInline crossOrigin="anonymous" onError={handleVideoError} onLoadedData={handleVideoLoaded} onLoadStart={() => setIsLoading(true)} />
42-45: Align behavior with PR title: auto-open on error (subject to popup blockers).If the requirement is to auto-open when playback fails, invoke the handler on error. Note many browsers block non-user-initiated window.open; keep the buttons as fallback.
const handleVideoError = () => { setVideoError(true); setIsLoading(false); + // Optional: auto-open fallback (may be blocked by popup settings) + handleOpenInNewTab(); };If desired, we can gate this behind a prop (e.g., autoOpenOnError) for configurability.
107-115: Spinner overlay should not intercept clicks.Set pointer-events to none so it doesn’t block interactions when visible.
- zIndex: 10 + zIndex: 10, + pointerEvents: 'none' }}>
61-75: Prefer semantic list markup for accessibility.Replace multiple Text blocks with a ul/li list for screen readers; keep styles as needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/components/table/task-logs/modals/ContentModal.jsx(1 hunks)
🔇 Additional comments (1)
web/src/components/table/task-logs/modals/ContentModal.jsx (1)
121-123: Verify need for crossOrigin on video.crossOrigin="anonymous" often triggers CORS failures unless the CDN sends appropriate CORS headers for media. If not strictly required, consider removing it or making it conditional.
| const handleCopyUrl = () => { | ||
| navigator.clipboard.writeText(modalContent); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clipboard: add error handling and user feedback (fallback included).
Improve resiliency and UX when copying.
- const handleCopyUrl = () => {
- navigator.clipboard.writeText(modalContent);
- };
+ const handleCopyUrl = async () => {
+ try {
+ await navigator.clipboard.writeText(modalContent);
+ Toast.success('链接已复制');
+ } catch {
+ try {
+ const el = document.createElement('textarea');
+ el.value = modalContent;
+ el.setAttribute('readonly', '');
+ el.style.position = 'absolute';
+ el.style.left = '-9999px';
+ document.body.appendChild(el);
+ el.select();
+ const ok = document.execCommand('copy');
+ document.body.removeChild(el);
+ if (ok) {
+ Toast.success('链接已复制');
+ } else {
+ throw new Error('copy failed');
+ }
+ } catch {
+ Toast.error('复制失败');
+ }
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleCopyUrl = () => { | |
| navigator.clipboard.writeText(modalContent); | |
| }; | |
| const handleCopyUrl = async () => { | |
| try { | |
| await navigator.clipboard.writeText(modalContent); | |
| Toast.success('链接已复制'); | |
| } catch { | |
| try { | |
| const el = document.createElement('textarea'); | |
| el.value = modalContent; | |
| el.setAttribute('readonly', ''); | |
| el.style.position = 'absolute'; | |
| el.style.left = '-9999px'; | |
| document.body.appendChild(el); | |
| el.select(); | |
| const ok = document.execCommand('copy'); | |
| document.body.removeChild(el); | |
| if (ok) { | |
| Toast.success('链接已复制'); | |
| } else { | |
| throw new Error('copy failed'); | |
| } | |
| } catch { | |
| Toast.error('复制失败'); | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
In web/src/components/table/task-logs/modals/ContentModal.jsx around lines 51 to
53, the handleCopyUrl function currently writes text to the clipboard without
error handling or user feedback. Update this function to include a try-catch
block to handle potential errors when accessing the clipboard API. Provide user
feedback on success or failure, and implement a fallback method for copying the
text if the clipboard API is unavailable or fails.
| const handleOpenInNewTab = () => { | ||
| window.open(modalContent, '_blank'); | ||
| }; |
There was a problem hiding this comment.
Secure window.open and validate URL before opening (prevent tabnabbing + unsafe schemes).
Harden the open logic and block unsafe protocols. Also add Toast feedback.
Apply these diffs:
- import { Modal, Button, Typography, Spin } from '@douyinfe/semi-ui';
+ import { Modal, Button, Typography, Spin, Toast } from '@douyinfe/semi-ui';- const handleOpenInNewTab = () => {
- window.open(modalContent, '_blank');
- };
+ const handleOpenInNewTab = () => {
+ try {
+ const u = new URL(modalContent, window.location.href);
+ if (!['http:', 'https:', 'blob:'].includes(u.protocol)) {
+ Toast.error('链接协议不被支持');
+ return;
+ }
+ const w = window.open(u.toString(), '_blank', 'noopener,noreferrer');
+ if (w) {
+ w.opener = null;
+ } else {
+ Toast.warning('浏览器阻止了弹窗，请允许弹窗或手动点击按钮');
+ }
+ } catch {
+ Toast.error('链接无效');
+ }
+ };Also applies to: 21-21
🤖 Prompt for AI Agents
In web/src/components/table/task-logs/modals/ContentModal.jsx around lines 55 to
57, the handleOpenInNewTab function uses window.open without validating the URL
or securing against tabnabbing. To fix this, validate that modalContent is a
safe URL with allowed protocols (e.g., http, https), prevent tabnabbing by
adding rel="noopener noreferrer" to the window.open call, and add a Toast
notification to inform the user if the URL is invalid or blocked.
| closable={null} | ||
| bodyStyle={{ height: '400px', overflow: 'auto' }} | ||
| bodyStyle={{ | ||
| height: isVideo ? '450px' : '400px', | ||
| overflow: 'auto', | ||
| padding: isVideo && videoError ? '0' : '24px' | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid non-boolean closable prop; make modal sizing responsive.
Use a proper boolean and use viewport-based height for better small-screen UX.
- closable={null}
- bodyStyle={{
- height: isVideo ? '450px' : '400px',
- overflow: 'auto',
- padding: isVideo && videoError ? '0' : '24px'
- }}
+ closable={false}
+ bodyStyle={{
+ height: isVideo ? '70vh' : '60vh',
+ maxHeight: '80vh',
+ overflow: 'auto',
+ padding: isVideo && videoError ? '0' : '24px'
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| closable={null} | |
| bodyStyle={{ height: '400px', overflow: 'auto' }} | |
| bodyStyle={{ | |
| height: isVideo ? '450px' : '400px', | |
| overflow: 'auto', | |
| padding: isVideo && videoError ? '0' : '24px' | |
| }} | |
| closable={false} | |
| bodyStyle={{ | |
| height: isVideo ? '70vh' : '60vh', | |
| maxHeight: '80vh', | |
| overflow: 'auto', | |
| padding: isVideo && videoError ? '0' : '24px' | |
| }} |
🤖 Prompt for AI Agents
In web/src/components/table/task-logs/modals/ContentModal.jsx around lines 136
to 141, the closable prop is set to null which is not a valid boolean value and
the modal height is fixed in pixels. Change the closable prop to a proper
boolean value (true or false) as required. Replace the fixed height values with
viewport-relative units (e.g., vh) to make the modal height responsive and
improve usability on small screens.
部分视频地址无法在模态框中直接播放, 如火山云
无法播放的改为支持弹出新窗口播放
Summary by CodeRabbit
New Features
Bug Fixes