Skip to content

Conversation

@tedw87
Copy link
Contributor

@tedw87 tedw87 commented Oct 18, 2025

Caution

The Volto Team has suspended its review of new pull requests from first-time contributors until the release of Plone 7, which is preliminarily scheduled for the second quarter of 2026.
Read details.



If your pull request closes an open issue, include the exact text below, immediately followed by the issue number. When your pull request gets merged, then that issue will close automatically.

Closes #


If your pull request includes changes to the documentation—either in narrative documentation, Storybook, or configuration—then a pull request preview will be generated and a link will populate in the description of your pull request below.
By clicking that link, you can use the visual diff menu in the upper right corner to navigate to pages that have changes, then display the diff by checking the Show diff checkbox.

@pnicolli pnicolli requested a review from Copilot October 19, 2025 07:21
Copy link

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

Adds a Video block View for the Seven theme and introduces a lightweight isInternalURL helper to detect internal URLs.

  • Introduces isInternalURL helper with unit tests.
  • Registers and implements the Video block view, including body logic, styles, and tests.
  • Adds news entries and a stylelint config update for the blocks package.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/helpers/src/isInternalURL.ts Adds helper to detect internal URLs.
packages/helpers/src/isInternalURL.test.ts Unit tests for isInternalURL.
packages/helpers/src/index.ts Re-exports new helper.
packages/helpers/news/7518.feature News entry for helpers change.
packages/blocks/news/7518.feature News entry for Video block view.
packages/blocks/index.ts Registers the Video block in blocksConfig.
packages/blocks/Video/index.ts Declares Video block metadata and lazy-loaded view.
packages/blocks/Video/VideoBlockView.tsx Video block view container.
packages/blocks/Video/VideoBlockView.test.tsx Snapshot tests for VideoBlockView.
packages/blocks/Video/VideoBlockView.css Styles for the Video block view.
packages/blocks/Video/VideoBlockBody.tsx Core embed logic for YouTube, Vimeo, and mp4 sources.
packages/blocks/Video/VideoBlockBody.test.tsx Tests for parsing and rendering logic.
packages/blocks/.stylelintrc Stylelint config for the blocks package.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +9 to +21
const settings = config.settings ?? ({} as Record<string, string>);
const prefixes = [
settings.publicURL,
settings.internalApiPath,
settings.apiPath,
];

return (
prefixes.some((prefix) => prefix && url.startsWith(prefix as string)) ||
url.startsWith('/') ||
url.startsWith('.') ||
url.startsWith('#')
);
Copy link

Copilot AI Oct 19, 2025

Choose a reason for hiding this comment

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

Using startsWith against raw string prefixes can misclassify external domains that share the same leading substring as internal (e.g., https://example.com.evil.com will match a publicURL of https://example.com). Compare URL origins (protocol+host+port) when prefixes are absolute URLs and only treat them as internal if the origins match; keep the current handling for relative URLs and anchors.

Copilot uses AI. Check for mistakes.
Comment on lines 13 to 16
if (url.match('youtu')) {
if (url.match('list')) {
const matches = url.match(/^.*\?list=(.*)|^.*&list=(.*)$/);
listID = matches?.[1] || matches?.[2] || null;
Copy link

Copilot AI Oct 19, 2025

Choose a reason for hiding this comment

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

[nitpick] Prefer string includes or a RegExp .test(...) over match with a string literal to avoid implicit RegExp creation and improve clarity, e.g., use url.includes('youtu') and url.includes('list').

Copilot uses AI. Check for mistakes.
Comment on lines 96 to 98
if (data.url.match('youtu')) {
if (data.url.match('list') && listID) {
content = (
Copy link

Copilot AI Oct 19, 2025

Choose a reason for hiding this comment

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

[nitpick] Replace match('...') with includes('...') or a proper regex test for these substring checks to avoid implicit RegExp construction and to better convey intent.

Copilot uses AI. Check for mistakes.
Comment on lines 134 to 142
if (!content) {
const message = isEditMode
? 'Please enter a valid URL by deleting the block and adding a new video block.'
: '';
return (
<div className="invalid-video-format" aria-live="polite">
{message}
</div>
);
Copy link

Copilot AI Oct 19, 2025

Choose a reason for hiding this comment

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

Avoid rendering an empty aria-live region in view mode; return null when not in edit mode so assistive technologies are not presented with an empty live region.

Copilot uses AI. Check for mistakes.
@davisagli davisagli added this to Seven Oct 23, 2025
@tedw87 tedw87 requested review from pnicolli and sneridagh October 28, 2025 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant