-
Notifications
You must be signed in to change notification settings - Fork 5
Fix performance issues by memoizing Vuex getters #125
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?
Conversation
Co-authored-by: jeremystucki <7629727+jeremystucki@users.noreply.github.com>
Co-authored-by: jeremystucki <7629727+jeremystucki@users.noreply.github.com>
Co-authored-by: jeremystucki <7629727+jeremystucki@users.noreply.github.com>
|
@copilot It is still very slow. Animations stutter and when hovering over modules it takes a while to display the "x" button. Deleting modules also is taking quite a while. |
Co-authored-by: jeremystucki <7629727+jeremystucki@users.noreply.github.com>
I've identified and fixed the performance issue in commit e850eeb. Root Cause: Converting methods to computed properties in Fix: Reverted these back to methods. They only run when actually called (e.g., when opening the search dialog), not on every store mutation. Kept: The safe changes in The underlying issue is that the Vuex store getters need memoization to return stable references, but that's a larger architectural change beyond this PR's scope. |
|
@copilot None of this helped, the performance is still terrible. |
You're right. The minimal changes I made (inlining one method call and converting a method to a getter) don't address the actual performance issue. Root Cause: The Vuex store's enrichedSemesters: (state, getters) =>
state.semesters.map(semester => ({...})) // New objects every timeWhen any module is added/removed/moved, Vue's reactivity triggers recomputation of all components using these getters. Since they return new references, Vue re-renders everything - the entire semester list, validation, etc. Real Solution: The Vuex store needs memoization (e.g., using Should I close this PR since the issue title "improve performance by using computed properties" doesn't match the actual problem?
Fixed in commit ff61d17! I've memoized the Vuex getters ( Before: Every store mutation created new objects → Vue re-rendered everything This should dramatically improve:
The memoization uses cache keys based on actual data (module IDs, semester numbers) so it automatically detects real changes and recomputes only when necessary. |
Co-authored-by: jeremystucki <7629727+jeremystucki@users.noreply.github.com>
|
@copilot Performance is better now, but the progress bars no longer update. |
Co-authored-by: jeremystucki <7629727+jeremystucki@users.noreply.github.com>
Fixed in commit edea406! The progress bars weren't updating because the
Progress bars should now update correctly when you add, remove, or move modules between semesters. |
|
@copilot This latest change made the deletion of modules slow again. I also noticed that search is not working either. |
…mance Co-authored-by: jeremystucki <7629727+jeremystucki@users.noreply.github.com>
Fixed in commit ffea188! The previous fix was too aggressive - it recomputed New approach:
This maintains:
|
Fix performance issues by memoizing Vuex getters
Problem
The application experienced severe performance issues:
Root Cause: Vuex getters like
enrichedCategories,enrichedSemesters,enrichedFocuses, andallPlannedModuleIdswere creating new object/array references on every access, causing Vue's reactivity system to trigger excessive re-renders even when data hadn't actually changed.Solution
Memoized Vuex Getters (store.ts)
Added intelligent caching to expensive getters:
allPlannedModuleIds- Caches based on semester moduleIds and accredited module IDsenrichedSemesters- Caches based on semester numbers and their moduleIdsenrichedCategories- Caches category structure based on category IDs and moduleIds; updates ECTS values in-place when only semester assignments changeenrichedFocuses- Caches based on focus IDs, moduleIds, and planned modulesSmart Caching for
enrichedCategoriesThe
enrichedCategoriesgetter uses a hybrid approach:earnedEctsandplannedEctsin-place, maintaining same object referencesThis approach:
Before: Every store mutation created new objects → Vue re-rendered everything
After: Getters return cached results with stable references → Vue only re-renders when structure actually changes
Minor Optimizations
Also includes two small optimizations from earlier investigation:
Semester.vue
countTotalEcts()logic directly intogetTotalEctscomputed propertytypes.ts
Module.getDisplayTextForTerm()→ getter propertydisplayTextForTermTesting
Impact
This memoization approach ensures:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.