Improve homepage brewery video reliability and replace fallback poster - #136
Merged
Merged
Conversation
- Re-encode both sources: MP4 H.264 Main→High CRF 27 faststart (13.6MB→2.1MB) and WebM VP9 CRF 33 (4.3MB→2.2MB), SSIM ≥0.98 vs the originals, no audio tracks, same 1280x720/24fps/21.25s. - Swap the brewery section poster from the duplicated grain hero image to the approved video-still.jpg derivative (PNG source 678KB → JPEG 78.7KB; the PNG remains the committed source). - Log play() rejections and a fully-exhausted source list in development only — expected autoplay denials stay out of monitoring. - Document the deliberate <=768px static policy and update the hero video diagnostic script + performance notes. - Add hero-video.spec.ts: still-not-hero-poster split, desktop source order/attributes, still preserved until canplay, refused play() leaves the design intact, no layout shift, mobile + reduced-motion static. Closes #135 Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideThe PR makes the brewery video substantially smaller and faster to load, replaces the duplicated grain poster with the approved brewery still, adds development-only playback diagnostics while preserving graceful static fallback behavior, and expands smoke coverage for desktop, mobile, reduced-motion, and failure scenarios. Sequence diagram for reliable brewery video playbacksequenceDiagram
participant Browser
participant HeroVideo
participant Video
participant CDN
Browser->>HeroVideo: IntersectionObserver enters view
HeroVideo->>Video: set preload to metadata
HeroVideo->>Video: play()
Video->>CDN: Fetch WebM source
alt WebM unsupported or fails
Video->>CDN: Fetch MP4 fallback
end
alt canplay
Video-->>HeroVideo: onCanPlay()
HeroVideo-->>Browser: Fade from still to video
else play() rejected or all sources fail
Video-->>HeroVideo: Rejection or last source onError
HeroVideo-->>Browser: Keep video-still.jpg visible
HeroVideo-->>HeroVideo: Development-only console.info()
end
Flow diagram for responsive brewery video behaviorflowchart TD
A[Render brewery section] --> B{Reduced motion or viewport <= 768px?}
B -->|Yes| C[Show video-still.jpg only]
B -->|No| D[Render video with WebM then MP4 sources]
D --> E{Section enters viewport?}
E -->|No| F[Keep approved still visible]
E -->|Yes| G["Set preload to metadata and call play()"]
G --> H{Video reaches canplay?}
H -->|Yes| I[Fade in video]
H -->|No| C
G -->|Rejected or sources fail| C
File-Level Changes
Possibly linked issues
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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="smoke-tests/hero-video.spec.ts" line_range="76" />
<code_context>
+ const video = section.locator("video");
+ await expect(still).toBeVisible();
+ // Before readiness the video is transparent over the still.
+ if (!(await video.evaluate((v) => (v as HTMLVideoElement).readyState >= 4))) {
+ await expect(video).toHaveClass(/opacity-0/);
+ }
+ // Once the browser reports it can play, the video fades in — the still
</code_context>
<issue_to_address>
**issue (testing):** The test requires `readyState >= 4` before skipping the opacity assertion, but the component sets `canPlay` on `canplay`, which fires when the ready state is only `HAVE_FUTURE_DATA` (3). When playback reaches `canplay` with readyState 3, the test still expects `opacity-0` even though the component correctly renders `opacity-100`, causing a false failure.
**Triggers:** When headless playback reaches `canplay` before the video reaches `HAVE_ENOUGH_DATA`.
**Suggested fix:** Check the component's readiness condition (`readyState >= 3` or the `canplay` event) instead of requiring `readyState >= 4`.
```suggestion
if (!(await video.evaluate((v) => (v as HTMLVideoElement).readyState >= 3))) {
```
</issue_to_address>
### Comment 2
<location path="smoke-tests/hero-video.spec.ts" line_range="79-89" />
<code_context>
+ if (!(await video.evaluate((v) => (v as HTMLVideoElement).readyState >= 4))) {
+ await expect(video).toHaveClass(/opacity-0/);
+ }
+ // Once the browser reports it can play, the video fades in — the still
+ // remains mounted underneath regardless.
+ await video
+ .evaluate(
+ (v) =>
+ new Promise<void>((resolve) => {
+ const el = v as HTMLVideoElement;
+ if (el.readyState >= 4) resolve();
+ else el.addEventListener("canplay", () => resolve(), { once: true });
+ setTimeout(resolve, 15000);
+ })
+ )
+ .catch(() => {});
</code_context>
<issue_to_address>
**issue (testing):** The readiness wait always resolves after 15 seconds without asserting that `canplay` occurred, and the surrounding `.catch(() => {})` suppresses failures. The test therefore passes even when the video never becomes playable, so it does not verify the documented fade-in/readiness behavior.
**Triggers:** When both video playback and the `canplay` event fail or are unavailable.
**Suggested fix:** Use a bounded Playwright expectation or reject the promise on timeout, then assert the expected opacity/readiness state separately.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: smoke-tests/hero-video.spec.ts:76, smoke-tests/hero-video.spec.ts:89
Per review: the wait previously resolved on a timeout even if canplay never arrived, and gated the opacity assertion on readyState 4 while the component fades in at canplay (readyState 3). The test now waits for the opacity-100 transition directly (bounded), so it genuinely fails if the video never becomes playable. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
canplay fires even when play() is refused (the fetch proceeds), so the old threshold could fade in a frozen first frame over the still. The video now fades in on the playing event — actual rendered frames — so the approved still remains whenever playback never starts. The refused-play test now stubs the media route, leaving no in-flight fetch to abort at teardown (CI requestfailed noise). Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This branch was successfully deployed
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.
Summary
Two-part fix for #135:
Why users saw only a static image. Partly by design (mobile ≤768px and reduced-motion intentionally render only the poster), partly because Safari/iOS and older browsers fall through VP9 WebM to a 13.6MB, 5.1Mbps MP4 — heavy enough that
canplaymay never arrive on constrained connections, leaving the static state indefinitely. Autoplay denials were also silently swallowed. No codec/MIME/range/faststart defect was found — production delivery is verified correct (correctContent-Type,Accept-Ranges: bytes→206,moovbeforemdat).Static experience now looks intentional. The brewery section poster is the owner-approved brewery still instead of a duplicate of the grain hero above it.
Closes #135
Changes
public/videos/ddbwebvid.mp4— re-encoded H.264 High CRF 27,-movflags +faststart, no audio: 13.6MB → 2.1MB (−85%), SSIM 0.985 vs original.public/videos/ddbwebvid.webm— re-encoded VP9 CRF 33, no audio: 4.3MB → 2.2MB (−48%), SSIM 0.990. Same 1280×720/24fps/21.25s. Source order unchanged (WebM first, MP4 fallback for Safari/iOS).public/photos/video-still.jpg— new 78.7KB JPEG derivative of the approvedvideo-still.png(1280×720, committed as the source).components/hero-video.tsx—POSTER_SRC→video-still.jpg; dev-onlyconsole.infodiagnostics onplay()rejection (logs theDOMExceptionname +MediaErrorcode) and on the last<source>error, so genuine failures are distinguishable locally without sending expected conditions to monitoring. Preload→IntersectionObserver→play()sequence andcanplayfade threshold verified correct and unchanged.smoke-tests/hero-video.spec.ts— new (7 tests): still-vs-hero poster split, desktop source order/attributes, still preserved until readiness, refusedplay()leaves the design intact, no layout shift/overflow, mobile + reduced-motion static-only.scripts/hero-video-check.mjs,docs/operations/performance.md— updated stale references/sizes.Verification
npm ci·check:react-versions·tsc --noEmit·lint— passnpm test— 355/355 ·test:rules— 29/29 ·build— passtest:smoke— 130/130 (incl. 7 new tests)check:md-links— pass ·npm audit --omit=dev— 0 vulnsreadyState4, fading in) — verified live, not just DOM<video>, no overflownext startplaysInline+muted is the correct coverage, and Playwright WebKit equivalence is noted as a limitationRisk / deployment notes
Static
public/assets serve withmust-revalidate(unhashed names) — repeat visits revalidate via ETag, so updated bytes deploy cleanly. No config, schema, or API changes.Generated with Devin
Summary by Sourcery
Improve the brewery video experience by optimizing its media assets, using an intentional static still, and preserving graceful behavior when playback is unavailable.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: