Make refreshing repositories more efficient (#16793)

* more efficient way to get repositories on refresh

* handle race condition

* copilot feedback

* error handling for refresh repo

---------

Co-authored-by: Mo Mesgin <mmesgin@Mos-M2-MacBook-Pro.local>

Author: momesgin

shell/assets/translations/en-us.yaml

@@ -1435,6 +1435,8 @@ catalog:
     refreshInterval:
       label: Refresh Interval
       placeholder: 'default: {hours}'
+    error:
+      refresh: Error refreshing repository
   tools:
     iconAlt: Icon for {app} Cluster Tool
     header: Cluster Tools

shell/models/__tests__/catalog.cattle.io.clusterrepo.test.ts

@@ -0,0 +1,84 @@
+import ClusterRepo from '../catalog.cattle.io.clusterrepo';
+
+describe('clusterRepo', () => {
+  let model: any;
+
+  beforeEach(() => {
+    model = new ClusterRepo({
+      metadata: { name: 'test-repo' },
+      spec:     { url: 'https://test-url.com' }
+    }, {
+      getters:     {},
+      dispatch:    jest.fn(),
+      rootGetters: {}
+    });
+
+    jest.spyOn(model, '_key', 'get').mockReturnValue('test-key');
+    jest.spyOn(model, 'save').mockImplementation().mockResolvedValue(true);
+    jest.spyOn(model, 'waitForState').mockImplementation().mockResolvedValue(true);
+    jest.spyOn(model, '$dispatch', 'get').mockReturnValue(jest.fn());
+  });
+
+  describe('refresh', () => {
+    it('updates forceUpdate, saves, waits for active state, and dispatches load by default', async() => {
+      // Mock Date to ensure deterministic forceUpdate value
+      const mockDate = new Date('2023-01-01T12:00:00.000Z');
+      const spy = jest.spyOn(global, 'Date').mockImplementation(() => mockDate as any);
+
+      await model.refresh();
+
+      expect(model.spec.forceUpdate).toBe('2023-01-01T12:00:00Z');
+      expect(model.save).toHaveBeenCalledWith();
+      expect(model.waitForState).toHaveBeenCalledWith('active', 10000, 1000);
+      expect(model.$dispatch).toHaveBeenCalledWith('catalog/load', { force: true, repoKeys: [model._key] }, { root: true });
+
+      spy.mockRestore();
+    });
+
+    it('updates forceUpdate, saves, waits for active state, but DOES NOT dispatch load if dispatchLoad is false', async() => {
+      await model.refresh(false);
+
+      expect(model.save).toHaveBeenCalledWith();
+      expect(model.waitForState).toHaveBeenCalledWith('active', 10000, 1000);
+      expect(model.$dispatch).not.toHaveBeenCalled();
+    });
+
+    it('dispatches error to growl if save or waitForState fails', async() => {
+      const error = new Error('waitForState timeout');
+
+      model.waitForState.mockRejectedValue(error);
+      jest.spyOn(model, 't', 'get').mockReturnValue(jest.fn().mockReturnValue('Error refreshing repository'));
+      jest.spyOn(model, 'nameDisplay', 'get').mockReturnValue('Test Repo');
+
+      await model.refresh();
+
+      expect(model.$dispatch).toHaveBeenCalledWith('growl/fromError', {
+        title: 'Error refreshing repository',
+        err:   error
+      }, { root: true });
+    });
+  });
+
+  describe('refreshBulk', () => {
+    it('calls refresh(false) on all items and then dispatches a single catalog/load with all repoKeys', async() => {
+      const mockItem1 = {
+        _key:    'repo-1',
+        refresh: jest.fn().mockResolvedValue(true)
+      };
+      const mockItem2 = {
+        _key:    'repo-2',
+        refresh: jest.fn().mockResolvedValue(true)
+      };
+
+      await model.refreshBulk([mockItem1, mockItem2]);
+
+      expect(mockItem1.refresh).toHaveBeenCalledWith(false);
+      expect(mockItem2.refresh).toHaveBeenCalledWith(false);
+
+      expect(model.$dispatch).toHaveBeenCalledWith('catalog/load', {
+        force:    true,
+        repoKeys: ['repo-1', 'repo-2']
+      }, { root: true });
+    });
+  });
+});

shell/models/catalog.cattle.io.clusterrepo.js

@@ -41,26 +41,54 @@ export default class ClusterRepo extends SteveModel {
       });
 
       insertAt(out, 0, {
-        action:   'refresh',
-        label:    this.t('action.refresh'),
-        icon:     'icon icon-refresh',
-        enabled:  !!this.links.update,
-        bulkable: true,
+        action:     'refresh',
+        label:      this.t('action.refresh'),
+        icon:       'icon icon-refresh',
+        enabled:    !!this.links.update,
+        bulkable:   true,
+        bulkAction: 'refreshBulk',
       });
     }
 
     return out;
   }
 
-  async refresh() {
-    const now = (new Date()).toISOString().replace(/\.\d+Z$/, 'Z');
+  /**
+   * Refreshes the repository by updating its forceUpdate annotation and waiting for it to become active.
+   * @param {boolean} dispatchLoad - Whether to dispatch the catalog/load action after refreshing. Defaults to true.
+   */
+  async refresh(dispatchLoad = true) {
+    try {
+      const now = (new Date()).toISOString().replace(/\.\d+Z$/, 'Z');
 
-    this.spec.forceUpdate = now;
-    await this.save();
+      this.spec.forceUpdate = now;
+      await this.save();
+
+      await this.waitForState('active', 10000, 1000);
 
-    await this.waitForState('active', 10000, 1000);
+      if (dispatchLoad) {
+        this.$dispatch('catalog/load', { force: true, repoKeys: [this._key] }, { root: true });
+      }
+    } catch (err) {
+      this.$dispatch('growl/fromError', {
+        title: this.t('catalog.repo.error.refresh', {}, 'Error refreshing repository'),
+        err,
+      }, { root: true });
+    }
+  }
 
-    this.$dispatch('catalog/load', { force: true, reset: true }, { root: true });
+  /**
+   * Performs a bulk refresh on multiple repositories concurrently, bypassing individual
+   * catalog loads, and dispatches a single catalog/load for all repositories once they are active.
+   * @param {ClusterRepo[]} items - Array of repository instances to refresh.
+   */
+  async refreshBulk(items) {
+    await Promise.allSettled(items.map((item) => item.refresh(false)));
+
+    this.$dispatch('catalog/load', {
+      force:    true,
+      repoKeys: items.map((item) => item._key)
+    }, { root: true });
   }
 
   async disableClusterRepo() {

shell/store/__tests__/catalog.test.ts

@@ -205,6 +205,102 @@ describe('catalog', () => {
       // Version info should be changed (it's now empty given reset)
       expect(store.state[catalogStoreName].versionInfos).toStrictEqual({ });
     });
+
+    it('repoKeys provided', async() => {
+      const store = createStore(constructStore());
+
+      // Validate initial state of store
+      expect(store.getters[`${ catalogStoreName }/rawCharts`]).toStrictEqual(initialRawCharts);
+      expect(store.getters[`${ catalogStoreName }/charts`]).toStrictEqual([]);
+
+      // Make the request targeting specific repo (we provide repo._key)
+      await store.dispatch(`${ catalogStoreName }/load`, {
+        force:    true,
+        repoKeys: [repo._key]
+      });
+
+      const rawCharts = store.getters[`${ catalogStoreName }/rawCharts`];
+      const charts = store.getters[`${ catalogStoreName }/charts`];
+
+      // We expect the old chart belonging to this repo to be wiped out
+      // and replaced entirely by the newly fetched chart.
+      expect(rawCharts[initialRawChart.id]).not.toBeDefined();
+      expect(rawCharts[expectedChartKey]).toBeDefined();
+      expect(rawCharts[expectedChartKey].id).toBe(expectedChartKey);
+      expect(rawCharts[expectedChartKey].versions[0].version).toBe(repoChart.version);
+
+      expect(charts).toHaveLength(1);
+      expect(charts[0]).toBeDefined();
+      expect(charts[0].id).toBe(expectedChartKey);
+    });
+
+    it('repoKeys provided, ignoring unrelated charts and versions', async() => {
+      // We will manually inject an unrelated chart to prove it isn't wiped out
+      const store = createStore(constructStore());
+      const unrelatedChart = {
+        id:       `namespace/unrelatedRepo/unrelatedChart`,
+        repoKey:  'unrelatedRepo',
+        type:     'namespaced',
+        name:     'unrelatedChart',
+        version:  1,
+        metadata: { name: 'unrelatedChart' }
+      };
+
+      const targetedVersionKey = `namespace/testRepo/myChart/1.0.0`;
+      const unrelatedVersionKey = `namespace/unrelatedRepo/unrelatedChart/1.0.0`;
+
+      store.state[catalogStoreName].charts = {
+        ...initialRawCharts,
+        [unrelatedChart.id]: unrelatedChart
+      };
+
+      store.state[catalogStoreName].versionInfos = {
+        [targetedVersionKey]:  { data: 'old-data' },
+        [unrelatedVersionKey]: { data: 'keep-me' }
+      };
+
+      // Make the request targeting specific repo
+      await store.dispatch(`${ catalogStoreName }/load`, {
+        force:    true,
+        repoKeys: [repo._key]
+      });
+
+      const rawCharts = store.getters[`${ catalogStoreName }/rawCharts`];
+      const versionInfos = store.state[catalogStoreName].versionInfos;
+
+      // The unrelated chart should NOT be wiped
+      expect(rawCharts[unrelatedChart.id]).toBeDefined();
+      expect(rawCharts[unrelatedChart.id].repoKey).toBe('unrelatedRepo');
+
+      // The old initialRawChart (with repoKey: repo._key) SHOULD be wiped and replaced
+      expect(rawCharts[initialRawChart.id]).not.toBeDefined();
+      expect(rawCharts[expectedChartKey]).toBeDefined();
+
+      // The targeted version info SHOULD be wiped
+      expect(versionInfos[targetedVersionKey]).not.toBeDefined();
+      // The unrelated version info should NOT be wiped
+      expect(versionInfos[unrelatedVersionKey]).toBeDefined();
+      expect(versionInfos[unrelatedVersionKey].data).toBe('keep-me');
+    });
+  });
+
+  describe('refresh', () => {
+    it('calls refresh(false) on all repos and then dispatches a reset load', async() => {
+      const mockRepo1 = { refresh: jest.fn().mockResolvedValue(true) };
+      const mockRepo2 = { refresh: jest.fn().mockResolvedValue(true) };
+
+      const getters = { repos: [mockRepo1, mockRepo2] };
+      const dispatch = jest.fn().mockResolvedValue(true);
+      const commit = jest.fn();
+
+      await actions.refresh({
+        getters, commit, dispatch
+      });
+
+      expect(mockRepo1.refresh).toHaveBeenCalledWith(false);
+      expect(mockRepo2.refresh).toHaveBeenCalledWith(false);
+      expect(dispatch).toHaveBeenCalledWith('load', { force: true, reset: true });
+    });
   });
 
   describe('filterAndArrangeCharts', () => {

shell/store/catalog.js

@@ -325,8 +325,12 @@ export const actions = {
    * force: Always refresh catalog's helm repo by re-fetching index.yaml
    *
    * reset: clear existing charts and version cache
+   *
+   * repoKeys: Optional array of specific repo keys (IDs) to refresh. When provided, only these specific
+   * repos will be fetched, and only their existing charts will be cleared from the cache to avoid
+   * duplicate chart entries or wiping out unrelated chart data.
    */
-  async load(ctx, { force, reset } = {}) {
+  async load(ctx, { force, reset, repoKeys = [] } = {}) {
     const {
       state, getters, rootGetters, commit, dispatch
     } = ctx;
@@ -360,14 +364,34 @@ export const actions = {
     promises = {};
 
     for ( const repo of repos ) {
-      if ( (force === true || !getters.isLoaded(repo)) && repo.canLoad ) {
+      let shouldLoad = false;
+
+      if (repoKeys.length) {
+        // If repoKeys are explicitly provided (e.g. refreshing a single repo from the UI),
+        // we ONLY want to load the repos in that array. We intentionally ignore `!getters.isLoaded(repo)`
+        // here so we don't accidentally fetch other unrelated repos just because they haven't loaded yet.
+        shouldLoad = repoKeys.includes(repo._key);
+      } else {
+        // Default behavior: load if explicitly forced, OR if the repo hasn't been loaded into state yet.
+        shouldLoad = force === true || !getters.isLoaded(repo);
+      }
+
+      if ( shouldLoad && repo.canLoad ) {
         console.info('Loading index for repo', repo.name, `(${ repo._key })`); // eslint-disable-line no-console
         promises[repo._key] = repo.followLink('index');
       }
     }
 
     const res = await allHashSettled(promises);
-    const charts = reset ? {} : state.charts;
+    const charts = reset ? {} : { ...state.charts };
+    let versionInfos = null;
+
+    if (reset) {
+      versionInfos = {};
+    } else if (repoKeys.length) {
+      versionInfos = { ...state.versionInfos };
+    }
+
     const errors = [];
 
     for ( const key of Object.keys(res) ) {
@@ -379,6 +403,28 @@ export const actions = {
         continue;
       }
 
+      // We are targeting specific repos. To prevent duplicate chart versions from appearing,
+      // we must remove the old charts for this specific repo before appending the newly fetched ones,
+      // but ONLY if the fetch was successful.
+      if (repoKeys.length && repoKeys.includes(key)) {
+        for (const chartKey in charts) {
+          if (charts[chartKey].repoKey === key) {
+            delete charts[chartKey];
+          }
+        }
+
+        // Also clear out cached version info for this repo so we don't display stale READMEs/values
+        const repoType = repo.type === CATALOG.CLUSTER_REPO ? 'cluster' : 'namespace';
+        const repoName = repo.metadata.name;
+        const versionPrefix = `${ repoType }/${ repoName }/`;
+
+        for (const versionKey in versionInfos) {
+          if (versionKey.startsWith(versionPrefix)) {
+            delete versionInfos[versionKey];
+          }
+        }
+      }
+
       for ( const k in obj.value.entries ) {
         for ( const entry of obj.value.entries[k] ) {
           addChart(ctx, charts, entry, repo);
@@ -394,13 +440,17 @@ export const actions = {
       loaded,
     });
 
-    if (reset) {
-      commit('setVersions', {});
+    if (versionInfos) {
+      commit('setVersions', versionInfos);
     }
   },
 
+  /**
+   * Globally refreshes all loaded repositories by triggering their refresh actions concurrently,
+   * bypassing individual catalog loads, and then performs a single, global catalog/load.
+   */
   async refresh({ getters, commit, dispatch }) {
-    const promises = getters.repos.map((x) => x.refresh());
+    const promises = getters.repos.map((x) => x.refresh(false));
 
     // @TODO wait for repo state to indicate they're done once the API has that