Skip to content

fix: resolve performance issues with O(n*m) lookups and redundant computations#129

Merged
jeremystucki merged 10 commits into
mainfrom
claude/fix-performance-issues-KIJSz
May 26, 2026
Merged

fix: resolve performance issues with O(n*m) lookups and redundant computations#129
jeremystucki merged 10 commits into
mainfrom
claude/fix-performance-issues-KIJSz

Conversation

@jeremystucki

@jeremystucki jeremystucki commented Mar 7, 2026

Copy link
Copy Markdown
Member

Summary

Replaces O(n) array lookups with O(1) Map/Set equivalents across the store and components, eliminates redundant computations, and fixes a pre-existing null-safety bug.

Store (src/helpers/store.ts)

  • Added moduleMap getter — a Map<string, Module> keyed by ID, replacing O(n) Array.find calls throughout
  • Added allPlannedModuleIdsSet getter — a Set<string> of all planned module IDs, replacing O(n) Array.includes calls throughout
  • Removed unused modulesByIds getter
  • Fixed allPlannedModuleIds filter to use a type-guard (.filter((id): id is string => !!id)) so the array and derived Set are truly string[]
  • enrichedSemesters, enrichedCategories, enrichedFocuses: use moduleMap.get() for O(1) lookups and use the Vuex state parameter instead of accessing store.state directly
  • enrichedFocuses: fixed null-safety bug — successorModuleId can be null/undefined, now guarded before being passed to Set.has()
  • getPlannedEcts / getEarnedEcts: accept enrichedSemesters as a parameter instead of re-reading it from store.getters, avoiding redundant recomputation

Components

  • GraphModule.vue: recommendedModules and dependentModules use moduleMap.get() and allPlannedModuleIdsSet.has()
  • GraphModuleHightlight.vue, ModuleSearchListItem.vue: isPlanned / moduleIsInPlan use allPlannedModuleIdsSet.has()
  • ModuleSearch.vue:
    • categoryFilterData, ectsFilterData, semesterFilterData moved from methods to computed properties to enable Vue caching
    • semesterFilterData backed by a module-level constant (data never changes)
    • ectsFilterData deduplication changed from O(n²) indexOf to O(n) new Set()
    • isOneModuleAvailable converted from a deep-watched data property to a computed using some()
    • Three sequential module filter passes (ECTS, semester, query) collapsed into one
    • modulesInGroupIds uses a Set for O(1) membership check

Composable / View

  • useGraphView.ts: computeLayout builds a local Set from allPlannedModuleIds for the filter instead of O(n) Array.includes
  • Home.vue: replaced setTimeout(() => {}, 0) with this.$nextTick() for correct Vue timing semantics

…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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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 moduleMap and allPlannedModuleIdsSet Vuex getters and refactored multiple derived getters to use them.
  • Updated several components/composables to use Set.has() / Map.get() instead of includes() / find().
  • Refactored ModuleSearch filtering 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.

Comment thread src/helpers/store.ts Outdated
Comment thread src/helpers/store.ts Outdated
Comment thread src/helpers/store.ts
Comment thread src/helpers/store.ts Outdated
Comment thread src/helpers/store.ts Outdated
Comment thread src/helpers/store.ts Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI commented Mar 7, 2026

Copy link
Copy Markdown

@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>
- Remove unused modulesByIds getter
- Use state parameter in enrichedFocuses instead of store.state directly
- Add proper type guard to allPlannedModuleIds filter
- Fix ectsFilterData deduplication to use Set instead of indexOf (O(n²))
- Extract semesterFilterData to module-level constant

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (2)

