Repository navigation
feat(studio): preview copy progress, retry and a remembered quality - #6366
Conversation
0ff271c to
f414d18
Compare
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 2 blockers · 0 important · 0 nits
Both blockers are inline: the studio build breaks, and a retry of a file that keeps failing re-transcodes forever. The Comments ratchet is also red: core/src/previewProxyBox.ts goes from 14.9% to 16.5% (+2 lines, the new param/header docs).
Scope
Reviewed the stack delta 6fb77f5e..f414d182: previewCopies reporting and throttling, proxyWait progress, the retry path from client to server, the studio slice, forwarding, preferences and the message router. Failure evidence for the build comes from the Player Safari perf run on this head (Run bun run build). preview-regression, regression and Player perf were still running when I posted this.
| try { | ||
| // A cached copy settles before any timer; a transcode never holds one of | ||
| // the browser's few connections to this host. 202 until the copy lands. | ||
| if (c.req.query(PREVIEW_PROXY_RETRY_PARAM) !== undefined) { |
There was a problem hiding this comment.
[blocker] Retrying a copy that keeps failing re-transcodes forever and never reports failed. retryNext.delete(el) adds hf-proxy-retry=1 to href once, but waitForServedProxy polls that same href on every ask. Each poll lands here and clears failedTranscodes, so resolveProxy starts a fresh ffmpeg job and the reply is 202 again. A non-OK reply never comes back, so failed never reaches MAX_FAILED_ASKS.
How to trigger it: a corrupt source fails, then the user clicks retry. The copy stays at "making" and ffmpeg reruns every Retry-After seconds. The URL that finally loads also keeps the flag on every range request.
Fix: send the retry only on the first ask. One option is to give waitForServedProxy a first-ask-only href, or strip the param after one fetch. A server-side guard that clears only when no job is in flight does not help, because the failed job has already left inFlight. A test where the transcode fails twice and the stub expects one restart would pin this.
| @@ -1,3 +1,4 @@ | |||
| import { isPreviewQuality, type PreviewQuality } from "@hyperframes/core"; | |||
There was a problem hiding this comment.
[blocker] The studio build fails. These are the studio's first value imports from the @hyperframes/core root; the existing root imports are all import type. The same goes for PREVIEW_QUALITY_LEVELS in studio/src/index.ts:37. The root index pulls safePath, hyperframesRuntime.engine and parsers' assetPaths into the browser bundle. Player Safari perf at this head fails in bun run build with ../parsers/src/assetPaths.ts (8:31): "resolve" is not exported by "__vite-browser-external".
Fix: export previewProxyBox through a browser-safe subpath, for example @hyperframes/core/preview-proxy-box next to ./studio-preview-mark, and import from that in both places.
There was a problem hiding this comment.
✅ Fixed in d8f546b (@hyperframes/core/preview-quality).
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 0 resolved, 2 open, 0 new · f414d182 → 3453b93d
Neither earlier blocker is fixed at this head. studioUiPreferences.ts:1 and studio/src/index.ts:37 still import values from the @hyperframes/core root. routes/preview.ts:759 still honours hf-proxy-retry on every poll, while proxyWait keeps re-fetching the same href. The new playbackHealth and preview-facts code adds no new finding.
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 resolved, 1 open, 0 new · 3453b93d → d8f546b1
The build fix holds: the new ./preview-quality entry is pinned by lightEntries.test.ts. The retry loop is still open, because routes/preview.ts:759 honours hf-proxy-retry on every poll of the same href. The Comments ratchet is still red on previewProxyBox.ts (14.9% → 15.2%, +1 line).
2655700 to
2d5faf8
Compare
d8f546b to
5318ef2
Compare
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 0 resolved, 1 open, 0 new · d8f546b1 → a027e9c5
The retry loop is still open. routes/preview.ts:759 still honours hf-proxy-retry on every poll of the same href, and proxyWait.ts is unchanged. This head sits on #6365 2d5faf88, which is not fixed yet: it still fails typecheck there (CopyJob.level: number, mediaProxy.ts:391). The CI workflow does not run on this stacked PR, so nothing here catches that.
a027e9c to
723e5c4
Compare
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 resolved, 0 open, 1 new · a027e9c5 → 1eae0d17
The retry loop is fixed: proxyWait sends the flag on the first ask only, and the new test pins it. Typecheck passes locally for core, studio and studio-server. [blocker] The Comments check fails at this head against b3dc2150:
previewProxyBox.ts+1 comment lineproxyTranscoder.ts+2 comment linesuseTimelinePlayer.ts+1, from the newfallow-ignore-next-line complexitydirective
The other workflows pass.
Edit accuracy: accurate 271 (base branch 271), smooth 134 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
1eae0d1 to
60471b0
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 resolved, 0 open, 1 new · 1eae0d17 → b5d0681d
The rebase onto main (after #6365) carries the reviewed commits through unchanged. The comment cuts, the playerStore line cap and the producer unpacked budget (raised to 3015439 with the reason in the body) are fine. CI, regression and Windows pass on this head. I re-ran a regression shard that failed on a docker layer fetch, and the first CI run's edit-accuracy miss passed on the second run (271/271).
[blocker] The required Studio and player captures check fails. The body has no ## Before / ## After sections, and this PR changes packages/studio. Add the captures, or use ## No visible change if the gate's size rule allows it, then re-run the check.
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 resolved, 0 open, 1 new · b5d0681d → 7897e457
The captures are in the body, and the check passes. The health-window fixes are sound: noteDisplayScale restarts only on a changed scale, and a scene mount no longer clears the window. CI, regression, captures and both perf runs pass on this head. One of the two Windows runs passed. The other failed in gsapValueAtPlayhead.browser.test.ts, which this PR does not touch, so it looks flaky.
[blocker] The PR no longer merges: mergeStateStatus is DIRTY. main raised the producer unpacked budget to 2897429 in scripts/package-size-budgets.json, which conflicts with this PR's 3015439. Rebase onto main, then re-measure the producer size on top of main before choosing the budget.
…ve in the player store
… is kept per computer
7897e45 to
16f11bb
Compare
There was a problem hiding this comment.
Verdict: APPROVE · 1 resolved, 0 open, 0 new · 7897e457 → 16f11bb7
The rebase onto main carries every reviewed commit through unchanged and drops only the budget commit. scripts/package-size-budgets.json is no longer touched, and the PR merges cleanly. Every workflow passes on this head: CI including packed sizes, regression, Windows, captures, both perf runs, preview-regression, Comments and CodeQL. The CodeQL note on previewMessageRouter.ts:142 is a false positive: the lookup only reaches three fixed handlers, and any other message type matches none of them, so no unexpected method runs.
There was a problem hiding this comment.
Verdict: APPROVE · 0 resolved, 0 open, 0 new · 16f11bb7 → e9d22a65
The one change is applyStoreReport, which replaces the Map lookup with a switch over the three report types. Behaviour is unchanged: any other type returns false and falls through as before. Every workflow passes on this head, including CodeQL.
What
choppyis true at 25% or more. Only play is measured (not paused or scrubbing), the first second after play starts or after a seek is ignored, and the window restarts on a level change, a copy swap, a scene mount, a window or zoom change, a rate change, a hidden page, and a project reload. Renders never measure. All numbers sit in one constant block (PLAYBACK_HEALTH).Why
A wait of tens of seconds on a big file looked like a hang. People need to see progress, retry a failure, and keep their choice between sessions, and the host needs to know when a level is too heavy for the machine.
Tests
Server tests for progress and retry; runtime tests for reporting, retry, the health window and the video grouping; store, router, forwarding hook and preference tests in studio. Health tests fail when the threshold or the warm-up is removed.
Before
The Studio player store on main, driven by the real preview: playing, and nothing else to read. The panel is a dev-only readout of the store (the dev handle), added to the page by the capture script.
After
The same project and playback with this PR, quality set to 1/2: the store holds the chosen quality and who chose it, a playback health reading from 3 s of play, and the preview facts. This is a store and runtime change; the controls that use it ship in the host app.