Open
Conversation
Reviewer's GuideImplements generalized video URL support by centralizing video URL validation and error messaging, updating the video embed flow to use ReactPlayer’s capabilities, and improving the user-facing error handling for failed video loads. Sequence diagram for video URL paste and embed insertionsequenceDiagram
actor User
participant BrowserEditor
participant withPasted
participant VideoUrlUtils
participant ReactPlayer
participant Document
User->>BrowserEditor: Paste URL
BrowserEditor->>withPasted: handleURLPaste(editor, url)
withPasted->>VideoUrlUtils: isValidVideoUrl(url)
VideoUrlUtils->>UrlUtils: processUrl(url)
UrlUtils-->>VideoUrlUtils: processedUrl
alt processedUrl is http or https
VideoUrlUtils->>ReactPlayer: canPlay(processedUrl)
ReactPlayer-->>VideoUrlUtils: canPlayResult
else invalid protocol or no processedUrl
VideoUrlUtils-->>withPasted: false
end
VideoUrlUtils-->>withPasted: isVideoUrl
alt isVideoUrl is true
withPasted->>Document: insertBlock(type Video, url)
else isVideoUrl is false
withPasted-->>BrowserEditor: false (not handled)
end
Class diagram for updated video embed utilities and componentsclassDiagram
class VideoRender {
+VideoBlockNode node
+Function onError
+ReactPlayer player
+playerProps
+onDragStart()
}
class VideoBlockPopoverContent {
+string blockId
+Function onClose
+handleInsertEmbedLink(url)
}
class withPasted {
+handleURLPaste(editor, url) boolean
}
class VideoUrlUtils {
+isValidVideoUrl(url) boolean
+getVideoErrorMessage(url) string
}
class ReactPlayer {
+canPlay(url) boolean
}
class UrlUtils {
+processUrl(url) string
}
VideoRender ..> ReactPlayer : uses
VideoRender ..> VideoUrlUtils : getVideoErrorMessage
VideoBlockPopoverContent ..> VideoUrlUtils : isValidVideoUrl
withPasted ..> VideoUrlUtils : isValidVideoUrl
VideoUrlUtils ..> ReactPlayer : canPlay
VideoUrlUtils ..> UrlUtils : processUrl
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The user-facing strings in
getVideoErrorMessageare hard-coded English while the rest of the video UI appears to be localized; consider routing these through your i18n layer so error messages are consistent with the rest of the app. getVideoErrorMessagecurrently inspects the rawurlstring (e.g.,includes('facebook.com'), extension regex) whileisValidVideoUrlfirst normalizes withprocessUrl; using the normalized URL for both validation and error classification would avoid edge cases where processed/validated URLs don’t match the error logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The user-facing strings in `getVideoErrorMessage` are hard-coded English while the rest of the video UI appears to be localized; consider routing these through your i18n layer so error messages are consistent with the rest of the app.
- `getVideoErrorMessage` currently inspects the raw `url` string (e.g., `includes('facebook.com')`, extension regex) while `isValidVideoUrl` first normalizes with `processUrl`; using the normalized URL for both validation and error classification would avoid edge cases where processed/validated URLs don’t match the error logic.
## Individual Comments
### Comment 1
<location> `src/utils/video-url.ts:25-32` </location>
<code_context>
+/**
+ * Enhanced error message for video loading failures
+ */
+export function getVideoErrorMessage(url: string): string {
+ if (url.includes('facebook.com')) {
+ return 'Facebook video couldn\'t be loaded. Check privacy settings.';
+ }
+ if (url.match(/\.(mp4|webm|mov|ogv)$/i)) {
+ return 'Video file couldn\'t be loaded. Check URL and CORS settings.';
+ }
+ return 'The video embed couldn\'t be loaded. Check URL and privacy settings.';
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Error classification uses raw URLs while validation uses `processUrl`, which can lead to inconsistent behavior.
`isValidVideoUrl` normalizes via `processUrl`, but `getVideoErrorMessage` inspects the raw `url`. If `processUrl` alters the URL (e.g., protocol, case, domain), you can end up with URLs that validate successfully but don’t match the specific error checks (Facebook or file extensions). Please reuse `processUrl` (or equivalent normalization) in `getVideoErrorMessage` so error classification stays aligned with the validation logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
🥷 Ninja i18n – 🛎️ Translations need to be updatedProject
|
| lint rule | new reports | level | link |
|---|---|---|---|
| Missing translation | 112 | warning | contribute (via Fink 🐦) |
- Add type assertion for dynamic translation key in t() call - Remove unnecessary React fragment wrapper
LucasXu0
force-pushed
the
feat/support_custom_video_url
branch
from
377a86e to
7f18620
Compare
- Add eslint-disable-next-line for necessary any type in VideoEmpty.tsx - Fix padding-line-between-statements errors by adding blank lines - Ensure consistent code formatting across video-related files
LucasXu0
force-pushed
the
feat/support_custom_video_url
branch
from
eae7c29 to
48df2f6
Compare
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.
Description
Checklist
General
Testing
Feature-Specific
Summary by Sourcery
Support embedding and pasting a broader range of video URLs while improving validation and error feedback for video blocks.
New Features:
Enhancements: