Centralize editor defaults#605
Merged
siddharthvaddem merged 5 commits intoMay 23, 2026
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughthis pr centralizes scattered editor defaults into ChangesEditor Defaults Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
EtienneLescot
changed the title
EtienneLescot
force-pushed
the
codex/editor-defaults-ssot
branch
from
165bbba to
b36a32d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/components/video-editor/editorDefaults.ts (1)
32-55: ⚡ Quick winMake the shared default objects readonly too.
lowkey risky with SSOT constants:
DEFAULT_EDITOR_APPEARANCE_SETTINGS,DEFAULT_EDITOR_LAYOUT_SETTINGS,DEFAULT_EXPORT_SETTINGS, andDEFAULT_GIF_SETTINGSare still mutable object literals. one accidental write in a consumer can mutate global defaults.nit: cleaner + safer typing pattern
-export const DEFAULT_EDITOR_APPEARANCE_SETTINGS: { - shadowIntensity: number; - showBlur: boolean; - motionBlurAmount: number; - borderRadius: number; -} = { +export const DEFAULT_EDITOR_APPEARANCE_SETTINGS = { shadowIntensity: 0, showBlur: false, motionBlurAmount: 0, borderRadius: 0, -}; +} as const satisfies { + shadowIntensity: number; + showBlur: boolean; + motionBlurAmount: number; + borderRadius: number; +}; -export const DEFAULT_EDITOR_LAYOUT_SETTINGS: { - padding: number; - aspectRatio: AspectRatio; - cropRegion: typeof DEFAULT_CROP_REGION; - wallpaper: string; -} = { +export const DEFAULT_EDITOR_LAYOUT_SETTINGS = { padding: 50, aspectRatio: "16:9", cropRegion: DEFAULT_CROP_REGION, wallpaper: DEFAULT_WALLPAPER, -}; +} as const satisfies { + padding: number; + aspectRatio: AspectRatio; + cropRegion: typeof DEFAULT_CROP_REGION; + wallpaper: string; +}; -export const DEFAULT_EXPORT_SETTINGS: { - quality: ExportQuality; - format: ExportFormat; -} = { +export const DEFAULT_EXPORT_SETTINGS = { quality: "good", format: "mp4", -}; +} as const satisfies { + quality: ExportQuality; + format: ExportFormat; +}; -export const DEFAULT_GIF_SETTINGS: { - frameRate: GifFrameRate; - loop: boolean; - sizePreset: GifSizePreset; - outputDimensions: typeof DEFAULT_GIF_OUTPUT_DIMENSIONS; -} = { +export const DEFAULT_GIF_SETTINGS = { frameRate: 15, loop: true, sizePreset: "medium", outputDimensions: DEFAULT_GIF_OUTPUT_DIMENSIONS, -}; +} as const satisfies { + frameRate: GifFrameRate; + loop: boolean; + sizePreset: GifSizePreset; + outputDimensions: typeof DEFAULT_GIF_OUTPUT_DIMENSIONS; +};Also applies to: 77-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/editorDefaults.ts` around lines 32 - 55, The default settings objects (DEFAULT_EDITOR_APPEARANCE_SETTINGS, DEFAULT_EDITOR_LAYOUT_SETTINGS, DEFAULT_EXPORT_SETTINGS, DEFAULT_GIF_SETTINGS) are mutable and should be made readonly so consumers can't accidentally mutate global defaults; update their declarations to use an immutable type (e.g., Readonly<...> or the equivalent readonly property types) and produce immutable runtime values (use Object.freeze(...) or the TypeScript "as const" pattern) and ensure related defaults referenced inside them (like DEFAULT_CROP_REGION, DEFAULT_WALLPAPER) are also readonly/frozen so the entire object graph is immutable.src/components/video-editor/projectPersistence.ts (1)
487-490: ⚡ Quick winNormalize export enums independently from current default values.
kinda subtle but lowkey risky: these checks currently preserve only a subset and rely on defaults to recover the rest. explicit full whitelists keep behavior stable even if defaults change later.
safer normalization (future-proof)
exportQuality: - editor.exportQuality === "medium" || editor.exportQuality === "source" + editor.exportQuality === "medium" || + editor.exportQuality === "good" || + editor.exportQuality === "source" ? editor.exportQuality : DEFAULT_EXPORT_SETTINGS.quality, - exportFormat: editor.exportFormat === "gif" ? "gif" : DEFAULT_EXPORT_SETTINGS.format, + exportFormat: + editor.exportFormat === "gif" || editor.exportFormat === "mp4" + ? editor.exportFormat + : DEFAULT_EXPORT_SETTINGS.format,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/projectPersistence.ts` around lines 487 - 490, The current ternary normalization for exportQuality and exportFormat relies on DEFAULT_EXPORT_SETTINGS to recover invalid values; instead implement explicit whitelists so valid enums are normalized independently of defaults. Replace the exportQuality ternary that checks only "medium" and "source" with a whitelist check against an allowedQualities array (e.g., allowedQualities = ['low','medium','high','source']) and fall back to DEFAULT_EXPORT_SETTINGS.quality only if the value is not in the whitelist; do the same for exportFormat using an allowedFormats whitelist (e.g., ['mp4','gif','webm']) and reference editor.exportQuality, editor.exportFormat and DEFAULT_EXPORT_SETTINGS in the implementation to ensure future default changes won’t alter allowed values.src/hooks/useEditorHistory.ts (1)
18-18: ⚡ Quick winRoute
cropRegionthrough the centralized layout defaults too.right now this is the one remaining direct default in this initializer, so drift can sneak in later if layout defaults evolve.
small consistency fix
-import { DEFAULT_CROP_REGION } from "`@/components/video-editor/types`"; @@ - cropRegion: DEFAULT_CROP_REGION, + cropRegion: DEFAULT_EDITOR_LAYOUT_SETTINGS.cropRegion,Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useEditorHistory.ts` at line 18, The hook useEditorHistory currently imports and uses DEFAULT_CROP_REGION directly; change it to obtain cropRegion from the centralized layout defaults instead so defaults don't drift. In the initializer inside useEditorHistory replace the direct use/import of DEFAULT_CROP_REGION with the value from the shared layout defaults API (e.g., call or import that provides the default layout object and read its cropRegion) so the hook uses the centralized default consistently (update any references to DEFAULT_CROP_REGION in useEditorHistory to use the layoutDefaults.cropRegion).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/video-editor/editorDefaults.ts`:
- Around line 32-55: The default settings objects
(DEFAULT_EDITOR_APPEARANCE_SETTINGS, DEFAULT_EDITOR_LAYOUT_SETTINGS,
DEFAULT_EXPORT_SETTINGS, DEFAULT_GIF_SETTINGS) are mutable and should be made
readonly so consumers can't accidentally mutate global defaults; update their
declarations to use an immutable type (e.g., Readonly<...> or the equivalent
readonly property types) and produce immutable runtime values (use
Object.freeze(...) or the TypeScript "as const" pattern) and ensure related
defaults referenced inside them (like DEFAULT_CROP_REGION, DEFAULT_WALLPAPER)
are also readonly/frozen so the entire object graph is immutable.
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 487-490: The current ternary normalization for exportQuality and
exportFormat relies on DEFAULT_EXPORT_SETTINGS to recover invalid values;
instead implement explicit whitelists so valid enums are normalized
independently of defaults. Replace the exportQuality ternary that checks only
"medium" and "source" with a whitelist check against an allowedQualities array
(e.g., allowedQualities = ['low','medium','high','source']) and fall back to
DEFAULT_EXPORT_SETTINGS.quality only if the value is not in the whitelist; do
the same for exportFormat using an allowedFormats whitelist (e.g.,
['mp4','gif','webm']) and reference editor.exportQuality, editor.exportFormat
and DEFAULT_EXPORT_SETTINGS in the implementation to ensure future default
changes won’t alter allowed values.
In `@src/hooks/useEditorHistory.ts`:
- Line 18: The hook useEditorHistory currently imports and uses
DEFAULT_CROP_REGION directly; change it to obtain cropRegion from the
centralized layout defaults instead so defaults don't drift. In the initializer
inside useEditorHistory replace the direct use/import of DEFAULT_CROP_REGION
with the value from the shared layout defaults API (e.g., call or import that
provides the default layout object and read its cropRegion) so the hook uses the
centralized default consistently (update any references to DEFAULT_CROP_REGION
in useEditorHistory to use the layoutDefaults.cropRegion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c14c159d-b24b-4e51-ab3c-168c8e30cdf0
📒 Files selected for processing (10)
src/components/video-editor/CropControl.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/editorDefaults.test.tssrc/components/video-editor/editorDefaults.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/types.tssrc/hooks/useEditorHistory.tssrc/lib/userPreferences.ts
siddharthvaddem
previously approved these changes
May 23, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 82-84: Remove the unused DEFAULT_WEBCAM_SIZE_PRESET symbol from
the import/destructuring in SettingsPanel (the list containing
MAX_PLAYBACK_SPEED, DEFAULT_WEBCAM_SIZE_PRESET, MAX_ZOOM_SCALE) so the file no
longer imports or references DEFAULT_WEBCAM_SIZE_PRESET; leave
MAX_PLAYBACK_SPEED and MAX_ZOOM_SCALE intact and ensure there are no other
references to DEFAULT_WEBCAM_SIZE_PRESET elsewhere in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78aa6c6b-57fd-44df-92a1-672201571586
📒 Files selected for processing (1)
src/components/video-editor/SettingsPanel.tsx
Removes MAX_PLAYBACK_SPEED and DEFAULT_WEBCAM_SIZE_PRESET (TS6133) and runs biome's organize-imports to satisfy the Lint check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR siddharthvaddem#600 (now on main) removed WEBCAM_TARGET_WIDTH/HEIGHT and switched this call site to width/height: 0 so the native helper picks the camera's native dimensions. Align this branch with that so CI's fresh PR-merge stops erroring on the undeclared identifiers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
siddharthvaddem
approved these changes
May 23, 2026
17 tasks
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
2 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
editorDefaultsmoduleWhy
Several editor defaults were repeated across UI state, props fallbacks, user preferences, and project persistence. That makes small behavior changes easy to miss in one layer. This PR keeps the behavior the same while making the default source clearer and easier to validate.
Validation
npm run lintnpm testnpm run build-vitenpm test -- editorDefaults projectPersistenceNote:
npm run i18n:checkcurrently fails onmainwith pre-existing locale drift unrelated to this PR; this refactor does not touch translations.Summary by CodeRabbit
Refactor
New Features
Tests