-
Notifications
You must be signed in to change notification settings - Fork 30
feat: Google Drive Video ifrmae support #163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @rui-reazon99, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, gemini-code-assist here with a summary of this pull request. This PR introduces support for embedding Google Drive videos directly within markdown content. It achieves this by adding a new filter that recognizes a specific markdown syntax, converts the provided Google Drive share URL into an embeddable preview URL, and replaces the original markdown with an <iframe>
element pointing to the preview URL. Additionally, the PR updates the iframe sanitization logic to explicitly allow iframes sourced from drive.google.com
for video previews, ensuring the generated embeds are not removed or overly modified.
Highlights
- New Feature: Google Drive Video Embeds: Adds the ability to embed Google Drive videos in markdown using a new syntax.
- New Markdown Syntax: Introduces the
@[gdrive_video](url)
syntax to specify Google Drive videos for embedding. - Embed Filter Implementation: Adds a new
HTML::Pipeline
filter (GoogleDriveVideo
) to detect the syntax, transform the URL, and generate the appropriate<iframe>
tag. - Iframe Sanitization Update: Modifies the
FilterIframe
transformer to explicitly allow iframes withsrc
attributes pointing todrive.google.com
for video previews. - Test Coverage: Includes new tests to verify the correct handling and sanitization (or lack thereof) of Google Drive video embed iframes.
Changelog
Click here to see the changelog
- lib/qiita/markdown.rb
- Added
require
statement for the newgoogle_drive_video
embed module to integrate it into the library.
- Added
- lib/qiita/markdown/embed/google_drive_video.rb
- Created a new module to define constants related to Google Drive video embeds, specifically the
SCRIPT_HOST
(drive.google.com
).
- Created a new module to define constants related to Google Drive video embeds, specifically the
- lib/qiita/markdown/filters/google_drive_video.rb
- Added a new
HTML::Pipeline
filter (GoogleDriveVideo
) responsible for finding the@[gdrive_video](url)
pattern in text nodes. - Implemented logic to extract the URL, convert it to a Google Drive preview embed URL (
/file/d/FILE_ID/preview
), and replace the markdown with a standard<iframe>
tag. - Includes a check to prevent processing within
pre
,code
, ortt
tags.
- Added a new
- lib/qiita/markdown/transformers/filter_iframe.rb
- Added
Embed::GoogleDriveVideo::SCRIPT_HOST
to the list of allowed iframe sources, permitting iframes fromdrive.google.com
for video previews.
- Added
- spec/qiita/markdown/processor_spec.rb
- Added new test cases specifically for handling HTML embed code for Google Drive videos.
- Tests confirm that iframes with Google Drive preview URLs are allowed and their attributes (like
width
) are not forcibly changed by theFilterIframe
transformer.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Markdown magic, new tag appears,
Google Drive video, calms all fears.
An iframe takes hold,
A story unfolds,
Through pixels, the message endears.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for embedding Google Drive Videos by converting share URLs to embeddable iframe code and updating tests and configuration accordingly.
- Added new tests in processor_spec.rb to validate the handling of Google Drive Video embed codes
- Introduced a new filter (google_drive_video.rb) to convert share URLs into iframe embed tags and updated the allowed script hosts
- Added a corresponding embed module and updated the main markdown file to require the new embed
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
spec/qiita/markdown/processor_spec.rb | Adds tests for Google Drive Video iframe handling |
lib/qiita/markdown/transformers/filter_iframe.rb | Registers the Google Drive Video script host |
lib/qiita/markdown/filters/google_drive_video.rb | Implements conversion of Google Drive share URLs to iframe embeds |
lib/qiita/markdown/embed/google_drive_video.rb | Defines the SCRIPT_HOST for Google Drive Video |
lib/qiita/markdown.rb | Requires the new google_drive_video embed module |
if embed_url | ||
%(<iframe src="#{embed_url}" width="640" height="480" frameborder="0" allowfullscreen="true"></iframe>) | ||
else | ||
Regexp.last_match[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears the gsub block is missing a closing 'end' after the if/else block; please add an extra 'end' to properly close the block.
Regexp.last_match[0] | |
Regexp.last_match[0] | |
end |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable feature: support for embedding Google Drive videos using a custom markdown tag @[gdrive_video](URL)
. The implementation approach is generally sound, integrating with the existing HTML pipeline.
I've identified a few areas for improvement, primarily concerning the robustness of URL parsing and the need for comprehensive tests for the new markdown syntax. There's also a minor typo in the PR title ("ifrmae" -> "iframe") and the description is empty, which would be good to update for clarity.
No explicit style guide was provided, so feedback regarding style adheres to common Ruby conventions and best practices.
Summary of Findings
- Missing Tests for Markdown Syntax: The new markdown syntax
@[gdrive_video](...)
is not covered by tests. It's crucial to add tests for theFilters::GoogleDriveVideo
to verify:
- Successful conversion of valid Google Drive URLs.
- Correct fallback behavior for invalid or unsupported Google Drive URLs (e.g., different domain, wrong path format).
- Non-conversion when the tag is within
pre
,code
, ortt
elements. - Handling of
http
vshttps
in the input URL.
- URL Parsing Robustness: The
convert_to_embed_url
method inlib/qiita/markdown/filters/google_drive_video.rb
could be more robust by usingAddressable::URI.parse
for initial URL validation and component extraction, and by using a more specific regex ([-\w]+
) for capturing Google Drive file IDs. - Hardcoded Iframe Height: The generated iframe has a hardcoded
height="480"
. While the width becomes responsive due toFilterIframe
, this fixed height might not be ideal for all video aspect ratios. This should be confirmed if it's the desired behavior. - Missing Newline at End of File: The file
lib/qiita/markdown/filters/google_drive_video.rb
is missing a newline character at the end. (Severity: Low, not commented directly due to review settings) - PR Title and Description: The PR title contains a typo ('ifrmae' instead of 'iframe'). The PR description is empty and could benefit from a brief summary of the feature. (Severity: Low, not commented directly due to review settings)
Merge Readiness
The pull request introduces a useful feature for embedding Google Drive videos. However, before merging, I strongly recommend addressing the high-severity issue of missing tests for the new markdown filter. Additionally, the suggestions for improving the robustness of the convert_to_embed_url
method should be considered.
Once these points, particularly the test coverage, are addressed, the PR will be in much better shape. As a reviewer, I am not authorized to approve pull requests, so please ensure further review and approval from authorized maintainers after making the necessary changes.
def convert_to_embed_url(share_url) | ||
if share_url =~ /drive\.google\.com\/file\/d\/([^\/]+)/ | ||
file_id = $1 | ||
"https://drive.google.com/file/d/#{file_id}/preview" | ||
else | ||
nil | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convert_to_embed_url
method could be made more robust and align better with existing patterns:
- URL Parsing: Instead of relying solely on regex for the
share_url
, consider usingAddressable::URI.parse
first. This would allow for more robust handling of URL components and easier validation of the host againstQiita::Markdown::Embed::GoogleDriveVideo::SCRIPT_HOST
. - File ID Regex: The current regex
drive\.google\.com\/file\/d\/([^\/]+)
captures the file ID with([^\/]+)
, which is quite broad. Google Drive file IDs typically consist of alphanumeric characters, hyphens, and underscores. A more specific regex like([-\w]+)
would be safer and more precise. - Capture Group Access: While
$1
is functional,Regexp.last_match[1]
is often preferred for clarity, especially if the method were to grow or be refactored.
Could we refactor this method to incorporate these points for better robustness and clarity? For example:
# private
#
# def convert_to_embed_url(share_url_string)
# begin
# uri = Addressable::URI.parse(share_url_string)
# rescue Addressable::URI::InvalidURIError
# return nil
# end
#
# return nil unless uri.host == Qiita::Markdown::Embed::GoogleDriveVideo::SCRIPT_HOST
#
# # Match /file/d/FILE_ID/...
# # Google Drive file IDs are typically alphanumeric with hyphens and underscores.
# match_data = uri.path.match(/^\/file\/d\/([-\w]+)/)
# return nil unless match_data
#
# file_id = match_data[1]
# "https://#{Qiita::Markdown::Embed::GoogleDriveVideo::SCRIPT_HOST}/file/d/#{file_id}/preview"
# end
This approach also centralizes the host check using the defined constant.
def convert_to_embed_url(share_url_string)
begin
uri = Addressable::URI.parse(share_url_string)
rescue Addressable::URI::InvalidURIError
return nil
end
return nil unless uri.host == Qiita::Markdown::Embed::GoogleDriveVideo::SCRIPT_HOST
# Google Drive file IDs are typically alphanumeric with hyphens and underscores.
match_data = uri.path.match(/^\/file\/d\/([-\w]+)/)
return nil unless match_data
file_id = match_data[1]
"https://#{Qiita::Markdown::Embed::GoogleDriveVideo::SCRIPT_HOST}/file/d/#{file_id}/preview"
end
video_url = Regexp.last_match[:url] | ||
embed_url = convert_to_embed_url(video_url) | ||
if embed_url | ||
%(<iframe src="#{embed_url}" width="640" height="480" frameborder="0" allowfullscreen="true"></iframe>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The iframe dimensions width="640" height="480"
are hardcoded here. I see that FilterIframe
(which runs later) will change the width
to "100%"
. However, the height="480"
will persist.
Is this fixed height of 480px the desired default for all embedded Google Drive videos? If videos have different aspect ratios, this could lead to letterboxing or incorrect display.
Just wanted to confirm if this fixed height is intentional and considered appropriate for various video aspect ratios, or if there are plans or thoughts on making the height more adaptive (though that's often more complex, sometimes requiring JavaScript or CSS aspect ratio tricks).
Hi, this is the first time that I've worked with Ruby.
Below is a summary of my work in this PR. Please let me know if anything needs to be corrected, thank you :)
What
Ref
#162