Repository navigation
fix(core): make grading writes previewable and honor computed properties - #5157
Conversation
Edit accuracy: accurate 2059 (base branch 2059), smooth 1534 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
…sing an inline start
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
jrusso1020
left a comment
There was a problem hiding this comment.
Review at 88883305: approved on the code. GitHub reports this branch as conflicting with main (mergeable_state: dirty). A conflict resolution makes a new head, and this repo needs an approval on the last push, so I'll re-check the merge's own patch when it's re-pinned.
What I checked
- Hue curves.
serializeHfColorGradingnow writes only the authored curves throughauthoredHueCurves. Reading is unchanged, because a missing curve normalizes to identity, so the round-trip stays byte-identical (the existing round-trip test still passes). RGB curves are untouched. - The repair is narrow.
withoutEmptyHueCurvesdrops only an empty array under one of the three known keys. An unknown key, or a non-array, still reachesassertKnownGradingShape, and a mutation that drops unknown empties too is caught by the tests. - Dry-run.
--gradingwith--dry-runalone now previews, and a grade with neither flag still refuses. Dry-run never writes, and the JSON carriesattribute,valueand the color-gradinglintverdict computed from the prepared HTML. The example drops the old--apply --dry-runpairing. - Computed style.
readAnimatedValuereadsgetComputedStyle(...). Nothing in the runtime registers these properties with@property/registerProperty, so an unset property still reads as""→null, and the payload's value still wins when nothing is set. I grepped the registry, templates and skills: no shipped composition sets a--hf-color-grading-*property on a container, so the new inheritance changes no existing output. - Docs and skill.
cli.mdx,color-grading.mdxandgrading.mdnow say a set property overrides the payload.media-treatment-recipes.mdstill authors start values inline, but only for tweens (:215,:694,:754,:918), which matches the new rule. - Tests (local, this head): core
colorGrading.test.tsand runtimecolorGrading.test.ts93/93,media-treatment.test.ts21/21. CI is green apart from the conflict. - Mutations: 7 of 8 caught. Caught: serializing every curve again, the runtime reading inline style only, no repair, the repair dropping unknown keys, dry-run needing
--apply, dry-run writing, andvaluemissing from the JSON. Survived: replacing the reportedlintwith{ ok: true, findings: [] }. Every test grade lints clean, so the verdict is never seen failing. A dry-run test on a payload that lints with a color-grading error would pin it.
Nits
readAnimatedGradingcallsreadAnimatedValueup to nine times per entry per redraw, and each call is now a freshgetComputedStyle(element). That's one style recalc per frame at most, so it's cheap, but reading the declaration once per entry and passing it in would make that explicit.- Inherited means a property set on a parent now reaches every graded media under it, not just one. The docs say "on a parent", which is accurate. One more sentence to make the fan-out explicit would help authors who group several clips.
--dry-runwith no--gradingand no--clearstill says--apply requires --grading <json>, which names a flag the person didn't pass.
— Rames
# Conflicts: # skills-manifest.json
somanshreddy
left a comment
There was a problem hiding this comment.
Reviewed at 7281e78f, my first pass on this PR. No blockers; approving.
The merge is code-free. I compared the PR's content at 88883305 (from its merge base) with 7281e78f (from its main parent). The only difference is the regenerated skills-manifest.json hash; every code and docs hunk is identical.
Checked:
- Hue curves:
authoredHueCurvesserializes only non-empty curves, so a single hue curve no longer writes the other keys as[]. - Legacy files: on the CLI side,
withoutEmptyHueCurvesstrips empty known curves from a grading saved earlier before merging, while unknown keys and non-array values pass through toassertKnownGradingShape. - Computed style:
readAnimatedValuenow readsgetComputedStyle. The--hf-*properties aren't registered (CSS.registerProperty/@property), so an unset property still computes to""and returnsnull. Inheritance from a parent is the intended new behaviour, and the capability rule text now says so. - Dry run:
--dry-runwithout--applyvalidates, lints and reports, and never writes. - Lint dependency:
@hyperframes/lintis already statically imported elsewhere in the CLI (utils/lintProject.ts), so the new import adds no new dependency.
Lows / nits (none blocking):
- Lint
okuntested: forcingoktotrue(M7) passes all 21 CLI tests. Add a case with an error-severitycolor_grading_*finding. - Lint can block a write:
lintHyperframeHtmlruns beforewriteFileSync, so a lint exception now aborts an--applythat used to succeed. Consider wrapping it and reportinglint: nullon failure. - Error message:
--dry-runwith neither--gradingnor--clearsays "--apply requires --grading ", which is misleading for a dry run. - Per-frame cost: each
readAnimatedValuecall makes its owngetComputedStyle(element), about 9 per element per frame across the call sites. Style only recalculates once per frame, but hoisting one computed style per entry would be cheaper.
Tests and CI:
vitestpasses: corecolorGradingand runtimecolorGrading93/93, CLImedia-treatment21/21.- Mutants: 7 of 8 caught. Caught were inline-only reads, serializing every hue key, not stripping legacy empty curves, the strip filter, dry-run requiring
--apply, dry-run writing, and leaking non-grading lint findings. M7 (above) survived. - All 11 required checks are green at the head (86 checks succeeded, 2 skipped). It's mergeable, and
git merge-treeagainst current main is clean.
— Somu
What
Color grading now produces valid, previewable writes and honors CSS custom properties from computed style:
A single hue curve failed
lint.hyperframes media-treatment --grading '{"hueCurves":{"hueVsHue":[...3 points]}}' --applysucceeded, but the composition it wrote then failedhyperframes lintwithcolor_grading_invalid_structure(twice). Studio's grading panel writes through the same function.A grading property set anywhere but the element's own inline style was ignored without a warning. For example,
--hf-color-grading-blurset in a stylesheet or on a parent, with the image staying sharp while saturation and vignette from the same grade showed. On top of that, our docs, skill and CLI capability output told authors to put the animated property's start value inline. Once set, that value overrides the payload's value for that control, so an author who stored a blur and followed the advice pinned it at the start value until a tween moved it.Dry-run required a write flag.
media-treatment --selector X --grading <json> --dry-run --jsonrefused to preview unless--applywas also present. Dry-run now uses the same preparation as apply, reports the exact normalized attribute and color-grading lint verdict, and writes nothing.Dry-run
JSON output includes
attribute: "data-color-grading",value(the exact normalized string ornullfor removal), andlintwith grading findings from the proposed HTML. Both apply and dry-run run the real linter on that prepared HTML and report its color-grading findings. A grade without either--applyor--dry-runstill refuses to write.The command regression fails on main, then passes 21/21 three consecutive runs after the fix. It checks that preview leaves the original bytes unchanged and that apply writes exactly the previewed attribute value with the same lint verdict. The actual source CLI command also returned normalized grading and a successful grading lint verdict without changing the file. CLI typecheck and commit checks pass.
Hue curves
The writer and the validator disagreed about an untouched hue curve:
serializeHfColorGradingwrote the wholehueCurvesobject as soon as any one curve had values, so the untouched curves went out as"hueVsSaturation":[],"hueVsLuma":[].The serializer now writes only the hue curves that have points. Reading is unchanged: a missing curve normalizes to identity, so the written grading round-trips byte for byte. RGB curves are not affected, since their identity is a valid two-point line.
Files an earlier version already wrote stay fixable.
media-treatment --applyvalidated the stored grading strictly before merging a patch, so a file with empty hue curves refused every later apply, even an unrelated one. It now drops only the three known empty hue curves from the stored grading before merging (an empty curve is what the old writer meant by identity), so the next apply writes the file back valid. Everything else in a stored grading is still checked as before.Grading properties and the blur report
--hf-color-grading-blur,-intensity,-exposureand the rest) from the element's computed style instead of only its inline style. A value from a stylesheet or inherited from a parent now applies, as CSS custom properties normally do. Changes to parent styles, classes or stylesheets on a paused page apply on the next seek or play; this PR does not add paused style-change observation.This closes the report that blur did not render on an image inside a templated sub-composition while saturation and vignette did. That report does not reproduce on main or on 0.8.137. Blur renders the same in the root, in a
<template>sub-composition, in adata-composition-srcsub-composition and in a sub-composition mounted twice, in both snapshot and render. The two causes that do give that exact symptom on any media are the ones fixed here: a property set outside inline style, and an inline start value overriding a stored blur. A third behaviour also weakens blur: it is a crossfade toward a blurred copy, not a true radius, so at 0.45 about half the edge detail survives. It is left as is, because changing it would change every existing render.Proof
Real CLI on the reported hue-curve project,
media-treatment --selector "#v" --grading '{"hueCurves":{"hueVsHue":[[0,0],[120,20],[240,0]]}}' --apply, thenlint:hueCurveslint{"hueVsHue":[...],"hueVsSaturation":[],"hueVsLuma":[]}color_grading_invalid_structureerrors{"hueVsHue":[...]}--grading '{"adjust":{"exposure":0.1}}' --apply{"hueVsHue":[...]}hyperframes snapshotin Chromium of a checkerboard image whose grade sets only saturation, with--hf-color-grading-blur: 1in a stylesheet rule. Sharpness is the Laplacian variance of the image area; lower is blurrier:Checks
hueVsHueserializes that one curve, and the written attribute passeslint;--hf-color-grading-blurset in a stylesheet;colorGrading.test.ts(core and runtime) 93/93 andmedia-treatment.test.ts20/20, 3 runs in a row, including the existing byte-identical round-trip and inline-property tests. Core and CLI typecheck pass; the skills manifest is regenerated and in sync.The repair preserves unknown empty curve keys for the validator to reject. Its public-API regression fails against the broad empty-array filter and passes with the bounded repair. The changed CLI test and CLI typecheck passed on the final head.
Known gate red: Studio timeline viewport gate has the overscan regression introduced by #5109. The approved fix is #5151, owned separately; this PR leaves that work in its existing lane.