Repository navigation
feat(producer): a render request can force screenshot capture for that render - #7066
Merged
Merged
Conversation
…t render A retried render can ask for the slower, safer capture mode without changing the pod's environment.
Contributor
Edit accuracy: accurate 267 (base branch 267), smooth 112 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (1)
|
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes at e30d690b for one reason: the Comments check fails on this head.
RAISED comment share: packages/producer/src/server.ts is 10.6% (113/1065), at the base 10.6% (112/1060)
1 comment line(s) added.
CONTRIBUTING.md says "A package file's comment share may not rise" (scripts/comment-ratchet.mjs), and the PR body ticks "Comments follow CONTRIBUTING.md". The fix is to drop the /** Capture with screenshots for this render only… */ line on RenderInput.forceScreenshot. The name and the ?: true type already say it, and the explanation is in the PR description. Run node scripts/comment-ratchet.mjs before pushing.
Everything else checks out, so I'll approve once the ratchet is green:
- New test fails without the change. I restored
server.tsfrom main (merge base3baf81f9a): the forced case fails ("expected … to match object { producerConfig: … }"), 1 failed and 3 passed. At the head all 4 pass.tsc --noEmitonpackages/produceris clean. - The path is correct.
forceScreenshot: truebecomesengineOverrides→resolveConfig(overrides), so an explicittruewins overPRODUCER_FORCE_SCREENSHOT=false. Both handlers that go throughbuildRenderJobConfigget it. Absent,falseand"true"leave the env value in charge.
Contributor
There was a problem hiding this comment.
Approving at a68f7161.
- Only the comment changed. Compared with
e30d690b, the one difference is the removed/** Capture with screenshots… */line inpackages/producer/src/server.ts. The merge base is still3baf81f9a, and the code and tests are unchanged. - Comments now passes on this head, so the ratchet finding is resolved.
- My earlier checks still apply to the unchanged code. The new test fails on main (1 of 4) and passes at the head (4 of 4).
tsc --noEmitis clean. An explicittruereachesresolveConfigthroughengineOverridesand wins overPRODUCER_FORCE_SCREENSHOT=false.
CI, regression, Windows render verification and CodeQL were still running when I approved.
5 of 6 tasks
Merged
1 task done
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.
What
The producer server accepts
forceScreenshot: truein a render request body. That one render captures with screenshots, whateverPRODUCER_FORCE_SCREENSHOTsays. A missing field,falseor any non-boolean keeps today's behaviour.Why
A deployment can run the faster BeginFrame capture by default and still need a safe way out. A retried render can now ask for the slower screenshot capture for that attempt only, without changing the pod's environment or affecting other renders.
How
parseRenderOptionsreadsbody.forceScreenshot === true.buildRenderJobConfigpasses it tocreateRenderRequestas an engine override, so the job'sproducerConfig.forceScreenshotistrue. The engine already honours an explicittrueoverPRODUCER_FORCE_SCREENSHOT=false.Test plan
server.forceScreenshot.test.ts: withPRODUCER_FORCE_SCREENSHOT=false, a body withforceScreenshot: truegivesproducerConfig.forceScreenshot === true; an absent field,falseand"true"keepfalse. The forced case fails on main (expected false to be true) and passes with the change, 3 runs in a row, together with the neighbouring server test (9 of 9). Breaking the override line turns it red again.Size
About 85 changed lines: one parse line, one override line, one type field, plus the test.