ACT-2120: Prevent double initialization of mvc controllers #19
Conversation
elbunuelo
commented
Aug 20, 2025
elbunuelo
commented
- Prevent double initialization by adding an initializing state and running the initialization code only when uninitialized
…ning the initialization code only when uninitialized
There was a problem hiding this comment.
Pull Request Overview
This PR prevents double initialization of MVC controllers by introducing a state machine with three states: uninitialized, initializing, and initialized. The change replaces the simple boolean initialized flag with a more robust state tracking system.
- Replaced boolean
initializedflag withInitializationStateenum - Added protection against re-initialization attempts during the initialization process
- Maintained backward compatibility with a getter for the
initializedproperty
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
| throw new Error( | ||
| 'Attempted to reinitialize controller before initialization finished.' | ||
| ); |
There was a problem hiding this comment.
Maybe just logging a warning would be more appropriate? Depends how this was happening I suppose but I worry throwing an error will cause any later/random other code to not be executed?
There was a problem hiding this comment.
I'm curious what would cause double initialization in a way that is prevented by this change, given internalInitialize is sync, so there's no async window where a double initialisation might happen? Is it that the controller's initialize method might synchronously call internalInitialize?
| @@ -138,8 +146,8 @@ class ApplicationController< | |||
|
|
|||
| this.state = store(cloneDeep(this.initialState)); | |||
| if (this.initialize) this.initialize(initialArgs); | |||
There was a problem hiding this comment.
I think this line is at least partially related to the problem. It's possible for the initialize method to throw an error, and I think in that case, we might get into a bad state.
But also, I think the problem is if initialize makes changes that cause a re-render during the initial render? Can/should we run the internalInitialize in a different way perhaps?