src/helpers/store.ts:59

  • In enrichedSemesters, semester.moduleIds.map(id => map.get(id)).filter(f => f) removes undefined at runtime but doesn’t narrow the type, even though Semester.modules is declared as Module[]. Prefer a type-guard filter ((m): m is Module => !!m) to keep the getter output consistent with the model types.
    enrichedSemesters: (state, getters) => {
      const map = getters.moduleMap as Map<string, Module>;
      return state.semesters.map(semester => ({
        ...semester,
        modules: semester.moduleIds.map(id => map.get(id)).filter(f => f),
      }));

src/helpers/store.ts:70

  • In enrichedCategories, modules: category.moduleIds.map(id => map.get(id)).filter(f => f) has the same issue as enrichedSemesters: runtime filtering doesn’t type-narrow to Module[] even though Category.modules is declared as Module[]. Use a type-guard filter to keep types consistent and avoid downstream casts.
    enrichedCategories: (state, getters) => {
      const map = getters.moduleMap as Map<string, Module>;
      const enrichedSemesters = getters.enrichedSemesters;
      return state.categories.map(category => ({
        ...category,
        earnedEcts: getEarnedEcts(category, enrichedSemesters),
        plannedEcts: getPlannedEcts(category, enrichedSemesters),
        colorClass: getColorClassForCategoryId(category.id),
        modules: category.moduleIds.map(id => map.get(id)).filter(f => f),
      }));

Comment thread src/helpers/store.ts Outdated
Comment thread src/helpers/store.ts Outdated
Comment thread src/helpers/store.ts Outdated
Comment thread src/components/ModuleSearch.vue Outdated
…terData constant

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

src/helpers/store.ts:66

  • enrichedCategories sets modules with .map(...).filter(f => f), which doesn’t type-narrow and can leave modules typed as (Module | undefined)[]. Use a type-guard filter ((m): m is Module => !!m) to keep the getter’s return shape consistent with Category.modules: Module[] and avoid the need for later type assertions.
        ...category,
        earnedEcts: getEarnedEcts(category, enrichedSemesters),
        plannedEcts: getPlannedEcts(category, enrichedSemesters),
        colorClass: getColorClassForCategoryId(category.id),
        modules: category.moduleIds.map(id => map.get(id)).filter(f => f),
      }));

src/helpers/store.ts:74

  • allModulesForFocus is created with .filter(f => f) as Module[]. The as Module[] assertion is hiding the fact that the filter isn’t a type guard. Replace it with a proper type-guard filter so this is actually Module[] without an unsafe cast.
      const numberOfModulesRequiredToGetFocus = 8;
      return state.focuses.map(focus => {
        const allModulesForFocus = focus.moduleIds.map(id => map.get(id)).filter(f => f) as Module[];
        return {

src/helpers/store.ts:92

  • availableModules finishes with .map(id => map.get(id)).filter(f => f), which again isn’t a type-guard and leaves the resulting array typed as (Module | undefined)[]. Use a type-guard filter ((m): m is Module => !!m) so availableModules is unambiguously Module[].
            })
            .map(id => map.get(id))
            .filter(f => f),

Comment thread src/helpers/store.ts
Comment thread src/helpers/store.ts
jeremystucki and others added 4 commits May 25, 2026 09:59
…helpers from store.getters, use allPlannedModuleIdsSet in composable

- enrichedFocuses: replace allModulesForFocus.find() with map.get() (O(n²) → O(1))
- getEarnedEcts/getPlannedEcts: pass startSemester and accreditedModules as params instead of reading store.getters directly, making the functions fully decoupled from global state
- useGraphView: use allPlannedModuleIdsSet store getter instead of constructing a local Set on every computeLayout call
- ModuleSearch: move semesterFilterData from computed to data since it returns a module-level constant with no reactive dependencies

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Module[]

Replace .filter(f => f) with .filter((m): m is Module => !!m) in
enrichedSemesters, enrichedCategories, and enrichedFocuses (allModulesForFocus
and availableModules). Also removes the now-unnecessary `as Module[]` cast on
allModulesForFocus since the type guard makes it redundant.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@jeremystucki jeremystucki requested a review from Copilot May 25, 2026 08:08
@jeremystucki jeremystucki marked this pull request as ready for review May 25, 2026 08:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

@jeremystucki jeremystucki requested a review from Untersander May 25, 2026 08:12

@Untersander Untersander left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested it and it is more performant.

@jeremystucki jeremystucki merged commit 1afa656 into main May 26, 2026
5 checks passed
@jeremystucki jeremystucki deleted the claude/fix-performance-issues-KIJSz branch May 26, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants