Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces comprehensive initialization state management to fix loading state issues and prevent race conditions in the FormeoEditor. The changes add explicit state tracking throughout the async initialization process, preserve form data during language changes, and provide new APIs for checking editor readiness.
Changes:
- Added initialization state tracking with five states (created, loading, initializing, ready, error)
- Implemented data locking mechanism to prevent race conditions during async initialization
- Refactored language change handling to preserve form data by refreshing only the UI
- Added new public APIs:
initState,isReady, andwhenReady()for managing initialization lifecycle
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/js/editor.js | Core initialization refactoring with state machine, data locking, and new readiness APIs |
| src/lib/js/components/index.js | Enhanced form data validation with fallback structure for missing properties |
| src/lib/js/editor.test.js | Unit tests for Components.load data handling and DEFAULT_FORMDATA behavior |
| tests/editor-initialization.spec.js | E2E tests covering initialization states, data persistence, and language changes |
| docs/editor/initialization.md | Comprehensive documentation of initialization lifecycle and best practices |
| docs/options/i18n/README.md | Documentation for runtime language changes with data preservation guarantees |
| docs/README.md | Updated main docs to reference new initialization lifecycle documentation |
Comments suppressed due to low confidence (1)
src/lib/js/editor.js:127
- The ERROR state is defined in INIT_STATES but is never set anywhere in the code. If loadResources fails (e.g., network error loading icons/CSS), the state remains stuck in LOADING_RESOURCES rather than transitioning to ERROR. This could cause whenReady() to hang indefinitely. Add error handling with try-catch to set the state to ERROR when resource loading fails.
async loadResources() {
document.removeEventListener('DOMContentLoaded', this.loadResources)
this.#initState = INIT_STATES.LOADING_RESOURCES
const promises = [
fetchIcons(this.opts.svgSprite),
fetchFormeoStyle(this.opts.style),
i18n.init({
preloaded: { 'en-US': enUS },
...this.opts.i18n,
locale: globalThis.sessionStorage?.getItem(SESSION_LOCALE_KEY),
}),
].filter(Boolean)
await Promise.all(promises)
if (this.opts.allowEdit) {
this.init()
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return { | ||
| id: cloned.id || DEFAULT_FORMDATA().id, | ||
| stages: cloned.stages || DEFAULT_FORMDATA().stages, | ||
| rows: cloned.rows || {}, | ||
| columns: cloned.columns || {}, | ||
| fields: cloned.fields || {}, | ||
| } |
There was a problem hiding this comment.
DEFAULT_FORMDATA() is called multiple times on lines 27 and 28 to provide fallback values for individual properties. This is inefficient since DEFAULT_FORMDATA() generates a new object with all properties each time. Consider calling it once and reusing the result to avoid redundant object creation.
| async whenReady() { | ||
| if (this.#initState === INIT_STATES.READY) { | ||
| return this | ||
| } | ||
| if (this.#initPromise) { | ||
| return this.#initPromise | ||
| } | ||
| // Fallback: poll for ready state | ||
| return new Promise(resolve => { | ||
| const checkReady = () => { | ||
| if (this.#initState === INIT_STATES.READY) { | ||
| resolve(this) | ||
| } else { | ||
| globalThis.requestAnimationFrame(checkReady) | ||
| } | ||
| } | ||
| checkReady() | ||
| }) | ||
| } |
There was a problem hiding this comment.
The whenReady fallback polling mechanism (lines 260-269) will never resolve if the editor enters the ERROR state, causing the Promise to hang indefinitely. The polling loop should also check for the ERROR state and reject the Promise in that case to allow callers to handle initialization failures.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| /** | ||
| * Helper to get the editor instance from the demo page | ||
| */ | ||
| const getEditor = () => window.frameworkLoader?.currentDemo?.editor | ||
|
|
There was a problem hiding this comment.
The helper function getEditor is defined but never used in any of the tests. All tests inline the same expression (window.frameworkLoader?.currentDemo?.editor) instead of using this helper. Either remove this unused helper or update the tests to use it consistently.
| /** | |
| * Helper to get the editor instance from the demo page | |
| */ | |
| const getEditor = () => window.frameworkLoader?.currentDemo?.editor |
| ```javascript | ||
| // This is safe - form data is preserved | ||
| await editor.whenReady() | ||
| editor.i18n.setLang('de-DE') |
There was a problem hiding this comment.
The documentation example shows using editor.i18n.setLang('de-DE') without await, but looking at the implementation, #setLanguage is an async function that returns a Promise. The documentation should show await editor.i18n.setLang('de-DE') to properly wait for the language change to complete before assuming form data remains intact.
| editor.i18n.setLang('de-DE') | |
| await editor.i18n.setLang('de-DE') |
| await editor.whenReady() | ||
|
|
||
| // Change to German | ||
| editor.i18n.setLang('de-DE') |
There was a problem hiding this comment.
The documentation example shows using editor.i18n.setLang('de-DE') without await, but the implementation shows #setLanguage is an async function. The documentation should demonstrate the correct usage with await to match the async nature of the method.
| editor.i18n.setLang('de-DE') | |
| await editor.i18n.setLang('de-DE') |
| set formData(data = {}) { | ||
| this.userFormData = cleanFormData(data) | ||
| const cleaned = cleanFormData(data) | ||
| this.#lockedFormData = cleaned |
There was a problem hiding this comment.
When formData is set via the setter (line 72-77), it updates #lockedFormData which is intended to preserve user-provided data during initialization. However, this could cause issues if formData is set after initialization but before the first data load completes. The #dataLoadedOnce flag might still be false, and #getDataWithPriority would use the newly locked data, potentially overwriting the user's intention. Consider checking the initialization state before updating #lockedFormData in the setter.
| this.#lockedFormData = cleaned | |
| // Only treat setter-provided data as "locked" initial data | |
| // if the editor is still in the CREATED state. This prevents | |
| // mid-initialization updates from interfering with the initial | |
| // data selection logic that relies on #lockedFormData. | |
| if (this.#initState === INIT_STATES.CREATED) { | |
| this.#lockedFormData = cleaned | |
| } |
|
@kevinchappell I've opened a new pull request, #425, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
This reverts commit 7e1436d.
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
This reverts commit bd78f23.
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
|
@kevinchappell I've opened a new pull request, #427, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
Co-authored-by: kevinchappell <1457540+kevinchappell@users.noreply.github.com>
|
🎉 This PR is included in version 5.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.