fix: resolve performance issues with O(n*m) lookups and redundant computations#129
fix: resolve performance issues with O(n*m) lookups and redundant computations#129jeremystucki wants to merge 3 commits intomainfrom
Conversation
…putations - Add moduleMap getter (Map<id, Module>) for O(1) module lookups, replacing O(n) .find() calls - Add allPlannedModuleIdsSet getter (Set) for O(1) membership checks, replacing O(n) .includes() - Pass enrichedSemesters as parameter to getEarnedEcts/getPlannedEcts to avoid redundant getter calls - Replace deep watch on groupedModules with a simple computed property - Convert filter data methods to computed properties to leverage Vue's caching - Combine 3 sequential filter passes into single pass in ModuleSearch - Replace setTimeout(0) with $nextTick in Home.vue route watcher https://claude.ai/code/session_01QVdw6xYjwtg3SujE4WpExq
There was a problem hiding this comment.
Pull request overview
This PR targets performance bottlenecks caused by repeated linear lookups (Array.find / Array.includes) and redundant recomputation, primarily by introducing Map/Set-backed access patterns and consolidating filtering logic.
Changes:
- Added
moduleMapandallPlannedModuleIdsSetVuex getters and refactored multiple derived getters to use them. - Updated several components/composables to use
Set.has()/Map.get()instead ofincludes()/find(). - Refactored
ModuleSearchfiltering and moved filter data builders to computed properties to reduce redundant work.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/views/Home.vue | Uses $nextTick instead of setTimeout(…, 0) before reading plan data from URL after route changes. |
| src/helpers/store.ts | Introduces moduleMap/allPlannedModuleIdsSet, updates enriched getters and ECTS computations to reuse enriched semesters. |
| src/composables/useGraphView.ts | Uses a Set for planned-module membership checks during layout computation. |
| src/components/ModuleSearchListItem.vue | Switches plan-membership check from includes to Set.has. |
| src/components/ModuleSearch.vue | Avoids calling filter-data builders from the template, simplifies filtering, and uses a Set for “modules in groups” checks. |
| src/components/GraphModuleHightlight.vue | Switches planned-state check from includes to Set.has. |
| src/components/GraphModule.vue | Switches recommended/dependent module lookups to Map.get and planned checks to Set.has. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| modulesByIds: (_state, getters) => (moduleIds: string[]) => { | ||
| const map = getters.moduleMap as Map<string, Module>; | ||
| return moduleIds.map(id => map.get(id)).filter(f => f); | ||
| }, |
There was a problem hiding this comment.
modulesByIds returns moduleIds.map(...).filter(f => f), which does not type-narrow and can leave this getter typed as (Module | undefined)[] in TS. Consider using a type-guard filter ((m): m is Module => !!m) and/or remove this getter if it’s no longer used anywhere in the codebase (it appears unused outside this file).
| allPlannedModuleIds: state => state.semesters | ||
| .flatMap(semester => semester.moduleIds) | ||
| .concat(state.accreditedModules.map(m => m.moduleId)) | ||
| .filter(id => id), | ||
| allPlannedModuleIdsSet: (_state, getters) => { | ||
| return new Set<string>(getters.allPlannedModuleIds); | ||
| }, |
There was a problem hiding this comment.
allPlannedModuleIdsSet is constructed from getters.allPlannedModuleIds, but that getter concatenates accreditedModules.map(m => m.moduleId) where moduleId can be undefined. Since .filter(id => id) doesn’t type-narrow, this can leak undefined into the Set and forces casts elsewhere. Consider using a type-guard filter so allPlannedModuleIds (and therefore the Set) is truly string[].
src/helpers/store.ts
Outdated
| enrichedCategories: (_state, getters) => { | ||
| const state = store.state; | ||
| const map = getters.moduleMap as Map<string, Module>; | ||
| const enrichedSemesters = getters.enrichedSemesters; |
There was a problem hiding this comment.
enrichedCategories also pulls const state = store.state instead of using the Vuex state argument. This ties the getter to the global store instance and can cause surprises if the store is replaced or mocked; use the passed-in state for consistency and cleaner reactivity tracking.
| enrichedFocuses: (_state, getters) => { | ||
| const state = store.state; | ||
| const plannedSet = getters.allPlannedModuleIdsSet as Set<string>; | ||
| const map = getters.moduleMap as Map<string, Module>; |
There was a problem hiding this comment.
enrichedFocuses uses const state = store.state rather than the Vuex state argument. Keeping getters dependent on their state/getters parameters avoids coupling to a specific store instance and is easier to test and reason about.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@jeremystucki I've opened a new pull request, #130, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.