From 6dd40c250724985470073517ebcd0784d3df6e92 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 4 Jun 2025 17:11:53 +0000 Subject: [PATCH 1/4] Initial plan for issue From dbcb1542c4767d173488364e92f4e33c61cee46b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 4 Jun 2025 17:22:14 +0000 Subject: [PATCH 2/4] Implement video URL deduplication for LazyVideo component Co-authored-by: weotch <77567+weotch@users.noreply.github.com> --- .../cypress/component/ContentfulVisual.cy.tsx | 28 ++++++++++++++++ .../WidthBasedContentfulVisual.cy.tsx | 31 ++++++++++++++++++ .../react/cypress/component/LazyVideo.cy.tsx | 32 +++++++++++++++++++ .../react/src/LazyVideo/LazyVideoServer.tsx | 13 +++++++- 4 files changed, 103 insertions(+), 1 deletion(-) diff --git a/packages/contentful/cypress/component/ContentfulVisual.cy.tsx b/packages/contentful/cypress/component/ContentfulVisual.cy.tsx index 19198e9..48f851e 100644 --- a/packages/contentful/cypress/component/ContentfulVisual.cy.tsx +++ b/packages/contentful/cypress/component/ContentfulVisual.cy.tsx @@ -211,4 +211,32 @@ describe("contentful visual entry props", () => { cy.viewport(400, 500); cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); }); + + it("handles same video asset for both portrait and landscape", () => { + // This test simulates the bug scenario: using the same Contentful video asset + // for both video and portraitVideo properties + cy.mount( + , + ); + + // Video should load in portrait mode (where portraitVideo is used) + cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); + + // Switch to landscape mode (where video is used) + cy.viewport(500, 400); + cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); + + // Switch back to portrait mode + cy.viewport(399, 500); + cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); + }); }); diff --git a/packages/contentful/cypress/component/WidthBasedContentfulVisual.cy.tsx b/packages/contentful/cypress/component/WidthBasedContentfulVisual.cy.tsx index 4165654..960563d 100644 --- a/packages/contentful/cypress/component/WidthBasedContentfulVisual.cy.tsx +++ b/packages/contentful/cypress/component/WidthBasedContentfulVisual.cy.tsx @@ -61,4 +61,35 @@ describe("contentful visual entry props", () => { cy.viewport(400, 500); cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); }); + + it("handles same video asset for both portrait and landscape with width-based breakpoints", () => { + // This test simulates using the same Contentful video asset for both portrait and landscape + // in the width-based responsive setup + cy.mount( + , + ); + + // Small screen (should use portraitVideo, but it's the same asset) + cy.viewport(399, 500); + cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); + + // Large screen (should use video, same asset) + cy.viewport(400, 500); + cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); + + // Switch back to small screen + cy.viewport(399, 500); + cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); + }); }); diff --git a/packages/react/cypress/component/LazyVideo.cy.tsx b/packages/react/cypress/component/LazyVideo.cy.tsx index 7916405..a26d90c 100644 --- a/packages/react/cypress/component/LazyVideo.cy.tsx +++ b/packages/react/cypress/component/LazyVideo.cy.tsx @@ -70,6 +70,38 @@ describe("responsive video", () => { cy.viewport(500, 600); cy.get("video").its("[0].currentSrc").should("contain", "portrait"); }); + + it("handles duplicate video URLs for different media queries", () => { + // This test simulates the Contentful scenario where the same video asset + // is used for both portrait and landscape, which should result in deduplication + const sameVideoUrl = "https://github.com/BKWLD/react-visual/raw/refs/heads/main/packages/react/cypress/fixtures/300x200.mp4"; + + cy.mount( + { + // Both media queries return the same URL, simulating Contentful behavior + return sameVideoUrl; + }} + alt="Duplicate video URL test" + />, + ); + + // Video should load in portrait mode + cy.get("video").its("[0].currentSrc").should("contain", "300x200.mp4"); + + // Switch to landscape - video should still load + cy.viewport(500, 250); + cy.get("video").its("[0].currentSrc").should("contain", "300x200.mp4"); + + // Switch back to portrait - video should still load + cy.viewport(500, 600); + cy.get("video").its("[0].currentSrc").should("contain", "300x200.mp4"); + }); }); describe("Accessibility controls", () => { diff --git a/packages/react/src/LazyVideo/LazyVideoServer.tsx b/packages/react/src/LazyVideo/LazyVideoServer.tsx index d746aae..158bb35 100644 --- a/packages/react/src/LazyVideo/LazyVideoServer.tsx +++ b/packages/react/src/LazyVideo/LazyVideoServer.tsx @@ -26,8 +26,19 @@ export default function LazyVideo(props: LazyVideoProps): ReactNode { // If the array ended up empty, abort if (mediaSrcEntries.filter(([url]) => !!url).length == 0) return null; + // Deduplicate entries to prevent conflicts when the same URL is returned + // for different media queries (e.g., same Contentful asset for portrait/landscape) + const seenUrls = new Set(); + const deduplicatedEntries = mediaSrcEntries.filter(([url, media]) => { + if (!url || seenUrls.has(url)) { + return false; + } + seenUrls.add(url); + return true; + }); + // Make the hash - mediaSrcs = Object.fromEntries(mediaSrcEntries); + mediaSrcs = Object.fromEntries(deduplicatedEntries); // Make a simple string src url } else { From 5400e6a67cf325d5fa55e588f4474af95f10a24f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 4 Jun 2025 18:03:25 +0000 Subject: [PATCH 3/4] Refactor video URL deduplication to use reducer pattern Co-authored-by: weotch <77567+weotch@users.noreply.github.com> --- packages/react/src/LazyVideo/LazyVideoServer.tsx | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/react/src/LazyVideo/LazyVideoServer.tsx b/packages/react/src/LazyVideo/LazyVideoServer.tsx index 158bb35..89d23d6 100644 --- a/packages/react/src/LazyVideo/LazyVideoServer.tsx +++ b/packages/react/src/LazyVideo/LazyVideoServer.tsx @@ -28,14 +28,12 @@ export default function LazyVideo(props: LazyVideoProps): ReactNode { // Deduplicate entries to prevent conflicts when the same URL is returned // for different media queries (e.g., same Contentful asset for portrait/landscape) - const seenUrls = new Set(); - const deduplicatedEntries = mediaSrcEntries.filter(([url, media]) => { - if (!url || seenUrls.has(url)) { - return false; + const deduplicatedEntries = mediaSrcEntries.reduce<[string, string][]>((acc, [url, media]) => { + if (!url || acc.some(([seenUrl]) => seenUrl === url)) { + return acc; } - seenUrls.add(url); - return true; - }); + return [...acc, [url, media]]; + }, []); // Make the hash mediaSrcs = Object.fromEntries(deduplicatedEntries); From 5aed612ad15ddc4b716bf56204f741b6f3dfc8c2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Jun 2025 15:58:18 +0000 Subject: [PATCH 4/4] Remove tests from contentful package as requested Co-authored-by: weotch <77567+weotch@users.noreply.github.com> --- .../cypress/component/ContentfulVisual.cy.tsx | 28 ----------------- .../WidthBasedContentfulVisual.cy.tsx | 31 ------------------- 2 files changed, 59 deletions(-) diff --git a/packages/contentful/cypress/component/ContentfulVisual.cy.tsx b/packages/contentful/cypress/component/ContentfulVisual.cy.tsx index 48f851e..19198e9 100644 --- a/packages/contentful/cypress/component/ContentfulVisual.cy.tsx +++ b/packages/contentful/cypress/component/ContentfulVisual.cy.tsx @@ -211,32 +211,4 @@ describe("contentful visual entry props", () => { cy.viewport(400, 500); cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); }); - - it("handles same video asset for both portrait and landscape", () => { - // This test simulates the bug scenario: using the same Contentful video asset - // for both video and portraitVideo properties - cy.mount( - , - ); - - // Video should load in portrait mode (where portraitVideo is used) - cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); - - // Switch to landscape mode (where video is used) - cy.viewport(500, 400); - cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); - - // Switch back to portrait mode - cy.viewport(399, 500); - cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); - }); }); diff --git a/packages/contentful/cypress/component/WidthBasedContentfulVisual.cy.tsx b/packages/contentful/cypress/component/WidthBasedContentfulVisual.cy.tsx index 960563d..4165654 100644 --- a/packages/contentful/cypress/component/WidthBasedContentfulVisual.cy.tsx +++ b/packages/contentful/cypress/component/WidthBasedContentfulVisual.cy.tsx @@ -61,35 +61,4 @@ describe("contentful visual entry props", () => { cy.viewport(400, 500); cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); }); - - it("handles same video asset for both portrait and landscape with width-based breakpoints", () => { - // This test simulates using the same Contentful video asset for both portrait and landscape - // in the width-based responsive setup - cy.mount( - , - ); - - // Small screen (should use portraitVideo, but it's the same asset) - cy.viewport(399, 500); - cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); - - // Large screen (should use video, same asset) - cy.viewport(400, 500); - cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); - - // Switch back to small screen - cy.viewport(399, 500); - cy.get("video").its("[0].currentSrc").should("contain", videoAsset.url); - }); });