-
Notifications
You must be signed in to change notification settings - Fork 6
Fix: ensure tools are configured before editor renders on note switch #355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
40f6f52
f47175c
46ac19f
06d665b
cf24fa8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "name": "@codexteam/ui", | ||
| "version": "0.2.3", | ||
| "version": "0.2.5", | ||
| "type": "module", | ||
| "sideEffects": [ | ||
| "*.css", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,13 @@ export const useNoteEditor = function useNoteEditor(options: UseNoteEditorOption | |
| */ | ||
| const toolsUserConfigLoaded = ref<boolean>(false); | ||
|
|
||
| /** | ||
| * Incremented on each new load request to discard stale async results. | ||
| * Prevents race conditions when rapid note switching causes multiple | ||
| * concurrent loadToolsScripts invocations. | ||
| */ | ||
| let currentLoadId = 0; | ||
|
|
||
| /** | ||
| * Combine note and user tools | ||
| * Undefined when user or note is not loaded | ||
|
|
@@ -102,10 +109,12 @@ export const useNoteEditor = function useNoteEditor(options: UseNoteEditorOption | |
| }); | ||
|
|
||
| /** | ||
| * Downloads passed tools scripts and toggles-on the isEditorReady flag | ||
| * Downloads passed tools scripts and returns the loaded config object. | ||
| * Does not mutate shared state — the caller is responsible for applying the result | ||
| * @param toolsConfigs - tools to download | ||
| * @returns loaded tools config | ||
| */ | ||
| async function loadToolsScripts(toolsConfigs: EditorTool[]): Promise<void> { | ||
| async function loadToolsScripts(toolsConfigs: EditorTool[]): Promise<Record<string, { class: EditorjsConfigTool; inlineToolbar: boolean }>> { | ||
| const loadedTools = await editorToolsService.getToolsLoaded(toolsConfigs); | ||
|
|
||
| /** | ||
|
|
@@ -114,7 +123,7 @@ export const useNoteEditor = function useNoteEditor(options: UseNoteEditorOption | |
| */ | ||
| const loadedToolsWithoutParagraph = loadedTools.filter(tool => tool.tool.name !== 'paragraph'); | ||
|
|
||
| toolsUserConfig = Object.fromEntries( | ||
| return Object.fromEntries( | ||
| loadedToolsWithoutParagraph | ||
| .map(toolClassAndInfo => [ | ||
| toolClassAndInfo.tool.name, | ||
|
|
@@ -124,12 +133,6 @@ export const useNoteEditor = function useNoteEditor(options: UseNoteEditorOption | |
| }, | ||
| ]) | ||
| ); | ||
| toolsUserConfigLoaded.value = true; | ||
|
|
||
| /** | ||
| * Now all tools are loaded, we're ready to use the editor | ||
| */ | ||
| isEditorReady.value = true; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -144,7 +147,28 @@ export const useNoteEditor = function useNoteEditor(options: UseNoteEditorOption | |
| return; | ||
| } | ||
|
|
||
| await loadToolsScripts(tools); | ||
| const loadId = ++currentLoadId; | ||
|
|
||
| isEditorReady.value = false; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| toolsUserConfigLoaded.value = false; | ||
|
|
||
| const loadedConfig = await loadToolsScripts(tools); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Might be worth wrapping this in a try/catch and surfacing an error on failure? Or maybe restoring some usable state? |
||
|
|
||
| /** | ||
| * If a newer load request has superseded this one — discard stale results | ||
| * to prevent overwriting state with tools from a previous note. | ||
| */ | ||
| if (loadId !== currentLoadId) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add test case so the race condition this is meant to fix doesn't regress silently? |
||
| return; | ||
| } | ||
|
|
||
| toolsUserConfig = loadedConfig; | ||
| toolsUserConfigLoaded.value = true; | ||
|
|
||
| /** | ||
| * Now all tools are loaded, we're ready to use the editor | ||
| */ | ||
| isEditorReady.value = true; | ||
| }, { | ||
| immediate: true, // load tools if they are passed to the composable immediately | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth extracting a shared type alias so
loadToolsScriptsandtoolsUserConfigcan't drift apart?