Conversation
📝 WalkthroughWalkthroughThis PR introduces a context-driven state management architecture, replacing module-level store imports with a new EditorContext. A new Changes
Sequence DiagramsequenceDiagram
participant App as App Startup
participant Layout as +layout.svelte
participant EditorCtx as EditorContext
participant LocalStorage as localStorage
participant ProjectStore as ProjectStore
participant Component as Editor Components
App->>Layout: render
Layout->>EditorCtx: initializeEditorContext()
EditorCtx->>LocalStorage: loadFromLocalStorage()
alt Data exists in localStorage
LocalStorage-->>EditorCtx: Project
else No stored data
EditorCtx->>EditorCtx: getDefaultProject()
EditorCtx-->>EditorCtx: Project
end
EditorCtx->>ProjectStore: new ProjectStore(initialProject)
EditorCtx->>EditorCtx: setUpWatchers()
note over EditorCtx: Watch projectStore.state changes
EditorCtx-->>Layout: EditorContext ready
Layout->>Component: render with context
Component->>EditorCtx: getEditorState()
EditorCtx-->>Component: editorState (with projectStore)
Component->>Component: derive local state
User->>Component: modify project
Component->>ProjectStore: update state
ProjectStore->>EditorCtx: notify (via watcher)
EditorCtx->>EditorCtx: debounce 500ms
EditorCtx->>LocalStorage: saveToLocalStorage()
LocalStorage-->>LocalStorage: persist Project JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/stores/project.svelte.ts (1)
290-297: 🛠️ Refactor suggestion | 🟠 MajorMissing Zod validation on imported JSON.
importFromJSONparses external JSON and passes it directly toloadProjectwithout schema validation. As per coding guidelines, external data insrc/lib/**/*.tsshould be validated with Zod schemas before use.🛡️ Proposed fix
+import { ProjectSchema } from '$lib/schemas/animation'; + importFromJSON(json: string) { try { - const project = JSON.parse(json); - this.loadProject(project); + const parsed = JSON.parse(json); + const project = ProjectSchema.parse(parsed); + this.loadProject(project); } catch (error) { console.error('Failed to import project:', error); } }As per coding guidelines: "Validate all external data with Zod schemas before using in the application" (
src/lib/**/*.ts).
🤖 Fix all issues with AI agents
In `@src/lib/components/editor/project-settings-dialog.svelte`:
- Around line 68-75: handleSave is directly mutating projectStore.state (e.g.,
projectStore.state.name = ...) which breaks the immutable pattern used by
ProjectStore; add a new method on ProjectStore named updateProject(updates:
Partial<Project>) that reassigns the whole state (this.state = { ...this.state,
...updates }) and then replace the direct assignments in handleSave with a
single call projectStore.updateProject({ name: formData.name, width:
formData.width, height: formData.height, duration: formData.duration,
background: formData.background }) and close the dialog (open = false) as
before.
In `@src/lib/contexts/editor.svelte.ts`:
- Line 54: Remove the noisy console.log in the production save path: locate the
console.log('Project saved to localStorage') call in
src/lib/contexts/editor.svelte.ts (the debounced save handler) and either delete
it or wrap it behind a development-only guard (e.g., check NODE_ENV !==
'production' or use the app's debug/logging facility) so it does not run in
production while preserving dev visibility.
- Around line 32-47: loadFromLocalStorage currently JSON.parses untrusted data;
validate the parsed object against the ProjectSchema using Zod (e.g.
ProjectSchema.safeParse) inside loadFromLocalStorage and only return the parsed
value when validation succeeds; on validation failure (or parse error) log the
detailed validation errors and return null to avoid propagating malformed
Project objects to the rest of the app (refer to function name
loadFromLocalStorage and the ProjectSchema symbol when making the change).
In `@src/lib/layers/components/ImageLayer.svelte`:
- Around line 26-47: The Zod .default() callbacks in the ImageLayer schema call
getEditorState() (used inside calculateCoverDimensions with
ASPECT_RATIOS.IMAGE_DEFAULT) which can throw because getEditorState likely uses
Svelte getContext and isn’t safe when the schema defaults run outside component
init; fix by removing context calls from schema defaults—either change the layer
creation flow to pass explicit width/height into the Image layer constructor (so
the schema uses provided values instead of calling getEditorState), or maintain
a reliable module-level project state (e.g., projectStore) and read dimensions
from that inside the .default() instead of calling getEditorState(); update
where new image layers are created to supply dimensions or initialize the
module-level state before schema defaults run (affecting functions/places that
call createLayer/createImageLayer and the ImageLayer schema's default
callbacks).
In `@src/lib/layers/components/VideoLayer.svelte`:
- Around line 25-46: The Zod schema uses getEditorState() inside .default() for
width/height which can throw outside a Svelte component (getContext
unavailable); update the defaults to avoid calling getEditorState() at parse
time by: remove direct calls to getEditorState() from the Zod .default() for the
width and height fields in VideoLayer.svelte and instead accept explicit
dimensions when creating a video layer (or provide a safe fallback factory
function that checks for context before calling getEditorState()); use
calculateCoverDimensions and ASPECT_RATIOS.VIDEO_DEFAULT only from within
component initialization or from a safe helper (e.g., getSafeEditorDimensions())
so the schema’s defaults are context-free.
🧹 Nitpick comments (11)
src/lib/components/editor/panels/layers-panel.svelte (1)
2-8: Import order: external packages should come before internal lib imports.As per coding guidelines, imports should be ordered: External packages → SvelteKit → Internal lib → Relative imports.
@lucide/svelte(line 4) is an external package but appears after internal lib imports.Suggested reorder
<script lang="ts"> + import { Eye, EyeOff, Lock, Unlock, Trash2 } from '@lucide/svelte'; import { getEditorState } from '$lib/contexts/editor.svelte'; import { Button } from '$lib/components/ui/button'; - import { Eye, EyeOff, Lock, Unlock, Trash2 } from '@lucide/svelte'; import * as Popover from '$lib/components/ui/popover';src/lib/layers/properties/GenerateCaption.svelte (1)
2-8: Import order:@lucide/svelteshould precede$lib/*imports.Same import ordering issue as noted in other files — external packages should come first. As per coding guidelines: "External packages → SvelteKit → Internal lib → Relative imports."
src/lib/components/editor/keyframe-card.svelte (1)
2-7: Import order:@lucide/svelte(line 6) should precede$lib/*imports.src/lib/components/editor/panels/properties-panel.svelte (1)
2-20: Import order: external packages (@lucide/svelte,nanoid) should precede$lib/*imports.As per coding guidelines: "External packages → SvelteKit → Internal lib → Relative imports."
src/lib/layers/properties/SourceLayerRef.svelte (1)
7-8: Remove redundant$derivedwrapper aroundgetEditorState().
getEditorState()returns a stable context reference that never changes after initialization; wrapping it in$derivedprovides no reactivity benefit. The second line's$derived(editorState.project)is still useful if the project property can be reassigned, but the first wrapper is unnecessary.Suggested simplification
- const editorState = $derived(getEditorState()); + const editorState = getEditorState(); const projectStore = $derived(editorState.project);src/lib/components/editor/toolbar.svelte (1)
17-22: Imports are interleaved with declarations — consolidate import blocks.Lines 17–21 insert
getEditorStateimport and theeditorState/projectStoredeclarations between two import groups (lines 4–16 and lines 22–31). As per coding guidelines, imports should be organized together: External packages → SvelteKit → Internal lib → Relative imports, before any declarations.Suggested reordering
- import { getEditorState } from '$lib/contexts/editor.svelte'; import ExportDialog from './export-dialog.svelte'; - - const editorState = $derived(getEditorState()); - const projectStore = $derived(editorState.project); import * as DropdownMenu from '$lib/components/ui/dropdown-menu/index.js'; + import { getEditorState } from '$lib/contexts/editor.svelte'; ... + + const editorState = $derived(getEditorState()); + const projectStore = $derived(editorState.project);As per coding guidelines, "Organize imports in order: External packages → SvelteKit → Internal lib → Relative imports."
src/lib/components/editor/timeline/timeline.svelte (1)
9-10:$derivedwrapping ofgetEditorState()is likely redundant.
getEditorState()returns the same reactive$stateproxy from Svelte'screateContext— its reference doesn't change after initialization. Wrapping it in$derivedadds a reactive computation that always returns the same object. You could simplify to:const editorState = getEditorState(); const projectStore = editorState.project;This pattern is repeated across multiple files; if it's intentional for future flexibility (e.g., swappable contexts), that's fine, but it's worth noting the overhead is unnecessary today.
src/lib/stores/project.svelte.ts (1)
44-55: Default project definition is duplicated.The default project shape here (lines 46-54) is identical to
getDefaultProject()insrc/lib/contexts/editor.svelte.ts(lines 60-71). Consider reusing one definition to avoid drift.♻️ Suggested approach
+import { getDefaultProject } from '$lib/contexts/editor.svelte'; + constructor(initialProject?: Project) { - this.state = initialProject ?? { - id: nanoid(), - name: 'Untitled Project', - width: 720, - height: 1280, - duration: 5, - fps: 30, - background: '#000000', - layers: [] - }; + this.state = initialProject ?? getDefaultProject(); }Or export
getDefaultProjectfrom the store file and import it in the context — whichever direction avoids circular dependencies.src/lib/contexts/editor.svelte.ts (2)
126-155:$effect.rootreturn value (cleanup) is never captured — watcher and timeout may leak.
$effect.rootreturns a cleanup function that should be called when the context is torn down. Currently the return value is discarded, so thewatchsubscription and any pendingsaveTimeoutwill persist indefinitely. IfinitializeEditorContextis only ever called once in a long-lived root layout, this is likely harmless in practice, but it's still a resource management gap.Additionally,
resetToNew(line 115-123) doesn't clear a pendingsaveTimeout, which could cause a stale save to fire after the reset.♻️ Suggested fix
- $effect.root(() => { + const cleanup = $effect.root(() => { watch( () => $state.snapshot(projectStore.state), () => { if (!initialized) { initialized = true; return; } editorContext.hasUnsavedChanges = true; if (editorContext.dbProjectId === null) { if (saveTimeout) { clearTimeout(saveTimeout); } saveTimeout = setTimeout(() => { saveToLocalStorage(projectStore.state); }, DEBOUNCE_MS); } } ); + return () => { + if (saveTimeout) clearTimeout(saveTimeout); + }; });And expose or store
cleanupso it can be called if needed. ForresetToNew, clear the timeout:resetToNew: () => { + if (saveTimeout) clearTimeout(saveTimeout); editorContext.dbProjectId = null;
13-30:setDbContextsignature doesn't includeisMcp.The
EditorContexttype has anisMcpfield (line 19), butsetDbContext(lines 22-27) doesn't accept it as a parameter. This meansisMcpcan only be set through direct property mutation, which conflicts with the pattern of using dedicated methods. If MCP projects are a real scenario, consider addingisMcpto thesetDbContextsignature.src/lib/ai/ai-operations.svelte.ts (1)
118-122: Unnecessarily indirect state access viagetContext.
getContext(projectStore).project.layersjust resolves toprojectStore.state.layers. Creating a newMutationContextobject to read a property you already have a reference to adds noise.♻️ Suggested simplification
if (result.success) { if ( projectStore.selectedLayerId && - getContext(projectStore).project.layers.find((l) => l.id === projectStore.selectedLayerId) === - undefined + !projectStore.state.layers.find((l) => l.id === projectStore.selectedLayerId) ) { projectStore.selectedLayerId = null; } }
1 participant
Summary by CodeRabbit
New Features
UI Changes
Refactor