fix(core): per-property-group keyframe foundations#1354
Merged
Conversation
Add PropertyGroupName type system (position/scale/size/rotation/visual/other), PROPERTY_GROUPS constant, classifyPropertyGroup/classifyTweenPropertyGroup functions. Parser generates group-aware animation IDs, resolves position strings (+=, -=, <, >), uses numeric matching with 2% tolerance, and preserves IDs across all mutations.
Collaborator
Author
This was referenced Jun 11, 2026
Fallow audit reportFound 64 findings. Duplication (44)
Health (20)
Generated by fallow. |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verdict: approve-with-nits. Load-bearing base of the 7-PR restructure; all 3 R2 carry-overs from monolithic #1344 resolved.
R2 carry-overs
- Goldens refreshed ✅ —
moderate.parsed.jsonnow contains 4propertyGroupinstances + newresolvedStart/implicitPositionfields;complex.parsed.json/fromto.parsed.json/minimal.parsed.jsonupdated. CITest+Tests on windows-latestgreen. (Vai R1 Blocker 1 closed.) fromTo"inversion" intent now explicit ✅ —gsapParser.ts:1932-1972resolveConversionPropscarries a docstring statingresolvedFromValuesrepresents the new destination of a drag and is intentionally merged intotoPropsforfromTo()(L1964-1968: "This is intentional and not inverted."). The R2 concern was a misread of intent; author made it unmisreadable. (Vai R1 Blocker 3 closed.)- ID-suffix scope bounded by inline invariant ✅ —
gsapParser.ts:1031-1039: "No database, localStorage, or file stores animation IDs, so changing the ID format (e.g. adding a-scale/-positionsuffix) is safe." Group suffix emitted at L1047-1048. Did not independently verify the no-persistence claim in pacific or studio localStorage — if false, it'd be a separate-PR break, not this PR's regression. Worth a one-line cross-repo grep before flipping any cohort that depends on stable IDs.
Taxonomy + types
PROPERTY_GROUPStaxonomy atgsapConstants.ts:52-86(definesPropertyGroupName+PROPERTY_GROUPS+classifyPropertyGroup+classifyTweenPropertyGroup). Subpath export./gsap-constantsexists inpackages/core/package.json:77-80— downstream studio PRs can consume via@hyperframes/core/gsap-constants. ✅- New
GsapAnimationfields all optional —gsapSerialize.ts:34,36,39addsresolvedStart?,implicitPosition?,propertyGroup?. Backwards-compat for existing serialized state. playerStore.activeKeyframePctNOT in this PR — expected in #1356 (confirmed there).
Concerns
- Fallow audit 64 findings (only failing CI check) — 44 minor test-scaffolding clones in
gsapParser.test.ts+ 20 health/CRAP-score warnings.resolveConversionPropslands at CRAP 49.5 (major). Pre-existing criticals (unrollDynamicAnimations482,resolveNode315,setArcPathInScript315,parseMotionPathNode210) are getting worse in the foundations slot, not better. Worth a follow-up extraction PR before the rest of the stack lands more parser growth on top. Date.now()ID atgsapParser.ts:1328inaddAnimationToScript— author-time studio mutator path; ID rebuilds canonically on nextparseGsapScript. Not a determinism violation, but consider a monotonic counter for in-flight sessions where two adds within the same ms produce duplicate transient IDs.
Test coverage
gsapParser.test.ts +341/-10, ~145 it() blocks total, 51 references to new propertyGroup/classify/resolvedStart/implicitPosition machinery. New describe blocks: resolvedStart — timeline position resolution (L292), splitIntoPropertyGroups (L2163).
Net
Solid foundation. The Fallow trend is the only real concern — extraction follow-up before more parser growth lands.
— Review by Rames D Jusso
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R3 verification — approve-with-nit.
The new docstring at gsapParser.ts:1093-1096 ("This is intentional and not inverted") closes my R2 axis (the fromTo direction concern). Via's R2 ask — rename resolveConversionProps to resolvedLiveValues OR split into per-method helpers — still open. Not blocking; the inline docstring shields the next reader, but the rename would have been cheaper than the comment.
Review by Rames D Jusso
vanceingalls
approved these changes
Jun 12, 2026
vanceingalls
left a comment
vanceingalls
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — Via reviewed, stamp applied.
— Vai
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
3 participants
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.
Summary
PropertyGroupNametype system (position/scale/size/rotation/visual/other) andPROPERTY_GROUPSconstantclassifyPropertyGroupandclassifyTweenPropertyGroupclassification functions#box-to-0-position)+=,-=,<,>timeline position stringsTest plan
propertyGroupfieldStack: 1/7