mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-05 17:30:50 +00:00
Vance Ingalls
539e027b60
Fixes two of the three adversarial findings from the second #2416 tip re-review; the third is a pre-existing runtime-protocol gap, explained in the PR thread rather than patched here. - The Grade rollback added in the previous commit could never fire through the real Studio callback: runDomEditCommit (the shared commit runner used by every data-attribute commit, not just Grade) catches persist failures internally and always resolves, reporting outcome only via its own onError side effect. A caller awaiting the promise never sees a rejection, so the revert-on-reject logic was dead code against the actual app. Added an optional onSettled(ok) callback to DomEditCommitRunnerConfig (purely additive — every existing caller that doesn't pass it is unaffected) and threaded it through commitDataAttribute -> handleDomAttributeLiveCommit -> the onSetAttributeLive prop type (now accepts an optional 3rd argument) -> useColorGradingController, which now drives the revert from the real signal. The promise-rejection path stays as a fallback for any other implementation of onSetAttributeLive that rejects instead. - Selection flushing performed a real side effect (writing the outgoing element's pending edit) during the render-phase identity-reset block. Adjusting STATE during render (comparing against a ref) is React's documented pattern, but it doesn't license actual I/O — React can invoke render more than once per commit, which could double-fire or misorder the write. The reset block now only enqueues the flush (a pure ref write); a new effect keyed on the identity performs it after commit. - Async persist completions (both the onSettled callback and its promise- rejection fallback) now capture the identity key the attempt was made for and check it against the CURRENT identity before touching confirmedGradingRef/grading/runtimeStatus. Without this, a persist that settles after selection has moved on to a THIRD element could clobber that element's freshly-reset state with a result that belongs to an element no longer selected. Not fixed here: the runtime Grade target (HfColorGradingTarget, used by core's resolveTarget to find the DOM element inside the preview iframe) has no source-file/composition-scope discriminator, matching the same gap selectionIdentityKey had before this stack — but fixing it means changing a wire-protocol type shared across core/player/studio and the legacy ColorGradingSection too. hfId (checked first, before id/selector) is minted uniquely per element at parse time in the common case, so this is a narrow residual risk for hfId-less same-selector elements across different source files, not a regression introduced by this stack. Flagged as a follow-up in the PR thread. New/updated regression tests: real onSettled(false) path (distinct from the promise-rejection fallback), and a stale in-flight persist settling after selection has moved on twice more. Full studio suite still at the known pre-existing 55-failure baseline, zero regressions.
58 lines
1.7 KiB
TypeScript
58 lines
1.7 KiB
TypeScript
interface DomEditCommitRunnerConfig {
|
|
capture: () => void;
|
|
apply: () => void;
|
|
persist: () => Promise<void>;
|
|
shouldRevert: (error: unknown) => boolean;
|
|
revert: () => void;
|
|
onError: (error: unknown) => void;
|
|
shouldResync: () => boolean;
|
|
resync: () => void | Promise<void>;
|
|
/**
|
|
* Reports success/failure without changing this function's own resolve-
|
|
* always contract — `persist` failures are handled here (revert + onError)
|
|
* and never rethrown, so callers awaiting `runDomEditCommit` can't observe
|
|
* failure via rejection. A caller that needs to react to a specific
|
|
* commit's outcome (e.g. reverting its OWN optimistic state) can pass this
|
|
* instead of relying on a rejection that will never come.
|
|
*/
|
|
onSettled?: (ok: boolean) => void;
|
|
}
|
|
|
|
interface CommitVersionRef {
|
|
current: number;
|
|
}
|
|
|
|
export function bumpDomEditCommitVersion(versionRef: CommitVersionRef): () => boolean {
|
|
const commitVersion = versionRef.current + 1;
|
|
versionRef.current = commitVersion;
|
|
return () => versionRef.current === commitVersion;
|
|
}
|
|
|
|
export function bumpDomEditCommitMapVersion<TKey>(
|
|
versionMap: Map<TKey, number>,
|
|
versionKey: TKey,
|
|
): () => boolean {
|
|
const commitVersion = (versionMap.get(versionKey) ?? 0) + 1;
|
|
versionMap.set(versionKey, commitVersion);
|
|
return () => versionMap.get(versionKey) === commitVersion;
|
|
}
|
|
|
|
export async function runDomEditCommit(config: DomEditCommitRunnerConfig): Promise<void> {
|
|
config.capture();
|
|
config.apply();
|
|
|
|
try {
|
|
await config.persist();
|
|
config.onSettled?.(true);
|
|
} catch (error) {
|
|
if (config.shouldRevert(error)) {
|
|
config.revert();
|
|
}
|
|
config.onError(error);
|
|
config.onSettled?.(false);
|
|
}
|
|
|
|
if (!config.shouldResync()) return;
|
|
await config.resync();
|
|
}
|