Open
Conversation
Adds a migration to migrate theme data. Note that for __customFontAssets this is not sufficient as this will only apply when the editor is opened. A user could upgrade a site without opening the editor at all. Adds versioning to themes The first migration simply removes __customFontPreloads as these have been replaced with __customFontAssets which are dynamically generated on theme load and change.
jwartofsky-yext
requested review from
asanehisa,
benlife5,
briantstephan and
mkilpatrick
auto-screenshot-update: true
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 32 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (17)
WalkthroughThe PR adds a theme migration system and applies it across the visual editor. A new migrateTheme utility, version key, and migration registry are introduced (first migration removes legacy __customFontPreloads). Editor now migrates theme data separately from layout data and passes migrated theme/layout to ThemeEditor and LayoutEditor. ThemeEditor and LayoutEditor apply migrateTheme when initializing theme histories (including dev-mode localStorage flows). migrateTheme is exported and covered by unit tests. Sequence Diagram(s)sequenceDiagram
participant Editor
participant migrateTheme
participant migrateLayout (migrationRegistry)
participant ThemeEditor
participant LayoutEditor
Editor->>migrateLayout: migrate(layoutData, migrationRegistry)
migrateLayout-->>Editor: migratedLayoutData
Editor->>migrateTheme: migrateTheme(themeData)
migrateTheme-->>Editor: migratedThemeData
Editor->>ThemeEditor: themeData = migratedThemeData, layoutData = migratedLayoutData
Editor->>LayoutEditor: themeData = migratedThemeData, layoutData = migratedLayoutData
sequenceDiagram
participant ThemeEditor
participant LocalStorage
participant migrateTheme
participant DevServer
ThemeEditor->>LocalStorage: read theme history (dev mode)
LocalStorage-->>ThemeEditor: themeHistory[]
ThemeEditor->>migrateTheme: migrateTheme(each history.data)
migrateTheme-->>ThemeEditor: migrated history entries
ThemeEditor->>DevServer: sendDevThemeSaveStateData(migrated latest)
ThemeEditor-->>ThemeEditor: updateThemeInEditor(migrated latest)
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/visual-editor/src/utils/migrateTheme.test.ts (1)
21-31: Add a forward-version regression test.Please add coverage for a payload where
[THEME_VERSION_KEY]is greater than the local registry length, so we explicitly guard against version rollback behavior.🧪 Suggested test case
+ it("does not downgrade a theme stamped by a newer editor", () => { + const migratedTheme = migrateTheme({ + "--fontFamily-body-fontFamily": "'Inter', sans-serif", + [THEME_VERSION_KEY]: 99, + }); + + expect(migratedTheme).toEqual({ + "--fontFamily-body-fontFamily": "'Inter', sans-serif", + [THEME_VERSION_KEY]: 99, + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/utils/migrateTheme.test.ts` around lines 21 - 31, Add a regression test for migrateTheme to cover forward-version payloads: create a test that calls migrateTheme with a theme object containing [THEME_VERSION_KEY] set to a value greater than the local migration registry length and assert that migrateTheme returns the theme unchanged (i.e., it should only stamp current version and must not attempt to rollback or apply older migrations). Reference the migrateTheme function and THEME_VERSION_KEY constant when adding this test so it explicitly verifies behavior when incoming version > registry length.packages/visual-editor/src/internal/themeMigrations/migrationRegistry.ts (1)
1-1: Consider usingimport typefor clarity, though not required by current configuration.
ThemeMigrationRegistryis a type-only export, and while TypeScript will automatically elide the import at compile time with the current configuration (which does not haveverbatimModuleSyntaxorpreserveValueImportsenabled), explicitly marking it asimport typeclarifies the intent and follows best practice.Suggested improvement
-import { ThemeMigrationRegistry } from "../../utils/migrateTheme.ts"; +import type { ThemeMigrationRegistry } from "../../utils/migrateTheme.ts";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/internal/themeMigrations/migrationRegistry.ts` at line 1, The import in migrationRegistry.ts currently pulls a type-only symbol as a value import; change the statement importing ThemeMigrationRegistry from "../../utils/migrateTheme.ts" to an explicit type-only import (use "import type { ThemeMigrationRegistry } from ...") so the intent is clear and the compiler/tree-shaker knows it's only a type; update the import line that references ThemeMigrationRegistry accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/visual-editor/src/utils/migrateTheme.ts`:
- Around line 22-42: The current migration sets [THEME_VERSION_KEY] to
migrationRegistry.length which can downgrade a theme saved with a higher
version; update the final assignment so the saved version is never reduced:
compute the incoming numeric version (version), run migrations from
migrationRegistry.slice(version) into migratedThemeValues as you do, and when
returning set [THEME_VERSION_KEY] to Math.max(version, migrationRegistry.length)
(or simply keep the existing higher version) so THEME_VERSION_KEY is never
lowered; use the existing symbols version, migrationRegistry,
migratedThemeValues and THEME_VERSION_KEY to implement this change.
---
Nitpick comments:
In `@packages/visual-editor/src/internal/themeMigrations/migrationRegistry.ts`:
- Line 1: The import in migrationRegistry.ts currently pulls a type-only symbol
as a value import; change the statement importing ThemeMigrationRegistry from
"../../utils/migrateTheme.ts" to an explicit type-only import (use "import type
{ ThemeMigrationRegistry } from ...") so the intent is clear and the
compiler/tree-shaker knows it's only a type; update the import line that
references ThemeMigrationRegistry accordingly.
In `@packages/visual-editor/src/utils/migrateTheme.test.ts`:
- Around line 21-31: Add a regression test for migrateTheme to cover
forward-version payloads: create a test that calls migrateTheme with a theme
object containing [THEME_VERSION_KEY] set to a value greater than the local
migration registry length and assert that migrateTheme returns the theme
unchanged (i.e., it should only stamp current version and must not attempt to
rollback or apply older migrations). Reference the migrateTheme function and
THEME_VERSION_KEY constant when adding this test so it explicitly verifies
behavior when incoming version > registry length.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4f4d7324-3d2a-4686-aa9c-90df5820c6f1
📒 Files selected for processing (8)
packages/visual-editor/src/editor/Editor.tsxpackages/visual-editor/src/internal/components/LayoutEditor.tsxpackages/visual-editor/src/internal/components/ThemeEditor.tsxpackages/visual-editor/src/internal/themeMigrations/0001_remove_custom_font_preloads.tspackages/visual-editor/src/internal/themeMigrations/migrationRegistry.tspackages/visual-editor/src/utils/index.tspackages/visual-editor/src/utils/migrateTheme.test.tspackages/visual-editor/src/utils/migrateTheme.ts
auto-screenshot-update: true
auto-screenshot-update: true
auto-screenshot-update: true
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
1 participant
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.
Adds a migration to migrate theme data
Adds versioning to themes
The first migration simply removes __customFontPreloads as these have been replaced with __customFontAssets which are dynamically generated on theme load and change.
Refactors the code for loading themes to match the pattern used for Layouts
migrate is renamed to migrateLayout. migrate still works, but is marked deprecated