Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rui-reazon99
Copy link

@rui-reazon99 rui-reazon99 commented May 21, 2025

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

  • Added a filter that converts Google Drive links into iframes if the content within the link is a video.
  • Enables videos from Google Drive to be displayed in the blog.

Ref

#162

@Copilot Copilot AI review requested due to automatic review settings May 21, 2025 10:03
Copy link

@gemini-code-assist gemini-code-assist bot left a 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 with src attributes pointing to drive.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 new google_drive_video embed module to integrate it into the library.
  • 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).
  • 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, or tt tags.
  • lib/qiita/markdown/transformers/filter_iframe.rb
    • Added Embed::GoogleDriveVideo::SCRIPT_HOST to the list of allowed iframe sources, permitting iframes from drive.google.com for video previews.
  • 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 the FilterIframe 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

  1. 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.

Copy link

@Copilot Copilot AI left a 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]
Copy link
Preview

Copilot AI May 21, 2025

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.

Suggested change
Regexp.last_match[0]
Regexp.last_match[0]
end

Copilot uses AI. Check for mistakes.

Copy link

@gemini-code-assist gemini-code-assist bot left a 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 the Filters::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, or tt elements.
  • Handling of http vs https in the input URL.
  • URL Parsing Robustness: The convert_to_embed_url method in lib/qiita/markdown/filters/google_drive_video.rb could be more robust by using Addressable::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 to FilterIframe, 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.

Comment on lines +28 to +35
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The convert_to_embed_url method could be made more robust and align better with existing patterns:

  1. URL Parsing: Instead of relying solely on regex for the share_url, consider using Addressable::URI.parse first. This would allow for more robust handling of URL components and easier validation of the host against Qiita::Markdown::Embed::GoogleDriveVideo::SCRIPT_HOST.
  2. 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.
  3. 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>)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant