-
Notifications
You must be signed in to change notification settings - Fork 0
feat(editor-state): optimize compilation flow #221
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
Conversation
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.
Pull request overview
This PR optimizes the compilation flow in the editor state management by making several key improvements: moving interval cleanup closer to initialization, removing the disableAutoCompilation check from config compilation to allow configs to always compile, and switching config value application from conditional to always-reset behavior. Additionally, it adds a new standalone project example with pre-compiled WASM and updates related tests.
- Config compilation now always runs regardless of disableAutoCompilation flag
- Config values (disableAutoCompilation, memorySizeBytes) now reset to defaults when not provided
- Runtime interval cleanup moved to just before new interval creation for better timing
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/examples/projects/standaloneProject.ts | New example project with pre-compiled WASM demonstrating runtime-ready projects |
| src/examples/projects/simpleCounterMainThread.ts | Simplified scope syntax from two lines to one using bracket notation |
| src/examples/projects/index.ts | Added metadata for new standaloneProject example |
| packages/runtime-web-worker-midi/src/index.ts | Moved clearInterval calls closer to setInterval for better timing control |
| packages/runtime-web-worker-logic/src/index.ts | Moved clearInterval calls closer to setInterval with added spacing |
| packages/runtime-main-thread-logic/src/index.ts | Added clearInterval before setInterval to prevent duplicate intervals |
| packages/editor/packages/editor-state/src/impureHelpers/config/applyConfigToState.ts | Changed from conditional to always-reset behavior for config values using nullish coalescing |
| packages/editor/packages/editor-state/src/impureHelpers/config/applyConfigToState.test.ts | Updated test expectations to match new reset-to-default behavior |
| packages/editor/packages/editor-state/src/effects/disableCompilation.test.ts | Updated tests to reflect that config compilation now runs even when disableAutoCompilation is true |
| packages/editor/packages/editor-state/src/effects/config.ts | Removed disableAutoCompilation check from config compilation; renamed forceCompileConfig to rebuildConfig |
| packages/editor/packages/editor-state/src/effects/compiler.ts | Changed codeBuffer assignments to use store.set() for consistency |
| docs/todos/160-compiler-buffer-loop-strategy-config.md | New planning document for future buffer generation strategy improvements |
| docs/brainstorming_notes/archived/005-audio-buffer-cycle-length-configuration.md | Archived design document explaining buffer size configuration decisions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| '', | ||
| 'rescope disableAutoCompilation', | ||
| 'push true', | ||
| 'set ', |
Copilot
AI
Jan 4, 2026
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.
There is a trailing space after 'set' on this line. This should be removed for consistency and clean code formatting.
| 'set ', | |
| 'set', |
| }, | ||
| }; | ||
|
|
||
| export default standalonProject; |
Copilot
AI
Jan 4, 2026
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.
The export statement references 'standalonProject' but the variable is declared as 'standalonProject' on line 5. Both should use the correct spelling 'standaloneProject' with an 'e'.
| if (state.initialProjectState?.compiledWasm && state.initialProjectState.compiledModules) { | ||
| try { | ||
| state.compiler.codeBuffer = decodeBase64ToUint8Array(state.initialProjectState.compiledWasm); | ||
| state.compiler.compiledModules = state.initialProjectState.compiledModules; | ||
| store.set('compiler.codeBuffer', decodeBase64ToUint8Array(state.initialProjectState.compiledWasm)); | ||
| log(state, 'Pre-compiled WASM loaded and decoded successfully', 'Loader'); | ||
| } catch (err) { | ||
| state.compiler.codeBuffer = new Uint8Array(); | ||
| state.compiler.compiledModules = {}; | ||
| store.set('compiler.codeBuffer', new Uint8Array()); |
Copilot
AI
Jan 4, 2026
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.
Inconsistent state mutation approach: line 127 uses direct assignment (state.compiler.codeBuffer = new Uint8Array()) while lines 136 and 140 use store.set(). For consistency and to ensure state management works correctly (subscriptions, tracking, etc.), all state modifications should use store.set(). The same applies to lines 125-126, 128-131 which also use direct assignment.
|
|
||
| import type { Project } from '@8f4e/editor-state'; | ||
|
|
||
| const standalonProject: Project = { |
Copilot
AI
Jan 4, 2026
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.
The variable name 'standalonProject' is missing an 'e'. It should be 'standaloneProject' to match the file name and maintain consistency.
No description provided.