Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR restructures the transform data model from flat fields (x, y, z, rotationX/Y/Z, scaleX/Y) to a nested hierarchy (position, rotation, scale objects), updates the interpolation system to support optional interpolation and family-based strategies, and refactors UI components and AI mutations to align with the new structure. New utility modules provide property categorization and interpolation family management. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/ai/mutations.ts (1)
303-310:⚠️ Potential issue | 🟠 MajorMerge transform updates instead of replacing to preserve existing anchor.
When
input.updates.transformis provided (line 304), the TransformSchema applies defaults during parsing, includinganchor: 'center'if not explicitly provided. This replaces the entirelayer.transformobject, silently losing any previously-set custom anchor—even if the AI only intended to update position or rotation. The separate anchor check (line 308) only helps if the caller explicitly provides anchor alongside the transform.Proposed fix
if (input.updates.transform) { - layer.transform = input.updates.transform; + layer.transform = { ...layer.transform, ...input.updates.transform }; }
🤖 Fix all issues with AI agents
In `@src/lib/ai/system-prompt.ts`:
- Line 126: The system prompt says rotations are in degrees but the canvas state
string uses "rad" for layer.transform.rotation.z; update the canvas-state
template in src/lib/ai/system-prompt.ts so the rotation is presented in degrees
(convert layer.transform.rotation.z from radians to degrees using * 180 /
Math.PI) and change the unit label from "rad" to "deg" (or "degrees") to make
units consistent with the prompt.
In `@src/lib/components/editor/keyframe-card.svelte`:
- Around line 20-24: Remove the unused import getDefaultInterpolationForProperty
from the import list in keyframe-card.svelte; update the import statement that
currently imports getStrategyOptionsForFamilies,
getSupportedInterpolationFamilies, and getDefaultInterpolationForProperty so it
only imports getStrategyOptionsForFamilies and
getSupportedInterpolationFamilies, and run a quick build/lint to confirm no
references to getDefaultInterpolationForProperty remain.
- Around line 90-97: The getPropertyIcon function currently checks
isTransformProperty first which catches rotation.* and scale.* cases and returns
Move; change the order so specific checks for property.startsWith('rotation.')
and property.startsWith('scale.') come before the isTransformProperty(property)
check, keeping the remaining checks for isStyleProperty(property), property ===
'color' (Palette) and defaulting to Move; update the function that references
getPropertyIcon, RotateCw, Scale, isTransformProperty, isStyleProperty, Palette,
and Move accordingly.
In `@src/lib/components/editor/panels/input-propery.svelte`:
- Line 69: The file name is misspelled: rename the component file
input-propery.svelte to input-property.svelte and update all imports/usages to
the new name (look for imports or references to input-propery.svelte and the
component where it's used in editor/panels); ensure any export/default component
name or path strings inside code are consistent with the new filename so nothing
breaks at compile-time.
In `@src/lib/components/editor/panels/properties-panel.svelte`:
- Line 257: Remove the debug console.log call in the properties-panel update
flow: delete the console.log('updateProperty', propertyName, value, target)
statement inside the updateProperty handler (or wherever updateProperty is
defined/used) in the properties-panel.svelte component so the function no longer
emits debug output to the console before returning or dispatching updates.
- Around line 117-131: currentValues.transform currently mixes flat position
fields with nested rotation/scale; change it to the canonical nested shape so
position is under position: { x,y,z } (i.e.
currentValues.transform.position.x/y/z) while keeping rotation and scale nested,
and ensure the object you return uses animatedTransform.position.{x,y,z} ??
selectedLayer.transform.position.{x,y,z} for position and
animatedTransform.rotation/scale for their nested fields; then update all
callers and spreads that assume the canonical Transform (references to
currentValues.transform, animatedTransform, and selectedLayer.transform) to use
.position.* instead of .x/.y/.z so merges/spreads into selectedLayer.transform
remain type-consistent.
In `@src/lib/engine/interpolation.ts`:
- Around line 19-24: The current code in the function handling optional
interpolation (parameter interpolation?: Interpolation) warns on every frame
when interpolation is missing, which is noisy; instead silently default missing
interpolation to a sensible value (e.g. { family: 'continuous', strategy:
'ease-in-out' }) and proceed, or at minimum log once per keyframe creation—so
replace the console.warn and early return with code that sets interpolation =
interpolation ?? { family: 'continuous', strategy: 'ease-in-out' } (or use a
one-time logger keyed by the KeyframeSchema id) and continue using interpolation
to compute the result.
In `@src/lib/layers/LayerWrapper.svelte`:
- Around line 194-200: The code currently builds newTransform using a shallow
spread that adds top-level x/y keys (const newTransform = { ...layer.transform,
x: ..., y: ... }) which leaves layer.transform.position unchanged; change the
update to set the nested position object instead — construct newTransform by
copying layer.transform but replacing position with a new object that sets
position.x and position.y (using hasXKeyframes/hasYKeyframes to decide whether
to use existing position.x/position.y or add movementX/movementY), then call
projectStore.updateLayer(id, { transform: newTransform }) so the updated values
live at transform.position.x and transform.position.y.
In `@src/lib/schemas/base.ts`:
- Around line 93-101: The validator currently rejects inputs when both
hasNewFormat and hasOldFormat are false, which causes valid anchor-only objects
like { anchor: 'top-left' } to be refused; update the check in the Transform
validation to treat an explicit anchor as acceptable by including a condition
for the presence of the anchor field (e.g., check for input.anchor or similar)
before calling ctx.addIssue, so that only inputs lacking new format, old format,
and anchor are rejected; adjust the conditional using the existing hasNewFormat,
hasOldFormat, and the anchor presence check and keep the ctx.addIssue call for
the true-invalid case.
- Around line 103-109: The superRefine block in src/lib/schemas/base.ts
currently calls ctx.addIssue when both hasNewFormat and hasOldFormat are true,
which creates a failing Zod validation; change this to a non-failing behavior:
either remove the ctx.addIssue call entirely or replace it with a non-blocking
notification (e.g., console.warn) and ensure the schema's transform logic (in
the same schema's .transform or the function handling transform) prefers the new
nested fields and silently drops/ignores the old flat fields; reference the
superRefine callback and ctx.addIssue so you update that exact spot and make
sure downstream code (the transform) reconciles/normalizes inputs accordingly.
In `@src/lib/stores/project.svelte.ts`:
- Around line 326-329: In removeLayerFromGroup, the child layer's scale is
incorrectly being added to the group transform scale; change the combination
from addition to multiplication so scale becomes l.transform.scale.x *
gt.scale.x and l.transform.scale.y * gt.scale.y (match the logic used in
ungroupLayers); update the scale computation where l.transform.scale and
gt.scale are combined to use multiplication instead of +.
🧹 Nitpick comments (9)
src/lib/components/ui/select/select.svelte (1)
39-39: Consider??instead of||for the fallback.
||treats empty string""as falsy, so a value of""would incorrectly show the placeholder. If that's not intended, prefer nullish coalescing:Suggested fix
- {value || placeholder} + {value ?? placeholder}src/lib/schemas/animation.ts (1)
128-128: Makinginterpolationoptional has a runtime consequence worth noting.With
interpolationnow optional onKeyframeSchema, the engine'sinterpolateValuefalls back to returningendValueand emittingconsole.warnon every frame when interpolation is missing. This could be noisy during playback for any keyframe that was created without an explicit interpolation. Consider whethergetDefaultInterpolationForProperty(frominterpolation-utils.ts) should be used at keyframe creation time to always populate a default, or alternatively suppress the warning for expected cases.src/lib/utils/property-names.ts (2)
48-60:isTransformPropertyduplicates logic already ingetPropertyCategory.Consider implementing
isTransformPropertyin terms ofgetPropertyCategoryto avoid maintaining the same prefix checks in two places:♻️ DRY suggestion
export function isTransformProperty(property: AnimatableProperty): boolean { - return ( - property.startsWith('position.') || - property.startsWith('rotation.') || - property.startsWith('scale.') - ); + return getPropertyCategory(property) === 'transform'; }Also applies to: 65-71
76-78:isStylePropertyis narrower than thegetPropertyCategory'style' catch-all.
getPropertyCategoryreturns'style'for any property that isn't transform or props (catch-all at line 59), butisStylePropertyonly returnstruefor'opacity'and'color'. If a new style property is introduced,getPropertyCategorywould classify it as style, butisStylePropertywould miss it. Consider deriving from the category helper for consistency, or documenting thatisStylePropertyis intentionally restrictive.src/lib/utils/interpolation-utils.ts (1)
43-44: Property name extraction can collide for built-in vs. props properties.
property.split('.').pop()!extracts the last segment, so both"position.x"and"props.x"would yield"x". If a layer schema happens to define a prop namedx, the built-in property would incorrectly pick up that prop's interpolation metadata instead of defaulting to'continuous'.Consider checking the prefix before extracting:
♻️ Proposed fix
- // Extract property name from animatable property (e.g., "props.fontSize" -> "fontSize") - const propertyName = property.includes('.') ? property.split('.').pop()! : property; + // Only extract from layer-specific props; built-in properties use defaults + if (!property.startsWith('props.')) { + // Built-in properties (position.x, scale.y, opacity, etc.) always use continuous + return ['continuous']; + } + + const propertyName = property.slice('props.'.length);src/routes/(app)/p/[id]/+page.svelte (1)
28-38: Manual field enumeration is redundant —projectDataalready has the correct shape.After Zod parsing,
projectDataconforms to the schema. You can spread it directly:♻️ Simplification
editorState.project.loadProject({ id: data.project.id, - name: projectData.name, - width: projectData.width, - height: projectData.height, - duration: projectData.duration, - fps: projectData.fps, - background: projectData.background, - layers: projectData.layers, - fontFamily: projectData.fontFamily + ...projectData });src/lib/components/editor/panels/add-layer.svelte (1)
18-29: Redundant transform override — all values matchcreateLayerdefaults.The transform passed here is identical to what
createLayeruses internally as defaults. You could simplify this to justcreateLayer(type, { projectDimensions: { ... } }). Not blocking, just a simplification opportunity.src/lib/components/editor/timeline/timeline-keyframe.svelte (1)
20-21: Hardcoded fallback type'rectangle'may mask bugs.If the layer isn't found (e.g., deleted while the popover is open), silently falling back to
'rectangle'could produce confusing UI behavior. Consider whether you'd rather guard against rendering the keyframe card at all when the layer is missing.src/lib/ai/schemas.ts (1)
57-63: Remove unusedPositionSchema.
PositionSchema(lines 57–63) is not referenced anywhere in the codebase. It was replaced byTransformSchemainCreateLayerInputSchemaand should be deleted.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/ai/system-prompt.ts`:
- Line 33: The system prompt's property examples use flat names (rotationX,
rotationY, etc.) that no longer match the nested animatable paths; update the
prompt text in src/lib/ai/system-prompt.ts (the prompt string that contains the
"Rotation", "Position" and "Scale" descriptions) to use dot-notation paths:
rotation.x, rotation.y, rotation.z and likewise position.x/position.y/position.z
and scale.x/scale.y so the AI generates mutations against the actual property
names.
🧹 Nitpick comments (4)
src/lib/components/editor/keyframe-card.svelte (3)
216-230: Hardcoded 500ms delay is fragile for scroll-then-focus.The
setTimeout(500)assumes smooth scrolling completes within 500ms, which varies by browser and scroll distance. Consider listening for thescrollendevent on the scrollable container instead, or at minimum extracting the magic number into a named constant.Sketch using scrollend
- if (input) { - input.scrollIntoView({ behavior: 'smooth', block: 'center' }); - setTimeout(() => { - (input as HTMLInputElement)?.focus?.(); - (input as HTMLInputElement)?.select?.(); - }, 500); - } + if (input) { + const scrollContainer = input.closest('[data-scroll-container]') ?? document.documentElement; + const onScrollEnd = () => { + scrollContainer.removeEventListener('scrollend', onScrollEnd); + (input as HTMLInputElement)?.focus?.(); + (input as HTMLInputElement)?.select?.(); + }; + scrollContainer.addEventListener('scrollend', onScrollEnd, { once: true }); + input.scrollIntoView({ behavior: 'smooth', block: 'center' }); + }
121-141: Consider adding exhaustiveness check for the family switch.If
InterpolationFamilyis extended with a new variant, this switch will silently leavenewInterpolationuninitialized. Adefault: neverguard makes this a compile-time error:default: { const _exhaustive: never = family; throw new Error(`Unknown family: ${_exhaustive}`); }Same applies to the switch in
handleStrategyChange(line 147).
38-56: Wheninterpolationis undefined, the strategySelectshows a placeholder but may confuse users.
activeFamilydefaults tosupportedFamilies[0]when there's no interpolation (line 47), but the strategySelectat line 353 will show "Select Strategy" placeholder sinceinterpolation?.strategyisundefined. This creates a state where a family appears selected but no strategy is visually chosen.Consider defaulting the displayed strategy to the first option in
currentFamilyOptionsfor visual consistency, or making the placeholder text clearer (e.g., "Default" or "Auto").src/lib/components/editor/panels/properties-panel.svelte (1)
229-246:setDeepassumes max two levels of nesting — document or guard this.The helper works correctly for the current use case (
position.x,scale.y, etc.) but will silently produce incorrect results if called with a single key (no dot) since it would still work, or with 3+ levels since it only shallow-copies each level. This is fine for now but consider adding a brief JSDoc note about the expected depth.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes