Repository navigation
fix(viewer): Fit recorded video and scrub bar within the browser window - #6
Merged
Merged
Conversation
Co-authored-by: Codex <noreply@openai.com>
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
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.
Intent
Fix ProofShot viewer.html on constrained browser viewports such as Doggo's 1280x577 Browser so the entire recorded video, native controls, and ProofShot scrub bar remain visible without distorting the recorded aspect ratio or misaligning overlays. Preserve responsive mobile behavior and legacy sessions whose aspect ratio becomes known only after metadata loads. The user explicitly reported the previously merged intrinsic-dimension fix was still visibly clipped on Doggo.
What Changed
src/artifacts/viewer.tsto a window-height flex column:bodyis100vhwithoverflow: hidden, the header is capped atmax-height: 50%with internal scrolling, and.viewergrows into the remainder viaflex: 1 1 auto; min-height: 0(replacing the fixedcalc(100vh - 180px))..video-fitsizing path — the container carries anaspect-ratiofrom the recorded viewport, and a newfitVideoToAvailableSpace()computes the fitted width from the intrinsic (or recorded) video size and applies it to.video-wrapper, so the scrub bar spans exactly the video width. It re-runs onresize, onloadedmetadata(covering legacy sessions with no recorded viewport), and on description show/hide; the≤768pxmedia query restores block layout,height: auto, and page scrolling.src/artifacts/viewer.test.tswith astyleRule()helper and assertions for the fit container markup,recordedVideoSizescript constant, body/.viewer/.video-fitrules, and scrub-bar/header sizing; updatedcontent/docs/concepts/interactive-viewer.mdxto describe the desktop window-fit behavior alongside the mobile stacking.Risk Assessment
✅ Low: The change is bounded to one generated artifact, resolves both prior warnings with targeted test coverage, and satisfies every stated acceptance criterion; the only residual is a few pixels of scrub-bar clipping in a narrow race that the existing
loadedmetadatare-fit usually repairs.Testing
Ran the two targeted vitest files for the changed module and its consumer (viewer.test.ts, stop.test.ts — all green), then produced the real evidence in Chrome: generated viewer.html from both the base and target commits against a labelled 1280x720 recording and rendered each at Doggo's 1280x577 viewport. The baseline reproduces the reported bug (recording clipped 21px below the fold, scrub bar entirely off-screen, page not scrollable); the target commit fits the whole recording, native controls, and scrub bar on screen with the recorded aspect ratio preserved and overlays and scrub bar pixel-aligned to the fitted video. Also verified the legacy no-viewport-metadata path, the mobile stacked layout, a live resize sweep down to 450px height, the long-description header cap with Show more, and that the new
body { overflow: hidden }does not strand side-panel content. Screenshots for before, after, legacy, mobile, long-description, and overlay-during-playback are in the evidence directory. One caveat: the mobile capture is 500x667 rather than 375 wide because macOS Chrome will not size a window narrower than 500px, but themax-width: 768pxbranch is the one exercised./var/folders/xh/qnn4w61965q9ml19t2yy0fk00000gp/T/no-mistakes-evidence/01KZY1WK38CDSXNKC43AADWN71/01-before-1280x577-clipped.png)/var/folders/xh/qnn4w61965q9ml19t2yy0fk00000gp/T/no-mistakes-evidence/01KZY1WK38CDSXNKC43AADWN71/02-after-1280x577-fits.png)/var/folders/xh/qnn4w61965q9ml19t2yy0fk00000gp/T/no-mistakes-evidence/01KZY1WK38CDSXNKC43AADWN71/03-after-legacy-no-viewport-metadata-1280x577.png)/var/folders/xh/qnn4w61965q9ml19t2yy0fk00000gp/T/no-mistakes-evidence/01KZY1WK38CDSXNKC43AADWN71/04-after-mobile-500x667-fullpage.png)/var/folders/xh/qnn4w61965q9ml19t2yy0fk00000gp/T/no-mistakes-evidence/01KZY1WK38CDSXNKC43AADWN71/05-after-long-description-expanded-1280x577.png)/var/folders/xh/qnn4w61965q9ml19t2yy0fk00000gp/T/no-mistakes-evidence/01KZY1WK38CDSXNKC43AADWN71/06-after-overlay-aligned-during-playback-1280x577.png)Evidence: Measured geometry, before vs after, 1280x577 viewport
Evidence: Evidence generator used to produce the before/after viewer.html pair
Pipeline
Updates from git push no-mistakes
✅ **Intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
src/artifacts/viewer.ts:717- The video is now height-driven and centered, but the scrub bar underneath it is still full-width, so the two no longer form one card..video-wrappergainedalign-items: center(line 717) while.video-container.video-fitiswidth: auto; max-width: 100%(line 725-730) and.scrub-barkeepswidth: 100%(line 760). Concretely at 1280x577 with a 1280x800 recording: the panel content box is ~761px wide, available height ~371px, so the container resolves to 371 x ~594px and is centered — while the scrub bar spans the full 761px, overhanging ~83px on each side and breaking the joinedborder-radius: 8px 8px 0 0/0 0 8px 8pxseam. Functionally the track still seeks correctly (it maps against its own rect), but the markers no longer sit under the video they annotate. Fix by making the scrub bar share the video's resolved width (e.g. move the aspect-fit sizing onto a shared inner element that wraps both, or set.scrub-bar { width: auto; align-self: stretch }inside an inner box sized by the video).src/artifacts/viewer.ts:483-bodynow hasheight: 100vh+overflow: hidden(line 480-483) and.headerisflex: 0 0 auto(line 487), so the page can never scroll and the header can never shrink. The description renders 2-line-clamped with a "Show more" button (line 287,toggleDescription), which expands it in place with no cap. On a 577px-tall viewport a ~20-line description grows the header past ~550px;.viewerisflex: 1 1 auto; min-height: 0(line 697-698) so it collapses toward zero, hiding the video, native controls, and scrub bar entirely — and withoverflow: hiddenthere is no scrollbar to recover them. Before this changebodywasmin-height: 100vhand the page scrolled. Suggested fix: cap header growth and let it scroll internally (.header { flex: 0 1 auto; max-height: 50%; overflow-y: auto }) so the viewer keeps a usable floor.src/artifacts/viewer.test.ts:188- The new regression assertions match whole CSS rule blocks verbatim, including declarations unrelated to the layout invariant being protected — thebodyassertion (line 188-195) pins thefont-familylist andbackground/colorvalues, so any unrelated theme tweak fails a test named for video sizing. Narrow the assertions to the declarations that carry the invariant (height: 100vh,overflow: hidden,flex: 1 1 auto,min-height: 0).🔧 Fix: Align scrub bar to fitted video and cap header height
1 warning still open:
src/artifacts/viewer.ts:1365- The height-fit invariant is no longer enforced by CSS (.video-container.video-fitis nowflex: 0 0 auto; width: 100%, so the card height is derived from the px width JS writes onto.video-wrapper). It is enforced only byfitVideoToAvailableSpace()at three hand-enumerated triggers: script parse (line 1365),resize(line 1366),toggleDescription(line 1294), andloadedmetadata(line 1745). One layout mutation is missed:initDescription()(line 1274-1286) reveals the "Show more" button inside arequestAnimationFrame, which fires after the line-1365 fit. For any description longer than the 2-line clamp, the header grows ~18px,.viewershrinks by the same amount, and the card keeps its already-computed height — so it overflows the wrapper..video-panel's 16px padding absorbs most of it, leaving ~2px of the 21px scrub bar clipped, and theloadedmetadatare-fit usually repairs it if metadata resolves after the rAF. Small in magnitude, but it is the same stale-measurement class the change exists to close. Earliest shared boundary: replace the ad-hoc call sites with a singleResizeObserveron.video-panel(safe — writing the wrapper width cannot resize aflex: 0 0 62%; overflow: hiddenpanel, so no observer loop), rather than adding a fourth trigger inside the rAF.✅ **Build** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
npx vitest run src/artifacts/viewer.test.ts— 5 passednpx vitest run src/commands/stop.test.ts— 13 passed (viewer.html consumer)Generated viewer.html from both1a8b119(before) andeb28a83(after) via esbuild-bundledgenerateViewer, paired with a real 1280x720 VP9 recording whose top/bottom edges are labelled so clipping is visibleChrome DevTools: loaded before/viewer.html at 1280x577 and measured video + scrub rects — video bottom 598px vs 577px viewport, scrub bar 598-619px off-screen,document.scrollHeight === innerHeight(unreachable)Chrome DevTools: loaded after/viewer.html at 1280x577 — video 657x370 fully visible, scrub 539-560px visible,overlayAlignedToVideoandscrubAlignedToVideotrue, rendered aspect 1.7757 vs recorded 1.7778Legacy session (viewer generated withviewport: undefined,recordedVideoSize = null) at 1280x577 —video-fitclass andaspect-ratio: 1280 / 720applied afterloadedmetadata, same fitted geometryMobile branch at 500x667 (Chrome minimum window width;matchMedia('(max-width: 768px)')confirmed active) — stacked layout, wrapper inline width cleared, page scrolls, full-page screenshot capturedLive resize sweep without reload at heights 900 / 577 / 450 — video re-fits each time (762x428 / 657x370 / 432x243), aspect 1.778 throughout, scrub always visible and width-matchedLong-description variant at 1280x577: clickedShow more— header capped at exactly 289px (50% of 577) with own scroll, video re-fit 597x336 -> 419x236, nothing clippedOverlay accuracy: seeked to a click entry with element bbox (300,150,200x60) in a 1280x720 frame, polled for.rippleduring playback — rendered centre within 0.1px of the proportional expected point inside the fitted video boxRegression probe forbody { overflow: hidden }: switched to the Console tab at 1280x577 — timeline panel stays within the viewport with its ownoverflow-y: auto, video still fully visibledocs/architecture.md:106- docs/architecture.md:106-131 keeps a second full copy of the viewer layout diagram and feature inventory that content/docs/concepts/interactive-viewer.mdx owns. This change did not make the architecture.md copy stale (it states nothing about video sizing or page scrolling), so I updated only the owner and left the duplicate alone — but the two copies will keep drifting on every viewer change. Proposed follow-up: reduce the architecture.md section to a pointer at the concepts doc. Out of scope here, and a prior phase already reverted an architecture.md refresh for scope (325342c).src/environment/tmux.ts:103-npx tsc --noEmitreports 3 pre-existing TS2339 "does not exist on type 'never'" errors in src/environment/tmux.ts:103 and src/environment/runtime.test.ts:223,226. They are untouched by this change (which only edits src/artifacts/viewer.ts and viewer.test.ts, both type-clean), and a prior phase's fix plus CI typecheck step was deliberately reverted for scope (334121f, then 4c70904 "Revert unrelated typecheck expansion"). There is notypecheckscript or CI typecheck today, so nothing enforces this. Left unfixed: fixing requires editing unrelated functional and test files.✅ **Push** - passed
✅ No issues found.