refactor(wizard): use computed signal for internal states#1855
refactor(wizard): use computed signal for internal states#1855
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the SiWizardComponent to utilize Angular signals for derived state, replacing several methods with computed signals that pre-calculate step properties such as states, icons, and ARIA attributes. While this aligns with signal-based state management, the canActivateSteps signal introduces an O(N²) complexity that should be optimized to O(N). Additionally, consolidating the multiple computed signals that iterate over the steps array into a single metadata signal would improve maintainability and reduce redundant allocations.
| protected readonly canActivateSteps = computed(() => { | ||
| const index = this._index(); | ||
| const steps = this.steps(); | ||
| return steps.map((_, stepIndex) => { |
There was a problem hiding this comment.
I am not sure if this is right approach since now there are 6 computed all of them loops through same steps() so whenever steps changes this would trigger 6 time steps.map in each computed.
spliffone
force-pushed
the
refactor/wizard-migrate-to-computed
branch
from
4820cba to
4d395a0
Compare
spliffone
force-pushed
the
refactor/wizard-migrate-to-computed
branch
from
a3d67f9 to
f386731
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the SiWizardComponent to use a computed signal for step metadata, optimizing performance by pre-calculating state, accessibility attributes, and icons for each step. This change replaces multiple individual method calls in the template with a unified metadata object. Feedback was provided to add a bounds check when accessing the metadata array in the next method to prevent potential runtime errors from out-of-bounds indices.
| const stepIndex = this.index + delta; | ||
| const nextStep = steps[stepIndex]; | ||
| if (this.canActivate(stepIndex)) { | ||
| if (this.stepsMetadata()[stepIndex].canActivate) { |
There was a problem hiding this comment.
The this.stepsMetadata()[stepIndex] access could potentially throw an error if stepIndex is out of bounds (e.g., if delta is large). While the current usage in the template is safe, adding a bounds check here would make the next method more robust as a public/protected API.
| if (this.stepsMetadata()[stepIndex].canActivate) { | |
| if (stepIndex < steps.length && this.stepsMetadata()[stepIndex].canActivate) { |
spliffone
force-pushed
the
refactor/wizard-migrate-to-computed
branch
from
f386731 to
fb43ccd
Compare
2 participants
Use computed signal to calculate and cache the internal wizard step states instead of function call which would be called in each check cycle.
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: